All of lore.kernel.org
 help / color / mirror / Atom feed
* unfixed regression in 2.6.20-rc6 (since 2.6.19)
@ 2007-01-25 15:20 Rainer Weikusat
  2007-01-25 18:51 ` Greg KH
  0 siblings, 1 reply; 12+ messages in thread
From: Rainer Weikusat @ 2007-01-25 15:20 UTC (permalink / raw)
  To: torvalds; +Cc: bunk, linux-kernel

2.6.19 introduced changes to the UHCI handling of interrupt URBs that
caused at least some keyspan USB-to-serial converters to fail, because
the driver code incorrectly assumes that all URBs of the device are
bulk URBs, while some of them (in the default configuration) are
actually interrupt URBs. This has been reported via kernel bugzilla as
bug 7544 on 2006/11/17:

	http://bugzilla.kernel.org/show_bug.cgi?id=7544

I happen to use such a device, but I didn't have the time to deal with
this issue until end of december. A fix for this regression has been
available at least since 2006/12/26. The patch available via bugzilla
has been reworked by me because of comments from Mr. Kroah-Hartmann,

	http://marc.theaimsgroup.com/?l=linux-kernel&m=116783507105717&w=2

It has been part of the -mm tree until about three days ago and was
then dropped by Andrew Morton because Mr Kroah-Hartmann added a
structurally identical and functionally neutral (or slightly worse)
rewritten-by-him version of it to something called 'gregkh-2.6',
presumably, judging from the age of other patches in this particular
tree (back until 2006/09/19), to never see the light of the day again.

If this issue is not going to be fixed for some reason, it would at
least be appropriate to close the bugzilla entry with something like
WILL_NOT_FIX, because that appears to be the situation at present and
for an indefinite time to come.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: unfixed regression in 2.6.20-rc6 (since 2.6.19)
  2007-01-25 15:20 unfixed regression in 2.6.20-rc6 (since 2.6.19) Rainer Weikusat
@ 2007-01-25 18:51 ` Greg KH
  2007-01-26 10:43   ` Rainer Weikusat
  0 siblings, 1 reply; 12+ messages in thread
From: Greg KH @ 2007-01-25 18:51 UTC (permalink / raw)
  To: Rainer Weikusat; +Cc: torvalds, bunk, linux-kernel

On Thu, Jan 25, 2007 at 04:20:55PM +0100, Rainer Weikusat wrote:
> 2.6.19 introduced changes to the UHCI handling of interrupt URBs that
> caused at least some keyspan USB-to-serial converters to fail, because
> the driver code incorrectly assumes that all URBs of the device are
> bulk URBs, while some of them (in the default configuration) are
> actually interrupt URBs. This has been reported via kernel bugzilla as
> bug 7544 on 2006/11/17:
> 
> 	http://bugzilla.kernel.org/show_bug.cgi?id=7544
> 
> I happen to use such a device, but I didn't have the time to deal with
> this issue until end of december. A fix for this regression has been
> available at least since 2006/12/26. The patch available via bugzilla
> has been reworked by me because of comments from Mr. Kroah-Hartmann,
> 
> 	http://marc.theaimsgroup.com/?l=linux-kernel&m=116783507105717&w=2
> 
> It has been part of the -mm tree until about three days ago and was
> then dropped by Andrew Morton because Mr Kroah-Hartmann added a
> structurally identical and functionally neutral (or slightly worse)
> rewritten-by-him version of it to something called 'gregkh-2.6',
> presumably, judging from the age of other patches in this particular
> tree (back until 2006/09/19), to never see the light of the day again.

Huh?  That's my staging tree for what goes to Linus and Andrew.

Yes, some patches in there are very old, and are not supposed to ever go
to mainline, but that is the minority.

I copied you when this patch was added to my tree, as I had reworked it
to be a bit more acceptable (no pointer arithmatic, proper coding style,
use the newer macros for endpoint detection, etc.), so you were aware
when this change was added.  If this patch does not work, please let me
know, and I will be glad to work to fix it.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: unfixed regression in 2.6.20-rc6 (since 2.6.19)
  2007-01-25 18:51 ` Greg KH
