[PATCH] net: stmmac: Variable "val" in function sun8i_dwmac_set_syscon() could be uninitialized

[PATCH] net: stmmac: Variable "val" in function sun8i_dwmac_set_syscon() could be uninitialized

[Yizhuo]

In function sun8i_dwmac_set_syscon(), local variable "val" could
be uninitialized if function regmap_read() returns -EINVAL.
However, it will be used directly in the if statement, which
is potentially unsafe.

Signed-off-by: Yizhuo <yzhai003@ucr.edu>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index 39c2122a4f26..11d481c9e7ab 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -639,9 +639,14 @@ static int sun8i_dwmac_set_syscon(struct stmmac_priv *priv)
 	struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
 	struct device_node *node = priv->device->of_node;
 	int ret;
-	u32 reg, val;
+	u32 reg, val = 0;
+
+	ret = regmap_read(gmac->regmap, SYSCON_EMAC_REG, &val);
+	if (ret) {
+		dev_err(priv->device, "Fail to read SYSCON_EMAC_REG.\n");
+		return ret;
+	}
 
-	regmap_read(gmac->regmap, SYSCON_EMAC_REG, &val);
 	reg = gmac->variant->default_syscon_value;
 	if (reg != val)
 		dev_warn(priv->device,
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] net: stmmac: Variable "val" in function sun8i_dwmac_set_syscon() could be uninitialized

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1171 bytes --]

Hi,

On Tue, Feb 05, 2019 at 02:15:59PM -0800, Yizhuo wrote:
> In function sun8i_dwmac_set_syscon(), local variable "val" could
> be uninitialized if function regmap_read() returns -EINVAL.
> However, it will be used directly in the if statement, which
> is potentially unsafe.
> 
> Signed-off-by: Yizhuo <yzhai003@ucr.edu>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> index 39c2122a4f26..11d481c9e7ab 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> @@ -639,9 +639,14 @@ static int sun8i_dwmac_set_syscon(struct stmmac_priv *priv)
>  	struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
>  	struct device_node *node = priv->device->of_node;
>  	int ret;
> -	u32 reg, val;
> +	u32 reg, val = 0;

I guess we don't need to initialize it anymore with the check you add?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] net: stmmac: Variable "val" in function sun8i_dwmac_set_syscon() could be uninitialized

[David Miller]

From: Yizhuo <yzhai003@ucr.edu>
Date: Tue,  5 Feb 2019 14:15:59 -0800

> @@ -639,9 +639,14 @@ static int sun8i_dwmac_set_syscon(struct stmmac_priv *priv)
>  	struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
>  	struct device_node *node = priv->device->of_node;
>  	int ret;
> -	u32 reg, val;
> +	u32 reg, val = 0;
> +
> +	ret = regmap_read(gmac->regmap, SYSCON_EMAC_REG, &val);
> +	if (ret) {
> +		dev_err(priv->device, "Fail to read SYSCON_EMAC_REG.\n");
> +		return ret;
> +	}

I agree with the other reviewer that since you check 'ret' the initialization of
'val' is no longer needed.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] net: stmmac: Variable "val" in function sun8i_dwmac_set_syscon() could be uninitialized

[Yizhuo Zhai]

Thanks, but why initialization matters here? Is performance the main concern?


On Wed, Feb 6, 2019 at 9:52 PM Yizhuo Zhai <yzhai003@ucr.edu> wrote:
>
> Thanks, but why initialization matters here? Is performance the main concern?
>
> On Wed, Feb 6, 2019 at 8:17 PM David Miller <davem@davemloft.net> wrote:
>>
>> From: Yizhuo <yzhai003@ucr.edu>
>> Date: Tue,  5 Feb 2019 14:15:59 -0800
>>
>> > @@ -639,9 +639,14 @@ static int sun8i_dwmac_set_syscon(struct stmmac_priv *priv)
>> >       struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
>> >       struct device_node *node = priv->device->of_node;
>> >       int ret;
>> > -     u32 reg, val;
>> > +     u32 reg, val = 0;
>> > +
>> > +     ret = regmap_read(gmac->regmap, SYSCON_EMAC_REG, &val);
>> > +     if (ret) {
>> > +             dev_err(priv->device, "Fail to read SYSCON_EMAC_REG.\n");
>> > +             return ret;
>> > +     }
>>
>> I agree with the other reviewer that since you check 'ret' the initialization of
>> 'val' is no longer needed.
>
>
>
> --
> Kind Regards,
>
> Yizhuo Zhai
>
> Computer Science, Graduate Student
> University of California, Riverside



