from:"Jonathan Liu"

Re: [v6] usb: ohci: Proper handling of ed_rm_list to handle race condition between usb_kill_urb() and finish_unlinks()

2018-03-25 Thread Jonathan Liu

Hi,

On 25 March 2018 at 12:21, Jonathan Liu <net...@gmail.com> wrote:
> On 8 February 2018 at 14:55, Jeffy Chen <jeffy.c...@rock-chips.com> wrote:
>> From: AMAN DEEP <aman.d...@samsung.com>
>>
>> There is a race condition between finish_unlinks->finish_urb() function
>> and usb_kill_urb() in ohci controller case. The finish_urb calls
>> spin_unlock(>lock) before usb_hcd_giveback_urb() function call,
>> then if during this time, usb_kill_urb is called for another endpoint,
>> then new ed will be added to ed_rm_list at beginning for unlink, and
>> ed_rm_list will point to newly added.
>>
>> When finish_urb() is completed in finish_unlinks() and ed->td_list
>> becomes empty as in below code (in finish_unlinks() function):
>>
>> if (list_empty(>td_list)) {
>> *last = ed->ed_next;
>> ed->ed_next = NULL;
>> } else if (ohci->rh_state == OHCI_RH_RUNNING) {
>> *last = ed->ed_next;
>> ed->ed_next = NULL;
>> ed_schedule(ohci, ed);
>> }
>>
>> The *last = ed->ed_next will make ed_rm_list to point to ed->ed_next
>> and previously added ed by usb_kill_urb will be left unreferenced by
>> ed_rm_list. This causes usb_kill_urb() hang forever waiting for
>> finish_unlink to remove added ed from ed_rm_list.
>>
>> The main reason for hang in this race condtion is addition and removal
>> of ed from ed_rm_list in the beginning during usb_kill_urb and later
>> last* is modified in finish_unlinks().
>>
>> As suggested by Alan Stern, the solution for proper handling of
>> ohci->ed_rm_list is to remove ed from the ed_rm_list before finishing
>> any URBs. Then at the end, we can add ed back to the list if necessary.
>>
>> This properly handle the updated ohci->ed_rm_list in usb_kill_urb().
>>
>> Fixes:977dcfdc6031("USB:OHCI:don't lose track of EDs when a controller dies")
>> Acked-by: Alan Stern <st...@rowland.harvard.edu>
>> CC: <sta...@vger.kernel.org>
>> Signed-off-by: Aman Deep <aman.d...@samsung.com>
>> Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
>> ---
>>
>> Changes in v6:
>> This is a resend of Aman Deep's v5 patch [0], which solved the hang we
>> hit [1]. (Thanks Aman :)
>>
>> The v5 has some format issues, so i slightly adjust the commit message.
>>
>> [0] https://www.spinics.net/lists/linux-usb/msg129010.html
>> [1] https://bugs.chromium.org/p/chromium/issues/detail?id=803749
>>
>>  drivers/usb/host/ohci-q.c | 17 ++---
>>  1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/host/ohci-q.c b/drivers/usb/host/ohci-q.c
>> index b2ec8c399363..4ccb85a67bb3 100644
>> --- a/drivers/usb/host/ohci-q.c
>> +++ b/drivers/usb/host/ohci-q.c
>> @@ -1019,6 +1019,8 @@ static void finish_unlinks(struct ohci_hcd *ohci)
>>  * have modified this list.  normally it's just prepending
>>  * entries (which we'd ignore), but paranoia won't hurt.
>>  */
>> +   *last = ed->ed_next;
>> +   ed->ed_next = NULL;
>> modified = 0;
>>
>> /* unlink urbs as requested, but rescan the list after
>> @@ -1077,21 +1079,22 @@ static void finish_unlinks(struct ohci_hcd *ohci)
>> goto rescan_this;
>>
>> /*
>> -* If no TDs are queued, take ED off the ed_rm_list.
>> +* If no TDs are queued, ED is now idle.
>>  * Otherwise, if the HC is running, reschedule.
>> -* If not, leave it on the list for further dequeues.
>> +* If the HC isn't running, add ED back to the
>> +* start of the list for later processing.
>>  */
>> if (list_empty(>td_list)) {
>> -   *last = ed->ed_next;
>> -   ed->ed_next = NULL;
>> ed->state = ED_IDLE;
>> list_del(>in_use_list);
>> } else if (ohci->rh_state == OHCI_RH_RUNNING) {
>> -   *last = ed->ed_next;
>> -   ed->ed_next = NULL;
>> ed_schedule(ohci, ed);
>> } else {
>> -   last = >ed_next;
>> +   ed->ed_next = ohci->ed_rm_list;
>> +

Re: [v6] usb: ohci: Proper handling of ed_rm_list to handle race condition between usb_kill_urb() and finish_unlinks()

2018-03-24 Thread Jonathan Liu

Hi,

On 25 March 2018 at 12:21, Jonathan Liu <net...@gmail.com> wrote:
> Hi,
>
> On 8 February 2018 at 14:55, Jeffy Chen <jeffy.c...@rock-chips.com> wrote:
>> From: AMAN DEEP <aman.d...@samsung.com>
>>
>> There is a race condition between finish_unlinks->finish_urb() function
>> and usb_kill_urb() in ohci controller case. The finish_urb calls
>> spin_unlock(>lock) before usb_hcd_giveback_urb() function call,
>> then if during this time, usb_kill_urb is called for another endpoint,
>> then new ed will be added to ed_rm_list at beginning for unlink, and
>> ed_rm_list will point to newly added.
>>
>> When finish_urb() is completed in finish_unlinks() and ed->td_list
>> becomes empty as in below code (in finish_unlinks() function):
>>
>> if (list_empty(>td_list)) {
>> *last = ed->ed_next;
>> ed->ed_next = NULL;
>> } else if (ohci->rh_state == OHCI_RH_RUNNING) {
>> *last = ed->ed_next;
>> ed->ed_next = NULL;
>> ed_schedule(ohci, ed);
>> }
>>
>> The *last = ed->ed_next will make ed_rm_list to point to ed->ed_next
>> and previously added ed by usb_kill_urb will be left unreferenced by
>> ed_rm_list. This causes usb_kill_urb() hang forever waiting for
>> finish_unlink to remove added ed from ed_rm_list.
>>
>> The main reason for hang in this race condtion is addition and removal
>> of ed from ed_rm_list in the beginning during usb_kill_urb and later
>> last* is modified in finish_unlinks().
>>
>> As suggested by Alan Stern, the solution for proper handling of
>> ohci->ed_rm_list is to remove ed from the ed_rm_list before finishing
>> any URBs. Then at the end, we can add ed back to the list if necessary.
>>
>> This properly handle the updated ohci->ed_rm_list in usb_kill_urb().
>>
>> Fixes:977dcfdc6031("USB:OHCI:don't lose track of EDs when a controller dies")
>> Acked-by: Alan Stern <st...@rowland.harvard.edu>
>> CC: <sta...@vger.kernel.org>
>> Signed-off-by: Aman Deep <aman.d...@samsung.com>
>> Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
>> ---
>>
>> Changes in v6:
>> This is a resend of Aman Deep's v5 patch [0], which solved the hang we
>> hit [1]. (Thanks Aman :)
>>
>> The v5 has some format issues, so i slightly adjust the commit message.
>>
>> [0] https://www.spinics.net/lists/linux-usb/msg129010.html
>> [1] https://bugs.chromium.org/p/chromium/issues/detail?id=803749
>>
>>  drivers/usb/host/ohci-q.c | 17 ++---
>>  1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/host/ohci-q.c b/drivers/usb/host/ohci-q.c
>> index b2ec8c399363..4ccb85a67bb3 100644
>> --- a/drivers/usb/host/ohci-q.c
>> +++ b/drivers/usb/host/ohci-q.c
>> @@ -1019,6 +1019,8 @@ static void finish_unlinks(struct ohci_hcd *ohci)
>>  * have modified this list.  normally it's just prepending
>>  * entries (which we'd ignore), but paranoia won't hurt.
>>  */
>> +   *last = ed->ed_next;
>> +   ed->ed_next = NULL;
>> modified = 0;
>>
>> /* unlink urbs as requested, but rescan the list after
>> @@ -1077,21 +1079,22 @@ static void finish_unlinks(struct ohci_hcd *ohci)
>> goto rescan_this;
>>
>> /*
>> -* If no TDs are queued, take ED off the ed_rm_list.
>> +* If no TDs are queued, ED is now idle.
>>  * Otherwise, if the HC is running, reschedule.
>> -* If not, leave it on the list for further dequeues.
>> +* If the HC isn't running, add ED back to the
>> +* start of the list for later processing.
>>  */
>> if (list_empty(>td_list)) {
>> -   *last = ed->ed_next;
>> -   ed->ed_next = NULL;
>> ed->state = ED_IDLE;
>> list_del(>in_use_list);
>> } else if (ohci->rh_state == OHCI_RH_RUNNING) {
>> -   *last = ed->ed_next;
>> -   ed->ed_next = NULL;
>> ed_schedule(ohci, ed);
>> } else {
>> -   last = >ed_next;
>> +   ed->ed_next = ohci->ed_rm_list;
&g

Re: [v6] usb: ohci: Proper handling of ed_rm_list to handle race condition between usb_kill_urb() and finish_unlinks()

2018-03-24 Thread Jonathan Liu

Hi,

On 8 February 2018 at 14:55, Jeffy Chen  wrote:
> From: AMAN DEEP 
>
> There is a race condition between finish_unlinks->finish_urb() function
> and usb_kill_urb() in ohci controller case. The finish_urb calls
> spin_unlock(>lock) before usb_hcd_giveback_urb() function call,
> then if during this time, usb_kill_urb is called for another endpoint,
> then new ed will be added to ed_rm_list at beginning for unlink, and
> ed_rm_list will point to newly added.
>
> When finish_urb() is completed in finish_unlinks() and ed->td_list
> becomes empty as in below code (in finish_unlinks() function):
>
> if (list_empty(>td_list)) {
> *last = ed->ed_next;
> ed->ed_next = NULL;
> } else if (ohci->rh_state == OHCI_RH_RUNNING) {
> *last = ed->ed_next;
> ed->ed_next = NULL;
> ed_schedule(ohci, ed);
> }
>
> The *last = ed->ed_next will make ed_rm_list to point to ed->ed_next
> and previously added ed by usb_kill_urb will be left unreferenced by
> ed_rm_list. This causes usb_kill_urb() hang forever waiting for
> finish_unlink to remove added ed from ed_rm_list.
>
> The main reason for hang in this race condtion is addition and removal
> of ed from ed_rm_list in the beginning during usb_kill_urb and later
> last* is modified in finish_unlinks().
>
> As suggested by Alan Stern, the solution for proper handling of
> ohci->ed_rm_list is to remove ed from the ed_rm_list before finishing
> any URBs. Then at the end, we can add ed back to the list if necessary.
>
> This properly handle the updated ohci->ed_rm_list in usb_kill_urb().
>
> Fixes:977dcfdc6031("USB:OHCI:don't lose track of EDs when a controller dies")
> Acked-by: Alan Stern 
> CC: 
> Signed-off-by: Aman Deep 
> Signed-off-by: Jeffy Chen 
> ---
>
> Changes in v6:
> This is a resend of Aman Deep's v5 patch [0], which solved the hang we
> hit [1]. (Thanks Aman :)
>
> The v5 has some format issues, so i slightly adjust the commit message.
>
> [0] https://www.spinics.net/lists/linux-usb/msg129010.html
> [1] https://bugs.chromium.org/p/chromium/issues/detail?id=803749
>
>  drivers/usb/host/ohci-q.c | 17 ++---
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/host/ohci-q.c b/drivers/usb/host/ohci-q.c
> index b2ec8c399363..4ccb85a67bb3 100644
> --- a/drivers/usb/host/ohci-q.c
> +++ b/drivers/usb/host/ohci-q.c
> @@ -1019,6 +1019,8 @@ static void finish_unlinks(struct ohci_hcd *ohci)
>  * have modified this list.  normally it's just prepending
>  * entries (which we'd ignore), but paranoia won't hurt.
>  */
> +   *last = ed->ed_next;
> +   ed->ed_next = NULL;
> modified = 0;
>
> /* unlink urbs as requested, but rescan the list after
> @@ -1077,21 +1079,22 @@ static void finish_unlinks(struct ohci_hcd *ohci)
> goto rescan_this;
>
> /*
> -* If no TDs are queued, take ED off the ed_rm_list.
> +* If no TDs are queued, ED is now idle.
>  * Otherwise, if the HC is running, reschedule.
> -* If not, leave it on the list for further dequeues.
> +* If the HC isn't running, add ED back to the
> +* start of the list for later processing.
>  */
> if (list_empty(>td_list)) {
> -   *last = ed->ed_next;
> -   ed->ed_next = NULL;
> ed->state = ED_IDLE;
> list_del(>in_use_list);
> } else if (ohci->rh_state == OHCI_RH_RUNNING) {
> -   *last = ed->ed_next;
> -   ed->ed_next = NULL;
> ed_schedule(ohci, ed);
> } else {
> -   last = >ed_next;
> +   ed->ed_next = ohci->ed_rm_list;
> +   ohci->ed_rm_list = ed;
> +   /* Don't loop on the same ED */
> +   if (last == >ed_rm_list)
> +   last = >ed_next;
> }
>
> if (modified)

I am experiencing a USB function call hang from userspace with OCHI
(full speed USB device) after updating from Linux 4.14.15 to 4.14.24
and noticed this commit.

Here is the Linux 4.14.24 kernel stack trace (extracted from SysRq+w
and amended with addr2line):
[] (__schedule) from [] (schedule+0x50/0xb4)
kernel/sched/core.c:2792
[] (schedule) from []
(usb_kill_urb.part.3+0x78/0xa8) include/asm-generic/preempt.h:59
[] (usb_kill_urb.part.3) from []
(usbdev_ioctl+0x1288/0x1cf0) drivers/usb/core/urb.c:690
[] (usbdev_ioctl) from []
(do_vfs_ioctl+0x9c/0x8ec) drivers/usb/core/devio.c:1835

Re: [PATCH v2] usb: musb: sunxi: Explicitly release USB PHY on exit

2017-10-16 Thread Jonathan Liu

On 16 October 2017 at 23:49, Bin Liu <b-...@ti.com> wrote:
> On Mon, Oct 16, 2017 at 04:13:51PM +1100, Jonathan Liu wrote:
>> On 10 October 2017 at 14:22, Bin Liu <b-...@ti.com> wrote:
>> > On Tue, Oct 10, 2017 at 01:45:25PM +1100, Jonathan Liu wrote:
>> >> This fixes a kernel oops when unloading the driver due to usb_put_phy
>> >> being called after usb_phy_generic_unregister when the device is
>> >> detached. Calling usb_phy_generic_unregister causes x->dev->driver to
>> >> be NULL in usb_put_phy and results in a NULL pointer dereference.
>> >>
>> >> Cc: sta...@vger.kernel.org # v4.3+
>> >> Signed-off-by: Jonathan Liu <net...@gmail.com>
>> >> ---
>> >> Changes for v2:
>> >>  - Use devm_usb_put_phy instead of usb_put_phy
>> >>
>> >>  drivers/usb/musb/sunxi.c | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >>
>> >> diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
>> >> index c9a09b5bb6e5..dc353e24d53c 100644
>> >> --- a/drivers/usb/musb/sunxi.c
>> >> +++ b/drivers/usb/musb/sunxi.c
>> >> @@ -297,6 +297,8 @@ static int sunxi_musb_exit(struct musb *musb)
>> >>   if (test_bit(SUNXI_MUSB_FL_HAS_SRAM, >flags))
>> >>   sunxi_sram_release(musb->controller->parent);
>> >>
>> >> + devm_usb_put_phy(glue->dev, glue->xceiv);
>> >> +
>> >>   return 0;
>> >>  }
>> >
>> >
>>
>> > Applied. Thanks.
>> > -Bin.
>>
>> Which repository was it applied to?
>
> I don't have a public repo (yet), but the patch has been sent to Greg,
> so it should be merged into the next -rc.
>
> Regards,
> -Bin.

The MAINTAINERS file has
git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git listed as
the tree for drivers/usb/musb/. I guess that should be updated.

Regards,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: musb: sunxi: Explicitly release USB PHY on exit

2017-10-15 Thread Jonathan Liu

On 10 October 2017 at 14:22, Bin Liu <b-...@ti.com> wrote:
> On Tue, Oct 10, 2017 at 01:45:25PM +1100, Jonathan Liu wrote:
>> This fixes a kernel oops when unloading the driver due to usb_put_phy
>> being called after usb_phy_generic_unregister when the device is
>> detached. Calling usb_phy_generic_unregister causes x->dev->driver to
>> be NULL in usb_put_phy and results in a NULL pointer dereference.
>>
>> Cc: sta...@vger.kernel.org # v4.3+
>> Signed-off-by: Jonathan Liu <net...@gmail.com>
>> ---
>> Changes for v2:
>>  - Use devm_usb_put_phy instead of usb_put_phy
>>
>>  drivers/usb/musb/sunxi.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
>> index c9a09b5bb6e5..dc353e24d53c 100644
>> --- a/drivers/usb/musb/sunxi.c
>> +++ b/drivers/usb/musb/sunxi.c
>> @@ -297,6 +297,8 @@ static int sunxi_musb_exit(struct musb *musb)
>>   if (test_bit(SUNXI_MUSB_FL_HAS_SRAM, >flags))
>>   sunxi_sram_release(musb->controller->parent);
>>
>> + devm_usb_put_phy(glue->dev, glue->xceiv);
>> +
>>   return 0;
>>  }
>
>

> Applied. Thanks.
> -Bin.

Which repository was it applied to?

Regards,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] usb: musb: sunxi: Explicitly release USB PHY on exit

2017-10-09 Thread Jonathan Liu

This fixes a kernel oops when unloading the driver due to usb_put_phy
being called after usb_phy_generic_unregister when the device is
detached. Calling usb_phy_generic_unregister causes x->dev->driver to
be NULL in usb_put_phy and results in a NULL pointer dereference.

Cc: sta...@vger.kernel.org # v4.3+
Signed-off-by: Jonathan Liu <net...@gmail.com>
---
Changes for v2:
 - Use devm_usb_put_phy instead of usb_put_phy

 drivers/usb/musb/sunxi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
index c9a09b5bb6e5..dc353e24d53c 100644
--- a/drivers/usb/musb/sunxi.c
+++ b/drivers/usb/musb/sunxi.c
@@ -297,6 +297,8 @@ static int sunxi_musb_exit(struct musb *musb)
if (test_bit(SUNXI_MUSB_FL_HAS_SRAM, >flags))
sunxi_sram_release(musb->controller->parent);
 
+   devm_usb_put_phy(glue->dev, glue->xceiv);
+
return 0;
 }
 
-- 
2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: musb: sunxi: Explicitly release USB PHY on exit

2017-10-09 Thread Jonathan Liu

Hi,

On 10 October 2017 at 13:14, Bin Liu <b-...@ti.com> wrote:
> On Tue, Oct 10, 2017 at 08:17:34AM +1100, Jonathan Liu wrote:
>> Hi,
>>
>> On 10 October 2017 at 01:23, Bin Liu <b-...@ti.com> wrote:
>> > Hi,
>> >
>> > On Tue, Sep 26, 2017 at 09:39:23PM +1000, Jonathan Liu wrote:
>> >> This fixes a kernel oops when unloading the driver due to usb_put_phy
>> >> being called after usb_phy_generic_unregister when the device is
>> >> detached. Calling usb_phy_generic_unregister causes x->dev->driver to
>> >> be NULL in usb_put_phy and results in a NULL pointer dereference.
>> >>
>> >> As we are now explicitly managing the lifetime of usb_phy,
>> >> devm_usb_get_phy is changed to usb_get_phy and we call usb_put_phy in
>> >> the probe error path.
>> >>
>> >> Cc: sta...@vger.kernel.org # v4.3+
>> >> Signed-off-by: Jonathan Liu <net...@gmail.com>
>> >> ---
>> >>  drivers/usb/musb/sunxi.c | 8 ++--
>> >>  1 file changed, 6 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
>> >> index c9a09b5bb6e5..8eed61042103 100644
>> >> --- a/drivers/usb/musb/sunxi.c
>> >> +++ b/drivers/usb/musb/sunxi.c
>> >> @@ -297,6 +297,8 @@ static int sunxi_musb_exit(struct musb *musb)
>> >>   if (test_bit(SUNXI_MUSB_FL_HAS_SRAM, >flags))
>> >>   sunxi_sram_release(musb->controller->parent);
>> >>
>> >> + usb_put_phy(glue->xceiv);
>> >
>> > Will devm_usb_put_phy() solve the issue too? Then you don't need the
>> > changes in sunxi_musb_probe() below.
>> >
>>
>> devm_usb_put_phy does solve the issue without changes in
>> sunxi_musb_probe but I think it is clearer that the lifetime is
>> explicitly managed if we don't mix implicit and explicit freeing of
>> resources.
>
> Then what is the purpose of the devm_* API?
>
> The original bug was that devm_usb_get_phy() is called in _probe, but
> devm_usb_put_phy() was never called, which caused the device ref count
> unbalanced. It is not related to implicit/explicit resource management.
>
> devm_* is recommended whenever applicable.
>

Ok. Will use devm_usb_put_phy in v2 patch.

> Regards,
> -Bin.
>

Regards,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: musb: sunxi: Explicitly release USB PHY on exit

2017-10-09 Thread Jonathan Liu

Hi,

On 10 October 2017 at 01:23, Bin Liu <b-...@ti.com> wrote:
> Hi,
>
> On Tue, Sep 26, 2017 at 09:39:23PM +1000, Jonathan Liu wrote:
>> This fixes a kernel oops when unloading the driver due to usb_put_phy
>> being called after usb_phy_generic_unregister when the device is
>> detached. Calling usb_phy_generic_unregister causes x->dev->driver to
>> be NULL in usb_put_phy and results in a NULL pointer dereference.
>>
>> As we are now explicitly managing the lifetime of usb_phy,
>> devm_usb_get_phy is changed to usb_get_phy and we call usb_put_phy in
>> the probe error path.
>>
>> Cc: sta...@vger.kernel.org # v4.3+
>> Signed-off-by: Jonathan Liu <net...@gmail.com>
>> ---
>>  drivers/usb/musb/sunxi.c | 8 ++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
>> index c9a09b5bb6e5..8eed61042103 100644
>> --- a/drivers/usb/musb/sunxi.c
>> +++ b/drivers/usb/musb/sunxi.c
>> @@ -297,6 +297,8 @@ static int sunxi_musb_exit(struct musb *musb)
>>   if (test_bit(SUNXI_MUSB_FL_HAS_SRAM, >flags))
>>   sunxi_sram_release(musb->controller->parent);
>>
>> + usb_put_phy(glue->xceiv);
>
> Will devm_usb_put_phy() solve the issue too? Then you don't need the
> changes in sunxi_musb_probe() below.
>

devm_usb_put_phy does solve the issue without changes in
sunxi_musb_probe but I think it is clearer that the lifetime is
explicitly managed if we don't mix implicit and explicit freeing of
resources.

>> +
>>   return 0;
>>  }
>>
>> @@ -781,7 +783,7 @@ static int sunxi_musb_probe(struct platform_device *pdev)
>>   return PTR_ERR(glue->usb_phy);
>>   }
>>
>> - glue->xceiv = devm_usb_get_phy(>dev, USB_PHY_TYPE_USB2);
>> + glue->xceiv = usb_get_phy(USB_PHY_TYPE_USB2);
>>   if (IS_ERR(glue->xceiv)) {
>>   ret = PTR_ERR(glue->xceiv);
>>   dev_err(>dev, "Error getting usb-phy %d\n", ret);
>> @@ -803,11 +805,13 @@ static int sunxi_musb_probe(struct platform_device 
>> *pdev)
>>   if (IS_ERR(glue->musb_pdev)) {
>>   ret = PTR_ERR(glue->musb_pdev);
>>   dev_err(>dev, "Error registering musb dev: %d\n", ret);
>> - goto err_unregister_usb_phy;
>> + goto err_put_usb_phy;
>>   }
>>
>>   return 0;
>>
>> +err_put_usb_phy:
>> + usb_put_phy(glue->xceiv);
>>  err_unregister_usb_phy:
>>   usb_phy_generic_unregister(glue->usb_phy);
>>   return ret;
>> --
>> 2.13.2
>
> Regards,
> -Bin.

Regards,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: musb: Check for host-mode using is_host_active() on reset interrupt

2017-09-27 Thread Jonathan Liu

The sunxi musb has a bug where sometimes it will generate a babble
error on device disconnect instead of a disconnect IRQ. When this
happens the musb controller switches from host mode to device mode
(it clears MUSB_DEVCTL_HM/MUSB_DEVCTL_SESSION and sets
MUSB_DEVCTL_BDEVICE) and gets stuck in this state.

The babble error is misdetected as a bus reset because MUSB_DEVCTL_HM
was cleared.

To fix this, use is_host_active() rather than (devctl & MUSB_DEVCTL_HM)
to detect babble error so that sunxi musb babble recovery can handle it
by restoring the mode. This information is provided by the driver logic
and does not rely on register contents.

Cc: sta...@vger.kernel.org # v4.1+
Signed-off-by: Jonathan Liu <net...@gmail.com>
---
 drivers/usb/musb/musb_core.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 029692053dd3..1bce7df3e110 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -906,7 +906,7 @@ static irqreturn_t musb_stage0_irq(struct musb *musb, u8 
int_usb,
 */
if (int_usb & MUSB_INTR_RESET) {
handled = IRQ_HANDLED;
-   if (devctl & MUSB_DEVCTL_HM) {
+   if (is_host_active(musb)) {
/*
 * When BABBLE happens what we can depends on which
 * platform MUSB is running, because some platforms
@@ -916,9 +916,7 @@ static irqreturn_t musb_stage0_irq(struct musb *musb, u8 
int_usb,
 * drop the session.
 */
dev_err(musb->controller, "Babble\n");
-
-   if (is_host_active(musb))
-   musb_recover_from_babble(musb);
+   musb_recover_from_babble(musb);
} else {
musb_dbg(musb, "BUS RESET as %s",
usb_otg_state_string(musb->xceiv->otg->state));
-- 
2.13.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: musb: sunxi: Explicitly release USB PHY on exit

2017-09-26 Thread Jonathan Liu

Hi,

On 26 September 2017 at 21:39, Jonathan Liu <net...@gmail.com> wrote:
> This fixes a kernel oops when unloading the driver due to usb_put_phy
> being called after usb_phy_generic_unregister when the device is
> detached. Calling usb_phy_generic_unregister causes x->dev->driver to
> be NULL in usb_put_phy and results in a NULL pointer dereference.
>
> As we are now explicitly managing the lifetime of usb_phy,
> devm_usb_get_phy is changed to usb_get_phy and we call usb_put_phy in
> the probe error path.
>
> Cc: sta...@vger.kernel.org # v4.3+
> Signed-off-by: Jonathan Liu <net...@gmail.com>
> ---
>  drivers/usb/musb/sunxi.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
> index c9a09b5bb6e5..8eed61042103 100644
> --- a/drivers/usb/musb/sunxi.c
> +++ b/drivers/usb/musb/sunxi.c
> @@ -297,6 +297,8 @@ static int sunxi_musb_exit(struct musb *musb)
> if (test_bit(SUNXI_MUSB_FL_HAS_SRAM, >flags))
> sunxi_sram_release(musb->controller->parent);
>
> +   usb_put_phy(glue->xceiv);
> +
> return 0;
>  }
>
> @@ -781,7 +783,7 @@ static int sunxi_musb_probe(struct platform_device *pdev)
> return PTR_ERR(glue->usb_phy);
> }
>
> -   glue->xceiv = devm_usb_get_phy(>dev, USB_PHY_TYPE_USB2);
> +   glue->xceiv = usb_get_phy(USB_PHY_TYPE_USB2);
> if (IS_ERR(glue->xceiv)) {
> ret = PTR_ERR(glue->xceiv);
> dev_err(>dev, "Error getting usb-phy %d\n", ret);
> @@ -803,11 +805,13 @@ static int sunxi_musb_probe(struct platform_device 
> *pdev)
> if (IS_ERR(glue->musb_pdev)) {
> ret = PTR_ERR(glue->musb_pdev);
> dev_err(>dev, "Error registering musb dev: %d\n", ret);
> -   goto err_unregister_usb_phy;
> +   goto err_put_usb_phy;
> }
>
> return 0;
>
> +err_put_usb_phy:
> +   usb_put_phy(glue->xceiv);
>  err_unregister_usb_phy:
> usb_phy_generic_unregister(glue->usb_phy);
> return ret;
> --
> 2.13.2
>

Another way I could fix this would be to check if x->dev->driver is
NULL before dereferencing in usb_put_phy but I am not sure if this
breaks any expectations in the USB subsystem...

Regards,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: musb: sunxi: Explicitly release USB PHY on exit

2017-09-26 Thread Jonathan Liu

This fixes a kernel oops when unloading the driver due to usb_put_phy
being called after usb_phy_generic_unregister when the device is
detached. Calling usb_phy_generic_unregister causes x->dev->driver to
be NULL in usb_put_phy and results in a NULL pointer dereference.

As we are now explicitly managing the lifetime of usb_phy,
devm_usb_get_phy is changed to usb_get_phy and we call usb_put_phy in
the probe error path.

Cc: sta...@vger.kernel.org # v4.3+
Signed-off-by: Jonathan Liu <net...@gmail.com>
---
 drivers/usb/musb/sunxi.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
index c9a09b5bb6e5..8eed61042103 100644
--- a/drivers/usb/musb/sunxi.c
+++ b/drivers/usb/musb/sunxi.c
@@ -297,6 +297,8 @@ static int sunxi_musb_exit(struct musb *musb)
if (test_bit(SUNXI_MUSB_FL_HAS_SRAM, >flags))
sunxi_sram_release(musb->controller->parent);
 
+   usb_put_phy(glue->xceiv);
+
return 0;
 }
 
@@ -781,7 +783,7 @@ static int sunxi_musb_probe(struct platform_device *pdev)
return PTR_ERR(glue->usb_phy);
}
 
-   glue->xceiv = devm_usb_get_phy(>dev, USB_PHY_TYPE_USB2);
+   glue->xceiv = usb_get_phy(USB_PHY_TYPE_USB2);
if (IS_ERR(glue->xceiv)) {
ret = PTR_ERR(glue->xceiv);
dev_err(>dev, "Error getting usb-phy %d\n", ret);
@@ -803,11 +805,13 @@ static int sunxi_musb_probe(struct platform_device *pdev)
if (IS_ERR(glue->musb_pdev)) {
ret = PTR_ERR(glue->musb_pdev);
dev_err(>dev, "Error registering musb dev: %d\n", ret);
-   goto err_unregister_usb_phy;
+   goto err_put_usb_phy;
}
 
return 0;
 
+err_put_usb_phy:
+   usb_put_phy(glue->xceiv);
 err_unregister_usb_phy:
usb_phy_generic_unregister(glue->usb_phy);
return ret;
-- 
2.13.2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-sunxi] [PATCH v5 2/7] phy-sun4i-usb: Add extcon support for the otg phy (phy0)

2015-07-31 Thread Jonathan Liu


Hi Hans,

On 13/06/2015 10:37 PM, Hans de Goede wrote:

The sunxi musb glue needs to know if a host or normal usb cable is plugged
in, add extcon support so that the musb glue can monitor the host status.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
Changes in v5:
-Split out of the Add id and vbus detection support commit
-Ported to the new extcon API queued for 4.2
---
  drivers/phy/Kconfig |  1 +
  drivers/phy/phy-sun4i-usb.c | 32 +++-
  2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index a53bd5b..9841780 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -173,6 +173,7 @@ config PHY_SUN4I_USB
tristate Allwinner sunxi SoC USB PHY driver
depends on ARCH_SUNXI  HAS_IOMEM  OF
depends on RESET_CONTROLLER
+   depends on EXTCON
select GENERIC_PHY
help
  Enable this to support the transceiver that is part of Allwinner

Should probably add CONFIG_EXTCON=y to arch/arm/configs/sunxi_defconfig 
so that CONFIG_PHY_SUN4I_USB=y works out of the box.


Regards,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v6] usb: ohci: Proper handling of ed_rm_list to handle race condition between usb_kill_urb() and finish_unlinks()

Re: [v6] usb: ohci: Proper handling of ed_rm_list to handle race condition between usb_kill_urb() and finish_unlinks()

Re: [v6] usb: ohci: Proper handling of ed_rm_list to handle race condition between usb_kill_urb() and finish_unlinks()

Re: [PATCH v2] usb: musb: sunxi: Explicitly release USB PHY on exit

Re: [PATCH v2] usb: musb: sunxi: Explicitly release USB PHY on exit

[PATCH v2] usb: musb: sunxi: Explicitly release USB PHY on exit

Re: [PATCH] usb: musb: sunxi: Explicitly release USB PHY on exit

Re: [PATCH] usb: musb: sunxi: Explicitly release USB PHY on exit

[PATCH] usb: musb: Check for host-mode using is_host_active() on reset interrupt

Re: [PATCH] usb: musb: sunxi: Explicitly release USB PHY on exit

[PATCH] usb: musb: sunxi: Explicitly release USB PHY on exit

Re: [linux-sunxi] [PATCH v5 2/7] phy-sun4i-usb: Add extcon support for the otg phy (phy0)

12 matches

Site Navigation

Mail list logo

Footer information