Re: [PATCH v5 10/16] usb: hub: Power on connected M.2 E-key connectors with power sequencing API

On Fri, Jul 17, 2026 at 1:34 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Jul 16, 2026 at 06:02:38PM +0800, Chen-Yu Tsai wrote:
> > On Wed, Jul 15, 2026 at 9:25 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Wed, Jul 15, 2026 at 04:53:40PM +0800, Chen-Yu Tsai wrote:
>
> ...
>
>
> > > >       fwnode_handle_put(dev_fwnode(dev));
> > > > +     /* usb_hub_create_port_device() could leave an error value */
> > > > +     if (!IS_ERR(port_dev->pwrseq))
> > > > +             pwrseq_put(port_dev->pwrseq);
> > >
> > > Hmm... I would rather make pwrseq_put() NULL and error pointer-aware, so
> > > it will be no-op in such cases. But I think Bart has his own opinion about
> > > this.
> > >
> > > >       kfree(port_dev->req);
> > > >       kfree(port_dev);
> > > >  }
> > >
> > > >               put_device(&port_dev->dev);
> > > > +             hub->ports[port1 - 1] = NULL;
> > > >               return retval;
> > > >       }
>
> ...
>
> > > > int usb_hub_create_port_device(struct usb_hub *hub, int port1)
> > >
> > > > +     port_dev->pwrseq = usb_hub_port_pwrseq_get(port_dev);
> > > > +     if (IS_ERR(port_dev->pwrseq)) {
> > > > +             retval = dev_err_probe(&port_dev->dev, PTR_ERR(port_dev->pwrseq),
> > > > +                                    "failed to get power sequencing descriptor\n");
> > > > +             goto err_put_kn;
> > >
> > > OTOH, how pwrseq can be non-NULL and at the same time port be created?
> >
> > I assume by non-NULL you mean returning an error?
> >
> > device_register(&port_dev->dev) is called some lines above this.
> >
> > usb_port is weird in that usb_hub_create_port_device() does a bunch
> > of stuff that is normally done in a driver's probe function. However
> > usb_port's driver is just a stub driver add for userspace compatibility
> > reasons. See commit d99f6b4130877 ("usb: rename usb_port device objects").
> >
> > So at this point the usb_port device has been added to the driver model.
> > Or we can just zero out port_dev->pwrseq instead? I guess that is cleaner
> > than having some weird check in the release function.
>
> So, why do we need to assign the error pointer to the field directly then?
> Make a local variable and assign it if and only if there is no error pointer
> in it. Would it work?

That's the next thing to do on my list. :)


ChenYu