Re: [RFC ebeam PATCH 2/2] input: misc: New USB eBeam input driver

[Oliver Neukum]

On Mon, 2015-07-20 at 23:03 +0200, Yann Cantin wrote:
> Signed-off-by: Yann Cantin 
> ---
>  Documentation/ABI/testing/sysfs-driver-ebeam |  53 ++
>  drivers/input/misc/Kconfig   |  22 +
>  drivers/input/misc/Makefile  |   1 +
>  drivers/input/misc/ebeam.c   | 777 
> +++
>  4 files changed, 853 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-ebeam
>  create mode 100644 drivers/input/misc/ebeam.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-ebeam 
> b/Documentation/ABI/testing/sysfs-driver-ebeam
> new file mode 100644
> index 000..6873db5
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-ebeam
> @@ -0,0 +1,53 @@
> +What:/sys/class/input/inputXX/device/min_x
> + /sys/class/input/inputXX/device/min_y
> + /sys/class/input/inputXX/device/max_x
> + /sys/class/input/inputXX/device/max_y
> +Date:Jul 2015
> +Kernel Version:  4.1
> +Contact: yann.can...@laposte.net
> + linux-usb@vger.kernel.org
> +Description:
> + Reading from these files return the actually used range values 
> of
> + the reported coordinates.
> + Writing to these files preset these range values.
> + See below for the calibration procedure.
> +
> +What:/sys/class/input/inputXX/device/h[1..9]
> +Date:Jul 2015
> +Kernel Version:  4.1
> +Contact: yann.can...@laposte.net
> + linux-usb@vger.kernel.org
> +Description:
> + Reading from these files return the 3x3 transformation matrix 
> elements
> + actually used, in row-major.
> + Writing to these files preset these elements values.
> + See below for the calibration procedure.
> +
> +What:/sys/class/input/inputXX/device/calibrated
> +Date:Jul 2015
> +Kernel Version:  4.1
> +Contact: yann.can...@laposte.net
> + linux-usb@vger.kernel.org
> +Description:
> + Reading from this file :
> + - Return 0 if the driver is in "un-calibrated" mode : it 
> actually send
> +   raw coordinates in the device's internal coordinates system.
> + - Return 1 if the driver is in "calibrated" mode : it send 
> computed coordinates
> +   that (hopefully) matches the screen's coordinates system.
> + Writing 1 to this file enable the "calibrated" mode, 0 reset 
> the driver in
> + "un-calibrated" mode.
> +
> +Calibration procedure :
> +
> +When loaded, the driver is in "un-calibrated" mode : it send device's raw 
> coordinates
> +in the [0..65535]x[0..65535] range, the transformation matrix is the 
> identity.
> +
> +A calibration program have to compute a homography transformation matrix 
> that convert
> +the device's raw coordinates to the matching screen's ones.
> +It then write to the appropriate sysfs files the computed values, 
> pre-setting the
> +driver's parameters : xy range, and the matrix's elements.
> +When all values are passed, it write 1 to the calibrated sysfs file to 
> enable the "calibrated" mode.
> +
> +Warning : The parameters aren't used until 1 is writen to the calibrated 
> sysfs file.
> +
> +Writing 0 to the calibrated sysfs file reset the driver in "un-calibrated" 
> mode.
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index d4f0a81..22c46a4 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -103,6 +103,28 @@ config INPUT_E3X0_BUTTON
> To compile this driver as a module, choose M here: the
> module will be called e3x0_button.
>  
> +config INPUT_EBEAM_USB
> + tristate "USB eBeam driver"
> + depends on USB_ARCH_HAS_HCD
> + select USB
> + help
> +   Say Y here if you have a USB eBeam pointing device and want to
> +   use it without any proprietary user space tools.
> +
> +   Have a look at  for
> +   a usage description and the required user-space tools.
> +
> +   Supported devices :
> + - Luidia eBeam Classic Projection and eBeam Edge Projection
> + - Nec NP01Wi1 & NP01Wi2 interactive solution
> +
> +   Supposed working devices, need test, may lack functionality :
> + - Luidia eBeam Edge Whiteboard and eBeam Engage
> + - Hitachi Starboard FX-63, FX-77, FX-82, FX-77GII
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called ebeam.
> +
>  config INPUT_PCSPKR
>   tristate "PC Speaker support"
>   depends on PCSPKR_PLATFORM
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 53df07d..125f8a9 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_INPUT_DA9055_ONKEY)+= da9055_onkey.o
>  obj-$(CONFIG_INPUT_DA9

Re: [PATCH] Fix data loss in cdc-acm

[Johan Hovold]

On Mon, Jul 20, 2015 at 08:07:33PM +0200, Sven Brauch wrote:
> On 20/07/15 19:25, Johan Hovold wrote:

> > The idea of adding another layer of buffering in the cdc-acm driver has
> > been suggested in the past but was rejected (or at least questioned).
> > See for example this thread:
> > 
> > https://lkml.kernel.org/r/20110608164626.22bc8...@lxorguk.ukuu.org.uk
> Yes, that is indeed pretty much the same problem and the same solution.
> Answering to the questions brought up in that thread:
> 
> > a) Why is your setup filling 64K in the time it takes the throttle
> > response to occur
> As far as I understand, the throttle happens only when there's less than
> 128 bytes of free space in the tty buffer. Data can already be lost
> before the tty even decides it should start throttling, simply because
> the throttle threshold is smaller than the amount of data potentially in
> each urb. Also (excuse my cluelessness) it seems that when exactly the
> throttling happens depends on some scheduling "jitter" as well.
> Additionally, the response of the cdc_acm driver to a throttle request
> is not very prompt; it might have a queue of up to 16kB (16 urbs) pending.

Not really. The n_tty ldisc throttles when *its* buffer space is low,
but there should still be plenty of room in the tty buffers.

Looks like the ldisc processing is being starved.

> > b) Do we care (is the right thing to do to lose bits anyway at
> > that point)
> This I cannot answer, I don't know enough about the architecture or
> standards. I can just say that for my case, there's a lot of losses;
> this it not an issue which happens after hours when the system is under
> heavy load, it happens after just a few seconds reproducably.

In general if data isn't being consumed fast enough we eventually need
to drop it (unless we can throttle the producer).

But this isn't necessarily the case in your setup.

> > The tty buffers are quite large these days, but could possibly be bumped
> > further if needed to give the ldisc some more time to throttle the
> > device at very high speeds.
> I do not like this solution. It will again be based on luck, and you
> will still be unable to rely on the delivery guarantee made by the USB
> stack (at least when using bulk).

See above.

> My suggestion instead stops the host system from accepting any more data
> from the device when its buffers are full, forcing the device to wait
> before sending out more data (which many kinds of devices might very
> well be able to do).

This is what we are supposed to do today, but by relying on the ldisc
and tty buffers rather than forcing every driver to implement another
custom throttling mechanism.

But something obviously isn't working as intended.

Johan
--
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 v3 0/5] ARM: dts: OMAP2+: Enable USB dual-role on supported boards

[Tony Lindgren]

* Roger Quadros  [150708 03:45]:
> Hi,
> 
> Enables dual-role feaure on supported boards.
> 
> Depends on
> [1] - core USB DRD support - 
> http://thread.gmane.org/gmane.linux.kernel/1991413
> [2] - dwc3 DRD support - 
> http://thread.gmane.org/gmane.linux.usb.general/127890

Is this series safe to apply separately actually? Adding the
interrupts and the mode name may not be a problem without
the related driver changes?

Regards,

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

Re: [PATCH v3 07/11] usb: otg: add OTG core

[Li Jun]

Hi,

[...]

> >> +  otg_timer_init(A_WAIT_ENUM, otgd, set_tmout, TB_SRP_FAIL, NULL);
> > 
> > 2 timers are missing: B_DATA_PLS, B_SSEND_SRP.
> 
> Those 2 are not used by usb-otg-fsm.c. We can add it when usb-otg-fsm.c is 
> updated.
> 

ok.

> > 
> >> +}

[...]

> >> +
> >> +/**
> >> + * OTG FSM ops function to start/stop host
> >> + */
> >> +static int usb_otg_start_host(struct otg_fsm *fsm, int on)
> >> +{
> >> +  struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
> >> +  struct otg_hcd_ops *hcd_ops;
> >> +
> >> +  dev_dbg(otgd->dev, "otg: %s %d\n", __func__, on);
> >> +  if (!fsm->otg->host) {
> >> +  WARN_ONCE(1, "otg: fsm running without host\n");
> >> +  return 0;
> >> +  }
> >> +
> >> +  if (on) {
> >> +  /* OTG device operations */
> >> +  if (otgd->start_host)
> >> +  otgd->start_host(fsm, on);
> >> +
> >> +  /* start host */
> >> +  hcd_ops = otgd->primary_hcd.ops;
> >> +  hcd_ops->add(otgd->primary_hcd.hcd, otgd->primary_hcd.irqnum,
> >> +   otgd->primary_hcd.irqflags);
> >> +  if (otgd->shared_hcd.hcd) {
> >> +  hcd_ops = otgd->shared_hcd.ops;
> >> +  hcd_ops->add(otgd->shared_hcd.hcd,
> >> +   otgd->shared_hcd.irqnum,
> >> +   otgd->shared_hcd.irqflags);
> >> +  }
> >> +  } else {
> >> +  /* stop host */
> >> +  if (otgd->shared_hcd.hcd) {
> >> +  hcd_ops = otgd->shared_hcd.ops;
> >> +  hcd_ops->remove(otgd->shared_hcd.hcd);
> >> +  }
> >> +  hcd_ops = otgd->primary_hcd.ops;
> >> +  hcd_ops->remove(otgd->primary_hcd.hcd);
> >> +
> >> +  /* OTG device operations */
> >> +  if (otgd->start_host)
> >> +  otgd->start_host(fsm, on);
> >> +  }
> >> +
> >> +  return 0;
> >> +}
> > 
> > I do not see much benefit by this override function, usb_add/remove_hcd
> > can be simply included by controller's start_host function, also there
> > maybe some additional operations after usb_add_hcd, but this override
> > function limit usb_add_hcd() is the last step.
> 
> I had tried host start/stop way before but Alan's suggestion was to use
> bind/unbind the host controller completely as that is much simpler
> 
> [1] http://article.gmane.org/gmane.linux.usb.general/123842
> 

I did not mean host start/stop in your first version, I agree using
usb_add/remove_hcd() for simple.

> > 
> > Maybe your intention is to make usb_add_hcd is the only operation required
> > to start host, so ideally controller driver need not define its start_host
> > routine for this otg ops, I am not sure if this can work for different otg
> 
> Yes that was the intention.
> 
> > platforms. If the shared code is only usb_add/remove_hcd(), maybe leave this
> > ops defined by controller driver can make core code simple and give 
> > flexibility
> > to controller drivers.
> 
> We don't completely override start/stop_host(). The flexibility is still 
> there.
> We call controllers start_host(1) before starting the controller and 
> controllers
> start_host(0) after stopping the controller.
> So the the controller can still do what they want in 
> otg_fsm_ops.start_host/gadget().
> 

But if controller driver wants to do something after usb_otg_add_hcd(),
it's impossible with your current usb_otg_start_host().

> The OTG core only takes care of actually starting/stopping the host 
> controller.
> 
> If we don't do that then the code in usb_otg_start_host() has to be pasted
> in every OTG controller driver. This is code duplication.
> 

Actually the only duplication code may be a function call to original
usb_add/remove_hcd().

> > 
> >> +
> >> +/**
> >> + * OTG FSM ops function to start/stop gadget
> >> + */
> >> +static int usb_otg_start_gadget(struct otg_fsm *fsm, int on)
> >> +{
> >> +  struct otg_data *otgd = container_of(fsm, struct otg_data, fsm);
> >> +  struct usb_gadget *gadget = fsm->otg->gadget;
> >> +
> >> +  dev_dbg(otgd->dev, "otg: %s %d\n", __func__, on);
> >> +  if (!gadget) {
> >> +  WARN_ONCE(1, "otg: fsm running without gadget\n");
> >> +  return 0;
> >> +  }
> >> +
> >> +  if (on) {
> >> +  /* OTG device operations */
> >> +  if (otgd->start_gadget)
> >> +  otgd->start_gadget(fsm, on);
> >> +
> >> +  otgd->gadget_ops->start(fsm->otg->gadget);
> >> +  } else {
> >> +  otgd->gadget_ops->stop(fsm->otg->gadget);
> >> +
> >> +  /* OTG device operations */
> >> +  if (otgd->start_gadget)
> >> +  otgd->start_gadget(fsm, on);
> >> +  }
> >> +
> >> +  return 0;
> >> +}
> >> +
> >> +/**
> >> + * OTG FSM work function
> >> + */
> >> +static void usb_otg_work(struct work_struct *work)
> >> +{
> >> +  struct otg_data *otgd = container_of(work, struct otg_data, work);
> >> +
> >> +  otg_statemachine(&otgd->fsm);
> > 
> > Need consider run

Re: Several races in "usbnet" module (kernel 4.1.x)

[Oliver Neukum]

On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:
> Hi,
> 
> I have recently found several data races in "usbnet" module, checked on 
> vanilla kernel 4.1.0 on x86_64. The races do actually happen, I have 
> confirmed it by adding delays and using hardware breakpoints to detect 
> the conflicting memory accesses (with RaceHound tool, 
> https://github.com/winnukem/racehound).
> 
> I have not analyzed yet how harmful these races are (if they are), but 
> it is better to report them anyway, I think.
> 
> Everything was checked using YOTA 4G LTE Modem that works via "usbnet" 
> and "cdc_ether" kernel modules.
> --
> 
> [Race #1]
> 
> Race on skb_queue ('next' pointer) between usbnet_stop() and rx_complete().
> 
> Reproduced that by unplugging the device while the system was 
> downloading a large file from the Net.
> 
> Here is part of the call stack with the code where the changes to the 
> queue happen:
> 
> #0 __skb_unlink (skbuff.h:1517)   
>   prev->next = next;
> #1 defer_bh (usbnet.c:430)
>   spin_lock_irqsave(&list->lock, flags);
>   old_state = entry->state;
>   entry->state = state;
>   __skb_unlink(skb, list);
>   spin_unlock(&list->lock);
>   spin_lock(&dev->done.lock);
>   __skb_queue_tail(&dev->done, skb);
>   if (dev->done.qlen == 1)
>   tasklet_schedule(&dev->bh);
>   spin_unlock_irqrestore(&dev->done.lock, flags);
> #2 rx_complete (usbnet.c:640)
>   state = defer_bh(dev, skb, &dev->rxq, state);
> 
> At the same time, the following code repeatedly checks if the queue is 
> empty and reads the same values concurrently with the above changes:
> 
> #0  usbnet_terminate_urbs (usbnet.c:765)
>   /* maybe wait for deletions to finish. */
>   while (!skb_queue_empty(&dev->rxq)
>   && !skb_queue_empty(&dev->txq)
>   && !skb_queue_empty(&dev->done)) {
>   schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
>   set_current_state(TASK_UNINTERRUPTIBLE);
>   netif_dbg(dev, ifdown, dev->net,
> "waited for %d urb completions\n", temp);
>   }
> #1  usbnet_stop (usbnet.c:806)
>   if (!(info->flags & FLAG_AVOID_UNLINK_URBS))
>   usbnet_terminate_urbs(dev);
> 
> For example, it is possible that the skb is removed from dev->rxq by 
> __skb_unlink() before the check "!skb_queue_empty(&dev->rxq)" in 
> usbnet_terminate_urbs() is made. It is also possible in this case that 
> the skb is added to dev->done queue after "!skb_queue_empty(&dev->done)" 
> is checked. So usbnet_terminate_urbs() may stop waiting and return while 
> dev->done queue still has an item.

Hi,

your analysis is correct and it looks like in addition to your proposed
fix locking needs to be simplified and a common lock to be taken.
Suggestions?

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: [RFC ebeam PATCH 2/2] input: misc: New USB eBeam input driver

[Yann Cantin]

Hi,

Le 20/07/2015 23:59, Greg KH a écrit :

On Mon, Jul 20, 2015 at 11:03:19PM +0200, Yann Cantin wrote:

diff --git a/Documentation/ABI/testing/sysfs-driver-ebeam 
b/Documentation/ABI/testing/sysfs-driver-ebeam
+++ b/Documentation/ABI/testing/sysfs-driver-ebeam
@@ -0,0 +1,53 @@
+What:  /sys/class/input/inputXX/device/min_x
+   /sys/class/input/inputXX/device/min_y
+   /sys/class/input/inputXX/device/max_x
+   /sys/class/input/inputXX/device/max_y
+What:  /sys/class/input/inputXX/device/h[1..9]
+What:  /sys/class/input/inputXX/device/calibrated



What tool(s) use these sysfs files?  Don't we already have "normal"
events for these types of things such that we don't have to make up new
sysfs files for these?


The ebeam_calibrator tool is there : http://ebeam.tuxfamily.org.

I agree this can be a problem : this driver is totally useless without a
userspace dedicated calibration tool.

By nature these device's coordinate system can't be mapped to screen via
trivial transformations, such as scaling, flipping and rotating, hence the
special calibration and mapping procedures. I choose to use an homography
transformation as it is more robust and faster than linear interpolation
in this case. And anyway, it requires 9 calibration data and xy range
parameters.

I haven't found any existing tools performing that : xinput_calibrator
(witch ebeam_calibrator is based on) and other touchscreen calibration
tools can't do much more than trivial transformations.



+static DEVICE_ATTR(MM, S_IRUSR | S_IRGRP | S_IWUSR | S_IWGRP, \
+  ebeam_##MM##_get,   \
+  ebeam_##MM##_set)


DEVICE_ATTR_RW()?


Ok, will do.



+   /* sysfs setup */
+   err = sysfs_create_group(&intf->dev.kobj, &ebeam_attr_group);


Ick, you just added the sysfs files to the USB device, not your input
device, are you sure you tested this?


Yes, "run fine since 3.3.6, both x86_32 and 64.".

thanks,

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

Re: [PATCH v5 0/6] usb: interface authorization

[Stefan Koch]

Am Montag, den 29.06.2015, 15:41 -0700 schrieb Greg KH:
> On Mon, Jun 29, 2015 at 11:23:47PM +0200, Stefan Koch wrote:
> > Am Donnerstag, den 18.06.2015, 13:30 -0400 schrieb Alan Stern:
> > > On Thu, 18 Jun 2015, Stefan Koch wrote:
> > > 
> > > > This patch introduces an interface authorization for USB devices.
> > > > The kernel supports a device authorization because of wireless USB.
> > > > 
> > > > But the new interface authorization allows to authorize or deauthorize
> > > > individual interfaces instead authorization or deauthorize a whole 
> > > > device.
> > > > 
> > > > Therefore the authorized attribute is introduced for each interface.
> > > > 
> > > > Each patch depends on all patches with a lesser number.
> > > > 
> > > > Stefan Koch (6):
> > > >   usb: interface authorization: Declare authorized attribute
> > > >   usb: interface authorization: Introduces the default interface
> > > > authorization
> > > >   usb: interface authorization: Control interface probing and claiming
> > > >   usb: interface authorization: Introduces the USB interface
> > > > authorization.
> > > >   usb: interface authorization: SysFS part of USB interface
> > > > authorization.
> > > >   usb: interface authorization: Documentation part
> > > > 
> > > >  Documentation/ABI/testing/sysfs-bus-usb | 22 ++
> > > >  Documentation/usb/authorization.txt | 34 +
> > > >  drivers/usb/core/driver.c   | 11 +++
> > > >  drivers/usb/core/hcd.c  | 52 
> > > > +
> > > >  drivers/usb/core/message.c  | 48 
> > > > ++
> > > >  drivers/usb/core/sysfs.c| 41 ++
> > > >  drivers/usb/core/usb.h  |  2 ++
> > > >  include/linux/usb.h |  1 +
> > > >  include/linux/usb/hcd.h |  3 ++
> > > >  9 files changed, 214 insertions(+)
> > > 
> > > Acked-by: Alan Stern 
> > > 
> > > Other people may still have some comments.
> > > 
> > > Alan Stern
> > > 
> > Hi
> > 
> > is there any current merging state?
> > 
> > In what git tree the patch will submitted at the first step?
> 
> I can't do anything at the moment due to the merge window.  I'll review
> it after 4.2-rc1 is out (usually a week or so afterward, as my queue is
> quite large).  Don't worry, it's not lost.
> 
> greg k-h

Are there any news about the merge state?

Is there enogh time for testing to get the patch into 4.2?
If not the documentation must changed
(Documentation/ABI/testing/sysfs-bus-usb) because there is noted 4.2

Thanks

Stefan


--
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: [RFC ebeam PATCH 2/2] input: misc: New USB eBeam input driver

[Yann Cantin]

Hi,

Le 21/07/2015 10:19, Oliver Neukum a écrit :

On Mon, 2015-07-20 at 23:03 +0200, Yann Cantin wrote:



diff --git a/drivers/input/misc/ebeam.c b/drivers/input/misc/ebeam.c
new file mode 100644
index 000..79cac51
--- /dev/null
+++ b/drivers/input/misc/ebeam.c



+/* Electronics For Imaging, Inc */
+#define USB_VENDOR_ID_EFI  0x2650


You are defining these IDs twice. That is not good.


Is it okay to do this :

#if !defined(CONFIG_INPUT_EBEAM_USB)
#define USB_VENDOR_ID_EFI   0x2650
#...
#endif

so that the driver can also be build outside the kernel tree ?

thanks,

--
Yann Cantin
A4FEB47F
--
--
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: Several races in "usbnet" module (kernel 4.1.x)

[Oliver Neukum]

On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:
> Races on dev->rx_qlen. Reproduced these by repeatedly changing MTU
> (1500 
> <-> 1400) while downloading large files.

Hi,

I don't see how it matters much. The number of buffers is just
an optimization. As long as it eventually is corrected I don't
see harm.

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] Fix data loss in cdc-acm

[Oliver Neukum]

On Mon, 2015-07-20 at 20:07 +0200, Sven Brauch wrote:
> On 20/07/15 19:25, Johan Hovold wrote:
> > What kernel version are you using?
> I'm using linux 4.1.2.
> 
> > The idea of adding another layer of buffering in the cdc-acm driver has
> > been suggested in the past but was rejected (or at least questioned).
> > See for example this thread:
> > 
> > https://lkml.kernel.org/r/20110608164626.22bc8...@lxorguk.ukuu.org.uk
> Yes, that is indeed pretty much the same problem and the same solution.
> Answering to the questions brought up in that thread:

Yes, but that was not really a modem. The stuff on the other end
doesn't come over an external conection.

> > a) Why is your setup filling 64K in the time it takes the throttle
> > response to occur
> As far as I understand, the throttle happens only when there's less than
> 128 bytes of free space in the tty buffer. Data can already be lost
> before the tty even decides it should start throttling, simply because
> the throttle threshold is smaller than the amount of data potentially in
> each urb. Also (excuse my cluelessness) it seems that when exactly the

Then there is a problem in the tty code. Now it may not have enough
information, but there is no point of turning the buffers of cdc-acm
into an inofficial buffer.

> throttling happens depends on some scheduling "jitter" as well.
> Additionally, the response of the cdc_acm driver to a throttle request
> is not very prompt; it might have a queue of up to 16kB (16 urbs) pending.
> 
> > b) Do we care (is the right thing to do to lose bits anyway at
> > that point)
> This I cannot answer, I don't know enough about the architecture or
> standards. I can just say that for my case, there's a lot of losses;
> this it not an issue which happens after hours when the system is under
> heavy load, it happens after just a few seconds reproducably.

Then the tty layer needs to throttle earlier. In the general case we
must be ready to throw away data.

> > The tty buffers are quite large these days, but could possibly be bumped
> > further if needed to give the ldisc some more time to throttle the
> > device at very high speeds.
> I do not like this solution. It will again be based on luck, and you
> will still be unable to rely on the delivery guarantee made by the USB
> stack (at least when using bulk).
> My suggestion instead stops the host system from accepting any more data
> from the device when its buffers are full, forcing the device to wait
> before sending out more data (which many kinds of devices might very
> well be able to do).

But others won't and we'd preserve stale data in preference over fresh
data.

> Also note that this patch does not introduce an extra layer of
> buffering. The buffers are already there; this change just alters the
> process which decides when to submit the buffers to the tty, and when to
> free them for more input data from the device.

It does. It turns those buffers from temporary scratch buffers into
a queue.

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: [RFC ebeam PATCH 2/2] input: misc: New USB eBeam input driver

[Yann Cantin]

Hi,

Le 21/07/2015 00:40, Greg KH a écrit :

On Mon, Jul 20, 2015 at 03:26:40PM -0700, Dmitry Torokhov wrote:

On Mon, Jul 20, 2015 at 02:59:56PM -0700, Greg KH wrote:

On Mon, Jul 20, 2015 at 11:03:19PM +0200, Yann Cantin wrote:

Signed-off-by: Yann Cantin 



+
+   /* sysfs setup */
+   err = sysfs_create_group(&intf->dev.kobj, &ebeam_attr_group);


Ick, you just added the sysfs files to the USB device, not your input
device, are you sure you tested this?

And there should be a race-free way to add an attribute group to an
input device, as this is, you are adding them to the device _after_ it
is created, so userspace will not see them at creation time, causing a
race.


No, there are no driver-specific attributed on input devices themselves,
they belong to the actual hardware devices. The input devices only
export standard attributes applicable to every and all input devices
in the system.


Then the Documentation in this patch better be fixed up, as it points to
the input device as having these sysfs files :)

