On Wed, Dec 20, 2017 at 01:54:41AM -0800, Joe Perches wrote:
On Wed, 2017-12-20 at 10:32 +0100, Greg Kroah-Hartman wrote:
On Wed, Dec 20, 2017 at 01:24:44AM -0800, Joe Perches wrote:
On Wed, 2017-12-20 at 10:34 +0200, Jarkko Nikula wrote:
On Tue, Dec 19, 2017 at 10:15:07AM -0800, Joe Perches wrote:
Convert DEVICE_ATTR uses to DEVICE_ATTR_RW where possible.
[]
diff --git a/sound/soc/omap/mcbsp.c b/sound/soc/omap/mcbsp.c
[]
@@ -854,7 +854,7 @@ static ssize_t dma_op_mode_store(struct device *dev, return size; }
-static DEVICE_ATTR(dma_op_mode, 0644, dma_op_mode_show, dma_op_mode_store); +static DEVICE_ATTR_RW(dma_op_mode);
While I can ack this part here if it helps generic cleanup effort I don't understart would it improve code readability in general? Mode 644 is clear and don't need any grepping but for DEVICE_ATTR_RW() I had to go through all of these files in order to see what does it mean:
Yeah, 644 is "clear", but _RW() is even more clear. Ideally I want to get rid of all of the "non-standard" users that set random modes of sysfs files, as we get it wrong too many times. Using the "defaults" is much better.
Are you suggesting that device.h (as that is where DEVICE_ATTR and the other DEVICE_ATTR_<FOO> variants are #defined) should have better comments for the _<FOO> variants?
DEVICE_ATTR_RW: include/linux/device.h __ATTR_RW: include/linux/sysfs.h S_IWUSR: include/uapi/linux/stat.h S_IRUGO: include/linux/stat.h
btw: patch 1/4 of the series does remove the uses of S_<PERMS> from these macros in sysfs.h and converts them to simple octal instead.
Why you didn't send that patch to the sysfs maintainer is a bit odd... :)
So here's an opportunity for you:
The sysfs maintainer hasn't added include/linux/sysfs.h to the list of maintained files...
DRIVER CORE, KOBJECTS, DEBUGFS AND SYSFS M: Greg Kroah-Hartman gregkh@linuxfoundation.org T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git S: Supported F: Documentation/kobject.txt F: drivers/base/ F: fs/debugfs/ F: fs/sysfs/ F: include/linux/debugfs.h F: include/linux/kobj* F: lib/kobj*
Heh, good point, but using get_maintainer.pl does put me at the top of the list that you should be cc:ing:
$ ./scripts/get_maintainer.pl --file include/linux/sysfs.h Greg Kroah-Hartman gregkh@linuxfoundation.org (commit_signer:3/3=100%,authored:2/3=67%,added_lines:7/8=88%) Kate Stewart kstewart@linuxfoundation.org (commit_signer:1/3=33%) Thomas Gleixner tglx@linutronix.de (commit_signer:1/3=33%) Philippe Ombredanne pombredanne@nexb.com (commit_signer:1/3=33%) Nick Desaulniers nick.desaulniers@gmail.com (commit_signer:1/3=33%,authored:1/3=33%,added_lines:1/8=12%,removed_lines:1/1=100%) linux-kernel@vger.kernel.org (open list)
Anyway, I'll go add sysfs.h to the list, thanks for pointing it out.
I should be taking this whole series through my tree. Joe, thanks for doing this in an automated way, I've been wanting to see this done for a long time.
btw: there are many uses of a reversed declaration style of DEVICE_ATTR
Here's another thing that could be done for more DEVICE_ATTR_<FOO> uses.
===
Some DEVICE_ATTR definitions use a reversed static function form from the typical. Convert them to use the more common macro form so it is easier to grep for the style.
i.e.: static ssize_t show_<foo>(...) and static ssize_t store_<foo>(...)
where the static function names in the DEVICE_ATTR_RW macro are reversed
static ssize_t <foo>_show(...)and static ssize_t <foo>_store(...)
Done with perl script
$ cat dev_attr_rw_backwards.perl local $/; while (<>) { my $file = $_; while ($file =~ m/\bDEVICE_ATTR\s*(\s*(\w+)\s*,/g) { my $var = $1; if ($file =~ s/\bDEVICE_ATTR\s*(\s*${var}\s*,\s*(?(\s*S_IRUGO\s*|\s*S $file =~ s/\bshow_${var}\b/${var}_show/g; $file =~ s/\bstore_${var}\b/${var}_store/g; } } print $file; }
$ git grep --name-only -w DEVICE_ATTR | \ xargs perl -i dev_attr_rw_backwards.perl
Ah, nice, I love perl :)
greg k-h