Hi Timur,
On 25/09/15 00:11, Timur Tabi wrote:
> On Thu, May 21, 2015 at 11:26 AM, Andre Przywara <andre.przywara@xxxxxxx> wrote:
>> +static void pl011_enable_interrupts(struct uart_amba_port *uap)
>> +{
>> + spin_lock_irq(&uap->port.lock);
>> +
>> + /* Clear out any spuriously appearing RX interrupts */
>> + writew(UART011_RTIS | UART011_RXIS,
>> + uap->port.membase + UART011_ICR);
>> + uap->im = UART011_RTIM;
>> + if (!pl011_dma_rx_running(uap))
>> + uap->im |= UART011_RXIM;
>> + writew(uap->im, uap->port.membase + UART011_IMSC);
>> + spin_unlock_irq(&uap->port.lock);
>> +}
>
> Shouldn't this function be using spin_lock_irqsave() and
> spin_unlock_irqrestore()? If interrupts are generally disabled before
> calling this function, then they will be enabled by the
> spin_unlock_irq() call, and I don't think we want that. This function
> is only supposed to enable pl011 interrupts, not all interrupts.
Are you seeing an actual issue with this? Does changing it fix anything?
Looking at the history I see that these locks predate git history.
If I get this correctly, going from spin_{un,}lock_irq to the _irqsave
variants should always be safe, but I'd like to hear more opinions on this.
Cheers,
Andre.
--
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
