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

[Grant Grundler <grundler@google.com>]

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
>