All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] arcmsr: code cleanup and some corrections
@ 2011-02-10 14:22 Tomas Henzl
  2011-02-10 14:36 ` [PATCH 2/2] " Tomas Henzl
  2011-02-17 15:07 ` [PATCH 1/2] " Tomas Henzl
  0 siblings, 2 replies; 15+ messages in thread
From: Tomas Henzl @ 2011-02-10 14:22 UTC (permalink / raw)
  To: 'linux-scsi@vger.kernel.org'; +Cc: nick.cheng

I removed outer loops in ...wait_msgint_ready
the sleeptime and retrycount are in fact never changed so I changed them
into defines. In arcmsr_flush_hba_cache is a loop removed, which
printed the same printk 100 times, one line in log is enough I think.
The arcmsr_sleep_for_bus_reset has lost a functionality with the latest patches,
The only thing the function does is a long sleep, so it's replaced with a ssleep.

Signed-off-by: Tomas henzl <thenzl@redhat.com>

diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
index 984bd52..4cd522b 100644
--- a/drivers/scsi/arcmsr/arcmsr_hba.c
+++ b/drivers/scsi/arcmsr/arcmsr_hba.c
@@ -75,8 +75,10 @@ MODULE_AUTHOR("Nick Cheng <support@areca.com.tw>");
 MODULE_DESCRIPTION("ARECA (ARC11xx/12xx/16xx/1880) SATA/SAS RAID Host Bus Adapter");
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_VERSION(ARCMSR_DRIVER_VERSION);
-static int sleeptime = 10;
-static int retrycount = 12;
+
+#define	ARCMSR_SLEEPTIME	10
+#define	ARCMSR_RETRYCOUNT	12
+
 wait_queue_head_t wait_q;
 static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
 					struct scsi_cmnd *cmd);
@@ -171,24 +173,6 @@ static struct pci_driver arcmsr_pci_driver = {
 ****************************************************************************
 ****************************************************************************
 */
-int arcmsr_sleep_for_bus_reset(struct scsi_cmnd *cmd)
-{
-		struct Scsi_Host *shost = NULL;
-		int i, isleep;
-		shost = cmd->device->host;
-		isleep = sleeptime / 10;
-		if (isleep > 0) {
-			for (i = 0; i < isleep; i++) {
-				msleep(10000);
-			}
-		}
-
-		isleep = sleeptime % 10;
-		if (isleep > 0) {
-			msleep(isleep*1000);
-		}
-		return 0;
-}
 
 static void arcmsr_free_hbb_mu(struct AdapterControlBlock *acb)
 {
@@ -323,66 +307,64 @@ static void arcmsr_define_adapter_type(struct AdapterControlBlock *acb)
 
 	default: acb->adapter_type = ACB_ADAPTER_TYPE_A;
 	}
-}	
+}
 
 static uint8_t arcmsr_hba_wait_msgint_ready(struct AdapterControlBlock *acb)
 {
 	struct MessageUnit_A __iomem *reg = acb->pmuA;
-	uint32_t Index;
-	uint8_t Retries = 0x00;
-	do {
-		for (Index = 0; Index < 100; Index++) {
-			if (readl(&reg->outbound_intstatus) &
-					ARCMSR_MU_OUTBOUND_MESSAGE0_INT) {
-				writel(ARCMSR_MU_OUTBOUND_MESSAGE0_INT,
-					&reg->outbound_intstatus);
-				return true;
-			}
-			msleep(10);
-		}/*max 1 seconds*/
+	int i;
+
+	for (i = 0; i < 2000; i++) {
+		if (readl(&reg->outbound_intstatus) &
+				ARCMSR_MU_OUTBOUND_MESSAGE0_INT) {
+			writel(ARCMSR_MU_OUTBOUND_MESSAGE0_INT,
+				&reg->outbound_intstatus);
+			return true;
+		}
+		msleep(10);
+	}/* max 20 seconds */
 
-	} while (Retries++ < 20);/*max 20 sec*/
 	return false;
 }
 
 static uint8_t arcmsr_hbb_wait_msgint_ready(struct AdapterControlBlock *acb)
 {
 	struct MessageUnit_B *reg = acb->pmuB;
-	uint32_t Index;
-	uint8_t Retries = 0x00;
-	do {
-		for (Index = 0; Index < 100; Index++) {
-			if (readl(reg->iop2drv_doorbell)
-				& ARCMSR_IOP2DRV_MESSAGE_CMD_DONE) {
-				writel(ARCMSR_MESSAGE_INT_CLEAR_PATTERN
-					, reg->iop2drv_doorbell);
-				writel(ARCMSR_DRV2IOP_END_OF_INTERRUPT, reg->drv2iop_doorbell);
-				return true;
-			}
-			msleep(10);
-		}/*max 1 seconds*/
+	int i;
+
+	for (i = 0; i < 2000; i++) {
+		if (readl(reg->iop2drv_doorbell)
+			& ARCMSR_IOP2DRV_MESSAGE_CMD_DONE) {
+			writel(ARCMSR_MESSAGE_INT_CLEAR_PATTERN,
+					reg->iop2drv_doorbell);
+			writel(ARCMSR_DRV2IOP_END_OF_INTERRUPT,
+					reg->drv2iop_doorbell);
+			return true;
+		}
+		msleep(10);
+	}/* max 20 seconds */
 
-	} while (Retries++ < 20);/*max 20 sec*/
 	return false;
 }
 
 static uint8_t arcmsr_hbc_wait_msgint_ready(struct AdapterControlBlock *pACB)
 {
 	struct MessageUnit_C *phbcmu = (struct MessageUnit_C *)pACB->pmuC;
-	unsigned char Retries = 0x00;
-	uint32_t Index;
-	do {
-		for (Index = 0; Index < 100; Index++) {
-			if (readl(&phbcmu->outbound_doorbell) & ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE) {
-				writel(ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE_DOORBELL_CLEAR, &phbcmu->outbound_doorbell_clear);/*clear interrupt*/
-				return true;
-			}
-			/* one us delay	*/
-			msleep(10);
-		} /*max 1 seconds*/
-	} while (Retries++ < 20); /*max 20 sec*/
+	int i;
+
+	for (i = 0; i < 2000; i++) {
+		if (readl(&phbcmu->outbound_doorbell)
+				& ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE) {
+			writel(ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE_DOORBELL_CLEAR,
+				&phbcmu->outbound_doorbell_clear); /*clear interrupt*/
+			return true;
+		}
+		msleep(10);
+	} /* max 20 seconds */
+
 	return false;
 }
+
 static void arcmsr_flush_hba_cache(struct AdapterControlBlock *acb)
 {
 	struct MessageUnit_A __iomem *reg = acb->pmuA;
@@ -2602,12 +2584,8 @@ static int arcmsr_iop_confirm(struct AdapterControlBlock *acb)
 		if (cdb_phyaddr_hi32 != 0) {
 			struct MessageUnit_C *reg = (struct MessageUnit_C *)acb->pmuC;
 
-			if (cdb_phyaddr_hi32 != 0) {
-				unsigned char Retries = 0x00;
-				do {
-					printk(KERN_NOTICE "arcmsr%d: cdb_phyaddr_hi32=0x%x \n", acb->adapter_index, cdb_phyaddr_hi32);
-				} while (Retries++ < 100);
-			}
+			printk(KERN_NOTICE "arcmsr%d: cdb_phyaddr_hi32=0x%x \n",
+					acb->adapter_index, cdb_phyaddr_hi32);
 			writel(ARCMSR_SIGNATURE_SET_CONFIG, &reg->msgcode_rwbuffer[0]);
 			writel(cdb_phyaddr_hi32, &reg->msgcode_rwbuffer[1]);
 			writel(ARCMSR_INBOUND_MESG0_SET_CONFIG, &reg->inbound_msgaddr0);
@@ -2955,10 +2933,10 @@ static int arcmsr_bus_reset(struct scsi_cmnd *cmd)
 				arcmsr_hardware_reset(acb);
 				acb->acb_flags &= ~ACB_F_IOP_INITED;
 sleep_again:
-				arcmsr_sleep_for_bus_reset(cmd);
+				ssleep(ARCMSR_SLEEPTIME);
 				if ((readl(&reg->outbound_msgaddr1) & ARCMSR_OUTBOUND_MESG1_FIRMWARE_OK) == 0) {
 					printk(KERN_ERR "arcmsr%d: waiting for hw bus reset return, retry=%d \n", acb->host->host_no, retry_count);
-					if (retry_count > retrycount) {
+					if (retry_count > ARCMSR_RETRYCOUNT) {
 						acb->fw_flag = FW_DEADLOCK;
 						printk(KERN_ERR "arcmsr%d: waiting for hw bus reset return, RETRY TERMINATED!! \n", acb->host->host_no);
 						return FAILED;
@@ -3025,10 +3003,10 @@ sleep_again:
 				arcmsr_hardware_reset(acb);
 				acb->acb_flags &= ~ACB_F_IOP_INITED;
 sleep:
-				arcmsr_sleep_for_bus_reset(cmd);
+				ssleep(ARCMSR_SLEEPTIME);
 				if ((readl(&reg->host_diagnostic) & 0x04) != 0) {
 					printk(KERN_ERR "arcmsr%d: waiting for hw bus reset return, retry=%d \n", acb->host->host_no, retry_count);
-					if (retry_count > retrycount) {
+					if (retry_count > ARCMSR_RETRYCOUNT) {
 						acb->fw_flag = FW_DEADLOCK;
 						printk(KERN_ERR "arcmsr%d: waiting for hw bus reset return, RETRY TERMINATED!! \n", acb->host->host_no);
 						return FAILED;



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/2] arcmsr: code cleanup and some corrections
  2011-02-10 14:22 [PATCH 1/2] arcmsr: code cleanup and some corrections Tomas Henzl
@ 2011-02-10 14:36 ` Tomas Henzl
  2011-02-11  1:44   ` NickCheng
  2011-02-17 15:07 ` [PATCH 1/2] " Tomas Henzl
  1 sibling, 1 reply; 15+ messages in thread
From: Tomas Henzl @ 2011-02-10 14:36 UTC (permalink / raw)
  To: 'linux-scsi@vger.kernel.org'; +Cc: nick.cheng

Hi Nick,
I'm confused with what this code does, I think I must miss something.

	dma_coherent = dma_alloc_coherent(&pdev->dev, acb->uncache_size, &dma_coherent_handle, GFP_KERNEL);
...
I think that the dma_alloc_coherent returns a page address with zeroes in the lowest bits 
That would mean the offset computed below is useless as the result is zero every time.
Added to that is that you then increase dma_coherent_handle by the offset,
while dma_coherent is increased by multiples of sizeof CommandControlBlock, at least it looks so
...
	offset = roundup((unsigned long)dma_coherent, 32) - (unsigned long)dma_coherent;
	dma_coherent_handle = dma_coherent_handle + offset;
	dma_coherent = (struct CommandControlBlock *)dma_coherent + offset;

The patch below removes the offset computation.

Signed-off-by: Tomas henzl <thenzl@redhat.com>


diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
index 4cd522b..da93974 100644
--- a/drivers/scsi/arcmsr/arcmsr_hba.c
+++ b/drivers/scsi/arcmsr/arcmsr_hba.c
@@ -441,10 +441,11 @@ static int arcmsr_alloc_ccb_pool(struct AdapterControlBlock *acb)
 	struct CommandControlBlock *ccb_tmp;
 	int i = 0, j = 0;
 	dma_addr_t cdb_phyaddr;
-	unsigned long roundup_ccbsize = 0, offset;
+	unsigned long roundup_ccbsize;
 	unsigned long max_xfer_len;
 	unsigned long max_sg_entrys;
 	uint32_t  firm_config_version;
+
 	for (i = 0; i < ARCMSR_MAX_TARGETID; i++)
 		for (j = 0; j < ARCMSR_MAX_TARGETLUN; j++)
 			acb->devstate[i][j] = ARECA_RAID_GONE;
@@ -454,12 +455,12 @@ static int arcmsr_alloc_ccb_pool(struct AdapterControlBlock *acb)
 	firm_config_version = acb->firm_cfg_version;
 	if((firm_config_version & 0xFF) >= 3){
 		max_xfer_len = (ARCMSR_CDB_SG_PAGE_LENGTH << ((firm_config_version >> 8) & 0xFF)) * 1024;/* max 4M byte */
-		max_sg_entrys = (max_xfer_len/4096);	
+		max_sg_entrys = (max_xfer_len/4096);
 	}
 	acb->host->max_sectors = max_xfer_len/512;
 	acb->host->sg_tablesize = max_sg_entrys;
 	roundup_ccbsize = roundup(sizeof(struct CommandControlBlock) + (max_sg_entrys - 1) * sizeof(struct SG64ENTRY), 32);
-	acb->uncache_size = roundup_ccbsize * ARCMSR_MAX_FREECCB_NUM + 32;
+	acb->uncache_size = roundup_ccbsize * ARCMSR_MAX_FREECCB_NUM;
 	dma_coherent = dma_alloc_coherent(&pdev->dev, acb->uncache_size, &dma_coherent_handle, GFP_KERNEL);
 	if(!dma_coherent){
 		printk(KERN_NOTICE "arcmsr%d: dma_alloc_coherent got error \n", acb->host->host_no);
@@ -468,9 +469,6 @@ static int arcmsr_alloc_ccb_pool(struct AdapterControlBlock *acb)
 	acb->dma_coherent = dma_coherent;
 	acb->dma_coherent_handle = dma_coherent_handle;
 	memset(dma_coherent, 0, acb->uncache_size);
-	offset = roundup((unsigned long)dma_coherent, 32) - (unsigned long)dma_coherent;
-	dma_coherent_handle = dma_coherent_handle + offset;
-	dma_coherent = (struct CommandControlBlock *)dma_coherent + offset;
 	ccb_tmp = dma_coherent;
 	acb->vir2phy_offset = (unsigned long)dma_coherent - (unsigned long)dma_coherent_handle;
 	for(i = 0; i < ARCMSR_MAX_FREECCB_NUM; i++){



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* RE: [PATCH 2/2] arcmsr: code cleanup and some corrections
  2011-02-10 14:36 ` [PATCH 2/2] " Tomas Henzl
@ 2011-02-11  1:44   ` NickCheng
  2011-02-17 15:05     ` Tomas Henzl
  0 siblings, 1 reply; 15+ messages in thread
From: NickCheng @ 2011-02-11  1:44 UTC (permalink / raw)
  To: 'Tomas Henzl', linux-scsi

Hi Tomas,
As you said, the dma_alloc_coherent should return a page address with zeroes
in the lowest bits, but I have 100% confidence in that.
Therefore, I try to round it up if not. Otherwise, it will lead the
controller panic.
The code over there is just in case.

-----Original Message-----
From: Tomas Henzl [mailto:thenzl@redhat.com] 
Sent: Thursday, February 10, 2011 10:37 PM
To: 'linux-scsi@vger.kernel.org'
Cc: nick.cheng@areca.com.tw
Subject: [PATCH 2/2] arcmsr: code cleanup and some corrections

Hi Nick,
I'm confused with what this code does, I think I must miss something.

	dma_coherent = dma_alloc_coherent(&pdev->dev, acb->uncache_size,
&dma_coherent_handle, GFP_KERNEL);
...
I think that the dma_alloc_coherent returns a page address with zeroes in
the lowest bits 
That would mean the offset computed below is useless as the result is zero
every time.
Added to that is that you then increase dma_coherent_handle by the offset,
while dma_coherent is increased by multiples of sizeof CommandControlBlock,
at least it looks so
...
	offset = roundup((unsigned long)dma_coherent, 32) - (unsigned
long)dma_coherent;
	dma_coherent_handle = dma_coherent_handle + offset;
	dma_coherent = (struct CommandControlBlock *)dma_coherent + offset;

The patch below removes the offset computation.

Signed-off-by: Tomas henzl <thenzl@redhat.com>


diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c
b/drivers/scsi/arcmsr/arcmsr_hba.c
index 4cd522b..da93974 100644
--- a/drivers/scsi/arcmsr/arcmsr_hba.c
+++ b/drivers/scsi/arcmsr/arcmsr_hba.c
@@ -441,10 +441,11 @@ static int arcmsr_alloc_ccb_pool(struct
AdapterControlBlock *acb)
 	struct CommandControlBlock *ccb_tmp;
 	int i = 0, j = 0;
 	dma_addr_t cdb_phyaddr;
-	unsigned long roundup_ccbsize = 0, offset;
+	unsigned long roundup_ccbsize;
 	unsigned long max_xfer_len;
 	unsigned long max_sg_entrys;
 	uint32_t  firm_config_version;
+
 	for (i = 0; i < ARCMSR_MAX_TARGETID; i++)
 		for (j = 0; j < ARCMSR_MAX_TARGETLUN; j++)
 			acb->devstate[i][j] = ARECA_RAID_GONE;
@@ -454,12 +455,12 @@ static int arcmsr_alloc_ccb_pool(struct
AdapterControlBlock *acb)
 	firm_config_version = acb->firm_cfg_version;
 	if((firm_config_version & 0xFF) >= 3){
 		max_xfer_len = (ARCMSR_CDB_SG_PAGE_LENGTH <<
((firm_config_version >> 8) & 0xFF)) * 1024;/* max 4M byte */
-		max_sg_entrys = (max_xfer_len/4096);	
+		max_sg_entrys = (max_xfer_len/4096);
 	}
 	acb->host->max_sectors = max_xfer_len/512;
 	acb->host->sg_tablesize = max_sg_entrys;
 	roundup_ccbsize = roundup(sizeof(struct CommandControlBlock) +
(max_sg_entrys - 1) * sizeof(struct SG64ENTRY), 32);
-	acb->uncache_size = roundup_ccbsize * ARCMSR_MAX_FREECCB_NUM + 32;
+	acb->uncache_size = roundup_ccbsize * ARCMSR_MAX_FREECCB_NUM;
 	dma_coherent = dma_alloc_coherent(&pdev->dev, acb->uncache_size,
&dma_coherent_handle, GFP_KERNEL);
 	if(!dma_coherent){
 		printk(KERN_NOTICE "arcmsr%d: dma_alloc_coherent got error
\n", acb->host->host_no);
@@ -468,9 +469,6 @@ static int arcmsr_alloc_ccb_pool(struct
AdapterControlBlock *acb)
 	acb->dma_coherent = dma_coherent;
 	acb->dma_coherent_handle = dma_coherent_handle;
 	memset(dma_coherent, 0, acb->uncache_size);
-	offset = roundup((unsigned long)dma_coherent, 32) - (unsigned
long)dma_coherent;
-	dma_coherent_handle = dma_coherent_handle + offset;
-	dma_coherent = (struct CommandControlBlock *)dma_coherent + offset;
 	ccb_tmp = dma_coherent;
 	acb->vir2phy_offset = (unsigned long)dma_coherent - (unsigned
long)dma_coherent_handle;
 	for(i = 0; i < ARCMSR_MAX_FREECCB_NUM; i++){



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] arcmsr: code cleanup and some corrections
  2011-02-11  1:44   ` NickCheng
@ 2011-02-17 15:05     ` Tomas Henzl
  2011-02-18  1:04       ` FUJITA Tomonori
  0 siblings, 1 reply; 15+ messages in thread
From: Tomas Henzl @ 2011-02-17 15:05 UTC (permalink / raw)
  To: NickCheng; +Cc: linux-scsi, 'James.Bottomley@HansenPartnership.com'

James,

the discussion is mostly if 'dma_alloc_coherent' 
...
dma_coherent = dma_alloc_coherent(&pdev->dev, acb->uncache_size, &dma_coherent_handle, GFP_KERNEL);
...
returns a page address - dma_coherent with last, I think five bits, zeroed.
Could you help us with that?



> Hi Tomas,
> As you said, the dma_alloc_coherent should return a page address with zeroes
> in the lowest bits, but I have 100% confidence in that.
> Therefore, I try to round it up if not. Otherwise, it will lead the
> controller panic.
> The code over there is just in case.
>   
Nick, let me explain why I think the fix you put there 'just in case' is broken.

* if the values were set for example so : dma_coherent = 0xc1000001  dma_coherent_handle = 0xc0000001

	offset = roundup((unsigned long)dma_coherent, 32) - (unsigned long)dma_coherent;
you will get an offset = 31

	dma_coherent_handle = dma_coherent_handle + offset;
here dma_coherent_handle has the right value of dma_coherent_handle = 0xc0000020

	dma_coherent = (struct CommandControlBlock *)dma_coherent + offset;
but dma_coherent = 0xc1000b25
this seems to be wrong, the right value should be 0xc1000020, am I right?


* you will maybe get a controller panic if you pass to your card a wrong value 
with last bits not zeroed out, but with that computation you only ensure that 
dma_coherent is rounded up and not the dma_coherent_handle which you then further
pass to the card.

---------
So my conclusion is that because nobody has complained, (and this has had 
a lot of testing) that a situation with a non null offset  hasn't happened yet.


--tomash


> Hi Nick,
> I'm confused with what this code does, I think I must miss something.
>
> 	dma_coherent = dma_alloc_coherent(&pdev->dev, acb->uncache_size,
> &dma_coherent_handle, GFP_KERNEL);
> ...
> I think that the dma_alloc_coherent returns a page address with zeroes in
> the lowest bits 
> That would mean the offset computed below is useless as the result is zero
> every time.
> Added to that is that you then increase dma_coherent_handle by the offset,
> while dma_coherent is increased by multiples of sizeof CommandControlBlock,
> at least it looks so
> ...
> 	offset = roundup((unsigned long)dma_coherent, 32) - (unsigned
> long)dma_coherent;
> 	dma_coherent_handle = dma_coherent_handle + offset;
> 	dma_coherent = (struct CommandControlBlock *)dma_coherent + offset;
>
> The patch below removes the offset computation.
>
> Signed-off-by: Tomas henzl <thenzl@redhat.com>
>
>
> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c
> b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 4cd522b..da93974 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -441,10 +441,11 @@ static int arcmsr_alloc_ccb_pool(struct
> AdapterControlBlock *acb)
>  	struct CommandControlBlock *ccb_tmp;
>  	int i = 0, j = 0;
>  	dma_addr_t cdb_phyaddr;
> -	unsigned long roundup_ccbsize = 0, offset;
> +	unsigned long roundup_ccbsize;
>  	unsigned long max_xfer_len;
>  	unsigned long max_sg_entrys;
>  	uint32_t  firm_config_version;
> +
>  	for (i = 0; i < ARCMSR_MAX_TARGETID; i++)
>  		for (j = 0; j < ARCMSR_MAX_TARGETLUN; j++)
>  			acb->devstate[i][j] = ARECA_RAID_GONE;
> @@ -454,12 +455,12 @@ static int arcmsr_alloc_ccb_pool(struct
> AdapterControlBlock *acb)
>  	firm_config_version = acb->firm_cfg_version;
>  	if((firm_config_version & 0xFF) >= 3){
>  		max_xfer_len = (ARCMSR_CDB_SG_PAGE_LENGTH <<
> ((firm_config_version >> 8) & 0xFF)) * 1024;/* max 4M byte */
> -		max_sg_entrys = (max_xfer_len/4096);	
> +		max_sg_entrys = (max_xfer_len/4096);
>  	}
>  	acb->host->max_sectors = max_xfer_len/512;
>  	acb->host->sg_tablesize = max_sg_entrys;
>  	roundup_ccbsize = roundup(sizeof(struct CommandControlBlock) +
> (max_sg_entrys - 1) * sizeof(struct SG64ENTRY), 32);
> -	acb->uncache_size = roundup_ccbsize * ARCMSR_MAX_FREECCB_NUM + 32;
> +	acb->uncache_size = roundup_ccbsize * ARCMSR_MAX_FREECCB_NUM;
>  	dma_coherent = dma_alloc_coherent(&pdev->dev, acb->uncache_size,
> &dma_coherent_handle, GFP_KERNEL);
>  	if(!dma_coherent){
>  		printk(KERN_NOTICE "arcmsr%d: dma_alloc_coherent got error
> \n", acb->host->host_no);
> @@ -468,9 +469,6 @@ static int arcmsr_alloc_ccb_pool(struct
> AdapterControlBlock *acb)
>  	acb->dma_coherent = dma_coherent;
>  	acb->dma_coherent_handle = dma_coherent_handle;
>  	memset(dma_coherent, 0, acb->uncache_size);
> -	offset = roundup((unsigned long)dma_coherent, 32) - (unsigned
> long)dma_coherent;
> -	dma_coherent_handle = dma_coherent_handle + offset;
> -	dma_coherent = (struct CommandControlBlock *)dma_coherent + offset;
>  	ccb_tmp = dma_coherent;
>  	acb->vir2phy_offset = (unsigned long)dma_coherent - (unsigned
> long)dma_coherent_handle;
>  	for(i = 0; i < ARCMSR_MAX_FREECCB_NUM; i++){
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] arcmsr: code cleanup and some corrections
  2011-02-10 14:22 [PATCH 1/2] arcmsr: code cleanup and some corrections Tomas Henzl
  2011-02-10 14:36 ` [PATCH 2/2] " Tomas Henzl
@ 2011-02-17 15:07 ` Tomas Henzl
  2011-02-18  1:26   ` NickCheng
  2011-02-24  2:02   ` NickCheng
  1 sibling, 2 replies; 15+ messages in thread
From: Tomas Henzl @ 2011-02-17 15:07 UTC (permalink / raw)
  To: 'linux-scsi@vger.kernel.org'; +Cc: nick.cheng

Hi Nick,

any comments here?

tomash


> I removed outer loops in ...wait_msgint_ready
> the sleeptime and retrycount are in fact never changed so I changed them
> into defines. In arcmsr_flush_hba_cache is a loop removed, which
> printed the same printk 100 times, one line in log is enough I think.
> The arcmsr_sleep_for_bus_reset has lost a functionality with the latest patches,
> The only thing the function does is a long sleep, so it's replaced with a ssleep.
>
> Signed-off-by: Tomas henzl <thenzl@redhat.com>
>
> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 984bd52..4cd522b 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -75,8 +75,10 @@ MODULE_AUTHOR("Nick Cheng <support@areca.com.tw>");
>  MODULE_DESCRIPTION("ARECA (ARC11xx/12xx/16xx/1880) SATA/SAS RAID Host Bus Adapter");
>  MODULE_LICENSE("Dual BSD/GPL");
>  MODULE_VERSION(ARCMSR_DRIVER_VERSION);
> -static int sleeptime = 10;
> -static int retrycount = 12;
> +
> +#define	ARCMSR_SLEEPTIME	10
> +#define	ARCMSR_RETRYCOUNT	12
> +
>  wait_queue_head_t wait_q;
>  static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
>  					struct scsi_cmnd *cmd);
> @@ -171,24 +173,6 @@ static struct pci_driver arcmsr_pci_driver = {
>  ****************************************************************************
>  ****************************************************************************
>  */
> -int arcmsr_sleep_for_bus_reset(struct scsi_cmnd *cmd)
> -{
> -		struct Scsi_Host *shost = NULL;
> -		int i, isleep;
> -		shost = cmd->device->host;
> -		isleep = sleeptime / 10;
> -		if (isleep > 0) {
> -			for (i = 0; i < isleep; i++) {
> -				msleep(10000);
> -			}
> -		}
> -
> -		isleep = sleeptime % 10;
> -		if (isleep > 0) {
> -			msleep(isleep*1000);
> -		}
> -		return 0;
> -}
>  
>  static void arcmsr_free_hbb_mu(struct AdapterControlBlock *acb)
>  {
> @@ -323,66 +307,64 @@ static void arcmsr_define_adapter_type(struct AdapterControlBlock *acb)
>  
>  	default: acb->adapter_type = ACB_ADAPTER_TYPE_A;
>  	}
> -}	
> +}
>  
>  static uint8_t arcmsr_hba_wait_msgint_ready(struct AdapterControlBlock *acb)
>  {
>  	struct MessageUnit_A __iomem *reg = acb->pmuA;
> -	uint32_t Index;
> -	uint8_t Retries = 0x00;
> -	do {
> -		for (Index = 0; Index < 100; Index++) {
> -			if (readl(&reg->outbound_intstatus) &
> -					ARCMSR_MU_OUTBOUND_MESSAGE0_INT) {
> -				writel(ARCMSR_MU_OUTBOUND_MESSAGE0_INT,
> -					&reg->outbound_intstatus);
> -				return true;
> -			}
> -			msleep(10);
> -		}/*max 1 seconds*/
> +	int i;
> +
> +	for (i = 0; i < 2000; i++) {
> +		if (readl(&reg->outbound_intstatus) &
> +				ARCMSR_MU_OUTBOUND_MESSAGE0_INT) {
> +			writel(ARCMSR_MU_OUTBOUND_MESSAGE0_INT,
> +				&reg->outbound_intstatus);
> +			return true;
> +		}
> +		msleep(10);
> +	}/* max 20 seconds */
>  
> -	} while (Retries++ < 20);/*max 20 sec*/
>  	return false;
>  }
>  
>  static uint8_t arcmsr_hbb_wait_msgint_ready(struct AdapterControlBlock *acb)
>  {
>  	struct MessageUnit_B *reg = acb->pmuB;
> -	uint32_t Index;
> -	uint8_t Retries = 0x00;
> -	do {
> -		for (Index = 0; Index < 100; Index++) {
> -			if (readl(reg->iop2drv_doorbell)
> -				& ARCMSR_IOP2DRV_MESSAGE_CMD_DONE) {
> -				writel(ARCMSR_MESSAGE_INT_CLEAR_PATTERN
> -					, reg->iop2drv_doorbell);
> -				writel(ARCMSR_DRV2IOP_END_OF_INTERRUPT, reg->drv2iop_doorbell);
> -				return true;
> -			}
> -			msleep(10);
> -		}/*max 1 seconds*/
> +	int i;
> +
> +	for (i = 0; i < 2000; i++) {
> +		if (readl(reg->iop2drv_doorbell)
> +			& ARCMSR_IOP2DRV_MESSAGE_CMD_DONE) {
> +			writel(ARCMSR_MESSAGE_INT_CLEAR_PATTERN,
> +					reg->iop2drv_doorbell);
> +			writel(ARCMSR_DRV2IOP_END_OF_INTERRUPT,
> +					reg->drv2iop_doorbell);
> +			return true;
> +		}
> +		msleep(10);
> +	}/* max 20 seconds */
>  
> -	} while (Retries++ < 20);/*max 20 sec*/
>  	return false;
>  }
>  
>  static uint8_t arcmsr_hbc_wait_msgint_ready(struct AdapterControlBlock *pACB)
>  {
>  	struct MessageUnit_C *phbcmu = (struct MessageUnit_C *)pACB->pmuC;
> -	unsigned char Retries = 0x00;
> -	uint32_t Index;
> -	do {
> -		for (Index = 0; Index < 100; Index++) {
> -			if (readl(&phbcmu->outbound_doorbell) & ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE) {
> -				writel(ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE_DOORBELL_CLEAR, &phbcmu->outbound_doorbell_clear);/*clear interrupt*/
> -				return true;
> -			}
> -			/* one us delay	*/
> -			msleep(10);
> -		} /*max 1 seconds*/
> -	} while (Retries++ < 20); /*max 20 sec*/
> +	int i;
> +
> +	for (i = 0; i < 2000; i++) {
> +		if (readl(&phbcmu->outbound_doorbell)
> +				& ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE) {
> +			writel(ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE_DOORBELL_CLEAR,
> +				&phbcmu->outbound_doorbell_clear); /*clear interrupt*/
> +			return true;
> +		}
> +		msleep(10);
> +	} /* max 20 seconds */
> +
>  	return false;
>  }
> +
>  static void arcmsr_flush_hba_cache(struct AdapterControlBlock *acb)
>  {
>  	struct MessageUnit_A __iomem *reg = acb->pmuA;
> @@ -2602,12 +2584,8 @@ static int arcmsr_iop_confirm(struct AdapterControlBlock *acb)
>  		if (cdb_phyaddr_hi32 != 0) {
>  			struct MessageUnit_C *reg = (struct MessageUnit_C *)acb->pmuC;
>  
> -			if (cdb_phyaddr_hi32 != 0) {
> -				unsigned char Retries = 0x00;
> -				do {
> -					printk(KERN_NOTICE "arcmsr%d: cdb_phyaddr_hi32=0x%x \n", acb->adapter_index, cdb_phyaddr_hi32);
> -				} while (Retries++ < 100);
> -			}
> +			printk(KERN_NOTICE "arcmsr%d: cdb_phyaddr_hi32=0x%x \n",
> +					acb->adapter_index, cdb_phyaddr_hi32);
>  			writel(ARCMSR_SIGNATURE_SET_CONFIG, &reg->msgcode_rwbuffer[0]);
>  			writel(cdb_phyaddr_hi32, &reg->msgcode_rwbuffer[1]);
>  			writel(ARCMSR_INBOUND_MESG0_SET_CONFIG, &reg->inbound_msgaddr0);
> @@ -2955,10 +2933,10 @@ static int arcmsr_bus_reset(struct scsi_cmnd *cmd)
>  				arcmsr_hardware_reset(acb);
>  				acb->acb_flags &= ~ACB_F_IOP_INITED;
>  sleep_again:
> -				arcmsr_sleep_for_bus_reset(cmd);
> +				ssleep(ARCMSR_SLEEPTIME);
>  				if ((readl(&reg->outbound_msgaddr1) & ARCMSR_OUTBOUND_MESG1_FIRMWARE_OK) == 0) {
>  					printk(KERN_ERR "arcmsr%d: waiting for hw bus reset return, retry=%d \n", acb->host->host_no, retry_count);
> -					if (retry_count > retrycount) {
> +					if (retry_count > ARCMSR_RETRYCOUNT) {
>  						acb->fw_flag = FW_DEADLOCK;
>  						printk(KERN_ERR "arcmsr%d: waiting for hw bus reset return, RETRY TERMINATED!! \n", acb->host->host_no);
>  						return FAILED;
> @@ -3025,10 +3003,10 @@ sleep_again:
>  				arcmsr_hardware_reset(acb);
>  				acb->acb_flags &= ~ACB_F_IOP_INITED;
>  sleep:
> -				arcmsr_sleep_for_bus_reset(cmd);
> +				ssleep(ARCMSR_SLEEPTIME);
>  				if ((readl(&reg->host_diagnostic) & 0x04) != 0) {
>  					printk(KERN_ERR "arcmsr%d: waiting for hw bus reset return, retry=%d \n", acb->host->host_no, retry_count);
> -					if (retry_count > retrycount) {
> +					if (retry_count > ARCMSR_RETRYCOUNT) {
>  						acb->fw_flag = FW_DEADLOCK;
>  						printk(KERN_ERR "arcmsr%d: waiting for hw bus reset return, RETRY TERMINATED!! \n", acb->host->host_no);
>  						return FAILED;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] arcmsr: code cleanup and some corrections
  2011-02-17 15:05     ` Tomas Henzl
@ 2011-02-18  1:04       ` FUJITA Tomonori
  2011-02-18 10:59         ` Tomas Henzl
  0 siblings, 1 reply; 15+ messages in thread
From: FUJITA Tomonori @ 2011-02-18  1:04 UTC (permalink / raw)
  To: thenzl; +Cc: nick.cheng, linux-scsi, James.Bottomley

On Thu, 17 Feb 2011 16:05:37 +0100
Tomas Henzl <thenzl@redhat.com> wrote:

> the discussion is mostly if 'dma_alloc_coherent' 
> ...
> dma_coherent = dma_alloc_coherent(&pdev->dev, acb->uncache_size, &dma_coherent_handle, GFP_KERNEL);
> ...
> returns a page address - dma_coherent with last, I think five bits, zeroed.
> Could you help us with that?

Documentation/DMA-API-HOWTO.txt is not clear enough?

=
dma_alloc_coherent returns two values: the virtual address which you
can use to access it from the CPU and dma_handle which you pass to the
card.

The cpu return address and the DMA bus master address are both
guaranteed to be aligned to the smallest PAGE_SIZE order which
is greater than or equal to the requested size.  This invariant
exists (for example) to guarantee that if you allocate a chunk
which is smaller than or equal to 64 kilobytes, the extent of the
buffer you receive will not cross a 64K boundary.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH 1/2] arcmsr: code cleanup and some corrections
  2011-02-17 15:07 ` [PATCH 1/2] " Tomas Henzl
@ 2011-02-18  1:26   ` NickCheng
  2011-02-18 11:04     ` Tomas Henzl
  2011-02-24  2:02   ` NickCheng
  1 sibling, 1 reply; 15+ messages in thread
From: NickCheng @ 2011-02-18  1:26 UTC (permalink / raw)
  To: 'Tomas Henzl', linux-scsi

Hi Tomas,
I have not yet tested this code.
Do you try that?

-----Original Message-----
From: Tomas Henzl [mailto:thenzl@redhat.com] 
Sent: Thursday, February 17, 2011 11:08 PM
To: 'linux-scsi@vger.kernel.org'
Cc: nick.cheng@areca.com.tw
Subject: Re: [PATCH 1/2] arcmsr: code cleanup and some corrections

Hi Nick,

any comments here?

tomash


> I removed outer loops in ...wait_msgint_ready
> the sleeptime and retrycount are in fact never changed so I changed them
> into defines. In arcmsr_flush_hba_cache is a loop removed, which
> printed the same printk 100 times, one line in log is enough I think.
> The arcmsr_sleep_for_bus_reset has lost a functionality with the latest
patches,
> The only thing the function does is a long sleep, so it's replaced with a
ssleep.
>
> Signed-off-by: Tomas henzl <thenzl@redhat.com>
>
> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c
b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 984bd52..4cd522b 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -75,8 +75,10 @@ MODULE_AUTHOR("Nick Cheng <support@areca.com.tw>");
>  MODULE_DESCRIPTION("ARECA (ARC11xx/12xx/16xx/1880) SATA/SAS RAID Host Bus
Adapter");
>  MODULE_LICENSE("Dual BSD/GPL");
>  MODULE_VERSION(ARCMSR_DRIVER_VERSION);
> -static int sleeptime = 10;
> -static int retrycount = 12;
> +
> +#define	ARCMSR_SLEEPTIME	10
> +#define	ARCMSR_RETRYCOUNT	12
> +
>  wait_queue_head_t wait_q;
>  static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
>  					struct scsi_cmnd *cmd);
> @@ -171,24 +173,6 @@ static struct pci_driver arcmsr_pci_driver = {
>
****************************************************************************
>
****************************************************************************
>  */
> -int arcmsr_sleep_for_bus_reset(struct scsi_cmnd *cmd)
> -{
> -		struct Scsi_Host *shost = NULL;
> -		int i, isleep;
> -		shost = cmd->device->host;
> -		isleep = sleeptime / 10;
> -		if (isleep > 0) {
> -			for (i = 0; i < isleep; i++) {
> -				msleep(10000);
> -			}
> -		}
> -
> -		isleep = sleeptime % 10;
> -		if (isleep > 0) {
> -			msleep(isleep*1000);
> -		}
> -		return 0;
> -}
>  
>  static void arcmsr_free_hbb_mu(struct AdapterControlBlock *acb)
>  {
> @@ -323,66 +307,64 @@ static void arcmsr_define_adapter_type(struct
AdapterControlBlock *acb)
>  
>  	default: acb->adapter_type = ACB_ADAPTER_TYPE_A;
>  	}
> -}	
> +}
>  
>  static uint8_t arcmsr_hba_wait_msgint_ready(struct AdapterControlBlock
*acb)
>  {
>  	struct MessageUnit_A __iomem *reg = acb->pmuA;
> -	uint32_t Index;
> -	uint8_t Retries = 0x00;
> -	do {
> -		for (Index = 0; Index < 100; Index++) {
> -			if (readl(&reg->outbound_intstatus) &
> -					ARCMSR_MU_OUTBOUND_MESSAGE0_INT) {
> -				writel(ARCMSR_MU_OUTBOUND_MESSAGE0_INT,
> -					&reg->outbound_intstatus);
> -				return true;
> -			}
> -			msleep(10);
> -		}/*max 1 seconds*/
> +	int i;
> +
> +	for (i = 0; i < 2000; i++) {
> +		if (readl(&reg->outbound_intstatus) &
> +				ARCMSR_MU_OUTBOUND_MESSAGE0_INT) {
> +			writel(ARCMSR_MU_OUTBOUND_MESSAGE0_INT,
> +				&reg->outbound_intstatus);
> +			return true;
> +		}
> +		msleep(10);
> +	}/* max 20 seconds */
>  
> -	} while (Retries++ < 20);/*max 20 sec*/
>  	return false;
>  }
>  
>  static uint8_t arcmsr_hbb_wait_msgint_ready(struct AdapterControlBlock
*acb)
>  {
>  	struct MessageUnit_B *reg = acb->pmuB;
> -	uint32_t Index;
> -	uint8_t Retries = 0x00;
> -	do {
> -		for (Index = 0; Index < 100; Index++) {
> -			if (readl(reg->iop2drv_doorbell)
> -				& ARCMSR_IOP2DRV_MESSAGE_CMD_DONE) {
> -				writel(ARCMSR_MESSAGE_INT_CLEAR_PATTERN
> -					, reg->iop2drv_doorbell);
> -				writel(ARCMSR_DRV2IOP_END_OF_INTERRUPT,
reg->drv2iop_doorbell);
> -				return true;
> -			}
> -			msleep(10);
> -		}/*max 1 seconds*/
> +	int i;
> +
> +	for (i = 0; i < 2000; i++) {
> +		if (readl(reg->iop2drv_doorbell)
> +			& ARCMSR_IOP2DRV_MESSAGE_CMD_DONE) {
> +			writel(ARCMSR_MESSAGE_INT_CLEAR_PATTERN,
> +					reg->iop2drv_doorbell);
> +			writel(ARCMSR_DRV2IOP_END_OF_INTERRUPT,
> +					reg->drv2iop_doorbell);
> +			return true;
> +		}
> +		msleep(10);
> +	}/* max 20 seconds */
>  
> -	} while (Retries++ < 20);/*max 20 sec*/
>  	return false;
>  }
>  
>  static uint8_t arcmsr_hbc_wait_msgint_ready(struct AdapterControlBlock
*pACB)
>  {
>  	struct MessageUnit_C *phbcmu = (struct MessageUnit_C *)pACB->pmuC;
> -	unsigned char Retries = 0x00;
> -	uint32_t Index;
> -	do {
> -		for (Index = 0; Index < 100; Index++) {
> -			if (readl(&phbcmu->outbound_doorbell) &
ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE) {
> -
writel(ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE_DOORBELL_CLEAR,
&phbcmu->outbound_doorbell_clear);/*clear interrupt*/
> -				return true;
> -			}
> -			/* one us delay	*/
> -			msleep(10);
> -		} /*max 1 seconds*/
> -	} while (Retries++ < 20); /*max 20 sec*/
> +	int i;
> +
> +	for (i = 0; i < 2000; i++) {
> +		if (readl(&phbcmu->outbound_doorbell)
> +				& ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE) {
> +
writel(ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE_DOORBELL_CLEAR,
> +				&phbcmu->outbound_doorbell_clear); /*clear
interrupt*/
> +			return true;
> +		}
> +		msleep(10);
> +	} /* max 20 seconds */
> +
>  	return false;
>  }
> +
>  static void arcmsr_flush_hba_cache(struct AdapterControlBlock *acb)
>  {
>  	struct MessageUnit_A __iomem *reg = acb->pmuA;
> @@ -2602,12 +2584,8 @@ static int arcmsr_iop_confirm(struct
AdapterControlBlock *acb)
>  		if (cdb_phyaddr_hi32 != 0) {
>  			struct MessageUnit_C *reg = (struct MessageUnit_C
*)acb->pmuC;
>  
> -			if (cdb_phyaddr_hi32 != 0) {
> -				unsigned char Retries = 0x00;
> -				do {
> -					printk(KERN_NOTICE "arcmsr%d:
cdb_phyaddr_hi32=0x%x \n", acb->adapter_index, cdb_phyaddr_hi32);
> -				} while (Retries++ < 100);
> -			}
> +			printk(KERN_NOTICE "arcmsr%d: cdb_phyaddr_hi32=0x%x
\n",
> +					acb->adapter_index,
cdb_phyaddr_hi32);
>  			writel(ARCMSR_SIGNATURE_SET_CONFIG,
&reg->msgcode_rwbuffer[0]);
>  			writel(cdb_phyaddr_hi32, &reg->msgcode_rwbuffer[1]);
>  			writel(ARCMSR_INBOUND_MESG0_SET_CONFIG,
&reg->inbound_msgaddr0);
> @@ -2955,10 +2933,10 @@ static int arcmsr_bus_reset(struct scsi_cmnd *cmd)
>  				arcmsr_hardware_reset(acb);
>  				acb->acb_flags &= ~ACB_F_IOP_INITED;
>  sleep_again:
> -				arcmsr_sleep_for_bus_reset(cmd);
> +				ssleep(ARCMSR_SLEEPTIME);
>  				if ((readl(&reg->outbound_msgaddr1) &
ARCMSR_OUTBOUND_MESG1_FIRMWARE_OK) == 0) {
>  					printk(KERN_ERR "arcmsr%d: waiting
for hw bus reset return, retry=%d \n", acb->host->host_no, retry_count);
> -					if (retry_count > retrycount) {
> +					if (retry_count > ARCMSR_RETRYCOUNT)
{
>  						acb->fw_flag = FW_DEADLOCK;
>  						printk(KERN_ERR "arcmsr%d:
waiting for hw bus reset return, RETRY TERMINATED!! \n",
acb->host->host_no);
>  						return FAILED;
> @@ -3025,10 +3003,10 @@ sleep_again:
>  				arcmsr_hardware_reset(acb);
>  				acb->acb_flags &= ~ACB_F_IOP_INITED;
>  sleep:
> -				arcmsr_sleep_for_bus_reset(cmd);
> +				ssleep(ARCMSR_SLEEPTIME);
>  				if ((readl(&reg->host_diagnostic) & 0x04) !=
0) {
>  					printk(KERN_ERR "arcmsr%d: waiting
for hw bus reset return, retry=%d \n", acb->host->host_no, retry_count);
> -					if (retry_count > retrycount) {
> +					if (retry_count > ARCMSR_RETRYCOUNT)
{
>  						acb->fw_flag = FW_DEADLOCK;
>  						printk(KERN_ERR "arcmsr%d:
waiting for hw bus reset return, RETRY TERMINATED!! \n",
acb->host->host_no);
>  						return FAILED;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] arcmsr: code cleanup and some corrections
  2011-02-18  1:04       ` FUJITA Tomonori
@ 2011-02-18 10:59         ` Tomas Henzl
  2011-02-24 14:14           ` Tomas Henzl
  0 siblings, 1 reply; 15+ messages in thread
From: Tomas Henzl @ 2011-02-18 10:59 UTC (permalink / raw)
  To: FUJITA Tomonori; +Cc: nick.cheng, linux-scsi, James.Bottomley

On 02/18/2011 02:04 AM, FUJITA Tomonori wrote:
> On Thu, 17 Feb 2011 16:05:37 +0100
> Tomas Henzl <thenzl@redhat.com> wrote:
>
>   
>> the discussion is mostly if 'dma_alloc_coherent' 
>> ...
>> dma_coherent = dma_alloc_coherent(&pdev->dev, acb->uncache_size, &dma_coherent_handle, GFP_KERNEL);
>> ...
>> returns a page address - dma_coherent with last, I think five bits, zeroed.
>> Could you help us with that?
>>     
> Documentation/DMA-API-HOWTO.txt is not clear enough?
>
> =
> dma_alloc_coherent returns two values: the virtual address which you
> can use to access it from the CPU and dma_handle which you pass to the
> card.
>
> The cpu return address and the DMA bus master address are both
> guaranteed to be aligned to the smallest PAGE_SIZE order which
> is greater than or equal to the requested size.  This invariant
> exists (for example) to guarantee that if you allocate a chunk
> which is smaller than or equal to 64 kilobytes, the extent of the
> buffer you receive will not cross a 64K boundary.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   
It's clear enough I think, I asked this question because Nick doesn't
have a "100% confidence in that.". So I wanted a help to convince him.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] arcmsr: code cleanup and some corrections
  2011-02-18  1:26   ` NickCheng
@ 2011-02-18 11:04     ` Tomas Henzl
  2011-02-18 11:36       ` NickCheng
  0 siblings, 1 reply; 15+ messages in thread
From: Tomas Henzl @ 2011-02-18 11:04 UTC (permalink / raw)
  To: NickCheng; +Cc: linux-scsi

On 02/18/2011 02:26 AM, NickCheng wrote:
> Hi Tomas,
> I have not yet tested this code.
> Do you try that?
>   
Yes, I tried it, but only a very basic test.
Most of the code paths in the patch weren't tested.

If you want I can post a set of individual changes, maybe this would
be better readable?

> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com] 
> Sent: Thursday, February 17, 2011 11:08 PM
> To: 'linux-scsi@vger.kernel.org'
> Cc: nick.cheng@areca.com.tw
> Subject: Re: [PATCH 1/2] arcmsr: code cleanup and some corrections
>
> Hi Nick,
>
> any comments here?
>
> tomash
>
>
>   
>> I removed outer loops in ...wait_msgint_ready
>> the sleeptime and retrycount are in fact never changed so I changed them
>> into defines. In arcmsr_flush_hba_cache is a loop removed, which
>> printed the same printk 100 times, one line in log is enough I think.
>> The arcmsr_sleep_for_bus_reset has lost a functionality with the latest
>>     
> patches,
>   
>> The only thing the function does is a long sleep, so it's replaced with a
>>     
> ssleep.
>   
>> Signed-off-by: Tomas henzl <thenzl@redhat.com>
>>
>> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c
>>     
> b/drivers/scsi/arcmsr/arcmsr_hba.c
>   
>> index 984bd52..4cd522b 100644
>> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
>> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
>> @@ -75,8 +75,10 @@ MODULE_AUTHOR("Nick Cheng <support@areca.com.tw>");
>>  MODULE_DESCRIPTION("ARECA (ARC11xx/12xx/16xx/1880) SATA/SAS RAID Host Bus
>>     
> Adapter");
>   
>>  MODULE_LICENSE("Dual BSD/GPL");
>>  MODULE_VERSION(ARCMSR_DRIVER_VERSION);
>> -static int sleeptime = 10;
>> -static int retrycount = 12;
>> +
>> +#define	ARCMSR_SLEEPTIME	10
>> +#define	ARCMSR_RETRYCOUNT	12
>> +
>>  wait_queue_head_t wait_q;
>>  static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
>>  					struct scsi_cmnd *cmd);
>> @@ -171,24 +173,6 @@ static struct pci_driver arcmsr_pci_driver = {
>>
>>     
> ****************************************************************************
>   
>>     
> ****************************************************************************
>   
>>  */
>> -int arcmsr_sleep_for_bus_reset(struct scsi_cmnd *cmd)
>> -{
>> -		struct Scsi_Host *shost = NULL;
>> -		int i, isleep;
>> -		shost = cmd->device->host;
>> -		isleep = sleeptime / 10;
>> -		if (isleep > 0) {
>> -			for (i = 0; i < isleep; i++) {
>> -				msleep(10000);
>> -			}
>> -		}
>> -
>> -		isleep = sleeptime % 10;
>> -		if (isleep > 0) {
>> -			msleep(isleep*1000);
>> -		}
>> -		return 0;
>> -}
>>  
>>  static void arcmsr_free_hbb_mu(struct AdapterControlBlock *acb)
>>  {
>> @@ -323,66 +307,64 @@ static void arcmsr_define_adapter_type(struct
>>     
> AdapterControlBlock *acb)
>   
>>  
>>  	default: acb->adapter_type = ACB_ADAPTER_TYPE_A;
>>  	}
>> -}	
>> +}
>>  
>>  static uint8_t arcmsr_hba_wait_msgint_ready(struct AdapterControlBlock
>>     
> *acb)
>   
>>  {
>>  	struct MessageUnit_A __iomem *reg = acb->pmuA;
>> -	uint32_t Index;
>> -	uint8_t Retries = 0x00;
>> -	do {
>> -		for (Index = 0; Index < 100; Index++) {
>> -			if (readl(&reg->outbound_intstatus) &
>> -					ARCMSR_MU_OUTBOUND_MESSAGE0_INT) {
>> -				writel(ARCMSR_MU_OUTBOUND_MESSAGE0_INT,
>> -					&reg->outbound_intstatus);
>> -				return true;
>> -			}
>> -			msleep(10);
>> -		}/*max 1 seconds*/
>> +	int i;
>> +
>> +	for (i = 0; i < 2000; i++) {
>> +		if (readl(&reg->outbound_intstatus) &
>> +				ARCMSR_MU_OUTBOUND_MESSAGE0_INT) {
>> +			writel(ARCMSR_MU_OUTBOUND_MESSAGE0_INT,
>> +				&reg->outbound_intstatus);
>> +			return true;
>> +		}
>> +		msleep(10);
>> +	}/* max 20 seconds */
>>  
>> -	} while (Retries++ < 20);/*max 20 sec*/
>>  	return false;
>>  }
>>  
>>  static uint8_t arcmsr_hbb_wait_msgint_ready(struct AdapterControlBlock
>>     
> *acb)
>   
>>  {
>>  	struct MessageUnit_B *reg = acb->pmuB;
>> -	uint32_t Index;
>> -	uint8_t Retries = 0x00;
>> -	do {
>> -		for (Index = 0; Index < 100; Index++) {
>> -			if (readl(reg->iop2drv_doorbell)
>> -				& ARCMSR_IOP2DRV_MESSAGE_CMD_DONE) {
>> -				writel(ARCMSR_MESSAGE_INT_CLEAR_PATTERN
>> -					, reg->iop2drv_doorbell);
>> -				writel(ARCMSR_DRV2IOP_END_OF_INTERRUPT,
>>     
> reg->drv2iop_doorbell);
>   
>> -				return true;
>> -			}
>> -			msleep(10);
>> -		}/*max 1 seconds*/
>> +	int i;
>> +
>> +	for (i = 0; i < 2000; i++) {
>> +		if (readl(reg->iop2drv_doorbell)
>> +			& ARCMSR_IOP2DRV_MESSAGE_CMD_DONE) {
>> +			writel(ARCMSR_MESSAGE_INT_CLEAR_PATTERN,
>> +					reg->iop2drv_doorbell);
>> +			writel(ARCMSR_DRV2IOP_END_OF_INTERRUPT,
>> +					reg->drv2iop_doorbell);
>> +			return true;
>> +		}
>> +		msleep(10);
>> +	}/* max 20 seconds */
>>  
>> -	} while (Retries++ < 20);/*max 20 sec*/
>>  	return false;
>>  }
>>  
>>  static uint8_t arcmsr_hbc_wait_msgint_ready(struct AdapterControlBlock
>>     
> *pACB)
>   
>>  {
>>  	struct MessageUnit_C *phbcmu = (struct MessageUnit_C *)pACB->pmuC;
>> -	unsigned char Retries = 0x00;
>> -	uint32_t Index;
>> -	do {
>> -		for (Index = 0; Index < 100; Index++) {
>> -			if (readl(&phbcmu->outbound_doorbell) &
>>     
> ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE) {
>   
>> -
>>     
> writel(ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE_DOORBELL_CLEAR,
> &phbcmu->outbound_doorbell_clear);/*clear interrupt*/
>   
>> -				return true;
>> -			}
>> -			/* one us delay	*/
>> -			msleep(10);
>> -		} /*max 1 seconds*/
>> -	} while (Retries++ < 20); /*max 20 sec*/
>> +	int i;
>> +
>> +	for (i = 0; i < 2000; i++) {
>> +		if (readl(&phbcmu->outbound_doorbell)
>> +				& ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE) {
>> +
>>     
> writel(ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE_DOORBELL_CLEAR,
>   
>> +				&phbcmu->outbound_doorbell_clear); /*clear
>>     
> interrupt*/
>   
>> +			return true;
>> +		}
>> +		msleep(10);
>> +	} /* max 20 seconds */
>> +
>>  	return false;
>>  }
>> +
>>  static void arcmsr_flush_hba_cache(struct AdapterControlBlock *acb)
>>  {
>>  	struct MessageUnit_A __iomem *reg = acb->pmuA;
>> @@ -2602,12 +2584,8 @@ static int arcmsr_iop_confirm(struct
>>     
> AdapterControlBlock *acb)
>   
>>  		if (cdb_phyaddr_hi32 != 0) {
>>  			struct MessageUnit_C *reg = (struct MessageUnit_C
>>     
> *)acb->pmuC;
>   
>>  
>> -			if (cdb_phyaddr_hi32 != 0) {
>> -				unsigned char Retries = 0x00;
>> -				do {
>> -					printk(KERN_NOTICE "arcmsr%d:
>>     
> cdb_phyaddr_hi32=0x%x \n", acb->adapter_index, cdb_phyaddr_hi32);
>   
>> -				} while (Retries++ < 100);
>> -			}
>> +			printk(KERN_NOTICE "arcmsr%d: cdb_phyaddr_hi32=0x%x
>>     
> \n",
>   
>> +					acb->adapter_index,
>>     
> cdb_phyaddr_hi32);
>   
>>  			writel(ARCMSR_SIGNATURE_SET_CONFIG,
>>     
> &reg->msgcode_rwbuffer[0]);
>   
>>  			writel(cdb_phyaddr_hi32, &reg->msgcode_rwbuffer[1]);
>>  			writel(ARCMSR_INBOUND_MESG0_SET_CONFIG,
>>     
> &reg->inbound_msgaddr0);
>   
>> @@ -2955,10 +2933,10 @@ static int arcmsr_bus_reset(struct scsi_cmnd *cmd)
>>  				arcmsr_hardware_reset(acb);
>>  				acb->acb_flags &= ~ACB_F_IOP_INITED;
>>  sleep_again:
>> -				arcmsr_sleep_for_bus_reset(cmd);
>> +				ssleep(ARCMSR_SLEEPTIME);
>>  				if ((readl(&reg->outbound_msgaddr1) &
>>     
> ARCMSR_OUTBOUND_MESG1_FIRMWARE_OK) == 0) {
>   
>>  					printk(KERN_ERR "arcmsr%d: waiting
>>     
> for hw bus reset return, retry=%d \n", acb->host->host_no, retry_count);
>   
>> -					if (retry_count > retrycount) {
>> +					if (retry_count > ARCMSR_RETRYCOUNT)
>>     
> {
>   
>>  						acb->fw_flag = FW_DEADLOCK;
>>  						printk(KERN_ERR "arcmsr%d:
>>     
> waiting for hw bus reset return, RETRY TERMINATED!! \n",
> acb->host->host_no);
>   
>>  						return FAILED;
>> @@ -3025,10 +3003,10 @@ sleep_again:
>>  				arcmsr_hardware_reset(acb);
>>  				acb->acb_flags &= ~ACB_F_IOP_INITED;
>>  sleep:
>> -				arcmsr_sleep_for_bus_reset(cmd);
>> +				ssleep(ARCMSR_SLEEPTIME);
>>  				if ((readl(&reg->host_diagnostic) & 0x04) !=
>>     
> 0) {
>   
>>  					printk(KERN_ERR "arcmsr%d: waiting
>>     
> for hw bus reset return, retry=%d \n", acb->host->host_no, retry_count);
>   
>> -					if (retry_count > retrycount) {
>> +					if (retry_count > ARCMSR_RETRYCOUNT)
>>     
> {
>   
>>  						acb->fw_flag = FW_DEADLOCK;
>>  						printk(KERN_ERR "arcmsr%d:
>>     
> waiting for hw bus reset return, RETRY TERMINATED!! \n",
> acb->host->host_no);
>   
>>  						return FAILED;
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>   
>>     
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   


^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH 1/2] arcmsr: code cleanup and some corrections
  2011-02-18 11:04     ` Tomas Henzl
@ 2011-02-18 11:36       ` NickCheng
  0 siblings, 0 replies; 15+ messages in thread
From: NickCheng @ 2011-02-18 11:36 UTC (permalink / raw)
  To: 'Tomas Henzl'; +Cc: linux-scsi

Hi Tomas,
I will make time to test it.
Please give me few days.

-----Original Message-----
From: Tomas Henzl [mailto:thenzl@redhat.com] 
Sent: Friday, February 18, 2011 7:04 PM
To: NickCheng
Cc: linux-scsi@vger.kernel.org
Subject: Re: [PATCH 1/2] arcmsr: code cleanup and some corrections

On 02/18/2011 02:26 AM, NickCheng wrote:
> Hi Tomas,
> I have not yet tested this code.
> Do you try that?
>   
Yes, I tried it, but only a very basic test.
Most of the code paths in the patch weren't tested.

If you want I can post a set of individual changes, maybe this would
be better readable?

> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com] 
> Sent: Thursday, February 17, 2011 11:08 PM
> To: 'linux-scsi@vger.kernel.org'
> Cc: nick.cheng@areca.com.tw
> Subject: Re: [PATCH 1/2] arcmsr: code cleanup and some corrections
>
> Hi Nick,
>
> any comments here?
>
> tomash
>
>
>   
>> I removed outer loops in ...wait_msgint_ready
>> the sleeptime and retrycount are in fact never changed so I changed them
>> into defines. In arcmsr_flush_hba_cache is a loop removed, which
>> printed the same printk 100 times, one line in log is enough I think.
>> The arcmsr_sleep_for_bus_reset has lost a functionality with the latest
>>     
> patches,
>   
>> The only thing the function does is a long sleep, so it's replaced with a
>>     
> ssleep.
>   
>> Signed-off-by: Tomas henzl <thenzl@redhat.com>
>>
>> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c
>>     
> b/drivers/scsi/arcmsr/arcmsr_hba.c
>   
>> index 984bd52..4cd522b 100644
>> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
>> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
>> @@ -75,8 +75,10 @@ MODULE_AUTHOR("Nick Cheng <support@areca.com.tw>");
>>  MODULE_DESCRIPTION("ARECA (ARC11xx/12xx/16xx/1880) SATA/SAS RAID Host
Bus
>>     
> Adapter");
>   
>>  MODULE_LICENSE("Dual BSD/GPL");
>>  MODULE_VERSION(ARCMSR_DRIVER_VERSION);
>> -static int sleeptime = 10;
>> -static int retrycount = 12;
>> +
>> +#define	ARCMSR_SLEEPTIME	10
>> +#define	ARCMSR_RETRYCOUNT	12
>> +
>>  wait_queue_head_t wait_q;
>>  static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
>>  					struct scsi_cmnd *cmd);
>> @@ -171,24 +173,6 @@ static struct pci_driver arcmsr_pci_driver = {
>>
>>     
>
****************************************************************************
>   
>>     
>
****************************************************************************
>   
>>  */
>> -int arcmsr_sleep_for_bus_reset(struct scsi_cmnd *cmd)
>> -{
>> -		struct Scsi_Host *shost = NULL;
>> -		int i, isleep;
>> -		shost = cmd->device->host;
>> -		isleep = sleeptime / 10;
>> -		if (isleep > 0) {
>> -			for (i = 0; i < isleep; i++) {
>> -				msleep(10000);
>> -			}
>> -		}
>> -
>> -		isleep = sleeptime % 10;
>> -		if (isleep > 0) {
>> -			msleep(isleep*1000);
>> -		}
>> -		return 0;
>> -}
>>  
>>  static void arcmsr_free_hbb_mu(struct AdapterControlBlock *acb)
>>  {
>> @@ -323,66 +307,64 @@ static void arcmsr_define_adapter_type(struct
>>     
> AdapterControlBlock *acb)
>   
>>  
>>  	default: acb->adapter_type = ACB_ADAPTER_TYPE_A;
>>  	}
>> -}	
>> +}
>>  
>>  static uint8_t arcmsr_hba_wait_msgint_ready(struct AdapterControlBlock
>>     
> *acb)
>   
>>  {
>>  	struct MessageUnit_A __iomem *reg = acb->pmuA;
>> -	uint32_t Index;
>> -	uint8_t Retries = 0x00;
>> -	do {
>> -		for (Index = 0; Index < 100; Index++) {
>> -			if (readl(&reg->outbound_intstatus) &
>> -					ARCMSR_MU_OUTBOUND_MESSAGE0_INT) {
>> -				writel(ARCMSR_MU_OUTBOUND_MESSAGE0_INT,
>> -					&reg->outbound_intstatus);
>> -				return true;
>> -			}
>> -			msleep(10);
>> -		}/*max 1 seconds*/
>> +	int i;
>> +
>> +	for (i = 0; i < 2000; i++) {
>> +		if (readl(&reg->outbound_intstatus) &
>> +				ARCMSR_MU_OUTBOUND_MESSAGE0_INT) {
>> +			writel(ARCMSR_MU_OUTBOUND_MESSAGE0_INT,
>> +				&reg->outbound_intstatus);
>> +			return true;
>> +		}
>> +		msleep(10);
>> +	}/* max 20 seconds */
>>  
>> -	} while (Retries++ < 20);/*max 20 sec*/
>>  	return false;
>>  }
>>  
>>  static uint8_t arcmsr_hbb_wait_msgint_ready(struct AdapterControlBlock
>>     
> *acb)
>   
>>  {
>>  	struct MessageUnit_B *reg = acb->pmuB;
>> -	uint32_t Index;
>> -	uint8_t Retries = 0x00;
>> -	do {
>> -		for (Index = 0; Index < 100; Index++) {
>> -			if (readl(reg->iop2drv_doorbell)
>> -				& ARCMSR_IOP2DRV_MESSAGE_CMD_DONE) {
>> -				writel(ARCMSR_MESSAGE_INT_CLEAR_PATTERN
>> -					, reg->iop2drv_doorbell);
>> -				writel(ARCMSR_DRV2IOP_END_OF_INTERRUPT,
>>     
> reg->drv2iop_doorbell);
>   
>> -				return true;
>> -			}
>> -			msleep(10);
>> -		}/*max 1 seconds*/
>> +	int i;
>> +
>> +	for (i = 0; i < 2000; i++) {
>> +		if (readl(reg->iop2drv_doorbell)
>> +			& ARCMSR_IOP2DRV_MESSAGE_CMD_DONE) {
>> +			writel(ARCMSR_MESSAGE_INT_CLEAR_PATTERN,
>> +					reg->iop2drv_doorbell);
>> +			writel(ARCMSR_DRV2IOP_END_OF_INTERRUPT,
>> +					reg->drv2iop_doorbell);
>> +			return true;
>> +		}
>> +		msleep(10);
>> +	}/* max 20 seconds */
>>  
>> -	} while (Retries++ < 20);/*max 20 sec*/
>>  	return false;
>>  }
>>  
>>  static uint8_t arcmsr_hbc_wait_msgint_ready(struct AdapterControlBlock
>>     
> *pACB)
>   
>>  {
>>  	struct MessageUnit_C *phbcmu = (struct MessageUnit_C *)pACB->pmuC;
>> -	unsigned char Retries = 0x00;
>> -	uint32_t Index;
>> -	do {
>> -		for (Index = 0; Index < 100; Index++) {
>> -			if (readl(&phbcmu->outbound_doorbell) &
>>     
> ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE) {
>   
>> -
>>     
> writel(ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE_DOORBELL_CLEAR,
> &phbcmu->outbound_doorbell_clear);/*clear interrupt*/
>   
>> -				return true;
>> -			}
>> -			/* one us delay	*/
>> -			msleep(10);
>> -		} /*max 1 seconds*/
>> -	} while (Retries++ < 20); /*max 20 sec*/
>> +	int i;
>> +
>> +	for (i = 0; i < 2000; i++) {
>> +		if (readl(&phbcmu->outbound_doorbell)
>> +				& ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE) {
>> +
>>     
> writel(ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE_DOORBELL_CLEAR,
>   
>> +				&phbcmu->outbound_doorbell_clear); /*clear
>>     
> interrupt*/
>   
>> +			return true;
>> +		}
>> +		msleep(10);
>> +	} /* max 20 seconds */
>> +
>>  	return false;
>>  }
>> +
>>  static void arcmsr_flush_hba_cache(struct AdapterControlBlock *acb)
>>  {
>>  	struct MessageUnit_A __iomem *reg = acb->pmuA;
>> @@ -2602,12 +2584,8 @@ static int arcmsr_iop_confirm(struct
>>     
> AdapterControlBlock *acb)
>   
>>  		if (cdb_phyaddr_hi32 != 0) {
>>  			struct MessageUnit_C *reg = (struct MessageUnit_C
>>     
> *)acb->pmuC;
>   
>>  
>> -			if (cdb_phyaddr_hi32 != 0) {
>> -				unsigned char Retries = 0x00;
>> -				do {
>> -					printk(KERN_NOTICE "arcmsr%d:
>>     
> cdb_phyaddr_hi32=0x%x \n", acb->adapter_index, cdb_phyaddr_hi32);
>   
>> -				} while (Retries++ < 100);
>> -			}
>> +			printk(KERN_NOTICE "arcmsr%d: cdb_phyaddr_hi32=0x%x
>>     
> \n",
>   
>> +					acb->adapter_index,
>>     
> cdb_phyaddr_hi32);
>   
>>  			writel(ARCMSR_SIGNATURE_SET_CONFIG,
>>     
> &reg->msgcode_rwbuffer[0]);
>   
>>  			writel(cdb_phyaddr_hi32, &reg->msgcode_rwbuffer[1]);
>>  			writel(ARCMSR_INBOUND_MESG0_SET_CONFIG,
>>     
> &reg->inbound_msgaddr0);
>   
>> @@ -2955,10 +2933,10 @@ static int arcmsr_bus_reset(struct scsi_cmnd
*cmd)
>>  				arcmsr_hardware_reset(acb);
>>  				acb->acb_flags &= ~ACB_F_IOP_INITED;
>>  sleep_again:
>> -				arcmsr_sleep_for_bus_reset(cmd);
>> +				ssleep(ARCMSR_SLEEPTIME);
>>  				if ((readl(&reg->outbound_msgaddr1) &
>>     
> ARCMSR_OUTBOUND_MESG1_FIRMWARE_OK) == 0) {
>   
>>  					printk(KERN_ERR "arcmsr%d: waiting
>>     
> for hw bus reset return, retry=%d \n", acb->host->host_no, retry_count);
>   
>> -					if (retry_count > retrycount) {
>> +					if (retry_count > ARCMSR_RETRYCOUNT)
>>     
> {
>   
>>  						acb->fw_flag = FW_DEADLOCK;
>>  						printk(KERN_ERR "arcmsr%d:
>>     
> waiting for hw bus reset return, RETRY TERMINATED!! \n",
> acb->host->host_no);
>   
>>  						return FAILED;
>> @@ -3025,10 +3003,10 @@ sleep_again:
>>  				arcmsr_hardware_reset(acb);
>>  				acb->acb_flags &= ~ACB_F_IOP_INITED;
>>  sleep:
>> -				arcmsr_sleep_for_bus_reset(cmd);
>> +				ssleep(ARCMSR_SLEEPTIME);
>>  				if ((readl(&reg->host_diagnostic) & 0x04) !=
>>     
> 0) {
>   
>>  					printk(KERN_ERR "arcmsr%d: waiting
>>     
> for hw bus reset return, retry=%d \n", acb->host->host_no, retry_count);
>   
>> -					if (retry_count > retrycount) {
>> +					if (retry_count > ARCMSR_RETRYCOUNT)
>>     
> {
>   
>>  						acb->fw_flag = FW_DEADLOCK;
>>  						printk(KERN_ERR "arcmsr%d:
>>     
> waiting for hw bus reset return, RETRY TERMINATED!! \n",
> acb->host->host_no);
>   
>>  						return FAILED;
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>   
>>     
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   


^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH 1/2] arcmsr: code cleanup and some corrections
  2011-02-17 15:07 ` [PATCH 1/2] " Tomas Henzl
  2011-02-18  1:26   ` NickCheng
@ 2011-02-24  2:02   ` NickCheng
  2011-02-24 14:09     ` Tomas Henzl
  1 sibling, 1 reply; 15+ messages in thread
From: NickCheng @ 2011-02-24  2:02 UTC (permalink / raw)
  To: 'Tomas Henzl'; +Cc: linux-scsi

Hi Tomas,
The test on my site is so far so good.

-----Original Message-----
From: NickCheng [mailto:nick.cheng@areca.com.tw] 
Sent: Friday, February 18, 2011 9:26 AM
To: 'Tomas Henzl'; 'linux-scsi@vger.kernel.org'
Subject: RE: [PATCH 1/2] arcmsr: code cleanup and some corrections

Hi Tomas,
I have not yet tested this code.
Do you try that?

-----Original Message-----
From: Tomas Henzl [mailto:thenzl@redhat.com] 
Sent: Thursday, February 17, 2011 11:08 PM
To: 'linux-scsi@vger.kernel.org'
Cc: nick.cheng@areca.com.tw
Subject: Re: [PATCH 1/2] arcmsr: code cleanup and some corrections

Hi Nick,

any comments here?

tomash


> I removed outer loops in ...wait_msgint_ready
> the sleeptime and retrycount are in fact never changed so I changed them
> into defines. In arcmsr_flush_hba_cache is a loop removed, which
> printed the same printk 100 times, one line in log is enough I think.
> The arcmsr_sleep_for_bus_reset has lost a functionality with the latest
patches,
> The only thing the function does is a long sleep, so it's replaced with a
ssleep.
>
> Signed-off-by: Tomas henzl <thenzl@redhat.com>
>
> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c
b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 984bd52..4cd522b 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -75,8 +75,10 @@ MODULE_AUTHOR("Nick Cheng <support@areca.com.tw>");
>  MODULE_DESCRIPTION("ARECA (ARC11xx/12xx/16xx/1880) SATA/SAS RAID Host Bus
Adapter");
>  MODULE_LICENSE("Dual BSD/GPL");
>  MODULE_VERSION(ARCMSR_DRIVER_VERSION);
> -static int sleeptime = 10;
> -static int retrycount = 12;
> +
> +#define	ARCMSR_SLEEPTIME	10
> +#define	ARCMSR_RETRYCOUNT	12
> +
>  wait_queue_head_t wait_q;
>  static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
>  					struct scsi_cmnd *cmd);
> @@ -171,24 +173,6 @@ static struct pci_driver arcmsr_pci_driver = {
>
****************************************************************************
>
****************************************************************************
>  */
> -int arcmsr_sleep_for_bus_reset(struct scsi_cmnd *cmd)
> -{
> -		struct Scsi_Host *shost = NULL;
> -		int i, isleep;
> -		shost = cmd->device->host;
> -		isleep = sleeptime / 10;
> -		if (isleep > 0) {
> -			for (i = 0; i < isleep; i++) {
> -				msleep(10000);
> -			}
> -		}
> -
> -		isleep = sleeptime % 10;
> -		if (isleep > 0) {
> -			msleep(isleep*1000);
> -		}
> -		return 0;
> -}
>  
>  static void arcmsr_free_hbb_mu(struct AdapterControlBlock *acb)
>  {
> @@ -323,66 +307,64 @@ static void arcmsr_define_adapter_type(struct
AdapterControlBlock *acb)
>  
>  	default: acb->adapter_type = ACB_ADAPTER_TYPE_A;
>  	}
> -}	
> +}
>  
>  static uint8_t arcmsr_hba_wait_msgint_ready(struct AdapterControlBlock
*acb)
>  {
>  	struct MessageUnit_A __iomem *reg = acb->pmuA;
> -	uint32_t Index;
> -	uint8_t Retries = 0x00;
> -	do {
> -		for (Index = 0; Index < 100; Index++) {
> -			if (readl(&reg->outbound_intstatus) &
> -					ARCMSR_MU_OUTBOUND_MESSAGE0_INT) {
> -				writel(ARCMSR_MU_OUTBOUND_MESSAGE0_INT,
> -					&reg->outbound_intstatus);
> -				return true;
> -			}
> -			msleep(10);
> -		}/*max 1 seconds*/
> +	int i;
> +
> +	for (i = 0; i < 2000; i++) {
> +		if (readl(&reg->outbound_intstatus) &
> +				ARCMSR_MU_OUTBOUND_MESSAGE0_INT) {
> +			writel(ARCMSR_MU_OUTBOUND_MESSAGE0_INT,
> +				&reg->outbound_intstatus);
> +			return true;
> +		}
> +		msleep(10);
> +	}/* max 20 seconds */
>  
> -	} while (Retries++ < 20);/*max 20 sec*/
>  	return false;
>  }
>  
>  static uint8_t arcmsr_hbb_wait_msgint_ready(struct AdapterControlBlock
*acb)
>  {
>  	struct MessageUnit_B *reg = acb->pmuB;
> -	uint32_t Index;
> -	uint8_t Retries = 0x00;
> -	do {
> -		for (Index = 0; Index < 100; Index++) {
> -			if (readl(reg->iop2drv_doorbell)
> -				& ARCMSR_IOP2DRV_MESSAGE_CMD_DONE) {
> -				writel(ARCMSR_MESSAGE_INT_CLEAR_PATTERN
> -					, reg->iop2drv_doorbell);
> -				writel(ARCMSR_DRV2IOP_END_OF_INTERRUPT,
reg->drv2iop_doorbell);
> -				return true;
> -			}
> -			msleep(10);
> -		}/*max 1 seconds*/
> +	int i;
> +
> +	for (i = 0; i < 2000; i++) {
> +		if (readl(reg->iop2drv_doorbell)
> +			& ARCMSR_IOP2DRV_MESSAGE_CMD_DONE) {
> +			writel(ARCMSR_MESSAGE_INT_CLEAR_PATTERN,
> +					reg->iop2drv_doorbell);
> +			writel(ARCMSR_DRV2IOP_END_OF_INTERRUPT,
> +					reg->drv2iop_doorbell);
> +			return true;
> +		}
> +		msleep(10);
> +	}/* max 20 seconds */
>  
> -	} while (Retries++ < 20);/*max 20 sec*/
>  	return false;
>  }
>  
>  static uint8_t arcmsr_hbc_wait_msgint_ready(struct AdapterControlBlock
*pACB)
>  {
>  	struct MessageUnit_C *phbcmu = (struct MessageUnit_C *)pACB->pmuC;
> -	unsigned char Retries = 0x00;
> -	uint32_t Index;
> -	do {
> -		for (Index = 0; Index < 100; Index++) {
> -			if (readl(&phbcmu->outbound_doorbell) &
ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE) {
> -
writel(ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE_DOORBELL_CLEAR,
&phbcmu->outbound_doorbell_clear);/*clear interrupt*/
> -				return true;
> -			}
> -			/* one us delay	*/
> -			msleep(10);
> -		} /*max 1 seconds*/
> -	} while (Retries++ < 20); /*max 20 sec*/
> +	int i;
> +
> +	for (i = 0; i < 2000; i++) {
> +		if (readl(&phbcmu->outbound_doorbell)
> +				& ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE) {
> +
writel(ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE_DOORBELL_CLEAR,
> +				&phbcmu->outbound_doorbell_clear); /*clear
interrupt*/
> +			return true;
> +		}
> +		msleep(10);
> +	} /* max 20 seconds */
> +
>  	return false;
>  }
> +
>  static void arcmsr_flush_hba_cache(struct AdapterControlBlock *acb)
>  {
>  	struct MessageUnit_A __iomem *reg = acb->pmuA;
> @@ -2602,12 +2584,8 @@ static int arcmsr_iop_confirm(struct
AdapterControlBlock *acb)
>  		if (cdb_phyaddr_hi32 != 0) {
>  			struct MessageUnit_C *reg = (struct MessageUnit_C
*)acb->pmuC;
>  
> -			if (cdb_phyaddr_hi32 != 0) {
> -				unsigned char Retries = 0x00;
> -				do {
> -					printk(KERN_NOTICE "arcmsr%d:
cdb_phyaddr_hi32=0x%x \n", acb->adapter_index, cdb_phyaddr_hi32);
> -				} while (Retries++ < 100);
> -			}
> +			printk(KERN_NOTICE "arcmsr%d: cdb_phyaddr_hi32=0x%x
\n",
> +					acb->adapter_index,
cdb_phyaddr_hi32);
>  			writel(ARCMSR_SIGNATURE_SET_CONFIG,
&reg->msgcode_rwbuffer[0]);
>  			writel(cdb_phyaddr_hi32, &reg->msgcode_rwbuffer[1]);
>  			writel(ARCMSR_INBOUND_MESG0_SET_CONFIG,
&reg->inbound_msgaddr0);
> @@ -2955,10 +2933,10 @@ static int arcmsr_bus_reset(struct scsi_cmnd *cmd)
>  				arcmsr_hardware_reset(acb);
>  				acb->acb_flags &= ~ACB_F_IOP_INITED;
>  sleep_again:
> -				arcmsr_sleep_for_bus_reset(cmd);
> +				ssleep(ARCMSR_SLEEPTIME);
>  				if ((readl(&reg->outbound_msgaddr1) &
ARCMSR_OUTBOUND_MESG1_FIRMWARE_OK) == 0) {
>  					printk(KERN_ERR "arcmsr%d: waiting
for hw bus reset return, retry=%d \n", acb->host->host_no, retry_count);
> -					if (retry_count > retrycount) {
> +					if (retry_count > ARCMSR_RETRYCOUNT)
{
>  						acb->fw_flag = FW_DEADLOCK;
>  						printk(KERN_ERR "arcmsr%d:
waiting for hw bus reset return, RETRY TERMINATED!! \n",
acb->host->host_no);
>  						return FAILED;
> @@ -3025,10 +3003,10 @@ sleep_again:
>  				arcmsr_hardware_reset(acb);
>  				acb->acb_flags &= ~ACB_F_IOP_INITED;
>  sleep:
> -				arcmsr_sleep_for_bus_reset(cmd);
> +				ssleep(ARCMSR_SLEEPTIME);
>  				if ((readl(&reg->host_diagnostic) & 0x04) !=
0) {
>  					printk(KERN_ERR "arcmsr%d: waiting
for hw bus reset return, retry=%d \n", acb->host->host_no, retry_count);
> -					if (retry_count > retrycount) {
> +					if (retry_count > ARCMSR_RETRYCOUNT)
{
>  						acb->fw_flag = FW_DEADLOCK;
>  						printk(KERN_ERR "arcmsr%d:
waiting for hw bus reset return, RETRY TERMINATED!! \n",
acb->host->host_no);
>  						return FAILED;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] arcmsr: code cleanup and some corrections
  2011-02-24  2:02   ` NickCheng
@ 2011-02-24 14:09     ` Tomas Henzl
  2011-02-25  1:30       ` NickCheng
  0 siblings, 1 reply; 15+ messages in thread
From: Tomas Henzl @ 2011-02-24 14:09 UTC (permalink / raw)
  To: NickCheng; +Cc: linux-scsi

On 02/24/2011 03:02 AM, NickCheng wrote:
> Hi Tomas,
> The test on my site is so far so good.
>   
Thanks Nick,
if this means you accept this patch please
add an 'Acked-by: ...' line so it is clear what you mean.

> -----Original Message-----
> From: NickCheng [mailto:nick.cheng@areca.com.tw] 
> Sent: Friday, February 18, 2011 9:26 AM
> To: 'Tomas Henzl'; 'linux-scsi@vger.kernel.org'
> Subject: RE: [PATCH 1/2] arcmsr: code cleanup and some corrections
>
> Hi Tomas,
> I have not yet tested this code.
> Do you try that?
>
> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com] 
> Sent: Thursday, February 17, 2011 11:08 PM
> To: 'linux-scsi@vger.kernel.org'
> Cc: nick.cheng@areca.com.tw
> Subject: Re: [PATCH 1/2] arcmsr: code cleanup and some corrections
>
> Hi Nick,
>
> any comments here?
>
> tomash
>
>
>   
>> I removed outer loops in ...wait_msgint_ready
>> the sleeptime and retrycount are in fact never changed so I changed them
>> into defines. In arcmsr_flush_hba_cache is a loop removed, which
>> printed the same printk 100 times, one line in log is enough I think.
>> The arcmsr_sleep_for_bus_reset has lost a functionality with the latest
>>     
> patches,
>   
>> The only thing the function does is a long sleep, so it's replaced with a
>>     
> ssleep.
>   
>> Signed-off-by: Tomas henzl <thenzl@redhat.com>
>>
>> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c
>>     
> b/drivers/scsi/arcmsr/arcmsr_hba.c
>   
>> index 984bd52..4cd522b 100644
>> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
>> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
>> @@ -75,8 +75,10 @@ MODULE_AUTHOR("Nick Cheng <support@areca.com.tw>");
>>  MODULE_DESCRIPTION("ARECA (ARC11xx/12xx/16xx/1880) SATA/SAS RAID Host Bus
>>     
> Adapter");
>   
>>  MODULE_LICENSE("Dual BSD/GPL");
>>  MODULE_VERSION(ARCMSR_DRIVER_VERSION);
>> -static int sleeptime = 10;
>> -static int retrycount = 12;
>> +
>> +#define	ARCMSR_SLEEPTIME	10
>> +#define	ARCMSR_RETRYCOUNT	12
>> +
>>  wait_queue_head_t wait_q;
>>  static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
>>  					struct scsi_cmnd *cmd);
>> @@ -171,24 +173,6 @@ static struct pci_driver arcmsr_pci_driver = {
>>
>>     
> ****************************************************************************
>   
>>     
> ****************************************************************************
>   
>>  */
>> -int arcmsr_sleep_for_bus_reset(struct scsi_cmnd *cmd)
>> -{
>> -		struct Scsi_Host *shost = NULL;
>> -		int i, isleep;
>> -		shost = cmd->device->host;
>> -		isleep = sleeptime / 10;
>> -		if (isleep > 0) {
>> -			for (i = 0; i < isleep; i++) {
>> -				msleep(10000);
>> -			}
>> -		}
>> -
>> -		isleep = sleeptime % 10;
>> -		if (isleep > 0) {
>> -			msleep(isleep*1000);
>> -		}
>> -		return 0;
>> -}
>>  
>>  static void arcmsr_free_hbb_mu(struct AdapterControlBlock *acb)
>>  {
>> @@ -323,66 +307,64 @@ static void arcmsr_define_adapter_type(struct
>>     
> AdapterControlBlock *acb)
>   
>>  
>>  	default: acb->adapter_type = ACB_ADAPTER_TYPE_A;
>>  	}
>> -}	
>> +}
>>  
>>  static uint8_t arcmsr_hba_wait_msgint_ready(struct AdapterControlBlock
>>     
> *acb)
>   
>>  {
>>  	struct MessageUnit_A __iomem *reg = acb->pmuA;
>> -	uint32_t Index;
>> -	uint8_t Retries = 0x00;
>> -	do {
>> -		for (Index = 0; Index < 100; Index++) {
>> -			if (readl(&reg->outbound_intstatus) &
>> -					ARCMSR_MU_OUTBOUND_MESSAGE0_INT) {
>> -				writel(ARCMSR_MU_OUTBOUND_MESSAGE0_INT,
>> -					&reg->outbound_intstatus);
>> -				return true;
>> -			}
>> -			msleep(10);
>> -		}/*max 1 seconds*/
>> +	int i;
>> +
>> +	for (i = 0; i < 2000; i++) {
>> +		if (readl(&reg->outbound_intstatus) &
>> +				ARCMSR_MU_OUTBOUND_MESSAGE0_INT) {
>> +			writel(ARCMSR_MU_OUTBOUND_MESSAGE0_INT,
>> +				&reg->outbound_intstatus);
>> +			return true;
>> +		}
>> +		msleep(10);
>> +	}/* max 20 seconds */
>>  
>> -	} while (Retries++ < 20);/*max 20 sec*/
>>  	return false;
>>  }
>>  
>>  static uint8_t arcmsr_hbb_wait_msgint_ready(struct AdapterControlBlock
>>     
> *acb)
>   
>>  {
>>  	struct MessageUnit_B *reg = acb->pmuB;
>> -	uint32_t Index;
>> -	uint8_t Retries = 0x00;
>> -	do {
>> -		for (Index = 0; Index < 100; Index++) {
>> -			if (readl(reg->iop2drv_doorbell)
>> -				& ARCMSR_IOP2DRV_MESSAGE_CMD_DONE) {
>> -				writel(ARCMSR_MESSAGE_INT_CLEAR_PATTERN
>> -					, reg->iop2drv_doorbell);
>> -				writel(ARCMSR_DRV2IOP_END_OF_INTERRUPT,
>>     
> reg->drv2iop_doorbell);
>   
>> -				return true;
>> -			}
>> -			msleep(10);
>> -		}/*max 1 seconds*/
>> +	int i;
>> +
>> +	for (i = 0; i < 2000; i++) {
>> +		if (readl(reg->iop2drv_doorbell)
>> +			& ARCMSR_IOP2DRV_MESSAGE_CMD_DONE) {
>> +			writel(ARCMSR_MESSAGE_INT_CLEAR_PATTERN,
>> +					reg->iop2drv_doorbell);
>> +			writel(ARCMSR_DRV2IOP_END_OF_INTERRUPT,
>> +					reg->drv2iop_doorbell);
>> +			return true;
>> +		}
>> +		msleep(10);
>> +	}/* max 20 seconds */
>>  
>> -	} while (Retries++ < 20);/*max 20 sec*/
>>  	return false;
>>  }
>>  
>>  static uint8_t arcmsr_hbc_wait_msgint_ready(struct AdapterControlBlock
>>     
> *pACB)
>   
>>  {
>>  	struct MessageUnit_C *phbcmu = (struct MessageUnit_C *)pACB->pmuC;
>> -	unsigned char Retries = 0x00;
>> -	uint32_t Index;
>> -	do {
>> -		for (Index = 0; Index < 100; Index++) {
>> -			if (readl(&phbcmu->outbound_doorbell) &
>>     
> ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE) {
>   
>> -
>>     
> writel(ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE_DOORBELL_CLEAR,
> &phbcmu->outbound_doorbell_clear);/*clear interrupt*/
>   
>> -				return true;
>> -			}
>> -			/* one us delay	*/
>> -			msleep(10);
>> -		} /*max 1 seconds*/
>> -	} while (Retries++ < 20); /*max 20 sec*/
>> +	int i;
>> +
>> +	for (i = 0; i < 2000; i++) {
>> +		if (readl(&phbcmu->outbound_doorbell)
>> +				& ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE) {
>> +
>>     
> writel(ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE_DOORBELL_CLEAR,
>   
>> +				&phbcmu->outbound_doorbell_clear); /*clear
>>     
> interrupt*/
>   
>> +			return true;
>> +		}
>> +		msleep(10);
>> +	} /* max 20 seconds */
>> +
>>  	return false;
>>  }
>> +
>>  static void arcmsr_flush_hba_cache(struct AdapterControlBlock *acb)
>>  {
>>  	struct MessageUnit_A __iomem *reg = acb->pmuA;
>> @@ -2602,12 +2584,8 @@ static int arcmsr_iop_confirm(struct
>>     
> AdapterControlBlock *acb)
>   
>>  		if (cdb_phyaddr_hi32 != 0) {
>>  			struct MessageUnit_C *reg = (struct MessageUnit_C
>>     
> *)acb->pmuC;
>   
>>  
>> -			if (cdb_phyaddr_hi32 != 0) {
>> -				unsigned char Retries = 0x00;
>> -				do {
>> -					printk(KERN_NOTICE "arcmsr%d:
>>     
> cdb_phyaddr_hi32=0x%x \n", acb->adapter_index, cdb_phyaddr_hi32);
>   
>> -				} while (Retries++ < 100);
>> -			}
>> +			printk(KERN_NOTICE "arcmsr%d: cdb_phyaddr_hi32=0x%x
>>     
> \n",
>   
>> +					acb->adapter_index,
>>     
> cdb_phyaddr_hi32);
>   
>>  			writel(ARCMSR_SIGNATURE_SET_CONFIG,
>>     
> &reg->msgcode_rwbuffer[0]);
>   
>>  			writel(cdb_phyaddr_hi32, &reg->msgcode_rwbuffer[1]);
>>  			writel(ARCMSR_INBOUND_MESG0_SET_CONFIG,
>>     
> &reg->inbound_msgaddr0);
>   
>> @@ -2955,10 +2933,10 @@ static int arcmsr_bus_reset(struct scsi_cmnd *cmd)
>>  				arcmsr_hardware_reset(acb);
>>  				acb->acb_flags &= ~ACB_F_IOP_INITED;
>>  sleep_again:
>> -				arcmsr_sleep_for_bus_reset(cmd);
>> +				ssleep(ARCMSR_SLEEPTIME);
>>  				if ((readl(&reg->outbound_msgaddr1) &
>>     
> ARCMSR_OUTBOUND_MESG1_FIRMWARE_OK) == 0) {
>   
>>  					printk(KERN_ERR "arcmsr%d: waiting
>>     
> for hw bus reset return, retry=%d \n", acb->host->host_no, retry_count);
>   
>> -					if (retry_count > retrycount) {
>> +					if (retry_count > ARCMSR_RETRYCOUNT)
>>     
> {
>   
>>  						acb->fw_flag = FW_DEADLOCK;
>>  						printk(KERN_ERR "arcmsr%d:
>>     
> waiting for hw bus reset return, RETRY TERMINATED!! \n",
> acb->host->host_no);
>   
>>  						return FAILED;
>> @@ -3025,10 +3003,10 @@ sleep_again:
>>  				arcmsr_hardware_reset(acb);
>>  				acb->acb_flags &= ~ACB_F_IOP_INITED;
>>  sleep:
>> -				arcmsr_sleep_for_bus_reset(cmd);
>> +				ssleep(ARCMSR_SLEEPTIME);
>>  				if ((readl(&reg->host_diagnostic) & 0x04) !=
>>     
> 0) {
>   
>>  					printk(KERN_ERR "arcmsr%d: waiting
>>     
> for hw bus reset return, retry=%d \n", acb->host->host_no, retry_count);
>   
>> -					if (retry_count > retrycount) {
>> +					if (retry_count > ARCMSR_RETRYCOUNT)
>>     
> {
>   
>>  						acb->fw_flag = FW_DEADLOCK;
>>  						printk(KERN_ERR "arcmsr%d:
>>     
> waiting for hw bus reset return, RETRY TERMINATED!! \n",
> acb->host->host_no);
>   
>>  						return FAILED;
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>   
>>     
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] arcmsr: code cleanup and some corrections
  2011-02-18 10:59         ` Tomas Henzl
@ 2011-02-24 14:14           ` Tomas Henzl
  2011-02-25  1:30             ` NickCheng
  0 siblings, 1 reply; 15+ messages in thread
From: Tomas Henzl @ 2011-02-24 14:14 UTC (permalink / raw)
  To: FUJITA Tomonori; +Cc: nick.cheng, linux-scsi, James.Bottomley

Hi Nick,
again thanks for your response in the 
'arcmsr: code cleanup and some corrections' thread.

Let us find a resolution here too.
Is the original patch fine for you, or do you prefer not to take it?

Tomas


> On 02/18/2011 02:04 AM, FUJITA Tomonori wrote:
>   
>> On Thu, 17 Feb 2011 16:05:37 +0100
>> Tomas Henzl <thenzl@redhat.com> wrote:
>>
>>   
>>     
>>> the discussion is mostly if 'dma_alloc_coherent' 
>>> ...
>>> dma_coherent = dma_alloc_coherent(&pdev->dev, acb->uncache_size, &dma_coherent_handle, GFP_KERNEL);
>>> ...
>>> returns a page address - dma_coherent with last, I think five bits, zeroed.
>>> Could you help us with that?
>>>     
>>>       
>> Documentation/DMA-API-HOWTO.txt is not clear enough?
>>
>> =
>> dma_alloc_coherent returns two values: the virtual address which you
>> can use to access it from the CPU and dma_handle which you pass to the
>> card.
>>
>> The cpu return address and the DMA bus master address are both
>> guaranteed to be aligned to the smallest PAGE_SIZE order which
>> is greater than or equal to the requested size.  This invariant
>> exists (for example) to guarantee that if you allocate a chunk
>> which is smaller than or equal to 64 kilobytes, the extent of the
>> buffer you receive will not cross a 64K boundary.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>   
>>     
> It's clear enough I think, I asked this question because Nick doesn't
> have a "100% confidence in that.". So I wanted a help to convince him.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   


^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH 2/2] arcmsr: code cleanup and some corrections
  2011-02-24 14:14           ` Tomas Henzl
@ 2011-02-25  1:30             ` NickCheng
  0 siblings, 0 replies; 15+ messages in thread
From: NickCheng @ 2011-02-25  1:30 UTC (permalink / raw)
  To: 'Tomas Henzl', 'FUJITA Tomonori'
  Cc: linux-scsi, James.Bottomley

Acked-by: Nick Cheng<nick.cheng@areca.com.tw>
-----Original Message-----
From: Tomas Henzl [mailto:thenzl@redhat.com] 
Sent: Thursday, February 24, 2011 10:15 PM
To: FUJITA Tomonori
Cc: nick.cheng@areca.com.tw; linux-scsi@vger.kernel.org;
James.Bottomley@HansenPartnership.com
Subject: Re: [PATCH 2/2] arcmsr: code cleanup and some corrections

Hi Nick,
again thanks for your response in the 
'arcmsr: code cleanup and some corrections' thread.

Let us find a resolution here too.
Is the original patch fine for you, or do you prefer not to take it?

Tomas


> On 02/18/2011 02:04 AM, FUJITA Tomonori wrote:
>   
>> On Thu, 17 Feb 2011 16:05:37 +0100
>> Tomas Henzl <thenzl@redhat.com> wrote:
>>
>>   
>>     
>>> the discussion is mostly if 'dma_alloc_coherent' 
>>> ...
>>> dma_coherent = dma_alloc_coherent(&pdev->dev, acb->uncache_size,
&dma_coherent_handle, GFP_KERNEL);
>>> ...
>>> returns a page address - dma_coherent with last, I think five bits,
zeroed.
>>> Could you help us with that?
>>>     
>>>       
>> Documentation/DMA-API-HOWTO.txt is not clear enough?
>>
>> =
>> dma_alloc_coherent returns two values: the virtual address which you
>> can use to access it from the CPU and dma_handle which you pass to the
>> card.
>>
>> The cpu return address and the DMA bus master address are both
>> guaranteed to be aligned to the smallest PAGE_SIZE order which
>> is greater than or equal to the requested size.  This invariant
>> exists (for example) to guarantee that if you allocate a chunk
>> which is smaller than or equal to 64 kilobytes, the extent of the
>> buffer you receive will not cross a 64K boundary.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>   
>>     
> It's clear enough I think, I asked this question because Nick doesn't
> have a "100% confidence in that.". So I wanted a help to convince him.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   


^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH 1/2] arcmsr: code cleanup and some corrections
  2011-02-24 14:09     ` Tomas Henzl
@ 2011-02-25  1:30       ` NickCheng
  0 siblings, 0 replies; 15+ messages in thread
From: NickCheng @ 2011-02-25  1:30 UTC (permalink / raw)
  To: 'Tomas Henzl'; +Cc: linux-scsi

Acked-by: Nick Cheng<nick.cheng@areca.com.tw>
-----Original Message-----
From: Tomas Henzl [mailto:thenzl@redhat.com] 
Sent: Thursday, February 24, 2011 10:10 PM
To: NickCheng
Cc: linux-scsi@vger.kernel.org
Subject: Re: [PATCH 1/2] arcmsr: code cleanup and some corrections

On 02/24/2011 03:02 AM, NickCheng wrote:
> Hi Tomas,
> The test on my site is so far so good.
>   
Thanks Nick,
if this means you accept this patch please
add an 'Acked-by: ...' line so it is clear what you mean.

> -----Original Message-----
> From: NickCheng [mailto:nick.cheng@areca.com.tw] 
> Sent: Friday, February 18, 2011 9:26 AM
> To: 'Tomas Henzl'; 'linux-scsi@vger.kernel.org'
> Subject: RE: [PATCH 1/2] arcmsr: code cleanup and some corrections
>
> Hi Tomas,
> I have not yet tested this code.
> Do you try that?
>
> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com] 
> Sent: Thursday, February 17, 2011 11:08 PM
> To: 'linux-scsi@vger.kernel.org'
> Cc: nick.cheng@areca.com.tw
> Subject: Re: [PATCH 1/2] arcmsr: code cleanup and some corrections
>
> Hi Nick,
>
> any comments here?
>
> tomash
>
>
>   
>> I removed outer loops in ...wait_msgint_ready
>> the sleeptime and retrycount are in fact never changed so I changed them
>> into defines. In arcmsr_flush_hba_cache is a loop removed, which
>> printed the same printk 100 times, one line in log is enough I think.
>> The arcmsr_sleep_for_bus_reset has lost a functionality with the latest
>>     
> patches,
>   
>> The only thing the function does is a long sleep, so it's replaced with a
>>     
> ssleep.
>   
>> Signed-off-by: Tomas henzl <thenzl@redhat.com>
>>
>> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c
>>     
> b/drivers/scsi/arcmsr/arcmsr_hba.c
>   
>> index 984bd52..4cd522b 100644
>> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
>> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
>> @@ -75,8 +75,10 @@ MODULE_AUTHOR("Nick Cheng <support@areca.com.tw>");
>>  MODULE_DESCRIPTION("ARECA (ARC11xx/12xx/16xx/1880) SATA/SAS RAID Host
Bus
>>     
> Adapter");
>   
>>  MODULE_LICENSE("Dual BSD/GPL");
>>  MODULE_VERSION(ARCMSR_DRIVER_VERSION);
>> -static int sleeptime = 10;
>> -static int retrycount = 12;
>> +
>> +#define	ARCMSR_SLEEPTIME	10
>> +#define	ARCMSR_RETRYCOUNT	12
>> +
>>  wait_queue_head_t wait_q;
>>  static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
>>  					struct scsi_cmnd *cmd);
>> @@ -171,24 +173,6 @@ static struct pci_driver arcmsr_pci_driver = {
>>
>>     
>
****************************************************************************
>   
>>     
>
****************************************************************************
>   
>>  */
>> -int arcmsr_sleep_for_bus_reset(struct scsi_cmnd *cmd)
>> -{
>> -		struct Scsi_Host *shost = NULL;
>> -		int i, isleep;
>> -		shost = cmd->device->host;
>> -		isleep = sleeptime / 10;
>> -		if (isleep > 0) {
>> -			for (i = 0; i < isleep; i++) {
>> -				msleep(10000);
>> -			}
>> -		}
>> -
>> -		isleep = sleeptime % 10;
>> -		if (isleep > 0) {
>> -			msleep(isleep*1000);
>> -		}
>> -		return 0;
>> -}
>>  
>>  static void arcmsr_free_hbb_mu(struct AdapterControlBlock *acb)
>>  {
>> @@ -323,66 +307,64 @@ static void arcmsr_define_adapter_type(struct
>>     
> AdapterControlBlock *acb)
>   
>>  
>>  	default: acb->adapter_type = ACB_ADAPTER_TYPE_A;
>>  	}
>> -}	
>> +}
>>  
>>  static uint8_t arcmsr_hba_wait_msgint_ready(struct AdapterControlBlock
>>     
> *acb)
>   
>>  {
>>  	struct MessageUnit_A __iomem *reg = acb->pmuA;
>> -	uint32_t Index;
>> -	uint8_t Retries = 0x00;
>> -	do {
>> -		for (Index = 0; Index < 100; Index++) {
>> -			if (readl(&reg->outbound_intstatus) &
>> -					ARCMSR_MU_OUTBOUND_MESSAGE0_INT) {
>> -				writel(ARCMSR_MU_OUTBOUND_MESSAGE0_INT,
>> -					&reg->outbound_intstatus);
>> -				return true;
>> -			}
>> -			msleep(10);
>> -		}/*max 1 seconds*/
>> +	int i;
>> +
>> +	for (i = 0; i < 2000; i++) {
>> +		if (readl(&reg->outbound_intstatus) &
>> +				ARCMSR_MU_OUTBOUND_MESSAGE0_INT) {
>> +			writel(ARCMSR_MU_OUTBOUND_MESSAGE0_INT,
>> +				&reg->outbound_intstatus);
>> +			return true;
>> +		}
>> +		msleep(10);
>> +	}/* max 20 seconds */
>>  
>> -	} while (Retries++ < 20);/*max 20 sec*/
>>  	return false;
>>  }
>>  
>>  static uint8_t arcmsr_hbb_wait_msgint_ready(struct AdapterControlBlock
>>     
> *acb)
>   
>>  {
>>  	struct MessageUnit_B *reg = acb->pmuB;
>> -	uint32_t Index;
>> -	uint8_t Retries = 0x00;
>> -	do {
>> -		for (Index = 0; Index < 100; Index++) {
>> -			if (readl(reg->iop2drv_doorbell)
>> -				& ARCMSR_IOP2DRV_MESSAGE_CMD_DONE) {
>> -				writel(ARCMSR_MESSAGE_INT_CLEAR_PATTERN
>> -					, reg->iop2drv_doorbell);
>> -				writel(ARCMSR_DRV2IOP_END_OF_INTERRUPT,
>>     
> reg->drv2iop_doorbell);
>   
>> -				return true;
>> -			}
>> -			msleep(10);
>> -		}/*max 1 seconds*/
>> +	int i;
>> +
>> +	for (i = 0; i < 2000; i++) {
>> +		if (readl(reg->iop2drv_doorbell)
>> +			& ARCMSR_IOP2DRV_MESSAGE_CMD_DONE) {
>> +			writel(ARCMSR_MESSAGE_INT_CLEAR_PATTERN,
>> +					reg->iop2drv_doorbell);
>> +			writel(ARCMSR_DRV2IOP_END_OF_INTERRUPT,
>> +					reg->drv2iop_doorbell);
>> +			return true;
>> +		}
>> +		msleep(10);
>> +	}/* max 20 seconds */
>>  
>> -	} while (Retries++ < 20);/*max 20 sec*/
>>  	return false;
>>  }
>>  
>>  static uint8_t arcmsr_hbc_wait_msgint_ready(struct AdapterControlBlock
>>     
> *pACB)
>   
>>  {
>>  	struct MessageUnit_C *phbcmu = (struct MessageUnit_C *)pACB->pmuC;
>> -	unsigned char Retries = 0x00;
>> -	uint32_t Index;
>> -	do {
>> -		for (Index = 0; Index < 100; Index++) {
>> -			if (readl(&phbcmu->outbound_doorbell) &
>>     
> ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE) {
>   
>> -
>>     
> writel(ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE_DOORBELL_CLEAR,
> &phbcmu->outbound_doorbell_clear);/*clear interrupt*/
>   
>> -				return true;
>> -			}
>> -			/* one us delay	*/
>> -			msleep(10);
>> -		} /*max 1 seconds*/
>> -	} while (Retries++ < 20); /*max 20 sec*/
>> +	int i;
>> +
>> +	for (i = 0; i < 2000; i++) {
>> +		if (readl(&phbcmu->outbound_doorbell)
>> +				& ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE) {
>> +
>>     
> writel(ARCMSR_HBCMU_IOP2DRV_MESSAGE_CMD_DONE_DOORBELL_CLEAR,
>   
>> +				&phbcmu->outbound_doorbell_clear); /*clear
>>     
> interrupt*/
>   
>> +			return true;
>> +		}
>> +		msleep(10);
>> +	} /* max 20 seconds */
>> +
>>  	return false;
>>  }
>> +
>>  static void arcmsr_flush_hba_cache(struct AdapterControlBlock *acb)
>>  {
>>  	struct MessageUnit_A __iomem *reg = acb->pmuA;
>> @@ -2602,12 +2584,8 @@ static int arcmsr_iop_confirm(struct
>>     
> AdapterControlBlock *acb)
>   
>>  		if (cdb_phyaddr_hi32 != 0) {
>>  			struct MessageUnit_C *reg = (struct MessageUnit_C
>>     
> *)acb->pmuC;
>   
>>  
>> -			if (cdb_phyaddr_hi32 != 0) {
>> -				unsigned char Retries = 0x00;
>> -				do {
>> -					printk(KERN_NOTICE "arcmsr%d:
>>     
> cdb_phyaddr_hi32=0x%x \n", acb->adapter_index, cdb_phyaddr_hi32);
>   
>> -				} while (Retries++ < 100);
>> -			}
>> +			printk(KERN_NOTICE "arcmsr%d: cdb_phyaddr_hi32=0x%x
>>     
> \n",
>   
>> +					acb->adapter_index,
>>     
> cdb_phyaddr_hi32);
>   
>>  			writel(ARCMSR_SIGNATURE_SET_CONFIG,
>>     
> &reg->msgcode_rwbuffer[0]);
>   
>>  			writel(cdb_phyaddr_hi32, &reg->msgcode_rwbuffer[1]);
>>  			writel(ARCMSR_INBOUND_MESG0_SET_CONFIG,
>>     
> &reg->inbound_msgaddr0);
>   
>> @@ -2955,10 +2933,10 @@ static int arcmsr_bus_reset(struct scsi_cmnd
*cmd)
>>  				arcmsr_hardware_reset(acb);
>>  				acb->acb_flags &= ~ACB_F_IOP_INITED;
>>  sleep_again:
>> -				arcmsr_sleep_for_bus_reset(cmd);
>> +				ssleep(ARCMSR_SLEEPTIME);
>>  				if ((readl(&reg->outbound_msgaddr1) &
>>     
> ARCMSR_OUTBOUND_MESG1_FIRMWARE_OK) == 0) {
>   
>>  					printk(KERN_ERR "arcmsr%d: waiting
>>     
> for hw bus reset return, retry=%d \n", acb->host->host_no, retry_count);
>   
>> -					if (retry_count > retrycount) {
>> +					if (retry_count > ARCMSR_RETRYCOUNT)
>>     
> {
>   
>>  						acb->fw_flag = FW_DEADLOCK;
>>  						printk(KERN_ERR "arcmsr%d:
>>     
> waiting for hw bus reset return, RETRY TERMINATED!! \n",
> acb->host->host_no);
>   
>>  						return FAILED;
>> @@ -3025,10 +3003,10 @@ sleep_again:
>>  				arcmsr_hardware_reset(acb);
>>  				acb->acb_flags &= ~ACB_F_IOP_INITED;
>>  sleep:
>> -				arcmsr_sleep_for_bus_reset(cmd);
>> +				ssleep(ARCMSR_SLEEPTIME);
>>  				if ((readl(&reg->host_diagnostic) & 0x04) !=
>>     
> 0) {
>   
>>  					printk(KERN_ERR "arcmsr%d: waiting
>>     
> for hw bus reset return, retry=%d \n", acb->host->host_no, retry_count);
>   
>> -					if (retry_count > retrycount) {
>> +					if (retry_count > ARCMSR_RETRYCOUNT)
>>     
> {
>   
>>  						acb->fw_flag = FW_DEADLOCK;
>>  						printk(KERN_ERR "arcmsr%d:
>>     
> waiting for hw bus reset return, RETRY TERMINATED!! \n",
> acb->host->host_no);
>   
>>  						return FAILED;
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>   
>>     
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2011-02-25  1:31 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-10 14:22 [PATCH 1/2] arcmsr: code cleanup and some corrections Tomas Henzl
2011-02-10 14:36 ` [PATCH 2/2] " Tomas Henzl
2011-02-11  1:44   ` NickCheng
2011-02-17 15:05     ` Tomas Henzl
2011-02-18  1:04       ` FUJITA Tomonori
2011-02-18 10:59         ` Tomas Henzl
2011-02-24 14:14           ` Tomas Henzl
2011-02-25  1:30             ` NickCheng
2011-02-17 15:07 ` [PATCH 1/2] " Tomas Henzl
2011-02-18  1:26   ` NickCheng
2011-02-18 11:04     ` Tomas Henzl
2011-02-18 11:36       ` NickCheng
2011-02-24  2:02   ` NickCheng
2011-02-24 14:09     ` Tomas Henzl
2011-02-25  1:30       ` NickCheng

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.