RE: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB suspend

[Wenyou.Yang]

Hi Alan,

> -Original Message-
> From: Alan Stern [mailto:st...@rowland.harvard.edu]
> Sent: 2016年8月4日 23:02
> To: Wenyou Yang 
> Cc: Greg Kroah-Hartman ; Nicolas Ferre
> ; Alexandre Belloni  electrons.com>; linux-ker...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-usb@vger.kernel.org
> Subject: Re: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB
> suspend
> 
> On Thu, 4 Aug 2016, Wenyou Yang wrote:
> 
> > The usb controller does not managed correctly the suspend mode for the
> > ehci. In echi mode, there is no way to have utmi_suspend_o_n[1]
> > suspend without any device connected to it. This is why this specific
> > control is added to fix this issue. The suspend mode works in ohci
> > mode.
> 
> Why are you talking about EHCI mode?  This patch is only for the
> ohci-at91 driver.

Actually I described the issue according to the documents from our IP,
and this specific control is recommended to do in ohci mode.

> 
> > This specific control is by setting the SUSPEND_A/B/C fields of
> > SFR_OHCIICR(OHCI Interrupt Configuration Register) in the SFR while
> > OHCI USB suspend.
> >
> > This setting operation must be done before the USB clock disabled,
> > clear them after the USB clock enabled.
> >
> > Signed-off-by: Wenyou Yang 
> > Reviewed-by: Alexandre Belloni 
> > Acked-by: Nicolas Ferre 
> 
> I don't know if this is any better than before!  See the comments below.

Yes, I think so.

What else advice?

