Re: [PATCH 3/6] ravb: remove custom .set_link_ksettings from ethtool ops

[Vladimir Zapolskiy]

Hello Sergei,

On 05/26/2018 10:50 PM, Sergei Shtylyov wrote:
> On 05/24/2018 02:11 PM, Vladimir Zapolskiy wrote:
> 
>> The change replaces a custom implementation of .set_link_ksettings
>> callback with a shared phy_ethtool_set_link_ksettings(), this fixes
>> sleep in atomic context bug, which is encountered every time when link
>> settings are changed by ethtool.
> 
>Seeing it now...
> 
>> Now duplex mode setting is enforced in ravb_adjust_link() only, also
>> now TX/RX is disabled when link is put down or modifications to E-MAC
>> registers ECMR and GECMR are expected for both cases of checked and
>> ignored link status pin state from E-MAC interrupt handler.
>>
>> Signed-off-by: Vladimir Zapolskiy 
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 58 
>> +---
>>  1 file changed, 15 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
>> b/drivers/net/ethernet/renesas/ravb_main.c
>> index 3d91caa44176..0d811c02ff34 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -980,6 +980,13 @@ static void ravb_adjust_link(struct net_device *ndev)
>>  struct ravb_private *priv = netdev_priv(ndev);
>>  struct phy_device *phydev = ndev->phydev;
>>  bool new_state = false;
>> +unsigned long flags;
>> +
>> +spin_lock_irqsave(>lock, flags);
>> +
>> +/* Disable TX and RX right over here, if E-MAC change is ignored */
>> +if (priv->no_avb_link)
>> +ravb_rcv_snd_disable(ndev);
>>  
>>  if (phydev->link) {
>>  if (phydev->duplex != priv->duplex) {
>> @@ -997,18 +1004,21 @@ static void ravb_adjust_link(struct net_device *ndev)
>>  ravb_modify(ndev, ECMR, ECMR_TXF, 0);
>>  new_state = true;
>>  priv->link = phydev->link;
>> -if (priv->no_avb_link)
>> -ravb_rcv_snd_enable(ndev);
>>  }
>>  } else if (priv->link) {
>>  new_state = true;
>>  priv->link = 0;
>>  priv->speed = 0;
>>  priv->duplex = -1;
>> -if (priv->no_avb_link)
>> -ravb_rcv_snd_disable(ndev);
>>  }
>>  
>> +/* Enable TX and RX right over here, if E-MAC change is ignored */
>> +if (priv->no_avb_link && phydev->link)
>> +ravb_rcv_snd_enable(ndev);
>> +
>> +mmiowb();
>> +spin_unlock_irqrestore(>lock, flags);
>> +
> 
>I like this part. :-)
> 

A weight off my mind :) And I hope that this change will remain the less
questionable one, other ones from the series are trivial.

Anyway I hope it is understandable that this part of the change can not
be simply extracted from the rest one below, otherwise there'll be bugs of
another type intorduced.

>>  if (new_state && netif_msg_link(priv))
>>  phy_print_status(phydev);
>>  }
>> @@ -1096,44 +1106,6 @@ static int ravb_phy_start(struct net_device *ndev)
>>  return 0;
>>  }
>>  
>> -static int ravb_set_link_ksettings(struct net_device *ndev,
>> -   const struct ethtool_link_ksettings *cmd)
>> -{
>> -struct ravb_private *priv = netdev_priv(ndev);
>> -unsigned long flags;
>> -int error;
>> -
>> -if (!ndev->phydev)
>> -return -ENODEV;
>> -
>> -spin_lock_irqsave(>lock, flags);
>> -
>> -/* Disable TX and RX */
>> -ravb_rcv_snd_disable(ndev);
>> -
>> -error = phy_ethtool_ksettings_set(ndev->phydev, cmd);
>> -if (error)
>> -goto error_exit;
>> -
>> -if (cmd->base.duplex == DUPLEX_FULL)
>> -priv->duplex = 1;
>> -else
>> -priv->duplex = 0;
>> -
>> -ravb_set_duplex(ndev);
>> -
>> -error_exit:
>> -mdelay(1);
>> -
>> -/* Enable TX and RX */
>> -ravb_rcv_snd_enable(ndev);
>> -
>> -mmiowb();
>> -spin_unlock_irqrestore(>lock, flags);
>> -
>> -return error;
>> -}
>> -
> 
>But this part is clearly lumping it all together... 

Please elaborate.

> 
> [...]
>> @@ -1357,7 +1329,7 @@ static const struct ethtool_ops ravb_ethtool_ops = {
>>  .set_ringparam  = ravb_set_ringparam,
>>  .get_ts_info= ravb_get_ts_info,
>>  .get_link_ksettings = phy_ethtool_get_link_ksettings,
>> -.set_link_ksettings = ravb_set_link_ksettings,
>> +.set_link_ksettings = phy_ethtool_set_link_ksettings,
> 
>Should have been a part of the final patch in the fix/enhancement chain...

Please elaborate.

Do you mean that firstly I have to make erroneous ravb_set_link_ksettings()
to look similar to phy_ethtool_set_link_ksettings() and then remove it?

As I see it in the current context (removal of ravb_set_duplex() call and
so on), the problem with this approach is that the actual fix change will
be done on top of a number of enchancement changes, thus it contradicts to
the accepted 

Re: [PATCH 3/6] ravb: remove custom .set_link_ksettings from ethtool ops

[Sergei Shtylyov]

On 05/24/2018 02:11 PM, Vladimir Zapolskiy wrote:

> The change replaces a custom implementation of .set_link_ksettings
> callback with a shared phy_ethtool_set_link_ksettings(), this fixes
> sleep in atomic context bug, which is encountered every time when link
> settings are changed by ethtool.

   Seeing it now...

> Now duplex mode setting is enforced in ravb_adjust_link() only, also
> now TX/RX is disabled when link is put down or modifications to E-MAC
> registers ECMR and GECMR are expected for both cases of checked and
> ignored link status pin state from E-MAC interrupt handler.
> 
> Signed-off-by: Vladimir Zapolskiy 
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 58 
> +---
>  1 file changed, 15 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
> b/drivers/net/ethernet/renesas/ravb_main.c
> index 3d91caa44176..0d811c02ff34 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -980,6 +980,13 @@ static void ravb_adjust_link(struct net_device *ndev)
>   struct ravb_private *priv = netdev_priv(ndev);
>   struct phy_device *phydev = ndev->phydev;
>   bool new_state = false;
> + unsigned long flags;
> +
> + spin_lock_irqsave(>lock, flags);
> +
> + /* Disable TX and RX right over here, if E-MAC change is ignored */
> + if (priv->no_avb_link)
> + ravb_rcv_snd_disable(ndev);
>  
>   if (phydev->link) {
>   if (phydev->duplex != priv->duplex) {
> @@ -997,18 +1004,21 @@ static void ravb_adjust_link(struct net_device *ndev)
>   ravb_modify(ndev, ECMR, ECMR_TXF, 0);
>   new_state = true;
>   priv->link = phydev->link;
> - if (priv->no_avb_link)
> - ravb_rcv_snd_enable(ndev);
>   }
>   } else if (priv->link) {
>   new_state = true;
>   priv->link = 0;
>   priv->speed = 0;
>   priv->duplex = -1;
> - if (priv->no_avb_link)
> - ravb_rcv_snd_disable(ndev);
>   }
>  
> + /* Enable TX and RX right over here, if E-MAC change is ignored */
> + if (priv->no_avb_link && phydev->link)
> + ravb_rcv_snd_enable(ndev);
> +
> + mmiowb();
> + spin_unlock_irqrestore(>lock, flags);
> +

   I like this part. :-)

>   if (new_state && netif_msg_link(priv))
>   phy_print_status(phydev);
>  }
> @@ -1096,44 +1106,6 @@ static int ravb_phy_start(struct net_device *ndev)
>   return 0;
>  }
>  
> -static int ravb_set_link_ksettings(struct net_device *ndev,
> -const struct ethtool_link_ksettings *cmd)
> -{
> - struct ravb_private *priv = netdev_priv(ndev);
> - unsigned long flags;
> - int error;
> -
> - if (!ndev->phydev)
> - return -ENODEV;
> -
> - spin_lock_irqsave(>lock, flags);
> -
> - /* Disable TX and RX */
> - ravb_rcv_snd_disable(ndev);
> -
> - error = phy_ethtool_ksettings_set(ndev->phydev, cmd);
> - if (error)
> - goto error_exit;
> -
> - if (cmd->base.duplex == DUPLEX_FULL)
> - priv->duplex = 1;
> - else
> - priv->duplex = 0;
> -
> - ravb_set_duplex(ndev);
> -
> -error_exit:
> - mdelay(1);
> -
> - /* Enable TX and RX */
> - ravb_rcv_snd_enable(ndev);
> -
> - mmiowb();
> - spin_unlock_irqrestore(>lock, flags);
> -
> - return error;
> -}
> -

   But this part is clearly lumping it all together... 

[...]
> @@ -1357,7 +1329,7 @@ static const struct ethtool_ops ravb_ethtool_ops = {
>   .set_ringparam  = ravb_set_ringparam,
>   .get_ts_info= ravb_get_ts_info,
>   .get_link_ksettings = phy_ethtool_get_link_ksettings,
> - .set_link_ksettings = ravb_set_link_ksettings,
> + .set_link_ksettings = phy_ethtool_set_link_ksettings,

   Should have been a part of the final patch in the fix/enhancement chain...

>   .get_wol= ravb_get_wol,
>   .set_wol= ravb_set_wol,
>  };

MBR, Sergei

Re: [PATCH 3/6] ravb: remove custom .set_link_ksettings from ethtool ops

[Vladimir Zapolskiy]

Hi Andrew,

On 05/24/2018 04:29 PM, Andrew Lunn wrote:
> On Thu, May 24, 2018 at 02:11:55PM +0300, Vladimir Zapolskiy wrote:
>> The change replaces a custom implementation of .set_link_ksettings
>> callback with a shared phy_ethtool_set_link_ksettings(), this fixes
>> sleep in atomic context bug, which is encountered every time when link
>> settings are changed by ethtool.
>>
>> Now duplex mode setting is enforced in ravb_adjust_link() only, also
>> now TX/RX is disabled when link is put down or modifications to E-MAC
>> registers ECMR and GECMR are expected for both cases of checked and
>> ignored link status pin state from E-MAC interrupt handler.
>>
>> Signed-off-by: Vladimir Zapolskiy 
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 58 
>> +---
>>  1 file changed, 15 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
>> b/drivers/net/ethernet/renesas/ravb_main.c
>> index 3d91caa44176..0d811c02ff34 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -980,6 +980,13 @@ static void ravb_adjust_link(struct net_device *ndev)
>>  struct ravb_private *priv = netdev_priv(ndev);
>>  struct phy_device *phydev = ndev->phydev;
>>  bool new_state = false;
>> +unsigned long flags;
>> +
>> +spin_lock_irqsave(>lock, flags);
> 
> Hi Vladimir
> 
> It is pretty unusual to see an adjust_link callback take a lock. Is it
> clearly defined what it is protecting?
> 

thank you for review.

As the commit message says, the hardware manual claims that
any modifications to ECMR and GECMR registers, i.e. calls to
ravb_set_duplex() and ravb_set_rate() from the modified
ravb_adjust_link() function, has to be done when RX/TX is
disabled (same ECMR register bit fields), the spinlock
serializes interrupt handlers and modifications to ECMR contents,
its previous usage for ethtool handlers was obviously wrong.

The information is quite implicit, but the change emphasizes
it.

--
With best wishes,
Vladimir

Re: [PATCH 3/6] ravb: remove custom .set_link_ksettings from ethtool ops

[Andrew Lunn]

On Thu, May 24, 2018 at 02:11:55PM +0300, Vladimir Zapolskiy wrote:
> The change replaces a custom implementation of .set_link_ksettings
> callback with a shared phy_ethtool_set_link_ksettings(), this fixes
> sleep in atomic context bug, which is encountered every time when link
> settings are changed by ethtool.
> 
> Now duplex mode setting is enforced in ravb_adjust_link() only, also
> now TX/RX is disabled when link is put down or modifications to E-MAC
> registers ECMR and GECMR are expected for both cases of checked and
> ignored link status pin state from E-MAC interrupt handler.
> 
> Signed-off-by: Vladimir Zapolskiy 
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 58 
> +---
>  1 file changed, 15 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
> b/drivers/net/ethernet/renesas/ravb_main.c
> index 3d91caa44176..0d811c02ff34 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -980,6 +980,13 @@ static void ravb_adjust_link(struct net_device *ndev)
>   struct ravb_private *priv = netdev_priv(ndev);
>   struct phy_device *phydev = ndev->phydev;
>   bool new_state = false;
> + unsigned long flags;
> +
> + spin_lock_irqsave(>lock, flags);

