[PATCH] arm pl011 serial: support multi-irq request

[PATCH] arm pl011 serial: support multi-irq request

[Bing Fan]

From: Bing Fan <tombinfan@tencent.com>

In order to make pl011 work better, multiple interrupts are
required, such as TXIM, RXIM, RTIM, error interrupt(FE/PE/BE/OE);
at the same time, pl011 to GIC does not merge the interrupt
lines(each serial-interrupt corresponding to different GIC hardware
interrupt), so need to enable and request multiple gic interrupt
numbers in the driver.

Signed-off-by: Bing Fan <tombinfan@tencent.com>
---
 drivers/tty/serial/amba-pl011.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 78682c12156a..b63164e89903 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1703,9 +1703,30 @@ static void pl011_write_lcr_h(struct uart_amba_port *uap, unsigned int lcr_h)
 
 static int pl011_allocate_irq(struct uart_amba_port *uap)
 {
+	int ret = -1;
+	int i = 0;
+	unsigned int virq = 0;
+	struct amba_device *amba_dev = (struct amba_device *)uap->port.dev;
+
+	if (!amba_dev)
+		return -1;
+
 	pl011_write(uap->im, uap, REG_IMSC);
 
-	return request_irq(uap->port.irq, pl011_int, IRQF_SHARED, "uart-pl011", uap);
+	for (i = 0; i < AMBA_NR_IRQS; i++) {
+		virq = amba_dev->irq[i];
+		if (virq == 0)
+			break;
+
+		ret = request_irq(virq, pl011_int, IRQF_SHARED, "uart-pl011-*", uap);
+		if (ret < 0) {
+			dev_info(uap->port.dev, "%s %d request %u interrupt failed\n",
+					__func__, __LINE__, virq);
+			break;
+		}
+	}
+
+	return ret;
 }
 
 /*
-- 
2.17.1

Re: [PATCH] arm pl011 serial: support multi-irq request

[Greg KH]

On Tue, Jun 29, 2021 at 09:29:24AM +0800, Bing Fan wrote:
> From: Bing Fan <tombinfan@tencent.com>
> 
> In order to make pl011 work better, multiple interrupts are
> required, such as TXIM, RXIM, RTIM, error interrupt(FE/PE/BE/OE);
> at the same time, pl011 to GIC does not merge the interrupt
> lines(each serial-interrupt corresponding to different GIC hardware
> interrupt), so need to enable and request multiple gic interrupt
> numbers in the driver.
> 
> Signed-off-by: Bing Fan <tombinfan@tencent.com>
> ---
>  drivers/tty/serial/amba-pl011.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 78682c12156a..b63164e89903 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -1703,9 +1703,30 @@ static void pl011_write_lcr_h(struct uart_amba_port *uap, unsigned int lcr_h)
>  
>  static int pl011_allocate_irq(struct uart_amba_port *uap)
>  {
> +	int ret = -1;

Pick a real error value.

> +	int i = 0;

Why is this initialized?

> +	unsigned int virq = 0;

Why is this initialized?

> +	struct amba_device *amba_dev = (struct amba_device *)uap->port.dev;

Are you sure you can just cast this like this?  Did you test this?

> +
> +	if (!amba_dev)
> +		return -1;

Do not make up error numbers, return a specific -ERR* value.

And how can this happen?

> +
>  	pl011_write(uap->im, uap, REG_IMSC);
>  
> -	return request_irq(uap->port.irq, pl011_int, IRQF_SHARED, "uart-pl011", uap);
> +	for (i = 0; i < AMBA_NR_IRQS; i++) {
> +		virq = amba_dev->irq[i];
> +		if (virq == 0)
> +			break;
> +
> +		ret = request_irq(virq, pl011_int, IRQF_SHARED, "uart-pl011-*", uap);
> +		if (ret < 0) {
> +			dev_info(uap->port.dev, "%s %d request %u interrupt failed\n",
> +					__func__, __LINE__, virq);

dev_err() is for errors, not dev_info().

And no need for __func__ and __LINE__ for the dev_* calls.



> +			break;
> +		}
> +	}
> +
> +	return ret;
>  }
>  
>  /*
> -- 
> 2.17.1
> 

Re: [PATCH] arm pl011 serial: support multi-irq request

[Greg KH]

On Tue, Jun 29, 2021 at 07:32:36PM +0800, Bing Fan wrote:
> hello,
> 
> replied as below.
> 
> and new patch is at the bottom.

Please submit this properly as the documentation says to do so, I can't
take an attachment :(

> > > +	struct amba_device *amba_dev = (struct amba_device *)uap->port.dev;
> > Are you sure you can just cast this like this?  Did you test this?
> 
> 
> Yes, i have tested and applied in my project.
> 
> The function pl011_probe calls pl011_setup_port with &amba_dev->dev and uap
> params;
> 
> and pl011_setup_port set uap->port.dev to the address of amba_dev->dev;
> 
> the two structs' relationship is:
> 
>     struct amba_device {
> 
>         struct device dev;
> 
>         ……
> 
>     };
> 
> When pointer(uap->port.dev) points to amba_dev->dev address, the momery
> actully stores
> 
> content of struct amba_device; so the cast assignment can be forced to
> amba_dev.

That is now how this should work, use the correct container_of() cast
instead.  That will always work no matter where struct device is in the
structure.  You got lucky here :)

> > > +
> > > +	if (!amba_dev)
> > > +		return -1;
> > Do not make up error numbers, return a specific -ERR* value.
> 
> changed to "return -ENODEV"

So this changed the logic of this function, is that ok?

> > 
> > And how can this happen?
> 
> The function pl011_setup_port isn't called, event pl011_probe isn't called.

And how can that ever happen?


thanks,

greg k-h

Re: [PATCH] arm pl011 serial: support multi-irq request

[Greg KH]

On Tue, Jun 29, 2021 at 08:31:00PM +0800, Bing Fan wrote:
> hello,
> 
> 
> 在 6/29/2021 20:18, Greg KH 写道:
> > On Tue, Jun 29, 2021 at 07:32:36PM +0800, Bing Fan wrote:
> > > hello, replied as below. and new patch is at the bottom.
> > Please submit this properly as the documentation says to do so, I can't
> > take an attachment :(
> Ok.
> > > > > + struct amba_device *amba_dev = (struct amba_device *)uap->port.dev;
> > > > Are you sure you can just cast this like this? Did you test this?
> > > Yes, i have tested and applied in my project. The function
> > > pl011_probe calls pl011_setup_port with &amba_dev->dev and uap
> > > params; and pl011_setup_port set uap->port.dev to the address of
> > > amba_dev->dev; the two structs' relationship is:     struct
> > > amba_device {         struct device dev;         ……     }; When
> > > pointer(uap->port.dev) points to amba_dev->dev address, the momery
> > > actully stores content of struct amba_device; so the cast assignment
> > > can be forced to amba_dev.
> > That is now how this should work, use the correct container_of() cast
> > instead. That will always work no matter where struct device is in the
> > structure. You got lucky here :)
> 
> changed to "struct amba_device *amba_dev = container_of(uap->port.dev, struct amba_device, dev);"
> 
> 
> > > > > + + if (!amba_dev) + return -1;
> > > > Do not make up error numbers, return a specific -ERR* value.
> > > changed to "return -ENODEV"
> > So this changed the logic of this function, is that ok?
> 
> No, just sanity check.

If it can never happen, no need to check for it.

> > > > And how can this happen?
> > > The function pl011_setup_port isn't called, event pl011_probe isn't
> > > called.
> > And how can that ever happen?
> 
> If there is no pl011 device.

How can that happen here?

thanks,

greg k-h

Re: [PATCH] arm pl011 serial: support multi-irq request

[Greg Kroah-Hartman]

On Mon, Jun 28, 2021 at 03:19:13PM +0800, bing fan wrote:
> From: Bing Fan <tombinfan@tencent.com>
> 
> In order to make pl011 work better, multiple interrupts are
> required, such as TXIM, RXIM, RTIM, error interrupt(FE/PE/BE/OE);
> at the same time, pl011 to GIC does not merge the interrupt
> lines(each serial-interrupt corresponding to different GIC hardware
> interrupt), so need to enable and request multiple gic interrupt
> numbers in the driver.
> 
> Signed-off-by: Bing Fan <hptsfb@gmail.com>
> ---
>  drivers/tty/serial/amba-pl011.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 78682c12156a..2b6f43c27dea 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -1703,9 +1703,30 @@ static void pl011_write_lcr_h(struct
> uart_amba_port *uap, unsigned int lcr_h)
> 
>  static int pl011_allocate_irq(struct uart_amba_port *uap)
>  {
> +       int ret = -1;
> +       int i = 0;
> +       unsigned int virq = 0;
> +       struct amba_device *amba_dev = (struct amba_device *)uap->port.dev;
> +
> +       if (!amba_dev)
> +               return -1;
> +
>         pl011_write(uap->im, uap, REG_IMSC);
> 
> -       return request_irq(uap->port.irq, pl011_int, IRQF_SHARED,
> "uart-pl011", uap);
> +       for (i = 0; i < AMBA_NR_IRQS; i++) {
> +               virq = amba_dev->irq[i];
> +               if (virq == 0)          // request irq until virq is 0
> +                       break;
> +
> +               ret = request_irq(virq, pl011_int, IRQF_SHARED,
> "uart-pl011-*", uap);
> +               if (ret < 0) {
> +                       dev_info(uap->port.dev, "%s %d request %u
> interrupt failed\n",
> +                                       __func__, __LINE__, virq);
> +                       break;
> +               }
> +       }
> +
> +       return ret;
>  }
> 
>  /*
> -- 
> 2.17.1


Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch is malformed (tabs converted to spaces, linewrapped, etc.)
  and can not be applied.  Please read the file,
  Documentation/email-clients.txt in order to fix this.


If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

[PATCH] arm pl011 serial: support multi-irq request

[bing fan]

From: Bing Fan <tombinfan@tencent.com>

In order to make pl011 work better, multiple interrupts are
required, such as TXIM, RXIM, RTIM, error interrupt(FE/PE/BE/OE);
at the same time, pl011 to GIC does not merge the interrupt
lines(each serial-interrupt corresponding to different GIC hardware
interrupt), so need to enable and request multiple gic interrupt
numbers in the driver.

Signed-off-by: Bing Fan <hptsfb@gmail.com>
---
 drivers/tty/serial/amba-pl011.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 78682c12156a..2b6f43c27dea 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1703,9 +1703,30 @@ static void pl011_write_lcr_h(struct
uart_amba_port *uap, unsigned int lcr_h)

 static int pl011_allocate_irq(struct uart_amba_port *uap)
 {
+       int ret = -1;
+       int i = 0;
+       unsigned int virq = 0;
+       struct amba_device *amba_dev = (struct amba_device *)uap->port.dev;
+
+       if (!amba_dev)
+               return -1;
+
        pl011_write(uap->im, uap, REG_IMSC);

-       return request_irq(uap->port.irq, pl011_int, IRQF_SHARED,
"uart-pl011", uap);
+       for (i = 0; i < AMBA_NR_IRQS; i++) {
+               virq = amba_dev->irq[i];
+               if (virq == 0)          // request irq until virq is 0
+                       break;
+
+               ret = request_irq(virq, pl011_int, IRQF_SHARED,
"uart-pl011-*", uap);
+               if (ret < 0) {
+                       dev_info(uap->port.dev, "%s %d request %u
interrupt failed\n",
+                                       __func__, __LINE__, virq);
+                       break;
+               }
+       }
+
+       return ret;
 }

 /*
-- 
2.17.1