linux-kernel - Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with "max

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]

Date:	Sat, 20 Aug 2011 02:28:06 +0300
From:	Felipe Balbi <balbi@...com>
To:	Michal Nazarewicz <mnazarewicz@...gle.com>
Cc:	Alan Stern <stern@...land.harvard.edu>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Yang Rui Rui <ruirui.r.yang@...to.com>,
	Dave Young <hidave.darkstar@...il.com>,
	Felipe Balbi <balbi@...com>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCHv3 2/4] usb: gadget: replace "is_dualspeed" with
 "max_speed"

Hi,

On Sat, Aug 20, 2011 at 12:33:00AM +0200, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@...a86.com>
> 
> This commit replaces usb_gadget's is_dualspeed field with
> a max_speed field.
> 
> This change is made so that one will be able to check at
> run-time if given gadget supports super speed.

IMHO the logic is inverted here. It should start from the function
drivers. They should say which USB speeds they support, that would go up
to composite layer and composite would call e.g.
usb_gadget_set_speed(gadget, maximum_speed);

that function will set gadget->maximum_speed and call into the UDC
driver which would write maximum_speed into its internal registers, thus
preventing the core from working at speeds unsupported by the gadget
driver.

All of this should happen before we even connect to the host, so first
all UDCs should have their conversion to udc-core finished and start
using udc_start()/udc_stop(). The driver I just sent [1] illustrates
what udc_start()/udc_stop() should do.

[1] http://marc.info/?l=linux-usb&m=131376669022473&w=2

<snip>

> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
> index e1ecdbc..25058b4 100644
> --- a/drivers/usb/gadget/udc-core.c
> +++ b/drivers/usb/gadget/udc-core.c
> @@ -371,14 +371,28 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
>  }
>  static DEVICE_ATTR(soft_connect, S_IWUSR, NULL, usb_udc_softconn_store);
>  
> -static ssize_t usb_udc_speed_show(struct device *dev,
> +#define USB_UDC_SPEED_ATTR(name)					\
> +ssize_t usb_udc_##name##_show(struct device *dev,			\
> +		struct device_attribute *attr, char *buf)		\
> +{									\
> +	struct usb_udc *udc = container_of(dev, struct usb_udc, dev);	\
> +	return snprintf(buf, PAGE_SIZE, "%s\n",				\
> +			usb_device_speed_name(udc->gadget->name));	\
> +}									\
> +static DEVICE_ATTR(name, S_IRUSR, usb_udc_##name##_show, NULL)
> +
> +static USB_UDC_SPEED_ATTR(speed);

how about "current_speed" ?

> +static USB_UDC_SPEED_ATTR(max_speed);

in this case, humans will be reading sysfs, so maximum_speed will look
nicer, IMHO.

> +/* Provide "is_dualspeed" for backward compatibility. */
> +static ssize_t usb_udc_is_dualspeed_show(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> -	struct usb_udc		*udc = container_of(dev, struct usb_udc, dev);
> -	return snprintf(buf, PAGE_SIZE, "%s\n",
> -			usb_device_speed_name(udc->gadget->speed));
> +	struct usb_udc *udc = container_of(dev, struct usb_udc, dev);
> +	return snprintf(buf, PAGE_SIZE, "%d\n",
> +			gadget_is_dualspeed(udc->gadget));
>  }
> -static DEVICE_ATTR(speed, S_IRUSR, usb_udc_speed_show, NULL);
> +static DEVICE_ATTR(is_dualspeed, S_IRUSR, usb_udc_is_dualspeed_show, NULL);

maybe deprecate this one on feature-removal-schedule ??

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (491 bytes)