Re: [RFC PATCH 02/14] usb: gadget: Add callback to set lane and transfer rate

From: Felipe Balbi <balbi@kernel.org>
To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	linux-usb@vger.kernel.org, Alan Stern <stern@rowland.harvard.edu>,
	Roger Quadros <rogerq@ti.com>, zhengbin <zhengbin13@huawei.com>
Cc: John Youn <John.Youn@synopsys.com>
Subject: Re: [RFC PATCH 02/14] usb: gadget: Add callback to set lane and transfer rate
Date: Thu, 12 Dec 2019 09:58:30 +0200	[thread overview]
Message-ID: <874ky6x9eh.fsf@kernel.org> (raw)
In-Reply-To: <25394a433bb79ddf36d0ec52bc998b164e373bf4.1576118671.git.thinhn@synopsys.com>

[-- Attachment #1: Type: text/plain, Size: 4979 bytes --]

Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> Introduce gadget opts udc_set_sublink_speed callback to set the lane
> count and transfer rate (in lane speed mantissa of Gbps) for SuperSpeed
> Plus capable gadgets. In the same way udc_set_speed, this function can
> control the gadget's sublink attributes for SuperSpeed Plus.
>
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> ---
>  drivers/usb/gadget/composite.c           |  2 ++
>  drivers/usb/gadget/legacy/mass_storage.c |  2 ++

I would rather not add new features to the legacy gadgets and focus on
our configfs interface for anything new. Moreover, using the feature
you introduced could, arguably, be done as a separate patch.

> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 3b4f67000315..a4de5a8c0f19 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -2353,6 +2353,8 @@ int usb_composite_probe(struct usb_composite_driver *driver)
>  	gadget_driver->function =  (char *) driver->name;
>  	gadget_driver->driver.name = driver->name;
>  	gadget_driver->max_speed = driver->max_speed;
> +	gadget_driver->max_lane_count = driver->max_lane_count;
> +	gadget_driver->max_lsm = driver->max_lsm;
>  
>  	return usb_gadget_probe_driver(gadget_driver);
>  }
> diff --git a/drivers/usb/gadget/legacy/mass_storage.c b/drivers/usb/gadget/legacy/mass_storage.c
> index f18f77584fc2..a0912c5afffc 100644
> --- a/drivers/usb/gadget/legacy/mass_storage.c
> +++ b/drivers/usb/gadget/legacy/mass_storage.c
> @@ -223,6 +223,8 @@ static struct usb_composite_driver msg_driver = {
>  	.name		= "g_mass_storage",
>  	.dev		= &msg_device_desc,
>  	.max_speed	= USB_SPEED_SUPER_PLUS,
> +	.max_lane_count	= 2,
> +	.max_lsm	= 10,

Right, as mentioned, I'd prefer not touch the legacy gadgets. But in any
case, why is it so that the gadget is telling you about max lane count
and lsm? That should be abstracted away from the gadget driver. Gadget
driver shouldn't have knowledge of number of lanes because, at the end
of the day, that doesn't really change anything in practice. Unlike HS
vs SS which changes a bunch of things.

>  	.needs_serial	= 1,
>  	.strings	= dev_strings,
>  	.bind		= msg_bind,
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 51fa614b4079..a3b106a22a6e 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -1120,6 +1120,35 @@ static inline void usb_gadget_udc_set_speed(struct usb_udc *udc,
>  	}
>  }
>  
> +/**
> + * usb_gadget_udc_set_sublink_attr - tells usb device controller the sublink
> + *    attributes supported by the current driver
> + * @udc: The device we want to set maximum speed
> + * @lane_count: The maximum number of lanes to connect
> + * @lsm: The maximum lane speed mantissa in Gbps to run
> + *
> + * In the same way as usb_gadget_udc_set_speed(), this function can set the
> + * gadget's sublink attributes for SuperSpeed Plus.
> + *
> + * This call is issued by the UDC Class driver before calling
> + * usb_gadget_udc_start() in order to make sure that we don't try to
> + * connect on speeds the gadget driver doesn't support.
> + */
> +static inline void usb_gadget_udc_set_sublink_attr(struct usb_udc *udc,
> +						   unsigned int lane_count,
> +						   unsigned int lsm)

do we envision a possibility of future USB spec releases adding more
data here? How about introducing a struct usb_sublink_attr to be passed
around? Could be used by both host and gadget stacks.

> +{
> +	if (udc->gadget->ops->udc_set_sublink_attr) {
> +		unsigned int rate;
> +		unsigned int lanes;
> +
> +		rate = min(lsm, udc->gadget->max_lsm);
> +		lanes = min(lane_count, udc->gadget->max_lane_count);

considering that lsm and lane_count are from 0 to their respective max
values, do you need a min() here? Might be better to WARN() when either
in over their max values.

> +		udc->gadget->ops->udc_set_sublink_attr(udc->gadget,
> +						       lanes, rate);

indentation using spaces? (same above, please fix)

> @@ -1353,7 +1382,14 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri
>  	udc->dev.driver = &driver->driver;
>  	udc->gadget->dev.driver = &driver->driver;
>  
> -	usb_gadget_udc_set_speed(udc, driver->max_speed);
> +	if (udc->gadget->ops->udc_set_sublink_attr &&
> +	    udc->gadget->max_speed == USB_SPEED_SUPER_PLUS &&
> +	    driver->max_lsm && driver->max_lane_count &&
> +	    driver->max_speed == USB_SPEED_SUPER_PLUS)

So if driver doesn't provide max_lsm and max_speed you don't set sublink
attr? Won't this cause problems? Also, the sublink_attr is still,
conceptually, setting the max speed for the bus, right? So you may want
to call usb_gadget_udc_set_sublink_attr() from inside
usb_gadget_udc_set_speed(), then we don't need to modify all the callers.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]