But as these are input device attributes, and not USB device interface
attributes, putting them on the USB interface doesn't make much sense,


To sum up : these attributes are USB device's not input's, only indirectly
accessed via inputXX/device/, and they only modify the driver's behavior.
So, it make sense to correct the documentation to point at
/sys/bus/usb/drivers/ebeam/X-X:1.0/. Right ?

thanks,

--
Yann Cantin
A4FEB47F
--
--
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 7/7] xhci: do not report PLC when link is in internal resume state

[Mathias Nyman]

From: Zhuang Jin Can 

Port link change with port in resume state should not be
reported to usbcore, as this is an internal state to be
handled by xhci driver. Reporting PLC to usbcore may
cause usbcore clearing PLC first and port change event irq
won't be generated.

Cc: 
Signed-off-by: Zhuang Jin Can 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-hub.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index fdca8ed..78241b5 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -591,7 +591,14 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
status |= USB_PORT_STAT_C_RESET << 16;
/* USB3.0 only */
if (hcd->speed == HCD_USB3) {
-   if ((raw_port_status & PORT_PLC))
+   /* Port link change with port in resume state should not be
+* reported to usbcore, as this is an internal state to be
+* handled by xhci driver. Reporting PLC to usbcore may
+* cause usbcore clearing PLC first and port change event
+* irq won't be generated.
+*/
+   if ((raw_port_status & PORT_PLC) &&
+   (raw_port_status & PORT_PLS_MASK) != XDEV_RESUME)
status |= USB_PORT_STAT_C_LINK_STATE << 16;
if ((raw_port_status & PORT_WRC))
status |= USB_PORT_STAT_C_BH_RESET << 16;
-- 
1.8.3.2

--
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 1/7] xhci: call BIOS workaround to enable runtime suspend on Intel Braswell

[Mathias Nyman]

Intel xhci hw that require XHCI_PME_STUCK quirk have as default disabled
xhci from going to D3 state in runtime suspend. Driver needs to verify
it can deal with the hw by calling an ACPI _DSM method to get D3 enabled.

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-pci.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 4a4cb1d..da10dc7 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "xhci.h"
 #include "xhci-trace.h"
@@ -190,6 +191,19 @@ static void xhci_pme_quirk(struct xhci_hcd *xhci)
readl(reg);
 }
 
