* [PATCH] drivers: ethernet: cpsw: fix panic when interrupt coaleceing is set via ethtool
@ 2022-03-22 6:32 Sondhauß, Jan
2022-03-22 10:34 ` Vignesh Raghavendra
0 siblings, 1 reply; 5+ messages in thread
From: Sondhauß, Jan @ 2022-03-22 6:32 UTC (permalink / raw)
To: grygorii.strashko; +Cc: linux-omap, Sondhauß, Jan
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.
Signed-off-by: Jan Sondhauss <jan.sondhauss@wago.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);
--
2.35.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drivers: ethernet: cpsw: fix panic when interrupt coaleceing is set via ethtool
2022-03-22 6:32 [PATCH] drivers: ethernet: cpsw: fix panic when interrupt coaleceing is set via ethtool Sondhauß, Jan
@ 2022-03-22 10:34 ` Vignesh Raghavendra
2022-03-22 12:20 ` Sondhauß, Jan
0 siblings, 1 reply; 5+ messages in thread
From: Vignesh Raghavendra @ 2022-03-22 10:34 UTC (permalink / raw)
To: Sondhauß, Jan, grygorii.strashko
Cc: linux-omap, David S . Miller, netdev, Jakub Kicinski
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
> Signed-off-by: Jan Sondhauss <jan.sondhauss@wago.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?
Regards
Vignesh
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drivers: ethernet: cpsw: fix panic when interrupt coaleceing is set via ethtool
2022-03-22 10:34 ` Vignesh Raghavendra
@ 2022-03-22 12:20 ` Sondhauß, Jan
2022-03-22 13:40 ` Vignesh Raghavendra
0 siblings, 1 reply; 5+ messages in thread
From: Sondhauß, Jan @ 2022-03-22 12:20 UTC (permalink / raw)
To: Vignesh Raghavendra, grygorii.strashko
Cc: linux-omap, David S . Miller, netdev, Jakub Kicinski
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@wago.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.
>
> Regards
> Vignesh
>
Regards,
Jan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drivers: ethernet: cpsw: fix panic when interrupt coaleceing is set via ethtool
2022-03-22 12:20 ` Sondhauß, Jan
@ 2022-03-22 13:40 ` Vignesh Raghavendra
2022-03-22 14:24 ` Sondhauß, Jan
0 siblings, 1 reply; 5+ messages in thread
From: Vignesh Raghavendra @ 2022-03-22 13:40 UTC (permalink / raw)
To: Sondhauß, Jan, grygorii.strashko
Cc: linux-omap, David S . Miller, netdev, Jakub Kicinski
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@wago.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?
Regards
Vignesh
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drivers: ethernet: cpsw: fix panic when interrupt coaleceing is set via ethtool
2022-03-22 13:40 ` Vignesh Raghavendra
@ 2022-03-22 14:24 ` Sondhauß, Jan
0 siblings, 0 replies; 5+ messages in thread
From: Sondhauß, Jan @ 2022-03-22 14:24 UTC (permalink / raw)
To: Vignesh Raghavendra, grygorii.strashko
Cc: linux-omap, David S . Miller, netdev, Jakub Kicinski
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@wago.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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-03-22 14:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-22 6:32 [PATCH] drivers: ethernet: cpsw: fix panic when interrupt coaleceing is set via ethtool Sondhauß, Jan
2022-03-22 10:34 ` Vignesh Raghavendra
2022-03-22 12:20 ` Sondhauß, Jan
2022-03-22 13:40 ` Vignesh Raghavendra
2022-03-22 14:24 ` Sondhauß, Jan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.