diff mbox series

USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag

Message ID YASt5wgOCkXhH2Dv@watson
State New
Headers show
Series USB: usblp: add USBLP_QUIRK_NO_SET_INTF flag | expand

Commit Message

Jeremy Figgins Jan. 17, 2021, 9:36 p.m. UTC
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(-)

Comments

Pete Zaitcev Jan. 18, 2021, 5:44 a.m. UTC | #1
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
Sergei Shtylyov Jan. 18, 2021, 9:02 a.m. UTC | #2
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
Alan Stern Jan. 18, 2021, 4:31 p.m. UTC | #3
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 Jan. 18, 2021, 4:43 p.m. UTC | #4
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
Jeremy Figgins Jan. 19, 2021, 12:01 a.m. UTC | #5
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
Pete Zaitcev Jan. 21, 2021, 7:19 p.m. UTC | #6
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
Pete Zaitcev Jan. 21, 2021, 7:21 p.m. UTC | #7
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
Alan Stern Jan. 21, 2021, 7:29 p.m. UTC | #8
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
Pete Zaitcev Jan. 21, 2021, 11:02 p.m. UTC | #9
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
Jeremy Figgins Jan. 22, 2021, 1:06 a.m. UTC | #10
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
Alan Stern Jan. 22, 2021, 4:20 p.m. UTC | #11
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
Alan Stern Jan. 22, 2021, 4:22 p.m. UTC | #12
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
Jeremy Figgins Jan. 23, 2021, 6:46 p.m. UTC | #13
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
Alan Stern Jan. 23, 2021, 9:33 p.m. UTC | #14
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
diff mbox series

Patch

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