Re: [PATCH 2/2] serial: Make uart_remove_one_port() return void

[Richard Genoud <richard.genoud@xxxxxxxxx>]

Le 15/05/2023 à 11:57, Claudiu.Beznea@xxxxxxxxxxxxx a écrit :
> On 12.05.2023 20:38, Uwe Kleine-König wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > The return value is only ever used as a return value for remove callbacks
> > of platform drivers. This return value is ignored by the driver core.
> > (The only effect is an error message, but uart_remove_one_port() already
> > emitted one in this case.)
> >
> > So the return value isn't used at all and uart_remove_one_port() can be
> > changed to return void without any loss. Also this better matches the
> > Linux device model as remove functions are not supposed to fail.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
>
> Reviewed-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx> # atmel_serial.c
Acked-by: Richard Genodu <richard.genoud@xxxxxxxxx> # atmel_serial.c

> > ---
> >   drivers/tty/serial/atmel_serial.c           |  5 ++---
> >   drivers/tty/serial/clps711x.c               |  4 +++-
> >   drivers/tty/serial/cpm_uart/cpm_uart_core.c |  5 ++++-
> >   drivers/tty/serial/imx.c                    |  4 +++-
> >   drivers/tty/serial/lantiq.c                 |  4 +++-
> >   drivers/tty/serial/serial_core.c            |  6 +-----
> >   drivers/tty/serial/st-asc.c                 |  4 +++-
> >   drivers/tty/serial/uartlite.c               | 12 ++++--------
> >   drivers/tty/serial/xilinx_uartps.c          |  5 ++---
> >   include/linux/serial_core.h                 |  2 +-
> >   10 files changed, 26 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> > index 9cd7479b03c0..6e9192f122aa 100644
> > --- a/drivers/tty/serial/atmel_serial.c
> > +++ b/drivers/tty/serial/atmel_serial.c
> > @@ -3006,14 +3006,13 @@ static int atmel_serial_remove(struct platform_device *pdev)
> >   {
> >          struct uart_port *port = platform_get_drvdata(pdev);
> >          struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> > -       int ret = 0;
> >
> >          tasklet_kill(&atmel_port->tasklet_rx);
> >          tasklet_kill(&atmel_port->tasklet_tx);
> >
> >          device_init_wakeup(&pdev->dev, 0);
> >
> > -       ret = uart_remove_one_port(&atmel_uart, port);
> > +       uart_remove_one_port(&atmel_uart, port);
> >
> >          kfree(atmel_port->rx_ring.buf);
> >
> > @@ -3023,7 +3022,7 @@ static int atmel_serial_remove(struct platform_device *pdev)
> >
> >          pdev->dev.of_node = NULL;
> >
> > -       return ret;
> > +       return 0;
> >   }
> >
> >   static SIMPLE_DEV_PM_OPS(atmel_serial_pm_ops, atmel_serial_suspend,
> > diff --git a/drivers/tty/serial/clps711x.c b/drivers/tty/serial/clps711x.c
> > index e190dce58f46..e49bc4019b50 100644
> > --- a/drivers/tty/serial/clps711x.c
> > +++ b/drivers/tty/serial/clps711x.c
> > @@ -514,7 +514,9 @@ static int uart_clps711x_remove(struct platform_device *pdev)
> >   {
> >          struct clps711x_port *s = platform_get_drvdata(pdev);
> >
> > -       return uart_remove_one_port(&clps711x_uart, &s->port);
> > +       uart_remove_one_port(&clps711x_uart, &s->port);
> > +
> > +       return 0;
> >   }
> >
> >   static const struct of_device_id __maybe_unused clps711x_uart_dt_ids[] = {
> > diff --git a/drivers/tty/serial/cpm_uart/cpm_uart_core.c b/drivers/tty/serial/cpm_uart/cpm_uart_core.c
> > index 349e7da643f0..66afa9bea6bf 100644
> > --- a/drivers/tty/serial/cpm_uart/cpm_uart_core.c
> > +++ b/drivers/tty/serial/cpm_uart/cpm_uart_core.c
> > @@ -1431,7 +1431,10 @@ static int cpm_uart_probe(struct platform_device *ofdev)
> >   static int cpm_uart_remove(struct platform_device *ofdev)
> >   {
> >          struct uart_cpm_port *pinfo = platform_get_drvdata(ofdev);
> > -       return uart_remove_one_port(&cpm_reg, &pinfo->port);
> > +
> > +       uart_remove_one_port(&cpm_reg, &pinfo->port);
> > +
> > +       return 0;
> >   }
> >
> >   static const struct of_device_id cpm_uart_match[] = {
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index c5e17569c3ad..b2bf3cb449f4 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -2467,7 +2467,9 @@ static int imx_uart_remove(struct platform_device *pdev)
> >   {
> >          struct imx_port *sport = platform_get_drvdata(pdev);
> >
> > -       return uart_remove_one_port(&imx_uart_uart_driver, &sport->port);
> > +       uart_remove_one_port(&imx_uart_uart_driver, &sport->port);
> > +
> > +       return 0;
> >   }
> >
> >   static void imx_uart_restore_context(struct imx_port *sport)
> > diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
> > index a58e9277dfad..d413f97f7190 100644
> > --- a/drivers/tty/serial/lantiq.c
> > +++ b/drivers/tty/serial/lantiq.c
> > @@ -889,7 +889,9 @@ static int lqasc_remove(struct platform_device *pdev)
> >   {
> >          struct uart_port *port = platform_get_drvdata(pdev);
> >
> > -       return uart_remove_one_port(&lqasc_reg, port);
> > +       uart_remove_one_port(&lqasc_reg, port);
> > +
> > +       return 0;
> >   }
> >
> >   static const struct ltq_soc_data soc_data_lantiq = {
> > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > index 54e82f476a2c..4b98d13555c0 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -3154,13 +3154,12 @@ EXPORT_SYMBOL(uart_add_one_port);
> >    * This unhooks (and hangs up) the specified port structure from the core
> >    * driver. No further calls will be made to the low-level code for this port.
> >    */
> > -int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> > +void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> >   {
> >          struct uart_state *state = drv->state + uport->line;
> >          struct tty_port *port = &state->port;
> >          struct uart_port *uart_port;
> >          struct tty_struct *tty;
> > -       int ret = 0;
> >
> >          mutex_lock(&port_mutex);
> >
> > @@ -3176,7 +3175,6 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> >
> >          if (!uart_port) {
> >                  mutex_unlock(&port->mutex);
> > -               ret = -EINVAL;
> >                  goto out;
> >          }
> >          uport->flags |= UPF_DEAD;
> > @@ -3219,8 +3217,6 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> >          mutex_unlock(&port->mutex);
> >   out:
> >          mutex_unlock(&port_mutex);
> > -
> > -       return ret;
> >   }
> >   EXPORT_SYMBOL(uart_remove_one_port);
> >
> > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> > index 5215e6910f68..dc2f2051435c 100644
> > --- a/drivers/tty/serial/st-asc.c
> > +++ b/drivers/tty/serial/st-asc.c
> > @@ -796,7 +796,9 @@ static int asc_serial_remove(struct platform_device *pdev)
> >   {
> >          struct uart_port *port = platform_get_drvdata(pdev);
> >
> > -       return uart_remove_one_port(&asc_uart_driver, port);
> > +       uart_remove_one_port(&asc_uart_driver, port);
> > +
> > +       return 0;
> >   }
> >
> >   #ifdef CONFIG_PM_SLEEP
> > diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> > index 94584e54ebbe..679574893ebe 100644
> > --- a/drivers/tty/serial/uartlite.c
> > +++ b/drivers/tty/serial/uartlite.c
> > @@ -685,18 +685,15 @@ static int ulite_assign(struct device *dev, int id, phys_addr_t base, int irq,
> >    *
> >    * @dev: pointer to device structure
> >    */
> > -static int ulite_release(struct device *dev)
> > +static void ulite_release(struct device *dev)
> >   {
> >          struct uart_port *port = dev_get_drvdata(dev);
> > -       int rc = 0;
> >
> >          if (port) {
> > -               rc = uart_remove_one_port(&ulite_uart_driver, port);
> > +               uart_remove_one_port(&ulite_uart_driver, port);
> >                  dev_set_drvdata(dev, NULL);
> >                  port->mapbase = 0;
> >          }
> > -
> > -       return rc;
> >   }
> >
> >   /**
> > @@ -900,14 +897,13 @@ static int ulite_remove(struct platform_device *pdev)
> >   {
> >          struct uart_port *port = dev_get_drvdata(&pdev->dev);
> >          struct uartlite_data *pdata = port->private_data;
> > -       int rc;
> >
> >          clk_disable_unprepare(pdata->clk);
> > -       rc = ulite_release(&pdev->dev);
> > +       ulite_release(&pdev->dev);
> >          pm_runtime_disable(&pdev->dev);
> >          pm_runtime_set_suspended(&pdev->dev);
> >          pm_runtime_dont_use_autosuspend(&pdev->dev);
> > -       return rc;
> > +       return 0;
> >   }
> >
> >   /* work with hotplug and coldplug */
> > diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> > index 8e521c69a959..20a751663ef9 100644
> > --- a/drivers/tty/serial/xilinx_uartps.c
> > +++ b/drivers/tty/serial/xilinx_uartps.c
> > @@ -1670,14 +1670,13 @@ static int cdns_uart_remove(struct platform_device *pdev)
> >   {
> >          struct uart_port *port = platform_get_drvdata(pdev);
> >          struct cdns_uart *cdns_uart_data = port->private_data;
> > -       int rc;
> >
> >          /* Remove the cdns_uart port from the serial core */
> >   #ifdef CONFIG_COMMON_CLK
> >          clk_notifier_unregister(cdns_uart_data->uartclk,
> >                          &cdns_uart_data->clk_rate_change_nb);
> >   #endif
> > -       rc = uart_remove_one_port(cdns_uart_data->cdns_uart_driver, port);
> > +       uart_remove_one_port(cdns_uart_data->cdns_uart_driver, port);
> >          port->mapbase = 0;
> >          clk_disable_unprepare(cdns_uart_data->uartclk);
> >          clk_disable_unprepare(cdns_uart_data->pclk);
> > @@ -1693,7 +1692,7 @@ static int cdns_uart_remove(struct platform_device *pdev)
> >
> >          if (!--instances)
> >                  uart_unregister_driver(cdns_uart_data->cdns_uart_driver);
> > -       return rc;
> > +       return 0;
> >   }
> >
> >   static struct platform_driver cdns_uart_platform_driver = {
> > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> > index 66ecec15a1bf..ddcdb5b8523e 100644
> > --- a/include/linux/serial_core.h
> > +++ b/include/linux/serial_core.h
> > @@ -853,7 +853,7 @@ void uart_console_write(struct uart_port *port, const char *s,
> >   int uart_register_driver(struct uart_driver *uart);
> >   void uart_unregister_driver(struct uart_driver *uart);
> >   int uart_add_one_port(struct uart_driver *reg, struct uart_port *port);
> > -int uart_remove_one_port(struct uart_driver *reg, struct uart_port *port);
> > +void uart_remove_one_port(struct uart_driver *reg, struct uart_port *port);
> >   bool uart_match_port(const struct uart_port *port1,
> >                  const struct uart_port *port2);
> >
> > --
> > 2.39.2

Thanks !