| Message ID | 20220322063221.28132-1-jan.sondhauss@wago.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drivers: ethernet: cpsw: fix panic when interrupt coaleceing is set via ethtool | expand |
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
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
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
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 >
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);
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(-)