diff mbox series

[2/3] hwmon: (coretemp) Remove unnecessary dependency of array index

Message ID 20231127131651.476795-3-rui.zhang@intel.com
State New
Headers
Series hwmon: (coretemp) Fix core count limitation |

Commit Message

Zhang, Rui Nov. 27, 2023, 1:16 p.m. UTC 
  When sensor_device_attribute pointer is available, use container_of() to
get the temp_data address.

This removes the unnecessary dependency of cached index in
pdata->core_data[].

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/hwmon/coretemp.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

Comments

Ashok Raj Dec. 1, 2023, 1:27 a.m. UTC | #1 
On Mon, Nov 27, 2023 at 09:16:50PM +0800, Zhang Rui wrote:
> When sensor_device_attribute pointer is available, use container_of() to
> get the temp_data address.
> 
> This removes the unnecessary dependency of cached index in
> pdata->core_data[].
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/hwmon/coretemp.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 6053ed3761c2..cef43fedbd58 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -342,7 +342,7 @@ static ssize_t show_label(struct device *dev,
>  {
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>  	struct platform_data *pdata = dev_get_drvdata(dev);
> -	struct temp_data *tdata = pdata->core_data[attr->index];
> +	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_LABEL]);
>  
>  	if (tdata->is_pkg_data)
>  		return sprintf(buf, "Package id %u\n", pdata->pkg_id);
> @@ -355,8 +355,7 @@ static ssize_t show_crit_alarm(struct device *dev,
>  {
>  	u32 eax, edx;
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct platform_data *pdata = dev_get_drvdata(dev);
> -	struct temp_data *tdata = pdata->core_data[attr->index];
> +	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_CRIT_ALARM]);
>  
>  	mutex_lock(&tdata->update_lock);
>  	rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx);
> @@ -369,8 +368,7 @@ static ssize_t show_tjmax(struct device *dev,
>  			struct device_attribute *devattr, char *buf)
>  {
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct platform_data *pdata = dev_get_drvdata(dev);
> -	struct temp_data *tdata = pdata->core_data[attr->index];
> +	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TJMAX]);
>  	int tjmax;
>  
>  	mutex_lock(&tdata->update_lock);
> @@ -384,8 +382,7 @@ static ssize_t show_ttarget(struct device *dev,
>  				struct device_attribute *devattr, char *buf)
>  {
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct platform_data *pdata = dev_get_drvdata(dev);
> -	struct temp_data *tdata = pdata->core_data[attr->index];
> +	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TTARGET]);
>  	int ttarget;
>  
>  	mutex_lock(&tdata->update_lock);
> @@ -402,8 +399,7 @@ static ssize_t show_temp(struct device *dev,
>  {
>  	u32 eax, edx;
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct platform_data *pdata = dev_get_drvdata(dev);
> -	struct temp_data *tdata = pdata->core_data[attr->index];
> +	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TEMP]);
>  	int tjmax;
>  
>  	mutex_lock(&tdata->update_lock);
> @@ -445,7 +441,6 @@ static int create_core_attrs(struct temp_data *tdata, struct device *dev,
>  		tdata->sd_attrs[i].dev_attr.attr.name = tdata->attr_name[i];
>  		tdata->sd_attrs[i].dev_attr.attr.mode = 0444;
>  		tdata->sd_attrs[i].dev_attr.show = rd_ptr[i];
> -		tdata->sd_attrs[i].index = attr_no;

I was naively thinking if we could nuke that "index". I can see that used
in couple macros, but seems like we can lose it?

Completely untested.. and uncertain :-) 


diff --git a/include/linux/hwmon-sysfs.h b/include/linux/hwmon-sysfs.h
index d896713359cd..4855893f9401 100644
--- a/include/linux/hwmon-sysfs.h
+++ b/include/linux/hwmon-sysfs.h
@@ -12,36 +12,35 @@
 
 struct sensor_device_attribute{
 	struct device_attribute dev_attr;
-	int index;
 };
 #define to_sensor_dev_attr(_dev_attr) \
 	container_of(_dev_attr, struct sensor_device_attribute, dev_attr)
 
-#define SENSOR_ATTR(_name, _mode, _show, _store, _index)	\
+#define SENSOR_ATTR(_name, _mode, _show, _store)	\
 	{ .dev_attr = __ATTR(_name, _mode, _show, _store),	\
-	  .index = _index }
+	  }
 
-#define SENSOR_ATTR_RO(_name, _func, _index)			\
+#define SENSOR_ATTR_RO(_name, _func)			\
 	SENSOR_ATTR(_name, 0444, _func##_show, NULL, _index)
 
-#define SENSOR_ATTR_RW(_name, _func, _index)			\
-	SENSOR_ATTR(_name, 0644, _func##_show, _func##_store, _index)
+#define SENSOR_ATTR_RW(_name, _func)			\
+	SENSOR_ATTR(_name, 0644, _func##_show, _func##_store)
 
-#define SENSOR_ATTR_WO(_name, _func, _index)			\
-	SENSOR_ATTR(_name, 0200, NULL, _func##_store, _index)
+#define SENSOR_ATTR_WO(_name, _func)			\
+	SENSOR_ATTR(_name, 0200, NULL, _func##_store)
 
-#define SENSOR_DEVICE_ATTR(_name, _mode, _show, _store, _index)	\
+#define SENSOR_DEVICE_ATTR(_name, _mode, _show, _store)	\
 struct sensor_device_attribute sensor_dev_attr_##_name		\
-	= SENSOR_ATTR(_name, _mode, _show, _store, _index)
+	= SENSOR_ATTR(_name, _mode, _show, _store)
 
-#define SENSOR_DEVICE_ATTR_RO(_name, _func, _index)		\
-	SENSOR_DEVICE_ATTR(_name, 0444, _func##_show, NULL, _index)
+#define SENSOR_DEVICE_ATTR_RO(_name, _func)		\
+	SENSOR_DEVICE_ATTR(_name, 0444, _func##_show, NULL)
 
 #define SENSOR_DEVICE_ATTR_RW(_name, _func, _index)		\
-	SENSOR_DEVICE_ATTR(_name, 0644, _func##_show, _func##_store, _index)
+	SENSOR_DEVICE_ATTR(_name, 0644, _func##_show, _func##_store)
 
-#define SENSOR_DEVICE_ATTR_WO(_name, _func, _index)		\
-	SENSOR_DEVICE_ATTR(_name, 0200, NULL, _func##_store, _index)
+#define SENSOR_DEVICE_ATTR_WO(_name, _func)		\
+	SENSOR_DEVICE_ATTR(_name, 0200, NULL, _func##_store)
 
 struct sensor_device_attribute_2 {
 	struct device_attribute dev_attr;
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
index 975da8e7f2a9..c3bbf2f7d6eb 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -239,7 +239,7 @@ hwm_power1_max_interval_store(struct device *dev,
 
 static SENSOR_DEVICE_ATTR(power1_max_interval, 0664,
 			  hwm_power1_max_interval_show,
-			  hwm_power1_max_interval_store, 0);
+			  hwm_power1_max_interval_store);
 
 static struct attribute *hwm_attributes[] = {
 	&sensor_dev_attr_power1_max_interval.dev_attr.attr,
Guenter Roeck Dec. 1, 2023, 3:26 a.m. UTC | #2 
On 11/30/23 17:27, Ashok Raj wrote:
> On Mon, Nov 27, 2023 at 09:16:50PM +0800, Zhang Rui wrote:
>> When sensor_device_attribute pointer is available, use container_of() to
>> get the temp_data address.
>>
>> This removes the unnecessary dependency of cached index in
>> pdata->core_data[].
>>
>> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>> ---
>>   drivers/hwmon/coretemp.c | 15 +++++----------
>>   1 file changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
>> index 6053ed3761c2..cef43fedbd58 100644
>> --- a/drivers/hwmon/coretemp.c
>> +++ b/drivers/hwmon/coretemp.c
>> @@ -342,7 +342,7 @@ static ssize_t show_label(struct device *dev,
>>   {
>>   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>>   	struct platform_data *pdata = dev_get_drvdata(dev);
>> -	struct temp_data *tdata = pdata->core_data[attr->index];
>> +	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_LABEL]);
>>   
>>   	if (tdata->is_pkg_data)
>>   		return sprintf(buf, "Package id %u\n", pdata->pkg_id);
>> @@ -355,8 +355,7 @@ static ssize_t show_crit_alarm(struct device *dev,
>>   {
>>   	u32 eax, edx;
>>   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> -	struct platform_data *pdata = dev_get_drvdata(dev);
>> -	struct temp_data *tdata = pdata->core_data[attr->index];
>> +	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_CRIT_ALARM]);
>>   
>>   	mutex_lock(&tdata->update_lock);
>>   	rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx);
>> @@ -369,8 +368,7 @@ static ssize_t show_tjmax(struct device *dev,
>>   			struct device_attribute *devattr, char *buf)
>>   {
>>   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> -	struct platform_data *pdata = dev_get_drvdata(dev);
>> -	struct temp_data *tdata = pdata->core_data[attr->index];
>> +	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TJMAX]);
>>   	int tjmax;
>>   
>>   	mutex_lock(&tdata->update_lock);
>> @@ -384,8 +382,7 @@ static ssize_t show_ttarget(struct device *dev,
>>   				struct device_attribute *devattr, char *buf)
>>   {
>>   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> -	struct platform_data *pdata = dev_get_drvdata(dev);
>> -	struct temp_data *tdata = pdata->core_data[attr->index];
>> +	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TTARGET]);
>>   	int ttarget;
>>   
>>   	mutex_lock(&tdata->update_lock);
>> @@ -402,8 +399,7 @@ static ssize_t show_temp(struct device *dev,
>>   {
>>   	u32 eax, edx;
>>   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> -	struct platform_data *pdata = dev_get_drvdata(dev);
>> -	struct temp_data *tdata = pdata->core_data[attr->index];
>> +	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TEMP]);
>>   	int tjmax;
>>   
>>   	mutex_lock(&tdata->update_lock);
>> @@ -445,7 +441,6 @@ static int create_core_attrs(struct temp_data *tdata, struct device *dev,
>>   		tdata->sd_attrs[i].dev_attr.attr.name = tdata->attr_name[i];
>>   		tdata->sd_attrs[i].dev_attr.attr.mode = 0444;
>>   		tdata->sd_attrs[i].dev_attr.show = rd_ptr[i];
>> -		tdata->sd_attrs[i].index = attr_no;
> 
> I was naively thinking if we could nuke that "index". I can see that used
> in couple macros, but seems like we can lose it?
> 
> Completely untested.. and uncertain :-)
> 

If you had suggested to replace
	struct sensor_device_attribute sd_attrs[TOTAL_ATTRS];
with
	struct device_attribute sd_attrs[TOTAL_ATTRS];
what you suggested may actually be possible and make sense. However,
suggesting to dump the index parameter of SENSOR_ATTR() completely
because _this_ driver may no longer need it seems to be a little excessive.

> 
> diff --git a/include/linux/hwmon-sysfs.h b/include/linux/hwmon-sysfs.h
> index d896713359cd..4855893f9401 100644
> --- a/include/linux/hwmon-sysfs.h
> +++ b/include/linux/hwmon-sysfs.h
> @@ -12,36 +12,35 @@
>   
>   struct sensor_device_attribute{
>   	struct device_attribute dev_attr;
> -	int index;
>   };
>   #define to_sensor_dev_attr(_dev_attr) \
>   	container_of(_dev_attr, struct sensor_device_attribute, dev_attr)
>   
> -#define SENSOR_ATTR(_name, _mode, _show, _store, _index)	\
> +#define SENSOR_ATTR(_name, _mode, _show, _store)	\
>   	{ .dev_attr = __ATTR(_name, _mode, _show, _store),	\
> -	  .index = _index }
> +	  }
>   
> -#define SENSOR_ATTR_RO(_name, _func, _index)			\
> +#define SENSOR_ATTR_RO(_name, _func)			\
>   	SENSOR_ATTR(_name, 0444, _func##_show, NULL, _index)
>   
> -#define SENSOR_ATTR_RW(_name, _func, _index)			\
> -	SENSOR_ATTR(_name, 0644, _func##_show, _func##_store, _index)
> +#define SENSOR_ATTR_RW(_name, _func)			\
> +	SENSOR_ATTR(_name, 0644, _func##_show, _func##_store)
>   
> -#define SENSOR_ATTR_WO(_name, _func, _index)			\
> -	SENSOR_ATTR(_name, 0200, NULL, _func##_store, _index)
> +#define SENSOR_ATTR_WO(_name, _func)			\
> +	SENSOR_ATTR(_name, 0200, NULL, _func##_store)
>   
> -#define SENSOR_DEVICE_ATTR(_name, _mode, _show, _store, _index)	\
> +#define SENSOR_DEVICE_ATTR(_name, _mode, _show, _store)	\
>   struct sensor_device_attribute sensor_dev_attr_##_name		\
> -	= SENSOR_ATTR(_name, _mode, _show, _store, _index)
> +	= SENSOR_ATTR(_name, _mode, _show, _store)
>   
> -#define SENSOR_DEVICE_ATTR_RO(_name, _func, _index)		\
> -	SENSOR_DEVICE_ATTR(_name, 0444, _func##_show, NULL, _index)
> +#define SENSOR_DEVICE_ATTR_RO(_name, _func)		\
> +	SENSOR_DEVICE_ATTR(_name, 0444, _func##_show, NULL)
>   
>   #define SENSOR_DEVICE_ATTR_RW(_name, _func, _index)		\
> -	SENSOR_DEVICE_ATTR(_name, 0644, _func##_show, _func##_store, _index)
> +	SENSOR_DEVICE_ATTR(_name, 0644, _func##_show, _func##_store)
>   
> -#define SENSOR_DEVICE_ATTR_WO(_name, _func, _index)		\
> -	SENSOR_DEVICE_ATTR(_name, 0200, NULL, _func##_store, _index)
> +#define SENSOR_DEVICE_ATTR_WO(_name, _func)		\
> +	SENSOR_DEVICE_ATTR(_name, 0200, NULL, _func##_store)
>   
>   struct sensor_device_attribute_2 {
>   	struct device_attribute dev_attr;
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> index 975da8e7f2a9..c3bbf2f7d6eb 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -239,7 +239,7 @@ hwm_power1_max_interval_store(struct device *dev,
>   
>   static SENSOR_DEVICE_ATTR(power1_max_interval, 0664,
>   			  hwm_power1_max_interval_show,
> -			  hwm_power1_max_interval_store, 0);
> +			  hwm_power1_max_interval_store);
>   

That driver could and should have used DEVICE_ATTR() instead of
SENSOR_DEVICE_ATTR(), and there are various other drivers where
that would have made sense. Actually, it should have used
DEVICE_ATTR_RW() but that is just a detail.

However, there are more than 2,000 uses of SENSOR_DEVICE_ATTR() and derived
macros in the kernel. The large majority of those do set index to values != 0,
and the affected drivers would not be happy if that argument disappeared.

Frankly, I am not entirely sure if you were serious with your suggestion.
I tried to give a serious reply, but I am not entirely sure if I succeeded.
My apologies if some sarcasm was bleeding through.

Guenter
Ashok Raj Dec. 1, 2023, 4:34 a.m. UTC | #3 
On Thu, Nov 30, 2023 at 07:26:31PM -0800, Guenter Roeck wrote:
> On 11/30/23 17:27, Ashok Raj wrote:
> > On Mon, Nov 27, 2023 at 09:16:50PM +0800, Zhang Rui wrote:
> > > When sensor_device_attribute pointer is available, use container_of() to
> > > get the temp_data address.
> > > 
> > > This removes the unnecessary dependency of cached index in
> > > pdata->core_data[].
> > > 
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > >   drivers/hwmon/coretemp.c | 15 +++++----------
> > >   1 file changed, 5 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> > > index 6053ed3761c2..cef43fedbd58 100644
> > > --- a/drivers/hwmon/coretemp.c
> > > +++ b/drivers/hwmon/coretemp.c
> > > @@ -342,7 +342,7 @@ static ssize_t show_label(struct device *dev,
> > >   {
> > >   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > >   	struct platform_data *pdata = dev_get_drvdata(dev);
> > > -	struct temp_data *tdata = pdata->core_data[attr->index];
> > > +	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_LABEL]);
> > >   	if (tdata->is_pkg_data)
> > >   		return sprintf(buf, "Package id %u\n", pdata->pkg_id);
> > > @@ -355,8 +355,7 @@ static ssize_t show_crit_alarm(struct device *dev,
> > >   {
> > >   	u32 eax, edx;
> > >   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > > -	struct platform_data *pdata = dev_get_drvdata(dev);
> > > -	struct temp_data *tdata = pdata->core_data[attr->index];
> > > +	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_CRIT_ALARM]);
> > >   	mutex_lock(&tdata->update_lock);
> > >   	rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx);
> > > @@ -369,8 +368,7 @@ static ssize_t show_tjmax(struct device *dev,
> > >   			struct device_attribute *devattr, char *buf)
> > >   {
> > >   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > > -	struct platform_data *pdata = dev_get_drvdata(dev);
> > > -	struct temp_data *tdata = pdata->core_data[attr->index];
> > > +	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TJMAX]);
> > >   	int tjmax;
> > >   	mutex_lock(&tdata->update_lock);
> > > @@ -384,8 +382,7 @@ static ssize_t show_ttarget(struct device *dev,
> > >   				struct device_attribute *devattr, char *buf)
> > >   {
> > >   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > > -	struct platform_data *pdata = dev_get_drvdata(dev);
> > > -	struct temp_data *tdata = pdata->core_data[attr->index];
> > > +	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TTARGET]);
> > >   	int ttarget;
> > >   	mutex_lock(&tdata->update_lock);
> > > @@ -402,8 +399,7 @@ static ssize_t show_temp(struct device *dev,
> > >   {
> > >   	u32 eax, edx;
> > >   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > > -	struct platform_data *pdata = dev_get_drvdata(dev);
> > > -	struct temp_data *tdata = pdata->core_data[attr->index];
> > > +	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TEMP]);
> > >   	int tjmax;
> > >   	mutex_lock(&tdata->update_lock);
> > > @@ -445,7 +441,6 @@ static int create_core_attrs(struct temp_data *tdata, struct device *dev,
> > >   		tdata->sd_attrs[i].dev_attr.attr.name = tdata->attr_name[i];
> > >   		tdata->sd_attrs[i].dev_attr.attr.mode = 0444;
> > >   		tdata->sd_attrs[i].dev_attr.show = rd_ptr[i];
> > > -		tdata->sd_attrs[i].index = attr_no;
> > 
> > I was naively thinking if we could nuke that "index". I can see that used
> > in couple macros, but seems like we can lose it?
> > 
> > Completely untested.. and uncertain :-)
> > 
> 
> If you had suggested to replace
> 	struct sensor_device_attribute sd_attrs[TOTAL_ATTRS];
> with
> 	struct device_attribute sd_attrs[TOTAL_ATTRS];
> what you suggested may actually be possible and make sense. However,
> suggesting to dump the index parameter of SENSOR_ATTR() completely
> because _this_ driver may no longer need it seems to be a little excessive.

I should have highlighted the uncertain :-).. Said naively thinking to add
color that I'm calling it blind. But what you suggest might make more
sense.

I was just suggesting if there is more cleanup that could be accomplished along
with this might be a good thing.

I tried a quick and dirty cleanup.. apparently it was more dirty I guess
:-)

> 
> > 
> > diff --git a/include/linux/hwmon-sysfs.h b/include/linux/hwmon-sysfs.h
> > index d896713359cd..4855893f9401 100644
> > --- a/include/linux/hwmon-sysfs.h
> > +++ b/include/linux/hwmon-sysfs.h
> > @@ -12,36 +12,35 @@
> >   struct sensor_device_attribute{
> >   	struct device_attribute dev_attr;
> > -	int index;
> >   };
> >   #define to_sensor_dev_attr(_dev_attr) \
> >   	container_of(_dev_attr, struct sensor_device_attribute, dev_attr)
> > -#define SENSOR_ATTR(_name, _mode, _show, _store, _index)	\
> > +#define SENSOR_ATTR(_name, _mode, _show, _store)	\
> >   	{ .dev_attr = __ATTR(_name, _mode, _show, _store),	\
> > -	  .index = _index }
> > +	  }
> > -#define SENSOR_ATTR_RO(_name, _func, _index)			\
> > +#define SENSOR_ATTR_RO(_name, _func)			\
> >   	SENSOR_ATTR(_name, 0444, _func##_show, NULL, _index)
> > -#define SENSOR_ATTR_RW(_name, _func, _index)			\
> > -	SENSOR_ATTR(_name, 0644, _func##_show, _func##_store, _index)
> > +#define SENSOR_ATTR_RW(_name, _func)			\
> > +	SENSOR_ATTR(_name, 0644, _func##_show, _func##_store)
> > -#define SENSOR_ATTR_WO(_name, _func, _index)			\
> > -	SENSOR_ATTR(_name, 0200, NULL, _func##_store, _index)
> > +#define SENSOR_ATTR_WO(_name, _func)			\
> > +	SENSOR_ATTR(_name, 0200, NULL, _func##_store)
> > -#define SENSOR_DEVICE_ATTR(_name, _mode, _show, _store, _index)	\
> > +#define SENSOR_DEVICE_ATTR(_name, _mode, _show, _store)	\
> >   struct sensor_device_attribute sensor_dev_attr_##_name		\
> > -	= SENSOR_ATTR(_name, _mode, _show, _store, _index)
> > +	= SENSOR_ATTR(_name, _mode, _show, _store)
> > -#define SENSOR_DEVICE_ATTR_RO(_name, _func, _index)		\
> > -	SENSOR_DEVICE_ATTR(_name, 0444, _func##_show, NULL, _index)
> > +#define SENSOR_DEVICE_ATTR_RO(_name, _func)		\
> > +	SENSOR_DEVICE_ATTR(_name, 0444, _func##_show, NULL)
> >   #define SENSOR_DEVICE_ATTR_RW(_name, _func, _index)		\
> > -	SENSOR_DEVICE_ATTR(_name, 0644, _func##_show, _func##_store, _index)
> > +	SENSOR_DEVICE_ATTR(_name, 0644, _func##_show, _func##_store)
> > -#define SENSOR_DEVICE_ATTR_WO(_name, _func, _index)		\
> > -	SENSOR_DEVICE_ATTR(_name, 0200, NULL, _func##_store, _index)
> > +#define SENSOR_DEVICE_ATTR_WO(_name, _func)		\
> > +	SENSOR_DEVICE_ATTR(_name, 0200, NULL, _func##_store)
> >   struct sensor_device_attribute_2 {
> >   	struct device_attribute dev_attr;
> > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> > index 975da8e7f2a9..c3bbf2f7d6eb 100644
> > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > @@ -239,7 +239,7 @@ hwm_power1_max_interval_store(struct device *dev,
> >   static SENSOR_DEVICE_ATTR(power1_max_interval, 0664,
> >   			  hwm_power1_max_interval_show,
> > -			  hwm_power1_max_interval_store, 0);
> > +			  hwm_power1_max_interval_store);
> 
> That driver could and should have used DEVICE_ATTR() instead of
> SENSOR_DEVICE_ATTR(), and there are various other drivers where
> that would have made sense. Actually, it should have used
> DEVICE_ATTR_RW() but that is just a detail.
> 
> However, there are more than 2,000 uses of SENSOR_DEVICE_ATTR() and derived
> macros in the kernel. The large majority of those do set index to values != 0,
> and the affected drivers would not be happy if that argument disappeared.
> 
> Frankly, I am not entirely sure if you were serious with your suggestion.

Certainly can't be serious.. but I was hinting at additional cleanups.. but
I picked the wrong one obviously. 

> I tried to give a serious reply, but I am not entirely sure if I succeeded.
> My apologies if some sarcasm was bleeding through.

:-)... sarcasm is OK..
Zhang, Rui Dec. 1, 2023, 5:31 p.m. UTC | #4 
> > >         mutex_lock(&tdata->update_lock);
> > > @@ -445,7 +441,6 @@ static int create_core_attrs(struct temp_data
> > > *tdata, struct device *dev,
> > >                 tdata->sd_attrs[i].dev_attr.attr.name = tdata-
> > > >attr_name[i];
> > >                 tdata->sd_attrs[i].dev_attr.attr.mode = 0444;
> > >                 tdata->sd_attrs[i].dev_attr.show = rd_ptr[i];
> > > -               tdata->sd_attrs[i].index = attr_no;
> > 
> > I was naively thinking if we could nuke that "index". I can see
> > that used
> > in couple macros, but seems like we can lose it?
> > 
> > Completely untested.. and uncertain :-)
> > 
> 
> If you had suggested to replace
>         struct sensor_device_attribute sd_attrs[TOTAL_ATTRS];
> with
>         struct device_attribute sd_attrs[TOTAL_ATTRS];
> what you suggested may actually be possible and make sense.

Too late for me today.
Let me check if I can convert it to use device_attribute instead
tomorrow.

thanks,
rui
diff mbox series

Patch

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 6053ed3761c2..cef43fedbd58 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -342,7 +342,7 @@  static ssize_t show_label(struct device *dev,
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct platform_data *pdata = dev_get_drvdata(dev);
-	struct temp_data *tdata = pdata->core_data[attr->index];
+	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_LABEL]);
 
 	if (tdata->is_pkg_data)
 		return sprintf(buf, "Package id %u\n", pdata->pkg_id);
@@ -355,8 +355,7 @@  static ssize_t show_crit_alarm(struct device *dev,
 {
 	u32 eax, edx;
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct platform_data *pdata = dev_get_drvdata(dev);
-	struct temp_data *tdata = pdata->core_data[attr->index];
+	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_CRIT_ALARM]);
 
 	mutex_lock(&tdata->update_lock);
 	rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx);
@@ -369,8 +368,7 @@  static ssize_t show_tjmax(struct device *dev,
 			struct device_attribute *devattr, char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct platform_data *pdata = dev_get_drvdata(dev);
-	struct temp_data *tdata = pdata->core_data[attr->index];
+	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TJMAX]);
 	int tjmax;
 
 	mutex_lock(&tdata->update_lock);
@@ -384,8 +382,7 @@  static ssize_t show_ttarget(struct device *dev,
 				struct device_attribute *devattr, char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct platform_data *pdata = dev_get_drvdata(dev);
-	struct temp_data *tdata = pdata->core_data[attr->index];
+	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TTARGET]);
 	int ttarget;
 
 	mutex_lock(&tdata->update_lock);
@@ -402,8 +399,7 @@  static ssize_t show_temp(struct device *dev,
 {
 	u32 eax, edx;
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct platform_data *pdata = dev_get_drvdata(dev);
-	struct temp_data *tdata = pdata->core_data[attr->index];
+	struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TEMP]);
 	int tjmax;
 
 	mutex_lock(&tdata->update_lock);
@@ -445,7 +441,6 @@  static int create_core_attrs(struct temp_data *tdata, struct device *dev,
 		tdata->sd_attrs[i].dev_attr.attr.name = tdata->attr_name[i];
 		tdata->sd_attrs[i].dev_attr.attr.mode = 0444;
 		tdata->sd_attrs[i].dev_attr.show = rd_ptr[i];
-		tdata->sd_attrs[i].index = attr_no;
 		tdata->attrs[i] = &tdata->sd_attrs[i].dev_attr.attr;
 	}
 	tdata->attr_group.attrs = tdata->attrs;