Re: [PATCH v3 06/13] usb: hub: Pass |struct usb_port*| to usb_port_is_power_on()

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Jul 07, 2026 at 12:29:05PM +0800, Chen-Yu Tsai wrote:
> On Tue, Jul 7, 2026 at 12:01 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Jul 06, 2026 at 07:08:07PM +0800, Chen-Yu Tsai wrote:
> > > On Fri, Jul 3, 2026 at 9:33 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, Jul 03, 2026 at 09:17:16PM +0800, Chen-Yu Tsai wrote:
> > > > > On Fri, Jul 3, 2026 at 9:11 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Fri, Jul 03, 2026 at 07:03:07PM +0800, Chen-Yu Tsai wrote:

...

> > > > > > At a brief look this will be the only function that takes usb_port
> > > > > > instead of usb_hub in the entire hub.h (I don't count container_of()
> > > > > > as a function). With that being said I would rather see it to be moved
> > > > > > to port.c altogether (yes, it's more invasive change, but looks more
> > > > > > consistent).  I would even dare to move struct usb_port (and container_of()
> > > > > > accompanied with that) and this function to port.h. This might require
> > > > > > a separate patch, though.
> > > > >
> > > > > I agree with the reasoning, especially given the function name. However
> > > > > I wonder if it would cause problems given the linking order. I'll give
> > > > > it a try nevertheless and report back.
> > > >
> > > > Thanks!
> > > >
> > > > In case it won't fly (but I still think it's better to split), can you at least
> > > > group usb_port APIs and struct? Means moving the proto closer to that struct
> > > > usb_port followed by container_of().
> > >
> > > Looks like it works out fine. port.o is linked in after hub.o, so moving
> > > the function from the latter to the former doesn't cause issues. One might
> > > even say that it is the correct thing to do.
> >
> > That's what I say! :-)
> >
> > > > > > Perhaps something like: 1) "move struct usb_port and associated APIs to port.h";
> > > > > > 2) "...this patch...".
> > >
> > > There are still some left in hub.c:
> > >
> > > static void usb_lock_port(struct usb_port *port_dev)
> > > static void usb_unlock_port(struct usb_port *port_dev)
> 
> These are only used internally in hub.c as a wrapper for the port mutex.
> 
> > > static void port_over_current_notify(struct usb_port *port_dev)
> 
> This is only used by the hub's overcurrent event handler.
> 
> > > static void hub_usb3_port_prepare_disable(struct usb_hub *hub,
> > >                                           struct usb_port *port_dev)
> >
> > This one takes hub as well.
> >
> > > These are used only in hub.c and moving them probably makes things
> > > messier?
> >
> > Perhaps better naming to each (kinda moving to usb hub namespace)?
> >
> > > OOTH, the following bits are in port.c but declared in hub.h:
> > >
> > > int usb_hub_create_port_device(struct usb_hub *hub,
> > >          int port1);
> > > void usb_hub_remove_port_device(struct usb_hub *hub,
> > >          int port1);
> > >
> > > Any preferences for these while we're reorganizing the code?
> >
> > These ones would be good to be in hub.c if there is no circular dependencies or
> > alike.
> 
> Those two, along with all the static functions they call, ends up being
> half of port.c. That doesn't help the size of hub.c, which is already
> over 6000 lines. I think I'll leave this one as is.

For now, perhaps at some point in the future we can reconsider and refactor
it more.

> Both this part and the remaining bits in hub.c I mentioned above are
> outside the scope of this series. We can discuss whether they really
> need to be moved or renamed.

Sure.

-- 
With Best Regards,
Andy Shevchenko