[PATCH net v1] lan78xx: Fix crash with multiple device attach

[PATCH net v1] lan78xx: Fix crash with multiple device attach

[Rengarajan S]

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(-)

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:
-- 
2.25.1

Re: [PATCH net v1] lan78xx: Fix crash with multiple device attach

[Greg KH]

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

Re: [PATCH net v1] lan78xx: Fix crash with multiple device attach

[Paolo Abeni]

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

Re: [PATCH net v1] lan78xx: Fix crash with multiple device attach

[Rengarajan.S]

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
> 

Re: [PATCH net v1] lan78xx: Fix crash with multiple device attach

[Andrew Lunn]

> 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