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

[Summers, Stuart]

On Wed, 2025-08-06 at 18:38 +0200, Raag Jadav wrote:
> 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.

I agree this isn't used when we have skip_pcode=true, but in the
skip_pcode=false case, we are still instantiating this sysfs entry and
accessing that will cause the write to pcode with undefined val here.

Thanks,
Stuart

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