linux-kernel - Re: [PATCH 2/4] ASoC: codecs: lpass-rx-macro: Keep static regmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Fri, 28 Jun 2024 17:17:20 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>, Banajit Goswami <bgoswami@...cinc.com>, 
	Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, 
	Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org, 
	linux-arm-msm@...r.kernel.org, linux-sound@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] ASoC: codecs: lpass-rx-macro: Keep static
 regmap_config as const

On Fri, 28 Jun 2024 at 11:39, Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org> wrote:
>
> On 28/06/2024 10:34, Dmitry Baryshkov wrote:
> > On Thu, Jun 27, 2024 at 05:23:44PM GMT, Krzysztof Kozlowski wrote:
> >> The driver has static 'struct regmap_config', which is then customized
> >> depending on device version.  This works fine, because there should not
> >> be two devices in a system simultaneously and even less likely that such
> >> two devices would have different versions, thus different regmap config.
> >> However code is cleaner and more obvious when static data in the driver
> >> is also const - it serves as a template.
> >>
> >> Mark the 'struct regmap_config' as const and duplicate it in the probe()
> >> with devm_kmemdup to allow customizing per detected device variant.
> >>
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> >> ---
> >>  sound/soc/codecs/lpass-rx-macro.c | 17 +++++++++++++----
> >>  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> >
> >>
> >> diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c
> >> index 59fe76b13cdb..3d8149665439 100644
> >> --- a/sound/soc/codecs/lpass-rx-macro.c
> >> +++ b/sound/soc/codecs/lpass-rx-macro.c
> >> @@ -1662,7 +1662,7 @@ static bool rx_is_readable_register(struct device *dev, unsigned int reg)
> >>      return rx_is_rw_register(dev, reg);
> >>  }
> >>
> >> -static struct regmap_config rx_regmap_config = {
> >> +static const struct regmap_config rx_regmap_config = {
> >>      .name = "rx_macro",
> >>      .reg_bits = 16,
> >>      .val_bits = 32, /* 8 but with 32 bit read/write */
> >> @@ -3765,6 +3765,7 @@ static const struct snd_soc_component_driver rx_macro_component_drv = {
> >>  static int rx_macro_probe(struct platform_device *pdev)
> >>  {
> >>      struct reg_default *reg_defaults;
> >> +    struct regmap_config *reg_config;
> >>      struct device *dev = &pdev->dev;
> >>      kernel_ulong_t flags;
> >>      struct rx_macro *rx;
> >> @@ -3851,14 +3852,22 @@ static int rx_macro_probe(struct platform_device *pdev)
> >>              goto err;
> >>      }
> >>
> >> -    rx_regmap_config.reg_defaults = reg_defaults;
> >> -    rx_regmap_config.num_reg_defaults = def_count;
> >> +    reg_config = devm_kmemdup(dev, &rx_regmap_config, sizeof(*reg_config),
> >> +                              GFP_KERNEL);
> >> +    if (!reg_config) {
> >> +            ret = -ENOMEM;
> >> +            goto err;
> >> +    }
> >>
> >> -    rx->regmap = devm_regmap_init_mmio(dev, base, &rx_regmap_config);
> >> +    reg_config->reg_defaults = reg_defaults;
> >> +    reg_config->num_reg_defaults = def_count;
> >> +
> >> +    rx->regmap = devm_regmap_init_mmio(dev, base, reg_config);
> >>      if (IS_ERR(rx->regmap)) {
> >>              ret = PTR_ERR(rx->regmap);
> >>              goto err;
> >>      }
> >> +    devm_kfree(dev, reg_config);
> >>      devm_kfree(dev, reg_defaults);
> >
> > Seeing devm_kfree in the non-error path makes me feel strange. Maybe
> > it's one of the rare occasions when I can say that __free is suitable
> > here.
>
> These would have a bit different meaning in such case. The __free would
> not clean it in this spot, but on exit from the scope. I wanted to
> kfree() here, because the config (and reg_defaults) are not used by past
> regmap_init. I mentioned it briefly in previous patch msg.
>
> To me this code is readable and obvious - past this point nothing uses
> that allocation. However maybe instead of devm(), the code would be
> easier to read if non-devm-malloc + __free()?

Yes, that's what I was thinking about. But it's definitely an optional
topic. Your code is correct.


-- 
With best wishes
Dmitry