| Message ID | 20230524091722.522118-1-jiawenwu@trustnetic.com |
|---|---|
| Headers | show |
| Series | TXGBE PHYLINK support | expand |
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?
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 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 05:22:29PM +0800, Jiawen Wu wrote: > On Friday, May 26, 2023 5:07 PM, Russell King (Oracle) wrote: > > 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); > > Looks great, it can eliminate to create mdiodev in the ethernet driver, this device > only be used in xpcs. 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.
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 Wed, May 24, 2023 at 05:17:15PM +0800, Jiawen Wu wrote: > Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate > with SFP. > > Introduce the property "wx,i2c-snps-model" to match device data for Wangxun > in software node case. Since IO resource was mapped on the ethernet driver, > add a model quirk to get regmap from parent device. > > The exists IP limitations are dealt as workarounds: > - IP does not support interrupt mode, it works on polling mode. > - Additionally set FIFO depth address the chip issue. Looks better, thank you! My comments below. ... > + if (device_property_present(&pdev->dev, "wx,i2c-snps-model")) Assuming people are fine with this, I have no objection on the name. > + dev->flags |= MODEL_WANGXUN_SP; You probably has to clear the model in dev_flags, but here still a question which one should have a priority.
On Wed, May 24, 2023 at 05:17:17PM +0800, Jiawen Wu wrote: > Register the platform device to use Designware I2C bus master driver. > Use regmap to read/write I2C device region from given base offset. ... > +#include <linux/platform_device.h> Can this be ordered (to some extent), please? > #include <linux/gpio/property.h> > #include <linux/clk-provider.h> > #include <linux/clkdev.h> > +#include <linux/regmap.h> This too. > #include <linux/i2c.h> > #include <linux/pci.h> Somewhere here... ... Otherwise looks good, thank you.