@ 2007-01-26 10:43   ` Rainer Weikusat
  2007-01-26 17:48     ` Greg KH
  0 siblings, 1 reply; 12+ messages in thread
From: Rainer Weikusat @ 2007-01-26 10:43 UTC (permalink / raw)
  To: Greg KH; +Cc: Rainer Weikusat, torvalds, bunk, linux-kernel

Greg KH <greg@kroah.com> writes:
> On Thu, Jan 25, 2007 at 04:20:55PM +0100, Rainer Weikusat wrote:
>> 2.6.19 introduced changes to the UHCI handling of interrupt URBs that
>> caused at least some keyspan USB-to-serial converters to fail,

[...]

> I copied you when this patch was added to my tree,

The only thing I received was a 'this patch has been dropped from
the MM-tree because something happened' message.

> as I had reworked it to be a bit more acceptable (no pointer
> arithmatic, proper coding style, use the newer macros for endpoint
> detection, etc.),

You have basically done a couple of (functionally) totally pointless
changes, like

	- not using iterator-style array traversals, but indexed ones
          instead

	- doing the calculation to determine the endpoint type in
          three different places instead of in one place, because the
          somewhat silly endpoint classification interfaces enforces
          this

	- not using a single switch-statement but an if-else-cascade,
          again due to limitations of that interface

	- replacing the while-loop with an identical for-loop

The net effect of these changes is that an optimizing compiler will
have to work somewhat more to remove all the redundant stuff that
was added. As for 'proper coding style', the code conforms to what is
documented as 'proper kernel coding style'. If this assumption of mine
is incorrect, I'd be happy to hear about reasons why this would be so,
to take them into account for eventual future patches.

> I this patch does not work, please let me know, and I will be glad
> to work to fix it.

I think I'll just resend the working one in this case. But the logic
appears to be identical, so I do not so a reason why it shouldn't.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: unfixed regression in 2.6.20-rc6 (since 2.6.19)
  2007-01-26 10:43   ` Rainer Weikusat
@ 2007-01-26 17:48     ` Greg KH
  2007-01-28 13:34       ` Rainer Weikusat
  0 siblings, 1 reply; 12+ messages in thread
From: Greg KH @ 2007-01-26 17:48 UTC (permalink / raw)
  To: Rainer Weikusat; +Cc: Rainer Weikusat, torvalds, bunk, linux-kernel

On Fri, Jan 26, 2007 at 11:43:22AM +0100, Rainer Weikusat wrote:
> Greg KH <greg@kroah.com> writes:
> > On Thu, Jan 25, 2007 at 04:20:55PM +0100, Rainer Weikusat wrote:
> >> 2.6.19 introduced changes to the UHCI handling of interrupt URBs that
> >> caused at least some keyspan USB-to-serial converters to fail,
> 
> [...]
> 
> > I copied you when this patch was added to my tree,
> 
> The only thing I received was a 'this patch has been dropped from
> the MM-tree because something happened' message.

You were also copied on the patch when it was added to my tree.  Email
was sent to your rweikusat@sncag.com address.

> > as I had reworked it to be a bit more acceptable (no pointer
> > arithmatic, proper coding style, use the newer macros for endpoint
> > detection, etc.),
> 
> You have basically done a couple of (functionally) totally pointless
> changes, like
> 
> 	- not using iterator-style array traversals, but indexed ones
>           instead

Which is the better way to document exactly how you are walking through
all of the endpoints.

> 	- doing the calculation to determine the endpoint type in
>           three different places instead of in one place, because the
>           somewhat silly endpoint classification interfaces enforces
>           this

3 places?  You mean in the if call?

> 	- not using a single switch-statement but an if-else-cascade,
>           again due to limitations of that interface

This really isn't a problem.

> 	- replacing the while-loop with an identical for-loop

Again, this is the better way to walk through all of the endpoints for a
device.

> The net effect of these changes is that an optimizing compiler will
> have to work somewhat more to remove all the redundant stuff that
> was added.

This is initialization code, it's better to be obvious as to what you
are trying to do here than trying to do the compiler's job for it.
Especially as it is not a critical path by any means.

> As for 'proper coding style', the code conforms to what is
> documented as 'proper kernel coding style'. If this assumption of mine
> is incorrect, I'd be happy to hear about reasons why this would be so,
> to take them into account for eventual future patches.

There were some statements that had more than one on one line (I think
in a few usages of 'if'), but as I don't have the original patch handy,
I'll defer.

The _main_ coding style difference is that it was not obvious as to what
you were doing while walking the list of endpoints in the device.  That
is why I changed the code, to be more obvious to who ever looks at the
code next (most likely me in about 2 years...)

> > I this patch does not work, please let me know, and I will be glad
> > to work to fix it.
> 
> I think I'll just resend the working one in this case. But the logic
> appears to be identical, so I do not so a reason why it shouldn't.

Please work to see what is wrong with the existing patch.  Is there
anything that I can do to help you out?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: unfixed regression in 2.6.20-rc6 (since 2.6.19)
  2007-01-26 17:48     ` Greg KH
