Re: [PATCH] powerpc/kernel/sysfs: cleanup set up macros for pmc/non pmc sprs

[Michael Ellerman <michael@ellerman.id.au>]

On Mon, Sep 30, 2013 at 04:47:29PM +0530, Madhavan Srinivasan wrote:
> Currently pmc setup macros are used for non pmc sprs. This patch
> add new set of macros and cleans up the code to use the new setup macro
> for non pmc sprs.

Hi Maddy,

Firstly you should use "PMC" not pmc, it's an acronym. You should also
spell out what it means the first time you use it, eg:

  Currently the PMC (Performance Monitor Counter) macros are used ..

Secondly you need to say _why_ it is a bad idea to use the PMC macros
for non-PMC SPRs.

> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
> index 27a90b9..73b6f9f 100644
> --- a/arch/powerpc/kernel/sysfs.c
> +++ b/arch/powerpc/kernel/sysfs.c
> @@ -139,6 +139,37 @@ static ssize_t __used \
>  	return count; \
>  }
> 
> +#define SYSFS_SPRSETUP(NAME, ADDRESS) \
> +static void read_##NAME(void *val) \
> +{ \
> +	*(unsigned long *)val = mfspr(ADDRESS);	\
> +} \
> +static void write_##NAME(void *val) \
> +{ \
> +	mtspr(ADDRESS, *(unsigned long *)val);	\
> +} \
> +static ssize_t show_##NAME(struct device *dev, \
> +			struct device_attribute *attr, \
> +			char *buf) \
> +{ \
> +	struct cpu *cpu = container_of(dev, struct cpu, dev); \
> +	unsigned long val; \
> +	smp_call_function_single(cpu->dev.id, read_##NAME, &val, 1);	\
> +	return sprintf(buf, "%lx\n", val); \
> +} \
> +static ssize_t __used \
> +	store_##NAME(struct device *dev, struct device_attribute *attr, \
> +			const char *buf, size_t count) \
> +{ \
> +	struct cpu *cpu = container_of(dev, struct cpu, dev); \
> +	unsigned long val; \
> +	int ret = sscanf(buf, "%lx", &val); \
> +	if (ret != 1) \
> +		return -EINVAL; \
> +	smp_call_function_single(cpu->dev.id, write_##NAME, &val, 1); \
> +	return count; \
> +}

This is basically a complete copy of the SYSFS_PMCSETUP() macro, except
for the one line removal of the call to ppc_enable_pmcs().

You should be able to do better, by defining a macro that does all the
boiler plate and takes an "extra" argument, which for PMCs is
"ppc_enable_pmcs()" and for regular SPRs is empty.

cheers