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

[Summers, Stuart]

On Wed, 2025-08-06 at 16:46 +0000, Summers, Stuart wrote:
> 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.

Just to reiterate here. We have a variable that is being passed in as a
write (as I explained above) to an external interface - in this case
pcode. While it might be true that this particular value is ignored by
pcode today, the actual usage is outside of the control of the driver.
So IMO the prior code which did not initialize the value being passed
in here was incorrect. As such, now that we have correct code in place
which initializes the data being passed in here, I am against reverting
back to the old, uninitialized value.

I also sent a new patch that covers the rest of the pcode read
locations. I did review the write side as well and don't see anything
uninitialized there, so hopefully this should cover everything:
https://patchwork.freedesktop.org/series/152700/

Thanks,
Stuart

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