* [PATCH v2 1/2] soc: qcom: llcc: Name regmaps to avoid collisions
2019-10-08 23:45 [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver Stephen Boyd
@ 2019-10-08 23:45 ` Stephen Boyd
2019-10-09 15:25 ` Evan Green
2019-10-08 23:45 ` [PATCH v2 2/2] soc: qcom: llcc: Move regmap config to local variable Stephen Boyd
2019-10-08 23:55 ` [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver Bjorn Andersson
2 siblings, 1 reply; 10+ messages in thread
From: Stephen Boyd @ 2019-10-08 23:45 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson
Cc: linux-kernel, linux-arm-msm, Venkata Narendra Kumar Gutta, Evan Green
We'll end up with debugfs collisions if we don't give names to the
regmaps created by this driver. Change the name of the config before
registering it so we don't collide in debugfs.
Fixes: 7f9c136216c7 ("soc: qcom: Add broadcast base for Last Level Cache Controller (LLCC)")
Cc: Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org>
Cc: Evan Green <evgreen@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
drivers/soc/qcom/llcc-slice.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/soc/qcom/llcc-slice.c b/drivers/soc/qcom/llcc-slice.c
index 9090ea12eaf3..4a6111635f82 100644
--- a/drivers/soc/qcom/llcc-slice.c
+++ b/drivers/soc/qcom/llcc-slice.c
@@ -48,7 +48,7 @@
static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
-static const struct regmap_config llcc_regmap_config = {
+static struct regmap_config llcc_regmap_config = {
.reg_bits = 32,
.reg_stride = 4,
.val_bits = 32,
@@ -323,6 +323,7 @@ static struct regmap *qcom_llcc_init_mmio(struct platform_device *pdev,
if (IS_ERR(base))
return ERR_CAST(base);
+ llcc_regmap_config.name = name;
return devm_regmap_init_mmio(&pdev->dev, base, &llcc_regmap_config);
}
--
Sent by a computer through tubes
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 1/2] soc: qcom: llcc: Name regmaps to avoid collisions
2019-10-08 23:45 ` [PATCH v2 1/2] soc: qcom: llcc: Name regmaps to avoid collisions Stephen Boyd
@ 2019-10-09 15:25 ` Evan Green
0 siblings, 0 replies; 10+ messages in thread
From: Evan Green @ 2019-10-09 15:25 UTC (permalink / raw)
To: Stephen Boyd
Cc: Andy Gross, Bjorn Andersson, LKML, linux-arm-msm,
Venkata Narendra Kumar Gutta
On Tue, Oct 8, 2019 at 4:45 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> We'll end up with debugfs collisions if we don't give names to the
> regmaps created by this driver. Change the name of the config before
> registering it so we don't collide in debugfs.
>
> Fixes: 7f9c136216c7 ("soc: qcom: Add broadcast base for Last Level Cache Controller (LLCC)")
> Cc: Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org>
> Cc: Evan Green <evgreen@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Evan Green <evgreen@chromium.org>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] soc: qcom: llcc: Move regmap config to local variable
2019-10-08 23:45 [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver Stephen Boyd
2019-10-08 23:45 ` [PATCH v2 1/2] soc: qcom: llcc: Name regmaps to avoid collisions Stephen Boyd
@ 2019-10-08 23:45 ` Stephen Boyd
2019-10-08 23:55 ` [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver Bjorn Andersson
2 siblings, 0 replies; 10+ messages in thread
From: Stephen Boyd @ 2019-10-08 23:45 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson
Cc: linux-kernel, linux-arm-msm, Venkata Narendra Kumar Gutta, Evan Green
This is now a global variable that we're modifying to fix the name.
That isn't terribly thread safe and it's not necessary to be a global so
let's just move this to a local variable instead. This saves space in
the symtab and actually reduces kernel image size because the regmap
config is large and we can replace the initialization of that structure
with a memset and a few member assignments.
Cc: Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org>
Cc: Evan Green <evgreen@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
drivers/soc/qcom/llcc-slice.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/soc/qcom/llcc-slice.c b/drivers/soc/qcom/llcc-slice.c
index 4a6111635f82..50aea3f0be41 100644
--- a/drivers/soc/qcom/llcc-slice.c
+++ b/drivers/soc/qcom/llcc-slice.c
@@ -48,13 +48,6 @@
static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
-static struct regmap_config llcc_regmap_config = {
- .reg_bits = 32,
- .reg_stride = 4,
- .val_bits = 32,
- .fast_io = true,
-};
-
/**
* llcc_slice_getd - get llcc slice descriptor
* @uid: usecase_id for the client
@@ -314,6 +307,12 @@ static struct regmap *qcom_llcc_init_mmio(struct platform_device *pdev,
{
struct resource *res;
void __iomem *base;
+ struct regmap_config llcc_regmap_config = {
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .fast_io = true,
+ };
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
if (!res)
--
Sent by a computer through tubes
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver
2019-10-08 23:45 [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver Stephen Boyd
2019-10-08 23:45 ` [PATCH v2 1/2] soc: qcom: llcc: Name regmaps to avoid collisions Stephen Boyd
2019-10-08 23:45 ` [PATCH v2 2/2] soc: qcom: llcc: Move regmap config to local variable Stephen Boyd
@ 2019-10-08 23:55 ` Bjorn Andersson
2019-10-09 1:58 ` Stephen Boyd
2 siblings, 1 reply; 10+ messages in thread
From: Bjorn Andersson @ 2019-10-08 23:55 UTC (permalink / raw)
To: Stephen Boyd
Cc: Andy Gross, linux-kernel, linux-arm-msm,
Venkata Narendra Kumar Gutta, Evan Green
On Tue 08 Oct 16:45 PDT 2019, Stephen Boyd wrote:
> Now a two part series. These patches fix a debugfs name collision for
> the llcc regmaps and moves the config to a local variable to save on
> image size.
>
> Changes from v1 (https://lkml.kernel.org/r/20191004233132.194336-1-swboyd@chromium.org):
> * New second patch
> * Dropped static
> * See range-diff below!
>
> Stephen Boyd (2):
> soc: qcom: llcc: Name regmaps to avoid collisions
> soc: qcom: llcc: Move regmap config to local variable
>
> drivers/soc/qcom/llcc-slice.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> Cc: Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org>
> Cc: Evan Green <evgreen@chromium.org>
>
> Range-diff against v1:
> -: ------------ > 1: 07bc0e8bdb6e soc: qcom: llcc: Name regmaps to avoid collisions
> 1: 0c54fc8a7ed6 ! 2: 5c4446e36783 soc: qcom: llcc: Name regmaps to avoid collisions
> @@ Metadata
> Author: Stephen Boyd <swboyd@chromium.org>
>
> ## Commit message ##
> - soc: qcom: llcc: Name regmaps to avoid collisions
> + soc: qcom: llcc: Move regmap config to local variable
>
> - We'll end up with debugfs collisions if we don't give names to the
> - regmaps created inside this driver. Copy the template config over into
> - this function and give the regmap the same name as the resource name.
> + This is now a global variable that we're modifying to fix the name.
> + That isn't terribly thread safe and it's not necessary to be a global so
> + let's just move this to a local variable instead. This saves space in
> + the symtab and actually reduces kernel image size because the regmap
> + config is large and we can replace the initialization of that structure
> + with a memset and a few member assignments.
>
> - Fixes: 7f9c136216c7 ("soc: qcom: Add broadcast base for Last Level Cache Controller (LLCC)")
> - Cc: Venkata Narendra Kumar Gutta <vnkgutta@codeaurora.org>
> - Cc: Evan Green <evgreen@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>
> ## drivers/soc/qcom/llcc-slice.c ##
> @@ drivers/soc/qcom/llcc-slice.c
>
> static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
>
> --static const struct regmap_config llcc_regmap_config = {
> +-static struct regmap_config llcc_regmap_config = {
> - .reg_bits = 32,
> - .reg_stride = 4,
> - .val_bits = 32,
> @@ drivers/soc/qcom/llcc-slice.c: static struct regmap *qcom_llcc_init_mmio(struct
> {
> struct resource *res;
> void __iomem *base;
> -+ static struct regmap_config llcc_regmap_config = {
> ++ struct regmap_config llcc_regmap_config = {
Now that this isn't static I like the end result better. Not sure about
the need for splitting it in two patches, but if Evan is happy I'll take
it.
Regards,
Bjorn
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> @@ drivers/soc/qcom/llcc-slice.c: static struct regmap *qcom_llcc_init_mmio(struct
>
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> if (!res)
> -@@ drivers/soc/qcom/llcc-slice.c: static struct regmap *qcom_llcc_init_mmio(struct platform_device *pdev,
> - if (IS_ERR(base))
> - return ERR_CAST(base);
> -
> -+ llcc_regmap_config.name = name;
> - return devm_regmap_init_mmio(&pdev->dev, base, &llcc_regmap_config);
> - }
> -
>
> base-commit: 8b0eed9f6e36a5488967b0acc51444d658dd711b
> --
> Sent by a computer through tubes
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver
2019-10-08 23:55 ` [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver Bjorn Andersson
@ 2019-10-09 1:58 ` Stephen Boyd
2019-10-09 16:01 ` Evan Green
0 siblings, 1 reply; 10+ messages in thread
From: Stephen Boyd @ 2019-10-09 1:58 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Andy Gross, linux-kernel, linux-arm-msm,
Venkata Narendra Kumar Gutta, Evan Green
Quoting Bjorn Andersson (2019-10-08 16:55:04)
> On Tue 08 Oct 16:45 PDT 2019, Stephen Boyd wrote:
> > @@ drivers/soc/qcom/llcc-slice.c
> >
> > static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
> >
> > --static const struct regmap_config llcc_regmap_config = {
> > +-static struct regmap_config llcc_regmap_config = {
> > - .reg_bits = 32,
> > - .reg_stride = 4,
> > - .val_bits = 32,
> > @@ drivers/soc/qcom/llcc-slice.c: static struct regmap *qcom_llcc_init_mmio(struct
> > {
> > struct resource *res;
> > void __iomem *base;
> > -+ static struct regmap_config llcc_regmap_config = {
> > ++ struct regmap_config llcc_regmap_config = {
>
> Now that this isn't static I like the end result better. Not sure about
> the need for splitting it in two patches, but if Evan is happy I'll take
> it.
>
Well I split it into bug fix and micro-optimization so backport choices
can be made. But yeah, I hope Evan is happy enough to provide a
reviewed-by tag!
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver
2019-10-09 1:58 ` Stephen Boyd
@ 2019-10-09 16:01 ` Evan Green
2019-10-09 17:46 ` Bjorn Andersson
0 siblings, 1 reply; 10+ messages in thread
From: Evan Green @ 2019-10-09 16:01 UTC (permalink / raw)
To: Stephen Boyd
Cc: Bjorn Andersson, Andy Gross, LKML, linux-arm-msm,
Venkata Narendra Kumar Gutta
On Tue, Oct 8, 2019 at 6:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Bjorn Andersson (2019-10-08 16:55:04)
> > On Tue 08 Oct 16:45 PDT 2019, Stephen Boyd wrote:
> > > @@ drivers/soc/qcom/llcc-slice.c
> > >
> > > static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
> > >
> > > --static const struct regmap_config llcc_regmap_config = {
> > > +-static struct regmap_config llcc_regmap_config = {
> > > - .reg_bits = 32,
> > > - .reg_stride = 4,
> > > - .val_bits = 32,
> > > @@ drivers/soc/qcom/llcc-slice.c: static struct regmap *qcom_llcc_init_mmio(struct
> > > {
> > > struct resource *res;
> > > void __iomem *base;
> > > -+ static struct regmap_config llcc_regmap_config = {
> > > ++ struct regmap_config llcc_regmap_config = {
> >
> > Now that this isn't static I like the end result better. Not sure about
> > the need for splitting it in two patches, but if Evan is happy I'll take
> > it.
> >
>
> Well I split it into bug fix and micro-optimization so backport choices
> can be made. But yeah, I hope Evan is happy enough to provide a
> reviewed-by tag!
It's definitely better without the static local since it no longer has
the cognitive trap, but I still don't really get why we're messing
with the global v. local aspect of it. We're now inconsistent with
every other caller of this function, and for what exactly? We've
traded some data space for a call to memset() and some instructions. I
would have thought anecdotally that memory was the cheaper thing (ie
cpu speeds stopped increasing awhile ago, but memory is still getting
cheaper).
But either way it's correct, so really it's fine if you ignore me :)
-Evan
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver
2019-10-09 16:01 ` Evan Green
@ 2019-10-09 17:46 ` Bjorn Andersson
2019-10-09 17:59 ` Evan Green
0 siblings, 1 reply; 10+ messages in thread
From: Bjorn Andersson @ 2019-10-09 17:46 UTC (permalink / raw)
To: Evan Green
Cc: Stephen Boyd, Andy Gross, LKML, linux-arm-msm,
Venkata Narendra Kumar Gutta
On Wed 09 Oct 09:01 PDT 2019, Evan Green wrote:
> On Tue, Oct 8, 2019 at 6:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Bjorn Andersson (2019-10-08 16:55:04)
> > > On Tue 08 Oct 16:45 PDT 2019, Stephen Boyd wrote:
> > > > @@ drivers/soc/qcom/llcc-slice.c
> > > >
> > > > static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
> > > >
> > > > --static const struct regmap_config llcc_regmap_config = {
> > > > +-static struct regmap_config llcc_regmap_config = {
> > > > - .reg_bits = 32,
> > > > - .reg_stride = 4,
> > > > - .val_bits = 32,
> > > > @@ drivers/soc/qcom/llcc-slice.c: static struct regmap *qcom_llcc_init_mmio(struct
> > > > {
> > > > struct resource *res;
> > > > void __iomem *base;
> > > > -+ static struct regmap_config llcc_regmap_config = {
> > > > ++ struct regmap_config llcc_regmap_config = {
> > >
> > > Now that this isn't static I like the end result better. Not sure about
> > > the need for splitting it in two patches, but if Evan is happy I'll take
> > > it.
> > >
> >
> > Well I split it into bug fix and micro-optimization so backport choices
> > can be made. But yeah, I hope Evan is happy enough to provide a
> > reviewed-by tag!
>
> It's definitely better without the static local since it no longer has
> the cognitive trap, but I still don't really get why we're messing
> with the global v. local aspect of it. We're now inconsistent with
> every other caller of this function, and for what exactly? We've
> traded some data space for a call to memset() and some instructions. I
> would have thought anecdotally that memory was the cheaper thing (ie
> cpu speeds stopped increasing awhile ago, but memory is still getting
> cheaper).
>
The reason for making the structure local is because it's being modified
per instance, meaning it would still work as long as
qcom_llcc_init_mmio() is never called concurrently for two llcc
instances. But the correctness outweighs the performance degradation of
setting it up on the stack in my view.
Or am I missing something?
Regards,
Bjorn
> But either way it's correct, so really it's fine if you ignore me :)
> -Evan
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver
2019-10-09 17:46 ` Bjorn Andersson
@ 2019-10-09 17:59 ` Evan Green
2019-10-10 3:57 ` Bjorn Andersson
0 siblings, 1 reply; 10+ messages in thread
From: Evan Green @ 2019-10-09 17:59 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Stephen Boyd, Andy Gross, LKML, linux-arm-msm,
Venkata Narendra Kumar Gutta
On Wed, Oct 9, 2019 at 10:46 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Wed 09 Oct 09:01 PDT 2019, Evan Green wrote:
>
> > On Tue, Oct 8, 2019 at 6:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Bjorn Andersson (2019-10-08 16:55:04)
> > > > On Tue 08 Oct 16:45 PDT 2019, Stephen Boyd wrote:
> > > > > @@ drivers/soc/qcom/llcc-slice.c
> > > > >
> > > > > static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
> > > > >
> > > > > --static const struct regmap_config llcc_regmap_config = {
> > > > > +-static struct regmap_config llcc_regmap_config = {
> > > > > - .reg_bits = 32,
> > > > > - .reg_stride = 4,
> > > > > - .val_bits = 32,
> > > > > @@ drivers/soc/qcom/llcc-slice.c: static struct regmap *qcom_llcc_init_mmio(struct
> > > > > {
> > > > > struct resource *res;
> > > > > void __iomem *base;
> > > > > -+ static struct regmap_config llcc_regmap_config = {
> > > > > ++ struct regmap_config llcc_regmap_config = {
> > > >
> > > > Now that this isn't static I like the end result better. Not sure about
> > > > the need for splitting it in two patches, but if Evan is happy I'll take
> > > > it.
> > > >
> > >
> > > Well I split it into bug fix and micro-optimization so backport choices
> > > can be made. But yeah, I hope Evan is happy enough to provide a
> > > reviewed-by tag!
> >
> > It's definitely better without the static local since it no longer has
> > the cognitive trap, but I still don't really get why we're messing
> > with the global v. local aspect of it. We're now inconsistent with
> > every other caller of this function, and for what exactly? We've
> > traded some data space for a call to memset() and some instructions. I
> > would have thought anecdotally that memory was the cheaper thing (ie
> > cpu speeds stopped increasing awhile ago, but memory is still getting
> > cheaper).
> >
>
> The reason for making the structure local is because it's being modified
> per instance, meaning it would still work as long as
> qcom_llcc_init_mmio() is never called concurrently for two llcc
> instances. But the correctness outweighs the performance degradation of
> setting it up on the stack in my view.
>
I hadn't considered the concurrency aspect of the change, since I had
anchored myself on the static local. I'm convinced. Might be worth
mentioning that in the commit message.
For the series:
Reviewed-by: Evan Green <evgreen@chromium.org>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver
2019-10-09 17:59 ` Evan Green
@ 2019-10-10 3:57 ` Bjorn Andersson
0 siblings, 0 replies; 10+ messages in thread
From: Bjorn Andersson @ 2019-10-10 3:57 UTC (permalink / raw)
To: Evan Green
Cc: Stephen Boyd, Andy Gross, LKML, linux-arm-msm,
Venkata Narendra Kumar Gutta
On Wed 09 Oct 10:59 PDT 2019, Evan Green wrote:
> On Wed, Oct 9, 2019 at 10:46 AM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Wed 09 Oct 09:01 PDT 2019, Evan Green wrote:
> >
> > > On Tue, Oct 8, 2019 at 6:58 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Quoting Bjorn Andersson (2019-10-08 16:55:04)
> > > > > On Tue 08 Oct 16:45 PDT 2019, Stephen Boyd wrote:
> > > > > > @@ drivers/soc/qcom/llcc-slice.c
> > > > > >
> > > > > > static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
> > > > > >
> > > > > > --static const struct regmap_config llcc_regmap_config = {
> > > > > > +-static struct regmap_config llcc_regmap_config = {
> > > > > > - .reg_bits = 32,
> > > > > > - .reg_stride = 4,
> > > > > > - .val_bits = 32,
> > > > > > @@ drivers/soc/qcom/llcc-slice.c: static struct regmap *qcom_llcc_init_mmio(struct
> > > > > > {
> > > > > > struct resource *res;
> > > > > > void __iomem *base;
> > > > > > -+ static struct regmap_config llcc_regmap_config = {
> > > > > > ++ struct regmap_config llcc_regmap_config = {
> > > > >
> > > > > Now that this isn't static I like the end result better. Not sure about
> > > > > the need for splitting it in two patches, but if Evan is happy I'll take
> > > > > it.
> > > > >
> > > >
> > > > Well I split it into bug fix and micro-optimization so backport choices
> > > > can be made. But yeah, I hope Evan is happy enough to provide a
> > > > reviewed-by tag!
> > >
> > > It's definitely better without the static local since it no longer has
> > > the cognitive trap, but I still don't really get why we're messing
> > > with the global v. local aspect of it. We're now inconsistent with
> > > every other caller of this function, and for what exactly? We've
> > > traded some data space for a call to memset() and some instructions. I
> > > would have thought anecdotally that memory was the cheaper thing (ie
> > > cpu speeds stopped increasing awhile ago, but memory is still getting
> > > cheaper).
> > >
> >
> > The reason for making the structure local is because it's being modified
> > per instance, meaning it would still work as long as
> > qcom_llcc_init_mmio() is never called concurrently for two llcc
> > instances. But the correctness outweighs the performance degradation of
> > setting it up on the stack in my view.
> >
>
> I hadn't considered the concurrency aspect of the change, since I had
> anchored myself on the static local. I'm convinced. Might be worth
> mentioning that in the commit message.
>
> For the series:
> Reviewed-by: Evan Green <evgreen@chromium.org>
Thank you, patches applied.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 10+ messages in thread