[net-next,v3,5/6] net: ravb: Do not apply features to hardware if the interface is down

  From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Do not apply features 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 the clocks will switch the IP to
reset mode, which will lead to losing register contents) and applying
settings in reset mode is not an option. Instead, cache the features and
apply them in ravb_open() through ravb_emac_init().

To avoid accessing the hardware while the interface is down
pm_runtime_active() check was introduced. Along with it the device runtime
PM usage counter has been incremented to avoid disabling the device clocks
while the check is in progress (if any).

Commit prepares for the addition of runtime PM.

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

Changes in v3:
- updated patch title and description
- updated patch content due to patch 4/6

Changes in v2:
- fixed typo in patch description
- adjusted ravb_set_features_gbeth(); didn't collect the Sergey's Rb
  tag due to this 

Changes since [2]:
- use pm_runtime_get_noresume() and pm_runtime_active() and updated the
  commit message to describe that
- fixed typos
- s/CSUM/checksum in patch title and description

Changes in v3 of [2]:
- this was patch 20/21 in v2
- fixed typos in patch description
- removed code from ravb_open()
- use ndev->flags & IFF_UP checks instead of netif_running()

Changes in v2 of [2]:
- none; this patch is new


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

Hi Claudiu,

> -----Original Message-----
> From: Claudiu <claudiu.beznea@tuxon.dev>
> Sent: Tuesday, February 13, 2024 9:41 AM
> Subject: [PATCH net-next v3 5/6] net: ravb: Do not apply features to
> hardware if the interface is down
> 
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Do not apply features 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 the clocks will switch the IP
> to reset mode, which will lead to losing register contents) and applying
> settings in reset mode is not an option. Instead, cache the features and
> apply them in ravb_open() through ravb_emac_init().
> 
> To avoid accessing the hardware while the interface is down
> pm_runtime_active() check was introduced. Along with it the device runtime
> PM usage counter has been incremented to avoid disabling the device clocks
> while the check is in progress (if any).
> 
> Commit prepares for the addition of runtime PM.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v3:
> - updated patch title and description
> - updated patch content due to patch 4/6
> 
> Changes in v2:
> - fixed typo in patch description
> - adjusted ravb_set_features_gbeth(); didn't collect the Sergey's Rb
>   tag due to this
> 
> Changes since [2]:
> - use pm_runtime_get_noresume() and pm_runtime_active() and updated the
>   commit message to describe that
> - fixed typos
> - s/CSUM/checksum in patch title and description
> 
> Changes in v3 of [2]:
> - this was patch 20/21 in v2
> - fixed typos in patch description
> - removed code from ravb_open()
> - use ndev->flags & IFF_UP checks instead of netif_running()
> 
> Changes in v2 of [2]:
> - none; this patch is new
> 
> 
>  drivers/net/ethernet/renesas/ravb_main.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> b/drivers/net/ethernet/renesas/ravb_main.c
> index b3b91783bb7a..4dd0520dea90 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2566,15 +2566,23 @@ 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;
> +	struct device *dev = &priv->pdev->dev;
> +	int ret = 0;
> +
> +	pm_runtime_get_noresume(dev);
> +
> +	if (!pm_runtime_active(dev))
> +		goto out_set_features;

This can be simplified, which avoids 1 goto statement and
Unnecessary ret initialization. I am leaving to you and Sergey.

	if (!pm_runtime_active(dev))
		ret = 0;
	else
		ret = info->set_feature(ndev, features);

	pm_runtime_put_noidle(dev);
	if (ret)
		goto err;

	ndev->features = features;

err:
	return ret;

Cheers,
Biju

