linux-kernel - Re: [RFC][PATCH 2/2] usb: dwc2: Add a quirk to allow speed negotiation for Hisilicon Hi6220

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2c58c084-704e-3cac-70a1-c8b31661b3d6@synopsys.com>
Date:   Fri, 14 Oct 2016 12:37:15 -0700
From:   John Youn <John.Youn@...opsys.com>
To:     John Stultz <john.stultz@...aro.org>,
        lkml <linux-kernel@...r.kernel.org>
CC:     Chen Yu <chenyu56@...wei.com>, Wei Xu <xuwei5@...ilicon.com>,
        Guodong Xu <guodong.xu@...aro.org>,
        Amit Pundir <amit.pundir@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        John Youn <John.Youn@...opsys.com>,
        Douglas Anderson <dianders@...omium.org>,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: Re: [RFC][PATCH 2/2] usb: dwc2: Add a quirk to allow speed
 negotiation for Hisilicon Hi6220

On 10/13/2016 4:36 PM, John Stultz wrote:
> From: Chen Yu <chenyu56@...wei.com>
> 
> The Hi6220's usb controller is limited in that it does not
> automatically autonegotiate the usb speed. Thus it requires a
> quirk so that we can manually negotiate the best usb speed for
> the attached device.

Hi,

Could you expand more on this by explaining what exactly is the
limitation and the workaround?

[snip]

> +/*
> + * HPRT0_SPD_HIGH_SPEED: high speed
> + * HPRT0_SPD_FULL_SPEED: full speed
> + */
> +static void dwc2_change_bus_speed(struct usb_hcd *hcd, int speed)
> +{
> +	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
> +
> +	if (hsotg->core_params->speed == speed)
> +		return;
> +
> +	hsotg->core_params->speed = speed;
> +	queue_work(hsotg->wq_otg, &hsotg->wf_otg);
> +}
> +
> +static int dwc2_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
> +{
> +	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
> +
> +	if (!hsotg->change_speed_quirk)
> +		return 1;
> +
> +	hsotg->device_count++;

Why do you need to track the device count?

> +	dev_info(hsotg->dev, "Device count is %u after alloc dev\n",
> +			hsotg->device_count);
> +
> +	return 1;
> +}
> +
> +static void dwc2_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
> +{
> +	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
> +
> +	if (!hsotg->change_speed_quirk)
> +		return;
> +
> +	if (hsotg->device_count)
> +		hsotg->device_count--;
> +
> +	dev_info(hsotg->dev, "Device count is %u after free dev\n",
> +			hsotg->device_count);
> +
> +	if (hsotg->device_count == 1 && udev->parent &&
> +			udev->parent->speed > USB_SPEED_UNKNOWN &&
> +			udev->parent->speed < USB_SPEED_HIGH) {
> +		dev_info(hsotg->dev, "Set speed to default high-speed\n");
> +		dwc2_change_bus_speed(hcd, HPRT0_SPD_HIGH_SPEED);
> +	}
> +}
> +
> +static int dwc2_reset_device(struct usb_hcd *hcd, struct usb_device *udev)
> +{
> +	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
> +
> +	if (!hsotg->change_speed_quirk)
> +		return 0;
> +
> +	if (udev->speed == USB_SPEED_HIGH) {
> +		dev_info(hsotg->dev, "Set speed to high-speed\n");
> +		dwc2_change_bus_speed(hcd, HPRT0_SPD_HIGH_SPEED);
> +	} else if (udev->speed == USB_SPEED_FULL
> +			|| udev->speed == USB_SPEED_LOW) {
> +		dev_info(hsotg->dev, "Set speed to full-speed\n");
> +		dwc2_change_bus_speed(hcd, HPRT0_SPD_FULL_SPEED);
> +	}

It seems you are reinitializing the core every time a device is reset
and the udev->speed does not match the core_param speed. But how is
the udev->speed being set correctly if the hw cannot negotiate the
speed in the first place?

Also should it be for every device? What about if a device gets
plugged in behind a hub? I don't think you want to execute this code
in that case.

This should only affect devices plugged into the root hub, correct?
And the hsotg controller only has one root hub port. It seems things
could be simplified a bit.

Regards,
John