Remove isa_unchecked_dma and some more GFP_DMAs in the mid layer v3
Subsystems (14)
cover Remove isa_unchecked_dma and some more GFP_DMAs in the mid layer v3
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
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.25rc4
These are a lot of patches. I can set up a git tree if that makes merging easier. Please let me now if I should do that.
-Andi
PATCH 1/20 Add sense_buffer_isa to host template
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.
include/scsi/scsi_host.h +8 -0
@@ -453,6 +453,11 @@ struct scsi_host_template { 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;
@@ -615,6 +620,9 @@ struct Scsi_Host {*/ 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
@@ -1268,6 +1268,7 @@ of interest: 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
@@ -342,6 +342,7 @@ struct Scsi_Host *scsi_host_alloc(struct shost->use_clustering = sht->use_clustering; shost->ordered_tag = sht->ordered_tag; shost->active_mode = sht->supported_mode; + shost->sense_buffer_isa = sht->sense_buffer_isa;if (sht->supported_mode ==MODE_UNKNOWN )/* means we didn't set it ... default to INITIATOR */
drivers/scsi/scsi.c +2 -1
@@ -322,7 +322,8 @@ int scsi_setup_command_freelist(struct S* yet existent. */ mutex_lock (&host_cmd_pool_mutex); - pool = (shost->unchecked_isa_dma ? &scsi_cmd_dma_pool : &scsi_cmd_pool); + pool = ((shost->unchecked_isa_dma || shost->sense_buffer_isa) ? + &scsi_cmd_dma_pool : &scsi_cmd_pool);if (!pool->users) { pool->cmd_slab = kmem_cache_create(pool->cmd_name, sizeof(struct scsi_cmnd), 0,
PATCH 2/20 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
@@ -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 int BusLogic_ProbeInfoCount;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 void BusLogic_AnnounceDriver(strustatic 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; }
@@ -1607,9 +1620,6 @@ static bool __init BusLogic_ReadHostAdapBIOS_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;/*
@@ -2128,7 +2138,9 @@ static void __init BusLogic_InitializeHo 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; }
@@ -2142,7 +2154,7 @@ static void __init BusLogic_InitializeHo*/ 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];
@@ -2167,6 +2179,35 @@ static int BusLogic_SlaveConfigure(strucreturn 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
@@ -2267,12 +2308,12 @@ static int __init BusLogic_init(void)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;
@@ -2318,7 +2359,7 @@ static int __init BusLogic_init(void)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,
@@ -2332,7 +2373,7 @@ static int __init BusLogic_init(void)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);
@@ -2351,7 +2392,7 @@ static int __init BusLogic_init(void)BusLogic_DestroyCCBs (HostAdapter);BusLogic_ReleaseResources (HostAdapter);list_del (&HostAdapter->host_list); -scsi_host_put (Host); +buslogic_free_host (Host); ret = -ENODEV ; } }
@@ -2395,7 +2436,7 @@ static int __exit BusLogic_ReleaseHostAd*/ list_del (&HostAdapter->host_list); -scsi_host_put (Host); +buslogic_free_host (Host);return 0 ; }
@@ -2783,7 +2824,7 @@ static bool BusLogic_WriteOutgoingMailbostatic 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];
@@ -2805,7 +2846,7 @@ static int BusLogic_host_reset(struct scstatic 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;
@@ -2998,7 +3039,7 @@ static int BusLogic_QueueCommand(structstatic 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 ;
@@ -3128,7 +3169,7 @@ static int BusLogic_ResetHostAdapter(strstatic 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 */ ) {
@@ -3199,7 +3240,7 @@ static int BusLogic_BIOSDiskParameters(sstatic 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;
@@ -3572,9 +3613,9 @@ static struct scsi_host_template Bus_Log#if 0 .eh_abort_handler = BusLogic_AbortCommand, #endif - .unchecked_isa_dma = 1, .max_sectors = 128, .use_clustering = ENABLE_CLUSTERING, + .slave_alloc = buslogic_adjust_queue, };/*
PATCH 3/20 Remove unchecked_isa_dma in advansys.c
In reply to Andi KleenThat 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 | 231 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 166 insertions(+), 65 deletions(-)
drivers/scsi/advansys.c +103 -59
@@ -2212,7 +2212,7 @@ do { \#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 @@ struct asc_stats {/* * 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 @@ struct asc_board {#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 @@ struct asc_board {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_adv_dvc_cfg(ADV_DVC_*/ 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 @@ static void asc_prt_scsi_host(struct Scsprintk (" 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 void * advansys_srb_to_ptr(structstatic 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_line(char *buf, int b*/ 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_board_devices(struct*/ 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_get_eeprom_string(ushort*/ 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_asc_board_eeprom(stru*/ 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_adv_board_eeprom(stru*/ 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 @@ static int asc_prt_driver_conf(struct Sc 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_driver_conf(struct Sc*/ 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_asc_board_info(struct*/ 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 @@ asc_proc_copy(off_t advoffset, off_t off*/ 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 @@ static int 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 @@ static void adv_isr_callback(ADV_DVC_VARASC_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 @@ static void asc_isr_callback(ASC_DVC_VARASC_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 AscISR(ASC_DVC_VAR *asc_dvc)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 @@ static int 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 @@ advansys_biosparam(struct scsi_device *sstatic 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 @@ advansys_narrow_slave_configure(struct sASC_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 @@ advansys_wide_slave_configure(struct scs*/ 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 AdvExeScsiQueue(ADV_DVC_VAR *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 ushort __devinit AscInitFromEEP(Astatic 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 AscInitGetConfig(ststatic 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 AdvInitFrom38C1600Estatic 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 @@ AdvInitGetConfig(struct pci_dev *pdev, s }#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 @@ static struct scsi_host_template advansy .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 struct scsi_host_template advansystatic 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 @@ static int __devinit advansys_board_foun 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 @@ static int __devinit advansys_board_foun }#endif /* CONFIG_PROC_FS */ + ret =0 ; +if (ASC_NARROW_BOARD (boardp)) {/* * Set the board bus type and PCI IRQ before
@@ -13472,28 +13485,23 @@ static int __devinit advansys_board_founswitch (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 @@ static int __devinit advansys_board_foun* 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 @@ static int __devinit advansys_board_founreturn 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 __devinit advansys_board_foun*/ 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 @@ static int advansys_release(struct Scsi_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 11static PortAddr _asc_def_iop_base[ASC_IOADR_TABLE_MAX_IX ] = {
@@ -13954,11 +13993,13 @@ static int __devinit advansys_isa_probe( 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 @@ static int __devinit advansys_isa_probe(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 @@ static int __devinit advansys_vlb_probe( 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 @@ static int __devinit advansys_vlb_probe(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 @@ static int __devinit advansys_eisa_probe 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 @@ advansys_pci_probe(struct pci_dev *pdev, 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 @@ advansys_pci_probe(struct pci_dev *pdev,return 0 ; free_host: -scsi_host_put (shost); +adv_free_host (shost); release_region:pci_release_regions (pdev); disable_device:
PATCH 4/20 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
@@ -138,6 +138,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);
@@ -490,7 +499,7 @@ static void gdth_scsi_done(struct scsi_cint __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;struct scatterlist one_sg;
@@ -3933,7 +3942,7 @@ static const char *gdth_ctr_name(gdth_hastatic 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);
@@ -3941,7 +3950,7 @@ static const char *gdth_info(struct Scsistatic 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;
@@ -3994,7 +4003,7 @@ static int gdth_eh_bus_reset(Scsi_Cmnd *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;
@@ -4023,7 +4032,7 @@ static int gdth_bios_param(struct scsi_dstatic 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 ]));
@@ -4717,6 +4726,13 @@ static int gdth_slave_configure(struct sreturn 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,
@@ -4730,10 +4746,38 @@ static struct scsi_host_template gdth_te .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) {
@@ -4745,10 +4789,10 @@ static int __init gdth_isa_probe_one(uloif (!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))
@@ -4772,7 +4816,7 @@ static int __init gdth_isa_probe_one(uloset_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;
@@ -4860,7 +4904,7 @@ static int __init gdth_isa_probe_one(ulo out_free_irq:free_irq (ha->irq, ha); out_host_put: -scsi_host_put (shp); +gdth_free_host (shp); return error; }#endif /* CONFIG_ISA */
@@ -4876,10 +4920,11 @@ static int __init gdth_eisa_probe_one(usif (!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))
@@ -4895,7 +4940,6 @@ static int __init gdth_eisa_probe_one(us goto out_host_put; } - shp->unchecked_isa_dma =0 ; shp->irq = ha->irq; shp->dma_channel =0xff ;
@@ -4992,7 +5036,7 @@ static int __init gdth_eisa_probe_one(usfree_irq (ha->irq, ha); gdth_ctr_count--; out_host_put: -scsi_host_put (shp); +gdth_free_host (shp); return error; }#endif /* CONFIG_EISA */
@@ -5005,10 +5049,10 @@ static int __init gdth_pci_probe_one(gdtdma_addr_t scratch_dma_handle =0 ;int error, i; - 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 (&pcistr[ctr],ha))
@@ -5027,7 +5071,6 @@ static int __init gdth_pci_probe_one(gdt goto out_host_put; } - shp->unchecked_isa_dma =0 ; shp->irq = ha->irq; shp->dma_channel =0xff ;
@@ -5129,7 +5172,7 @@ static int __init gdth_pci_probe_one(gdtfree_irq (ha->irq, ha); gdth_ctr_count--; out_host_put: -scsi_host_put (shp); +gdth_free_host (shp); return error; }#endif /* CONFIG_PCI */
@@ -5171,7 +5214,7 @@ static void gdth_remove_one(gdth_ha_strpci_unmap_single (ha->pdev,ha->ccb_phys,sizeof (gdth_cmd_str),PCI_DMA_BIDIRECTIONAL ); -scsi_host_put (shp); +gdth_free_host (shp); }static int __init gdth_init (void )
PATCH 5/20 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
@@ -512,6 +512,13 @@ static int eata2x_bios_param(struct scsi 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 @@ static struct scsi_host_template driver_ .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 hostdata {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 @@ static int port_detect(unsigned long por#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 @@ static int port_detect(unsigned long por 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 @@ static int port_detect(unsigned long por 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 @@ static int port_detect(unsigned long porfor (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 @@ static int eata2x_queuecommand(struct sc 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_queuecommand(struct scstatic 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 @@ static int eata2x_eh_host_reset(struct sint 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 @@ static irqreturn_t ihdlr(int irq, structunsigned 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);if (shost->irq != irq) panic("%s: ihdlr, irq %d, shost->irq %d.\n", ha->board_name, irq,
@@ -2555,7 +2577,7 @@ static irqreturn_t do_interrupt_handler(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++)
@@ -2575,6 +2597,8 @@ static int eata2x_release(struct Scsi_Hofree_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 ; }
PATCH 6/20 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
@@ -151,7 +151,13 @@ struct aha1542_hostdata {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 struct Scsi_Host *aha_host[7 ];/* One for each IRQ level (9-15) */
@@ -1132,23 +1138,28 @@ static int __init aha1542_detect(struct }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 */
@@ -1265,6 +1276,8 @@ fail:continue ; unregister:release_region (bases[indx],4 ); +dma_free_coherent (NULL ,sizeof (struct aha1542_hostdata ), +HOSTDATA (shpnt),HOSTDMA (shpnt));scsi_unregister (shpnt);continue ;
@@ -1281,6 +1294,8 @@ static int aha1542_release(struct Scsi_Hfree_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 ; }
@@ -1752,6 +1767,11 @@ static int aha1542_biosparam(struct scsi }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" ,
@@ -1767,7 +1787,7 @@ static struct scsi_host_template driver_ .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"
PATCH 7/20 Remove unchecked_isa_dma in aha152x/wd7000/sym53c416/u14-34f/NCR53c406a
In reply to Andi KleenI 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
@@ -551,6 +551,10 @@ struct aha152x_hostdata {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 @@ struct aha152x_scdata {/* 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 @@ static irqreturn_t swintr(int irqno, voistruct 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 @@ struct Scsi_Host *aha152x_probe_one(stru 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 @@ void aha152x_release(struct Scsi_Host *s#endif list_del (&HOSTDATA (shpnt)->host_list); +dma_free_coherent (NULL ,sizeof (struct aha152x_hostdata ), +HOSTDATA (shpnt),HOSTDMA (shpnt));scsi_host_put (shpnt); }
@@ -3456,7 +3477,7 @@ static int aha152x_proc_info(struct Scsistatic 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
@@ -1045,6 +1045,12 @@ static void __init calc_port_addr(void)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 @@ static struct scsi_host_template driver_ .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
@@ -825,6 +825,12 @@ module_param_array(sym53c416_3, uint, NU#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 @@ static struct scsi_host_template driver_ .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
@@ -350,6 +350,12 @@ static void log_ultrastor_abort(struct u }#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 @@ static int ultrastor_14f_detect(struct s 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 @@ static int ultrastor_24f_detect(struct s 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 @@ static struct scsi_host_template driver_ .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"
PATCH 9/20 Add blk_kmalloc/blk_alloc_pages
In reply to Andi KleenWhen a user is doing IO in the kernel and wants to avoid bouncing it is best to just ask the block layer to allocate the memory for it. This patch adds two simple wrappers: blk_kmalloc and blk_alloc_pages and respective free functions to do this.
blk_alloc_pages is a little unusual in that it takes size instead of order arguments -- i did this because I have later patches to convert it over to a new allocator which does not require power of two for pages.
Needed for followup patches
--- block/blk-settings.c | 13 +++++++++++++ include/linux/blkdev.h | 12 ++++++++++++ 2 files changed, 25 insertions(+)
include/linux/blkdev.h +13 -0
@@ -555,6 +555,19 @@ static inline void blk_queue_bounce(stru }#endif /* CONFIG_MMU */ +extern struct page *blk_alloc_pages (struct request_queue *q,gfp_t gfp, +int size); +extern void *blk_kmalloc (struct request_queue *q,unsigned size,gfp_t gfp); +static inline void blk_free_pages (struct page *p,int size) +{ +__free_pages (p,get_order (size)); +} + +static inline void blk_kfree (void *p,int size) +{ +kfree (p); +} +struct req_iterator {int i;struct bio *bio;
block/blk-settings.c +13 -0
@@ -156,6 +156,19 @@ void blk_queue_bounce_limit(struct reque }EXPORT_SYMBOL (blk_queue_bounce_limit); +struct page *blk_alloc_pages (struct request_queue *q, +gfp_t gfp,int size) +{ +return alloc_pages ((q->bounce_gfp & ~GFP_NOIO ) | gfp,get_order (size)); +} +EXPORT_SYMBOL (blk_alloc_pages); + +void *blk_kmalloc (struct request_queue *q,unsigned size,gfp_t gfp) +{ +return kmalloc (size, gfp | (q->bounce_gfp & ~GFP_NOIO )); +} +EXPORT_SYMBOL (blk_kmalloc); +/** * blk_queue_max_sectors - set max sectors for a request for this queue * @q: the request queue for the device
PATCH 8/20 Remove random noop unchecked_isa_dma users
In reply to Andi KleenLots of drivers set it to 0. Remove that. Patch should be a nop.
--- drivers/scsi/arm/acornscsi.c | 1 - drivers/scsi/arm/cumana_1.c | 1 - drivers/scsi/dc395x.c | 1 - drivers/scsi/eata_pio.c | 2 -- drivers/scsi/hptiop.c | 1 - drivers/scsi/ips.c | 1 - drivers/scsi/mac_scsi.c | 1 - drivers/scsi/scsi_debug.c | 1 - 8 files changed, 9 deletions(-)
drivers/scsi/arm/acornscsi.c +0 -1
@@ -2983,7 +2983,6 @@ static struct scsi_host_template acornsc .this_id = 7, .sg_tablesize = SG_ALL, .cmd_per_lun = 2, - .unchecked_isa_dma = 0, .use_clustering = DISABLE_CLUSTERING, .proc_name = "acornscsi", };
drivers/scsi/arm/cumana_1.c +0 -1
@@ -222,7 +222,6 @@ static struct scsi_host_template cumanas .this_id = 7, .sg_tablesize = SG_ALL, .cmd_per_lun = 2, - .unchecked_isa_dma = 0, .use_clustering = DISABLE_CLUSTERING, .proc_name = "CumanaSCSI-1", };
drivers/scsi/dc395x.c +0 -1
@@ -4761,7 +4761,6 @@ static struct scsi_host_template dc395x_ .cmd_per_lun = DC395x_MAX_CMD_PER_LUN, .eh_abort_handler = dc395x_eh_abort, .eh_bus_reset_handler = dc395x_eh_bus_reset, - .unchecked_isa_dma = 0, .use_clustering = DISABLE_CLUSTERING, };
drivers/scsi/eata_pio.c +0 -2
@@ -815,8 +815,6 @@ static int register_pio_HBA(long base, selse hd->primary =1 ; - sh->unchecked_isa_dma =0 ;/* We can only do PIO */ - hd->next =NULL ;/* build a linked list of all HBAs */ hd->prev = last_HBA;if (hd->prev !=NULL )
drivers/scsi/hptiop.c +0 -1
@@ -903,7 +903,6 @@ static struct scsi_host_template driver_ .eh_device_reset_handler = hptiop_reset, .eh_bus_reset_handler = hptiop_reset, .info = hptiop_info, - .unchecked_isa_dma = 0, .emulated = 0, .use_clustering = ENABLE_CLUSTERING, .proc_name = driver_name,
drivers/scsi/ips.c +0 -1
@@ -6842,7 +6842,6 @@ ips_register_scsi(int index) sh->sg_tablesize = sh->hostt->sg_tablesize; sh->can_queue = sh->hostt->can_queue; sh->cmd_per_lun = sh->hostt->cmd_per_lun; - sh->unchecked_isa_dma = sh->hostt->unchecked_isa_dma; sh->use_clustering = sh->hostt->use_clustering; sh->max_sectors =128 ;
drivers/scsi/mac_scsi.c +0 -1
@@ -592,7 +592,6 @@ static struct scsi_host_template driver_ .this_id = 7, .sg_tablesize = SG_ALL, .cmd_per_lun = CMD_PER_LUN, - .unchecked_isa_dma = 0, .use_clustering = DISABLE_CLUSTERING };
drivers/scsi/scsi_debug.c +0 -1
@@ -221,7 +221,6 @@ static struct scsi_host_template sdebug_ .sg_tablesize = 256, .cmd_per_lun = 16, .max_sectors = 0xffff, - .unchecked_isa_dma = 0, .use_clustering = DISABLE_CLUSTERING, .module = THIS_MODULE, };
PATCH 11/20 Remove unchecked_isa_dma support for hostdata
In reply to Andi KleenAll 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
@@ -297,9 +297,6 @@ struct Scsi_Host *scsi_host_alloc(structgfp_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 ;
PATCH 12/20 Remove unchecked_isa_dma checks in sg.c
In reply to Andi KleenThey are only used to allocate data buffers, and those are bounced by the block layer anyways.
--- drivers/scsi/sg.c | 47 ++++++++++++----------------------------------- 1 file changed, 12 insertions(+), 35 deletions(-)
drivers/scsi/sg.c +12 -35
@@ -150,7 +150,6 @@ typedef struct sg_fd { /* holds the staSg_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 */
@@ -195,7 +194,7 @@ static void sg_remove_scat(Sg_scatter_hostatic void sg_build_reserve (Sg_fd * sfp,int req_size);static void sg_link_reserve (Sg_fd * sfp,Sg_request * srp,int size);static void sg_unlink_reserve (Sg_fd * sfp,Sg_request * srp); -static struct page *sg_page_malloc (int rqSz,int lowDma,int *retSzp); +static struct page *sg_page_malloc (struct scsi_device *dev,int rqSz,int *retSzp);static void sg_page_free (struct page *page,int size);static Sg_fd *sg_add_sfp (Sg_device * sdp,int dev);static int sg_remove_sfp (Sg_device * sdp,Sg_fd * sfp);
@@ -844,8 +843,7 @@ sg_ioctl(struct inode *inode, struct filif (result)return result;if (val) { - sfp->low_dma =1 ; -if ((0 == sfp->low_dma) && (0 ==sg_res_in_use (sfp))) { +if (0 ==sg_res_in_use (sfp)) { val = (int ) sfp->reserve.bufflen;sg_remove_scat (&sfp->reserve);sg_build_reserve (sfp, val);
@@ -853,11 +851,10 @@ sg_ioctl(struct inode *inode, struct fil }else {if (sdp->detached)return -ENODEV ; - sfp->low_dma = sdp->device->host->unchecked_isa_dma; }return 0 ;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 ;
@@ -1622,8 +1619,7 @@ sg_start_req(Sg_request * srp)if ((dxfer_len <=0 ) || (dxfer_dir ==SG_DXFER_NONE ))return 0 ;if (sg_allow_dio && (hp->flags &SG_FLAG_DIRECT_IO ) && - (dxfer_dir !=SG_DXFER_UNKNOWN ) && (0 == hp->iovec_count) && - (!sfp->parentdp->device->host->unchecked_isa_dma)) { + (dxfer_dir !=SG_DXFER_UNKNOWN ) && (0 == hp->iovec_count)) { res =sg_build_direct (srp, sfp, dxfer_len);if (res <=0 )/* -ve -> error, 0 -> done, 1 -> try indirect */ return res;
@@ -1660,14 +1656,6 @@ sg_build_sgat(Sg_scatter_hold * schp, coint sg_bufflen = tablesize *sizeof (struct scatterlist );gfp_t gfp_flags =GFP_ATOMIC | __GFP_NOWARN; -/* -* TODO: test without low_dma, we should not need it since -* the block layer will bounce the buffer for us -* -* XXX(hch): we shouldn't need GFP_DMA for the actual S/G list. -*/ -if (sfp->low_dma) - gfp_flags |=GFP_DMA ; schp->buffer =kzalloc (sg_bufflen, gfp_flags);if (!schp->buffer)return -ENOMEM ;
@@ -1860,7 +1848,7 @@ sg_build_indirect(Sg_scatter_hold * schp num = (rem_sz > scatter_elem_sz_prev) ? scatter_elem_sz_prev : rem_sz; - p =sg_page_malloc (num, sfp->low_dma, &ret_sz); + p =sg_page_malloc (sfp->parentdp->device, num, &ret_sz);if (!p)return -ENOMEM ;
@@ -2345,8 +2333,6 @@ sg_add_sfp(Sg_device * sdp, int dev) 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;
@@ -2456,7 +2442,7 @@ sg_res_in_use(Sg_fd * sfp)/* The size fetched (value output via retSzp) set when non-NULL return */ static struct page * -sg_page_malloc (int rqSz,int lowDma,int *retSzp) +sg_page_malloc (struct scsi_device *dev,int rqSz,int *retSzp) {struct page *resp =NULL ;gfp_t page_mask;
@@ -2466,15 +2452,12 @@ sg_page_malloc(int rqSz, int lowDma, intif ((rqSz <=0 ) || (NULL == retSzp))return resp; -if (lowDma) - page_mask =GFP_ATOMIC |GFP_DMA | __GFP_COMP | __GFP_NOWARN; -else - page_mask =GFP_ATOMIC | __GFP_COMP | __GFP_NOWARN; + page_mask =GFP_ATOMIC | __GFP_COMP | __GFP_NOWARN;for (order =0 , a_size =PAGE_SIZE ; a_size < rqSz; order++, a_size <<=1 ) ; resSz = a_size;/* rounded up if necessary */ - resp =alloc_pages (page_mask, order); + resp =blk_alloc_pages (dev->request_queue, page_mask, resSz);while ((!resp) && order) { --order; a_size >>=1 ;/* divide by 2, until PAGE_SIZE */
@@ -2492,13 +2475,8 @@ sg_page_malloc(int rqSz, int lowDma, intstatic void sg_page_free (struct page *page,int size) { -int order, a_size; - -if (!page) -return ; -for (order =0 , a_size =PAGE_SIZE ; a_size < size; - order++, a_size <<=1 ) ; -__free_pages (page, order); +if (page) +blk_free_pages (page, size); }#ifndef MAINTENANCE_IN_CMD
@@ -2869,11 +2847,10 @@ static void sg_proc_debug_helper(structfor (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);
PATCH 13/20 Use blk_kmalloc in scsi_scan
In reply to Andi KleenAsk the block layer to get dmaable memory for IO that does not require bouncing.
Removes unchecked_isa_dma references -- this goes through the bounce_gfp now
--- drivers/scsi/scsi_scan.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
drivers/scsi/scsi_scan.c +6 -7
@@ -1010,8 +1010,7 @@ static int scsi_probe_and_add_lun(structif (!sdev) goto out; - result =kmalloc (result_len,GFP_ATOMIC | - ((shost->unchecked_isa_dma) ? __GFP_DMA :0 )); + result =blk_kmalloc (sdev->request_queue, result_len,GFP_ATOMIC );if (!result) goto out_free_sdev;
@@ -1091,7 +1090,7 @@ static int scsi_probe_and_add_lun(struct } out_free_result: -kfree (result); +blk_kfree (result, result_len); out_free_sdev:if (res ==SCSI_SCAN_LUN_PRESENT ) {if (sdevp) {
@@ -1278,7 +1277,7 @@ static int scsi_report_lun_scan(struct s {char devname[64 ];unsigned char scsi_cmd[MAX_COMMAND_SIZE ]; -unsigned int length; +unsigned int length, olength;unsigned int lun;unsigned int num_luns;unsigned int retries;
@@ -1328,8 +1327,8 @@ static int scsi_report_lun_scan(struct s* prevent us from finding any LUNs on this target. */ length = (max_scsi_report_luns +1 ) *sizeof (struct scsi_lun ); - lun_data =kmalloc (length,GFP_ATOMIC | - (sdev->host->unchecked_isa_dma ? __GFP_DMA :0 )); + olength = length; + lun_data =blk_kmalloc (sdev->request_queue, length,GFP_ATOMIC );if (!lun_data) {printk (ALLOC_FAILURE_MSG , __FUNCTION__); goto out;
@@ -1457,7 +1456,7 @@ static int scsi_report_lun_scan(struct s } out_err: -kfree (lun_data); +blk_kfree (lun_data, olength); out:scsi_device_put (sdev);if (sdev->sdev_state ==SDEV_CREATED )
PATCH 14/20 Don't disable direct_io for unchecked_isa_dma in st.c
In reply to Andi KleenBlock layer bounces anyways.
--- drivers/scsi/st.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/st.c +1 -1
@@ -3990,7 +3990,7 @@ static int st_probe(struct device *dev) 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]);
PATCH 15/20 Remove automatic block layer bouncing for unchecked_isa_dma
In reply to Andi KleenAll 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
@@ -1547,8 +1547,6 @@ u64 scsi_calculate_bounce_limit(struct Sstruct 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.
PATCH 16/20 Convert sr driver over the blk_kmalloc
In reply to Andi KleenThis replaces references to unchecked_isa_dma. Instead just let the block layer allocate dma able memory for it.
--- drivers/scsi/sr.c | 12 ++++++------ drivers/scsi/sr_ioctl.c | 22 +++++++++------------- drivers/scsi/sr_vendor.c | 8 ++++---- 3 files changed, 19 insertions(+), 23 deletions(-)
drivers/scsi/sr_ioctl.c +9 -13
@@ -30,11 +30,6 @@ static int xa_test = 0;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 @@ static int sr_read_tochdr(struct cdrom_dint result;unsigned char *buffer; - buffer =kmalloc (32 ,GFP_KERNEL |SR_GFP_DMA (cd)); + buffer =blk_kmalloc (cd->device->request_queue,32 ,GFP_KERNEL );if (!buffer)return -ENOMEM ;
@@ -61,7 +56,7 @@ static int sr_read_tochdr(struct cdrom_d tochdr->cdth_trk0 = buffer[2 ]; tochdr->cdth_trk1 = buffer[3 ]; -kfree (buffer); +blk_kfree (buffer,32 );return result; }
@@ -73,7 +68,7 @@ static int sr_read_tocentry(struct cdromint result;unsigned char *buffer; - buffer =kmalloc (32 ,GFP_KERNEL |SR_GFP_DMA (cd)); + buffer =blk_kmalloc (cd->device->request_queue,32 ,GFP_KERNEL );if (!buffer)return -ENOMEM ;
@@ -100,7 +95,7 @@ static int sr_read_tocentry(struct cdrom tocentry->cdte_addr.lba = (((((buffer[8 ] <<8 ) + buffer[9 ]) <<8 ) + buffer[10 ]) <<8 ) + buffer[11 ]; -kfree (buffer); +blk_kfree (buffer,32 );return result; }
@@ -385,7 +380,8 @@ int sr_get_mcn(struct cdrom_device_info {Scsi_CD *cd = cdi->handle;struct packet_command cgc; -char *buffer =kmalloc (32 ,GFP_KERNEL |SR_GFP_DMA (cd)); +struct scsi_device *dev = cd->device; +char *buffer =blk_kmalloc (dev->request_queue,32 ,GFP_KERNEL );int result;if (!buffer)
@@ -405,7 +401,7 @@ int sr_get_mcn(struct cdrom_device_infomemcpy (mcn->medium_catalog_number, buffer +9 ,13 ); mcn->medium_catalog_number[13 ] =0 ; -kfree (buffer); +blk_kfree (buffer,32 );return result; }
@@ -564,7 +560,7 @@ int sr_is_xa(Scsi_CD *cd)if (!xa_test)return 0 ; - raw_sector =kmalloc (2048 ,GFP_KERNEL |SR_GFP_DMA (cd)); + raw_sector =blk_kmalloc (cd->device->request_queue,2048 ,GFP_KERNEL );if (!raw_sector)return -ENOMEM ; if (0 == sr_read_sector(cd, cd->ms_offset + 16,
@@ -574,7 +570,7 @@ int sr_is_xa(Scsi_CD *cd)/* read a raw sector failed for some reason. */ is_xa =-1 ; } -kfree (raw_sector); +blk_kfree (raw_sector,2048 );#ifdef DEBUG printk ("%s: sr_is_xa: %d\n" , cd->cdi.name, is_xa);#endif
drivers/scsi/sr.c +6 -6
@@ -674,7 +674,8 @@ static void get_sectorsize(struct scsi_cint sector_size;struct request_queue *queue; - buffer =kmalloc (512 ,GFP_KERNEL |GFP_DMA ); + queue = cd->device->request_queue; + buffer =blk_kmalloc (queue,512 ,GFP_KERNEL );if (!buffer) goto Enomem;
@@ -739,10 +740,9 @@ static void get_sectorsize(struct scsi_cset_capacity (cd->disk, cd->capacity); } - queue = cd->device->request_queue;blk_queue_hardsect_size (queue, sector_size); out: -kfree (buffer); +blk_kfree (buffer,512 );return ; Enomem:
@@ -772,7 +772,7 @@ static void get_capabilities(struct scsi/* allocate transfer buffer */ - buffer =kmalloc (512 ,GFP_KERNEL |GFP_DMA ); + buffer =blk_kmalloc (cd->device->request_queue,512 ,GFP_KERNEL );if (!buffer) {printk (KERN_ERR "sr: out of memory.\n" );return ;
@@ -792,7 +792,7 @@ static void get_capabilities(struct scsiCDC_DVD |CDC_DVD_RAM |CDC_SELECT_DISC |CDC_SELECT_SPEED | CDC_MRW | CDC_MRW_W | CDC_RAM); -kfree (buffer); +blk_kfree (buffer,512 );printk ("%s: scsi-1 drive\n" , cd->cdi.name);return ; }
@@ -851,7 +851,7 @@ static void get_capabilities(struct scsi cd->device->writeable =1 ; } -kfree (buffer); +blk_kfree (buffer,512 ); }/*
drivers/scsi/sr_vendor.c +4 -4
@@ -117,7 +117,7 @@ int sr_set_blocklength(Scsi_CD *cd, int density = (blocklength >2048 ) ?0x81 :0x83 ;#endif - buffer =kmalloc (512 ,GFP_KERNEL |GFP_DMA ); + buffer =blk_kmalloc (cd->device->request_queue,512 ,GFP_KERNEL );if (!buffer)return -ENOMEM ;
@@ -146,7 +146,7 @@ int sr_set_blocklength(Scsi_CD *cd, intprintk ("%s: switching blocklength to %d bytes failed\n" , cd->cdi.name, blocklength);#endif -kfree (buffer); +blk_kfree (buffer,512 );return rc; }
@@ -164,7 +164,7 @@ int sr_cd_check(struct cdrom_device_infoif (cd->cdi.mask &CDC_MULTI_SESSION )return 0 ; - buffer =kmalloc (512 ,GFP_KERNEL |GFP_DMA ); + buffer =blk_kmalloc (cd->device->request_queue,512 ,GFP_KERNEL );if (!buffer)return -ENOMEM ;
@@ -323,6 +323,6 @@ int sr_cd_check(struct cdrom_device_infoprintk (KERN_DEBUG "%s: multisession offset=%lu\n" , cd->cdi.name, sector);#endif -kfree (buffer); +blk_kfree (buffer,512 );return rc; }
PATCH 17/20 Remove unchecked_isa_dma from sysfs
In reply to Andi KleenI 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
@@ -237,7 +237,6 @@ shost_rd_attr(host_busy, "%hu\n");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_attr2 (proc_name, hostt->proc_name,"%s\n" );static struct class_device_attribute *scsi_sysfs_shost_attrs[] = {
@@ -246,7 +245,6 @@ static struct class_device_attribute *sc &class_device_attr_cmd_per_lun, &class_device_attr_can_queue, &class_device_attr_sg_tablesize, - &class_device_attr_unchecked_isa_dma, &class_device_attr_proc_name, &class_device_attr_scan, &class_device_attr_state,
PATCH 18/20 Switch to a single SCSI command pool
In reply to Andi KleenNow 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 | 47 ++++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 23 deletions(-)
drivers/scsi/scsi.c +26 -26
@@ -140,24 +140,22 @@ const char * scsi_device_type(unsigned tEXPORT_SYMBOL (scsi_device_type); +static struct kmem_cache *scsi_cmd_slab; +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,
@@ -178,10 +176,8 @@ struct scsi_cmnd *__scsi_get_command(strstruct scsi_cmnd *cmd;unsigned char *buf; - cmd =kmem_cache_alloc (shost->cmd_pool->cmd_slab, - gfp_mask | shost->cmd_pool->gfp_mask); - -if (unlikely (!cmd)) { + cmd =kmem_cache_alloc (scsi_cmd_slab, gfp_mask); +if (!cmd) {unsigned long flags;spin_lock_irqsave (&shost->free_list_lock, flags);
@@ -204,7 +200,7 @@ struct scsi_cmnd *__scsi_get_command(strmemset (cmd,0 ,sizeof (*cmd)); cmd->sense_buffer = buf; }else { -kmem_cache_free (shost->cmd_pool->cmd_slab, cmd); +kmem_cache_free (scsi_cmd_slab, cmd); cmd =NULL ; } }
@@ -269,7 +265,7 @@ void __scsi_put_command(struct Scsi_Hostif (likely (cmd !=NULL )) {kmem_cache_free (shost->cmd_pool->sense_slab, cmd->sense_buffer); -kmem_cache_free (shost->cmd_pool->cmd_slab, cmd); +kmem_cache_free (scsi_cmd_slab, cmd); }put_device (dev);
@@ -322,20 +318,23 @@ int scsi_setup_command_freelist(struct S* yet existent. */ mutex_lock (&host_cmd_pool_mutex); - pool = ((shost->unchecked_isa_dma || shost->sense_buffer_isa) ? - &scsi_cmd_dma_pool : &scsi_cmd_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) +if (!scsi_cmd_slab) { + scsi_cmd_slab =kmem_cache_create ("scsi_cmd_cache" , +sizeof (struct scsi_cmnd ),0 , +SLAB_HWCACHE_ALIGN ,NULL ); +if (!scsi_cmd_slab) goto fail; + } + pool = shost->sense_buffer_isa ? &scsi_sense_dma_pool : + &scsi_sense_pool; +if (!pool->users) { 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 ; goto fail; } }
@@ -347,8 +346,7 @@ int scsi_setup_command_freelist(struct S/* * Get one backup command for this host. */ - cmd =kmem_cache_alloc (shost->cmd_pool->cmd_slab, -GFP_KERNEL | shost->cmd_pool->gfp_mask); + cmd =kmem_cache_alloc (scsi_cmd_slab,GFP_KERNEL );if (!cmd) goto fail2;
@@ -363,10 +361,11 @@ int scsi_setup_command_freelist(struct S fail2:if (cmd) -kmem_cache_free (shost->cmd_pool->cmd_slab, cmd); +kmem_cache_free (scsi_cmd_slab, cmd);mutex_lock (&host_cmd_pool_mutex);if (!--pool->users) { -kmem_cache_destroy (pool->cmd_slab); +kmem_cache_destroy (scsi_cmd_slab); + scsi_cmd_slab =NULL ;kmem_cache_destroy (pool->sense_slab); } fail:
@@ -387,12 +386,13 @@ void scsi_destroy_command_freelist(struclist_del_init (&cmd->list);kmem_cache_free (shost->cmd_pool->sense_slab, cmd->sense_buffer); -kmem_cache_free (shost->cmd_pool->cmd_slab, cmd); +kmem_cache_free (scsi_cmd_slab, cmd); }mutex_lock (&host_cmd_pool_mutex);if (!--shost->cmd_pool->users) { -kmem_cache_destroy (shost->cmd_pool->cmd_slab); +kmem_cache_destroy (scsi_cmd_slab); + scsi_cmd_slab =NULL ;kmem_cache_destroy (shost->cmd_pool->sense_slab); }mutex_unlock (&host_cmd_pool_mutex);
PATCH 19/20 Finally kill unchecked_isa_dma
In reply to Andi KleenNow 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
@@ -1254,9 +1254,6 @@ of interest: 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
@@ -335,7 +335,6 @@ struct Scsi_Host *scsi_host_alloc(struct 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; shost->active_mode = sht->supported_mode;
include/scsi/scsi_host.h +0 -6
@@ -423,11 +423,6 @@ struct scsi_host_template { 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
@@ -601,7 +596,6 @@ struct Scsi_Host {unsigned long cmd_serial_number; unsigned active_mode:2; - unsigned unchecked_isa_dma:1; unsigned use_clustering:1; unsigned use_blk_tcq:1;
PATCH 20/20 Convert DMA buffers in ch.c to allocate via the block layer
In reply to Andi KleenInstead of specifying GFP_DMA always ask the block layer to get some dma'able memory
--- drivers/scsi/ch.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
drivers/scsi/ch.c +7 -6
@@ -231,7 +231,7 @@ ch_read_element_status(scsi_changer *ch,u_char *buffer;int result; - buffer =kmalloc (512 ,GFP_KERNEL |GFP_DMA ); + buffer =blk_kmalloc (ch->device->request_queue,512 ,GFP_KERNEL );if (!buffer)return -ENOMEM ;
@@ -261,7 +261,7 @@ ch_read_element_status(scsi_changer *ch, }dprintk ("READ ELEMENT STATUS for element 0x%x failed\n" ,elem); } -kfree (buffer); +blk_kfree (buffer,512 );return result; }
@@ -288,9 +288,10 @@ ch_readconfig(scsi_changer *ch)int result,id,lun,i;u_int elem; - buffer =kzalloc (512 ,GFP_KERNEL |GFP_DMA ); + buffer =blk_kmalloc (ch->device->request_queue,512 ,GFP_KERNEL );if (!buffer)return -ENOMEM ; +memset (buffer,0 ,512 );memset (cmd,0 ,sizeof (cmd)); cmd[0 ] =MODE_SENSE ;
@@ -401,7 +402,7 @@ ch_readconfig(scsi_changer *ch) } } ch->voltags =1 ; -kfree (buffer); +blk_kfree (buffer,512 );return 0 ; }
@@ -733,7 +734,7 @@ static long ch_ioctl(struct file *file,return -EINVAL ; elem = ch->firsts[cge.cge_type] + cge.cge_unit; - buffer =kmalloc (512 ,GFP_KERNEL |GFP_DMA ); + buffer =blk_kmalloc (ch->device->request_queue,512 ,GFP_KERNEL );if (!buffer)return -ENOMEM ;mutex_lock (&ch->lock);
@@ -786,7 +787,7 @@ static long ch_ioctl(struct file *file,vprintk ("device has no volume tag support\n" ); goto voltag_retry; } -kfree (buffer); +blk_kfree (buffer,512 );mutex_unlock (&ch->lock);if (copy_to_user (argp, &cge,sizeof (cge)))
Quoted 41 lines · Andi Kleen
v2: various cleanups, use
dma_alloc_coherentinstead of nakedGFP_DMA
add blk_* allocators to avoid bouncing in SCSI scan
v3: Remove cmnd/sense buffer bouncing in advansys.
Replacesense_buffer_maskwith a single bitOriginal description:
This patchkit fixes all existing drivers that used
isa_unchecked_dmato not need that anymore. I have some upcoming infrastructure changes for DMA memory management andisa_unchecked_dmawas in the way.Enabling
isa_unchecked_dmahad several effects:
- All incomingscsi_cmndswere inGFP_DMAmemory.
Only one driver relied on that actually (advansys), the others all accessed thescsi_cmndsonly with the CPU.
- scsi hostdata is allocated withGFP_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_bufferis allocated withGFP_DMA. That was also commonly required and not easy to fix so I created a separate host template field that enablessense_bufferbouncing.Also while I was it I removed also a lot of
GFP_DMAsin 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.
Quoted 6 lines
Patches against 2.6.25rc4
These are a lot of patches. I can set up a git tree if that makes merging easier. Please let me now if I should do that.
-Andi
Andi hi.
First let me start by saying that your "remove of ISA cruft from scsi land" patchset was like the smell of flowers on that first spring morning.
It perfectly fits with two parallel efforts I'm perusing.
- First is with regard to scsi_cmnd->cmnd cleanup:
http://www.spinics.net/lists/linux-scsi/msg23676.html
You have just done my job where I needed to audit all uses of scsi_cmnd->cmnd with regard to the isa_unchecked_dma. (see: http://www.spinics.net/lists/linux-scsi/msg23747.html)
- Second, with my sense buffer effort:
(http://www.spinics.net/lists/linux-scsi/msg23231.html)
The second effort might help in also eliminating the use of the new .sense_buffer_isa by streamlining the use of the sense_buffer to use the same bounce buffer mechanism used for regular IO. Though farther cleaning what you have already started.
(I will have some questions regarding this later)
The "scan-and-change of all scsi LLDS that..." is a very unrewarding and sure to encounter-hidden-ghosts effort. As I should know freshly. Reviewing few of the drivers in this patchset I've spotted some minor problems. Mainly two things right now. For instance aha152x.c is a file used for both ISA and a PCMCIA devices, your change unnecessarily restricts the PCMCIA device. That driver, as well as other drivers in the patchset, are currently allocated low by the isa_unchecked_dma, but from a closer inspection, none of the host_data is actually used for DMA and the allocation can stay embedded. I think it is worth it to do that now in one go. One more thing is the EISA devices that are now pushed to behave like ISA because they do not have a device, like gdth, For that particular driver Jeff Garzik has a patch in Q that might help. I will try to rebase your patch on top of his series.
I will put all your patches on a local git tree and try to stare at them deeper. Do you have these (And all the dma patches sent to lkml) on a public git tree somewhere? I would like to study and test them.
(The ftp url you specified in lkml returns an alphabetical order of patches and I'm not sure which subdirectory has the latest set)
Thanks for doing this and the all effort looks commendable.
Boaz
Quoted 8 lines · Boaz Harrosh
It perfectly fits with two parallel efforts I'm perusing.
- First is with regard to
scsi_cmnd->cmndcleanup:
http://www.spinics.net/lists/linux-scsi/msg23676.htmlYou have just done my job where I needed to audit all uses of
scsi_cmnd->cmndwith regard to theisa_unchecked_dma. (see: http://www.spinics.net/lists/linux-scsi/msg23747.html)
Some reauditing would be probably still a good idea, just to make sure i didn't miss anything. Some of the code was rather unclean and not always obvious too.
Quoted 5 lines
The "scan-and-change of all scsi LLDS that..." is a very unrewarding and sure to encounter-hidden-ghosts effort. As I should know freshly. Reviewing few of the drivers in this patchset I've spotted some minor problems. Mainly two things right now. For instance
aha152x.cis a file used for both ISA and a PCMCIA devices, your change unnecessarily restricts the PCMCIA device. That driver, as
But it was already restricted before wasn't it?
I'm definitely not trying to tune or improve these drivers in any way, just remove a crufty bit of infrastructure.
Quoted 3 lines
well as other drivers in the patchset, are currently allocated low by the
isa_unchecked_dma, but from a closer inspection, none of thehost_datais actually used for DMA and the allocation can stay embedded. I think it is worth it to do
Hmm, I thought i had audited them for needing or not needing dma on hostdata and only converted the ones who needed it, but I might have err'ed on the side of caution in some cases I wasn't 100% sure.
Even if the hostdata is allocated unnecessarily as dma it is only a quite small amount of memory so it shouldn't really hurt.
Quoted 2 lines
that now in one go. One more thing is the EISA devices that are now pushed to behave like ISA because they do not have a device, like gdth, For that particular
Yes, but they already behaved before like that. No change. I was especially careful to not add unnecessary data bouncing for that case.
Quoted 6 lines
driver Jeff Garzik has a patch in Q that might help. I will try to rebase your patch on top of his series.
I will put all your patches on a local git tree and try to stare at them deeper. Do you have these (And all the dma patches sent to lkml) on a public git tree somewhere? I would like to study and test them.
I have them in quilt on the ftp server. Actually
it contains some more, it is just the first part of larger work I'm doing to rework DMA memory allocation. The SCSI parts are up to "scsi-ch-dma" in the series.
I used to have a recipe to turn quilt into git, but it doesn't seem to work anymore with latest git (or maybe I forgot it).
I can try to reconstruct that again later if you really need it.
Quoted 2 lines
(The ftp url you specified in lkml returns an alphabetical order of patches and I'm not sure which subdirectory has the latest set)
It's a series of patches with the "series" file defining the order, aka quilt format.
The usual way for quilt is to get quilt get the patches dir ln -s it into the source tree and then use quilt push -a to apply them Or if you only want the SCSI stuff only go upto "scsi-ch-dma"
Or you could probably write some simple scripts to apply it based on the series. It's essentially just
grep -v '#' patches/series | while read i ; do patch -p1 < patches/$i ; done
Thanks for your help.
-Andi
James Bottomley
maintainerRe: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
In reply to Andi KleenQuoted 9 lines · Andi Kleen
When a user is doing IO in the kernel and wants to avoid bouncing it is best to just ask the block layer to allocate the memory for it. This patch adds two simple wrappers:
blk_kmallocandblk_alloc_pagesand respective free functions to do this.
blk_alloc_pagesis a little unusual in that it takes size instead of order arguments -- i did this because I have later patches to convert it over to a new allocator which does not require power of two for pages.
I really don't like this ... it's wedging something in the block layer that shouldn't be there just to avoid doing it properly in terms of allocations on the device dma_mask.
I also think the kfree takes a length part is asking for trouble because it's pretty fragile.
However, if Jens will ack it, I'll (reluctantly) add it.
James
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
Quoted 11 lines · James Bottomley
Quoted 9 lines · Andi Kleen
When a user is doing IO in the kernel and wants to avoid bouncing it is best to just ask the block layer to allocate the memory for it. This patch adds two simple wrappers:
blk_kmallocandblk_alloc_pagesand respective free functions to do this.
blk_alloc_pagesis a little unusual in that it takes size instead of order arguments -- i did this because I have later patches to convert it over to a new allocator which does not require power of two for pages.
Quoted 8 lines
I really don't like this ... it's wedging something in the block layer that shouldn't be there just to avoid doing it properly in terms of allocations on the device
dma_mask.I also think the kfree takes a length part is asking for trouble because it's pretty fragile.
However, if Jens will ack it, I'll (reluctantly) add it.
I agree with you, I don't like it at all (for a variety of reasons). Since callers need to remember size for free anyway (I've always hated such interfaces), they may as well do their own allocations. I don't think the block abstraction buys us anything.
If anything, add a generic allocator helper that you can pass a mask to instead.
Signature · 1 line
Jens Axboe
Re: [PATCH] [14/20] Don't disable direct_io for unchecked_isa_dma in st.c
In reply to Andi KleenQuoted 2 lines · Andi Kleen
Block layer bounces anyways.
Are you sure? Seems to me that st builds its own list with get_user_pages() for direct io, where will that get bounced? The block layer will only bounce things that are mapped directly, so if st used blk_rq_map_kern() and inserted that request in the queue, we could proceed with killing that check in st.
Signature · 1 line
Jens Axboe
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
In reply to Jens AxboeQuoted 16 lines · Jens Axboe
Quoted 15 lines · James Bottomley
Quoted 9 lines · Andi Kleen
When a user is doing IO in the kernel and wants to avoid bouncing it is best to just ask the block layer to allocate the memory for it. This patch adds two simple wrappers:
blk_kmallocandblk_alloc_pagesand respective free functions to do this.
blk_alloc_pagesis a little unusual in that it takes size instead of order arguments -- i did this because I have later patches to convert it over to a new allocator which does not require power of two for pages.I really don't like this ... it's wedging something in the block layer that shouldn't be there just to avoid doing it properly in terms of allocations on the device
dma_mask.
Quoted 6 lines
Quoted 4 lines
I also think the kfree takes a length part is asking for trouble because it's pretty fragile.
However, if Jens will ack it, I'll (reluctantly) add it.
I agree with you, I don't like it at all (for a variety of reasons).
For what reasons exactly?
In my original patchkit I didn't have blk_kmalloc etc. but just relied on the block layer bouncing everything for ISA as needed, but James thought it was important that SCSI LUN scan etc do not bounce for ISA. That is when I came up with these helpers.
You think it should go back to always bouncing? That would be fine for me too. With that they wouldn't be needed.
I think the only issue where it might be a problem would be with sg.c where it might be a performance issue.
Quoted 7 lines
Since callers need to remember size for free anyway (I've always hated such interfaces), they may as well do their own allocations. I don't think the block abstraction buys us anything.
If anything, add a generic allocator helper that you can pass a mask to instead.
The reason I added the size is that the mask allocator (not in the SCSI specific patchkit, but I posted that to l-k later) needs a size to free because it is based on a similar design as free_pages which always needed size. Also at least for the few users I converted (there are not really that many) the size is either constant anyways or very easy to remember (please take a look at the concrete patches before judging it)
The other problem is that there is no mask, except for the bounce mask. These old cruft ISA drivers don't have a device (and before anybody asks again -- no i don't plan to rewrite them all to be device based); but they do have correct bounce_pfn which has all the needed information. I don't think it would make sense to add another field for this.
That is why I ended up with the helpers
Of course if you don't want the helpers it could be something like
get_pages_mask(GFP_KERNEL, size, blk_q_mask(request_Queue))
written out but why not have simple helpers that allows this a little nicer?
The nice property about the helper is that it also avoids a circular depency cycle in patchkits.
-Andi
Re: [PATCH] [14/20] Don't disable direct_io for unchecked_isa_dma in st.c
In reply to Jens AxboeQuoted 5 lines · Jens Axboe
Are you sure? Seems to me that st builds its own list with
get_user_pages()for direct io, where will that get bounced? The block layer will only bounce things that are mapped directly, so if st usedblk_rq_map_kern()and inserted that request in the queue, we could proceed with killing that check in st.
Which shouldn't be all too difficult and would be the right thing to do. We really need someone to sit down and convert st/osst/sg to use the proper block layer helpers.
Re: [PATCH] [14/20] Don't disable direct_io for unchecked_isa_dma in st.c
Quoted 11 lines · Christoph Hellwig
Quoted 5 lines · Jens Axboe
Are you sure? Seems to me that st builds its own list with
get_user_pages()for direct io, where will that get bounced? The block layer will only bounce things that are mapped directly, so if st usedblk_rq_map_kern()and inserted that request in the queue, we could proceed with killing that check in st.Which shouldn't be all too difficult and would be the right thing to do. We really need someone to sit down and convert st/osst/sg to use the proper block layer helpers.
First they do. They all call scsi_execute_async which at the end produces a BIO and then calls blk_execute_nowait(). and all is swell.
Second some one did, long a go, and have submitted them rebased more then 3 times. I'd say their time is do long a go. (I mean Mike Christie)
Mike, please, do you have these patches on a public git somewhere I want to help push them.
Boaz
Re: [PATCH] [14/20] Don't disable direct_io for unchecked_isa_dma in st.c
Quoted 8 lines · Boaz Harrosh
Quoted 7 lines · Christoph Hellwig
Quoted 5 lines · Jens Axboe
Are you sure? Seems to me that st builds its own list with
get_user_pages()for direct io, where will that get bounced? The block layer will only bounce things that are mapped directly, so if st usedblk_rq_map_kern()and inserted that request in the queue, we could proceed with killing that check in st.
Quoted 7 lines
Quoted 4 lines
Which shouldn't be all too difficult and would be the right thing to do. We really need someone to sit down and convert st/osst/sg to use the proper block layer helpers.
First they do. They all call
scsi_execute_asyncwhich at the end produces a BIO and then callsblk_execute_nowait(). and all is swell.
I thought i had audited them all too, but wasn't 100% sure anymore. But thanks for double checking.
Jens, that answers your earlier question.
-Andi
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
In reply to Andi KleenQuoted 17 lines · Andi Kleen
Quoted 16 lines · Jens Axboe
Quoted 15 lines · James Bottomley
Quoted 9 lines · Andi Kleen
When a user is doing IO in the kernel and wants to avoid bouncing it is best to just ask the block layer to allocate the memory for it. This patch adds two simple wrappers:
blk_kmallocandblk_alloc_pagesand respective free functions to do this.
blk_alloc_pagesis a little unusual in that it takes size instead of order arguments -- i did this because I have later patches to convert it over to a new allocator which does not require power of two for pages.I really don't like this ... it's wedging something in the block layer that shouldn't be there just to avoid doing it properly in terms of allocations on the device
dma_mask.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
I also think the kfree takes a length part is asking for trouble because it's pretty fragile.
However, if Jens will ack it, I'll (reluctantly) add it.
I agree with you, I don't like it at all (for a variety of reasons).
For what reasons exactly?
Mainly the one I list below. Using the interface for allocation is actually more involved that just doing it yourself, which is not a sign of a good abstraction.
Quoted 7 lines
In my original patchkit I didn't have
blk_kmallocetc. but just relied on the block layer bouncing everything for ISA as needed, but James thought it was important that SCSI LUN scan etc do not bounce for ISA. That is when I came up with these helpers.You think it should go back to always bouncing? That would be fine for me too. With that they wouldn't be needed.
I don't see a reason for not bouncing for scanning - James?
Quoted 24 lines
I think the only issue where it might be a problem would be with
sg.cwhere it might be a performance issue.Quoted 7 lines
Since callers need to remember size for free anyway (I've always hated such interfaces), they may as well do their own allocations. I don't think the block abstraction buys us anything.
If anything, add a generic allocator helper that you can pass a mask to instead.
The reason I added the size is that the mask allocator (not in the SCSI specific patchkit, but I posted that to l-k later) needs a size to free because it is based on a similar design as
free_pageswhich always needed size. Also at least for the few users I converted (there are not really that many) the size is either constant anyways or very easy to remember (please take a look at the concrete patches before judging it)The other problem is that there is no mask, except for the bounce mask. These old cruft ISA drivers don't have a device (and before anybody asks again -- no i don't plan to rewrite them all to be device based); but they do have correct
bounce_pfnwhich has all the needed information. I don't think it would make sense to add another field for this.
Quoted 7 lines
That is why I ended up with the helpers
Of course if you don't want the helpers it could be something like
get_pages_mask(GFP_KERNEL, size, blk_q_mask(request_Queue))written out but why not have simple helpers that allows this a little nicer?
I'd greatly prefer that, a helper that converts the dma mask to a suitable GFP for the architecture. The benefit is that all allocators takes this mask, and it still leaves the driver free to do whatever it wants to allocate memory.
Signature · 1 line
Jens Axboe
Re: [PATCH] [14/20] Don't disable direct_io for unchecked_isa_dma in st.c
In reply to Christoph HellwigQuoted 10 lines · Christoph Hellwig
Quoted 5 lines · Jens Axboe
Are you sure? Seems to me that st builds its own list with
get_user_pages()for direct io, where will that get bounced? The block layer will only bounce things that are mapped directly, so if st usedblk_rq_map_kern()and inserted that request in the queue, we could proceed with killing that check in st.Which shouldn't be all too difficult and would be the right thing to do. We really need someone to sit down and convert st/osst/sg to use the proper block layer helpers.
Right, I'm just saying that it currently will not work!
Signature · 1 line
Jens Axboe
Re: [PATCH] [14/20] Don't disable direct_io for unchecked_isa_dma in st.c
In reply to Andi KleenQuoted 14 lines · Andi Kleen
Quoted 13 lines · Boaz Harrosh
Quoted 11 lines · Christoph Hellwig
Quoted 5 lines · Jens Axboe
Are you sure? Seems to me that st builds its own list with
get_user_pages()for direct io, where will that get bounced? The block layer will only bounce things that are mapped directly, so if st usedblk_rq_map_kern()and inserted that request in the queue, we could proceed with killing that check in st.Which shouldn't be all too difficult and would be the right thing to do. We really need someone to sit down and convert st/osst/sg to use the proper block layer helpers.
Quoted 7 lines
Quoted 2 lines
First they do. They all call
scsi_execute_asyncwhich at the end produces a BIO and then callsblk_execute_nowait(). and all is swell.I thought i had audited them all too, but wasn't 100% sure anymore. But thanks for double checking.
Jens, that answers your earlier question.
OK good, I didn't realize that that path was already done. No problem with this part, then.
Signature · 1 line
Jens Axboe
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
In reply to Jens AxboeQuoted 23 lines · Jens Axboe
Quoted 22 lines · Andi Kleen
Quoted 21 lines · Jens Axboe
Quoted 19 lines · James Bottomley
Quoted 9 lines · Andi Kleen
When a user is doing IO in the kernel and wants to avoid bouncing it is best to just ask the block layer to allocate the memory for it. This patch adds two simple wrappers:
blk_kmallocandblk_alloc_pagesand respective free functions to do this.
blk_alloc_pagesis a little unusual in that it takes size instead of order arguments -- i did this because I have later patches to convert it over to a new allocator which does not require power of two for pages.I really don't like this ... it's wedging something in the block layer that shouldn't be there just to avoid doing it properly in terms of allocations on the device
dma_mask.I also think the kfree takes a length part is asking for trouble because it's pretty fragile.
However, if Jens will ack it, I'll (reluctantly) add it.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
I agree with you, I don't like it at all (for a variety of reasons).
For what reasons exactly?
Mainly the one I list below. Using the interface for allocation is actually more involved that just doing it yourself, which is not a sign of a good abstraction.
hmm, missed the point sorry. How is using the interface more involved? It has to get a mask or a pfn from somewhere and without the interface that's actually more code to write out.
Quoted 17 lines
Quoted 17 lines
The reason I added the size is that the mask allocator (not in the SCSI specific patchkit, but I posted that to l-k later) needs a size to free because it is based on a similar design as
free_pageswhich always needed size. Also at least for the few users I converted (there are not really that many) the size is either constant anyways or very easy to remember (please take a look at the concrete patches before judging it)The other problem is that there is no mask, except for the bounce mask. These old cruft ISA drivers don't have a device (and before anybody asks again -- no i don't plan to rewrite them all to be device based); but they do have correct
bounce_pfnwhich has all the needed information. I don't think it would make sense to add another field for this.That is why I ended up with the helpers
Of course if you don't want the helpers it could be something like
Quoted 7 lines
Quoted 3 lines
get_pages_mask(GFP_KERNEL, size, blk_q_mask(request_Queue))written out but why not have simple helpers that allows this a little nicer?
I'd greatly prefer that, a helper that converts the dma mask to a suitable GFP for the architecture. The benefit is that all allocators takes this mask, and it still leaves the driver free to do whatever it
These are not really used by drivers (except libata)
Quoted 1 line
wants to allocate memory.
Well it already exists, it is just called blk_q_mask() and is added later with the mask allocator block layer code and was used there internally. I can move that out into SCSI if it's really needed.
Short term it would be a little problematic because it would turn the patch dependencies around. Currently the early SCSI patchkit is nicely independent from the mask allocator and it would be somewhat messy to add it later. If you really it is important I can do it, but if it's only a slight preference or something it would avoid me quite some work to not do that.
Really to be honest I cannot see all that much difference between blk_kmalloc(q, ...) and get_pages_mask(..., blk_q_mask(q))
-Andi
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
Quoted 28 lines · Andi Kleen
Quoted 27 lines · Jens Axboe
Quoted 25 lines · Andi Kleen
Quoted 16 lines · Jens Axboe
Quoted 15 lines · James Bottomley
Quoted 9 lines · Andi Kleen
When a user is doing IO in the kernel and wants to avoid bouncing it is best to just ask the block layer to allocate the memory for it. This patch adds two simple wrappers:
blk_kmallocandblk_alloc_pagesand respective free functions to do this.
blk_alloc_pagesis a little unusual in that it takes size instead of order arguments -- i did this because I have later patches to convert it over to a new allocator which does not require power of two for pages.I really don't like this ... it's wedging something in the block layer that shouldn't be there just to avoid doing it properly in terms of allocations on the device
dma_mask.Quoted 6 lines
Quoted 4 lines
I also think the kfree takes a length part is asking for trouble because it's pretty fragile.
However, if Jens will ack it, I'll (reluctantly) add it.
I agree with you, I don't like it at all (for a variety of reasons).
For what reasons exactly?
Quoted 7 lines
Quoted 3 lines
Mainly the one I list below. Using the interface for allocation is actually more involved that just doing it yourself, which is not a sign of a good abstraction.
hmm, missed the point sorry. How is using the interface more involved? It has to get a mask or a pfn from somewhere and without the interface that's actually more code to write out.
Because it requires tracking length, for instance. Your interface just doesn't add anything useful, it's not a good abstraction/api imho.
Quoted 25 lines
Quoted 17 lines
Quoted 17 lines
The reason I added the size is that the mask allocator (not in the SCSI specific patchkit, but I posted that to l-k later) needs a size to free because it is based on a similar design as
free_pageswhich always needed size. Also at least for the few users I converted (there are not really that many) the size is either constant anyways or very easy to remember (please take a look at the concrete patches before judging it)The other problem is that there is no mask, except for the bounce mask. These old cruft ISA drivers don't have a device (and before anybody asks again -- no i don't plan to rewrite them all to be device based); but they do have correct
bounce_pfnwhich has all the needed information. I don't think it would make sense to add another field for this.That is why I ended up with the helpers
Of course if you don't want the helpers it could be something like
Quoted 7 lines
Quoted 3 lines
get_pages_mask(GFP_KERNEL, size, blk_q_mask(request_Queue))written out but why not have simple helpers that allows this a little nicer?
I'd greatly prefer that, a helper that converts the dma mask to a suitable GFP for the architecture. The benefit is that all allocators takes this mask, and it still leaves the driver free to do whatever it
Quoted 7 lines
These are not really used by drivers (except libata)
Quoted 1 line
wants to allocate memory.
Well it already exists, it is just called
blk_q_mask()and is added later with the mask allocator block layer code and was used there internally. I can move that out into SCSI if it's really needed.
Put it in block/ or blkdev.h and call it queue_mask_to_gfp() or something, that would be fine.
Quoted 9 lines
Short term it would be a little problematic because it would turn the patch dependencies around. Currently the early SCSI patchkit is nicely independent from the mask allocator and it would be somewhat messy to add it later. If you really it is important I can do it, but if it's only a slight preference or something it would avoid me quite some work to not do that.
Really to be honest I cannot see all that much difference between
blk_kmalloc(q, ...)andget_pages_mask(..., blk_q_mask(q))
A bit of patch churn is better than getting stuck with blk_free(ptr, size), so I think it's the way to do. I already voiced my opinion on why I don't like the allocator abstraction, so I wont repeat that.
Signature · 1 line
Jens Axboe
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
Quoted 33 lines · Jens Axboe
Quoted 32 lines · Andi Kleen
Quoted 30 lines · Jens Axboe
Quoted 17 lines · Andi Kleen
Quoted 16 lines · Jens Axboe
Quoted 15 lines · James Bottomley
Quoted 9 lines · Andi Kleen
When a user is doing IO in the kernel and wants to avoid bouncing it is best to just ask the block layer to allocate the memory for it. This patch adds two simple wrappers:
blk_kmallocandblk_alloc_pagesand respective free functions to do this.
blk_alloc_pagesis a little unusual in that it takes size instead of order arguments -- i did this because I have later patches to convert it over to a new allocator which does not require power of two for pages.I really don't like this ... it's wedging something in the block layer that shouldn't be there just to avoid doing it properly in terms of allocations on the device
dma_mask.Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
I also think the kfree takes a length part is asking for trouble because it's pretty fragile.
However, if Jens will ack it, I'll (reluctantly) add it.
I agree with you, I don't like it at all (for a variety of reasons).
For what reasons exactly?
Mainly the one I list below. Using the interface for allocation is actually more involved that just doing it yourself, which is not a sign of a good abstraction.
Quoted 5 lines
Quoted 3 lines
hmm, missed the point sorry. How is using the interface more involved? It has to get a mask or a pfn from somewhere and without the interface that's actually more code to write out.
Because it requires tracking length, for instance. Your interface just
While the implementation in the first patch does not require tracking length, the mask allocator version that is only introduced about 15 patches later or so does. That is why i added the length parameter in the first patch already, although it is not used yet.
Sorry for not making that more clear in the changelog
I actually considered tracking length in the struct page in the mask allocator for this, but when I looked at the callers I noticed 90+% of them only used a constant length which does not really need to get tracked. It is always the same. Then there are two or so with a variable length, but it is all local to a single function so it's also not particularly burdensome to track either. Please take a look at the callers and you'll see the same.
Quoted 1 line
doesn't add anything useful,
Well for once it allows easy conversion to the mask alloactor linux world. That is the reason'd'etre of the whole patchkit as far as I'm concerned anyways.
Quoted 1 line
it's not a good abstraction/api imho.
I'm still trying to figure out what exactly you object to.
Is the only problem the length? Would blk_kmalloc/kfree() be ok for you if kfree didn't require a length parameter? It would not be that hard to add one, although frankly I personally don't see the need.
Or alternatively I will just remove it if James can get convinced to back down on his earlier requirement for bounce less scanning.
Quoted 13 lines
Quoted 13 lines
Quoted 10 lines
Quoted 6 lines
Of course if you don't want the helpers it could be something like
get_pages_mask(GFP_KERNEL, size, blk_q_mask(request_Queue))written out but why not have simple helpers that allows this a little nicer?
I'd greatly prefer that, a helper that converts the dma mask to a suitable GFP for the architecture. The benefit is that all allocators takes this mask, and it still leaves the driver free to do whatever it
These are not really used by drivers (except libata)
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
wants to allocate memory.
Well it already exists, it is just called
blk_q_mask()and is added later with the mask allocator block layer code and was used there internally. I can move that out into SCSI if it's really needed.Put it in
block/orblkdev.hand call itqueue_mask_to_gfp()or something, that would be fine.
There is no gfp anymore for this in the mask allocator world!!!! Only masks and a slight abstraction for some cases request queues with bounce pfns.
-Andi
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
Quoted 81 lines · Andi Kleen
Quoted 24 lines · Jens Axboe
Quoted 23 lines · Andi Kleen
Quoted 22 lines · Jens Axboe
Quoted 21 lines · Andi Kleen
Quoted 20 lines · Jens Axboe
Quoted 10 lines · James Bottomley
Quoted 9 lines · Andi Kleen
When a user is doing IO in the kernel and wants to avoid bouncing it is best to just ask the block layer to allocate the memory for it. This patch adds two simple wrappers:
blk_kmallocandblk_alloc_pagesand respective free functions to do this.
blk_alloc_pagesis a little unusual in that it takes size instead of order arguments -- i did this because I have later patches to convert it over to a new allocator which does not require power of two for pages.Quoted 8 lines
I really don't like this ... it's wedging something in the block layer that shouldn't be there just to avoid doing it properly in terms of allocations on the device
dma_mask.I also think the kfree takes a length part is asking for trouble because it's pretty fragile.
However, if Jens will ack it, I'll (reluctantly) add it.
I agree with you, I don't like it at all (for a variety of reasons).
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
For what reasons exactly?
Mainly the one I list below. Using the interface for allocation is actually more involved that just doing it yourself, which is not a sign of a good abstraction.
hmm, missed the point sorry. How is using the interface more involved? It has to get a mask or a pfn from somewhere and without the interface that's actually more code to write out.
Because it requires tracking length, for instance. Your interface just
While the implementation in the first patch does not require tracking length, the mask allocator version that is only introduced about 15 patches later or so does. That is why i added the length parameter in the first patch already, although it is not used yet.
Sorry for not making that more clear in the changelog
I actually considered tracking length in the struct page in the mask allocator for this, but when I looked at the callers I noticed 90+% of them only used a constant length which does not really need to get tracked. It is always the same. Then there are two or so with a variable length, but it is all local to a single function so it's also not particularly burdensome to track either. Please take a look at the callers and you'll see the same.
Quoted 1 line
doesn't add anything useful,
Well for once it allows easy conversion to the mask alloactor linux world. That is the reason'd'etre of the whole patchkit as far as I'm concerned anyways.
Quoted 1 line
it's not a good abstraction/api imho.
I'm still trying to figure out what exactly you object to.
Is the only problem the length? Would blk_kmalloc/kfree() be ok for you if kfree didn't require a length parameter? It would not be that hard to add one, although frankly I personally don't see the need.
Or alternatively I will just remove it if James can get convinced to back down on his earlier requirement for bounce less scanning.
Quoted 14 lines
Quoted 14 lines
Quoted 8 lines
Quoted 5 lines
Of course if you don't want the helpers it could be something like
get_pages_mask(GFP_KERNEL, size, blk_q_mask(request_Queue))written out but why not have simple helpers that allows this a little nicer?
I'd greatly prefer that, a helper that converts the dma mask to a suitable GFP for the architecture. The benefit is that all allocators takes this mask, and it still leaves the driver free to do whatever it
These are not really used by drivers (except libata)
Quoted 1 line
wants to allocate memory.
Well it already exists, it is just called
blk_q_mask()and is added later with the mask allocator block layer code and was used there internally. I can move that out into SCSI if it's really needed.
Quoted 8 lines
Quoted 2 lines
Put it in
block/orblkdev.hand call itqueue_mask_to_gfp()or something, that would be fine.There is no gfp anymore for this in the mask allocator world!!!! Only masks and a slight abstraction for some cases request queues with bounce pfns.
-Andi
Andi, Jens
I would like if we could do a regular allocation and let the block layer bounce it if needed. Please bear in mind that a lot of this code wants to change to use proper BIO and blk_ API as was proposed by Mike Christie. So there is only that one user at scsi_scan where the performance gains are so marginal, comparing the sizes used, to the latency and slowness of the actual IO.
On the other hand it is not only the ISA cases right? Even in an x86_64 system with a PCI-32 host, we are going to bounce. Perhaps if there is a convenient API for allocating un-bounced memory for a block device there will be more users for it, like file-systems.
The API, I would like that if failing to allocate un--bounced memory would then allocate any memory and bounce later.
Boaz
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
Quoted 1 line
I would like if we could do a regular allocation and let the
That is what v1 of the patchkit did. I only introduced blk_* based on feedback from James who was concerned about the additional bounces. It would be fine for me to go back to that state.
Please Jens and James work out what way is the preferred one. Right now there are conflicting requirements from different maintainers.
Quoted 2 lines
this code wants to change to use proper BIO and
blk_API as was proposed by Mike Christie. So there is only that one user atscsi_scan
It's actually not only scsi-scan, but a couple of users (see the whole patchkit)
Quoted 7 lines
On the other hand it is not only the ISA cases right? Even in an
x86_64system with a PCI-32 host, we are going to bounce. Perhaps if there is a convenient API for allocating un-bounced memory for a block device there will be more users for it, like file-systems.The API, I would like that if failing to allocate un--bounced memory would then allocate any memory and bounce later.
pci dma api/ swiotlb is that API (which either bounces or uses an iommu), but it doesn't work for ISA DMA
-Andi
Re: [PATCH] [14/20] Don't disable direct_io for unchecked_isa_dma in st.c
In reply to Christoph HellwigQuoted 10 lines · On Fri, 14 Mar 2008 10:24:12 -0400 Christoph Hellwig <[email protected]> wrote:
Quoted 5 lines · Jens Axboe
Are you sure? Seems to me that st builds its own list with
get_user_pages()for direct io, where will that get bounced? The block layer will only bounce things that are mapped directly, so if st usedblk_rq_map_kern()and inserted that request in the queue, we could proceed with killing that check in st.Which shouldn't be all too difficult and would be the right thing to do. We really need someone to sit down and convert st/osst/sg to use the proper block layer helpers.
I've been working on this (hopefully, I'll submit an updated patchset this week).
I think that the main problem was a lack of motivation for these changes. I bet that nobody reviewed the patchset that Mike posted the last time. Hopefully, the new patchset will be reviewed and merged.
With sg chaining, drain, padding, etc, building a scatterlist is much complicated than in the past. I believe that we should kill home-made scatterlist code and convert the users to use the block layer to avoid potential scatterlist bugs.
Re: [PATCH] [14/20] Don't disable direct_io for unchecked_isa_dma in st.c
Quoted 13 lines · FUJITA Tomonori
Quoted 9 lines · On Fri, 14 Mar 2008 10:24:12 -0400 Christoph Hellwig <[email protected]> wrote:
Quoted 5 lines · Jens Axboe
Are you sure? Seems to me that st builds its own list with
get_user_pages()for direct io, where will that get bounced? The block layer will only bounce things that are mapped directly, so if st usedblk_rq_map_kern()and inserted that request in the queue, we could proceed with killing that check in st.Which shouldn't be all too difficult and would be the right thing to do. We really need someone to sit down and convert st/osst/sg to use the proper block layer helpers.
Quoted 7 lines
I've been working on this (hopefully, I'll submit an updated patchset this week).
I think that the main problem was a lack of motivation for these changes. I bet that nobody reviewed the patchset that Mike posted the last time. Hopefully, the new patchset will be reviewed and merged.
I have ran and tested these patches for a long time, in my personal workstation (long story). I had no problems with them. Burn CDs and all.
Quoted 5 lines
With sg chaining, drain, padding, etc, building a scatterlist is much complicated than in the past. I believe that we should kill home-made scatterlist code and convert the users to use the block layer to avoid potential scatterlist bugs.
--
Definitely, as I remember, Mike's code just prepares BIO's and requests and completely avoids any use of sg-lists. I will be waiting for your patches and will test them here.
Thanks
Boaz
James Bottomley
maintainerRe: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
In reply to Jens AxboeQuoted 9 lines · Jens Axboe
Quoted 7 lines
In my original patchkit I didn't have
blk_kmallocetc. but just relied on the block layer bouncing everything for ISA as needed, but James thought it was important that SCSI LUN scan etc do not bounce for ISA. That is when I came up with these helpers.You think it should go back to always bouncing? That would be fine for me too. With that they wouldn't be needed.
I don't see a reason for not bouncing for scanning - James?
If we didn't know the allocation mask, or it was hard to get, then yes, I'd agree. However, I think that any time the kernel has the device mask available, it should actually use it for an allocation to avoid bouncing.
James
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
In reply to Andi KleenQuoted 55 lines · Andi Kleen
Quoted 33 lines · Jens Axboe
Quoted 32 lines · Andi Kleen
Quoted 30 lines · Jens Axboe
Quoted 17 lines · Andi Kleen
Quoted 16 lines · Jens Axboe
Quoted 15 lines · James Bottomley
Quoted 9 lines · Andi Kleen
When a user is doing IO in the kernel and wants to avoid bouncing it is best to just ask the block layer to allocate the memory for it. This patch adds two simple wrappers:
blk_kmallocandblk_alloc_pagesand respective free functions to do this.
blk_alloc_pagesis a little unusual in that it takes size instead of order arguments -- i did this because I have later patches to convert it over to a new allocator which does not require power of two for pages.I really don't like this ... it's wedging something in the block layer that shouldn't be there just to avoid doing it properly in terms of allocations on the device
dma_mask.Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
I also think the kfree takes a length part is asking for trouble because it's pretty fragile.
However, if Jens will ack it, I'll (reluctantly) add it.
I agree with you, I don't like it at all (for a variety of reasons).
For what reasons exactly?
Mainly the one I list below. Using the interface for allocation is actually more involved that just doing it yourself, which is not a sign of a good abstraction.
Quoted 5 lines
Quoted 3 lines
hmm, missed the point sorry. How is using the interface more involved? It has to get a mask or a pfn from somewhere and without the interface that's actually more code to write out.
Because it requires tracking length, for instance. Your interface just
While the implementation in the first patch does not require tracking length, the mask allocator version that is only introduced about 15 patches later or so does. That is why i added the length parameter in the first patch already, although it is not used yet.
Sorry for not making that more clear in the changelog
I actually considered tracking length in the struct page in the mask allocator for this, but when I looked at the callers I noticed 90+% of them only used a constant length which does not really need to get tracked. It is always the same. Then there are two or so with a variable length, but it is all local to a single function so it's also not particularly burdensome to track either. Please take a look at the callers and you'll see the same.
Quoted 5 lines
Quoted 1 line
doesn't add anything useful,
Well for once it allows easy conversion to the mask alloactor linux world. That is the reason'd'etre of the whole patchkit as far as I'm concerned anyways.
The queue_to_gfp_mask() would make that conversion equally simple.
Quoted 8 lines
Quoted 1 line
it's not a good abstraction/api imho.
I'm still trying to figure out what exactly you object to.
Is the only problem the length? Would blk_kmalloc/kfree() be ok for you if kfree didn't require a length parameter? It would not be that hard to add one, although frankly I personally don't see the need.
Well both - as mentioned, the queue_to_gfp_mask() is cleaner I think. And I just don't think it's a good API to begin with, to me it's inventing something that doesn't really buy you anything. With an exported mask function, you can pass that to any allocator.
Quoted 2 lines
Or alternatively I will just remove it if James can get convinced to back down on his earlier requirement for bounce less scanning.
Personally, for scanning I could not care less. It may actually be a good idea, as bouncing would get tested more so we're sure it doesn't get broken ;-)
Quoted 6 lines
Quoted 2 lines
Put it in
block/orblkdev.hand call itqueue_mask_to_gfp()or something, that would be fine.There is no gfp anymore for this in the mask allocator world!!!! Only masks and a slight abstraction for some cases request queues with bounce pfns.
Then name it queue_bounce_mask() or something, principle is the same (export mask vs export silly api for allocation/free).
Signature · 1 line
Jens Axboe
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
Quoted 1 line
The
queue_to_gfp_mask()would make that conversion equally simple.
Except that there is no gfp for this in the mask allocator world. If I wanted to stay with GFPs i would have never attempted any of this in the first place.
-Andi
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
Quoted 5 lines · Andi Kleen
Quoted 1 line
The
queue_to_gfp_mask()would make that conversion equally simple.Except that there is no gfp for this in the mask allocator world. If I wanted to stay with GFPs i would have never attempted any of this in the first place.
Look further down in the email, queue_bounce_to_mask() or whatever you would want to call it. As also written there, the PRINCIPLE is the same. And that is that exporting a to_allocator_mask() helper is a lot saner than exporting an allocator api tied to the queue.
Can we get over this, please?
Signature · 1 line
Jens Axboe
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
Quoted 6 lines
Look further down in the email,
queue_bounce_to_mask()or whatever you would want to call it. As also written there, the PRINCIPLE is the same. And that is that exporting ato_allocator_mask()helper is a lot saner than exporting an allocator api tied to the queue.Can we get over this, please?
Ok it would have helped if you had explained why it is saner, but I bow to your superior experience on block layer issues.
The only open issue is right now if it isn't better to go back for automatic bouncing for SCSI scan and the other users. Do you have an opinion on that too? If you have one can you please convince James of it too.
-Andi
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
Quoted 9 lines · Andi Kleen
Quoted 6 lines
Look further down in the email,
queue_bounce_to_mask()or whatever you would want to call it. As also written there, the PRINCIPLE is the same. And that is that exporting ato_allocator_mask()helper is a lot saner than exporting an allocator api tied to the queue.Can we get over this, please?
Ok it would have helped if you had explained why it is saner, but I bow to your superior experience on block layer issues.
Do I detect just a touch of sarcasm there? Andi, I have in (approx) 4 emails explained why I think it is saner. It's even right there, above your own text here. I'm not sure how much more I can say...
Quoted 4 lines
The only open issue is right now if it isn't better to go back for automatic bouncing for SCSI scan and the other users. Do you have an opinion on that too? If you have one can you please convince James of it too.
Sure, I also wrote that in several emails - I'm fine with bouncing for the scanning, it's not a big deal imho. I can see why James may not like it so much since we can fairly easily avoid the bounce, but as I wrote in the previous email, I think that doing a bit of bounce there may not be such a bad idea after all.
Signature · 1 line
Jens Axboe
Re: [PATCH] [14/20] Don't disable direct_io for unchecked_isa_dma in st.c
In reply to Boaz HarroshQuoted 12 lines · Boaz Harrosh
Quoted 10 lines · Christoph Hellwig
Quoted 5 lines · Jens Axboe
Are you sure? Seems to me that st builds its own list with
get_user_pages()for direct io, where will that get bounced? The block layer will only bounce things that are mapped directly, so if st usedblk_rq_map_kern()and inserted that request in the queue, we could proceed with killing that check in st.Which shouldn't be all too difficult and would be the right thing to do. We really need someone to sit down and convert st/osst/sg to use the proper block layer helpers.
Quoted 9 lines
First they do. They all call
scsi_execute_asyncwhich at the end produces a BIO and then callsblk_execute_nowait(). and all is swell.Second some one did, long a go, and have submitted them rebased more then 3 times. I'd say their time is do long a go. (I mean Mike Christie)
Mike, please, do you have these patches on a public git somewhere I want to help push them.
Sorry I have been so bad with my linux-scsi email. Here is my current patches:
http://kernel.org/pub/linux/kernel/people/mnc/sg/sg-bioset17/
They are very broken (patch -p1 wise), because of all the block layer changes. I got around to converting st. It is not tested. I just need to convert osst and we can kill scsi_execute_async.
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
In reply to Andi KleenQuoted 7 lines · On Mon, 17 Mar 2008 22:34:22 +0100 Andi Kleen <[email protected]> wrote:
Quoted 6 lines
Look further down in the email,
queue_bounce_to_mask()or whatever you would want to call it. As also written there, the PRINCIPLE is the same. And that is that exporting ato_allocator_mask()helper is a lot saner than exporting an allocator api tied to the queue.Can we get over this, please?
Quoted 7 lines
Ok it would have helped if you had explained why it is saner, but I bow to your superior experience on block layer issues.
The only open issue is right now if it isn't better to go back for automatic bouncing for SCSI scan and the other users. Do you have an opinion on that too? If you have one can you please convince James of it too.
There are other things that don't want the automatic bouncing; sg, st, and osst.
Your patches remove unchecked_isa_dma in them and Boaz said that they are fine since they get bounced anyway, however, it's not correct.
As Doug said in another thread (about your patch to change GFP_ATOMIC to GFP_KERNEL in sg), they try to avoid waiting for a long time and want an early failure (though they are not complete; can't avoid non unchecked_isa_dma bouncing) .
We can change the bounce path in that way, but I think it's better if they can allocate memory that will not get bounced.
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
Quoted 23 lines · FUJITA Tomonori
Quoted 5 lines · On Mon, 17 Mar 2008 22:34:22 +0100 Andi Kleen <[email protected]> wrote:
Quoted 5 lines
Look further down in the email,
queue_bounce_to_mask()or whatever you would want to call it. As also written there, the PRINCIPLE is the same. And that is that exporting ato_allocator_mask()helper is a lot saner than exporting an allocator api tied to the queue.Quoted 8 lines
Quoted 1 line
Can we get over this, please?
Ok it would have helped if you had explained why it is saner, but I bow to your superior experience on block layer issues.
The only open issue is right now if it isn't better to go back for automatic bouncing for SCSI scan and the other users. Do you have an opinion on that too? If you have one can you please convince James of it too.
There are other things that don't want the automatic bouncing; sg, st, and osst.
Your patches remove
unchecked_isa_dmain them and Boaz said that they are fine since they get bounced anyway, however, it's not correct.
Quoted 7 lines
As Doug said in another thread (about your patch to change
GFP_ATOMICtoGFP_KERNELin sg), they try to avoid waiting for a long time and want an early failure (though they are not complete; can't avoid nonunchecked_isa_dmabouncing) .We can change the bounce path in that way, but I think it's better if they can allocate memory that will not get bounced.
I was looking in blk_queue_bounce (in mm/bounce.c) and have not seen any case of q->bounce_gfp with __GFP_WAIT. Are you sure we are waiting for the bounce buffers? It will take few more CPU cycles, but I don't see where it will sleep.
Boaz
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
Quoted 29 lines · On Wed, 02 Apr 2008 11:43:21 +0300 Boaz Harrosh <[email protected]> wrote:
Quoted 28 lines · FUJITA Tomonori
Quoted 5 lines · On Mon, 17 Mar 2008 22:34:22 +0100 Andi Kleen <[email protected]> wrote:
Quoted 5 lines
Look further down in the email,
queue_bounce_to_mask()or whatever you would want to call it. As also written there, the PRINCIPLE is the same. And that is that exporting ato_allocator_mask()helper is a lot saner than exporting an allocator api tied to the queue.Quoted 8 lines
Quoted 1 line
Can we get over this, please?
Ok it would have helped if you had explained why it is saner, but I bow to your superior experience on block layer issues.
The only open issue is right now if it isn't better to go back for automatic bouncing for SCSI scan and the other users. Do you have an opinion on that too? If you have one can you please convince James of it too.
There are other things that don't want the automatic bouncing; sg, st, and osst.
Your patches remove
unchecked_isa_dmain them and Boaz said that they are fine since they get bounced anyway, however, it's not correct.As Doug said in another thread (about your patch to change
GFP_ATOMICtoGFP_KERNELin sg), they try to avoid waiting for a long time and want an early failure (though they are not complete; can't avoid nonunchecked_isa_dmabouncing) .
Quoted 7 lines
Quoted 2 lines
We can change the bounce path in that way, but I think it's better if they can allocate memory that will not get bounced.
I was looking in
blk_queue_bounce(inmm/bounce.c) and have not seen any case ofq->bounce_gfpwith__GFP_WAIT. Are you sure we are waiting for the bounce buffers? It will take few more CPU cycles, but I don't see where it will sleep.
Hmm, q->bounce_gfp is GFP_NOIO or (GFP_NOIO | __GFP_DMA). And __blk_queue_bounce also allocates bio with GFP_NOIO. So the bounce path always use __GFP_WAIT.
Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages
Quoted 33 lines · FUJITA Tomonori
Quoted 30 lines · On Wed, 02 Apr 2008 11:43:21 +0300 Boaz Harrosh <[email protected]> wrote:
Quoted 29 lines · FUJITA Tomonori
Quoted 5 lines · On Mon, 17 Mar 2008 22:34:22 +0100 Andi Kleen <[email protected]> wrote:
Quoted 5 lines
Look further down in the email,
queue_bounce_to_mask()or whatever you would want to call it. As also written there, the PRINCIPLE is the same. And that is that exporting ato_allocator_mask()helper is a lot saner than exporting an allocator api tied to the queue.Quoted 8 lines
Quoted 1 line
Can we get over this, please?
Ok it would have helped if you had explained why it is saner, but I bow to your superior experience on block layer issues.
The only open issue is right now if it isn't better to go back for automatic bouncing for SCSI scan and the other users. Do you have an opinion on that too? If you have one can you please convince James of it too.
There are other things that don't want the automatic bouncing; sg, st, and osst.
Your patches remove
unchecked_isa_dmain them and Boaz said that they are fine since they get bounced anyway, however, it's not correct.As Doug said in another thread (about your patch to change
GFP_ATOMICtoGFP_KERNELin sg), they try to avoid waiting for a long time and want an early failure (though they are not complete; can't avoid nonunchecked_isa_dmabouncing) .We can change the bounce path in that way, but I think it's better if they can allocate memory that will not get bounced.
Quoted 8 lines
Quoted 4 lines
I was looking in
blk_queue_bounce(inmm/bounce.c) and have not seen any case ofq->bounce_gfpwith__GFP_WAIT. Are you sure we are waiting for the bounce buffers? It will take few more CPU cycles, but I don't see where it will sleep.Hmm,
q->bounce_gfpisGFP_NOIOor (GFP_NOIO|__GFP_DMA). And__blk_queue_bouncealso allocates bio withGFP_NOIO. So the bounce path always use__GFP_WAIT.
OK, Thanks, GFP_NOIO is __GFP_WAIT I didn't realize that. Any way in my new patchset we do not use bouncing and we use the new mask allocator so every one should be happy.
Boaz