Skip to contentSkip to conversation

Remove isa_unchecked_dma and some more GFP_DMAs in the mid layer v5

PATCH In review Andi Kleen 5 participants 42 messages Nov 15, 2008 – Nov 17, 2008

Subsystems (10)
lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

cover Remove isa_unchecked_dma and some more GFP_DMAs in the mid layer v5

[repost of the earlier patch kit, ported to 2.6.28rc4. Was acked by axboe earlier]
[Can this be considered for .29 please? This is the older origial
patchkit which was relatively uncontroversal before the mid layer sense buffer and with blk_*alloc* changes removed.]

v2: various cleanups, use dma_alloc_coherent instead of naked GFP_DMA
add blk_* allocators to avoid bouncing in SCSI scan
v3: Remove cmnd/sense buffer bouncing in advansys.
Replace sense_buffer_mask with a single bit
v4: this is the original patchkit which was relatively uncontroversal
without any sense buffer changes.
Just forward ported to a 2.6.26-rc8 tree.
v5: Ported to 2.6.28rc4

Original description:

This patchkit fixes all existing drivers that used isa_unchecked_dma to not need that anymore. I have some upcoming infrastructure changes for DMA memory management and isa_unchecked_dma was in the way.

Enabling isa_unchecked_dma had several effects:
- All incoming scsi_cmnds were in GFP_DMA memory.
Only one driver relied on that actually (advansys), the others all accessed the scsi_cmnds only with the CPU.
- scsi hostdata is allocated with GFP_DMA
A lot of drivers relied on that. I converted them all to allocate hostdata in a separate buffer linked to rom the scsi host structure.
- Enabling block layer bouncing for all data. That's the most important one. I changed all drivers to do that directly instead of relying on the mid layer for it.
- sense_buffer is allocated with GFP_DMA. That was also commonly required and not easy to fix so I created a separate host template field that enables sense_buffer bouncing.

Also while I was it I removed also a lot of GFP_DMAs in the frontend drivers which are not needed anymore because the block layer does the bouncing for all data anyways. Or rather we ask the block layer now to bounce.

The main problem of the patchkit is that is that I wasn't able to test the drivers because I don't have any of the hardware. All changes (except perhaps advansys) were relatively simple and straight forward so I don't expect many problems though.

If anybody has any of these ISA SCSI adapters and would be willing to test them with these patches that would be appreciated.
I suspect actually that some of the ISA drivers are actually already bitrotted independently of these changes. Hopefully they won't make anything worse though.

Patches against 2.6.28rc4

Also available as a git tree at

The following changes since commit 9bf1a2445f3c569098b8de7097ca324e65abecc2:
Linus Torvalds (1):

        Linux 2.6.28-rc5

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/ak/linux-misc-2.6 scsi-dma

Andi Kleen (21):
Add the alloc/get_pages_mask calls
Add blk_q_mask
Pass gfp to scsi_allocate_command
Add sense_buffer_isa to host template
Remove unchecked_isa in BusLogic
Remove unchecked_isa_dma in advansys.c
Remove unchecked_isa_dma in gdth
Remove unchecked_isa_dma in eata.c
Remove unchecked_isa_dma in aha1542
Remove unchecked_isa_dma in aha152x/wd7000/sym53c416/u14-34f/NCR53c406a Remove GFP_DMA uses in st/osst
Remove unchecked_isa_dma support for hostdata
Use blk_q_mask/get_pages_mask in sg driver
Rely on block layer bouncing for ISA DMA devices scanning Don't disable direct_io for unchecked_isa_dma in st.c Remove automatic block layer bouncing for unchecked_isa_dma Remove GFP_DMA use in sr.c
Remove unchecked_isa_dma from sysfs
Switch to a single SCSI command pool
Finally kill unchecked_isa_dma
Convert DMA buffers in ch.c to allocate via the block layer

 Documentation/scsi/scsi_mid_low_api.txt |    4 +-
 drivers/scsi/BusLogic.c                 |   77 +++++++++++----
 drivers/scsi/NCR53c406a.c               |    8 ++-
 drivers/scsi/advansys.c                 |  162 ++++++++++++++++++++-----------
 drivers/scsi/aha152x.c                  |   29 +++++-
 drivers/scsi/aha1542.c                  |   40 ++++++--
 drivers/scsi/ch.c                       |   16 ++-
 drivers/scsi/eata.c                     |   54 ++++++++---
 drivers/scsi/gdth.c                     |   81 ++++++++++++----
 drivers/scsi/hosts.c                    |    5 +-
 drivers/scsi/osst.c                     |   24 ++---
 drivers/scsi/osst.h                     |    2 -
 drivers/scsi/scsi.c                     |   93 +++++++++++-------
 drivers/scsi/scsi_lib.c                 |    2 -
 drivers/scsi/scsi_scan.c                |    6 +-
 drivers/scsi/scsi_sysfs.c               |    2 -
 drivers/scsi/sg.c                       |   36 ++------
 drivers/scsi/sr.c                       |    2 +-
 drivers/scsi/sr_ioctl.c                 |   13 +--
 drivers/scsi/sr_vendor.c                |    4 +-
 drivers/scsi/st.c                       |   37 +++----
 drivers/scsi/st.h                       |    2 -
 drivers/scsi/sym53c416.c                |    9 ++-
 drivers/scsi/u14-34f.c                  |   54 ++++++++---
 drivers/scsi/ultrastor.c                |   13 ++-
 drivers/scsi/wd7000.c                   |   48 ++++++---
 include/linux/blkdev.h                  |    5 +
 include/linux/gfp.h                     |   22 ++++-
 include/scsi/scsi_cmnd.h                |    5 +-
 include/scsi/scsi_host.h                |   14 ++-
 include/scsi/sg.h                       |    1 -
 31 files changed, 564 insertions(+), 306 deletions(-)

-Andi

author
#1 lore raw

PATCH 1/21 Add the alloc/get_pages_mask calls

[Merge note: this patch will be in a SCSI patchkit and in the core mask allocator patchkit. They are equivalent. If one of them is merged first simply drop it in the other]

Add the compat version of the *_pages_* calls. These just check if the mask is inside the DMA range and if yes set __GFP_DMA.

This is needed so that some code can start using the _mask calls early.

---
 include/linux/gfp.h |   22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)
include/linux/gfp.h +19 -3
Index: linux/include/linux/gfp.h
===================================================================
@@ -4,6 +4,7 @@
 #include <linux/mmzone.h>
 #include <linux/stddef.h>
 #include <linux/linkage.h>
+#include <asm/dma.h>  /* For TRAD_DMA_MASK/MAX_DMA_ADDRESS */
 
 struct vm_area_struct;
 
@@ -234,14 +235,29 @@
 #define __get_free_page(gfp_mask) \
 		__get_free_pages((gfp_mask),0)
 
-#define __get_dma_pages(gfp_mask, order) \
-		__get_free_pages((gfp_mask) | GFP_DMA,(order))
-
 extern void __free_pages(struct page *page, unsigned int order);
 extern void free_pages(unsigned long addr, unsigned int order);
 extern void free_hot_page(struct page *page);
 extern void free_cold_page(struct page *page);
 
+#define gfp_mask(m) ((__pa(MAX_DMA_ADDRESS - 1) & (m)) ? 0 : __GFP_DMA)
+
+#define alloc_pages_mask(gfp, size, mask) \
+	alloc_pages((gfp) | gfp_mask(mask), get_order(size))
+#define get_pages_mask(gfp, size, mask) \
+	((void *)__get_free_pages((gfp) | gfp_mask(mask), get_order(size)))
+#define __free_pages_mask(p, s) __free_pages((p), get_order(s))
+#define free_pages_mask(addr, s) free_pages((unsigned long)(addr), get_order(s))
+
+#define __get_dma_pages(gfp_mask, order) \
+		__get_free_pages((gfp_mask) | GFP_DMA, (order))
+
+#define get_page_mask(gfp, mask) get_pages_mask((gfp), PAGE_SIZE, mask)
+#define alloc_page_mask(gfp, mask) alloc_pages_mask((gfp), PAGE_SIZE, mask)
+
+#define __free_page_mask(page) __free_pages_mask(page, PAGE_SIZE)
+#define free_page_mask(addr) free_pages_mask((addr), PAGE_SIZE)
+
 #define __free_page(page) __free_pages((page), 0)
 #define free_page(addr) free_pages((addr),0)
 
author
#2 lore raw

PATCH 2/21 Add blk_q_mask

In reply to Andi Kleen

[Merge note: this patch will be in a SCSI patchkit and in the core mask allocator patchkit. They are equivalent. If one of them is merged first simply drop it in the other]

Converts the queue bounce_pfn to a DMA mask suitable for the mask allocator

---
 include/linux/blkdev.h |    5 +++++
 1 file changed, 5 insertions(+)
include/linux/blkdev.h +5 -0
Index: linux/include/linux/blkdev.h
===================================================================
@@ -966,6 +966,11 @@
 	return bdev->bd_block_size;
 }
 
+static inline u64 blk_q_mask(struct request_queue *q)
+{
+	return ~(-1LL << (PAGE_SHIFT + fls64(q->bounce_pfn)));
+}
+
 typedef struct {struct page *v;} Sector;
 
 unsigned char *read_dev_sector(struct block_device *, sector_t, Sector *);
maintainer
#3 lore raw

Re: [PATCH] [2/21] Add blk_q_mask

Quoted 9 lines · Andi Kleen

[Merge note: this patch will be in a SCSI patchkit and in the core mask allocator patchkit. They are equivalent. If one of them is merged first simply drop it in the other]

Converts the queue bounce_pfn to a DMA mask suitable for the mask allocator

I'm confused by this signoff chain. You're supposed to give signoffs from each of the entities whose hands the patch passed through ... unless you've taken to emailing your own patches to yourself, this doesn't look right ...

Could you just pick one? If it's about credit, then the reporting stats go by Author. All three signoffs will do is inflate your rank in the Signed-off-by stats (which I suppose we use for the kernel summit), but I don't think it will give equal credit to the three entities.

James

author
#4 lore raw

Re: [PATCH] [2/21] Add blk_q_mask

Quoted 1 line

I'm confused by this signoff chain. You're supposed to give signoffs

That is because I started writing this patchkit at SUSE, then worked a bit while I was just on my own (firstfloor) and now finally did some minor changes at Intel. The bulk of the work was all done under the SUSE banner, so if it's a problem I can remove the others, but I thought adding uptodate ones wouldn't hurt.

Quoted 6 lines

from each of the entities whose hands the patch passed through ... unless you've taken to emailing your own patches to yourself, this doesn't look right ...

Could you just pick one? If it's about credit, then the reporting stats go by Author. All three signoffs will do is inflate your rank in the

That wasn't my intention.

I'll remove the two newer ones in the next batch.

-Andi

#5 lore raw

Re: [PATCH] [2/21] Add blk_q_mask

In reply to James Bottomley
Quoted 4 lines · James Bottomley

Could you just pick one? If it's about credit, then the reporting stats go by Author. All three signoffs will do is inflate your rank in the Signed-off-by stats (which I suppose we use for the kernel summit), but I don't think it will give equal credit to the three entities.

It also risks annoying the person who collects stats for the kernel summit, who might react if he noticed someone was trying to game the system by arbitrary dividing the person's signed-off stats by 3, or otherwise adjust the statistic so it matches with reality, plus a little extra discount for the annoyance factor. :-)

- Ted

author
#6 lore raw

PATCH 3/21 Pass gfp to scsi_allocate_command

In reply to Andi Kleen

Instead of deciding low level based on unchecked_isa_dma pass in the GFP. This makes it easier to remove unchecked_isa_dma later and also simplifies some code.

---
 drivers/scsi/scsi.c      |   45 +++++++++++++++++++++++++++++----------------
 include/scsi/scsi_cmnd.h |    5 +++--
 2 files changed, 32 insertions(+), 18 deletions(-)
include/scsi/scsi_cmnd.h +3 -2
Index: linux/include/scsi/scsi_cmnd.h
===================================================================
@@ -148,8 +148,9 @@
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
 
