Remove ->done from scsi_cmnd
Subsystems (2)
This series of four patches gets rid of ->done from scsi_cmnd. It applies on top of the patch I sent earlier today to improve gdth's abuse of scsi_done. Sorry, Boaz. I'd like to thank Christoph for talking me through some of the bits of the block layer I didn't grok.
PATCH 1/1 Fix mistaken uses of ->done
From: Matthew Wilcox <[email protected]>
All these drivers meant to call ->scsi_done() but got confused.
--- drivers/scsi/NCR5380.c | 8 ++++---- drivers/scsi/NCR53C9x.c | 2 +- drivers/scsi/atari_NCR5380.c | 4 ++-- drivers/scsi/sun3_NCR5380.c | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-)
drivers/scsi/NCR5380.c +4 -4
@@ -2133,7 +2133,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance) { sink =1 ;do_abort (instance); cmd->result =DID_ERROR <<16 ; - cmd->done (cmd); + cmd->scsi_done (cmd);return ;#endif /*
@@ -2196,7 +2196,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance) { sink =1 ;do_abort (instance); cmd->result =DID_ERROR <<16 ; - cmd->done (cmd); + cmd->scsi_done (cmd);/* XXX - need to source or sink data here, as appropriate */ }else cmd->SCp.this_residual -= transfersize - len;
@@ -2740,7 +2740,7 @@ static int NCR5380_abort(Scsi_Cmnd * cmd) { tmp->host_scribble =NULL ; tmp->result =DID_ABORT <<16 ;dprintk (NDEBUG_ABORT , ("scsi%d : abort removed command from issue queue.\n" , instance->host_no)); - tmp->done (tmp); + tmp->scsi_done (tmp);return SUCCESS ; }#if (NDEBUG &NDEBUG_ABORT )
@@ -2805,7 +2805,7 @@ static int NCR5380_abort(Scsi_Cmnd * cmd) { *prev = (Scsi_Cmnd *) tmp->host_scribble; tmp->host_scribble =NULL ; tmp->result =DID_ABORT <<16 ; - tmp->done (tmp); + tmp->scsi_done (tmp);return SUCCESS ; } }
drivers/scsi/NCR53C9x.c +1 -1
@@ -1385,7 +1385,7 @@ int esp_abort(Scsi_Cmnd *SCptr) this->host_scribble =NULL ;esp_release_dmabufs (esp, this); this->result =DID_ABORT <<16 ; - this->done (this); + this->scsi_done (this);if (don) esp->dma_ints_on (esp);return SUCCESS ;
drivers/scsi/atari_NCR5380.c +2 -2
@@ -2041,7 +2041,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance) sink =1 ;do_abort (instance); cmd->result =DID_ERROR <<16 ; - cmd->done (cmd); + cmd->scsi_done (cmd);return ;#endif case PHASE_DATAIN :
@@ -2100,7 +2100,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance) sink =1 ;do_abort (instance); cmd->result =DID_ERROR <<16 ; - cmd->done (cmd); + cmd->scsi_done (cmd);/* XXX - need to source or sink data here, as appropriate */ }else {#ifdef REAL_DMA
drivers/scsi/sun3_NCR5380.c +2 -2
@@ -2055,7 +2055,7 @@ static void NCR5380_information_transfer (struct Scsi_Host *instance) sink =1 ;do_abort (instance); cmd->result =DID_ERROR <<16 ; - cmd->done (cmd); + cmd->scsi_done (cmd);return ;#endif case PHASE_DATAIN :
@@ -2115,7 +2115,7 @@ static void NCR5380_information_transfer (struct Scsi_Host *instance) sink =1 ;do_abort (instance); cmd->result =DID_ERROR <<16 ; - cmd->done (cmd); + cmd->scsi_done (cmd);/* XXX - need to source or sink data here, as appropriate */ }else {#ifdef REAL_DMA
Signature · 1 line
1.5.3.1
PATCH 1/1 Get rid of scsi_cmnd->done
In reply to Matthew WilcoxFrom: Matthew Wilcox <[email protected]>
The ULD ->done callback moves into the scsi_driver. By moving the call to scsi_io_completion() from scsi_blk_pc_done() to scsi_finish_command(), we can eliminate the latter entirely. By returning 'good_bytes' from the ->done callback (rather than invoking scsi_io_completion()), we can stop exporting scsi_io_completion().
Also move the prototypes from sd.h to sd.c as they're all internal anyway. Rename sd_rw_intr to sd_done and rw_intr to sr_done.
--- drivers/scsi/scsi.c | 20 +++++++++++++++++--- drivers/scsi/scsi_error.c | 1 - drivers/scsi/scsi_lib.c | 14 -------------- drivers/scsi/scsi_priv.h | 1 + drivers/scsi/sd.c | 28 ++++++++++++++++++---------- drivers/scsi/sr.c | 21 ++++++--------------- include/scsi/scsi_cmnd.h | 2 -- include/scsi/scsi_driver.h | 1 + include/scsi/sd.h | 13 ------------- 9 files changed, 43 insertions(+), 58 deletions(-)
drivers/scsi/scsi.c +17 -3
@@ -59,6 +59,7 @@#include <scsi/scsi_cmnd.h> #include <scsi/scsi_dbg.h> #include <scsi/scsi_device.h> +#include <scsi/scsi_driver.h> #include <scsi/scsi_eh.h> #include <scsi/scsi_host.h> #include <scsi/scsi_tcq.h>
@@ -367,9 +368,8 @@ void scsi_log_send(struct scsi_cmnd *cmd)scsi_print_command (cmd);if (level >3 ) {printk (KERN_INFO "buffer = 0x%p, bufflen = %d," -" done = 0x%p, queuecommand 0x%p\n" , +" queuecommand 0x%p\n" ,scsi_sglist (cmd),scsi_bufflen (cmd), - cmd->done, cmd->device->host->hostt->queuecommand); }
@@ -658,6 +658,12 @@ void __scsi_done(struct scsi_cmnd *cmd)blk_complete_request (rq); } +/* Move this to a header if it becomes more generally useful */ +static struct scsi_driver *scsi_cmd_to_driver (struct scsi_cmnd *cmd) +{ +return *(struct scsi_driver **)cmd->request->rq_disk->private_data; +} +/* * Function: scsi_finish_command *
@@ -669,6 +675,8 @@ void scsi_finish_command(struct scsi_cmnd *cmd) {struct scsi_device *sdev = cmd->device;struct Scsi_Host *shost = sdev->host; +struct scsi_driver *drv; +unsigned int good_bytes;scsi_device_unbusy (sdev);
@@ -694,7 +702,13 @@ void scsi_finish_command(struct scsi_cmnd *cmd)"Notifying upper driver of completion " "(result %x)\n", cmd->result)); - cmd->done (cmd); + good_bytes = cmd->request_bufflen; +if (cmd->request->cmd_type !=REQ_TYPE_BLOCK_PC ) { + drv =scsi_cmd_to_driver (cmd); +if (drv->done) + good_bytes = drv->done (cmd); + } +scsi_io_completion (cmd, good_bytes); }EXPORT_SYMBOL (scsi_finish_command);
drivers/scsi/scsi_error.c +0 -1
@@ -1671,7 +1671,6 @@ scsi_reset_provider(struct scsi_device *dev, int flag)memset (&scmd->cmnd,'\0' ,sizeof (scmd->cmnd)); scmd->scsi_done = scsi_reset_provider_done_command; - scmd->done =NULL ; scmd->request_buffer =NULL ; scmd->request_bufflen =0 ;
drivers/scsi/scsi_lib.c +0 -14
@@ -982,7 +982,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes) }scsi_end_request (cmd,0 , this_count, !result); } -EXPORT_SYMBOL (scsi_io_completion);/* * Function: scsi_init_io()
@@ -1063,18 +1062,6 @@ static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,return cmd; } -static void scsi_blk_pc_done (struct scsi_cmnd *cmd) -{ -BUG_ON (!blk_pc_request (cmd->request)); -/* -* This will complete the whole command with uptodate=1 so -* as far as the block layer is concerned the command completed -* successfully. Since this is a REQ_BLOCK_PC command the -* caller should check the request's errors value -*/ -scsi_io_completion (cmd, cmd->request_bufflen); -} -int scsi_setup_blk_pc_cmnd (struct scsi_device *sdev,struct request *req) {struct scsi_cmnd *cmd;
@@ -1124,7 +1111,6 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) cmd->transfersize = req->data_len; cmd->allowed = req->retries; cmd->timeout_per_command = req->timeout; - cmd->done = scsi_blk_pc_done;return BLKPREP_OK ; }EXPORT_SYMBOL (scsi_setup_blk_pc_cmnd);
drivers/scsi/scsi_priv.h +1 -0
@@ -68,6 +68,7 @@ extern int scsi_maybe_unblock_host(struct scsi_device *sdev);extern void scsi_device_unbusy (struct scsi_device *sdev);extern int scsi_queue_insert (struct scsi_cmnd *cmd,int reason);extern void scsi_next_command (struct scsi_cmnd *cmd); +extern void scsi_io_completion (struct scsi_cmnd *,unsigned int );extern void scsi_run_host_queues (struct Scsi_Host *shost);extern struct request_queue *scsi_alloc_queue (struct scsi_device *sdev);extern void scsi_free_queue (struct request_queue *q);
drivers/scsi/sd.c +18 -10
@@ -86,6 +86,19 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_DISK);MODULE_ALIAS_SCSI_DEVICE (TYPE_MOD );MODULE_ALIAS_SCSI_DEVICE (TYPE_RBC ); +static int sd_revalidate_disk (struct gendisk *); +static int sd_probe (struct device *); +static int sd_remove (struct device *); +static void sd_shutdown (struct device *); +static int sd_suspend (struct device *,pm_message_t state); +static int sd_resume (struct device *); +static void sd_rescan (struct device *); +static int sd_done (struct scsi_cmnd *); +static void sd_read_capacity (struct scsi_disk *sdkp,unsigned char *buffer); +static void scsi_disk_release (struct class_device *cdev); +static void sd_print_sense_hdr (struct scsi_disk *,struct scsi_sense_hdr *); +static void sd_print_result (struct scsi_disk *,int ); +static DEFINE_IDR (sd_index_idr);static DEFINE_SPINLOCK (sd_index_lock);
@@ -240,6 +253,7 @@ static struct scsi_driver sd_template = { .shutdown = sd_shutdown, }, .rescan = sd_rescan, + .done = sd_done, };/*
@@ -509,12 +523,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq) SCpnt->timeout_per_command = timeout;/* -* This is the completion routine we use. This is matched in terms -* of capability to this function. -*/ - SCpnt->done = sd_rw_intr; - -/* * This indicates that the command is ready from our end to be * queued. */
@@ -908,13 +916,13 @@ static struct block_device_operations sd_fops = { };/** -* sd_rw_intr - bottom half handler: called when the lower level +* sd_done - bottom half handler: called when the lower level * driver has completed (successfully or otherwise) a scsi command. * @SCpnt: mid-level's per command structure. * * Note: potentially run from within an ISR. Must not block. **/ -static void sd_rw_intr (struct scsi_cmnd * SCpnt) +static int sd_done (struct scsi_cmnd *SCpnt) {int result = SCpnt->result;unsigned int xfer_size = SCpnt->request_bufflen;
@@ -935,7 +943,7 @@ static void sd_rw_intr(struct scsi_cmnd * SCpnt)SCSI_LOG_HLCOMPLETE (1 ,scsi_print_result (SCpnt));if (sense_valid) { SCSI_LOG_HLCOMPLETE(1, scmd_printk(KERN_INFO, SCpnt, - "sd_rw_intr: sb[respc,sk,asc," + "sd_done: sb[respc,sk,asc," "ascq]=%x,%x,%x,%x\n", sshdr.response_code, sshdr.sense_key, sshdr.asc,
@@ -1007,7 +1015,7 @@ static void sd_rw_intr(struct scsi_cmnd * SCpnt)break ; } out: -scsi_io_completion (SCpnt, good_bytes); +return good_bytes; } static int media_not_present(struct scsi_disk *sdkp,
drivers/scsi/sr.c +6 -15
@@ -78,6 +78,7 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_WORM);static int sr_probe (struct device *);static int sr_remove (struct device *); +static int sr_done (struct scsi_cmnd *);static struct scsi_driver sr_template = { .owner =THIS_MODULE ,
@@ -86,6 +87,7 @@ static struct scsi_driver sr_template = { .probe = sr_probe, .remove = sr_remove, }, + .done = sr_done, };static unsigned long sr_index_bits[SR_DISKS /BITS_PER_LONG ];
@@ -208,12 +210,12 @@ static int sr_media_change(struct cdrom_device_info *cdi, int slot) }/* -* rw_intr is the interrupt routine for the device driver. +* sr_done is the interrupt routine for the device driver. * -* It will be notified on the end of a SCSI read / write, and will take on +* It will be notified on the end of a SCSI read / write, and will take one * of several actions based on success or failure. */ -static void rw_intr (struct scsi_cmnd * SCpnt) +static int sr_done (struct scsi_cmnd *SCpnt) {int result = SCpnt->result;int this_count = SCpnt->request_bufflen;
@@ -286,12 +288,7 @@ static void rw_intr(struct scsi_cmnd * SCpnt) } } -/* -* This calls the generic completion function, now that we know -* how many actual sectors finished, and how many sectors we need -* to say have failed. -*/ -scsi_io_completion (SCpnt, good_bytes); +return good_bytes; }static int sr_prep_fn (struct request_queue *q,struct request *rq)
@@ -428,12 +425,6 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq) SCpnt->timeout_per_command = timeout;/* -* This is the completion routine we use. This is matched in terms -* of capability to this function. -*/ - SCpnt->done = rw_intr; - -/* * This indicates that the command is ready from our end to be * queued. */
include/scsi/scsi_cmnd.h +0 -2
@@ -33,7 +33,6 @@ struct scsi_cmnd {struct list_head list;/* scsi_cmnd participates in queue lists */ struct list_head eh_entry;/* entry for the host eh_cmd_q */ int eh_eflags;/* Used by error handlr */ -void (*done) (struct scsi_cmnd *);/* Mid-level done function */ /* * A SCSI Command is assigned a nonzero serial_number before passed
@@ -124,7 +123,6 @@ extern struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *, gfp_t);extern void scsi_put_command (struct scsi_cmnd *);extern void __scsi_put_command (struct Scsi_Host *,struct scsi_cmnd *,struct device *); -extern void scsi_io_completion (struct scsi_cmnd *,unsigned int );extern void scsi_finish_command (struct scsi_cmnd *cmd);extern void scsi_req_abort_cmd (struct scsi_cmnd *cmd);
include/scsi/scsi_driver.h +1 -0
@@ -15,6 +15,7 @@ struct scsi_driver {struct device_driver gendrv;void (*rescan)(struct device *); +int (*done)(struct scsi_cmnd *); };#define to_scsi_driver (drv) \ container_of((drv), struct scsi_driver, gendrv)
include/scsi/sd.h +0 -13
@@ -47,19 +47,6 @@ struct scsi_disk { };#define to_scsi_disk (obj) container_of(obj,struct scsi_disk,cdev) -static int sd_revalidate_disk (struct gendisk *disk); -static void sd_rw_intr (struct scsi_cmnd * SCpnt); -static int sd_probe (struct device *); -static int sd_remove (struct device *); -static void sd_shutdown (struct device *dev); -static int sd_suspend (struct device *dev,pm_message_t state); -static int sd_resume (struct device *dev); -static void sd_rescan (struct device *); -static void sd_read_capacity (struct scsi_disk *sdkp,unsigned char *buffer); -static void scsi_disk_release (struct class_device *cdev); -static void sd_print_sense_hdr (struct scsi_disk *,struct scsi_sense_hdr *); -static void sd_print_result (struct scsi_disk *,int ); - #define sd_printk(prefix, sdsk, fmt, a...) \ (sdsk)->disk ? \ sdev_printk(prefix, (sdsk)->device, "[%s] " fmt, \
Signature · 1 line
1.5.3.1
Re: [PATCH] Get rid of scsi_cmnd->done
Quoted 71 lines · Matthew Wilcox
From: Matthew Wilcox <[email protected]>
The ULD ->done callback moves into the
scsi_driver. By moving the call toscsi_io_completion()fromscsi_blk_pc_done()toscsi_finish_command(), we can eliminate the latter entirely. By returning 'good_bytes' from the ->done callback (rather than invokingscsi_io_completion()), we can stop exportingscsi_io_completion().Also move the prototypes from
sd.htosd.cas they're all internal anyway. Renamesd_rw_intrtosd_doneandrw_intrtosr_done.Inspired-by Christoph Hellwig <[email protected]>Signed-off-by Matthew Wilcox <[email protected]>--- drivers/scsi/scsi.c | 20 +++++++++++++++++--- drivers/scsi/scsi_error.c | 1 - drivers/scsi/scsi_lib.c | 14 -------------- drivers/scsi/scsi_priv.h | 1 + drivers/scsi/sd.c | 28 ++++++++++++++++++---------- drivers/scsi/sr.c | 21 ++++++--------------- include/scsi/scsi_cmnd.h | 2 -- include/scsi/scsi_driver.h | 1 + include/scsi/sd.h | 13 ------------- 9 files changed, 43 insertions(+), 58 deletions(-)drivers/scsi/scsi.c +10
-2@@ -59,6 +59,7 @@#include <scsi/scsi_cmnd.h> #include <scsi/scsi_dbg.h> #include <scsi/scsi_device.h> +#include <scsi/scsi_driver.h> #include <scsi/scsi_eh.h> #include <scsi/scsi_host.h> #include <scsi/scsi_tcq.h> @@ -367,9 +368,8 @@ void scsi_log_send(struct scsi_cmnd *cmd)scsi_print_command (cmd);if (level >3 ) {printk (KERN_INFO "buffer = 0x%p, bufflen = %d," -" done = 0x%p, queuecommand 0x%p\n" , +" queuecommand 0x%p\n" ,scsi_sglist (cmd),scsi_bufflen (cmd), - cmd->done, cmd->device->host->hostt->queuecommand); }@@ -658,6 +658,12 @@ void __scsi_done(struct scsi_cmnd *cmd)blk_complete_request (rq); } +/* Move this to a header if it becomes more generally useful */ +static struct scsi_driver *scsi_cmd_to_driver (struct scsi_cmnd *cmd) +{ +return *(struct scsi_driver **)cmd->request->rq_disk->private_data; +} +/* * Function: scsi_finish_command * @@ -669,6 +675,8 @@ void scsi_finish_command(struct scsi_cmnd *cmd) {struct scsi_device *sdev = cmd->device;struct Scsi_Host *shost = sdev->host; +struct scsi_driver *drv; +unsigned int good_bytes;scsi_device_unbusy (sdev);
drivers/scsi/scsi.c void scsi_finish_command(struct scsi_cmnd *cmd) · line 705
Quoted 5 lines
drivers/scsi/scsi.c +1
-1@@ -694,7 +702,13 @@ void scsi_finish_command(struct scsi_cmnd *cmd)"Notifying upper driver of completion " "(result %x)\n", cmd->result)); - cmd->done (cmd); + good_bytes = cmd->request_bufflen;
Please use scsi_bufflen(cmd) this file was already converted to accessors.
Quoted 260 lines
drivers/scsi/scsi.c +6
-0+if (cmd->request->cmd_type !=REQ_TYPE_BLOCK_PC ) { + drv =scsi_cmd_to_driver (cmd); +if (drv->done) + good_bytes = drv->done (cmd); + } +scsi_io_completion (cmd, good_bytes); }EXPORT_SYMBOL (scsi_finish_command);drivers/scsi/scsi_error.c +0
-1@@ -1671,7 +1671,6 @@ scsi_reset_provider(struct scsi_device *dev, int flag)memset (&scmd->cmnd,'\0' ,sizeof (scmd->cmnd)); scmd->scsi_done = scsi_reset_provider_done_command; - scmd->done =NULL ; scmd->request_buffer =NULL ; scmd->request_bufflen =0 ;drivers/scsi/scsi_lib.c +0
-14@@ -982,7 +982,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes) }scsi_end_request (cmd,0 , this_count, !result); } -EXPORT_SYMBOL (scsi_io_completion);/* * Function: scsi_init_io() @@ -1063,18 +1062,6 @@ static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,return cmd; } -static void scsi_blk_pc_done (struct scsi_cmnd *cmd) -{ -BUG_ON (!blk_pc_request (cmd->request)); -/* -* This will complete the whole command with uptodate=1 so -* as far as the block layer is concerned the command completed -* successfully. Since this is a REQ_BLOCK_PC command the -* caller should check the request's errors value -*/ -scsi_io_completion (cmd, cmd->request_bufflen); -} -int scsi_setup_blk_pc_cmnd (struct scsi_device *sdev,struct request *req) {struct scsi_cmnd *cmd;@@ -1124,7 +1111,6 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) cmd->transfersize = req->data_len; cmd->allowed = req->retries; cmd->timeout_per_command = req->timeout; - cmd->done = scsi_blk_pc_done;return BLKPREP_OK ; }EXPORT_SYMBOL (scsi_setup_blk_pc_cmnd);drivers/scsi/scsi_priv.h +1
-0@@ -68,6 +68,7 @@ extern int scsi_maybe_unblock_host(struct scsi_device *sdev);extern void scsi_device_unbusy (struct scsi_device *sdev);extern int scsi_queue_insert (struct scsi_cmnd *cmd,int reason);extern void scsi_next_command (struct scsi_cmnd *cmd); +extern void scsi_io_completion (struct scsi_cmnd *,unsigned int );extern void scsi_run_host_queues (struct Scsi_Host *shost);extern struct request_queue *scsi_alloc_queue (struct scsi_device *sdev);extern void scsi_free_queue (struct request_queue *q);drivers/scsi/sd.c +18
-10@@ -86,6 +86,19 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_DISK);MODULE_ALIAS_SCSI_DEVICE (TYPE_MOD );MODULE_ALIAS_SCSI_DEVICE (TYPE_RBC ); +static int sd_revalidate_disk (struct gendisk *); +static int sd_probe (struct device *); +static int sd_remove (struct device *); +static void sd_shutdown (struct device *); +static int sd_suspend (struct device *,pm_message_t state); +static int sd_resume (struct device *); +static void sd_rescan (struct device *); +static int sd_done (struct scsi_cmnd *); +static void sd_read_capacity (struct scsi_disk *sdkp,unsigned char *buffer); +static void scsi_disk_release (struct class_device *cdev); +static void sd_print_sense_hdr (struct scsi_disk *,struct scsi_sense_hdr *); +static void sd_print_result (struct scsi_disk *,int ); +static DEFINE_IDR (sd_index_idr);static DEFINE_SPINLOCK (sd_index_lock);@@ -240,6 +253,7 @@ static struct scsi_driver sd_template = { .shutdown = sd_shutdown, }, .rescan = sd_rescan, + .done = sd_done, };/* @@ -509,12 +523,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq) SCpnt->timeout_per_command = timeout;/* -* This is the completion routine we use. This is matched in terms -* of capability to this function. -*/ - SCpnt->done = sd_rw_intr; - -/* * This indicates that the command is ready from our end to be * queued. */ @@ -908,13 +916,13 @@ static struct block_device_operations sd_fops = { };/** -* sd_rw_intr - bottom half handler: called when the lower level +* sd_done - bottom half handler: called when the lower level * driver has completed (successfully or otherwise) a scsi command. * @SCpnt: mid-level's per command structure. * * Note: potentially run from within an ISR. Must not block. **/ -static void sd_rw_intr (struct scsi_cmnd * SCpnt) +static int sd_done (struct scsi_cmnd *SCpnt) {int result = SCpnt->result;unsigned int xfer_size = SCpnt->request_bufflen;@@ -935,7 +943,7 @@ static void sd_rw_intr(struct scsi_cmnd * SCpnt)SCSI_LOG_HLCOMPLETE (1 ,scsi_print_result (SCpnt));if (sense_valid) { SCSI_LOG_HLCOMPLETE(1, scmd_printk(KERN_INFO, SCpnt, - "sd_rw_intr: sb[respc,sk,asc," + "sd_done: sb[respc,sk,asc," "ascq]=%x,%x,%x,%x\n", sshdr.response_code, sshdr.sense_key, sshdr.asc,@@ -1007,7 +1015,7 @@ static void sd_rw_intr(struct scsi_cmnd * SCpnt)break ; } out: -scsi_io_completion (SCpnt, good_bytes); +return good_bytes; } static int media_not_present(struct scsi_disk *sdkp,drivers/scsi/sr.c +6
-15@@ -78,6 +78,7 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_WORM);static int sr_probe (struct device *);static int sr_remove (struct device *); +static int sr_done (struct scsi_cmnd *);static struct scsi_driver sr_template = { .owner =THIS_MODULE ,@@ -86,6 +87,7 @@ static struct scsi_driver sr_template = { .probe = sr_probe, .remove = sr_remove, }, + .done = sr_done, };static unsigned long sr_index_bits[SR_DISKS /BITS_PER_LONG ];@@ -208,12 +210,12 @@ static int sr_media_change(struct cdrom_device_info *cdi, int slot) }/* -* rw_intr is the interrupt routine for the device driver. +* sr_done is the interrupt routine for the device driver. * -* It will be notified on the end of a SCSI read / write, and will take on +* It will be notified on the end of a SCSI read / write, and will take one * of several actions based on success or failure. */ -static void rw_intr (struct scsi_cmnd * SCpnt) +static int sr_done (struct scsi_cmnd *SCpnt) {int result = SCpnt->result;int this_count = SCpnt->request_bufflen;@@ -286,12 +288,7 @@ static void rw_intr(struct scsi_cmnd * SCpnt) } } -/* -* This calls the generic completion function, now that we know -* how many actual sectors finished, and how many sectors we need -* to say have failed. -*/ -scsi_io_completion (SCpnt, good_bytes); +return good_bytes; }static int sr_prep_fn (struct request_queue *q,struct request *rq)@@ -428,12 +425,6 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq) SCpnt->timeout_per_command = timeout;/* -* This is the completion routine we use. This is matched in terms -* of capability to this function. -*/ - SCpnt->done = rw_intr; - -/* * This indicates that the command is ready from our end to be * queued. */ include/scsi/scsi_cmnd.h +0
-2@@ -33,7 +33,6 @@ struct scsi_cmnd {struct list_head list;/* scsi_cmnd participates in queue lists */ struct list_head eh_entry;/* entry for the host eh_cmd_q */ int eh_eflags;/* Used by error handlr */ -void (*done) (struct scsi_cmnd *);/* Mid-level done function */ /* * A SCSI Command is assigned a nonzero serial_number before passed @@ -124,7 +123,6 @@ extern struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *, gfp_t);extern void scsi_put_command (struct scsi_cmnd *);extern void __scsi_put_command (struct Scsi_Host *,struct scsi_cmnd *,struct device *); -extern void scsi_io_completion (struct scsi_cmnd *,unsigned int );extern void scsi_finish_command (struct scsi_cmnd *cmd);extern void scsi_req_abort_cmd (struct scsi_cmnd *cmd);include/scsi/scsi_driver.h +1
-0@@ -15,6 +15,7 @@ struct scsi_driver {struct device_driver gendrv;void (*rescan)(struct device *); +int (*done)(struct scsi_cmnd *); };#define to_scsi_driver (drv) \ container_of((drv), struct scsi_driver, gendrv)include/scsi/sd.h +0
-6@@ -47,19 +47,6 @@ struct scsi_disk { };#define to_scsi_disk (obj) container_of(obj,struct scsi_disk,cdev) -static int sd_revalidate_disk (struct gendisk *disk); -static void sd_rw_intr (struct scsi_cmnd * SCpnt); -static int sd_probe (struct device *); -static int sd_remove (struct device *); -static void sd_shutdown (struct device *dev); -static int sd_suspend (struct device *dev,pm_message_t state);
include/scsi/sd.h struct scsi_disk { · line 52
Quoted 10 lines
include/scsi/sd.h +0
-7-static int sd_resume (struct device *dev); -static void sd_rescan (struct device *); -static void sd_read_capacity (struct scsi_disk *sdkp,unsigned char *buffer); -static void scsi_disk_release (struct class_device *cdev); -static void sd_print_sense_hdr (struct scsi_disk *,struct scsi_sense_hdr *); -static void sd_print_result (struct scsi_disk *,int ); - #define sd_printk(prefix, sdsk, fmt, a...) \ (sdsk)->disk ? \ sdev_printk(prefix, (sdsk)->device, "[%s] " fmt, \
Thanks otherwise looks very good
Boaz
Re: [PATCH] Get rid of scsi_cmnd->done
Quoted 8 lines · Boaz Harrosh
drivers/scsi/scsi.c void scsi_finish_command(struct scsi_cmnd *cmd) · line 705
Quoted 6 lines
drivers/scsi/scsi.c +1
-1@@ -694,7 +702,13 @@ void scsi_finish_command(struct scsi_cmnd *cmd)"Notifying upper driver of completion " "(result %x)\n", cmd->result)); - cmd->done (cmd); + good_bytes = cmd->request_bufflen;Please use
scsi_bufflen(cmd)this file was already converted to accessors.
Ah; I was basing this on linus + scsi-misc. I guess scsi_lib hadn't been converted because I just took the code from there.
Quoted 1 line
Thanks otherwise looks very good
Thank you for the review!
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
PATCH 1/1 pluto: Don't abuse ->done for internal commands
In reply to Matthew WilcoxFrom: Matthew Wilcox <[email protected]>
We can simply call the internal done function directly
--- drivers/scsi/pluto.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
drivers/scsi/pluto.c +1 -2
@@ -160,7 +160,6 @@ int __init pluto_detect(struct scsi_host_template *tpnt) SCpnt->request->cmd_flags &= ~REQ_STARTED ; - SCpnt->done = pluto_detect_done; SCpnt->request_bufflen =256 ; SCpnt->request_buffer = fcs[i].inquiry;PLD (("set up %d %08lx\n" , i, (long )SCpnt))
@@ -195,7 +194,7 @@ int __init pluto_detect(struct scsi_host_template *tpnt) SCpnt = &(fcs[i].cmd);/* Let FC mid-level free allocated resources */ - SCpnt->done (SCpnt); +pluto_detect_scsi_done (SCpnt);if (!SCpnt->result) {struct pluto_inquiry *inq;
Signature · 1 line
1.5.3.1
PATCH 1/1 gdth: Stop abusing ->done for internal commands
In reply to Matthew WilcoxFrom: Matthew Wilcox <[email protected]>
The ->done member was being used to mark commands as being internal. I decided to put a magic number in ->underflow instead. I believe this to be safe as no current user of ->underflow has any of the bottom 9 bits set.
--- drivers/scsi/gdth.c | 19 +++++++++++-------- drivers/scsi/gdth_proc.c | 4 ++-- 2 files changed, 13 insertions(+), 10 deletions(-)
drivers/scsi/gdth.c +11 -8
@@ -691,6 +691,9 @@ static const struct file_operations gdth_fops = { .release = gdth_close, }; +#define GDTH_MAGIC 0xc2e7c389/* I got it from /dev/urandom */ +#define IS_GDTH_INTERNAL_CMD (scp) (scp->underflow == GDTH_MAGIC) +#include "gdth_proc.h" #include "gdth_proc.c"
@@ -713,7 +716,7 @@ static void gdth_scsi_done(struct scsi_cmnd *scp) {TRACE2 (("gdth_scsi_done()\n" )); -if (scp->done == gdth_scsi_done) +if (IS_GDTH_INTERNAL_CMD (scp))complete ((struct completion *)scp->request);else scp->scsi_done (scp);
@@ -738,7 +741,7 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd, scp->cmd_len =12 ;memcpy (scp->cmnd, cmnd,12 ); scp->SCp.this_residual =IOCTL_PRI ;/* priority */ - scp->done = gdth_scsi_done; + scp->underflow =GDTH_MAGIC ;gdth_queuecommand (scp,NULL );wait_for_completion (&wait);
@@ -2319,7 +2322,7 @@ static void gdth_putq(int hanum,Scsi_Cmnd *scp,unchar priority) ha =HADATA (gdth_ctr_tab[hanum]);spin_lock_irqsave (&ha->smp_lock, flags); -if (scp->done != gdth_scsi_done) { +if (!IS_GDTH_INTERNAL_CMD (scp)) { scp->SCp.this_residual = (int )priority; b = virt_ctr ?NUMDATA (scp->device->host)->busnum:scp->device->channel; t = scp->device->id;
@@ -2382,7 +2385,7 @@ static void gdth_next(int hanum)for (nscp = pscp = ha->req_first; nscp; nscp = (Scsi_Cmnd *)nscp->SCp.ptr) {if (nscp != pscp && nscp != (Scsi_Cmnd *)pscp->SCp.ptr) pscp = (Scsi_Cmnd *)pscp->SCp.ptr; -if (nscp->done != gdth_scsi_done) { +if (!IS_GDTH_INTERNAL_CMD (nscp)) { b = virt_ctr ?NUMDATA (nscp->device->host)->busnum : nscp->device->channel; t = nscp->device->id;
@@ -2408,7 +2411,7 @@ static void gdth_next(int hanum) firsttime = FALSE; } -if (nscp->done != gdth_scsi_done) { +if (!IS_GDTH_INTERNAL_CMD (nscp)) {if (nscp->SCp.phase ==-1 ) { nscp->SCp.phase =CACHESERVICE ;/* default: cache svc. */ if (nscp->cmnd[0 ] ==TEST_UNIT_READY ) {
@@ -2471,7 +2474,7 @@ static void gdth_next(int hanum)else gdth_scsi_done (nscp); } - }else if (nscp->done == gdth_scsi_done) { + }else if (IS_GDTH_INTERNAL_CMD (nscp)) {if (!(cmd_index=gdth_special_cmd (hanum,nscp))) this_cmd = FALSE; next_cmd = FALSE;
@@ -3812,7 +3815,7 @@ static int gdth_sync_event(int hanum,int service,unchar index,Scsi_Cmnd *scp) scp->sense_buffer[2 ] =NOT_READY ; scp->result = (DID_OK <<16 ) | (CHECK_CONDITION <<1 ); } -if (scp->done != gdth_scsi_done) { +if (!IS_GDTH_INTERNAL_CMD (scp)) { ha->dvr.size =sizeof (ha->dvr.eu.sync); ha->dvr.eu.sync.ionode = hanum; ha->dvr.eu.sync.service = service;
@@ -4910,7 +4913,7 @@ static int gdth_queuecommand(struct scsi_cmnd *scp,#endif priority =DEFAULT_PRI ; -if (scp->done == gdth_scsi_done) +if (IS_GDTH_INTERNAL_CMD (scp)) priority = scp->SCp.this_residual;else gdth_update_timeout (hanum, scp, scp->timeout_per_command *6 );
drivers/scsi/gdth_proc.c +2 -2
@@ -805,7 +805,7 @@ static void gdth_stop_timeout(int hanum, int busnum, int id)spin_lock_irqsave (&ha->smp_lock, flags);for (scp = ha->req_first; scp; scp = (Scsi_Cmnd *)scp->SCp.ptr) { -if (scp->done != gdth_scsi_done) { +if (!IS_GDTH_INTERNAL_CMD (scp)) { b = virt_ctr ?NUMDATA (scp->device->host)->busnum : scp->device->channel; t = scp->device->id;
@@ -829,7 +829,7 @@ static void gdth_start_timeout(int hanum, int busnum, int id)spin_lock_irqsave (&ha->smp_lock, flags);for (scp = ha->req_first; scp; scp = (Scsi_Cmnd *)scp->SCp.ptr) { -if (scp->done != gdth_scsi_done) { +if (!IS_GDTH_INTERNAL_CMD (scp)) { b = virt_ctr ?NUMDATA (scp->device->host)->busnum : scp->device->channel; t = scp->device->id;
Signature · 1 line
1.5.3.1
Re: [PATCH] gdth: Stop abusing ->done for internal commands
Quoted 113 lines · Matthew Wilcox
From: Matthew Wilcox <[email protected]>
The ->done member was being used to mark commands as being internal. I decided to put a magic number in ->underflow instead. I believe this to be safe as no current user of ->underflow has any of the bottom 9 bits set.
Signed-off-by Matthew Wilcox <[email protected]>--- drivers/scsi/gdth.c | 19 +++++++++++-------- drivers/scsi/gdth_proc.c | 4 ++-- 2 files changed, 13 insertions(+), 10 deletions(-)drivers/scsi/gdth.c +11
-8@@ -691,6 +691,9 @@ static const struct file_operations gdth_fops = { .release = gdth_close, }; +#define GDTH_MAGIC 0xc2e7c389/* I got it from /dev/urandom */ +#define IS_GDTH_INTERNAL_CMD (scp) (scp->underflow == GDTH_MAGIC) +#include "gdth_proc.h" #include "gdth_proc.c" @@ -713,7 +716,7 @@ static void gdth_scsi_done(struct scsi_cmnd *scp) {TRACE2 (("gdth_scsi_done()\n" )); -if (scp->done == gdth_scsi_done) +if (IS_GDTH_INTERNAL_CMD (scp))complete ((struct completion *)scp->request);else scp->scsi_done (scp);@@ -738,7 +741,7 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd, scp->cmd_len =12 ;memcpy (scp->cmnd, cmnd,12 ); scp->SCp.this_residual =IOCTL_PRI ;/* priority */ - scp->done = gdth_scsi_done; + scp->underflow =GDTH_MAGIC ;gdth_queuecommand (scp,NULL );wait_for_completion (&wait);@@ -2319,7 +2322,7 @@ static void gdth_putq(int hanum,Scsi_Cmnd *scp,unchar priority) ha =HADATA (gdth_ctr_tab[hanum]);spin_lock_irqsave (&ha->smp_lock, flags); -if (scp->done != gdth_scsi_done) { +if (!IS_GDTH_INTERNAL_CMD (scp)) { scp->SCp.this_residual = (int )priority; b = virt_ctr ?NUMDATA (scp->device->host)->busnum:scp->device->channel; t = scp->device->id;@@ -2382,7 +2385,7 @@ static void gdth_next(int hanum)for (nscp = pscp = ha->req_first; nscp; nscp = (Scsi_Cmnd *)nscp->SCp.ptr) {if (nscp != pscp && nscp != (Scsi_Cmnd *)pscp->SCp.ptr) pscp = (Scsi_Cmnd *)pscp->SCp.ptr; -if (nscp->done != gdth_scsi_done) { +if (!IS_GDTH_INTERNAL_CMD (nscp)) { b = virt_ctr ?NUMDATA (nscp->device->host)->busnum : nscp->device->channel; t = nscp->device->id;@@ -2408,7 +2411,7 @@ static void gdth_next(int hanum) firsttime = FALSE; } -if (nscp->done != gdth_scsi_done) { +if (!IS_GDTH_INTERNAL_CMD (nscp)) {if (nscp->SCp.phase ==-1 ) { nscp->SCp.phase =CACHESERVICE ;/* default: cache svc. */ if (nscp->cmnd[0 ] ==TEST_UNIT_READY ) {@@ -2471,7 +2474,7 @@ static void gdth_next(int hanum)else gdth_scsi_done (nscp); } - }else if (nscp->done == gdth_scsi_done) { + }else if (IS_GDTH_INTERNAL_CMD (nscp)) {if (!(cmd_index=gdth_special_cmd (hanum,nscp))) this_cmd = FALSE; next_cmd = FALSE;@@ -3812,7 +3815,7 @@ static int gdth_sync_event(int hanum,int service,unchar index,Scsi_Cmnd *scp) scp->sense_buffer[2 ] =NOT_READY ; scp->result = (DID_OK <<16 ) | (CHECK_CONDITION <<1 ); } -if (scp->done != gdth_scsi_done) { +if (!IS_GDTH_INTERNAL_CMD (scp)) { ha->dvr.size =sizeof (ha->dvr.eu.sync); ha->dvr.eu.sync.ionode = hanum; ha->dvr.eu.sync.service = service;@@ -4910,7 +4913,7 @@ static int gdth_queuecommand(struct scsi_cmnd *scp,#endif priority =DEFAULT_PRI ; -if (scp->done == gdth_scsi_done) +if (IS_GDTH_INTERNAL_CMD (scp)) priority = scp->SCp.this_residual;else gdth_update_timeout (hanum, scp, scp->timeout_per_command *6 );drivers/scsi/gdth_proc.c +1
-1@@ -805,7 +805,7 @@ static void gdth_stop_timeout(int hanum, int busnum, int id)spin_lock_irqsave (&ha->smp_lock, flags);for (scp = ha->req_first; scp; scp = (Scsi_Cmnd *)scp->SCp.ptr) { -if (scp->done != gdth_scsi_done) { +if (!IS_GDTH_INTERNAL_CMD (scp)) { b = virt_ctr ?NUMDATA (scp->device->host)->busnum : scp->device->channel; t = scp->device->id;
drivers/scsi/gdth_proc.c static void gdth_start_timeout(int hanum, int busnum, int id) · line 835
Quoted 8 lines
drivers/scsi/gdth_proc.c +1
-1@@ -829,7 +829,7 @@ static void gdth_start_timeout(int hanum, int busnum, int id)spin_lock_irqsave (&ha->smp_lock, flags);for (scp = ha->req_first; scp; scp = (Scsi_Cmnd *)scp->SCp.ptr) { -if (scp->done != gdth_scsi_done) { +if (!IS_GDTH_INTERNAL_CMD (scp)) { b = virt_ctr ?NUMDATA (scp->device->host)->busnum : scp->device->channel; t = scp->device->id;
OK This is on top of the first patch that I will take care of, right? (http://www.spinics.net/lists/linux-scsi/msg19470.html)
Here 2 it will not merge with my patchset. So I'll take it off your hands and rebase it to mine.
In my patchset I have introduced a gdth_cmnd_priv. That is associated with every command and is put on cmnd->host_scribble, I than moved lots of abused Scp and other members to that new structure. I also put a simple boolean that states that these are Internal commands. So what is left of this patch is the:
- scp->done = gdth_scsi_done;
I will add that one line removal to your previous patch.
Quoted 2 lines
Ah; I was basing this on linus + scsi-misc. I guess
scsi_libhadn't been converted because I just took the code from there.
Yes scsi_lib was not converted, but scsi.c was.
Boaz
Re: [PATCH] gdth: Stop abusing ->done for internal commands
Quoted 2 lines · Boaz Harrosh
OK This is on top of the first patch that I will take care of, right? (http://www.spinics.net/lists/linux-scsi/msg19470.html)
That's right.
Quoted 2 lines
Here 2 it will not merge with my patchset. So I'll take it off your hands and rebase it to mine.
Wonderful. Again, I don't care about this driver, just want it to not break (more than it's already broken).
Quoted 7 lines
In my patchset I have introduced a
gdth_cmnd_priv. That is associated with every command and is put oncmnd->host_scribble, I than moved lots of abused Scp and other members to that new structure. I also put a simple boolean that states that these are Internal commands. So what is left of this patch is the:
-scp->done=gdth_scsi_done;
I will add that one line removal to your previous patch.
Excellent. Makes perfect sense.
Quoted 3 lines
Quoted 2 lines
Ah; I was basing this on linus + scsi-misc. I guess
scsi_libhadn't been converted because I just took the code from there.Yes
scsi_libwas not converted, butscsi.cwas.
OK. I've converted that in my tree now, and fixed a small whitespace error I introduced. I'll wait for more comments before posting a replacement patch.
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
This whole patch series looks very nice.