| Message ID | 20210912192805.1394305-1-vladimir.oltean@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,net] Revert "net: phy: Uniform PHY driver access" | expand |
> This reverts commit 3ac8eed62596387214869319379c1fcba264d8c6. > > I am not actually sure I follow the patch author's logic, because the > change does more than it says on the box, but this patch breaks > suspend/resume on NXP LS1028A and probably on any other systems which > have PHY devices with no driver bound, because the patch has removed the > "!phydev->drv" check without actually explaining why that is fine. The wrong assumption was that the driver is set for every device during probe before suspend. Intention of the patch was only clean up of to_phy_driver() usage. > static bool mdio_bus_phy_may_suspend(struct phy_device *phydev) > { > + struct device_driver *drv = phydev->mdio.dev.driver; > + struct phy_driver *phydrv = to_phy_driver(drv); > struct net_device *netdev = phydev->attached_dev; > > - if (!phydev->drv->suspend) > + if (!drv || !phydrv->suspend) > return false; > > /* PHY not attached? May suspend if the PHY has not already been I suggest to add the "!phydev->drv" check, but others may know it better than me. Gerhard
On Sun, Sep 12, 2021 at 10:49:25PM +0200, Gerhard Engleder wrote: > > This reverts commit 3ac8eed62596387214869319379c1fcba264d8c6. > > > > I am not actually sure I follow the patch author's logic, because the > > change does more than it says on the box, but this patch breaks > > suspend/resume on NXP LS1028A and probably on any other systems which > > have PHY devices with no driver bound, because the patch has removed the > > "!phydev->drv" check without actually explaining why that is fine. > > The wrong assumption was that the driver is set for every device during probe > before suspend. Intention of the patch was only clean up of > to_phy_driver() usage. I am not sure why "to_phy_driver" needs cleanup. Au contraire, I think the PHY library's usage of struct phy_device :: drv is what is strange and potentially buggy, it is the only subsystem I know of that keeps its own driver pointer rather than looking at struct device :: driver. I think this is largely for historical reasons (it has done this since the first commit), but it looks to me like to_phy_driver could be used as part of a larger macro called something like phydev_get_drv which retrieves the phy_driver from the phydev->mdio.dev.driver. I say it is buggy because when probing fails ("fails" includes things like -EPROBE_DEFER) it does not even bother to clear phydev->drv back to NULL, even though the device will not have a driver pointer. There are also other things which it does not clean up on probe failure, btw, each with its own interesting side effects. > > static bool mdio_bus_phy_may_suspend(struct phy_device *phydev) > > { > > + struct device_driver *drv = phydev->mdio.dev.driver; > > + struct phy_driver *phydrv = to_phy_driver(drv); > > struct net_device *netdev = phydev->attached_dev; > > > > - if (!phydev->drv->suspend) > > + if (!drv || !phydrv->suspend) > > return false; > > > > /* PHY not attached? May suspend if the PHY has not already been > > I suggest to add the "!phydev->drv" check, but others may know it > better than me. So in this case, the difference will be that with your change, a phy_probe that returns an error code like -EPROBE_DEFER will have the phydev->drv set, and it will not return false ("may not suspend") quickly, while the code in its original form will not attempt to suspend that PHY. The implication is that we may call the ->suspend method of a PHY that is deferring probe, _before_ the probe has actually succeeded. To me, making that change and moving the code in yet a third state is way outside of the scope, which was to restore it to a known working condition (aka bug fix). If you want to make that change, feel free, I will not.
> I am not sure why "to_phy_driver" needs cleanup. Au contraire, I think > the PHY library's usage of struct phy_device :: drv is what is strange > and potentially buggy, it is the only subsystem I know of that keeps its > own driver pointer rather than looking at struct device :: driver. There is one odd driver in the mix. Take a look at xilinx_gmii2rgmii.c. It probably could be done a better way, but that is what we have. Andrew
On Mon, Sep 13, 2021 at 08:49:19PM +0200, Andrew Lunn wrote: > > I am not sure why "to_phy_driver" needs cleanup. Au contraire, I think > > the PHY library's usage of struct phy_device :: drv is what is strange > > and potentially buggy, it is the only subsystem I know of that keeps its > > own driver pointer rather than looking at struct device :: driver. > > There is one odd driver in the mix. Take a look at xilinx_gmii2rgmii.c. > > It probably could be done a better way, but that is what we have. Interesting, to say the least. Also, is there any connection between that and the revert I'm proposing? So compared to other vendors, where the RGMII gasket is part of the MAC device, with Xilinx Zynq it is accessible via MDIO? This is not all that different from dpaa2-eth and dpaa2-mac which are different devices on the bus, with different drivers, and the phy-handle is present on the dpaa2-mac OF node, but the net_device is registered by the dpaa2-eth device, is it? It seems to be even simpler in fact, because the dpaa2-mac and dpaa2-eth can even connect/disconnect from each other at runtime, something which does not appear possible with the Xilinx MAC and its RGMII gasket. What was done in that case was that all drivers which could possibly connect to the DPMAC would need to call dpaa2_mac_connect(), this is done currently from dpaa2-eth and dpaa2-switch. It looks like it is said that this GMII2RGMII converter can be placed in front of any GMII MAC. Nice that there are zero in-tree users of "xlnx,gmii-to-rgmii-1.0" so that I could figure out exactly how that plays out in practice. If it is only a few drivers that use the GMII2RGMII converter, maybe something like that (export a few symbols from this driver, make all MAC drivers call them) could take less of a toll on the overall PHY library architecture. Note that the usage of priv->phy_dev, priv->phy_drv, priv->conv_phy_drv beats me. Why is "phy_dev" kept inside "priv" even though it is accessed only inside xgmiitorgmii_probe? Why does xgmiitorgmii_configure() need to be called from xgmiitorgmii_read_status() which in turn hooks into the attached PHY driver's phy_read_status()? Why does xgmiitorgmii_configure not get exported and called from an .adjust_link method or the phylink equivalent, like any other MAC-side hardware linked with the PHY library in the kernel?
On Tue, Sep 14, 2021 at 12:06:18PM +0000, Vladimir Oltean wrote: > On Mon, Sep 13, 2021 at 08:49:19PM +0200, Andrew Lunn wrote: > > > I am not sure why "to_phy_driver" needs cleanup. Au contraire, I think > > > the PHY library's usage of struct phy_device :: drv is what is strange > > > and potentially buggy, it is the only subsystem I know of that keeps its > > > own driver pointer rather than looking at struct device :: driver. > > > > There is one odd driver in the mix. Take a look at xilinx_gmii2rgmii.c. > > > > It probably could be done a better way, but that is what we have. > > Interesting, to say the least. Also, is there any connection between > that and the revert I'm proposing? If i remember correctly, Gerhard Engleder is actually using this, and ran into a problem because the wrong driver structure was used. > So compared to other vendors, where the RGMII gasket is part of the MAC > device, with Xilinx Zynq it is accessible via MDIO? Yes. Its control plane sits on the MDIO bus. Unfortunately, it does not have any ID registers, so it does not directly appear as a PHY. So it does interesting things it put itself in the control path to the real PHY. > It looks like it is said that this GMII2RGMII converter can be placed in > front of any GMII MAC. Nice that there are zero in-tree users of > "xlnx,gmii-to-rgmii-1.0" so that I could figure out exactly how that > plays out in practice. If you look back at the thread for that patch, i think Gerhard posted a DT fragment he is using. Hopefully it will get submitted as a full board description at some point. > Note that the usage of priv->phy_dev, priv->phy_drv, priv->conv_phy_drv > beats me. Why is "phy_dev" kept inside "priv" even though it is accessed > only inside xgmiitorgmii_probe? Why does xgmiitorgmii_configure() need to > be called from xgmiitorgmii_read_status() which in turn hooks into the > attached PHY driver's phy_read_status()? Why does xgmiitorgmii_configure > not get exported and called from an .adjust_link method or the phylink > equivalent, like any other MAC-side hardware linked with the PHY library > in the kernel? I was never happy with this driver. It got submitted before i went on vacation, i had a few rounds trying to get the submitter to refactor it and was mostly ignored. I left on vacation with lots of open review points, and when i got back it had been merged. And the original submitters never responded to my requests for improvements. Andrew
On Tue, Sep 14, 2021 at 04:33:25PM +0200, Andrew Lunn wrote: > On Tue, Sep 14, 2021 at 12:06:18PM +0000, Vladimir Oltean wrote: > > On Mon, Sep 13, 2021 at 08:49:19PM +0200, Andrew Lunn wrote: > > > > I am not sure why "to_phy_driver" needs cleanup. Au contraire, I think > > > > the PHY library's usage of struct phy_device :: drv is what is strange > > > > and potentially buggy, it is the only subsystem I know of that keeps its > > > > own driver pointer rather than looking at struct device :: driver. > > > > > > There is one odd driver in the mix. Take a look at xilinx_gmii2rgmii.c. > > > > > > It probably could be done a better way, but that is what we have. > > > > Interesting, to say the least. Also, is there any connection between > > that and the revert I'm proposing? > > If i remember correctly, Gerhard Engleder is actually using this, and > ran into a problem because the wrong driver structure was used. > > > So compared to other vendors, where the RGMII gasket is part of the MAC > > device, with Xilinx Zynq it is accessible via MDIO? > > Yes. Its control plane sits on the MDIO bus. Unfortunately, it does > not have any ID registers, so it does not directly appear as a PHY. So > it does interesting things it put itself in the control path to the > real PHY. > > > It looks like it is said that this GMII2RGMII converter can be placed in > > front of any GMII MAC. Nice that there are zero in-tree users of > > "xlnx,gmii-to-rgmii-1.0" so that I could figure out exactly how that > > plays out in practice. > > If you look back at the thread for that patch, i think Gerhard posted > a DT fragment he is using. Hopefully it will get submitted as a full > board description at some point. > > > Note that the usage of priv->phy_dev, priv->phy_drv, priv->conv_phy_drv > > beats me. Why is "phy_dev" kept inside "priv" even though it is accessed > > only inside xgmiitorgmii_probe? Why does xgmiitorgmii_configure() need to > > be called from xgmiitorgmii_read_status() which in turn hooks into the > > attached PHY driver's phy_read_status()? Why does xgmiitorgmii_configure > > not get exported and called from an .adjust_link method or the phylink > > equivalent, like any other MAC-side hardware linked with the PHY library > > in the kernel? > > I was never happy with this driver. It got submitted before i went on > vacation, i had a few rounds trying to get the submitter to refactor > it and was mostly ignored. I left on vacation with lots of open review > points, and when i got back it had been merged. And the original > submitters never responded to my requests for improvements. Sorry, this is a rabbit hole I really don't want to go into. Allowing it to override PHY driver functions in order to 'automagically' configure itself when the PHY driver does stuff is probably where the bad decision was, everything from there is just the resulting fallout. Why don't all MAC drivers just hook themselves into the PHY driver's ->read_status method and configure themselves from there?! Why do we even need adjust_link, phylink, any of that? It's just a small pointer/driver override, the PHY library supports it. I have dug up this discussion where your stance seemed to be that "you want the MAC phy-handle to point to the gmii_to_rgmii 'PHY'" https://lore.kernel.org/netdev/20190309161912.GD9000@lunn.ch/#t I am not really sure if that particular reply went towards making this driver's design any saner than it is. As explained by Harini Katakam in his reply to you, the GMII2RGMII converter is not a PHY, and should therefore not be treated like one. It is an RGMII gasket for the MAC. Treating it as a satellite device of the MAC, which happens by chance to sit on an MDIO bus, but having otherwise nothing to do with the PHY library, sounds like a more normal approach (please note that it is quite likely I am oversimplifying some things since I just learned about this).
On Tue, Sep 14, 2021 at 5:15 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > On Tue, Sep 14, 2021 at 04:33:25PM +0200, Andrew Lunn wrote: > > On Tue, Sep 14, 2021 at 12:06:18PM +0000, Vladimir Oltean wrote: > > > On Mon, Sep 13, 2021 at 08:49:19PM +0200, Andrew Lunn wrote: > > > > > I am not sure why "to_phy_driver" needs cleanup. Au contraire, I think > > > > > the PHY library's usage of struct phy_device :: drv is what is strange > > > > > and potentially buggy, it is the only subsystem I know of that keeps its > > > > > own driver pointer rather than looking at struct device :: driver. > > > > > > > > There is one odd driver in the mix. Take a look at xilinx_gmii2rgmii.c. > > > > > > > > It probably could be done a better way, but that is what we have. > > > > > > Interesting, to say the least. Also, is there any connection between > > > that and the revert I'm proposing? > > > > If i remember correctly, Gerhard Engleder is actually using this, and > > ran into a problem because the wrong driver structure was used. Yes, but that was about phy_loopback and was fixed in the commit before. With this commit I tried to fix the remaining similar problems like the wrong driver structure use in phy_loopback. But as explained by Vladimir I failed. So it is totally ok to revert this commit, no functionality is lost. > > > So compared to other vendors, where the RGMII gasket is part of the MAC > > > device, with Xilinx Zynq it is accessible via MDIO? > > > > Yes. Its control plane sits on the MDIO bus. Unfortunately, it does > > not have any ID registers, so it does not directly appear as a PHY. So > > it does interesting things it put itself in the control path to the > > real PHY. > > > > > It looks like it is said that this GMII2RGMII converter can be placed in > > > front of any GMII MAC. Nice that there are zero in-tree users of > > > "xlnx,gmii-to-rgmii-1.0" so that I could figure out exactly how that > > > plays out in practice. > > > > If you look back at the thread for that patch, i think Gerhard posted > > a DT fragment he is using. Hopefully it will get submitted as a full > > board description at some point. I submitted it, but Michal Simek argumented that dts files of FPGA logic shall not be part of mainline. I suggested that at least one reference platform for every FPGA based IP core should be allowed, but he said that no one is able to test it. So it seems that you will never see any dts file which contains FPGA logic in mainline. I will try to submit it again if anyone will support me? > > > Note that th e usage of priv->phy_dev, priv->phy_drv, priv->conv_phy_drv > > > beats me. Why is "phy_dev" kept inside "priv" even though it is accessed > > > only inside xgmiitorgmii_probe? Why does xgmiitorgmii_configure() need to > > > be called from xgmiitorgmii_read_status() which in turn hooks into the > > > attached PHY driver's phy_read_status()? Why does xgmiitorgmii_configure > > > not get exported and called from an .adjust_link method or the phylink > > > equivalent, like any other MAC-side hardware linked with the PHY library > > > in the kernel? > > > > I was never happy with this driver. It got submitted before i went on > > vacation, i had a few rounds trying to get the submitter to refactor > > it and was mostly ignored. I left on vacation with lots of open review > > points, and when i got back it had been merged. And the original > > submitters never responded to my requests for improvements. > > Sorry, this is a rabbit hole I really don't want to go into. Allowing it > to override PHY driver functions in order to 'automagically' configure > itself when the PHY driver does stuff is probably where the bad decision > was, everything from there is just the resulting fallout. > > Why don't all MAC drivers just hook themselves into the PHY driver's > ->read_status method and configure themselves from there?! Why do we > even need adjust_link, phylink, any of that? It's just a small > pointer/driver override, the PHY library supports it. > > I have dug up this discussion where your stance seemed to be that > "you want the MAC phy-handle to point to the gmii_to_rgmii 'PHY'" > https://lore.kernel.org/netdev/20190309161912.GD9000@lunn.ch/#t > > I am not really sure if that particular reply went towards making this > driver's design any saner than it is. As explained by Harini Katakam in > his reply to you, the GMII2RGMII converter is not a PHY, and should > therefore not be treated like one. It is an RGMII gasket for the MAC. > Treating it as a satellite device of the MAC, which happens by chance to > sit on an MDIO bus, but having otherwise nothing to do with the PHY > library, sounds like a more normal approach (please note that it is > quite likely I am oversimplifying some things since I just learned about > this). Just for information dts with working GMII2RGMII looks like this: tnsep0: ethernet@a0000000 { compatible = "engleder,tsnep"; reg = <0x0 0xa0000000 0x0 0x10000>; interrupts = <0 89 1>; interrupt-parent = <&gic>; local-mac-address = [00 00 00 00 00 00]; phy-mode = "rgmii"; phy-handle = <&phy1>; mdio { #address-cells = <1>; #size-cells = <0>; phy1: ethernet-phy@1 { reg = <1>; rxc-skew-ps = <1080>; }; gmiitorgmii@8 { compatible = "xlnx,gmii-to-rgmii-1.0"; reg = <8>; phy-handle = <&phy1>; }; }; }; Gerhard
> I submitted it, but Michal Simek argumented that dts files of FPGA > logic shall not be part of mainline. I suggested that at least one > reference platform for every FPGA based IP core should be allowed, > but he said that no one is able to test it. So it seems that you > will never see any dts file which contains FPGA logic in mainline. I > will try to submit it again if anyone will support me? My opinion: If there is a real product out in the field using this, the DT for the product can be in mainline. Reference Design Kits for ASICs are well supported in mainline. So the question is, is an FPGA sufficiently different to an ASIC that is should be treated differently? Do you have an off the shelf platform or something custom? How easy is it to get the platform which is used as an RDK? Can you make a bitstream available for anybody to use? Andrew
> > I submitted it, but Michal Simek argumented that dts files of FPGA > > logic shall not be part of mainline. I suggested that at least one > > reference platform for every FPGA based IP core should be allowed, > > but he said that no one is able to test it. So it seems that you > > will never see any dts file which contains FPGA logic in mainline. I > > will try to submit it again if anyone will support me? > > My opinion: If there is a real product out in the field using this, > the DT for the product can be in mainline. > > Reference Design Kits for ASICs are well supported in mainline. So the > question is, is an FPGA sufficiently different to an ASIC that is > should be treated differently? Do you have an off the shelf platform > or something custom? How easy is it to get the platform which is used > as an RDK? Can you make a bitstream available for anybody to use? At least in combination with the board I can see no difference between ASIC and FPGA. Usually a FPGA bitstream targets a specific board, so the devices within the FPGA can be treated like devices on the board. The reference platform is based on off the shelf stuff (Xilinx ZCU104 and Avnet AES-FMC-NETW1-G). At least I had no problem buying the boards. Yes, I can provide a bitstream for everybody. Gerhard
On Tue, Sep 14, 2021 at 07:14:12PM +0200, Gerhard Engleder wrote: > > > I submitted it, but Michal Simek argumented that dts files of FPGA > > > logic shall not be part of mainline. I suggested that at least one > > > reference platform for every FPGA based IP core should be allowed, > > > but he said that no one is able to test it. So it seems that you > > > will never see any dts file which contains FPGA logic in mainline. I > > > will try to submit it again if anyone will support me? > > > > My opinion: If there is a real product out in the field using this, > > the DT for the product can be in mainline. > > > > Reference Design Kits for ASICs are well supported in mainline. So the > > question is, is an FPGA sufficiently different to an ASIC that is > > should be treated differently? Do you have an off the shelf platform > > or something custom? How easy is it to get the platform which is used > > as an RDK? Can you make a bitstream available for anybody to use? > > At least in combination with the board I can see no difference between ASIC > and FPGA. Usually a FPGA bitstream targets a specific board, so the devices > within the FPGA can be treated like devices on the board. > > The reference platform is based on off the shelf stuff (Xilinx ZCU104 and Avnet > AES-FMC-NETW1-G). At least I had no problem buying the boards. > > Yes, I can provide a bitstream for everybody. My opinion is that Linux has gotten into the position of maintaining the central repository of device tree blobs by some sort of strange accident, and these blobs do not really have to describe "a real product out in the field" in order to have a place in that central repository, no matter where that might be hosted. On some platforms it is not even possible to change the device tree (easily) since it is provided by the firmware, nonetheless it is still valuable to be able to look at it for reference. So I think anyone should be able to post their toy TSN driver running on their toy bit stream described by their toy device tree, and not be too concerned that they are littering the kernel. I would leave it upon the device tree maintainers to figure out the scalability concern, after all Linux took it upon itself to manage the central reference of device trees. But I do agree that the hardware setup needs at least to be reasonably reproducible by somebody non-you. I think the implication of not welcoming this kind of work is marginalizing hardware vendors such as Xilinx, and their users ending up in a worse place than if the device trees had a place in the mainline kernel. I am not a Xilinx engineer nor a Xilinx customer, but I am dreading each time I get a support request for an NXP switch attached to a Zynq SoC, just because I am always told that the person I'm helping is trapped with some sort of odd and old SDK kernel with modifications and it is not possible for them to move to mainline. So I am really happy to see people getting past that barrier and submitting drivers developed on Xilinx SoCs, it would be even nicer if they had an unimpeded path to make forward progress with their work. Does this invalidate my point about the GMII2RGMII converter, where I said that it would be better for all MAC drivers to treat it like a satellite device, instead of stowing it inside the PHY library with all the hacks associated with that? After all, Gerhard's TSN endpoint driver has nothing to do with Xilinx per se, it is only by chance that it runs on Xilinx hardware, so it may seem reasonable for his driver to not need to explicitly manage the platform's RGMII gasket. His TSN endpoint might be ported to a different FPGA manufacturer with no such oddity. Having this device hidden in the PHY library makes his life easier (at least apparently, until he hits things that don't work as they should). So while that is a valid point, there might be other places to put that converter, which are not in the direct path of the attached PHY's driver. For example, phylink is the melting pot of a lot of devices, on-board PHYs, SFP modules with and without PHYs, PCSes, it may even gain support for retimers, this was brought up a while ago. So maybe it would happily deal with another off-label device, a standalone RGMII gasket. It could have a structure with generic ops such as what is in place for struct phylink_pcs, and it ensures that this will be programmed to the right speed, put in loopback, etc etc, automatically. MAC driver uses phylink, the device tree has a phandle to the rgmii-gasket, and it works. Odd, and may or may not be worth it depending on how much demand there is, but at least it's an option worth considering.
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 9e2891d8e8dd..ba5ad86ec826 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -233,9 +233,11 @@ static DEFINE_MUTEX(phy_fixup_lock); static bool mdio_bus_phy_may_suspend(struct phy_device *phydev) { + struct device_driver *drv = phydev->mdio.dev.driver; + struct phy_driver *phydrv = to_phy_driver(drv); struct net_device *netdev = phydev->attached_dev; - if (!phydev->drv->suspend) + if (!drv || !phydrv->suspend) return false; /* PHY not attached? May suspend if the PHY has not already been
This reverts commit 3ac8eed62596387214869319379c1fcba264d8c6. I am not actually sure I follow the patch author's logic, because the change does more than it says on the box, but this patch breaks suspend/resume on NXP LS1028A and probably on any other systems which have PHY devices with no driver bound, because the patch has removed the "!phydev->drv" check without actually explaining why that is fine. Examples why that is not fine: - There is an MDIO bus with a PHY device that doesn't have a specific PHY driver loaded, because mdiobus_register() automatically creates a PHY device for it but there is no specific PHY driver in the system. Normally under those circumstances, the generic PHY driver will be bound lazily to it (at phy_attach_direct time), but since no one has attempted to connect to that PHY device (yet), it will not have a driver. - There is an internal MDIO bus with PCS devices on it, for serial links such as SGMII. If the driver does not set bus->phy_mask = ~0 to prevent auto-scanning, PHY devices will get created for those PCSes too, and although those PHY devices are not used, they do not bother anybody either. PCS devices are usually managed in Linux as raw MDIO devices. Nonetheless, they do not have a PHY driver, nor does anybody attempt to connect to them (because they are not a PHY), and therefore this patch breaks that. The stack trace is: Unable to handle kernel NULL pointer dereference at virtual address 00000000000000e8 pc : mdio_bus_phy_suspend+0xd8/0xec lr : dpm_run_callback+0x38/0x90 Call trace: mdio_bus_phy_suspend+0xd8/0xec dpm_run_callback+0x38/0x90 __device_suspend+0x108/0x3cc dpm_suspend+0x140/0x210 dpm_suspend_start+0x7c/0xa0 suspend_devices_and_enter+0x13c/0x540 pm_suspend+0x2a4/0x330 Fixes: 3ac8eed62596 ("net: phy: Uniform PHY driver access") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/phy/phy_device.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)