Re: [PATCH v1] drm/xe: Don't expose device attributes for VFs

["Summers, Stuart" <stuart.summers@intel.com>]

On Sat, 2025-07-12 at 05:24 +0300, Raag Jadav wrote:
> On Sat, Jul 12, 2025 at 02:19:50AM +0530, Summers, Stuart wrote:
> > On Sat, 2025-07-12 at 00:38 +0530, Raag Jadav wrote:
> > > Commit a7f87deac229 ("drm/xe: Default auto_link_downgrade status
> > > to
> > > false")
> > > tried to fix the side effect of xe_pcode_read() returning
> > > successfully
> > > without valid data-out value when xe->info.skip_pcode is set.
> > > Since
> > > this
> > > is true for IS_SRIOV_VF() cases, a much robust fix would be to
> > > not
> > > expose
> > > device attributes for VFs, which covers wider pcode mailbox
> > > users.
> > 
> > I'm not fully understanding your reasoning here. Why can't we just
> > use
> > a check against skip_pcode in that case? VF might be the main user
> > there, but it isn't inconceivable that we will use that in other
> > scenarios too and then hit a similar problem of accessing an
> > uninitialized variable.
> 
> This also covers non-mailbox attributes which would be unusable for
> VFs.
> So we can either fix the commit message or use skip_pcode check
> against
> mailbox attributes, whichever makes more sense.

Can you add both skip_pcode and IS_SRIOV_VF to the check because these
really are independent cases.

Thanks,
Stuart

> 
> Raag
> 
> > > Fixes: a7f87deac229 ("drm/xe: Default auto_link_downgrade status
> > > to
> > > false")
> > > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_device_sysfs.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_device_sysfs.c
> > > b/drivers/gpu/drm/xe/xe_device_sysfs.c
> > > index e5fd0cd537bc..8250aa4620ee 100644
> > > --- a/drivers/gpu/drm/xe/xe_device_sysfs.c
> > > +++ b/drivers/gpu/drm/xe/xe_device_sysfs.c
> > > @@ -259,8 +259,7 @@ auto_link_downgrade_status_show(struct device
> > > *dev, struct device_attribute *att
> > >  {
> > >         struct pci_dev *pdev = to_pci_dev(dev);
> > >         struct xe_device *xe = pdev_to_xe_device(pdev);
> > > -       /* default the auto_link_downgrade status to 0 */
> > > -       u32 val = 0;
> > > +       u32 val;
> > >         int ret;
> > >  
> > >         xe_pm_runtime_get(xe);
> > > @@ -297,6 +296,9 @@ int xe_device_sysfs_init(struct xe_device
> > > *xe)
> > >         struct device *dev = xe->drm.dev;
> > >         int ret;
> > >  
> > > +       if (IS_SRIOV_VF(xe))
> > > +               return 0;
> > > +
> > >         if (xe->d3cold.capable) {
> > >                 ret = sysfs_create_file(&dev->kobj,
> > > &dev_attr_vram_d3cold_threshold.attr);
> > >                 if (ret)
> >