On 23/03/2022 12:10, Vignesh Raghavendra wrote:
> On 23/03/22 2:17 pm, Sondhauß, Jan wrote: > cpsw_ethtool_begin directly
> returns the result of pm_runtime_get_sync > when successful. >
> pm_runtime_get_sync returns -error code on failure and 0 on successful >
> resume but also 1 when ZjQcmQRYFpfptBannerStart
> This Message Is From an External Sender
> Please use caution when clicking on links or opening attachments!
> ZjQcmQRYFpfptBannerEnd
>
> On 23/03/22 2:17 pm, Sondhauß, Jan wrote:
>> cpsw_ethtool_begin directly returns the result of pm_runtime_get_sync
>> when successful.
>> pm_runtime_get_sync returns -error code on failure and 0 on successful
>> resume but also 1 when the device is already active. So the common case
>> for cpsw_ethtool_begin is to return 1. That leads to inconsistent calls
>> to pm_runtime_put in the call-chain so that pm_runtime_put is called
>> one too many times and as result leaving the cpsw dev behind suspended.
>>
>> The suspended cpsw dev leads to an access violation later on by
>> different parts of the cpsw driver.
>>
>> Fix this by calling the return-friendly pm_runtime_resume_and_get
>> function.
>>
>> Signed-off-by: Jan Sondhauss <jan.sondhauss@xxxxxxxx>
>> ---
>> drivers/net/ethernet/ti/cpsw_ethtool.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/cpsw_ethtool.c b/drivers/net/ethernet/ti/cpsw_ethtool.c
>> index 158c8d3793f4..b5bae6324970 100644
>> --- a/drivers/net/ethernet/ti/cpsw_ethtool.c
>> +++ b/drivers/net/ethernet/ti/cpsw_ethtool.c
>> @@ -364,11 +364,9 @@ int cpsw_ethtool_op_begin(struct net_device *ndev)
>> struct cpsw_common *cpsw = priv->cpsw;
>> int ret;
>>
>> - ret = pm_runtime_get_sync(cpsw->dev);
>> - if (ret < 0) {
>> + 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);
>> - }
>>
>> return ret;
>> }
>
> Thanks for the fix!
>
> Reviewed-by: Vignesh Raghavendra <vigneshr@xxxxxx>
>
> I am guessing this issue started with d43c65b05b84 (ethtool:
> runtime-resume netdev parent in ethnl_ops_begin) ? If so, could you add
> a Fixes tag so that patch gets backported to all affected stable kernels?
Yes it started with that commit.
>
> Regards
> Vignesh
>
