From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1764051AbcLVPpw (ORCPT ); Thu, 22 Dec 2016 10:45:52 -0500 Received: from mail3-relais-sop.national.inria.fr ([192.134.164.104]:46318 "EHLO mail3-relais-sop.national.inria.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752854AbcLVPpu (ORCPT ); Thu, 22 Dec 2016 10:45:50 -0500 X-IronPort-AV: E=Sophos;i="5.33,389,1477954800"; d="scan'208";a="205541904" Date: Thu, 22 Dec 2016 16:45:45 +0100 (CET) From: Julia Lawall X-X-Sender: jll@hadrien To: Guenter Roeck cc: cocci@systeme.lip6.fr, Greg KH , linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time In-Reply-To: <83039af2-7b45-8b1f-d842-85a70b71ba11@roeck-us.net> Message-ID: References: <0bcce41c-88ce-09a0-2ddd-5ba2e406d482@roeck-us.net> <20161221162926.GB6188@roeck-us.net> <20161221214425.GA2315@roeck-us.net> <83039af2-7b45-8b1f-d842-85a70b71ba11@roeck-us.net> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 22 Dec 2016, Guenter Roeck wrote: > On 12/22/2016 04:29 AM, Julia Lawall wrote: > > > > > > On Wed, 21 Dec 2016, Guenter Roeck wrote: > > > > > Hi Julia, > > > > > > On Wed, Dec 21, 2016 at 08:39:38PM +0100, Julia Lawall wrote: > > > > > > > > > > > > On Wed, 21 Dec 2016, Guenter Roeck wrote: > > > > > > > > > Hi Julia, > > > > > > > > > > On Wed, Dec 21, 2016 at 03:05:37PM +0100, Julia Lawall wrote: > > > > > > A solution is below: the semantic patch, an explanation of the > > > > > > semantic > > > > > > patch, and the results. I have only tried to compile the results > > > > > > (make > > > > > > drivers/hwmon/). Two affected files were not considered for > > > > > > compilation: > > > > > > > > > > > > drivers/hwmon/vexpress-hwmon.o > > > > > > drivers/hwmon/jz4740-hwmon.o > > > > > > I compile tested those two patches. If possible please drop > > > vexpress-hwmon.c > > > from the patch series; the changes in that file don't add any value. > > > > > > I compile tested all files, and reviewed the patch. It all looks good. > > > Please submit the series. > > > > > > Again, thanks a lot for your help! > > > > I have sent the patches. I adjusted the semantic patch so that the > > indentation of function parameters/arguments would only change if the > > length of the function name changes. > > > > Do you think this could be of more general interest in the Linux kernel? > > Since the semantic patch works pretty well, I could add it to the > > scripts/coccinelle directory? Previously, however, I got some negative > > feedback about this change, because people felt that the new names hid the > > actual behavior, so I didn't pursue it. > > > > I do think it would add a lot of value, if for nothing else as an excellent > example > of what can be done with coccinelle. > > I actually liked the name changes. I think it is a good idea if the function > name > reflects the sysfs attribute it serves (isn't that exactly what it does, ie > its > behavior ?). But, as you have experienced, some people inadvertently did not > like > it. Given that, I am not sure if it is worth adding it to the kernel source > tree. > Maybe you could submit it as RFC so it is at least on record. > > Anyway, for SENSOR_DEVICE_ATTR(), I'll have to be a bit more flexible since > the function _will_ be reused. I'll need something like > SENSOR_DEVICE_ATTR_{RO,RW,WO}(attr, func, param) Chosen at random, static SENSOR_DEVICE_ATTR_2(sf2_point4_fan1, S_IRUGO | S_IWUSR, show_sf2_point, store_sf2_point, 4, 1); should become static SENSOR_DEVICE_ATTR_2_RW(sf2_point4_fan1, sf2_point, 4, 1); ? And the functions should be renamed with show and store at the end? > Maybe Greg would be open to something like > DEVICE_ATTR_FUNC_{RO,RW,WO}(attr,func) > to accommodate the "I want my own function name" crowd ? That would also solve > the case where the function is reused for multiple attributes. Actually, it was the DEVICE_ATTR_{RO,RW,WO} that wasn't liked. It doesn't show the exact permission numbers. The fact that not all DEVICE_ATTR uses can be changed due to function reuse is awkward, though. Greg, do you have any thoughts about that? Currently, there are around 1100 calls to DEVICE_ATTR_{RO,RW,WO}. julia