-struct scsi_cmnd *scsi_allocate_command(gfp_t gfp_mask);
-void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd);
+struct scsi_cmnd *scsi_allocate_command(struct Scsi_Host *shost,
+					gfp_t gfp_mask);
+void scsi_free_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd);
 
 static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
 {
drivers/scsi/scsi.c +29 -16

116 lines not on this page · raw message

maintainer
#7 lore raw

Re: [PATCH] [3/21] Pass gfp to scsi_allocate_command

Quoted 10 lines · Andi Kleen
drivers/scsi/scsi.c +2 -2
-static void scsi_put_host_cmd_pool(gfp_t gfp_mask)
+static void scsi_put_host_cmd_pool(struct Scsi_Host *shost)
 {
        struct scsi_host_cmd_pool *pool;
 
        mutex_lock(&host_cmd_pool_mutex);
-       pool = (gfp_mask & __GFP_DMA) ? &scsi_cmd_dma_pool :
+       pool = (shost && shost->unchecked_isa_dma) ?

&scsi_cmd_dma_pool :

                &scsi_cmd_pool;

There's a conceptual problem with this: The API is designed to be used on host setup (i.e. when allocating a host based command for the driver) so the scsi_host may not be available (or at least not fully set up) when this is called.

What about making it take a mask instead? That should always be known by the driver.

James

author
#8 lore raw

Re: [PATCH] [3/21] Pass gfp to scsi_allocate_command

Quoted 6 lines · James Bottomley
Quoted 5 lines · Andi Kleen
drivers/scsi/scsi.c +1 -1
-static void scsi_put_host_cmd_pool(gfp_t gfp_mask)
+static void scsi_put_host_cmd_pool(struct Scsi_Host *shost)
 {
        struct scsi_host_cmd_pool *pool;
Quoted 10 lines
Quoted 5 lines
        mutex_lock(&host_cmd_pool_mutex);
(quoted diff) +1 -1
-       pool = (gfp_mask & __GFP_DMA) ? &scsi_cmd_dma_pool :
+       pool = (shost && shost->unchecked_isa_dma) ?

&scsi_cmd_dma_pool :

                &scsi_cmd_pool;

There's a conceptual problem with this: The API is designed to be used on host setup (i.e. when allocating a host based command for the driver) so the scsi_host may not be available (or at least not fully set up) when this is called.

I think it wasn't a problem on the ISA drivers I changed.

Quoted 3 lines

What about making it take a mask instead? That should always be known by the driver.

Ok.

-Andi

author
#9 lore raw

PATCH 4/21 Add sense_buffer_isa to host template

In reply to Andi Kleen

Instead of having the global "unchecked_isa_dma" bit add a single bit that tells the mid layer that the sense buffer needs to be ISA DMA'able.

Right now when the bit is set we still force all commands to the ISA DMA zone too, but that can change in the future.

---
 Documentation/scsi/scsi_mid_low_api.txt |    1 +
 drivers/scsi/hosts.c                    |    1 +
 drivers/scsi/scsi.c                     |   10 ++++++----
 include/scsi/scsi_host.h                |    8 ++++++++
 4 files changed, 16 insertions(+), 4 deletions(-)
include/scsi/scsi_host.h +8 -0
Index: linux/include/scsi/scsi_host.h
===================================================================
@@ -447,6 +447,11 @@
 	unsigned ordered_tag:1;
 
 	/*
+	 * True if sense buffers need to be ISA-DMAable
+	 */
+	unsigned sense_buffer_isa:1;
+
+	/*
 	 * Countdown for host blocking with no commands outstanding.
 	 */
 	unsigned int max_host_blocked;
@@ -607,6 +612,9 @@
 	 */
 	unsigned ordered_tag:1;
 
+	/* Sense buffer needs to be ISA dma'able */
+	unsigned sense_buffer_isa:1;
+
 	/* Task mgmt function in progress */
 	unsigned tmf_in_progress:1;
 
Documentation/scsi/scsi_mid_low_api.txt +1 -0
Index: linux/Documentation/scsi/scsi_mid_low_api.txt
===================================================================
@@ -1268,6 +1268,7 @@
                    instances (currently ordered by ascending host_no)
     my_devices   - a double linked list of pointers to struct scsi_device 
                    instances that belong to this host.
+    sense_buffer_isa - bit flag; true when the sense buffer needs to be ISA DMAable
     hostdata[0]  - area reserved for LLD at end of struct Scsi_Host. Size
                    is set by the second argument (named 'xtr_bytes') to
                    scsi_host_alloc() or scsi_register().
drivers/scsi/hosts.c +1 -0
Index: linux/drivers/scsi/hosts.c
===================================================================
@@ -364,6 +364,7 @@
 		shost->active_mode = MODE_INITIATOR;
 	else
 		shost->active_mode = sht->supported_mode;
+	shost->sense_buffer_isa = sht->sense_buffer_isa;
 
 	if (sht->max_host_blocked)
 		shost->max_host_blocked = sht->max_host_blocked;
drivers/scsi/scsi.c +6 -4
Index: linux/drivers/scsi/scsi.c
===================================================================
@@ -362,7 +362,7 @@
 	 */
 	mutex_lock(&host_cmd_pool_mutex);
 	pool = &scsi_cmd_pool;
-	if (shost && shost->unchecked_isa_dma)
+	if (shost && (shost->unchecked_isa_dma || shost->sense_buffer_isa))
 		pool = &scsi_cmd_dma_pool;
 	if (!pool->users) {
 		pool->cmd_slab = kmem_cache_create(pool->cmd_name,
@@ -392,8 +392,8 @@
 	struct scsi_host_cmd_pool *pool;
 
 	mutex_lock(&host_cmd_pool_mutex);
-	pool = (shost && shost->unchecked_isa_dma) ? &scsi_cmd_dma_pool :
-		&scsi_cmd_pool;
+	pool = (shost && (shost->unchecked_isa_dma || shost->sense_buffer_isa)) ?
+		&scsi_cmd_dma_pool : &scsi_cmd_pool;
 	/*
 	 * This may happen if a driver has a mismatched get and put
 	 * of the command pool; the driver should be implicated in
@@ -488,7 +488,9 @@
 int scsi_setup_command_freelist(struct Scsi_Host *shost)
 {
 	struct scsi_cmnd *cmd;
-	const gfp_t gfp_mask = shost->unchecked_isa_dma ? GFP_DMA : GFP_KERNEL;
+	gfp_t gfp_mask = GFP_KERNEL;
+	if (shost->unchecked_isa_dma || shost->sense_buffer_isa)
+		gfp_mask = GFP_DMA;
 
 	spin_lock_init(&shost->free_list_lock);
 	INIT_LIST_HEAD(&shost->free_list);
author
#10 lore raw

PATCH 5/21 Remove unchecked_isa in BusLogic

In reply to Andi Kleen

- ->cmnd handling audited and does always copy
- Allocate hostdata separately
- Enable sense_buffer isa bounce buffering
- Remove unchecked_isa_dma
- Enable block layer bouncing explicitely for isa adapters

Untested due to lack of hardware

---
 drivers/scsi/BusLogic.c |   77 ++++++++++++++++++++++++++++++++++++------------
 1 file changed, 59 insertions(+), 18 deletions(-)
drivers/scsi/BusLogic.c +59 -18
Index: linux/drivers/scsi/BusLogic.c
===================================================================
@@ -62,6 +62,13 @@
 
 static struct scsi_host_template Bus_Logic_template;
 
+struct host_ptr {
+	struct BusLogic_HostAdapter *host;
+	dma_addr_t dma;
+};
+#define bl_shost_priv(shost) (((struct host_ptr *)shost_priv(shost))->host)
+#define bl_shost_dma(shost) (((struct host_ptr *)shost_priv(shost))->dma)
+
 /*
   BusLogic_DriverOptionsCount is a count of the number of BusLogic Driver
   Options specifications provided via the Linux Kernel Command Line or via
@@ -124,6 +131,12 @@
 
 static struct BusLogic_ProbeInfo *BusLogic_ProbeInfoList;
 
+static int buslogic_adjust_queue(struct scsi_device *device)
+{
+	if (device->host->sense_buffer_isa)
+		blk_queue_bounce_limit(device->request_queue, BLK_BOUNCE_ISA);
+	return 0;
+}
 
 /*
   BusLogic_CommandFailureReason holds a string identifying the reason why a
@@ -152,7 +165,7 @@
 
 static const char *BusLogic_DriverInfo(struct Scsi_Host *Host)
 {
-	struct BusLogic_HostAdapter *HostAdapter = (struct BusLogic_HostAdapter *) Host->hostdata;
+	struct BusLogic_HostAdapter *HostAdapter = bl_shost_priv(Host);
 	return HostAdapter->FullModelName;
 }
 
@@ -1610,9 +1623,6 @@
 	   BIOS_Address is 0.
 	 */
 	HostAdapter->BIOS_Address = ExtendedSetupInformation.BIOS_Address << 12;
-	/*
-	   ISA Host Adapters require Bounce Buffers if there is more than 16MB memory.
-	 */
 	if (HostAdapter->HostAdapterBusType == BusLogic_ISA_Bus && (void *) high_memory > (void *) MAX_DMA_ADDRESS)
 		HostAdapter->BounceBuffersRequired = true;
 	/*
@@ -2131,7 +2141,9 @@
 	Host->this_id = HostAdapter->SCSI_ID;
 	Host->can_queue = HostAdapter->DriverQueueDepth;
 	Host->sg_tablesize = HostAdapter->DriverScatterGatherLimit;
-	Host->unchecked_isa_dma = HostAdapter->BounceBuffersRequired;
+	if (HostAdapter->BounceBuffersRequired)
+		Host->sense_buffer_isa = 1;
+
 	Host->cmd_per_lun = HostAdapter->UntaggedQueueDepth;
 }
 
@@ -2145,7 +2157,7 @@
 */
 static int BusLogic_SlaveConfigure(struct scsi_device *Device)
 {
-	struct BusLogic_HostAdapter *HostAdapter = (struct BusLogic_HostAdapter *) Device->host->hostdata;
+	struct BusLogic_HostAdapter *HostAdapter = bl_shost_priv(Device->host);
 	int TargetID = Device->id;
 	int QueueDepth = HostAdapter->QueueDepth[TargetID];
 
@@ -2170,6 +2182,35 @@
 	return 0;
 }
 
+static struct Scsi_Host *buslogic_host_alloc(gfp_t gfp)
+{
+	struct BusLogic_HostAdapter *board;
+	struct Scsi_Host *shost;
+
+
+	shost = scsi_host_alloc(&Bus_Logic_template, sizeof(struct host_ptr));
+	if (!shost)
+		return NULL;
+
+	board = dma_alloc_coherent(NULL, sizeof(struct BusLogic_HostAdapter),
+					&bl_shost_dma(shost), GFP_KERNEL);
+	if (!board) {
+		scsi_host_put(shost);
+		return NULL;
+	}
+
+	bl_shost_priv(shost) = board;
+
+	return shost;
+}
+
+static void buslogic_free_host(struct Scsi_Host *shost)
+{
+	dma_free_coherent(NULL, sizeof(struct BusLogic_HostAdapter),
+			  bl_shost_priv(shost), bl_shost_dma(shost));
+	scsi_host_put(shost);
+}
+
 /*
   BusLogic_DetectHostAdapter probes for BusLogic Host Adapters at the standard
   I/O Addresses where they may be located, initializing, registering, and
@@ -2270,12 +2311,12 @@
 		   Register the SCSI Host structure.
 		 */
 
-		Host = scsi_host_alloc(&Bus_Logic_template, sizeof(struct BusLogic_HostAdapter));
+		Host = buslogic_host_alloc(PrototypeHostAdapter->BounceBuffersRequired ? GFP_DMA : 0);
 		if (Host == NULL) {
 			release_region(HostAdapter->IO_Address, HostAdapter->AddressCount);
 			continue;
 		}
-		HostAdapter = (struct BusLogic_HostAdapter *) Host->hostdata;
+		HostAdapter = bl_shost_priv(Host);
 		memcpy(HostAdapter, PrototypeHostAdapter, sizeof(struct BusLogic_HostAdapter));
 		HostAdapter->SCSI_Host = Host;
 		HostAdapter->HostNumber = Host->host_no;
@@ -2321,7 +2362,7 @@
 				BusLogic_DestroyCCBs(HostAdapter);
 				BusLogic_ReleaseResources(HostAdapter);
 				list_del(&HostAdapter->host_list);
-				scsi_host_put(Host);
+				buslogic_free_host(Host);
 				ret = -ENOMEM;
 			} else {
 				BusLogic_InitializeHostStructure(HostAdapter,
@@ -2335,7 +2376,7 @@
 					BusLogic_DestroyCCBs(HostAdapter);
 					BusLogic_ReleaseResources(HostAdapter);
 					list_del(&HostAdapter->host_list);
-					scsi_host_put(Host);
+					buslogic_free_host(Host);
 					ret = -ENODEV;
 				} else {
 					scsi_scan_host(Host);
@@ -2354,7 +2395,7 @@
 			BusLogic_DestroyCCBs(HostAdapter);
 			BusLogic_ReleaseResources(HostAdapter);
 			list_del(&HostAdapter->host_list);
-			scsi_host_put(Host);
+			buslogic_free_host(Host);
 			ret = -ENODEV;
 		}
 	}
@@ -2398,7 +2439,7 @@
 	 */
 	list_del(&HostAdapter->host_list);
 
-	scsi_host_put(Host);
+	buslogic_free_host(Host);
 	return 0;
 }
 
@@ -2786,7 +2827,7 @@
 
 static int BusLogic_host_reset(struct scsi_cmnd * SCpnt)
 {
-	struct BusLogic_HostAdapter *HostAdapter = (struct BusLogic_HostAdapter *) SCpnt->device->host->hostdata;
+	struct BusLogic_HostAdapter *HostAdapter = bl_shost_priv(SCpnt->device->host);
 
 	unsigned int id = SCpnt->device->id;
 	struct BusLogic_TargetStatistics *stats = &HostAdapter->TargetStatistics[id];
@@ -2808,7 +2849,7 @@
 
 static int BusLogic_QueueCommand(struct scsi_cmnd *Command, void (*CompletionRoutine) (struct scsi_cmnd *))
 {
-	struct BusLogic_HostAdapter *HostAdapter = (struct BusLogic_HostAdapter *) Command->device->host->hostdata;
+	struct BusLogic_HostAdapter *HostAdapter = bl_shost_priv(Command->device->host);
 	struct BusLogic_TargetFlags *TargetFlags = &HostAdapter->TargetFlags[Command->device->id];
 	struct BusLogic_TargetStatistics *TargetStatistics = HostAdapter->TargetStatistics;
 	unsigned char *CDB = Command->cmnd;
@@ -3001,7 +3042,7 @@
 
 static int BusLogic_AbortCommand(struct scsi_cmnd *Command)
 {
-	struct BusLogic_HostAdapter *HostAdapter = (struct BusLogic_HostAdapter *) Command->device->host->hostdata;
+	struct BusLogic_HostAdapter *HostAdapter = bl_shost_priv(Command->device->host);
 
 	int TargetID = Command->device->id;
 	struct BusLogic_CCB *CCB;
@@ -3131,7 +3172,7 @@
 
 static int BusLogic_BIOSDiskParameters(struct scsi_device *sdev, struct block_device *Device, sector_t capacity, int *Parameters)
 {
-	struct BusLogic_HostAdapter *HostAdapter = (struct BusLogic_HostAdapter *) sdev->host->hostdata;
+	struct BusLogic_HostAdapter *HostAdapter = bl_shost_priv(sdev->host);
 	struct BIOS_DiskParameters *DiskParameters = (struct BIOS_DiskParameters *) Parameters;
 	unsigned char *buf;
 	if (HostAdapter->ExtendedTranslationEnabled && capacity >= 2 * 1024 * 1024 /* 1 GB in 512 byte sectors */ ) {
@@ -3202,7 +3243,7 @@
 
 static int BusLogic_ProcDirectoryInfo(struct Scsi_Host *shost, char *ProcBuffer, char **StartPointer, off_t Offset, int BytesAvailable, int WriteFlag)
 {
-	struct BusLogic_HostAdapter *HostAdapter = (struct BusLogic_HostAdapter *) shost->hostdata;
+	struct BusLogic_HostAdapter *HostAdapter = bl_shost_priv(shost);
 	struct BusLogic_TargetStatistics *TargetStatistics;
 	int TargetID, Length;
 	char *Buffer;
@@ -3575,9 +3616,9 @@
 #if 0
 	.eh_abort_handler = BusLogic_AbortCommand,
 #endif
-	.unchecked_isa_dma = 1,
 	.max_sectors = 128,
 	.use_clustering = ENABLE_CLUSTERING,
+	.slave_alloc = buslogic_adjust_queue,
 };
 
 /*
maintainer
#11 lore raw

Re: [PATCH] [5/21] Remove unchecked_isa in BusLogic

Quoted 6 lines · Andi Kleen
drivers/scsi/BusLogic.c +6 -0
+static struct Scsi_Host *buslogic_host_alloc(gfp_t gfp)
+{
+       struct BusLogic_HostAdapter *board;
+       struct Scsi_Host *shost;
+
+

drivers/scsi/BusLogic.c line 2200

Quoted 10 lines
drivers/scsi/BusLogic.c +10 -0
+       shost = scsi_host_alloc(&Bus_Logic_template, sizeof(struct host_ptr));
+       if (!shost)
+               return NULL;
+
+       board = dma_alloc_coherent(NULL, sizeof(struct BusLogic_HostAdapter),
+                                       &bl_shost_dma(shost), GFP_KERNEL);
+       if (!board) {
+               scsi_host_put(shost);
+               return NULL;
+       }

What makes you think that BusLogic_HostAdapter has to be in coherent memory? You can tell from your own patch that nothing ever uses its DMA address, so the card never actually DMAs data to or from this area.

James

author
#12 lore raw

Re: [PATCH] [5/21] Remove unchecked_isa in BusLogic

Quoted 13 lines · James Bottomley
Quoted 12 lines · Andi Kleen
drivers/scsi/BusLogic.c +12 -0
+static struct Scsi_Host *buslogic_host_alloc(gfp_t gfp)
+{
+       struct BusLogic_HostAdapter *board;
+       struct Scsi_Host *shost;
+
+
+       shost = scsi_host_alloc(&Bus_Logic_template, sizeof(struct host_ptr));
+       if (!shost)
+               return NULL;
+
+       board = dma_alloc_coherent(NULL, sizeof(struct BusLogic_HostAdapter),
+                                       &bl_shost_dma(shost), GFP_KERNEL);
Quoted 8 lines

drivers/scsi/BusLogic.c line 2200

Quoted 4 lines
drivers/scsi/BusLogic.c +4 -0
+       if (!board) {
+               scsi_host_put(shost);
+               return NULL;
+       }

What makes you think that BusLogic_HostAdapter has to be in coherent memory? You can tell from your own patch that nothing ever uses its DMA address, so the card never actually DMAs data to or from this area.

Hmm I think it was mostly about having an equivalent transformation. The structure was in GFP_DMA before, so I wanted to keep that

Also the driver does some funny things with transforming some bus addresses on its own, which made me double conservative. On looking more closely it doesn't do virt_to_bus, but at least one bus_to_virt (but it seems to data outside BusAdapter)

I don't have that hardware so I wanted to be very conservative in any

But I'll fix it in the next version.

-Andi

maintainer
#13 lore raw

Re: [PATCH] [5/21] Remove unchecked_isa in BusLogic

Quoted 31 lines · Andi Kleen
Quoted 13 lines · James Bottomley
Quoted 12 lines · Andi Kleen
drivers/scsi/BusLogic.c +12 -0
+static struct Scsi_Host *buslogic_host_alloc(gfp_t gfp)
+{
+       struct BusLogic_HostAdapter *board;
+       struct Scsi_Host *shost;
+
+
+       shost = scsi_host_alloc(&Bus_Logic_template, sizeof(struct host_ptr));
+       if (!shost)
+               return NULL;
+
+       board = dma_alloc_coherent(NULL, sizeof(struct BusLogic_HostAdapter),
+                                       &bl_shost_dma(shost), GFP_KERNEL);
Quoted 8 lines

drivers/scsi/BusLogic.c line 2200

Quoted 4 lines
drivers/scsi/BusLogic.c +4 -0
+       if (!board) {
+               scsi_host_put(shost);
+               return NULL;
+       }

What makes you think that BusLogic_HostAdapter has to be in coherent memory? You can tell from your own patch that nothing ever uses its DMA address, so the card never actually DMAs data to or from this area.

Hmm I think it was mostly about having an equivalent transformation. The structure was in GFP_DMA before, so I wanted to keep that

Also the driver does some funny things with transforming some bus addresses on its own, which made me double conservative. On looking more closely it doesn't do virt_to_bus, but at least one bus_to_virt (but it seems to data outside BusAdapter)

Quoted 4 lines

I don't have that hardware so I wanted to be very conservative in any

But I'll fix it in the next version.

Yes, the problem is that on several architectures dma_alloc_coherent() uses a special vaddr to get coherent memory. Unless you use it by the returned handle (which I've checked the patch doesn't do) the use is likely invalid. In particular, you can't call dma_map_... on it because usually that sets up a dual mapping (one cached and one uncached most likely) which is illegal on most CPUs (even x86, I believe).

James

author
#14 lore raw

PATCH 6/21 Remove unchecked_isa_dma in advansys.c

In reply to Andi Kleen

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.

- allocate hostdata with GFP_DMA separately for the ISA case
- Tell block layer explicitely to bounce for ISA case
- remove unchecked_isa_dma

Untested due to lack of hardware

v2: use dma api for all allocations
v3: remove cmnd buffer bouncing (thanks m.willcox), move sense bouncing
to mid layer

---
 drivers/scsi/advansys.c |  162 ++++++++++++++++++++++++++++++------------------
 1 file changed, 103 insertions(+), 59 deletions(-)
drivers/scsi/advansys.c +103 -59
Index: linux/drivers/scsi/advansys.c
===================================================================
@@ -2212,7 +2212,7 @@
 #define ASC_STATS_ADD(shost, counter, count)
 #else /* ADVANSYS_STATS */
 #define ASC_STATS_ADD(shost, counter, count) \
-	(((struct asc_board *) shost_priv(shost))->asc_stats.counter += (count))
+	(asc_shost_priv(shost)->asc_stats.counter += (count))
 #endif /* ADVANSYS_STATS */
 
 /* If the result wraps when calculating tenths, return 0. */
@@ -2354,10 +2354,6 @@
 
 /*
  * Structure allocated for each board.
- *
- * This structure is allocated by scsi_host_alloc() at the end
- * of the 'Scsi_Host' structure starting at the 'hostdata'
- * field. It is guaranteed to be allocated from DMA-able memory.
  */
 struct asc_board {
 	struct device *dev;
@@ -2388,6 +2384,7 @@
 #ifdef ADVANSYS_STATS
 	struct asc_stats asc_stats;	/* Board statistics */
 #endif				/* ADVANSYS_STATS */
+
 	/*
 	 * The following fields are used only for Narrow Boards.
 	 */
@@ -2403,8 +2400,19 @@
 	ushort bios_version;	/* BIOS Version. */
 	ushort bios_codeseg;	/* BIOS Code Segment. */
 	ushort bios_codelen;	/* BIOS Code Segment Length. */
+
+};
+
+struct asc_board_ptr {
+	struct asc_board *b;
+	dma_addr_t dma;
 };
 
+
+
+#define asc_shost_priv(h) (((struct asc_board_ptr *)shost_priv(h))->b)
+#define asc_shost_dma(h) (((struct asc_board_ptr *)shost_priv(h))->dma)
+
 #define asc_dvc_to_board(asc_dvc) container_of(asc_dvc, struct asc_board, \
 							dvc_var.asc_dvc_var)
 #define adv_dvc_to_board(adv_dvc) container_of(adv_dvc, struct asc_board, \
@@ -2525,7 +2533,7 @@
  */
 static void asc_prt_scsi_host(struct Scsi_Host *s)
 {
-	struct asc_board *boardp = shost_priv(s);
+	struct asc_board *boardp = asc_shost_priv(s);
 
 	printk("Scsi_Host at addr 0x%p, device %s\n", s, boardp->dev->bus_id);
 	printk(" host_busy %u, host_no %d, last_reset %d,\n",
@@ -2537,8 +2545,8 @@
 	printk(" dma_channel %d, this_id %d, can_queue %d,\n",
 	       s->dma_channel, s->this_id, s->can_queue);
 
-	printk(" cmd_per_lun %d, sg_tablesize %d, unchecked_isa_dma %d\n",
-	       s->cmd_per_lun, s->sg_tablesize, s->unchecked_isa_dma);
+	printk(" cmd_per_lun %d, sg_tablesize %d\n",
+	       s->cmd_per_lun, s->sg_tablesize);
 
 	if (ASC_NARROW_BOARD(boardp)) {
 		asc_prt_asc_dvc_var(&boardp->dvc_var.asc_dvc_var);
@@ -2803,7 +2811,7 @@
 static const char *advansys_info(struct Scsi_Host *shost)
 {
 	static char info[ASC_INFO_SIZE];
-	struct asc_board *boardp = shost_priv(shost);
+	struct asc_board *boardp = asc_shost_priv(shost);
 	ASC_DVC_VAR *asc_dvc_varp;
 	ADV_DVC_VAR *adv_dvc_varp;
 	char *busname;
@@ -2919,7 +2927,7 @@
  */
 static int asc_prt_board_devices(struct Scsi_Host *shost, char *cp, int cplen)
 {
-	struct asc_board *boardp = shost_priv(shost);
+	struct asc_board *boardp = asc_shost_priv(shost);
 	int leftlen;
 	int totlen;
 	int len;
@@ -2959,7 +2967,7 @@
  */
 static int asc_prt_adv_bios(struct Scsi_Host *shost, char *cp, int cplen)
 {
-	struct asc_board *boardp = shost_priv(shost);
+	struct asc_board *boardp = asc_shost_priv(shost);
 	int leftlen;
 	int totlen;
 	int len;
@@ -3124,7 +3132,7 @@
  */
 static int asc_prt_asc_board_eeprom(struct Scsi_Host *shost, char *cp, int cplen)
 {
-	struct asc_board *boardp = shost_priv(shost);
+	struct asc_board *boardp = asc_shost_priv(shost);
 	ASC_DVC_VAR *asc_dvc_varp;
 	int leftlen;
 	int totlen;
@@ -3257,7 +3265,7 @@
  */
 static int asc_prt_adv_board_eeprom(struct Scsi_Host *shost, char *cp, int cplen)
 {
-	struct asc_board *boardp = shost_priv(shost);
+	struct asc_board *boardp = asc_shost_priv(shost);
 	ADV_DVC_VAR *adv_dvc_varp;
 	int leftlen;
 	int totlen;
@@ -3543,7 +3551,7 @@
  */
 static int asc_prt_driver_conf(struct Scsi_Host *shost, char *cp, int cplen)
 {
-	struct asc_board *boardp = shost_priv(shost);
+	struct asc_board *boardp = asc_shost_priv(shost);
 	int leftlen;
 	int totlen;
 	int len;
@@ -3569,9 +3577,7 @@
 			   shost->sg_tablesize, shost->cmd_per_lun);
 	ASC_PRT_NEXT();
 
-	len = asc_prt_line(cp, leftlen,
-			   " unchecked_isa_dma %d, use_clustering %d\n",
-			   shost->unchecked_isa_dma, shost->use_clustering);
+	len = asc_prt_line(cp, leftlen, " use_clustering %d\n", shost->use_clustering);
 	ASC_PRT_NEXT();
 
 	len = asc_prt_line(cp, leftlen,
@@ -3605,7 +3611,7 @@
  */
 static int asc_prt_asc_board_info(struct Scsi_Host *shost, char *cp, int cplen)
 {
-	struct asc_board *boardp = shost_priv(shost);
+	struct asc_board *boardp = asc_shost_priv(shost);
 	int chip_scsi_id;
 	int leftlen;
 	int totlen;
@@ -3787,7 +3793,7 @@
  */
 static int asc_prt_adv_board_info(struct Scsi_Host *shost, char *cp, int cplen)
 {
-	struct asc_board *boardp = shost_priv(shost);
+	struct asc_board *boardp = asc_shost_priv(shost);
 	int leftlen;
 	int totlen;
 	int len;
@@ -4065,7 +4071,7 @@
  */
 static int asc_prt_board_stats(struct Scsi_Host *shost, char *cp, int cplen)
 {
-	struct asc_board *boardp = shost_priv(shost);
+	struct asc_board *boardp = asc_shost_priv(shost);
 	struct asc_stats *s = &boardp->asc_stats;
 
 	int leftlen = cplen;
@@ -4151,7 +4157,7 @@
 advansys_proc_info(struct Scsi_Host *shost, char *buffer, char **start,
 		   off_t offset, int length, int inout)
 {
-	struct asc_board *boardp = shost_priv(shost);
+	struct asc_board *boardp = asc_shost_priv(shost);
 	char *cp;
 	int cplen;
 	int cnt;
@@ -8200,7 +8206,7 @@
 	ASC_STATS(shost, callback);
 	ASC_DBG(1, "shost 0x%p\n", shost);
 
-	boardp = shost_priv(shost);
+	boardp = asc_shost_priv(shost);
 	BUG_ON(adv_dvc_varp != &boardp->dvc_var.adv_dvc_var);
 
 	/*
@@ -9132,7 +9138,7 @@
 	ASC_STATS(shost, callback);
 	ASC_DBG(1, "shost 0x%p\n", shost);
 
-	boardp = shost_priv(shost);
+	boardp = asc_shost_priv(shost);
 	BUG_ON(asc_dvc_varp != &boardp->dvc_var.asc_dvc_var);
 
 	dma_unmap_single(boardp->dev, scp->SCp.dma_handle,
@@ -9484,7 +9490,7 @@
 static int advansys_reset(struct scsi_cmnd *scp)
 {
 	struct Scsi_Host *shost = scp->device->host;
-	struct asc_board *boardp = shost_priv(shost);
+	struct asc_board *boardp = asc_shost_priv(shost);
 	unsigned long flags;
 	int status;
 	int ret = SUCCESS;
@@ -9567,7 +9573,7 @@
 advansys_biosparam(struct scsi_device *sdev, struct block_device *bdev,
 		   sector_t capacity, int ip[])
 {
-	struct asc_board *boardp = shost_priv(sdev->host);
+	struct asc_board *boardp = asc_shost_priv(sdev->host);
 
 	ASC_DBG(1, "begin\n");
 	ASC_STATS(sdev->host, biosparam);
@@ -9603,7 +9609,7 @@
 static irqreturn_t advansys_interrupt(int irq, void *dev_id)
 {
 	struct Scsi_Host *shost = dev_id;
-	struct asc_board *boardp = shost_priv(shost);
+	struct asc_board *boardp = asc_shost_priv(shost);
 	irqreturn_t result = IRQ_NONE;
 
 	ASC_DBG(2, "boardp 0x%p\n", boardp);
@@ -9691,6 +9697,9 @@
 	ASC_SCSI_BIT_ID_TYPE tid_bit = 1 << sdev->id;
 	ASC_SCSI_BIT_ID_TYPE orig_use_tagged_qng = asc_dvc->use_tagged_qng;
 
+	if (sdev->host->sense_buffer_isa)
+		blk_queue_bounce_limit(sdev->request_queue, BLK_BOUNCE_ISA);
+
 	if (sdev->lun == 0) {
 		ASC_SCSI_BIT_ID_TYPE orig_init_sdtr = asc_dvc->init_sdtr;
 		if ((asc_dvc->cfg->sdtr_enable & tid_bit) && sdev->sdtr) {
@@ -9865,7 +9874,7 @@
  */
 static int advansys_slave_configure(struct scsi_device *sdev)
 {
-	struct asc_board *boardp = shost_priv(sdev->host);
+	struct asc_board *boardp = asc_shost_priv(sdev->host);
 
 	if (ASC_NARROW_BOARD(boardp))
 		advansys_narrow_slave_configure(sdev,
@@ -10925,7 +10934,7 @@
 static int asc_execute_scsi_cmnd(struct scsi_cmnd *scp)
 {
 	int ret, err_code;
-	struct asc_board *boardp = shost_priv(scp->device->host);
+	struct asc_board *boardp = asc_shost_priv(scp->device->host);
 
 	ASC_DBG(1, "scp 0x%p\n", scp);
 
@@ -11725,7 +11734,7 @@
 
 static int __devinit AscInitGetConfig(struct Scsi_Host *shost)
 {
-	struct asc_board *board = shost_priv(shost);
+	struct asc_board *board = asc_shost_priv(shost);
 	ASC_DVC_VAR *asc_dvc = &board->dvc_var.asc_dvc_var;
 	unsigned short warn_code = 0;
 
@@ -11779,7 +11788,7 @@
 
 static int __devinit AscInitSetConfig(struct pci_dev *pdev, struct Scsi_Host *shost)
 {
-	struct asc_board *board = shost_priv(shost);
+	struct asc_board *board = asc_shost_priv(shost);
 	ASC_DVC_VAR *asc_dvc = &board->dvc_var.asc_dvc_var;
 	PortAddr iop_base = asc_dvc->iop_base;
 	unsigned short cfg_msw;
@@ -13172,7 +13181,7 @@
 static int __devinit
 AdvInitGetConfig(struct pci_dev *pdev, struct Scsi_Host *shost)
 {
-	struct asc_board *board = shost_priv(shost);
+	struct asc_board *board = asc_shost_priv(shost);
 	ADV_DVC_VAR *asc_dvc = &board->dvc_var.adv_dvc_var;
 	unsigned short warn_code = 0;
 	AdvPortAddr iop_base = asc_dvc->iop_base;
@@ -13250,6 +13259,13 @@
 }
 #endif
 
+static int advansys_adjust_queue(struct scsi_device *device)
+{
+	if (device->host->sense_buffer_isa)
+		blk_queue_bounce_limit(device->request_queue, BLK_BOUNCE_ISA);
+	return 0;
+}
+
 static struct scsi_host_template advansys_template = {
 	.proc_name = DRV_NAME,
 #ifdef CONFIG_PROC_FS
@@ -13261,12 +13277,7 @@
 	.eh_bus_reset_handler = advansys_reset,
 	.bios_param = advansys_biosparam,
 	.slave_configure = advansys_slave_configure,
-	/*
-	 * Because the driver may control an ISA adapter 'unchecked_isa_dma'
-	 * must be set. The flag will be cleared in advansys_board_found
-	 * for non-ISA adapters.
-	 */
-	.unchecked_isa_dma = 1,
+	.slave_alloc = advansys_adjust_queue,
 	/*
 	 * All adapters controlled by this driver are capable of large
 	 * scatter-gather lists. According to the mid-level SCSI documentation
@@ -13279,7 +13290,7 @@
 
 static int __devinit advansys_wide_init_chip(struct Scsi_Host *shost)
 {
-	struct asc_board *board = shost_priv(shost);
+	struct asc_board *board = asc_shost_priv(shost);
 	struct adv_dvc_var *adv_dvc = &board->dvc_var.adv_dvc_var;
 	int req_cnt = 0;
 	adv_req_t *reqp = NULL;
@@ -13394,7 +13405,7 @@
 					  unsigned int iop, int bus_type)
 {
 	struct pci_dev *pdev;
-	struct asc_board *boardp = shost_priv(shost);
+	struct asc_board *boardp = asc_shost_priv(shost);
 	ASC_DVC_VAR *asc_dvc_varp = NULL;
 	ADV_DVC_VAR *adv_dvc_varp = NULL;
 	int share_irq, warn_code, ret;
@@ -13464,6 +13475,8 @@
 	}
 #endif /* CONFIG_PROC_FS */
 
+	ret = 0;
+
 	if (ASC_NARROW_BOARD(boardp)) {
 		/*
 		 * Set the board bus type and PCI IRQ before
@@ -13472,28 +13485,23 @@
 		switch (asc_dvc_varp->bus_type) {
 #ifdef CONFIG_ISA
 		case ASC_IS_ISA:
-			shost->unchecked_isa_dma = TRUE;
 			share_irq = 0;
 			break;
 		case ASC_IS_VL:
-			shost->unchecked_isa_dma = FALSE;
 			share_irq = 0;
 			break;
 		case ASC_IS_EISA:
-			shost->unchecked_isa_dma = FALSE;
 			share_irq = IRQF_SHARED;
 			break;
 #endif /* CONFIG_ISA */
 #ifdef CONFIG_PCI
 		case ASC_IS_PCI:
-			shost->unchecked_isa_dma = FALSE;
 			share_irq = IRQF_SHARED;
 			break;
 #endif /* CONFIG_PCI */
 		default:
 			shost_printk(KERN_ERR, shost, "unknown adapter type: "
 					"%d\n", asc_dvc_varp->bus_type);
-			shost->unchecked_isa_dma = TRUE;
 			share_irq = 0;
 			break;
 		}
@@ -13505,14 +13513,14 @@
 		 * referenced only use the bit-wise AND operator "&".
 		 */
 		ASC_DBG(2, "AscInitGetConfig()\n");
-		ret = AscInitGetConfig(shost) ? -ENODEV : 0;
+		if (!ret)
+			ret = AscInitGetConfig(shost) ? -ENODEV : 0;
 	} else {
 #ifdef CONFIG_PCI
 		/*
 		 * For Wide boards set PCI information before calling
 		 * AdvInitGetConfig().
 		 */
-		shost->unchecked_isa_dma = FALSE;
 		share_irq = IRQF_SHARED;
 		ASC_DBG(2, "AdvInitGetConfig()\n");
 
@@ -13883,6 +13891,16 @@
 	return ret;
 }
 
+static void adv_free_host(struct Scsi_Host *shost)
+{
+	struct asc_board *board = asc_shost_priv(shost);
+	struct device *device = board->dev;
+	dma_free_coherent(device, sizeof(struct asc_board),
+				  asc_shost_priv(shost),
+				  asc_shost_dma(shost));
+	scsi_host_put(shost);
+}
+
 /*
  * advansys_release()
  *
@@ -13890,7 +13908,7 @@
  */
 static int advansys_release(struct Scsi_Host *shost)
 {
-	struct asc_board *board = shost_priv(shost);
+	struct asc_board *board = asc_shost_priv(shost);
 	ASC_DBG(1, "begin\n");
 	scsi_remove_host(shost);
 	free_irq(board->irq, shost);
@@ -13908,11 +13926,32 @@
 		advansys_wide_free_mem(board);
 	}
 	kfree(board->prtbuf);
-	scsi_host_put(shost);
+	adv_free_host(shost);
 	ASC_DBG(1, "end\n");
 	return 0;
 }
 
+static struct Scsi_Host *adv_host_alloc(struct device *dev)
+{
+	struct asc_board *board;
+	struct Scsi_Host *shost;
+	shost = scsi_host_alloc(&advansys_template,
+				sizeof(struct asc_board_ptr));
+	if (!shost)
+		return NULL;
+
+	board = dma_alloc_coherent(dev,
+				   sizeof(struct asc_board),
+				   &asc_shost_dma(shost),
+				   GFP_KERNEL);
+	if (!board) {
+		scsi_host_put(shost);
+		return NULL;
+	}
+	asc_shost_priv(shost) = board;
+	return shost;
+}
+
 #define ASC_IOADR_TABLE_MAX_IX  11
 
 static PortAddr _asc_def_iop_base[ASC_IOADR_TABLE_MAX_IX] = {
@@ -13954,11 +13993,13 @@
 		goto release_region;
 
 	err = -ENOMEM;
-	shost = scsi_host_alloc(&advansys_template, sizeof(*board));
+	shost = adv_host_alloc(NULL);
 	if (!shost)
 		goto release_region;
+	shost->sense_buffer_isa = 1;
+
+	board = asc_shost_priv(shost);
 
-	board = shost_priv(shost);
 	board->irq = advansys_isa_irq_no(iop_base);
 	board->dev = dev;
 
@@ -13970,7 +14011,7 @@
 	return 0;
 
  free_host:
-	scsi_host_put(shost);
+	adv_free_host(shost);
  release_region:
 	release_region(iop_base, ASC_IOADR_GAP);
 	return err;
@@ -14036,11 +14077,12 @@
 		goto release_region;
 
 	err = -ENOMEM;
-	shost = scsi_host_alloc(&advansys_template, sizeof(*board));
+	shost = adv_host_alloc(NULL);
 	if (!shost)
 		goto release_region;
+	shost->sense_buffer_isa = 1;
 
-	board = shost_priv(shost);
+	board = asc_shost_priv(shost);
 	board->irq = advansys_vlb_irq_no(iop_base);
 	board->dev = dev;
 
@@ -14052,7 +14094,7 @@
 	return 0;
 
  free_host:
-	scsi_host_put(shost);
+	adv_free_host(shost);
  release_region:
 	release_region(iop_base, ASC_IOADR_GAP);
 	return -ENODEV;
@@ -14143,11 +14185,13 @@
 			irq = advansys_eisa_irq_no(edev);
 
 		err = -ENOMEM;
-		shost = scsi_host_alloc(&advansys_template, sizeof(*board));
+		/* RED-PEN does use GFP_DMA unnecessarily since EISA is 32bit */
+		shost = adv_host_alloc(NULL);
+
 		if (!shost)
 			goto release_region;
 
-		board = shost_priv(shost);
+		board = asc_shost_priv(shost);
 		board->irq = irq;
 		board->dev = dev;
 
@@ -14259,11 +14303,11 @@
 	ioport = pci_resource_start(pdev, 0);
 
 	err = -ENOMEM;
-	shost = scsi_host_alloc(&advansys_template, sizeof(*board));
+	shost = adv_host_alloc(&pdev->dev);
 	if (!shost)
 		goto release_region;
 
-	board = shost_priv(shost);
+	board = asc_shost_priv(shost);
 	board->irq = pdev->irq;
 	board->dev = &pdev->dev;
 
@@ -14281,7 +14325,7 @@
 	return 0;
 
  free_host:
-	scsi_host_put(shost);
+	adv_free_host(shost);
  release_region:
 	pci_release_regions(pdev);
  disable_device:
maintainer
#15 lore raw

Re: [PATCH] [6/21] Remove unchecked_isa_dma in advansys.c

Quoted 6 lines · Andi Kleen

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.

Quoted 7 lines

- allocate hostdata with GFP_DMA separately for the ISA case
- Tell block layer explicitely to bounce for ISA case
- remove unchecked_isa_dma

Untested due to lack of hardware

v2: use dma api for all allocations

Just a minute: this is another driver where you allocate coherent memory for the host structure but fail to use it for I/O as far as I can tell.

What's going on here? If this is just a ploy to get GFP_DMA memory, then this isn't the way to do it. Firstly, it doesn't seem to be needed and secondly, coherent memory is hard (or impossible) to manufacture on certain platforms. You should only use it for DMA mailboxes and other register exchanges between the device and main memory ... expanding its use is asking for resource exhaustion.

James

author
#16 lore raw

Re: [PATCH] [6/21] Remove unchecked_isa_dma in advansys.c

Quoted 13 lines · James Bottomley
Quoted 12 lines · Andi Kleen

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.

- allocate hostdata with GFP_DMA separately for the ISA case
- Tell block layer explicitely to bounce for ISA case
- remove unchecked_isa_dma

Untested due to lack of hardware

Quoted 8 lines
Quoted 1 line

v2: use dma api for all allocations

Just a minute: this is another driver where you allocate coherent memory for the host structure but fail to use it for I/O as far as I can tell.

What's going on here? If this is just a ploy to get GFP_DMA memory, then this isn't the way to do it. Firstly, it doesn't seem to be needed

It's just that the structure was in GFP_DMA before due to unchecked_isa_dma so I wanted to do a equivalent convertion

Quoted 4 lines

and secondly, coherent memory is hard (or impossible) to manufacture on certain platforms. You should only use it for DMA mailboxes and other register exchanges between the device and main memory ... expanding its use is asking for resource exhaustion.

It's not expanding it, in fact it lowers it because they allocate less in GFP_DMA now than with unchecked_isa_dma (e.g. no more commands and sense buffers)

-Andi

Signature · 1 line
[email protected]
maintainer
#17 lore raw

Re: [PATCH] [6/21] Remove unchecked_isa_dma in advansys.c

Quoted 13 lines · Andi Kleen
Quoted 8 lines · James Bottomley
Quoted 1 line

v2: use dma api for all allocations

Just a minute: this is another driver where you allocate coherent memory for the host structure but fail to use it for I/O as far as I can tell.

What's going on here? If this is just a ploy to get GFP_DMA memory, then this isn't the way to do it. Firstly, it doesn't seem to be needed

It's just that the structure was in GFP_DMA before due to unchecked_isa_dma so I wanted to do a equivalent convertion

Quoted 8 lines
Quoted 4 lines

and secondly, coherent memory is hard (or impossible) to manufacture on certain platforms. You should only use it for DMA mailboxes and other register exchanges between the device and main memory ... expanding its use is asking for resource exhaustion.

It's not expanding it, in fact it lowers it because they allocate less in GFP_DMA now than with unchecked_isa_dma (e.g. no more commands and sense buffers)

but dma_alloc_coherent() isn't equivalent to an allocator with the dma_mask on most platforms. It is on x86 because that platform is fully coherent, but on most others it's taking memory from a special coherent pool. This is the resource which can exhaust easily.

James

author
#18 lore raw

PATCH 7/21 Remove unchecked_isa_dma in gdth

In reply to Andi Kleen

- Audited ->cmnd use and it always copies
- Allocate hostdata separately with GFP_DMA for the ISA case
- Tell scsi layer to bounce sense_buffer for ISA case
- Tell block layer to bounce for isa case
- Remove unchecked_isa_dma

Untested due to lack of hardware

---
 drivers/scsi/gdth.c |   81 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 62 insertions(+), 19 deletions(-)
drivers/scsi/gdth.c +62 -19
Index: linux/drivers/scsi/gdth.c
===================================================================
@@ -139,6 +139,15 @@
 #include <scsi/scsi_host.h>
 #include "gdth.h"
 
+
+struct host_ptr {
+	gdth_ha_str *host_ptr;
+	dma_addr_t dma;
+};
+
+#define gdth_shost_priv(host) (((struct host_ptr *)shost_priv(host))->host_ptr)
+#define gdth_shost_dma(host) (((struct host_ptr *)shost_priv(host))->dma)
+
 static void gdth_delay(int milliseconds);
 static void gdth_eval_mapping(ulong32 size, ulong32 *cyls, int *heads, int *secs);
 static irqreturn_t gdth_interrupt(int irq, void *dev_id);
@@ -443,7 +452,7 @@
 int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
                    int timeout, u32 *info)
 {
-    gdth_ha_str *ha = shost_priv(sdev->host);
+    gdth_ha_str *ha = gdth_shost_priv(sdev->host);
     Scsi_Cmnd *scp;
     struct gdth_cmndinfo cmndinfo;
     DECLARE_COMPLETION_ONSTACK(wait);
@@ -3881,7 +3890,7 @@
 
 static const char *gdth_info(struct Scsi_Host *shp)
 {
-    gdth_ha_str *ha = shost_priv(shp);
+    gdth_ha_str *ha = gdth_shost_priv(shp);
 
     TRACE2(("gdth_info()\n"));
     return ((const char *)ha->binfo.type_string);
@@ -3922,7 +3931,7 @@
 
 static int gdth_eh_bus_reset(Scsi_Cmnd *scp)
 {
-    gdth_ha_str *ha = shost_priv(scp->device->host);
+    gdth_ha_str *ha = gdth_shost_priv(scp->device->host);
     int i;
     ulong flags;
     Scsi_Cmnd *cmnd;
@@ -3975,7 +3984,7 @@
 static int gdth_bios_param(struct scsi_device *sdev,struct block_device *bdev,sector_t cap,int *ip)
 {
     unchar b, t;
-    gdth_ha_str *ha = shost_priv(sdev->host);
+    gdth_ha_str *ha = gdth_shost_priv(sdev->host);
     struct scsi_device *sd;
     unsigned capacity;
 
@@ -4004,7 +4013,7 @@
 static int gdth_queuecommand(struct scsi_cmnd *scp,
 				void (*done)(struct scsi_cmnd *))
 {
-    gdth_ha_str *ha = shost_priv(scp->device->host);
+    gdth_ha_str *ha = gdth_shost_priv(scp->device->host);
     struct gdth_cmndinfo *cmndinfo;
 
     TRACE(("gdth_queuecommand() cmd 0x%x\n", scp->cmnd[0]));
@@ -4651,6 +4660,13 @@
     return 0;
 }
 
+static int gdth_adjust_queue(struct scsi_device *device)
+{
+	if (device->host->sense_buffer_isa)
+		blk_queue_bounce_limit(device->request_queue, BLK_BOUNCE_ISA);
+	return 0;
+}
+
 static struct scsi_host_template gdth_template = {
         .name                   = "GDT SCSI Disk Array Controller",
         .info                   = gdth_info, 
@@ -4665,10 +4681,38 @@
         .this_id                = -1,
         .sg_tablesize           = GDTH_MAXSG,
         .cmd_per_lun            = GDTH_MAXC_P_L,
-        .unchecked_isa_dma      = 1,
         .use_clustering         = ENABLE_CLUSTERING,
+	.slave_alloc		= gdth_adjust_queue,
 };
 
+static struct Scsi_Host *gdth_host_alloc(struct device *dev)
+{
+	struct Scsi_Host *shost;
+	gdth_ha_str *board;
+	shost = scsi_host_alloc(&gdth_template, sizeof(struct host_ptr));
+	if (!shost)
+		return NULL;
+
+	board = dma_alloc_coherent(dev, sizeof(gdth_ha_str),
+				&gdth_shost_dma(shost), GFP_KERNEL);
+	if (!board) {
+		scsi_host_put(shost);
+		return NULL;
+	}
+
+	gdth_shost_priv(shost) = board;
+
+	return shost;
+}
+
+static void gdth_free_host(struct Scsi_Host *shost)
+{
+	gdth_ha_str *h = gdth_shost_priv(shost);
+	dma_free_coherent(h->pdev ? &h->pdev->dev : NULL, sizeof(gdth_ha_str),
+			  h, gdth_shost_dma(shost));
+	scsi_host_put(shost);
+}
+
 #ifdef CONFIG_ISA
 static int __init gdth_isa_probe_one(ulong32 isa_bios)
 {
@@ -4680,10 +4724,10 @@
 	if (!gdth_search_isa(isa_bios))
 		return -ENXIO;
 
-	shp = scsi_host_alloc(&gdth_template, sizeof(gdth_ha_str));
+	shp = gdth_host_alloc(NULL);
 	if (!shp)
 		return -ENOMEM;
-	ha = shost_priv(shp);
+	ha = gdth_shost_priv(shp);
 
 	error = -ENODEV;
 	if (!gdth_init_isa(isa_bios,ha))
@@ -4707,7 +4751,7 @@
 
 	set_dma_mode(ha->drq,DMA_MODE_CASCADE);
 	enable_dma(ha->drq);
-	shp->unchecked_isa_dma = 1;
+	shp->sense_buffer_isa = 1;
 	shp->irq = ha->irq;
 	shp->dma_channel = ha->drq;
 
@@ -4796,7 +4840,7 @@
  out_free_irq:
 	free_irq(ha->irq, ha);
  out_host_put:
-	scsi_host_put(shp);
+	gdth_free_host(shp);
 	return error;
 }
 #endif /* CONFIG_ISA */
@@ -4812,10 +4856,11 @@
 	if (!gdth_search_eisa(eisa_slot))
 		return -ENXIO;
 
-	shp = scsi_host_alloc(&gdth_template, sizeof(gdth_ha_str));
+	/* RED-PEN NULL device ISA mask is 24bit, but EISA 32bit */
+	shp = gdth_host_alloc(NULL);
 	if (!shp)
 		return -ENOMEM;
-	ha = shost_priv(shp);
+	ha = gdth_shost_priv(shp);
 
 	error = -ENODEV;
 	if (!gdth_init_eisa(eisa_slot,ha))
@@ -4831,7 +4876,6 @@
 		goto out_host_put;
 	}
 
-	shp->unchecked_isa_dma = 0;
 	shp->irq = ha->irq;
 	shp->dma_channel = 0xff;
 
@@ -4929,7 +4973,7 @@
 	free_irq(ha->irq, ha);
 	gdth_ctr_count--;
  out_host_put:
-	scsi_host_put(shp);
+	gdth_free_host(shp);
 	return error;
 }
 #endif /* CONFIG_EISA */
@@ -4946,10 +4990,10 @@
 
 	*ha_out = NULL;
 
-	shp = scsi_host_alloc(&gdth_template, sizeof(gdth_ha_str));
+	shp = gdth_host_alloc(&pcistr->pdev->dev);
 	if (!shp)
 		return -ENOMEM;
-	ha = shost_priv(shp);
+	ha = gdth_shost_priv(shp);
 
 	error = -ENODEV;
 	if (!gdth_init_pci(pdev, pcistr, ha))
@@ -4968,7 +5012,6 @@
 		goto out_host_put;
 	}
 
-	shp->unchecked_isa_dma = 0;
 	shp->irq = ha->irq;
 	shp->dma_channel = 0xff;
 
@@ -5075,7 +5118,7 @@
 	free_irq(ha->irq, ha);
 	gdth_ctr_count--;
  out_host_put:
-	scsi_host_put(shp);
+	gdth_free_host(shp);
 	return error;
 }
 #endif /* CONFIG_PCI */
@@ -5117,7 +5160,7 @@
 		pci_unmap_single(ha->pdev,ha->ccb_phys,
 			sizeof(gdth_cmd_str),PCI_DMA_BIDIRECTIONAL);
 
-	scsi_host_put(shp);
+	gdth_free_host(shp);
 }
 
 static int gdth_halt(struct notifier_block *nb, ulong event, void *buf)
author
#19 lore raw

PATCH 8/21 Remove unchecked_isa_dma in eata.c

In reply to Andi Kleen

- Allocate hostdata with DMA separately in the driver
- Audited ->cmnd uses and it only ever copies them
- Enable sense_buffer bouncing using new sense_buffer_isa flag
- Enable block layer bouncing explicitely
- Remove unchecked_isa_dma

Untested due to lack of hardware

---
 drivers/scsi/eata.c |   54 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 15 deletions(-)
drivers/scsi/eata.c +39 -15
Index: linux/drivers/scsi/eata.c
===================================================================
@@ -512,6 +512,13 @@
 			     sector_t, int *);
 static int eata2x_slave_configure(struct scsi_device *);
 
+static int eata_adjust_queue(struct scsi_device *device)
+{
+	if (device->host->sense_buffer_isa)
+		blk_queue_bounce_limit(device->request_queue, BLK_BOUNCE_ISA);
+	return 0;
+}
+
 static struct scsi_host_template driver_template = {
 	.name = "EATA/DMA 2.0x rev. 8.10.00 ",
 	.detect = eata2x_detect,
@@ -522,8 +529,8 @@
 	.bios_param = eata2x_bios_param,
 	.slave_configure = eata2x_slave_configure,
 	.this_id = 7,
-	.unchecked_isa_dma = 1,
 	.use_clustering = ENABLE_CLUSTERING,
+	.slave_alloc = eata_adjust_queue,
 };
 
 #if !defined(__BIG_ENDIAN_BITFIELD) && !defined(__LITTLE_ENDIAN_BITFIELD)
@@ -834,6 +841,16 @@
 	struct mssp sp;		/* Local copy of sp buffer */
 };
 
+struct hostdata_ptr {
+	struct hostdata *host;
+	dma_addr_t dma;
+};
+
+#define eata_shost_priv(shost) \
+	(((struct hostdata_ptr *)shost_priv(shost))->host)
+#define eata_shost_dma(shost) \
+	(((struct hostdata_ptr *)shost_priv(shost))->dma)
+
 static struct Scsi_Host *sh[MAX_BOARDS];
 static const char *driver_name = "EATA";
 static char sha[MAX_BOARDS];
@@ -1266,14 +1283,23 @@
 #endif
 
 	spin_unlock_irq(&driver_lock);
-	sh[j] = shost = scsi_register(tpnt, sizeof(struct hostdata));
+	sh[j] = shost = scsi_register(tpnt, sizeof(struct hostdata_ptr));
+	ha = dma_alloc_coherent(pdev ? &pdev->dev : NULL,
+				sizeof(struct hostdata),
+				&eata_shost_dma(shost), GFP_KERNEL);
 	spin_lock_irq(&driver_lock);
 
-	if (shost == NULL) {
+	if (shost == NULL || ha == NULL) {
+		if (ha)
+			dma_free_coherent(pdev ? &pdev->dev : NULL,
+					  sizeof(struct hostdata),
+					  ha, eata_shost_dma(shost));
 		printk("%s: unable to register host, detaching.\n", name);
 		goto freedma;
 	}
 
+	eata_shost_priv(shost) = ha;
+
 	shost->io_port = port_base;
 	shost->unique_id = port_base;
 	shost->n_io_port = REGION_SIZE;
@@ -1283,8 +1309,6 @@
 	shost->this_id = (ushort) info.host_addr[3];
 	shost->can_queue = (ushort) info.queue_size;
 	shost->cmd_per_lun = MAX_CMD_PER_LUN;
-
-	ha = (struct hostdata *)shost->hostdata;
 	
 	memset(ha, 0, sizeof(struct hostdata));
 	ha->subversion = subversion;
@@ -1293,11 +1317,9 @@
 	ha->pdev = pdev;
 	ha->board_number = j;
 
-	if (ha->subversion == ESA)
-		shost->unchecked_isa_dma = 0;
-	else {
+	if (ha->subversion != ESA) {
 		unsigned long flags;
-		shost->unchecked_isa_dma = 1;
+		shost->sense_buffer_isa = 1;
 
 		flags = claim_dma_lock();
 		disable_dma(dma_channel);
@@ -1355,7 +1377,7 @@
 
 	for (i = 0; i < shost->can_queue; i++) {
 		size_t sz = shost->sg_tablesize *sizeof(struct sg_list);
-		gfp_t gfp_mask = (shost->unchecked_isa_dma ? GFP_DMA : 0) | GFP_ATOMIC;
+		gfp_t gfp_mask = (shost->sense_buffer_isa ? GFP_DMA : 0) | GFP_ATOMIC;
 		ha->cp[i].sglist = kmalloc(sz, gfp_mask);
 		if (!ha->cp[i].sglist) {
 			printk
@@ -1752,7 +1774,7 @@
 			       void (*done) (struct scsi_cmnd *))
 {
 	struct Scsi_Host *shost = SCpnt->device->host;
-	struct hostdata *ha = (struct hostdata *)shost->hostdata;
+	struct hostdata *ha = eata_shost_priv(shost);
 	unsigned int i, k;
 	struct mscp *cpp;
 
@@ -1836,7 +1858,7 @@
 static int eata2x_eh_abort(struct scsi_cmnd *SCarg)
 {
 	struct Scsi_Host *shost = SCarg->device->host;
-	struct hostdata *ha = (struct hostdata *)shost->hostdata;
+	struct hostdata *ha = eata_shost_priv(shost);
 	unsigned int i;
 
 	if (SCarg->host_scribble == NULL) {
@@ -1906,7 +1928,7 @@
 	int arg_done = 0;
 	struct scsi_cmnd *SCpnt;
 	struct Scsi_Host *shost = SCarg->device->host;
-	struct hostdata *ha = (struct hostdata *)shost->hostdata;
+	struct hostdata *ha = eata_shost_priv(shost);
 
 	scmd_printk(KERN_INFO, SCarg,
 		"reset, enter, pid %ld.\n", SCarg->serial_number);
@@ -2292,7 +2314,7 @@
 	unsigned int i, k, c, status, tstatus, reg;
 	struct mssp *spp;
 	struct mscp *cpp;
-	struct hostdata *ha = (struct hostdata *)shost->hostdata;
+	struct hostdata *ha = eata_shost_priv(shost);
 	int irq = shost->irq;
 
 	/* Check if this board need to be serviced */
@@ -2552,7 +2574,7 @@
 
 static int eata2x_release(struct Scsi_Host *shost)
 {
-	struct hostdata *ha = (struct hostdata *)shost->hostdata;
+	struct hostdata *ha = eata_shost_priv(shost);
 	unsigned int i;
 
 	for (i = 0; i < shost->can_queue; i++)
@@ -2572,6 +2594,8 @@
 		free_dma(shost->dma_channel);
 
 	release_region(shost->io_port, shost->n_io_port);
+	dma_free_coherent(ha->pdev ? &ha->pdev->dev : NULL,
+			  sizeof(*ha), ha, eata_shost_dma(shost));
 	scsi_unregister(shost);
 	return 0;
 }
author
#20 lore raw

PATCH 9/21 Remove unchecked_isa_dma in aha1542

In reply to Andi Kleen

- Audited ->cmnd use and it always copies
- Allocate DMAable hostdata separately
- Tell block layer explicitely to bounce
- Audited sense_buffer use and it always copies
- Remove unchecked_isa_dma finally

Untested due to lack of hardware

---
 drivers/scsi/aha1542.c |   40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)
drivers/scsi/aha1542.c +30 -10
Index: linux/drivers/scsi/aha1542.c
===================================================================
@@ -151,7 +151,13 @@
 	struct ccb ccb[AHA1542_MAILBOXES];
 };
 
-#define HOSTDATA(host) ((struct aha1542_hostdata *) &host->hostdata)
+struct hd_ptr {
+	struct aha1542_hostdata *hostptr;
+	dma_addr_t dma;
+};
+
+#define HOSTDATA(host) (((struct hd_ptr *)shost_priv(host))->hostptr)
+#define HOSTDMA(host) (((struct hd_ptr *)shost_priv(host))->dma)
 
 static DEFINE_SPINLOCK(aha1542_lock);
 
@@ -1125,23 +1131,28 @@
 	}
 	for (indx = 0; indx < ARRAY_SIZE(bases); indx++)
 		if (bases[indx] != 0 && request_region(bases[indx], 4, "aha1542")) {
-			shpnt = scsi_register(tpnt,
-					sizeof(struct aha1542_hostdata));
+			struct aha1542_hostdata *host;
+
+			shpnt = scsi_register(tpnt, sizeof(struct hd_ptr));
 
 			if(shpnt==NULL) {
 				release_region(bases[indx], 4);
 				continue;
 			}
-			/* For now we do this - until kmalloc is more intelligent
-			   we are resigned to stupid hacks like this */
-			if (SCSI_BUF_PA(shpnt) >= ISA_DMA_THRESHOLD) {
-				printk(KERN_ERR "Invalid address for shpnt with 1542.\n");
-				goto unregister;
+
+			host = dma_alloc_coherent(NULL, sizeof(*host),
+						  &HOSTDMA(shpnt), GFP_KERNEL);
+			if (!host) {
+				scsi_unregister(shpnt);
+				release_region(bases[indx], 4);
+				continue;
 			}
+
+			HOSTDATA(shpnt) = host;
+
 			if (!aha1542_test_port(bases[indx], shpnt))
 				goto unregister;
 
-
 			base_io = bases[indx];
 
 			/* Set the Bus on/off-times as not to ruin floppy performance */
@@ -1259,6 +1270,8 @@
 			continue;
 unregister:
 			release_region(bases[indx], 4);
+			dma_free_coherent(NULL, sizeof(struct aha1542_hostdata),
+					  HOSTDATA(shpnt), HOSTDMA(shpnt));
 			scsi_unregister(shpnt);
 			continue;
 
@@ -1275,6 +1288,8 @@
 		free_dma(shost->dma_channel);
 	if (shost->io_port && shost->n_io_port)
 		release_region(shost->io_port, shost->n_io_port);
+	dma_free_coherent(NULL, sizeof(struct aha1542_hostdata),
+			  HOSTDATA(shost), HOSTDMA(shost));
 	scsi_unregister(shost);
 	return 0;
 }
@@ -1746,6 +1761,11 @@
 }
 MODULE_LICENSE("GPL");
 
+static int aha154x_adjust_queue(struct scsi_device *device)
+{
+	blk_queue_bounce_limit(device->request_queue, BLK_BOUNCE_ISA);
+	return 0;
+}
 
 static struct scsi_host_template driver_template = {
 	.proc_name		= "aha1542",
@@ -1761,7 +1781,7 @@
 	.this_id		= 7,
 	.sg_tablesize		= AHA1542_SCATTER,
 	.cmd_per_lun		= AHA1542_CMDLUN,
-	.unchecked_isa_dma	= 1, 
 	.use_clustering		= ENABLE_CLUSTERING,
+	.slave_alloc		= aha154x_adjust_queue,
 };
 #include "scsi_module.c"
author
#21 lore raw

PATCH 10/21 Remove unchecked_isa_dma in aha152x/wd7000/sym53c416/u14-34f/NCR53c406a

In reply to Andi Kleen

I lumped these all together because these old ISA only drivers all look very unmaintained and the changes were relatively simple.

I audited them for possible use of unchecked_isa_dma and fixed the cases who needed them:
- Allocate separate dma'able hostdata when needed
- Checked that they all always copy ->cmnd
- Checked if they need sense_buffer bouncing and enable when needed (i'm not 100% sure what it means if a driver does not reference sense_buffer, but all except ultrastor and u14-34f do not)
- Add a explicit slave_alloc callback to enable block layer bouncing

Untested due to lack of hardware.

---
 drivers/scsi/NCR53c406a.c |    8 +++++-
 drivers/scsi/aha152x.c    |   29 +++++++++++++++++++++---
 drivers/scsi/sym53c416.c  |    9 ++++++-
 drivers/scsi/u14-34f.c    |   54 +++++++++++++++++++++++++++++++++++-----------
 drivers/scsi/ultrastor.c  |   13 ++++++++---
 drivers/scsi/wd7000.c     |   48 +++++++++++++++++++++++++++-------------
 6 files changed, 124 insertions(+), 37 deletions(-)
drivers/scsi/aha152x.c +25 -4
Index: linux/drivers/scsi/aha152x.c
===================================================================
@@ -551,6 +551,10 @@
 	struct list_head host_list;
 };
 
+struct aha152x_hostdata_ptr {
+	struct aha152x_hostdata *host;
+	dma_addr_t dma;
+};
 
 /*
  * host specific command extension
@@ -564,7 +568,10 @@
 
 /* access macros for hostdata */
 
-#define HOSTDATA(shpnt)		((struct aha152x_hostdata *) &shpnt->hostdata)
+#define HOSTDATA(shpnt) \
+	(((struct aha152x_hostdata_ptr *) shost_priv(shpnt))->host)
+#define HOSTDMA(shpnt) \
+	(((struct aha152x_hostdata_ptr *) shost_priv(shpnt))->dma)
 
 #define HOSTNO			((shpnt)->host_no)
 
@@ -771,14 +778,24 @@
 struct Scsi_Host *aha152x_probe_one(struct aha152x_setup *setup)
 {
 	struct Scsi_Host *shpnt;
+	struct aha152x_hostdata *host;
 
-	shpnt = scsi_host_alloc(&aha152x_driver_template, sizeof(struct aha152x_hostdata));
+	shpnt = scsi_host_alloc(&aha152x_driver_template,
+				sizeof(struct aha152x_hostdata_ptr));
 	if (!shpnt) {
 		printk(KERN_ERR "aha152x: scsi_host_alloc failed\n");
 		return NULL;
 	}
 
-	memset(HOSTDATA(shpnt), 0, sizeof *HOSTDATA(shpnt));
+	host = dma_alloc_coherent(NULL, sizeof(struct aha152x_hostdata),
+				  &HOSTDMA(shpnt), GFP_KERNEL);
+	HOSTDATA(shpnt) = host;
+	if (!host) {
+		scsi_host_put(shpnt);
+		printk(KERN_ERR "aha152x: dma alloc of hostdata failed\n");
+		return NULL;
+	}
+
 	INIT_LIST_HEAD(&HOSTDATA(shpnt)->host_list);
 
 	/* need to have host registered before triggering any interrupt */
@@ -899,6 +916,8 @@
 
 out_host_put:
 	list_del(&HOSTDATA(shpnt)->host_list);
+	dma_free_coherent(NULL, sizeof(struct aha152x_hostdata),
+			  HOSTDATA(shpnt), HOSTDMA(shpnt));
 	scsi_host_put(shpnt);
 
 	return NULL;
@@ -924,6 +943,8 @@
 #endif
 
 	list_del(&HOSTDATA(shpnt)->host_list);
+	dma_free_coherent(NULL, sizeof(struct aha152x_hostdata),
+			  HOSTDATA(shpnt), HOSTDMA(shpnt));
 	scsi_host_put(shpnt);
 }
 
@@ -3451,7 +3472,7 @@
 
 static int aha152x_adjust_queue(struct scsi_device *device)
 {
-	blk_queue_bounce_limit(device->request_queue, BLK_BOUNCE_HIGH);
+	blk_queue_bounce_limit(device->request_queue, BLK_BOUNCE_ISA);
 	return 0;
 }
 
drivers/scsi/wd7000.c +32 -16

123 lines not on this page · raw message

drivers/scsi/NCR53c406a.c +7 -1
Index: linux/drivers/scsi/NCR53c406a.c
===================================================================
@@ -1045,6 +1045,12 @@
 
 MODULE_LICENSE("GPL");
 
+static int NCR53c406a_adjust_queue(struct scsi_device *device)
+{
+	blk_queue_bounce_limit(device->request_queue, BLK_BOUNCE_ISA);
+	return 0;
+}
+
 /* NOTE:  scatter-gather support only works in PIO mode.
  * Use SG_NONE if DMA mode is enabled!
  */
@@ -1063,8 +1069,8 @@
      .this_id           	= 7			/* SCSI ID of the chip */,
      .sg_tablesize      	= 32			/*SG_ALL*/ /*SG_NONE*/, 
      .cmd_per_lun       	= 1			/* commands per lun */, 
-     .unchecked_isa_dma 	= 1			/* unchecked_isa_dma */,
      .use_clustering    	= ENABLE_CLUSTERING,
+     .slave_alloc		= NCR53c406a_adjust_queue,
 };
 
 #include "scsi_module.c"
drivers/scsi/u14-34f.c +42 -12

149 lines not on this page · raw message

drivers/scsi/sym53c416.c +8 -1
Index: linux/drivers/scsi/sym53c416.c
===================================================================
@@ -825,6 +825,12 @@
 
 #endif
 
+static int sym53c416_adjust_queue(struct scsi_device *device)
+{
+	blk_queue_bounce_limit(device->request_queue, BLK_BOUNCE_ISA);
+	return 0;
+}
+
 static struct scsi_host_template driver_template = {
 	.proc_name =		"sym53c416",
 	.name =			"Symbios Logic 53c416",
@@ -838,7 +844,8 @@
 	.this_id =		SYM53C416_SCSI_ID,
 	.sg_tablesize =		32,
 	.cmd_per_lun =		1,
-	.unchecked_isa_dma =	1,
 	.use_clustering =	ENABLE_CLUSTERING,
+	.slave_alloc =		sym53c416_adjust_queue,
+
 };
 #include "scsi_module.c"
drivers/scsi/ultrastor.c +10 -3
Index: linux/drivers/scsi/ultrastor.c
===================================================================
@@ -350,6 +350,12 @@
 }
 #endif
 
+static int ultrastor_adjust_queue(struct scsi_device *device)
+{
+	blk_queue_bounce_limit(device->request_queue, BLK_BOUNCE_ISA);
+	return 0;
+}
+
 static int ultrastor_14f_detect(struct scsi_host_template * tpnt)
 {
     size_t i;
@@ -501,7 +507,10 @@
 	   config.dma_channel, config.ha_scsi_id, config.subversion);
 #endif
     tpnt->this_id = config.ha_scsi_id;
-    tpnt->unchecked_isa_dma = (config.subversion != U34F);
+    if (config.subversion != U34F) {
+	tpnt->sense_buffer_isa = 1;
+	tpnt->slave_alloc = ultrastor_adjust_queue;
+    }
 
 #if ULTRASTOR_MAX_CMDS > 1
     config.mscp_free = ~0;
@@ -605,7 +614,6 @@
 	     config.interrupt, config.ha_scsi_id);
 #endif
       tpnt->this_id = config.ha_scsi_id;
-      tpnt->unchecked_isa_dma = 0;
       tpnt->sg_tablesize = ULTRASTOR_24F_MAX_SG;
 
       shpnt = scsi_register(tpnt, 0);
@@ -1202,7 +1210,6 @@
 	.can_queue         = ULTRASTOR_MAX_CMDS,
 	.sg_tablesize      = ULTRASTOR_14F_MAX_SG,
 	.cmd_per_lun       = ULTRASTOR_MAX_CMDS_PER_LUN,
-	.unchecked_isa_dma = 1,
 	.use_clustering    = ENABLE_CLUSTERING,
 };
 #include "scsi_module.c"
author
#22 lore raw

PATCH 11/21 Remove GFP_DMA uses in st/osst

In reply to Andi Kleen

Parts of it were already dead code: the ->dma member was only set, but not read.

Then also remove the code to allocate buffers with GFP_DMA, because this goes all through the block layer which bounces anyways.

Also remove code to disable direct sg for unchecked_isa_dma

---
 drivers/scsi/osst.c |   24 ++++++++++--------------
 drivers/scsi/osst.h |    2 --
 drivers/scsi/st.c   |   35 ++++++++++++++++-------------------
 drivers/scsi/st.h   |    2 --
 4 files changed, 26 insertions(+), 37 deletions(-)
drivers/scsi/osst.c +10 -14
Index: linux/drivers/scsi/osst.c
===================================================================
@@ -158,8 +158,8 @@
 
 static int modes_defined = 0;
 
-static struct osst_buffer *new_tape_buffer(int, int, int);
-static int enlarge_buffer(struct osst_buffer *, int);
+static struct osst_buffer *new_tape_buffer(int, int);
+static int enlarge_buffer(struct osst_buffer *);
 static void normalize_buffer(struct osst_buffer *);
 static int append_to_buffer(const char __user *, struct osst_buffer *, int);
 static int from_buffer(struct osst_buffer *, char __user *, int);
@@ -4431,7 +4431,7 @@
 		STp->header_ok = 0;
 
 	/* Allocate data segments for this device's tape buffer */
-	if (!enlarge_buffer(STp->buffer, STp->restr_dma)) {
+	if (!enlarge_buffer(STp->buffer)) {
 		printk(KERN_ERR "%s:E: Unable to allocate memory segments for tape buffer.\n", name);
 		retval = (-EOVERFLOW);
 		goto err_out;
@@ -5210,7 +5210,7 @@
 /* Memory handling routines */
 
 /* Try to allocate a new tape buffer skeleton. Caller must not hold os_scsi_tapes_lock */
-static struct osst_buffer * new_tape_buffer( int from_initialization, int need_dma, int max_sg )
+static struct osst_buffer * new_tape_buffer( int from_initialization, int max_sg )
 {
 	int i;
 	gfp_t priority;
@@ -5231,19 +5231,18 @@
 	tb->sg_segs = tb->orig_sg_segs = 0;
 	tb->use_sg = max_sg;
 	tb->in_use = 1;
-	tb->dma = need_dma;
 	tb->buffer_size = 0;
 #if DEBUG
 	if (debugging) 
 		printk(OSST_DEB_MSG
-			"osst :D: Allocated tape buffer skeleton (%d bytes, %d segments, dma: %d).\n",
-			   i, max_sg, need_dma);
+			"osst :D: Allocated tape buffer skeleton (%d bytes, %d segments).\n",
+			   i, max_sg);
 #endif
 	return tb;
 }
 
 /* Try to allocate a temporary (while a user has the device open) enlarged tape buffer */
-static int enlarge_buffer(struct osst_buffer *STbuffer, int need_dma)
+static int enlarge_buffer(struct osst_buffer *STbuffer)
 {
 	int segs, nbr, max_segs, b_size, order, got;
 	gfp_t priority;
@@ -5261,8 +5260,6 @@
 		return 0;
 
 	priority = GFP_KERNEL /* | __GFP_NOWARN */;
-	if (need_dma)
-		priority |= GFP_DMA;
 
 	/* Try to allocate the first segment up to OS_DATA_SIZE and the others
 	   big enough to reach the goal (code assumes no segments in place) */
@@ -5307,8 +5304,8 @@
 #if DEBUG
 	if (debugging) {
 		printk(OSST_DEB_MSG
-			   "osst :D: Expanded tape buffer (%d bytes, %d->%d segments, dma: %d, at: %p).\n",
-			   got, STbuffer->orig_sg_segs, STbuffer->sg_segs, need_dma, STbuffer->b_data);
+			   "osst :D: Expanded tape buffer (%d bytes, %d->%d segments, at: %p).\n",
+			   got, STbuffer->orig_sg_segs, STbuffer->sg_segs, STbuffer->b_data);
 		printk(OSST_DEB_MSG
 			   "osst :D: segment sizes: first %d at %p, last %d bytes at %p.\n",
 			   STbuffer->sg[0].length, page_address(STbuffer->sg[0].page),
@@ -5806,7 +5803,7 @@
 	i = SDp->host->sg_tablesize;
 	if (osst_max_sg_segs < i)
 		i = osst_max_sg_segs;
-	buffer = new_tape_buffer(1, SDp->host->unchecked_isa_dma, i);
+	buffer = new_tape_buffer(1, i);
 	if (buffer == NULL) {
 		write_unlock(&os_scsi_tapes_lock);
 		printk(KERN_ERR "osst :E: Unable to allocate a tape buffer, device not attached.\n");
@@ -5824,7 +5821,6 @@
 	tpnt->capacity = 0xfffff;
 	tpnt->dirty = 0;
 	tpnt->drv_buffer = 1;  /* Try buffering if no mode sense */
-	tpnt->restr_dma = (SDp->host)->unchecked_isa_dma;
 	tpnt->density = 0;
 	tpnt->do_auto_lock = OSST_AUTO_LOCK;
 	tpnt->can_bsr = OSST_IN_FILE_POS;
drivers/scsi/osst.h +0 -2
Index: linux/drivers/scsi/osst.h
===================================================================
@@ -510,7 +510,6 @@
 /* The OnStream tape buffer descriptor. */
 struct osst_buffer {
   unsigned char in_use;
-  unsigned char dma;	/* DMA-able buffer */
   int buffer_size;
   int buffer_blocks;
   int buffer_bytes;
@@ -544,7 +543,6 @@
   unsigned char can_partitions;
   unsigned char two_fm;
   unsigned char fast_mteom;
-  unsigned char restr_dma;
   unsigned char scsi2_logical;
   unsigned char default_drvbuffer;  /* 0xff = don't touch, value 3 bits */
   unsigned char pos_unknown;        /* after reset position unknown */
drivers/scsi/st.c +16 -19
Index: linux/drivers/scsi/st.c
===================================================================
@@ -182,8 +182,8 @@
 
 static int modes_defined;
 
-static struct st_buffer *new_tape_buffer(int, int, int);
-static int enlarge_buffer(struct st_buffer *, int, int);
+static struct st_buffer *new_tape_buffer(int, int);
+static int enlarge_buffer(struct scsi_tape *t, struct st_buffer *, int);
 static void clear_buffer(struct st_buffer *);
 static void normalize_buffer(struct st_buffer *);
 static int append_to_buffer(const char __user *, struct st_buffer *, int);
@@ -1155,7 +1155,7 @@
 	}
 
 	/* See that we have at least a one page buffer available */
-	if (!enlarge_buffer(STp->buffer, PAGE_SIZE, STp->restr_dma)) {
+	if (!enlarge_buffer(STp, STp->buffer, PAGE_SIZE)) {
 		printk(KERN_WARNING "%s: Can't allocate one page tape buffer.\n",
 		       name);
 		retval = (-EOVERFLOW);
@@ -1447,7 +1447,7 @@
 		}
 
 		if (bufsize > STbp->buffer_size &&
-		    !enlarge_buffer(STbp, bufsize, STp->restr_dma)) {
+		    !enlarge_buffer(STp, STbp, bufsize)) {
 			printk(KERN_WARNING "%s: Can't allocate %d byte tape buffer.\n",
 			       tape_name(STp), bufsize);
 			retval = (-EOVERFLOW);
@@ -3606,7 +3606,7 @@
 /* Try to allocate a new tape buffer. Calling function must not hold
    dev_arr_lock. */
 static struct st_buffer *
- new_tape_buffer(int from_initialization, int need_dma, int max_sg)
+ new_tape_buffer(int from_initialization, int max_sg)
 {
 	int i, got = 0;
 	gfp_t priority;
@@ -3628,7 +3628,6 @@
 	tb->use_sg = max_sg;
 	tb->frp = (struct st_buf_fragment *)(&(tb->sg[0]) + max_sg);
 
-	tb->dma = need_dma;
 	tb->buffer_size = got;
 	sg_init_table(tb->sg, max_sg);
 
@@ -3637,9 +3636,11 @@
 
 
 /* Try to allocate enough space in the tape buffer */
-static int enlarge_buffer(struct st_buffer * STbuffer, int new_size, int need_dma)
+static int enlarge_buffer(struct scsi_tape *t, struct st_buffer * STbuffer,
+			  int new_size)
 {
-	int segs, nbr, max_segs, b_size, order, got;
+	struct request_queue *q = t->device->request_queue;
+	int segs, nbr, max_segs, b_size, got;
 	gfp_t priority;
 
 	if (new_size <= STbuffer->buffer_size)
@@ -3654,20 +3655,18 @@
 		return 0;
 
 	priority = GFP_KERNEL | __GFP_NOWARN;
-	if (need_dma)
-		priority |= GFP_DMA;
-	for (b_size = PAGE_SIZE, order=0; order <= 6 &&
+	for (b_size = PAGE_SIZE; b_size <= (PAGE_SIZE << 6) &&
 	     b_size < new_size - STbuffer->buffer_size;
-	     order++, b_size *= 2)
+	     b_size *= 2)
 		;  /* empty */
 
 	for (segs = STbuffer->frp_segs, got = STbuffer->buffer_size;
 	     segs < max_segs && got < new_size;) {
-		STbuffer->frp[segs].page = alloc_pages(priority, order);
+		STbuffer->frp[segs].page = alloc_pages_mask(priority,
+					b_size, blk_q_mask(q));
 		if (STbuffer->frp[segs].page == NULL) {
 			if (new_size - got <= (max_segs - segs) * b_size / 2) {
 				b_size /= 2; /* Large enough for the rest of the buffers */
-				order--;
 				continue;
 			}
 			DEB(STbuffer->buffer_size = got);
@@ -3702,11 +3701,10 @@
 /* Release the extra buffer */
 static void normalize_buffer(struct st_buffer * STbuffer)
 {
-	int i, order;
+	int i;
 
 	for (i = STbuffer->orig_frp_segs; i < STbuffer->frp_segs; i++) {
-		order = get_order(STbuffer->frp[i].length);
-		__free_pages(STbuffer->frp[i].page, order);
+		__free_pages_mask(STbuffer->frp[i].page, STbuffer->frp[i].length);
 		STbuffer->buffer_size -= STbuffer->frp[i].length;
 	}
 	STbuffer->frp_segs = STbuffer->orig_frp_segs;
@@ -3935,7 +3933,7 @@
 		SDp->request_queue->max_phys_segments);
 	if (st_max_sg_segs < i)
 		i = st_max_sg_segs;
-	buffer = new_tape_buffer(1, (SDp->host)->unchecked_isa_dma, i);
+	buffer = new_tape_buffer(1, i);
 	if (buffer == NULL) {
 		printk(KERN_ERR
 		       "st: Can't allocate new tape buffer. Device not attached.\n");
@@ -4014,7 +4012,6 @@
 	tpnt->dirty = 0;
 	tpnt->in_use = 0;
 	tpnt->drv_buffer = 1;	/* Try buffering if no mode sense */
-	tpnt->restr_dma = (SDp->host)->unchecked_isa_dma;
 	tpnt->use_pf = (SDp->scsi_level >= SCSI_2);
 	tpnt->density = 0;
 	tpnt->do_auto_lock = ST_AUTO_LOCK;
drivers/scsi/st.h +0 -2
Index: linux/drivers/scsi/st.h
===================================================================
@@ -33,7 +33,6 @@
 
 /* The tape buffer descriptor. */
 struct st_buffer {
-	unsigned char dma;	/* DMA-able buffer */
 	unsigned char do_dio;   /* direct i/o set up? */
 	unsigned char cleared;  /* internal buffer cleared after open? */
 	int buffer_size;
@@ -113,7 +112,6 @@
 	unsigned char two_fm;
 	unsigned char fast_mteom;
 	unsigned char immediate;
-	unsigned char restr_dma;
 	unsigned char scsi2_logical;
 	unsigned char default_drvbuffer;	/* 0xff = don't touch, value 3 bits */
 	unsigned char cln_mode;			/* 0 = none, otherwise sense byte nbr */
author
#23 lore raw

PATCH 12/21 Remove unchecked_isa_dma support for hostdata

In reply to Andi Kleen

All ISA drivers who relied on dma able hostdata have been converted to allocate it separately. So remove the unchecked_isa_dma hostdata support in the mid layer

---
 drivers/scsi/hosts.c |    3 ---
 1 file changed, 3 deletions(-)
drivers/scsi/hosts.c +0 -3
Index: linux/drivers/scsi/hosts.c
===================================================================
@@ -314,9 +314,6 @@
 	gfp_t gfp_mask = GFP_KERNEL;
 	int rval;
 
-	if (sht->unchecked_isa_dma && privsize)
-		gfp_mask |= __GFP_DMA;
-
 	shost = kzalloc(sizeof(struct Scsi_Host) + privsize, gfp_mask);
 	if (!shost)
 		return NULL;
author
#24 lore raw

PATCH 13/21 Use blk_q_mask/get_pages_mask in sg driver

In reply to Andi Kleen

Instead of using GFP_DMA directly.

Also I stubbed SG_SET_FORCE_LOW_DMA ioctls which don't make any sense because the kernel should always use the correct values on its own.

---
 drivers/scsi/sg.c |   36 ++++++++----------------------------
 include/scsi/sg.h |    1 -
 2 files changed, 8 insertions(+), 29 deletions(-)
drivers/scsi/sg.c +8 -28
Index: linux/drivers/scsi/sg.c
===================================================================
@@ -49,6 +49,7 @@
 #include <linux/delay.h>
 #include <linux/blktrace_api.h>
 #include <linux/smp_lock.h>
+#include <linux/gfp.h>
 
 #include "scsi.h"
 #include <scsi/scsi_dbg.h>
@@ -151,7 +152,6 @@
 	Sg_request *headrp;	/* head of request slist, NULL->empty */
 	struct fasync_struct *async_qp;	/* used by asynchronous notification */
 	Sg_request req_arr[SG_MAX_QUEUE];	/* used as singly-linked list */
-	char low_dma;		/* as in parent but possibly overridden to 1 */
 	char force_packid;	/* 1 -> pack_id input to read(), 0 -> ignored */
 	volatile char closed;	/* 1 -> fd closed but request(s) outstanding */
 	char cmd_q;		/* 1 -> allow command queuing, 0 -> don't */
@@ -844,24 +844,9 @@
 				/* strange ..., for backward compatibility */
 		return sfp->timeout_user;
 	case SG_SET_FORCE_LOW_DMA:
-		result = get_user(val, ip);
-		if (result)
-			return result;
-		if (val) {
-			sfp->low_dma = 1;
-			if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
-				val = (int) sfp->reserve.bufflen;
-				sg_remove_scat(&sfp->reserve);
-				sg_build_reserve(sfp, val);
-			}
-		} else {
-			if (sdp->detached)
-				return -ENODEV;
-			sfp->low_dma = sdp->device->host->unchecked_isa_dma;
-		}
-		return 0;
+		return -EINVAL;
 	case SG_GET_LOW_DMA:
-		return put_user((int) sfp->low_dma, ip);
+		return put_user(0, ip);
 	case SG_GET_SCSI_ID:
 		if (!access_ok(VERIFY_WRITE, p, sizeof (sg_scsi_id_t)))
 			return -EFAULT;
@@ -1651,7 +1636,6 @@
 
 	if (sg_allow_dio && hp->flags & SG_FLAG_DIRECT_IO &&
 	    dxfer_dir != SG_DXFER_UNKNOWN && !iov_count &&
-	    !sfp->parentdp->device->host->unchecked_isa_dma &&
 	    blk_rq_aligned(q, hp->dxferp, dxfer_len))
 		md = NULL;
 	else
@@ -1727,6 +1711,7 @@
 static int
 sg_build_indirect(Sg_scatter_hold * schp, Sg_fd * sfp, int buff_size)
 {
+	struct scsi_device *dev = sfp->parentdp->device;
 	int ret_sz = 0, i, k, rem_sz, num, mx_sc_elems;
 	int sg_tablesize = sfp->parentdp->sg_tablesize;
 	int blk_size = buff_size, order;
@@ -1755,9 +1740,6 @@
 			scatter_elem_sz_prev = num;
 	}
 
-	if (sfp->low_dma)
-		gfp_mask |= GFP_DMA;
-
 	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
 		gfp_mask |= __GFP_ZERO;
 
@@ -1771,7 +1753,8 @@
 		num = (rem_sz > scatter_elem_sz_prev) ?
 			scatter_elem_sz_prev : rem_sz;
 
-		schp->pages[k] = alloc_pages(gfp_mask, order);
+		schp->pages[k] = alloc_pages_mask(gfp_mask, num,
+						  blk_q_mask(dev->request_queue));
 		if (!schp->pages[k])
 			goto out;
 
@@ -2063,8 +2046,6 @@
 	sfp->timeout = SG_DEFAULT_TIMEOUT;
 	sfp->timeout_user = SG_DEFAULT_TIMEOUT_USER;
 	sfp->force_packid = SG_DEF_FORCE_PACK_ID;
-	sfp->low_dma = (SG_DEF_FORCE_LOW_DMA == 0) ?
-	    sdp->device->host->unchecked_isa_dma : 1;
 	sfp->cmd_q = SG_DEF_COMMAND_Q;
 	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
 	sfp->parentdp = sdp;
@@ -2512,11 +2493,10 @@
 
 	for (k = 0; (fp = sg_get_nth_sfp(sdp, k)); ++k) {
 		seq_printf(s, "   FD(%d): timeout=%dms bufflen=%d "
-			   "(res)sgat=%d low_dma=%d\n", k + 1,
+			   "(res)sgat=%d\n", k + 1,
 			   jiffies_to_msecs(fp->timeout),
 			   fp->reserve.bufflen,
-			   (int) fp->reserve.k_use_sg,
-			   (int) fp->low_dma);
+			   (int) fp->reserve.k_use_sg);
 		seq_printf(s, "   cmd_q=%d f_packid=%d k_orphan=%d closed=%d\n",
 			   (int) fp->cmd_q, (int) fp->force_packid,
 			   (int) fp->keep_orphan, (int) fp->closed);
include/scsi/sg.h +0 -1
Index: linux/include/scsi/sg.h
===================================================================
@@ -231,7 +231,6 @@
 #define SG_DEFAULT_RETRIES 0
 
 /* Defaults, commented if they differ from original sg driver */
-#define SG_DEF_FORCE_LOW_DMA 0  /* was 1 -> memory below 16MB on i386 */
 #define SG_DEF_FORCE_PACK_ID 0
 #define SG_DEF_KEEP_ORPHAN 0
 #define SG_DEF_RESERVED_SIZE SG_SCATTER_SZ /* load time option */
maintainer
#25 lore raw

Re: [PATCH] [13/21] Use blk_q_mask/get_pages_mask in sg driver

Quoted 4 lines · Andi Kleen

Instead of using GFP_DMA directly.

Also I stubbed SG_SET_FORCE_LOW_DMA ioctls which don't make any sense because the kernel should always use the correct values on its own.

You propose removing a define from a public interface and thereby might break existing code. A comment in sg.h might be appropriate. Something that might trip:
find <kernel_src_tree> -name '*.[hc]' -exec \
grep SG_SET_FORCE_LOW_DMA {} \; -print

when the compiler gets upset.

Doug Gilbert

Quoted 132 lines
---
 drivers/scsi/sg.c |   36 ++++++++----------------------------
 include/scsi/sg.h |    1 -
 2 files changed, 8 insertions(+), 29 deletions(-)

Index: linux/drivers/scsi/sg.c
===================================================================

drivers/scsi/sg.c +8 -28
@@ -49,6 +49,7 @@
 #include <linux/delay.h>
 #include <linux/blktrace_api.h>
 #include <linux/smp_lock.h>
+#include <linux/gfp.h>
 
 #include "scsi.h"
 #include <scsi/scsi_dbg.h>
@@ -151,7 +152,6 @@
 	Sg_request *headrp;	/* head of request slist, NULL->empty */
 	struct fasync_struct *async_qp;	/* used by asynchronous notification */
 	Sg_request req_arr[SG_MAX_QUEUE];	/* used as singly-linked list */
-	char low_dma;		/* as in parent but possibly overridden to 1 */
 	char force_packid;	/* 1 -> pack_id input to read(), 0 -> ignored */
 	volatile char closed;	/* 1 -> fd closed but request(s) outstanding */
 	char cmd_q;		/* 1 -> allow command queuing, 0 -> don't */
@@ -844,24 +844,9 @@
 				/* strange ..., for backward compatibility */
 		return sfp->timeout_user;
 	case SG_SET_FORCE_LOW_DMA:
-		result = get_user(val, ip);
-		if (result)
-			return result;
-		if (val) {
-			sfp->low_dma = 1;
-			if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
-				val = (int) sfp->reserve.bufflen;
-				sg_remove_scat(&sfp->reserve);
-				sg_build_reserve(sfp, val);
-			}
-		} else {
-			if (sdp->detached)
-				return -ENODEV;
-			sfp->low_dma = sdp->device->host->unchecked_isa_dma;
-		}
-		return 0;
+		return -EINVAL;
 	case SG_GET_LOW_DMA:
-		return put_user((int) sfp->low_dma, ip);
+		return put_user(0, ip);
 	case SG_GET_SCSI_ID:
 		if (!access_ok(VERIFY_WRITE, p, sizeof (sg_scsi_id_t)))
 			return -EFAULT;
@@ -1651,7 +1636,6 @@
 
 	if (sg_allow_dio && hp->flags & SG_FLAG_DIRECT_IO &&
 	    dxfer_dir != SG_DXFER_UNKNOWN && !iov_count &&
-	    !sfp->parentdp->device->host->unchecked_isa_dma &&
 	    blk_rq_aligned(q, hp->dxferp, dxfer_len))
 		md = NULL;
 	else
@@ -1727,6 +1711,7 @@
 static int
 sg_build_indirect(Sg_scatter_hold * schp, Sg_fd * sfp, int buff_size)
 {
+	struct scsi_device *dev = sfp->parentdp->device;
 	int ret_sz = 0, i, k, rem_sz, num, mx_sc_elems;
 	int sg_tablesize = sfp->parentdp->sg_tablesize;
 	int blk_size = buff_size, order;
@@ -1755,9 +1740,6 @@
 			scatter_elem_sz_prev = num;
 	}
 
-	if (sfp->low_dma)
-		gfp_mask |= GFP_DMA;
-
 	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
 		gfp_mask |= __GFP_ZERO;
 
@@ -1771,7 +1753,8 @@
 		num = (rem_sz > scatter_elem_sz_prev) ?
 			scatter_elem_sz_prev : rem_sz;
 
-		schp->pages[k] = alloc_pages(gfp_mask, order);
+		schp->pages[k] = alloc_pages_mask(gfp_mask, num,
+						  blk_q_mask(dev->request_queue));
 		if (!schp->pages[k])
 			goto out;
 
@@ -2063,8 +2046,6 @@
 	sfp->timeout = SG_DEFAULT_TIMEOUT;
 	sfp->timeout_user = SG_DEFAULT_TIMEOUT_USER;
 	sfp->force_packid = SG_DEF_FORCE_PACK_ID;
-	sfp->low_dma = (SG_DEF_FORCE_LOW_DMA == 0) ?
-	    sdp->device->host->unchecked_isa_dma : 1;
 	sfp->cmd_q = SG_DEF_COMMAND_Q;
 	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
 	sfp->parentdp = sdp;
@@ -2512,11 +2493,10 @@
 
 	for (k = 0; (fp = sg_get_nth_sfp(sdp, k)); ++k) {
 		seq_printf(s, "   FD(%d): timeout=%dms bufflen=%d "
-			   "(res)sgat=%d low_dma=%d\n", k + 1,
+			   "(res)sgat=%d\n", k + 1,
 			   jiffies_to_msecs(fp->timeout),
 			   fp->reserve.bufflen,
-			   (int) fp->reserve.k_use_sg,
-			   (int) fp->low_dma);
+			   (int) fp->reserve.k_use_sg);
 		seq_printf(s, "   cmd_q=%d f_packid=%d k_orphan=%d closed=%d\n",
 			   (int) fp->cmd_q, (int) fp->force_packid,
 			   (int) fp->keep_orphan, (int) fp->closed);

Index: linux/include/scsi/sg.h
===================================================================

include/scsi/sg.h +0 -2
@@ -231,7 +231,6 @@
 #define SG_DEFAULT_RETRIES 0
 
 /* Defaults, commented if they differ from original sg driver */
-#define SG_DEF_FORCE_LOW_DMA 0  /* was 1 -> memory below 16MB on i386 */
 #define SG_DEF_FORCE_PACK_ID 0
 #define SG_DEF_KEEP_ORPHAN 0
 #define SG_DEF_RESERVED_SIZE SG_SCATTER_SZ /* load time option */
--

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

author
#26 lore raw

Re: [PATCH] [13/21] Use blk_q_mask/get_pages_mask in sg driver

Quoted 8 lines · Douglas Gilbert
Quoted 4 lines · Andi Kleen

Instead of using GFP_DMA directly.

Also I stubbed SG_SET_FORCE_LOW_DMA ioctls which don't make any sense because the kernel should always use the correct values on its own.

You propose removing a define from a public interface and thereby might break existing code. A comment in sg.h might be appropriate.

Hmm, I assume those SG_SET_FORCE_* defines are only used in the kernel. Is there a realistic case where user space could use them or are just just talking in general, objecting to any change outside #ifdef __KERNEL__ ?

Quoted 5 lines

Something that might trip:
find <kernel_src_tree> -name '*.[hc]' -exec \
grep SG_SET_FORCE_LOW_DMA {} \; -print

when the compiler gets upset.

I did that of course (using id-utils) and there were no users in tree.

-Andi

#27 lore raw

Re: [PATCH] [13/21] Use blk_q_mask/get_pages_mask in sg driver

Quoted 4 lines · On Sun, 16 Nov 2008 23:17:22 +0100 Andi Kleen <[email protected]> wrote:
Quoted 3 lines · Douglas Gilbert
Quoted 2 lines · Andi Kleen

Instead of using GFP_DMA directly.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

Also I stubbed SG_SET_FORCE_LOW_DMA ioctls which don't make any sense because the kernel should always use the correct values on its own.

You propose removing a define from a public interface and thereby might break existing code. A comment in sg.h might be appropriate.

Hmm, I assume those SG_SET_FORCE_* defines are only used in the kernel.

I don't think so. They are exported to userspace, the sg ioctl interface. There may be applications that use them.

Quoted 3 lines

Is there a realistic case where user space
could use them or are just just talking in general, objecting to any change outside #ifdef __KERNEL__ ?

Users can use them to avoid the block-layer bouncing with a driver doesn't set unchecked_isa_dma but can't dma with all the memory addresses.

Well, I don't think that they are useful or a nice feature. They would become pointless when you finish your dma allocation rework. But I don't think that we can remove the existing API exported to userspace.

author
#28 lore raw

Re: [PATCH] [13/21] Use blk_q_mask/get_pages_mask in sg driver

Quoted 10 lines · FUJITA Tomonori
Quoted 7 lines · On Sun, 16 Nov 2008 23:17:22 +0100 Andi Kleen <[email protected]> wrote:
Quoted 6 lines · Douglas Gilbert
Quoted 4 lines · Andi Kleen

Instead of using GFP_DMA directly.

Also I stubbed SG_SET_FORCE_LOW_DMA ioctls which don't make any sense because the kernel should always use the correct values on its own.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

You propose removing a define from a public interface and thereby might break existing code. A comment in sg.h might be appropriate.

Hmm, I assume those SG_SET_FORCE_* defines are only used in the kernel.

I don't think so. They are exported to userspace, the sg ioctl interface. There may be applications that use them.

I doubt it. For what would they use it? And when exactly should an application know that some device has a 16MB limit when the kernel driver doesn't know about that?

For me it looks more like a really old debugging interface.

Quoted 3 lines

Well, I don't think that they are useful or a nice feature. They would become pointless when you finish your dma allocation rework. But I don't think that we can remove the existing API exported to userspace.

Ok I'll restore the defines. But readding the ioctls would be messy. Do you really think that is needed? I would rather prefer to fix the driver to always GFP_DMA, but I am not aware of any driver who needs it. Are you?

-Andi

#29 lore raw

Re: [PATCH] [13/21] Use blk_q_mask/get_pages_mask in sg driver

Quoted 6 lines · On Mon, 17 Nov 2008 11:22:28 +0100 Andi Kleen <[email protected]> wrote:
Quoted 3 lines

Well, I don't think that they are useful or a nice feature. They would become pointless when you finish your dma allocation rework. But I don't think that we can remove the existing API exported to userspace.

Ok I'll restore the defines. But readding the ioctls would be messy. Do you really think that is needed?

As I wrote in the previous mail, I don't think that it's useful. But it's not the point. My point is that we can't remove the existing sg ioctl interface like we can't remove the system calls. Do I misunderstand the rules about the userspace API?

Quoted 2 lines

I would rather prefer to fix the driver to always GFP_DMA, but I am not aware of any driver who needs it. Are you?

Well, theoretically, there migth be an user who want these ioctl with all the drivers that are not capable of 64bit DMA since sg allocates the reserved buffer with GFP_KERNEL. These driver might use the block-layer bounce buffer. Users might want to use SG_SET_FORCE_LOW_DMA to avoid the bounce buffer latency.

author
#30 lore raw

Re: [PATCH] [13/21] Use blk_q_mask/get_pages_mask in sg driver

Quoted 8 lines
Quoted 2 lines

I would rather prefer to fix the driver to always GFP_DMA, but I am not aware of any driver who needs it. Are you?

Well, theoretically, there migth be an user who want these ioctl with all the drivers that are not capable of 64bit DMA since sg allocates the reserved buffer with GFP_KERNEL. These driver might use the block-layer bounce buffer. Users might want to use SG_SET_FORCE_LOW_DMA to avoid the bounce buffer latency.

But that would force everything through the 16MB zone, putting a lot of pressure on it and might even overflow on some loads. I don't think that's a good strategy for any application.

-Andi

Signature · 1 line
[email protected]
#31 lore raw

Re: [PATCH] [13/21] Use blk_q_mask/get_pages_mask in sg driver

Quoted 9 lines · On Mon, 17 Nov 2008 12:23:46 +0100 Andi Kleen <[email protected]> wrote:
Quoted 8 lines
Quoted 2 lines

I would rather prefer to fix the driver to always GFP_DMA, but I am not aware of any driver who needs it. Are you?

Well, theoretically, there migth be an user who want these ioctl with all the drivers that are not capable of 64bit DMA since sg allocates the reserved buffer with GFP_KERNEL. These driver might use the block-layer bounce buffer. Users might want to use SG_SET_FORCE_LOW_DMA to avoid the bounce buffer latency.

Quoted 3 lines

But that would force everything through the 16MB zone, putting a lot of pressure on it and might even overflow on some loads. I don't think that's a good strategy for any application.

Yeah, I know. It's not nice (as I wrote in the first mail), it doesn't work well, etc. But it's not the point (as I wrote in the previous mail). I'm against the patch just because I don't think the rules about the userspace API permit removing the exiting API like this.

If the rules permit, I'm happy to try to remove more old features of sg.

author
#32 lore raw

Re: [PATCH] [13/21] Use blk_q_mask/get_pages_mask in sg driver

Quoted 4 lines

Yeah, I know. It's not nice (as I wrote in the first mail), it doesn't work well, etc. But it's not the point (as I wrote in the previous mail). I'm against the patch just because I don't think the rules about the userspace API permit removing the exiting API like this.

Ok perhaps removing the define was a bit much, but at least stubbing the ioctl should be transparent to the application.

Ok except that I return EINVAL in the set variant now. I could lie and return 0 instead, although I suspect it wouldn't make much difference either way.

Quoted 3 lines

If the rules permit, I'm happy to try to remove more old features of sg.

As long as old binaries don't break I don't see a problem with any such changes (afaik that's true for my change)

If you want to break something you have to just go through feature deprecation and printk, but normally it's only a good idea to do that if it saves a lot of code or ugliness.

-Andi

Signature · 1 line
[email protected]
#33 lore raw

Re: [PATCH] [13/21] Use blk_q_mask/get_pages_mask in sg driver

Quoted 13 lines · On Mon, 17 Nov 2008 12:43:38 +0100 Andi Kleen <[email protected]> wrote:
Quoted 4 lines

Yeah, I know. It's not nice (as I wrote in the first mail), it doesn't work well, etc. But it's not the point (as I wrote in the previous mail). I'm against the patch just because I don't think the rules about the userspace API permit removing the exiting API like this.

Ok perhaps removing the define was a bit much, but at least stubbing the ioctl should be transparent to the application.

Ok except that I return EINVAL in the set variant now. I could lie and return 0 instead, although I suspect it wouldn't make much difference either way.

Quoted 1 line
Quoted 5 lines
Quoted 2 lines

If the rules permit, I'm happy to try to remove more old features of sg.

As long as old binaries don't break I don't see a problem with any such changes (afaik that's true for my change)

Well, your change might break old binaries. We never know.

As discussed, SG_SET_FORCE_LOW_DMA is not useful and doesn't work well. But it might work in some environments.

If you make it lie and just return 0, the bounce buffer latency could break old binaries. It's unlikely but we never know, that's the whole point of the rules about the userspace API?

But if the rules permit, the patch is fine by me.

author
#34 lore raw

PATCH 14/21 Rely on block layer bouncing for ISA DMA devices scanning

In reply to Andi Kleen

Remove unchecked_isa_dma references in scsi_scan.c Instead we rely on the block layer bouncing here. This is safe because all ISA DMA drivers have been converted to set a suitable bounce pfn. Also scanning is not performance critical, so a copy more or less doesn't matter.

---
 drivers/scsi/scsi_scan.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/scsi/scsi_scan.c +2 -4
Index: linux/drivers/scsi/scsi_scan.c
===================================================================
@@ -1028,8 +1028,7 @@
 	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;
 
@@ -1348,8 +1347,7 @@
 	 * 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, __func__);
 		goto out;
author
#35 lore raw

PATCH 15/21 Don't disable direct_io for unchecked_isa_dma in st.c

In reply to Andi Kleen

Block layer bounces anyways.

---
 drivers/scsi/st.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/st.c +1 -1
Index: linux/drivers/scsi/st.c
===================================================================
@@ -4028,7 +4028,7 @@
 	tpnt->nbr_partitions = 0;
 	tpnt->device->timeout = ST_TIMEOUT;
 	tpnt->long_timeout = ST_LONG_TIMEOUT;
-	tpnt->try_dio = try_direct_io && !SDp->host->unchecked_isa_dma;
+	tpnt->try_dio = try_direct_io;
 
 	for (i = 0; i < ST_NBR_MODES; i++) {
 		STm = &(tpnt->modes[i]);
author
#36 lore raw

PATCH 16/21 Remove automatic block layer bouncing for unchecked_isa_dma

In reply to Andi Kleen

All ISA drivers explicitely tell the block layer now that it needs to bounce for them, so SCSI mid layer doesn't need to do that automatically for unchecked_isa_dma anymore.

---
 drivers/scsi/scsi_lib.c |    2 --
 1 file changed, 2 deletions(-)
drivers/scsi/scsi_lib.c +0 -2
Index: linux/drivers/scsi/scsi_lib.c
===================================================================
@@ -1732,8 +1732,6 @@
 	struct device *host_dev;
 	u64 bounce_limit = 0xffffffff;
 
-	if (shost->unchecked_isa_dma)
-		return BLK_BOUNCE_ISA;
 	/*
 	 * Platforms with virtual-DMA translation
 	 * hardware have no practical limit.
author
#37 lore raw

PATCH 17/21 Remove GFP_DMA use in sr.c

In reply to Andi Kleen

Just remove GFP_DMA in all explicit IO calls in sr.c and friends.

The block layer will bounce correctly now, and as far as I can see all the explicit IOs here are only for very small data sizes, so one copy more does not really matter.

---
 drivers/scsi/sr.c        |    2 +-
 drivers/scsi/sr_ioctl.c  |   13 ++++---------
 drivers/scsi/sr_vendor.c |    4 ++--
 3 files changed, 7 insertions(+), 12 deletions(-)
drivers/scsi/sr.c +1 -1
Index: linux/drivers/scsi/sr.c
===================================================================
@@ -761,7 +761,7 @@
 
 
 	/* allocate transfer buffer */
-	buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
+	buffer = kmalloc(512, GFP_KERNEL);
 	if (!buffer) {
 		printk(KERN_ERR "sr: out of memory.\n");
 		return;
drivers/scsi/sr_ioctl.c +4 -9
Index: linux/drivers/scsi/sr_ioctl.c
===================================================================
@@ -30,11 +30,6 @@
 
 module_param(xa_test, int, S_IRUGO | S_IWUSR);
 
-/* primitive to determine whether we need to have GFP_DMA set based on
- * the status of the unchecked_isa_dma flag in the host structure */
-#define SR_GFP_DMA(cd) (((cd)->device->host->unchecked_isa_dma) ? GFP_DMA : 0)
-
-
 static int sr_read_tochdr(struct cdrom_device_info *cdi,
 		struct cdrom_tochdr *tochdr)
 {
@@ -43,7 +38,7 @@
 	int result;
 	unsigned char *buffer;
 
-	buffer = kmalloc(32, GFP_KERNEL | SR_GFP_DMA(cd));
+	buffer = kmalloc(32, GFP_KERNEL);
 	if (!buffer)
 		return -ENOMEM;
 
@@ -73,7 +68,7 @@
 	int result;
 	unsigned char *buffer;
 
-	buffer = kmalloc(32, GFP_KERNEL | SR_GFP_DMA(cd));
+	buffer = kmalloc(32, GFP_KERNEL);
 	if (!buffer)
 		return -ENOMEM;
 
@@ -385,7 +380,7 @@
 {
 	Scsi_CD *cd = cdi->handle;
 	struct packet_command cgc;
-	char *buffer = kmalloc(32, GFP_KERNEL | SR_GFP_DMA(cd));
+	char *buffer = kmalloc(32, GFP_KERNEL);
 	int result;
 
 	if (!buffer)
@@ -564,7 +559,7 @@
 	if (!xa_test)
 		return 0;
 
-	raw_sector = kmalloc(2048, GFP_KERNEL | SR_GFP_DMA(cd));
+	raw_sector = kmalloc(2048, GFP_KERNEL);
 	if (!raw_sector)
 		return -ENOMEM;
 	if (0 == sr_read_sector(cd, cd->ms_offset + 16,
drivers/scsi/sr_vendor.c +2 -2
Index: linux/drivers/scsi/sr_vendor.c
===================================================================
@@ -117,7 +117,7 @@
 		density = (blocklength > 2048) ? 0x81 : 0x83;
 #endif
 
-	buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
+	buffer = kmalloc(512, GFP_KERNEL);
 	if (!buffer)
 		return -ENOMEM;
 
@@ -164,7 +164,7 @@
 	if (cd->cdi.mask & CDC_MULTI_SESSION)
 		return 0;
 
-	buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
+	buffer = kmalloc(512, GFP_KERNEL);
 	if (!buffer)
 		return -ENOMEM;
 
author
#38 lore raw

PATCH 18/21 Remove unchecked_isa_dma from sysfs

In reply to Andi Kleen

I opted to remove it because it's unlikely that user space uses it.

An alternative would be to always make it report 0 now.

---
 drivers/scsi/scsi_sysfs.c |    2 --
 1 file changed, 2 deletions(-)
drivers/scsi/scsi_sysfs.c +0 -2
Index: linux/drivers/scsi/scsi_sysfs.c
===================================================================
@@ -249,7 +249,6 @@
 shost_rd_attr(cmd_per_lun, "%hd\n");
 shost_rd_attr(can_queue, "%hd\n");
 shost_rd_attr(sg_tablesize, "%hu\n");
-shost_rd_attr(unchecked_isa_dma, "%d\n");
 shost_rd_attr(prot_capabilities, "%u\n");
 shost_rd_attr(prot_guard_type, "%hd\n");
 shost_rd_attr2(proc_name, hostt->proc_name, "%s\n");
@@ -260,7 +259,6 @@
 	&dev_attr_cmd_per_lun.attr,
 	&dev_attr_can_queue.attr,
 	&dev_attr_sg_tablesize.attr,
-	&dev_attr_unchecked_isa_dma.attr,
 	&dev_attr_proc_name.attr,
 	&dev_attr_scan.attr,
 	&dev_attr_hstate.attr,
author
#39 lore raw

PATCH 19/21 Switch to a single SCSI command pool

In reply to Andi Kleen

Now that no low level driver relies on ISA DMAable scsi_cmnds anymore it is safe to always use the same static slab for them.

---
 drivers/scsi/scsi.c |   58 +++++++++++++++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 27 deletions(-)
drivers/scsi/scsi.c +31 -27
Index: linux/drivers/scsi/scsi.c
===================================================================
@@ -131,24 +131,24 @@
 
 EXPORT_SYMBOL(scsi_device_type);
 
+/* Writing of both protected by host_cmd_pool_mutex */
+static struct kmem_cache *scsi_cmd_slab;
+static int cmd_pool_users;
+
 struct scsi_host_cmd_pool {
-	struct kmem_cache	*cmd_slab;
 	struct kmem_cache	*sense_slab;
 	unsigned int		users;
-	char			*cmd_name;
 	char			*sense_name;
 	unsigned int		slab_flags;
 	gfp_t			gfp_mask;
 };
 
-static struct scsi_host_cmd_pool scsi_cmd_pool = {
-	.cmd_name	= "scsi_cmd_cache",
+static struct scsi_host_cmd_pool scsi_sense_pool = {
 	.sense_name	= "scsi_sense_cache",
 	.slab_flags	= SLAB_HWCACHE_ALIGN,
 };
 
-static struct scsi_host_cmd_pool scsi_cmd_dma_pool = {
-	.cmd_name	= "scsi_cmd_cache(DMA)",
+static struct scsi_host_cmd_pool scsi_sense_dma_pool = {
 	.sense_name	= "scsi_sense_cache(DMA)",
 	.slab_flags	= SLAB_HWCACHE_ALIGN|SLAB_CACHE_DMA,
 	.gfp_mask	= __GFP_DMA,
@@ -169,7 +169,7 @@
 {
 	struct scsi_cmnd *cmd;
 
-	cmd = kmem_cache_alloc(pool->cmd_slab, gfp_mask | pool->gfp_mask);
+	cmd = kmem_cache_alloc(scsi_cmd_slab, gfp_mask);
 	if (!cmd)
 		return NULL;
 
@@ -178,7 +178,7 @@
 	cmd->sense_buffer = kmem_cache_alloc(pool->sense_slab,
 					     gfp_mask | pool->gfp_mask);
 	if (!cmd->sense_buffer) {
-		kmem_cache_free(pool->cmd_slab, cmd);
+		kmem_cache_free(scsi_cmd_slab, cmd);
 		return NULL;
 	}
 
@@ -201,7 +201,7 @@
 		kmem_cache_free(scsi_sdb_cache, cmd->prot_sdb);
 
 	kmem_cache_free(pool->sense_slab, cmd->sense_buffer);
-	kmem_cache_free(pool->cmd_slab, cmd);
+	kmem_cache_free(scsi_cmd_slab, cmd);
 }
 
 /**
@@ -361,21 +361,26 @@
 	 * yet existent.
 	 */
 	mutex_lock(&host_cmd_pool_mutex);
-	pool = &scsi_cmd_pool;
-	if (shost && (shost->unchecked_isa_dma || shost->sense_buffer_isa))
-		pool = &scsi_cmd_dma_pool;
+	if (!scsi_cmd_slab) {
+		scsi_cmd_slab = kmem_cache_create("scsi_cmd_pool",
+						  sizeof(struct scsi_cmnd), 0,
+						  0, NULL);
+	}
+	if (!scsi_cmd_slab)
+		goto fail;
+	cmd_pool_users++;
+
+	pool = &scsi_sense_pool;
+	if (shost && shost->sense_buffer_isa)
+		pool = &scsi_sense_dma_pool;
 	if (!pool->users) {
-		pool->cmd_slab = kmem_cache_create(pool->cmd_name,
-						   sizeof(struct scsi_cmnd), 0,
-						   pool->slab_flags, NULL);
-		if (!pool->cmd_slab)
-			goto fail;
-
 		pool->sense_slab = kmem_cache_create(pool->sense_name,
 						     SCSI_SENSE_BUFFERSIZE, 0,
 						     pool->slab_flags, NULL);
 		if (!pool->sense_slab) {
-			kmem_cache_destroy(pool->cmd_slab);
+			kmem_cache_destroy(scsi_cmd_slab);
+			scsi_cmd_slab = NULL;
+			cmd_pool_users--;
 			goto fail;
 		}
 	}
@@ -392,8 +397,8 @@
 	struct scsi_host_cmd_pool *pool;
 
 	mutex_lock(&host_cmd_pool_mutex);
-	pool = (shost && (shost->unchecked_isa_dma || shost->sense_buffer_isa)) ?
-		&scsi_cmd_dma_pool : &scsi_cmd_pool;
+	pool = (shost && shost->sense_buffer_isa) ?
+		&scsi_sense_dma_pool : &scsi_sense_pool;
 	/*
 	 * This may happen if a driver has a mismatched get and put
 	 * of the command pool; the driver should be implicated in
@@ -401,9 +406,11 @@
 	 */
 	BUG_ON(pool->users == 0);
 
-	if (!--pool->users) {
-		kmem_cache_destroy(pool->cmd_slab);
+	if (!--pool->users)
 		kmem_cache_destroy(pool->sense_slab);
+	if (!--cmd_pool_users) {
+		kmem_cache_destroy(scsi_cmd_slab);
+		scsi_cmd_slab = NULL;
 	}
 	mutex_unlock(&host_cmd_pool_mutex);
 }
@@ -488,9 +495,6 @@
 int scsi_setup_command_freelist(struct Scsi_Host *shost)
 {
 	struct scsi_cmnd *cmd;
-	gfp_t gfp_mask = GFP_KERNEL;
-	if (shost->unchecked_isa_dma || shost->sense_buffer_isa)
-		gfp_mask = GFP_DMA;
 
 	spin_lock_init(&shost->free_list_lock);
 	INIT_LIST_HEAD(&shost->free_list);
@@ -503,7 +507,7 @@
 	/*
 	 * Get one backup command for this host.
 	 */
-	cmd = scsi_host_alloc_command(shost, gfp_mask);
+	cmd = scsi_host_alloc_command(shost, 0);
 	if (!cmd) {
 		scsi_put_host_cmd_pool(shost);
 		shost->cmd_pool = NULL;
author
#40 lore raw

PATCH 20/21 Finally kill unchecked_isa_dma

In reply to Andi Kleen

Now that all users are gone it can be safely completely removed.

---
 Documentation/scsi/scsi_mid_low_api.txt |    3 ---
 drivers/scsi/hosts.c                    |    1 -
 include/scsi/scsi_host.h                |    6 ------
 3 files changed, 10 deletions(-)
Documentation/scsi/scsi_mid_low_api.txt +0 -3
Index: linux/Documentation/scsi/scsi_mid_low_api.txt
===================================================================
@@ -1254,9 +1254,6 @@
     cmd_per_lun  - maximum number of commands that can be queued on devices
                    controlled by the host. Overridden by LLD calls to
                    scsi_adjust_queue_depth().
-    unchecked_isa_dma - 1=>only use bottom 16 MB of ram (ISA DMA addressing
-                   restriction), 0=>can use full 32 bit (or better) DMA
-                   address space
     use_clustering - 1=>SCSI commands in mid level's queue can be merged,
                      0=>disallow SCSI command merging
     hostt        - pointer to driver's struct scsi_host_template from which
drivers/scsi/hosts.c +0 -1
Index: linux/drivers/scsi/hosts.c
===================================================================
@@ -352,7 +352,6 @@
 	shost->can_queue = sht->can_queue;
 	shost->sg_tablesize = sht->sg_tablesize;
 	shost->cmd_per_lun = sht->cmd_per_lun;
-	shost->unchecked_isa_dma = sht->unchecked_isa_dma;
 	shost->use_clustering = sht->use_clustering;
 	shost->ordered_tag = sht->ordered_tag;
 
include/scsi/scsi_host.h +0 -6
Index: linux/include/scsi/scsi_host.h
===================================================================
@@ -417,11 +417,6 @@
 	unsigned supported_mode:2;
 
 	/*
-	 * True if this host adapter uses unchecked DMA onto an ISA bus.
-	 */
-	unsigned unchecked_isa_dma:1;
-
-	/*
 	 * True if this host adapter can make good use of clustering.
 	 * I originally thought that if the tablesize was large that it
 	 * was a waste of CPU cycles to prepare a cluster list, but
@@ -590,7 +585,6 @@
 	unsigned long cmd_serial_number;
 	
 	unsigned active_mode:2;
-	unsigned unchecked_isa_dma:1;
 	unsigned use_clustering:1;
 	unsigned use_blk_tcq:1;
 
author
#41 lore raw

PATCH 21/21 Convert DMA buffers in ch.c to allocate via the block layer

In reply to Andi Kleen

Instead of specifying GFP_DMA always ask the block layer to get some dma'able memory

---
 drivers/scsi/ch.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
drivers/scsi/ch.c +10 -6
Index: linux/drivers/scsi/ch.c
===================================================================
@@ -232,7 +232,8 @@
 	u_char  *buffer;
 	int     result;
 
-	buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
+	buffer = get_pages_mask(GFP_KERNEL, 512,
+				blk_q_mask(ch->device->request_queue));
 	if(!buffer)
 		return -ENOMEM;
 
@@ -262,7 +263,7 @@
 		}
 		dprintk("READ ELEMENT STATUS for element 0x%x failed\n",elem);
 	}
-	kfree(buffer);
+	free_pages_mask(buffer, 512);
 	return result;
 }
 
@@ -289,9 +290,11 @@
 	int     result,id,lun,i;
 	u_int   elem;
 
-	buffer = kzalloc(512, GFP_KERNEL | GFP_DMA);
+	buffer = get_pages_mask(GFP_KERNEL, 512,
+				blk_q_mask(ch->device->request_queue));
 	if (!buffer)
 		return -ENOMEM;
+	memset(buffer, 0, 512);
 
 	memset(cmd,0,sizeof(cmd));
 	cmd[0] = MODE_SENSE;
@@ -402,7 +405,7 @@
 		}
 	}
 	ch->voltags = 1;
-	kfree(buffer);
+	free_pages_mask(buffer, 512);
 
 	return 0;
 }
@@ -737,7 +740,8 @@
 			return -EINVAL;
 		elem = ch->firsts[cge.cge_type] + cge.cge_unit;
 
-		buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
+		buffer = get_pages_mask(GFP_KERNEL, 512,
+					blk_q_mask(ch->device->request_queue));
 		if (!buffer)
 			return -ENOMEM;
 		mutex_lock(&ch->lock);
@@ -791,7 +795,7 @@
 			vprintk("device has no volume tag support\n");
 			goto voltag_retry;
 		}
-		kfree(buffer);
+		free_pages_mask(buffer, 512);
 		mutex_unlock(&ch->lock);
 
 		if (copy_to_user(argp, &cge, sizeof (cge)))