Re: Error reading USB camera in BeagleBone with ARM Debian
On Thu, 18 Oct 2018, Josep M. Mirats Tur wrote: > Hi > > I've recently acquired a BeagleBone Green with AM335x processor. I 'm > connecting a USB device (actually a 3D camera from ORBBEC) to the > Beagle USB host port. It recognizes well as I can see at the dmesg > output: > > [12411.643517] usb 1-1: new high-speed USB device number 2 using musb-hdrc > [12411.784848] usb 1-1: New USB device found, idVendor=2bc5, idProduct=0404 > [12411.784912] usb 1-1: New USB device strings: Mfr=1, Product=2, > SerialNumber=0 > [12411.784951] usb 1-1: Product: ORBBEC Depth Sensor > [12411.784986] usb 1-1: Manufacturer: Orbbec(R) > > However, when I try to read data from the camera using the OpenNi SDK > that ORBBEC provides for Arm devices, although I can connect to the > camera (in fact it toggles on the IR) I get a timeout and get no data > ever. The 'dmesg' output show the errors: > > [12446.755020] usb 1-1: usbfs: usb_submit_urb returned -121 > > Other users seemed to have this problem in the past, and it seems > related to the EHCI driver itself, The log message above indicates that the BeagleBone's USB host controller uses the musb-hdrc driver. This means it is not EHCI. > but I've not been able to find a > solution and need help urgently to solve this issue. > > The system version in my BeagleBoneGreen is Debian Debian 8.3 > (4.1.15-ti-rt-r43). In fact I've tested several versions including the > newest one available for BeagleBone (the debian 9.5 - 4.14.71-ti-r80), > but without success. In this case I got a different error from dmesg: > > Oct 17 16:59:29 arm kernel: [ 215.611056] musb_host_rx 1965: Rx > interrupt with no errors or packet! > Oct 17 16:59:29 arm kernel: [ 215.617649] musb_host_rx 1965: Rx > interrupt with no errors or packet! > Oct 17 16:59:29 arm kernel: [ 215.625557] musb_ep_program 916: broken > !rx_reinit, ep2 csr 0003 > > > Please I need some clues about this. First question: Does you get similar failures if you plug the device into a standard PC rather than the BeagleBone? Second question: Have you tried looking at usbmon traces to see where the communication starts to fail? > Should I load different usb modules in the system kernel? Should I > modify a given module? It certainly looks like a problem in the musb-hdrc driver. You might try asking the maintainer for that driver. Alan Stern
Error reading USB camera in BeagleBone with ARM Debian
Hi I've recently acquired a BeagleBone Green with AM335x processor. I 'm connecting a USB device (actually a 3D camera from ORBBEC) to the Beagle USB host port. It recognizes well as I can see at the dmesg output: [12411.643517] usb 1-1: new high-speed USB device number 2 using musb-hdrc [12411.784848] usb 1-1: New USB device found, idVendor=2bc5, idProduct=0404 [12411.784912] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0 [12411.784951] usb 1-1: Product: ORBBEC Depth Sensor [12411.784986] usb 1-1: Manufacturer: Orbbec(R) However, when I try to read data from the camera using the OpenNi SDK that ORBBEC provides for Arm devices, although I can connect to the camera (in fact it toggles on the IR) I get a timeout and get no data ever. The 'dmesg' output show the errors: [12446.755020] usb 1-1: usbfs: usb_submit_urb returned -121 Other users seemed to have this problem in the past, and it seems related to the EHCI driver itself, but I've not been able to find a solution and need help urgently to solve this issue. The system version in my BeagleBoneGreen is Debian Debian 8.3 (4.1.15-ti-rt-r43). In fact I've tested several versions including the newest one available for BeagleBone (the debian 9.5 - 4.14.71-ti-r80), but without success. In this case I got a different error from dmesg: Oct 17 16:59:29 arm kernel: [ 215.611056] musb_host_rx 1965: Rx interrupt with no errors or packet! Oct 17 16:59:29 arm kernel: [ 215.617649] musb_host_rx 1965: Rx interrupt with no errors or packet! Oct 17 16:59:29 arm kernel: [ 215.625557] musb_ep_program 916: broken !rx_reinit, ep2 csr 0003 Please I need some clues about this. Should I load different usb modules in the system kernel? Should I modify a given module? Thanks Josep M.
Re: Query on usb/core/devio.c
On Thu, 18 Oct 2018, Mayuresh Kulkarni wrote: > > The only way to make the ioctl work properly is to have it do a > > runtime-PM put at the start and then a runtime-PM get before it > > returns. This is true regardless of the reason for returning: normal > > termination, timeout, signal, whatever. Nothing else would be safe. > > > > Will below steps work safely (sometimes pseudo-code/snippets help to align)? - > > "new" ioctl - > > timeout is parameter to ioctl. > > /* attempt suspend of device */ > usb_autosuspend_device(dev); > > usb_unlock_device(dev); > r = wait_event_interruptible_timeout(ps->resume_wait, > (ps->resume_done == true), timeout * HZ); Not exactly. The condition to test here is whether the device has been suspended, so it's more like this: r = wait_event_interruptible_timeout(ps->suspend_wait, (ps->suspend_done == true), timeout * HZ); where ps->suspend_done is set by the runtime_suspend callback. After this we will do: if (r > 0) /* Device suspended before the timeout expired */ r = wait_event_interruptible(ps->resume_wait, (ps->resume_done == true)); > usb_lock_device(dev); > > /* > * There are 3 possibilities here: > * 1. Device did suspend and resume (success) > * 2. Signal was received (failed suspend) > * 3. Time-out happened (failed suspend) 4. Device did suspend but a signal was received before the device resumed. > * In any of above cases, we need to resume device. > */ > usb_autoresume_device(dev); > > ps->resume_done = false; > > "ps->resume_done = true;" will be done by the runtime resume call-back. Exactly. You got it. Will that let you accomplish what you want? Alan Stern
Re: Query on usb/core/devio.c
On Thu, 18 Oct 2018, Mayuresh Kulkarni wrote: > As I understand, you seem to mention - user-space to tell the USB > device when to remote-wake. > But in our case, we cannot tell the device when to remote-wake as the > USB device needs to detect an "activity" by end user (after link goes > into L2), which can happen at anytime. That's not how it works. The userspace program doesn't tell the device anything, as far as suspend and wakeup are concerned. It just tells the kernel that it has stopped using the device. When nothing is using the device, the kernel tells the device to go into suspend. At the same time, the kernel tells the device to enable remote wakeup. Thus, the device will send a wakeup request if any activity occurs while it is suspended. > I think the approach we are discussing here seems to hint the > USB-core to attempt suspend from user-space's point-of-view, if there > is no other outstanding PM ref-count, but still have ability for > user-space to know if suspend/resume happened or not. Not quite; in the scheme I described, the userspace program would not know whether the device went into suspend. But perhaps that information could be encoded into the ioctl's return value. For example, the ioctl could return 1 if the timeout expired or some other signal arrived before the device went into suspend, or 2 if the device did go into suspend and the call was interrupted by a signal, or 0 if the call was terminated by the device resuming after a suspend. In the end, though, do you really care whether the device suspended? Alan Stern
Re: [PATCH] HID: hiddev: fix potential Spectre v1
On Thu, Oct 18, 2018 at 01:50:26PM -0300, Breno Leitao wrote:
> Hi Gustavo,
>
> On 10/17/2018 05:30 PM, Gustavo A. R. Silva wrote:
> >
> > Hi Breno,
> >
> > On 10/17/18 9:47 PM, Breno Leitao wrote:
> >> uref->usage_index can be indirectly controlled by userspace, hence leading
> >> to a potential exploitation of the Spectre variant 1 vulnerability.
> >>
> >> This problem might show up in the cmd = HIDIOCGCOLLECTIONINDEX flow at
> >> function
> >> hiddev_ioctl_usage(), where uref->usage_index is compared to
> >> field->maxusage
> >> and then used as an index to dereference field->usage array.
> >>
> >> This is a summary of the current flow, which matches the traditional
> >> Spectre V1 issue:
> >>
> >>copy_from_user(uref, user_arg, sizeof(*uref))
> >>if (uref->usage_index >= field->maxusage)
> >>goto inval;
> >>i = field->usage[uref->usage_index].collection_index;
> >>return i;
> >>
> >> This patch fixes this by sanitizing field uref->usage_index before using
> >> it to
> >> index field->usage, thus, avoiding speculation in the first load.
> >>
> >> Signed-off-by: Breno Leitao
> >> ---
> >> drivers/hid/usbhid/hiddev.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> >> index 23872d08308c..8829cbc1f6b1 100644
> >> --- a/drivers/hid/usbhid/hiddev.c
> >> +++ b/drivers/hid/usbhid/hiddev.c
> >> @@ -512,6 +512,9 @@ static noinline int hiddev_ioctl_usage(struct hiddev
> >> *hiddev, unsigned int cmd,
> >>if (cmd == HIDIOCGCOLLECTIONINDEX) {
> >>if (uref->usage_index >= field->maxusage)
> >>goto inval;
> >> + uref->usage_index =
> >> + array_index_nospec(uref->usage_index,
> >> + field->maxusage);
> >
> > Good catch.
> >
> >>} else if (uref->usage_index >= field->report_count)
> >>goto inval;
> >
> > Although, notice that this is the same index, and it can be used to index
> > field->value[]
> > at lines 526 and 532.
>
> Right, this seems to be a possible problem also, when 'cmd' =
> HIDIOC{G,S}USAGES.
>
> I am reworking the patch to cover both issues. What do you think of the draft
> below?
>
> Thank you for reviewing it!
>
> ---
>
> Subject: [PATCH] HID: hiddev: fix potential Spectre v1
>
> uref->usage_index can be indirectly controlled by userspace, hence leading
> to a potential exploitation of the Spectre variant 1 vulnerability.
>
> This field is used as an array index by the hiddev_ioctl_usage() function,
> when 'cmd' is HIDIOCGCOLLECTIONINDEX, HIDIOCGUSAGES or HIDIOCSUSAGES.
>
> For cmd == HIDIOCGCOLLECTIONINDEX case, uref->usage_index is compared to
> field->maxusage and then used as an index to dereference field->usage
> array. The very same thing happens to the cmd == HIDIOC{G,S}USAGES cases,
> where uref->usage_index is checked against an array maximum value and then
> it is used as an index in this array.
>
> This is a summary of the HIDIOCGCOLLECTIONINDEX case, which matches the
> traditional Spectre V1 first load:
>
> copy_from_user(uref, user_arg, sizeof(*uref))
> if (uref->usage_index >= field->maxusage)
> goto inval;
> i = field->usage[uref->usage_index].collection_index;
> return i;
>
> This patch fixes this by sanitizing field uref->usage_index before using it
> to index field->usage, thus, avoiding speculation in the first load.
>
> Signed-off-by: Breno Leitao
> ---
> drivers/hid/usbhid/hiddev.c | 18 ++
> 1 file changed, 14 insertions(+), 4 deletions(-)
Care to cc: stable as well?
thanks,
greg k-h
Re: [PATCH] HID: hiddev: fix potential Spectre v1
Hi Gustavo,
On 10/17/2018 05:30 PM, Gustavo A. R. Silva wrote:
>
> Hi Breno,
>
> On 10/17/18 9:47 PM, Breno Leitao wrote:
>> uref->usage_index can be indirectly controlled by userspace, hence leading
>> to a potential exploitation of the Spectre variant 1 vulnerability.
>>
>> This problem might show up in the cmd = HIDIOCGCOLLECTIONINDEX flow at
>> function
>> hiddev_ioctl_usage(), where uref->usage_index is compared to field->maxusage
>> and then used as an index to dereference field->usage array.
>>
>> This is a summary of the current flow, which matches the traditional
>> Spectre V1 issue:
>>
>> copy_from_user(uref, user_arg, sizeof(*uref))
>> if (uref->usage_index >= field->maxusage)
>> goto inval;
>> i = field->usage[uref->usage_index].collection_index;
>> return i;
>>
>> This patch fixes this by sanitizing field uref->usage_index before using it
>> to
>> index field->usage, thus, avoiding speculation in the first load.
>>
>> Signed-off-by: Breno Leitao
>> ---
>> drivers/hid/usbhid/hiddev.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
>> index 23872d08308c..8829cbc1f6b1 100644
>> --- a/drivers/hid/usbhid/hiddev.c
>> +++ b/drivers/hid/usbhid/hiddev.c
>> @@ -512,6 +512,9 @@ static noinline int hiddev_ioctl_usage(struct hiddev
>> *hiddev, unsigned int cmd,
>> if (cmd == HIDIOCGCOLLECTIONINDEX) {
>> if (uref->usage_index >= field->maxusage)
>> goto inval;
>> +uref->usage_index =
>> +array_index_nospec(uref->usage_index,
>> + field->maxusage);
>
> Good catch.
>
>> } else if (uref->usage_index >= field->report_count)
>> goto inval;
>
> Although, notice that this is the same index, and it can be used to index
> field->value[]
> at lines 526 and 532.
Right, this seems to be a possible problem also, when 'cmd' = HIDIOC{G,S}USAGES.
I am reworking the patch to cover both issues. What do you think of the draft
below?
Thank you for reviewing it!
---
Subject: [PATCH] HID: hiddev: fix potential Spectre v1
uref->usage_index can be indirectly controlled by userspace, hence leading
to a potential exploitation of the Spectre variant 1 vulnerability.
This field is used as an array index by the hiddev_ioctl_usage() function,
when 'cmd' is HIDIOCGCOLLECTIONINDEX, HIDIOCGUSAGES or HIDIOCSUSAGES.
For cmd == HIDIOCGCOLLECTIONINDEX case, uref->usage_index is compared to
field->maxusage and then used as an index to dereference field->usage
array. The very same thing happens to the cmd == HIDIOC{G,S}USAGES cases,
where uref->usage_index is checked against an array maximum value and then
it is used as an index in this array.
This is a summary of the HIDIOCGCOLLECTIONINDEX case, which matches the
traditional Spectre V1 first load:
copy_from_user(uref, user_arg, sizeof(*uref))
if (uref->usage_index >= field->maxusage)
goto inval;
i = field->usage[uref->usage_index].collection_index;
return i;
This patch fixes this by sanitizing field uref->usage_index before using it
to index field->usage, thus, avoiding speculation in the first load.
Signed-off-by: Breno Leitao
---
drivers/hid/usbhid/hiddev.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 23872d08308c..053f93bdee72 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -512,14 +512,24 @@ static noinline int hiddev_ioctl_usage(struct hiddev
*hiddev, unsigned int cmd,
if (cmd == HIDIOCGCOLLECTIONINDEX) {
if (uref->usage_index >= field->maxusage)
goto inval;
+ uref->usage_index =
+ array_index_nospec(uref->usage_index,
+ field->maxusage);
} else if (uref->usage_index >= field->report_count)
goto inval;
}
- if ((cmd == HIDIOCGUSAGES || cmd == HIDIOCSUSAGES) &&
- (uref_multi->num_values > HID_MAX_MULTI_USAGES ||
-uref->usage_index + uref_multi->num_values >
field->report_count))
- goto inval;
+ if (cmd == HIDIOCGUSAGES || cmd == HIDIOCSUSAGES) {
+ if (uref_multi->num_values > HID_MAX_MULTI_USAGES ||
+ uref->usage_index + uref_multi->num_values >
+field->report_count)
+ goto inval;
+
+ uref->usage_index =
+
Re: Query on usb/core/devio.c
On Wed, 17 Oct 2018 10:32:14 -0400 Alan Stern wrote: > On Wed, 17 Oct 2018, Oliver Neukum wrote: > > > On Di, 2018-10-16 at 10:46 -0400, Alan Stern wrote: > > > On Tue, 16 Oct 2018, Mayuresh Kulkarni wrote: > > > > > > > 1. User space decides when it is time to suspend the device and calls > > > > this ioctl. The implmentation takes care to ensure, no URB is in > > > > flight from this caller to this device. Then proceed with suspend. > > > > > > Well, I did not imagine the ioctl would try to ensure that no URBs are > > > in flight. The user would be responsible for that. > > > > Well, we have to check for URBs in flight. In fact we would have to > > "plug" the device against new URBs. Now, we could use a new counter. > > But if we check asynclist, we may just as well walk it and kill the > > URBs. It just seems a bit silly not to do that. > > In fact, the USB core already flushes outstanding URBs when a device > gets suspended. > > I don't know that "plugging" is needed. We could simply let new URBs > fail (the core prevents URBs from being sent to a suspended device). > > > > Also, proceeding with the suspend is difficult, as Oliver has pointed > > > out. There is no guarantee that the device will go into suspend, > > > merely because the userspace driver isn't accessing it. (In fact, > > > there's no guarantee that the device will _ever_ go into suspend.) The > > > ioctl would probably have to include some sort of timeout; it would > > > return early if the timeout expired before the device was suspended. > > > > Exactly. The alternative is to have an ioctl() to just allow or block > > suspend, more or less just exporting autopm_get/put(). The disadvantage > > is that > > > > 1. The kernel has to cancel URBs in flight > > 2. There needs to be a mechanism to notify user space about events > > > > > > 2. In an another thread, the user-space calls poll(USB-device-fd). > > > > When poll() returns, that means the device is active now. User space > > > > can then request an URB to read out the async notification, if any. > > > > > > No; no other thread or polling is needed. The ioctl call returns when > > > the device resumes. At that point the user program can check whether > > > there is an async notification pending. > > > > This is problematic. For example, what do we do if a signal needs > > to be delivered? The obvious solution of just repeating the call will > > not work. It races with wakeup. You'd need to restart IO to query > > the device. But the device may be suspended. Or do you want a signal > > to up the PM counter again? > > The only way to make the ioctl work properly is to have it do a > runtime-PM put at the start and then a runtime-PM get before it > returns. This is true regardless of the reason for returning: normal > termination, timeout, signal, whatever. Nothing else would be safe. > Will below steps work safely (sometimes pseudo-code/snippets help to align)? - "new" ioctl - timeout is parameter to ioctl. /* attempt suspend of device */ usb_autosuspend_device(dev); usb_unlock_device(dev); r = wait_event_interruptible_timeout(ps->resume_wait, (ps->resume_done == true), timeout * HZ); usb_lock_device(dev); /* * There are 3 possibilities here: * 1. Device did suspend and resume (success) * 2. Signal was received (failed suspend) * 3. Time-out happened (failed suspend) * In any of above cases, we need to resume device. */ usb_autoresume_device(dev); ps->resume_done = false; "ps->resume_done = true;" will be done by the runtime resume call-back. > > Making Ctrl-C wake up an unrelated device? > > I don't follow. > > > What do we do in case of a failed suspend or resume? > > The ioctl returns when the usbfs runtime-resume callback is invoked. > This may or may not happen after a failed suspend. But if the device > doesn't go into suspend within the ioctl's timeout period, the ioctl > will return in any case. > > In case of a failed resume, what _can_ we do? > > > How about reset_resume? > > Indeed, what happens if a device in use by usbfs gets reset even while > it isn't suspended? We should do the same thing as much as possible, > regardless of the device's state when the reset occurred. > > Alan Stern
Re: Query on usb/core/devio.c
On Tue, 16 Oct 2018 10:35:47 -0400 Alan Stern wrote: > On Tue, 16 Oct 2018, Mayuresh Kulkarni wrote: > > > > How about instead having a mechanism whereby your usrspace driver can > > > tell when the device does a remote wakeup? At that point it could > > > submit an URB via usbfs and receive the async notification. > > > > > > Alan Stern > > > > Unfortunately, that is not possible in our use-case. It is the composite > > USB device that needs to detect an activity and convey it to host for > > taking appropriate action. > > Sorry, I don't understand. Isn't that exactly the same as what I wrote > above? > > Alan Stern As I understand, you seem to mention - user-space to tell the USB device when to remote-wake. But in our case, we cannot tell the device when to remote-wake as the USB device needs to detect an "activity" by end user (after link goes into L2), which can happen at anytime. I think the approach we are discussing here seems to hint the USB-core to attempt suspend from user-space's point-of-view, if there is no other outstanding PM ref-count, but still have ability for user-space to know if suspend/resume happened or not. Apologies if my comment was not clear enough. Please do correct, if I have misunderstood something. > > > But since the activity depends on end-user, it can happen at anytime. We do > > not want to keep the link active, just to detect this, which is not good > > from battery life point of view.
Re: [GIT PULL] USB-serial updates for v4.20-rc1
On Thu, Oct 18, 2018 at 12:55:42PM +0200, Johan Hovold wrote: > The following changes since commit 7876320f88802b22d4e2daf7eb027dd14175a0f8: > > Linux 4.19-rc4 (2018-09-16 11:52:37 -0700) > > are available in the Git repository at: > > https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git > tags/usb-serial-4.20-rc1 > > for you to fetch changes up to 17c42e34997ae172c794f84fefe47f00bec13f9a: > > USB: serial: cypress_m8: remove set but not used variable 'iflag' > (2018-10-05 08:58:42 +0200) Now merged, thanks greg k-h
[GIT PULL] USB-serial updates for v4.20-rc1
The following changes since commit 7876320f88802b22d4e2daf7eb027dd14175a0f8: Linux 4.19-rc4 (2018-09-16 11:52:37 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git tags/usb-serial-4.20-rc1 for you to fetch changes up to 17c42e34997ae172c794f84fefe47f00bec13f9a: USB: serial: cypress_m8: remove set but not used variable 'iflag' (2018-10-05 08:58:42 +0200) USB-serial updates for v4.20-rc1 Here are the USB-serial updates for 4.20-rc1, including: - support for CBUS GPIO on FTDI devices (FTX and FT232R) - fix of a long-standing transfer-length bug Included are also various clean ups. All have been in linux-next with no reported issues. Signed-off-by: Johan Hovold Colin Ian King (1): USB: serial: cypress_m8: fix spelling mistake "retreiving" -> "retrieving" Johan Hovold (3): USB: serial: ftdi_sio: fix gpio name collisions USB: serial: ftdi_sio: add support for FT232R CBUS gpios USB: serial: cypress_m8: fix interrupt-out transfer length Karoly Pados (1): USB: serial: ftdi_sio: implement GPIO support for FT-X devices YueHaibing (1): USB: serial: cypress_m8: remove set but not used variable 'iflag' drivers/usb/serial/cypress_m8.c | 7 +- drivers/usb/serial/ftdi_sio.c | 391 +++- drivers/usb/serial/ftdi_sio.h | 28 ++- 3 files changed, 420 insertions(+), 6 deletions(-)
RE: [PATCH 0/4] usb: chipidea: imx: add HSIC support
> >> My board is currently off-tree so I can't send any patch for the > >> pinmux settings in devicetree. But I will send a patch with the > >> changes that should go to imx6qdl.dtsi, imx6sl.dtsi and imx6sx.dtsi. > >> Though I > only tested on i.MX6S. > >> > > > > No, the changes are board level specific, you could not add it into SoC > > file. > > Not all changes are board specific. The HSIC-only host controllers can only > be used > to interface with on-board chips via HSIC. So they need a "usb-nop-xceiv" > dummy > PHY in all use cases. > > So if I understand this correctly this should be done in the SoCs dtsi files > like in this > patch I just sent: [1]. > > Please also note that for i.MX7S this is already added in imx7s.dtsi: [2] > Oh, I have misunderstood your meaning, the [1] is needed since it is for SoC. I thought your board level changes (like pinctrl) would go into SoC dtsi file. If there are no more comments, I will send v2 next week, feel free to add your reviewed-by and tested-by tag, thanks. Peter > > [1]: > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2 > Flkml%2F2018%2F10%2F18%2F414data=02%7C01%7Cpeter.chen%40nxp. > com%7C3c73fdf4ec98477da49308d634d78b39%7C686ea1d3bc2b4c6fa92cd99c5c > 301635%7C0%7C0%7C636754497687793991sdata=aHhMhaj%2FbdTFu0B > qXi0loiBh%2BHzl%2F%2BTSw3txyjBSNLQ%3Dreserved=0 > [2]: > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com > %2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Farch%2Farm%2Fboot%2Fdts%2Fim > x7s.dtsi%23L88data=02%7C01%7Cpeter.chen%40nxp.com%7C3c73fdf4ec9 > 8477da49308d634d78b39%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0% > 7C636754497687793991sdata=7cVhAMQpWWYjGHjY8Xh2g0CG3jPaeG8X > 9K368AooQcc%3Dreserved=0
Re: [PATCH 0/4] usb: chipidea: imx: add HSIC support
On 18.10.18 10:48, Peter Chen wrote: Thanks, Frieder, no more tests are needed. You could send me your dts changes as patches, I will append it at my v2 patch series. My board is currently off-tree so I can't send any patch for the pinmux settings in devicetree. But I will send a patch with the changes that should go to imx6qdl.dtsi, imx6sl.dtsi and imx6sx.dtsi. Though I only tested on i.MX6S. No, the changes are board level specific, you could not add it into SoC file. Not all changes are board specific. The HSIC-only host controllers can only be used to interface with on-board chips via HSIC. So they need a "usb-nop-xceiv" dummy PHY in all use cases. So if I understand this correctly this should be done in the SoCs dtsi files like in this patch I just sent: [1]. Please also note that for i.MX7S this is already added in imx7s.dtsi: [2] Thanks, Frieder [1]: https://lkml.org/lkml/2018/10/18/414 [2]: https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/imx7s.dtsi#L88
RE: [PATCH 0/4] usb: chipidea: imx: add HSIC support
> > > > Thanks, Frieder, no more tests are needed. > > You could send me your dts changes as patches, I will append it at my v2 > > patch > series. > > My board is currently off-tree so I can't send any patch for the pinmux > settings in > devicetree. But I will send a patch with the changes that should go to > imx6qdl.dtsi, > imx6sl.dtsi and imx6sx.dtsi. Though I only tested on i.MX6S. > No, the changes are board level specific, you could not add it into SoC file. Peter
[PATCH 1/2] usb: phy: ab8500: silence some uninitialized variable warnings
Smatch complains that "reg" can be uninitialized if the
abx500_get_register_interruptible() call fails. It's an interruptable
function, so we should check if the user presses CTRL-C.
Signed-off-by: Dan Carpenter
---
drivers/usb/phy/phy-ab8500-usb.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/phy/phy-ab8500-usb.c b/drivers/usb/phy/phy-ab8500-usb.c
index 66143ab8c043..aaf363f19714 100644
--- a/drivers/usb/phy/phy-ab8500-usb.c
+++ b/drivers/usb/phy/phy-ab8500-usb.c
@@ -505,15 +505,19 @@ static int abx500_usb_link_status_update(struct
ab8500_usb *ab)
if (is_ab8500(ab->ab8500)) {
enum ab8500_usb_link_status lsts;
- abx500_get_register_interruptible(ab->dev,
+ ret = abx500_get_register_interruptible(ab->dev,
AB8500_USB, AB8500_USB_LINE_STAT_REG, );
+ if (ret < 0)
+ return ret;
lsts = (reg >> 3) & 0x0F;
ret = ab8500_usb_link_status_update(ab, lsts);
} else if (is_ab8505(ab->ab8500)) {
enum ab8505_usb_link_status lsts;
- abx500_get_register_interruptible(ab->dev,
+ ret = abx500_get_register_interruptible(ab->dev,
AB8500_USB, AB8505_USB_LINE_STAT_REG, );
+ if (ret < 0)
+ return ret;
lsts = (reg >> 3) & 0x1F;
ret = ab8505_usb_link_status_update(ab, lsts);
}
--
2.11.0
Re: [PATCH] usb: dwc2: pci: Fix an error code in probe
On 10/18/2018 11:37 AM, Dan Carpenter wrote:
> We added some error handling to this function but forgot to set the
> error code on this path.
>
> Fixes: ecd29dabb2ba ("usb: dwc2: pci: Handle error cleanup in probe")
> Signed-off-by: Dan Carpenter
Acked-by: Minas Harutyunyan
> ---
> drivers/usb/dwc2/pci.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/dwc2/pci.c b/drivers/usb/dwc2/pci.c
> index d257c541e51b..7afc10872f1f 100644
> --- a/drivers/usb/dwc2/pci.c
> +++ b/drivers/usb/dwc2/pci.c
> @@ -120,6 +120,7 @@ static int dwc2_pci_probe(struct pci_dev *pci,
> dwc2 = platform_device_alloc("dwc2", PLATFORM_DEVID_AUTO);
> if (!dwc2) {
> dev_err(dev, "couldn't allocate dwc2 device\n");
> + ret = -ENOMEM;
> goto err;
> }
>
>
Re: [PATCH 0/4] usb: chipidea: imx: add HSIC support
On 18.10.18 03:22, Peter Chen wrote: - System suspend/resume 1. Enable USB wakeup for i in $(find /sys -name wakeup | grep usb);do echo enabled > $i;echo "echo enabled > $i";done; 2. Let the system enter suspend using below command echo mem > /sys/power/state 3. And see if there is a wakeup block system entering suspend, and check if USB HSIC works ok after system resume System suspend/resume seems to work fine. After resume the ethernet controller works. root@imxceet-solo-s-43:~# echo mem > /sys/power/state PM: suspend entry (deep) PM: Syncing filesystems ... done. Freezing user space processes ... (elapsed 0.001 seconds) done. OOM killer disabled. Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. smsc95xx 3-1:1.0 eth1: entering SUSPEND2 mode PM: suspend devices took 0.050 seconds Disabling non-boot CPUs ... usb 3-1: reset high-speed USB device number 2 using ci_hdrc PM: resume devices took 0.590 seconds OOM killer enabled. Restarting tasks ... done. PM: suspend exit smsc95xx 3-1:1.0 eth1: link up, 100Mbps, full-duplex, lpa 0x4DE1 fec 2188000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx - Runtime suspend 1. Enable auto suspend for all USB devices, and check if USBOH3 clock is closed, make sure do not plug any ethernet cable on the RJ45 port. /* Enable auto suspend */ for i in $(find /sys -name control | grep usb);do echo auto > $i;echo "echo auto > $i";done; This doesn't work. When the port is suspended it gets into a loop of suspending/resuming endlessly. If i put two logs in ci_hdrc_imx_runtime_suspend() and ci_hdrc_imx_runtime_resume(), I get this: [...] ci_hdrc_imx_runtime_resume:603 ci_hdrc_imx_runtime_suspend:574 ci_hdrc_imx_runtime_resume:603 ci_hdrc_imx_runtime_suspend:574 ci_hdrc_imx_runtime_resume:603 ci_hdrc_imx_runtime_suspend:574 ci_hdrc_imx_runtime_resume:603 ci_hdrc_imx_runtime_suspend:574 [...] Ok, forget about the loop, this was caused by one of the other ports, that had issues with overcurrent detection. But still it doesn't work for the HSIC port. The HSIC device is stuck in status "suspending" (note: "suspending" and not "suspended"): ~# cat /sys/bus/usb/devices/usb1/power/runtime_status suspended ~# cat /sys/bus/usb/devices/usb2/power/runtime_status suspended ~# cat /sys/bus/usb/devices/usb3/power/runtime_status active ~# cat /sys/bus/usb/devices/usb3/3-1/power/runtime_status suspending It seems like this is a problem with the smsc95xx driver. I fixed this and now the device is suspending properly, when the network interface is down and resumes when it comes up. I also checked the USBOH3 clock and its properly switched on and off. What didn't work is auto suspend and resume on ethernet link down/up, but this seems to be a restriction of the smsc95xx/usbnet driver. So all in all this looks good to me. Please let me know if you need any more information or tests. Thanks, Frieder, no more tests are needed. You could send me your dts changes as patches, I will append it at my v2 patch series. My board is currently off-tree so I can't send any patch for the pinmux settings in devicetree. But I will send a patch with the changes that should go to imx6qdl.dtsi, imx6sl.dtsi and imx6sx.dtsi. Though I only tested on i.MX6S. Thanks, Frieder
[PATCH] usb: dwc2: pci: Fix an error code in probe
We added some error handling to this function but forgot to set the
error code on this path.
Fixes: ecd29dabb2ba ("usb: dwc2: pci: Handle error cleanup in probe")
Signed-off-by: Dan Carpenter
---
drivers/usb/dwc2/pci.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/dwc2/pci.c b/drivers/usb/dwc2/pci.c
index d257c541e51b..7afc10872f1f 100644
--- a/drivers/usb/dwc2/pci.c
+++ b/drivers/usb/dwc2/pci.c
@@ -120,6 +120,7 @@ static int dwc2_pci_probe(struct pci_dev *pci,
dwc2 = platform_device_alloc("dwc2", PLATFORM_DEVID_AUTO);
if (!dwc2) {
dev_err(dev, "couldn't allocate dwc2 device\n");
+ ret = -ENOMEM;
goto err;
}
--
2.11.0
