Re: [Intel-wired-lan] [PATCH net-next v5 4/5] ice: disable FW logging on driver unload

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

On Fri, Jan 13, 2023 at 02:13:04PM -0800, Paul M Stillwell Jr wrote:
> On 1/12/2023 2:49 AM, Michal Swiatkowski wrote:
> > On Wed, Jan 11, 2023 at 11:19:05AM -0800, Paul M Stillwell Jr wrote:
> > > The FW is running in it's own context irregardless of what the driver
> > > is doing. In this case, if the driver previously registered for FW
> > > log events and then the driver unloads without informing the FW to
> > > unregister for FW log events then the FW still has a timer running to
> > > output FW logs.
> > > 
> > > The next time the driver loads and tries to register for FW log events
> > > then the FW returns an error, but still enables the continued
> > > outputting of FW logs. This causes an IO error to devlink which isn't
> > > intuitive since the logs are still being output.
> > > 
> > > Fix this by disabling FW logging when the driver is being unloaded.
> > > 
> > > Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> > > ---
> > >   drivers/net/ethernet/intel/ice/ice_main.c | 29 +++++++++++++++++++++++
> > >   1 file changed, 29 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> > > index 1b5debc3109d..593efc064f5b 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_main.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> > > @@ -4500,6 +4500,33 @@ static void ice_unregister_netdev(struct ice_vsi *vsi)
> > >   	clear_bit(ICE_VSI_NETDEV_REGISTERED, vsi->state);
> > >   }
> > > +/**
> > > + * ice_pf_fwlog_deinit - clear FW logging metadata on device exit
> > > + * @pf: pointer to the PF struct
> > > + */
> > > +static void ice_pf_fwlog_deinit(struct ice_pf *pf)
> > > +{
> > > +	struct ice_hw *hw = &pf->hw;
> > > +
> > > +	/* make sure FW logging is disabled to not put the FW in a weird state
> > > +	 * for the next driver load
> > > +	 */
> > > +	if (hw->fwlog_ena) {
> > > +		int status;
> > > +
> > > +		hw->fwlog_cfg.options &= ~ICE_FWLOG_OPTION_ARQ_ENA;
> > > +		status = ice_fwlog_set(hw, &hw->fwlog_cfg);
> > > +		if (status)
> > > +			dev_warn(ice_pf_to_dev(pf), "Unable to turn off FW logging, status: %d\n",
> > > +				 status);
> > > +
> > > +		status = ice_fwlog_unregister(hw);
> > > +		if (status)
> > > +			dev_warn(ice_pf_to_dev(pf), "Unable to unregister FW logging, status: %d\n",
> > > +				 status);
> > Shouldn't hw->fwlog_cfg be free on deinit? Or if not here, where does
> > this happen?
> > 
> 
> No need to free hw->fwlog_cfg since it's not a pointer with memory
> allocated, it's a member of ice_hw. See ice_type.h, 863
> 

Sure, didn't notice that, thanks

> > > +	}
> > > +}
> > > +
> > >   /**
> > >    * ice_cfg_netdev - Allocate, configure and register a netdev
> > >    * @vsi: the VSI associated with the new netdev
> > > @@ -5237,6 +5264,8 @@ static void ice_remove(struct pci_dev *pdev)
> > >   		msleep(100);
> > >   	}
> > > +	ice_pf_fwlog_deinit(pf);
> > > +
> > >   	if (test_bit(ICE_FLAG_SRIOV_ENA, pf->flags)) {
> > >   		set_bit(ICE_VF_RESETS_DISABLED, pf->state);
> > >   		ice_free_vfs(pf);
> > > -- 
> > > 2.35.1
> > > 
> > > _______________________________________________
> > > Intel-wired-lan mailing list
> > > Intel-wired-lan@osuosl.org
> > > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> 
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan