Re: [PATCH 05/12] usb: usblp: use irqsave() in USB's complete callback

[Pete Zaitcev]

On Mon, 25 Jun 2018 00:08:38 +0200
Sebastian Andrzej Siewior  wrote:

> Use the _irqsave() variant of the locking primitives.

>   usblp->wcomplete = 1;
>   wake_up(>wwait);
> - spin_unlock(>lock);
> + spin_unlock_irqrestore(>lock, flags);

Code is correct, addresses the stated problem, no forgotten parts
found left to fix.

Acked-by: Pete Zaitcev 

-- P
--
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 05/22] USB: mon: no need to check return value of debugfs_create functions

[Pete Zaitcev]

On Tue, 29 May 2018 17:30:50 +0200
Greg Kroah-Hartman  wrote:

> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.

Okay, fair enough. And the code works, surprisingly, even when the
debugfs is disabled (well, according to my review). I just have one
question - wouldn't it be cleaner to deprecate and remove the text
API altogether?

-- Pete
--
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: Read text within supplied buffer size

[Pete Zaitcev]

This change fixes buffer overflows and silent data corruption with the
usbmon device driver text file read operations.

Signed-off-by: Fredrik Noring <nor...@nocrew.org>
Signed-off-by: Pete Zaitcev <zait...@redhat.com>

---
diff --git a/drivers/usb/mon/mon_text.c b/drivers/usb/mon/mon_text.c
index f5e1bb5e5217..984f7e12a6a5 100644
--- a/drivers/usb/mon/mon_text.c
+++ b/drivers/usb/mon/mon_text.c
@@ -85,6 +85,8 @@ struct mon_reader_text {
 
wait_queue_head_t wait;
int printf_size;
+   size_t printf_offset;
+   size_t printf_togo;
char *printf_buf;
struct mutex printf_lock;
 
@@ -376,75 +378,103 @@ static int mon_text_open(struct inode *inode, struct 
file *file)
return rc;
 }
 
-/*
- * For simplicity, we read one record in one system call and throw out
- * what does not fit. This means that the following does not work:
- *   dd if=/dbg/usbmon/0t bs=10
- * Also, we do not allow seeks and do not bother advancing the offset.
- */
+static ssize_t mon_text_copy_to_user(struct mon_reader_text *rp,
+char __user * const buf, const size_t nbytes)
+{
+   const size_t togo = min(nbytes, rp->printf_togo);
+
+   if (copy_to_user(buf, >printf_buf[rp->printf_offset], togo))
+   return -EFAULT;
+   rp->printf_togo -= togo;
+   rp->printf_offset += togo;
+   return togo;
+}
+
+/* ppos is not advanced since the llseek operation is not permitted. */
 static ssize_t mon_text_read_t(struct file *file, char __user *buf,
-   size_t nbytes, loff_t *ppos)
+size_t nbytes, loff_t *ppos)
 {
struct mon_reader_text *rp = file->private_data;
struct mon_event_text *ep;
struct mon_text_ptr ptr;
+   ssize_t ret;
 
-   ep = mon_text_read_wait(rp, file);
-   if (IS_ERR(ep))
-   return PTR_ERR(ep);
mutex_lock(>printf_lock);
-   ptr.cnt = 0;
-   ptr.pbuf = rp->printf_buf;
-   ptr.limit = rp->printf_size;
-
-   mon_text_read_head_t(rp, , ep);
-   mon_text_read_statset(rp, , ep);
-   ptr.cnt += snprintf(ptr.pbuf + ptr.cnt, ptr.limit - ptr.cnt,
-   " %d", ep->length);
-   mon_text_read_data(rp, , ep);
-
-   if (copy_to_user(buf, rp->printf_buf, ptr.cnt))
-   ptr.cnt = -EFAULT;
+
+   if (rp->printf_togo == 0) {
+
+   ep = mon_text_read_wait(rp, file);
+   if (IS_ERR(ep)) {
+   mutex_unlock(>printf_lock);
+   return PTR_ERR(ep);
+   }
+   ptr.cnt = 0;
+   ptr.pbuf = rp->printf_buf;
+   ptr.limit = rp->printf_size;
+
+   mon_text_read_head_t(rp, , ep);
+   mon_text_read_statset(rp, , ep);
+   ptr.cnt += snprintf(ptr.pbuf + ptr.cnt, ptr.limit - ptr.cnt,
+   " %d", ep->length);
+   mon_text_read_data(rp, , ep);
+
+   rp->printf_togo = ptr.cnt;
+   rp->printf_offset = 0;
+
+   kmem_cache_free(rp->e_slab, ep);
+   }
+
+   ret = mon_text_copy_to_user(rp, buf, nbytes);
mutex_unlock(>printf_lock);
-   kmem_cache_free(rp->e_slab, ep);
-   return ptr.cnt;
+   return ret;
 }
 
