Hi everyone,
This is the fourth time I send this patch. For some reason it seems unable
to get any feedback at all. I'd really appreciate a clear ACK or NACK on
it and I'll keep resending it until it's either merged or I get a NACK
with a reason.
It is fairly trivial, so I'm naturally hoping for ACK+Merge (I really
would like to just get it out of my queue), but if that doesn't happen
please let me know why not.
This time around I've chosen to broaden the Cc list quite a bit in the
hope of getting some sort of feedback from someone (previously it was
just submitted to linux-kernel, linux-scsi and the SCSI maintainer).
The patch was previously submitted on
24 April 2008
27 April 2008
14 November 2010
This patch reduces the number of sequential pointer derefs in
drivers/scsi/scsi_error.c (and also removes a bit of trailing whitespace
this time around since I got fed up with it and thought that perhaps that
would help trigger a response - if nothing else then just a "do that in a
separate patch" would be an improvement).
It's on top of Linus' git tree as of just now (HEAD is at
2d42dc3feb6649c0e08641b0a6f0e0bad22aeeb2)
The bennefits are:
- makes the code easier to read. Lots of sequential derefs of the same
pointers is not easy on the eye.
- theoretically at least, just dereferencing the pointers once can allow
the compiler to generate slightly faster code, so in theory this could
also be a micro speed optimization.
- reduces size
before:
text data bss dec hex filename
11861 6 6 11873 2e61 drivers/scsi/scsi_error.o
after:
text data bss dec hex filename
11829 6 6 11841 2e41 drivers/scsi/scsi_error.o
- gets rid of annoying trailing whitespace.
Signed-off-by: Jesper Juhl <[email protected]>
---
scsi_error.c | 86 ++++++++++++++++++++++++++++++-----------------------------
1 file changed, 45 insertions(+), 41 deletions(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 824b8fc..ee91327 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -3,14 +3,14 @@
*
* SCSI error/timeout handling
* Initial versions: Eric Youngdale. Based upon conversations with
- * Leonard Zubkoff and David Miller at Linux Expo,
+ * Leonard Zubkoff and David Miller at Linux Expo,
* ideas originating from all over the place.
*
* Restructured scsi_unjam_host and associated functions.
* September 04, 2002 Mike Anderson ([email protected])
*
* Forward port of Russell King's ([email protected]) changes and
- * minor cleanups.
+ * minor cleanups.
* September 30, 2002 Mike Anderson ([email protected])
*/
@@ -129,14 +129,15 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
{
struct scsi_cmnd *scmd = req->special;
enum blk_eh_timer_return rtn = BLK_EH_NOT_HANDLED;
+ struct Scsi_Host *host = scmd->device->host;
trace_scsi_dispatch_cmd_timeout(scmd);
scsi_log_completion(scmd, TIMEOUT_ERROR);
- if (scmd->device->host->transportt->eh_timed_out)
- rtn = scmd->device->host->transportt->eh_timed_out(scmd);
- else if (scmd->device->host->hostt->eh_timed_out)
- rtn = scmd->device->host->hostt->eh_timed_out(scmd);
+ if (host->transportt->eh_timed_out)
+ rtn = host->transportt->eh_timed_out(scmd);
+ else if (host->hostt->eh_timed_out)
+ rtn = host->hostt->eh_timed_out(scmd);
if (unlikely(rtn == BLK_EH_NOT_HANDLED &&
!scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD))) {
@@ -195,7 +196,7 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
++total_failures;
if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD)
++cmd_cancel;
- else
+ else
++cmd_failed;
}
}
@@ -294,7 +295,7 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
return NEEDS_RETRY;
}
/*
- * if the device is in the process of becoming ready, we
+ * if the device is in the process of becoming ready, we
* should retry.
*/
if ((sshdr.asc == 0x04) && (sshdr.ascq == 0x01))
@@ -507,22 +508,23 @@ static int scsi_try_host_reset(struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct Scsi_Host *host = scmd->device->host;
+ struct scsi_host_template *hostt = host->hostt;
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Snd Host RST\n",
__func__));
- if (!scmd->device->host->hostt->eh_host_reset_handler)
+ if (!hostt->eh_host_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_host_reset_handler(scmd);
+ rtn = hostt->eh_host_reset_handler(scmd);
if (rtn == SUCCESS) {
- if (!scmd->device->host->hostt->skip_settle_delay)
+ if (!hostt->skip_settle_delay)
ssleep(HOST_RESET_SETTLE_TIME);
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
- scsi_report_bus_reset(scmd->device->host,
- scmd_channel(scmd));
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(host->host_lock, flags);
+ scsi_report_bus_reset(host, scmd_channel(scmd));
+ spin_unlock_irqrestore(host->host_lock, flags);
}
return rtn;
@@ -536,22 +538,23 @@ static int scsi_try_bus_reset(struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct Scsi_Host *host = scmd->device->host;
+ struct scsi_host_template *hostt = host->hostt;
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Snd Bus RST\n",
__func__));
- if (!scmd->device->host->hostt->eh_bus_reset_handler)
+ if (!hostt->eh_bus_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_bus_reset_handler(scmd);
+ rtn = hostt->eh_bus_reset_handler(scmd);
if (rtn == SUCCESS) {
- if (!scmd->device->host->hostt->skip_settle_delay)
+ if (!hostt->skip_settle_delay)
ssleep(BUS_RESET_SETTLE_TIME);
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
- scsi_report_bus_reset(scmd->device->host,
- scmd_channel(scmd));
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(host->host_lock, flags);
+ scsi_report_bus_reset(host, scmd_channel(scmd));
+ spin_unlock_irqrestore(host->host_lock, flags);
}
return rtn;
@@ -577,16 +580,18 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct Scsi_Host *host = scmd->device->host;
+ struct scsi_host_template *hostt = host->hostt;
- if (!scmd->device->host->hostt->eh_target_reset_handler)
+ if (!hostt->eh_target_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_target_reset_handler(scmd);
+ rtn = hostt->eh_target_reset_handler(scmd);
if (rtn == SUCCESS) {
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(host->host_lock, flags);
__starget_for_each_device(scsi_target(scmd->device), NULL,
__scsi_report_device_reset);
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_unlock_irqrestore(host->host_lock, flags);
}
return rtn;
@@ -605,11 +610,12 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
{
int rtn;
+ struct scsi_host_template *hostt = scmd->device->host->hostt;
- if (!scmd->device->host->hostt->eh_device_reset_handler)
+ if (!hostt->eh_device_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_device_reset_handler(scmd);
+ rtn = hostt->eh_device_reset_handler(scmd);
if (rtn == SUCCESS)
__scsi_report_device_reset(scmd->device, NULL);
return rtn;
@@ -617,10 +623,9 @@ static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
{
- if (!scmd->device->host->hostt->eh_abort_handler)
- return FAILED;
-
- return scmd->device->host->hostt->eh_abort_handler(scmd);
+ int (*eh_abort_handler)(struct scsi_cmnd *) =
+ scmd->device->host->hostt->eh_abort_handler;
+ return eh_abort_handler ? eh_abort_handler(scmd) : FAILED;
}
/**
@@ -867,7 +872,7 @@ EXPORT_SYMBOL(scsi_eh_finish_cmd);
*
* Description:
* See if we need to request sense information. if so, then get it
- * now, so we have a better idea of what to do.
+ * now, so we have a better idea of what to do.
*
* Notes:
* This has the unfortunate side effect that if a shost adapter does
@@ -987,7 +992,6 @@ static int scsi_eh_abort_cmds(struct list_head *work_q,
!scsi_eh_tur(scmd)) {
scsi_eh_finish_cmd(scmd, done_q);
}
-
} else
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: aborting"
" cmd failed:"
@@ -1031,7 +1035,7 @@ static int scsi_eh_try_stu(struct scsi_cmnd *scmd)
*
* Notes:
* If commands are failing due to not ready, initializing command required,
- * try revalidating the device, which will end up sending a start unit.
+ * try revalidating the device, which will end up sending a start unit.
*/
static int scsi_eh_stu(struct Scsi_Host *shost,
struct list_head *work_q,
@@ -1085,7 +1089,7 @@ static int scsi_eh_stu(struct Scsi_Host *shost,
* Try a bus device reset. Still, look to see whether we have multiple
* devices that are jammed or not - if we have multiple devices, it
* makes no sense to try bus_device_reset - we really would need to try
- * a bus_reset instead.
+ * a bus_reset instead.
*/
static int scsi_eh_bus_device_reset(struct Scsi_Host *shost,
struct list_head *work_q,
@@ -1196,7 +1200,7 @@ static int scsi_eh_target_reset(struct Scsi_Host *shost,
}
/**
- * scsi_eh_bus_reset - send a bus reset
+ * scsi_eh_bus_reset - send a bus reset
* @shost: &scsi host being recovered.
* @work_q: &list_head for pending commands.
* @done_q: &list_head for processed commands.
@@ -1213,7 +1217,7 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
* we really want to loop over the various channels, and do this on
* a channel by channel basis. we should also check to see if any
* of the failed commands are on soft_reset devices, and if so, skip
- * the reset.
+ * the reset.
*/
for (channel = 0; channel <= shost->max_channel; channel++) {
@@ -1255,7 +1259,7 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
}
/**
- * scsi_eh_host_reset - send a host reset
+ * scsi_eh_host_reset - send a host reset
* @work_q: list_head for processed commands.
* @done_q: list_head for processed commands.
*/
@@ -1408,7 +1412,7 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
return SUCCESS;
/*
* when the low level driver returns did_soft_error,
- * it is responsible for keeping an internal retry counter
+ * it is responsible for keeping an internal retry counter
* in order to avoid endless loops (db)
*
* actually this is a bug in this function here. we should
@@ -2005,7 +2009,7 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
if (sb_len > 7)
sshdr->additional_length = sense_buffer[7];
} else {
- /*
+ /*
* fixed format
*/
if (sb_len > 2)
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.
On Thu, Nov 18, 2010 at 11:30 AM, Jesper Juhl <[email protected]> wrote:
>
> This is the fourth time I send this patch. For some reason it seems unable
> to get any feedback at all. I'd really appreciate a clear ACK or NACK on
> it and I'll keep resending it until it's either merged or I get a NACK
> with a reason.
The patch looks ok to me, but you've basically selected the least
interesting file possible. No wonder people can't seem to care.
Also, this is just ugly as hell, and doesn't help anything:
+ int (*eh_abort_handler)(struct scsi_cmnd *) =
+ scmd->device->host->hostt->eh_abort_handler;
since the compiler will have optimized that double access away anyway
(no writes in between). So you could have made it about a thousand
times more readable with no downside by doing
struct scsi_host_template *hostt = scmd->device->host->hostt;
if (!hostt->eh_abort_handler)
return FAILED;
return hostt->eh_abort_handler(scmd);
instead. Look ma, no long lines.
Rule of thumb: if you need more than one line for an expression or
variable definition, you're doing something wrong.
Linus
On Thu, 18 Nov 2010, Linus Torvalds wrote:
> On Thu, Nov 18, 2010 at 11:30 AM, Jesper Juhl <[email protected]> wrote:
> >
> > This is the fourth time I send this patch. For some reason it seems unable
> > to get any feedback at all. I'd really appreciate a clear ACK or NACK on
> > it and I'll keep resending it until it's either merged or I get a NACK
> > with a reason.
>
> The patch looks ok to me, but you've basically selected the least
> interesting file possible. No wonder people can't seem to care.
>
Thank you very much for reviewing the patch and replying with your
feedback!
I agree that this is not the most sexy file in the tree to be modifying,
but I'm not seeking fame and fortune by doing this, just trying to
optimize the few corners I find - every little bit helps; right?
> Also, this is just ugly as hell, and doesn't help anything:
>
> + int (*eh_abort_handler)(struct scsi_cmnd *) =
> + scmd->device->host->hostt->eh_abort_handler;
>
> since the compiler will have optimized that double access away anyway
> (no writes in between). So you could have made it about a thousand
> times more readable with no downside by doing
>
> struct scsi_host_template *hostt = scmd->device->host->hostt;
>
> if (!hostt->eh_abort_handler)
> return FAILED;
> return hostt->eh_abort_handler(scmd);
>
> instead. Look ma, no long lines.
>
> Rule of thumb: if you need more than one line for an expression or
> variable definition, you're doing something wrong.
>
Fair enough. Seeing your version of this and looking a second time at mine
I have to agree completely.
You are absolutely right in stating that the compiler will handle this
just fine, so it's only a readabillity issue and your version is a *lot*
more readable than what I came up with.
I've updated the patch below - the original changelog bit still applies,
only change is what you pointed out above.
Signed-off-by: Jesper Juhl <[email protected]>
---
scsi_error.c | 87 +++++++++++++++++++++++++++++++----------------------------
1 file changed, 46 insertions(+), 41 deletions(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 824b8fc..8dc02c6 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -3,14 +3,14 @@
*
* SCSI error/timeout handling
* Initial versions: Eric Youngdale. Based upon conversations with
- * Leonard Zubkoff and David Miller at Linux Expo,
+ * Leonard Zubkoff and David Miller at Linux Expo,
* ideas originating from all over the place.
*
* Restructured scsi_unjam_host and associated functions.
* September 04, 2002 Mike Anderson ([email protected])
*
* Forward port of Russell King's ([email protected]) changes and
- * minor cleanups.
+ * minor cleanups.
* September 30, 2002 Mike Anderson ([email protected])
*/
@@ -129,14 +129,15 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
{
struct scsi_cmnd *scmd = req->special;
enum blk_eh_timer_return rtn = BLK_EH_NOT_HANDLED;
+ struct Scsi_Host *host = scmd->device->host;
trace_scsi_dispatch_cmd_timeout(scmd);
scsi_log_completion(scmd, TIMEOUT_ERROR);
- if (scmd->device->host->transportt->eh_timed_out)
- rtn = scmd->device->host->transportt->eh_timed_out(scmd);
- else if (scmd->device->host->hostt->eh_timed_out)
- rtn = scmd->device->host->hostt->eh_timed_out(scmd);
+ if (host->transportt->eh_timed_out)
+ rtn = host->transportt->eh_timed_out(scmd);
+ else if (host->hostt->eh_timed_out)
+ rtn = host->hostt->eh_timed_out(scmd);
if (unlikely(rtn == BLK_EH_NOT_HANDLED &&
!scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD))) {
@@ -195,7 +196,7 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
++total_failures;
if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD)
++cmd_cancel;
- else
+ else
++cmd_failed;
}
}
@@ -294,7 +295,7 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
return NEEDS_RETRY;
}
/*
- * if the device is in the process of becoming ready, we
+ * if the device is in the process of becoming ready, we
* should retry.
*/
if ((sshdr.asc == 0x04) && (sshdr.ascq == 0x01))
@@ -507,22 +508,23 @@ static int scsi_try_host_reset(struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct Scsi_Host *host = scmd->device->host;
+ struct scsi_host_template *hostt = host->hostt;
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Snd Host RST\n",
__func__));
- if (!scmd->device->host->hostt->eh_host_reset_handler)
+ if (!hostt->eh_host_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_host_reset_handler(scmd);
+ rtn = hostt->eh_host_reset_handler(scmd);
if (rtn == SUCCESS) {
- if (!scmd->device->host->hostt->skip_settle_delay)
+ if (!hostt->skip_settle_delay)
ssleep(HOST_RESET_SETTLE_TIME);
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
- scsi_report_bus_reset(scmd->device->host,
- scmd_channel(scmd));
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(host->host_lock, flags);
+ scsi_report_bus_reset(host, scmd_channel(scmd));
+ spin_unlock_irqrestore(host->host_lock, flags);
}
return rtn;
@@ -536,22 +538,23 @@ static int scsi_try_bus_reset(struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct Scsi_Host *host = scmd->device->host;
+ struct scsi_host_template *hostt = host->hostt;
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Snd Bus RST\n",
__func__));
- if (!scmd->device->host->hostt->eh_bus_reset_handler)
+ if (!hostt->eh_bus_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_bus_reset_handler(scmd);
+ rtn = hostt->eh_bus_reset_handler(scmd);
if (rtn == SUCCESS) {
- if (!scmd->device->host->hostt->skip_settle_delay)
+ if (!hostt->skip_settle_delay)
ssleep(BUS_RESET_SETTLE_TIME);
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
- scsi_report_bus_reset(scmd->device->host,
- scmd_channel(scmd));
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(host->host_lock, flags);
+ scsi_report_bus_reset(host, scmd_channel(scmd));
+ spin_unlock_irqrestore(host->host_lock, flags);
}
return rtn;
@@ -577,16 +580,18 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct Scsi_Host *host = scmd->device->host;
+ struct scsi_host_template *hostt = host->hostt;
- if (!scmd->device->host->hostt->eh_target_reset_handler)
+ if (!hostt->eh_target_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_target_reset_handler(scmd);
+ rtn = hostt->eh_target_reset_handler(scmd);
if (rtn == SUCCESS) {
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(host->host_lock, flags);
__starget_for_each_device(scsi_target(scmd->device), NULL,
__scsi_report_device_reset);
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_unlock_irqrestore(host->host_lock, flags);
}
return rtn;
@@ -605,11 +610,12 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
{
int rtn;
+ struct scsi_host_template *hostt = scmd->device->host->hostt;
- if (!scmd->device->host->hostt->eh_device_reset_handler)
+ if (!hostt->eh_device_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_device_reset_handler(scmd);
+ rtn = hostt->eh_device_reset_handler(scmd);
if (rtn == SUCCESS)
__scsi_report_device_reset(scmd->device, NULL);
return rtn;
@@ -617,10 +623,10 @@ static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
{
- if (!scmd->device->host->hostt->eh_abort_handler)
- return FAILED;
-
- return scmd->device->host->hostt->eh_abort_handler(scmd);
+ struct scsi_host_template *hostt = scmd->device->host->hostt;
+ if (!hostt->eh_abort_handler)
+ return FAILED;
+ return hostt->eh_abort_handler(scmd);
}
/**
@@ -867,7 +873,7 @@ EXPORT_SYMBOL(scsi_eh_finish_cmd);
*
* Description:
* See if we need to request sense information. if so, then get it
- * now, so we have a better idea of what to do.
+ * now, so we have a better idea of what to do.
*
* Notes:
* This has the unfortunate side effect that if a shost adapter does
@@ -987,7 +993,6 @@ static int scsi_eh_abort_cmds(struct list_head *work_q,
!scsi_eh_tur(scmd)) {
scsi_eh_finish_cmd(scmd, done_q);
}
-
} else
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: aborting"
" cmd failed:"
@@ -1031,7 +1036,7 @@ static int scsi_eh_try_stu(struct scsi_cmnd *scmd)
*
* Notes:
* If commands are failing due to not ready, initializing command required,
- * try revalidating the device, which will end up sending a start unit.
+ * try revalidating the device, which will end up sending a start unit.
*/
static int scsi_eh_stu(struct Scsi_Host *shost,
struct list_head *work_q,
@@ -1085,7 +1090,7 @@ static int scsi_eh_stu(struct Scsi_Host *shost,
* Try a bus device reset. Still, look to see whether we have multiple
* devices that are jammed or not - if we have multiple devices, it
* makes no sense to try bus_device_reset - we really would need to try
- * a bus_reset instead.
+ * a bus_reset instead.
*/
static int scsi_eh_bus_device_reset(struct Scsi_Host *shost,
struct list_head *work_q,
@@ -1196,7 +1201,7 @@ static int scsi_eh_target_reset(struct Scsi_Host *shost,
}
/**
- * scsi_eh_bus_reset - send a bus reset
+ * scsi_eh_bus_reset - send a bus reset
* @shost: &scsi host being recovered.
* @work_q: &list_head for pending commands.
* @done_q: &list_head for processed commands.
@@ -1213,7 +1218,7 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
* we really want to loop over the various channels, and do this on
* a channel by channel basis. we should also check to see if any
* of the failed commands are on soft_reset devices, and if so, skip
- * the reset.
+ * the reset.
*/
for (channel = 0; channel <= shost->max_channel; channel++) {
@@ -1255,7 +1260,7 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
}
/**
- * scsi_eh_host_reset - send a host reset
+ * scsi_eh_host_reset - send a host reset
* @work_q: list_head for processed commands.
* @done_q: list_head for processed commands.
*/
@@ -1408,7 +1413,7 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
return SUCCESS;
/*
* when the low level driver returns did_soft_error,
- * it is responsible for keeping an internal retry counter
+ * it is responsible for keeping an internal retry counter
* in order to avoid endless loops (db)
*
* actually this is a bug in this function here. we should
@@ -2005,7 +2010,7 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
if (sb_len > 7)
sshdr->additional_length = sense_buffer[7];
} else {
- /*
+ /*
* fixed format
*/
if (sb_len > 2)
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.
On Thu, Nov 18, 2010 at 09:49:29PM +0100, Jesper Juhl wrote:
> @@ -617,10 +623,10 @@ static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
>
> static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
> {
> - if (!scmd->device->host->hostt->eh_abort_handler)
> - return FAILED;
> -
> - return scmd->device->host->hostt->eh_abort_handler(scmd);
> + struct scsi_host_template *hostt = scmd->device->host->hostt;
> + if (!hostt->eh_abort_handler)
> + return FAILED;
> + return hostt->eh_abort_handler(scmd);
Not that I have much to do with SCSI anymore... I spotted the above.
Is there any particular reason for using spaces to indent here rather
than our usual tabs?
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Thu, 18 Nov 2010, Jesper Juhl wrote:
> On Thu, 18 Nov 2010, Linus Torvalds wrote:
>
> > On Thu, Nov 18, 2010 at 11:30 AM, Jesper Juhl <[email protected]> wrote:
> > >
> > > This is the fourth time I send this patch. For some reason it seems unable
> > > to get any feedback at all. I'd really appreciate a clear ACK or NACK on
> > > it and I'll keep resending it until it's either merged or I get a NACK
> > > with a reason.
> >
> > The patch looks ok to me, but you've basically selected the least
> > interesting file possible. No wonder people can't seem to care.
> >
>
> Thank you very much for reviewing the patch and replying with your
> feedback!
>
> I agree that this is not the most sexy file in the tree to be modifying,
> but I'm not seeking fame and fortune by doing this, just trying to
> optimize the few corners I find - every little bit helps; right?
>
>
> > Also, this is just ugly as hell, and doesn't help anything:
> >
> > + int (*eh_abort_handler)(struct scsi_cmnd *) =
> > + scmd->device->host->hostt->eh_abort_handler;
> >
> > since the compiler will have optimized that double access away anyway
> > (no writes in between). So you could have made it about a thousand
> > times more readable with no downside by doing
> >
> > struct scsi_host_template *hostt = scmd->device->host->hostt;
> >
> > if (!hostt->eh_abort_handler)
> > return FAILED;
> > return hostt->eh_abort_handler(scmd);
> >
> > instead. Look ma, no long lines.
> >
> > Rule of thumb: if you need more than one line for an expression or
> > variable definition, you're doing something wrong.
> >
>
> Fair enough. Seeing your version of this and looking a second time at mine
> I have to agree completely.
> You are absolutely right in stating that the compiler will handle this
> just fine, so it's only a readabillity issue and your version is a *lot*
> more readable than what I came up with.
>
> I've updated the patch below - the original changelog bit still applies,
> only change is what you pointed out above.
>
Aww crap, emacs just screwed me over on this one - that'll teach me to
keep my .emacs files in sync across machines. Sorry about that, the
previous mail used spaces for indentation not tabs - this should be
better:
Signed-off-by: Jesper Juhl <[email protected]>
---
scsi_error.c | 85 +++++++++++++++++++++++++++++++----------------------------
1 file changed, 45 insertions(+), 40 deletions(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 824b8fc..ef89f3f 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -3,14 +3,14 @@
*
* SCSI error/timeout handling
* Initial versions: Eric Youngdale. Based upon conversations with
- * Leonard Zubkoff and David Miller at Linux Expo,
+ * Leonard Zubkoff and David Miller at Linux Expo,
* ideas originating from all over the place.
*
* Restructured scsi_unjam_host and associated functions.
* September 04, 2002 Mike Anderson ([email protected])
*
* Forward port of Russell King's ([email protected]) changes and
- * minor cleanups.
+ * minor cleanups.
* September 30, 2002 Mike Anderson ([email protected])
*/
@@ -129,14 +129,15 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
{
struct scsi_cmnd *scmd = req->special;
enum blk_eh_timer_return rtn = BLK_EH_NOT_HANDLED;
+ struct Scsi_Host *host = scmd->device->host;
trace_scsi_dispatch_cmd_timeout(scmd);
scsi_log_completion(scmd, TIMEOUT_ERROR);
- if (scmd->device->host->transportt->eh_timed_out)
- rtn = scmd->device->host->transportt->eh_timed_out(scmd);
- else if (scmd->device->host->hostt->eh_timed_out)
- rtn = scmd->device->host->hostt->eh_timed_out(scmd);
+ if (host->transportt->eh_timed_out)
+ rtn = host->transportt->eh_timed_out(scmd);
+ else if (host->hostt->eh_timed_out)
+ rtn = host->hostt->eh_timed_out(scmd);
if (unlikely(rtn == BLK_EH_NOT_HANDLED &&
!scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD))) {
@@ -195,7 +196,7 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
++total_failures;
if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD)
++cmd_cancel;
- else
+ else
++cmd_failed;
}
}
@@ -294,7 +295,7 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
return NEEDS_RETRY;
}
/*
- * if the device is in the process of becoming ready, we
+ * if the device is in the process of becoming ready, we
* should retry.
*/
if ((sshdr.asc == 0x04) && (sshdr.ascq == 0x01))
@@ -507,22 +508,23 @@ static int scsi_try_host_reset(struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct Scsi_Host *host = scmd->device->host;
+ struct scsi_host_template *hostt = host->hostt;
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Snd Host RST\n",
__func__));
- if (!scmd->device->host->hostt->eh_host_reset_handler)
+ if (!hostt->eh_host_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_host_reset_handler(scmd);
+ rtn = hostt->eh_host_reset_handler(scmd);
if (rtn == SUCCESS) {
- if (!scmd->device->host->hostt->skip_settle_delay)
+ if (!hostt->skip_settle_delay)
ssleep(HOST_RESET_SETTLE_TIME);
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
- scsi_report_bus_reset(scmd->device->host,
- scmd_channel(scmd));
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(host->host_lock, flags);
+ scsi_report_bus_reset(host, scmd_channel(scmd));
+ spin_unlock_irqrestore(host->host_lock, flags);
}
return rtn;
@@ -536,22 +538,23 @@ static int scsi_try_bus_reset(struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct Scsi_Host *host = scmd->device->host;
+ struct scsi_host_template *hostt = host->hostt;
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Snd Bus RST\n",
__func__));
- if (!scmd->device->host->hostt->eh_bus_reset_handler)
+ if (!hostt->eh_bus_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_bus_reset_handler(scmd);
+ rtn = hostt->eh_bus_reset_handler(scmd);
if (rtn == SUCCESS) {
- if (!scmd->device->host->hostt->skip_settle_delay)
+ if (!hostt->skip_settle_delay)
ssleep(BUS_RESET_SETTLE_TIME);
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
- scsi_report_bus_reset(scmd->device->host,
- scmd_channel(scmd));
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(host->host_lock, flags);
+ scsi_report_bus_reset(host, scmd_channel(scmd));
+ spin_unlock_irqrestore(host->host_lock, flags);
}
return rtn;
@@ -577,16 +580,18 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct Scsi_Host *host = scmd->device->host;
+ struct scsi_host_template *hostt = host->hostt;
- if (!scmd->device->host->hostt->eh_target_reset_handler)
+ if (!hostt->eh_target_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_target_reset_handler(scmd);
+ rtn = hostt->eh_target_reset_handler(scmd);
if (rtn == SUCCESS) {
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(host->host_lock, flags);
__starget_for_each_device(scsi_target(scmd->device), NULL,
__scsi_report_device_reset);
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_unlock_irqrestore(host->host_lock, flags);
}
return rtn;
@@ -605,11 +610,12 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
{
int rtn;
+ struct scsi_host_template *hostt = scmd->device->host->hostt;
- if (!scmd->device->host->hostt->eh_device_reset_handler)
+ if (!hostt->eh_device_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_device_reset_handler(scmd);
+ rtn = hostt->eh_device_reset_handler(scmd);
if (rtn == SUCCESS)
__scsi_report_device_reset(scmd->device, NULL);
return rtn;
@@ -617,10 +623,10 @@ static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
{
- if (!scmd->device->host->hostt->eh_abort_handler)
+ struct scsi_host_template *hostt = scmd->device->host->hostt;
+ if (!hostt->eh_abort_handler)
return FAILED;
-
- return scmd->device->host->hostt->eh_abort_handler(scmd);
+ return hostt->eh_abort_handler(scmd);
}
/**
@@ -867,7 +873,7 @@ EXPORT_SYMBOL(scsi_eh_finish_cmd);
*
* Description:
* See if we need to request sense information. if so, then get it
- * now, so we have a better idea of what to do.
+ * now, so we have a better idea of what to do.
*
* Notes:
* This has the unfortunate side effect that if a shost adapter does
@@ -987,7 +993,6 @@ static int scsi_eh_abort_cmds(struct list_head *work_q,
!scsi_eh_tur(scmd)) {
scsi_eh_finish_cmd(scmd, done_q);
}
-
} else
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: aborting"
" cmd failed:"
@@ -1031,7 +1036,7 @@ static int scsi_eh_try_stu(struct scsi_cmnd *scmd)
*
* Notes:
* If commands are failing due to not ready, initializing command required,
- * try revalidating the device, which will end up sending a start unit.
+ * try revalidating the device, which will end up sending a start unit.
*/
static int scsi_eh_stu(struct Scsi_Host *shost,
struct list_head *work_q,
@@ -1085,7 +1090,7 @@ static int scsi_eh_stu(struct Scsi_Host *shost,
* Try a bus device reset. Still, look to see whether we have multiple
* devices that are jammed or not - if we have multiple devices, it
* makes no sense to try bus_device_reset - we really would need to try
- * a bus_reset instead.
+ * a bus_reset instead.
*/
static int scsi_eh_bus_device_reset(struct Scsi_Host *shost,
struct list_head *work_q,
@@ -1196,7 +1201,7 @@ static int scsi_eh_target_reset(struct Scsi_Host *shost,
}
/**
- * scsi_eh_bus_reset - send a bus reset
+ * scsi_eh_bus_reset - send a bus reset
* @shost: &scsi host being recovered.
* @work_q: &list_head for pending commands.
* @done_q: &list_head for processed commands.
@@ -1213,7 +1218,7 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
* we really want to loop over the various channels, and do this on
* a channel by channel basis. we should also check to see if any
* of the failed commands are on soft_reset devices, and if so, skip
- * the reset.
+ * the reset.
*/
for (channel = 0; channel <= shost->max_channel; channel++) {
@@ -1255,7 +1260,7 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
}
/**
- * scsi_eh_host_reset - send a host reset
+ * scsi_eh_host_reset - send a host reset
* @work_q: list_head for processed commands.
* @done_q: list_head for processed commands.
*/
@@ -1408,7 +1413,7 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
return SUCCESS;
/*
* when the low level driver returns did_soft_error,
- * it is responsible for keeping an internal retry counter
+ * it is responsible for keeping an internal retry counter
* in order to avoid endless loops (db)
*
* actually this is a bug in this function here. we should
@@ -2005,7 +2010,7 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
if (sb_len > 7)
sshdr->additional_length = sense_buffer[7];
} else {
- /*
+ /*
* fixed format
*/
if (sb_len > 2)
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.
On Thu, 18 Nov 2010, Russell King wrote:
> On Thu, Nov 18, 2010 at 09:49:29PM +0100, Jesper Juhl wrote:
> > @@ -617,10 +623,10 @@ static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
> >
> > static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
> > {
> > - if (!scmd->device->host->hostt->eh_abort_handler)
> > - return FAILED;
> > -
> > - return scmd->device->host->hostt->eh_abort_handler(scmd);
> > + struct scsi_host_template *hostt = scmd->device->host->hostt;
> > + if (!hostt->eh_abort_handler)
> > + return FAILED;
> > + return hostt->eh_abort_handler(scmd);
>
> Not that I have much to do with SCSI anymore... I spotted the above.
> Is there any particular reason for using spaces to indent here rather
> than our usual tabs?
>
No. That was me and Emacs having a slight disagreement (see the mail I
just sent)...
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.
On Thu, Nov 18, 2010 at 12:49 PM, Jesper Juhl <[email protected]> wrote:
>
> Fair enough. Seeing your version of this and looking a second time at mine
> I have to agree completely.
> You are absolutely right in stating that the compiler will handle this
> just fine, so it's only a readabillity issue and your version is a *lot*
> more readable than what I came up with.
Btw, one thing to look out for is all those function calls: it really
looks like many of them would be better off not having to dereference
the thing inside each helper function, but just have it dereferenced
in the caller.
You might trying passing in "struct Scsi_Host *host" to a lot of those
helper functions in addition to the 'scmd' part. There's a lot of
"scmd->device->host" going on, and even if you remove some of them
_within_ a function, if you really want to get rid of them you should
probably do one of them in the caller.
That's why the queuecommand() function was changed to take
struct Scsi_Host *h, struct scsi_cmnd *
as its arguments, because that host is used so commonly. And passing
two arguments is usually free (ie almost all architectures pass it in
registers), and that host variable almost always already exists in the
caller because the caller already needed it.
So if you changed the functions that only take "scsi_cmnd *" as an
argument to match the new queuecommand() interface, I bet you'd get
more cleanups. And the interfaces would match.
Linus
On Thu, 18 Nov 2010, Linus Torvalds wrote:
> On Thu, Nov 18, 2010 at 12:49 PM, Jesper Juhl <[email protected]> wrote:
> >
> > Fair enough. Seeing your version of this and looking a second time at mine
> > I have to agree completely.
> > You are absolutely right in stating that the compiler will handle this
> > just fine, so it's only a readabillity issue and your version is a *lot*
> > more readable than what I came up with.
>
> Btw, one thing to look out for is all those function calls: it really
> looks like many of them would be better off not having to dereference
> the thing inside each helper function, but just have it dereferenced
> in the caller.
>
> You might trying passing in "struct Scsi_Host *host" to a lot of those
> helper functions in addition to the 'scmd' part. There's a lot of
> "scmd->device->host" going on, and even if you remove some of them
> _within_ a function, if you really want to get rid of them you should
> probably do one of them in the caller.
>
> That's why the queuecommand() function was changed to take
>
> struct Scsi_Host *h, struct scsi_cmnd *
>
> as its arguments, because that host is used so commonly. And passing
> two arguments is usually free (ie almost all architectures pass it in
> registers), and that host variable almost always already exists in the
> caller because the caller already needed it.
>
> So if you changed the functions that only take "scsi_cmnd *" as an
> argument to match the new queuecommand() interface, I bet you'd get
> more cleanups. And the interfaces would match.
>
That's a good point.
I don't have time to look at that tonight, but I'll (hopefully) get around
to it over the weekend. I'll submit a new patch (or at least email) during
the weekend (or early next week) when I've had some time to look into
that.
Thanks.
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.
On Thu, 18 Nov 2010, Linus Torvalds wrote:
> On Thu, Nov 18, 2010 at 12:49 PM, Jesper Juhl <[email protected]> wrote:
> >
> > Fair enough. Seeing your version of this and looking a second time at mine
> > I have to agree completely.
> > You are absolutely right in stating that the compiler will handle this
> > just fine, so it's only a readabillity issue and your version is a *lot*
> > more readable than what I came up with.
>
> Btw, one thing to look out for is all those function calls: it really
> looks like many of them would be better off not having to dereference
> the thing inside each helper function, but just have it dereferenced
> in the caller.
>
> You might trying passing in "struct Scsi_Host *host" to a lot of those
> helper functions in addition to the 'scmd' part. There's a lot of
> "scmd->device->host" going on, and even if you remove some of them
> _within_ a function, if you really want to get rid of them you should
> probably do one of them in the caller.
>
> That's why the queuecommand() function was changed to take
>
> struct Scsi_Host *h, struct scsi_cmnd *
>
> as its arguments, because that host is used so commonly. And passing
> two arguments is usually free (ie almost all architectures pass it in
> registers), and that host variable almost always already exists in the
> caller because the caller already needed it.
>
> So if you changed the functions that only take "scsi_cmnd *" as an
> argument to match the new queuecommand() interface, I bet you'd get
> more cleanups. And the interfaces would match.
>
Ok, I tried doing that (see patch below), but the result ended up as a
larger object file.
text data bss dec hex filename
11861 6 6 11873 2e61 drivers/scsi/scsi_error.o
So, I think (the 'interfaces matching' argument aside) that I'll stick to
my previous version of the patch. Do you disagree?
If not, perhaps you could ACK the previous version?
This is the alternative I tried:
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 824b8fc..c44a08e 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -3,14 +3,14 @@
*
* SCSI error/timeout handling
* Initial versions: Eric Youngdale. Based upon conversations with
- * Leonard Zubkoff and David Miller at Linux Expo,
+ * Leonard Zubkoff and David Miller at Linux Expo,
* ideas originating from all over the place.
*
* Restructured scsi_unjam_host and associated functions.
* September 04, 2002 Mike Anderson ([email protected])
*
* Forward port of Russell King's ([email protected]) changes and
- * minor cleanups.
+ * minor cleanups.
* September 30, 2002 Mike Anderson ([email protected])
*/
@@ -91,27 +91,26 @@ EXPORT_SYMBOL_GPL(scsi_schedule_eh);
* Return value:
* 0 on failure.
*/
-int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
+int scsi_eh_scmd_add(struct Scsi_Host *h, struct scsi_cmnd *scmd, int eh_flag)
{
- struct Scsi_Host *shost = scmd->device->host;
unsigned long flags;
int ret = 0;
- if (!shost->ehandler)
+ if (!h->ehandler)
return 0;
- spin_lock_irqsave(shost->host_lock, flags);
- if (scsi_host_set_state(shost, SHOST_RECOVERY))
- if (scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY))
+ spin_lock_irqsave(h->host_lock, flags);
+ if (scsi_host_set_state(h, SHOST_RECOVERY))
+ if (scsi_host_set_state(h, SHOST_CANCEL_RECOVERY))
goto out_unlock;
ret = 1;
scmd->eh_eflags |= eh_flag;
- list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
- shost->host_failed++;
- scsi_eh_wakeup(shost);
+ list_add_tail(&scmd->eh_entry, &h->eh_cmd_q);
+ h->host_failed++;
+ scsi_eh_wakeup(h);
out_unlock:
- spin_unlock_irqrestore(shost->host_lock, flags);
+ spin_unlock_irqrestore(h->host_lock, flags);
return ret;
}
@@ -125,7 +124,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
* normal completion function determines that the timer has already
* fired, then it mustn't do anything.
*/
-enum blk_eh_timer_return scsi_times_out(struct request *req)
+enum blk_eh_timer_return scsi_times_out(struct Scsi_Host *h, struct request *req)
{
struct scsi_cmnd *scmd = req->special;
enum blk_eh_timer_return rtn = BLK_EH_NOT_HANDLED;
@@ -133,13 +132,14 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
trace_scsi_dispatch_cmd_timeout(scmd);
scsi_log_completion(scmd, TIMEOUT_ERROR);
- if (scmd->device->host->transportt->eh_timed_out)
- rtn = scmd->device->host->transportt->eh_timed_out(scmd);
- else if (scmd->device->host->hostt->eh_timed_out)
- rtn = scmd->device->host->hostt->eh_timed_out(scmd);
+ if (h->transportt->eh_timed_out)
+ rtn = h->transportt->eh_timed_out(scmd);
+ else if (h->hostt->eh_timed_out)
+ rtn = h->hostt->eh_timed_out(scmd);
if (unlikely(rtn == BLK_EH_NOT_HANDLED &&
- !scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD))) {
+ !scsi_eh_scmd_add(scmd->device->host, scmd,
+ SCSI_EH_CANCEL_CMD))) {
scmd->result |= DID_TIME_OUT << 16;
rtn = BLK_EH_HANDLED;
}
@@ -195,7 +195,7 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
++total_failures;
if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD)
++cmd_cancel;
- else
+ else
++cmd_failed;
}
}
@@ -294,7 +294,7 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
return NEEDS_RETRY;
}
/*
- * if the device is in the process of becoming ready, we
+ * if the device is in the process of becoming ready, we
* should retry.
*/
if ((sshdr.asc == 0x04) && (sshdr.ascq == 0x01))
@@ -503,26 +503,26 @@ static void scsi_eh_done(struct scsi_cmnd *scmd)
* scsi_try_host_reset - ask host adapter to reset itself
* @scmd: SCSI cmd to send hsot reset.
*/
-static int scsi_try_host_reset(struct scsi_cmnd *scmd)
+static int scsi_try_host_reset(struct Scsi_Host *h, struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct scsi_host_template *hostt = h->hostt;
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Snd Host RST\n",
__func__));
- if (!scmd->device->host->hostt->eh_host_reset_handler)
+ if (!hostt->eh_host_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_host_reset_handler(scmd);
+ rtn = hostt->eh_host_reset_handler(scmd);
if (rtn == SUCCESS) {
- if (!scmd->device->host->hostt->skip_settle_delay)
+ if (!hostt->skip_settle_delay)
ssleep(HOST_RESET_SETTLE_TIME);
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
- scsi_report_bus_reset(scmd->device->host,
- scmd_channel(scmd));
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(h->host_lock, flags);
+ scsi_report_bus_reset(h, scmd_channel(scmd));
+ spin_unlock_irqrestore(h->host_lock, flags);
}
return rtn;
@@ -532,26 +532,26 @@ static int scsi_try_host_reset(struct scsi_cmnd *scmd)
* scsi_try_bus_reset - ask host to perform a bus reset
* @scmd: SCSI cmd to send bus reset.
*/
-static int scsi_try_bus_reset(struct scsi_cmnd *scmd)
+static int scsi_try_bus_reset(struct Scsi_Host *h, struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct scsi_host_template *hostt = h->hostt;
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Snd Bus RST\n",
__func__));
- if (!scmd->device->host->hostt->eh_bus_reset_handler)
+ if (!hostt->eh_bus_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_bus_reset_handler(scmd);
+ rtn = hostt->eh_bus_reset_handler(scmd);
if (rtn == SUCCESS) {
- if (!scmd->device->host->hostt->skip_settle_delay)
+ if (!hostt->skip_settle_delay)
ssleep(BUS_RESET_SETTLE_TIME);
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
- scsi_report_bus_reset(scmd->device->host,
- scmd_channel(scmd));
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(h->host_lock, flags);
+ scsi_report_bus_reset(h, scmd_channel(scmd));
+ spin_unlock_irqrestore(h->host_lock, flags);
}
return rtn;
@@ -573,20 +573,21 @@ static void __scsi_report_device_reset(struct scsi_device *sdev, void *data)
* timer on it, and set the host back to a consistent state prior to
* returning.
*/
-static int scsi_try_target_reset(struct scsi_cmnd *scmd)
+static int scsi_try_target_reset(struct Scsi_Host *h, struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct scsi_host_template *hostt = h->hostt;
- if (!scmd->device->host->hostt->eh_target_reset_handler)
+ if (!hostt->eh_target_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_target_reset_handler(scmd);
+ rtn = hostt->eh_target_reset_handler(scmd);
if (rtn == SUCCESS) {
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(h->host_lock, flags);
__starget_for_each_device(scsi_target(scmd->device), NULL,
__scsi_report_device_reset);
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_unlock_irqrestore(h->host_lock, flags);
}
return rtn;
@@ -602,14 +603,16 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
* timer on it, and set the host back to a consistent state prior to
* returning.
*/
+
static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
{
int rtn;
+ struct scsi_host_template *hostt = scmd->device->host->hostt;
- if (!scmd->device->host->hostt->eh_device_reset_handler)
+ if (!hostt->eh_device_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_device_reset_handler(scmd);
+ rtn = hostt->eh_device_reset_handler(scmd);
if (rtn == SUCCESS)
__scsi_report_device_reset(scmd->device, NULL);
return rtn;
@@ -617,10 +620,10 @@ static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
{
- if (!scmd->device->host->hostt->eh_abort_handler)
+ struct scsi_host_template *hostt = scmd->device->host->hostt;
+ if (!hostt->eh_abort_handler)
return FAILED;
-
- return scmd->device->host->hostt->eh_abort_handler(scmd);
+ return hostt->eh_abort_handler(scmd);
}
/**
@@ -647,11 +650,14 @@ static int scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
{
+ struct Scsi_Host *h;
if (__scsi_try_to_abort_cmd(scmd) != SUCCESS)
- if (scsi_try_bus_device_reset(scmd) != SUCCESS)
- if (scsi_try_target_reset(scmd) != SUCCESS)
- if (scsi_try_bus_reset(scmd) != SUCCESS)
- scsi_try_host_reset(scmd);
+ if (scsi_try_bus_device_reset(scmd) != SUCCESS) {
+ h = scmd->device->host;
+ if (scsi_try_target_reset(h, scmd) != SUCCESS)
+ if (scsi_try_bus_reset(h, scmd) != SUCCESS)
+ scsi_try_host_reset(h, scmd);
+ }
}
/**
@@ -867,7 +873,7 @@ EXPORT_SYMBOL(scsi_eh_finish_cmd);
*
* Description:
* See if we need to request sense information. if so, then get it
- * now, so we have a better idea of what to do.
+ * now, so we have a better idea of what to do.
*
* Notes:
* This has the unfortunate side effect that if a shost adapter does
@@ -987,7 +993,6 @@ static int scsi_eh_abort_cmds(struct list_head *work_q,
!scsi_eh_tur(scmd)) {
scsi_eh_finish_cmd(scmd, done_q);
}
-
} else
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: aborting"
" cmd failed:"
@@ -1031,7 +1036,7 @@ static int scsi_eh_try_stu(struct scsi_cmnd *scmd)
*
* Notes:
* If commands are failing due to not ready, initializing command required,
- * try revalidating the device, which will end up sending a start unit.
+ * try revalidating the device, which will end up sending a start unit.
*/
static int scsi_eh_stu(struct Scsi_Host *shost,
struct list_head *work_q,
@@ -1085,7 +1090,7 @@ static int scsi_eh_stu(struct Scsi_Host *shost,
* Try a bus device reset. Still, look to see whether we have multiple
* devices that are jammed or not - if we have multiple devices, it
* makes no sense to try bus_device_reset - we really would need to try
- * a bus_reset instead.
+ * a bus_reset instead.
*/
static int scsi_eh_bus_device_reset(struct Scsi_Host *shost,
struct list_head *work_q,
@@ -1174,7 +1179,7 @@ static int scsi_eh_target_reset(struct Scsi_Host *shost,
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Sending target reset "
"to target %d\n",
current->comm, id));
- rtn = scsi_try_target_reset(tgtr_scmd);
+ rtn = scsi_try_target_reset(tgtr_scmd->device->host, tgtr_scmd);
if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
if (id == scmd_id(scmd))
@@ -1196,7 +1201,7 @@ static int scsi_eh_target_reset(struct Scsi_Host *shost,
}
/**
- * scsi_eh_bus_reset - send a bus reset
+ * scsi_eh_bus_reset - send a bus reset
* @shost: &scsi host being recovered.
* @work_q: &list_head for pending commands.
* @done_q: &list_head for processed commands.
@@ -1213,7 +1218,7 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
* we really want to loop over the various channels, and do this on
* a channel by channel basis. we should also check to see if any
* of the failed commands are on soft_reset devices, and if so, skip
- * the reset.
+ * the reset.
*/
for (channel = 0; channel <= shost->max_channel; channel++) {
@@ -1234,7 +1239,7 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Sending BRST chan:"
" %d\n", current->comm,
channel));
- rtn = scsi_try_bus_reset(chan_scmd);
+ rtn = scsi_try_bus_reset(chan_scmd->device->host, chan_scmd);
if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
if (channel == scmd_channel(scmd))
@@ -1255,7 +1260,7 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
}
/**
- * scsi_eh_host_reset - send a host reset
+ * scsi_eh_host_reset - send a host reset
* @work_q: list_head for processed commands.
* @done_q: list_head for processed commands.
*/
@@ -1272,7 +1277,7 @@ static int scsi_eh_host_reset(struct list_head *work_q,
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Sending HRST\n"
, current->comm));
- rtn = scsi_try_host_reset(scmd);
+ rtn = scsi_try_host_reset(scmd->device->host, scmd);
if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
if (!scsi_device_online(scmd->device) ||
@@ -1408,7 +1413,7 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
return SUCCESS;
/*
* when the low level driver returns did_soft_error,
- * it is responsible for keeping an internal retry counter
+ * it is responsible for keeping an internal retry counter
* in order to avoid endless loops (db)
*
* actually this is a bug in this function here. we should
@@ -1922,17 +1927,17 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
break;
/* FALLTHROUGH */
case SCSI_TRY_RESET_TARGET:
- rtn = scsi_try_target_reset(scmd);
+ rtn = scsi_try_target_reset(shost, scmd);
if (rtn == SUCCESS)
break;
/* FALLTHROUGH */
case SCSI_TRY_RESET_BUS:
- rtn = scsi_try_bus_reset(scmd);
+ rtn = scsi_try_bus_reset(shost, scmd);
if (rtn == SUCCESS)
break;
/* FALLTHROUGH */
case SCSI_TRY_RESET_HOST:
- rtn = scsi_try_host_reset(scmd);
+ rtn = scsi_try_host_reset(shost, scmd);
break;
default:
rtn = FAILED;
@@ -2005,7 +2010,7 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
if (sb_len > 7)
sshdr->additional_length = sense_buffer[7];
} else {
- /*
+ /*
* fixed format
*/
if (sb_len > 2)
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index b4056d1..083c02c 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -63,11 +63,13 @@ extern int __init scsi_init_devinfo(void);
extern void scsi_exit_devinfo(void);
/* scsi_error.c */
-extern enum blk_eh_timer_return scsi_times_out(struct request *req);
+extern enum blk_eh_timer_return scsi_times_out(struct Scsi_Host *h,
+ struct request *req);
extern int scsi_error_handler(void *host);
extern int scsi_decide_disposition(struct scsi_cmnd *cmd);
extern void scsi_eh_wakeup(struct Scsi_Host *shost);
-extern int scsi_eh_scmd_add(struct scsi_cmnd *, int);
+extern int scsi_eh_scmd_add(struct Scsi_Host *h, struct scsi_cmnd *scmd,
+ int eh_flag);
void scsi_eh_ready_devs(struct Scsi_Host *shost,
struct list_head *work_q,
struct list_head *done_q);
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.
On Sat, Nov 20, 2010 at 12:30 PM, Jesper Juhl <[email protected]> wrote:
>
> Ok, I tried doing that (see patch below)
Actually, you kind of chose exactly the reverse of the functions I'd
have chosen.
Try doing the added parameter to the small static helper functions.
Those are the ones that tend to get inlined, and then the parameter
should actually result in _fewer_ pointer reloads.
So the ones like this:
> ?static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
> ?{
> - ? ? ? if (!scmd->device->host->hostt->eh_abort_handler)
> + ? ? ? struct scsi_host_template *hostt = scmd->device->host->hostt;
> + ? ? ? if (!hostt->eh_abort_handler)
> ? ? ? ? ? ? ? ?return FAILED;
> -
> - ? ? ? return scmd->device->host->hostt->eh_abort_handler(scmd);
> + ? ? ? return hostt->eh_abort_handler(scmd);
> ?}
Where the function is trivial, and almost all it does is to just
dereference those pointers.
If that function got the "host" pointer as an argument, it should make
it much smaller. Because half of that function is that
"scmd->device->host" lookup.
(ok, that's exaggerated, but not by much).
Linus
On Sat, 20 Nov 2010, Linus Torvalds wrote:
> On Sat, Nov 20, 2010 at 12:30 PM, Jesper Juhl <[email protected]> wrote:
> >
> > Ok, I tried doing that (see patch below)
>
> Actually, you kind of chose exactly the reverse of the functions I'd
> have chosen.
>
> Try doing the added parameter to the small static helper functions.
> Those are the ones that tend to get inlined, and then the parameter
> should actually result in _fewer_ pointer reloads.
>
> So the ones like this:
>
> > ?static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
[...]
I see your point now.
I tried this with most of the functions where it seemed that it could
possibly be a gain, but in the end it turned out that only the one you
pointed out above actually saw any benefit, so that's the only one I
changed.
In the end, the object size is down to this:
text data bss dec hex filename
18713 128 4704 23545 5bf9 drivers/scsi/scsi_error.o
from this:
text data bss dec hex filename
18790 128 4712 23630 5c4e drivers/scsi/scsi_error.o
and the patch looks like this now:
Signed-off-by: Jesper Juhl <[email protected]>
---
scsi_error.c | 93 +++++++++++++++++++++++++++++++----------------------------
1 file changed, 49 insertions(+), 44 deletions(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 824b8fc..1d7958a 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -3,14 +3,14 @@
*
* SCSI error/timeout handling
* Initial versions: Eric Youngdale. Based upon conversations with
- * Leonard Zubkoff and David Miller at Linux Expo,
+ * Leonard Zubkoff and David Miller at Linux Expo,
* ideas originating from all over the place.
*
* Restructured scsi_unjam_host and associated functions.
* September 04, 2002 Mike Anderson ([email protected])
*
* Forward port of Russell King's ([email protected]) changes and
- * minor cleanups.
+ * minor cleanups.
* September 30, 2002 Mike Anderson ([email protected])
*/
@@ -129,14 +129,15 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
{
struct scsi_cmnd *scmd = req->special;
enum blk_eh_timer_return rtn = BLK_EH_NOT_HANDLED;
+ struct Scsi_Host *host = scmd->device->host;
trace_scsi_dispatch_cmd_timeout(scmd);
scsi_log_completion(scmd, TIMEOUT_ERROR);
- if (scmd->device->host->transportt->eh_timed_out)
- rtn = scmd->device->host->transportt->eh_timed_out(scmd);
- else if (scmd->device->host->hostt->eh_timed_out)
- rtn = scmd->device->host->hostt->eh_timed_out(scmd);
+ if (host->transportt->eh_timed_out)
+ rtn = host->transportt->eh_timed_out(scmd);
+ else if (host->hostt->eh_timed_out)
+ rtn = host->hostt->eh_timed_out(scmd);
if (unlikely(rtn == BLK_EH_NOT_HANDLED &&
!scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD))) {
@@ -195,7 +196,7 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
++total_failures;
if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD)
++cmd_cancel;
- else
+ else
++cmd_failed;
}
}
@@ -294,7 +295,7 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
return NEEDS_RETRY;
}
/*
- * if the device is in the process of becoming ready, we
+ * if the device is in the process of becoming ready, we
* should retry.
*/
if ((sshdr.asc == 0x04) && (sshdr.ascq == 0x01))
@@ -488,7 +489,7 @@ static int scsi_eh_completed_normally(struct scsi_cmnd *scmd)
*/
static void scsi_eh_done(struct scsi_cmnd *scmd)
{
- struct completion *eh_action;
+ struct completion *eh_action;
SCSI_LOG_ERROR_RECOVERY(3,
printk("%s scmd: %p result: %x\n",
@@ -507,22 +508,23 @@ static int scsi_try_host_reset(struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct Scsi_Host *host = scmd->device->host;
+ struct scsi_host_template *hostt = host->hostt;
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Snd Host RST\n",
__func__));
- if (!scmd->device->host->hostt->eh_host_reset_handler)
+ if (!hostt->eh_host_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_host_reset_handler(scmd);
+ rtn = hostt->eh_host_reset_handler(scmd);
if (rtn == SUCCESS) {
- if (!scmd->device->host->hostt->skip_settle_delay)
+ if (!hostt->skip_settle_delay)
ssleep(HOST_RESET_SETTLE_TIME);
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
- scsi_report_bus_reset(scmd->device->host,
- scmd_channel(scmd));
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(host->host_lock, flags);
+ scsi_report_bus_reset(host, scmd_channel(scmd));
+ spin_unlock_irqrestore(host->host_lock, flags);
}
return rtn;
@@ -536,22 +538,23 @@ static int scsi_try_bus_reset(struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct Scsi_Host *host = scmd->device->host;
+ struct scsi_host_template *hostt = host->hostt;
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Snd Bus RST\n",
__func__));
- if (!scmd->device->host->hostt->eh_bus_reset_handler)
+ if (!hostt->eh_bus_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_bus_reset_handler(scmd);
+ rtn = hostt->eh_bus_reset_handler(scmd);
if (rtn == SUCCESS) {
- if (!scmd->device->host->hostt->skip_settle_delay)
+ if (!hostt->skip_settle_delay)
ssleep(BUS_RESET_SETTLE_TIME);
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
- scsi_report_bus_reset(scmd->device->host,
- scmd_channel(scmd));
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(host->host_lock, flags);
+ scsi_report_bus_reset(host, scmd_channel(scmd));
+ spin_unlock_irqrestore(host->host_lock, flags);
}
return rtn;
@@ -577,16 +580,18 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct Scsi_Host *host = scmd->device->host;
+ struct scsi_host_template *hostt = host->hostt;
- if (!scmd->device->host->hostt->eh_target_reset_handler)
+ if (!hostt->eh_target_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_target_reset_handler(scmd);
+ rtn = hostt->eh_target_reset_handler(scmd);
if (rtn == SUCCESS) {
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(host->host_lock, flags);
__starget_for_each_device(scsi_target(scmd->device), NULL,
__scsi_report_device_reset);
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_unlock_irqrestore(host->host_lock, flags);
}
return rtn;
@@ -605,22 +610,23 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
{
int rtn;
+ struct scsi_host_template *hostt = scmd->device->host->hostt;
- if (!scmd->device->host->hostt->eh_device_reset_handler)
+ if (!hostt->eh_device_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_device_reset_handler(scmd);
+ rtn = hostt->eh_device_reset_handler(scmd);
if (rtn == SUCCESS)
__scsi_report_device_reset(scmd->device, NULL);
return rtn;
}
-static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
+static int __scsi_try_to_abort_cmd(struct scsi_host_template *hostt,
+ struct scsi_cmnd *scmd)
{
- if (!scmd->device->host->hostt->eh_abort_handler)
+ if (!hostt->eh_abort_handler)
return FAILED;
-
- return scmd->device->host->hostt->eh_abort_handler(scmd);
+ return hostt->eh_abort_handler(scmd);
}
/**
@@ -642,12 +648,12 @@ static int scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
*/
if (scmd->serial_number == 0)
return SUCCESS;
- return __scsi_try_to_abort_cmd(scmd);
+ return __scsi_try_to_abort_cmd(scmd->device->host->hostt, scmd);
}
static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
{
- if (__scsi_try_to_abort_cmd(scmd) != SUCCESS)
+ if (__scsi_try_to_abort_cmd(scmd->device->host->hostt, scmd) != SUCCESS)
if (scsi_try_bus_device_reset(scmd) != SUCCESS)
if (scsi_try_target_reset(scmd) != SUCCESS)
if (scsi_try_bus_reset(scmd) != SUCCESS)
@@ -867,7 +873,7 @@ EXPORT_SYMBOL(scsi_eh_finish_cmd);
*
* Description:
* See if we need to request sense information. if so, then get it
- * now, so we have a better idea of what to do.
+ * now, so we have a better idea of what to do.
*
* Notes:
* This has the unfortunate side effect that if a shost adapter does
@@ -987,7 +993,6 @@ static int scsi_eh_abort_cmds(struct list_head *work_q,
!scsi_eh_tur(scmd)) {
scsi_eh_finish_cmd(scmd, done_q);
}
-
} else
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: aborting"
" cmd failed:"
@@ -1031,7 +1036,7 @@ static int scsi_eh_try_stu(struct scsi_cmnd *scmd)
*
* Notes:
* If commands are failing due to not ready, initializing command required,
- * try revalidating the device, which will end up sending a start unit.
+ * try revalidating the device, which will end up sending a start unit.
*/
static int scsi_eh_stu(struct Scsi_Host *shost,
struct list_head *work_q,
@@ -1085,7 +1090,7 @@ static int scsi_eh_stu(struct Scsi_Host *shost,
* Try a bus device reset. Still, look to see whether we have multiple
* devices that are jammed or not - if we have multiple devices, it
* makes no sense to try bus_device_reset - we really would need to try
- * a bus_reset instead.
+ * a bus_reset instead.
*/
static int scsi_eh_bus_device_reset(struct Scsi_Host *shost,
struct list_head *work_q,
@@ -1196,7 +1201,7 @@ static int scsi_eh_target_reset(struct Scsi_Host *shost,
}
/**
- * scsi_eh_bus_reset - send a bus reset
+ * scsi_eh_bus_reset - send a bus reset
* @shost: &scsi host being recovered.
* @work_q: &list_head for pending commands.
* @done_q: &list_head for processed commands.
@@ -1213,7 +1218,7 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
* we really want to loop over the various channels, and do this on
* a channel by channel basis. we should also check to see if any
* of the failed commands are on soft_reset devices, and if so, skip
- * the reset.
+ * the reset.
*/
for (channel = 0; channel <= shost->max_channel; channel++) {
@@ -1255,7 +1260,7 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
}
/**
- * scsi_eh_host_reset - send a host reset
+ * scsi_eh_host_reset - send a host reset
* @work_q: list_head for processed commands.
* @done_q: list_head for processed commands.
*/
@@ -1408,7 +1413,7 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
return SUCCESS;
/*
* when the low level driver returns did_soft_error,
- * it is responsible for keeping an internal retry counter
+ * it is responsible for keeping an internal retry counter
* in order to avoid endless loops (db)
*
* actually this is a bug in this function here. we should
@@ -2005,7 +2010,7 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
if (sb_len > 7)
sshdr->additional_length = sense_buffer[7];
} else {
- /*
+ /*
* fixed format
*/
if (sb_len > 2)
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.
On Sun, 2010-11-21 at 19:48 +0100, Jesper Juhl wrote:
> On Sat, 20 Nov 2010, Linus Torvalds wrote:
>
> > On Sat, Nov 20, 2010 at 12:30 PM, Jesper Juhl <[email protected]> wrote:
> > >
> > > Ok, I tried doing that (see patch below)
> >
> > Actually, you kind of chose exactly the reverse of the functions I'd
> > have chosen.
> >
> > Try doing the added parameter to the small static helper functions.
> > Those are the ones that tend to get inlined, and then the parameter
> > should actually result in _fewer_ pointer reloads.
> >
> > So the ones like this:
> >
> > > static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
> [...]
>
> I see your point now.
>
> I tried this with most of the functions where it seemed that it could
> possibly be a gain, but in the end it turned out that only the one you
> pointed out above actually saw any benefit, so that's the only one I
> changed.
>
> In the end, the object size is down to this:
>
> text data bss dec hex filename
> 18713 128 4704 23545 5bf9 drivers/scsi/scsi_error.o
>
> from this:
>
> text data bss dec hex filename
> 18790 128 4712 23630 5c4e drivers/scsi/scsi_error.o
>
>
> and the patch looks like this now:
>
> Signed-off-by: Jesper Juhl <[email protected]>
This is rejecting against scsi-misc:
patching file drivers/scsi/scsi_error.c
Hunk #9 FAILED at 610.
Hunk #10 FAILED at 647.
Hunk #11 succeeded at 850 (offset -22 lines).
Hunk #12 succeeded at 970 (offset -22 lines).
Hunk #13 succeeded at 1013 (offset -22 lines).
Hunk #14 succeeded at 1067 (offset -22 lines).
Hunk #15 succeeded at 1167 (offset -33 lines).
Hunk #16 succeeded at 1184 (offset -33 lines).
Hunk #17 succeeded at 1226 (offset -33 lines).
Hunk #18 succeeded at 1379 (offset -33 lines).
Hunk #19 succeeded at 1976 (offset -33 lines).
2 out of 19 hunks FAILED -- saving rejects to file
drivers/scsi/scsi_error.c.rej
It looks like changes caused by
commit 459dbf72e4d2b4aa13620e6b70d54f098547bf13
[SCSI] Eliminate error handler overload of the SCSI serial number
Could you respin so it applies, please?
Thanks,
James
On Tue, 21 Dec 2010, James Bottomley wrote:
> On Sun, 2010-11-21 at 19:48 +0100, Jesper Juhl wrote:
> > On Sat, 20 Nov 2010, Linus Torvalds wrote:
> >
> > > On Sat, Nov 20, 2010 at 12:30 PM, Jesper Juhl <[email protected]> wrote:
> > > >
> > > > Ok, I tried doing that (see patch below)
> > >
> > > Actually, you kind of chose exactly the reverse of the functions I'd
> > > have chosen.
> > >
> > > Try doing the added parameter to the small static helper functions.
> > > Those are the ones that tend to get inlined, and then the parameter
> > > should actually result in _fewer_ pointer reloads.
> > >
> > > So the ones like this:
> > >
> > > > static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
> > [...]
> >
> > I see your point now.
> >
> > I tried this with most of the functions where it seemed that it could
> > possibly be a gain, but in the end it turned out that only the one you
> > pointed out above actually saw any benefit, so that's the only one I
> > changed.
> >
> > In the end, the object size is down to this:
> >
> > text data bss dec hex filename
> > 18713 128 4704 23545 5bf9 drivers/scsi/scsi_error.o
> >
> > from this:
> >
> > text data bss dec hex filename
> > 18790 128 4712 23630 5c4e drivers/scsi/scsi_error.o
> >
> >
> > and the patch looks like this now:
> >
> > Signed-off-by: Jesper Juhl <[email protected]>
>
> This is rejecting against scsi-misc:
>
...
>
> Could you respin so it applies, please?
>
Sure. The following applies against the tip of Linus' tree as of right now
(head at 90a8a73c06cc32b609a880d48449d7083327e11a).
We are now down to this as far as size goes:
text data bss dec hex filename
18691 128 4696 23515 5bdb drivers/scsi/scsi_error.o
Signed-off-by: Jesper Juhl <[email protected]>
---
scsi_error.c | 94 ++++++++++++++++++++++++++++++-----------------------------
1 file changed, 49 insertions(+), 45 deletions(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 30ac116..b37e10f 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -3,14 +3,14 @@
*
* SCSI error/timeout handling
* Initial versions: Eric Youngdale. Based upon conversations with
- * Leonard Zubkoff and David Miller at Linux Expo,
+ * Leonard Zubkoff and David Miller at Linux Expo,
* ideas originating from all over the place.
*
* Restructured scsi_unjam_host and associated functions.
* September 04, 2002 Mike Anderson ([email protected])
*
* Forward port of Russell King's ([email protected]) changes and
- * minor cleanups.
+ * minor cleanups.
* September 30, 2002 Mike Anderson ([email protected])
*/
@@ -129,14 +129,15 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
{
struct scsi_cmnd *scmd = req->special;
enum blk_eh_timer_return rtn = BLK_EH_NOT_HANDLED;
+ struct Scsi_Host *host = scmd->device->host;
trace_scsi_dispatch_cmd_timeout(scmd);
scsi_log_completion(scmd, TIMEOUT_ERROR);
- if (scmd->device->host->transportt->eh_timed_out)
- rtn = scmd->device->host->transportt->eh_timed_out(scmd);
- else if (scmd->device->host->hostt->eh_timed_out)
- rtn = scmd->device->host->hostt->eh_timed_out(scmd);
+ if (host->transportt->eh_timed_out)
+ rtn = host->transportt->eh_timed_out(scmd);
+ else if (host->hostt->eh_timed_out)
+ rtn = host->hostt->eh_timed_out(scmd);
if (unlikely(rtn == BLK_EH_NOT_HANDLED &&
!scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD))) {
@@ -195,7 +196,7 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
++total_failures;
if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD)
++cmd_cancel;
- else
+ else
++cmd_failed;
}
}
@@ -214,7 +215,7 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
SCSI_LOG_ERROR_RECOVERY(2, printk("Total of %d commands on %d"
" devices require eh work\n",
- total_failures, devices_failed));
+ total_failures, devices_failed));
}
#endif
@@ -294,7 +295,7 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
return NEEDS_RETRY;
}
/*
- * if the device is in the process of becoming ready, we
+ * if the device is in the process of becoming ready, we
* should retry.
*/
if ((sshdr.asc == 0x04) && (sshdr.ascq == 0x01))
@@ -488,7 +489,7 @@ static int scsi_eh_completed_normally(struct scsi_cmnd *scmd)
*/
static void scsi_eh_done(struct scsi_cmnd *scmd)
{
- struct completion *eh_action;
+ struct completion *eh_action;
SCSI_LOG_ERROR_RECOVERY(3,
printk("%s scmd: %p result: %x\n",
@@ -507,22 +508,23 @@ static int scsi_try_host_reset(struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct Scsi_Host *host = scmd->device->host;
+ struct scsi_host_template *hostt = host->hostt;
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Snd Host RST\n",
__func__));
- if (!scmd->device->host->hostt->eh_host_reset_handler)
+ if (!hostt->eh_host_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_host_reset_handler(scmd);
+ rtn = hostt->eh_host_reset_handler(scmd);
if (rtn == SUCCESS) {
- if (!scmd->device->host->hostt->skip_settle_delay)
+ if (!hostt->skip_settle_delay)
ssleep(HOST_RESET_SETTLE_TIME);
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
- scsi_report_bus_reset(scmd->device->host,
- scmd_channel(scmd));
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(host->host_lock, flags);
+ scsi_report_bus_reset(host, scmd_channel(scmd));
+ spin_unlock_irqrestore(host->host_lock, flags);
}
return rtn;
@@ -536,22 +538,23 @@ static int scsi_try_bus_reset(struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct Scsi_Host *host = scmd->device->host;
+ struct scsi_host_template *hostt = host->hostt;
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Snd Bus RST\n",
__func__));
- if (!scmd->device->host->hostt->eh_bus_reset_handler)
+ if (!hostt->eh_bus_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_bus_reset_handler(scmd);
+ rtn = hostt->eh_bus_reset_handler(scmd);
if (rtn == SUCCESS) {
- if (!scmd->device->host->hostt->skip_settle_delay)
+ if (!hostt->skip_settle_delay)
ssleep(BUS_RESET_SETTLE_TIME);
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
- scsi_report_bus_reset(scmd->device->host,
- scmd_channel(scmd));
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(host->host_lock, flags);
+ scsi_report_bus_reset(host, scmd_channel(scmd));
+ spin_unlock_irqrestore(host->host_lock, flags);
}
return rtn;
@@ -577,16 +580,18 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
{
unsigned long flags;
int rtn;
+ struct Scsi_Host *host = scmd->device->host;
+ struct scsi_host_template *hostt = host->hostt;
- if (!scmd->device->host->hostt->eh_target_reset_handler)
+ if (!hostt->eh_target_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_target_reset_handler(scmd);
+ rtn = hostt->eh_target_reset_handler(scmd);
if (rtn == SUCCESS) {
- spin_lock_irqsave(scmd->device->host->host_lock, flags);
+ spin_lock_irqsave(host->host_lock, flags);
__starget_for_each_device(scsi_target(scmd->device), NULL,
__scsi_report_device_reset);
- spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+ spin_unlock_irqrestore(host->host_lock, flags);
}
return rtn;
@@ -605,27 +610,28 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
{
int rtn;
+ struct scsi_host_template *hostt = scmd->device->host->hostt;
- if (!scmd->device->host->hostt->eh_device_reset_handler)
+ if (!hostt->eh_device_reset_handler)
return FAILED;
- rtn = scmd->device->host->hostt->eh_device_reset_handler(scmd);
+ rtn = hostt->eh_device_reset_handler(scmd);
if (rtn == SUCCESS)
__scsi_report_device_reset(scmd->device, NULL);
return rtn;
}
-static int scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
+static int scsi_try_to_abort_cmd(struct scsi_host_template *hostt, struct scsi_cmnd *scmd)
{
- if (!scmd->device->host->hostt->eh_abort_handler)
+ if (!hostt->eh_abort_handler)
return FAILED;
- return scmd->device->host->hostt->eh_abort_handler(scmd);
+ return hostt->eh_abort_handler(scmd);
}
static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
{
- if (scsi_try_to_abort_cmd(scmd) != SUCCESS)
+ if (scsi_try_to_abort_cmd(scmd->device->host->hostt, scmd) != SUCCESS)
if (scsi_try_bus_device_reset(scmd) != SUCCESS)
if (scsi_try_target_reset(scmd) != SUCCESS)
if (scsi_try_bus_reset(scmd) != SUCCESS)
@@ -845,7 +851,7 @@ EXPORT_SYMBOL(scsi_eh_finish_cmd);
*
* Description:
* See if we need to request sense information. if so, then get it
- * now, so we have a better idea of what to do.
+ * now, so we have a better idea of what to do.
*
* Notes:
* This has the unfortunate side effect that if a shost adapter does
@@ -957,7 +963,7 @@ static int scsi_eh_abort_cmds(struct list_head *work_q,
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: aborting cmd:"
"0x%p\n", current->comm,
scmd));
- rtn = scsi_try_to_abort_cmd(scmd);
+ rtn = scsi_try_to_abort_cmd(scmd->device->host->hostt, scmd);
if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
scmd->eh_eflags &= ~SCSI_EH_CANCEL_CMD;
if (!scsi_device_online(scmd->device) ||
@@ -965,7 +971,6 @@ static int scsi_eh_abort_cmds(struct list_head *work_q,
!scsi_eh_tur(scmd)) {
scsi_eh_finish_cmd(scmd, done_q);
}
-
} else
SCSI_LOG_ERROR_RECOVERY(3, printk("%s: aborting"
" cmd failed:"
@@ -1009,7 +1014,7 @@ static int scsi_eh_try_stu(struct scsi_cmnd *scmd)
*
* Notes:
* If commands are failing due to not ready, initializing command required,
- * try revalidating the device, which will end up sending a start unit.
+ * try revalidating the device, which will end up sending a start unit.
*/
static int scsi_eh_stu(struct Scsi_Host *shost,
struct list_head *work_q,
@@ -1063,7 +1068,7 @@ static int scsi_eh_stu(struct Scsi_Host *shost,
* Try a bus device reset. Still, look to see whether we have multiple
* devices that are jammed or not - if we have multiple devices, it
* makes no sense to try bus_device_reset - we really would need to try
- * a bus_reset instead.
+ * a bus_reset instead.
*/
static int scsi_eh_bus_device_reset(struct Scsi_Host *shost,
struct list_head *work_q,
@@ -1174,7 +1179,7 @@ static int scsi_eh_target_reset(struct Scsi_Host *shost,
}
/**
- * scsi_eh_bus_reset - send a bus reset
+ * scsi_eh_bus_reset - send a bus reset
* @shost: &scsi host being recovered.
* @work_q: &list_head for pending commands.
* @done_q: &list_head for processed commands.
@@ -1191,7 +1196,7 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
* we really want to loop over the various channels, and do this on
* a channel by channel basis. we should also check to see if any
* of the failed commands are on soft_reset devices, and if so, skip
- * the reset.
+ * the reset.
*/
for (channel = 0; channel <= shost->max_channel; channel++) {
@@ -1233,7 +1238,7 @@ static int scsi_eh_bus_reset(struct Scsi_Host *shost,
}
/**
- * scsi_eh_host_reset - send a host reset
+ * scsi_eh_host_reset - send a host reset
* @work_q: list_head for processed commands.
* @done_q: list_head for processed commands.
*/
@@ -1386,7 +1391,7 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
return SUCCESS;
/*
* when the low level driver returns did_soft_error,
- * it is responsible for keeping an internal retry counter
+ * it is responsible for keeping an internal retry counter
* in order to avoid endless loops (db)
*
* actually this is a bug in this function here. we should
@@ -1424,7 +1429,6 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
*/
break;
/* fallthrough */
-
case DID_BUS_BUSY:
case DID_PARITY:
goto maybe_retry;
@@ -1983,7 +1987,7 @@ int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
if (sb_len > 7)
sshdr->additional_length = sense_buffer[7];
} else {
- /*
+ /*
* fixed format
*/
if (sb_len > 2)
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.