Re: [PATCH v4 3/4] ACPI/MRRM: Add /sys files to describe memory ranges

["Luck, Tony" <tony.luck@intel.com>]

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
>