Messages in this thread |  | | Date | Tue, 8 Sep 2020 10:32:49 +0200 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH 2/4] drivers core: Remove strcat uses around sysfs_emit and neaten |
| |
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
|  |