From: Russell King - ARM Linux <linux@armlinux.org.uk> To: Juri Lelli <juri.lelli@arm.com> Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, peterz@infradead.org, vincent.guittot@linaro.org, robh+dt@kernel.org, mark.rutland@arm.com, sudeep.holla@arm.com, lorenzo.pieralisi@arm.com, catalin.marinas@arm.com, will.deacon@arm.com, morten.rasmussen@arm.com, dietmar.eggemann@arm.com, broonie@kernel.org Subject: Re: [PATCH v7 REPOST 8/9] arm: add sysfs cpu_capacity attribute Date: Sun, 30 Oct 2016 20:45:51 +0000 [thread overview] Message-ID: <20161030204551.GM1041@n2100.armlinux.org.uk> (raw) In-Reply-To: <20161017154650.18779-9-juri.lelli@arm.com> On Mon, Oct 17, 2016 at 04:46:49PM +0100, Juri Lelli wrote: > +#ifdef CONFIG_PROC_SYSCTL > +#include <asm/cpu.h> > +#include <linux/string.h> Include files at the top of the file please. No need to ifdef them. They're sorted alphabetically, so new additions should be alphabetical. (That's a general rule - if something is already alphabetical, do not make it non-alphabetical.) > +static ssize_t show_cpu_capacity(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct cpu *cpu = container_of(dev, struct cpu, dev); > + ssize_t rc; > + int cpunum = cpu->dev.id; > + unsigned long capacity = arch_scale_cpu_capacity(NULL, cpunum); > + > + rc = sprintf(buf, "%lu\n", capacity); > + > + return rc; Way too many lines for such a simple function. This can be simplified to just: struct cpu *cpu = container_of(dev, struct cpu, dev); return sprintf(buf, "%lu\n", arch_scale_cpu_capacity(NULL, cpu->dev.id); If you don't like the last line ending on column 79, then feel free to break it across two lines after the format string. > +} > + > +static ssize_t store_cpu_capacity(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t count) > +{ > + struct cpu *cpu = container_of(dev, struct cpu, dev); > + int this_cpu = cpu->dev.id, i; > + unsigned long new_capacity; > + ssize_t ret; > + > + if (count) { > + char *p = (char *) buf; > + > + ret = kstrtoul(p, 0, &new_capacity); Unnecessary cast - kstrtoul takes a const char pointer, and in any case it's really bad form to cast away the "const-ness" of any pointer. So, just: if (count) { ret = kstrtoul(buf, 0, &new_capacity); should work just fine. > + if (ret) > + return ret; > + if (new_capacity > SCHED_CAPACITY_SCALE) > + return -EINVAL; > + > + mutex_lock(&cpu_scale_mutex); > + for_each_cpu(i, &cpu_topology[this_cpu].core_sibling) > + set_capacity_scale(i, new_capacity); > + mutex_unlock(&cpu_scale_mutex); > + } > + > + return count; > +} > + > +static DEVICE_ATTR(cpu_capacity, > + 0644, > + show_cpu_capacity, > + store_cpu_capacity); There's a move to use the named DEVICE_ATTR_RW() for this kind of thing, it'll want the functions named xxx_show() and xxx_store(). I see there's some recent patches to do this conversion across the kernel, so this should probably be done before submission. > + > +static int register_cpu_capacity_sysctl(void) > +{ > + int i; > + struct device *cpu; > + > + for_each_possible_cpu(i) { > + cpu = get_cpu_device(i); > + if (!cpu) { > + pr_err("%s: too early to get CPU%d device!\n", > + __func__, i); > + continue; > + } > + device_create_file(cpu, &dev_attr_cpu_capacity); > + } > + > + return 0; > +} > +late_initcall(register_cpu_capacity_sysctl); Hmm, this is really weird. topology_init() in arch/arm/kernel/setup.c is where these devices get created, and they're created at subsys_initcall() time. By that point, the list of possible CPUs has to be static, it's not going to change. I don't see why this has to be done at late_initcall() - and since topology.c will be linked after setup.c, I don't see why it shouldn't be at subsys_initcall() level to follow on after topology_init(). -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
WARNING: multiple messages have this Message-ID (diff)
From: linux@armlinux.org.uk (Russell King - ARM Linux) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v7 REPOST 8/9] arm: add sysfs cpu_capacity attribute Date: Sun, 30 Oct 2016 20:45:51 +0000 [thread overview] Message-ID: <20161030204551.GM1041@n2100.armlinux.org.uk> (raw) In-Reply-To: <20161017154650.18779-9-juri.lelli@arm.com> On Mon, Oct 17, 2016 at 04:46:49PM +0100, Juri Lelli wrote: > +#ifdef CONFIG_PROC_SYSCTL > +#include <asm/cpu.h> > +#include <linux/string.h> Include files at the top of the file please. No need to ifdef them. They're sorted alphabetically, so new additions should be alphabetical. (That's a general rule - if something is already alphabetical, do not make it non-alphabetical.) > +static ssize_t show_cpu_capacity(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct cpu *cpu = container_of(dev, struct cpu, dev); > + ssize_t rc; > + int cpunum = cpu->dev.id; > + unsigned long capacity = arch_scale_cpu_capacity(NULL, cpunum); > + > + rc = sprintf(buf, "%lu\n", capacity); > + > + return rc; Way too many lines for such a simple function. This can be simplified to just: struct cpu *cpu = container_of(dev, struct cpu, dev); return sprintf(buf, "%lu\n", arch_scale_cpu_capacity(NULL, cpu->dev.id); If you don't like the last line ending on column 79, then feel free to break it across two lines after the format string. > +} > + > +static ssize_t store_cpu_capacity(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t count) > +{ > + struct cpu *cpu = container_of(dev, struct cpu, dev); > + int this_cpu = cpu->dev.id, i; > + unsigned long new_capacity; > + ssize_t ret; > + > + if (count) { > + char *p = (char *) buf; > + > + ret = kstrtoul(p, 0, &new_capacity); Unnecessary cast - kstrtoul takes a const char pointer, and in any case it's really bad form to cast away the "const-ness" of any pointer. So, just: if (count) { ret = kstrtoul(buf, 0, &new_capacity); should work just fine. > + if (ret) > + return ret; > + if (new_capacity > SCHED_CAPACITY_SCALE) > + return -EINVAL; > + > + mutex_lock(&cpu_scale_mutex); > + for_each_cpu(i, &cpu_topology[this_cpu].core_sibling) > + set_capacity_scale(i, new_capacity); > + mutex_unlock(&cpu_scale_mutex); > + } > + > + return count; > +} > + > +static DEVICE_ATTR(cpu_capacity, > + 0644, > + show_cpu_capacity, > + store_cpu_capacity); There's a move to use the named DEVICE_ATTR_RW() for this kind of thing, it'll want the functions named xxx_show() and xxx_store(). I see there's some recent patches to do this conversion across the kernel, so this should probably be done before submission. > + > +static int register_cpu_capacity_sysctl(void) > +{ > + int i; > + struct device *cpu; > + > + for_each_possible_cpu(i) { > + cpu = get_cpu_device(i); > + if (!cpu) { > + pr_err("%s: too early to get CPU%d device!\n", > + __func__, i); > + continue; > + } > + device_create_file(cpu, &dev_attr_cpu_capacity); > + } > + > + return 0; > +} > +late_initcall(register_cpu_capacity_sysctl); Hmm, this is really weird. topology_init() in arch/arm/kernel/setup.c is where these devices get created, and they're created at subsys_initcall() time. By that point, the list of possible CPUs has to be static, it's not going to change. I don't see why this has to be done at late_initcall() - and since topology.c will be linked after setup.c, I don't see why it shouldn't be at subsys_initcall() level to follow on after topology_init(). -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
next prev parent reply other threads:[~2016-10-30 20:46 UTC|newest] Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-10-17 15:46 [PATCH v7 REPOST 0/9] CPUs capacity information for heterogeneous systems Juri Lelli 2016-10-17 15:46 ` Juri Lelli 2016-10-17 15:46 ` [PATCH v7 REPOST 1/9] Documentation: arm: define DT cpu capacity-dmips-mhz bindings Juri Lelli 2016-10-17 15:46 ` Juri Lelli 2016-10-17 15:46 ` Juri Lelli 2016-10-17 15:46 ` [PATCH v7 REPOST 2/9] arm: parse cpu capacity-dmips-mhz from DT Juri Lelli 2016-10-17 15:46 ` Juri Lelli 2016-10-17 15:46 ` [PATCH v7 REPOST 3/9] arm, dts: add TC2 cpu capacity-dmips-mhz information Juri Lelli 2016-10-17 15:46 ` Juri Lelli 2016-10-17 15:46 ` Juri Lelli 2016-10-17 15:46 ` [PATCH v7 REPOST 4/9] arm64: parse cpu capacity-dmips-mhz from DT Juri Lelli 2016-10-17 15:46 ` Juri Lelli 2016-10-17 15:46 ` [PATCH v7 REPOST 5/9] arm64, dts: add Juno cpu capacity-dmips-mhz information Juri Lelli 2016-10-17 15:46 ` Juri Lelli 2016-10-17 15:46 ` [PATCH v7 REPOST 6/9] arm64, dts: add Juno r1 " Juri Lelli 2016-10-17 15:46 ` Juri Lelli 2016-10-17 15:46 ` [PATCH v7 REPOST 7/9] arm64, dts: add Juno r2 " Juri Lelli 2016-10-17 15:46 ` Juri Lelli 2016-10-17 15:46 ` [PATCH v7 REPOST 8/9] arm: add sysfs cpu_capacity attribute Juri Lelli 2016-10-17 15:46 ` Juri Lelli 2016-10-30 20:45 ` Russell King - ARM Linux [this message] 2016-10-30 20:45 ` Russell King - ARM Linux 2016-11-03 5:29 ` Juri Lelli 2016-11-03 5:29 ` Juri Lelli 2016-11-03 5:29 ` Juri Lelli 2016-11-18 8:27 ` Juri Lelli 2016-11-18 8:27 ` Juri Lelli 2016-11-18 8:27 ` Juri Lelli 2016-10-17 15:46 ` [PATCH v7 REPOST 9/9] arm64: " Juri Lelli 2016-10-17 15:46 ` Juri Lelli 2016-10-17 15:46 ` Juri Lelli 2016-11-03 5:40 ` Juri Lelli 2016-11-03 5:40 ` Juri Lelli 2016-10-17 16:39 ` [PATCH v7 REPOST 0/9] CPUs capacity information for heterogeneous systems Sudeep Holla 2016-10-17 16:39 ` Sudeep Holla 2016-10-17 16:46 ` Juri Lelli 2016-10-17 16:46 ` Juri Lelli 2016-10-17 16:46 ` Juri Lelli 2016-10-30 14:22 ` Catalin Marinas 2016-10-30 14:22 ` Catalin Marinas 2016-10-30 14:22 ` Catalin Marinas 2016-11-03 5:37 ` Juri Lelli 2016-11-03 5:37 ` Juri Lelli 2016-11-03 5:37 ` Juri Lelli
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20161030204551.GM1041@n2100.armlinux.org.uk \ --to=linux@armlinux.org.uk \ --cc=broonie@kernel.org \ --cc=catalin.marinas@arm.com \ --cc=devicetree@vger.kernel.org \ --cc=dietmar.eggemann@arm.com \ --cc=juri.lelli@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=mark.rutland@arm.com \ --cc=morten.rasmussen@arm.com \ --cc=peterz@infradead.org \ --cc=robh+dt@kernel.org \ --cc=sudeep.holla@arm.com \ --cc=vincent.guittot@linaro.org \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.