Hi
On 22/03/2022 11:34, Vignesh Raghavendra wrote:
> Hi, Adding netdev list and maintainers Please cc netdev ML and net
> maintainers ./scripts/get_maintainer.pl -f
> drivers/net/ethernet/ti/cpsw_ethtool.c On 22/03/22 12:02 pm, Sondhauß,
> Jan wrote: > cpsw_ethtool uses the power management in the
> ZjQcmQRYFpfptBannerStart
> This Message Is From an External Sender
> Please use caution when clicking on links or opening attachments!
> ZjQcmQRYFpfptBannerEnd
>
> Hi,
>
> Adding netdev list and maintainers
>
> Please cc netdev ML and net maintainers
>
> ./scripts/get_maintainer.pl -f drivers/net/ethernet/ti/cpsw_ethtool.c
>
> On 22/03/22 12:02 pm, Sondhauß, Jan wrote:
>> cpsw_ethtool uses the power management in the begin and complete
>> functions of the ethtool_ops. The result of pm_runtime_get_sync was
>> returned unconditionally, which results in problems since the ethtool-
>> interface relies on 0 for success and negativ values for errors.
>> d43c65b05b84 (ethtool: runtime-resume netdev parent in ethnl_ops_begin)
>> introduced power management to the netlink implementation for the
>> ethtool interface and does not explicitly check for negative return
>> values.
>>
>> As a result the pm_runtime_suspend function is called one-too-many
>> times in ethnl_ops_begin and that leads to an access violation when
>> the cpsw hardware is accessed after using
>> 'ethtool -C eth-of-cpsw rx-usecs 1234'. To fix this the call to
>> pm_runtime_get_sync in cpsw_ethtool_op_begin is replaced with a call
>> to pm_runtime_resume_and_get as it provides a returnable error-code.
>>
>
> pm_runtime_resume_and_get() is just wrapper around pm_runtime_get_sync()
> + error handling (as done in the below code) and both return 0 on
> success and -ve error code on failure
pm_runtime_get_sync returns -ve error code on failure and 0 on success
and also 1 is returned if nothing has to be done besides increment of
the usage counter.
So for active devices that don't need to be resumed a 1 is returned.
pm_runtime_resume_and_get is a return-friendly wrapper that returns
-error code on failure but returns 0 on both other cases.
>
>
>> Signed-off-by: Jan Sondhauss <jan.sondhauss@xxxxxxxx>
>> ---
>> drivers/net/ethernet/ti/cpsw_ethtool.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/ti/cpsw_ethtool.c b/drivers/net/ethernet/ti/cpsw_ethtool.c
>> index 158c8d3793f4..5eda20039cc1 100644
>> --- a/drivers/net/ethernet/ti/cpsw_ethtool.c
>> +++ b/drivers/net/ethernet/ti/cpsw_ethtool.c
>> @@ -364,7 +364,7 @@ int cpsw_ethtool_op_begin(struct net_device *ndev)
>> struct cpsw_common *cpsw = priv->cpsw;
>> int ret;
>>
>> - ret = pm_runtime_get_sync(cpsw->dev);
>> + ret = pm_runtime_resume_and_get(cpsw->dev)> if (ret < 0) {
>> cpsw_err(priv, drv, "ethtool begin failed %d\n", ret);
>> pm_runtime_put_noidle(cpsw->dev);
>
>
> In fact code now ends up calling pm_runtime_put_noidle() twice in case
> of failure, once inside pm_runtime_resume_and_get() and again here?
>
> So something looks fishy?
Sort of. There is no actual failure but pm_runtime_put is still called
twice. That is due to
1. cpsw_ethtool_op_begin returning 1 when it should return 0
2. ethnl_ops_begin treating values not equal to 0 as failure
3. coalesce_prepare_data only treating negative values as failure
The patch addresses 1.
In net/ethtool/netlink.c:33 ethnl_ops_begin() the cpsw_ethtool_op_begin
is called (returning 1) and in the error path of ethnl_ops_begin a
pm_runtime_put is called. The function calling ethnl_ops_begin only
checks for negative values: net/ethtool/coalesce.c:60
coalesce_prepare_data and continues the sucess path calling
ethnl_ops_complete. ethnl_ops_complete also calls pm_runtime_put. So the
success path of coalesce_prepare_data and the error path of
ethnl_ops_begin both end up calling pm_runtime_put when only one of them
should.
>
> Regards
> Vignesh
>
Regards,
Jan