+/* ppos is not advanced since the llseek operation is not permitted. */
 static ssize_t mon_text_read_u(struct file *file, char __user *buf,
-   size_t nbytes, loff_t *ppos)
+size_t nbytes, loff_t *ppos)
 {
struct mon_reader_text *rp = file->private_data;
struct mon_event_text *ep;
struct mon_text_ptr ptr;
+   ssize_t ret;
 
-   ep = mon_text_read_wait(rp, file);
-   if (IS_ERR(ep))
-   return PTR_ERR(ep);
mutex_lock(>printf_lock);
-   ptr.cnt = 0;
-   ptr.pbuf = rp->printf_buf;
-   ptr.limit = rp->printf_size;
 
-   mon_text_read_head_u(rp, , ep);
-   if (ep->type == 'E') {
-   mon_text_read_statset(rp, , ep);
-   } else if (ep->xfertype == USB_ENDPOINT_XFER_ISOC) {
-   mon_text_read_isostat(rp, , ep);
-   mon_text_read_isodesc(rp, , ep);
-   } else if (ep->xfertype == USB_ENDPOINT_XFER_INT) {
-   mon_text_read_intstat(rp, , ep);
-   } else {
-   mon_text_read_statset(rp, , ep);
+   if (rp->printf_togo == 0) {
+
+   ep = mon_text_read_wait(rp, file);
+   if (IS_ERR(ep)) {
+   mutex_unlock(>printf_lock);
+   return PTR_ERR(ep);
+   }
+   ptr.cnt = 0;
+   ptr.pbuf = rp->printf_buf;
+   ptr.limit = rp->printf_size;
+
+   mon_text_read_head_u(rp, , ep);
+   if (ep->type == 'E') {
+   mon_text_read_statset(rp, , ep);
+   } else if (ep->xfertype == USB

Re: [PATCH 2/6] USB: move many drivers to use DEVICE_ATTR_RO

[Pete Zaitcev]

On Tue, 23 Jan 2018 11:24:06 +0100
Greg Kroah-Hartman <gre...@linuxfoundation.org> wrote:

> +++ b/drivers/usb/class/usblp.c

> -static ssize_t usblp_show_ieee1284_id(struct device *dev, struct 
> device_attribute *attr, char *buf)
> +static ssize_t ieee1284_id_show(struct device *dev, struct device_attribute 
> *attr, char *buf)

> -static DEVICE_ATTR(ieee1284_id, S_IRUGO, usblp_show_ieee1284_id, NULL);
> +static DEVICE_ATTR_RO(ieee1284_id);

You know, I hate this interface a little: it's too implicit. Before,
one could hit '*' in vi and get to the function that implements it.
Now, you have to know the foo_show convention. But if you think
it's worth being a bit more explicit about "this is intended to
be a RO attribute", then okay.

Acked-by: Pete Zaitcev <zait...@redhat.com>

-- P
--
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] The usbmon triggers a BUG in ./include/linux/mm.h

[Pete Zaitcev]

Automated tests triggered this by opening usbmon and accessing the
mmap while simultaneously resizing the buffers. This bug was with
us since 2006, because typically applications only size the buffers
once and thus avoid racing. Reported by Kirill A. Shutemov.

Signed-off-by: Pete Zaitcev <zait...@redhat.com>
---
 drivers/usb/mon/mon_bin.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index f6ae753ab99b..f932f40302df 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -1004,7 +1004,9 @@ static long mon_bin_ioctl(struct file *file, unsigned int 
cmd, unsigned long arg
break;
 
case MON_IOCQ_RING_SIZE:
+   mutex_lock(>fetch_lock);
ret = rp->b_size;
+   mutex_unlock(>fetch_lock);
break;
 
case MON_IOCT_RING_SIZE:
@@ -1231,12 +1233,16 @@ static int mon_bin_vma_fault(struct vm_fault *vmf)
unsigned long offset, chunk_idx;
struct page *pageptr;
 
+   mutex_lock(>fetch_lock);
offset = vmf->pgoff << PAGE_SHIFT;
-   if (offset >= rp->b_size)
+   if (offset >= rp->b_size) {
+   mutex_unlock(>fetch_lock);
return VM_FAULT_SIGBUS;
+   }
chunk_idx = offset / CHUNK_SIZE;
pageptr = rp->b_vec[chunk_idx].pg;
get_page(pageptr);
+   mutex_unlock(>fetch_lock);
vmf->page = pageptr;
return 0;
 }
-- 
2.14.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: kernel BUG at ./include/linux/mm.h:LINE! (3)

[Pete Zaitcev]

On Wed, 3 Jan 2018 12:26:04 +0300
"Kirill A. Shutemov" <kir...@shutemov.name> wrote:

> > > - unsigned long offset, chunk_idx;
> > > + unsigned long offset, chunk_idx, flags;
> > >   struct page *pageptr;
> > >  
> > > + mutex_lock(>fetch_lock);
> > > + spin_lock_irqsave(>b_lock, flags);
> > >   offset = vmf->pgoff << PAGE_SHIFT;

> > I think that grabbing the spinlock is not really necessary in
> > this case. The ->b_lock is designed for things that are accessed
> > from interrupts that Host Controller Driver serves -- mostly
> > various pointers. By defintion it's not covering things that
> > are related to re-allocation. Now, the re-allocation itself
> > grabs it, because it resets indexes into the new buffer, but
> > does not appear to apply here, does it now?  
> 
> Please, double check everything. I remember that the mutex wasn't enough
> to stop bug from triggering. But I didn't spend much time understanding
> the code.

Attached is the test that I used to reproduce the problem, and my
patch with just the ->fetch_lock fixes it. That said, your test
suite may be more comprehensive, or you may have a device that
generates enough USB traffic with associated monitoring callbacks.
But I don't see it.

At this point, I'm going to post the patch with a Signed-Off-By.

-- Pete

(this reproduces very quickly)

/*
 * usbmontest: The crash test for usbmon, crashes kernel 4.15
 *
 * Copyright (c) 2007 Red Hat, Inc.
 * Copyright (c) 2016 Mike Frysinger <vap...@gentoo.org>
 * Copyright (c) 2018 Pete Zaitcev <zait...@redhat.com>
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 2 as
 * published by the Free Software Foundation.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License along
 * with this program; if not, write to the Free Software Foundation, Inc.,
 * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 */
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

#define TAG "usbmontest"

#ifdef __GNUC__
#define __unused __attribute__((unused))
#else
#define __unused /**/
#endif

#define MON_IOC_MAGIC 0x92

#define MON_IOCG_STATS _IOR(MON_IOC_MAGIC, 3, struct usbmon_stats)

#define MON_IOCT_RING_SIZE _IO(MON_IOC_MAGIC, 4)

#define MON_IOCQ_RING_SIZE _IO(MON_IOC_MAGIC, 5)

struct usbmon_mfetch_arg {
unsigned int *offvec;   /* Vector of events fetched */
unsigned int nfetch;/* Num. of events to fetch / fetched */
unsigned int nflush;/* Number of events to flush */
};

#define MON_IOCX_MFETCH _IOWR(MON_IOC_MAGIC, 7, struct usbmon_mfetch_arg)

/*
 */
struct params {
int ifnum;  /* USB bus number */
char *devname;  /* /dev/usbmonN */

unsigned int map_size;
};
const unsigned int page_size = 8192;/* 2x the size on x86, cross-platform */
const unsigned int num_pages = 3;

void Usage(void);

void parse_params(struct params *p, char **argv);
void make_device(const struct params *p);
int find_major(void);

struct params par;

int main(int argc __unused, char **argv)
{
int fd;
unsigned char *data_buff;
unsigned int n;
int wstat;
int rc;

parse_params(, argv+1);

/*
 * Two reasons to do this:
 * 1. Reduce weird error messages.
 * 2. If we create device nodes, we want them owned by root.
 */
if (geteuid() != 0) {
fprintf(stderr, TAG ": Must run as root\n");
exit(1);
}

if ((fd = open(par.devname, O_RDWR)) == -1) {
if (errno == ENOENT) {
make_device();
fd = open(par.devname, O_RDWR);
}
if (fd == -1) {
if (errno == ENODEV && par.ifnum == 0) {
fprintf(stderr, TAG
": Can't open pseudo-bus zero at %s"
" (probably not supported by kernel)\n",
par.devname);
} else {
fprintf(stderr, TAG ": Can't open %s: %s\n",
par.devname, strerror(errno));
}
exit(1);
}
}

rc = ioctl(fd, MON_IOCT_RING

Re: kernel BUG at ./include/linux/mm.h:LINE! (3)

[Pete Zaitcev]

On Fri, 29 Dec 2017 16:24:20 +0300
"Kirill A. Shutemov"  wrote:

> Looks like MON_IOCT_RING_SIZE reallocates ring buffer without any
> serialization wrt mon_bin_vma_fault(). By the time of get_page() the page
> may be freed.

As an update: I tried to make a smaller test for this, but was unsuccessful
so far. I'll poke a bit at it later, but it may take me some time.

-- Pete
--
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: kernel BUG at ./include/linux/mm.h:LINE! (3)

[Pete Zaitcev]

On Wed, 3 Jan 2018 13:08:12 -0800
Matthew Wilcox  wrote:

> > +   mutex_lock(>fetch_lock);
> > offset = vmf->pgoff << PAGE_SHIFT;
> > if (offset >= rp->b_size)
> > +   mutex_unlock(>fetch_lock);
> > return VM_FAULT_SIGBUS;
> > chunk_idx = offset / CHUNK_SIZE;  
> 
> missing braces ... maybe you'd rather a 'goto sigbus' approach?

Thanks. What a way to return to kernel programming for me. I'm going
to test it anyway, already started to unpacking a PC, but yeah...

-- Pete
--
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: kernel BUG at ./include/linux/mm.h:LINE! (3)

[Pete Zaitcev]

On Wed, 3 Jan 2018 12:26:04 +0300
"Kirill A. Shutemov"  wrote:

> > > +++ b/drivers/usb/mon/mon_bin.c
> > > @@ -1228,15 +1228,24 @@ static void mon_bin_vma_close(struct 
> > > vm_area_struct *vma)
> > >  static int mon_bin_vma_fault(struct vm_fault *vmf)
> > >  {
> > >   struct mon_reader_bin *rp = vmf->vma->vm_private_data;
> > > - unsigned long offset, chunk_idx;
> > > + unsigned long offset, chunk_idx, flags;
> > >   struct page *pageptr;
> > >  
> > > + mutex_lock(>fetch_lock);
> > > + spin_lock_irqsave(>b_lock, flags);
> > >   offset = vmf->pgoff << PAGE_SHIFT;
> > > - if (offset >= rp->b_size)
> > > + if (offset >= rp->b_size) {
> > > + spin_unlock_irqrestore(>b_lock, flags);
> > > + mutex_unlock(>fetch_lock);
> > >   return VM_FAULT_SIGBUS;
> > > + }
> > >   chunk_idx = offset / CHUNK_SIZE;
> > > +
> > >   pageptr = rp->b_vec[chunk_idx].pg;
> > >   get_page(pageptr);
> > > + spin_unlock_irqrestore(>b_lock, flags);
> > > + mutex_unlock(>fetch_lock);
> > > +
> > >   vmf->page = pageptr;
> > >   return 0;
> > >  }  
> > 
> > I think that grabbing the spinlock is not really necessary in
> > this case. [...]
> 
> Please, double check everything. I remember that the mutex wasn't enough
> to stop bug from triggering. But I didn't spend much time understanding
> the code.

I just don't understand why. The only two fields that are used
in the fault routine are rp->b_vec and rp->b_size. They are
protected by the mutex rp->fetch_lock. I don't see anything else
can spill into these fields by dirtying adjacent words in memory,
either except this:

case MON_IOCQ_RING_SIZE:
ret = rp->b_size;
break;

In the old days, this was safe, but who knows what CPUs do today.
It needs the same mutex taken around the read-only reference too.
How about this:

diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index f6ae753ab99b..cb3612f28804 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -1004,7 +1004,9 @@ static long mon_bin_ioctl(struct file *file, unsigned int 
cmd, unsigned long arg
break;
 
case MON_IOCQ_RING_SIZE:
+   mutex_lock(>fetch_lock);
ret = rp->b_size;
+   mutex_unlock(>fetch_lock);
break;
 
case MON_IOCT_RING_SIZE:
@@ -1231,12 +1233,15 @@ static int mon_bin_vma_fault(struct vm_fault *vmf)
unsigned long offset, chunk_idx;
struct page *pageptr;
 
+   mutex_lock(>fetch_lock);
offset = vmf->pgoff << PAGE_SHIFT;
if (offset >= rp->b_size)
+   mutex_unlock(>fetch_lock);
return VM_FAULT_SIGBUS;
chunk_idx = offset / CHUNK_SIZE;
pageptr = rp->b_vec[chunk_idx].pg;
get_page(pageptr);
+   mutex_unlock(>fetch_lock);
vmf->page = pageptr;
return 0;
 }

-- Pete
--
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: kernel BUG at ./include/linux/mm.h:LINE! (3)

[Pete Zaitcev]

On Fri, 29 Dec 2017 16:24:20 +0300
"Kirill A. Shutemov"  wrote:

> Looks like MON_IOCT_RING_SIZE reallocates ring buffer without any
> serialization wrt mon_bin_vma_fault(). By the time of get_page() the page
> may be freed.

Okay. Who knew that you could fork while holding an open descriptor. :-)

> The patch below seems help the crash to go away, but I *think* more work
> is required. For instance, after ring buffer reallocation the old pages
> will stay mapped. Nothing pulls them.

You know, this bothered me all these years too, but I was assured
back in the day (as much as I can remember), that doing get_page()
in the .fault() is just the right thing. In my defense, you can
see other drivers doing it, such as:

./drivers/char/agp/alpha-agp.c
./drivers/hsi/clients/cmt_speech.c

I'd appreciate insight from someone who knows how VM subsystem works.

Now, about the code:

> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index f6ae753ab99b..ac168fecf04f 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -1228,15 +1228,24 @@ static void mon_bin_vma_close(struct vm_area_struct 
> *vma)
>  static int mon_bin_vma_fault(struct vm_fault *vmf)
>  {
>   struct mon_reader_bin *rp = vmf->vma->vm_private_data;
> - unsigned long offset, chunk_idx;
> + unsigned long offset, chunk_idx, flags;
>   struct page *pageptr;
>  
> + mutex_lock(>fetch_lock);
> + spin_lock_irqsave(>b_lock, flags);
>   offset = vmf->pgoff << PAGE_SHIFT;
> - if (offset >= rp->b_size)
> + if (offset >= rp->b_size) {
> + spin_unlock_irqrestore(>b_lock, flags);
> + mutex_unlock(>fetch_lock);
>   return VM_FAULT_SIGBUS;
> + }
>   chunk_idx = offset / CHUNK_SIZE;
> +
>   pageptr = rp->b_vec[chunk_idx].pg;
>   get_page(pageptr);
> + spin_unlock_irqrestore(>b_lock, flags);
> + mutex_unlock(>fetch_lock);
> +
>   vmf->page = pageptr;
>   return 0;
>  }

I think that grabbing the spinlock is not really necessary in
this case. The ->b_lock is designed for things that are accessed
from interrupts that Host Controller Driver serves -- mostly
various pointers. By defintion it's not covering things that
are related to re-allocation. Now, the re-allocation itself
grabs it, because it resets indexes into the new buffer, but
does not appear to apply here, does it now?

-- Pete
--
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/3] USB: serial: mct_u232: fix big-endian baud-rate handling

[Pete Zaitcev]

On Thu, 11 May 2017 11:41:20 +0200
Johan Hovold <jo...@kernel.org> wrote:

> Drop erroneous cpu_to_le32 when setting the baud rate, something which
> corrupted the divisor on big-endian hosts.

> +++ b/drivers/usb/serial/mct_u232.c
> @@ -189,7 +189,7 @@ static int mct_u232_set_baud_rate(struct tty_struct *tty,
>   divisor = mct_u232_calculate_baud_rate(serial, value, );
> - put_unaligned_le32(cpu_to_le32(divisor), buf);
> + put_unaligned_le32(divisor, buf);
>   rc = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),

Acked-By: Pete Zaitcev <zait...@yahoo.com>

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

Re: [PATCH 1/1] usblp: do not set TASK_INTERRUPTIBLE before lock

[Pete Zaitcev]

On Mon,  2 Nov 2015 10:27:00 +0100
Jiri Slaby <jsl...@suse.cz> wrote:

> Signed-off-by: Jiri Slaby <jsl...@suse.cz>
> --- a/drivers/usb/class/usblp.c
> +++ b/drivers/usb/class/usblp.c
> @@ -884,11 +884,11 @@ static int usblp_wwait(struct usblp *usblp, int 
> nonblock)
>  
>   add_wait_queue(>wwait, );
>   for (;;) {
> - set_current_state(TASK_INTERRUPTIBLE);
>   if (mutex_lock_interruptible(>mut)) {
>   rc = -EINTR;
>   break;
>   }
> + set_current_state(TASK_INTERRUPTIBLE);
>   rc = usblp_wtest(usblp, nonblock);
>   mutex_unlock(>mut);
>   if (rc <= 0)

I'm fully onboard with this. In the original "big cleanup" 317c67b8f,
I got this right. But then I either missed that mutex_lock_interruptible()
can set the state, or it didn't do it back then (it was in 2007), and
the 7f477358e introduced the existing code.

Acked-By: Pete Zaitcev <zait...@yahoo.com>

-- Pete
--
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: class: Use USB_CLASS_PRINTER instead of number 7

[Pete Zaitcev]

On Fri, 19 Jun 2015 11:37:08 +0200
Krzysztof Opasiak k.opas...@samsung.com wrote:

 Kernel provides very nice defines for USB device class
 so it's a good idea to use them in suitable places.
 It is much easier to grep for such define instead of 7.

  static const struct usb_device_id usblp_ids[] = {
 - { USB_INTERFACE_INFO(7, 1, 3) },
 + { USB_INTERFACE_INFO(USB_CLASS_PRINTER, 1, 3) },
   { USB_DEVICE(0x04b8, 0x0202) }, /* Seiko Epson Receipt Printer M129C */

If you're concerned about grepping, then fix all other places like
usblp_select_alts(), too.

-- Pete
--
To unsubscribe from this list: send the line unsubscribe linux-usb in

Re: [PATCH v1 05/49] USB: usblp: prepare for enabling irq in complete()

[Pete Zaitcev]

On Sun, 18 Aug 2013 00:24:30 +0800
Ming Lei ming@canonical.com wrote:

 Complete() will be run with interrupt enabled, so change to
 spin_lock_irqsave().
 
 Signed-off-by: Pete Zaitcev zait...@kotori.zaitcev.us
 Signed-off-by: Ming Lei ming@canonical.com

Still looks good.

-- P
--
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 03/50] USB: usblp: spin_lock in complete() cleanup

[Pete Zaitcev]

On Thu, 11 Jul 2013 17:05:26 +0800
Ming Lei ming@canonical.com wrote:

 Complete() will be run with interrupt enabled, so change to
 spin_lock_irqsave().
 
 Cc: Pete Zaitcev zait...@redhat.com
 Signed-off-by: Ming Lei ming@canonical.com

Signed-off-by: Pete Zaitcev zait...@kotori.zaitcev.us

Good luck with that, Ming. I think the spin_lock_irqsave thing should've
been done back in days of uhci-hcd. But we always had some super annoying
edge cases when we considered it previously. I see Alan Stern signed off
the HCD changes at the beginning of July, so you're on the right track.

BTW, perfect formatting in usblp.c, too bad for Sergei's heartburn ^_^

-- Pete

P.S. Is it just me, or you forgot to fix usb-skeleton.c in your 50
patches?
--
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: prevent overlapping access by usb-storage and usbfs

[Pete Zaitcev]

On Mon, 14 Jan 2013 12:23:05 -0800
Greg KH gre...@linuxfoundation.org wrote:

 Forgot to mention the side effect of the patch: one can't submit read and
  write URB simultaneously via USBDEVFS_BULK ioctl(). That has been dealt in 
  2.4
  by later patch by Pete, which I can try to port if needed.
 
 That's not good, it would need to be part of this patch, we don't want
 to break that existing functionality.

I knew that it was no good, but hoped to skate by :-). My fix for Sergey's
patch looked like this, with a bitmask:

diff -urp -X dontdiff linux-2.4.33-rc3/drivers/usb/devices.c 
linux-2.4.33-rc3-u/drivers/usb/devices.c
--- linux-2.4.33-rc3/drivers/usb/devices.c  2004-11-17 03:54:21.0 
-0800
+++ linux-2.4.33-rc3-u/drivers/usb/devices.c2006-08-04 14:56:11.0 
-0700
@@ -392,7 +392,7 @@ static char *usb_dump_desc(char *start, 
 * Grab device's exclusive_access mutex to prevent its driver or
 * devio from using this device while we are accessing it.
 */
-   down (dev-exclusive_access);
+   usb_excl_lock(dev, 3, 0);
 
start = usb_dump_device_descriptor(start, end, dev-descriptor);
 
@@ -411,7 +411,7 @@ static char *usb_dump_desc(char *start, 
}
 
 out:
-   up (dev-exclusive_access);
+   usb_excl_unlock(dev, 3);
return start;
 }
 
diff -urp -X dontdiff linux-2.4.33-rc3/drivers/usb/devio.c 
linux-2.4.33-rc3-u/drivers/usb/devio.c
--- linux-2.4.33-rc3/drivers/usb/devio.c2006-04-13 19:02:30.0 
-0700
+++ linux-2.4.33-rc3-u/drivers/usb/devio.c  2006-08-04 14:56:11.0 
-0700
@@ -623,7 +623,12 @@ static int proc_bulk(struct dev_state *p
free_page((unsigned long)tbuf);
return -EINVAL;
}
+   if (usb_excl_lock(dev, 1, 1) != 0) {
+   free_page((unsigned long)tbuf);
+   return -ERESTARTSYS;
+   }
i = usb_bulk_msg(dev, pipe, tbuf, len1, len2, tmo);
+   usb_excl_unlock(dev, 1);
if (!i  len2) {
if (copy_to_user(bulk.data, tbuf, len2)) {
free_page((unsigned long)tbuf);
@@ -637,7 +642,12 @@ static int proc_bulk(struct dev_state *p
return -EFAULT;
}
}
+   if (usb_excl_lock(dev, 2, 1) != 0) {
+   free_page((unsigned long)tbuf);
+   return -ERESTARTSYS;
+   }
i = usb_bulk_msg(dev, pipe, tbuf, len1, len2, tmo);
+   usb_excl_unlock(dev, 2);
}
free_page((unsigned long)tbuf);
if (i  0) {
@@ -1160,12 +1170,6 @@ static int usbdev_ioctl_exclusive(struct
inode-i_mtime = CURRENT_TIME;
break;
 
-   case USBDEVFS_BULK:
-   ret = proc_bulk(ps, (void *)arg);
-   if (ret = 0)
-   inode-i_mtime = CURRENT_TIME;
-   break;
-
case USBDEVFS_RESETEP:
ret = proc_resetep(ps, (void *)arg);
if (ret = 0)
@@ -1259,8 +1263,13 @@ static int usbdev_ioctl(struct inode *in
ret = proc_disconnectsignal(ps, (void *)arg);
break;
 
-   case USBDEVFS_CONTROL:
case USBDEVFS_BULK:
+   ret = proc_bulk(ps, (void *)arg);
+   if (ret = 0)
+   inode-i_mtime = CURRENT_TIME;
+   break;
+
+   case USBDEVFS_CONTROL:
case USBDEVFS_RESETEP:
case USBDEVFS_RESET:
case USBDEVFS_CLEAR_HALT:
@@ -1272,9 +1281,9 @@ static int usbdev_ioctl(struct inode *in
case USBDEVFS_RELEASEINTERFACE:
case USBDEVFS_IOCTL:
ret = -ERESTARTSYS;
-   if (down_interruptible(ps-dev-exclusive_access) == 0) {
+   if (usb_excl_lock(ps-dev, 3, 1) == 0) {
ret = usbdev_ioctl_exclusive(ps, inode, cmd, arg);
-   up(ps-dev-exclusive_access);
+   usb_excl_unlock(ps-dev, 3);
}
break;
 
diff -urp -X dontdiff linux-2.4.33-rc3/drivers/usb/storage/transport.c 
linux-2.4.33-rc3-u/drivers/usb/storage/transport.c
--- linux-2.4.33-rc3/drivers/usb/storage/transport.c2005-04-03 
18:42:19.0 -0700
+++ linux-2.4.33-rc3-u/drivers/usb/storage/transport.c  2006-08-04 
14:35:15.0 -0700
@@ -628,16 +628,16 @@ void usb_stor_invoke_transport(Scsi_Cmnd
int result;
 
/*
-* Grab device's exclusive_access mutex to prevent libusb/usbfs from
+* Grab device's exclusive access lock to prevent libusb/usbfs from
 * sending out a command in the middle of ours (if libusb sends a
 * get_descriptor or something on pipe 0 after our CBW and before
 * our CSW, and then we get a stall, we have trouble).
 */
-   

Re: [Patch v2] block: remove the deprecated ub driver

[Pete Zaitcev]

On Sun, 26 Aug 2012 14:12:17 -0700
Greg Kroah-Hartman gre...@linuxfoundation.org wrote:

  Unless you have other plans, I would take this and send it you together 
  with the
  removal of libusual as you asked.
 
 Why not just send a follow-on patch to do the libusual fixups instead?

Sending it in one go seems more self-consistent, but either way is
fine by me.

-- Pete
--
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: Huawei E220 and usb storage

[Pete Zaitcev]

On Wed, 20 Feb 2008 07:57:23 +0100, Norbert Preining [EMAIL PROTECTED] wrote:

  that you did, after taking care of detection and initialization.
  Look at his dmesg in comment #44 in this:
 
 Yes, that looks very similar.

Someone reported on the bug that a firmware update exists to resolve
the issue (version 11.117.07.00.67). Probably they started to comply
with the spec and return 12 bytes of sense according to the allocation
length in the SCSI command.
  https://bugzilla.redhat.com/show_bug.cgi?id=253096#c51

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Huawei E220 and usb storage

[Pete Zaitcev]

On Fri, 16 Nov 2007 14:22:56 +0100, Norbert Preining [EMAIL PROTECTED] wrote:

   The difference with huaweiAktBbo.c seems that kernel uses a nonzero 
   length.
   Can you try zero length with the kernel? It's the second argument to the 
   last.
  
  I tried with the git patch plus changing the penultimage argument from
  0x1 to 0.
 
 Ok, did new tests with 2.6.24-rc2:
 - with plain kernel the usb-storage modules attaches and detaches
   permanently a virtual cd drive, I stopped after 30+ iterations.

It looks like between Dave Russel and I, we hit the same problem
that you did, after taking care of detection and initialization.
Look at his dmesg in comment #44 in this:
 https://bugzilla.redhat.com/show_bug.cgi?id=253096#c44

 - changing the penultimage argument in the usb_stor_huawei_e220_init
   function from 0x1 to 0 stopped this misbehaviour, but
 
 - with the change from 0x1 to 0 the initialization works automatically.

I may be able to test this.

As you recall, Huawei people themselves suggested nonzero length,
this is why we didn't want to change it. But perhaps they are
mistaken about the operation of their own hardware. Stranger
things happened...

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Flushing URBs for small control URBs

[Pete Zaitcev]

On Wed, 13 Feb 2008 13:31:54 -0600, Andrew McKay [EMAIL PROTECTED] wrote:

spin_lock_bh(port-lock);
if (port-write_urb_busy) {

If you lock against callbacks and do not trigger tasklets yourself,
you must use spin_lock_irqsave (or spin_lock_irq if you are belong
to Oliver's denomination).

 This leads me to my question.  Is there a buffer that the control URB ends up 
 sitting in that doesn't get flushed until a certain number of bytes are sent 
 to 
 the USB end point?  If this is the case, is there an accepted method of 
 fushing 
 the URB to the USB endpoint?

Don't overthink it.

Look at the traffic with usbmon, it provides you timestamps for
(S)ubmission and (C)allback events.

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Flushing URBs for small control URBs

[Pete Zaitcev]

On Wed, 13 Feb 2008 15:53:48 -0600, Andrew McKay [EMAIL PROTECTED] wrote:

   Once the callback function has been called does this guarantee that the URB 
 has already been sent to hardware? 

Yes (if the returned status was zero).

 I'll definitely look into this usbmon app.  I have timed things out and from 
 the 
 time I call usb_submit_urb until the callback is 1 jiffie or less.

In this case you don't need usbmon for timing, but still have
a look, it might clear the picture by giving a more complete view.

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch]pm counter leak in usblp

[Pete Zaitcev]

On Tue, 12 Feb 2008 19:08:30 +0100, Oliver Neukum [EMAIL PROTECTED] wrote:

 Signed-off-by: Oliver Neukum [EMAIL PROTECTED]

   if (handle_bidir(usblp)  0) {
 + usb_autopm_put_interface(intf);
   usblp-used = 0;

Signed-Off-By: Pete Zaitcev [EMAIL PROTECTED]

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: usbmon output to trace read/writes

[Pete Zaitcev]

On Tue, 5 Feb 2008 11:17:46 -0800 (PST), Siddharth Choudhuri [EMAIL 
PROTECTED] wrote:

 - Linux 2.6.19 on ARM

 The problem is, I see the following in /tmp/log (no data words): 
 c04087dc 57025 C Bo:002:02 0 31   
 c000ec14 57209 S Bo:002:02 -115 512 D  

The problem stems from the fact that usbmon's hooks may not have
access to the virtual address of the data. On cache-coherent
architectures, such as x86, usbmon works around it by remapping
data temporarily. I think the problem should be transparent if
you look at the code in and around usb_hcd_submit_urb().

You can change usb-storage to not pre-map commands and replies
easily, but data often comes from the highmem zone and thus
kernel does not have its virtual address.

So, if you want to see the data, you need to attack the main issue.
If you find a way to implement mon_dmapeek() for ARM, I'll be happy
to include it.

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [2.6.24 REGRESSION] BUG: Soft lockup - with VFS

[Pete Zaitcev]

On Tue, 5 Feb 2008 14:05:06 -0800, Andrew Morton [EMAIL PROTECTED] wrote:

 Looks like you deadlocked in ub_request_fn().  I assume that you were using
 ub.c in 2.6.23 and that it worked OK?  If so, we broke it, possibly via
 changes to the core block layer.
 
 I think ub.c is basically abandoned in favour of usb-storage.  If so,
 perhaps we should remove or disble ub.c?

Actually I think it may be an argument for keeping ub, if ub exposes
a bug in the __blk_end_request. I'll look at the head of the thread
and see if Mr. Pinter has hit anything related to Mr. Ueda's work.

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: usbmon output to trace read/writes

[Pete Zaitcev]

On Tue, 5 Feb 2008 17:02:58 -0500 (EST), Alan Stern [EMAIL PROTECTED] wrote:

 In such cases the page number is stored in a scatter-gather entry.  
 Should we modify the core to keep a copy of the page number in the URB,
 for use by mon_dmapeek()?

If you ask me, I'd say that rules of what is mapped and what is
not have grown plenty complex already. I don't want to touch it
unless we create a cleaned-up URB from scratch.

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: How to debug EOVERFLOW errors

[Pete Zaitcev]

On Mon, 04 Feb 2008 08:30:37 -0800, Alan Nisota [EMAIL PROTECTED] wrote:

 [...]  If I do 
 this using blocking reads, the device never sends any data back  It 
 appears to be waiting for multiple bulk request URBS in the queue.  This 
 apparently eliminates my ability to use libusb, so now I need to do the 
 same thing in kernel-space.

You can submit multiply outstanding URBs and reap them using usbfs.

 One thing I noticed is that endpoint 82 declares a max packet size of 
 0x400 where I thought the largest allowed size for usb 2.0 is 0x200. 
 The requests from Windows also have a buffer size of 0x2, but I 
 assume the driver is splitting that into smaller packets.
 
 I get the babble error regardless of the read-size I specify for the URB 
 request.

Very interesting. The EHCI spec allows for 1024 maximum packet size
in clause 3.6.2. The USB spec allows 1024 byte isochronous payloads
in clause 5.6.3. However, as far as I understand, bulk transfers
are limited to 512 maximum packet size. What type is endpoint 82?

By the way, there's virtually no way for you use buffers this big
in Linux. You'll have to restrict yourself to 8KB and chain them
with multiply URBs. Otherwise, your driver won't be able to allocate
them due to fragmentation.

 I'm not sure whether I'm initializing something incorrectly, or if the 
 device is doing something odd.  So I'm wondering if there is a way to 
 see the raw data that got returned for the URB even though it had a 
 babble error (I seem to always get a 0 len result, so I assume the 
 EOVERFLOW nukes whatever data might have been sent).  this would help 
 determine where the problem lies.

Whatever is DMA-ed into the buffer is there, however if we set
actual_len to zero, most likely nothing was transferred.

 If it is useful, my read handler looks sort of like this:
  urb = usb_alloc_urb(0, GFP_KERNEL);
  buf = usb_buffer_alloc(dev-udev, readsize, GFP_KERNEL, 
 urb-transfer_dma);
  usb_fill_bulk_urb(urb, dev-udev,
usb_rcvbulkpipe(dev-udev, 0x82),
buf, readsize, skel_read_bulk_callback, dev);
  urb-transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
  retval = usb_submit_urb(urb, GFP_KERNEL);
   usb_free_urb(urb);

Allocating a buffer with usb_buffer_alloc makes no sense for high-
bandwidth transfers, please use normal kmalloc or get_free_page.

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: request: patch for oracom mp3 stick -- usb-storage: unusual_devs.h

[Pete Zaitcev]

On Fri, 1 Feb 2008 11:59:56 -0500 (EST), Alan Stern [EMAIL PROTECTED] wrote:

 You missed the point.  Windows does _not_ do it -- i.e., does not clear 
 a halt on either bulk endpoint.  Linux does so only because somebody 
 (either Pete Zaitcev or Pat Lavarre, I can't remember which) pointed 
 out that the ZIP-100 drive needs it.

I have a ZIP-100 which does not need it. Pat said the firmware was
changed, so it was only the case for really old ones.

BTW, ub does not touch bulk pipes when or after doing GetMaxLUN,
and it seems to work, for what it's worth: the installed base is
quite small. My ZIP drive stalls ep0 and then works.

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [usb-storage] request: patch for oracom mp3 stick -- usb-storage: unusual_devs.h

[Pete Zaitcev]

On Fri, 1 Feb 2008 11:54:12 -0800, Matthew Dharm [EMAIL PROTECTED] wrote:

 There's no way to know until Robert tests with no clear-halt at all.

Which is very easy to do: enable ub, run usbmon... voila.

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Blackberry through a TT hub

[Pete Zaitcev]

Hi, Greg:

I think the berry_charge is your baby. Please direct me to the
right person if it's not so.

I have a bug in Fedora:
 https://bugzilla.redhat.com/show_bug.cgi?id=379341

It's pretty long, but the summary is this. When Blackberry is hooked
to a hub, it seems to try to re-initialize again and then something
(khubd ?) spams udevd with add/remove events without actually
disconnecting. The box melts down as a result.

My question is, do you have one of those things? Would you hook
it to a hub, check what happens then? I suspect the Berry manages
to detect that it's on a hub (how?!) and supplies wrong descriptors.
Then, berry_charge fails this:
   if ((udev-actconfig-desc.bMaxPower * 2) == 500)

Cheers,
-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Periodic USB failure

[Pete Zaitcev]

On Fri, 18 Jan 2008 16:25:51 -0500, Jon Smirl [EMAIL PROTECTED] wrote:

 I guess I have to buy another hub. The USB 2.0 hubs I have aren't
 multi-tt and don't work right with the audio devices.

I would rather think about adding a USB bus or two. The thought of
three isochronous devices on the same bus makes me uncomfortable.
It's a miracle your HCs scheduler manages to find bandwidth for them.

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: WD My Book 500G external drive: error -110

[Pete Zaitcev]

On Tue, 08 Jan 2008 18:56:03 -0500, Sean Darcy [EMAIL PROTECTED] wrote:

   head -20 /tmp/1.mon.out
 810037f38d80 3045534577 S Ii:2:001:1 -115:128 2 
 81000fe64cc0 3045534596 S Ci:2:001:0 s a3 00  0001 0004 4 

20 lines is never sufficient with usbmon. It's more like 20 thousand.
But never mind that, I think the -110 you're getting is a clear
indication that interrupts are not routed for the device.

As such it has nothing to do with USB.

There are some command line options to work around such issues, like
pci=noacpi, pci=biosirq and so on. Please look it up in
Documentation/kernel-parameters.txt.

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Kingston Datatraveler

[Pete Zaitcev]

On Mon, 7 Jan 2008 16:36:56 -0500 (EST), Alan Stern [EMAIL PROTECTED] wrote:

 I'm afraid I'm out of good ideas.  You can try playing around with the
 code in drivers/usb/storage/transport.c, adding delays here and there
 to see if they make any difference.

Sorry for repeating this, but usbmon provides a useful timestamp,
especially on more modern systems with something like HPET timer.
By comparing timestamps for the case of cat writing to serial port
and to disk, one may be able to detect the difference and thus know
where to add delays that Alan mentions above.

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: usblp not waiting for read data

[Pete Zaitcev]

On Wed, 2 Jan 2008 15:25:11 +0100, Martin Mares [EMAIL PROTECTED] wrote:

 | open(/dev/usb/lp0, O_RDONLY|O_LARGEFILE) = 3
 | fstat64(3, {st_mode=S_IFCHR|0660, st_rdev=makedev(180, 0), ...}) = 0
 | read(3, , 4096)   = 0
 | close(3)= 0

Figures...

 albireo:/sys/kernel/debug/usbmon# cat 1u
 # (ran cat for the first time)
 ddbeb7c0 1940101507 S Bi:1:003:1 -115 1024 
 ddbeb7c0 1940101645 C Bi:1:003:1 0 0

This is where your zero length comes from: printer returns zero bytes.

 ddbeb7c0 1940101675 S Bi:1:003:1 -115 1024 
 ddbeb7c0 1940101766 C Bi:1:003:1 -2 0

This is an unlink on close, but it's interesting that either printer is
not fast enough to send another zero data reply or it begins NAK-ing for
the second time around.

I checked the old driver, it does not seem to retry either and should
return EOF happily too. Although it should not make any difference in
view of the trace above, is there a box with something like 2.6.21 to try?

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Kingston Datatraveler

[Pete Zaitcev]

On Sun, 30 Dec 2007 22:48:39 +, Brian Murphy [EMAIL PROTECTED] wrote:

 I install Win XP on the system and the drive works fine.

Great. I hope it was a dual-boot setup.

 Is there any way I can modifie the driver so I see the same result as
 when I put the debug output to the serial port. It seem to effect the
 timing and lets the drive work.

No, I don't think we have enough resolution for this... Although you can
compare timestamps which usbmon reports in both cases. I do it sometimes
in such instances.

BTW:

   I think your device is asking for US_FL_GO_SLOW or equivalent.

You might want to throw that in and see if it sticks.

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Kingston Datatraveler

[Pete Zaitcev]

On Fri, 21 Dec 2007 10:50:06 +, Brian Murphy [EMAIL PROTECTED] wrote:

 I just discover some thing, If I ouput the trace to
 a serial port the system works fine. However If I output the trace to
 the disk I have the same problem as before. [...]

I think your device is asking for US_FL_GO_SLOW or equivalent.
It keels over on the very first read after the WP sensing:

 c12e7ea0 43055373 S Bo:1:004:2 -115 31 = 55534243 0b00 0010 8a28 
  0008  00
 c12e7ea0 43056075 C Bo:1:004:2 0 31 
 c12e7720 43056160 S Bi:1:004:1 -115 4096 
 c12e7720 43162104 C Bi:1:004:1 -84 0

I had major issues with the Kingston in the past because they do not
like a halt clear while not halted, but nothing like this. Perhaps
it's a new cheaper revision, buggier than before.

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Disabling interrupts during IRQ in ohci-hcd

[Pete Zaitcev]

On Wed, 5 Dec 2007 17:31:20 -0500 (EST), Alan Stern [EMAIL PROTECTED] wrote:

 [...] In fact every HCD _must_ keep interrupts disabled 
 while doing most of its IRQ processing.  This is because an interrupt 
 could cause some random driver to submit an URB -- and URB submission 
 can't be done concurrently with IRQ processing since they both alter 
 the same data structures.

Fair enough, but this is why the HCD has to do spin_lock_irqsave
across the time when lists are being modified or otherwise inconsistent.
It should not need to keep interrupts closed across executing callbacks
for complete URBs. IMHO.

In other message you wrote that:

 However usb_hcd_unlink_urb_from_ep is documented as requiring
 that it be called with interrupts disabled.

I continue to think that methods with such requirements must not
exist (at least not at the level we operate in USB stack; I can see
a need for something like that in the architecture support code).
It is unduly onerous to make sure the requirement is met while the
code changes, and it leads to unnecessary expansion of time spent with
interrupts off.

I'm not going to appeal to Greg to have you overruled or anything, but
I remain unconvinced.

-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html