@ 2007-01-28 13:34       ` Rainer Weikusat
  2007-01-29  1:22         ` Oleg Verych
  0 siblings, 1 reply; 12+ messages in thread
From: Rainer Weikusat @ 2007-01-28 13:34 UTC (permalink / raw)
  To: Greg KH; +Cc: Rainer Weikusat, torvalds, bunk, linux-kernel

Greg KH <greg@kroah.com> writes:
> On Fri, Jan 26, 2007 at 11:43:22AM +0100, Rainer Weikusat wrote:
>> Greg KH <greg@kroah.com> writes:
>> > On Thu, Jan 25, 2007 at 04:20:55PM +0100, Rainer Weikusat wrote:
>> >> 2.6.19 introduced changes to the UHCI handling of interrupt URBs that
>> >> caused at least some keyspan USB-to-serial converters to fail,
>> 
>> [...]
>> 
>> > I copied you when this patch was added to my tree,
>> 
>> The only thing I received was a 'this patch has been dropped from
>> the MM-tree because something happened' message.
>
> You were also copied on the patch when it was added to my tree.  Email
> was sent to your rweikusat@sncag.com address.

It certainly never arrived at the MTA running on my working machine.

>> > as I had reworked it to be a bit more acceptable (no pointer
>> > arithmatic, proper coding style, use the newer macros for endpoint
>> > detection, etc.),
>> 
>> You have basically done a couple of (functionally) totally pointless
>> changes, like
>> 
>> 	- not using iterator-style array traversals, but indexed ones
>>           instead
>
> Which is the better way to document exactly how you are walking through
> all of the endpoints.

But the code does not walk trough the endpoints anymore, that's what
it did before the change. Now, it is walking through a certain subset
of the set of integral numbers, using the numbers from those set as
offset to indirectly address something. Additionally, it is harder to
read because it contains more 'noise characters' that are not
pronouncable. Arguing about this is not really reasonable, though,
because this is mostly a matter of aesthetics and a decent compiler
will generate identical code for both, ie remove the indexing
automatically again. A sensible way to enforce regulations of code
aesthetic would be to document them.

>> 	- doing the calculation to determine the endpoint type in
>>           three different places instead of in one place, because the
>>           somewhat silly endpoint classification interfaces enforces
>>           this
>
> 3 places?  You mean in the if call?

