[PATCH v2] drm/xe: Don't expose mailbox attributes for skip_pcode cases

[Raag Jadav]

On Wed, Aug 06, 2025 at 09:25:03PM +0530, Summers, Stuart wrote:
> On Wed, 2025-08-06 at 20:52 +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. Since this is true for cases where
> > skip_pcode
> > flag is set, a much robust fix would be to not expose mailbox
> > attributes
> > at all for such cases.
> > 
> > v2: Check against skip_pcode (Stuart)
> > 
> > Fixes: a7f87deac229 ("drm/xe: Default auto_link_downgrade status to
> > false")
> > Signed-off-by: Raag Jadav <raag.jadav at intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_device_sysfs.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_device_sysfs.c
> > b/drivers/gpu/drm/xe/xe_device_sysfs.c
> > index bd9015761aa0..75b6cd816fa8 100644
> > --- a/drivers/gpu/drm/xe/xe_device_sysfs.c
> > +++ b/drivers/gpu/drm/xe/xe_device_sysfs.c
> > @@ -264,8 +264,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;
> 
> Why are we making this change again? Looking at xe_pcode_read, which
> calls pcode_mailbox_rw with this value, the first thing we're doing
> with this is a write:
> xe_mmio_write32(mmio, PCODE_DATA0, *data0);
> 
> There's no NULL check here like we have for the second parameter in
> xe_pcode_read. So we need some initial value here so we don't write
> garbage.

Data-in value is only relevant for mailboxes that use it. So with
the introduction of skip_pcode fix here, not much use of redundant
initialization.

Raag

> >         int ret;
> >  
> >         xe_pm_runtime_get(xe);
> > @@ -291,7 +290,7 @@ static void xe_device_sysfs_fini(void *arg)
> >         if (xe->d3cold.capable)
> >                 sysfs_remove_file(&xe->drm.dev->kobj,
> > &dev_attr_vram_d3cold_threshold.attr);
> >  
> > -       if (xe->info.platform == XE_BATTLEMAGE) {
> > +       if (xe->info.platform == XE_BATTLEMAGE && !xe-
> > >info.skip_pcode) {
> >                 sysfs_remove_files(&xe->drm.dev->kobj,
> > auto_link_downgrade_attrs);
> >                 late_bind_remove_files(xe->drm.dev);
> >         }
> > @@ -308,7 +307,7 @@ int xe_device_sysfs_init(struct xe_device *xe)
> >                         return ret;
> >         }
> >  
> > -       if (xe->info.platform == XE_BATTLEMAGE) {
> > +       if (xe->info.platform == XE_BATTLEMAGE && !xe-
> > >info.skip_pcode) {
> 
> This part looks good. We have skip_pcode in the pcode_mailbox_rw
> routine, but these files are checking values here that don't make sense
> if we aren't reading them.
> 
> Thanks,
> Stuart
> 
> >                 ret = sysfs_create_files(&dev->kobj,
> > auto_link_downgrade_attrs);
> >                 if (ret)
> >                         return ret;
>