[PATCH v2] usb: misc: ehset: Workaround for "special" hubs

[PATCH v2] usb: misc: ehset: Workaround for "special" hubs

[Razvan Heghedus]

The USB2.0 spec chapter 11.24.2.13 says that the USB port which is going
under test needs to be put in suspend state before sending the test
command. Many hubs, don't enforce this precondition and they work fine
without this step. But there are some "special" hubs, which requires to
disable the port power before sending the test command.

Because the USB spec mention that the port should be suspended, also
do this step before sending the test command. This could rise the
problem with other hubs which are not compliant with the spec and the
test command will not work if the port is suspend. If such hubs are
found, a similar workaround like the disable part could be implemented
to skip the suspend port command.

Signed-off-by: Razvan Heghedus <heghedus.razvan@gmail.com>
---
 Changes in v2:
  - style change regarding multi-line comments and a new black line
    after local variable definitions
  - No more corporate email annotation
This time without that corporate email annotation.
Also has a couple of style changes regardind multi-line comments and a
black line after local variable definitions.
 drivers/usb/misc/ehset.c | 81 ++++++++++++++++++++++++++++++++--------
 1 file changed, 65 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/misc/ehset.c b/drivers/usb/misc/ehset.c
index f87890f9cd26..b848bbdee802 100644
--- a/drivers/usb/misc/ehset.c
+++ b/drivers/usb/misc/ehset.c
@@ -18,6 +18,47 @@
 #define TEST_SINGLE_STEP_GET_DEV_DESC		0x0107
 #define TEST_SINGLE_STEP_SET_FEATURE		0x0108
 
+/*
+ * A list of USB hubs which requires to disable the power
+ * to the port before starting the testing procedures.
+ */
+static const struct usb_device_id ehset_hub_list[] = {
+	{USB_DEVICE(0x0424, 0x4502)},
+	{USB_DEVICE(0x0424, 0x4913)},
+	{USB_DEVICE(0x0451, 0x8027)},
+	{}
+};
+
+static int ehset_prepare_port_for_testing(struct usb_device *hub_udev, u16 portnum)
+{
+	int ret = 0;
+
+	/*
+	 * The USB2.0 spec chapter 11.24.2.13 says that the USB port which is
+	 * going under test needs to be put in suspend before sending the
+	 * test command. Most hubs don't enforce this precondition, but there
+	 * are some hubs which needs to disable the power to the port before
+	 * starting the test.
+	 */
+	if (usb_match_id(to_usb_interface(hub_udev->dev.parent), ehset_hub_list)) {
+		ret = usb_control_msg_send(hub_udev, 0, USB_REQ_CLEAR_FEATURE,
+					   USB_RT_PORT, USB_PORT_FEAT_ENABLE,
+					   portnum, NULL, 0, 1000, GFP_KERNEL);
+		/* Wait for the port to be disabled. It's an arbitrary value
+		 * which worked every time.
+		 */
+		msleep(100);
+	} else {
+		/* For the hubs which are compliant with the spec,
+		 * put the port in SUSPEND.
+		 */
+		ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+					   USB_RT_PORT, USB_PORT_FEAT_SUSPEND,
+					   portnum, NULL, 0, 1000, GFP_KERNEL);
+	}
+	return ret;
+}
+
 static int ehset_probe(struct usb_interface *intf,
 		       const struct usb_device_id *id)
 {
@@ -30,28 +71,36 @@ static int ehset_probe(struct usb_interface *intf,
 
 	switch (test_pid) {
 	case TEST_SE0_NAK_PID:
-		ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
-					   USB_RT_PORT, USB_PORT_FEAT_TEST,
-					   (USB_TEST_SE0_NAK << 8) | portnum,
-					   NULL, 0, 1000, GFP_KERNEL);
+		ret = ehset_prepare_port_for_testing(hub_udev, portnum);
+		if (!ret)
+			ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+						   USB_RT_PORT, USB_PORT_FEAT_TEST,
+						   (USB_TEST_SE0_NAK << 8) | portnum,
+						   NULL, 0, 1000, GFP_KERNEL);
 		break;
 	case TEST_J_PID:
-		ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
-					   USB_RT_PORT, USB_PORT_FEAT_TEST,
-					   (USB_TEST_J << 8) | portnum, NULL, 0,
-					   1000, GFP_KERNEL);
+		ret = ehset_prepare_port_for_testing(hub_udev, portnum);
+		if (!ret)
+			ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+						   USB_RT_PORT, USB_PORT_FEAT_TEST,
+						   (USB_TEST_J << 8) | portnum, NULL, 0,
+						   1000, GFP_KERNEL);
 		break;
 	case TEST_K_PID:
