[PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[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()
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.

Signed-off-by: Jeremy Figgins <kernel@jeremyfiggins.com>
---
 drivers/usb/class/usblp.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
index 134dc2005ce9..6e2d58813d7d 100644
--- a/drivers/usb/class/usblp.c
+++ b/drivers/usb/class/usblp.c
@@ -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 <kernel@zut.de> */
 	{ 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);
-- 
2.29.0

Re: [PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[Pete Zaitcev]

On Sun, 17 Jan 2021 15:36:39 -0600
Jeremy Figgins <kernel@jeremyfiggins.com> wrote:

> 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.
> +	{ 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:

diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
index 37062130a03c..0c4a98f00797 100644
--- a/drivers/usb/class/usblp.c
+++ b/drivers/usb/class/usblp.c
@@ -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);
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 1b4eb7046b07..632c60401d53 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -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

Re: [PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[Sergei Shtylyov]

On 18.01.2021 0:36, Jeremy Figgins wrote:

> 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()?

> 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?

>  Signed-off-by: Jeremy Figgins <kernel@jeremyfiggins.com>
> ---
>   drivers/usb/class/usblp.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
> index 134dc2005ce9..6e2d58813d7d 100644
> --- a/drivers/usb/class/usblp.c
> +++ b/drivers/usb/class/usblp.c
[...]
> @@ -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

Re: [PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[Alan Stern]

On Sun, Jan 17, 2021 at 11:44:16PM -0600, Pete Zaitcev wrote:
> On Sun, 17 Jan 2021 15:36:39 -0600
> Jeremy Figgins <kernel@jeremyfiggins.com> wrote:
> 
> > 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.
> > +	{ 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:
> 
> diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
> index 37062130a03c..0c4a98f00797 100644
> --- a/drivers/usb/class/usblp.c
> +++ b/drivers/usb/class/usblp.c
> @@ -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);

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

Re: [PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[Michael Sweet]

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


> On Jan 18, 2021, at 11:31 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> On Sun, Jan 17, 2021 at 11:44:16PM -0600, Pete Zaitcev wrote:
>> On Sun, 17 Jan 2021 15:36:39 -0600
>> Jeremy Figgins <kernel@jeremyfiggins.com> wrote:
>> 
>>> 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.
>>> +	{ 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:
>> 
>> diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
>> index 37062130a03c..0c4a98f00797 100644
>> --- a/drivers/usb/class/usblp.c
>> +++ b/drivers/usb/class/usblp.c
>> @@ -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);
> 
> 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

________________________
Michael Sweet

Re: [PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[Jeremy Figgins]

On 1/18/21 10:43 AM, Michael Sweet wrote:
> FWIW, the CUPS libusb-based backend only sets the alt setting if there is more than 1 alt setting in the descriptor.
> 
> 
>> On Jan 18, 2021, at 11:31 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>>
>> On Sun, Jan 17, 2021 at 11:44:16PM -0600, Pete Zaitcev wrote:
>>> On Sun, 17 Jan 2021 15:36:39 -0600
>>> Jeremy Figgins <kernel@jeremyfiggins.com> wrote:
>>>
>>>> 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.
>>>> +	{ 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:
>>>
>>> diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
>>> index 37062130a03c..0c4a98f00797 100644
>>> --- a/drivers/usb/class/usblp.c
>>> +++ b/drivers/usb/class/usblp.c
>>> @@ -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);
>>
>> 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
> 
> ________________________
> 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

Re: [PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[Pete Zaitcev]

On Mon, 18 Jan 2021 11:31:17 -0500
Alan Stern <stern@rowland.harvard.edu> wrote:

> > diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
> > index 37062130a03c..0c4a98f00797 100644
> > --- a/drivers/usb/class/usblp.c
> > +++ b/drivers/usb/class/usblp.c
> > @@ -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);  
> 
> 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

Re: [PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[Pete Zaitcev]

On Mon, 18 Jan 2021 18:01:53 -0600
Jeremy Figgins <jeremy@jeremyfiggins.com> wrote:

> 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

Re: [PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[Alan Stern]

On Thu, Jan 21, 2021 at 01:19:54PM -0600, Pete Zaitcev wrote:
> On Mon, 18 Jan 2021 11:31:17 -0500
> Alan Stern <stern@rowland.harvard.edu> 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.

> 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

Re: [PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[Pete Zaitcev]

On Thu, 21 Jan 2021 14:29:29 -0500
Alan Stern <stern@rowland.harvard.edu> wrote:

> > 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

Re: [PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[Jeremy Figgins]

On 1/21/21 5:02 PM, Pete Zaitcev wrote:
> On Thu, 21 Jan 2021 14:29:29 -0500
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
>>> 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
> 


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

Re: [PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[Alan Stern]

On Thu, Jan 21, 2021 at 05:02:49PM -0600, Pete Zaitcev wrote:
> On Thu, 21 Jan 2021 14:29:29 -0500
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > > 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?

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.

> 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

Re: [PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[Alan Stern]

On Thu, Jan 21, 2021 at 07:06:27PM -0600, Jeremy Figgins wrote:
> On 1/21/21 5:02 PM, Pete Zaitcev wrote:
> > On Thu, 21 Jan 2021 14:29:29 -0500
> > Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > > 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
> > 
> 
> 
> 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

Re: [PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[Jeremy Figgins]

On 1/22/21 10:22 AM, Alan Stern wrote:
> On Thu, Jan 21, 2021 at 07:06:27PM -0600, Jeremy Figgins wrote:
>> On 1/21/21 5:02 PM, Pete Zaitcev wrote:
>>> On Thu, 21 Jan 2021 14:29:29 -0500
>>> Alan Stern <stern@rowland.harvard.edu> wrote:
>>>
>>>>> 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
>>>
>>
>>
>> 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
> 

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

Re: [PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[Alan Stern]

On Sat, Jan 23, 2021 at 12:46:06PM -0600, Jeremy Figgins wrote:
> On 1/22/21 10:22 AM, Alan Stern wrote:
> > On Thu, Jan 21, 2021 at 07:06:27PM -0600, Jeremy Figgins wrote:
> > > On 1/21/21 5:02 PM, Pete Zaitcev wrote:
> > > > On Thu, 21 Jan 2021 14:29:29 -0500
> > > > Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > 
> > > > > > 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
> > > > 
> > > 
> > > 
> > > 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
> > 
> 
> 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

[PATCH] USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

[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()
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.

Signed-off-by: Jeremy Figgins <kernel@jeremyfiggins.com>
---
 drivers/usb/class/usblp.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
index 134dc2005ce9..6e2d58813d7d 100644
--- a/drivers/usb/class/usblp.c
+++ b/drivers/usb/class/usblp.c
@@ -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 <kernel@zut.de> */
 	{ 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);
-- 
2.29.0