* [PATCH] Revert "net: ethernet: ixgbe: check the return value of ixgbe_mii_bus_init()"
@ 2020-09-25 2:42 Yongxin Liu
2020-09-25 8:14 ` Bartosz Golaszewski
0 siblings, 1 reply; 5+ messages in thread
From: Yongxin Liu @ 2020-09-25 2:42 UTC (permalink / raw)
To: bgolaszewski, davem; +Cc: netdev, linux-kernel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Revert "net: ethernet: ixgbe: check the return value of ixgbe_mii_bus_init()"
2020-09-25 2:42 [PATCH] Revert "net: ethernet: ixgbe: check the return value of ixgbe_mii_bus_init()" Yongxin Liu
@ 2020-09-25 8:14 ` Bartosz Golaszewski
2020-09-25 8:51 ` Liu, Yongxin
0 siblings, 1 reply; 5+ messages in thread
From: Bartosz Golaszewski @ 2020-09-25 8:14 UTC (permalink / raw)
To: Yongxin Liu; +Cc: David S . Miller, netdev, LKML
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] Revert "net: ethernet: ixgbe: check the return value of ixgbe_mii_bus_init()"
2020-09-25 8:14 ` Bartosz Golaszewski
@ 2020-09-25 8:51 ` Liu, Yongxin
2020-09-25 9:43 ` Bartosz Golaszewski
0 siblings, 1 reply; 5+ messages in thread
From: Liu, Yongxin @ 2020-09-25 8:51 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: David S . Miller, netdev, LKML
> -----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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Revert "net: ethernet: ixgbe: check the return value of ixgbe_mii_bus_init()"
2020-09-25 8:51 ` Liu, Yongxin
@ 2020-09-25 9:43 ` Bartosz Golaszewski
2020-09-26 23:27 ` Liu, Yongxin
0 siblings, 1 reply; 5+ messages in thread
From: Bartosz Golaszewski @ 2020-09-25 9:43 UTC (permalink / raw)
To: Liu, Yongxin; +Cc: David S . Miller, netdev, LKML
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] Revert "net: ethernet: ixgbe: check the return value of ixgbe_mii_bus_init()"
2020-09-25 9:43 ` Bartosz Golaszewski
@ 2020-09-26 23:27 ` Liu, Yongxin
0 siblings, 0 replies; 5+ messages in thread
From: Liu, Yongxin @ 2020-09-26 23:27 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: David S . Miller, netdev, LKML
> -----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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-26 23:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-25 2:42 [PATCH] Revert "net: ethernet: ixgbe: check the return value of ixgbe_mii_bus_init()" Yongxin Liu
2020-09-25 8:14 ` Bartosz Golaszewski
2020-09-25 8:51 ` Liu, Yongxin
2020-09-25 9:43 ` Bartosz Golaszewski
2020-09-26 23:27 ` Liu, Yongxin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).