stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] ata: sata_rcar: Fix DMA boundary mask
@ 2020-09-17 13:09 Geert Uytterhoeven
  2020-10-15 18:34 ` Geert Uytterhoeven
  2020-10-16 16:40 ` Andy Shevchenko
  0 siblings, 2 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2020-09-17 13:09 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Ulf Hansson, Greg Kroah-Hartman, Christoph Hellwig, Ming Lei,
	Sergei Shtylyov, Yoshihiro Shimoda, linux-ide, linux-renesas-soc,
	linux-kernel, stable, Geert Uytterhoeven

Before commit 9495b7e92f716ab2 ("driver core: platform: Initialize
dma_parms for platform devices"), the R-Car SATA device didn't have DMA
parameters.  Hence the DMA boundary mask supplied by its driver was
silently ignored, as __scsi_init_queue() doesn't check the return value
of dma_set_seg_boundary(), and the default value of 0xffffffff was used.

Now the device has gained DMA parameters, the driver-supplied value is
used, and the following warning is printed on Salvator-XS:

    DMA-API: sata_rcar ee300000.sata: mapping sg segment across boundary [start=0x00000000ffffe000] [end=0x00000000ffffefff] [boundary=0x000000001ffffffe]
    WARNING: CPU: 5 PID: 38 at kernel/dma/debug.c:1233 debug_dma_map_sg+0x298/0x300

(the range of start/end values depend on whether IOMMU support is
 enabled or not)

The issue here is that SATA_RCAR_DMA_BOUNDARY doesn't have bit 0 set, so
any typical end value, which is odd, will trigger the check.

Fix this by increasing the DMA boundary value by 1.

This also fixes the following WRITE DMA EXT timeout issue:

    # dd if=/dev/urandom of=/mnt/de1/file1-1024M bs=1M count=1024
    ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
    ata1.00: failed command: WRITE DMA EXT
    ata1.00: cmd 35/00:00:00:e6:0c/00:0a:00:00:00/e0 tag 0 dma 1310720 out
    res 40/00:01:00:00:00/00:00:00:00:00/00 Emask 0x4 (timeout)
    ata1.00: status: { DRDY }

as seen by Shimoda-san since commit 429120f3df2dba2b ("block: fix
splitting segments on boundary masks").

Fixes: 8bfbeed58665dbbf ("sata_rcar: correct 'sata_rcar_sht'")
Fixes: 9495b7e92f716ab2 ("driver core: platform: Initialize dma_parms for platform devices")
Fixes: 429120f3df2dba2b ("block: fix splitting segments on boundary masks")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: stable <stable@vger.kernel.org>
---
v3:
  - Add Reviewed-by, Tested-by,
  - Augment description and Fixes: with Shimoda-san's problem report
    https://lore.kernel.org/r/1600255098-21411-1-git-send-email-yoshihiro.shimoda.uh@renesas.com,

v2:
  - Add Reviewed-by, Tested-by, Cc.
---
 drivers/ata/sata_rcar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/sata_rcar.c b/drivers/ata/sata_rcar.c
index 141ac600b64c87ef..44b0ed8f6bb8a120 100644
--- a/drivers/ata/sata_rcar.c
+++ b/drivers/ata/sata_rcar.c
@@ -120,7 +120,7 @@
 /* Descriptor table word 0 bit (when DTA32M = 1) */
 #define SATA_RCAR_DTEND			BIT(0)
 
-#define SATA_RCAR_DMA_BOUNDARY		0x1FFFFFFEUL
+#define SATA_RCAR_DMA_BOUNDARY		0x1FFFFFFFUL
 
 /* Gen2 Physical Layer Control Registers */
 #define RCAR_GEN2_PHY_CTL1_REG		0x1704
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] ata: sata_rcar: Fix DMA boundary mask
  2020-09-17 13:09 [PATCH v3] ata: sata_rcar: Fix DMA boundary mask Geert Uytterhoeven
@ 2020-10-15 18:34 ` Geert Uytterhoeven
  2020-10-16 15:32   ` Jens Axboe
  2020-10-16 16:40 ` Andy Shevchenko
  1 sibling, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2020-10-15 18:34 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Ulf Hansson, Greg Kroah-Hartman, Christoph Hellwig, Ming Lei,
	Sergei Shtylyov, Yoshihiro Shimoda, linux-ide, Linux-Renesas,
	Linux Kernel Mailing List, stable, Lad, Prabhakar

Hi Jens,

On Thu, Sep 17, 2020 at 3:09 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> Before commit 9495b7e92f716ab2 ("driver core: platform: Initialize
> dma_parms for platform devices"), the R-Car SATA device didn't have DMA
> parameters.  Hence the DMA boundary mask supplied by its driver was
> silently ignored, as __scsi_init_queue() doesn't check the return value
> of dma_set_seg_boundary(), and the default value of 0xffffffff was used.
>
> Now the device has gained DMA parameters, the driver-supplied value is
> used, and the following warning is printed on Salvator-XS:
>
>     DMA-API: sata_rcar ee300000.sata: mapping sg segment across boundary [start=0x00000000ffffe000] [end=0x00000000ffffefff] [boundary=0x000000001ffffffe]
>     WARNING: CPU: 5 PID: 38 at kernel/dma/debug.c:1233 debug_dma_map_sg+0x298/0x300
>
> (the range of start/end values depend on whether IOMMU support is
>  enabled or not)
>
> The issue here is that SATA_RCAR_DMA_BOUNDARY doesn't have bit 0 set, so
> any typical end value, which is odd, will trigger the check.
>
> Fix this by increasing the DMA boundary value by 1.
>
> This also fixes the following WRITE DMA EXT timeout issue:
>
>     # dd if=/dev/urandom of=/mnt/de1/file1-1024M bs=1M count=1024
>     ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
>     ata1.00: failed command: WRITE DMA EXT
>     ata1.00: cmd 35/00:00:00:e6:0c/00:0a:00:00:00/e0 tag 0 dma 1310720 out
>     res 40/00:01:00:00:00/00:00:00:00:00/00 Emask 0x4 (timeout)
>     ata1.00: status: { DRDY }
>
> as seen by Shimoda-san since commit 429120f3df2dba2b ("block: fix
> splitting segments on boundary masks").
>
> Fixes: 8bfbeed58665dbbf ("sata_rcar: correct 'sata_rcar_sht'")
> Fixes: 9495b7e92f716ab2 ("driver core: platform: Initialize dma_parms for platform devices")
> Fixes: 429120f3df2dba2b ("block: fix splitting segments on boundary masks")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Cc: stable <stable@vger.kernel.org>

Can you please apply this patch?
This is a fix for a regression in v5.7-rc5, and was first posted almost
5 months ago.

Thanks!

> ---
> v3:
>   - Add Reviewed-by, Tested-by,
>   - Augment description and Fixes: with Shimoda-san's problem report
>     https://lore.kernel.org/r/1600255098-21411-1-git-send-email-yoshihiro.shimoda.uh@renesas.com,
>
> v2:
>   - Add Reviewed-by, Tested-by, Cc.
> ---
>  drivers/ata/sata_rcar.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/ata/sata_rcar.c b/drivers/ata/sata_rcar.c
> index 141ac600b64c87ef..44b0ed8f6bb8a120 100644
> --- a/drivers/ata/sata_rcar.c
> +++ b/drivers/ata/sata_rcar.c
> @@ -120,7 +120,7 @@
>  /* Descriptor table word 0 bit (when DTA32M = 1) */
>  #define SATA_RCAR_DTEND                        BIT(0)
>
> -#define SATA_RCAR_DMA_BOUNDARY         0x1FFFFFFEUL
> +#define SATA_RCAR_DMA_BOUNDARY         0x1FFFFFFFUL
>
>  /* Gen2 Physical Layer Control Registers */
>  #define RCAR_GEN2_PHY_CTL1_REG         0x1704

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] ata: sata_rcar: Fix DMA boundary mask
  2020-10-15 18:34 ` Geert Uytterhoeven
@ 2020-10-16 15:32   ` Jens Axboe
  0 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2020-10-16 15:32 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Ulf Hansson, Greg Kroah-Hartman, Christoph Hellwig, Ming Lei,
	Sergei Shtylyov, Yoshihiro Shimoda, linux-ide, Linux-Renesas,
	Linux Kernel Mailing List, stable, Lad, Prabhakar

On 10/15/20 12:34 PM, Geert Uytterhoeven wrote:
> Hi Jens,
> 
> On Thu, Sep 17, 2020 at 3:09 PM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
>> Before commit 9495b7e92f716ab2 ("driver core: platform: Initialize
>> dma_parms for platform devices"), the R-Car SATA device didn't have DMA
>> parameters.  Hence the DMA boundary mask supplied by its driver was
>> silently ignored, as __scsi_init_queue() doesn't check the return value
>> of dma_set_seg_boundary(), and the default value of 0xffffffff was used.
>>
>> Now the device has gained DMA parameters, the driver-supplied value is
>> used, and the following warning is printed on Salvator-XS:
>>
>>     DMA-API: sata_rcar ee300000.sata: mapping sg segment across boundary [start=0x00000000ffffe000] [end=0x00000000ffffefff] [boundary=0x000000001ffffffe]
>>     WARNING: CPU: 5 PID: 38 at kernel/dma/debug.c:1233 debug_dma_map_sg+0x298/0x300
>>
>> (the range of start/end values depend on whether IOMMU support is
>>  enabled or not)
>>
>> The issue here is that SATA_RCAR_DMA_BOUNDARY doesn't have bit 0 set, so
>> any typical end value, which is odd, will trigger the check.
>>
>> Fix this by increasing the DMA boundary value by 1.
>>
>> This also fixes the following WRITE DMA EXT timeout issue:
>>
>>     # dd if=/dev/urandom of=/mnt/de1/file1-1024M bs=1M count=1024
>>     ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
>>     ata1.00: failed command: WRITE DMA EXT
>>     ata1.00: cmd 35/00:00:00:e6:0c/00:0a:00:00:00/e0 tag 0 dma 1310720 out
>>     res 40/00:01:00:00:00/00:00:00:00:00/00 Emask 0x4 (timeout)
>>     ata1.00: status: { DRDY }
>>
>> as seen by Shimoda-san since commit 429120f3df2dba2b ("block: fix
>> splitting segments on boundary masks").
>>
>> Fixes: 8bfbeed58665dbbf ("sata_rcar: correct 'sata_rcar_sht'")
>> Fixes: 9495b7e92f716ab2 ("driver core: platform: Initialize dma_parms for platform devices")
>> Fixes: 429120f3df2dba2b ("block: fix splitting segments on boundary masks")
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>> Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>> Cc: stable <stable@vger.kernel.org>
> 
> Can you please apply this patch?
> This is a fix for a regression in v5.7-rc5, and was first posted almost
> 5 months ago.

Applied, sorry that got missed.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] ata: sata_rcar: Fix DMA boundary mask
  2020-09-17 13:09 [PATCH v3] ata: sata_rcar: Fix DMA boundary mask Geert Uytterhoeven
  2020-10-15 18:34 ` Geert Uytterhoeven
@ 2020-10-16 16:40 ` Andy Shevchenko
  2020-10-17 19:15   ` Sergei Shtylyov
  1 sibling, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-10-16 16:40 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Jens Axboe, Ulf Hansson, Greg Kroah-Hartman, Christoph Hellwig,
	Ming Lei, Sergei Shtylyov, Yoshihiro Shimoda, linux-ide,
	Linux-Renesas, Linux Kernel Mailing List, stable

On Thu, Sep 17, 2020 at 4:12 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> Before commit 9495b7e92f716ab2 ("driver core: platform: Initialize
> dma_parms for platform devices"), the R-Car SATA device didn't have DMA
> parameters.  Hence the DMA boundary mask supplied by its driver was
> silently ignored, as __scsi_init_queue() doesn't check the return value
> of dma_set_seg_boundary(), and the default value of 0xffffffff was used.
>
> Now the device has gained DMA parameters, the driver-supplied value is
> used, and the following warning is printed on Salvator-XS:
>
>     DMA-API: sata_rcar ee300000.sata: mapping sg segment across boundary [start=0x00000000ffffe000] [end=0x00000000ffffefff] [boundary=0x000000001ffffffe]
>     WARNING: CPU: 5 PID: 38 at kernel/dma/debug.c:1233 debug_dma_map_sg+0x298/0x300
>
> (the range of start/end values depend on whether IOMMU support is
>  enabled or not)
>
> The issue here is that SATA_RCAR_DMA_BOUNDARY doesn't have bit 0 set, so
> any typical end value, which is odd, will trigger the check.
>
> Fix this by increasing the DMA boundary value by 1.
>
> This also fixes the following WRITE DMA EXT timeout issue:
>
>     # dd if=/dev/urandom of=/mnt/de1/file1-1024M bs=1M count=1024
>     ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
>     ata1.00: failed command: WRITE DMA EXT
>     ata1.00: cmd 35/00:00:00:e6:0c/00:0a:00:00:00/e0 tag 0 dma 1310720 out
>     res 40/00:01:00:00:00/00:00:00:00:00/00 Emask 0x4 (timeout)
>     ata1.00: status: { DRDY }
>
> as seen by Shimoda-san since commit 429120f3df2dba2b ("block: fix
> splitting segments on boundary masks").
>
> Fixes: 8bfbeed58665dbbf ("sata_rcar: correct 'sata_rcar_sht'")
> Fixes: 9495b7e92f716ab2 ("driver core: platform: Initialize dma_parms for platform devices")
> Fixes: 429120f3df2dba2b ("block: fix splitting segments on boundary masks")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Cc: stable <stable@vger.kernel.org>
> ---
> v3:
>   - Add Reviewed-by, Tested-by,
>   - Augment description and Fixes: with Shimoda-san's problem report
>     https://lore.kernel.org/r/1600255098-21411-1-git-send-email-yoshihiro.shimoda.uh@renesas.com,
>
> v2:
>   - Add Reviewed-by, Tested-by, Cc.
> ---
>  drivers/ata/sata_rcar.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/ata/sata_rcar.c b/drivers/ata/sata_rcar.c
> index 141ac600b64c87ef..44b0ed8f6bb8a120 100644
> --- a/drivers/ata/sata_rcar.c
> +++ b/drivers/ata/sata_rcar.c
> @@ -120,7 +120,7 @@
>  /* Descriptor table word 0 bit (when DTA32M = 1) */
>  #define SATA_RCAR_DTEND                        BIT(0)
>
> -#define SATA_RCAR_DMA_BOUNDARY         0x1FFFFFFEUL
> +#define SATA_RCAR_DMA_BOUNDARY         0x1FFFFFFFUL

Wondering if GENMASK() here will be better to avoid such mistakes.

>
>  /* Gen2 Physical Layer Control Registers */
>  #define RCAR_GEN2_PHY_CTL1_REG         0x1704
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] ata: sata_rcar: Fix DMA boundary mask
  2020-10-16 16:40 ` Andy Shevchenko
@ 2020-10-17 19:15   ` Sergei Shtylyov
  2020-10-18  9:59     ` Geert Uytterhoeven
  0 siblings, 1 reply; 6+ messages in thread
From: Sergei Shtylyov @ 2020-10-17 19:15 UTC (permalink / raw)
  To: Andy Shevchenko, Geert Uytterhoeven
  Cc: Jens Axboe, Ulf Hansson, Greg Kroah-Hartman, Christoph Hellwig,
	Ming Lei, Yoshihiro Shimoda, linux-ide, Linux-Renesas,
	Linux Kernel Mailing List, stable

On 10/16/20 7:40 PM, Andy Shevchenko wrote:

>> Before commit 9495b7e92f716ab2 ("driver core: platform: Initialize
>> dma_parms for platform devices"), the R-Car SATA device didn't have DMA
>> parameters.  Hence the DMA boundary mask supplied by its driver was
>> silently ignored, as __scsi_init_queue() doesn't check the return value
>> of dma_set_seg_boundary(), and the default value of 0xffffffff was used.
>>
>> Now the device has gained DMA parameters, the driver-supplied value is
>> used, and the following warning is printed on Salvator-XS:
>>
>>     DMA-API: sata_rcar ee300000.sata: mapping sg segment across boundary [start=0x00000000ffffe000] [end=0x00000000ffffefff] [boundary=0x000000001ffffffe]
>>     WARNING: CPU: 5 PID: 38 at kernel/dma/debug.c:1233 debug_dma_map_sg+0x298/0x300
>>
>> (the range of start/end values depend on whether IOMMU support is
>>  enabled or not)
>>
>> The issue here is that SATA_RCAR_DMA_BOUNDARY doesn't have bit 0 set, so
>> any typical end value, which is odd, will trigger the check.
>>
>> Fix this by increasing the DMA boundary value by 1.
>>
>> This also fixes the following WRITE DMA EXT timeout issue:
>>
>>     # dd if=/dev/urandom of=/mnt/de1/file1-1024M bs=1M count=1024
>>     ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
>>     ata1.00: failed command: WRITE DMA EXT
>>     ata1.00: cmd 35/00:00:00:e6:0c/00:0a:00:00:00/e0 tag 0 dma 1310720 out
>>     res 40/00:01:00:00:00/00:00:00:00:00/00 Emask 0x4 (timeout)
>>     ata1.00: status: { DRDY }
>>
>> as seen by Shimoda-san since commit 429120f3df2dba2b ("block: fix
>> splitting segments on boundary masks").
>>
>> Fixes: 8bfbeed58665dbbf ("sata_rcar: correct 'sata_rcar_sht'")
>> Fixes: 9495b7e92f716ab2 ("driver core: platform: Initialize dma_parms for platform devices")
>> Fixes: 429120f3df2dba2b ("block: fix splitting segments on boundary masks")
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>> Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>> Cc: stable <stable@vger.kernel.org>
>> ---
>> v3:
>>   - Add Reviewed-by, Tested-by,
>>   - Augment description and Fixes: with Shimoda-san's problem report
>>     https://lore.kernel.org/r/1600255098-21411-1-git-send-email-yoshihiro.shimoda.uh@renesas.com,
>>
>> v2:
>>   - Add Reviewed-by, Tested-by, Cc.
>> ---
>>  drivers/ata/sata_rcar.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/sata_rcar.c b/drivers/ata/sata_rcar.c
>> index 141ac600b64c87ef..44b0ed8f6bb8a120 100644
>> --- a/drivers/ata/sata_rcar.c
>> +++ b/drivers/ata/sata_rcar.c
>> @@ -120,7 +120,7 @@
>>  /* Descriptor table word 0 bit (when DTA32M = 1) */
>>  #define SATA_RCAR_DTEND                        BIT(0)
>>
>> -#define SATA_RCAR_DMA_BOUNDARY         0x1FFFFFFEUL
>> +#define SATA_RCAR_DMA_BOUNDARY         0x1FFFFFFFUL
> 
> Wondering if GENMASK() here will be better to avoid such mistakes.

   How? The bit 0 is reserved, so only even byte counts are possiblе...

[...]

MBR, Sergei

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] ata: sata_rcar: Fix DMA boundary mask
  2020-10-17 19:15   ` Sergei Shtylyov
@ 2020-10-18  9:59     ` Geert Uytterhoeven
  0 siblings, 0 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2020-10-18  9:59 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: Andy Shevchenko, Jens Axboe, Ulf Hansson, Greg Kroah-Hartman,
	Christoph Hellwig, Ming Lei, Yoshihiro Shimoda, linux-ide,
	Linux-Renesas, Linux Kernel Mailing List, stable

Hi Sergei,

On Sat, Oct 17, 2020 at 9:15 PM Sergei Shtylyov
<sergei.shtylyov@gmail.com> wrote:
> On 10/16/20 7:40 PM, Andy Shevchenko wrote:
> >> Before commit 9495b7e92f716ab2 ("driver core: platform: Initialize
> >> dma_parms for platform devices"), the R-Car SATA device didn't have DMA
> >> parameters.  Hence the DMA boundary mask supplied by its driver was
> >> silently ignored, as __scsi_init_queue() doesn't check the return value
> >> of dma_set_seg_boundary(), and the default value of 0xffffffff was used.
> >>
> >> Now the device has gained DMA parameters, the driver-supplied value is
> >> used, and the following warning is printed on Salvator-XS:
> >>
> >>     DMA-API: sata_rcar ee300000.sata: mapping sg segment across boundary [start=0x00000000ffffe000] [end=0x00000000ffffefff] [boundary=0x000000001ffffffe]
> >>     WARNING: CPU: 5 PID: 38 at kernel/dma/debug.c:1233 debug_dma_map_sg+0x298/0x300
> >>
> >> (the range of start/end values depend on whether IOMMU support is
> >>  enabled or not)
> >>
> >> The issue here is that SATA_RCAR_DMA_BOUNDARY doesn't have bit 0 set, so
> >> any typical end value, which is odd, will trigger the check.
> >>
> >> Fix this by increasing the DMA boundary value by 1.
> >>
> >> This also fixes the following WRITE DMA EXT timeout issue:
> >>
> >>     # dd if=/dev/urandom of=/mnt/de1/file1-1024M bs=1M count=1024
> >>     ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
> >>     ata1.00: failed command: WRITE DMA EXT
> >>     ata1.00: cmd 35/00:00:00:e6:0c/00:0a:00:00:00/e0 tag 0 dma 1310720 out
> >>     res 40/00:01:00:00:00/00:00:00:00:00/00 Emask 0x4 (timeout)
> >>     ata1.00: status: { DRDY }
> >>
> >> as seen by Shimoda-san since commit 429120f3df2dba2b ("block: fix
> >> splitting segments on boundary masks").
> >>
> >> Fixes: 8bfbeed58665dbbf ("sata_rcar: correct 'sata_rcar_sht'")
> >> Fixes: 9495b7e92f716ab2 ("driver core: platform: Initialize dma_parms for platform devices")
> >> Fixes: 429120f3df2dba2b ("block: fix splitting segments on boundary masks")
> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >> Reviewed-by: Christoph Hellwig <hch@lst.de>
> >> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> >> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >> Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> >> Cc: stable <stable@vger.kernel.org>
> >> ---
> >> v3:
> >>   - Add Reviewed-by, Tested-by,
> >>   - Augment description and Fixes: with Shimoda-san's problem report
> >>     https://lore.kernel.org/r/1600255098-21411-1-git-send-email-yoshihiro.shimoda.uh@renesas.com,
> >>
> >> v2:
> >>   - Add Reviewed-by, Tested-by, Cc.
> >> ---
> >>  drivers/ata/sata_rcar.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/ata/sata_rcar.c b/drivers/ata/sata_rcar.c
> >> index 141ac600b64c87ef..44b0ed8f6bb8a120 100644
> >> --- a/drivers/ata/sata_rcar.c
> >> +++ b/drivers/ata/sata_rcar.c
> >> @@ -120,7 +120,7 @@
> >>  /* Descriptor table word 0 bit (when DTA32M = 1) */
> >>  #define SATA_RCAR_DTEND                        BIT(0)
> >>
> >> -#define SATA_RCAR_DMA_BOUNDARY         0x1FFFFFFEUL
> >> +#define SATA_RCAR_DMA_BOUNDARY         0x1FFFFFFFUL
> >
> > Wondering if GENMASK() here will be better to avoid such mistakes.
>
>    How? The bit 0 is reserved, so only even byte counts are possiblе...

The DMA Transfer Count Register (ATAPI_DMA_TRANS_CNT) indeed does not
support odd values ("Bit 0 is ignored because the ATAPI data bus is
handled on a 16-bit basis (on a word basis)"), and is limited to
transfers less than 512 MiB.
Similarly the DMA Start Address Register (ATAPI_DMA_START_ADR) requires
that start address to be 32-bit aligned.

I believe the alignment and even size restrictions are met by the block
layer, by performing only transfers that are a multiple of the device's
block size (512 bytes minimum).

However, the SATA_RCAR_DMA_BOUNDARY definition is only used to set the
DMA boundary mask, which limits the MSB bits of an address. The LSB
bits of the mask should always be set, as a typical segment end (=
length - 1, i.e. ending in 0x1ff for a 512-byte block size) will always
have the 9 LSB bits set, else it will trigger the warning.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-10-18 10:00 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-17 13:09 [PATCH v3] ata: sata_rcar: Fix DMA boundary mask Geert Uytterhoeven
2020-10-15 18:34 ` Geert Uytterhoeven
2020-10-16 15:32   ` Jens Axboe
2020-10-16 16:40 ` Andy Shevchenko
2020-10-17 19:15   ` Sergei Shtylyov
2020-10-18  9:59     ` Geert Uytterhoeven

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).