imx: Race when disabling RX in IMX.6 UART in half duplex

imx: Race when disabling RX in IMX.6 UART in half duplex

[Piotr Figiel]

Hello,

  I'm looking to find a correct way to disable RX in the I.MX6 UART 
during TX-ing. I stumbled on the issue described below when working on 
~SER_RS485_RX_DURING_TX on custom 3.10 tree (branched from 
Freescale's/NXP's BSP release) independently to what is now in the main 
line imx.c, but I see now that the implementation in mainline also seem 
to have the same problem I'm trying to solve now. I would like to 
request for comments to confirm/deny whether the issue I raise here is 
valid and how to best approach this.

  I'm mostly concerned about the fact that the URXD register must not be 
accessed (as documented in IMX.6 RM) when RX is disabled (RXEN=0). The 
issue is that in the following sequence happening during imx_start_tx 
with ~SER_RS485_RX_DURING_TX set:

1. Acquire spinlock, disable local interrupts (from serial_core.c),
2. Disable RX receiver (RXEN=0 in UCR2),
3. Release spinlock, reenable interrupts.

The RX fifo may become not empty between #1 and #2. This will raise 
interrupt which will be handled after re-enabling interrupts (after #3). 
ISR in this case will check the status bit of the interrupt and fetch RX 
FIFO contents, which I understand is forbidden by the documentation and 
may raise error on the bus [1]. In addition disabling RX IRQ in this 
procedure, e.g. after #1 doesn't seem to be enough, as the IRQ still may 
trigger after #1 and will need to be serviced.

I came to the conclusion that the above problem can be solved by 
adjusting the procedure so that it's:

1. Acquire spinlock, disable local interrupts,
2. Disable RX interrupt,
3. Fetch RX fifo contents,
4. Disable RX receiver,
5. Check if RX fifo is not-empty by use of USR2, if it's not-empty it 
means the characters were not added between steps #3 and #4. If RX FIFO 
is not empty re-enable RX receiver and go to 3. Otherwise continue.
6. Release spinlock, reenable interrupts.

I wonder is this the only way to do that, as it doesn't seem clean. 
Other option would be to check in the ISR whether the RX is enabled 
before getting FIFO contents, but what would be correct behavior if it 
is not empty? (Interrupt would be handled but FIFO contents wouldn't be 
cleaned, when those should be fetched?).

Could you please comment?

Best regards, Piotr.

[1] 
http://www.nxp.com/assets/documents/data/en/reference-manuals/IMX6DQRM.pdf 
(64.15.1)

imx: Race when disabling RX in IMX.6 UART in half duplex

[Piotr Figiel]

Hello,

  I'm looking to find a correct way to disable RX in the I.MX6 UART 
during TX-ing. I stumbled on the issue described below when working on 
~SER_RS485_RX_DURING_TX on custom 3.10 tree (branched from 
Freescale's/NXP's BSP release) independently to what is now in the main 
line imx.c, but I see now that the implementation in mainline also seem 
to have the same problem I'm trying to solve now. I would like to 
request for comments to confirm/deny whether the issue I raise here is 
valid and how to best approach this.

  I'm mostly concerned about the fact that the URXD register must not be 
accessed (as documented in IMX.6 RM) when RX is disabled (RXEN=0). The 
issue is that in the following sequence happening during imx_start_tx 
with ~SER_RS485_RX_DURING_TX set:

1. Acquire spinlock, disable local interrupts (from serial_core.c),
2. Disable RX receiver (RXEN=0 in UCR2),
3. Release spinlock, reenable interrupts.

The RX fifo may become not empty between #1 and #2. This will raise 
interrupt which will be handled after re-enabling interrupts (after #3). 
ISR in this case will check the status bit of the interrupt and fetch RX 
FIFO contents, which I understand is forbidden by the documentation and 
may raise error on the bus [1]. In addition disabling RX IRQ in this 
procedure, e.g. after #1 doesn't seem to be enough, as the IRQ still may 
trigger after #1 and will need to be serviced.

I came to the conclusion that the above problem can be solved by 
adjusting the procedure so that it's:

1. Acquire spinlock, disable local interrupts,
2. Disable RX interrupt,
3. Fetch RX fifo contents,
4. Disable RX receiver,
5. Check if RX fifo is not-empty by use of USR2, if it's not-empty it 
means the characters were not added between steps #3 and #4. If RX FIFO 
is not empty re-enable RX receiver and go to 3. Otherwise continue.
6. Release spinlock, reenable interrupts.

I wonder is this the only way to do that, as it doesn't seem clean. 
Other option would be to check in the ISR whether the RX is enabled 
before getting FIFO contents, but what would be correct behavior if it 
is not empty? (Interrupt would be handled but FIFO contents wouldn't be 
cleaned, when those should be fetched?).

Could you please comment?

Best regards, Piotr.

[1] 
http://www.nxp.com/assets/documents/data/en/reference-manuals/IMX6DQRM.pdf 
(64.15.1)

Re: imx: Race when disabling RX in IMX.6 UART in half duplex

[Baruch Siach]

Hi Piotr,

On Mon, Feb 20, 2017 at 02:54:02PM +0100, Piotr Figiel wrote:
>  I'm looking to find a correct way to disable RX in the I.MX6 UART during
> TX-ing. I stumbled on the issue described below when working on
> ~SER_RS485_RX_DURING_TX on custom 3.10 tree (branched from Freescale's/NXP's
> BSP release) independently to what is now in the main line imx.c, but I see
> now that the implementation in mainline also seem to have the same problem
> I'm trying to solve now. I would like to request for comments to
> confirm/deny whether the issue I raise here is valid and how to best
> approach this.
> 
>  I'm mostly concerned about the fact that the URXD register must not be
> accessed (as documented in IMX.6 RM) when RX is disabled (RXEN=0). The issue
> is that in the following sequence happening during imx_start_tx with
> ~SER_RS485_RX_DURING_TX set:
> 
> 1. Acquire spinlock, disable local interrupts (from serial_core.c),
> 2. Disable RX receiver (RXEN=0 in UCR2),
> 3. Release spinlock, reenable interrupts.
> 
> The RX fifo may become not empty between #1 and #2. This will raise
> interrupt which will be handled after re-enabling interrupts (after #3). ISR
> in this case will check the status bit of the interrupt and fetch RX FIFO
> contents, which I understand is forbidden by the documentation and may raise
> error on the bus [1]. In addition disabling RX IRQ in this procedure, e.g.
> after #1 doesn't seem to be enough, as the IRQ still may trigger after #1
> and will need to be serviced.

As I understand it, SER_RS485_RX_DURING_TX is for half-duplex RS-485 where 
only one device is allowed to transmit at any given time. Higher level code 
determines when any given device on the bus is allowed to transmit. If Rx FIFO 
becomes non-empty during Tx enable, you have a contention problem to solve.

What is your use case?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

imx: Race when disabling RX in IMX.6 UART in half duplex

[Baruch Siach]

Hi Piotr,

On Mon, Feb 20, 2017 at 02:54:02PM +0100, Piotr Figiel wrote:
>  I'm looking to find a correct way to disable RX in the I.MX6 UART during
> TX-ing. I stumbled on the issue described below when working on
> ~SER_RS485_RX_DURING_TX on custom 3.10 tree (branched from Freescale's/NXP's
> BSP release) independently to what is now in the main line imx.c, but I see
> now that the implementation in mainline also seem to have the same problem
> I'm trying to solve now. I would like to request for comments to
> confirm/deny whether the issue I raise here is valid and how to best
> approach this.
> 
>  I'm mostly concerned about the fact that the URXD register must not be
> accessed (as documented in IMX.6 RM) when RX is disabled (RXEN=0). The issue
> is that in the following sequence happening during imx_start_tx with
> ~SER_RS485_RX_DURING_TX set:
> 
> 1. Acquire spinlock, disable local interrupts (from serial_core.c),
> 2. Disable RX receiver (RXEN=0 in UCR2),
> 3. Release spinlock, reenable interrupts.
> 
> The RX fifo may become not empty between #1 and #2. This will raise
> interrupt which will be handled after re-enabling interrupts (after #3). ISR
> in this case will check the status bit of the interrupt and fetch RX FIFO
> contents, which I understand is forbidden by the documentation and may raise
> error on the bus [1]. In addition disabling RX IRQ in this procedure, e.g.
> after #1 doesn't seem to be enough, as the IRQ still may trigger after #1
> and will need to be serviced.

As I understand it, SER_RS485_RX_DURING_TX is for half-duplex RS-485 where 
only one device is allowed to transmit at any given time. Higher level code 
determines when any given device on the bus is allowed to transmit. If Rx FIFO 
becomes non-empty during Tx enable, you have a contention problem to solve.

What is your use case?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: imx: Race when disabling RX in IMX.6 UART in half duplex

[Piotr Figiel]

Hi Baruch,


On 20.02.2017 20:20, Baruch Siach wrote:
> Hi Piotr,
>
> On Mon, Feb 20, 2017 at 02:54:02PM +0100, Piotr Figiel wrote:
>>   I'm looking to find a correct way to disable RX in the I.MX6 UART during
>> TX-ing. I stumbled on the issue described below when working on
>> ~SER_RS485_RX_DURING_TX on custom 3.10 tree (branched from Freescale's/NXP's
>> BSP release) independently to what is now in the main line imx.c, but I see
>> now that the implementation in mainline also seem to have the same problem
>> I'm trying to solve now. I would like to request for comments to
>> confirm/deny whether the issue I raise here is valid and how to best
>> approach this.
>>
>>   I'm mostly concerned about the fact that the URXD register must not be
>> accessed (as documented in IMX.6 RM) when RX is disabled (RXEN=0). The issue
>> is that in the following sequence happening during imx_start_tx with
>> ~SER_RS485_RX_DURING_TX set:
>>
>> 1. Acquire spinlock, disable local interrupts (from serial_core.c),
>> 2. Disable RX receiver (RXEN=0 in UCR2),
>> 3. Release spinlock, reenable interrupts.
>>
>> The RX fifo may become not empty between #1 and #2. This will raise
>> interrupt which will be handled after re-enabling interrupts (after #3). ISR
>> in this case will check the status bit of the interrupt and fetch RX FIFO
>> contents, which I understand is forbidden by the documentation and may raise
>> error on the bus [1]. In addition disabling RX IRQ in this procedure, e.g.
>> after #1 doesn't seem to be enough, as the IRQ still may trigger after #1
>> and will need to be serviced.
> As I understand it, SER_RS485_RX_DURING_TX is for half-duplex RS-485 where
> only one device is allowed to transmit at any given time. Higher level code
> determines when any given device on the bus is allowed to transmit. If Rx FIFO
> becomes non-empty during Tx enable, you have a contention problem to solve.

That's correct, although I don't want bugs from the user-space or 
misbehaving devices on RS-485 to cause errors on the AXI/AHB bus 
resulting with possible crash/exception on the CPU core. The reason I 
write about this is that the RM explicitly forbids accessing those 
registers in such case.

> What is your use case?

My use-case is as you wrote - RS-485 in half duplex. I need to disable 
acquisition of data on RX on UART ip core level when TXing. Upper layer 
protocol in normal conditions does not let the issue I mentioned here 
happen.

Best regards, Piotr.

imx: Race when disabling RX in IMX.6 UART in half duplex

[Piotr Figiel]

Hi Baruch,


On 20.02.2017 20:20, Baruch Siach wrote:
> Hi Piotr,
>
> On Mon, Feb 20, 2017 at 02:54:02PM +0100, Piotr Figiel wrote:
>>   I'm looking to find a correct way to disable RX in the I.MX6 UART during
>> TX-ing. I stumbled on the issue described below when working on
>> ~SER_RS485_RX_DURING_TX on custom 3.10 tree (branched from Freescale's/NXP's
>> BSP release) independently to what is now in the main line imx.c, but I see
>> now that the implementation in mainline also seem to have the same problem
>> I'm trying to solve now. I would like to request for comments to
>> confirm/deny whether the issue I raise here is valid and how to best
>> approach this.
>>
>>   I'm mostly concerned about the fact that the URXD register must not be
>> accessed (as documented in IMX.6 RM) when RX is disabled (RXEN=0). The issue
>> is that in the following sequence happening during imx_start_tx with
>> ~SER_RS485_RX_DURING_TX set:
>>
>> 1. Acquire spinlock, disable local interrupts (from serial_core.c),
>> 2. Disable RX receiver (RXEN=0 in UCR2),
>> 3. Release spinlock, reenable interrupts.
>>
>> The RX fifo may become not empty between #1 and #2. This will raise
>> interrupt which will be handled after re-enabling interrupts (after #3). ISR
>> in this case will check the status bit of the interrupt and fetch RX FIFO
>> contents, which I understand is forbidden by the documentation and may raise
>> error on the bus [1]. In addition disabling RX IRQ in this procedure, e.g.
>> after #1 doesn't seem to be enough, as the IRQ still may trigger after #1
>> and will need to be serviced.
> As I understand it, SER_RS485_RX_DURING_TX is for half-duplex RS-485 where
> only one device is allowed to transmit at any given time. Higher level code
> determines when any given device on the bus is allowed to transmit. If Rx FIFO
> becomes non-empty during Tx enable, you have a contention problem to solve.

That's correct, although I don't want bugs from the user-space or 
misbehaving devices on RS-485 to cause errors on the AXI/AHB bus 
resulting with possible crash/exception on the CPU core. The reason I 
write about this is that the RM explicitly forbids accessing those 
registers in such case.

> What is your use case?

My use-case is as you wrote - RS-485 in half duplex. I need to disable 
acquisition of data on RX on UART ip core level when TXing. Upper layer 
protocol in normal conditions does not let the issue I mentioned here 
happen.

Best regards, Piotr.

Re: imx: Race when disabling RX in IMX.6 UART in half duplex

[Baruch Siach]

Hi Piotr,

On Tue, Feb 21, 2017 at 09:00:39AM +0100, Piotr Figiel wrote:
> On 20.02.2017 20:20, Baruch Siach wrote:
> > On Mon, Feb 20, 2017 at 02:54:02PM +0100, Piotr Figiel wrote:
> > >   I'm looking to find a correct way to disable RX in the I.MX6 UART during
> > > TX-ing. I stumbled on the issue described below when working on
> > > ~SER_RS485_RX_DURING_TX on custom 3.10 tree (branched from Freescale's/NXP's
> > > BSP release) independently to what is now in the main line imx.c, but I see
> > > now that the implementation in mainline also seem to have the same problem
> > > I'm trying to solve now. I would like to request for comments to
> > > confirm/deny whether the issue I raise here is valid and how to best
> > > approach this.
> > > 
> > >   I'm mostly concerned about the fact that the URXD register must not be
> > > accessed (as documented in IMX.6 RM) when RX is disabled (RXEN=0). The issue
> > > is that in the following sequence happening during imx_start_tx with
> > > ~SER_RS485_RX_DURING_TX set:
> > > 
> > > 1. Acquire spinlock, disable local interrupts (from serial_core.c),
> > > 2. Disable RX receiver (RXEN=0 in UCR2),
> > > 3. Release spinlock, reenable interrupts.
> > > 
> > > The RX fifo may become not empty between #1 and #2. This will raise
> > > interrupt which will be handled after re-enabling interrupts (after #3). ISR
> > > in this case will check the status bit of the interrupt and fetch RX FIFO
> > > contents, which I understand is forbidden by the documentation and may raise
> > > error on the bus [1]. In addition disabling RX IRQ in this procedure, e.g.
> > > after #1 doesn't seem to be enough, as the IRQ still may trigger after #1
> > > and will need to be serviced.
> > As I understand it, SER_RS485_RX_DURING_TX is for half-duplex RS-485 where
> > only one device is allowed to transmit at any given time. Higher level code
> > determines when any given device on the bus is allowed to transmit. If Rx FIFO
> > becomes non-empty during Tx enable, you have a contention problem to solve.
> 
> That's correct, although I don't want bugs from the user-space or
> misbehaving devices on RS-485 to cause errors on the AXI/AHB bus resulting
> with possible crash/exception on the CPU core. The reason I write about this
> is that the RM explicitly forbids accessing those registers in such case.

The trivial solution might be to return early from imx_rxint() when RXEN=0. 
Would that be sufficient?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

imx: Race when disabling RX in IMX.6 UART in half duplex

[Baruch Siach]

Hi Piotr,

On Tue, Feb 21, 2017 at 09:00:39AM +0100, Piotr Figiel wrote:
> On 20.02.2017 20:20, Baruch Siach wrote:
> > On Mon, Feb 20, 2017 at 02:54:02PM +0100, Piotr Figiel wrote:
> > >   I'm looking to find a correct way to disable RX in the I.MX6 UART during
> > > TX-ing. I stumbled on the issue described below when working on
> > > ~SER_RS485_RX_DURING_TX on custom 3.10 tree (branched from Freescale's/NXP's
> > > BSP release) independently to what is now in the main line imx.c, but I see
> > > now that the implementation in mainline also seem to have the same problem
> > > I'm trying to solve now. I would like to request for comments to
> > > confirm/deny whether the issue I raise here is valid and how to best
> > > approach this.
> > > 
> > >   I'm mostly concerned about the fact that the URXD register must not be
> > > accessed (as documented in IMX.6 RM) when RX is disabled (RXEN=0). The issue
> > > is that in the following sequence happening during imx_start_tx with
> > > ~SER_RS485_RX_DURING_TX set:
> > > 
> > > 1. Acquire spinlock, disable local interrupts (from serial_core.c),
> > > 2. Disable RX receiver (RXEN=0 in UCR2),
> > > 3. Release spinlock, reenable interrupts.
> > > 
> > > The RX fifo may become not empty between #1 and #2. This will raise
> > > interrupt which will be handled after re-enabling interrupts (after #3). ISR
> > > in this case will check the status bit of the interrupt and fetch RX FIFO
> > > contents, which I understand is forbidden by the documentation and may raise
> > > error on the bus [1]. In addition disabling RX IRQ in this procedure, e.g.
> > > after #1 doesn't seem to be enough, as the IRQ still may trigger after #1
> > > and will need to be serviced.
> > As I understand it, SER_RS485_RX_DURING_TX is for half-duplex RS-485 where
> > only one device is allowed to transmit at any given time. Higher level code
> > determines when any given device on the bus is allowed to transmit. If Rx FIFO
> > becomes non-empty during Tx enable, you have a contention problem to solve.
> 
> That's correct, although I don't want bugs from the user-space or
> misbehaving devices on RS-485 to cause errors on the AXI/AHB bus resulting
> with possible crash/exception on the CPU core. The reason I write about this
> is that the RM explicitly forbids accessing those registers in such case.

The trivial solution might be to return early from imx_rxint() when RXEN=0. 
Would that be sufficient?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: imx: Race when disabling RX in IMX.6 UART in half duplex

[Piotr Figiel]

Hi Baruch,


On 21.02.2017 09:18, Baruch Siach wrote:
> Hi Piotr,
>
> On Tue, Feb 21, 2017 at 09:00:39AM +0100, Piotr Figiel wrote:
>> On 20.02.2017 20:20, Baruch Siach wrote:
>>> On Mon, Feb 20, 2017 at 02:54:02PM +0100, Piotr Figiel wrote:
>>>>    I'm looking to find a correct way to disable RX in the I.MX6 UART during
>>>> TX-ing. I stumbled on the issue described below when working on
>>>> ~SER_RS485_RX_DURING_TX on custom 3.10 tree (branched from Freescale's/NXP's
>>>> BSP release) independently to what is now in the main line imx.c, but I see
>>>> now that the implementation in mainline also seem to have the same problem
>>>> I'm trying to solve now. I would like to request for comments to
>>>> confirm/deny whether the issue I raise here is valid and how to best
>>>> approach this.
>>>>
>>>>    I'm mostly concerned about the fact that the URXD register must not be
>>>> accessed (as documented in IMX.6 RM) when RX is disabled (RXEN=0). The issue
>>>> is that in the following sequence happening during imx_start_tx with
>>>> ~SER_RS485_RX_DURING_TX set:
>>>>
>>>> 1. Acquire spinlock, disable local interrupts (from serial_core.c),
>>>> 2. Disable RX receiver (RXEN=0 in UCR2),
>>>> 3. Release spinlock, reenable interrupts.
>>>>
>>>> The RX fifo may become not empty between #1 and #2. This will raise
>>>> interrupt which will be handled after re-enabling interrupts (after #3). ISR
>>>> in this case will check the status bit of the interrupt and fetch RX FIFO
>>>> contents, which I understand is forbidden by the documentation and may raise
>>>> error on the bus [1]. In addition disabling RX IRQ in this procedure, e.g.
>>>> after #1 doesn't seem to be enough, as the IRQ still may trigger after #1
>>>> and will need to be serviced.
>>> As I understand it, SER_RS485_RX_DURING_TX is for half-duplex RS-485 where
>>> only one device is allowed to transmit at any given time. Higher level code
>>> determines when any given device on the bus is allowed to transmit. If Rx FIFO
>>> becomes non-empty during Tx enable, you have a contention problem to solve.
>> That's correct, although I don't want bugs from the user-space or
>> misbehaving devices on RS-485 to cause errors on the AXI/AHB bus resulting
>> with possible crash/exception on the CPU core. The reason I write about this
>> is that the RM explicitly forbids accessing those registers in such case.
> The trivial solution might be to return early from imx_rxint() when RXEN=0.
> Would that be sufficient?

I'm not sure if it's best idea, I don't know if the UART will keep 
triggering the interrupt if the source won't be cleared (FIFO drained), 
but if it does we'll be irq flooded, if it doesn't there will be old 
contents in the RX fifo no-one will touch until some other interrupt 
(other than TX fifo empty, as at that time RXEN would still be 0) comes. 
RX fifo could be alternatively cleaned somewhere else, this is actually 
question I raise in my initial e-mail.

Best regards, Piotr.

imx: Race when disabling RX in IMX.6 UART in half duplex

[Piotr Figiel]

Hi Baruch,


On 21.02.2017 09:18, Baruch Siach wrote:
> Hi Piotr,
>
> On Tue, Feb 21, 2017 at 09:00:39AM +0100, Piotr Figiel wrote:
>> On 20.02.2017 20:20, Baruch Siach wrote:
>>> On Mon, Feb 20, 2017 at 02:54:02PM +0100, Piotr Figiel wrote:
>>>>    I'm looking to find a correct way to disable RX in the I.MX6 UART during
>>>> TX-ing. I stumbled on the issue described below when working on
>>>> ~SER_RS485_RX_DURING_TX on custom 3.10 tree (branched from Freescale's/NXP's
>>>> BSP release) independently to what is now in the main line imx.c, but I see
>>>> now that the implementation in mainline also seem to have the same problem
>>>> I'm trying to solve now. I would like to request for comments to
>>>> confirm/deny whether the issue I raise here is valid and how to best
>>>> approach this.
>>>>
>>>>    I'm mostly concerned about the fact that the URXD register must not be
>>>> accessed (as documented in IMX.6 RM) when RX is disabled (RXEN=0). The issue
>>>> is that in the following sequence happening during imx_start_tx with
>>>> ~SER_RS485_RX_DURING_TX set:
>>>>
>>>> 1. Acquire spinlock, disable local interrupts (from serial_core.c),
>>>> 2. Disable RX receiver (RXEN=0 in UCR2),
>>>> 3. Release spinlock, reenable interrupts.
>>>>
>>>> The RX fifo may become not empty between #1 and #2. This will raise
>>>> interrupt which will be handled after re-enabling interrupts (after #3). ISR
>>>> in this case will check the status bit of the interrupt and fetch RX FIFO
>>>> contents, which I understand is forbidden by the documentation and may raise
>>>> error on the bus [1]. In addition disabling RX IRQ in this procedure, e.g.
>>>> after #1 doesn't seem to be enough, as the IRQ still may trigger after #1
>>>> and will need to be serviced.
>>> As I understand it, SER_RS485_RX_DURING_TX is for half-duplex RS-485 where
>>> only one device is allowed to transmit at any given time. Higher level code
>>> determines when any given device on the bus is allowed to transmit. If Rx FIFO
>>> becomes non-empty during Tx enable, you have a contention problem to solve.
>> That's correct, although I don't want bugs from the user-space or
>> misbehaving devices on RS-485 to cause errors on the AXI/AHB bus resulting
>> with possible crash/exception on the CPU core. The reason I write about this
>> is that the RM explicitly forbids accessing those registers in such case.
> The trivial solution might be to return early from imx_rxint() when RXEN=0.
> Would that be sufficient?

I'm not sure if it's best idea, I don't know if the UART will keep 
triggering the interrupt if the source won't be cleared (FIFO drained), 
but if it does we'll be irq flooded, if it doesn't there will be old 
contents in the RX fifo no-one will touch until some other interrupt 
(other than TX fifo empty, as at that time RXEN would still be 0) comes. 
RX fifo could be alternatively cleaned somewhere else, this is actually 
question I raise in my initial e-mail.

Best regards, Piotr.

Re: imx: Race when disabling RX in IMX.6 UART in half duplex

[Fabio Estevam]

Hi Piotr,

On Tue, Feb 21, 2017 at 5:48 AM, Piotr Figiel
<p.figiel@camlintechnologies.com> wrote:

> I'm not sure if it's best idea, I don't know if the UART will keep
> triggering the interrupt if the source won't be cleared (FIFO drained), but
> if it does we'll be irq flooded, if it doesn't there will be old contents in
> the RX fifo no-one will touch until some other interrupt (other than TX fifo
> empty, as at that time RXEN would still be 0) comes. RX fifo could be
> alternatively cleaned somewhere else, this is actually question I raise in
> my initial e-mail.

How does the half-duplex RS485 problem manifest in practice?

Do you think the issue you see is similar to what Clemens reported at:
https://lkml.org/lkml/2017/1/4/579 ?

Also, could you try kernel 4.10 instead of 3.10 so that we are all in
the same code base?

Thanks

imx: Race when disabling RX in IMX.6 UART in half duplex

[Fabio Estevam]

Hi Piotr,

On Tue, Feb 21, 2017 at 5:48 AM, Piotr Figiel
<p.figiel@camlintechnologies.com> wrote:

> I'm not sure if it's best idea, I don't know if the UART will keep
> triggering the interrupt if the source won't be cleared (FIFO drained), but
> if it does we'll be irq flooded, if it doesn't there will be old contents in
> the RX fifo no-one will touch until some other interrupt (other than TX fifo
> empty, as at that time RXEN would still be 0) comes. RX fifo could be
> alternatively cleaned somewhere else, this is actually question I raise in
> my initial e-mail.

How does the half-duplex RS485 problem manifest in practice?

Do you think the issue you see is similar to what Clemens reported at:
https://lkml.org/lkml/2017/1/4/579 ?

Also, could you try kernel 4.10 instead of 3.10 so that we are all in
the same code base?

Thanks

Re: imx: Race when disabling RX in IMX.6 UART in half duplex

[Uwe Kleine-König]

Hello,

FTR: Sascha already saw (I think) this problem and already worked on a
fix. He is not in the office today, maybe he can comment tomorrow.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

imx: Race when disabling RX in IMX.6 UART in half duplex

[Uwe Kleine-König]

Hello,

FTR: Sascha already saw (I think) this problem and already worked on a
fix. He is not in the office today, maybe he can comment tomorrow.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: imx: Race when disabling RX in IMX.6 UART in half duplex

[Sascha Hauer]

On Tue, Feb 21, 2017 at 09:48:54AM +0100, Piotr Figiel wrote:
> Hi Baruch,
> 
> 
> On 21.02.2017 09:18, Baruch Siach wrote:
> > Hi Piotr,
> > 
> > On Tue, Feb 21, 2017 at 09:00:39AM +0100, Piotr Figiel wrote:
> > > On 20.02.2017 20:20, Baruch Siach wrote:
> > > > On Mon, Feb 20, 2017 at 02:54:02PM +0100, Piotr Figiel wrote:
> > > > >    I'm looking to find a correct way to disable RX in the I.MX6 UART during
> > > > > TX-ing. I stumbled on the issue described below when working on
> > > > > ~SER_RS485_RX_DURING_TX on custom 3.10 tree (branched from Freescale's/NXP's
> > > > > BSP release) independently to what is now in the main line imx.c, but I see
> > > > > now that the implementation in mainline also seem to have the same problem
> > > > > I'm trying to solve now. I would like to request for comments to
> > > > > confirm/deny whether the issue I raise here is valid and how to best
> > > > > approach this.
> > > > > 
> > > > >    I'm mostly concerned about the fact that the URXD register must not be
> > > > > accessed (as documented in IMX.6 RM) when RX is disabled (RXEN=0). The issue
> > > > > is that in the following sequence happening during imx_start_tx with
> > > > > ~SER_RS485_RX_DURING_TX set:
> > > > > 
> > > > > 1. Acquire spinlock, disable local interrupts (from serial_core.c),
> > > > > 2. Disable RX receiver (RXEN=0 in UCR2),
> > > > > 3. Release spinlock, reenable interrupts.
> > > > > 
> > > > > The RX fifo may become not empty between #1 and #2. This will raise
> > > > > interrupt which will be handled after re-enabling interrupts (after #3). ISR
> > > > > in this case will check the status bit of the interrupt and fetch RX FIFO
> > > > > contents, which I understand is forbidden by the documentation and may raise
> > > > > error on the bus [1]. In addition disabling RX IRQ in this procedure, e.g.
> > > > > after #1 doesn't seem to be enough, as the IRQ still may trigger after #1
> > > > > and will need to be serviced.
> > > > As I understand it, SER_RS485_RX_DURING_TX is for half-duplex RS-485 where
> > > > only one device is allowed to transmit at any given time. Higher level code
> > > > determines when any given device on the bus is allowed to transmit. If Rx FIFO
> > > > becomes non-empty during Tx enable, you have a contention problem to solve.
> > > That's correct, although I don't want bugs from the user-space or
> > > misbehaving devices on RS-485 to cause errors on the AXI/AHB bus resulting
> > > with possible crash/exception on the CPU core. The reason I write about this
> > > is that the RM explicitly forbids accessing those registers in such case.
> > The trivial solution might be to return early from imx_rxint() when RXEN=0.
> > Would that be sufficient?
> 
> I'm not sure if it's best idea, I don't know if the UART will keep
> triggering the interrupt if the source won't be cleared (FIFO drained), but
> if it does we'll be irq flooded,

And indeed that's what happens. The RX irq is acked by reading all
characters from the FIFO. I had the same problem as you describe, see
below for the patch I came up with (with the intention to mainline i
t when I find the time). My "usecase" was to send a character while
being flooded with received characters from the other side. That of
course was only for testing purposes, but I could reliably crash
the kernel with this setup.

Sascha

----------------------8<-------------------------

>From 185781f393e7a38eb96d3af0cd54562481a32222 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Mon, 30 Jan 2017 15:06:20 +0100
Subject: [PATCH] serial: imx: Fix receiver disable

Disabling the receiver just by clearing the UCR2_RXEN is not enough.
If the receiver is disabled while there are characters in the FIFO
then the receive data ready interrupt is triggered (even when
UCR1_RRDYEN is cleared beforehand). This interrupt is normally acked
by reading all characters from the FIFO. However, reading from the FIFO
is only allowed when the Receiver is enabled, otherwise a data abort
is triggered. In the current code this can be triggered by connecting
a board to the i.MX which constantly sends characters. Triggering a
send on the i.MX with "echo huhu > /dev/ttymxcx" then results in a data
abort.

Fix this by safely disabling the receiver. This means that after
disabling the receiver we check if there are characters in the receive
FIFO. If there are, we enable the receiver again and pull the characters
out. We repeat this until we managed to disable the receiver while the
FIFO is empty.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/tty/serial/imx.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 0816afd4a107..eca1c662766e 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -418,6 +418,10 @@ static void imx_stop_tx(struct uart_port *port)
 			temp |= UCR2_RXEN;
 			writel(temp, port->membase + UCR2);
 
+			temp = readl(sport->port.membase + UCR1);
+			temp |= UCR1_RRDYEN;
+			writel(temp, sport->port.membase + UCR1);
+
 			sport->tx_state = OFF;
 		} else if (sport->tx_state == WAIT_AFTER_SEND) {
 			mod_timer(&sport->trigger_stop_tx, sport->tx_state_next_change);
@@ -612,6 +616,38 @@ static void imx_dma_tx(struct imx_port *sport)
 	return;
 }
 
+static void imx_receiver_safe_disable(struct imx_port *sport)
+{
+	u32 ucr2, ucr1;
+	int t = 50;
+
+	ucr1 = readl(sport->port.membase + UCR1);
+	ucr1 &= ~UCR1_RRDYEN;
+	writel(ucr1, sport->port.membase + UCR1);
+
+	ucr2 = readl(sport->port.membase + UCR2);
+
+	/*
+	 * Disable the receiver and make sure that no characters are left
+	 * in the FIFO. Otherwise an interrupt will be triggered even when
+	 * UCR1_RRDYEN is cleared. We can't ack this interrupt because with
+	 * disabled receiver we are not allowed to read from the FIFO.
+	 */
+	do {
+		writel(ucr2 | UCR2_RXEN, sport->port.membase + UCR2);
+
+		while (readl(sport->port.membase + USR2) & USR2_RDR)
+			readl(sport->port.membase + URXD0);
+
+		writel(ucr2 & ~UCR2_RXEN, sport->port.membase + UCR2);
+
+		if (!t--) {
+			dev_err(sport->port.dev, "Failed to disable the receiver\n");
+			return;
+		}
+	} while (readl(sport->port.membase + USR2) & USR2_RDR);
+}
+
 /*
  * interrupts disabled on entry
  */
@@ -633,8 +669,10 @@ static void imx_start_tx(struct uart_port *port)
 				imx_port_rts_active(sport, &temp);
 			else
 				imx_port_rts_inactive(sport, &temp);
-			if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
+			if (!(port->rs485.flags & SER_RS485_RX_DURING_TX)) {
+				imx_receiver_safe_disable(sport);
 				temp &= ~UCR2_RXEN;
+			}
 			writel(temp, port->membase + UCR2);
 			sport->tx_state = WAIT_AFTER_RTS;
 			sport->tx_state_next_change =
-- 
2.11.0


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

imx: Race when disabling RX in IMX.6 UART in half duplex

[Sascha Hauer]

On Tue, Feb 21, 2017 at 09:48:54AM +0100, Piotr Figiel wrote:
> Hi Baruch,
> 
> 
> On 21.02.2017 09:18, Baruch Siach wrote:
> > Hi Piotr,
> > 
> > On Tue, Feb 21, 2017 at 09:00:39AM +0100, Piotr Figiel wrote:
> > > On 20.02.2017 20:20, Baruch Siach wrote:
> > > > On Mon, Feb 20, 2017 at 02:54:02PM +0100, Piotr Figiel wrote:
> > > > >    I'm looking to find a correct way to disable RX in the I.MX6 UART during
> > > > > TX-ing. I stumbled on the issue described below when working on
> > > > > ~SER_RS485_RX_DURING_TX on custom 3.10 tree (branched from Freescale's/NXP's
> > > > > BSP release) independently to what is now in the main line imx.c, but I see
> > > > > now that the implementation in mainline also seem to have the same problem
> > > > > I'm trying to solve now. I would like to request for comments to
> > > > > confirm/deny whether the issue I raise here is valid and how to best
> > > > > approach this.
> > > > > 
> > > > >    I'm mostly concerned about the fact that the URXD register must not be
> > > > > accessed (as documented in IMX.6 RM) when RX is disabled (RXEN=0). The issue
> > > > > is that in the following sequence happening during imx_start_tx with
> > > > > ~SER_RS485_RX_DURING_TX set:
> > > > > 
> > > > > 1. Acquire spinlock, disable local interrupts (from serial_core.c),
> > > > > 2. Disable RX receiver (RXEN=0 in UCR2),
> > > > > 3. Release spinlock, reenable interrupts.
> > > > > 
> > > > > The RX fifo may become not empty between #1 and #2. This will raise
> > > > > interrupt which will be handled after re-enabling interrupts (after #3). ISR
> > > > > in this case will check the status bit of the interrupt and fetch RX FIFO
> > > > > contents, which I understand is forbidden by the documentation and may raise
> > > > > error on the bus [1]. In addition disabling RX IRQ in this procedure, e.g.
> > > > > after #1 doesn't seem to be enough, as the IRQ still may trigger after #1
> > > > > and will need to be serviced.
> > > > As I understand it, SER_RS485_RX_DURING_TX is for half-duplex RS-485 where
> > > > only one device is allowed to transmit at any given time. Higher level code
> > > > determines when any given device on the bus is allowed to transmit. If Rx FIFO
> > > > becomes non-empty during Tx enable, you have a contention problem to solve.
> > > That's correct, although I don't want bugs from the user-space or
> > > misbehaving devices on RS-485 to cause errors on the AXI/AHB bus resulting
> > > with possible crash/exception on the CPU core. The reason I write about this
> > > is that the RM explicitly forbids accessing those registers in such case.
> > The trivial solution might be to return early from imx_rxint() when RXEN=0.
> > Would that be sufficient?
> 
> I'm not sure if it's best idea, I don't know if the UART will keep
> triggering the interrupt if the source won't be cleared (FIFO drained), but
> if it does we'll be irq flooded,

And indeed that's what happens. The RX irq is acked by reading all
characters from the FIFO. I had the same problem as you describe, see
below for the patch I came up with (with the intention to mainline i
t when I find the time). My "usecase" was to send a character while
being flooded with received characters from the other side. That of
course was only for testing purposes, but I could reliably crash
the kernel with this setup.

Sascha

----------------------8<-------------------------

>From 185781f393e7a38eb96d3af0cd54562481a32222 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Mon, 30 Jan 2017 15:06:20 +0100
Subject: [PATCH] serial: imx: Fix receiver disable

Disabling the receiver just by clearing the UCR2_RXEN is not enough.
If the receiver is disabled while there are characters in the FIFO
then the receive data ready interrupt is triggered (even when
UCR1_RRDYEN is cleared beforehand). This interrupt is normally acked
by reading all characters from the FIFO. However, reading from the FIFO
is only allowed when the Receiver is enabled, otherwise a data abort
is triggered. In the current code this can be triggered by connecting
a board to the i.MX which constantly sends characters. Triggering a
send on the i.MX with "echo huhu > /dev/ttymxcx" then results in a data
abort.

Fix this by safely disabling the receiver. This means that after
disabling the receiver we check if there are characters in the receive
FIFO. If there are, we enable the receiver again and pull the characters
out. We repeat this until we managed to disable the receiver while the
FIFO is empty.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/tty/serial/imx.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 0816afd4a107..eca1c662766e 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -418,6 +418,10 @@ static void imx_stop_tx(struct uart_port *port)
 			temp |= UCR2_RXEN;
 			writel(temp, port->membase + UCR2);
 
+			temp = readl(sport->port.membase + UCR1);
+			temp |= UCR1_RRDYEN;
+			writel(temp, sport->port.membase + UCR1);
+
 			sport->tx_state = OFF;
 		} else if (sport->tx_state == WAIT_AFTER_SEND) {
 			mod_timer(&sport->trigger_stop_tx, sport->tx_state_next_change);
@@ -612,6 +616,38 @@ static void imx_dma_tx(struct imx_port *sport)
 	return;
 }
 
+static void imx_receiver_safe_disable(struct imx_port *sport)
+{
+	u32 ucr2, ucr1;
+	int t = 50;
+
+	ucr1 = readl(sport->port.membase + UCR1);
+	ucr1 &= ~UCR1_RRDYEN;
+	writel(ucr1, sport->port.membase + UCR1);
+
+	ucr2 = readl(sport->port.membase + UCR2);
+
+	/*
+	 * Disable the receiver and make sure that no characters are left
+	 * in the FIFO. Otherwise an interrupt will be triggered even when
+	 * UCR1_RRDYEN is cleared. We can't ack this interrupt because with
+	 * disabled receiver we are not allowed to read from the FIFO.
+	 */
+	do {
+		writel(ucr2 | UCR2_RXEN, sport->port.membase + UCR2);
+
+		while (readl(sport->port.membase + USR2) & USR2_RDR)
+			readl(sport->port.membase + URXD0);
+
+		writel(ucr2 & ~UCR2_RXEN, sport->port.membase + UCR2);
+
+		if (!t--) {
+			dev_err(sport->port.dev, "Failed to disable the receiver\n");
+			return;
+		}
+	} while (readl(sport->port.membase + USR2) & USR2_RDR);
+}
+
 /*
  * interrupts disabled on entry
  */
@@ -633,8 +669,10 @@ static void imx_start_tx(struct uart_port *port)
 				imx_port_rts_active(sport, &temp);
 			else
 				imx_port_rts_inactive(sport, &temp);
-			if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
+			if (!(port->rs485.flags & SER_RS485_RX_DURING_TX)) {
+				imx_receiver_safe_disable(sport);
 				temp &= ~UCR2_RXEN;
+			}
 			writel(temp, port->membase + UCR2);
 			sport->tx_state = WAIT_AFTER_RTS;
 			sport->tx_state_next_change =
-- 
2.11.0


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: imx: Race when disabling RX in IMX.6 UART in half duplex

[Piotr Figiel]

Hi,


On 22.02.2017 11:44, Sascha Hauer wrote:
> On Tue, Feb 21, 2017 at 09:48:54AM +0100, Piotr Figiel wrote:
>> Hi Baruch,
>>
>>
>> On 21.02.2017 09:18, Baruch Siach wrote:
>>> Hi Piotr,
>>>
>>> On Tue, Feb 21, 2017 at 09:00:39AM +0100, Piotr Figiel wrote:
>>>> On 20.02.2017 20:20, Baruch Siach wrote:
>>>>> On Mon, Feb 20, 2017 at 02:54:02PM +0100, Piotr Figiel wrote:
>>>>>>     I'm looking to find a correct way to disable RX in the I.MX6 UART during
>>>>>> TX-ing. I stumbled on the issue described below when working on
>>>>>> ~SER_RS485_RX_DURING_TX on custom 3.10 tree (branched from Freescale's/NXP's
>>>>>> BSP release) independently to what is now in the main line imx.c, but I see
>>>>>> now that the implementation in mainline also seem to have the same problem
>>>>>> I'm trying to solve now. I would like to request for comments to
>>>>>> confirm/deny whether the issue I raise here is valid and how to best
>>>>>> approach this.
>>>>>>
>>>>>>     I'm mostly concerned about the fact that the URXD register must not be
>>>>>> accessed (as documented in IMX.6 RM) when RX is disabled (RXEN=0). The issue
>>>>>> is that in the following sequence happening during imx_start_tx with
>>>>>> ~SER_RS485_RX_DURING_TX set:
>>>>>>
>>>>>> 1. Acquire spinlock, disable local interrupts (from serial_core.c),
>>>>>> 2. Disable RX receiver (RXEN=0 in UCR2),
>>>>>> 3. Release spinlock, reenable interrupts.
>>>>>>
>>>>>> The RX fifo may become not empty between #1 and #2. This will raise
>>>>>> interrupt which will be handled after re-enabling interrupts (after #3). ISR
>>>>>> in this case will check the status bit of the interrupt and fetch RX FIFO
>>>>>> contents, which I understand is forbidden by the documentation and may raise
>>>>>> error on the bus [1]. In addition disabling RX IRQ in this procedure, e.g.
>>>>>> after #1 doesn't seem to be enough, as the IRQ still may trigger after #1
>>>>>> and will need to be serviced.
>>>>> As I understand it, SER_RS485_RX_DURING_TX is for half-duplex RS-485 where
>>>>> only one device is allowed to transmit at any given time. Higher level code
>>>>> determines when any given device on the bus is allowed to transmit. If Rx FIFO
>>>>> becomes non-empty during Tx enable, you have a contention problem to solve.
>>>> That's correct, although I don't want bugs from the user-space or
>>>> misbehaving devices on RS-485 to cause errors on the AXI/AHB bus resulting
>>>> with possible crash/exception on the CPU core. The reason I write about this
>>>> is that the RM explicitly forbids accessing those registers in such case.
>>> The trivial solution might be to return early from imx_rxint() when RXEN=0.
>>> Would that be sufficient?
>> I'm not sure if it's best idea, I don't know if the UART will keep
>> triggering the interrupt if the source won't be cleared (FIFO drained), but
>> if it does we'll be irq flooded,
> And indeed that's what happens. The RX irq is acked by reading all
> characters from the FIFO. I had the same problem as you describe, see
> below for the patch I came up with (with the intention to mainline i
> t when I find the time). My "usecase" was to send a character while
> being flooded with received characters from the other side. That of
> course was only for testing purposes, but I could reliably crash
> the kernel with this setup.

Thanks for confirming this, this probably requires CVE, since user with 
tty access can crash mainline kernel by correctly timing a write 
(personally though I wasn't able to trigger this without putting delay 
in the code to trigger the race). Some comments regarding the 
implementation (thanks for sharing this btw):

> +static void imx_receiver_safe_disable(struct imx_port *sport)
> +{
> +	u32 ucr2, ucr1;
> +	int t = 50;
> +
> +	ucr1 = readl(sport->port.membase + UCR1);
> +	ucr1 &= ~UCR1_RRDYEN;
> +	writel(ucr1, sport->port.membase + UCR1);
> +
> +	ucr2 = readl(sport->port.membase + UCR2);
> +
> +	/*
> +	 * Disable the receiver and make sure that no characters are left
> +	 * in the FIFO. Otherwise an interrupt will be triggered even when
> +	 * UCR1_RRDYEN is cleared. We can't ack this interrupt because with
> +	 * disabled receiver we are not allowed to read from the FIFO.
> +	 */
> +	do {
> +		writel(ucr2 | UCR2_RXEN, sport->port.membase + UCR2);
> +
> +		while (readl(sport->port.membase + USR2) & USR2_RDR)
> +			readl(sport->port.membase + URXD0);
> +
> +		writel(ucr2 & ~UCR2_RXEN, sport->port.membase + UCR2);
> +
> +		if (!t--) {
> +			dev_err(sport->port.dev, "Failed to disable the receiver\n");
> +			return;
In this unlikely event perhaps an error should be returned so that the 
RXEN is not disabled in imx_start_tx() (i.e. I think it's better to 
ignore ~SER_RS485_RX_DURING_TX for a while than to crash).

> +		}
> +	} while (readl(sport->port.membase + USR2) & USR2_RDR);
> +}
> +
>   /*
>    * interrupts disabled on entry
>    */
> @@ -633,8 +669,10 @@ static void imx_start_tx(struct uart_port *port)
>   				imx_port_rts_active(sport, &temp);
>   			else
>   				imx_port_rts_inactive(sport, &temp);
> -			if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
> +			if (!(port->rs485.flags & SER_RS485_RX_DURING_TX)) {
> +				imx_receiver_safe_disable(sport);
>   				temp &= ~UCR2_RXEN;
> +			}
>   			writel(temp, port->membase + UCR2);
>   			sport->tx_state = WAIT_AFTER_RTS;
>   			sport->tx_state_next_change =

Other than that I think it's OK (it's also consistent with the procedure 
from my first post [1]).

Best regards, Piotr.

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/488957.html

imx: Race when disabling RX in IMX.6 UART in half duplex

[Piotr Figiel]

Hi,


On 22.02.2017 11:44, Sascha Hauer wrote:
> On Tue, Feb 21, 2017 at 09:48:54AM +0100, Piotr Figiel wrote:
>> Hi Baruch,
>>
>>
>> On 21.02.2017 09:18, Baruch Siach wrote:
>>> Hi Piotr,
>>>
>>> On Tue, Feb 21, 2017 at 09:00:39AM +0100, Piotr Figiel wrote:
>>>> On 20.02.2017 20:20, Baruch Siach wrote:
>>>>> On Mon, Feb 20, 2017 at 02:54:02PM +0100, Piotr Figiel wrote:
>>>>>>     I'm looking to find a correct way to disable RX in the I.MX6 UART during
>>>>>> TX-ing. I stumbled on the issue described below when working on
>>>>>> ~SER_RS485_RX_DURING_TX on custom 3.10 tree (branched from Freescale's/NXP's
>>>>>> BSP release) independently to what is now in the main line imx.c, but I see
>>>>>> now that the implementation in mainline also seem to have the same problem
>>>>>> I'm trying to solve now. I would like to request for comments to
>>>>>> confirm/deny whether the issue I raise here is valid and how to best
>>>>>> approach this.
>>>>>>
>>>>>>     I'm mostly concerned about the fact that the URXD register must not be
>>>>>> accessed (as documented in IMX.6 RM) when RX is disabled (RXEN=0). The issue
>>>>>> is that in the following sequence happening during imx_start_tx with
>>>>>> ~SER_RS485_RX_DURING_TX set:
>>>>>>
>>>>>> 1. Acquire spinlock, disable local interrupts (from serial_core.c),
>>>>>> 2. Disable RX receiver (RXEN=0 in UCR2),
>>>>>> 3. Release spinlock, reenable interrupts.
>>>>>>
>>>>>> The RX fifo may become not empty between #1 and #2. This will raise
>>>>>> interrupt which will be handled after re-enabling interrupts (after #3). ISR
>>>>>> in this case will check the status bit of the interrupt and fetch RX FIFO
>>>>>> contents, which I understand is forbidden by the documentation and may raise
>>>>>> error on the bus [1]. In addition disabling RX IRQ in this procedure, e.g.
>>>>>> after #1 doesn't seem to be enough, as the IRQ still may trigger after #1
>>>>>> and will need to be serviced.
>>>>> As I understand it, SER_RS485_RX_DURING_TX is for half-duplex RS-485 where
>>>>> only one device is allowed to transmit at any given time. Higher level code
>>>>> determines when any given device on the bus is allowed to transmit. If Rx FIFO
>>>>> becomes non-empty during Tx enable, you have a contention problem to solve.
>>>> That's correct, although I don't want bugs from the user-space or
>>>> misbehaving devices on RS-485 to cause errors on the AXI/AHB bus resulting
>>>> with possible crash/exception on the CPU core. The reason I write about this
>>>> is that the RM explicitly forbids accessing those registers in such case.
>>> The trivial solution might be to return early from imx_rxint() when RXEN=0.
>>> Would that be sufficient?
>> I'm not sure if it's best idea, I don't know if the UART will keep
>> triggering the interrupt if the source won't be cleared (FIFO drained), but
>> if it does we'll be irq flooded,
> And indeed that's what happens. The RX irq is acked by reading all
> characters from the FIFO. I had the same problem as you describe, see
> below for the patch I came up with (with the intention to mainline i
> t when I find the time). My "usecase" was to send a character while
> being flooded with received characters from the other side. That of
> course was only for testing purposes, but I could reliably crash
> the kernel with this setup.

Thanks for confirming this, this probably requires CVE, since user with 
tty access can crash mainline kernel by correctly timing a write 
(personally though I wasn't able to trigger this without putting delay 
in the code to trigger the race). Some comments regarding the 
implementation (thanks for sharing this btw):

> +static void imx_receiver_safe_disable(struct imx_port *sport)
> +{
> +	u32 ucr2, ucr1;
> +	int t = 50;
> +
> +	ucr1 = readl(sport->port.membase + UCR1);
> +	ucr1 &= ~UCR1_RRDYEN;
> +	writel(ucr1, sport->port.membase + UCR1);
> +
> +	ucr2 = readl(sport->port.membase + UCR2);
> +
> +	/*
> +	 * Disable the receiver and make sure that no characters are left
> +	 * in the FIFO. Otherwise an interrupt will be triggered even when
> +	 * UCR1_RRDYEN is cleared. We can't ack this interrupt because with
> +	 * disabled receiver we are not allowed to read from the FIFO.
> +	 */
> +	do {
> +		writel(ucr2 | UCR2_RXEN, sport->port.membase + UCR2);
> +
> +		while (readl(sport->port.membase + USR2) & USR2_RDR)
> +			readl(sport->port.membase + URXD0);
> +
> +		writel(ucr2 & ~UCR2_RXEN, sport->port.membase + UCR2);
> +
> +		if (!t--) {
> +			dev_err(sport->port.dev, "Failed to disable the receiver\n");
> +			return;
In this unlikely event perhaps an error should be returned so that the 
RXEN is not disabled in imx_start_tx() (i.e. I think it's better to 
ignore ~SER_RS485_RX_DURING_TX for a while than to crash).

> +		}
> +	} while (readl(sport->port.membase + USR2) & USR2_RDR);
> +}
> +
>   /*
>    * interrupts disabled on entry
>    */
> @@ -633,8 +669,10 @@ static void imx_start_tx(struct uart_port *port)
>   				imx_port_rts_active(sport, &temp);
>   			else
>   				imx_port_rts_inactive(sport, &temp);
> -			if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
> +			if (!(port->rs485.flags & SER_RS485_RX_DURING_TX)) {
> +				imx_receiver_safe_disable(sport);
>   				temp &= ~UCR2_RXEN;
> +			}
>   			writel(temp, port->membase + UCR2);
>   			sport->tx_state = WAIT_AFTER_RTS;
>   			sport->tx_state_next_change =

Other than that I think it's OK (it's also consistent with the procedure 
from my first post [1]).

Best regards, Piotr.

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/488957.html