RE: Patch added to scsi-rc-fixes-2.6: [SCSI] arcmsr: fix messageallocation
James, Appreciate for your answer. One more question: do you think there are any compatibility issues on kmalloc/kfree pair? I just trace the kernel code. It looks like it would not. But I am not quite sure absolutely. May I have your comments? Thank you, -Original Message- From: James Bottomley [mailto:[EMAIL PROTECTED] Sent: Monday, February 25, 2008 10:39 PM To: [EMAIL PROTECTED] Cc: 'Daniel Drake'; linux-scsi@vger.kernel.org; 'erich' Subject: RE: Patch added to scsi-rc-fixes-2.6: [SCSI] arcmsr: fix messageallocation On Mon, 2008-02-25 at 18:47 +0800, nickcheng wrote: > Sorry, maybe I did not ask distinctly enough. > I mean if I would like to allocate a memory space from ZONE_DMA for atomic > context, why can I not use kmalloc(1032, GFP_ATOMIC|GFP_DMA)? > In case of lack of GFP_DMA, kmalloc would grab the memory from ZONE_HIGH or > ZONE_HIGHMEM, isn't it?(I read it from the textbook of Linux Kernel > Development by Robert Love) Um, no that's not true at all. GFP_DMA only allocates memory from ZONE_DMA and fails otherwise. You only need memory from ZONE_DMA if you cannot address physical memory above 24 bits (the old ISA restriction). This only applies if you're a standard ISA device and you're going actually to DMA to the memory in question. Neither of which applies in the arcmsr case, since you're only using the memory as a copy buffer within the kernel (it never sees an actual DMA transfer), and arcmsr doesn't have the ISA restrictions. > Or the basic is that you don't think it is necessary to allocate a memory > space from DMA area? > Please give me some comments. It's unnecessary because you never DMA to it, but even if you did, since arcmsr is a non-ISA device (with 64 bit DMA mask falling back to 32) you can just use ordinary kmalloc'd memory for that (provided you obey the coherence requirements). 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] [3/22] Remove unchecked_isa_dma in advansys.c
On Mon, Feb 25, 2008 at 03:50:22PM -0700, Matthew Wilcox wrote: > On Mon, Feb 25, 2008 at 11:40:35PM +0100, Andi Kleen wrote: > > > (I must have fixed it somehow because it works on parisc, which is most > > > unforgiving of drivers which do DMA without the DMA API). > > > > At least on x86 the DMA API cannot do ISA bouncing. > > You're saying that if I set a 24-bit DMA mask, and then do a > pci_alloc_coherent(), x86 might hand me back something that's not > accessible? That would be just broken. No pci_alloc_coherent works, but pci_map_* will not. -Andi - 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: Aborted commands with arcmsr and 2xWD1500ADFD in RAID1
Hi Aron, Thanks for your patience. If you still got into trouble, please let me know. Thank you again, -Original Message- From: Aron Stansvik [mailto:[EMAIL PROTECTED] Sent: Tuesday, February 26, 2008 6:52 AM To: erich Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; linux-scsi@vger.kernel.org; [EMAIL PROTECTED] Subject: Re: Aborted commands with arcmsr and 2xWD1500ADFD in RAID1 Hi Erich. 2008/2/25, nickcheng <[EMAIL PROTECTED]>: > Hi Aron, > From our field experiences and customers' feedbacks, all of them direct to > vibration and power issues. > The vibration could be caused by FANs not only by themselves. Okay. I have a chassi fan that is quite close to the drives, I will try disabling it. I've also ordered two Nexus TwinDisk anti vibration harddrive mounts with which I'll place the disks in my 5.25" slots instead, away from any fans. If this doesn't work, I'm stumped, as I really don't think it's the power supply and I don't have the money to buy a new one. > You mentioned it could be the F/W issue. > If the environment does not meet the prerequisite, FW could not work > correctly. > Actually FW just reacts to the situations not it causes the issue. Of course, I understand this. Just trying to figure this problem out.. > Please check it out!! I'll report back with my findings with moving disk away from fans and using anti-vibrations mounts. Thanks for taking your time to reply. Aron > Thank you, > > > -Original Message- > From: Aron Stansvik [mailto:[EMAIL PROTECTED] > Sent: Sunday, February 24, 2008 1:54 AM > To: [EMAIL PROTECTED] > Cc: erich; [EMAIL PROTECTED]; linux-scsi@vger.kernel.org; > [EMAIL PROTECTED] > > Subject: Re: Aborted commands with arcmsr and 2xWD1500ADFD in RAID1 > > Hello again Areca and LKML hackers. > > 2008/2/18, Aron Stansvik <[EMAIL PROTECTED]>: > > Hello Nick. > > > > Sorry that I'm not answering until now. I've been busy. > > > > 2008/2/13, nickcheng <[EMAIL PROTECTED]>: > > > > > Hi Aron, > > > From our experience and some customers' feedback, your issue could be > caused > > > by power instability or vibration to your HDs. > > > Please check step by step: > > > (1).under your original environment, increase the SCSI command value, > > > default=30, with the shell script, set_scsicmd_timeout(). 90 or 120 is > > > enough. > > > (2).if method 1 does not work, find out the vibration source or change > the > > > power supply > > > > > > I will try to increase that value. I don't think it's vibration; the > > disks are firmly in place in a very heavy chassi (Silverstone > > SST-TJ05B-T). And I really don't think there's something wrong with > > the power supply, it's a pretty new Silverstone ST65ZF 650W. This is > > my own personal workstation, so I don't just have another power supply > > to test with :/ > > > > I will report back on my success/failure. Thanks for your answer. > > I've now tried with both 90 and 120 for the timeout value, and the > problem still persists. It seems to happen when lots of small writes > are occuring, e.g. when installing something. > > I really don't think the disks are vibrating, I don't see how they > could. One more thing I'm going to test is to use the legacy ATA power > connector instead of the SATA power connector. This was what I was > using before when I only had a single drive and no RAID controller. > Maybe my power supply is malfunctioning and not giving enough power on > the SATA power connectors.. but I doubt it. > > Is there anything else that could cause this? Have you guys at Areca > tested the ARC-1200 with Raptors in RAID1? > > :( > > Regards, > Aron > > > > > > > Aron > > > > > > > If your still have any questions, please feel free to let me know. > > > > > > P.S. The attached driver source, arcmsr-1.20.00.15-71224, has been > > > upstreamed to kernel.org and will be released in kernel 2.6.25. If you > like, > > > you could update your driver with it. > > > It fixes some minor bugs, but these bugs are nothing to do with your > issue. > > > > > > > > > -Original Message- > > > From: erich [mailto:[EMAIL PROTECTED] > > > Sent: Wednesday, February 13, 2008 4:33 PM > > > To: (廣安科技)鄭守謙 > > > Subject: Fw: Aborted commands with arcmsr and 2xWD1500ADFD in RAID1 > > > > > > > > > > > > - Original Message - > > > From: "Andrew Morton" <[EMAIL PROTECTED]> > > > To: "Aron Stansvik" <[EMAIL PROTECTED]> > > > Cc: <[EMAIL PROTECTED]>; ; > "erich" > > > <[EMAIL PROTECTED]> > > > Sent: Wednesday, February 13, 2008 4:03 PM > > > Subject: Re: Aborted commands with arcmsr and 2xWD1500ADFD in RAID1 > > > > > > > > > > > > > > (cc's added) > > > > > > > > On Mon, 11 Feb 2008 17:44:08 +0100 "Aron Stansvik" > <[EMAIL PROTECTED]> > > > > wrote: > > > > > > > >> Hello LKML. > > > >> > > > >> Under semi-high disk I/O (e.g. installing a co
[PATCH 1/1] cciss: remove READ_AHEAD define and use block layer defaults
PATCH 1/1
This patch removes the #define READ_AHEAD 1024 from the driver and uses the
block layer defaults, instead. We have found that under certain workloads
the setting can cause a disk connected to the e200 controller to go offline.
If the disk hiccups the link may try to downshift but the controller is
never notified that the link successfully completed the renegotiation.
We've also found that performance using the block layer default of 32 pages
was on par with the 1024 setting. We tried setting it to zero at one time
based on info from our firmware guys but that killed performance. Turns out
we were talking about 2 different read ahead settings.
Please consider this for inclusion.
Signed-off-by: Mike Miller <[EMAIL PROTECTED]>
diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 9715be3..2f6cc3b 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -131,7 +131,6 @@ static struct board_type products[] = {
/*define how many times we will try a command because of bus resets */
#define MAX_CMD_RETRIES 3
-#define READ_AHEAD 1024
#define MAX_CTLR 32
/* Originally cciss driver only supports 8 major numbers */
@@ -1341,7 +1340,6 @@ geo_inq:
disk->private_data = &h->drv[drv_index];
/* Set up queue information */
- disk->queue->backing_dev_info.ra_pages = READ_AHEAD;
blk_queue_bounce_limit(disk->queue, hba[ctlr]->pdev->dma_mask);
/* This is a hardware imposed limit. */
@@ -3434,7 +3432,6 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
}
drv->queue = q;
- q->backing_dev_info.ra_pages = READ_AHEAD;
blk_queue_bounce_limit(q, hba[i]->pdev->dma_mask);
/* This is a hardware imposed limit. */
-
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
[Bug 7246] 3w-xxxx, IOMMU and >1go RAM
http://bugzilla.kernel.org/show_bug.cgi?id=7246 [EMAIL PROTECTED] changed: What|Removed |Added Status|NEW |CLOSED Resolution||PATCH_ALREADY_AVAILABLE --- Comment #14 from [EMAIL PROTECTED] 2008-02-25 16:20 --- Thanks, Adam - closing the bug. -- Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee. - 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] libsas: Don't issue commands to devices that have been hot-removed.
(digging through old email) Darrick J. Wong wrote: On Tue, Dec 04, 2007 at 05:48:33PM -0500, Jeff Garzik wrote: As an aside, issues like this really really imply a need to move libsas away from the old libata EH stuff (like brking did with ipr, in patches). Hm... does the new libata EH handle the case of "device was unplugged, don't bother trying to send any more commands"? Yes, most certainly :) We wouldn't have hotplug support without that... In general, I agree that sas-ata should adopt the new EH. Unfortunately, I believe the old way of sas-ata configuring ATA ports is somehow not compatible with the new EH stuff and causes a crash during the device probe with my patch to move sas-ata to the new EH. If I apply the patch that migrates sas-ata to use brking's latest ata-sas configuration mechanism (the one that creates real ata_hosts), I see (a) lots and lots of ATA hosts getting created (one per ATA port; possibly undesirable if you've a SAS topology with a lot of SATA disks) and (b) NCQ disks don't seem to work if you unplug the disk and plug it back in (unless NCQ is disabled entirely). Jeff, by any chance have you tried plugging SATA devices into your SAS controllers? Just tested mvsas here... James Bottomley wondered if it would be easier to have sas-ata call only into the parts of libata that convert SCSI commands to ATA taskfiles, though I'm unsure how many wormy cans that would open. Like Brian K noted, libata-EH is heavily involved in "anything not hotpath read/write", including but not limited to: PMP, hotplug, device probing, device revalidation, explicit sequencing of ATA commands during initialization (critical for getting many ATA devices working) You don't want to reinvent or duplicate all those ATA device initialization/revalidation quirks. Jeff - 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] mvsas: convert from rough draft to working driver
Ke Wei wrote: On Sat, Feb 23, 2008 at 11:28:50AM -0500, Jeff Garzik wrote: Ke Wei wrote: Convert rough draft Marvell 6440 driver to a working driver. Added support for SAS and SATA devices, hotplug, wide port, and expanders. This patch is based on: branch 'mvsas' of git.kernel.org:/pub/scm/linux/kernel/git/jejb/scsi-misc-2.6.git Yay! Perfect patch: good tech content, applyable to #mvsas, and the most important part, the driver works :) One minor (though important) detail: may we assume this patch has the same Signed-off-by as previous patches? Oah, I have no idea how to diff between the previous 5th commit and current to use git-format-patch command, so I had to use git-diff to create patch. As a result, I format to place my Signed-off-by when sending email. Jeff, Do you have a good suggestion? Thank you for your help. That's just a simple mistake, so I wouldn't worry about it. We all forget such things, every now and then. If I forget to sign-off on a patch, I usually just reply to my own email (publicly), providing the Signed-off-by line requested... which is precisely what you did below :) Signed-off-by: Ke Wei <[EMAIL PROTECTED]> Thanks! Jeff - 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] [3/22] Remove unchecked_isa_dma in advansys.c
On Mon, 2008-02-25 at 17:54 -0500, Jeff Garzik wrote: > Matthew Wilcox wrote: > > On Mon, Feb 25, 2008 at 11:40:35PM +0100, Andi Kleen wrote: > >>> (I must have fixed it somehow because it works on parisc, which is most > >>> unforgiving of drivers which do DMA without the DMA API). > >> At least on x86 the DMA API cannot do ISA bouncing. > > > > You're saying that if I set a 24-bit DMA mask, and then do a > > pci_alloc_coherent(), x86 might hand me back something that's not > > accessible? That would be just broken. > > Indeed. It's doing the right thing (from pci-dma_32.c): void *dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle, gfp_t gfp) [...] if (dev == NULL || (dev->coherent_dma_mask < 0x)) gfp |= GFP_DMA; ret = (void *)__get_free_pages(gfp, order); Which correctly allocates the region. 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] [3/22] Remove unchecked_isa_dma in advansys.c
Matthew Wilcox wrote: On Mon, Feb 25, 2008 at 11:40:35PM +0100, Andi Kleen wrote: (I must have fixed it somehow because it works on parisc, which is most unforgiving of drivers which do DMA without the DMA API). At least on x86 the DMA API cannot do ISA bouncing. You're saying that if I set a 24-bit DMA mask, and then do a pci_alloc_coherent(), x86 might hand me back something that's not accessible? That would be just broken. Indeed. Jeff - 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] [3/22] Remove unchecked_isa_dma in advansys.c
On Mon, Feb 25, 2008 at 11:40:35PM +0100, Andi Kleen wrote: > > (I must have fixed it somehow because it works on parisc, which is most > > unforgiving of drivers which do DMA without the DMA API). > > At least on x86 the DMA API cannot do ISA bouncing. You're saying that if I set a 24-bit DMA mask, and then do a pci_alloc_coherent(), x86 might hand me back something that's not accessible? That would be just broken. -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." - 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] [3/22] Remove unchecked_isa_dma in advansys.c
On Mon, Feb 25, 2008 at 02:47:46PM -0700, Matthew Wilcox wrote: > On Mon, Feb 25, 2008 at 12:35:16AM +0100, Andi Kleen wrote: > > That patch is a little more complicated than the others. advansys > > was the only ISA driver who actually passed ->cmnd to the firmware. > > So I implemented a simple own bounce buffer scheme for this case. > > Also did sense_buffer bouncing in the driver while I was at it; > > which means it doesn't require the mid layer to do this anymore. > > I have to say I really don't like this patch. I'll look up my current Why do you not like it? What would you do better? > patch queue for this driver next week -- I think I fixed this differently. > (I must have fixed it somehow because it works on parisc, which is most > unforgiving of drivers which do DMA without the DMA API). At least on x86 the DMA API cannot do ISA bouncing. -Andi - 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] [3/22] Remove unchecked_isa_dma in advansys.c
On Mon, Feb 25, 2008 at 12:35:16AM +0100, Andi Kleen wrote: > That patch is a little more complicated than the others. advansys > was the only ISA driver who actually passed ->cmnd to the firmware. > So I implemented a simple own bounce buffer scheme for this case. > Also did sense_buffer bouncing in the driver while I was at it; > which means it doesn't require the mid layer to do this anymore. I have to say I really don't like this patch. I'll look up my current patch queue for this driver next week -- I think I fixed this differently. (I must have fixed it somehow because it works on parisc, which is most unforgiving of drivers which do DMA without the DMA API). -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." - 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 1/1] scsi: megaraid_sas - Fix random failure of DCDBcmds with sense info
James, Thanks for the reply. We will resubmit the patch to rollback it to our original implementation. Regards. Bo Yang -Original Message- From: James Bottomley [mailto:[EMAIL PROTECTED] Sent: Friday, February 22, 2008 11:36 AM To: Yang, Bo Cc: Matthew Wilcox; linux-scsi@vger.kernel.org; [EMAIL PROTECTED]; [EMAIL PROTECTED]; Patro, Sumant; Kolli, Neela Subject: RE: [PATCH 1/1] scsi: megaraid_sas - Fix random failure of DCDBcmds with sense info On Fri, 2008-02-22 at 09:17 -0700, Yang, Bo wrote: > James, > > What is the status for this patch? We need to submit more patches > based on the acceptance of this patch. OK, read the thread; Matthew is right. What you propose would pretty much destroy compat ioctl handling within the driver. You need a compat handler for MEGASAS_IOC_FW. With your current patch you'd get a failure both from a 64 bit binary running on x86-64 and if someone ran the x86 binary on ia64. The driver already uses the compat infrastructure, it shouldn't be too hard to add this in the correct manner. 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
[Bug 7246] 3w-xxxx, IOMMU and >1go RAM
http://bugzilla.kernel.org/show_bug.cgi?id=7246 [EMAIL PROTECTED] changed: What|Removed |Added CC||[EMAIL PROTECTED] --- Comment #13 from [EMAIL PROTECTED] 2008-02-25 11:54 --- Javier, The final issue you are talking about (different from original issue in this bugzilla entry): The 3w- driver calling pci_map_sg() with sc_data_direction == DMA_BIDIRECTIONAL caused data corruption when going through swiotlb.c, on EM64T with 4GB or higher of RAM. AMD64 systems using IOMMU were never affected. This problem doesn't exist in the 3w- driver in kernels 2.6.23 and higher. The reason is the 'scsi data buffer accessors' patches removed most instances of scsi drivers calling pci_map_sg() and replaced them with scsi_dma_map(). This corrected the problem of the 3w- driver over-riding the default sc_data_direction that was causing data corruption with EM64T systems with 4GB+ RAM. If you need a driver update for an older kernel to fix this issue, please go to www.3ware.com, however no driver patch to 3w- needs to be sent to the kernel tree to fix this issue. Please close this BUG. -Adam Radford -- Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee. - 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
[PATCH] Fixed build failure when using -j option of make.
This patch removed make dependency on any other .o files which were causing build failure when make -j 10 used. So moved other .o related code under openfc dir as these .o files were used only by openfc module. Signed-off-by: Vasu Dev <[EMAIL PROTECTED]> --- drivers/scsi/ofc/Makefile|3 drivers/scsi/ofc/fcoe/Makefile |2 drivers/scsi/ofc/libcrc/Makefile |9 drivers/scsi/ofc/libcrc/crc32_le.c | 149 -- drivers/scsi/ofc/libcrc/crc32_le_tab.c | 594 drivers/scsi/ofc/libfc/Makefile | 20 drivers/scsi/ofc/libfc/fc_disc_targ.c| 655 - drivers/scsi/ofc/libfc/fc_disc_targ.h| 48 - drivers/scsi/ofc/libfc/fc_exch.c | 1560 -- drivers/scsi/ofc/libfc/fc_exch_impl.h| 141 -- drivers/scsi/ofc/libfc/fc_fcip.h | 131 -- drivers/scsi/ofc/libfc/fc_frame.c| 105 - drivers/scsi/ofc/libfc/fc_ils.h | 524 --- drivers/scsi/ofc/libfc/fc_local_port.c | 1856 -- drivers/scsi/ofc/libfc/fc_local_port_impl.h | 162 -- drivers/scsi/ofc/libfc/fc_port.c | 246 --- drivers/scsi/ofc/libfc/fc_print.c| 138 -- drivers/scsi/ofc/libfc/fc_remote_port.c | 329 - drivers/scsi/ofc/libfc/fc_scsi.h | 463 -- drivers/scsi/ofc/libfc/fc_sess.c | 1445 drivers/scsi/ofc/libfc/fc_sess_impl.h| 75 - drivers/scsi/ofc/libfc/fc_virt_fab.c | 90 - drivers/scsi/ofc/libfc/fc_virt_fab.h | 43 - drivers/scsi/ofc/libfc/fc_virt_fab_impl.h| 48 - drivers/scsi/ofc/libfc/fcs_attr.c| 297 drivers/scsi/ofc/libfc/fcs_cmd.c | 255 drivers/scsi/ofc/libfc/fcs_event.c | 225 --- drivers/scsi/ofc/libfc/fcs_state.c | 448 -- drivers/scsi/ofc/libfc/fcs_state_impl.h | 41 - drivers/scsi/ofc/libsa/Makefile |9 drivers/scsi/ofc/libsa/sa_event.c| 232 --- drivers/scsi/ofc/libsa/sa_hash_kern.c| 141 -- drivers/scsi/ofc/openfc/Makefile | 22 drivers/scsi/ofc/openfc/crc32_le.c | 149 ++ drivers/scsi/ofc/openfc/crc32_le_tab.c | 594 drivers/scsi/ofc/openfc/fc_disc_targ.c | 655 + drivers/scsi/ofc/openfc/fc_disc_targ.h | 48 + drivers/scsi/ofc/openfc/fc_exch.c| 1560 ++ drivers/scsi/ofc/openfc/fc_exch_impl.h | 141 ++ drivers/scsi/ofc/openfc/fc_fcip.h| 131 ++ drivers/scsi/ofc/openfc/fc_frame.c | 105 + drivers/scsi/ofc/openfc/fc_ils.h | 524 +++ drivers/scsi/ofc/openfc/fc_local_port.c | 1856 ++ drivers/scsi/ofc/openfc/fc_local_port_impl.h | 162 ++ drivers/scsi/ofc/openfc/fc_port.c| 246 +++ drivers/scsi/ofc/openfc/fc_print.c | 138 ++ drivers/scsi/ofc/openfc/fc_remote_port.c | 329 + drivers/scsi/ofc/openfc/fc_scsi.h| 463 ++ drivers/scsi/ofc/openfc/fc_sess.c| 1445 drivers/scsi/ofc/openfc/fc_sess_impl.h | 75 + drivers/scsi/ofc/openfc/fc_virt_fab.c| 90 + drivers/scsi/ofc/openfc/fc_virt_fab.h| 43 + drivers/scsi/ofc/openfc/fc_virt_fab_impl.h | 48 + drivers/scsi/ofc/openfc/fcs_attr.c | 297 drivers/scsi/ofc/openfc/fcs_cmd.c| 255 drivers/scsi/ofc/openfc/fcs_event.c | 225 +++ drivers/scsi/ofc/openfc/fcs_state.c | 448 ++ drivers/scsi/ofc/openfc/fcs_state_impl.h | 41 + drivers/scsi/ofc/openfc/sa_event.c | 232 +++ drivers/scsi/ofc/openfc/sa_hash_kern.c | 141 ++ 60 files changed, 10460 insertions(+), 10487 deletions(-) diff --git a/drivers/scsi/ofc/Makefile b/drivers/scsi/ofc/Makefile index 912494b..233f487 100644 --- a/drivers/scsi/ofc/Makefile +++ b/drivers/scsi/ofc/Makefile @@ -4,8 +4,5 @@ OFC_DIR ?= drivers/scsi/ofc export OFC_DIR -obj-$(CONFIG_OFC) += libcrc/ -obj-$(CONFIG_OFC) += libsa/ -obj-$(CONFIG_OFC) += libfc/ obj-$(CONFIG_OFC) += openfc/ obj-$(CONFIG_FCOE) += fcoe/ diff --git a/drivers/scsi/ofc/fcoe/Makefile b/drivers/scsi/ofc/fcoe/Makefile index b46aeae..9ce08da 100644 --- a/drivers/scsi/ofc/fcoe/Makefile +++ b/drivers/scsi/ofc/fcoe/Makefile @@ -4,7 +4,7 @@ EXTRA_CFLAGS += -I$(OFC_DIR)/include obj-$(CONFIG_FCOE) += fcoe.o -fcoe-y := \ +fcoe-objs := \ fcoe_dev.o \ fcoe_if.o \ fcoeinit.o \ diff --git a/drivers/scsi/ofc/libcrc/Makefile b/drivers/scsi/ofc/libcrc/Makefile deleted file mode 100644 index b556b9d..000 --- a/drivers/scsi/ofc/libcrc/Makefile +++ /dev/null @@ -1,9 +0,0 @@ -# $Id: Makefile - -EXTRA_CFLAGS += -I$(OFC_DIR)/include - -obj-y += libcrc.o - -libcrc-y := \ - crc32_le.o \ - crc32_le_tab.o diff --git a/drivers/scsi/ofc/libfc/Makefile b/drivers/scsi/ofc/libfc/Makefile
[PATCH 2/5] Removed outer port use from fcs
Removed outer port use from fcs create and fcs destroy.
Made fcdev available via fcs_state for previously used some of the fields
from outer port, e.g. egress handler and framesize from outer port.
Also added fcdev to fc_local_port and initialized this fcdev during
local port create, this fcdev will be used for frame alloc
and removing inner port later.
Also moved outer port event list to fcs_state.
Signed-off-by: Vasu Dev <[EMAIL PROTECTED]>
---
drivers/scsi/ofc/include/fc_local_port.h|2 +
drivers/scsi/ofc/include/fcs_state.h|4 ++
drivers/scsi/ofc/libfc/fc_local_port.c |2 +
drivers/scsi/ofc/libfc/fc_local_port_impl.h |1 +
drivers/scsi/ofc/libfc/fcs_state.c | 47 +++
drivers/scsi/ofc/libfc/fcs_state_impl.h |4 ++
drivers/scsi/ofc/openfc/openfc_if.c |7 ++--
drivers/scsi/ofc/openfc/openfc_ioctl.c | 10 +++---
8 files changed, 48 insertions(+), 29 deletions(-)
diff --git a/drivers/scsi/ofc/include/fc_local_port.h
b/drivers/scsi/ofc/include/fc_local_port.h
index 0781a8d..c7bb5ac 100644
--- a/drivers/scsi/ofc/include/fc_local_port.h
+++ b/drivers/scsi/ofc/include/fc_local_port.h
@@ -27,6 +27,7 @@
*/
#include "sa_event.h"
#include "fc_fs.h"
+#include "fcdev.h"
struct fc_local_port; /* semi-opaque. See fc_local_port_impl.h */
struct fc_remote_port;
@@ -38,6 +39,7 @@ struct fc_frame;
struct fc_ns_fts;
struct fc_local_port *fc_local_port_create(struct fc_virt_fab *,
+ struct fcdev *,
struct fc_port *,
fc_wwn_t wwpn, fc_wwn_t wwnn,
u_int timeout_msec,
diff --git a/drivers/scsi/ofc/include/fcs_state.h
b/drivers/scsi/ofc/include/fcs_state.h
index 67b9890..720d3da 100644
--- a/drivers/scsi/ofc/include/fcs_state.h
+++ b/drivers/scsi/ofc/include/fcs_state.h
@@ -20,6 +20,8 @@
#ifndef _LIBFC_FCS_STATE_H_
#define _LIBFC_FCS_STATE_H_
+#include "fcdev.h"
+
struct fcs_state;
struct fc_remote_port;
struct fc_local_port;
@@ -34,6 +36,7 @@ struct fcs_create_args {
void(*fca_prlo_notify)(void *arg, struct fc_remote_port *);
void*fca_cb_arg;/* arg for callbacks */
struct fc_port *fca_port; /* transport interface to FC fabric */
+ struct fcdev*dev; /* transport driver instance */
u_int fca_service_params; /* service parm flags from fc/fcp.h */
fc_xid_tfca_min_xid;/* starting exchange ID */
fc_xid_tfca_max_xid;/* maximum exchange ID */
@@ -51,6 +54,7 @@ void fcs_recv(struct fcs_state *, struct fc_frame *);
int fcs_local_port_set(struct fcs_state *, fc_wwn_t node, fc_wwn_t port);
int fcs_cmd_send(struct fcs_state *, struct fc_frame *,
struct fc_frame *, u_int, u_int);
+void fcs_send_event(struct fcs_state *sp, enum fc_event event);
struct fc_local_port *fcs_get_local_port(struct fcs_state *);
/*
diff --git a/drivers/scsi/ofc/libfc/fc_local_port.c
b/drivers/scsi/ofc/libfc/fc_local_port.c
index 5f28f4a..be5a559 100644
--- a/drivers/scsi/ofc/libfc/fc_local_port.c
+++ b/drivers/scsi/ofc/libfc/fc_local_port.c
@@ -869,6 +869,7 @@ void fc_local_port_table_destroy(struct fc_virt_fab *vp)
* Create Local Port.
*/
struct fc_local_port *fc_local_port_create(struct fc_virt_fab *vf,
+ struct fcdev *dev,
struct fc_port *port,
fc_wwn_t wwpn, fc_wwn_t wwnn,
u_int timeout_msec,
@@ -882,6 +883,7 @@ struct fc_local_port *fc_local_port_create(struct
fc_virt_fab *vf,
memset(lp, 0, sizeof(*lp));
lp->fl_vf = vf;
atomic_set(&lp->fl_refcnt, 1);
+ lp->dev = dev;
lp->fl_port = port;
lp->fl_port_wwn = wwpn;
lp->fl_node_wwn = wwnn;
diff --git a/drivers/scsi/ofc/libfc/fc_local_port_impl.h
b/drivers/scsi/ofc/libfc/fc_local_port_impl.h
index ce32176..ce06533 100644
--- a/drivers/scsi/ofc/libfc/fc_local_port_impl.h
+++ b/drivers/scsi/ofc/libfc/fc_local_port_impl.h
@@ -55,6 +55,7 @@ struct fc_local_port {
struct fc_virt_fab *fl_vf; /* virtual fabric */
struct list_head fl_list; /* list headed in virt_fab */
struct fc_port *fl_port; /* port to use when sending */
+ struct fcdev*dev; /* fc device instance */
struct fc_sess *fl_dns_sess; /* session for dNS queries */
struct fc_remote_port *fl_ptp_rp; /* point-to-point remote port */
struct list_head fl_sess_list; /* list of sessions */
diff --git a/drivers/scsi/ofc/libfc/fcs_state.c
b/drivers/scsi/ofc/libfc/fcs_state.c
index 78dd61a..e514504 100644
--- a/drivers/scsi/ofc/libfc/fcs_state.c
+++ b/dri
[PATCH 4/5] Fixed style error reported by checkpatch
Fixed checkpatch ERROR here: do not use assignment in if condition (+ }
else if ((fp = fc_frame_alloc(lp->dev, sizeof(*pl))) == NULL)
Signed-off-by: Vasu Dev <[EMAIL PROTECTED]>
---
drivers/scsi/ofc/libfc/fc_sess.c | 57 +-
1 files changed, 31 insertions(+), 26 deletions(-)
diff --git a/drivers/scsi/ofc/libfc/fc_sess.c b/drivers/scsi/ofc/libfc/fc_sess.c
index 0d5f611..ff570e5 100644
--- a/drivers/scsi/ofc/libfc/fc_sess.c
+++ b/drivers/scsi/ofc/libfc/fc_sess.c
@@ -1189,35 +1189,40 @@ static void fc_sess_recv_plogi_req(struct fc_sess *sess,
if (reject) {
fc_seq_ls_rjt(sp, reject, ELS_EXPL_NONE);
fc_frame_free(fp);
- } else if ((fp = fc_frame_alloc(lp->fl_port, sizeof(*pl))) == NULL) {
- fp = rx_fp;
- fc_seq_ls_rjt(sp, ELS_RJT_UNAB, ELS_EXPL_NONE);
- fc_frame_free(fp);
} else {
- sp = fc_seq_start_next(sp);
- WARN_ON(!sp);
- fc_frame_free(rx_fp);
- fc_remote_port_set_name(rp, wwpn, wwnn);
+ fp = fc_frame_alloc(lp->fl_port, sizeof(*pl));
+ if (fp == NULL) {
+ fp = rx_fp;
+ fc_seq_ls_rjt(sp, ELS_RJT_UNAB, ELS_EXPL_NONE);
+ fc_frame_free(fp);
+ } else {
+ sp = fc_seq_start_next(sp);
+ WARN_ON(!sp);
+ fc_frame_free(rx_fp);
+ fc_remote_port_set_name(rp, wwpn, wwnn);
- /*
-* Get session payload size from incoming PLOGI.
-*/
- sess->fs_max_payload = (uint16_t)
- fc_local_port_get_payload_size(pl, lp->fl_max_payload);
- pl = fc_frame_payload_get(fp, sizeof(*pl));
- WARN_ON(!pl);
- fc_local_port_flogi_fill(lp, pl, ELS_LS_ACC);
+ /*
+* Get session payload size from incoming PLOGI.
+*/
+ sess->fs_max_payload = (uint16_t)
+ fc_local_port_get_payload_size(pl,
+ lp->fl_max_payload);
+ pl = fc_frame_payload_get(fp, sizeof(*pl));
+ WARN_ON(!pl);
+ fc_local_port_flogi_fill(lp, pl, ELS_LS_ACC);
- /*
-* Send LS_ACC. If this fails, the originator should retry.
-*/
- fc_seq_send_last(sp, fp, FC_RCTL_ELS_REP, FC_TYPE_ELS);
- if (sess->fs_state == SESS_ST_PLOGI)
- fc_sess_enter_prli(sess);
- else
- fc_sess_state_enter(sess, SESS_ST_PLOGI_RECV);
- fc_sess_hold(sess); /* represents login */
- sess->fs_plogi_held = 1;
+ /*
+* Send LS_ACC. If this fails,
+* the originator should retry.
+*/
+ fc_seq_send_last(sp, fp, FC_RCTL_ELS_REP, FC_TYPE_ELS);
+ if (sess->fs_state == SESS_ST_PLOGI)
+ fc_sess_enter_prli(sess);
+ else
+ fc_sess_state_enter(sess, SESS_ST_PLOGI_RECV);
+ fc_sess_hold(sess); /* represents login */
+ sess->fs_plogi_held = 1;
+ }
}
fc_sess_unlock_send(sess);
}
-
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
[PATCH 0/5] Series short description
The following series implements... Currently OpenFC maintains several control structures for each enabled fcoe interface, such as inner and outer fc_port, fcdev, openfc_softc, fcs_state (for fc_local_port and fc_virt_fab) etc. Here fc_port should not be confused with FC protocol's ports related structs, instead fc_port provides generic/portable interface to FCS sub module embedded inside openFC for egress, ingress handlers and sa events lists to OpenFC per FCoE interface instance. I removed outer port instance in these patches, instead used existing fcdev structure by direct functions calls between FCS & OpenFC for outer port egress and inegress functions. The fcdev is key shared structure per FCoE interface between OpenFC and FCoE modules, fcdev can be extended for all libfc(TBD) users to access per libfc user instance. Consolidation of more control structures in fcdev will simplify openFC implementation which will help in converting OpenFC into generic libfc library as suggested by linux-scsi reviewers. -- Signature : Vasu Dev <[EMAIL PROTECTED]> - 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
[PATCH 3/5] Removed outer port use in fc_exch
Used fcdev for frame_alloc instead outer port. The frame_alloc() is modified
in next patch with its all other references.
Signed-off-by: Vasu Dev <[EMAIL PROTECTED]>
---
drivers/scsi/ofc/include/fc_exch.h |5 +++--
drivers/scsi/ofc/include/fc_frame.h|2 ++
drivers/scsi/ofc/libfc/fc_exch.c | 32
drivers/scsi/ofc/libfc/fc_exch_impl.h |4 ++--
drivers/scsi/ofc/libfc/fc_local_port.c |2 +-
drivers/scsi/ofc/libfc/fc_sess.c |2 +-
drivers/scsi/ofc/libfc/fcs_cmd.c |2 +-
drivers/scsi/ofc/libfc/fcs_state.c |1 +
8 files changed, 27 insertions(+), 23 deletions(-)
diff --git a/drivers/scsi/ofc/include/fc_exch.h
b/drivers/scsi/ofc/include/fc_exch.h
index 91339ca..b97263f 100644
--- a/drivers/scsi/ofc/include/fc_exch.h
+++ b/drivers/scsi/ofc/include/fc_exch.h
@@ -27,6 +27,7 @@
#include "fc_event.h"
#include "fc_fs.h"
#include "fc_els.h"
+#include "fcdev.h"
/*
* Principles of Operation.
@@ -92,9 +93,9 @@ void fc_exch_mgr_reset(struct fc_exch_mgr *, fc_fid_t s_id,
fc_fid_t d_id);
void fc_exch_set_addr(struct fc_exch *, fc_fid_t orig, fc_fid_t resp);
/*
- * Set the output port to be used for an exchange.
+ * Set the device to be used for an exchange.
*/
-void fc_exch_set_port(struct fc_exch *, struct fc_port *);
+void fc_exch_set_dev(struct fc_exch *, struct fcdev *);
/*
* Start a new sequence as originator on a new exchange.
diff --git a/drivers/scsi/ofc/include/fc_frame.h
b/drivers/scsi/ofc/include/fc_frame.h
index 3fce700..7c7e0c5 100644
--- a/drivers/scsi/ofc/include/fc_frame.h
+++ b/drivers/scsi/ofc/include/fc_frame.h
@@ -44,6 +44,7 @@
struct fc_frame {
struct fc_port *fr_in_port;/* port where frame was received */
+ struct fcdev*dev; /* fc device instance */
struct fc_seq *fr_seq;/* for use with exchange manager */
struct fc_frame_header *fr_hdr; /* pointer to frame header in buffer */
const char *fr_stamp; /* debug info on last usage */
@@ -85,6 +86,7 @@ struct fc_frame {
static inline void fc_frame_init(struct fc_frame *fp)
{
fp->fr_in_port = NULL;
+ fp->dev = NULL;
fp->fr_seq = NULL;
fp->fr_flags = 0;
fp->fr_sg_len = 0;
diff --git a/drivers/scsi/ofc/libfc/fc_exch.c b/drivers/scsi/ofc/libfc/fc_exch.c
index db67d98..a6ea157 100644
--- a/drivers/scsi/ofc/libfc/fc_exch.c
+++ b/drivers/scsi/ofc/libfc/fc_exch.c
@@ -481,7 +481,7 @@ int fc_seq_abort_exch(const struct fc_seq *req_sp)
/*
* Send an abort for the sequence that timed out.
*/
- fp = fc_frame_alloc(ep->ex_port, 0);
+ fp = fc_frame_alloc(ep->dev, 0);
if (fp) {
fc_frame_setup(fp, FC_RCTL_BA_ABTS, FC_TYPE_BLS);
sp->seq_f_ctl |= FC_FC_SEQ_INIT;
@@ -609,7 +609,7 @@ static struct fc_exch *fc_exch_resp(struct fc_exch_mgr *mp,
ep = fc_exch_alloc(mp);
if (ep) {
- ep->ex_port = fp->fr_in_port;
+ ep->dev = fp->dev;
ep->ex_class = fc_frame_class(fp);
/*
@@ -788,9 +788,9 @@ static struct fc_seq *fc_seq_lookup_orig(struct fc_exch_mgr
*mp,
/*
* Set the output port to be used for an exchange.
*/
-void fc_exch_set_port(struct fc_exch *ep, struct fc_port *port)
+void fc_exch_set_dev(struct fc_exch *ep, struct fcdev *dev)
{
- ep->ex_port = port;
+ ep->dev = dev;
}
/*
@@ -934,7 +934,7 @@ size_t fc_seq_mfs(struct fc_seq *sp)
int fc_seq_send_frag(struct fc_seq *sp, struct fc_frame *fp)
{
struct fc_exch *ep;
- struct fc_port *port;
+ struct fcdev *dev;
struct fc_frame_header *fh;
enum fc_class class;
u_int32_t f_ctl;
@@ -942,7 +942,7 @@ int fc_seq_send_frag(struct fc_seq *sp, struct fc_frame *fp)
ep = fc_seq_exch(sp);
WARN_ON((ep->ex_e_stat & ESB_ST_SEQ_INIT) != ESB_ST_SEQ_INIT);
- port = ep->ex_port;
+ dev = ep->dev;
WARN_ON((sp->seq_f_ctl & FC_FC_END_SEQ) != 0);
WARN_ON(fp->fr_len % 4 != 0); /* can't pad except on last frame */
@@ -969,7 +969,7 @@ int fc_seq_send_frag(struct fc_seq *sp, struct fc_frame *fp)
/*
* Send the frame.
*/
- error = fc_port_egress(port, fp);
+ error = dev->port_ops.send(dev, fp);
/*
* Update the exchange and sequence flags.
@@ -989,7 +989,7 @@ int fc_seq_send_frag(struct fc_seq *sp, struct fc_frame *fp)
int fc_seq_send(struct fc_seq *sp, struct fc_frame *fp)
{
struct fc_exch *ep;
- struct fc_port *port;
+ struct fcdev *dev;
struct fc_frame_header *fh;
enum fc_class class;
u_int32_t f_ctl;
@@ -998,7 +998,7 @@ int fc_seq_send(struct fc_seq *sp, struct fc_frame *fp)
ep = fc_seq_exch(sp);
WARN_ON((ep->ex_e_stat & ESB_ST_SEQ_INIT) != ESB_ST_SEQ_INIT);
- port = ep->ex
[PATCH 1/5] Removed outer port allocation
Instead used fcs_recv() port ingress function directly.
The port egress and frame-alloc functions will be used from fcdev directly.
Signed-off-by: Vasu Dev <[EMAIL PROTECTED]>
---
drivers/scsi/ofc/openfc/openfc_if.c | 19 +--
1 files changed, 1 insertions(+), 18 deletions(-)
diff --git a/drivers/scsi/ofc/openfc/openfc_if.c
b/drivers/scsi/ofc/openfc/openfc_if.c
index 179f750..53bbfb1 100644
--- a/drivers/scsi/ofc/openfc/openfc_if.c
+++ b/drivers/scsi/ofc/openfc/openfc_if.c
@@ -801,9 +801,8 @@ static void openfc_timeout_handler(ulong vp)
void openfc_rcv(struct fcdev *dev, struct fc_frame *fp)
{
struct openfc_softc *openfcp = openfc_get_softc(dev);
- struct fc_port *portp = openfcp->fcs_port;
- fc_port_ingress(portp, fp);
+ fcs_recv(openfcp->fcs_state, fp);
}
EXPORT_SYMBOL(openfc_rcv);
@@ -923,7 +922,6 @@ int openfc_register(struct fcdev *dev)
{
struct openfc_softc *openfcp;
struct Scsi_Host *host;
- struct fc_port *port;
struct class_device *cp;
int i;
int rc;
@@ -972,19 +970,6 @@ int openfc_register(struct fcdev *dev)
*/
openfcp->state = OPENFC_INITIALIZATION;
openfcp->status = OPENFC_LINK_UP;
- port = fc_port_alloc();
- if (!port) {
- OFC_DBG("Could not create fc_port structure");
- goto out_host_rem;
- }
- openfcp->fcs_port = port;
- if (dev->port_ops.frame_alloc)
- fc_port_set_frame_alloc(port, dev->port_ops.frame_alloc);
-
- fc_port_set_egress(port,
- (int (*)(void *, struct fc_frame *))dev->port_ops.
- send, dev);
- ofc_fcs_args.fca_port = port;
if (dev->min_xid)
ofc_fcs_args.fca_min_xid = dev->min_xid;
@@ -995,12 +980,10 @@ int openfc_register(struct fcdev *dev)
else
ofc_fcs_args.fca_max_xid = OPENFC_MAX_XID;
- fc_port_set_max_frame_size(port, dev->framesize);
ofc_fcs_args.fca_cb_arg = (void *)openfcp;
openfcp->fcs_state = fcs_create(&ofc_fcs_args);
if (openfcp->fcs_state == NULL) {
OFC_DBG("Could not create fcs_state structure");
- fc_port_close_ingress(port);
goto out_host_rem;
}
-
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
[PATCH 5/5] Removed fc_port from fc_frame_alloc()
Instead used fcdev.
Signed-off-by: Vasu Dev <[EMAIL PROTECTED]>
---
drivers/scsi/ofc/include/fc_frame.h| 17 +
drivers/scsi/ofc/libfc/fc_disc_targ.c |4 ++--
drivers/scsi/ofc/libfc/fc_frame.c |4 ++--
drivers/scsi/ofc/libfc/fc_local_port.c | 18 +-
drivers/scsi/ofc/libfc/fc_sess.c | 12 ++--
drivers/scsi/ofc/openfc/openfc_ioctl.c |4 ++--
drivers/scsi/ofc/openfc/openfc_scsi.c |9 -
7 files changed, 34 insertions(+), 34 deletions(-)
diff --git a/drivers/scsi/ofc/include/fc_frame.h
b/drivers/scsi/ofc/include/fc_frame.h
index 7c7e0c5..e30686d 100644
--- a/drivers/scsi/ofc/include/fc_frame.h
+++ b/drivers/scsi/ofc/include/fc_frame.h
@@ -29,6 +29,7 @@
#include "fc_fs.h"
#include "fc_encaps.h"
#include "fc_port.h"
+#include "fcdev.h"
#include
@@ -120,18 +121,18 @@ static inline int fc_frame_freed_static(struct fc_frame
*fp)
*/
struct fc_frame *fc_frame_alloc_int(size_t payload_size);
-struct fc_frame *fc_frame_alloc_fill(struct fc_port *, size_t payload_len);
+struct fc_frame *fc_frame_alloc_fill(struct fcdev *, size_t payload_len);
/*
* Get frame for sending via port.
*/
-static inline struct fc_frame *fc_port_frame_alloc(struct fc_port *port,
+static inline struct fc_frame *_fc_frame_alloc(struct fcdev *dev,
size_t payload_len)
{
- return (*port->np_frame_alloc)(payload_len);
+ return ((dev->port_ops.frame_alloc)(payload_len));
}
-static inline struct fc_frame *fc_frame_alloc_inline(struct fc_port *port,
+static inline struct fc_frame *fc_frame_alloc_inline(struct fcdev *dev,
size_t len, const char *stamp)
{
struct fc_frame *fp;
@@ -141,9 +142,9 @@ static inline struct fc_frame *fc_frame_alloc_inline(struct
fc_port *port,
* this check will usually be evaluated and eliminated at compile time.
*/
if ((len % 4) != 0)
- fp = fc_frame_alloc_fill(port, len);
+ fp = fc_frame_alloc_fill(dev, len);
else
- fp = fc_port_frame_alloc(port, len);
+ fp = _fc_frame_alloc(dev, len);
#ifdef FC_FRAME_DEBUG
if (fp)
fp->fr_stamp = stamp;
@@ -156,8 +157,8 @@ static inline struct fc_frame *fc_frame_alloc_inline(struct
fc_port *port,
* payload_size + sizeof (struct fc_frame_header).
* This version of fc_frame_alloc() stamps the frame to help find leaks.
*/
-#define fc_frame_alloc(port, len) \
- fc_frame_alloc_inline(port, len, __FUNCTION__)
+#define fc_frame_alloc(dev, len) \
+ fc_frame_alloc_inline(dev, len, __FUNCTION__)
/*
* Free the fc_frame structure and buffer.
*/
diff --git a/drivers/scsi/ofc/libfc/fc_disc_targ.c
b/drivers/scsi/ofc/libfc/fc_disc_targ.c
index b6a5d64..c200b04 100644
--- a/drivers/scsi/ofc/libfc/fc_disc_targ.c
+++ b/drivers/scsi/ofc/libfc/fc_disc_targ.c
@@ -340,7 +340,7 @@ static void fcdt_gpn_ft_req(struct fc_local_port *lp)
lp->fl_disc_buf_len = 0;
lp->fl_disc_seq_cnt = 0;
- fp = fc_frame_alloc(lp->fl_port, sizeof(*rp));
+ fp = fc_frame_alloc(lp->dev, sizeof(*rp));
if (fp == NULL) {
error = ENOMEM;
} else {
@@ -562,7 +562,7 @@ static int fcdt_gpn_id_req(struct fc_local_port *lp, struct
fc_remote_port *rp)
} *cp;
int error;
- fp = fc_frame_alloc(lp->fl_port, sizeof(*cp));
+ fp = fc_frame_alloc(lp->dev, sizeof(*cp));
if (fp == NULL) {
error = ENOMEM;
} else {
diff --git a/drivers/scsi/ofc/libfc/fc_frame.c
b/drivers/scsi/ofc/libfc/fc_frame.c
index 34d7a8a..359018d 100644
--- a/drivers/scsi/ofc/libfc/fc_frame.c
+++ b/drivers/scsi/ofc/libfc/fc_frame.c
@@ -52,7 +52,7 @@ u_int32_t fc_frame_crc_check(struct fc_frame *fp)
return (error);
}
-struct fc_frame *fc_frame_alloc_fill(struct fc_port *port, size_t payload_len)
+struct fc_frame *fc_frame_alloc_fill(struct fcdev *dev, size_t payload_len)
{
struct fc_frame *fp;
size_t fill;
@@ -60,7 +60,7 @@ struct fc_frame *fc_frame_alloc_fill(struct fc_port *port,
size_t payload_len)
fill = payload_len % 4;
if (fill != 0)
fill = 4 - fill;
- fp = fc_port_frame_alloc(port, payload_len + fill);
+ fp = _fc_frame_alloc(dev, payload_len + fill);
if (fp) {
fp->fr_len -= fill;
memset((char *)fp->fr_hdr + fp->fr_len, 0, fill);
diff --git a/drivers/scsi/ofc/libfc/fc_local_port.c
b/drivers/scsi/ofc/libfc/fc_local_port.c
index 6160bdf..f0c5625 100644
--- a/drivers/scsi/ofc/libfc/fc_local_port.c
+++ b/drivers/scsi/ofc/libfc/fc_local_port.c
@@ -421,7 +421,7 @@ static void fc_local_port_flogi_send(struct fc_local_port
*lp)
sp = fc_seq_start_exch(lp->fl_vf->vf_exch_mgr,
fc_local_port_flogi_resp,
fc_local_port_error, lp, 0, FC_FID_FLOGI);
- fp = fc_frame_alloc(lp->fl_port, sizeof(*flp))
Re: Need help with libata error handling in libsas
James Bottomley wrote: On Mon, 2008-02-25 at 10:34 -0600, Brian King wrote: The new libata-eh is used for more than just EH. It is used for device probing, device revalidation, and power management. It is also woken for all command failures and is where the request sense for ATAPI devices is issued. Device revalidation following reset is also critical for ATA and ATAPI devices. One example of this is some SATA/PATA converter chips lose their DMA xfer settings following a reset and default to PIO mode only. Any DMA transfer that is attempted simply hangs. Strongly seconded. Doing your own ATA EH would be foolish, as that would imply duplicating all that carefully-time-tested logic handling devices which follow the ATA specs... about 98% of the time :) Just the set-transfer-mode logic took years to get right for the majority of ATA devices. OK ... I'm grepping around in the source trying to figure out all of this. Is it documented anywhere? That would really help me out at the moment. Unfortunately, not really. The simplistic version is... freeze, set some flags, call a function to schedule EH as needed -- most notably when your HBA signals an ATA device error or some other error in the ATA domain. Regardless of all this... libsas IMO will cause some libata-EH growing pains. libsas needs libata-EH for probing, revalidation, initialization, etc. But libsas probably does NOT need libata-EH for certain duties like SATA PHY diagnosis and link handling. libsas needs libata-EH. Unfortunately for libsas, libata-EH was written from the "libata controls the world" point of view, and probably needs some modifications to play well in the new SATA/SAS shared worldview. Brian's recommendation is quite sane... your ->error_handler() probably just needs hard reset (aka COMRESET) capability. Jeff - 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: Need help with libata error handling in libsas
On Mon, 2008-02-25 at 10:34 -0600, Brian King wrote: > The new libata-eh is used for more than just EH. It is used for device > probing, device revalidation, and power management. It is also woken for > all command failures and is where the request sense for ATAPI devices is > issued. Device revalidation following reset is also critical for ATA and > ATAPI devices. One example of this is some SATA/PATA converter chips > lose their DMA xfer settings following a reset and default to PIO mode > only. Any DMA transfer that is attempted simply hangs. OK ... I'm grepping around in the source trying to figure out all of this. Is it documented anywhere? That would really help me out at the moment. 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: Need help with libata error handling in libsas
James Bottomley wrote: > I keep hearing that we need to convert libsas to use libata's new error > handling. Unfortunately, I have very little conception of what that > means. Right at the moment, libsas doesn't use any error handling > functions of libata at all. > > I've looked through the libata-eh functions, and I find them frankly > incomprehensible. > > Firstly, let me say what SAS error handling actually does: Let me chime in with what ipr error handling does/can do. The ipr firmware provides two basic SATA error handling methods with some modifiers to each. Cancel All - This cancels all outstanding commands to the device. When issued to an ATA device, this gets escalated by the firmware to an SRST. When issued to an ATAPI device, an ATA NOOP is issued. Reset Device - This command has modifiers to indicate either a soft reset or a hard reset. Currently, the only SATA devices that ipr officially attaches are ATAPI DVD devices. In our testing we've come to the conclusion that trying to use anything but a hard reset for ERP is generally more trouble than it is worth. > All of this leads me to conclude, that all libsas needs is to plumb in > the ATA equivalent of abort, junk the task query for libata devices and > simply proceed, as if the task is held at the target, along the > escalating reset path. The new libata-eh is used for more than just EH. It is used for device probing, device revalidation, and power management. It is also woken for all command failures and is where the request sense for ATAPI devices is issued. Device revalidation following reset is also critical for ATA and ATAPI devices. One example of this is some SATA/PATA converter chips lose their DMA xfer settings following a reset and default to PIO mode only. Any DMA transfer that is attempted simply hangs. The other issue is PMP support. The more that gets pushed into libsas, the more libsas needs to know about things such as PMP. -Brian -- Brian King Linux on Power Virtualization IBM Linux Technology Center - 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] [12/22] Remove GFP_DMAs/unchecked_isa_dma checks in scsi_scan.c
> You've already added a new special field with your u64 > sense_buffer_mask; Ok I'll add another one for this case so that you can still have zero copy SCSI scan. Or maybe it can check the block layer bounce information directly here. > It's a special case field for ISA devices because everything else has a > dev->dma_mask you can use. I'm not sure that trying to use ->dma_mask for this is all that useful. The issue is that a lot of drivers do not actually DMA specific areas because either they don't care about the data or the data is passed in some other implicit way. Trying to lump it all together into a single mask would be a mistake I think. > So, in fact, you're removing my single bit isa_unchecked_dma flag and > using 64 bits in its place that preserves essentially the same > information. It just doesn't look like a bargain to me. Particularly > as you're removing a lot of the memory allocation optimisation paths at > the same time. Essentially it removes an ISA specific optimization and makes ISA work like all other IO without IOMMU. > There is a proposal that's been under consideration for a while because > it might solve other issues ATA has with mdma (they need to use ... that'll have to be done by someone else. -Andi - 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 2/2] ps3rom: Simplify fill_from_dev_buffer()
On Tue, 26 Feb 2008, FUJITA Tomonori wrote: > On Mon, 25 Feb 2008 14:24:31 +0100 (CET) > Geert Uytterhoeven <[EMAIL PROTECTED]> wrote: > > > Subject: [PATCH] ps3rom: Simplify fill_from_dev_buffer() > > > > As we no longer need to calculate the data length of the whole scatterlist, > > we can abort the loop earlier and coalesce req_len and act_len into one > > variable, making fill_from_dev_buffer() more similar to > > fetch_to_dev_buffer(). > > I'll add new APIs to copy data between a sg list and a buffer soon > after cleaning up (and fixing) some drivers on this area. I plan to > remove fill_from_dev_buffer and fetch_to_dev_buffer in ps3rom. As you > know, they are same functions that scsi_debug uses. There are other > drivers that need similar functions. I expect my ps3rom patch to be > applied to the scsi-fixes tree so I didn't change much as you did in > this patch. IC, thanks! 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] [12/22] Remove GFP_DMAs/unchecked_isa_dma checks in scsi_scan.c
On Mon, 2008-02-25 at 16:11 +0100, Andi Kleen wrote:
> On Mon, Feb 25, 2008 at 07:04:20AM -0800, James Bottomley wrote:
> > On Mon, 2008-02-25 at 15:58 +0100, Andi Kleen wrote:
> > > On Mon, Feb 25, 2008 at 06:46:58AM -0800, James Bottomley wrote:
> > > >
> > > > On Mon, 2008-02-25 at 00:35 +0100, Andi Kleen wrote:
> > > > > Should not be needed because the block layer bounces that all.
> > > > >
> > > > > Signed-off-by: Andi Kleen <[EMAIL PROTECTED]>
> > > > >
> > > > > ---
> > > > > drivers/scsi/scsi_scan.c |6 ++
> > > > > 1 file changed, 2 insertions(+), 4 deletions(-)
> > > > >
> > > > > Index: linux/drivers/scsi/scsi_scan.c
> > > > > ===
> > > > > --- linux.orig/drivers/scsi/scsi_scan.c
> > > > > +++ linux/drivers/scsi/scsi_scan.c
> > > > > @@ -1010,8 +1010,7 @@ static int scsi_probe_and_add_lun(struct
> > > > > if (!sdev)
> > > > > goto out;
> > > > >
> > > > > - result = kmalloc(result_len, GFP_ATOMIC |
> > > > > - ((shost->unchecked_isa_dma) ? __GFP_DMA : 0));
> > > > > + result = kmalloc(result_len, GFP_ATOMIC);
> > > > > if (!result)
> > > > > goto out_free_sdev;
> > > > >
> > > > > @@ -1328,8 +1327,7 @@ static int scsi_report_lun_scan(struct s
> > > > >* prevent us from finding any LUNs on this target.
> > > > >*/
> > > > > length = (max_scsi_report_luns + 1) * sizeof(struct scsi_lun);
> > > > > - lun_data = kmalloc(length, GFP_ATOMIC |
> > > > > -(sdev->host->unchecked_isa_dma ? __GFP_DMA :
> > > > > 0));
> > > > > + lun_data = kmalloc(length, GFP_ATOMIC);
> > > > > if (!lun_data) {
> > > > > printk(ALLOC_FAILURE_MSG, __FUNCTION__);
> > > > > goto out;
> > > >
> > > > Andi, this can't be right.
> > >
> > > You mean it is incorrect or just not optimal?
> >
> > It's not optimal ... but that's sufficient an objection.
> >
> > > >
> > > > You're removing something that's actually useful. I'm happy to
> > > > substitute this kmalloc for kmalloc_mask on the device dma mask which
> > > > will do the same thing and so junk unchecked_isa_dma() that way (and
> > > > actually fix us up for other weird mask devices), but just using
> > > > ZONE_NORMAL is wrong because we'll then bounce all the time for
> > > > something we knew a priori how to avoid.
> > >
> > > That would require adding a separate mask just for this to the
> > > template. I figured the SCSI scan was not performance critical
> > > so a few more copies just for this case was an ok trade off
> > > for simpler code.
> >
> > Why? We already have the device; can't we just use its mask?
>
> These ISA drivers I worked with definitely don't know
> anything about a device in any form.
>
> Also there is the issue that 50+% of the ISA drivers actually
> don't need it even if they had a ISA device.
>
> >
> > > You think it makes sense to optimize scsi scan?
> >
> > It makes sense to use information we already know to optimise the path,
>
> In this case we would need to add a special new field for this
> to the SCSI template (assume unchecked_isa_dma is already gone).
> You think that is worth it?
You've already added a new special field with your u64
sense_buffer_mask;
It's a special case field for ISA devices because everything else has a
dev->dma_mask you can use.
So, in fact, you're removing my single bit isa_unchecked_dma flag and
using 64 bits in its place that preserves essentially the same
information. It just doesn't look like a bargain to me. Particularly
as you're removing a lot of the memory allocation optimisation paths at
the same time.
Don't get me wrong; I'm all for removing the special casing of isa
devices we have to do, of which isa_unchecked_dma is the switch ... I
just want to do it properly, and that would have to be by bringing isa
devices under the device model, so we don't have to special case them.
There is a proposal that's been under consideration for a while because
it might solve other issues ATA has with mdma (they need to use
different DMA masks for different devices on the same host). The
proposal was to use the struct device in the struct scsi_device as the
device for the dma_ API ... this has all the properties you need ... you
can give it a DMA_24BIT_MASK for isa devices, and use it to key the
allocations off, since it's always present ... then we can junk
isa_unchecked_dma and not add any other strange flags.
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 2/2] ps3rom: Simplify fill_from_dev_buffer()
On Mon, 25 Feb 2008 14:24:31 +0100 (CET) Geert Uytterhoeven <[EMAIL PROTECTED]> wrote: > Subject: [PATCH] ps3rom: Simplify fill_from_dev_buffer() > > As we no longer need to calculate the data length of the whole scatterlist, > we can abort the loop earlier and coalesce req_len and act_len into one > variable, making fill_from_dev_buffer() more similar to fetch_to_dev_buffer(). I'll add new APIs to copy data between a sg list and a buffer soon after cleaning up (and fixing) some drivers on this area. I plan to remove fill_from_dev_buffer and fetch_to_dev_buffer in ps3rom. As you know, they are same functions that scsi_debug uses. There are other drivers that need similar functions. I expect my ps3rom patch to be applied to the scsi-fixes tree so I didn't change much as you did in this 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] [12/22] Remove GFP_DMAs/unchecked_isa_dma checks in scsi_scan.c
On Mon, Feb 25, 2008 at 07:04:20AM -0800, James Bottomley wrote:
> On Mon, 2008-02-25 at 15:58 +0100, Andi Kleen wrote:
> > On Mon, Feb 25, 2008 at 06:46:58AM -0800, James Bottomley wrote:
> > >
> > > On Mon, 2008-02-25 at 00:35 +0100, Andi Kleen wrote:
> > > > Should not be needed because the block layer bounces that all.
> > > >
> > > > Signed-off-by: Andi Kleen <[EMAIL PROTECTED]>
> > > >
> > > > ---
> > > > drivers/scsi/scsi_scan.c |6 ++
> > > > 1 file changed, 2 insertions(+), 4 deletions(-)
> > > >
> > > > Index: linux/drivers/scsi/scsi_scan.c
> > > > ===
> > > > --- linux.orig/drivers/scsi/scsi_scan.c
> > > > +++ linux/drivers/scsi/scsi_scan.c
> > > > @@ -1010,8 +1010,7 @@ static int scsi_probe_and_add_lun(struct
> > > > if (!sdev)
> > > > goto out;
> > > >
> > > > - result = kmalloc(result_len, GFP_ATOMIC |
> > > > - ((shost->unchecked_isa_dma) ? __GFP_DMA : 0));
> > > > + result = kmalloc(result_len, GFP_ATOMIC);
> > > > if (!result)
> > > > goto out_free_sdev;
> > > >
> > > > @@ -1328,8 +1327,7 @@ static int scsi_report_lun_scan(struct s
> > > > * prevent us from finding any LUNs on this target.
> > > > */
> > > > length = (max_scsi_report_luns + 1) * sizeof(struct scsi_lun);
> > > > - lun_data = kmalloc(length, GFP_ATOMIC |
> > > > - (sdev->host->unchecked_isa_dma ? __GFP_DMA :
> > > > 0));
> > > > + lun_data = kmalloc(length, GFP_ATOMIC);
> > > > if (!lun_data) {
> > > > printk(ALLOC_FAILURE_MSG, __FUNCTION__);
> > > > goto out;
> > >
> > > Andi, this can't be right.
> >
> > You mean it is incorrect or just not optimal?
>
> It's not optimal ... but that's sufficient an objection.
>
> > >
> > > You're removing something that's actually useful. I'm happy to
> > > substitute this kmalloc for kmalloc_mask on the device dma mask which
> > > will do the same thing and so junk unchecked_isa_dma() that way (and
> > > actually fix us up for other weird mask devices), but just using
> > > ZONE_NORMAL is wrong because we'll then bounce all the time for
> > > something we knew a priori how to avoid.
> >
> > That would require adding a separate mask just for this to the
> > template. I figured the SCSI scan was not performance critical
> > so a few more copies just for this case was an ok trade off
> > for simpler code.
>
> Why? We already have the device; can't we just use its mask?
These ISA drivers I worked with definitely don't know
anything about a device in any form.
Also there is the issue that 50+% of the ISA drivers actually
don't need it even if they had a ISA device.
>
> > You think it makes sense to optimize scsi scan?
>
> It makes sense to use information we already know to optimise the path,
In this case we would need to add a special new field for this
to the SCSI template (assume unchecked_isa_dma is already gone).
You think that is worth it?
-Andi
-
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] Don't use unnecessary GFP_ATOMIC in sg.c
On Mon, Feb 25, 2008 at 07:00:34AM -0800, James Bottomley wrote: > > On Mon, 2008-02-25 at 15:56 +0100, Andi Kleen wrote: > > On Mon, Feb 25, 2008 at 06:43:08AM -0800, James Bottomley wrote: > > > On Mon, 2008-02-25 at 00:36 +0100, Andi Kleen wrote: > > > > Don't use unnecessary GFP_ATOMIC in sg.c > > > > > > > > sg.c uses GFP_ATOMIC for a lot of allocations where it isn't necessary. > > > > There are no locks hold and I don't see any other reasons why GFP_ATOMIC > > > > should be needed here. So remove them for more reliable allocations. > > > > > > > > Depends on the earlier GFP_DMA patchkit, but only because it happens > > > > to patch the same places (no real functional dependency) > > > > > > > > Tested by burning a CD on a kernel with all lock/sleep/etc. debugging > > > > enabled. > > > > > > I'm afraid this is wrong. Those paths are used in the block layer issue > > > path. Most of the time this path does have user context. However, it's > > > legal to call it from tasklet context, and most error retries do this. > > > In that case, your GFP_KERNEL will have problems, so these have to be > > > GFP_ATOMIC, I'm afraid. > > > > In what path? I couldn't find one while reading the code. But I don't > > claim to understand it completely so I might have missed something. Hmm, but as far as I can figure out these allocations are only used in code that does copy_from_user or get_user_pages or wait_event_* or some other API that sleeps. That task let path you're talking about must be well hidden. I would appreciate if someone in the know could point me to the path I'm missing. Anyways I hadn't expected it to be that controversal and since that patch is only a side show I retract it for now. -Andi - 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] [12/22] Remove GFP_DMAs/unchecked_isa_dma checks in scsi_scan.c
On Mon, 2008-02-25 at 15:58 +0100, Andi Kleen wrote:
> On Mon, Feb 25, 2008 at 06:46:58AM -0800, James Bottomley wrote:
> >
> > On Mon, 2008-02-25 at 00:35 +0100, Andi Kleen wrote:
> > > Should not be needed because the block layer bounces that all.
> > >
> > > Signed-off-by: Andi Kleen <[EMAIL PROTECTED]>
> > >
> > > ---
> > > drivers/scsi/scsi_scan.c |6 ++
> > > 1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > Index: linux/drivers/scsi/scsi_scan.c
> > > ===
> > > --- linux.orig/drivers/scsi/scsi_scan.c
> > > +++ linux/drivers/scsi/scsi_scan.c
> > > @@ -1010,8 +1010,7 @@ static int scsi_probe_and_add_lun(struct
> > > if (!sdev)
> > > goto out;
> > >
> > > - result = kmalloc(result_len, GFP_ATOMIC |
> > > - ((shost->unchecked_isa_dma) ? __GFP_DMA : 0));
> > > + result = kmalloc(result_len, GFP_ATOMIC);
> > > if (!result)
> > > goto out_free_sdev;
> > >
> > > @@ -1328,8 +1327,7 @@ static int scsi_report_lun_scan(struct s
> > >* prevent us from finding any LUNs on this target.
> > >*/
> > > length = (max_scsi_report_luns + 1) * sizeof(struct scsi_lun);
> > > - lun_data = kmalloc(length, GFP_ATOMIC |
> > > -(sdev->host->unchecked_isa_dma ? __GFP_DMA : 0));
> > > + lun_data = kmalloc(length, GFP_ATOMIC);
> > > if (!lun_data) {
> > > printk(ALLOC_FAILURE_MSG, __FUNCTION__);
> > > goto out;
> >
> > Andi, this can't be right.
>
> You mean it is incorrect or just not optimal?
It's not optimal ... but that's sufficient an objection.
> >
> > You're removing something that's actually useful. I'm happy to
> > substitute this kmalloc for kmalloc_mask on the device dma mask which
> > will do the same thing and so junk unchecked_isa_dma() that way (and
> > actually fix us up for other weird mask devices), but just using
> > ZONE_NORMAL is wrong because we'll then bounce all the time for
> > something we knew a priori how to avoid.
>
> That would require adding a separate mask just for this to the
> template. I figured the SCSI scan was not performance critical
> so a few more copies just for this case was an ok trade off
> for simpler code.
Why? We already have the device; can't we just use its mask?
> You think it makes sense to optimize scsi scan?
It makes sense to use information we already know to optimise the path,
yes, when it's something as simple as keeping the dma buffer within the
mask bounds of the device.
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] [1/22] Add new sense_buffer_mask host template field
On Mon, Feb 25, 2008 at 06:48:28AM -0800, James Bottomley wrote: > On Mon, 2008-02-25 at 00:35 +0100, Andi Kleen wrote: > > sense buffers are something that still needs to be explicitely > > bounced in the scsi layer. Instead of using the global unchecked_isa_dma > > flag define a special fine grained mask for this. > > > > I decided to use a full dma mask because that is most useful for some future > > infrastructure work I'm doing. > > Why do we need a separate mask? Why can't we just use the device > dma_mask? First a lot of drivers don't need it because they handle the sense buffers without DMA. So if we just used a device mask we would either do unnecessary work for those or require a special sense buffer mask in the device (which wouldn't belong into the device layer) Then there isn't really a device anywhere in these drivers and I didn't plan to rewrite them to add one. So just having a SCSI specific sense buffer mask seemed best. It is only set when a ISA driver actually does sense buffer DMA, which is only true in a handful of drivers. -Andi - 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] Don't use unnecessary GFP_ATOMIC in sg.c
On Mon, 2008-02-25 at 15:56 +0100, Andi Kleen wrote: > On Mon, Feb 25, 2008 at 06:43:08AM -0800, James Bottomley wrote: > > On Mon, 2008-02-25 at 00:36 +0100, Andi Kleen wrote: > > > Don't use unnecessary GFP_ATOMIC in sg.c > > > > > > sg.c uses GFP_ATOMIC for a lot of allocations where it isn't necessary. > > > There are no locks hold and I don't see any other reasons why GFP_ATOMIC > > > should be needed here. So remove them for more reliable allocations. > > > > > > Depends on the earlier GFP_DMA patchkit, but only because it happens > > > to patch the same places (no real functional dependency) > > > > > > Tested by burning a CD on a kernel with all lock/sleep/etc. debugging > > > enabled. > > > > I'm afraid this is wrong. Those paths are used in the block layer issue > > path. Most of the time this path does have user context. However, it's > > legal to call it from tasklet context, and most error retries do this. > > In that case, your GFP_KERNEL will have problems, so these have to be > > GFP_ATOMIC, I'm afraid. > > In what path? I couldn't find one while reading the code. But I don't > claim to understand it completely so I might have missed something. It's a fundamental lack of guarantee of the API. Block *may* give you user context but doesn't guarantee it. It's so we can call blk_run_queue from the block tasklet. This is usually done by queue restarts. SCSI does it via scsi_next_command, which is usually in tasklet context. You'll really only see this for a TCQ device (which no CDs are). 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] [12/22] Remove GFP_DMAs/unchecked_isa_dma checks in scsi_scan.c
On Mon, Feb 25, 2008 at 06:46:58AM -0800, James Bottomley wrote:
>
> On Mon, 2008-02-25 at 00:35 +0100, Andi Kleen wrote:
> > Should not be needed because the block layer bounces that all.
> >
> > Signed-off-by: Andi Kleen <[EMAIL PROTECTED]>
> >
> > ---
> > drivers/scsi/scsi_scan.c |6 ++
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > Index: linux/drivers/scsi/scsi_scan.c
> > ===
> > --- linux.orig/drivers/scsi/scsi_scan.c
> > +++ linux/drivers/scsi/scsi_scan.c
> > @@ -1010,8 +1010,7 @@ static int scsi_probe_and_add_lun(struct
> > if (!sdev)
> > goto out;
> >
> > - result = kmalloc(result_len, GFP_ATOMIC |
> > - ((shost->unchecked_isa_dma) ? __GFP_DMA : 0));
> > + result = kmalloc(result_len, GFP_ATOMIC);
> > if (!result)
> > goto out_free_sdev;
> >
> > @@ -1328,8 +1327,7 @@ static int scsi_report_lun_scan(struct s
> > * prevent us from finding any LUNs on this target.
> > */
> > length = (max_scsi_report_luns + 1) * sizeof(struct scsi_lun);
> > - lun_data = kmalloc(length, GFP_ATOMIC |
> > - (sdev->host->unchecked_isa_dma ? __GFP_DMA : 0));
> > + lun_data = kmalloc(length, GFP_ATOMIC);
> > if (!lun_data) {
> > printk(ALLOC_FAILURE_MSG, __FUNCTION__);
> > goto out;
>
> Andi, this can't be right.
You mean it is incorrect or just not optimal?
>
> You're removing something that's actually useful. I'm happy to
> substitute this kmalloc for kmalloc_mask on the device dma mask which
> will do the same thing and so junk unchecked_isa_dma() that way (and
> actually fix us up for other weird mask devices), but just using
> ZONE_NORMAL is wrong because we'll then bounce all the time for
> something we knew a priori how to avoid.
That would require adding a separate mask just for this to the
template. I figured the SCSI scan was not performance critical
so a few more copies just for this case was an ok trade off
for simpler code.
You think it makes sense to optimize scsi scan?
-Andi
-
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] Don't use unnecessary GFP_ATOMIC in sg.c
On Mon, Feb 25, 2008 at 06:43:08AM -0800, James Bottomley wrote: > On Mon, 2008-02-25 at 00:36 +0100, Andi Kleen wrote: > > Don't use unnecessary GFP_ATOMIC in sg.c > > > > sg.c uses GFP_ATOMIC for a lot of allocations where it isn't necessary. > > There are no locks hold and I don't see any other reasons why GFP_ATOMIC > > should be needed here. So remove them for more reliable allocations. > > > > Depends on the earlier GFP_DMA patchkit, but only because it happens > > to patch the same places (no real functional dependency) > > > > Tested by burning a CD on a kernel with all lock/sleep/etc. debugging > > enabled. > > I'm afraid this is wrong. Those paths are used in the block layer issue > path. Most of the time this path does have user context. However, it's > legal to call it from tasklet context, and most error retries do this. > In that case, your GFP_KERNEL will have problems, so these have to be > GFP_ATOMIC, I'm afraid. In what path? I couldn't find one while reading the code. But I don't claim to understand it completely so I might have missed something. -Andi - 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] [1/22] Add new sense_buffer_mask host template field
On Mon, 2008-02-25 at 00:35 +0100, Andi Kleen wrote: > sense buffers are something that still needs to be explicitely > bounced in the scsi layer. Instead of using the global unchecked_isa_dma > flag define a special fine grained mask for this. > > I decided to use a full dma mask because that is most useful for some future > infrastructure work I'm doing. Why do we need a separate mask? Why can't we just use the device dma_mask? 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] [12/22] Remove GFP_DMAs/unchecked_isa_dma checks in scsi_scan.c
On Mon, 2008-02-25 at 00:35 +0100, Andi Kleen wrote:
> Should not be needed because the block layer bounces that all.
>
> Signed-off-by: Andi Kleen <[EMAIL PROTECTED]>
>
> ---
> drivers/scsi/scsi_scan.c |6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> Index: linux/drivers/scsi/scsi_scan.c
> ===
> --- linux.orig/drivers/scsi/scsi_scan.c
> +++ linux/drivers/scsi/scsi_scan.c
> @@ -1010,8 +1010,7 @@ static int scsi_probe_and_add_lun(struct
> if (!sdev)
> goto out;
>
> - result = kmalloc(result_len, GFP_ATOMIC |
> - ((shost->unchecked_isa_dma) ? __GFP_DMA : 0));
> + result = kmalloc(result_len, GFP_ATOMIC);
> if (!result)
> goto out_free_sdev;
>
> @@ -1328,8 +1327,7 @@ static int scsi_report_lun_scan(struct s
>* prevent us from finding any LUNs on this target.
>*/
> length = (max_scsi_report_luns + 1) * sizeof(struct scsi_lun);
> - lun_data = kmalloc(length, GFP_ATOMIC |
> -(sdev->host->unchecked_isa_dma ? __GFP_DMA : 0));
> + lun_data = kmalloc(length, GFP_ATOMIC);
> if (!lun_data) {
> printk(ALLOC_FAILURE_MSG, __FUNCTION__);
> goto out;
Andi, this can't be right.
You're removing something that's actually useful. I'm happy to
substitute this kmalloc for kmalloc_mask on the device dma mask which
will do the same thing and so junk unchecked_isa_dma() that way (and
actually fix us up for other weird mask devices), but just using
ZONE_NORMAL is wrong because we'll then bounce all the time for
something we knew a priori how to avoid.
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] Don't use unnecessary GFP_ATOMIC in sg.c
On Mon, 2008-02-25 at 00:36 +0100, Andi Kleen wrote: > Don't use unnecessary GFP_ATOMIC in sg.c > > sg.c uses GFP_ATOMIC for a lot of allocations where it isn't necessary. > There are no locks hold and I don't see any other reasons why GFP_ATOMIC > should be needed here. So remove them for more reliable allocations. > > Depends on the earlier GFP_DMA patchkit, but only because it happens > to patch the same places (no real functional dependency) > > Tested by burning a CD on a kernel with all lock/sleep/etc. debugging enabled. I'm afraid this is wrong. Those paths are used in the block layer issue path. Most of the time this path does have user context. However, it's legal to call it from tasklet context, and most error retries do this. In that case, your GFP_KERNEL will have problems, so these have to be GFP_ATOMIC, I'm afraid. 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 added to scsi-rc-fixes-2.6: [SCSI] arcmsr: fix message allocation
On Mon, 2008-02-25 at 18:47 +0800, nickcheng wrote: > Sorry, maybe I did not ask distinctly enough. > I mean if I would like to allocate a memory space from ZONE_DMA for atomic > context, why can I not use kmalloc(1032, GFP_ATOMIC|GFP_DMA)? > In case of lack of GFP_DMA, kmalloc would grab the memory from ZONE_HIGH or > ZONE_HIGHMEM, isn't it?(I read it from the textbook of Linux Kernel > Development by Robert Love) Um, no that's not true at all. GFP_DMA only allocates memory from ZONE_DMA and fails otherwise. You only need memory from ZONE_DMA if you cannot address physical memory above 24 bits (the old ISA restriction). This only applies if you're a standard ISA device and you're going actually to DMA to the memory in question. Neither of which applies in the arcmsr case, since you're only using the memory as a copy buffer within the kernel (it never sees an actual DMA transfer), and arcmsr doesn't have the ISA restrictions. > Or the basic is that you don't think it is necessary to allocate a memory > space from DMA area? > Please give me some comments. It's unnecessary because you never DMA to it, but even if you did, since arcmsr is a non-ISA device (with 64 bit DMA mask falling back to 32) you can just use ordinary kmalloc'd memory for that (provided you obey the coherence requirements). 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] Don't use unnecessary GFP_ATOMIC in sg.c v2
> Oh no, not again. This isn't the first time kernel folks > have tried to demote the sg driver's GFP_ATOMIC to GFP_KERNEL. That is because it is abusing GFP_ATOMIC. > In the past it has ended in grief. The driver was written > to attempt _fast_ allocation and if that failed then: You want it to not swap? Then __GFP_NOIO would be correct. > - lessen the requested allocation (e.g. smaller elements > but more of them in a scatter gather list), or What is when the allocation is already 1 page? > - report the error back to the user (i.e. ENOMEM) assuming > that they are knowledgeable enough to do something about it > (e.g. do two smaller READs rather than one larger one). But the kernel can actually do something about it, just not when you pass it __GFP_ATOMIC. -Andi - 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] [8/22] Remove random noop unchecked_isa_dma users
ACK (for ips portion, see no harm other chunks). Sincerely -- Mark Salyzyn > -Original Message- > From: [EMAIL PROTECTED] > [mailto:[EMAIL PROTECTED] On Behalf Of Andi Kleen > Sent: Sunday, February 24, 2008 6:35 PM > To: linux-scsi@vger.kernel.org; [EMAIL PROTECTED] > Subject: [PATCH] [8/22] Remove random noop unchecked_isa_dma users > > > Lots of drivers set it to 0. Remove that. Patch should be a nop. > > Signed-off-by: Andi Kleen <[EMAIL PROTECTED]> > > --- > drivers/scsi/arm/acornscsi.c |1 - > drivers/scsi/arm/cumana_1.c |1 - > drivers/scsi/dc395x.c|1 - > drivers/scsi/eata_pio.c |2 -- > drivers/scsi/hptiop.c|1 - > drivers/scsi/ips.c |1 - > drivers/scsi/mac_scsi.c |1 - > drivers/scsi/scsi_debug.c|1 - > 8 files changed, 9 deletions(-) > > Index: linux/drivers/scsi/arm/acornscsi.c > === > --- linux.orig/drivers/scsi/arm/acornscsi.c > +++ linux/drivers/scsi/arm/acornscsi.c > @@ -2983,7 +2983,6 @@ static struct scsi_host_template acornsc > .this_id= 7, > .sg_tablesize = SG_ALL, > .cmd_per_lun= 2, > - .unchecked_isa_dma = 0, > .use_clustering = DISABLE_CLUSTERING, > .proc_name = "acornscsi", > }; > Index: linux/drivers/scsi/arm/cumana_1.c > === > --- linux.orig/drivers/scsi/arm/cumana_1.c > +++ linux/drivers/scsi/arm/cumana_1.c > @@ -222,7 +222,6 @@ static struct scsi_host_template cumanas > .this_id= 7, > .sg_tablesize = SG_ALL, > .cmd_per_lun= 2, > - .unchecked_isa_dma = 0, > .use_clustering = DISABLE_CLUSTERING, > .proc_name = "CumanaSCSI-1", > }; > Index: linux/drivers/scsi/dc395x.c > === > --- linux.orig/drivers/scsi/dc395x.c > +++ linux/drivers/scsi/dc395x.c > @@ -4761,7 +4761,6 @@ static struct scsi_host_template dc395x_ > .cmd_per_lun= DC395x_MAX_CMD_PER_LUN, > .eh_abort_handler = dc395x_eh_abort, > .eh_bus_reset_handler = dc395x_eh_bus_reset, > - .unchecked_isa_dma = 0, > .use_clustering = DISABLE_CLUSTERING, > }; > > Index: linux/drivers/scsi/eata_pio.c > === > --- linux.orig/drivers/scsi/eata_pio.c > +++ linux/drivers/scsi/eata_pio.c > @@ -815,8 +815,6 @@ static int register_pio_HBA(long base, s > else > hd->primary = 1; > > - sh->unchecked_isa_dma = 0; /* We can only do PIO */ > - > hd->next = NULL;/* build a linked list of all HBAs */ > hd->prev = last_HBA; > if (hd->prev != NULL) > Index: linux/drivers/scsi/hptiop.c > === > --- linux.orig/drivers/scsi/hptiop.c > +++ linux/drivers/scsi/hptiop.c > @@ -903,7 +903,6 @@ static struct scsi_host_template driver_ > .eh_device_reset_handler= hptiop_reset, > .eh_bus_reset_handler = hptiop_reset, > .info = hptiop_info, > - .unchecked_isa_dma = 0, > .emulated = 0, > .use_clustering = ENABLE_CLUSTERING, > .proc_name = driver_name, > Index: linux/drivers/scsi/ips.c > === > --- linux.orig/drivers/scsi/ips.c > +++ linux/drivers/scsi/ips.c > @@ -6842,7 +6842,6 @@ ips_register_scsi(int index) > sh->sg_tablesize = sh->hostt->sg_tablesize; > sh->can_queue = sh->hostt->can_queue; > sh->cmd_per_lun = sh->hostt->cmd_per_lun; > - sh->unchecked_isa_dma = sh->hostt->unchecked_isa_dma; > sh->use_clustering = sh->hostt->use_clustering; > sh->max_sectors = 128; > > Index: linux/drivers/scsi/mac_scsi.c > === > --- linux.orig/drivers/scsi/mac_scsi.c > +++ linux/drivers/scsi/mac_scsi.c > @@ -592,7 +592,6 @@ static struct scsi_host_template driver_ > .this_id= 7, > .sg_tablesize = SG_ALL, > .cmd_per_lun= CMD_PER_LUN, > - .unchecked_isa_dma = 0, > .use_clustering = DISABLE_CLUSTERING > }; > > Index: linux/drivers/scsi/scsi_debug.c > === > --- linux.orig/drivers/scsi/scsi_debug.c > +++ linux/drivers/scsi/scsi_debug.c > @@ -221,7 +221,6 @@ static struct scsi_host_template sdebug_ > .sg_tablesize = 256, > .cmd_per_lun = 16, > .max_sectors = 0x, > - .unchecked_isa
RE: [PATCH] aacraid: READ_CAPACITY_16 shouldn't trust allocation length in cdb
ACK
Sincerely -- Mark Salyzyn
> -Original Message-
> From: fujita [mailto:[EMAIL PROTECTED] On Behalf Of FUJITA Tomonori
> Sent: Sunday, February 24, 2008 6:25 PM
> To: linux-scsi@vger.kernel.org
> Cc: [EMAIL PROTECTED]; FUJITA Tomonori; Salyzyn, Mark; James Bottomley
> Subject: [PATCH] aacraid: READ_CAPACITY_16 shouldn't trust
> allocation length in cdb
>
> When aacraid spoofs READ_CAPACITY_16, it assumes that the data length
> in the sg list is equal to allocation length in cdb. But sg can put
> any value in scb so the driver needs to check both the data length in
> the sg list and allocation length in cdb.
>
> If allocation length is larger than the response length that the
> driver expects, it clears the data buffer in the sg list to zero but
> it doesn't need to do. Just setting resid is fine.
>
> Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]>
> Cc: Mark Salyzyn <[EMAIL PROTECTED]>
> Cc: James Bottomley <[EMAIL PROTECTED]>
> ---
> drivers/scsi/aacraid/aachba.c | 22 +++---
> 1 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/scsi/aacraid/aachba.c
> b/drivers/scsi/aacraid/aachba.c
> index c05092f..b9fc9b1 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -2047,6 +2047,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
> {
> u64 capacity;
> char cp[13];
> + unsigned int alloc_len;
>
> dprintk((KERN_DEBUG "READ CAPACITY_16 command.\n"));
> capacity = fsa_dev_ptr[cid].size - 1;
> @@ -2063,18 +2064,17 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
> cp[10] = 2;
> cp[11] = 0;
> cp[12] = 0;
> - aac_internal_transfer(scsicmd, cp, 0,
> - min_t(size_t, scsicmd->cmnd[13], sizeof(cp)));
> - if (sizeof(cp) < scsicmd->cmnd[13]) {
> - unsigned int len, offset = sizeof(cp);
>
> - memset(cp, 0, offset);
> - do {
> - len = min_t(size_t,
> scsicmd->cmnd[13] - offset,
> - sizeof(cp));
> -
> aac_internal_transfer(scsicmd, cp, offset, len);
> - } while ((offset += len) < scsicmd->cmnd[13]);
> - }
> + alloc_len = ((scsicmd->cmnd[10] << 24)
> ++ (scsicmd->cmnd[11] << 16)
> ++ (scsicmd->cmnd[12] << 8) +
> scsicmd->cmnd[13]);
> +
> + alloc_len = min_t(size_t, alloc_len, sizeof(cp));
> + aac_internal_transfer(scsicmd, cp, 0, alloc_len);
> +
> + if (alloc_len < scsi_bufflen(scsicmd))
> + scsi_set_resid(scsicmd,
> + scsi_bufflen(scsicmd)
> - alloc_len);
>
> /* Do not cache partition table for arrays */
> scsicmd->device->removable = 1;
> --
> 1.5.3.7
>
>
-
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
[PATCH 2/2] ps3rom: Simplify fill_from_dev_buffer()
Subject: [PATCH] ps3rom: Simplify fill_from_dev_buffer()
As we no longer need to calculate the data length of the whole scatterlist,
we can abort the loop earlier and coalesce req_len and act_len into one
variable, making fill_from_dev_buffer() more similar to fetch_to_dev_buffer().
Signed-off-by: Geert Uytterhoeven <[EMAIL PROTECTED]>
---
drivers/scsi/ps3rom.c | 30 ++
1 file changed, 14 insertions(+), 16 deletions(-)
--- a/drivers/scsi/ps3rom.c
+++ b/drivers/scsi/ps3rom.c
@@ -95,7 +95,7 @@ static int ps3rom_slave_configure(struct
*/
static int fill_from_dev_buffer(struct scsi_cmnd *cmd, const void *buf)
{
- int k, req_len, act_len, len, active;
+ int k, req_len, len, fin;
void *kaddr;
struct scatterlist *sgpnt;
unsigned int buflen;
@@ -107,24 +107,22 @@ static int fill_from_dev_buffer(struct s
if (!scsi_sglist(cmd))
return -1;
- active = 1;
- req_len = act_len = 0;
+ req_len = fin = 0;
scsi_for_each_sg(cmd, sgpnt, scsi_sg_count(cmd), k) {
- if (active) {
- kaddr = kmap_atomic(sg_page(sgpnt), KM_IRQ0);
- len = sgpnt->length;
- if ((req_len + len) > buflen) {
- active = 0;
- len = buflen - req_len;
- }
- memcpy(kaddr + sgpnt->offset, buf + req_len, len);
- flush_kernel_dcache_page(sg_page(sgpnt));
- kunmap_atomic(kaddr, KM_IRQ0);
- act_len += len;
+ kaddr = kmap_atomic(sg_page(sgpnt), KM_IRQ0);
+ len = sgpnt->length;
+ if ((req_len + len) > buflen) {
+ len = buflen - req_len;
+ fin = 1;
}
- req_len += sgpnt->length;
+ memcpy(kaddr + sgpnt->offset, buf + req_len, len);
+ flush_kernel_dcache_page(sg_page(sgpnt));
+ kunmap_atomic(kaddr, KM_IRQ0);
+ req_len += len;
+ if (fin)
+ break;
}
- scsi_set_resid(cmd, buflen - act_len);
+ scsi_set_resid(cmd, buflen - req_len);
return 0;
}
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
[PATCH 1/2] ps3rom: fix wrong resid calculation bug
From: FUJITA Tomonori <[EMAIL PROTECTED]> sg driver rounds up the length in struct scatterlist to be a multiple of 512 in some conditions. So LLDs can't use the data length in a sg list to calculate residual. Instead, the length in struct scsi_cmnd should be used. [Geert: the variable buflen already contains scsi_bufflen(cmd)] Signed-off-by: FUJITA Tomonori <[EMAIL PROTECTED]> Signed-off-by: Geert Uytterhoeven <[EMAIL PROTECTED]> Cc: James Bottomley <[EMAIL PROTECTED]> --- drivers/scsi/ps3rom.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- a/drivers/scsi/ps3rom.c +++ b/drivers/scsi/ps3rom.c @@ -124,7 +124,7 @@ static int fill_from_dev_buffer(struct s } req_len += sgpnt->length; } - scsi_set_resid(cmd, req_len - act_len); + scsi_set_resid(cmd, buflen - act_len); return 0; } 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] ps3rom: fix wrong resid calculation bug
On Sun, 24 Feb 2008, FUJITA Tomonori wrote: > sg driver rounds up the length in struct scatterlist to be a multiple > of 512 in some conditions. So LLDs can't use the data length in a sg > list to calculate residual. Instead, the length in struct scsi_cmnd > should be used. Thanks! > --- a/drivers/scsi/ps3rom.c > +++ b/drivers/scsi/ps3rom.c > @@ -124,7 +124,7 @@ static int fill_from_dev_buffer(struct scsi_cmnd *cmd, > const void *buf) > } > req_len += sgpnt->length; > } > - scsi_set_resid(cmd, req_len - act_len); > + scsi_set_resid(cmd, scsi_bufflen(cmd) - act_len); > return 0; > } My 2 comments: - The variable buflen already contains scsi_bufflen(cmd), - We no longer need to calculate the data length of the whole scatterlist. I'll follow up to this email with a replacement patch and an addendum patch. 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] Don't use unnecessary GFP_ATOMIC in sg.c v2
Andi Kleen wrote:
Don't use unnecessary GFP_ATOMIC in sg.c v2
[Update for the previous version of the patch]
sg.c uses GFP_ATOMIC for a lot of allocations where it isn't necessary.
There are no locks hold and I don't see any other reasons why GFP_ATOMIC
should be needed here. So remove them.
Depends on the earlier GFP_DMA patchkit, but only because it happens
to patch the same places (no real functional dependency)
Tested by burning a CD on a kernel with all lock/sleep/etc. debugging enabled.
v2: Actually always use GFP_KERNEL instead of 0 (which is equivalent to
GFP_ATOMIC). Retested.
Oh no, not again. This isn't the first time kernel folks
have tried to demote the sg driver's GFP_ATOMIC to GFP_KERNEL.
In the past it has ended in grief. The driver was written
to attempt _fast_ allocation and if that failed then:
- lessen the requested allocation (e.g. smaller elements
but more of them in a scatter gather list), or
- report the error back to the user (i.e. ENOMEM) assuming
that they are knowledgeable enough to do something about it
(e.g. do two smaller READs rather than one larger one).
With GFP_KERNEL in place high end performance suffers.
I wouldn't consider cdrecord a high performance app.
Doug Gilbert
Signed-off-by: Andi Kleen <[EMAIL PROTECTED]>
---
drivers/scsi/sg.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
Index: linux/drivers/scsi/sg.c
===
--- linux.orig/drivers/scsi/sg.c
+++ linux/drivers/scsi/sg.c
@@ -745,7 +745,7 @@ sg_common_write(Sg_fd * sfp, Sg_request
if (scsi_execute_async(sdp->device, cmnd, hp->cmd_len, data_dir, srp->data.buffer,
hp->dxfer_len, srp->data.k_use_sg, timeout,
SG_DEFAULT_RETRIES, srp, sg_cmd_done,
- GFP_ATOMIC)) {
+ GFP_KERNEL)) {
SCSI_LOG_TIMEOUT(1, printk("sg_common_write: scsi_execute_async
failed\n"));
/*
* most likely out of mem, but could also be a bad map
@@ -1654,7 +1654,7 @@ static int
sg_build_sgat(Sg_scatter_hold * schp, const Sg_fd * sfp, int tablesize)
{
int sg_bufflen = tablesize * sizeof(struct scatterlist);
- gfp_t gfp_flags = GFP_ATOMIC | __GFP_NOWARN;
+ gfp_t gfp_flags = GFP_KERNEL | __GFP_NOWARN;
schp->buffer = kzalloc(sg_bufflen, gfp_flags);
if (!schp->buffer)
@@ -1694,7 +1694,7 @@ st_map_user_pages(struct scatterlist *sg
if (count == 0)
return 0;
- if ((pages = kmalloc(max_pages * sizeof(*pages), GFP_ATOMIC)) == NULL)
+ if ((pages = kmalloc(max_pages * sizeof(*pages), GFP_KERNEL)) == NULL)
return -ENOMEM;
/* Try to fault in all of the necessary pages */
@@ -2323,7 +2323,7 @@ sg_add_sfp(Sg_device * sdp, int dev)
unsigned long iflags;
int bufflen;
- sfp = kzalloc(sizeof(*sfp), GFP_ATOMIC | __GFP_NOWARN);
+ sfp = kzalloc(sizeof(*sfp), GFP_KERNEL|__GFP_NOWARN);
if (!sfp)
return NULL;
@@ -2452,7 +2452,7 @@ sg_page_malloc(int rqSz, int *retSzp)
if ((rqSz <= 0) || (NULL == retSzp))
return resp;
- page_mask = GFP_ATOMIC | __GFP_COMP | __GFP_NOWARN;
+ page_mask = GFP_KERNEL | __GFP_COMP | __GFP_NOWARN;
for (order = 0, a_size = PAGE_SIZE; a_size < rqSz;
order++, a_size <<= 1) ;
-
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
-
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 added to scsi-rc-fixes-2.6: [SCSI] arcmsr: fix message allocation
Sorry, maybe I did not ask distinctly enough.
I mean if I would like to allocate a memory space from ZONE_DMA for atomic
context, why can I not use kmalloc(1032, GFP_ATOMIC|GFP_DMA)?
In case of lack of GFP_DMA, kmalloc would grab the memory from ZONE_HIGH or
ZONE_HIGHMEM, isn't it?(I read it from the textbook of Linux Kernel
Development by Robert Love)
Or the basic is that you don't think it is necessary to allocate a memory
space from DMA area?
Please give me some comments.
Thank you very much, :-）
-Original Message-
From: Daniel Drake [mailto:[EMAIL PROTECTED]
Sent: Monday, February 25, 2008 6:15 PM
To: [EMAIL PROTECTED]
Cc: 'James Bottomley'; linux-scsi@vger.kernel.org
Subject: Re: Patch added to scsi-rc-fixes-2.6: [SCSI] arcmsr: fix message
allocation
nickcheng wrote:
> Hi,
> I definitely agree it is in atomic context but why is the memory not for
> DMA?
> Would you please show me why?
It would probably be easier if you could explain where you believe the
memory IS used for DMA :)
Anyway, looking at ARCMSR_MESSAGE_READ_RQBUFFER
current code does this:
ver_addr = kmalloc(1032, GFP_ATOMIC);
Here are the cases when that buffer is used:
checking for successful malloc: not DMA
if (!ver_addr) {
copying the address: not DMA
ptmpQbuffer = ver_addr;
memcpying 1 byte into the buffer: not DMA
memcpy(ptmpQbuffer, pQbuffer, 1);
incrementing address: not DMA
ptmpQbuffer++;
memcpying from the buffer: not DMA
memcpy(pcmdmessagefld->messagedatabuffer, ver_addr,
allxfer_len);
freeing the buffer: not DMA
kfree(ver_addr);
Daniel
-
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 added to scsi-rc-fixes-2.6: [SCSI] arcmsr: fix message allocation
nickcheng wrote:
Hi,
I definitely agree it is in atomic context but why is the memory not for
DMA?
Would you please show me why?
It would probably be easier if you could explain where you believe the
memory IS used for DMA :)
Anyway, looking at ARCMSR_MESSAGE_READ_RQBUFFER
current code does this:
ver_addr = kmalloc(1032, GFP_ATOMIC);
Here are the cases when that buffer is used:
checking for successful malloc: not DMA
if (!ver_addr) {
copying the address: not DMA
ptmpQbuffer = ver_addr;
memcpying 1 byte into the buffer: not DMA
memcpy(ptmpQbuffer, pQbuffer, 1);
incrementing address: not DMA
ptmpQbuffer++;
memcpying from the buffer: not DMA
memcpy(pcmdmessagefld->messagedatabuffer, ver_addr,
allxfer_len);
freeing the buffer: not DMA
kfree(ver_addr);
Daniel
-
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
[Bug 7246] 3w-xxxx, IOMMU and >1go RAM
http://bugzilla.kernel.org/show_bug.cgi?id=7246 [EMAIL PROTECTED] changed: What|Removed |Added CC||[EMAIL PROTECTED] ||m --- Comment #12 from [EMAIL PROTECTED] 2008-02-25 01:34 --- (In reply to comment #11) > If there is no more problems then it looks like the bug can be closed now. > Thanks. > The bug still exists as you can see in the following bug report on debian: "linux-2.6.18-6-amd64: 3w- v1.26.02.001 causes datacorruption with AMD64/EM64T with >2GB RAM" http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=464923 The problem is with the driver included in the kernel. More information on http://www.3ware.com/KB/article.aspx?id=15243 I'll quote the first post of the bug report, it includes very relevant/useful information: " Package: linux-2.6.18-6-amd64 Severity: critical Tags: patch Justification: causes serious data loss How to reproduce: 10036:/tmp# uname -a Linux 10036 2.6.18-6-amd64 #1 SMP Wed Jan 23 06:27:23 UTC 2008 x86_64 GNU/Linux 10036:/tmp# grep MemTotal /proc/meminfo MemTotal: 8179992 kB 10036:/tmp# dd if=/dev/urandom bs=1048576 count=10240 | tee testfile | md5sum 00a513dc3da637c4c86557102b0e6098 - 10239+1 records in 10239+1 records out 10737297534 bytes (11 GB) copied, 1584.6 seconds, 6.8 MB/s 10036:/tmp# md5sum testfile bfceb91a358dfc3d09e22ad74b7ebefb testfile 10036:/tmp# How to fix: There is a 3ware KB article on this: http://www.3ware.com/KB/article.aspx?id=15243 This includes "3ware Storage Controller device driver for Linux v1.26.03.000-2.6.18." >From the driver: 1.26.03.000 - Use default DMA data direction to prevent data corruption when using SWIOTLB with 4GB+ on EM64T. Installing this fixes the problem for me. -- System Information: Debian Release: 4.0 APT prefers stable APT policy: (500, 'stable') Architecture: i386 (i686) Shell: /bin/sh linked to /bin/bash Kernel: Linux 2.6.18-4-vserver-k7 Locale: LANG=C, LC_CTYPE=C (charmap=ANSI_X3.4-1968) " My server installation is also suffering from this bug (datacorruption): Debian Release: 4.0 (stable) Architecture: amd64 (64bits) Kernel: 2.6.18-5-amd64 RAM: 4GB RAID Controller: 3w-8500 series Processor: Intel Xeon - EMT64 As I've stated before, the problem resides in the driver included in the kernel, so it's just a matter of replacing it as a patch in the kernel. -- Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee. - 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
[PATCH] Don't use unnecessary GFP_ATOMIC in sg.c v2
Don't use unnecessary GFP_ATOMIC in sg.c v2
[Update for the previous version of the patch]
sg.c uses GFP_ATOMIC for a lot of allocations where it isn't necessary.
There are no locks hold and I don't see any other reasons why GFP_ATOMIC
should be needed here. So remove them.
Depends on the earlier GFP_DMA patchkit, but only because it happens
to patch the same places (no real functional dependency)
Tested by burning a CD on a kernel with all lock/sleep/etc. debugging enabled.
v2: Actually always use GFP_KERNEL instead of 0 (which is equivalent to
GFP_ATOMIC). Retested.
Signed-off-by: Andi Kleen <[EMAIL PROTECTED]>
---
drivers/scsi/sg.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
Index: linux/drivers/scsi/sg.c
===
--- linux.orig/drivers/scsi/sg.c
+++ linux/drivers/scsi/sg.c
@@ -745,7 +745,7 @@ sg_common_write(Sg_fd * sfp, Sg_request
if (scsi_execute_async(sdp->device, cmnd, hp->cmd_len, data_dir,
srp->data.buffer,
hp->dxfer_len, srp->data.k_use_sg, timeout,
SG_DEFAULT_RETRIES, srp, sg_cmd_done,
- GFP_ATOMIC)) {
+ GFP_KERNEL)) {
SCSI_LOG_TIMEOUT(1, printk("sg_common_write: scsi_execute_async
failed\n"));
/*
* most likely out of mem, but could also be a bad map
@@ -1654,7 +1654,7 @@ static int
sg_build_sgat(Sg_scatter_hold * schp, const Sg_fd * sfp, int tablesize)
{
int sg_bufflen = tablesize * sizeof(struct scatterlist);
- gfp_t gfp_flags = GFP_ATOMIC | __GFP_NOWARN;
+ gfp_t gfp_flags = GFP_KERNEL | __GFP_NOWARN;
schp->buffer = kzalloc(sg_bufflen, gfp_flags);
if (!schp->buffer)
@@ -1694,7 +1694,7 @@ st_map_user_pages(struct scatterlist *sg
if (count == 0)
return 0;
- if ((pages = kmalloc(max_pages * sizeof(*pages), GFP_ATOMIC)) == NULL)
+ if ((pages = kmalloc(max_pages * sizeof(*pages), GFP_KERNEL)) == NULL)
return -ENOMEM;
/* Try to fault in all of the necessary pages */
@@ -2323,7 +2323,7 @@ sg_add_sfp(Sg_device * sdp, int dev)
unsigned long iflags;
int bufflen;
- sfp = kzalloc(sizeof(*sfp), GFP_ATOMIC | __GFP_NOWARN);
+ sfp = kzalloc(sizeof(*sfp), GFP_KERNEL|__GFP_NOWARN);
if (!sfp)
return NULL;
@@ -2452,7 +2452,7 @@ sg_page_malloc(int rqSz, int *retSzp)
if ((rqSz <= 0) || (NULL == retSzp))
return resp;
- page_mask = GFP_ATOMIC | __GFP_COMP | __GFP_NOWARN;
+ page_mask = GFP_KERNEL | __GFP_COMP | __GFP_NOWARN;
for (order = 0, a_size = PAGE_SIZE; a_size < rqSz;
order++, a_size <<= 1) ;
-
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
