Re: [PATCH v6 09/11] usb: dwc3: gadget: Implement setting of SSP rate

From: Felipe Balbi <balbi@kernel.org>
To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>
Cc: John Youn <John.Youn@synopsys.com>
Subject: Re: [PATCH v6 09/11] usb: dwc3: gadget: Implement setting of SSP rate
Date: Fri, 15 Jan 2021 13:13:21 +0200	[thread overview]
Message-ID: <87v9by1mi6.fsf@kernel.org> (raw)
In-Reply-To: <13fad6ff-f056-f6ab-713b-68794113e03c@synopsys.com>

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

Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> Felipe Balbi wrote:
>> Hi,
>>
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> @@ -2476,6 +2506,17 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
>>>  	spin_unlock_irqrestore(&dwc->lock, flags);
>>>  }
>>>  
>>> +static void dwc3_gadget_set_ssp_rate(struct usb_gadget *g,
>>> +				     enum usb_ssp_rate rate)
>>> +{
>>> +	struct dwc3		*dwc = gadget_to_dwc(g);
>>> +	unsigned long		flags;
>>> +
>>> +	spin_lock_irqsave(&dwc->lock, flags);
>>> +	dwc->gadget_ssp_rate = rate;
>>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>>> +}
>> it would be best to make this return a value. If udc_set_ssp_rate() is
>> called with invalid rate, UDC can notify core.
>>
>
> The core should know what ssp rate the gadget supports via the
> gadget->max_ssp_rate capability field. Any rate beyond that is invalid.
> Is it necessary to have a return value here? This uses the same logic as
> udc_set_speed()

Yeah, I don't know what I had in mind when I made ->udc_set_speed()
void. Then again, we know exactly who's calling it and we can guarantee
that no invalid values will be passed. There's no way for, for example,
userspace (via ffs) to call it with a bogus value.

Perhaps it's okay, but something to keep an eye for both
->udc_set_ssp_rate() and ->udc_set_speed().

Thanks for pointing me back at ->udc_set_speed().

-- 
balbi

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