2018-03-08 20:44:57

by Merlijn Wajer

[permalink] [raw]
Subject: [PATCH] usb: musb: Fix external abort in musb_remove

This fixes an oops on unbind / module unload.

musb_remove function now calls musb_platform_exit before disabling
runtime pm.

Signed-off-by: Merlijn Wajer <[email protected]>
---
drivers/usb/musb/musb_core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index e2e95071328a..cf90d34f7199 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -2472,13 +2472,14 @@ static int musb_remove(struct platform_device *pdev)
musb_platform_disable(musb);
spin_lock_irqsave(&musb->lock, flags);
musb_disable_interrupts(musb);
- musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
spin_unlock_irqrestore(&musb->lock, flags);

+ musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
+ musb_platform_exit(musb);
+
pm_runtime_dont_use_autosuspend(musb->controller);
pm_runtime_put_sync(musb->controller);
pm_runtime_disable(musb->controller);
- musb_platform_exit(musb);
musb_phy_callback = NULL;
if (musb->dma_controller)
musb_dma_controller_destroy(musb->dma_controller);
--
2.16.2



2018-03-08 21:16:37

by Bin Liu

[permalink] [raw]
Subject: Re: [PATCH] usb: musb: Fix external abort in musb_remove

Hi,

please add patch version numbers in the subject when necessary. This
helps cross-referencing.

On Thu, Mar 08, 2018 at 09:40:47PM +0100, Merlijn Wajer wrote:
> This fixes an oops on unbind / module unload.
>
> musb_remove function now calls musb_platform_exit before disabling
> runtime pm.
>
> Signed-off-by: Merlijn Wajer <[email protected]>
> ---
> drivers/usb/musb/musb_core.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index e2e95071328a..cf90d34f7199 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -2472,13 +2472,14 @@ static int musb_remove(struct platform_device *pdev)
> musb_platform_disable(musb);
> spin_lock_irqsave(&musb->lock, flags);
> musb_disable_interrupts(musb);
> - musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
> spin_unlock_irqrestore(&musb->lock, flags);
>
> + musb_writeb(musb->mregs, MUSB_DEVCTL, 0);

Does it solve the issue if not moving this line? I'd like to have
minimum change if possible.

> + musb_platform_exit(musb);
> +
> pm_runtime_dont_use_autosuspend(musb->controller);
> pm_runtime_put_sync(musb->controller);
> pm_runtime_disable(musb->controller);
> - musb_platform_exit(musb);
> musb_phy_callback = NULL;
> if (musb->dma_controller)
> musb_dma_controller_destroy(musb->dma_controller);

Regards,
-Bin.


2018-03-08 21:19:59

by Bin Liu

[permalink] [raw]
Subject: Re: [PATCH] usb: musb: Fix external abort in musb_remove

On Thu, Mar 08, 2018 at 09:40:47PM +0100, Merlijn Wajer wrote:
> This fixes an oops on unbind / module unload.

Please also mention here that it happens on omap2430 platform.

(omap2430 is the only platform affected by this bug.)

Regards,
-Bin.

2018-03-08 22:19:45

by Merlijn Wajer

[permalink] [raw]
Subject: Re: [PATCH] usb: musb: Fix external abort in musb_remove

Hi,

On 08/03/18 22:15, Bin Liu wrote:

> please add patch version numbers in the subject when necessary. This
> helps cross-referencing.

Will do. I naively assumed that the first patch would implicitly be
number 1. Will send out v2 now.

>>
>> + musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
>
> Does it solve the issue if not moving this line? I'd like to have
> minimum change if possible.

Yes, it does. The only reason I moved musb_writeb is because I
understood you wanted me to move both (per previous message: "This can
be move down to out side of holding the spinlock").

Thanks for the tips.

Cheers,
Merlijn


Attachments:
signature.asc (235.00 B)
OpenPGP digital signature

2018-03-09 13:41:06

by Bin Liu

[permalink] [raw]
Subject: Re: [PATCH] usb: musb: Fix external abort in musb_remove

On Thu, Mar 08, 2018 at 11:17:48PM +0100, Merlijn Wajer wrote:
> Hi,
>
> On 08/03/18 22:15, Bin Liu wrote:
>
> > please add patch version numbers in the subject when necessary. This
> > helps cross-referencing.
>
> Will do. I naively assumed that the first patch would implicitly be
> number 1. Will send out v2 now.

Sorry, my bad, I forgot the first patch was a RFC. You are doing it
right.

>
> >>
> >> + musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
> >
> > Does it solve the issue if not moving this line? I'd like to have
> > minimum change if possible.
>
> Yes, it does. The only reason I moved musb_writeb is because I
> understood you wanted me to move both (per previous message: "This can
> be move down to out side of holding the spinlock").

Typically the comments would only applies to the modified code.
Otherwise the comments have to be explicit to avoid confusion.
No worries here anyway, confusion does happen sometimes ;)

Regards,
-Bin.