Re: [PATCH RESEND] tty: don't dead lock while flushing workqueue
On Tue, 2012-11-27 at 19:01 +0100, Sebastian Andrzej Siewior wrote:
> Since commit 89c8d91e31f2 ("tty: localise the lock") I see a dead lock
> in one of my dummy_hcd + g_nokia test cases. The first run one was usually
> okay, the second often resulted in a splat by lockdep and the third was
> usually a dead lock.
>
> Before the path mentioned tty_ldisc_release() look like this:
>
> | tty_ldisc_halt(tty);
> | tty_ldisc_flush_works(tty);
> | tty_lock();
>
> As it can be seen, it first flushes the workqueue and then grabs the
> tty_lock. Now we grab the lock first:
>
> | tty_lock_pair(tty, o_tty);
> | tty_ldisc_halt(tty);
> | tty_ldisc_flush_works(tty);
>
> so lockdep's complaint seems valid.
>
> The other user of tty_ldisc_flush_works() is tty_set_ldisc() and I tried
> to mimnic its logic:
The lock logic for tty_set_ldisc() is wrong. Despite existing code in
tty_set_ldisc() and tty_ldisc_hangup(), the ldisc_mutex does **not**
(and should not) play a role in acquiring or releasing ldisc references.
The only thing that needs to happen here is below (don't actually use
below because I just hand-edited it):
> See http://lkml.org/lkml/2012/11/21/347
>
> drivers/tty/tty_ldisc.c | 13 +
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
> index 0f2a2c5..fb76818 100644
> --- a/drivers/tty/tty_ldisc.c
> +++ b/drivers/tty/tty_ldisc.c
> @@ -930,16 +930,21 @@ void tty_ldisc_release(struct tty_struct *tty, struct
> tty_struct *o_tty)
>*/
>
> - tty_lock_pair(tty, o_tty);
> tty_ldisc_halt(tty);
> tty_ldisc_flush_works(tty);
> + tty_lock_pair(tty, o_tty);
> /* This will need doing differently if we need to lock */
> tty_ldisc_kill(tty);
> -
> if (o_tty)
> tty_ldisc_kill(o_tty);
>
--
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 3/6] TTY: fix DTR being raised on hang up
On Thu, 2013-03-07 at 15:55 +0100, Johan Hovold wrote:
> Make sure to check ASYNC_INITIALISED before raising DTR when waking up
> from blocked open in tty_port_block_til_ready.
>
> Currently DTR could get raised at hang up as a blocked process would
> raise DTR unconditionally before checking for hang up and returning.
>
> Signed-off-by: Johan Hovold
> ---
> drivers/tty/tty_port.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> index 3de5918..52f1066 100644
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -355,7 +355,7 @@ int tty_port_block_til_ready(struct tty_port *port,
>
> while (1) {
> /* Indicate we are open */
> - if (tty->termios.c_cflag & CBAUD)
> + if (C_BAUD(tty) && test_bit(ASYNCB_INITIALIZED, &port->flags))
> tty_port_raise_dtr_rts(port);
>
> prepare_to_wait(&port->open_wait, &wait, TASK_INTERRUPTIBLE);
This is ok, but there are 6 other *_block_til_ready() functions:
This one doesn't use DTR/RTS so can be ignored:
./drivers/staging/sb105x/sb_pci_mp.c:static int mp_block_til_ready(struct file
*filp, struct sb_uart_state *state)
This one is so scary you should probably leave it alone:
./drivers/tty/serial/crisv10.c:block_til_ready(struct tty_struct *tty, struct
file * filp,
These will need the same change (although be careful: some use
ASYNC_INITIALIZED rather than ASYNCB_INITIALIZED).
./drivers/tty/synclinkmp.c:static int block_til_ready(struct tty_struct *tty,
struct file *filp,
./drivers/tty/synclink_gt.c:static int block_til_ready(struct tty_struct *tty,
struct file *filp,
./drivers/tty/synclink.c:static int block_til_ready(struct tty_struct *tty,
struct file * filp,
./net/irda/ircomm/ircomm_tty.c:static int ircomm_tty_block_til_ready(struct
ircomm_tty_cb *self,
Please be sure to Cc: David Miller on changes to
net/irda.
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 v3 0/6] TTY: port hangup and close fixes
On Thu, 2013-03-07 at 15:55 +0100, Johan Hovold wrote: > These patches against 3.9-rc1 fix a few issues with tty-port hangup and > close. > > The first and fifth patch are essentially clean ups. > > The second and third patch fix the fact that DTR could get raised > (rather than dropped) at hangup if there are blocked opens. [ Note that > the second patch has been separated into its own patch and that the > third patch is new in v3 of this series. ] > > The fourth patch makes sure DTR is dropped also on hangup and that DTR > is only dropped for initialised ports (where it could have been raised > in the first place). > > The sixth and final patch, makes sure no tty callbacks are made from > tty_port_close_start when the port has not been initialised (successfully > opened). This was previously only done for wait_until_sent but there's > no reason to call flush_buffer or to honour port drain delay either. > The latter could cause a failed open to stall for up to two seconds. > > As a side-effect, these patches also fix an issue in USB-serial where we > could get tty-port callbacks on an uninitialised port after having hung > up and unregistered a device after disconnect. > > Johan > > > v3: > - amend series with fix of DTR sometimes being raised on hang-up > - do not lower DTR on hangup if port tty is gone > - make sure tty in call to shutdown is refcounted > - use cflag-macros throughout Other than the comments for patch 3/6, this series looks good. Thanks again for your work on this. Please cc: me on the USB serial core changes as well, if you don't mind. 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: [3.9-rc1] irq 16: nobody cared (was [3.9-rc1] very poor interrupt responses)
On Thu, 2013-03-14 at 17:09 +0100, Jiri Kosina wrote: > On Thu, 14 Mar 2013, Jiri Kosina wrote: > > > > > I don't think I have seen this message on rc1+ (8343bce, to be > > > > precise), > > > > but I have definitely seen sluggish system response on that kernel as > > > > well. > > > > > > > > Attaching lspci, /proc/interrupts and dmesg. > > > > > > Can you try to do a git bisect for this? Is the sluggish system > > > response clear enough that you can tell reliably when it is present and > > > when it isn't? > > > > That was my first thought, but unfortunately I am afraid there will be > > point at which I will easily make a bisection mistake, as the > > responsiveness of the system varies over time, so it's not really a > > 100% objective measure. > > So I will try a bisect, but it'll take some time so that I could claim it > to be trustworthy. > > Therefore in case anyone has any idea in parallel, I am all ears. When I had this happen on -next, it was PCI + ACPI-related and I had to bisect it. But for me the problem was quite noticable and showed up right at login prompt. Regards, Peter Hurley PS - I already confirmed that the commit that fixes that is in 3.9-rc1 -- 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: [3.9-rc1] irq 16: nobody cared (was [3.9-rc1] very poor interrupt responses)
On Thu, 2013-03-14 at 17:46 +0100, Rafael J. Wysocki wrote: > On Thursday, March 14, 2013 05:09:59 PM Jiri Kosina wrote: > > On Thu, 14 Mar 2013, Jiri Kosina wrote: > > > > > > > I don't think I have seen this message on rc1+ (8343bce, to be > > > > > precise), > > > > > but I have definitely seen sluggish system response on that kernel as > > > > > well. > > > > > > > > > > Attaching lspci, /proc/interrupts and dmesg. > > > > > > > > Can you try to do a git bisect for this? Is the sluggish system > > > > response clear enough that you can tell reliably when it is present and > > > > when it isn't? > > > > > > That was my first thought, but unfortunately I am afraid there will be > > > point at which I will easily make a bisection mistake, as the > > > responsiveness of the system varies over time, so it's not really a > > > 100% objective measure. > > > > So I will try a bisect, but it'll take some time so that I could claim it > > to be trustworthy. > > > > Therefore in case anyone has any idea in parallel, I am all ears. > > This one is a candidate to focus on I think: > > commit 181380b702eee1a9aca51354d7b87c7b08541fcf > Author: Yinghai Lu > Date: Sat Feb 16 11:58:34 2013 -0700 > > PCI/ACPI: Don't cache _PRT, and don't associate them with bus numbers This patch __fixed__ this problem for me in linux-next back in February. Rafael, did you hold back some ACPI patches from 3.9 that would have made fix no longer applicable? -- 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: [3.9-rc1] irq 16: nobody cared (was [3.9-rc1] very poor interrupt responses)
On Thu, 2013-03-14 at 18:22 +0100, Rafael J. Wysocki wrote:
> On Thursday, March 14, 2013 01:06:04 PM Peter Hurley wrote:
> > On Thu, 2013-03-14 at 17:46 +0100, Rafael J. Wysocki wrote:
> > > On Thursday, March 14, 2013 05:09:59 PM Jiri Kosina wrote:
> > > > On Thu, 14 Mar 2013, Jiri Kosina wrote:
> > > >
> > > > > > > I don't think I have seen this message on rc1+ (8343bce, to be
> > > > > > > precise),
> > > > > > > but I have definitely seen sluggish system response on that
> > > > > > > kernel as
> > > > > > > well.
> > > > > > >
> > > > > > > Attaching lspci, /proc/interrupts and dmesg.
> > > > > >
> > > > > > Can you try to do a git bisect for this? Is the sluggish system
> > > > > > response clear enough that you can tell reliably when it is present
> > > > > > and
> > > > > > when it isn't?
> > > > >
> > > > > That was my first thought, but unfortunately I am afraid there will
> > > > > be
> > > > > point at which I will easily make a bisection mistake, as the
> > > > > responsiveness of the system varies over time, so it's not really a
> > > > > 100% objective measure.
> > > >
> > > > So I will try a bisect, but it'll take some time so that I could claim
> > > > it
> > > > to be trustworthy.
> > > >
> > > > Therefore in case anyone has any idea in parallel, I am all ears.
> > >
> > > This one is a candidate to focus on I think:
> > >
> > > commit 181380b702eee1a9aca51354d7b87c7b08541fcf
> > > Author: Yinghai Lu
> > > Date: Sat Feb 16 11:58:34 2013 -0700
> > >
> > > PCI/ACPI: Don't cache _PRT, and don't associate them with bus numbers
> >
> > This patch __fixed__ this problem for me in linux-next back in February.
> >
> > Rafael, did you hold back some ACPI patches from 3.9 that would have
> > made fix no longer applicable?
>
> No, I didn't.
>
> I'm afraid, though, that the fix might not be effective on some systems for a
> reason that's unclear at the moment.
>
> So in fact the one to check is commit 4f535093cf ("PCI: Put pci_dev in device
> tree as early as possible") and if the problem doesn't appear before that, we
> need to figure out why the fix may not be sufficient.
I agree.
Commit 4f535093cf ("PCI: Put pci_dev in device tree as early as
possible") is the likely culprit, and "Don't cache _PRT..." is probably
an insufficient fix.
Not so sure about the other reporters though because they had active
devices on those USB ports.
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 v2 33/85] USB: serial: clean up usb-serial bus device removal
On Thu, 2013-03-14 at 16:23 +0100, Johan Hovold wrote: > Make sure to unregister the tty-device before calling subdriver > port_remove. > > This way remove will reverse probe, and specifically any port data > released in port_remove will be available throughout tty unregister. > > Note that the order currently does not matter as the tty-layer can make > callbacks also after the device has been unregistered. This is > handled in usb-serial core using the disconnected flag, which is > already set when usb-serial bus device remove is called. > > Cc: Peter Hurley > Reported-by: Peter Hurley > Signed-off-by: Johan Hovold Looks fine to me. The thing I did notice is that the ftdi usb serial mini-driver has its own ioctl() handler for TIOCMIWAIT which can be holding the tty reference open after the unregister. Its port_remove() method, ftdi_sio_port_remove(), does the wake up to kick it off the loop. Now that the call order is reversed, that will need to be fixed. A quick review of the usage seems like the whole ioctl() should be torn out along with the private delta_msr_wait. 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 v3 3/6] TTY: fix DTR being raised on hang up
On Fri, 2013-03-15 at 10:24 +0100, Johan Hovold wrote:
> On Wed, Mar 13, 2013 at 03:43:43PM -0400, Peter Hurley wrote:
> > On Thu, 2013-03-07 at 15:55 +0100, Johan Hovold wrote:
> > > Make sure to check ASYNC_INITIALISED before raising DTR when waking up
> > > from blocked open in tty_port_block_til_ready.
> > >
> > > Currently DTR could get raised at hang up as a blocked process would
> > > raise DTR unconditionally before checking for hang up and returning.
> > >
> > > Signed-off-by: Johan Hovold
> > > ---
> > > drivers/tty/tty_port.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> > > index 3de5918..52f1066 100644
> > > --- a/drivers/tty/tty_port.c
> > > +++ b/drivers/tty/tty_port.c
> > > @@ -355,7 +355,7 @@ int tty_port_block_til_ready(struct tty_port *port,
> > >
> > > while (1) {
> > > /* Indicate we are open */
> > > - if (tty->termios.c_cflag & CBAUD)
> > > + if (C_BAUD(tty) && test_bit(ASYNCB_INITIALIZED, &port->flags))
> > > tty_port_raise_dtr_rts(port);
> > >
> > > prepare_to_wait(&port->open_wait, &wait, TASK_INTERRUPTIBLE);
> >
> > This is ok, but there are 6 other *_block_til_ready() functions:
^^
Comment on patch
> Yes, but that's not really a comment on this patch, is it?
>
> The purpose of this series is to fix the tty-port implementation, and
> I've only touched individual drivers when I had to in order not to break
> anything due to changed assumptions.
>
> There's a ton of buggy and odd behaviour to be found once you start
> turning the stones. Drivers like the ones below really ought to be
> using tty ports and it's helpers.
Sure, I understand.
OTOH, tty_port and these drivers stem from the same ancestor and it's
partly because of localized bug fixes like these that the drivers have
buggy and odd behavior (because tty_port gets fixed and these do not).
As you can verify from the changelogs of these drivers, it's traditional
to continue to maintain the common aspects, despite the desire to
abandon them.
That said, I'm not the maintainer so feel free to disagree with my
point-of-view.
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 v3 3/6] TTY: fix DTR being raised on hang up
On Fri, 2013-03-15 at 12:30 +0100, Johan Hovold wrote:
> On Fri, Mar 15, 2013 at 07:03:08AM -0400, Peter Hurley wrote:
> > On Fri, 2013-03-15 at 10:24 +0100, Johan Hovold wrote:
> > > On Wed, Mar 13, 2013 at 03:43:43PM -0400, Peter Hurley wrote:
> > > > On Thu, 2013-03-07 at 15:55 +0100, Johan Hovold wrote:
> > > > > Make sure to check ASYNC_INITIALISED before raising DTR when waking up
> > > > > from blocked open in tty_port_block_til_ready.
> > > > >
> > > > > Currently DTR could get raised at hang up as a blocked process would
> > > > > raise DTR unconditionally before checking for hang up and returning.
> > > > >
> > > > > Signed-off-by: Johan Hovold
> > > > > ---
> > > > > drivers/tty/tty_port.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> > > > > index 3de5918..52f1066 100644
> > > > > --- a/drivers/tty/tty_port.c
> > > > > +++ b/drivers/tty/tty_port.c
> > > > > @@ -355,7 +355,7 @@ int tty_port_block_til_ready(struct tty_port
> > > > > *port,
> > > > >
> > > > > while (1) {
> > > > > /* Indicate we are open */
> > > > > - if (tty->termios.c_cflag & CBAUD)
> > > > > + if (C_BAUD(tty) && test_bit(ASYNCB_INITIALIZED,
> > > > > &port->flags))
> > > > > tty_port_raise_dtr_rts(port);
> > > > >
> > > > > prepare_to_wait(&port->open_wait, &wait,
> > > > > TASK_INTERRUPTIBLE);
> > > >
> > > > This is ok, but there are 6 other *_block_til_ready() functions:
> > ^^
> > Comment on patch
>
> I saw that, but just wanted to stress that those comments shouldn't
> block the series.
I completely agree. In fact, I should have said as much in the initial
review. Sorry.
> > > Yes, but that's not really a comment on this patch, is it?
> > >
> > > The purpose of this series is to fix the tty-port implementation, and
> > > I've only touched individual drivers when I had to in order not to break
> > > anything due to changed assumptions.
> > >
> > > There's a ton of buggy and odd behaviour to be found once you start
> > > turning the stones. Drivers like the ones below really ought to be
> > > using tty ports and it's helpers.
> >
> > Sure, I understand.
> >
> > OTOH, tty_port and these drivers stem from the same ancestor and it's
> > partly because of localized bug fixes like these that the drivers have
> > buggy and odd behavior (because tty_port gets fixed and these do not).
>
> Arguably, fixing the core isn't really a localised bug fix. Some of
> those drivers you mentioned have custom open, close, hangup which are
> quite different from the tty port implementation, and surely would have
> a lot to gain from being ported to tty ports if someone could find the
> time to do so.
I think the reluctance to do a full port is partly due to lack of
testable hardware.
> > As you can verify from the changelogs of these drivers, it's traditional
> > to continue to maintain the common aspects, despite the desire to
> > abandon them.
>
> Most entries I see have to do with changed interfaces.
>
> > That said, I'm not the maintainer so feel free to disagree with my
> > point-of-view.
>
> You do have a point, and I will try to find the time for a follow-up
> series fixing at least a few of those five-or-so custom block_til_ready
> you pointed to.
Thanks.
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 v2 33/85] USB: serial: clean up usb-serial bus device removal
On Thu, 2013-03-14 at 19:30 +0100, Johan Hovold wrote: > On Thu, Mar 14, 2013 at 01:39:39PM -0400, Peter Hurley wrote: > > On Thu, 2013-03-14 at 16:23 +0100, Johan Hovold wrote: > > > Make sure to unregister the tty-device before calling subdriver > > > port_remove. > > > > > > This way remove will reverse probe, and specifically any port data > > > released in port_remove will be available throughout tty unregister. > > > > > > Note that the order currently does not matter as the tty-layer can make > > > callbacks also after the device has been unregistered. This is > > > handled in usb-serial core using the disconnected flag, which is > > > already set when usb-serial bus device remove is called. > > > > > > Cc: Peter Hurley > > > Reported-by: Peter Hurley > > > Signed-off-by: Johan Hovold > > > > Looks fine to me. > > > > The thing I did notice is that the ftdi usb serial mini-driver has its > > own ioctl() handler for TIOCMIWAIT which can be holding the tty > > reference open after the unregister. > > Yes, this is expected and perfectly fine. User-space may hold any > tty-ref indefinitely, but we must unregister the tty-device at > disconnect and then deal with it using the disconnected flag. > > > Its port_remove() method, ftdi_sio_port_remove(), does the wake up to > > kick it off the loop. > > Yes, all is good (except for another use-after free bug in that very > implementation that I fixed elsewhere). We hang up the port at > disconnect, unregister the device from the bus, which calls ftdi port > remove and wakes up any processes waiting on the delta_msr_wait queue, > which then return immediately based on the disconnected flag (this last > bit wasn't the case before the fix mentioned above but has nothing to > do with tty unregister). > > > Now that the call order is reversed, that will need to be fixed. > > No, the call order in usb-serial bus remove does not change anything > really. Any process doing TIOCMIWAIT will only be woken up slightly > later (and again there is nothing wrong in keeping a tty ref after tty > unregister). I didn't mean it needed fixing because it would blow up; I just meant it needed fixing because it's not robust. Generally, if a tty driver needs to perform tty device-related cleanup, it should do so in the either the hangup() or close() call chain, not after it has knowingly unregistered the tty device. For the usb serial mini-drivers, this should be in their close() methods. > > A quick review of the usage seems like the whole ioctl() should be > > torn out along with the private delta_msr_wait. > > Most usb-serial TIOCIMWAIT-implementations, including the ftdi one, are > replaced by a generic implementation by this very series. Ok. I have a look at those. 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 v2 33/85] USB: serial: clean up usb-serial bus device removal
On Fri, 2013-03-15 at 16:20 +0100, Johan Hovold wrote: > Either way, this series moves the wake-up from the individual driver's > port remove to usb-serial disconnect and before unregistering the device > from the bus. > > However, looking at this again now, I realise that the wake up must be > done in hangup rather than disconnect (the common source for hangup in > usb-serial) in order to fully fix the problem with processes not being > woken up on hangup. (Perhaps, this is what you referred to with > "robust".) I didn't review the other patches yet, so I wasn't specifically referring to that. I just meant that current and future contributors may not have as much comprehension of the tty layer and the subtleties of tty lifetimes (which has obviously been a recurrent problem). As you point out, the problem is fixed by consolidating this construct into the glue layer. Thanks again, 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: Regression: ftdi_sio is slow (since Wed Oct 10 15:05:06 2012)
On 05/04/2013 07:15 AM, Johan Hovold wrote: On Sat, May 04, 2013 at 01:50:42AM +0400, Stas Sergeev wrote: 04.05.2013 00:34, Greg KH пишет: On Fri, May 03, 2013 at 10:27:18PM +0400, Stas Sergeev wrote: 03.05.2013 21:16, Greg KH пишет: [...] There's no guarantee as to how long select or an ioctl will take, and now that we have fixed another bug, this device is slower. If you change hardware types to use a different usb to serial chip, that select call might take 4 times as long. Are we somehow supposed to change the kernel to "fix" that? Previously, the kernel was not calling to a device at all, so select() was independent of the chip, and it was fast. I was not aware you changed that willingly. I don't understand, what do you mean by this? Some drivers just return the value of an internally held number, and don't query the device. The only way the FTDI driver can determine if the hardware buffer on the chip way out on the end of the USB cable is empty or not, is to query it. So the driver now does so. It does so only for one char. And the query takes longer than to just xmit that char. So why do you think this even works as expected? The query takes longer than the transmit at decent baudrates (>=38k) and under the assumption that flow control isn't causing any delays. But you do have a point, and I have been meaning to look into whether the added overhead of checking the hardware buffers could be mitigated by adding wait_until_sent support to usb-serial. This way the we would only query the hardware buffers on tty_wait_until_sent (e.g. at close) and select and TIOCMOUTQ would not suffer. This is also the way things are handled in serial_core. Agreed. This is the correct solution. I'll prepare a series which adds wait_until_sent to usb-serial, but I doubt it would be stable material (even if it could get into 3.10). What do you think Greg, is this overhead to chars_in_buffer reason enough to disable it in the stable trees or should we simply fix it in 3.11 (or 3.10)? (The overhead is about 3-400 us per call when the port fifo is empty, which makes chars_in_buffer about 100 times slower on my test system.) A better solution for stable would be to set port->drain_delay. It won't help tcdrain() but at least the port won't shutdown on live outbound data. 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 1/2] Revert "Revert "HID: Fix logitech-dj: missing Unifying device issue""
On 08/12/2013 05:54 PM, Peter Wu wrote: On Thursday 18 July 2013 16:28:01 Peter Hurley wrote: Before we revert to using the workaround, I'd like to suggest that this new "hidden" problem may be an interaction with the xhci_hcd host controller driver only. Looking at the related bug, the OP indicates the machine only has USB3 ports. Additionally, comments #7, #100, and #104 of the original bug report add additional information that would seem to confirm this suspicion. Let me add I have this USB device running on the uhci_hcd driver with or without this workaround on v3.10. This problem does not seem specific to xhci, uhci seems also effected. If true, it would certainly help to have a bug report confirming uhci failure from a bare-metal system which contained: 1) kernel version 2) complete dmesg output 3) lsusb -v output 4) lsmod output 5) usbmon capture from a plug attempt Today I upgraded a system (running Arch Linux) from kernel 3.9.9 to 3.10.5. After a reboot to 3.10.5, things broke. The setup: - There are two USB receivers plugged into USB 1.1 ports (different buses according to lsusb, uhci), each receiver is paired to a K360 keyboard. - One of the receivers are passed to a QEMU guest with -usbdevice host:$busid. $devid. This keyboard is working (probably because QEMU performed a reset). - Since 3.10.5, the keyboard that is *not* passed to the QEMU guest is not functioning on reboot. After closing the QEMU guest, the USB bus gets reset(?) after which the other keyboard suddenly gets detected. I had only booted 3.10.5 twice before rolling back to 3.9.9, both boots triggered the issue. Do I need to provide a usbmon, lsusb, dmesg and/ or other details from 3.10.5? Do both keyboards work on bare metal? Seems like this problem might be specific to qemu (or kvm) and you may get more insight on those lists. Note that there are other Arch Linux users who have reported issues[1][2] Unfortunately, not even one user in the referenced reports identified the usb hub the receiver was plugged into. since upgrading to 3.10.z. Triggering a re-enumeration by writing the magic HID++ message[3] makes the paired devices appear again (as reported in forums[1], I haven't tried this on the affected UHCI machine). While the underlying bug is fixed, can this patch be forwarded to stable? I see that 3.10.6 has been released, but still without this patch. This is still a workaround and not really a fix for the underlying bug. Regards, Peter [1]: https://bbs.archlinux.org/viewtopic.php?id=167210 [2]: https://bugs.archlinux.org/task/35991 [3]: https://bbs.archlinux.org/viewtopic.php?pid=1309535#p1309535 -- 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/2] Revert "Revert "HID: Fix logitech-dj: missing Unifying device issue""
On 08/13/2013 11:42 AM, Peter Wu wrote: On Tuesday 13 August 2013 08:13:17 Peter Hurley wrote: On 08/12/2013 05:54 PM, Peter Wu wrote: On Thursday 18 July 2013 16:28:01 Peter Hurley wrote: Before we revert to using the workaround, I'd like to suggest that this new "hidden" problem may be an interaction with the xhci_hcd host controller driver only. Looking at the related bug, the OP indicates the machine only has USB3 ports. Additionally, comments #7, #100, and #104 of the original bug report add additional information that would seem to confirm this suspicion. Let me add I have this USB device running on the uhci_hcd driver with or without this workaround on v3.10. This problem does not seem specific to xhci, uhci seems also effected. If true, it would certainly help to have a bug report confirming uhci failure from a bare-metal system which contained: 1) kernel version 2) complete dmesg output 3) lsusb -v output 4) lsmod output 5) usbmon capture from a plug attempt I was too fast in drawing a conclusion, besides the kernel I also upgraded some other packages. Today the issue also showed up in 3.9.9 + updated packages. When checking the dmesg, the issue solved by this patch did not occur (the enumeration was successful). Thanks for double-checking. Today I upgraded a system (running Arch Linux) from kernel 3.9.9 to 3.10.5. After a reboot to 3.10.5, things broke. The setup: - There are two USB receivers plugged into USB 1.1 ports (different buses according to lsusb, uhci), each receiver is paired to a K360 keyboard. - One of the receivers are passed to a QEMU guest with -usbdevice host:$busid. $devid. This keyboard is working (probably because QEMU performed a reset). - Since 3.10.5, the keyboard that is *not* passed to the QEMU guest is not functioning on reboot. After closing the QEMU guest, the USB bus gets reset(?) after which the other keyboard suddenly gets detected. I had only booted 3.10.5 twice before rolling back to 3.9.9, both boots triggered the issue. Do I need to provide a usbmon, lsusb, dmesg and/ or other details from 3.10.5? Do both keyboards work on bare metal? Seems like this problem might be specific to qemu (or kvm) and you may get more insight on those lists. I haven't tested that, the system automatically boots into openbox + QEMU. Previously, both keyboards worked on bare metal, so I think it still works. Note that there are other Arch Linux users who have reported issues[1][2] Unfortunately, not even one user in the referenced reports identified the usb hub the receiver was plugged into. I've asked it now. Thanks. And if someone has a uhci failure, filing a new bug on bugzilla.kernel.org _and_ posting to linux-usb@ and linux-in...@vger.kernel.org improves the chances of the right people seeing the problem. [xhci already has several reports plus I subscribed to the kernel bug linked from the ArchLinux bug report.] since upgrading to 3.10.z. Triggering a re-enumeration by writing the magic HID++ message[3] makes the paired devices appear again (as reported in forums[1], I haven't tried this on the affected UHCI machine). While the underlying bug is fixed, can this patch be forwarded to stable? I see that 3.10.6 has been released, but still without this patch. This is still a workaround and not really a fix for the underlying bug. I meant to say, "while the underlying bug is *being* fixed". Anyway, can this patch be applied to 3.10? That's really Jiri's call. TBH, I can't blame him for wanting to shake this out in 3.11 before pushing to stable. Sorry for the confusion with uhci, looking further it seems that the wrong USB receiver is being passed to QEMU. It's not a kernel issue, perhaps I can blame libusbx. No apologies necessary. 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
[-next-20130204] usb/hcd: irq 18: nobody cared
With -next-20130204:
[ 33.855570] irq 18: nobody cared (try booting with the "irqpoll" option)
[ 33.855580] Pid: 0, comm: swapper/4 Not tainted 3.8.0-next-20130204-xeon
#20130204
[ 33.855582] Call Trace:
[ 33.855585][] __report_bad_irq+0x36/0xe0
[ 33.855600] [] note_interrupt+0x1aa/0x200
[ 33.855606] [] ? mwait_idle+0x82/0x1b0
[ 33.855610] [] handle_irq_event_percpu+0xc9/0x260
[ 33.855614] [] handle_irq_event+0x48/0x70
[ 33.855618] [] handle_fasteoi_irq+0x5a/0x100
[ 33.855624] [] handle_irq+0x22/0x40
[ 33.855630] [] do_IRQ+0x5a/0xd0
[ 33.855636] [] common_interrupt+0x6d/0x6d
[ 33.855638][] ? rcu_eqs_enter_common+0x4a/0x320
[ 33.855646] [] ? mwait_idle+0x82/0x1b0
[ 33.855649] [] ? mwait_idle+0x29/0x1b0
[ 33.855653] [] cpu_idle+0x116/0x130
[ 33.855658] [] start_secondary+0x251/0x258
[ 33.855660] handlers:
[ 33.855664] [] usb_hcd_irq
[ 33.855667] Disabling IRQ #18
>From earlier in the boot log:
[1.297020] uhci_hcd :00:1d.2: setting latency timer to 64
[1.297032] uhci_hcd :00:1d.2: UHCI Host Controller
[1.297040] uhci_hcd :00:1d.2: new USB bus registered, assigned bus
number 4
[1.297076] uhci_hcd :00:1d.2: irq 18, io base 0xff40
[1.297213] hub 4-0:1.0: USB hub found
[1.297221] hub 4-0:1.0: 2 ports detected
lsusb:
...
Bus 004 Device 002: ID 0764:0501 Cyber Power System, Inc. CP1500 AVR UPS
Bus 004 Device 003: ID 046d:c52b Logitech, Inc. Unifying Receiver
...
Didn't have this problem with -next-20120125. One of these commits, maybe?
next/drivers/usb/core$ git log --oneline next-20130125..next-20130204 -- *
2c2e865 usb: forbid memory allocation with I/O during bus reset
e9121a8 Merge remote-tracking branch 'usb/usb-next'
3b2ab2b Revert "usb: Register usb port's acpi power resources"
da0aa71 USB: add usb_hcd_{start,end}_port_resume
192fef1 usb: enable usb port device's async suspend.
f6cced1 usb: expose usb port's pm qos flags to user space
ad493e5 usb: add usb port auto power off mechanism
971fcd4 usb: add runtime pm support for usb port device
88bb965 usb: Register usb port's acpi power resources
54a3ac0 usb: Using correct way to clear usb3.0 device's remote wakeup feature.
I didn't see any changes in the drivers/hid/hid-logitech-dj.c or usbhid
but maybe it's doing something wrong anyway?
I'll open a bugzilla if a bunch more info is necessary.
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: [-next-20130204] usb/hcd: irq 18: nobody cared
On Tue, 2013-02-05 at 15:26 -0500, Alan Stern wrote:
> On Tue, 5 Feb 2013, Peter Hurley wrote:
>
> > With -next-20130204:
> >
> > [ 33.855570] irq 18: nobody cared (try booting with the "irqpoll" option)
> > [ 33.855580] Pid: 0, comm: swapper/4 Not tainted 3.8.0-next-20130204-xeon
> > #20130204
> > [ 33.855582] Call Trace:
> > [ 33.855585][] __report_bad_irq+0x36/0xe0
> > [ 33.855600] [] note_interrupt+0x1aa/0x200
> > [ 33.855606] [] ? mwait_idle+0x82/0x1b0
> > [ 33.855610] [] handle_irq_event_percpu+0xc9/0x260
> > [ 33.855614] [] handle_irq_event+0x48/0x70
> > [ 33.855618] [] handle_fasteoi_irq+0x5a/0x100
> > [ 33.855624] [] handle_irq+0x22/0x40
> > [ 33.855630] [] do_IRQ+0x5a/0xd0
> > [ 33.855636] [] common_interrupt+0x6d/0x6d
> > [ 33.855638][] ?
> > rcu_eqs_enter_common+0x4a/0x320
> > [ 33.855646] [] ? mwait_idle+0x82/0x1b0
> > [ 33.855649] [] ? mwait_idle+0x29/0x1b0
> > [ 33.855653] [] cpu_idle+0x116/0x130
> > [ 33.855658] [] start_secondary+0x251/0x258
> > [ 33.855660] handlers:
> > [ 33.855664] [] usb_hcd_irq
> > [ 33.855667] Disabling IRQ #18
> >
> > From earlier in the boot log:
> > [1.297020] uhci_hcd :00:1d.2: setting latency timer to 64
> > [1.297032] uhci_hcd :00:1d.2: UHCI Host Controller
> > [1.297040] uhci_hcd :00:1d.2: new USB bus registered, assigned bus
> > number 4
> > [1.297076] uhci_hcd :00:1d.2: irq 18, io base 0xff40
> > [1.297213] hub 4-0:1.0: USB hub found
> > [1.297221] hub 4-0:1.0: 2 ports detected
> >
> > lsusb:
> > ...
> > Bus 004 Device 002: ID 0764:0501 Cyber Power System, Inc. CP1500 AVR UPS
> > Bus 004 Device 003: ID 046d:c52b Logitech, Inc. Unifying Receiver
> > ...
> >
> > Didn't have this problem with -next-20120125. One of these commits, maybe?
> > next/drivers/usb/core$ git log --oneline next-20130125..next-20130204 -- *
> > 2c2e865 usb: forbid memory allocation with I/O during bus reset
> > e9121a8 Merge remote-tracking branch 'usb/usb-next'
> > 3b2ab2b Revert "usb: Register usb port's acpi power resources"
> > da0aa71 USB: add usb_hcd_{start,end}_port_resume
> > 192fef1 usb: enable usb port device's async suspend.
> > f6cced1 usb: expose usb port's pm qos flags to user space
> > ad493e5 usb: add usb port auto power off mechanism
> > 971fcd4 usb: add runtime pm support for usb port device
> > 88bb965 usb: Register usb port's acpi power resources
> > 54a3ac0 usb: Using correct way to clear usb3.0 device's remote wakeup
> > feature.
>
> None of those commits seems likely to have caused the problem. The
> most likely is da0aa71, combined with one you didn't mention:
>
> 840008bb USB: UHCI: notify usbcore about port resumes
>
> If you revert these two commits, does that make any difference? What
> if you revert all of these commits?
None of these commits are directly causing this. I tested Greg's usb
master and usb-next and both run fine for me.
> > I didn't see any changes in the drivers/hid/hid-logitech-dj.c or usbhid
> > but maybe it's doing something wrong anyway?
> >
> > I'll open a bugzilla if a bunch more info is necessary.
>
> If the suggestion above doesn't work out, bisection may be the best way
> to find the answer.
Any pointers on how to one of) build a linear history for -next, get
bisect to work on an integration tree, or third option I haven't thought
of?
Thanks,
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
[Bisected] [-next-20130204] usb/hcd: irq 18: nobody cared
On Tue, 2013-02-05 at 15:26 -0500, Alan Stern wrote: > On Tue, 5 Feb 2013, Peter Hurley wrote: > > > With -next-20130204: > > > > [ 33.855570] irq 18: nobody cared (try booting with the "irqpoll" option) > > [ 33.855580] Pid: 0, comm: swapper/4 Not tainted 3.8.0-next-20130204-xeon > > #20130204 > > [ 33.855582] Call Trace: > > [ 33.855585][] __report_bad_irq+0x36/0xe0 > > [ 33.855600] [] note_interrupt+0x1aa/0x200 > > [ 33.855606] [] ? mwait_idle+0x82/0x1b0 > > [ 33.855610] [] handle_irq_event_percpu+0xc9/0x260 > > [ 33.855614] [] handle_irq_event+0x48/0x70 > > [ 33.855618] [] handle_fasteoi_irq+0x5a/0x100 > > [ 33.855624] [] handle_irq+0x22/0x40 > > [ 33.855630] [] do_IRQ+0x5a/0xd0 > > [ 33.855636] [] common_interrupt+0x6d/0x6d > > [ 33.855638][] ? > > rcu_eqs_enter_common+0x4a/0x320 > > [ 33.855646] [] ? mwait_idle+0x82/0x1b0 > > [ 33.855649] [] ? mwait_idle+0x29/0x1b0 > > [ 33.855653] [] cpu_idle+0x116/0x130 > > [ 33.855658] [] start_secondary+0x251/0x258 > > [ 33.855660] handlers: > > [ 33.855664] [] usb_hcd_irq > > [ 33.855667] Disabling IRQ #18 > > > > From earlier in the boot log: > > [1.297020] uhci_hcd :00:1d.2: setting latency timer to 64 > > [1.297032] uhci_hcd :00:1d.2: UHCI Host Controller > > [1.297040] uhci_hcd :00:1d.2: new USB bus registered, assigned bus > > number 4 > > [1.297076] uhci_hcd :00:1d.2: irq 18, io base 0xff40 > > [1.297213] hub 4-0:1.0: USB hub found > > [1.297221] hub 4-0:1.0: 2 ports detected > > > > lsusb: > > ... > > Bus 004 Device 002: ID 0764:0501 Cyber Power System, Inc. CP1500 AVR UPS > > Bus 004 Device 003: ID 046d:c52b Logitech, Inc. Unifying Receiver [...] > If the suggestion above doesn't work out, bisection may be the best way > to find the answer. This bad irq bisected to: 4f535093cf8f6da8cfda7c36c2c1ecd2e9586ee4 is the first bad commit commit 4f535093cf8f6da8cfda7c36c2c1ecd2e9586ee4 Author: Yinghai Lu Date: Mon Jan 21 13:20:52 2013 -0800 PCI: Put pci_dev in device tree as early as possible We want to put pci_dev structs in the device tree as soon as possible so for_each_pci_dev() iteration will not miss them, but driver attachment needs to be delayed until after pci_assign_unassigned_resources() to make sure all devices have resources assigned first. This patch moves device registering from pci_bus_add_devices() to pci_device_add(), which happens earlier, leaving driver attachment in pci_bus_add_devices(). It also removes unattached child bus handling in pci_bus_add_devices(). That's not needed because child bus via pci_add_new_bus() is already in parent bus children list. [bhelgaas: changelog] Signed-off-by: Yinghai Lu Signed-off-by: Bjorn Helgaas Acked-by: Rafael J. Wysocki :04 04 0540c98d04d00de24b4e12fa750f6cd26c5addd2 2e24946cb7165a4028b7efb0a622271cc3990005 M drivers All is fine if I revert these 2 commits: 40064ac PCI: Remove unused "rc" in virtfn_add_bus() 4f53509 (HEAD, refs/bisect/bad) PCI: Put pci_dev in device tree as early as possible Any ideas how these are causing the USB HCD core / hid-logitech-dj driver to drop interrupts? 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: [Bisected] [-next-20130204] usb/hcd: irq 18: nobody cared
On Sat, 2013-02-09 at 22:14 -0500, Peter Hurley wrote: > On Tue, 2013-02-05 at 15:26 -0500, Alan Stern wrote: > > On Tue, 5 Feb 2013, Peter Hurley wrote: > > > > > With -next-20130204: > > > > > > [ 33.855570] irq 18: nobody cared (try booting with the "irqpoll" > > > option) > > > [ 33.855580] Pid: 0, comm: swapper/4 Not tainted > > > 3.8.0-next-20130204-xeon #20130204 > > > [ 33.855582] Call Trace: > > > [ 33.855585][] __report_bad_irq+0x36/0xe0 > > > [ 33.855600] [] note_interrupt+0x1aa/0x200 > > > [ 33.855606] [] ? mwait_idle+0x82/0x1b0 > > > [ 33.855610] [] handle_irq_event_percpu+0xc9/0x260 > > > [ 33.855614] [] handle_irq_event+0x48/0x70 > > > [ 33.855618] [] handle_fasteoi_irq+0x5a/0x100 > > > [ 33.855624] [] handle_irq+0x22/0x40 > > > [ 33.855630] [] do_IRQ+0x5a/0xd0 > > > [ 33.855636] [] common_interrupt+0x6d/0x6d > > > [ 33.855638][] ? > > > rcu_eqs_enter_common+0x4a/0x320 > > > [ 33.855646] [] ? mwait_idle+0x82/0x1b0 > > > [ 33.855649] [] ? mwait_idle+0x29/0x1b0 > > > [ 33.855653] [] cpu_idle+0x116/0x130 > > > [ 33.855658] [] start_secondary+0x251/0x258 > > > [ 33.855660] handlers: > > > [ 33.855664] [] usb_hcd_irq > > > [ 33.855667] Disabling IRQ #18 > > > > > > From earlier in the boot log: > > > [1.297020] uhci_hcd :00:1d.2: setting latency timer to 64 > > > [1.297032] uhci_hcd :00:1d.2: UHCI Host Controller > > > [1.297040] uhci_hcd :00:1d.2: new USB bus registered, assigned > > > bus number 4 > > > [1.297076] uhci_hcd :00:1d.2: irq 18, io base 0xff40 > > > [1.297213] hub 4-0:1.0: USB hub found > > > [1.297221] hub 4-0:1.0: 2 ports detected > > > > > > lsusb: > > > ... > > > Bus 004 Device 002: ID 0764:0501 Cyber Power System, Inc. CP1500 AVR UPS > > > Bus 004 Device 003: ID 046d:c52b Logitech, Inc. Unifying Receiver > > [...] > > > If the suggestion above doesn't work out, bisection may be the best way > > to find the answer. > > This bad irq bisected to: > > 4f535093cf8f6da8cfda7c36c2c1ecd2e9586ee4 is the first bad commit > commit 4f535093cf8f6da8cfda7c36c2c1ecd2e9586ee4 > Author: Yinghai Lu > Date: Mon Jan 21 13:20:52 2013 -0800 > > PCI: Put pci_dev in device tree as early as possible > > We want to put pci_dev structs in the device tree as soon as possible so > for_each_pci_dev() iteration will not miss them, but driver attachment > needs to be delayed until after pci_assign_unassigned_resources() to make > sure all devices have resources assigned first. > > This patch moves device registering from pci_bus_add_devices() to > pci_device_add(), which happens earlier, leaving driver attachment in > pci_bus_add_devices(). > > It also removes unattached child bus handling in pci_bus_add_devices(). > That's not needed because child bus via pci_add_new_bus() is already > in parent bus children list. > > [bhelgaas: changelog] > Signed-off-by: Yinghai Lu > Signed-off-by: Bjorn Helgaas > Acked-by: Rafael J. Wysocki > > :04 04 0540c98d04d00de24b4e12fa750f6cd26c5addd2 > 2e24946cb7165a4028b7efb0a622271cc3990005 Mdrivers > > All is fine if I revert these 2 commits: > > 40064ac PCI: Remove unused "rc" in virtfn_add_bus() > 4f53509 (HEAD, refs/bisect/bad) PCI: Put pci_dev in device tree as early as > possible > > Any ideas how these are causing the USB HCD core / hid-logitech-dj > driver to drop interrupts? https://bugzilla.kernel.org/show_bug.cgi?id=53561 Maybe this is some interaction with all the new ACPI code and fixes written in those 8 days. peter@thor:~/src/kernels/next$ git log --oneline next-20130125..next-20130204 | grep -i acpi 65d6ce7 Merge remote-tracking branch 'acpi/next' 0d50e8c Merge branch 'acpi-pm-next' into linux-next be6d286 PCI: acpiphp: Remove dead code for PCI host bridge hotplug 4b794a0 Merge branch 'acpi-assorted' into linux-next bd3e4a3 Merge branch 'acpi-cleanup' into linux-next 02df734 ACPI / dock: Fix acpi_bus_get_device() check in drivers/acpi/dock.c 7217dac Merge branch 'acpi-lpss' into linux-next 4bede3f Merge branch 'acpi-pm' into linux-next cc0755b Merge branch 'acpica' into linux-next 308b10d Merge branch 'acpi-scan' into linux-next 2ca344e PCI: acpiphp: Create companion ACPI devices before creating PCI devices 741220e cpufreq: Make acpi-cpuf
Re: [Bisected] [-next-20130204] usb/hcd: irq 18: nobody cared
On Sun, 2013-02-10 at 22:40 -0800, Yinghai Lu wrote: > On Sun, Feb 10, 2013 at 12:33 PM, Yinghai Lu wrote: > > On Sun, Feb 10, 2013 at 6:23 AM, Peter Hurley > > wrote: > >> On Sat, 2013-02-09 at 22:14 -0500, Peter Hurley wrote: > >>> On Tue, 2013-02-05 at 15:26 -0500, Alan Stern wrote: > >>> > On Tue, 5 Feb 2013, Peter Hurley wrote: > >>> > > >>> > > With -next-20130204: > >>> > > > >>> > > [ 33.855570] irq 18: nobody cared (try booting with the "irqpoll" > >>> > > option) > >>> > > [ 33.855580] Pid: 0, comm: swapper/4 Not tainted > >>> > > 3.8.0-next-20130204-xeon #20130204 > >>> > > [ 33.855582] Call Trace: > >>> > > [ 33.855585][] __report_bad_irq+0x36/0xe0 > >>> > > [ 33.855600] [] note_interrupt+0x1aa/0x200 > >>> > > [ 33.855606] [] ? mwait_idle+0x82/0x1b0 > >>> > > [ 33.855610] [] > >>> > > handle_irq_event_percpu+0xc9/0x260 > >>> > > [ 33.855614] [] handle_irq_event+0x48/0x70 > >>> > > [ 33.855618] [] handle_fasteoi_irq+0x5a/0x100 > >>> > > [ 33.855624] [] handle_irq+0x22/0x40 > >>> > > [ 33.855630] [] do_IRQ+0x5a/0xd0 > >>> > > [ 33.855636] [] common_interrupt+0x6d/0x6d > >>> > > [ 33.855638][] ? > >>> > > rcu_eqs_enter_common+0x4a/0x320 > >>> > > [ 33.855646] [] ? mwait_idle+0x82/0x1b0 > >>> > > [ 33.855649] [] ? mwait_idle+0x29/0x1b0 > >>> > > [ 33.855653] [] cpu_idle+0x116/0x130 > >>> > > [ 33.855658] [] start_secondary+0x251/0x258 > >>> > > [ 33.855660] handlers: > >>> > > [ 33.855664] [] usb_hcd_irq > >>> > > [ 33.855667] Disabling IRQ #18 > >>> > > > >> https://bugzilla.kernel.org/show_bug.cgi?id=53561 > >> > >> Maybe this is some interaction with all the new ACPI code and fixes > >> written in those 8 days. > > > > interrupt routing seems get changed: > > next: > >5: 0 0 0 0 0 > > 0 0 0 IO-APIC-fasteoi snd_ctxfi > > 18: 99970 13 16 20 99940 > > 13 13 16 IO-APIC-fasteoi uhci_hcd:usb4 > > v3.8-rc7: > > 18:424 15 11112420 > > 16 18105 IO-APIC-fasteoi uhci_hcd:usb4, snd_ctxfi > > > > These messages in the bad dmesg log are interesting since PCI INT A is > > routed > > on > > irq 18 with the kernels that work. > > [8.983246] pci :00:1e.0: can't derive routing for PCI INT A > > [8.983600] snd_ctxfi :09:02.0: PCI INT A: no GSI - using ISA IRQ 5 > > ... > > > > acpi_pci_irq_add_prt() add wrong bus for that bridge, because that > > that bridge is not scanned. > > > > Will check if I can produce one patch for it. > > Hi Peter, > > Can you try attached debug patch? Fixed, thanks. 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: USB Ooops PL2303 when unplug while use (linux v3.7.3)
On Wed, 2013-02-13 at 15:25 +0100, Johan Hovold wrote:
> On Fri, Feb 01, 2013 at 10:44:25AM +0800, Chris Ruehl wrote:
> > I file a report for you, please have a look when you have time.
>
> Thanks for the bug report, Chris.
>
> You have come across what looks like a known issue, which since it's
> discovery last summer has been made worse by an unrelated change.
>
> A similar oops was reported and its cause identified in this thread:
>
> http://marc.info/?l=linux-usb&m=133837337927749&w=2
>
> It turns out that the tty-layer may call the driver's dtr_rts even after
> the device has been disconnected and the tty device unregistered. Since
> last summer another change has made the problem worse by setting the
> port data to NULL which results in even more drivers hitting the
> problem.
The tty driver's close() routine must be called even if the open()
failed because the tty layer doesn't know if the driver left unfinished
business and is expecting to receive a close() to cleanup even if it
failed the open(). This behavior was just recently documented in
include/linux/tty_driver.h (ie., is in linux-next).
> While waiting for input from the tty-guru Alan Cox, and as the immediate
> cause of that oops was remedied (by moving the offending interface
> access in the driver in question), the problem was unfortunately
> forgotten (or rather down-prioritised) until now.
Looks to me like a bug the usb serial mini-port interface design.
A usb bus disconnect has the pl2303 (and every other) mini-port freeing
the private data (before unregistering with tty anyway -- not that
putting that first would fix the problem).
static int usb_serial_device_remove(struct device *dev)
{
struct usb_serial_driver *driver;
struct usb_serial_port *port;
int retval = 0;
int minor;
port = to_usb_serial_port(dev);
if (!port)
return -ENODEV;
/* make sure suspend/resume doesn't race against port_remove */
usb_autopm_get_interface(port->serial->interface);
device_remove_file(&port->dev, &dev_attr_port_number);
driver = port->serial->type;
if (driver->port_remove)
> retval = driver->port_remove(port);
minor = port->number;
tty_unregister_device(usb_serial_tty_driver, minor);
dev_info(dev, "%s converter now disconnected from ttyUSB%d\n",
driver->description, minor);
usb_autopm_put_interface(port->serial->interface);
return retval;
}
The pl2303 mini-port dutifully:
static int pl2303_port_remove(struct usb_serial_port *port)
{
struct pl2303_private *priv;
priv = usb_get_serial_port_data(port);
===>kfree(priv);
return 0;
}
while the tty layer still has outstanding references to the port.
static void pl2303_dtr_rts(struct usb_serial_port *port, int on)
{
=> struct pl2303_private *priv = usb_get_serial_port_data(port);
unsigned long flags;
u8 control;
[...]
The tty layer (and its port implementation) can't protect the pl2303
mini-port from this behavior/interface design.
It's the glue layer's responsibility to correctly manage the differing
lifetimes of its bus devices with tty devices (and tty_ports, if used).
Meaning, that if the physical device disconnects from the bus, the ports
must simply become in-operative; they can't disappear.
BTW, just fixing this one part won't work because the usb serial driver
is also abruptly deleting the usb_serial_port device as well:
static void usb_serial_disconnect(struct usb_interface *interface)
{
[...]
for (i = 0; i < serial->num_ports; ++i) {
port = serial->port[i];
if (port) {
struct tty_struct *tty = tty_port_tty_get(&port->port);
if (tty) {
tty_vhangup(tty);
tty_kref_put(tty);
}
kill_traffic(port);
cancel_work_sync(&port->work);
if (device_is_registered(&port->dev))
> device_del(&port->dev);
}
}
[...]
}
Ummm, wasn't that where the port private data pointer was?
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: [RFC 2/4] tty: fix DTR/RTS not being dropped on hang up
On Wed, 2013-02-13 at 18:27 +0100, Johan Hovold wrote:
> Move HUPCL handling to port shutdown so that DTR/RTS is dropped also on
> hang up.
>
> Currently a hung up port will return immediately from
> tty_port_close_start leaving DTR/RTS unchanged.
> ---
> drivers/tty/tty_port.c | 22 --
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> index 57a061e..ffe3689 100644
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -198,11 +198,20 @@ EXPORT_SYMBOL(tty_port_tty_set);
>
> static void tty_port_shutdown(struct tty_port *port)
> {
> + struct tty_struct *tty = port->tty;
> +
> mutex_lock(&port->mutex);
> if (port->console)
> goto out;
>
> if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
> + /*
> + * Drop DTR/RTS if HUPCL is set. This causes any attached
> + * modem to hang up the line.
> + */
> + if (!tty || tty->termios.c_cflag & HUPCL)
> + tty_port_lower_dtr_rts(port);
> +
port->ops->shutdown() requires the hardware to reset anyway, including
the DTR/RTS state.
--
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 2/4] tty: fix DTR/RTS not being dropped on hang up
On Wed, 2013-02-13 at 14:32 -0500, Peter Hurley wrote:
> On Wed, 2013-02-13 at 18:27 +0100, Johan Hovold wrote:
> > Move HUPCL handling to port shutdown so that DTR/RTS is dropped also on
> > hang up.
> >
> > Currently a hung up port will return immediately from
> > tty_port_close_start leaving DTR/RTS unchanged.
> > ---
> > drivers/tty/tty_port.c | 22 --
> > 1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> > index 57a061e..ffe3689 100644
> > --- a/drivers/tty/tty_port.c
> > +++ b/drivers/tty/tty_port.c
> > @@ -198,11 +198,20 @@ EXPORT_SYMBOL(tty_port_tty_set);
> >
> > static void tty_port_shutdown(struct tty_port *port)
> > {
> > + struct tty_struct *tty = port->tty;
> > +
> > mutex_lock(&port->mutex);
> > if (port->console)
> > goto out;
> >
> > if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
> > + /*
> > +* Drop DTR/RTS if HUPCL is set. This causes any attached
> > +* modem to hang up the line.
> > +*/
> > + if (!tty || tty->termios.c_cflag & HUPCL)
> > + tty_port_lower_dtr_rts(port);
> > +
>
> port->ops->shutdown() requires the hardware to reset anyway, including
> the DTR/RTS state.
Ok, after reviewing this further, I like this more.
For one, there is more flexibility for tty drivers that have a custom
close() routine (ie, one that doesn't call tty_port_close()). At the
same time, every call site of tty_port_close_start() needs to be checked
to ensure it does not assume (or does not require) DTR to be dropped:
char/pcmcia/synclink_cs.c: if (tty_port_close_start(port, tty, filp) == 0)
tty/synclinkmp.c: if (tty_port_close_start(&info->port, tty, filp) == 0)
tty/rocket.c: if (tty_port_close_start(port, tty, filp) == 0)
tty/amiserial.c:if (tty_port_close_start(port, tty, filp) == 0)
tty/n_gsm.c:if (tty_port_close_start(&dlci->port, tty, filp) == 0)
tty/mxser.c:if (tty_port_close_start(port, tty, filp) == 0)
tty/synclink_gt.c: if (tty_port_close_start(&info->port, tty, filp) == 0)
tty/serial/serial_core.c: if (tty_port_close_start(port, tty, filp) == 0)
tty/synclink.c: if (tty_port_close_start(&info->port, tty, filp) == 0)
In the case of the serial core, this actually fixes the code excerpt
below where tty_port_close_start() prematurely drops DTR before the uart
can stop_rx() or wait_until_sent().
static void uart_close(struct tty_struct *tty, struct file *filp)
{
struct uart_state *state = tty->driver_data;
struct tty_port *port;
struct uart_port *uport;
unsigned long flags;
if (!state)
return;
uport = state->uart_port;
port = &state->port;
pr_debug("uart_close(%d) called\n", uport->line);
if (tty_port_close_start(port, tty, filp) == 0)
return;
/*
* At this point, we stop accepting input. To do this, we
* disable the receive line status interrupts.
*/
if (port->flags & ASYNC_INITIALIZED) {
unsigned long flags;
spin_lock_irqsave(&uport->lock, flags);
uport->ops->stop_rx(uport);
spin_unlock_irqrestore(&uport->lock, flags);
/*
* Before we drop DTR, make sure the UART transmitter
* has completely drained; this is especially
* important if there is a transmit FIFO!
*/
uart_wait_until_sent(tty, uport->timeout);
}
mutex_lock(&port->mutex);
uart_shutdown(tty, state);
uart_flush_buffer(tty);
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: [RFC 3/4] tty: clean up port drain-delay handling
On Wed, 2013-02-13 at 18:27 +0100, Johan Hovold wrote:
> Move port drain-delay handling to a separate function.
> ---
> drivers/tty/tty_port.c | 29 ++---
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> index ffe3689..0a5e955 100644
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -407,6 +407,23 @@ int tty_port_block_til_ready(struct tty_port *port,
> }
> EXPORT_SYMBOL(tty_port_block_til_ready);
>
> +static void tty_port_drain_delay(struct tty_port *port, struct tty_struct
> *tty)
> +{
> + unsigned int bps = tty_get_baud_rate(tty);
> + long timeout;
> + if (!port->drain_delay)
> + return;
How about drop this and ...
> + if (bps > 1200) {
> + timeout = (HZ * 10 * port->drain_delay) / bps;
> + timeout = max_t(long, timeout, HZ / 10);
> + } else {
> + timeout = 2 * HZ;
> + }
> + schedule_timeout_interruptible(timeout);
> +}
> +
> int tty_port_close_start(struct tty_port *port,
> struct tty_struct *tty, struct file *filp)
> {
> @@ -445,17 +462,7 @@ int tty_port_close_start(struct tty_port *port,
> if (test_bit(ASYNCB_INITIALIZED, &port->flags) &&
> port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
> tty_wait_until_sent_from_close(tty, port->closing_wait);
> - if (port->drain_delay) {
> - unsigned int bps = tty_get_baud_rate(tty);
> - long timeout;
> -
> - if (bps > 1200)
> - timeout = max_t(long,
> - (HZ * 10 * port->drain_delay) / bps, HZ / 10);
> - else
> - timeout = 2 * HZ;
> - schedule_timeout_interruptible(timeout);
> - }
> + tty_port_drain_delay(port, tty);
if (port->drain_delay)
tty_port_drain_delay(port, tty);
> /* Flush the ldisc buffering */
> tty_ldisc_flush(tty);
>
--
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 0/4] TTY: port hangup and close fixes
On Wed, 2013-02-20 at 17:02 +0100, Johan Hovold wrote: > These patches against tty-next fix a few issues with tty-port hangup and > close. > > The first and third patch are essentially clean ups. > > The second patch makes sure DTR is dropped also on hangup and that DTR > is only dropped for initialised ports (where is could have been raised > in the first place). > > The fourth and final patch, make sure no tty callbacks are made from > tty_port_close_start when the port has not been initialised (successfully > opened). This was previously only done for wait_until_sent but there's > no reason to call flush_buffer or to honour port drain delay either. > The latter could cause a failed open to stall for up to two seconds. > > As a side-effect, this patches also fix an issue in USB-serial where we > could get tty-port callbacks on an uninitialised port after having hung > up and unregistered a device after disconnect. > > Johan Looks good to me. No further objections :) > Changes since RFC-series: > - fix up the two driver relying on tty_port_close_start directly but >that did not manage DTR themselves > > > Johan Hovold (4): > TTY: clean up port shutdown > TTY: fix DTR not being dropped on hang up > TTY: clean up port drain-delay handling > TTY: fix close of uninitialised ports > > drivers/tty/mxser.c| 4 +++ > drivers/tty/n_gsm.c| 4 +++ > drivers/tty/tty_port.c | 71 > ++ > 3 files changed, 51 insertions(+), 28 deletions(-) > -- 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: USB: serial: port lifetimes (was: Re: USB Ooops PL2303 when unplug while use (linux v3.7.3))
On Fri, 2013-02-22 at 12:35 -0500, Alan Stern wrote:
> On Fri, 22 Feb 2013, Johan Hovold wrote:
>
> > > > So we end up with an unregistered device still possibly referenced by
> > > > tty instead, and I suspect we can't do much else than deal with any
> > > > post-disconnect callbacks. [ These should be few, especially with my
> > > > latest TTY-patches applied. ]
> > > >
> > > > Alan, do you see any way around this? It's not possible (or desirable)
> > > > to pin the parent device (interface) until the last reference is
> > > > dropped, is it?
> > >
> > > On the contrary, it is customary to pin data structures until the last
> > > reference to them is gone. That's what krefs are for.
> >
> > I was referring to the usb device in the device hierarchy, which
> > apparently is not pinned despite the outstanding reference we have in
> > struct usb_serial.
>
> Are you talking about the usb_device or the usb_interface? The
> usb_serial structure _does_ pin the usb_device structure. But it
> doesn't pin the usb_interface. I don't know why things were done this
> way; maybe it's a mistake.
>
> Anyway, keeping a pointer to a non-pinned data structure after
> unregistration is okay, provided you know you will never dereference
> the pointer. If you don't know this in the case of the usb_interface,
> pinning is acceptable -- depending on _how_ the usb_interface is
> accessed. For example, no URBs should be submitted for any of the
> interface's endpoints.
>
> > There is an unconditional call to device_del in usb_disconnect which
> > unlinks the parent usb device from the device hierarchy resulting in the
> > broken devpaths above if we do not unregister the usb-serial port (and
> > tty device) in disconnect.
>
> Sure. But unregistering is different from deallocation. It's not
> clear what your point is.
>
> > > On the other hand, the port private data was owned entirely by the
> > > serial sub-driver. Neither the serial core nor the tty layer is able
> > > to use it meaningfully -- they don't even know what type of structure
> > > it is.
> > >
> > > Therefore freeing the structure when the port is removed should be
> > > harmless -- unless the subdriver is called after the structure is
> > > deallocated.
> >
> > Which could happen (and is happening), unless we defer port unregister
> > until the last tty reference is dropped -- but then we get the broken
> > uevents.
>
> Unregistration should not be deferred. We mustn't have those broken
> devpaths.
>
> > > This means there still is one bug remaining. In
> > > usb_serial_device_remove(), the call to tty_unregister_device() should
> > > occur _before_ the call to driver->port_remove(), not _after_. Do you
> > > think changing the order cause any new problems?
> >
> > Yes, Peter noticed that one too. Changing the order shouldn't cause any
> > new issues as far as I can see. I'll cook up a patch for this one as
> > well, but just to be clear: this is not directly related to the problem
> > discussed above as there may be outstanding tty references long after
> > both functions return (not that anyone has claimed anything else).
>
> This is related to the problem of the port's private data being
> accessed after it is deallocated. The only way that can happen is if
> the tty layer calls the subdriver after the private data structure is
> freed -- and you said above that this does happen.
>
> But if change things so that the structure isn't freed until after the
> port is unregistered from the tty layer, this would mean that the tty
> layer is trying to do stuff to an unregistered port. That would be a
> bug in the tty layer.
>
> I'm not saying such bugs don't exist. However, if they do exist then
> the tty layer needs to be fixed, not the usb-serial layer.
ISTM the usb_serial_port private data should not be freed until
port_release().
If usb traffic isn't halted until port_release() ...
static void port_release(struct device *dev)
{
struct usb_serial_port *port = to_usb_serial_port(dev);
int i;
dev_dbg(dev, "%s\n", __func__);
/*
* Stop all the traffic before cancelling the work, so that
* nobody will restart it by calling usb_serial_port_softint.
*/
> kill_traffic(port);
cancel_work_sync(&port->work);
then what happens here if ->port_remove() has already been called?
static void garmin_write_bulk_callback(struct urb *urb)
{
struct usb_serial_port *port = urb->context;
if (port) {
struct garmin_data *garmin_data_p =
usb_get_serial_port_data(port);
if (GARMIN_LAYERID_APPL == getLayerId(urb->transfer_buffer)) {
> if (garmin_data_p->mode == MODE_GARMIN_SERIAL) {
gsp_send_ack(garmin_data_p,
((__u8 *)urb->transfer_buffer)[4]);
}
--
To unsubscribe from
Re: [PATCH 2/4] TTY: fix DTR not being dropped on hang up
On Wed, 2013-02-20 at 17:02 +0100, Johan Hovold wrote:
> Move HUPCL handling to port shutdown so that DTR is dropped also on hang
> up (tty_port_close is a noop for hung-up ports).
>
> Also do not try to drop DTR for uninitialised ports where it has never
> been raised (e.g. after a failed open).
>
> Nine drivers currently call tty_port_close_start directly (rather than
> through tty_port_close) and seven of them lower DTR as part of their
> close (if the port has been initialised). Fixup the remaining two
> drivers so that it continues to be lowered also on normal (non-HUP)
> close. [ Note that most of those other seven drivers did not expect DTR
> to have been dropped by tty_port_close_start in the first place. ]
>
> Signed-off-by: Johan Hovold
> ---
> drivers/tty/mxser.c| 4
> drivers/tty/n_gsm.c| 4
> drivers/tty/tty_port.c | 23 +--
> 3 files changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
> index 484b6a3..c547887 100644
> --- a/drivers/tty/mxser.c
> +++ b/drivers/tty/mxser.c
> @@ -1084,6 +1084,10 @@ static void mxser_close(struct tty_struct *tty, struct
> file *filp)
> mutex_lock(&port->mutex);
> mxser_close_port(port);
> mxser_flush_buffer(tty);
> + if (test_bit(ASYNCB_INITIALIZED, &port->flags)) {
> + if (tty->termios.c_cflag & HUPCL)
> + tty_port_lower_dtr_rts(port);
> + }
> mxser_shutdown_port(port);
> clear_bit(ASYNCB_INITIALIZED, &port->flags);
> mutex_unlock(&port->mutex);
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 4a43ef5d7..049013e 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2968,6 +2968,10 @@ static void gsmtty_close(struct tty_struct *tty,
> struct file *filp)
> if (tty_port_close_start(&dlci->port, tty, filp) == 0)
> goto out;
> gsm_dlci_begin_close(dlci);
> + if (test_bit(ASYNCB_INITIALIZED, &dlci->port.flags)) {
> + if (tty->termios.c_cflag & HUPCL)
> + tty_port_lower_dtr_rts(&dlci->port);
> + }
> tty_port_close_end(&dlci->port, tty);
> tty_port_tty_set(&dlci->port, NULL);
> out:
> diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> index 57a061e..506f579 100644
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -198,15 +198,25 @@ EXPORT_SYMBOL(tty_port_tty_set);
>
> static void tty_port_shutdown(struct tty_port *port)
> {
> + struct tty_struct *tty = tty_port_tty_get(port);
> +
I know I said this was done, but if this hasn't been pushed into
tty-next yet, I'd rather this was:
static void tty_port_shutdown(struct tty_port *port,
struct tty_struct *tty)
and not get the port reference. Both call sites already ensure that
there is a kref on the tty or tty is NULL.
(I ended up re-looking at this patch to double-check that it won't drop
DTR for the console, which it doesn't so that is good).
> mutex_lock(&port->mutex);
> if (port->console)
> goto out;
>
> if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
> + /*
> + * Drop DTR/RTS if HUPCL is set. This causes any attached
> + * modem to hang up the line.
> + */
> + if (!tty || tty->termios.c_cflag & HUPCL)
> + tty_port_lower_dtr_rts(port);
> +
> if (port->ops->shutdown)
> port->ops->shutdown(port);
> }
> out:
> + tty_kref_put(tty);
> mutex_unlock(&port->mutex);
> }
>
> @@ -225,15 +235,13 @@ void tty_port_hangup(struct tty_port *port)
> spin_lock_irqsave(&port->lock, flags);
> port->count = 0;
> port->flags &= ~ASYNC_NORMAL_ACTIVE;
> - if (port->tty) {
> + if (port->tty)
> set_bit(TTY_IO_ERROR, &port->tty->flags);
> - tty_kref_put(port->tty);
> - }
> - port->tty = NULL;
> spin_unlock_irqrestore(&port->lock, flags);
> + tty_port_shutdown(port);
> + tty_port_tty_set(port, NULL);
> wake_up_interruptible(&port->open_wait);
> wake_up_interruptible(&port->delta_msr_wait);
> - tty_port_shutdown(port);
> }
> EXPORT_SYMBOL(tty_port_hangup);
>
> @@ -452,11 +460,6 @@ int tty_port_close_start(struct tty_port *port,
> /* Flush the ldisc buffering */
> tty_ldisc_flush(tty);
>
> - /* Drop DTR/RTS if HUPCL is set. This causes any attached modem to
> -hang up the line */
> - if (tty->termios.c_cflag & HUPCL)
> - tty_port_lower_dtr_rts(port);
> -
> /* Don't call port->drop for the last reference. Callers will want
> to drop the last active reference in ->shutdown() or the tty
> shutdown path */
--
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.ke
[PATCH] usb/serial: Remove unnecessary check for console
The tty port ops shutdown() routine is not called for console ports;
remove extra check.
Signed-off-by: Peter Hurley
---
drivers/usb/serial/usb-serial.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index a19ed74..8424478 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -256,22 +256,18 @@ static int serial_open(struct tty_struct *tty, struct
file *filp)
* serial_down - shut down hardware
* @tport: tty port to shut down
*
- * Shut down a USB serial port unless it is the console. We never
- * shut down the console hardware as it will always be in use. Serialized
- * against activate by the tport mutex and kept to matching open/close pairs
+ * Shut down a USB serial port. Serialized against activate by the
+ * tport mutex and kept to matching open/close pairs
* of calls by the ASYNCB_INITIALIZED flag.
+ *
+ * Not called if tty is console.
*/
static void serial_down(struct tty_port *tport)
{
struct usb_serial_port *port =
container_of(tport, struct usb_serial_port, port);
struct usb_serial_driver *drv = port->serial->type;
- /*
-* The console is magical. Do not hang up the console hardware
-* or there will be tears.
-*/
- if (port->port.console)
- return;
+
if (drv->close)
drv->close(port);
}
--
1.8.1.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: [PATCH v2 0/4] TTY: port hangup and close fixes
On Tue, 2013-02-26 at 12:14 +0100, Johan Hovold wrote: > These patches against tty-next fix a few issues with tty-port hangup and > close. > > The first and third patch are essentially clean ups. > > The second patch makes sure DTR is dropped also on hangup and that DTR > is only dropped for initialised ports (where is could have been raised > in the first place). > > The fourth and final patch, make sure no tty callbacks are made from > tty_port_close_start when the port has not been initialised (successfully > opened). This was previously only done for wait_until_sent but there's > no reason to call flush_buffer or to honour port drain delay either. > The latter could cause a failed open to stall for up to two seconds. > > As a side-effect, these patches also fix an issue in USB-serial where we could > get tty-port callbacks on an uninitialised port after having hung up and > unregistered a device after disconnect. > > Johan > > > v2: > - reuse tty reference from hangup and close in shutdown. Both call sites >guarantee tty is either NULL or has a kref. Great, thank you. Peter -- 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: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes]
On Tue, 2013-03-05 at 17:02 +0100, Jiri Slaby wrote:
> On 02/28/2013 09:57 PM, Peter Hurley wrote:
> > Hi Jiri,
> >
> > Just wanted to make sure you saw this series.
>
> Hi, thanks for letting me know. Johan, care to CC Alan Cox and me (or at
> least LKML) when you're changing the TTY core next time?
Alan asked to be dropped.
On Wed, 2013-02-13 at 17:36 +, Alan Cox wrote:
> [ Note that this closing of an uninitialised port seems to be a bug in
> > itself, which these patches aim to fix. ]
>
> You don't want to be cc'ing me on these - not my problem any more.
> I have a couple of questions for 2/4:
>
> > Move HUPCL handling to port shutdown so that DTR is dropped also on
> > hang up (tty_port_close is a noop for hung-up ports).
>
> It makes sense, but I'm not sure -- is this expected, i.e. does this
> conform to standards and/or BSDs?
This is the existing behavior of the serial core.
uart_hangup() -> uart_shutdown()
static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
{
struct uart_port *uport = state->uart_port;
struct tty_port *port = &state->port;
/*
* Set the TTY IO error marker
*/
if (tty)
set_bit(TTY_IO_ERROR, &tty->flags);
if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
/*
* Turn off DTR and RTS early.
*/
if (!tty || (tty->termios.c_cflag & HUPCL))
> uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
uart_port_shutdown(port);
}
> > @@ -196,13 +196,20 @@ void tty_port_tty_set(struct tty_port *port, struct
> > tty_struct *tty)
> > }
> > EXPORT_SYMBOL(tty_port_tty_set);
>
> -static void tty_port_shutdown(struct tty_port *port)
> +static void tty_port_shutdown(struct tty_port *port, struct tty_struct
> *tty)
> {
> mutex_lock(&port->mutex);
> if (port->console)
> goto out;
>
> if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
> + /*
> +* Drop DTR/RTS if HUPCL is set. This causes any attached
> +* modem to hang up the line.
> +*/
[See my comment below]
> if (!tty || tty->termios.c_cflag & HUPCL)
> > + tty_port_lower_dtr_rts(port);
> > +
>
> So you drop the line even thought the user didn't necessarily want to,
> in case the tty is gone already?
>
> > @@ -225,15 +232,13 @@ void tty_port_hangup(struct tty_port *port)
> spin_lock_irqsave(&port->lock, flags);
> port->count = 0;
> port->flags &= ~ASYNC_NORMAL_ACTIVE;
> - if (port->tty) {
> + if (port->tty)
> set_bit(TTY_IO_ERROR, &port->tty->flags);
> - tty_kref_put(port->tty);
> - }
> - port->tty = NULL;
> spin_unlock_irqrestore(&port->lock, flags);
> > + tty_port_shutdown(port, port->tty);
>
> What prevents port->tty to be NULL here already?
Nothing. That's why it's tested in tty_port_shutdown() above.
> > + tty_port_tty_set(port, NULL);
> > wake_up_interruptible(&port->open_wait);
> > wake_up_interruptible(&port->delta_msr_wait);
> > - tty_port_shutdown(port);
>
> Did you investigate if the order matters here? I don't know, just curious...
I did.
It makes sense to wake blocked opens only _after_ the port has been
shutdown. This way any blocked opens wake up, discover the port has been
shutdown and exit their wait loops in xx_block_til_ready().
Similarly for delta_msr_wait. Although I think the delta_msr_wait loop
is already unsafe (or at least not robust).
> > @@ -452,11 +457,6 @@ int tty_port_close_start(struct tty_port *port,
> /* Flush the ldisc buffering */
> tty_ldisc_flush(tty);
>
> - /* Drop DTR/RTS if HUPCL is set. This causes any attached modem to
> - hang up the line */
> - if (tty->termios.c_cflag & HUPCL)
> - tty_port_lower_dtr_rts(port);
> -
> /* Don't call port->drop for the last reference. Callers will want
>to drop the last active reference in ->shutdown() or the tty
> >shutdown path */
>
> > Forwarded Message
> > From: Johan Hovold
> > To: Greg KH
> > Cc: Alan Stern , linux-usb@vger.kernel.org,
> > linux-ser...@vger.kernel.org, Peter Hurley ,
> > Johan Hovold
> > Subject: [PATCH v2 0/4] TTY: port hangup an
Re: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes]
On Tue, 2013-03-05 at 22:56 +0100, Jiri Slaby wrote:
> On 03/05/2013 06:06 PM, Peter Hurley wrote:
> >>> @@ -225,15 +232,13 @@ void tty_port_hangup(struct tty_port *port)
> >> spin_lock_irqsave(&port->lock, flags);
> >> port->count = 0;
> >> port->flags &= ~ASYNC_NORMAL_ACTIVE;
> >> - if (port->tty) {
> >> + if (port->tty)
> >> set_bit(TTY_IO_ERROR, &port->tty->flags);
> >> - tty_kref_put(port->tty);
> >> - }
> >> - port->tty = NULL;
> >> spin_unlock_irqrestore(&port->lock, flags);
> >>> + tty_port_shutdown(port, port->tty);
> >>
> >> What prevents port->tty to be NULL here already?
> >
> > Nothing. That's why it's tested in tty_port_shutdown() above.
>
> I know :).
Sorry :)
> But the question is rather don't we want to pass the real
> refcounted port->tty (take a snapshot inside the lock) instead?
I think that's why he moved the kref release to after the shutdown (via
tty_port_set_tty()) -- but I'm tired and maybe I'm missing something
here?
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: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes]
On Tue, 2013-03-05 at 23:10 +0100, Jiri Slaby wrote:
> On 03/05/2013 11:02 PM, Peter Hurley wrote:
> > On Tue, 2013-03-05 at 22:56 +0100, Jiri Slaby wrote:
> >> On 03/05/2013 06:06 PM, Peter Hurley wrote:
> >>>>> @@ -225,15 +232,13 @@ void tty_port_hangup(struct tty_port *port)
> >>>> spin_lock_irqsave(&port->lock, flags);
> >>>> port->count = 0;
> >>>> port->flags &= ~ASYNC_NORMAL_ACTIVE;
> >>>> - if (port->tty) {
> >>>> + if (port->tty)
> >>>> set_bit(TTY_IO_ERROR, &port->tty->flags);
> >>>> - tty_kref_put(port->tty);
> >>>> - }
> >>>> - port->tty = NULL;
> >>>> spin_unlock_irqrestore(&port->lock, flags);
> >>>>> + tty_port_shutdown(port, port->tty);
> >>>>
> >>>> What prevents port->tty to be NULL here already?
> >>>
> >>> Nothing. That's why it's tested in tty_port_shutdown() above.
> >>
> >> I know :).
> >
> > Sorry :)
> >
> >> But the question is rather don't we want to pass the real
> >> refcounted port->tty (take a snapshot inside the lock) instead?
> >
> > I think that's why he moved the kref release to after the shutdown (via
> > tty_port_set_tty()) -- but I'm tired and maybe I'm missing something
> > here?
>
> port->tty can be changed right after the unlock.
Right. My bad. Thanks for catching this.
> So I'm thinking about
> something like this:
>
> if (port->tty)
>set_bit(TTY_IO_ERROR, &port->tty->flags);
> tty = port->tty; <=== take a snapshot
> spin_unlock_irqrestore(&port->lock, flags);
> tty_port_shutdown(port, tty); <=== use the snapshot
> set_tty_port(port, NULL); <=== put kref on that tty
Yeah, that's better.
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 v3 00/23] ldisc fixes
[now this discussion has turned to usb gadget +cc Felipe Balbi, linux-usb, -cc Dave Jones, Ilya Zykov] On Tue, 2013-03-05 at 23:39 +0100, Jiri Slaby wrote: > On 03/05/2013 11:20 PM, Peter Hurley wrote: > > [--cc Alan Cox] > > > > On Tue, 2013-03-05 at 21:50 +0100, Sebastian Andrzej Siewior wrote: > >> * Peter Hurley | 2013-02-05 15:20:15 [-0500]: > >> > >>> Please re-test with your dummy_hcd/g_nokia testcase, although > >>> I'm not convinced that usb gadget is using tty_hangup() appropriately. > >>> tty drivers use this for async carrier loss coming from an IRQ > >>> which will be disabled if the tty has been shutdown. Does gserial > >>> prevent async hangup to a dead tty in a similar fashion? > >> > >> Not sure I understood. tty_hangup() is only called from within > >> gserial_disconnect() which calls right after usb_ep_disable(). After > >> usb_ep_disable() no further serial packets can be received until the > >> endpoints are re-enabled. This happens in gserial_connect(). > > > > That's why I asked. There are two potential issues: > > > > First, tty_hangup() is asynchronous -- ie., it returns immediately. It > > does not wait for the tty device to actually perform the hangup. So if > > the gadget layers start cleanup immediately after, expecting that they > > won't get a flurry of tty calls, that would be bad. > > Sorry, I missed what driver is this? g_serial. drivers/usb/gadget/u_serial.c > > tty_vhangup() is synchronous -- ie., you wait while it cleans up. This > > is what the usb serial core does on it's disconnect() method. But I > > didn't research further if the circumstances were the same. > > Even when tty_vhangup returns, it does not guarantee a closed tty. And > it also does not guarantee that any of tty->ops won't be called. The > latter is true only for devices that can be consoles. (For those, > file->ops are not redirected.) In that case one needs to wait for > port->count to become 0. Perhaps I was oversimplifying. But my point was I doubt usb gadget is conducting its teardown safely wrt tty. 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: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes]
On Wed, 2013-03-06 at 17:52 +0100, Johan Hovold wrote:
> > > @@ -225,15 +232,13 @@ void tty_port_hangup(struct tty_port *port)
> > spin_lock_irqsave(&port->lock, flags);
> > port->count = 0;
> > port->flags &= ~ASYNC_NORMAL_ACTIVE;
> > - if (port->tty) {
> > + if (port->tty)
> > set_bit(TTY_IO_ERROR, &port->tty->flags);
> > - tty_kref_put(port->tty);
> > - }
> > - port->tty = NULL;
> > spin_unlock_irqrestore(&port->lock, flags);
> > > + tty_port_shutdown(port, port->tty);
> >
> > What prevents port->tty to be NULL here already?
>
> Nothing, I'll get a new reference within the port lock section as you
> just suggested elsewhere in this thread.
Don't do that. Steal the tty and put the kref after like this:
void tty_port_hangup(struct tty_port *port)
{
struct tty_struct *tty;
unsigned long flags;
spin_lock_irqsave(&port->lock, flags);
port->count = 0;
port->flags &= ~ASYNC_NORMAL_ACTIVE;
tty = port->tty;
port->tty = NULL;
if (tty)
set_bit(TTY_IO_ERROR, &tty->flags);
spin_unlock_irqrestore(&port->lock, flags);
tty_port_shutdown(port, tty);
tty_kref_put(tty);
wake_up_interruptible(&port->open_wait);
wake_up_interruptible(&port->delta_msr_wait);
}
> Yes, I did. First, the order should not matter for blocked opens as they
> will exit their wait loops based on tty_hung_up_p(filp) either way.
Only if the open() was ever successful, otherwise the filp won't be in
the tty->tty_files list. That's why the blocking opens also check
ASYNC_INITIALIZED (or ASYNCB_INITIALIZED depending on which they use).
Which is why I said it was actually better to shutdown() first, then
wake up the blocked opens.
> As for delta_msr_wait the changed order is actually preferred as it
> allows the waiting process to return based on ASYNC_INITIALIZED. This is
> also the order used by serial_core. Note however that the current
> serial_core TIOCMIWAIT is broken in that it doesn't return on hangups at
> all.
>
> Perhaps I should separate this to a patch of its own, and send a fix
> for serial_core TIOCMIWAIT as well.
uart_wait_modem_status() is what I was referring to and should be fixed.
Patches always welcome.
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: [Fwd: [PATCH v2 0/4] TTY: port hangup and close fixes]
On Thu, 2013-03-07 at 10:43 +0100, Johan Hovold wrote: > On Wed, Mar 06, 2013 at 02:14:56PM -0500, Peter Hurley wrote: > > On Wed, 2013-03-06 at 17:52 +0100, Johan Hovold wrote: > > > Yes, I did. First, the order should not matter for blocked opens as they > > > will exit their wait loops based on tty_hung_up_p(filp) either way. > > > > Only if the open() was ever successful, otherwise the filp won't be in > > the tty->tty_files list. That's why the blocking opens also check > > ASYNC_INITIALIZED (or ASYNCB_INITIALIZED depending on which they use). > > Which is why I said it was actually better to shutdown() first, then > > wake up the blocked opens. > > ASYNC_INITIALIZED have also been cleared when the blocked opens are > being woken up from tty_port_close_end. > > And the filp is added to tty_files before open() is called: > > ===>tty_add_file(tty, filp); > > check_tty_count(tty, __func__); > if (tty->driver->type == TTY_DRIVER_TYPE_PTY && > tty->driver->subtype == PTY_TYPE_MASTER) > noctty = 1; > #ifdef TTY_DEBUG_HANGUP > printk(KERN_DEBUG "%s: opening %s...\n", __func__, tty->name); > #endif > if (tty->ops->open) > ===> retval = tty->ops->open(tty, filp); > > so a blocked open must have hung_up_tty_fops when woken up from hangup, > right? You're right, my mistake. > Either way, postponing wake-up somewhat in tty_port_hangup should be > fine. Yep. 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: 3.9.0-rc1+: irq 16: nobody cared
[ +linux-usb, linux-pci] This is the 2nd report today for unhandled interrupts with usb hcd. On Fri, 2013-03-08 at 08:37 +0100, Thomas Meyer wrote: > [1.883369] input: C-Media Electronics Inc. USB Multimedia Audio Device as > /devices/pci:00/:00:1d.7/usb2/2-1/2-1.2/2-1.2:1.3/input/input7 > [1.883668] hid-generic 0003:0D8C:0105.0001: input,hidraw0: USB HID v1.00 > Device [C-Media Electronics Inc. USB Multimedia Audio Device] on > usb-:00:1d.7-1.2/input3 > [1.950222] usb 2-1.3: new high-speed USB device number 7 using ehci-pci > [1.969193] irq 16: nobody cared (try booting with the "irqpoll" option) > [1.969254] Pid: 0, comm: swapper Not tainted 3.9.0-rc1+ #24 > [1.969256] Call Trace: > [1.969258][] __report_bad_irq.isra.6+0x24/0xb0 > [1.969272] [] note_interrupt+0x1a3/0x1f0 > [1.969276] [] handle_irq_event_percpu+0x89/0x160 > [1.969281] [] ? ktime_get+0x4d/0xd0 > [1.969286] [] ? intel_pstate_timer_func+0x370/0x370 > [1.969290] [] handle_irq_event+0x23/0x40 > [1.969294] [] handle_fasteoi_irq+0x46/0xd0 > [1.969299] [] handle_irq+0x1d/0x30 > [1.969303] [] do_IRQ+0x52/0xd0 > [1.969309] [] common_interrupt+0x68/0x68 > [1.969311][] ? cpuidle_wrap_enter+0x48/0x90 > [1.969318] [] ? cpuidle_wrap_enter+0x44/0x90 > [1.969323] [] cpuidle_enter_tk+0x10/0x20 > [1.969326] [] cpuidle_idle_call+0x7a/0x100 > [1.969331] [] cpu_idle+0x85/0xe0 > [1.969336] [] rest_init+0x62/0x70 > [1.969340] [] start_kernel+0x38b/0x398 > [1.969344] [] ? repair_env_string+0x5e/0x5e > [1.969348] [] x86_64_start_reservations+0x2a/0x2c > [1.969351] [] x86_64_start_kernel+0xce/0xd2 > [1.969353] handlers: > [1.969377] [] usb_hcd_irq > [1.969416] [] usb_hcd_irq > [1.969452] Disabling IRQ #16 > [2.047980] usb 2-1.3: New USB device found, idVendor=0b95, idProduct=772a > [2.047984] usb 2-1.3: New USB device strings: Mfr=1, Product=2, > SerialNumber=3 > [2.047988] usb 2-1.3: Product: AX88x72A > [2.047991] usb 2-1.3: Manufacturer: ASIX Elec. Corp. > [2.047994] usb 2-1.3: SerialNumber: 555C7C -- 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: [3.9-rc1] irq 16: nobody cared (was [3.9-rc1] very poor interrupt responses)
[ +linux-pci, +linux-acpi, +Rafael Wysocki, +Bjorn Helgaas ] On Sat, 2013-03-09 at 09:53 +0100, Thomas Meyer wrote: > Am Freitag, den 08.03.2013, 21:19 -0500 schrieb Alan Stern: > > On Fri, 8 Mar 2013, Peter Hurley wrote: > > > > > [ +linux-usb ] > > > > > > On Fri, 2013-03-08 at 14:12 -0500, Shawn Starr wrote: > > > > Hello folks, > > > > > > > > I am noticing since rc0 and now rc1, very poor interrupt handling. > > > > Keyboard response, mouse movements, display refreshing etc. General > > > > input/display sluggishness. Did something break IRQ handling somewhere? > > > > I need to validate if this happens with X not running also if it is > > > > i915 related somehow. The behavor is noticed in a console login however. > > > > > > > > Device: Lenovo W500 laptop > > > > > > Hi Shawn, > > > > > > Unhandled interrupts is the problem. > > > > > > Is the device below being id'd properly? > > > If you remove this device, does the problem go away? > > > > Does either of the kernels in question have commit 0f815a0a700b (USB: > > UHCI: fix IRQ race during initialization)? That commit was added to > > fix precisely this sort of thing. > > I think so: > > $ git describe > v3.9-rc1-211-g47b3bc9 > > $ git branch --contains 0f815a0a700b > * master This might not be caused by USB. There were a lot of changes to PCI and ACPI for 3.9. Probably best to each file a bug at bugzilla.kernel.org with: Last known good kernel version -- For both good and bad kernels (preferably as attachments) -- /proc/interrupts lsusb lspci dmesg and reply back with the bugzilla #. It may be necessary to bisect this problem. Regards, Peter Hurley PS - I know it can be difficult to get those things on the bad kernel. It's easier if you boot to console. -- 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/2] Revert "Revert "HID: Fix logitech-dj: missing Unifying device issue""
[ +cc Sarah Sharp, linux-usb ]
On 07/18/2013 09:21 AM, Nestor Lopez Casado wrote:
This reverts commit 8af6c08830b1ae114d1a8b548b1f8b056e068887.
This patch re-adds the workaround introduced by 596264082f10dd4
which was reverted by 8af6c08830b1ae114.
The original patch 596264 was needed to overcome a situation where
the hid-core would drop incoming reports while probe() was being
executed.
This issue was solved by c849a6143bec520af which added
hid_device_io_start() and hid_device_io_stop() that enable a specific
hid driver to opt-in for input reports while its probe() is being
executed.
Commit a9dd22b730857347 modified hid-logitech-dj so as to use the
functionality added to hid-core. Having done that, workaround 596264
was no longer necessary and was reverted by 8af6c08.
We now encounter a different problem that ends up 'again' thwarting
the Unifying receiver enumeration. The problem is time and usb controller
dependent. Ocasionally the reports sent to the usb receiver to start
the paired devices enumeration fail with -EPIPE and the receiver never
gets to enumerate the paired devices.
With dcd9006b1b053c7b1c the problem was "hidden" as the call to the usb
driver became asynchronous and none was catching the error from the
failing URB.
As the root cause for this failing SET_REPORT is not understood yet,
-possibly a race on the usb controller drivers or a problem with the
Unifying receiver- reintroducing this workaround solves the problem.
Before we revert to using the workaround, I'd like to suggest that
this new "hidden" problem may be an interaction with the xhci_hcd host
controller driver only.
Looking at the related bug, the OP indicates the machine only has
USB3 ports. Additionally, comments #7, #100, and #104 of the original
bug report [1] add additional information that would seem to confirm
this suspicion.
Let me add I have this USB device running on the uhci_hcd driver
with or without this workaround on v3.10.
Overall what this workaround does is: If an input report from an
unknown device is received, then a (re)enumeration is performed.
related bug:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1194649
I thought I saw someone reporting this problem recently on LKML;
where is the Reported-by so that Sarah can follow-up with the
reporter directly, if desired?
Regards,
Peter Hurley
[1]
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1039143
Signed-off-by: Nestor Lopez Casado
---
drivers/hid/hid-logitech-dj.c | 45 +
drivers/hid/hid-logitech-dj.h |1 +
2 files changed, 46 insertions(+)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index db3192b..0d13389 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -192,6 +192,7 @@ static struct hid_ll_driver logi_dj_ll_driver;
static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
size_t count,
unsigned char report_type);
+static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev
*djrcv_dev);
static void logi_dj_recv_destroy_djhid_device(struct dj_receiver_dev
*djrcv_dev,
struct dj_report *dj_report)
@@ -232,6 +233,7 @@ static void logi_dj_recv_add_djhid_device(struct
dj_receiver_dev *djrcv_dev,
if (dj_report->report_params[DEVICE_PAIRED_PARAM_SPFUNCTION] &
SPFUNCTION_DEVICE_LIST_EMPTY) {
dbg_hid("%s: device list is empty\n", __func__);
+ djrcv_dev->querying_devices = false;
return;
}
@@ -242,6 +244,12 @@ static void logi_dj_recv_add_djhid_device(struct
dj_receiver_dev *djrcv_dev,
return;
}
+ if (djrcv_dev->paired_dj_devices[dj_report->device_index]) {
+ /* The device is already known. No need to reallocate it. */
+ dbg_hid("%s: device is already known\n", __func__);
+ return;
+ }
+
dj_hiddev = hid_allocate_device();
if (IS_ERR(dj_hiddev)) {
dev_err(&djrcv_hdev->dev, "%s: hid_allocate_device failed\n",
@@ -305,6 +313,7 @@ static void delayedwork_callback(struct work_struct *work)
struct dj_report dj_report;
unsigned long flags;
int count;
+ int retval;
dbg_hid("%s\n", __func__);
@@ -337,6 +346,25 @@ static void delayedwork_callback(struct work_struct *work)
logi_dj_recv_destroy_djhid_device(djrcv_dev, &dj_report);
break;
default:
+ /* A normal report (i. e. not belonging to a pair/unpair notification)
+* arriving here, means that the report arrived but we did not have a
+* paired dj_device associated to the report's device_index, this
+* means that the or
Re: [PATCH 1/2] Revert "Revert "HID: Fix logitech-dj: missing Unifying device issue""
On 07/18/2013 06:09 PM, Sarah Sharp wrote: On Thu, Jul 18, 2013 at 04:28:01PM -0400, Peter Hurley wrote: [ +cc Sarah Sharp, linux-usb ] On 07/18/2013 09:21 AM, Nestor Lopez Casado wrote: This reverts commit 8af6c08830b1ae114d1a8b548b1f8b056e068887. This patch re-adds the workaround introduced by 596264082f10dd4 which was reverted by 8af6c08830b1ae114. The original patch 596264 was needed to overcome a situation where the hid-core would drop incoming reports while probe() was being executed. This issue was solved by c849a6143bec520af which added hid_device_io_start() and hid_device_io_stop() that enable a specific hid driver to opt-in for input reports while its probe() is being executed. Commit a9dd22b730857347 modified hid-logitech-dj so as to use the functionality added to hid-core. Having done that, workaround 596264 was no longer necessary and was reverted by 8af6c08. We now encounter a different problem that ends up 'again' thwarting the Unifying receiver enumeration. The problem is time and usb controller dependent. Ocasionally the reports sent to the usb receiver to start the paired devices enumeration fail with -EPIPE and the receiver never gets to enumerate the paired devices. With dcd9006b1b053c7b1c the problem was "hidden" as the call to the usb driver became asynchronous and none was catching the error from the failing URB. As the root cause for this failing SET_REPORT is not understood yet, -possibly a race on the usb controller drivers or a problem with the Unifying receiver- reintroducing this workaround solves the problem. Before we revert to using the workaround, I'd like to suggest that this new "hidden" problem may be an interaction with the xhci_hcd host controller driver only. Looking at the related bug, the OP indicates the machine only has USB3 ports. Additionally, comments #7, #100, and #104 of the original bug report [1] add additional information that would seem to confirm this suspicion. Question: does this USB device need a control transfer to reset its endpoints when the endpoints are not actually halted? If so, yes, that is a known xHCI driver bug that needs to be fixed. The xHCI host will not accept a Reset Endpoint command when the endpoints are not actually halted, but the USB core will send the control transfer to reset the endpoint. That means the device and host toggles will be out of sync, and all messages will start to fail with -EPIPE. Can the OP capture a usbmon trace when the device starts failing? That will reveal whether this actually is the issue. dmesg output with CONFIG_USB_DEBUG and CONFIG_USB_XHCI_HCD_DEBUGGING turned on would also be helpful. Sarah, I forwarded your usbmon capture request to the OP in the bug report (I don't have an email address for the reporter). As far as getting printk output from a custom kernel, I think that may be beyond the reporter's capability. Perhaps one of the Ubuntu devs triaging this bug could provide a test kernel for the OP with those options on. Joseph, would you be willing to do that? 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 1/2] Revert "Revert "HID: Fix logitech-dj: missing Unifying device issue""
On 07/22/2013 07:44 AM, Jiri Kosina wrote: On Fri, 19 Jul 2013, Peter Hurley wrote: As far as getting printk output from a custom kernel, I think that may be beyond the reporter's capability. Perhaps one of the Ubuntu devs triaging this bug could provide a test kernel for the OP with those options on. Joseph, would you be willing to do that? Sure thing. I'll build a kernel and request that the bug reporter collect usbmon data. Thanks Joseph for building the test kernel and getting it to the reporter! Sarah, I've attached the dmesg capture supplied by the original reporter on a 3.10 custom kernel w/ the kbuild options you requested. It seems as if your initial suspicion is correct: [ 46.785490] xhci_hcd :00:14.0: Endpoint 0x81 not halted, refusing to reset. [ 46.785493] xhci_hcd :00:14.0: Endpoint 0x82 not halted, refusing to reset. [ 46.785496] xhci_hcd :00:14.0: Endpoint 0x83 not halted, refusing to reset. [ 46.785952] xhci_hcd :00:14.0: Waiting for status stage event At this point, would you recommend proceeding with the workaround or waiting for an xHCI bug fix? Thanks for your efforts. Seems like this might be a part of the picture, but not a complete one. Linus claims to have similar problem, but his receiver is not connected through xHCI (I got this as an off-list report, so can't really provide a pointer to ML archives). Ah, ok. I wasn't aware of that. I'll assume then that the necessary people have the complete picture. 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 v2 5/9] usb: don't use PREPARE_DELAYED_WORK
On 02/22/2014 10:14 AM, Alan Stern wrote:
On Sat, 22 Feb 2014, Tejun Heo wrote:
Hello,
If this is actually safe, let's do it from the get-go.
Thanks!
--- 8< ---
PREPARE_[DELAYED_]WORK() are being phased out. They have few users
and a nasty surprise in terms of reentrancy guarantee as workqueue
considers work items to be different if they don't have the same work
function.
usb_hub->init_work is multiplexed with multiple work functions;
however, the work item is never queued while in-flight, so we can
simply use INIT_DELAYED_WORK() before each queueing.
It would probably be best to route this with other related updates
through the workqueue tree.
Lightly tested.
v2: Greg and Alan confirm that the work item is never queued while
in-flight. Simply use INIT_DELAYED_WORK().
Signed-off-by: Tejun Heo
Cc: Greg Kroah-Hartman
Cc: Alan Stern
Cc: linux-usb@vger.kernel.org
---
drivers/usb/core/hub.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1040,7 +1040,7 @@ static void hub_activate(struct usb_hub
*/
if (type == HUB_INIT) {
delay = hub_power_on(hub, false);
- PREPARE_DELAYED_WORK(&hub->init_work, hub_init_func2);
+ INIT_DELAYED_WORK(&hub->init_work, hub_init_func2);
schedule_delayed_work(&hub->init_work,
msecs_to_jiffies(delay));
@@ -1194,7 +1194,7 @@ static void hub_activate(struct usb_hub
/* Don't do a long sleep inside a workqueue routine */
if (type == HUB_INIT2) {
- PREPARE_DELAYED_WORK(&hub->init_work, hub_init_func3);
+ INIT_DELAYED_WORK(&hub->init_work, hub_init_func3);
schedule_delayed_work(&hub->init_work,
msecs_to_jiffies(delay));
return; /* Continues at init3: below */
This should work okay. But while you're making these changes, you
should remove the INIT_DELAYED_WORK(&hub->init_work, NULL) call in
hub_probe(). It is now unnecessary.
Is the cancel_delayed_work_sync(&hub->init_work) call in hub_quiesce()
going to get confused by all this?
It's worth mentioning that the only reason for the hub_init_func3 stuff
is, as the comment says, to avoid a long sleep (100 ms) inside a work
routine.
If a running hub init does not need to be single-threaded wrt
a different running hub init, then a single init work could be queued to
the system_unbound_wq which doesn't care about running times.
With all the changes to the work queue infrastructure, maybe
this doesn't matter so much any more. If we got rid of it then there
wouldn't be any multiplexing, and this whole issue would become moot.
--
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] u_serial.c - Force destroy tty_port in gserial_free_port
Hi Andre,
On 10/08/2014 11:54 PM, Andre Wolokita wrote:
> On 09/10/14 14:38, Greg KH wrote:
>> On Thu, Oct 09, 2014 at 02:08:04PM +1100, Andre Wolokita wrote:
>>> On 09/10/14 13:56, Greg KH wrote:
>>>> On Thu, Oct 09, 2014 at 11:23:59AM +1100, Andre Wolokita wrote:
>>>>> Issuing a modprobe -r g_serial command to the target
>>>>> over the gadget serial communications line causes
>>>>> modprobe to enter uninterruptable sleep, leaving the
>>>>> system in an unstable state.
>>>>>
>>>>> The associated tty_port.count won't drop to 0 because
>>>>> the command is issued over the very line being removed.
>>>>>
>>>>> Destroying the tty_port will ensure that resources are
>>>>> freed and modprobe will not hang.
>>>>>
>>>>> Signed-off-by: Andre Wolokita
>>>>> ---
>>>>> drivers/usb/gadget/function/u_serial.c |2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/usb/gadget/function/u_serial.c
>>>>> b/drivers/usb/gadget/function/u_serial.c
>>>>> index ad0aca8..db631c4 100644
>>>>> --- a/drivers/usb/gadget/function/u_serial.c
>>>>> +++ b/drivers/usb/gadget/function/u_serial.c
>>>>> @@ -1074,10 +1074,10 @@ static int gs_closed(struct gs_port *port)
>>>>> static void gserial_free_port(struct gs_port *port)
>>>>> {
>>>>> tasklet_kill(&port->push);
>>>>> + tty_port_destroy(&port->port);
>>>>> /* wait for old opens to finish */
>>>>> wait_event(port->port.close_wait, gs_closed(port));
>>>>
>>>> Isn't this now a "use-after-free" issue?
>>>>
>>>
>>> Are you referring to the subsequent call to wait event() on gs_closed()?
>>
>> Yes.
>>
>>> Testing the use-case with this patch applied seemed to work without any
>>> issues. The ttyGS0 reference in /dev/ is gone after running modprobe -r
>>> but I'm just a newbie, so I could be doing sometime horrible here.
>>
>> Hm, I dug into the tty core, it should be ok, but it just seems really
>> odd, and bad-form to be doing something with port->port after calling a
>> "destroy" function with it, don't you agree?
>
> I do. The call to wait_event() can be removed as we no longer care whether
> gs_closed(port) is returning true - if it even can, having destroyed the
> tty_port.
Neither the patch nor the idea that this wait_event() is unnecessary is
correct (within the current design of u_serial).
tty_port_destroy() frees the input processing buffers, which may be in use
right at this moment, if a port is still in use by its tty. Since whatever
was using those buffers doesn't know this has happened, it may still be writing
to that memory, which may now be reallocated to some other kernel subsystem,
like
file cache buffers.
The wait_event() tries to ensure that the port destruction can't take place
while the port is still in use, so when it's hung there, it's preventing bad
things from happening.
There is no way to simply 'force' the port to no longer be in use, since a
userspace process can maintain an open file indefinitely.
There are a couple of possible solutions though:
In gserial_free_line(), hangup the tty associated with the port. You can
look at usb_serial_disconnect() for how to do that properly. This doesn't
guarantee that the userspace process will close the tty, but most programs
will close the file on end-of-file read (which is what hangup will cause).
It doesn't look like making the wait_event() interruptible is possible
(or desirable).
The ideal solution would be for u_serial to reference count its gs_ports;
that's what usb serial does for its usb_serial_port. Then gserial_free_line()
becomes a 'disconnect', and the actual cleanup happens later. The key
implementation details are:
1. The tty core helps keep reference counting simple by calling the tty
driver's install() and cleanup() methods; install() for the first open() and
cleanup() when the tty is being released. usb serial does this with
usb_serial_port_get_by_minor() from serial_install() and usb_serial_put() in
serial_cleanup() and usb_serial_disconnect().
2. a flag (like usb serial's '->disconnected') to prevent continued port
allocation after 'disconnect'.
The key concept is that although the tty and port still exist, neither
does anything useful after the disconnect.
And u_serial.c should really be ported to using tty_port which takes care of
stuff like parallel opens and closes without looping and other stuff like
the port->openclose flag.
FWIW, the tty_unregister_device() does not need to be after gserial_free_port()
because existing ttys maintain a device reference count which prevents the
underlying tty device from being released.
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] u_serial.c - Force destroy tty_port in gserial_free_port
On 10/09/2014 10:26 AM, Alan Stern wrote: > On Thu, 9 Oct 2014, Andre Wolokita wrote: > >>>>> Isn't this now a "use-after-free" issue? >>>>> >>>> >>>> Are you referring to the subsequent call to wait event() on gs_closed()? >>> >>> Yes. >>> >>>> Testing the use-case with this patch applied seemed to work without any >>>> issues. The ttyGS0 reference in /dev/ is gone after running modprobe -r >>>> but I'm just a newbie, so I could be doing sometime horrible here. >>> >>> Hm, I dug into the tty core, it should be ok, but it just seems really >>> odd, and bad-form to be doing something with port->port after calling a >>> "destroy" function with it, don't you agree? >> >> I do. The call to wait_event() can be removed as we no longer care whether >> gs_closed(port) is returning true - if it even can, having destroyed the >> tty_port. > > Maybe you don't care whether gs_closed(port) returns true, but you > should care about whether gs_closed() crashes -- which it might well do > if it tries to access deallocated memory. > > Did you test your patch by unloading the module while there were > pending opens? gs_closed() won't crash because that's not the memory being deallocated. tty_port_destroy() is somewhat misnamed; should be more like tty_port_cleanup(), as it doesn't actually deallocate the port (because tty_port could be an embedded structure in a larger 'port' which is the most common usage). I've been struggling to understand how this patch could even cause the wait_event() to complete and the only thing I can hypothesize is that the deallocation in tty_port_destroy() somehow causes the process with the open tty to crash but I don't see how. Regards, Peter Hurley PS - My earlier email was greylisted so that's why I appeared to reply well after your answer :) X-Greylist: delayed 4693 seconds by postgrey-1.27 at vger.kernel.org; Thu, 09 Oct 2014 11:06:05 EDT -- 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] u_serial.c - Force destroy tty_port in gserial_free_port
On 10/10/2014 12:05 AM, Andre Wolokita wrote:
>
>
> On 10/10/14 00:47, Peter Hurley wrote:
>> Hi Andre,
>>
>> On 10/08/2014 11:54 PM, Andre Wolokita wrote:
>>> On 09/10/14 14:38, Greg KH wrote:
>>>> On Thu, Oct 09, 2014 at 02:08:04PM +1100, Andre Wolokita wrote:
>>>>> On 09/10/14 13:56, Greg KH wrote:
>>>>>> On Thu, Oct 09, 2014 at 11:23:59AM +1100, Andre Wolokita wrote:
>>>>>>> Issuing a modprobe -r g_serial command to the target
>>>>>>> over the gadget serial communications line causes
>>>>>>> modprobe to enter uninterruptable sleep, leaving the
>>>>>>> system in an unstable state.
>>>>>>>
>>>>>>> The associated tty_port.count won't drop to 0 because
>>>>>>> the command is issued over the very line being removed.
>>>>>>>
>>>>>>> Destroying the tty_port will ensure that resources are
>>>>>>> freed and modprobe will not hang.
>>>>>>>
>>>>>>> Signed-off-by: Andre Wolokita
>>>>>>> ---
>>>>>>> drivers/usb/gadget/function/u_serial.c |2 +-
>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/gadget/function/u_serial.c
>>>>>>> b/drivers/usb/gadget/function/u_serial.c
>>>>>>> index ad0aca8..db631c4 100644
>>>>>>> --- a/drivers/usb/gadget/function/u_serial.c
>>>>>>> +++ b/drivers/usb/gadget/function/u_serial.c
>>>>>>> @@ -1074,10 +1074,10 @@ static int gs_closed(struct gs_port *port)
>>>>>>> static void gserial_free_port(struct gs_port *port)
>>>>>>> {
>>>>>>> tasklet_kill(&port->push);
>>>>>>> + tty_port_destroy(&port->port);
>>>>>>> /* wait for old opens to finish */
>>>>>>> wait_event(port->port.close_wait, gs_closed(port));
>>>>>>
>>>>>> Isn't this now a "use-after-free" issue?
>>>>>>
>>>>>
>>>>> Are you referring to the subsequent call to wait event() on gs_closed()?
>>>>
>>>> Yes.
>>>>
>>>>> Testing the use-case with this patch applied seemed to work without any
>>>>> issues. The ttyGS0 reference in /dev/ is gone after running modprobe -r
>>>>> but I'm just a newbie, so I could be doing sometime horrible here.
>>>>
>>>> Hm, I dug into the tty core, it should be ok, but it just seems really
>>>> odd, and bad-form to be doing something with port->port after calling a
>>>> "destroy" function with it, don't you agree?
>>>
>>> I do. The call to wait_event() can be removed as we no longer care whether
>>> gs_closed(port) is returning true - if it even can, having destroyed the
>>> tty_port.
>>
>> Neither the patch nor the idea that this wait_event() is unnecessary is
>> correct (within the current design of u_serial).
>>
>> tty_port_destroy() frees the input processing buffers, which may be in use
>> right at this moment, if a port is still in use by its tty. Since whatever
>> was using those buffers doesn't know this has happened, it may still be
>> writing
>> to that memory, which may now be reallocated to some other kernel subsystem,
>> like
>> file cache buffers.
>>
>> The wait_event() tries to ensure that the port destruction can't take place
>> while the port is still in use, so when it's hung there, it's preventing bad
>> things from happening.
>>
>> There is no way to simply 'force' the port to no longer be in use, since a
>> userspace process can maintain an open file indefinitely.
>>
>> There are a couple of possible solutions though:
>>
>> In gserial_free_line(), hangup the tty associated with the port. You can
>> look at usb_serial_disconnect() for how to do that properly. This doesn't
>> guarantee that the userspace process will close the tty, but most programs
>> will close the file on end-of-file read (which is what hangup will cause).
>
> I tried adding the adding the same tty_vhangup(tty) and tty_kref_put(tty)
> logic that usb serial has
> but the problem still occurs.
That's because g_serial has no hangup() method; ports aren't going to
close
Re: hso. hso_serial_close oops.
[ +Jan Dumon, netdev ]
Forwarding oops report to maintainer.
On 10/10/2014 10:02 AM, Christian Melki wrote:
> I'm using a ppc 8347 with a normal 3.16.1 kernel.
> The software opens the driver tty in question and then sets it as stdin,
> stdout for chat-program and pppd.
> When I yank the modem while running, the software detects this and tries to
> close the open socket with a kernel crash as a result.
>
> Unable to handle kernel paging request for unknown fault
> Faulting instruction address: 0xc03a4420
> Oops: Kernel access of bad area, sig: 11 [#1]
> PREEMPT ASP8347E
> Modules linked in:
> CPU: 0 PID: 1536 Comm: pppd Not tainted 3.16.1 #1
> task: c31272e0 ti: c39e8000 task.ti: c39e8000
> NIP: c03a4420 LR: c020752c CTR: c02074e0
> REGS: c39e9d40 TRAP: 0600 Not tainted (3.16.1)
> MSR: 9032 CR: 24004224 XER: 2000
> DAR: 004d DSISR:
> GPR00: c39e9df0 c31272e0 004d c3235460 c39c1934
> GPR08: c39c1964 c39c1800 24004228 10047610 1004 1003f6ec
> GPR16: 1003f6b4 1003f618 1003f6b0 1003f6bc 1003f700 1003f7b4 c39e9edc 1003f6c8
> GPR24: 1003f6dc c03bd1a8 0004 c03bd2b4 c3235460 c38cca00
> NIP [c03a4420] mutex_lock+0x0/0x1c
> LR [c020752c] hso_serial_close+0x4c/0x11c
> Call Trace:
> [c39e9df0] [c3235460] 0xc3235460 (unreliable)
> [c39e9e00] [c0188944] tty_release+0x134/0x560
> [c39e9e90] [c00a1968] __fput+0x94/0x214
> [c39e9eb0] [c0032854] task_work_run+0xcc/0xf4
> [c39e9ed0] [c0019108] do_exit+0x208/0x874
> [c39e9f20] [c00198c0] do_group_exit+0x44/0xd8
> [c39e9f30] [c0019968] __wake_up_parent+0x0/0x34
> [c39e9f40] [c000e60c] ret_from_syscall+0x0/0x38
> --- Exception: c01 at 0xfebd4cc
> LR = 0xff79a98
> Instruction dump:
> 409e0014 801f003c 70090004 41820008 4bffe6ad 80010034 bb410018 38210030
> 7c0803a6 4e800020 4bffe695 4bc4 <7c001828> 3000 7c00192d 40a2fff4
> ---[ end trace bfebaf22f6f5795a ]---
>
> Fixing recursive fault but reboot is needed!
>
> I have simulated the same error with a simple userland program without using
> pppd.
>
>
> #include
> #include
> #include
> #include
> #include
>
> int
> main(int argc, char *argv[]) {
> int fd;
> fd = open(argv[1], O_RDWR);
> sleep(atoi(argv[2]));
> close(fd);
>
> return 0;
> }
>
> If I yank the modem while the program is sleeping, I get exactly the same
> kernel error as with pppd.
> I have looked at the hso.c (hso_serial_close) driver but can't figure it out.
> The structs might not be intact at that time, but those are tty structs.. Im
> not sure what is going on. I tried to check the integrity of the structs but
> still get a crash. The tty layer is a mystery to me.
>
> regards,
> Christian
--
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: RCU bug with v3.17-rc3 ?
On 10/10/2014 09:44 PM, Nathan Lynch wrote:
> On 10/10/2014 11:25 AM, Russell King - ARM Linux wrote:
>>
>> Right, so GCC 4.8.{1,2} are totally unsuitable for kernel building (and
>> it seems that this has been known about for some time.)
>
> Looking at http://gcc.gnu.org/PR58854 it seems that all 4.8.x for x < 3
> are affected, as well as 4.9.0.
>
>> We can blacklist these GCC versions quite easily. We already have GCC
>> 3.3 blacklisted, and it's trivial to add others. I would want to include
>> some proper details about the bug, just like the other existing entries
>> we already have in asm-offsets.c, where we name the functions that the
>> compiler is known to break where appropriate.
>
> Before blacklisting anything, it's worth considering that simple version
> checks would break existing pre-4.8.3 compilers that have been patched
> for PR58854. It looks like Yocto and Buildroot issued releases with
> patched 4.8.2 compilers well before the (fixed) 4.8.3 release. I think
> the most we can reasonably do without breaking some correctly-behaving
> toolchains is to emit a warning.
Providing a manual switch to override blacklisting is way more sane
than a build warning that no one's looking at.
--
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: RCU bug with v3.17-rc3 ?
On 10/11/2014 10:51 AM, Otavio Salvador wrote:
> Hello Russell,
>
> On Sat, Oct 11, 2014 at 11:16 AM, Russell King - ARM Linux
> wrote:
>> On Sat, Oct 11, 2014 at 11:54:32AM +0800, Peter Chen wrote:
>>> On Fri, Oct 10, 2014 at 08:44:33PM -0500, Nathan Lynch wrote:
On 10/10/2014 11:25 AM, Russell King - ARM Linux wrote:
>
> Right, so GCC 4.8.{1,2} are totally unsuitable for kernel building (and
> it seems that this has been known about for some time.)
Looking at http://gcc.gnu.org/PR58854 it seems that all 4.8.x for x < 3
are affected, as well as 4.9.0.
> We can blacklist these GCC versions quite easily. We already have GCC
> 3.3 blacklisted, and it's trivial to add others. I would want to include
> some proper details about the bug, just like the other existing entries
> we already have in asm-offsets.c, where we name the functions that the
> compiler is known to break where appropriate.
Before blacklisting anything, it's worth considering that simple version
checks would break existing pre-4.8.3 compilers that have been patched
for PR58854. It looks like Yocto and Buildroot issued releases with
patched 4.8.2 compilers well before the (fixed) 4.8.3 release. I think
the most we can reasonably do without breaking some correctly-behaving
toolchains is to emit a warning.
>>>
>>> Yocto has PR58854 problem patch.
>>>
>>> http://git.yoctoproject.org/cgit/cgit.cgi/poky/tree/meta/recipes-devtools/gcc/gcc-4.8/0048-PR58854_fix_arm_apcs_epilogue.patch?h=daisy
>>
>> Right, and we can provide links to these in the comments above the #error
>> so people have the right places to do a bit of research into whether their
>> compiler is safe.
>>
>> It is unfortunate that they are indistinguishable from the broken versions,
>> but that's really a distro problem for causing that issue themselves -
>> especially given how serious this bug is.
>
> What about checking if GCC_PR58854_FIXED is not defined for error? So
> build systems and people could easily define it if they know their GCC
> has the fix applied.
If the distro/build system/individual is capable of patching gcc, then it
seems reasonable that the same distro/build system/individual is capable
of carrying a patch on top of mainline kernel for building with their
"special" compiler.
--
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: RCU bug with v3.17-rc3 ?
On 10/13/2014 10:06 PM, Greg KH wrote:
> On Mon, Oct 13, 2014 at 12:43:07PM +0100, Russell King - ARM Linux wrote:
>> On Mon, Oct 13, 2014 at 09:11:34AM +, David Laight wrote:
>>> From: Nathan Lynch
>>>> On 10/10/2014 11:25 AM, Russell King - ARM Linux wrote:
>>>>>
>>>>> Right, so GCC 4.8.{1,2} are totally unsuitable for kernel building (and
>>>>> it seems that this has been known about for some time.)
>>>>
>>>> Looking at http://gcc.gnu.org/PR58854 it seems that all 4.8.x for x < 3
>>>> are affected, as well as 4.9.0.
>>>>
>>>>> We can blacklist these GCC versions quite easily. We already have GCC
>>>>> 3.3 blacklisted, and it's trivial to add others. I would want to include
>>>>> some proper details about the bug, just like the other existing entries
>>>>> we already have in asm-offsets.c, where we name the functions that the
>>>>> compiler is known to break where appropriate.
>>>>
>>>> Before blacklisting anything, it's worth considering that simple version
>>>> checks would break existing pre-4.8.3 compilers that have been patched
>>>> for PR58854. It looks like Yocto and Buildroot issued releases with
>>>> patched 4.8.2 compilers well before the (fixed) 4.8.3 release. I think
>>>> the most we can reasonably do without breaking some correctly-behaving
>>>> toolchains is to emit a warning.
>>>
>>> Is it possible to compile a small code fragment and check the generated
>>> code for the bug?
>>> Possibly predicated on the broken version number to avoid false positives.
>>
>> I don't see how - it looks like it requires an interrupt to occur at an
>> opportune moment to provoke the function to fail. The alternative would
>> be to parse the assembly generated by the compiler to determine how it
>> is dealing with the stack.
>>
>> I think the only viable solution here is that:
>>
>> 1. We blacklist the bad compiler versions outright in the kernel.
>
> Yes, please do this, it's what we have done for other buggy compiler
> versions, no need to do something different here.
>
>> Remember, it's the distro's choice to fix these buggy compilers, so the
>> onus is on _them_ to deal with the mess they've created by doing so.
>
> I totally agree.
>
> Is someone going to send this patch, or do I have to write it myself?
I did on Friday (arm: Blacklist gcc 4.8.[012] ...) but Russell said he
was doing it himself.
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
[PATCH] USB: kobil_sct: Remove unused transfer buffer allocs
Commit 90419cfcb5d9c889b10dc51363c56a4d394d670e,
"USB: kobil_sct: fix control requests without data stage", removed
the bogus data buffer arguments, but still allocate transfer
buffers which are not used.
Cc: Johan Hovold
Signed-off-by: Peter Hurley
---
drivers/usb/serial/kobil_sct.c | 15 ---
1 file changed, 15 deletions(-)
diff --git a/drivers/usb/serial/kobil_sct.c b/drivers/usb/serial/kobil_sct.c
index 078f9ed..3d2bd65 100644
--- a/drivers/usb/serial/kobil_sct.c
+++ b/drivers/usb/serial/kobil_sct.c
@@ -414,8 +414,6 @@ static int kobil_tiocmset(struct tty_struct *tty,
int result;
int dtr = 0;
int rts = 0;
- unsigned char *transfer_buffer;
- int transfer_buffer_length = 8;
/* FIXME: locking ? */
priv = usb_get_serial_port_data(port);
@@ -425,11 +423,6 @@ static int kobil_tiocmset(struct tty_struct *tty,
return -EINVAL;
}
- /* allocate memory for transfer buffer */
- transfer_buffer = kzalloc(transfer_buffer_length, GFP_KERNEL);
- if (!transfer_buffer)
- return -ENOMEM;
-
if (set & TIOCM_RTS)
rts = 1;
if (set & TIOCM_DTR)
@@ -469,7 +462,6 @@ static int kobil_tiocmset(struct tty_struct *tty,
KOBIL_TIMEOUT);
}
dev_dbg(dev, "%s - Send set_status_line URB returns: %i\n", __func__,
result);
- kfree(transfer_buffer);
return (result < 0) ? result : 0;
}
@@ -530,8 +522,6 @@ static int kobil_ioctl(struct tty_struct *tty,
{
struct usb_serial_port *port = tty->driver_data;
struct kobil_private *priv = usb_get_serial_port_data(port);
- unsigned char *transfer_buffer;
- int transfer_buffer_length = 8;
int result;
if (priv->device_type == KOBIL_USBTWIN_PRODUCT_ID ||
@@ -541,10 +531,6 @@ static int kobil_ioctl(struct tty_struct *tty,
switch (cmd) {
case TCFLSH:
- transfer_buffer = kmalloc(transfer_buffer_length, GFP_KERNEL);
- if (!transfer_buffer)
- return -ENOBUFS;
-
result = usb_control_msg(port->serial->dev,
usb_sndctrlpipe(port->serial->dev, 0),
SUSBCRequest_Misc,
@@ -559,7 +545,6 @@ static int kobil_ioctl(struct tty_struct *tty,
dev_dbg(&port->dev,
"%s - Send reset_all_queues (FLUSH) URB returns: %i\n",
__func__, result);
- kfree(transfer_buffer);
return (result < 0) ? -EIO: 0;
default:
return -ENOIOCTLCMD;
--
2.1.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] USB: kobil_sct: Remove unused transfer buffer allocs
On 10/16/2014 01:59 PM, Peter Hurley wrote:
> Commit 90419cfcb5d9c889b10dc51363c56a4d394d670e,
> "USB: kobil_sct: fix control requests without data stage", removed
> the bogus data buffer arguments, but still allocate transfer
> buffers which are not used.
>
> Cc: Johan Hovold
> Signed-off-by: Peter Hurley
> ---
> drivers/usb/serial/kobil_sct.c | 15 ---
> 1 file changed, 15 deletions(-)
>
> diff --git a/drivers/usb/serial/kobil_sct.c b/drivers/usb/serial/kobil_sct.c
> index 078f9ed..3d2bd65 100644
> --- a/drivers/usb/serial/kobil_sct.c
> +++ b/drivers/usb/serial/kobil_sct.c
> @@ -414,8 +414,6 @@ static int kobil_tiocmset(struct tty_struct *tty,
> int result;
> int dtr = 0;
> int rts = 0;
> - unsigned char *transfer_buffer;
> - int transfer_buffer_length = 8;
>
> /* FIXME: locking ? */
> priv = usb_get_serial_port_data(port);
> @@ -425,11 +423,6 @@ static int kobil_tiocmset(struct tty_struct *tty,
> return -EINVAL;
> }
>
> - /* allocate memory for transfer buffer */
> - transfer_buffer = kzalloc(transfer_buffer_length, GFP_KERNEL);
> - if (!transfer_buffer)
> - return -ENOMEM;
> -
> if (set & TIOCM_RTS)
> rts = 1;
> if (set & TIOCM_DTR)
> @@ -469,7 +462,6 @@ static int kobil_tiocmset(struct tty_struct *tty,
> KOBIL_TIMEOUT);
> }
> dev_dbg(dev, "%s - Send set_status_line URB returns: %i\n", __func__,
> result);
> - kfree(transfer_buffer);
> return (result < 0) ? result : 0;
> }
>
> @@ -530,8 +522,6 @@ static int kobil_ioctl(struct tty_struct *tty,
> {
> struct usb_serial_port *port = tty->driver_data;
> struct kobil_private *priv = usb_get_serial_port_data(port);
> - unsigned char *transfer_buffer;
> - int transfer_buffer_length = 8;
> int result;
>
> if (priv->device_type == KOBIL_USBTWIN_PRODUCT_ID ||
> @@ -541,10 +531,6 @@ static int kobil_ioctl(struct tty_struct *tty,
>
> switch (cmd) {
> case TCFLSH:
> - transfer_buffer = kmalloc(transfer_buffer_length, GFP_KERNEL);
> - if (!transfer_buffer)
> - return -ENOBUFS;
> -
> result = usb_control_msg(port->serial->dev,
> usb_sndctrlpipe(port->serial->dev, 0),
> SUSBCRequest_Misc,
> @@ -559,7 +545,6 @@ static int kobil_ioctl(struct tty_struct *tty,
> dev_dbg(&port->dev,
> "%s - Send reset_all_queues (FLUSH) URB returns: %i\n",
> __func__, result);
> - kfree(transfer_buffer);
> return (result < 0) ? -EIO: 0;
^^^
Returning 0 is almost certainly wrong; no further processing for
TCFLSH is performed.
Only this driver returns 0 (of all the tty drivers in mainline).
Returning -ENOIOCTLCMD allows further processing to continue;
especially the line discipline's input flushing, if TCIFLUSH/TCIOFLUSH.
Is it trying to avoid the tty_driver_flush_buffer() because it doesn't
have an output buffer?
> default:
> return -ENOIOCTLCMD;
>
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] USB: kobil_sct: Remove unused transfer buffer allocs
On 10/22/2014 04:20 AM, Johan Hovold wrote:
> On Thu, Oct 16, 2014 at 01:59:22PM -0400, Peter Hurley wrote:
>> Commit 90419cfcb5d9c889b10dc51363c56a4d394d670e,
>> "USB: kobil_sct: fix control requests without data stage", removed
>
> checkpatch.pl complains on your commit-reference style so you know.
Thanks for letting me know.
Joe,
Why is checkpatch complaining about the commit reference above?
$ ./scripts/checkpatch.pl
../patches/fixes-3.18/0001-USB-kobil_sct-Remove-unused-transfer-buffer-allocs.patch
ERROR: Please use 12 or more chars for the git commit ID like: 'Commit
90419cfcb5d9 ("USB: kobil_sct: fix control requests without data stage")'
#6:
Commit 90419cfcb5d9c889b10dc51363c56a4d394d670e,
total: 1 errors, 0 warnings, 51 lines checked
>> the bogus data buffer arguments, but still allocate transfer
>> buffers which are not used.
>>
>> Cc: Johan Hovold
>> Signed-off-by: Peter Hurley
>
> Applied now.
>
> Thanks,
> 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] USB: kobil_sct: Remove unused transfer buffer allocs
On 10/19/2014 01:12 PM, Johan Hovold wrote:
> [ +CC: Jiri, Alan, linux-serial ]
>
> On Thu, Oct 16, 2014 at 02:09:29PM -0400, Peter Hurley wrote:
>> On 10/16/2014 01:59 PM, Peter Hurley wrote:
>>> Commit 90419cfcb5d9c889b10dc51363c56a4d394d670e,
>>> "USB: kobil_sct: fix control requests without data stage", removed
>>> the bogus data buffer arguments, but still allocate transfer
>>> buffers which are not used.
>>>
>>> Cc: Johan Hovold
>>> Signed-off-by: Peter Hurley
>>> ---
>>> drivers/usb/serial/kobil_sct.c | 15 ---
>>> 1 file changed, 15 deletions(-)
>>>
>>> diff --git a/drivers/usb/serial/kobil_sct.c b/drivers/usb/serial/kobil_sct.c
>>> index 078f9ed..3d2bd65 100644
>>> --- a/drivers/usb/serial/kobil_sct.c
>>> +++ b/drivers/usb/serial/kobil_sct.c
>>> @@ -414,8 +414,6 @@ static int kobil_tiocmset(struct tty_struct *tty,
>>> int result;
>>> int dtr = 0;
>>> int rts = 0;
>>> - unsigned char *transfer_buffer;
>>> - int transfer_buffer_length = 8;
>>>
>>> /* FIXME: locking ? */
>>> priv = usb_get_serial_port_data(port);
>>> @@ -425,11 +423,6 @@ static int kobil_tiocmset(struct tty_struct *tty,
>>> return -EINVAL;
>>> }
>>>
>>> - /* allocate memory for transfer buffer */
>>> - transfer_buffer = kzalloc(transfer_buffer_length, GFP_KERNEL);
>>> - if (!transfer_buffer)
>>> - return -ENOMEM;
>>> -
>>> if (set & TIOCM_RTS)
>>> rts = 1;
>>> if (set & TIOCM_DTR)
>>> @@ -469,7 +462,6 @@ static int kobil_tiocmset(struct tty_struct *tty,
>>> KOBIL_TIMEOUT);
>>> }
>>> dev_dbg(dev, "%s - Send set_status_line URB returns: %i\n", __func__,
>>> result);
>>> - kfree(transfer_buffer);
>>> return (result < 0) ? result : 0;
>>> }
>>>
>>> @@ -530,8 +522,6 @@ static int kobil_ioctl(struct tty_struct *tty,
>>> {
>>> struct usb_serial_port *port = tty->driver_data;
>>> struct kobil_private *priv = usb_get_serial_port_data(port);
>>> - unsigned char *transfer_buffer;
>>> - int transfer_buffer_length = 8;
>>> int result;
>>>
>>> if (priv->device_type == KOBIL_USBTWIN_PRODUCT_ID ||
>>> @@ -541,10 +531,6 @@ static int kobil_ioctl(struct tty_struct *tty,
>>>
>>> switch (cmd) {
>>> case TCFLSH:
>>> - transfer_buffer = kmalloc(transfer_buffer_length, GFP_KERNEL);
>>> - if (!transfer_buffer)
>>> - return -ENOBUFS;
>>> -
>>> result = usb_control_msg(port->serial->dev,
>>> usb_sndctrlpipe(port->serial->dev, 0),
>>> SUSBCRequest_Misc,
>>> @@ -559,7 +545,6 @@ static int kobil_ioctl(struct tty_struct *tty,
>>> dev_dbg(&port->dev,
>>> "%s - Send reset_all_queues (FLUSH) URB returns: %i\n",
>>> __func__, result);
>>> - kfree(transfer_buffer);
>>> return (result < 0) ? -EIO: 0;
>>^^^
>> Returning 0 is almost certainly wrong; no further processing for
>> TCFLSH is performed.
>
> Indeed.
>
>> Only this driver returns 0 (of all the tty drivers in mainline).
>>
>> Returning -ENOIOCTLCMD allows further processing to continue;
>> especially the line discipline's input flushing, if TCIFLUSH/TCIOFLUSH.
>
> That doesn't seem like a very good idea, and only two *staging* drivers
> try to play such games (i.e. pretending not to implement the ioctl) as
> far as I can see.
Well, returning EIONOCTLCMD is the standard method of ioctl passthrough
from driver to line discipline.
Since driver 'input buffer' flushing is not currently supported by the
core, this seems the only available workaround.
> The only non-staging tty driver which appears to implement TCFLSH,
> ipwireless, calls tty_perform_flush directly to flush the ldisc buffers.
> That doesn't seem right either.
I'm not sure why ipwireless does this; I can only guess that it's a
workaround for some line discipline that doesn't use n_tty_ioctl_helper().
> Shouldn't this be fixed by removing TCFLSH from these tty drivers'
> ioctl callbacks and implementing flush_buffer()?
>
> The stag
Re: [PATCH] USB: kobil_sct: Remove unused transfer buffer allocs
On 10/22/2014 08:07 AM, Joe Perches wrote:
> On Wed, 2014-10-22 at 06:26 -0400, Peter Hurley wrote:
>> On 10/22/2014 04:20 AM, Johan Hovold wrote:
>>> On Thu, Oct 16, 2014 at 01:59:22PM -0400, Peter Hurley wrote:
>>>> Commit 90419cfcb5d9c889b10dc51363c56a4d394d670e,
>>>> "USB: kobil_sct: fix control requests without data stage", removed
>>>
>>> checkpatch.pl complains on your commit-reference style so you know.
>>
>> Thanks for letting me know.
>>
>> Joe,
>>
>> Why is checkpatch complaining about the commit reference above?
>
>> $ ./scripts/checkpatch.pl
>> ../patches/fixes-3.18/0001-USB-kobil_sct-Remove-unused-transfer-buffer-allocs.patch
>>
>> ERROR: Please use 12 or more chars for the git commit ID like: 'Commit
>> 90419cfcb5d9 ("USB: kobil_sct: fix control requests without data stage")'
>> #6:
>> Commit 90419cfcb5d9c889b10dc51363c56a4d394d670e,
>
> It wants parentheses around the commit description.
Ah, ok.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: gadget serial: Honour termios CLOCAL on disconnect
On 10/27/2014 09:38 AM, Felipe Balbi wrote:
> Hi,
>
> On Sun, Oct 26, 2014 at 08:01:30PM +0200, Kyösti Mälkki wrote:
>> There are applications where it is desirable to not hangup ttyGS* when
>> USB disconnect is detected. USB host side of communication may
>> power-cycle periodically or there may be the actual need to physically
>> disconnect and reconnect USB cable temporarily.
>>
>> USB disconnects on serial gadget are comparable to loss of Carrier Detect
>> of conventional UARTs. With the change, if ttyGS* has termios CLOCAL flag
>> set, disconnect on USB does not hangup the TTY.
>>
>> Signed-off-by: Kyösti Mälkki
>> ---
>> drivers/usb/gadget/function/u_serial.c | 9 +++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/u_serial.c
>> b/drivers/usb/gadget/function/u_serial.c
>> index 491082a..e68ffd7 100644
>> --- a/drivers/usb/gadget/function/u_serial.c
>> +++ b/drivers/usb/gadget/function/u_serial.c
>> @@ -1254,8 +1254,13 @@ void gserial_disconnect(struct gserial *gser)
>> gser->ioport = NULL;
>> if (port->port.count > 0 || port->openclose) {
>> wake_up_interruptible(&port->drain_wait);
>> -if (port->port.tty)
>> -tty_hangup(port->port.tty);
>> +struct tty_struct *tty = port->port.tty;
>
> declare above as Sergei said.
>
>> +if (tty) {
>
> is there any situation where tty would be NULL here ?
>
>> +if (tty->termios.c_cflag & CLOCAL)
>> +stop_tty(tty);
>> +else
>> + tty_hangup(tty);
>
> this I'll defer to Greg who also maintains tty.
I'm curious what happens without stop_tty().
The tty is restartable from userspace with
tcflow(fd, TCOOFF);
tcflow(fd, TCOON);
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] cdc-acm: ensure that termios get set when the port is opened
Hi Jim,
On 10/28/2014 07:26 PM, Jim Paris wrote:
> Do what other drivers like ftdi_sio do, and ensure that termios are
> written to the device when the port is first opened.
>
> Signed-off-by: Jim Paris
> ---
>
> Tested on v3.16.5.
>
> I've seen a problem on two CDC-ACM systems based on a Segger J-Link
> where the port does not get initialized at the correct baudrate when
> opening (using e.g. python-serial). I think this occurs when the tty
> device was previously opened at the same baudrate, then the device was
> unplugged and replugged. While the port is open, manually switching
> to a different baudrate and back fixes it.
>
> Debug output in the failing case is e.g.:
>
> Oct 28 18:37:45 pilot kernel: [1214446.586460] tty ttyACM0: acm_tty_install
> Oct 28 18:37:45 pilot kernel: [1214446.586474] tty ttyACM0: acm_tty_open
> Oct 28 18:37:45 pilot kernel: [1214446.586477] cdc_acm 1-2.7:1.0:
> acm_port_activate
> Oct 28 18:37:45 pilot kernel: [1214446.586670] cdc_acm 1-2.7:1.0:
> acm_ctrl_msg - rq 0x22, val 0x3, len 0x0, result 0
>
> which is missing the important:
>
> Oct 28 19:03:18 pilot kernel: [1215981.178020] cdc_acm 1-2.7:1.0:
> acm_tty_set_termios - set line: 38400 0 0 8
> Oct 28 19:03:18 pilot kernel: [1215981.178135] cdc_acm 1-2.7:1.0:
> acm_ctrl_msg - rq 0x20, val 0x0, len 0x7, result 7
>
> that I get when changing settings to something different than they
> previously were.
>
> I don't really follow all of the termios and tty stuff, so I don't
> know if this is the right fix or the real cause. I suspect it
> have to do with cached values associated with the particular TTY
> ("lazy saved data" in tty-io.c); this patch just does what I see in
> ftdi_sio and ensures that the termios settings are written to the
> device when the port is opened.
Yeah, you're right that the cdc-acm driver isn't properly configuring
the hardware for the current termios settings under all conditions.
But you don't want to do it for every tty open, only for opens
requiring port initialization, which is what the tty_port->activate()
method is for (ie., acm_port_activate()).
Note that the ftdi_sio driver is a usb serial port driver; ftdi_open()
is called from serial_port_activate(), which is the activate() method
for all usb serial drivers.
Regards,
Peter Hurley
> ---
> drivers/usb/class/cdc-acm.c | 21 -
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index e934e19f49f5..144bf43c9190 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -495,15 +495,6 @@ error_init_termios:
> return retval;
> }
>
> -static int acm_tty_open(struct tty_struct *tty, struct file *filp)
> -{
> - struct acm *acm = tty->driver_data;
> -
> - dev_dbg(tty->dev, "%s\n", __func__);
> -
> - return tty_port_open(&acm->port, tty, filp);
> -}
> -
> static void acm_port_dtr_rts(struct tty_port *port, int raise)
> {
> struct acm *acm = container_of(port, struct acm, port);
> @@ -1000,6 +991,18 @@ static void acm_tty_set_termios(struct tty_struct *tty,
> }
> }
>
> +static int acm_tty_open(struct tty_struct *tty, struct file *filp)
> +{
> + struct acm *acm = tty->driver_data;
> +
> + dev_dbg(tty->dev, "%s\n", __func__);
> +
> + if (tty)
> + acm_tty_set_termios(tty, NULL);
> +
> + return tty_port_open(&acm->port, tty, filp);
> +}
> +
> static const struct tty_port_operations acm_port_ops = {
> .dtr_rts = acm_port_dtr_rts,
> .shutdown = acm_port_shutdown,
>
--
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: usbserial / ftdi_sio (+ others) bug?
On 10/29/2014 04:51 AM, Johan Hovold wrote: > [ +CC: Peter, linux-serial ] > > On Wed, Oct 29, 2014 at 10:07:26AM +0200, Janne Huttunen wrote: >> I own a device that implements a data logging interface using the >> FT232 USB-serial -chip. Very often it happens that connecting the >> associated software with the device requires multiple attempts. >> There seems to be two kinds of issues; either the program reports >> that it did not receive any data or it reports reading lots of >> data, but it was all invalid. I haven't yet looked at the former, >> but I did spend some time investigating the latter. >> >> Simple strace of the program startup showed that when connecting >> fails, the program gets a lot (hundreds) of binary zeros while >> reading the device. So you're only getting status and not data. > I used usbmon to capture the traffic between >> the host and the device and the zeros are not strictly speaking >> coming from the device. However when this problem happens the >> device seems to report quite lot of overruns for a while, which >> was a clue. After a somewhat successful attempt to understand >> the operation of the tty code in Linux, I have a theory. >> >> The usbserial driver sets the TTY_DRIVER_REAL_RAW flag. Based on >> the comment in tty_driver.h this implies that the driver is not >> supposed to report any statuses (including overruns) to ldisc >> if they are ignored by the application (like they are in this >> case). It's just that AFAICS the ftdi_sio subdriver (and many >> others) doesn't seem quite honor this, but seems to report any >> status unconditionally. Also AFAICS this then means that every >> overrun will get converted into single binary zero delivered to >> the application(?). If so, this probably isn't what is supposed >> to happen and would explain the flood of extraneous zeros the >> application was seeing when the connecting failed. >> >> I haven't had yet the time to test this theory, but at least it >> seems plausible to me. Any thoughts, anybody? > > You are correct. The usb-serial drivers, and at least some serial > drivers, fail to implement TTY_DRIVER_REAL_RAW correctly in that they > do not honour ((IGNBRK || (!BRKINT && !PARMRK)) && (IGNPAR || !INPCK)). These settings are a constant source of bugs in serial drivers. We really need to abstract the way these settings are processed; even the 8250 driver is getting this wrong. 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] USB: kobil_sct: Remove unused transfer buffer allocs
On 10/30/2014 08:08 AM, Johan Hovold wrote:
> Hi Peter,
>
> Forgot to reply to this one.
>
> On Wed, Oct 22, 2014 at 07:40:20AM -0400, Peter Hurley wrote:
>> On 10/19/2014 01:12 PM, Johan Hovold wrote:
>>> [ +CC: Jiri, Alan, linux-serial ]
>>>
>>> On Thu, Oct 16, 2014 at 02:09:29PM -0400, Peter Hurley wrote:
>>>> On 10/16/2014 01:59 PM, Peter Hurley wrote:
>
>>>>> @@ -541,10 +531,6 @@ static int kobil_ioctl(struct tty_struct *tty,
>>>>>
>>>>> switch (cmd) {
>>>>> case TCFLSH:
>>>>> - transfer_buffer = kmalloc(transfer_buffer_length, GFP_KERNEL);
>>>>> - if (!transfer_buffer)
>>>>> - return -ENOBUFS;
>>>>> -
>>>>> result = usb_control_msg(port->serial->dev,
>>>>> usb_sndctrlpipe(port->serial->dev, 0),
>>>>> SUSBCRequest_Misc,
>>>>> @@ -559,7 +545,6 @@ static int kobil_ioctl(struct tty_struct *tty,
>>>>> dev_dbg(&port->dev,
>>>>> "%s - Send reset_all_queues (FLUSH) URB returns: %i\n",
>>>>> __func__, result);
>>>>> - kfree(transfer_buffer);
>>>>> return (result < 0) ? -EIO: 0;
>>>>^^^
>>>> Returning 0 is almost certainly wrong; no further processing for
>>>> TCFLSH is performed.
>>>
>>> Indeed.
>>>
>>>> Only this driver returns 0 (of all the tty drivers in mainline).
>>>>
>>>> Returning -ENOIOCTLCMD allows further processing to continue;
>>>> especially the line discipline's input flushing, if TCIFLUSH/TCIOFLUSH.
>>>
>>> That doesn't seem like a very good idea, and only two *staging* drivers
>>> try to play such games (i.e. pretending not to implement the ioctl) as
>>> far as I can see.
>>
>> Well, returning EIONOCTLCMD is the standard method of ioctl passthrough
>> from driver to line discipline.
>
> I disagree with you there. AFAICS only these two staging drivers are
> abusing the meaning of EIONOCTLCMD (unrecognised ioctl) to have the
> line discipline also act on the ioctl.
Sorry, I wasn't as clear as I should have been here.
My point was that every driver gets ioctl(TCFLSH) and returns ENOIOCTLCMD
so that the line discipline will handle it. You're absolutely correct, in
that, only these drivers (and ipwireless) doing anything with TCFLSH
>> Since driver 'input buffer' flushing is not currently supported by the
>> core, this seems the only available workaround.
>
> That is true. But I doubt we should use these two staging drivers as a
> model for how this should be handled, if it's at all needed.
Right. My comments implied approval of the design, which I don't.
The main problem with the existing design is that it allows for a
significant variation in how ioctl(WHATEVER) is handled by various
driver/ldisc combinations. That's a bad thing because it makes
audit/review really time-consuming and makes changes prone to userspace
regressions.
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 v2 2/2] usb: gadget serial: Honour termios CLOCAL on disconnect
Hi Kyösti,
On 11/03/2014 10:18 AM, Kyösti Mälkki wrote:
> There are applications where it is desirable to not hangup ttyGS* when
> USB disconnect is detected. USB host side of communication may
> power-cycle periodically or there may be the actual need to physically
> disconnect and reconnect USB cable temporarily.
>
> USB disconnects on serial gadget are comparable to loss of Carrier Detect
> of conventional UARTs. With the change, if ttyGS* has termios CLOCAL flag
> set, disconnect on USB does not hangup the TTY.
>
> Signed-off-by: Kyösti Mälkki
> ---
> drivers/usb/gadget/function/u_serial.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/u_serial.c
> b/drivers/usb/gadget/function/u_serial.c
> index 491082a..dabc165 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -1253,9 +1253,16 @@ void gserial_disconnect(struct gserial *gser)
> port->port_usb = NULL;
> gser->ioport = NULL;
> if (port->port.count > 0 || port->openclose) {
> + struct tty_struct *tty;
> +
> wake_up_interruptible(&port->drain_wait);
> - if (port->port.tty)
> - tty_hangup(port->port.tty);
> + tty = port->port.tty;
> + if (tty) {
> + if (tty->termios.c_cflag & CLOCAL)
> + stop_tty(tty);
> + else
> + tty_hangup(tty);
It seems you missed my earlier email: what happens if you leave
out the stop_tty() call here?
I ask because the tty is still restartable from userspace after you
stop_tty() here. So if your goal is to prevent write() from
happening, this won't work.
Regards,
Peter Hurley
> + }
> }
> spin_unlock_irqrestore(&port->port_lock, flags);
>
>
--
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
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, and at extremely high bit rates may not be appropriate. (It's mostly a historical vestige from pre-tty buffer days). 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
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
On 07/22/2015 04:40 AM, Oliver Neukum wrote: > On Tue, 2015-07-21 at 12:45 -0400, Peter Hurley wrote: >> 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). > > Could you please expand on how this is supposed to work? > For once how does one learn that room is available again? There are basically 3 mechanisms for managing rx data: 1. Allocate space when the data arrives; drop data if no space is avail and indicate buf_overrun. This is what most drivers do. 2. Allocate space when the data arrives; try to buffer uncopied data and resubmit the data later. Some high-throughput drivers (in the wild) do this (but less so now that the tty buffer space is configurable). 3. Pre-allocate space _before_ the data arrives (with tty_buffer_request_room()); this is applicable to subsystems which know how much data can be in-flight at any one time. This guarantees that when rx data arrives buffer space is available (since it has already been allocated). Drivers that use method 2 typically attempt to recopy the buffered data when either new data arrives or @ unthrottle. I've seen others use deferred work as well. AFAIK no driver/subsystem is using method 3 for guaranteed delivery of in-flight data, but it seems ideally suited to usb serial. 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
Hi Sven, On 07/21/2015 08:47 PM, Sven Brauch wrote: > 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. Thanks for this, which confirms that roughly 10.4ms elapses from kworker schedule (of input into nearly empty tty buffers) to throttle notification. The premature unthrottle actually leads to the data loss but the throttling with a mere 2K left is _way too late_. 10ms is a _really_ long time for a cpu not to attend to a kworker. Which raises 2 questions: 1. What are the termios settings of the tty receiving input? Is it 'raw' mode or typical terminal mode (icanon, echo, etc.) or something else? 2. Are there RT threads that are hogging cpu time? 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: USB - Generic Serial device : Unable to read more than 4095 bytes
On 08/03/2015 03:44 AM, arun k wrote: > Hi , > > I have a trouble with using usb serial generic device. > I am using USB - Generic Serial driver for communicating with my usb > device and my embedded device. > My usb device sending data at a rate of 409600 bytes/sec, and in host > side application I tried to read 16384 bytes in one read. But the read > size returning is always 4095 > I don't know whether I missed any settings or this is limited in driver. > Could you please tell me is it possible to read large size(16384 > bytes) using usb serial generic driver. read() is limited to 4096 bytes by the driver. What linux version are you using? 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: USB - Generic Serial device : Unable to read more than 4095 bytes
On 08/03/2015 10:47 PM, arun k wrote: > Thank you for the reply > >>> The tty layer is limiting you, just keep reading in a loop until you >>> run out of data, you should not ever expect to read a specific number of >>> bytes from a tty device at a time, the read call will tell you the >>> number that was read properly. > > I tried this method but in my case I found data loss. By changing any buffer > size modification in driver side , can we fix this issue ? The data loss is probably occurring because you have all flow control disabled and the line speed is too fast for no flow control. Regards, Peter Hurley >>> Please don't use the "generic" driver, it's slow and not the best to >>> use > > Could you tell me is generic driver is suitable for my application ( My usb > device sending data at a rate of 409600 bytes/sec) , ? > or do you have any other suggestion for me ? > > My Linux kernel version is 3.4.35 > > Regards, > Arun > > On Tue, Aug 4, 2015 at 1:13 AM, Greg KH <mailto:gre...@linuxfoundation.org>> wrote: > > On Mon, Aug 03, 2015 at 04:44:07PM +0900, arun k wrote: > > Hi , > > > > I have a trouble with using usb serial generic device. > > I am using USB - Generic Serial driver for communicating with my usb > > device and my embedded device. > > Please don't use the "generic" driver, it's slow and not the best to > use. > > > My usb device sending data at a rate of 409600 bytes/sec, and in host > > side application I tried to read 16384 bytes in one read. But the read > > size returning is always 4095 > > The tty layer is limiting you, just keep reading in a loop until you > run out of data, you should not ever expect to read a specific number of > bytes from a tty device at a time, the read call will tell you the > number that was read properly. > > 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] Fix data loss in cdc-acm
On 07/22/2015 11:01 AM, Oliver Neukum wrote: > On Wed, 2015-07-22 at 10:30 -0400, Peter Hurley wrote: >> 3. Pre-allocate space _before_ the data arrives (with >> tty_buffer_request_room()); >>this is applicable to subsystems which know how much data can be >> in-flight >>at any one time. This guarantees that when rx data arrives buffer >> space is >>available (since it has already been allocated). >> >> Drivers that use method 2 typically attempt to recopy the buffered >> data >> when either new data arrives or @ unthrottle. I've seen others use >> deferred >> work as well. >> >> AFAIK no driver/subsystem is using method 3 for guaranteed delivery >> of in-flight data, but it seems ideally suited to usb serial. > > Indeed. But flow control is still done by throttle/unthrottle, isn't it? Oliver, somehow I never saw this. Please accept my apologies. Well, input flow control from the driver would be implicitly limited by a failure to allocate new buffer space after the old buffer was inserted and pushed. In the case of urbs, this would simply mean that the urb could not be resubmitted yet, and would have to be deferred. So preventing buffer overrun wouldn't rely on throttle. WRT flow control, method 3 only needs to know when to resume. As with method 2, this could be @ unthrottle or by retry in deferred work (essentially polling). At least that's what's currently available. Regardless, tty drivers should still respond to throttle/unthrottle requests so as to limit input data when there is no active reader. All that being said, unthrottle behaves degenerately at very high line rates and throughput, which needs to be resolved. The central issue is that the wrong thread is evaluating the wrong conditions for restart. A further complication is that unthrottle is guaranteed race-free reads of struct termios which limit the contexts from which unthrottle can be called. So to recap; the tty buffer api already provides the necessary interface for preventing buffer overrun and drivers/subsystems with very high line rates should be utilizing that interface instead of relying on throttle. In addition, a more reliable/appropriate method of restart is required (preferably by fixing when/why unthrottle is invoked). I'll look into that. 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: USB - Generic Serial device : Unable to read more than 4095 bytes
On 08/04/2015 10:00 PM, arun k wrote: > I enabled software flow control like below > > tty.c_iflag |= (IXON | IXOFF | IXANY); Ok, but does the sending device know how to process in-band software flow control and is it set up to respond properly? Also, I doubt software flow control is going to work @ 4Mbaud line rate. If you're stuck using software flow control, start with a 115kbaud line rate and see if that fixes your data loss problem. Regards, Peter Hurley > On Wed, Aug 5, 2015 at 12:59 AM, Peter Hurley > wrote: >> On 08/03/2015 10:47 PM, arun k wrote: >>> Thank you for the reply >>> >>>>> The tty layer is limiting you, just keep reading in a loop until you >>>>> run out of data, you should not ever expect to read a specific number of >>>>> bytes from a tty device at a time, the read call will tell you the >>>>> number that was read properly. >>> >>> I tried this method but in my case I found data loss. By changing any >>> buffer size modification in driver side , can we fix this issue ? >> >> The data loss is probably occurring because you have all flow control >> disabled >> and the line speed is too fast for no flow control. >> >> Regards, >> Peter Hurley >> >>>>> Please don't use the "generic" driver, it's slow and not the best to >>>>> use >>> >>> Could you tell me is generic driver is suitable for my application ( My usb >>> device sending data at a rate of 409600 bytes/sec) , ? >>> or do you have any other suggestion for me ? >>> >>> My Linux kernel version is 3.4.35 >>> >>> Regards, >>> Arun >>> >>> On Tue, Aug 4, 2015 at 1:13 AM, Greg KH >> <mailto:gre...@linuxfoundation.org>> wrote: >>> >>> On Mon, Aug 03, 2015 at 04:44:07PM +0900, arun k wrote: >>> > Hi , >>> > >>> > I have a trouble with using usb serial generic device. >>> > I am using USB - Generic Serial driver for communicating with my usb >>> > device and my embedded device. >>> >>> Please don't use the "generic" driver, it's slow and not the best to >>> use. >>> >>> > My usb device sending data at a rate of 409600 bytes/sec, and in host >>> > side application I tried to read 16384 bytes in one read. But the read >>> > size returning is always 4095 >>> >>> The tty layer is limiting you, just keep reading in a loop until you >>> run out of data, you should not ever expect to read a specific number of >>> bytes from a tty device at a time, the read call will tell you the >>> number that was read properly. >>> >>> 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: USB - Generic Serial device : Unable to read more than 4095 bytes
On 08/04/2015 11:57 PM, arun k wrote: >> Ok, but does the sending device know how to process in-band software flow >> control >> and is it set up to respond properly? > > I am not sure about this. I need to check this. > >> Also, I doubt software flow control is going to work @ 4Mbaud line rate. >> If you're stuck using software flow control, start with a 115kbaud line rate >> and see if that fixes your data loss problem > > I tried B115200 also but issue is still there. I think the generic USB driver ignores baud rate. Like Greg suggested, you need to use the proper driver for your hardware. What is the output from 'lsusb -v' and please identify in that output which device you are using. > I am using a USB device and that sending data at a rate of *409600 > bytes/sec*. Is this data rate have any relation with issue. Yes. Also, can you test this on a newer kernel like 3.18? Regards, Peter Hurley > On Wed, Aug 5, 2015 at 11:24 AM, Peter Hurley <mailto:pe...@hurleysoftware.com>> wrote: > > On 08/04/2015 10:00 PM, arun k wrote: > > I enabled software flow control like below > > > > tty.c_iflag |= (IXON | IXOFF | IXANY); > > Ok, but does the sending device know how to process in-band software flow > control > and is it set up to respond properly? > > Also, I doubt software flow control is going to work @ 4Mbaud line rate. > If you're stuck using software flow control, start with a 115kbaud line > rate > and see if that fixes your data loss problem. > > Regards, > Peter Hurley > > > > On Wed, Aug 5, 2015 at 12:59 AM, Peter Hurley <mailto:pe...@hurleysoftware.com>> wrote: > >> On 08/03/2015 10:47 PM, arun k wrote: > >>> Thank you for the reply > >>> > >>>>> The tty layer is limiting you, just keep reading in a loop until you > >>>>> run out of data, you should not ever expect to read a specific > number of > >>>>> bytes from a tty device at a time, the read call will tell you the > >>>>> number that was read properly. > >>> > >>> I tried this method but in my case I found data loss. By changing any > buffer size modification in driver side , can we fix this issue ? > >> > >> The data loss is probably occurring because you have all flow control > disabled > >> and the line speed is too fast for no flow control. > >> > >> Regards, > >> Peter Hurley > >> > >>>>> Please don't use the "generic" driver, it's slow and not the best to > >>>>> use > >>> > >>> Could you tell me is generic driver is suitable for my application ( > My usb device sending data at a rate of 409600 bytes/sec) , ? > >>> or do you have any other suggestion for me ? > >>> > >>> My Linux kernel version is 3.4.35 > >>> > >>> Regards, > >>> Arun > >>> > >>> On Tue, Aug 4, 2015 at 1:13 AM, Greg KH <mailto:gre...@linuxfoundation.org> <mailto:gre...@linuxfoundation.org > <mailto:gre...@linuxfoundation.org>>> wrote: > >>> > >>> On Mon, Aug 03, 2015 at 04:44:07PM +0900, arun k wrote: > >>> > Hi , > >>> > > >>> > I have a trouble with using usb serial generic device. > >>> > I am using USB - Generic Serial driver for communicating with > my usb > >>> > device and my embedded device. > >>> > >>> Please don't use the "generic" driver, it's slow and not the best > to > >>> use. > >>> > >>> > My usb device sending data at a rate of 409600 bytes/sec, and > in host > >>> > side application I tried to read 16384 bytes in one read. But > the read > >>> > size returning is always 4095 > >>> > >>> The tty layer is limiting you, just keep reading in a loop until > you > >>> run out of data, you should not ever expect to read a specific > number of > >>> bytes from a tty device at a time, the read call will tell you the > >>> number that was read properly. > >>> > >>> 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] Fix data loss in cdc-acm
On 07/22/2015 06:53 PM, Sven Brauch wrote:
> Hi,
>
> On 23/07/15 00:12, Peter Hurley wrote:
>> The premature unthrottle actually leads to the data loss but the throttling
>> with a mere 2K left is _way too late_.
> Ok, yes, I think so too.
>
>> 10ms is a _really_ long time for a cpu not to attend to a kworker.
I haven't forgotten about this problem and I still plan to look into why
the long delay, particularly focusing on the scheduling latency from
tty_flip_buffer_push() => flush_to_ldisc().
Regards,
Peter Hurley
>> Which raises 2 questions:
>> 1. What are the termios settings of the tty receiving input? Is it 'raw'
>>mode or typical terminal mode (icanon, echo, etc.) or something else?
> In my test code, I open the tty like
> fd = open("/dev/ttyACM0", O_RDWR | O_NOCTTY | O_NONBLOCK);
> I don't make any other changes to the default settings. To be honest,
> I'm not sure in which mode it is operating then (I was assuming raw, but
> I might be wrong?).
>
>> 2. Are there RT threads that are hogging cpu time?
> I can't see any, I think the only thing which occasionally goes to RT is
> pulseaudio (but during at least some of the tests I wasn't even playing
> audio, so that sounds very unplausible to me). I also cannot see a
> correlation between failure rate and CPU load.
>
> Best regards,
> Sven
>
--
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: g_serial hangs on write when the cable is disconnected
[ +cc Felipe ]
Hi Laszlo,
On 08/06/2015 10:03 AM, Laszlo Papp wrote:
> On Thu, Aug 6, 2015 at 2:52 PM, Alan Stern wrote:
>> On Thu, 6 Aug 2015, Laszlo Papp wrote:
>>> On Wed, Aug 5, 2015 at 7:15 PM, Alan Stern
>>> wrote:
>>>> On Wed, 5 Aug 2015, Greg KH wrote:
>>>>
>>>>> hm, wait, is this really the n_gsm line discipline? Or is it something
>>>>> else?
>>>>>
>>>>> g_serial is the device side of a serial connection, there is no "cable
>>>>> removed" notification that it even knows about, that has to come from
>>>>> the gadget driver somehow, which you should listen for and then kick
>>>>> your userspace program.
>>>>
>>>> There is the gserial_disconnect() callback, which gets invoked when the
>>>> Vbus power (provided by the host) is removed. It's a pretty good
>>>> indicator that the USB cable has been unplugged.
>>>>
>>>> I don't understand all the stuff that gserial_disconnect() does, but it
>>>> ought to be more or less equivalent to a "hangup" -- as the kerneldoc
>>>> says. If it doesn't do what users expect, there's probably a bug
>>>> somewhere.
>>>>
>>>> Of course, it's possible that the callback does not get invoked in
>>>> Laszlo's case. Then the question would be: Why not?
>>>
>>> Hmm, that is a good question. I wonder if there had been any recent
>>> fixes for that lately... I suppose that I will need to skim through
>>> the git log. Thank you for the hints!
>>
>> You should also add a printk statement to the disconnect callback so
>> that you can verify whether it really is getting called.
>
> Thanks. Should that also be called if I just boot up the board with
> Linux on it while the cable is not attached. In other words, the
> problem that I am also experiencing is that it blocks even when no
> cable is attached to the board and there has been no cable ever
> attached for the last boot.
You have everyone hopelessly confused about what your problem actually is.
Your $subject says that "g_serial hangs on write" and for a reproducer
you provide:
#include
#include
#include
int main()
{
const char buf[] = "Hello World!\n";
int fd = open("/dev/ttyGS0", O_RDWR | O_NONBLOCK);
int sent = write(fd, buf, sizeof(buf)-1);
close(fd);
return 0;
}
Only a careful reader would have caught this in your subsequent reply:
On Wed, Aug 05, 2015 at 04:40:21PM +0100, Laszlo Papp wrote:
> Wow, I managed to mess it up in my original email! So, this code above
> made it working, but without O_NONBLOCK, it was not working. Using
> O_NONBLOCK is my current nasty workaround.
To answer your original question, yes, the write() is behaving as expected.
If you open a tty without O_NONBLOCK, then both read() and write() to that
tty will _block_ until the operation is successful or the tty is hung up.
Your comparison with how ttyS* ports behave when a cable is not connected
is based on incorrect assumptions. When a ttyS port is in CLOCAL mode
(the default), the cable state is ignored when _opening_. On write(),
without hardware flow control (CRTSCTS), the serial port is successfully
writing the data out without knowing that nothing is listening.
I don't see a bug here.
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: USB - Generic Serial device : Unable to read more than 4095 bytes
On 08/06/2015 02:42 AM, arun k wrote: > / # lsusb -v > Bus 002 Device 006: ID 045b:0218 > Bus 001 Device 001: ID 1d6b:0002 > Bus 002 Device 001: ID 1d6b:0001 > Bus 003 Device 001: ID 1d6b:0002 > Bus 004 Device 001: ID 1d6b:0003 > > In this my device is > Bus 002 Device 006: ID 045b:0218 I have no idea what this device is, sorry. And the device descriptor dump below is not very helpful because it doesn't have the configurations. What is this for? > I enabled data dumping in usb_serial_generic_read_bulk_callback() > function and found data received in this function is losing. > In this function data is received as 64 bytes packets and two > continuous 64 bytes having no data loss but the next packets are lost > ie > 1st packet ok(64 byte) > 2nd packet ok(64 byte) > . > . packet lossing > . > 7th packet ok(64 byte) > 8th packet ok(64 byte) > . > . > In this way data loss is happening. Number of packets losing is > different not constant always. It's unclear what you are saying here; are you saying (a) the sending device is dropping data so the urbs do not contain the data you expect, or (b) all of the data is being received but the not all of it is being buffered. Either way, the fundamental problem you have is that your reader is not fast enough. What is your application? Have you tried a simple test jig that only reads data without processing it to see if the application is the problem? What h/w platform is this? You mentioned earlier you're running 3.4.35. That's really old; current 3.4 is .108. Where did you get this kernel? Regards, Peter Hurley PS - Please stop top-posting. See how I had to reiterate your linux version... >>> I think the generic USB driver ignores baud rate. Like Greg suggested, >>> >>you need to >>> use the proper driver for your hardware. > > I did't find any other suitable driver inside Linux. > I checked this device in windows in that usbser.sys is detecting(.inf > file is attached with the mail). > > Could you suggest any available drivers for my device. Sorry I am > very new to Linux drivers. > I will post my USB device descriptor values below > > [ 1291.908071] ## udev->descriptor.bLength 0x12 > [ 1291.908089] ## udev->descriptor.bDescriptorType 0x1 > [ 1291.908147] ## udev->descriptor.bcdUSB 0x200 > [ 1291.908165] ## udev->descriptor.bDeviceClass 0xef > [ 1291.908181] ## udev->descriptor.bDeviceSubClass 0x2 > [ 1291.908196] ## udev->descriptor.bDeviceProtocol 0x1 > [ 1291.908211] ## udev->descriptor.bMaxPacketSize0 0x40 > [ 1291.908226] ## udev->descriptor.idVendor 0x45b > [ 1291.908241] ## udev->descriptor.idProduct 0x218 > [ 1291.908255] ## udev->descriptor.bcdDevice 0x1 > [ 1291.908270] ## udev->descriptor.iManufacturer 0x0 > [ 1291.908284] ## udev->descriptor.iProduct 0x0 > [ 1291.908298] ## udev->descriptor.iSerialNumber 0x0 > [ 1291.908313] ## udev->descriptor.bNumConfigurations 0x1 > > Regards, > Arun > > > > > > > > > > > > On Wed, Aug 5, 2015 at 9:31 PM, Peter Hurley wrote: >> On 08/04/2015 11:57 PM, arun k wrote: >>>> Ok, but does the sending device know how to process in-band software flow >>>> control >>>> and is it set up to respond properly? >>> >>> I am not sure about this. I need to check this. >>> >>>> Also, I doubt software flow control is going to work @ 4Mbaud line rate. >>>> If you're stuck using software flow control, start with a 115kbaud line >>>> rate >>>> and see if that fixes your data loss problem >>> >>> I tried B115200 also but issue is still there. >> >> I think the generic USB driver ignores baud rate. Like Greg suggested, you >> need to >> use the proper driver for your hardware. >> >> What is the output from 'lsusb -v' and please identify in that output which >> device you are using. >> >>> I am using a USB device and that sending data at a rate of *409600 >>> bytes/sec*. Is this data rate have any relation with issue. >> >> Yes. Also, can you test this on a newer kernel like 3.18? >> >> Regards, >> Peter Hurley >> >>> On Wed, Aug 5, 2015 at 11:24 AM, Peter Hurley >> <mailto:pe...@hurleysoftware.com>> wrote: >>> >>> On 08/04/2015 10:00 PM, arun k wrote: >>> > I enabled software flow control like below >>> > >>> > tty.c_iflag |= (IXON | IXOFF | IXANY); >>> >>> Ok, but does the sending device know how to process in-band software >>>
Re: Unable to enumerate USB Device, error -110 Kernel 3.19-0.49-lowlatency netboot
On 03/04/2016 12:24 PM, Greg KH wrote: > On Fri, Mar 04, 2016 at 02:53:59PM -0500, Devon Ash wrote: >> Regarding Kernel 4.2.0-30-lowlatency, > > What is "lowlatency"? -lowlatency is Ubuntu's optional kernel config. A stock Ubuntu kernel is CONFIG_PREEMPT_VOLUNTARY=y && CONFIG_HZ=250 The -lowlatency kernel is CONFIG_PREEMPT=y && CONFIG_HZ=1000 That is the only difference. 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 5/5] TTY: fix tty_wait_until_sent maximum timeout
On 03/04/2015 04:39 AM, Johan Hovold wrote: > Currently tty_wait_until_sent may take up to twice as long as the > requested timeout while waiting for driver and hardware buffers to > drain. > > Fix this by taking the remaining number of jiffies after waiting for > driver buffers to drain into account so that the timeout actually > becomes a maximum timeout as it is documented to be. > > Note that this specifically implies tighter timings when closing a port > as a consequence of actually honouring the port closing-wait setting > for drivers relying on tty_wait_until_sent_from_close (e.g. via > tty_port_close_start). Reviewed-by: 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 2/5] TTY: bfin_jtag_comm: remove incorrect wait_until_sent operation
On 03/04/2015 04:39 AM, Johan Hovold wrote: > Remove incorrect and redundant wait_until_sent operation, which waits > for the driver buffer rather than any hardware buffers to drain, > something which is already taken care of by the tty layer (and > chars_in_buffer). Reviewed-by: 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 4/5] TTY: fix tty_wait_until_sent on 64-bit machines
On 03/04/2015 04:39 AM, Johan Hovold wrote: > Fix overflow bug in tty_wait_until_sent on 64-bit machines, where an > infinite timeout (0) would be passed to the underlying tty-driver's > wait_until_sent-operation as a negative timeout (-1), causing it to > return immediately. Wow, that is a nasty bug. > This manifests itself for example as tcdrain() returning immediately, > drivers not honouring the drain flags when setting terminal attributes, > or even dropped data on close as a requested infinite closing-wait > timeout would be ignored. > > The first symptom was reported by Asier LLANO who noted that tcdrain() > returned prematurely when using the ftdi_sio usb-serial driver. > > Fix this by passing 0 rather than MAX_SCHEDULE_TIMEOUT (LONG_MAX) to the > underlying tty driver. > > Note that the serial-core wait_until_sent-implementation is not affected > by this bug due to a lucky chance (comparison to an unsigned maximum > timeout), and neither is the cyclades one that had an explicit check for > negative timeouts, but all other tty drivers appear to be affected. Reviewed-by: 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
On 08/05/2015 01:36 PM, Peter Hurley wrote:
> On 07/22/2015 06:53 PM, Sven Brauch wrote:
>> On 23/07/15 00:12, Peter Hurley wrote:
>>> The premature unthrottle actually leads to the data loss but the throttling
>>> with a mere 2K left is _way too late_.
>> Ok, yes, I think so too.
>>
>>> 10ms is a _really_ long time for a cpu not to attend to a kworker.
>
> I haven't forgotten about this problem and I still plan to look into why
> the long delay, particularly focusing on the scheduling latency from
> tty_flip_buffer_push() => flush_to_ldisc().
I made some changes wrt which CPU is scheduled for the flush_to_ldisc()
input worker, which should reduce the kworker latency. The changes are
on the tty-next branch of Greg's tty.git tree; please test at your earliest
convenience.
>>> 1. What are the termios settings of the tty receiving input? Is it 'raw'
>>>mode or typical terminal mode (icanon, echo, etc.) or something else?
>> In my test code, I open the tty like
>> fd = open("/dev/ttyACM0", O_RDWR | O_NOCTTY | O_NONBLOCK);
>> I don't make any other changes to the default settings. To be honest,
>> I'm not sure in which mode it is operating then (I was assuming raw, but
>> I might be wrong?).
ECHO is on by default and the cdc-acm driver does not implement the
put_char() and flush_chars() tty driver methods, which made the problem
_way worse_, since every echoed char is sent as it's own URB.
Since echoing is performed by the input worker, this seems the likely
cause for the significant delay in ldisc processing.
Echo processing should probably be performed by a separate kworker rather
than by the input kworker.
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
On 10/21/2015 06:12 AM, Oliver Neukum wrote:
> On Tue, 2015-10-20 at 14:16 -0400, Peter Hurley wrote:
>> ECHO is on by default and the cdc-acm driver does not implement the
>> put_char() and flush_chars() tty driver methods, which made the
>> problem
>> _way worse_, since every echoed char is sent as it's own URB.
>
> That can be fixed. How can I test this?
I'm working on getting my tty tests organized for upstreaming, but it's
not there yet. Let me see if I can get you a standalone test for
accuracy, at least.
> Will the tty layer use write() and put_char()?
Echoing uses put_char() + flush_chars() for echoing, if the driver
supports it; otherwise, it uses write().
Be aware that while the N_TTY line discipline ensures that put_char()
and write() usage will not be interleaved without flush_chars(), other
line disciplines might not, so at least be sure that driver won't
crash if that happens.
Regards,
Peter Hurley
> From ac75a2c9ea67ec22c704a6bcaa30e6fc415d64d3 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum
> Date: Wed, 21 Oct 2015 12:10:07 +0200
> Subject: [PATCH] cdc-acm: implement put_char() and flush_chars()
>
> This should cut down latencies and waste if the tty layer writes single bytes.
>
> Signed-off-by: Oliver Neukum >oneu...@suse.com>
> ---
> drivers/usb/class/cdc-acm.c | 58
> +
> drivers/usb/class/cdc-acm.h | 1 +
> 2 files changed, 59 insertions(+)
>
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index b30e742..262f179 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -725,6 +725,62 @@ static int acm_tty_write(struct tty_struct *tty,
> return count;
> }
>
> +static void acm_tty_flush_chars(struct tty_struct *tty)
> +{
> + struct acm *acm = tty->driver_data;
> + struct acm_wb *cur = acm->putbuffer;
> + int err;
> + unsigned long flags;
> +
> + acm->putbuffer = NULL;
> + err = usb_autopm_get_interface_async(acm->control);
> + spin_lock_irqsave(&acm->write_lock, flags);
> + if (err < 0) {
> + cur->use = 0;
> + goto out;
> + }
> +
> + if (acm->susp_count) {
> + usb_anchor_urb(cur->urb, &acm->delayed);
> + goto out;
> + }
> +
> + acm_start_wb(acm, cur);
> +out:
> + spin_unlock_irqrestore(&acm->write_lock, flags);
> + return;
> +}
> +
> +static int acm_tty_put_char(struct tty_struct *tty, unsigned char ch)
> +{
> + struct acm *acm = tty->driver_data;
> + struct acm_wb *cur;
> + int wbn;
> + unsigned long flags;
> +
> +overflow:
> + cur = acm->putbuffer;
> + if (!cur) {
> + spin_lock_irqsave(&acm->write_lock, flags);
> + wbn = acm_wb_alloc(acm);
> + if (wbn >= 0) {
> + cur = &acm->wb[wbn];
> + acm->putbuffer = cur;
> + }
> + spin_unlock_irqrestore(&acm->write_lock, flags);
> + if (!cur)
> + return 0;
> + }
> +
> + if (cur->len == acm->writesize) {
> + acm_tty_flush_chars(tty);
> + goto overflow;
> + }
> +
> + cur->buf[cur->len++] = ch;
> + return 1;
> +}
> +
> static int acm_tty_write_room(struct tty_struct *tty)
> {
> struct acm *acm = tty->driver_data;
> @@ -1888,6 +1944,8 @@ static const struct tty_operations acm_ops = {
> .cleanup = acm_tty_cleanup,
> .hangup = acm_tty_hangup,
> .write =acm_tty_write,
> + .put_char = acm_tty_put_char,
> + .flush_chars = acm_tty_flush_chars,
> .write_room = acm_tty_write_room,
> .ioctl =acm_tty_ioctl,
> .throttle = acm_tty_throttle,
> diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
> index dd9af38..648a6f7 100644
> --- a/drivers/usb/class/cdc-acm.h
> +++ b/drivers/usb/class/cdc-acm.h
> @@ -94,6 +94,7 @@ struct acm {
> unsigned long read_urbs_free;
> struct urb *read_urbs[ACM_NR];
> struct acm_rb read_buffers[ACM_NR];
> + struct acm_wb *putbuffer; /* for
> acm_tty_put_char() */
> int rx_buflimit;
> int rx_endpoint;
> spinlock_t read_lock;
>
--
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
On 10/21/2015 08:09 AM, Peter Hurley wrote: > Be aware that while the N_TTY line discipline ensures that put_char() > and write() usage will not be interleaved without flush_chars(), other > line disciplines might not, so at least be sure that driver won't > crash if that happens. Sorry, I take that back: certain echoes are output via write() in OPOST mode, specifically for O_ONLCR and XTABS. -- 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: implement put_char() in cdc-acm
On 10/28/2015 07:04 AM, Oliver Neukum wrote: > On Tue, 2015-10-27 at 16:45 +0100, Sven Brauch wrote: >> Hey Oliver, >> >> On 27/10/15 16:07, Oliver Neukum wrote: >>> the theory that the lack of support for put_char() is a >>> major contributor to character loss in cdc-acm can be tested. >>> Sven, could you test the attached patch? It implements >>> the support. >> Thanks a lot for caring about this. I applied the patch to a plain linux >> 4.2 tree, but unfortunately I couldn't see any improvements. Applying > > That is unfortunate. So your problem is with not enough buffers. Well, not necessarily. As I wrote in the original thread, I changed the way input work is scheduled which should impact Sven's use-case. Those patches are on Greg's tty-next branch; afaik Sven has not tested with those. Sven, please test Oliver's patch on that tree. Also, please attach your target .config here. Lastly, please confirm your test method/termios settings (iow, are you using a reproducer or just 'cat big_file > /dev/ttyACM1') >> the patch I originally sent to the list for comparison still fixed the >> issue. > > Peter, what to do about this? 1. Re-evaluate the input kworker latency with my patches in tty-next. If the max input kworker latency can be cut in 1/2, then Sven's use case will be comfortably in only 1/2 the tty buffer space. I have a quick howto doc in progress for measuring that scheduling lag with ftrace, and a simple awk script for processing that ftrace output to aid in determining why. This is the fundamental problem. 2. Fix unthrottle Ever since my work on firewire tty, I've known throttle/unthrottle needed rework, but the i/o path and locking problems were more pressing. This is near top of my TODO list. > I think there are issues with Sven's > original patch. In particular I can't see how it can guarantee > progress. Is there a way to just really increase tty buffers, > e.g. quadruple them? Yes, this can be done per tty port (typically by the port driver install() method). Code fragment below doubles the tty buffer ceiling: retval = tty_buffer_set_limit(port, 128 * 1024); if (retval) goto error; > Sven, could we run a few more tests before we look at your patch > again? It really has issues, but it does help and nothing else > does. If necessary I will merge an improved version of your patch. I would much rather rework URB flow + unthrottle, as I previously outlined in the original thread instead of introducing another buffering layer. 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: implement put_char() in cdc-acm
On 10/28/2015 08:33 AM, Oliver Neukum wrote: > Peter, do you think I ought to upstream the support for put_char() ? Yes, but I owe you a test jig to validate it first. I'll do my best to get that to you today. 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: implement put_char() in cdc-acm
On 10/28/2015 11:53 AM, Sven Brauch wrote: > On 28/10/15 13:23, Peter Hurley wrote: >> Sven, please test Oliver's patch on that tree. > I will do as soon as I get around to it, I hope on the weekend. Ok. I'll get you the kworker scheduler latency profiling howto so we can find out what accounts for the scheduling lag, if still causing issues when you test that tree. >> Lastly, please confirm your test method/termios settings (iow, are >> you using a reproducer or just 'cat big_file > /dev/ttyACM1') > Sorry, what exactly do you mean by "reproducer"? I have a > microcontroller which acquires and transmits the data on the device end > of the USB connection. The data flows from device to host. Ok. (A reproducer in your case would be a _very simple_ host sink, and maybe a software simulation of the device.) Are you using an application to setup and log the host side? Or are you just using the shell (eg., 'cat /dev/ttyACM1 > log_file')? I ask because the termios settings on the host are relevant to the problem. If you're using an application, then you can capture the termios settings on the host side _while the app is acquiring_ with, eg.: sudo stty -a -F /dev/ttyACM1 >> I would much rather rework URB flow + unthrottle, as I previously >> outlined in the original thread instead of introducing another >> buffering layer. > From my non-kernel-dev point of view, this seems the way to go if the > strategy in my patch (technical flaws aside) is not acceptable. > Everything else, i.e. larger buffers or less delay, will certainly be a > welcome improvement but still does not guarantee data delivery. > > A very similar patch, by the way, was already submitted a few years ago > [1] but not accepted for similar reasons as brought up here (I only > found that thread later on). That patch has a more elegant > implementation than mine, so you might prefer reviving that, if it > becomes relevant. I've done similar things on very-high-speed serial connections as a stopgap, but I'd really like to solve this within the tty layer rather than by each very-high-speed driver. > I will be happy to test any fixes which come up, although I can't > promise I can get around to do it immediately. Thanks again. 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: implement put_char() in cdc-acm
On 11/01/2015 02:28 PM, Sven Brauch wrote: Hey, On 28/10/15 13:33, Oliver Neukum wrote: Sven, please test Oliver's patch on that tree. It can be found at https://kernel.googlesource.com/pub/scm/linux/kernel/git/gregkh/tty/ I finally got around to test this; sorry for the delay. I cloned that tree (git describe said v4.3-rc5), and applied Oliver's patch. With that kernel booted, the error occured much less frequently, but was still reproducable under load. With 4.2.5, I get a failure after a few hundred MB; with 4.3-rc5 plus patch, it transferred serveral gigabytes without errors, but failed when I started compiling something in the background as a test. So, I guess, this improves things but there's still no guarantee that the data sent by the device will actually be delivered. Querying for the stty options while the program is running gives this: # sudo stty -a -F /dev/ttyACM0 speed 9600 baud; rows 0; columns 0; line = 0; intr = ^C; quit = ^\; erase = ^?; kill = ^U; eof = ^D; eol = ; eol2 = ; swtch = ; start = ^Q; stop = ^S; susp = ^Z; rprnt = ^R; werase = ^W; lnext = ^V; discard = ^O; min = 0; time = 0; -parenb -parodd -cmspar cs8 hupcl -cstopb cread clocal -crtscts -ignbrk -brkint -ignpar -parmrk -inpck -istrip -inlcr -igncr -icrnl -ixon -ixoff -iuclc -ixany -imaxbel -iutf8 -opost -olcuc -ocrnl onlcr -onocr -onlret -ofill -ofdel nl0 cr0 tab0 bs0 vt0 ff0 -isig -icanon -iexten -echo -echoe -echok -echonl -noflsh -xcase -tostop -echoprt -echoctl -echoke -extproc If there's anything else which would be helpful for you to have tested, please let me know. Just the kernel .config of the host please. 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: implement put_char() in cdc-acm
On 11/02/2015 06:32 AM, Oliver Neukum wrote: > It looks like the throttling code isn't perfect yet Yeah, that's still a todo (ie., fixing the premature unthrottle). 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] usb: gadget: Add the console support for usb-to-serial port
Hi Baolin,
On 11/16/2015 02:05 AM, Baolin Wang wrote:
> It dose not work when we want to use the usb-to-serial port based
> on one usb gadget as a console. Thus this patch adds the console
> initialization to support this request.
I have some high level concerns.
1. I would defer registering the console until the port has at least been
allocated in gserial_alloc_line(), and unregister when the line is freed.
That also reduces many of the conditions that you shouldn't need to
check, like port number range and so on.
Further, consider deferring the console registration until gserial_connect();
that would further simplify things. In this case, unregistration would
happen at disconnect.
2. Why are you using a kworker for actual device i/o when all of the i/o
is done with interrupts disabled anyway?
Getting rid of the work would eliminate using the 8K intermediate buffer
as well.
3. If for some reason i/o deferral is really necessary, consider using a kthread
kworker instead of the pooled kworker. The scheduler response will be _way_
better.
4. If for some reason i/o deferral is really necessary, use a circular buffer
for the intermediate buffer, preferably lockless since there is only
one producer and one consumer.
Some other review comments below; please ignore anything other reviewers
have already noted.
Regards,
Peter Hurley
> Signed-off-by: Baolin Wang
> ---
> drivers/usb/gadget/Kconfig |6 +
> drivers/usb/gadget/function/u_serial.c | 238
>
> 2 files changed, 244 insertions(+)
>
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 33834aa..be5aab9 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -127,6 +127,12 @@ config USB_GADGET_STORAGE_NUM_BUFFERS
> a module parameter as well.
> If unsure, say 2.
>
> +config U_SERIAL_CONSOLE
> + bool "Serial gadget console support"
> + depends on USB_G_SERIAL
> + help
> +It supports the serial gadget can be used as a console.
> +
> source "drivers/usb/gadget/udc/Kconfig"
>
> #
> diff --git a/drivers/usb/gadget/function/u_serial.c
> b/drivers/usb/gadget/function/u_serial.c
> index f7771d8..4ade527 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
>
> #include "u_serial.h"
>
> @@ -79,6 +80,16 @@
> */
> #define QUEUE_SIZE 16
> #define WRITE_BUF_SIZE 8192/* TX only */
> +#define GS_BUFFER_SIZE (4096)
> +#define GS_CONSOLE_BUF_SIZE (2 * GS_BUFFER_SIZE)
> +
> +struct gscons_info {
> + struct gs_port *port;
> + struct tty_driver *tty_driver;
> + struct work_struct work;
> + int buf_tail;
> + charbuf[GS_CONSOLE_BUF_SIZE];
> +};
Make struct gscons_info a static declaration instead of
allocating it.
>
> /* circular buffer */
> struct gs_buf {
> @@ -118,6 +129,7 @@ struct gs_port {
>
> /* REVISIT this state ... */
> struct usb_cdc_line_coding port_line_coding;/* 8-N-1 etc */
> + struct usb_request *console_req;
> };
>
> static struct portmaster {
> @@ -1054,6 +1066,7 @@ gs_port_alloc(unsigned port_num, struct
> usb_cdc_line_coding *coding)
>
> port->port_num = port_num;
> port->port_line_coding = *coding;
> + port->console_req = NULL;
>
> ports[port_num].port = port;
> out:
> @@ -1143,6 +1156,227 @@ err:
> }
> EXPORT_SYMBOL_GPL(gserial_alloc_line);
>
> +#ifdef CONFIG_U_SERIAL_CONSOLE
> +
> +static struct usb_request *gs_request_new(struct usb_ep *ep, int buffer_size)
^^^
With only a single caller that uses a symbolic constant, is the
'buffer_size' parameter necessary?
> +{
> + struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
> +
Remove this newline.
> + if (!req)
> + return NULL;
> +
> + /* now allocate buffers for the requests */
Unnecessary comment.
> + req->buf = kmalloc(buffer_size, GFP_ATOMIC);
> + if (!req->buf) {
> + usb_ep_free_request(ep, req);
> + return NULL;
> + }
> +
> + return req;
> +}
> +
> +static void gs_request_free(struct usb_request *req, struct usb_ep *ep)
> +{
> + if (req) {
> + kfree(req->buf);
> + usb_ep_free_request(ep, req);
> + }
> +}
> +
> +static void g
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port
On 11/19/2015 01:48 AM, Baolin Wang wrote:
>>
>>> +{
>>> + struct gscons_info *info = gserial_cons.data;
>>> + int port_num = gserial_cons.index;
>>> + struct usb_request *req;
>>> + struct gs_port *port;
>>> + struct usb_ep *ep;
>>> +
>>> + if (port_num >= MAX_U_SERIAL_PORTS || port_num < 0) {
>>> + pr_err("%s: port num [%d] exceeds the range.\n",
>>> +__func__, port_num);
>>> + return -ENXIO;
>>> + }
>>> +
>>> + port = ports[port_num].port;
>>> + if (!port) {
>>> + pr_err("%s: serial line [%d] not allocated.\n",
>>> +__func__, port_num);
>>> + return -ENODEV;
>>> + }
>>> +
>>> + if (!port->port_usb) {
>>> + pr_err("%s: no port usb.\n", __func__);
>>> + return -ENODEV;
>>> + }
>>> +
>>> + ep = port->port_usb->in;
>>> + if (!ep) {
>>> + pr_err("%s: no usb endpoint.\n", __func__);
>>> + return -ENXIO;
>>> + }
>>
>> Looking at the caller, gserial_connect(), none of the error
>> conditions above look possible.
>>
>
> I re-look the code and do some tests, I found the checking is
> necessary. Cause we get the port number from the console->index, if
> the cmdline is not set the ttyGS0 as the console, the console->index
> will be -1 that is a wrong value. Also the serial.c file will create 1
> usb-to-seial port as default (default n_ports = 1), so we need to
> check the port and the endpoint of the port. So I think here checking
> is necessary and I have tested it.
static void gs_console_connect(int port_num)
{
.
.
if (port_num != gserial_cons.index)
return;
.
.
@@ -1219,6 +1453,7 @@ int gserial_connect(struct gserial *gser, u8 port_num)
gser->disconnect(gser);
}
+ status = gs_console_connect(port_num);
spin_unlock_irqrestore(&port->port_lock, flags);
return status;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port
On 11/18/2015 09:35 PM, Baolin Wang wrote:
> On 18 November 2015 at 23:32, Peter Hurley wrote:
>> Hi Baolin,
>>
>> On 11/16/2015 02:05 AM, Baolin Wang wrote:
>>> It dose not work when we want to use the usb-to-serial port based
>>> on one usb gadget as a console. Thus this patch adds the console
>>> initialization to support this request.
>>
>> I have some high level concerns.
>>
>> 1. I would defer registering the console until the port has at least been
>>allocated in gserial_alloc_line(), and unregister when the line is freed.
>>That also reduces many of the conditions that you shouldn't need to
>>check, like port number range and so on.
>
> The 'setup' callback of 'struct console' is just do some memory
> allocation and member initialization, that no need to defer
> registering the console in gserial_alloc_line(). But the
> 'gs_console_connect()' which will use the port need to be called in
> gserial_connect().
My point here was why are you registering the console before the port
table is even allocated and initialized? The console can't possibly
perform i/o that early because the port doesn't even exist.
Which is why I suggested waiting until gserial_alloc_line() to
register the console.
A typical console setup() performs the cross-reference linking between
the console data structure and the port table. The reason for that
is the console needs to be cleaned up if the port is being torn down.
For example, in gserial_disconnect() the port->port_usb is reset to NULL,
and later in gserial_console_exit():
if (port && port->port_usb) {
gs_request_free(req, ep);
}
which means your leaking the request allocation when the port has been
disconnected.
>>Further, consider deferring the console registration until
>> gserial_connect();
>>that would further simplify things. In this case, unregistration would
>>happen at disconnect.
>
> That sounds reasonable. I would try.
>
>>
>> 2. Why are you using a kworker for actual device i/o when all of the i/o
>>is done with interrupts disabled anyway?
>>Getting rid of the work would eliminate using the 8K intermediate buffer
>>as well.
>
> If remove the kworker, there are some deadlocks to call the printk()
> when in the process of transferring data with usb endpoint. So we need
> a async kworker to complete the io or it can not work.
The commit log should detail the major design choices, including why you
used the kworker (because of re-entrancy issues with usb endpoint).
>> 3. If for some reason i/o deferral is really necessary, consider using a
>> kthread
>>kworker instead of the pooled kworker. The scheduler response will be
>> _way_
>>better.
>>
>
> OK, make sense.
>
>> 4. If for some reason i/o deferral is really necessary, use a circular buffer
>>for the intermediate buffer, preferably lockless since there is only
>>one producer and one consumer.
>>
>
> Yeah, the circular buffer is better but more tricky. I would try.
>
>> Some other review comments below; please ignore anything other reviewers
>> have already noted.
>>
>> Regards,
>> Peter Hurley
>>
>>> Signed-off-by: Baolin Wang
>>> ---
>>> drivers/usb/gadget/Kconfig |6 +
>>> drivers/usb/gadget/function/u_serial.c | 238
>>>
>>> 2 files changed, 244 insertions(+)
>>>
>>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>>> index 33834aa..be5aab9 100644
>>> --- a/drivers/usb/gadget/Kconfig
>>> +++ b/drivers/usb/gadget/Kconfig
>>> @@ -127,6 +127,12 @@ config USB_GADGET_STORAGE_NUM_BUFFERS
>>> a module parameter as well.
>>> If unsure, say 2.
>>>
>>> +config U_SERIAL_CONSOLE
>>> + bool "Serial gadget console support"
>>> + depends on USB_G_SERIAL
>>> + help
>>> +It supports the serial gadget can be used as a console.
>>> +
>>> source "drivers/usb/gadget/udc/Kconfig"
>>>
>>> #
>>> diff --git a/drivers/usb/gadget/function/u_serial.c
>>> b/drivers/usb/gadget/function/u_serial.c
>>> index f7771d8..4ade527 100644
>>> --- a/drivers/usb/gadget/function/u_serial.c
>>> +++ b/drivers/usb/gadget/function/u_serial.c
>>> @@ -27,6 +27,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>>
>>&
Re: [PATCH 01/21] uas: replace WARN_ON_ONCE() with lockdep_assert_held()
[ +cc Peter Zijlstra, Ingo Molnar ]
On 09/10/2014 07:46 AM, Hans de Goede wrote:
> From: Sanjeev Sharma
>
> On some architecture spin_is_locked() always return false in
> uniprocessor configuration and therefore it would be advise to replace
> with lockdep_assert_held().
>
> Signed-off-by: Sanjeev Sharma
> Signed-off-by: Hans de Goede
> ---
> drivers/usb/storage/uas.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 3f42785..05b2d8e 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -154,7 +154,7 @@ static void uas_mark_cmd_dead(struct uas_dev_info
> *devinfo,
> struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
>
> uas_log_cmd_state(cmnd, caller);
> - WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> + lockdep_assert_held(&devinfo->lock);
This change isn't equivalent.
lockdep_assert_held() will continue to emit warnings; ie., there is no
"once" functionality. Same for the other changes below.
Regards,
Peter Hurley
> WARN_ON_ONCE(cmdinfo->state & COMMAND_ABORTED);
> cmdinfo->state |= COMMAND_ABORTED;
> cmdinfo->state &= ~IS_IN_WORK_LIST;
> @@ -181,7 +181,7 @@ static void uas_add_work(struct uas_cmd_info *cmdinfo)
> struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, SCp);
> struct uas_dev_info *devinfo = cmnd->device->hostdata;
>
> - WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> + lockdep_assert_held(&devinfo->lock);
> cmdinfo->state |= IS_IN_WORK_LIST;
> schedule_work(&devinfo->work);
> }
> @@ -283,7 +283,7 @@ static int uas_try_complete(struct scsi_cmnd *cmnd, const
> char *caller)
> struct uas_cmd_info *cmdinfo = (void *)&cmnd->SCp;
> struct uas_dev_info *devinfo = (void *)cmnd->device->hostdata;
>
> - WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> + lockdep_assert_held(&devinfo->lock);
> if (cmdinfo->state & (COMMAND_INFLIGHT |
> DATA_IN_URB_INFLIGHT |
> DATA_OUT_URB_INFLIGHT |
> @@ -622,7 +622,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
> struct urb *urb;
> int err;
>
> - WARN_ON_ONCE(!spin_is_locked(&devinfo->lock));
> + lockdep_assert_held(&devinfo->lock);
> if (cmdinfo->state & SUBMIT_STATUS_URB) {
> urb = uas_submit_sense_urb(cmnd, gfp, cmdinfo->stream);
> if (!urb)
>
--
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: PROBLEM: Mouse connected to USB-3 stopped working 2.6.38->39 regression
On 04/09/2016 08:00 AM, Alan Stern wrote: > On Sat, 9 Apr 2016, Sam Sany wrote: >> I have yet to try the 4.5 kernel. I prefer to use a low-latency kernel, bc >> the primary purpose of this machine is recording music. So I have been >> downloading the newest source code available in the Ubuntu Studio >> repositories, configuring with the xhci drivers modulized (by default Ubuntu >> makes them builtin), and compiling from that. If I download the source for >> 4.5, is there somewhere I can find a .config file that would allow for low- >> latency performance? > > What do you mean by low-latency kernel? I'm not aware of any > low-latency patches. > > There is the RT series, of course. But it's for bounded latency, not > low latency. And as far as I know, it's not available for 4.5. low-latency kernel is a packaged Ubuntu alternate kernel configured as CONFIG_PREEMPT=y + CONFIG_HZ=1000. The stock Ubuntu kernel is CONFIG_PREEMPT_VOLUNTARY=y + CONFIG_HZ=250. 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: USB-serial console and lockdep
Hi Johan,
On 11/18/2014 11:18 AM, Johan Hovold wrote:
> I get this missing-lockdep-annotation warning which I haven't seen
> before when booting with a usb-serial console on 3.18-rc5. It's been a
> while since I last tested this, though, and the tty_ldisc_ref wasn't
> introduced until 833efc0ed19c ("USB: serial: invoke dcd_change ldisc's
> handler.").
Sorry it took me so long to finally look at this -- at least I'm looking
at it in the same year ;) (in my tzone anyway)
Is this easily reproducible?
Because for lockdep to be trying to register the ldsem lock class
from the tty_ldisc_ref() means that no tty has yet been opened [see 1].
So how did the call to tty_port_tty_get() in pl2303_update_line_status()
return a tty?
If you can easily reproduce this, I can supply you with a debug patch
to find out what's going on.
Regards,
Peter Hurley
> [ 10.575225] usbserial: USB Serial support registered for pl2303
> [ 10.575561] pl2303 1-2.1:1.0: pl2303 converter detected
> [ 10.627563] usb 1-2.1: pl2303 converter now attached to ttyUSB0
> [ 10.650939] INFO: trying to register non-static key.
> [ 10.651000] the code is fine but needs lockdep annotation.
> [ 10.651000] turning off the locking correctness validator.
> [ 10.651031] CPU: 0 PID: 68 Comm: udevd Tainted: GW 3.18.0-rc5
> #10
> [ 10.651092] [] (unwind_backtrace) from []
> (show_stack+0x20/0x24)
> [ 10.651123] [] (show_stack) from []
> (dump_stack+0x24/0x28)
> [ 10.651184] [] (dump_stack) from []
> (__lock_acquire+0x1e50/0x2004)
> [ 10.651214] [] (__lock_acquire) from []
> (lock_acquire+0xe4/0x18c)
> [ 10.651245] [] (lock_acquire) from []
> (ldsem_down_read_trylock+0x78/0x90)
> [ 10.651275] [] (ldsem_down_read_trylock) from []
> (tty_ldisc_ref+0x24/0x58)
> [ 10.651306] [] (tty_ldisc_ref) from []
> (usb_serial_handle_dcd_change+0x48/0xe8)
> [ 10.651367] [] (usb_serial_handle_dcd_change) from []
> (pl2303_read_int_callback+0x210/0x220 [pl2303])
> [ 10.651428] [] (pl2303_read_int_callback [pl2303]) from
> [] (__usb_hcd_giveback_urb+0x80/0x140)
> [ 10.651458] [] (__usb_hcd_giveback_urb) from []
> (usb_giveback_urb_bh+0x98/0xd4)
> [ 10.651489] [] (usb_giveback_urb_bh) from []
> (tasklet_hi_action+0x9c/0x108)
> [ 10.651519] [] (tasklet_hi_action) from []
> (__do_softirq+0x148/0x42c)
> [ 10.651550] [] (__do_softirq) from []
> (irq_exit+0xd8/0x114)
> [ 10.651580] [] (irq_exit) from []
> (__handle_domain_irq+0x84/0xdc)
> [ 10.651611] [] (__handle_domain_irq) from []
> (omap_intc_handle_irq+0xd8/0xe0)
> [ 10.651641] [] (omap_intc_handle_irq) from []
> (__irq_svc+0x44/0x7c)
> [ 10.651641] Exception stack(0xdf4e7f08 to 0xdf4e7f50)
> [ 10.651672] 7f00: debc0b80 df4e7f5c
> debc0b80 be8da96c
> [ 10.651702] 7f20: 0128 c000fc84 df4e6000 df4e7f94
> 0004 df4e7f50
> [ 10.651702] 7f40: c038ebc0 c038d74c 600f0013
> [ 10.651733] [] (__irq_svc) from []
> (___sys_sendmsg.part.29+0x0/0x2e0)
> [ 10.651763] [] (___sys_sendmsg.part.29) from []
> (SyS_sendmsg+0x18/0x1c)
> [ 10.651794] [] (SyS_sendmsg) from []
> (ret_fast_syscall+0x0/0x48)
> [ 10.692871] console [ttyUSB0] enabled
[1] Call tree for registering the ldsem lock class
tty_open
tty_init_dev
alloc_tty_struct
init_ldsem (macro in include/linux/tty_ldisc.h)
* the static lock_class_key is expanded here *
--
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: USB-serial console and lockdep
On 01/03/2015 11:26 AM, Johan Hovold wrote:
> On Wed, Dec 31, 2014 at 09:07:59PM -0500, Peter Hurley wrote:
>> Hi Johan,
>>
>> On 11/18/2014 11:18 AM, Johan Hovold wrote:
>>> I get this missing-lockdep-annotation warning which I haven't seen
>>> before when booting with a usb-serial console on 3.18-rc5. It's been a
>>> while since I last tested this, though, and the tty_ldisc_ref wasn't
>>> introduced until 833efc0ed19c ("USB: serial: invoke dcd_change ldisc's
>>> handler.").
>>
>> Sorry it took me so long to finally look at this -- at least I'm looking
>> at it in the same year ;) (in my tzone anyway)
>
> No worries. Wasn't a top prio of mine either. :)
>
> Thanks for taking a look.
>
>> Is this easily reproducible?
>
> Yes, happens on every boot with the pl2303 driver.
>
>> Because for lockdep to be trying to register the ldsem lock class
>> from the tty_ldisc_ref() means that no tty has yet been opened [see 1].
>> So how did the call to tty_port_tty_get() in pl2303_update_line_status()
>> return a tty?
>
> Because the USB console driver is using a only partially initialised,
> "fake" tty struct to pass terminal settings to the underlying driver.
> So no wonder things can blow up.
Ahh, I did not know that.
> This particular issue can be fixed by making sure to initialise the
> ldisc semaphore, but there are likely more potential problems here,
> including use-after-free as the fake tty wasn't released using the
> kref. I'll post two fixes as a follow up.
>
> A more long term solution might be to rewrite all usb-serial drivers to
> handle a NULL termios and pass a ktermios to set_termios similar to how
> serial-core does this.
I agree that this definitely needs a more robust solution.
FWIW, I don't think serial-core is a particularly good model.
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
