| Message ID | 20240502045748.37627-1-rengarajan.s@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1] lan78xx: Fix crash with multiple device attach | expand |
On Thu, May 02, 2024 at 10:27:48AM +0530, Rengarajan S wrote: > After the first device(MAC + PHY) is attached, the corresponding > fixup gets registered and before it is unregistered next device > is attached causing the dev pointer of second device to be NULL. > Fixed the issue with multiple PHY attach by unregistering PHY > at the end of probe. Removed the unregistration during phy_init > since the handling has been taken care in probe. > > Fixes: 89b36fb5e532 ("lan78xx: Lan7801 Support for Fixed PHY") > Signed-off-by: Rengarajan S <rengarajan.s@microchip.com> > --- > > drivers/net/usb/lan78xx.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - You have marked a patch with a "Fixes:" tag for a commit that is in an older released kernel, yet you do not have a cc: stable line in the signed-off-by area at all, which means that the patch will not be applied to any older kernel releases. To properly fix this, please follow the documented rules in the Documentation/process/stable-kernel-rules.rst file for how to resolve this. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot
On Thu, 2024-05-02 at 10:27 +0530, Rengarajan S wrote: > After the first device(MAC + PHY) is attached, the corresponding > fixup gets registered and before it is unregistered next device > is attached causing the dev pointer of second device to be NULL. > Fixed the issue with multiple PHY attach by unregistering PHY > at the end of probe. Removed the unregistration during phy_init > since the handling has been taken care in probe. The above description is unclear to me. Could you please list the exact sequence of events/calls that lead to the problem? > Fixes: 89b36fb5e532 ("lan78xx: Lan7801 Support for Fixed PHY") > Signed-off-by: Rengarajan S <rengarajan.s@microchip.com> > --- > > drivers/net/usb/lan78xx.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c > index 5add4145d..3ec79620f 100644 > --- a/drivers/net/usb/lan78xx.c > +++ b/drivers/net/usb/lan78xx.c > @@ -2383,14 +2383,8 @@ static int lan78xx_phy_init(struct lan78xx_net *dev) > netdev_err(dev->net, "can't attach PHY to %s\n", > dev->mdiobus->id); > if (dev->chipid == ID_REV_CHIP_ID_7801_) { > - if (phy_is_pseudo_fixed_link(phydev)) { > + if (phy_is_pseudo_fixed_link(phydev)) > fixed_phy_unregister(phydev); > - } else { > - phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, > - 0xfffffff0); > - phy_unregister_fixup_for_uid(PHY_LAN8835, > - 0xfffffff0); > - } > } > return -EIO; > } > @@ -4458,6 +4452,14 @@ static int lan78xx_probe(struct usb_interface *intf, > pm_runtime_set_autosuspend_delay(&udev->dev, > DEFAULT_AUTOSUSPEND_DELAY); > > + /* Unregistering Fixup to avoid crash with multiple device > + * attach. > + */ > + phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, > + 0xfffffff0); > + phy_unregister_fixup_for_uid(PHY_LAN8835, > + 0xfffffff0); > + Minor nit: the above 2 statments can now fit a single line each. Thanks, Paolo
Hi Paolo, Thanks for Reviewing the patch. Please find my comments inline. On Mon, 2024-05-06 at 11:38 +0200, Paolo Abeni wrote: > [You don't often get email from pabeni@redhat.com. Learn why this is > important at https://aka.ms/LearnAboutSenderIdentification ] > > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Thu, 2024-05-02 at 10:27 +0530, Rengarajan S wrote: > > After the first device(MAC + PHY) is attached, the corresponding > > fixup gets registered and before it is unregistered next device > > is attached causing the dev pointer of second device to be NULL. > > Fixed the issue with multiple PHY attach by unregistering PHY > > at the end of probe. Removed the unregistration during phy_init > > since the handling has been taken care in probe. > > The above description is unclear to me. Could you please list the > exact > sequence of events/calls that lead to the problem? The issue was when dual setup of LAN7801 with an external PHY(LAN8841 in this case) are connected to the same DUT PC, the PC got hanged. The issue in seen with external phys only and not observed in case of internal PHY being connected(LAN7800). When we looked into the code flow we found that in phy_scan_fixup allocates a dev for the first device. Before it is unregistered, the second device is attached and since we already have a phydev it ignores and does not allocate dev for second device. This is the reason why we unregister the first device before the second device attach. > > > Fixes: 89b36fb5e532 ("lan78xx: Lan7801 Support for Fixed PHY") > > Signed-off-by: Rengarajan S <rengarajan.s@microchip.com> > > --- > > > > drivers/net/usb/lan78xx.c | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c > > index 5add4145d..3ec79620f 100644 > > --- a/drivers/net/usb/lan78xx.c > > +++ b/drivers/net/usb/lan78xx.c > > @@ -2383,14 +2383,8 @@ static int lan78xx_phy_init(struct > > lan78xx_net *dev) > > netdev_err(dev->net, "can't attach PHY to %s\n", > > dev->mdiobus->id); > > if (dev->chipid == ID_REV_CHIP_ID_7801_) { > > - if (phy_is_pseudo_fixed_link(phydev)) { > > + if (phy_is_pseudo_fixed_link(phydev)) > > fixed_phy_unregister(phydev); > > - } else { > > - > > phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, > > - > > 0xfffffff0); > > - > > phy_unregister_fixup_for_uid(PHY_LAN8835, > > - > > 0xfffffff0); > > - } > > } > > return -EIO; > > } > > @@ -4458,6 +4452,14 @@ static int lan78xx_probe(struct > > usb_interface *intf, > > pm_runtime_set_autosuspend_delay(&udev->dev, > > DEFAULT_AUTOSUSPEND_DELAY); > > > > + /* Unregistering Fixup to avoid crash with multiple device > > + * attach. > > + */ > > + phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, > > + 0xfffffff0); > > + phy_unregister_fixup_for_uid(PHY_LAN8835, > > + 0xfffffff0); > > + > > Minor nit: the above 2 statments can now fit a single line each. Sure. Will update it in the next revision. > > Thanks, > > Paolo >
> The issue was when dual setup of LAN7801 with an external PHY(LAN8841 > in this case) are connected to the same DUT PC, the PC got hanged. The > issue in seen with external phys only and not observed in case of > internal PHY being connected(LAN7800). When we looked into the code > flow we found that in phy_scan_fixup allocates a dev for the first > device. Before it is unregistered, the second device is attached and > since we already have a phydev it ignores and does not allocate dev for > second device. This is the reason why we unregister the first device > before the second device attach. This is not making any sense to me. What this driver is doing odd is registers a fixup per device. So if you plug in 42 USB dongles, you get the same fixup registered 42 times. What normally happens is that the fixup is registered once, globally. So you probably want to move the registration of the fixup into the module init function, and the unregister into the module exit function. Andrew
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c index 5add4145d..3ec79620f 100644 --- a/drivers/net/usb/lan78xx.c +++ b/drivers/net/usb/lan78xx.c @@ -2383,14 +2383,8 @@ static int lan78xx_phy_init(struct lan78xx_net *dev) netdev_err(dev->net, "can't attach PHY to %s\n", dev->mdiobus->id); if (dev->chipid == ID_REV_CHIP_ID_7801_) { - if (phy_is_pseudo_fixed_link(phydev)) { + if (phy_is_pseudo_fixed_link(phydev)) fixed_phy_unregister(phydev); - } else { - phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, - 0xfffffff0); - phy_unregister_fixup_for_uid(PHY_LAN8835, - 0xfffffff0); - } } return -EIO; } @@ -4458,6 +4452,14 @@ static int lan78xx_probe(struct usb_interface *intf, pm_runtime_set_autosuspend_delay(&udev->dev, DEFAULT_AUTOSUSPEND_DELAY); + /* Unregistering Fixup to avoid crash with multiple device + * attach. + */ + phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, + 0xfffffff0); + phy_unregister_fixup_for_uid(PHY_LAN8835, + 0xfffffff0); + return 0; out8:
After the first device(MAC + PHY) is attached, the corresponding fixup gets registered and before it is unregistered next device is attached causing the dev pointer of second device to be NULL. Fixed the issue with multiple PHY attach by unregistering PHY at the end of probe. Removed the unregistration during phy_init since the handling has been taken care in probe. Fixes: 89b36fb5e532 ("lan78xx: Lan7801 Support for Fixed PHY") Signed-off-by: Rengarajan S <rengarajan.s@microchip.com> --- drivers/net/usb/lan78xx.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)