Re: [PATCH v6 part1 8/8] usb: block suspension of superspeed port while hispeed peer is active

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 5 Mar 2014, Dan Williams wrote:

> 8<---------
> Subject: usb: block suspension of superspeed port while hispeed peer is active
> 
> From: Dan Williams <dan.j.williams@xxxxxxxxx>
> 
> ClearPortFeature(PORT_POWER) on a usb3 port places the port in either a
> DSPORT.Powered-off-detect / DSPORT.Powered-off-reset loop, or the
> DSPORT.Powered-off state.  There is no way to ensure that RX
> terminations will persist in this state, so it is possible a device will
> degrade to its usb2 connection.  Prevent this by blocking power-off of a
> usb3 port while its usb2 peer is active, and powering on a usb3 port
> before its usb2 peer.
> 
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>

> @@ -129,6 +139,15 @@ static int usb_port_runtime_suspend(struct device *dev)
>  	usb_clear_port_feature(hdev, port1,	USB_PORT_FEAT_C_ENABLE);
>  	clear_bit(port1, hub->busy_bits);
>  	usb_autopm_put_interface(intf);
> +
> +	/*
> +	 * Our peer usb3 port may now be able to suspend, asynchronously

s/,/, so/

> +	 * queue a suspend request to observe that this usb2 peer port

s/peer //

> +	 * is now off.
> +	 */
> +	if (!hub_is_superspeed(hdev) && peer)
> +		pm_runtime_put(&peer->dev);
> +
>  	return retval;
>  }
>  #endif
> @@ -151,8 +170,26 @@ static struct device_driver usb_port_driver = {
>  	.owner = THIS_MODULE,
>  };
>  
> +/*
> + * Modifying ->peer affects usb_port_runtime_{suspend|resume} so make
> + * sure devices are active before the change and re-evaluate
> + * afterwards
> + */
> +static void pre_modify_peers(struct usb_port *left, struct usb_port *right)
> +{
> +	pm_runtime_get_sync(&left->dev);
> +	pm_runtime_get_sync(&right->dev);
> +}
> +
> +static void post_modify_peers(struct usb_port *left, struct usb_port *right)
> +{
> +	pm_runtime_put(&left->dev);
> +	pm_runtime_put(&right->dev);
> +}

You can put the contents of these routines directly inline; no need to
split them out.

> @@ -178,9 +215,26 @@ static int link_peers(struct usb_port *left, struct usb_port *right)
>  		return rc;
>  	}
>  
> +	pre_modify_peers(left, right);
>  	left->peer = right;
>  	right->peer = left;
>  
> +	/*
> +	 * Ports are peer linked, hold a reference on the superspeed
> +	 * port which the hispeed port drops when it suspends.  This
> +	 * ensures that superspeed ports only suspend after their
> +	 * hispeed peer.
> +	 */
> +	ldev = to_usb_device(left->dev.parent->parent);
> +	rdev = to_usb_device(right->dev.parent->parent);

Please call these lhdev and rhdev.

> +	if (hub_is_superspeed(ldev))
> +		pm_runtime_get_noresume(&left->dev);
> +	else {
> +		WARN_ON(!hub_is_superspeed(rdev));
> +		pm_runtime_get_noresume(&right->dev);
> +	}
> +	post_modify_peers(left, right);

In fact, do we need the pre/post_modify actions at all?  Wouldn't it be 
sufficient to do just a pm_runtime_get_sync on the SuperSpeed peer?

> @@ -200,14 +254,32 @@ static void link_peers_report(struct usb_port *left, struct usb_port *right)
>  
>  static void unlink_peers(struct usb_port *left, struct usb_port *right)
>  {
> +	struct usb_device *ldev, *rdev;
> +
>  	WARN(right->peer != left || left->peer != right,
>  			"%s and %s are not peers?\n",
>  			dev_name(&left->dev), dev_name(&right->dev));
>  
> +	pre_modify_peers(left, right);

I don't think this is necessary.  If the peer is already suspended, why 
power it up again?

>  	sysfs_remove_link(&left->dev.kobj, "peer");
>  	right->peer = NULL;
>  	sysfs_remove_link(&right->dev.kobj, "peer");
>  	left->peer = NULL;
> +
> +	/*
> +	 * Ports are no longer peer linked, drop the reference that
> +	 * keeps the superspeed port (may be 'right' or 'left') powered
> +	 * when its peer is active
> +	 */
> +	ldev = to_usb_device(left->dev.parent->parent);
> +	rdev = to_usb_device(right->dev.parent->parent);

Again, lhdev and rhdev.

> +	if (hub_is_superspeed(ldev))
> +		pm_runtime_put_noidle(&left->dev);
> +	else {
> +		WARN_ON(!hub_is_superspeed(rdev));

The WARN_ON above should be sufficient; we don't need this one as well.

> +		pm_runtime_put_noidle(&right->dev);

These calls should become pm_runtime_put().

> +	}
> +	post_modify_peers(left, right);

Not needed.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux