diff mbox series

[net-next,v2,20/21] net: ravb: Do not apply RX CSUM settings to hardware if the interface is down

Message ID 20231214114600.2451162-21-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S | expand

Commit Message

Claudiu Dec. 14, 2023, 11:45 a.m. UTC 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Do not apply the RX CSUM settings to hardware if the interface is down. In
case runtime PM is enabled, and while the interface is down, the IP will be
in reset mode (as for some platforms disabling/enabling the clocks will
switch the IP to standby mode, which will lead to losing registers'
content) and applying settings in reset mode is not an option. Instead,
cache the RX CSUM settings and apply them in ravb_open().

Commit prepares for the addition of runtime PM.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb_main.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Sergey Shtylyov Dec. 16, 2023, 8:36 p.m. UTC | #1 
On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Do not apply the RX CSUM settings to hardware if the interface is down. In
> case runtime PM is enabled, and while the interface is down, the IP will be
> in reset mode (as for some platforms disabling/enabling the clocks will
> switch the IP to standby mode, which will lead to losing registers'

   To/From perhaps?
   And just "register".

> content) and applying settings in reset mode is not an option. Instead,
> cache the RX CSUM settings and apply them in ravb_open().

   Have this issue actually occurred for you?

> Commit prepares for the addition of runtime PM.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 633346b6cd7c..9ff943dff522 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1868,6 +1868,15 @@ static int ravb_open(struct net_device *ndev)
>  	if (info->gptp || info->ccc_gac)
>  		ravb_ptp_init(ndev, priv->pdev);
>  
> +	/* Apply features that might have been changed while the interface
> +	 * was down.
> +	 */
> +	if (ndev->hw_features & NETIF_F_RXCSUM) {

   I'm afraid this is a wrong field; we need ndev->features, no?

> +		u32 val = (ndev->features & NETIF_F_RXCSUM) ? ECMR_RCSC : 0;
> +
> +		ravb_modify(ndev, ECMR, ECMR_RCSC, val);
> +	}
> +

   The ECMR setting is already done in ravb_emac_init_rcar(), no need
to duplicate it here, I think...

>  	/* PHY control start */
>  	error = ravb_phy_start(ndev);
>  	if (error)
> @@ -2337,6 +2346,9 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	unsigned long flags;
>  
> +	if (!netif_running(ndev))

   Racy as well...

> +		return;
> +

   Hm, sh_eth.c doesn't have such check -- perhaps should be fixed
as well...

[...]

MBR, Sergey
Claudiu Dec. 17, 2023, 2:34 p.m. UTC | #2 
On 16.12.2023 22:36, Sergey Shtylyov wrote:
> On 12/14/23 2:45 PM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Do not apply the RX CSUM settings to hardware if the interface is down. In
>> case runtime PM is enabled, and while the interface is down, the IP will be
>> in reset mode (as for some platforms disabling/enabling the clocks will
>> switch the IP to standby mode, which will lead to losing registers'
> 
>    To/From perhaps?
>    And just "register".
> 
>> content) and applying settings in reset mode is not an option. Instead,
>> cache the RX CSUM settings and apply them in ravb_open().
> 
>    Have this issue actually occurred for you?

Setting RX CSUM while the if is down? No.

> 
>> Commit prepares for the addition of runtime PM.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> [...]
> 
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 633346b6cd7c..9ff943dff522 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -1868,6 +1868,15 @@ static int ravb_open(struct net_device *ndev)
>>  	if (info->gptp || info->ccc_gac)
>>  		ravb_ptp_init(ndev, priv->pdev);
>>  
>> +	/* Apply features that might have been changed while the interface
>> +	 * was down.
>> +	 */
>> +	if (ndev->hw_features & NETIF_F_RXCSUM) {
> 
>    I'm afraid this is a wrong field; we need ndev->features, no?

RX CSUM is not enabled for all ravb aware devices (see struct
ravb_hw_info::net_hw_features). We should be setting the ECMR only for
these ones. ravb_hw_info::net_hw_features is set in ndev->hw_features in
probe(). So here code checks if platforms supports RXCSUM and then below it
applies what has been requested though ndo_set_features(), if any.

> 
>> +		u32 val = (ndev->features & NETIF_F_RXCSUM) ? ECMR_RCSC : 0;
>> +
>> +		ravb_modify(ndev, ECMR, ECMR_RCSC, val);
>> +	}
>> +
> 
>    The ECMR setting is already done in ravb_emac_init_rcar(), no need
> to duplicate it here, I think...

Ok, it worth being moved there.

> 
>>  	/* PHY control start */
>>  	error = ravb_phy_start(ndev);
>>  	if (error)
>> @@ -2337,6 +2346,9 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
>>  	struct ravb_private *priv = netdev_priv(ndev);
>>  	unsigned long flags;
>>  
>> +	if (!netif_running(ndev))
> 
>    Racy as well...

It's also called with rtnl_mutex locked.

> 
>> +		return;
>> +
> 
>    Hm, sh_eth.c doesn't have such check -- perhaps should be fixed
> as well...
> 
> [...]
> 
> MBR, Sergey
Sergey Shtylyov Dec. 21, 2023, 6:50 p.m. UTC | #3 
On 12/17/23 5:34 PM, claudiu beznea wrote:

[...]

>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> Do not apply the RX CSUM settings to hardware if the interface is down. In
>>> case runtime PM is enabled, and while the interface is down, the IP will be
>>> in reset mode (as for some platforms disabling/enabling the clocks will
>>> switch the IP to standby mode, which will lead to losing registers'
>>
>>    To/From perhaps?
>>    And just "register".
>>
>>> content) and applying settings in reset mode is not an option. Instead,
>>> cache the RX CSUM settings and apply them in ravb_open().
>>
>>    Have this issue actually occurred for you?
> 
> Setting RX CSUM while the if is down? No.
> 
>>> Commit prepares for the addition of runtime PM.
>>>
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> [...]
>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 633346b6cd7c..9ff943dff522 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -1868,6 +1868,15 @@ static int ravb_open(struct net_device *ndev)
>>>  	if (info->gptp || info->ccc_gac)
>>>  		ravb_ptp_init(ndev, priv->pdev);
>>>  
>>> +	/* Apply features that might have been changed while the interface
>>> +	 * was down.
>>> +	 */
>>> +	if (ndev->hw_features & NETIF_F_RXCSUM) {
>>
>>    I'm afraid this is a wrong field; we need ndev->features, no?
> 
> RX CSUM is not enabled for all ravb aware devices (see struct
> ravb_hw_info::net_hw_features). We should be setting the ECMR only for
> these ones. ravb_hw_info::net_hw_features is set in ndev->hw_features in
> probe(). So here code checks if platforms supports RXCSUM and then below it
> applies what has been requested though ndo_set_features(), if any.

  OK. But we don't need this snippet here anyway...

>>> +		u32 val = (ndev->features & NETIF_F_RXCSUM) ? ECMR_RCSC : 0;
>>> +
>>> +		ravb_modify(ndev, ECMR, ECMR_RCSC, val);
>>> +	}
>>> +
>>
>>    The ECMR setting is already done in ravb_emac_init_rcar(), no need
>> to duplicate it here, I think...
> 
> Ok, it worth being moved there.

   No need to move, it's already there...

[...]
>>> @@ -2337,6 +2346,9 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
>>>  	struct ravb_private *priv = netdev_priv(ndev);
>>>  	unsigned long flags;
>>>  
>>> +	if (!netif_running(ndev))
>>
>>    Racy as well...
> 
> It's also called with rtnl_mutex locked.

   Well, at least the only place that calls the ndo_set_features()
method, __netdev_update_features() calls ASSERT_RTNL() at the start.
However, since we'd have to use the is_opened flag anyway, let's rely
on it instead...

[...]

MBR, Sergey
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 633346b6cd7c..9ff943dff522 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1868,6 +1868,15 @@  static int ravb_open(struct net_device *ndev)
 	if (info->gptp || info->ccc_gac)
 		ravb_ptp_init(ndev, priv->pdev);
 
+	/* Apply features that might have been changed while the interface
+	 * was down.
+	 */
+	if (ndev->hw_features & NETIF_F_RXCSUM) {
+		u32 val = (ndev->features & NETIF_F_RXCSUM) ? ECMR_RCSC : 0;
+
+		ravb_modify(ndev, ECMR, ECMR_RCSC, val);
+	}
+
 	/* PHY control start */
 	error = ravb_phy_start(ndev);
 	if (error)
@@ -2337,6 +2346,9 @@  static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
 	struct ravb_private *priv = netdev_priv(ndev);
 	unsigned long flags;
 
+	if (!netif_running(ndev))
+		return;
+
 	spin_lock_irqsave(&priv->lock, flags);
 
 	/* Disable TX and RX */