| Message ID | 20200416182402.16858-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 098c408b040d49158dc6aea52db7493187ac5e09 |
| Headers | show |
| Series | mmc: sdhci-of-arasan: Remove uninitialized ret variables | expand |
On Thu, Apr 16, 2020 at 11:24 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > > Clang warns: > > drivers/mmc/host/sdhci-of-arasan.c:784:9: warning: variable 'ret' is > uninitialized when used here [-Wuninitialized] > return ret; > ^~~ > drivers/mmc/host/sdhci-of-arasan.c:738:9: note: initialize the variable > 'ret' to silence this warning > int ret; > ^ > = 0 > drivers/mmc/host/sdhci-of-arasan.c:860:9: warning: variable 'ret' is > uninitialized when used here [-Wuninitialized] > return ret; > ^~~ > drivers/mmc/host/sdhci-of-arasan.c:810:9: note: initialize the variable > 'ret' to silence this warning > int ret; > ^ > = 0 > 2 warnings generated. > > This looks like a copy paste error. Neither function has handling that > needs ret so just remove it and return 0 directly. Forgive me for not taking the time to look into this more carefully, but just a thought: Having functions always return a single integer literal as opposed to having a `void` return type in their function signature is a code smell. Did you consider the call sites of these functions to see if they do anything with the return value? I understand it may not be worthwhile/possible if these functions fulfil an interface that requires the int return type function signature. (It's also probably faster for me to just look rather than type this all out, but I saw no mention of this consideration in the commit message or patch, so wanted to check that it had been performed). > > Fixes: f73e66a36772 ("sdhci: arasan: Add support for Versal Tap Delays") > Link: https://github.com/ClangBuiltLinux/linux/issues/996 > Reported-by: kernelci.org bot <bot@kernelci.org> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > drivers/mmc/host/sdhci-of-arasan.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c > index 16e26c217a77..18bf0e76b1eb 100644 > --- a/drivers/mmc/host/sdhci-of-arasan.c > +++ b/drivers/mmc/host/sdhci-of-arasan.c > @@ -735,7 +735,6 @@ static int sdhci_versal_sdcardclk_set_phase(struct clk_hw *hw, int degrees) > container_of(clk_data, struct sdhci_arasan_data, clk_data); > struct sdhci_host *host = sdhci_arasan->host; > u8 tap_delay, tap_max = 0; > - int ret; > > /* > * This is applicable for SDHCI_SPEC_300 and above > @@ -781,7 +780,7 @@ static int sdhci_versal_sdcardclk_set_phase(struct clk_hw *hw, int degrees) > sdhci_writel(host, regval, SDHCI_ARASAN_OTAPDLY_REGISTER); > } > > - return ret; > + return 0; > } > > static const struct clk_ops versal_sdcardclk_ops = { > @@ -807,7 +806,6 @@ static int sdhci_versal_sampleclk_set_phase(struct clk_hw *hw, int degrees) > container_of(clk_data, struct sdhci_arasan_data, clk_data); > struct sdhci_host *host = sdhci_arasan->host; > u8 tap_delay, tap_max = 0; > - int ret; > > /* > * This is applicable for SDHCI_SPEC_300 and above > @@ -857,7 +855,7 @@ static int sdhci_versal_sampleclk_set_phase(struct clk_hw *hw, int degrees) > sdhci_writel(host, regval, SDHCI_ARASAN_ITAPDLY_REGISTER); > } > > - return ret; > + return 0; > } > > static const struct clk_ops versal_sampleclk_ops = { > > base-commit: a3ca59b9af21e68069555ffff1ad89bd2a7c40fc > -- > 2.26.1 > > -- > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group. > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200416182402.16858-1-natechancellor%40gmail.com.
On Thu, Apr 16, 2020 at 01:16:27PM -0700, Nick Desaulniers wrote: > On Thu, Apr 16, 2020 at 11:24 AM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > Clang warns: > > > > drivers/mmc/host/sdhci-of-arasan.c:784:9: warning: variable 'ret' is > > uninitialized when used here [-Wuninitialized] > > return ret; > > ^~~ > > drivers/mmc/host/sdhci-of-arasan.c:738:9: note: initialize the variable > > 'ret' to silence this warning > > int ret; > > ^ > > = 0 > > drivers/mmc/host/sdhci-of-arasan.c:860:9: warning: variable 'ret' is > > uninitialized when used here [-Wuninitialized] > > return ret; > > ^~~ > > drivers/mmc/host/sdhci-of-arasan.c:810:9: note: initialize the variable > > 'ret' to silence this warning > > int ret; > > ^ > > = 0 > > 2 warnings generated. > > > > This looks like a copy paste error. Neither function has handling that > > needs ret so just remove it and return 0 directly. > > Forgive me for not taking the time to look into this more carefully, > but just a thought: > > Having functions always return a single integer literal as opposed to > having a `void` return type in their function signature is a code > smell. Did you consider the call sites of these functions to see if > they do anything with the return value? I understand it may not be > worthwhile/possible if these functions fulfil an interface that > requires the int return type function signature. (It's also probably Which is the case. These functions are passed to 'struct clk_ops', which defines the set_phase member as int (*set_phase)(struct clk_hw *hw, int degrees); so we cannot just change the return to void since there are other set_phase functions that do set a return value other than zero. > faster for me to just look rather than type this all out, but I saw no > mention of this consideration in the commit message or patch, so > wanted to check that it had been performed). Yeah, I should have probably mentioned that. I can do so if the maintainers feel it worthwhile. Cheers, Nathan
On Thu, Apr 16, 2020 at 1:24 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Thu, Apr 16, 2020 at 01:16:27PM -0700, Nick Desaulniers wrote: > > On Thu, Apr 16, 2020 at 11:24 AM Nathan Chancellor > > <natechancellor@gmail.com> wrote: > > > > > > Clang warns: > > > > > > drivers/mmc/host/sdhci-of-arasan.c:784:9: warning: variable 'ret' is > > > uninitialized when used here [-Wuninitialized] > > > return ret; > > > ^~~ > > > drivers/mmc/host/sdhci-of-arasan.c:738:9: note: initialize the variable > > > 'ret' to silence this warning > > > int ret; > > > ^ > > > = 0 > > > drivers/mmc/host/sdhci-of-arasan.c:860:9: warning: variable 'ret' is > > > uninitialized when used here [-Wuninitialized] > > > return ret; > > > ^~~ > > > drivers/mmc/host/sdhci-of-arasan.c:810:9: note: initialize the variable > > > 'ret' to silence this warning > > > int ret; > > > ^ > > > = 0 > > > 2 warnings generated. > > > > > > This looks like a copy paste error. Neither function has handling that > > > needs ret so just remove it and return 0 directly. > > > > Forgive me for not taking the time to look into this more carefully, > > but just a thought: > > > > Having functions always return a single integer literal as opposed to > > having a `void` return type in their function signature is a code > > smell. Did you consider the call sites of these functions to see if > > they do anything with the return value? I understand it may not be > > worthwhile/possible if these functions fulfil an interface that > > requires the int return type function signature. (It's also probably > > Which is the case. These functions are passed to 'struct clk_ops', which > defines the set_phase member as > > int (*set_phase)(struct clk_hw *hw, int degrees); > > so we cannot just change the return to void since there are other > set_phase functions that do set a return value other than zero. > > > faster for me to just look rather than type this all out, but I saw no > > mention of this consideration in the commit message or patch, so > > wanted to check that it had been performed). > > Yeah, I should have probably mentioned that. I can do so if the > maintainers feel it worthwhile. No worries, I should hold my comments until I can give a more thorough review, which I've now done. LGTM and thanks for the patch! Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
On Thu, 16 Apr 2020 at 20:24, Nathan Chancellor <natechancellor@gmail.com> wrote: > > Clang warns: > > drivers/mmc/host/sdhci-of-arasan.c:784:9: warning: variable 'ret' is > uninitialized when used here [-Wuninitialized] > return ret; > ^~~ > drivers/mmc/host/sdhci-of-arasan.c:738:9: note: initialize the variable > 'ret' to silence this warning > int ret; > ^ > = 0 > drivers/mmc/host/sdhci-of-arasan.c:860:9: warning: variable 'ret' is > uninitialized when used here [-Wuninitialized] > return ret; > ^~~ > drivers/mmc/host/sdhci-of-arasan.c:810:9: note: initialize the variable > 'ret' to silence this warning > int ret; > ^ > = 0 > 2 warnings generated. > > This looks like a copy paste error. Neither function has handling that > needs ret so just remove it and return 0 directly. > > Fixes: f73e66a36772 ("sdhci: arasan: Add support for Versal Tap Delays") > Link: https://github.com/ClangBuiltLinux/linux/issues/996 > Reported-by: kernelci.org bot <bot@kernelci.org> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> Applied for next, thanks! Kind regards Uffe > --- > drivers/mmc/host/sdhci-of-arasan.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c > index 16e26c217a77..18bf0e76b1eb 100644 > --- a/drivers/mmc/host/sdhci-of-arasan.c > +++ b/drivers/mmc/host/sdhci-of-arasan.c > @@ -735,7 +735,6 @@ static int sdhci_versal_sdcardclk_set_phase(struct clk_hw *hw, int degrees) > container_of(clk_data, struct sdhci_arasan_data, clk_data); > struct sdhci_host *host = sdhci_arasan->host; > u8 tap_delay, tap_max = 0; > - int ret; > > /* > * This is applicable for SDHCI_SPEC_300 and above > @@ -781,7 +780,7 @@ static int sdhci_versal_sdcardclk_set_phase(struct clk_hw *hw, int degrees) > sdhci_writel(host, regval, SDHCI_ARASAN_OTAPDLY_REGISTER); > } > > - return ret; > + return 0; > } > > static const struct clk_ops versal_sdcardclk_ops = { > @@ -807,7 +806,6 @@ static int sdhci_versal_sampleclk_set_phase(struct clk_hw *hw, int degrees) > container_of(clk_data, struct sdhci_arasan_data, clk_data); > struct sdhci_host *host = sdhci_arasan->host; > u8 tap_delay, tap_max = 0; > - int ret; > > /* > * This is applicable for SDHCI_SPEC_300 and above > @@ -857,7 +855,7 @@ static int sdhci_versal_sampleclk_set_phase(struct clk_hw *hw, int degrees) > sdhci_writel(host, regval, SDHCI_ARASAN_ITAPDLY_REGISTER); > } > > - return ret; > + return 0; > } > > static const struct clk_ops versal_sampleclk_ops = { > > base-commit: a3ca59b9af21e68069555ffff1ad89bd2a7c40fc > -- > 2.26.1 >
diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c index 16e26c217a77..18bf0e76b1eb 100644 --- a/drivers/mmc/host/sdhci-of-arasan.c +++ b/drivers/mmc/host/sdhci-of-arasan.c @@ -735,7 +735,6 @@ static int sdhci_versal_sdcardclk_set_phase(struct clk_hw *hw, int degrees) container_of(clk_data, struct sdhci_arasan_data, clk_data); struct sdhci_host *host = sdhci_arasan->host; u8 tap_delay, tap_max = 0; - int ret; /* * This is applicable for SDHCI_SPEC_300 and above @@ -781,7 +780,7 @@ static int sdhci_versal_sdcardclk_set_phase(struct clk_hw *hw, int degrees) sdhci_writel(host, regval, SDHCI_ARASAN_OTAPDLY_REGISTER); } - return ret; + return 0; } static const struct clk_ops versal_sdcardclk_ops = { @@ -807,7 +806,6 @@ static int sdhci_versal_sampleclk_set_phase(struct clk_hw *hw, int degrees) container_of(clk_data, struct sdhci_arasan_data, clk_data); struct sdhci_host *host = sdhci_arasan->host; u8 tap_delay, tap_max = 0; - int ret; /* * This is applicable for SDHCI_SPEC_300 and above @@ -857,7 +855,7 @@ static int sdhci_versal_sampleclk_set_phase(struct clk_hw *hw, int degrees) sdhci_writel(host, regval, SDHCI_ARASAN_ITAPDLY_REGISTER); } - return ret; + return 0; } static const struct clk_ops versal_sampleclk_ops = {
Clang warns: drivers/mmc/host/sdhci-of-arasan.c:784:9: warning: variable 'ret' is uninitialized when used here [-Wuninitialized] return ret; ^~~ drivers/mmc/host/sdhci-of-arasan.c:738:9: note: initialize the variable 'ret' to silence this warning int ret; ^ = 0 drivers/mmc/host/sdhci-of-arasan.c:860:9: warning: variable 'ret' is uninitialized when used here [-Wuninitialized] return ret; ^~~ drivers/mmc/host/sdhci-of-arasan.c:810:9: note: initialize the variable 'ret' to silence this warning int ret; ^ = 0 2 warnings generated. This looks like a copy paste error. Neither function has handling that needs ret so just remove it and return 0 directly. Fixes: f73e66a36772 ("sdhci: arasan: Add support for Versal Tap Delays") Link: https://github.com/ClangBuiltLinux/linux/issues/996 Reported-by: kernelci.org bot <bot@kernelci.org> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- drivers/mmc/host/sdhci-of-arasan.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) base-commit: a3ca59b9af21e68069555ffff1ad89bd2a7c40fc