Re: [PATCH v2] usb: gadget: Add per-lun inquiry string
-<| Quoting Philipp Gesang , on Monday,
2016-08-01 17:04:19 |>-
> Introduce an attribute "inquiry_string" to the lun.
>
> In some environments, e. g. BIOS boot menus, the inquiry string
> is the only information about devices presented to the user. The
> default string depends on the "cdrom" bit of the first lun as
> well as the kernel version and allows no further customization.
> So without access to the client it is not obvious which gadget is
> active at a given point and what any of the available luns might
> contain.
>
> If "inquiry_string" is ignored or set to the empty string, the
> old behavior is preserved.
>
> Signed-off-by: Philipp Gesang
> ---
> drivers/usb/gadget/function/f_mass_storage.c | 28
> ++--
> drivers/usb/gadget/function/f_mass_storage.h | 1 +
> drivers/usb/gadget/function/storage_common.c | 24
> drivers/usb/gadget/function/storage_common.h | 10 ++
> 4 files changed, 57 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c
> b/drivers/usb/gadget/function/f_mass_storage.c
> index 5c6d4d7..efcc3de 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -311,11 +311,7 @@ struct fsg_common {
> /* Gadget's private data. */
> void*private_data;
>
> - /*
> - * Vendor (8 chars), product (16 chars), release (4
> - * hexadecimal digits) and NUL byte
> - */
> - char inquiry_string[8 + 16 + 4 + 1];
> + char inquiry_string[INQUIRY_STRING_LEN];
>
> struct kref ref;
> };
> @@ -1107,7 +1103,12 @@ static int do_inquiry(struct fsg_common *common,
> struct fsg_buffhd *bh)
> buf[5] = 0; /* No special options */
> buf[6] = 0;
> buf[7] = 0;
> - memcpy(buf + 8, common->inquiry_string, sizeof common->inquiry_string);
> + if (curlun->inquiry_string[0])
> + memcpy(buf + 8, curlun->inquiry_string,
> +sizeof(curlun->inquiry_string));
> + else
> + memcpy(buf + 8, common->inquiry_string,
> +sizeof(common->inquiry_string));
> return 36;
> }
>
> @@ -3225,12 +3226,27 @@ static ssize_t fsg_lun_opts_nofua_store(struct
> config_item *item,
>
> CONFIGFS_ATTR(fsg_lun_opts_, nofua);
>
> +static ssize_t fsg_lun_opts_inquiry_string_show(struct config_item *item,
> + char *page)
> +{
> + return fsg_show_inquiry_string(to_fsg_lun_opts(item)->lun, page);
> +}
> +
> +static ssize_t fsg_lun_opts_inquiry_string_store(struct config_item *item,
> + const char *page, size_t len)
> +{
> + return fsg_store_inquiry_string(to_fsg_lun_opts(item)->lun, page, len);
> +}
> +
> +CONFIGFS_ATTR(fsg_lun_opts_, inquiry_string);
> +
> static struct configfs_attribute *fsg_lun_attrs[] = {
> &fsg_lun_opts_attr_file,
> &fsg_lun_opts_attr_ro,
> &fsg_lun_opts_attr_removable,
> &fsg_lun_opts_attr_cdrom,
> &fsg_lun_opts_attr_nofua,
> + &fsg_lun_opts_attr_inquiry_string,
> NULL,
> };
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.h
> b/drivers/usb/gadget/function/f_mass_storage.h
> index b6a9918..d390231 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.h
> +++ b/drivers/usb/gadget/function/f_mass_storage.h
> @@ -100,6 +100,7 @@ struct fsg_lun_config {
> char removable;
> char cdrom;
> char nofua;
> + char inquiry_string[INQUIRY_STRING_LEN];
> };
>
> struct fsg_config {
> diff --git a/drivers/usb/gadget/function/storage_common.c
> b/drivers/usb/gadget/function/storage_common.c
> index 990df22..8fbf686 100644
> --- a/drivers/usb/gadget/function/storage_common.c
> +++ b/drivers/usb/gadget/function/storage_common.c
> @@ -369,6 +369,12 @@ ssize_t fsg_show_removable(struct fsg_lun *curlun, char
> *buf)
> }
> EXPORT_SYMBOL_GPL(fsg_show_removable);
>
> +ssize_t fsg_show_inquiry_string(struct fsg_lun *curlun, char *buf)
> +{
> + return sprintf(buf, "%s\n", curlun->inquiry_string);
> +}
> +EXPORT_SYMBOL_GPL(fsg_show_inquiry_string);
> +
> /*
> * The caller must hold fsg->filesem for reading when calling this function.
> */
> @@ -499,4 +505,22 @@ ssize_t fsg_store_removable(struct fsg_lun *curlun,
> const char *buf,
> }
> EXPORT_SYMBOL_GPL(fsg_store_removable);
>
> +ssize_t fsg_store_inquiry_string(struct fsg_lun *curlun, const char *buf,
> + size_t count)
> +{
> + const size_t len = min(count, sizeof(curlun->inquiry_string));
> +
> + if (len == 0 || buf[0] == '\n') {
> + curlun->inquiry_string[0] = 0;
> + } else {
> + snprintf(curlun->inquiry_string,
> + sizeof(curlun->inquiry_string), "%-28s", buf);
> + if (curlun->inquiry_string[len-1] == '\n')
> + curlun->inquiry_string[l
Re: Memory barrier needed with wake_up_process()?
On Sat, Sep 03, 2016 at 10:16:31AM -0400, Alan Stern wrote:
> > Sorry, but that is horrible code. A barrier cannot ensure writes are
> > 'complete', at best they can ensure order between writes (or reads
> > etc..).
>
> The code is better than the comment. What I really meant was that the
> write of bh->state needs to be visible to the thread after it wakes up
> (or after it checks the wakeup condition and skips going to sleep).
Yeah, I got that.
> > Also, looking at that thing, that common->thread_wakeup_needed variable
> > is 100% redundant. All sleep_thread() invocations are inside a loop of
> > sorts and basically wait for other conditions to become true.
> >
> > For example:
> >
> > while (bh->state != BUF_STATE_EMPTY) {
> > rc = sleep_thread(common, false);
> > if (rc)
> > return rc;
> > }
> >
> > All you care about there is bh->state, _not_
> > common->thread_wakeup_needed.
>
> You know, I never went through and verified that _all_ the invocations
> of sleep_thread() are like that.
Well, thing is, they're all inside a loop which checks other conditions
for forward progress. Therefore the loop inside sleep_thread() is
pointless. Even if you were to return early, you'd simply loop in the
outer loop and go back to sleep again.
> In fact, I wrote the sleep/wakeup
> routines _before_ the rest of the code, and I didn't know in advance
> exactly how they were going to be called.
Still seems strange to me, why not use wait-queues for the first cut?
Only if you find a performance issue with wait-queues, which cannot be
fixed in the wait-queue proper, then do you do custom thingies.
Starting with a custom sleeper, just doesn't make sense to me.
> > That said, I cannot spot an obvious fail, but the code can certainly use
> > help.
>
> The problem may be that when the thread wakes up (or skips going to
> sleep), it needs to see more than just bh->state. Those other values
> it needs are not written by the same CPU that calls wakeup_thread(),
> and so to ensure that they are visible that smp_wmb() really ought to
> be smp_mb() (and correspondingly in the thread. That's what Felipe has
> been testing.
So you're saying something like:
CPU0CPU1CPU2
X = 1 sleep_thread()
wakeup_thread()
r = X
But how does CPU1 know to do the wakeup? That is, how are CPU0 and CPU1
coupled.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory barrier needed with wake_up_process()?
On Sat, Sep 03, 2016 at 04:51:07PM +0300, Felipe Balbi wrote: > > That said, I cannot spot an obvious fail, > > okay, but a fail does exist. Any hints on what extra information I could > capture to help figuring this one out? More information on which sleep is not waking woudl help I suppose. That greatly limits the amount of code one has to stare at. Also, a better picture of all threads involved in the wakeup. Alan seems to suggest there's multiple CPUs involved in the wakeup. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: g_mass_storage not queueing requests
Hi,
Alan Stern writes:
> On Sat, 3 Sep 2016, Felipe Balbi wrote:
>
>> Hi,
>>
>> Alan Stern writes:
>> > On Fri, 2 Sep 2016, Felipe Balbi wrote:
>> >
>> >>
>> >> Hi,
>> >>
>> >> Alan Stern writes:
>> >>
>> >> [...]
>> >>
>> >> > No, that would be too late. The barrier needs to go between the write
>> >> > to common->thead_wakeup_needed and the call to wake_up_process().
>> >>
>> >> alright, I tested this but it doesn't work. It still hangs :-(
>> >
>> > Okay, I was wrong. But see my recent reply to Paul; maybe the
>> > smp_wmb() in wakeup_thread() needs to be smp_mb().
>>
>> okay, this seems to be working so far:
>>
>> modified drivers/usb/gadget/function/f_mass_storage.c
>> @@ -395,7 +395,7 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct
>> usb_ep *ep)
>> /* Caller must hold fsg->lock */
>> static void wakeup_thread(struct fsg_common *common)
>> {
>> -smp_wmb(); /* ensure the write of bh->state is complete */
>> +smp_mb(); /* ensure the write of bh->state is complete */
>> /* Tell the main thread that something has happened */
>> common->thread_wakeup_needed = 1;
>> if (common->thread_task)
>> @@ -626,7 +626,7 @@ static int sleep_thread(struct fsg_common *common, bool
>> can_freeze)
>> }
>> __set_current_state(TASK_RUNNING);
>> common->thread_wakeup_needed = 0;
>> -smp_rmb(); /* ensure the latest bh->state is visible */
>> +smp_mb(); /* ensure the latest bh->state is visible */
>> return rc;
>> }
>>
>> I'll keep it running over the weekend.
>
> Can you also check the question I mentioned earlier? That is, if you
> revert to the original code then when the thread hangs, what does the
> call to received_cbw() in get_next_command() return? Or does that
> routine not get called at all because the preceding sleep_thread() is
> the one that doesn't return?
here are the last few calls
> [ 34.122210] ==> bulk_out_complete 484
CBW received
> [ 34.122213] ==> wakeup_thread 403: caller
> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
set thread_wakeup_needed to 1 and wakeup fsg_main_thread
> [ 34.122255] ==> fsg_main_thread 2587 -> get_next_command() -> 0
get_next_command()
> [ 34.122328] ==> fsg_main_thread 2608 -> send_status -> 0
CSW
> [ 34.122331] ==> get_next_command 2251 -> state != full
now waiting for CBW:
> [ 34.122333] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
> [usb_f_mass_storage]: going to sleep
thread goes to sleep
> [ 34.122379] ==> bulk_in_complete 461
We send data to host
> [ 34.122381] ==> wakeup_thread 403: caller
> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122383] ==> get_next_command 2251 -> state != full
still waiting for CBW
> [ 34.122385] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
> [usb_f_mass_storage]: going to sleep
> [ 34.122386] ==> bulk_in_complete 461
send data to host
> [ 34.122388] ==> wakeup_thread 403: caller
> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122389] ==> get_next_command 2251 -> state != full
still waiting for CBW
> [ 34.122391] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
> [usb_f_mass_storage]: going to sleep
> [ 34.122468] ==> bulk_in_complete 461
> [ 34.122469] ==> wakeup_thread 403: caller
> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122470] ==> get_next_command 2251 -> state != full
> [ 34.122473] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
> [usb_f_mass_storage]: going to sleep
> [ 34.122473] ==> bulk_in_complete 461
> [ 34.122474] ==> wakeup_thread 403: caller
> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122476] ==> get_next_command 2251 -> state != full
> [ 34.122479] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
> [usb_f_mass_storage]: going to sleep
> [ 34.122556] ==> bulk_in_complete 461
> [ 34.122557] ==> wakeup_thread 403: caller
> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122559] ==> get_next_command 2251 -> state != full
> [ 34.122561] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
> [usb_f_mass_storage]: going to sleep
> [ 34.122561] ==> bulk_in_complete 461
> [ 34.122563] ==> wakeup_thread 403: caller
> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122563] ==> get_next_command 2251 -> state != full
> [ 34.122565] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
> [usb_f_mass_storage]: going to sleep
> [ 34.122647] ==> bulk_in_complete 461
> [ 34.122648] ==> wakeup_thread 403: caller
> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122650] ==> get_next_command 2251 -> state != full
> [ 34.122653] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
> [usb_f_mass_storage]: going to sleep
> [ 34.122654] ==> bulk_in_complete 461
> [ 34.122655] ==> wake
Re: Memory barrier needed with wake_up_process()?
On Sat, Sep 03, 2016 at 10:49:39AM -0400, Alan Stern wrote: > On Sat, 3 Sep 2016, Alan Stern wrote: > > > In other words, we have: > > > > CPU 0 CPU 1 > > - - > > Start DMA Handle DMA-complete irq > > Sleep until bh->state Set bh->state > > smp_wmb() > > Wake up CPU 0 > > smp_rmb() > > Compute rc based on contents > > of the DMA buffer > > > > This was written many years ago, at a time when I did not fully > > understand all the details of memory ordering. Do you agree that both > > of those barriers should really be smp_mb()? That's what Felipe has > > been testing. > > Actually, seeing it written out like this, one realizes that it really > ought to be: > > CPU 0 CPU 1 > - - > Start DMA Handle DMA-complete irq > Sleep until bh->state smp_mb() > set bh->state > Wake up CPU 0 > smp_mb() > Compute rc based on contents of the DMA buffer > > (Bear in mind also that on some platforms, the I/O operation is carried > out by PIO rather than DMA.) I'm sorry, but I still don't follow. This could be because I seldom interact with DMA agents and therefore am not familiar with that stuff. Also, smp_mb() doesn't necessarily interact with MMIO/DMA at all IIRC. Its only defined to do CPU/CPU interactions. I would very much expect the device IO stuff to order things for us in this case. "Start DMA" should very much include sufficient fences to ensure the data under DMA is visible to the DMA engine, this would very much include things like flushing store buffers and maybe even writeback caches, depending on platform needs. At the same time, I would expect "Handle DMA-complete irq", even if it were done with a PIO polling loop, to guarantee ordering against later operations such that 'complete' really means that. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb: gadget: Add per-lun inquiry string
Hi, On 08/15/2016 10:37 AM, Philipp Gesang wrote: > Ping. > > @Krzysztof The resubmitted patch bounced from the Cc: to you due > to your MX blocking one of our hosters. I can resend in case you > didn’t receive the mail from the list. Sorry for my late response but I have been out of office for some time. > > -<| Quoting Philipp Gesang , on Monday, > 2016-08-01 17:04:19 |>- >> Introduce an attribute "inquiry_string" to the lun. >> >> In some environments, e. g. BIOS boot menus, the inquiry string >> is the only information about devices presented to the user. The >> default string depends on the "cdrom" bit of the first lun as >> well as the kernel version and allows no further customization. >> So without access to the client it is not obvious which gadget is >> active at a given point and what any of the available luns might >> contain. >> >> If "inquiry_string" is ignored or set to the empty string, the >> old behavior is preserved. >> >> Signed-off-by: Philipp Gesang Reviewed-by: Krzysztof Opasiak Best regards, -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 17/44] usb: host: xhci-tegra: don't print on ENOMEM
On Thu, Aug 25, 2016 at 07:39:09PM +0200, Wolfram Sang wrote: > All kmalloc-based functions print enough information on failures. > > Signed-off-by: Wolfram Sang > --- > drivers/usb/host/xhci-tegra.c | 1 - > 1 file changed, 1 deletion(-) Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [PATCH V4] leds: trigger: Introduce an USB port trigger
On 4 September 2016 at 02:24, Alan Stern wrote: > On Sat, 3 Sep 2016, Jacek Anaszewski wrote: > >> >> The remaining issue is the sysfs interface design for defining and >> >> presenting multiple USB ports. I'm still in favour of a single >> >> attribute with space separated list. This scheme is commonly used >> >> in existing interfaces. >> > >> > No such interface is needed if you do things the way I outlined above. >> > Each trigger would require the user to specify either one port, one >> > hub, or nothing at all. Multiple ports would not be used. >> >> The patch assumes that it is possible to register trigger for many >> ports. > > The patch could be changed to remove that assumption. I did this assumption for a reason. There are many devices with more than 1 USB port and these ports are handled by the same controllers, just different ports. Let me share USB details of my SmartRG SR400ac device with 2 physical USB ports: Physical USB 3.0 port is handled by following controllers and their root hub ports: 1) OHCI's port 1 2) EHCI's port 1 3) XHCI's port 1 Physical USB 2.0 port is handled by following controllers and their root hub ports: 1) OHCI's port 2 2) EHCI's port 2 This devices has 3 separated LEDs. I'll list them and tell which ports should be assigned to them: bcm53xx:green:usb3 should have assigned: 1) OHCI's port 1 2) EHCI's port 1 bcm53xx:white:usb3 should have assigned: 1) XHCI's port 1 bcm53xx:white:usb2 should have assigned: 1) OHCI's port 2 2) EHCI's port 2 As you can see, I need to specify ports precisely. Assigning a single port is not enough. Assigning whole hub is too much. So none of: > echo usb1-4.2 >/sys/class/led/foo/trigger > echo hub1-4 >/sys/class/led/foo/trigger > echo usb >/sys/class/led/foo/trigger would be sufficient here :( -- Rafał -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: g_mass_storage not queueing requests
Hi Alan,
Felipe Balbi writes:
> Hi,
>
> Alan Stern writes:
>> On Sat, 3 Sep 2016, Felipe Balbi wrote:
>>
>>> Hi,
>>>
>>> Alan Stern writes:
>>> > On Fri, 2 Sep 2016, Felipe Balbi wrote:
>>> >
>>> >>
>>> >> Hi,
>>> >>
>>> >> Alan Stern writes:
>>> >>
>>> >> [...]
>>> >>
>>> >> > No, that would be too late. The barrier needs to go between the write
>>> >> > to common->thead_wakeup_needed and the call to wake_up_process().
>>> >>
>>> >> alright, I tested this but it doesn't work. It still hangs :-(
>>> >
>>> > Okay, I was wrong. But see my recent reply to Paul; maybe the
>>> > smp_wmb() in wakeup_thread() needs to be smp_mb().
>>>
>>> okay, this seems to be working so far:
>>>
>>> modified drivers/usb/gadget/function/f_mass_storage.c
>>> @@ -395,7 +395,7 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct
>>> usb_ep *ep)
>>> /* Caller must hold fsg->lock */
>>> static void wakeup_thread(struct fsg_common *common)
>>> {
>>> - smp_wmb(); /* ensure the write of bh->state is complete */
>>> + smp_mb(); /* ensure the write of bh->state is complete */
>>> /* Tell the main thread that something has happened */
>>> common->thread_wakeup_needed = 1;
>>> if (common->thread_task)
>>> @@ -626,7 +626,7 @@ static int sleep_thread(struct fsg_common *common, bool
>>> can_freeze)
>>> }
>>> __set_current_state(TASK_RUNNING);
>>> common->thread_wakeup_needed = 0;
>>> - smp_rmb(); /* ensure the latest bh->state is visible */
>>> + smp_mb(); /* ensure the latest bh->state is visible */
>>> return rc;
>>> }
>>>
>>> I'll keep it running over the weekend.
>>
>> Can you also check the question I mentioned earlier? That is, if you
>> revert to the original code then when the thread hangs, what does the
>> call to received_cbw() in get_next_command() return? Or does that
>> routine not get called at all because the preceding sleep_thread() is
>> the one that doesn't return?
>
> here are the last few calls
>
>> [ 34.122210] ==> bulk_out_complete 484
>
> CBW received
>
>> [ 34.122213] ==> wakeup_thread 403: caller
>> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
>
> set thread_wakeup_needed to 1 and wakeup fsg_main_thread
>
>> [ 34.122255] ==> fsg_main_thread 2587 -> get_next_command() -> 0
>
> get_next_command()
>
>> [ 34.122328] ==> fsg_main_thread 2608 -> send_status -> 0
>
> CSW
>
>> [ 34.122331] ==> get_next_command 2251 -> state != full
>
> now waiting for CBW:
>
>> [ 34.122333] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
>> [usb_f_mass_storage]: going to sleep
>
> thread goes to sleep
>
>> [ 34.122379] ==> bulk_in_complete 461
>
> We send data to host
>
>> [ 34.122381] ==> wakeup_thread 403: caller
>> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
>> [ 34.122383] ==> get_next_command 2251 -> state != full
>
> still waiting for CBW
>
>> [ 34.122385] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
>> [usb_f_mass_storage]: going to sleep
>> [ 34.122386] ==> bulk_in_complete 461
>
> send data to host
>
>> [ 34.122388] ==> wakeup_thread 403: caller
>> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
>> [ 34.122389] ==> get_next_command 2251 -> state != full
>
> still waiting for CBW
>
>> [ 34.122391] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
>> [usb_f_mass_storage]: going to sleep
>> [ 34.122468] ==> bulk_in_complete 461
>> [ 34.122469] ==> wakeup_thread 403: caller
>> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
>> [ 34.122470] ==> get_next_command 2251 -> state != full
>> [ 34.122473] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
>> [usb_f_mass_storage]: going to sleep
>> [ 34.122473] ==> bulk_in_complete 461
>> [ 34.122474] ==> wakeup_thread 403: caller
>> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
>> [ 34.122476] ==> get_next_command 2251 -> state != full
>> [ 34.122479] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
>> [usb_f_mass_storage]: going to sleep
>> [ 34.122556] ==> bulk_in_complete 461
>> [ 34.122557] ==> wakeup_thread 403: caller
>> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
>> [ 34.122559] ==> get_next_command 2251 -> state != full
>> [ 34.122561] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
>> [usb_f_mass_storage]: going to sleep
>> [ 34.122561] ==> bulk_in_complete 461
>> [ 34.122563] ==> wakeup_thread 403: caller
>> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
>> [ 34.122563] ==> get_next_command 2251 -> state != full
>> [ 34.122565] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
>> [usb_f_mass_storage]: going to sleep
>> [ 34.122647] ==> bulk_in_complete 461
>> [ 34.122648] ==> wakeup_thread 403: caller
>> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
>> [ 34.122650] ==> get_next_command 2251 -> state != full
>> [ 34.12
Re: Memory barrier needed with wake_up_process()?
On Sat, Sep 03, 2016 at 12:16:29AM +0200, Peter Zijlstra wrote: > On Sat, Sep 03, 2016 at 12:14:13AM +0200, Peter Zijlstra wrote: > > On Fri, Sep 02, 2016 at 04:16:54PM -0400, Alan Stern wrote: > > > > > > Actually, that's not entirely true (although presumably it works okay > > > for most architectures). > > > > Yeah, all load-store archs (with exception of PowerPC and ARM64 and > > possibly MIPS) implement ACQUIRE with a general fence (after the ll/sc). > > > > ( and MIPS doesn't use their fancy barriers in Linux ) > > > > PowerPC does the full fence for smp_mb__before_spinlock, which leaves > > ARM64, I'm not sure its correct, but I'm way too tired to think about > > that now. > > > > The TSO archs imply full barriers with all atomic RmW ops and are > > therefore also good. > > > > Forgot to Cc Will. Will, does ARM64 need to make smp_mb__before_spinlock > smp_mb() too? Yes, probably. Just to confirm, the test is something like: CPU0 Wx=1 smp_mb__before_spinlock() LOCK(y) Rz=0 CPU1 Wz=1 smp_mb() Rx=0 and that should be forbidden? Will -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: g_mass_storage not queueing requests
Hi,
Felipe Balbi writes:
[...]
>> get_next_command() still sees bh->state != BUF_STATE_FULL.
>>
>>> [ 34.123286] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
>>> [usb_f_mass_storage]: going to sleep
>>> [ 34.123289] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
>>> [usb_f_mass_storage]: going to sleep
>>
>> so it goes to sleep and never wakes up again.
>
> one thing that caught my attention is that not all accesses to bh->state
> are locked. Is that an oversight or is there a valid reason for that?
>
> Considering that accessing bh->state would always guarantee a full
> barrier (at least on x86, per Peter Z), I'm wondering if we should make
> every access to bh->state locked. Or, perhaps, they should all be
> preceeded with a call to smp_mb() in order to guarantee that previous
> writes to bh->state are seen by current CPU?
>
> I can test that out, but I'll wait for a proper patch from you as I
> cannot predict all memory ordering problems that could arise from
> current code. Still, f_mass_storage.c looks really odd :-s
okay, I found the one place where changing smp_wmb() to smp_mb() solves
the problem
@@ -395,7 +395,7 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct usb_ep
*ep)
/* Caller must hold fsg->lock */
static void wakeup_thread(struct fsg_common *common)
{
- smp_wmb(); /* ensure the write of bh->state is complete */
+ smp_mb(); /* ensure the write of bh->state is complete */
/* Tell the main thread that something has happened */
common->thread_wakeup_needed = 1;
if (common->thread_task)
Based on what Peter Z said on our other thread, this shouldn't be
necessary for this particular occasion. bulk_out_complete() writes to
bh->state inside a spin_lock() which is implemented with a full barrier
in x86.
So, how come we need another full barrier in wakeup_thread() ?
Is is because the call to wakeup_process() is *also* inside the
spinlocked region and the full barrier is added only at spin_unlock()
time? (just wondering here)
cheers
--
balbi
signature.asc
Description: PGP signature
Re: g_mass_storage not queueing requests
Hi,
Felipe Balbi writes:
>>> get_next_command() still sees bh->state != BUF_STATE_FULL.
>>>
[ 34.123286] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
[usb_f_mass_storage]: going to sleep
[ 34.123289] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00
[usb_f_mass_storage]: going to sleep
>>>
>>> so it goes to sleep and never wakes up again.
>>
>> one thing that caught my attention is that not all accesses to bh->state
>> are locked. Is that an oversight or is there a valid reason for that?
>>
>> Considering that accessing bh->state would always guarantee a full
>> barrier (at least on x86, per Peter Z), I'm wondering if we should make
>> every access to bh->state locked. Or, perhaps, they should all be
>> preceeded with a call to smp_mb() in order to guarantee that previous
>> writes to bh->state are seen by current CPU?
>>
>> I can test that out, but I'll wait for a proper patch from you as I
>> cannot predict all memory ordering problems that could arise from
>> current code. Still, f_mass_storage.c looks really odd :-s
>
> okay, I found the one place where changing smp_wmb() to smp_mb() solves
> the problem
>
> @@ -395,7 +395,7 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct
> usb_ep *ep)
> /* Caller must hold fsg->lock */
> static void wakeup_thread(struct fsg_common *common)
> {
> - smp_wmb(); /* ensure the write of bh->state is complete */
> + smp_mb(); /* ensure the write of bh->state is complete */
> /* Tell the main thread that something has happened */
> common->thread_wakeup_needed = 1;
> if (common->thread_task)
>
> Based on what Peter Z said on our other thread, this shouldn't be
> necessary for this particular occasion. bulk_out_complete() writes to
> bh->state inside a spin_lock() which is implemented with a full barrier
> in x86.
>
> So, how come we need another full barrier in wakeup_thread() ?
>
> Is is because the call to wakeup_process() is *also* inside the
> spinlocked region and the full barrier is added only at spin_unlock()
> time? (just wondering here)
okay, I'm thinking that's a plausible explanation, please correct me if
I'm wrong. What I'm speculating now is that inside a locked region,
reordering can happen (can it?) which means that our:
spin_lock(&common->lock);
bh->outreq_busy = 0;
bh->state = BUF_STATE_FULL;
wakeup_thread(common);
spin_unlock(&common->lock);
ends up being reordered as:
spin_lock(&common->lock);
bh->outreq_busy = 0;
wakeup_thread(common);
bh->state = BUF_STATE_FULL;
spin_unlock(&common->lock);
which triggers the fault. Is that possible?
Looking at the history.git tree [0] I found a little more of
file_storage gadget's history. Here are some of the details I could find
out:
When you first wrote the file_storage gadget, wakeup_thread() was called
outside the lock from bulk_{in,out}_complete() (see [1]). A follow-up
patch ended up moving wakeup_thread() inside the locked region (while
also removing a wait_queue()) which, I'm speculating, introduced the
race condition (see [2])
Another piece of speculation (which I can test, if necessary) is that
larger mass storage buffers (e.g. 64k or 128k) would hide this race
because it would take longer for UDC to complete the request. This might
be why we never saw this before with HS-only UDCs.
Are we really dealing with a regression introduced back in 2.6.15?
[0] https://git.kernel.org/cgit/linux/kernel/git/history/history.git/
[1]
https://git.kernel.org/cgit/linux/kernel/git/history/history.git/commit/?id=23a070c9db2e444360f05cf69b33c8248b03bc06
[2]
https://git.kernel.org/cgit/linux/kernel/git/history/history.git/commit/?id=a21d4fed4b00eaf7e7c3b2e2b25de24f540bfa66
--
balbi
signature.asc
Description: PGP signature
Re: [uas] ORICO 2588US3 USB HDD enclosure goes offline after idle
On Sun, 2016-09-04 at 01:40 +0300, Dmitry Gutov wrote: > Hi all, > > The external HDD enclosure I've bought recently stops responding after > it's been idle for ~5 minutes. Excerpt from typical dmesg output > attached (the "Valid eCryptfs headers" are irrelevant, and there for > completeness). > > After searching the web, eventually I've come upon this Q&A, which > contains a viable workaround: > https://ask.fedoraproject.org/en/question/87852/usb3-external-drive-caddy-goes-offline-after-a-while/ This very much looks like power saving misbehaving. Are you using autosuspend on this device? Have you tried the RESET_RESUME quirk on the USB level? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [uas] ORICO 2588US3 USB HDD enclosure goes offline after idle
On 05.09.2016 13:42, Oliver Neukum wrote: This very much looks like power saving misbehaving. It does. But it doesn't seem like my distro enables autosuspend there by default. Are you using autosuspend on this device? $ lsusb ... Bus 002 Device 002: ID 357d:7788 Sharkoon QuickPort XT ... $ cat /sys/bus/usb/devices/usb2/2-1/power/control on $ cat /sys/bus/usb/devices/usb2/2-2/power/control on Have you tried the RESET_RESUME quirk on the USB level? How do I do that? -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: g_mass_storage not queueing requests
Hi,
Felipe Balbi writes:
[..]
> okay, I found the one place where changing smp_wmb() to smp_mb() solves
> the problem
>
> @@ -395,7 +395,7 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct
> usb_ep *ep)
> /* Caller must hold fsg->lock */
> static void wakeup_thread(struct fsg_common *common)
> {
> - smp_wmb(); /* ensure the write of bh->state is complete */
> + smp_mb(); /* ensure the write of bh->state is complete */
> /* Tell the main thread that something has happened */
> common->thread_wakeup_needed = 1;
> if (common->thread_task)
turns out this helps, but it doesn't seem to be enough. I could still
see a hang followed by a bus reset.
I'll wait for your replies now.
cheers
--
balbi
signature.asc
Description: PGP signature
Re: [uas] ORICO 2588US3 USB HDD enclosure goes offline after idle
On Mon, 2016-09-05 at 14:01 +0300, Dmitry Gutov wrote: > On 05.09.2016 13:42, Oliver Neukum wrote: > > > This very much looks like power saving misbehaving. > > It does. But it doesn't seem like my distro enables autosuspend there by > default. > > > Are you using autosuspend on this device? > > $ lsusb > ... > Bus 002 Device 002: ID 357d:7788 Sharkoon QuickPort XT > ... > > $ cat /sys/bus/usb/devices/usb2/2-1/power/control > on > > $ cat /sys/bus/usb/devices/usb2/2-2/power/control > on OK > > > Have you tried the RESET_RESUME quirk on the USB > > level? > > How do I do that? You add the device to the kernel quirks list. But if you don't use autosuspend, the point is moot. Do you use LPM, as described in Documentation/usb/power-management.txt ? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [uas] ORICO 2588US3 USB HDD enclosure goes offline after idle
On 05.09.2016 14:19, Oliver Neukum wrote: You add the device to the kernel quirks list. But if you don't use autosuspend, the point is moot. The device does get spinned down (after not using it for a while, I have to wait before it responds for the first time), but not sure which method is used. Do you use LPM, as described in Documentation/usb/power-management.txt ? Is this what you're looking for? $ cat /sys/bus/usb/devices/2-1/power/usb3_hardware_lpm_u1 enabled $ cat /sys/bus/usb/devices/2-1/power/usb3_hardware_lpm_u2 enabled -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: xHCI problem? [was Re: Erratic USB device behavior and device loss]
-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 On Sun, 2016-09-04 at 15:46 -0400, Alan Stern wrote: > > This is not the problem I was discussing with Ulf. The problem was why > the device kept going into and out of runtime suspend every three > seconds. The kernel log above does not say whether this was happening. > One way to tell is to look at a usbmon trace (like we did before). https://people.debian.org/~rrs/tmp/usbmon.txt.gz This should have the logs you asked for, running on 4.8-rc4. - -- Ritesh Raj Sarraf RESEARCHUT - http://www.researchut.com "Necessity is the mother of invention." -BEGIN PGP SIGNATURE- iQIcBAEBCgAGBQJXzWwYAAoJEKY6WKPy4XVpFKgP/i76Ek/1e1CYqStgakQUtmPb eeGuSPFVqvQt4PEvGNvMtXcLMx5IKIEpKbqua7N4/ZfKmf5EA00wUCXzm85MnyAp fIN9K1uOW1ZepnsViJ/U1bNZvHVDXy/LEfd5me+7JzWsPqaR/KjyZmZ8VSlu+aov CvBDrt41x9plf1xJBYffHc2KvdM0lYFJjft2Gop9qr/hMhIy8RcJdbylPpnf4rtU cbm3Ti3DdA+r86MLCKagq50Iow3xcfBEEYZLnn+ucnpTOWz56TyO3+44sjgLgrpy IDRBlCbgk2RZkm8lpj+d052ssk3MjLBupBFkoRfXfXfear3zLrut2Q51xjeJIINz oZdmAxozsvGxCtgC6dIb/79clruL8kqdr/TqF/YtwzB6JnFvZEgzBcpAlxFBxTlK RtvAyPVs4Np1vtTli/Uv1O+odoR0FPhzpV+nDSsea7im6/oMav40UpXvgs72EEUS cr5SwY8xClfNeemCHFZ/wFeYwPfjPfNU5Lgm/Hkpx6oaOABCnfacF1nWxbIRbuoh gP9vW/Z0LI4azo940xlWfRiDck5W9IQXeI3v2wXVVIE8TDwxraQzyBXKKPBFxmfX jTKWLRDDqyOWGcWxLJpMHMOa9rNtAZeiBI7TMbnqZKj6eLsdjuOW76zDhEHnMAtb dUCOFYRG24arYTUpIVnB =YG3r -END PGP SIGNATURE- -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 3/4] usb: dwc2: assert phy reset when waking up in rk3288 platform
From: Randy Li > Sent: 03 September 2016 22:55 ... > + if (of_device_is_compatible(np, "rockchip,rk3288-usb") > + && (NULL != hsotg->phy->ops->reset)) > + hsotg->phy->ops->reset(hsotg->phy); > + Is this the only place ops->reset() is called? Probably much better to check it first. if (hsotg->phy->ops->reset && of_device_is_compatible(np, "rockchip,rk3288-usb") hsotg->phy->ops->reset(hsotg->phy); David -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory barrier needed with wake_up_process()?
On Mon, 5 Sep 2016, Peter Zijlstra wrote: > > You know, I never went through and verified that _all_ the invocations > > of sleep_thread() are like that. > > Well, thing is, they're all inside a loop which checks other conditions > for forward progress. Therefore the loop inside sleep_thread() is > pointless. Even if you were to return early, you'd simply loop in the > outer loop and go back to sleep again. > > > In fact, I wrote the sleep/wakeup > > routines _before_ the rest of the code, and I didn't know in advance > > exactly how they were going to be called. > > Still seems strange to me, why not use wait-queues for the first cut? > > Only if you find a performance issue with wait-queues, which cannot be > fixed in the wait-queue proper, then do you do custom thingies. > > Starting with a custom sleeper, just doesn't make sense to me. I really don't remember. Felipe says that the ancient history shows the initial implementation did use a wait-queue, and then it was changed. Perhaps I was imitating the structure of scsi_error_handler(). > > The problem may be that when the thread wakes up (or skips going to > > sleep), it needs to see more than just bh->state. Those other values > > it needs are not written by the same CPU that calls wakeup_thread(), > > and so to ensure that they are visible that smp_wmb() really ought to > > be smp_mb() (and correspondingly in the thread. That's what Felipe has > > been testing. > > So you're saying something like: > > > CPU0CPU1CPU2 > > X = 1 sleep_thread() > wakeup_thread() > r = X > > But how does CPU1 know to do the wakeup? That is, how are CPU0 and CPU1 > coupled. As mentioned later on, "CPU0" is actually a DMA master, not another CPU. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: g_mass_storage not queueing requests
On Mon, 5 Sep 2016, Felipe Balbi wrote: > >> [ 34.123281] ==> bulk_out_complete 484 > > > > received CBW. Now, bh->state is set to BUF_STATE_FULL, and ... > > > >> [ 34.123283] ==> wakeup_thread 403: caller > >> usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1 > > > > wakeup_thread() is called, however ... > > > >> [ 34.123283] ==> get_next_command 2251 -> state != full > > > > get_next_command() still sees bh->state != BUF_STATE_FULL. Okay, that's very strange. It suggests something may actively be changing bh->state back to BUF_STATE_EMPTY. I can't imagine what. > >> [ 34.123286] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 > >> [usb_f_mass_storage]: going to sleep > >> [ 34.123289] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 > >> [usb_f_mass_storage]: going to sleep > > > > so it goes to sleep and never wakes up again. It looks like the thread wakes up and goes back to sleep right away several times. This wouldn't happen if the problem were that the CPU didn't see the update to bh->state. > one thing that caught my attention is that not all accesses to bh->state > are locked. Is that an oversight or is there a valid reason for that? To the largest extent possible, I tried to write the driver so that the thread wouldn't have to do any locking, only the completion handlers would. Looking back on it now, I realize that even less locking should be necessary, because for each IN buffer the thread is a producer and the completion handler is a consumer, and for each OUT buffer the handler is a producer and the thread is a consumer. Producer/consumer patterns don't need locking, just proper ordering. As for what ordering is needed, I'll discuss that in the other email thread. > Considering that accessing bh->state would always guarantee a full > barrier (at least on x86, per Peter Z), I'm wondering if we should make > every access to bh->state locked. Or, perhaps, they should all be > preceeded with a call to smp_mb() in order to guarantee that previous > writes to bh->state are seen by current CPU? > > I can test that out, but I'll wait for a proper patch from you as I > cannot predict all memory ordering problems that could arise from > current code. Still, f_mass_storage.c looks really odd :-s It definitely could use some clean-up attention. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: g_mass_storage not queueing requests
On Mon, 5 Sep 2016, Felipe Balbi wrote:
> okay, I'm thinking that's a plausible explanation, please correct me if
> I'm wrong. What I'm speculating now is that inside a locked region,
> reordering can happen (can it?)
Yes, it can.
> which means that our:
>
> spin_lock(&common->lock);
> bh->outreq_busy = 0;
> bh->state = BUF_STATE_FULL;
> wakeup_thread(common);
> spin_unlock(&common->lock);
>
> ends up being reordered as:
>
> spin_lock(&common->lock);
> bh->outreq_busy = 0;
> wakeup_thread(common);
> bh->state = BUF_STATE_FULL;
> spin_unlock(&common->lock);
>
> which triggers the fault. Is that possible?
That's why wakeup_thread needs to include a memory barrier -- to
prevent exactly this sort of thing from happening.
> Looking at the history.git tree [0] I found a little more of
> file_storage gadget's history. Here are some of the details I could find
> out:
>
> When you first wrote the file_storage gadget, wakeup_thread() was called
> outside the lock from bulk_{in,out}_complete() (see [1]). A follow-up
> patch ended up moving wakeup_thread() inside the locked region (while
> also removing a wait_queue()) which, I'm speculating, introduced the
> race condition (see [2])
>
> Another piece of speculation (which I can test, if necessary) is that
> larger mass storage buffers (e.g. 64k or 128k) would hide this race
> because it would take longer for UDC to complete the request. This might
> be why we never saw this before with HS-only UDCs.
>
> Are we really dealing with a regression introduced back in 2.6.15?
Quite possibly. Or maybe it was there from the very beginning, but it
doesn't show up unless you're using a fast processor.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: g_mass_storage not queueing requests
Hi,
Alan Stern writes:
> On Mon, 5 Sep 2016, Felipe Balbi wrote:
>
>> okay, I'm thinking that's a plausible explanation, please correct me if
>> I'm wrong. What I'm speculating now is that inside a locked region,
>> reordering can happen (can it?)
>
> Yes, it can.
alright, thanks
>> which means that our:
>>
>> spin_lock(&common->lock);
>> bh->outreq_busy = 0;
>> bh->state = BUF_STATE_FULL;
>> wakeup_thread(common);
>> spin_unlock(&common->lock);
>>
>> ends up being reordered as:
>>
>> spin_lock(&common->lock);
>> bh->outreq_busy = 0;
>> wakeup_thread(common);
>> bh->state = BUF_STATE_FULL;
>> spin_unlock(&common->lock);
>>
>> which triggers the fault. Is that possible?
>
> That's why wakeup_thread needs to include a memory barrier -- to
> prevent exactly this sort of thing from happening.
but is smp_wmb() enough or do we need a full barrier?
>> Looking at the history.git tree [0] I found a little more of
>> file_storage gadget's history. Here are some of the details I could find
>> out:
>>
>> When you first wrote the file_storage gadget, wakeup_thread() was called
>> outside the lock from bulk_{in,out}_complete() (see [1]). A follow-up
>> patch ended up moving wakeup_thread() inside the locked region (while
>> also removing a wait_queue()) which, I'm speculating, introduced the
>> race condition (see [2])
>>
>> Another piece of speculation (which I can test, if necessary) is that
>> larger mass storage buffers (e.g. 64k or 128k) would hide this race
>> because it would take longer for UDC to complete the request. This might
>> be why we never saw this before with HS-only UDCs.
>>
>> Are we really dealing with a regression introduced back in 2.6.15?
>
> Quite possibly. Or maybe it was there from the very beginning, but it
> doesn't show up unless you're using a fast processor.
not only a fast processor, but also a really fast USB. I couldn't
reproduce this before optimizing dwc3 (we're getting really good mass
storage throughput)
--
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory barrier needed with wake_up_process()?
On Mon, 5 Sep 2016, Peter Zijlstra wrote: > > Actually, seeing it written out like this, one realizes that it really > > ought to be: > > > > CPU 0 CPU 1 > > - - > > Start DMA Handle DMA-complete irq > > Sleep until bh->state smp_mb() > > set bh->state > > Wake up CPU 0 > > smp_mb() > > Compute rc based on contents of the DMA buffer > > > > (Bear in mind also that on some platforms, the I/O operation is carried > > out by PIO rather than DMA.) > > I'm sorry, but I still don't follow. This could be because I seldom > interact with DMA agents and therefore am not familiar with that stuff. I haven't seen the details of memory ordering requirements in the presence of DMA spelled out anywhere. The documents I have read merely state that you have to careful to flush caches before doing DMA OUT and to avoid filling caches before DMA IN is complete. Neither of those is an issue here, apparently. > Also, smp_mb() doesn't necessarily interact with MMIO/DMA at all IIRC. > Its only defined to do CPU/CPU interactions. Suppose the DMA master finishes filling up an input buffer and issues a completion irq to CPU1. Presumably the data would then be visible to CPU1 if the interrupt handler looked at it. So if CPU1 executes smp_mb before setting bh->state and waking up CPU0, and if CPU0 executes smp_mb after testing bh->state and before reading the data buffer, wouldn't CPU0 then see the correct data in the buffer? Even if CPU0 never did go to sleep? Or would something more be needed? > I would very much expect the device IO stuff to order things for us in > this case. "Start DMA" should very much include sufficient fences to > ensure the data under DMA is visible to the DMA engine, this would very > much include things like flushing store buffers and maybe even writeback > caches, depending on platform needs. > > At the same time, I would expect "Handle DMA-complete irq", even if it > were done with a PIO polling loop, to guarantee ordering against later > operations such that 'complete' really means that. That's what I would expect too. Back in the original email thread where the problem was first reported, Felipe says that the problem appears to be something else. Here's what it looks like now, in schematic form: CPU0 get_next_command(): while (bh->state != BUF_STATE_EMPTY) sleep_thread(); start an input request for bh while (bh->state != BUF_STATE_FULL) sleep_thread(); As mentioned above, the input involves DMA and is terminated by an irq. The request's completion handler is bulk_out_complete(): CPU1 bulk_out_complete(): bh->state = BUF_STATE_FULL; wakeup_thread(); According to Felipe, when CPU0 wakes up and checks bh->state, it sees a value different from BUF_STATE_FULL. So it goes back to sleep again and doesn't make any forward progress. It's possible that something else is changing bh->state when it shouldn't. But if this were the explanation, why would Felipe see that the problem goes away when he changes the memory barriers in sleep_thread() and wakeup_thread() to smp_mb()? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: g_mass_storage not queueing requests
On Mon, 5 Sep 2016, Felipe Balbi wrote:
> Hi,
>
> Alan Stern writes:
> > On Mon, 5 Sep 2016, Felipe Balbi wrote:
> >
> >> okay, I'm thinking that's a plausible explanation, please correct me if
> >> I'm wrong. What I'm speculating now is that inside a locked region,
> >> reordering can happen (can it?)
> >
> > Yes, it can.
>
> alright, thanks
>
> >> which means that our:
> >>
> >>spin_lock(&common->lock);
> >>bh->outreq_busy = 0;
> >>bh->state = BUF_STATE_FULL;
> >>wakeup_thread(common);
> >>spin_unlock(&common->lock);
> >>
> >> ends up being reordered as:
> >>
> >>spin_lock(&common->lock);
> >>bh->outreq_busy = 0;
> >>wakeup_thread(common);
> >>bh->state = BUF_STATE_FULL;
> >>spin_unlock(&common->lock);
> >>
> >> which triggers the fault. Is that possible?
> >
> > That's why wakeup_thread needs to include a memory barrier -- to
> > prevent exactly this sort of thing from happening.
>
> but is smp_wmb() enough or do we need a full barrier?
That's more or less the question that started the other email thread.
According to Peter, the barriers already present in wake_up_process()
and try_to_wake_up() should be sufficient to prevent this reordering.
Besides, x86 is more strongly ordered than architectures like PPC or
ARM. It rarely needs extra barriers (mostly to prevent a later read
from moving up before an earlier write).
> >> Looking at the history.git tree [0] I found a little more of
> >> file_storage gadget's history. Here are some of the details I could find
> >> out:
> >>
> >> When you first wrote the file_storage gadget, wakeup_thread() was called
> >> outside the lock from bulk_{in,out}_complete() (see [1]). A follow-up
> >> patch ended up moving wakeup_thread() inside the locked region (while
> >> also removing a wait_queue()) which, I'm speculating, introduced the
> >> race condition (see [2])
> >>
> >> Another piece of speculation (which I can test, if necessary) is that
> >> larger mass storage buffers (e.g. 64k or 128k) would hide this race
> >> because it would take longer for UDC to complete the request. This might
> >> be why we never saw this before with HS-only UDCs.
> >>
> >> Are we really dealing with a regression introduced back in 2.6.15?
> >
> > Quite possibly. Or maybe it was there from the very beginning, but it
> > doesn't show up unless you're using a fast processor.
>
> not only a fast processor, but also a really fast USB. I couldn't
> reproduce this before optimizing dwc3 (we're getting really good mass
> storage throughput)
Indeed.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: gadget: prevent potenial null pointer dereference on skb->len
From: Colin Ian King
An earlier fix partially fixed the null pointer dereference on skb->len
by moving the assignment of len after the check on skb being non-null,
however it failed to remove the erroneous dereference when assigning len.
Correctly fix this by removing the initialisation of len as was
originally intended.
Fixes: 70237dc8efd092 ("usb: gadget: function: f_eem: socket buffer may be
NULL")
Signed-off-by: Colin Ian King
---
drivers/usb/gadget/function/f_eem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_eem.c
b/drivers/usb/gadget/function/f_eem.c
index 8741fd7..007ec6e 100644
--- a/drivers/usb/gadget/function/f_eem.c
+++ b/drivers/usb/gadget/function/f_eem.c
@@ -342,7 +342,7 @@ static struct sk_buff *eem_wrap(struct gether *port, struct
sk_buff *skb)
struct sk_buff *skb2 = NULL;
struct usb_ep *in = port->in_ep;
int headroom, tailroom, padlen = 0;
- u16 len = skb->len;
+ u16 len;
if (!skb)
return NULL;
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Friday, September 2, 2016 5:16:31 PM CEST Leo Li wrote: > > Can we use the firmware or bootloader information to provide the > default dma-mapping attributes for devices that doesn't have an > of_node pointer or ACPI data? This will at least restore what we had > previously provided . I'm concerned that changing all the drivers > that are creating child device will be a big effort. Like I mentioned > in another thread, there are many instances of platform_device_add() > under the drivers/ directory. Fortunately, there are not too many drivers that call platform_device_add *and* try to set up a dma mask for the child device: git grep -wl dma_mask drivers | xargs grep -wl 'platform_device_\(add\|register\)' drivers/base/platform.c drivers/bcma/main.c drivers/eisa/virtual_root.c drivers/mfd/mfd-core.c drivers/mfd/omap-usb-host.c drivers/misc/mic/card/mic_x100.c drivers/platform/goldfish/pdev_bus.c drivers/ssb/main.c drivers/usb/chipidea/core.c drivers/usb/dwc3/dwc3-exynos.c drivers/usb/dwc3/host.c drivers/usb/gadget/udc/bdc/bdc_pci.c drivers/usb/host/bcma-hcd.c drivers/usb/host/fsl-mph-dr-of.c drivers/usb/host/ssb-hcd.c drivers/usb/misc/ftdi-elan.c drivers/usb/musb/blackfin.c drivers/usb/musb/musb_dsps.c drivers/usb/musb/omap2430.c drivers/usb/musb/ux500.c Most of these are probably never used with any nonstandard DMA settings (IOMMU, cache coherency, offset, ...). One thing we could possibly do is to go through these and replace the hardcoded dma mask setup with of_dma_configure() in all cases in which we actually use DT for probing, which should cover the interesting cases. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: xHCI problem? [was Re: Erratic USB device behavior and device loss]
On Mon, 5 Sep 2016, Ritesh Raj Sarraf wrote: > -BEGIN PGP SIGNED MESSAGE- > Hash: SHA512 > > On Sun, 2016-09-04 at 15:46 -0400, Alan Stern wrote: > > > > This is not the problem I was discussing with Ulf. The problem was why > > the device kept going into and out of runtime suspend every three > > seconds. The kernel log above does not say whether this was happening. > > One way to tell is to look at a usbmon trace (like we did before). > > https://people.debian.org/~rrs/tmp/usbmon.txt.gz > > This should have the logs you asked for, running on 4.8-rc4. Confirmed, the runtime suspends and resumes are still happening. Ulf, any insights? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: chipidea: udc: kernel panic in isr_setup_status_phase
On Mon, Sep 05, 2016 at 11:10:22AM +0800, Peter Chen wrote:
> How about below, it will set halt for device, and host will get stall
> from the device.
>
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index 0f692fc..3c46ccb 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -946,6 +946,11 @@ static int isr_setup_status_phase(struct ci_hdrc *ci)
> int retval;
> struct ci_hw_ep *hwep;
>
> + if (!ci->status) {
> + WARN_ON(1);
> + return -EPIPE;
> + }
> +
> hwep = (ci->ep0_dir == TX) ? ci->ep0out : ci->ep0in;
> ci->status->context = ci;
> ci->status->complete = isr_setup_status_complete;
>
Returning -EPIPE works! I would however suggest to only warn once, as
this otherwise floods the kernel log.
I'll send a patch shortly, also adding a comment for people experiencing
a similar hardware problem.
Thank you very much for your help, Peter!
Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: chipidea: udc: fix NULL ptr dereference in isr_setup_status_phase
Problems with the signal integrity of the high speed USB data lines or noise on reference ground lines can cause the i.MX6 USB controller to violate USB specs and exhibit unexpected behavior. It was observed that USBi_UI interrupts were triggered first and when isr_setup_status_phase was called, ci->status was NULL, which lead to a NULL pointer dereference kernel panic. This patch fixes the kernel panic, emits a warning once and returns -EPIPE to halt the device and let the host get stalled. It also adds a comment to point people, who are experiencing this issue, to their USB hardware design. Signed-off-by: Clemens Gruber --- drivers/usb/chipidea/udc.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index dfec5a1..b933568 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -949,6 +949,15 @@ static int isr_setup_status_phase(struct ci_hdrc *ci) int retval; struct ci_hw_ep *hwep; + /* +* Unexpected USB controller behavior, caused by bad signal integrity +* or ground reference problems, can lead to isr_setup_status_phase +* being called with ci->status equal to NULL. +* If this situation occurs, you should review your USB hardware design. +*/ + if (WARN_ON_ONCE(!ci->status)) + return -EPIPE; + hwep = (ci->ep0_dir == TX) ? ci->ep0out : ci->ep0in; ci->status->context = ci; ci->status->complete = isr_setup_status_complete; -- 2.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Bug] System reboots instead of shutting down if xhci is enabled in BIOS and USB hub is connected
System reboots instead of shutting down if xhci is enabled in BIOS and USB hub is connected When I connect the usb hub built into my monitor (http://www.samsung.com/uk/support/model/LS27D85KTSN/XU), the system will reboot instead of shutting down when I do a shut down. If I disable XCHI in the BIOS, the problem does not happen. If I disconnect the hub and enable XHCI, the problem does not occur. Possibly related bugs: https://bugzilla.kernel.org/show_bug.cgi?id=76291 https://bugzilla.kernel.org/show_bug.cgi?id=66171 dmi.bios.date: 06/06/2016 dmi.bios.vendor: Intel Corporation dmi.bios.version: RYBDWi35.86A.0358.2016.0606.1423 dmi.board.name: NUC5i5RYB dmi.board.vendor: Intel Corporation dmi.board.version: H40999-503 dmi.chassis.type: 3 dmi.modalias: dmi:bvnIntelCorporation:bvrRYBDWi35.86A.0358.2016.0606.1423:bd06/06/2016:svn:pn:pvr:rvnIntelCorporation:rnNUC5i5RYB:rvrH40999-503:cvn:ct3:cvr: $ cat /proc/version Linux version 4.8.0-040800rc5-lowlatency (kernel@tangerine) (gcc version 6.2.0 20160830 (Ubuntu 6.2.0-2ubuntu11) ) #201609041832 SMP PREEMPT Sun Sep 4 22:38:41 UTC 2016 launchpad bug: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1606030 Thanks, Hasan. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 2/3] usb: musb: da8xx: Use devm in probe
Simplify things a bit by using devm functions where possible.
Signed-off-by: David Lechner
---
drivers/usb/musb/da8xx.c | 19 +--
1 file changed, 5 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index b03d3b8..0c1997c 100644
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c
@@ -490,20 +490,18 @@ static int da8xx_probe(struct platform_device *pdev)
struct da8xx_glue *glue;
struct platform_device_info pinfo;
struct clk *clk;
+ int ret;
- int ret = -ENOMEM;
-
- glue = kzalloc(sizeof(*glue), GFP_KERNEL);
+ glue = devm_kzalloc(&pdev->dev, sizeof(*glue), GFP_KERNEL);
if (!glue) {
dev_err(&pdev->dev, "failed to allocate glue context\n");
- goto err0;
+ return -ENOMEM;
}
- clk = clk_get(&pdev->dev, "usb20");
+ clk = devm_clk_get(&pdev->dev, "usb20");
if (IS_ERR(clk)) {
dev_err(&pdev->dev, "failed to get clock\n");
- ret = PTR_ERR(clk);
- goto err3;
+ return PTR_ERR(clk);
}
ret = clk_enable(clk);
@@ -560,12 +558,7 @@ err5:
clk_disable(clk);
err4:
- clk_put(clk);
-
-err3:
- kfree(glue);
-err0:
return ret;
}
@@ -576,8 +569,6 @@ static int da8xx_remove(struct platform_device *pdev)
platform_device_unregister(glue->musb);
usb_phy_generic_unregister(glue->phy);
clk_disable(glue->clk);
- clk_put(glue->clk);
- kfree(glue);
return 0;
}
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 3/3] usb: musb: da8xx: Remove mach code
Use the new phy-da8xx-usb driver to take the place of the mach code that
pokes CFGCHIP2 in the da8xx musb glue driver. This unbreaks the driver.
Signed-off-by: David Lechner
---
drivers/usb/musb/Kconfig | 2 +-
drivers/usb/musb/da8xx.c | 135 ++-
2 files changed, 51 insertions(+), 86 deletions(-)
diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
index 886526b..c73221a 100644
--- a/drivers/usb/musb/Kconfig
+++ b/drivers/usb/musb/Kconfig
@@ -82,7 +82,7 @@ config USB_MUSB_DA8XX
tristate "DA8xx/OMAP-L1x"
depends on ARCH_DAVINCI_DA8XX
depends on NOP_USB_XCEIV
- depends on BROKEN
+ select PHY_DA8XX_USB
config USB_MUSB_TUSB6010
tristate "TUSB6010"
diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index 0c1997c..6af8c3d 100644
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c
@@ -30,13 +30,11 @@
#include
#include
#include
+#include
#include
#include
#include
-#include
-#include
-
#include "musb_core.h"
/*
@@ -80,61 +78,15 @@
#define DA8XX_MENTOR_CORE_OFFSET 0x400
-#define CFGCHIP2 IO_ADDRESS(DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP2_REG)
-
struct da8xx_glue {
struct device *dev;
struct platform_device *musb;
- struct platform_device *phy;
+ struct platform_device *usb_phy;
struct clk *clk;
+ struct phy *phy;
};
/*
- * REVISIT (PM): we should be able to keep the PHY in low power mode most
- * of the time (24 MHz oscillator and PLL off, etc.) by setting POWER.D0
- * and, when in host mode, autosuspending idle root ports... PHY_PLLON
- * (overriding SUSPENDM?) then likely needs to stay off.
- */
-
-static inline void phy_on(void)
-{
- u32 cfgchip2 = __raw_readl(CFGCHIP2);
-
- /*
-* Start the on-chip PHY and its PLL.
-*/
- cfgchip2 &= ~(CFGCHIP2_RESET | CFGCHIP2_PHYPWRDN | CFGCHIP2_OTGPWRDN);
- cfgchip2 |= CFGCHIP2_PHY_PLLON;
- __raw_writel(cfgchip2, CFGCHIP2);
-
- pr_info("Waiting for USB PHY clock good...\n");
- while (!(__raw_readl(CFGCHIP2) & CFGCHIP2_PHYCLKGD))
- cpu_relax();
-}
-
-static inline void phy_off(void)
-{
- u32 cfgchip2 = __raw_readl(CFGCHIP2);
-
- /*
-* Ensure that USB 1.1 reference clock is not being sourced from
-* USB 2.0 PHY. Otherwise do not power down the PHY.
-*/
- if (!(cfgchip2 & CFGCHIP2_USB1PHYCLKMUX) &&
-(cfgchip2 & CFGCHIP2_USB1SUSPENDM)) {
- pr_warning("USB 1.1 clocked from USB 2.0 PHY -- "
- "can't power it down\n");
- return;
- }
-
- /*
-* Power down the on-chip PHY.
-*/
- cfgchip2 |= CFGCHIP2_PHYPWRDN | CFGCHIP2_OTGPWRDN;
- __raw_writel(cfgchip2, CFGCHIP2);
-}
-
-/*
* Because we don't set CTRL.UINT, it's "important" to:
* - not read/write INTRUSB/INTRUSBE (except during
* initial setup, as a workaround);
@@ -385,29 +337,29 @@ static irqreturn_t da8xx_musb_interrupt(int irq, void
*hci)
static int da8xx_musb_set_mode(struct musb *musb, u8 musb_mode)
{
- u32 cfgchip2 = __raw_readl(CFGCHIP2);
+ struct da8xx_glue *glue = dev_get_drvdata(musb->controller->parent);
+ enum phy_mode phy_mode;
- cfgchip2 &= ~CFGCHIP2_OTGMODE;
switch (musb_mode) {
case MUSB_HOST: /* Force VBUS valid, ID = 0 */
- cfgchip2 |= CFGCHIP2_FORCE_HOST;
+ phy_mode = PHY_MODE_USB_HOST;
break;
case MUSB_PERIPHERAL: /* Force VBUS valid, ID = 1 */
- cfgchip2 |= CFGCHIP2_FORCE_DEVICE;
+ phy_mode = PHY_MODE_USB_DEVICE;
break;
case MUSB_OTG: /* Don't override the VBUS/ID comparators */
- cfgchip2 |= CFGCHIP2_NO_OVERRIDE;
+ phy_mode = PHY_MODE_USB_OTG;
break;
default:
- dev_dbg(musb->controller, "Trying to set unsupported mode
%u\n", musb_mode);
+ return -EINVAL;
}
- __raw_writel(cfgchip2, CFGCHIP2);
- return 0;
+ return phy_set_mode(glue->phy, phy_mode);
}
static int da8xx_musb_init(struct musb *musb)
{
+ struct da8xx_glue *glue = dev_get_drvdata(musb->controller->parent);
void __iomem *reg_base = musb->ctrl_base;
u32 rev;
int ret = -ENODEV;
@@ -425,32 +377,56 @@ static int da8xx_musb_init(struct musb *musb)
goto fail;
}
+ ret = clk_prepare_enable(glue->clk);
+ if (ret) {
+ dev_err(glue->dev, "failed to enable clock\n");
+ goto fail;
+ }
+
setup_timer(&otg_workaround, otg_timer, (unsigned long)musb);
/* Reset the controller */
musb_writel(reg_base, DA8XX_USB_CTRL_REG, DA8XX_SOFT_RESET_MASK);
/* Start the on-chip PHY and its PLL. */
-
[PATCH v6 0/3] da8xx USB PHY platform devices and clocks
Just resending to get these merged into usb. The phy parts of this patch series have already been merged into Linus' tree. I have rebased on 4.8-rc5 but there have not been any changes to these since the last time I submitted. David Lechner (3): usb: ohci-da8xx: Remove code that references mach usb: musb: da8xx: Use devm in probe usb: musb: da8xx: Remove mach code drivers/usb/host/Kconfig | 1 + drivers/usb/host/ohci-da8xx.c | 102 +++- drivers/usb/musb/Kconfig | 2 +- drivers/usb/musb/da8xx.c | 154 +++--- 4 files changed, 112 insertions(+), 147 deletions(-) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 1/3] usb: ohci-da8xx: Remove code that references mach
Including mach/* is frowned upon in device drivers, so get rid of it.
This replaces usb20_clk and code that pokes CFGCHIP2 with a proper phy
driver.
Signed-off-by: David Lechner
Acked-by: Alan Stern
---
drivers/usb/host/Kconfig | 1 +
drivers/usb/host/ohci-da8xx.c | 102 +++---
2 files changed, 56 insertions(+), 47 deletions(-)
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 2e710a4..1f0cdab8 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -482,6 +482,7 @@ config USB_OHCI_HCD_DAVINCI
bool "OHCI support for TI DaVinci DA8xx"
depends on ARCH_DAVINCI_DA8XX
depends on USB_OHCI_HCD=y
+ select PHY_DA8XX_USB
default y
help
Enables support for the DaVinci DA8xx integrated OHCI
diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
index e5c33bc..3656d7c 100644
--- a/drivers/usb/host/ohci-da8xx.c
+++ b/drivers/usb/host/ohci-da8xx.c
@@ -15,58 +15,50 @@
#include
#include
#include
-
-#include
+#include
#include
#ifndef CONFIG_ARCH_DAVINCI_DA8XX
#error "This file is DA8xx bus glue. Define CONFIG_ARCH_DAVINCI_DA8XX."
#endif
-#define CFGCHIP2 DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG)
-
static struct clk *usb11_clk;
-static struct clk *usb20_clk;
+static struct phy *usb11_phy;
/* Over-current indicator change bitmask */
static volatile u16 ocic_mask;
-static void ohci_da8xx_clock(int on)
+static int ohci_da8xx_enable(void)
{
- u32 cfgchip2;
-
- cfgchip2 = __raw_readl(CFGCHIP2);
- if (on) {
- clk_enable(usb11_clk);
-
- /*
-* If USB 1.1 reference clock is sourced from USB 2.0 PHY, we
-* need to enable the USB 2.0 module clocking, start its PHY,
-* and not allow it to stop the clock during USB 2.0 suspend.
-*/
- if (!(cfgchip2 & CFGCHIP2_USB1PHYCLKMUX)) {
- clk_enable(usb20_clk);
-
- cfgchip2 &= ~(CFGCHIP2_RESET | CFGCHIP2_PHYPWRDN);
- cfgchip2 |= CFGCHIP2_PHY_PLLON;
- __raw_writel(cfgchip2, CFGCHIP2);
-
- pr_info("Waiting for USB PHY clock good...\n");
- while (!(__raw_readl(CFGCHIP2) & CFGCHIP2_PHYCLKGD))
- cpu_relax();
- }
+ int ret;
- /* Enable USB 1.1 PHY */
- cfgchip2 |= CFGCHIP2_USB1SUSPENDM;
- } else {
- clk_disable(usb11_clk);
- if (!(cfgchip2 & CFGCHIP2_USB1PHYCLKMUX))
- clk_disable(usb20_clk);
+ ret = clk_prepare_enable(usb11_clk);
+ if (ret)
+ return ret;
- /* Disable USB 1.1 PHY */
- cfgchip2 &= ~CFGCHIP2_USB1SUSPENDM;
- }
- __raw_writel(cfgchip2, CFGCHIP2);
+ ret = phy_init(usb11_phy);
+ if (ret)
+ goto err_phy_init;
+
+ ret = phy_power_on(usb11_phy);
+ if (ret)
+ goto err_phy_power_on;
+
+ return 0;
+
+err_phy_power_on:
+ phy_exit(usb11_phy);
+err_phy_init:
+ clk_disable_unprepare(usb11_clk);
+
+ return ret;
+}
+
+static void ohci_da8xx_disable(void)
+{
+ phy_power_off(usb11_phy);
+ phy_exit(usb11_phy);
+ clk_disable_unprepare(usb11_clk);
}
/*
@@ -92,7 +84,9 @@ static int ohci_da8xx_init(struct usb_hcd *hcd)
dev_dbg(dev, "starting USB controller\n");
- ohci_da8xx_clock(1);
+ result = ohci_da8xx_enable();
+ if (result < 0)
+ return result;
/*
* DA8xx only have 1 port connected to the pins but the HC root hub
@@ -101,8 +95,10 @@ static int ohci_da8xx_init(struct usb_hcd *hcd)
ohci->num_ports = 1;
result = ohci_init(ohci);
- if (result < 0)
+ if (result < 0) {
+ ohci_da8xx_disable();
return result;
+ }
/*
* Since we're providing a board-specific root hub port power control
@@ -129,7 +125,7 @@ static int ohci_da8xx_init(struct usb_hcd *hcd)
static void ohci_da8xx_stop(struct usb_hcd *hcd)
{
ohci_stop(hcd);
- ohci_da8xx_clock(0);
+ ohci_da8xx_disable();
}
static int ohci_da8xx_start(struct usb_hcd *hcd)
@@ -301,12 +297,18 @@ static int usb_hcd_da8xx_probe(const struct hc_driver
*driver,
return -ENODEV;
usb11_clk = devm_clk_get(&pdev->dev, "usb11");
- if (IS_ERR(usb11_clk))
+ if (IS_ERR(usb11_clk)) {
+ if (PTR_ERR(usb11_clk) != -EPROBE_DEFER)
+ dev_err(&pdev->dev, "Failed to get clock.\n");
return PTR_ERR(usb11_clk);
+ }
- usb20_clk = devm_clk_get(&pdev->dev, "usb20");
- if (IS_ERR(usb20_clk))
- return PTR_ERR(usb20_clk);
+ usb11_phy = devm_phy_get(&pdev->dev, "usb-phy");
Great Offer
You are a recipient to Mrs Julie Leach Donation of $2 million USD. Contact (julieleach...@hotmail.com) for claims. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Great Offer
You are a recipient to Mrs Julie Leach Donation of $2 million USD. Contact (julieleach...@hotmail.com) for claims. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Great Offer
You are a recipient to Mrs Julie Leach Donation of $2 million USD. Contact (julieleach...@hotmail.com) for claims. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 3/4] usb: dwc2: assert phy reset when waking up in rk3288 platform
從我的 iPad 傳送 > David Laight 於 2016年9月5日 下午9:35 寫道： > > From: Randy Li >> Sent: 03 September 2016 22:55 > ... >> +if (of_device_is_compatible(np, "rockchip,rk3288-usb") >> +&& (NULL != hsotg->phy->ops->reset)) >> +hsotg->phy->ops->reset(hsotg->phy); >> + > > Is this the only place ops->reset() is called? Yes. > Probably much better to check it first. Sure. > >if (hsotg->phy->ops->reset >&& of_device_is_compatible(np, "rockchip,rk3288-usb") >hsotg->phy->ops->reset(hsotg->phy); I see, if there is no the other review request for this version, I would submit a new version to fix this problem tonight. > >David -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: chipidea: udc: fix NULL ptr dereference in isr_setup_status_phase
On Mon, Sep 05, 2016 at 07:29:58PM +0200, Clemens Gruber wrote: > Problems with the signal integrity of the high speed USB data lines or > noise on reference ground lines can cause the i.MX6 USB controller to > violate USB specs and exhibit unexpected behavior. > > It was observed that USBi_UI interrupts were triggered first and when > isr_setup_status_phase was called, ci->status was NULL, which lead to a > NULL pointer dereference kernel panic. > > This patch fixes the kernel panic, emits a warning once and returns > -EPIPE to halt the device and let the host get stalled. > It also adds a comment to point people, who are experiencing this issue, > to their USB hardware design. > > Signed-off-by: Clemens Gruber > --- > drivers/usb/chipidea/udc.c | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index dfec5a1..b933568 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -949,6 +949,15 @@ static int isr_setup_status_phase(struct ci_hdrc *ci) > int retval; > struct ci_hw_ep *hwep; > > + /* > + * Unexpected USB controller behavior, caused by bad signal integrity > + * or ground reference problems, can lead to isr_setup_status_phase > + * being called with ci->status equal to NULL. > + * If this situation occurs, you should review your USB hardware design. > + */ > + if (WARN_ON_ONCE(!ci->status)) > + return -EPIPE; > + > hwep = (ci->ep0_dir == TX) ? ci->ep0out : ci->ep0in; > ci->status->context = ci; > ci->status->complete = isr_setup_status_complete; Thanks, I will apply it. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] usb: gadget: prevent potenial null pointer dereference on skb->len
>
>From: Colin Ian King
>
>An earlier fix partially fixed the null pointer dereference on skb->len by
>moving the
>assignment of len after the check on skb being non-null, however it failed to
>remove
>the erroneous dereference when assigning len.
>Correctly fix this by removing the initialisation of len as was originally
>intended.
>
>Fixes: 70237dc8efd092 ("usb: gadget: function: f_eem: socket buffer may be
>NULL")
>Signed-off-by: Colin Ian King
>---
> drivers/usb/gadget/function/f_eem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/usb/gadget/function/f_eem.c
>b/drivers/usb/gadget/function/f_eem.c
>index 8741fd7..007ec6e 100644
>--- a/drivers/usb/gadget/function/f_eem.c
>+++ b/drivers/usb/gadget/function/f_eem.c
>@@ -342,7 +342,7 @@ static struct sk_buff *eem_wrap(struct gether *port, struct
>sk_buff *skb)
> struct sk_buff *skb2 = NULL;
> struct usb_ep *in = port->in_ep;
> int headroom, tailroom, padlen = 0;
>- u16 len = skb->len;
>+ u16 len;
>
> if (!skb)
> return NULL;
Sorry, my careless, Thanks for fixing it.
Acked-by: Peter Chen
Peter
Re: [PATCH v7 3/4] usb: dwc2: assert phy reset when waking up in rk3288 platform
Hi,
On Sunday 04 September 2016 03:25 AM, Randy Li wrote:
> On the rk3288 USB host-only port (the one that's not the OTG-enabled
> port) the PHY can get into a bad state when a wakeup is asserted (not
> just a wakeup from full system suspend but also a wakeup from
> autosuspend).
>
> We can get the PHY out of its bad state by asserting its "port reset",
> but unfortunately that seems to assert a reset onto the USB bus so it
> could confuse things if we don't actually deenumerate / reenumerate the
> device.
>
> We can also get the PHY out of its bad state by fully resetting it using
> the reset from the CRU (clock reset unit) in chip, which does a more full
> reset. The CRU-based reset appears to actually cause devices on the bus
> to be removed and reinserted, which fixes the problem (albeit in a hacky
> way).
>
> It's unfortunate that we need to do a full re-enumeration of devices at
> wakeup time, but this is better than alternative of letting the bus get
> wedged.
>
> Signed-off-by: Randy Li
> ---
> drivers/usb/dwc2/core_intr.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index d85c5c9..08485b7 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct
> dwc2_hsotg *hsotg)
> static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
> {
> int ret;
> + struct device_node *np = hsotg->dev->of_node;
>
> /* Clear interrupt */
> dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
> @@ -379,6 +380,17 @@ static void dwc2_handle_wakeup_detected_intr(struct
> dwc2_hsotg *hsotg)
> /* Restart the Phy Clock */
> pcgcctl &= ~PCGCTL_STOPPCLK;
> dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
> +
> + /*
> + * It is a quirk in Rockchip RK3288, causing by
> + * a hardware bug. This will propagate out and
> + * eventually we'll re-enumerate the device.
> + * Not great but the best we can do
> + */
> + if (of_device_is_compatible(np, "rockchip,rk3288-usb")
> + && (NULL != hsotg->phy->ops->reset))
> + hsotg->phy->ops->reset(hsotg->phy);
never call the phy ops directly from the controller driver. It has to be
protected as well.
Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
On Mon, Aug 29 2016, Baolin Wang wrote: > Hi Felipe, > > On 11 August 2016 at 11:14, Baolin Wang wrote: >> Hi Felipe, >> >> On 1 August 2016 at 15:09, Baolin Wang wrote: >>> Currently the Linux kernel does not provide any standard integration of this >>> feature that integrates the USB subsystem with the system power regulation >>> provided by PMICs meaning that either vendors must add this in their kernels >>> or USB gadget devices based on Linux (such as mobile phones) may not behave >>> as they should. Thus provide a standard framework for doing this in kernel. >>> >>> Now introduce one user with wm831x_power to support and test the usb >>> charger, >>> which is pending testing. Moreover there may be other potential users will >>> use >>> it in future. >> >> Could you please apply this patchset into your 'next' branch if you >> have no comments about it? Thank you. > > Since there are no other comments about this patchset for a long time, > could you please apply this patchset? Thanks. Sorry, I should have replied earlier. Tim Bird mentioned on the ksummit-discuss list that there was a frustration with this not making progress so I decided to contribute what I could now. I think this patch set is attempting to address an important problem that needs solving. However I think it gets some key aspects wrong. Maybe they can get fixed up after the patchset is upstream, maybe they should be fixed first - I have no strong opinion on that. My main complaints involve the detection and handling of the different charger types - DCP, CDP, ACA etc. The big-picture requirement here that the PHY will detect the physical properties of the cable (e.g. resistance to ground on ID) and determine the type of charger expected. This information must be communicated to the PMIC "power_supply" device so it can regulate the power being drawn through the cable. The first problem is that there are two different ways that the distinction between DCP, CDP, ACA etc can be represented in Linux. They are cable types in the 'extcon' subsystem, and they are power_supply types in the 'power_supply' subsystem. This duplication is confusing. It is not caused by your patch set, but I believe your patchset needs to work with the duplication and I think it does so poorly. In my mind, the power_supply should *not* know about this distinction at all (except possibly as an advisor attribute simiarly to the current battery technology attribute). The other types it knows of are "AC", "USB", and "BATTERY". The contrast between these is quite different From the contrast between DCP, CDP, ACA, which, from the perspective of the power supply, are almost irrelevant. Your patchset effectively examines the power_supply_type of one power_supply, and communicates it to another. It isn't clear to me how the first power_supply gets the information, or what the relationship between the two power_supplies is meant to be. It makes much more sense, to me, to utilized the knowledge of this distinction that extcon provides. A usb PHY can register an extcon, declare the sorts of cables that it can detect, and tell the extcon as cables appear or disappear. The PMIC power_supply can then register with that extcon for events and can find out when a cable is attached, and what sort of cable. Your usb-charging framework would be well placed to help the power_supply to find the correct extcon, and possibly even to handle the registration for events. Your framework does currently register with extcon, but only listens for EXTCON_USB cables. I don't think that cable type is (reliably) reported when a DCP (for example) is plugged in. Here there is another problem that is not of your making, but still needs fixing. Extcon declares a number of cable types like: /* USB external connector */ #define EXTCON_USB 1 #define EXTCON_USB_HOST 2 /* Charging external connector */ #define EXTCON_CHG_USB_SDP 5 /* Standard Downstream Port */ #define EXTCON_CHG_USB_DCP 6 /* Dedicated Charging Port */ #define EXTCON_CHG_USB_CDP 7 /* Charging Downstream Port */ #define EXTCON_CHG_USB_ACA 8 /* Accessory Charger Adapter */ #define EXTCON_CHG_USB_FAST 9 #define EXTCON_CHG_USB_SLOW 10 However it doesn't define what those mean, so we are left to guess. They each correspond to bits in a bitmap, so a cable can have multiple types. I think the best interpretation is that: EXTCON_USB means that the cable carries USB data from a host. EXTCON_USB_HOST means that that cable carries USB data to a host. EXTCON_CHG_* means that power is available as described in the standards. (what EXTCON_CHG_USB_FAST and EXTCON_CHG_USB_SLOW mean is not at all clear). There is some support for this in the code, but it is not universally acknowledged. For a USB charging framework to be genuinely useful, it must (I think) make sure this issue gets clarified, and the various cable types used properly. Once the cable/
Re: [PATCH v6 1/8] binding-doc: power: pwrseq-generic: add binding doc for generic power sequence library
On Friday 02 September 2016 06:30 AM, Peter Chen wrote:
On Thu, Sep 01, 2016 at 01:33:22PM +0530, Vaibhav Hiremath wrote:
On Monday 15 August 2016 02:43 PM, Peter Chen wrote:
Add binding doc for generic power sequence library.
Signed-off-by: Peter Chen
Acked-by: Philipp Zabel
Acked-by: Rob Herring
---
.../bindings/power/pwrseq/pwrseq-generic.txt | 48 ++
1 file changed, 48 insertions(+)
create mode 100644
Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
new file mode 100644
index 000..ebf0d47
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
@@ -0,0 +1,48 @@
+The generic power sequence library
+
+Some hard-wired devices (eg USB/MMC) need to do power sequence before
+the device can be enumerated on the bus, the typical power sequence
+like: enable USB PHY clock, toggle reset pin, etc. But current
+Linux device driver lacks of such code to do it, it may cause some
+hard-wired devices works abnormal or can't be recognized by
+controller at all. The power sequence will be done before this device
+can be found at the bus.
+
+The power sequence properties is under the device node.
+
+Optional properties:
+- clocks: the input clocks for device.
+- reset-gpios: Should specify the GPIO for reset.
+- reset-duration-us: the duration in microsecond for assert reset signal.
+
+Below is the example of USB power sequence properties on USB device
+nodes which have two level USB hubs.
+
+&usbotg1 {
+ vbus-supply = <®_usb_otg1_vbus>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usb_otg1_id>;
+ status = "okay";
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+ genesys: hub@1 {
+ compatible = "usb5e3,608";
+ reg = <1>;
+
+ clocks = <&clks IMX6SX_CLK_CKO>;
+ reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>; /* hub reset pin */
+ reset-duration-us = <10>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+ asix: ethernet@1 {
+ compatible = "usbb95,1708";
So I assume, with our recent discussion and the change
we are proposing, the library would have some knowledge
about this compatible string, right?
Yes
what I was asking on other email was, how are you connecting
multiple power sequence libraries to their respective consumers ?
The consumers has its of_node, then it can find related power sequence
library according to compatible string.
Exactly. Thats what I was referring and wanted to confirm.
Thanks,
Vaibhav
--
Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
On Mon, Sep 05, 2016 at 05:39:27PM +0200, Arnd Bergmann wrote: > On Friday, September 2, 2016 5:16:31 PM CEST Leo Li wrote: > > > > Can we use the firmware or bootloader information to provide the > > default dma-mapping attributes for devices that doesn't have an > > of_node pointer or ACPI data? This will at least restore what we had > > previously provided . I'm concerned that changing all the drivers > > that are creating child device will be a big effort. Like I mentioned > > in another thread, there are many instances of platform_device_add() > > under the drivers/ directory. > > Fortunately, there are not too many drivers that call platform_device_add > *and* try to set up a dma mask for the child device: > > git grep -wl dma_mask drivers | xargs grep -wl > 'platform_device_\(add\|register\)' > > drivers/base/platform.c > drivers/bcma/main.c > drivers/eisa/virtual_root.c > drivers/mfd/mfd-core.c > drivers/mfd/omap-usb-host.c > drivers/misc/mic/card/mic_x100.c > drivers/platform/goldfish/pdev_bus.c > drivers/ssb/main.c > drivers/usb/chipidea/core.c > drivers/usb/dwc3/dwc3-exynos.c > drivers/usb/dwc3/host.c > drivers/usb/gadget/udc/bdc/bdc_pci.c > drivers/usb/host/bcma-hcd.c > drivers/usb/host/fsl-mph-dr-of.c > drivers/usb/host/ssb-hcd.c > drivers/usb/misc/ftdi-elan.c > drivers/usb/musb/blackfin.c > drivers/usb/musb/musb_dsps.c > drivers/usb/musb/omap2430.c > drivers/usb/musb/ux500.c > > Most of these are probably never used with any nonstandard > DMA settings (IOMMU, cache coherency, offset, ...). > > One thing we could possibly do is to go through these and > replace the hardcoded dma mask setup with of_dma_configure() > in all cases in which we actually use DT for probing, which > should cover the interesting cases. > One case I am going to work is to let USB chipidea driver support iommu, the chipidea core device is no of_node, and created by platform_add_device on the runtime. Using of_dma_configure with parent of_node is a solution from my point, like [1]. https://lkml.org/lkml/2016/2/22/7 -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev
Hi, Peter Chen writes: > On Mon, Sep 05, 2016 at 05:39:27PM +0200, Arnd Bergmann wrote: >> On Friday, September 2, 2016 5:16:31 PM CEST Leo Li wrote: >> > >> > Can we use the firmware or bootloader information to provide the >> > default dma-mapping attributes for devices that doesn't have an >> > of_node pointer or ACPI data? This will at least restore what we had >> > previously provided . I'm concerned that changing all the drivers >> > that are creating child device will be a big effort. Like I mentioned >> > in another thread, there are many instances of platform_device_add() >> > under the drivers/ directory. >> >> Fortunately, there are not too many drivers that call platform_device_add >> *and* try to set up a dma mask for the child device: >> >> git grep -wl dma_mask drivers | xargs grep -wl >> 'platform_device_\(add\|register\)' >> >> drivers/base/platform.c >> drivers/bcma/main.c >> drivers/eisa/virtual_root.c >> drivers/mfd/mfd-core.c >> drivers/mfd/omap-usb-host.c >> drivers/misc/mic/card/mic_x100.c >> drivers/platform/goldfish/pdev_bus.c >> drivers/ssb/main.c >> drivers/usb/chipidea/core.c >> drivers/usb/dwc3/dwc3-exynos.c >> drivers/usb/dwc3/host.c >> drivers/usb/gadget/udc/bdc/bdc_pci.c >> drivers/usb/host/bcma-hcd.c >> drivers/usb/host/fsl-mph-dr-of.c >> drivers/usb/host/ssb-hcd.c >> drivers/usb/misc/ftdi-elan.c >> drivers/usb/musb/blackfin.c >> drivers/usb/musb/musb_dsps.c >> drivers/usb/musb/omap2430.c >> drivers/usb/musb/ux500.c >> >> Most of these are probably never used with any nonstandard >> DMA settings (IOMMU, cache coherency, offset, ...). >> >> One thing we could possibly do is to go through these and >> replace the hardcoded dma mask setup with of_dma_configure() >> in all cases in which we actually use DT for probing, which >> should cover the interesting cases. >> > > One case I am going to work is to let USB chipidea driver support iommu, > the chipidea core device is no of_node, and created by > platform_add_device on the runtime. Using of_dma_configure with parent > of_node is a solution from my point, like [1]. > > https://lkml.org/lkml/2016/2/22/7 this only solves the problem for DT devices. Legacy devices and PCI-based systems will still suffer from the same problem. At least for dwc3, I will only be taking patches that solve the problem for all users, not a subset of them. -- balbi signature.asc Description: PGP signature
Re: [PATCH 1/3] usb: rename bU2DevExitLat to wU2DevExitLat
Hi,
Felipe Balbi writes:
> According to USB 3.1 Specification, that field is 2
> bytes wide and is named with a 'w' prefix, not 'b'.
>
> Just to make grepping in the spec easier, let's
> match the name.
>
> Signed-off-by: Felipe Balbi
I'll wait for another week or so, if nobody has any objections to this
series, I plan on sending it to Greg on my pull request.
If anybody needs a resend, just let me know ;-) (patch left below for
reference)
> ---
> drivers/usb/core/hub.c | 8
> drivers/usb/gadget/composite.c | 4 ++--
> drivers/usb/host/xhci-hub.c| 2 +-
> drivers/usb/host/xhci.c| 2 +-
> include/linux/usb/gadget.h | 2 +-
> include/uapi/linux/usb/ch9.h | 2 +-
> 6 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 1d5fc32d06d0..0f87c62a81db 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -152,7 +152,7 @@ int usb_device_supports_lpm(struct usb_device *udev)
> }
>
> if (udev->bos->ss_cap->bU1devExitLat == 0 &&
> - udev->bos->ss_cap->bU2DevExitLat == 0) {
> + udev->bos->ss_cap->wU2DevExitLat == 0) {
> if (udev->parent)
> dev_info(&udev->dev, "LPM exit latency is zeroed,
> disabling LPM.\n");
> else
> @@ -311,9 +311,9 @@ static void usb_set_lpm_parameters(struct usb_device
> *udev)
> return;
>
> udev_u1_del = udev->bos->ss_cap->bU1devExitLat;
> - udev_u2_del = le16_to_cpu(udev->bos->ss_cap->bU2DevExitLat);
> + udev_u2_del = le16_to_cpu(udev->bos->ss_cap->wU2DevExitLat);
> hub_u1_del = udev->parent->bos->ss_cap->bU1devExitLat;
> - hub_u2_del = le16_to_cpu(udev->parent->bos->ss_cap->bU2DevExitLat);
> + hub_u2_del = le16_to_cpu(udev->parent->bos->ss_cap->wU2DevExitLat);
>
> usb_set_lpm_mel(udev, &udev->u1_params, udev_u1_del,
> hub, &udev->parent->u1_params, hub_u1_del);
> @@ -3909,7 +3909,7 @@ static void usb_enable_link_state(struct usb_hcd *hcd,
> struct usb_device *udev,
> {
> int timeout, ret;
> __u8 u1_mel = udev->bos->ss_cap->bU1devExitLat;
> - __le16 u2_mel = udev->bos->ss_cap->bU2DevExitLat;
> + __le16 u2_mel = udev->bos->ss_cap->wU2DevExitLat;
>
> /* If the device says it doesn't have *any* exit latency to come out of
>* U1 or U2, it's probably lying. Assume it doesn't implement that link
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 32176f779861..7c914dabfed4 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -646,11 +646,11 @@ static int bos_desc(struct usb_composite_dev *cdev)
> cdev->gadget->ops->get_config_params(&dcd_config_params);
> else {
> dcd_config_params.bU1devExitLat = USB_DEFAULT_U1_DEV_EXIT_LAT;
> - dcd_config_params.bU2DevExitLat =
> + dcd_config_params.wU2DevExitLat =
> cpu_to_le16(USB_DEFAULT_U2_DEV_EXIT_LAT);
> }
> ss_cap->bU1devExitLat = dcd_config_params.bU1devExitLat;
> - ss_cap->bU2DevExitLat = dcd_config_params.bU2DevExitLat;
> + ss_cap->wU2DevExitLat = dcd_config_params.wU2DevExitLat;
>
> /* The SuperSpeedPlus USB Device Capability descriptor */
> if (gadget_is_superspeed_plus(cdev->gadget)) {
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 730b9fd26685..ea1a7349ef02 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -48,7 +48,7 @@ static u8 usb_bos_descriptor [] = {
> 0x03, /* bFunctionalitySupport,
> USB 3.0 speed only */
> 0x00, /* bU1DevExitLat, set later. */
> - 0x00, 0x00, /* __le16 bU2DevExitLat, set later. */
> + 0x00, 0x00, /* __le16 wU2DevExitLat, set later. */
> /* Second device capability, SuperSpeedPlus */
> 0x1c, /* bLength 28, will be adjusted later */
> USB_DT_DEVICE_CAPABILITY, /* Device Capability */
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 01d96c9b3a75..6b86d1112ef1 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4405,7 +4405,7 @@ static unsigned long long
> xhci_calculate_intel_u2_timeout(
> (xhci_service_interval_to_ns(desc) > timeout_ns))
> timeout_ns = xhci_service_interval_to_ns(desc);
>
> - u2_del_ns = le16_to_cpu(udev->bos->ss_cap->bU2DevExitLat) * 1000ULL;
> + u2_del_ns = le16_to_cpu(udev->bos->ss_cap->wU2DevExitLat) * 1000ULL;
> if (u2_del_ns > timeout_ns)
> timeout_ns = u2_del_ns;
>
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 8e81f9eb95e4..ad29bfa23bd9 100644
> --- a/include/linux/
