Re: [PATCH v2] usb: hub: Set proper message when usb_hub_create_port_device() fails

[Chen-Yu Tsai]

On Tue, Sep 22, 2026 at 10:21 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Sep 22, 2026 at 04:05:41PM +0800, Chen-Yu Tsai wrote:
> > Right now when usb_hub_create_port_device() fails, it prints a separate
> > error message to say which port failed, but otherwise leaves 'message'
> > set to the default "out of memory", which is somewhat misleading.
> >
> > Allocate some memory to put the custom formatted error message in and
> > use it as the error message. If the allocation fails, use a generic
> > version of the error message. The allocation is explicitly freed after
> > the error message is printed.
> >
> > Variants of __free() were not used as it ends up mixing usage of __free()
> > and goto. Also, kfree_const() won't work as it uses is_kernel_rodata(),
> > which only returns true if the symbol or address is in the kernel image's
> > .rodata section; it doesn't work for module .rodata. And the USB
> > subsystem can be built as a module.
> >
> > Assisted-by: LLM # local reviews
> > Signed-off-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>
> > ---
> > Changes since v1:
> > - Explicitly track and free the allocated custom error message instead
> >   of using devm_kasprintf()
> > - Link to v1:
> >   https://lore.kernel.org/all/20260728100005.413868-1-wenst@xxxxxxxxxxxx/
> >
> > Sorry Andy, I ended up not using your __free(kfree_const) patch. My LLM
> > was telling me that it won't work correctly if CONFIG_USB=m.
> > ---
> >  drivers/usb/core/hub.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 0e929a4c9fa1..398ae53cd8d7 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -1480,6 +1480,8 @@ static int hub_configure(struct usb_hub *hub,
> >       unsigned int pipe;
> >       int maxp, ret, i;
> >       char *message = "out of memory";
> > +     /* Used to hold and clean up kasprintf()-ed failure message */
>
> Comment is unnecessary.  People will see what the variable is used for
> by reading the code; it's not complicated.

OK.

> > +     char *msg_alloc = NULL;
> >       unsigned unit_load;
> >       unsigned full_load;
> >       unsigned maxchild;
> > @@ -1756,8 +1758,11 @@ static int hub_configure(struct usb_hub *hub,
> >       for (i = 0; i < maxchild; i++) {
> >               ret = usb_hub_create_port_device(hub, i + 1);
> >               if (ret < 0) {
> > -                     dev_err(hub->intfdev,
> > -                             "couldn't create port%d device.\n", i + 1);
> > +                     msg_alloc = kasprintf(GFP_KERNEL, "couldn't create port%d device", i + 1);
>
> Why not use a fixed-size 64-byte buffer on the stack instead of
> allocating extra memory?

I was avoiding bloating the stack usage. That said, 64 bytes isn't too
much, and it could even go down to 32.

I'll put it on the stack. Makes things simpler.


Thanks
ChenYu

> Alan Stern
>
> > +                     if (msg_alloc)
> > +                             message = msg_alloc;
> > +                     else
> > +                             message = "couldn't create port device";
> >                       break;
> >               }
> >       }
> > @@ -1792,6 +1797,7 @@ static int hub_configure(struct usb_hub *hub,
> >  fail:
> >       dev_err(hub_dev, "config failed, %s (err %d)\n",
> >                       message, ret);
> > +     kfree(msg_alloc);
> >       /* hub_disconnect() frees urb and descriptor */
> >       return ret;
> >  }
> > --
> > 2.55.0.1082.g2b9226bbc0-goog
> >
> >