-- 
Kind Regards,

Yizhuo Zhai

Computer Science, Graduate Student
University of California, Riverside

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] net: stmmac: Variable "val" in function sun8i_dwmac_set_syscon() could be uninitialized

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1373 bytes --]

On Wed, Feb 06, 2019 at 09:53:16PM -0800, Yizhuo Zhai wrote:
> 
> 
> On Wed, Feb 6, 2019 at 9:52 PM Yizhuo Zhai <yzhai003@ucr.edu> wrote:
> >
> > Thanks, but why initialization matters here? Is performance the main concern?
> >
> > On Wed, Feb 6, 2019 at 8:17 PM David Miller <davem@davemloft.net> wrote:
> >>
> >> From: Yizhuo <yzhai003@ucr.edu>
> >> Date: Tue,  5 Feb 2019 14:15:59 -0800
> >>
> >> > @@ -639,9 +639,14 @@ static int sun8i_dwmac_set_syscon(struct stmmac_priv *priv)
> >> >       struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
> >> >       struct device_node *node = priv->device->of_node;
> >> >       int ret;
> >> > -     u32 reg, val;
> >> > +     u32 reg, val = 0;
> >> > +
> >> > +     ret = regmap_read(gmac->regmap, SYSCON_EMAC_REG, &val);
> >> > +     if (ret) {
> >> > +             dev_err(priv->device, "Fail to read SYSCON_EMAC_REG.\n");
> >> > +             return ret;
> >> > +     }
> >>
> >> I agree with the other reviewer that since you check 'ret' the initialization of
> >> 'val' is no longer needed.
>
> Thanks, but why initialization matters here? Is performance the main
> concern?

Not really, but if we turn this the other way around, why should we do
something that doesn't bring anything?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] net: stmmac: Variable "val" in function sun8i_dwmac_set_syscon() could be uninitialized

[Yizhuo Zhai]

Make sense, I will send the new patch. Thanks for the opinion.

On Thu, Feb 7, 2019 at 1:25 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Wed, Feb 06, 2019 at 09:53:16PM -0800, Yizhuo Zhai wrote:
> >
> >
> > On Wed, Feb 6, 2019 at 9:52 PM Yizhuo Zhai <yzhai003@ucr.edu> wrote:
> > >
> > > Thanks, but why initialization matters here? Is performance the main concern?
> > >
> > > On Wed, Feb 6, 2019 at 8:17 PM David Miller <davem@davemloft.net> wrote:
> > >>
> > >> From: Yizhuo <yzhai003@ucr.edu>
> > >> Date: Tue,  5 Feb 2019 14:15:59 -0800
> > >>
> > >> > @@ -639,9 +639,14 @@ static int sun8i_dwmac_set_syscon(struct stmmac_priv *priv)
> > >> >       struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
> > >> >       struct device_node *node = priv->device->of_node;
> > >> >       int ret;
> > >> > -     u32 reg, val;
> > >> > +     u32 reg, val = 0;
> > >> > +
> > >> > +     ret = regmap_read(gmac->regmap, SYSCON_EMAC_REG, &val);
> > >> > +     if (ret) {
> > >> > +             dev_err(priv->device, "Fail to read SYSCON_EMAC_REG.\n");
> > >> > +             return ret;
> > >> > +     }
> > >>
> > >> I agree with the other reviewer that since you check 'ret' the initialization of
> > >> 'val' is no longer needed.
> >
> > Thanks, but why initialization matters here? Is performance the main
> > concern?
>
> Not really, but if we turn this the other way around, why should we do
> something that doesn't bring anything?
>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com



