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

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

[Oleksij Rempel]

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

[PATCH v1 2/2] net: dsa: sja1105: fix sja1105_parse_rgmii_delays()

[Oleksij Rempel]

This function was using configuration of port 0 in devicetree for all ports.
In case CPU port was not 0, the delay settings was ignored. This resulted not
working communication between CPU and the switch.

Fixes: f5b8631c293b ("net: dsa: sja1105: Error out if RGMII delays are requested in DT")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 1238fd68b2cd..34544b1c30dc 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -594,15 +594,15 @@ static int sja1105_parse_rgmii_delays(struct sja1105_private *priv,
 	int i;
 
 	for (i = 0; i < SJA1105_NUM_PORTS; i++) {
-		if (ports->role == XMII_MAC)
+		if (ports[i].role == XMII_MAC)
 			continue;
 
-		if (ports->phy_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
-		    ports->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
+		if (ports[i].phy_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
+		    ports[i].phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
 			priv->rgmii_rx_delay[i] = true;
 
-		if (ports->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID ||
-		    ports->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
+		if (ports[i].phy_mode == PHY_INTERFACE_MODE_RGMII_TXID ||
+		    ports[i].phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
 			priv->rgmii_tx_delay[i] = true;
 
 		if ((priv->rgmii_rx_delay[i] || priv->rgmii_tx_delay[i]) &&
-- 
2.24.0

Re: [PATCH v1 2/2] net: dsa: sja1105: fix sja1105_parse_rgmii_delays()

[Vladimir Oltean]

On Mon, 25 Nov 2019 at 12:03, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>
> This function was using configuration of port 0 in devicetree for all ports.
> In case CPU port was not 0, the delay settings was ignored. This resulted not
> working communication between CPU and the switch.
>
> Fixes: f5b8631c293b ("net: dsa: sja1105: Error out if RGMII delays are requested in DT")
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

>  drivers/net/dsa/sja1105/sja1105_main.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
> index 1238fd68b2cd..34544b1c30dc 100644
> --- a/drivers/net/dsa/sja1105/sja1105_main.c
> +++ b/drivers/net/dsa/sja1105/sja1105_main.c
> @@ -594,15 +594,15 @@ static int sja1105_parse_rgmii_delays(struct sja1105_private *priv,
>         int i;
>
>         for (i = 0; i < SJA1105_NUM_PORTS; i++) {
> -               if (ports->role == XMII_MAC)
> +               if (ports[i].role == XMII_MAC)
>                         continue;
>
> -               if (ports->phy_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
> -                   ports->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
> +               if (ports[i].phy_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
> +                   ports[i].phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
>                         priv->rgmii_rx_delay[i] = true;
>
> -               if (ports->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID ||
> -                   ports->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
> +               if (ports[i].phy_mode == PHY_INTERFACE_MODE_RGMII_TXID ||
> +                   ports[i].phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
>                         priv->rgmii_tx_delay[i] = true;
>
>                 if ((priv->rgmii_rx_delay[i] || priv->rgmii_tx_delay[i]) &&
> --
> 2.24.0
>

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

[Vladimir Oltean]

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

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

[Vladimir Oltean]

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>
> ---

Also there are some typos which should be corrected:
probet -> probed
every thing -> everything
filed -> failed

"failed for some reason" -> "was deferred"

>  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
>

Thanks,
-Vladimir

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

[Oleksij Rempel]

On 25.11.19 11:22, Vladimir Oltean wrote:
> 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>
>> ---
> 
> Also there are some typos which should be corrected:
> probet -> probed
> every thing -> everything
> filed -> failed
> 
> "failed for some reason" -> "was deferred"

Ok, thx.

should i resend both patches separately or only this one with spell fixes?

> 
>>   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
>>
> 
> Thanks,
> -Vladimir
> 

Kind regards,
Oleksij Rempel

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Vladimir Oltean]

On Mon, 25 Nov 2019 at 12:32, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> On 25.11.19 11:22, Vladimir Oltean wrote:
> > 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>
> >> ---
> >
> > Also there are some typos which should be corrected:
> > probet -> probed
> > every thing -> everything
> > filed -> failed
> >
> > "failed for some reason" -> "was deferred"
>
> Ok, thx.
>
> should i resend both patches separately or only this one with spell fixes?
>

I don't know if David/Jakub like applying partial series (just 2/2). I
would send a v2 to each patch specifying the tree clearly.
Also I think I would just move the "Probed...." message somewhere at
the beginning of sja1105_setup, where no probe deferral can happen.

>
> Kind regards,
> Oleksij Rempel
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Thanks,
-Vladimir

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

[Oleksij Rempel]

On 25.11.19 11:39, Vladimir Oltean wrote:
> On Mon, 25 Nov 2019 at 12:32, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>> On 25.11.19 11:22, Vladimir Oltean wrote:
>>> 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>
>>>> ---
>>>
>>> Also there are some typos which should be corrected:
>>> probet -> probed
>>> every thing -> everything
>>> filed -> failed
>>>
>>> "failed for some reason" -> "was deferred"
>>
>> Ok, thx.
>>
>> should i resend both patches separately or only this one with spell fixes?
>>
> 
> I don't know if David/Jakub like applying partial series (just 2/2). I
> would send a v2 to each patch specifying the tree clearly.
> Also I think I would just move the "Probed...." message somewhere at
> the beginning of sja1105_setup, where no probe deferral can happen.

Ok, I'll drop the second patch by now, currently it is not critical issue for me.

Kind regards,
Oleksij Rempel

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Andrew Lunn]

On Mon, Nov 25, 2019 at 11:02:58AM +0100, Oleksij Rempel 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>

For net-next, this is O.K.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Andrew Lunn]

> 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.

There is a rather unfortunate chicken/egg problem here for any switch
using MDIO. At the moment i don't know how to solve it. As a result
the first probe is pretty much guaranteed to return -EPROBE_DEFER. The
problem is that the MAC driver registers its MDIO bus. That causes the
DT to be walked for the bus and the switch probed. The switch probe
registers the switch with the DSA core. It then tries to get a handle
on the master device. But the MAC driver has not called
netdev_register() yet, it is busy registering its MDIO bus. So the
master device is not there, and so we get a -EPROBE_DEFER and the
switch driver needs to unwind.

So for an MDIO switch, i suggest the probe it kept to a minimum, and
all the real work is done in the setup callback. Setup is called when
all resources the DSA core needs are available.

I've got a patch somewhere for mv88e6xxx which does this move, and it
cut boot time by a noticeable amount.

    Andrew