On Sun, May 04, 2025 at 11:23:50PM -0700, Fenghua Yu wrote:
> Hi, Tony,
> > +#define RANGE_ATTR(name, fmt) \
> > +static ssize_t name##_show(struct kobject *kobj, \
>
> "name" is used as a macro parameter. But "name" is also used as a variable
> mre->name in the macro. checkpatch complains this kind of usage.
The checkpatch complaint is that is is used twice. Once as "mre->name"
(as you noted), but also as "__ATTR_RO(name)" two lines after.
Checkpatch is worried that the macor might be invoked with an argument
that has side effects ("foo++", or return from function call "baz()")
which would result in the side-effects happening twice.
It's a false positive in this case because:
1) This macro is only used with a simple argument (and can only ever be
used in that way.
2) It's used for a static initialization of compile time, so a 2nd
reason why side-effects from an argment are not possible.
> Maybe change the parameter "name" as something like "range_name" to avoid
> the potential confusion?
>
> > + struct kobj_attribute *attr, char *buf) \
> > +{ \
> > + struct mrrm_mem_range_entry *mre; \
> > + const char *kname = kobject_name(kobj); \
> > + int n, ret; \
> > + \
> > + ret = kstrtoint(kname + 5, 10, &n); \
> > + if (ret) \
> > + return ret; \
> > + \
> > + mre = mrrm_mem_range_entry + n; \
> > + \
> > + return sysfs_emit(buf, fmt, mre->name); \
> > +} \
> > +static struct kobj_attribute name##_attr = __ATTR_RO(name)
> > +
> > +RANGE_ATTR(base, "0x%llx\n");
> > +RANGE_ATTR(length, "0x%llx\n");
> > +RANGE_ATTR(node, "%d\n");
> > +RANGE_ATTR(local_region_id, "%d\n");
> > +RANGE_ATTR(remote_region_id, "%d\n");
...
> > +
> > static __init int mrrm_init(void)
> > {
> > int ret;
> > ret = acpi_table_parse(ACPI_SIG_MRRM, acpi_parse_mrrm);
> This blank line seems redundant. Maybe remove it so that the "if (ret < 0)"
> sentence follows the "ret = ...." sentence immediately?
Agreed. I will delete in next version.
> > - return ret;
> > + if (ret < 0)
> > + return ret;
> > +
> > + return add_boot_memory_ranges();
> > }
> > device_initcall(mrrm_init);
>
> Thanks.
>
> -Fenghua
Thanks for the review.
-Tony
>