+#ifdef CONFIG_ACPI
+static void xhci_pme_acpi_rtd3_enable(struct pci_dev *dev)
+{
+   static const u8 intel_dsm_uuid[] = {
+   0xb7, 0x0c, 0x34, 0xac, 0x01, 0xe9, 0xbf, 0x45,
+   0xb7, 0xe6, 0x2b, 0x34, 0xec, 0x93, 0x1e, 0x23,
+   };
+   acpi_evaluate_dsm(ACPI_HANDLE(&dev->dev), intel_dsm_uuid, 3, 1, NULL);
+}
+#else
+   static void xhci_pme_acpi_rtd3_enable(struct pci_dev *dev) { }
+#endif /* CONFIG_ACPI */
+
 /* called during probe() after chip reset completes */
 static int xhci_pci_setup(struct usb_hcd *hcd)
 {
@@ -263,6 +277,9 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct 
pci_device_id *id)
HCC_MAX_PSA(xhci->hcc_params) >= 4)
xhci->shared_hcd->can_do_streams = 1;
 
+   if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
+   xhci_pme_acpi_rtd3_enable(dev);
+
/* USB-2 and USB-3 roothubs initialized, allow runtime pm suspend */
pm_runtime_put_noidle(&dev->dev);
 
-- 
1.8.3.2

--
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 0/7] xhci fixes for 4.2 rc

[Mathias Nyman]

Hi Greg

These xhci fixes for 4.2-rc4 are mostly powermanagement related.
Fixing xhci SuperSpeed port resume state issues, and Intel Braswell xhci
specific D3 issues.

A couple other fixes correcting software TT bandwidth calculation, and a fix
for a "off by one" ring cache issue that caused NULL pointer dereference.

-Mathias

AMAN DEEP (1):
  usb: xhci: Bugfix for NULL pointer deference in xhci_endpoint_init()
function

Brian Campbell (1):
  xhci: Calculate old endpoints correctly on device reset

Mathias Nyman (1):
  xhci: call BIOS workaround to enable runtime suspend on Intel Braswell

Rajmohan Mani (1):
  xhci: Workaround to get D3 working in Intel xHCI

Zhuang Jin Can (3):
  xhci: report U3 when link is in resume state
  xhci: prevent bus_suspend if SS port resuming in phase 1
  xhci: do not report PLC when link is in internal resume state

 drivers/usb/host/xhci-hub.c  | 22 -
 drivers/usb/host/xhci-mem.c  |  2 +-
 drivers/usb/host/xhci-pci.c  | 57 +---
 drivers/usb/host/xhci-ring.c |  3 +++
 drivers/usb/host/xhci.c  |  3 +++
 drivers/usb/host/xhci.h  |  1 +
 6 files changed, 78 insertions(+), 10 deletions(-)

-- 
1.8.3.2

--
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 2/7] xhci: Workaround to get D3 working in Intel xHCI

[Mathias Nyman]

From: Rajmohan Mani 

The xHCI in Intel CherryView / Braswell Platform requires
a driver workaround to get xHCI D3 working. Without this
workaround, xHCI might not enter D3.

Workaround is to configure SSIC PORT as "unused" before D3
entry and "used" after D3 exit. This is done through a
vendor specific register (PORT2_SSIC_CONFIG_REG2 at offset
0x883c), in xhci suspend / resume callbacks.

Verified xHCI D3 works fine in CherryView / Braswell platform.

Signed-off-by: Rajmohan Mani 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-pci.c | 40 +---
 1 file changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index da10dc7..5590eac 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -28,6 +28,10 @@
 #include "xhci.h"
 #include "xhci-trace.h"
 
+#define PORT2_SSIC_CONFIG_REG2 0x883c
+#define PROG_DONE  (1 << 30)
+#define SSIC_PORT_UNUSED   (1 << 31)
+
 /* Device for a quirk */
 #define PCI_VENDOR_ID_FRESCO_LOGIC 0x1b73
 #define PCI_DEVICE_ID_FRESCO_LOGIC_PDK 0x1000
@@ -177,14 +181,44 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
 }
 
 /*
+ * In some Intel xHCI controllers, in order to get D3 working,
+ * through a vendor specific SSIC CONFIG register at offset 0x883c,
+ * SSIC PORT need to be marked as "unused" before putting xHCI
+ * into D3. After D3 exit, the SSIC port need to be marked as "used".
+ * Without this change, xHCI might not enter D3 state.
  * Make sure PME works on some Intel xHCI controllers by writing 1 to clear
  * the Internal PME flag bit in vendor specific PMCTRL register at offset 
0x80a4
  */
-static void xhci_pme_quirk(struct xhci_hcd *xhci)
+static void xhci_pme_quirk(struct usb_hcd *hcd, bool suspend)
 {
+   struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+   struct pci_dev  *pdev = to_pci_dev(hcd->self.controller);
u32 val;
void __iomem *reg;
 
+   if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
+pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) {
+
+   reg = (void __iomem *) xhci->cap_regs + PORT2_SSIC_CONFIG_REG2;
+
+   /* Notify SSIC that SSIC profile programming is not done */
+   val = readl(reg) & ~PROG_DONE;
+   writel(val, reg);
+
+   /* Mark SSIC port as unused(suspend) or used(resume) */
+   val = readl(reg);
+   if (suspend)
+   val |= SSIC_PORT_UNUSED;
+   else
+   val &= ~SSIC_PORT_UNUSED;
+   writel(val, reg);
+
+   /* Notify SSIC that SSIC profile programming is done */
+   val = readl(reg) | PROG_DONE;
+   writel(val, reg);
+   readl(reg);
+   }
+
reg = (void __iomem *) xhci->cap_regs + 0x80a4;
val = readl(reg);
writel(val | BIT(28), reg);
@@ -324,7 +358,7 @@ static int xhci_pci_suspend(struct usb_hcd *hcd, bool 
do_wakeup)
pdev->no_d3cold = true;
 
if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
-   xhci_pme_quirk(xhci);
+   xhci_pme_quirk(hcd, true);
 
return xhci_suspend(xhci, do_wakeup);
 }
@@ -357,7 +391,7 @@ static int xhci_pci_resume(struct usb_hcd *hcd, bool 
hibernated)
usb_enable_intel_xhci_ports(pdev);
 
if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
-   xhci_pme_quirk(xhci);
+   xhci_pme_quirk(hcd, false);
 
retval = xhci_resume(xhci, hibernated);
return retval;
-- 
1.8.3.2

--
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 4/7] xhci: Calculate old endpoints correctly on device reset

[Mathias Nyman]

From: Brian Campbell 

When resetting a device the number of active TTs may need to be
corrected by xhci_update_tt_active_eps, but the number of old active
endpoints supplied to it was always zero, so the number of TTs and the
bandwidth reserved for them was not updated, and could rise
unnecessarily.

This affected systems using Intel's Patherpoint chipset, which rely on
software bandwidth checking.  For example, a Lenovo X230 would lose the
ability to use ports on the docking station after enough suspend/resume
cycles because the bandwidth calculated would rise with every cycle when
a suitable device is attached.

The correct number of active endpoints is calculated in the same way as
in xhci_reserve_bandwidth.

Cc: 
Signed-off-by: Brian Campbell 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 7da0d60..526ebc0 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3453,6 +3453,9 @@ int xhci_discover_or_reset_device(struct usb_hcd *hcd, 
struct usb_device *udev)
return -EINVAL;
}
 
+   if (virt_dev->tt_info)
+   old_active_eps = virt_dev->tt_info->active_eps;
+
if (virt_dev->udev != udev) {
/* If the virt_dev and the udev does not match, this virt_dev
 * may belong to another udev.
-- 
1.8.3.2

--
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 3/7] usb: xhci: Bugfix for NULL pointer deference in xhci_endpoint_init() function

[Mathias Nyman]

From: AMAN DEEP 

virt_dev->num_cached_rings counts on freed ring and is not updated
correctly. In xhci_free_or_cache_endpoint_ring() function, the free ring
is added into cache and then num_rings_cache is incremented as below:
virt_dev->ring_cache[rings_cached] =
virt_dev->eps[ep_index].ring;
virt_dev->num_rings_cached++;
here, free ring pointer is added to a current index and then
index is incremented.
So current index always points to empty location in the ring cache.
For getting available free ring, current index should be decremented
first and then corresponding ring buffer value should be taken from ring
cache.

But In function xhci_endpoint_init(), the num_rings_cached index is
accessed before decrement.
virt_dev->eps[ep_index].new_ring =
virt_dev->ring_cache[virt_dev->num_rings_cached];
virt_dev->ring_cache[virt_dev->num_rings_cached] = NULL;
virt_dev->num_rings_cached--;
This is bug in manipulating the index of ring cache.
And it should be as below:
virt_dev->num_rings_cached--;
virt_dev->eps[ep_index].new_ring =
virt_dev->ring_cache[virt_dev->num_rings_cached];
virt_dev->ring_cache[virt_dev->num_rings_cached] = NULL;

Cc: 
Signed-off-by: Aman Deep 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index f833640..3e442f7 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1427,10 +1427,10 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
/* Attempt to use the ring cache */
if (virt_dev->num_rings_cached == 0)
return -ENOMEM;
+   virt_dev->num_rings_cached--;
virt_dev->eps[ep_index].new_ring =
virt_dev->ring_cache[virt_dev->num_rings_cached];
virt_dev->ring_cache[virt_dev->num_rings_cached] = NULL;
-   virt_dev->num_rings_cached--;
xhci_reinit_cached_ring(xhci, virt_dev->eps[ep_index].new_ring,
1, type);
}
-- 
1.8.3.2

--
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 6/7] xhci: prevent bus_suspend if SS port resuming in phase 1

[Mathias Nyman]

From: Zhuang Jin Can 

When the link is just waken, it's in Resume state, and driver sets PLS to
U0. This refers to Phase 1. Phase 2 refers to when the link has completed
the transition from Resume state to U0.

