USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag
Subsystems (2)
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
@@ -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
Pete Zaitcev
maintainerReview
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_INTFflag, 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
@@ -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
@@ -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
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_INTFflag, 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->quirksinstead?How about this:
drivers/usb/class/usblp.c +0
-0@@ -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
-1return -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
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_INTFflag, 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->quirksinstead?How about this:
drivers/usb/class/usblp.c +0
-0@@ -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
-1return -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
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_INTFflag, 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->quirksinstead?How about this:
drivers/usb/class/usblp.c +0
-0@@ -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
-1return -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
Pete Zaitcev
maintainerQuoted 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_INTFbecause I was concerned about overloading the meaning ofUSB_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
Pete Zaitcev
maintainerQuoted 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@@ -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
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
Pete Zaitcev
maintainerQuoted 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
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_INTFis set. But if we look intodrivers/usb/core/message.c, the control exchange to set the altsetting is skipped in that case anyway. So, usblp was callingusb_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
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_INTFis set. But if we look intodrivers/usb/core/message.c, the control exchange to set the altsetting is skipped in that case anyway. So, usblp was callingusb_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 bothusb_set_interface()andusb_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 tousb_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
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_INTFis set. But if we look intodrivers/usb/core/message.c, the control exchange to set the altsetting is skipped in that case anyway. So, usblp was callingusb_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 bothusb_set_interface()andusb_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 tousb_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_interfaceorusb_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
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_INTFis set. But if we look intodrivers/usb/core/message.c, the control exchange to set the altsetting is skipped in that case anyway. So, usblp was callingusb_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 bothusb_set_interface()andusb_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 tousb_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_interfaceorusb_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()ifUSB_QUIRK_NO_SET_INTFis 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
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_INTFis set. But if we look intodrivers/usb/core/message.c, the control exchange to set the altsetting is skipped in that case anyway. So, usblp was callingusb_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
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 havingusb_set_interface()called, so this flag prevents that call.The naming is designed to mirror the existing
USB_QUIRK_NO_SET_INTFflag, 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
Signed-off-by Jeremy Figgins <[email protected]>--- drivers/usb/class/usblp.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)drivers/usb/class/usblp.c +0
-0
[...]
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