sata_sil24: Use memory barriers before issuing commands
in [Kernel]
|
Prev: [PATCH for-2.6.35] virtio_net: do not reschedule rx refill forever
Next: [PATCH for-2.6.35] virtio_net: fix oom handling on tx
From: Tejun Heo on 10 Jun 2010 11:30 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(a)arm.com> > Tested-by: Colin Tuckley <colin.tuckley(a)arm.com> > Cc: Tejun Heo <tj(a)kernel.org> Can you please add comments explaining why they're needed? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Tejun Heo on 10 Jun 2010 12:20 On 06/10/2010 06:02 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(a)arm.com> > Tested-by: Colin Tuckley <colin.tuckley(a)arm.com> > Cc: Tejun Heo <tj(a)kernel.org> > Cc: Jeff Garzik <jeff(a)garzik.org> I suppose you have tested and verified that this is actually necessary, right? I've been looking through the docs but couldn't find anything which described the ordering between writes to main memory and write[bwl]()'s. One thing that kind of bothers me is that r/wmb()'s are for ordering memory accesses among CPUs which participate in cache coherency protocol and although it may work right in the above case I'm not really sure whether this is the right thing to do. Do you have more information on the subject? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jeff Garzik on 10 Jun 2010 16:20 On 06/10/2010 12:02 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(a)arm.com> > Tested-by: Colin Tuckley<colin.tuckley(a)arm.com> > Cc: Tejun Heo<tj(a)kernel.org> > Cc: Jeff Garzik<jeff(a)garzik.org> > --- > drivers/ata/sata_sil24.c | 10 ++++++++++ > 1 files changed, 10 insertions(+), 0 deletions(-) > > diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c > index e925051..a5d5aff 100644 > --- a/drivers/ata/sata_sil24.c > +++ b/drivers/ata/sata_sil24.c > @@ -622,6 +622,11 @@ 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); > > + /* > + * The barrier is required to ensure that writes to cmd_block reach > + * the memory before the write to PORT_CMD_ACTIVATE. > + */ > + wmb(); > writel((u32)paddr, port + PORT_CMD_ACTIVATE); > writel((u64)paddr>> 32, port + PORT_CMD_ACTIVATE + 4); > > @@ -895,6 +900,11 @@ 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; > > + /* > + * The barrier is required to ensure that writes to cmd_block reach > + * the memory before the write to PORT_CMD_ACTIVATE. > + */ > + wmb(); > writel((u32)paddr, activate); > writel((u64)paddr>> 32, activate + 4); > applied -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Robert Hancock on 10 Jun 2010 20:50 On 06/10/2010 10:23 AM, Catalin Marinas wrote: > On Thu, 2010-06-10 at 17:12 +0100, Tejun Heo wrote: >> On 06/10/2010 06:02 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(a)arm.com> >>> Tested-by: Colin Tuckley<colin.tuckley(a)arm.com> >>> Cc: Tejun Heo<tj(a)kernel.org> >>> Cc: Jeff Garzik<jeff(a)garzik.org> >> >> I suppose you have tested and verified that this is actually >> necessary, right? > > Yes, otherwise we get random failures with this device on ARM. > >> I've been looking through the docs but couldn't >> find anything which described the ordering between writes to main >> memory and write[bwl]()'s. One thing that kind of bothers me is that >> r/wmb()'s are for ordering memory accesses among CPUs which >> participate in cache coherency protocol and although it may work right >> in the above case I'm not really sure whether this is the right thing >> to do. Do you have more information on the subject? > > The mb() are not for ordering accesses among CPUs (though they would > cover this case as well). For inter-CPU ordering, we have smp_mb() and > friends. For all other cases, we have the mandatory barriers mb() and > friends and DMA is one of them. > > Apart from the memory-barriers.txt document, there is the Device I/O > docbook which mentions something about DMA buffers, though not very > clear on which barriers to use (something like just make sure that the > writes to the buffer reached the memory). > > There were some past discussions on linux-arch before and I'm cc'ing > this list again (ARM is not the only architecture with a weakly memory > ordering model). > > I'm copying the patch below again for the linux-arch people that haven't > seen the beginning of the thread: My memory is fuzzy but I thought this came up before on PPC and I also thought the conclusion was that the platform code (for writel, etc.) should enforce ordering of MMIO accesses with respect to normal RAM accesses. (Or maybe it was just MMIO accesses with respect to each other?) I don't think the answer to that question has been clearly documented anywhere, which is somewhat unfortunate. If the answer is that this is needed then there are likely a lot of other drivers in libata and elsewhere which need to be fixed as well. For example, I don't see any such barriers in libahci.c when I presume it would need them. IMHO, it would be better for the platform code to ensure that MMIO access was strongly ordered with respect to each other and to RAM access. Drivers are just too likely to get this wrong, especially when x86, the most tested platform, doesn't have such issues. > > >> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c >> index e925051..a5d5aff 100644 >> --- a/drivers/ata/sata_sil24.c >> +++ b/drivers/ata/sata_sil24.c >> @@ -622,6 +622,11 @@ 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); >> >> + /* >> + * The barrier is required to ensure that writes to cmd_block reach >> + * the memory before the write to PORT_CMD_ACTIVATE. >> + */ >> + wmb(); >> writel((u32)paddr, port + PORT_CMD_ACTIVATE); >> writel((u64)paddr>> 32, port + PORT_CMD_ACTIVATE + 4); >> >> @@ -895,6 +900,11 @@ 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; >> >> + /* >> + * The barrier is required to ensure that writes to cmd_block reach >> + * the memory before the write to PORT_CMD_ACTIVATE. >> + */ >> + wmb(); >> writel((u32)paddr, activate); >> writel((u64)paddr>> 32, activate + 4); >> > > Thanks. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Mark Lord on 23 Jun 2010 09:20
On 10/06/10 08:43 PM, Robert Hancock wrote: ... > My memory is fuzzy but I thought this came up before on PPC and I also > thought the conclusion was that the platform code (for writel, etc.) > should enforce ordering of MMIO accesses with respect to normal RAM > accesses. (Or maybe it was just MMIO accesses with respect to each > other?) I don't think the answer to that question has been clearly > documented anywhere, which is somewhat unfortunate. ... Different problem. That discussion was for PIO reads into the page cache, and ensuring coherency from all of that. Whereas this patch is just ordinary low-level chipset programming, and ensuring the descriptors are visible before issuing the "go" command. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |