| Message ID | 20210301041550.795500-1-ztong0001@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | can: c_can: move runtime PM enable/disable to c_can_platform | expand |
On 28.02.2021 23:15:48, Tong Zhang wrote: > Currently doing modprobe c_can_pci will make kernel complain > "Unbalanced pm_runtime_enable!", this is caused by pm_runtime_enable() > called before pm is initialized in register_candev() and doing so will I don't see where register_candev() is doing any pm related initialization. > also cause it to enable twice. > This fix is similar to 227619c3ff7c, move those pm_enable/disable code to > c_can_platform. As I understand 227619c3ff7c ("can: m_can: move runtime PM enable/disable to m_can_platform"), PCI devices automatically enable PM, when the "PCI device is added". Please clarify the above point, otherwise the code looks OK, small nitpick inline: > Signed-off-by: Tong Zhang <ztong0001@gmail.com> > --- > drivers/net/can/c_can/c_can.c | 26 ++------------------------ > drivers/net/can/c_can/c_can_platform.c | 6 +++++- > 2 files changed, 7 insertions(+), 25 deletions(-) > > diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c > index ef474bae47a1..04f783b3d9d3 100644 > --- a/drivers/net/can/c_can/c_can.c > +++ b/drivers/net/can/c_can/c_can.c > @@ -212,18 +212,6 @@ static const struct can_bittiming_const c_can_bittiming_const = { > .brp_inc = 1, > }; > > -static inline void c_can_pm_runtime_enable(const struct c_can_priv *priv) > -{ > - if (priv->device) > - pm_runtime_enable(priv->device); > -} > - > -static inline void c_can_pm_runtime_disable(const struct c_can_priv *priv) > -{ > - if (priv->device) > - pm_runtime_disable(priv->device); > -} > - > static inline void c_can_pm_runtime_get_sync(const struct c_can_priv *priv) > { > if (priv->device) > @@ -1335,7 +1323,6 @@ static const struct net_device_ops c_can_netdev_ops = { > > int register_c_can_dev(struct net_device *dev) > { > - struct c_can_priv *priv = netdev_priv(dev); > int err; > > /* Deactivate pins to prevent DRA7 DCAN IP from being > @@ -1345,28 +1332,19 @@ int register_c_can_dev(struct net_device *dev) > */ > pinctrl_pm_select_sleep_state(dev->dev.parent); > > - c_can_pm_runtime_enable(priv); > - > dev->flags |= IFF_ECHO; /* we support local echo */ > dev->netdev_ops = &c_can_netdev_ops; > > err = register_candev(dev); > - if (err) > - c_can_pm_runtime_disable(priv); > - else > - devm_can_led_init(dev); > - > + if (!err) > + devm_can_led_init(dev); Please indent with two tabs here. regards, Marc
On Mon, Mar 1, 2021 at 2:49 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > On 28.02.2021 23:15:48, Tong Zhang wrote: > > Currently doing modprobe c_can_pci will make kernel complain > > "Unbalanced pm_runtime_enable!", this is caused by pm_runtime_enable() > > called before pm is initialized in register_candev() and doing so will > > I don't see where register_candev() is doing any pm related > initialization. > > > also cause it to enable twice. > > > This fix is similar to 227619c3ff7c, move those pm_enable/disable code to > > c_can_platform. > > As I understand 227619c3ff7c ("can: m_can: move runtime PM > enable/disable to m_can_platform"), PCI devices automatically enable PM, > when the "PCI device is added". Hi Marc, Thanks for the comments. I thinks you are right -- I was mislead by the trace -- I have corrected the commit log along with the indent fix in v2 patch. Thanks again for your help, - Tong > > Please clarify the above point, otherwise the code looks OK, small > nitpick inline:
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c index ef474bae47a1..04f783b3d9d3 100644 --- a/drivers/net/can/c_can/c_can.c +++ b/drivers/net/can/c_can/c_can.c @@ -212,18 +212,6 @@ static const struct can_bittiming_const c_can_bittiming_const = { .brp_inc = 1, }; -static inline void c_can_pm_runtime_enable(const struct c_can_priv *priv) -{ - if (priv->device) - pm_runtime_enable(priv->device); -} - -static inline void c_can_pm_runtime_disable(const struct c_can_priv *priv) -{ - if (priv->device) - pm_runtime_disable(priv->device); -} - static inline void c_can_pm_runtime_get_sync(const struct c_can_priv *priv) { if (priv->device) @@ -1335,7 +1323,6 @@ static const struct net_device_ops c_can_netdev_ops = { int register_c_can_dev(struct net_device *dev) { - struct c_can_priv *priv = netdev_priv(dev); int err; /* Deactivate pins to prevent DRA7 DCAN IP from being @@ -1345,28 +1332,19 @@ int register_c_can_dev(struct net_device *dev) */ pinctrl_pm_select_sleep_state(dev->dev.parent); - c_can_pm_runtime_enable(priv); - dev->flags |= IFF_ECHO; /* we support local echo */ dev->netdev_ops = &c_can_netdev_ops; err = register_candev(dev); - if (err) - c_can_pm_runtime_disable(priv); - else - devm_can_led_init(dev); - + if (!err) + devm_can_led_init(dev); return err; } EXPORT_SYMBOL_GPL(register_c_can_dev); void unregister_c_can_dev(struct net_device *dev) { - struct c_can_priv *priv = netdev_priv(dev); - unregister_candev(dev); - - c_can_pm_runtime_disable(priv); } EXPORT_SYMBOL_GPL(unregister_c_can_dev); diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c index 05f425ceb53a..47b251b1607c 100644 --- a/drivers/net/can/c_can/c_can_platform.c +++ b/drivers/net/can/c_can/c_can_platform.c @@ -29,6 +29,7 @@ #include <linux/list.h> #include <linux/io.h> #include <linux/platform_device.h> +#include <linux/pm_runtime.h> #include <linux/clk.h> #include <linux/of.h> #include <linux/of_device.h> @@ -386,6 +387,7 @@ static int c_can_plat_probe(struct platform_device *pdev) platform_set_drvdata(pdev, dev); SET_NETDEV_DEV(dev, &pdev->dev); + pm_runtime_enable(priv->device); ret = register_c_can_dev(dev); if (ret) { dev_err(&pdev->dev, "registering %s failed (err=%d)\n", @@ -398,6 +400,7 @@ static int c_can_plat_probe(struct platform_device *pdev) return 0; exit_free_device: + pm_runtime_disable(priv->device); free_c_can_dev(dev); exit: dev_err(&pdev->dev, "probe failed\n"); @@ -408,9 +411,10 @@ static int c_can_plat_probe(struct platform_device *pdev) static int c_can_plat_remove(struct platform_device *pdev) { struct net_device *dev = platform_get_drvdata(pdev); + struct c_can_priv *priv = netdev_priv(dev); unregister_c_can_dev(dev); - + pm_runtime_disable(priv->device); free_c_can_dev(dev); return 0;
Currently doing modprobe c_can_pci will make kernel complain "Unbalanced pm_runtime_enable!", this is caused by pm_runtime_enable() called before pm is initialized in register_candev() and doing so will also cause it to enable twice. This fix is similar to 227619c3ff7c, move those pm_enable/disable code to c_can_platform. Signed-off-by: Tong Zhang <ztong0001@gmail.com> --- drivers/net/can/c_can/c_can.c | 26 ++------------------------ drivers/net/can/c_can/c_can_platform.c | 6 +++++- 2 files changed, 7 insertions(+), 25 deletions(-)