Patchwork powerpc/kernel/sysfs: cleanup set up macros for pmc/non pmc sprs

login
register
mail settings
Submitter Madhavan Srinivasan
Date Sept. 30, 2013, 11:17 a.m.
Message ID <1380539849-6162-1-git-send-email-maddy@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/279116/
State Superseded
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

Madhavan Srinivasan - Sept. 30, 2013, 11:17 a.m.
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.

Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/sysfs.c |   95 ++++++++++++++++++++++++++++---------------
 1 file changed, 63 insertions(+), 32 deletions(-)

--
1.7.10.4
Michael Ellerman - Oct. 1, 2013, 4:50 a.m.
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
Madhavan Srinivasan - Oct. 1, 2013, 6:44 a.m.
On Tuesday 01 October 2013 10:20 AM, Michael Ellerman wrote:
> 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.
> 
Will change it.
>> 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.
>
Sorry about that. Will make the changes.

> cheers
> 
Thanks for the feedback

Patch

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; \
+}
+
 
 /* Let's define all possible registers, we'll only hook up the ones
  * that are implemented on the current processor
@@ -174,10 +205,10 @@  SYSFS_PMCSETUP(pmc7, SPRN_PMC7);
 SYSFS_PMCSETUP(pmc8, SPRN_PMC8);
 
 SYSFS_PMCSETUP(mmcra, SPRN_MMCRA);
-SYSFS_PMCSETUP(purr, SPRN_PURR);
-SYSFS_PMCSETUP(spurr, SPRN_SPURR);
-SYSFS_PMCSETUP(dscr, SPRN_DSCR);
-SYSFS_PMCSETUP(pir, SPRN_PIR);
+SYSFS_SPRSETUP(purr, SPRN_PURR);
+SYSFS_SPRSETUP(spurr, SPRN_SPURR);
+SYSFS_SPRSETUP(dscr, SPRN_DSCR);
+SYSFS_SPRSETUP(pir, SPRN_PIR);
 
 static DEVICE_ATTR(mmcra, 0600, show_mmcra, store_mmcra);
 static DEVICE_ATTR(spurr, 0400, show_spurr, NULL);
@@ -238,34 +269,34 @@  SYSFS_PMCSETUP(pa6t_pmc3, SPRN_PA6T_PMC3);
 SYSFS_PMCSETUP(pa6t_pmc4, SPRN_PA6T_PMC4);
 SYSFS_PMCSETUP(pa6t_pmc5, SPRN_PA6T_PMC5);
 #ifdef CONFIG_DEBUG_KERNEL
-SYSFS_PMCSETUP(hid0, SPRN_HID0);
-SYSFS_PMCSETUP(hid1, SPRN_HID1);
-SYSFS_PMCSETUP(hid4, SPRN_HID4);
-SYSFS_PMCSETUP(hid5, SPRN_HID5);
-SYSFS_PMCSETUP(ima0, SPRN_PA6T_IMA0);
-SYSFS_PMCSETUP(ima1, SPRN_PA6T_IMA1);
-SYSFS_PMCSETUP(ima2, SPRN_PA6T_IMA2);
-SYSFS_PMCSETUP(ima3, SPRN_PA6T_IMA3);
-SYSFS_PMCSETUP(ima4, SPRN_PA6T_IMA4);
-SYSFS_PMCSETUP(ima5, SPRN_PA6T_IMA5);
-SYSFS_PMCSETUP(ima6, SPRN_PA6T_IMA6);
-SYSFS_PMCSETUP(ima7, SPRN_PA6T_IMA7);
-SYSFS_PMCSETUP(ima8, SPRN_PA6T_IMA8);
-SYSFS_PMCSETUP(ima9, SPRN_PA6T_IMA9);
-SYSFS_PMCSETUP(imaat, SPRN_PA6T_IMAAT);
-SYSFS_PMCSETUP(btcr, SPRN_PA6T_BTCR);
-SYSFS_PMCSETUP(pccr, SPRN_PA6T_PCCR);
-SYSFS_PMCSETUP(rpccr, SPRN_PA6T_RPCCR);
-SYSFS_PMCSETUP(der, SPRN_PA6T_DER);
-SYSFS_PMCSETUP(mer, SPRN_PA6T_MER);
-SYSFS_PMCSETUP(ber, SPRN_PA6T_BER);
-SYSFS_PMCSETUP(ier, SPRN_PA6T_IER);
-SYSFS_PMCSETUP(sier, SPRN_PA6T_SIER);
-SYSFS_PMCSETUP(siar, SPRN_PA6T_SIAR);
-SYSFS_PMCSETUP(tsr0, SPRN_PA6T_TSR0);
-SYSFS_PMCSETUP(tsr1, SPRN_PA6T_TSR1);
-SYSFS_PMCSETUP(tsr2, SPRN_PA6T_TSR2);
-SYSFS_PMCSETUP(tsr3, SPRN_PA6T_TSR3);
+SYSFS_SPRSETUP(hid0, SPRN_HID0);
+SYSFS_SPRSETUP(hid1, SPRN_HID1);
+SYSFS_SPRSETUP(hid4, SPRN_HID4);
+SYSFS_SPRSETUP(hid5, SPRN_HID5);
+SYSFS_SPRSETUP(ima0, SPRN_PA6T_IMA0);
+SYSFS_SPRSETUP(ima1, SPRN_PA6T_IMA1);
+SYSFS_SPRSETUP(ima2, SPRN_PA6T_IMA2);
+SYSFS_SPRSETUP(ima3, SPRN_PA6T_IMA3);
+SYSFS_SPRSETUP(ima4, SPRN_PA6T_IMA4);
+SYSFS_SPRSETUP(ima5, SPRN_PA6T_IMA5);
+SYSFS_SPRSETUP(ima6, SPRN_PA6T_IMA6);
+SYSFS_SPRSETUP(ima7, SPRN_PA6T_IMA7);
+SYSFS_SPRSETUP(ima8, SPRN_PA6T_IMA8);
+SYSFS_SPRSETUP(ima9, SPRN_PA6T_IMA9);
+SYSFS_SPRSETUP(imaat, SPRN_PA6T_IMAAT);
+SYSFS_SPRSETUP(btcr, SPRN_PA6T_BTCR);
+SYSFS_SPRSETUP(pccr, SPRN_PA6T_PCCR);
+SYSFS_SPRSETUP(rpccr, SPRN_PA6T_RPCCR);
+SYSFS_SPRSETUP(der, SPRN_PA6T_DER);
+SYSFS_SPRSETUP(mer, SPRN_PA6T_MER);
+SYSFS_SPRSETUP(ber, SPRN_PA6T_BER);
+SYSFS_SPRSETUP(ier, SPRN_PA6T_IER);
+SYSFS_SPRSETUP(sier, SPRN_PA6T_SIER);
+SYSFS_SPRSETUP(siar, SPRN_PA6T_SIAR);
+SYSFS_SPRSETUP(tsr0, SPRN_PA6T_TSR0);
+SYSFS_SPRSETUP(tsr1, SPRN_PA6T_TSR1);
+SYSFS_SPRSETUP(tsr2, SPRN_PA6T_TSR2);
+SYSFS_SPRSETUP(tsr3, SPRN_PA6T_TSR3);
 #endif /* CONFIG_DEBUG_KERNEL */
 #endif /* HAS_PPC_PMC_PA6T */