RE: [PATCH] lan78xx: Don't reset the interface on open

[Nisar.Sayed]

Hi Phil,

> Hi Nisar,
> 
> On 10/04/2018 15:16, nisar.sa...@microchip.com wrote:
> > Thanks Phil, for identifying the issues.
> >
> >> -  ret = lan78xx_reset(dev);
> >> -  if (ret < 0)
> >> -  goto done;
> >> -
> >>phy_start(net->phydev);
> >>
> >>netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
> >> --
> >
> > You may need to start the interrupts before "phy_start" instead of
> suppressing call to "lan78xx_reset".
> >
> > + if (dev->domain_data.phyirq > 0)
> > + phy_start_interrupts(dev->net->phydev);
> 
> Shouldn't phy_connect_direct, called from lan78xx_phy_init, already have
> enabled interrupts for us?
> 
> This patch addresses two problems - time wasted by renegotiating the link
> after the reset and the missed interrupt - and I'd like both to be fixed. 
> Unless
> you can come up with a good reason for performing the reset from the open
> handler I think it should be removed.
> 
> Phil

Thanks, we have verified suspected test cases and these are passed, the changes 
are good to go.

- Nisar

RE: [PATCH] lan78xx: Don't reset the interface on open

[Nisar.Sayed]

Thanks Phil, for identifying the issues.

> - ret = lan78xx_reset(dev);
> - if (ret < 0)
> - goto done;
> -
>   phy_start(net->phydev);
> 
>   netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
> --

You may need to start the interrupts before "phy_start" instead of suppressing 
call to "lan78xx_reset".

+ if (dev->domain_data.phyirq > 0)
+ phy_start_interrupts(dev->net->phydev);

- Nisar

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] lan78xx: Connect phy early

[Nisar.Sayed]

Hi Alexander,

Thanks for the patch.

> @@ -2575,13 +2571,7 @@ static int lan78xx_stop(struct net_device *net)
>   if (timer_pending(>stat_monitor))
>   del_timer_sync(>stat_monitor);
> 
> - phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfff0);
> - phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfff0);
> -
>   phy_stop(net->phydev);
> - phy_disconnect(net->phydev);
> -
> - net->phydev = NULL;
> 
>   clear_bit(EVENT_DEV_OPEN, >flags);
>   netif_stop_queue(net);

Please do add valid "phydev"  check before phy_stop, since "phy_disconnect" 
should be called before "unregister_netdev"

+ if (net->phydev)
phy_stop(net->phydev);

> @@ -3481,6 +3471,11 @@ static void lan78xx_disconnect(struct
> usb_interface *intf)
>   net = dev->net;
>   unregister_netdev(net);
> 
> + phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfff0);
> + phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfff0);
> +
> + phy_disconnect(net->phydev);
> +
>   cancel_delayed_work_sync(>wq);
> 
>   usb_scuttle_anchored_urbs(>deferred);

Please move "unregister_netdev" after "phy_disconnect", otherwise 
"phy_disconnect" will fail while we disconnect USB.

> @@ -3634,6 +3629,10 @@ static int lan78xx_probe(struct usb_interface
> *intf,
>   pm_runtime_set_autosuspend_delay(>dev,
>DEFAULT_AUTOSUSPEND_DELAY);
> 
> + ret = lan78xx_phy_init(dev);
> + if (ret < 0)
> + return ret;
> +
>   return 0;
> 
>  out3:
> --
> 2.12.3

We should "goto out4" instead of "return" upon "lan78xx_phy_init" fail

+ out4:
+   unregister_netdev(netdev);

In addition to current changes, you might have to take care of the following.

In function "lan78xx_reset_resume"

-  lan78xx_phy_init(dev);
+ phy_start(dev->net->phydev);

Thanks,
Sd.Nisar


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html