Re: [PATCH 03/12] drm/xe/pf: Drop GuC notifications for non-existing VF

["Piotr Piórkowski" <piotr.piorkowski@intel.com>]

Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on pon [2024-sie-19 19:51:28 +0200]:
> 
> 
> On 16.08.2024 15:01, Piotr Piórkowski wrote:
> > Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on pią [2024-sie-09 18:51:50 +0200]:
> >> It is unlikely that GuC will ever send a G2H notification with an
> >> invalid VFID and it is currently harmless if that actually happen.
> >> But in upcoming patches we will start using that VFID as an index
> >> and we must be sure it is a valid to avoid a crash due to a buggy
> >> firmware or a currupted G2H message.
> >>
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> ---
> >>  drivers/gpu/drm/xe/xe_gt_sriov_pf_control.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_control.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_control.c
> >> index d11839d9e7d4..ad447d867e51 100644
> >> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_control.c
> >> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_control.c
> >> @@ -8,6 +8,7 @@
> >>  #include "xe_device.h"
> >>  #include "xe_gt.h"
> >>  #include "xe_gt_sriov_pf_control.h"
> >> +#include "xe_gt_sriov_pf_helpers.h"
> >>  #include "xe_gt_sriov_printk.h"
> >>  #include "xe_guc_ct.h"
> >>  #include "xe_sriov.h"
> >> @@ -212,6 +213,11 @@ static void pf_handle_vf_flr_done(struct xe_gt *gt, u32 vfid)
> >>  
> >>  static int pf_handle_vf_event(struct xe_gt *gt, u32 vfid, u32 eventid)
> >>  {
> >> +	xe_gt_sriov_dbg_verbose(gt, "received VF%u event %#x\n", vfid, eventid);
> >> +
> >> +	if (vfid > xe_gt_sriov_pf_get_totalvfs(gt))
> >> +		return -EPROTO;
> > 
> > I would probably add some log here.
> > I'm afraid that during debug the information about the last event is not enough
> 
> it should be the CTB layer responsibility to print an error message and
> dump this invalid or corrupted or unexpected G2H message, no point in
> doing this in every possible G2H message handler; note that 'silent'
> return is aligned with the existing code in this file.
> 
> for now CTB will just report:
> 
> 	xe_gt_err(gt, "G2H action 0x%04x failed (%pe)\n",
> 		  action, ERR_PTR(ret));
> 
> but it should be easy to add (what we had in i915) full dump of the
> problematic G2H message in case of at least the EPROTO error


This sounds reasonable to me:
Reviewed-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> 
> > 
> > Thanks,
> > Piotr
> >> +
> >>  	switch (eventid) {
> >>  	case GUC_PF_NOTIFY_VF_FLR:
> >>  		pf_handle_vf_flr(gt, vfid);
> >> -- 
> >> 2.43.0
> >>
> > 

--