[bug report] ASoC: tegra: Add Tegra210 based OPE driver

[bug report] ASoC: tegra: Add Tegra210 based OPE driver

[Dan Carpenter]

Hello Sameer Pujar,

The patch 7358a803c778: "ASoC: tegra: Add Tegra210 based OPE driver"
from Jun 3, 2022, leads to the following Smatch static checker
warning:

	sound/soc/tegra/tegra210_mbdrc.c:778 tegra210_mbdrc_hw_params()
	warn: bitwise AND condition is false here

sound/soc/tegra/tegra210_mbdrc.c
    769 int tegra210_mbdrc_hw_params(struct snd_soc_component *cmpnt)
    770 {
    771         struct tegra210_ope *ope = snd_soc_component_get_drvdata(cmpnt);
    772         const struct tegra210_mbdrc_config *conf = &mbdrc_init_config;
    773         u32 val = 0;
    774         unsigned int i;
    775 
    776         regmap_read(ope->mbdrc_regmap, TEGRA210_MBDRC_CFG, &val);
    777 
--> 778         if (val & TEGRA210_MBDRC_CFG_MBDRC_MODE_BYPASS)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

TEGRA210_MBDRC_CFG_MBDRC_MODE_BYPASS is zero so this can't be true.

#define TEGRA210_MBDRC_CFG_MBDRC_MODE_BYPASS            (0 << TEGRA210_MBDRC_CFG_MBDRC_MODE_SHIFT)

The common false positive with this warning is that the macro is
something which depends on the .config and in that case I just add it
to the list in smatch_data/kernel.unconstant_macros.  But in this case
the macro is just always zero...  Is there a plan to make it
configurable or something?

    779                 return 0;
    780 
    781         for (i = 0; i < MBDRC_NUM_BAND; i++) {
    782                 const struct tegra210_mbdrc_band_params *params =
    783                         &conf->band_params[i];
    784 
    785                 u32 reg_off = i * TEGRA210_MBDRC_FILTER_PARAM_STRIDE;
    786 
    787                 tegra210_mbdrc_write_ram(ope->mbdrc_regmap,
    788                                          reg_off + TEGRA210_MBDRC_CFG_RAM_CTRL,
    789                                          reg_off + TEGRA210_MBDRC_CFG_RAM_DATA,
    790                                          0, (u32 *)&params->biquad_params[0],
    791                                          TEGRA210_MBDRC_MAX_BIQUAD_STAGES * 5);
    792         }
    793         return 0;
    794 }

regards,
dan carpenter

Re: [bug report] ASoC: tegra: Add Tegra210 based OPE driver

[Sameer Pujar]