> 
> > ---
> >
> > Changes in v5:
> >  - Use the USB_PORT_FEAT_SUSPEND subcase of the SetPortFeature case
> >to take care it.
> >  - Update the commit log.
> >
> > Changes in v4:
> >  - To check whether the SFR node with "atmel,sama5d2-sfr" compatible
> >is present or not to decide if this feature is applied or not
> >when USB OHCI suspend/resume, instead of new compatible.
> >  - Drop the compatible "atmel,sama5d2-ohci".
> >  - Drop [PATCH 2/2] ARM: at91/dt: sama5d2: Use new compatible for
> >ohci node.
> >  - Drop include/soc/at91/at91_sfr.h, move the macro definitions to
> >atmel-sfr.h which already exists.
> >  - Change the defines to align the exists.
> >
> > Changes in v3:
> >  - Change the compatible description for more precise.
> >
> > Changes in v2:
> >  - Add compatible to support forcibly suspend the ports.
> >  - Add soc/at91/at91_sfr.h to accommodate the defines.
> >  - Add error checking for .sfr_regmap.
> >  - Remove unnecessary regmap_read() statement.
> 
> 
> > @@ -282,6 +301,28 @@ static int ohci_at91_hub_status_data(struct usb_hcd
> *hcd, char *buf)
> > return length;
> >  }
> >
> > +static int ohci_at91_port_ctrl(struct regmap *regmap, u16 port, u8
> > +set) {
> > +   u32 regval;
> > +   int ret;
> > +
> > +   if (!regmap)
> > +   return 0;
> > +
> > +   ret = regmap_read(regmap, AT91_SFR_OHCIICR, );
> > +   if (ret)
> > +   return ret;
> > +
> > +   if (set)
> > +   regval |= AT91_OHCIICR_SUSPEND(port);
> > +   else
> > +   regval &= ~AT91_OHCIICR_SUSPEND(port);
> 
> In the earlier versions of this patch, you did not use the port number.
> Why has this changed?

This function is called by ohci_at91_hub_control(), which is invoked on basis 
of port number.

So, I think it is more reasonable of adding the port argument. 

> 
> How many ports does this controller have?

This controller has three ports.

> 
> > +
> > +   regmap_write(regmap, AT91_SFR_OHCIICR, regval);
> > +
> > +   return 0;
> > +}
> > +
> >  /*
> >   * Look at the control requests to the root hub and see if we need to 
> > override.
> >   */
> > @@ -289,6 +330,7 @@ static int ohci_at91_hub_control(struct usb_hcd *hcd,
> u16 typeReq, u16 wValue,
> >  u16 wIndex, char *buf, u16 wLength)  {
> > struct at91_usbh_data *pdata =
> > dev_get_platdata(hcd->self.controller);
> > +   struct ohci_at91_priv *ohci_at91 = hcd_to_ohci_at91_priv(hcd);
> > struct usb_hub_descriptor *desc;
> > int ret = -EINVAL;
> > u32 *data = (u32 *)buf;
> > @@ -301,7 +343,8 @@ static int ohci_at91_hub_control(struct usb_hcd
> > *hcd, u16 typeReq, u16 wValue,
> >
> > switch (typeReq) {
> > case SetPortFeature:
> > -   if (wValue == USB_PORT_FEAT_POWER) {
> > +   switch (wValue) {
> > +   case USB_PORT_FEAT_POWER:
> > dev_dbg(hcd->self.controller, "SetPortFeat: POWER\n");
> > if (valid_port(wIndex)) {
> > ohci_at91_usb_set_power(pdata, wIndex, 1); @@
> -309,6 +352,11 @@
> > static int ohci_at91_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 
> > wValue,
> > }
> >
> > goto out;
> > +
> > +   case USB_PORT_FEAT_SUSPEND:
> > +   dev_dbg(hcd->self.controller, "SetPortFeat: 

Re: functionfs example

[Felipe Balbi]

Hi,

Patrick Doyle  writes:
> So far, the Google hasn't helped me much, so I'd like to play "Ask the
> experts".
>
> Can anybody point me at an example of code using functionfs.
>
> I have legacy code that works with gadgetfs that I would like to
> convert to use functionfs, so a wiki page or presentation walking
> through that process would be ideal.

There's an example that ships with kernel sources. It sits in
tools/usb/ffs-test


-- 
balbi


signature.asc
Description: PGP signature

functionfs example

[Patrick Doyle]

So far, the Google hasn't helped me much, so I'd like to play "Ask the experts".

Can anybody point me at an example of code using functionfs.

I have legacy code that works with gadgetfs that I would like to
convert to use functionfs, so a wiki page or presentation walking
through that process would be ideal.

Thanks.

--wpd
--
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: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Andreas Kemnade]

On Thu, 4 Aug 2016 16:49:30 +0200
"H. Nikolaus Schaller"  wrote:

> > Rationale:
> > 
> > The charger driver calls pm_runtime_get_sync(bci->transceiver->dev);
> > which should indirectly call twl4030_usb_set_mode to set the
> > POWER_CTRL_OTG_ENAB bit. This enables the prescaler hardware
> > for ADC8 (VBUS) channel. But this does not happen for reasons
> > outside the charger driver.
> > 
how should that work?
I only have seen the call trace from the musb/omap2430.c glue though
the phy subsystem (via phy_ops) to  twl4030_phy_power_on (which sets
the important bit).
And the phy subsystem has its own power refcounting system which
is brought into disorder by unbalanced calls from the musb system...

Regards,
Andreas
--
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: fix invalid memory access in hub_activate()

[Viresh Kumar]

On 04-08-16, 11:49, Viresh Kumar wrote:
> On 04-08-16, 14:47, Alan Stern wrote:
> > On Thu, 4 Aug 2016, Viresh Kumar wrote:
> > 
> > > On 04-08-16, 11:20, Alan Stern wrote:
> > > > Yes, it could lead to this deadlock.
> > > > 
> > > > > Should we rather have device_trylock instead of device_lock
> > > > 
> > > > No.
> > > > 
> > > > > > @@ -1031,10 +1031,20 @@ static void hub_activate(struct usb_hub
> > > > > > unsigned delay;
> > > > > > 
> > > > > > /* Continue a partial initialization */
> > > > > > -   if (type == HUB_INIT2)
> > > > > > -   goto init2;
> > > > > > -   if (type == HUB_INIT3)
> > > > > > +   if (type == HUB_INIT2 || type == HUB_INIT3) {
> > > > > > +   device_lock(hub->intfdev);
> > > > > 
> > > > > device_trylock?? If failed to acquire then treat that as disconnect?
> > > > 
> > > > No, that's not the right answer.  I believe the correct solution is to
> > > > remove the cancel_delayed_work_sync() call in hub_quiesce().
> > > > 
> > > > Viresh, do you agree?
> > > 
> > > Hmm, so I think we have two different problems here and maybe we should 
> > > fix them
> > > separately. But surely, my patch isn't enough and we need to do it the 
> > > way you
> > > suggested, i.e. let the work die by itself.
> > > 
> > > What about another patch on top of my patch to fix the deadlock?
> > 
> > Or another patch in place of yours to fix both problems.  Has your
> > patch been merged yet?  I don't see it in any of the branches in
> > https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git, and it's
> > not in the current mainline.
> 
> I don't think Greg has applied it yet. Do you want me to send the new patch?

I have sent a V2 now.

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

[PATCH V2] usb: hub: Fix unbalanced reference count/memory leak/deadlocks

[Viresh Kumar]

Memory leak and unbalanced reference count:

If the hub gets disconnected while the core is still activating it, this
can result in leaking memory of few USB structures.

This will happen if we have done a kref_get() from hub_activate() and
scheduled a delayed work item for HUB_INIT2/3. Now if hub_disconnect()
gets called before the delayed work expires, then we will cancel the
work from hub_quiesce(), but wouldn't do a kref_put(). And so the
unbalance.

kmemleak reports this as (with the commit e50293ef9775 backported to
3.10 kernel with other changes, though the same is true for mainline as
well):

unreferenced object 0xffc08af5b800 (size 1024):
  comm "khubd", pid 73, jiffies 4295051211 (age 6482.350s)
  hex dump (first 32 bytes):
30 68 f3 8c c0 ff ff ff 00 a0 b2 2e c0 ff ff ff  0h..
01 00 00 00 00 00 00 00 00 94 7d 40 c0 ff ff ff  ..}@
  backtrace:
[] create_object+0x148/0x2a0
[] kmemleak_alloc+0x80/0xbc
[] kmem_cache_alloc_trace+0x120/0x1ac
[] hub_probe+0x120/0xb84
[] usb_probe_interface+0x1ec/0x298
[] driver_probe_device+0x160/0x374
[] __device_attach+0x28/0x4c
[] bus_for_each_drv+0x78/0xac
[] device_attach+0x6c/0x9c
[] bus_probe_device+0x28/0xa0
[] device_add+0x324/0x604
[] usb_set_configuration+0x660/0x6cc
[] generic_probe+0x44/0x84
[] usb_probe_device+0x54/0x74
[] driver_probe_device+0x160/0x374
[] __device_attach+0x28/0x4c

Deadlocks:

If the hub gets disconnected early enough (i.e. before INIT2/INIT3 are
finished and the init_work is still queued), the core may call
hub_quiesce() after acquiring interface device locks and it will wait
for the work to be cancelled synchronously. But if the work handler is
already running in parallel, it may try to acquire the same interface
device lock and this may result in deadlock.

Fix both the issues by removing the call to cancel_delayed_work_sync().

CC:  #4.4+
Fixes: e50293ef9775 ("USB: fix invalid memory access in hub_activate()")
Reported-by: Manu Gautam 
Signed-off-by: Viresh Kumar 
Acked-by: Alan Stern 
---
V1->V2:
- Solve the deadlock problem as well
- Added Ack from Alan as he was happy with this solution in advance :)
- Dropped cancel-work and kref-put from V1

 drivers/usb/core/hub.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index bee13517676f..3ccffac0f647 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1315,8 +1315,6 @@ static void hub_quiesce(struct usb_hub *hub, enum 
hub_quiescing_type type)
struct usb_device *hdev = hub->hdev;
int i;
 
-   cancel_delayed_work_sync(>init_work);
-
/* hub_wq and related activity won't re-trigger */
hub->quiescing = 1;
 
-- 
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

Re: [PATCH] USB: fix invalid memory access in hub_activate()

[Alan Stern]

On Thu, 4 Aug 2016, Viresh Kumar wrote:

> On 04-08-16, 11:49, Viresh Kumar wrote:
> > On 04-08-16, 14:47, Alan Stern wrote:
> > > On Thu, 4 Aug 2016, Viresh Kumar wrote:
> > > 
> > > > On 04-08-16, 11:20, Alan Stern wrote:
> > > > > Yes, it could lead to this deadlock.
> > > > > 
> > > > > > Should we rather have device_trylock instead of device_lock
> > > > > 
> > > > > No.
> > > > > 
> > > > > > > @@ -1031,10 +1031,20 @@ static void hub_activate(struct usb_hub
> > > > > > >   unsigned delay;
> > > > > > > 
> > > > > > >   /* Continue a partial initialization */
> > > > > > > - if (type == HUB_INIT2)
> > > > > > > - goto init2;
> > > > > > > - if (type == HUB_INIT3)
> > > > > > > + if (type == HUB_INIT2 || type == HUB_INIT3) {
> > > > > > > + device_lock(hub->intfdev);
> > > > > > 
> > > > > > device_trylock?? If failed to acquire then treat that as disconnect?
> > > > > 
> > > > > No, that's not the right answer.  I believe the correct solution is to
> > > > > remove the cancel_delayed_work_sync() call in hub_quiesce().
> > > > > 
> > > > > Viresh, do you agree?
> > > > 
> > > > Hmm, so I think we have two different problems here and maybe we should 
> > > > fix them
> > > > separately. But surely, my patch isn't enough and we need to do it the 
> > > > way you
> > > > suggested, i.e. let the work die by itself.
> > > > 
> > > > What about another patch on top of my patch to fix the deadlock?
> > > 
> > > Or another patch in place of yours to fix both problems.  Has your
> > > patch been merged yet?  I don't see it in any of the branches in
> > > https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git, and it's
> > > not in the current mainline.
> > 
> > I don't think Greg has applied it yet. Do you want me to send the new patch?
> 
> I have sent a V2 now.

Okay.  I'll add my two changes on top of yours.

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: [PATCH] USB: fix invalid memory access in hub_activate()

[Alan Stern]

On Thu, 4 Aug 2016, Viresh Kumar wrote:

> > > What about another patch on top of my patch to fix the deadlock?
> > 
> > Or another patch in place of yours to fix both problems.  Has your
> > patch been merged yet?  I don't see it in any of the branches in
> > https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git, and it's
> > not in the current mainline.
> 
> I don't think Greg has applied it yet. Do you want me to send the new patch?

I'm concerned about the locking.  hub_activate needs to be mutually
exclusive with hub_quiesce.  But hub_activate acquire the lock for the
usb_interface, whereas callers of hub_quiesce (like hub_pre_reset and
hub_event) acquire the lock for the usb_device.  I can't remember why I
did it that way.  It looks like hub_activate really should acquire the
usb_device lock.

Also, although this isn't strictly necessary, the early-exit path 
should call usb_autopm_put_interface_async.  Ideally, it should jump 
directly to the end of the routine, where these things are already 
being done.

In short, it appears that fixing this properly requires a series of 3 
patches.  I'd better write them.

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: usbtmc: vendor specific i/o

[Ladislav Michl]

On Thu, Aug 04, 2016 at 06:59:30AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Aug 03, 2016 at 09:06:25AM +0200, Ladislav Michl wrote:
> > Yes, also was lacking proper description and signoff. So, I'm considering
> > ioctls based approach okay, although that question (the only one I really
> > had) was never answered.
> > 
> > After re-reading specifications [*] I decided to allow arbitrary MsgID
> > selection, as USB488 adds MsgID=TRIGGER (128) and other subclass
> > specifications may add other values.
> > 
> > [*] http://www.usb.org/developers/docs/devclass_docs/USBTMC_1_006a.zip
> > 
> > After sorting out all eventual objections, patch bellow will be turned
> > into proper one.
> 
> Looks reasonable to me.

After all, it is not that reasonable. Spec does not define anything like
EOM or TermCharEnabled bits in bmTransferAttributes nor TermChar field
in vendor specific messages - usbtmc_read and usbtmc_write are using
these fields when concatenating usb transfers. I'll need to think a bit
more about it...

Meanwhile, there's a small race condition, which needs to be fixed first.
When device is disconnected there's a chance usbtmc_read tries to lock
already destroyd mutex, see bellow. Fix will come later in separate patch.

thank you,
ladis

Unable to handle kernel NULL pointer dereference at virtual address 0008
pgd = cd55c000
[0008] *pgd=8d52e831, *pte=, *ppte=
Internal error: Oops: 17 [#1] ARM
Modules linked in: usbtmc ppp_deflate bsd_comp ppp_async crc_ccitt ppp_generic 
slhc cpufreq_dt udlfb syscopyarea sysfillrect sysimgblt fb_sys_fops omap_aes 
omap_sham crypto_engine omap_mailbox option cdc_acm usb_wwan usbserial 
usb_storage scsi_mod
CPU: 0 PID: 205 Comm: tvm3 Tainted: GW   4.6.0 #1
Hardware name: Generic OMAP36xx (Flattened Device Tree)
task: ce2ae700 ti: ce1e2000 task.ti: ce1e2000
PC is at __bfs+0x11c/0x23c
LR is at warn_slowpath_null+0x1c/0x24
pc : []lr : []psr: 60010093
sp : ce1e3d30  ip :   fp : c0a76388
r10:   r9 : ce1e3d74  r8 : c0a72398
r7 : ce1e3d70  r6 : c0a76388  r5 : c0c433f4  r4 : c0a72398
r3 : 0200  r2 : e55130aa  r1 : c0951cd0  r0 : 
Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 8d55c019  DAC: 0051
Process tvm3 (pid: 205, stack limit = 0xce1e2210)
Stack: (0xce1e3d30 to 0xce1e4000)
3d20: ce1e4000 c014f928  e55130aa
3d40: 0003 0001 ce2aeab0 ce2ae700 c073ceee   c0151090
3d60:  c015113c  c0c99af8 ce2aeab0 c010adc4 c0c4333c c0c432fc
3d80:    ce1e3fa8 c01074a0  0072 0001
3da0: 0002 ce2aeab0 c0603f60 ce2ae700  c0151cc4 ce2ae700 
3dc0: 3c48dade 001e 0001 ce103300 ce2ae700 c0152c88  ce2a0030
3de0: ce2aeab0  0100 c0151fcc ce2ae700 0001 c01c6a8c 001a9ca0
3e00: ce80 c0152174 cd4e5980 a0010013 cfc7cf7c c01c6a8c  ce245420
3e20: 0003  60010013 0001 c10ed4ec ce2ae700 ce1e2000 
3e40: 0100 c015470c 0001   bf14d88c  
3e60: cd5a5a60 bf14d840 ce20c800 c05a8c94 0001  bf14d88c a0010013
3e80: c0107644 c0152174 ce20c800 ce800300 cd5a5a10 bf14d840 ce20c800 c10f5cc8
3ea0: c0107644 ce1e2000  bf14d88c ce2aeab0 0001 0051 
3ec0: b2afeb60 cd5a5a60 ce1e3f88 0001 000670f1 c0f3c608  cd5a7a00
3ee0: bf14d840 ce1e3f88 b2afeb60 c0107644 ce1e2000   c01cc0b8
3f00:  c02e34f0   0c00  cd5a7a00 0100
3f20:  c01ccb40 4000 c0943184 0038   b2afeb60
3f40: cd5a7a00 ce1e3f88 b2afeb60 b2afeb60 cd5a7a00 ce1e3f88 b2afeb60 c01ccc58
3f60: cd5a7a00 b2afeb60 0100 cd5a7a00 cd5a7a01 0100 b2afeb60 c0107644
3f80: ce1e2000 c01cd834 0c00  0100 000e  000a8050
3fa0: 0003 c01074a0 000e  000e b2afeb60 0100 
3fc0: 000e  000a8050 0003 000a8054 000a67cc 0008e3b4 
3fe0:  b2afeb40  b6d48b60 80010030 000e 8fef6861 8fef6c61
[] (__bfs) from [] (check_usage_backwards+0xac/0x140)
[] (check_usage_backwards) from [] (mark_lock+0x36c/0x618)
[] (mark_lock) from [] (__lock_acquire+0x880/0x1b88)
[] (__lock_acquire) from [] (lock_acquire+0x70/0x90)
[] (lock_acquire) from [] (mutex_lock_nested+0x3c/0x314)
[] (mutex_lock_nested) from [] (usbtmc_read+0x4c/0x4e8 
[usbtmc])
[] (usbtmc_read [usbtmc]) from [] (__vfs_read+0x20/0xcc)
[] (__vfs_read) from [] (vfs_read+0x84/0xec)
[] (vfs_read) from [] (SyS_read+0x40/0x80)
[] (SyS_read) from [] (ret_fast_syscall+0x0/0x1c)
Code: e30013af e58d200c ebff61be e59d200c (e59a1008)
---[ end trace dd5c876458afcc20 ]---
Kernel panic - not syncing: Fatal exception
--
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  

Re: pwc over musb: 100% frame drop (lost) on high resolution stream

[Matwey V. Kornilov]

When DMA is not used, I see the same behavior: lots of zero-length
packages received.

Can It be related to some kind of USB overflow due to long input data
processing with disabled IRQ?
When HCD_BC is used then part of processing is postponed and this can
explain greater throughput due to better latency.

2016-08-04 22:58 GMT+03:00 Matwey V. Kornilov :
> I've just found that in such cases, when DMA actual length is zero,
> both cppi41_channel->prog_len and txstate.residue equal 960 at
> musb_cppi41 line 225:
>
> http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/usb/musb/musb_cppi41.c#n225
>
> 2016-08-04 22:08 GMT+03:00 Matwey V. Kornilov :
>> I've just found that dma->actual_len equals to zero in most cases at
>> musb_host.c line 1946.
>> And this produces zero-length packages.
>>
>> http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/usb/musb/musb_host.c#n1946
>>
>> Any ideas why?
>>
>> 2016-08-04 19:57 GMT+03:00 Matwey V. Kornilov :
>>> I've just found that many packages in URBs have zero actual_length (It
>>> is a question why).
>>> Then the following end of frame criteria leads to `frame underflow' message:
>>>
>>> if (flen < pdev->vlast_packet_size) {
>>> /* Shorter packet... end of frame */
>>> if (pdev->vsync == 2)
>>> pwc_frame_complete(pdev);
>>> if (pdev->fill_buf == NULL)
>>> pdev->fill_buf = 
>>> pwc_get_next_fill_buf(pdev);
>>> if (pdev->fill_buf) {
>>> pdev->fill_buf->filled = 0;
>>> pdev->vsync = 1;
>>> }
>>> }
>>>
>>> 2016-08-01 21:16 GMT+03:00 Matwey V. Kornilov :
 pwc module output with trace=511 is the following:

 [   24.793109] usbcore: registered new interface driver Philips webcam
 [   29.276979] pwc: Unsupported pixel format
 [   29.277055] pwc: pwc_vidioc_fill_fmt() width=640, height=480,
 bytesperline=640, sizeimage=460800, pixelformat=YU12
 [   29.277090] pwc: Trying to set format to: width=640 height=480
 fps=15 format=YU12
 [   29.277123] pwc: set_video_mode(640x480 @ 30, pixfmt 32315559).
 [   29.277145] pwc: decode_size = 5.
 [   29.277180] pwc: frame_size=63120, vframes=15, vsize=5, vbandlength=526
 [   29.277204] pwc: Set resolution to 640x480
 [   29.277225] pwc: pwc_set_video_mode(), return=0
 [   29.277256] pwc: pwc_vidioc_fill_fmt() width=640, height=480,
 bytesperline=640, sizeimage=460800, pixelformat=YU12
 [   29.277306] pwc: ioctl(VIDIOC_G_FMT) return size 640x480
 [   29.277337] pwc: pwc_vidioc_fill_fmt() width=640, height=480,
 bytesperline=640, sizeimage=460800, pixelformat=YU12
 [   29.277449] pwc: set_video_mode(640x480 @ 10, pixfmt 32315559).
 [   29.277475] pwc: decode_size = 5.
 [   29.278726] pwc: frame_size=94560, vframes=10, vsize=5, vbandlength=788
 [   29.278750] pwc: Set resolution to 640x480
 [   29.300374] pwc: set_video_mode(640x480 @ 10, pixfmt 32315559).
 [   29.300420] pwc: decode_size = 5.
 [   29.441759] pwc: frame_size=94560, vframes=10, vsize=5, vbandlength=788
 [   29.441792] pwc: Set resolution to 640x480
 [   29.441824] pwc: Setting alternate interface 9
 [   29.455061] pwc: Allocated URB at 0xc9b83600
 [   29.455850] pwc: Allocated URB at 0xc9b83400
 [   29.456040] pwc: Allocated URB at 0xc9b83200
 [   29.456271] pwc: URB 0xc9b83600 submitted.
 [   29.456310] pwc: URB 0xc9b83400 submitted.
 [   29.456341] pwc: URB 0xc9b83200 submitted.
 [   29.456362] pwc: << pwc_isoc_init()
 [   30.078550] pwc: Frame buffer underflow (20076 bytes); discarded.
 [   30.170543] pwc: Frame buffer underflow (12428 bytes); discarded.
 [   30.272538] pwc: Frame buffer underflow (14340 bytes); discarded.
 [   30.374541] pwc: Frame buffer underflow (16252 bytes); discarded.
 [   30.476535] pwc: Frame buffer underflow (18164 bytes); discarded.
 [   30.578532] pwc: Frame buffer underflow (20076 bytes); discarded.
 [   30.670538] pwc: Frame buffer underflow (12428 bytes); discarded.
 [   30.772544] pwc: Frame buffer underflow (14340 bytes); discarded.
 [   30.874547] pwc: Frame buffer underflow (16252 bytes); discarded.
 [   30.976552] pwc: Frame buffer underflow (18164 bytes); discarded.
 [   31.078536] pwc: Frame buffer underflow (20076 bytes); discarded.
 [   31.170533] pwc: Frame buffer underflow (12428 bytes); discarded.
 [   31.272549] pwc: Frame buffer underflow (14340 bytes); discarded.
 [   31.374545] pwc: Frame buffer underflow (16252 bytes); discarded.
 [   31.476537] pwc: Frame buffer underflow (18164 bytes); discarded.
 [   31.578536] pwc: Frame buffer 

Re: pwc over musb: 100% frame drop (lost) on high resolution stream

[Matwey V. Kornilov]

I've just found that in such cases, when DMA actual length is zero,
both cppi41_channel->prog_len and txstate.residue equal 960 at
musb_cppi41 line 225:

http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/usb/musb/musb_cppi41.c#n225

2016-08-04 22:08 GMT+03:00 Matwey V. Kornilov :
> I've just found that dma->actual_len equals to zero in most cases at
> musb_host.c line 1946.
> And this produces zero-length packages.
>
> http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/usb/musb/musb_host.c#n1946
>
> Any ideas why?
>
> 2016-08-04 19:57 GMT+03:00 Matwey V. Kornilov :
>> I've just found that many packages in URBs have zero actual_length (It
>> is a question why).
>> Then the following end of frame criteria leads to `frame underflow' message:
>>
>> if (flen < pdev->vlast_packet_size) {
>> /* Shorter packet... end of frame */
>> if (pdev->vsync == 2)
>> pwc_frame_complete(pdev);
>> if (pdev->fill_buf == NULL)
>> pdev->fill_buf = pwc_get_next_fill_buf(pdev);
>> if (pdev->fill_buf) {
>> pdev->fill_buf->filled = 0;
>> pdev->vsync = 1;
>> }
>> }
>>
>> 2016-08-01 21:16 GMT+03:00 Matwey V. Kornilov :
>>> pwc module output with trace=511 is the following:
>>>
>>> [   24.793109] usbcore: registered new interface driver Philips webcam
>>> [   29.276979] pwc: Unsupported pixel format
>>> [   29.277055] pwc: pwc_vidioc_fill_fmt() width=640, height=480,
>>> bytesperline=640, sizeimage=460800, pixelformat=YU12
>>> [   29.277090] pwc: Trying to set format to: width=640 height=480
>>> fps=15 format=YU12
>>> [   29.277123] pwc: set_video_mode(640x480 @ 30, pixfmt 32315559).
>>> [   29.277145] pwc: decode_size = 5.
>>> [   29.277180] pwc: frame_size=63120, vframes=15, vsize=5, vbandlength=526
>>> [   29.277204] pwc: Set resolution to 640x480
>>> [   29.277225] pwc: pwc_set_video_mode(), return=0
>>> [   29.277256] pwc: pwc_vidioc_fill_fmt() width=640, height=480,
>>> bytesperline=640, sizeimage=460800, pixelformat=YU12
>>> [   29.277306] pwc: ioctl(VIDIOC_G_FMT) return size 640x480
>>> [   29.277337] pwc: pwc_vidioc_fill_fmt() width=640, height=480,
>>> bytesperline=640, sizeimage=460800, pixelformat=YU12
>>> [   29.277449] pwc: set_video_mode(640x480 @ 10, pixfmt 32315559).
>>> [   29.277475] pwc: decode_size = 5.
>>> [   29.278726] pwc: frame_size=94560, vframes=10, vsize=5, vbandlength=788
>>> [   29.278750] pwc: Set resolution to 640x480
>>> [   29.300374] pwc: set_video_mode(640x480 @ 10, pixfmt 32315559).
>>> [   29.300420] pwc: decode_size = 5.
>>> [   29.441759] pwc: frame_size=94560, vframes=10, vsize=5, vbandlength=788
>>> [   29.441792] pwc: Set resolution to 640x480
>>> [   29.441824] pwc: Setting alternate interface 9
>>> [   29.455061] pwc: Allocated URB at 0xc9b83600
>>> [   29.455850] pwc: Allocated URB at 0xc9b83400
>>> [   29.456040] pwc: Allocated URB at 0xc9b83200
>>> [   29.456271] pwc: URB 0xc9b83600 submitted.
>>> [   29.456310] pwc: URB 0xc9b83400 submitted.
>>> [   29.456341] pwc: URB 0xc9b83200 submitted.
>>> [   29.456362] pwc: << pwc_isoc_init()
>>> [   30.078550] pwc: Frame buffer underflow (20076 bytes); discarded.
>>> [   30.170543] pwc: Frame buffer underflow (12428 bytes); discarded.
>>> [   30.272538] pwc: Frame buffer underflow (14340 bytes); discarded.
>>> [   30.374541] pwc: Frame buffer underflow (16252 bytes); discarded.
>>> [   30.476535] pwc: Frame buffer underflow (18164 bytes); discarded.
>>> [   30.578532] pwc: Frame buffer underflow (20076 bytes); discarded.
>>> [   30.670538] pwc: Frame buffer underflow (12428 bytes); discarded.
>>> [   30.772544] pwc: Frame buffer underflow (14340 bytes); discarded.
>>> [   30.874547] pwc: Frame buffer underflow (16252 bytes); discarded.
>>> [   30.976552] pwc: Frame buffer underflow (18164 bytes); discarded.
>>> [   31.078536] pwc: Frame buffer underflow (20076 bytes); discarded.
>>> [   31.170533] pwc: Frame buffer underflow (12428 bytes); discarded.
>>> [   31.272549] pwc: Frame buffer underflow (14340 bytes); discarded.
>>> [   31.374545] pwc: Frame buffer underflow (16252 bytes); discarded.
>>> [   31.476537] pwc: Frame buffer underflow (18164 bytes); discarded.
>>> [   31.578536] pwc: Frame buffer underflow (20076 bytes); discarded.
>>> [   31.660895] pwc: Frame buffer underflow (11472 bytes); discarded.
>>> [   31.772554] pwc: Frame buffer underflow (14340 bytes); discarded.
>>> [   31.874548] pwc: Frame buffer underflow (16252 bytes); discarded.
>>> [   31.976533] pwc: Frame buffer underflow (18164 bytes); discarded.
>>>
>>>
>>> 2016-07-31 23:31 GMT+03:00 Matwey V. Kornilov :
 Hello,

 I've also just found that the same commit 

Re: pwc over musb: 100% frame drop (lost) on high resolution stream

[Matwey V. Kornilov]

I've just found that dma->actual_len equals to zero in most cases at
musb_host.c line 1946.
And this produces zero-length packages.

http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/usb/musb/musb_host.c#n1946

Any ideas why?

2016-08-04 19:57 GMT+03:00 Matwey V. Kornilov :
> I've just found that many packages in URBs have zero actual_length (It
> is a question why).
> Then the following end of frame criteria leads to `frame underflow' message:
>
> if (flen < pdev->vlast_packet_size) {
> /* Shorter packet... end of frame */
> if (pdev->vsync == 2)
> pwc_frame_complete(pdev);
> if (pdev->fill_buf == NULL)
> pdev->fill_buf = pwc_get_next_fill_buf(pdev);
> if (pdev->fill_buf) {
> pdev->fill_buf->filled = 0;
> pdev->vsync = 1;
> }
> }
>
> 2016-08-01 21:16 GMT+03:00 Matwey V. Kornilov :
>> pwc module output with trace=511 is the following:
>>
>> [   24.793109] usbcore: registered new interface driver Philips webcam
>> [   29.276979] pwc: Unsupported pixel format
>> [   29.277055] pwc: pwc_vidioc_fill_fmt() width=640, height=480,
>> bytesperline=640, sizeimage=460800, pixelformat=YU12
>> [   29.277090] pwc: Trying to set format to: width=640 height=480
>> fps=15 format=YU12
>> [   29.277123] pwc: set_video_mode(640x480 @ 30, pixfmt 32315559).
>> [   29.277145] pwc: decode_size = 5.
>> [   29.277180] pwc: frame_size=63120, vframes=15, vsize=5, vbandlength=526
>> [   29.277204] pwc: Set resolution to 640x480
>> [   29.277225] pwc: pwc_set_video_mode(), return=0
>> [   29.277256] pwc: pwc_vidioc_fill_fmt() width=640, height=480,
>> bytesperline=640, sizeimage=460800, pixelformat=YU12
>> [   29.277306] pwc: ioctl(VIDIOC_G_FMT) return size 640x480
>> [   29.277337] pwc: pwc_vidioc_fill_fmt() width=640, height=480,
>> bytesperline=640, sizeimage=460800, pixelformat=YU12
>> [   29.277449] pwc: set_video_mode(640x480 @ 10, pixfmt 32315559).
>> [   29.277475] pwc: decode_size = 5.
>> [   29.278726] pwc: frame_size=94560, vframes=10, vsize=5, vbandlength=788
>> [   29.278750] pwc: Set resolution to 640x480
>> [   29.300374] pwc: set_video_mode(640x480 @ 10, pixfmt 32315559).
>> [   29.300420] pwc: decode_size = 5.
>> [   29.441759] pwc: frame_size=94560, vframes=10, vsize=5, vbandlength=788
>> [   29.441792] pwc: Set resolution to 640x480
>> [   29.441824] pwc: Setting alternate interface 9
>> [   29.455061] pwc: Allocated URB at 0xc9b83600
>> [   29.455850] pwc: Allocated URB at 0xc9b83400
>> [   29.456040] pwc: Allocated URB at 0xc9b83200
>> [   29.456271] pwc: URB 0xc9b83600 submitted.
>> [   29.456310] pwc: URB 0xc9b83400 submitted.
>> [   29.456341] pwc: URB 0xc9b83200 submitted.
>> [   29.456362] pwc: << pwc_isoc_init()
>> [   30.078550] pwc: Frame buffer underflow (20076 bytes); discarded.
>> [   30.170543] pwc: Frame buffer underflow (12428 bytes); discarded.
>> [   30.272538] pwc: Frame buffer underflow (14340 bytes); discarded.
>> [   30.374541] pwc: Frame buffer underflow (16252 bytes); discarded.
>> [   30.476535] pwc: Frame buffer underflow (18164 bytes); discarded.
>> [   30.578532] pwc: Frame buffer underflow (20076 bytes); discarded.
>> [   30.670538] pwc: Frame buffer underflow (12428 bytes); discarded.
>> [   30.772544] pwc: Frame buffer underflow (14340 bytes); discarded.
>> [   30.874547] pwc: Frame buffer underflow (16252 bytes); discarded.
>> [   30.976552] pwc: Frame buffer underflow (18164 bytes); discarded.
>> [   31.078536] pwc: Frame buffer underflow (20076 bytes); discarded.
>> [   31.170533] pwc: Frame buffer underflow (12428 bytes); discarded.
>> [   31.272549] pwc: Frame buffer underflow (14340 bytes); discarded.
>> [   31.374545] pwc: Frame buffer underflow (16252 bytes); discarded.
>> [   31.476537] pwc: Frame buffer underflow (18164 bytes); discarded.
>> [   31.578536] pwc: Frame buffer underflow (20076 bytes); discarded.
>> [   31.660895] pwc: Frame buffer underflow (11472 bytes); discarded.
>> [   31.772554] pwc: Frame buffer underflow (14340 bytes); discarded.
>> [   31.874548] pwc: Frame buffer underflow (16252 bytes); discarded.
>> [   31.976533] pwc: Frame buffer underflow (18164 bytes); discarded.
>>
>>
>> 2016-07-31 23:31 GMT+03:00 Matwey V. Kornilov :
>>> Hello,
>>>
>>> I've also just found that the same commit breaks cpufreq on BeagleBone 
>>> Black :)
>>>
>>> So, probably without HCD_BH flag musb works correctly only at 1Ghz CPU
>>> frequency, which is unlisted and being set to 720Mhz by cpufreq driver
>>> (as it did whet there was cpufreq driver).
>>>
>>> 2016-07-29 21:01 GMT+03:00 Matwey V. Kornilov :
 Hello,

 I've found that the following commit fixes the issue:

 commit 

[PATCH] usb: musb: remove redundant stack buffers

[Rasmus Villemoes]

aDate is always the empty string, so entirely pointless. The aRevision
formatting might as well be done as part of the pr_debug() call - that
also avoids it altogether if pr_debug is compiled out.

Signed-off-by: Rasmus Villemoes 
---
 drivers/usb/musb/musb_core.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 74fc3069cb42..1724f1889c99 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1448,7 +1448,7 @@ static int musb_core_init(u16 musb_type, struct musb 
*musb)
 {
u8 reg;
char *type;
-   char aInfo[90], aRevision[32], aDate[12];
+   char aInfo[90];
void __iomem*mbase = musb->mregs;
int status = 0;
int i;
@@ -1482,7 +1482,6 @@ static int musb_core_init(u16 musb_type, struct musb 
*musb)
 
pr_debug("%s: ConfigData=0x%02x (%s)\n", musb_driver_name, reg, aInfo);
 
-   aDate[0] = 0;
if (MUSB_CONTROLLER_MHDRC == musb_type) {
musb->is_multipoint = 1;
type = "M";
@@ -1497,11 +1496,10 @@ static int musb_core_init(u16 musb_type, struct musb 
*musb)
 
/* log release info */
musb->hwvers = musb_read_hwvers(mbase);
-   snprintf(aRevision, 32, "%d.%d%s", MUSB_HWVERS_MAJOR(musb->hwvers),
-   MUSB_HWVERS_MINOR(musb->hwvers),
-   (musb->hwvers & MUSB_HWVERS_RC) ? "RC" : "");
-   pr_debug("%s: %sHDRC RTL version %s %s\n",
-musb_driver_name, type, aRevision, aDate);
+   pr_debug("%s: %sHDRC RTL version %d.%d%s\n",
+musb_driver_name, type, MUSB_HWVERS_MAJOR(musb->hwvers),
+MUSB_HWVERS_MINOR(musb->hwvers),
+(musb->hwvers & MUSB_HWVERS_RC) ? "RC" : "");
 
/* configure ep0 */
musb_configure_ep0(musb);
-- 
2.1.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

Re: [PATCH] USB: fix invalid memory access in hub_activate()

[Viresh Kumar]

On 04-08-16, 14:47, Alan Stern wrote:
> On Thu, 4 Aug 2016, Viresh Kumar wrote:
> 
> > On 04-08-16, 11:20, Alan Stern wrote:
> > > Yes, it could lead to this deadlock.
> > > 
> > > > Should we rather have device_trylock instead of device_lock
> > > 
> > > No.
> > > 
> > > > > @@ -1031,10 +1031,20 @@ static void hub_activate(struct usb_hub
> > > > >   unsigned delay;
> > > > > 
> > > > >   /* Continue a partial initialization */
> > > > > - if (type == HUB_INIT2)
> > > > > - goto init2;
> > > > > - if (type == HUB_INIT3)
> > > > > + if (type == HUB_INIT2 || type == HUB_INIT3) {
> > > > > + device_lock(hub->intfdev);
> > > > 
> > > > device_trylock?? If failed to acquire then treat that as disconnect?
> > > 
> > > No, that's not the right answer.  I believe the correct solution is to
> > > remove the cancel_delayed_work_sync() call in hub_quiesce().
> > > 
> > > Viresh, do you agree?
> > 
> > Hmm, so I think we have two different problems here and maybe we should fix 
> > them
> > separately. But surely, my patch isn't enough and we need to do it the way 
> > you
> > suggested, i.e. let the work die by itself.
> > 
> > What about another patch on top of my patch to fix the deadlock?
> 
> Or another patch in place of yours to fix both problems.  Has your
> patch been merged yet?  I don't see it in any of the branches in
> https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git, and it's
> not in the current mainline.

I don't think Greg has applied it yet. Do you want me to send the new patch?

-- 
viresh
--
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: fix invalid memory access in hub_activate()

[Alan Stern]

On Thu, 4 Aug 2016, Viresh Kumar wrote:

> On 04-08-16, 11:20, Alan Stern wrote:
> > Yes, it could lead to this deadlock.
> > 
> > > Should we rather have device_trylock instead of device_lock
> > 
> > No.
> > 
> > > > @@ -1031,10 +1031,20 @@ static void hub_activate(struct usb_hub
> > > > unsigned delay;
> > > > 
> > > > /* Continue a partial initialization */
> > > > -   if (type == HUB_INIT2)
> > > > -   goto init2;
> > > > -   if (type == HUB_INIT3)
> > > > +   if (type == HUB_INIT2 || type == HUB_INIT3) {
> > > > +   device_lock(hub->intfdev);
> > > 
> > > device_trylock?? If failed to acquire then treat that as disconnect?
> > 
> > No, that's not the right answer.  I believe the correct solution is to
> > remove the cancel_delayed_work_sync() call in hub_quiesce().
> > 
> > Viresh, do you agree?
> 
> Hmm, so I think we have two different problems here and maybe we should fix 
> them
> separately. But surely, my patch isn't enough and we need to do it the way you
> suggested, i.e. let the work die by itself.
> 
> What about another patch on top of my patch to fix the deadlock?

Or another patch in place of yours to fix both problems.  Has your
patch been merged yet?  I don't see it in any of the branches in
https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git, and it's
not in the current mainline.

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: [PACTH v1] cdc-wdm: Clear read pipeline in case of error

[Robert Foss]

On 2016-08-03 06:39 AM, Oliver Neukum wrote:

On Tue, 2016-08-02 at 10:37 -0400, Robert Foss wrote:


On 2016-08-02 09:59 AM, Oliver Neukum wrote:

On Tue, 2016-08-02 at 09:54 -0400, Robert Foss wrote:


On 2016-08-02 08:23 AM, Oliver Neukum wrote:

On Thu, 2016-07-28 at 14:19 -0400, robert.f...@collabora.com wrote:

From: Prathmesh Prabhu 

Implemented queued response handling. This queue is processed every
time the
WDM_READ flag is cleared.

In case of a read error, userspace may not actually read the data,
since the
driver returns an error through wdm_poll. After this, the underlying
device may
attempt to send us more data, but the queue is not processed. While
userspace is
also blocked, because the read error is never cleared.


Could you explain why user space cannot just read more data?
That will clear the error.


Userspace certainly could read more data, but for the case when
userspace doesn't read and clear a potential an error, we still would
like to not be stuck if the device sends more data. space

I hope that answers your question, if not I'll try to be more elaborate.


Clear, but why does that require the suppression of an error condition?
errors should always be delivered.


The goal is not to clear the error condition, but that is required to
not stay stuck.


How can that depend on what we return to user space?
In the driver we can continue just ignoring errors.
Now, if user space stops reading because we reported an error,
that is the decision user space has made. We cannot ignore errors
in the kernel because we don't like what user space does when it
sees the error.


So perhaps the better solution is to be more intelligent about how 
desc->rerr is written to during after an error to be able to maintain 
the error condition?




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: [PATCH] USB: fix invalid memory access in hub_activate()

[Viresh Kumar]

On 04-08-16, 11:20, Alan Stern wrote:
> Yes, it could lead to this deadlock.
> 
> > Should we rather have device_trylock instead of device_lock
> 
> No.
> 
> > > @@ -1031,10 +1031,20 @@ static void hub_activate(struct usb_hub
> > >   unsigned delay;
> > > 
> > >   /* Continue a partial initialization */
> > > - if (type == HUB_INIT2)
> > > - goto init2;
> > > - if (type == HUB_INIT3)
> > > + if (type == HUB_INIT2 || type == HUB_INIT3) {
> > > + device_lock(hub->intfdev);
> > 
> > device_trylock?? If failed to acquire then treat that as disconnect?
> 
> No, that's not the right answer.  I believe the correct solution is to
> remove the cancel_delayed_work_sync() call in hub_quiesce().
> 
> Viresh, do you agree?

Hmm, so I think we have two different problems here and maybe we should fix them
separately. But surely, my patch isn't enough and we need to do it the way you
suggested, i.e. let the work die by itself.

What about another patch on top of my patch to fix the deadlock?

-- 
viresh
--
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 2/2] usb: dwc3: core: allow device to runtime_suspend several times

[Alan Stern]

On Thu, 4 Aug 2016, Felipe Balbi wrote:

> Hi,
> 
> Alan Stern  writes:
> 
> > On Thu, 4 Aug 2016, Felipe Balbi wrote:
> >
> >> > What are you trying to accomplish?  Is the problem that wakeup signals
> >> > only cause the platform device to be runtime-resumed, but you also need
> >> > the HCD to wake up?  And conversely, whenever the HCD gets
> >> > runtime-suspended you also want the platform device to go into runtime
> >> > suspend?
> >> 
> >> this is DWC3 as peripheral-only. PCI Wakeup (PME) wakes up dwc3-pci, but
> >> we need dwc3 core (a platform device) to be resumed as well.
> >
> > Oh, so I got the types wrong.  But the basic idea is still the same: 
> > When the parent wakes up, it does a runtime-resume of the child.  The 
> > child is then responsible for making sure it stays awake as long as 
> > necessary, and when it goes back to low power the parent naturally does 
> > the same.
> 
> right. Here's a doubt though: if dwc3-pci does a pm_runtime_resume() of
> the child, doesn't that mean child will try to sleep right away again
> since its usage_counter will be zero? We _do_ have a 5s timeout which
> should be enough to trigger enumeration, but still... I guess I'm
> missing something here.

That is the subtle part of the scheme.  It depends on why the wakeup 
occurred.

For example, suppose the wakeup signal was triggered by a port-connect
(Vbus) event.  When the child dwc3-core driver's runtime-resume
callback runs, it should check the port status.  When it sees the Vbus
change, it will need to increment the runtime-PM usage counter until
the connect event has been handled (or until the driver decides it can
safely let the dwc3-core device go back to low power, which might be
some time later).

The same sort of thing should happen for other kinds of wakeup events.
This is how the PM core expects all drivers to work.  When the 
runtime-resume callback is invoked, the core guarantees there won't be 
any other runtime-PM callbacks until runtime-resume returns.  But once 
the callback does return, all bets are off -- if the driver wants to 
keep the device active, it needs to increment the usage counter.

> I guess it'll work just fine since we set a driver flag when we see a
> bus reset and that gets cleared once a disconnect shows up. Seems like
> the whole idea is to avoid an extra pm_runtime_put() in
> ->runtime_resume()?

The whole idea of doing pm_runtime_put() in the runtime_resume callback 
is just bizarre.  It means that somebody had previously incremented the 
usage counter for no reason (since the counter won't be used for 
keeping the device at full power).

Instead of having some other code call pm_runtime_get() and then
calling pm_runtime_put() in the resume handler, why not simply have
that other code call pm_request_resume() and the resume handler do
nothing?

> What if user sets autosuspend_delay to 0 in sysfs? Seems like this is
> likely to break, no?

No, the driver should be written to work properly even in that case.  
Setting the autosuspend_delay to 0 might cause a bunch of unnecessary 
or very short suspend/resume cycles to occur, but it shouldn't break 
anything.

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: [PATCH 1/1] Add driver for smsc usb251x i2c configuration

[Rob Herring]

On Tue, Aug 02, 2016 at 04:31:54PM +0200, Fabien Lahoudere wrote:
> This driver copy the configuration of the controller EEPROM via i2c.
> Configuration information is available in Documentation/usb/usb251x.txt
> 
> Signed-off-by: Fabien Lahoudere 
> ---
>  Documentation/devicetree/bindings/usb/usb251x.txt |  27 +++
>  drivers/usb/misc/Kconfig  |   9 +
>  drivers/usb/misc/Makefile |   1 +
>  drivers/usb/misc/usb251x.c| 265 
> ++
>  4 files changed, 302 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/usb251x.txt
>  create mode 100644 drivers/usb/misc/usb251x.c
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb251x.txt 
> b/Documentation/devicetree/bindings/usb/usb251x.txt
> new file mode 100644
> index 000..2b0863a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/usb251x.txt
> @@ -0,0 +1,27 @@
> +SMSC USB 2.0 Hi-Speed Hub Controller
> +
> +Required properties:
> +- compatible = "smsc,usb251x";
> +- reg = <0x2c>;
> +
> +Optional properties:
> +- smsc,usb251x-cfg-data1 : u8, set configuration data 1 (byte 0x06)
> +- smsc,usb251x-cfg-data2 : u8, set configuration data 2 (byte 0x07)
> +- smsc,usb251x-cfg-data3 : u8, set configuration data 3 (byte 0x08)
> +- smsc,usb251x-portmap12 : u8, set port mapping for ports 1 and 2 (byte 0xfb)
> +- smsc,usb251x-portmap34 : u8, set port mapping for ports 3 and 4 (byte 0xfc)
> +- smsc,usb251x-portmap56 : u8, set port mapping for ports 5 and 6 (byte 0xfd)
> +- smsc,usb251x-portmap7 : u8, set port mapping for port 7 (byte 0xfe)
> +- smsc,usb251x-status-command : u8, configure smbus behaviour (byte 0xff)

This is to override what's in the EEPROM or for when there is no EEPROM 
(or both)? 

What about the other values? Are they likely to need to be added later? 
I think I'd rather see either specific properties for specific settings 
if only a few or a single property for the whole EEPROM.

Rob
--
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: pwc over musb: 100% frame drop (lost) on high resolution stream

[Matwey V. Kornilov]

I've just found that many packages in URBs have zero actual_length (It
is a question why).
Then the following end of frame criteria leads to `frame underflow' message:

if (flen < pdev->vlast_packet_size) {
/* Shorter packet... end of frame */
if (pdev->vsync == 2)
pwc_frame_complete(pdev);
if (pdev->fill_buf == NULL)
pdev->fill_buf = pwc_get_next_fill_buf(pdev);
if (pdev->fill_buf) {
pdev->fill_buf->filled = 0;
pdev->vsync = 1;
}
}

2016-08-01 21:16 GMT+03:00 Matwey V. Kornilov :
> pwc module output with trace=511 is the following:
>
> [   24.793109] usbcore: registered new interface driver Philips webcam
> [   29.276979] pwc: Unsupported pixel format
> [   29.277055] pwc: pwc_vidioc_fill_fmt() width=640, height=480,
> bytesperline=640, sizeimage=460800, pixelformat=YU12
> [   29.277090] pwc: Trying to set format to: width=640 height=480
> fps=15 format=YU12
> [   29.277123] pwc: set_video_mode(640x480 @ 30, pixfmt 32315559).
> [   29.277145] pwc: decode_size = 5.
> [   29.277180] pwc: frame_size=63120, vframes=15, vsize=5, vbandlength=526
> [   29.277204] pwc: Set resolution to 640x480
> [   29.277225] pwc: pwc_set_video_mode(), return=0
> [   29.277256] pwc: pwc_vidioc_fill_fmt() width=640, height=480,
> bytesperline=640, sizeimage=460800, pixelformat=YU12
> [   29.277306] pwc: ioctl(VIDIOC_G_FMT) return size 640x480
> [   29.277337] pwc: pwc_vidioc_fill_fmt() width=640, height=480,
> bytesperline=640, sizeimage=460800, pixelformat=YU12
> [   29.277449] pwc: set_video_mode(640x480 @ 10, pixfmt 32315559).
> [   29.277475] pwc: decode_size = 5.
> [   29.278726] pwc: frame_size=94560, vframes=10, vsize=5, vbandlength=788
> [   29.278750] pwc: Set resolution to 640x480
> [   29.300374] pwc: set_video_mode(640x480 @ 10, pixfmt 32315559).
> [   29.300420] pwc: decode_size = 5.
> [   29.441759] pwc: frame_size=94560, vframes=10, vsize=5, vbandlength=788
> [   29.441792] pwc: Set resolution to 640x480
> [   29.441824] pwc: Setting alternate interface 9
> [   29.455061] pwc: Allocated URB at 0xc9b83600
> [   29.455850] pwc: Allocated URB at 0xc9b83400
> [   29.456040] pwc: Allocated URB at 0xc9b83200
> [   29.456271] pwc: URB 0xc9b83600 submitted.
> [   29.456310] pwc: URB 0xc9b83400 submitted.
> [   29.456341] pwc: URB 0xc9b83200 submitted.
> [   29.456362] pwc: << pwc_isoc_init()
> [   30.078550] pwc: Frame buffer underflow (20076 bytes); discarded.
> [   30.170543] pwc: Frame buffer underflow (12428 bytes); discarded.
> [   30.272538] pwc: Frame buffer underflow (14340 bytes); discarded.
> [   30.374541] pwc: Frame buffer underflow (16252 bytes); discarded.
> [   30.476535] pwc: Frame buffer underflow (18164 bytes); discarded.
> [   30.578532] pwc: Frame buffer underflow (20076 bytes); discarded.
> [   30.670538] pwc: Frame buffer underflow (12428 bytes); discarded.
> [   30.772544] pwc: Frame buffer underflow (14340 bytes); discarded.
> [   30.874547] pwc: Frame buffer underflow (16252 bytes); discarded.
> [   30.976552] pwc: Frame buffer underflow (18164 bytes); discarded.
> [   31.078536] pwc: Frame buffer underflow (20076 bytes); discarded.
> [   31.170533] pwc: Frame buffer underflow (12428 bytes); discarded.
> [   31.272549] pwc: Frame buffer underflow (14340 bytes); discarded.
> [   31.374545] pwc: Frame buffer underflow (16252 bytes); discarded.
> [   31.476537] pwc: Frame buffer underflow (18164 bytes); discarded.
> [   31.578536] pwc: Frame buffer underflow (20076 bytes); discarded.
> [   31.660895] pwc: Frame buffer underflow (11472 bytes); discarded.
> [   31.772554] pwc: Frame buffer underflow (14340 bytes); discarded.
> [   31.874548] pwc: Frame buffer underflow (16252 bytes); discarded.
> [   31.976533] pwc: Frame buffer underflow (18164 bytes); discarded.
>
>
> 2016-07-31 23:31 GMT+03:00 Matwey V. Kornilov :
>> Hello,
>>
>> I've also just found that the same commit breaks cpufreq on BeagleBone Black 
>> :)
>>
>> So, probably without HCD_BH flag musb works correctly only at 1Ghz CPU
>> frequency, which is unlisted and being set to 720Mhz by cpufreq driver
>> (as it did whet there was cpufreq driver).
>>
>> 2016-07-29 21:01 GMT+03:00 Matwey V. Kornilov :
>>> Hello,
>>>
>>> I've found that the following commit fixes the issue:
>>>
>>> commit 7694ca6e1d6f01122f05039b81f70f64b1ec4063
>>> Author: Viresh Kumar 
>>> Date:   Fri Apr 22 16:58:42 2016 +0530
>>>
>>> cpufreq: omap: Use generic platdev driver
>>>
>>> The cpufreq-dt-platdev driver supports creation of cpufreq-dt platform
>>> device now, reuse that and remove similar code from platform code.
>>>
>>>
>>> 2016-07-28 19:16 GMT+03:00 Matwey V. Kornilov :
 

Re: [PATCH 2/2] usb: dwc3: core: allow device to runtime_suspend several times

[Felipe Balbi]

Hi,

Alan Stern  writes:

> On Thu, 4 Aug 2016, Felipe Balbi wrote:
>
>> > What are you trying to accomplish?  Is the problem that wakeup signals
>> > only cause the platform device to be runtime-resumed, but you also need
>> > the HCD to wake up?  And conversely, whenever the HCD gets
>> > runtime-suspended you also want the platform device to go into runtime
>> > suspend?
>> 
>> this is DWC3 as peripheral-only. PCI Wakeup (PME) wakes up dwc3-pci, but
>> we need dwc3 core (a platform device) to be resumed as well.
>
> Oh, so I got the types wrong.  But the basic idea is still the same: 
> When the parent wakes up, it does a runtime-resume of the child.  The 
> child is then responsible for making sure it stays awake as long as 
> necessary, and when it goes back to low power the parent naturally does 
> the same.

right. Here's a doubt though: if dwc3-pci does a pm_runtime_resume() of
the child, doesn't that mean child will try to sleep right away again
since its usage_counter will be zero? We _do_ have a 5s timeout which
should be enough to trigger enumeration, but still... I guess I'm
missing something here.

I guess it'll work just fine since we set a driver flag when we see a
bus reset and that gets cleared once a disconnect shows up. Seems like
the whole idea is to avoid an extra pm_runtime_put() in
->runtime_resume()?

What if user sets autosuspend_delay to 0 in sysfs? Seems like this is
likely to break, no?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/2] usb: dwc3: core: allow device to runtime_suspend several times

[Alan Stern]

On Thu, 4 Aug 2016, Felipe Balbi wrote:

> > What are you trying to accomplish?  Is the problem that wakeup signals
> > only cause the platform device to be runtime-resumed, but you also need
> > the HCD to wake up?  And conversely, whenever the HCD gets
> > runtime-suspended you also want the platform device to go into runtime
> > suspend?
> 
> this is DWC3 as peripheral-only. PCI Wakeup (PME) wakes up dwc3-pci, but
> we need dwc3 core (a platform device) to be resumed as well.

Oh, so I got the types wrong.  But the basic idea is still the same: 
When the parent wakes up, it does a runtime-resume of the child.  The 
child is then responsible for making sure it stays awake as long as 
necessary, and when it goes back to low power the parent naturally does 
the same.

> > If that's so, the proper solution is for the platform device's
> > runtime_resume routine to call pm_runtime_resume() for the HCD, and
> > never to do pm_runtime_get_* or pm_runtime_put_* on the platform
> > device.  The HCD's callback routines would then be responsible for
> > doing runtime-PM gets and puts on the HCD as required.
> 
> hmm, interesting approach. Should probably work with my setup as
> well. I'll test that out tomorrow :-)
> 
> Thanks for the hint. In any case, I suppose this would be material for
> v4.9 while $subject is -rc material, agree?

Sure.

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: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Andreas Kemnade]

On Thu, 4 Aug 2016 18:31:29 +0200
Andreas Kemnade  wrote:

> Hi,
> 
> On Thu, 4 Aug 2016 07:29:19 -0700
> Tony Lindgren  wrote:
> 
> > Hi,
> > 
> > * H. Nikolaus Schaller  [160803 10:07]:
> > > All this prevents detection of cable plugin-events and VBUS
> > > measurement and setting OTG_EN before charging is attempted.
> > 
> > So I gave this patch a try but it now blocks all deeper SoC idle
> > states as the PHY stays active. I think the real fix is to make
> > sure the charger behaves independent of the USB PHY state. So
> > probably this needs to be fixed in phy-twl4030-usb.c and
> > twl4030_charger.c instead. Now it sounds like we're also shutting
> > down the charger with the USB PHY.
> > 
> Then there is another power management issue. The patch is not about
> fixing every pm issue in musb. That is not only about charging, it is
> about enabling/disabling() the phy unbalanced:
> Again what happens here without the patch:
> 
> musb will be initialized:
> omap2430_musb_disable()
>calls phy_power_off(), phy will be disabled,
>   phy->power_count goes to -1.
> 
sorry mixed something up.
Nothing happens here, so the previous state of the phy remains.
It would be disabled by the generic phy layer in drivers/phy/phy-core.c

> gadget driver is loaded.
> musb_start() is called
> omap2430_musb_enable() is called
>   calls phy_power_on(),
>   phy->power_count goes to 0,
>   phy is not powered on because power_count != 1
> -> no gadget working, no charging.
> 
... if not configured by u-boot before. USB gadget might work when
initialized earlier in the boot process (u-boot/x-loader/mlo ...)
and phy-twl4030 cannot do anything about it besides if we change
drivers/phy/phy-core.c

Regards,
Andreas
--
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: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Andreas Kemnade]

Hi,

On Thu, 4 Aug 2016 07:29:19 -0700
Tony Lindgren  wrote:

> Hi,
> 
> * H. Nikolaus Schaller  [160803 10:07]:
> > All this prevents detection of cable plugin-events and VBUS
> > measurement and setting OTG_EN before charging is attempted.
> 
> So I gave this patch a try but it now blocks all deeper SoC idle
> states as the PHY stays active. I think the real fix is to make
> sure the charger behaves independent of the USB PHY state. So
> probably this needs to be fixed in phy-twl4030-usb.c and
> twl4030_charger.c instead. Now it sounds like we're also shutting
> down the charger with the USB PHY.
> 
Then there is another power management issue. The patch is not about
fixing every pm issue in musb. That is not only about charging, it is
about enabling/disabling() the phy unbalanced:
Again what happens here without the patch:

musb will be initialized:
omap2430_musb_disable()
   calls phy_power_off(), phy will be disabled,
phy->power_count goes to -1.

gadget driver is loaded.
musb_start() is called
omap2430_musb_enable() is called
calls phy_power_on(),
phy->power_count goes to 0,
phy is not powered on because power_count != 1
-> no gadget working, no charging.

Regards
Andreas
--
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 2/2] usb: dwc3: core: allow device to runtime_suspend several times

[Felipe Balbi]

Hi,

Alan Stern  writes:
> On Wed, 3 Aug 2016, Felipe Balbi wrote:
>
>> >> --- a/drivers/usb/dwc3/core.c
>> >> +++ b/drivers/usb/dwc3/core.c
>> >> @@ -1192,6 +1192,7 @@ static int dwc3_runtime_resume(struct device *dev)
>> >>   }
>> >>  
>> >>   pm_runtime_mark_last_busy(dev);
>> >> + pm_runtime_put(dev);
>> >>  
>> >>   return 0;
>> >>  }
>> >
>> > This may be correct, but it certainly looks odd.  For example, it 
>> > wouldn't work right if you ever called pm_runtime_resume() instead of 
>> > pm_runtime_get().
>> 
>> well, we don't. But is there an alternative for this?
>
> What are you trying to accomplish?  Is the problem that wakeup signals
> only cause the platform device to be runtime-resumed, but you also need
> the HCD to wake up?  And conversely, whenever the HCD gets
> runtime-suspended you also want the platform device to go into runtime
> suspend?