Exactly. The 'interface' that is used here does not provide a way to
ask for the type, only to ask if the type is type X. At the source
level, this means

	if ((value & mask) == x) {
        	.
                .
                .
	} else if ((value & mask) == y) {
        	.
                .
                .
	else printf("%u\n", values & mask);

A decent compiler will again probably catch this and do CSE-removal,
but this doesn't make the algorithm more sensible as it is written
down (C is not 'math', and C-expressions don't represent equal states,
like equations would do, but request that the CPU performs certain
calcluations).

>> 	- not using a single switch-statement but an if-else-cascade,
>>           again due to limitations of that interface
>
> This really isn't a problem.

It did not claim this was 'a problem' (except insofar 'clumsiness' can
be construed to be one). It is just a code change for the sake of
having changed the code.

>> 	- replacing the while-loop with an identical for-loop
>
> Again, this is the better way to walk through all of the endpoints for a
> device.

This may be the way you happen to like more and a lot of people will
probably agree with that. But the device does not care and they are
both functionally identical, only the 'textual complexity' of using
for is higher.

>> The net effect of these changes is that an optimizing compiler will
>> have to work somewhat more to remove all the redundant stuff that
>> was added.
>
> This is initialization code, it's better to be obvious as to what you
> are trying to do here than trying to do the compiler's job for it.

The claim that the meaning of something with a lower signal-to-noise
ratio is more obvious in itself is somewhat absurd: If I have to
understand the code, I have to read it and think about it and I may
reach the conclusion that certain calculations are actually
meaningless, but I still need to understand them first.

[...]

> The _main_ coding style difference is that it was not obvious as to what
> you were doing while walking the list of endpoints in the device.

That is not 'a coding style difference'. It basically just means that
you didn't understand a text not written by yourself without reading
it first.

[...]

>> > I this patch does not work, please let me know, and I will be glad
>> > to work to fix it.
>> 
>> I think I'll just resend the working one in this case. But the logic
>> appears to be identical, so I do not so a reason why it shouldn't.
>
> Please work to see what is wrong with the existing patch.  Is there
> anything that I can do to help you out?

This thing has consumed something like sixteen hours of my life in
total, with a gain-to-be-expected of exactly zero (I don't need to run
'current' kernels on my work machine, I have just grown into the habit
of doing so) and those sixteen hours cannot come back (and I even have
had these type of discussions around 'should it rather look like math
or rather like text' in sufficent quantities :->), so, except that I
would be very much obliged to you if a fix for this issue could go
into the 'official' tree rather sooner than later, no.

Apart from that, I make a (fairly miserable) living by adapting open
source code to be usable in specific situations (ie adding or
modifying features, fixing bugs, writing drivers etc) and I
ocassionally have dreams in C (this is not a joke :-), ie understand
it fairly well, no matter who was the author.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: unfixed regression in 2.6.20-rc6 (since 2.6.19)
  2007-01-28 13:34       ` Rainer Weikusat
@ 2007-01-29  1:22         ` Oleg Verych
  2007-01-29 23:43           ` Greg KH
  0 siblings, 1 reply; 12+ messages in thread
From: Oleg Verych @ 2007-01-29  1:22 UTC (permalink / raw)
  To: linux-kernel

> From: Rainer Weikusat
> Newsgroups: gmane.linux.kernel
> Subject: Re: unfixed regression in 2.6.20-rc6 (since 2.6.19)
> Date: Sun, 28 Jan 2007 14:34:56 +0100

Hallo.

> Greg KH <greg@kroah.com> writes:
[]
>> Please work to see what is wrong with the existing patch.  Is there
>> anything that I can do to help you out?
>
> This thing has consumed something like sixteen hours of my life in
> total, with a gain-to-be-expected of exactly zero (I don't need to run
> 'current' kernels on my work machine, I have just grown into the habit
> of doing so) and those sixteen hours cannot come back (and I even have
> had these type of discussions around 'should it rather look like math
> or rather like text' in sufficent quantities :->), so, except that I
> would be very much obliged to you if a fix for this issue could go
> into the 'official' tree rather sooner than later, no.

It's hot here.

I'm in similar situation (even *usb-serial* driver [TI USB] led me there;)

In short, it turned, that usb drivers aren't drivers at all, they are
just "USB interface drivers", i.e. managers of the particular USB
interface *in* the device.

Problem is: after changing ti-usb-serial's firmware, it is being reset
and apears with new device ID. It's OK so far, but even this may be
better (from USB hardware implementation point of view). Then this
device, after being caught with new ID by the same "driver" requires
seting USB configuration #2, in order to be usb-serial converter. Day was
lost to make this happen _inside_ driver (kernel 2.6.18). It turned, that
only way to do so is SYSFS, that set up by udev, and
"usb_set_configuration()" function is being used for that.

1. Why it's not called "sysfs_usb_set_configuration()"?

2. Why

   [USB device view]: vID:dID -- bNumConfigurations -- bNumInterfaces

is being only device IDs tables in usb drivers?

3. Where's configuration and/or interfaces choosing? Yes, this may be not
so wide used, but hey, it's design issue! There's a big cave in device
setup and configuration chain!

Look at 2.6.19 with "usb_driver_set_configuration()" to see it.

And don't say, USB device requires userspace to setup (external
firmware is another question). I can be young and stupid, and this is
very wired only for my understanding. Simple NULL by default or set
table of {num_usb_conf, num_interface} for "drivers" will be enough.

> Apart from that, I make a (fairly miserable) living by adapting open
> source code to be usable in specific situations (ie adding or
> modifying features, fixing bugs, writing drivers etc)

So and i. I wanted to adopt request_firmware() for TI USB serial, but
i became very confused and upset.

--
-o--=O`C
 #oo'L O
<___=E M


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: unfixed regression in 2.6.20-rc6 (since 2.6.19)
  2007-01-29  1:22         ` Oleg Verych
@ 2007-01-29 23:43           ` Greg KH
  2007-01-30 17:40             ` Rainer Weikusat
  0 siblings, 1 reply; 12+ messages in thread
From: Greg KH @ 2007-01-29 23:43 UTC (permalink / raw)
  To: LKML, Rainer Weikusat, Oleg Verych, Linus Torvalds, Adrian Bunk

On Mon, Jan 29, 2007 at 01:31:09AM +0000, Oleg Verych wrote:
> 
> > From: Rainer Weikusat
> > Newsgroups: gmane.linux.kernel
> > Subject: Re: unfixed regression in 2.6.20-rc6 (since 2.6.19)
> > Date: Sun, 28 Jan 2007 14:34:56 +0100
> 
> Hallo.
> 
> > Greg KH <greg@kroah.com> writes:
> []
> >> Please work to see what is wrong with the existing patch.  Is there
> >> anything that I can do to help you out?
> >
> > This thing has consumed something like sixteen hours of my life in
> > total, with a gain-to-be-expected of exactly zero (I don't need to run
> > 'current' kernels on my work machine, I have just grown into the habit
> > of doing so) and those sixteen hours cannot come back (and I even have
> > had these type of discussions around 'should it rather look like math
> > or rather like text' in sufficent quantities :->), so, except that I
> > would be very much obliged to you if a fix for this issue could go
> > into the 'official' tree rather sooner than later, no.
> 
> It's hot here.
> 
> I'm in similar situation (even *usb-serial* driver [TI USB] led me there;)

No, not at all.  Your situation is you object to the current way the USB
subsystem binds devices to drivers (well, interfaces), and wish to rip
the firmware out of a usb-serial driver that is working just fine right
now.

I still don't understand why you wish to take the firmware out and move
it to userspace, why do you want to do this?

Rainer's problem is a real bug in the USB driver code, which we need to
work on getting fixed, vastly different from your objections.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: unfixed regression in 2.6.20-rc6 (since 2.6.19)
  2007-01-29 23:43           ` Greg KH
@ 2007-01-30 17:40             ` Rainer Weikusat
  2007-02-04  4:39               ` Greg KH
  0 siblings, 1 reply; 12+ messages in thread
From: Rainer Weikusat @ 2007-01-30 17:40 UTC (permalink / raw)
  To: Greg KH; +Cc: LKML, Rainer Weikusat, Oleg Verych, Linus Torvalds, Adrian Bunk

Greg KH <greg@kroah.com> writes:

[...]

> Rainer's problem is a real bug in the USB driver code, which we need to
> work on getting fixed,

[...]

To clarify this: My 'problem' was that my USB-to-serial converter
stopped working with the 2.6.19 release end of November 2007. It
wasn't until end of December that I could spend some time apart from
my day job, whose main occupation has been kernel programming since October,
albeit for the 2.4 kernels used in the devices I am responsible for, to
track this down (took around a workday, because I had never seen the
respective code before) and to implement a fix that I am using since
then.

While I would prefer to not have to maintain private patchsets for
kernels I just want to use, this has never really been possible since
1998 or so, when a well-meaning person dropped 'rst provoking' from
the dynip support code without me noticing it before it had costed me
some 500 DM in additional telephone costs, meaning, except that the necessity
for this makes me 'philosophically unhappy', I can use my device
again and there is no 'problem' anymore.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: unfixed regression in 2.6.20-rc6 (since 2.6.19)
  2007-01-30 17:40             ` Rainer Weikusat
@ 2007-02-04  4:39               ` Greg KH
  2007-02-12 14:30                 ` Rainer Weikusat
  0 siblings, 1 reply; 12+ messages in thread
From: Greg KH @ 2007-02-04  4:39 UTC (permalink / raw)
  To: Rainer Weikusat
  Cc: LKML, Rainer Weikusat, Oleg Verych, Linus Torvalds, Adrian Bunk

On Tue, Jan 30, 2007 at 06:40:10PM +0100, Rainer Weikusat wrote:
> Greg KH <greg@kroah.com> writes:
> 
> [...]
> 
> > Rainer's problem is a real bug in the USB driver code, which we need to
> > work on getting fixed,

Ok, here's an updated version, that should tell us where things are
going wrong (if they are.)

Can you try this out with debugging enabled for the driver:
	modprobe keyspan debug=1

and let me know what the output of the kernel log is when you plug your
device in?

I threw it at all of the keyspan devices that I have here, but
unfortunatly, they all have BULK endpoints.

But it all still works, which is good to see :)

Let me know what you find.

thanks,

greg k-h

-----------------------
From: Rainer Weikusat <rainer.weikusat@sncag.com>
Date: Wed, 03 Jan 2007 15:36:25 +0100
Subject: [PATCH 2.6.20-rc3] fix for bugzilla #7544 (keyspan USB-to-serial converter)
To: Greg KH <greg@kroah.com>
Message-ID: <87ejqcdvo6.fsf@semtex.sncag.com>


At least the Keyspan USA-19HS USB-to-serial converter supports
two different configurations, one where the input endpoints
have interrupt transfer type and one where they are bulk endpoints.
The default UHCI configuration uses the interrupt input endpoints.
The keyspan driver, OTOH, assumes that the device has only bulk
endpoints (all URBs are initialized by calling usb_fill_bulk_urb
in keyspan.c/ keyspan_setup_urb). This causes the interval field
of the input URBs to have a value of zero instead of one, which
'accidentally' worked with Linux at least up to 2.6.17.11 but
stopped to with 2.6.18, which changed the UHCI support code handling
URBs for interrupt endpoints. The patch below modifies to driver to
initialize its input URBs either as interrupt or as bulk URBs,
depending on the transfertype contained in the associated endpoint
descriptor (only tested with the default configuration) enabling
the driver to again receive data from the serial converter.

Greg K-H reworked the patch.

Signed-off-by: Rainer Weikusat <rweikusat@sncag.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/usb/serial/keyspan.c |   49 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 4 deletions(-)

--- gregkh-2.6.orig/drivers/usb/serial/keyspan.c
+++ gregkh-2.6/drivers/usb/serial/keyspan.c
@@ -1275,11 +1275,31 @@ static int keyspan_fake_startup (struct 
 }
 
 /* Helper functions used by keyspan_setup_urbs */
+static struct usb_endpoint_descriptor const *find_ep(struct usb_serial const *serial,
+						     int endpoint)
+{
+	struct usb_host_interface *iface_desc;
+	struct usb_endpoint_descriptor *ep;
+	int i;
+
+	iface_desc = serial->interface->cur_altsetting;
+	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
+		ep = &iface_desc->endpoint[i].desc;
+		if (ep->bEndpointAddress == endpoint)
+			return ep;
+	}
+	dev_warn(&serial->interface->dev, "found no endpoint descriptor for "
+		 "endpoint %x\n", endpoint);
+	return NULL;
+}
+
 static struct urb *keyspan_setup_urb (struct usb_serial *serial, int endpoint,
 				      int dir, void *ctx, char *buf, int len,
 				      void (*callback)(struct urb *))
 {
 	struct urb *urb;
+	struct usb_endpoint_descriptor const *ep_desc;
+	char const *ep_type_name;
 
 	if (endpoint == -1)
 		return NULL;		/* endpoint not needed */
@@ -1291,11 +1311,32 @@ static struct urb *keyspan_setup_urb (st
 		return NULL;
 	}
 
-		/* Fill URB using supplied data. */
-	usb_fill_bulk_urb(urb, serial->dev,
-		      usb_sndbulkpipe(serial->dev, endpoint) | dir,
-		      buf, len, callback, ctx);
+	ep_desc = find_ep(serial, endpoint);
+	if (!ep_desc) {
+		/* leak the urb, something's wrong and the callers don't care */
+		return urb;
+	}
+	if (usb_endpoint_xfer_int(ep_desc)) {
+		ep_type_name = "INT";
+		usb_fill_int_urb(urb, serial->dev,
+				 usb_sndintpipe(serial->dev, endpoint) | dir,
+				 buf, len, callback, ctx,
+				 ep_desc->bInterval);
+	} else if (usb_endpoint_xfer_bulk(ep_desc)) {
+		ep_type_name = "BULK";
+		usb_fill_bulk_urb(urb, serial->dev,
+				  usb_sndbulkpipe(serial->dev, endpoint) | dir,
+				  buf, len, callback, ctx);
+	} else {
+		dev_warn(&serial->interface->dev,
+			 "unsupported endpoint type %x\n",
+			 ep_desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK);
+		usb_free_urb(urb);
+		return NULL;
+	}
 
+	dbg("%s - using urb %p for %s endpoint %x",
+	    __func__, urb, ep_type_name, endpoint);
 	return urb;
 }
 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: unfixed regression in 2.6.20-rc6 (since 2.6.19)
  2007-02-04  4:39               ` Greg KH
@ 2007-02-12 14:30                 ` Rainer Weikusat
  2007-02-12 16:37                   ` Greg KH
  0 siblings, 1 reply; 12+ messages in thread
From: Rainer Weikusat @ 2007-02-12 14:30 UTC (permalink / raw)
  To: Greg KH; +Cc: Rainer Weikusat, LKML, Oleg Verych, Linus Torvalds, Adrian Bunk

Greg KH <greg@kroah.com> writes:
> On Tue, Jan 30, 2007 at 06:40:10PM +0100, Rainer Weikusat wrote:
>> Greg KH <greg@kroah.com> writes:
>> 
>> [...]
>> 
>> > Rainer's problem is a real bug in the USB driver code, which we need to
>> > work on getting fixed,
>
> Ok, here's an updated version, that should tell us where things are
> going wrong (if they are.)
>
> Can you try this out with debugging enabled for the driver:
> 	modprobe keyspan debug=1
>
> and let me know what the output of the kernel log is when you plug your
> device in?

I can do all sorts of things, but frankly, I just need my device to
work (which it does again, courtesy of myself). Apart from that, this
is still just what a SCO-person would call 'a structural copy' of my
code with no functional changes, especially none that could result in
any additional output. I believe it will work as well. Mine does, so
I am not going to test a different one for the sake of having done so.

If you want to fix it only if you can claim to have written the code,
fine. If you'll never fix it --- fine as well. In either case, I have
already put more time into this as I have available.




^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: unfixed regression in 2.6.20-rc6 (since 2.6.19)
  2007-02-12 14:30                 ` Rainer Weikusat
@ 2007-02-12 16:37                   ` Greg KH
  2007-02-12 17:10                     ` Rainer Weikusat
  0 siblings, 1 reply; 12+ messages in thread
From: Greg KH @ 2007-02-12 16:37 UTC (permalink / raw)
  To: Rainer Weikusat
  Cc: Rainer Weikusat, LKML, Oleg Verych, Linus Torvalds, Adrian Bunk

On Mon, Feb 12, 2007 at 03:30:22PM +0100, Rainer Weikusat wrote:
> Greg KH <greg@kroah.com> writes:
> > On Tue, Jan 30, 2007 at 06:40:10PM +0100, Rainer Weikusat wrote:
> >> Greg KH <greg@kroah.com> writes:
> >> 
> >> [...]
> >> 
> >> > Rainer's problem is a real bug in the USB driver code, which we need to
> >> > work on getting fixed,
> >
> > Ok, here's an updated version, that should tell us where things are
> > going wrong (if they are.)
> >
> > Can you try this out with debugging enabled for the driver:
> > 	modprobe keyspan debug=1
> >
> > and let me know what the output of the kernel log is when you plug your
> > device in?
> 
> I can do all sorts of things, but frankly, I just need my device to
> work (which it does again, courtesy of myself). Apart from that, this
> is still just what a SCO-person would call 'a structural copy' of my
> code with no functional changes, especially none that could result in
> any additional output. I believe it will work as well. Mine does, so
> I am not going to test a different one for the sake of having done so.

Well, as others have reported that this fix works for them, I've sent it
on to Linus and will add it to the 2.6.20-stable tree too.

> If you want to fix it only if you can claim to have written the code,
> fine. If you'll never fix it --- fine as well.

I do not claim I have rewritten the code, if you look, you get full
authorship credit for this fix, I just say I restructured it a bit.

> In either case, I have already put more time into this as I have
> available.

If you still have problems, please let me know.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: unfixed regression in 2.6.20-rc6 (since 2.6.19)
  2007-02-12 16:37                   ` Greg KH
@ 2007-02-12 17:10                     ` Rainer Weikusat
  0 siblings, 0 replies; 12+ messages in thread
From: Rainer Weikusat @ 2007-02-12 17:10 UTC (permalink / raw)
  To: Greg KH; +Cc: Rainer Weikusat, LKML, Oleg Verych, Linus Torvalds, Adrian Bunk

Greg KH <greg@kroah.com> writes:
> On Mon, Feb 12, 2007 at 03:30:22PM +0100, Rainer Weikusat wrote:

[...]

>> If you want to fix it only if you can claim to have written the code,
>> fine.
>
> I do not claim I have rewritten the code, if you look, you get full
> authorship credit for this fix, I just say I restructured it a bit.

You have rewritten it to match set of popular beliefs I consider to be
misguided. This means _you_ are the author of this code and all I can
to is to basically accept that you have the power to force your
beliefs on others and I haven't. So be it. Could this be the end of
this useless discussion?


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2007-02-12 17:10 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-01-25 15:20 unfixed regression in 2.6.20-rc6 (since 2.6.19) Rainer Weikusat
2007-01-25 18:51 ` Greg KH
2007-01-26 10:43   ` Rainer Weikusat
2007-01-26 17:48     ` Greg KH
2007-01-28 13:34       ` Rainer Weikusat
2007-01-29  1:22         ` Oleg Verych
2007-01-29 23:43           ` Greg KH
2007-01-30 17:40             ` Rainer Weikusat
2007-02-04  4:39               ` Greg KH
2007-02-12 14:30                 ` Rainer Weikusat
2007-02-12 16:37                   ` Greg KH
2007-02-12 17:10                     ` Rainer Weikusat

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.