| Message ID | 20210706061909.17555-1-m.shams@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] serial: samsung: Checks the return value of function | expand |
On 06. 07. 21, 8:19, Tamseel Shams wrote: > "uart_add_one_port" function call may fail and return > some error code, so adding a check for return value. > If it is returning some error code, then displaying the > result, unregistering the driver and then returning from > probe function with error code. > > Signed-off-by: Tamseel Shams <m.shams@samsung.com> > --- > Changes since v1: > 1. Added support to unregister driver on failure of "uart_add_one_port" > function call. > 2. Commit message updated. > > Changes since v2: > 1. Added support to unwind clocks on failure of "uart_add_one_port" > function call. > > drivers/tty/serial/samsung_tty.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c > index 9fbc61151c2e..a3f3a17fb54b 100644 > --- a/drivers/tty/serial/samsung_tty.c > +++ b/drivers/tty/serial/samsung_tty.c > @@ -2253,7 +2253,11 @@ static int s3c24xx_serial_probe(struct platform_device *pdev) > } > > dev_dbg(&pdev->dev, "%s: adding port\n", __func__); > - uart_add_one_port(&s3c24xx_uart_drv, &ourport->port); > + ret = uart_add_one_port(&s3c24xx_uart_drv, &ourport->port); > + if (ret < 0) { > + dev_err(&pdev->dev, "Failed to add uart port, err %d\n", ret); > + goto add_port_error; > + } > platform_set_drvdata(pdev, &ourport->port); > > /* > @@ -2272,6 +2276,17 @@ static int s3c24xx_serial_probe(struct platform_device *pdev) > probe_index++; > > return 0; > + > +add_port_error: > + ourport->port.mapbase = 0; > + clk_disable_unprepare(ourport->clk); > + clk_put(ourport->clk); > + if (!IS_ERR(ourport->baudclk)) { > + clk_disable_unprepare(ourport->baudclk); > + clk_put(ourport->baudclk); > + } LGTM, now I only wonder why s3c24xx_serial_remove() does not put clocks? > + uart_unregister_driver(&s3c24xx_uart_drv); > + return ret; > } > > static int s3c24xx_serial_remove(struct platform_device *dev) >
On Tue, Jul 06, 2021 at 11:49:09AM +0530, Tamseel Shams wrote: Please provide a better commit summary; "Checks the return value of function" is too vague. > "uart_add_one_port" function call may fail and return > some error code, so adding a check for return value. > If it is returning some error code, then displaying the > result, unregistering the driver and then returning from > probe function with error code. > > Signed-off-by: Tamseel Shams <m.shams@samsung.com> > --- > Changes since v1: > 1. Added support to unregister driver on failure of "uart_add_one_port" > function call. > 2. Commit message updated. > > Changes since v2: > 1. Added support to unwind clocks on failure of "uart_add_one_port" > function call. > > drivers/tty/serial/samsung_tty.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c > index 9fbc61151c2e..a3f3a17fb54b 100644 > --- a/drivers/tty/serial/samsung_tty.c > +++ b/drivers/tty/serial/samsung_tty.c > @@ -2253,7 +2253,11 @@ static int s3c24xx_serial_probe(struct platform_device *pdev) > } > > dev_dbg(&pdev->dev, "%s: adding port\n", __func__); > - uart_add_one_port(&s3c24xx_uart_drv, &ourport->port); > + ret = uart_add_one_port(&s3c24xx_uart_drv, &ourport->port); > + if (ret < 0) { > + dev_err(&pdev->dev, "Failed to add uart port, err %d\n", ret); > + goto add_port_error; > + } > platform_set_drvdata(pdev, &ourport->port); > > /* > @@ -2272,6 +2276,17 @@ static int s3c24xx_serial_probe(struct platform_device *pdev) > probe_index++; > > return 0; > + > +add_port_error: Name error labels after what they do, not where jump from (e.g. "err_disable_clk"). > + ourport->port.mapbase = 0; > + clk_disable_unprepare(ourport->clk); > + clk_put(ourport->clk); > + if (!IS_ERR(ourport->baudclk)) { > + clk_disable_unprepare(ourport->baudclk); > + clk_put(ourport->baudclk); > + } > + uart_unregister_driver(&s3c24xx_uart_drv); You can't just deregister the serial driver if probe of a single port fails. What if there are more than one port? Looks like the driver has the same bug in remove(). What a mess... Added by 6f134c3c7703 ("serial: samsung: Move uart_register_driver call to device probe") in 2014. And the clocks are never disabled and released in case uart_register_driver() fails above either. > + return ret; > } > > static int s3c24xx_serial_remove(struct platform_device *dev) Johan
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c index 9fbc61151c2e..a3f3a17fb54b 100644 --- a/drivers/tty/serial/samsung_tty.c +++ b/drivers/tty/serial/samsung_tty.c @@ -2253,7 +2253,11 @@ static int s3c24xx_serial_probe(struct platform_device *pdev) } dev_dbg(&pdev->dev, "%s: adding port\n", __func__); - uart_add_one_port(&s3c24xx_uart_drv, &ourport->port); + ret = uart_add_one_port(&s3c24xx_uart_drv, &ourport->port); + if (ret < 0) { + dev_err(&pdev->dev, "Failed to add uart port, err %d\n", ret); + goto add_port_error; + } platform_set_drvdata(pdev, &ourport->port); /* @@ -2272,6 +2276,17 @@ static int s3c24xx_serial_probe(struct platform_device *pdev) probe_index++; return 0; + +add_port_error: + ourport->port.mapbase = 0; + clk_disable_unprepare(ourport->clk); + clk_put(ourport->clk); + if (!IS_ERR(ourport->baudclk)) { + clk_disable_unprepare(ourport->baudclk); + clk_put(ourport->baudclk); + } + uart_unregister_driver(&s3c24xx_uart_drv); + return ret; } static int s3c24xx_serial_remove(struct platform_device *dev)
"uart_add_one_port" function call may fail and return some error code, so adding a check for return value. If it is returning some error code, then displaying the result, unregistering the driver and then returning from probe function with error code. Signed-off-by: Tamseel Shams <m.shams@samsung.com> --- Changes since v1: 1. Added support to unregister driver on failure of "uart_add_one_port" function call. 2. Commit message updated. Changes since v2: 1. Added support to unwind clocks on failure of "uart_add_one_port" function call. drivers/tty/serial/samsung_tty.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)