* [PATCH v4 1/2] arcmsr: simplify ioctl data read/write
@ 2014-09-12 7:29 Ching Huang
2014-09-12 13:34 ` Tomas Henzl
0 siblings, 1 reply; 7+ messages in thread
From: Ching Huang @ 2014-09-12 7:29 UTC (permalink / raw)
To: hch, thenzl, jbottomley, dan.carpenter, agordeev, linux-scsi,
linux-kernel
From: Ching Huang <ching2048@areca.com.tw>
This patch is to modify previous patch 13/17 and it is relative to
http://git.infradead.org/users/hch/scsi-queue.git/tree/arcmsr-for-3.18:/drivers/scsi/arcmsr
change in v4:
1. for readability, rename firstindex to getIndex, rename lastindex to putIndex
2. define ARCMSR_API_DATA_BUFLEN as 1032
3. simplify ioctl data read by macro CIRC_CNT_TO_END and CIRC_CNT
Signed-off-by: Ching Huang <ching2048@areca.com.tw>
---
diff -uprN a/drivers/scsi/arcmsr/arcmsr_attr.c b/drivers/scsi/arcmsr/arcmsr_attr.c
--- a/drivers/scsi/arcmsr/arcmsr_attr.c 2014-08-21 12:14:27.000000000 +0800
+++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-09-12 15:18:46.659125000 +0800
@@ -50,6 +50,7 @@
#include <linux/errno.h>
#include <linux/delay.h>
#include <linux/pci.h>
+#include <linux/circ_buf.h>
#include <scsi/scsi_cmnd.h>
#include <scsi/scsi_device.h>
@@ -68,7 +69,7 @@ static ssize_t arcmsr_sysfs_iop_message_
struct device *dev = container_of(kobj,struct device,kobj);
struct Scsi_Host *host = class_to_shost(dev);
struct AdapterControlBlock *acb = (struct AdapterControlBlock *) host->hostdata;
- uint8_t *pQbuffer,*ptmpQbuffer;
+ uint8_t *ptmpQbuffer;
int32_t allxfer_len = 0;
unsigned long flags;
@@ -78,57 +79,22 @@ static ssize_t arcmsr_sysfs_iop_message_
/* do message unit read. */
ptmpQbuffer = (uint8_t *)buf;
spin_lock_irqsave(&acb->rqbuffer_lock, flags);
- if (acb->rqbuf_firstindex != acb->rqbuf_lastindex) {
- pQbuffer = &acb->rqbuffer[acb->rqbuf_firstindex];
- if (acb->rqbuf_firstindex > acb->rqbuf_lastindex) {
- if ((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex) >= 1032) {
- memcpy(ptmpQbuffer, pQbuffer, 1032);
- acb->rqbuf_firstindex += 1032;
- acb->rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
- allxfer_len = 1032;
- } else {
- if (((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex)
- + acb->rqbuf_lastindex) > 1032) {
- memcpy(ptmpQbuffer, pQbuffer,
- ARCMSR_MAX_QBUFFER
- - acb->rqbuf_firstindex);
- ptmpQbuffer += ARCMSR_MAX_QBUFFER
- - acb->rqbuf_firstindex;
- memcpy(ptmpQbuffer, acb->rqbuffer, 1032
- - (ARCMSR_MAX_QBUFFER -
- acb->rqbuf_firstindex));
- acb->rqbuf_firstindex = 1032 -
- (ARCMSR_MAX_QBUFFER -
- acb->rqbuf_firstindex);
- allxfer_len = 1032;
- } else {
- memcpy(ptmpQbuffer, pQbuffer,
- ARCMSR_MAX_QBUFFER -
- acb->rqbuf_firstindex);
- ptmpQbuffer += ARCMSR_MAX_QBUFFER -
- acb->rqbuf_firstindex;
- memcpy(ptmpQbuffer, acb->rqbuffer,
- acb->rqbuf_lastindex);
- allxfer_len = ARCMSR_MAX_QBUFFER -
- acb->rqbuf_firstindex +
- acb->rqbuf_lastindex;
- acb->rqbuf_firstindex =
- acb->rqbuf_lastindex;
- }
- }
- } else {
- if ((acb->rqbuf_lastindex - acb->rqbuf_firstindex) > 1032) {
- memcpy(ptmpQbuffer, pQbuffer, 1032);
- acb->rqbuf_firstindex += 1032;
- allxfer_len = 1032;
- } else {
- memcpy(ptmpQbuffer, pQbuffer, acb->rqbuf_lastindex
- - acb->rqbuf_firstindex);
- allxfer_len = acb->rqbuf_lastindex -
- acb->rqbuf_firstindex;
- acb->rqbuf_firstindex = acb->rqbuf_lastindex;
- }
+ if (acb->rqbuf_getIndex != acb->rqbuf_putIndex) {
+ unsigned int tail = acb->rqbuf_getIndex;
+ unsigned int head = acb->rqbuf_putIndex;
+ unsigned int cnt_to_end = CIRC_CNT_TO_END(head, tail, ARCMSR_MAX_QBUFFER);
+
+ allxfer_len = CIRC_CNT(head, tail, ARCMSR_MAX_QBUFFER);
+ if (allxfer_len > ARCMSR_API_DATA_BUFLEN)
+ allxfer_len = ARCMSR_API_DATA_BUFLEN;
+
+ if (allxfer_len <= cnt_to_end)
+ memcpy(ptmpQbuffer, acb->rqbuffer + tail, allxfer_len);
+ else {
+ memcpy(ptmpQbuffer, acb->rqbuffer + tail, cnt_to_end);
+ memcpy(ptmpQbuffer + cnt_to_end, acb->rqbuffer, allxfer_len - cnt_to_end);
}
+ acb->rqbuf_getIndex = (acb->rqbuf_getIndex + allxfer_len) % ARCMSR_MAX_QBUFFER;
}
if (acb->acb_flags & ACB_F_IOPDATA_OVERFLOW) {
struct QBUFFER __iomem *prbuffer;
@@ -150,33 +116,32 @@ static ssize_t arcmsr_sysfs_iop_message_
struct device *dev = container_of(kobj,struct device,kobj);
struct Scsi_Host *host = class_to_shost(dev);
struct AdapterControlBlock *acb = (struct AdapterControlBlock *) host->hostdata;
- int32_t my_empty_len, user_len, wqbuf_firstindex, wqbuf_lastindex;
+ int32_t my_empty_len, user_len, wqbuf_getIndex, wqbuf_putIndex;
uint8_t *pQbuffer, *ptmpuserbuffer;
unsigned long flags;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
- if (count > 1032)
+ if (count > ARCMSR_API_DATA_BUFLEN)
return -EINVAL;
/* do message unit write. */
ptmpuserbuffer = (uint8_t *)buf;
user_len = (int32_t)count;
spin_lock_irqsave(&acb->wqbuffer_lock, flags);
- wqbuf_lastindex = acb->wqbuf_lastindex;
- wqbuf_firstindex = acb->wqbuf_firstindex;
- if (wqbuf_lastindex != wqbuf_firstindex) {
+ wqbuf_putIndex = acb->wqbuf_putIndex;
+ wqbuf_getIndex = acb->wqbuf_getIndex;
+ if (wqbuf_putIndex != wqbuf_getIndex) {
arcmsr_write_ioctldata2iop(acb);
spin_unlock_irqrestore(&acb->wqbuffer_lock, flags);
return 0; /*need retry*/
} else {
- my_empty_len = (wqbuf_firstindex-wqbuf_lastindex - 1)
- &(ARCMSR_MAX_QBUFFER - 1);
+ my_empty_len = ARCMSR_MAX_QBUFFER - 1;
if (my_empty_len >= user_len) {
while (user_len > 0) {
- pQbuffer = &acb->wqbuffer[acb->wqbuf_lastindex];
+ pQbuffer = &acb->wqbuffer[acb->wqbuf_putIndex];
memcpy(pQbuffer, ptmpuserbuffer, 1);
- acb->wqbuf_lastindex++;
- acb->wqbuf_lastindex %= ARCMSR_MAX_QBUFFER;
+ acb->wqbuf_putIndex++;
+ acb->wqbuf_putIndex %= ARCMSR_MAX_QBUFFER;
ptmpuserbuffer++;
user_len--;
}
@@ -215,12 +180,12 @@ static ssize_t arcmsr_sysfs_iop_message_
| ACB_F_MESSAGE_RQBUFFER_CLEARED
| ACB_F_MESSAGE_WQBUFFER_READED);
spin_lock_irqsave(&acb->rqbuffer_lock, flags);
- acb->rqbuf_firstindex = 0;
- acb->rqbuf_lastindex = 0;
+ acb->rqbuf_getIndex = 0;
+ acb->rqbuf_putIndex = 0;
spin_unlock_irqrestore(&acb->rqbuffer_lock, flags);
spin_lock_irqsave(&acb->wqbuffer_lock, flags);
- acb->wqbuf_firstindex = 0;
- acb->wqbuf_lastindex = 0;
+ acb->wqbuf_getIndex = 0;
+ acb->wqbuf_putIndex = 0;
spin_unlock_irqrestore(&acb->wqbuffer_lock, flags);
pQbuffer = acb->rqbuffer;
memset(pQbuffer, 0, sizeof (struct QBUFFER));
@@ -234,7 +199,7 @@ static struct bin_attribute arcmsr_sysfs
.name = "mu_read",
.mode = S_IRUSR ,
},
- .size = 1032,
+ .size = ARCMSR_API_DATA_BUFLEN,
.read = arcmsr_sysfs_iop_message_read,
};
@@ -243,7 +208,7 @@ static struct bin_attribute arcmsr_sysfs
.name = "mu_write",
.mode = S_IWUSR,
},
- .size = 1032,
+ .size = ARCMSR_API_DATA_BUFLEN,
.write = arcmsr_sysfs_iop_message_write,
};
diff -uprN a/drivers/scsi/arcmsr/arcmsr.h b/drivers/scsi/arcmsr/arcmsr.h
--- a/drivers/scsi/arcmsr/arcmsr.h 2014-08-21 12:14:27.000000000 +0800
+++ b/drivers/scsi/arcmsr/arcmsr.h 2014-08-25 17:25:20.000000000 +0800
@@ -107,10 +107,11 @@ struct CMD_MESSAGE
** IOP Message Transfer Data for user space
*******************************************************************************
*/
+#define ARCMSR_API_DATA_BUFLEN 1032
struct CMD_MESSAGE_FIELD
{
struct CMD_MESSAGE cmdmessage;
- uint8_t messagedatabuffer[1032];
+ uint8_t messagedatabuffer[ARCMSR_API_DATA_BUFLEN];
};
/* IOP message transfer */
#define ARCMSR_MESSAGE_FAIL 0x0001
@@ -678,15 +679,15 @@ struct AdapterControlBlock
unsigned int uncache_size;
uint8_t rqbuffer[ARCMSR_MAX_QBUFFER];
/* data collection buffer for read from 80331 */
- int32_t rqbuf_firstindex;
+ int32_t rqbuf_getIndex;
/* first of read buffer */
- int32_t rqbuf_lastindex;
+ int32_t rqbuf_putIndex;
/* last of read buffer */
uint8_t wqbuffer[ARCMSR_MAX_QBUFFER];
/* data collection buffer for write to 80331 */
- int32_t wqbuf_firstindex;
+ int32_t wqbuf_getIndex;
/* first of write buffer */
- int32_t wqbuf_lastindex;
+ int32_t wqbuf_putIndex;
/* last of write buffer */
uint8_t devstate[ARCMSR_MAX_TARGETID][ARCMSR_MAX_TARGETLUN];
/* id0 ..... id15, lun0...lun7 */
diff -uprN a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
--- a/drivers/scsi/arcmsr/arcmsr_hba.c 2014-08-21 12:14:27.000000000 +0800
+++ b/drivers/scsi/arcmsr/arcmsr_hba.c 2014-09-12 12:43:16.956653000 +0800
@@ -58,6 +58,7 @@
#include <linux/slab.h>
#include <linux/pci.h>
#include <linux/aer.h>
+#include <linux/circ_buf.h>
#include <asm/dma.h>
#include <asm/io.h>
#include <asm/uaccess.h>
@@ -1724,16 +1725,15 @@ arcmsr_Read_iop_rqbuffer_in_DWORD(struct
buf2 = (uint32_t *)buf1;
}
while (iop_len > 0) {
- pQbuffer = &acb->rqbuffer[acb->rqbuf_lastindex];
+ pQbuffer = &acb->rqbuffer[acb->rqbuf_putIndex];
*pQbuffer = *buf1;
- acb->rqbuf_lastindex++;
+ acb->rqbuf_putIndex++;
/* if last, index number set it to 0 */
- acb->rqbuf_lastindex %= ARCMSR_MAX_QBUFFER;
+ acb->rqbuf_putIndex %= ARCMSR_MAX_QBUFFER;
buf1++;
iop_len--;
}
- if (buf2)
- kfree(buf2);
+ kfree(buf2);
/* let IOP know data has been read */
arcmsr_iop_message_read(acb);
return 1;
@@ -1752,10 +1752,10 @@ arcmsr_Read_iop_rqbuffer_data(struct Ada
iop_data = (uint8_t __iomem *)prbuffer->data;
iop_len = readl(&prbuffer->data_len);
while (iop_len > 0) {
- pQbuffer = &acb->rqbuffer[acb->rqbuf_lastindex];
+ pQbuffer = &acb->rqbuffer[acb->rqbuf_putIndex];
*pQbuffer = readb(iop_data);
- acb->rqbuf_lastindex++;
- acb->rqbuf_lastindex %= ARCMSR_MAX_QBUFFER;
+ acb->rqbuf_putIndex++;
+ acb->rqbuf_putIndex %= ARCMSR_MAX_QBUFFER;
iop_data++;
iop_len--;
}
@@ -1771,7 +1771,7 @@ static void arcmsr_iop2drv_data_wrote_ha
spin_lock_irqsave(&acb->rqbuffer_lock, flags);
prbuffer = arcmsr_get_iop_rqbuffer(acb);
- buf_empty_len = (acb->rqbuf_lastindex - acb->rqbuf_firstindex - 1) &
+ buf_empty_len = (acb->rqbuf_putIndex - acb->rqbuf_getIndex - 1) &
(ARCMSR_MAX_QBUFFER - 1);
if (buf_empty_len >= readl(&prbuffer->data_len)) {
if (arcmsr_Read_iop_rqbuffer_data(acb, prbuffer) == 0)
@@ -1798,12 +1798,12 @@ static void arcmsr_write_ioctldata2iop_i
acb->acb_flags &= (~ACB_F_MESSAGE_WQBUFFER_READED);
pwbuffer = arcmsr_get_iop_wqbuffer(acb);
iop_data = (uint32_t __iomem *)pwbuffer->data;
- while ((acb->wqbuf_firstindex != acb->wqbuf_lastindex)
+ while ((acb->wqbuf_getIndex != acb->wqbuf_putIndex)
&& (allxfer_len < 124)) {
- pQbuffer = &acb->wqbuffer[acb->wqbuf_firstindex];
+ pQbuffer = &acb->wqbuffer[acb->wqbuf_getIndex];
*buf1 = *pQbuffer;
- acb->wqbuf_firstindex++;
- acb->wqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
+ acb->wqbuf_getIndex++;
+ acb->wqbuf_getIndex %= ARCMSR_MAX_QBUFFER;
buf1++;
allxfer_len++;
}
@@ -1841,12 +1841,12 @@ arcmsr_write_ioctldata2iop(struct Adapte
acb->acb_flags &= (~ACB_F_MESSAGE_WQBUFFER_READED);
pwbuffer = arcmsr_get_iop_wqbuffer(acb);
iop_data = (uint8_t __iomem *)pwbuffer->data;
- while ((acb->wqbuf_firstindex != acb->wqbuf_lastindex)
+ while ((acb->wqbuf_getIndex != acb->wqbuf_putIndex)
&& (allxfer_len < 124)) {
- pQbuffer = &acb->wqbuffer[acb->wqbuf_firstindex];
+ pQbuffer = &acb->wqbuffer[acb->wqbuf_getIndex];
writeb(*pQbuffer, iop_data);
- acb->wqbuf_firstindex++;
- acb->wqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
+ acb->wqbuf_getIndex++;
+ acb->wqbuf_getIndex %= ARCMSR_MAX_QBUFFER;
iop_data++;
allxfer_len++;
}
@@ -1861,9 +1861,9 @@ static void arcmsr_iop2drv_data_read_han
spin_lock_irqsave(&acb->wqbuffer_lock, flags);
acb->acb_flags |= ACB_F_MESSAGE_WQBUFFER_READED;
- if (acb->wqbuf_firstindex != acb->wqbuf_lastindex)
+ if (acb->wqbuf_getIndex != acb->wqbuf_putIndex)
arcmsr_write_ioctldata2iop(acb);
- if (acb->wqbuf_firstindex == acb->wqbuf_lastindex)
+ if (acb->wqbuf_getIndex == acb->wqbuf_putIndex)
acb->acb_flags |= ACB_F_MESSAGE_WQBUFFER_CLEARED;
spin_unlock_irqrestore(&acb->wqbuffer_lock, flags);
}
@@ -2243,14 +2243,14 @@ void arcmsr_clear_iop2drv_rqueue_buffer(
for (i = 0; i < 15; i++) {
if (acb->acb_flags & ACB_F_IOPDATA_OVERFLOW) {
acb->acb_flags &= ~ACB_F_IOPDATA_OVERFLOW;
- acb->rqbuf_firstindex = 0;
- acb->rqbuf_lastindex = 0;
+ acb->rqbuf_getIndex = 0;
+ acb->rqbuf_putIndex = 0;
arcmsr_iop_message_read(acb);
mdelay(30);
- } else if (acb->rqbuf_firstindex !=
- acb->rqbuf_lastindex) {
- acb->rqbuf_firstindex = 0;
- acb->rqbuf_lastindex = 0;
+ } else if (acb->rqbuf_getIndex !=
+ acb->rqbuf_putIndex) {
+ acb->rqbuf_getIndex = 0;
+ acb->rqbuf_putIndex = 0;
mdelay(30);
} else
break;
@@ -2289,9 +2289,9 @@ static int arcmsr_iop_message_xfer(struc
switch (controlcode) {
case ARCMSR_MESSAGE_READ_RQBUFFER: {
unsigned char *ver_addr;
- uint8_t *pQbuffer, *ptmpQbuffer;
+ uint8_t *ptmpQbuffer;
uint32_t allxfer_len = 0;
- ver_addr = kmalloc(1032, GFP_ATOMIC);
+ ver_addr = kmalloc(ARCMSR_API_DATA_BUFLEN, GFP_ATOMIC);
if (!ver_addr) {
retvalue = ARCMSR_MESSAGE_FAIL;
pr_info("%s: memory not enough!\n", __func__);
@@ -2299,66 +2299,22 @@ static int arcmsr_iop_message_xfer(struc
}
ptmpQbuffer = ver_addr;
spin_lock_irqsave(&acb->rqbuffer_lock, flags);
- if (acb->rqbuf_firstindex != acb->rqbuf_lastindex) {
- pQbuffer = &acb->rqbuffer[acb->rqbuf_firstindex];
- if (acb->rqbuf_firstindex > acb->rqbuf_lastindex) {
- if ((ARCMSR_MAX_QBUFFER -
- acb->rqbuf_firstindex) >= 1032) {
- memcpy(ptmpQbuffer, pQbuffer, 1032);
- acb->rqbuf_firstindex += 1032;
- acb->rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
- allxfer_len = 1032;
- } else {
- if (((ARCMSR_MAX_QBUFFER -
- acb->rqbuf_firstindex) +
- acb->rqbuf_lastindex) > 1032) {
- memcpy(ptmpQbuffer,
- pQbuffer, ARCMSR_MAX_QBUFFER
- - acb->rqbuf_firstindex);
- ptmpQbuffer +=
- ARCMSR_MAX_QBUFFER -
- acb->rqbuf_firstindex;
- memcpy(ptmpQbuffer,
- acb->rqbuffer, 1032 -
- (ARCMSR_MAX_QBUFFER
- - acb->rqbuf_firstindex));
- acb->rqbuf_firstindex =
- 1032 - (ARCMSR_MAX_QBUFFER
- - acb->rqbuf_firstindex);
- allxfer_len = 1032;
- } else {
- memcpy(ptmpQbuffer,
- pQbuffer, ARCMSR_MAX_QBUFFER
- - acb->rqbuf_firstindex);
- ptmpQbuffer +=
- ARCMSR_MAX_QBUFFER -
- acb->rqbuf_firstindex;
- memcpy(ptmpQbuffer,
- acb->rqbuffer,
- acb->rqbuf_lastindex);
- allxfer_len = ARCMSR_MAX_QBUFFER
- - acb->rqbuf_firstindex +
- acb->rqbuf_lastindex;
- acb->rqbuf_firstindex =
- acb->rqbuf_lastindex;
- }
- }
- } else {
- if ((acb->rqbuf_lastindex -
- acb->rqbuf_firstindex) > 1032) {
- memcpy(ptmpQbuffer, pQbuffer, 1032);
- acb->rqbuf_firstindex += 1032;
- allxfer_len = 1032;
- } else {
- memcpy(ptmpQbuffer, pQbuffer,
- acb->rqbuf_lastindex -
- acb->rqbuf_firstindex);
- allxfer_len = acb->rqbuf_lastindex
- - acb->rqbuf_firstindex;
- acb->rqbuf_firstindex =
- acb->rqbuf_lastindex;
- }
+ if (acb->rqbuf_getIndex != acb->rqbuf_putIndex) {
+ unsigned int tail = acb->rqbuf_getIndex;
+ unsigned int head = acb->rqbuf_putIndex;
+ unsigned int cnt_to_end = CIRC_CNT_TO_END(head, tail, ARCMSR_MAX_QBUFFER);
+
+ allxfer_len = CIRC_CNT(head, tail, ARCMSR_MAX_QBUFFER);
+ if (allxfer_len > ARCMSR_API_DATA_BUFLEN)
+ allxfer_len = ARCMSR_API_DATA_BUFLEN;
+
+ if (allxfer_len <= cnt_to_end)
+ memcpy(ptmpQbuffer, acb->rqbuffer + tail, allxfer_len);
+ else {
+ memcpy(ptmpQbuffer, acb->rqbuffer + tail, cnt_to_end);
+ memcpy(ptmpQbuffer + cnt_to_end, acb->rqbuffer, allxfer_len - cnt_to_end);
}
+ acb->rqbuf_getIndex = (acb->rqbuf_getIndex + allxfer_len) % ARCMSR_MAX_QBUFFER;
}
memcpy(pcmdmessagefld->messagedatabuffer, ver_addr,
allxfer_len);
@@ -2382,9 +2338,9 @@ static int arcmsr_iop_message_xfer(struc
}
case ARCMSR_MESSAGE_WRITE_WQBUFFER: {
unsigned char *ver_addr;
- int32_t my_empty_len, user_len, wqbuf_firstindex, wqbuf_lastindex;
+ int32_t my_empty_len, user_len, wqbuf_getIndex, wqbuf_putIndex, cnt2end;
uint8_t *pQbuffer, *ptmpuserbuffer;
- ver_addr = kmalloc(1032, GFP_ATOMIC);
+ ver_addr = kmalloc(ARCMSR_API_DATA_BUFLEN, GFP_ATOMIC);
if (!ver_addr) {
retvalue = ARCMSR_MESSAGE_FAIL;
goto message_out;
@@ -2394,9 +2350,9 @@ static int arcmsr_iop_message_xfer(struc
memcpy(ptmpuserbuffer,
pcmdmessagefld->messagedatabuffer, user_len);
spin_lock_irqsave(&acb->wqbuffer_lock, flags);
- wqbuf_lastindex = acb->wqbuf_lastindex;
- wqbuf_firstindex = acb->wqbuf_firstindex;
- if (wqbuf_lastindex != wqbuf_firstindex) {
+ wqbuf_putIndex = acb->wqbuf_putIndex;
+ wqbuf_getIndex = acb->wqbuf_getIndex;
+ if (wqbuf_putIndex != wqbuf_getIndex) {
struct SENSE_DATA *sensebuffer =
(struct SENSE_DATA *)cmd->sense_buffer;
arcmsr_write_ioctldata2iop(acb);
@@ -2408,27 +2364,24 @@ static int arcmsr_iop_message_xfer(struc
sensebuffer->Valid = 1;
retvalue = ARCMSR_MESSAGE_FAIL;
} else {
- my_empty_len = (wqbuf_firstindex - wqbuf_lastindex - 1)
- & (ARCMSR_MAX_QBUFFER - 1);
+ my_empty_len = ARCMSR_MAX_QBUFFER - 1;
if (my_empty_len >= user_len) {
while (user_len > 0) {
- pQbuffer = &acb->wqbuffer[acb->wqbuf_lastindex];
- if ((acb->wqbuf_lastindex + user_len)
+ pQbuffer = &acb->wqbuffer[acb->wqbuf_putIndex];
+ if ((acb->wqbuf_putIndex + user_len)
> ARCMSR_MAX_QBUFFER) {
+ cnt2end = ARCMSR_MAX_QBUFFER -
+ acb->wqbuf_putIndex;
memcpy(pQbuffer, ptmpuserbuffer,
- ARCMSR_MAX_QBUFFER -
- acb->wqbuf_lastindex);
- ptmpuserbuffer +=
- (ARCMSR_MAX_QBUFFER
- - acb->wqbuf_lastindex);
- user_len -= (ARCMSR_MAX_QBUFFER
- - acb->wqbuf_lastindex);
- acb->wqbuf_lastindex = 0;
+ cnt2end);
+ ptmpuserbuffer += cnt2end;
+ user_len -= cnt2end;
+ acb->wqbuf_putIndex = 0;
} else {
memcpy(pQbuffer, ptmpuserbuffer,
user_len);
- acb->wqbuf_lastindex += user_len;
- acb->wqbuf_lastindex %=
+ acb->wqbuf_putIndex += user_len;
+ acb->wqbuf_putIndex %=
ARCMSR_MAX_QBUFFER;
user_len = 0;
}
@@ -2468,8 +2421,8 @@ static int arcmsr_iop_message_xfer(struc
arcmsr_clear_iop2drv_rqueue_buffer(acb);
spin_lock_irqsave(&acb->rqbuffer_lock, flags);
acb->acb_flags |= ACB_F_MESSAGE_RQBUFFER_CLEARED;
- acb->rqbuf_firstindex = 0;
- acb->rqbuf_lastindex = 0;
+ acb->rqbuf_getIndex = 0;
+ acb->rqbuf_putIndex = 0;
memset(pQbuffer, 0, ARCMSR_MAX_QBUFFER);
spin_unlock_irqrestore(&acb->rqbuffer_lock, flags);
if (acb->fw_flag == FW_DEADLOCK)
@@ -2485,8 +2438,8 @@ static int arcmsr_iop_message_xfer(struc
spin_lock_irqsave(&acb->wqbuffer_lock, flags);
acb->acb_flags |= (ACB_F_MESSAGE_WQBUFFER_CLEARED |
ACB_F_MESSAGE_WQBUFFER_READED);
- acb->wqbuf_firstindex = 0;
- acb->wqbuf_lastindex = 0;
+ acb->wqbuf_getIndex = 0;
+ acb->wqbuf_putIndex = 0;
memset(pQbuffer, 0, ARCMSR_MAX_QBUFFER);
spin_unlock_irqrestore(&acb->wqbuffer_lock, flags);
if (acb->fw_flag == FW_DEADLOCK)
@@ -2502,16 +2455,16 @@ static int arcmsr_iop_message_xfer(struc
arcmsr_clear_iop2drv_rqueue_buffer(acb);
spin_lock_irqsave(&acb->rqbuffer_lock, flags);
acb->acb_flags |= ACB_F_MESSAGE_RQBUFFER_CLEARED;
- acb->rqbuf_firstindex = 0;
- acb->rqbuf_lastindex = 0;
+ acb->rqbuf_getIndex = 0;
+ acb->rqbuf_putIndex = 0;
pQbuffer = acb->rqbuffer;
memset(pQbuffer, 0, sizeof(struct QBUFFER));
spin_unlock_irqrestore(&acb->rqbuffer_lock, flags);
spin_lock_irqsave(&acb->wqbuffer_lock, flags);
acb->acb_flags |= (ACB_F_MESSAGE_WQBUFFER_CLEARED |
ACB_F_MESSAGE_WQBUFFER_READED);
- acb->wqbuf_firstindex = 0;
- acb->wqbuf_lastindex = 0;
+ acb->wqbuf_getIndex = 0;
+ acb->wqbuf_putIndex = 0;
pQbuffer = acb->wqbuffer;
memset(pQbuffer, 0, sizeof(struct QBUFFER));
spin_unlock_irqrestore(&acb->wqbuffer_lock, flags);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] arcmsr: simplify ioctl data read/write
2014-09-12 7:29 [PATCH v4 1/2] arcmsr: simplify ioctl data read/write Ching Huang
@ 2014-09-12 13:34 ` Tomas Henzl
2014-09-15 2:56 ` Ching Huang
0 siblings, 1 reply; 7+ messages in thread
From: Tomas Henzl @ 2014-09-12 13:34 UTC (permalink / raw)
To: Ching Huang, hch, jbottomley, dan.carpenter, agordeev,
linux-scsi, linux-kernel
On 09/12/2014 09:29 AM, Ching Huang wrote:
> From: Ching Huang <ching2048@areca.com.tw>
>
> This patch is to modify previous patch 13/17 and it is relative to
> http://git.infradead.org/users/hch/scsi-queue.git/tree/arcmsr-for-3.18:/drivers/scsi/arcmsr
>
> change in v4:
> 1. for readability, rename firstindex to getIndex, rename lastindex to putIndex
For some reason, the names head+tail areusual for a circular buffer.
But let us ignore the names, I don't care.
> 2. define ARCMSR_API_DATA_BUFLEN as 1032
> 3. simplify ioctl data read by macro CIRC_CNT_TO_END and CIRC_CNT
It's definitely better when you post renames and other non-functional changes in separate
patches, it's easier for the reviewer.
>
> Signed-off-by: Ching Huang <ching2048@areca.com.tw>
> ---
>
> diff -uprN a/drivers/scsi/arcmsr/arcmsr_attr.c b/drivers/scsi/arcmsr/arcmsr_attr.c
> --- a/drivers/scsi/arcmsr/arcmsr_attr.c 2014-08-21 12:14:27.000000000 +0800
> +++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-09-12 15:18:46.659125000 +0800
> @@ -50,6 +50,7 @@
> #include <linux/errno.h>
> #include <linux/delay.h>
> #include <linux/pci.h>
> +#include <linux/circ_buf.h>
>
> #include <scsi/scsi_cmnd.h>
> #include <scsi/scsi_device.h>
> @@ -68,7 +69,7 @@ static ssize_t arcmsr_sysfs_iop_message_
> struct device *dev = container_of(kobj,struct device,kobj);
> struct Scsi_Host *host = class_to_shost(dev);
> struct AdapterControlBlock *acb = (struct AdapterControlBlock *) host->hostdata;
> - uint8_t *pQbuffer,*ptmpQbuffer;
> + uint8_t *ptmpQbuffer;
> int32_t allxfer_len = 0;
> unsigned long flags;
>
> @@ -78,57 +79,22 @@ static ssize_t arcmsr_sysfs_iop_message_
> /* do message unit read. */
> ptmpQbuffer = (uint8_t *)buf;
> spin_lock_irqsave(&acb->rqbuffer_lock, flags);
> - if (acb->rqbuf_firstindex != acb->rqbuf_lastindex) {
> - pQbuffer = &acb->rqbuffer[acb->rqbuf_firstindex];
> - if (acb->rqbuf_firstindex > acb->rqbuf_lastindex) {
> - if ((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex) >= 1032) {
> - memcpy(ptmpQbuffer, pQbuffer, 1032);
> - acb->rqbuf_firstindex += 1032;
> - acb->rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
> - allxfer_len = 1032;
> - } else {
> - if (((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex)
> - + acb->rqbuf_lastindex) > 1032) {
> - memcpy(ptmpQbuffer, pQbuffer,
> - ARCMSR_MAX_QBUFFER
> - - acb->rqbuf_firstindex);
> - ptmpQbuffer += ARCMSR_MAX_QBUFFER
> - - acb->rqbuf_firstindex;
> - memcpy(ptmpQbuffer, acb->rqbuffer, 1032
> - - (ARCMSR_MAX_QBUFFER -
> - acb->rqbuf_firstindex));
> - acb->rqbuf_firstindex = 1032 -
> - (ARCMSR_MAX_QBUFFER -
> - acb->rqbuf_firstindex);
> - allxfer_len = 1032;
> - } else {
> - memcpy(ptmpQbuffer, pQbuffer,
> - ARCMSR_MAX_QBUFFER -
> - acb->rqbuf_firstindex);
> - ptmpQbuffer += ARCMSR_MAX_QBUFFER -
> - acb->rqbuf_firstindex;
> - memcpy(ptmpQbuffer, acb->rqbuffer,
> - acb->rqbuf_lastindex);
> - allxfer_len = ARCMSR_MAX_QBUFFER -
> - acb->rqbuf_firstindex +
> - acb->rqbuf_lastindex;
> - acb->rqbuf_firstindex =
> - acb->rqbuf_lastindex;
> - }
> - }
> - } else {
> - if ((acb->rqbuf_lastindex - acb->rqbuf_firstindex) > 1032) {
> - memcpy(ptmpQbuffer, pQbuffer, 1032);
> - acb->rqbuf_firstindex += 1032;
> - allxfer_len = 1032;
> - } else {
> - memcpy(ptmpQbuffer, pQbuffer, acb->rqbuf_lastindex
> - - acb->rqbuf_firstindex);
> - allxfer_len = acb->rqbuf_lastindex -
> - acb->rqbuf_firstindex;
> - acb->rqbuf_firstindex = acb->rqbuf_lastindex;
> - }
> + if (acb->rqbuf_getIndex != acb->rqbuf_putIndex) {
> + unsigned int tail = acb->rqbuf_getIndex;
> + unsigned int head = acb->rqbuf_putIndex;
> + unsigned int cnt_to_end = CIRC_CNT_TO_END(head, tail, ARCMSR_MAX_QBUFFER);
> +
> + allxfer_len = CIRC_CNT(head, tail, ARCMSR_MAX_QBUFFER);
> + if (allxfer_len > ARCMSR_API_DATA_BUFLEN)
> + allxfer_len = ARCMSR_API_DATA_BUFLEN;
> +
> + if (allxfer_len <= cnt_to_end)
> + memcpy(ptmpQbuffer, acb->rqbuffer + tail, allxfer_len);
> + else {
> + memcpy(ptmpQbuffer, acb->rqbuffer + tail, cnt_to_end);
> + memcpy(ptmpQbuffer + cnt_to_end, acb->rqbuffer, allxfer_len - cnt_to_end);
> }
> + acb->rqbuf_getIndex = (acb->rqbuf_getIndex + allxfer_len) % ARCMSR_MAX_QBUFFER;
> }
> if (acb->acb_flags & ACB_F_IOPDATA_OVERFLOW) {
> struct QBUFFER __iomem *prbuffer;
> @@ -150,33 +116,32 @@ static ssize_t arcmsr_sysfs_iop_message_
> struct device *dev = container_of(kobj,struct device,kobj);
> struct Scsi_Host *host = class_to_shost(dev);
> struct AdapterControlBlock *acb = (struct AdapterControlBlock *) host->hostdata;
> - int32_t my_empty_len, user_len, wqbuf_firstindex, wqbuf_lastindex;
> + int32_t my_empty_len, user_len, wqbuf_getIndex, wqbuf_putIndex;
> uint8_t *pQbuffer, *ptmpuserbuffer;
> unsigned long flags;
>
> if (!capable(CAP_SYS_ADMIN))
> return -EACCES;
> - if (count > 1032)
> + if (count > ARCMSR_API_DATA_BUFLEN)
> return -EINVAL;
> /* do message unit write. */
> ptmpuserbuffer = (uint8_t *)buf;
> user_len = (int32_t)count;
> spin_lock_irqsave(&acb->wqbuffer_lock, flags);
> - wqbuf_lastindex = acb->wqbuf_lastindex;
> - wqbuf_firstindex = acb->wqbuf_firstindex;
> - if (wqbuf_lastindex != wqbuf_firstindex) {
> + wqbuf_putIndex = acb->wqbuf_putIndex;
> + wqbuf_getIndex = acb->wqbuf_getIndex;
> + if (wqbuf_putIndex != wqbuf_getIndex) {
> arcmsr_write_ioctldata2iop(acb);
> spin_unlock_irqrestore(&acb->wqbuffer_lock, flags);
> return 0; /*need retry*/
> } else {
> - my_empty_len = (wqbuf_firstindex-wqbuf_lastindex - 1)
> - &(ARCMSR_MAX_QBUFFER - 1);
> + my_empty_len = ARCMSR_MAX_QBUFFER - 1;
This^ doesn't look like like an rename can you explain?
Let us stop here, or we end in an endless loop of corrections. The original 13/17
you are trying to improve here was at least without bugs (better said I haven't noticed) so
improving it now only complicates the process.
My suggestion is -let us skip this patch and focus only on fixing the spinlock problem found in 16/17.
OKay?
Cheers,
Tomas
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] arcmsr: simplify ioctl data read/write
2014-09-12 13:34 ` Tomas Henzl
@ 2014-09-15 2:56 ` Ching Huang
2014-09-15 10:25 ` Tomas Henzl
0 siblings, 1 reply; 7+ messages in thread
From: Ching Huang @ 2014-09-15 2:56 UTC (permalink / raw)
To: Tomas Henzl
Cc: hch, jbottomley, dan.carpenter, agordeev, linux-scsi, linux-kernel
On Fri, 2014-09-12 at 15:34 +0200, Tomas Henzl wrote:
> On 09/12/2014 09:29 AM, Ching Huang wrote:
> > From: Ching Huang <ching2048@areca.com.tw>
> >
> > This patch is to modify previous patch 13/17 and it is relative to
> > http://git.infradead.org/users/hch/scsi-queue.git/tree/arcmsr-for-3.18:/drivers/scsi/arcmsr
> >
> > change in v4:
> > 1. for readability, rename firstindex to getIndex, rename lastindex to putIndex
> For some reason, the names head+tail areusual for a circular buffer.
> But let us ignore the names, I don't care.
> > 2. define ARCMSR_API_DATA_BUFLEN as 1032
> > 3. simplify ioctl data read by macro CIRC_CNT_TO_END and CIRC_CNT
> It's definitely better when you post renames and other non-functional changes in separate
> patches, it's easier for the reviewer.
> >
> > Signed-off-by: Ching Huang <ching2048@areca.com.tw>
> > ---
> >
> > diff -uprN a/drivers/scsi/arcmsr/arcmsr_attr.c b/drivers/scsi/arcmsr/arcmsr_attr.c
> > --- a/drivers/scsi/arcmsr/arcmsr_attr.c 2014-08-21 12:14:27.000000000 +0800
> > +++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-09-12 15:18:46.659125000 +0800
> > @@ -50,6 +50,7 @@
> > #include <linux/errno.h>
> > #include <linux/delay.h>
> > #include <linux/pci.h>
> > +#include <linux/circ_buf.h>
> >
> > #include <scsi/scsi_cmnd.h>
> > #include <scsi/scsi_device.h>
> > @@ -68,7 +69,7 @@ static ssize_t arcmsr_sysfs_iop_message_
> > struct device *dev = container_of(kobj,struct device,kobj);
> > struct Scsi_Host *host = class_to_shost(dev);
> > struct AdapterControlBlock *acb = (struct AdapterControlBlock *) host->hostdata;
> > - uint8_t *pQbuffer,*ptmpQbuffer;
> > + uint8_t *ptmpQbuffer;
> > int32_t allxfer_len = 0;
> > unsigned long flags;
> >
> > @@ -78,57 +79,22 @@ static ssize_t arcmsr_sysfs_iop_message_
> > /* do message unit read. */
> > ptmpQbuffer = (uint8_t *)buf;
> > spin_lock_irqsave(&acb->rqbuffer_lock, flags);
> > - if (acb->rqbuf_firstindex != acb->rqbuf_lastindex) {
> > - pQbuffer = &acb->rqbuffer[acb->rqbuf_firstindex];
> > - if (acb->rqbuf_firstindex > acb->rqbuf_lastindex) {
> > - if ((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex) >= 1032) {
> > - memcpy(ptmpQbuffer, pQbuffer, 1032);
> > - acb->rqbuf_firstindex += 1032;
> > - acb->rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
> > - allxfer_len = 1032;
> > - } else {
> > - if (((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex)
> > - + acb->rqbuf_lastindex) > 1032) {
> > - memcpy(ptmpQbuffer, pQbuffer,
> > - ARCMSR_MAX_QBUFFER
> > - - acb->rqbuf_firstindex);
> > - ptmpQbuffer += ARCMSR_MAX_QBUFFER
> > - - acb->rqbuf_firstindex;
> > - memcpy(ptmpQbuffer, acb->rqbuffer, 1032
> > - - (ARCMSR_MAX_QBUFFER -
> > - acb->rqbuf_firstindex));
> > - acb->rqbuf_firstindex = 1032 -
> > - (ARCMSR_MAX_QBUFFER -
> > - acb->rqbuf_firstindex);
> > - allxfer_len = 1032;
> > - } else {
> > - memcpy(ptmpQbuffer, pQbuffer,
> > - ARCMSR_MAX_QBUFFER -
> > - acb->rqbuf_firstindex);
> > - ptmpQbuffer += ARCMSR_MAX_QBUFFER -
> > - acb->rqbuf_firstindex;
> > - memcpy(ptmpQbuffer, acb->rqbuffer,
> > - acb->rqbuf_lastindex);
> > - allxfer_len = ARCMSR_MAX_QBUFFER -
> > - acb->rqbuf_firstindex +
> > - acb->rqbuf_lastindex;
> > - acb->rqbuf_firstindex =
> > - acb->rqbuf_lastindex;
> > - }
> > - }
> > - } else {
> > - if ((acb->rqbuf_lastindex - acb->rqbuf_firstindex) > 1032) {
> > - memcpy(ptmpQbuffer, pQbuffer, 1032);
> > - acb->rqbuf_firstindex += 1032;
> > - allxfer_len = 1032;
> > - } else {
> > - memcpy(ptmpQbuffer, pQbuffer, acb->rqbuf_lastindex
> > - - acb->rqbuf_firstindex);
> > - allxfer_len = acb->rqbuf_lastindex -
> > - acb->rqbuf_firstindex;
> > - acb->rqbuf_firstindex = acb->rqbuf_lastindex;
> > - }
> > + if (acb->rqbuf_getIndex != acb->rqbuf_putIndex) {
> > + unsigned int tail = acb->rqbuf_getIndex;
> > + unsigned int head = acb->rqbuf_putIndex;
> > + unsigned int cnt_to_end = CIRC_CNT_TO_END(head, tail, ARCMSR_MAX_QBUFFER);
> > +
> > + allxfer_len = CIRC_CNT(head, tail, ARCMSR_MAX_QBUFFER);
> > + if (allxfer_len > ARCMSR_API_DATA_BUFLEN)
> > + allxfer_len = ARCMSR_API_DATA_BUFLEN;
> > +
> > + if (allxfer_len <= cnt_to_end)
> > + memcpy(ptmpQbuffer, acb->rqbuffer + tail, allxfer_len);
> > + else {
> > + memcpy(ptmpQbuffer, acb->rqbuffer + tail, cnt_to_end);
> > + memcpy(ptmpQbuffer + cnt_to_end, acb->rqbuffer, allxfer_len - cnt_to_end);
> > }
> > + acb->rqbuf_getIndex = (acb->rqbuf_getIndex + allxfer_len) % ARCMSR_MAX_QBUFFER;
> > }
> > if (acb->acb_flags & ACB_F_IOPDATA_OVERFLOW) {
> > struct QBUFFER __iomem *prbuffer;
> > @@ -150,33 +116,32 @@ static ssize_t arcmsr_sysfs_iop_message_
> > struct device *dev = container_of(kobj,struct device,kobj);
> > struct Scsi_Host *host = class_to_shost(dev);
> > struct AdapterControlBlock *acb = (struct AdapterControlBlock *) host->hostdata;
> > - int32_t my_empty_len, user_len, wqbuf_firstindex, wqbuf_lastindex;
> > + int32_t my_empty_len, user_len, wqbuf_getIndex, wqbuf_putIndex;
> > uint8_t *pQbuffer, *ptmpuserbuffer;
> > unsigned long flags;
> >
> > if (!capable(CAP_SYS_ADMIN))
> > return -EACCES;
> > - if (count > 1032)
> > + if (count > ARCMSR_API_DATA_BUFLEN)
> > return -EINVAL;
> > /* do message unit write. */
> > ptmpuserbuffer = (uint8_t *)buf;
> > user_len = (int32_t)count;
> > spin_lock_irqsave(&acb->wqbuffer_lock, flags);
> > - wqbuf_lastindex = acb->wqbuf_lastindex;
> > - wqbuf_firstindex = acb->wqbuf_firstindex;
> > - if (wqbuf_lastindex != wqbuf_firstindex) {
> > + wqbuf_putIndex = acb->wqbuf_putIndex;
> > + wqbuf_getIndex = acb->wqbuf_getIndex;
> > + if (wqbuf_putIndex != wqbuf_getIndex) {
> > arcmsr_write_ioctldata2iop(acb);
> > spin_unlock_irqrestore(&acb->wqbuffer_lock, flags);
> > return 0; /*need retry*/
> > } else {
> > - my_empty_len = (wqbuf_firstindex-wqbuf_lastindex - 1)
> > - &(ARCMSR_MAX_QBUFFER - 1);
> > + my_empty_len = ARCMSR_MAX_QBUFFER - 1;
> This^ doesn't look like like an rename can you explain?
It is just a code simplification.
>
> Let us stop here, or we end in an endless loop of corrections. The original 13/17
> you are trying to improve here was at least without bugs (better said I haven't noticed) so
> improving it now only complicates the process.
> My suggestion is -let us skip this patch and focus only on fixing the spinlock problem found in 16/17.
> OKay?
Agree.
>
> Cheers,
> Tomas
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] arcmsr: simplify ioctl data read/write
2014-09-15 2:56 ` Ching Huang
@ 2014-09-15 10:25 ` Tomas Henzl
2014-09-15 10:36 ` Ching Huang
0 siblings, 1 reply; 7+ messages in thread
From: Tomas Henzl @ 2014-09-15 10:25 UTC (permalink / raw)
To: Ching Huang
Cc: hch, jbottomley, dan.carpenter, agordeev, linux-scsi, linux-kernel
On 09/15/2014 04:56 AM, Ching Huang wrote:
> On Fri, 2014-09-12 at 15:34 +0200, Tomas Henzl wrote:
>> On 09/12/2014 09:29 AM, Ching Huang wrote:
>>> From: Ching Huang <ching2048@areca.com.tw>
>>>
>>> This patch is to modify previous patch 13/17 and it is relative to
>>> http://git.infradead.org/users/hch/scsi-queue.git/tree/arcmsr-for-3.18:/drivers/scsi/arcmsr
>>>
>>> change in v4:
>>> 1. for readability, rename firstindex to getIndex, rename lastindex to putIndex
>> For some reason, the names head+tail areusual for a circular buffer.
>> But let us ignore the names, I don't care.
>>> 2. define ARCMSR_API_DATA_BUFLEN as 1032
>>> 3. simplify ioctl data read by macro CIRC_CNT_TO_END and CIRC_CNT
>> It's definitely better when you post renames and other non-functional changes in separate
>> patches, it's easier for the reviewer.
>>> Signed-off-by: Ching Huang <ching2048@areca.com.tw>
>>> ---
>>>
>>> diff -uprN a/drivers/scsi/arcmsr/arcmsr_attr.c b/drivers/scsi/arcmsr/arcmsr_attr.c
>>> --- a/drivers/scsi/arcmsr/arcmsr_attr.c 2014-08-21 12:14:27.000000000 +0800
>>> +++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-09-12 15:18:46.659125000 +0800
>>> @@ -50,6 +50,7 @@
>>> #include <linux/errno.h>
>>> #include <linux/delay.h>
>>> #include <linux/pci.h>
>>> +#include <linux/circ_buf.h>
>>>
>>> #include <scsi/scsi_cmnd.h>
>>> #include <scsi/scsi_device.h>
>>> @@ -68,7 +69,7 @@ static ssize_t arcmsr_sysfs_iop_message_
>>> struct device *dev = container_of(kobj,struct device,kobj);
>>> struct Scsi_Host *host = class_to_shost(dev);
>>> struct AdapterControlBlock *acb = (struct AdapterControlBlock *) host->hostdata;
>>> - uint8_t *pQbuffer,*ptmpQbuffer;
>>> + uint8_t *ptmpQbuffer;
>>> int32_t allxfer_len = 0;
>>> unsigned long flags;
>>>
>>> @@ -78,57 +79,22 @@ static ssize_t arcmsr_sysfs_iop_message_
>>> /* do message unit read. */
>>> ptmpQbuffer = (uint8_t *)buf;
>>> spin_lock_irqsave(&acb->rqbuffer_lock, flags);
>>> - if (acb->rqbuf_firstindex != acb->rqbuf_lastindex) {
>>> - pQbuffer = &acb->rqbuffer[acb->rqbuf_firstindex];
>>> - if (acb->rqbuf_firstindex > acb->rqbuf_lastindex) {
>>> - if ((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex) >= 1032) {
>>> - memcpy(ptmpQbuffer, pQbuffer, 1032);
>>> - acb->rqbuf_firstindex += 1032;
>>> - acb->rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
>>> - allxfer_len = 1032;
>>> - } else {
>>> - if (((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex)
>>> - + acb->rqbuf_lastindex) > 1032) {
>>> - memcpy(ptmpQbuffer, pQbuffer,
>>> - ARCMSR_MAX_QBUFFER
>>> - - acb->rqbuf_firstindex);
>>> - ptmpQbuffer += ARCMSR_MAX_QBUFFER
>>> - - acb->rqbuf_firstindex;
>>> - memcpy(ptmpQbuffer, acb->rqbuffer, 1032
>>> - - (ARCMSR_MAX_QBUFFER -
>>> - acb->rqbuf_firstindex));
>>> - acb->rqbuf_firstindex = 1032 -
>>> - (ARCMSR_MAX_QBUFFER -
>>> - acb->rqbuf_firstindex);
>>> - allxfer_len = 1032;
>>> - } else {
>>> - memcpy(ptmpQbuffer, pQbuffer,
>>> - ARCMSR_MAX_QBUFFER -
>>> - acb->rqbuf_firstindex);
>>> - ptmpQbuffer += ARCMSR_MAX_QBUFFER -
>>> - acb->rqbuf_firstindex;
>>> - memcpy(ptmpQbuffer, acb->rqbuffer,
>>> - acb->rqbuf_lastindex);
>>> - allxfer_len = ARCMSR_MAX_QBUFFER -
>>> - acb->rqbuf_firstindex +
>>> - acb->rqbuf_lastindex;
>>> - acb->rqbuf_firstindex =
>>> - acb->rqbuf_lastindex;
>>> - }
>>> - }
>>> - } else {
>>> - if ((acb->rqbuf_lastindex - acb->rqbuf_firstindex) > 1032) {
>>> - memcpy(ptmpQbuffer, pQbuffer, 1032);
>>> - acb->rqbuf_firstindex += 1032;
>>> - allxfer_len = 1032;
>>> - } else {
>>> - memcpy(ptmpQbuffer, pQbuffer, acb->rqbuf_lastindex
>>> - - acb->rqbuf_firstindex);
>>> - allxfer_len = acb->rqbuf_lastindex -
>>> - acb->rqbuf_firstindex;
>>> - acb->rqbuf_firstindex = acb->rqbuf_lastindex;
>>> - }
>>> + if (acb->rqbuf_getIndex != acb->rqbuf_putIndex) {
>>> + unsigned int tail = acb->rqbuf_getIndex;
>>> + unsigned int head = acb->rqbuf_putIndex;
>>> + unsigned int cnt_to_end = CIRC_CNT_TO_END(head, tail, ARCMSR_MAX_QBUFFER);
>>> +
>>> + allxfer_len = CIRC_CNT(head, tail, ARCMSR_MAX_QBUFFER);
>>> + if (allxfer_len > ARCMSR_API_DATA_BUFLEN)
>>> + allxfer_len = ARCMSR_API_DATA_BUFLEN;
>>> +
>>> + if (allxfer_len <= cnt_to_end)
>>> + memcpy(ptmpQbuffer, acb->rqbuffer + tail, allxfer_len);
>>> + else {
>>> + memcpy(ptmpQbuffer, acb->rqbuffer + tail, cnt_to_end);
>>> + memcpy(ptmpQbuffer + cnt_to_end, acb->rqbuffer, allxfer_len - cnt_to_end);
>>> }
>>> + acb->rqbuf_getIndex = (acb->rqbuf_getIndex + allxfer_len) % ARCMSR_MAX_QBUFFER;
>>> }
>>> if (acb->acb_flags & ACB_F_IOPDATA_OVERFLOW) {
>>> struct QBUFFER __iomem *prbuffer;
>>> @@ -150,33 +116,32 @@ static ssize_t arcmsr_sysfs_iop_message_
>>> struct device *dev = container_of(kobj,struct device,kobj);
>>> struct Scsi_Host *host = class_to_shost(dev);
>>> struct AdapterControlBlock *acb = (struct AdapterControlBlock *) host->hostdata;
>>> - int32_t my_empty_len, user_len, wqbuf_firstindex, wqbuf_lastindex;
>>> + int32_t my_empty_len, user_len, wqbuf_getIndex, wqbuf_putIndex;
>>> uint8_t *pQbuffer, *ptmpuserbuffer;
>>> unsigned long flags;
>>>
>>> if (!capable(CAP_SYS_ADMIN))
>>> return -EACCES;
>>> - if (count > 1032)
>>> + if (count > ARCMSR_API_DATA_BUFLEN)
>>> return -EINVAL;
>>> /* do message unit write. */
>>> ptmpuserbuffer = (uint8_t *)buf;
>>> user_len = (int32_t)count;
>>> spin_lock_irqsave(&acb->wqbuffer_lock, flags);
>>> - wqbuf_lastindex = acb->wqbuf_lastindex;
>>> - wqbuf_firstindex = acb->wqbuf_firstindex;
>>> - if (wqbuf_lastindex != wqbuf_firstindex) {
>>> + wqbuf_putIndex = acb->wqbuf_putIndex;
>>> + wqbuf_getIndex = acb->wqbuf_getIndex;
>>> + if (wqbuf_putIndex != wqbuf_getIndex) {
>>> arcmsr_write_ioctldata2iop(acb);
>>> spin_unlock_irqrestore(&acb->wqbuffer_lock, flags);
>>> return 0; /*need retry*/
>>> } else {
>>> - my_empty_len = (wqbuf_firstindex-wqbuf_lastindex - 1)
>>> - &(ARCMSR_MAX_QBUFFER - 1);
>>> + my_empty_len = ARCMSR_MAX_QBUFFER - 1;
>> This^ doesn't look like like an rename can you explain?
> It is just a code simplification.
Yes it is of some kind. But I think it's also a bug, because you have replaced
'firstindex - lastindex' with nothing.
>> Let us stop here, or we end in an endless loop of corrections. The original 13/17
>> you are trying to improve here was at least without bugs (better said I haven't noticed) so
>> improving it now only complicates the process.
>> My suggestion is -let us skip this patch and focus only on fixing the spinlock problem found in 16/17.
>> OKay?
> Agree.
>> Cheers,
>> Tomas
>>
>
> --
> 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] 7+ messages in thread
* Re: [PATCH v4 1/2] arcmsr: simplify ioctl data read/write
2014-09-15 10:25 ` Tomas Henzl
@ 2014-09-15 10:36 ` Ching Huang
2014-09-15 11:50 ` Tomas Henzl
0 siblings, 1 reply; 7+ messages in thread
From: Ching Huang @ 2014-09-15 10:36 UTC (permalink / raw)
To: Tomas Henzl
Cc: hch, jbottomley, dan.carpenter, agordeev, linux-scsi, linux-kernel
On Mon, 2014-09-15 at 12:25 +0200, Tomas Henzl wrote:
> On 09/15/2014 04:56 AM, Ching Huang wrote:
> > On Fri, 2014-09-12 at 15:34 +0200, Tomas Henzl wrote:
> >> On 09/12/2014 09:29 AM, Ching Huang wrote:
> >>> From: Ching Huang <ching2048@areca.com.tw>
> >>>
> >>> This patch is to modify previous patch 13/17 and it is relative to
> >>> http://git.infradead.org/users/hch/scsi-queue.git/tree/arcmsr-for-3.18:/drivers/scsi/arcmsr
> >>>
> >>> change in v4:
> >>> 1. for readability, rename firstindex to getIndex, rename lastindex to putIndex
> >> For some reason, the names head+tail areusual for a circular buffer.
> >> But let us ignore the names, I don't care.
> >>> 2. define ARCMSR_API_DATA_BUFLEN as 1032
> >>> 3. simplify ioctl data read by macro CIRC_CNT_TO_END and CIRC_CNT
> >> It's definitely better when you post renames and other non-functional changes in separate
> >> patches, it's easier for the reviewer.
> >>> Signed-off-by: Ching Huang <ching2048@areca.com.tw>
> >>> ---
> >>>
> >>> diff -uprN a/drivers/scsi/arcmsr/arcmsr_attr.c b/drivers/scsi/arcmsr/arcmsr_attr.c
> >>> --- a/drivers/scsi/arcmsr/arcmsr_attr.c 2014-08-21 12:14:27.000000000 +0800
> >>> +++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-09-12 15:18:46.659125000 +0800
> >>> @@ -50,6 +50,7 @@
> >>> #include <linux/errno.h>
> >>> #include <linux/delay.h>
> >>> #include <linux/pci.h>
> >>> +#include <linux/circ_buf.h>
> >>>
> >>> #include <scsi/scsi_cmnd.h>
> >>> #include <scsi/scsi_device.h>
> >>> @@ -68,7 +69,7 @@ static ssize_t arcmsr_sysfs_iop_message_
> >>> struct device *dev = container_of(kobj,struct device,kobj);
> >>> struct Scsi_Host *host = class_to_shost(dev);
> >>> struct AdapterControlBlock *acb = (struct AdapterControlBlock *) host->hostdata;
> >>> - uint8_t *pQbuffer,*ptmpQbuffer;
> >>> + uint8_t *ptmpQbuffer;
> >>> int32_t allxfer_len = 0;
> >>> unsigned long flags;
> >>>
> >>> @@ -78,57 +79,22 @@ static ssize_t arcmsr_sysfs_iop_message_
> >>> /* do message unit read. */
> >>> ptmpQbuffer = (uint8_t *)buf;
> >>> spin_lock_irqsave(&acb->rqbuffer_lock, flags);
> >>> - if (acb->rqbuf_firstindex != acb->rqbuf_lastindex) {
> >>> - pQbuffer = &acb->rqbuffer[acb->rqbuf_firstindex];
> >>> - if (acb->rqbuf_firstindex > acb->rqbuf_lastindex) {
> >>> - if ((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex) >= 1032) {
> >>> - memcpy(ptmpQbuffer, pQbuffer, 1032);
> >>> - acb->rqbuf_firstindex += 1032;
> >>> - acb->rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
> >>> - allxfer_len = 1032;
> >>> - } else {
> >>> - if (((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex)
> >>> - + acb->rqbuf_lastindex) > 1032) {
> >>> - memcpy(ptmpQbuffer, pQbuffer,
> >>> - ARCMSR_MAX_QBUFFER
> >>> - - acb->rqbuf_firstindex);
> >>> - ptmpQbuffer += ARCMSR_MAX_QBUFFER
> >>> - - acb->rqbuf_firstindex;
> >>> - memcpy(ptmpQbuffer, acb->rqbuffer, 1032
> >>> - - (ARCMSR_MAX_QBUFFER -
> >>> - acb->rqbuf_firstindex));
> >>> - acb->rqbuf_firstindex = 1032 -
> >>> - (ARCMSR_MAX_QBUFFER -
> >>> - acb->rqbuf_firstindex);
> >>> - allxfer_len = 1032;
> >>> - } else {
> >>> - memcpy(ptmpQbuffer, pQbuffer,
> >>> - ARCMSR_MAX_QBUFFER -
> >>> - acb->rqbuf_firstindex);
> >>> - ptmpQbuffer += ARCMSR_MAX_QBUFFER -
> >>> - acb->rqbuf_firstindex;
> >>> - memcpy(ptmpQbuffer, acb->rqbuffer,
> >>> - acb->rqbuf_lastindex);
> >>> - allxfer_len = ARCMSR_MAX_QBUFFER -
> >>> - acb->rqbuf_firstindex +
> >>> - acb->rqbuf_lastindex;
> >>> - acb->rqbuf_firstindex =
> >>> - acb->rqbuf_lastindex;
> >>> - }
> >>> - }
> >>> - } else {
> >>> - if ((acb->rqbuf_lastindex - acb->rqbuf_firstindex) > 1032) {
> >>> - memcpy(ptmpQbuffer, pQbuffer, 1032);
> >>> - acb->rqbuf_firstindex += 1032;
> >>> - allxfer_len = 1032;
> >>> - } else {
> >>> - memcpy(ptmpQbuffer, pQbuffer, acb->rqbuf_lastindex
> >>> - - acb->rqbuf_firstindex);
> >>> - allxfer_len = acb->rqbuf_lastindex -
> >>> - acb->rqbuf_firstindex;
> >>> - acb->rqbuf_firstindex = acb->rqbuf_lastindex;
> >>> - }
> >>> + if (acb->rqbuf_getIndex != acb->rqbuf_putIndex) {
> >>> + unsigned int tail = acb->rqbuf_getIndex;
> >>> + unsigned int head = acb->rqbuf_putIndex;
> >>> + unsigned int cnt_to_end = CIRC_CNT_TO_END(head, tail, ARCMSR_MAX_QBUFFER);
> >>> +
> >>> + allxfer_len = CIRC_CNT(head, tail, ARCMSR_MAX_QBUFFER);
> >>> + if (allxfer_len > ARCMSR_API_DATA_BUFLEN)
> >>> + allxfer_len = ARCMSR_API_DATA_BUFLEN;
> >>> +
> >>> + if (allxfer_len <= cnt_to_end)
> >>> + memcpy(ptmpQbuffer, acb->rqbuffer + tail, allxfer_len);
> >>> + else {
> >>> + memcpy(ptmpQbuffer, acb->rqbuffer + tail, cnt_to_end);
> >>> + memcpy(ptmpQbuffer + cnt_to_end, acb->rqbuffer, allxfer_len - cnt_to_end);
> >>> }
> >>> + acb->rqbuf_getIndex = (acb->rqbuf_getIndex + allxfer_len) % ARCMSR_MAX_QBUFFER;
> >>> }
> >>> if (acb->acb_flags & ACB_F_IOPDATA_OVERFLOW) {
> >>> struct QBUFFER __iomem *prbuffer;
> >>> @@ -150,33 +116,32 @@ static ssize_t arcmsr_sysfs_iop_message_
> >>> struct device *dev = container_of(kobj,struct device,kobj);
> >>> struct Scsi_Host *host = class_to_shost(dev);
> >>> struct AdapterControlBlock *acb = (struct AdapterControlBlock *) host->hostdata;
> >>> - int32_t my_empty_len, user_len, wqbuf_firstindex, wqbuf_lastindex;
> >>> + int32_t my_empty_len, user_len, wqbuf_getIndex, wqbuf_putIndex;
> >>> uint8_t *pQbuffer, *ptmpuserbuffer;
> >>> unsigned long flags;
> >>>
> >>> if (!capable(CAP_SYS_ADMIN))
> >>> return -EACCES;
> >>> - if (count > 1032)
> >>> + if (count > ARCMSR_API_DATA_BUFLEN)
> >>> return -EINVAL;
> >>> /* do message unit write. */
> >>> ptmpuserbuffer = (uint8_t *)buf;
> >>> user_len = (int32_t)count;
> >>> spin_lock_irqsave(&acb->wqbuffer_lock, flags);
> >>> - wqbuf_lastindex = acb->wqbuf_lastindex;
> >>> - wqbuf_firstindex = acb->wqbuf_firstindex;
> >>> - if (wqbuf_lastindex != wqbuf_firstindex) {
> >>> + wqbuf_putIndex = acb->wqbuf_putIndex;
> >>> + wqbuf_getIndex = acb->wqbuf_getIndex;
> >>> + if (wqbuf_putIndex != wqbuf_getIndex) {
> >>> arcmsr_write_ioctldata2iop(acb);
> >>> spin_unlock_irqrestore(&acb->wqbuffer_lock, flags);
> >>> return 0; /*need retry*/
> >>> } else {
> >>> - my_empty_len = (wqbuf_firstindex-wqbuf_lastindex - 1)
> >>> - &(ARCMSR_MAX_QBUFFER - 1);
> >>> + my_empty_len = ARCMSR_MAX_QBUFFER - 1;
> >> This^ doesn't look like like an rename can you explain?
> > It is just a code simplification.
>
> Yes it is of some kind. But I think it's also a bug, because you have replaced
> 'firstindex - lastindex' with nothing.
It will be not a bug. At here, firstindex == lastindex, so firstindex-lastindex is 0
>
> >> Let us stop here, or we end in an endless loop of corrections. The original 13/17
> >> you are trying to improve here was at least without bugs (better said I haven't noticed) so
> >> improving it now only complicates the process.
> >> My suggestion is -let us skip this patch and focus only on fixing the spinlock problem found in 16/17.
> >> OKay?
> > Agree.
> >> Cheers,
> >> Tomas
> >>
> >
> > --
> > 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] 7+ messages in thread
* Re: [PATCH v4 1/2] arcmsr: simplify ioctl data read/write
2014-09-15 10:36 ` Ching Huang
@ 2014-09-15 11:50 ` Tomas Henzl
2014-09-15 12:03 ` Ching Huang
0 siblings, 1 reply; 7+ messages in thread
From: Tomas Henzl @ 2014-09-15 11:50 UTC (permalink / raw)
To: Ching Huang
Cc: hch, jbottomley, dan.carpenter, agordeev, linux-scsi, linux-kernel
On 09/15/2014 12:36 PM, Ching Huang wrote:
> On Mon, 2014-09-15 at 12:25 +0200, Tomas Henzl wrote:
>> On 09/15/2014 04:56 AM, Ching Huang wrote:
>>> On Fri, 2014-09-12 at 15:34 +0200, Tomas Henzl wrote:
>>>> On 09/12/2014 09:29 AM, Ching Huang wrote:
>>>>> From: Ching Huang <ching2048@areca.com.tw>
>>>>>
>>>>> This patch is to modify previous patch 13/17 and it is relative to
>>>>> http://git.infradead.org/users/hch/scsi-queue.git/tree/arcmsr-for-3.18:/drivers/scsi/arcmsr
>>>>>
>>>>> change in v4:
>>>>> 1. for readability, rename firstindex to getIndex, rename lastindex to putIndex
>>>> For some reason, the names head+tail areusual for a circular buffer.
>>>> But let us ignore the names, I don't care.
>>>>> 2. define ARCMSR_API_DATA_BUFLEN as 1032
>>>>> 3. simplify ioctl data read by macro CIRC_CNT_TO_END and CIRC_CNT
>>>> It's definitely better when you post renames and other non-functional changes in separate
>>>> patches, it's easier for the reviewer.
>>>>> Signed-off-by: Ching Huang <ching2048@areca.com.tw>
>>>>> ---
>>>>>
>>>>> diff -uprN a/drivers/scsi/arcmsr/arcmsr_attr.c b/drivers/scsi/arcmsr/arcmsr_attr.c
>>>>> --- a/drivers/scsi/arcmsr/arcmsr_attr.c 2014-08-21 12:14:27.000000000 +0800
>>>>> +++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-09-12 15:18:46.659125000 +0800
>>>>> @@ -50,6 +50,7 @@
>>>>> #include <linux/errno.h>
>>>>> #include <linux/delay.h>
>>>>> #include <linux/pci.h>
>>>>> +#include <linux/circ_buf.h>
>>>>>
>>>>> #include <scsi/scsi_cmnd.h>
>>>>> #include <scsi/scsi_device.h>
>>>>> @@ -68,7 +69,7 @@ static ssize_t arcmsr_sysfs_iop_message_
>>>>> struct device *dev = container_of(kobj,struct device,kobj);
>>>>> struct Scsi_Host *host = class_to_shost(dev);
>>>>> struct AdapterControlBlock *acb = (struct AdapterControlBlock *) host->hostdata;
>>>>> - uint8_t *pQbuffer,*ptmpQbuffer;
>>>>> + uint8_t *ptmpQbuffer;
>>>>> int32_t allxfer_len = 0;
>>>>> unsigned long flags;
>>>>>
>>>>> @@ -78,57 +79,22 @@ static ssize_t arcmsr_sysfs_iop_message_
>>>>> /* do message unit read. */
>>>>> ptmpQbuffer = (uint8_t *)buf;
>>>>> spin_lock_irqsave(&acb->rqbuffer_lock, flags);
>>>>> - if (acb->rqbuf_firstindex != acb->rqbuf_lastindex) {
>>>>> - pQbuffer = &acb->rqbuffer[acb->rqbuf_firstindex];
>>>>> - if (acb->rqbuf_firstindex > acb->rqbuf_lastindex) {
>>>>> - if ((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex) >= 1032) {
>>>>> - memcpy(ptmpQbuffer, pQbuffer, 1032);
>>>>> - acb->rqbuf_firstindex += 1032;
>>>>> - acb->rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
>>>>> - allxfer_len = 1032;
>>>>> - } else {
>>>>> - if (((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex)
>>>>> - + acb->rqbuf_lastindex) > 1032) {
>>>>> - memcpy(ptmpQbuffer, pQbuffer,
>>>>> - ARCMSR_MAX_QBUFFER
>>>>> - - acb->rqbuf_firstindex);
>>>>> - ptmpQbuffer += ARCMSR_MAX_QBUFFER
>>>>> - - acb->rqbuf_firstindex;
>>>>> - memcpy(ptmpQbuffer, acb->rqbuffer, 1032
>>>>> - - (ARCMSR_MAX_QBUFFER -
>>>>> - acb->rqbuf_firstindex));
>>>>> - acb->rqbuf_firstindex = 1032 -
>>>>> - (ARCMSR_MAX_QBUFFER -
>>>>> - acb->rqbuf_firstindex);
>>>>> - allxfer_len = 1032;
>>>>> - } else {
>>>>> - memcpy(ptmpQbuffer, pQbuffer,
>>>>> - ARCMSR_MAX_QBUFFER -
>>>>> - acb->rqbuf_firstindex);
>>>>> - ptmpQbuffer += ARCMSR_MAX_QBUFFER -
>>>>> - acb->rqbuf_firstindex;
>>>>> - memcpy(ptmpQbuffer, acb->rqbuffer,
>>>>> - acb->rqbuf_lastindex);
>>>>> - allxfer_len = ARCMSR_MAX_QBUFFER -
>>>>> - acb->rqbuf_firstindex +
>>>>> - acb->rqbuf_lastindex;
>>>>> - acb->rqbuf_firstindex =
>>>>> - acb->rqbuf_lastindex;
>>>>> - }
>>>>> - }
>>>>> - } else {
>>>>> - if ((acb->rqbuf_lastindex - acb->rqbuf_firstindex) > 1032) {
>>>>> - memcpy(ptmpQbuffer, pQbuffer, 1032);
>>>>> - acb->rqbuf_firstindex += 1032;
>>>>> - allxfer_len = 1032;
>>>>> - } else {
>>>>> - memcpy(ptmpQbuffer, pQbuffer, acb->rqbuf_lastindex
>>>>> - - acb->rqbuf_firstindex);
>>>>> - allxfer_len = acb->rqbuf_lastindex -
>>>>> - acb->rqbuf_firstindex;
>>>>> - acb->rqbuf_firstindex = acb->rqbuf_lastindex;
>>>>> - }
>>>>> + if (acb->rqbuf_getIndex != acb->rqbuf_putIndex) {
>>>>> + unsigned int tail = acb->rqbuf_getIndex;
>>>>> + unsigned int head = acb->rqbuf_putIndex;
>>>>> + unsigned int cnt_to_end = CIRC_CNT_TO_END(head, tail, ARCMSR_MAX_QBUFFER);
>>>>> +
>>>>> + allxfer_len = CIRC_CNT(head, tail, ARCMSR_MAX_QBUFFER);
>>>>> + if (allxfer_len > ARCMSR_API_DATA_BUFLEN)
>>>>> + allxfer_len = ARCMSR_API_DATA_BUFLEN;
>>>>> +
>>>>> + if (allxfer_len <= cnt_to_end)
>>>>> + memcpy(ptmpQbuffer, acb->rqbuffer + tail, allxfer_len);
>>>>> + else {
>>>>> + memcpy(ptmpQbuffer, acb->rqbuffer + tail, cnt_to_end);
>>>>> + memcpy(ptmpQbuffer + cnt_to_end, acb->rqbuffer, allxfer_len - cnt_to_end);
>>>>> }
>>>>> + acb->rqbuf_getIndex = (acb->rqbuf_getIndex + allxfer_len) % ARCMSR_MAX_QBUFFER;
>>>>> }
>>>>> if (acb->acb_flags & ACB_F_IOPDATA_OVERFLOW) {
>>>>> struct QBUFFER __iomem *prbuffer;
>>>>> @@ -150,33 +116,32 @@ static ssize_t arcmsr_sysfs_iop_message_
>>>>> struct device *dev = container_of(kobj,struct device,kobj);
>>>>> struct Scsi_Host *host = class_to_shost(dev);
>>>>> struct AdapterControlBlock *acb = (struct AdapterControlBlock *) host->hostdata;
>>>>> - int32_t my_empty_len, user_len, wqbuf_firstindex, wqbuf_lastindex;
>>>>> + int32_t my_empty_len, user_len, wqbuf_getIndex, wqbuf_putIndex;
>>>>> uint8_t *pQbuffer, *ptmpuserbuffer;
>>>>> unsigned long flags;
>>>>>
>>>>> if (!capable(CAP_SYS_ADMIN))
>>>>> return -EACCES;
>>>>> - if (count > 1032)
>>>>> + if (count > ARCMSR_API_DATA_BUFLEN)
>>>>> return -EINVAL;
>>>>> /* do message unit write. */
>>>>> ptmpuserbuffer = (uint8_t *)buf;
>>>>> user_len = (int32_t)count;
>>>>> spin_lock_irqsave(&acb->wqbuffer_lock, flags);
>>>>> - wqbuf_lastindex = acb->wqbuf_lastindex;
>>>>> - wqbuf_firstindex = acb->wqbuf_firstindex;
>>>>> - if (wqbuf_lastindex != wqbuf_firstindex) {
>>>>> + wqbuf_putIndex = acb->wqbuf_putIndex;
>>>>> + wqbuf_getIndex = acb->wqbuf_getIndex;
>>>>> + if (wqbuf_putIndex != wqbuf_getIndex) {
>>>>> arcmsr_write_ioctldata2iop(acb);
>>>>> spin_unlock_irqrestore(&acb->wqbuffer_lock, flags);
>>>>> return 0; /*need retry*/
>>>>> } else {
>>>>> - my_empty_len = (wqbuf_firstindex-wqbuf_lastindex - 1)
>>>>> - &(ARCMSR_MAX_QBUFFER - 1);
>>>>> + my_empty_len = ARCMSR_MAX_QBUFFER - 1;
>>>> This^ doesn't look like like an rename can you explain?
>>> It is just a code simplification.
>> Yes it is of some kind. But I think it's also a bug, because you have replaced
>> 'firstindex - lastindex' with nothing.
> It will be not a bug. At here, firstindex == lastindex, so firstindex-lastindex is 0
Thanks, I haven't seen it before. In that case because user_len is max 1032 and ARCMSR_MAX_QBUFFER is 4k
the next if statement '(my_empty_len >= user_len)' will be true for any user_len (<1032),
and it looks like you could remove my_empty_len completely.
>>>> Let us stop here, or we end in an endless loop of corrections. The original 13/17
>>>> you are trying to improve here was at least without bugs (better said I haven't noticed) so
>>>> improving it now only complicates the process.
>>>> My suggestion is -let us skip this patch and focus only on fixing the spinlock problem found in 16/17.
>>>> OKay?
>>> Agree.
>>>> Cheers,
>>>> Tomas
>>>>
>>> --
>>> 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] 7+ messages in thread
* Re: [PATCH v4 1/2] arcmsr: simplify ioctl data read/write
2014-09-15 11:50 ` Tomas Henzl
@ 2014-09-15 12:03 ` Ching Huang
0 siblings, 0 replies; 7+ messages in thread
From: Ching Huang @ 2014-09-15 12:03 UTC (permalink / raw)
To: Tomas Henzl
Cc: hch, jbottomley, dan.carpenter, agordeev, linux-scsi, linux-kernel
On Mon, 2014-09-15 at 13:50 +0200, Tomas Henzl wrote:
> On 09/15/2014 12:36 PM, Ching Huang wrote:
> > On Mon, 2014-09-15 at 12:25 +0200, Tomas Henzl wrote:
> >> On 09/15/2014 04:56 AM, Ching Huang wrote:
> >>> On Fri, 2014-09-12 at 15:34 +0200, Tomas Henzl wrote:
> >>>> On 09/12/2014 09:29 AM, Ching Huang wrote:
> >>>>> From: Ching Huang <ching2048@areca.com.tw>
> >>>>>
> >>>>> This patch is to modify previous patch 13/17 and it is relative to
> >>>>> http://git.infradead.org/users/hch/scsi-queue.git/tree/arcmsr-for-3.18:/drivers/scsi/arcmsr
> >>>>>
> >>>>> change in v4:
> >>>>> 1. for readability, rename firstindex to getIndex, rename lastindex to putIndex
> >>>> For some reason, the names head+tail areusual for a circular buffer.
> >>>> But let us ignore the names, I don't care.
> >>>>> 2. define ARCMSR_API_DATA_BUFLEN as 1032
> >>>>> 3. simplify ioctl data read by macro CIRC_CNT_TO_END and CIRC_CNT
> >>>> It's definitely better when you post renames and other non-functional changes in separate
> >>>> patches, it's easier for the reviewer.
> >>>>> Signed-off-by: Ching Huang <ching2048@areca.com.tw>
> >>>>> ---
> >>>>>
> >>>>> diff -uprN a/drivers/scsi/arcmsr/arcmsr_attr.c b/drivers/scsi/arcmsr/arcmsr_attr.c
> >>>>> --- a/drivers/scsi/arcmsr/arcmsr_attr.c 2014-08-21 12:14:27.000000000 +0800
> >>>>> +++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-09-12 15:18:46.659125000 +0800
> >>>>> @@ -50,6 +50,7 @@
> >>>>> #include <linux/errno.h>
> >>>>> #include <linux/delay.h>
> >>>>> #include <linux/pci.h>
> >>>>> +#include <linux/circ_buf.h>
> >>>>>
> >>>>> #include <scsi/scsi_cmnd.h>
> >>>>> #include <scsi/scsi_device.h>
> >>>>> @@ -68,7 +69,7 @@ static ssize_t arcmsr_sysfs_iop_message_
> >>>>> struct device *dev = container_of(kobj,struct device,kobj);
> >>>>> struct Scsi_Host *host = class_to_shost(dev);
> >>>>> struct AdapterControlBlock *acb = (struct AdapterControlBlock *) host->hostdata;
> >>>>> - uint8_t *pQbuffer,*ptmpQbuffer;
> >>>>> + uint8_t *ptmpQbuffer;
> >>>>> int32_t allxfer_len = 0;
> >>>>> unsigned long flags;
> >>>>>
> >>>>> @@ -78,57 +79,22 @@ static ssize_t arcmsr_sysfs_iop_message_
> >>>>> /* do message unit read. */
> >>>>> ptmpQbuffer = (uint8_t *)buf;
> >>>>> spin_lock_irqsave(&acb->rqbuffer_lock, flags);
> >>>>> - if (acb->rqbuf_firstindex != acb->rqbuf_lastindex) {
> >>>>> - pQbuffer = &acb->rqbuffer[acb->rqbuf_firstindex];
> >>>>> - if (acb->rqbuf_firstindex > acb->rqbuf_lastindex) {
> >>>>> - if ((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex) >= 1032) {
> >>>>> - memcpy(ptmpQbuffer, pQbuffer, 1032);
> >>>>> - acb->rqbuf_firstindex += 1032;
> >>>>> - acb->rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
> >>>>> - allxfer_len = 1032;
> >>>>> - } else {
> >>>>> - if (((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex)
> >>>>> - + acb->rqbuf_lastindex) > 1032) {
> >>>>> - memcpy(ptmpQbuffer, pQbuffer,
> >>>>> - ARCMSR_MAX_QBUFFER
> >>>>> - - acb->rqbuf_firstindex);
> >>>>> - ptmpQbuffer += ARCMSR_MAX_QBUFFER
> >>>>> - - acb->rqbuf_firstindex;
> >>>>> - memcpy(ptmpQbuffer, acb->rqbuffer, 1032
> >>>>> - - (ARCMSR_MAX_QBUFFER -
> >>>>> - acb->rqbuf_firstindex));
> >>>>> - acb->rqbuf_firstindex = 1032 -
> >>>>> - (ARCMSR_MAX_QBUFFER -
> >>>>> - acb->rqbuf_firstindex);
> >>>>> - allxfer_len = 1032;
> >>>>> - } else {
> >>>>> - memcpy(ptmpQbuffer, pQbuffer,
> >>>>> - ARCMSR_MAX_QBUFFER -
> >>>>> - acb->rqbuf_firstindex);
> >>>>> - ptmpQbuffer += ARCMSR_MAX_QBUFFER -
> >>>>> - acb->rqbuf_firstindex;
> >>>>> - memcpy(ptmpQbuffer, acb->rqbuffer,
> >>>>> - acb->rqbuf_lastindex);
> >>>>> - allxfer_len = ARCMSR_MAX_QBUFFER -
> >>>>> - acb->rqbuf_firstindex +
> >>>>> - acb->rqbuf_lastindex;
> >>>>> - acb->rqbuf_firstindex =
> >>>>> - acb->rqbuf_lastindex;
> >>>>> - }
> >>>>> - }
> >>>>> - } else {
> >>>>> - if ((acb->rqbuf_lastindex - acb->rqbuf_firstindex) > 1032) {
> >>>>> - memcpy(ptmpQbuffer, pQbuffer, 1032);
> >>>>> - acb->rqbuf_firstindex += 1032;
> >>>>> - allxfer_len = 1032;
> >>>>> - } else {
> >>>>> - memcpy(ptmpQbuffer, pQbuffer, acb->rqbuf_lastindex
> >>>>> - - acb->rqbuf_firstindex);
> >>>>> - allxfer_len = acb->rqbuf_lastindex -
> >>>>> - acb->rqbuf_firstindex;
> >>>>> - acb->rqbuf_firstindex = acb->rqbuf_lastindex;
> >>>>> - }
> >>>>> + if (acb->rqbuf_getIndex != acb->rqbuf_putIndex) {
> >>>>> + unsigned int tail = acb->rqbuf_getIndex;
> >>>>> + unsigned int head = acb->rqbuf_putIndex;
> >>>>> + unsigned int cnt_to_end = CIRC_CNT_TO_END(head, tail, ARCMSR_MAX_QBUFFER);
> >>>>> +
> >>>>> + allxfer_len = CIRC_CNT(head, tail, ARCMSR_MAX_QBUFFER);
> >>>>> + if (allxfer_len > ARCMSR_API_DATA_BUFLEN)
> >>>>> + allxfer_len = ARCMSR_API_DATA_BUFLEN;
> >>>>> +
> >>>>> + if (allxfer_len <= cnt_to_end)
> >>>>> + memcpy(ptmpQbuffer, acb->rqbuffer + tail, allxfer_len);
> >>>>> + else {
> >>>>> + memcpy(ptmpQbuffer, acb->rqbuffer + tail, cnt_to_end);
> >>>>> + memcpy(ptmpQbuffer + cnt_to_end, acb->rqbuffer, allxfer_len - cnt_to_end);
> >>>>> }
> >>>>> + acb->rqbuf_getIndex = (acb->rqbuf_getIndex + allxfer_len) % ARCMSR_MAX_QBUFFER;
> >>>>> }
> >>>>> if (acb->acb_flags & ACB_F_IOPDATA_OVERFLOW) {
> >>>>> struct QBUFFER __iomem *prbuffer;
> >>>>> @@ -150,33 +116,32 @@ static ssize_t arcmsr_sysfs_iop_message_
> >>>>> struct device *dev = container_of(kobj,struct device,kobj);
> >>>>> struct Scsi_Host *host = class_to_shost(dev);
> >>>>> struct AdapterControlBlock *acb = (struct AdapterControlBlock *) host->hostdata;
> >>>>> - int32_t my_empty_len, user_len, wqbuf_firstindex, wqbuf_lastindex;
> >>>>> + int32_t my_empty_len, user_len, wqbuf_getIndex, wqbuf_putIndex;
> >>>>> uint8_t *pQbuffer, *ptmpuserbuffer;
> >>>>> unsigned long flags;
> >>>>>
> >>>>> if (!capable(CAP_SYS_ADMIN))
> >>>>> return -EACCES;
> >>>>> - if (count > 1032)
> >>>>> + if (count > ARCMSR_API_DATA_BUFLEN)
> >>>>> return -EINVAL;
> >>>>> /* do message unit write. */
> >>>>> ptmpuserbuffer = (uint8_t *)buf;
> >>>>> user_len = (int32_t)count;
> >>>>> spin_lock_irqsave(&acb->wqbuffer_lock, flags);
> >>>>> - wqbuf_lastindex = acb->wqbuf_lastindex;
> >>>>> - wqbuf_firstindex = acb->wqbuf_firstindex;
> >>>>> - if (wqbuf_lastindex != wqbuf_firstindex) {
> >>>>> + wqbuf_putIndex = acb->wqbuf_putIndex;
> >>>>> + wqbuf_getIndex = acb->wqbuf_getIndex;
> >>>>> + if (wqbuf_putIndex != wqbuf_getIndex) {
> >>>>> arcmsr_write_ioctldata2iop(acb);
> >>>>> spin_unlock_irqrestore(&acb->wqbuffer_lock, flags);
> >>>>> return 0; /*need retry*/
> >>>>> } else {
> >>>>> - my_empty_len = (wqbuf_firstindex-wqbuf_lastindex - 1)
> >>>>> - &(ARCMSR_MAX_QBUFFER - 1);
> >>>>> + my_empty_len = ARCMSR_MAX_QBUFFER - 1;
> >>>> This^ doesn't look like like an rename can you explain?
> >>> It is just a code simplification.
> >> Yes it is of some kind. But I think it's also a bug, because you have replaced
> >> 'firstindex - lastindex' with nothing.
> > It will be not a bug. At here, firstindex == lastindex, so firstindex-lastindex is 0
>
> Thanks, I haven't seen it before. In that case because user_len is max 1032 and ARCMSR_MAX_QBUFFER is 4k
> the next if statement '(my_empty_len >= user_len)' will be true for any user_len (<1032),
> and it looks like you could remove my_empty_len completely.
Yes. You are right. thanks
>
> >>>> Let us stop here, or we end in an endless loop of corrections. The original 13/17
> >>>> you are trying to improve here was at least without bugs (better said I haven't noticed) so
> >>>> improving it now only complicates the process.
> >>>> My suggestion is -let us skip this patch and focus only on fixing the spinlock problem found in 16/17.
> >>>> OKay?
> >>> Agree.
> >>>> Cheers,
> >>>> Tomas
> >>>>
> >>> --
> >>> 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] 7+ messages in thread
end of thread, other threads:[~2014-09-15 12:04 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-12 7:29 [PATCH v4 1/2] arcmsr: simplify ioctl data read/write Ching Huang
2014-09-12 13:34 ` Tomas Henzl
2014-09-15 2:56 ` Ching Huang
2014-09-15 10:25 ` Tomas Henzl
2014-09-15 10:36 ` Ching Huang
2014-09-15 11:50 ` Tomas Henzl
2014-09-15 12:03 ` Ching Huang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).