| Message ID | 1508227542-13165-5-git-send-email-chenhc@lemote.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 10/17/2017 11:05 AM, Huacai Chen wrote: > In non-coherent DMA mode, kernel uses cache flushing operations to > maintain I/O coherency, so in ata_do_dev_read_id() the DMA buffer > should be aligned to ARCH_DMA_MINALIGN. Otherwise, If a DMA buffer > and a kernel structure share a same cache line, and if the kernel > structure has dirty data, cache_invalidate (no writeback) will cause > data corruption. > > Cc: stable@vger.kernel.org > Signed-off-by: Huacai Chen <chenhc@lemote.com> > --- > drivers/ata/libata-core.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index ee4c1ec..e134955 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -1833,8 +1833,19 @@ static u32 ata_pio_mask_no_iordy(const struct ata_device *adev) > unsigned int ata_do_dev_read_id(struct ata_device *dev, > struct ata_taskfile *tf, u16 *id) > { > - return ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, > - id, sizeof(id[0]) * ATA_ID_WORDS, 0); > + u16 *devid; > + int res, size = sizeof(u16) * ATA_ID_WORDS; > + > + if (IS_ALIGNED((unsigned long)id, dma_get_cache_alignment(&dev->tdev))) > + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, id, size, 0); > + else { > + devid = kmalloc(size, GFP_KERNEL); > + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, devid, size, 0); > + memcpy(id, devid, size); > + kfree(devid); > + } > + > + return res; This function is called only for the PIO mode commands, so I doubt this is necessary... MBR, Sergei
Hi, Sergei, Without this patch, in non-coherent mode, ata_do_set_mode() return with "no PIO support", and disk detection fails. Huacai ------------------ Original ------------------ From: "Sergei Shtylyov"<sergei.shtylyov@cogentembedded.com>; Date: Tue, Oct 17, 2017 05:43 PM To: "Huacai Chen"<chenhc@lemote.com>; "Christoph Hellwig"<hch@lst.de>; Cc: "Marek Szyprowski"<m.szyprowski@samsung.com>; "Robin Murphy"<robin.murphy@arm.com>; "Andrew Morton"<akpm@linux-foundation.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "linux-kernel"<linux-kernel@vger.kernel.org>; "Ralf Baechle"<ralf@linux-mips.org>; "James Hogan"<james.hogan@imgtec.com>; "linux-mips"<linux-mips@linux-mips.org>; "James E . J . Bottomley"<jejb@linux.vnet.ibm.com>; "Martin K . Petersen"<martin.petersen@oracle.com>; "linux-scsi"<linux-scsi@vger.kernel.org>; "Tejun Heo"<tj@kernel.org>; "linux-ide"<linux-ide@vger.kernel.org>; "stable"<stable@vger.kernel.org>; Subject: Re: [PATCH V8 5/5] libata: Align DMA buffer todma_get_cache_alignment() On 10/17/2017 11:05 AM, Huacai Chen wrote: > In non-coherent DMA mode, kernel uses cache flushing operations to > maintain I/O coherency, so in ata_do_dev_read_id() the DMA buffer > should be aligned to ARCH_DMA_MINALIGN. Otherwise, If a DMA buffer > and a kernel structure share a same cache line, and if the kernel > structure has dirty data, cache_invalidate (no writeback) will cause > data corruption. > > Cc: stable@vger.kernel.org > Signed-off-by: Huacai Chen <chenhc@lemote.com> > --- > drivers/ata/libata-core.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index ee4c1ec..e134955 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -1833,8 +1833,19 @@ static u32 ata_pio_mask_no_iordy(const struct ata_device *adev) > unsigned int ata_do_dev_read_id(struct ata_device *dev, > struct ata_taskfile *tf, u16 *id) > { > - return ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, > - id, sizeof(id[0]) * ATA_ID_WORDS, 0); > + u16 *devid; > + int res, size = sizeof(u16) * ATA_ID_WORDS; > + > + if (IS_ALIGNED((unsigned long)id, dma_get_cache_alignment(&dev->tdev))) > + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, id, size, 0); > + else { > + devid = kmalloc(size, GFP_KERNEL); > + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, devid, size, 0); > + memcpy(id, devid, size); > + kfree(devid); > + } > + > + return res; This function is called only for the PIO mode commands, so I doubt this is necessary... MBR, Sergei
On Tue, Oct 17, 2017 at 04:05:42PM +0800, Huacai Chen wrote: > In non-coherent DMA mode, kernel uses cache flushing operations to > maintain I/O coherency, so in ata_do_dev_read_id() the DMA buffer > should be aligned to ARCH_DMA_MINALIGN. Otherwise, If a DMA buffer > and a kernel structure share a same cache line, and if the kernel > structure has dirty data, cache_invalidate (no writeback) will cause > data corruption. > > Cc: stable@vger.kernel.org > Signed-off-by: Huacai Chen <chenhc@lemote.com> > --- > drivers/ata/libata-core.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index ee4c1ec..e134955 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -1833,8 +1833,19 @@ static u32 ata_pio_mask_no_iordy(const struct ata_device *adev) > unsigned int ata_do_dev_read_id(struct ata_device *dev, > struct ata_taskfile *tf, u16 *id) > { > - return ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, > - id, sizeof(id[0]) * ATA_ID_WORDS, 0); > + u16 *devid; > + int res, size = sizeof(u16) * ATA_ID_WORDS; > + > + if (IS_ALIGNED((unsigned long)id, dma_get_cache_alignment(&dev->tdev))) > + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, id, size, 0); > + else { > + devid = kmalloc(size, GFP_KERNEL); > + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, devid, size, 0); > + memcpy(id, devid, size); > + kfree(devid); > + } > + > + return res; Hmm... I think it'd be a lot better to ensure that the buffers are aligned properly to begin with. There are only two buffers which are used for id reading - ata_port->sector_buf and ata_device->id. Both are embedded arrays but making them separately allocated aligned buffers shouldn't be difficult. Thanks.
> This function is called only for the PIO mode commands, so I doubt this is > necessary... That is true but there are platforms out there that issue disk level PIO commands via DMA (or can do so). Indeed the Cyrix MediaGX could do that in the 1990s but I never add support 8) So I think it makes sense to allocate the buffers DMA aligned, but it doesn't seem to explain the situation in this case and I think it would be helpful to know what platform and ATA driver is tripping this and wny they are the only people in the universe to have the problem. Alan
On 18/10/17 14:03, Tejun Heo wrote: > On Tue, Oct 17, 2017 at 04:05:42PM +0800, Huacai Chen wrote: >> In non-coherent DMA mode, kernel uses cache flushing operations to >> maintain I/O coherency, so in ata_do_dev_read_id() the DMA buffer >> should be aligned to ARCH_DMA_MINALIGN. Otherwise, If a DMA buffer >> and a kernel structure share a same cache line, and if the kernel >> structure has dirty data, cache_invalidate (no writeback) will cause >> data corruption. >> >> Cc: stable@vger.kernel.org >> Signed-off-by: Huacai Chen <chenhc@lemote.com> >> --- >> drivers/ata/libata-core.c | 15 +++++++++++++-- >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c >> index ee4c1ec..e134955 100644 >> --- a/drivers/ata/libata-core.c >> +++ b/drivers/ata/libata-core.c >> @@ -1833,8 +1833,19 @@ static u32 ata_pio_mask_no_iordy(const struct ata_device *adev) >> unsigned int ata_do_dev_read_id(struct ata_device *dev, >> struct ata_taskfile *tf, u16 *id) >> { >> - return ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, >> - id, sizeof(id[0]) * ATA_ID_WORDS, 0); >> + u16 *devid; >> + int res, size = sizeof(u16) * ATA_ID_WORDS; >> + >> + if (IS_ALIGNED((unsigned long)id, dma_get_cache_alignment(&dev->tdev))) >> + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, id, size, 0); >> + else { >> + devid = kmalloc(size, GFP_KERNEL); >> + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, devid, size, 0); >> + memcpy(id, devid, size); >> + kfree(devid); >> + } >> + >> + return res; > Hmm... I think it'd be a lot better to ensure that the buffers are > aligned properly to begin with. There are only two buffers which are > used for id reading - ata_port->sector_buf and ata_device->id. Both > are embedded arrays but making them separately allocated aligned > buffers shouldn't be difficult. > > Thanks. FWIW, I agree that the buffers used for DMA should be split out from the structure. We ran into this problem on MIPS last year, 4ee34ea3a12396f35b26d90a094c75db95080baa ("libata: Align ata_device's id on a cacheline") partially fixed it, but likely should have also cacheline aligned the following devslp_timing in the struct such that we guarantee that members of the struct not used for DMA do not share the same cacheline as the DMA buffer. Not having this means that architectures, such as MIPS, which in some cases have to perform manual invalidation of DMA buffer can clobber valid adjacent data if it is in the same cacheline. Thanks, Matt
Hi, Matt,
I found that 4ee34ea3a12396f35b26d90a094c75db ("libata: Align ata_device's id on a cacheline") can resolve everything. Because the size of id[ATA_ID_WORDS] is already aligned and devslp_timing needn't to be aligned. So, In V9 of this series I will drop this patch. Why I had problems before? because I used linux-4.4.
Huacai
------------------ Original ------------------
From: "Matt Redfearn"<matt.redfearn@mips.com>;
Date: Thu, Oct 19, 2017 03:52 PM
To: "Tejun Heo"<tj@kernel.org>; "Huacai Chen"<chenhc@lemote.com>;
Cc: "Christoph Hellwig"<hch@lst.de>; "Marek Szyprowski"<m.szyprowski@samsung.com>; "Robin Murphy"<robin.murphy@arm.com>; "AndrewMorton"<akpm@linux-foundation.org>; "Fuxin Zhang"<zhangfx@lemote.com>; "linux-kernel"<linux-kernel@vger.kernel.org>; "Ralf Baechle"<ralf@linux-mips.org>; "JamesHogan"<james.hogan@imgtec.com>; "linux-mips"<linux-mips@linux-mips.org>; "James E . J .Bottomley"<jejb@linux.vnet.ibm.com>; "Martin K . Petersen"<martin.petersen@oracle.com>; "linux-scsi"<linux-scsi@vger.kernel.org>; "linux-ide"<linux-ide@vger.kernel.org>; "stable"<stable@vger.kernel.org>;
Subject: Re: [PATCH V8 5/5] libata: Align DMA buffer todma_get_cache_alignment()
On 18/10/17 14:03, Tejun Heo wrote:
> On Tue, Oct 17, 2017 at 04:05:42PM +0800, Huacai Chen wrote:
>> In non-coherent DMA mode, kernel uses cache flushing operations to
>> maintain I/O coherency, so in ata_do_dev_read_id() the DMA buffer
>> should be aligned to ARCH_DMA_MINALIGN. Otherwise, If a DMA buffer
>> and a kernel structure share a same cache line, and if the kernel
>> structure has dirty data, cache_invalidate (no writeback) will cause
>> data corruption.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Huacai Chen <chenhc@lemote.com>
>> ---
>> drivers/ata/libata-core.c | 15 +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index ee4c1ec..e134955 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -1833,8 +1833,19 @@ static u32 ata_pio_mask_no_iordy(const struct ata_device *adev)
>> unsigned int ata_do_dev_read_id(struct ata_device *dev,
>> struct ata_taskfile *tf, u16 *id)
>> {
>> - return ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE,
>> - id, sizeof(id[0]) * ATA_ID_WORDS, 0);
>> + u16 *devid;
>> + int res, size = sizeof(u16) * ATA_ID_WORDS;
>> +
>> + if (IS_ALIGNED((unsigned long)id, dma_get_cache_alignment(&dev->tdev)))
>> + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, id, size, 0);
>> + else {
>> + devid = kmalloc(size, GFP_KERNEL);
>> + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, devid, size, 0);
>> + memcpy(id, devid, size);
>> + kfree(devid);
>> + }
>> +
>> + return res;
> Hmm... I think it'd be a lot better to ensure that the buffers are
> aligned properly to begin with. There are only two buffers which are
> used for id reading - ata_port->sector_buf and ata_device->id. Both
> are embedded arrays but making them separately allocated aligned
> buffers shouldn't be difficult.
>
> Thanks.
FWIW, I agree that the buffers used for DMA should be split out from the
structure. We ran into this problem on MIPS last year,
4ee34ea3a12396f35b26d90a094c75db95080baa ("libata: Align ata_device's id
on a cacheline") partially fixed it, but likely should have also
cacheline aligned the following devslp_timing in the struct such that we
guarantee that members of the struct not used for DMA do not share the
same cacheline as the DMA buffer. Not having this means that
architectures, such as MIPS, which in some cases have to perform manual
invalidation of DMA buffer can clobber valid adjacent data if it is in
the same cacheline.
Thanks,
Matt
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index ee4c1ec..e134955 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -1833,8 +1833,19 @@ static u32 ata_pio_mask_no_iordy(const struct ata_device *adev) unsigned int ata_do_dev_read_id(struct ata_device *dev, struct ata_taskfile *tf, u16 *id) { - return ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, - id, sizeof(id[0]) * ATA_ID_WORDS, 0); + u16 *devid; + int res, size = sizeof(u16) * ATA_ID_WORDS; + + if (IS_ALIGNED((unsigned long)id, dma_get_cache_alignment(&dev->tdev))) + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, id, size, 0); + else { + devid = kmalloc(size, GFP_KERNEL); + res = ata_exec_internal(dev, tf, NULL, DMA_FROM_DEVICE, devid, size, 0); + memcpy(id, devid, size); + kfree(devid); + } + + return res; } /**
In non-coherent DMA mode, kernel uses cache flushing operations to maintain I/O coherency, so in ata_do_dev_read_id() the DMA buffer should be aligned to ARCH_DMA_MINALIGN. Otherwise, If a DMA buffer and a kernel structure share a same cache line, and if the kernel structure has dirty data, cache_invalidate (no writeback) will cause data corruption. Cc: stable@vger.kernel.org Signed-off-by: Huacai Chen <chenhc@lemote.com> --- drivers/ata/libata-core.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)