Re: linux kernel panic when ejecting ieee1394 ipod

Re: linux kernel panic when ejecting ieee1394 ipod

[Stefan Richter]

Andrew de Quincey wrote to linux1394-devel:
> Reposting to the -devel list since there was no response on -user.

I would have responded eventually... :-)

> Hi, I'm using linux kernel 2.6.14 with an generation 1 ipod connected via 
> ieee1394/sbp2.
> 
> I can mount/unmount it and use it fine. It even works with HAL now.
> 
> The problem comes when I want to remove it. If I do:
> 
> eject /dev/sdb
> 
> The kernel panics with the following error:
> Kernel panic - not syncing: PCI-DMA: high address but no IOMMU

I think there was another report about "eject" of a FireWire iPod 
crashing the kernel but I cannot find it in any archive anymore and do 
not remember which kernel version and machine architecture was affected.

The eject command does work for me with a few different SBP-2 devices, 
but I do not have an iPod. But more importantly, I have currently only 
i386 with 512 MB RAM to test. The "high address but no IOMMU" panic 
message is specific to x86_64.

I had only a superficial look at it so far and did not spot anything 
obvious. (That's because I do not know anything about Linux' memory 
management.) My only idea at the moment is to Cc this to linux-scsi 
since sd_mod does work for you but the one thing you do via the sg 
driver does not.

> I'm using an nforce 4 motherboard (Asus A8N-SLI Deluxe). The ipod is attached 
> to the ieee1394 port on a creative labs Audigy2 sound card (uses OHCI-1394).
> 
> I do _not_ have "Enable Phys DMA support for SBP2 (Debug)" enabled if thats 
> any help.

-- 
Stefan Richter
-=====-=-=-= ==-- -=---
http://arcgraph.de/sr/

Re: linux kernel panic when ejecting ieee1394 ipod

[Andrew de Quincey]

(resent to include other lists)

On Wednesday 07 December 2005 23:58, Andrew de Quincey wrote:
> Reposting to the -devel list since there was no response on -user.
>
> Hi, I'm using linux kernel 2.6.14 with an generation 1 ipod connected via
> ieee1394/sbp2.
>
> I can mount/unmount it and use it fine. It even works with HAL now.
>
> The problem comes when I want to remove it. If I do:
>
> eject /dev/sdb
>
> The kernel panics with the following error:
> Kernel panic - not syncing: PCI-DMA: high address but no IOMMU
>
> I'm using an nforce 4 motherboard (Asus A8N-SLI Deluxe). The ipod is
> attached to the ieee1394 port on a creative labs Audigy2 sound card (uses
> OHCI-1394).

