diff mbox series

[net-next,v3,08/15] ice: don't always return an error for Get PHY Abilities AQ command

Message ID 20201113214429.2131951-9-anthony.l.nguyen@intel.com
State Superseded
Headers show
Series 100GbE Intel Wired LAN Driver Updates 2020-11-13 | expand

Commit Message

Tony Nguyen Nov. 13, 2020, 9:44 p.m. UTC
From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>

There are times when the driver shouldn't return an error when the Get
PHY abilities AQ command (0x0600) returns an error. Instead the driver
should log that the error occurred and continue on. This allows the
driver to load even though the AQ command failed. The user can then
later determine the reason for the failure and correct it.

Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alexander Duyck Nov. 14, 2020, 1:25 a.m. UTC | #1
On Fri, Nov 13, 2020 at 1:49 PM Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
>
> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
>
> There are times when the driver shouldn't return an error when the Get
> PHY abilities AQ command (0x0600) returns an error. Instead the driver
> should log that the error occurred and continue on. This allows the
> driver to load even though the AQ command failed. The user can then
> later determine the reason for the failure and correct it.
>
> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index 7db5fd977367..3c600808d0da 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -925,7 +925,7 @@ enum ice_status ice_init_hw(struct ice_hw *hw)
>                                      ICE_AQC_REPORT_TOPO_CAP, pcaps, NULL);
>         devm_kfree(ice_hw_to_dev(hw), pcaps);
>         if (status)
> -               goto err_unroll_sched;
> +               ice_debug(hw, ICE_DBG_PHY, "Get PHY capabilities failed, continuing anyway\n");
>
>         /* Initialize port_info struct with link information */
>         status = ice_aq_get_link_info(hw->port_info, false, NULL, NULL);
> --
> 2.26.2
>

If we are expecting the user to correct things then we should be
putting out a warning via dev_warn() rather than a debug message.
Otherwise the user is just going to have to come back through and turn
on debugging and reload the driver in order to figure out what is
going on. In my mind it should get the same treatment as an outdated
NVM image in ice_aq_ver_check().
Tony Nguyen Nov. 21, 2020, 12:43 a.m. UTC | #2
On Fri, 2020-11-13 at 17:25 -0800, Alexander Duyck wrote:
> On Fri, Nov 13, 2020 at 1:49 PM Tony Nguyen <
> anthony.l.nguyen@intel.com> wrote:
> > 
> > From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> > 
> > There are times when the driver shouldn't return an error when the
> > Get
> > PHY abilities AQ command (0x0600) returns an error. Instead the
> > driver
> > should log that the error occurred and continue on. This allows the
> > driver to load even though the AQ command failed. The user can then
> > later determine the reason for the failure and correct it.
> > 
> > Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> > Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice_common.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c
> > b/drivers/net/ethernet/intel/ice/ice_common.c
> > index 7db5fd977367..3c600808d0da 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_common.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> > @@ -925,7 +925,7 @@ enum ice_status ice_init_hw(struct ice_hw *hw)
> >                                      ICE_AQC_REPORT_TOPO_CAP,
> > pcaps, NULL);
> >         devm_kfree(ice_hw_to_dev(hw), pcaps);
> >         if (status)
> > -               goto err_unroll_sched;
> > +               ice_debug(hw, ICE_DBG_PHY, "Get PHY capabilities
> > failed, continuing anyway\n");
> > 
> >         /* Initialize port_info struct with link information */
> >         status = ice_aq_get_link_info(hw->port_info, false, NULL,
> > NULL);
> > --
> > 2.26.2
> > 
> 
> If we are expecting the user to correct things then we should be
> putting out a warning via dev_warn() rather than a debug message.
> Otherwise the user is just going to have to come back through and
> turn
> on debugging and reload the driver in order to figure out what is
> going on. In my mind it should get the same treatment as an outdated
> NVM image in ice_aq_ver_check().

Will change this to a dev_warn().

Thanks,
Tony
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 7db5fd977367..3c600808d0da 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -925,7 +925,7 @@  enum ice_status ice_init_hw(struct ice_hw *hw)
 				     ICE_AQC_REPORT_TOPO_CAP, pcaps, NULL);
 	devm_kfree(ice_hw_to_dev(hw), pcaps);
 	if (status)
-		goto err_unroll_sched;
+		ice_debug(hw, ICE_DBG_PHY, "Get PHY capabilities failed, continuing anyway\n");
 
 	/* Initialize port_info struct with link information */
 	status = ice_aq_get_link_info(hw->port_info, false, NULL, NULL);