Hi Vladimir

It is pretty unusual to see an adjust_link callback take a lock. Is it
clearly defined what it is protecting?

Andrew

[PATCH 3/6] ravb: remove custom .set_link_ksettings from ethtool ops

[Vladimir Zapolskiy]

The change replaces a custom implementation of .set_link_ksettings
callback with a shared phy_ethtool_set_link_ksettings(), this fixes
sleep in atomic context bug, which is encountered every time when link
settings are changed by ethtool.

Now duplex mode setting is enforced in ravb_adjust_link() only, also
now TX/RX is disabled when link is put down or modifications to E-MAC
registers ECMR and GECMR are expected for both cases of checked and
ignored link status pin state from E-MAC interrupt handler.

Signed-off-by: Vladimir Zapolskiy 
---
 drivers/net/ethernet/renesas/ravb_main.c | 58 +---
 1 file changed, 15 insertions(+), 43 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c 
b/drivers/net/ethernet/renesas/ravb_main.c
index 3d91caa44176..0d811c02ff34 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -980,6 +980,13 @@ static void ravb_adjust_link(struct net_device *ndev)
struct ravb_private *priv = netdev_priv(ndev);
struct phy_device *phydev = ndev->phydev;
bool new_state = false;
+   unsigned long flags;
+
+   spin_lock_irqsave(>lock, flags);
+
+   /* Disable TX and RX right over here, if E-MAC change is ignored */
+   if (priv->no_avb_link)
+   ravb_rcv_snd_disable(ndev);
 
if (phydev->link) {
if (phydev->duplex != priv->duplex) {
@@ -997,18 +1004,21 @@ static void ravb_adjust_link(struct net_device *ndev)
ravb_modify(ndev, ECMR, ECMR_TXF, 0);
new_state = true;
priv->link = phydev->link;
-   if (priv->no_avb_link)
-   ravb_rcv_snd_enable(ndev);
}
} else if (priv->link) {
new_state = true;
priv->link = 0;
priv->speed = 0;
priv->duplex = -1;
-   if (priv->no_avb_link)
-   ravb_rcv_snd_disable(ndev);
}
 
+   /* Enable TX and RX right over here, if E-MAC change is ignored */
+   if (priv->no_avb_link && phydev->link)
+   ravb_rcv_snd_enable(ndev);
+
+   mmiowb();
+   spin_unlock_irqrestore(>lock, flags);
+
if (new_state && netif_msg_link(priv))
phy_print_status(phydev);
 }
@@ -1096,44 +1106,6 @@ static int ravb_phy_start(struct net_device *ndev)
return 0;
 }
 
-static int ravb_set_link_ksettings(struct net_device *ndev,
-  const struct ethtool_link_ksettings *cmd)
-{
-   struct ravb_private *priv = netdev_priv(ndev);
-   unsigned long flags;
-   int error;
-
-   if (!ndev->phydev)
-   return -ENODEV;
-
-   spin_lock_irqsave(>lock, flags);
-
-   /* Disable TX and RX */
-   ravb_rcv_snd_disable(ndev);
-
-   error = phy_ethtool_ksettings_set(ndev->phydev, cmd);
-   if (error)
-   goto error_exit;
-
-   if (cmd->base.duplex == DUPLEX_FULL)
-   priv->duplex = 1;
-   else
-   priv->duplex = 0;
-
-   ravb_set_duplex(ndev);
-
-error_exit:
-   mdelay(1);
-
-   /* Enable TX and RX */
-   ravb_rcv_snd_enable(ndev);
-
-   mmiowb();
-   spin_unlock_irqrestore(>lock, flags);
-
-   return error;
-}
-
 static u32 ravb_get_msglevel(struct net_device *ndev)
 {
struct ravb_private *priv = netdev_priv(ndev);
@@ -1357,7 +1329,7 @@ static const struct ethtool_ops ravb_ethtool_ops = {
.set_ringparam  = ravb_set_ringparam,
.get_ts_info= ravb_get_ts_info,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
-   .set_link_ksettings = ravb_set_link_ksettings,
+   .set_link_ksettings = phy_ethtool_set_link_ksettings,
.get_wol= ravb_get_wol,
.set_wol= ravb_set_wol,
 };
-- 
2.8.1