More info (I'm on an AMD64 in 64 bit mode BTW):

eject sends a CDROMEJECT ioctl first of all. It is this IOCTL that kills my 
system. If I hack/tell it to just use a "SCSI eject", it works and actually 
makes my ipod show that tick thing showing it is safe to remove it.


Tracing through the kernel for the CDROMEJECT shows:
drivers/scsi/sd.c/sd_ioctl() 
	-- calls scsi_cmd_ioctl()

drivers/block/scsi_ioctl.c/scsi_cmd_ioctl()
	-- emulates the CDROMEJECT ioctl as a START_STOP unit SCSI packet command

... then that passes through the blockdev/scsi layers until:

drivers/ieee1394/sbp2.c/sbp2scsi_queuecommand()
	-- this function actually works fine - it must be either in the callback 
handler when the command completes (?) or sometime afterwards.

The last SCSI command received by sbp2 before the crash is:

1b 00 00 02 00

Which is definitely what scsi_cmd_ioctl() is setting up.


CDROMEJECT works on other scsi devices - e.g. my USB storage pendrive appears 
as a SCSI drive, and can be "safely removed" by simply calling a CDROMEJECT 
ioctl.


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: linux kernel panic when ejecting ieee1394 ipod

[Andrew de Quincey]

On Thursday 08 December 2005 01:19, Andrew de Quincey wrote:
> On Wednesday 07 December 2005 23:58, Andrew de Quincey wrote:
> > Reposting to the -devel list since there was no response on -user.
> >
> > Hi, I'm using linux kernel 2.6.14 with an generation 1 ipod connected via
> > ieee1394/sbp2.
> >
> > I can mount/unmount it and use it fine. It even works with HAL now.
> >
> > The problem comes when I want to remove it. If I do:
> >
> > eject /dev/sdb
> >
> > The kernel panics with the following error:
> > Kernel panic - not syncing: PCI-DMA: high address but no IOMMU
> >
> > I'm using an nforce 4 motherboard (Asus A8N-SLI Deluxe). The ipod is
> > attached to the ieee1394 port on a creative labs Audigy2 sound card (uses
> > OHCI-1394).
>
> More info (I'm on an AMD64 in 64 bit mode BTW):
>
> eject sends a CDROMEJECT ioctl first of all. It is this IOCTL that kills my
> system. If I hack/tell it to just use a "SCSI eject", it works and actually
> makes my ipod show that tick thing showing it is safe to remove it.

Yet more info:

eject -s sends the following SCSI packet command to the sbp/ipod device:

1b 00 00 00 02 00 with a direction of DMA_NONE

eject -r results in the same via the CDROMEJECT translator in 
scsi_cmd_ioctl(), but with a direction of DMA_TO_DEVICE. Thats what crashes 
it - looks like its trying to set up a DMA transfer for 0 bytes or something. 
I just added a really horrible hack to the start of sbp2_send_command() to 
check, as follows:

   if (*cmd == 0x1b) {
      SCpnt->sc_data_direction = 3;
   }

Now, eject -r works perfectly. Though thats obviously not a good way to fix 
it :)


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: linux kernel panic when ejecting ieee1394 ipod

[Andrew de Quincey]

[-- Attachment #1: Type: text/plain, Size: 1993 bytes --]

On Thursday 08 December 2005 01:59, Andrew de Quincey wrote:
> On Thursday 08 December 2005 01:19, Andrew de Quincey wrote:
> > On Wednesday 07 December 2005 23:58, Andrew de Quincey wrote:
> > > Reposting to the -devel list since there was no response on -user.
> > >
> > > Hi, I'm using linux kernel 2.6.14 with an generation 1 ipod connected
> > > via ieee1394/sbp2.
> > >
> > > I can mount/unmount it and use it fine. It even works with HAL now.
> > >
> > > The problem comes when I want to remove it. If I do:
> > >
> > > eject /dev/sdb
> > >
> > > The kernel panics with the following error:
> > > Kernel panic - not syncing: PCI-DMA: high address but no IOMMU
> > >
> > > I'm using an nforce 4 motherboard (Asus A8N-SLI Deluxe). The ipod is
> > > attached to the ieee1394 port on a creative labs Audigy2 sound card
> > > (uses OHCI-1394).
> >
> > More info (I'm on an AMD64 in 64 bit mode BTW):
> >
> > eject sends a CDROMEJECT ioctl first of all. It is this IOCTL that kills
> > my system. If I hack/tell it to just use a "SCSI eject", it works and
> > actually makes my ipod show that tick thing showing it is safe to remove
> > it.
>
> Yet more info:
>
> eject -s sends the following SCSI packet command to the sbp/ipod device:
>
> 1b 00 00 00 02 00 with a direction of DMA_NONE
>
> eject -r results in the same via the CDROMEJECT translator in
> scsi_cmd_ioctl(), but with a direction of DMA_TO_DEVICE. Thats what crashes
> it - looks like its trying to set up a DMA transfer for 0 bytes or
> something. I just added a really horrible hack to the start of
> sbp2_send_command() to check, as follows:
>
>    if (*cmd == 0x1b) {
>       SCpnt->sc_data_direction = 3;
>    }
>
> Now, eject -r works perfectly. Though thats obviously not a good way to fix
> it :)

Possible "proper" patch attached - no idea if its a good way to do this. 
Basically if it gets a command with a scsi_request_bufflen of zero bytes, it 
forces dma_dir to DMA_NONE.

The ipod works perfectly with this now.

[-- Attachment #2: possible-sbp2-eject-fix-1.patch --]
[-- Type: text/x-diff, Size: 552 bytes --]

--- linux-2.6.14/drivers/ieee1394/sbp2.c	2005-12-08 02:05:27.000000000 +0000
+++ linux/drivers/ieee1394/sbp2.c	2005-12-08 02:06:18.000000000 +0000
@@ -1760,6 +1760,10 @@
 	command_orb->misc |= ORB_SET_SPEED(scsi_id->speed_code);
 	command_orb->misc |= ORB_SET_NOTIFY(1);		/* Notify us when complete */
 
+	/* check for duff DMA transfer direction with a zero length buffer */
+	if (scsi_request_bufflen == 0)
+		dma_dir = DMA_NONE;
+
 	/*
 	 * Get the direction of the transfer. If the direction is unknown, then use our
 	 * goofy table as a back-up.

Re: linux kernel panic when ejecting ieee1394 ipod

[Stefan Richter]

Andrew de Quincey wrote:
> On Thursday 08 December 2005 01:59, Andrew de Quincey wrote:
>>eject -s sends the following SCSI packet command to the sbp/ipod device:
>>
>>1b 00 00 00 02 00 with a direction of DMA_NONE
>>
>>eject -r results in the same via the CDROMEJECT translator in
>>scsi_cmd_ioctl(), but with a direction of DMA_TO_DEVICE. Thats what crashes
>>it - looks like its trying to set up a DMA transfer for 0 bytes or
>>something. I just added a really horrible hack to the start of
>>sbp2_send_command() to check, as follows:
>>
>>   if (*cmd == 0x1b) {
>>      SCpnt->sc_data_direction = 3;
>>   }
>>
>>Now, eject -r works perfectly. Though thats obviously not a good way to fix
>>it :)

That was a long time ago when sbp2 had to take care of data direction. I 
think we don't want to go back there. :-)

> Possible "proper" patch attached - no idea if its a good way to do this. 
> Basically if it gets a command with a scsi_request_bufflen of zero bytes, it 
> forces dma_dir to DMA_NONE.
> 
> The ipod works perfectly with this now.
...
[in sbp2_create_command_orb]
> +	/* check for duff DMA transfer direction with a zero length buffer */
> +	if (scsi_request_bufflen == 0)
> +		dma_dir = DMA_NONE;

What if you replace WRITE by READ in 
drivers/scsi/scsi_ioctl.c::scsi_cmd_ioctl()::case CDROMEJECT ?

And BTW, can/should this latter case block be converted to use 
scsi_execute() or scsi_execute_req()?
-- 
Stefan Richter
-=====-=-=-= ==-- -=---
http://arcgraph.de/sr/


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: linux kernel panic when ejecting ieee1394 ipod

[Andrew de Quincey]

> [in sbp2_create_command_orb]
>
> > +	/* check for duff DMA transfer direction with a zero length buffer */
> > +	if (scsi_request_bufflen == 0)
> > +		dma_dir = DMA_NONE;
>
> What if you replace WRITE by READ in
> drivers/scsi/scsi_ioctl.c::scsi_cmd_ioctl()::case CDROMEJECT ?

Yup, using READ there fixes it too (and I did remember to remove my other 
patch before testing :)

USB storage devices still work with the CDROMEJECT ioctl and this change as 
well.

> And BTW, can/should this latter case block be converted to use
> scsi_execute() or scsi_execute_req()?

Can't comment - don't know enough about the linux SCSI subsystem.

Re: linux kernel panic when ejecting ieee1394 ipod

[Stefan Richter]

Andrew de Quincey wrote:
>>What if you replace WRITE by READ in
>>drivers/scsi/scsi_ioctl.c::scsi_cmd_ioctl()::case CDROMEJECT ?
> 
> Yup, using READ there fixes it too (and I did remember to remove my other 
> patch before testing :)
> 
> USB storage devices still work with the CDROMEJECT ioctl and this change as 
> well.

I'll check if they have a safeguard which sbp2 should have too.
Thanks a lot.
-- 
Stefan Richter
-=====-=-=-= ==-- -=---
http://arcgraph.de/sr/

Re: linux kernel panic when ejecting ieee1394 ipod

[Patrick Mansfield]

On Thu, Dec 08, 2005 at 03:44:42AM +0100, Stefan Richter wrote:

> What if you replace WRITE by READ in 
> drivers/scsi/scsi_ioctl.c::scsi_cmd_ioctl()::case CDROMEJECT ?

You must mean block/scsi_ioctl.c? i.e.:

diff -uprN -X /home/patman/dontdiff linux-2.6.15-rc5-git1/block/scsi_ioctl.c mod- linux-2.6.15-rc5-git1/block/scsi_ioctl.c
--- /home/linux/views/linux-2.6.15-rc5-git1/block/scsi_ioctl.c	2005-12-03 22:51:54.000000000 -0800
+++ linux-2.6.15-rc5-git1/block/scsi_ioctl.c	2005-12-08 09:31:52.000000000 -0800
@@ -566,7 +566,7 @@ int scsi_cmd_ioctl(struct file *file, st
 		case CDROMCLOSETRAY:
 			close = 1;
 		case CDROMEJECT:
-			rq = blk_get_request(q, WRITE, __GFP_WAIT);
+			rq = blk_get_request(q, READ, __GFP_WAIT);
 			rq->flags |= REQ_BLOCK_PC;
 			rq->data = NULL;
 			rq->data_len = 0;

> And BTW, can/should this latter case block be converted to use 
> scsi_execute() or scsi_execute_req()?

And then scsi_cmd_ioctl() already uses blk layer REQ_BLOCK_PC.

-- Patrick Mansfield

Re: linux kernel panic when ejecting ieee1394 ipod

[Stefan Richter]

Patrick Mansfield wrote:
> On Thu, Dec 08, 2005 at 03:44:42AM +0100, Stefan Richter wrote:
>>What if you replace WRITE by READ in 
>>drivers/scsi/scsi_ioctl.c::scsi_cmd_ioctl()::case CDROMEJECT ?

(unless blk_get_request() knows another flag which comes closer to 
DMA_NONE than READ)

> You must mean block/scsi_ioctl.c? i.e.:

Yes, actually. %-)

> diff -uprN -X /home/patman/dontdiff linux-2.6.15-rc5-git1/block/scsi_ioctl.c mod- linux-2.6.15-rc5-git1/block/scsi_ioctl.c
> --- /home/linux/views/linux-2.6.15-rc5-git1/block/scsi_ioctl.c	2005-12-03 22:51:54.000000000 -0800
> +++ linux-2.6.15-rc5-git1/block/scsi_ioctl.c	2005-12-08 09:31:52.000000000 -0800
> @@ -566,7 +566,7 @@ int scsi_cmd_ioctl(struct file *file, st
>  		case CDROMCLOSETRAY:
>  			close = 1;
>  		case CDROMEJECT:
> -			rq = blk_get_request(q, WRITE, __GFP_WAIT);
> +			rq = blk_get_request(q, READ, __GFP_WAIT);
>  			rq->flags |= REQ_BLOCK_PC;
>  			rq->data = NULL;
>  			rq->data_len = 0;

This should also go to -stable.

BTW, a kernel oops after "eject /mnt/ipod" under 2.6.12 (though no 
panic) was also reported by Bryan Olmstead on July 16 2005. 
http://marc.theaimsgroup.com/?l=linux1394-user&m=112152701817435
 From the multiple problems reported there, everything except the eject 
issue has been fixed in the meantime.
-- 
Stefan Richter
-=====-=-=-= ==-- -=---
http://arcgraph.de/sr/

Re: linux kernel panic when ejecting ieee1394 ipod

[Stefan Richter]

I wrote:
> Andrew de Quincey wrote:
>> USB storage devices still work with the CDROMEJECT ioctl and this 
>> change as well.
> 
> I'll check if they have a safeguard which sbp2 should have too.

I browsed a bit through usb/storage/ but did not spot an obvious sanity 
check of this kind. AFAIU, slightly differently arranged case 
discriminations let usb-storage avoid this pitfall.

Your patch to sbp2_create_command_orb() is basically the right thing to 
do --- not as an actual fix of the bogus transfer direction passed to 
sbp2 but rather as a protection against catastrophic failure. I'd like 
to integrate your check in marginally different form though. Patch 
follows shortly.

Thanks again.
-- 
Stefan Richter
-=====-=-=-= ==-- -=---
http://arcgraph.de/sr/

Re: linux kernel panic when ejecting ieee1394 ipod

[Jens Axboe]

On Thu, Dec 08 2005, Patrick Mansfield wrote:
> On Thu, Dec 08, 2005 at 03:44:42AM +0100, Stefan Richter wrote:
> 
> > What if you replace WRITE by READ in 
> > drivers/scsi/scsi_ioctl.c::scsi_cmd_ioctl()::case CDROMEJECT ?
> 
> You must mean block/scsi_ioctl.c? i.e.:
> 
> diff -uprN -X /home/patman/dontdiff linux-2.6.15-rc5-git1/block/scsi_ioctl.c mod- linux-2.6.15-rc5-git1/block/scsi_ioctl.c
> --- /home/linux/views/linux-2.6.15-rc5-git1/block/scsi_ioctl.c	2005-12-03 22:51:54.000000000 -0800
> +++ linux-2.6.15-rc5-git1/block/scsi_ioctl.c	2005-12-08 09:31:52.000000000 -0800
> @@ -566,7 +566,7 @@ int scsi_cmd_ioctl(struct file *file, st
>  		case CDROMCLOSETRAY:
>  			close = 1;
>  		case CDROMEJECT:
> -			rq = blk_get_request(q, WRITE, __GFP_WAIT);
> +			rq = blk_get_request(q, READ, __GFP_WAIT);
>  			rq->flags |= REQ_BLOCK_PC;
>  			rq->data = NULL;
>  			rq->data_len = 0;

This should not make a difference, the direction flag must be ignored
for !rq->data_len. Similarly you could translate the request with the
change above into DMA_FROM_DEVICE which is clearly false as well. So it
must be a bug somewhere that somebody only looks at the direction flag.

-- 
Jens Axboe

Re: linux kernel panic when ejecting ieee1394 ipod

[Jens Axboe]

On Fri, Dec 09 2005, Jens Axboe wrote:
> On Thu, Dec 08 2005, Patrick Mansfield wrote:
> > On Thu, Dec 08, 2005 at 03:44:42AM +0100, Stefan Richter wrote:
> > 
> > > What if you replace WRITE by READ in 
> > > drivers/scsi/scsi_ioctl.c::scsi_cmd_ioctl()::case CDROMEJECT ?
> > 
> > You must mean block/scsi_ioctl.c? i.e.:
> > 
> > diff -uprN -X /home/patman/dontdiff linux-2.6.15-rc5-git1/block/scsi_ioctl.c mod- linux-2.6.15-rc5-git1/block/scsi_ioctl.c
> > --- /home/linux/views/linux-2.6.15-rc5-git1/block/scsi_ioctl.c	2005-12-03 22:51:54.000000000 -0800
> > +++ linux-2.6.15-rc5-git1/block/scsi_ioctl.c	2005-12-08 09:31:52.000000000 -0800
> > @@ -566,7 +566,7 @@ int scsi_cmd_ioctl(struct file *file, st
> >  		case CDROMCLOSETRAY:
> >  			close = 1;
> >  		case CDROMEJECT:
> > -			rq = blk_get_request(q, WRITE, __GFP_WAIT);
> > +			rq = blk_get_request(q, READ, __GFP_WAIT);
> >  			rq->flags |= REQ_BLOCK_PC;
> >  			rq->data = NULL;
> >  			rq->data_len = 0;
> 
> This should not make a difference, the direction flag must be ignored
> for !rq->data_len. Similarly you could translate the request with the
> change above into DMA_FROM_DEVICE which is clearly false as well. So it
> must be a bug somewhere that somebody only looks at the direction flag.

This looks like the real bug.

---


diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 4afef5c..0978887 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1215,12 +1215,12 @@ static int scsi_prep_fn(struct request_q
 		} else {
 			memcpy(cmd->cmnd, req->cmd, sizeof(cmd->cmnd));
 			cmd->cmd_len = req->cmd_len;
-			if (rq_data_dir(req) == WRITE)
+			if (!req->data_len)
+				cmd->sc_data_direction = DMA_NONE;
+			else if (rq_data_dir(req) == WRITE)
 				cmd->sc_data_direction = DMA_TO_DEVICE;
-			else if (req->data_len)
-				cmd->sc_data_direction = DMA_FROM_DEVICE;
 			else
-				cmd->sc_data_direction = DMA_NONE;
+				cmd->sc_data_direction = DMA_FROM_DEVICE;
 			
 			cmd->transfersize = req->data_len;
 			cmd->allowed = 3;


-- 
Jens Axboe

Re: linux kernel panic when ejecting ieee1394 ipod

[Stefan Richter]

Jens Axboe wrote:
> This looks like the real bug.
> 
> ---
> 
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 4afef5c..0978887 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1215,12 +1215,12 @@ static int scsi_prep_fn(struct request_q
>  		} else {
>  			memcpy(cmd->cmnd, req->cmd, sizeof(cmd->cmnd));
>  			cmd->cmd_len = req->cmd_len;
> -			if (rq_data_dir(req) == WRITE)
> +			if (!req->data_len)
> +				cmd->sc_data_direction = DMA_NONE;
> +			else if (rq_data_dir(req) == WRITE)
>  				cmd->sc_data_direction = DMA_TO_DEVICE;
> -			else if (req->data_len)
> -				cmd->sc_data_direction = DMA_FROM_DEVICE;
>  			else
> -				cmd->sc_data_direction = DMA_NONE;
> +				cmd->sc_data_direction = DMA_FROM_DEVICE;
>  			
>  			cmd->transfersize = req->data_len;
>  			cmd->allowed = 3;
> 
> 

And then there are also sd_init_command(), sr_init_command(), 
st_init_command() in s[drt].c. sr_init_command() looks OK to me, the 
other two feature the same flaw like scsi_prep_fn().

Or is there any reason why they should set DMA_TO_DEVICE while 
rq->data_len == 0?

Furthermore, is there any other code path which may set 
cmd->sc_data_direction, aside from s[drt]_init_command() and 
scsi_prep_fn()? I was just browsing the code to find out myself but am 
not 100% sure.
-- 
Stefan Richter
-=====-=-=-= ==-- -=--=
http://arcgraph.de/sr/

Re: linux kernel panic when ejecting ieee1394 ipod

[Stefan Richter]

scsi: dont allow DMA_TO_DEVICE with zero data length

When preparing a request in scsi_lib or in a SCSI high-level driver,
always set a transfer direction of DMA_NONE if data length is zero,
even for alleged write requests. (Extended patch derived from Jens
Axboe's version.)

Write requests with request buffer length == 0 lead to kernel panic
or oops if channeled through sbp2:
http://marc.theaimsgroup.com/?l=linux1394-devel&m=113399994920181
http://marc.theaimsgroup.com/?l=linux1394-user&m=112152701817435

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

---
 drivers/scsi/scsi_lib.c |    8 ++++----
 drivers/scsi/sd.c       |    8 ++++----
 drivers/scsi/st.c       |    8 ++++----
 3 files changed, 12 insertions(+), 12 deletions(-)

diff -uprN -X linux/Documentation/dontdiff linux/drivers/scsi.orig/scsi_lib.c linux/drivers/scsi/scsi_lib.c
--- linux/drivers/scsi.orig/scsi_lib.c	2005-11-24 23:10:21.000000000 +0100
+++ linux/drivers/scsi/scsi_lib.c	2005-12-09 20:11:59.000000000 +0100
@@ -1266,12 +1266,12 @@ static int scsi_prep_fn(struct request_q
 		} else {
 			memcpy(cmd->cmnd, req->cmd, sizeof(cmd->cmnd));
 			cmd->cmd_len = req->cmd_len;
-			if (rq_data_dir(req) == WRITE)
+			if (!req->data_len)
+				cmd->sc_data_direction = DMA_NONE;
+			else if (rq_data_dir(req) == WRITE)
 				cmd->sc_data_direction = DMA_TO_DEVICE;
-			else if (req->data_len)
-				cmd->sc_data_direction = DMA_FROM_DEVICE;
 			else
-				cmd->sc_data_direction = DMA_NONE;
+				cmd->sc_data_direction = DMA_FROM_DEVICE;
 			
 			cmd->transfersize = req->data_len;
 			cmd->allowed = 3;
diff -uprN -X linux/Documentation/dontdiff linux/drivers/scsi.orig/sd.c linux/drivers/scsi/sd.c
--- linux/drivers/scsi.orig/sd.c	2005-11-24 23:10:21.000000000 +0100
+++ linux/drivers/scsi/sd.c	2005-12-09 20:13:12.000000000 +0100
@@ -236,12 +236,12 @@ static int sd_init_command(struct scsi_c
 
 		memcpy(SCpnt->cmnd, rq->cmd, sizeof(SCpnt->cmnd));
 		SCpnt->cmd_len = rq->cmd_len;
-		if (rq_data_dir(rq) == WRITE)
+		if (!rq->data_len)
+			SCpnt->sc_data_direction = DMA_NONE;
+		else if (rq_data_dir(rq) == WRITE)
 			SCpnt->sc_data_direction = DMA_TO_DEVICE;
-		else if (rq->data_len)
-			SCpnt->sc_data_direction = DMA_FROM_DEVICE;
 		else
-			SCpnt->sc_data_direction = DMA_NONE;
+			SCpnt->sc_data_direction = DMA_FROM_DEVICE;
 
 		this_count = rq->data_len;
 		if (rq->timeout)
diff -uprN -X linux/Documentation/dontdiff linux/drivers/scsi.orig/st.c linux/drivers/scsi/st.c
--- linux/drivers/scsi.orig/st.c	2005-11-24 23:10:21.000000000 +0100
+++ linux/drivers/scsi/st.c	2005-12-09 20:14:29.000000000 +0100
@@ -4208,12 +4208,12 @@ static int st_init_command(struct scsi_c
 	memcpy(SCpnt->cmnd, rq->cmd, sizeof(SCpnt->cmnd));
 	SCpnt->cmd_len = rq->cmd_len;
 
-	if (rq_data_dir(rq) == WRITE)
+	if (!rq->data_len)
+		SCpnt->sc_data_direction = DMA_NONE;
+	else if (rq_data_dir(rq) == WRITE)
 		SCpnt->sc_data_direction = DMA_TO_DEVICE;
-	else if (rq->data_len)
-		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
 	else
-		SCpnt->sc_data_direction = DMA_NONE;
+		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
 
 	SCpnt->timeout_per_command = rq->timeout;
 	SCpnt->transfersize = rq->data_len;




-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: linux kernel panic when ejecting ieee1394 ipod

[James Bottomley]

On Fri, 2005-12-09 at 20:35 +0100, Stefan Richter wrote:
> When preparing a request in scsi_lib or in a SCSI high-level driver,
> always set a transfer direction of DMA_NONE if data length is zero,
> even for alleged write requests. (Extended patch derived from Jens
> Axboe's version.)
> 
> Write requests with request buffer length == 0 lead to kernel panic
> or oops if channeled through sbp2:
> http://marc.theaimsgroup.com/?l=linux1394-devel&m=113399994920181
> http://marc.theaimsgroup.com/?l=linux1394-user&m=112152701817435
> 
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

The problem is that I already picked up Jens' patch in rc fixes, so this
no-longer applies.  However, given that the fix needed to be in four
separate places, which looks rather bad, I propose the following
consolidation instead.

James

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1078,6 +1078,26 @@ static void scsi_generic_done(struct scs
 	scsi_io_completion(cmd, cmd->result == 0 ? cmd->bufflen : 0, 0);
 }
 
+void scsi_setup_blk_pc_cmnd(struct scsi_cmnd *cmd, int retries)
+{
+	struct request *req = cmd->request;
+
+	BUG_ON(sizeof(req->cmd) > sizeof(cmd->cmnd));
+	memcpy(cmd->cmnd, req->cmd, sizeof(cmd->cmnd));
+	cmd->cmd_len = req->cmd_len;
+	if (!req->data_len)
+		cmd->sc_data_direction = DMA_NONE;
+	else if (rq_data_dir(req) == WRITE)
+		cmd->sc_data_direction = DMA_TO_DEVICE;
+	else
+		cmd->sc_data_direction = DMA_FROM_DEVICE;
+	
+	cmd->transfersize = req->data_len;
+	cmd->allowed = retries;
+	cmd->timeout_per_command = req->timeout;
+}
+EXPORT_SYMBOL(scsi_setup_blk_pc_cmnd);
+
 static int scsi_prep_fn(struct request_queue *q, struct request *req)
 {
 	struct scsi_device *sdev = q->queuedata;
@@ -1213,18 +1233,7 @@ static int scsi_prep_fn(struct request_q
 				goto kill;
 			}
 		} else {
-			memcpy(cmd->cmnd, req->cmd, sizeof(cmd->cmnd));
-			cmd->cmd_len = req->cmd_len;
-			if (!req->data_len)
-				cmd->sc_data_direction = DMA_NONE;
-			else if (rq_data_dir(req) == WRITE)
-				cmd->sc_data_direction = DMA_TO_DEVICE;
-			else
-				cmd->sc_data_direction = DMA_FROM_DEVICE;
-			
-			cmd->transfersize = req->data_len;
-			cmd->allowed = 3;
-			cmd->timeout_per_command = req->timeout;
+			scsi_setup_blk_pc_cmnd(cmd, 3);
 			cmd->done = scsi_generic_done;
 		}
 	}
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -245,24 +245,10 @@ static int sd_init_command(struct scsi_c
 	 * SG_IO from block layer already setup, just copy cdb basically
 	 */
 	if (blk_pc_request(rq)) {
-		if (sizeof(rq->cmd) > sizeof(SCpnt->cmnd))
-			return 0;
-
-		memcpy(SCpnt->cmnd, rq->cmd, sizeof(SCpnt->cmnd));
-		SCpnt->cmd_len = rq->cmd_len;
-		if (rq_data_dir(rq) == WRITE)
-			SCpnt->sc_data_direction = DMA_TO_DEVICE;
-		else if (rq->data_len)
-			SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-		else
-			SCpnt->sc_data_direction = DMA_NONE;
-
-		this_count = rq->data_len;
+		scsi_setup_blk_pc_cmnd(SCpnt, SD_PASSTHROUGH_RETRIES);
 		if (rq->timeout)
 			timeout = rq->timeout;
 
-		SCpnt->transfersize = rq->data_len;
-		SCpnt->allowed = SD_PASSTHROUGH_RETRIES;
 		goto queue;
 	}
 
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -320,25 +320,11 @@ static int sr_init_command(struct scsi_c
 	 * these are already setup, just copy cdb basically
 	 */
 	if (SCpnt->request->flags & REQ_BLOCK_PC) {
-		struct request *rq = SCpnt->request;
+		scsi_setup_blk_pc_cmnd(SCpnt, MAX_RETRIES);
 
-		if (sizeof(rq->cmd) > sizeof(SCpnt->cmnd))
-			return 0;
-
-		memcpy(SCpnt->cmnd, rq->cmd, sizeof(SCpnt->cmnd));
-		SCpnt->cmd_len = rq->cmd_len;
-		if (!rq->data_len)
-			SCpnt->sc_data_direction = DMA_NONE;
-		else if (rq_data_dir(rq) == WRITE)
-			SCpnt->sc_data_direction = DMA_TO_DEVICE;
-		else
-			SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-
-		this_count = rq->data_len;
-		if (rq->timeout)
-			timeout = rq->timeout;
+		if (SCpnt->timeout_per_command)
+			timeout = SCpnt->timeout_per_command;
 
-		SCpnt->transfersize = rq->data_len;
 		goto queue;
 	}
 
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -4194,27 +4194,10 @@ static void st_intr(struct scsi_cmnd *SC
  */
 static int st_init_command(struct scsi_cmnd *SCpnt)
 {
-	struct request *rq;
-
 	if (!(SCpnt->request->flags & REQ_BLOCK_PC))
 		return 0;
 
-	rq = SCpnt->request;
-	if (sizeof(rq->cmd) > sizeof(SCpnt->cmnd))
-		return 0;
-
-	memcpy(SCpnt->cmnd, rq->cmd, sizeof(SCpnt->cmnd));
-	SCpnt->cmd_len = rq->cmd_len;
-
-	if (rq_data_dir(rq) == WRITE)
-		SCpnt->sc_data_direction = DMA_TO_DEVICE;
-	else if (rq->data_len)
-		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-	else
-		SCpnt->sc_data_direction = DMA_NONE;
-
-	SCpnt->timeout_per_command = rq->timeout;
-	SCpnt->transfersize = rq->data_len;
+	scsi_setup_blk_pc_cmnd(SCpnt, 0);
 	SCpnt->done = st_intr;
 	return 1;
 }




-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: linux kernel panic when ejecting ieee1394 ipod

[Jens Axboe]

On Fri, Dec 09 2005, Stefan Richter wrote:
> scsi: dont allow DMA_TO_DEVICE with zero data length
> 
> When preparing a request in scsi_lib or in a SCSI high-level driver,
> always set a transfer direction of DMA_NONE if data length is zero,
> even for alleged write requests. (Extended patch derived from Jens
> Axboe's version.)
> 
> Write requests with request buffer length == 0 lead to kernel panic
> or oops if channeled through sbp2:
> http://marc.theaimsgroup.com/?l=linux1394-devel&m=113399994920181
> http://marc.theaimsgroup.com/?l=linux1394-user&m=112152701817435
> 
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

My quick grep just showed the one, I'd encourage you to turn this into a
static inline function instead of duplicating the same code again 3 (or
more) times.

-- 
Jens Axboe

Re: linux kernel panic when ejecting ieee1394 ipod

[Christoph Hellwig]

On Sat, Dec 10, 2005 at 09:48:33AM +0100, Jens Axboe wrote:
> On Fri, Dec 09 2005, Stefan Richter wrote:
> > scsi: dont allow DMA_TO_DEVICE with zero data length
> > 
> > When preparing a request in scsi_lib or in a SCSI high-level driver,
> > always set a transfer direction of DMA_NONE if data length is zero,
> > even for alleged write requests. (Extended patch derived from Jens
> > Axboe's version.)
> > 
> > Write requests with request buffer length == 0 lead to kernel panic
> > or oops if channeled through sbp2:
> > http://marc.theaimsgroup.com/?l=linux1394-devel&m=113399994920181
> > http://marc.theaimsgroup.com/?l=linux1394-user&m=112152701817435
> > 
> > Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
> 
> My quick grep just showed the one, I'd encourage you to turn this into a
> static inline function instead of duplicating the same code again 3 (or
> more) times.

Actually Mike has code pending to remove all copies but the scsi_lib.c
one.

Re: linux kernel panic when ejecting ieee1394 ipod

[Stefan Richter]

Christoph Hellwig wrote:
> On Sat, Dec 10, 2005 at 09:48:33AM +0100, Jens Axboe wrote:
>>On Fri, Dec 09 2005, Stefan Richter wrote:
>>>scsi: dont allow DMA_TO_DEVICE with zero data length
>>>
>>>When preparing a request in scsi_lib or in a SCSI high-level driver,
>>>always set a transfer direction of DMA_NONE if data length is zero,
>>>even for alleged write requests. (Extended patch derived from Jens
>>>Axboe's version.)
...
>>My quick grep just showed the one, I'd encourage you to turn this into a
>>static inline function instead of duplicating the same code again 3 (or
>>more) times.
> 
> Actually Mike has code pending to remove all copies but the scsi_lib.c
> one.

I was about to prepare a subsequent patch which collapses 4 x 9 lines of
common code of scsi_prep_fn and s[drt]_init_command. I guess Mike's work
goes _much_ further than that, so I'd like to leave it to him. :-)
-- 
Stefan Richter
-=====-=-=-= ==-- -=-=-
http://arcgraph.de/sr/


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: linux kernel panic when ejecting ieee1394 ipod

[Christoph Hellwig]

On Fri, Dec 09, 2005 at 05:45:22PM -0500, James Bottomley wrote:
> On Fri, 2005-12-09 at 20:35 +0100, Stefan Richter wrote:
> > When preparing a request in scsi_lib or in a SCSI high-level driver,
> > always set a transfer direction of DMA_NONE if data length is zero,
> > even for alleged write requests. (Extended patch derived from Jens
> > Axboe's version.)
> > 
> > Write requests with request buffer length == 0 lead to kernel panic
> > or oops if channeled through sbp2:
> > http://marc.theaimsgroup.com/?l=linux1394-devel&m=113399994920181
> > http://marc.theaimsgroup.com/?l=linux1394-user&m=112152701817435
> > 
> > Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
> 
> The problem is that I already picked up Jens' patch in rc fixes, so this
> no-longer applies.  However, given that the fix needed to be in four
> separate places, which looks rather bad, I propose the following
> consolidation instead.

Please make the export _GPL so people see it's internal and can go away
real soon.  Hopefull for 2.6.16 already when Mike's patch series is merged.

Re: linux kernel panic when ejecting ieee1394 ipod

[Stefan Richter]

James Bottomley wrote:
> On Fri, 2005-12-09 at 20:35 +0100, Stefan Richter wrote:
>>When preparing a request in scsi_lib or in a SCSI high-level driver,
>>always set a transfer direction of DMA_NONE if data length is zero,
>>even for alleged write requests. (Extended patch derived from Jens
>>Axboe's version.)
[...]
> The problem is that I already picked up Jens' patch in rc fixes, so this
> no-longer applies.  However, given that the fix needed to be in four
> separate places, which looks rather bad, I propose the following
> consolidation instead.

Jens' patch alone does not fix the kernel panic AFAICS. "eject /dev/sdX" 
goes through sd_init_command. James, could you put your patch into 
scsi-rc-fixes too? And what about -stable?

[BTW, some revisions of iPods require to either "eject" them or to 
unload sbp2 or to disassociate them from sbp2 via an arcane echo to 
ieee1394's sysfs interface before they can be unplugged. Else their 
firmware needs to be reset. This is why "eject" is the recommended 
procedure to disconnect an iPod.]

Thanks,
-- 
Stefan Richter
-=====-=-=-= ==-- -==-=
http://arcgraph.de/sr/

Re: linux kernel panic when ejecting ieee1394 ipod

[Jens Axboe]

On Tue, Dec 13 2005, Stefan Richter wrote:
> James Bottomley wrote:
> >On Fri, 2005-12-09 at 20:35 +0100, Stefan Richter wrote:
> >>When preparing a request in scsi_lib or in a SCSI high-level driver,
> >>always set a transfer direction of DMA_NONE if data length is zero,
> >>even for alleged write requests. (Extended patch derived from Jens
> >>Axboe's version.)
> [...]
> >The problem is that I already picked up Jens' patch in rc fixes, so this
> >no-longer applies.  However, given that the fix needed to be in four
> >separate places, which looks rather bad, I propose the following
> >consolidation instead.
> 
> Jens' patch alone does not fix the kernel panic AFAICS. "eject /dev/sdX" 
> goes through sd_init_command. James, could you put your patch into 
> scsi-rc-fixes too? And what about -stable?

I trust James already included his own updated version, which is
basically fixing that bug but abstracting the rq_to_scsi_dir() stuff
into a helper instead. That should fix it for all cases.

-- 
Jens Axboe



-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click