* RE: [PATCH2.5] ips 3/4: use pci_dma APIs and remove GFP abuse
@ 2003-08-18 16:09 Jeffery, David
2003-08-18 23:52 ` Jeff Garzik
0 siblings, 1 reply; 4+ messages in thread
From: Jeffery, David @ 2003-08-18 16:09 UTC (permalink / raw)
To: 'Jeff Garzik'; +Cc: linux-scsi
> -----Original Message-----
> From: Jeff Garzik
> Sent: Friday, August 15, 2003 5:10 PM
> To: Jeffery, David
> Cc: linux-scsi
> Subject: Re: [PATCH2.5] ips 3/4: use pci_dma APIs and remove GFP abuse
>
>
> David Jeffery wrote:
> > -#if !defined(__i386__) && !defined(__ia64__)
> > -#error "This driver has only been tested on the x86/ia64 platforms"
> > +#if !defined(__i386__) && !defined(__ia64__) &&
> !defined(__x86_64__)
> > +#error "This driver has only been tested on the
> x86/ia64/x86_64 platforms"
> > #endif
>
> This should be removed completely.
>
> Under Linux, you limit the architectures that build your driver via
> Kconfig (or in 2.4, Config.in) dependencies.
>
> Further, Linux actively encourages that all PCI platforms at least
> attempt to build PCI drivers -- even if the company does not
> support the
> driver on that platform. This leads to an increase in driver
> quality,
> as it increases the opportunities others have to point out bugs, and
> offer fixes.
I need to fix up the Kconfig file. How about I also downgrade that to
a #warning like Andi suggested earlier?
>
> > #if LINUX_VERSION_CODE <= KERNEL_VERSION(2,5,0)
> > @@ -250,6 +250,7 @@
> > static int ips_ioctlsize = IPS_IOCTL_SIZE; /* Size of the
> ioctl buffer */
> > static int ips_cd_boot; /* Booting from Manager
> CD */
> > static char *ips_FlashData = NULL; /* CD Boot - Flash Data
> Buffer */
> > +static dma_addr_t ips_flashbusaddr;
> > static long ips_FlashDataInUse; /* CD Boot - Flash Data
> In Use Flag */
> > static uint32_t MaxLiteCmds = 32; /* Max Active Cmds for
> a Lite Adapter */
> > static Scsi_Host_Template ips_driver_template = {
> > @@ -589,17 +590,6 @@
> > ips_setup(ips);
> > #endif
> >
> > - /* If Booting from the Manager CD, Allocate a large
> Flash */
> > - /* Buffer ( so we won't need to allocate one for each
> adapter ). */
> > - if (ips_cd_boot) {
> > - ips_FlashData = (char *)
> __get_free_pages(IPS_INIT_GFP, 7);
> > - if (ips_FlashData == NULL) {
> > - /* The validity of this pointer is
> checked in ips_make_passthru() before it is used */
> > - printk(KERN_WARNING
> > - "ERROR: Can't Allocate Large
> Buffer for Flashing\n");
> > - }
> > - }
> > -
> > for (i = 0; i < ips_num_controllers; i++) {
> > if (ips_register_scsi(i))
> > ips_free(ips_ha[i]);
> > @@ -1630,21 +1620,20 @@
> > ips_alloc_passthru_buffer(ips_ha_t * ha, int length)
> > {
> > void *bigger_buf;
> > - int count;
> > - int order;
> > + dma_addr_t dma_busaddr;
> >
> > - if (ha->ioctl_data && length <= (PAGE_SIZE << ha->ioctl_order))
> > + if (ha->ioctl_data && length <= ha->ioctl_len)
> > return 0;
> > /* there is no buffer or it's not big enough, allocate
> a new one */
> > - for (count = PAGE_SIZE, order = 0;
> > - count < length; order++, count <<= 1) ;
> > - bigger_buf = (void *) __get_free_pages(IPS_ATOMIC_GFP, order);
> > + bigger_buf = pci_alloc_consistent(ha->pcidev, length,
> &dma_busaddr);
>
> It's not wrong... but it's a bit nasty to be calling
> pci_alloc_consistent from the interrupt handler. Typically,
> if you can,
> it would be better to have buffers waiting for you, and you
> simply map
> them here, using pci_map_single or pci_map_page or whatever.
Normally, that is what happens. A new, larger buffer will
only be allocated in a few very rare cases. No user should
ever hit those allocations when using the ServeRAID tools.
Only a few special commands like flashing firmware should
ever trigger reallocation.
> Also, as a tangent, I was thinking that it might be
> beneficial to move
> ips_next() to a tasklet. Change all callers to call
> schedule_tasklet()
> instead. That should allow more streamlined usage, IMO...
Reworking ips_next is on my large list of Things To Do if I
Had Time.
>
>
> > @@ -7394,7 +7366,7 @@
> > return ips_abort_init(ha, index);
> > }
> >
> > - ha->nvram = kmalloc(sizeof (IPS_NVRAM_P5), IPS_INIT_GFP);
> > + ha->nvram = kmalloc(sizeof (IPS_NVRAM_P5), GFP_KERNEL);
> >
> > if (!ha->nvram) {
> > IPS_PRINTK(KERN_WARNING, pci_dev,
> > @@ -7402,7 +7374,7 @@
> > return ips_abort_init(ha, index);
> > }
> >
> > - ha->subsys = kmalloc(sizeof (IPS_SUBSYS), IPS_INIT_GFP);
> > + ha->subsys = kmalloc(sizeof (IPS_SUBSYS), GFP_KERNEL);
>
> Looks good. Now if we could convince you to change structs to style
> "struct foo" instead of SHOUTING STYLE, it would be even cooler :)
The code still has plenty of warts and I wish I had more time to work
on them. (Notice a theme about time? :)
David Jeffery
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH2.5] ips 3/4: use pci_dma APIs and remove GFP abuse
2003-08-18 16:09 [PATCH2.5] ips 3/4: use pci_dma APIs and remove GFP abuse Jeffery, David
@ 2003-08-18 23:52 ` Jeff Garzik
0 siblings, 0 replies; 4+ messages in thread
From: Jeff Garzik @ 2003-08-18 23:52 UTC (permalink / raw)
To: Jeffery, David; +Cc: linux-scsi
Jeffery, David wrote:
>>-----Original Message-----
>>From: Jeff Garzik
>>Sent: Friday, August 15, 2003 5:10 PM
>>To: Jeffery, David
>>Cc: linux-scsi
>>Subject: Re: [PATCH2.5] ips 3/4: use pci_dma APIs and remove GFP abuse
>>
>>
>>David Jeffery wrote:
>>
>>>-#if !defined(__i386__) && !defined(__ia64__)
>>>-#error "This driver has only been tested on the x86/ia64 platforms"
>>>+#if !defined(__i386__) && !defined(__ia64__) &&
>>
>>!defined(__x86_64__)
>>
>>>+#error "This driver has only been tested on the
>>
>>x86/ia64/x86_64 platforms"
>>
>>> #endif
>>
>>This should be removed completely.
>>
>>Under Linux, you limit the architectures that build your driver via
>>Kconfig (or in 2.4, Config.in) dependencies.
>>
>>Further, Linux actively encourages that all PCI platforms at least
>>attempt to build PCI drivers -- even if the company does not
>>support the
>>driver on that platform. This leads to an increase in driver
>>quality,
>>as it increases the opportunities others have to point out bugs, and
>>offer fixes.
>
>
> I need to fix up the Kconfig file. How about I also downgrade that to
> a #warning like Andi suggested earlier?
Yeah, that's definitely an improvement, thanks.
>>> #if LINUX_VERSION_CODE <= KERNEL_VERSION(2,
>>It's not wrong... but it's a bit nasty to be calling
>>pci_alloc_consistent from the interrupt handler. Typically,
>>if you can,
>>it would be better to have buffers waiting for you, and you
>>simply map
>>them here, using pci_map_single or pci_map_page or whatever.
>
>
> Normally, that is what happens. A new, larger buffer will
> only be allocated in a few very rare cases. No user should
> ever hit those allocations when using the ServeRAID tools.
> Only a few special commands like flashing firmware should
> ever trigger reallocation.
>
>
>>Also, as a tangent, I was thinking that it might be
>>beneficial to move
>>ips_next() to a tasklet. Change all callers to call
>>schedule_tasklet()
>>instead. That should allow more streamlined usage, IMO...
>
>
> Reworking ips_next is on my large list of Things To Do if I
> Had Time.
hehe :)
> The code still has plenty of warts and I wish I had more time to work
> on them. (Notice a theme about time? :)
Not an unique situation, either :) Thanks,
Jeff
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH2.5] ips 3/4: use pci_dma APIs and remove GFP abuse
@ 2003-08-15 12:43 David Jeffery
2003-08-15 21:09 ` Jeff Garzik
0 siblings, 1 reply; 4+ messages in thread
From: David Jeffery @ 2003-08-15 12:43 UTC (permalink / raw)
To: linux-scsi@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 235 bytes --]
This patch removes several places where we were using kmalloc flag
tricks to always get memory <4GB instead of using the proper
dma_alloc_consistent() API. It also no longer #errors on when compiled
for x86-64 kernels.
David Jeffery
[-- Attachment #2: test3.patch3 --]
[-- Type: text/x-patch, Size: 14483 bytes --]
diff -urN linux-2.6.0-test3_p2/drivers/scsi/ips.c linux-2.6.0-test3_p3/drivers/scsi/ips.c
--- linux-2.6.0-test3_p2/drivers/scsi/ips.c 2003-08-11 13:53:24.000000000 -0400
+++ linux-2.6.0-test3_p3/drivers/scsi/ips.c 2003-08-11 13:54:05.000000000 -0400
@@ -198,8 +198,8 @@
#define IPS_VERSION_HIGH "5.99"
#define IPS_VERSION_LOW ".01-BETA"
-#if !defined(__i386__) && !defined(__ia64__)
-#error "This driver has only been tested on the x86/ia64 platforms"
+#if !defined(__i386__) && !defined(__ia64__) && !defined(__x86_64__)
+#error "This driver has only been tested on the x86/ia64/x86_64 platforms"
#endif
#if LINUX_VERSION_CODE <= KERNEL_VERSION(2,5,0)
@@ -250,6 +250,7 @@
static int ips_ioctlsize = IPS_IOCTL_SIZE; /* Size of the ioctl buffer */
static int ips_cd_boot; /* Booting from Manager CD */
static char *ips_FlashData = NULL; /* CD Boot - Flash Data Buffer */
+static dma_addr_t ips_flashbusaddr;
static long ips_FlashDataInUse; /* CD Boot - Flash Data In Use Flag */
static uint32_t MaxLiteCmds = 32; /* Max Active Cmds for a Lite Adapter */
static Scsi_Host_Template ips_driver_template = {
@@ -589,17 +590,6 @@
ips_setup(ips);
#endif
- /* If Booting from the Manager CD, Allocate a large Flash */
- /* Buffer ( so we won't need to allocate one for each adapter ). */
- if (ips_cd_boot) {
- ips_FlashData = (char *) __get_free_pages(IPS_INIT_GFP, 7);
- if (ips_FlashData == NULL) {
- /* The validity of this pointer is checked in ips_make_passthru() before it is used */
- printk(KERN_WARNING
- "ERROR: Can't Allocate Large Buffer for Flashing\n");
- }
- }
-
for (i = 0; i < ips_num_controllers; i++) {
if (ips_register_scsi(i))
ips_free(ips_ha[i]);
@@ -1630,21 +1620,20 @@
ips_alloc_passthru_buffer(ips_ha_t * ha, int length)
{
void *bigger_buf;
- int count;
- int order;
+ dma_addr_t dma_busaddr;
- if (ha->ioctl_data && length <= (PAGE_SIZE << ha->ioctl_order))
+ if (ha->ioctl_data && length <= ha->ioctl_len)
return 0;
/* there is no buffer or it's not big enough, allocate a new one */
- for (count = PAGE_SIZE, order = 0;
- count < length; order++, count <<= 1) ;
- bigger_buf = (void *) __get_free_pages(IPS_ATOMIC_GFP, order);
+ bigger_buf = pci_alloc_consistent(ha->pcidev, length, &dma_busaddr);
if (bigger_buf) {
/* free the old memory */
- free_pages((unsigned long) ha->ioctl_data, ha->ioctl_order);
+ pci_free_consistent(ha->pcidev, ha->ioctl_len, ha->ioctl_data,
+ ha->ioctl_busaddr);
/* use the new memory */
ha->ioctl_data = (char *) bigger_buf;
- ha->ioctl_order = order;
+ ha->ioctl_len = length;
+ ha->ioctl_busaddr = dma_busaddr;
} else {
return -1;
}
@@ -1761,7 +1750,7 @@
static int
ips_flash_copperhead(ips_ha_t * ha, ips_passthru_t * pt, ips_scb_t * scb)
{
- int datasize, count;
+ int datasize;
/* Trombone is the only copperhead that can do packet flash, but only
* for firmware. No one said it had to make sence. */
@@ -1781,24 +1770,28 @@
pt->BasicStatus = 0;
return ips_flash_bios(ha, pt, scb);
} else if (pt->CoppCP.cmd.flashfw.packet_num == 0) {
- if (ips_FlashData && !test_and_set_bit(0, &ips_FlashDataInUse)) {
+ if (ips_FlashData && !test_and_set_bit(0, &ips_FlashDataInUse)){
ha->flash_data = ips_FlashData;
- ha->flash_order = 7;
+ ha->flash_busaddr = ips_flashbusaddr;
+ ha->flash_len = PAGE_SIZE << 7;
ha->flash_datasize = 0;
} else if (!ha->flash_data) {
datasize = pt->CoppCP.cmd.flashfw.total_packets *
pt->CoppCP.cmd.flashfw.count;
- for (count = PAGE_SIZE, ha->flash_order = 0;
- count < datasize; ha->flash_order++, count <<= 1) ;
- ha->flash_data =
- (char *) __get_free_pages(IPS_ATOMIC_GFP,
- ha->flash_order);
+ ha->flash_data = pci_alloc_consistent(ha->pcidev,
+ datasize,
+ &ha->flash_busaddr);
+ if (!ha->flash_data){
+ printk(KERN_WARNING "Unable to allocate a flash buffer\n");
+ return IPS_FAILURE;
+ }
ha->flash_datasize = 0;
+ ha->flash_len = datasize;
} else
return IPS_FAILURE;
} else {
if (pt->CoppCP.cmd.flashfw.count + ha->flash_datasize >
- (PAGE_SIZE << ha->flash_order)) {
+ ha->flash_len) {
ips_free_flash_copperhead(ha);
IPS_PRINTK(KERN_WARNING, ha->pcidev,
"failed size sanity check\n");
@@ -1987,7 +1980,8 @@
if (ha->flash_data == ips_FlashData)
test_and_clear_bit(0, &ips_FlashDataInUse);
else if (ha->flash_data)
- free_pages((unsigned long) ha->flash_data, ha->flash_order);
+ pci_free_consistent(ha->pcidev, ha->flash_len, ha->flash_data,
+ ha->flash_busaddr);
ha->flash_data = NULL;
}
@@ -2040,12 +2034,7 @@
if (pt->CmdBSize) {
scb->data_len = pt->CmdBSize;
- scb->data_busaddr = pci_map_single(ha->pcidev,
- ha->ioctl_data +
- sizeof (ips_passthru_t),
- pt->CmdBSize,
- IPS_DMA_DIR(scb));
- scb->flags |= IPS_SCB_MAP_SINGLE;
+ scb->data_busaddr = ha->ioctl_busaddr + sizeof (ips_passthru_t);
} else {
scb->data_busaddr = 0L;
}
@@ -2469,9 +2458,7 @@
} else {
/* Morpheus Family - Send Command to the card */
- buffer = kmalloc(0x1000, IPS_ATOMIC_GFP);
- if (!buffer)
- return;
+ buffer = ha->ioctl_data;
memset(buffer, 0, 0x1000);
@@ -2490,11 +2477,7 @@
scb->cmd.flashfw.total_packets = 1;
scb->cmd.flashfw.packet_num = 0;
scb->data_len = 0x1000;
- scb->data_busaddr =
- pci_map_single(ha->pcidev, buffer, scb->data_len,
- IPS_DMA_DIR(scb));
- scb->cmd.flashfw.buffer_addr = scb->data_busaddr;
- scb->flags |= IPS_SCB_MAP_SINGLE;
+ scb->cmd.flashfw.buffer_addr = ha->ioctl_busaddr;
/* issue the command */
if (((ret =
@@ -2503,7 +2486,6 @@
|| (ret == IPS_SUCCESS_IMM)
|| ((scb->basic_status & IPS_GSC_STATUS_MASK) > 1)) {
/* Error occurred */
- kfree(buffer);
return;
}
@@ -2513,11 +2495,8 @@
minor = buffer[0x1fe + 0xC0]; /* Offset 0x1fe after the header (0xc0) */
subminor = buffer[0x1fd + 0xC0]; /* Offset 0x1fd after the header (0xc0) */
} else {
- kfree(buffer);
return;
}
-
- kfree(buffer);
}
ha->bios_version[0] = hexDigits[(major & 0xF0) >> 4];
@@ -3984,11 +3963,7 @@
scb->cmd.basic_io.segment_4G = 0;
scb->cmd.basic_io.enhanced_sg = 0;
scb->data_len = sizeof (*ha->enq);
- scb->data_busaddr = pci_map_single(ha->pcidev, ha->enq,
- scb->data_len,
- IPS_DMA_DIR(scb));
- scb->cmd.basic_io.sg_addr = scb->data_busaddr;
- scb->flags |= IPS_SCB_MAP_SINGLE;
+ scb->cmd.basic_io.sg_addr = ha->enq_busaddr;
ret = IPS_SUCCESS;
break;
@@ -4550,7 +4525,8 @@
if (ha) {
if (ha->enq) {
- kfree(ha->enq);
+ pci_free_consistent(ha->pcidev, sizeof(IPS_ENQ),
+ ha->enq, ha->enq_busaddr);
ha->enq = NULL;
}
@@ -4578,11 +4554,11 @@
}
if (ha->ioctl_data) {
- free_pages((unsigned long) ha->ioctl_data,
- ha->ioctl_order);
+ pci_free_consistent(ha->pcidev, ha->ioctl_len,
+ ha->ioctl_data, ha->ioctl_busaddr);
ha->ioctl_data = NULL;
ha->ioctl_datasize = 0;
- ha->ioctl_order = 0;
+ ha->ioctl_len = 0;
}
ips_deallocatescbs(ha, ha->max_cmds);
@@ -5920,10 +5896,7 @@
scb->cmd.basic_io.sector_count = 0;
scb->cmd.basic_io.log_drv = 0;
scb->data_len = sizeof (*ha->enq);
- scb->data_busaddr = pci_map_single(ha->pcidev, ha->enq, scb->data_len,
- IPS_DMA_DIR(scb));
- scb->cmd.basic_io.sg_addr = scb->data_busaddr;
- scb->flags |= IPS_SCB_MAP_SINGLE;
+ scb->cmd.basic_io.sg_addr = ha->enq_busaddr;
/* send command */
if (((ret =
@@ -5966,10 +5939,7 @@
scb->cmd.basic_io.sector_count = 0;
scb->cmd.basic_io.log_drv = 0;
scb->data_len = sizeof (*ha->subsys);
- scb->data_busaddr = pci_map_single(ha->pcidev, ha->subsys,
- scb->data_len, IPS_DMA_DIR(scb));
- scb->cmd.basic_io.sg_addr = scb->data_busaddr;
- scb->flags |= IPS_SCB_MAP_SINGLE;
+ scb->cmd.basic_io.sg_addr = ha->ioctl_busaddr;
/* send command */
if (((ret =
@@ -5978,6 +5948,7 @@
|| ((scb->basic_status & IPS_GSC_STATUS_MASK) > 1))
return (0);
+ memcpy(ha->subsys, ha->ioctl_data, sizeof(*ha->subsys));
return (1);
}
@@ -6013,10 +5984,7 @@
scb->cmd.basic_io.op_code = IPS_CMD_READ_CONF;
scb->cmd.basic_io.command_id = IPS_COMMAND_ID(ha, scb);
scb->data_len = sizeof (*ha->conf);
- scb->data_busaddr = pci_map_single(ha->pcidev, ha->conf,
- scb->data_len, IPS_DMA_DIR(scb));
- scb->cmd.basic_io.sg_addr = scb->data_busaddr;
- scb->flags |= IPS_SCB_MAP_SINGLE;
+ scb->cmd.basic_io.sg_addr = ha->ioctl_busaddr;
/* send command */
if (((ret =
@@ -6037,7 +6005,8 @@
return (0);
}
-
+
+ memcpy(ha->conf, ha->ioctl_data, sizeof(*ha->conf));
return (1);
}
@@ -6072,11 +6041,10 @@
scb->cmd.nvram.reserved = 0;
scb->cmd.nvram.reserved2 = 0;
scb->data_len = sizeof (*ha->nvram);
- scb->data_busaddr = pci_map_single(ha->pcidev, ha->nvram,
- scb->data_len, IPS_DMA_DIR(scb));
- scb->cmd.nvram.buffer_addr = scb->data_busaddr;
- scb->flags |= IPS_SCB_MAP_SINGLE;
-
+ scb->cmd.nvram.buffer_addr = ha->ioctl_busaddr;
+ if (write)
+ memcpy(ha->ioctl_data, ha->nvram, sizeof(*ha->nvram));
+
/* issue the command */
if (((ret =
ips_send_wait(ha, scb, ips_cmd_timeout, intr)) == IPS_FAILURE)
@@ -6087,7 +6055,8 @@
return (0);
}
-
+ if (!write)
+ memcpy(ha->nvram, ha->ioctl_data, sizeof(*ha->nvram));
return (1);
}
@@ -7242,7 +7211,6 @@
uint16_t subdevice_id;
int j;
int index;
- uint32_t count;
dma_addr_t dma_address;
char *ioremap_ptr;
char *mem_ptr;
@@ -7367,9 +7335,13 @@
return ips_abort_init(ha, index);
}
}
+ if(ips_cd_boot && !ips_FlashData){
+ ips_FlashData = pci_alloc_consistent(pci_dev, PAGE_SIZE << 7,
+ &ips_flashbusaddr);
+ }
- ha->enq = kmalloc(sizeof (IPS_ENQ), IPS_INIT_GFP);
-
+ ha->enq = pci_alloc_consistent(pci_dev, sizeof (IPS_ENQ),
+ &ha->enq_busaddr);
if (!ha->enq) {
IPS_PRINTK(KERN_WARNING, pci_dev,
"Unable to allocate host inquiry structure\n");
@@ -7386,7 +7358,7 @@
ha->adapt->hw_status_start = dma_address;
ha->dummy = (void *) (ha->adapt + 1);
- ha->conf = kmalloc(sizeof (IPS_CONF), IPS_INIT_GFP);
+ ha->conf = kmalloc(sizeof (IPS_CONF), GFP_KERNEL);
if (!ha->conf) {
IPS_PRINTK(KERN_WARNING, pci_dev,
@@ -7394,7 +7366,7 @@
return ips_abort_init(ha, index);
}
- ha->nvram = kmalloc(sizeof (IPS_NVRAM_P5), IPS_INIT_GFP);
+ ha->nvram = kmalloc(sizeof (IPS_NVRAM_P5), GFP_KERNEL);
if (!ha->nvram) {
IPS_PRINTK(KERN_WARNING, pci_dev,
@@ -7402,7 +7374,7 @@
return ips_abort_init(ha, index);
}
- ha->subsys = kmalloc(sizeof (IPS_SUBSYS), IPS_INIT_GFP);
+ ha->subsys = kmalloc(sizeof (IPS_SUBSYS), GFP_KERNEL);
if (!ha->subsys) {
IPS_PRINTK(KERN_WARNING, pci_dev,
@@ -7410,19 +7382,18 @@
return ips_abort_init(ha, index);
}
- for (count = PAGE_SIZE, ha->ioctl_order = 0;
- count < ips_ioctlsize; ha->ioctl_order++, count <<= 1) ;
-
- ha->ioctl_data =
- (char *) __get_free_pages(IPS_INIT_GFP, ha->ioctl_order);
- ha->ioctl_datasize = count;
-
+ /* the ioctl buffer is now used during adapter initialization, so its
+ * successful allocation is now required */
+ if (ips_ioctlsize < PAGE_SIZE)
+ ips_ioctlsize = PAGE_SIZE;
+
+ ha->ioctl_data = pci_alloc_consistent(pci_dev, ips_ioctlsize,
+ &ha->ioctl_busaddr);
+ ha->ioctl_len = ips_ioctlsize;
if (!ha->ioctl_data) {
IPS_PRINTK(KERN_WARNING, pci_dev,
"Unable to allocate IOCTL data\n");
- ha->ioctl_data = NULL;
- ha->ioctl_order = 0;
- ha->ioctl_datasize = 0;
+ return ips_abort_init(ha, index);
}
/*
diff -urN linux-2.6.0-test3_p2/drivers/scsi/ips.h linux-2.6.0-test3_p3/drivers/scsi/ips.h
--- linux-2.6.0-test3_p2/drivers/scsi/ips.h 2003-08-11 12:43:05.000000000 -0400
+++ linux-2.6.0-test3_p3/drivers/scsi/ips.h 2003-08-11 13:54:05.000000000 -0400
@@ -128,22 +128,13 @@
#define min(x,y) ((x) < (y) ? x : y)
#endif
+ #define pci_dma_hi32(a) ((a >> 16) >> 16)
#define pci_dma_lo32(a) (a & 0xffffffff)
#if (BITS_PER_LONG > 32) || (defined CONFIG_HIGHMEM64G && defined IPS_HIGHIO)
#define IPS_ENABLE_DMA64 (1)
- #define pci_dma_hi32(a) (a >> 32)
#else
#define IPS_ENABLE_DMA64 (0)
- #define pci_dma_hi32(a) (0)
- #endif
-
- #if defined(__ia64__)
- #define IPS_ATOMIC_GFP (GFP_DMA | GFP_ATOMIC)
- #define IPS_INIT_GFP GFP_DMA
- #else
- #define IPS_ATOMIC_GFP GFP_ATOMIC
- #define IPS_INIT_GFP GFP_KERNEL
#endif
/*
@@ -1110,7 +1101,8 @@
uint16_t device_id; /* PCI device ID */
uint8_t slot_num; /* PCI Slot Number */
uint16_t subdevice_id; /* Subsystem device ID */
- uint8_t ioctl_order; /* Number of pages in ioctl */
+ int ioctl_len; /* size of ioctl buffer */
+ dma_addr_t ioctl_busaddr; /* dma address of ioctl buffer*/
uint8_t bios_version[8]; /* BIOS Revision */
uint32_t mem_addr; /* Memory mapped address */
uint32_t io_len; /* Size of IO Address */
@@ -1120,8 +1112,10 @@
ips_hw_func_t func; /* hw function pointers */
struct pci_dev *pcidev; /* PCI device handle */
char *flash_data; /* Save Area for flash data */
- u8 flash_order; /* Save Area for flash size order */
+ int flash_len; /* length of flash buffer */
u32 flash_datasize; /* Save Area for flash data size */
+ dma_addr_t flash_busaddr; /* dma address of flash buffer*/
+ dma_addr_t enq_busaddr; /* dma address of enq struct */
uint8_t requires_esl; /* Requires an EraseStripeLock */
} ips_ha_t;
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH2.5] ips 3/4: use pci_dma APIs and remove GFP abuse
2003-08-15 12:43 David Jeffery
@ 2003-08-15 21:09 ` Jeff Garzik
0 siblings, 0 replies; 4+ messages in thread
From: Jeff Garzik @ 2003-08-15 21:09 UTC (permalink / raw)
To: David Jeffery; +Cc: linux-scsi@vger.kernel.org
David Jeffery wrote:
> -#if !defined(__i386__) && !defined(__ia64__)
> -#error "This driver has only been tested on the x86/ia64 platforms"
> +#if !defined(__i386__) && !defined(__ia64__) && !defined(__x86_64__)
> +#error "This driver has only been tested on the x86/ia64/x86_64 platforms"
> #endif
This should be removed completely.
Under Linux, you limit the architectures that build your driver via
Kconfig (or in 2.4, Config.in) dependencies.
Further, Linux actively encourages that all PCI platforms at least
attempt to build PCI drivers -- even if the company does not support the
driver on that platform. This leads to an increase in driver quality,
as it increases the opportunities others have to point out bugs, and
offer fixes.
> #if LINUX_VERSION_CODE <= KERNEL_VERSION(2,5,0)
> @@ -250,6 +250,7 @@
> static int ips_ioctlsize = IPS_IOCTL_SIZE; /* Size of the ioctl buffer */
> static int ips_cd_boot; /* Booting from Manager CD */
> static char *ips_FlashData = NULL; /* CD Boot - Flash Data Buffer */
> +static dma_addr_t ips_flashbusaddr;
> static long ips_FlashDataInUse; /* CD Boot - Flash Data In Use Flag */
> static uint32_t MaxLiteCmds = 32; /* Max Active Cmds for a Lite Adapter */
> static Scsi_Host_Template ips_driver_template = {
> @@ -589,17 +590,6 @@
> ips_setup(ips);
> #endif
>
> - /* If Booting from the Manager CD, Allocate a large Flash */
> - /* Buffer ( so we won't need to allocate one for each adapter ). */
> - if (ips_cd_boot) {
> - ips_FlashData = (char *) __get_free_pages(IPS_INIT_GFP, 7);
> - if (ips_FlashData == NULL) {
> - /* The validity of this pointer is checked in ips_make_passthru() before it is used */
> - printk(KERN_WARNING
> - "ERROR: Can't Allocate Large Buffer for Flashing\n");
> - }
> - }
> -
> for (i = 0; i < ips_num_controllers; i++) {
> if (ips_register_scsi(i))
> ips_free(ips_ha[i]);
> @@ -1630,21 +1620,20 @@
> ips_alloc_passthru_buffer(ips_ha_t * ha, int length)
> {
> void *bigger_buf;
> - int count;
> - int order;
> + dma_addr_t dma_busaddr;
>
> - if (ha->ioctl_data && length <= (PAGE_SIZE << ha->ioctl_order))
> + if (ha->ioctl_data && length <= ha->ioctl_len)
> return 0;
> /* there is no buffer or it's not big enough, allocate a new one */
> - for (count = PAGE_SIZE, order = 0;
> - count < length; order++, count <<= 1) ;
> - bigger_buf = (void *) __get_free_pages(IPS_ATOMIC_GFP, order);
> + bigger_buf = pci_alloc_consistent(ha->pcidev, length, &dma_busaddr);
It's not wrong... but it's a bit nasty to be calling
pci_alloc_consistent from the interrupt handler. Typically, if you can,
it would be better to have buffers waiting for you, and you simply map
them here, using pci_map_single or pci_map_page or whatever.
Also, as a tangent, I was thinking that it might be beneficial to move
ips_next() to a tasklet. Change all callers to call schedule_tasklet()
instead. That should allow more streamlined usage, IMO...
> @@ -7394,7 +7366,7 @@
> return ips_abort_init(ha, index);
> }
>
> - ha->nvram = kmalloc(sizeof (IPS_NVRAM_P5), IPS_INIT_GFP);
> + ha->nvram = kmalloc(sizeof (IPS_NVRAM_P5), GFP_KERNEL);
>
> if (!ha->nvram) {
> IPS_PRINTK(KERN_WARNING, pci_dev,
> @@ -7402,7 +7374,7 @@
> return ips_abort_init(ha, index);
> }
>
> - ha->subsys = kmalloc(sizeof (IPS_SUBSYS), IPS_INIT_GFP);
> + ha->subsys = kmalloc(sizeof (IPS_SUBSYS), GFP_KERNEL);
Looks good. Now if we could convince you to change structs to style
"struct foo" instead of SHOUTING STYLE, it would be even cooler :)
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2003-08-18 23:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2003-08-18 16:09 [PATCH2.5] ips 3/4: use pci_dma APIs and remove GFP abuse Jeffery, David
2003-08-18 23:52 ` Jeff Garzik
-- strict thread matches above, loose matches on Subject: below --
2003-08-15 12:43 David Jeffery
2003-08-15 21:09 ` Jeff Garzik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox