| Message ID | 4efea815a9fddfc0dc1b29d16f7485de0f8ee866.1599501047.git.joe@perches.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | drivers core: Use sysfs_emit functions | expand |
Hi Joe,
I love your patch! Yet something to improve:
[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on pm/linux-next linus/master v5.9-rc4 next-20200903]
[cannot apply to linux/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Joe-Perches/drivers-core-Use-sysfs_emit-functions/20200908-021333
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 4024823563d05bbe880885c9987d14a7130801da
config: s390-randconfig-s031-20200908 (attached as .config)
compiler: s390-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.2-191-g10164920-dirty
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=s390
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/base/memory.c: In function 'phys_index_show':
drivers/base/memory.c:122:9: error: implicit declaration of function 'sysfs_emit'; did you mean 'sysfs_init'? [-Werror=implicit-function-declaration]
122 | return sysfs_emit(buf, "%08lx\n", phys_index);
| ^~~~~~~~~~
| sysfs_init
drivers/base/memory.c: In function 'print_allowed_zone':
>> drivers/base/memory.c:317:9: error: implicit declaration of function 'sysfs_emit_at'; did you mean 'sysfs_init'? [-Werror=implicit-function-declaration]
317 | return sysfs_emit_at(buf, len, " %s", zone->name);
| ^~~~~~~~~~~~~
| sysfs_init
cc1: some warnings being treated as errors
# https://github.com/0day-ci/linux/commit/a8789a23413de463f5e591d57748493242aa4be4
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Joe-Perches/drivers-core-Use-sysfs_emit-functions/20200908-021333
git checkout a8789a23413de463f5e591d57748493242aa4be4
vim +317 drivers/base/memory.c
306
307 #ifdef CONFIG_MEMORY_HOTREMOVE
308 static int print_allowed_zone(char *buf, int len, int nid,
309 unsigned long start_pfn, unsigned long nr_pages,
310 int online_type, struct zone *default_zone)
311 {
312 struct zone *zone;
313
314 zone = zone_for_pfn_range(online_type, nid, start_pfn, nr_pages);
315 if (zone == default_zone)
316 return 0;
> 317 return sysfs_emit_at(buf, len, " %s", zone->name);
318 }
319
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Mon, Sep 07, 2020 at 10:58:06AM -0700, Joe Perches wrote: > strcat is no longer necessary for sysfs_emit and sysfs_emit_at uses. > > Convert the strcat uses to sysfs_emit calls and neaten other block > uses of direct returns to use an intermediate const char *. > > Signed-off-by: Joe Perches <joe@perches.com> > --- > drivers/base/cacheinfo.c | 26 ++++++++++++++------- > drivers/base/core.c | 10 ++++---- > drivers/base/memory.c | 47 ++++++++++++++++++-------------------- > drivers/base/power/sysfs.c | 23 +++++++++++-------- > 4 files changed, 59 insertions(+), 47 deletions(-) > > diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c > index 6a8c2b5881be..7b3608fb8700 100644 > --- a/drivers/base/cacheinfo.c > +++ b/drivers/base/cacheinfo.c > @@ -404,17 +404,23 @@ static ssize_t type_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > struct cacheinfo *this_leaf = dev_get_drvdata(dev); > + const char *type = NULL; > > switch (this_leaf->type) { > case CACHE_TYPE_DATA: > - return sysfs_emit(buf, "Data\n"); > + type = "Data"; > + break; > case CACHE_TYPE_INST: > - return sysfs_emit(buf, "Instruction\n"); > + type = "Instruction"; > + break; > case CACHE_TYPE_UNIFIED: > - return sysfs_emit(buf, "Unified\n"); > + type = "Unified"; > + break; > default: > return -EINVAL; > } > + > + return sysfs_emit(buf, "%s\n", type); > } This function is now longer, with an assignment that is not needed (type=NULL), so why make this "cleanup"? > > static ssize_t allocation_policy_show(struct device *dev, > @@ -422,15 +428,19 @@ static ssize_t allocation_policy_show(struct device *dev, > { > struct cacheinfo *this_leaf = dev_get_drvdata(dev); > unsigned int ci_attr = this_leaf->attributes; > - int n = 0; > + const char *type = NULL; It's a policy, not a type. > > if ((ci_attr & CACHE_READ_ALLOCATE) && (ci_attr & CACHE_WRITE_ALLOCATE)) > - n = sysfs_emit(buf, "ReadWriteAllocate\n"); > + type = "ReadWriteAllocate"; > else if (ci_attr & CACHE_READ_ALLOCATE) > - n = sysfs_emit(buf, "ReadAllocate\n"); > + type = "ReadAllocate"; > else if (ci_attr & CACHE_WRITE_ALLOCATE) > - n = sysfs_emit(buf, "WriteAllocate\n"); > - return n; > + type = "WriteAllocate"; > + > + if (!type) > + return 0; > + > + return sysfs_emit(buf, "%s\n", type); > } Overall, this is a bit nicer, thanks. > > static ssize_t write_policy_show(struct device *dev, > diff --git a/drivers/base/core.c b/drivers/base/core.c > index fdadfa047968..2314a4e541b4 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -267,16 +267,16 @@ static ssize_t auto_remove_on_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > struct device_link *link = to_devlink(dev); > - char *str; > + char *type; > > if (link->flags & DL_FLAG_AUTOREMOVE_SUPPLIER) > - str = "supplier unbind"; > + type = "supplier unbind"; > else if (link->flags & DL_FLAG_AUTOREMOVE_CONSUMER) > - str = "consumer unbind"; > + type = "consumer unbind"; > else > - str = "never"; > + type = "never"; > > - return sysfs_emit(buf, "%s\n", str); > + return sysfs_emit(buf, "%s\n", type); This isn't a "type", it is just a string :( > } > static DEVICE_ATTR_RO(auto_remove_on); > > diff --git a/drivers/base/memory.c b/drivers/base/memory.c > index 2fdab1ea036b..b559f2b1d1ff 100644 > --- a/drivers/base/memory.c > +++ b/drivers/base/memory.c > @@ -139,7 +139,7 @@ static ssize_t state_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > struct memory_block *mem = to_memory_block(dev); > - ssize_t len = 0; > + const char *type; > > /* > * We can probably put these states in a nice little array > @@ -147,22 +147,20 @@ static ssize_t state_show(struct device *dev, struct device_attribute *attr, > */ > switch (mem->state) { > case MEM_ONLINE: > - len = sysfs_emit(buf, "online\n"); > + type = "online"; > break; > case MEM_OFFLINE: > - len = sysfs_emit(buf, "offline\n"); > + type = "offline"; > break; > case MEM_GOING_OFFLINE: > - len = sysfs_emit(buf, "going-offline\n"); > + type = "going-offline"; > break; > default: > - len = sysfs_emit(buf, "ERROR-UNKNOWN-%ld\n", > - mem->state); > WARN_ON(1); > - break; > + return sysfs_emit(buf, "ERROR-UNKNOWN-%ld\n", mem->state); > } > > - return len; > + return sysfs_emit(buf, "%s\n", type); Again, not a type, it's a state. And you did not merge all sysfs_emit() calls into one here, so it's messier now, don't you think? > } > > int memory_notify(unsigned long val, void *v) > @@ -307,17 +305,16 @@ static ssize_t phys_device_show(struct device *dev, > } > > #ifdef CONFIG_MEMORY_HOTREMOVE > -static void print_allowed_zone(char *buf, int nid, unsigned long start_pfn, > - unsigned long nr_pages, int online_type, > - struct zone *default_zone) > +static int print_allowed_zone(char *buf, int len, int nid, > + unsigned long start_pfn, unsigned long nr_pages, > + int online_type, struct zone *default_zone) Unrelated change :( > { > struct zone *zone; > > zone = zone_for_pfn_range(online_type, nid, start_pfn, nr_pages); > - if (zone != default_zone) { > - strcat(buf, " "); > - strcat(buf, zone->name); > - } > + if (zone == default_zone) > + return 0; > + return sysfs_emit_at(buf, len, " %s", zone->name); > } Nicer looking, thanks. > > static ssize_t valid_zones_show(struct device *dev, > @@ -327,6 +324,7 @@ static ssize_t valid_zones_show(struct device *dev, > unsigned long start_pfn = section_nr_to_pfn(mem->start_section_nr); > unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block; > struct zone *default_zone; > + int len = 0; > int nid; > > /* > @@ -341,24 +339,23 @@ static ssize_t valid_zones_show(struct device *dev, > default_zone = test_pages_in_a_zone(start_pfn, > start_pfn + nr_pages); > if (!default_zone) > - return sysfs_emit(buf, "none\n"); > - strcat(buf, default_zone->name); > + return sysfs_emit(buf, "%s\n", "none"); > + len += sysfs_emit_at(buf, len, "%s", default_zone->name); > goto out; > } > > nid = mem->nid; > default_zone = zone_for_pfn_range(MMOP_ONLINE, nid, start_pfn, > nr_pages); > - strcat(buf, default_zone->name); > > - print_allowed_zone(buf, nid, start_pfn, nr_pages, MMOP_ONLINE_KERNEL, > - default_zone); > - print_allowed_zone(buf, nid, start_pfn, nr_pages, MMOP_ONLINE_MOVABLE, > - default_zone); > + len += sysfs_emit_at(buf, len, "%s", default_zone->name); > + len += print_allowed_zone(buf, len, nid, start_pfn, nr_pages, > + MMOP_ONLINE_KERNEL, default_zone); > + len += print_allowed_zone(buf, len, nid, start_pfn, nr_pages, > + MMOP_ONLINE_MOVABLE, default_zone); > out: > - strcat(buf, "\n"); > - > - return strlen(buf); > + len += sysfs_emit_at(buf, len, "%s", "\n"); > + return len; This is better. > } > static DEVICE_ATTR_RO(valid_zones); > #endif > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c > index 4276b792d0aa..c3d303c4c9c2 100644 > --- a/drivers/base/power/sysfs.c > +++ b/drivers/base/power/sysfs.c > @@ -255,9 +255,9 @@ static ssize_t pm_qos_latency_tolerance_us_show(struct device *dev, > s32 value = dev_pm_qos_get_user_latency_tolerance(dev); > > if (value < 0) > - return sysfs_emit(buf, "auto\n"); > + return sysfs_emit(buf, "%s\n", "auto"); > if (value == PM_QOS_LATENCY_ANY) > - return sysfs_emit(buf, "any\n"); > + return sysfs_emit(buf, "%s\n", "any"); > > return sysfs_emit(buf, "%d\n", value); > } Unrelated change :( > @@ -538,13 +538,18 @@ static DEVICE_ATTR_RO(runtime_active_kids); > static ssize_t runtime_enabled_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > - if (dev->power.disable_depth && (dev->power.runtime_auto == false)) > - return sysfs_emit(buf, "disabled & forbidden\n"); > - if (dev->power.disable_depth) > - return sysfs_emit(buf, "disabled\n"); > - if (dev->power.runtime_auto == false) > - return sysfs_emit(buf, "forbidden\n"); > - return sysfs_emit(buf, "enabled\n"); > + const char *type; > + > + if (dev->power.disable_depth && !dev->power.runtime_auto) > + type = "disabled & forbidden"; > + else if (dev->power.disable_depth) > + type = "disabled"; > + else if (!dev->power.runtime_auto) > + type = "forbidden"; > + else > + type = "enabled"; > + > + return sysfs_emit(buf, "%s\n", type); It's not a type :( thanks, greg k-h
On Tue, 2020-09-08 at 10:32 +0200, Greg Kroah-Hartman wrote: > On Mon, Sep 07, 2020 at 10:58:06AM -0700, Joe Perches wrote: > > strcat is no longer necessary for sysfs_emit and sysfs_emit_at uses. > > > > Convert the strcat uses to sysfs_emit calls and neaten other block > > uses of direct returns to use an intermediate const char *. [] > This function is now longer, with an assignment that is not needed > (type=NULL), so why make this "cleanup"? It's smaller object code. [] > > Again, not a type, it's a state. And you did not merge all sysfs_emit() > calls into one here, so it's messier now, don't you think? You can't because the default type uses an argument and not a fixed string. I don't think it's messier, no. > > int memory_notify(unsigned long val, void *v) > > @@ -307,17 +305,16 @@ static ssize_t phys_device_show(struct device *dev, > > } > > > > #ifdef CONFIG_MEMORY_HOTREMOVE > > -static void print_allowed_zone(char *buf, int nid, unsigned long start_pfn, > > - unsigned long nr_pages, int online_type, > > - struct zone *default_zone) > > +static int print_allowed_zone(char *buf, int len, int nid, > > + unsigned long start_pfn, unsigned long nr_pages, > > + int online_type, struct zone *default_zone) > > Unrelated change :( No it's not, it's outputting to buf so it needs len to output to appropriate spot to be able to use sysfs_emit_at. > > { > > struct zone *zone; > > > > zone = zone_for_pfn_range(online_type, nid, start_pfn, nr_pages); > > - if (zone != default_zone) { > > - strcat(buf, " "); > > - strcat(buf, zone->name); > > - } > > + if (zone == default_zone) > > + return 0; > > + return sysfs_emit_at(buf, len, " %s", zone->name); here. > [] > This is better. All of it is better. <smile> > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c [] > > @@ -255,9 +255,9 @@ static ssize_t pm_qos_latency_tolerance_us_show(struct device *dev, > > s32 value = dev_pm_qos_get_user_latency_tolerance(dev); > > > > if (value < 0) > > - return sysfs_emit(buf, "auto\n"); > > + return sysfs_emit(buf, "%s\n", "auto"); > > if (value == PM_QOS_LATENCY_ANY) > > - return sysfs_emit(buf, "any\n"); > > + return sysfs_emit(buf, "%s\n", "any"); > > > > return sysfs_emit(buf, "%d\n", value); > > } > > Unrelated change :( Again, no it's not unrelated, it's consistent.
diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c index 6a8c2b5881be..7b3608fb8700 100644 --- a/drivers/base/cacheinfo.c +++ b/drivers/base/cacheinfo.c @@ -404,17 +404,23 @@ static ssize_t type_show(struct device *dev, struct device_attribute *attr, char *buf) { struct cacheinfo *this_leaf = dev_get_drvdata(dev); + const char *type = NULL; switch (this_leaf->type) { case CACHE_TYPE_DATA: - return sysfs_emit(buf, "Data\n"); + type = "Data"; + break; case CACHE_TYPE_INST: - return sysfs_emit(buf, "Instruction\n"); + type = "Instruction"; + break; case CACHE_TYPE_UNIFIED: - return sysfs_emit(buf, "Unified\n"); + type = "Unified"; + break; default: return -EINVAL; } + + return sysfs_emit(buf, "%s\n", type); } static ssize_t allocation_policy_show(struct device *dev, @@ -422,15 +428,19 @@ static ssize_t allocation_policy_show(struct device *dev, { struct cacheinfo *this_leaf = dev_get_drvdata(dev); unsigned int ci_attr = this_leaf->attributes; - int n = 0; + const char *type = NULL; if ((ci_attr & CACHE_READ_ALLOCATE) && (ci_attr & CACHE_WRITE_ALLOCATE)) - n = sysfs_emit(buf, "ReadWriteAllocate\n"); + type = "ReadWriteAllocate"; else if (ci_attr & CACHE_READ_ALLOCATE) - n = sysfs_emit(buf, "ReadAllocate\n"); + type = "ReadAllocate"; else if (ci_attr & CACHE_WRITE_ALLOCATE) - n = sysfs_emit(buf, "WriteAllocate\n"); - return n; + type = "WriteAllocate"; + + if (!type) + return 0; + + return sysfs_emit(buf, "%s\n", type); } static ssize_t write_policy_show(struct device *dev, diff --git a/drivers/base/core.c b/drivers/base/core.c index fdadfa047968..2314a4e541b4 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -267,16 +267,16 @@ static ssize_t auto_remove_on_show(struct device *dev, struct device_attribute *attr, char *buf) { struct device_link *link = to_devlink(dev); - char *str; + char *type; if (link->flags & DL_FLAG_AUTOREMOVE_SUPPLIER) - str = "supplier unbind"; + type = "supplier unbind"; else if (link->flags & DL_FLAG_AUTOREMOVE_CONSUMER) - str = "consumer unbind"; + type = "consumer unbind"; else - str = "never"; + type = "never"; - return sysfs_emit(buf, "%s\n", str); + return sysfs_emit(buf, "%s\n", type); } static DEVICE_ATTR_RO(auto_remove_on); diff --git a/drivers/base/memory.c b/drivers/base/memory.c index 2fdab1ea036b..b559f2b1d1ff 100644 --- a/drivers/base/memory.c +++ b/drivers/base/memory.c @@ -139,7 +139,7 @@ static ssize_t state_show(struct device *dev, struct device_attribute *attr, char *buf) { struct memory_block *mem = to_memory_block(dev); - ssize_t len = 0; + const char *type; /* * We can probably put these states in a nice little array @@ -147,22 +147,20 @@ static ssize_t state_show(struct device *dev, struct device_attribute *attr, */ switch (mem->state) { case MEM_ONLINE: - len = sysfs_emit(buf, "online\n"); + type = "online"; break; case MEM_OFFLINE: - len = sysfs_emit(buf, "offline\n"); + type = "offline"; break; case MEM_GOING_OFFLINE: - len = sysfs_emit(buf, "going-offline\n"); + type = "going-offline"; break; default: - len = sysfs_emit(buf, "ERROR-UNKNOWN-%ld\n", - mem->state); WARN_ON(1); - break; + return sysfs_emit(buf, "ERROR-UNKNOWN-%ld\n", mem->state); } - return len; + return sysfs_emit(buf, "%s\n", type); } int memory_notify(unsigned long val, void *v) @@ -307,17 +305,16 @@ static ssize_t phys_device_show(struct device *dev, } #ifdef CONFIG_MEMORY_HOTREMOVE -static void print_allowed_zone(char *buf, int nid, unsigned long start_pfn, - unsigned long nr_pages, int online_type, - struct zone *default_zone) +static int print_allowed_zone(char *buf, int len, int nid, + unsigned long start_pfn, unsigned long nr_pages, + int online_type, struct zone *default_zone) { struct zone *zone; zone = zone_for_pfn_range(online_type, nid, start_pfn, nr_pages); - if (zone != default_zone) { - strcat(buf, " "); - strcat(buf, zone->name); - } + if (zone == default_zone) + return 0; + return sysfs_emit_at(buf, len, " %s", zone->name); } static ssize_t valid_zones_show(struct device *dev, @@ -327,6 +324,7 @@ static ssize_t valid_zones_show(struct device *dev, unsigned long start_pfn = section_nr_to_pfn(mem->start_section_nr); unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block; struct zone *default_zone; + int len = 0; int nid; /* @@ -341,24 +339,23 @@ static ssize_t valid_zones_show(struct device *dev, default_zone = test_pages_in_a_zone(start_pfn, start_pfn + nr_pages); if (!default_zone) - return sysfs_emit(buf, "none\n"); - strcat(buf, default_zone->name); + return sysfs_emit(buf, "%s\n", "none"); + len += sysfs_emit_at(buf, len, "%s", default_zone->name); goto out; } nid = mem->nid; default_zone = zone_for_pfn_range(MMOP_ONLINE, nid, start_pfn, nr_pages); - strcat(buf, default_zone->name); - print_allowed_zone(buf, nid, start_pfn, nr_pages, MMOP_ONLINE_KERNEL, - default_zone); - print_allowed_zone(buf, nid, start_pfn, nr_pages, MMOP_ONLINE_MOVABLE, - default_zone); + len += sysfs_emit_at(buf, len, "%s", default_zone->name); + len += print_allowed_zone(buf, len, nid, start_pfn, nr_pages, + MMOP_ONLINE_KERNEL, default_zone); + len += print_allowed_zone(buf, len, nid, start_pfn, nr_pages, + MMOP_ONLINE_MOVABLE, default_zone); out: - strcat(buf, "\n"); - - return strlen(buf); + len += sysfs_emit_at(buf, len, "%s", "\n"); + return len; } static DEVICE_ATTR_RO(valid_zones); #endif diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c index 4276b792d0aa..c3d303c4c9c2 100644 --- a/drivers/base/power/sysfs.c +++ b/drivers/base/power/sysfs.c @@ -255,9 +255,9 @@ static ssize_t pm_qos_latency_tolerance_us_show(struct device *dev, s32 value = dev_pm_qos_get_user_latency_tolerance(dev); if (value < 0) - return sysfs_emit(buf, "auto\n"); + return sysfs_emit(buf, "%s\n", "auto"); if (value == PM_QOS_LATENCY_ANY) - return sysfs_emit(buf, "any\n"); + return sysfs_emit(buf, "%s\n", "any"); return sysfs_emit(buf, "%d\n", value); } @@ -538,13 +538,18 @@ static DEVICE_ATTR_RO(runtime_active_kids); static ssize_t runtime_enabled_show(struct device *dev, struct device_attribute *attr, char *buf) { - if (dev->power.disable_depth && (dev->power.runtime_auto == false)) - return sysfs_emit(buf, "disabled & forbidden\n"); - if (dev->power.disable_depth) - return sysfs_emit(buf, "disabled\n"); - if (dev->power.runtime_auto == false) - return sysfs_emit(buf, "forbidden\n"); - return sysfs_emit(buf, "enabled\n"); + const char *type; + + if (dev->power.disable_depth && !dev->power.runtime_auto) + type = "disabled & forbidden"; + else if (dev->power.disable_depth) + type = "disabled"; + else if (!dev->power.runtime_auto) + type = "forbidden"; + else + type = "enabled"; + + return sysfs_emit(buf, "%s\n", type); } static DEVICE_ATTR_RO(runtime_enabled);
strcat is no longer necessary for sysfs_emit and sysfs_emit_at uses. Convert the strcat uses to sysfs_emit calls and neaten other block uses of direct returns to use an intermediate const char *. Signed-off-by: Joe Perches <joe@perches.com> --- drivers/base/cacheinfo.c | 26 ++++++++++++++------- drivers/base/core.c | 10 ++++---- drivers/base/memory.c | 47 ++++++++++++++++++-------------------- drivers/base/power/sysfs.c | 23 +++++++++++-------- 4 files changed, 59 insertions(+), 47 deletions(-)