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@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).
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
