On Tue, 6 Feb 2024 16:42:07 -0500 Hugo Villeneuve <hugo@xxxxxxxxxxx> wrote: > From: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx> > > Before, sc16is7xx_lines was checked for a free (zero) bit first, and then > later it was set only if UART port registration succeeded. > > Now that sc16is7xx_lines is shared for the I2C and SPI drivers, make sure > it is reserved and modified atomically, and use a new variable to hold the > status of UART port regisration. > > Remove need to check for previous port registration in sc16is7xx_remove(), > because if sc16is7xx_probe() succeeded, we are guaranteed to have > successfully registered both ports. > > Signed-off-by: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx> > --- > drivers/tty/serial/sc16is7xx.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > index 5b53c88b7133..5073ebfc45bf 100644 > --- a/drivers/tty/serial/sc16is7xx.c > +++ b/drivers/tty/serial/sc16is7xx.c > @@ -1468,6 +1468,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > u32 uartclk = 0; > int i, ret; > struct sc16is7xx_port *s; > + bool port_registered[SC16IS7XX_MAX_PORTS]; > > for (i = 0; i < devtype->nr_uart; i++) > if (IS_ERR(regmaps[i])) > @@ -1533,8 +1534,10 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > SC16IS7XX_IOCONTROL_SRESET_BIT); > > for (i = 0; i < devtype->nr_uart; ++i) { > - s->p[i].port.line = find_first_zero_bit(sc16is7xx_lines, > - SC16IS7XX_MAX_DEVS); > + port_registered[i] = false; I just realised that this will not work. All port_registered[] members need to be initialized before entering the loop. I will add this (for V2) before the loop: memset(port_registered, 0, sizeof(port_registered)); Hugo Villeneuve > + > + s->p[i].port.line = find_and_set_bit(sc16is7xx_lines, > + SC16IS7XX_MAX_DEVS); > if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) { > ret = -ERANGE; > goto out_ports; > @@ -1584,7 +1587,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > if (ret) > goto out_ports; > > - set_bit(s->p[i].port.line, sc16is7xx_lines); > + port_registered[i] = true; > > /* Enable EFR */ > sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG, > @@ -1642,9 +1645,11 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > #endif > > out_ports: > - for (i = 0; i < devtype->nr_uart; i++) > - if (test_and_clear_bit(s->p[i].port.line, sc16is7xx_lines)) > + for (i = 0; i < devtype->nr_uart; i++) { > + clear_bit(s->p[i].port.line, sc16is7xx_lines); > + if (port_registered[i]) > uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port); > + } > > kthread_stop(s->kworker_task); > > @@ -1667,8 +1672,8 @@ void sc16is7xx_remove(struct device *dev) > > for (i = 0; i < s->devtype->nr_uart; i++) { > kthread_cancel_delayed_work_sync(&s->p[i].ms_work); > - if (test_and_clear_bit(s->p[i].port.line, sc16is7xx_lines)) > - uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port); > + clear_bit(s->p[i].port.line, sc16is7xx_lines); > + uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port); > sc16is7xx_power(&s->p[i].port, 0); > } > > -- > 2.39.2 > >