[2/8] spi: sh-msiof: Fix DMA transfer size check

Commit Message

Dirk Behme Sept. 6, 2017, 7:05 a.m. UTC

From: Hiromitsu Yamasaki <[e-mail]>

DMA supports 32-bit words only,
even if BITLEN1 of SITMDR2 register is 16bit.

Signed-off-by: Hiromitsu Yamasaki <[e-mail]>
Signed-off-by: Dirk Behme <[e-mail]>
---
 drivers/spi/spi-sh-msiof.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Geert Uytterhoeven Sept. 7, 2017, 8:31 a.m. UTC | #1

Hi Dirk,

On Wed, Sep 6, 2017 at 9:05 AM, Dirk Behme <[e-mail]> wrote:
> From: Hiromitsu Yamasaki <[e-mail]>
>
> DMA supports 32-bit words only,
> even if BITLEN1 of SITMDR2 register is 16bit.
>
> Signed-off-by: Hiromitsu Yamasaki <[e-mail]>
> Signed-off-by: Dirk Behme <[e-mail]>
> ---
>  drivers/spi/spi-sh-msiof.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
> index 2b4d3a520176..f9300fdf41e5 100644
> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
> @@ -904,7 +904,7 @@ static int sh_msiof_transfer_one(struct spi_master *master,
>                                 break;
>                         copy32 = copy_bswap32;
>                 } else if (bits <= 16) {
> -                       if (l & 1)
> +                       if (l & 3)
>                                 break;
>                         copy32 = copy_wswap32;
>                 } else {

Looks OK, as l is in bytes, not 16-bit words.

Reviewed-by: Geert Uytterhoeven <[e-mail]>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Dirk Behme Sept. 7, 2017, 8:33 a.m. UTC | #2

On 07.09.2017 10:31, Geert Uytterhoeven wrote:
> Hi Dirk,
> 
> On Wed, Sep 6, 2017 at 9:05 AM, Dirk Behme <[e-mail]> wrote:
>> From: Hiromitsu Yamasaki <[e-mail]>
>>
>> DMA supports 32-bit words only,
>> even if BITLEN1 of SITMDR2 register is 16bit.
>>
>> Signed-off-by: Hiromitsu Yamasaki <[e-mail]>
>> Signed-off-by: Dirk Behme <[e-mail]>
>> ---
>>   drivers/spi/spi-sh-msiof.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
>> index 2b4d3a520176..f9300fdf41e5 100644
>> --- a/drivers/spi/spi-sh-msiof.c
>> +++ b/drivers/spi/spi-sh-msiof.c
>> @@ -904,7 +904,7 @@ static int sh_msiof_transfer_one(struct spi_master *master,
>>                                  break;
>>                          copy32 = copy_bswap32;
>>                  } else if (bits <= 16) {
>> -                       if (l & 1)
>> +                       if (l & 3)
>>                                  break;
>>                          copy32 = copy_wswap32;
>>                  } else {
> 
> Looks OK, as l is in bytes, not 16-bit words.
> 
> Reviewed-by: Geert Uytterhoeven <[e-mail]>


What do you think about CCing this to -stable?

Best regards

Dirk

Geert Uytterhoeven Sept. 7, 2017, 8:39 a.m. UTC | #3

Hi Dirk,

On Thu, Sep 7, 2017 at 10:33 AM, Dirk Behme <[e-mail]> wrote:
> On 07.09.2017 10:31, Geert Uytterhoeven wrote:
>> On Wed, Sep 6, 2017 at 9:05 AM, Dirk Behme <[e-mail]>
>> wrote:
>>> From: Hiromitsu Yamasaki <[e-mail]>
>>>
>>> DMA supports 32-bit words only,
>>> even if BITLEN1 of SITMDR2 register is 16bit.
>>>
>>> Signed-off-by: Hiromitsu Yamasaki <[e-mail]>
>>> Signed-off-by: Dirk Behme <[e-mail]>
>>> ---
>>>   drivers/spi/spi-sh-msiof.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
>>> index 2b4d3a520176..f9300fdf41e5 100644
>>> --- a/drivers/spi/spi-sh-msiof.c
>>> +++ b/drivers/spi/spi-sh-msiof.c
>>> @@ -904,7 +904,7 @@ static int sh_msiof_transfer_one(struct spi_master
>>> *master,
>>>                                  break;
>>>                          copy32 = copy_bswap32;
>>>                  } else if (bits <= 16) {
>>> -                       if (l & 1)
>>> +                       if (l & 3)
>>>                                  break;
>>>                          copy32 = copy_wswap32;
>>>                  } else {
>>
>>
>> Looks OK, as l is in bytes, not 16-bit words.
>>
>> Reviewed-by: Geert Uytterhoeven <[e-mail]>
>
> What do you think about CCing this to -stable?

Sounds OK. Have you tested this?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Dirk Behme Sept. 7, 2017, 8:42 a.m. UTC | #4

On 07.09.2017 10:39, Geert Uytterhoeven wrote:
> Hi Dirk,
> 
> On Thu, Sep 7, 2017 at 10:33 AM, Dirk Behme <[e-mail]> wrote:
>> On 07.09.2017 10:31, Geert Uytterhoeven wrote:
>>> On Wed, Sep 6, 2017 at 9:05 AM, Dirk Behme <[e-mail]>
>>> wrote:
>>>> From: Hiromitsu Yamasaki <[e-mail]>
>>>>
>>>> DMA supports 32-bit words only,
>>>> even if BITLEN1 of SITMDR2 register is 16bit.
>>>>
>>>> Signed-off-by: Hiromitsu Yamasaki <[e-mail]>
>>>> Signed-off-by: Dirk Behme <[e-mail]>
>>>> ---
>>>>    drivers/spi/spi-sh-msiof.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
>>>> index 2b4d3a520176..f9300fdf41e5 100644
>>>> --- a/drivers/spi/spi-sh-msiof.c
>>>> +++ b/drivers/spi/spi-sh-msiof.c
>>>> @@ -904,7 +904,7 @@ static int sh_msiof_transfer_one(struct spi_master
>>>> *master,
>>>>                                   break;
>>>>                           copy32 = copy_bswap32;
>>>>                   } else if (bits <= 16) {
>>>> -                       if (l & 1)
>>>> +                       if (l & 3)
>>>>                                   break;
>>>>                           copy32 = copy_wswap32;
>>>>                   } else {
>>>
>>>
>>> Looks OK, as l is in bytes, not 16-bit words.
>>>
>>> Reviewed-by: Geert Uytterhoeven <[e-mail]>
>>
>> What do you think about CCing this to -stable?
> 
> Sounds OK. Have you tested this?


I tested all 8 patches together if they fix the issues I observed with 
plain mainline.

Best regards

Dirk

Geert Uytterhoeven Sept. 7, 2017, 8:59 a.m. UTC | #5

Hi Dirk,

On Thu, Sep 7, 2017 at 10:42 AM, Dirk Behme <[e-mail]> wrote:
> On 07.09.2017 10:39, Geert Uytterhoeven wrote:
>> On Thu, Sep 7, 2017 at 10:33 AM, Dirk Behme <[e-mail]>
>> wrote:
>>> On 07.09.2017 10:31, Geert Uytterhoeven wrote:
>>>> On Wed, Sep 6, 2017 at 9:05 AM, Dirk Behme <[e-mail]>
>>>> wrote:
>>>>> From: Hiromitsu Yamasaki <[e-mail]>
>>>>>
>>>>> DMA supports 32-bit words only,
>>>>> even if BITLEN1 of SITMDR2 register is 16bit.
>>>>>
>>>>> Signed-off-by: Hiromitsu Yamasaki <[e-mail]>
>>>>> Signed-off-by: Dirk Behme <[e-mail]>
>>>>> ---
>>>>>    drivers/spi/spi-sh-msiof.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
>>>>> index 2b4d3a520176..f9300fdf41e5 100644
>>>>> --- a/drivers/spi/spi-sh-msiof.c
>>>>> +++ b/drivers/spi/spi-sh-msiof.c
>>>>> @@ -904,7 +904,7 @@ static int sh_msiof_transfer_one(struct spi_master
>>>>> *master,
>>>>>                                   break;
>>>>>                           copy32 = copy_bswap32;
>>>>>                   } else if (bits <= 16) {
>>>>> -                       if (l & 1)
>>>>> +                       if (l & 3)
>>>>>                                   break;
>>>>>                           copy32 = copy_wswap32;
>>>>>                   } else {
>>>>
>>>>
>>>>
>>>> Looks OK, as l is in bytes, not 16-bit words.
>>>>
>>>> Reviewed-by: Geert Uytterhoeven <[e-mail]>
>>>
>>> What do you think about CCing this to -stable?
>>
>> Sounds OK. Have you tested this?
>
> I tested all 8 patches together if they fix the issues I observed with plain
> mainline.

Could you please elaborate about the issues you observed with plain mainline?
It may help to identify which patches are responsible for fixing them.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Dirk Behme Sept. 7, 2017, 9:05 a.m. UTC | #6

On 07.09.2017 10:59, Geert Uytterhoeven wrote:
> Hi Dirk,
> 
> On Thu, Sep 7, 2017 at 10:42 AM, Dirk Behme <[e-mail]> wrote:
>> On 07.09.2017 10:39, Geert Uytterhoeven wrote:
>>> On Thu, Sep 7, 2017 at 10:33 AM, Dirk Behme <[e-mail]>
>>> wrote:
>>>> On 07.09.2017 10:31, Geert Uytterhoeven wrote:
>>>>> On Wed, Sep 6, 2017 at 9:05 AM, Dirk Behme <[e-mail]>
>>>>> wrote:
>>>>>> From: Hiromitsu Yamasaki <[e-mail]>
>>>>>>
>>>>>> DMA supports 32-bit words only,
>>>>>> even if BITLEN1 of SITMDR2 register is 16bit.
>>>>>>
>>>>>> Signed-off-by: Hiromitsu Yamasaki <[e-mail]>
>>>>>> Signed-off-by: Dirk Behme <[e-mail]>
>>>>>> ---
>>>>>>     drivers/spi/spi-sh-msiof.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
>>>>>> index 2b4d3a520176..f9300fdf41e5 100644
>>>>>> --- a/drivers/spi/spi-sh-msiof.c
>>>>>> +++ b/drivers/spi/spi-sh-msiof.c
>>>>>> @@ -904,7 +904,7 @@ static int sh_msiof_transfer_one(struct spi_master
>>>>>> *master,
>>>>>>                                    break;
>>>>>>                            copy32 = copy_bswap32;
>>>>>>                    } else if (bits <= 16) {
>>>>>> -                       if (l & 1)
>>>>>> +                       if (l & 3)
>>>>>>                                    break;
>>>>>>                            copy32 = copy_wswap32;
>>>>>>                    } else {
>>>>>
>>>>>
>>>>>
>>>>> Looks OK, as l is in bytes, not 16-bit words.
>>>>>
>>>>> Reviewed-by: Geert Uytterhoeven <[e-mail]>
>>>>
>>>> What do you think about CCing this to -stable?
>>>
>>> Sounds OK. Have you tested this?
>>
>> I tested all 8 patches together if they fix the issues I observed with plain
>> mainline.
> 
> Could you please elaborate about the issues you observed with plain mainline?
> It may help to identify which patches are responsible for fixing them.


I've been told that an easy test case is to just cat random data (any 
mid sized file) into SPI. E.g.:

# cat /proc/cpuinfo > /dev/spidev32764.2
[  504.544948] spi_sh_msiof e6e90000.spi: failed to shut down hardware
[  504.551265] spidev spi32764.2: SPI transfer failed: -110
[  504.556625] spi_master spi32764: failed to transfer one message from 
queue
[  504.564857] spi_sh_msiof e6e90000.spi: failed to shut down hardware
[  504.571177] spidev spi32764.2: SPI transfer failed: -110
[  504.576528] spi_master spi32764: failed to transfer one message from 
queue
cat: write error: Connection timed out

done on plain 4.13-rc6.

Best regards

Dirk

Geert Uytterhoeven Sept. 7, 2017, 9:12 a.m. UTC | #7

Hi Dirk,

On Thu, Sep 7, 2017 at 11:05 AM, Dirk Behme <[e-mail]> wrote:
> On 07.09.2017 10:59, Geert Uytterhoeven wrote:
>> On Thu, Sep 7, 2017 at 10:42 AM, Dirk Behme <[e-mail]>
>> wrote:
>>> On 07.09.2017 10:39, Geert Uytterhoeven wrote:
>>>> On Thu, Sep 7, 2017 at 10:33 AM, Dirk Behme <[e-mail]>
>>>> wrote:
>>>>> On 07.09.2017 10:31, Geert Uytterhoeven wrote:
>>>>>> On Wed, Sep 6, 2017 at 9:05 AM, Dirk Behme <[e-mail]>
>>>>>> wrote:
>>>>>>> From: Hiromitsu Yamasaki <[e-mail]>
>>>>>>> DMA supports 32-bit words only,
>>>>>>> even if BITLEN1 of SITMDR2 register is 16bit.
>>>>>>>
>>>>>>> Signed-off-by: Hiromitsu Yamasaki <[e-mail]>
>>>>>>> Signed-off-by: Dirk Behme <[e-mail]>
>>>>>>> ---
>>>>>>>     drivers/spi/spi-sh-msiof.c | 2 +-
>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
>>>>>>> index 2b4d3a520176..f9300fdf41e5 100644
>>>>>>> --- a/drivers/spi/spi-sh-msiof.c
>>>>>>> +++ b/drivers/spi/spi-sh-msiof.c
>>>>>>> @@ -904,7 +904,7 @@ static int sh_msiof_transfer_one(struct
>>>>>>> spi_master
>>>>>>> *master,
>>>>>>>                                    break;
>>>>>>>                            copy32 = copy_bswap32;
>>>>>>>                    } else if (bits <= 16) {
>>>>>>> -                       if (l & 1)
>>>>>>> +                       if (l & 3)
>>>>>>>                                    break;
>>>>>>>                            copy32 = copy_wswap32;
>>>>>>>                    } else {
>>>>>>
>>>>>> Looks OK, as l is in bytes, not 16-bit words.
>>>>>>
>>>>>> Reviewed-by: Geert Uytterhoeven <[e-mail]>
>>>>>
>>>>> What do you think about CCing this to -stable?
>>>>
>>>> Sounds OK. Have you tested this?
>>>
>>> I tested all 8 patches together if they fix the issues I observed with
>>> plain
>>> mainline.
>>
>> Could you please elaborate about the issues you observed with plain
>> mainline?
>> It may help to identify which patches are responsible for fixing them.
>
> I've been told that an easy test case is to just cat random data (any mid
> sized file) into SPI. E.g.:
>
> # cat /proc/cpuinfo > /dev/spidev32764.2
> [  504.544948] spi_sh_msiof e6e90000.spi: failed to shut down hardware
> [  504.551265] spidev spi32764.2: SPI transfer failed: -110
> [  504.556625] spi_master spi32764: failed to transfer one message from
> queue
> [  504.564857] spi_sh_msiof e6e90000.spi: failed to shut down hardware
> [  504.571177] spidev spi32764.2: SPI transfer failed: -110
> [  504.576528] spi_master spi32764: failed to transfer one message from
> queue
> cat: write error: Connection timed out
>
> done on plain 4.13-rc6.

Thank you, that's very valuable information!
We'll give it a try...

Note that this test does not exercise the code path affected by this particular
patch ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Geert Uytterhoeven Jan. 3, 2018, 5:25 p.m. UTC | #8

Hi Dirk,

On Thu, Sep 7, 2017 at 11:12 AM, Geert Uytterhoeven
<[e-mail]> wrote:
> On Thu, Sep 7, 2017 at 11:05 AM, Dirk Behme <[e-mail]> wrote:
>> On 07.09.2017 10:59, Geert Uytterhoeven wrote:
>>> On Thu, Sep 7, 2017 at 10:42 AM, Dirk Behme <[e-mail]>
>>> wrote:
>>>> On 07.09.2017 10:39, Geert Uytterhoeven wrote:
>>>>> On Thu, Sep 7, 2017 at 10:33 AM, Dirk Behme <[e-mail]>
>>>>> wrote:
>>>>>> On 07.09.2017 10:31, Geert Uytterhoeven wrote:
>>>>>>> On Wed, Sep 6, 2017 at 9:05 AM, Dirk Behme <[e-mail]>
>>>>>>> wrote:
>>>>>>>> From: Hiromitsu Yamasaki <[e-mail]>
>>>>>>>> DMA supports 32-bit words only,
>>>>>>>> even if BITLEN1 of SITMDR2 register is 16bit.
>>>>>>>>
>>>>>>>> Signed-off-by: Hiromitsu Yamasaki <[e-mail]>
>>>>>>>> Signed-off-by: Dirk Behme <[e-mail]>
>>>>>>>> ---
>>>>>>>>     drivers/spi/spi-sh-msiof.c | 2 +-
>>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
>>>>>>>> index 2b4d3a520176..f9300fdf41e5 100644
>>>>>>>> --- a/drivers/spi/spi-sh-msiof.c
>>>>>>>> +++ b/drivers/spi/spi-sh-msiof.c
>>>>>>>> @@ -904,7 +904,7 @@ static int sh_msiof_transfer_one(struct
>>>>>>>> spi_master
>>>>>>>> *master,
>>>>>>>>                                    break;
>>>>>>>>                            copy32 = copy_bswap32;
>>>>>>>>                    } else if (bits <= 16) {
>>>>>>>> -                       if (l & 1)
>>>>>>>> +                       if (l & 3)
>>>>>>>>                                    break;
>>>>>>>>                            copy32 = copy_wswap32;
>>>>>>>>                    } else {
>>>>>>>
>>>>>>> Looks OK, as l is in bytes, not 16-bit words.
>>>>>>>
>>>>>>> Reviewed-by: Geert Uytterhoeven <[e-mail]>
>>>>>>
>>>>>> What do you think about CCing this to -stable?
>>>>>
>>>>> Sounds OK. Have you tested this?
>>>>
>>>> I tested all 8 patches together if they fix the issues I observed with
>>>> plain
>>>> mainline.
>>>
>>> Could you please elaborate about the issues you observed with plain
>>> mainline?
>>> It may help to identify which patches are responsible for fixing them.
>>
>> I've been told that an easy test case is to just cat random data (any mid
>> sized file) into SPI. E.g.:
>>
>> # cat /proc/cpuinfo > /dev/spidev32764.2
>> [  504.544948] spi_sh_msiof e6e90000.spi: failed to shut down hardware
>> [  504.551265] spidev spi32764.2: SPI transfer failed: -110
>> [  504.556625] spi_master spi32764: failed to transfer one message from
>> queue
>> [  504.564857] spi_sh_msiof e6e90000.spi: failed to shut down hardware
>> [  504.571177] spidev spi32764.2: SPI transfer failed: -110
>> [  504.576528] spi_master spi32764: failed to transfer one message from
>> queue
>> cat: write error: Connection timed out
>>
>> done on plain 4.13-rc6.
>
> Thank you, that's very valuable information!
> We'll give it a try...

After some investigation, this is caused by DMA completion triggering
too early for TX-only transfers.  This should indeed be fixed by "[PATCH 5/8]
spi: sh-msiof: Wait for Tx FIFO empty after DMA" you extracted from the BSP.

As that patch needs changes to apply (a) on current spi/for-next and (b)
without other patches from your series, I reworked it into "[PATCH] spi:
sh-msiof: Fix timeout failures for TX-only DMA transfers".

As for the other patches from your series:
  - Upstream already has commit 36735783fdb599c9 ("spi: sh-msiof: Fix DMA
    transfer size check"),
  - spi/for-next has commit b8761434bdec32fa ("spi: sh-msiof: Implement
    cs-gpios configuration").
The remaining were either rejected, retracted, or it is unclear which problem
(if any) they are really fixing.

Do you agree?
Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Dirk Behme Jan. 4, 2018, 7:19 a.m. UTC | #9

On 03.01.2018 18:25, Geert Uytterhoeven wrote:
> Hi Dirk,
> 
> On Thu, Sep 7, 2017 at 11:12 AM, Geert Uytterhoeven
> <[e-mail]> wrote:
>> On Thu, Sep 7, 2017 at 11:05 AM, Dirk Behme <[e-mail]> wrote:
>>> On 07.09.2017 10:59, Geert Uytterhoeven wrote:
>>>> On Thu, Sep 7, 2017 at 10:42 AM, Dirk Behme <[e-mail]>
>>>> wrote:
>>>>> On 07.09.2017 10:39, Geert Uytterhoeven wrote:
>>>>>> On Thu, Sep 7, 2017 at 10:33 AM, Dirk Behme <[e-mail]>
>>>>>> wrote:
>>>>>>> On 07.09.2017 10:31, Geert Uytterhoeven wrote:
>>>>>>>> On Wed, Sep 6, 2017 at 9:05 AM, Dirk Behme <[e-mail]>
>>>>>>>> wrote:
>>>>>>>>> From: Hiromitsu Yamasaki <[e-mail]>
>>>>>>>>> DMA supports 32-bit words only,
>>>>>>>>> even if BITLEN1 of SITMDR2 register is 16bit.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Hiromitsu Yamasaki <[e-mail]>
>>>>>>>>> Signed-off-by: Dirk Behme <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>      drivers/spi/spi-sh-msiof.c | 2 +-
>>>>>>>>>      1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
>>>>>>>>> index 2b4d3a520176..f9300fdf41e5 100644
>>>>>>>>> --- a/drivers/spi/spi-sh-msiof.c
>>>>>>>>> +++ b/drivers/spi/spi-sh-msiof.c
>>>>>>>>> @@ -904,7 +904,7 @@ static int sh_msiof_transfer_one(struct
>>>>>>>>> spi_master
>>>>>>>>> *master,
>>>>>>>>>                                     break;
>>>>>>>>>                             copy32 = copy_bswap32;
>>>>>>>>>                     } else if (bits <= 16) {
>>>>>>>>> -                       if (l & 1)
>>>>>>>>> +                       if (l & 3)
>>>>>>>>>                                     break;
>>>>>>>>>                             copy32 = copy_wswap32;
>>>>>>>>>                     } else {
>>>>>>>>
>>>>>>>> Looks OK, as l is in bytes, not 16-bit words.
>>>>>>>>
>>>>>>>> Reviewed-by: Geert Uytterhoeven <[e-mail]>
>>>>>>>
>>>>>>> What do you think about CCing this to -stable?
>>>>>>
>>>>>> Sounds OK. Have you tested this?
>>>>>
>>>>> I tested all 8 patches together if they fix the issues I observed with
>>>>> plain
>>>>> mainline.
>>>>
>>>> Could you please elaborate about the issues you observed with plain
>>>> mainline?
>>>> It may help to identify which patches are responsible for fixing them.
>>>
>>> I've been told that an easy test case is to just cat random data (any mid
>>> sized file) into SPI. E.g.:
>>>
>>> # cat /proc/cpuinfo > /dev/spidev32764.2
>>> [  504.544948] spi_sh_msiof e6e90000.spi: failed to shut down hardware
>>> [  504.551265] spidev spi32764.2: SPI transfer failed: -110
>>> [  504.556625] spi_master spi32764: failed to transfer one message from
>>> queue
>>> [  504.564857] spi_sh_msiof e6e90000.spi: failed to shut down hardware
>>> [  504.571177] spidev spi32764.2: SPI transfer failed: -110
>>> [  504.576528] spi_master spi32764: failed to transfer one message from
>>> queue
>>> cat: write error: Connection timed out
>>>
>>> done on plain 4.13-rc6.
>>
>> Thank you, that's very valuable information!
>> We'll give it a try...
> 
> After some investigation, this is caused by DMA completion triggering
> too early for TX-only transfers.  This should indeed be fixed by "[PATCH 5/8]
> spi: sh-msiof: Wait for Tx FIFO empty after DMA" you extracted from the BSP.
> 
> As that patch needs changes to apply (a) on current spi/for-next and (b)
> without other patches from your series, I reworked it into "[PATCH] spi:
> sh-msiof: Fix timeout failures for TX-only DMA transfers".
> 
> As for the other patches from your series:
>    - Upstream already has commit 36735783fdb599c9 ("spi: sh-msiof: Fix DMA
>      transfer size check"),
>    - spi/for-next has commit b8761434bdec32fa ("spi: sh-msiof: Implement
>      cs-gpios configuration").
> The remaining were either rejected, retracted, or it is unclear which problem
> (if any) they are really fixing.


First, many thanks for looking into this!


> Do you agree?


If there are no more issues, then I agree ;) So lets rephrase that a 
little: We'll test what we have, now. And in case we find any remaining 
issues, we'll come back with more details, then. What will help us to 
look at the remaining patches.

I apologize already now in case testing will take some time ;)

Anyhow, many thanks again for looking into this.

Best regards

Dirk

Geert Uytterhoeven Jan. 4, 2018, 8:24 a.m. UTC | #10

Hi Dirk,

On Thu, Jan 4, 2018 at 8:19 AM, Dirk Behme <[e-mail]> wrote:
> On 03.01.2018 18:25, Geert Uytterhoeven wrote:
>> On Thu, Sep 7, 2017 at 11:12 AM, Geert Uytterhoeven
>> <[e-mail]> wrote:
>>> On Thu, Sep 7, 2017 at 11:05 AM, Dirk Behme <[e-mail]>
>>> wrote:
>>>> On 07.09.2017 10:59, Geert Uytterhoeven wrote:
>>>>> On Thu, Sep 7, 2017 at 10:42 AM, Dirk Behme <[e-mail]>
>>>>> wrote:
>>>>>> On 07.09.2017 10:39, Geert Uytterhoeven wrote:
>>>>>>> Sounds OK. Have you tested this?
>>>>>>
>>>>>> I tested all 8 patches together if they fix the issues I observed with
>>>>>> plain
>>>>>> mainline.
>>>>>
>>>>> Could you please elaborate about the issues you observed with plain
>>>>> mainline?
>>>>> It may help to identify which patches are responsible for fixing them.
>>>>
>>>> I've been told that an easy test case is to just cat random data (any
>>>> mid
>>>> sized file) into SPI. E.g.:
>>>>
>>>> # cat /proc/cpuinfo > /dev/spidev32764.2
>>>> [  504.544948] spi_sh_msiof e6e90000.spi: failed to shut down hardware
>>>> [  504.551265] spidev spi32764.2: SPI transfer failed: -110
>>>> [  504.556625] spi_master spi32764: failed to transfer one message from
>>>> queue
>>>> [  504.564857] spi_sh_msiof e6e90000.spi: failed to shut down hardware
>>>> [  504.571177] spidev spi32764.2: SPI transfer failed: -110
>>>> [  504.576528] spi_master spi32764: failed to transfer one message from
>>>> queue
>>>> cat: write error: Connection timed out
>>>>
>>>> done on plain 4.13-rc6.
>>>
>>> Thank you, that's very valuable information!
>>> We'll give it a try...
>>
>> After some investigation, this is caused by DMA completion triggering
>> too early for TX-only transfers.  This should indeed be fixed by "[PATCH
>> 5/8]
>> spi: sh-msiof: Wait for Tx FIFO empty after DMA" you extracted from the
>> BSP.
>>
>> As that patch needs changes to apply (a) on current spi/for-next and (b)
>> without other patches from your series, I reworked it into "[PATCH] spi:
>> sh-msiof: Fix timeout failures for TX-only DMA transfers".
>>
>> As for the other patches from your series:
>>    - Upstream already has commit 36735783fdb599c9 ("spi: sh-msiof: Fix DMA
>>      transfer size check"),
>>    - spi/for-next has commit b8761434bdec32fa ("spi: sh-msiof: Implement
>>      cs-gpios configuration").
>> The remaining were either rejected, retracted, or it is unclear which
>> problem
>> (if any) they are really fixing.
>
> First, many thanks for looking into this!
>
>> Do you agree?
>
> If there are no more issues, then I agree ;) So lets rephrase that a little:
> We'll test what we have, now. And in case we find any remaining issues,
> we'll come back with more details, then. What will help us to look at the
> remaining patches.

Sure, if more issues can be identified, we should look into fixing them as
well.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Patch

diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index 2b4d3a520176..f9300fdf41e5 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -904,7 +904,7 @@  static int sh_msiof_transfer_one(struct spi_master *master,
 				break;
 			copy32 = copy_bswap32;
 		} else if (bits <= 16) {
-			if (l & 1)
+			if (l & 3)
 				break;
 			copy32 = copy_wswap32;
 		} else {