linux-kernel - Re: [PATCH v1 1/2] net: dsa: sja1105: print info about probet chip only after every thing was done.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+h21hrOO6AFhvXQL47LwqCKU9vpRZ47feWB6fkn=WfrdZr6tA@mail.gmail.com>
Date:   Mon, 25 Nov 2019 12:19:50 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     mkl@...gutronix.de, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>, kernel@...gutronix.de,
        netdev <netdev@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>, david@...tonic.nl
Subject: Re: [PATCH v1 1/2] net: dsa: sja1105: print info about probet chip
 only after every thing was done.

Hi Oleksij,

On Mon, 25 Nov 2019 at 12:03, Oleksij Rempel <o.rempel@...gutronix.de> wrote:
>
> Currently we will get "Probed switch chip" notification multiple times
> if first probe filed by some reason. To avoid this confusing notifications move
> dev_info to the end of probe.
>
> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
> ---
>  drivers/net/dsa/sja1105/sja1105_main.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
> index 7687ddcae159..1238fd68b2cd 100644
> --- a/drivers/net/dsa/sja1105/sja1105_main.c
> +++ b/drivers/net/dsa/sja1105/sja1105_main.c
> @@ -2191,8 +2191,6 @@ static int sja1105_probe(struct spi_device *spi)
>                 return rc;
>         }
>
> -       dev_info(dev, "Probed switch chip: %s\n", priv->info->name);
> -
>         ds = dsa_switch_alloc(dev, SJA1105_NUM_PORTS);
>         if (!ds)
>                 return -ENOMEM;
> @@ -2218,7 +2216,13 @@ static int sja1105_probe(struct spi_device *spi)
>
>         sja1105_tas_setup(ds);
>
> -       return dsa_register_switch(priv->ds);
> +       rc = dsa_register_switch(priv->ds);
> +       if (rc)
> +               return rc;
> +
> +       dev_info(dev, "Probed switch chip: %s\n", priv->info->name);
> +
> +       return 0;
>  }
>
>  static int sja1105_remove(struct spi_device *spi)
> --
> 2.24.0
>

I don't think cosmetic patches should be send against the "net" tree.
At the very least I would not keep the RGMII delays fix and this one
in the same series, since they aren't related and they can be applied
independently.

If you want to actually fix something, there is also a memory leak
related to this. It is present in most DSA drivers. When
dsa_register_switch returns -EPROBE_DEFER, anything allocated with
devm_kzalloc will be overwritten and the old memory will leak. It's a
bit tricky to solve though, and especially tricky to figure out a
proper Fixes: tag, since that devm_kzalloc was also hidden in
dsa_switch_alloc for most of the time (which in net-next was
eliminated by Vivien, thus making it more obvious).

So I think some better mechanism should be thought of, that as little
as possible is done in the period of time where -EPROBE_DEFER can be
returned.

Thanks,
-Vladimir