| Message ID | 20191008234505.222991-1-swboyd@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Avoid regmap debugfs collisions in qcom llcc driver | expand |
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 >
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 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
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
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>
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
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