Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Thu, 30 Nov 2023 20:34:19 -0800
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
Message-ID: <ZWliS5pSORH1Ua9J@a4bf019067fa.jf.intel.com>
References: <20231127131651.476795-1-rui.zhang@intel.com>
 <20231127131651.476795-3-rui.zhang@intel.com>
 <ZWk2ZAxuyOFDCTmv@araj-dh-work.jf.intel.com>
 <b4723259-92a1-4c9a-8f4a-52b4b61940c5@roeck-us.net>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <b4723259-92a1-4c9a-8f4a-52b4b61940c5@roeck-us.net>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?1Ipyj215orJqCdObHEHJuJTvPBULfIQUTFWlG9HVmHaoaJ3dL4IT1sY5vL+0?=
 =?us-ascii?Q?1yay5kavz42eMRX45O5dtbmGubxrX4+WI0j4XjgpxYd/KL13cQ+3/oOMfWjJ?=
 =?us-ascii?Q?wA6/QI8LvlT3SEJ4QG8ghvgl+lgVs0ww9HcNEFtW1u7KWraX0KH8+ByU4YDs?=
 =?us-ascii?Q?stbp7Xewey6X8fcNAz3dUrgzIZ+IsSoK0/3h7mh0XhS1b4oQr5PEAV7PirB2?=
 =?us-ascii?Q?WjtDh6sPVq8w06yI0GPOvR8gn+pSNdnFHI+n2EhsVPto/rHnDo7PRAlxKy2t?=
 =?us-ascii?Q?Ecs2+X0LouuU+VHYOorBI6N0o7y10gG7o5OhH3RzBtFn1OAOcrSTw3cnN7dY?=
 =?us-ascii?Q?bvTKVPT8uST0iMdNnqqSjxwZfJGxCA6/EDbFUdaAAfciFM/amh0hCaD57twm?=
 =?us-ascii?Q?ClT5XXspQVY1BCQl5OJjjVn7ycwgnxw/l4mbaj2Hv+/XuNaTEfIlqnEG/vm4?=
 =?us-ascii?Q?Igi4ifOHrFn7IjweAZU+meVuXIJhEPa8Ajh/k17h/D0VIpButiS+8tH7aqX5?=
 =?us-ascii?Q?F34rVnrGsZR/hM9+3ZU6L4bL0aOCy7T6JsPenWt6vvOKZKfz/UO8k9pFmYsa?=
 =?us-ascii?Q?F51RL8GXuZdUJTwub9HWLwigOCLSrjwGQzb/t6JDrUN9aCzc3Gv0ZgSe25rK?=
 =?us-ascii?Q?1F6FRlp7sHbkvxxWY/8U6F6Fh7l5rJiJ5qJ1SQ+UQDS+YoX8QUj7zTlNfCCK?=
 =?us-ascii?Q?4ZbFTyICpWIb7nwdSexkSqVTB7FaZuU7/uJY+d5YpTuat+BaESijcw412+Px?=
 =?us-ascii?Q?JoKh8+3t7AXexOR4+lfx5Sj2YHyT6EkmDBiHFIwTAg06HZdVD+D+3fA0Te4p?=
 =?us-ascii?Q?12M73/CNskuNXCNnYmKU5me91HCbCJiabUGe/3hJVo4v9G7YtsevQL1QdoGj?=
 =?us-ascii?Q?Te8/WOMCxir59vOox1zXUDhJ6rcJ8KZ1hNOQPgGd22P1BA/Y0K6gp2o+X5HV?=
 =?us-ascii?Q?Bcko0ZYCB/DUQQi3sdfz4YnrzDTjrB+wh/IUSV0rMTzAS88f+JdaQuBXyGyP?=
 =?us-ascii?Q?yfMYjjkEb68zMpIhsAZmzc4uwFa/VB8rsv0jzxaqdtQbKiQn89/T/8umGIUE?=
 =?us-ascii?Q?ijQkqr6gqsc4oxPSw1fouE5a6VL1he9RV8YdX36TAJGlYYuMw4oGt3E85i2W?=
 =?us-ascii?Q?VjN4MLXLDDIRGwi7W9TercWMSdWa4R/7UldCNshmM34b0ohTSrV8iQqdd37v?=
 =?us-ascii?Q?0FZzADQJY2lxG+bhlkMpUZhZ0hWMX/wmIAcGSfQwD/GUh8hhIxHITPqp/LA2?=
 =?us-ascii?Q?Qvi68jxJqvl6MAuUBld9swW97zxnlNcj2WuTshdFCYf6LwCfQGux6+QcpQOO?=
 =?us-ascii?Q?lyp0RObFmeI6uTXnaAA1I6YHYrA493dLPNkMHKiQRarZx4uOi53AFxqxCdnL?=
 =?us-ascii?Q?DrjiV/kTudO7wmHen5mZFxqSHtIwnhQDncUFkxSV/69ESPh7C3MtVe3R8F/Q?=
 =?us-ascii?Q?+l7wCSrVxBFoe2xxu8Xngeox8Jqt1gK7/R6aEqxW/wEvVZUx7BGEUfgxycct?=
 =?us-ascii?Q?mV0F5+MMKYtzcNwhIE5aaA0FwZpCV9qVl6FS5M4xfrkf9GjPtXK82uoJOAQ6?=
 =?us-ascii?Q?PcvBKLlxrdo4HlMlnNEy6gtnV1+atmXULD7kpYygYoZkLrEkS794Wuo5e67r?=
 =?us-ascii?Q?Hw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c83d2b57-d99a-46de-9b01-08dbf226cc09
X-MS-Exchange-CrossTenant-AuthSource: SJ1PR11MB6201.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Dec 2023 04:34:25.0101
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: fw9aeDqjIdJ4xxSYYXuzukHiDHEHftR+Qyzc4mW2PTxEcpT0DfPav7gSy8S5qgQ1kvWA0uTpV1Vn25b9w1PT5g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7382
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Thu, 30 Nov 2023 20:34:45 -0800 (PST)

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