With the fix of xhci: report U3 when link is in resume state, it also
exposes an issue that usb3 roothub and controller can suspend right
after phase 1, and this causes a hard hang in controller.

To fix the issue, we need to prevent usb3 bus suspend if any port is
resuming in phase 1.

[merge separate USB2 and USB3 port resume checking to one -Mathias]
Cc: 
Signed-off-by: Zhuang Jin Can 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-hub.c  | 6 +++---
 drivers/usb/host/xhci-ring.c | 3 +++
 drivers/usb/host/xhci.h  | 1 +
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 7df8878..fdca8ed 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1123,10 +1123,10 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
spin_lock_irqsave(&xhci->lock, flags);
 
if (hcd->self.root_hub->do_remote_wakeup) {
-   if (bus_state->resuming_ports) {
+   if (bus_state->resuming_ports ||/* USB2 */
+   bus_state->port_remote_wakeup) {/* USB3 */
spin_unlock_irqrestore(&xhci->lock, flags);
-   xhci_dbg(xhci, "suspend failed because "
-   "a port is resuming\n");
+   xhci_dbg(xhci, "suspend failed because a port is 
resuming\n");
return -EBUSY;
}
}
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 94416ff..6a8fc52 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1546,6 +1546,9 @@ static void handle_port_status(struct xhci_hcd *xhci,
usb_hcd_resume_root_hub(hcd);
}
 
+   if (hcd->speed == HCD_USB3 && (temp & PORT_PLS_MASK) == XDEV_INACTIVE)
+   bus_state->port_remote_wakeup &= ~(1 << faked_port_index);
+
if ((temp & PORT_PLC) && (temp & PORT_PLS_MASK) == XDEV_RESUME) {
xhci_dbg(xhci, "port resume event for port %d\n", port_id);
 
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 31e46cc..ed2ebf6 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -285,6 +285,7 @@ struct xhci_op_regs {
 #define XDEV_U0(0x0 << 5)
 #define XDEV_U2(0x2 << 5)
 #define XDEV_U3(0x3 << 5)
+#define XDEV_INACTIVE  (0x6 << 5)
 #define XDEV_RESUME(0xf << 5)
 /* true: port has power (see HCC_PPC) */
 #define PORT_POWER (1 << 9)
-- 
1.8.3.2

--
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 5/7] xhci: report U3 when link is in resume state

[Mathias Nyman]

From: Zhuang Jin Can 

xhci_hub_report_usb3_link_state() returns pls as U0 when the link
is in resume state, and this causes usb core to think the link is in
U0 while actually it's in resume state. When usb core transfers
control request on the link, it fails with TRB error as the link
is not ready for transfer.

To fix the issue, report U3 when the link is in resume state, thus
usb core knows the link it's not ready for transfer.

Cc: 
Signed-off-by: Zhuang Jin Can 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-hub.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index e75c565..7df8878 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -484,10 +484,13 @@ static void xhci_hub_report_usb3_link_state(struct 
xhci_hcd *xhci,
u32 pls = status_reg & PORT_PLS_MASK;
 
/* resume state is a xHCI internal state.
-* Do not report it to usb core.
+* Do not report it to usb core, instead, pretend to be U3,
+* thus usb core knows it's not ready for transfer
 */
-   if (pls == XDEV_RESUME)
+   if (pls == XDEV_RESUME) {
+   *status |= USB_SS_PORT_LS_U3;
return;
+   }
 
/* When the CAS bit is set then warm reset
 * should be performed on port
-- 
1.8.3.2

--
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: Several races in "usbnet" module (kernel 4.1.x)

[Oliver Neukum]

On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:
> And here, the code clears EVENT_RX_KILL bit in dev->flags, which may 
> execute concurrently with the above operation:
> #0 clear_bit (bitops.h:113, inlined)
> #1 usbnet_bh (usbnet.c:1475)
> /* restart RX again after disabling due to high error rate */
> clear_bit(EVENT_RX_KILL, &dev->flags);
> 
> If clear_bit() is atomic w.r.t. setting dev->flags to 0, this race is 
> not a problem, I guess. Otherwise, it may be.

clear_bit is atomic with respect to other atomic operations.
So how about this:

Regards
Oliver

>From 1c4e685b3a9c183e04c46b661830e5c7ed35b513 Mon Sep 17 00:00:00 2001
From: Oliver Neukum 
Date: Tue, 21 Jul 2015 16:19:40 +0200
Subject: [PATCH] usbnet: fix race between usbnet_stop() and the BH

Does this do the job?

Signed-off-by: Oliver Neukum 
---
 drivers/net/usb/usbnet.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 3c86b10..77a9a86 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -778,7 +778,7 @@ int usbnet_stop (struct net_device *net)
 {
struct usbnet   *dev = netdev_priv(net);
struct driver_info  *info = dev->driver_info;
-   int retval, pm;
+   int retval, pm, mpn;
 
clear_bit(EVENT_DEV_OPEN, &dev->flags);
netif_stop_queue (net);
@@ -813,14 +813,17 @@ int usbnet_stop (struct net_device *net)
 * can't flush_scheduled_work() until we drop rtnl (later),
 * else workers could deadlock; so make workers a NOP.
 */
+   mpn = !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
dev->flags = 0;
del_timer_sync (&dev->delay);
tasklet_kill (&dev->bh);
+   mpn |= !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags);
+   /* in case the bh reset a flag */
+   dev->flags = 0;
if (!pm)
usb_autopm_put_interface(dev->intf);
 
-   if (info->manage_power &&
-   !test_and_clear_bit(EVENT_NO_RUNTIME_PM, &dev->flags))
+   if (info->manage_power && mpn)
info->manage_power(dev, 0);
else
usb_autopm_put_interface(dev->intf);
-- 
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] usb: dwc3: Reset the transfer resource index on SET_INTERFACE

[Felipe Balbi]

From: John Youn 

This fixes an issue introduced in commit b23c843992b6 (usb: dwc3:
gadget: fix DEPSTARTCFG for non-EP0 EPs) that made sure we would
only use DEPSTARTCFG once per SetConfig.

The trick is that we should use one DEPSTARTCFG per SetConfig *OR*
SetInterface. SetInterface was completely missed from the original
patch.

This problem became aparent after commit 76e838c9f776 (usb: dwc3:
gadget: return error if command sent to DEPCMD register fails)
added checking of the return status of device endpoint commands.

'Set Endpoint Transfer Resource' command was caught failing
occasionally. This is because the Transfer Resource
Index was not getting reset during a SET_INTERFACE request.

Finally, to fix the issue, was we have to do is make sure that
our start_config_issued flag gets reset whenever we receive a
SetInterface request.

To verify the problem (and its fix), all we have to do is run
test 9 from testusb with 'testusb -t 9 -s 2048 -a -c 5000'.

Fixes: b23c843992b6 (usb: dwc3: gadget: fix DEPSTARTCFG for non-EP0 EPs)
Cc:  # v3.2+
Signed-off-by: John Youn 
Signed-off-by: Felipe Balbi 
---

I have improved commit log and blamed correct commit. Also added stable Cc so
we have a chance of fixing older kernels.

If you can test on your end, I'd be glad to add Tested-bys, but do so by
tomorrow because it's extra important that we get this patch in.

This is working fine for me with test.sh/testusb

cheers

 drivers/usb/dwc3/ep0.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 2ef3c8d6a9db..69e769c35cf5 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -727,6 +727,10 @@ static int dwc3_ep0_std_request(struct dwc3 *dwc, struct 
usb_ctrlrequest *ctrl)
dwc3_trace(trace_dwc3_ep0, "USB_REQ_SET_ISOCH_DELAY");
ret = dwc3_ep0_set_isoch_delay(dwc, ctrl);
break;
+   case USB_REQ_SET_INTERFACE:
+   dwc3_trace(trace_dwc3_ep0, "USB_REQ_SET_INTERFACE");
+   dwc->start_config_issued = false;
+   /* Fall through */
default:
dwc3_trace(trace_dwc3_ep0, "Forwarding to gadget driver");
ret = dwc3_ep0_delegate_req(dwc, ctrl);
-- 
2.4.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] usb: dwc3: Reset the transfer resource index on SET_INTERFACE

[Felipe Balbi]

From: John Youn 

This fixes an issue introduced in commit b23c843992b6 (usb: dwc3:
gadget: fix DEPSTARTCFG for non-EP0 EPs) that made sure we would
only use DEPSTARTCFG once per SetConfig.

The trick is that we should use one DEPSTARTCFG per SetConfig *OR*
SetInterface. SetInterface was completely missed from the original
patch.

This problem became aparent after commit 76e838c9f776 (usb: dwc3:
gadget: return error if command sent to DEPCMD register fails)
added checking of the return status of device endpoint commands.

'Set Endpoint Transfer Resource' command was caught failing
occasionally. This is because the Transfer Resource
Index was not getting reset during a SET_INTERFACE request.

Finally, to fix the issue, was we have to do is make sure that
our start_config_issued flag gets reset whenever we receive a
SetInterface request.

To verify the problem (and its fix), all we have to do is run
test 9 from testusb with 'testusb -t 9 -s 2048 -a -c 5000'.

Fixes: b23c843992b6 (usb: dwc3: gadget: fix DEPSTARTCFG for non-EP0 EPs)
Cc:  # v3.2+
Signed-off-by: John Youn 
Signed-off-by: Felipe Balbi 
---

I have improved commit log and blamed correct commit. Also added stable Cc so
we have a chance of fixing older kernels.

If you can test on your end, I'd be glad to add Tested-bys, but do so by
tomorrow because it's extra important that we get this patch in.

This is working fine for me with test.sh/testusb

cheers

 drivers/usb/dwc3/ep0.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 2ef3c8d6a9db..69e769c35cf5 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -727,6 +727,10 @@ static int dwc3_ep0_std_request(struct dwc3 *dwc, struct 
usb_ctrlrequest *ctrl)
dwc3_trace(trace_dwc3_ep0, "USB_REQ_SET_ISOCH_DELAY");
ret = dwc3_ep0_set_isoch_delay(dwc, ctrl);
break;
+   case USB_REQ_SET_INTERFACE:
+   dwc3_trace(trace_dwc3_ep0, "USB_REQ_SET_INTERFACE");
+   dwc->start_config_issued = false;
+   /* Fall through */
default:
dwc3_trace(trace_dwc3_ep0, "Forwarding to gadget driver");
ret = dwc3_ep0_delegate_req(dwc, ctrl);
-- 
2.4.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: xhci_hcd 0000:00:14.0: ERROR Transfer event TRB DMA ptr not part of current TD ep_index 2 comp_code 1

[Mathias Nyman]

On 20.07.2015 23:13, Arkadiusz Miskiewicz wrote:
> On Saturday 18 of July 2015, Arkadiusz Miskiewicz wrote:
>> Hi.
>>
>> I'm on 4.2.0-rc2-00077-gf760b87 kernel and while trying to copy some file
>> from usb storage (sata disk behind sata-usb bridge or pendrive; hapens in
>> both cases) copying process hangs just early after start with:
> 
> Looks like suspend & resume is enough. Reloading bluetooth firmware done by 
> kernel
> triggers problem:
> 
> [  106.302783] rtc_cmos 00:02: System wakeup disabled by ACPI
> [  106.313280] PM: resume of devices complete after 3003.032 msecs
> [  106.314079] Restarting tasks ... done.
> [  106.326434] Bluetooth: hci0: read Intel version: 370710018002030d00
> [  106.330422] Bluetooth: hci0: Intel Bluetooth firmware file: 
> intel/ibt-hw-37.7.10-fw-1.80.2.3.d.bseq
> [  106.398223] xhci_hcd :00:14.0: ERROR Transfer event TRB DMA ptr not 
> part of current TD ep_index 0 comp_code 1

Looks like we get an event for a really old transfer for some reason, it should 
probably be handled already.

I got a patch that adds more paranoid checks for TRB cancel, which has been one 
major reasons
for the "Transfer event TRB DMA ptr not part of current TD" Errors. It also 
adds some logging
to show what's went wrong. (patch attached, applies on 4.2-rc3) Can you see if 
it helps?

If it doesn't, then adding xhci debugging could give us some clue:
echo -n 'module xhci_hcd =p' > /sys/kernel/debug/dynamic_debug/control

You said 3.19.3 works fine, but 4.0.8 and 4.2-rc2 fail, any chance you could 
bisect it?

Thanks

-Mathias   
  

>From cd27574451792569dff002ab33148cbaf9d52faf Mon Sep 17 00:00:00 2001
From: Mathias Nyman 
Date: Tue, 25 Nov 2014 17:35:27 +0200
Subject: [PATCH] xhci: Don't touch URB TD memory if they are no longer on the
 endpoint ring

If a URB is cancelled we want to make sure the URB's TRBs still point
to memory on the endpoint ring. If the ring was already dropped then that
TRB may point to memory already in use by another ring, or freed.

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-ring.c | 33 ++---
 drivers/usb/host/xhci.c  | 29 -
 drivers/usb/host/xhci.h  |  1 +
 3 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 94416ff..1e46d4f 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -136,6 +136,25 @@ static void next_trb(struct xhci_hcd *xhci,
 	}
 }
 
+/* check if the TD is on the ring */
+bool xhci_td_on_ring(struct xhci_td *td, struct xhci_ring *ring)
+{
+	struct xhci_segment *seg;
+
+	if (!td->start_seg || !ring || !ring->first_seg)
+		return false;
+
+	seg = ring->first_seg;
+	do {
+		if (td->start_seg == seg)
+			return true;
+		seg = seg->next;
+	} while (seg != ring->first_seg);
+
+	return false;
+}
+
+
 /*
  * See Cycle bit rules. SW is the consumer for the event ring only.
  * Don't make a ring full of link TRBs.  That would be dumb and this would loop.
@@ -685,10 +704,16 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, int slot_id,
 	cur_td->urb->stream_id);
 			goto remove_finished_td;
 		}
-		/*
-		 * If we stopped on the TD we need to cancel, then we have to
-		 * move the xHC endpoint ring dequeue pointer past this TD.
+		/* In case ring was dropped and segments freed or cached we
+		 * don't want to touch that memory anymore, or, if we stopped
+		 * on the TD we want to remove we need to move the dq pointer
+		 * past this TD, otherwise just turn TD to no-op
 		 */
+		if (!xhci_td_on_ring(cur_td, ep_ring)) {
+			xhci_err(xhci, "Cancelled TD not on stopped ring\n");
+			goto remove_finished_td;
+		}
+
 		if (cur_td == ep->stopped_td)
 			xhci_find_new_dequeue_state(xhci, slot_id, ep_index,
 	cur_td->urb->stream_id,
@@ -1295,11 +1320,13 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
 	/* Is the command ring deq ptr out of sync with the deq seg ptr? */
 	if (cmd_dequeue_dma == 0) {
 		xhci->error_bitmask |= 1 << 4;
+		xhci_err(xhci, "Command completion ptr and seg out of sync\n");
 		return;
 	}
 	/* Does the DMA address match our internal dequeue pointer address? */
 	if (cmd_dma != (u64) cmd_dequeue_dma) {
 		xhci->error_bitmask |= 1 << 5;
+		xhci_err(xhci, "Command completion DMA address mismatch\n");
 		return;
 	}
 
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 7da0d60..d72b46e 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1527,6 +1527,7 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 	struct xhci_ring *ep_ring;
 	struct xhci_virt_ep *ep;
 	struct xhci_command *command;
+	bool remove_td_from_ring = false;
 
 	xhci = hcd_to_xhci(hcd);
 	spin_lock_irqsave(&xhci->lock, flags);
@@ -1587,9 +1588,28 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 	urb_priv->td[i]->start_seg,
 	urb_priv->td[i]->first_trb));
 
