linux-kernel - Re: [PATCH V2 2/2] MSP onchip root hub over current quirk.

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: <Pine.LNX.4.44L0.1012222112200.26667-100000@netrider.rowland.org>
Date:	Wed, 22 Dec 2010 21:18:20 -0500 (EST)
From:	Alan Stern <stern@...land.harvard.edu>
To:	"Anoop P.A" <anoop.pa@...il.com>
cc:	Ralf Baechle <ralf@...ux-mips.org>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Anatolij Gustschin <agust@...x.de>,
	Anand Gadiyar <gadiyar@...com>, <linux-mips@...ux-mips.org>,
	<linux-kernel@...r.kernel.org>, <linux-usb@...r.kernel.org>,
	Sarah Sharp <sarah.a.sharp@...ux.intel.com>,
	Oliver Neukum <oneukum@...e.de>,
	Hans de Goede <hdegoede@...hat.com>,
	Paul Mortier <mortier@...nternet.com>,
	Andiry Xu <andiry.xu@....com>
Subject: Re: [PATCH V2 2/2] MSP onchip root hub over current quirk.

On Wed, 22 Dec 2010, Anoop P.A wrote:

> From: Anoop P A <anoop.pa@...il.com>
> 
> Adding chip specific code under quirk.

NAK.  See below.

> Signed-off-by: Anoop P A <anoop.pa@...il.com>
> ---
>  drivers/usb/core/hub.c     |   45 ++++++++++++++++++++++++++++++++++++++-----
>  drivers/usb/core/quirks.c  |    3 ++
>  include/linux/usb/quirks.h |    3 ++
>  3 files changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 27115b4..4bff994 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3377,12 +3377,45 @@ static void hub_events(void)
>  			}
>  			
>  			if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
> -				dev_err (hub_dev,
> -					"over-current change on port %d\n",
> -					i);
> -				clear_port_feature(hdev, i,
> -					USB_PORT_FEAT_C_OVER_CURRENT);
> -				hub_power_on(hub, true);
> +				usb_detect_quirks(hdev);

This line is wrong.  usb_detect_quirks() gets called only once per 
device, when the device is initialized.  Besides, you probably want to 
use a hub-specific flag for this rather than a device-specific flag.

> +				if (hdev->quirks & USB_QUIRK_MSP_OVERCURRENT) {

Also, it would be better to put this code in a separate subroutine 
instead of indenting it so far.

> +					/* clear OCC bit */
> +					clear_port_feature(hdev, i,
> +						USB_PORT_FEAT_C_OVER_CURRENT);
> +
> +					/* This step is required to toggle the
> +					* PP bit to 0 and 1 (by hub_power_on)
> +					* in order the CSC bit to be
> +					* transitioned properly for device
> +					* hotplug
> +					*/
> +					/* clear PP bit */
> +					clear_port_feature(hdev, i,
> +						USB_PORT_FEAT_POWER);
> +
> +					/* resume power */
> +					hub_power_on(hub, true);
> +
> +					/* delay 100 usec */
> +					udelay(100);
> +
> +					/* read OCA bit */
> +					if (portstatus &
> +					(1<<USB_PORT_FEAT_OVER_CURRENT)) {
> +						/* declare overcurrent */
> +						dev_err(hub_dev,
> +						"over-current change \
> +							on port %d\n", i);
> +					}
> +				} else {
> +					dev_err(hub_dev,
> +						"over-current change \
> +							on port %d\n", i);
> +					clear_port_feature(hdev, i,
> +						USB_PORT_FEAT_C_OVER_CURRENT);
> +					hub_power_on(hub, true);
> +				}
> +
>  			}
>  
>  			if (portchange & USB_PORT_STAT_C_RESET) {
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index 25719da..59843b9 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -88,6 +88,9 @@ static const struct usb_device_id usb_quirk_list[] = {
>  	/* INTEL VALUE SSD */
>  	{ USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME },
>  
> +	/* PMC MSP over current quirk */
> +	{ USB_DEVICE(0x1d6b, 0x0002), .driver_info = USB_QUIRK_MSP_OVERCURRENT },
> +

This implementation is completely wrong.  It applies to all USB-2.0
root hubs in Linux, not just the PMC MSP.

>  	{ }  /* terminating entry must be last */
>  };
>  
> diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
> index 3e93de7..97ab168 100644
> --- a/include/linux/usb/quirks.h
> +++ b/include/linux/usb/quirks.h
> @@ -30,4 +30,7 @@
>     descriptor */
>  #define USB_QUIRK_DELAY_INIT		0x00000040
>  
> +/*MSP SoC onchip EHCI overcurrent issue */
> +#define USB_QUIRK_MSP_OVERCURRENT	0x00000080
> +
>  #endif /* __LINUX_USB_QUIRKS_H */
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/