[PATCH] sata_sil24: Use memory barriers before issuing commands

[PATCH] sata_sil24: Use memory barriers before issuing commands

[Catalin Marinas]

The data in the cmd_block buffers may reach the main memory after the
writel() to the device ports. This patch introduces two calls to wmb()
to ensure the relative ordering.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Tested-by: Colin Tuckley <colin.tuckley@arm.com>
Cc: Tejun Heo <tj@kernel.org>
---
 drivers/ata/sata_sil24.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index e925051..6392fdb 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -622,6 +622,7 @@ static int sil24_exec_polled_cmd(struct ata_port *ap, int pmp,
 	irq_enabled = readl(port + PORT_IRQ_ENABLE_SET);
 	writel(PORT_IRQ_COMPLETE | PORT_IRQ_ERROR, port + PORT_IRQ_ENABLE_CLR);
 
+	wmb();
 	writel((u32)paddr, port + PORT_CMD_ACTIVATE);
 	writel((u64)paddr >> 32, port + PORT_CMD_ACTIVATE + 4);
 
@@ -895,6 +896,7 @@ static unsigned int sil24_qc_issue(struct ata_queued_cmd *qc)
 	paddr = pp->cmd_block_dma + tag * sizeof(*pp->cmd_block);
 	activate = port + PORT_CMD_ACTIVATE + tag * 8;
 
+	wmb();
 	writel((u32)paddr, activate);
 	writel((u64)paddr >> 32, activate + 4);
 

Re: [PATCH] sata_sil24: Use memory barriers before issuing commands

[Tejun Heo]

On 06/10/2010 04:57 PM, Catalin Marinas wrote:
> The data in the cmd_block buffers may reach the main memory after the
> writel() to the device ports. This patch introduces two calls to wmb()
> to ensure the relative ordering.
> 
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Tested-by: Colin Tuckley <colin.tuckley@arm.com>
> Cc: Tejun Heo <tj@kernel.org>

Can you please add comments explaining why they're needed?

Thanks.

-- 
tejun

Re: [PATCH] sata_sil24: Use memory barriers before issuing commands

[Grant Grundler]

On Thu, Jun 10, 2010 at 7:57 AM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> The data in the cmd_block buffers may reach the main memory after the
> writel() to the device ports.

"ia-64 Linux Kernel" (mosberger and eranian) uses exactly this sequence
as an example for wmb() on page 303.

I'm curious about the system that exposed this problem. I believe wmb() fixes
 an issue not exposed on most machines. Can any general comments be
made about cache coherency, memory ordering (weak?), instruction ordering
 (super scalar?), etc. ?

The explanation above is a bit short (most people won't understand it).


> This patch introduces two calls to wmb() to ensure the relative ordering.

And as Tejun asked, the comment where wmb() gets used should clearly
explain which host memory writes are targetted by the wmb().

thanks,
grant

>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Tested-by: Colin Tuckley <colin.tuckley@arm.com>
> Cc: Tejun Heo <tj@kernel.org>
> ---
>  drivers/ata/sata_sil24.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> index e925051..6392fdb 100644
> --- a/drivers/ata/sata_sil24.c
> +++ b/drivers/ata/sata_sil24.c
> @@ -622,6 +622,7 @@ static int sil24_exec_polled_cmd(struct ata_port *ap, int pmp,
>        irq_enabled = readl(port + PORT_IRQ_ENABLE_SET);
>        writel(PORT_IRQ_COMPLETE | PORT_IRQ_ERROR, port + PORT_IRQ_ENABLE_CLR);
>
> +       wmb();
>        writel((u32)paddr, port + PORT_CMD_ACTIVATE);
>        writel((u64)paddr >> 32, port + PORT_CMD_ACTIVATE + 4);
>
> @@ -895,6 +896,7 @@ static unsigned int sil24_qc_issue(struct ata_queued_cmd *qc)
>        paddr = pp->cmd_block_dma + tag * sizeof(*pp->cmd_block);
>        activate = port + PORT_CMD_ACTIVATE + tag * 8;
>
> +       wmb();
>        writel((u32)paddr, activate);
>        writel((u64)paddr >> 32, activate + 4);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] sata_sil24: Use memory barriers before issuing commands

[Catalin Marinas]

Hi Grant,

Grant Grundler <grundler@google.com> wrote:
> On Thu, Jun 10, 2010 at 7:57 AM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
>> The data in the cmd_block buffers may reach the main memory after the
>> writel() to the device ports.
>
> "ia-64 Linux Kernel" (mosberger and eranian) uses exactly this sequence
> as an example for wmb() on page 303.
>
> I'm curious about the system that exposed this problem. I believe wmb() fixes
>  an issue not exposed on most machines. Can any general comments be
> made about cache coherency, memory ordering (weak?), instruction ordering
>  (super scalar?), etc. ?
>
> The explanation above is a bit short (most people won't understand it).

I already posted a second version of this patch, though it triggered a
longer discussion on whether we should do this (cross-post between LKML,
linux-ide and linux-arch):

http://thread.gmane.org/gmane.linux.ide/46414

I know IA-64 and a several other architectures have weak memory ordering
but some of them just add barriers in the I/O accessors (with some
performance penalty).

Since the (new) patch is already in mainline, please comment on the other
thread for memory ordering etc.

Thanks.

-- 
Catalin