| Message ID | 20220711160519.741990-1-sean.anderson@seco.com |
|---|---|
| Headers | show |
| Series | net: pcs: Add support for devices probed in the "usual" manner | expand |
Hi Sean, On Mon, Jul 11, 2022 at 12:05:10PM -0400, Sean Anderson wrote: > For a long time, PCSs have been tightly coupled with their MACs. For > this reason, the MAC creates the "phy" or mdio device, and then passes > it to the PCS to initialize. This has a few disadvantages: > > - Each MAC must re-implement the same steps to look up/create a PCS > - The PCS cannot use functions tied to device lifetime, such as devm_*. > - Generally, the PCS does not have easy access to its device tree node > > I'm not sure if these are terribly large disadvantages. In fact, I'm not > sure if this series provides any benefit which could not be achieved > with judicious use of helper functions. In any case, here it is. > > NB: Several (later) patches in this series should not be applied. See > the notes in each commit for details on when they can be applied. Sorry to burst your bubble, but the networking drivers on NXP LS1028A (device tree at arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi, drivers at drivers/net/ethernet/freescale/enetc/ and drivers/net/dsa/ocelot/) do not use the Lynx PCS through a pcs-handle, because the Lynx PCS in fact has no backing OF node there, nor do the internal MDIO buses of the ENETC and of the switch. It seems that I need to point this out explicitly: you need to provide at least a working migration path to your PCS driver model. Currently there isn't one, and as a result, networking is broken on the LS1028A with this patch set.
Hi Vladimir, On 7/19/22 11:25 AM, Vladimir Oltean wrote: > Hi Sean, > > On Mon, Jul 11, 2022 at 12:05:10PM -0400, Sean Anderson wrote: >> For a long time, PCSs have been tightly coupled with their MACs. For >> this reason, the MAC creates the "phy" or mdio device, and then passes >> it to the PCS to initialize. This has a few disadvantages: >> >> - Each MAC must re-implement the same steps to look up/create a PCS >> - The PCS cannot use functions tied to device lifetime, such as devm_*. >> - Generally, the PCS does not have easy access to its device tree node >> >> I'm not sure if these are terribly large disadvantages. In fact, I'm not >> sure if this series provides any benefit which could not be achieved >> with judicious use of helper functions. In any case, here it is. >> >> NB: Several (later) patches in this series should not be applied. See >> the notes in each commit for details on when they can be applied. > > Sorry to burst your bubble, but the networking drivers on NXP LS1028A > (device tree at arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi, drivers > at drivers/net/ethernet/freescale/enetc/ and drivers/net/dsa/ocelot/) > do not use the Lynx PCS through a pcs-handle, because the Lynx PCS in > fact has no backing OF node there, nor do the internal MDIO buses of the > ENETC and of the switch. > > It seems that I need to point this out explicitly: you need to provide > at least a working migration path to your PCS driver model. Currently > there isn't one, and as a result, networking is broken on the LS1028A > with this patch set. > Please refer to patches 4, 5, and 6. --Sean
On Tue, Jul 19, 2022 at 11:28:42AM -0400, Sean Anderson wrote: > Hi Vladimir, > > On 7/19/22 11:25 AM, Vladimir Oltean wrote: > > Hi Sean, > > > > On Mon, Jul 11, 2022 at 12:05:10PM -0400, Sean Anderson wrote: > >> For a long time, PCSs have been tightly coupled with their MACs. For > >> this reason, the MAC creates the "phy" or mdio device, and then passes > >> it to the PCS to initialize. This has a few disadvantages: > >> > >> - Each MAC must re-implement the same steps to look up/create a PCS > >> - The PCS cannot use functions tied to device lifetime, such as devm_*. > >> - Generally, the PCS does not have easy access to its device tree node > >> > >> I'm not sure if these are terribly large disadvantages. In fact, I'm not > >> sure if this series provides any benefit which could not be achieved > >> with judicious use of helper functions. In any case, here it is. > >> > >> NB: Several (later) patches in this series should not be applied. See > >> the notes in each commit for details on when they can be applied. > > > > Sorry to burst your bubble, but the networking drivers on NXP LS1028A > > (device tree at arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi, drivers > > at drivers/net/ethernet/freescale/enetc/ and drivers/net/dsa/ocelot/) > > do not use the Lynx PCS through a pcs-handle, because the Lynx PCS in > > fact has no backing OF node there, nor do the internal MDIO buses of the > > ENETC and of the switch. > > > > It seems that I need to point this out explicitly: you need to provide > > at least a working migration path to your PCS driver model. Currently > > there isn't one, and as a result, networking is broken on the LS1028A > > with this patch set. > > > > Please refer to patches 4, 5, and 6. I don't understand, could you be more clear? Are you saying that I shouldn't have applied patch 9 while testing? When would be a good moment to apply patch 9?
On 7/19/22 11:38 AM, Vladimir Oltean wrote: > On Tue, Jul 19, 2022 at 11:28:42AM -0400, Sean Anderson wrote: >> Hi Vladimir, >> >> On 7/19/22 11:25 AM, Vladimir Oltean wrote: >> > Hi Sean, >> > >> > On Mon, Jul 11, 2022 at 12:05:10PM -0400, Sean Anderson wrote: >> >> For a long time, PCSs have been tightly coupled with their MACs. For >> >> this reason, the MAC creates the "phy" or mdio device, and then passes >> >> it to the PCS to initialize. This has a few disadvantages: >> >> >> >> - Each MAC must re-implement the same steps to look up/create a PCS >> >> - The PCS cannot use functions tied to device lifetime, such as devm_*. >> >> - Generally, the PCS does not have easy access to its device tree node >> >> >> >> I'm not sure if these are terribly large disadvantages. In fact, I'm not >> >> sure if this series provides any benefit which could not be achieved >> >> with judicious use of helper functions. In any case, here it is. >> >> >> >> NB: Several (later) patches in this series should not be applied. See >> >> the notes in each commit for details on when they can be applied. >> > >> > Sorry to burst your bubble, but the networking drivers on NXP LS1028A >> > (device tree at arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi, drivers >> > at drivers/net/ethernet/freescale/enetc/ and drivers/net/dsa/ocelot/) >> > do not use the Lynx PCS through a pcs-handle, because the Lynx PCS in >> > fact has no backing OF node there, nor do the internal MDIO buses of the >> > ENETC and of the switch. >> > >> > It seems that I need to point this out explicitly: you need to provide >> > at least a working migration path to your PCS driver model. Currently >> > there isn't one, and as a result, networking is broken on the LS1028A >> > with this patch set. >> > >> >> Please refer to patches 4, 5, and 6. > > I don't understand, could you be more clear? Are you saying that I > shouldn't have applied patch 9 while testing? When would be a good > moment to apply patch 9? I'm saying that patches 4 and 5 [1] provide "...a working migration path to [my] PCS driver model." Since enetc/ocelot do not use devicetree for the PCS, patch 9 should have no effect. That said, if you've tested this on actual hardware, I'm interested in your results. I do not have access to enetc/ocelot hardware, so I was unable to test whether my proposed migration would work. --Sean [1] I listed 6 but it seems like it just has some small hunks which should have been in 5 instead
On Tue, Jul 19, 2022 at 11:46:23AM -0400, Sean Anderson wrote: > I'm saying that patches 4 and 5 [1] provide "...a working migration > path to [my] PCS driver model." Since enetc/ocelot do not use > devicetree for the PCS, patch 9 should have no effect. > > That said, if you've tested this on actual hardware, I'm interested > in your results. I do not have access to enetc/ocelot hardware, so > I was unable to test whether my proposed migration would work. > > --Sean > > [1] I listed 6 but it seems like it just has some small hunks which should have been in 5 instead Got it, thanks. So things actually work up until the end, after fixing the compilation errors and warnings and applying my phy_mask patch first. However, as mentioned by Russell King, this patch set now gives us the possibility of doing this, which happily kills the system: echo "0000:00:00.5-imdio:03" > /sys/bus/mdio_bus/drivers/lynx-pcs/unbind For your information, pcs-rzn1-miic.c already has a device_link_add() call to its consumer, and it does avoid the unbinding problem. It is a bit of a heavy hammer as Russell points out (a DSA switch is a single struct device, but has multiple net_devices and phylink instances, and the switch device would be unregistered in its entirety), but on the other hand, this is one of the simpler things we can do, until we have something more fine-grained. I, for one, am perfectly happy with a device link. The alternative would be reworking phylink to react on PCS devices coming and going. I don't even know what the implications are upon mac_select_pcs() and such...
On Tue, Jul 19, 2022 at 03:34:45PM -0400, Sean Anderson wrote: > We could do it, but it'd be a pretty big hack. Something like the > following. Phylink would need to be modified to grab the lock before > every op and check if the PCS is dead or not. This is of course still > not optimal, since there's no way to re-attach a PCS once it goes away. You assume it's just phylink who operates on a PCS structure, but if you include your search pool to also cover include/linux/pcs/pcs-xpcs.h, you'll see a bunch of exported functions which are called directly by the client drivers (stmmac, sja1105). At this stage it gets pretty hard to validate that drivers won't attempt from any code path to do something stupid with a dead PCS. All in all it creates an environment with insanely weak guarantees; that's pretty hard to get behind IMO. > IMO a better solution is to use devlink and submit a patch to add > notifications which the MAC driver can register for. That way it can > find out when the PCS goes away and potentially do something about it > (or just let itself get removed). Not sure I understand what connection there is between devlink (device links) and PCS {de}registration notifications. We could probably add those notifications without any intervention from the device core: we would just need to make this new PCS "core" to register an blocking_notifier_call_chain to which interested drivers could add their notifier blocks. How a certain phylink user is going to determine that "hey, this PCS is definitely mine and I can use it" is an open question. In any case, my expectation is that we have a notifier chain, we can at least continue operating (avoid unbinding the struct device), but essentially move our phylink_create/phylink_destroy calls to within those notifier blocks. Again, retrofitting this model to existing drivers, phylink API (and maybe even its internal structure) is something that's hard to hop on board of; I think it's a solution waiting for a problem, and I don't have an interest to develop or even review it.
On 7/20/22 9:53 AM, Vladimir Oltean wrote: > On Tue, Jul 19, 2022 at 03:34:45PM -0400, Sean Anderson wrote: >> We could do it, but it'd be a pretty big hack. Something like the >> following. Phylink would need to be modified to grab the lock before >> every op and check if the PCS is dead or not. This is of course still >> not optimal, since there's no way to re-attach a PCS once it goes away. > > You assume it's just phylink who operates on a PCS structure, but if you > include your search pool to also cover include/linux/pcs/pcs-xpcs.h, > you'll see a bunch of exported functions which are called directly by > the client drivers (stmmac, sja1105). At this stage it gets pretty hard > to validate that drivers won't attempt from any code path to do > something stupid with a dead PCS. All in all it creates an environment > with insanely weak guarantees; that's pretty hard to get behind IMO. Right. To do this properly, we'd need wrapper functions for all the PCS operations. And the super-weak guarantees is why I referred to this as a "hack". But we could certainly make it so that removing a PCS might not bring down the MAC. >> IMO a better solution is to use devlink and submit a patch to add >> notifications which the MAC driver can register for. That way it can >> find out when the PCS goes away and potentially do something about it >> (or just let itself get removed). > > Not sure I understand what connection there is between devlink (device > links) and PCS {de}registration notifications. The default action when a supplier is going to be removed is to remove the consumers. However, it'd be nice to notify the consumer beforehand. If we used device links, this would need to be integrated (since otherwise we'd only find out that a PCS was gone after the MAC was gone too). > We could probably add those > notifications without any intervention from the device core: we would > just need to make this new PCS "core" to register an blocking_notifier_call_chain > to which interested drivers could add their notifier blocks. How a> certain phylink user is going to determine that "hey, this PCS is > definitely mine and I can use it" is an open question. In any case, my > expectation is that we have a notifier chain, we can at least continue > operating (avoid unbinding the struct device), but essentially move our > phylink_create/phylink_destroy calls to within those notifier blocks. > Again, retrofitting this model to existing drivers, phylink API (and > maybe even its internal structure) is something that's hard to hop on > board of; I think it's a solution waiting for a problem, and I don't > have an interest to develop or even review it. I don't either. I'd much rather just bring down the whole MAC when any PCS gets removed. Whatever we decide on doing here should also be done for (serdes) phys as well, since they have all the same pitfalls. For that reason I'd rather use a generic, non-intrusive solution like device links. I know Russell mentioned composite devices, but I think those would have similar advantages/drawbacks as a device-link-based solution (unbinding of one device unbinds the rest). --Sean