[PATCH] mmc: sdhci-cadence: Fix -Wuninitialized in sdhci_cdns_tune_blkgap()

[PATCH] mmc: sdhci-cadence: Fix -Wuninitialized in sdhci_cdns_tune_blkgap()

[Nathan Chancellor]

Clang warns (or errors with CONFIG_WERROR=y):

  drivers/mmc/host/sdhci-cadence.c:297:9: error: variable 'hrs37_mode' is uninitialized when used here [-Werror,-Wuninitialized]
    297 |         writel(hrs37_mode, hrs37_reg);
        |                ^~~~~~~~~~
  drivers/mmc/host/sdhci-cadence.c:291:16: note: initialize the variable 'hrs37_mode' to silence this warning
    291 |         u32 hrs37_mode;
        |                       ^
        |                        = 0

A previous revision assigned SDHCI_CDNS_HRS37_MODE_MMC_HS200 to
hrs37_mode in a switch statement but the final revision moved to a
simple if statement. Pass that as the value to writel() and
remove hrs37_mode, clearing up the warning.

Fixes: 60613a8b9b81 ("mmc: sdhci-cadence: implement multi-block read gap tuning")
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 drivers/mmc/host/sdhci-cadence.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
index a2a4a5b0ab96..eaa88897a256 100644
--- a/drivers/mmc/host/sdhci-cadence.c
+++ b/drivers/mmc/host/sdhci-cadence.c
@@ -288,13 +288,12 @@ static int sdhci_cdns_tune_blkgap(struct mmc_host *mmc)
 	void __iomem *hrs38_reg = priv->hrs_addr + SDHCI_CDNS_HRS38;
 	int ret;
 	u32 gap;
-	u32 hrs37_mode;
 
 	/* Currently only needed in HS200 mode */
 	if (host->timing != MMC_TIMING_MMC_HS200)
 		return 0;
 