> 
>  	ret = info->set_feature(ndev, features);
>  	if (ret)
> -		return ret;
> +		goto out_rpm_put;
> 
> +out_set_features:
>  	ndev->features = features;
> -
> -	return 0;
> +out_rpm_put:
> +	pm_runtime_put_noidle(dev);
> +	return ret;
>  }
> 
>  static const struct net_device_ops ravb_netdev_ops = {
> --
> 2.39.2
  

Hi, Biju,

On 13.02.2024 12:13, Biju Das wrote:
> 
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: Claudiu <claudiu.beznea@tuxon.dev>
>> Sent: Tuesday, February 13, 2024 9:41 AM
>> Subject: [PATCH net-next v3 5/6] net: ravb: Do not apply features to
>> hardware if the interface is down
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Do not apply features 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 the clocks will switch the IP
>> to reset mode, which will lead to losing register contents) and applying
>> settings in reset mode is not an option. Instead, cache the features and
>> apply them in ravb_open() through ravb_emac_init().
>>
>> To avoid accessing the hardware while the interface is down
>> pm_runtime_active() check was introduced. Along with it the device runtime
>> PM usage counter has been incremented to avoid disabling the device clocks
>> while the check is in progress (if any).
>>
>> Commit prepares for the addition of runtime PM.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v3:
>> - updated patch title and description
>> - updated patch content due to patch 4/6
>>
>> Changes in v2:
>> - fixed typo in patch description
>> - adjusted ravb_set_features_gbeth(); didn't collect the Sergey's Rb
>>   tag due to this
>>
>> Changes since [2]:
>> - use pm_runtime_get_noresume() and pm_runtime_active() and updated the
>>   commit message to describe that
>> - fixed typos
>> - s/CSUM/checksum in patch title and description
>>
>> Changes in v3 of [2]:
>> - this was patch 20/21 in v2
>> - fixed typos in patch description
>> - removed code from ravb_open()
>> - use ndev->flags & IFF_UP checks instead of netif_running()
>>
>> Changes in v2 of [2]:
>> - none; this patch is new
>>
>>
>>  drivers/net/ethernet/renesas/ravb_main.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>> b/drivers/net/ethernet/renesas/ravb_main.c
>> index b3b91783bb7a..4dd0520dea90 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2566,15 +2566,23 @@ 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;
>> +	struct device *dev = &priv->pdev->dev;
>> +	int ret = 0;
>> +
>> +	pm_runtime_get_noresume(dev);
>> +
>> +	if (!pm_runtime_active(dev))
>> +		goto out_set_features;
> 
> This can be simplified, which avoids 1 goto statement and
> Unnecessary ret initialization. I am leaving to you and Sergey.
> 
> 	if (!pm_runtime_active(dev))
> 		ret = 0;
> 	else
> 		ret = info->set_feature(ndev, features);
> 
> 	pm_runtime_put_noidle(dev);
> 	if (ret)
> 		goto err;
> 
> 	ndev->features = features;
> 
> err:
> 	return ret;
> 

I find it a bit difficult to follow this way.

Thank you,
Claudiu Beznea

