diff mbox series

[net-next,v3,4/6] net: ravb: Move the update of ndev->features to ravb_set_features()

Message ID 20240213094110.853155-5-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: ravb: Add runtime PM support (part 2) | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 989 this patch: 989
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 1006 this patch: 1006
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1006 this patch: 1006
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 47 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-02-13--21-00 (tests: 1437)

Commit Message

Claudiu Feb. 13, 2024, 9:41 a.m. UTC 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Commit c2da9408579d ("ravb: Add Rx checksum offload support for GbEth")
introduced support for setting GbEth features. With this the IP-specific
features update functions update the ndev->features individually.

Next commits add runtime PM support for the ravb driver. The runtime PM
implementation will enable/disable the IP clocks on
the ravb_open()/ravb_close() functions. Accessing the IP registers with
clocks disabled blocks the system.

The ravb_set_features() function could be executed when the Ethernet
interface is closed so we need to ensure we don't access IP registers while
the interface is down when runtime PM support will be in place.

For these, move the update of ndev->features to ravb_set_features() and
make the IP-specific features set function return int. In this way we
update the ndev->features only when the IP-specific features set function
returns success and we can avoid code duplication when introducing
runtime PM registers protection.

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

Changes in v3:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb_main.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Sergey Shtylyov Feb. 13, 2024, 7:36 p.m. UTC | #1 
On 2/13/24 12:41 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Commit c2da9408579d ("ravb: Add Rx checksum offload support for GbEth")
> introduced support for setting GbEth features. With this the IP-specific
> features update functions update the ndev->features individually.
> 
> Next commits add runtime PM support for the ravb driver. The runtime PM
> implementation will enable/disable the IP clocks on
> the ravb_open()/ravb_close() functions. Accessing the IP registers with
> clocks disabled blocks the system.
> 
> The ravb_set_features() function could be executed when the Ethernet
> interface is closed so we need to ensure we don't access IP registers while
> the interface is down when runtime PM support will be in place.
> 
> For these, move the update of ndev->features to ravb_set_features() and
> make the IP-specific features set function return int. In this way we
> update the ndev->features only when the IP-specific features set function
> returns success and we can avoid code duplication when introducing
> runtime PM registers protection.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey
Sergey Shtylyov Feb. 13, 2024, 7:52 p.m. UTC | #2 
On 2/13/24 12:41 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Commit c2da9408579d ("ravb: Add Rx checksum offload support for GbEth")
> introduced support for setting GbEth features. With this the IP-specific
> features update functions update the ndev->features individually.
> 
> Next commits add runtime PM support for the ravb driver. The runtime PM
> implementation will enable/disable the IP clocks on
> the ravb_open()/ravb_close() functions. Accessing the IP registers with
> clocks disabled blocks the system.
> 
> The ravb_set_features() function could be executed when the Ethernet
> interface is closed so we need to ensure we don't access IP registers while
> the interface is down when runtime PM support will be in place.
> 
> For these, move the update of ndev->features to ravb_set_features() and
> make the IP-specific features set function return int. In this way we
> update the ndev->features only when the IP-specific features set function
> returns success and we can avoid code duplication when introducing
> runtime PM registers protection.
> 
> 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 7a7f743a1fef..b3b91783bb7a 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2475,7 +2475,7 @@ static int ravb_change_mtu(struct net_device *ndev, int new_mtu)
>  	return 0;
>  }
>  
> -static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
> +static int ravb_set_rx_csum(struct net_device *ndev, bool enable)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	unsigned long flags;
> @@ -2492,6 +2492,8 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
>  	ravb_rcv_snd_enable(ndev);
>  
>  	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	return 0;
>  }
>  
>  static int ravb_endisable_csum_gbeth(struct net_device *ndev, enum ravb_reg reg,

   Wait! You're not updating the call site of ravb_set_rx_csum(), are you?
It looks like the above 2 hunks aren't needed...

[...]

MBR, Sergey
Sergey Shtylyov Feb. 13, 2024, 7:58 p.m. UTC | #3 
On 2/13/24 10:36 PM, Sergey Shtylyov wrote:
[...]

>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Commit c2da9408579d ("ravb: Add Rx checksum offload support for GbEth")
>> introduced support for setting GbEth features. With this the IP-specific
>> features update functions update the ndev->features individually.
>>
>> Next commits add runtime PM support for the ravb driver. The runtime PM
>> implementation will enable/disable the IP clocks on
>> the ravb_open()/ravb_close() functions. Accessing the IP registers with
>> clocks disabled blocks the system.
>>
>> The ravb_set_features() function could be executed when the Ethernet
>> interface is closed so we need to ensure we don't access IP registers while
>> the interface is down when runtime PM support will be in place.
>>
>> For these, move the update of ndev->features to ravb_set_features() and
>> make the IP-specific features set function return int. In this way we
>> update the ndev->features only when the IP-specific features set function
>> returns success and we can avoid code duplication when introducing
>> runtime PM registers protection.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

   Have to withdraw this... :-/

[...]

MBR, Sergey
Claudiu Feb. 14, 2024, 5:45 a.m. UTC | #4 
On 13.02.2024 21:52, Sergey Shtylyov wrote:
> On 2/13/24 12:41 PM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Commit c2da9408579d ("ravb: Add Rx checksum offload support for GbEth")
>> introduced support for setting GbEth features. With this the IP-specific
>> features update functions update the ndev->features individually.
>>
>> Next commits add runtime PM support for the ravb driver. The runtime PM
>> implementation will enable/disable the IP clocks on
>> the ravb_open()/ravb_close() functions. Accessing the IP registers with
>> clocks disabled blocks the system.
>>
>> The ravb_set_features() function could be executed when the Ethernet
>> interface is closed so we need to ensure we don't access IP registers while
>> the interface is down when runtime PM support will be in place.
>>
>> For these, move the update of ndev->features to ravb_set_features() and
>> make the IP-specific features set function return int. In this way we
>> update the ndev->features only when the IP-specific features set function
>> returns success and we can avoid code duplication when introducing
>> runtime PM registers protection.
>>
>> 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 7a7f743a1fef..b3b91783bb7a 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2475,7 +2475,7 @@ static int ravb_change_mtu(struct net_device *ndev, int new_mtu)
>>  	return 0;
>>  }
>>  
>> -static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
>> +static int ravb_set_rx_csum(struct net_device *ndev, bool enable)
>>  {
>>  	struct ravb_private *priv = netdev_priv(ndev);
>>  	unsigned long flags;
>> @@ -2492,6 +2492,8 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
>>  	ravb_rcv_snd_enable(ndev);
>>  
>>  	spin_unlock_irqrestore(&priv->lock, flags);
>> +
>> +	return 0;
>>  }
>>  
>>  static int ravb_endisable_csum_gbeth(struct net_device *ndev, enum ravb_reg reg,
> 
>    Wait! You're not updating the call site of ravb_set_rx_csum(), are you?
> It looks like the above 2 hunks aren't needed...

A, you're right. I'll update it in the next version.

Thank you,
Claudiu Beznea

> 
> [...]
> 
> 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 7a7f743a1fef..b3b91783bb7a 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2475,7 +2475,7 @@  static int ravb_change_mtu(struct net_device *ndev, int new_mtu)
 	return 0;
 }
 
-static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
+static int ravb_set_rx_csum(struct net_device *ndev, bool enable)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	unsigned long flags;
@@ -2492,6 +2492,8 @@  static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
 	ravb_rcv_snd_enable(ndev);
 
 	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return 0;
 }
 
 static int ravb_endisable_csum_gbeth(struct net_device *ndev, enum ravb_reg reg,
@@ -2542,7 +2544,6 @@  static int ravb_set_features_gbeth(struct net_device *ndev,
 			goto done;
 	}
 
-	ndev->features = features;
 done:
 	spin_unlock_irqrestore(&priv->lock, flags);
 
@@ -2557,8 +2558,6 @@  static int ravb_set_features_rcar(struct net_device *ndev,
 	if (changed & NETIF_F_RXCSUM)
 		ravb_set_rx_csum(ndev, features & NETIF_F_RXCSUM);
 
-	ndev->features = features;
-
 	return 0;
 }
 
@@ -2567,8 +2566,15 @@  static int ravb_set_features(struct net_device *ndev,
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
+	int ret;
 
-	return info->set_feature(ndev, features);
+	ret = info->set_feature(ndev, features);
+	if (ret)
+		return ret;
+
+	ndev->features = features;
+
+	return 0;
 }
 
 static const struct net_device_ops ravb_netdev_ops = {