| Message ID | 20160615115603.4897-5-grygorii.strashko@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/15/2016 04:55 AM, Grygorii Strashko wrote: > The CPSW might be suspended by RPM if all ethX interfaces are down, > but it still could be accesible through ethtool interfce. In this case > ethtool operations, requiring registers access, will cause L3 errors and > CPSW crash. > > Hence, fix it by adding RPM get/put calls in ethtool callbcaks which > can access CPSW registers: .set_coalesce(), .get_ethtool_stats(), > .set_pauseparam(), .get_regs() Provided that you implement an ethtool_ops::begin, it will be called before each ethtool operation runs, so that could allow you to eliminate some of the duplication here. Conversely ethtool_ops::end terminates each operation and can be used for that purpose. > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > --- > drivers/net/ethernet/ti/cpsw.c | 36 +++++++++++++++++++++++++++++++++++- > 1 file changed, 35 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c > index ba81d4e..1ba0c09 100644 > --- a/drivers/net/ethernet/ti/cpsw.c > +++ b/drivers/net/ethernet/ti/cpsw.c > @@ -931,6 +931,13 @@ static int cpsw_set_coalesce(struct net_device *ndev, > u32 prescale = 0; > u32 addnl_dvdr = 1; > u32 coal_intvl = 0; > + int ret; > + > + ret = pm_runtime_get_sync(&priv->pdev->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(&priv->pdev->dev); > + return ret; > + } > > coal_intvl = coal->rx_coalesce_usecs; > > @@ -985,6 +992,8 @@ update_return: > priv->coal_intvl = coal_intvl; > } > > + pm_runtime_put(&priv->pdev->dev); > + > return 0; > } > > @@ -1022,7 +1031,13 @@ static void cpsw_get_ethtool_stats(struct net_device *ndev, > struct cpdma_chan_stats tx_stats; > u32 val; > u8 *p; > - int i; > + int i, ret; > + > + ret = pm_runtime_get_sync(&priv->pdev->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(&priv->pdev->dev); > + return; > + } > > /* Collect Davinci CPDMA stats for Rx and Tx Channel */ > cpdma_chan_get_stats(priv->rxch, &rx_stats); > @@ -1049,6 +1064,8 @@ static void cpsw_get_ethtool_stats(struct net_device *ndev, > break; > } > } > + > + pm_runtime_put(&priv->pdev->dev); > } > > static int cpsw_common_res_usage_state(struct cpsw_priv *priv) > @@ -1780,11 +1797,20 @@ static void cpsw_get_regs(struct net_device *ndev, > { > struct cpsw_priv *priv = netdev_priv(ndev); > u32 *reg = p; > + int ret; > + > + ret = pm_runtime_get_sync(&priv->pdev->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(&priv->pdev->dev); > + return; > + } > > /* update CPSW IP version */ > regs->version = priv->version; > > cpsw_ale_dump(priv->ale, reg); > + > + pm_runtime_put(&priv->pdev->dev); > } > > static void cpsw_get_drvinfo(struct net_device *ndev, > @@ -1902,12 +1928,20 @@ static int cpsw_set_pauseparam(struct net_device *ndev, > { > struct cpsw_priv *priv = netdev_priv(ndev); > bool link; > + int ret; > + > + ret = pm_runtime_get_sync(&priv->pdev->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(&priv->pdev->dev); > + return ret; > + } > > priv->rx_pause = pause->rx_pause ? true : false; > priv->tx_pause = pause->tx_pause ? true : false; > > for_each_slave(priv, _cpsw_adjust_link, priv, &link); > > + pm_runtime_put(&priv->pdev->dev); > return 0; > } > >
On 06/15/2016 07:14 PM, Florian Fainelli wrote: > On 06/15/2016 04:55 AM, Grygorii Strashko wrote: >> The CPSW might be suspended by RPM if all ethX interfaces are down, >> but it still could be accesible through ethtool interfce. In this case >> ethtool operations, requiring registers access, will cause L3 errors and >> CPSW crash. >> >> Hence, fix it by adding RPM get/put calls in ethtool callbcaks which >> can access CPSW registers: .set_coalesce(), .get_ethtool_stats(), >> .set_pauseparam(), .get_regs() > > Provided that you implement an ethtool_ops::begin, it will be called > before each ethtool operation runs, so that could allow you to eliminate > some of the duplication here. Conversely ethtool_ops::end terminates > each operation and can be used for that purpose. Ah. Thanks for the advice. (assume you've meant .complete())
On 15.06.16 19:45, Grygorii Strashko wrote: > On 06/15/2016 07:14 PM, Florian Fainelli wrote: >> On 06/15/2016 04:55 AM, Grygorii Strashko wrote: >>> The CPSW might be suspended by RPM if all ethX interfaces are down, >>> but it still could be accesible through ethtool interfce. In this case >>> ethtool operations, requiring registers access, will cause L3 errors and >>> CPSW crash. >>> >>> Hence, fix it by adding RPM get/put calls in ethtool callbcaks which >>> can access CPSW registers: .set_coalesce(), .get_ethtool_stats(), >>> .set_pauseparam(), .get_regs() >> >> Provided that you implement an ethtool_ops::begin, it will be called >> before each ethtool operation runs, so that could allow you to eliminate >> some of the duplication here. Conversely ethtool_ops::end terminates >> each operation and can be used for that purpose. > > Ah. Thanks for the advice. (assume you've meant .complete()) Maybe better leave as is, I'm going to add two additional ehttool ops containing RPM sensitive part and not, better to split code on those, if possible...and hiding put/get in begin op can call rpm when it's not needed at all.. > > >
On 16.06.16 15:48, Ivan Khoronzhuk wrote: > > > On 15.06.16 19:45, Grygorii Strashko wrote: >> On 06/15/2016 07:14 PM, Florian Fainelli wrote: >>> On 06/15/2016 04:55 AM, Grygorii Strashko wrote: >>>> The CPSW might be suspended by RPM if all ethX interfaces are down, >>>> but it still could be accesible through ethtool interfce. In this case >>>> ethtool operations, requiring registers access, will cause L3 errors and >>>> CPSW crash. >>>> >>>> Hence, fix it by adding RPM get/put calls in ethtool callbcaks which >>>> can access CPSW registers: .set_coalesce(), .get_ethtool_stats(), >>>> .set_pauseparam(), .get_regs() >>> >>> Provided that you implement an ethtool_ops::begin, it will be called >>> before each ethtool operation runs, so that could allow you to eliminate >>> some of the duplication here. Conversely ethtool_ops::end terminates >>> each operation and can be used for that purpose. >> >> Ah. Thanks for the advice. (assume you've meant .complete()) > > Maybe better leave as is, I'm going to add two additional ehttool ops > containing RPM sensitive part and not, better to split code on those, > if possible...and hiding put/get in begin op can call rpm when it's not > needed at all.. But it's not a big deal, ethtool operations are rarelly anyway, and have impact only when interface is down that makes it even more rarely. > >> >> >> >
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c index ba81d4e..1ba0c09 100644 --- a/drivers/net/ethernet/ti/cpsw.c +++ b/drivers/net/ethernet/ti/cpsw.c @@ -931,6 +931,13 @@ static int cpsw_set_coalesce(struct net_device *ndev, u32 prescale = 0; u32 addnl_dvdr = 1; u32 coal_intvl = 0; + int ret; + + ret = pm_runtime_get_sync(&priv->pdev->dev); + if (ret < 0) { + pm_runtime_put_noidle(&priv->pdev->dev); + return ret; + } coal_intvl = coal->rx_coalesce_usecs; @@ -985,6 +992,8 @@ update_return: priv->coal_intvl = coal_intvl; } + pm_runtime_put(&priv->pdev->dev); + return 0; } @@ -1022,7 +1031,13 @@ static void cpsw_get_ethtool_stats(struct net_device *ndev, struct cpdma_chan_stats tx_stats; u32 val; u8 *p; - int i; + int i, ret; + + ret = pm_runtime_get_sync(&priv->pdev->dev); + if (ret < 0) { + pm_runtime_put_noidle(&priv->pdev->dev); + return; + } /* Collect Davinci CPDMA stats for Rx and Tx Channel */ cpdma_chan_get_stats(priv->rxch, &rx_stats); @@ -1049,6 +1064,8 @@ static void cpsw_get_ethtool_stats(struct net_device *ndev, break; } } + + pm_runtime_put(&priv->pdev->dev); } static int cpsw_common_res_usage_state(struct cpsw_priv *priv) @@ -1780,11 +1797,20 @@ static void cpsw_get_regs(struct net_device *ndev, { struct cpsw_priv *priv = netdev_priv(ndev); u32 *reg = p; + int ret; + + ret = pm_runtime_get_sync(&priv->pdev->dev); + if (ret < 0) { + pm_runtime_put_noidle(&priv->pdev->dev); + return; + } /* update CPSW IP version */ regs->version = priv->version; cpsw_ale_dump(priv->ale, reg); + + pm_runtime_put(&priv->pdev->dev); } static void cpsw_get_drvinfo(struct net_device *ndev, @@ -1902,12 +1928,20 @@ static int cpsw_set_pauseparam(struct net_device *ndev, { struct cpsw_priv *priv = netdev_priv(ndev); bool link; + int ret; + + ret = pm_runtime_get_sync(&priv->pdev->dev); + if (ret < 0) { + pm_runtime_put_noidle(&priv->pdev->dev); + return ret; + } priv->rx_pause = pause->rx_pause ? true : false; priv->tx_pause = pause->tx_pause ? true : false; for_each_slave(priv, _cpsw_adjust_link, priv, &link); + pm_runtime_put(&priv->pdev->dev); return 0; }
The CPSW might be suspended by RPM if all ethX interfaces are down, but it still could be accesible through ethtool interfce. In this case ethtool operations, requiring registers access, will cause L3 errors and CPSW crash. Hence, fix it by adding RPM get/put calls in ethtool callbcaks which can access CPSW registers: .set_coalesce(), .get_ethtool_stats(), .set_pauseparam(), .get_regs() Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- drivers/net/ethernet/ti/cpsw.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)