-		ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
-					   USB_RT_PORT, USB_PORT_FEAT_TEST,
-					   (USB_TEST_K << 8) | portnum, NULL, 0,
-					   1000, GFP_KERNEL);
+		ret = ehset_prepare_port_for_testing(hub_udev, portnum);
+		if (!ret)
+			ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+						   USB_RT_PORT, USB_PORT_FEAT_TEST,
+						   (USB_TEST_K << 8) | portnum, NULL, 0,
+						   1000, GFP_KERNEL);
 		break;
 	case TEST_PACKET_PID:
-		ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
-					   USB_RT_PORT, USB_PORT_FEAT_TEST,
-					   (USB_TEST_PACKET << 8) | portnum,
-					   NULL, 0, 1000, GFP_KERNEL);
+		ret = ehset_prepare_port_for_testing(hub_udev, portnum);
+		if (!ret)
+			ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+						   USB_RT_PORT, USB_PORT_FEAT_TEST,
+						   (USB_TEST_PACKET << 8) | portnum,
+						   NULL, 0, 1000, GFP_KERNEL);
 		break;
 	case TEST_HS_HOST_PORT_SUSPEND_RESUME:
 		/* Test: wait for 15secs -> suspend -> 15secs delay -> resume */
-- 
2.33.0

Re: [PATCH v2] usb: misc: ehset: Workaround for "special" hubs

[Johan Hovold]

On Wed, Sep 15, 2021 at 03:16:13PM +0300, Razvan Heghedus wrote:
> The USB2.0 spec chapter 11.24.2.13 says that the USB port which is going
> under test needs to be put in suspend state before sending the test
> command. Many hubs, don't enforce this precondition and they work fine
> without this step. But there are some "special" hubs, which requires to
> disable the port power before sending the test command.

So you're essentially doing two things in one patch here; you now
sending a suspend request for all hubs except a for the ones in the
quirk list that are sent a port power disable.

This isn't really reflected in the commit summary which says "workaround
for special hubs" as you're also changing the default implementation.

Probably better handled in two patches, or at least this needs to be
reflected in the commit summary/message better.

But this patch is so broken that I just sent a revert. There also some
style issues that should be addressed if you send a new version.