On 13-06-2022 19:40, Dan Carpenter wrote:
> Hello Sameer Pujar,
>
> The patch 7358a803c778: "ASoC: tegra: Add Tegra210 based OPE driver"
> from Jun 3, 2022, leads to the following Smatch static checker
> warning:
>
>          sound/soc/tegra/tegra210_mbdrc.c:778 tegra210_mbdrc_hw_params()
>          warn: bitwise AND condition is false here
>
> sound/soc/tegra/tegra210_mbdrc.c
>      769 int tegra210_mbdrc_hw_params(struct snd_soc_component *cmpnt)
>      770 {
>      771         struct tegra210_ope *ope = snd_soc_component_get_drvdata(cmpnt);
>      772         const struct tegra210_mbdrc_config *conf = &mbdrc_init_config;
>      773         u32 val = 0;
>      774         unsigned int i;
>      775
>      776         regmap_read(ope->mbdrc_regmap, TEGRA210_MBDRC_CFG, &val);
>      777
> --> 778         if (val & TEGRA210_MBDRC_CFG_MBDRC_MODE_BYPASS)
>                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> TEGRA210_MBDRC_CFG_MBDRC_MODE_BYPASS is zero so this can't be true.
>
> #define TEGRA210_MBDRC_CFG_MBDRC_MODE_BYPASS            (0 << TEGRA210_MBDRC_CFG_MBDRC_MODE_SHIFT)
>
> The common false positive with this warning is that the macro is
> something which depends on the .config and in that case I just add it
> to the list in smatch_data/kernel.unconstant_macros.  But in this case
> the macro is just always zero...  Is there a plan to make it
> configurable or something?

Thanks Dan for reporting this. The device is actually configurable. I 
will provide a patch to fix above condition.

Re: [bug report] ASoC: tegra: Add Tegra210 based OPE driver

[Dan Carpenter]

On Tue, Jun 14, 2022 at 09:47:21AM +0530, Sameer Pujar wrote:
> 
> On 13-06-2022 19:40, Dan Carpenter wrote:
> > Hello Sameer Pujar,
> > 
> > The patch 7358a803c778: "ASoC: tegra: Add Tegra210 based OPE driver"
> > from Jun 3, 2022, leads to the following Smatch static checker
> > warning:
> > 
> >          sound/soc/tegra/tegra210_mbdrc.c:778 tegra210_mbdrc_hw_params()
> >          warn: bitwise AND condition is false here
> > 
> > sound/soc/tegra/tegra210_mbdrc.c
> >      769 int tegra210_mbdrc_hw_params(struct snd_soc_component *cmpnt)
> >      770 {
> >      771         struct tegra210_ope *ope = snd_soc_component_get_drvdata(cmpnt);
> >      772         const struct tegra210_mbdrc_config *conf = &mbdrc_init_config;
> >      773         u32 val = 0;
> >      774         unsigned int i;
> >      775
> >      776         regmap_read(ope->mbdrc_regmap, TEGRA210_MBDRC_CFG, &val);
> >      777
> > --> 778         if (val & TEGRA210_MBDRC_CFG_MBDRC_MODE_BYPASS)
> >                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > TEGRA210_MBDRC_CFG_MBDRC_MODE_BYPASS is zero so this can't be true.
> > 
> > #define TEGRA210_MBDRC_CFG_MBDRC_MODE_BYPASS            (0 << TEGRA210_MBDRC_CFG_MBDRC_MODE_SHIFT)
> > 
> > The common false positive with this warning is that the macro is
> > something which depends on the .config and in that case I just add it
> > to the list in smatch_data/kernel.unconstant_macros.  But in this case
> > the macro is just always zero...  Is there a plan to make it
> > configurable or something?
> 
> Thanks Dan for reporting this. The device is actually configurable. I will
> provide a patch to fix above condition.

What I meant by configurable is:

#ifdef CONFIG_FOO
#define MASK 0x30
#else
#define MASK 0
#endif

Smatch works on the preprocessed source so it doesn't see that there
are two definitions of MASK.

regards,
dan carpenter

Re: [bug report] ASoC: tegra: Add Tegra210 based OPE driver

[Sameer Pujar]

On 14-06-2022 11:39, Dan Carpenter wrote:
>>> Hello Sameer Pujar,
>>>
>>> The patch 7358a803c778: "ASoC: tegra: Add Tegra210 based OPE driver"
>>> from Jun 3, 2022, leads to the following Smatch static checker
>>> warning:
>>>
>>>           sound/soc/tegra/tegra210_mbdrc.c:778 tegra210_mbdrc_hw_params()
>>>           warn: bitwise AND condition is false here
>>>
>>> sound/soc/tegra/tegra210_mbdrc.c
>>>       769 int tegra210_mbdrc_hw_params(struct snd_soc_component *cmpnt)
>>>       770 {
>>>       771         struct tegra210_ope *ope = snd_soc_component_get_drvdata(cmpnt);
>>>       772         const struct tegra210_mbdrc_config *conf = &mbdrc_init_config;
>>>       773         u32 val = 0;
>>>       774         unsigned int i;
>>>       775
>>>       776         regmap_read(ope->mbdrc_regmap, TEGRA210_MBDRC_CFG, &val);
>>>       777
>>> --> 778         if (val & TEGRA210_MBDRC_CFG_MBDRC_MODE_BYPASS)
>>>                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>
>>> TEGRA210_MBDRC_CFG_MBDRC_MODE_BYPASS is zero so this can't be true.
>>>
>>> #define TEGRA210_MBDRC_CFG_MBDRC_MODE_BYPASS            (0 << TEGRA210_MBDRC_CFG_MBDRC_MODE_SHIFT)
>>>
>>> The common false positive with this warning is that the macro is
>>> something which depends on the .config and in that case I just add it
>>> to the list in smatch_data/kernel.unconstant_macros.  But in this case
>>> the macro is just always zero...  Is there a plan to make it
>>> configurable or something?
>> Thanks Dan for reporting this. The device is actually configurable. I will
>> provide a patch to fix above condition.
> What I meant by configurable is:
>
> #ifdef CONFIG_FOO
> #define MASK 0x30
> #else
> #define MASK 0
> #endif
>
> Smatch works on the preprocessed source so it doesn't see that there
> are two definitions of MASK.

By configurable I meant, the device (OPE in this case) supports 
different configurations driven by user space control settings. Above if 
condition (val & TEGRA210_MBDRC_CFG_MBDRC_MODE_BYPASS) is wrong, instead 
proper mask needs to be used. I have sent a patch to fix this now.