| Message ID | 20220321112211.8895-1-sherry.sun@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | tty: serial: fsl_lpuart: fix potential bug when using both of_alias_get_id and ida_simple_get | expand |
Hi Shenwei, > > Subject: [PATCH] tty: serial: fsl_lpuart: fix potential bug when using > > both of_alias_get_id and ida_simple_get > > > > Now fsl_lpuart driver use both of_alias_get_id() and ida_simple_get() > > in .probe(), which has the potential bug. For example, when remove the > > lpuart7 alias in dts, of_alias_get_id() will return error, then call > > ida_simple_get() to allocate the id 0 for lpuart7, this may confilct > > with the > > lpuart4 which has alias 0. > > The behavior of ida_simple_get() is to allocate an unused alias number. Do > you see any conflict when you test? > Yes, I have verified this on imx8ulp, I print the port.line value for each uart port. When I remove the lpuart7 alias, the lpuart7 will get the id 0, same as lpuart4, which caused the lpuart7 probe fail. ida_simple_get() cannot figure out which id has been used in the uart driver. [ 0.894549] port.line = 0 --> lpuart4 [ 0.908148] port.line = 1 --> lpuart5 [ 0.939844] port.line = 2 --> lpuart6 [ 0.953608] port.line = 0 --> lpuart7 --- a/arch/arm64/boot/dts/freescale/imx8ulp.dtsi +++ b/arch/arm64/boot/dts/freescale/imx8ulp.dtsi @@ -25,7 +25,6 @@ serial0 = &lpuart4; serial1 = &lpuart5; serial2 = &lpuart6; - serial3 = &lpuart7; }; Best regards Sherry > Regards, > Shenwei > > > > > aliases { > > ... > > serial0 = &lpuart4; > > serial1 = &lpuart5; > > serial2 = &lpuart6; > > serial3 = &lpuart7; > > } > > > > So remove the ida_simple_get() in .probe(), return an error directly > > when calling > > of_alias_get_id() fails, which is consistent with other uart drivers behavior. > > > > Fixes: 3bc3206e1c0f ("serial: fsl_lpuart: Remove the alias node > > dependence") > > Signed-off-by: Sherry Sun <sherry.sun@nxp.com> > > --- > > drivers/tty/serial/fsl_lpuart.c | 24 ++++-------------------- > > 1 file changed, 4 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/tty/serial/fsl_lpuart.c > > b/drivers/tty/serial/fsl_lpuart.c index 452a015825ba..40465d23d7ad > > 100644 > > --- a/drivers/tty/serial/fsl_lpuart.c > > +++ b/drivers/tty/serial/fsl_lpuart.c > > @@ -239,8 +239,6 @@ > > /* IMX lpuart has four extra unused regs located at the beginning */ > > #define IMX_REG_OFF 0x10 > > > > -static DEFINE_IDA(fsl_lpuart_ida); > > - > > enum lpuart_type { > > VF610_LPUART, > > LS1021A_LPUART, > > @@ -276,7 +274,6 @@ struct lpuart_port { > > int rx_dma_rng_buf_len; > > unsigned int dma_tx_nents; > > wait_queue_head_t dma_wait; > > - bool id_allocated; > > }; > > > > struct lpuart_soc_data { > > @@ -2716,23 +2713,18 @@ static int lpuart_probe(struct platform_device > > *pdev) > > > > ret = of_alias_get_id(np, "serial"); > > if (ret < 0) { > > - ret = ida_simple_get(&fsl_lpuart_ida, 0, UART_NR, > > GFP_KERNEL); > > - if (ret < 0) { > > - dev_err(&pdev->dev, "port line is full, add device > > failed\n"); > > - return ret; > > - } > > - sport->id_allocated = true; > > + dev_err(&pdev->dev, "failed to get alias id, errno %d\n", ret); > > + return ret; > > } > > if (ret >= ARRAY_SIZE(lpuart_ports)) { > > dev_err(&pdev->dev, "serial%d out of range\n", ret); > > - ret = -EINVAL; > > - goto failed_out_of_range; > > + return -EINVAL; > > } > > sport->port.line = ret; > > > > ret = lpuart_enable_clks(sport); > > if (ret) > > - goto failed_clock_enable; > > + return ret; > > sport->port.uartclk = lpuart_get_baud_clk_rate(sport); > > > > lpuart_ports[sport->port.line] = sport; @@ -2781,10 +2773,6 @@ > > static int lpuart_probe(struct platform_device *pdev) > > failed_attach_port: > > failed_irq_request: > > lpuart_disable_clks(sport); > > -failed_clock_enable: > > -failed_out_of_range: > > - if (sport->id_allocated) > > - ida_simple_remove(&fsl_lpuart_ida, sport->port.line); > > return ret; > > } > > > > @@ -2794,9 +2782,6 @@ static int lpuart_remove(struct platform_device > > *pdev) > > > > uart_remove_one_port(&lpuart_reg, &sport->port); > > > > - if (sport->id_allocated) > > - ida_simple_remove(&fsl_lpuart_ida, sport->port.line); > > - > > lpuart_disable_clks(sport); > > > > if (sport->dma_tx_chan) > > @@ -2926,7 +2911,6 @@ static int __init lpuart_serial_init(void) > > > > static void __exit lpuart_serial_exit(void) { > > - ida_destroy(&fsl_lpuart_ida); > > platform_driver_unregister(&lpuart_driver); > > uart_unregister_driver(&lpuart_reg); > > } > > -- > > 2.17.1
Gentle ping... > -----Original Message----- > From: Sherry Sun > Sent: 2022年3月21日 19:25 > To: gregkh@linuxfoundation.org; jirislaby@kernel.org; Vabhav Sharma > <vabhav.sharma@nxp.com> > Cc: linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com> > Subject: [PATCH] tty: serial: fsl_lpuart: fix potential bug when using both > of_alias_get_id and ida_simple_get > > Now fsl_lpuart driver use both of_alias_get_id() and ida_simple_get() > in .probe(), which has the potential bug. For example, when remove the > lpuart7 alias in dts, of_alias_get_id() will return error, then call > ida_simple_get() to allocate the id 0 for lpuart7, this may confilct with the > lpuart4 which has alias 0. > > aliases { > ... > serial0 = &lpuart4; > serial1 = &lpuart5; > serial2 = &lpuart6; > serial3 = &lpuart7; > } > > So remove the ida_simple_get() in .probe(), return an error directly when > calling of_alias_get_id() fails, which is consistent with other uart drivers > behavior. > > Fixes: 3bc3206e1c0f ("serial: fsl_lpuart: Remove the alias node dependence") > Signed-off-by: Sherry Sun <sherry.sun@nxp.com> > --- > drivers/tty/serial/fsl_lpuart.c | 24 ++++-------------------- > 1 file changed, 4 insertions(+), 20 deletions(-) > > diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c > index 452a015825ba..40465d23d7ad 100644 > --- a/drivers/tty/serial/fsl_lpuart.c > +++ b/drivers/tty/serial/fsl_lpuart.c > @@ -239,8 +239,6 @@ > /* IMX lpuart has four extra unused regs located at the beginning */ > #define IMX_REG_OFF 0x10 > > -static DEFINE_IDA(fsl_lpuart_ida); > - > enum lpuart_type { > VF610_LPUART, > LS1021A_LPUART, > @@ -276,7 +274,6 @@ struct lpuart_port { > int rx_dma_rng_buf_len; > unsigned int dma_tx_nents; > wait_queue_head_t dma_wait; > - bool id_allocated; > }; > > struct lpuart_soc_data { > @@ -2716,23 +2713,18 @@ static int lpuart_probe(struct platform_device > *pdev) > > ret = of_alias_get_id(np, "serial"); > if (ret < 0) { > - ret = ida_simple_get(&fsl_lpuart_ida, 0, UART_NR, > GFP_KERNEL); > - if (ret < 0) { > - dev_err(&pdev->dev, "port line is full, add device > failed\n"); > - return ret; > - } > - sport->id_allocated = true; > + dev_err(&pdev->dev, "failed to get alias id, errno %d\n", ret); > + return ret; > } > if (ret >= ARRAY_SIZE(lpuart_ports)) { > dev_err(&pdev->dev, "serial%d out of range\n", ret); > - ret = -EINVAL; > - goto failed_out_of_range; > + return -EINVAL; > } > sport->port.line = ret; > > ret = lpuart_enable_clks(sport); > if (ret) > - goto failed_clock_enable; > + return ret; > sport->port.uartclk = lpuart_get_baud_clk_rate(sport); > > lpuart_ports[sport->port.line] = sport; @@ -2781,10 +2773,6 @@ > static int lpuart_probe(struct platform_device *pdev) > failed_attach_port: > failed_irq_request: > lpuart_disable_clks(sport); > -failed_clock_enable: > -failed_out_of_range: > - if (sport->id_allocated) > - ida_simple_remove(&fsl_lpuart_ida, sport->port.line); > return ret; > } > > @@ -2794,9 +2782,6 @@ static int lpuart_remove(struct platform_device > *pdev) > > uart_remove_one_port(&lpuart_reg, &sport->port); > > - if (sport->id_allocated) > - ida_simple_remove(&fsl_lpuart_ida, sport->port.line); > - > lpuart_disable_clks(sport); > > if (sport->dma_tx_chan) > @@ -2926,7 +2911,6 @@ static int __init lpuart_serial_init(void) > > static void __exit lpuart_serial_exit(void) { > - ida_destroy(&fsl_lpuart_ida); > platform_driver_unregister(&lpuart_driver); > uart_unregister_driver(&lpuart_reg); > } > -- > 2.17.1
On Fri, Mar 25, 2022 at 01:27:05AM +0000, Sherry Sun wrote:
> Gentle ping...
It is the middle of the merge window. I can't do anything with new
changes until after 5.18-rc1 is out. Please wait until then.
In the meantime, please help out by reviewing other pending patches from
the mailing list.
thanks!
greg k-h
Hi Greg, > Subject: Re: [PATCH] tty: serial: fsl_lpuart: fix potential bug when using both > of_alias_get_id and ida_simple_get > > On Fri, Mar 25, 2022 at 01:27:05AM +0000, Sherry Sun wrote: > > Gentle ping... > > It is the middle of the merge window. I can't do anything with new > changes until after 5.18-rc1 is out. Please wait until then. > > In the meantime, please help out by reviewing other pending patches from > the mailing list. Got it, thanks for the info. Best regards Sherry > > thanks! > > greg k-h
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c index 452a015825ba..40465d23d7ad 100644 --- a/drivers/tty/serial/fsl_lpuart.c +++ b/drivers/tty/serial/fsl_lpuart.c @@ -239,8 +239,6 @@ /* IMX lpuart has four extra unused regs located at the beginning */ #define IMX_REG_OFF 0x10 -static DEFINE_IDA(fsl_lpuart_ida); - enum lpuart_type { VF610_LPUART, LS1021A_LPUART, @@ -276,7 +274,6 @@ struct lpuart_port { int rx_dma_rng_buf_len; unsigned int dma_tx_nents; wait_queue_head_t dma_wait; - bool id_allocated; }; struct lpuart_soc_data { @@ -2716,23 +2713,18 @@ static int lpuart_probe(struct platform_device *pdev) ret = of_alias_get_id(np, "serial"); if (ret < 0) { - ret = ida_simple_get(&fsl_lpuart_ida, 0, UART_NR, GFP_KERNEL); - if (ret < 0) { - dev_err(&pdev->dev, "port line is full, add device failed\n"); - return ret; - } - sport->id_allocated = true; + dev_err(&pdev->dev, "failed to get alias id, errno %d\n", ret); + return ret; } if (ret >= ARRAY_SIZE(lpuart_ports)) { dev_err(&pdev->dev, "serial%d out of range\n", ret); - ret = -EINVAL; - goto failed_out_of_range; + return -EINVAL; } sport->port.line = ret; ret = lpuart_enable_clks(sport); if (ret) - goto failed_clock_enable; + return ret; sport->port.uartclk = lpuart_get_baud_clk_rate(sport); lpuart_ports[sport->port.line] = sport; @@ -2781,10 +2773,6 @@ static int lpuart_probe(struct platform_device *pdev) failed_attach_port: failed_irq_request: lpuart_disable_clks(sport); -failed_clock_enable: -failed_out_of_range: - if (sport->id_allocated) - ida_simple_remove(&fsl_lpuart_ida, sport->port.line); return ret; } @@ -2794,9 +2782,6 @@ static int lpuart_remove(struct platform_device *pdev) uart_remove_one_port(&lpuart_reg, &sport->port); - if (sport->id_allocated) - ida_simple_remove(&fsl_lpuart_ida, sport->port.line); - lpuart_disable_clks(sport); if (sport->dma_tx_chan) @@ -2926,7 +2911,6 @@ static int __init lpuart_serial_init(void) static void __exit lpuart_serial_exit(void) { - ida_destroy(&fsl_lpuart_ida); platform_driver_unregister(&lpuart_driver); uart_unregister_driver(&lpuart_reg); }
Now fsl_lpuart driver use both of_alias_get_id() and ida_simple_get() in .probe(), which has the potential bug. For example, when remove the lpuart7 alias in dts, of_alias_get_id() will return error, then call ida_simple_get() to allocate the id 0 for lpuart7, this may confilct with the lpuart4 which has alias 0. aliases { ... serial0 = &lpuart4; serial1 = &lpuart5; serial2 = &lpuart6; serial3 = &lpuart7; } So remove the ida_simple_get() in .probe(), return an error directly when calling of_alias_get_id() fails, which is consistent with other uart drivers behavior. Fixes: 3bc3206e1c0f ("serial: fsl_lpuart: Remove the alias node dependence") Signed-off-by: Sherry Sun <sherry.sun@nxp.com> --- drivers/tty/serial/fsl_lpuart.c | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-)