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

Greg Kroah-Hartman
2025-06-29T06:10:08Z

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@dartmouth.edu>
> Signed-off-by: Kyungtae Kim <Kyungtae.Kim@dartmouth.edu>
> Signed-off-by: Seungjin Bae <eeodqql09@gmail.com>
> ---

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