From: Yang Wei <[email protected]>
While loading g_mass_storage module, the following warning is triggered.
In fact, it is more easy to reproduce it with RT kernel.
WARNING: at drivers/usb/gadget/composite.c:
usb_composite_setup_continue: Unexpected call
Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
[<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
[<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
[<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
[<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
[<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
[<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
[<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
[<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
The root cause just likes the following scenario.
irq thread
composite_disconnect()
|
|->fsg_disable() fsg->common->new_fsg = NULL
and then wake fsg_main_thread
with seting common->state to
FSG_STATE_CONFIG_CHANGE.
fsg_main_thread
|
|->do_set_interface()
irq thread
set_config()
|
|->fsg_set_alt() fsg->common->new_fsg = new_fsg
and then also wake up fsg_main_thread
with setting common->state to
FSG_STATE_CONFIG_CHANGE.
|-> if(common->new_fsg)
usb_composite_setup_continue()
In this case, fsg_main_thread would invoke usb_composite_setup_continue
twice, so the second call would trigger the above call trace, as we also
save common->new_fsg while changing the common->state.
Signed-off-by: Yang Wei <[email protected]>
---
drivers/usb/gadget/f_mass_storage.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Hi All,
This patch is based on git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-next branch,
and is validated it with altera cyclone V board.
Thanks
Wei
diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index b963939..e3b1798 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2342,6 +2342,7 @@ static void handle_exception(struct fsg_common *common)
struct fsg_buffhd *bh;
enum fsg_state old_state;
struct fsg_lun *curlun;
+ struct fsg_dev *new;
unsigned int exception_req_tag;
/*
@@ -2421,6 +2422,7 @@ static void handle_exception(struct fsg_common *common)
}
common->state = FSG_STATE_IDLE;
}
+ new = common->new_fsg;
spin_unlock_irq(&common->lock);
/* Carry out any extra actions required for the exception */
@@ -2460,8 +2462,8 @@ static void handle_exception(struct fsg_common *common)
break;
case FSG_STATE_CONFIG_CHANGE:
- do_set_interface(common, common->new_fsg);
- if (common->new_fsg)
+ do_set_interface(common, new);
+ if (new)
usb_composite_setup_continue(common->cdev);
break;
--
1.7.9.5
On Tue, 3 Jun 2014 [email protected] wrote:
> From: Yang Wei <[email protected]>
>
> While loading g_mass_storage module, the following warning is triggered.
> In fact, it is more easy to reproduce it with RT kernel.
>
> WARNING: at drivers/usb/gadget/composite.c:
> usb_composite_setup_continue: Unexpected call
> Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
> [<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
> [<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
> [<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
> [<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
> [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
> [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
> [<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
>
> The root cause just likes the following scenario.
>
> irq thread
>
> composite_disconnect()
> |
> |->fsg_disable() fsg->common->new_fsg = NULL
> and then wake fsg_main_thread
> with seting common->state to
> FSG_STATE_CONFIG_CHANGE.
> fsg_main_thread
> |
> |->do_set_interface()
> irq thread
>
> set_config()
> |
> |->fsg_set_alt() fsg->common->new_fsg = new_fsg
> and then also wake up fsg_main_thread
> with setting common->state to
> FSG_STATE_CONFIG_CHANGE.
> |-> if(common->new_fsg)
> usb_composite_setup_continue()
>
> In this case, fsg_main_thread would invoke usb_composite_setup_continue
> twice, so the second call would trigger the above call trace, as we also
> save common->new_fsg while changing the common->state.
Michal and Andrzej:
I haven't paid much attention to these matters, because you handled the
conversion from g_file_storage to f_mass_storage using the composite
framework. But this patch seemed odd, so I took a closer look.
In f_mass_storage.c, struct fsg_common is shared among all the function
instances. This structure includes things like cmnd and nluns, which
will in general be different for different functions.
That's okay if each function is in a separate config, but what happens
when there are multiple functions in the same config, using different
interfaces? What if the host sends concurrent commands to two of these
functions?
Am I missing something?
Alan Stern
On 06/03/2014 10:48 PM, Alan Stern wrote:
> On Tue, 3 Jun 2014 [email protected] wrote:
>
>> From: Yang Wei <[email protected]>
>>
>> While loading g_mass_storage module, the following warning is triggered.
>> In fact, it is more easy to reproduce it with RT kernel.
>>
>> WARNING: at drivers/usb/gadget/composite.c:
>> usb_composite_setup_continue: Unexpected call
>> Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
>> [<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
>> [<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
>> [<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
>> [<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
>> [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
>> [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
>> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
>> [<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
>>
>> The root cause just likes the following scenario.
>>
>> irq thread
>>
>> composite_disconnect()
>> |
>> |->fsg_disable() fsg->common->new_fsg = NULL
>> and then wake fsg_main_thread
>> with seting common->state to
>> FSG_STATE_CONFIG_CHANGE.
>> fsg_main_thread
>> |
>> |->do_set_interface()
>> irq thread
>>
>> set_config()
>> |
>> |->fsg_set_alt() fsg->common->new_fsg = new_fsg
>> and then also wake up fsg_main_thread
>> with setting common->state to
>> FSG_STATE_CONFIG_CHANGE.
>> |-> if(common->new_fsg)
>> usb_composite_setup_continue()
>>
>> In this case, fsg_main_thread would invoke usb_composite_setup_continue
>> twice, so the second call would trigger the above call trace, as we also
>> save common->new_fsg while changing the common->state.
> Michal and Andrzej:
>
> I haven't paid much attention to these matters, because you handled the
> conversion from g_file_storage to f_mass_storage using the composite
> framework. But this patch seemed odd, so I took a closer look.
Let me make sense it, Robert once introduced the following patch to fix
disconnect handling of s3c-hsotg.
commit d18f7116a5ddb8263fe62b05ad63e5ceb5875791
Author: Robert Baldyga <[email protected]>
Date: Thu Nov 21 13:49:18 2013 +0100
usb: gadget: s3c-hsotg: fix disconnect handling
This patch moves s3c_hsotg_disconnect function call from USBSusp
interrupt
handler to SET_ADDRESS request handler.
It's because disconnected state can't be detected directly, because
this
hardware doesn't support Disconnected interrupt for device mode.
For both
Suspend and Disconnect events there is one interrupt USBSusp, but
calling
s3c_hsotg_disconnect from this interrupt handler causes config reset in
composite layer, which is not undesirable for Suspended state.
For this reason s3c_hsotg_disconnect is called from SET_ADDRESS request
handler, which occurs always after disconnection, so we do disconnect
immediately before we are connected again. It's probably only way we
can do handle disconnection correctly.
Signed-off-by: Robert Baldyga <[email protected]>
Signed-off-by: Kyungmin Park <[email protected]>
Signed-off-by: Felipe Balbi <[email protected]>
Just like what the commit log described, s3c_hsotg_disconnect is called
from SET_ADDRESS request handler, therefore,
reset_config would finally be called, it raises a
FSG_STATE_CONFIG_CHANGE exception and wakes up fsg_main_thread to
handle this exception. After handling SET_ADDRESS, subsequently the irq
hanler of s3c-hsotg would also invokes composite_setup()
function to handle USB_REQ_SET_CONFIGURATION request, set_config would
be invoked, it also raises a FSG_STATE_CONFIG_CHANGE
exception and wakes up fsg_main_thread, in mean time,
cdev->delayed_status would be plus one. Right? If I am missing
something, please
let me know it.:-) If so, the following scenario would trigger the above
call trace.
irq handler
|
|-> s3c_hsotg_disconnect()
|
|-> common->new_fsg = NULL
|-> common->state to FSG_STATE_CONFIG.
|-> wakes up fsg_main_thread.
|-> set USB device address
fsg_main_thread finds the common->state == FSG_STATE_CONFIG
|
|-> handle_execption
|
|-> set common->state to FSG_STATE_IDLE
irq hanppens ------------>|
irq handler needs to hanle USB_REQ_SET_CONFIGURATION request.
|->do_set_interface()
|-> set_config()
|
|-> common->new_fsg = new_fsg;
|-> common->state = FSG_STATE_CONFIG
|-> cdev->delayed_status++
|-> wakes up fsg_main_thread
|-> Now the common->state == FSG_STATE_CONFIG
|-> if(common->new_fsg)
usb_composite_setup_continue()
|->cdev->delayed_status--
|
fsg_main_thread finds the common->state still is FSG_STATE_CONFIG,
| so it would invoke handle_execption again.
|->hanle_execption
|-> set common->state to FSG_STATE_IDLE
|-> do_set_interface()
|-> if (common->new_fsg)
usb_composite_setup_continue()
|-> cdev->delayed_status ==
0, so
this warning is triggered.
Thanks
Wei
>
> In f_mass_storage.c, struct fsg_common is shared among all the function
> instances. This structure includes things like cmnd and nluns, which
> will in general be different for different functions.
>
> That's okay if each function is in a separate config, but what happens
> when there are multiple functions in the same config, using different
> interfaces? What if the host sends concurrent commands to two of these
> functions?
>
> Am I missing something?
>
> Alan Stern
>
>
>
>
> commit d18f7116a5ddb8263fe62b05ad63e5ceb5875791
> Author: Robert Baldyga <[email protected]>
> Date: Thu Nov 21 13:49:18 2013 +0100
>
> usb: gadget: s3c-hsotg: fix disconnect handling
>
> This patch moves s3c_hsotg_disconnect function call from USBSusp
> interrupt
> handler to SET_ADDRESS request handler.
>
It is a little strange we call gadget's disconnect at SET_ADDRESS?
How the udc calls gadget driver the disconnection has happened when
the usb cable is disconnected from the host?
Usually, we call gadget's disconnect at two situations
- udc's reset handler if udc's speed is not UNKNOWN, it is usually happened
when the host sends reset after enumeration.
- udc's disconnect handler, it is usually happened when the usb cable
is disconnected from host.
Peter
Guys,
It seems the previous description is out of order. I describe it again.
Sorry for it.
irq handler
|
|-> s3c_hsotg_disconnect()
|
|-> common->new_fsg = NULL
|-> common->state = FSG_STATE_CONFIG
|-> wakes up fsg_main_thread.
|->set USB device address.
fsg_main_thread
|
|-> handle_exception
|
|-> common->state = FSG_STATE_IDLE
|-> do_set_interface()
irq happens -------------->
irq handler needs to handle USB_REQ_SET_CONFIGURATION request
|
|-> set_config()
|
|-> common->new_fsg = new_fsg;
|-> common->state = FSG_STATE_CONFIG
|-> cdev->delayed_status++
|-> wakes up fsg_main_thread
fsg_main_thread
|
|-> if(common->new_fsg)
|-> usb_composite_setup_continue()
|-> cdev->delayed_status--
|-> fsg_main_thread still finds the common->state is equal
to FSG_STATE_IDLE
|-> so it invokes handle_exception again, subsequently the
usb_composite_setup_continue
|-> is executed again. It would fininally results in the
warning.
Thanks
Wei
On 06/04/2014 09:20 AM, Yang,Wei wrote:
> On 06/03/2014 10:48 PM, Alan Stern wrote:
>> On Tue, 3 Jun 2014 [email protected] wrote:
>>
>>> From: Yang Wei <[email protected]>
>>>
>>> While loading g_mass_storage module, the following warning is
>>> triggered.
>>> In fact, it is more easy to reproduce it with RT kernel.
>>>
>>> WARNING: at drivers/usb/gadget/composite.c:
>>> usb_composite_setup_continue: Unexpected call
>>> Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1
>>> g_mass_storage
>>> [<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>]
>>> (dump_stack+0x20/0x24)
>>> [<80619608>] (dump_stack+0x20/0x24) from [<80025100>]
>>> (warn_slowpath_common+0x64/0x74)
>>> [<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>]
>>> (warn_slowpath_fmt+0x40/0x48)
>>> [<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>]
>>> (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
>>> [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc
>>> [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4
>>> [g_mass_storage])
>>> [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from
>>> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
>>> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from
>>> [<8004bc90>] (kthread+0x98/0x9c)
>>> [<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>]
>>> (kernel_thread_exit+0x0/0x8)
>>>
>>> The root cause just likes the following scenario.
>>>
>>> irq thread
>>>
>>> composite_disconnect()
>>> |
>>> |->fsg_disable() fsg->common->new_fsg = NULL
>>> and then wake fsg_main_thread
>>> with seting common->state to
>>> FSG_STATE_CONFIG_CHANGE.
>>> fsg_main_thread
>>> |
>>> |->do_set_interface()
>>> irq thread
>>>
>>> set_config()
>>> |
>>> |->fsg_set_alt() fsg->common->new_fsg = new_fsg
>>> and then also wake up fsg_main_thread
>>> with setting common->state to
>>> FSG_STATE_CONFIG_CHANGE.
>>> |->
>>> if(common->new_fsg)
>>> usb_composite_setup_continue()
>>>
>>> In this case, fsg_main_thread would invoke usb_composite_setup_continue
>>> twice, so the second call would trigger the above call trace, as we
>>> also
>>> save common->new_fsg while changing the common->state.
>> Michal and Andrzej:
>>
>> I haven't paid much attention to these matters, because you handled the
>> conversion from g_file_storage to f_mass_storage using the composite
>> framework. But this patch seemed odd, so I took a closer look.
>
> Let me make sense it, Robert once introduced the following patch to
> fix disconnect handling of s3c-hsotg.
>
> commit d18f7116a5ddb8263fe62b05ad63e5ceb5875791
> Author: Robert Baldyga <[email protected]>
> Date: Thu Nov 21 13:49:18 2013 +0100
>
> usb: gadget: s3c-hsotg: fix disconnect handling
>
> This patch moves s3c_hsotg_disconnect function call from USBSusp
> interrupt
> handler to SET_ADDRESS request handler.
>
> It's because disconnected state can't be detected directly,
> because this
> hardware doesn't support Disconnected interrupt for device mode.
> For both
> Suspend and Disconnect events there is one interrupt USBSusp, but
> calling
> s3c_hsotg_disconnect from this interrupt handler causes config
> reset in
> composite layer, which is not undesirable for Suspended state.
>
> For this reason s3c_hsotg_disconnect is called from SET_ADDRESS
> request
> handler, which occurs always after disconnection, so we do disconnect
> immediately before we are connected again. It's probably only way we
> can do handle disconnection correctly.
>
> Signed-off-by: Robert Baldyga <[email protected]>
> Signed-off-by: Kyungmin Park <[email protected]>
> Signed-off-by: Felipe Balbi <[email protected]>
>
> Just like what the commit log described, s3c_hsotg_disconnect is
> called from SET_ADDRESS request handler, therefore,
> reset_config would finally be called, it raises a
> FSG_STATE_CONFIG_CHANGE exception and wakes up fsg_main_thread to
> handle this exception. After handling SET_ADDRESS, subsequently the
> irq hanler of s3c-hsotg would also invokes composite_setup()
> function to handle USB_REQ_SET_CONFIGURATION request, set_config would
> be invoked, it also raises a FSG_STATE_CONFIG_CHANGE
> exception and wakes up fsg_main_thread, in mean time,
> cdev->delayed_status would be plus one. Right? If I am missing
> something, please
> let me know it.:-) If so, the following scenario would trigger the
> above call trace.
>
> irq handler
> |
> |-> s3c_hsotg_disconnect()
> |
> |-> common->new_fsg = NULL
> |-> common->state to FSG_STATE_CONFIG.
> |-> wakes up fsg_main_thread.
> |-> set USB device address
>
> fsg_main_thread finds the common->state == FSG_STATE_CONFIG
> |
> |-> handle_execption
> |
> |-> set common->state to FSG_STATE_IDLE
> irq hanppens ------------>|
> irq handler needs to hanle USB_REQ_SET_CONFIGURATION request.
> |->do_set_interface()
> |-> set_config()
> |
> |-> common->new_fsg = new_fsg;
> |-> common->state = FSG_STATE_CONFIG
> |-> cdev->delayed_status++
> |-> wakes up fsg_main_thread
>
> |-> Now the common->state == FSG_STATE_CONFIG
> |-> if(common->new_fsg)
> usb_composite_setup_continue()
> |->cdev->delayed_status--
> |
> fsg_main_thread finds the common->state still is FSG_STATE_CONFIG,
> | so it would invoke handle_execption again.
> |->hanle_execption
> |-> set common->state to FSG_STATE_IDLE
> |-> do_set_interface()
> |-> if (common->new_fsg)
> usb_composite_setup_continue()
> |-> cdev->delayed_status
> == 0, so
> this warning is triggered.
>
>
> Thanks
> Wei
>
>>
>> In f_mass_storage.c, struct fsg_common is shared among all the function
>> instances. This structure includes things like cmnd and nluns, which
>> will in general be different for different functions.
>>
>> That's okay if each function is in a separate config, but what happens
>> when there are multiple functions in the same config, using different
>> interfaces? What if the host sends concurrent commands to two of these
>> functions?
>>
>> Am I missing something?
>>
>> Alan Stern
>>
>>
>>
>
On 06/04/2014 09:45 AM, Peter Chen wrote:
>
>> commit d18f7116a5ddb8263fe62b05ad63e5ceb5875791
>> Author: Robert Baldyga <[email protected]>
>> Date: Thu Nov 21 13:49:18 2013 +0100
>>
>> usb: gadget: s3c-hsotg: fix disconnect handling
>>
>> This patch moves s3c_hsotg_disconnect function call from USBSusp
>> interrupt
>> handler to SET_ADDRESS request handler.
>>
> It is a little strange we call gadget's disconnect at SET_ADDRESS?
> How the udc calls gadget driver the disconnection has happened when
> the usb cable is disconnected from the host?
>
> Usually, we call gadget's disconnect at two situations
>
> - udc's reset handler if udc's speed is not UNKNOWN, it is usually happened
> when the host sends reset after enumeration.
> - udc's disconnect handler, it is usually happened when the usb cable
> is disconnected from host.
Hmm, usually the two situations, but according to the commit log, s3c
hsotg does not support Disconnected interrupt for device mode,
so the second situation does not happen for s3c hsotg, therefore, he has
to disconnect the connection before it is connected again.
Wei
>
> Peter
>
>
From: Yang Wei <[email protected]>
While loading g_mass_storage module, the following warning is triggered.
WARNING: at drivers/usb/gadget/composite.c:
usb_composite_setup_continue: Unexpected call
Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
[<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
[<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
[<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
[<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
[<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
[<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
[<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
[<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
The root cause is that Robert once introduced the following patch to fix
disconnect handling of s3c-hsotg.
[
commit d18f7116a5ddb8263fe62b05ad63e5ceb5875791
Author: Robert Baldyga <[email protected]>
Date: Thu Nov 21 13:49:18 2013 +0100
usb: gadget: s3c-hsotg: fix disconnect handling
This patch moves s3c_hsotg_disconnect function call from USBSusp interrupt
handler to SET_ADDRESS request handler.
It's because disconnected state can't be detected directly, because this
hardware doesn't support Disconnected interrupt for device mode. For both
Suspend and Disconnect events there is one interrupt USBSusp, but calling
s3c_hsotg_disconnect from this interrupt handler causes config reset in
composite layer, which is not undesirable for Suspended state.
For this reason s3c_hsotg_disconnect is called from SET_ADDRESS request
handler, which occurs always after disconnection, so we do disconnect
immediately before we are connected again. It's probably only way we
can do handle disconnection correctly.
Signed-off-by: Robert Baldyga <[email protected]>
Signed-off-by: Kyungmin Park <[email protected]>
Signed-off-by: Felipe Balbi <[email protected]>
]
So it also means that s3c_hsotg_disconnect is called from SET_ADDRESS request handler,
ultimately reset_config would finally be called, it raises a FSG_STATE_CONFIG_CHANGE exception,
and set common->new_fsg to NULL, and then wakes up fsg_main_thread to handle this exception.
After handling SET_ADDRESS, subsequently the irq hanler of s3c-hsotg would also invokes composite_setup()
function to handle USB_REQ_SET_CONFIGURATION request, set_config would be invoked, it
not only raises a FSG_STATE_CONFIG_CHANGE and set common->new_fsg to new_fsg but also makes
cdev->delayed_status plus one. If the execution ordering just likes the following scenario, the warning
would be triggered.
irq handler
|
|-> s3c_hsotg_disconnect()
|
|-> common->new_fsg = NULL
|-> common->state = FSG_STATE_CONFIG
|-> wakes up fsg_main_thread.
|->set USB device address.
fsg_main_thread
|
|-> handle_exception
|
|-> common->state = FSG_STATE_IDLE
|-> do_set_interface()
irq happens -------------->
irq handler needs to handle USB_REQ_SET_CONFIGURATION request
|
|-> set_config()
|
|-> common->new_fsg = new_fsg;
|-> common->state = FSG_STATE_CONFIG
|-> cdev->delayed_status++
|-> wakes up fsg_main_thread
fsg_main_thread
|
|-> Now the common->state = FSG_STATE_CONFIG and common->new_fsg is not equal to NULL
|-> if(common->new_fsg)
|-> usb_composite_setup_continue()
|-> cdev->delayed_status--
|-> fsg_main_thread still finds the common->state is equal to FSG_STATE_IDLE
|-> so it invokes handle_exception again, subsequently the usb_composite_setup_continue
|-> is executed again. It would fininally results in the warning.
So we also need to define a variable(struct fsg_dev *new) and then save common->new_fsg
to it with the common->lock protection.
Signed-off-by: Yang Wei <[email protected]>
---
drivers/usb/gadget/f_mass_storage.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Changes in v2:
Only rephrase the commit log to make sense this issue.
Wei
diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index b963939..e3b1798 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2342,6 +2342,7 @@ static void handle_exception(struct fsg_common *common)
struct fsg_buffhd *bh;
enum fsg_state old_state;
struct fsg_lun *curlun;
+ struct fsg_dev *new;
unsigned int exception_req_tag;
/*
@@ -2421,6 +2422,7 @@ static void handle_exception(struct fsg_common *common)
}
common->state = FSG_STATE_IDLE;
}
+ new = common->new_fsg;
spin_unlock_irq(&common->lock);
/* Carry out any extra actions required for the exception */
@@ -2460,8 +2462,8 @@ static void handle_exception(struct fsg_common *common)
break;
case FSG_STATE_CONFIG_CHANGE:
- do_set_interface(common, common->new_fsg);
- if (common->new_fsg)
+ do_set_interface(common, new);
+ if (new)
usb_composite_setup_continue(common->cdev);
break;
--
1.7.9.5
> On 06/04/2014 09:45 AM, Peter Chen wrote:
> >
> >> commit d18f7116a5ddb8263fe62b05ad63e5ceb5875791
> >> Author: Robert Baldyga <[email protected]>
> >> Date: Thu Nov 21 13:49:18 2013 +0100
> >>
> >> usb: gadget: s3c-hsotg: fix disconnect handling
> >>
> >> This patch moves s3c_hsotg_disconnect function call from
> >> USBSusp interrupt
> >> handler to SET_ADDRESS request handler.
> >>
> > It is a little strange we call gadget's disconnect at SET_ADDRESS?
> > How the udc calls gadget driver the disconnection has happened when
> > the usb cable is disconnected from the host?
> >
> > Usually, we call gadget's disconnect at two situations
> >
> > - udc's reset handler if udc's speed is not UNKNOWN, it is usually
> > happened when the host sends reset after enumeration.
> > - udc's disconnect handler, it is usually happened when the usb cable
> > is disconnected from host.
>
> Hmm, usually the two situations, but according to the commit log, s3c
> hsotg does not support Disconnected interrupt for device mode, so the
> second situation does not happen for s3c hsotg, therefore, he has to
> disconnect the connection before it is connected again.
>
Then, it seems Samsung uses other solution to detect disconnection, otherwise
how it notifies the app it has disconnected, eg, we disconnect usb cable with
pc for android phone, the usb icon should be disappeared.
Peter
Hi Alan,
W dniu 03.06.2014 16:48, Alan Stern pisze:
> On Tue, 3 Jun 2014 [email protected] wrote:
>
>> From: Yang Wei <[email protected]>
>>
>> While loading g_mass_storage module, the following warning is triggered.
>> In fact, it is more easy to reproduce it with RT kernel.
>>
>> WARNING: at drivers/usb/gadget/composite.c:
>> usb_composite_setup_continue: Unexpected call
>> Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
>> [<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
>> [<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
>> [<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
>> [<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
>> [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
>> [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
>> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
>> [<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
>>
>> The root cause just likes the following scenario.
>>
>> irq thread
>>
>> composite_disconnect()
>> |
>> |->fsg_disable() fsg->common->new_fsg = NULL
>> and then wake fsg_main_thread
>> with seting common->state to
>> FSG_STATE_CONFIG_CHANGE.
>> fsg_main_thread
>> |
>> |->do_set_interface()
>> irq thread
>>
>> set_config()
>> |
>> |->fsg_set_alt() fsg->common->new_fsg = new_fsg
>> and then also wake up fsg_main_thread
>> with setting common->state to
>> FSG_STATE_CONFIG_CHANGE.
>> |-> if(common->new_fsg)
>> usb_composite_setup_continue()
>>
>> In this case, fsg_main_thread would invoke usb_composite_setup_continue
>> twice, so the second call would trigger the above call trace, as we also
>> save common->new_fsg while changing the common->state.
>
> Michal and Andrzej:
>
> I haven't paid much attention to these matters, because you handled the
> conversion from g_file_storage to f_mass_storage using the composite
> framework. But this patch seemed odd, so I took a closer look.
Actually when I started dealing with usb gadgets the f_mass_storage
had already been there. My involvement started with some cleanup,
then moving to the new function registration interface
(usb_get/put_function_instance(), usb_get/put_function())
and adding configfs support. That said, it is not impossible for me
to have spoilt something :O
>
> In f_mass_storage.c, struct fsg_common is shared among all the function
> instances. This structure includes things like cmnd and nluns, which
> will in general be different for different functions.
>
> That's okay if each function is in a separate config, but what happens
> when there are multiple functions in the same config, using different
> interfaces? What if the host sends concurrent commands to two of these
> functions?
>
When Sebastian introduced the function registration interface I didn't
specially like the naming: struct usb_function_instance is something
different than an instance of struct usb_function.
The purpose of fsg_alloc_inst() is to create a usb_function_instance
whose container_of is struct fsg_opts. In fact it is struct fsg_opts
which is actually allocated; one of its members is struct fsg_common
which is also allocated - individually for each struct usb_function_instance.
Among traditional gadgets there is no gadget which uses mass storage function
more than once. On the other hand, when gadgets are created with configfs
it is forbidden to link a given function more than once into a given
config, that is a struct usb_function_instance can be referenced by more
than one config, but can be referenced only once in a given config;
each symbolic link corresponds to an instance of struct usb_function
(don't confuse with struct usb_function_instance).
So yes, an fsg_common can be shared among instances of struct usb_function,
but neither with traditional gadgets as they are now nor with configfs
is it possible to have the same fsg_common referenced more than once
in a given config.
AP
On Wed, 4 Jun 2014, Yang,Wei wrote:
> On 06/04/2014 09:45 AM, Peter Chen wrote:
> >
> >> commit d18f7116a5ddb8263fe62b05ad63e5ceb5875791
> >> Author: Robert Baldyga <[email protected]>
> >> Date: Thu Nov 21 13:49:18 2013 +0100
> >>
> >> usb: gadget: s3c-hsotg: fix disconnect handling
> >>
> >> This patch moves s3c_hsotg_disconnect function call from USBSusp
> >> interrupt
> >> handler to SET_ADDRESS request handler.
> >>
> > It is a little strange we call gadget's disconnect at SET_ADDRESS?
> > How the udc calls gadget driver the disconnection has happened when
> > the usb cable is disconnected from the host?
> >
> > Usually, we call gadget's disconnect at two situations
> >
> > - udc's reset handler if udc's speed is not UNKNOWN, it is usually happened
> > when the host sends reset after enumeration.
> > - udc's disconnect handler, it is usually happened when the usb cable
> > is disconnected from host.
>
> Hmm, usually the two situations, but according to the commit log, s3c
> hsotg does not support Disconnected interrupt for device mode,
> so the second situation does not happen for s3c hsotg, therefore, he has
> to disconnect the connection before it is connected again.
Why does he need to do that? Why not just skip the disconnect
notification if the hardware can't detect a disconnect?
It makes no sense at all to call a disconnect handler from within the
SET_ADDRESS routine.
Alan Stern
On Wed, 4 Jun 2014, Andrzej Pietrasiewicz wrote:
> When Sebastian introduced the function registration interface I didn't
> specially like the naming: struct usb_function_instance is something
> different than an instance of struct usb_function.
What is the difference in purpose between usb_function and
usb_function_instance? I can't tell just by reading the header file.
Does one of them get created dynamically when the host sets the
appropriate config?
It's quite noticeable that composite.h does not contain nearly enough
documentation. Only four of the structures defined there have any
kerneldoc, and none of the functions do.
Also, there seems to be some confusion between structures that
represent drivers and those that represent devices (or parts of a
device). For example, struct usb_function contains instance data as
well as driver callbacks.
> The purpose of fsg_alloc_inst() is to create a usb_function_instance
> whose container_of is struct fsg_opts. In fact it is struct fsg_opts
> which is actually allocated; one of its members is struct fsg_common
> which is also allocated - individually for each struct usb_function_instance.
>
> Among traditional gadgets there is no gadget which uses mass storage function
> more than once. On the other hand, when gadgets are created with configfs
> it is forbidden to link a given function more than once into a given
> config,
What is the reason for this restriction?
> that is a struct usb_function_instance can be referenced by more
> than one config, but can be referenced only once in a given config;
> each symbolic link corresponds to an instance of struct usb_function
> (don't confuse with struct usb_function_instance).
It's extremely easy to confuse them, since I don't understand the
differences between them. It even seems like you confused them in this
description: You mentioned "link a given function", "link corresponds
to an instance of struct usb_function", and "struct
usb_function_instance can be referenced by more than one config".
What's the difference between linking a usb_function and referencing a
usb_function_instance? Normally "linking" and "referencing" mean more
or less the same thing.
> So yes, an fsg_common can be shared among instances of struct usb_function,
> but neither with traditional gadgets as they are now nor with configfs
> is it possible to have the same fsg_common referenced more than once
> in a given config.
That's a relief. But it still seems like a bad design. If there can
be only one struct fsg_dev associated with struct fsg_common, why have
separate structures? And if there can be multiple fsg_dev structures
associated with struct fsg_common, why does struct fsg_common contain a
pointer to an fsg_dev (in fact, two of them)?
The issue that started these thoughts was the way fsg_common.new_fsg
gets used, as modified by the patch in the thread's original email.
It's not clear why new_fsg is needed at all.
Alan Stern
> From: [email protected] [mailto:[email protected]] On Behalf Of Alan Stern
> Sent: Wednesday, June 04, 2014 6:57 AM
>
> On Wed, 4 Jun 2014, Yang,Wei wrote:
>
> > On 06/04/2014 09:45 AM, Peter Chen wrote:
> > >
> > >> commit d18f7116a5ddb8263fe62b05ad63e5ceb5875791
> > >> Author: Robert Baldyga <[email protected]>
> > >> Date: Thu Nov 21 13:49:18 2013 +0100
> > >>
> > >> usb: gadget: s3c-hsotg: fix disconnect handling
> > >>
> > >> This patch moves s3c_hsotg_disconnect function call from USBSusp
> > >> interrupt
> > >> handler to SET_ADDRESS request handler.
> > >>
> > > It is a little strange we call gadget's disconnect at SET_ADDRESS?
> > > How the udc calls gadget driver the disconnection has happened when
> > > the usb cable is disconnected from the host?
> > >
> > > Usually, we call gadget's disconnect at two situations
> > >
> > > - udc's reset handler if udc's speed is not UNKNOWN, it is usually happened
> > > when the host sends reset after enumeration.
> > > - udc's disconnect handler, it is usually happened when the usb cable
> > > is disconnected from host.
> >
> > Hmm, usually the two situations, but according to the commit log, s3c
> > hsotg does not support Disconnected interrupt for device mode,
> > so the second situation does not happen for s3c hsotg, therefore, he has
> > to disconnect the connection before it is connected again.
>
> Why does he need to do that? Why not just skip the disconnect
> notification if the hardware can't detect a disconnect?
>
> It makes no sense at all to call a disconnect handler from within the
> SET_ADDRESS routine.
FWIW, here is the section from the DWC2 databook that describes how a
disconnect should be handled for the device:
When OTG_MODE is set to 0, 1, or 3, the device disconnect flow is as
follows:
1. When the USB cable is unplugged or when the VBUS is switched
off by the Host, the Device core triggers GINTSTS.OTGInt
[bit 2] interrupt bit.
2. When the device application detects GINTSTS.OTGInt interrupt,
it checks that the GOTGINT.SesEndDet (Session End Detected)
bit is set to 1'b1.
When OTG_MODE is set to 2 or 4, the device disconnect flow is as
follows:
1. When the USB cable is unplugged or when the VBUS is switched
off by the Host, the Device core triggers GINTSTS.USBRst
[bit 12] interrupt bit.
2. When the device application detects GINTSTS.USBRst, the
application sets a timeout check for SET ADDRESS Control Xfer
from Host.
3. If application does not receive SET ADDRESS Control Xfer from
Host before the timeout period, it is treated as a device
disconnection.
OTG_MODE is a configuration parameter that is set when the core is
built. From this discussion, it sounds like the s3c-hsotg core is
built for either mode 2 or 4. So SET ADDRESS should be involved, but
not in the way the driver is currently doing it.
Unfortunately I don't have the s3c-hsotg hardware, so I can't work on this
myself.
--
Paul
> On Wed, 4 Jun 2014, Yang,Wei wrote:
>
> > On 06/04/2014 09:45 AM, Peter Chen wrote:
> > >
> > >> commit d18f7116a5ddb8263fe62b05ad63e5ceb5875791
> > >> Author: Robert Baldyga <[email protected]>
> > >> Date: Thu Nov 21 13:49:18 2013 +0100
> > >>
> > >> usb: gadget: s3c-hsotg: fix disconnect handling
> > >>
> > >> This patch moves s3c_hsotg_disconnect function call from
> > >> USBSusp interrupt
> > >> handler to SET_ADDRESS request handler.
> > >>
> > > It is a little strange we call gadget's disconnect at SET_ADDRESS?
> > > How the udc calls gadget driver the disconnection has happened when
> > > the usb cable is disconnected from the host?
> > >
> > > Usually, we call gadget's disconnect at two situations
> > >
> > > - udc's reset handler if udc's speed is not UNKNOWN, it is usually
> > > happened when the host sends reset after enumeration.
> > > - udc's disconnect handler, it is usually happened when the usb
> > > cable is disconnected from host.
> >
> > Hmm, usually the two situations, but according to the commit log, s3c
> > hsotg does not support Disconnected interrupt for device mode, so the
> > second situation does not happen for s3c hsotg, therefore, he has to
> > disconnect the connection before it is connected again.
>
> Why does he need to do that? Why not just skip the disconnect
> notification if the hardware can't detect a disconnect?
>
We must call gadget driver's disconnect, otherwise, there at least
has a warning when unload module, please refer to __composite_unbind
at drivers/usb/gadget/composite.c
Peter
> It makes no sense at all to call a disconnect handler from within the
> SET_ADDRESS routine.
>
> Alan Stern
W dniu 04.06.2014 17:26, Alan Stern pisze:
> On Wed, 4 Jun 2014, Andrzej Pietrasiewicz wrote:
>
<snip>
> What is the difference in purpose between usb_function and
> usb_function_instance? I can't tell just by reading the header file.
> Does one of them get created dynamically when the host sets the
> appropriate config?
>
Please see below.
<snip>
>>
>> Among traditional gadgets there is no gadget which uses mass storage function
>> more than once. On the other hand, when gadgets are created with configfs
>> it is forbidden to link a given function more than once into a given
>> config,
>
> What is the reason for this restriction?
Please see below.
>
>> that is a struct usb_function_instance can be referenced by more
>> than one config, but can be referenced only once in a given config;
>> each symbolic link corresponds to an instance of struct usb_function
>> (don't confuse with struct usb_function_instance).
>
> It's extremely easy to confuse them, since I don't understand the
> differences between them. It even seems like you confused them in this
> description: You mentioned "link a given function", "link corresponds
> to an instance of struct usb_function", and "struct
> usb_function_instance can be referenced by more than one config".
> What's the difference between linking a usb_function and referencing a
> usb_function_instance? Normally "linking" and "referencing" mean more
> or less the same thing.
As I said, I didn't like the naming here. I got used to it, though,
but understand (and agree) that it is confusing. As far as explaining
the difference is concerned, being a non-native speaker of English
has its influence, too, so let me do it again.
I think it is easier to tell the purpose of the two structures taking
gadgets composed with configfs as example.
In each gadget there is "functions" directory. Function directories
are created there:
$ cd $CONFIGFS_ROOT/usb_gadget/our_gadget
$ mkdir functions/mass_storage.0
mass_storage.0 is internally represented as an instance of
struct usb_function_instance, which has its associated
struct fsg_common (the fsg_common is a member of
container_of(struct usb_function_instance)).
It can be referenced from multiple configurations.
$ ln -s functions/mass_storage.0 configs/config.1
$ ln -s functions/mass_storage.0 configs/config.2
Each reference (symbolic link) is internally represented as
an instance of struct usb_function. The struct usb_function here
has its associated struct fsg_dev (the fsg_dev is a
container_of(struct usb_function)).
By the very nature of any file system one cannot do:
$ ln -s functions/mass_storage.0 configs/config.1
$ ln -s functions/mass_storage.0 configs/config.1 => -EEXIST
By design of how configfs is applied to any usb gadget on cannot even do:
$ ln -s functions/mass_storage.0 configs/config.1/my_mass_storage.0
$ ln -s functions/mass_storage.0 configs/config.1/the_same_mass_storage.0 => -EEXIST
However, there should be no problem with this:
$ mkdir functions/mass_storage.0
$ mkdir functions/mass_storage.1
$ ln -s functions/mass_storage.0 configs/config.1
$ ln -s functions/mass_storage.1 configs/config.1
Legacy gadgets (g_mass_storage, g_acm_ms, g_multi) in fact operate in
a somewhat similar manner, the difference is that instead of creating directories
and making symbolic links, usb_get_function_instance() and usb_get_function()
are called, respectively, and composing a gadget happens from beginning to end
at module init.
I hope this clarifies things a bit.
AP
On Thu, 5 Jun 2014, Peter Chen wrote:
> > > > It is a little strange we call gadget's disconnect at SET_ADDRESS?
> > > > How the udc calls gadget driver the disconnection has happened when
> > > > the usb cable is disconnected from the host?
> > > >
> > > > Usually, we call gadget's disconnect at two situations
> > > >
> > > > - udc's reset handler if udc's speed is not UNKNOWN, it is usually
> > > > happened when the host sends reset after enumeration.
> > > > - udc's disconnect handler, it is usually happened when the usb
> > > > cable is disconnected from host.
> > >
> > > Hmm, usually the two situations, but according to the commit log, s3c
> > > hsotg does not support Disconnected interrupt for device mode, so the
> > > second situation does not happen for s3c hsotg, therefore, he has to
> > > disconnect the connection before it is connected again.
> >
> > Why does he need to do that? Why not just skip the disconnect
> > notification if the hardware can't detect a disconnect?
> >
>
> We must call gadget driver's disconnect, otherwise, there at least
> has a warning when unload module, please refer to __composite_unbind
> at drivers/usb/gadget/composite.c
The disconnect callback can run just before unbind. That's not a valid
reason for doing a disconnect notification as part of SET_ADDRESS.
Alan Stern
On Wed, 4 Jun 2014 [email protected] wrote:
> From: Yang Wei <[email protected]>
>
> While loading g_mass_storage module, the following warning is triggered.
>
> WARNING: at drivers/usb/gadget/composite.c:
> usb_composite_setup_continue: Unexpected call
> Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
> [<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
> [<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
> [<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
> [<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
> [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
> [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
> [<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
>
> The root cause is that Robert once introduced the following patch to fix
> disconnect handling of s3c-hsotg.
> [
> commit d18f7116a5ddb8263fe62b05ad63e5ceb5875791
> Author: Robert Baldyga <[email protected]>
> Date: Thu Nov 21 13:49:18 2013 +0100
>
> usb: gadget: s3c-hsotg: fix disconnect handling
>
> This patch moves s3c_hsotg_disconnect function call from USBSusp interrupt
> handler to SET_ADDRESS request handler.
>
> It's because disconnected state can't be detected directly, because this
> hardware doesn't support Disconnected interrupt for device mode. For both
> Suspend and Disconnect events there is one interrupt USBSusp, but calling
> s3c_hsotg_disconnect from this interrupt handler causes config reset in
> composite layer, which is not undesirable for Suspended state.
>
> For this reason s3c_hsotg_disconnect is called from SET_ADDRESS request
> handler, which occurs always after disconnection, so we do disconnect
> immediately before we are connected again. It's probably only way we
> can do handle disconnection correctly.
>
> Signed-off-by: Robert Baldyga <[email protected]>
> Signed-off-by: Kyungmin Park <[email protected]>
> Signed-off-by: Felipe Balbi <[email protected]>
> ]
>
> So it also means that s3c_hsotg_disconnect is called from SET_ADDRESS request handler,
> ultimately reset_config would finally be called, it raises a FSG_STATE_CONFIG_CHANGE exception,
> and set common->new_fsg to NULL, and then wakes up fsg_main_thread to handle this exception.
> After handling SET_ADDRESS, subsequently the irq hanler of s3c-hsotg would also invokes composite_setup()
> function to handle USB_REQ_SET_CONFIGURATION request, set_config would be invoked, it
> not only raises a FSG_STATE_CONFIG_CHANGE and set common->new_fsg to new_fsg but also makes
> cdev->delayed_status plus one. If the execution ordering just likes the following scenario, the warning
> would be triggered.
>
> irq handler
> |
> |-> s3c_hsotg_disconnect()
> |
> |-> common->new_fsg = NULL
> |-> common->state = FSG_STATE_CONFIG
> |-> wakes up fsg_main_thread.
> |->set USB device address.
>
> fsg_main_thread
> |
> |-> handle_exception
> |
> |-> common->state = FSG_STATE_IDLE
> |-> do_set_interface()
> irq happens -------------->
>
> irq handler needs to handle USB_REQ_SET_CONFIGURATION request
> |
> |-> set_config()
> |
> |-> common->new_fsg = new_fsg;
> |-> common->state = FSG_STATE_CONFIG
> |-> cdev->delayed_status++
> |-> wakes up fsg_main_thread
>
> fsg_main_thread
> |
> |-> Now the common->state = FSG_STATE_CONFIG and common->new_fsg is not equal to NULL
> |-> if(common->new_fsg)
> |-> usb_composite_setup_continue()
> |-> cdev->delayed_status--
> |-> fsg_main_thread still finds the common->state is equal to FSG_STATE_IDLE
> |-> so it invokes handle_exception again, subsequently the usb_composite_setup_continue
> |-> is executed again. It would fininally results in the warning.
>
> So we also need to define a variable(struct fsg_dev *new) and then save common->new_fsg
> to it with the common->lock protection.
>
> Signed-off-by: Yang Wei <[email protected]>
> ---
> drivers/usb/gadget/f_mass_storage.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> Changes in v2:
>
> Only rephrase the commit log to make sense this issue.
>
> Wei
>
> diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
> index b963939..e3b1798 100644
> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -2342,6 +2342,7 @@ static void handle_exception(struct fsg_common *common)
> struct fsg_buffhd *bh;
> enum fsg_state old_state;
> struct fsg_lun *curlun;
> + struct fsg_dev *new;
The spacing is different from the lines above. There should be two tab
characters between the 'v' and the '*', not three spaces.
Also, the variable should be named "new_fsg", not "new".
> unsigned int exception_req_tag;
>
> /*
> @@ -2421,6 +2422,7 @@ static void handle_exception(struct fsg_common *common)
> }
> common->state = FSG_STATE_IDLE;
> }
> + new = common->new_fsg;
> spin_unlock_irq(&common->lock);
>
> /* Carry out any extra actions required for the exception */
> @@ -2460,8 +2462,8 @@ static void handle_exception(struct fsg_common *common)
> break;
>
> case FSG_STATE_CONFIG_CHANGE:
> - do_set_interface(common, common->new_fsg);
> - if (common->new_fsg)
> + do_set_interface(common, new);
> + if (new)
> usb_composite_setup_continue(common->cdev);
> break;
The description is long-winded and confusing, but the patch is correct.
The existing code fails to take into account the possibility that
common->new_fsg can change while do_set_interface() is running, because
the spinlock isn't held at this point.
Alan Stern
On Thu, 5 Jun 2014, Andrzej Pietrasiewicz wrote:
> I think it is easier to tell the purpose of the two structures taking
> gadgets composed with configfs as example.
>
> In each gadget there is "functions" directory. Function directories
> are created there:
>
> $ cd $CONFIGFS_ROOT/usb_gadget/our_gadget
> $ mkdir functions/mass_storage.0
>
> mass_storage.0 is internally represented as an instance of
> struct usb_function_instance, which has its associated
> struct fsg_common (the fsg_common is a member of
> container_of(struct usb_function_instance)).
Okay.
> It can be referenced from multiple configurations.
>
> $ ln -s functions/mass_storage.0 configs/config.1
> $ ln -s functions/mass_storage.0 configs/config.2
>
> Each reference (symbolic link) is internally represented as
> an instance of struct usb_function. The struct usb_function here
> has its associated struct fsg_dev (the fsg_dev is a
> container_of(struct usb_function)).
So in essence, a usb_function connects a particular function to a
particular config.
> By the very nature of any file system one cannot do:
>
> $ ln -s functions/mass_storage.0 configs/config.1
> $ ln -s functions/mass_storage.0 configs/config.1 => -EEXIST
>
> By design of how configfs is applied to any usb gadget on cannot even do:
>
> $ ln -s functions/mass_storage.0 configs/config.1/my_mass_storage.0
> $ ln -s functions/mass_storage.0 configs/config.1/the_same_mass_storage.0 => -EEXIST
So for any given usb_function_instance, only one usb_function will be
active at any time: the one connecting the function to the current
config.
And presumably the reasons why struct fsg_dev contains
interface_number, bulk_in, and bulk_out members are because these
values are determined when the config is originally set up, and they
can vary from one config to another. Right? Whereas the items in
struct fsg_common don't depend on the config.
> However, there should be no problem with this:
>
> $ mkdir functions/mass_storage.0
> $ mkdir functions/mass_storage.1
> $ ln -s functions/mass_storage.0 configs/config.1
> $ ln -s functions/mass_storage.1 configs/config.1
That makes sense now.
> Legacy gadgets (g_mass_storage, g_acm_ms, g_multi) in fact operate in
> a somewhat similar manner, the difference is that instead of creating directories
> and making symbolic links, usb_get_function_instance() and usb_get_function()
> are called, respectively, and composing a gadget happens from beginning to end
> at module init.
>
> I hope this clarifies things a bit.
Yes, it helps a lot. Thank you.
Alan Stern
Hi Alan,
Sorry for my late response, as I was 000, Additionally, I am very
grateful for your review.
On 06/06/2014 02:08 AM, Alan Stern wrote:
> On Wed, 4 Jun 2014 [email protected] wrote:
>
>> From: Yang Wei <[email protected]>
>>
>> While loading g_mass_storage module, the following warning is triggered.
>>
>> WARNING: at drivers/usb/gadget/composite.c:
>> usb_composite_setup_continue: Unexpected call
>> Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
>> [<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
>> [<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
>> [<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
>> [<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
>> [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
>> [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
>> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
>> [<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
>>
>> The root cause is that Robert once introduced the following patch to fix
>> disconnect handling of s3c-hsotg.
>> [
>> commit d18f7116a5ddb8263fe62b05ad63e5ceb5875791
>> Author: Robert Baldyga <[email protected]>
>> Date: Thu Nov 21 13:49:18 2013 +0100
>>
>> usb: gadget: s3c-hsotg: fix disconnect handling
>>
>> This patch moves s3c_hsotg_disconnect function call from USBSusp interrupt
>> handler to SET_ADDRESS request handler.
>>
>> It's because disconnected state can't be detected directly, because this
>> hardware doesn't support Disconnected interrupt for device mode. For both
>> Suspend and Disconnect events there is one interrupt USBSusp, but calling
>> s3c_hsotg_disconnect from this interrupt handler causes config reset in
>> composite layer, which is not undesirable for Suspended state.
>>
>> For this reason s3c_hsotg_disconnect is called from SET_ADDRESS request
>> handler, which occurs always after disconnection, so we do disconnect
>> immediately before we are connected again. It's probably only way we
>> can do handle disconnection correctly.
>>
>> Signed-off-by: Robert Baldyga <[email protected]>
>> Signed-off-by: Kyungmin Park <[email protected]>
>> Signed-off-by: Felipe Balbi <[email protected]>
>> ]
>>
>> So it also means that s3c_hsotg_disconnect is called from SET_ADDRESS request handler,
>> ultimately reset_config would finally be called, it raises a FSG_STATE_CONFIG_CHANGE exception,
>> and set common->new_fsg to NULL, and then wakes up fsg_main_thread to handle this exception.
>> After handling SET_ADDRESS, subsequently the irq hanler of s3c-hsotg would also invokes composite_setup()
>> function to handle USB_REQ_SET_CONFIGURATION request, set_config would be invoked, it
>> not only raises a FSG_STATE_CONFIG_CHANGE and set common->new_fsg to new_fsg but also makes
>> cdev->delayed_status plus one. If the execution ordering just likes the following scenario, the warning
>> would be triggered.
>>
>> irq handler
>> |
>> |-> s3c_hsotg_disconnect()
>> |
>> |-> common->new_fsg = NULL
>> |-> common->state = FSG_STATE_CONFIG
>> |-> wakes up fsg_main_thread.
>> |->set USB device address.
>>
>> fsg_main_thread
>> |
>> |-> handle_exception
>> |
>> |-> common->state = FSG_STATE_IDLE
>> |-> do_set_interface()
>> irq happens -------------->
>>
>> irq handler needs to handle USB_REQ_SET_CONFIGURATION request
>> |
>> |-> set_config()
>> |
>> |-> common->new_fsg = new_fsg;
>> |-> common->state = FSG_STATE_CONFIG
>> |-> cdev->delayed_status++
>> |-> wakes up fsg_main_thread
>>
>> fsg_main_thread
>> |
>> |-> Now the common->state = FSG_STATE_CONFIG and common->new_fsg is not equal to NULL
>> |-> if(common->new_fsg)
>> |-> usb_composite_setup_continue()
>> |-> cdev->delayed_status--
>> |-> fsg_main_thread still finds the common->state is equal to FSG_STATE_IDLE
>> |-> so it invokes handle_exception again, subsequently the usb_composite_setup_continue
>> |-> is executed again. It would fininally results in the warning.
>>
>> So we also need to define a variable(struct fsg_dev *new) and then save common->new_fsg
>> to it with the common->lock protection.
>>
>> Signed-off-by: Yang Wei <[email protected]>
>> ---
>> drivers/usb/gadget/f_mass_storage.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> Changes in v2:
>>
>> Only rephrase the commit log to make sense this issue.
>>
>> Wei
>>
>> diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
>> index b963939..e3b1798 100644
>> --- a/drivers/usb/gadget/f_mass_storage.c
>> +++ b/drivers/usb/gadget/f_mass_storage.c
>> @@ -2342,6 +2342,7 @@ static void handle_exception(struct fsg_common *common)
>> struct fsg_buffhd *bh;
>> enum fsg_state old_state;
>> struct fsg_lun *curlun;
>> + struct fsg_dev *new;
> The spacing is different from the lines above. There should be two tab
> characters between the 'v' and the '*', not three spaces.
>
> Also, the variable should be named "new_fsg", not "new".
Okay, I would fix it in v3.
>
>> unsigned int exception_req_tag;
>>
>> /*
>> @@ -2421,6 +2422,7 @@ static void handle_exception(struct fsg_common *common)
>> }
>> common->state = FSG_STATE_IDLE;
>> }
>> + new = common->new_fsg;
>> spin_unlock_irq(&common->lock);
>>
>> /* Carry out any extra actions required for the exception */
>> @@ -2460,8 +2462,8 @@ static void handle_exception(struct fsg_common *common)
>> break;
>>
>> case FSG_STATE_CONFIG_CHANGE:
>> - do_set_interface(common, common->new_fsg);
>> - if (common->new_fsg)
>> + do_set_interface(common, new);
>> + if (new)
>> usb_composite_setup_continue(common->cdev);
>> break;
> The description is long-winded and confusing, but the patch is correct.
> The existing code fails to take into account the possibility that
> common->new_fsg can change while do_set_interface() is running, because
> the spinlock isn't held at this point.
Yes, I used a long description to want to point out that the
common->new_fsg can be changed while do_set_interface is running. I would
use the above your description instead of mine in v3.
Thanks
Wei
>
> Alan Stern
>
>
>
From: Yang Wei <[email protected]>
While loading g_mass_storage module, the following warning
is triggered.
WARNING: at drivers/usb/gadget/composite.c:
usb_composite_setup_continue: Unexpected call
Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
[<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
[<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
[<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
[<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
[<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
[<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
[<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
[<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
The root cause is that the existing code fails to take into
account the possibility that common->new_fsg can change while
do_set_interface() is running, because the spinlock isn't held
at this point.
Signed-off-by: Yang Wei <[email protected]>
Signed-off-by: Alan Stern <[email protected]>
---
drivers/usb/gadget/f_mass_storage.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Hi Alan,
Thanks for your review, there are a few changes in v3:
1) Fix a coding style issue.
2) Refine the commit log
Regards
Wei
diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index b963939..0cd8f21 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2342,6 +2342,7 @@ static void handle_exception(struct fsg_common *common)
struct fsg_buffhd *bh;
enum fsg_state old_state;
struct fsg_lun *curlun;
+ struct fsg_dev *new_fsg;
unsigned int exception_req_tag;
/*
@@ -2421,6 +2422,7 @@ static void handle_exception(struct fsg_common *common)
}
common->state = FSG_STATE_IDLE;
}
+ new_fsg = common->new_fsg;
spin_unlock_irq(&common->lock);
/* Carry out any extra actions required for the exception */
@@ -2460,8 +2462,8 @@ static void handle_exception(struct fsg_common *common)
break;
case FSG_STATE_CONFIG_CHANGE:
- do_set_interface(common, common->new_fsg);
- if (common->new_fsg)
+ do_set_interface(common, new_fsg);
+ if (new_fsg)
usb_composite_setup_continue(common->cdev);
break;
--
1.7.9.5
On 06/09/2014 02:19 PM, [email protected] wrote:
> From: Yang Wei <[email protected]>
>
> While loading g_mass_storage module, the following warning
> is triggered.
>
> WARNING: at drivers/usb/gadget/composite.c:
> usb_composite_setup_continue: Unexpected call
> Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
> [<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
> [<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
> [<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
> [<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
> [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
> [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
> [<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
>
> The root cause is that the existing code fails to take into
> account the possibility that common->new_fsg can change while
> do_set_interface() is running, because the spinlock isn't held
> at this point.
>
> Signed-off-by: Yang Wei <[email protected]>
> Signed-off-by: Alan Stern <[email protected]>
> ---
> drivers/usb/gadget/f_mass_storage.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> Hi Alan,
>
> Thanks for your review, there are a few changes in v3:
>
> 1) Fix a coding style issue.
> 2) Refine the commit log
>
> Regards
> Wei
Ping, Alan, What do you think of it?
Wei
>
> diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
> index b963939..0cd8f21 100644
> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -2342,6 +2342,7 @@ static void handle_exception(struct fsg_common *common)
> struct fsg_buffhd *bh;
> enum fsg_state old_state;
> struct fsg_lun *curlun;
> + struct fsg_dev *new_fsg;
> unsigned int exception_req_tag;
>
> /*
> @@ -2421,6 +2422,7 @@ static void handle_exception(struct fsg_common *common)
> }
> common->state = FSG_STATE_IDLE;
> }
> + new_fsg = common->new_fsg;
> spin_unlock_irq(&common->lock);
>
> /* Carry out any extra actions required for the exception */
> @@ -2460,8 +2462,8 @@ static void handle_exception(struct fsg_common *common)
> break;
>
> case FSG_STATE_CONFIG_CHANGE:
> - do_set_interface(common, common->new_fsg);
> - if (common->new_fsg)
> + do_set_interface(common, new_fsg);
> + if (new_fsg)
> usb_composite_setup_continue(common->cdev);
> break;
>
On Mon, Jun 09 2014, [email protected] wrote:
> From: Yang Wei <[email protected]>
>
> While loading g_mass_storage module, the following warning
> is triggered.
>
> WARNING: at drivers/usb/gadget/composite.c:
> usb_composite_setup_continue: Unexpected call
> Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
> [<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
> [<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
> [<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
> [<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
> [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
> [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
> [<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
>
> The root cause is that the existing code fails to take into
> account the possibility that common->new_fsg can change while
> do_set_interface() is running, because the spinlock isn't held
> at this point.
common->new_fsg is not protected by common->lock so this justification
is not valid.
>
> Signed-off-by: Yang Wei <[email protected]>
> Signed-off-by: Alan Stern <[email protected]>
> ---
> drivers/usb/gadget/f_mass_storage.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> Hi Alan,
>
> Thanks for your review, there are a few changes in v3:
>
> 1) Fix a coding style issue.
> 2) Refine the commit log
>
> Regards
> Wei
>
> diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
> index b963939..0cd8f21 100644
> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -2342,6 +2342,7 @@ static void handle_exception(struct fsg_common *common)
> struct fsg_buffhd *bh;
> enum fsg_state old_state;
> struct fsg_lun *curlun;
> + struct fsg_dev *new_fsg;
> unsigned int exception_req_tag;
>
> /*
> @@ -2421,6 +2422,7 @@ static void handle_exception(struct fsg_common *common)
> }
> common->state = FSG_STATE_IDLE;
> }
> + new_fsg = common->new_fsg;
Also, because common->new_fsg is not protected by common->lock, doing
this under a lock is kinda pointless.
> spin_unlock_irq(&common->lock);
>
> /* Carry out any extra actions required for the exception */
> @@ -2460,8 +2462,8 @@ static void handle_exception(struct fsg_common *common)
> break;
>
> case FSG_STATE_CONFIG_CHANGE:
> - do_set_interface(common, common->new_fsg);
> - if (common->new_fsg)
> + do_set_interface(common, new_fsg);
> + if (new_fsg)
> usb_composite_setup_continue(common->cdev);
As far as I can tell, it's safe to move the assignment to new_fsg here,
e.g.:
new_fsg = common->new_fsg;
do_set_interface(common, new_fsg);
if (new_fsg)
usb_composite_setup_continue(common->cdev);
> break;
But perhaps new_fsg should be protected by the lock. I think valid fix
(which I did not test in *any* way) will be this:
-------------- >8 ------------------------------------------------------------
>From 1d0b638fab05489dfb52a96556b73e2670ab52ee Mon Sep 17 00:00:00 2001
From: Michal Nazarewicz <[email protected]>
Date: Fri, 13 Jun 2014 11:40:45 +0200
Subject: [PATCH] usb: gadget: f_mass_storage: Fix a warning while loading
g_mass_storage
While loading g_mass_storage module, the following warning can
trigger:
WARNING: at drivers/usb/gadget/composite.c:
usb_composite_setup_continue: Unexpected call
Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
[<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
[<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
[<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
[<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
[<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
[<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
[<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
[<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
The root cause is that the existing code fails to take into account
the possibility that common->new_fsg can change while
do_set_interface() is running, because the spinlock does not protect
it.
Change the code so that the common->new_fsg field is protected by the
common->lock spin lock.
Reported-By: Yang Wei <[email protected]>
Signed-off-by: Michal Nazarewicz <[email protected]>
---
drivers/usb/gadget/f_mass_storage.c | 54 +++++++++++++++++++++++--------------
1 file changed, 34 insertions(+), 20 deletions(-)
diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index b963939..bd663c2 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -264,7 +264,7 @@ struct fsg_common {
/* filesem protects: backing files in use */
struct rw_semaphore filesem;
- /* lock protects: state, all the req_busy's */
+ /* lock protects: state, all the req_busy's, and new_fsg */
spinlock_t lock;
struct usb_ep *ep0; /* Copy of gadget->ep0 */
@@ -407,23 +407,39 @@ static void wakeup_thread(struct fsg_common *common)
wake_up_process(common->thread_task);
}
-static void raise_exception(struct fsg_common *common, enum fsg_state new_state)
+static void __raise_exception(struct fsg_common *common,
+ enum fsg_state new_state)
{
- unsigned long flags;
-
/*
* Do nothing if a higher-priority exception is already in progress.
* If a lower-or-equal priority exception is in progress, preempt it
* and notify the main thread by sending it a signal.
*/
+ if (common->state > new_state)
+ return;
+
+ common->exception_req_tag = common->ep0_req_tag;
+ common->state = new_state;
+ if (common->thread_task)
+ send_sig_info(SIGUSR1, SEND_SIG_FORCED, common->thread_task);
+}
+
+static void raise_exception(struct fsg_common *common, enum fsg_state new_state)
+{
+ unsigned long flags;
spin_lock_irqsave(&common->lock, flags);
- if (common->state <= new_state) {
- common->exception_req_tag = common->ep0_req_tag;
- common->state = new_state;
- if (common->thread_task)
- send_sig_info(SIGUSR1, SEND_SIG_FORCED,
- common->thread_task);
- }
+ __raise_exception(common, new_state);
+ spin_unlock_irqrestore(&common->lock, flags);
+}
+
+static void raise_config_change_exception(struct fsg_common *common,
+ struct fsg_dev *new_fsg)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&common->lock, flags);
+ common->new_fsg = new_fsg;
+ __raise_exception(common, FSG_STATE_CONFIG_CHANGE);
spin_unlock_irqrestore(&common->lock, flags);
}
@@ -2320,16 +2336,14 @@ reset:
static int fsg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
{
struct fsg_dev *fsg = fsg_from_func(f);
- fsg->common->new_fsg = fsg;
- raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
+ raise_config_change_exception(fsg->common, fsg);
return USB_GADGET_DELAYED_STATUS;
}
static void fsg_disable(struct usb_function *f)
{
struct fsg_dev *fsg = fsg_from_func(f);
- fsg->common->new_fsg = NULL;
- raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
+ raise_config_change_exception(fsg->common, NULL);
}
@@ -2342,6 +2356,7 @@ static void handle_exception(struct fsg_common *common)
struct fsg_buffhd *bh;
enum fsg_state old_state;
struct fsg_lun *curlun;
+ struct fsg_dev *new_fsg;
unsigned int exception_req_tag;
/*
@@ -2405,6 +2420,7 @@ static void handle_exception(struct fsg_common *common)
common->next_buffhd_to_drain = &common->buffhds[0];
exception_req_tag = common->exception_req_tag;
old_state = common->state;
+ new_fsg = common->new_fsg;
if (old_state == FSG_STATE_ABORT_BULK_OUT)
common->state = FSG_STATE_STATUS_PHASE;
@@ -2460,8 +2476,8 @@ static void handle_exception(struct fsg_common *common)
break;
case FSG_STATE_CONFIG_CHANGE:
- do_set_interface(common, common->new_fsg);
- if (common->new_fsg)
+ do_set_interface(common, new_fsg);
+ if (new_fsg)
usb_composite_setup_continue(common->cdev);
break;
@@ -3178,8 +3194,7 @@ static void fsg_unbind(struct usb_configuration *c, struct usb_function *f)
DBG(fsg, "unbind\n");
if (fsg->common->fsg == fsg) {
- fsg->common->new_fsg = NULL;
- raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
+ raise_config_change_exception(fsg->common, NULL);
/* FIXME: make interruptible or killable somehow? */
wait_event(common->fsg_wait, common->fsg != fsg);
}
@@ -3665,4 +3680,3 @@ void fsg_config_from_params(struct fsg_config *cfg,
cfg->fsg_num_buffers = fsg_num_buffers;
}
EXPORT_SYMBOL_GPL(fsg_config_from_params);
-
--
2.0.0.526.g5318336
On Fri, 13 Jun 2014, Yang,Wei wrote:
> On 06/09/2014 02:19 PM, [email protected] wrote:
> > From: Yang Wei <[email protected]>
> >
> > While loading g_mass_storage module, the following warning
> > is triggered.
> >
> > WARNING: at drivers/usb/gadget/composite.c:
> > usb_composite_setup_continue: Unexpected call
> > Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
> > [<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
> > [<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
> > [<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
> > [<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
> > [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
> > [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
> > [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
> > [<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
> >
> > The root cause is that the existing code fails to take into
> > account the possibility that common->new_fsg can change while
> > do_set_interface() is running, because the spinlock isn't held
> > at this point.
> >
> > Signed-off-by: Yang Wei <[email protected]>
> > Signed-off-by: Alan Stern <[email protected]>
> > ---
> > drivers/usb/gadget/f_mass_storage.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > Hi Alan,
> >
> > Thanks for your review, there are a few changes in v3:
> >
> > 1) Fix a coding style issue.
> > 2) Refine the commit log
> >
> > Regards
> > Wei
>
> Ping, Alan, What do you think of it?
You should not have added my "Signed-off-by:"; I did not give you
permission to do that.
Michal's comment about common->new_fsg not being protected by the lock
is a good one. It would be better for the patch description to say:
The value of common->new_fsg that gets tested after
do_set_interface() returns needs to be the same as the value
used by do_set_interface().
With that change, you may add
Acked-by: Alan Stern <[email protected]>
Alan Stern
On Fri, 13 Jun 2014, Michal Nazarewicz wrote:
> > The root cause is that the existing code fails to take into
> > account the possibility that common->new_fsg can change while
> > do_set_interface() is running, because the spinlock isn't held
> > at this point.
>
> common->new_fsg is not protected by common->lock so this justification
> is not valid.
That's true. A better justification would be that the same value of
new_fsg should be used in do_set_interface() and in the test that
follows.
> > @@ -2421,6 +2422,7 @@ static void handle_exception(struct fsg_common *common)
> > }
> > common->state = FSG_STATE_IDLE;
> > }
> > + new_fsg = common->new_fsg;
>
> Also, because common->new_fsg is not protected by common->lock, doing
> this under a lock is kinda pointless.
Yes, but it doesn't hurt.
> > spin_unlock_irq(&common->lock);
> >
> > /* Carry out any extra actions required for the exception */
> > @@ -2460,8 +2462,8 @@ static void handle_exception(struct fsg_common *common)
> > break;
> >
> > case FSG_STATE_CONFIG_CHANGE:
> > - do_set_interface(common, common->new_fsg);
> > - if (common->new_fsg)
> > + do_set_interface(common, new_fsg);
> > + if (new_fsg)
> > usb_composite_setup_continue(common->cdev);
>
> As far as I can tell, it's safe to move the assignment to new_fsg here,
> e.g.:
>
> new_fsg = common->new_fsg;
> do_set_interface(common, new_fsg);
> if (new_fsg)
> usb_composite_setup_continue(common->cdev);
That would be equally correct. I don't see any strong reason for
preferring one over the other.
> > break;
>
> But perhaps new_fsg should be protected by the lock. I think valid fix
> (which I did not test in *any* way) will be this:
No, I think this change is both too big and unnecessary.
common->new_fsg does not need protection; in a sense it is "owned" by
the composite driver.
Alan Stern
On Fri, Jun 13 2014, Alan Stern <[email protected]> wrote:
> On Fri, 13 Jun 2014, Michal Nazarewicz wrote:
>
>> > The root cause is that the existing code fails to take into
>> > account the possibility that common->new_fsg can change while
>> > do_set_interface() is running, because the spinlock isn't held
>> > at this point.
>>
>> common->new_fsg is not protected by common->lock so this justification
>> is not valid.
>
> That's true. A better justification would be that the same value of
> new_fsg should be used in do_set_interface() and in the test that
> follows.
>
>> > @@ -2421,6 +2422,7 @@ static void handle_exception(struct fsg_common *common)
>> > }
>> > common->state = FSG_STATE_IDLE;
>> > }
>> > + new_fsg = common->new_fsg;
>>
>> Also, because common->new_fsg is not protected by common->lock, doing
>> this under a lock is kinda pointless.
>
> Yes, but it doesn't hurt.
>
>> > spin_unlock_irq(&common->lock);
>> >
>> > /* Carry out any extra actions required for the exception */
>> > @@ -2460,8 +2462,8 @@ static void handle_exception(struct fsg_common *common)
>> > break;
>> >
>> > case FSG_STATE_CONFIG_CHANGE:
>> > - do_set_interface(common, common->new_fsg);
>> > - if (common->new_fsg)
>> > + do_set_interface(common, new_fsg);
>> > + if (new_fsg)
>> > usb_composite_setup_continue(common->cdev);
>>
>> As far as I can tell, it's safe to move the assignment to new_fsg here,
>> e.g.:
>>
>> new_fsg = common->new_fsg;
>> do_set_interface(common, new_fsg);
>> if (new_fsg)
>> usb_composite_setup_continue(common->cdev);
>
> That would be equally correct. I don't see any strong reason for
> preferring one over the other.
Doing the read under the lock may give an impression that the value is
indeed protected by the lock. Doing it outside of the lock creates no
such confusion. Therefore, I would prefer having the read outside. But
no strong feelings.
>> > break;
>>
>> But perhaps new_fsg should be protected by the lock. I think valid fix
>> (which I did not test in *any* way) will be this:
> No, I think this change is both too big and unnecessary.
> common->new_fsg does not need protection; in a sense it is "owned" by
> the composite driver.
No strong feelings on my side.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--
On 06/13/2014 09:39 PM, Alan Stern wrote:
> On Fri, 13 Jun 2014, Yang,Wei wrote:
>
>> On 06/09/2014 02:19 PM, [email protected] wrote:
>>> From: Yang Wei <[email protected]>
>>>
>>> While loading g_mass_storage module, the following warning
>>> is triggered.
>>>
>>> WARNING: at drivers/usb/gadget/composite.c:
>>> usb_composite_setup_continue: Unexpected call
>>> Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
>>> [<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
>>> [<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
>>> [<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
>>> [<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
>>> [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
>>> [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
>>> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
>>> [<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
>>>
>>> The root cause is that the existing code fails to take into
>>> account the possibility that common->new_fsg can change while
>>> do_set_interface() is running, because the spinlock isn't held
>>> at this point.
>>>
>>> Signed-off-by: Yang Wei <[email protected]>
>>> Signed-off-by: Alan Stern <[email protected]>
>>> ---
>>> drivers/usb/gadget/f_mass_storage.c | 6 ++++--
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> Hi Alan,
>>>
>>> Thanks for your review, there are a few changes in v3:
>>>
>>> 1) Fix a coding style issue.
>>> 2) Refine the commit log
>>>
>>> Regards
>>> Wei
>> Ping, Alan, What do you think of it?
> You should not have added my "Signed-off-by:"; I did not give you
> permission to do that.
Sorry for it, I considered that you give me a few advice and hep me to
refine the commit log,
so I added your signed off. Sorry again!
>
> Michal's comment about common->new_fsg not being protected by the lock
> is a good one. It would be better for the patch description to say:
>
> The value of common->new_fsg that gets tested after
> do_set_interface() returns needs to be the same as the value
> used by do_set_interface().
>
> With that change, you may add
>
> Acked-by: Alan Stern <[email protected]>
Okay, I would use the above description in v4.
Regards
Wei
>
> Alan Stern
>
>
>
From: Yang Wei <[email protected]>
While loading g_mass_storage module, the following warning
is triggered.
WARNING: at drivers/usb/gadget/composite.c:
usb_composite_setup_continue: Unexpected call
Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
[<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
[<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
[<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
[<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
[<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
[<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
[<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
[<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
The root cause is that the existing code fails to take into
account the possibility that common->new_fsg can change while
do_set_interface() is running, so the value of common->new_fsg
that gets tested after do_set_interface returns needs to be
the same as the value used by do_set_interface.
Signed-off-by: Yang Wei <[email protected]>
Acked-by: Alan Stern <[email protected]>
---
drivers/usb/gadget/f_mass_storage.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Changes in v3 ----> v4
move "new_fsg = common->new_fsg" out of comm->lock protection, and refine the commit log.
Thanks
Wei
diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index b963939..a7e24c8 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2342,6 +2342,7 @@ static void handle_exception(struct fsg_common *common)
struct fsg_buffhd *bh;
enum fsg_state old_state;
struct fsg_lun *curlun;
+ struct fsg_dev *new_fsg;
unsigned int exception_req_tag;
/*
@@ -2460,8 +2461,9 @@ static void handle_exception(struct fsg_common *common)
break;
case FSG_STATE_CONFIG_CHANGE:
- do_set_interface(common, common->new_fsg);
- if (common->new_fsg)
+ new_fsg = common->new_fsg;
+ do_set_interface(common, new_fsg);
+ if (new_fsg)
usb_composite_setup_continue(common->cdev);
break;
--
1.7.9.5
Its v4, sorry for missing it in subject.
Regards
Wei
On 06/15/2014 10:40 AM, [email protected] wrote:
> From: Yang Wei <[email protected]>
>
> While loading g_mass_storage module, the following warning
> is triggered.
>
> WARNING: at drivers/usb/gadget/composite.c:
> usb_composite_setup_continue: Unexpected call
> Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
> [<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
> [<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
> [<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
> [<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
> [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
> [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
> [<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
>
> The root cause is that the existing code fails to take into
> account the possibility that common->new_fsg can change while
> do_set_interface() is running, so the value of common->new_fsg
> that gets tested after do_set_interface returns needs to be
> the same as the value used by do_set_interface.
>
> Signed-off-by: Yang Wei <[email protected]>
> Acked-by: Alan Stern <[email protected]>
> ---
> drivers/usb/gadget/f_mass_storage.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> Changes in v3 ----> v4
>
> move "new_fsg = common->new_fsg" out of comm->lock protection, and refine the commit log.
>
> Thanks
> Wei
>
> diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
> index b963939..a7e24c8 100644
> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -2342,6 +2342,7 @@ static void handle_exception(struct fsg_common *common)
> struct fsg_buffhd *bh;
> enum fsg_state old_state;
> struct fsg_lun *curlun;
> + struct fsg_dev *new_fsg;
> unsigned int exception_req_tag;
>
> /*
> @@ -2460,8 +2461,9 @@ static void handle_exception(struct fsg_common *common)
> break;
>
> case FSG_STATE_CONFIG_CHANGE:
> - do_set_interface(common, common->new_fsg);
> - if (common->new_fsg)
> + new_fsg = common->new_fsg;
> + do_set_interface(common, new_fsg);
> + if (new_fsg)
> usb_composite_setup_continue(common->cdev);
> break;
>
On 06/15/2014 10:42 AM, Yang,Wei wrote:
> Its v4, sorry for missing it in subject.
Alan, How about this version?
Cheers
Wei
>
> Regards
> Wei
> On 06/15/2014 10:40 AM, [email protected] wrote:
>> From: Yang Wei <[email protected]>
>>
>> While loading g_mass_storage module, the following warning
>> is triggered.
>>
>> WARNING: at drivers/usb/gadget/composite.c:
>> usb_composite_setup_continue: Unexpected call
>> Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
>> [<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>]
>> (dump_stack+0x20/0x24)
>> [<80619608>] (dump_stack+0x20/0x24) from [<80025100>]
>> (warn_slowpath_common+0x64/0x74)
>> [<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>]
>> (warn_slowpath_fmt+0x40/0x48)
>> [<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>]
>> (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
>> [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc
>> [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4
>> [g_mass_storage])
>> [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from
>> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
>> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from
>> [<8004bc90>] (kthread+0x98/0x9c)
>> [<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>]
>> (kernel_thread_exit+0x0/0x8)
>>
>> The root cause is that the existing code fails to take into
>> account the possibility that common->new_fsg can change while
>> do_set_interface() is running, so the value of common->new_fsg
>> that gets tested after do_set_interface returns needs to be
>> the same as the value used by do_set_interface.
>>
>> Signed-off-by: Yang Wei <[email protected]>
>> Acked-by: Alan Stern <[email protected]>
>> ---
>> drivers/usb/gadget/f_mass_storage.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> Changes in v3 ----> v4
>>
>> move "new_fsg = common->new_fsg" out of comm->lock protection, and
>> refine the commit log.
>>
>> Thanks
>> Wei
>>
>> diff --git a/drivers/usb/gadget/f_mass_storage.c
>> b/drivers/usb/gadget/f_mass_storage.c
>> index b963939..a7e24c8 100644
>> --- a/drivers/usb/gadget/f_mass_storage.c
>> +++ b/drivers/usb/gadget/f_mass_storage.c
>> @@ -2342,6 +2342,7 @@ static void handle_exception(struct fsg_common
>> *common)
>> struct fsg_buffhd *bh;
>> enum fsg_state old_state;
>> struct fsg_lun *curlun;
>> + struct fsg_dev *new_fsg;
>> unsigned int exception_req_tag;
>> /*
>> @@ -2460,8 +2461,9 @@ static void handle_exception(struct fsg_common
>> *common)
>> break;
>> case FSG_STATE_CONFIG_CHANGE:
>> - do_set_interface(common, common->new_fsg);
>> - if (common->new_fsg)
>> + new_fsg = common->new_fsg;
>> + do_set_interface(common, new_fsg);
>> + if (new_fsg)
>> usb_composite_setup_continue(common->cdev);
>> break;
>
>
>
On Tue, 17 Jun 2014, Yang,Wei wrote:
> On 06/15/2014 10:42 AM, Yang,Wei wrote:
> > Its v4, sorry for missing it in subject.
>
> Alan, How about this version?
>
> Cheers
> Wei
...
> >> Signed-off-by: Yang Wei <[email protected]>
> >> Acked-by: Alan Stern <[email protected]>
That is a strange question to ask. If you did not know that I approved
the patch, why did you insert my Acked-By:?
Alan Stern
On Sun, Jun 15 2014, [email protected] wrote:
> From: Yang Wei <[email protected]>
>
> While loading g_mass_storage module, the following warning
> is triggered.
>
> WARNING: at drivers/usb/gadget/composite.c:
> usb_composite_setup_continue: Unexpected call
> Modules linked in: fat vfat minix nls_cp437 nls_iso8859_1 g_mass_storage
> [<800179cc>] (unwind_backtrace+0x0/0x104) from [<80619608>] (dump_stack+0x20/0x24)
> [<80619608>] (dump_stack+0x20/0x24) from [<80025100>] (warn_slowpath_common+0x64/0x74)
> [<80025100>] (warn_slowpath_common+0x64/0x74) from [<800251cc>] (warn_slowpath_fmt+0x40/0x48)
> [<800251cc>] (warn_slowpath_fmt+0x40/0x48) from [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage])
> [<7f047774>] (usb_composite_setup_continue+0xb4/0xbc [g_mass_storage]) from [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage])
> [<7f047ad4>] (handle_exception+0x358/0x3e4 [g_mass_storage]) from [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage])
> [<7f048080>] (fsg_main_thread+0x520/0x157c [g_mass_storage]) from [<8004bc90>] (kthread+0x98/0x9c)
> [<8004bc90>] (kthread+0x98/0x9c) from [<8000faec>] (kernel_thread_exit+0x0/0x8)
>
> The root cause is that the existing code fails to take into
> account the possibility that common->new_fsg can change while
> do_set_interface() is running, so the value of common->new_fsg
> that gets tested after do_set_interface returns needs to be
> the same as the value used by do_set_interface.
>
> Signed-off-by: Yang Wei <[email protected]>
> Acked-by: Alan Stern <[email protected]>
Acked-by: Michal Nazarewicz <[email protected]>
> ---
> drivers/usb/gadget/f_mass_storage.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> Changes in v3 ----> v4
>
> move "new_fsg = common->new_fsg" out of comm->lock protection, and refine the commit log.
>
> Thanks
> Wei
>
> diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
> index b963939..a7e24c8 100644
> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -2342,6 +2342,7 @@ static void handle_exception(struct fsg_common *common)
> struct fsg_buffhd *bh;
> enum fsg_state old_state;
> struct fsg_lun *curlun;
> + struct fsg_dev *new_fsg;
> unsigned int exception_req_tag;
>
> /*
> @@ -2460,8 +2461,9 @@ static void handle_exception(struct fsg_common *common)
> break;
>
> case FSG_STATE_CONFIG_CHANGE:
> - do_set_interface(common, common->new_fsg);
> - if (common->new_fsg)
> + new_fsg = common->new_fsg;
> + do_set_interface(common, new_fsg);
> + if (new_fsg)
> usb_composite_setup_continue(common->cdev);
> break;
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--
On 06/17/2014 10:18 PM, Alan Stern wrote:
> That is a strange question to ask. If you did not know that I approved
> the patch, why did you insert my Acked-By:?
I added your Acked-By, as when you reviewed V3, you mentioned that I
*may* add your Acked-by in this patch. If I misunderstood your point, I
am so sorry for that.
Best Regards
Wei
> With that change, you may add
>
> Acked-by: Alan Stern<[email protected]>
>
> Alan Stern
On Wed, Jun 18 2014, Yang,Wei wrote:
> On 06/17/2014 10:18 PM, Alan Stern wrote:
>> That is a strange question to ask. If you did not know that I approved
>> the patch, why did you insert my Acked-By:?
> I added your Acked-By, as when you reviewed V3, you mentioned that I
> *may* add your Acked-by in this patch. If I misunderstood your point, I
> am so sorry for that.
Alan's point is that if you have any doubts whether someone approves
your patch you should *not* add their Acked-by. So adding someone's
Acked-by and then asking if they are fine with the patch is
contradictory -- the former indicates that you believe the person is
fine with the patch, while the latter shows that you aren't.
Alan wrote:
>>> With that change, you may add
>>>
>>> Acked-by: Alan Stern <[email protected]>
so after adding “that change” you are in your right to add Alan's
Acked-by, but what that also means is that Alan will be fine with the
patch if you make the requested change, so you don't need to ask for an
opinion again.
PS. Note that “having any doubts” also means that if someone gave you
their Acked-by, but then you substantially rewrite the patch, you
probably should consider removing their Acked-by.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--
On Wed, 18 Jun 2014, Michal Nazarewicz wrote:
> On Wed, Jun 18 2014, Yang,Wei wrote:
> > On 06/17/2014 10:18 PM, Alan Stern wrote:
> >> That is a strange question to ask. If you did not know that I approved
> >> the patch, why did you insert my Acked-By:?
>
> > I added your Acked-By, as when you reviewed V3, you mentioned that I
> > *may* add your Acked-by in this patch. If I misunderstood your point, I
> > am so sorry for that.
>
> Alan's point is that if you have any doubts whether someone approves
> your patch you should *not* add their Acked-by. So adding someone's
> Acked-by and then asking if they are fine with the patch is
> contradictory -- the former indicates that you believe the person is
> fine with the patch, while the latter shows that you aren't.
>
> Alan wrote:
>
> >>> With that change, you may add
> >>>
> >>> Acked-by: Alan Stern <[email protected]>
>
> so after adding “that change” you are in your right to add Alan's
> Acked-by, but what that also means is that Alan will be fine with the
> patch if you make the requested change, so you don't need to ask for an
> opinion again.
>
> PS. Note that “having any doubts” also means that if someone gave you
> their Acked-by, but then you substantially rewrite the patch, you
> probably should consider removing their Acked-by.
Exactly so.
Alan Stern
On 06/18/2014 07:44 PM, Michal Nazarewicz wrote:
> On Wed, Jun 18 2014, Yang,Wei wrote:
>> On 06/17/2014 10:18 PM, Alan Stern wrote:
>>> That is a strange question to ask. If you did not know that I approved
>>> the patch, why did you insert my Acked-By:?
>> I added your Acked-By, as when you reviewed V3, you mentioned that I
>> *may* add your Acked-by in this patch. If I misunderstood your point, I
>> am so sorry for that.
> Alan's point is that if you have any doubts whether someone approves
> your patch you should *not* add their Acked-by. So adding someone's
> Acked-by and then asking if they are fine with the patch is
> contradictory -- the former indicates that you believe the person is
> fine with the patch, while the latter shows that you aren't.
>
> Alan wrote:
>
>>>> With that change, you may add
>>>>
>>>> Acked-by: Alan Stern <[email protected]>
> so after adding “that change” you are in your right to add Alan's
> Acked-by, but what that also means is that Alan will be fine with the
> patch if you make the requested change, so you don't need to ask for an
> opinion again.
>
> PS. Note that “having any doubts” also means that if someone gave you
> their Acked-by, but then you substantially rewrite the patch, you
> probably should consider removing their Acked-by.
>
Michal, Thank you for your detailed explanation, it is very helpful to me.
Best Regards
Wei