2021-06-29 01:30:57

by Bing Fan

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

From: Bing Fan <[email protected]>

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 <[email protected]>
---
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


2021-06-29 06:20:47

by Greg Kroah-Hartman

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

On Tue, Jun 29, 2021 at 09:29:24AM +0800, Bing Fan wrote:
> From: Bing Fan <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>

2021-06-29 12:20:11

by Greg Kroah-Hartman

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

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

2021-06-29 13:17:16

by Greg Kroah-Hartman

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

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