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.

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>
---
 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];
 		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];
 		pipe_stall(m66592, ep->pipenum);
 
 		control_end(m66592, 1);
-- 
2.43.0

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.

Actually that's not correct.  What the spec says is:

	It is neither required nor recommended that the Halt feature be 
	implemented for the Default Control Pipe. However, devices may 
	set the Halt feature of the Default Control Pipe in order to 
	reflect a functional error condition. If the feature is set to 
	one, the device will return STALL in the Data and Status stages 
	of each standard request to the pipe except GetStatus(), 
	SetFeature(), and ClearFeature() requests. The device need not 
	return STALL for class-specific and vendor-specific requests.

Thus, it is valid for devices to support the ENDPOINT_HALT feature for 
control endpoints.

Alan Stern

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