linux-kernel - Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 28 Apr 2020 13:30:25 +0200
From:   Hauke Mehrtens <hauke@...ke-m.de>
To:     Daniel Schwierzeck <daniel.schwierzeck@...il.com>,
        Dilip Kota <eswara.kota@...ux.intel.com>, broonie@...nel.org,
        robh@...nel.org, linux-spi@...r.kernel.org,
        devicetree@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org, andriy.shevchenko@...el.com,
        cheol.yong.kim@...el.com, chuanhua.lei@...ux.intel.com,
        qi-ming.wu@...el.com
Subject: Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and
 transfers

On 4/28/20 1:10 PM, Daniel Schwierzeck wrote:
> 
> 
> Am 24.04.20 um 12:42 schrieb Dilip Kota:
>> Synchronize tx, rx and error interrupts by registering to the
>> same interrupt handler. Interrupt handler will recognize and process
>> the appropriate interrupt on the basis of interrupt status register.
>> Also, establish synchronization between the interrupt handler and
>> transfer operation by taking the locks and registering the interrupt
>> handler as thread IRQ which avoids the bottom half.
> 
> actually there is no real bottom half. Reading or writing the FIFOs is
> fast and is therefore be done in hard IRQ context. But as the comment
> for lantiq_ssc_bussy_work() state, the driver needs some busy-waiting
> after the last interrupt. I don't think it's worth to replace this with
> threaded interrupts which add more runtime overhead and likely decrease
> the maximum transfer speed.
> 
>> Fixes the wrongly populated interrupt register offsets too.
>>
>> Fixes: 17f84b793c01 ("spi: lantiq-ssc: add support for Lantiq SSC SPI controller")
>> Fixes: ad2fca0721d1 ("spi: lantiq-ssc: add LTQ_ prefix to defines")
>> Signed-off-by: Dilip Kota <eswara.kota@...ux.intel.com>
>> ---
>>  drivers/spi/spi-lantiq-ssc.c | 89 ++++++++++++++++++++++----------------------
>>  1 file changed, 45 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/spi/spi-lantiq-ssc.c b/drivers/spi/spi-lantiq-ssc.c
>> index 1fd7ee53d451..b67f5925bcb0 100644
>> --- a/drivers/spi/spi-lantiq-ssc.c
>> +++ b/drivers/spi/spi-lantiq-ssc.c
>> @@ -6,6 +6,7 @@
>>  
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>> +#include <linux/mutex.h>
>>  #include <linux/of_device.h>
>>  #include <linux/clk.h>
>>  #include <linux/io.h>
>> @@ -13,7 +14,6 @@
>>  #include <linux/interrupt.h>
>>  #include <linux/sched.h>
>>  #include <linux/completion.h>
>> -#include <linux/spinlock.h>
>>  #include <linux/err.h>
>>  #include <linux/gpio.h>
>>  #include <linux/pm_runtime.h>
>> @@ -50,8 +50,8 @@
>>  #define LTQ_SPI_RXCNT		0x84
>>  #define LTQ_SPI_DMACON		0xec
>>  #define LTQ_SPI_IRNEN		0xf4
>> -#define LTQ_SPI_IRNICR		0xf8
>> -#define LTQ_SPI_IRNCR		0xfc
>> +#define LTQ_SPI_IRNCR		0xf8
>> +#define LTQ_SPI_IRNICR		0xfc
> 
> the values are matching the datasheets for Danube and VRX200 family.
> AFAICS the registers have been swapped for some newer SoCs like GRX330
> or GRX550. It didn't matter until now because those registers were
> unused by the driver. So if you want to use those registers, you have to
> deal somehow with the register offset swap in struct lantiq_ssc_hwcfg.

Hi,

The Interrupt controller found on Danube till xrx300 which is probably
from Infineon like this SPI controller IP acknowledges the interrupts
also inside this SPI controller IP automatically, this has to be done
manually on the xrx500 and probably also LGM as they use a different
interrupt controller. I prepared patches for this internally 2.5 years
ago but did not send them upstream because of internal processes.

I would suggest to only do this ack on the newer platforms starting with
the xrx500 and not on the older.

On SMP systems a lock is needed in lantiq_ssc_xmit_interrupt() to
protect against an other thread reading from the RX buffer or writing to
the TX buffer in parallel.

@Dilip. Did you try the patches I send you one months ago on the LGM?

I would be helpful to split this patch into multiple like already
suggest to make it easier to find the bugs.

Hauke