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

[Andy Shevchenko]

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
> <andriy.shevchenko@xxxxxxxxxxxxxxx> 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?

-- 
With Best Regards,
Andy Shevchenko