this is DWC3 as peripheral-only. PCI Wakeup (PME) wakes up dwc3-pci, but
we need dwc3 core (a platform device) to be resumed as well.

> If that's so, the proper solution is for the platform device's
> runtime_resume routine to call pm_runtime_resume() for the HCD, and
> never to do pm_runtime_get_* or pm_runtime_put_* on the platform
> device.  The HCD's callback routines would then be responsible for
> doing runtime-PM gets and puts on the HCD as required.

hmm, interesting approach. Should probably work with my setup as
well. I'll test that out tomorrow :-)

Thanks for the hint. In any case, I suppose this would be material for
v4.9 while $subject is -rc material, agree?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] USB: fix invalid memory access in hub_activate()

[Alan Stern]

Viresh, I believe this bug report indicates we need to use the 
alternate approach instead of your "usb: hub: Fix unbalanced reference 
count and memory leak" patch.

On Thu, 4 Aug 2016 mgau...@codeaurora.org wrote:

> On 2015-12-17 00:02, Alan Stern wrote:
> > Commit 8520f38099cc ("USB: change hub initialization sleeps to
> > delayed_work") changed the hub_activate() routine to make part of it
> > run in a workqueue.  However, the commit failed to take a reference to
> > the usb_hub structure or to lock the hub interface while doing so.  As
> > a result, if a hub is plugged in and quickly unplugged before the work
> > routine can run, the routine will try to access memory that has been
> > deallocated.  Or, if the hub is unplugged while the routine is
> > running, the memory may be deallocated while it is in active use.
> > 
> > This patch fixes the problem by taking a reference to the usb_hub at
> > the start of hub_activate() and releasing it at the end (when the work
> > is finished), and by locking the hub interface while the work routine
> > is running.  It also adds a check at the start of the routine to see
> > if the hub has already been disconnected, in which nothing should be
> > done.
> 
 > Could this change result in below mutex lock that I am seeing:
> "kworker/3:2" call stack:
> {code}
> -000|__switch_to(prev = 0xFFC032EAAA00, next = 0xFFC075D59C00)
> -001|context_switch(inline)
> -001|__schedule()
> -002|schedule()
> -003|schedule_preempt_disabled()
> -004|__mutex_lock_common(inline)
> -004|__mutex_lock_slowpath(lock_count = 0xFFC07BE94580)
> -005|current_thread_info(inline)
> -005|mutex_set_owner(inline)
> -005|mutex_lock(
>  |lock = 0xFFC00255EE90 -> (
>  |  count = (counter = -1),
>  |  wait_lock = (rlock = (raw_lock = (owner = 4, next = 4), magic 
> = 3735899821, owner_cpu = 4294967295, owner = 0x)),
>  |  wait_list = (next = 0xFFC0457DBC78, prev = 
> 0xFFC0457DBC78),
>  |  owner = 0xFFC075CD5400,
>  |  name = 0x0,
>  |  magic = 0xFFC00255EE90))
> -006|hub_activate(hub = 0xFFC00253A680, type = 472914991)
> -007|hub_init_func2(?)
> -008|static_key_count(inline)
> -008|static_key_false(inline)
> -008|trace_workqueue_execute_end(inline)
> -008|process_one_work(worker = 0xFFC017C8CA00, work = 
> 0xFFC00253A848)
> -009|worker_thread(__worker = 0xFFC017C8CA00)
> 
> 
> 
> "kworker/u16:0" call stack:
> {code}
> -000|__switch_to(prev = 0xFFC075CD5400, next = 0xFFC02451)
> -001|context_switch(inline)
> -001|__schedule()
> -002|schedule()
> -003|schedule_timeout(timeout = -272798837376)
> -004|do_wait_for_common(inline)
> -004|__wait_for_common(inline)
> -004|wait_for_common(x = 0xFFC075D3F928, timeout = 
> 9223372036854775807, ?)
> -005|wait_for_completion(?)
> -006|flush_work(<-- wq is 
> already blocked for the mutex above
>  |work = 0xFFC00253A848 -> (
>  |  data = (counter = 417),
>  |  entry = (next = 0xFFC00253A850, prev = 
> 0xFFC00253A850),
>  |  func = 0xFFC000659B20 -> ))
>  |  barr = (work = (data = (counter = -272799003163), entry = (next = 
> 0xFFC017C8CA30, prev = 0xFFC017C8CA30), func =
>  |  pool = 0xFFC0703784C0
>  |  pwq = 0xFFC07BE98C00
>  |  linked = 26644480
>  |  __key = ()
> -007|clear_work_data(inline)
> -007|__cancel_work_timer(work = 0xFFC00253A848, is_dwork = FALSE)
> -008|cancel_delayed_work_sync(?)
> -009|hub_quiesce(hub = 0xFFC00253A680, ?)
> -010|hub_disconnect(intf = 0xFFC00255EE00)
> -011|usb_unbind_interface(dev = 0xFFC00255EE30)
> -012|__device_release_driver(dev = 0xCB88537FDC8CAE50)
> -013|device_unlock(inline)
> -013|device_release_driver(dev = 0xFFC00255EE30)
> -014|bus_remove_device(dev = 0xFFC00255EE30)
> -015|device_del(dev = 0xFFC00255EE30)
> -016|usb_disable_device(dev = 0xFFC032C42600, skip_ep0 = 0)
> -017|usb_disconnect(pdev = 0xFFC075D3FCA0)
> -018|usb_remove_hcd(hcd = 0xFFC0700AE880)

Yes, it could lead to this deadlock.

> Should we rather have device_trylock instead of device_lock

No.

> > @@ -1031,10 +1031,20 @@ static void hub_activate(struct usb_hub
> > unsigned delay;
> > 
> > /* Continue a partial initialization */
> > -   if (type == HUB_INIT2)
> > -   goto init2;
> > -   if (type == HUB_INIT3)
> > +   if (type == HUB_INIT2 || type == HUB_INIT3) {
> > +   device_lock(hub->intfdev);
> 
> device_trylock?? If failed to acquire then treat that as disconnect?

No, that's not the right answer.  I believe the correct solution is to
remove the cancel_delayed_work_sync() call in hub_quiesce().

Viresh, do you agree?

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: [PATCH v5] usb: ohci-at91: Forcibly suspend ports while USB suspend

[Alan Stern]

On Thu, 4 Aug 2016, Wenyou Yang wrote:

> The usb controller does not managed correctly the suspend mode for
> the ehci. In echi mode, there is no way to have utmi_suspend_o_n[1]
> suspend without any device connected to it. This is why this specific
> control is added to fix this issue. The suspend mode works in ohci
> mode.

Why are you talking about EHCI mode?  This patch is only for the 
ohci-at91 driver.

> This specific control is by setting the SUSPEND_A/B/C fields of
> SFR_OHCIICR(OHCI Interrupt Configuration Register) in the SFR
> while OHCI USB suspend.
> 
> This setting operation must be done before the USB clock disabled,
> clear them after the USB clock enabled.
> 
> Signed-off-by: Wenyou Yang 
> Reviewed-by: Alexandre Belloni 
> Acked-by: Nicolas Ferre 

I don't know if this is any better than before!  See the comments 
below.

> ---
> 
> Changes in v5:
>  - Use the USB_PORT_FEAT_SUSPEND subcase of the SetPortFeature case
>to take care it.
>  - Update the commit log.
> 
> Changes in v4:
>  - To check whether the SFR node with "atmel,sama5d2-sfr" compatible
>is present or not to decide if this feature is applied or not
>when USB OHCI suspend/resume, instead of new compatible.
>  - Drop the compatible "atmel,sama5d2-ohci".
>  - Drop [PATCH 2/2] ARM: at91/dt: sama5d2: Use new compatible for
>ohci node.
>  - Drop include/soc/at91/at91_sfr.h, move the macro definitions to
>atmel-sfr.h which already exists.
>  - Change the defines to align the exists.
> 
> Changes in v3:
>  - Change the compatible description for more precise.
> 
> Changes in v2:
>  - Add compatible to support forcibly suspend the ports.
>  - Add soc/at91/at91_sfr.h to accommodate the defines.
>  - Add error checking for .sfr_regmap.
>  - Remove unnecessary regmap_read() statement.


> @@ -282,6 +301,28 @@ static int ohci_at91_hub_status_data(struct usb_hcd 
> *hcd, char *buf)
>   return length;
>  }
>  
> +static int ohci_at91_port_ctrl(struct regmap *regmap, u16 port, u8 set)
> +{
> + u32 regval;
> + int ret;
> +
> + if (!regmap)
> + return 0;
> +
> + ret = regmap_read(regmap, AT91_SFR_OHCIICR, );
> + if (ret)
> + return ret;
> +
> + if (set)
> + regval |= AT91_OHCIICR_SUSPEND(port);
> + else
> + regval &= ~AT91_OHCIICR_SUSPEND(port);

In the earlier versions of this patch, you did not use the port number.  
Why has this changed?

How many ports does this controller have?

> +
> + regmap_write(regmap, AT91_SFR_OHCIICR, regval);
> +
> + return 0;
> +}
> +
>  /*
>   * Look at the control requests to the root hub and see if we need to 
> override.
>   */
> @@ -289,6 +330,7 @@ static int ohci_at91_hub_control(struct usb_hcd *hcd, u16 
> typeReq, u16 wValue,
>u16 wIndex, char *buf, u16 wLength)
>  {
>   struct at91_usbh_data *pdata = dev_get_platdata(hcd->self.controller);
> + struct ohci_at91_priv *ohci_at91 = hcd_to_ohci_at91_priv(hcd);
>   struct usb_hub_descriptor *desc;
>   int ret = -EINVAL;
>   u32 *data = (u32 *)buf;
> @@ -301,7 +343,8 @@ static int ohci_at91_hub_control(struct usb_hcd *hcd, u16 
> typeReq, u16 wValue,
>  
>   switch (typeReq) {
>   case SetPortFeature:
> - if (wValue == USB_PORT_FEAT_POWER) {
> + switch (wValue) {
> + case USB_PORT_FEAT_POWER:
>   dev_dbg(hcd->self.controller, "SetPortFeat: POWER\n");
>   if (valid_port(wIndex)) {
>   ohci_at91_usb_set_power(pdata, wIndex, 1);
> @@ -309,6 +352,11 @@ static int ohci_at91_hub_control(struct usb_hcd *hcd, 
> u16 typeReq, u16 wValue,
>   }
>  
>   goto out;
> +
> + case USB_PORT_FEAT_SUSPEND:
> + dev_dbg(hcd->self.controller, "SetPortFeat: SUSPEND\n");
> + ohci_at91_port_ctrl(ohci_at91->sfr_regmap, wIndex, 1);
> + break;
>   }
>   break;
>  
> @@ -342,6 +390,12 @@ static int ohci_at91_hub_control(struct usb_hcd *hcd, 
> u16 typeReq, u16 wValue,
>   ohci_at91_usb_set_power(pdata, wIndex, 0);
>   return 0;
>   }
> + break;
> +
> + case USB_PORT_FEAT_SUSPEND:
> + dev_dbg(hcd->self.controller, "ClearPortFeature: 
> SUSPEND\n");
> + ohci_at91_port_ctrl(ohci_at91->sfr_regmap, wIndex, 0);
> + break;
>   }
>   break;
>   }

Note that after all this, the code goes ahead to call 
ohci_bub_control().

> @@ -587,7 +641,8 @@ ohci_hcd_at91_drv_suspend(struct device *dev)
>   struct usb_hcd  *hcd = dev_get_drvdata(dev);
>   struct ohci_hcd *ohci = hcd_to_ohci(hcd);
>   struct 

Re: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Tony Lindgren]

* H. Nikolaus Schaller  [160804 07:50]:
> > Am 04.08.2016 um 16:29 schrieb Tony Lindgren :
> > 
> > So I gave this patch a try but it now blocks all deeper SoC idle
> > states as the PHY stays active. I think the real fix is to make
> > sure the charger behaves independent of the USB PHY state.
> 
> IMHO, plugin detection of the cable is a phy task and then it tells
> the charger to start. This part works.

OK

> Charging did work up to kernel 4.3. It started to fail with 4.4-rc1
> without obvious changes to the charger but many patches to phy
> and musb. We had even backported the 4.7 charger driver
> to 4.3 and it failed as well.

OK

> > So
> > probably this needs to be fixed in phy-twl4030-usb.c and
> > twl4030_charger.c instead. Now it sounds like we're also shutting
> > down the charger with the USB PHY.
> 
> As a very deeply hidden side-effect the charger is shut down immediately
> after being started. Because phy-twl4030-usb.c does not do what it is expected
> and told to do.
> 
> I have developed a workaround for the charger driver but I do not consider it
> as the solution.
> 
> http://git.goldelico.com/?p=gta04-kernel.git;a=commit;h=b8c538e75c6dd034889bdb0d66e00ca6e128e616
...
> With that we have a workaround in the charger, but not a correct solution.
> That is what Andreas is trying to fix. The charger driver seems to be ok to
> me.

OK. So does the charger work with just phy-twl4030-usb and charger
modules loaded when cable is inserted?

Regards,

Tony
--
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: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[H. Nikolaus Schaller]

Hi Tony,

> Am 04.08.2016 um 16:29 schrieb Tony Lindgren :
> 
> Hi,
> 
> * H. Nikolaus Schaller  [160803 10:07]:
>> All this prevents detection of cable plugin-events and VBUS measurement
>> and setting OTG_EN before charging is attempted.
> 
> So I gave this patch a try but it now blocks all deeper SoC idle
> states as the PHY stays active. I think the real fix is to make
> sure the charger behaves independent of the USB PHY state.

IMHO, plugin detection of the cable is a phy task and then it tells
the charger to start. This part works.

Charging did work up to kernel 4.3. It started to fail with 4.4-rc1
without obvious changes to the charger but many patches to phy
and musb. We had even backported the 4.7 charger driver
to 4.3 and it failed as well.

> So
> probably this needs to be fixed in phy-twl4030-usb.c and
> twl4030_charger.c instead. Now it sounds like we're also shutting
> down the charger with the USB PHY.

As a very deeply hidden side-effect the charger is shut down immediately
after being started. Because phy-twl4030-usb.c does not do what it is expected
and told to do.

I have developed a workaround for the charger driver but I do not consider it
as the solution.

http://git.goldelico.com/?p=gta04-kernel.git;a=commit;h=b8c538e75c6dd034889bdb0d66e00ca6e128e616

> power:twl4030_charger: hack to set POWER_CTRL_OTG_ENAB what twl4030-phy does 
> not do
> 
> This hack is a workaround in the charger driver to do what the  twl4030-usb
> driver is expected to have done. It is designed in a way that it can be
> removed after the twl4030-usb issue is solved, but it does not harm if it
> isn't removed immediately.
> 
> Rationale:
> 
> The charger driver calls pm_runtime_get_sync(bci->transceiver->dev);
> which should indirectly call twl4030_usb_set_mode to set the
> POWER_CTRL_OTG_ENAB bit. This enables the prescaler hardware
> for ADC8 (VBUS) channel. But this does not happen for reasons outside
> the charger driver.
> 
> If this bit is not set there are strange effects:
> * VBUS reports 0mV through MADC
> * the automatic charging stops after some ms
> * ITHEN is disabled automatically and the temperature
>   reports 56°C through MADC
> 
> The TPS65950 TRM says:
> "The prescaler for the ADCIN8 channel is in the On-The-Go (OTG) module of
> the USB subchip. This prescaler is enabled when the OTG is enabled by
> writing 1 to the OTG_EN bit of the POWER_CTRL register of the USB subchip."
> 
> and
> 
> "The software must set the POWER_CTRL[5] OTG_EN bit to 1 at least 50 ms
> before forcing the BCIMFSTS4[2] USBFASTMCHG bit to 1."
> 
> For unknown reasons this does not happen with current phy-twl4030-usb code.
> 
> Therefore we add a hack that ensures that this bit is set and the 50ms
> delay is done.
> 
> It appears as if this problem occurred for the first time in linux 4.4-rc1.


With that we have a workaround in the charger, but not a correct solution.
That is what Andreas is trying to fix. The charger driver seems to be ok to
me.

Hope this helps to better understand what is going wrong in phy4030 or musb.

BR,
Nikolaus

--
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: [Letux-kernel] [PATCH v2] musb: omap2430: do not assume balanced enable()/disable()

[Tony Lindgren]

Hi,

* H. Nikolaus Schaller  [160803 10:07]:
> All this prevents detection of cable plugin-events and VBUS measurement
> and setting OTG_EN before charging is attempted.

So I gave this patch a try but it now blocks all deeper SoC idle
states as the PHY stays active. I think the real fix is to make
sure the charger behaves independent of the USB PHY state. So
probably this needs to be fixed in phy-twl4030-usb.c and
twl4030_charger.c instead. Now it sounds like we're also shutting
down the charger with the USB PHY.

Regards,

Tony
--
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:serial: Add Fintek F81532/534 driver

[Ji-Ze Hong (Peter Hong)]

Hi Alan,

One Thousand Gnomes 於 2016/7/29 下午 08:48 寫道:

O

+static int f81534_set_normal_register(struct usb_device *dev, u16 reg, u8 data)
+{
+   size_t count = F81534_USB_MAX_RETRY;
+   int status;
+   u8 *tmp;
+
+   tmp = kmalloc(sizeof(u8), GFP_KERNEL);
+   if (!tmp)
+   return -ENOMEM;


You end up doing huge numbers of tiny allocation and frees in some of the
code paths. I think it would be better to allocate them at a higher level
as they are not that cheap on CPU time.


+static int f81534_read_data(struct usb_serial *usbserial, u32 address,
+   size_t size, unsigned char *buf)
+{


Is a particularly good example - you do 4 mallocs plus two per byte of
data.



I'll re-factor the newest V9 patch with your suggestion. To malloc a
byte within usb_serial privates, and make a mutex to protect it.

I'll send it as V10 when I tested it.

Thanks for your suggestion.
--
With Best Regards,
Peter Hong
--
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 v4 2/6] power: add power sequence library

[Peter Chen]

On Wed, Aug 03, 2016 at 03:16:58PM -0700, Matthias Kaehlcke wrote:
> El Tue, Aug 02, 2016 at 11:30:48AM +0800 Peter Chen ha dit:
> 
> > diff --git a/drivers/power/pwrseq/core.c b/drivers/power/pwrseq/core.c
> >
> > ...
> >
> > +static DEFINE_MUTEX(pwrseq_list_mutex);
> > +static LIST_HEAD(pwrseq_list);
> > +
> > +int pwrseq_get(struct device_node *np, struct pwrseq *p)
> > +{
> > +   if (p && p->get)
> > +   return p->get(np, p);
> > +
> > +   return -ENOTSUPP;
> > +}
> > +
> > +int pwrseq_on(struct device_node *np, struct pwrseq *p)
> > +{
> > +   if (p && p->on)
> > +   return p->on(np, p);
> > +
> > +   return -ENOTSUPP;
> > +}
> > +
> > +void pwrseq_off(struct pwrseq *p)
> > +{
> > +   if (p && p->off)
> > +   p->off(p);
> > +}
> > +
> > +void pwrseq_put(struct pwrseq *p)
> > +{
> > +   if (p && p->put)
> > +   p->put(p);
> > +}
> > +
> > +void pwrseq_free(struct pwrseq *p)
> > +{
> > +   if (p && p->free)
> > +   p->free(p);
> > +}
> > +
> > +void pwrseq_register(struct pwrseq *pwrseq)
> > +{
> > +   mutex_lock(_list_mutex);
> > +   list_add(>node, _list);
> > +   mutex_unlock(_list_mutex);
> > +}
> > +
> > +void pwrseq_unregister(struct pwrseq *pwrseq)
> > +{
> > +   mutex_lock(_list_mutex);
> > +   list_del(>node);
> > +   mutex_unlock(_list_mutex);
> > +}
> 
> What is the purpose of pwrseq_register/unregister()? The pwrseq
> structs are added and removed from pwrseq_list, but besides that
> pwrseq_list is not used.
> 

I had thought we may need to dump the pwrseq list in future for debug
purpose. I will delete this unnecessary code.

-- 

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