Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191125100259.5147-1-o.rempel@pengutronix.de>
In-Reply-To: <20191125100259.5147-1-o.rempel@pengutronix.de>
From:   Vladimir Oltean <olteanv@gmail.com>
Date:   Mon, 25 Nov 2019 12:19:50 +0200
Message-ID: <CA+h21hrOO6AFhvXQL47LwqCKU9vpRZ47feWB6fkn=WfrdZr6tA@mail.gmail.com>
Subject: Re: [PATCH v1 1/2] net: dsa: sja1105: print info about probet chip
 only after every thing was done.
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     mkl@pengutronix.de, Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>, kernel@pengutronix.de,
        netdev <netdev@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>, david@protonic.nl
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Oleksij,

On Mon, 25 Nov 2019 at 12:03, Oleksij Rempel <o.rempel@pengutronix.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@pengutronix.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