| Message ID | 20221128214749.110-5-paul.m.stillwell.jr@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | add v2 FW logging for ice driver | expand |
On 11/28/2022 10:47 PM, 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_devlink.c | 32 +++++++++++++++----- > 1 file changed, 24 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c > index ca67f2741f83..923556e6ae79 100644 > --- a/drivers/net/ethernet/intel/ice/ice_devlink.c > +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c > @@ -378,6 +378,10 @@ static int ice_devlink_info_get(struct devlink *devlink, > enum ice_devlink_param_id { > ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, > ICE_DEVLINK_PARAM_ID_TX_BALANCE, > + ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED, > + ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED, > + ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL, > + ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION, My understanding was the the community doesn't like private devlink params. Was some investigation done whether the FW logging can be done using some generic API ? I believe we had a discussion about this a long time ago. Maybe I'm a bit out of a loop at this point :) > }; > > /** > @@ -1484,14 +1488,6 @@ ice_devlink_enable_iw_validate(struct devlink *devlink, u32 id, > return 0; > } > > -enum ice_devlink_param_id { > - ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, > - ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED, > - ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED, > - ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL, > - ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION, > -}; > - > static int > ice_devlink_fwlog_supported_get(struct devlink *devlink, u32 id, > struct devlink_param_gset_ctx *ctx) > @@ -1743,6 +1739,26 @@ void ice_devlink_register(struct ice_pf *pf) > */ > void ice_devlink_unregister(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); > + } > + > devlink_unregister(priv_to_devlink(pf)); > } >
On 11/29/2022 4:05 AM, Wilczynski, Michal wrote: > > > On 11/28/2022 10:47 PM, 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_devlink.c | 32 +++++++++++++++----- >> 1 file changed, 24 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c >> index ca67f2741f83..923556e6ae79 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c >> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c >> @@ -378,6 +378,10 @@ static int ice_devlink_info_get(struct devlink *devlink, >> enum ice_devlink_param_id { >> ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, >> ICE_DEVLINK_PARAM_ID_TX_BALANCE, >> + ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED, >> + ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED, >> + ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL, >> + ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION, > > My understanding was the the community doesn't like private devlink params. > Was some investigation done whether the FW logging can be done using some > generic API ? I believe we had a discussion about this a long time ago. Maybe > I'm a bit out of a loop at this point :) > Yeah, I looked at all the other devlink interfaces and couldn't find one that really worked. I considered adding a brand new interface, but didn't want to go that route unless we have to because I'm guessing that every device that can get FW logs has a slightly different way to do it. If you have a suggestion then I'm all ears :). >> }; >> >> /** >> @@ -1484,14 +1488,6 @@ ice_devlink_enable_iw_validate(struct devlink *devlink, u32 id, >> return 0; >> } >> >> -enum ice_devlink_param_id { >> - ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, >> - ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED, >> - ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED, >> - ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL, >> - ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION, >> -}; >> - >> static int >> ice_devlink_fwlog_supported_get(struct devlink *devlink, u32 id, >> struct devlink_param_gset_ctx *ctx) >> @@ -1743,6 +1739,26 @@ void ice_devlink_register(struct ice_pf *pf) >> */ >> void ice_devlink_unregister(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); >> + } >> + >> devlink_unregister(priv_to_devlink(pf)); >> } >> >
On 11/28/2022 1:47 PM, 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_devlink.c | 32 +++++++++++++++----- > 1 file changed, 24 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c > index ca67f2741f83..923556e6ae79 100644 > --- a/drivers/net/ethernet/intel/ice/ice_devlink.c > +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c > @@ -378,6 +378,10 @@ static int ice_devlink_info_get(struct devlink *devlink, > enum ice_devlink_param_id { > ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, > ICE_DEVLINK_PARAM_ID_TX_BALANCE, > + ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED, > + ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED, > + ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL, > + ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION, > }; > > /** > @@ -1484,14 +1488,6 @@ ice_devlink_enable_iw_validate(struct devlink *devlink, u32 id, > return 0; > } > > -enum ice_devlink_param_id { > - ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, > - ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED, > - ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED, > - ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL, > - ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION, > -}; Is there a reason you didn't add to the original enum in the previous patches and are it combining now? > - > static int > ice_devlink_fwlog_supported_get(struct devlink *devlink, u32 id, > struct devlink_param_gset_ctx *ctx) > @@ -1743,6 +1739,26 @@ void ice_devlink_register(struct ice_pf *pf) > */ > void ice_devlink_unregister(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); > + } > + > devlink_unregister(priv_to_devlink(pf)); > } >
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c index ca67f2741f83..923556e6ae79 100644 --- a/drivers/net/ethernet/intel/ice/ice_devlink.c +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c @@ -378,6 +378,10 @@ static int ice_devlink_info_get(struct devlink *devlink, enum ice_devlink_param_id { ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, ICE_DEVLINK_PARAM_ID_TX_BALANCE, + ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED, + ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED, + ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL, + ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION, }; /** @@ -1484,14 +1488,6 @@ ice_devlink_enable_iw_validate(struct devlink *devlink, u32 id, return 0; } -enum ice_devlink_param_id { - ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, - ICE_DEVLINK_PARAM_ID_FWLOG_SUPPORTED, - ICE_DEVLINK_PARAM_ID_FWLOG_ENABLED, - ICE_DEVLINK_PARAM_ID_FWLOG_LEVEL, - ICE_DEVLINK_PARAM_ID_FWLOG_RESOLUTION, -}; - static int ice_devlink_fwlog_supported_get(struct devlink *devlink, u32 id, struct devlink_param_gset_ctx *ctx) @@ -1743,6 +1739,26 @@ void ice_devlink_register(struct ice_pf *pf) */ void ice_devlink_unregister(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); + } + devlink_unregister(priv_to_devlink(pf)); }
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_devlink.c | 32 +++++++++++++++----- 1 file changed, 24 insertions(+), 8 deletions(-)