-	writel(hrs37_mode, hrs37_reg);
+	writel(SDHCI_CDNS_HRS37_MODE_MMC_HS200, hrs37_reg);
 
 	for (gap = 0; gap <= SDHCI_CDNS_HRS38_BLKGAP_MAX; gap++) {
 		writel(gap, hrs38_reg);

---
base-commit: 7138017038c42feb682445407974ed736e1ff308
change-id: 20250819-mmc-sdhci-cadence-fix-uninit-hrs37_mode-cc1246cb39d8

Best regards,
--  
Nathan Chancellor <nathan@kernel.org>

Re: [PATCH] mmc: sdhci-cadence: Fix -Wuninitialized in sdhci_cdns_tune_blkgap()

[Benoît Monin]

Hi Nathan,

On Tuesday, 19 August 2025 at 19:28:49 CEST, Nathan Chancellor wrote:
> Clang warns (or errors with CONFIG_WERROR=y):
> 
>   drivers/mmc/host/sdhci-cadence.c:297:9: error: variable 'hrs37_mode' is uninitialized when used here [-Werror,-Wuninitialized]
>     297 |         writel(hrs37_mode, hrs37_reg);
>         |                ^~~~~~~~~~
>   drivers/mmc/host/sdhci-cadence.c:291:16: note: initialize the variable 'hrs37_mode' to silence this warning
>     291 |         u32 hrs37_mode;
>         |                       ^
>         |                        = 0
> 
> A previous revision assigned SDHCI_CDNS_HRS37_MODE_MMC_HS200 to
> hrs37_mode in a switch statement but the final revision moved to a
> simple if statement. Pass that as the value to writel() and
> remove hrs37_mode, clearing up the warning.
> 
> Fixes: 60613a8b9b81 ("mmc: sdhci-cadence: implement multi-block read gap tuning")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>  drivers/mmc/host/sdhci-cadence.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
> index a2a4a5b0ab96..eaa88897a256 100644
> --- a/drivers/mmc/host/sdhci-cadence.c
> +++ b/drivers/mmc/host/sdhci-cadence.c
> @@ -288,13 +288,12 @@ static int sdhci_cdns_tune_blkgap(struct mmc_host *mmc)
>  	void __iomem *hrs38_reg = priv->hrs_addr + SDHCI_CDNS_HRS38;
>  	int ret;
>  	u32 gap;
> -	u32 hrs37_mode;
>  
>  	/* Currently only needed in HS200 mode */
>  	if (host->timing != MMC_TIMING_MMC_HS200)
>  		return 0;
>  
> -	writel(hrs37_mode, hrs37_reg);
> +	writel(SDHCI_CDNS_HRS37_MODE_MMC_HS200, hrs37_reg);
>  
>  	for (gap = 0; gap <= SDHCI_CDNS_HRS38_BLKGAP_MAX; gap++) {
>  		writel(gap, hrs38_reg);
> 
Thanks for the catch!

I don't get how gcc does not raise a warning here, only clang does. I
did build with gcc-11 and gcc-15 and they don't complain about this
uninitialized variable.

Tested-by: Benoît Monin <benoit.monin@bootlin.com>

> ---
> base-commit: 7138017038c42feb682445407974ed736e1ff308
> change-id: 20250819-mmc-sdhci-cadence-fix-uninit-hrs37_mode-cc1246cb39d8
> 
> Best regards,
> --  
> Nathan Chancellor <nathan@kernel.org>
> 
> 

Best regards,
-- 
Benoît Monin, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] mmc: sdhci-cadence: Fix -Wuninitialized in sdhci_cdns_tune_blkgap()

[Dan Carpenter]

On Wed, Aug 20, 2025 at 10:07:35AM +0200, Benoît Monin wrote:
> > --- a/drivers/mmc/host/sdhci-cadence.c
> > +++ b/drivers/mmc/host/sdhci-cadence.c
> > @@ -288,13 +288,12 @@ static int sdhci_cdns_tune_blkgap(struct mmc_host *mmc)
> >  	void __iomem *hrs38_reg = priv->hrs_addr + SDHCI_CDNS_HRS38;
> >  	int ret;
> >  	u32 gap;
> > -	u32 hrs37_mode;
> >  
> >  	/* Currently only needed in HS200 mode */
> >  	if (host->timing != MMC_TIMING_MMC_HS200)
> >  		return 0;
> >  
> > -	writel(hrs37_mode, hrs37_reg);
> > +	writel(SDHCI_CDNS_HRS37_MODE_MMC_HS200, hrs37_reg);
> >  
> >  	for (gap = 0; gap <= SDHCI_CDNS_HRS38_BLKGAP_MAX; gap++) {
> >  		writel(gap, hrs38_reg);
> > 
> Thanks for the catch!
> 
> I don't get how gcc does not raise a warning here, only clang does. I
> did build with gcc-11 and gcc-15 and they don't complain about this
> uninitialized variable.
> 

We disabled uninitialized variable checking on GCC.  It was too crazy
and especially if we want to use -Werror.

Smatch also detects this bug.

regards,
dan carpenter

Re: [PATCH] mmc: sdhci-cadence: Fix -Wuninitialized in sdhci_cdns_tune_blkgap()

[Nathan Chancellor]

On Wed, Aug 20, 2025 at 01:43:09PM +0300, Dan Carpenter wrote:
> On Wed, Aug 20, 2025 at 10:07:35AM +0200, Benoît Monin wrote:
> > > --- a/drivers/mmc/host/sdhci-cadence.c
> > > +++ b/drivers/mmc/host/sdhci-cadence.c
> > > @@ -288,13 +288,12 @@ static int sdhci_cdns_tune_blkgap(struct mmc_host *mmc)
> > >  	void __iomem *hrs38_reg = priv->hrs_addr + SDHCI_CDNS_HRS38;
> > >  	int ret;
> > >  	u32 gap;
> > > -	u32 hrs37_mode;
> > >  
> > >  	/* Currently only needed in HS200 mode */
> > >  	if (host->timing != MMC_TIMING_MMC_HS200)
> > >  		return 0;
> > >  
> > > -	writel(hrs37_mode, hrs37_reg);
> > > +	writel(SDHCI_CDNS_HRS37_MODE_MMC_HS200, hrs37_reg);
> > >  
> > >  	for (gap = 0; gap <= SDHCI_CDNS_HRS38_BLKGAP_MAX; gap++) {
> > >  		writel(gap, hrs38_reg);
> > > 
> > Thanks for the catch!
> > 
> > I don't get how gcc does not raise a warning here, only clang does. I
> > did build with gcc-11 and gcc-15 and they don't complain about this
> > uninitialized variable.
> > 
> 
> We disabled uninitialized variable checking on GCC.  It was too crazy
> and especially if we want to use -Werror.
> 
> Smatch also detects this bug.

Technically, -Wuninitialized is on for GCC as well but as soon as there
is control flow between the declaration of the variable and its usage,
it gets changed into -Wmaybe-uninitialized :/

In file included from arch/arm64/include/asm/io.h:288,
                 from include/linux/io.h:12,
                 from include/linux/iopoll.h:14,
                 from drivers/mmc/host/sdhci-cadence.c:9:
In function 'writel',
    inlined from 'sdhci_cdns_tune_blkgap' at drivers/mmc/host/sdhci-cadence.c:297:2,
    inlined from 'sdhci_cdns_execute_tuning' at drivers/mmc/host/sdhci-cadence.c:352:9:
include/asm-generic/io.h:276:9: error: 'hrs37_mode' may be used uninitialized [-Werror=maybe-uninitialized]
  276 |         log_write_mmio(value, 32, addr, _THIS_IP_, _RET_IP_);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/mmc/host/sdhci-cadence.c: In function 'sdhci_cdns_execute_tuning':
drivers/mmc/host/sdhci-cadence.c:291:13: note: 'hrs37_mode' was declared here
  291 |         u32 hrs37_mode;
      |             ^~~~~~~~~~

vs

diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
index a2a4a5b0ab96..b968c0fb925d 100644
--- a/drivers/mmc/host/sdhci-cadence.c
+++ b/drivers/mmc/host/sdhci-cadence.c
@@ -290,12 +290,12 @@ static int sdhci_cdns_tune_blkgap(struct mmc_host *mmc)
        u32 gap;
        u32 hrs37_mode;

+       writel(hrs37_mode, hrs37_reg);
+
        /* Currently only needed in HS200 mode */
        if (host->timing != MMC_TIMING_MMC_HS200)
                return 0;

-       writel(hrs37_mode, hrs37_reg);
-
        for (gap = 0; gap <= SDHCI_CDNS_HRS38_BLKGAP_MAX; gap++) {
                writel(gap, hrs38_reg);
                ret = mmc_read_tuning(mmc, 512, 32);

In file included from arch/arm64/include/asm/io.h:288,
                 from include/linux/io.h:12,
                 from include/linux/iopoll.h:14,
                 from drivers/mmc/host/sdhci-cadence.c:9:
drivers/mmc/host/sdhci-cadence.c: In function 'sdhci_cdns_tune_blkgap':
include/asm-generic/io.h:273:16: error: 'hrs37_mode' is used uninitialized [-Werror=uninitialized]
  273 | #define writel writel
drivers/mmc/host/sdhci-cadence.c:293:9: note: in expansion of macro 'writel'
  293 |         writel(hrs37_mode, hrs37_reg);
      |         ^~~~~~
drivers/mmc/host/sdhci-cadence.c:291:13: note: 'hrs37_mode' was declared here
  291 |         u32 hrs37_mode;
      |             ^~~~~~~~~~

It would be nice if GCC could adopt clang's semantics of "uninitialized
when used _here_" for -Wuninitalized versus the current scheme of
converting it to a "may be used uninitialized" warning.

Cheers,
Nathan

Re: [PATCH] mmc: sdhci-cadence: Fix -Wuninitialized in sdhci_cdns_tune_blkgap()

[Ulf Hansson]

On Tue, 19 Aug 2025 at 19:29, Nathan Chancellor <nathan@kernel.org> wrote:
>
> Clang warns (or errors with CONFIG_WERROR=y):
>
>   drivers/mmc/host/sdhci-cadence.c:297:9: error: variable 'hrs37_mode' is uninitialized when used here [-Werror,-Wuninitialized]
>     297 |         writel(hrs37_mode, hrs37_reg);
>         |                ^~~~~~~~~~
>   drivers/mmc/host/sdhci-cadence.c:291:16: note: initialize the variable 'hrs37_mode' to silence this warning
>     291 |         u32 hrs37_mode;
>         |                       ^
>         |                        = 0
>
> A previous revision assigned SDHCI_CDNS_HRS37_MODE_MMC_HS200 to
> hrs37_mode in a switch statement but the final revision moved to a
> simple if statement. Pass that as the value to writel() and
> remove hrs37_mode, clearing up the warning.
>
> Fixes: 60613a8b9b81 ("mmc: sdhci-cadence: implement multi-block read gap tuning")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

Applied for next, thanks!

Kind regards
Uffe

> ---
>  drivers/mmc/host/sdhci-cadence.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
> index a2a4a5b0ab96..eaa88897a256 100644
> --- a/drivers/mmc/host/sdhci-cadence.c
> +++ b/drivers/mmc/host/sdhci-cadence.c
> @@ -288,13 +288,12 @@ static int sdhci_cdns_tune_blkgap(struct mmc_host *mmc)
>         void __iomem *hrs38_reg = priv->hrs_addr + SDHCI_CDNS_HRS38;
>         int ret;
>         u32 gap;
> -       u32 hrs37_mode;
>
>         /* Currently only needed in HS200 mode */
>         if (host->timing != MMC_TIMING_MMC_HS200)
>                 return 0;
>
> -       writel(hrs37_mode, hrs37_reg);
> +       writel(SDHCI_CDNS_HRS37_MODE_MMC_HS200, hrs37_reg);
>
>         for (gap = 0; gap <= SDHCI_CDNS_HRS38_BLKGAP_MAX; gap++) {
>                 writel(gap, hrs38_reg);
>
> ---
> base-commit: 7138017038c42feb682445407974ed736e1ff308
> change-id: 20250819-mmc-sdhci-cadence-fix-uninit-hrs37_mode-cc1246cb39d8
>
> Best regards,
> --
> Nathan Chancellor <nathan@kernel.org>
>