Re: [PATCH v2 2/2] drm/xe/sysfs: Simplify sysfs registration

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

On Wed, Sep 17, 2025 at 12:24:24PM +0200, Michal Wajdeczko wrote:
> On 9/17/2025 8:18 AM, Raag Jadav wrote:
> > On Tue, Sep 16, 2025 at 07:00:29PM +0200, Michal Wajdeczko wrote:
> > > Instead of manually maintaining each sysfs file define and use
> > > attribute groups and register them using device managed function.
> > > Then use is_visible() to filter-out unsupported attributes.
> > >
> > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Raag Jadav <raag.jadav@intel.com>
> > > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > > v2: rebased, reordered
> > > ---
> > >  drivers/gpu/drm/xe/xe_device_sysfs.c | 104 +++++++++++----------------
> > >  1 file changed, 41 insertions(+), 63 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_device_sysfs.c b/drivers/gpu/drm/xe/xe_device_sysfs.c
> > > index 896484c8fbcc..c5151c86a98a 100644
> > > --- a/drivers/gpu/drm/xe/xe_device_sysfs.c
> > > +++ b/drivers/gpu/drm/xe/xe_device_sysfs.c
> > > @@ -71,6 +71,15 @@ vram_d3cold_threshold_store(struct device *dev, struct device_attribute *attr,
> > >  
> > >  static DEVICE_ATTR_RW(vram_d3cold_threshold);
> > >  
> > > +static struct attribute *vram_attrs[] = {
> > > +	&dev_attr_vram_d3cold_threshold.attr,
> > > +	NULL
> > > +};
> > > +
> > > +static const struct attribute_group vram_attr_group = {
> > > +	.attrs = vram_attrs,
> > > +};
> > > +
> > >  static ssize_t
> > >  lb_fan_control_version_show(struct device *dev, struct device_attribute *attr, char *buf)
> > >  {
> > > @@ -149,8 +158,16 @@ lb_voltage_regulator_version_show(struct device *dev, struct device_attribute *a
> > >  }
> > >  static DEVICE_ATTR_ADMIN_RO(lb_voltage_regulator_version);
> > >  
> > > -static int late_bind_create_files(struct device *dev)
> > > +static struct attribute *late_bind_attrs[] = {
> > > +	&dev_attr_lb_fan_control_version.attr,
> > > +	&dev_attr_lb_voltage_regulator_version.attr,
> > > +	NULL
> > > +};
> > > +
> > > +static umode_t late_bind_attr_is_visible(struct kobject *kobj,
> > > +					 struct attribute *attr, int n)
> > >  {
> > > +	struct device *dev = kobj_to_dev(kobj);
> > >  	struct xe_device *xe = pdev_to_xe_device(to_pci_dev(dev));
> > >  	struct xe_tile *root = xe_device_get_root_tile(xe);
> > >  	u32 cap = 0;
> > > @@ -160,51 +177,25 @@ static int late_bind_create_files(struct device *dev)
> > >  
> > >  	ret = xe_pcode_read(root, PCODE_MBOX(PCODE_LATE_BINDING, GET_CAPABILITY_STATUS, 0),
> > >  			    &cap, NULL);
> > > -	if (ret) {
> > > -		if (ret == -ENXIO) {
> > > -			drm_dbg(&xe->drm, "Late binding not supported by firmware\n");
> > > -			ret = 0;
> > > -		}
> > > -		goto out;
> > > -	}
> > > -
> > > -	if (REG_FIELD_GET(V1_FAN_SUPPORTED, cap)) {
> > > -		ret = sysfs_create_file(&dev->kobj, &dev_attr_lb_fan_control_version.attr);
> > > -		if (ret)
> > > -			goto out;
> > > -	}
> > > -
> > > -	if (REG_FIELD_GET(VR_PARAMS_SUPPORTED, cap))
> > > -		ret = sysfs_create_file(&dev->kobj, &dev_attr_lb_voltage_regulator_version.attr);
> > > -out:
> > 
> > >  	xe_pm_runtime_put(xe);
> > 
> > The device is already in D0 while probing so runtime PM is probably
> > not needed here.
> 
> true, but this is how it was initially coded and I didn't want to change it now
> 
> we can either drop it completely or replace with
> 
> 	xe_pm_runtime_get_noresume(xe);
> 	xe_pm_runtime_put(xe);
> 
> do you want this to be done as part of this series or it can be done later as follow-up?

Since there's no functional impact, I don't see any harm in dropping
it as part of refactoring here. But whatever the maintainers think is
best.

Raag