| Message ID | 1444064531-25607-22-git-send-email-suzuki.poulose@arm.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Oct 05, 2015 at 06:02:10PM +0100, Suzuki K. Poulose wrote: > +static int __init cpuinfo_regs_init(void) > +{ > + int cpu, ret; > + > + for_each_present_cpu(cpu) { > + struct device *dev = get_cpu_device(cpu); > + > + if (!dev) > + return -1; NAK. Go figure out why, I'm too lazy to tell you. > + > + ret = sysfs_create_group(&dev->kobj, &cpuregs_attr_group); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +device_initcall(cpuinfo_regs_init); > -- > 1.7.9.5 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 6 October 2015 at 10:09, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Mon, Oct 05, 2015 at 06:02:10PM +0100, Suzuki K. Poulose wrote: >> +static int __init cpuinfo_regs_init(void) >> +{ >> + int cpu, ret; >> + >> + for_each_present_cpu(cpu) { >> + struct device *dev = get_cpu_device(cpu); >> + >> + if (!dev) >> + return -1; > > NAK. Go figure out why, I'm too lazy to tell you. I will correct the return code to be -ENODEV. Was that the reasoning behind the NAK? > >> + >> + ret = sysfs_create_group(&dev->kobj, &cpuregs_attr_group); >> + if (ret) >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +device_initcall(cpuinfo_regs_init); >> -- >> 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tue, Oct 06, 2015 at 11:18:42AM +0100, Steve Capper wrote: > On 6 October 2015 at 10:09, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Mon, Oct 05, 2015 at 06:02:10PM +0100, Suzuki K. Poulose wrote: > >> +static int __init cpuinfo_regs_init(void) > >> +{ > >> + int cpu, ret; > >> + > >> + for_each_present_cpu(cpu) { > >> + struct device *dev = get_cpu_device(cpu); > >> + > >> + if (!dev) > >> + return -1; > > > > NAK. Go figure out why, I'm too lazy to tell you. > > I will correct the return code to be -ENODEV. > Was that the reasoning behind the NAK? I suspect the half-initialised sysfs groups also have something to do with it... Mark. > > > > >> + > >> + ret = sysfs_create_group(&dev->kobj, &cpuregs_attr_group); > >> + if (ret) > >> + return ret; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +device_initcall(cpuinfo_regs_init); > >> -- > >> 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 6 October 2015 at 11:25, Mark Rutland <mark.rutland@arm.com> wrote: > On Tue, Oct 06, 2015 at 11:18:42AM +0100, Steve Capper wrote: >> On 6 October 2015 at 10:09, Russell King - ARM Linux >> <linux@arm.linux.org.uk> wrote: >> > On Mon, Oct 05, 2015 at 06:02:10PM +0100, Suzuki K. Poulose wrote: >> >> +static int __init cpuinfo_regs_init(void) >> >> +{ >> >> + int cpu, ret; >> >> + >> >> + for_each_present_cpu(cpu) { >> >> + struct device *dev = get_cpu_device(cpu); >> >> + >> >> + if (!dev) >> >> + return -1; >> > >> > NAK. Go figure out why, I'm too lazy to tell you. >> >> I will correct the return code to be -ENODEV. >> Was that the reasoning behind the NAK? > > I suspect the half-initialised sysfs groups also have something to do > with it... Okay, cheers Mark, I see what you mean. > > Mark. > >> >> > >> >> + >> >> + ret = sysfs_create_group(&dev->kobj, &cpuregs_attr_group); >> >> + if (ret) >> >> + return ret; >> >> + } >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +device_initcall(cpuinfo_regs_init); >> >> -- >> >> 1.7.9.5 >> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Tue, Oct 06, 2015 at 11:18:42AM +0100, Steve Capper wrote: > On 6 October 2015 at 10:09, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Mon, Oct 05, 2015 at 06:02:10PM +0100, Suzuki K. Poulose wrote: > >> +static int __init cpuinfo_regs_init(void) > >> +{ > >> + int cpu, ret; > >> + > >> + for_each_present_cpu(cpu) { > >> + struct device *dev = get_cpu_device(cpu); > >> + > >> + if (!dev) > >> + return -1; > > > > NAK. Go figure out why, I'm too lazy to tell you. > > I will correct the return code to be -ENODEV. > Was that the reasoning behind the NAK? The reason behind the NAK was indeed returning -1 from a function where a negative return code is a negative errno value. I'm fed up with telling people about this, and I put it down to laziness. So, I've decided I'm going to NAK such patches and let the author have the hard work of working out why - hopefully learning something by doing so, and hopefully remembering for next time. :) -1 corresponds to -EPERM, which means "operation not permitted" (and, if this gets returned to userspace, the user program executing the syscall will most probably tell the user "operation not permitted" as the reason for failure - which is rather daft when you think about it. I have my mail client setup to highlight with a red background on a "\+.*return -1" regexp, which nicely lets me catch every one of those in any patch I skim through. This is about quality of implementation issue - and, tbh, if someone can't spend the time to check what happens to the return value, and then can't spend the time to look up a proper errno value, then what's the chance the rest of the patch (or patch set) has had sufficient care and attention paid to it? I'm getting to the point where if I see a "return -1" I just stop reviewing the rest of the patch set, especially if it's a large patch set.
diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h index b5e9cee..bb1b0cf 100644 --- a/arch/arm64/include/asm/cpu.h +++ b/arch/arm64/include/asm/cpu.h @@ -29,6 +29,7 @@ struct cpuinfo_arm64 { u32 reg_cntfrq; u32 reg_dczid; u32 reg_midr; + u32 reg_revidr; u64 reg_id_aa64dfr0; u64 reg_id_aa64dfr1; diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c index 52331ff..93e0488 100644 --- a/arch/arm64/kernel/cpuinfo.c +++ b/arch/arm64/kernel/cpuinfo.c @@ -199,6 +199,7 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info) info->reg_ctr = read_cpuid_cachetype(); info->reg_dczid = read_cpuid(DCZID_EL0); info->reg_midr = read_cpuid_id(); + info->reg_revidr = read_cpuid(REVIDR_EL1); info->reg_id_aa64dfr0 = read_cpuid(ID_AA64DFR0_EL1); info->reg_id_aa64dfr1 = read_cpuid(ID_AA64DFR1_EL1); @@ -247,3 +248,50 @@ void __init cpuinfo_store_boot_cpu(void) boot_cpu_data = *info; init_cpu_features(&boot_cpu_data); } + +#define CPUINFO_ATTR_RO(_name) \ + static ssize_t show_##_name (struct device *dev, \ + struct device_attribute *attr, char *buf) \ + { \ + struct cpuinfo_arm64 *info = &per_cpu(cpu_data, dev->id); \ + \ + if (info->reg_midr) \ + return sprintf(buf, "0x%016x\n", info->reg_##_name); \ + else \ + return 0; \ + } \ + static DEVICE_ATTR(_name, 0444, show_##_name, NULL) + +CPUINFO_ATTR_RO(midr); +CPUINFO_ATTR_RO(revidr); + +static struct attribute *cpuregs_attrs[] = { + &dev_attr_midr.attr, + &dev_attr_revidr.attr, + NULL +}; + +static struct attribute_group cpuregs_attr_group = { + .attrs = cpuregs_attrs, + .name = "identification" +}; + +static int __init cpuinfo_regs_init(void) +{ + int cpu, ret; + + for_each_present_cpu(cpu) { + struct device *dev = get_cpu_device(cpu); + + if (!dev) + return -1; + + ret = sysfs_create_group(&dev->kobj, &cpuregs_attr_group); + if (ret) + return ret; + } + + return 0; +} + +device_initcall(cpuinfo_regs_init);