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 >
