Skip to contentSkip to conversation

USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

PATCH In review Jeremy Figgins 6 participants 15 messages Jan 17, 2021 – Jan 23, 2021

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

Certain devices such as Winbond Virtual Com Port, which is used in some usb printers, will stop responding after the usb_control_msg_send() calls in usb_set_interface(). These devices work fine without having usb_set_interface() called, so this flag prevents that call.

The naming is designed to mirror the existing USB_QUIRK_NO_SET_INTF flag, but that flag is not sufficient to make these devices work.

---
 drivers/usb/class/usblp.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/usb/class/usblp.c +7 -1
index 134dc2005ce9..6e2d58813d7d 100644
@@ -209,6 +209,7 @@ struct quirk_printer_struct {
 #define USBLP_QUIRK_BIDIR	0x1	/* reports bidir but requires unidirectional mode (no INs/reads) */
 #define USBLP_QUIRK_USB_INIT	0x2	/* needs vendor USB init string */
 #define USBLP_QUIRK_BAD_CLASS	0x4	/* descriptor uses vendor-specific Class or SubClass */
+#define USBLP_QUIRK_NO_SET_INTF	0x8	/* device can't handle Set-Interface requests */
 
 static const struct quirk_printer_struct quirk_printers[] = {
 	{ 0x03f0, 0x0004, USBLP_QUIRK_BIDIR }, /* HP DeskJet 895C */
@@ -227,6 +228,7 @@ static const struct quirk_printer_struct quirk_printers[] = {
 	{ 0x0482, 0x0010, USBLP_QUIRK_BIDIR }, /* Kyocera Mita FS 820, by zut <[email protected]> */
 	{ 0x04f9, 0x000d, USBLP_QUIRK_BIDIR }, /* Brother Industries, Ltd HL-1440 Laser Printer */
 	{ 0x04b8, 0x0202, USBLP_QUIRK_BAD_CLASS }, /* Seiko Epson Receipt Printer M129C */
+	{ 0x0416, 0x5011, USBLP_QUIRK_NO_SET_INTF }, /* Winbond Electronics Corp. Virtual Com Port */
 	{ 0, 0 }
 };
 
@@ -1332,7 +1334,11 @@ static int usblp_set_protocol(struct usblp *usblp, int protocol)
 	alts = usblp->protocol[protocol].alt_setting;
 	if (alts < 0)
 		return -EINVAL;
-	r = usb_set_interface(usblp->dev, usblp->ifnum, alts);
+	if (usblp->quirks & USBLP_QUIRK_NO_SET_INTF) {
+		r = 0;
+	} else {
+		r = usb_set_interface(usblp->dev, usblp->ifnum, alts);
+	}
 	if (r < 0) {
 		printk(KERN_ERR "usblp: can't set desired altsetting %d on interface %d\n",
 			alts, usblp->ifnum);
Signature · 1 line
2.29.0
maintainer
#1 lore raw

Review

Quoted 4 lines · On Sun, 17 Jan 2021 15:36:39 -0600 Jeremy Figgins <[email protected]>…

The naming is designed to mirror the existing
USB_QUIRK_NO_SET_INTF flag, but that flag is
not sufficient to make these devices work.

(quoted diff) +1 -0
+	{ 0x0416, 0x5011, USBLP_QUIRK_NO_SET_INTF }, /* Winbond Electronics Corp. Virtual Com Port */

Jeremy, thanks for the patch. It looks mostly fine code-wise (quirk is out of numerical order), but I have a question: did you consider keying off usblp->dev->quirks instead?

How about this:

drivers/usb/class/usblp.c +5 -1
index 37062130a03c..0c4a98f00797 100644
@@ -1315,7 +1315,11 @@ static int usblp_set_protocol(struct usblp *usblp, int protocol)
 	alts = usblp->protocol[protocol].alt_setting;
 	if (alts < 0)
 		return -EINVAL;
-	r = usb_set_interface(usblp->dev, usblp->ifnum, alts);
+	if (usblp->dev->quirks & USB_QUIRK_NO_SET_INTF) {
+		r = 0;
+	} else {
+		r = usb_set_interface(usblp->dev, usblp->ifnum, alts);
+	}
 	if (r < 0) {
 		printk(KERN_ERR "usblp: can't set desired altsetting %d on interface %d\n",
 			alts, usblp->ifnum);
drivers/usb/core/quirks.c +3 -0
index 1b4eb7046b07..632c60401d53 100644
@@ -205,6 +205,9 @@ static const struct usb_device_id usb_quirk_list[] = {
 	/* HP v222w 16GB Mini USB Drive */
 	{ USB_DEVICE(0x03f0, 0x3f40), .driver_info = USB_QUIRK_DELAY_INIT },
 
+	/* Winbond Electronics Corp. Virtual Com Port */
+	{ USB_DEVICE(0x0416, 0x5011), .driver_info = USB_QUIRK_NO_SET_INTF },
+
 	/* Creative SB Audigy 2 NX */
 	{ USB_DEVICE(0x041e, 0x3020), .driver_info = USB_QUIRK_RESET_RESUME },
 

Please let me know if it works for you.

-- Pete

#2 lore raw
Quoted 21 lines · Pete Zaitcev

Review

Quoted 4 lines · On Sun, 17 Jan 2021 15:36:39 -0600 Jeremy Figgins <[email protected]>…

The naming is designed to mirror the existing
USB_QUIRK_NO_SET_INTF flag, but that flag is
not sufficient to make these devices work.

(quoted diff) +1 -0
+	{ 0x0416, 0x5011, USBLP_QUIRK_NO_SET_INTF }, /* Winbond Electronics Corp. Virtual Com Port */

Jeremy, thanks for the patch. It looks mostly fine code-wise (quirk is out of numerical order), but I have a question: did you consider keying off usblp->dev->quirks instead?

How about this:

drivers/usb/class/usblp.c +0 -0
index 37062130a03c..0c4a98f00797 100644
@@ -1315,7 +1315,11 @@ static int usblp_set_protocol(struct usblp *usblp, int protocol)
 	alts = usblp->protocol[protocol].alt_setting;
 	if (alts < 0)

drivers/usb/class/usblp.c static int usblp_set_protocol(struct usblp *usblp, int protocol) · line 1325

Quoted 10 lines
drivers/usb/class/usblp.c +5 -1
 		return -EINVAL;
-	r = usb_set_interface(usblp->dev, usblp->ifnum, alts);
+	if (usblp->dev->quirks & USB_QUIRK_NO_SET_INTF) {
+		r = 0;
+	} else {
+		r = usb_set_interface(usblp->dev, usblp->ifnum, alts);
+	}
 	if (r < 0) {
 		printk(KERN_ERR "usblp: can't set desired altsetting %d on interface %d\n",
 			alts, usblp->ifnum);

Would it be practical simply to skip the usb_set_interface() call whenever alts is 0? After all, devices use altsetting 0 by default; it shouldn't be necessary to tell them to do so.

Alan Stern

#3 lore raw

FWIW, the CUPS libusb-based backend only sets the alt setting if there is more than 1 alt setting in the descriptor.

Quoted 39 lines · Alan Stern
Quoted 21 lines · Pete Zaitcev

Review

Quoted 4 lines · On Sun, 17 Jan 2021 15:36:39 -0600 Jeremy Figgins <[email protected]>…

The naming is designed to mirror the existing
USB_QUIRK_NO_SET_INTF flag, but that flag is
not sufficient to make these devices work.

(quoted diff) +1 -0
+	{ 0x0416, 0x5011, USBLP_QUIRK_NO_SET_INTF }, /* Winbond Electronics Corp. Virtual Com Port */

Jeremy, thanks for the patch. It looks mostly fine code-wise (quirk is out of numerical order), but I have a question: did you consider keying off usblp->dev->quirks instead?

How about this:

drivers/usb/class/usblp.c +0 -0
index 37062130a03c..0c4a98f00797 100644
@@ -1315,7 +1315,11 @@ static int usblp_set_protocol(struct usblp *usblp, int protocol)
 	alts = usblp->protocol[protocol].alt_setting;
 	if (alts < 0)

drivers/usb/class/usblp.c static int usblp_set_protocol(struct usblp *usblp, int protocol) · line 1325

Quoted 10 lines
drivers/usb/class/usblp.c +5 -1
 		return -EINVAL;
-	r = usb_set_interface(usblp->dev, usblp->ifnum, alts);
+	if (usblp->dev->quirks & USB_QUIRK_NO_SET_INTF) {
+		r = 0;
+	} else {
+		r = usb_set_interface(usblp->dev, usblp->ifnum, alts);
+	}
 	if (r < 0) {
 		printk(KERN_ERR "usblp: can't set desired altsetting %d on interface %d\n",
 			alts, usblp->ifnum);

Would it be practical simply to skip the usb_set_interface() call whenever alts is 0? After all, devices use altsetting 0 by default; it shouldn't be necessary to tell them to do so.

Quoted 1 line

Alan Stern

________________________
Michael Sweet

#4 lore raw
Quoted 42 lines · Michael Sweet

FWIW, the CUPS libusb-based backend only sets the alt setting if there is more than 1 alt setting in the descriptor.

Quoted 39 lines · Alan Stern
Quoted 21 lines · Pete Zaitcev

Review

Quoted 4 lines · On Sun, 17 Jan 2021 15:36:39 -0600 Jeremy Figgins <[email protected]>…

The naming is designed to mirror the existing
USB_QUIRK_NO_SET_INTF flag, but that flag is
not sufficient to make these devices work.

(quoted diff) +1 -0
+	{ 0x0416, 0x5011, USBLP_QUIRK_NO_SET_INTF }, /* Winbond Electronics Corp. Virtual Com Port */

Jeremy, thanks for the patch. It looks mostly fine code-wise (quirk is out of numerical order), but I have a question: did you consider keying off usblp->dev->quirks instead?

How about this:

drivers/usb/class/usblp.c +0 -0
index 37062130a03c..0c4a98f00797 100644
@@ -1315,7 +1315,11 @@ static int usblp_set_protocol(struct usblp *usblp, int protocol)
 	alts = usblp->protocol[protocol].alt_setting;
 	if (alts < 0)

drivers/usb/class/usblp.c static int usblp_set_protocol(struct usblp *usblp, int protocol) · line 1325

Quoted 10 lines
drivers/usb/class/usblp.c +5 -1
 		return -EINVAL;
-	r = usb_set_interface(usblp->dev, usblp->ifnum, alts);
+	if (usblp->dev->quirks & USB_QUIRK_NO_SET_INTF) {
+		r = 0;
+	} else {
+		r = usb_set_interface(usblp->dev, usblp->ifnum, alts);
+	}
 	if (r < 0) {
 		printk(KERN_ERR "usblp: can't set desired altsetting %d on interface %d\n",
 			alts, usblp->ifnum);

Would it be practical simply to skip the usb_set_interface() call whenever alts is 0? After all, devices use altsetting 0 by default; it shouldn't be necessary to tell them to do so.

Quoted 8 lines
Quoted 1 line

Alan Stern

________________________
Michael Sweet

Pete, your proposed change does work. I created USBLP_QUIRK_NO_SET_INTF because I was concerned about overloading the meaning of USB_QUIRK_NO_SET_INTF, but if you think that's the better approach, I'm happy to resubmit the patch.

Alan, just to confirm, alts=0 for this device.

Jeremy Figgins

maintainer
#5 lore raw
Quoted 4 lines · On Mon, 18 Jan 2021 18:01:53 -0600 Jeremy Figgins <[email protected]>…

Pete, your proposed change does work. I created USBLP_QUIRK_NO_SET_INTF because I was concerned about overloading the meaning of USB_QUIRK_NO_SET_INTF, but if you think that's the better approach, I'm happy to resubmit the patch.

I do think it's better, so if you submit the channged version, I'll ack. But note that we need to get Alan Stern onboard too.

-- Pete

maintainer
#6 lore raw
In reply to Alan Stern
Quoted 13 lines · On Mon, 18 Jan 2021 11:31:17 -0500 Alan Stern <[email protected]> wrote:
Quoted 13 lines
drivers/usb/class/usblp.c +4 -1
index 37062130a03c..0c4a98f00797 100644
@@ -1315,7 +1315,11 @@ static int usblp_set_protocol(struct usblp *usblp, int protocol)
 	alts = usblp->protocol[protocol].alt_setting;
 	if (alts < 0)
 		return -EINVAL;
-	r = usb_set_interface(usblp->dev, usblp->ifnum, alts);
+	if (usblp->dev->quirks & USB_QUIRK_NO_SET_INTF) {
+		r = 0;
+	} else {
+		r = usb_set_interface(usblp->dev, usblp->ifnum, alts);
Quoted 8 lines

drivers/usb/class/usblp.c static int usblp_set_protocol(struct usblp *usblp, int protocol) · line 1325

Quoted 4 lines
drivers/usb/class/usblp.c +1 -0
+	}
 	if (r < 0) {
 		printk(KERN_ERR "usblp: can't set desired altsetting %d on interface %d\n",
 			alts, usblp->ifnum);  

Would it be practical simply to skip the usb_set_interface() call whenever alts is 0? After all, devices use altsetting 0 by default; it shouldn't be necessary to tell them to do so.

Is it possible to bind and unbind the driver without enumeration, and thus inherit a non-zero altsetting?

I'm also concerned about regressions. This is a legacy class driver, only used where CUPS is not applicable, mostly with truly ancient devices. So yes, setting a zero altsetting after enumeration should be unnecessary. But you never know with the old firmware.

-- Pete

#7 lore raw
Quoted 9 lines · Pete Zaitcev
Quoted 3 lines · On Mon, 18 Jan 2021 11:31:17 -0500 Alan Stern <[email protected]> wrote:

Would it be practical simply to skip the usb_set_interface() call whenever alts is 0? After all, devices use altsetting 0 by default; it shouldn't be necessary to tell them to do so.

Is it possible to bind and unbind the driver without enumeration, and thus inherit a non-zero altsetting?

In theory, yes. But the only way it could happen is if either the driver itself or a userspace program installed the nonzero altsetting.

Quoted 4 lines

I'm also concerned about regressions. This is a legacy class driver, only used where CUPS is not applicable, mostly with truly ancient devices. So yes, setting a zero altsetting after enumeration should be unnecessary. But you never know with the old firmware.

True, although I seriously doubt anyone would have written firmware that required a Set-Interface request for initialization. Particularly if the interface has only one altsetting.

How about skipping the call whenever the interface has only one altsetting?

Alan Stern

maintainer
#8 lore raw
Quoted 7 lines · On Thu, 21 Jan 2021 14:29:29 -0500 Alan Stern <[email protected]> wrote:
Quoted 4 lines

I'm also concerned about regressions. This is a legacy class driver, only used where CUPS is not applicable, mostly with truly ancient devices. So yes, setting a zero altsetting after enumeration should be unnecessary. But you never know with the old firmware.

How about skipping the call whenever the interface has only one altsetting?

Do you mean when it's only one and not equal to zero?

BTW, one other thing bothers me. Jeremy confirmed that my patch worked, which skips the call when USB_QUIRK_NO_SET_INTF is set. But if we look into drivers/usb/core/message.c, the control exchange to set the altsetting is skipped in that case anyway. So, usblp was calling usb_set_protocol, the suspect control was skipped, but something else caused a problem. Could it be the attempt to clear halt that triggered the problem?

-- Pete

#9 lore raw
Quoted 21 lines · Pete Zaitcev
Quoted 7 lines · On Thu, 21 Jan 2021 14:29:29 -0500 Alan Stern <[email protected]> wrote:
Quoted 4 lines

I'm also concerned about regressions. This is a legacy class driver, only used where CUPS is not applicable, mostly with truly ancient devices. So yes, setting a zero altsetting after enumeration should be unnecessary. But you never know with the old firmware.

How about skipping the call whenever the interface has only one altsetting?

Do you mean when it's only one and not equal to zero?

BTW, one other thing bothers me. Jeremy confirmed that my patch worked, which skips the call when USB_QUIRK_NO_SET_INTF is set. But if we look into drivers/usb/core/message.c, the control exchange to set the altsetting is skipped in that case anyway. So, usblp was calling usb_set_protocol, the suspect control was skipped, but something else caused a problem. Could it be the attempt to clear halt that triggered the problem?

Quoted 2 lines

-- Pete

In my debugging, I found that it was the calls to usb_control_msg_send() in both usb_set_interface() and usb_clear_halt() caused the printer to lockup. I suppose another way to resolve this would to have a flag to prevent usb_clear_halt()'s call to usb_control_msg_send(), but I'm not an expert in USB, so I'm not sure of the ramifications of that.

-- Jeremy

#10 lore raw
Quoted 26 lines · Jeremy Figgins
Quoted 21 lines · Pete Zaitcev
Quoted 7 lines · On Thu, 21 Jan 2021 14:29:29 -0500 Alan Stern <[email protected]> wrote:
Quoted 4 lines

I'm also concerned about regressions. This is a legacy class driver, only used where CUPS is not applicable, mostly with truly ancient devices. So yes, setting a zero altsetting after enumeration should be unnecessary. But you never know with the old firmware.

How about skipping the call whenever the interface has only one altsetting?

Do you mean when it's only one and not equal to zero?

BTW, one other thing bothers me. Jeremy confirmed that my patch worked, which skips the call when USB_QUIRK_NO_SET_INTF is set. But if we look into drivers/usb/core/message.c, the control exchange to set the altsetting is skipped in that case anyway. So, usblp was calling usb_set_protocol, the suspect control was skipped, but something else caused a problem. Could it be the attempt to clear halt that triggered the problem?

Quoted 2 lines

-- Pete

Quoted 5 lines

In my debugging, I found that it was the calls to usb_control_msg_send() in both usb_set_interface() and usb_clear_halt() caused the printer to lockup. I suppose another way to resolve this would to have a flag to prevent usb_clear_halt()'s call to usb_control_msg_send(), but I'm not an expert in USB, so I'm not sure of the ramifications of that.

The simplest solution in all cases is just to avoid calling either usb_set_interface or usb_clear_halt. Especially since in cases where the interface has only one altsetting, neither call should be necessary.

Alan Stern

#11 lore raw
Quoted 33 lines · Alan Stern
Quoted 26 lines · Jeremy Figgins
Quoted 21 lines · Pete Zaitcev
Quoted 7 lines · On Thu, 21 Jan 2021 14:29:29 -0500 Alan Stern <[email protected]> wrote:
Quoted 4 lines

I'm also concerned about regressions. This is a legacy class driver, only used where CUPS is not applicable, mostly with truly ancient devices. So yes, setting a zero altsetting after enumeration should be unnecessary. But you never know with the old firmware.

How about skipping the call whenever the interface has only one altsetting?

Do you mean when it's only one and not equal to zero?

BTW, one other thing bothers me. Jeremy confirmed that my patch worked, which skips the call when USB_QUIRK_NO_SET_INTF is set. But if we look into drivers/usb/core/message.c, the control exchange to set the altsetting is skipped in that case anyway. So, usblp was calling usb_set_protocol, the suspect control was skipped, but something else caused a problem. Could it be the attempt to clear halt that triggered the problem?

Quoted 2 lines

-- Pete

Quoted 5 lines

In my debugging, I found that it was the calls to usb_control_msg_send() in both usb_set_interface() and usb_clear_halt() caused the printer to lockup. I suppose another way to resolve this would to have a flag to prevent usb_clear_halt()'s call to usb_control_msg_send(), but I'm not an expert in USB, so I'm not sure of the ramifications of that.

Quoted 6 lines

The simplest solution in all cases is just to avoid calling either usb_set_interface or usb_clear_halt. Especially since in cases where the interface has only one altsetting, neither call should be necessary.

Alan Stern

I can confirm that only calling usb_clear_halt() if USB_QUIRK_NO_SET_INTF is not set (and setting it for my device) does allow my device to work. What is the next step? Should I submit a v2 patch?

-- Jeremy

#12 lore raw
Quoted 38 lines · Jeremy Figgins
Quoted 37 lines · Alan Stern
Quoted 26 lines · Jeremy Figgins
Quoted 21 lines · Pete Zaitcev
Quoted 7 lines · On Thu, 21 Jan 2021 14:29:29 -0500 Alan Stern <[email protected]> wrote:
Quoted 4 lines

I'm also concerned about regressions. This is a legacy class driver, only used where CUPS is not applicable, mostly with truly ancient devices. So yes, setting a zero altsetting after enumeration should be unnecessary. But you never know with the old firmware.

How about skipping the call whenever the interface has only one altsetting?

Do you mean when it's only one and not equal to zero?

BTW, one other thing bothers me. Jeremy confirmed that my patch worked, which skips the call when USB_QUIRK_NO_SET_INTF is set. But if we look into drivers/usb/core/message.c, the control exchange to set the altsetting is skipped in that case anyway. So, usblp was calling usb_set_protocol, the suspect control was skipped, but something else caused a problem. Could it be the attempt to clear halt that triggered the problem?

Quoted 2 lines

-- Pete

Quoted 5 lines

In my debugging, I found that it was the calls to usb_control_msg_send() in both usb_set_interface() and usb_clear_halt() caused the printer to lockup. I suppose another way to resolve this would to have a flag to prevent usb_clear_halt()'s call to usb_control_msg_send(), but I'm not an expert in USB, so I'm not sure of the ramifications of that.

The simplest solution in all cases is just to avoid calling either usb_set_interface or usb_clear_halt. Especially since in cases where the interface has only one altsetting, neither call should be necessary.

Quoted 6 lines
Quoted 2 lines

Alan Stern

I can confirm that only calling usb_clear_halt() if USB_QUIRK_NO_SET_INTF is not set (and setting it for my device) does allow my device to work. What is the next step? Should I submit a v2 patch?

Why don't you write, test, and submit a patch that will make usblp avoid calling usb_set_interface and usb_clear_halt when there's only one altsetting?

Alan Stern

#13 lore raw
In reply to Pete Zaitcev
Quoted 8 lines · Pete Zaitcev
Quoted 4 lines · On Thu, 21 Jan 2021 14:29:29 -0500 Alan Stern <[email protected]> wrote:
Quoted 4 lines

I'm also concerned about regressions. This is a legacy class driver, only used where CUPS is not applicable, mostly with truly ancient devices. So yes, setting a zero altsetting after enumeration should be unnecessary. But you never know with the old firmware.

Quoted 4 lines
Quoted 2 lines

How about skipping the call whenever the interface has only one altsetting?

Do you mean when it's only one and not equal to zero?

If there's only one, it _has_ to be equal to 0. According to section 9.2.3 of the USB-2 spec:

Alternate settings range from zero to one less than the number of alternate settings for a specific interface.

Quoted 7 lines

BTW, one other thing bothers me. Jeremy confirmed that my patch worked, which skips the call when USB_QUIRK_NO_SET_INTF is set. But if we look into drivers/usb/core/message.c, the control exchange to set the altsetting is skipped in that case anyway. So, usblp was calling usb_set_protocol, the suspect control was skipped, but something else caused a problem. Could it be the attempt to clear halt that triggered the problem?

It could very well be. The printer might not reset the endpoint toggle when it gets the Clear-Halt request.

It's also possible that when the quirk flag wasn't set (so the Set-Interface request was issued), the printer failed reset the endpoint toggle.

Alan Stern

#14 lore raw
In reply to Jeremy Figgins
Quoted 3 lines · Jeremy Figgins

Certain devices such as Winbond Virtual Com Port, which is used in some usb printers, will stop responding after the usb_control_msg_send()

Hm, not usblp_set_protocol()?

Quoted 7 lines

calls in usb_set_interface(). These devices work fine without having usb_set_interface() called, so this flag prevents that call.

The naming is designed to mirror the existing USB_QUIRK_NO_SET_INTF flag, but that flag is not sufficient to make these devices work.

Perhaps the handling of that flag should just be extended to yuor case?

Quoted 9 lines
---
  drivers/usb/class/usblp.c | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)
drivers/usb/class/usblp.c +0 -0
index 134dc2005ce9..6e2d58813d7d 100644

[...]

drivers/usb/class/usblp.c static int usblp_set_protocol(struct usblp *usblp, int protocol) · line 1341

Quoted 10 lines
drivers/usb/class/usblp.c +5 -1
@@ -1332,7 +1334,11 @@ static int usblp_set_protocol(struct usblp *usblp, int protocol)
  	alts = usblp->protocol[protocol].alt_setting;
  	if (alts < 0)
  		return -EINVAL;
-	r = usb_set_interface(usblp->dev, usblp->ifnum, alts);
+	if (usblp->quirks & USBLP_QUIRK_NO_SET_INTF) {
+		r = 0;
+	} else {
+		r = usb_set_interface(usblp->dev, usblp->ifnum, alts);
+	}

The braces above not needed at all.

[...]

MBR, Sergei