* [PATCH] staging: gasket: replace symbolic permissions @ 2020-06-01 0:52 Rodolfo C. Villordo 2020-06-18 7:47 ` Greg Kroah-Hartman 0 siblings, 1 reply; 5+ messages in thread From: Rodolfo C. Villordo @ 2020-06-01 0:52 UTC (permalink / raw) To: linux-kernel Cc: rodolfovillordo, Rob Springer, Todd Poynor, Ben Chan, Richard Yeh, Greg Kroah-Hartman, devel WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'. + .attr = __ATTR(_name, S_IRUGO, _show_function, NULL), \ warning detected by checkpatch.pl Signed-off-by: Rodolfo C. Villordo <rodolfovillordo@gmail.com> --- drivers/staging/gasket/gasket_sysfs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/gasket/gasket_sysfs.h b/drivers/staging/gasket/gasket_sysfs.h index ab5aa351d555..d5e167dfbe76 100644 --- a/drivers/staging/gasket/gasket_sysfs.h +++ b/drivers/staging/gasket/gasket_sysfs.h @@ -71,7 +71,7 @@ struct gasket_sysfs_attribute { #define GASKET_SYSFS_RO(_name, _show_function, _attr_type) \ { \ - .attr = __ATTR(_name, S_IRUGO, _show_function, NULL), \ + .attr = __ATTR(_name, 0444, _show_function, NULL), \ .data.attr_type = _attr_type \ } -- 2.17.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: gasket: replace symbolic permissions 2020-06-01 0:52 [PATCH] staging: gasket: replace symbolic permissions Rodolfo C. Villordo @ 2020-06-18 7:47 ` Greg Kroah-Hartman 2020-06-19 8:27 ` Rodolfo C Villordo 0 siblings, 1 reply; 5+ messages in thread From: Greg Kroah-Hartman @ 2020-06-18 7:47 UTC (permalink / raw) To: Rodolfo C. Villordo Cc: linux-kernel, devel, Richard Yeh, Rob Springer, Todd Poynor On Mon, Jun 01, 2020 at 12:52:40AM +0000, Rodolfo C. Villordo wrote: > WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'. > + .attr = __ATTR(_name, S_IRUGO, _show_function, NULL), \ > warning detected by checkpatch.pl > > Signed-off-by: Rodolfo C. Villordo <rodolfovillordo@gmail.com> > --- > drivers/staging/gasket/gasket_sysfs.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/gasket/gasket_sysfs.h b/drivers/staging/gasket/gasket_sysfs.h > index ab5aa351d555..d5e167dfbe76 100644 > --- a/drivers/staging/gasket/gasket_sysfs.h > +++ b/drivers/staging/gasket/gasket_sysfs.h > @@ -71,7 +71,7 @@ struct gasket_sysfs_attribute { > > #define GASKET_SYSFS_RO(_name, _show_function, _attr_type) \ > { \ > - .attr = __ATTR(_name, S_IRUGO, _show_function, NULL), \ > + .attr = __ATTR(_name, 0444, _show_function, NULL), \ What about using __ATTR_RO() instead? thanks, greg k-h ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: gasket: replace symbolic permissions 2020-06-18 7:47 ` Greg Kroah-Hartman @ 2020-06-19 8:27 ` Rodolfo C Villordo 2020-06-19 8:32 ` Greg Kroah-Hartman 0 siblings, 1 reply; 5+ messages in thread From: Rodolfo C Villordo @ 2020-06-19 8:27 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-kernel, devel, Richard Yeh, Rob Springer, Todd Poynor On Thu, Jun 18, 2020 at 09:47:50AM +0200, Greg Kroah-Hartman wrote: > On Mon, Jun 01, 2020 at 12:52:40AM +0000, Rodolfo C. Villordo wrote: > > WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'. > > + .attr = __ATTR(_name, S_IRUGO, _show_function, NULL), \ > > warning detected by checkpatch.pl > > > > Signed-off-by: Rodolfo C. Villordo <rodolfovillordo@gmail.com> > > --- > > drivers/staging/gasket/gasket_sysfs.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/gasket/gasket_sysfs.h b/drivers/staging/gasket/gasket_sysfs.h > > index ab5aa351d555..d5e167dfbe76 100644 > > --- a/drivers/staging/gasket/gasket_sysfs.h > > +++ b/drivers/staging/gasket/gasket_sysfs.h > > @@ -71,7 +71,7 @@ struct gasket_sysfs_attribute { > > > > #define GASKET_SYSFS_RO(_name, _show_function, _attr_type) \ > > { \ > > - .attr = __ATTR(_name, S_IRUGO, _show_function, NULL), \ > > + .attr = __ATTR(_name, 0444, _show_function, NULL), \ > > What about using __ATTR_RO() instead? > I'm not sure if __ATTR_RO() is a good match here. The GASKET_SYSFS_RO() is invoked with different show functions across the code. These functions don't follow the name pattern attr_name_show used in __ATTR_RO(). Please correct me if I misunderstood anything. ### from include/linux/sysfs.h ### #define __ATTR_RO(_name) { \ .attr = { .name = __stringify(_name), .mode = 0444 }, \ .show = _name##_show, \ } ### ### macro usage across the driver: ### $ grep GASKET_SYSFS_RO drivers/staging/gasket/* drivers/staging/gasket/apex_driver.c: GASKET_SYSFS_RO(node_0_page_table_entries, sysfs_show, drivers/staging/gasket/apex_driver.c: GASKET_SYSFS_RO(node_0_simple_page_table_entries, sysfs_show, drivers/staging/gasket/apex_driver.c: GASKET_SYSFS_RO(node_0_num_mapped_pages, sysfs_show, drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(bar_offsets, gasket_sysfs_data_show, ATTR_BAR_OFFSETS), drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(bar_sizes, gasket_sysfs_data_show, ATTR_BAR_SIZES), drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(driver_version, gasket_sysfs_data_show, drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(framework_version, gasket_sysfs_data_show, drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(device_type, gasket_sysfs_data_show, ATTR_DEVICE_TYPE), drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(revision, gasket_sysfs_data_show, drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(pci_address, gasket_sysfs_data_show, ATTR_PCI_ADDRESS), drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(status, gasket_sysfs_data_show, ATTR_STATUS), drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(is_device_owned, gasket_sysfs_data_show, drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(device_owner, gasket_sysfs_data_show, drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(write_open_count, gasket_sysfs_data_show, drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(reset_count, gasket_sysfs_data_show, ATTR_RESET_COUNT), drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(user_mem_ranges, gasket_sysfs_data_show, drivers/staging/gasket/gasket_interrupt.c: GASKET_SYSFS_RO(interrupt_counts, interrupt_sysfs_show, ### Thank you! > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: gasket: replace symbolic permissions 2020-06-19 8:27 ` Rodolfo C Villordo @ 2020-06-19 8:32 ` Greg Kroah-Hartman 2020-06-22 7:13 ` Rodolfo C Villordo 0 siblings, 1 reply; 5+ messages in thread From: Greg Kroah-Hartman @ 2020-06-19 8:32 UTC (permalink / raw) To: Rodolfo C Villordo Cc: devel, Richard Yeh, Todd Poynor, Rob Springer, linux-kernel On Fri, Jun 19, 2020 at 08:27:14AM +0000, Rodolfo C Villordo wrote: > On Thu, Jun 18, 2020 at 09:47:50AM +0200, Greg Kroah-Hartman wrote: > > On Mon, Jun 01, 2020 at 12:52:40AM +0000, Rodolfo C. Villordo wrote: > > > WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'. > > > + .attr = __ATTR(_name, S_IRUGO, _show_function, NULL), \ > > > warning detected by checkpatch.pl > > > > > > Signed-off-by: Rodolfo C. Villordo <rodolfovillordo@gmail.com> > > > --- > > > drivers/staging/gasket/gasket_sysfs.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/staging/gasket/gasket_sysfs.h b/drivers/staging/gasket/gasket_sysfs.h > > > index ab5aa351d555..d5e167dfbe76 100644 > > > --- a/drivers/staging/gasket/gasket_sysfs.h > > > +++ b/drivers/staging/gasket/gasket_sysfs.h > > > @@ -71,7 +71,7 @@ struct gasket_sysfs_attribute { > > > > > > #define GASKET_SYSFS_RO(_name, _show_function, _attr_type) \ > > > { \ > > > - .attr = __ATTR(_name, S_IRUGO, _show_function, NULL), \ > > > + .attr = __ATTR(_name, 0444, _show_function, NULL), \ > > > > What about using __ATTR_RO() instead? > > > > I'm not sure if __ATTR_RO() is a good match here. The > GASKET_SYSFS_RO() is invoked with different show functions across the > code. These functions don't follow the name pattern attr_name_show > used in __ATTR_RO(). Please correct me if I misunderstood anything. > > ### from include/linux/sysfs.h ### > #define __ATTR_RO(_name) { \ > .attr = { .name = __stringify(_name), .mode = 0444 }, \ > .show = _name##_show, \ > } > ### > > ### macro usage across the driver: ### > $ grep GASKET_SYSFS_RO drivers/staging/gasket/* > drivers/staging/gasket/apex_driver.c: GASKET_SYSFS_RO(node_0_page_table_entries, sysfs_show, > drivers/staging/gasket/apex_driver.c: GASKET_SYSFS_RO(node_0_simple_page_table_entries, sysfs_show, > drivers/staging/gasket/apex_driver.c: GASKET_SYSFS_RO(node_0_num_mapped_pages, sysfs_show, > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(bar_offsets, gasket_sysfs_data_show, ATTR_BAR_OFFSETS), > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(bar_sizes, gasket_sysfs_data_show, ATTR_BAR_SIZES), > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(driver_version, gasket_sysfs_data_show, > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(framework_version, gasket_sysfs_data_show, > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(device_type, gasket_sysfs_data_show, ATTR_DEVICE_TYPE), > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(revision, gasket_sysfs_data_show, > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(pci_address, gasket_sysfs_data_show, ATTR_PCI_ADDRESS), > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(status, gasket_sysfs_data_show, ATTR_STATUS), > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(is_device_owned, gasket_sysfs_data_show, > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(device_owner, gasket_sysfs_data_show, > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(write_open_count, gasket_sysfs_data_show, > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(reset_count, gasket_sysfs_data_show, ATTR_RESET_COUNT), > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(user_mem_ranges, gasket_sysfs_data_show, > drivers/staging/gasket/gasket_interrupt.c: GASKET_SYSFS_RO(interrupt_counts, interrupt_sysfs_show, > ### Ugh, you are right, that's a mess. Your original patch is fine, can you resend it and say in the changelog why it's not ok to use __ATTR_RO()? thanks, greg k-h ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: gasket: replace symbolic permissions 2020-06-19 8:32 ` Greg Kroah-Hartman @ 2020-06-22 7:13 ` Rodolfo C Villordo 0 siblings, 0 replies; 5+ messages in thread From: Rodolfo C Villordo @ 2020-06-22 7:13 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: devel, Richard Yeh, Todd Poynor, Rob Springer, linux-kernel On Fri, Jun 19, 2020 at 10:32:24AM +0200, Greg Kroah-Hartman wrote: > On Fri, Jun 19, 2020 at 08:27:14AM +0000, Rodolfo C Villordo wrote: > > On Thu, Jun 18, 2020 at 09:47:50AM +0200, Greg Kroah-Hartman wrote: > > > On Mon, Jun 01, 2020 at 12:52:40AM +0000, Rodolfo C. Villordo wrote: > > > > WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'. > > > > + .attr = __ATTR(_name, S_IRUGO, _show_function, NULL), \ > > > > warning detected by checkpatch.pl > > > > > > > > Signed-off-by: Rodolfo C. Villordo <rodolfovillordo@gmail.com> > > > > --- > > > > drivers/staging/gasket/gasket_sysfs.h | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/staging/gasket/gasket_sysfs.h b/drivers/staging/gasket/gasket_sysfs.h > > > > index ab5aa351d555..d5e167dfbe76 100644 > > > > --- a/drivers/staging/gasket/gasket_sysfs.h > > > > +++ b/drivers/staging/gasket/gasket_sysfs.h > > > > @@ -71,7 +71,7 @@ struct gasket_sysfs_attribute { > > > > > > > > #define GASKET_SYSFS_RO(_name, _show_function, _attr_type) \ > > > > { \ > > > > - .attr = __ATTR(_name, S_IRUGO, _show_function, NULL), \ > > > > + .attr = __ATTR(_name, 0444, _show_function, NULL), \ > > > > > > What about using __ATTR_RO() instead? > > > > > > > I'm not sure if __ATTR_RO() is a good match here. The > > GASKET_SYSFS_RO() is invoked with different show functions across the > > code. These functions don't follow the name pattern attr_name_show > > used in __ATTR_RO(). Please correct me if I misunderstood anything. > > > > ### from include/linux/sysfs.h ### > > #define __ATTR_RO(_name) { \ > > .attr = { .name = __stringify(_name), .mode = 0444 }, \ > > .show = _name##_show, \ > > } > > ### > > > > ### macro usage across the driver: ### > > $ grep GASKET_SYSFS_RO drivers/staging/gasket/* > > drivers/staging/gasket/apex_driver.c: GASKET_SYSFS_RO(node_0_page_table_entries, sysfs_show, > > drivers/staging/gasket/apex_driver.c: GASKET_SYSFS_RO(node_0_simple_page_table_entries, sysfs_show, > > drivers/staging/gasket/apex_driver.c: GASKET_SYSFS_RO(node_0_num_mapped_pages, sysfs_show, > > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(bar_offsets, gasket_sysfs_data_show, ATTR_BAR_OFFSETS), > > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(bar_sizes, gasket_sysfs_data_show, ATTR_BAR_SIZES), > > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(driver_version, gasket_sysfs_data_show, > > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(framework_version, gasket_sysfs_data_show, > > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(device_type, gasket_sysfs_data_show, ATTR_DEVICE_TYPE), > > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(revision, gasket_sysfs_data_show, > > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(pci_address, gasket_sysfs_data_show, ATTR_PCI_ADDRESS), > > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(status, gasket_sysfs_data_show, ATTR_STATUS), > > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(is_device_owned, gasket_sysfs_data_show, > > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(device_owner, gasket_sysfs_data_show, > > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(write_open_count, gasket_sysfs_data_show, > > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(reset_count, gasket_sysfs_data_show, ATTR_RESET_COUNT), > > drivers/staging/gasket/gasket_core.c: GASKET_SYSFS_RO(user_mem_ranges, gasket_sysfs_data_show, > > drivers/staging/gasket/gasket_interrupt.c: GASKET_SYSFS_RO(interrupt_counts, interrupt_sysfs_show, > > ### > > Ugh, you are right, that's a mess. Your original patch is fine, can you > resend it and say in the changelog why it's not ok to use __ATTR_RO()? > Sure, doing it now. Thanks > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-06-22 7:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-01 0:52 [PATCH] staging: gasket: replace symbolic permissions Rodolfo C. Villordo 2020-06-18 7:47 ` Greg Kroah-Hartman 2020-06-19 8:27 ` Rodolfo C Villordo 2020-06-19 8:32 ` Greg Kroah-Hartman 2020-06-22 7:13 ` Rodolfo C Villordo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).