[PATCH] can: c_can: fix segfault during rmmod

[PATCH] can: c_can: fix segfault during rmmod

[AnilKumar Ch]

This patch fixes an oops which occurs during unloading the driver. The
problem is that clock is not enabled during unregister_c_can_dev()
which does c_can/d_can module interrupts enable/disable.

Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
---
 drivers/net/can/c_can/c_can.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 2c4a21f..638e3cc 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -1265,8 +1265,10 @@ void unregister_c_can_dev(struct net_device *dev)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 
+	c_can_pm_runtime_get_sync(priv);
 	/* disable all interrupts */
 	c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
+	c_can_pm_runtime_put_sync(priv);
 
 	unregister_candev(dev);
 
-- 
1.7.0.4

Re: [PATCH] can: c_can: fix segfault during rmmod

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2723 bytes --]

On 09/27/2012 12:41 PM, AnilKumar Ch wrote:
> This patch fixes an oops which occurs during unloading the driver. The
> problem is that clock is not enabled during unregister_c_can_dev()
> which does c_can/d_can module interrupts enable/disable.

Looking at the code, I see that
    c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
is called during c_can_stop().

This should be sufficient as during unregister_netdev() the driver's
close function will be called (it the netdev is still opened) [1].

I think your patch
    4cdd34b can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller
has not reached the David's net branch, so I prefer to remove the
offending:
    c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
call from the unregister_c_can_dev().

Marc

[1] I recently fixed a OOPS in another driver with this stack trace:

    [  172.954498] [<bf00c768>] (ti_hecc_close+0xb0/0x100 [ti_hecc]) from [<c033be58>] (__dev__registered_many+0xc0/0x2a0)
    [  172.984161] [<c033c098>] (rollback_registered_many+0xc0/0x2a0) from [<c033c2f8>] (rollback_registered+0x20/0x30)
    [  172.994750] [<c033c2f8>] (rollback_registered+0x20/0x30) from [<c033c370>] (unregister_netdevice_queue+0x68/0x98)
    [  173.005401] [<c033c370>] (unregister_netdevice_queue+0x68/0x98) from [<c033c3b8>] (unregister_netdev+0x18/0x20)
    [  173.015899] [<c033c3b8>] (unregister_netdev+0x18/0x20) from [<bf00d3ac>] (ti_hecc_remove+0x60/0x80 [ti_hecc])
    [  173.026245] [<bf00d3ac>] (ti_hecc_remove+0x60/0x80 [ti_hecc]) from [<c02842dc>] (platform_drv_remove+0x14/0x18)
    [  173.036712] [<c02842dc>] (platform_drv_remove+0x14/0x18) from [<c0282f90>] (__device_release_driver+0x7c/0xbc)

Marc

> 
> Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
> ---
>  drivers/net/can/c_can/c_can.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 2c4a21f..638e3cc 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -1265,8 +1265,10 @@ void unregister_c_can_dev(struct net_device *dev)
>  {
>  	struct c_can_priv *priv = netdev_priv(dev);
>  
> +	c_can_pm_runtime_get_sync(priv);
>  	/* disable all interrupts */
>  	c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
> +	c_can_pm_runtime_put_sync(priv);
>  
>  	unregister_candev(dev);
>  
> 


-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

RE: [PATCH] can: c_can: fix segfault during rmmod

[AnilKumar, Chimata]

Hi Marc,

On Thu, Sep 27, 2012 at 16:41:56, Marc Kleine-Budde wrote:
> On 09/27/2012 12:41 PM, AnilKumar Ch wrote:
> > This patch fixes an oops which occurs during unloading the driver. The
> > problem is that clock is not enabled during unregister_c_can_dev()
> > which does c_can/d_can module interrupts enable/disable.
> 
> Looking at the code, I see that
>     c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
> is called during c_can_stop().
> 
> This should be sufficient as during unregister_netdev() the driver's
> close function will be called (it the netdev is still opened) [1].
> 
> I think your patch
>     4cdd34b can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller
> has not reached the David's net branch, so I prefer to remove the
> offending:
>     c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
> call from the unregister_c_can_dev().
> 

The moment I clicked the send button I thought this, I will send a new
patch by removing that line.

Thanks
AnilKumar 

[PATCH] can: c_can: fix segfault during rmmod

[AnilKumar Ch]

This patch fixes an oops which occurs during unloading the driver.
unregister_c_can_dev() is doing c_can/d_can module interrupts
disable, which requires module clock enable. c_can/d_can interrupts
enable/disable is handled properly in c_can_start and c_can_stop,
so removing from unregister_c_can_dev().

Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
---
 drivers/net/can/c_can/c_can.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 25f4356..7a2a7e4 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -1282,9 +1282,6 @@ void unregister_c_can_dev(struct net_device *dev)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 
-	/* disable all interrupts */
-	c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
-
 	unregister_candev(dev);
 
 	c_can_pm_runtime_disable(priv);
-- 
1.7.9.5

Re: [PATCH] can: c_can: fix segfault during rmmod

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 827 bytes --]

On 09/27/2012 02:24 PM, AnilKumar Ch wrote:
> This patch fixes an oops which occurs during unloading the driver.
> unregister_c_can_dev() is doing c_can/d_can module interrupts
> disable, which requires module clock enable. c_can/d_can interrupts
> enable/disable is handled properly in c_can_start and c_can_stop,
> so removing from unregister_c_can_dev().
> 
> Signed-off-by: AnilKumar Ch <anilkumar@ti.com>

Thanks, I've added a reference to the commit that added the runtime pm
support. I'll send a pull request to David soon.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]