> Cheers,
> Biju
> 
>>
>>  	ret = info->set_feature(ndev, features);
>>  	if (ret)
>> -		return ret;
>> +		goto out_rpm_put;
>>
>> +out_set_features:
>>  	ndev->features = features;
>> -
>> -	return 0;
>> +out_rpm_put:
>> +	pm_runtime_put_noidle(dev);
>> +	return ret;
>>  }
>>
>>  static const struct net_device_ops ravb_netdev_ops = {
>> --
>> 2.39.2
>
  

Hi Claudiu,

> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> Sent: Tuesday, February 13, 2024 11:07 AM
> Subject: Re: [PATCH net-next v3 5/6] net: ravb: Do not apply features to
> hardware if the interface is down
> 
> Hi, Biju,
> 
> On 13.02.2024 12:13, Biju Das wrote:
> >
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Claudiu <claudiu.beznea@tuxon.dev>
> >> Sent: Tuesday, February 13, 2024 9:41 AM
> >> Subject: [PATCH net-next v3 5/6] net: ravb: Do not apply features to
> >> hardware if the interface is down
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> Do not apply features 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 the clocks will
> >> switch the IP to reset mode, which will lead to losing register
> >> contents) and applying settings in reset mode is not an option.
> >> Instead, cache the features and apply them in ravb_open() through
> ravb_emac_init().
> >>
> >> To avoid accessing the hardware while the interface is down
> >> pm_runtime_active() check was introduced. Along with it the device
> >> runtime PM usage counter has been incremented to avoid disabling the
> >> device clocks while the check is in progress (if any).
> >>
> >> Commit prepares for the addition of runtime PM.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >> ---
> >>
> >> Changes in v3:
> >> - updated patch title and description
> >> - updated patch content due to patch 4/6
> >>
> >> Changes in v2:
> >> - fixed typo in patch description
> >> - adjusted ravb_set_features_gbeth(); didn't collect the Sergey's Rb
> >>   tag due to this
> >>
> >> Changes since [2]:
> >> - use pm_runtime_get_noresume() and pm_runtime_active() and updated the
> >>   commit message to describe that
> >> - fixed typos
> >> - s/CSUM/checksum in patch title and description
> >>
> >> Changes in v3 of [2]:
> >> - this was patch 20/21 in v2
> >> - fixed typos in patch description
> >> - removed code from ravb_open()
> >> - use ndev->flags & IFF_UP checks instead of netif_running()
> >>
> >> Changes in v2 of [2]:
> >> - none; this patch is new
> >>
> >>
> >>  drivers/net/ethernet/renesas/ravb_main.c | 16 ++++++++++++----
> >>  1 file changed, 12 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >> b/drivers/net/ethernet/renesas/ravb_main.c
> >> index b3b91783bb7a..4dd0520dea90 100644
> >> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >> @@ -2566,15 +2566,23 @@ 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;
> >> +	struct device *dev = &priv->pdev->dev;
> >> +	int ret = 0;
> >> +
> >> +	pm_runtime_get_noresume(dev);
> >> +
> >> +	if (!pm_runtime_active(dev))
> >> +		goto out_set_features;
> >
> > This can be simplified, which avoids 1 goto statement and Unnecessary
> > ret initialization. I am leaving to you and Sergey.
> >
> > 	if (!pm_runtime_active(dev))
> > 		ret = 0;
> > 	else
> > 		ret = info->set_feature(ndev, features);
> >
> > 	pm_runtime_put_noidle(dev);
> > 	if (ret)
> > 		goto err;
> >
> > 	ndev->features = features;
> >
> > err:
> > 	return ret;
> >
> 
> I find it a bit difficult to follow this way.

I find this patch complicated by doing unnecessary intiailzation
And goto statements for non-err cases.. 

Maybe others can suggest how to do it in a better way. 

Cheers,
Biju
> 
> Thank you,
> Claudiu Beznea
> 
> > Cheers,
> > Biju
> >
> >>
> >>  	ret = info->set_feature(ndev, features);
> >>  	if (ret)
> >> -		return ret;
> >> +		goto out_rpm_put;
> >>
> >> +out_set_features:
> >>  	ndev->features = features;
> >> -
> >> -	return 0;
> >> +out_rpm_put:
> >> +	pm_runtime_put_noidle(dev);
> >> +	return ret;
> >>  }
> >>
> >>  static const struct net_device_ops ravb_netdev_ops = {
> >> --
> >> 2.39.2
> >
  

On 13.02.2024 13:07, claudiu beznea wrote:
>>> @@ -2566,15 +2566,23 @@ 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;
>>> +	struct device *dev = &priv->pdev->dev;
>>> +	int ret = 0;
>>> +
>>> +	pm_runtime_get_noresume(dev);
>>> +
>>> +	if (!pm_runtime_active(dev))
>>> +		goto out_set_features;
>> This can be simplified, which avoids 1 goto statement and
>> Unnecessary ret initialization. I am leaving to you and Sergey.
>>
>> 	if (!pm_runtime_active(dev))
>> 		ret = 0;
>> 	else
>> 		ret = info->set_feature(ndev, features);
>>
>> 	pm_runtime_put_noidle(dev);
>> 	if (ret)
>> 		goto err;
>>
>> 	ndev->features = features;
>>
>> err:
>> 	return ret;
>>
> I find it a bit difficult to follow this way.

Looking again at it, your version seems better.
  

On 2/13/24 12:41 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Do not apply features 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 the clocks will switch the IP to
> reset mode, which will lead to losing register contents) and applying
> settings in reset mode is not an option. Instead, cache the features and
> apply them in ravb_open() through ravb_emac_init().
> 
> To avoid accessing the hardware while the interface is down
> pm_runtime_active() check was introduced. Along with it the device runtime
> PM usage counter has been incremented to avoid disabling the device clocks
> while the check is in progress (if any).
> 
> Commit prepares for the addition of runtime PM.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

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

[...]

MBR, Sergey