Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 13 Jul 2007, Geert Uytterhoeven wrote:
Ah, that explains it. flush_dcache_page() is used in some drivers.
I'll update my patches. Thanks for the comments!
Does this look OK?
- Replaced KM_USER0 by KM_IRQ0 (all routines are either called from an
interrupt handler, from .request_fn (ps3disk), or from .queuecommand
(ps3rom))
- Add a call to flush_kernel_dcache_page() in routines that write to buffers
If this is OK, I'll fold it into my original patch series and will resend.
Thanks!
---
drivers/block/ps3disk.c |5 +++--
drivers/scsi/ps3rom.c |9 +
2 files changed, 8 insertions(+), 6 deletions(-)
--- a/drivers/block/ps3disk.c
+++ b/drivers/block/ps3disk.c
@@ -109,13 +109,14 @@ static void ps3disk_scatter_gather(struc
bio_sectors(bio), sector);
bio_for_each_segment(bvec, bio, j) {
size = bio_cur_sectors(bio)*KERNEL_SECTOR_SIZE;
- buf = __bio_kmap_atomic(bio, j, KM_USER0);
+ buf = __bio_kmap_atomic(bio, j, KM_IRQ0);
if (gather)
memcpy(dev-bounce_buf+offset, buf, size);
else
memcpy(buf, dev-bounce_buf+offset, size);
offset += size;
- __bio_kunmap_atomic(bio, KM_USER0);
+ flush_kernel_dcache_page(bio_iovec_idx(bio,
j)-bv_page);
+ __bio_kunmap_atomic(bio, KM_IRQ0);
}
sectors += bio_sectors(bio);
i++;
--- a/drivers/scsi/ps3rom.c
+++ b/drivers/scsi/ps3rom.c
@@ -112,7 +112,7 @@ static int fill_from_dev_buffer(struct s
active = 1;
for (k = 0, req_len = 0, act_len = 0; k cmd-use_sg; ++k, ++sgpnt) {
if (active) {
- kaddr = kmap_atomic(sgpnt-page, KM_USER0);
+ kaddr = kmap_atomic(sgpnt-page, KM_IRQ0);
if (!kaddr)
return -1;
len = sgpnt-length;
@@ -121,7 +121,8 @@ static int fill_from_dev_buffer(struct s
len = buflen - req_len;
}
memcpy(kaddr + sgpnt-offset, buf + req_len, len);
- kunmap_atomic(kaddr, KM_USER0);
+ flush_kernel_dcache_page(sgpnt-page);
+ kunmap_atomic(kaddr, KM_IRQ0);
act_len += len;
}
req_len += sgpnt-length;
@@ -149,7 +150,7 @@ static int fetch_to_dev_buffer(struct sc
sgpnt = cmd-request_buffer;
for (k = 0, req_len = 0, fin = 0; k cmd-use_sg; ++k, ++sgpnt) {
- kaddr = kmap_atomic(sgpnt-page, KM_USER0);
+ kaddr = kmap_atomic(sgpnt-page, KM_IRQ0);
if (!kaddr)
return -1;
len = sgpnt-length;
@@ -158,7 +159,7 @@ static int fetch_to_dev_buffer(struct sc
fin = 1;
}
memcpy(buf + req_len, kaddr + sgpnt-offset, len);
- kunmap_atomic(kaddr, KM_USER0);
+ kunmap_atomic(kaddr, KM_IRQ0);
if (fin)
return req_len + len;
req_len += sgpnt-length;
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone:+32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [EMAIL PROTECTED]
Internet: http://www.sony-europe.com/
Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Mon, 16 Jul 2007, Jens Axboe wrote: On Mon, Jul 16 2007, Geert Uytterhoeven wrote: On Fri, 13 Jul 2007, Geert Uytterhoeven wrote: Ah, that explains it. flush_dcache_page() is used in some drivers. I'll update my patches. Thanks for the comments! Does this look OK? - Replaced KM_USER0 by KM_IRQ0 (all routines are either called from an interrupt handler, from .request_fn (ps3disk), or from .queuecommand (ps3rom)) That looks good. - Add a call to flush_kernel_dcache_page() in routines that write to buffers Hmm, I would have thought a flush_dcache_page() would be more appropriate, the backing could be page cache pages. OK, I'll change it to flush_dcache_page(). With kind regards, Geert Uytterhoeven Software Architect Sony Network and Software Technology Center Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone:+32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: [EMAIL PROTECTED] Internet: http://www.sony-europe.com/ Sony Network and Software Technology Center Europe A division of Sony Service Centre (Europe) N.V. Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium VAT BE 0413.825.160 · RPR Brussels Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Mon, 16 Jul 2007, Geert Uytterhoeven wrote: On Mon, 16 Jul 2007, Jens Axboe wrote: On Mon, Jul 16 2007, Geert Uytterhoeven wrote: On Fri, 13 Jul 2007, Geert Uytterhoeven wrote: Ah, that explains it. flush_dcache_page() is used in some drivers. I'll update my patches. Thanks for the comments! Does this look OK? - Replaced KM_USER0 by KM_IRQ0 (all routines are either called from an interrupt handler, from .request_fn (ps3disk), or from .queuecommand (ps3rom)) That looks good. - Add a call to flush_kernel_dcache_page() in routines that write to buffers Hmm, I would have thought a flush_dcache_page() would be more appropriate, the backing could be page cache pages. OK, I'll change it to flush_dcache_page(). Upon closer look, while flush_kernel_dcache_page() is a no-op on ppc64, flush_dcache_page() isn't. So I'd prefer to not call it if not really needed. And according to James, flush_kernel_dcache_page() should be sufficient... So I'm getting puzzled again... With kind regards, Geert Uytterhoeven Software Architect Sony Network and Software Technology Center Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone:+32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: [EMAIL PROTECTED] Internet: http://www.sony-europe.com/ Sony Network and Software Technology Center Europe A division of Sony Service Centre (Europe) N.V. Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium VAT BE 0413.825.160 · RPR Brussels Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Mon, Jul 16 2007, Geert Uytterhoeven wrote: On Mon, 16 Jul 2007, Geert Uytterhoeven wrote: On Mon, 16 Jul 2007, Jens Axboe wrote: On Mon, Jul 16 2007, Geert Uytterhoeven wrote: On Fri, 13 Jul 2007, Geert Uytterhoeven wrote: Ah, that explains it. flush_dcache_page() is used in some drivers. I'll update my patches. Thanks for the comments! Does this look OK? - Replaced KM_USER0 by KM_IRQ0 (all routines are either called from an interrupt handler, from .request_fn (ps3disk), or from .queuecommand (ps3rom)) That looks good. - Add a call to flush_kernel_dcache_page() in routines that write to buffers Hmm, I would have thought a flush_dcache_page() would be more appropriate, the backing could be page cache pages. OK, I'll change it to flush_dcache_page(). Upon closer look, while flush_kernel_dcache_page() is a no-op on ppc64, flush_dcache_page() isn't. So I'd prefer to not call it if not really needed. And according to James, flush_kernel_dcache_page() should be sufficient... So I'm getting puzzled again... I'll let James expand on why he thinks flush_kernel_dcache_page() should be sufficient. If the backing is always user memory from get_user_pages(), then the flush_kernel_dcache_page() looks sufficient. For the normal IO paths, it could be that or page cache pages too for instance. -- Jens Axboe - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Mon, Jul 16 2007, James Bottomley wrote: On Mon, 2007-07-16 at 14:16 +0200, Jens Axboe wrote: On Mon, Jul 16 2007, Geert Uytterhoeven wrote: On Fri, 13 Jul 2007, Geert Uytterhoeven wrote: Ah, that explains it. flush_dcache_page() is used in some drivers. I'll update my patches. Thanks for the comments! Does this look OK? - Replaced KM_USER0 by KM_IRQ0 (all routines are either called from an interrupt handler, from .request_fn (ps3disk), or from .queuecommand (ps3rom)) That looks good. - Add a call to flush_kernel_dcache_page() in routines that write to buffers Hmm, I would have thought a flush_dcache_page() would be more appropriate, the backing could be page cache pages. No ... that was the point of flush_kernel_dcache_page(). The page in question is page cache backed and contains user mappings. However, the block layer has already done a flush_dcache_page() in get_user_pages() and the user shouldn't be touching memory under I/O (unless they want self induced aliasing problems) so we're free to assume all the user cachelines are purged, hence all we have to do is flush the kernel alias to bring the page up to date and make the users see it correctly. Oh indeed, I missed the flush_dcache_page() in get_user_pages(). -- Jens Axboe - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Mon, 16 Jul 2007, Jens Axboe wrote: On Mon, Jul 16 2007, James Bottomley wrote: On Mon, 2007-07-16 at 14:16 +0200, Jens Axboe wrote: On Mon, Jul 16 2007, Geert Uytterhoeven wrote: On Fri, 13 Jul 2007, Geert Uytterhoeven wrote: Ah, that explains it. flush_dcache_page() is used in some drivers. I'll update my patches. Thanks for the comments! Does this look OK? - Replaced KM_USER0 by KM_IRQ0 (all routines are either called from an interrupt handler, from .request_fn (ps3disk), or from .queuecommand (ps3rom)) That looks good. - Add a call to flush_kernel_dcache_page() in routines that write to buffers Hmm, I would have thought a flush_dcache_page() would be more appropriate, the backing could be page cache pages. No ... that was the point of flush_kernel_dcache_page(). The page in question is page cache backed and contains user mappings. However, the block layer has already done a flush_dcache_page() in get_user_pages() and the user shouldn't be touching memory under I/O (unless they want self induced aliasing problems) so we're free to assume all the user cachelines are purged, hence all we have to do is flush the kernel alias to bring the page up to date and make the users see it correctly. Oh indeed, I missed the flush_dcache_page() in get_user_pages(). Good, thanks for reaching a consensus, so I can update my patch series. With kind regards, Geert Uytterhoeven Software Architect Sony Network and Software Technology Center Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone:+32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: [EMAIL PROTECTED] Internet: http://www.sony-europe.com/ Sony Network and Software Technology Center Europe A division of Sony Service Centre (Europe) N.V. Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium VAT BE 0413.825.160 · RPR Brussels Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
Upon closer look, while flush_kernel_dcache_page() is a no-op on ppc64, flush_dcache_page() isn't. So I'd prefer to not call it if not really needed. And according to James, flush_kernel_dcache_page() should be sufficient... So I'm getting puzzled again... flush_dcache_page() handles icache vs. dcache coherency by clearing the PG_arch_1 bit in the struct page that indicates that the page is cache clean. You -must- call it if you're going to use any form of CPU access to write to the page (basically dirtying the data cache) and that page can be ever mapped into user space and executed from. I don't know what flush_kernel_dcache_page() does and if it needs a similar treatement, it's a new interface, so maybe Jens and or James can tell me more about it.. Cheers, Ben. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Mon, 2007-07-16 at 17:03 -0500, James Bottomley wrote: On Tue, 2007-07-17 at 07:49 +1000, Benjamin Herrenschmidt wrote: No ... that was the point of flush_kernel_dcache_page(). The page in question is page cache backed and contains user mappings. However, the block layer has already done a flush_dcache_page() in get_user_pages() and the user shouldn't be touching memory under I/O (unless they want self induced aliasing problems) so we're free to assume all the user cachelines are purged, hence all we have to do is flush the kernel alias to bring the page up to date and make the users see it correctly. The block layer will have done that even in the swap-out path ? (Just asking... I'm not very familiar with the block layer) Er ... not really, this is the I/O path for user initiated I/O. The page out path, by definition, can't have any extant user mappings. For page out, the relevant page is flushed before its mapping is detached, and then it can be paged to the backing store (or for anonymous pages to the swap device) when no mappings remain. Ok, thanks. Ben. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, Jul 13 2007, James Bottomley wrote:
On Wed, 2007-07-04 at 15:22 +0200, Geert Uytterhoeven wrote:
+ kaddr = kmap_atomic(sgpnt-page, KM_USER0);
+ if (!kaddr)
+ return -1;
+ len = sgpnt-length;
+ if ((req_len + len) buflen) {
+ active = 0;
+ len = buflen - req_len;
+ }
+ memcpy(kaddr + sgpnt-offset, buf + req_len,
len);
+ kunmap_atomic(kaddr, KM_USER0);
This isn't a SCSI objection, but this sequence appears several times in
this driver. It's wrong for a non-PIPT architecture (and I believe the
PS3 is VIPT) because you copy into the kernel alias for the page, which
dirties the line in the cache of that alias (the user alias cache line
was already invalidated). However, unless you flush the kernel alias to
main memory, the user could read stale data. The way this is supposed
to be done is to do a
flush_kernel_dcache_page(kaddr)
before doing the kunmap.
Otherwise it looks OK from the SCSI point of view.
Well, even worse is that fact that it's using KM_USER0 from interrupt
context.
--
Jens Axboe
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Wed, 2007-07-04 at 15:22 +0200, Geert Uytterhoeven wrote:
+ kaddr = kmap_atomic(sgpnt-page, KM_USER0);
+ if (!kaddr)
+ return -1;
+ len = sgpnt-length;
+ if ((req_len + len) buflen) {
+ active = 0;
+ len = buflen - req_len;
+ }
+ memcpy(kaddr + sgpnt-offset, buf + req_len,
len);
+ kunmap_atomic(kaddr, KM_USER0);
This isn't a SCSI objection, but this sequence appears several times in
this driver. It's wrong for a non-PIPT architecture (and I believe the
PS3 is VIPT) because you copy into the kernel alias for the page, which
dirties the line in the cache of that alias (the user alias cache line
was already invalidated). However, unless you flush the kernel alias to
main memory, the user could read stale data. The way this is supposed
to be done is to do a
flush_kernel_dcache_page(kaddr)
before doing the kunmap.
Otherwise it looks OK from the SCSI point of view.
James
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 13 Jul 2007, James Bottomley wrote: On Fri, 2007-07-13 at 15:25 +0200, Geert Uytterhoeven wrote: kmap() just returns page_address() on ppc64, as there's no highmem. kunmap() is a no-op. So technically I could just use page_address() directly, but Christoph wanted me to keep the kmap()/kunmap() sequence because it's considered a good practice. The point isn't what kmap and kunmap do ... it's the addresses they return. By and large, a kernel virtual address for a page is different from the user virtual address. If the cache is virtually indexed you get different cache lines for the same page ... and that sets you up with aliases you need to resolve. parisc is the same ... our kmap/kunmap are nops as well, but our kernel virtual addresses are still different from the user virtual ones. IC. - flush_kernel_dcache_page() is a no-op on ppc64 (ARCH_HAS_FLUSH_KERNEL_DCACHE_PAGE is defined on parisc only). - For reference, drivers/scsi/ipr.c (another ppc64 driver) just uses a plain kmap/memcpy/kunmap sequence So what should I do? With kind regards, Geert Uytterhoeven Software Architect Sony Network and Software Technology Center Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone:+32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: [EMAIL PROTECTED] Internet: http://www.sony-europe.com/ Sony Network and Software Technology Center Europe A division of Sony Service Centre (Europe) N.V. Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium VAT BE 0413.825.160 · RPR Brussels Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 2007-07-13 at 15:45 +0200, Geert Uytterhoeven wrote: On Fri, 13 Jul 2007, James Bottomley wrote: On Fri, 2007-07-13 at 15:25 +0200, Geert Uytterhoeven wrote: kmap() just returns page_address() on ppc64, as there's no highmem. kunmap() is a no-op. So technically I could just use page_address() directly, but Christoph wanted me to keep the kmap()/kunmap() sequence because it's considered a good practice. The point isn't what kmap and kunmap do ... it's the addresses they return. By and large, a kernel virtual address for a page is different from the user virtual address. If the cache is virtually indexed you get different cache lines for the same page ... and that sets you up with aliases you need to resolve. parisc is the same ... our kmap/kunmap are nops as well, but our kernel virtual addresses are still different from the user virtual ones. IC. - flush_kernel_dcache_page() is a no-op on ppc64 (ARCH_HAS_FLUSH_KERNEL_DCACHE_PAGE is defined on parisc only). - For reference, drivers/scsi/ipr.c (another ppc64 driver) just uses a plain kmap/memcpy/kunmap sequence So what should I do? Ask someone who knows the architecture ... Anton, Paulus or Benh ... I'm fairly certain PPC is VIPT and will need some kind of alias resolution ... perhaps its associative enough not to let the aliases be a problem. James - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 2007-07-13 at 17:10 +0200, Geert Uytterhoeven wrote:
On Fri, 13 Jul 2007, Arnd Bergmann wrote:
On Friday 13 July 2007, James Bottomley wrote:
IC.
- flush_kernel_dcache_page() is a no-op on ppc64
(ARCH_HAS_FLUSH_KERNEL_DCACHE_PAGE is defined on parisc only).
- For reference, drivers/scsi/ipr.c (another ppc64 driver) just uses a
plain
kmap/memcpy/kunmap sequence
So what should I do?
Ask someone who knows the architecture ... Anton, Paulus or Benh ... I'm
fairly certain PPC is VIPT and will need some kind of alias
resolution ... perhaps its associative enough not to let the aliases be
a problem.
I'm pretty sure that no ppc64 machine needs alias resolution in the kernel,
although some are VIPT. Last time we discussed this, Segher explained it
to me, but I don't remember which way Cell does it. IIRC, it automatically
flushes cache lines that are accessed through aliases.
Thanks for confirming!
It's probably a good idea to have the flush_kernel_dcache_page() in there
anyway, if only to serve as an example for people that copy it into
architecture-independent drivers, same as what we do for the
k{,un}map_atomic() that is also not required on ppc64.
Now my next question: why should I add it, if currently no single driver in
mainline calls flush_kernel_dcache_page()?
`git grep' finds it in the following files only:
Documentation/cachetlb.txt
arch/parisc/kernel/cache.c
arch/parisc/kernel/pacache.S
include/asm-parisc/cacheflush.h
include/linux/highmem.h
It's a recent addition to the API ... the previous way of doing it was
flush_dcache_page() but that's expensive and flushes all the user
aliases. Since you know in this case there are no current user aliases
and you only need to flush the kernel alias, flush_kernel_dcache_page()
was introduced as the cheaper alternative.
James
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, Jul 13 2007, Geert Uytterhoeven wrote:
It's probably a good idea to have the flush_kernel_dcache_page() in there
anyway, if only to serve as an example for people that copy it into
architecture-independent drivers, same as what we do for the
k{,un}map_atomic() that is also not required on ppc64.
Now my next question: why should I add it, if currently no single driver in
mainline calls flush_kernel_dcache_page()?
`git grep' finds it in the following files only:
Documentation/cachetlb.txt
arch/parisc/kernel/cache.c
arch/parisc/kernel/pacache.S
include/asm-parisc/cacheflush.h
include/linux/highmem.h
Not many drivers fiddle around with stuff like this, it's usually hidden
behind the dma api or in helpers.
--
Jens Axboe
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 13 Jul 2007, James Bottomley wrote:
On Fri, 2007-07-13 at 17:10 +0200, Geert Uytterhoeven wrote:
On Fri, 13 Jul 2007, Arnd Bergmann wrote:
On Friday 13 July 2007, James Bottomley wrote:
IC.
- flush_kernel_dcache_page() is a no-op on ppc64
(ARCH_HAS_FLUSH_KERNEL_DCACHE_PAGE is defined on parisc only).
- For reference, drivers/scsi/ipr.c (another ppc64 driver) just uses
a plain
kmap/memcpy/kunmap sequence
So what should I do?
Ask someone who knows the architecture ... Anton, Paulus or Benh ... I'm
fairly certain PPC is VIPT and will need some kind of alias
resolution ... perhaps its associative enough not to let the aliases be
a problem.
I'm pretty sure that no ppc64 machine needs alias resolution in the
kernel,
although some are VIPT. Last time we discussed this, Segher explained it
to me, but I don't remember which way Cell does it. IIRC, it automatically
flushes cache lines that are accessed through aliases.
Thanks for confirming!
It's probably a good idea to have the flush_kernel_dcache_page() in there
anyway, if only to serve as an example for people that copy it into
architecture-independent drivers, same as what we do for the
k{,un}map_atomic() that is also not required on ppc64.
Now my next question: why should I add it, if currently no single driver in
mainline calls flush_kernel_dcache_page()?
`git grep' finds it in the following files only:
Documentation/cachetlb.txt
arch/parisc/kernel/cache.c
arch/parisc/kernel/pacache.S
include/asm-parisc/cacheflush.h
include/linux/highmem.h
It's a recent addition to the API ... the previous way of doing it was
flush_dcache_page() but that's expensive and flushes all the user
aliases. Since you know in this case there are no current user aliases
and you only need to flush the kernel alias, flush_kernel_dcache_page()
was introduced as the cheaper alternative.
Ah, that explains it. flush_dcache_page() is used in some drivers.
I'll update my patches. Thanks for the comments!
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone:+32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [EMAIL PROTECTED]
Internet: http://www.sony-europe.com/
Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 2007-07-13 at 09:02 -0400, James Bottomley wrote:
On Wed, 2007-07-04 at 15:22 +0200, Geert Uytterhoeven wrote:
+ kaddr = kmap_atomic(sgpnt-page, KM_USER0);
+ if (!kaddr)
+ return -1;
+ len = sgpnt-length;
+ if ((req_len + len) buflen) {
+ active = 0;
+ len = buflen - req_len;
+ }
+ memcpy(kaddr + sgpnt-offset, buf + req_len,
len);
+ kunmap_atomic(kaddr, KM_USER0);
This isn't a SCSI objection, but this sequence appears several times in
this driver. It's wrong for a non-PIPT architecture (and I believe the
PS3 is VIPT) because you copy into the kernel alias for the page, which
dirties the line in the cache of that alias (the user alias cache line
was already invalidated). However, unless you flush the kernel alias to
main memory, the user could read stale data. The way this is supposed
to be done is to do a
Nah, we have no cache aliasing on ppc, at least not that matter here and
not on cell.
Ben.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/6] ps3: BD/DVD/CD-ROM Storage Driver
On Fri, 2007-07-13 at 16:19 +0200, Arnd Bergmann wrote: I'm pretty sure that no ppc64 machine needs alias resolution in the kernel, although some are VIPT. Last time we discussed this, Segher explained it to me, but I don't remember which way Cell does it. IIRC, it automatically flushes cache lines that are accessed through aliases. Ah yes, I remember reading about this automatic flushing thing. I don't know how the caches actually work on most of our PPC's, but the fact is, we don't have aliasing issues, so I can safely ignore it for a bit longer :-) There are some aliasing issues with the instruction cache specifically on some 4xx models but that's irrelevant to this discussion (and I think we handle them elsewhere anyway). Ben. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
