[ +cc Felipe, Tony ]
On 08/03/2015 08:18 PM, Charles Manning wrote:
> On Tue, Aug 4, 2015 at 11:56 AM, Greg KH <greg@xxxxxxxxx> wrote:
>> On Tue, Aug 04, 2015 at 10:23:12AM +1200, Charles Manning wrote:
>>> I am debugging an issue on the OMAP where the serial irq processing is
>>> normally low (around 1%) but can sometimes rise to much higher (well
>>> over 10%).
>>>
>>> This lead me to sniff around the locking....
>>>
>>> I notice that in some drivers eg. omap-serial.c the whole interrupt
>>> handling loop is locked via spin_lock().
That looks like a deadlock waiting-to-happen to me. For example,
==> IRQ
serial_omap_irq()
spin_lock(port->lock)
...
==> higher priority IRQ
handler in some other driver
printk()
console_unlock()
call_console_drivers()
serial_omap_console_write()
spin_lock(port->lock)
** DEADLOCK **
Felipe, Tony, this is possible on OMAP, right?
>>> However, other drivers such as serial-tegra.c use spin_lock_irqsave()
>>> for a loop that pretty much has the same form.
>>>
>>> Why is it that they don't use the same locking strategy? What would be better?
>>
>> If something can be accessed from in interrupt, you have to use
>> spin_lock_irqsave everywhere else for that same lock to properly protect
>> the data.
>
> Thanks Greg
>
> So to repeat that back to make sure it was what I understand...
>
> Resource accesses from outside the interrupt must have
> spin_lock_irqsave() (eg. in set_termios or to kick off a tx) to
> prevent the interrupt from occurring while the registers are being
> touched.
Yes, but only for resource or state changes which affect
interrupt processing.
Also, the port->lock protects certain state variables as well.
For example, changes to the mctrl shadow are protected by the port->lock.
Most configuration hardware programming changes are mutually excluded by
the port->mutex (fixed since 3.19). For example, 8250 FCR register changes.
There's still plenty of brokenness though so I wouldn't look at any
particular driver as canonical reference atm. For example, IER register and
shadow register changes should be protected by port->lock in the 8250
driver; otherwise the following is possible:
CPU 0 | CPU 1
serial8250_console_write() | serial8250_do_shutdown()
spin_lock_irqsave(port->lock) |
ier = serial_port_in(UART_IER) |
. | up->ier = 0
. | serial_port_out(UART_IER, 0)
. | ...
serial_port_out(UART_IER, ier) |
** whoops, just re-enabled serial interrupts after shutdown **
> The interrupt itself just needs to do a spin_lock().
IFF:
1. the arch/platform/irqchip doesn't enable/allow nested interrupts, OR
2. the serial driver doesn't support console
>> So it depends on the hardware implementation here, both are probably
>> necessary because of that.
>
> So by using a spin_lock_irqsave() within the interrupt, this is
> preventing another interrupt from occurring.
Yes.
> Is it correct to assume spin_lock_irqsave() is needed when there are
> multiple interrupts for the same driver (eg. dma channels etc,)
Drivers with different tx and rx interrupts (for example) may or may
not require interrupts disabled. Without knowing the specific hardware,
it's safest to assume interrupts should be disabled in the presence of
multiple handlers.
> whereas spin_lock() is fine on simpler drivers with just one interrupt
> source (eg. "dumb" 16550 style drivers).
With the earlier proviso, that either nested interrupts are not possible
or the driver doesn't support console.
In other words, it's really printk()/console support that may force a serial
driver to use spin_lock_irqsave() in its irq handler. Other classes of
drivers don't typically have this additional requirement.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