> Because the USB spec mention that the port should be suspended, also
> do this step before sending the test command. This could rise the
> problem with other hubs which are not compliant with the spec and the
> test command will not work if the port is suspend. If such hubs are
> found, a similar workaround like the disable part could be implemented
> to skip the suspend port command.
> 
> Signed-off-by: Razvan Heghedus <heghedus.razvan@gmail.com>
> ---
>  Changes in v2:
>   - style change regarding multi-line comments and a new black line
>     after local variable definitions
>   - No more corporate email annotation
> This time without that corporate email annotation.
> Also has a couple of style changes regardind multi-line comments and a
> black line after local variable definitions.
>
>  drivers/usb/misc/ehset.c | 81 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 65 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/misc/ehset.c b/drivers/usb/misc/ehset.c
> index f87890f9cd26..b848bbdee802 100644
> --- a/drivers/usb/misc/ehset.c
> +++ b/drivers/usb/misc/ehset.c
> @@ -18,6 +18,47 @@
>  #define TEST_SINGLE_STEP_GET_DEV_DESC		0x0107
>  #define TEST_SINGLE_STEP_SET_FEATURE		0x0108
>  
> +/*
> + * A list of USB hubs which requires to disable the power
> + * to the port before starting the testing procedures.
> + */
> +static const struct usb_device_id ehset_hub_list[] = {
> +	{USB_DEVICE(0x0424, 0x4502)},
> +	{USB_DEVICE(0x0424, 0x4913)},
> +	{USB_DEVICE(0x0451, 0x8027)},
> +	{}

Missing spaces after { and before }.

> +};
> +
> +static int ehset_prepare_port_for_testing(struct usb_device *hub_udev, u16 portnum)
> +{
> +	int ret = 0;
> +
> +	/*
> +	 * The USB2.0 spec chapter 11.24.2.13 says that the USB port which is
> +	 * going under test needs to be put in suspend before sending the
> +	 * test command. Most hubs don't enforce this precondition, but there
> +	 * are some hubs which needs to disable the power to the port before
> +	 * starting the test.
> +	 */
> +	if (usb_match_id(to_usb_interface(hub_udev->dev.parent), ehset_hub_list)) {

This is the main problem: hub_udev->dev.parent does not represent a USB
interface so you cannot use to_usb_interface() or you'd pass a random
pointer to usb_match_id().

If hub_udev is a root hub, then hub_udev->dev.parent does not even
represent a USB device (but, for example, the parent PCI device).

> +		ret = usb_control_msg_send(hub_udev, 0, USB_REQ_CLEAR_FEATURE,
> +					   USB_RT_PORT, USB_PORT_FEAT_ENABLE,
> +					   portnum, NULL, 0, 1000, GFP_KERNEL);
> +		/* Wait for the port to be disabled. It's an arbitrary value
> +		 * which worked every time.
> +		 */

Multi-line comment style is

	/*
	 * blah blah
	 */

> +		msleep(100);
> +	} else {
> +		/* For the hubs which are compliant with the spec,
> +		 * put the port in SUSPEND.
> +		 */

Ditto.

> +		ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
> +					   USB_RT_PORT, USB_PORT_FEAT_SUSPEND,
> +					   portnum, NULL, 0, 1000, GFP_KERNEL);
> +	}
> +	return ret;
> +}
> +
>  static int ehset_probe(struct usb_interface *intf,
>  		       const struct usb_device_id *id)
>  {
> @@ -30,28 +71,36 @@ static int ehset_probe(struct usb_interface *intf,
>  
>  	switch (test_pid) {
>  	case TEST_SE0_NAK_PID:
> -		ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
> -					   USB_RT_PORT, USB_PORT_FEAT_TEST,
> -					   (USB_TEST_SE0_NAK << 8) | portnum,
> -					   NULL, 0, 1000, GFP_KERNEL);
> +		ret = ehset_prepare_port_for_testing(hub_udev, portnum);
> +		if (!ret)
> +			ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
> +						   USB_RT_PORT, USB_PORT_FEAT_TEST,
> +						   (USB_TEST_SE0_NAK << 8) | portnum,
> +						   NULL, 0, 1000, GFP_KERNEL);

Just break if ehset_prepare_port_for_testing() fails, which make the
code much more readable and avoids indenting the current code further
and break the 80 column (soft) rule.

Johan

Re: [PATCH v2] usb: misc: ehset: Workaround for "special" hubs

[Greg Kroah-Hartman]

On Thu, Oct 07, 2021 at 12:21:12PM +0200, Johan Hovold wrote:
> On Wed, Sep 15, 2021 at 03:16:13PM +0300, Razvan Heghedus wrote:
> > The USB2.0 spec chapter 11.24.2.13 says that the USB port which is going
> > under test needs to be put in suspend state before sending the test
> > command. Many hubs, don't enforce this precondition and they work fine
> > without this step. But there are some "special" hubs, which requires to
> > disable the port power before sending the test command.
> 
> So you're essentially doing two things in one patch here; you now
> sending a suspend request for all hubs except a for the ones in the
> quirk list that are sent a port power disable.
> 
> This isn't really reflected in the commit summary which says "workaround
> for special hubs" as you're also changing the default implementation.
> 
> Probably better handled in two patches, or at least this needs to be
> reflected in the commit summary/message better.
> 
> But this patch is so broken that I just sent a revert. There also some
> style issues that should be addressed if you send a new version.
> 
> > Because the USB spec mention that the port should be suspended, also
> > do this step before sending the test command. This could rise the
> > problem with other hubs which are not compliant with the spec and the
> > test command will not work if the port is suspend. If such hubs are
> > found, a similar workaround like the disable part could be implemented
> > to skip the suspend port command.
> > 
> > Signed-off-by: Razvan Heghedus <heghedus.razvan@gmail.com>
> > ---
> >  Changes in v2:
> >   - style change regarding multi-line comments and a new black line
> >     after local variable definitions
> >   - No more corporate email annotation
> > This time without that corporate email annotation.
> > Also has a couple of style changes regardind multi-line comments and a
> > black line after local variable definitions.
> >
> >  drivers/usb/misc/ehset.c | 81 ++++++++++++++++++++++++++++++++--------
> >  1 file changed, 65 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/usb/misc/ehset.c b/drivers/usb/misc/ehset.c
> > index f87890f9cd26..b848bbdee802 100644
> > --- a/drivers/usb/misc/ehset.c
> > +++ b/drivers/usb/misc/ehset.c
> > @@ -18,6 +18,47 @@
> >  #define TEST_SINGLE_STEP_GET_DEV_DESC		0x0107
> >  #define TEST_SINGLE_STEP_SET_FEATURE		0x0108
> >  
> > +/*
> > + * A list of USB hubs which requires to disable the power
> > + * to the port before starting the testing procedures.
> > + */
> > +static const struct usb_device_id ehset_hub_list[] = {
> > +	{USB_DEVICE(0x0424, 0x4502)},
> > +	{USB_DEVICE(0x0424, 0x4913)},
> > +	{USB_DEVICE(0x0451, 0x8027)},
> > +	{}
> 
> Missing spaces after { and before }.
> 
> > +};
> > +
> > +static int ehset_prepare_port_for_testing(struct usb_device *hub_udev, u16 portnum)
> > +{
> > +	int ret = 0;
> > +
> > +	/*
> > +	 * The USB2.0 spec chapter 11.24.2.13 says that the USB port which is
> > +	 * going under test needs to be put in suspend before sending the
> > +	 * test command. Most hubs don't enforce this precondition, but there
> > +	 * are some hubs which needs to disable the power to the port before
> > +	 * starting the test.
> > +	 */
> > +	if (usb_match_id(to_usb_interface(hub_udev->dev.parent), ehset_hub_list)) {
> 
> This is the main problem: hub_udev->dev.parent does not represent a USB
> interface so you cannot use to_usb_interface() or you'd pass a random
> pointer to usb_match_id().
> 
> If hub_udev is a root hub, then hub_udev->dev.parent does not even
> represent a USB device (but, for example, the parent PCI device).

Ugh, good catch, I totally missed this.

I'll go apply the revert now, thank you so much for it.

Razvan, how did you test this?

thanks,

greg k-h

Re: [PATCH v2] usb: misc: ehset: Workaround for "special" hubs

[Johan Hovold]

[ Please avoid top-posting. ]

On Thu, Oct 07, 2021 at 07:51:00PM +0300, heghedus razvan wrote:
> Hi all,
> 
> This was tested only with some external powered hubs. Indeed for the root
> hub there is
> a problem. I see that in the HCDs in hub_control there is the handling for
> testing
> procedures, but I don't know how they are used for the root hub.

This isn't just an issue with root hubs, the current implementation is
just completely broken for all hubs. Which begs the question of how you
tested this, if at all.

> I want to fix this problem, but I don't know how exactly, because I don't
> have a good
> grasp on the USB code since it's a huge beast. The main problem is how can I
> match the VID:PID of the hub_udev(the hub on which the USB testing device
> was connected) with the hub list for which I need to apply the quirk? I
> tried to
> use usb_match_id because I want to use functionality already in the kernel,
> but it seems that in this context I need to do the checking myself.

You can access the interfaces of a USB device through

	udev->actconfig->interface

but in this case it's probably better to just export
usb_device_match_id(), which seems to be what you need.

Johan