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 <[email protected]>
---
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);
@@ -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;
}
--
2.42.1
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.
--
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 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 <[email protected]>
> ---
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.
--
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 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 <[email protected]>
> ---
> 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.
--
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: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.
--
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: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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
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().
--
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: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 <[email protected]>
> > ---
> > 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/[email protected]/
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 <[email protected]>
> > ---
>
> 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.
--
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 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/[email protected]/
>
> 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 <[email protected]>
> > > ---
> >
>
> > 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/[email protected]/
> > 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/[email protected]/
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)