| Message ID | 20230524091722.522118-9-jiawenwu@trustnetic.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Friday, May 26, 2023 12:14 PM, Jakub Kicinski wrote: > On Wed, 24 May 2023 17:17:21 +0800 Jiawen Wu wrote: > > + ret = devm_mdiobus_register(&pdev->dev, mii_bus); > > + if (ret) > > + return ret; > > + > > + mdiodev = mdio_device_create(mii_bus, 0); > > + if (IS_ERR(mdiodev)) > > + return PTR_ERR(mdiodev); > > + > > + xpcs = xpcs_create(mdiodev, PHY_INTERFACE_MODE_10GBASER); > > + if (IS_ERR(xpcs)) { > > + mdio_device_free(mdiodev); > > + return PTR_ERR(xpcs); > > + } > > How does the mdiodev get destroyed in case of success? > Seems like either freeing it in case of xpcs error is unnecessary > or it needs to also be freed when xpcs is destroyed? When xpcs is destroyed, that means mdiodev is no longer needed. I think there is no need to free mdiodev in case of xpcs error, since devm_* function leads to free it.
On Fri, May 26, 2023 at 02:21:23PM +0800, Jiawen Wu wrote: > On Friday, May 26, 2023 12:14 PM, Jakub Kicinski wrote: > > On Wed, 24 May 2023 17:17:21 +0800 Jiawen Wu wrote: > > > + ret = devm_mdiobus_register(&pdev->dev, mii_bus); > > > + if (ret) > > > + return ret; > > > + > > > + mdiodev = mdio_device_create(mii_bus, 0); > > > + if (IS_ERR(mdiodev)) > > > + return PTR_ERR(mdiodev); > > > + > > > + xpcs = xpcs_create(mdiodev, PHY_INTERFACE_MODE_10GBASER); > > > + if (IS_ERR(xpcs)) { > > > + mdio_device_free(mdiodev); > > > + return PTR_ERR(xpcs); > > > + } > > > > How does the mdiodev get destroyed in case of success? > > Seems like either freeing it in case of xpcs error is unnecessary > > or it needs to also be freed when xpcs is destroyed? > > When xpcs is destroyed, that means mdiodev is no longer needed. > I think there is no need to free mdiodev in case of xpcs error, > since devm_* function leads to free it. If you are relying on the devm-ness of devm_mdiobus_register() then it won't. Although mdiobus_unregister() walks bus->mdio_map[], I think you are assuming that the mdio device you've created in mdio_device_create() will be in that array. MDIO devices only get added to that array when mdiobus_register_device() has been called, which must only be called from mdio_device_register(). Please arrange to call mdio_device_free() prior to destroying the XPCS in every case.
On Fri, May 26, 2023 at 05:01:49PM +0800, Jiawen Wu wrote: > On Friday, May 26, 2023 4:43 PM, Russell King (Oracle) wrote: > > On Fri, May 26, 2023 at 02:21:23PM +0800, Jiawen Wu wrote: > > > On Friday, May 26, 2023 12:14 PM, Jakub Kicinski wrote: > > > > On Wed, 24 May 2023 17:17:21 +0800 Jiawen Wu wrote: > > > > > + ret = devm_mdiobus_register(&pdev->dev, mii_bus); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + mdiodev = mdio_device_create(mii_bus, 0); > > > > > + if (IS_ERR(mdiodev)) > > > > > + return PTR_ERR(mdiodev); > > > > > + > > > > > + xpcs = xpcs_create(mdiodev, PHY_INTERFACE_MODE_10GBASER); > > > > > + if (IS_ERR(xpcs)) { > > > > > + mdio_device_free(mdiodev); > > > > > + return PTR_ERR(xpcs); > > > > > + } > > > > > > > > How does the mdiodev get destroyed in case of success? > > > > Seems like either freeing it in case of xpcs error is unnecessary > > > > or it needs to also be freed when xpcs is destroyed? > > > > > > When xpcs is destroyed, that means mdiodev is no longer needed. > > > I think there is no need to free mdiodev in case of xpcs error, > > > since devm_* function leads to free it. > > > > If you are relying on the devm-ness of devm_mdiobus_register() then > > it won't. Although mdiobus_unregister() walks bus->mdio_map[], I > > think you are assuming that the mdio device you've created in > > mdio_device_create() will be in that array. MDIO devices only get > > added to that array when mdiobus_register_device() has been called, > > which must only be called from mdio_device_register(). > > > > Please arrange to call mdio_device_free() prior to destroying the > > XPCS in every case. > > Get it. It seems this is becoming a pattern, so I think we need to solve it differently. How about something like this, which means you only have to care about calling xpcs_create_mdiodev() and xpcs_destroy() ? diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c index b87c69c4cdd7..802222581feb 100644 --- a/drivers/net/pcs/pcs-xpcs.c +++ b/drivers/net/pcs/pcs-xpcs.c @@ -1240,6 +1240,7 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev, if (!xpcs) return ERR_PTR(-ENOMEM); + mdio_device_get(mdiodev); xpcs->mdiodev = mdiodev; xpcs_id = xpcs_get_id(xpcs); @@ -1272,6 +1273,7 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev, ret = -ENODEV; out: + mdio_device_put(mdiodev); kfree(xpcs); return ERR_PTR(ret); @@ -1280,8 +1282,33 @@ EXPORT_SYMBOL_GPL(xpcs_create); void xpcs_destroy(struct dw_xpcs *xpcs) { + mdio_device_put(mdiodev); kfree(xpcs); } EXPORT_SYMBOL_GPL(xpcs_destroy); +struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr, + phy_interface_t interface) +{ + struct mdio_device *mdiodev; + struct dw_xpcs *xpcs; + + mdiodev = mdio_device_create(bus, addr); + if (IS_ERR(mdiodev)) + return ERR_CAST(mdiodev); + + xpcs = xpcs_create(mdiodev, interface); + + /* xpcs_create() has taken a refcount on the mdiodev if it was + * successful. If xpcs_create() fails, this will free the mdio + * device here. In any case, we don't need to hold our reference + * anymore, and putting it here will allow mdio_device_put() in + * xpcs_destroy() to automatically free the mdio device. + */ + mdio_device_put(mdiodev); + + return xpcs; +} +EXPORT_SYMBOL_GPL(xpcs_create_mdiodev); + MODULE_LICENSE("GPL v2"); diff --git a/include/linux/mdio.h b/include/linux/mdio.h index 1d7d550bbf1a..537b62330c90 100644 --- a/include/linux/mdio.h +++ b/include/linux/mdio.h @@ -108,6 +108,16 @@ int mdio_driver_register(struct mdio_driver *drv); void mdio_driver_unregister(struct mdio_driver *drv); int mdio_device_bus_match(struct device *dev, struct device_driver *drv); +static inline void mdio_device_get(struct mdio_device *mdiodev) +{ + get_device(&mdiodev->dev); +} + +static inline void mdio_device_put(struct mdio_device *mdiodev) +{ + mdio_device_free(mdiodev); +} + static inline bool mdio_phy_id_is_c45(int phy_id) { return (phy_id & MDIO_PHY_ID_C45) && !(phy_id & ~MDIO_PHY_ID_C45_MASK);
On Fri, May 26, 2023 at 10:37:04AM +0100, Russell King (Oracle) wrote: > I'm just creating a patch series for both xpcs and lynx, which this > morning have had patches identifying similar problems with creation > and destruction. https://lore.kernel.org/all/ZHCGZ8IgAAwr8bla@shell.armlinux.org.uk/
On Friday, May 26, 2023 6:42 PM, Russell King (Oracle) wrote: > On Fri, May 26, 2023 at 10:37:04AM +0100, Russell King (Oracle) wrote: > > I'm just creating a patch series for both xpcs and lynx, which this > > morning have had patches identifying similar problems with creation > > and destruction. > > https://lore.kernel.org/all/ZHCGZ8IgAAwr8bla@shell.armlinux.org.uk/ OK, I will send the updated patches after this patch series merged.
diff --git a/drivers/net/ethernet/wangxun/Kconfig b/drivers/net/ethernet/wangxun/Kconfig index 73f4492928c0..f3fb273e6fd0 100644 --- a/drivers/net/ethernet/wangxun/Kconfig +++ b/drivers/net/ethernet/wangxun/Kconfig @@ -45,6 +45,7 @@ config TXGBE select GPIOLIB select REGMAP select COMMON_CLK + select PCS_XPCS select LIBWX select SFP help diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c index 6aba22a4fc5e..d2a6f8ca78e7 100644 --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c @@ -8,6 +8,8 @@ #include <linux/clk-provider.h> #include <linux/clkdev.h> #include <linux/regmap.h> +#include <linux/pcs/pcs-xpcs.h> +#include <linux/mdio.h> #include <linux/i2c.h> #include <linux/pci.h> @@ -77,6 +79,88 @@ static int txgbe_swnodes_register(struct txgbe *txgbe) return software_node_register_node_group(nodes->group); } +static int txgbe_pcs_read(struct mii_bus *bus, int addr, int devnum, int regnum) +{ + struct wx *wx = bus->priv; + u32 offset, val; + + if (addr) + return -EOPNOTSUPP; + + offset = devnum << 16 | regnum; + + /* Set the LAN port indicator to IDA_ADDR */ + wr32(wx, TXGBE_XPCS_IDA_ADDR, offset); + + /* Read the data from IDA_DATA register */ + val = rd32(wx, TXGBE_XPCS_IDA_DATA); + + return (u16)val; +} + +static int txgbe_pcs_write(struct mii_bus *bus, int addr, int devnum, int regnum, u16 val) +{ + struct wx *wx = bus->priv; + u32 offset; + + if (addr) + return -EOPNOTSUPP; + + offset = devnum << 16 | regnum; + + /* Set the LAN port indicator to IDA_ADDR */ + wr32(wx, TXGBE_XPCS_IDA_ADDR, offset); + + /* Write the data to IDA_DATA register */ + wr32(wx, TXGBE_XPCS_IDA_DATA, val); + + return 0; +} + +static int txgbe_mdio_pcs_init(struct txgbe *txgbe) +{ + struct mdio_device *mdiodev; + struct mii_bus *mii_bus; + struct dw_xpcs *xpcs; + struct pci_dev *pdev; + struct wx *wx; + int ret = 0; + + wx = txgbe->wx; + pdev = wx->pdev; + + mii_bus = devm_mdiobus_alloc(&pdev->dev); + if (!mii_bus) + return -ENOMEM; + + mii_bus->name = "txgbe_pcs_mdio_bus"; + mii_bus->read_c45 = &txgbe_pcs_read; + mii_bus->write_c45 = &txgbe_pcs_write; + mii_bus->parent = &pdev->dev; + mii_bus->phy_mask = ~0; + mii_bus->priv = wx; + snprintf(mii_bus->id, MII_BUS_ID_SIZE, "txgbe_pcs-%x", + (pdev->bus->number << 8) | pdev->devfn); + + ret = devm_mdiobus_register(&pdev->dev, mii_bus); + if (ret) + return ret; + + mdiodev = mdio_device_create(mii_bus, 0); + if (IS_ERR(mdiodev)) + return PTR_ERR(mdiodev); + + xpcs = xpcs_create(mdiodev, PHY_INTERFACE_MODE_10GBASER); + if (IS_ERR(xpcs)) { + mdio_device_free(mdiodev); + return PTR_ERR(xpcs); + } + + txgbe->xpcs = xpcs; + + return 0; +} + static int txgbe_gpio_get(struct gpio_chip *chip, unsigned int offset) { struct wx *wx = gpiochip_get_data(chip); @@ -429,16 +513,22 @@ int txgbe_init_phy(struct txgbe *txgbe) return ret; } + ret = txgbe_mdio_pcs_init(txgbe); + if (ret) { + wx_err(txgbe->wx, "failed to init mdio pcs: %d\n", ret); + goto err_unregister_swnode; + } + ret = txgbe_gpio_init(txgbe); if (ret) { wx_err(txgbe->wx, "failed to init gpio\n"); - goto err_unregister_swnode; + goto err_destroy_xpcs; } ret = txgbe_clock_register(txgbe); if (ret) { wx_err(txgbe->wx, "failed to register clock: %d\n", ret); - goto err_unregister_swnode; + goto err_destroy_xpcs; } ret = txgbe_i2c_register(txgbe); @@ -460,6 +550,8 @@ int txgbe_init_phy(struct txgbe *txgbe) err_unregister_clk: clkdev_drop(txgbe->clock); clk_unregister(txgbe->clk); +err_destroy_xpcs: + xpcs_destroy(txgbe->xpcs); err_unregister_swnode: software_node_unregister_node_group(txgbe->nodes.group); @@ -472,5 +564,6 @@ void txgbe_remove_phy(struct txgbe *txgbe) platform_device_unregister(txgbe->i2c_dev); clkdev_drop(txgbe->clock); clk_unregister(txgbe->clk); + xpcs_destroy(txgbe->xpcs); software_node_unregister_node_group(txgbe->nodes.group); } diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h index 6c903e4517c7..d1f62f62c28c 100644 --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h @@ -80,6 +80,10 @@ /* I2C registers */ #define TXGBE_I2C_BASE 0x14900 +/************************************** ETH PHY ******************************/ +#define TXGBE_XPCS_IDA_ADDR 0x13000 +#define TXGBE_XPCS_IDA_DATA 0x13004 + /* Part Number String Length */ #define TXGBE_PBANUM_LENGTH 32 @@ -171,6 +175,7 @@ struct txgbe_nodes { struct txgbe { struct wx *wx; struct txgbe_nodes nodes; + struct dw_xpcs *xpcs; struct platform_device *sfp_dev; struct platform_device *i2c_dev; struct clk_lookup *clock;
Register MDIO bus for PCS layer to use Synopsys designware XPCS, support 10GBASE-R interface to the controller. Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> --- drivers/net/ethernet/wangxun/Kconfig | 1 + .../net/ethernet/wangxun/txgbe/txgbe_phy.c | 97 ++++++++++++++++++- .../net/ethernet/wangxun/txgbe/txgbe_type.h | 5 + 3 files changed, 101 insertions(+), 2 deletions(-)