+	/* make sure the TDs o

Re: [PATCH v5 0/6] usb: interface authorization

[Greg KH]

On Tue, Jul 21, 2015 at 02:22:20PM +0200, Stefan Koch wrote:
> Am Montag, den 29.06.2015, 15:41 -0700 schrieb Greg KH:
> > On Mon, Jun 29, 2015 at 11:23:47PM +0200, Stefan Koch wrote:
> > > Am Donnerstag, den 18.06.2015, 13:30 -0400 schrieb Alan Stern:
> > > > On Thu, 18 Jun 2015, Stefan Koch wrote:
> > > > 
> > > > > This patch introduces an interface authorization for USB devices.
> > > > > The kernel supports a device authorization because of wireless USB.
> > > > > 
> > > > > But the new interface authorization allows to authorize or deauthorize
> > > > > individual interfaces instead authorization or deauthorize a whole 
> > > > > device.
> > > > > 
> > > > > Therefore the authorized attribute is introduced for each interface.
> > > > > 
> > > > > Each patch depends on all patches with a lesser number.
> > > > > 
> > > > > Stefan Koch (6):
> > > > >   usb: interface authorization: Declare authorized attribute
> > > > >   usb: interface authorization: Introduces the default interface
> > > > > authorization
> > > > >   usb: interface authorization: Control interface probing and claiming
> > > > >   usb: interface authorization: Introduces the USB interface
> > > > > authorization.
> > > > >   usb: interface authorization: SysFS part of USB interface
> > > > > authorization.
> > > > >   usb: interface authorization: Documentation part
> > > > > 
> > > > >  Documentation/ABI/testing/sysfs-bus-usb | 22 ++
> > > > >  Documentation/usb/authorization.txt | 34 +
> > > > >  drivers/usb/core/driver.c   | 11 +++
> > > > >  drivers/usb/core/hcd.c  | 52 
> > > > > +
> > > > >  drivers/usb/core/message.c  | 48 
> > > > > ++
> > > > >  drivers/usb/core/sysfs.c| 41 
> > > > > ++
> > > > >  drivers/usb/core/usb.h  |  2 ++
> > > > >  include/linux/usb.h |  1 +
> > > > >  include/linux/usb/hcd.h |  3 ++
> > > > >  9 files changed, 214 insertions(+)
> > > > 
> > > > Acked-by: Alan Stern 
> > > > 
> > > > Other people may still have some comments.
> > > > 
> > > > Alan Stern
> > > > 
> > > Hi
> > > 
> > > is there any current merging state?
> > > 
> > > In what git tree the patch will submitted at the first step?
> > 
> > I can't do anything at the moment due to the merge window.  I'll review
> > it after 4.2-rc1 is out (usually a week or so afterward, as my queue is
> > quite large).  Don't worry, it's not lost.
> > 
> > greg k-h
> 
> Are there any news about the merge state?

It's in my to-review queue, along with all other USB patches, which I
hope to get to soon.

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 v3 07/46] usb: dwc3: gadget: add ep capabilities support

[Felipe Balbi]

Hi,

On Wed, Jul 15, 2015 at 08:31:54AM +0200, Robert Baldyga wrote:
> Convert endpoint configuration to new capabilities model.
> 
> Signed-off-by: Robert Baldyga 
> ---
>  drivers/usb/dwc3/gadget.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 333a7c0..8d1f768 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1713,6 +1713,19 @@ static int dwc3_gadget_init_hw_endpoints(struct dwc3 
> *dwc,
>   return ret;
>   }
>  
> + if (epnum == 0) {
> + dep->endpoint.caps.type_control = true;

as the name says, this function deals with hw endpoints. This means ep1
is also control.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH] Fix data loss in cdc-acm

[Peter Hurley]

Hi Sven,

On 07/21/2015 05:18 AM, Johan Hovold wrote:
> On Mon, Jul 20, 2015 at 08:07:33PM +0200, Sven Brauch wrote:
>> On 20/07/15 19:25, Johan Hovold wrote:
> 
>>> The idea of adding another layer of buffering in the cdc-acm driver has
>>> been suggested in the past but was rejected (or at least questioned).
>>> See for example this thread:
>>>
>>> https://lkml.kernel.org/r/20110608164626.22bc8...@lxorguk.ukuu.org.uk
>> Yes, that is indeed pretty much the same problem and the same solution.
>> Answering to the questions brought up in that thread:
>>
>>> a) Why is your setup filling 64K in the time it takes the throttle
>>> response to occur
>> As far as I understand, the throttle happens only when there's less than
>> 128 bytes of free space in the tty buffer. Data can already be lost
>> before the tty even decides it should start throttling, simply because
>> the throttle threshold is smaller than the amount of data potentially in
>> each urb. Also (excuse my cluelessness) it seems that when exactly the
>> throttling happens depends on some scheduling "jitter" as well.
>> Additionally, the response of the cdc_acm driver to a throttle request
>> is not very prompt; it might have a queue of up to 16kB (16 urbs) pending.
> 
> Not really. The n_tty ldisc throttles when *its* buffer space is low,
> but there should still be plenty of room in the tty buffers.
> 
> Looks like the ldisc processing is being starved.

Just to expand on Johan's explanation somewhat.

The tty buffers are separate from the fixed 4KB n_tty line discipline
read buffer. The 4KB read buffer holds data which has already been
processed as input based on the current termios settings.

The tty buffers hold unprocessed rx data; because the cdc-acm driver
does not use per-char flags, the default tty buffer limit amounts to
a 128KB buffer, which is roughly 10ms at your line rate of 96Mb/s.

10ms is a long time for a kworker thread (which would indicate throttle)
to not be running once scheduled, so I doubt scheduler latency is
actually the problem.

More likely suspects are:
1. Because most drivers don't properly handle concurrent throttle/unthrottle,
   the n_tty line discipline excludes one while the other is in-progress.
   acm_tty_unthrottle() first clears throttle state, thus letting input
   proceed, and then continues to submit previously in-flight urbs. During this
   time, the driver cannot receive throttle notification (ie., 
acm_tty_throttle()
   will not be called).

2. The reader/consumer can't process at that line rate, or related, the
   reader/consumer can't catch up once it has fallen behind.

Some interesting experiments would be:
1. Instrument acm_read_bulk_callback with tty_buffer_space_avail() to track
   the (approx) fill level of the tty buffers. I would use ftrace/trace_printk()
   to record this.
2. Similarly instrument acm_tty_throttle/acm_tty_unthrottle.


>>> b) Do we care (is the right thing to do to lose bits anyway at
>>> that point)
>> This I cannot answer, I don't know enough about the architecture or
>> standards. I can just say that for my case, there's a lot of losses;
>> this it not an issue which happens after hours when the system is under
>> heavy load, it happens after just a few seconds reproducably.
> 
> In general if data isn't being consumed fast enough we eventually need
> to drop it (unless we can throttle the producer).
> 
> But this isn't necessarily the case in your setup.
> 
>>> The tty buffers are quite large these days, but could possibly be bumped
>>> further if needed to give the ldisc some more time to throttle the
>>> device at very high speeds.
>> I do not like this solution. It will again be based on luck, and you
>> will still be unable to rely on the delivery guarantee made by the USB
>> stack (at least when using bulk).
> 
> See above.

A driver can set the upper limit of tty buffers on a per-tty basis with
tty_buffer_set_limit(). The default limit is 64KB. This limit must be
set before the tty can receive input.


>> My suggestion instead stops the host system from accepting any more data
>> from the device when its buffers are full, forcing the device to wait
>> before sending out more data (which many kinds of devices might very
>> well be able to do).
> 
> This is what we are supposed to do today, but by relying on the ldisc
> and tty buffers rather than forcing every driver to implement another
> custom throttling mechanism.
> 
> But something obviously isn't working as intended.

Let me know if you need help instrumenting the tty buffers/throttling
to help figure out what the actual problem is.

Regarding the patch itself, I have no opinion on the suitability of
simply not resubmitting urbs. However, that is exactly how the throttle
mechanism works, and the tty buffer API is specifically designed to
allow drivers to manage flow via that interface as well (especially
for high-throughput drivers).

The n_tty throttle/unthrottle is a very indirect method for restricting
flow,

Re: [PATCH 1/1] usb: chipidea: ehci_init_driver is intended to call one time

[Alan Stern]

On Tue, 21 Jul 2015, Peter Chen wrote:

> The ehci_init_driver is used to initialize hcd APIs for each
> ehci controller driver, it is designed to be called only one time
> and before driver register is called. The current design will
> cause ehci_init_driver is called multiple times at probe process,
> it will cause hc_driver's initialization affect current running hcd.
> 
> We run out NULL pointer dereference problem when one hcd is started
> by module_init, and the other is started by otg thread at SMP platform.
> The reason for this problem is ehci_init_driver will do memory copy
> for current uniform hc_driver, and this memory copy will do memset (as 0)
> first, so when the first hcd is running usb_add_hcd, and the second
> hcd may clear the uniform hc_driver's space (at ehci_init_driver),
> then the first hcd will meet NULL pointer at the same time.

> Cc: Jun Li 
> Cc: 
> Cc: Alan Stern 
> Signed-off-by: Peter Chen 

Acked-by: 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 v4 5/5] usb: gadget: mass_storage: Warn if LUNs ids are not contiguous

[Felipe Balbi]

On Mon, Jul 20, 2015 at 09:40:48PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 07/20/2015 09:15 PM, Krzysztof Opasiak wrote:
> 
> >According to mass storage specification:
> 
> >"Logical Unit Numbers on the device shall be numbered contiguously
> >  starting from LUN 0 to a maximum LUN of 15 (Fh)"
> 
> >So let's at least print a warning message that LUNs ids should be
> >contiguous.
> 
> >Signed-off-by: Krzysztof Opasiak 
> >Acked-by: Michal Nazarewicz 
> >---
> >  drivers/usb/gadget/function/f_mass_storage.c |7 +++
> >  1 file changed, 7 insertions(+)
> 
> >diff --git a/drivers/usb/gadget/function/f_mass_storage.c 
> >b/drivers/usb/gadget/function/f_mass_storage.c
> >index 5fcd9a0..69167ad 100644
> >--- a/drivers/usb/gadget/function/f_mass_storage.c
> >+++ b/drivers/usb/gadget/function/f_mass_storage.c
> >@@ -3042,6 +3042,13 @@ static int fsg_bind(struct usb_configuration *c, 
> >struct usb_function *f)
> > return -EINVAL;
> > }
> >
> >+for (i = 0; i < ARRAY_SIZE(common->luns); ++i)
> >+if (!common->luns[i])
> >+break;
> >+
> >+if (ret != i - 1)
> >+pr_err("LUN ids should be contiguous.\n");
> 
>I suggest just "LUNs" instead of "LUN ids" which sounds somewhat strange
> (logical unit number ids, hm?).

fixed while applying, thanks

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v4 4/5] usb: gadget: mass_storage: Use static array for luns

[Felipe Balbi]

On Mon, Jul 20, 2015 at 08:15:20PM +0200, Krzysztof Opasiak wrote:
> This patch replace dynamicly allocated luns array with static one.
> This simplifies the code of mass storage function and modules.
> 
> Signed-off-by: Krzysztof Opasiak 
> Acked-by: Michal Nazarewicz 

this breaks compilation of g_nokia. Can you rebase on my testing/next
and fix it up ? Thanks

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH] usb: phy: msm: Add D+/D- lines route control

[Felipe Balbi]

On Wed, Jul 08, 2015 at 12:45:54PM +0300, Ivan T. Ivanov wrote:
> apq8016-sbc board is using Dual SPDT USB Switch (TC7USB40MU),
> witch is controlled by GPIO to de/multiplex D+/D- USB lines to
> USB2513B Hub and uB connector. Add support for this.
> 
> Signed-off-by: Ivan T. Ivanov 

doesn't apply:

checking file Documentation/devicetree/bindings/usb/msm-hsusb.txt
checking file drivers/usb/phy/phy-msm-usb.c
Hunk #5 succeeded at 1632 (offset 2 lines).
Hunk #6 succeeded at 1809 (offset 2 lines).
Hunk #7 FAILED at 1844.
1 out of 7 hunks FAILED
checking file include/linux/usb/msm_hsusb.h

Please rebase on my testing/next

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH] Fix data loss in cdc-acm

[Sven Brauch]

Hi,

Thank you for your comments.

On 21/07/15 15:43, Oliver Neukum wrote:
> But others won't and we'd preserve stale data in preference over fresh
> data.
If that is important for your device, you should be using an isochronous
endpoint, not bulk, no?
Also note that the driver currently does this anyways. It loses a few kB
of data, and _then_ it throttles the producer and forces it to wait.

On 21/07/15 11:18, Johan Hovold wrote:
> In general if data isn't being consumed fast enough we eventually need
> to drop it (unless we can throttle the producer).
Yes, maybe this is the first thing which should be cleared up: Is
"throttle the producer" always preferable over "lose data"? I'd say yes
for bulk transfers, no for isochronous. It is in principle easy enough
to throttle the producer, that is what e.g. my patch does. Whether a
different approach may be more appropriate than the "don't resubmit the
urbs" thing is then of course open to debate.

As far as I can see, throttling the producer is the only way to
guarantee data delivery. So if we want that (and I certainly want it for
my application, I don't know about the general case), I think all
changes to the tty buffers or throttling mechanisms are "just"
performance optimization, since no such modification will ever guarantee
delivery if the producer is not throttled in time.
And, this I want to mention again, if your producer is timing-sensitive
you would not be using bulk anyways. The USB controller could just
decide that your device cannot send data for the next five seconds, and
it will have to handle that case as well. Thus I see no reason to not
throttle the producer if necessary.

On 21/07/15 18:45, Peter Hurley wrote:
> 1. Instrument acm_read_bulk_callback with tty_buffer_space_avail()
> to track the (approx) fill level of the tty buffers. I would
> use ftrace/trace_printk() to record this.
I already did this while debugging. For a while, the buffer is almost
empty (fluctuates by a few kB), then it rapidly drops to zero bytes
free. Only after a few urbs where submitted (or rather, not submitted)
into the full buffer, the throttle flag gets set.

Thank you for your offer to help figuring out what exactly goes wrong
with the throttling mechanism. If it turns out we should actually look
for a fix there, I'll be happy to make use of it.

Best,
Sven



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] Fix data loss in cdc-acm

[Peter Hurley]

On 07/21/2015 05:43 PM, Sven Brauch wrote:
> Hi,
> 
> Thank you for your comments.
> 
> On 21/07/15 15:43, Oliver Neukum wrote:
>> But others won't and we'd preserve stale data in preference over fresh
>> data.
> If that is important for your device, you should be using an isochronous
> endpoint, not bulk, no?
> Also note that the driver currently does this anyways. It loses a few kB
> of data, and _then_ it throttles the producer and forces it to wait.
> 
> On 21/07/15 11:18, Johan Hovold wrote:
>> In general if data isn't being consumed fast enough we eventually need
>> to drop it (unless we can throttle the producer).
> Yes, maybe this is the first thing which should be cleared up: Is
> "throttle the producer" always preferable over "lose data"? I'd say yes
> for bulk transfers, no for isochronous. It is in principle easy enough
> to throttle the producer, that is what e.g. my patch does. Whether a
> different approach may be more appropriate than the "don't resubmit the
> urbs" thing is then of course open to debate.
> 
> As far as I can see, throttling the producer is the only way to
> guarantee data delivery. So if we want that (and I certainly want it for
> my application, I don't know about the general case), I think all
> changes to the tty buffers or throttling mechanisms are "just"
> performance optimization, since no such modification will ever guarantee
> delivery if the producer is not throttled in time.
> And, this I want to mention again, if your producer is timing-sensitive
> you would not be using bulk anyways. The USB controller could just
> decide that your device cannot send data for the next five seconds, and
> it will have to handle that case as well. Thus I see no reason to not
> throttle the producer if necessary.

It's unclear to me that you haven't hit some other bug (buffer miscalc,
failure to progress, etc.) which is affecting other users but just not
to the extent you're experiencing.

For example, I made changes to the conditions required to restart the
input worker; I may have omitted some necessary condition which you've
triggered.


> On 21/07/15 18:45, Peter Hurley wrote:
>> 1. Instrument acm_read_bulk_callback with tty_buffer_space_avail()
>> to track the (approx) fill level of the tty buffers. I would
>> use ftrace/trace_printk() to record this.
> I already did this while debugging. For a while, the buffer is almost
> empty (fluctuates by a few kB), then it rapidly drops to zero bytes
> free. Only after a few urbs where submitted (or rather, not submitted)
> into the full buffer, the throttle flag gets set.

I'd like to see that data, if you can, which will help me understand
at least the timing.

Regards,
Peter Hurley

--
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] Fix data loss in cdc-acm

[Sven Brauch]

Hey,

On 22/07/15 01:34, Peter Hurley wrote:
> I'd like to see that data, if you can, which will help me understand
> at least the timing.
Sure, please see below for the code which produced the output
and the actual output. Let me know if you need anything else.
This was run with the unmodified version of the driver, i.e. without
my patch.

Sven
--

Code which produced the output:

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 5c8f581..3d26c87 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -404,12 +404,16 @@ static int acm_submit_read_urbs(struct acm *acm, gfp_t 
mem_flags)
 
 static void acm_process_read_urb(struct acm *acm, struct urb *urb)
 {
+int written;
+
if (!urb->actual_length)
return;
 
-   tty_insert_flip_string(&acm->port, urb->transfer_buffer,
+   written = tty_insert_flip_string(&acm->port, urb->transfer_buffer,
urb->actual_length);
tty_flip_buffer_push(&acm->port);
+printk("cdc-acm: acm_process_read_urb: processed %d of %d (free space 
left: %d)\n",
+   written, urb->actual_length, tty_buffer_space_avail(&acm->port));
 }
 
 static void acm_read_bulk_callback(struct urb *urb)
@@ -448,6 +452,9 @@ static void acm_read_bulk_callback(struct urb *urb)
/* throttle device if requested by tty */
spin_lock_irqsave(&acm->read_lock, flags);
acm->throttled = acm->throttle_req;
+if (acm->throttle_req) {
+printk("cdc-acm: acm_read_bulk_callback: throttle requested\n");
+}
if (!acm->throttled) {
spin_unlock_irqrestore(&acm->read_lock, flags);
acm_submit_read_urb(acm, rb->index, GFP_ATOMIC);
@@ -758,6 +765,9 @@ static void acm_tty_throttle(struct tty_struct *tty)
 {
struct acm *acm = tty->driver_data;
 
+printk("cdc-acm: acm_tty_throttle: called; free space in tty: %d\n",
+   tty_buffer_space_avail(&acm->port));
+
spin_lock_irq(&acm->read_lock);
acm->throttle_req = 1;
spin_unlock_irq(&acm->read_lock);
@@ -768,6 +778,9 @@ static void acm_tty_unthrottle(struct tty_struct *tty)
struct acm *acm = tty->driver_data;
unsigned int was_throttled;
 
+printk("cdc-acm: acm_tty_unthrottle: called; free space in tty: %d\n",
+   tty_buffer_space_avail(&acm->port));
+
spin_lock_irq(&acm->read_lock);
was_throttled = acm->throttled;
acm->throttled = 0;

__
Output from dmesg (cut a bit in the beginning):

[...]
[62517.602478] cdc-acm: acm_process_read_urb: processed 1024 of 1024 (free 
space left: 63232)
[62517.602480] cdc-acm: acm_process_read_urb: processed 1024 of 1024 (free 
space left: 63232)
[62517.602483] cdc-acm: acm_process_read_urb: processed 1024 of 1024 (free 
space left: 62208)
[62517.602485] cdc-acm: acm_process_read_urb: processed 1024 of 1024 (free 
space left: 62208)
[62517.602487] cdc-acm: acm_process_read_urb: processed 1024 of 1024 (free 
space left: 61184)
[62517.602489] cdc-acm: acm_process_read_urb: processed 1024 of 1024 (free 
space left: 61184)
[62517.602494] cdc-acm: acm_tty_throttle: called; free space in tty: 63488
[62517.602600] cdc-acm: acm_process_read_urb: processed 1024 of 1024 (free 
space left: 62464)
[62517.602602] cdc-acm: acm_read_bulk_callback: throttle requested
[62517.602603] cdc-acm: acm_process_read_urb: processed 1024 of 1024 (free 
space left: 62464)
[62517.602604] cdc-acm: acm_read_bulk_callback: throttle requested
[62517.602606] cdc-acm: acm_process_read_urb: processed 1024 of 1024 (free 
space left: 61440)
[62517.602607] cdc-acm: acm_read_bulk_callback: throttle requested
[62517.602609] cdc-acm: acm_process_read_urb: processed 1024 of 1024 (free 
space left: 61440)
[62517.602610] cdc-acm: acm_read_bulk_callback: throttle requested
[62517.602612] cdc-acm: acm_process_read_urb: processed 1024 of 1024 (free 
space left: 60416)
[62517.602613] cdc-acm: acm_read_bulk_callback: throttle requested
[62517.602726] cdc-acm: acm_process_read_urb: processed 1024 of 1024 (free 
space left: 63488)
[62517.602727] cdc-acm: acm_read_bulk_callback: throttle requested
[62517.602729] cdc-acm: acm_process_read_urb: processed 1024 of 1024 (free 
space left: 62464)
[62517.602730] cdc-acm: acm_read_bulk_callback: throttle requested
[62517.602731] cdc-acm: acm_process_read_urb: processed 1024 of 1024 (free 
space left: 62464)
[62517.602732] cdc-acm: acm_read_bulk_callback: throttle requested
[62517.602734] cdc-acm: acm_process_read_urb: processed 1024 of 1024 (free 
space left: 61440)
[62517.602735] cdc-acm: acm_read_bulk_callback: throttle requested
[62517.602737] cdc-acm: acm_process_read_urb: processed 1024 of 1024 (free 
space left: 61440)
[62517.602738] cdc-acm: acm_read_bulk_callback: throttle requested
[62517.602959] cdc-acm: acm_tty_unthrottle: called; free space in tty: 64512
[62517.603852] cdc-acm: acm_process_read_urb: processed 16 of 16 (free space