* [bug report] net: usb: lan78xx: refactor PHY init to separate detection and MAC configuration
@ 2025-05-09 9:03 Dan Carpenter
2025-05-09 12:43 ` Oleksij Rempel
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2025-05-09 9:03 UTC (permalink / raw)
To: Oleksij Rempel; +Cc: kernel-janitors
Hello Oleksij Rempel,
Commit d39f339d2603 ("net: usb: lan78xx: refactor PHY init to
separate detection and MAC configuration") from May 5, 2025
(linux-next), leads to the following Smatch static checker warning:
drivers/net/usb/lan78xx.c:2842 lan78xx_phy_init()
warn: missing unwind goto?
drivers/net/usb/lan78xx.c
2805 static int lan78xx_phy_init(struct lan78xx_net *dev)
2806 {
2807 __ETHTOOL_DECLARE_LINK_MODE_MASK(fc) = { 0, };
2808 int ret;
2809 u32 mii_adv;
2810 struct phy_device *phydev;
2811
2812 phydev = lan78xx_get_phy(dev);
2813 if (IS_ERR(phydev))
2814 return PTR_ERR(phydev);
2815
2816 ret = lan78xx_mac_prepare_for_phy(dev);
2817 if (ret < 0)
2818 goto free_phy;
2819
2820 /* if phyirq is not set, use polling mode in phylib */
2821 if (dev->domain_data.phyirq > 0)
2822 phydev->irq = dev->domain_data.phyirq;
2823 else
2824 phydev->irq = PHY_POLL;
2825 netdev_dbg(dev->net, "phydev->irq = %d\n", phydev->irq);
2826
2827 /* set to AUTOMDIX */
2828 phydev->mdix = ETH_TP_MDI_AUTO;
2829
2830 ret = phy_connect_direct(dev->net, phydev,
2831 lan78xx_link_status_change,
2832 dev->interface);
2833 if (ret) {
2834 netdev_err(dev->net, "can't attach PHY to %s\n",
2835 dev->mdiobus->id);
2836 if (dev->chipid == ID_REV_CHIP_ID_7801_) {
2837 if (phy_is_pseudo_fixed_link(phydev)) {
2838 fixed_phy_unregister(phydev);
2839 phy_device_free(phydev);
2840 }
2841 }
Why does this error path only cleanup for ID_REV_CHIP_ID_7801_ where the
others do it unconditionally?
--> 2842 return -EIO;
2843 }
2844
2845 /* MAC doesn't support 1000T Half */
2846 phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
2847
2848 /* support both flow controls */
2849 dev->fc_request_control = (FLOW_CTRL_RX | FLOW_CTRL_TX);
2850 linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT,
2851 phydev->advertising);
2852 linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
2853 phydev->advertising);
2854 mii_adv = (u32)mii_advertise_flowctrl(dev->fc_request_control);
2855 mii_adv_to_linkmode_adv_t(fc, mii_adv);
2856 linkmode_or(phydev->advertising, fc, phydev->advertising);
2857
2858 phy_support_eee(phydev);
2859
2860 ret = lan78xx_configure_leds_from_dt(dev, phydev);
2861 if (ret)
2862 goto free_phy;
2863
2864 genphy_config_aneg(phydev);
2865
2866 dev->fc_autoneg = phydev->autoneg;
2867
2868 return 0;
2869
2870 free_phy:
2871 if (phy_is_pseudo_fixed_link(phydev)) {
2872 fixed_phy_unregister(phydev);
2873 phy_device_free(phydev);
2874 }
2875
2876 return ret;
2877 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] net: usb: lan78xx: refactor PHY init to separate detection and MAC configuration
2025-05-09 9:03 [bug report] net: usb: lan78xx: refactor PHY init to separate detection and MAC configuration Dan Carpenter
@ 2025-05-09 12:43 ` Oleksij Rempel
2025-05-09 12:54 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Oleksij Rempel @ 2025-05-09 12:43 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors
On Fri, May 09, 2025 at 12:03:26PM +0300, Dan Carpenter wrote:
> Hello Oleksij Rempel,
>
> Commit d39f339d2603 ("net: usb: lan78xx: refactor PHY init to
> separate detection and MAC configuration") from May 5, 2025
> (linux-next), leads to the following Smatch static checker warning:
>
> drivers/net/usb/lan78xx.c:2842 lan78xx_phy_init()
> warn: missing unwind goto?
>
> drivers/net/usb/lan78xx.c
> 2805 static int lan78xx_phy_init(struct lan78xx_net *dev)
> 2806 {
> 2807 __ETHTOOL_DECLARE_LINK_MODE_MASK(fc) = { 0, };
> 2808 int ret;
> 2809 u32 mii_adv;
> 2810 struct phy_device *phydev;
> 2811
> 2812 phydev = lan78xx_get_phy(dev);
> 2813 if (IS_ERR(phydev))
> 2814 return PTR_ERR(phydev);
> 2815
> 2816 ret = lan78xx_mac_prepare_for_phy(dev);
> 2817 if (ret < 0)
> 2818 goto free_phy;
> 2819
> 2820 /* if phyirq is not set, use polling mode in phylib */
> 2821 if (dev->domain_data.phyirq > 0)
> 2822 phydev->irq = dev->domain_data.phyirq;
> 2823 else
> 2824 phydev->irq = PHY_POLL;
> 2825 netdev_dbg(dev->net, "phydev->irq = %d\n", phydev->irq);
> 2826
> 2827 /* set to AUTOMDIX */
> 2828 phydev->mdix = ETH_TP_MDI_AUTO;
> 2829
> 2830 ret = phy_connect_direct(dev->net, phydev,
> 2831 lan78xx_link_status_change,
> 2832 dev->interface);
> 2833 if (ret) {
> 2834 netdev_err(dev->net, "can't attach PHY to %s\n",
> 2835 dev->mdiobus->id);
> 2836 if (dev->chipid == ID_REV_CHIP_ID_7801_) {
> 2837 if (phy_is_pseudo_fixed_link(phydev)) {
> 2838 fixed_phy_unregister(phydev);
> 2839 phy_device_free(phydev);
> 2840 }
> 2841 }
>
> Why does this error path only cleanup for ID_REV_CHIP_ID_7801_ where the
> others do it unconditionally?
This chip-specific condition in the cleanup can be used, but it’s not
strictly necessary - none of the variants introduce regressions. The
non-conditional cleanup actually matches the logic already used in
lan78xx_disconnect(), where phy_is_pseudo_fixed_link() is checked
unconditionally.
That said, the purpose of this patch set is to prepare for migration to
phylink, where pseudo fixed links won’t be used at all. Due to the
10-patch limit, I’ve split the changes - the patch that removes pseudo
fixed-link support entirely will follow.
Nevertheless, I can send a cleanup patch that unconditionally jumps to
free_phy on phy_connect_direct() failure for consistency and clarity.
Should I?
Best Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] net: usb: lan78xx: refactor PHY init to separate detection and MAC configuration
2025-05-09 12:43 ` Oleksij Rempel
@ 2025-05-09 12:54 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2025-05-09 12:54 UTC (permalink / raw)
To: Oleksij Rempel; +Cc: kernel-janitors
On Fri, May 09, 2025 at 02:43:12PM +0200, Oleksij Rempel wrote:
> On Fri, May 09, 2025 at 12:03:26PM +0300, Dan Carpenter wrote:
> > Hello Oleksij Rempel,
> >
> > Commit d39f339d2603 ("net: usb: lan78xx: refactor PHY init to
> > separate detection and MAC configuration") from May 5, 2025
> > (linux-next), leads to the following Smatch static checker warning:
> >
> > drivers/net/usb/lan78xx.c:2842 lan78xx_phy_init()
> > warn: missing unwind goto?
> >
> > drivers/net/usb/lan78xx.c
> > 2805 static int lan78xx_phy_init(struct lan78xx_net *dev)
> > 2806 {
> > 2807 __ETHTOOL_DECLARE_LINK_MODE_MASK(fc) = { 0, };
> > 2808 int ret;
> > 2809 u32 mii_adv;
> > 2810 struct phy_device *phydev;
> > 2811
> > 2812 phydev = lan78xx_get_phy(dev);
> > 2813 if (IS_ERR(phydev))
> > 2814 return PTR_ERR(phydev);
> > 2815
> > 2816 ret = lan78xx_mac_prepare_for_phy(dev);
> > 2817 if (ret < 0)
> > 2818 goto free_phy;
> > 2819
> > 2820 /* if phyirq is not set, use polling mode in phylib */
> > 2821 if (dev->domain_data.phyirq > 0)
> > 2822 phydev->irq = dev->domain_data.phyirq;
> > 2823 else
> > 2824 phydev->irq = PHY_POLL;
> > 2825 netdev_dbg(dev->net, "phydev->irq = %d\n", phydev->irq);
> > 2826
> > 2827 /* set to AUTOMDIX */
> > 2828 phydev->mdix = ETH_TP_MDI_AUTO;
> > 2829
> > 2830 ret = phy_connect_direct(dev->net, phydev,
> > 2831 lan78xx_link_status_change,
> > 2832 dev->interface);
> > 2833 if (ret) {
> > 2834 netdev_err(dev->net, "can't attach PHY to %s\n",
> > 2835 dev->mdiobus->id);
> > 2836 if (dev->chipid == ID_REV_CHIP_ID_7801_) {
> > 2837 if (phy_is_pseudo_fixed_link(phydev)) {
> > 2838 fixed_phy_unregister(phydev);
> > 2839 phy_device_free(phydev);
> > 2840 }
> > 2841 }
> >
> > Why does this error path only cleanup for ID_REV_CHIP_ID_7801_ where the
> > others do it unconditionally?
>
> This chip-specific condition in the cleanup can be used, but it’s not
> strictly necessary - none of the variants introduce regressions. The
> non-conditional cleanup actually matches the logic already used in
> lan78xx_disconnect(), where phy_is_pseudo_fixed_link() is checked
> unconditionally.
>
> That said, the purpose of this patch set is to prepare for migration to
> phylink, where pseudo fixed links won’t be used at all. Due to the
> 10-patch limit, I’ve split the changes - the patch that removes pseudo
> fixed-link support entirely will follow.
>
> Nevertheless, I can send a cleanup patch that unconditionally jumps to
> free_phy on phy_connect_direct() failure for consistency and clarity.
> Should I?
Don't do anything on my account. These messages are only intended to be
helpful, so ignore them when they're not. People with questions know to
look it up on lore and they will find this thread.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-05-09 12:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-09 9:03 [bug report] net: usb: lan78xx: refactor PHY init to separate detection and MAC configuration Dan Carpenter
2025-05-09 12:43 ` Oleksij Rempel
2025-05-09 12:54 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.