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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 21 Nov 2020 00:43:21 +0000
From:   "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
To:     "alexander.duyck@...il.com" <alexander.duyck@...il.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "Brown, Aaron F" <aaron.f.brown@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Stillwell Jr, Paul M" <paul.m.stillwell.jr@...el.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>
Subject: Re: [net-next v3 08/15] ice: don't always return an error for Get PHY
 Abilities AQ command

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@...el.com> wrote:
> > 
> > From: Paul M Stillwell Jr <paul.m.stillwell.jr@...el.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@...el.com>
> > Tested-by: Aaron Brown <aaron.f.brown@...el.com>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.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