[PATCH 1/2] serial: sprd: Assign sprd_port after initialized to avoid wrong access

[PATCH 1/2] serial: sprd: Assign sprd_port after initialized to avoid wrong access

[Chunyan Zhang]

The global pointer 'sprd_port' maybe not zero when sprd_probe returns
fail, that is a risk for sprd_port to be accessed afterward, and will
lead unexpected errors.

For example:

There're two UART ports, UART1 is used for console and configured in kernel
command line, i.e. "console=";

The UART1 probe fail and the memory allocated to sprd_port[1] was released,
but sprd_port[1] was not set to NULL;

In UART2 probe, the same virtual address was allocated to sprd_port[2],
and UART2 probe process finally will go into sprd_console_setup() to
register UART1 as console since it is configured as preferred console
(filled to console_cmdline[]), but the console parameters (sprd_port[1])
actually belongs to UART2.

So move the sprd_port[] assignment to where the port already initialized
can avoid the above issue.

Fixes: b7396a38fb28 ("tty/serial: Add Spreadtrum sc9836-uart driver support")
Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 drivers/tty/serial/sprd_serial.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index b58f51296ace..942808517393 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -1106,7 +1106,7 @@ static bool sprd_uart_is_console(struct uart_port *uport)
 static int sprd_clk_init(struct uart_port *uport)
 {
 	struct clk *clk_uart, *clk_parent;
-	struct sprd_uart_port *u = sprd_port[uport->line];
+	struct sprd_uart_port *u = container_of(uport, struct sprd_uart_port, port);
 
 	clk_uart = devm_clk_get(uport->dev, "uart");
 	if (IS_ERR(clk_uart)) {
@@ -1149,22 +1149,22 @@ static int sprd_probe(struct platform_device *pdev)
 {
 	struct resource *res;
 	struct uart_port *up;
+	struct sprd_uart_port *sport;
 	int irq;
 	int index;
 	int ret;
 
 	index = of_alias_get_id(pdev->dev.of_node, "serial");
-	if (index < 0 || index >= ARRAY_SIZE(sprd_port)) {
+	if (index < 0 || index >= UART_NR_MAX) {
 		dev_err(&pdev->dev, "got a wrong serial alias id %d\n", index);
 		return -EINVAL;
 	}
 
-	sprd_port[index] = devm_kzalloc(&pdev->dev, sizeof(*sprd_port[index]),
-					GFP_KERNEL);
-	if (!sprd_port[index])
+	sport = devm_kzalloc(&pdev->dev, sizeof(*sport), GFP_KERNEL);
+	if (!sport)
 		return -ENOMEM;
 
-	up = &sprd_port[index]->port;
+	up = &sport->port;
 	up->dev = &pdev->dev;
 	up->line = index;
 	up->type = PORT_SPRD;
@@ -1195,7 +1195,7 @@ static int sprd_probe(struct platform_device *pdev)
 	 * Allocate one dma buffer to prepare for receive transfer, in case
 	 * memory allocation failure at runtime.
 	 */
-	ret = sprd_rx_alloc_buf(sprd_port[index]);
+	ret = sprd_rx_alloc_buf(sport);
 	if (ret)
 		return ret;
 
@@ -1208,12 +1208,20 @@ static int sprd_probe(struct platform_device *pdev)
 	}
 	sprd_ports_num++;
 
+	sprd_port[index] = sport;
+
 	ret = uart_add_one_port(&sprd_uart_driver, up);
 	if (ret)
-		sprd_remove(pdev);
+		goto clean_port;
 
 	platform_set_drvdata(pdev, up);
 
+	return 0;
+
+clean_port:
+	sprd_port[index] = NULL;
+	sprd_ports_num--;
+	uart_unregister_driver(&sprd_uart_driver);
 	return ret;
 }
 
-- 
2.41.0

[PATCH 2/2] serial: sprd: Fix DMA buffer leak issue

[Chunyan Zhang]

Release DMA buffer when _probe() returns fail to avoid memory leak.

Fixes: f4487db58eb7 ("serial: sprd: Add DMA mode support")
Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 drivers/tty/serial/sprd_serial.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 942808517393..e1f11382fc39 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -1203,7 +1203,7 @@ static int sprd_probe(struct platform_device *pdev)
 		ret = uart_register_driver(&sprd_uart_driver);
 		if (ret < 0) {
 			pr_err("Failed to register SPRD-UART driver\n");
-			return ret;
+			goto free_rx_buf;
 		}
 	}
 	sprd_ports_num++;
@@ -1222,6 +1222,8 @@ static int sprd_probe(struct platform_device *pdev)
 	sprd_port[index] = NULL;
 	sprd_ports_num--;
 	uart_unregister_driver(&sprd_uart_driver);
+free_rx_buf:
+	sprd_rx_free_buf(sport);
 	return ret;
 }
 
