[PATCH] usb: dwc3: pci: make better use of gpiod API

[Uwe Kleine-König]

Since 39b2bbe3d715 (gpio: add flags argument to gpiod_get*() functions)
which appeared in v3.17-rc1, the gpiod_get* functions take an additional
parameter that allows to specify direction and initial value for output.

Furthermore there is devm_gpiod_get_optional which is designed to get
optional gpios. Also use devm_gpiod_get* because otherwise the gpio
might be grabbed by a different driver.

Simplify driver accordingly.

Signed-off-by: Uwe Kleine-König 
---
Hello,

This usage without flags was introduced by commit a89d977cc04c (usb:
dwc3: pci: add quirk for Baytrails) that is currently in next.

Note I plan to make the flags parameter mandatory for 4.3. So unless
this change gets into 4.2, would it be ok to let it go in via the gpio
tree?

Best regards
Uwe

 drivers/usb/dwc3/dwc3-pci.c | 21 +++--
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 27e4fc896e9d..7e308730f955 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -84,18 +84,19 @@ static int dwc3_pci_quirks(struct pci_dev *pdev)
  acpi_dwc3_byt_gpios);
 
/* These GPIOs will turn on the USB2 PHY */
-   gpio = gpiod_get(&pdev->dev, "cs");
-   if (!IS_ERR(gpio)) {
-   gpiod_direction_output(gpio, 0);
-   gpiod_set_value_cansleep(gpio, 1);
-   gpiod_put(gpio);
-   }
+   gpio = devm_gpiod_get_optional(&pdev->dev, "cs", GPIOD_OUT_LOW);
+   if (IS_ERR(gpio))
+   return PTR_ERR(gpio);
+
+   gpiod_set_value_cansleep(gpio, 1);
+
+   gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
+  GPIOD_OUT_LOW);
+   if (IS_ERR(gpio))
+   return PTR_ERR(gpio);
 
-   gpio = gpiod_get(&pdev->dev, "reset");
-   if (!IS_ERR(gpio)) {
-   gpiod_direction_output(gpio, 0);
+   if (gpio) {
gpiod_set_value_cansleep(gpio, 1);
-   gpiod_put(gpio);
usleep_range(1, 11000);
}
}
-- 
2.1.4

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

Re: [PATCH v2] USB SERIAL: option.c: add 2020:4000 IDs

[Claudio Cappelli]

On Wednesday 10 June 2015 20:38:30 Claudio Cappelli wrote:
> From: Claudio Cappelli 
> 
> Add device Olivetti Olicard 300 (Network Connect: MT6225) - IDs 2020:4000.
> 
> Signed-off-by: Claudio Cappelli 
> Suggested-by: Lars Melin 
> 
> ---
> 
> drivers/usb/serial/option.c |1 +
>  1 file changed, 1 insertion(+)
> 
> 
> 
> --- linux/drivers/usb/serial/option.c.orig2015-06-10 10:42:43.0 
> +0200
> +++ linux/drivers/usb/serial/option.c 2015-06-10 10:53:06.825265579 +0200
> @@ -1765,6 +1765,7 @@ static const struct usb_device_id option
>   { USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d03, 0xff, 0x00, 0x00) },
>   { USB_DEVICE_AND_INTERFACE_INFO(0x07d1, 0x3e01, 0xff, 0xff, 0xff) }, /* 
> D-Link DWM-152/C1 */
>   { USB_DEVICE_AND_INTERFACE_INFO(0x07d1, 0x3e02, 0xff, 0xff, 0xff) }, /* 
> D-Link DWM-156/C1 */
> + { USB_DEVICE_INTERFACE_CLASS(0x2020, 0x4000, 0xff) },/* 
> OLICARD300 - MT6225 */
>   { USB_DEVICE(INOVIA_VENDOR_ID, INOVIA_SEW858) },
>   { USB_DEVICE(VIATELECOM_VENDOR_ID, VIATELECOM_PRODUCT_CDS7) },
>   { } /* Terminating entry */
>



Was this second version in the right format?
Since it was my first patch submit, I'd be
grateful if you could confirm.

Thanks!
See you

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

Re: [PATCH v2 05/22] doc: dt-binding: usb: add otg related properties

[Roger Quadros]

On Fri, 12 Jun 2015 09:42:04 +0800
Li Jun  wrote:

> On Thu, Jun 11, 2015 at 05:55:57PM +0300, Roger Quadros wrote:
> > > > drivers/usb/core/hub.c
> > > > 
> > > > static int usb_enumerate_device_otg(struct usb_device *udev)
> > > > {
> > > > int err = 0;
> > > > 
> > > > #ifdef  CONFIG_USB_OTG
> > > > /*
> > > >  * OTG-aware devices on OTG-capable root hubs may be able to 
> > > > use SRP,
> > > >  * to wake us after we've powered off VBUS; and HNP, switching 
> > > > roles
> > > >  * "host" to "peripheral".  The OTG descriptor helps figure 
> > > > this out.
> > > >  */
> > > > if (!udev->bus->is_b_host
> > > > && udev->config
> > > > && udev->parent == udev->bus->root_hub) {
> > > > struct usb_otg_descriptor   *desc = NULL;
> > > > struct usb_bus  *bus = udev->bus;
> > > > 
> > > > /* descriptor may appear anywhere in config */
> > > > if (__usb_get_extra_descriptor (udev->rawdescriptors[0],
> > > > 
> > > > le16_to_cpu(udev->config[0].desc.wTotalLength),
> > > > USB_DT_OTG, (void **) &desc) == 
> > > > 0) {
> > > > if (desc->bmAttributes & USB_OTG_HNP) {
> > > > unsignedport1 = 
> > > > udev->portnum;
> > > > 
> > > > dev_info(&udev->dev,
> > > > "Dual-Role OTG device on %sHNP 
> > > > port\n",
> > > > (port1 == bus->otg_port)
> > > > ? "" : "non-");
> > > > 
> > > > /* enable HNP before suspend, it's 
> > > > simpler */
> > > > if (port1 == bus->otg_port)
> > > > bus->b_hnp_enable = 1;
> > > > err = usb_control_msg(udev,
> > > > usb_sndctrlpipe(udev, 0),
> > > > USB_REQ_SET_FEATURE, 0,
> > > > bus->b_hnp_enable
> > > > ? 
> > > > USB_DEVICE_B_HNP_ENABLE
> > > > : 
> > > > USB_DEVICE_A_ALT_HNP_SUPPORT,
> > > > 0, NULL, 0, 
> > > > USB_CTRL_SET_TIMEOUT);
> > > > 
> > > > We're sending out this control request even if this host port is not 
> > > > OTG.
> > > > Isn't that wrong?
> > > 
> > > So send USB_DEVICE_A_ALT_HNP_SUPPORT request.
> > > This is correct in OTG 1.x, its intention is to remind the user who is
> > > connecting the HNP capable OTG device to a non-OTG port, He can change
> > > to another port of this machine which is a OTG port(with HNP).
> > 
> > I didn't understand.
> > If CONFIG_USB_OTG is enabled doesn't mean that this USB host port is OTG 
> > host.
> Yes, only CONFIG_USB_OTG enabled doesn't mean that this USB host port
> is a OTG port, there are might multiple usb host ports, but only one
> is a OTG port.

Not necessarily. Many systems don't have any OTG port so that request cannot
be sent even if CONFIG_USB_OTG is enabled.

> > So it should not send any OTG specific request to device. Right?
> Non-otg port(in OTG 1.x protocol) + OTG 1.x device, should send,
> otherwise, should not send.

What did you mean by Non-OTG port (in OTG 1.x protocol)?
If it is Non-OTG port it doesn't understand any OTG protocol.
So Non-OTG port should not send any OTG request.

> 
> So the code you pasted here was right only for OTG 1.x, I assume
> OTG 2.0 has not been released when it was designed. But now it's
> out of date, it's wrong if you connect a OTG 2.0 device. 
> 

The code is wrong for non-OTG ports when CONFIG_USB_OTG is set.

Peter/Felipe, any comments on this?

cheers,
-roger

> > 
> > > 
> > > > 
> > > > if (err < 0) {
> > > > /* OTG MESSAGE: report errors 
> > > > here,
> > > >  * customize to match your 
> > > > product.
> > > >  */
> > > > dev_info(&udev->dev,
> > > > "can't set HNP mode: 
> > > > %d\n",
> > > > err);
> > > > bus->b_hnp_enable = 0;
> > > > }
> > > > 
> > > > Instead it should be moved inside the if (port1 == bus->otg_port) 
> > > > condition.
> > > 
> > > Nope, as I explained above, this is really too detailed OTG protocol:)
> > > > 
> > > > }
> > > > }
> > > > }
> > > > #endif
> > > > r

Re: [PATCH v2 05/22] doc: dt-binding: usb: add otg related properties

[Li Jun]

On Fri, Jun 12, 2015 at 11:02:13AM +0300, Roger Quadros wrote:
> 
> On Fri, 12 Jun 2015 09:42:04 +0800
> Li Jun  wrote:
> 
> > On Thu, Jun 11, 2015 at 05:55:57PM +0300, Roger Quadros wrote:
> > > > > drivers/usb/core/hub.c
> > > > > 
> > > > > static int usb_enumerate_device_otg(struct usb_device *udev)
> > > > > {
> > > > >   int err = 0;
> > > > > 
> > > > > #ifdefCONFIG_USB_OTG
> > > > >   /*
> > > > >* OTG-aware devices on OTG-capable root hubs may be able to 
> > > > > use SRP,
> > > > >* to wake us after we've powered off VBUS; and HNP, switching 
> > > > > roles
> > > > >* "host" to "peripheral".  The OTG descriptor helps figure 
> > > > > this out.
> > > > >*/
> > > > >   if (!udev->bus->is_b_host
> > > > >   && udev->config
> > > > >   && udev->parent == udev->bus->root_hub) {
> > > > >   struct usb_otg_descriptor   *desc = NULL;
> > > > >   struct usb_bus  *bus = udev->bus;
> > > > > 
> > > > >   /* descriptor may appear anywhere in config */
> > > > >   if (__usb_get_extra_descriptor (udev->rawdescriptors[0],
> > > > >   
> > > > > le16_to_cpu(udev->config[0].desc.wTotalLength),
> > > > >   USB_DT_OTG, (void **) &desc) == 
> > > > > 0) {
> > > > >   if (desc->bmAttributes & USB_OTG_HNP) {
> > > > >   unsignedport1 = 
> > > > > udev->portnum;
> > > > > 
> > > > >   dev_info(&udev->dev,
> > > > >   "Dual-Role OTG device on %sHNP 
> > > > > port\n",
> > > > >   (port1 == bus->otg_port)
> > > > >   ? "" : "non-");
> > > > > 
> > > > >   /* enable HNP before suspend, it's 
> > > > > simpler */
> > > > >   if (port1 == bus->otg_port)
> > > > >   bus->b_hnp_enable = 1;
> > > > >   err = usb_control_msg(udev,
> > > > >   usb_sndctrlpipe(udev, 0),
> > > > >   USB_REQ_SET_FEATURE, 0,
> > > > >   bus->b_hnp_enable
> > > > >   ? 
> > > > > USB_DEVICE_B_HNP_ENABLE
> > > > >   : 
> > > > > USB_DEVICE_A_ALT_HNP_SUPPORT,
> > > > >   0, NULL, 0, 
> > > > > USB_CTRL_SET_TIMEOUT);
> > > > > 
> > > > > We're sending out this control request even if this host port is not 
> > > > > OTG.
> > > > > Isn't that wrong?
> > > > 
> > > > So send USB_DEVICE_A_ALT_HNP_SUPPORT request.
> > > > This is correct in OTG 1.x, its intention is to remind the user who is
> > > > connecting the HNP capable OTG device to a non-OTG port, He can change
> > > > to another port of this machine which is a OTG port(with HNP).
> > > 
> > > I didn't understand.
> > > If CONFIG_USB_OTG is enabled doesn't mean that this USB host port is OTG 
> > > host.
> > Yes, only CONFIG_USB_OTG enabled doesn't mean that this USB host port
> > is a OTG port, there are might multiple usb host ports, but only one
> > is a OTG port.
> 
> Not necessarily. Many systems don't have any OTG port so that request cannot
> be sent even if CONFIG_USB_OTG is enabled.
> 
That's the fact, but we are talking the code only for those systems which
have OTG port. If you think we need not cover the support for OTG 1.x host,
we can simply remove it.

> > > So it should not send any OTG specific request to device. Right?
> > Non-otg port(in OTG 1.x protocol) + OTG 1.x device, should send,
> > otherwise, should not send.
> 
> What did you mean by Non-OTG port (in OTG 1.x protocol)?

Means host only port without HNP.

> If it is Non-OTG port it doesn't understand any OTG protocol.

With common sense, yes, but there is one exception.

> So Non-OTG port should not send any OTG request.
> 
see below from OTG 1.3:

6.5.3 a_alt_hnp_support
Setting this feature indicates to the B-device that it is connected to
an A-device port that is not capable of HNP, but that the A-device does
have an alternate port that is capable of HNP.
The A-device is required to set this feature under the following conditions:
• the A-device has multiple receptacles
• the A-device port that connects to the B-device does not support HNP
• the A-device has another port that does support HNP
... ...

> > 
> > So the code you pasted here was right only for OTG 1.x, I assume
> > OTG 2.0 has not been released when it was designed. But now it's
> > out of date, it's wrong if you connect a OTG 2.0 device. 
> > 
> 
> The code is wrong for non-OTG ports when CONFIG_USB_OTG is set.
> 
> Peter/Felipe, any comments on this?
> 
> cheers,
> -roger
> 

Re: [PATCH v2 05/22] doc: dt-binding: usb: add otg related properties

[Roger Quadros]

On Fri, 12 Jun 2015 11:09:17 +0800
Li Jun  wrote:

> On Thu, Jun 11, 2015 at 05:52:15PM +0300, Roger Quadros wrote:
> > 
> > On Thu, 11 Jun 2015 22:11:22 +0800
> > Li Jun  wrote:
> > 
> > > On Thu, Jun 11, 2015 at 03:37:03PM +0300, Roger Quadros wrote:
> > > > 
> > > > On Thu, 11 Jun 2015 16:20:13 +0800
> > > > Li Jun  wrote:
> > > > 
> > > > > On Thu, Jun 11, 2015 at 10:18:53AM +0300, Roger Quadros wrote:
> > > > > > 
> > > > > > On Wed, 10 Jun 2015 21:47:51 +0800
> > > > > > Li Jun  wrote:
> > > > > > 
> > > > > > > On Wed, Jun 10, 2015 at 03:37:37PM +0800, Roger Quadros wrote:
> > > > > > > > On Tue, 9 Jun 2015 11:33:11 -0500
> > > > > > > > Rob Herring  wrote:
> > > > > > > > 
> > > > > > > > > On Tue, Jun 9, 2015 at 10:29 AM, Roger Quadros 
> > > > > > > > >  wrote:
> > > > > > > > > > Rob,
> > > > > > > > > >
> > > > > > > > > > On Tue, 9 Jun 2015 08:26:20 -0500
> > > > > > > > > > Rob Herring  wrote:
> > > > > > > > > >
> > > > > > > > > >> On Mon, Jun 8, 2015 at 8:18 PM, Li Jun 
> > > > > > > > > >>  wrote:
> > > > > > > > > >> > On Mon, Jun 08, 2015 at 11:06:49AM -0500, Rob Herring 
> > > > > > > > > >> > wrote:
> > > > > > > > > >> >> On Mon, Jun 8, 2015 at 10:02 AM, Li Jun 
> > > > > > > > > >> >>  wrote:
> > > > > > > > > >> >> > Add otg version, srp, hnp and adp support for usb OTG 
> > > > > > > > > >> >> > port, then those OTG
> > > > > > > > > >> >> > features don't have to be decided by usb gadget 
> > > > > > > > > >> >> > drivers.
> > > > > > > > > >> >> >
> > > > > > > > > >> >> > Signed-off-by: Li Jun 
> > > > > > > > > >> >> > ---
> > > > > > > > > >> >> >  Documentation/devicetree/bindings/usb/generic.txt | 
> > > > > > > > > >> >> > 10 ++
> > > > > > > > > >> >> >  1 file changed, 10 insertions(+)
> > > > > > > > > >> >> >
> > > > > > > > > >> >> > diff --git 
> > > > > > > > > >> >> > a/Documentation/devicetree/bindings/usb/generic.txt 
> > > > > > > > > >> >> > b/Documentation/devicetree/bindings/usb/generic.txt
> > > > > > > > > >> >> > index 477d5bb..7386f4a 100644
> > > > > > > > > >> >> > --- 
> > > > > > > > > >> >> > a/Documentation/devicetree/bindings/usb/generic.txt
> > > > > > > > > >> >> > +++ 
> > > > > > > > > >> >> > b/Documentation/devicetree/bindings/usb/generic.txt
> > > > > > > > > >> >> > @@ -11,6 +11,12 @@ Optional properties:
> > > > > > > > > >> >> > "peripheral" and "otg". In 
> > > > > > > > > >> >> > case this attribute isn't
> > > > > > > > > >> >> > passed via DT, USB DRD 
> > > > > > > > > >> >> > controllers should default to
> > > > > > > > > >> >> > OTG.
> > > > > > > > > >> >> > + - otg-rev: tells usb driver the release number of 
> > > > > > > > > >> >> > the OTG and EH supplement
> > > > > > > > > >> >> > +   with which the device and its 
> > > > > > > > > >> >> > descriptors are compliant,
> > > > > > > > > >> >> > +   in binary-coded decimal (i.e. 
> > > > > > > > > >> >> > 2.0 is 0200H).
> > > > > > > > > >> >>
> > > > > > > > > >> >> I would assume OTG 2.0 is somehow backwards compatible? 
> > > > > > > > > >> >> Is this a h/w
> > > > > > > > > >> >> dependency or a driver feature?
> > > > > > > > > >> >>
> > > > > > > > > >> > Not fully compatible, OTG 2.0 extend the 
> > > > > > > > > >> > usb_otg_descriptor by adding a new
> > > > > > > > > >> > member bcdOTG to identify the OTG version, this 
> > > > > > > > > >> > descriptor needs to be sent
> > > > > > > > > >> > to OTG host with correct size and content, so we have to 
> > > > > > > > > >> > know which release
> > > > > > > > > >> > version the OTG device is compliant with, either by 
> > > > > > > > > >> > menuconfig config or pass
> > > > > > > > > >> > via DT.
> > > > > > > > > >>
> > > > > > > > > >> So you have to change the version depending on the host 
> > > > > > > > > >> you are
> > > > > > > > > >> connected to? That really seems strange that plugging in a 
> > > > > > > > > >> OTG 2.0
> > > > > > > > > >> device to an OTG 1.3 host would not work and doesn't make 
> > > > > > > > > >> for a good
> > > > > > > > > >> user experience.
> > > > > > > > > >
> > > > > > > > > > No. The OTG version in the OTG descriptor for any device is 
> > > > > > > > > > usually fixed for the
> > > > > > > > > > lifetime of the product.
> > > > > > > > > >
> > > > > > > > > > Let's assume it is 2.0.
> > > > > > > > > >
> > > > > > > > > > If you plug this to OTG 1.0 host, it won't be an issue as 
> > > > > > > > > > OTG 1.0 host doesn't
> > > > > > > > > > read the BCD version.
> > > > > > > > > 
> > > > > > > > > That makes sense, but there was some discussion about the 
> > > > > > > > > size mattering.
> > > > > > > > > 
> > > > > > > > > So is there a reason not to always report 2.0 with any kernel 
> > > > > > > > > that has
> > > > > > > > > 2.0 support?
> > > > > > > > 
> > > > > > > > A 2.0 host would still need to know if the attached OTG device 
> > > > > > > > is 1

Re: [PATCH v2 05/22] doc: dt-binding: usb: add otg related properties

[Roger Quadros]

On Fri, 12 Jun 2015 16:23:59 +0800
Li Jun  wrote:

> On Fri, Jun 12, 2015 at 11:02:13AM +0300, Roger Quadros wrote:
> > 
> > On Fri, 12 Jun 2015 09:42:04 +0800
> > Li Jun  wrote:
> > 
> > > On Thu, Jun 11, 2015 at 05:55:57PM +0300, Roger Quadros wrote:
> > > > > > drivers/usb/core/hub.c
> > > > > > 
> > > > > > static int usb_enumerate_device_otg(struct usb_device *udev)
> > > > > > {
> > > > > > int err = 0;
> > > > > > 
> > > > > > #ifdef  CONFIG_USB_OTG
> > > > > > /*
> > > > > >  * OTG-aware devices on OTG-capable root hubs may be able to 
> > > > > > use SRP,
> > > > > >  * to wake us after we've powered off VBUS; and HNP, switching 
> > > > > > roles
> > > > > >  * "host" to "peripheral".  The OTG descriptor helps figure 
> > > > > > this out.
> > > > > >  */
> > > > > > if (!udev->bus->is_b_host
> > > > > > && udev->config
> > > > > > && udev->parent == udev->bus->root_hub) {
> > > > > > struct usb_otg_descriptor   *desc = NULL;
> > > > > > struct usb_bus  *bus = udev->bus;
> > > > > > 
> > > > > > /* descriptor may appear anywhere in config */
> > > > > > if (__usb_get_extra_descriptor (udev->rawdescriptors[0],
> > > > > > 
> > > > > > le16_to_cpu(udev->config[0].desc.wTotalLength),
> > > > > > USB_DT_OTG, (void **) &desc) == 
> > > > > > 0) {
> > > > > > if (desc->bmAttributes & USB_OTG_HNP) {
> > > > > > unsignedport1 = 
> > > > > > udev->portnum;
> > > > > > 
> > > > > > dev_info(&udev->dev,
> > > > > > "Dual-Role OTG device on %sHNP 
> > > > > > port\n",
> > > > > > (port1 == bus->otg_port)
> > > > > > ? "" : "non-");
> > > > > > 
> > > > > > /* enable HNP before suspend, it's 
> > > > > > simpler */
> > > > > > if (port1 == bus->otg_port)
> > > > > > bus->b_hnp_enable = 1;
> > > > > > err = usb_control_msg(udev,
> > > > > > usb_sndctrlpipe(udev, 0),
> > > > > > USB_REQ_SET_FEATURE, 0,
> > > > > > bus->b_hnp_enable
> > > > > > ? 
> > > > > > USB_DEVICE_B_HNP_ENABLE
> > > > > > : 
> > > > > > USB_DEVICE_A_ALT_HNP_SUPPORT,
> > > > > > 0, NULL, 0, 
> > > > > > USB_CTRL_SET_TIMEOUT);
> > > > > > 
> > > > > > We're sending out this control request even if this host port is 
> > > > > > not OTG.
> > > > > > Isn't that wrong?
> > > > > 
> > > > > So send USB_DEVICE_A_ALT_HNP_SUPPORT request.
> > > > > This is correct in OTG 1.x, its intention is to remind the user who is
> > > > > connecting the HNP capable OTG device to a non-OTG port, He can change
> > > > > to another port of this machine which is a OTG port(with HNP).
> > > > 
> > > > I didn't understand.
> > > > If CONFIG_USB_OTG is enabled doesn't mean that this USB host port is 
> > > > OTG host.
> > > Yes, only CONFIG_USB_OTG enabled doesn't mean that this USB host port
> > > is a OTG port, there are might multiple usb host ports, but only one
> > > is a OTG port.
> > 
> > Not necessarily. Many systems don't have any OTG port so that request cannot
> > be sent even if CONFIG_USB_OTG is enabled.
> > 
> That's the fact, but we are talking the code only for those systems which
> have OTG port. If you think we need not cover the support for OTG 1.x host,
> we can simply remove it.

We should not remove it but need to add the following check.

Send that request only if the system has at least one OTG (v1.x) port that is
capable of HNP _and_ this port is not that HNP capable port _and_
the connected device is OTG (v1.x)

> 
> > > > So it should not send any OTG specific request to device. Right?
> > > Non-otg port(in OTG 1.x protocol) + OTG 1.x device, should send,
> > > otherwise, should not send.
> > 
> > What did you mean by Non-OTG port (in OTG 1.x protocol)?
> 
> Means host only port without HNP.
> 
> > If it is Non-OTG port it doesn't understand any OTG protocol.
> 
> With common sense, yes, but there is one exception.
> 
> > So Non-OTG port should not send any OTG request.
> > 
> see below from OTG 1.3:
> 
> 6.5.3 a_alt_hnp_support
> Setting this feature indicates to the B-device that it is connected to
> an A-device port that is not capable of HNP, but that the A-device does
> have an alternate port that is capable of HNP.
> The A-device is required to set this feature under the following conditions:
> • the A-device has multiple receptacles
> • the A-device port that connects to th

Re: [PATCH v2 05/22] doc: dt-binding: usb: add otg related properties

[Roger Quadros]

On Fri, 12 Jun 2015 16:23:59 +0800
Li Jun  wrote:

> On Fri, Jun 12, 2015 at 11:02:13AM +0300, Roger Quadros wrote:
> > 
> > On Fri, 12 Jun 2015 09:42:04 +0800
> > Li Jun  wrote:
> > 
> > > On Thu, Jun 11, 2015 at 05:55:57PM +0300, Roger Quadros wrote:
> > > > > > drivers/usb/core/hub.c
> > > > > > 
> > > > > > static int usb_enumerate_device_otg(struct usb_device *udev)
> > > > > > {
> > > > > > int err = 0;
> > > > > > 
> > > > > > #ifdef  CONFIG_USB_OTG
> > > > > > /*
> > > > > >  * OTG-aware devices on OTG-capable root hubs may be able to 
> > > > > > use SRP,
> > > > > >  * to wake us after we've powered off VBUS; and HNP, switching 
> > > > > > roles
> > > > > >  * "host" to "peripheral".  The OTG descriptor helps figure 
> > > > > > this out.
> > > > > >  */
> > > > > > if (!udev->bus->is_b_host
> > > > > > && udev->config
> > > > > > && udev->parent == udev->bus->root_hub) {
> > > > > > struct usb_otg_descriptor   *desc = NULL;
> > > > > > struct usb_bus  *bus = udev->bus;
> > > > > > 
> > > > > > /* descriptor may appear anywhere in config */
> > > > > > if (__usb_get_extra_descriptor (udev->rawdescriptors[0],
> > > > > > 
> > > > > > le16_to_cpu(udev->config[0].desc.wTotalLength),
> > > > > > USB_DT_OTG, (void **) &desc) == 
> > > > > > 0) {
> > > > > > if (desc->bmAttributes & USB_OTG_HNP) {
> > > > > > unsignedport1 = 
> > > > > > udev->portnum;
> > > > > > 
> > > > > > dev_info(&udev->dev,
> > > > > > "Dual-Role OTG device on %sHNP 
> > > > > > port\n",
> > > > > > (port1 == bus->otg_port)
> > > > > > ? "" : "non-");
> > > > > > 
> > > > > > /* enable HNP before suspend, it's 
> > > > > > simpler */
> > > > > > if (port1 == bus->otg_port)
> > > > > > bus->b_hnp_enable = 1;
> > > > > > err = usb_control_msg(udev,
> > > > > > usb_sndctrlpipe(udev, 0),
> > > > > > USB_REQ_SET_FEATURE, 0,
> > > > > > bus->b_hnp_enable
> > > > > > ? 
> > > > > > USB_DEVICE_B_HNP_ENABLE
> > > > > > : 
> > > > > > USB_DEVICE_A_ALT_HNP_SUPPORT,
> > > > > > 0, NULL, 0, 
> > > > > > USB_CTRL_SET_TIMEOUT);
> > > > > > 
> > > > > > We're sending out this control request even if this host port is 
> > > > > > not OTG.
> > > > > > Isn't that wrong?
> > > > > 
> > > > > So send USB_DEVICE_A_ALT_HNP_SUPPORT request.
> > > > > This is correct in OTG 1.x, its intention is to remind the user who is
> > > > > connecting the HNP capable OTG device to a non-OTG port, He can change
> > > > > to another port of this machine which is a OTG port(with HNP).
> > > > 
> > > > I didn't understand.
> > > > If CONFIG_USB_OTG is enabled doesn't mean that this USB host port is 
> > > > OTG host.
> > > Yes, only CONFIG_USB_OTG enabled doesn't mean that this USB host port
> > > is a OTG port, there are might multiple usb host ports, but only one
> > > is a OTG port.
> > 
> > Not necessarily. Many systems don't have any OTG port so that request cannot
> > be sent even if CONFIG_USB_OTG is enabled.
> > 
> That's the fact, but we are talking the code only for those systems which
> have OTG port. If you think we need not cover the support for OTG 1.x host,
> we can simply remove it.
> 
> > > > So it should not send any OTG specific request to device. Right?
> > > Non-otg port(in OTG 1.x protocol) + OTG 1.x device, should send,
> > > otherwise, should not send.
> > 
> > What did you mean by Non-OTG port (in OTG 1.x protocol)?
> 
> Means host only port without HNP.
> 
> > If it is Non-OTG port it doesn't understand any OTG protocol.
> 
> With common sense, yes, but there is one exception.
> 
> > So Non-OTG port should not send any OTG request.
> > 
> see below from OTG 1.3:
> 
> 6.5.3 a_alt_hnp_support
> Setting this feature indicates to the B-device that it is connected to
> an A-device port that is not capable of HNP, but that the A-device does
> have an alternate port that is capable of HNP.
> The A-device is required to set this feature under the following conditions:
> • the A-device has multiple receptacles
> • the A-device port that connects to the B-device does not support HNP
> • the A-device has another port that does support HNP
> ... ...
> 
> > > 
> > > So the code you pasted here was right only for OTG 1.x, I assume
> > > OTG 2.0 has not been released when it was designed. But no

[PATCH v2 3/3] usb: dwc2: embed storage for reg backup in struct dwc2_hsotg

[Mian Yousaf Kaukab]

Register backup function can be called from atomic context. Instead
of using atomic memory pool, embed backup storage space in
struct dwc2_hsotg.

Also add a valid flag in each struct as NULL pointer can't be used as
the content validity check any more.

Acked-by: John Youn 
Tested-by: Heiko Stuebner 
Signed-off-by: Mian Yousaf Kaukab 
---
 drivers/usb/dwc2/core.c | 55 ++---
 drivers/usb/dwc2/core.h |  9 +---
 2 files changed, 21 insertions(+), 43 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index e5b546f..c3cc1a7 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -72,17 +72,7 @@ static int dwc2_backup_host_registers(struct dwc2_hsotg 
*hsotg)
dev_dbg(hsotg->dev, "%s\n", __func__);
 
/* Backup Host regs */
-   hr = hsotg->hr_backup;
-   if (!hr) {
-   hr = devm_kzalloc(hsotg->dev, sizeof(*hr), GFP_KERNEL);
-   if (!hr) {
-   dev_err(hsotg->dev, "%s: can't allocate host regs\n",
-   __func__);
-   return -ENOMEM;
-   }
-
-   hsotg->hr_backup = hr;
-   }
+   hr = &hsotg->hr_backup;
hr->hcfg = readl(hsotg->regs + HCFG);
hr->haintmsk = readl(hsotg->regs + HAINTMSK);
for (i = 0; i < hsotg->core_params->host_channels; ++i)
@@ -90,6 +80,7 @@ static int dwc2_backup_host_registers(struct dwc2_hsotg 
*hsotg)
 
hr->hprt0 = readl(hsotg->regs + HPRT0);
hr->hfir = readl(hsotg->regs + HFIR);
+   hr->valid = true;
 
return 0;
 }
@@ -109,12 +100,13 @@ static int dwc2_restore_host_registers(struct dwc2_hsotg 
*hsotg)
dev_dbg(hsotg->dev, "%s\n", __func__);
 
/* Restore host regs */
-   hr = hsotg->hr_backup;
-   if (!hr) {
+   hr = &hsotg->hr_backup;
+   if (!hr->valid) {
dev_err(hsotg->dev, "%s: no host registers to restore\n",
__func__);
return -EINVAL;
}
+   hr->valid = false;
 
writel(hr->hcfg, hsotg->regs + HCFG);
writel(hr->haintmsk, hsotg->regs + HAINTMSK);
@@ -152,17 +144,7 @@ static int dwc2_backup_device_registers(struct dwc2_hsotg 
*hsotg)
dev_dbg(hsotg->dev, "%s\n", __func__);
 
/* Backup dev regs */
-   dr = hsotg->dr_backup;
-   if (!dr) {
-   dr = devm_kzalloc(hsotg->dev, sizeof(*dr), GFP_KERNEL);
-   if (!dr) {
-   dev_err(hsotg->dev, "%s: can't allocate device regs\n",
-   __func__);
-   return -ENOMEM;
-   }
-
-   hsotg->dr_backup = dr;
-   }
+   dr = &hsotg->dr_backup;
 
dr->dcfg = readl(hsotg->regs + DCFG);
dr->dctl = readl(hsotg->regs + DCTL);
@@ -195,7 +177,7 @@ static int dwc2_backup_device_registers(struct dwc2_hsotg 
*hsotg)
dr->doeptsiz[i] = readl(hsotg->regs + DOEPTSIZ(i));
dr->doepdma[i] = readl(hsotg->regs + DOEPDMA(i));
}
-
+   dr->valid = true;
return 0;
 }
 
@@ -215,12 +197,13 @@ static int dwc2_restore_device_registers(struct 
dwc2_hsotg *hsotg)
dev_dbg(hsotg->dev, "%s\n", __func__);
 
/* Restore dev regs */
-   dr = hsotg->dr_backup;
-   if (!dr) {
+   dr = &hsotg->dr_backup;
+   if (!dr->valid) {
dev_err(hsotg->dev, "%s: no device registers to restore\n",
__func__);
return -EINVAL;
}
+   dr->valid = false;
 
writel(dr->dcfg, hsotg->regs + DCFG);
writel(dr->dctl, hsotg->regs + DCTL);
@@ -268,17 +251,7 @@ static int dwc2_backup_global_registers(struct dwc2_hsotg 
*hsotg)
int i;
 
/* Backup global regs */
-   gr = hsotg->gr_backup;
-   if (!gr) {
-   gr = devm_kzalloc(hsotg->dev, sizeof(*gr), GFP_KERNEL);
-   if (!gr) {
-   dev_err(hsotg->dev, "%s: can't allocate global regs\n",
-   __func__);
-   return -ENOMEM;
-   }
-
-   hsotg->gr_backup = gr;
-   }
+   gr = &hsotg->gr_backup;
 
gr->gotgctl = readl(hsotg->regs + GOTGCTL);
gr->gintmsk = readl(hsotg->regs + GINTMSK);
@@ -291,6 +264,7 @@ static int dwc2_backup_global_registers(struct dwc2_hsotg 
*hsotg)
for (i = 0; i < MAX_EPS_CHANNELS; i++)
gr->dtxfsiz[i] = readl(hsotg->regs + DPTXFSIZN(i));
 
+   gr->valid = true;
return 0;
 }
 
@@ -309,12 +283,13 @@ static int dwc2_restore_global_registers(struct 
dwc2_hsotg *hsotg)
dev_dbg(hsotg->dev, "%s\n", __func__);
 
/* Restore global regs */
-   gr = hsotg->gr_backup;
-   if (!gr) {
+   gr = &hsotg->gr_backup;
+   if (!gr->valid) {
dev_err(hsotg->dev, "%s: no global registers to resto

[PATCH v2 0/3] usb: dwc2: fix sleep while atomic bugs

[Mian Yousaf Kaukab]

This series fixes 3 sources of sleep while atomic bugs. Including
the one reported by Heiko Stuebner here:
http://www.spinics.net/lists/linux-usb/msg125186.html

Please review.

Thank you,

Best regards,
Yousaf

History:
v2:
 - Fixed John's comment
v1:
 - Added John's Acked-by

Mian Yousaf Kaukab (3):
  usb: dwc2: host: allocate qh before atomic enqueue
  usb: dwc2: host: allocate qtd before atomic enqueue
  usb: dwc2: embed storage for reg backup in struct dwc2_hsotg

 drivers/usb/dwc2/core.c  | 55 
 drivers/usb/dwc2/core.h  |  9 +---
 drivers/usb/dwc2/hcd.c   | 55 +---
 drivers/usb/dwc2/hcd.h   |  5 +++-
 drivers/usb/dwc2/hcd_queue.c | 49 ++-
 5 files changed, 79 insertions(+), 94 deletions(-)

-- 
2.3.3

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

[PATCH v2 1/3] usb: dwc2: host: allocate qh before atomic enqueue

[Mian Yousaf Kaukab]

To avoid sleep while atomic bugs, allocate qh before calling
dwc2_hcd_urb_enqueue. qh pointer can be used directly now instead of
passing ep->hcpriv as double pointer.

Acked-by: John Youn 
Tested-by: Heiko Stuebner 
Signed-off-by: Mian Yousaf Kaukab 
---
 drivers/usb/dwc2/hcd.c   | 31 
 drivers/usb/dwc2/hcd.h   |  5 -
 drivers/usb/dwc2/hcd_queue.c | 49 +++-
 3 files changed, 43 insertions(+), 42 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index b10377c..80bce71 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -359,7 +359,7 @@ void dwc2_hcd_stop(struct dwc2_hsotg *hsotg)
 
 /* Caller must hold driver lock */
 static int dwc2_hcd_urb_enqueue(struct dwc2_hsotg *hsotg,
-   struct dwc2_hcd_urb *urb, void **ep_handle,
+   struct dwc2_hcd_urb *urb, struct dwc2_qh *qh,
gfp_t mem_flags)
 {
struct dwc2_qtd *qtd;
@@ -391,8 +391,7 @@ static int dwc2_hcd_urb_enqueue(struct dwc2_hsotg *hsotg,
return -ENOMEM;
 
dwc2_hcd_qtd_init(qtd, urb);
-   retval = dwc2_hcd_qtd_add(hsotg, qtd, (struct dwc2_qh **)ep_handle,
- mem_flags);
+   retval = dwc2_hcd_qtd_add(hsotg, qtd, qh);
if (retval) {
dev_err(hsotg->dev,
"DWC OTG HCD URB Enqueue failed adding QTD. Error 
status %d\n",
@@ -2445,6 +2444,8 @@ static int _dwc2_hcd_urb_enqueue(struct usb_hcd *hcd, 
struct urb *urb,
u32 tflags = 0;
void *buf;
unsigned long flags;
+   struct dwc2_qh *qh;
+   bool qh_allocated = false;
 
if (dbg_urb(urb)) {
dev_vdbg(hsotg->dev, "DWC OTG HCD URB Enqueue\n");
@@ -2523,13 +2524,24 @@ static int _dwc2_hcd_urb_enqueue(struct usb_hcd *hcd, 
struct urb *urb,
 urb->iso_frame_desc[i].length);
 
urb->hcpriv = dwc2_urb;
+   qh = (struct dwc2_qh *) ep->hcpriv;
+   /* Create QH for the endpoint if it doesn't exist */
+   if (!qh) {
+   qh = dwc2_hcd_qh_create(hsotg, dwc2_urb, mem_flags);
+   if (!qh) {
+   retval = -ENOMEM;
+   goto fail0;
+   }
+   ep->hcpriv = qh;
+   qh_allocated = true;
+   }
 
spin_lock_irqsave(&hsotg->lock, flags);
retval = usb_hcd_link_urb_to_ep(hcd, urb);
if (retval)
goto fail1;
 
-   retval = dwc2_hcd_urb_enqueue(hsotg, dwc2_urb, &ep->hcpriv, mem_flags);
+   retval = dwc2_hcd_urb_enqueue(hsotg, dwc2_urb, qh, mem_flags);
if (retval)
goto fail2;
 
@@ -2549,6 +2561,17 @@ fail2:
 fail1:
spin_unlock_irqrestore(&hsotg->lock, flags);
urb->hcpriv = NULL;
+   if (qh_allocated) {
+   struct dwc2_qtd *qtd2, *qtd2_tmp;
+
+   ep->hcpriv = NULL;
+   dwc2_hcd_qh_unlink(hsotg, qh);
+   /* Free each QTD in the QH's QTD list */
+   list_for_each_entry_safe(qtd2, qtd2_tmp, &qh->qtd_list,
+qtd_list_entry)
+   dwc2_hcd_qtd_unlink_and_free(hsotg, qtd2, qh);
+   dwc2_hcd_qh_free(hsotg, qh);
+   }
 fail0:
kfree(dwc2_urb);
 
diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
index 7b5841c..fc10549 100644
--- a/drivers/usb/dwc2/hcd.h
+++ b/drivers/usb/dwc2/hcd.h
@@ -463,6 +463,9 @@ extern void dwc2_hcd_queue_transactions(struct dwc2_hsotg 
*hsotg,
 /* Schedule Queue Functions */
 /* Implemented in hcd_queue.c */
 extern void dwc2_hcd_init_usecs(struct dwc2_hsotg *hsotg);
+extern struct dwc2_qh *dwc2_hcd_qh_create(struct dwc2_hsotg *hsotg,
+ struct dwc2_hcd_urb *urb,
+ gfp_t mem_flags);
 extern void dwc2_hcd_qh_free(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh);
 extern int dwc2_hcd_qh_add(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh);
 extern void dwc2_hcd_qh_unlink(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh);
@@ -471,7 +474,7 @@ extern void dwc2_hcd_qh_deactivate(struct dwc2_hsotg 
*hsotg, struct dwc2_qh *qh,
 
 extern void dwc2_hcd_qtd_init(struct dwc2_qtd *qtd, struct dwc2_hcd_urb *urb);
 extern int dwc2_hcd_qtd_add(struct dwc2_hsotg *hsotg, struct dwc2_qtd *qtd,
-   struct dwc2_qh **qh, gfp_t mem_flags);
+   struct dwc2_qh *qh);
 
 /* Unlinks and frees a QTD */
 static inline void dwc2_hcd_qtd_unlink_and_free(struct dwc2_hsotg *hsotg,
diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index 9b5c362..3ad63d3 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -191,7 +191,7 @@ static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct 
dwc2_qh *qh,
  *
  * Return: Poin

[PATCH v2 2/3] usb: dwc2: host: allocate qtd before atomic enqueue

[Mian Yousaf Kaukab]

To avoid sleep while atomic bugs, allocate qtd before calling
dwc2_hcd_urb_enqueue. No need to pass mem_flags to
dwc2_hcd_urb_enqueue any more as no memory allocations are done in it.

Acked-by: John Youn 
Tested-by: Heiko Stuebner 
Signed-off-by: Mian Yousaf Kaukab 
---
 drivers/usb/dwc2/hcd.c | 26 --
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 80bce71..f845c41 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -360,9 +360,8 @@ void dwc2_hcd_stop(struct dwc2_hsotg *hsotg)
 /* Caller must hold driver lock */
 static int dwc2_hcd_urb_enqueue(struct dwc2_hsotg *hsotg,
struct dwc2_hcd_urb *urb, struct dwc2_qh *qh,
-   gfp_t mem_flags)
+   struct dwc2_qtd *qtd)
 {
-   struct dwc2_qtd *qtd;
u32 intr_mask;
int retval;
int dev_speed;
@@ -386,9 +385,8 @@ static int dwc2_hcd_urb_enqueue(struct dwc2_hsotg *hsotg,
return -ENODEV;
}
 
-   qtd = kzalloc(sizeof(*qtd), mem_flags);
if (!qtd)
-   return -ENOMEM;
+   return -EINVAL;
 
dwc2_hcd_qtd_init(qtd, urb);
retval = dwc2_hcd_qtd_add(hsotg, qtd, qh);
@@ -396,7 +394,6 @@ static int dwc2_hcd_urb_enqueue(struct dwc2_hsotg *hsotg,
dev_err(hsotg->dev,
"DWC OTG HCD URB Enqueue failed adding QTD. Error 
status %d\n",
retval);
-   kfree(qtd);
return retval;
}
 
@@ -2446,6 +2443,7 @@ static int _dwc2_hcd_urb_enqueue(struct usb_hcd *hcd, 
struct urb *urb,
unsigned long flags;
struct dwc2_qh *qh;
bool qh_allocated = false;
+   struct dwc2_qtd *qtd;
 
if (dbg_urb(urb)) {
dev_vdbg(hsotg->dev, "DWC OTG HCD URB Enqueue\n");
@@ -2536,14 +2534,20 @@ static int _dwc2_hcd_urb_enqueue(struct usb_hcd *hcd, 
struct urb *urb,
qh_allocated = true;
}
 
+   qtd = kzalloc(sizeof(*qtd), mem_flags);
+   if (!qtd) {
+   retval = -ENOMEM;
+   goto fail1;
+   }
+
spin_lock_irqsave(&hsotg->lock, flags);
retval = usb_hcd_link_urb_to_ep(hcd, urb);
if (retval)
-   goto fail1;
+   goto fail2;
 
-   retval = dwc2_hcd_urb_enqueue(hsotg, dwc2_urb, qh, mem_flags);
+   retval = dwc2_hcd_urb_enqueue(hsotg, dwc2_urb, qh, qtd);
if (retval)
-   goto fail2;
+   goto fail3;
 
if (alloc_bandwidth) {
dwc2_allocate_bus_bandwidth(hcd,
@@ -2555,12 +2559,14 @@ static int _dwc2_hcd_urb_enqueue(struct usb_hcd *hcd, 
struct urb *urb,
 
return 0;
 
-fail2:
+fail3:
dwc2_urb->priv = NULL;
usb_hcd_unlink_urb_from_ep(hcd, urb);
-fail1:
+fail2:
spin_unlock_irqrestore(&hsotg->lock, flags);
urb->hcpriv = NULL;
+   kfree(qtd);
+fail1:
if (qh_allocated) {
struct dwc2_qtd *qtd2, *qtd2_tmp;
 
-- 
2.3.3

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

Re: [PATCH v2 05/22] doc: dt-binding: usb: add otg related properties

[Li Jun]

On Fri, Jun 12, 2015 at 11:41:40AM +0300, Roger Quadros wrote:
> 
> 
> On Fri, 12 Jun 2015 16:23:59 +0800
> Li Jun  wrote:
> 
> > On Fri, Jun 12, 2015 at 11:02:13AM +0300, Roger Quadros wrote:
> > > 
> > > On Fri, 12 Jun 2015 09:42:04 +0800
> > > Li Jun  wrote:
> > > 
> > > > On Thu, Jun 11, 2015 at 05:55:57PM +0300, Roger Quadros wrote:
> > > > > > > drivers/usb/core/hub.c
> > > > > > > 
> > > > > > > static int usb_enumerate_device_otg(struct usb_device *udev)
> > > > > > > {
> > > > > > >   int err = 0;
> > > > > > > 
> > > > > > > #ifdefCONFIG_USB_OTG

> > > 
> > > Not necessarily. Many systems don't have any OTG port so that request 
> > > cannot
> > > be sent even if CONFIG_USB_OTG is enabled.
> > > 
> > That's the fact, but we are talking the code only for those systems which
> > have OTG port. If you think we need not cover the support for OTG 1.x host,
> > we can simply remove it.
> 
> We should not remove it but need to add the following check.
> 
> Send that request only if the system has at least one OTG (v1.x) port that is
> capable of HNP _and_ this port is not that HNP capable port _and_
> the connected device is OTG (v1.x)
> 
Agreed.

> > 
> > > > > So it should not send any OTG specific request to device. Right?
> > > > Non-otg port(in OTG 1.x protocol) + OTG 1.x device, should send,
> > > > otherwise, should not send.
> > > 
> > > What did you mean by Non-OTG port (in OTG 1.x protocol)?
> > 
> > Means host only port without HNP.
> > 
> > > If it is Non-OTG port it doesn't understand any OTG protocol.
> > 
> > With common sense, yes, but there is one exception.
> > 
> > > So Non-OTG port should not send any OTG request.
> > > 
> > see below from OTG 1.3:
> > 
> > 6.5.3 a_alt_hnp_support
> > Setting this feature indicates to the B-device that it is connected to
> > an A-device port that is not capable of HNP, but that the A-device does
> > have an alternate port that is capable of HNP.
> > The A-device is required to set this feature under the following conditions:
> > • the A-device has multiple receptacles
> > • the A-device port that connects to the B-device does not support HNP
> > • the A-device has another port that does support HNP
> > ... ...
> 
> Thanks for this info. I can't seem to find the OTG v1.3 spec.
> 
You can download from:
http://www.usb.org/developers/docs/USB_OTG_1-3.pdf

> cheers,
> -roger
> 
> > 
> > > > 
> > > > So the code you pasted here was right only for OTG 1.x, I assume
> > > > OTG 2.0 has not been released when it was designed. But now it's
> > > > out of date, it's wrong if you connect a OTG 2.0 device. 
> > > > 
> > > 
> > > The code is wrong for non-OTG ports when CONFIG_USB_OTG is set.
> > > 
> > > Peter/Felipe, any comments on this?
> > > 
> > > cheers,
> > > -roger
> > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > >   if (err < 0) {
> > > > > > >   /* OTG MESSAGE: report errors 
> > > > > > > here,
> > > > > > >* customize to match your 
> > > > > > > product.
> > > > > > >*/
> > > > > > >   dev_info(&udev->dev,
> > > > > > >   "can't set HNP mode: 
> > > > > > > %d\n",
> > > > > > >   err);
> > > > > > >   bus->b_hnp_enable = 0;
> > > > > > >   }
> > > > > > > 
> > > > > > > Instead it should be moved inside the if (port1 == bus->otg_port) 
> > > > > > > condition.
> > > > > > 
> > > > > > Nope, as I explained above, this is really too detailed OTG 
> > > > > > protocol:)
> > > > > > > 
> > > > > > >   }
> > > > > > >   }
> > > > > > >   }
> > > > > > > #endif
> > > > > > >   return err;
> > > > > > > }
> > > > > > > 
> > > 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] usb/at91: usb hub does not work

[Jiří Prchal]

On 11.6.2015 15:53, Alan Stern wrote:

On Thu, 11 Jun 2015, Jiří Prchal wrote:


Hi all,
I discovered some bug when I change kernel from 3.18.13 to 3.18.14. I have 
board with usb hub CY7C65632 on it.
In .13 it works fine but in .14 it repeats this message:
[   19.17] usb 2-3: new full-speed USB device number 56 using at91_ohci
and devices connected to usb through hub doesn't appear at all.



Any idea?


Try using git bisect to find the commit which caused this problem to
start.


This is result:
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[ae74ea64ccdb8b99ee2618b58020263d5b1d9b22] clk: at91: usb: propagate rate 
modification to the parent clk

Compilation has some warnings:
  CC  drivers/clk/at91/clk-usb.o
drivers/clk/at91/clk-usb.c:157:2: warning: initialization from incompatible 
pointer type [enabled by default]
drivers/clk/at91/clk-usb.c:157:2: warning: (near initialization for ‘at91sam9x5_usb_ops.determine_rate’) [enabled by 
default]

drivers/clk/at91/clk-usb.c:195:2: warning: initialization from incompatible 
pointer type [enabled by default]
drivers/clk/at91/clk-usb.c:195:2: warning: (near initialization for ‘at91sam9n12_usb_ops.determine_rate’) [enabled by 
default]


And kernel hangs up with this:
[4.48] ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver
[4.48] ehci-atmel: EHCI Atmel driver
[4.49] Unable to handle kernel paging request at virtual address 
1c9c3800
[4.50] pgd = c0004000
[4.50] [1c9c3800] *pgd=
[4.50] Internal error: Oops: 805 [#1] PREEMPT ARM
[4.50] Modules linked in:
[4.50] CPU: 0 PID: 1 Comm: swapper Tainted: GW  
3.18.13_cpm9g25+ #10
[4.50] task: c7821b00 ti: c7828000 task.ti: c7828000
[4.50] PC is at at91sam9x5_clk_usb_determine_rate+0xb8/0x120
[4.50] LR is at at91sam9x5_clk_usb_determine_rate+0xb4/0x120
[4.50] pc : []lr : []psr: a013
[4.50] sp : c7829d28  ip : c7829d28  fp : c7829d64
[4.50] r10: c7802ae0  r9 : c7806400  r8 : 02dc6c00
[4.50] r7 :   r6 : 0001  r5 :   r4 : 02dc6c00
[4.50] r3 : 17d78400  r2 : 1c9c3800  r1 : 14fb1800  r0 : c7802860
[4.50] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment 
kernel
[4.50] Control: 0005317f  Table: 20004000  DAC: 0017
[4.50] Process swapper (pid: 1, stack limit = 0xc78281b8)
[4.50] Stack: (0xc7829d28 to 0xc782a000)
[4.50] 9d20:    17d78400 14fb1800 17d78400 
c7a26400 c7806580
[4.50] 9d40: c7806480 c0731fb8 c7a26400 0026  c06a7dd0 
c7829d8c c7829d68
[4.50] 9d60: c038d574 c0392e94 c7806480 1c9c3800 c7806580 02dc6c00 
c0731fb8 c7a26400
[4.50] 9d80: c7829da4 c7829d90 c038ded8 c038d50c c79a5400 c79a5410 
c7829dcc c7829da8
[4.50] 9da0: c033e620 c038de8c c033e458 c79a5410 ffed c06ec394 
 c0696f58
[4.50] 9dc0: c7829dec c7829dd0 c02bd9e4 c033e468 c02bd98c c79a5410 
 c06ec394
[4.50] 9de0: c7829e0c c7829df0 c02bbe80 c02bd99c c79a5410 c79a5444 
c06ec394 c02bbfcc
[4.50] 9e00: c7829e2c c7829e10 c02bc044 c02bbdd0 c02bbfcc  
c7829e30 c06ec394
[4.50] 9e20: c7829e54 c7829e30 c02ba428 c02bbfdc c78687ec c799b490 
c06ec394 c79e4d00
[4.50] 9e40:  c06e4038 c7829e64 c7829e58 c02bb984 c02ba3d4 
c7829e8c c7829e68
[4.50] 9e60: c02bb5f8 c02bb96c c05d0c60 c7829e78 c06ec394 c06cd5d8 
c06cd5d8 c06ff700
[4.50] 9e80: c7829ea4 c7829e90 c02bca48 c02bb528  c06cd5d8 
c7829eb4 c7829ea8
[4.50] 9ea0: c02bd840 c02bc9ac c7829ecc c7829eb8 c0696f94 c02bd7f8 
 c7a31dc0
[4.50] 9ec0: c7829f54 c7829ed0 c0008acc c0696f68  c0042664 
 c0034c1c
[4.50] 9ee0: c7ffc978 c7ffc900 c7829f0c c7829ef8 c0034c1c c0034ba4 
c7ffc975 
[4.50] 9f00: c7829f54 c7829f10 c0034e48 c06745f4 c7829f2c c060ba60 
0006 0006
[4.50] 9f20: 007b 1aadfd66 c7829f54 0006 c06a7dc4 c06c0df4 
c06ff700 007b
[4.50] 9f40:  c06a7dd0 c7829f94 c7829f58 c0674de4 c00089c8 
0006 0006
[4.50] 9f60: c06745e4 c003bb38 c7829f94 c7829f78  c04989a0 
 
[4.50] 9f80:   c7829fac c7829f98 c04989b8 c0674d00 
c7828000 
[4.50] 9fa0:  c7829fb0 c0009790 c04989b0   
 
[4.50] 9fc0:       
 
[4.50] 9fe0:     0013  
a8210092 a39c4870
[4.50] [] (at91sam9x5_clk_usb_determine_rate) from 
[] (clk_calc_new_rates+0x78/0x174)
[4.50] [] (clk_calc_new_rates) from [] 
(clk_set_rate+0x5c/0xa4)
[4.50] [] (clk_set_rate) from [] 
(ehci_atmel_drv_probe+0x1c8/0x308)
[4.50] [] (ehci_atmel_drv_probe) from [] 
(platform_drv_probe+0x58/0xa0)
[4.50] [

Re: [PATCH RESEND (1-3)/4] Add usb interface authorization

[Stefan Koch]

Am Dienstag, den 09.06.2015, 13:20 -0400 schrieb Alan Stern:
> On Tue, 9 Jun 2015, Stefan Koch wrote:
> 
> > Am Dienstag, den 09.06.2015, 10:31 -0400 schrieb Alan Stern:
> > > On Tue, 9 Jun 2015, Stefan Koch wrote:
> > > 
> > > > Hi
> > > > 
> > > > I think whitespaces should be ok now. I have tested now three mail
> > > > clients...
> > > > 
> > > > This patch series enables the interface authorization. The description
> > > > is improved now. Is the series correct now?
> > > 
> > > This patch series is much, much more complicated than necessary.
> > > 
> > > > The first patch allows to handle the probing of drivers
> > > >  manually.
> > > 
> > > Why do you do that?  It's completely unrelated to the problem of
> > > authorizing USB interfaces.
> > To prevent drivers from binding.
> 
> But you don't care about all drivers in general; you only care about
> drivers binding to USB interfaces.  So you should change only the USB 
> binding routines, not the routines in drivers/base/bus.c.
> 
> > > > And the third patch introduces the usb interface authorization. This is
> > > > the main concern of this kernel modification.
> > > 
> > > This patch also is more complicated than it should be.  Why do you want
> > > to unregister and then re-register the interfaces?  All you need to do
> > > is unbind drivers and prevent other drivers from binding.
> > Do you see a much simplier method to prevent drivers from binding as
> > with patch 1?
> 
> Yes, and you already included it in your 3/4 patch -- the changes you 
> made to usb_probe_interface() and usb_claim_interface().
> 
> > Another approach could be to check in bus_probe_device() (bus.c) if the
> > bus is "usb" and then use to_usb_interface() or so on and check the intf
> > auth status bit.
> 
> Again, why bother?  It's silly to check for the usb bus in 
> bus_probe_device(), when usb_probe_interface() already _knows_ that the 
> bus is usb.
> 
> Alan Stern
> 
There is one problem with that simple approach without the manually
probe patch. As example the USB-Tethering with smartphones.

There is a driver (module) loading. It is loaded although
usb_probe_interface() denies the interface.

I think that's not so good that there are unused modules loaded.
So the approach with the manually probing possibility could solve that.

An other approach could be to make a device_add_opt_probe(DEVICE,
BOOLEAN), because that is used in usb_set_configuration(). Inside of
device_add() is the function bus_probe_device(). These could disabled
with unauthorized interfaces by the BOOLEAN parameter.

In usb_register_driver() it is not possible to detect an interface.

What do you think?

Thanks

Stefan Koch

dmesg with dump printing:

[  125.011443] usb 3-3: new high-speed USB device number 6 using
xhci_hcd
[  125.176930] usb 3-3: New USB device found, idVendor=04e8,
idProduct=6864
[  125.176934] usb 3-3: New USB device strings: Mfr=1, Product=2,
SerialNumber=3
[  125.176936] usb 3-3: Product: SAMSUNG_Android
[  125.176937] usb 3-3: Manufacturer: SAMSUNG
[  125.176939] usb 3-3: SerialNumber: 10850d17
[  125.231054] CPU: 5 PID: 1891 Comm: systemd-udevd Not tainted
4.1.0-rc7-21-desktop+ #8
[  125.231060]   880259253c78 8169e8b7
8001
[  125.231065]  a0a0a000 880259253ca8 814f3b65
81c15020
[  125.231068]  880264597e20  a0a0d000
880259253cb8
[  125.231071] Call Trace:
[  125.231079]  [] dump_stack+0x4c/0x6e
[  125.231084]  [] usb_register_driver+0x115/0x170
[  125.231091]  [] ? 0xa0a0d000
[  125.231094]  [] cdc_driver_init+0x1e/0x1000
[cdc_ether]
[  125.231096]  [] do_one_initcall+0xd4/0x210
[  125.231099]  [] do_init_module+0x61/0x1cc
[  125.231102]  [] load_module+0x1d64/0x2670
[  125.231104]  [] ? store_uevent+0x40/0x40
[  125.231107]  [] SyS_finit_module+0x86/0xb0
[  125.231112]  [] system_call_fastpath+0x16/0x75
[  125.231114] usbcore: registered new interface driver cdc_ether
[  125.240235] rndis_host 3-3:1.0: Interface 0x00 is not authorized for
usage
[  125.240265] CPU: 5 PID: 1891 Comm: systemd-udevd Not tainted
4.1.0-rc7-21-desktop+ #8
[  125.240270]   880259253c78 8169e8b7
8001
[  125.240274]  a0a03000 880259253ca8 814f3b65
81c15020
[  125.240278]  8802645973e0  a0a11000
880259253cb8
[  125.240281] Call Trace:
[  125.240286]  [] dump_stack+0x4c/0x6e
[  125.240290]  [] usb_register_driver+0x115/0x170
[  125.240297]  [] ? 0xa0a11000
[  125.240301]  [] rndis_driver_init+0x1e/0x1000
[rndis_host]
[  125.240303]  [] do_one_initcall+0xd4/0x210
[  125.240305]  [] do_init_module+0x61/0x1cc
[  125.240307]  [] load_module+0x1d64/0x2670
[  125.240309]  [] ? store_uevent+0x40/0x40
[  125.240314]  [] SyS_finit_module+0x86/0xb0
[  125.240317]  [] system_call_fastpath+0x16/0x75
[  125.240320] usbcore: registered new interface driver rndis_host


--
To unsubscribe from this list: s

Re: [PATCH RESEND (1-3)/4] Add usb interface authorization

[Oliver Neukum]

On Fri, 2015-06-12 at 12:17 +0200, Stefan Koch wrote:
> There is one problem with that simple approach without the manually
> probe patch. As example the USB-Tethering with smartphones.
> 
> There is a driver (module) loading. It is loaded although
> usb_probe_interface() denies the interface.
> 
> I think that's not so good that there are unused modules loaded.
> So the approach with the manually probing possibility could solve
> that.

We cannot influence that anyway. User space decides which driver
to load. So this consideration is secondary, thus it should not
block a simplification of kernel code.

Regards
Oliver


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

Re: [PATCH 00/21] On-demand device registration

[Alexander Holler]

Am 12.06.2015 um 09:25 schrieb Linus Walleij:

On Thu, Jun 11, 2015 at 6:40 PM, Alexander Holler  wrote:

Am 11.06.2015 um 14:30 schrieb Linus Walleij:



Certainly it is possible to create deadlocks in this scenario, but the
scope is not to create an ubreakable system.


IAnd what happens if you run into a deadlock? Do you print "you've lost, try
changing your kernel config" in some output hidden by a splash-screen? ;)


Sorry it sounds like a blanket argument, the fact that there are
mutexes in the kernel makes it possible to deadlock, it doesn't
mean we don't use mutexes. Some programming problems are
just like such.


I'm not talking about specific deadlocks through mutexes. I'm talking 
about what happens when driver A needs driver B which needs driver A. 
How do you recognise and handle that with your instrumented on-demand 
device initialization? Such a circular dependency might happen by just 
adding a new fucntion call or by changing the kernel configuration. And 
with the on-demand stuff, the possibility that the developer introducing 
this new (maybe optional) call will never hit such a circular dependency 
is high. So you will end up with a never ending stream of problem 
reports whenever someone introduced such a circular dependecy without 
having noticed it.


And to come back to specific deadlocks, if you are extending function 
calls from something former simple to something which might initialize a 
whole bunch of drivers, needing maybe seconds, I wouldn't say this is a 
blanket argument, but a real thread.


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

Re: [PATCH 00/21] On-demand device registration

[Alexander Holler]

Am 12.06.2015 um 13:19 schrieb Alexander Holler:

Am 12.06.2015 um 09:25 schrieb Linus Walleij:

On Thu, Jun 11, 2015 at 6:40 PM, Alexander Holler
 wrote:

Am 11.06.2015 um 14:30 schrieb Linus Walleij:



Certainly it is possible to create deadlocks in this scenario, but the
scope is not to create an ubreakable system.


IAnd what happens if you run into a deadlock? Do you print "you've
lost, try
changing your kernel config" in some output hidden by a
splash-screen? ;)


Sorry it sounds like a blanket argument, the fact that there are
mutexes in the kernel makes it possible to deadlock, it doesn't
mean we don't use mutexes. Some programming problems are
just like such.


I'm not talking about specific deadlocks through mutexes. I'm talking
about what happens when driver A needs driver B which needs driver A.
How do you recognise and handle that with your instrumented on-demand
device initialization? Such a circular dependency might happen by just
adding a new fucntion call or by changing the kernel configuration. And
with the on-demand stuff, the possibility that the developer introducing
this new (maybe optional) call will never hit such a circular dependency
is high. So you will end up with a never ending stream of problem
reports whenever someone introduced such a circular dependecy without
having noticed it.

And to come back to specific deadlocks, if you are extending function
calls from something former simple to something which might initialize a
whole bunch of drivers, needing maybe seconds, I wouldn't say this is a
blanket argument, but a real thread.


Keep in mind, that the possibility that a function call ends up with 
initializing a whole bunch of other drivers, is not determined 
statically, but depends on the configuration and runtime behaviour of 
the actual system the on-demand stuff actually happens.


E.g. if driver A is faster one system that driver B, the whole bunch of 
drivers might become initialized by a call in driver A. But if driver B 
was faster on the developers system (or the system is configured to 
first init driver B), than the whole bunch of drivers might have become 
initialized by driver B on the developers system. Thus he never might 
have hit a possible problem when the whole bunch of drivers got 
initialized in driver A.


That means it isn't always a good idea to create dynamic systems (like 
on-demand device initialization), because it's very hard to foresee and 
correctly handle their runtime behaviour.



Alexander Holler

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

Re: [ehci-orion] ETIMEOUT with ehci_setup()'s ehci_halt()

[Valentin Longchamp]

On 06/08/2015 01:46 PM, Valentin Longchamp wrote:
> On 06/06/2015 02:17 PM, Sebastian Hesselbarth wrote:
>> On 05.06.2015 17:19, Andrew Lunn wrote:
>>> On Fri, Jun 05, 2015 at 04:34:54PM +0200, Valentin Longchamp wrote:
 I am currently bringing up the USB 2.0 Host controller of the Bobcat's 
 (98DX4122
 Marvell switch) internal kirkwood CPU on a variation of Keymile's 
 km_kirwood
 hardware (kirkwood-km_kirkwood.dts).

 When the driver registers its hcd, it fails with a timemout as it can be 
 seen in
 the below log:

> root@kmcoge5un:~# dmesg -n 8
> root@kmcoge5un:~# insmod ehci-orion.ko
> ehci-orion: EHCI orion driver
> Initializing Orion-SoC USB Host Controller
> orion-ehci f105.ehci: EHCI Host Controller
> orion-ehci f105.ehci: new USB bus registered, assigned bus number 1
> orion-ehci f105.ehci: reset hcs_params 0x10011 dbg=0 ind cc=0 pcc=0 
> ordered ports=1
> orion-ehci f105.ehci: reset hcc_params 0006 thresh 0 uframes 
> 256/512/1024 park
> orion-ehci f105.ehci: park 0
> orion-ehci f105.ehci: reset command 0080002 (park)=0 ithresh=8 
> period=1024 Reset HALT
> orion-ehci f105.ehci: can't setup
> orion-ehci f105.ehci: USB bus 1 deregistered
> orion-ehci f105.ehci: init f105.ehci fail, -110
> orion-ehci: probe of f105.ehci failed with error -110

 This is very similar to this problem, except that it always happens:
 https://lkml.org/lkml/2012/6/4/381

 I have cornered it out to the last handshake() call of the ehci_halt() 
 call,
 that should set the controller in the HALT state. If I comment this 
 handshake()
 call out, the registration is fine and the controller then looks OK (I 
 don't
 have a hardware with a real USB bus to further test it yet).
>>
>> Valentin,
>>
>> can you specify what "real USB bus" means? Does your current hardware
>> support USB host or device with anything connected to it?
> 
> No. The current hardware I am testing on does not support USB since the USB 
> bus
> is not "wired" at all, there is no phy present (this maybe causes a problem 
> ?).
> What I test is if the ehci-orion USB host driver is able to register with and
> configure the USB host controller present in the SoC, in order to prepare 
> things
> for when the hardware is available.
> 
> Hopefully I get some new hardware prototypes where the USB bus is complete 
> this
> week so that I can test on the real setup.
> 

We have now received the hardware prototype and the behavior is exactly the
same: ETIMEOUT in ehci_halt(). If the last handshake() call is removed, the EHCI
host controller is correctly initialized and the device on the bus is detected
and works correctly.

I thus tend to think that this is an IP (or Bobcat) internal problem and is not
really related with the hardware nor a PHY.

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

[PATCH v2 1/5] usb: Add usb interface authorization: Declare attributes of structures

[Stefan Koch]

These attributes are authorized, mask and mask_changed.
The first shows the authorization state for an interface.
The second describes the authorization states of all device interfaces.
The third is a status bit to control a manual setting of the mask.

Signed-off-by: Stefan Koch 
---
 include/linux/usb.h | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/usb.h b/include/linux/usb.h
index 447fe29..ab96e21 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -171,6 +171,7 @@ struct usb_interface {
int minor;  /* minor number this interface is
 * bound to */
enum usb_interface_condition condition; /* state of binding */
+   unsigned authorized:1; /* for policy that allows using the interface */
unsigned sysfs_files_created:1; /* the sysfs attributes exist */
unsigned ep_devs_created:1; /* endpoint "devices" exist */
unsigned unregistering:1;   /* unregistration is in progress */
@@ -502,6 +503,7 @@ struct usb3_lpm_parameters {
  * @authenticated: Crypto authentication passed
  * @wusb: device is Wireless USB
  * @lpm_capable: device supports LPM
+ * @mask_changed: true if @mask was changed since configuration setup
  * @usb2_hw_lpm_capable: device can perform USB2 hardware LPM
  * @usb2_hw_lpm_besl_capable: device can perform USB2 hardware BESL LPM
  * @usb2_hw_lpm_enabled: USB2 hardware LPM is enabled
@@ -511,6 +513,7 @@ struct usb3_lpm_parameters {
  * @product: iProduct string, if present (static)
  * @manufacturer: iManufacturer string, if present (static)
  * @serial: iSerialNumber string, if present (static)
+ * @mask: interface authorization mask to allow or denz specific usb interfaces
  * @filelist: usbfs files that are open to this device
  * @maxchild: number of ports if hub
  * @quirks: quirks of the whole device
@@ -575,6 +578,7 @@ struct usb_device {
unsigned authenticated:1;
unsigned wusb:1;
unsigned lpm_capable:1;
+   unsigned mask_changed:1;
unsigned usb2_hw_lpm_capable:1;
unsigned usb2_hw_lpm_besl_capable:1;
unsigned usb2_hw_lpm_enabled:1;
@@ -587,6 +591,8 @@ struct usb_device {
char *manufacturer;
char *serial;
 
+   u32 mask;
+
struct list_head filelist;
 
int maxchild;
-- 
2.1.4

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

[PATCH v2 0/5] usb: Add usb interface authorization

[Stefan Koch]

This is a patch that introduces an interface authorization for USB devices.
The kernel supports already a device authorization bacause of wireless USB.

But the new interface authorization allows to enable or disable individual 
interfaces per bitmask
instead allow or deny a whole device.

The patch is made now much simplier as suggested by Alan Stern.

Each patch depends on all patches with a lesser number.

Stefan Koch (5):
  usb: Add usb interface authorization: Declare attributes of structures
  usb: Add usb interface authorization: Introduces the default interface
authorization
  usb: Add usb interface authorization: Control interface probing and
claiming
  usb: Add usb interface authorization: Introduces the usb interface
authorization.
  usb: Add usb interface authorization: SysFS part of usb interface
authorization.

 drivers/base/base.h|   1 -
 drivers/usb/core/driver.c  |  11 +
 drivers/usb/core/hcd.c |  47 ++
 drivers/usb/core/hub.c | 117 +
 drivers/usb/core/message.c |  12 -
 drivers/usb/core/sysfs.c   |  71 ++-
 drivers/usb/core/usb.c |  22 -
 drivers/usb/core/usb.h |   1 +
 include/linux/device.h |   1 +
 include/linux/usb.h|   6 +++
 include/linux/usb/hcd.h|   1 +
 11 files changed, 285 insertions(+), 5 deletions(-)

-- 
2.1.4

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

[PATCH v2 5/5] usb: Add usb interface authorization: SysFS part of usb interface authorization.

[Stefan Koch]

To allow (1) or deny (0) interfaces it is needed to write to the mask.
As default each bit has the initial value of the default authorization bit.
The value would showed or have to be written in hexadecimal format.
Entry: /sys/bus/usb/devices/*-*/interface_authorization_mask

Example: Only the interfaces 0 and 2 from device at 3-2 should allowed, the 
others should be denied.
echo 5 > /sys/bus/usb/devices/3-2/interface_authorization_mask

Signed-off-by: Stefan Koch 
---
 drivers/usb/core/sysfs.c | 71 +++-
 1 file changed, 70 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index d269738..afa0799e 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -622,7 +622,6 @@ usb_descriptor_attr(bDeviceProtocol, "%02x\n");
 usb_descriptor_attr(bNumConfigurations, "%d\n");
 usb_descriptor_attr(bMaxPacketSize0, "%d\n");
 
-
 /* show if the device is authorized (1) or not (0) */
 static ssize_t authorized_show(struct device *dev,
   struct device_attribute *attr, char *buf)
@@ -655,6 +654,74 @@ static ssize_t authorized_store(struct device *dev,
 static DEVICE_ATTR_IGNORE_LOCKDEP(authorized, S_IRUGO | S_IWUSR,
  authorized_show, authorized_store);
 
+/*
+ * show authorization status of usb interface as bitmask
+ * 1 is authorized, 0 is not authorized
+ *
+ * example: 0b0101 interfaces 0 and 2 are authorized
+ *  the others are not authorized
+ */
+static ssize_t interface_authorization_mask_show(struct device *dev,
+   struct device_attribute *attr, char *buf)
+{
+   struct usb_device
+   *usb_dev = to_usb_device(dev);
+   __u32 val = 0;
+   unsigned i, n;
+
+   if (!usb_dev->actconfig)
+   return -ENODEV;
+
+   /* number of device's interfaces */
+   n = usb_dev->actconfig->desc.bNumInterfaces;
+   for (i = 0; i < n; i++)
+   val |= usb_dev->actconfig->interface[i]->authorized << i;
+
+   return sprintf(buf, "%02x\n", val);
+}
+
+/*
+ * authorize or deauthorize an usb interface per bitmask
+ * 1 is to authorize, 0 is not to authorize
+ * example: 0b0101 authorizes interface 0 and 2
+ *  and deauthorizes all other interfaces
+ */
+static ssize_t interface_authorization_mask_store(struct device *dev,
+   struct device_attribute *attr, const char *buf, size_t count)
+{
+   struct usb_device
+   *usb_dev = to_usb_device(dev);
+   int rc = 0;
+   u32 val = 0;
+
+   if (!usb_dev->actconfig)
+   return -ENODEV;
+   else if (sscanf(buf, "%x\n", &val) != 1)
+   return -EINVAL;
+
+   rc = usb_device_set_mask(dev, val);
+
+   return rc < 0 ? rc : count;
+}
+static DEVICE_ATTR_RW(interface_authorization_mask);
+
+/*
+ * show authorization change status of usb interfaces bitmask
+ * if 1 the mask has modified if 0 is was not modified
+ *
+ * note: would be set after mask_store was called,
+ *   also if the mask has the same value as the old
+ */
+static ssize_t interface_authorization_mask_changed_show(struct device *dev,
+   struct device_attribute *attr, char *buf)
+{
+   struct usb_device
+   *usb_dev = to_usb_device(dev);
+
+   return sprintf(buf, "%u\n", usb_dev->mask_changed);
+}
+static DEVICE_ATTR_RO(interface_authorization_mask_changed);
+
 /* "Safely remove a device" */
 static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
@@ -703,6 +770,8 @@ static struct attribute *dev_attrs[] = {
&dev_attr_quirks.attr,
&dev_attr_avoid_reset_quirk.attr,
&dev_attr_authorized.attr,
+   &dev_attr_interface_authorization_mask.attr,
+   &dev_attr_interface_authorization_mask_changed.attr,
&dev_attr_remove.attr,
&dev_attr_removable.attr,
&dev_attr_ltm_capable.attr,
-- 
2.1.4

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

[PATCH v2 2/5] usb: Add usb interface authorization: Introduces the default interface authorization

[Stefan Koch]

Interfaces would allowed per default.
This can disabled or enabled by writing 0 or 1 to
/sys/bus/usb/devices/usb*/interface_authorized_default

Signed-off-by: Stefan Koch 
---
 drivers/usb/core/hcd.c | 47 ++
 drivers/usb/core/message.c |  8 
 drivers/usb/core/usb.c | 22 --
 include/linux/usb/hcd.h|  1 +
 4 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 45a915c..024cce5 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -882,9 +882,53 @@ static ssize_t authorized_default_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(authorized_default);
 
+/*
+ * show default authorization status of usb interface
+ *
+ * note: interface_auhorized_default is the default value
+ *   for initialising interface_authorized
+ */
+static ssize_t interface_authorized_default_show(struct device *dev,
+   struct device_attribute *attr, char *buf)
+{
+   struct usb_device
+   *usb_dev = to_usb_device(dev);
+   struct usb_hcd *hcd = bus_to_hcd(usb_dev->bus);
+   unsigned def = hcd->interface_authorized_default;
+
+   return sprintf(buf, "%u\n", def);
+}
+
+/*
+ * store default authorization status of usb interface
+ *
+ * note: interface_auhorized_default is the default value
+ *   for initialising interface_authorized
+ */
+static ssize_t interface_authorized_default_store(struct device *dev,
+   struct device_attribute *attr, const char *buf, size_t count)
+{
+   struct usb_device
+   *usb_dev = to_usb_device(dev);
+   struct usb_hcd *hcd = bus_to_hcd(usb_dev->bus);
+   int rc = count;
+   unsigned val;
+
+   if (!usb_dev || !hcd)
+   rc = -ENODEV;
+   else if (sscanf(buf, "%u\n", &val) != 1)
+   rc = -EINVAL;
+   else
+   hcd->interface_authorized_default = val ? 1 : 0;
+
+   return rc;
+}
+static DEVICE_ATTR_RW(interface_authorized_default);
+
 /* Group all the USB bus attributes */
 static struct attribute *usb_bus_attrs[] = {
&dev_attr_authorized_default.attr,
+   &dev_attr_interface_authorized_default.attr,
NULL,
 };
 
@@ -2679,6 +2723,9 @@ int usb_add_hcd(struct usb_hcd *hcd,
hcd->authorized_default = authorized_default;
set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 
+   /* per default all interfaces are authorized */
+   hcd->interface_authorized_default = 1;
+
/* HC is in reset state, but accessible.  Now do the one-time init,
 * bottom up so that hcds can customize the root hubs before hub_wq
 * starts talking to them.  (Note, bus id is assigned early too.)
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index f368d20..a353a42 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1804,9 +1804,17 @@ free_interfaces:
struct usb_host_interface *alt;
 
cp->interface[i] = intf = new_interfaces[i];
+
+   /* update device's mask at configuration change */
+   if (hcd->interface_authorized_default)
+   dev->mask |= (1 << i);
+   else
+   dev->mask &= ~(1 << i);
+
intfc = cp->intf_cache[i];
intf->altsetting = intfc->altsetting;
intf->num_altsetting = intfc->num_altsetting;
+   intf->authorized = dev->mask & (1 << i) ? true : false;
kref_get(&intfc->ref);
 
alt = usb_altnum_to_altsetting(intf, 0);
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 8d5b2f4..f633b0b 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -507,12 +507,30 @@ struct usb_device *usb_alloc_dev(struct usb_device 
*parent,
dev->connect_time = jiffies;
dev->active_duration = -jiffies;
 #endif
-   if (root_hub)   /* Root hub always ok [and always wired] */
+   if (root_hub) { /* Root hub always ok [and always wired] */
dev->authorized = 1;
-   else {
+   dev->mask = 0;
+
+   /* invert the mask. each bit of the mask is now TRUE.
+* all interfaces should be allowed. */
+   dev->mask = ~dev->mask;
+   } else {
dev->authorized = usb_hcd->authorized_default;
dev->wusb = usb_bus_is_wusb(bus) ? 1 : 0;
+   dev->mask = 0;
+
+   /* invert the mask if interface_authorized_default is TRUE
+* each bit of the mask is now TRUE
+* all interfaces should be allowed
+*
+* do not invert the mask
+* if interface_authorized_default is FALSE
+* each bit of the mask is now FALSE
+* no interface should be allowed */
+   dev->mask = usb_hcd->interface_a

[PATCH v2 4/5] usb: Add usb interface authorization: Introduces the usb interface authorization.

[Stefan Koch]

The kernel supports already a device authorization because of wireless USB.
These is usable for wired usb devices, too.
These new interface authorization allows to enable or disable
individual interfaces per bitmask instead a whole device.

The authorization is done stepwise for each respective
interface group (defined by respective mask) to avoid side effects.

The interface group mask would updated at authorization.
After that the usb interfaces are enabled.
Finally, the device would probed.

Signed-off-by: Stefan Koch 
---
 drivers/usb/core/hub.c | 117 +
 drivers/usb/core/message.c |   4 +-
 drivers/usb/core/usb.h |   1 +
 3 files changed, 121 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 3b71516..75b1555 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2577,6 +2577,123 @@ out_authorized:
return result;
 }
 
+/*
+ * authorize or deauthorize an usb interface per bitmask
+ *
+ * @dev: device structure
+ * @mask: authorization mask
+ * 1 is to authorize, 0 is not to authorize
+ *
+ * example: 0b0101 authorizes interface 1 and 3
+ *  and deauthorizes all other interfaces
+ *
+ * Returns: 0 at success, value<0 at failure
+ */
+int usb_device_set_mask(struct device *dev, u32 mask)
+{
+   struct usb_device *usb_dev = NULL;
+   struct usb_host_config *actconfig = NULL;
+   u32 oldmask = 0;
+   unsigned i, n, confNr;
+
+   /* get device lock */
+   device_lock(dev);
+
+   usb_dev = to_usb_device(dev);
+
+   if (usb_dev)
+   actconfig = usb_dev->actconfig;
+
+   if (!usb_dev || !actconfig) {
+   device_unlock(dev);
+   return -ENODEV;
+   }
+
+   usb_dev->mask_changed = 1;
+
+   /* get current configuration number */
+   confNr = actconfig->desc.bConfigurationValue;
+
+   /* number of device's interfaces */
+   n = actconfig->desc.bNumInterfaces;
+
+   oldmask = usb_dev->mask;
+
+   /* set the flags first and unbind interface if applicable */
+   for (i = 0; i < n; i++) {
+   bool oldauth = oldmask & (1 << i) ? true : false;
+   bool auth = mask & (1 << i) ? true : false;
+   struct usb_interface *intf = actconfig->interface[i];
+   struct device *intf_dev = NULL;
+   struct usb_device *intf_usb_dev = NULL;
+
+   if (!intf) {
+   device_unlock(dev);
+   return -ENODEV;
+   }
+
+   intf_dev = &intf->dev;
+
+   if (intf_dev)
+   intf_usb_dev = to_usb_device(intf_dev);
+
+   if (!intf_dev || !intf_usb_dev) {
+   device_unlock(dev);
+   return -ENODEV;
+   }
+
+   if (!auth && oldauth) { /* deauthorize */
+   usb_dev->mask &= ~(1 << i); /* update mask */
+   intf->authorized = 0; /* update flag */
+   intf->unregistering = 1;
+   usb_forced_unbind_intf(intf);
+   } else if (auth && !oldauth) { /* authorize */
+   usb_dev->mask |= (1 << i); /* update mask */
+   intf->authorized = 1; /* update flag */
+   }
+   }
+
+   /* disable or enable interfaces and delete device if applicable */
+   for (i = 0; i < n; i++) {
+   bool oldauth = oldmask & (1 << i) ? true : false;
+   bool auth = mask & (1 << i) ? true : false;
+   struct usb_interface *intf = actconfig->interface[i];
+
+   if (!intf) {
+   device_unlock(dev);
+   return -ENODEV;
+   }
+
+   if (!auth && oldauth) { /* deauthorize */
+   /* disable interface's endpoints for usage */
+   usb_disable_interface(usb_dev, intf, true);
+   } else if (auth && !oldauth) { /* authorize */
+   /* enable interface's endpoints for usage */
+   usb_enable_interface(usb_dev, intf, true);
+   }
+   }
+
+   /* probe an interface at authorization */
+   for (i = 0; i < n; i++) {
+   bool oldauth = oldmask & (1 << i) ? true : false;
+   bool auth = mask & (1 << i) ? true : false;
+   struct usb_interface *intf = actconfig->interface[i];
+
+   if (!intf) {
+   device_unlock(dev);
+   return -ENODEV;
+   }
+
+   if (auth && !oldauth) { /* deauthorize */
+   bus_probe_device(&intf->dev);
+   }
+   }
+
+   /* release device lock */
+   device_unlock(dev);
+
+   return 0;
+}
 
 /* Returns 1 if @hub is a WUSB root hub, 0 otherwise */
 static unsigned hub_is_wusb(stru

[PATCH v2 3/5] usb: Add usb interface authorization: Control interface probing and claiming

[Stefan Koch]

Driver probings and interface claims could rejected
if an interface is not authorized.

Signed-off-by: Stefan Koch 
---
 drivers/base/base.h   |  1 -
 drivers/usb/core/driver.c | 11 +++
 include/linux/device.h|  1 +
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 251c5d3..4b304a8 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -102,7 +102,6 @@ extern void container_dev_init(void);
 struct kobject *virtual_device_parent(struct device *dev);
 
 extern int bus_add_device(struct device *dev);
-extern void bus_probe_device(struct device *dev);
 extern void bus_remove_device(struct device *dev);
 
 extern int bus_add_driver(struct device_driver *drv);
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 818369a..9d4251f 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -295,6 +295,13 @@ static int usb_probe_interface(struct device *dev)
if (udev->authorized == 0) {
dev_err(&intf->dev, "Device is not authorized for usage\n");
return error;
+   } else if (intf->authorized == 0) {
+   unsigned intfNr = intf->altsetting->desc.bInterfaceNumber;
+
+   dev_err(&intf->dev, "Interface 0x%02x is not authorized for 
usage\n",
+   intfNr);
+
+   return error;
}
 
id = usb_match_dynamic_id(intf, driver);
@@ -507,6 +514,10 @@ int usb_driver_claim_interface(struct usb_driver *driver,
if (dev->driver)
return -EBUSY;
 
+   /* reject claim if not iterface is not authorized */
+   if (!iface->authorized)
+   return -ENODEV;
+
udev = interface_to_usbdev(iface);
 
dev->driver = &driver->drvwrap.driver;
diff --git a/include/linux/device.h b/include/linux/device.h
index 6558af9..598d282 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -976,6 +976,7 @@ extern void device_release_driver(struct device *dev);
 extern int  __must_check device_attach(struct device *dev);
 extern int __must_check driver_attach(struct device_driver *drv);
 extern int __must_check device_reprobe(struct device *dev);
+extern void bus_probe_device(struct device *dev);
 
 /*
  * Easy functions for dynamically creating devices on the fly
-- 
2.1.4

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

Re: [PATCH v2 5/5] usb: Add usb interface authorization: SysFS part of usb interface authorization.

[Krzysztof Opasiak]

On 06/12/2015 02:33 PM, Stefan Koch wrote:

To allow (1) or deny (0) interfaces it is needed to write to the mask.
As default each bit has the initial value of the default authorization bit.
The value would showed or have to be written in hexadecimal format.
Entry: /sys/bus/usb/devices/*-*/interface_authorization_mask

Example: Only the interfaces 0 and 2 from device at 3-2 should allowed, the 
others should be denied.
echo 5 > /sys/bus/usb/devices/3-2/interface_authorization_mask

Signed-off-by: Stefan Koch 
---
  drivers/usb/core/sysfs.c | 71 +++-
  1 file changed, 70 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index d269738..afa0799e 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -622,7 +622,6 @@ usb_descriptor_attr(bDeviceProtocol, "%02x\n");
  usb_descriptor_attr(bNumConfigurations, "%d\n");
  usb_descriptor_attr(bMaxPacketSize0, "%d\n");

-
  /* show if the device is authorized (1) or not (0) */
  static ssize_t authorized_show(struct device *dev,
   struct device_attribute *attr, char *buf)
@@ -655,6 +654,74 @@ static ssize_t authorized_store(struct device *dev,
  static DEVICE_ATTR_IGNORE_LOCKDEP(authorized, S_IRUGO | S_IWUSR,
  authorized_show, authorized_store);

+/*
+ * show authorization status of usb interface as bitmask
+ * 1 is authorized, 0 is not authorized
+ *
+ * example: 0b0101 interfaces 0 and 2 are authorized
+ *  the others are not authorized
+ */


Why do you need so complicated bit mask attribute in device dir instead 
of just a simple "authorized" attribute in interface directory?


Best regards,

--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] usb: Add usb interface authorization: SysFS part of usb interface authorization.

[Oliver Neukum]

On Fri, 2015-06-12 at 14:57 +0200, Krzysztof Opasiak wrote:

> > +/*
> > + * show authorization status of usb interface as bitmask
> > + * 1 is authorized, 0 is not authorized
> > + *
> > + * example: 0b0101 interfaces 0 and 2 are authorized
> > + *  the others are not authorized
> > + */
> 
> Why do you need so complicated bit mask attribute in device dir instead 
> of just a simple "authorized" attribute in interface directory?

Some drivers (BT, CDC ACM, ...) require multiple interfaces
The authorization must be given atomically or probing will
fail.

Regards
Oliver


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

[PATCH] usb: dwc3: Use ASCII space in Kconfig

[Thierry Reding]

From: Thierry Reding 

The USB_DWC3_ULPI Kconfig entry uses a UTF-8 non-breaking space (0xca20)
instead of a regular ASCII space (0x20). Commit 2e0d737fc76f ("kconfig:
don't silently ignore unhandled characters") exposes this by warning
about unhandled characters.

Signed-off-by: Thierry Reding 
---
 drivers/usb/dwc3/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 473bfaa96c30..dede32e809b6 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -13,7 +13,7 @@ if USB_DWC3
 
 config USB_DWC3_ULPI
bool "Register ULPI PHY Interface"
-   depends on USB_ULPI_BUS=y || USB_ULPI_BUS=USB_DWC3
+   depends on USB_ULPI_BUS=y || USB_ULPI_BUS=USB_DWC3
help
  Select this if you have ULPI type PHY attached to your DWC3
  controller.
-- 
2.4.1

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

Re: [PATCH v2 5/5] usb: Add usb interface authorization: SysFS part of usb interface authorization.

[Krzysztof Opasiak]

On 06/12/2015 03:10 PM, Oliver Neukum wrote:

On Fri, 2015-06-12 at 14:57 +0200, Krzysztof Opasiak wrote:


+/*
+ * show authorization status of usb interface as bitmask
+ * 1 is authorized, 0 is not authorized
+ *
+ * example: 0b0101 interfaces 0 and 2 are authorized
+ *  the others are not authorized
+ */


Why do you need so complicated bit mask attribute in device dir instead
of just a simple "authorized" attribute in interface directory?


Some drivers (BT, CDC ACM, ...) require multiple interfaces
The authorization must be given atomically or probing will
fail.



Please correct me if I'm wrong but this:

echo 1 > $(DATA_INTF)/authorized
echo 1 > $(CONTROL_INTF)/authorized

should also work fine even if driver needs more than one interface. The 
only issue is that you should authorize interface for which driver is 
probing as last one.


Best regards,

--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] usb: Add usb interface authorization: SysFS part of usb interface authorization.

[Oliver Neukum]

On Fri, 2015-06-12 at 15:22 +0200, Krzysztof Opasiak wrote:
> 
> On 06/12/2015 03:10 PM, Oliver Neukum wrote:

> > Some drivers (BT, CDC ACM, ...) require multiple interfaces
> > The authorization must be given atomically or probing will
> > fail.
> >
> 
> Please correct me if I'm wrong but this:
> 
> echo 1 > $(DATA_INTF)/authorized
> echo 1 > $(CONTROL_INTF)/authorized
> 
> should also work fine even if driver needs more than one interface. The 
> only issue is that you should authorize interface for which driver is 
> probing as last one.

Exactly. And that information may depend on the driver version.
It would be a bad idea to expose this.

Regards
Oliver


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

[Patch V2 3/3] xen: add Xen pvUSB maintainer

[Juergen Gross]

Add myself as maintainer for the Xen pvUSB stuff.

Signed-off-by: Juergen Gross 
Acked-by: Konrad Rzeszutek Wilk 
---
 MAINTAINERS | 8 
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index d8afd29..5f54a0d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10965,6 +10965,14 @@ F: drivers/scsi/xen-scsifront.c
 F: drivers/xen/xen-scsiback.c
 F: include/xen/interface/io/vscsiif.h
 
+XEN PVUSB DRIVERS
+M: Juergen Gross 
+L: xen-de...@lists.xenproject.org (moderated for non-subscribers)
+L: linux-usb@vger.kernel.org
+S: Supported
+F: divers/usb/xen/
+F: include/xen/interface/io/usbif.h
+
 XEN SWIOTLB SUBSYSTEM
 M: Konrad Rzeszutek Wilk 
 L: xen-de...@lists.xenproject.org (moderated for non-subscribers)
-- 
2.1.4

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

[Patch V2 0/3] xen, usb: support pvUSB frontend driver

[Juergen Gross]

This series adds XEN guest pvUSB support. With pvUSB it is possible to
use physical USB devices from a XEN domain.

The support consists of a frontend in the unprivileged domU passing
I/O-requests to the backend in a driver domain (usually Dom0). The
backend is not part of this patch series, as it will be supported
via qemu.

The code is taken (and adapted) from the original pvUSB implementation
done for Linux 2.6 in 2008 by Fujitsu.

Normal operation of USB devices by adding and removing them dynamically
to/from a domain has been tested using various USB devices (USB 1.1,
2.0 and 3.0). The pvUSB backend for these tests was a SUSE SLES Dom0
with a kernel based backend driver.

Changes since V1:
- removed backend, as it can be implemented in user land
- added some access macros and definitions to the pvUSB interface
  description to make it independant from linux kernel USB internals
- adapted frontend to newer kernel version and use new pvUSB
  interface macros
- set port status in one chunk as suggested by Oliver Neukum


Juergen Gross (3):
  usb: Add Xen pvUSB protocol description
  usb: Introduce Xen pvUSB frontend
  xen: add Xen pvUSB maintainer

 MAINTAINERS  |8 +
 drivers/usb/Kconfig  |2 +
 drivers/usb/Makefile |2 +
 drivers/usb/xen/Kconfig  |   10 +
 drivers/usb/xen/Makefile |5 +
 drivers/usb/xen/xen-usbfront.c   | 1647 ++
 include/xen/interface/io/usbif.h |  252 ++
 7 files changed, 1926 insertions(+)
 create mode 100644 drivers/usb/xen/Kconfig
 create mode 100644 drivers/usb/xen/Makefile
 create mode 100644 drivers/usb/xen/xen-usbfront.c
 create mode 100644 include/xen/interface/io/usbif.h

-- 
2.1.4

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

[Patch V2 2/3] usb: Introduce Xen pvUSB frontend

[Juergen Gross]

Introduces the Xen pvUSB frontend. With pvUSB it is possible for a Xen
domU to communicate with a USB device assigned to that domU. The
communication is all done via the pvUSB backend in a driver domain
(usually Dom0) which is owner of the physical device.

The code is taken from the pvUSB implementation in Xen done by Fujitsu
based on Linux kernel 2.6.18.

Changes from the original version are:
- port to upstream kernel
- put all code in just one source file
- move module to appropriate location in kernel tree
- adapt to Linux style guide
- minor code modifications to increase readability

Signed-off-by: Juergen Gross 
---
 drivers/usb/Kconfig|2 +
 drivers/usb/Makefile   |2 +
 drivers/usb/xen/Kconfig|   10 +
 drivers/usb/xen/Makefile   |5 +
 drivers/usb/xen/xen-usbfront.c | 1647 
 5 files changed, 1666 insertions(+)
 create mode 100644 drivers/usb/xen/Kconfig
 create mode 100644 drivers/usb/xen/Makefile
 create mode 100644 drivers/usb/xen/xen-usbfront.c

diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 8ed451d..de998f1 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -94,6 +94,8 @@ source "drivers/usb/image/Kconfig"
 
 source "drivers/usb/usbip/Kconfig"
 
+source "drivers/usb/xen/Kconfig"
+
 endif
 
 source "drivers/usb/musb/Kconfig"
diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index d8926c6..e92d86c 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -62,3 +62,5 @@ obj-$(CONFIG_USB_GADGET)  += gadget/
 obj-$(CONFIG_USB_COMMON)   += common/
 
 obj-$(CONFIG_USBIP_CORE)   += usbip/
+
+obj-$(CONFIG_XEN_USB_FRONTEND) += xen/
diff --git a/drivers/usb/xen/Kconfig b/drivers/usb/xen/Kconfig
new file mode 100644
index 000..5d995477
--- /dev/null
+++ b/drivers/usb/xen/Kconfig
@@ -0,0 +1,10 @@
+config XEN_USB_FRONTEND
+   tristate "Xen USB frontend driver"
+   depends on XEN
+   default m
+   select XEN_XENBUS_FRONTEND
+   help
+ The Xen USB frontend driver allows the kernel to access USB Devices
+ within another guest OS (usually Dom0).
+ Only needed if the kernel is running in a Xen guest and generic
+ access to a USB device is needed.
diff --git a/drivers/usb/xen/Makefile b/drivers/usb/xen/Makefile
new file mode 100644
index 000..4568c26
--- /dev/null
+++ b/drivers/usb/xen/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for Xen pvUSB drivers
+#
+
+obj-$(CONFIG_XEN_USB_FRONTEND) += xen-usbfront.o
diff --git a/drivers/usb/xen/xen-usbfront.c b/drivers/usb/xen/xen-usbfront.c
new file mode 100644
index 000..36e7858
--- /dev/null
+++ b/drivers/usb/xen/xen-usbfront.c
@@ -0,0 +1,1647 @@
+/*
+ * xen-usbfront.c
+ *
+ * Xen USB Virtual Host Controller driver
+ *
+ * Copyright (C) 2009, FUJITSU LABORATORIES LTD.
+ * Author: Noboru Iwamatsu 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see .
+ *
+ * or, by your choice,
+ *
+ * When distributed separately from the Linux kernel or incorporated into
+ * other software packages, subject to the following license:
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+/* Private per-URB data */
+struct urb_priv {
+   struct l

[Patch V2 1/3] usb: Add Xen pvUSB protocol description

[Juergen Gross]

Add the definition of pvUSB protocol used between the pvUSB frontend in
a Xen domU and the pvUSB backend in a Xen driver domain (usually Dom0).

This header was originally provided by Fujitsu for Xen based on Linux
2.6.18.

Changes are:
- adapt to Linux style guide

Signed-off-by: Juergen Gross 
---
 include/xen/interface/io/usbif.h | 252 +++
 1 file changed, 252 insertions(+)
 create mode 100644 include/xen/interface/io/usbif.h

diff --git a/include/xen/interface/io/usbif.h b/include/xen/interface/io/usbif.h
new file mode 100644
index 000..95eaac2
--- /dev/null
+++ b/include/xen/interface/io/usbif.h
@@ -0,0 +1,252 @@
+/*
+ * usbif.h
+ *
+ * USB I/O interface for Xen guest OSes.
+ *
+ * Copyright (C) 2009, FUJITSU LABORATORIES LTD.
+ * Author: Noboru Iwamatsu 
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef __XEN_PUBLIC_IO_USBIF_H__
+#define __XEN_PUBLIC_IO_USBIF_H__
+
+#include "ring.h"
+#include "../grant_table.h"
+
+/*
+ * Feature and Parameter Negotiation
+ * =
+ * The two halves of a Xen pvUSB driver utilize nodes within the XenStore to
+ * communicate capabilities and to negotiate operating parameters. This
+ * section enumerates these nodes which reside in the respective front and
+ * backend portions of the XenStore, following the XenBus convention.
+ *
+ * Any specified default value is in effect if the corresponding XenBus node
+ * is not present in the XenStore.
+ *
+ * XenStore nodes in sections marked "PRIVATE" are solely for use by the
+ * driver side whose XenBus tree contains them.
+ *
+ *
+ *Backend XenBus Nodes
+ *
+ *
+ *-- Backend Device Identification (PRIVATE) --
+ *
+ * num-ports
+ *  Values: unsigned [1...31]
+ *
+ *  Number of ports for this (virtual) USB host connector.
+ *
+ * usb-ver
+ *  Values: unsigned [1...2]
+ *
+ *  USB version of this host connector: 1 = USB 1.1, 2 = USB 2.0.
+ *
+ * port/[1...31]
+ *  Values: string
+ *
+ *  Physical USB device connected to the given port, e.g. "3-1.5".
+ *
+ *
+ *Frontend XenBus Nodes
+ *
+ *
+ *--- Request Transport Parameters ---
+ *
+ * event-channel
+ *  Values: unsigned
+ *
+ *  The identifier of the Xen event channel used to signal activity
+ *  in the ring buffer.
+ *
+ * urb-ring-ref
+ *  Values: unsigned
+ *
+ *  The Xen grant reference granting permission for the backend to map
+ *  the sole page in a single page sized ring buffer. This is the ring
+ *  buffer for urb requests.
+ *
+ * conn-ring-ref
+ *  Values: unsigned
+ *
+ *  The Xen grant reference granting permission for the backend to map
+ *  the sole page in a single page sized ring buffer. This is the ring
+ *  buffer for connection/disconnection requests.
+ *
+ * protocol
+ *  Values: string (XEN_IO_PROTO_ABI_*)
+ *  Default Value:  XEN_IO_PROTO_ABI_NATIVE
+ *
+ *  The machine ABI rules governing the format of all ring request and
+ *  response structures.
+ *
+ */
+
+enum usb_spec_version {
+   USB_VER_UNKNOWN = 0,
+   USB_VER_USB11,
+   USB_VER_USB20,
+   USB_VER_USB30,  /* not supported yet */
+};
+
+/*
+ *  USB pipe in usbif_request
+ *
+ *  - port number: bits 0-4
+ * (USB_MAXCHILDREN is 31)
+ *
+ *  - operation flag:  bit 5
+ * (0 = submit urb,
+ *  1 = unlink urb)
+ *
+ *  - direction:

Re: [PATCH v2 5/5] usb: Add usb interface authorization: SysFS part of usb interface authorization.

[Krzysztof Opasiak]

On 06/12/2015 03:42 PM, Oliver Neukum wrote:

On Fri, 2015-06-12 at 15:22 +0200, Krzysztof Opasiak wrote:


On 06/12/2015 03:10 PM, Oliver Neukum wrote:



Some drivers (BT, CDC ACM, ...) require multiple interfaces
The authorization must be given atomically or probing will
fail.



Please correct me if I'm wrong but this:

echo 1 > $(DATA_INTF)/authorized
echo 1 > $(CONTROL_INTF)/authorized

should also work fine even if driver needs more than one interface. The
only issue is that you should authorize interface for which driver is
probing as last one.


Exactly. And that information may depend on the driver version.
It would be a bad idea to expose this.


But I see that this could be easily fixed in a quite similar way  it is 
done in usb_device_set_mask() introduced by 4th patch in this series. 
Each interface should notify ubs_device if its authorized status has 
changed from 0 to 1 and device should go through all interfaces (in 
current config) that are authorized but not claimed and try to find a 
driver for it.


Maybe it is not the best solution (all others are very welcome if you 
have any) because we will be probing all free (and authorized) 
interfaces each time some interface has been authorized, but single 
attribute in interface directory to which we can simply write 1 or 0 is 
way more user friendly. Let's say that we have 3 interfaces, two of them 
are authorized now, to authorize third one we have to do:


MASK=`cat authorized_mask`
NEW_MASK=$((MASK|4))
echo $NEW_MASK > authorized_mask

on the other hand if we have authorized attribute in interface directory 
we simply do


echo 1 > intf_dir/authorized

In my humble opinion second approach looks way more obvious and easy for 
a user, also writing an udev rules which I think will be a common use 
case will be way simpler.


Best regards,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] usb: Add usb interface authorization: SysFS part of usb interface authorization.

[Stefan Koch]

Am Freitag, den 12.06.2015, 16:16 +0200 schrieb Krzysztof Opasiak:
> 
> On 06/12/2015 03:42 PM, Oliver Neukum wrote:
> > On Fri, 2015-06-12 at 15:22 +0200, Krzysztof Opasiak wrote:
> >>
> >> On 06/12/2015 03:10 PM, Oliver Neukum wrote:
> >
> >>> Some drivers (BT, CDC ACM, ...) require multiple interfaces
> >>> The authorization must be given atomically or probing will
> >>> fail.
> >>>
> >>
> >> Please correct me if I'm wrong but this:
> >>
> >> echo 1 > $(DATA_INTF)/authorized
> >> echo 1 > $(CONTROL_INTF)/authorized
> >>
> >> should also work fine even if driver needs more than one interface. The
> >> only issue is that you should authorize interface for which driver is
> >> probing as last one.
> >
> > Exactly. And that information may depend on the driver version.
> > It would be a bad idea to expose this.
> 
> But I see that this could be easily fixed in a quite similar way  it is 
> done in usb_device_set_mask() introduced by 4th patch in this series. 
> Each interface should notify ubs_device if its authorized status has 
> changed from 0 to 1 and device should go through all interfaces (in 
> current config) that are authorized but not claimed and try to find a 
> driver for it.
> 
> Maybe it is not the best solution (all others are very welcome if you 
> have any) because we will be probing all free (and authorized) 
> interfaces each time some interface has been authorized, but single 
> attribute in interface directory to which we can simply write 1 or 0 is 
> way more user friendly. Let's say that we have 3 interfaces, two of them 
> are authorized now, to authorize third one we have to do:
> 
> MASK=`cat authorized_mask`
> NEW_MASK=$((MASK|4))
> echo $NEW_MASK > authorized_mask
> 
> on the other hand if we have authorized attribute in interface directory 
> we simply do
> 
> echo 1 > intf_dir/authorized
> 
> In my humble opinion second approach looks way more obvious and easy for 
> a user, also writing an udev rules which I think will be a common use 
> case will be way simpler.
> 
> Best regards,

The code therefore is available. But as Oliver mentioned not included.
If there is a need for it I could make a sixth patch.

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

Re: [PATCH 1/1] staging:f81534 Add F81532/534 Driver

[Greg KH]

On Fri, Jun 12, 2015 at 01:38:39PM +0800, Peter Hung wrote:
> Greg KH 於 2015/6/12 下午 12:33 寫道:
> >Why not just do the work now to clean up the file and get it merged
> >"properly"?  Why put this in staging at all?
> >
> 
> I'll clear up the file and resend it to usb-serial subsystem mail list.

Note, the "custom" ioctls will have to be removed, we can't have them in
a usb-serial driver, sorry.  Use the "real" GPIO api interface instead.

thanks,

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

Re: [Patch V2 2/3] usb: Introduce Xen pvUSB frontend

[Greg KH]

On Fri, Jun 12, 2015 at 04:10:00PM +0200, Juergen Gross wrote:
> Introduces the Xen pvUSB frontend. With pvUSB it is possible for a Xen
> domU to communicate with a USB device assigned to that domU. The
> communication is all done via the pvUSB backend in a driver domain
> (usually Dom0) which is owner of the physical device.
> 
> The code is taken from the pvUSB implementation in Xen done by Fujitsu
> based on Linux kernel 2.6.18.
> 
> Changes from the original version are:
> - port to upstream kernel
> - put all code in just one source file
> - move module to appropriate location in kernel tree
> - adapt to Linux style guide
> - minor code modifications to increase readability
> 
> Signed-off-by: Juergen Gross 
> ---
>  drivers/usb/Kconfig|2 +
>  drivers/usb/Makefile   |2 +
>  drivers/usb/xen/Kconfig|   10 +
>  drivers/usb/xen/Makefile   |5 +
>  drivers/usb/xen/xen-usbfront.c | 1647 
> 

A subdirectory for a single file?  That seems like overkill, don't you
think?  As this is a USB "host" driver, why not put it in that
directory?

Also, last time these patches were posted, people asked why you can't
use libusb/usbfs instead, what happened with that?  Or usbip?

> +config XEN_USB_FRONTEND
> + tristate "Xen USB frontend driver"
> + depends on XEN
> + default m

Remove this, default should be 'n'.

> +/* status of attached device */
> +struct vdevice_status {
> + int devnum;
> + enum usb_device_state status;
> +  enum usb_device_speed speed;
> +};

Always run your patches through checkpatch.pl so people don't tell you
about the things that checkpatch.pl would have told you about...

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

Re: [Patch V2 1/3] usb: Add Xen pvUSB protocol description

[Greg KH]

On Fri, Jun 12, 2015 at 04:09:59PM +0200, Juergen Gross wrote:
> +enum usb_spec_version {
> + USB_VER_UNKNOWN = 0,
> + USB_VER_USB11,
> + USB_VER_USB20,
> + USB_VER_USB30,  /* not supported yet */
> +};
> +

You are defining a bunch of things in this .h file that start with
"usb" yet are not part of the USB core at all, but rather are xen
specific.  Please don't polute the namespace with such things.

> +/*
> + *  USB pipe in usbif_request
> + *
> + *  - port number:   bits 0-4
> + *   (USB_MAXCHILDREN is 31)
> + *
> + *  - operation flag:bit 5
> + *   (0 = submit urb,
> + *1 = unlink urb)
> + *
> + *  - direction: bit 7
> + *   (0 = Host-to-Device [Out]
> + *1 = Device-to-Host [In])
> + *
> + *  - device address:bits 8-14
> + *
> + *  - endpoint:  bits 15-18
> + *
> + *  - pipe type: bits 30-31
> + *   (00 = isochronous, 01 = interrupt,
> + *10 = control, 11 = bulk)
> + */
> +
> +#define USBIF_PIPE_PORT_MASK 0x001f
> +#define USBIF_PIPE_UNLINK0x0020
> +#define USBIF_PIPE_DIR   0x0080
> +#define USBIF_PIPE_DEV_MASK  0x007f
> +#define USBIF_PIPE_DEV_SHIFT 8
> +#define USBIF_PIPE_EP_MASK   0x000f
> +#define USBIF_PIPE_EP_SHIFT  15
> +#define USBIF_PIPE_TYPE_MASK 0x0003
> +#define USBIF_PIPE_TYPE_SHIFT30
> +#define USBIF_PIPE_TYPE_ISOC 0
> +#define USBIF_PIPE_TYPE_INT  1
> +#define USBIF_PIPE_TYPE_CTRL 2
> +#define USBIF_PIPE_TYPE_BULK 3

Why can't you just use the defines we have for this already?

> +
> +#define usbif_pipeportnum(pipe)  ((pipe) & 
> USBIF_PIPE_PORT_MASK)
> +#define usbif_setportnum_pipe(pipe, portnum) ((pipe) | (portnum))
> +
> +#define usbif_pipeunlink(pipe)   ((pipe) & 
> USBIF_PIPE_UNLINK)
> +#define usbif_pipesubmit(pipe)   
> (!usbif_pipeunlink(pipe))
> +#define usbif_setunlink_pipe(pipe)   ((pipe) | USBIF_PIPE_UNLINK)
> +
> +#define usbif_pipein(pipe)   ((pipe) & USBIF_PIPE_DIR)
> +#define usbif_pipeout(pipe)  (!usbif_pipein(pipe))
> +
> +#define usbif_pipedevice(pipe)   \
> + (((pipe) >> USBIF_PIPE_DEV_SHIFT) & USBIF_PIPE_DEV_MASK)
> +
> +#define usbif_pipeendpoint(pipe) \
> + (((pipe) >> USBIF_PIPE_EP_SHIFT) & USBIF_PIPE_EP_MASK)
> +
> +#define usbif_pipetype(pipe) \
> + (((pipe) >> USBIF_PIPE_TYPE_SHIFT) & USBIF_PIPE_TYPE_MASK)
> +#define usbif_pipeisoc(pipe) (usbif_pipetype(pipe) == USBIF_PIPE_TYPE_ISOC)
> +#define usbif_pipeint(pipe)  (usbif_pipetype(pipe) == USBIF_PIPE_TYPE_INT)
> +#define usbif_pipectrl(pipe) (usbif_pipetype(pipe) == USBIF_PIPE_TYPE_CTRL)
> +#define usbif_pipebulk(pipe) (usbif_pipetype(pipe) == USBIF_PIPE_TYPE_BULK)


Same for all of these?

> +
> +#define USBIF_MAX_SEGMENTS_PER_REQUEST (16)
> +#define USBIF_MAX_PORTNR 31

Why does userspace have to know this?

> +
> +/*
> + * RING for transferring urbs.
> + */
> +struct usbif_request_segment {
> + grant_ref_t gref;
> + uint16_t offset;
> + uint16_t length;
> +};

Please use proper kernel types for things that go outside of the kernel
(__u16 and the like).  There is no "uint16_t" as a valid type in the
kernel, sorry.  Well, ok, we have it, just because it snuck in somehow,
but it should be removed one of these days...

> +struct usbif_urb_request {

Again, very generic structure name for a xen specific thing :(

I stopped reading here.

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

Re: [Xen-devel] [Patch V2 2/3] usb: Introduce Xen pvUSB frontend

[David Vrabel]

On 12/06/15 17:20, Greg KH wrote:
> On Fri, Jun 12, 2015 at 04:10:00PM +0200, Juergen Gross wrote:
>> Introduces the Xen pvUSB frontend. With pvUSB it is possible for a Xen
>> domU to communicate with a USB device assigned to that domU. The
>> communication is all done via the pvUSB backend in a driver domain
>> (usually Dom0) which is owner of the physical device.
>>
>> The code is taken from the pvUSB implementation in Xen done by Fujitsu
>> based on Linux kernel 2.6.18.
>>
>> Changes from the original version are:
>> - port to upstream kernel
>> - put all code in just one source file
>> - move module to appropriate location in kernel tree
>> - adapt to Linux style guide
>> - minor code modifications to increase readability
>>
>> Signed-off-by: Juergen Gross 
>> ---
>>  drivers/usb/Kconfig|2 +
>>  drivers/usb/Makefile   |2 +
>>  drivers/usb/xen/Kconfig|   10 +
>>  drivers/usb/xen/Makefile   |5 +
>>  drivers/usb/xen/xen-usbfront.c | 1647 
>> 
> 
> A subdirectory for a single file?  That seems like overkill, don't you
> think?  As this is a USB "host" driver, why not put it in that
> directory?
> 
> Also, last time these patches were posted, people asked why you can't
> use libusb/usbfs instead, what happened with that?  Or usbip?

Using libusb was for the backend driver.  This frontend driver is a host
controller (which is why it should be drivers/usb/host/xen-pv-hcd.c or
similar).

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

Re: [PATCH 1/1] usb: core: lpm: set lpm_capable for root hub device

[Alan Stern]

On Fri, 12 Jun 2015, Lu Baolu wrote:

> Commit 25cd2882e2fc ("usb/xhci: Change how we indicate a host supports
> Link PM.") removed the code to set lpm_capable for USB 3.0 super-speed
> root hub. The intention of that change was to avoid touching usb core
> internal field, a.k.a. lpm_capable, and let usb core to set it by
> checking U1 and U2 exit latency values in the descriptor.
> 
> Usb core checks and sets lpm_capable in hub_port_init(). Unfortunately,
> root hub is a special usb device as it has no parent. Hub_port_init()
> will never be called for a root hub device. That means lpm_capable will
> by no means be set for the root hub. As the result, lpm isn't functional
> at all in Linux kernel.
> 
> This patch add the code to check and set lpm_capable when registering a
> root hub device. It could be back-ported to kernels as old as v3.15,
> that contains the Commit 25cd2882e2fc ("usb/xhci: Change how we indicate
> a host supports Link PM.").
> 
> Cc: sta...@vger.kernel.org # 3.15
> Reported-by: Kevin Strasser 
> Signed-off-by: Lu Baolu 
> ---
>  drivers/usb/core/hcd.c | 6 ++
>  drivers/usb/core/hub.c | 2 +-
>  drivers/usb/core/usb.h | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 45a915c..48b208d 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1032,6 +1032,12 @@ static int register_root_hub(struct usb_hcd *hcd)
>   }
>   }
>  
> + if (le16_to_cpu(usb_dev->descriptor.bcdUSB) >= 0x0201) {
> + retval = usb_get_bos_descriptor(usb_dev);
> + if (!retval)
> + usb_dev->lpm_capable = usb_device_supports_lpm(usb_dev);
> + }
> +

This should be integrated with the code immediately above it, which 
also calls usb_get_bos_descriptor().  In fact, maybe you should simply 
change the existing code to check bcdUSB >= 0x0201 instead of speed == 
USB_SPEED_SUPER.

>   retval = usb_new_device (usb_dev);
>   if (retval) {
>   dev_err (parent_dev, "can't register root hub for %s, %d\n",
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 3b71516..884d702 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -122,7 +122,7 @@ struct usb_hub *usb_hub_to_struct_hub(struct usb_device 
> *hdev)
>   return usb_get_intfdata(hdev->actconfig->interface[0]);
>  }
>  
> -static int usb_device_supports_lpm(struct usb_device *udev)
> +int usb_device_supports_lpm(struct usb_device *udev)
>  {
>   /* USB 2.1 (and greater) devices indicate LPM support through
>* their USB 2.0 Extended Capabilities BOS descriptor.
> diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
> index 7eb1e26..d5668c4 100644
> --- a/drivers/usb/core/usb.h
> +++ b/drivers/usb/core/usb.h
> @@ -82,6 +82,7 @@ extern int usb_runtime_suspend(struct device *dev);
>  extern int usb_runtime_resume(struct device *dev);
>  extern int usb_runtime_idle(struct device *dev);
>  extern int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable);
> +extern int usb_device_supports_lpm(struct usb_device *udev);
>  
>  #else

This will break if you build it with CONFIG_PM disabled.

Alan Stern

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

[PATCH RESENT] phy: tusb1210: make better use of gpiod API

[Uwe Kleine-König]

Since 39b2bbe3d715 (gpio: add flags argument to gpiod_get*() functions)
which appeared in v3.17-rc1, the gpiod_get* functions take an additional
parameter that allows to specify direction and initial value for output.

Furthermore there is devm_gpiod_get_optional which is designed to get
optional gpios.

Simplify driver accordingly. Furthermore this is one caller less that
stops us making the flags argument to gpiod_get*() mandatory.

Signed-off-by: Uwe Kleine-König 
---
Hello,

[for the initial submission I forgot linux-usb on Cc, Felipe Balbi
requested a resend]

note I plan to make the flags parameter mandatory for 4.3. So unless
this change gets into 4.2, would it be ok to let it go in via the gpio
tree?

Best regards
Uwe

 drivers/phy/phy-tusb1210.c | 30 --
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/phy/phy-tusb1210.c b/drivers/phy/phy-tusb1210.c
index 07efdd318bdc..93dd45f2f26e 100644
--- a/drivers/phy/phy-tusb1210.c
+++ b/drivers/phy/phy-tusb1210.c
@@ -61,32 +61,26 @@ static struct phy_ops phy_ops = {
 
 static int tusb1210_probe(struct ulpi *ulpi)
 {
-   struct gpio_desc *gpio;
struct tusb1210 *tusb;
u8 val, reg;
-   int ret;
 
tusb = devm_kzalloc(&ulpi->dev, sizeof(*tusb), GFP_KERNEL);
if (!tusb)
return -ENOMEM;
 
-   gpio = devm_gpiod_get(&ulpi->dev, "reset");
-   if (!IS_ERR(gpio)) {
-   ret = gpiod_direction_output(gpio, 0);
-   if (ret)
-   return ret;
-   gpiod_set_value_cansleep(gpio, 1);
-   tusb->gpio_reset = gpio;
-   }
+   tusb->gpio_reset = devm_gpiod_get_optional(&ulpi->dev, "reset",
+  GPIOD_OUT_LOW);
+   if (IS_ERR(tusb->gpio_reset))
+   return PTR_ERR(tusb->gpio_reset);
 
-   gpio = devm_gpiod_get(&ulpi->dev, "cs");
-   if (!IS_ERR(gpio)) {
-   ret = gpiod_direction_output(gpio, 0);
-   if (ret)
-   return ret;
-   gpiod_set_value_cansleep(gpio, 1);
-   tusb->gpio_cs = gpio;
-   }
+   gpiod_set_value_cansleep(tusb->gpio_reset, 1);
+
+   tusb->gpio_cs = devm_gpiod_get_optional(&ulpi->dev, "cs",
+   GPIOD_OUT_LOW);
+   if (IS_ERR(tusb->gpio_cs))
+   return PTR_ERR(tusb->gpio_cs);
+
+   gpiod_set_value_cansleep(tusb->gpio_cs, 1);
 
/*
 * VENDOR_SPECIFIC2 register in TUSB1210 can be used for configuring eye
-- 
2.1.4

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

Re: [Xen-devel] [Patch V2 2/3] usb: Introduce Xen pvUSB frontend

[Greg KH]

On Fri, Jun 12, 2015 at 05:33:46PM +0100, David Vrabel wrote:
> On 12/06/15 17:20, Greg KH wrote:
> > On Fri, Jun 12, 2015 at 04:10:00PM +0200, Juergen Gross wrote:
> >> Introduces the Xen pvUSB frontend. With pvUSB it is possible for a Xen
> >> domU to communicate with a USB device assigned to that domU. The
> >> communication is all done via the pvUSB backend in a driver domain
> >> (usually Dom0) which is owner of the physical device.
> >>
> >> The code is taken from the pvUSB implementation in Xen done by Fujitsu
> >> based on Linux kernel 2.6.18.
> >>
> >> Changes from the original version are:
> >> - port to upstream kernel
> >> - put all code in just one source file
> >> - move module to appropriate location in kernel tree
> >> - adapt to Linux style guide
> >> - minor code modifications to increase readability
> >>
> >> Signed-off-by: Juergen Gross 
> >> ---
> >>  drivers/usb/Kconfig|2 +
> >>  drivers/usb/Makefile   |2 +
> >>  drivers/usb/xen/Kconfig|   10 +
> >>  drivers/usb/xen/Makefile   |5 +
> >>  drivers/usb/xen/xen-usbfront.c | 1647 
> >> 
> > 
> > A subdirectory for a single file?  That seems like overkill, don't you
> > think?  As this is a USB "host" driver, why not put it in that
> > directory?
> > 
> > Also, last time these patches were posted, people asked why you can't
> > use libusb/usbfs instead, what happened with that?  Or usbip?
> 
> Using libusb was for the backend driver.  This frontend driver is a host
> controller (which is why it should be drivers/usb/host/xen-pv-hcd.c or
> similar).

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

Re: [PATCH v2 0/5] usb: Add usb interface authorization

[Alan Stern]

On Fri, 12 Jun 2015, Stefan Koch wrote:

> This is a patch that introduces an interface authorization for USB devices.
> The kernel supports already a device authorization bacause of wireless USB.
> 
> But the new interface authorization allows to enable or disable individual 
> interfaces per bitmask
> instead allow or deny a whole device.
> 
> The patch is made now much simplier as suggested by Alan Stern.
> 
> Each patch depends on all patches with a lesser number.
> 
> Stefan Koch (5):
>   usb: Add usb interface authorization: Declare attributes of structures
>   usb: Add usb interface authorization: Introduces the default interface
> authorization
>   usb: Add usb interface authorization: Control interface probing and
> claiming
>   usb: Add usb interface authorization: Introduces the usb interface
> authorization.
>   usb: Add usb interface authorization: SysFS part of usb interface
> authorization.

There is a lot of questionable material here.

First of all, I agree with Krzysztof that having an "authorized"  
attribute in each interface's sysfs directory would be simpler and 
easier to use than having a bitmask of all authorized interfaces.

Secondly, the patches have not been carefully edited.  There are
several misspelled words in comments and descriptions.  And why does
patch 3/5 modify drivers/base/base.h and include/linux/device.h?

Thirdly, what is the purpose of the mask_changed bit?  The changelog 
describes it as "a status bit to control a manual setting of the mask", 
which is not very clear.  _How_ does it control manual setting of the 
mask?  _Why_ does manual setting of the mask need to be controlled?

Fourthly, it doesn't look like usb_set_configuration() does the right 
thing.  It should call usb_enable_interface() whether the interface is 
authorized or not.

Fifthly, the code style is awkward in several places.  For instance, 
it looks silly to do this (usb_alloc_dev in patch 2/5):

dev->mask = 0;

/* invert the mask. each bit of the mask is now TRUE.
 * all interfaces should be allowed. */
dev->mask = ~dev->mask;

Not to mention that the accepted style for multi-line comments is
like this:

/*
 * This is a
 * long comment.
 */

Also, the kernel doesn't use CamelCase names like intfNr.

All these issues will have to be fixed before the patches can be 
accepted.

Alan Stern

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

FX3 on Linux

[Philip.Joslin]

Knowing that you have experts in USB 3.0 support for Linux, I am writing to 
your organization hoping that you may be able to offer some direction on a 
problem I have encountered.

I am trying to track down a problem I am having with a USB 3.0 PCIe-x1 card on 
Linux while trying to talk to a Cypress FX3 explorer board. I posted the 
following to the Cypress forums. However, after further investigation, I think 
it is the ASM1042 controller on the PCIe card. Other than going to a different 
USB 3.0 card, is there anything from the Linux side that I might be able to do 
to get by the error condition described below? I am doing asynchronous reads 
through libusb. Thanks for your help and insight.

 Cypress Post 
I have run into an odd problem using the FX3 (CYUSB3KIT-003) on Linux using 
libusb.
 
I have a java test app that calls down through C++ DLLs (so, dylib) to 
communicate
with the FX3 (which is running the default bulk src and sink firmware that came
programmed on the board). It does a simple sequence of operations: open, write 
block,
read block, single huge read of data, and close. Prior to each read, I fill the 
target
buffer with a set pattern. The FX3 firmware, on a read, fills the target with 
0xAA,
overwriting my pattern as it should. The blue LED on the board is blinking, so 
I know
we are using USB 3.0.
 
For Windows, these operations go through WinUSB. There are no problems. 
Everything
works well. Likewise everything works well with the Mac OSX using libusb. The
test programs can be run any number of times and the operations and incoming 
data
are fine.
 
With Linux, the first time through the test program, everything is fine. 
However,
running the program a second time results in the very first read either timing 
out with
the target buffer being filled with 0x00 (on one of my test machines) or (on 
another
test machine) having the read "succeed" (result indicates the number of bytes 
to be
read were read without error) but not having anything change in the target 
buffer.
If the read is immediately retried, it works fine (or if subsequent different 
reads
are done, they work fine).
 
It stays in this state until I either unplug and replug the board or reset the
system. On a couple of occasions, after unplugging and replugging the board, the
blue LED on the board does not even come on at all. At that point, I have to 
reboot
the system in order for it to even recognize that the FX3 is plugged in.
 
When I plug the FX3 board into a USB 2.0 only socket on the Linux machine (blue 
LED
on board is on solid), I see no problems. It is obviously slower, but everything
works as expected.
 
At this point, I am suspecting that either the Linux USB 3.0 handling has a 
problem,
libusb has a problem (though it worked fine on the 2.0 port), or the FX3 has a
problem (unlikely).
 
Are there know issues in using the FX3 on a USB 3.0 port on Linux? I am running 
a
64-bit quad core Xeon Intel machine running Mint 16. I have a PPA Int'l USB 3.0 
SuperSpeed
PCIe-x1 card that provides two USB 3.0 ports (per PPA's support group, the
GT50PCIEUSB3 USB 3.0 controller uses the ASM1042 chipset).
 
Thanks.
 End Cypress Post 
 

> lspci -vv (output for ASM1042 controller)

02:00.0 USB controller: ASMedia Technology Inc. ASM1042 SuperSpeed USB Host 
Controller (prog-if 30 [XHCI])
Subsystem: Device 1234:5678
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- SERR- 
Kernel driver in use: xhci_hcd

> lspci | grep USB
00:1a.0 USB controller: Intel Corporation 82801JI (ICH10 Family) USB UHCI 
Controller #4
00:1a.1 USB controller: Intel Corporation 82801JI (ICH10 Family) USB UHCI 
Controller #5
00:1a.7 USB controller: Intel Corporation 82801JI (ICH10 Family) USB2 EHCI 
Controller #2
00:1d.0 USB controller: Intel Corporation 82801JI (ICH10 Family) USB UHCI 
Controller #1
00:1d.1 USB controller: Intel Corporation 82801JI (ICH10 Family) USB UHCI 
Controller #2
00:1d.2 USB controller: Intel Corporation 82801JI (ICH10 Family) USB UHCI 
Controller #3
00:1d.3 USB controller: Intel Corporation 82801JI (ICH10 Family) USB UHCI 
Controller #6
00:1d.7 USB controller: Intel Corporation 82801JI (ICH10 Family) USB2 EHCI 
Controller #1
02:00.0 USB controller: ASMedia Technology Inc. ASM1042 SuperSpeed USB Host 
Controller

Thanks again.

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

Re: FX3 on Linux

[Rajaram R]

Please find reply inline

On Fri, Jun 12, 2015 at 11:48 PM,  wrote:
>
> Knowing that you have experts in USB 3.0 support for Linux, I am writing to 
> your organization hoping that you may be able to offer some direction on a 
> problem I have encountered.
>
> I am trying to track down a problem I am having with a USB 3.0 PCIe-x1 card 
> on Linux while trying to talk to a Cypress FX3 explorer board. I posted the 
> following to the Cypress forums. However, after further investigation, I 
> think it is the ASM1042 controller on the PCIe card. Other than going to a 
> different USB 3.0 card, is there anything from the Linux side that I might be 
> able to do to get by the error condition described below? I am doing 
> asynchronous reads through libusb. Thanks for your help and insight.
>
>  Cypress Post 
> I have run into an odd problem using the FX3 (CYUSB3KIT-003) on Linux using 
> libusb.
>
> I have a java test app that calls down through C++ DLLs (so, dylib) to 
> communicate
> with the FX3 (which is running the default bulk src and sink firmware that 
> came
> programmed on the board). It does a simple sequence of operations: open, 
> write block,
> read block, single huge read of data, and close. Prior to each read, I fill 
> the target
> buffer with a set pattern. The FX3 firmware, on a read, fills the target with 
> 0xAA,
> overwriting my pattern as it should. The blue LED on the board is blinking, 
> so I know
> we are using USB 3.0.
>
> For Windows, these operations go through WinUSB. There are no problems. 
> Everything
> works well. Likewise everything works well with the Mac OSX using libusb. The
> test programs can be run any number of times and the operations and incoming 
> data
> are fine.
>
> With Linux, the first time through the test program, everything is fine. 
> However,
> running the program a second time results in the very first read either 
> timing out with
> the target buffer being filled with 0x00 (on one of my test machines) or (on 
> another
> test machine) having the read "succeed" (result indicates the number of bytes 
> to be
> read were read without error) but not having anything change in the target 
> buffer.
> If the read is immediately retried, it works fine (or if subsequent different 
> reads
> are done, they work fine).

Please provide usbmon logs and dmesg prints from xHCI to understand
what is happening @ driver level.

>
> It stays in this state until I either unplug and replug the board or reset the
> system. On a couple of occasions, after unplugging and replugging the board, 
> the
> blue LED on the board does not even come on at all. At that point, I have to 
> reboot
> the system in order for it to even recognize that the FX3 is plugged in.
>
> When I plug the FX3 board into a USB 2.0 only socket on the Linux machine 
> (blue LED
> on board is on solid), I see no problems. It is obviously slower, but 
> everything
> works as expected.
>
> At this point, I am suspecting that either the Linux USB 3.0 handling has a 
> problem,
> libusb has a problem (though it worked fine on the 2.0 port), or the FX3 has a
> problem (unlikely).
>
> Are there know issues in using the FX3 on a USB 3.0 port on Linux?

There are no known issue. FX3 works with other platforms. This could
be xHCI limitation.

>I am running a
> 64-bit quad core Xeon Intel machine running Mint 16. I have a PPA Int'l USB 
> 3.0 SuperSpeed
> PCIe-x1 card that provides two USB 3.0 ports (per PPA's support group, the
> GT50PCIEUSB3 USB 3.0 controller uses the ASM1042 chipset).
>
> Thanks.
>  End Cypress Post 
>
>
> > lspci -vv (output for ASM1042 controller)
>
> 02:00.0 USB controller: ASMedia Technology Inc. ASM1042 SuperSpeed USB Host 
> Controller (prog-if 30 [XHCI])
> Subsystem: Device 1234:5678
> Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- 
> Stepping- SERR- FastB2B- DisINTx+
> Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- 
> SERR-  Latency: 0, Cache Line Size: 64 bytes
> Interrupt: pin A routed to IRQ 16
> Region 0: Memory at d410 (64-bit, non-prefetchable) [size=32K]
> Capabilities: 
> Kernel driver in use: xhci_hcd
>
> > lspci | grep USB
> 00:1a.0 USB controller: Intel Corporation 82801JI (ICH10 Family) USB UHCI 
> Controller #4
> 00:1a.1 USB controller: Intel Corporation 82801JI (ICH10 Family) USB UHCI 
> Controller #5
> 00:1a.7 USB controller: Intel Corporation 82801JI (ICH10 Family) USB2 EHCI 
> Controller #2
> 00:1d.0 USB controller: Intel Corporation 82801JI (ICH10 Family) USB UHCI 
> Controller #1
> 00:1d.1 USB controller: Intel Corporation 82801JI (ICH10 Family) USB UHCI 
> Controller #2
> 00:1d.2 USB controller: Intel Corporation 82801JI (ICH10 Family) USB UHCI 
> Controller #3
> 00:1d.3 USB controller: Intel Corporation 82801JI (ICH10 Family) USB UHCI 
> Controller #6
> 00:1d.7 USB controller: Intel Corporation 82801JI (ICH10 Family) USB2 EHCI 
> Controller #1
> 02:

Re: [PATCH v2 0/5] usb: Add usb interface authorization

[Stefan Koch]

Am Freitag, den 12.06.2015, 14:09 -0400 schrieb Alan Stern:
> On Fri, 12 Jun 2015, Stefan Koch wrote:
> There is a lot of questionable material here.
> 
> First of all, I agree with Krzysztof that having an "authorized"  
> attribute in each interface's sysfs directory would be simpler and 
> easier to use than having a bitmask of all authorized interfaces.
OK I can provide a patch for it. But note that the mask allows to enable
multiple interfaces at once. And the mechanism does enable all
(multiple) interfaces first and then does start the driver probing for
all interfaces. This mechanism is not possible without a mask.
But the attribute may be useful for single interfaces that work alone.
> 
> Secondly, the patches have not been carefully edited.  There are
> several misspelled words in comments and descriptions.  And why does
> patch 3/5 modify drivers/base/base.h and include/linux/device.h?
> 
It's needed that bus_probe_device() is defined in hub.c. To start
probing after authorize an interface.

> Thirdly, what is the purpose of the mask_changed bit?  The changelog 
> describes it as "a status bit to control a manual setting of the mask", 
> which is not very clear.  _How_ does it control manual setting of the 
> mask?  _Why_ does manual setting of the mask need to be controlled?
> 
If someone sets the mask the bit get's TRUE. After setting a new
configuration it is set back to FALSE.

So you could check if it is needed to ensure correct mask setting
(again).

> Fourthly, it doesn't look like usb_set_configuration() does the right 
> thing.  It should call usb_enable_interface() whether the interface is 
> authorized or not.
> 
> Fifthly, the code style is awkward in several places.  For instance, 
> it looks silly to do this (usb_alloc_dev in patch 2/5):
> 
>   dev->mask = 0;
> 
>   /* invert the mask. each bit of the mask is now TRUE.
>* all interfaces should be allowed. */
>   dev->mask = ~dev->mask;
> 
> Not to mention that the accepted style for multi-line comments is
> like this:
> 
>   /*
>* This is a
>* long comment.
>*/
> 
> Also, the kernel doesn't use CamelCase names like intfNr.
> 
> All these issues will have to be fixed before the patches can be 
> accepted.
> 
> Alan Stern
> 
Thanks

Stefan Koch


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

Re: [PATCH v2 0/5] usb: Add usb interface authorization

[Alan Stern]

On Fri, 12 Jun 2015, Stefan Koch wrote:

> Am Freitag, den 12.06.2015, 14:09 -0400 schrieb Alan Stern:
> > On Fri, 12 Jun 2015, Stefan Koch wrote:
> > There is a lot of questionable material here.
> > 
> > First of all, I agree with Krzysztof that having an "authorized"  
> > attribute in each interface's sysfs directory would be simpler and 
> > easier to use than having a bitmask of all authorized interfaces.
> OK I can provide a patch for it. But note that the mask allows to enable
> multiple interfaces at once. And the mechanism does enable all
> (multiple) interfaces first and then does start the driver probing for
> all interfaces. This mechanism is not possible without a mask.

You could probe all the interfaces whenever any interface is
authorized.  Or there could be a separate mechanism to initiate
probing.

> But the attribute may be useful for single interfaces that work alone.
> > 
> > Secondly, the patches have not been carefully edited.  There are
> > several misspelled words in comments and descriptions.  And why does
> > patch 3/5 modify drivers/base/base.h and include/linux/device.h?
> > 
> It's needed that bus_probe_device() is defined in hub.c. To start
> probing after authorize an interface.

How about calling device_attach() instead?

> > Thirdly, what is the purpose of the mask_changed bit?  The changelog 
> > describes it as "a status bit to control a manual setting of the mask", 
> > which is not very clear.  _How_ does it control manual setting of the 
> > mask?  _Why_ does manual setting of the mask need to be controlled?
> > 
> If someone sets the mask the bit get's TRUE. After setting a new
> configuration it is set back to FALSE.
> 
> So you could check if it is needed to ensure correct mask setting
> (again).

I don't understand.  If you want to make sure the mask is set 
correctly, you need to check the mask's _current_ value.  You don't 
care whether the mask has been changed from its _initial_ value.

Alan Stern

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

Re: [PATCH v2 0/5] usb: Add usb interface authorization

[Stefan Koch]

Am Freitag, den 12.06.2015, 16:34 -0400 schrieb Alan Stern:
> On Fri, 12 Jun 2015, Stefan Koch wrote:
> 
> > Am Freitag, den 12.06.2015, 14:09 -0400 schrieb Alan Stern:
> > > On Fri, 12 Jun 2015, Stefan Koch wrote:
> > > There is a lot of questionable material here.
> > > 
> > > First of all, I agree with Krzysztof that having an "authorized"  
> > > attribute in each interface's sysfs directory would be simpler and 
> > > easier to use than having a bitmask of all authorized interfaces.
> > OK I can provide a patch for it. But note that the mask allows to enable
> > multiple interfaces at once. And the mechanism does enable all
> > (multiple) interfaces first and then does start the driver probing for
> > all interfaces. This mechanism is not possible without a mask.
> 
> You could probe all the interfaces whenever any interface is
> authorized.  Or there could be a separate mechanism to initiate
> probing.
> 
Does this affect any running communication with the interface?
I'll send a simple patch. So in the one case the mask could used and in
the other case the interface attribute.

> > But the attribute may be useful for single interfaces that work alone.
> > > 
> > > Secondly, the patches have not been carefully edited.  There are
> > > several misspelled words in comments and descriptions.  And why does
> > > patch 3/5 modify drivers/base/base.h and include/linux/device.h?
> > > 
> > It's needed that bus_probe_device() is defined in hub.c. To start
> > probing after authorize an interface.
> 
> How about calling device_attach() instead?
bus_probe_device() checks the autoprobe status... Otherwise a getter for
the autoprobe status must implemented...
> 
> > > Thirdly, what is the purpose of the mask_changed bit?  The changelog 
> > > describes it as "a status bit to control a manual setting of the mask", 
> > > which is not very clear.  _How_ does it control manual setting of the 
> > > mask?  _Why_ does manual setting of the mask need to be controlled?
> > > 
> > If someone sets the mask the bit get's TRUE. After setting a new
> > configuration it is set back to FALSE.
> > 
> > So you could check if it is needed to ensure correct mask setting
> > (again).
> 
> I don't understand.  If you want to make sure the mask is set 
> correctly, you need to check the mask's _current_ value.  You don't 
> care whether the mask has been changed from its _initial_ value.
If you connect a device to an usb port udev runs for the device and all
interfaces.

If you change a configuration per hand udev runs only for interfaces,
not for the device.

So if you want to avoid to set the device's mask multiple times a status
bit helps.
> 
> Alan Stern
> 


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

[PATCH v2 6/5] usb: Add usb interface authorization: second SysFS part for usb interface authorization attribute.

[Stefan Koch]

To allow (1) or deny (0) interfaces a mask or interface attributes could 
written.
For interfaces that belongs together use the mask for authorization.
As default each bit has the initial value of the default authorization bit.
Entry: /sys/bus/usb/devices/*-*:*.*/interface_authorized

Signed-off-by: Stefan Koch 
---
 drivers/usb/core/sysfs.c | 55 
 1 file changed, 55 insertions(+)

diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index afa0799e..54e2e8e 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -995,6 +995,60 @@ static ssize_t supports_autosuspend_show(struct device 
*dev,
 }
 static DEVICE_ATTR_RO(supports_autosuspend);
 
+/*
+ * show authorization status of usb interface
+ * 1 is authorized, 0 is not authorized
+ */
+static ssize_t interface_authorized_show(struct device *dev,
+   struct device_attribute *attr, char *buf)
+{
+   struct usb_interface *intf = to_usb_interface(dev);
+
+   if (!intf)
+   return -ENODEV;
+
+   return sprintf(buf, "%u\n", intf->authorized);
+}
+
+/*
+ * authorize or deauthorize an usb interface
+ * 1 is to authorize, 0 is to deauthorize
+ */
+static ssize_t interface_authorized_store(struct device *dev,
+   struct device_attribute *attr, const char *buf, size_t count)
+{
+   struct usb_interface *intf = to_usb_interface(dev);
+   struct usb_device *usb_pdev = NULL;
+   int rc = 0;
+   unsigned val = 0;
+   unsigned intf_nr = 0;
+   u32 mask = 0;
+
+   if (!dev->parent || !intf || !intf->cur_altsetting)
+   return -ENODEV;
+
+   usb_pdev = to_usb_device(dev->parent);
+
+   if (!usb_pdev)
+   return -ENODEV;
+
+   mask = usb_pdev->mask;
+   intf_nr = intf->cur_altsetting->desc.bInterfaceNumber;
+
+   if (sscanf(buf, "%u\n", &val) != 1)
+   return -EINVAL;
+
+   if (val == 0)
+   mask &= ~(1 << intf_nr);
+   else if (val == 1)
+   mask |= (1 << intf_nr);
+
+   rc = usb_device_set_mask(dev->parent, mask);
+
+   return rc < 0 ? rc : count;
+}
+static DEVICE_ATTR_RW(interface_authorized);
+
 static struct attribute *intf_attrs[] = {
&dev_attr_bInterfaceNumber.attr,
&dev_attr_bAlternateSetting.attr,
@@ -1004,6 +1058,7 @@ static struct attribute *intf_attrs[] = {
&dev_attr_bInterfaceProtocol.attr,
&dev_attr_modalias.attr,
&dev_attr_supports_autosuspend.attr,
+   &dev_attr_interface_authorized.attr,
NULL,
 };
 static struct attribute_group intf_attr_grp = {
-- 
2.1.4

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

Re: [PATCH 1/1] usb: core: lpm: set lpm_capable for root hub device

[Lu, Baolu]

On 06/13/2015 01:43 AM, Alan Stern wrote:

On Fri, 12 Jun 2015, Lu Baolu wrote:


Commit 25cd2882e2fc ("usb/xhci: Change how we indicate a host supports
Link PM.") removed the code to set lpm_capable for USB 3.0 super-speed
root hub. The intention of that change was to avoid touching usb core
internal field, a.k.a. lpm_capable, and let usb core to set it by
checking U1 and U2 exit latency values in the descriptor.

Usb core checks and sets lpm_capable in hub_port_init(). Unfortunately,
root hub is a special usb device as it has no parent. Hub_port_init()
will never be called for a root hub device. That means lpm_capable will
by no means be set for the root hub. As the result, lpm isn't functional
at all in Linux kernel.

This patch add the code to check and set lpm_capable when registering a
root hub device. It could be back-ported to kernels as old as v3.15,
that contains the Commit 25cd2882e2fc ("usb/xhci: Change how we indicate
a host supports Link PM.").

Cc: sta...@vger.kernel.org # 3.15
Reported-by: Kevin Strasser 
Signed-off-by: Lu Baolu 
---
  drivers/usb/core/hcd.c | 6 ++
  drivers/usb/core/hub.c | 2 +-
  drivers/usb/core/usb.h | 1 +
  3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 45a915c..48b208d 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1032,6 +1032,12 @@ static int register_root_hub(struct usb_hcd *hcd)
}
}
  
+	if (le16_to_cpu(usb_dev->descriptor.bcdUSB) >= 0x0201) {

+   retval = usb_get_bos_descriptor(usb_dev);
+   if (!retval)
+   usb_dev->lpm_capable = usb_device_supports_lpm(usb_dev);
+   }
+

This should be integrated with the code immediately above it, which
also calls usb_get_bos_descriptor().  In fact, maybe you should simply
change the existing code to check bcdUSB >= 0x0201 instead of speed ==
USB_SPEED_SUPER.


I agree with you and will change it in v2 patch.




retval = usb_new_device (usb_dev);
if (retval) {
dev_err (parent_dev, "can't register root hub for %s, %d\n",
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 3b71516..884d702 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -122,7 +122,7 @@ struct usb_hub *usb_hub_to_struct_hub(struct usb_device 
*hdev)
return usb_get_intfdata(hdev->actconfig->interface[0]);
  }
  
-static int usb_device_supports_lpm(struct usb_device *udev)

+int usb_device_supports_lpm(struct usb_device *udev)
  {
/* USB 2.1 (and greater) devices indicate LPM support through
 * their USB 2.0 Extended Capabilities BOS descriptor.
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 7eb1e26..d5668c4 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -82,6 +82,7 @@ extern int usb_runtime_suspend(struct device *dev);
  extern int usb_runtime_resume(struct device *dev);
  extern int usb_runtime_idle(struct device *dev);
  extern int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable);
+extern int usb_device_supports_lpm(struct usb_device *udev);
  
  #else

This will break if you build it with CONFIG_PM disabled.


Yes. Will fix it in v2 patch.



Alan Stern


Thank you,
Baolu



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/




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

Re: [PATCH v2 6/5] usb: Add usb interface authorization: second SysFS part for usb interface authorization attribute.

[Greg KH]

On Fri, Jun 12, 2015 at 11:28:33PM +0200, Stefan Koch wrote:
> To allow (1) or deny (0) interfaces a mask or interface attributes could 
> written.
> For interfaces that belongs together use the mask for authorization.
> As default each bit has the initial value of the default authorization bit.
> Entry: /sys/bus/usb/devices/*-*:*.*/interface_authorized
> 
> Signed-off-by: Stefan Koch 
> ---
>  drivers/usb/core/sysfs.c | 55 
> 
>  1 file changed, 55 insertions(+)

Patch 6/5?  Please just fix up the series and resend a new whole series
please.

Also, all sysfs files need to have entries in Documentation/ABI/

thanks,

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

Re: [PATCH v2] USB SERIAL: option.c: add 2020:4000 IDs

[Greg Kroah-Hartman]

On Fri, Jun 12, 2015 at 09:32:31AM +0200, Claudio Cappelli wrote:
> On Wednesday 10 June 2015 20:38:30 Claudio Cappelli wrote:
> > From: Claudio Cappelli 
> > 
> > Add device Olivetti Olicard 300 (Network Connect: MT6225) - IDs 2020:4000.
> > 
> > Signed-off-by: Claudio Cappelli 
> > Suggested-by: Lars Melin 
> > 
> > ---
> > 
> > drivers/usb/serial/option.c |1 +
> >  1 file changed, 1 insertion(+)
> > 
> > 
> > 
> > --- linux/drivers/usb/serial/option.c.orig  2015-06-10 10:42:43.0 
> > +0200
> > +++ linux/drivers/usb/serial/option.c   2015-06-10 10:53:06.825265579 
> > +0200
> > @@ -1765,6 +1765,7 @@ static const struct usb_device_id option
> > { USB_DEVICE_AND_INTERFACE_INFO(0x2001, 0x7d03, 0xff, 0x00, 0x00) },
> > { USB_DEVICE_AND_INTERFACE_INFO(0x07d1, 0x3e01, 0xff, 0xff, 0xff) }, /* 
> > D-Link DWM-152/C1 */
> > { USB_DEVICE_AND_INTERFACE_INFO(0x07d1, 0x3e02, 0xff, 0xff, 0xff) }, /* 
> > D-Link DWM-156/C1 */
> > +   { USB_DEVICE_INTERFACE_CLASS(0x2020, 0x4000, 0xff) },/* 
> > OLICARD300 - MT6225 */
> > { USB_DEVICE(INOVIA_VENDOR_ID, INOVIA_SEW858) },
> > { USB_DEVICE(VIATELECOM_VENDOR_ID, VIATELECOM_PRODUCT_CDS7) },
> > { } /* Terminating entry */
> >
> 
> 
> 
> Was this second version in the right format?
> Since it was my first patch submit, I'd be
> grateful if you could confirm.

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

Re: Locking issues w/ functionfs gadget and aio?

[John Stultz]

On Mon, Jun 8, 2015 at 6:14 PM, John Stultz  wrote:
> After setting up functionfs for adb w/ 4.1-rc7, I noticed some flakey 
> behavior.
> I enabled some lock debugging and got the following:
>
> [   91.648093] read strings
> [   91.650264] g_ffs gadget: g_ffs ready
> [   91.652551] ci_hdrc ci_hdrc.0: CI_HDRC_CONTROLLER_RESET_EVENT received
> [   96.068693] BUG: spinlock lockup suspected on CPU#0, adbd/2791
> [   96.068751]  lock: 0xe7764880, .magic: e7764880, .owner: /-1,
> .owner_cpu: -407539900
> [   96.073448] CPU: 0 PID: 2791 Comm: adbd Not tainted
> 4.1.0-rc1-00032-g359b12f #147
> [   96.081688] Hardware name: Qualcomm (Flattened Device Tree)
> [   96.089266] [] (unwind_backtrace) from []
> (show_stack+0x10/0x14)
> [   96.094635] [] (show_stack) from []
> (dump_stack+0x70/0xbc)
> [   96.102627] [] (dump_stack) from []
> (do_raw_spin_lock+0x114/0x1a0)
> [   96.109661] [] (do_raw_spin_lock) from []
> (_raw_spin_lock_irqsave+0x50/0x5c)
> [   96.117560] [] (_raw_spin_lock_irqsave) from []
> (kiocb_set_cancel_fn+0x1c/0x60)
> [   96.126519] [] (kiocb_set_cancel_fn) from []
> (ffs_epfile_read_iter+0x8c/0x140)
> [   96.135289] [] (ffs_epfile_read_iter) from []
> (__vfs_read+0xb0/0xd4)
> [   96.144290] [] (__vfs_read) from [] 
> (vfs_read+0x7c/0x100)
> [   96.152535] [] (vfs_read) from [] (SyS_read+0x40/0x8c)
> [   96.159571] [] (SyS_read) from []
> (ret_fast_syscall+0x0/0x4c)
> [  117.678633] INFO: rcu_preempt detected stalls on CPUs/tasks:
> [  117.683069]  0: (1 GPs behind) idle=805/140/0
> softirq=7187/7189 fqs=2601
> [  117.683316]  (detected by 3, t=2603 jiffies, g=3028, c=3027, q=474)
> [  117.692345] Task dump for CPU 0:
> [  117.697202] adbdR running  0  2791  1 0x0002
> [  117.704296] [] (__schedule) from [] (0x)
>
>
> It seems we're stuck on the kioctx.ctx_lock, and reviewing that lock
> usage I don't see any problems in fs/aio.c right off.
>
> So I'm guessing the f_fs.c code is somehow not initializing the lock
> structure, or maybe calling kiocb_set_cancel_fn() from the wrong
> context?

So looking further at this, it seems that the __vfs_read() calls
new_sync_read(), which allocates a struct kiocb kiocb on the stack and
passes it to the ffs_epfile_read_iter() funciton. That then calls
kiocb_set_cancel_fn() passing a pointer to that kiocb. However,
kiocb_set_cancel_fn assumes the kiocb is a sub-element of a struct
aio_kiocb, and it tries to grab the kioctx from that parent structure.
However it seems there is no aio_kiocb structure here, so the
spin_lock_irqsave hangs trying to lock random data on the stack.

I'm not super sure what the right fix is, but if do something like the
following (sorry, whitespace corrupted via copy/paste), I don't seem
to run into the problem.

diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 3507f88..e322818 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -924,7 +924,8 @@ static ssize_t ffs_epfile_write_iter(struct kiocb
*kiocb, struct iov_iter *from)

kiocb->private = p;

-   kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);
+   if (p->aio)
+   kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);

res = ffs_epfile_io(kiocb->ki_filp, p);
if (res == -EIOCBQUEUED)
@@ -968,7 +969,8 @@ static ssize_t ffs_epfile_read_iter(struct kiocb
*kiocb, struct iov_iter *to)

kiocb->private = p;

-   kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);
+   if(p->aio)
+   kiocb_set_cancel_fn(kiocb, ffs_aio_cancel);

res = ffs_epfile_io(kiocb->ki_filp, p);
if (res == -EIOCBQUEUED)


Is there a better solution here? I'm not sure I see if the
is_sync_kiocb(kiocb) check would ever be false from here, so I'm not
sure if all the p->aio checking is really needed or not.

This issue seems to have been introduced with 70e60d917e91fff
(gadget/function/f_fs.c: switch to ->{read,write}_iter())

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