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/[email protected]):
* 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 <[email protected]>
Cc: Evan Green <[email protected]>
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 <[email protected]>
## 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 <[email protected]>
- Cc: Evan Green <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
## 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
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 <[email protected]>
Cc: Evan Green <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
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
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/[email protected]):
> * 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 <[email protected]>
> Cc: Evan Green <[email protected]>
>
> 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 <[email protected]>
>
> ## 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 <[email protected]>
> - Cc: Evan Green <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
>
> ## 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
>
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!
On Tue, Oct 8, 2019 at 4:45 PM Stephen Boyd <[email protected]> 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 <[email protected]>
> Cc: Evan Green <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
Reviewed-by: Evan Green <[email protected]>
On Tue, Oct 8, 2019 at 6:58 PM Stephen Boyd <[email protected]> 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
On Wed 09 Oct 09:01 PDT 2019, Evan Green wrote:
> On Tue, Oct 8, 2019 at 6:58 PM Stephen Boyd <[email protected]> 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
On Wed, Oct 9, 2019 at 10:46 AM Bjorn Andersson
<[email protected]> wrote:
>
> On Wed 09 Oct 09:01 PDT 2019, Evan Green wrote:
>
> > On Tue, Oct 8, 2019 at 6:58 PM Stephen Boyd <[email protected]> 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 <[email protected]>
On Wed 09 Oct 10:59 PDT 2019, Evan Green wrote:
> On Wed, Oct 9, 2019 at 10:46 AM Bjorn Andersson
> <[email protected]> wrote:
> >
> > On Wed 09 Oct 09:01 PDT 2019, Evan Green wrote:
> >
> > > On Tue, Oct 8, 2019 at 6:58 PM Stephen Boyd <[email protected]> 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 <[email protected]>
Thank you, patches applied.
Regards,
Bjorn