From: Ashok Raj <ashok.raj@intel.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Ashok Raj <ashok_raj@linux.intel.com>,
Zhang Rui <rui.zhang@intel.com>, <jdelvare@suse.com>,
<fenghua.yu@intel.com>, <linux-hwmon@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, Ashok Raj <ashok.raj@intel.com>
Subject: Re: [PATCH 2/3] hwmon: (coretemp) Remove unnecessary dependency of array index
Date: Thu, 30 Nov 2023 20:34:19 -0800 [thread overview]
Message-ID: <ZWliS5pSORH1Ua9J@a4bf019067fa.jf.intel.com> (raw)
In-Reply-To: <b4723259-92a1-4c9a-8f4a-52b4b61940c5@roeck-us.net>
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..
next prev parent reply other threads:[~2023-12-01 4:34 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-27 13:16 [PATCH 0/3] hwmon: (coretemp) Fix core count limitation Zhang Rui
2023-11-27 13:16 ` [PATCH 1/3] hwmon: (coretemp) Introduce enum for attr index Zhang Rui
2023-11-30 21:51 ` Ashok Raj
2023-12-01 4:14 ` Guenter Roeck
2023-12-01 4:47 ` Ashok Raj
2023-12-01 17:29 ` Zhang, Rui
2023-12-05 19:20 ` Ashok Raj
2023-11-27 13:16 ` [PATCH 2/3] hwmon: (coretemp) Remove unnecessary dependency of array index Zhang Rui
2023-12-01 1:27 ` Ashok Raj
2023-12-01 3:26 ` Guenter Roeck
2023-12-01 4:34 ` Ashok Raj [this message]
2023-12-01 17:31 ` Zhang, Rui
2023-11-27 13:16 ` [PATCH 3/3] hwmon: (coretemp) Fix core count limitation Zhang Rui
2023-12-01 2:08 ` Ashok Raj
2023-12-01 17:47 ` Zhang, Rui
2023-12-01 22:58 ` Ashok Raj
2023-12-02 7:22 ` Zhang, Rui
2023-12-01 3:06 ` Guenter Roeck
2023-12-01 17:41 ` Zhang, Rui
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZWliS5pSORH1Ua9J@a4bf019067fa.jf.intel.com \
--to=ashok.raj@intel.com \
--cc=ashok_raj@linux.intel.com \
--cc=fenghua.yu@intel.com \
--cc=jdelvare@suse.com \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=rui.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox