diff mbox series

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

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

Commit Message

Claudiu Feb. 14, 2024, 1:57 p.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(). 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 v4:
- remove unnecessary code
- updated the patch description

Changes in v3:
- none; this patch is new

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

Comments

Sergey Shtylyov Feb. 14, 2024, 4:55 p.m. UTC | #1 
On 2/14/24 4:57 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(). 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>

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 7a7f743a1fef..ac23779d1cc5 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -2567,8 +2564,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;

   I'd use 'error' here, it would fit well... :-)

> +
> +	ret = info->set_feature(ndev, features);
> +	if (ret)
> +		return ret;
>  
> -	return info->set_feature(ndev, features);
> +	ndev->features = features;
> +
> +	return 0;
>  }
>  
>  static const struct net_device_ops ravb_netdev_ops = {

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..ac23779d1cc5 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2542,7 +2542,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 +2556,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 +2564,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;
+
+	ret = info->set_feature(ndev, features);
+	if (ret)
+		return ret;
 
-	return info->set_feature(ndev, features);
+	ndev->features = features;
+
+	return 0;
 }
 
 static const struct net_device_ops ravb_netdev_ops = {