-- 
Kind Regards,

Yizhuo Zhai

Computer Science, Graduate Student
University of California, Riverside

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] net: stmmac: Variable "val" in function sun8i_dwmac_set_syscon() could be uninitialized

[David Miller]

From: Yizhuo Zhai <yzhai003@ucr.edu>
Date: Wed, 6 Feb 2019 21:52:15 -0800

> Thanks, but why initialization matters here? Is performance the main
> concern?

Code that is unnecessary is hard to audit.

People will ask "why" is it initialized?  In what situations is the
initialized value of "0" ever used?

You are wasting people's time and energy by writing unnecessary code.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] net: stmmac: Variable "val" in function sun8i_dwmac_set_syscon() could be uninitialized

[Yizhuo]

In function sun8i_dwmac_set_syscon(), local variable "val" could
be uninitialized if function regmap_read() returns -EINVAL.
However, it will be used directly in the if statement, which
is potentially unsafe.

Signed-off-by: Yizhuo <yzhai003@ucr.edu>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index 39c2122a4f26..50cfd6d83052 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -641,7 +641,12 @@ static int sun8i_dwmac_set_syscon(struct stmmac_priv *priv)
 	int ret;
 	u32 reg, val;
 
-	regmap_read(gmac->regmap, SYSCON_EMAC_REG, &val);
+	ret = regmap_read(gmac->regmap, SYSCON_EMAC_REG, &val);
+	if (ret) {
+		dev_err(priv->device, "Fail to read SYSCON_EMAC_REG.\n");
+		return ret;
+	}
+
 	reg = gmac->variant->default_syscon_value;
 	if (reg != val)
 		dev_warn(priv->device,
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] net: stmmac: Variable "val" in function sun8i_dwmac_set_syscon() could be uninitialized

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 491 bytes --]

On Thu, Feb 07, 2019 at 09:46:23AM -0800, Yizhuo wrote:
> In function sun8i_dwmac_set_syscon(), local variable "val" could
> be uninitialized if function regmap_read() returns -EINVAL.
> However, it will be used directly in the if statement, which
> is potentially unsafe.
> 
> Signed-off-by: Yizhuo <yzhai003@ucr.edu>

Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] net: stmmac: Variable "val" in function sun8i_dwmac_set_syscon() could be uninitialized

[David Miller]

From: Yizhuo <yzhai003@ucr.edu>
Date: Thu,  7 Feb 2019 09:46:23 -0800

> In function sun8i_dwmac_set_syscon(), local variable "val" could
> be uninitialized if function regmap_read() returns -EINVAL.
> However, it will be used directly in the if statement, which
> is potentially unsafe.
> 
> Signed-off-by: Yizhuo <yzhai003@ucr.edu>

This doesn't apply to any of my trees.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] net: stmmac: Variable "val" in function sun8i_dwmac_set_syscon() could be uninitialized

[Yizhuo Zhai]

Hi David:

Thanks for your feedback, this patch should work for v4.14.


On Fri, Feb 8, 2019 at 11:01 PM David Miller <davem@davemloft.net> wrote:
>
> From: Yizhuo <yzhai003@ucr.edu>
> Date: Thu,  7 Feb 2019 09:46:23 -0800
>
> > In function sun8i_dwmac_set_syscon(), local variable "val" could
> > be uninitialized if function regmap_read() returns -EINVAL.
> > However, it will be used directly in the if statement, which
> > is potentially unsafe.
> >
> > Signed-off-by: Yizhuo <yzhai003@ucr.edu>
>
> This doesn't apply to any of my trees.



--
Kind Regards,

Yizhuo Zhai

Computer Science, Graduate Student
University of California, Riverside

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] net: stmmac: Variable "val" in function sun8i_dwmac_set_syscon() could be uninitialized

[David Miller]

From: Yizhuo Zhai <yzhai003@ucr.edu>
Date: Fri, 30 Aug 2019 15:29:07 -0700

> Thanks for your feedback, this patch should work for v4.14.

You must always submit patches against the current tree.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel