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? > > 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
