arcmsr: &/| confusion in arcmsr_build_ccb()

The WRITE_{6,10,12} are defined numbers, so the branch was always taken.

---
 drivers/scsi/arcmsr/arcmsr_hba.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

See include/scsi/scsi.h:58-123 for

#define WRITE_6               0x0a
#define WRITE_10              0x2a
#define WRITE_12              0xaa

index 17e3df4..a6e5222 100644
@@ -1171,7 +1171,8 @@ static int arcmsr_build_ccb(struct AdapterControlBlock *acb,
 	arcmsr_cdb->msgPages = arccdbsize/0x100 + (arccdbsize % 0x100 ? 1 : 0);
 	if ( arccdbsize > 256)
 		arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_SGL_BSIZE;
-	if (pcmd->cmnd[0]|WRITE_6 || pcmd->cmnd[0]|WRITE_10 || pcmd->cmnd[0]|WRITE_12 ){
+	if (pcmd->cmnd[0] & WRITE_6 || pcmd->cmnd[0] & WRITE_10 ||
+			pcmd->cmnd[0] & WRITE_12 ){
 		arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_WRITE;
 	}
 	ccb->arc_cdb_size = arccdbsize;

You're right, but even this will basically always set the write flag, since some bits will be set the same, even for reads. Eg:

#define READ_10 0x28
#define WRITE_10 0x2a

It should instead be:

	if (pcmd->sc_data_direction == DMA_TO_DEVICE)
		arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_WRITE;

Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

<http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=ae52e7f09ff509df11cd408eabe90132b6be1231;hp=f034260db330bb3ffc815fcb682b1c84aca09591#patch2>
@@ -1034,10 +1075,10 @@ static int arcmsr_build_ccb(struct AdapterControlBlock *acb,

                }
                arcmsr_cdb->sgcount = (uint8_t)cdb_sgcount;
                arcmsr_cdb->DataLength = scsi_bufflen(pcmd);
+       arcmsr_cdb->msgPages = arccdbsize/0x100 + (arccdbsize % 0x100 ? 1 : 0);
                if ( arccdbsize > 256)
                        arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_SGL_BSIZE;
-       }
-       if (pcmd->sc_data_direction == DMA_TO_DEVICE ) {
+       if (pcmd->cmnd[0]|WRITE_6 || pcmd->cmnd[0] | WRITE_10 ||
pcmd->cmnd[0]|WRITE_12) {
                arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_WRITE;
                ccb->ccb_flags |= CCB_FLAG_WRITE;
        }

Until June 2010 it was that way...
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

So it was. Nick, why did you make this change? Your commit message is less than informative, partly because you're making such huge changes in a single commit.

Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Make sure no other command which does a write, such as UNMAP and WRITE_32, is missed.

---
 drivers/scsi/arcmsr/arcmsr_hba.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

If you agree this is the right fix, please ack.

index 17e3df4..a5acedc 100644
@@ -1171,7 +1171,7 @@ static int arcmsr_build_ccb(struct AdapterControlBlock *acb,
 	arcmsr_cdb->msgPages = arccdbsize/0x100 + (arccdbsize % 0x100 ? 1 : 0);
 	if ( arccdbsize > 256)
 		arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_SGL_BSIZE;
-	if (pcmd->cmnd[0]|WRITE_6 || pcmd->cmnd[0]|WRITE_10 || pcmd->cmnd[0]|WRITE_12 ){
+	if (pcmd->sc_data_direction == DMA_TO_DEVICE)
 		arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_WRITE;
 	}
 	ccb->arc_cdb_size = arccdbsize;

Hi Roel,
Thanks for your reminding.
I agree with you.
This patch is absolutely correct.
-----Original Message-----
From: roel kluin [mailto:[email protected]]
Sent: Sunday, January 02, 2011 2:40 AM
To: Matthew Wilcox; [email protected]
Cc: [email protected]; Andrew Morton; LKML; [email protected]; [email protected]
Subject: Re: [PATCH] [SCSI] arcmsr: &/| confusion in arcmsr_build_ccb()

Make sure no other command which does a write, such as UNMAP and WRITE_32, is missed.

---
 drivers/scsi/arcmsr/arcmsr_hba.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

If you agree this is the right fix, please ack.

b/drivers/scsi/arcmsr/arcmsr_hba.c
index 17e3df4..a5acedc 100644

@@ -1171,7 +1171,7 @@ static int arcmsr_build_ccb(struct AdapterControlBlock

*acb,
arcmsr_cdb->msgPages = arccdbsize/0x100 + (arccdbsize % 0x100 ? 1 :
0);
if ( arccdbsize > 256)
arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_SGL_BSIZE;
- if (pcmd->cmnd[0]|WRITE_6 || pcmd->cmnd[0]|WRITE_10 ||
pcmd->cmnd[0]|WRITE_12 ){
+ if (pcmd->sc_data_direction == DMA_TO_DEVICE)
arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_WRITE;
}
ccb->arc_cdb_size = arccdbsize;

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Fix the issue of system hangup after commands timeout on ARC-1200

---

index 475c31a..77b26f5 100644
@@ -2,7 +2,7 @@
 

****************************************************************************
***

 **        O.S   : Linux
 **   FILE NAME  : arcmsr.h
-**        BY    : Erich Chen
+**        BY    : Nick Cheng

** Description: SCSI RAID Device Driver for
** ARECA RAID Host adapter

****************************************************************************
***
@@ -46,8 +46,12 @@
struct device_attribute;
/*The limit of outstanding scsi command that firmware can handle*/ #define ARCMSR_MAX_OUTSTANDING_CMD
256
-#define ARCMSR_MAX_FREECCB_NUM
320
-#define ARCMSR_DRIVER_VERSION "Driver Version 1.20.00.15
2010/02/02"
+#ifdef CONFIG_XEN
+ #define ARCMSR_MAX_FREECCB_NUM 160
+#else
+ #define ARCMSR_MAX_FREECCB_NUM 320
+#endif
+#define ARCMSR_DRIVER_VERSION "Driver Version 1.20.00.15
2010/08/05"
#define ARCMSR_SCSI_INITIATOR_ID
255
#define ARCMSR_MAX_XFER_SECTORS
512
#define ARCMSR_MAX_XFER_SECTORS_B
4096
@@ -60,7 +64,6 @@ struct device_attribute;
#define ARCMSR_MAX_HBB_POSTQUEUE
264
#define ARCMSR_MAX_XFER_LEN
0x26000 /* 152K */
#define ARCMSR_CDB_SG_PAGE_LENGTH
256
-#define SCSI_CMD_ARECA_SPECIFIC
0xE1
#ifndef PCI_DEVICE_ID_ARECA_1880
#define PCI_DEVICE_ID_ARECA_1880 0x1880
#endif

b/drivers/scsi/arcmsr/arcmsr_attr.c
index a4e04c5..acdae33 100644

@@ -2,7 +2,7 @@
 

****************************************************************************
***

 **        O.S   : Linux
 **   FILE NAME  : arcmsr_attr.c
-**        BY    : Erich Chen
+**        BY    : Nick Cheng

** Description: attributes exported to sysfs and device host

****************************************************************************
***
** Copyright (C) 2002 - 2005, Areca Technology Corporation All rights
reserved

b/drivers/scsi/arcmsr/arcmsr_hba.c
index 17e3df4..c650310 100644

@@ -2,7 +2,7 @@
 

****************************************************************************
***

 **        O.S   : Linux
 **   FILE NAME  : arcmsr_hba.c
-**        BY    : Erich Chen
+**        BY    : Nick Cheng

** Description: SCSI RAID Device Driver for
** ARECA RAID Host adapter

****************************************************************************
***
@@ -76,7 +76,7 @@ MODULE_DESCRIPTION("ARECA (ARC11xx/12xx/16xx/1880) SATA/SAS RAID Host Bus Adapte
MODULE_LICENSE("Dual BSD/GPL"); MODULE_VERSION(ARCMSR_DRIVER_VERSION);
static int sleeptime = 10;
-static int retrycount = 30;
+static int retrycount = 12;
wait_queue_head_t wait_q;
static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
struct scsi_cmnd *cmd);
@@ -187,7 +187,6 @@ int arcmsr_sleep_for_bus_reset(struct scsi_cmnd *cmd)
if (isleep > 0) {
msleep(isleep*1000);
}
- printk(KERN_NOTICE "wake-up\n");
return 0;
}

@@ -921,7 +920,6 @@ static void arcmsr_report_ccb_state(struct AdapterControlBlock *acb,
}

static void arcmsr_drain_donequeue(struct AdapterControlBlock *acb, struct
CommandControlBlock *pCCB, bool error)
-
{
int id, lun;
if ((pCCB->acb != acb) || (pCCB->startdone != ARCMSR_CCB_START)) {
@@ -948,7 +946,7 @@ static void arcmsr_drain_donequeue(struct AdapterControlBlock *acb, struct Comma
, pCCB->startdone
, atomic_read(&acb->ccboutstandingcount));
return;
- }
+ }
arcmsr_report_ccb_state(acb, pCCB, error);
}

@@ -981,7 +979,7 @@ static void arcmsr_done4abort_postqueue(struct AdapterControlBlock *acb)
case ACB_ADAPTER_TYPE_B: {
struct MessageUnit_B *reg = acb->pmuB;
/*clear all outbound posted Q*/
- writel(ARCMSR_DOORBELL_INT_CLEAR_PATTERN,
&reg->iop2drv_doorbell); /* clear doorbell interrupt */
+ writel(ARCMSR_DOORBELL_INT_CLEAR_PATTERN,
reg->iop2drv_doorbell); /* clear doorbell interrupt */
for (i = 0; i < ARCMSR_MAX_HBB_POSTQUEUE; i++) {
if ((flag_ccb = readl(&reg->done_qbuffer[i])) != 0)
{
writel(0, &reg->done_qbuffer[i]);
@@ -1171,7 +1169,7 @@ static int arcmsr_build_ccb(struct AdapterControlBlock *acb,
arcmsr_cdb->msgPages = arccdbsize/0x100 + (arccdbsize % 0x100 ? 1 :
0);
if ( arccdbsize > 256)
arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_SGL_BSIZE;
- if (pcmd->cmnd[0]|WRITE_6 || pcmd->cmnd[0]|WRITE_10 ||
pcmd->cmnd[0]|WRITE_12 ){
+ if (pcmd->sc_data_direction == DMA_TO_DEVICE) {
arcmsr_cdb->Flags |= ARCMSR_CDB_FLAG_WRITE;
}
ccb->arc_cdb_size = arccdbsize;
@@ -1512,7 +1510,6 @@ static void arcmsr_hba_postqueue_isr(struct AdapterControlBlock *acb)
arcmsr_drain_donequeue(acb, pCCB, error);
}
}
-
static void arcmsr_hbb_postqueue_isr(struct AdapterControlBlock *acb) {
uint32_t index;
@@ -2107,10 +2104,6 @@ static int arcmsr_queue_command_lck(struct scsi_cmnd *cmd,
if (atomic_read(&acb->ccboutstandingcount) >=
ARCMSR_MAX_OUTSTANDING_CMD)
return SCSI_MLQUEUE_HOST_BUSY;
- if ((scsicmd == SCSI_CMD_ARECA_SPECIFIC)) {
- printk(KERN_NOTICE "Receiveing SCSI_CMD_ARECA_SPECIFIC
command..\n");
- return 0;
- }
ccb = arcmsr_get_freeccb(acb);
if (!ccb)
return SCSI_MLQUEUE_HOST_BUSY;
@@ -2394,6 +2387,7 @@ static int arcmsr_polling_hbb_ccbdone(struct AdapterControlBlock *acb,
int index, rtn;
bool error;
polling_hbb_ccb_retry:
+
poll_count++;
/* clear doorbell interrupt */ writel(ARCMSR_DOORBELL_INT_CLEAR_PATTERN, reg->iop2drv_doorbell);
@@ -2664,6 +2658,7 @@ static void arcmsr_request_hba_device_map(struct AdapterControlBlock *acb)
{
struct MessageUnit_A __iomem *reg = acb->pmuA;
if (unlikely(atomic_read(&acb->rq_map_token) == 0) ||
((acb->acb_flags & ACB_F_BUS_RESET) != 0 ) || ((acb->acb_flags & ACB_F_ABORT) != 0 )){
+ mod_timer(&acb->eternal_timer, jiffies + msecs_to_jiffies(6
* HZ));
return;
} else {
acb->fw_flag = FW_NORMAL;
@@ -2671,8 +2666,10 @@ static void arcmsr_request_hba_device_map(struct AdapterControlBlock *acb)
atomic_set(&acb->rq_map_token, 16);
}
atomic_set(&acb->ante_token_value,
atomic_read(&acb->rq_map_token));
- if (atomic_dec_and_test(&acb->rq_map_token))
+ if (atomic_dec_and_test(&acb->rq_map_token)) {
+ mod_timer(&acb->eternal_timer, jiffies +
msecs_to_jiffies(6 * HZ));
return;
+ }
writel(ARCMSR_INBOUND_MESG0_GET_CONFIG,
&reg->inbound_msgaddr0);
mod_timer(&acb->eternal_timer, jiffies + msecs_to_jiffies(6
* HZ));
}
@@ -2683,15 +2680,18 @@ static void arcmsr_request_hbb_device_map(struct AdapterControlBlock *acb)
{
struct MessageUnit_B __iomem *reg = acb->pmuB;
if (unlikely(atomic_read(&acb->rq_map_token) == 0) ||
((acb->acb_flags & ACB_F_BUS_RESET) != 0 ) || ((acb->acb_flags & ACB_F_ABORT) != 0 )){
+ mod_timer(&acb->eternal_timer, jiffies + msecs_to_jiffies(6
* HZ));
return;
} else {
acb->fw_flag = FW_NORMAL;
if (atomic_read(&acb->ante_token_value) ==
atomic_read(&acb->rq_map_token)) {
- atomic_set(&acb->rq_map_token,16);
+ atomic_set(&acb->rq_map_token, 16);
}
atomic_set(&acb->ante_token_value,
atomic_read(&acb->rq_map_token));
- if(atomic_dec_and_test(&acb->rq_map_token))
+ if(atomic_dec_and_test(&acb->rq_map_token)) {
+ mod_timer(&acb->eternal_timer, jiffies +
msecs_to_jiffies(6 * HZ));
return;
+ }
writel(ARCMSR_MESSAGE_GET_CONFIG, reg->drv2iop_doorbell); mod_timer(&acb->eternal_timer, jiffies + msecs_to_jiffies(6
* HZ));
}
@@ -2702,6 +2702,7 @@ static void arcmsr_request_hbc_device_map(struct AdapterControlBlock *acb)
{
struct MessageUnit_C __iomem *reg = acb->pmuC;
if (unlikely(atomic_read(&acb->rq_map_token) == 0) ||
((acb->acb_flags & ACB_F_BUS_RESET) != 0) || ((acb->acb_flags & ACB_F_ABORT) != 0)) {
+ mod_timer(&acb->eternal_timer, jiffies + msecs_to_jiffies(6
* HZ));
return;
} else {
acb->fw_flag = FW_NORMAL;
@@ -2709,8 +2710,10 @@ static void arcmsr_request_hbc_device_map(struct AdapterControlBlock *acb)
atomic_set(&acb->rq_map_token, 16);
}
atomic_set(&acb->ante_token_value,
atomic_read(&acb->rq_map_token));
- if (atomic_dec_and_test(&acb->rq_map_token))
+ if (atomic_dec_and_test(&acb->rq_map_token)) {
+ mod_timer(&acb->eternal_timer, jiffies +
msecs_to_jiffies(6 * HZ));
return;
+ }
writel(ARCMSR_INBOUND_MESG0_GET_CONFIG,
&reg->inbound_msgaddr0);
writel(ARCMSR_HBCMU_DRV2IOP_MESSAGE_CMD_DONE,
&reg->inbound_doorbell);
mod_timer(&acb->eternal_timer, jiffies + msecs_to_jiffies(6
* HZ));
@@ -2898,6 +2901,8 @@ static uint8_t arcmsr_iop_reset(struct AdapterControlBlock *acb)
uint32_t intmask_org;
uint8_t rtnval = 0x00;
int i = 0;
+ unsigned long flags;
+
if (atomic_read(&acb->ccboutstandingcount) != 0) {
/* disable all outbound interrupt */
intmask_org = arcmsr_disable_outbound_ints(acb);
@@ -2908,7 +2913,12 @@ static uint8_t arcmsr_iop_reset(struct AdapterControlBlock *acb)
for (i = 0; i < ARCMSR_MAX_FREECCB_NUM; i++) {
ccb = acb->pccb_pool[i];
if (ccb->startdone == ARCMSR_CCB_START) {
- arcmsr_ccb_complete(ccb);
+ scsi_dma_unmap(ccb->pcmd);
+ ccb->startdone = ARCMSR_CCB_DONE;
+ ccb->ccb_flags = 0;
+ spin_lock_irqsave(&acb->ccblist_lock,
flags);
+ list_add_tail(&ccb->list,
&acb->ccb_free_list);
+ spin_unlock_irqrestore(&acb->ccblist_lock,
flags);
}
}
atomic_set(&acb->ccboutstandingcount, 0);
@@ -2921,8 +2931,7 @@ static uint8_t arcmsr_iop_reset(struct AdapterControlBlock *acb)

static int arcmsr_bus_reset(struct scsi_cmnd *cmd)
{
- struct AdapterControlBlock *acb =
- (struct AdapterControlBlock *)cmd->device->host->hostdata;
+ struct AdapterControlBlock *acb;
uint32_t intmask_org, outbound_doorbell;
int retry_count = 0;
int rtn = FAILED;
@@ -2972,31 +2981,16 @@ sleep_again:
atomic_set(&acb->rq_map_token, 16); atomic_set(&acb->ante_token_value, 16); acb->fw_flag = FW_NORMAL;
- init_timer(&acb->eternal_timer);
- acb->eternal_timer.expires = jiffies +
msecs_to_jiffies(6*HZ);
- acb->eternal_timer.data = (unsigned long)
acb;
- acb->eternal_timer.function =
&arcmsr_request_device_map;
- add_timer(&acb->eternal_timer);
+ mod_timer(&acb->eternal_timer, jiffies +
msecs_to_jiffies(6 * HZ));
acb->acb_flags &= ~ACB_F_BUS_RESET; rtn = SUCCESS;
printk(KERN_ERR "arcmsr: scsi bus reset eh
returns with success\n");
} else {
acb->acb_flags &= ~ACB_F_BUS_RESET;
- if (atomic_read(&acb->rq_map_token) == 0) {
- atomic_set(&acb->rq_map_token, 16);
- atomic_set(&acb->ante_token_value,
16);
- acb->fw_flag = FW_NORMAL;
- init_timer(&acb->eternal_timer);
- acb->eternal_timer.expires =
jiffies + msecs_to_jiffies(6*HZ);
- acb->eternal_timer.data = (unsigned
long) acb;
- acb->eternal_timer.function =
&arcmsr_request_device_map;
- add_timer(&acb->eternal_timer);
- } else {
- atomic_set(&acb->rq_map_token, 16);
- atomic_set(&acb->ante_token_value,
16);
- acb->fw_flag = FW_NORMAL;
- mod_timer(&acb->eternal_timer,
jiffies + msecs_to_jiffies(6*HZ));
- }
+ atomic_set(&acb->rq_map_token, 16);
+ atomic_set(&acb->ante_token_value, 16);
+ acb->fw_flag = FW_NORMAL;
+ mod_timer(&acb->eternal_timer, jiffies +
msecs_to_jiffies(6*HZ));
rtn = SUCCESS;
}
break;
@@ -3008,21 +3002,10 @@ sleep_again:
rtn = FAILED;
} else {
acb->acb_flags &= ~ACB_F_BUS_RESET;
- if (atomic_read(&acb->rq_map_token) == 0) {
- atomic_set(&acb->rq_map_token, 16);
- atomic_set(&acb->ante_token_value,
16);
- acb->fw_flag = FW_NORMAL;
- init_timer(&acb->eternal_timer);
- acb->eternal_timer.expires =
jiffies + msecs_to_jiffies(6*HZ);
- acb->eternal_timer.data = (unsigned
long) acb;
- acb->eternal_timer.function =
&arcmsr_request_device_map;
- add_timer(&acb->eternal_timer);
- } else {
- atomic_set(&acb->rq_map_token, 16);
- atomic_set(&acb->ante_token_value,
16);
- acb->fw_flag = FW_NORMAL;
- mod_timer(&acb->eternal_timer,
jiffies + msecs_to_jiffies(6*HZ));
- }
+ atomic_set(&acb->rq_map_token, 16);
+ atomic_set(&acb->ante_token_value, 16);
+ acb->fw_flag = FW_NORMAL;
+ mod_timer(&acb->eternal_timer, jiffies +
msecs_to_jiffies(6 * HZ));
rtn = SUCCESS;
}
break;
@@ -3068,31 +3051,16 @@ sleep:
atomic_set(&acb->rq_map_token, 16); atomic_set(&acb->ante_token_value, 16); acb->fw_flag = FW_NORMAL;
- init_timer(&acb->eternal_timer);
- acb->eternal_timer.expires = jiffies +
msecs_to_jiffies(6 * HZ);
- acb->eternal_timer.data = (unsigned long)
acb;
- acb->eternal_timer.function =
&arcmsr_request_device_map;
- add_timer(&acb->eternal_timer);
+ mod_timer(&acb->eternal_timer, jiffies +
msecs_to_jiffies(6 * HZ));
acb->acb_flags &= ~ACB_F_BUS_RESET; rtn = SUCCESS;
printk(KERN_ERR "arcmsr: scsi bus reset eh
returns with success\n");
} else {
acb->acb_flags &= ~ACB_F_BUS_RESET;
- if (atomic_read(&acb->rq_map_token) == 0) {
- atomic_set(&acb->rq_map_token, 16);
- atomic_set(&acb->ante_token_value,
16);
- acb->fw_flag = FW_NORMAL;
- init_timer(&acb->eternal_timer);
- acb->eternal_timer.expires =
jiffies + msecs_to_jiffies(6*HZ);
- acb->eternal_timer.data = (unsigned
long) acb;
- acb->eternal_timer.function =
&arcmsr_request_device_map;
- add_timer(&acb->eternal_timer);
- } else {
- atomic_set(&acb->rq_map_token, 16);
- atomic_set(&acb->ante_token_value,
16);
- acb->fw_flag = FW_NORMAL;
- mod_timer(&acb->eternal_timer,
jiffies + msecs_to_jiffies(6*HZ));
- }
+ atomic_set(&acb->rq_map_token, 16);
+ atomic_set(&acb->ante_token_value, 16);
+ acb->fw_flag = FW_NORMAL;
+ mod_timer(&acb->eternal_timer, jiffies +
msecs_to_jiffies(6*HZ));
rtn = SUCCESS;
}
break;