[PATCH 2/2] libata: disable preemption during PIO copy

[PATCH 2/2] libata: disable preemption during PIO copy

[Sebastian Andrzej Siewior]

If the request has been made in user context it could be moved to a
different CPU on a SMP machine between the copy and cache flush. Thus we
could flush the dcache on the wrong CPU.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 drivers/ata/libata-sff.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 83ecf48..dca9f90 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -874,6 +874,9 @@ static void ata_pio_sector(struct ata_queued_cmd *qc)
 
 	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
 
+#ifdef CONFIG_SMP
+	preempt_disable();
+#endif
 	if (PageHighMem(page)) {
 		unsigned long flags;
 
@@ -896,6 +899,9 @@ static void ata_pio_sector(struct ata_queued_cmd *qc)
 	if (!do_write && !PageSlab(page))
 		flush_dcache_page(page);
 
+#ifdef CONFIG_SMP
+	preempt_enable();
+#endif
 	qc->curbytes += qc->sect_size;
 	qc->cursg_ofs += qc->sect_size;
 
-- 
1.6.6

Re: [PATCH 2/2] libata: disable preemption during PIO copy

[FUJITA Tomonori]

On Sun, 21 Mar 2010 22:53:08 +0100
Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote:

> If the request has been made in user context it could be moved to a
> different CPU on a SMP machine between the copy and cache flush. Thus we
> could flush the dcache on the wrong CPU.

This issue should be addressed in flush_dcache_page() inside?


> Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> ---
>  drivers/ata/libata-sff.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 83ecf48..dca9f90 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -874,6 +874,9 @@ static void ata_pio_sector(struct ata_queued_cmd *qc)
>  
>  	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
>  
> +#ifdef CONFIG_SMP
> +	preempt_disable();
> +#endif
>  	if (PageHighMem(page)) {
>  		unsigned long flags;
>  
> @@ -896,6 +899,9 @@ static void ata_pio_sector(struct ata_queued_cmd *qc)
>  	if (!do_write && !PageSlab(page))
>  		flush_dcache_page(page);
>  
> +#ifdef CONFIG_SMP
> +	preempt_enable();
> +#endif
>  	qc->curbytes += qc->sect_size;
>  	qc->cursg_ofs += qc->sect_size;
>  
> -- 
> 1.6.6
> 
> --
> 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 2/2] libata: disable preemption during PIO copy

[Sebastian Andrzej Siewior]

* FUJITA Tomonori | 2010-03-23 18:49:04 [+0900]:

>On Sun, 21 Mar 2010 22:53:08 +0100
>Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote:
>
>> If the request has been made in user context it could be moved to a
>> different CPU on a SMP machine between the copy and cache flush. Thus we
>> could flush the dcache on the wrong CPU.
>
>This issue should be addressed in flush_dcache_page() inside?

How so? You have to remain on the same CPU on which you started the copy
process. Once you get to flush_dcache_page() you may have allready
switched the CPU.

Sebastian

Re: [PATCH 2/2] libata: disable preemption during PIO copy

[FUJITA Tomonori]

On Tue, 23 Mar 2010 12:01:24 +0100
Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote:

> * FUJITA Tomonori | 2010-03-23 18:49:04 [+0900]:
> 
> >On Sun, 21 Mar 2010 22:53:08 +0100
> >Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote:
> >
> >> If the request has been made in user context it could be moved to a
> >> different CPU on a SMP machine between the copy and cache flush. Thus we
> >> could flush the dcache on the wrong CPU.
> >
> >This issue should be addressed in flush_dcache_page() inside?
> 
> How so? You have to remain on the same CPU on which you started the copy
> process. Once you get to flush_dcache_page() you may have allready
> switched the CPU.

Oops, I thought that you trying to address on the deferred
flush_dcache_page() issue. I don't know enough about how this pio code
works, but if your description is correct, then the patch is fine, I
guess.

Re: [PATCH 2/2] libata: disable preemption during PIO copy

[Sebastian Andrzej Siewior]

* FUJITA Tomonori | 2010-03-23 20:51:34 [+0900]:

>> How so? You have to remain on the same CPU on which you started the copy
>> process. Once you get to flush_dcache_page() you may have allready
>> switched the CPU.
>
>Oops, I thought that you trying to address on the deferred
>flush_dcache_page() issue. I don't know enough about how this pio code
>works, but if your description is correct, then the patch is fine, I
>guess.

No, it is not. The dcache flush is a global one so a CPU switch does not
matter. So I NAK that one myself.

Sebastian