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@xxxxxxxxxx. 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@xxxxxxxxxxxxx>
> > ---
> > 
> >  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
>