mbox series

[v1,00/16] serial: max3100: Put into shape

Message ID 20240402154219.3583679-1-andriy.shevchenko@linux.intel.com
Headers show
Series serial: max3100: Put into shape | expand

Message

Andy Shevchenko April 2, 2024, 3:38 p.m. UTC 
Put the driver into the shape with all new bells and whistles
from the kernel.

The first three patches marked as fixes, but there is no hurry (as it
was for ages like this in the kernel) to pipe them to stable. That's
why I sent all in one series and it's good for tty-next.

Tested on Intel Merrifield with MAX3111e connected.

Andy Shevchenko (16):
  serial: max3100: Lock port->lock when calling uart_handle_cts_change()
  serial: max3100: Update uart_driver_registered on driver removal
  serial: max3100: Fix bitwise types
  serial: max3100: Make struct plat_max3100 local
  serial: max3100: Remove custom HW shutdown support
  serial: max3100: Replace custom polling timeout with standard one
  serial: max3100: Enable TIOCM_LOOP
  serial: max3100: Get crystal frequency via device property
  serial: max3100: Remove duplicating irq field
  serial: max3100: Switch to use dev_err_probe()
  serial: max3100: Replace MODULE_ALIAS() with respective ID tables
  serial: max3100: Switch to DEFINE_SIMPLE_DEV_PM_OPS()
  serial: max3100: Extract to_max3100_port() helper macro
  serial: max3100: Remove unneeded forward declaration
  serial: max3100: Sort headers
  serial: max3100: Update Kconfig entry

 drivers/tty/serial/Kconfig     |   7 +-
 drivers/tty/serial/max3100.c   | 320 +++++++++++++--------------------
 include/linux/serial_max3100.h |  48 -----
 3 files changed, 131 insertions(+), 244 deletions(-)
 delete mode 100644 include/linux/serial_max3100.h

Comments

Hugo Villeneuve April 2, 2024, 5:18 p.m. UTC | #1 
On Tue,  2 Apr 2024 18:38:08 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

Hi Andy,

> The removal of the last MAX3100 device triggers the removal of
> the driver. However, code doesn't update the respective global
> variable and after insmod — rmmod — insmod cycle the kernel
> oopses:
> 
>   max3100 spi-PRP0001:01: max3100_probe: adding port 0
>   BUG: kernel NULL pointer dereference, address: 0000000000000408
>   ...
>   RIP: 0010:serial_core_register_port+0xa0/0x840
>   ...
>    max3100_probe+0x1b6/0x280 [max3100]
>    spi_probe+0x8d/0xb0
> 
> Update the actual state so next time UART driver will be registered
> again.
> 
> Fixes: 7831d56b0a35 ("tty: MAX3100")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/serial/max3100.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
> index 45022f2909f0..efe26f6d5ebf 100644
> --- a/drivers/tty/serial/max3100.c
> +++ b/drivers/tty/serial/max3100.c
> @@ -841,6 +841,7 @@ static void max3100_remove(struct spi_device *spi)
>  		}
>  	pr_debug("removing max3100 driver\n");
>  	uart_unregister_driver(&max3100_uart_driver);
> +	uart_driver_registered = 0;

At the beginning of the probe function, we have:

-----------------------
if (!uart_driver_registered) {
		uart_driver_registered = 1;
		retval = uart_register_driver(&max3100_uart_driver);
		if (retval) {
			printk(KERN_ERR "Couldn't register max3100 uart
driver\n"); mutex_unlock(&max3100s_lock);
			return retval;
...
-----------------------

If uart_register_driver() fails, uart_driver_registered would still be
true and would it prevent any other subsequent devices from being
properly registered? If yes, then maybe "uart_driver_registered = 1"
should be set only after a sucessfull call to uart_register_driver()?

Hugo.


>  
>  	mutex_unlock(&max3100s_lock);
>  }
> -- 
> 2.43.0.rc1.1.gbec44491f096
> 
>
Hugo Villeneuve April 2, 2024, 5:37 p.m. UTC | #2 
On Tue, 2 Apr 2024 20:31:50 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Tue, Apr 02, 2024 at 01:18:27PM -0400, Hugo Villeneuve wrote:
> > On Tue,  2 Apr 2024 18:38:08 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> ...
> 
> > >  	pr_debug("removing max3100 driver\n");
> > >  	uart_unregister_driver(&max3100_uart_driver);
> > > +	uart_driver_registered = 0;
> > 
> > At the beginning of the probe function, we have:
> > 
> > -----------------------
> > if (!uart_driver_registered) {
> > 		uart_driver_registered = 1;
> > 		retval = uart_register_driver(&max3100_uart_driver);
> > 		if (retval) {
> > 			printk(KERN_ERR "Couldn't register max3100 uart
> > driver\n"); mutex_unlock(&max3100s_lock);
> > 			return retval;
> > ...
> > -----------------------
> > 
> > If uart_register_driver() fails, uart_driver_registered would still be
> > true and would it prevent any other subsequent devices from being
> > properly registered? If yes, then maybe "uart_driver_registered = 1"
> > should be set only after a sucessfull call to uart_register_driver()?
> 
> Looks like yet another issue here (however I haven't hit it so far).
> I guess I can combine both fixes.  What do you think?

Hi Andy,
makes sense to me.

Hugo.


> 
> -- 
> With Best Regards,
> Andy Shevchenko