linux-kernel - Re: [PATCH v9 1/1] serial: core: Start managing serial controllers to enable runtime PM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 29 Mar 2023 13:14:05 +0300
From:   Tony Lindgren <tony@...mide.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Jiri Slaby <jirislaby@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        Johan Hovold <johan@...nel.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Vignesh Raghavendra <vigneshr@...com>,
        linux-omap@...r.kernel.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org
Subject: Re: [PATCH v9 1/1] serial: core: Start managing serial controllers
 to enable runtime PM

* Greg Kroah-Hartman <gregkh@...uxfoundation.org> [230329 09:19]:
> On Thu, Mar 23, 2023 at 09:10:47AM +0200, Tony Lindgren wrote:
> > -obj-$(CONFIG_SERIAL_CORE) += serial_core.o
> > +obj-$(CONFIG_SERIAL_CORE) += serial_base.o serial_core.o serial_ctrl.o serial_port.o
> 
> Why is this 3 new modules and not just all go into serial_base?  What's
> going to auto-load the other modules you created here?  Feels like this
> should all end up in the same .ko as they all depend on each other,
> right?

OK sure, I'll build them into serial_base. We now have uart_add_one_port()
and uart_remove_one_port() exported in serial_port so that ends up loading
the serial_base related modules.

> > +struct uart_port *serial_base_get_port(struct device *dev)
> > +{
> > +	struct serial_base_device *sbd;
> > +
> > +	if (!dev)
> > +		return NULL;
> > +
> > +	sbd = to_serial_base_device(dev);
> > +
> > +	/* Check in case serial_core_add_one_port() happened to fail */
> > +	if (!sbd->port->state) {
> 
> This is odd, how can it fail and then this function be called after that
> failure?

On uart_add_one_port(), runtime PM resume function in serial_port gets
called before the port registration has completed. Sounds like I need
to recheck this, maybe we can just enable runtime PM for serial_port
after registration has completed.

> > +/*
> > + * Find a registered serial core controller device if one exists. Returns
> > + * the first device matching the ctrl_id. Caller must hold port_mutex.
> > + */
> > +static struct device *serial_core_ctrl_find(struct uart_driver *drv,
> > +					    struct device *phys_dev,
> > +					    int ctrl_id)
> > +{
> > +	struct uart_state *state;
> > +	int i;
> > +
> > +	if (ctrl_id < 0)
> > +		return NULL;
> 
> Why is a negative number special here?

I think this can go, will check.

> > +	dev = serial_base_device_add(port, "port", ctrl_dev);
> 
> magic strings again :)
> 
> Do you really just want two different "types" of devices on this bus,
> controllers and ports?  If so, just do that, don't make the name magic
> here.
> 
> Then you can have:
> 	serial_base_port_add()
> 	serial_base_ctrl_add()
> 
> and one cleanup function will still work.

Yes two different types should do here, I'll take a look.

> Otherwise this looks good to me, thanks for doing all of this work.

OK great thanks,

Tony