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: PS3 Storage Driver O_DIRECT issue
On Fri, 13 Jul 2007, Olaf Hering wrote: This driver (or the generic PS3 code) has appearently problems with O_DIRECT. glibc aborts parted because the malloc metadata get corrupted. While it is reproducible, the place where it crashes changes with every version of the debug attempt. I dont have a handle right now, all I know is that the metadata after a malloc area get overwritten with zeros. Can you have a look at this? parted /dev/ps3da print (a few times) I can't seem to reproduce this with parted 1.7.1-5.1 (from Debian etch/lenny/sid) and kernel 2.6.22-g77320894. 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 0/6] PS3 Storage Drivers for 2.6.23, take 4
On Tue, 10 Jul 2007, Geert Uytterhoeven wrote: On Wed, 4 Jul 2007, Geert Uytterhoeven wrote: This is the fourth submission of the new PS3 storage drivers: [1] ps3: Preallocate bootmem memory for the PS3 FLASH ROM storage driver [2] ps3: Storage Driver Core [3] ps3: Storage device registration routines. [4] ps3: Disk Storage Driver [5] ps3: BD/DVD/CD-ROM Storage Driver [6] ps3: FLASH ROM Storage Driver They are based on: - the current Linux kernel source tree, - plus the PS3 patches already submitted by Geoff Levand on linuxppc-dev, - plus the shost_priv() patch in scsi-misc (commit bcd92c9fbcc679ee95003083056f0441a1f474fa). All issues raised during the previous review rounds should be fixed. There were no more comments after the third submission. Paul already integrated Geoff Levand's PS3 patches and the first 3 patches in this series in the for-2.6.23 branch in powerpc.git. Maintainers of block/SCSI/misc, can you please ack the last 3 patches so Paul can take them, too? Ping block/SCSI/misc maintainers? Ping SCSI and misc maintainers? Please either ack patches 5 and 6, so Paulus can integrate them for 2.6.23, or tell me why they can't go in. Thanks 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 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: mptsas drops then re-adds hard drive
Moore, Eric [EMAIL PROTECTED] writes: The [EMAIL PROTECTED] has been deleted, and replaced with distribution list. This was done about a month back, and should be updated in the MAINTAINERS file by now. It was not updated in the 2.6.22's MAINTAINERS file. A return code of 0x1000 means DID_NO_CONNECT. Do you have the entire dmesg log, as it would be interesting to know wether the driver recieved asyn events from the firmware notifing that the device has been deleted and/or added back. The link is going down over load testing, most likely firmware issue (controller or SATA device). Are you creating RAID using integrated RAID provided with the fusion controller, or are you using linux software RAID? Which company do you represent, or are you an end user? I would need to get this assinged to an Systems Engineer inorder to help with replication efforts, and/or analyzing bus traces. Eric, Thanks a lot for your reply. I am glad to see that LSI is standing behind its products and its Linux drivers. In the end, after a lot of troubleshooting and hardware swapping, the hard drives and the controller have been ruled out. It turned out that the problem was with the server's power supply. It was supplying a very wobbly and low 5V between 4.3 and 4.7V. The Seagate hard drives were not liking it and were resetting I suppose. We could hear the click from the heads parking and the drive recalibrating. For the record, the PSU was an Antec TruePower (ahem) 550W. Which should have been plenty of ooomph for the server and 8 drives. A power meter plugged onto the box showed a steady 280W power consumption with peaks at 320W when the drives were spinning up (staggered). It is the second Antec PSU that we've seen crapping out in this way, and my personal opinion and experience are that Antec PSUs are prone to do this. I would not recommend Antec. Again thank you very much for your reply. It is appreciated. Phil. - 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] mpt fusion: add support for Brocade branded LSI FC HBA
On Friday, July 13, 2007 3:29 AM, Prakash, Sathya wrote:
You need to include in this patch, the fix that occurred between the
4.00.09 and 4.00.10 drivers. That fix is in mptDisplayIocCapabilties,
where it was removing the first three characters from the prod_name.
Without this change, 040 would be displayed instead of BRE040.
Here are some additional request:
+mpt_get_product_name(u16 vendor, u16 device, u8 revision,
char *prod_name)
+{
+ char *product_str = NULL;
+
+ if (vendor == 0x1657) {
+ switch (device)
You should use the define PCI_VENDOR_ID_BROCADE instead of the hardcoded
0x1657 value.
+ if (vendor != PCI_VENDOR_ID_LSI_LOGIC)
+ goto out;
This sanity check on PCI_VENDOR_ID_LSI_LOGIC needs to be removed. I
will be required in the pending ATTO UL4D patch.
-
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
