Date:   Tue, 22 Mar 2022 14:24:50 +0000
From:   Sondhauß, Jan <Jan.Sondhauss@...o.com>
To:     Vignesh Raghavendra <vigneshr@...com>,
        "grygorii.strashko@...com" <grygorii.strashko@...com>
CC:     "linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH] drivers: ethernet: cpsw: fix panic when interrupt
 coaleceing is set via ethtool



On 22/03/2022 14:40, Vignesh Raghavendra wrote:
> On 22/03/22 5:50 pm, Sondhauß, Jan wrote: > 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 >> ZjQcmQRYFpfptBannerStart
> This Message Is From an External Sender
> Please use caution when clicking on links or opening attachments!
> ZjQcmQRYFpfptBannerEnd
> 
> On 22/03/22 5:50 pm, Sondhauß, Jan wrote:
>> 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.
>> 
> 
> I think this is a better explanation than the original commit message,
> but see below
> 
>>>
>>>
>>>> Signed-off-by: Jan Sondhauss <jan.sondhauss@...o.com>
>>>> ---
>>>>  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.
>> 
> 
> Thanks for the explanation!
> 
> Sorry, But what about the error case (ie ret < 0) With this patch, don't
> we end up calling pm_runtime_put_noidle() twice (once inside
> pm_runtime_resume_and_get() and again in cpsw_ethtool_op_begin()). How
> is that okay?

Of course its not okay. Thanks, totally missed that case.. I will be 
preparing a V2 where the additional call to pm_runtime_put_noidle is 
omitted in cpsw_ethtool. Also I will rework the commit message as you 
suggested.

Thanks for your feedback so far!

- Jan

> 
> 
> Regards
> Vignesh
> 

Powered by blists - more mailing lists