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

[Raag Jadav <raag.jadav@intel.com>]

On Mon, Jul 14, 2025 at 09:00:54PM +0530, Summers, Stuart wrote:
> 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.

So perhaps they should be independent patches?

Considering non-mailbox attributes aren't necessarily related to the
problem here, I'm in favor of using skip_pcode for now.

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)
> > > 
>