-- 
2.41.0

Re: [PATCH 2/2] serial: sprd: Fix DMA buffer leak issue

[Baolin Wang]

On 7/10/2023 4:03 PM, Chunyan Zhang wrote:
> Release DMA buffer when _probe() returns fail to avoid memory leak.
> 
> Fixes: f4487db58eb7 ("serial: sprd: Add DMA mode support")
> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>

LGTM.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

> ---
>   drivers/tty/serial/sprd_serial.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> index 942808517393..e1f11382fc39 100644
> --- a/drivers/tty/serial/sprd_serial.c
> +++ b/drivers/tty/serial/sprd_serial.c
> @@ -1203,7 +1203,7 @@ static int sprd_probe(struct platform_device *pdev)
>   		ret = uart_register_driver(&sprd_uart_driver);
>   		if (ret < 0) {
>   			pr_err("Failed to register SPRD-UART driver\n");
> -			return ret;
> +			goto free_rx_buf;
>   		}
>   	}
>   	sprd_ports_num++;
> @@ -1222,6 +1222,8 @@ static int sprd_probe(struct platform_device *pdev)
>   	sprd_port[index] = NULL;
>   	sprd_ports_num--;
>   	uart_unregister_driver(&sprd_uart_driver); > +free_rx_buf:
> +	sprd_rx_free_buf(sport);
>   	return ret;
>   }
>   

Re: [PATCH 2/2] serial: sprd: Fix DMA buffer leak issue

[Greg Kroah-Hartman]

On Mon, Jul 10, 2023 at 01:36:07PM +0200, Markus Elfring wrote:
> > Release DMA buffer when _probe() returns fail to avoid memory leak.
> 
> I would appreciate if this change description can be improved another bit.

Again, I would appreciate it if you do not provide reviews for any
kernel subsystems in which I am a maintainer of.

Please stop, it is not helpful at all.

greg k-h

Re: [PATCH 2/2] serial: sprd: Fix DMA buffer leak issue

[Chunyan Zhang]

On Mon, 10 Jul 2023 at 19:36, Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > Release DMA buffer when _probe() returns fail to avoid memory leak.
>
> I would appreciate if this change description can be improved another bit.
>

I will try my best.

>
> Will a cover letter become helpful also for the presented small patch series?

These two patches are all fixes and describe the details in each,
that's why I didn't write a cover letter which I don't think can
provide more useful information.

Thanks for your review,
Chunyan

>
> Regards,
> Markus

Re: [PATCH 1/2] serial: sprd: Assign sprd_port after initialized to avoid wrong access

[Baolin Wang]

On 7/10/2023 4:03 PM, Chunyan Zhang wrote:
> The global pointer 'sprd_port' maybe not zero when sprd_probe returns
> fail, that is a risk for sprd_port to be accessed afterward, and will
> lead unexpected errors.
> 
> For example:
> 
> There're two UART ports, UART1 is used for console and configured in kernel
> command line, i.e. "console=";
> 
> The UART1 probe fail and the memory allocated to sprd_port[1] was released,
> but sprd_port[1] was not set to NULL;

IMO, we should just set sprd_port[1] to be NULL, which seems simpler?

> 
> In UART2 probe, the same virtual address was allocated to sprd_port[2],
> and UART2 probe process finally will go into sprd_console_setup() to
> register UART1 as console since it is configured as preferred console
> (filled to console_cmdline[]), but the console parameters (sprd_port[1])
> actually belongs to UART2.

I'm confusing why the console parameters belongs to UART2? Since the 
console_cmdline[] will specify the serial index, that belongs to UART1. 
Please correct me if I miss something.

