From: Tomas Henzl <thenzl@redhat.com>
To: rajinikanth.pandurangan@pmcs.com, jbottomley@parallels.com,
linux-scsi@vger.kernel.org
Cc: aacraid@pmc-sierra.com, harry.yang@pmcs.com,
mahesh.rajashekhara@pmcs.com, rich.bono@pmcs.com,
achim.leubner@pmcs.com, murthy.bhat@pmcs.com
Subject: Re: [Patch V2 5/9] [SCSI] aacraid: Tune response path if IsFastPath bit set
Date: Thu, 25 Jun 2015 20:05:26 +0200 [thread overview]
Message-ID: <558C42E6.80603@redhat.com> (raw)
In-Reply-To: <1433986951-9033-7-git-send-email-rajinikanth.pandurangan@pmcs.com>
On 06/11/2015 03:42 AM, rajinikanth.pandurangan@pmcs.com wrote:
> From: Rajinikanth Pandurangan <rajinikanth.pandurangan@pmcs.com>
>
> Description:
> If 'IsFastPath' bit is set, then response path assumes no error
> and skips error check.
>
> Signed-off-by: Rajinikanth Pandurangan <rajinikanth.pandurangan@pmcs.com>
> ---
> drivers/scsi/aacraid/aachba.c | 259 ++++++++++++++++++++++--------------------
> 1 file changed, 137 insertions(+), 122 deletions(-)
>
With this patch applied we get :
if (fibptr->flags & FIB_CONTEXT_FLAG_FASTRESP) {
/* fast response */
srbreply->srb_status = cpu_to_le32(SRB_STATUS_SUCCESS);
srbreply->scsi_status = cpu_to_le32(SAM_STAT_GOOD);
} else {
/*
* Calculate resid for sg
*/
....
}
/*
* OR in the scsi status (already shifted up a bit)
*/
scsicmd->result |= le32_to_cpu(srbreply->scsi_status);
aac_fib_complete(fibptr);
aac_fib_free(fibptr);
scsicmd->scsi_done(scsicmd);
}
>From that it looks like you do not need set
srbreply->srb_status = cpu_to_le32(SRB_STATUS_SUCCESS);
because it's never used later.
You could you rearrange it to :
if (fibptr->flags & FIB_CONTEXT_FLAG_FASTRESP) {
/* fast response */
srbreply->scsi_status = cpu_to_le32(SAM_STAT_GOOD);
goto done;
}
/*
* Calculate resid for sg
*/
....
done:
/*
* OR in the scsi status (already shifted up a bit)
*/
scsicmd->result |= le32_to_cpu(srbreply->scsi_status);
aac_fib_complete(fibptr);
aac_fib_free(fibptr);
scsicmd->scsi_done(scsicmd);
The advantage is that you don't need move ~150 lines one tab to
the right and don't have to wrap lines because of the 80 char limit.
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
> index fe59b00..864e9f6 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -2977,11 +2977,16 @@ static void aac_srb_callback(void *context, struct fib * fibptr)
> return;
>
> BUG_ON(fibptr == NULL);
> -
> dev = fibptr->dev;
>
> - srbreply = (struct aac_srb_reply *) fib_data(fibptr);
> + scsi_dma_unmap(scsicmd);
>
> + /* expose physical device if expose_physicald flag is on */
> + if (scsicmd->cmnd[0] == INQUIRY && !(scsicmd->cmnd[1] & 0x01)
> + && expose_physicals > 0)
> + aac_expose_phy_device(scsicmd);
> +
> + srbreply = (struct aac_srb_reply *) fib_data(fibptr);
> scsicmd->sense_buffer[0] = '\0'; /* Initialize sense valid flag to false */
>
> if (fibptr->flags & FIB_CONTEXT_FLAG_FASTRESP) {
> @@ -2994,147 +2999,157 @@ static void aac_srb_callback(void *context, struct fib * fibptr)
> */
> scsi_set_resid(scsicmd, scsi_bufflen(scsicmd)
> - le32_to_cpu(srbreply->data_xfer_length));
> - }
> -
> - scsi_dma_unmap(scsicmd);
> -
> - /* expose physical device if expose_physicald flag is on */
> - if (scsicmd->cmnd[0] == INQUIRY && !(scsicmd->cmnd[1] & 0x01)
> - && expose_physicals > 0)
> - aac_expose_phy_device(scsicmd);
> + /*
> + * First check the fib status
> + */
>
> - /*
> - * First check the fib status
> - */
> + if (le32_to_cpu(srbreply->status) != ST_OK) {
> + int len;
>
> - if (le32_to_cpu(srbreply->status) != ST_OK){
> - int len;
> - printk(KERN_WARNING "aac_srb_callback: srb failed, status = %d\n", le32_to_cpu(srbreply->status));
> - len = min_t(u32, le32_to_cpu(srbreply->sense_data_size),
> - SCSI_SENSE_BUFFERSIZE);
> - scsicmd->result = DID_ERROR << 16 | COMMAND_COMPLETE << 8 | SAM_STAT_CHECK_CONDITION;
> - memcpy(scsicmd->sense_buffer, srbreply->sense_data, len);
> - }
> + printk(KERN_WARNING "aac_srb_callback: srb failed, status = %d\n", le32_to_cpu(srbreply->status));
> + len = min_t(u32, le32_to_cpu(srbreply->sense_data_size),
> + SCSI_SENSE_BUFFERSIZE);
> + scsicmd->result = DID_ERROR << 16
> + | COMMAND_COMPLETE << 8
> + | SAM_STAT_CHECK_CONDITION;
> + memcpy(scsicmd->sense_buffer,
> + srbreply->sense_data, len);
> + }
>
> - /*
> - * Next check the srb status
> - */
> - switch( (le32_to_cpu(srbreply->srb_status))&0x3f){
> - case SRB_STATUS_ERROR_RECOVERY:
> - case SRB_STATUS_PENDING:
> - case SRB_STATUS_SUCCESS:
> - scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8;
> - break;
> - case SRB_STATUS_DATA_OVERRUN:
> - switch(scsicmd->cmnd[0]){
> - case READ_6:
> - case WRITE_6:
> - case READ_10:
> - case WRITE_10:
> - case READ_12:
> - case WRITE_12:
> - case READ_16:
> - case WRITE_16:
> - if (le32_to_cpu(srbreply->data_xfer_length) < scsicmd->underflow) {
> - printk(KERN_WARNING"aacraid: SCSI CMD underflow\n");
> - } else {
> - printk(KERN_WARNING"aacraid: SCSI CMD Data Overrun\n");
> + /*
> + * Next check the srb status
> + */
> + switch ((le32_to_cpu(srbreply->srb_status))&0x3f) {
> + case SRB_STATUS_ERROR_RECOVERY:
> + case SRB_STATUS_PENDING:
> + case SRB_STATUS_SUCCESS:
> + scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8;
> + break;
> + case SRB_STATUS_DATA_OVERRUN:
> + switch (scsicmd->cmnd[0]) {
> + case READ_6:
> + case WRITE_6:
> + case READ_10:
> + case WRITE_10:
> + case READ_12:
> + case WRITE_12:
> + case READ_16:
> + case WRITE_16:
> + if (le32_to_cpu(srbreply->data_xfer_length)
> + < scsicmd->underflow)
> + printk(KERN_WARNING"aacraid: SCSI CMD underflow\n");
> + else
> + printk(KERN_WARNING"aacraid: SCSI CMD Data Overrun\n");
> + scsicmd->result = DID_ERROR << 16
> + | COMMAND_COMPLETE << 8;
> + break;
> + case INQUIRY: {
> + scsicmd->result = DID_OK << 16
> + | COMMAND_COMPLETE << 8;
> + break;
> + }
> + default:
> + scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8;
> + break;
> }
> - scsicmd->result = DID_ERROR << 16 | COMMAND_COMPLETE << 8;
> break;
> - case INQUIRY: {
> - scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8;
> + case SRB_STATUS_ABORTED:
> + scsicmd->result = DID_ABORT << 16 | ABORT << 8;
> break;
> - }
> - default:
> - scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8;
> + case SRB_STATUS_ABORT_FAILED:
> + /*
> + * Not sure about this one - but assuming the
> + * hba was trying to abort for some reason
> + */
> + scsicmd->result = DID_ERROR << 16 | ABORT << 8;
> + break;
> + case SRB_STATUS_PARITY_ERROR:
> + scsicmd->result = DID_PARITY << 16
> + | MSG_PARITY_ERROR << 8;
> + break;
> + case SRB_STATUS_NO_DEVICE:
> + case SRB_STATUS_INVALID_PATH_ID:
> + case SRB_STATUS_INVALID_TARGET_ID:
> + case SRB_STATUS_INVALID_LUN:
> + case SRB_STATUS_SELECTION_TIMEOUT:
> + scsicmd->result = DID_NO_CONNECT << 16
> + | COMMAND_COMPLETE << 8;
> break;
> - }
> - break;
> - case SRB_STATUS_ABORTED:
> - scsicmd->result = DID_ABORT << 16 | ABORT << 8;
> - break;
> - case SRB_STATUS_ABORT_FAILED:
> - // Not sure about this one - but assuming the hba was trying to abort for some reason
> - scsicmd->result = DID_ERROR << 16 | ABORT << 8;
> - break;
> - case SRB_STATUS_PARITY_ERROR:
> - scsicmd->result = DID_PARITY << 16 | MSG_PARITY_ERROR << 8;
> - break;
> - case SRB_STATUS_NO_DEVICE:
> - case SRB_STATUS_INVALID_PATH_ID:
> - case SRB_STATUS_INVALID_TARGET_ID:
> - case SRB_STATUS_INVALID_LUN:
> - case SRB_STATUS_SELECTION_TIMEOUT:
> - scsicmd->result = DID_NO_CONNECT << 16 | COMMAND_COMPLETE << 8;
> - break;
>
> - case SRB_STATUS_COMMAND_TIMEOUT:
> - case SRB_STATUS_TIMEOUT:
> - scsicmd->result = DID_TIME_OUT << 16 | COMMAND_COMPLETE << 8;
> - break;
> + case SRB_STATUS_COMMAND_TIMEOUT:
> + case SRB_STATUS_TIMEOUT:
> + scsicmd->result = DID_TIME_OUT << 16
> + | COMMAND_COMPLETE << 8;
> + break;
>
> - case SRB_STATUS_BUSY:
> - scsicmd->result = DID_BUS_BUSY << 16 | COMMAND_COMPLETE << 8;
> - break;
> + case SRB_STATUS_BUSY:
> + scsicmd->result = DID_BUS_BUSY << 16
> + | COMMAND_COMPLETE << 8;
> + break;
>
> - case SRB_STATUS_BUS_RESET:
> - scsicmd->result = DID_RESET << 16 | COMMAND_COMPLETE << 8;
> - break;
> + case SRB_STATUS_BUS_RESET:
> + scsicmd->result = DID_RESET << 16
> + | COMMAND_COMPLETE << 8;
> + break;
>
> - case SRB_STATUS_MESSAGE_REJECTED:
> - scsicmd->result = DID_ERROR << 16 | MESSAGE_REJECT << 8;
> - break;
> - case SRB_STATUS_REQUEST_FLUSHED:
> - case SRB_STATUS_ERROR:
> - case SRB_STATUS_INVALID_REQUEST:
> - case SRB_STATUS_REQUEST_SENSE_FAILED:
> - case SRB_STATUS_NO_HBA:
> - case SRB_STATUS_UNEXPECTED_BUS_FREE:
> - case SRB_STATUS_PHASE_SEQUENCE_FAILURE:
> - case SRB_STATUS_BAD_SRB_BLOCK_LENGTH:
> - case SRB_STATUS_DELAYED_RETRY:
> - case SRB_STATUS_BAD_FUNCTION:
> - case SRB_STATUS_NOT_STARTED:
> - case SRB_STATUS_NOT_IN_USE:
> - case SRB_STATUS_FORCE_ABORT:
> - case SRB_STATUS_DOMAIN_VALIDATION_FAIL:
> - default:
> + case SRB_STATUS_MESSAGE_REJECTED:
> + scsicmd->result = DID_ERROR << 16
> + | MESSAGE_REJECT << 8;
> + break;
> + case SRB_STATUS_REQUEST_FLUSHED:
> + case SRB_STATUS_ERROR:
> + case SRB_STATUS_INVALID_REQUEST:
> + case SRB_STATUS_REQUEST_SENSE_FAILED:
> + case SRB_STATUS_NO_HBA:
> + case SRB_STATUS_UNEXPECTED_BUS_FREE:
> + case SRB_STATUS_PHASE_SEQUENCE_FAILURE:
> + case SRB_STATUS_BAD_SRB_BLOCK_LENGTH:
> + case SRB_STATUS_DELAYED_RETRY:
> + case SRB_STATUS_BAD_FUNCTION:
> + case SRB_STATUS_NOT_STARTED:
> + case SRB_STATUS_NOT_IN_USE:
> + case SRB_STATUS_FORCE_ABORT:
> + case SRB_STATUS_DOMAIN_VALIDATION_FAIL:
> + default:
> #ifdef AAC_DETAILED_STATUS_INFO
> - printk("aacraid: SRB ERROR(%u) %s scsi cmd 0x%x - scsi status 0x%x\n",
> - le32_to_cpu(srbreply->srb_status) & 0x3F,
> - aac_get_status_string(
> - le32_to_cpu(srbreply->srb_status) & 0x3F),
> - scsicmd->cmnd[0],
> - le32_to_cpu(srbreply->scsi_status));
> + printk(KERN_INFO "aacraid: SRB ERROR(%u) %s scsi cmd 0x%x - scsi status 0x%x\n",
> + le32_to_cpu(srbreply->srb_status) & 0x3F,
> + aac_get_status_string(
> + le32_to_cpu(srbreply->srb_status) & 0x3F),
> + scsicmd->cmnd[0],
> + le32_to_cpu(srbreply->scsi_status));
> #endif
> - if ((scsicmd->cmnd[0] == ATA_12)
> - || (scsicmd->cmnd[0] == ATA_16)) {
> - if (scsicmd->cmnd[2] & (0x01 << 5)) {
> - scsicmd->result = DID_OK << 16
> - | COMMAND_COMPLETE << 8;
> + if ((scsicmd->cmnd[0] == ATA_12)
> + || (scsicmd->cmnd[0] == ATA_16)) {
> + if (scsicmd->cmnd[2] & (0x01 << 5)) {
> + scsicmd->result = DID_OK << 16
> + | COMMAND_COMPLETE << 8;
> break;
> + } else {
> + scsicmd->result = DID_ERROR << 16
> + | COMMAND_COMPLETE << 8;
> + break;
> + }
> } else {
> scsicmd->result = DID_ERROR << 16
> - | COMMAND_COMPLETE << 8;
> + | COMMAND_COMPLETE << 8;
> break;
> }
> - } else {
> - scsicmd->result = DID_ERROR << 16
> - | COMMAND_COMPLETE << 8;
> - break;
> }
> - }
> - if (le32_to_cpu(srbreply->scsi_status) == SAM_STAT_CHECK_CONDITION) {
> - int len;
> - scsicmd->result |= SAM_STAT_CHECK_CONDITION;
> - len = min_t(u32, le32_to_cpu(srbreply->sense_data_size),
> - SCSI_SENSE_BUFFERSIZE);
> + if (le32_to_cpu(srbreply->scsi_status)
> + == SAM_STAT_CHECK_CONDITION) {
> + int len;
> +
> + scsicmd->result |= SAM_STAT_CHECK_CONDITION;
> + len = min_t(u32, le32_to_cpu(srbreply->sense_data_size),
> + SCSI_SENSE_BUFFERSIZE);
> #ifdef AAC_DETAILED_STATUS_INFO
> - printk(KERN_WARNING "aac_srb_callback: check condition, status = %d len=%d\n",
> - le32_to_cpu(srbreply->status), len);
> + printk(KERN_WARNING "aac_srb_callback: check condition, status = %d len=%d\n",
> + le32_to_cpu(srbreply->status), len);
> #endif
> - memcpy(scsicmd->sense_buffer, srbreply->sense_data, len);
> + memcpy(scsicmd->sense_buffer,
> + srbreply->sense_data, len);
> + }
> }
> /*
> * OR in the scsi status (already shifted up a bit)
>
next prev parent reply other threads:[~2015-06-25 18:05 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-11 1:42 [Patch V2 0/9] [SCSI] aacraid: Patchset for aacraid driver version 41010 rajinikanth.pandurangan
2015-06-11 1:42 ` rajinikanth.pandurangan
2015-06-11 1:42 ` [Patch V2 1/9] [SCSI] aacraid: Fix for logical device name and UID not exposed to the OS rajinikanth.pandurangan
2015-06-16 11:27 ` Johannes Thumshirn
2015-06-24 5:40 ` Mahesh Rajashekhara
2015-06-25 15:21 ` Tomas Henzl
2015-06-11 1:42 ` [Patch V2 2/9] [SCSI] aacraid: Add Power Management support rajinikanth.pandurangan
2015-06-24 5:40 ` Mahesh Rajashekhara
2015-06-25 15:42 ` Tomas Henzl
2015-06-25 16:54 ` Rajinikanth Pandurangan
2015-06-25 17:52 ` Tomas Henzl
2015-06-11 1:42 ` [Patch V2 3/9] [SCSI] aacraid: Enable MSI interrupt for series-6 controller rajinikanth.pandurangan
2015-06-16 11:33 ` Johannes Thumshirn
2015-06-24 5:40 ` Mahesh Rajashekhara
2015-06-25 16:01 ` Tomas Henzl
2015-06-11 1:42 ` [Patch V2 4/9] [SCSI] aacraid: Enable 64-bit write to controller register rajinikanth.pandurangan
2015-06-11 10:01 ` Johannes Thumshirn
2015-06-11 18:08 ` Rajinikanth Pandurangan
2015-06-24 5:41 ` Mahesh Rajashekhara
2015-06-25 16:03 ` Tomas Henzl
2015-06-11 1:42 ` [Patch V2 5/9] [SCSI] aacraid: Tune response path if IsFastPath bit set rajinikanth.pandurangan
2015-06-18 17:30 ` Rajinikanth Pandurangan
2015-06-24 5:41 ` Mahesh Rajashekhara
2015-06-25 18:05 ` Tomas Henzl [this message]
2015-06-11 1:42 ` [Patch V2 6/9] [SCSI] aacraid: Reset irq affinity hints before releasing irq rajinikanth.pandurangan
2015-06-16 11:40 ` Johannes Thumshirn
2015-06-24 5:41 ` Mahesh Rajashekhara
2015-06-11 1:42 ` [Patch V2 7/9] [SCSI] aacraid: Unblock IOCTLs to controller once system resumed from suspend rajinikanth.pandurangan
2015-06-16 11:41 ` Johannes Thumshirn
2015-06-24 5:41 ` Mahesh Rajashekhara
2015-06-11 1:42 ` [Patch V2 8/9] [SCSI] aacraid: Send commit-config to controller firmware rajinikanth.pandurangan
2015-06-24 5:42 ` Mahesh Rajashekhara
2015-06-11 1:42 ` [Patch V2 9/9] [SCSI] aacraid: Update driver version rajinikanth.pandurangan
2015-06-16 11:43 ` Johannes Thumshirn
2015-06-24 5:42 ` Mahesh Rajashekhara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=558C42E6.80603@redhat.com \
--to=thenzl@redhat.com \
--cc=aacraid@pmc-sierra.com \
--cc=achim.leubner@pmcs.com \
--cc=harry.yang@pmcs.com \
--cc=jbottomley@parallels.com \
--cc=linux-scsi@vger.kernel.org \
--cc=mahesh.rajashekhara@pmcs.com \
--cc=murthy.bhat@pmcs.com \
--cc=rajinikanth.pandurangan@pmcs.com \
--cc=rich.bono@pmcs.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).