Re: [PATCH 07/24] arm: scsi convert to accessors and !use_sg cleanup
On Sun, Dec 16, 2007 at 05:28:33PM +0200, Boaz Harrosh wrote: > Russell or any other arm person. Please first see if this compiles at all, as > I do > not have a cross compiler set up, and please check that this code works. > (Should apply on top of Linus latest) This patch seems to work with one change: > + SCpnt->SCp.phase = > + min(len, scsi_bufflen(SCpnt)); This min wants to be + min_t(unsigned long, len, scsi_bufflen(SCpnt)); -- Russell King Linux kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/24] arm: scsi convert to accessors and !use_sg cleanup
On Wed, 2007-09-12 at 08:42 +0100, Russell King wrote:
> On Wed, Sep 12, 2007 at 02:55:19AM +0300, Boaz Harrosh wrote:
> > - if (SCpnt->request_bufflen != len)
> > + if (scsi_bufflen(SCpnt) != len) {
> > + WARN_ON(1);
>
> NAK. The call trace generally doesn't provide any additional information
> on the cause of the error.
>
> > printk(KERN_WARNING "scsi%d.%c: bad request buffer "
> >"length %d, should be %ld\n",
> > SCpnt->device->host->host_no,
> > - '0' + SCpnt->device->id, SCpnt->request_bufflen,
> > len);
> > - SCpnt->request_bufflen = len;
> > + '0' + SCpnt->device->id, scsi_bufflen(SCpnt),
> > len);
> > + }
> > #endif
> > } else {
> > - SCpnt->SCp.ptr = (unsigned char *)SCpnt->request_buffer;
> > - SCpnt->SCp.this_residual = SCpnt->request_bufflen;
> > - SCpnt->SCp.phase = SCpnt->request_bufflen;
> > - }
> > -
> > - /*
> > -* If the upper SCSI layers pass a buffer, but zero length,
> > -* we aren't interested in the buffer pointer.
> > -*/
> > - if (SCpnt->SCp.this_residual == 0 && SCpnt->SCp.ptr) {
> > -#if 0 //def BELT_AND_BRACES
> > - printk(KERN_WARNING "scsi%d.%c: zero length buffer passed for "
> > - "command ", SCpnt->host->host_no, '0' + SCpnt->target);
> > - __scsi_print_command(SCpnt->cmnd);
> > -#endif
> > SCpnt->SCp.ptr = NULL;
> > + SCpnt->SCp.this_residual = 0;
> > + SCpnt->SCp.phase = 0;
> > }
> > }
>
> Also NAK. This was added due to bad behaviour of the SCSI layer and
> was found to be necessary.
Time is up on this one: this driver now won't build in mainline because
of the promised sg_table updates. Either you ack the changes or provide
your own. If not, I'll mark the driver BROKEN.
James
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/24] arm: scsi convert to accessors and !use_sg cleanup
On Sat, Dec 15 2007 at 2:27 +0200, James Bottomley <[EMAIL PROTECTED]> wrote:
> On Tue, 2007-09-18 at 17:04 +0200, Boaz Harrosh wrote:
>> On Wed, Sep 12 2007 at 10:42 +0300, Russell King <[EMAIL PROTECTED]> wrote:
>>> On Wed, Sep 12, 2007 at 02:55:19AM +0300, Boaz Harrosh wrote:
- if (SCpnt->request_bufflen != len)
+ if (scsi_bufflen(SCpnt) != len) {
+ WARN_ON(1);
>>> NAK. The call trace generally doesn't provide any additional information
>>> on the cause of the error.
>>>
>> In my opinion this can not happen any more. If it does, I want to see that
>> it is not through the regular scsi-ml .queuecommand mechanism.
>> But if you insist than sure I will remove it.
>>
printk(KERN_WARNING "scsi%d.%c: bad request buffer "
"length %d, should be %ld\n",
SCpnt->device->host->host_no,
- '0' + SCpnt->device->id, SCpnt->request_bufflen,
len);
- SCpnt->request_bufflen = len;
+ '0' + SCpnt->device->id, scsi_bufflen(SCpnt),
len);
+ }
#endif
} else {
- SCpnt->SCp.ptr = (unsigned char *)SCpnt->request_buffer;
- SCpnt->SCp.this_residual = SCpnt->request_bufflen;
- SCpnt->SCp.phase = SCpnt->request_bufflen;
- }
-
- /*
- * If the upper SCSI layers pass a buffer, but zero length,
- * we aren't interested in the buffer pointer.
- */
- if (SCpnt->SCp.this_residual == 0 && SCpnt->SCp.ptr) {
-#if 0 //def BELT_AND_BRACES
- printk(KERN_WARNING "scsi%d.%c: zero length buffer passed for "
- "command ", SCpnt->host->host_no, '0' + SCpnt->target);
- __scsi_print_command(SCpnt->cmnd);
-#endif
SCpnt->SCp.ptr = NULL;
+ SCpnt->SCp.this_residual = 0;
+ SCpnt->SCp.phase = 0;
}
}
>>> Also NAK. This was added due to bad behaviour of the SCSI layer and
>>> was found to be necessary.
>>>
>> No! This check is no longer Relevant. The master if() is on bufflen() now,
>> and only than do we ever set SCp.ptr. The else will always set both to Zero.
>> (Which is what you want)
>>
>> In any way this check is done in scsi-ml, and since 2.6.18 only scsi-ml
>> can allocate and issue commands. All other sources of commands where removed.
>> All upper layers issue requests now.
>
> Russell, could you respond to this, please? Boaz's points seem valid to
> me and this conversion must be done soon otherwise these drivers will
> break.
>
> James
>
>
Reinspecting this code in view of the overall arm-scsi, I conclude that arm
is CURRENTLY BROKEN in 2.6.24-rcx tree. With or without this patch.
Resubmitted in this mail is the sg-safe version of this patch. Maybe
with the bug fixes it will finally be acknowledged by the arm people.
The reason it is currently broken is because of copy_SCp_to_sg() which I did
not previously
touched. It is broken both on the account of the wrong assumption about the
sg-list
coming from scsi at SCp.buffer, and because inspecting the code, the sg-list
pointed
to by destination sg is later passed to a DMA mapper and it is not initialized
properly.
These and more are fixed in the new patch.
After the new patch, arm drivers are not yet safe for chaining. But the
arm-scsi-mid-layer is. Meaning, good behaving arm drivers will no longer
suffer from bugs at the mid-level.
Russell or any other arm person. Please first see if this compiles at all, as I
do
not have a cross compiler set up, and please check that this code works.
(Should apply on top of Linus latest)
--
>From f02d6f44c9043258f8aa63c7dfe56c9a67db3fcf Mon Sep 17 00:00:00 2001
From: Boaz Harrosh <[EMAIL PROTECTED]>
Date: Sun, 9 Sep 2007 21:31:21 +0300
Subject: [PATCH] [SCSI] arm: sg bugfixes and convert to accessors
- convert to accessors and !use_sg cleanup
- fix for sg_chaining bugs
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
Cc: Russell King <[EMAIL PROTECTED]>
---
drivers/scsi/arm/acornscsi.c | 14 +++---
drivers/scsi/arm/scsi.h | 86 -
2 files changed, 57 insertions(+), 43 deletions(-)
diff --git a/drivers/scsi/arm/acornscsi.c b/drivers/scsi/arm/acornscsi.c
index eceacf6..3bedf24 100644
--- a/drivers/scsi/arm/acornscsi.c
+++ b/drivers/scsi/arm/acornscsi.c
@@ -1790,7 +1790,7 @@ int acornscsi_starttransfer(AS_Host *host)
return 0;
}
-residual = host->SCpnt->request_bufflen - host->scsi.SCp.scsi_xferred;
+residual = scsi_bufflen(host->SCpnt) - host->scsi.SCp.scsi_xferred;
sbic_arm_write(host->scsi.io_port, SBIC_SYNCHTRANSFER,
host->device[host->SCpnt->device->id].sync_xfer);
sbic_arm_writenext(host->scsi.io_port, residual >> 16);
@@ -2270,7 +2270,7 @@ intr_ret_t acornscsi_sbicintr(AS_Host *host, int in_irq)
case 0x4b:
Re: [PATCH 07/24] arm: scsi convert to accessors and !use_sg cleanup
On Tue, 2007-09-18 at 17:04 +0200, Boaz Harrosh wrote:
> On Wed, Sep 12 2007 at 10:42 +0300, Russell King <[EMAIL PROTECTED]> wrote:
> > On Wed, Sep 12, 2007 at 02:55:19AM +0300, Boaz Harrosh wrote:
> >> - if (SCpnt->request_bufflen != len)
> >> + if (scsi_bufflen(SCpnt) != len) {
> >> + WARN_ON(1);
> >
> > NAK. The call trace generally doesn't provide any additional information
> > on the cause of the error.
> >
> In my opinion this can not happen any more. If it does, I want to see that
> it is not through the regular scsi-ml .queuecommand mechanism.
> But if you insist than sure I will remove it.
>
> >>printk(KERN_WARNING "scsi%d.%c: bad request buffer "
> >> "length %d, should be %ld\n",
> >> SCpnt->device->host->host_no,
> >> - '0' + SCpnt->device->id, SCpnt->request_bufflen,
> >> len);
> >> - SCpnt->request_bufflen = len;
> >> + '0' + SCpnt->device->id, scsi_bufflen(SCpnt),
> >> len);
> >> + }
> >> #endif
> >>} else {
> >> - SCpnt->SCp.ptr = (unsigned char *)SCpnt->request_buffer;
> >> - SCpnt->SCp.this_residual = SCpnt->request_bufflen;
> >> - SCpnt->SCp.phase = SCpnt->request_bufflen;
> >> - }
> >> -
> >> - /*
> >> - * If the upper SCSI layers pass a buffer, but zero length,
> >> - * we aren't interested in the buffer pointer.
> >> - */
> >> - if (SCpnt->SCp.this_residual == 0 && SCpnt->SCp.ptr) {
> >> -#if 0 //def BELT_AND_BRACES
> >> - printk(KERN_WARNING "scsi%d.%c: zero length buffer passed for "
> >> - "command ", SCpnt->host->host_no, '0' + SCpnt->target);
> >> - __scsi_print_command(SCpnt->cmnd);
> >> -#endif
> >>SCpnt->SCp.ptr = NULL;
> >> + SCpnt->SCp.this_residual = 0;
> >> + SCpnt->SCp.phase = 0;
> >>}
> >> }
> >
> > Also NAK. This was added due to bad behaviour of the SCSI layer and
> > was found to be necessary.
> >
> No! This check is no longer Relevant. The master if() is on bufflen() now,
> and only than do we ever set SCp.ptr. The else will always set both to Zero.
> (Which is what you want)
>
> In any way this check is done in scsi-ml, and since 2.6.18 only scsi-ml
> can allocate and issue commands. All other sources of commands where removed.
> All upper layers issue requests now.
Russell, could you respond to this, please? Boaz's points seem valid to
me and this conversion must be done soon otherwise these drivers will
break.
James
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/24] arm: scsi convert to accessors and !use_sg cleanup
On Wed, Sep 12 2007 at 10:42 +0300, Russell King <[EMAIL PROTECTED]> wrote:
> On Wed, Sep 12, 2007 at 02:55:19AM +0300, Boaz Harrosh wrote:
>> -if (SCpnt->request_bufflen != len)
>> +if (scsi_bufflen(SCpnt) != len) {
>> +WARN_ON(1);
>
> NAK. The call trace generally doesn't provide any additional information
> on the cause of the error.
>
In my opinion this can not happen any more. If it does, I want to see that
it is not through the regular scsi-ml .queuecommand mechanism.
But if you insist than sure I will remove it.
>> printk(KERN_WARNING "scsi%d.%c: bad request buffer "
>> "length %d, should be %ld\n",
>> SCpnt->device->host->host_no,
>> - '0' + SCpnt->device->id, SCpnt->request_bufflen,
>> len);
>> -SCpnt->request_bufflen = len;
>> + '0' + SCpnt->device->id, scsi_bufflen(SCpnt),
>> len);
>> +}
>> #endif
>> } else {
>> -SCpnt->SCp.ptr = (unsigned char *)SCpnt->request_buffer;
>> -SCpnt->SCp.this_residual = SCpnt->request_bufflen;
>> -SCpnt->SCp.phase = SCpnt->request_bufflen;
>> -}
>> -
>> -/*
>> - * If the upper SCSI layers pass a buffer, but zero length,
>> - * we aren't interested in the buffer pointer.
>> - */
>> -if (SCpnt->SCp.this_residual == 0 && SCpnt->SCp.ptr) {
>> -#if 0 //def BELT_AND_BRACES
>> -printk(KERN_WARNING "scsi%d.%c: zero length buffer passed for "
>> - "command ", SCpnt->host->host_no, '0' + SCpnt->target);
>> -__scsi_print_command(SCpnt->cmnd);
>> -#endif
>> SCpnt->SCp.ptr = NULL;
>> +SCpnt->SCp.this_residual = 0;
>> +SCpnt->SCp.phase = 0;
>> }
>> }
>
> Also NAK. This was added due to bad behaviour of the SCSI layer and
> was found to be necessary.
>
No! This check is no longer Relevant. The master if() is on bufflen() now,
and only than do we ever set SCp.ptr. The else will always set both to Zero.
(Which is what you want)
In any way this check is done in scsi-ml, and since 2.6.18 only scsi-ml
can allocate and issue commands. All other sources of commands where removed.
All upper layers issue requests now.
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/24] arm: scsi convert to accessors and !use_sg cleanup
On Wed, Sep 12, 2007 at 02:55:19AM +0300, Boaz Harrosh wrote:
> - if (SCpnt->request_bufflen != len)
> + if (scsi_bufflen(SCpnt) != len) {
> + WARN_ON(1);
NAK. The call trace generally doesn't provide any additional information
on the cause of the error.
> printk(KERN_WARNING "scsi%d.%c: bad request buffer "
> "length %d, should be %ld\n",
> SCpnt->device->host->host_no,
> -'0' + SCpnt->device->id, SCpnt->request_bufflen,
> len);
> - SCpnt->request_bufflen = len;
> +'0' + SCpnt->device->id, scsi_bufflen(SCpnt),
> len);
> + }
> #endif
> } else {
> - SCpnt->SCp.ptr = (unsigned char *)SCpnt->request_buffer;
> - SCpnt->SCp.this_residual = SCpnt->request_bufflen;
> - SCpnt->SCp.phase = SCpnt->request_bufflen;
> - }
> -
> - /*
> - * If the upper SCSI layers pass a buffer, but zero length,
> - * we aren't interested in the buffer pointer.
> - */
> - if (SCpnt->SCp.this_residual == 0 && SCpnt->SCp.ptr) {
> -#if 0 //def BELT_AND_BRACES
> - printk(KERN_WARNING "scsi%d.%c: zero length buffer passed for "
> -"command ", SCpnt->host->host_no, '0' + SCpnt->target);
> - __scsi_print_command(SCpnt->cmnd);
> -#endif
> SCpnt->SCp.ptr = NULL;
> + SCpnt->SCp.this_residual = 0;
> + SCpnt->SCp.phase = 0;
> }
> }
Also NAK. This was added due to bad behaviour of the SCSI layer and
was found to be necessary.
--
Russell King
Linux kernel2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 07/24] arm: scsi convert to accessors and !use_sg cleanup
- convert to accessors and !use_sg cleanup
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
drivers/scsi/arm/acornscsi.c | 14 +++---
drivers/scsi/arm/scsi.h | 34 +++---
2 files changed, 18 insertions(+), 30 deletions(-)
diff --git a/drivers/scsi/arm/acornscsi.c b/drivers/scsi/arm/acornscsi.c
index eceacf6..3bedf24 100644
--- a/drivers/scsi/arm/acornscsi.c
+++ b/drivers/scsi/arm/acornscsi.c
@@ -1790,7 +1790,7 @@ int acornscsi_starttransfer(AS_Host *host)
return 0;
}
-residual = host->SCpnt->request_bufflen - host->scsi.SCp.scsi_xferred;
+residual = scsi_bufflen(host->SCpnt) - host->scsi.SCp.scsi_xferred;
sbic_arm_write(host->scsi.io_port, SBIC_SYNCHTRANSFER,
host->device[host->SCpnt->device->id].sync_xfer);
sbic_arm_writenext(host->scsi.io_port, residual >> 16);
@@ -2270,7 +2270,7 @@ intr_ret_t acornscsi_sbicintr(AS_Host *host, int in_irq)
case 0x4b: /* -> PHASE_STATUSIN
*/
case 0x8b: /* -> PHASE_STATUSIN
*/
/* DATA IN -> STATUS */
- host->scsi.SCp.scsi_xferred = host->SCpnt->request_bufflen -
+ host->scsi.SCp.scsi_xferred = scsi_bufflen(host->SCpnt) -
acornscsi_sbic_xfcount(host);
acornscsi_dma_stop(host);
acornscsi_readstatusbyte(host);
@@ -2281,7 +2281,7 @@ intr_ret_t acornscsi_sbicintr(AS_Host *host, int in_irq)
case 0x4e: /* -> PHASE_MSGOUT
*/
case 0x8e: /* -> PHASE_MSGOUT
*/
/* DATA IN -> MESSAGE OUT */
- host->scsi.SCp.scsi_xferred = host->SCpnt->request_bufflen -
+ host->scsi.SCp.scsi_xferred = scsi_bufflen(host->SCpnt) -
acornscsi_sbic_xfcount(host);
acornscsi_dma_stop(host);
acornscsi_sendmessage(host);
@@ -2291,7 +2291,7 @@ intr_ret_t acornscsi_sbicintr(AS_Host *host, int in_irq)
case 0x4f: /* message in
*/
case 0x8f: /* message in
*/
/* DATA IN -> MESSAGE IN */
- host->scsi.SCp.scsi_xferred = host->SCpnt->request_bufflen -
+ host->scsi.SCp.scsi_xferred = scsi_bufflen(host->SCpnt) -
acornscsi_sbic_xfcount(host);
acornscsi_dma_stop(host);
acornscsi_message(host);/* -> PHASE_MSGIN, PHASE_DISCONNECT
*/
@@ -2319,7 +2319,7 @@ intr_ret_t acornscsi_sbicintr(AS_Host *host, int in_irq)
case 0x4b: /* -> PHASE_STATUSIN
*/
case 0x8b: /* -> PHASE_STATUSIN
*/
/* DATA OUT -> STATUS */
- host->scsi.SCp.scsi_xferred = host->SCpnt->request_bufflen -
+ host->scsi.SCp.scsi_xferred = scsi_bufflen(host->SCpnt) -
acornscsi_sbic_xfcount(host);
acornscsi_dma_stop(host);
acornscsi_dma_adjust(host);
@@ -2331,7 +2331,7 @@ intr_ret_t acornscsi_sbicintr(AS_Host *host, int in_irq)
case 0x4e: /* -> PHASE_MSGOUT
*/
case 0x8e: /* -> PHASE_MSGOUT
*/
/* DATA OUT -> MESSAGE OUT */
- host->scsi.SCp.scsi_xferred = host->SCpnt->request_bufflen -
+ host->scsi.SCp.scsi_xferred = scsi_bufflen(host->SCpnt) -
acornscsi_sbic_xfcount(host);
acornscsi_dma_stop(host);
acornscsi_dma_adjust(host);
@@ -2342,7 +2342,7 @@ intr_ret_t acornscsi_sbicintr(AS_Host *host, int in_irq)
case 0x4f: /* message in
*/
case 0x8f: /* message in
*/
/* DATA OUT -> MESSAGE IN */
- host->scsi.SCp.scsi_xferred = host->SCpnt->request_bufflen -
+ host->scsi.SCp.scsi_xferred = scsi_bufflen(host->SCpnt) -
acornscsi_sbic_xfcount(host);
acornscsi_dma_stop(host);
acornscsi_dma_adjust(host);
diff --git a/drivers/scsi/arm/scsi.h b/drivers/scsi/arm/scsi.h
index 21ba571..95cfb21 100644
--- a/drivers/scsi/arm/scsi.h
+++ b/drivers/scsi/arm/scsi.h
@@ -70,48 +70,36 @@ static inline void init_SCp(struct scsi_cmnd *SCpnt)
{
memset(&SCpnt->SCp, 0, sizeof(struct scsi_pointer));
- if (SCpnt->use_sg) {
+ if (scsi_bufflen(SCpnt)) {
unsigned long len = 0;
int buf;
- SCpnt->SCp.buffer = (struct scatterlist *)
SCpnt->request_buffer;
-
