Re: [PATCH] usb: dwc2: Fix HiKey regression caused by power_down feature
Hi John, On 9/25/2018 21:59, John Stultz wrote: > On Tue, Sep 25, 2018 at 3:04 AM, Artur Petrosyan > wrote: >> Just a clarification by this commit "[PATCH] usb: dwc2: Fix HiKey >> regression caused by power_down feature" >> https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Dusb-26m-3D152669095513248-26w-3D2&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=0lMkv7adFVwkzyaUzD6-pUG0iwg4fd6b1-aHQgbqvSI&s=m8SZvo3J_Za08sMbo-S9EkhoA06YnzEN-SRm-uTPnbg&e= >> >> the power_down is disabled setting "p->power_down = false;" in >> "dwc2_set_his_params" function. >> >> Could you please clarify that the testes done for those 3 patches were >> done enabling "p->power_down = true;" in "dwc2_set_his_params" function. > > So if I remove the "power_down = true" initialization, USB does not > seem to function. > > If I boot w/ the gadget port removed, the USB host ports do work, but > plugging in the gadget cable results in a bunch of: > dwc2 f72c.usb: Waiting for Host Mode, Mode=Peripheral > messages. > > If I boot w/ the gadget port plugged in, USB gadget mode doesn't seem > to function at all, and when I remove the gadget cable nothing > happens, it doesn't switch to host mode. > > thanks > -john > Could you please send the dmesg logs for those situations? Also, please specify the version of the kernel that the testes has been done on. Regards, Artur
RE: [PATCH 1/3] usb: host: xhci: fix oops when removing hcd
> Hi Peter,
>
> On Fri, Sep 21, 2018 at 09:48:43AM +0800, Peter Chen wrote:
> > Type-C-to-A cable, and the USB3 HCD has already been NULL at that time.
> > The oops log like below:
> >
> > [681.782288] xhci-hcd xhci-hcd.1.auto: remove, state 1 [681.787490]
> > usb usb4: USB disconnect, device number 1 [681.792808] usb 4-1: USB
> > disconnect, device number 2 [681.818089] xhci-hcd xhci-hcd.1.auto: USB
> > bus 4 deregistered [681.823803] Unable to handle kernel NULL pointer
> > dereference at virtual address 00a0 [681.823806] Mem abort info:
> > [681.823809] Exception class = DABT (current EL), IL = 32 bits
> > [681.823811] SET = 0, FnV = 0
> > [681.823813] EA = 0, S1PTW = 0
> > [681.823814] Data abort info:
> > [681.823816] ISV = 0, ISS = 0x0004
> > [681.823818] CM = 0, WnR = 0
> > [681.823822] user pgtable: 4k pages, 48-bit VAs, pgd =
> > 8000ae3fd000 [681.823824] [00a0] *pgd=
> > [681.823829] Internal error: Oops: 9604 [#1] PREEMPT SMP
> > [681.823832] Modules linked in: 8021q garp stp mrp crc32_ce qca6174(O)
> crct10dif_ce galcore(O)
> > [681.823849] CPU: 0 PID: 94 Comm: kworker/0:1 Tainted: G O
> > 4.14.62-
> imx_4.14.y+gcd63def #1
> > [681.823851] Hardware name: Freescale i.MX8MQ EVK (DT) [681.823862]
> > Workqueue: events_freezable __dwc3_set_mode [681.823865] task:
> > 8000b8a18000 task.stack: 0a01 [681.823872] PC is at
> > xhci_irq+0x5fc/0x14b8 [681.823875] LR is at xhci_irq+0x3c/0x14b8
>
>
>
> > diff --git a/drivers/usb/host/xhci-ring.c
> > b/drivers/usb/host/xhci-ring.c index f0a99aa0ac58..2dc5176b79d0 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -2680,7 +2680,8 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
> > }
> >
> > if (xhci->xhc_state & XHCI_STATE_DYING ||
> > - xhci->xhc_state & XHCI_STATE_HALTED) {
> > + xhci->xhc_state & XHCI_STATE_HALTED ||
> > + xhci->xhc_state & XHCI_STATE_REMOVING) {
> > xhci_dbg(xhci, "xHCI dying, ignoring interrupt. "
> > "Shouldn't IRQs be disabled?\n");
> > /* Clear the event handler busy flag (RW1C);
>
> We also noticed the same crash as you found, and tried to fix it in a similar
> way, but
> noticed that this still allows a memory leak to happen.
>
> It seems from commit fe190ed0d602a ("xhci: Do not halt the host until both HCD
> have disconnected their devices.") this was added to xhci_stop(), and is the
> reason
> we encounter the NULL pointer in
> xhci_irq() when it tries to access xhci->shared_hcd.
>
> + /* Only halt host and free memory after both hcds are removed */
> if (!usb_hcd_is_primary_hcd(hcd)) {
> + /* usb core will free this hcd shortly, unset pointer */
> + xhci->shared_hcd = NULL;
> mutex_unlock(&xhci->mutex);
> return;
> }
>
> While your fix will simply abort the xhci_irq() function if it encounters
> XHCI_STATE_REMOVING, during xhci_plat_remove():
>
> static int xhci_plat_remove(struct platform_device *dev)
> {
> struct usb_hcd *hcd = platform_get_drvdata(dev);
> struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> struct clk *clk = xhci->clk;
> struct clk *reg_clk = xhci->reg_clk;
>
> xhci->xhc_state |= XHCI_STATE_REMOVING;
>
> usb_remove_hcd(xhci->shared_hcd);
> ^ calls xhci_stop() and sets shared_hcd=NULL
> usb_phy_shutdown(hcd->usb_phy);
>
> usb_remove_hcd(hcd);
> usb_put_hcd(xhci->shared_hcd);
> ^^^ shared_hcd==NULL, so this is a no-op
>
> Since usb_put_hcd() doesn't get called for shared_hcd, we end up with one
> additional kref count and hence a leak.
>
> Wondering if we need to also remove the xhci->shared_hcd = NULL from
> xhci_stop(),
> in addition to your patches. Thoughts?
>
Hi Jack,
With your thought of removing the xhci->shared_hcd = NULL at xhci_stop, the
oops in
this commit mentioned has disappeared. It seems removing USB3 HCD structure
not affect it handles USB3 interrupt during the disconnection. Please have a
test if
only this change can fix your problem. You could submit a patch for this change
as a fix, it fixed the memory leak for USB3 HCD structure too.
Peter
RE: [PATCH net 3/7] lan78xx: Check for supported Wake-on-LAN modes
Hi Florian, > @@ -1401,19 +1401,10 @@ static int lan78xx_set_wol(struct net_device ... > + if (pdata->wol & ~WAKE_ALL) > + return -EINVAL; If I understand correctly, it needs to check againt "wol->wolopts" than pdata->wol. > + > + pdata->wol = wol->wolopts; Thanks. Woojung
Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()
On Tue, 25 Sep 2018, Vladis Dronov wrote: > > > What about adding a WARN_ON()? It doesn't crash the kernel and it will > > > be detected and reported by syzbot. > > Yes, that would be a great solution. > > > Sure, we could do that. But would be the point? > > We know when usb_find_alt_setting() callers do smth weird and go fix them. > > > After c9a4cb204e9e, calling usb_find_alt_setting() with a NULL config is > > no more of a bug than calling kfree() with a NULL pointer. > > Yes, exactly. > > > You wouldn't want to put a WARN_ON in kfree(), would you? > > Honestly, in the ideal world I would, again, to be aware when some code does > something weird so we know about it. But this world is this world, it needs > more performance to the throne of performance. But is it really worthwhile? In terms of catching bugs, this would help in only one situation: when the programmer thinks the argument should always be non-NULL because a NULL argument indicates a bug. Such situations seem to be relatively rare, and we can handle them by inserting a WARN_ON() at the call site if need be. So it's a choice between: 1. Putting a single test for NULL in the function being called, together with WARN_ON() at a small number of call sites, or 2. Putting a WARN_ON() (or allowing a crash) in the function being called, together with tests for NULL at a potentially large number of call sites. 1 has two advantages over 2. First, it involves adding less code overall. Second, it doesn't require the programmer to remember to add special code (a test or a WARN_ON) in situation where it doesn't matter -- presumably the majority of them. Now consider the case at hand: the call to usb_find_alt_setting() from check_ctrlrecip(). In this case ps->dev->actconfig being NULL doesn't indicate an error or a bug; it merely indicates that the user is trying to send a control request to a device which happens to be unconfigured, which is a perfectly valid thing to do. Therefore it shouldn't require any special handling at the call site. Alan Stern
Re: [PATCH v2 1/2] dt-bindings: Documentation for qcom,eud
On Tue, Sep 04, 2018 at 02:34:12PM -0700, Prakruthi Deepak Heragu wrote:
> Documentation for Embedded USB Debugger (EUD) device tree bindings.
>
> Signed-off-by: Satya Durga Srinivasu Prabhala
> Signed-off-by: Prakruthi Deepak Heragu
> ---
> .../devicetree/bindings/soc/qcom/qcom,msm-eud.txt | 41
> ++
> 1 file changed, 41 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
>
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
> b/Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
> new file mode 100644
> index 000..a03021a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,msm-eud.txt
> @@ -0,0 +1,41 @@
> +* Qualcomm Technologies Inc Embedded USB Debugger (EUD)
> +
> +The EUD (Embedded USB Debugger) is a mini-USB hub implemented
> +on chip to support the USB-based debug and trace capabilities.
Is it just for debug and normally bypassed?
> +
> +Required properties:
> +
> + - compatible: Should be "qcom,msm-eud"
Needs to be SoC specific (though a fallback is fine).
> + - interrupts: Interrupt number
> + - reg: Should be address and size of EUD register space
> +
> +Driver notifies clients for VBUS attach/detach and charger enable/disable
Bindings are for h/w blocks, not drivers.
> +events. The link between client and EUD is established via a directed
> +graph. EUD driver has one endpoint of the graph link mentioning EUD as an
> +output link and clients registered for these notifications from the EUD
> +should have the other endpoint of the graph link as an input link.
OF graph is for describing data flows (i.e. h/w connections), not
clients wanting some event.
> Each of
> +these endpoints should contain a 'remote-endpoint' phandle property that
> +points to the corresponding endpoint in the port of the remote device.
You don't need to describe how the graph binding works. Just what the
port assignments are.
I worry this is going to collide with using the graph binding for USB
connectors.
> +
> +An example for EUD device node:
> +
> + eud: qcom,msm-eud@88e {
> + compatible = "qcom,msm-eud";
> + interrupts = ;
> + reg = <0x88e 0x4000>;
> + port {
> + eud_output: endpoint {
> + remote-endpoint = <&usb3_input>;
> +};
> + };
> + };
> +
> +An example for EUD client:
What are possible clients? Could we want to switch clients at runtime?
> +
> + usb3 {
> + port {
> + usb3_input: endpoint {
> + remote-endpoint = <&eud_output>;
> +};
> + };
> + };
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()
Hello, Alan, Andrey, all, > > > (You'll be lucky if Linus doesn't see that. He yells at anybody who > > > suggests adding BUG_ON for anything that doesn't completely crash Now, may be not ) > > > How is this different from calling kfree() with a NULL argument? It is not, it is the same case. > > What about adding a WARN_ON()? It doesn't crash the kernel and it will > > be detected and reported by syzbot. Yes, that would be a great solution. > Sure, we could do that. But would be the point? We know when usb_find_alt_setting() callers do smth weird and go fix them. > After c9a4cb204e9e, calling usb_find_alt_setting() with a NULL config is > no more of a bug than calling kfree() with a NULL pointer. Yes, exactly. > You wouldn't want to put a WARN_ON in kfree(), would you? Honestly, in the ideal world I would, again, to be aware when some code does something weird so we know about it. But this world is this world, it needs more performance to the throne of performance. I have no other arguments except the above, please, feel free to not to accept my patch. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer
Re: [PATCH net 3/7] lan78xx: Check for supported Wake-on-LAN modes
On 09/25/2018 10:32 AM, woojung@microchip.com wrote: > Hi Florian, > + if (pdata->wol == 0) + return -EINVAL; + >>> It will make function return when disabling WOL. >> >> Huh, yes, good point. >> >>> Is there other place handling this scenario? >> >> How do you mean? >> > I meant there is another path I might miss when disabling WOL > than this xxx_set_wol(). I don't think so, at least not from the ethtool perspective, this should fix the issue before, and simplifying the code, since all we are doing it taking a bitmask, checking each bit we support, and again, make it the same bitmask in pdata->wol, can you test that? If you have a new enough version of ethtool, try using: ethtool -s wol f, which was added recently and which this driver does not support: diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c index a9991c5f4736..2e37028ef6ca 100644 --- a/drivers/net/usb/lan78xx.c +++ b/drivers/net/usb/lan78xx.c @@ -1401,19 +1401,10 @@ static int lan78xx_set_wol(struct net_device *netdev, if (ret < 0) return ret; - pdata->wol = 0; - if (wol->wolopts & WAKE_UCAST) - pdata->wol |= WAKE_UCAST; - if (wol->wolopts & WAKE_MCAST) - pdata->wol |= WAKE_MCAST; - if (wol->wolopts & WAKE_BCAST) - pdata->wol |= WAKE_BCAST; - if (wol->wolopts & WAKE_MAGIC) - pdata->wol |= WAKE_MAGIC; - if (wol->wolopts & WAKE_PHY) - pdata->wol |= WAKE_PHY; - if (wol->wolopts & WAKE_ARP) - pdata->wol |= WAKE_ARP; + if (pdata->wol & ~WAKE_ALL) + return -EINVAL; + + pdata->wol = wol->wolopts; device_set_wakeup_enable(&dev->udev->dev, (bool)wol->wolopts); -- Florian
Re: [PATCH] usb: dwc2: Fix HiKey regression caused by power_down feature
On Tue, Sep 25, 2018 at 3:04 AM, Artur Petrosyan wrote: > Just a clarification by this commit "[PATCH] usb: dwc2: Fix HiKey > regression caused by power_down feature" > https://marc.info/?l=linux-usb&m=152669095513248&w=2 > > the power_down is disabled setting "p->power_down = false;" in > "dwc2_set_his_params" function. > > Could you please clarify that the testes done for those 3 patches were > done enabling "p->power_down = true;" in "dwc2_set_his_params" function. So if I remove the "power_down = true" initialization, USB does not seem to function. If I boot w/ the gadget port removed, the USB host ports do work, but plugging in the gadget cable results in a bunch of: dwc2 f72c.usb: Waiting for Host Mode, Mode=Peripheral messages. If I boot w/ the gadget port plugged in, USB gadget mode doesn't seem to function at all, and when I remove the gadget cable nothing happens, it doesn't switch to host mode. thanks -john
Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()
On Tue, 25 Sep 2018, Andrey Konovalov wrote: > On Tue, Sep 25, 2018 at 5:15 PM, Alan Stern wrote: > > On Tue, 25 Sep 2018, Vladis Dronov wrote: > > > >> > What reason is there for having two different fixes for the same bug? > >> > This one isn't going to get into any mainline trees that don't already > >> > have c9a4cb204e9e. > >> > >> I believe this is the right thing to do, so usb_find_alt_setting() > >> is not called with a known-bad argument. > >> > >> Honestly, I would change "if (!config)" in usb_find_alt_setting() to > >> "BUG_ON(!config)" so we know when its callers do smth wrong and go > > > > (You'll be lucky if Linus doesn't see that. He yells at anybody who > > suggests adding BUG_ON for anything that doesn't completely crash the > > kernel. The basic problem is that "BUG_ON" is not a good name: That > > routine doesn't really report bugs; instead it brings everything to a > > halt in situations where the kernel is unable to proceed. In practice > > this tends to make actual debugging more difficult.) > > What about adding a WARN_ON()? It doesn't crash the kernel and it will > be detected and reported by syzbot. Sure, we could do that. But would be the point? After c9a4cb204e9e, calling usb_find_alt_setting() with a NULL config is no more of a bug than calling kfree() with a NULL pointer. You wouldn't want to put a WARN_ON in kfree(), would you? Alan Stern
Re: Fwd: Lenovo wireless keyboard - kernel/driver problem
Yes, it is most likely HID issue. Thank you greatly for such a fast answer and for the direction! Peter On Tue, 25 Sep 2018 at 19:39, Greg KH wrote: > > On Tue, Sep 25, 2018 at 07:29:43PM +0200, Peter Hostačný wrote: > > Hello there, > > > > there is quite a big trouble around few specific keyboards that are > > not working in Linux. > > I personally have "Lenovo Professional Wireless Keyboard and Mouse > > Combo 4X30H56803" > > The keyboard is not working, mouse is. There is a lot of information > > spread across multiple threads of many sites. Issue is described quite > > well in many of them. > > > > - > > https://askubuntu.com/questions/897729/lenovo-professional-wireless-keyboard-and-mouse-combo-not-working-in-ubuntu?noredirect=1&lq=1 > > - https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1771431 > > - https://bugzilla.kernel.org/show_bug.cgi?id=197787 > > - > > https://unix.stackexchange.com/questions/377830/linux-hid-driver-for-primax-wireless-keyboards/377873#377873 > > - > > https://forums.lenovo.com/t5/Linux-Discussion/Professional-Wireless-Keyboard-not-working-on-Linux/td-p/3726486 > > > > Could you please have a look at it? I will be glad to provide any > > information if needed. > > You might want to post this to the linux-in...@vger.kernel.org list. If > one portion of the device is working, odds are it's not a USB problem, > but rather a HID issue. > > thanks, > > greg k-h
Re: Fwd: Lenovo wireless keyboard - kernel/driver problem
On Tue, Sep 25, 2018 at 07:29:43PM +0200, Peter Hostačný wrote: > Hello there, > > there is quite a big trouble around few specific keyboards that are > not working in Linux. > I personally have "Lenovo Professional Wireless Keyboard and Mouse > Combo 4X30H56803" > The keyboard is not working, mouse is. There is a lot of information > spread across multiple threads of many sites. Issue is described quite > well in many of them. > > - > https://askubuntu.com/questions/897729/lenovo-professional-wireless-keyboard-and-mouse-combo-not-working-in-ubuntu?noredirect=1&lq=1 > - https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1771431 > - https://bugzilla.kernel.org/show_bug.cgi?id=197787 > - > https://unix.stackexchange.com/questions/377830/linux-hid-driver-for-primax-wireless-keyboards/377873#377873 > - > https://forums.lenovo.com/t5/Linux-Discussion/Professional-Wireless-Keyboard-not-working-on-Linux/td-p/3726486 > > Could you please have a look at it? I will be glad to provide any > information if needed. You might want to post this to the linux-in...@vger.kernel.org list. If one portion of the device is working, odds are it's not a USB problem, but rather a HID issue. thanks, greg k-h
RE: [PATCH net 3/7] lan78xx: Check for supported Wake-on-LAN modes
Hi Florian, > >> + if (pdata->wol == 0) > >> + return -EINVAL; > >> + > > It will make function return when disabling WOL. > > Huh, yes, good point. > > > Is there other place handling this scenario? > > How do you mean? > I meant there is another path I might miss when disabling WOL than this xxx_set_wol(). Thanks Woojung
Fwd: Lenovo wireless keyboard - kernel/driver problem
Hello there, there is quite a big trouble around few specific keyboards that are not working in Linux. I personally have "Lenovo Professional Wireless Keyboard and Mouse Combo 4X30H56803" The keyboard is not working, mouse is. There is a lot of information spread across multiple threads of many sites. Issue is described quite well in many of them. - https://askubuntu.com/questions/897729/lenovo-professional-wireless-keyboard-and-mouse-combo-not-working-in-ubuntu?noredirect=1&lq=1 - https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1771431 - https://bugzilla.kernel.org/show_bug.cgi?id=197787 - https://unix.stackexchange.com/questions/377830/linux-hid-driver-for-primax-wireless-keyboards/377873#377873 - https://forums.lenovo.com/t5/Linux-Discussion/Professional-Wireless-Keyboard-not-working-on-Linux/td-p/3726486 Could you please have a look at it? I will be glad to provide any information if needed. Best regards, Peter
Re: [PATCH net 3/7] lan78xx: Check for supported Wake-on-LAN modes
On 09/25/2018 10:19 AM, woojung@microchip.com wrote: > Hi Florian, > >> @@ -1415,6 +1415,9 @@ static int lan78xx_set_wol(struct net_device *netdev, >> if (wol->wolopts & WAKE_ARP) >> pdata->wol |= WAKE_ARP; >> >> +if (pdata->wol == 0) >> +return -EINVAL; >> + > It will make function return when disabling WOL. Huh, yes, good point. > Is there other place handling this scenario? How do you mean? > >> device_set_wakeup_enable(&dev->udev->dev, (bool)wol->wolopts); >> >> phy_ethtool_set_wol(netdev->phydev, wol); > > > Thanks. > Woojung > -- Florian
RE: [PATCH net 3/7] lan78xx: Check for supported Wake-on-LAN modes
Hi Florian, > @@ -1415,6 +1415,9 @@ static int lan78xx_set_wol(struct net_device *netdev, > if (wol->wolopts & WAKE_ARP) > pdata->wol |= WAKE_ARP; > > + if (pdata->wol == 0) > + return -EINVAL; > + It will make function return when disabling WOL. Is there other place handling this scenario? > device_set_wakeup_enable(&dev->udev->dev, (bool)wol->wolopts); > > phy_ethtool_set_wol(netdev->phydev, wol); Thanks. Woojung
UVC gadget changes for v4.20
Hi Felipe, The following changes since commit 5b394b2ddf0347bef56e50c69a58773c94343ff3: Linux 4.19-rc1 (2018-08-26 14:11:59 -0700) are available in the Git repository at: git://linuxtv.org/pinchartl/media.git tags/uvcg-20180925 for you to fetch changes up to 3fb2fd76eda265ce5421318de38dd9b9f7c54737: usb: gadget: uvc: configfs: Use %u to print unsigned int values (2018-09-25 18:48:10 +0300) UVC gadget updates for v4.20 - configfs cleanups, fixes and extensions - Endianness fixes - Miscellaneous cleanups Joel Pepper (2): usb: gadget: uvc: configfs: Add bFrameIndex attributes usb: gadget: uvc: configfs: Prevent format changes after linking header Laurent Pinchart (14): usb: gadget: uvc: configfs: Don't wrap groups unnecessarily usb: gadget: uvc: configfs: Add section header comments usb: gadget: uvc: configfs: Drop leaked references to config items usb: gadget: uvc: configfs: Allocate groups dynamically usb: gadget: uvc: configfs: Add interface number attributes usb: gadget: uvc: configfs: Add bFormatIndex attributes usb: gadget: uvc: Factor out video USB request queueing usb: gadget: uvc: Only halt video streaming endpoint in bulk mode usb: gadget: uvc: Replace plain printk() with dev_*() usb: gadget: uvc: Remove uvc_set_trace_param() function usb: video: Fix endianness mismatches in descriptor structures usb: gadget: uvc: configfs: Fix operation on big endian platforms usb: gadget: uvc: configfs: Simplify attributes macros usb: gadget: uvc: configfs: Use %u to print unsigned int values Paul Elder (1): usb: gadget: uvc: configfs: Sort frame intervals upon writing Documentation/ABI/testing/configfs-usb-gadget-uvc | 24 + drivers/usb/gadget/function/f_uvc.c | 57 +- drivers/usb/gadget/function/u_uvc.h |3 + drivers/usb/gadget/function/uvc.h | 16 +- drivers/usb/gadget/function/uvc_configfs.c| 1168 ++- drivers/usb/gadget/function/uvc_v4l2.c|4 +- drivers/usb/gadget/function/uvc_video.c | 48 +- drivers/usb/gadget/function/uvc_video.h |2 +- include/uapi/linux/usb/video.h| 304 +++--- 9 files changed, 916 insertions(+), 710 deletions(-) -- Regards, Laurent Pinchart
Re: [PATCH v1] arm64: dts: dwc3: Add description of 'configure-gfladj'
On Wed, Aug 29, 2018 at 03:33:24PM +0800, Yinbo Zhu wrote: > This patch is to add description of 'configure-gfladj' to binding > so that configuring devicetree Not a complete sentence. The subject should be: "dt-bindings: usb: ..." > > Signed-off-by: Yinbo Zhu > --- > Documentation/devicetree/bindings/usb/dwc3.txt |1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt > b/Documentation/devicetree/bindings/usb/dwc3.txt > index 3e4c38b..40c6568 100644 > --- a/Documentation/devicetree/bindings/usb/dwc3.txt > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt > @@ -101,6 +101,7 @@ Optional properties: > When just one value, which means INCRX burst mode > enabled. When > more than one value, which means undefined length INCR > burst type > enabled. The values can be 1, 4, 8, 16, 32, 64, 128 and > 256. > + - configure-gfladj: determine whether frame length adjustment is required > or not. Needs a vendor prefix. What's the type? bool? What determines if this is needed. If fixed for an SoC or IP version, then the compatible string should imply this. Rob
Re: [PATCH 3/3] usb: gadget: uvc: Replace plain printk() with dev_*()
Hi Laurent,
On 18/09/18 11:35, Laurent Pinchart wrote:
> Adding device context to the kernel log messages make them more useful.
> Add new uvcg_* macros based on dev_*() that print both the gadget device
> name and the function name.
>
> While at it, remove a commented out printk statement and an unused
> printk-based macro.
>
> Signed-off-by: Laurent Pinchart
Great - looks good to me.
Reviewed-by: Kieran Bingham
> ---
> drivers/usb/gadget/function/f_uvc.c | 41
> ++---
> drivers/usb/gadget/function/uvc.h | 16 ++---
> drivers/usb/gadget/function/uvc_v4l2.c | 4 ++--
> drivers/usb/gadget/function/uvc_video.c | 18 +--
> drivers/usb/gadget/function/uvc_video.h | 2 +-
> 5 files changed, 39 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c
> b/drivers/usb/gadget/function/f_uvc.c
> index 4ea987741e6e..0cc4a6220050 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -232,13 +232,8 @@ uvc_function_setup(struct usb_function *f, const struct
> usb_ctrlrequest *ctrl)
> struct v4l2_event v4l2_event;
> struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
>
> - /* printk(KERN_INFO "setup request %02x %02x value %04x index %04x
> %04x\n",
> - * ctrl->bRequestType, ctrl->bRequest, le16_to_cpu(ctrl->wValue),
> - * le16_to_cpu(ctrl->wIndex), le16_to_cpu(ctrl->wLength));
> - */
> -
> if ((ctrl->bRequestType & USB_TYPE_MASK) != USB_TYPE_CLASS) {
> - INFO(f->config->cdev, "invalid request type\n");
> + uvcg_info(f, "invalid request type\n");
> return -EINVAL;
> }
>
> @@ -272,7 +267,7 @@ uvc_function_get_alt(struct usb_function *f, unsigned
> interface)
> {
> struct uvc_device *uvc = to_uvc(f);
>
> - INFO(f->config->cdev, "uvc_function_get_alt(%u)\n", interface);
> + uvcg_info(f, "%s(%u)\n", __func__, interface);
>
> if (interface == uvc->control_intf)
> return 0;
> @@ -291,13 +286,13 @@ uvc_function_set_alt(struct usb_function *f, unsigned
> interface, unsigned alt)
> struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
> int ret;
>
> - INFO(cdev, "uvc_function_set_alt(%u, %u)\n", interface, alt);
> + uvcg_info(f, "%s(%u, %u)\n", __func__, interface, alt);
>
> if (interface == uvc->control_intf) {
> if (alt)
> return -EINVAL;
>
> - INFO(cdev, "reset UVC Control\n");
> + uvcg_info(f, "reset UVC Control\n");
> usb_ep_disable(uvc->control_ep);
>
> if (!uvc->control_ep->desc)
> @@ -348,7 +343,7 @@ uvc_function_set_alt(struct usb_function *f, unsigned
> interface, unsigned alt)
> if (!uvc->video.ep)
> return -EINVAL;
>
> - INFO(cdev, "reset UVC\n");
> + uvcg_info(f, "reset UVC\n");
> usb_ep_disable(uvc->video.ep);
>
> ret = config_ep_by_speed(f->config->cdev->gadget,
> @@ -373,7 +368,7 @@ uvc_function_disable(struct usb_function *f)
> struct uvc_device *uvc = to_uvc(f);
> struct v4l2_event v4l2_event;
>
> - INFO(f->config->cdev, "uvc_function_disable\n");
> + uvcg_info(f, "%s()\n", __func__);
>
> memset(&v4l2_event, 0, sizeof(v4l2_event));
> v4l2_event.type = UVC_EVENT_DISCONNECT;
> @@ -392,21 +387,19 @@ uvc_function_disable(struct usb_function *f)
> void
> uvc_function_connect(struct uvc_device *uvc)
> {
> - struct usb_composite_dev *cdev = uvc->func.config->cdev;
> int ret;
>
> if ((ret = usb_function_activate(&uvc->func)) < 0)
> - INFO(cdev, "UVC connect failed with %d\n", ret);
> + uvcg_info(&uvc->func, "UVC connect failed with %d\n", ret);
> }
>
> void
> uvc_function_disconnect(struct uvc_device *uvc)
> {
> - struct usb_composite_dev *cdev = uvc->func.config->cdev;
> int ret;
>
> if ((ret = usb_function_deactivate(&uvc->func)) < 0)
> - INFO(cdev, "UVC disconnect failed with %d\n", ret);
> + uvcg_info(&uvc->func, "UVC disconnect failed with %d\n", ret);
> }
>
> /* --
> @@ -605,7 +598,7 @@ uvc_function_bind(struct usb_configuration *c, struct
> usb_function *f)
> struct f_uvc_opts *opts;
> int ret = -EINVAL;
>
> - INFO(cdev, "uvc_function_bind\n");
> + uvcg_info(f, "%s()\n", __func__);
>
> opts = fi_to_f_uvc_opts(f->fi);
> /* Sanity check the streaming endpoint module parameters.
> @@ -618,8 +611,8 @@ uvc_function_bind(struct usb_configuration *c, struct
> usb_function *f)
> if (opts->streaming_maxburst &&
> (opts->streaming_maxpacket % 1024) != 0) {
> opts->streaming_maxpacket = roundup(opts->streaming_maxpacket,
> 1024);
> - INFO(cdev
Re: [PATCH 2/3] usb: gadget: uvc: Only halt video streaming endpoint in bulk mode
Hi Laurent,
On 18/09/18 11:35, Laurent Pinchart wrote:
> When USB requests for video data fail to be submitted, the driver
> signals a problem to the host by halting the video streaming endpoint.
> This is only valid in bulk mode, as isochronous transfers have no
> handshake phase and can't thus report a stall. The usb_ep_set_halt()
> call returns an error when using isochronous endpoints, which we happily
> ignore, but some UDCs complain in the kernel log. Fix this by only
> trying to halt the endpoint in bulk mode.
>
Agreed,
> Signed-off-by: Laurent Pinchart
Reviewed-by: Kieran Bingham
> ---
> drivers/usb/gadget/function/uvc_video.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_video.c
> b/drivers/usb/gadget/function/uvc_video.c
> index a95c8e2364ed..2c9821ec836e 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -132,7 +132,9 @@ static int uvcg_video_ep_queue(struct uvc_video *video,
> struct usb_request *req)
> ret = usb_ep_queue(video->ep, req, GFP_ATOMIC);
> if (ret < 0) {
> printk(KERN_INFO "Failed to queue request (%d).\n", ret);
> - usb_ep_set_halt(video->ep);
> + /* Isochronous endpoints can't be halted. */
> + if (usb_endpoint_xfer_bulk(video->ep->desc))
> + usb_ep_set_halt(video->ep);
> }
>
> return ret;
>
--
Regards
--
Kieran
Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()
On Tue, Sep 25, 2018 at 5:15 PM, Alan Stern wrote: > On Tue, 25 Sep 2018, Vladis Dronov wrote: > >> > What reason is there for having two different fixes for the same bug? >> > This one isn't going to get into any mainline trees that don't already >> > have c9a4cb204e9e. >> >> I believe this is the right thing to do, so usb_find_alt_setting() >> is not called with a known-bad argument. >> >> Honestly, I would change "if (!config)" in usb_find_alt_setting() to >> "BUG_ON(!config)" so we know when its callers do smth wrong and go > > (You'll be lucky if Linus doesn't see that. He yells at anybody who > suggests adding BUG_ON for anything that doesn't completely crash the > kernel. The basic problem is that "BUG_ON" is not a good name: That > routine doesn't really report bugs; instead it brings everything to a > halt in situations where the kernel is unable to proceed. In practice > this tends to make actual debugging more difficult.) What about adding a WARN_ON()? It doesn't crash the kernel and it will be detected and reported by syzbot.
Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()
On Tue, 25 Sep 2018, Vladis Dronov wrote: > > What reason is there for having two different fixes for the same bug? > > This one isn't going to get into any mainline trees that don't already > > have c9a4cb204e9e. > > I believe this is the right thing to do, so usb_find_alt_setting() > is not called with a known-bad argument. > > Honestly, I would change "if (!config)" in usb_find_alt_setting() to > "BUG_ON(!config)" so we know when its callers do smth wrong and go (You'll be lucky if Linus doesn't see that. He yells at anybody who suggests adding BUG_ON for anything that doesn't completely crash the kernel. The basic problem is that "BUG_ON" is not a good name: That routine doesn't really report bugs; instead it brings everything to a halt in situations where the kernel is unable to proceed. In practice this tends to make actual debugging more difficult.) > fix callers. Unfortunately, I understand this hardly will be accepted. How is this different from calling kfree() with a NULL argument? Alan Stern
Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()
> What reason is there for having two different fixes for the same bug? > This one isn't going to get into any mainline trees that don't already > have c9a4cb204e9e. I believe this is the right thing to do, so usb_find_alt_setting() is not called with a known-bad argument. Honestly, I would change "if (!config)" in usb_find_alt_setting() to "BUG_ON(!config)" so we know when its callers do smth wrong and go fix callers. Unfortunately, I understand this hardly will be accepted. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer
[GIT PULL] USB driver fixes for 4.19-rc4
The following changes since commit 7876320f88802b22d4e2daf7eb027dd14175a0f8: Linux 4.19-rc4 (2018-09-16 11:52:37 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git tags/usb-4.19-rc6 for you to fetch changes up to 3e3b81965cbfa01fda6d77750feedc3c46fc28d0: usb: typec: mux: Take care of driver module reference counting (2018-09-20 13:35:01 +0200) USB fixes for 4.19-rc6 Here are some small USB core and driver fixes for reported issues for 4.19-rc6. The most visible is the oops fix for when the USB core is built into the kernel that is present in 4.18. Turns out not many people actually do that so it went unnoticed for a while. The rest is some tiny typec, musb, and other core fixes. All have been in linux-next with no reported issues. Signed-off-by: Greg Kroah-Hartman Alan Stern (3): USB: remove LPM management from usb_driver_claim_interface() USB: fix error handling in usb_driver_claim_interface() USB: handle NULL config in usb_find_alt_setting() Bin Liu (1): usb: musb: dsps: do not disable CPPI41 irq in driver teardown Harry Pan (1): usb: core: safely deal with the dynamic quirk lists Heikki Krogerus (2): usb: roles: Take care of driver module reference counting usb: typec: mux: Take care of driver module reference counting Oliver Neukum (2): USB: usbdevfs: sanitize flags more USB: usbdevfs: restore warning for nonsensical flags Sebastian Andrzej Siewior (1): Revert "usb: cdc-wdm: Fix a sleep-in-atomic-context bug in service_outstanding_interrupt()" drivers/usb/class/cdc-wdm.c | 2 +- drivers/usb/common/roles.c | 15 --- drivers/usb/core/devio.c | 24 +--- drivers/usb/core/driver.c| 28 ++-- drivers/usb/core/quirks.c| 3 ++- drivers/usb/core/usb.c | 2 ++ drivers/usb/musb/musb_dsps.c | 12 +--- drivers/usb/typec/mux.c | 17 + 8 files changed, 66 insertions(+), 37 deletions(-)
Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()
On Tue, 25 Sep 2018, Vladis Dronov wrote:
> ps->dev->actconfig can be NULL and cause NULL-deref in usb_find_alt_setting()
> before c9a4cb204e9e. fix this anyway by checking that ps->dev->actconfig is
> not
> NULL, so usb_find_alt_setting() is not called with a known-bad argument.
What reason is there for having two different fixes for the same bug?
This one isn't going to get into any mainline trees that don't already
have c9a4cb204e9e.
Alan Stern
> Signed-off-by: Vladis Dronov
> Reported-by: syzbot+19c3aaef85a89d451...@syzkaller.appspotmail.com
> ---
> drivers/usb/core/devio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 6ce77b33da61..26047620b003 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -824,7 +824,7 @@ static int check_ctrlrecip(struct usb_dev_state *ps,
> unsigned int requesttype,
>* class specification, which we always want to allow as it is used
>* to query things like ink level, etc.
>*/
> - if (requesttype == 0xa1 && request == 0) {
> + if (requesttype == 0xa1 && request == 0 && ps->dev->actconfig) {
> alt_setting = usb_find_alt_setting(ps->dev->actconfig,
> index >> 8, index & 0xff);
> if (alt_setting
[PATCH v7] USB: serial: ftdi_sio: implement GPIO support for FT-X devices
This patch allows using the CBUS pins of FT-X devices as GPIO in CBUS
bitbanging mode. There is no conflict between the GPIO and VCP
functionality in this mode. Tested on FT230X and FT231X.
As there is no way to request the current CBUS register configuration
from the device, all CBUS pins are set to a known state when the first
GPIO is requested. This allows using libftdi to set the GPIO pins
before loading this module for UART functionality, a behavior that
existing applications might be relying upon (though no specific case
is known to the authors of this patch).
Signed-off-by: Karoly Pados
---
Changelog:
- v2: Fix compile error when CONFIG_GPIOLIB is not defined.
- v3: Incorporate review feedback.
- v4: Include linux/gpio/driver.h unconditionally.
Replace and invert gpio_input with gpio_output.
Make ftdi_gpio_direction_get return 0/1.
Change dev_err msg in ftdi_set_bitmode_req.
Change formatting of error checking in ftdi_gpio_get.
Drop dev_err in ftdi_gpio_set.
Remove some line breaks and empty lines.
Change error handling in ftdi_read_eeprom (and adjust caller).
Replace SIO->FTX in FTDI_SIO_CBUS_MUX_GPIO macro name.
- v5: Resent v4 with no changes by mistake. Please ignore.
- v6: Read only 4 bytes from eeprom in ftx_gpioconf_init.
Compare ftdi_read_eeprom result with 0 instead of eq. cehck.
Reserve 4 GPIOs even for FT234X.
Release CBUS after gpiochip deregister to avoid possible race.
Adjust comment on FTDI_SIO_SET_BITMODE macro.
Protect GPIO value/dir setting with mutex.
Add support for gpiochip.get_multiple and set_multiple.
Add names to GPIO lines.
- v7: Move line for better include sorting.
Adjust comment on gpio_lock declaration.
Space vs tab formatting changes in struct ftdi_private.
Move ftdi_ftx_gpio_names to merge it with existing #ifdef.
Rename function ftdi_set_bitmode_req to ftdi_set_bitmode.
Rename rcvbuf to buf in function ftdi_read_cbus_pins.
AND *bits and *mask in ftdi_gpio_set_multiple as gpiolib does not.
Replace if in ftdi_gpio_direction_get with negation.
Move priv->gc.names assignment to chip-specific ftx_gpioconf_init.
Though there is no code copied, libftdi was used as
a reference. ftdi_read_eeprom is based on Loic Poulain's patch.
This patch uses CBUS bitbanging mode, which works nicely in parallel with
the VCP function. The other modes do not, and so IMHO it does not make
sense to try adding them to this same module.
For this device, whenever changing the state of any single pin, all the
others need to be written too. This means in order to change any pin, we
need to know the current state of all the others. So when using GPIO,
we need to have a known starting state for all pins, but there seems to
be no way to retrieve the existing GPIO configuration (directions and
output register values). The way I handle this in this patch is that when
requesting a GPIO for the first time, the module initializes all pins to
a known default state (to inputs). Input was chosen, because a potentially
floating pin is better than a potential driver conflict, because the latter
could result in hardware damage. However, if the user does not request a
GPIO, the CBUS pins are not initialized, avoiding unnecessarily changing
hardware state. I figured I cannot rely on the default power-on state of
the device for this as the user might have used libftdi before loading
our module. When the module is unloaded, CBUS bitbanging mode is exited
if it were us who entered it earlier.
drivers/usb/serial/ftdi_sio.c | 365 +-
drivers/usb/serial/ftdi_sio.h | 27 ++-
2 files changed, 390 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index b5cef322826f..667018125ab6 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -39,6 +39,7 @@
#include
#include
#include
+#include
#include
#include "ftdi_sio.h"
#include "ftdi_sio_ids.h"
@@ -72,6 +73,15 @@ struct ftdi_private {
unsigned int latency; /* latency setting in use */
unsigned short max_packet_size;
struct mutex cfg_lock; /* Avoid mess by parallel calls of config
ioctl() and change_speed() */
+#if defined(CONFIG_GPIOLIB)
+ struct gpio_chip gc;
+ struct mutex gpio_lock; /* protects GPIO state */
+ bool gpio_registered; /* is the gpiochip in kernel registered */
+ bool gpio_used; /* true if the user requested a gpio */
+ u8 gpio_altfunc;/* which pins are in gpio mode */
+ u8 gpio_output; /* pin directions cache */
+ u8 gpio_value; /* pin value for outputs */
+#endif
};
/* struct ftdi_sio_quirk is used by devices requiring special attention. */
@@ -1766,6 +1776,349 @@ static void remove_sysfs_attrs(struct usb_serial_port
*port)
}
+#if defined(CONFIG_GPIOLIB)
+
+static
Your urgent letter is needed immediately.
Dear Partner, We want to transfer to overseas ($50.500, 000.00) Fifth Million Five Hundred Thousand United States Dollars) from the Bank of Africa, I want to ask you to quietly look for a reliable and honest person who will be capable and fit to provide either an existing bank account or to set up a new Bank account immediately to receive this money, even an empty account can serve to receive this funds quietly. I am Dr. Bernard Zoungrana, the accountant personal confidant to Mr.Raymond Beck. Who died together with his wife in a plane crash on the Tuesday, November 2, 1999 on their way to attend wedding in Boston. Mr. Raymond Beck, is an American, a physician and industrialist, he died without having any beneficiary to his assets including his account here in Burkina Faso which he opened in a Bank of Africa in the year 1994 as his personal savings for the purpose of expansion and development of his company before his untimely death in 1999. The amount involved is ($50,500,000.00) Fifth Million Five Hundred Thousand United State Dollars, no other person knows about this account, I am contacting you for us to transfer this funds to your account as the beneficiary as foreigner, I am only contacting you as a foreigner because this money cannot be approved to a local person here, without valid international foreign passport, but can only be approved to any foreigner with valid international passport or driver's license and foreign account because the money is in US Dollars and the former owner of the account Mr. Raymond Beck is a foreigner too, and as such the money can only be approved into a foreign account. However, I am revealing this to you with believe in God that you will never let me down in this business, you are the first and the only person that I am contacting for this business, so please reply urgently so that I will inform you the next step to take urgently. Send also your private telephone and fax number including the full details of the account to be used for the deposit. I need your full co-operation to make this work fine. Because the management is ready to approve this payment to any foreigner who has correct information of this account, which I will give to you, upon your positive response and once I am convinced that you are capable and will meet up with instruction of a key bank official who is deeply involved with me in this business at the conclusion of this business, you will be given 40% of the total amount, 50% will be for me, while 10% will be for expenses both parties might have incurred during the process of transferring. I look forward to your earliest reply. Proved me below information immediately: (1) YOUR FULL NAME: _ (2) YOUR AGE:___ (3) MARITAL STATUS: __(4) YOUR CELL PHONE NUMBER: __ (5) YOUR FAX NUMBER: (6) YOUR COUNTRY:__ (7) YOUR OCCUPATION: (8) SEX:_ (9) YOUR RELIGION:___ (10) YOUR PRIVATE E-MAIL ADDRESS: ___ NOTE: THIS TRANSACTION IS CONFIDENTIAL. Thanks for your assistance in advance. Dr. Bernard Zoungrana. Bill and Exchange (Executive Director).
Re: usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
Hello, On Thu, 2 Aug 2018 14:23:51 +, Vincent Pelletier wrote: > On Thu, 2 Aug 2018 00:45:14 +, "He, Bo" wrote: > > Your patch fix the issue BUG: scheduling while atomic: > > Yes, although from my understanding of Felipe's answer, the actual bug > is the "scheduling" part (sleeping in dwc3 UDC) rather than the > "atomic" part. > > So my patch addresses, still if my understanding is correct, the wrong > half of the problem, and even introduced the regression you identified. > Hence my uncertainty... I notice that neither He's patch, nor a dwc3 change to prevent it from scheduling inside usb_ep_dequeue are in Linus' master. Please correct me if I missed something. Just in case my previous emails were not clear: - I have no objection to He's patch on its own (and I do not know the code nearly enough to provide a meaningful reviewed-by). - I do not intend to work on making changes to dwc3 gadget to stop it from scheduling in usb_ep_dequeue in the foreseeable future. So if there is no ongoing work on dwc3 cancel behaviour (Felipe ?), please do resume/carry on with reviewing and integrating He's patch. It is only *if* dwc3 cancel stops scheduling that I believe my patch should be reverted (here is the hash as of Linus' master): commit d52e4d0c0c428bf2ba35074a7495cdb28e2efbae Author: Vincent Pelletier Date: Wed Jun 13 11:05:06 2018 + usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers This bug happens only when the UDC needs to sleep during usb_ep_dequeue, as is the case for (at least) dwc3. and, in my understanding, a consequence is that He's fix would not be needed anymore - the bug my patch introduced disappearing in the revert. Regards, -- Vincent Pelletier
[PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()
ps->dev->actconfig can be NULL and cause NULL-deref in usb_find_alt_setting()
before c9a4cb204e9e. fix this anyway by checking that ps->dev->actconfig is not
NULL, so usb_find_alt_setting() is not called with a known-bad argument.
Signed-off-by: Vladis Dronov
Reported-by: syzbot+19c3aaef85a89d451...@syzkaller.appspotmail.com
---
drivers/usb/core/devio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 6ce77b33da61..26047620b003 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -824,7 +824,7 @@ static int check_ctrlrecip(struct usb_dev_state *ps,
unsigned int requesttype,
* class specification, which we always want to allow as it is used
* to query things like ink level, etc.
*/
- if (requesttype == 0xa1 && request == 0) {
+ if (requesttype == 0xa1 && request == 0 && ps->dev->actconfig) {
alt_setting = usb_find_alt_setting(ps->dev->actconfig,
index >> 8, index & 0xff);
if (alt_setting
--
2.14.4
Re: [PATCH v6] USB: serial: ftdi_sio: implement GPIO support for FT-X devices
On Tue, Sep 25, 2018 at 11:11:03AM +, Karoly Pados wrote: > >> + if (priv->gpio_output & BIT(gpio)) > >> + return 0; > >> + else > >> + return 1; > >> > >> This could just simplified using negation (!), but perhaps this is > >> easier to parse as it stands. > >> > >> Sorry, it is not clear what your preferred action here is. > >> So should I leave it as is then or not? > > > > Just do > > > > res = !(priv->gpio_output & BIT(gpio)); > > > > Locking here? priv->gpio_output is a u8, there is no way it can be partially > written. Or am I missing something else? No, you're right, no locking is needed. Johan
Re: [PATCH v6] USB: serial: ftdi_sio: implement GPIO support for FT-X devices
>> + if (priv->gpio_output & BIT(gpio)) >> + return 0; >> + else >> + return 1; >> >> This could just simplified using negation (!), but perhaps this is >> easier to parse as it stands. >> >> Sorry, it is not clear what your preferred action here is. >> So should I leave it as is then or not? > > Just do > > res = !(priv->gpio_output & BIT(gpio)); > Locking here? priv->gpio_output is a u8, there is no way it can be partially written. Or am I missing something else? Karoly
Re: [PATCH v6] USB: serial: ftdi_sio: implement GPIO support for FT-X devices
On Tue, Sep 25, 2018 at 10:46:30AM +, Karoly Pados wrote:
> Hi,
>
> >> +#if defined(CONFIG_GPIOLIB)
> >> +static const char * const ftdi_ftx_gpio_names[] = {
> >> + "CBUS0", "CBUS1", "CBUS2", "CBUS3"
> >> +};
> >> +#endif
> >
> > We want to keep the ifdeffery to a minimum, so move this inside the
> > gpiolib ifdef below (and possibly even into the function where it is
> > used).
> >
> > Also note that these names are shared with FT232R, but not with FT232H.
> >
>
> What naming do you suggest then?
>
> My personal preference would be however to leave this name as is, because
> this patch only adds support for the FT-X. Even if support for others can
> be added relatively trivially after this, there is explicitly no GPIO
> support for FT232R *yet*. If somebody else adds GPIO support for the FT232R
> in a later patch, he/she should make corresponding adjustments themselves,
> including naming changes. IMHO.
Yes, that's perfectly fine. I was merely pointing it out as background
info which could possibly affect how you choose to address this (e.g.
moving it into the ftx function or not, but also that can be changed
later of course).
> >> + if (priv->gpio_output & BIT(gpio))
> >> + return 0;
> >> + else
> >> + return 1;
> >
> > This could just simplified using negation (!), but perhaps this is
> > easier to parse as it stands.
> >
>
> Sorry, it is not clear what your preferred action here is.
> So should I leave it as is then or not?
Just do
res = !(priv->gpio_output & BIT(gpio));
And I think you should add locking here to.
Johan
Re: [PATCH v6] USB: serial: ftdi_sio: implement GPIO support for FT-X devices
>> gpiolib doesn't clear bits not in mask for you, so you need to OR with >> *mask here to avoid setting random other bits. > > That was of course meant to be: *AND* with *mask. > Ah, okay, one question from my previous e-mail down.
Re: [PATCH v6] USB: serial: ftdi_sio: implement GPIO support for FT-X devices
Hi,
>> +#if defined(CONFIG_GPIOLIB)
>> +static const char * const ftdi_ftx_gpio_names[] = {
>> + "CBUS0", "CBUS1", "CBUS2", "CBUS3"
>> +};
>> +#endif
>
> We want to keep the ifdeffery to a minimum, so move this inside the
> gpiolib ifdef below (and possibly even into the function where it is
> used).
>
> Also note that these names are shared with FT232R, but not with FT232H.
>
What naming do you suggest then?
My personal preference would be however to leave this name as is, because
this patch only adds support for the FT-X. Even if support for others can
be added relatively trivially after this, there is explicitly no GPIO
support for FT232R *yet*. If somebody else adds GPIO support for the FT232R
in a later patch, he/she should make corresponding adjustments themselves,
including naming changes. IMHO.
>> +static void ftdi_gpio_set_multiple(struct gpio_chip *gc,
>> + unsigned long *mask, unsigned long *bits)
>> +{
>> + struct usb_serial_port *port = gpiochip_get_data(gc);
>> + struct ftdi_private *priv = usb_get_serial_port_data(port);
>> +
>> + mutex_lock(&priv->gpio_lock);
>> +
>> + priv->gpio_value &= ~(*mask);
>> + priv->gpio_value |= *bits;
>
> gpiolib doesn't clear bits not in mask for you, so you need to OR with
> *mask here to avoid setting random other bits.
I guess you meant AND here?
>> + if (priv->gpio_output & BIT(gpio))
>> + return 0;
>> + else
>> + return 1;
>
> This could just simplified using negation (!), but perhaps this is
> easier to parse as it stands.
>
Sorry, it is not clear what your preferred action here is.
So should I leave it as is then or not?
Karoly
Re: [PATCH v6] USB: serial: ftdi_sio: implement GPIO support for FT-X devices
On Tue, Sep 25, 2018 at 12:06:35PM +0200, Johan Hovold wrote:
> On Mon, Sep 24, 2018 at 04:31:51PM +0200, Karoly Pados wrote:
> > +static void ftdi_gpio_set_multiple(struct gpio_chip *gc,
> > + unsigned long *mask, unsigned long *bits)
> > +{
> > + struct usb_serial_port *port = gpiochip_get_data(gc);
> > + struct ftdi_private *priv = usb_get_serial_port_data(port);
> > +
> > + mutex_lock(&priv->gpio_lock);
> > +
> > + priv->gpio_value &= ~(*mask);
> > + priv->gpio_value |= *bits;
>
> gpiolib doesn't clear bits not in mask for you, so you need to OR with
> *mask here to avoid setting random other bits.
That was of course meant to be: *AND* with *mask.
Johan
Re: [PATCH v6] USB: serial: ftdi_sio: implement GPIO support for FT-X devices
On Mon, Sep 24, 2018 at 04:31:51PM +0200, Karoly Pados wrote:
> This patch allows using the CBUS pins of FT-X devices as GPIO in CBUS
> bitbanging mode. There is no conflict between the GPIO and VCP
> functionality in this mode. Tested on FT230X and FT231X.
>
> As there is no way to request the current CBUS register configuration
> from the device, all CBUS pins are set to a known state when the first
> GPIO is requested. This allows using libftdi to set the GPIO pins
> before loading this module for UART functionality, a behavior that
> existing applications might be relying upon (though no specific case
> is known to the authors of this patch).
>
> Signed-off-by: Karoly Pados
> ---
> Changelog:
> - v2: Fix compile error when CONFIG_GPIOLIB is not defined.
> - v3: Incorporate review feedback.
> - v4: Include linux/gpio/driver.h unconditionally.
> Replace and invert gpio_input with gpio_output.
> Make ftdi_gpio_direction_get return 0/1.
> Change dev_err msg in ftdi_set_bitmode_req.
> Change formatting of error checking in ftdi_gpio_get.
> Drop dev_err in ftdi_gpio_set.
> Remove some line breaks and empty lines.
> Change error handling in ftdi_read_eeprom (and adjust caller).
> Replace SIO->FTX in FTDI_SIO_CBUS_MUX_GPIO macro name.
> - v5: Resent v4 with no changes by mistake. Please ignore.
> - v6: Read only 4 bytes from eeprom in ftx_gpioconf_init.
> Compare ftdi_read_eeprom result with 0 instead of eq. cehck.
> Reserve 4 GPIOs even for FT234X.
> Release CBUS after gpiochip deregister to avoid possible race.
> Adjust comment on FTDI_SIO_SET_BITMODE macro.
> Protect GPIO value/dir setting with mutex.
> Add support for gpiochip.get_multiple and set_multiple.
> Add names to GPIO lines.
>
> get_multiple / set_multiple methods have been added in addition to earlier
> review comments.
Adding new features like this late in a review cycle risks adding new
bugs and creates more work for everyone involved.
In this case, there's a bug in your set_multiple() implementation that
will need to be addressed in a v7, so I'll comment on some style issues
as well while at it.
> drivers/usb/serial/ftdi_sio.c | 369 +-
> drivers/usb/serial/ftdi_sio.h | 27 ++-
> 2 files changed, 394 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index b5cef322826f..566284e2c316 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -40,6 +40,7 @@
> #include
> #include
> #include
> +#include
Nit: place this above linux/usb/serial.h to keep at least those includes
sorted.
> #include "ftdi_sio.h"
> #include "ftdi_sio_ids.h"
>
> @@ -72,8 +73,23 @@ struct ftdi_private {
> unsigned int latency; /* latency setting in use */
> unsigned short max_packet_size;
> struct mutex cfg_lock; /* Avoid mess by parallel calls of config
> ioctl() and change_speed() */
> +#if defined(CONFIG_GPIOLIB)
Use #ifdef here and elsewhere (defined() can be used in actual code).
> + struct gpio_chip gc;
> + struct mutex gpio_lock; /* Protect GPIO state against parallel calls
> */
Just "protects gpio state", if anything.
> + boolgpio_registered; /* is the gpiochip in kernel registered */
> + boolgpio_used;/* true if the user requested a gpio */
> + u8 gpio_altfunc; /* which pins are in gpio mode */
> + u8 gpio_output; /* pin directions cache */
> + u8 gpio_value; /* pin value for outputs */
Your mixing tabs and spaces heavily above. Just stick to the current
style and drop the variable name alignment, remove superfluous spaces,
and use tabs only for indentation.
> +#endif
> };
>
> +#if defined(CONFIG_GPIOLIB)
> +static const char * const ftdi_ftx_gpio_names[] = {
> + "CBUS0", "CBUS1", "CBUS2", "CBUS3"
> +};
> +#endif
We want to keep the ifdeffery to a minimum, so move this inside the
gpiolib ifdef below (and possibly even into the function where it is
used).
Also note that these names are shared with FT232R, but not with FT232H.
> +
> /* struct ftdi_sio_quirk is used by devices requiring special attention. */
> struct ftdi_sio_quirk {
> int (*probe)(struct usb_serial *);
> @@ -1766,6 +1782,347 @@ static void remove_sysfs_attrs(struct usb_serial_port
> *port)
>
> }
>
> +#if defined(CONFIG_GPIOLIB)
> +
> +static int ftdi_set_bitmode_req(struct usb_serial_port *port, u8 mode)
Nit: please drop the _req suffix here.
> +{
> + struct ftdi_private *priv = usb_get_serial_port_data(port);
> + struct usb_serial *serial = port->serial;
> + int result;
> + u16 val;
> +
> + val = (mode << 8) | (priv->gpio_output << 4) | priv->gpio_value;
> + result = usb_control_msg(serial->dev,
> + usb_sndctrlpipe(serial->dev, 0),
> + FTDI_SIO_SET_BITM
Re: [PATCH 1/3] usb: gadget: uvc: Factor out video USB request queueing
Hi Laurent,
On 18/09/18 11:35, Laurent Pinchart wrote:
> USB requests for video data are queued from two different locations in
> the driver, with the same code block occurring twice. Factor it out to a
> function.
>
> Signed-off-by: Laurent Pinchart
Looks good, and simplifies locking. Win win.
Reviewed-by: Kieran Bingham
> ---
> drivers/usb/gadget/function/uvc_video.c | 30 --
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_video.c
> b/drivers/usb/gadget/function/uvc_video.c
> index d3567b90343a..a95c8e2364ed 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -125,6 +125,19 @@ uvc_video_encode_isoc(struct usb_request *req, struct
> uvc_video *video,
> * Request handling
> */
>
> +static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request
> *req)
> +{
> + int ret;
> +
> + ret = usb_ep_queue(video->ep, req, GFP_ATOMIC);
> + if (ret < 0) {
> + printk(KERN_INFO "Failed to queue request (%d).\n", ret);
> + usb_ep_set_halt(video->ep);
> + }
> +
> + return ret;
> +}
> +
> /*
> * I somehow feel that synchronisation won't be easy to achieve here. We have
> * three events that control USB requests submission:
> @@ -189,14 +202,13 @@ uvc_video_complete(struct usb_ep *ep, struct
> usb_request *req)
>
> video->encode(req, video, buf);
>
> - if ((ret = usb_ep_queue(ep, req, GFP_ATOMIC)) < 0) {
> - printk(KERN_INFO "Failed to queue request (%d).\n", ret);
> - usb_ep_set_halt(ep);
> - spin_unlock_irqrestore(&video->queue.irqlock, flags);
> + ret = uvcg_video_ep_queue(video, req);
> + spin_unlock_irqrestore(&video->queue.irqlock, flags);
> +
> + if (ret < 0) {
> uvcg_queue_cancel(queue, 0);
> goto requeue;
> }
> - spin_unlock_irqrestore(&video->queue.irqlock, flags);
>
> return;
>
> @@ -316,15 +328,13 @@ int uvcg_video_pump(struct uvc_video *video)
> video->encode(req, video, buf);
>
> /* Queue the USB request */
> - ret = usb_ep_queue(video->ep, req, GFP_ATOMIC);
> + ret = uvcg_video_ep_queue(video, req);
> + spin_unlock_irqrestore(&queue->irqlock, flags);
> +
> if (ret < 0) {
> - printk(KERN_INFO "Failed to queue request (%d)\n", ret);
> - usb_ep_set_halt(video->ep);
> - spin_unlock_irqrestore(&queue->irqlock, flags);
> uvcg_queue_cancel(queue, 0);
> break;
> }
> - spin_unlock_irqrestore(&queue->irqlock, flags);
> }
>
> spin_lock_irqsave(&video->req_lock, flags);
>
--
Regards
--
Kieran
Re: [PATCH] usb: dwc2: Fix HiKey regression caused by power_down feature
Hi John, On 9/24/2018 22:52, John Stultz wrote: > On Sun, Sep 23, 2018 at 10:57 PM, Artur Petrosyan > wrote: >> Hi John, >> >> On 9/21/2018 05:05, John Stultz wrote: >>> On Thu, Sep 20, 2018 at 7:17 AM, Artur Petrosyan >>> wrote: On 5/23/2018 01:57, John Stultz wrote: > Its done automatically, when the OTG cable is detected it the host > ports are disabled and when the OTG port is empty the host ports are > enabled. > > Let me know if you need anything else! > Please apply the patch set with this cover letter "[PATCH 0/3] usb: dwc2: Fix hibernation for switching between host and device modes." >>> >>> Sorry, can you send the patches to me, or point me to a git tree? I'm >>> not seeing that thread in my mailbox or on google. >>> Enable the power down on his devices. Let me know if you still see any issue. If there is no issue, please provide Tested-by tag. >>> >>> Would be happy to test it, thought I'm traveling tomorrow, so I may >>> not be able to validate till monday. >>> >>> thanks >>> -john >>> >> >> You can find the patch set following to this link. >> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Dusb-26m-3D153745139408236-26w-3D2&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=dfaz_WctFBdFgywM2g9w0XVhgOdAS4gIsj9o23RnyvY&s=_Nz3ephAop_nqrfcICii4OgMkV14Mi4yaKk8-qPqdis&e= > > I applied those three patches, and it seems to work ok. > > Just to be clear, was there anything else I was needing to do while testing > it? > > Otherwise, > Tested-by: John Stultz#On HiKey > > thanks > -john > Just a clarification by this commit "[PATCH] usb: dwc2: Fix HiKey regression caused by power_down feature" https://marc.info/?l=linux-usb&m=152669095513248&w=2 the power_down is disabled setting "p->power_down = false;" in "dwc2_set_his_params" function. Could you please clarify that the testes done for those 3 patches were done enabling "p->power_down = true;" in "dwc2_set_his_params" function. Regards, Artur
Re: [PATCH] xhci: Add check for invalid byte size error when UAS devices are connected.
On 9/24/2018 6:39 PM, Mathias Nyman wrote: > On 21.09.2018 16:22, Sandeep Singh wrote: >> From: Sandeep Singh >> >> Observed "TRB completion code (27)" error which corresponds to Stopped - >> Length Invalid error(xhci spec section 4.17.4) while connecting USB to >> SATA bridge. >> >> Looks like this case was not considered when the following patch[1] was >> committed. Hence adding this new check which can prevent >> the invalid byte size error. >> >> [1] ade2e3a xhci: handle transfer events without TRB pointer >> > > Thanks, adding to queue. > > Just out of curiosity, was the TRB pointer bits all zeroes > in this Stopped -Length invalid transfer event TRB? > Yes, TRB pointer bits all zeroes > -Mathias > Thanks Sandeep
Re: [PATCH 0/5] usb: renesas_usbhs: use otg mode and add support for R-Car E3
On Fri, Sep 21, 2018 at 09:26:27PM +0900, Yoshihiro Shimoda wrote: > This patch set is based on the latest Greg's usb.git / usb-testing branch > (the commit id is ae8a2ca8a2215c7e31e6d874f7303801bb15fbbc) > > The previous code set the mode as peripheral mode by the UGCTRL2 register > for R-Car D3. But, this SoC can select OTG mode in fact. So, at first, > I'd like to revert related patches I submitted in patch 1 and 2. > Then, in patch 3, it sets the mode as "OTG" and in patch 4 and 5, it > supports for R-Car E3. > > To use this controller for R-Car D3 and E3, we need the following > patch set. Otherwize, the mode will not be changed to peripheral mode > by the phy's COMMCTRL register: > https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=21629 Reviewed-by: Simon Horman
