| Message ID | 20200925024247.993-1-yongxin.liu@windriver.com |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "net: ethernet: ixgbe: check the return value of ixgbe_mii_bus_init()" | expand |
On Fri, Sep 25, 2020 at 4:45 AM Yongxin Liu <yongxin.liu@windriver.com> wrote: > > This reverts commit 09ef193fef7efb0175a04634853862d717adbb95. > > For C3000 family of SoCs, they have four ixgbe devices sharing a single MDIO bus. > ixgbe_mii_bus_init() returns -ENODEV for other three devices. The propagation > of the error code makes other three ixgbe devices unregistered. > > Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > index 2f8a4cfc5fa1..5e5223becf86 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > @@ -11031,14 +11031,10 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > IXGBE_LINK_SPEED_10GB_FULL | IXGBE_LINK_SPEED_1GB_FULL, > true); > > - err = ixgbe_mii_bus_init(hw); > - if (err) > - goto err_netdev; > + ixgbe_mii_bus_init(hw); > > return 0; > > -err_netdev: > - unregister_netdev(netdev); > err_register: > ixgbe_release_hw_control(adapter); > ixgbe_clear_interrupt_scheme(adapter); > -- > 2.14.4 > Then we should check if err == -ENODEV, not outright ignore all potential errors, right? Bartosz
> -----Original Message----- > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > Sent: Friday, September 25, 2020 16:15 > To: Liu, Yongxin <Yongxin.Liu@windriver.com> > Cc: David S . Miller <davem@davemloft.net>; netdev > <netdev@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH] Revert "net: ethernet: ixgbe: check the return value > of ixgbe_mii_bus_init()" > > On Fri, Sep 25, 2020 at 4:45 AM Yongxin Liu <yongxin.liu@windriver.com> > wrote: > > > > This reverts commit 09ef193fef7efb0175a04634853862d717adbb95. > > > > For C3000 family of SoCs, they have four ixgbe devices sharing a single > MDIO bus. > > ixgbe_mii_bus_init() returns -ENODEV for other three devices. The > > propagation of the error code makes other three ixgbe devices > unregistered. > > > > Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com> > > --- > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 +----- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > index 2f8a4cfc5fa1..5e5223becf86 100644 > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > @@ -11031,14 +11031,10 @@ static int ixgbe_probe(struct pci_dev *pdev, > const struct pci_device_id *ent) > > IXGBE_LINK_SPEED_10GB_FULL | > IXGBE_LINK_SPEED_1GB_FULL, > > true); > > > > - err = ixgbe_mii_bus_init(hw); > > - if (err) > > - goto err_netdev; > > + ixgbe_mii_bus_init(hw); > > > > return 0; > > > > -err_netdev: > > - unregister_netdev(netdev); > > err_register: > > ixgbe_release_hw_control(adapter); > > ixgbe_clear_interrupt_scheme(adapter); > > -- > > 2.14.4 > > > > Then we should check if err == -ENODEV, not outright ignore all potential > errors, right? > Hm, it is weird to take -ENODEV as a no error. How about just return 0 instead of -ENODEV in the following function? <drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c> 902 s32 ixgbe_mii_bus_init(struct ixgbe_hw *hw) 903 { . . . 913 switch (hw->device_id) { 914 /* C3000 SoCs */ 915 case IXGBE_DEV_ID_X550EM_A_KR: 916 case IXGBE_DEV_ID_X550EM_A_KR_L: 917 case IXGBE_DEV_ID_X550EM_A_SFP_N: 918 case IXGBE_DEV_ID_X550EM_A_SGMII: 919 case IXGBE_DEV_ID_X550EM_A_SGMII_L: 920 case IXGBE_DEV_ID_X550EM_A_10G_T: 921 case IXGBE_DEV_ID_X550EM_A_SFP: 922 case IXGBE_DEV_ID_X550EM_A_1G_T: 923 case IXGBE_DEV_ID_X550EM_A_1G_T_L: 924 if (!ixgbe_x550em_a_has_mii(hw)) 925 return -ENODEV; Thanks, Yongxin > Bartosz
On Fri, Sep 25, 2020 at 10:51 AM Liu, Yongxin <Yongxin.Liu@windriver.com> wrote: > [snip] > > > true); > > > > > > - err = ixgbe_mii_bus_init(hw); > > > - if (err) > > > - goto err_netdev; > > > + ixgbe_mii_bus_init(hw); > > > > > > return 0; > > > > > > -err_netdev: > > > - unregister_netdev(netdev); > > > err_register: > > > ixgbe_release_hw_control(adapter); > > > ixgbe_clear_interrupt_scheme(adapter); > > > -- > > > 2.14.4 > > > > > > > Then we should check if err == -ENODEV, not outright ignore all potential > > errors, right? > > > > Hm, it is weird to take -ENODEV as a no error. > How about just return 0 instead of -ENODEV in the following function? > No, it's perfectly fine. -ENODEV means there's no device and this can happen. The caller can then act accordingly - for example: ignore that fact. We do it in several places[1]. Bartosz [snip] [1] https://elixir.bootlin.com/linux/latest/source/drivers/misc/eeprom/at24.c#L714
> -----Original Message----- > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > Sent: Friday, September 25, 2020 17:44 > To: Liu, Yongxin <Yongxin.Liu@windriver.com> > Cc: David S . Miller <davem@davemloft.net>; netdev > <netdev@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH] Revert "net: ethernet: ixgbe: check the return value > of ixgbe_mii_bus_init()" > > On Fri, Sep 25, 2020 at 10:51 AM Liu, Yongxin <Yongxin.Liu@windriver.com> > wrote: > > > > [snip] > > > > > true); > > > > > > > > - err = ixgbe_mii_bus_init(hw); > > > > - if (err) > > > > - goto err_netdev; > > > > + ixgbe_mii_bus_init(hw); > > > > > > > > return 0; > > > > > > > > -err_netdev: > > > > - unregister_netdev(netdev); > > > > err_register: > > > > ixgbe_release_hw_control(adapter); > > > > ixgbe_clear_interrupt_scheme(adapter); > > > > -- > > > > 2.14.4 > > > > > > > > > > Then we should check if err == -ENODEV, not outright ignore all > > > potential errors, right? > > > > > > > Hm, it is weird to take -ENODEV as a no error. > > How about just return 0 instead of -ENODEV in the following function? > > > > No, it's perfectly fine. -ENODEV means there's no device and this can > happen. The caller can then act accordingly - for example: ignore that > fact. We do it in several places[1]. > > Bartosz > > [snip] > > [1] > https://elixir.bootlin.com/linux/latest/source/drivers/misc/eeprom/at24.c# > L714 Okay, please go ahead and fix this issue. Thanks, Yongxin
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index 2f8a4cfc5fa1..5e5223becf86 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -11031,14 +11031,10 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent) IXGBE_LINK_SPEED_10GB_FULL | IXGBE_LINK_SPEED_1GB_FULL, true); - err = ixgbe_mii_bus_init(hw); - if (err) - goto err_netdev; + ixgbe_mii_bus_init(hw); return 0; -err_netdev: - unregister_netdev(netdev); err_register: ixgbe_release_hw_control(adapter); ixgbe_clear_interrupt_scheme(adapter);
This reverts commit 09ef193fef7efb0175a04634853862d717adbb95. For C3000 family of SoCs, they have four ixgbe devices sharing a single MDIO bus. ixgbe_mii_bus_init() returns -ENODEV for other three devices. The propagation of the error code makes other three ixgbe devices unregistered. Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com> --- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)