| Message ID | 20231205103559.9605-7-fancer.lancer@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: pcs: xpcs: Add memory-based management iface support | expand |
On Tue, Dec 05, 2023 at 01:35:27PM +0300, Serge Semin wrote: > If the DW XPCS MDIO devices are either left unmasked for being auto-probed > or explicitly registered in the MDIO subsystem by means of the > mdiobus_register_board_info() method there is no point in creating the > dummy MDIO device instance in order to get the DW XPCS handler since the > MDIO core subsystem will create the device during the MDIO bus > registration procedure. Please reword this overly long sentence. If they're left unmasked, what prevents them being created as PHY devices? > @@ -1437,19 +1435,21 @@ struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr, > struct mdio_device *mdiodev; > struct dw_xpcs *xpcs; > > - mdiodev = mdio_device_create(bus, addr); > - if (IS_ERR(mdiodev)) > - return ERR_CAST(mdiodev); > + if (addr >= PHY_MAX_ADDR) > + return ERR_PTR(-EINVAL); > > - xpcs = xpcs_create(mdiodev, interface); > + if (mdiobus_is_registered_device(bus, addr)) { > + mdiodev = bus->mdio_map[addr]; > + mdio_device_get(mdiodev); No, this makes no sense now. This function is called xpcs_create_mdiodev() - note the "create_mdiodev" part. If it's getting the mdiodev from what is already there then it isn't creating it, so it's no longer doing what it says in its function name. If you want to add this functionality, create a new function to do it.
On Tue, Dec 05, 2023 at 10:49:47AM +0000, Russell King (Oracle) wrote: > On Tue, Dec 05, 2023 at 01:35:27PM +0300, Serge Semin wrote: > > If the DW XPCS MDIO devices are either left unmasked for being auto-probed > > or explicitly registered in the MDIO subsystem by means of the > > mdiobus_register_board_info() method there is no point in creating the > > dummy MDIO device instance in order to get the DW XPCS handler since the > > MDIO core subsystem will create the device during the MDIO bus > > registration procedure. > > Please reword this overly long sentence. Ok. > > If they're left unmasked, what prevents them being created as PHY > devices? Not sure I fully get what you meant. If they are left unmasked the MDIO-device descriptor will be created by the MDIO subsystem anyway. What the point in creating another one? > > > @@ -1437,19 +1435,21 @@ struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr, > > struct mdio_device *mdiodev; > > struct dw_xpcs *xpcs; > > > > - mdiodev = mdio_device_create(bus, addr); > > - if (IS_ERR(mdiodev)) > > - return ERR_CAST(mdiodev); > > + if (addr >= PHY_MAX_ADDR) > > + return ERR_PTR(-EINVAL); > > > > - xpcs = xpcs_create(mdiodev, interface); > > + if (mdiobus_is_registered_device(bus, addr)) { > > + mdiodev = bus->mdio_map[addr]; > > + mdio_device_get(mdiodev); > > No, this makes no sense now. This function is called > xpcs_create_mdiodev() - note the "create_mdiodev" part. If it's getting > the mdiodev from what is already there then it isn't creating it, so > it's no longer doing what it says in its function name. If you want to > add this functionality, create a new function to do it. AFAICS the method semantics is a bit different. It's responsibility is to create the DW XPCS descriptor. MDIO-device is utilized internally by the DW XPCS driver. The function callers don't access the created MDIO device directly (at least since some recent commit). So AFAIU "create" means creating the XPCS descriptor irrespective from the internal communication layer. So IMO the suffix is a bit misleading. I'll change it in one of the next commit anyway. Should I just merge that patch back in this one? -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Dec 05, 2023 at 01:35:27PM +0300, Serge Semin wrote: > If the DW XPCS MDIO devices are either left unmasked for being auto-probed > or explicitly registered in the MDIO subsystem by means of the > mdiobus_register_board_info() method mdiobus_register_board_info() has exactly one caller, and that is dsa_loop. I don't understand the relevance of it w.r.t. Synopsys XPCS. I'm reading the patches in order from the beginning. > there is no point in creating the dummy MDIO device instance in order Why dummy? There's nothing dummy about the mdio_device. It's how the PCS code accesses the hardware. > to get the DW XPCS handler since the MDIO core subsystem will create > the device during the MDIO bus registration procedure. It won't, though? Unless someone is using mdiobus_register_board_info() possibly, but who does that? > All what needs to be done is to just reuse the MDIO-device instance > available in the mii_bus.mdio_map array (using some getter for it > would look better though). It shall prevent the XPCS devices been > accessed over several MDIO-device instances. > > Note since the MDIO-device instance might be retrieved from the MDIO-bus > map array its reference counter shall be increased. If the MDIO-device > instance is created in the xpcs_create_mdiodev() method its reference > counter will be already increased. So there is no point in toggling the > reference counter in the xpcs_create() function. Just drop it from there. > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > --- Sorry, because the commit log lost me at the "context presentation" stage, I failed to understand the "what"s and the "why"s. Are you basically trying to add xpcs support on top of an mdio_device where the mdio_device_create() call was made externally to the xpcs code, through mdiobus_register_board_info() and mdiobus_setup_mdiodev_from_board_info()?
On Tue, Dec 05, 2023 at 02:31:41PM +0300, Serge Semin wrote: > On Tue, Dec 05, 2023 at 10:49:47AM +0000, Russell King (Oracle) wrote: > > On Tue, Dec 05, 2023 at 01:35:27PM +0300, Serge Semin wrote: > > > If the DW XPCS MDIO devices are either left unmasked for being auto-probed > > > or explicitly registered in the MDIO subsystem by means of the > > > mdiobus_register_board_info() method there is no point in creating the > > > dummy MDIO device instance in order to get the DW XPCS handler since the > > > MDIO core subsystem will create the device during the MDIO bus > > > registration procedure. > > > > > Please reword this overly long sentence. > > Ok. > > > > > If they're left unmasked, what prevents them being created as PHY > > devices? > > Not sure I fully get what you meant. If they are left unmasked the > MDIO-device descriptor will be created by the MDIO subsystem anyway. > What the point in creating another one? The MDIO bus scan looks for devices on the MDIO bus by probing each address. If it finds a response, it creates a PHY device (struct phy_device), and stores a pointer to the mdiodev embedded in this structure in the array. This device then gets registered as a PHY device, and becomes available for use by phylib and PHY drivers. This is something that needs to be avoided, but I don't see anything in your series that achieves that. > > No, this makes no sense now. This function is called > > xpcs_create_mdiodev() - note the "create_mdiodev" part. If it's getting > > the mdiodev from what is already there then it isn't creating it, so > > it's no longer doing what it says in its function name. If you want to > > add this functionality, create a new function to do it. > > AFAICS the method semantics is a bit different. It's responsibility is to > create the DW XPCS descriptor. MDIO-device is utilized internally by > the DW XPCS driver. The function callers don't access the created MDIO > device directly (at least since some recent commit). So AFAIU "create" > means creating the XPCS descriptor irrespective from the internal > communication layer. So IMO the suffix is a bit misleading. I'll > change it in one of the next commit anyway. Should I just merge that > patch back in this one? This function was created (by me) to also create the mdiodev. The function for use with a pre-existing mdiodev was xpcs_create(). But what do I know, I was only the author of this function, and of course you're correct. I don't like this patch anyway. Moving the mdio_device_get() etc out of xpcs_create() is wrong. Even if you get a mdiodev from some other place, then having xpcs_create() take a reference on it is still the correct thing to do. My conclusion is you don't understand refcounting.
On Tue, Dec 05, 2023 at 01:35:27PM +0300, Serge Semin wrote: > If the DW XPCS MDIO devices are either left unmasked for being auto-probed > or explicitly registered in the MDIO subsystem by means of the > mdiobus_register_board_info() method there is no point in creating the > dummy MDIO device instance in order to get the DW XPCS handler since the > MDIO core subsystem will create the device during the MDIO bus > registration procedure. All what needs to be done is to just reuse the > MDIO-device instance available in the mii_bus.mdio_map array (using some > getter for it would look better though). It shall prevent the XPCS devices > been accessed over several MDIO-device instances. > > Note since the MDIO-device instance might be retrieved from the MDIO-bus > map array its reference counter shall be increased. If the MDIO-device > instance is created in the xpcs_create_mdiodev() method its reference > counter will be already increased. So there is no point in toggling the > reference counter in the xpcs_create() function. Just drop it from there. > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > --- > drivers/net/pcs/pcs-xpcs.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > index 2850122f354a..a53376472394 100644 > --- a/drivers/net/pcs/pcs-xpcs.c > +++ b/drivers/net/pcs/pcs-xpcs.c > @@ -1376,7 +1376,6 @@ static 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); > @@ -1417,7 +1416,6 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev, > ret = -ENODEV; > > out: > - mdio_device_put(mdiodev); > kfree(xpcs); > > return ERR_PTR(ret); The above two hunks are a completely Unnecessary change. > @@ -1437,19 +1435,21 @@ struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr, > struct mdio_device *mdiodev; > struct dw_xpcs *xpcs; > > - mdiodev = mdio_device_create(bus, addr); > - if (IS_ERR(mdiodev)) > - return ERR_CAST(mdiodev); > + if (addr >= PHY_MAX_ADDR) > + return ERR_PTR(-EINVAL); > > - xpcs = xpcs_create(mdiodev, interface); > + if (mdiobus_is_registered_device(bus, addr)) { > + mdiodev = bus->mdio_map[addr]; > + mdio_device_get(mdiodev); This is fine - taking a reference on the mdiodev you've got from somewhere else is the right thing to do. > + } else { > + mdiodev = mdio_device_create(bus, addr); > + if (IS_ERR(mdiodev)) > + return ERR_CAST(mdiodev); > + } > > - /* 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); > + xpcs = xpcs_create(mdiodev, interface); > + if (IS_ERR(xpcs)) > + mdio_device_put(mdiodev); Without the change to xpcs_create() you don't need this change - and this is why I say you don't understand refcounting. The point here is that the refcounting management is in each function where references are gained or lost. xpcs_create() creates a new reference to the mdiodev by storing it in the dw_xpcs structure. Therefore, it takes a reference to the mdiodev. If something fails, it drops that reference to restore the refcount as it was on function entry. xpcs_create_mdiodev() as it originally stood creates the mdiodev from the bus/address, and then passes that to xpcs_create(). Once xpcs_create() has finished its work (irrespective of whether it was successful or not) we're done with the mdiodev in this function, so the reference is _always_ put. For your use case, it would be: mdiodev = bus->mdio_map[addr]; mdio_device_get(mdiodev); xpcs = xpcs_create(mdiodev, interface); mdio_device_put(mdiodev); return xpcs; which illustrates this point - we get a reference to the mdiodev by reading it from the array. We do something (calling xpcs_create) with it. If that something was successful, it takes its own refcount otherwise leaves it as-is. We're then done with the mdiodev so we drop the refcount we took. There is no need to make the code more complicated by changing this, so I regard the refcount changes in this patch to be wrong.
On Tue, Dec 05, 2023 at 02:31:41PM +0300, Serge Semin wrote: > On Tue, Dec 05, 2023 at 10:49:47AM +0000, Russell King (Oracle) wrote: > > On Tue, Dec 05, 2023 at 01:35:27PM +0300, Serge Semin wrote: > > > If the DW XPCS MDIO devices are either left unmasked for being auto-probed > > > or explicitly registered in the MDIO subsystem by means of the > > > mdiobus_register_board_info() method there is no point in creating the > > > dummy MDIO device instance in order to get the DW XPCS handler since the > > > MDIO core subsystem will create the device during the MDIO bus > > > registration procedure. > > > > > Please reword this overly long sentence. > > Ok. > > > > > If they're left unmasked, what prevents them being created as PHY > > devices? > > Not sure I fully get what you meant. If they are left unmasked the > MDIO-device descriptor will be created by the MDIO subsystem anyway. > What the point in creating another one? Saying what Russell said, in a different way: /* * Return true if the child node is for a phy. It must either: * o Compatible string of "ethernet-phy-idX.X" * o Compatible string of "ethernet-phy-ieee802.3-c45" * o Compatible string of "ethernet-phy-ieee802.3-c22" * o In the white list above (and issue a warning) * o No compatibility string * * A device which is not a phy is expected to have a compatible string * indicating what sort of device it is. */ bool of_mdiobus_child_is_phy(struct device_node *child) So when walking the bus, if a node is found which fits these criteria, its assumed to be a PHY. Anything on the MDIO bus which is not a PHY needs to use a compatible. Andrew
On Tue, Dec 05, 2023 at 02:52:24PM +0100, Andrew Lunn wrote: > On Tue, Dec 05, 2023 at 02:31:41PM +0300, Serge Semin wrote: > > On Tue, Dec 05, 2023 at 10:49:47AM +0000, Russell King (Oracle) wrote: > > > On Tue, Dec 05, 2023 at 01:35:27PM +0300, Serge Semin wrote: > > > > If the DW XPCS MDIO devices are either left unmasked for being auto-probed > > > > or explicitly registered in the MDIO subsystem by means of the > > > > mdiobus_register_board_info() method there is no point in creating the > > > > dummy MDIO device instance in order to get the DW XPCS handler since the > > > > MDIO core subsystem will create the device during the MDIO bus > > > > registration procedure. > > > > > > > > Please reword this overly long sentence. > > > > Ok. > > > > > > > > If they're left unmasked, what prevents them being created as PHY > > > devices? > > > > Not sure I fully get what you meant. If they are left unmasked the > > MDIO-device descriptor will be created by the MDIO subsystem anyway. > > What the point in creating another one? > > Saying what Russell said, in a different way: > > /* > * Return true if the child node is for a phy. It must either: > * o Compatible string of "ethernet-phy-idX.X" > * o Compatible string of "ethernet-phy-ieee802.3-c45" > * o Compatible string of "ethernet-phy-ieee802.3-c22" > * o In the white list above (and issue a warning) > * o No compatibility string > * > * A device which is not a phy is expected to have a compatible string > * indicating what sort of device it is. > */ > bool of_mdiobus_child_is_phy(struct device_node *child) > > So when walking the bus, if a node is found which fits these criteria, > its assumed to be a PHY. > > Anything on the MDIO bus which is not a PHY needs to use a compatible. Right. I'd actually forgotten about the firmware-based walking, and was thinking more of the non-firmware bus scanning as the commit message was talking about being _unmasked_ and the only mask we have is bus->phy_mask. It seems to me that this is yet another case of a really confusing commit message making review harder than it needs to be.
On Tue, Dec 05, 2023 at 01:46:44PM +0000, Russell King (Oracle) wrote: > For your use case, it would be: > > mdiodev = bus->mdio_map[addr]; By the way, this should be: mdiodev = mdiobus_find_device(bus, addr); if (!mdiodev) return ERR_PTR(-ENODEV); to avoid a layering violation. At some point, we should implement mdiobus_get_mdiodev() which also deals with the refcount.
Hi Andrew, Russell Sorry for the delay with response. I had to refresh my understanding of the series since it was created sometime ago and I already managed to forget some of its aspects (particularly regarding the MDIO-bus PHY-mask semantics). On Tue, Dec 05, 2023 at 02:50:58PM +0000, Russell King (Oracle) wrote: > On Tue, Dec 05, 2023 at 02:52:24PM +0100, Andrew Lunn wrote: > > On Tue, Dec 05, 2023 at 02:31:41PM +0300, Serge Semin wrote: > > > On Tue, Dec 05, 2023 at 10:49:47AM +0000, Russell King (Oracle) wrote: > > > > On Tue, Dec 05, 2023 at 01:35:27PM +0300, Serge Semin wrote: > > > > > If the DW XPCS MDIO devices are either left unmasked for being auto-probed > > > > > or explicitly registered in the MDIO subsystem by means of the > > > > > mdiobus_register_board_info() method there is no point in creating the > > > > > dummy MDIO device instance in order to get the DW XPCS handler since the > > > > > MDIO core subsystem will create the device during the MDIO bus > > > > > registration procedure. > > > > > > > > > > > Please reword this overly long sentence. > > > > > > Ok. > > > > > > > > > > > If they're left unmasked, what prevents them being created as PHY > > > > devices? > > > > > > Not sure I fully get what you meant. If they are left unmasked the > > > MDIO-device descriptor will be created by the MDIO subsystem anyway. > > > What the point in creating another one? > > > > Saying what Russell said, in a different way: > > > > /* > > * Return true if the child node is for a phy. It must either: > > * o Compatible string of "ethernet-phy-idX.X" > > * o Compatible string of "ethernet-phy-ieee802.3-c45" > > * o Compatible string of "ethernet-phy-ieee802.3-c22" > > * o In the white list above (and issue a warning) > > * o No compatibility string > > * > > * A device which is not a phy is expected to have a compatible string > > * indicating what sort of device it is. > > */ > > bool of_mdiobus_child_is_phy(struct device_node *child) > > > > So when walking the bus, if a node is found which fits these criteria, > > its assumed to be a PHY. > > > > Anything on the MDIO bus which is not a PHY needs to use a compatible. > > Right. I'd actually forgotten about the firmware-based walking, and > was thinking more of the non-firmware bus scanning as the commit > message was talking about being _unmasked_ and the only mask we have > is bus->phy_mask. Back then when I was working on the series and up until last week I had thought that having a device unmasked in mii_bus->phy_mask was a correct way to do for _any_ device including our DW XPCS (which BTW looks like a normal C45 PHY and if synthesized with a PMA attached could be passed to be handled by the PHY subsystem). Can't remember why exactly I came to that thought, but likely it was due to finding out examples of having mii_bus->phy_mask uninitialized in some of the PCS use-cases, like in drivers/net/dsa/ocelot/felix_vsc9959.c (but in case of DW XPCS the mask is always set indeed). Anyway obviously I was wrong and PHY-device is supposed to be created only if a device is actual PHY and handled by the PHY subsystem drivers. So the correct ways to create PHY MDIO-devices are: 1. Call mdiobus_register() with PHY-addresses unmasked 2. Call of_mdiobus_register() for a DT-node with sub-nodes for which of_mdiobus_child_is_phy() returns true. and the correct ways to create non-PHY MDIO-devices are: 1. Call mdiobus_register() with non-PHY-addresses masked and have those non-PHY device registered by mdiobus_register_board_info() beforehand. 2. Call of_mdiobus_register() with DT sub-nodes having specific compatible string (based on the of_mdiobus_child_is_phy() semantics). Only in case of having a non-PHY device registered it's allowed to use it in in non-PHY MDIO driver, like PCS, etc. Right? Please correct me if I am wrong or miss something. > > It seems to me that this is yet another case of a really confusing > commit message making review harder than it needs to be. From the perspective described above the patch log is indeed partly wrong. Sorry about that. I shouldn't have mentioned the mask at all but instead just listed two use-cases of creating the non-PHY MDIO-devices. I'll fix that in v2. -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! >
On Tue, Dec 05, 2023 at 02:54:00PM +0000, Russell King (Oracle) wrote: > On Tue, Dec 05, 2023 at 01:46:44PM +0000, Russell King (Oracle) wrote: > > For your use case, it would be: > > > > mdiodev = bus->mdio_map[addr]; > > By the way, this should be: > > mdiodev = mdiobus_find_device(bus, addr); > if (!mdiodev) > return ERR_PTR(-ENODEV); > > to avoid a layering violation. I would have used in the first place if it was externally visible, but it's defined as static. Do you suggest to make it global or ... > At some point, we should implement > mdiobus_get_mdiodev() which also deals with the refcount. ... create mdiobus_get_mdiodev() instead? * Note in the commit message I mentioned that having a getter would be * better than directly touching the mii_bus instance guts. -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Dec 12, 2023 at 06:26:16PM +0300, Serge Semin wrote: > I would have used in the first place if it was externally visible, but > it's defined as static. Do you suggest to make it global or ... That would be one option - I didn't make it visible when I introduced it beacuse there were no users for it. > > At some point, we should implement > > mdiobus_get_mdiodev() which also deals with the refcount. > > ... create mdiobus_get_mdiodev() instead? > > * Note in the commit message I mentioned that having a getter would be > * better than directly touching the mii_bus instance guts. What I'm thinking is: /** * mdiobus_get_mdiodev() - get a mdiodev for the specified bus * @bus: mii_bus to get mdio device from * @addr: mdio address of mdio device * * Return the struct mdio_device attached to the MII bus @bus at MDIO * address @addr. On success, the refcount on the device will be * increased, which must be dropped using mdio_device_put(), and the * mdio device returned. Otherwise, returns NULL. */ struct mdio_device *mdiobus_get_mdiodev(struct mii_bus *bus, int addr) { struct mdio_device *mdiodev; mdiodev = mdiobus_find_device(bus, addr); if (mdiodev) get_device(&mdiodev->dev); return mdiodev; } EXPORT_SYMBOL(mdiobus_get_mdiodev); should do it, and will hold a reference on the mdiodev structure (which won't be freed) and also on the mii_bus (since this device is a child of the bus device, the parent can't be released until the child has been, so struct mii_bus should at least stay around.) What would help the "the bus driver has been unbound" situation is if we took the mdio_lock on the bus, and then set the {read,write}{,_c45} functions to dummy stubs when the bus is being unregistered which then return e.g. -ENXIO. That will probably make unbinding/unloading all MDIO bus drivers safe from kernel oops, although phylib will spit out a non-useful backtrace if it tries an access. I don't think there's much which can be done about that - I did propose a patch to change that behaviour but apparently folk like having it! It isn't perfect - it's racy, but then accessing mdio_map[] is inherently racy due to no locking with mdiobus_.*register_device(). At least if we have everyone using a proper getter function rather than directly fiddling with bus->mdio_map[]. We only have one driver that accesses it directly at the moment (mscc_ptp): dev = phydev->mdio.bus->mdio_map[vsc8531->ts_base_addr]; phydev = container_of(dev, struct phy_device, mdio); return phydev->priv; and that should really be using mdiobus_get_phy().
On Tue, Dec 05, 2023 at 01:46:44PM +0000, Russell King (Oracle) wrote: > On Tue, Dec 05, 2023 at 01:35:27PM +0300, Serge Semin wrote: > > If the DW XPCS MDIO devices are either left unmasked for being auto-probed > > or explicitly registered in the MDIO subsystem by means of the > > mdiobus_register_board_info() method there is no point in creating the > > dummy MDIO device instance in order to get the DW XPCS handler since the > > MDIO core subsystem will create the device during the MDIO bus > > registration procedure. All what needs to be done is to just reuse the > > MDIO-device instance available in the mii_bus.mdio_map array (using some > > getter for it would look better though). It shall prevent the XPCS devices > > been accessed over several MDIO-device instances. > > > > Note since the MDIO-device instance might be retrieved from the MDIO-bus > > map array its reference counter shall be increased. If the MDIO-device > > instance is created in the xpcs_create_mdiodev() method its reference > > counter will be already increased. So there is no point in toggling the > > reference counter in the xpcs_create() function. Just drop it from there. > > > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > > --- > > drivers/net/pcs/pcs-xpcs.c | 26 +++++++++++++------------- > > 1 file changed, 13 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > > index 2850122f354a..a53376472394 100644 > > --- a/drivers/net/pcs/pcs-xpcs.c > > +++ b/drivers/net/pcs/pcs-xpcs.c > > @@ -1376,7 +1376,6 @@ static 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); > > @@ -1417,7 +1416,6 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev, > > ret = -ENODEV; > > > > out: > > - mdio_device_put(mdiodev); > > kfree(xpcs); > > > > return ERR_PTR(ret); > > The above two hunks are a completely Unnecessary change. > > > @@ -1437,19 +1435,21 @@ struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr, > > struct mdio_device *mdiodev; > > struct dw_xpcs *xpcs; > > > > - mdiodev = mdio_device_create(bus, addr); > > - if (IS_ERR(mdiodev)) > > - return ERR_CAST(mdiodev); > > + if (addr >= PHY_MAX_ADDR) > > + return ERR_PTR(-EINVAL); > > > > - xpcs = xpcs_create(mdiodev, interface); > > + if (mdiobus_is_registered_device(bus, addr)) { > > + mdiodev = bus->mdio_map[addr]; > > + mdio_device_get(mdiodev); > > This is fine - taking a reference on the mdiodev you've got from > somewhere else is the right thing to do. > > > + } else { > > + mdiodev = mdio_device_create(bus, addr); > > + if (IS_ERR(mdiodev)) > > + return ERR_CAST(mdiodev); > > + } > > > > - /* 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); > > + xpcs = xpcs_create(mdiodev, interface); > > + if (IS_ERR(xpcs)) > > + mdio_device_put(mdiodev); > > Without the change to xpcs_create() you don't need this change - and > this is why I say you don't understand refcounting. > > The point here is that the refcounting management is in each function > where references are gained or lost. > > xpcs_create() creates a new reference to the mdiodev by storing it in > the dw_xpcs structure. Therefore, it takes a reference to the mdiodev. > If something fails, it drops that reference to restore the refcount > as it was on function entry. > > xpcs_create_mdiodev() as it originally stood creates the mdiodev from > the bus/address, and then passes that to xpcs_create(). Once > xpcs_create() has finished its work (irrespective of whether it was > successful or not) we're done with the mdiodev in this function, so > the reference is _always_ put. Can't deny now I fully understood the whole concept indeed. It was the first time I met the double refcount management in a single place. My understanding was that it was enough to increment the counter once, when a driver got a pointer from somewhere else (like another subsystem) and decrement it after it's not used for sure. From that perspective getting a device in xpcs_create_mdiodev(), putting it in the cleanup-on-error path and in xpcs_destroy() was supposed to be enough. You say that it's required to manage the refcounting twice: when we get the reference from some external place and internally when the reference is stored in the XPCS descriptor. What's the point in such redundancy with the internal ref-counting if we know that the pointer can be safely stored and utilized afterwards? Better maintainability? Is it due to having the object retrieval and storing implemented in different functions? While at it if you happen to know an answer could you please also clarify the next question. None of the ordinary platform/PCI/USB/hwmon/etc drivers I've been working with managed refcounting on storing a passed to probe() device pointer in the private driver data. Is it wrong not doing that? Should the drivers call get_device() or it's derivatives in probe() if the respective object is stored in the driver data? If they shouldn't since the ref is already counted by the bus-specific probe() methods, then what makes the DW XPCS create/destroy methods special to have the double ref-counting utilized there? > > For your use case, it would be: > > mdiodev = bus->mdio_map[addr]; > mdio_device_get(mdiodev); > > xpcs = xpcs_create(mdiodev, interface); > > mdio_device_put(mdiodev); > > return xpcs; > > which illustrates this point - we get a reference to the mdiodev by > reading it from the array. We do something (calling xpcs_create) > with it. If that something was successful, it takes its own refcount > otherwise leaves it as-is. We're then done with the mdiodev so we > drop the refcount we took. I do understand the way the refcount management works in your implementation. It's just hard to figure out the reason of having the second get/set pair utilized for the internal reference. Anyway thanks for providing a very detailed comment and in advance for answering my questions. -Serge(y) > > There is no need to make the code more complicated by changing this, > so I regard the refcount changes in this patch to be wrong. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hi Vladimir, On Tue, Dec 05, 2023 at 01:52:34PM +0200, Vladimir Oltean wrote: > On Tue, Dec 05, 2023 at 01:35:27PM +0300, Serge Semin wrote: > > If the DW XPCS MDIO devices are either left unmasked for being auto-probed > > or explicitly registered in the MDIO subsystem by means of the > > mdiobus_register_board_info() method > > mdiobus_register_board_info() has exactly one caller, and that is > dsa_loop. I don't understand the relevance of it w.r.t. Synopsys XPCS. > I'm reading the patches in order from the beginning. Well, one user of the DW XPCS driver is updated in this series in the framework of the patch: [PATCH net-next 13/16] net: stmmac: intel: Register generic MDIO device https://lore.kernel.org/netdev/20231205103559.9605-14-fancer.lancer@gmail.com/ I can convert of them (it's sja1105 and wangxun txgbe) and then just drop the MDIO-device creation part from xpcs_create_mdiodev(). As I also described in another emails thread below this patch I used to think that unmasking non-PHY device is also appropriate to get the MDIO-device instance. I was wrong in that matter obviously. Anyway I just realized that my solution of using mdiobus_register_board_info() is a bit clumsy. Moreover the patch 13 (see the link above) shouldn't have the mdio_board_info instance allocation (it can be defined on stack) and most importantly is wrong in using the device-managed resources for it. The problem is that mdiobus_register_board_info() registers an MDIO-device once for entire system lifetime. It isn't that suitable for the hot-swappable devices and for drivers bind/unbind cases. Since there is no mdio_board_info-deregistration method, at the simplest case the no longer used board-info descriptors might be left registered if a device or driver are unloaded. That's why the device-managed allocation is harmful in such scenario. At the very least I'll need to convert the allocations to being non-managed. > > > there is no point in creating the dummy MDIO device instance in order > > Why dummy? There's nothing dummy about the mdio_device. It's how the PCS > code accesses the hardware. I call it 'dummy' because no actual device is registered (though 'redundant' or similar definition might sound more appropriate). The entire structure is used as a communication layer between the XPCS driver and MDIO device, where the device address is the only info needed. Basically nothing prevents us from converting the current DW XPCS driver to using the mdiobus_c45_read()/mdiobus_c45_write() methods. Though in that case I wouldn't be able to easily add the fwnode-based MDIO-devices support. > > > to get the DW XPCS handler since the MDIO core subsystem will create > > the device during the MDIO bus registration procedure. > > It won't, though? Unless someone is using mdiobus_register_board_info() > possibly, but who does that? As I said above I wrongly assumed that unmasking non-PHY device was ok. But mdiobus_register_board_info() could be used for that as I (a bit clumsy) demonstrated in the patch 13. > > > All what needs to be done is to just reuse the MDIO-device instance > > available in the mii_bus.mdio_map array (using some getter for it > > would look better though). It shall prevent the XPCS devices been > > accessed over several MDIO-device instances. > > > > Note since the MDIO-device instance might be retrieved from the MDIO-bus > > map array its reference counter shall be increased. If the MDIO-device > > instance is created in the xpcs_create_mdiodev() method its reference > > counter will be already increased. So there is no point in toggling the > > reference counter in the xpcs_create() function. Just drop it from there. > > > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > > --- > > Sorry, because the commit log lost me at the "context presentation" stage, > I failed to understand the "what"s and the "why"s. > > Are you basically trying to add xpcs support on top of an mdio_device > where the mdio_device_create() call was made externally to the xpcs code, > through mdiobus_register_board_info() and mdiobus_setup_mdiodev_from_board_info()? Basically yes, but there is more of it. The main idea is to convert the XPCS driver to using the already created non-PHY MDIO-devices instead of manually creating a 'dummy'/'redundant' one. From my point of view there are several reasons of doing so: 1. mdiobus_register_board_info() provides a way to assign the device platform data to being registered afterwards device. Thus we can pass some custom data to the XPCS-device driver (whether it's just an xpcs_create_*() call or a fully functional MDIO-device driver registered by the mdio_driver_register() method). For instance it can be utilized to drop the fake PHYSIDs implementation from drivers/net/dsa/sja1105/sja1105_mdio.c . 2. The MDIO-devices actually registered on the MDIO-bus will be visible in sysfs with for instance useful IO statistics provided by the MDIO-bus. Potentially (if it is required) at some point we'll be able to convert the DW XPCS driver to being true MDIO-device driver (bindable to the DW XPCS device) with less efforts. 3. Having an MDIO-device registered that way would make the DW XPCS IO-device implementation unified after the fwnode-based XPCS descriptor creation support is added in one of the subsequent patches. So based on the listed above I've got a question. Do you think all of that is worth to be implemented? Andrew, Russell? I am asking because the patchset advance depends on your answers. If you do I'll need to fix the problem described in my first message, implement some new mdiobus_register_board_info()-like but MDIO-bus-specific interface function (so MDIO-device infos would be attached to the allocated MDIO-bus and then used to register the respective MDIO-devices on the MDIO-bus registration), then convert the sja1105 and wangxun txgbe drivers to using it. If you don't I'll get back the xpcs_create_mdiodev() implementation and just provide a fwnode-based version of one. Note we already settled that converting DW XPCS driver to being normal MDIO-device driver is prone to errors at this stage due to a possibility to have the driver unbindable from user-space. I'll just move the DT-compatibles check to the xpcs_create_fwnode() method and drop the rest of the MDIO-device-driver-specific things. -Serge(y)
On Tue, Dec 12, 2023 at 07:06:46PM +0000, Russell King (Oracle) wrote: > On Tue, Dec 12, 2023 at 06:26:16PM +0300, Serge Semin wrote: > > I would have used in the first place if it was externally visible, but > > it's defined as static. Do you suggest to make it global or ... > > That would be one option - I didn't make it visible when I introduced it > beacuse there were no users for it. > > > > At some point, we should implement > > > mdiobus_get_mdiodev() which also deals with the refcount. > > > > ... create mdiobus_get_mdiodev() instead? > > > > * Note in the commit message I mentioned that having a getter would be > > * better than directly touching the mii_bus instance guts. > > What I'm thinking is: > > /** > * mdiobus_get_mdiodev() - get a mdiodev for the specified bus > * @bus: mii_bus to get mdio device from > * @addr: mdio address of mdio device > * > * Return the struct mdio_device attached to the MII bus @bus at MDIO > * address @addr. On success, the refcount on the device will be > * increased, which must be dropped using mdio_device_put(), and the > * mdio device returned. Otherwise, returns NULL. > */ > struct mdio_device *mdiobus_get_mdiodev(struct mii_bus *bus, int addr) > { > struct mdio_device *mdiodev; > > mdiodev = mdiobus_find_device(bus, addr); > if (mdiodev) > get_device(&mdiodev->dev); > return mdiodev; > } > EXPORT_SYMBOL(mdiobus_get_mdiodev); > > should do it, and will hold a reference on the mdiodev structure (which > won't be freed) and also on the mii_bus (since this device is a child > of the bus device, the parent can't be released until the child has > been, so struct mii_bus should at least stay around.) Right. That's exactly what had in mind. Thanks for suggesting a ready-to-apply solution. I'll add it to the series as a separate patch if we decide to keep the proposed in this patch change. See my question in the next message: https://lore.kernel.org/netdev/wnptneaxxe2tq2rf7ac6a72xtyluyggughvmtxbbg5qto64mpa@7gchl5e4qllu/ > > What would help the "the bus driver has been unbound" situation is if > we took the mdio_lock on the bus, and then set the {read,write}{,_c45} > functions to dummy stubs when the bus is being unregistered which then > return e.g. -ENXIO. That will probably make unbinding/unloading all > MDIO bus drivers safe from kernel oops, although phylib will spit out > a non-useful backtrace if it tries an access. I don't think there's > much which can be done about that - I did propose a patch to change > that behaviour but apparently folk like having it! > > It isn't perfect - it's racy, but then accessing mdio_map[] is > inherently racy due to no locking with mdiobus_.*register_device(). > At least if we have everyone using a proper getter function rather > than directly fiddling with bus->mdio_map[]. We only have one driver > that accesses it directly at the moment (mscc_ptp): > > dev = phydev->mdio.bus->mdio_map[vsc8531->ts_base_addr]; > phydev = container_of(dev, struct phy_device, mdio); > > return phydev->priv; > > and that should really be using mdiobus_get_phy(). Regarding the driver bind/unbind. I guess the maintainers just forget about that problem. Do you think it's worth reminding them about it? -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Dec 13, 2023 at 03:01:45AM +0300, Serge Semin wrote: > On Tue, Dec 05, 2023 at 01:46:44PM +0000, Russell King (Oracle) wrote: > > xpcs_create_mdiodev() as it originally stood creates the mdiodev from > > the bus/address, and then passes that to xpcs_create(). Once > > xpcs_create() has finished its work (irrespective of whether it was > > successful or not) we're done with the mdiodev in this function, so > > the reference is _always_ put. > > You say that it's required to manage the refcounting twice: when we > get the reference from some external place and internally when the > reference is stored in the XPCS descriptor. What's the point in such > redundancy with the internal ref-counting if we know that the pointer > can be safely stored and utilized afterwards? Better maintainability? > Is it due to having the object retrieval and storing implemented in > different functions? The point is that the error handling gets simpler: - One can see in xpcs_create_mdiodev() that the reference taken by mdio_device_create() is always dropped if that function was successful, irrespective of whether xpcs_create() was successful. - xpcs_create() is responsible for managing the refcount on the mdiodev that is passed to it - and if it's successful, it needs to increment the refcount, or leave it in the same state as it was on entry if failing. This avoids complexities in error paths, which are notorious for things being forgotten - since with this, each of these functions is resposible for managing its refcount. It's a different style of refcount management, one I think more people should adopt. > While at it if you happen to know an answer could you please also > clarify the next question. None of the ordinary > platform/PCI/USB/hwmon/etc drivers I've been working with managed > refcounting on storing a passed to probe() device pointer in the > private driver data. Is it wrong not doing that? If we wanted to do refcounting strictly, then every time a new pointer to a data structure is created, we should be taking a refcount on it, and each time that pointer is destroyed, we should be putting the refcount. That is what refcounting is all about. However, there are circumstances where this can be done lazily, and for drivers we would prefer driver authors not to end up with refcount errors where they've forgotten to put something. In the specific case of drivers, we have a well defined lifetime for a device bound to a driver. We guarantee that the struct device will not go away if a driver is bound to the device, until such time that the driver's .remove method has been called. Thus, we guarantee that the device driver will be notified of the struct device going away before it has been freed. This frees the driver author from having to worry about the refcount of the struct device. As soon as we start doing stuff that is outside of that model, then objects that are refcounted need to be dealt with, and I much prefer the "strict" refcounting implementation such as the one I added to xpcs, because IMHO it's much easier to see that the flow is obviously correct - even if it does need a comment to describe why we always do a put.
On Wed, Dec 13, 2023 at 04:32:21PM +0000, Russell King (Oracle) wrote: > On Wed, Dec 13, 2023 at 03:01:45AM +0300, Serge Semin wrote: > > On Tue, Dec 05, 2023 at 01:46:44PM +0000, Russell King (Oracle) wrote: > > > xpcs_create_mdiodev() as it originally stood creates the mdiodev from > > > the bus/address, and then passes that to xpcs_create(). Once > > > xpcs_create() has finished its work (irrespective of whether it was > > > successful or not) we're done with the mdiodev in this function, so > > > the reference is _always_ put. > > > > You say that it's required to manage the refcounting twice: when we > > get the reference from some external place and internally when the > > reference is stored in the XPCS descriptor. What's the point in such > > redundancy with the internal ref-counting if we know that the pointer > > can be safely stored and utilized afterwards? Better maintainability? > > Is it due to having the object retrieval and storing implemented in > > different functions? > > The point is that the error handling gets simpler: > - One can see in xpcs_create_mdiodev() that the reference taken by > mdio_device_create() is always dropped if that function was > successful, irrespective of whether xpcs_create() was successful. > > - xpcs_create() is responsible for managing the refcount on the mdiodev > that is passed to it - and if it's successful, it needs to increment > the refcount, or leave it in the same state as it was on entry if > failing. > > This avoids complexities in error paths, which are notorious for things > being forgotten - since with this, each of these functions is resposible > for managing its refcount. > > It's a different style of refcount management, one I think more people > should adopt. > > > While at it if you happen to know an answer could you please also > > clarify the next question. None of the ordinary > > platform/PCI/USB/hwmon/etc drivers I've been working with managed > > refcounting on storing a passed to probe() device pointer in the > > private driver data. Is it wrong not doing that? > > If we wanted to do refcounting strictly, then every time a new > pointer to a data structure is created, we should be taking a refcount > on it, and each time that pointer is destroyed, we should be putting > the refcount. That is what refcounting is all about. > > However, there are circumstances where this can be done lazily, and > for drivers we would prefer driver authors not to end up with > refcount errors where they've forgotten to put something. > > In the specific case of drivers, we have a well defined lifetime for > a device bound to a driver. We guarantee that the struct device will > not go away if a driver is bound to the device, until such time that > the driver's .remove method has been called. Thus, we guarantee that > the device driver will be notified of the struct device going away > before it has been freed. This frees the driver author from having > to worry about the refcount of the struct device. > > As soon as we start doing stuff that is outside of that model, then > objects that are refcounted need to be dealt with, and I much prefer > the "strict" refcounting implementation such as the one I added to > xpcs, because IMHO it's much easier to see that the flow is obviously > correct - even if it does need a comment to describe why we always > do a put. Ok. I fully get your point now: lazy refcounting for the drivers following standard model and the 'strict' one for others. It sounds reasonable. I'll get that adopted in my future developments. Thank you very much for the detailed explanation and for all your comments. -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hi Vladimir, Russell, Andrew On Wed, Dec 13, 2023 at 06:27:57PM +0300, Serge Semin wrote: > Hi Vladimir, > > On Tue, Dec 05, 2023 at 01:52:34PM +0200, Vladimir Oltean wrote: > > On Tue, Dec 05, 2023 at 01:35:27PM +0300, Serge Semin wrote: > > > If the DW XPCS MDIO devices are either left unmasked for being auto-probed > > > or explicitly registered in the MDIO subsystem by means of the > > > mdiobus_register_board_info() method > > > > > mdiobus_register_board_info() has exactly one caller, and that is > > dsa_loop. I don't understand the relevance of it w.r.t. Synopsys XPCS. > > I'm reading the patches in order from the beginning. > > Well, one user of the DW XPCS driver is updated in this series in the > framework of the patch: > [PATCH net-next 13/16] net: stmmac: intel: Register generic MDIO device > https://lore.kernel.org/netdev/20231205103559.9605-14-fancer.lancer@gmail.com/ > > I can convert of them (it's sja1105 and wangxun txgbe) and then just > drop the MDIO-device creation part from xpcs_create_mdiodev(). As I > also described in another emails thread below this patch I used to > think that unmasking non-PHY device is also appropriate to get the > MDIO-device instance. I was wrong in that matter obviously. > > Anyway I just realized that my solution of using > mdiobus_register_board_info() is a bit clumsy. Moreover the patch 13 > (see the link above) shouldn't have the mdio_board_info instance > allocation (it can be defined on stack) and most importantly is wrong > in using the device-managed resources for it. The problem is that > mdiobus_register_board_info() registers an MDIO-device once for entire > system lifetime. It isn't that suitable for the hot-swappable devices > and for drivers bind/unbind cases. Since there is no > mdio_board_info-deregistration method, at the simplest case the no > longer used board-info descriptors might be left registered if a > device or driver are unloaded. That's why the device-managed > allocation is harmful in such scenario. At the very least I'll need to > convert the allocations to being non-managed. > > > > > > there is no point in creating the dummy MDIO device instance in order > > > > > Why dummy? There's nothing dummy about the mdio_device. It's how the PCS > > code accesses the hardware. > > I call it 'dummy' because no actual device is registered (though > 'redundant' or similar definition might sound more appropriate). The > entire structure is used as a communication layer between the XPCS > driver and MDIO device, where the device address is the only info > needed. Basically nothing prevents us from converting the current DW > XPCS driver to using the mdiobus_c45_read()/mdiobus_c45_write() > methods. Though in that case I wouldn't be able to easily add the > fwnode-based MDIO-devices support. > > > > > > to get the DW XPCS handler since the MDIO core subsystem will create > > > the device during the MDIO bus registration procedure. > > > > > It won't, though? Unless someone is using mdiobus_register_board_info() > > possibly, but who does that? > > As I said above I wrongly assumed that unmasking non-PHY device was > ok. But mdiobus_register_board_info() could be used for that as I (a > bit clumsy) demonstrated in the patch 13. > > > > > > All what needs to be done is to just reuse the MDIO-device instance > > > available in the mii_bus.mdio_map array (using some getter for it > > > would look better though). It shall prevent the XPCS devices been > > > accessed over several MDIO-device instances. > > > > > > Note since the MDIO-device instance might be retrieved from the MDIO-bus > > > map array its reference counter shall be increased. If the MDIO-device > > > instance is created in the xpcs_create_mdiodev() method its reference > > > counter will be already increased. So there is no point in toggling the > > > reference counter in the xpcs_create() function. Just drop it from there. > > > > > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > > > --- > > > > > Sorry, because the commit log lost me at the "context presentation" stage, > > I failed to understand the "what"s and the "why"s. > > > > Are you basically trying to add xpcs support on top of an mdio_device > > where the mdio_device_create() call was made externally to the xpcs code, > > through mdiobus_register_board_info() and mdiobus_setup_mdiodev_from_board_info()? > > Basically yes, but there is more of it. The main idea is to convert > the XPCS driver to using the already created non-PHY MDIO-devices > instead of manually creating a 'dummy'/'redundant' one. From my point > of view there are several reasons of doing so: > > 1. mdiobus_register_board_info() provides a way to assign the device > platform data to being registered afterwards device. Thus we can pass > some custom data to the XPCS-device driver (whether it's just an > xpcs_create_*() call or a fully functional MDIO-device driver > registered by the mdio_driver_register() method). For instance it can > be utilized to drop the fake PHYSIDs implementation from > drivers/net/dsa/sja1105/sja1105_mdio.c . > > 2. The MDIO-devices actually registered on the MDIO-bus will be > visible in sysfs with for instance useful IO statistics provided by > the MDIO-bus. Potentially (if it is required) at some point we'll be > able to convert the DW XPCS driver to being true MDIO-device driver > (bindable to the DW XPCS device) with less efforts. > > 3. Having an MDIO-device registered that way would make the DW XPCS > IO-device implementation unified after the fwnode-based XPCS > descriptor creation support is added in one of the subsequent patches. > > So based on the listed above I've got a question. Do you think all of > that is worth to be implemented? Andrew, Russell? > > I am asking because the patchset advance depends on your answers. If > you do I'll need to fix the problem described in my first message, > implement some new mdiobus_register_board_info()-like but > MDIO-bus-specific interface function (so MDIO-device infos would be > attached to the allocated MDIO-bus and then used to register the > respective MDIO-devices on the MDIO-bus registration), then convert > the sja1105 and wangxun txgbe drivers to using it. If you don't I'll > get back the xpcs_create_mdiodev() implementation and just provide a > fwnode-based version of one. Folks, this is the only issue left to settle so I could move on with the series fixing up. So the question is: taking my comment above into account is it worth to convert the xpcs_create_mdiodev() method to re-using the already registered MDIO-device instance instead of always creating a stub-like MDIO-device? -Serge(y) > > Note we already settled that converting DW XPCS driver to being normal > MDIO-device driver is prone to errors at this stage due to a > possibility to have the driver unbindable from user-space. I'll just > move the DT-compatibles check to the xpcs_create_fwnode() method and > drop the rest of the MDIO-device-driver-specific things. > > -Serge(y)
On Tue, Dec 19, 2023 at 06:48:09PM +0300, Serge Semin wrote: > > > Sorry, because the commit log lost me at the "context presentation" stage, > > > I failed to understand the "what"s and the "why"s. > > > > > > Are you basically trying to add xpcs support on top of an mdio_device > > > where the mdio_device_create() call was made externally to the xpcs code, > > > through mdiobus_register_board_info() and mdiobus_setup_mdiodev_from_board_info()? > > > > Basically yes, but there is more of it. The main idea is to convert > > the XPCS driver to using the already created non-PHY MDIO-devices > > instead of manually creating a 'dummy'/'redundant' one. From my point > > of view there are several reasons of doing so: > > > > 1. mdiobus_register_board_info() provides a way to assign the device > > platform data to being registered afterwards device. Thus we can pass > > some custom data to the XPCS-device driver (whether it's just an > > xpcs_create_*() call or a fully functional MDIO-device driver > > registered by the mdio_driver_register() method). For instance it can > > be utilized to drop the fake PHYSIDs implementation from > > drivers/net/dsa/sja1105/sja1105_mdio.c . Ok. Seeing an alternative to the NXP_SJA1110_XPCS_ID hack will be interesting. FWIW, I'm looking at reworking the dsa_loop probing to use software nodes. Since dsa_loop is the only current user of mdiobus_register_board_info(), maybe that will lead to its deletion. It appears a matter of timing, but the mechanism looks promising. Maybe we can also use it somehow to add compatibility with existing lynx-pcs device trees where there is no compatible string, so a struct phy_device gets created. Device tree breakage was the fundamental reason why Sean Anderson's patch set couldn't make forward progress. https://patchwork.kernel.org/project/netdevbpf/cover/20221103210650.2325784-1-sean.anderson@seco.com/ > > 2. The MDIO-devices actually registered on the MDIO-bus will be > > visible in sysfs with for instance useful IO statistics provided by > > the MDIO-bus. Potentially (if it is required) at some point we'll be > > able to convert the DW XPCS driver to being true MDIO-device driver > > (bindable to the DW XPCS device) with less efforts. Ok. > > 3. Having an MDIO-device registered that way would make the DW XPCS > > IO-device implementation unified after the fwnode-based XPCS > > descriptor creation support is added in one of the subsequent patches. Unified how? You mean that "XPCS will always operate as a driver bound to an mdio_device"? You're not planning to unify the mdio_device and MMIO register handling by using regmap, right? > > So based on the listed above I've got a question. Do you think all of > > that is worth to be implemented? Andrew, Russell? > > > > I am asking because the patchset advance depends on your answers. If > > you do I'll need to fix the problem described in my first message, > > implement some new mdiobus_register_board_info()-like but > > MDIO-bus-specific interface function (so MDIO-device infos would be > > attached to the allocated MDIO-bus and then used to register the > > respective MDIO-devices on the MDIO-bus registration), then convert > > the sja1105 and wangxun txgbe drivers to using it. If you don't I'll > > get back the xpcs_create_mdiodev() implementation and just provide a > > fwnode-based version of one. > > Folks, this is the only issue left to settle so I could move on with > the series fixing up. So the question is: taking my comment above into > account is it worth to convert the xpcs_create_mdiodev() method to > re-using the already registered MDIO-device instance instead of > always creating a stub-like MDIO-device? I can't exactly say "yes, this is worth it", because it also depends on what the phylib/phylink maintainers say. So I haven't said anything. But I also don't have any objection, as long as the conversion doesn't break existing setups (in new ways; see the "unbind MDIO bus driver" case which is already problematic).
On Tue, Dec 19, 2023 at 06:28:03PM +0200, Vladimir Oltean wrote: > On Tue, Dec 19, 2023 at 06:48:09PM +0300, Serge Semin wrote: > > > > Sorry, because the commit log lost me at the "context presentation" stage, > > > > I failed to understand the "what"s and the "why"s. > > > > > > > > Are you basically trying to add xpcs support on top of an mdio_device > > > > where the mdio_device_create() call was made externally to the xpcs code, > > > > through mdiobus_register_board_info() and mdiobus_setup_mdiodev_from_board_info()? > > > > > > Basically yes, but there is more of it. The main idea is to convert > > > the XPCS driver to using the already created non-PHY MDIO-devices > > > instead of manually creating a 'dummy'/'redundant' one. From my point > > > of view there are several reasons of doing so: > > > > > > 1. mdiobus_register_board_info() provides a way to assign the device > > > platform data to being registered afterwards device. Thus we can pass > > > some custom data to the XPCS-device driver (whether it's just an > > > xpcs_create_*() call or a fully functional MDIO-device driver > > > registered by the mdio_driver_register() method). For instance it can > > > be utilized to drop the fake PHYSIDs implementation from > > > drivers/net/dsa/sja1105/sja1105_mdio.c . > > Ok. Seeing an alternative to the NXP_SJA1110_XPCS_ID hack will be interesting. > > FWIW, I'm looking at reworking the dsa_loop probing to use software nodes. > Since dsa_loop is the only current user of mdiobus_register_board_info(), > maybe that will lead to its deletion. It appears a matter of timing, but > the mechanism looks promising. I think I'll be able to fix my series within two weeks. Seeing it's going to be merge window soon and Xmas/NY holidays then, the patchset will be re-submitted afterwards. Note in my case mdiobus_register_board_info() isn't that well suitable. As I explained a bit earlier in this thread, mdiobus_register_board_info() isn't working for the hot-pluggable devices and for the case when MAC/MDIO-bus drivers unbinding is possible. What could be better is to have a method like this: mdiobus_register_bus_info(struct mii_bus *bus, struct mdio_board_info *info, unsigned int n) { // Add devs-info's into the internal mii_bus-instance list // for each of which the MDIO-devices will be then created and // registered by means of the method // mdiobus_setup_mdiodev_from_board_info() called in // __mdiobus_register(). } Which could be called for the just allocated and not yet registered MDIO-bus descriptor in order to prescribe the non-PHY devices on the bus. Alternatively, what might be even more preferable and easier to implement I could: 1. Globalise and export mdiobus_create_device(). 2. Make sure all non-PHY MDIO-devices are masked in mii_bus instance. 3. Register MDIO-bus. 4. Call mdiobus_create_device() for each XPCS device. 5. Then call xpcs_create_mdiodev() for each registered device. > > Maybe we can also use it somehow to add compatibility with existing > lynx-pcs device trees where there is no compatible string, so a struct > phy_device gets created. Device tree breakage was the fundamental reason > why Sean Anderson's patch set couldn't make forward progress. > https://patchwork.kernel.org/project/netdevbpf/cover/20221103210650.2325784-1-sean.anderson@seco.com/ In case of DW XPCS the solutions described in my comment above are only required for the runtime-registered non-OF MDIO-buses. DW XPCS DT-based devices are unsupported by the current STMMAC driver (adding that support is the main goal of this patchset). So my case is simpler. But regarding your proposal, I guess the first version of the solutions described in my comment above could be suitable for you if the _of_mdiobus_register() method is somehow fixed to considering the DT-nodes with no compatible strings as non-PHY MDIO-devices. For instance the _of_mdiobus_register() function can work that way if it detects some pre-registered mdio_board_info data in the being registered mii_bus instance. Alternatively you could introduce an additional mii_bus structure field which would indicate such non-PHY MDIO-devices. Note in order to make things backward compatible you would need to tweak the drivers/net/ethernet/freescale/xgmac_mdio.c driver so one would detect the platforms (for instance, based on the platform compatible string) on which the Lynx PCSs are specified with no compatible strings and activate the mechanism above. Then you can freely convert the currently available Lynx PCS dts nodes to having the compatible strings. The kernel will be still backwards compatible for old dtbs and will contain the Lynx PCS DT-nodes identified by the proper compatible strings. > > > > 2. The MDIO-devices actually registered on the MDIO-bus will be > > > visible in sysfs with for instance useful IO statistics provided by > > > the MDIO-bus. Potentially (if it is required) at some point we'll be > > > able to convert the DW XPCS driver to being true MDIO-device driver > > > (bindable to the DW XPCS device) with less efforts. > > Ok. > > > > 3. Having an MDIO-device registered that way would make the DW XPCS > > > IO-device implementation unified after the fwnode-based XPCS > > > descriptor creation support is added in one of the subsequent patches. > > Unified how? You mean that "XPCS will always operate as a driver bound > to an mdio_device"? No. I meant that mdio_device would be registered as a normal device on the MDIO-bus in both fwnode-based and runtime-based cases. No driver will be bound to those devices, but some platform-data could be specified and handled identically in both cases. Eventually when the net core is ready for it, the DW XPCS driver could be easily converted to being normal MDIO-device driver and bound to the XPCS devices registered on a MDIO-bus. > > You're not planning to unify the mdio_device and MMIO register handling > by using regmap, right? No. It will be too tiresome especially seeing the current devm_mdio_regmap() implementation doesn't support C45 IO ops and using regmaps would cause having redundant abstraction layers. I see no much benefits in that at the moment. In more details I explained it here: https://lore.kernel.org/netdev/cv6oo27tqbfst3jrgtkg7bcxmeshadtzoomn2xgnzh2arz4nwy@wq5k7oygto6n/ > > > > So based on the listed above I've got a question. Do you think all of > > > that is worth to be implemented? Andrew, Russell? > > > > > > I am asking because the patchset advance depends on your answers. If > > > you do I'll need to fix the problem described in my first message, > > > implement some new mdiobus_register_board_info()-like but > > > MDIO-bus-specific interface function (so MDIO-device infos would be > > > attached to the allocated MDIO-bus and then used to register the > > > respective MDIO-devices on the MDIO-bus registration), then convert > > > the sja1105 and wangxun txgbe drivers to using it. If you don't I'll > > > get back the xpcs_create_mdiodev() implementation and just provide a > > > fwnode-based version of one. > > > > Folks, this is the only issue left to settle so I could move on with > > the series fixing up. So the question is: taking my comment above into > > account is it worth to convert the xpcs_create_mdiodev() method to > > re-using the already registered MDIO-device instance instead of > > always creating a stub-like MDIO-device? > > I can't exactly say "yes, this is worth it", because it also depends on > what the phylib/phylink maintainers say. So I haven't said anything. > But I also don't have any objection, as long as the conversion doesn't > break existing setups (in new ways; see the "unbind MDIO bus driver" > case which is already problematic). Ok. Thanks. There won't be MDIO-device driver binding. I get to decide later what solution described in my first comment to implement (unless you insist on some of them particularly or suggest an alternative). -Serge(y)
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c index 2850122f354a..a53376472394 100644 --- a/drivers/net/pcs/pcs-xpcs.c +++ b/drivers/net/pcs/pcs-xpcs.c @@ -1376,7 +1376,6 @@ static 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); @@ -1417,7 +1416,6 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev, ret = -ENODEV; out: - mdio_device_put(mdiodev); kfree(xpcs); return ERR_PTR(ret); @@ -1437,19 +1435,21 @@ struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr, struct mdio_device *mdiodev; struct dw_xpcs *xpcs; - mdiodev = mdio_device_create(bus, addr); - if (IS_ERR(mdiodev)) - return ERR_CAST(mdiodev); + if (addr >= PHY_MAX_ADDR) + return ERR_PTR(-EINVAL); - xpcs = xpcs_create(mdiodev, interface); + if (mdiobus_is_registered_device(bus, addr)) { + mdiodev = bus->mdio_map[addr]; + mdio_device_get(mdiodev); + } else { + mdiodev = mdio_device_create(bus, addr); + if (IS_ERR(mdiodev)) + return ERR_CAST(mdiodev); + } - /* 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); + xpcs = xpcs_create(mdiodev, interface); + if (IS_ERR(xpcs)) + mdio_device_put(mdiodev); return xpcs; }
If the DW XPCS MDIO devices are either left unmasked for being auto-probed or explicitly registered in the MDIO subsystem by means of the mdiobus_register_board_info() method there is no point in creating the dummy MDIO device instance in order to get the DW XPCS handler since the MDIO core subsystem will create the device during the MDIO bus registration procedure. All what needs to be done is to just reuse the MDIO-device instance available in the mii_bus.mdio_map array (using some getter for it would look better though). It shall prevent the XPCS devices been accessed over several MDIO-device instances. Note since the MDIO-device instance might be retrieved from the MDIO-bus map array its reference counter shall be increased. If the MDIO-device instance is created in the xpcs_create_mdiodev() method its reference counter will be already increased. So there is no point in toggling the reference counter in the xpcs_create() function. Just drop it from there. Signed-off-by: Serge Semin <fancer.lancer@gmail.com> --- drivers/net/pcs/pcs-xpcs.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)