Re: [Intel-gfx] [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
On 2017-12-20 12:54, Jarkko Nikula wrote: > On Wed, Dec 20, 2017 at 10:32:11AM +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. >> > Fair enough. For the sound/soc/omap/ (Acked-by was missing from my > previous reply): > > Acked-by: Jarkko NikulaAnd from me to the same file (sound/soc/omap/mcbsp.c): Acked-by: Peter Ujfalusi - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
On Tuesday, December 19, 2017 10:15:07 AM Joe Perches wrote: > Convert DEVICE_ATTR uses to DEVICE_ATTR_RW where possible. > > Done with perl script: > > $ git grep -w --name-only DEVICE_ATTR | \ > xargs perl -i -e 'local $/; while (<>) { > s/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,\s*\(?(\s*S_IRUGO\s*\|\s*S_IWUSR|\s*S_IWUSR\s*\|\s*S_IRUGO\s*|\s*0644\s*)\)?\s*,\s*\1_show\s*,\s*\1_store\s*\)/DEVICE_ATTR_RW(\1)/g; > print;}' > > Signed-off-by: Joe Perches> --- > arch/s390/kernel/topology.c | 3 +-- > arch/tile/kernel/sysfs.c | 2 +- > drivers/gpu/drm/i915/i915_sysfs.c| 6 ++--- > drivers/platform/x86/compal-laptop.c | 18 +-- > drivers/s390/cio/device.c| 2 +- > drivers/scsi/lpfc/lpfc_attr.c| 43 > > drivers/thermal/thermal_sysfs.c | 9 > drivers/tty/serial/sh-sci.c | 2 +- > drivers/usb/host/xhci-dbgcap.c | 2 +- > drivers/usb/phy/phy-tahvo.c | 2 +- > drivers/video/fbdev/auo_k190x.c | 4 ++-- > drivers/video/fbdev/w100fb.c | 4 ++-- > lib/test_firmware.c | 14 +--- > lib/test_kmod.c | 14 +--- > sound/soc/omap/mcbsp.c | 4 ++-- > 15 files changed, 49 insertions(+), 80 deletions(-) For fbdev changes: Acked-by: Bartlomiej Zolnierkiewicz Best regards, -- Bartlomiej Zolnierkiewicz Samsung R Institute Poland Samsung Electronics ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
On Tue, 2017-12-19 at 10:15 -0800, Joe Perches wrote: > Convert DEVICE_ATTR uses to DEVICE_ATTR_RW where possible. > > Done with perl script: > > $ git grep -w --name-only DEVICE_ATTR | \ > xargs perl -i -e 'local $/; while (<>) { > s/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,\s*\(?(\s*S_IRUGO\s*\|\s*S_IWUSR|\s*S > _IWUSR\s*\|\s*S_IRUGO\s*|\s*0644\s*)\)?\s*,\s*\1_show\s*,\s*\1_store\ > s*\)/DEVICE_ATTR_RW(\1)/g; print;}' > > Signed-off-by: Joe Perches> --- > arch/s390/kernel/topology.c | 3 +-- > arch/tile/kernel/sysfs.c | 2 +- > drivers/gpu/drm/i915/i915_sysfs.c| 6 ++--- > drivers/platform/x86/compal-laptop.c | 18 +-- > drivers/s390/cio/device.c| 2 +- > drivers/scsi/lpfc/lpfc_attr.c| 43 > > drivers/thermal/thermal_sysfs.c | 9 For the thermal part, ACK-by: Zhang Rui thanks, rui > drivers/tty/serial/sh-sci.c | 2 +- > drivers/usb/host/xhci-dbgcap.c | 2 +- > drivers/usb/phy/phy-tahvo.c | 2 +- > drivers/video/fbdev/auo_k190x.c | 4 ++-- > drivers/video/fbdev/w100fb.c | 4 ++-- > lib/test_firmware.c | 14 +--- > lib/test_kmod.c | 14 +--- > sound/soc/omap/mcbsp.c | 4 ++-- > 15 files changed, 49 insertions(+), 80 deletions(-) > > diff --git a/arch/s390/kernel/topology.c > b/arch/s390/kernel/topology.c > index 4d5b65e527b5..4b6e0397f66d 100644 > --- a/arch/s390/kernel/topology.c > +++ b/arch/s390/kernel/topology.c > @@ -404,8 +404,7 @@ static ssize_t dispatching_store(struct device > *dev, > put_online_cpus(); > return rc ? rc : count; > } > -static DEVICE_ATTR(dispatching, 0644, dispatching_show, > - dispatching_store); > +static DEVICE_ATTR_RW(dispatching); > > static ssize_t cpu_polarization_show(struct device *dev, > struct device_attribute *attr, > char *buf) > diff --git a/arch/tile/kernel/sysfs.c b/arch/tile/kernel/sysfs.c > index 825867c53853..af5024f0fb5a 100644 > --- a/arch/tile/kernel/sysfs.c > +++ b/arch/tile/kernel/sysfs.c > @@ -184,7 +184,7 @@ static ssize_t hv_stats_store(struct device *dev, > return n < 0 ? n : count; > } > > -static DEVICE_ATTR(hv_stats, 0644, hv_stats_show, hv_stats_store); > +static DEVICE_ATTR_RW(hv_stats); > > static int hv_stats_device_add(struct device *dev, struct > subsys_interface *sif) > { > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c > b/drivers/gpu/drm/i915/i915_sysfs.c > index c74a20b80182..1d0ab8ff5915 100644 > --- a/drivers/gpu/drm/i915/i915_sysfs.c > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > @@ -447,9 +447,9 @@ static ssize_t gt_min_freq_mhz_store(struct > device *kdev, > > static DEVICE_ATTR(gt_act_freq_mhz, S_IRUGO, gt_act_freq_mhz_show, > NULL); > static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show, > NULL); > -static DEVICE_ATTR(gt_boost_freq_mhz, S_IRUGO | S_IWUSR, > gt_boost_freq_mhz_show, gt_boost_freq_mhz_store); > -static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, > gt_max_freq_mhz_show, gt_max_freq_mhz_store); > -static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, > gt_min_freq_mhz_show, gt_min_freq_mhz_store); > +static DEVICE_ATTR_RW(gt_boost_freq_mhz); > +static DEVICE_ATTR_RW(gt_max_freq_mhz); > +static DEVICE_ATTR_RW(gt_min_freq_mhz); > > static DEVICE_ATTR(vlv_rpe_freq_mhz, S_IRUGO, vlv_rpe_freq_mhz_show, > NULL); > > diff --git a/drivers/platform/x86/compal-laptop.c > b/drivers/platform/x86/compal-laptop.c > index 6bcb750e1865..4f9bc72f0584 100644 > --- a/drivers/platform/x86/compal-laptop.c > +++ b/drivers/platform/x86/compal-laptop.c > @@ -679,18 +679,12 @@ static int bat_writeable_property(struct > power_supply *psy, > /* == */ > /* Driver Globals */ > /* == */ > -static DEVICE_ATTR(wake_up_pme, > - 0644, wake_up_pme_show, wake_up_pme_s > tore); > -static DEVICE_ATTR(wake_up_modem, > - 0644, wake_up_modem_show, wake_up_modem_store > ); > -static DEVICE_ATTR(wake_up_lan, > - 0644, wake_up_lan_show, wake_up_lan_store); > -static DEVICE_ATTR(wake_up_wlan, > - 0644, wake_up_wlan_show,wake_up_wlan_store); > -static DEVICE_ATTR(wake_up_key, > - 0644, wake_up_key_show, wake_up_key_store); > -static DEVICE_ATTR(wake_up_mouse, > - 0644, wake_up_mouse_show, wake_up_mouse_store > ); > +static DEVICE_ATTR_RW(wake_up_pme); > +static DEVICE_ATTR_RW(wake_up_modem); > +static DEVICE_ATTR_RW(wake_up_lan); > +static DEVICE_ATTR_RW(wake_up_wlan); > +static DEVICE_ATTR_RW(wake_up_key); > +static DEVICE_ATTR_RW(wake_up_mouse); > > static DEVICE_ATTR(fan1_input, S_IRUGO, fan_show, NULL); > static DEVICE_ATTR(temp1_input, S_IRUGO, temp_cpu, NULL); > diff --git a/drivers/s390/cio/device.c b/drivers/s390/cio/device.c > index
Re: [Intel-gfx] [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
On Wed, Dec 20, 2017 at 10:32:11AM +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. > Fair enough. For the sound/soc/omap/ (Acked-by was missing from my previous reply): Acked-by: Jarkko Nikula___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
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 > index 7a54e3083203..79d4dc785e5c 100644 > --- 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: 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 Jarkko Nikula___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
On Wed, 20 Dec 2017, Joe Perches wrote: > On Wed, 2017-12-20 at 10:59 +0100, Greg Kroah-Hartman wrote: > > > > 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> > > 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 > > (commit_signer:3/3=100%,authored:2/3=67%,added_lines:7/8=88%) > > Kate Stewart (commit_signer:1/3=33%) > > Thomas Gleixner (commit_signer:1/3=33%) > > Philippe Ombredanne (commit_signer:1/3=33%) > > Nick Desaulniers > > (commit_signer:1/3=33%,authored:1/3=33%,added_lines:1/8=12%,removed_lines:1/1=100%) > > linux-ker...@vger.kernel.org (open list) > > The script I use to send patches adds --nogit --nogit-fallback > to copy only listed maintainers because people that send cleanup > patches don't generally like to get random patches. > > > > 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_ 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. > [] > > > $ git grep --name-only -w DEVICE_ATTR | \ > > > xargs perl -i dev_attr_rw_backwards.perl > > Ah, nice, I love perl : > > That was a bad copy/paste of the script. > > The actual script for RW is: > > $ 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_IWUSR|\s*S_IWUSR\s*\|\s*S_IRUGO\s*|\s*0644\s*)\)?\s*,\s*show_${var}\s*,\s*store_${var}\s*\)/DEVICE_ATTR_RW(${var})/g) > { > $file =~ s/\bshow_${var}\b/${var}_show/g; > $file =~ s/\bstore_${var}\b/${var}_store/g; > } > } > print $file; > } > > There are 3 different perl scripts for rw, ro, and wo. > and these scripts, because of function renaming and > possible reuse of the original function names by other > string concatenated macros, create some bad conversions > so they need some manual cleanups too. > > Perhaps coccinelle could do a better job of it, but > likely string concatenation macro uses are going to > be hard to deal with in any case. I made a rule for this at one point, but there are cases where the functions have the wrong names, and then these functions may be used elsewhere. julia ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
On Wed, 2017-12-20 at 10:59 +0100, Greg Kroah-Hartman wrote: > > > 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> > 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 > (commit_signer:3/3=100%,authored:2/3=67%,added_lines:7/8=88%) > Kate Stewart (commit_signer:1/3=33%) > Thomas Gleixner (commit_signer:1/3=33%) > Philippe Ombredanne (commit_signer:1/3=33%) > Nick Desaulniers > (commit_signer:1/3=33%,authored:1/3=33%,added_lines:1/8=12%,removed_lines:1/1=100%) > linux-ker...@vger.kernel.org (open list) The script I use to send patches adds --nogit --nogit-fallback to copy only listed maintainers because people that send cleanup patches don't generally like to get random patches. > > 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_ 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. [] > > $ git grep --name-only -w DEVICE_ATTR | \ > > xargs perl -i dev_attr_rw_backwards.perl > Ah, nice, I love perl : That was a bad copy/paste of the script. The actual script for RW is: $ 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_IWUSR|\s*S_IWUSR\s*\|\s*S_IRUGO\s*|\s*0644\s*)\)?\s*,\s*show_${var}\s*,\s*store_${var}\s*\)/DEVICE_ATTR_RW(${var})/g) { $file =~ s/\bshow_${var}\b/${var}_show/g; $file =~ s/\bstore_${var}\b/${var}_store/g; } } print $file; } There are 3 different perl scripts for rw, ro, and wo. and these scripts, because of function renaming and possible reuse of the original function names by other string concatenated macros, create some bad conversions so they need some manual cleanups too. Perhaps coccinelle could do a better job of it, but likely string concatenation macro uses are going to be hard to deal with in any case. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
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_ variants > > > are #defined) should have better comments for the > > > _ 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_ 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> 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 (commit_signer:3/3=100%,authored:2/3=67%,added_lines:7/8=88%) Kate Stewart (commit_signer:1/3=33%) Thomas Gleixner (commit_signer:1/3=33%) Philippe Ombredanne (commit_signer:1/3=33%) Nick Desaulniers (commit_signer:1/3=33%,authored:1/3=33%,added_lines:1/8=12%,removed_lines:1/1=100%) linux-ker...@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_ 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_(...) > and > static ssize_t store_(...) > > where the static function names in the DEVICE_ATTR_RW macro are reversed > > static ssize_t _show(...) > and > static ssize_t _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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
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_ variants > > are #defined) should have better comments for the > > _ 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_ 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-HartmanT: 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* > 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_ 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_(...) and static ssize_t store_(...) where the static function names in the DEVICE_ATTR_RW macro are reversed static ssize_t _show(...) and static ssize_t _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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
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_ variants > are #defined) should have better comments for the > _ 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_ 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... :) 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. thanks, greg k-h ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
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: Are you suggesting that device.h (as that is where DEVICE_ATTR and the other DEVICE_ATTR_ variants are #defined) should have better comments for the _ 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_ from these macros in sysfs.h and converts them to simple octal instead. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
On Tue, Dec 19, 2017 at 8:15 PM, Joe Percheswrote: > Convert DEVICE_ATTR uses to DEVICE_ATTR_RW where possible. > > Done with perl script: > > $ git grep -w --name-only DEVICE_ATTR | \ > xargs perl -i -e 'local $/; while (<>) { > s/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,\s*\(?(\s*S_IRUGO\s*\|\s*S_IWUSR|\s*S_IWUSR\s*\|\s*S_IRUGO\s*|\s*0644\s*)\)?\s*,\s*\1_show\s*,\s*\1_store\s*\)/DEVICE_ATTR_RW(\1)/g; > print;}' > drivers/platform/x86/compal-laptop.c | 18 +-- > --- a/drivers/platform/x86/compal-laptop.c > +++ b/drivers/platform/x86/compal-laptop.c > @@ -679,18 +679,12 @@ static int bat_writeable_property(struct power_supply > *psy, > /* == */ > /* Driver Globals */ > /* == */ > -static DEVICE_ATTR(wake_up_pme, > - 0644, wake_up_pme_show, wake_up_pme_store); > -static DEVICE_ATTR(wake_up_modem, > - 0644, wake_up_modem_show, wake_up_modem_store); > -static DEVICE_ATTR(wake_up_lan, > - 0644, wake_up_lan_show, wake_up_lan_store); > -static DEVICE_ATTR(wake_up_wlan, > - 0644, wake_up_wlan_show,wake_up_wlan_store); > -static DEVICE_ATTR(wake_up_key, > - 0644, wake_up_key_show, wake_up_key_store); > -static DEVICE_ATTR(wake_up_mouse, > - 0644, wake_up_mouse_show, wake_up_mouse_store); > +static DEVICE_ATTR_RW(wake_up_pme); > +static DEVICE_ATTR_RW(wake_up_modem); > +static DEVICE_ATTR_RW(wake_up_lan); > +static DEVICE_ATTR_RW(wake_up_wlan); > +static DEVICE_ATTR_RW(wake_up_key); > +static DEVICE_ATTR_RW(wake_up_mouse); Acked-by: Andy Shevchenko for PDx86 bits. Have to say that while it doesn't change the attributes here, it might require still to be revisited by security people, if they wish. -- With Best Regards, Andy Shevchenko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
Convert DEVICE_ATTR uses to DEVICE_ATTR_RW where possible. Done with perl script: $ git grep -w --name-only DEVICE_ATTR | \ xargs perl -i -e 'local $/; while (<>) { s/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,\s*\(?(\s*S_IRUGO\s*\|\s*S_IWUSR|\s*S_IWUSR\s*\|\s*S_IRUGO\s*|\s*0644\s*)\)?\s*,\s*\1_show\s*,\s*\1_store\s*\)/DEVICE_ATTR_RW(\1)/g; print;}' Signed-off-by: Joe Perches--- arch/s390/kernel/topology.c | 3 +-- arch/tile/kernel/sysfs.c | 2 +- drivers/gpu/drm/i915/i915_sysfs.c| 6 ++--- drivers/platform/x86/compal-laptop.c | 18 +-- drivers/s390/cio/device.c| 2 +- drivers/scsi/lpfc/lpfc_attr.c| 43 drivers/thermal/thermal_sysfs.c | 9 drivers/tty/serial/sh-sci.c | 2 +- drivers/usb/host/xhci-dbgcap.c | 2 +- drivers/usb/phy/phy-tahvo.c | 2 +- drivers/video/fbdev/auo_k190x.c | 4 ++-- drivers/video/fbdev/w100fb.c | 4 ++-- lib/test_firmware.c | 14 +--- lib/test_kmod.c | 14 +--- sound/soc/omap/mcbsp.c | 4 ++-- 15 files changed, 49 insertions(+), 80 deletions(-) diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c index 4d5b65e527b5..4b6e0397f66d 100644 --- a/arch/s390/kernel/topology.c +++ b/arch/s390/kernel/topology.c @@ -404,8 +404,7 @@ static ssize_t dispatching_store(struct device *dev, put_online_cpus(); return rc ? rc : count; } -static DEVICE_ATTR(dispatching, 0644, dispatching_show, -dispatching_store); +static DEVICE_ATTR_RW(dispatching); static ssize_t cpu_polarization_show(struct device *dev, struct device_attribute *attr, char *buf) diff --git a/arch/tile/kernel/sysfs.c b/arch/tile/kernel/sysfs.c index 825867c53853..af5024f0fb5a 100644 --- a/arch/tile/kernel/sysfs.c +++ b/arch/tile/kernel/sysfs.c @@ -184,7 +184,7 @@ static ssize_t hv_stats_store(struct device *dev, return n < 0 ? n : count; } -static DEVICE_ATTR(hv_stats, 0644, hv_stats_show, hv_stats_store); +static DEVICE_ATTR_RW(hv_stats); static int hv_stats_device_add(struct device *dev, struct subsys_interface *sif) { diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index c74a20b80182..1d0ab8ff5915 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -447,9 +447,9 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev, static DEVICE_ATTR(gt_act_freq_mhz, S_IRUGO, gt_act_freq_mhz_show, NULL); static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show, NULL); -static DEVICE_ATTR(gt_boost_freq_mhz, S_IRUGO | S_IWUSR, gt_boost_freq_mhz_show, gt_boost_freq_mhz_store); -static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, gt_max_freq_mhz_store); -static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, gt_min_freq_mhz_store); +static DEVICE_ATTR_RW(gt_boost_freq_mhz); +static DEVICE_ATTR_RW(gt_max_freq_mhz); +static DEVICE_ATTR_RW(gt_min_freq_mhz); static DEVICE_ATTR(vlv_rpe_freq_mhz, S_IRUGO, vlv_rpe_freq_mhz_show, NULL); diff --git a/drivers/platform/x86/compal-laptop.c b/drivers/platform/x86/compal-laptop.c index 6bcb750e1865..4f9bc72f0584 100644 --- a/drivers/platform/x86/compal-laptop.c +++ b/drivers/platform/x86/compal-laptop.c @@ -679,18 +679,12 @@ static int bat_writeable_property(struct power_supply *psy, /* == */ /* Driver Globals */ /* == */ -static DEVICE_ATTR(wake_up_pme, - 0644, wake_up_pme_show, wake_up_pme_store); -static DEVICE_ATTR(wake_up_modem, - 0644, wake_up_modem_show, wake_up_modem_store); -static DEVICE_ATTR(wake_up_lan, - 0644, wake_up_lan_show, wake_up_lan_store); -static DEVICE_ATTR(wake_up_wlan, - 0644, wake_up_wlan_show,wake_up_wlan_store); -static DEVICE_ATTR(wake_up_key, - 0644, wake_up_key_show, wake_up_key_store); -static DEVICE_ATTR(wake_up_mouse, - 0644, wake_up_mouse_show, wake_up_mouse_store); +static DEVICE_ATTR_RW(wake_up_pme); +static DEVICE_ATTR_RW(wake_up_modem); +static DEVICE_ATTR_RW(wake_up_lan); +static DEVICE_ATTR_RW(wake_up_wlan); +static DEVICE_ATTR_RW(wake_up_key); +static DEVICE_ATTR_RW(wake_up_mouse); static DEVICE_ATTR(fan1_input, S_IRUGO, fan_show, NULL); static DEVICE_ATTR(temp1_input, S_IRUGO, temp_cpu, NULL); diff --git a/drivers/s390/cio/device.c b/drivers/s390/cio/device.c index 75a245f38e2e..6eefb67b31f3 100644 --- a/drivers/s390/cio/device.c +++ b/drivers/s390/cio/device.c @@ -600,7 +600,7 @@ static ssize_t vpm_show(struct device *dev, struct device_attribute *attr, static DEVICE_ATTR(devtype, 0444, devtype_show, NULL); static DEVICE_ATTR(cutype, 0444, cutype_show, NULL); static DEVICE_ATTR(modalias, 0444, modalias_show,