| Message ID | 1449376769-13369-7-git-send-email-soren.brinkmann@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/05/2015 08:39 PM, Soren Brinkmann wrote: > Request_irq() should be _after_ h/w programming, otherwise an > interrupt could be triggered and in-progress before the h/w has been > setup. Slight misunderstanding. My fault; I should have been more explicit. 1. Any setup necessary for the isr not to be confused and misdirect spurious interrupts (or hang) should be before installing the isr with request_irq() None of this code should trigger an interrupt. 2. Clear pending interrupts 3. Install the isr with request_irq() 4. Enable interrupts For extra safety, first disable interrupts before starting h/w programming. I would do the v5 series in the same order as the v3 series only up to what I reviewed. Then do another series with the remainder plus new changes, ok? Regards, Peter Hurley > Reported-by: Peter Hurley <peter@hurleysoftware.com> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > --- > v4: > - this patch has been added. Thanks to Peter for pointing it out and providing > commit message > --- > drivers/tty/serial/xilinx_uartps.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c > index 6ffd3bbe3e18..1e9053656610 100644 > --- a/drivers/tty/serial/xilinx_uartps.c > +++ b/drivers/tty/serial/xilinx_uartps.c > @@ -759,12 +759,7 @@ static void cdns_uart_set_termios(struct uart_port *port, > static int cdns_uart_startup(struct uart_port *port) > { > unsigned long flags; > - unsigned int retval = 0, status = 0; > - > - retval = request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME, > - (void *)port); > - if (retval) > - return retval; > + unsigned int status = 0; > > spin_lock_irqsave(&port->lock, flags); > > @@ -818,7 +813,7 @@ static int cdns_uart_startup(struct uart_port *port) > > spin_unlock_irqrestore(&port->lock, flags); > > - return retval; > + return request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME, port); > } > > /** >
On Thu, 2015-12-10 at 01:41PM -0800, Peter Hurley wrote: > On 12/05/2015 08:39 PM, Soren Brinkmann wrote: > > Request_irq() should be _after_ h/w programming, otherwise an > > interrupt could be triggered and in-progress before the h/w has been > > setup. > > Slight misunderstanding. My fault; I should have been more explicit. > > 1. Any setup necessary for the isr not to be confused and misdirect spurious > interrupts (or hang) should be before installing the isr with request_irq() > None of this code should trigger an interrupt. > 2. Clear pending interrupts > 3. Install the isr with request_irq() > 4. Enable interrupts Isn't that what the startup function is doing now - more or less. I think 3 and 4 are swapped to release the lock and then do the request_irq, but I believe that should be OK. The startup function configures the HW. Clears the ISR. Enables the intended IRQs and then does the request_irq call. > > For extra safety, first disable interrupts before starting h/w programming. It's done within spin_lock_irqsave, which gives us at least locally disabled IRQs. I guess we could add a disabling all IRQs in the UART core, but it should not really be necessary. > > I would do the v5 series in the same order as the v3 series only up to > what I reviewed. Then do another series with the remainder plus new changes, ok? Sure. Sören > > Regards, > Peter Hurley > > > Reported-by: Peter Hurley <peter@hurleysoftware.com> > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > --- > > v4: > > - this patch has been added. Thanks to Peter for pointing it out and providing > > commit message > > --- > > drivers/tty/serial/xilinx_uartps.c | 9 ++------- > > 1 file changed, 2 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c > > index 6ffd3bbe3e18..1e9053656610 100644 > > --- a/drivers/tty/serial/xilinx_uartps.c > > +++ b/drivers/tty/serial/xilinx_uartps.c > > @@ -759,12 +759,7 @@ static void cdns_uart_set_termios(struct uart_port *port, > > static int cdns_uart_startup(struct uart_port *port) > > { > > unsigned long flags; > > - unsigned int retval = 0, status = 0; > > - > > - retval = request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME, > > - (void *)port); > > - if (retval) > > - return retval; > > + unsigned int status = 0; > > > > spin_lock_irqsave(&port->lock, flags); > > > > @@ -818,7 +813,7 @@ static int cdns_uart_startup(struct uart_port *port) > > > > spin_unlock_irqrestore(&port->lock, flags); > > > > - return retval; > > + return request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME, port); > > } > > > > /** > > >
On 12/15/2015 07:41 AM, Sören Brinkmann wrote: > On Thu, 2015-12-10 at 01:41PM -0800, Peter Hurley wrote: >> On 12/05/2015 08:39 PM, Soren Brinkmann wrote: >>> Request_irq() should be _after_ h/w programming, otherwise an >>> interrupt could be triggered and in-progress before the h/w has been >>> setup. >> >> Slight misunderstanding. My fault; I should have been more explicit. >> >> 1. Any setup necessary for the isr not to be confused and misdirect spurious >> interrupts (or hang) should be before installing the isr with request_irq() >> None of this code should trigger an interrupt. >> 2. Clear pending interrupts >> 3. Install the isr with request_irq() >> 4. Enable interrupts > > Isn't that what the startup function is doing now - more or less. I > think 3 and 4 are swapped to release the lock and then do the > request_irq, but I believe that should be OK. > The startup function configures the HW. Clears the ISR. Enables the > intended IRQs and then does the request_irq call. If the driver enables interrupts before installing the isr with request_irq() and an interrupt occurs there will the no handler to catch it and EOI the device. >> For extra safety, first disable interrupts before starting h/w programming. > > It's done within spin_lock_irqsave, which gives us at least locally > disabled IRQs. I guess we could add a disabling all IRQs in the UART > core, but it should not really be necessary. Similar issue. What I mean is to mask interrupts from this device so that h/w programming doesn't accidentally trigger an interrupt for which no isr is installed. It's a bit overkill; that's why I said "extra safety". Regards, Peter Hurley >> I would do the v5 series in the same order as the v3 series only up to >> what I reviewed. Then do another series with the remainder plus new changes, ok? > > Sure. > > Sören > >> >> Regards, >> Peter Hurley >> >>> Reported-by: Peter Hurley <peter@hurleysoftware.com> >>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> >>> --- >>> v4: >>> - this patch has been added. Thanks to Peter for pointing it out and providing >>> commit message >>> --- >>> drivers/tty/serial/xilinx_uartps.c | 9 ++------- >>> 1 file changed, 2 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c >>> index 6ffd3bbe3e18..1e9053656610 100644 >>> --- a/drivers/tty/serial/xilinx_uartps.c >>> +++ b/drivers/tty/serial/xilinx_uartps.c >>> @@ -759,12 +759,7 @@ static void cdns_uart_set_termios(struct uart_port *port, >>> static int cdns_uart_startup(struct uart_port *port) >>> { >>> unsigned long flags; >>> - unsigned int retval = 0, status = 0; >>> - >>> - retval = request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME, >>> - (void *)port); >>> - if (retval) >>> - return retval; >>> + unsigned int status = 0; >>> >>> spin_lock_irqsave(&port->lock, flags); >>> >>> @@ -818,7 +813,7 @@ static int cdns_uart_startup(struct uart_port *port) >>> >>> spin_unlock_irqrestore(&port->lock, flags); >>> >>> - return retval; >>> + return request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME, port); >>> } >>> >>> /** >>> >>
On Tue, 2015-12-15 at 03:26PM -0800, Peter Hurley wrote: > On 12/15/2015 07:41 AM, Sören Brinkmann wrote: > > On Thu, 2015-12-10 at 01:41PM -0800, Peter Hurley wrote: > >> On 12/05/2015 08:39 PM, Soren Brinkmann wrote: > >>> Request_irq() should be _after_ h/w programming, otherwise an > >>> interrupt could be triggered and in-progress before the h/w has been > >>> setup. > >> > >> Slight misunderstanding. My fault; I should have been more explicit. > >> > >> 1. Any setup necessary for the isr not to be confused and misdirect spurious > >> interrupts (or hang) should be before installing the isr with request_irq() > >> None of this code should trigger an interrupt. > >> 2. Clear pending interrupts > >> 3. Install the isr with request_irq() > >> 4. Enable interrupts > > > > Isn't that what the startup function is doing now - more or less. I > > think 3 and 4 are swapped to release the lock and then do the > > request_irq, but I believe that should be OK. > > The startup function configures the HW. Clears the ISR. Enables the > > intended IRQs and then does the request_irq call. > > If the driver enables interrupts before installing the isr with request_irq() > and an interrupt occurs there will the no handler to catch it and EOI the > device. Really? Shouldn't the IRQ be masked in the interrupt controller until everything is in place? Sören
On 12/16/2015 01:03 AM, Sören Brinkmann wrote: > On Tue, 2015-12-15 at 03:26PM -0800, Peter Hurley wrote: >> On 12/15/2015 07:41 AM, Sören Brinkmann wrote: >>> On Thu, 2015-12-10 at 01:41PM -0800, Peter Hurley wrote: >>>> On 12/05/2015 08:39 PM, Soren Brinkmann wrote: >>>>> Request_irq() should be _after_ h/w programming, otherwise an >>>>> interrupt could be triggered and in-progress before the h/w has been >>>>> setup. >>>> >>>> Slight misunderstanding. My fault; I should have been more explicit. >>>> >>>> 1. Any setup necessary for the isr not to be confused and misdirect spurious >>>> interrupts (or hang) should be before installing the isr with request_irq() >>>> None of this code should trigger an interrupt. >>>> 2. Clear pending interrupts >>>> 3. Install the isr with request_irq() >>>> 4. Enable interrupts >>> >>> Isn't that what the startup function is doing now - more or less. I >>> think 3 and 4 are swapped to release the lock and then do the >>> request_irq, but I believe that should be OK. >>> The startup function configures the HW. Clears the ISR. Enables the >>> intended IRQs and then does the request_irq call. >> >> If the driver enables interrupts before installing the isr with request_irq() >> and an interrupt occurs there will the no handler to catch it and EOI the >> device. > > Really? Shouldn't the IRQ be masked in the interrupt controller until > everything is in place? Sorry, I'm used to shared interrupts, where that isn't the case. Regards, Peter Hurley
On Wed, 2015-12-16 at 06:37AM -0800, Peter Hurley wrote: > On 12/16/2015 01:03 AM, Sören Brinkmann wrote: > > On Tue, 2015-12-15 at 03:26PM -0800, Peter Hurley wrote: > >> On 12/15/2015 07:41 AM, Sören Brinkmann wrote: > >>> On Thu, 2015-12-10 at 01:41PM -0800, Peter Hurley wrote: > >>>> On 12/05/2015 08:39 PM, Soren Brinkmann wrote: > >>>>> Request_irq() should be _after_ h/w programming, otherwise an > >>>>> interrupt could be triggered and in-progress before the h/w has been > >>>>> setup. > >>>> > >>>> Slight misunderstanding. My fault; I should have been more explicit. > >>>> > >>>> 1. Any setup necessary for the isr not to be confused and misdirect spurious > >>>> interrupts (or hang) should be before installing the isr with request_irq() > >>>> None of this code should trigger an interrupt. > >>>> 2. Clear pending interrupts > >>>> 3. Install the isr with request_irq() > >>>> 4. Enable interrupts > >>> > >>> Isn't that what the startup function is doing now - more or less. I > >>> think 3 and 4 are swapped to release the lock and then do the > >>> request_irq, but I believe that should be OK. > >>> The startup function configures the HW. Clears the ISR. Enables the > >>> intended IRQs and then does the request_irq call. > >> > >> If the driver enables interrupts before installing the isr with request_irq() > >> and an interrupt occurs there will the no handler to catch it and EOI the > >> device. > > > > Really? Shouldn't the IRQ be masked in the interrupt controller until > > everything is in place? > > Sorry, I'm used to shared interrupts, where that isn't the case. Ahh, I didn't have to deal with such cases yet. Makes sense though. Thanks, Sören
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c index 6ffd3bbe3e18..1e9053656610 100644 --- a/drivers/tty/serial/xilinx_uartps.c +++ b/drivers/tty/serial/xilinx_uartps.c @@ -759,12 +759,7 @@ static void cdns_uart_set_termios(struct uart_port *port, static int cdns_uart_startup(struct uart_port *port) { unsigned long flags; - unsigned int retval = 0, status = 0; - - retval = request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME, - (void *)port); - if (retval) - return retval; + unsigned int status = 0; spin_lock_irqsave(&port->lock, flags); @@ -818,7 +813,7 @@ static int cdns_uart_startup(struct uart_port *port) spin_unlock_irqrestore(&port->lock, flags); - return retval; + return request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME, port); } /**
Request_irq() should be _after_ h/w programming, otherwise an interrupt could be triggered and in-progress before the h/w has been setup. Reported-by: Peter Hurley <peter@hurleysoftware.com> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> --- v4: - this patch has been added. Thanks to Peter for pointing it out and providing commit message --- drivers/tty/serial/xilinx_uartps.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)