Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver
On Wed 09 Oct 10:59 PDT 2019, Evan Green wrote:
> On Wed, Oct 9, 2019 at 10:46 AM Bjorn Andersson
> wrote:
> >
> > On Wed 09 Oct 09:01 PDT 2019, Evan Green wrote:
> >
> > > On Tue, Oct 8, 2019 at 6:58 PM Stephen Boyd 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
Thank you, patches applied.
Regards,
Bjorn
Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver
On Wed, Oct 9, 2019 at 10:46 AM Bjorn Andersson
wrote:
>
> On Wed 09 Oct 09:01 PDT 2019, Evan Green wrote:
>
> > On Tue, Oct 8, 2019 at 6:58 PM Stephen Boyd 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
Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver
On Wed 09 Oct 09:01 PDT 2019, Evan Green wrote:
> On Tue, Oct 8, 2019 at 6:58 PM Stephen Boyd 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
On Tue, Oct 8, 2019 at 6:58 PM Stephen Boyd 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
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 0/2] Avoid regmap debugfs collisions in qcom llcc driver
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-swb...@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
> Cc: Evan Green
>
> 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
>
> ## 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
> -Cc: Evan Green
> Signed-off-by: Stephen Boyd
>
> ## 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(>dev, base,
> _regmap_config);
> - }
> -
>
> base-commit: 8b0eed9f6e36a5488967b0acc51444d658dd711b
> --
> Sent by a computer through tubes
>
[PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver
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-swb...@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
Cc: Evan Green
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
## 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
-Cc: Evan Green
Signed-off-by: Stephen Boyd
## 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(>dev, base, _regmap_config);
- }
-
base-commit: 8b0eed9f6e36a5488967b0acc51444d658dd711b
--
Sent by a computer through tubes