> So move the sprd_port[] assignment to where the port already initialized
> can avoid the above issue.
> 
> Fixes: b7396a38fb28 ("tty/serial: Add Spreadtrum sc9836-uart driver support")
> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> ---
>   drivers/tty/serial/sprd_serial.c | 24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> index b58f51296ace..942808517393 100644
> --- a/drivers/tty/serial/sprd_serial.c
> +++ b/drivers/tty/serial/sprd_serial.c
> @@ -1106,7 +1106,7 @@ static bool sprd_uart_is_console(struct uart_port *uport)
>   static int sprd_clk_init(struct uart_port *uport)
>   {
>   	struct clk *clk_uart, *clk_parent;
> -	struct sprd_uart_port *u = sprd_port[uport->line];
> +	struct sprd_uart_port *u = container_of(uport, struct sprd_uart_port, port);
>   
>   	clk_uart = devm_clk_get(uport->dev, "uart");
>   	if (IS_ERR(clk_uart)) {
> @@ -1149,22 +1149,22 @@ static int sprd_probe(struct platform_device *pdev)
>   {
>   	struct resource *res;
>   	struct uart_port *up;
> +	struct sprd_uart_port *sport;
>   	int irq;
>   	int index;
>   	int ret;
>   
>   	index = of_alias_get_id(pdev->dev.of_node, "serial");
> -	if (index < 0 || index >= ARRAY_SIZE(sprd_port)) {
> +	if (index < 0 || index >= UART_NR_MAX) {
>   		dev_err(&pdev->dev, "got a wrong serial alias id %d\n", index);
>   		return -EINVAL;
>   	}
>   
> -	sprd_port[index] = devm_kzalloc(&pdev->dev, sizeof(*sprd_port[index]),
> -					GFP_KERNEL);
> -	if (!sprd_port[index])
> +	sport = devm_kzalloc(&pdev->dev, sizeof(*sport), GFP_KERNEL);
> +	if (!sport)
>   		return -ENOMEM;
>   
> -	up = &sprd_port[index]->port;
> +	up = &sport->port;
>   	up->dev = &pdev->dev;
>   	up->line = index;
>   	up->type = PORT_SPRD;
> @@ -1195,7 +1195,7 @@ static int sprd_probe(struct platform_device *pdev)
>   	 * Allocate one dma buffer to prepare for receive transfer, in case
>   	 * memory allocation failure at runtime.
>   	 */
> -	ret = sprd_rx_alloc_buf(sprd_port[index]);
> +	ret = sprd_rx_alloc_buf(sport);
>   	if (ret)
>   		return ret;
>   
> @@ -1208,12 +1208,20 @@ static int sprd_probe(struct platform_device *pdev)
>   	}
>   	sprd_ports_num++;
>   
> +	sprd_port[index] = sport;
> +
>   	ret = uart_add_one_port(&sprd_uart_driver, up);
>   	if (ret)
> -		sprd_remove(pdev);
> +		goto clean_port;
>   
>   	platform_set_drvdata(pdev, up);
>   
> +	return 0;
> +
> +clean_port:
> +	sprd_port[index] = NULL;
> +	sprd_ports_num--;
> +	uart_unregister_driver(&sprd_uart_driver);
>   	return ret;
>   }
>   

Re: [PATCH 1/2] serial: sprd: Assign sprd_port after initialized to avoid wrong access

[Chunyan Zhang]

On Mon, 10 Jul 2023 at 17:57, Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
>
>
>
> On 7/10/2023 4:03 PM, Chunyan Zhang wrote:
> > The global pointer 'sprd_port' maybe not zero when sprd_probe returns
> > fail, that is a risk for sprd_port to be accessed afterward, and will
> > lead unexpected errors.
> >
> > For example:
> >
> > There're two UART ports, UART1 is used for console and configured in kernel
> > command line, i.e. "console=";
> >
> > The UART1 probe fail and the memory allocated to sprd_port[1] was released,
> > but sprd_port[1] was not set to NULL;
>
> IMO, we should just set sprd_port[1] to be NULL, which seems simpler?

This patch just does like this indeed, in the label of 'clean_port'.
Adding a local variable instead of using global pointer (sprd_port[])
to store the virtual address allocated for sprd_port can avoid
overmany goto labels.

>
> >
> > In UART2 probe, the same virtual address was allocated to sprd_port[2],
> > and UART2 probe process finally will go into sprd_console_setup() to
> > register UART1 as console since it is configured as preferred console
> > (filled to console_cmdline[]), but the console parameters (sprd_port[1])
> > actually belongs to UART2.
>
> I'm confusing why the console parameters belongs to UART2? Since the
> console_cmdline[] will specify the serial index, that belongs to UART1.

The same virtual address stored in sprd_port[1] was reallocated to
sprd_port[2] after the UART1 probe returned failure.

Thanks for the review,
Chunyan


> Please correct me if I miss something.
>
> > So move the sprd_port[] assignment to where the port already initialized
> > can avoid the above issue.
> >
> > Fixes: b7396a38fb28 ("tty/serial: Add Spreadtrum sc9836-uart driver support")
> > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > ---
> >   drivers/tty/serial/sprd_serial.c | 24 ++++++++++++++++--------
> >   1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> > index b58f51296ace..942808517393 100644
> > --- a/drivers/tty/serial/sprd_serial.c
> > +++ b/drivers/tty/serial/sprd_serial.c
> > @@ -1106,7 +1106,7 @@ static bool sprd_uart_is_console(struct uart_port *uport)
> >   static int sprd_clk_init(struct uart_port *uport)
> >   {
> >       struct clk *clk_uart, *clk_parent;
> > -     struct sprd_uart_port *u = sprd_port[uport->line];
> > +     struct sprd_uart_port *u = container_of(uport, struct sprd_uart_port, port);
> >
> >       clk_uart = devm_clk_get(uport->dev, "uart");
> >       if (IS_ERR(clk_uart)) {
> > @@ -1149,22 +1149,22 @@ static int sprd_probe(struct platform_device *pdev)
> >   {
> >       struct resource *res;
> >       struct uart_port *up;
> > +     struct sprd_uart_port *sport;
> >       int irq;
> >       int index;
> >       int ret;
> >
> >       index = of_alias_get_id(pdev->dev.of_node, "serial");
> > -     if (index < 0 || index >= ARRAY_SIZE(sprd_port)) {
> > +     if (index < 0 || index >= UART_NR_MAX) {
> >               dev_err(&pdev->dev, "got a wrong serial alias id %d\n", index);
> >               return -EINVAL;
> >       }
> >
> > -     sprd_port[index] = devm_kzalloc(&pdev->dev, sizeof(*sprd_port[index]),
> > -                                     GFP_KERNEL);
> > -     if (!sprd_port[index])
> > +     sport = devm_kzalloc(&pdev->dev, sizeof(*sport), GFP_KERNEL);
> > +     if (!sport)
> >               return -ENOMEM;
> >
> > -     up = &sprd_port[index]->port;
> > +     up = &sport->port;
> >       up->dev = &pdev->dev;
> >       up->line = index;
> >       up->type = PORT_SPRD;
> > @@ -1195,7 +1195,7 @@ static int sprd_probe(struct platform_device *pdev)
> >        * Allocate one dma buffer to prepare for receive transfer, in case
> >        * memory allocation failure at runtime.
> >        */
> > -     ret = sprd_rx_alloc_buf(sprd_port[index]);
> > +     ret = sprd_rx_alloc_buf(sport);
> >       if (ret)
> >               return ret;
> >
> > @@ -1208,12 +1208,20 @@ static int sprd_probe(struct platform_device *pdev)
> >       }
> >       sprd_ports_num++;
> >
> > +     sprd_port[index] = sport;
> > +
> >       ret = uart_add_one_port(&sprd_uart_driver, up);
> >       if (ret)
> > -             sprd_remove(pdev);
> > +             goto clean_port;
> >
> >       platform_set_drvdata(pdev, up);
> >
> > +     return 0;
> > +
> > +clean_port:
> > +     sprd_port[index] = NULL;
> > +     sprd_ports_num--;
> > +     uart_unregister_driver(&sprd_uart_driver);
> >       return ret;
> >   }
> >

Re: [PATCH 1/2] serial: sprd: Assign sprd_port after initialized to avoid wrong access

[Baolin Wang]

On 7/11/2023 10:57 AM, Chunyan Zhang wrote:
> On Mon, 10 Jul 2023 at 17:57, Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
>>
>>
>>
>> On 7/10/2023 4:03 PM, Chunyan Zhang wrote:
>>> The global pointer 'sprd_port' maybe not zero when sprd_probe returns
>>> fail, that is a risk for sprd_port to be accessed afterward, and will
>>> lead unexpected errors.
>>>
>>> For example:
>>>
>>> There're two UART ports, UART1 is used for console and configured in kernel
>>> command line, i.e. "console=";
>>>
>>> The UART1 probe fail and the memory allocated to sprd_port[1] was released,
>>> but sprd_port[1] was not set to NULL;
>>
>> IMO, we should just set sprd_port[1] to be NULL, which seems simpler?
> 
> This patch just does like this indeed, in the label of 'clean_port'.
> Adding a local variable instead of using global pointer (sprd_port[])
> to store the virtual address allocated for sprd_port can avoid
> overmany goto labels.
> 
>>
>>>
>>> In UART2 probe, the same virtual address was allocated to sprd_port[2],
>>> and UART2 probe process finally will go into sprd_console_setup() to
>>> register UART1 as console since it is configured as preferred console
>>> (filled to console_cmdline[]), but the console parameters (sprd_port[1])
>>> actually belongs to UART2.
>>
>> I'm confusing why the console parameters belongs to UART2? Since the
>> console_cmdline[] will specify the serial index, that belongs to UART1.
> 
> The same virtual address stored in sprd_port[1] was reallocated to
> sprd_port[2] after the UART1 probe returned failure.
> 

After more thinking, I understood your case. :)

But I see a nit in this patch, you added a 'clean_port' label to clear 
the resource for the fail-probe-port instead of sprd_remove(), however 
sprd_remove() will call sprd_rx_free_buf() to free the DMA buffer 
originally. I know the 2nd patch will add it back, but patch 1 is not 
git-bisect safe, right?

So I think you should also add sprd_rx_free_buf() under the 'clean_port' 
label in patch 1, then patch 2 moves the sprd_rx_free_buf() to the 
correct place.

>> Please correct me if I miss something.
>>
>>> So move the sprd_port[] assignment to where the port already initialized
>>> can avoid the above issue.
>>>
>>> Fixes: b7396a38fb28 ("tty/serial: Add Spreadtrum sc9836-uart driver support")
>>> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
>>> ---
>>>    drivers/tty/serial/sprd_serial.c | 24 ++++++++++++++++--------
>>>    1 file changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
>>> index b58f51296ace..942808517393 100644
>>> --- a/drivers/tty/serial/sprd_serial.c
>>> +++ b/drivers/tty/serial/sprd_serial.c
>>> @@ -1106,7 +1106,7 @@ static bool sprd_uart_is_console(struct uart_port *uport)
>>>    static int sprd_clk_init(struct uart_port *uport)
>>>    {
>>>        struct clk *clk_uart, *clk_parent;
>>> -     struct sprd_uart_port *u = sprd_port[uport->line];
>>> +     struct sprd_uart_port *u = container_of(uport, struct sprd_uart_port, port);
>>>
>>>        clk_uart = devm_clk_get(uport->dev, "uart");
>>>        if (IS_ERR(clk_uart)) {
>>> @@ -1149,22 +1149,22 @@ static int sprd_probe(struct platform_device *pdev)
>>>    {
>>>        struct resource *res;
>>>        struct uart_port *up;
>>> +     struct sprd_uart_port *sport;
>>>        int irq;
>>>        int index;
>>>        int ret;
>>>
>>>        index = of_alias_get_id(pdev->dev.of_node, "serial");
>>> -     if (index < 0 || index >= ARRAY_SIZE(sprd_port)) {
>>> +     if (index < 0 || index >= UART_NR_MAX) {
>>>                dev_err(&pdev->dev, "got a wrong serial alias id %d\n", index);
>>>                return -EINVAL;
>>>        }
>>>
>>> -     sprd_port[index] = devm_kzalloc(&pdev->dev, sizeof(*sprd_port[index]),
>>> -                                     GFP_KERNEL);
>>> -     if (!sprd_port[index])
>>> +     sport = devm_kzalloc(&pdev->dev, sizeof(*sport), GFP_KERNEL);
>>> +     if (!sport)
>>>                return -ENOMEM;
>>>
>>> -     up = &sprd_port[index]->port;
>>> +     up = &sport->port;
>>>        up->dev = &pdev->dev;
>>>        up->line = index;
>>>        up->type = PORT_SPRD;
>>> @@ -1195,7 +1195,7 @@ static int sprd_probe(struct platform_device *pdev)
>>>         * Allocate one dma buffer to prepare for receive transfer, in case
>>>         * memory allocation failure at runtime.
>>>         */
>>> -     ret = sprd_rx_alloc_buf(sprd_port[index]);
>>> +     ret = sprd_rx_alloc_buf(sport);
>>>        if (ret)
>>>                return ret;
>>>
>>> @@ -1208,12 +1208,20 @@ static int sprd_probe(struct platform_device *pdev)
>>>        }
>>>        sprd_ports_num++;
>>>
>>> +     sprd_port[index] = sport;
>>> +
>>>        ret = uart_add_one_port(&sprd_uart_driver, up);
>>>        if (ret)
>>> -             sprd_remove(pdev);
>>> +             goto clean_port;
>>>
>>>        platform_set_drvdata(pdev, up);
>>>
>>> +     return 0;
>>> +
>>> +clean_port:
>>> +     sprd_port[index] = NULL;
>>> +     sprd_ports_num--;
>>> +     uart_unregister_driver(&sprd_uart_driver);
>>>        return ret;
>>>    }
>>>

Re: [PATCH 1/2] serial: sprd: Assign sprd_port after initialized to avoid wrong access

[Chunyan Zhang]

On Wed, 12 Jul 2023 at 09:39, Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
>
>
>
> On 7/11/2023 10:57 AM, Chunyan Zhang wrote:
> > On Mon, 10 Jul 2023 at 17:57, Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
> >>
> >>
> >>
> >> On 7/10/2023 4:03 PM, Chunyan Zhang wrote:
> >>> The global pointer 'sprd_port' maybe not zero when sprd_probe returns
> >>> fail, that is a risk for sprd_port to be accessed afterward, and will
> >>> lead unexpected errors.
> >>>
> >>> For example:
> >>>
> >>> There're two UART ports, UART1 is used for console and configured in kernel
> >>> command line, i.e. "console=";
> >>>
> >>> The UART1 probe fail and the memory allocated to sprd_port[1] was released,
> >>> but sprd_port[1] was not set to NULL;
> >>
> >> IMO, we should just set sprd_port[1] to be NULL, which seems simpler?
> >
> > This patch just does like this indeed, in the label of 'clean_port'.
> > Adding a local variable instead of using global pointer (sprd_port[])
> > to store the virtual address allocated for sprd_port can avoid
> > overmany goto labels.
> >
> >>
> >>>
> >>> In UART2 probe, the same virtual address was allocated to sprd_port[2],
> >>> and UART2 probe process finally will go into sprd_console_setup() to
> >>> register UART1 as console since it is configured as preferred console
> >>> (filled to console_cmdline[]), but the console parameters (sprd_port[1])
> >>> actually belongs to UART2.
> >>
> >> I'm confusing why the console parameters belongs to UART2? Since the
> >> console_cmdline[] will specify the serial index, that belongs to UART1.
> >
> > The same virtual address stored in sprd_port[1] was reallocated to
> > sprd_port[2] after the UART1 probe returned failure.
> >
>
> After more thinking, I understood your case. :)
>
> But I see a nit in this patch, you added a 'clean_port' label to clear
> the resource for the fail-probe-port instead of sprd_remove(), however
> sprd_remove() will call sprd_rx_free_buf() to free the DMA buffer
> originally. I know the 2nd patch will add it back, but patch 1 is not
> git-bisect safe, right?

Actually it doesn't make git-bisect unsafe, the driver can work with a
memory leak issue which has been there and fixed in another patch.
But I can add back sprd_remove() to keep the unrelated code logic the same.

Thanks for the review,
Chunyan

>
> So I think you should also add sprd_rx_free_buf() under the 'clean_port'
> label in patch 1, then patch 2 moves the sprd_rx_free_buf() to the
> correct place.
>