Re: [PATCH] usb: gadget: m66592-udc: Ignore feature requests for EP0

[Greg Kroah-Hartman]

On Sat, Jun 28, 2025 at 10:59:53PM -0400, Seungjin Bae wrote:
> The `set_feature() and clear_feature() functions handle requests to set or clear the ENDPOINT_HALT feature.
> Currently, these requests are processed for any endpoint, including the control endpoint (EP0).
> 
> The ENDPOINT_HALT feature is not defined for control endpoints according to the USB specification 9.4.5.

Nit, shouldn't you wrap these lines at 72 columns?  Didn't checkpatch
complain about this?

> 
> Fixes: 4cf2503c6801a ("USB: m66592-udc: peripheral controller driver for M66592")
> Co-developed-by: Kyungtae Kim <Kyungtae.Kim@xxxxxxxxxxxxx>
> Signed-off-by: Kyungtae Kim <Kyungtae.Kim@xxxxxxxxxxxxx>
> Signed-off-by: Seungjin Bae <eeodqql09@xxxxxxxxx>
> ---

Any reason why you didn't also cc: stable for this?

>  drivers/usb/gadget/udc/m66592-udc.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/m66592-udc.c b/drivers/usb/gadget/udc/m66592-udc.c
> index 715791737499..38cc11ae80b6 100644
> --- a/drivers/usb/gadget/udc/m66592-udc.c
> +++ b/drivers/usb/gadget/udc/m66592-udc.c
> @@ -1010,8 +1010,13 @@ static void clear_feature(struct m66592 *m66592, struct usb_ctrlrequest *ctrl)
>  		struct m66592_ep *ep;
>  		struct m66592_request *req;
>  		u16 w_index = le16_to_cpu(ctrl->wIndex);
> +		u16 ep_num = w_index & USB_ENDPOINT_NUMBER_MASK;
>  
> -		ep = m66592->epaddr2ep[w_index & USB_ENDPOINT_NUMBER_MASK];
> +		if (ep_num == 0) {
> +			control_end(m66592, 1);
> +			break;
> +		}
> +		ep = m66592->epaddr2ep[ep_num];

What in-kernel user calls this function for endpoint 0?


>  		pipe_stop(m66592, ep->pipenum);
>  		control_reg_sqclr(m66592, ep->pipenum);
>  
> @@ -1067,8 +1072,13 @@ static void set_feature(struct m66592 *m66592, struct usb_ctrlrequest *ctrl)
>  	case USB_RECIP_ENDPOINT: {
>  		struct m66592_ep *ep;
>  		u16 w_index = le16_to_cpu(ctrl->wIndex);
> +		u16 ep_num = w_index & USB_ENDPOINT_NUMBER_MASK;
>  
> -		ep = m66592->epaddr2ep[w_index & USB_ENDPOINT_NUMBER_MASK];
> +		if (ep_num == 0) {
> +			control_end(m66592, 1);
> +			break;
> +		}
> +		ep = m66592->epaddr2ep[ep_num];

Same here, what in-kernel user calls this for endpoint 0?

And was this tested on real hardware?

thanks,

greg k-h