| Message ID | 20170327131825.32134-1-juri.lelli@arm.com |
|---|---|
| Headers | show |
| Series | Fix issues and factorize arm/arm64 capacity information code | expand |
On 29/03/17 09:37, Vincent Guittot wrote: > On 27 March 2017 at 15:18, Juri Lelli <juri.lelli@arm.com> wrote: > > parse_cpu_capacity() has to return 0 on failure, but it currently returns > > 1 instead if raw_capacity kcalloc failed. > > > > Fix it by removing the negation of the return value. > > > > Cc: Russell King <linux@arm.linux.org.uk> > > Reported-by: Morten Rasmussen <morten.rasmussen@arm.com> > > Fixes: 06073ee26775 ('ARM: 8621/3: parse cpu capacity-dmips-mhz from DT') > > Signed-off-by: Juri Lelli <juri.lelli@arm.com> > > --- > > arch/arm/kernel/topology.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c > > index f8a3ab82e77f..4e4af809606a 100644 > > --- a/arch/arm/kernel/topology.c > > +++ b/arch/arm/kernel/topology.c > > @@ -166,7 +166,7 @@ static int __init parse_cpu_capacity(struct device_node *cpu_node, int cpu) > > if (!raw_capacity) { > > pr_err("cpu_capacity: failed to allocate memory for raw capacities\n"); > > cap_parsing_failed = true; > > - return !ret; > > + return ret; > > Why not directly returning 0 ? whatever the value of ret, the parse of > cpu capacity has failed in this case > Sure, can change that. Thanks, - Juri -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 27/03/17 14:18, Juri Lelli wrote: > Hi, > > arm and arm64 topology.c share a lot of code related to parsing of capacity > information. This is v3 of a solution [1] (based on Will's, Catalin's and > Mark's off-line suggestions) to move such common code in a single place: > drivers/base/arch_topology.c (by creating such file and conditionally compiling > it for arm and arm64 only). > > First 4 patches are actually fixes for the current code. > > Patch 5 is the actual refactoring. > > Patch 6 is a minor change suggested by Greg and can be squashed as needed. > > Patch 7 removes one of the extern symbols by changing a bit the now common > code. > > Patch 8 removes the remaining externs (as required by Russell during v1 review) > by creating a new header file include/linux/arch_topology.h and including that > from arm, arm64 and drivers. > > Last patch addresses Dietmar's comments to v1 and adds a 'atd_' prefix to > interfaces exported by drivers code and used by arch (and potentially others in > the future). > > Changes from v2: > > - rebase on top of 4.11-rc4 > - fix various problems pointed out by Greg, thanks for the review! > (see patch 5 for details) > Thanks Vincent for your comments. Everybody else, ping? Thanks, - Juri -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 27, 2017 at 02:18:21PM +0100, Juri Lelli wrote: > arm and arm64 share lot of code relative to parsing CPU capacity > information from DT, using that information for appropriate scaling and > exposing a sysfs interface for chaging such values at runtime. > > Factorize such code in a common place (driver/base/arch_topology.c) in > preparation for further additions. > > Suggested-by: Will Deacon <will.deacon@arm.com> > Suggested-by: Mark Rutland <mark.rutland@arm.com> > Suggested-by: Catalin Marinas <catalin.marinas@arm.com> I need some acks here from the arm maintainers before I can take this series... thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 27, 2017 at 02:18:22PM +0100, Juri Lelli wrote: > Printing out an error message when we failed to get the cpu device is > not helping anyone. Remove it. (1) the subject line talks about removing a "comment" but you're actually removing an error printk (2) I don't think it's "not helping anyone", although the description above doesn't say _why_ - it's reporting the lack of a missing CPU device that we expect to be present. If it's not present, then we're not going to end up with the cpu capacity attribute, and the error message answers the "why is that sysfs file missing" question. I think a better commit message is needed for this change. > > Signed-off-by: Juri Lelli <juri.lelli@arm.com> > --- > drivers/base/arch_topology.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > index c33482121b7d..b24d9a2af2c5 100644 > --- a/drivers/base/arch_topology.c > +++ b/drivers/base/arch_topology.c > @@ -81,11 +81,9 @@ static int register_cpu_capacity_sysctl(void) > > 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); > + if (!cpu) > continue; > - } > + > device_create_file(cpu, &dev_attr_cpu_capacity); > } > > -- > 2.10.0 > -- 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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 10/04/17 14:51, Russell King - ARM Linux wrote: > On Mon, Mar 27, 2017 at 02:18:22PM +0100, Juri Lelli wrote: > > Printing out an error message when we failed to get the cpu device is > > not helping anyone. Remove it. > > (1) the subject line talks about removing a "comment" but you're > actually removing an error printk > (2) I don't think it's "not helping anyone", although the description > above doesn't say _why_ - it's reporting the lack of a missing CPU > device that we expect to be present. If it's not present, then > we're not going to end up with the cpu capacity attribute, and the > error message answers the "why is that sysfs file missing" question. That's the same I was thinking when I put the error message there in the first place. But, then Greg didn't seem to like it. > > I think a better commit message is needed for this change. > We could just skip this patch entirely. Or, of course, I can easily update the commit message. Which way is to be preferred? Thanks, - Juri > > > > Signed-off-by: Juri Lelli <juri.lelli@arm.com> > > --- > > drivers/base/arch_topology.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > > index c33482121b7d..b24d9a2af2c5 100644 > > --- a/drivers/base/arch_topology.c > > +++ b/drivers/base/arch_topology.c > > @@ -81,11 +81,9 @@ static int register_cpu_capacity_sysctl(void) > > > > 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); > > + if (!cpu) > > continue; > > - } > > + > > device_create_file(cpu, &dev_attr_cpu_capacity); > > } > > > > -- > > 2.10.0 > > > > -- > 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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 10/04/17 09:18, Catalin Marinas wrote: > On Mon, Mar 27, 2017 at 02:18:21PM +0100, Juri Lelli wrote: > > arm and arm64 share lot of code relative to parsing CPU capacity > > information from DT, using that information for appropriate scaling and > > exposing a sysfs interface for chaging such values at runtime. > > > > Factorize such code in a common place (driver/base/arch_topology.c) in > > preparation for further additions. > > > > Suggested-by: Will Deacon <will.deacon@arm.com> > > Suggested-by: Mark Rutland <mark.rutland@arm.com> > > Suggested-by: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Russell King <linux@armlinux.org.uk> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Will Deacon <will.deacon@arm.com> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Signed-off-by: Juri Lelli <juri.lelli@arm.com> > > --- > > > > Changes from v2: > > - make capacity_scale and raw_capacity static > > - added SPDX header > > - improved indent > > - misc. whitespaces/newlines fixes > > > > Changes from v1: > > - keep the original GPLv2 header > > --- > > arch/arm/Kconfig | 1 + > > arch/arm/kernel/topology.c | 213 ++----------------------------------- > > arch/arm64/Kconfig | 1 + > > arch/arm64/kernel/topology.c | 219 +-------------------------------------- > > drivers/base/Kconfig | 8 ++ > > drivers/base/Makefile | 1 + > > drivers/base/arch_topology.c | 242 +++++++++++++++++++++++++++++++++++++++++++ > > For arm64: > > Acked-by: Catalin Marinas <catalin.marinas@arm.com> Thanks for reviewing the series. Best, - Juri -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 10, 2017 at 03:02:14PM +0100, Juri Lelli wrote: > Hi, > > On 10/04/17 14:51, Russell King - ARM Linux wrote: > > On Mon, Mar 27, 2017 at 02:18:22PM +0100, Juri Lelli wrote: > > > Printing out an error message when we failed to get the cpu device is > > > not helping anyone. Remove it. > > > > (1) the subject line talks about removing a "comment" but you're > > actually removing an error printk > > (2) I don't think it's "not helping anyone", although the description > > above doesn't say _why_ - it's reporting the lack of a missing CPU > > device that we expect to be present. If it's not present, then > > we're not going to end up with the cpu capacity attribute, and the > > error message answers the "why is that sysfs file missing" question. > > That's the same I was thinking when I put the error message there in the > first place. But, then Greg didn't seem to like it. I don't think it was a case of "not liking it" - Greg asked what use it was. Greg also pointed out the race with userspace. I think dropping this patch is the quickest way to move forward. -- 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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, arm and arm64 topology.c share a lot of code related to parsing of capacity information. This is v3 of a solution [1] (based on Will's, Catalin's and Mark's off-line suggestions) to move such common code in a single place: drivers/base/arch_topology.c (by creating such file and conditionally compiling it for arm and arm64 only). First 4 patches are actually fixes for the current code. Patch 5 is the actual refactoring. Patch 6 is a minor change suggested by Greg and can be squashed as needed. Patch 7 removes one of the extern symbols by changing a bit the now common code. Patch 8 removes the remaining externs (as required by Russell during v1 review) by creating a new header file include/linux/arch_topology.h and including that from arm, arm64 and drivers. Last patch addresses Dietmar's comments to v1 and adds a 'atd_' prefix to interfaces exported by drivers code and used by arch (and potentially others in the future). Changes from v2: - rebase on top of 4.11-rc4 - fix various problems pointed out by Greg, thanks for the review! (see patch 5 for details) The set is based on top of linux/master (4.11-rc4 c02ed2e75ef4) and it is also available from: git://linux-arm.org/linux-jl.git upstream/default_caps_factorize-v3 Best, - Juri [1] v1 - https://marc.info/?l=linux-kernel&m=148483680119355&w=2 v2 - https://marc.info/?l=linux-kernel&m=148663344018205&w=2 Juri Lelli (9): Documentation: arm: fix wrong reference number in DT definition Documentation/ABI: add information about cpu_capacity arm: fix return value of parse_cpu_capacity arm: remove wrong CONFIG_PROC_SYSCTL ifdef arm, arm64: factorize common cpu capacity default code drivers: remove useless comment from base/arch_topology.c arm,arm64,drivers: reduce scope of cap_parsing_failed arm,arm64,drivers: move externs in a new header file arm,arm64,drivers: add a prefix to drivers arch_topology interfaces Documentation/ABI/testing/sysfs-devices-system-cpu | 7 + Documentation/devicetree/bindings/arm/cpus.txt | 4 +- arch/arm/Kconfig | 1 + arch/arm/kernel/topology.c | 221 +------------------ arch/arm64/Kconfig | 1 + arch/arm64/kernel/topology.c | 226 +------------------ drivers/base/Kconfig | 8 + drivers/base/Makefile | 1 + drivers/base/arch_topology.c | 241 +++++++++++++++++++++ include/linux/arch_topology.h | 17 ++ 10 files changed, 288 insertions(+), 439 deletions(-) create mode 100644 drivers/base/arch_topology.c create mode 100644 include/linux/arch_topology.h -- 2.10.0 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html