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