[PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver

[PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver

[Stephen Boyd]

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 = {
     +		.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

[PATCH v2 1/2] soc: qcom: llcc: Name regmaps to avoid collisions

[Stephen Boyd]

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

[PATCH v2 2/2] soc: qcom: llcc: Move regmap config to local variable

[Stephen Boyd]

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

Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver

[Bjorn Andersson]

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
> 

Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver

[Stephen Boyd]

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!

Re: [PATCH v2 1/2] soc: qcom: llcc: Name regmaps to avoid collisions

[Evan Green]

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>

Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver

[Evan Green]

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

Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver

[Bjorn Andersson]

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

Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver

[Evan Green]

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>

Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver

[Bjorn Andersson]

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