Re: [PATCH v3 13/17] arcmsr: fix ioctl data read/write error for adapter type C
On Tue, 2014-08-26 at 15:20 +0200, Tomas Henzl wrote:
On 08/26/2014 10:27 PM, Ching Huang wrote:
On Mon, 2014-08-25 at 12:29 +0200, Tomas Henzl wrote:
On 08/25/2014 07:59 PM, Ching Huang wrote:
On Fri, 2014-08-22 at 18:00 +0200, Tomas Henzl wrote:
On 08/19/2014 09:17 AM, Ching Huang wrote:
From: Ching Huang ching2...@areca.com.tw
Rewrite ioctl entry and its relate function.
This patch fix ioctl data read/write error and change data I/O access
from byte to Dword.
Signed-off-by: Ching Huang ching2...@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-02-06 17:47:24.0
+0800
+++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-04-29 17:10:42.0
+0800
@@ -70,40 +70,75 @@ static ssize_t arcmsr_sysfs_iop_message_
struct AdapterControlBlock *acb = (struct AdapterControlBlock
*) host-hostdata;
uint8_t *pQbuffer,*ptmpQbuffer;
int32_t allxfer_len = 0;
+ unsigned long flags;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
/* do message unit read. */
ptmpQbuffer = (uint8_t *)buf;
- while ((acb-rqbuf_firstindex != acb-rqbuf_lastindex)
-(allxfer_len 1031)) {
+ spin_lock_irqsave(acb-rqbuffer_lock, flags);
+ if (acb-rqbuf_firstindex != acb-rqbuf_lastindex) {
Hi - does this condition (acb-rqbuf_firstindex == acb-rqbuf_lastindex)
mean we could just release
the spinlock and return ?
NO. We have to check the input buffer that may have message data come
from IOP.
pQbuffer = acb-rqbuffer[acb-rqbuf_firstindex];
- memcpy(ptmpQbuffer, pQbuffer, 1);
- acb-rqbuf_firstindex++;
- acb-rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
- ptmpQbuffer++;
- allxfer_len++;
+ 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));
This code looks like you were copying some data from a ring buffer,
in that case - shouldn't be acb-rqbuf_lastindex used instead of
firstindex?
Yes, there copying data from a ring buffer. firstindex and lastindex are
bad name. For readability, I rename the firstindex to getIndex,
lastindex to putIndex.
My comment is not about names, but in this path '(ARCMSR_MAX_QBUFFER -
acb-rqbuf_firstindex)+ acb-rqbuf_lastindex) 1032)'
you copy something twice and in both cases the 'firstindex' is used and
never the 'lastindex'.
Is this correct?
The firstindex is a get index and lastindex is a put index of a ring buffer.
At here, firstindex lastindex, so the data remain in buffer are
(ARCMSR_MAX_QBUFFER - acb-rqbuf_firstindex)+ acb-rqbuf_lastindex
Yes, it's correct, I misinterpreted the from value with the amount of bytes
to copy.
But well it's also still overcomplicated and I believe that a copy like this
could be
rearranged with just few lines of code as a result - have you looked at the
code I sent?
Let's go with this patch as it is otherwise we will never end, repost is not
needed because
of this and also not because of arcmsr_Read_iop_rqbuffer_in_DWORD.
I'll continue with reviewing the remaining patches.
tomas
I have test the code you sent. It works.
I will modify the code by your idea, then send the patch.
Thanks,
Ching
What does the 1032 mean is that a hw. limit, actually could you explain
the code
should do? Maybe I'm just wrong with my assumptions.
1032 is the API data buffer limitation.
Thanks,
Tomas
+ acb-rqbuf_firstindex = 1032 -
+ (ARCMSR_MAX_QBUFFER -
+ acb-rqbuf_firstindex);
Re: [PATCH v3 13/17] arcmsr: fix ioctl data read/write error for adapter type C
On 08/27/2014 10:19 PM, Ching Huang wrote:
On Tue, 2014-08-26 at 15:20 +0200, Tomas Henzl wrote:
On 08/26/2014 10:27 PM, Ching Huang wrote:
On Mon, 2014-08-25 at 12:29 +0200, Tomas Henzl wrote:
On 08/25/2014 07:59 PM, Ching Huang wrote:
On Fri, 2014-08-22 at 18:00 +0200, Tomas Henzl wrote:
On 08/19/2014 09:17 AM, Ching Huang wrote:
From: Ching Huang ching2...@areca.com.tw
Rewrite ioctl entry and its relate function.
This patch fix ioctl data read/write error and change data I/O access
from byte to Dword.
Signed-off-by: Ching Huang ching2...@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-02-06 17:47:24.0
+0800
+++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-04-29 17:10:42.0
+0800
@@ -70,40 +70,75 @@ static ssize_t arcmsr_sysfs_iop_message_
struct AdapterControlBlock *acb = (struct AdapterControlBlock
*) host-hostdata;
uint8_t *pQbuffer,*ptmpQbuffer;
int32_t allxfer_len = 0;
+ unsigned long flags;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
/* do message unit read. */
ptmpQbuffer = (uint8_t *)buf;
- while ((acb-rqbuf_firstindex != acb-rqbuf_lastindex)
-(allxfer_len 1031)) {
+ spin_lock_irqsave(acb-rqbuffer_lock, flags);
+ if (acb-rqbuf_firstindex != acb-rqbuf_lastindex) {
Hi - does this condition (acb-rqbuf_firstindex == acb-rqbuf_lastindex)
mean we could just release
the spinlock and return ?
NO. We have to check the input buffer that may have message data come
from IOP.
pQbuffer = acb-rqbuffer[acb-rqbuf_firstindex];
- memcpy(ptmpQbuffer, pQbuffer, 1);
- acb-rqbuf_firstindex++;
- acb-rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
- ptmpQbuffer++;
- allxfer_len++;
+ 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));
This code looks like you were copying some data from a ring buffer,
in that case - shouldn't be acb-rqbuf_lastindex used instead of
firstindex?
Yes, there copying data from a ring buffer. firstindex and lastindex are
bad name. For readability, I rename the firstindex to getIndex,
lastindex to putIndex.
My comment is not about names, but in this path '(ARCMSR_MAX_QBUFFER -
acb-rqbuf_firstindex)+ acb-rqbuf_lastindex) 1032)'
you copy something twice and in both cases the 'firstindex' is used and
never the 'lastindex'.
Is this correct?
The firstindex is a get index and lastindex is a put index of a ring buffer.
At here, firstindex lastindex, so the data remain in buffer are
(ARCMSR_MAX_QBUFFER - acb-rqbuf_firstindex)+ acb-rqbuf_lastindex
Yes, it's correct, I misinterpreted the from value with the amount of bytes
to copy.
But well it's also still overcomplicated and I believe that a copy like this
could be
rearranged with just few lines of code as a result - have you looked at the
code I sent?
Let's go with this patch as it is otherwise we will never end, repost is not
needed because
of this and also not because of arcmsr_Read_iop_rqbuffer_in_DWORD.
I'll continue with reviewing the remaining patches.
tomas
I have test the code you sent. It works.
I will modify the code by your idea, then send the patch.
I think that after so many repost, and because it is not a fix, wait till we
get this series in
and post a new patch later.
Btw. a similar copying is in arcmsr_iop_message_xfer too, and whet you want
rename the fields
in your ring buffer, please use the more usual names 'head+tail'.
Conclusion - post nothing right now, let the changes go in in a new patch.
tomash
Thanks,
Ching
What does the 1032 mean is that a hw. limit, actually could you explain
the code
should
Re: [PATCH v3 13/17] arcmsr: fix ioctl data read/write error for adapter type C
On Wed, 2014-08-27 at 14:29 +0200, Tomas Henzl wrote:
On 08/27/2014 10:19 PM, Ching Huang wrote:
On Tue, 2014-08-26 at 15:20 +0200, Tomas Henzl wrote:
On 08/26/2014 10:27 PM, Ching Huang wrote:
On Mon, 2014-08-25 at 12:29 +0200, Tomas Henzl wrote:
On 08/25/2014 07:59 PM, Ching Huang wrote:
On Fri, 2014-08-22 at 18:00 +0200, Tomas Henzl wrote:
On 08/19/2014 09:17 AM, Ching Huang wrote:
From: Ching Huang ching2...@areca.com.tw
Rewrite ioctl entry and its relate function.
This patch fix ioctl data read/write error and change data I/O access
from byte to Dword.
Signed-off-by: Ching Huang ching2...@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-02-06
17:47:24.0 +0800
+++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-04-29
17:10:42.0 +0800
@@ -70,40 +70,75 @@ static ssize_t arcmsr_sysfs_iop_message_
struct AdapterControlBlock *acb = (struct AdapterControlBlock
*) host-hostdata;
uint8_t *pQbuffer,*ptmpQbuffer;
int32_t allxfer_len = 0;
+ unsigned long flags;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
/* do message unit read. */
ptmpQbuffer = (uint8_t *)buf;
- while ((acb-rqbuf_firstindex != acb-rqbuf_lastindex)
- (allxfer_len 1031)) {
+ spin_lock_irqsave(acb-rqbuffer_lock, flags);
+ if (acb-rqbuf_firstindex != acb-rqbuf_lastindex) {
Hi - does this condition (acb-rqbuf_firstindex ==
acb-rqbuf_lastindex) mean we could just release
the spinlock and return ?
NO. We have to check the input buffer that may have message data come
from IOP.
pQbuffer = acb-rqbuffer[acb-rqbuf_firstindex];
- memcpy(ptmpQbuffer, pQbuffer, 1);
- acb-rqbuf_firstindex++;
- acb-rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
- ptmpQbuffer++;
- allxfer_len++;
+ 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));
This code looks like you were copying some data from a ring buffer,
in that case - shouldn't be acb-rqbuf_lastindex used instead of
firstindex?
Yes, there copying data from a ring buffer. firstindex and lastindex are
bad name. For readability, I rename the firstindex to getIndex,
lastindex to putIndex.
My comment is not about names, but in this path '(ARCMSR_MAX_QBUFFER -
acb-rqbuf_firstindex)+ acb-rqbuf_lastindex) 1032)'
you copy something twice and in both cases the 'firstindex' is used and
never the 'lastindex'.
Is this correct?
The firstindex is a get index and lastindex is a put index of a ring
buffer.
At here, firstindex lastindex, so the data remain in buffer are
(ARCMSR_MAX_QBUFFER - acb-rqbuf_firstindex)+ acb-rqbuf_lastindex
Yes, it's correct, I misinterpreted the from value with the amount of
bytes to copy.
But well it's also still overcomplicated and I believe that a copy like
this could be
rearranged with just few lines of code as a result - have you looked at
the code I sent?
Let's go with this patch as it is otherwise we will never end, repost is
not needed because
of this and also not because of arcmsr_Read_iop_rqbuffer_in_DWORD.
I'll continue with reviewing the remaining patches.
tomas
I have test the code you sent. It works.
I will modify the code by your idea, then send the patch.
I think that after so many repost, and because it is not a fix, wait till we
get this series in
and post a new patch later.
Btw. a similar copying is in arcmsr_iop_message_xfer too, and whet you want
rename the fields
in your ring buffer, please use the more usual names 'head+tail'.
Conclusion - post nothing right now, let the changes go in in a new patch.
Re: [PATCH v3 13/17] arcmsr: fix ioctl data read/write error for adapter type C
On Mon, 2014-08-25 at 12:29 +0200, Tomas Henzl wrote:
On 08/25/2014 07:59 PM, Ching Huang wrote:
On Fri, 2014-08-22 at 18:00 +0200, Tomas Henzl wrote:
On 08/19/2014 09:17 AM, Ching Huang wrote:
From: Ching Huang ching2...@areca.com.tw
Rewrite ioctl entry and its relate function.
This patch fix ioctl data read/write error and change data I/O access
from byte to Dword.
Signed-off-by: Ching Huang ching2...@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-02-06 17:47:24.0
+0800
+++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-04-29 17:10:42.0
+0800
@@ -70,40 +70,75 @@ static ssize_t arcmsr_sysfs_iop_message_
struct AdapterControlBlock *acb = (struct AdapterControlBlock *)
host-hostdata;
uint8_t *pQbuffer,*ptmpQbuffer;
int32_t allxfer_len = 0;
+ unsigned long flags;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
/* do message unit read. */
ptmpQbuffer = (uint8_t *)buf;
- while ((acb-rqbuf_firstindex != acb-rqbuf_lastindex)
- (allxfer_len 1031)) {
+ spin_lock_irqsave(acb-rqbuffer_lock, flags);
+ if (acb-rqbuf_firstindex != acb-rqbuf_lastindex) {
Hi - does this condition (acb-rqbuf_firstindex == acb-rqbuf_lastindex)
mean we could just release
the spinlock and return ?
NO. We have to check the input buffer that may have message data come
from IOP.
pQbuffer = acb-rqbuffer[acb-rqbuf_firstindex];
- memcpy(ptmpQbuffer, pQbuffer, 1);
- acb-rqbuf_firstindex++;
- acb-rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
- ptmpQbuffer++;
- allxfer_len++;
+ 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));
This code looks like you were copying some data from a ring buffer,
in that case - shouldn't be acb-rqbuf_lastindex used instead of
firstindex?
Yes, there copying data from a ring buffer. firstindex and lastindex are
bad name. For readability, I rename the firstindex to getIndex,
lastindex to putIndex.
My comment is not about names, but in this path '(ARCMSR_MAX_QBUFFER -
acb-rqbuf_firstindex)+ acb-rqbuf_lastindex) 1032)'
you copy something twice and in both cases the 'firstindex' is used and never
the 'lastindex'.
Is this correct?
The firstindex is a get index and lastindex is a put index of a ring buffer.
At here, firstindex lastindex, so the data remain in buffer are
(ARCMSR_MAX_QBUFFER - acb-rqbuf_firstindex)+ acb-rqbuf_lastindex
What does the 1032 mean is that a hw. limit, actually could you explain
the code
should do? Maybe I'm just wrong with my assumptions.
1032 is the API data buffer limitation.
Thanks,
Tomas
+ 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 {
Re: [PATCH v3 13/17] arcmsr: fix ioctl data read/write error for adapter type C
On 08/26/2014 10:27 PM, Ching Huang wrote:
On Mon, 2014-08-25 at 12:29 +0200, Tomas Henzl wrote:
On 08/25/2014 07:59 PM, Ching Huang wrote:
On Fri, 2014-08-22 at 18:00 +0200, Tomas Henzl wrote:
On 08/19/2014 09:17 AM, Ching Huang wrote:
From: Ching Huang ching2...@areca.com.tw
Rewrite ioctl entry and its relate function.
This patch fix ioctl data read/write error and change data I/O access
from byte to Dword.
Signed-off-by: Ching Huang ching2...@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-02-06 17:47:24.0
+0800
+++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-04-29 17:10:42.0
+0800
@@ -70,40 +70,75 @@ static ssize_t arcmsr_sysfs_iop_message_
struct AdapterControlBlock *acb = (struct AdapterControlBlock *)
host-hostdata;
uint8_t *pQbuffer,*ptmpQbuffer;
int32_t allxfer_len = 0;
+ unsigned long flags;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
/* do message unit read. */
ptmpQbuffer = (uint8_t *)buf;
- while ((acb-rqbuf_firstindex != acb-rqbuf_lastindex)
- (allxfer_len 1031)) {
+ spin_lock_irqsave(acb-rqbuffer_lock, flags);
+ if (acb-rqbuf_firstindex != acb-rqbuf_lastindex) {
Hi - does this condition (acb-rqbuf_firstindex == acb-rqbuf_lastindex)
mean we could just release
the spinlock and return ?
NO. We have to check the input buffer that may have message data come
from IOP.
pQbuffer = acb-rqbuffer[acb-rqbuf_firstindex];
- memcpy(ptmpQbuffer, pQbuffer, 1);
- acb-rqbuf_firstindex++;
- acb-rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
- ptmpQbuffer++;
- allxfer_len++;
+ 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));
This code looks like you were copying some data from a ring buffer,
in that case - shouldn't be acb-rqbuf_lastindex used instead of
firstindex?
Yes, there copying data from a ring buffer. firstindex and lastindex are
bad name. For readability, I rename the firstindex to getIndex,
lastindex to putIndex.
My comment is not about names, but in this path '(ARCMSR_MAX_QBUFFER -
acb-rqbuf_firstindex)+ acb-rqbuf_lastindex) 1032)'
you copy something twice and in both cases the 'firstindex' is used and
never the 'lastindex'.
Is this correct?
The firstindex is a get index and lastindex is a put index of a ring buffer.
At here, firstindex lastindex, so the data remain in buffer are
(ARCMSR_MAX_QBUFFER - acb-rqbuf_firstindex)+ acb-rqbuf_lastindex
Yes, it's correct, I misinterpreted the from value with the amount of bytes to
copy.
But well it's also still overcomplicated and I believe that a copy like this
could be
rearranged with just few lines of code as a result - have you looked at the
code I sent?
Let's go with this patch as it is otherwise we will never end, repost is not
needed because
of this and also not because of arcmsr_Read_iop_rqbuffer_in_DWORD.
I'll continue with reviewing the remaining patches.
tomas
What does the 1032 mean is that a hw. limit, actually could you explain
the code
should do? Maybe I'm just wrong with my assumptions.
1032 is the API data buffer limitation.
Thanks,
Tomas
+ 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,
+
Re: [PATCH v3 13/17] arcmsr: fix ioctl data read/write error for adapter type C
On Fri, 2014-08-22 at 18:00 +0200, Tomas Henzl wrote:
On 08/19/2014 09:17 AM, Ching Huang wrote:
From: Ching Huang ching2...@areca.com.tw
Rewrite ioctl entry and its relate function.
This patch fix ioctl data read/write error and change data I/O access from
byte to Dword.
Signed-off-by: Ching Huang ching2...@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-02-06 17:47:24.0
+0800
+++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-04-29 17:10:42.0
+0800
@@ -70,40 +70,75 @@ static ssize_t arcmsr_sysfs_iop_message_
struct AdapterControlBlock *acb = (struct AdapterControlBlock *)
host-hostdata;
uint8_t *pQbuffer,*ptmpQbuffer;
int32_t allxfer_len = 0;
+ unsigned long flags;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
/* do message unit read. */
ptmpQbuffer = (uint8_t *)buf;
- while ((acb-rqbuf_firstindex != acb-rqbuf_lastindex)
-(allxfer_len 1031)) {
+ spin_lock_irqsave(acb-rqbuffer_lock, flags);
+ if (acb-rqbuf_firstindex != acb-rqbuf_lastindex) {
Hi - does this condition (acb-rqbuf_firstindex == acb-rqbuf_lastindex) mean
we could just release
the spinlock and return ?
NO. We have to check the input buffer that may have message data come
from IOP.
pQbuffer = acb-rqbuffer[acb-rqbuf_firstindex];
- memcpy(ptmpQbuffer, pQbuffer, 1);
- acb-rqbuf_firstindex++;
- acb-rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
- ptmpQbuffer++;
- allxfer_len++;
+ 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));
This code looks like you were copying some data from a ring buffer,
in that case - shouldn't be acb-rqbuf_lastindex used instead of firstindex?
Yes, there copying data from a ring buffer. firstindex and lastindex are
bad name. For readability, I rename the firstindex to getIndex,
lastindex to putIndex.
What does the 1032 mean is that a hw. limit, actually could you explain the
code
should do? Maybe I'm just wrong with my assumptions.
1032 is the API data buffer limitation.
Thanks,
Tomas
+ 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);
+
Re: [PATCH v3 13/17] arcmsr: fix ioctl data read/write error for adapter type C
On 08/25/2014 07:59 PM, Ching Huang wrote:
On Fri, 2014-08-22 at 18:00 +0200, Tomas Henzl wrote:
On 08/19/2014 09:17 AM, Ching Huang wrote:
From: Ching Huang ching2...@areca.com.tw
Rewrite ioctl entry and its relate function.
This patch fix ioctl data read/write error and change data I/O access from
byte to Dword.
Signed-off-by: Ching Huang ching2...@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-02-06 17:47:24.0
+0800
+++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-04-29 17:10:42.0
+0800
@@ -70,40 +70,75 @@ static ssize_t arcmsr_sysfs_iop_message_
struct AdapterControlBlock *acb = (struct AdapterControlBlock *)
host-hostdata;
uint8_t *pQbuffer,*ptmpQbuffer;
int32_t allxfer_len = 0;
+ unsigned long flags;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
/* do message unit read. */
ptmpQbuffer = (uint8_t *)buf;
- while ((acb-rqbuf_firstindex != acb-rqbuf_lastindex)
-(allxfer_len 1031)) {
+ spin_lock_irqsave(acb-rqbuffer_lock, flags);
+ if (acb-rqbuf_firstindex != acb-rqbuf_lastindex) {
Hi - does this condition (acb-rqbuf_firstindex == acb-rqbuf_lastindex)
mean we could just release
the spinlock and return ?
NO. We have to check the input buffer that may have message data come
from IOP.
pQbuffer = acb-rqbuffer[acb-rqbuf_firstindex];
- memcpy(ptmpQbuffer, pQbuffer, 1);
- acb-rqbuf_firstindex++;
- acb-rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
- ptmpQbuffer++;
- allxfer_len++;
+ 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));
This code looks like you were copying some data from a ring buffer,
in that case - shouldn't be acb-rqbuf_lastindex used instead of firstindex?
Yes, there copying data from a ring buffer. firstindex and lastindex are
bad name. For readability, I rename the firstindex to getIndex,
lastindex to putIndex.
My comment is not about names, but in this path '(ARCMSR_MAX_QBUFFER -
acb-rqbuf_firstindex)+ acb-rqbuf_lastindex) 1032)'
you copy something twice and in both cases the 'firstindex' is used and never
the 'lastindex'.
Is this correct?
What does the 1032 mean is that a hw. limit, actually could you explain the
code
should do? Maybe I'm just wrong with my assumptions.
1032 is the API data buffer limitation.
Thanks,
Tomas
+ 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;
+
Re: [PATCH v3 13/17] arcmsr: fix ioctl data read/write error for adapter type C
On 08/25/2014 12:29 PM, Tomas Henzl wrote:
On 08/25/2014 07:59 PM, Ching Huang wrote:
On Fri, 2014-08-22 at 18:00 +0200, Tomas Henzl wrote:
On 08/19/2014 09:17 AM, Ching Huang wrote:
From: Ching Huang ching2...@areca.com.tw
Rewrite ioctl entry and its relate function.
This patch fix ioctl data read/write error and change data I/O access from
byte to Dword.
Signed-off-by: Ching Huang ching2...@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.c2014-02-06 17:47:24.0
+0800
+++ b/drivers/scsi/arcmsr/arcmsr_attr.c2014-04-29 17:10:42.0
+0800
@@ -70,40 +70,75 @@ static ssize_t arcmsr_sysfs_iop_message_
struct AdapterControlBlock *acb = (struct AdapterControlBlock *)
host-hostdata;
uint8_t *pQbuffer,*ptmpQbuffer;
int32_t allxfer_len = 0;
+ unsigned long flags;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
/* do message unit read. */
ptmpQbuffer = (uint8_t *)buf;
- while ((acb-rqbuf_firstindex != acb-rqbuf_lastindex)
- (allxfer_len 1031)) {
+ spin_lock_irqsave(acb-rqbuffer_lock, flags);
+ if (acb-rqbuf_firstindex != acb-rqbuf_lastindex) {
Hi - does this condition (acb-rqbuf_firstindex == acb-rqbuf_lastindex)
mean we could just release
the spinlock and return ?
NO. We have to check the input buffer that may have message data come
from IOP.
pQbuffer = acb-rqbuffer[acb-rqbuf_firstindex];
- memcpy(ptmpQbuffer, pQbuffer, 1);
- acb-rqbuf_firstindex++;
- acb-rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
- ptmpQbuffer++;
- allxfer_len++;
+ 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));
This code looks like you were copying some data from a ring buffer,
in that case - shouldn't be acb-rqbuf_lastindex used instead of firstindex?
Yes, there copying data from a ring buffer. firstindex and lastindex are
bad name. For readability, I rename the firstindex to getIndex,
lastindex to putIndex.
My comment is not about names, but in this path '(ARCMSR_MAX_QBUFFER -
acb-rqbuf_firstindex)+ acb-rqbuf_lastindex) 1032)'
you copy something twice and in both cases the 'firstindex' is used and never
the 'lastindex'.
Is this correct?
And when it is copying from a ring buffer, maybe it could be made in a simpler
way?
What do you think about this (not even compile tested, just an idea):
spin_lock_irqsave(acb-rqbuffer_lock, flags);
unsigned int tail = acb-rqbuf_firstindex;
unsigned int head = acb-rqbuf_lastindex;
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 1032)
allxfer_len = 1032;
if (allxfer_len = cnt_to_end)
memcpy(buf, acb-rqbuffer + tail, allxfer_len);
else {
memcpy(buf, acb-rqbuffer + tails, cnt_to_end);
memcpy(buf + cnt_to_end, acb-rqbuffer, allxfer_len -
cnt_to_end);
}
acb-rqbuf_firstindex = (acb-rqbuf_firstindex + allxfer_len) %
ARCMSR_MAX_QBUFFER;
What does the 1032 mean is that a hw. limit, actually could you explain the
code
should do? Maybe I'm just wrong with my assumptions.
1032 is the API data buffer limitation.
Thanks,
Tomas
+ acb-rqbuf_firstindex = 1032 -
+ (ARCMSR_MAX_QBUFFER -
+ acb-rqbuf_firstindex);
+ allxfer_len = 1032;
+ } else {
+ memcpy(ptmpQbuffer, pQbuffer,
+ ARCMSR_MAX_QBUFFER -
+ acb-rqbuf_firstindex);
+ ptmpQbuffer +=
Re: [PATCH v3 13/17] arcmsr: fix ioctl data read/write error for adapter type C
On 08/19/2014 09:17 AM, Ching Huang wrote:
From: Ching Huang ching2...@areca.com.tw
Rewrite ioctl entry and its relate function.
This patch fix ioctl data read/write error and change data I/O access from
byte to Dword.
Signed-off-by: Ching Huang ching2...@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-02-06 17:47:24.0
+0800
+++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-04-29 17:10:42.0
+0800
@@ -70,40 +70,75 @@ static ssize_t arcmsr_sysfs_iop_message_
struct AdapterControlBlock *acb = (struct AdapterControlBlock *)
host-hostdata;
uint8_t *pQbuffer,*ptmpQbuffer;
int32_t allxfer_len = 0;
+ unsigned long flags;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
/* do message unit read. */
ptmpQbuffer = (uint8_t *)buf;
- while ((acb-rqbuf_firstindex != acb-rqbuf_lastindex)
- (allxfer_len 1031)) {
+ spin_lock_irqsave(acb-rqbuffer_lock, flags);
+ if (acb-rqbuf_firstindex != acb-rqbuf_lastindex) {
Hi - does this condition (acb-rqbuf_firstindex == acb-rqbuf_lastindex) mean
we could just release
the spinlock and return ?
pQbuffer = acb-rqbuffer[acb-rqbuf_firstindex];
- memcpy(ptmpQbuffer, pQbuffer, 1);
- acb-rqbuf_firstindex++;
- acb-rqbuf_firstindex %= ARCMSR_MAX_QBUFFER;
- ptmpQbuffer++;
- allxfer_len++;
+ 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));
This code looks like you were copying some data from a ring buffer,
in that case - shouldn't be acb-rqbuf_lastindex used instead of firstindex?
What does the 1032 mean is that a hw. limit, actually could you explain the
code
should do? Maybe I'm just wrong with my assumptions.
Thanks,
Tomas
+ 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-acb_flags ACB_F_IOPDATA_OVERFLOW) {
struct
