On 11/28/25 9:42 AM, Mattijs Korpershoek wrote:
On Thu, Nov 27, 2025 at 14:44, Petr Benes <[email protected]> wrote:
On 11/27/25 11:12 AM, Mattijs Korpershoek wrote:
Hi Michal,
Thank you for the patch.
On Tue, Nov 25, 2025 at 09:58, Michal Vokáč <[email protected]> wrote:
From: Petr Beneš <[email protected]>
There are two places where ci_ep->desc could be accessed despite it is
not valid at that moment. Either the endpoint has not been enabled yet
or it has been disabled meanwhile (The ethernet gadged behaves this way
at least.). That results in dereferencing a null pointer.
I wonder if this is not a generic problem (that ep->desc) is reused.
Looking at usb_ep_enable() in include/linux/usb/gadget.h:
"""
static inline int usb_ep_enable(struct usb_ep *ep,
const struct usb_endpoint_descriptor *desc)
{
int ret;
if (ep->enabled)
return 0;
ret = ep->ops->enable(ep, desc);
if (ret)
return ret;
"""
Can you please explain why the generic code is not enough to handle
this?
Can't speak for for generic gadget stuff without preparation. In this
case it is solely ci_udc who does the mess.
It works with its own
struct ci_ep {
struct usb_ep ep;
struct list_head queue;
bool req_primed;
const struct usb_endpoint_descriptor *desc;
};
Which are held in (struct ci_drv).ep[] and the problematic code is
ci_udc specific.
Ok.
I have compared the changes with the linux driver
(drivers/usb/chipidea/udc.c) and see a couple of differences in error
handling
For example, in linux, in ep_disable(), we return -EBUSY instead of 0.
0 seems to tell the caller that we sucesfully disabled an ep but we did
not do anything since it was already disabled.
Fair.>
Can we do the same error handling as in the linux driver ?
Sure. -EBUSY seems to used mainly by *_register_driver(), I'll test it
and return with the updated patch.>
Moreover, the patch gets rid of possible outstanding requests if the
endpoint's state changes to disabled.
Signed-off-by: Petr Beneš <[email protected]>
Signed-off-by: Michal Vokáč <[email protected]>
---
drivers/usb/gadget/ci_udc.c | 43 +++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index 4bff75da759d..b3bbbb6ad32c 100644
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -308,6 +308,27 @@ static void ci_ep_free_request(struct usb_ep *ep, struct
usb_request *req)
free(ci_req);
}
+static void request_complete(struct usb_ep *ep, struct ci_req *req, int status)
+{
+ if (req->req.status == -EINPROGRESS)
+ req->req.status = status;
+
+ DBG("%s: req %p complete: status %d, actual %u\n",
+ ep->name, req, req->req.status, req->req.actual);
+
+ req->req.complete(ep, &req->req);
+}
+
+static void request_complete_list(struct usb_ep *ep, struct list_head *list,
int status)
+{
+ struct ci_req *req, *tmp_req;
+
+ list_for_each_entry_safe(req, tmp_req, list, queue) {
+ list_del_init(&req->queue);
+ request_complete(ep, req, status);
+ }
+}
+
static void ep_enable(int num, int in, int maxpacket)
{
struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
@@ -335,6 +356,12 @@ static int ci_ep_enable(struct usb_ep *ep,
int num, in;
num = desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
in = (desc->bEndpointAddress & USB_DIR_IN) != 0;
+
+ if (ci_ep->desc) {
+ DBG("%s: endpoint num %d in %d already enabled\n", __func__, num,
in);
+ return 0;
+ }
+
ci_ep->desc = desc;
ep->desc = desc;
@@ -385,16 +412,27 @@ static int ep_disable(int num, int in)
static int ci_ep_disable(struct usb_ep *ep)
{
struct ci_ep *ci_ep = container_of(ep, struct ci_ep, ep);
+ LIST_HEAD(req_list);
int num, in, err;
+ if (!ci_ep->desc) {
+ DBG("%s: attempt to disable a not enabled yet endpoint\n",
__func__);
+ goto nodesc;
+ }
+
num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
in = (ci_ep->desc->bEndpointAddress & USB_DIR_IN) != 0;
+ list_splice_init(&ci_ep->queue, &req_list);
+ request_complete_list(ep, &req_list, -ESHUTDOWN);
+
err = ep_disable(num, in);
if (err)
return err;
ci_ep->desc = NULL;
+
+nodesc:
ep->desc = NULL;
ci_ep->req_primed = false;
return 0;
@@ -606,6 +644,11 @@ static int ci_ep_queue(struct usb_ep *ep,
int in, ret;
int __maybe_unused num;
+ if (!ci_ep->desc) {
+ DBG("%s: ci_ep->desc == NULL, nothing to do!\n", __func__);
+ return -EINVAL;
+ }
+
num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
in = (ci_ep->desc->bEndpointAddress & USB_DIR_IN) != 0;
--
2.43.0
________________________________
CONFIDENTIALITY NOTICE
This message is for the named person's use only. It may contain confidential,
proprietary or legally privileged information.
If you receive this message in error, please immediately delete it and all
copies of it from your system, destroy any hard copies of it and notify us by
email to [email protected] with a copy of this message. You must not, directly or
indirectly, use, disclose, distribute, print or copy any part of this message
if you are not the intended recipient. Y Soft and any of its subsidiaries each
reserves the right to monitor all e-mail communications through its networks.
Y Soft is neither liable for the proper, complete transmission of the
information contained in this communication nor any delay in its receipt. This
email was scanned for the presence of computer viruses. In the unfortunate
event of infection Y Soft does not accept liability.
Any views expressed in this message are those of the individual sender, except
where the message states otherwise and the sender is authorised to state them.
