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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
From: Yizhuo <[email protected]>
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?
On Wed, Feb 6, 2019 at 9:52 PM Yizhuo Zhai <[email protected]> wrote:
>
> Thanks, but why initialization matters here? Is performance the main concern?
>
> On Wed, Feb 6, 2019 at 8:17 PM David Miller <[email protected]> wrote:
>>
>> From: Yizhuo <[email protected]>
>> 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
On Wed, Feb 06, 2019 at 09:53:16PM -0800, Yizhuo Zhai wrote:
>
>
> On Wed, Feb 6, 2019 at 9:52 PM Yizhuo Zhai <[email protected]> wrote:
> >
> > Thanks, but why initialization matters here? Is performance the main concern?
> >
> > On Wed, Feb 6, 2019 at 8:17 PM David Miller <[email protected]> wrote:
> >>
> >> From: Yizhuo <[email protected]>
> >> 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
Make sense, I will send the new patch. Thanks for the opinion.
On Thu, Feb 7, 2019 at 1:25 AM Maxime Ripard <[email protected]> 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 <[email protected]> wrote:
> > >
> > > Thanks, but why initialization matters here? Is performance the main concern?
> > >
> > > On Wed, Feb 6, 2019 at 8:17 PM David Miller <[email protected]> wrote:
> > >>
> > >> From: Yizhuo <[email protected]>
> > >> 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