| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b4723259-92a1-4c9a-8f4a-52b4b61940c5@roeck-us.net>
Date: Thu, 30 Nov 2023 19:26:31 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Ashok Raj <ashok_raj@...ux.intel.com>,
Zhang Rui <rui.zhang@...el.com>
Cc: jdelvare@...e.com, fenghua.yu@...el.com,
linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
Ashok Raj <ashok.raj@...el.com>
Subject: Re: [PATCH 2/3] hwmon: (coretemp) Remove unnecessary dependency of
array index
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@...el.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
Powered by blists - more mailing lists