| 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
| ||
|
Date: Mon, 23 May 2011 10:05:58 +0300
From: Felipe Balbi <balbi@...com>
To: Tatyana Brokhman <tlinder@...eaurora.org>
Cc: greg@...ah.com, linux-usb@...r.kernel.org,
linux-arm-msm@...r.kernel.org, balbi@...com, ablay@...eaurora.org,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v12 7/8] usb: Adding SuperSpeed support to dummy_hcd
Hi,
On Mon, May 23, 2011 at 09:41:16AM +0300, Tatyana Brokhman wrote:
> This patch adds SS support to the dummy hcd module. It may be used to test
> SS device when no (SS) HW is available.
> USB 3.0 hub includes 2 hubs - HS and SS ones.
> This patch adds support for a SS hub in the dummy_hcd module. Thus, when
> dummy_hcd enabled it will register 2 root hubs (SS and HS).
> A new module parameter was added to simulate a SuperSpeed connection.
> When a new device is connected it will enumerate via the HS hub by default.
> In order to simulate SuperSpeed connection set the is_super_speed module
> parameter to true.
>
> Signed-off-by: Tatyana Brokhman <tlinder@...eaurora.org>
>
> ---
> drivers/usb/gadget/dummy_hcd.c | 540 +++++++++++++++++++++++++++++++++++-----
> 1 files changed, 483 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
> index bf7981d..c2731d3 100644
> --- a/drivers/usb/gadget/dummy_hcd.c
> +++ b/drivers/usb/gadget/dummy_hcd.c
> @@ -70,6 +70,15 @@ MODULE_DESCRIPTION (DRIVER_DESC);
> MODULE_AUTHOR ("David Brownell");
> MODULE_LICENSE ("GPL");
>
> +struct dummy_hcd_module_parameters {
> + bool is_super_speed;
> +};
> +
> +static struct dummy_hcd_module_parameters mod_data = {
> + .is_super_speed = false
> +};
> +module_param_named(is_super_speed, mod_data.is_super_speed, bool, S_IRUGO);
> +MODULE_PARM_DESC(is_super_speed, "true to simulate SuperSpeed connection");
you shouldn't need this. You should always enable SuperSpeed for this
driver.
> /*-------------------------------------------------------------------------*/
>
> /* gadget side driver data structres */
> @@ -188,6 +197,7 @@ struct dummy {
> * MASTER/HOST side support
> */
> struct dummy_hcd *hs_hcd;
> + struct dummy_hcd *ss_hcd;
> };
>
> static inline struct dummy_hcd *hcd_to_dummy_hcd(struct usb_hcd *hcd)
> @@ -218,7 +228,10 @@ static inline struct dummy *ep_to_dummy (struct dummy_ep *ep)
> static inline struct dummy_hcd *gadget_to_dummy_hcd(struct usb_gadget *gadget)
> {
> struct dummy *dum = container_of(gadget, struct dummy, gadget);
> - return dum->hs_hcd;
> + if (dum->gadget.speed == USB_SPEED_SUPER)
> + return dum->ss_hcd;
> + else
> + return dum->hs_hcd;
> }
>
> static inline struct dummy *gadget_dev_to_dummy (struct device *dev)
> @@ -266,10 +279,88 @@ stop_activity (struct dummy *dum)
> /* driver now does any non-usb quiescing necessary */
> }
>
> +/**
> + * set_ss_link_state() - Sets the current state of the SuperSpeed link
> + * @dum_hcd: pointer to the dummy_hcd structure to update the link
> + * state for
> + *
> + * This function updates the port_status according to the link
> + * state. The old status is saved befor updating.
> + * Note: this function should be called only for SuperSpeed
> + * master and the caller must hold the lock.
> + */
> +static void
> +set_ss_link_state(struct dummy_hcd *dum_hcd)
a significant part of this function is similar to the USB2.0 version
set_link_state(), so it's better to phase out the similar parts to an
internal function and use that on both set_*_link_state(). Add something
like:
__set_link_state_by_speed(struct dummy_hcd *dum_hcd, enum
usb_device_speed speed);
then do all the similar parts there copying for speed, then just call
that function on both set_ss_link_state() and set_link_state().
> +{
> + struct dummy *dum = dum_hcd->dum;
add a blank line here
> + if (dum->gadget.speed < USB_SPEED_SUPER)
this assumes USB_SPEED_SUPER has an assigned value bigger than the
others which might not be true if someone changes the order of the
enumeration. So it's better to use:
if (dum->gadget.speed != USB_SPEED_SUPER)
> + return;
> + dum_hcd->active = 0;
> + if ((dum_hcd->port_status & USB_SS_PORT_STAT_POWER) == 0)
> + dum_hcd->port_status = 0;
if one branch has {} all of them should have :-) You can fix it while
re-factoring this code.
> + /* UDC suspend must cause a disconnect */
> + else if (!dum->pullup || dum->udc_suspended) {
> + dum_hcd->port_status &= ~(USB_PORT_STAT_CONNECTION |
> + USB_PORT_STAT_ENABLE);
> + if ((dum_hcd->old_status & USB_PORT_STAT_CONNECTION) != 0)
> + dum_hcd->port_status |=
> + (USB_PORT_STAT_C_CONNECTION << 16);
> + } else {
> + /* device is connected and not suspended */
> + dum_hcd->port_status |= (USB_PORT_STAT_CONNECTION |
> + USB_PORT_STAT_SPEED_5GBPS) ;
> + if ((dum_hcd->old_status & USB_PORT_STAT_CONNECTION) == 0)
> + dum_hcd->port_status |=
> + (USB_PORT_STAT_C_CONNECTION << 16);
> + if ((dum_hcd->port_status & USB_PORT_STAT_ENABLE) == 1 &&
> + (dum_hcd->port_status & USB_SS_PORT_LS_U0) == 1 &&
> + dum_hcd->rh_state != DUMMY_RH_SUSPENDED)
> + dum_hcd->active = 1;
> + }
> +
> +
only one blank line :-)
> + if ((dum_hcd->port_status & USB_PORT_STAT_ENABLE) == 0 ||
> + dum_hcd->active)
> + dum_hcd->resuming = 0;
> +
> + /* if !connected or reset */
> + if ((dum_hcd->port_status & USB_PORT_STAT_CONNECTION) == 0 ||
> + (dum_hcd->port_status & USB_PORT_STAT_RESET) != 0) {
> + /*
> + * We're connected and not reseted (reset occured now),
'reseted' isn't proper spelling :-)
> + * and driver attached - disconnect!
> + */
> + if ((dum_hcd->old_status & USB_PORT_STAT_CONNECTION) != 0 &&
> + (dum_hcd->old_status & USB_PORT_STAT_RESET) == 0 &&
> + dum->driver) {
> + stop_activity(dum);
> + spin_unlock(&dum->lock);
> + dum->driver->disconnect(&dum->gadget);
> + spin_lock(&dum->lock);
> + }
> + } else if (dum_hcd->active != dum_hcd->old_active) {
> + if (dum_hcd->old_active && dum->driver->suspend) {
> + spin_unlock(&dum->lock);
> + dum->driver->suspend(&dum->gadget);
> + spin_lock(&dum->lock);
> + } else if (!dum_hcd->old_active && dum->driver->resume) {
> + spin_unlock(&dum->lock);
> + dum->driver->resume(&dum->gadget);
> + spin_lock(&dum->lock);
> + }
> + }
> +
> + dum_hcd->old_status = dum_hcd->port_status;
> + dum_hcd->old_active = dum_hcd->active;
> +}
> +
> /* caller must hold lock */
> static void set_link_state(struct dummy_hcd *dum_hcd)
> {
> struct dummy *dum = dum_hcd->dum;
add a blank line here.
> @@ -1371,6 +1577,10 @@ static void dummy_timer(unsigned long _dum_hcd)
> case USB_SPEED_HIGH:
> total = 512/*bytes*/ * 13/*packets*/ * 8/*uframes*/;
> break;
> + case USB_SPEED_SUPER:
> + /* Bus speed is 500000 bytes/ms, so use a little less */
isn't it 500000 bits/ms ?
--
balbi
Download attachment "signature.asc" of type "application/pgp-signature" (491 bytes)
Powered by blists - more mailing lists