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
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, 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: [dm-devel] dm-mpath-rdac.patch problem
Andrew Vasquez wrote: > On Thu, 12 Jul 2007, Mike Anderson wrote: > >> Copying this mail to linux-scsi and Ccing Andrew Vasquez to possibly >> provide input on the Qlogic behavior. >> >> Chandra Seetharaman <[EMAIL PROTECTED]> wrote: >>> On Thu, 2007-07-12 at 18:35 -0700, Brian De Wolf wrote: Hello All, I'm not sure if this is the right place for this, but it seems to be the only mailing list related to dm, multipath, and rdac, as far as I can tell. I've been trying out the dm-mpath-rdac patch (both yesterday's and previous) with gentoo's unstable 2.6.22 kernel, on a Sun x4100 through a QLA2422 HBA (firmware ql2400_fw.bin.4.00.27) to an IBM DS4000. I am using a version of multipath-tools that I got with git a few days ago. I've got multipath working, it reports the hwhandler correctly ([hwhandler=1 rdac]), and the volume is mountable, etc. It also shows one link as active, the other as ghost. However, once the active link dies, the volume becomes read only, and both connections are listed as failed. Most importantly, something like this shows up in the logs: Jul 12 17:11:15 jimbo kernel: device-mapper: multipath rdac: queueing MODE_SELECT command on 8:32 >>> It does look like the rdac hardware handler is doing the right thing and >>> the qlogic is dying for some reason. >>> >>> I have tested this code in both RHEL5 and SLES10 environments (qla23xx) >>> and they work fine. Can you try in one of those and see if it is any >>> different. >>> >>> Just an FYI w.r.t multipath tools: please remove the patch >>> http://git.kernel.org/?p=linux/storage/multipath- >>> tools/.git;a=commit;h=e1e1a1bfb2cf76bfd1a49335e3deec5360fb09db from your >>> tree for the tools to calculate the path priorities properly. >>> >>> Jul 12 17:11:15 jimbo kernel: qla2xxx :02:01.1: ISP System Error - mbx1=0h mbx2=8012h mbx3=8002h. Jul 12 17:11:15 jimbo kernel: qla2xxx :02:01.1: Firmware has been previously dumped (c2000171d000) -- ignoring request... Jul 12 17:11:16 jimbo kernel: qla2xxx :02:01.1: Performing ISP error recovery - ha= 81007e85c530. > > Hmm yes, there's some real problems going on within the firmware which > we need to triage. From the snippet above, the driver was able to > capture a firmware-dump of a failure (not sure of the timing and how > it relates to the window in which you recognized a 'problem'), but > I'll need to to 'capture' the firmware trace and forward it along to > us to inspect. > > 1) download the following shell script: > > ftp://ftp.qlogic.com/outgoing/linux/beta/8.x/test/qla_dmp.sh > > 2) copy the script to the host (/tmp) which is experiencing the >problems. > > 3) reboot and load the driver with the ql2xextended_error_logging >module parameter set to 1. e.g.: > > $ insmod qla2xxx.ko ql2xextended_error_logging=1 > > 4) rerun your test and monitor the kernel-messages file for a message >similar to: > > Firmware dump saved to temp buffer (1/adcdabcd) > > 5) To retrieve the dump, go to a console and type the following: > > # cd /tmp/ > # ./qla_dmp.sh 1 > >The value passed to qla_dmp.sh should be the same as the first integer >in the 'saved to temp buffer' string (in this example, 1). If the >operation was successful, a message like to following should be >displayed: > > Firmware dumped to file fw_dump_1_20041217_023222.txt.gz > >Formward the >forward over the file. > > 6) forward over the /var/log/messages file of the driver load and >failure snippet. > > > Not sure which firmware version you are running, but an additional > datapoint which may be useful after you send the firmware-dump is to > download the latest 24xx firmware file from QLogic.com: > > ftp://ftp.qlogic.com/outgoing/linux/firmware/ql2400_fw.bin > > and retry the test. If you still see problems, and see a similar > 'Firmware dump saved...' messages. Follow the steps above again and > forward the same datapoints. > I have tried both the ql2400_fw.bin.4.00.18 and ql2400_fw.bin.4.00.27 firmwares and the HBA had the same error. The attached datapoints were done using ql2400_fw.bin.4.00.27. Note: This is a resend to the mailing list without attachments. While this may be something for the maintainer of the qla2xxx module (I can't figure out where I'd send it, in that case...) I think it may be of interest that the dm_rdac module tries to push something over the HBA that causes it to bail completely and start from scratch (it starts init processes and loading firmware again). Not to say that I'm not interested in any help getting this working, that is. If you have any suggestions on how to get this working, I'd love to hear them. I'm also willing to gui
RE: [PATCH] mpt fusion: add sysfs attributes to display IOC parameters
On Thursday, July 12, 2007 10:11 PM, Prakash, Sathya wrote: > > Resubmitting with the following change suggested by Brian King: > The driver version from scsi_host attribute is redundant as > it is already > available in sysfs: /sys/module/mptscsih/version. > So it is removed from the patch > > --- > New sysfs scsi_host attributes are added to provide > information about Firmware > version, BIOS version, MPI version and other product related > information. > > signed-off-by: Sathya Praksh <[EMAIL PROTECTED]> > ACK, please apply. Eric Moore - 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] add PCI_VENDOR_ID macro for Brocade in pci_ids.h
Sathya, Sorry for the previous comment about pci_ids.h, I did not see you sent 2 patches. Gwendal. > -Original Message- > From: Prakash, Sathya [mailto:[EMAIL PROTECTED] > Sent: Friday, July 13, 2007 2:14 AM > To: linux-scsi@vger.kernel.org > Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; Gwendal Grignou > Subject: [PATCH] add PCI_VENDOR_ID macro for Brocade in pci_ids.h > > Adds PCI_VENDOR_ID_BROCADE macro in include/linux/pci_ids.h file. This > macro > is used in MPT Fusion FC drivers to support Brocade branded FC controllers > > signed-off-by: Sathya Prakash <[EMAIL PROTECTED]> > > --- > > diff -Naurp b/include/linux/pci_ids.h a/include/linux/pci_ids.h > --- b/include/linux/pci_ids.h 2007-07-02 16:20:47.0 +0530 > +++ a/include/linux/pci_ids.h 2007-07-13 11:34:32.0 +0530 > @@ -2043,6 +2043,8 @@ > > #define PCI_VENDOR_ID_ARIMA 0x161f > > +#define PCI_VENDOR_ID_BROCADE0x1657 > + > #define PCI_VENDOR_ID_SIBYTE 0x166d > #define PCI_DEVICE_ID_BCM1250_PCI0x0001 > #define PCI_DEVICE_ID_BCM1250_HT 0x0002 - 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: 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: [dm-devel] dm-mpath-rdac.patch problem
On Thu, 12 Jul 2007, Mike Anderson wrote: > Copying this mail to linux-scsi and Ccing Andrew Vasquez to possibly > provide input on the Qlogic behavior. > > Chandra Seetharaman <[EMAIL PROTECTED]> wrote: > > On Thu, 2007-07-12 at 18:35 -0700, Brian De Wolf wrote: > > > Hello All, > > > > > > I'm not sure if this is the right place for this, but it seems to be the > > > only > > > mailing list related to dm, multipath, and rdac, as far as I can tell. > > > I've > > > been trying out the dm-mpath-rdac patch (both yesterday's and previous) > > > with > > > gentoo's unstable 2.6.22 kernel, on a Sun x4100 through a QLA2422 HBA > > > (firmware > > > ql2400_fw.bin.4.00.27) to an IBM DS4000. I am using a version of > > > multipath-tools that I got with git a few days ago. > > > > > > I've got multipath working, it reports the hwhandler correctly > > > ([hwhandler=1 > > > rdac]), and the volume is mountable, etc. It also shows one link as > > > active, the > > > other as ghost. However, once the active link dies, the volume becomes > > > read > > > only, and both connections are listed as failed. Most importantly, > > > something > > > like this shows up in the logs: > > > > > > Jul 12 17:11:15 jimbo kernel: device-mapper: multipath rdac: queueing > > > MODE_SELECT command on 8:32 > > > > It does look like the rdac hardware handler is doing the right thing and > > the qlogic is dying for some reason. > > > > I have tested this code in both RHEL5 and SLES10 environments (qla23xx) > > and they work fine. Can you try in one of those and see if it is any > > different. > > > > Just an FYI w.r.t multipath tools: please remove the patch > > http://git.kernel.org/?p=linux/storage/multipath- > > tools/.git;a=commit;h=e1e1a1bfb2cf76bfd1a49335e3deec5360fb09db from your > > tree for the tools to calculate the path priorities properly. > > > > > > > Jul 12 17:11:15 jimbo kernel: qla2xxx :02:01.1: ISP System Error - > > > mbx1=0h > > > mbx2=8012h mbx3=8002h. > > > Jul 12 17:11:15 jimbo kernel: qla2xxx :02:01.1: Firmware has been > > > previously > > > dumped (c2000171d000) -- ignoring request... > > > Jul 12 17:11:16 jimbo kernel: qla2xxx :02:01.1: Performing ISP error > > > recovery - ha= 81007e85c530. Hmm yes, there's some real problems going on within the firmware which we need to triage. From the snippet above, the driver was able to capture a firmware-dump of a failure (not sure of the timing and how it relates to the window in which you recognized a 'problem'), but I'll need to to 'capture' the firmware trace and forward it along to us to inspect. 1) download the following shell script: ftp://ftp.qlogic.com/outgoing/linux/beta/8.x/test/qla_dmp.sh 2) copy the script to the host (/tmp) which is experiencing the problems. 3) reboot and load the driver with the ql2xextended_error_logging module parameter set to 1. e.g.: $ insmod qla2xxx.ko ql2xextended_error_logging=1 4) rerun your test and monitor the kernel-messages file for a message similar to: Firmware dump saved to temp buffer (1/adcdabcd) 5) To retrieve the dump, go to a console and type the following: # cd /tmp/ # ./qla_dmp.sh 1 The value passed to qla_dmp.sh should be the same as the first integer in the 'saved to temp buffer' string (in this example, 1). If the operation was successful, a message like to following should be displayed: Firmware dumped to file fw_dump_1_20041217_023222.txt.gz Formward the forward over the file. 6) forward over the /var/log/messages file of the driver load and failure snippet. Not sure which firmware version you are running, but an additional datapoint which may be useful after you send the firmware-dump is to download the latest 24xx firmware file from QLogic.com: ftp://ftp.qlogic.com/outgoing/linux/firmware/ql2400_fw.bin and retry the test. If you still see problems, and see a similar 'Firmware dump saved...' messages. Follow the steps above again and forward the same datapoints. > > > While this may be something for the maintainer of the qla2xxx module (I > > > can't > > > figure out where I'd send it, in that case...) I think it may be of > > > interest > > > that the dm_rdac module tries to push something over the HBA that causes > > > it to > > > bail completely and start from scratch (it starts init processes and > > > loading > > > firmware again). > > > > > > Not to say that I'm not interested in any help getting this working, that > > > is. > > > If you have any suggestions on how to get this working, I'd love to hear > > > them. > > > I'm also willing to guinea pig some testing if you need it (This box > > > still has a > > > bit before it will have to be put in use). I may use redhat to ensure > > > that it's > > > not just a broken HBA, but for the long run we would like it to join our > > > gentoo > > > environment. > > > > > > Thanks! > > > Brian De Wolf >
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, 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
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, Jul 13 2007, Geert Uytterhoeven wrote:
> On Fri, 13 Jul 2007, Jens Axboe wrote:
> > 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.
>
> 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.
If you have the kmap sequence there, put the flush in as well. People
copy code, you know... Or put a big comment explaining why it isn't
needed.
> > Well, even worse is that fact that it's using KM_USER0 from interrupt
> > context.
>
> So should I replace it by e.g. KM_IRQ0?
> I'm not so familiar with these parts, and I couldn't find what these values
> really mean.
You corrupt data, using KM_USER0 from interrupt context. So it's a big
flaw right now. Use KM_IRQ0 for code where interrupts are always
disabled.
--
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 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.
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.
Arnd <><
-
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 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, 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: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. 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: 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, 13 Jul 2007, Jens Axboe wrote:
> 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.
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.
> Well, even worse is that fact that it's using KM_USER0 from interrupt
> context.
So should I replace it by e.g. KM_IRQ0?
I'm not so familiar with these parts, and I couldn't find what these values
really mean.
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, 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
PS3 Storage Driver O_DIRECT issue
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) - 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
