lkml.org 
[lkml]   [2014]   [Jun]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v1] USB:gadget: Fix a warning while loading g_mass_storage
    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 Wei.Yang@windriver.com wrote:
    >>
    >>> From: Yang Wei <Wei.Yang@windriver.com>
    >>>
    >>> 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 <r.baldyga@samsung.com>
    > 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 <r.baldyga@samsung.com>
    > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
    > Signed-off-by: Felipe Balbi <balbi@ti.com>
    >
    > 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
    >>
    >>
    >>
    >



    \
     
     \ /
      Last update: 2014-06-04 05:21    [W:2.832 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site