Bug 7667

Created attachment 9786 [details]
Full dmesg

On Mon, 11 Dec 2006 13:59:48 -0800
bugme-daemon@bugzilla.kernel.org wrote:

> http://bugzilla.kernel.org/show_bug.cgi?id=7667
> 
>            Summary: BUG at drivers/scsi/scsi_lib.c:1118 caused by "pktsetup
>                     dvd /dev/sr0"
>     Kernel Version: 2.6.19-rc6-mm2, 2.6.19-git*
>             Status: NEW
>           Severity: normal
>              Owner: scsi_drivers-other@kernel-bugs.osdl.org
>          Submitter: laurent.riffard@free.fr
> 
> 
> Most recent kernel where this bug did *NOT* occur: 2.6.19
> 
> Distribution: Mandriva Linux release 2007.0 (Official) for i586
> 
> Hardware Environment: Desktop
>   Motherboard: Asus A7V133-C,
>   Processor: AMD Athlon XP 1600+,
>   IDE DVD-burner: HL-DT-ST DVDRAM GSA-4165B 
> 
> Software Environment:
>   Relevant kernel modules: pata_via, scsi_mod, sr_mod, sd_mod, libata, pktcdvd
>   Userspace tools: udftools-1.0.0-0.b3.10mdv2007.0 
> 
> Problem Description: The kernel BUGs when I issue "pktsetup dvd /dev/sr0"
> 
> pktcdvd: writer pktcdvd0 mapped to sr0
> ------------[ cut here ]------------
> kernel BUG at drivers/scsi/scsi_lib.c:1118!
> invalid opcode: 0000 [#1]
> Modules linked in: pktcdvd snd_seq_oss snd_seq_midi_event snd_seq bonding
> snd_pcm_oss snd_mixer_oss af_packet snd_ens1371 gameport snd_rawmidi snd_seq_device
>  snd_ac97_codec snd_ac97_bus snd_pcm via686a snd_timer snd_page_alloc w83781d
> snd soundcore hwmon_vid i2c_isa i2c_viapro binfmt_misc loop nls_iso8859_15 nls_
> cp850 vfat fat reiserfs via_agp agpgart lp parport_pc parport 8250 serial_core
> pcspkr rtc ohci1394 ieee1394 uhci_hcd usbcore sr_mod cdrom dm_mirror dm_mod sd
> _mod pata_via libata scsi_mod
> CPU:    0
> EIP:    0060:[<e0833017>]    Not tainted VLI
> EFLAGS: 00010002   (2.6.19-gaf1713e0 #5)
> EIP is at scsi_prep_fn+0x10b/0x24f [scsi_mod]
> eax: c15f9c34   ebx: dfc00be4   ecx: dfc39628   edx: c15f9c34
> esi: dfc39598   edi: c15f7448   ebp: c156daf0   esp: c156dad0
> ds: 007b   es: 007b   ss: 0068
> Process vol_id (pid: 2099, ti=c156c000 task=dff6a570 task.ti=c156c000)
> Stack: 00011200 c1479de0 00000833 c15f9c34 c15f7448 c15f9c34 c15f7448 c15f7448 
>        c156db14 c019ed65 dfc23d28 dfc39690 c156db0c c15f9c34 dfc39598 dfc34000 
>        c15f7448 c156db44 e08335db 00000082 00000082 c15f7448 dfc39628 c01a1465 
> Call Trace:
>  [<c0103a92>] show_trace_log_lvl+0x1a/0x2f
>  [<c0103b42>] show_stack_log_lvl+0x9b/0xa3
>  [<c0103cdc>] show_registers+0x192/0x268
>  [<c0103e9c>] die+0xea/0x1bd
>  [<c0103fe8>] do_trap+0x79/0x91
>  [<c01047c6>] do_invalid_op+0x97/0xa1
>  [<c027fac4>] error_code+0x74/0x7c
>  [<c019ed65>] elv_next_request+0x6b/0x116
>  [<e08335db>] scsi_request_fn+0x5e/0x26d [scsi_mod]
>  [<c019ee6a>] elv_insert+0x5a/0x134
>  [<c019efc1>] __elv_add_request+0x7d/0x82
>  [<c019f0ab>] elv_add_request+0x16/0x1d
>  [<e0e8d2ed>] pkt_generic_packet+0x107/0x133 [pktcdvd]
>  [<e0e8d772>] pkt_get_disc_info+0x42/0x7b [pktcdvd]
>  [<e0e8eae3>] pkt_open+0xbf/0xc56 [pktcdvd]
>  [<c0168078>] do_open+0x7e/0x246
>  [<c01683df>] blkdev_open+0x28/0x51
>  [<c014a057>] __dentry_open+0xb5/0x160
>  [<c014a183>] nameidata_to_filp+0x27/0x37
>  [<c014a1c6>] do_filp_open+0x33/0x3b
>  [<c014a211>] do_sys_open+0x43/0xc7
>  [<c014a2cd>] sys_open+0x1c/0x1e
>  [<c0102b82>] sysenter_past_esp+0x5f/0x85
>  =======================
> Code: 74 1d 66 83 78 60 00 75 04 0f 0b eb fe 89 d8 e8 3d fe ff ff 85 c0 89 c7 74
> 43 e9 00 01 00 00 8b 55 ec 83 ba 90 00 00 00 00 74 04 <0f> 0b eb fe 8b 45 ec
>  83 b8 98 00 00 00 00 74 04 0f 0b eb fe c7 
> EIP: [<e0833017>] scsi_prep_fn+0x10b/0x24f [scsi_mod] SS:ESP 0068:c156dad0
> 
> 
> Steps to reproduce:
> - load an UDF-formatted DVD-RW
> - issue "modprobe pktcdvd"
> - issue "pktsetup dvd /dev/sr0"
> 
> Notes:
> 
> I did a git-bisect, which gave this result:
>   commit b00315799d78f76531b71435fbc2643cd71ae4c
> (http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=3b00315799d78f76531b71435fbc2643cd71ae4c)
>   Author: Christoph Hellwig <hch@lst.de>
>   Date:   Sat Nov 4 20:10:55 2006 +0100
>   [SCSI] untangle scsi_prep_fn
> 

Reply-To: James.Bottomley@SteelEye.com

On Mon, 2006-12-11 at 14:10 -0800, Andrew Morton wrote:
> > pktcdvd: writer pktcdvd0 mapped to sr0
> > ------------[ cut here ]------------
> > kernel BUG at drivers/scsi/scsi_lib.c:1118!
> > invalid opcode: 0000 [#1]
> > Modules linked in: pktcdvd snd_seq_oss snd_seq_midi_event snd_seq bonding
> > snd_pcm_oss snd_mixer_oss af_packet snd_ens1371 gameport snd_rawmidi snd_seq_device
> >  snd_ac97_codec snd_ac97_bus snd_pcm via686a snd_timer snd_page_alloc w83781d
> > snd soundcore hwmon_vid i2c_isa i2c_viapro binfmt_misc loop nls_iso8859_15 nls_
> > cp850 vfat fat reiserfs via_agp agpgart lp parport_pc parport 8250 serial_core
> > pcspkr rtc ohci1394 ieee1394 uhci_hcd usbcore sr_mod cdrom dm_mirror dm_mod sd
> > _mod pata_via libata scsi_mod
> > CPU:    0
> > EIP:    0060:[<e0833017>]    Not tainted VLI
> > EFLAGS: 00010002   (2.6.19-gaf1713e0 #5)
> > EIP is at scsi_prep_fn+0x10b/0x24f [scsi_mod]
> > eax: c15f9c34   ebx: dfc00be4   ecx: dfc39628   edx: c15f9c34
> > esi: dfc39598   edi: c15f7448   ebp: c156daf0   esp: c156dad0
> > ds: 007b   es: 007b   ss: 0068
> > Process vol_id (pid: 2099, ti=c156c000 task=dff6a570 task.ti=c156c000)
> > Stack: 00011200 c1479de0 00000833 c15f9c34 c15f7448 c15f9c34 c15f7448 c15f7448 
> >        c156db14 c019ed65 dfc23d28 dfc39690 c156db0c c15f9c34 dfc39598 dfc34000 
> >        c15f7448 c156db44 e08335db 00000082 00000082 c15f7448 dfc39628 c01a1465 
> > Call Trace:
> >  [<c0103a92>] show_trace_log_lvl+0x1a/0x2f
> >  [<c0103b42>] show_stack_log_lvl+0x9b/0xa3
> >  [<c0103cdc>] show_registers+0x192/0x268
> >  [<c0103e9c>] die+0xea/0x1bd
> >  [<c0103fe8>] do_trap+0x79/0x91
> >  [<c01047c6>] do_invalid_op+0x97/0xa1
> >  [<c027fac4>] error_code+0x74/0x7c
> >  [<c019ed65>] elv_next_request+0x6b/0x116
> >  [<e08335db>] scsi_request_fn+0x5e/0x26d [scsi_mod]
> >  [<c019ee6a>] elv_insert+0x5a/0x134
> >  [<c019efc1>] __elv_add_request+0x7d/0x82
> >  [<c019f0ab>] elv_add_request+0x16/0x1d
> >  [<e0e8d2ed>] pkt_generic_packet+0x107/0x133 [pktcdvd]
> >  [<e0e8d772>] pkt_get_disc_info+0x42/0x7b [pktcdvd]
> >  [<e0e8eae3>] pkt_open+0xbf/0xc56 [pktcdvd]
> >  [<c0168078>] do_open+0x7e/0x246
> >  [<c01683df>] blkdev_open+0x28/0x51
> >  [<c014a057>] __dentry_open+0xb5/0x160
> >  [<c014a183>] nameidata_to_filp+0x27/0x37
> >  [<c014a1c6>] do_filp_open+0x33/0x3b
> >  [<c014a211>] do_sys_open+0x43/0xc7
> >  [<c014a2cd>] sys_open+0x1c/0x1e
> >  [<c0102b82>] sysenter_past_esp+0x5f/0x85
> >  =======================
> > Code: 74 1d 66 83 78 60 00 75 04 0f 0b eb fe 89 d8 e8 3d fe ff ff 85 c0 89 c7 74
> > 43 e9 00 01 00 00 8b 55 ec 83 ba 90 00 00 00 00 74 04 <0f> 0b eb fe 8b 45 ec
> >  83 b8 98 00 00 00 00 74 04 0f 0b eb fe c7 
> > EIP: [<e0833017>] scsi_prep_fn+0x10b/0x24f [scsi_mod] SS:ESP 0068:c156dad0
> > 
> > 
> > Steps to reproduce:
> > - load an UDF-formatted DVD-RW
> > - issue "modprobe pktcdvd"
> > - issue "pktsetup dvd /dev/sr0"
> > 
> > Notes:
> > 
> > I did a git-bisect, which gave this result:
> >   commit b00315799d78f76531b71435fbc2643cd71ae4c
> > (http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=3b00315799d78f76531b71435fbc2643cd71ae4c)
> >   Author: Christoph Hellwig <hch@lst.de>
> >   Date:   Sat Nov 4 20:10:55 2006 +0100
> >   [SCSI] untangle scsi_prep_fn
> > 

This is as a result of extra checking introduced into the prep function.
What it's BUGgin on is the fact that the request has no bio with
non-zero transfer length.  This seems to be a legitimate complaint
precipitated by the fact that pkt_generic_packet doesn't call
blk_rq_map_kern which would have generated the bio.

James

Reply-To: hch@lst.de

This is because the packet driver tries to send down read/write
BLOCK_PC commands that don't use a bio and do not use sg lists.
As part of the patch you mentioned I added strict assertations for that
case because the scsi layer doesn't handle those anymore.

The right fix is to replace all the packet_command stuff in the packet
driver by scsi_execute() which needs to be lifted from scsi code to
the block code for that.  I'll prepare a patch this weekend unless
someone beets me in doing that work.

Reply-To: hch@lst.de

On Tue, Dec 12, 2006 at 11:38:42AM +0100, Christoph Hellwig wrote:
> This is because the packet driver tries to send down read/write
> BLOCK_PC commands that don't use a bio and do not use sg lists.
> As part of the patch you mentioned I added strict assertations for that
> case because the scsi layer doesn't handle those anymore.
> 
> The right fix is to replace all the packet_command stuff in the packet
> driver by scsi_execute() which needs to be lifted from scsi code to
> the block code for that.  I'll prepare a patch this weekend unless
> someone beets me in doing that work.

Please try the patch below to fix the bug for now.  It's not the
full way to a generic execute block pc infrastcuture but should fix
the bug for the time beeing:


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/block/pktcdvd.c
===================================================================
--- linux-2.6.orig/drivers/block/pktcdvd.c	2006-12-12 11:30:45.000000000 +0100
+++ linux-2.6/drivers/block/pktcdvd.c	2006-12-12 14:23:37.000000000 +0100
@@ -765,47 +765,34 @@
  */
 static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *cgc)
 {
-	char sense[SCSI_SENSE_BUFFERSIZE];
-	request_queue_t *q;
+	request_queue_t *q = bdev_get_queue(pd->bdev);
 	struct request *rq;
-	DECLARE_COMPLETION_ONSTACK(wait);
-	int err = 0;
+	int ret = 0;
 
-	q = bdev_get_queue(pd->bdev);
+	rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ?
+			     WRITE : READ, __GFP_WAIT);
+
+	if (cgc->buflen) {
+		if (blk_rq_map_kern(q, rq, cgc->buffer, cgc->buflen, __GFP_WAIT))
+			goto out;
+	}
+
+	rq->cmd_len = COMMAND_SIZE(rq->cmd[0]);
+	memcpy(rq->cmd, cgc->cmd, CDROM_PACKET_SIZE);
+	if (sizeof(rq->cmd) > CDROM_PACKET_SIZE)
+		memset(rq->cmd + CDROM_PACKET_SIZE, 0, sizeof(rq->cmd) - CDROM_PACKET_SIZE);
 
-	rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ? WRITE : READ,
-			     __GFP_WAIT);
-	rq->errors = 0;
-	rq->rq_disk = pd->bdev->bd_disk;
-	rq->bio = NULL;
-	rq->buffer = NULL;
 	rq->timeout = 60*HZ;
-	rq->data = cgc->buffer;
-	rq->data_len = cgc->buflen;
-	rq->sense = sense;
-	memset(sense, 0, sizeof(sense));
-	rq->sense_len = 0;
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 	rq->cmd_flags |= REQ_HARDBARRIER;
 	if (cgc->quiet)
 		rq->cmd_flags |= REQ_QUIET;
-	memcpy(rq->cmd, cgc->cmd, CDROM_PACKET_SIZE);
-	if (sizeof(rq->cmd) > CDROM_PACKET_SIZE)
-		memset(rq->cmd + CDROM_PACKET_SIZE, 0, sizeof(rq->cmd) - CDROM_PACKET_SIZE);
-	rq->cmd_len = COMMAND_SIZE(rq->cmd[0]);
-
-	rq->ref_count++;
-	rq->end_io_data = &wait;
-	rq->end_io = blk_end_sync_rq;
-	elv_add_request(q, rq, ELEVATOR_INSERT_BACK, 1);
-	generic_unplug_device(q);
-	wait_for_completion(&wait);
-
-	if (rq->errors)
-		err = -EIO;
 
+	blk_execute_rq(rq->q, pd->bdev->bd_disk, rq, 0);
+	ret = rq->errors;
+out:
 	blk_put_request(rq);
-	return err;
+	return ret;
 }
 
 /*

Reply-To: bharrosh@panasas.com

Christoph Hellwig wrote:
> On Tue, Dec 12, 2006 at 11:38:42AM +0100, Christoph Hellwig wrote:
>> This is because the packet driver tries to send down read/write
>> BLOCK_PC commands that don't use a bio and do not use sg lists.
>> As part of the patch you mentioned I added strict assertations for that
>> case because the scsi layer doesn't handle those anymore.
>>
>> The right fix is to replace all the packet_command stuff in the packet
>> driver by scsi_execute() which needs to be lifted from scsi code to
>> the block code for that.  I'll prepare a patch this weekend unless
>> someone beets me in doing that work.
> 
> Please try the patch below to fix the bug for now.  It's not the
> full way to a generic execute block pc infrastcuture but should fix
> the bug for the time beeing:
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: linux-2.6/drivers/block/pktcdvd.c
> ===================================================================
> --- linux-2.6.orig/drivers/block/pktcdvd.c	2006-12-12 11:30:45.000000000 +0100
> +++ linux-2.6/drivers/block/pktcdvd.c	2006-12-12 14:23:37.000000000 +0100
> @@ -765,47 +765,34 @@
>   */
>  static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *cgc)
>  {
> -	char sense[SCSI_SENSE_BUFFERSIZE];
> -	request_queue_t *q;
> +	request_queue_t *q = bdev_get_queue(pd->bdev);
>  	struct request *rq;
> -	DECLARE_COMPLETION_ONSTACK(wait);
> -	int err = 0;
> +	int ret = 0;
>  
> -	q = bdev_get_queue(pd->bdev);
> +	rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ?
> +			     WRITE : READ, __GFP_WAIT);
> +
> +	if (cgc->buflen) {
> +		if (blk_rq_map_kern(q, rq, cgc->buffer, cgc->buflen, __GFP_WAIT))
> +			goto out;
> +	}
> +
> +	rq->cmd_len = COMMAND_SIZE(rq->cmd[0]);
> +	memcpy(rq->cmd, cgc->cmd, CDROM_PACKET_SIZE);
> +	if (sizeof(rq->cmd) > CDROM_PACKET_SIZE)
> +		memset(rq->cmd + CDROM_PACKET_SIZE, 0, sizeof(rq->cmd) - CDROM_PACKET_SIZE);
>  
> -	rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ? WRITE : READ,
> -			     __GFP_WAIT);
> -	rq->errors = 0;
> -	rq->rq_disk = pd->bdev->bd_disk;
> -	rq->bio = NULL;
> -	rq->buffer = NULL;
>  	rq->timeout = 60*HZ;
> -	rq->data = cgc->buffer;
> -	rq->data_len = cgc->buflen;
> -	rq->sense = sense;
> -	memset(sense, 0, sizeof(sense));
> -	rq->sense_len = 0;
>  	rq->cmd_type = REQ_TYPE_BLOCK_PC;
>  	rq->cmd_flags |= REQ_HARDBARRIER;
>  	if (cgc->quiet)
>  		rq->cmd_flags |= REQ_QUIET;
> -	memcpy(rq->cmd, cgc->cmd, CDROM_PACKET_SIZE);
> -	if (sizeof(rq->cmd) > CDROM_PACKET_SIZE)
> -		memset(rq->cmd + CDROM_PACKET_SIZE, 0, sizeof(rq->cmd) - CDROM_PACKET_SIZE);
> -	rq->cmd_len = COMMAND_SIZE(rq->cmd[0]);
> -
> -	rq->ref_count++;
> -	rq->end_io_data = &wait;
> -	rq->end_io = blk_end_sync_rq;
> -	elv_add_request(q, rq, ELEVATOR_INSERT_BACK, 1);
> -	generic_unplug_device(q);
> -	wait_for_completion(&wait);
> -
> -	if (rq->errors)
> -		err = -EIO;
>  
> +	blk_execute_rq(rq->q, pd->bdev->bd_disk, rq, 0);
> +	ret = rq->errors;
> +out:
>  	blk_put_request(rq);
> -	return err;
> +	return ret;
>  }
>  
>  /*
> -
> 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
I'm afraid this might not be enough because of code in drivers/ide/ide-cd.c. It does IO off of request->data.

[background]
pkt_generic_packet and ton of other places mainly cd(s), floppy(s), and other ide code paths,
are using what I call BLACK requests. They put some data on request->buffer or request->data
stick it in the Q and than advance on them later down the ladder. Remove of "buffer" and "data" from
struct request will reveal all these places. At one time I had plans to do just that. But 1/2 way through I gave
up, it is too risky, too much Hardware that I don't have, that needs checking.

below patch combined with your patch might get a bit closer for this code path.
At struct request I have changed the name of "data" member to "user_data". than changed the code paths that used
"data" as IO to use request->buffer instead. This is just as bad but is a more common practice.

I suspect there is a problem with what I did in scsi_lib.c Christoph please check me out with the new BUG_ON.
Mainly what you need from below is only the code in ide-cd.c.
(And there are 3-4 places that do exactly like pkt_generic_packet, though I'm not sure they end up through SCSI.
At first I thought this code doesn't either)

diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index 9eaee66..f52a4f2 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -257,7 +257,7 @@ static void rq_init(request_queue_t *q,
 	rq->q = q;
 	rq->special = NULL;
 	rq->data_len = 0;
-	rq->data = NULL;
+	rq->user_data = NULL;
 	rq->nr_phys_segments = 0;
 	rq->sense = NULL;
 	rq->end_io = NULL;
@@ -1199,7 +1199,7 @@ void blk_dump_rq_flags(struct request *r
 	printk("\nsector %llu, nr/cnr %lu/%u\n", (unsigned long long)rq->sector,
 						       rq->nr_sectors,
 						       rq->current_nr_sectors);
-	printk("bio %p, biotail %p, buffer %p, data %p, len %u\n", rq->bio, rq->biotail, rq->buffer, rq->data, rq->data_len);
+	printk("bio %p, biotail %p, buffer %p, user_data %p, len %u\n", rq->bio, rq->biotail, rq->buffer, rq->user_data, rq->data_len);

 	if (blk_pc_request(rq)) {
 		printk("cdb: ");
@@ -2370,7 +2370,7 @@ int blk_rq_map_user(request_queue_t *q,
 		rq->bio = rq->biotail = bio;
 		blk_rq_bio_prep(q, rq, bio);

-		rq->buffer = rq->data = NULL;
+		rq->buffer = NULL;
 		rq->data_len = len;
 		return 0;
 	}
@@ -2420,7 +2420,7 @@ int blk_rq_map_user_iov(request_queue_t

 	rq->bio = rq->biotail = bio;
 	blk_rq_bio_prep(q, rq, bio);
-	rq->buffer = rq->data = NULL;
+	rq->buffer = NULL;
 	rq->data_len = bio->bi_size;
 	return 0;
 }
@@ -2479,7 +2479,7 @@ int blk_rq_map_kern(request_queue_t *q,
 	rq->bio = rq->biotail = bio;
 	blk_rq_bio_prep(q, rq, bio);

-	rq->buffer = rq->data = NULL;
+	rq->buffer = NULL;
 	rq->data_len = len;
 	return 0;
 }
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index dcd9c71..d163a2c 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -502,7 +502,6 @@ static int __blk_send_generic(request_qu

 	rq = blk_get_request(q, WRITE, __GFP_WAIT);
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
-	rq->data = NULL;
 	rq->data_len = 0;
 	rq->timeout = BLK_DEFAULT_TIMEOUT;
 	memset(rq->cmd, 0, sizeof(rq->cmd));
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index f2904f6..b72758c 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -360,9 +360,8 @@ static int pkt_generic_packet(struct pkt
 	rq->errors = 0;
 	rq->rq_disk = pd->bdev->bd_disk;
 	rq->bio = NULL;
-	rq->buffer = NULL;
 	rq->timeout = 60*HZ;
-	rq->data = cgc->buffer;
+	rq->buffer = cgc->buffer;
 	rq->data_len = cgc->buflen;
 	rq->sense = sense;
 	memset(sense, 0, sizeof(sense));
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 8821494..00fce03 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -613,14 +613,14 @@ static void cdrom_queue_request_sense(id
 	/* stuff the sense request in front of our current request */
 	cdrom_prepare_request(drive, rq);

-	rq->data = sense;
+	rq->buffer = sense;
 	rq->cmd[0] = GPCMD_REQUEST_SENSE;
 	rq->cmd[4] = rq->data_len = 18;

 	rq->cmd_type = REQ_TYPE_SENSE;

-	/* NOTE! Save the failed command in "rq->buffer" */
-	rq->buffer = (void *) failed_command;
+	/* NOTE! Save the failed command in "rq->user_data" */
+	rq->user_data = failed_command;

 	(void) ide_do_drive_cmd(drive, rq, ide_preempt);
 }
@@ -632,10 +632,10 @@ static void cdrom_end_request (ide_drive

 	if (blk_sense_request(rq) && uptodate) {
 		/*
-		 * For REQ_TYPE_SENSE, "rq->buffer" points to the original
+		 * For REQ_TYPE_SENSE, "rq->user_data" points to the original
 		 * failed request
 		 */
-		struct request *failed = (struct request *) rq->buffer;
+		struct request *failed = rq->user_data;
 		struct cdrom_info *info = drive->driver_data;
 		void *sense = &info->sense_data;
 		unsigned long flags;
@@ -1441,7 +1441,7 @@ static ide_startstop_t cdrom_pc_intr (id
 		    rq->data_len > 0 &&
 		    rq->data_len <= 5) {
 			while (rq->data_len > 0) {
-				*(unsigned char *)rq->data++ = 0;
+				*(unsigned char *)rq->buffer++ = 0;
 				--rq->data_len;
 			}
 		}
@@ -1468,12 +1468,12 @@ static ide_startstop_t cdrom_pc_intr (id

 	/* The drive wants to be written to. */
 	if ((ireason & 3) == 0) {
-		if (!rq->data) {
+		if (!rq->buffer) {
 			blk_dump_rq_flags(rq, "cdrom_pc_intr, write");
 			goto confused;
 		}
 		/* Transfer the data. */
-		HWIF(drive)->atapi_output_bytes(drive, rq->data, thislen);
+		HWIF(drive)->atapi_output_bytes(drive, rq->buffer, thislen);

 		/* If we haven't moved enough data to satisfy the drive,
 		   add some padding. */
@@ -1484,18 +1484,18 @@ static ide_startstop_t cdrom_pc_intr (id
 		}

 		/* Keep count of how much data we've moved. */
-		rq->data += thislen;
+		rq->buffer += thislen;
 		rq->data_len -= thislen;
 	}

 	/* Same drill for reading. */
 	else if ((ireason & 3) == 2) {
-		if (!rq->data) {
+		if (!rq->buffer) {
 			blk_dump_rq_flags(rq, "cdrom_pc_intr, write");
 			goto confused;
 		}
 		/* Transfer the data. */
-		HWIF(drive)->atapi_input_bytes(drive, rq->data, thislen);
+		HWIF(drive)->atapi_input_bytes(drive, rq->buffer, thislen);

 		/* If we haven't moved enough data to satisfy the drive,
 		   add some padding. */
@@ -1506,7 +1506,7 @@ static ide_startstop_t cdrom_pc_intr (id
 		}

 		/* Keep count of how much data we've moved. */
-		rq->data += thislen;
+		rq->buffer += thislen;
 		rq->data_len -= thislen;

 		if (blk_sense_request(rq))
@@ -1644,7 +1644,7 @@ static void post_transform_command(struc
 	if (req->bio)
 		ibuf = bio_data(req->bio);
 	else
-		ibuf = req->data;
+		ibuf = req->buffer;

 	if (!ibuf)
 		return;
@@ -1662,7 +1662,7 @@ typedef void (xfer_func_t)(ide_drive_t *

 /*
  * best way to deal with dma that is not sector aligned right now... note
- * that in this path we are not using ->data or ->buffer at all. this irs
+ * that in this path we are not using ->buffer at all. this irs
  * can replace cdrom_pc_intr, cdrom_read_intr, and cdrom_write_intr in the
  * future.
  */
@@ -1745,7 +1745,7 @@ static ide_startstop_t cdrom_newpc_intr(
 	 */
 	while (thislen > 0) {
 		int blen = blen = rq->data_len;
-		char *ptr = rq->data;
+		char *ptr = rq->buffer;

 		/*
 		 * bio backed?
@@ -1772,7 +1772,7 @@ static ide_startstop_t cdrom_newpc_intr(
 		if (rq->bio)
 			end_that_request_chunk(rq, 1, blen);
 		else
-			rq->data += blen;
+			rq->buffer += blen;
 	}

 	/*
@@ -2206,7 +2206,7 @@ static int cdrom_read_capacity(ide_drive

 	req.sense = sense;
 	req.cmd[0] = GPCMD_READ_CDVD_CAPACITY;
-	req.data = (char *)&capbuf;
+	req.buffer = (char *)&capbuf;
 	req.data_len = sizeof(capbuf);
 	req.cmd_flags |= REQ_QUIET;

@@ -2229,7 +2229,7 @@ static int cdrom_read_tocentry(ide_drive
 	cdrom_prepare_request(drive, &req);

 	req.sense = sense;
-	req.data =  buf;
+	req.buffer =  buf;
 	req.data_len = buflen;
 	req.cmd_flags |= REQ_QUIET;
 	req.cmd[0] = GPCMD_READ_TOC_PMA_ATIP;
@@ -2324,7 +2324,7 @@ #endif  /* not STANDARD_ATAPI */
 		   If we get an error for the regular case, we assume
 		   a CDI without additional audio tracks. In this case
 		   the readable TOC is empty (CDI tracks are not included)
-		   and only holds the Leadout entry. Heiko Ei�eldt */
+		   and only holds the Leadout entry. Heiko Ei�eldt */
 		ntracks = 0;
 		stat = cdrom_read_tocentry(drive, CDROM_LEADOUT, 1, 0,
 					   (char *)&toc->hdr,
@@ -2426,7 +2426,7 @@ static int cdrom_read_subchannel(ide_dri
 	cdrom_prepare_request(drive, &req);

 	req.sense = sense;
-	req.data = buf;
+	req.buffer = buf;
 	req.data_len = buflen;
 	req.cmd[0] = GPCMD_READ_SUBCHANNEL;
 	req.cmd[1] = 2;     /* MSF addressing */
@@ -2527,7 +2527,7 @@ static int ide_cdrom_packet(struct cdrom
 	memcpy(req.cmd, cgc->cmd, CDROM_PACKET_SIZE);
 	if (cgc->sense)
 		memset(cgc->sense, 0, sizeof(struct request_sense));
-	req.data = cgc->buffer;
+	req.buffer = cgc->buffer;
 	req.data_len = cgc->buflen;
 	req.timeout = cgc->timeout;

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index 2614f41..dbf0295 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -141,7 +141,7 @@ enum {

 static void ide_complete_power_step(ide_drive_t *drive, struct request *rq, u8 stat, u8 error)
 {
-	struct request_pm_state *pm = rq->data;
+	struct request_pm_state *pm = rq->user_data;

 	if (drive->media != ide_disk)
 		return;
@@ -167,7 +167,7 @@ static void ide_complete_power_step(ide_

 static ide_startstop_t ide_start_power_step(ide_drive_t *drive, struct request *rq)
 {
-	struct request_pm_state *pm = rq->data;
+	struct request_pm_state *pm = rq->user_data;
 	ide_task_t *args = rq->special;

 	memset(args, 0, sizeof(*args));
@@ -433,7 +433,7 @@ void ide_end_drive_cmd (ide_drive_t *dri
 			}
 		}
 	} else if (blk_pm_request(rq)) {
-		struct request_pm_state *pm = rq->data;
+		struct request_pm_state *pm = rq->user_data;
 #ifdef DEBUG_PM
 		printk("%s: complete_power_step(step: %d, stat: %x, err: %x)\n",
 			drive->name, rq->pm->pm_step, stat, err);
@@ -945,7 +945,7 @@ #endif

 static void ide_check_pm_state(ide_drive_t *drive, struct request *rq)
 {
-	struct request_pm_state *pm = rq->data;
+	struct request_pm_state *pm = rq->user_data;

 	if (blk_pm_suspend_request(rq) &&
 	    pm->pm_step == ide_pm_state_start_suspend)
@@ -1030,7 +1030,7 @@ #endif
 		    rq->cmd_type == REQ_TYPE_ATA_TASKFILE)
 			return execute_drive_cmd(drive, rq);
 		else if (blk_pm_request(rq)) {
-			struct request_pm_state *pm = rq->data;
+			struct request_pm_state *pm = rq->user_data;
 #ifdef DEBUG_PM
 			printk("%s: start_power_step(step: %d)\n",
 				drive->name, rq->pm->pm_step);
diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
index 287a662..47a6110 100644
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -1223,7 +1223,7 @@ static int generic_ide_suspend(struct de
 	memset(&args, 0, sizeof(args));
 	rq.cmd_type = REQ_TYPE_PM_SUSPEND;
 	rq.special = &args;
-	rq.data = &rqpm;
+	rq.user_data = &rqpm;
 	rqpm.pm_step = ide_pm_state_start_suspend;
 	if (mesg.event == PM_EVENT_PRETHAW)
 		mesg.event = PM_EVENT_FREEZE;
@@ -1244,7 +1244,7 @@ static int generic_ide_resume(struct dev
 	memset(&args, 0, sizeof(args));
 	rq.cmd_type = REQ_TYPE_PM_RESUME;
 	rq.special = &args;
-	rq.data = &rqpm;
+	rq.user_data = &rqpm;
 	rqpm.pm_step = ide_pm_state_start_resume;
 	rqpm.pm_state = PM_EVENT_ON;

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index fb616c6..bea6e9e 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -351,7 +351,7 @@ static int scsi_req_map_sg(struct reques
 		}
 	}

-	rq->buffer = rq->data = NULL;
+	rq->buffer = NULL;
 	rq->data_len = data_len;
 	return 0;

@@ -1116,12 +1116,11 @@ static int scsi_setup_blk_pc_cmnd(struct
 			return ret;
 	} else {
 		BUG_ON(req->data_len);
-		BUG_ON(req->data);
+		BUG_ON(req->buffer);

 		cmd->request_bufflen = 0;
 		cmd->request_buffer = NULL;
 		cmd->use_sg = 0;
-		req->buffer = NULL;
 	}

 	BUILD_BUG_ON(sizeof(req->cmd) > sizeof(cmd->cmnd));
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 7bfcde2..beb1f8d 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -288,7 +288,8 @@ struct request {
 	unsigned short ioprio;

 	void *special;
-	char *buffer;
+	char *buffer;			/* FIXME: Deprecated */
+	void *user_data;		/* FIXME: used to be *data. Deprecated this allready */

 	int tag;
 	int errors;
@@ -303,7 +304,6 @@ struct request {

 	unsigned int data_len;
 	unsigned int sense_len;
-	void *data;
 	void *sense;

 	unsigned int timeout;

Le 12.12.2006 15:21, Boaz Harrosh a écrit :
> Christoph Hellwig wrote:
>> On Tue, Dec 12, 2006 at 11:38:42AM +0100, Christoph Hellwig wrote:
>>> This is because the packet driver tries to send down read/write
>>> BLOCK_PC commands that don't use a bio and do not use sg lists.
>>> As part of the patch you mentioned I added strict assertations for that
>>> case because the scsi layer doesn't handle those anymore.
>>>
>>> The right fix is to replace all the packet_command stuff in the packet
>>> driver by scsi_execute() which needs to be lifted from scsi code to
>>> the block code for that.  I'll prepare a patch this weekend unless
>>> someone beets me in doing that work.
>> Please try the patch below to fix the bug for now.  It's not the
>> full way to a generic execute block pc infrastcuture but should fix
>> the bug for the time beeing:
>>
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>
>> Index: linux-2.6/drivers/block/pktcdvd.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/block/pktcdvd.c	2006-12-12 11:30:45.000000000 +0100
>> +++ linux-2.6/drivers/block/pktcdvd.c	2006-12-12 14:23:37.000000000 +0100
>> @@ -765,47 +765,34 @@
>>   */
>>  static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *cgc)
>>  {
>> -	char sense[SCSI_SENSE_BUFFERSIZE];
>> -	request_queue_t *q;
>> +	request_queue_t *q = bdev_get_queue(pd->bdev);
>>  	struct request *rq;
>> -	DECLARE_COMPLETION_ONSTACK(wait);
>> -	int err = 0;
>> +	int ret = 0;
>>  
>> -	q = bdev_get_queue(pd->bdev);
>> +	rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ?
>> +			     WRITE : READ, __GFP_WAIT);
>> +
>> +	if (cgc->buflen) {
>> +		if (blk_rq_map_kern(q, rq, cgc->buffer, cgc->buflen, __GFP_WAIT))
>> +			goto out;
>> +	}
>> +
>> +	rq->cmd_len = COMMAND_SIZE(rq->cmd[0]);
>> +	memcpy(rq->cmd, cgc->cmd, CDROM_PACKET_SIZE);
>> +	if (sizeof(rq->cmd) > CDROM_PACKET_SIZE)
>> +		memset(rq->cmd + CDROM_PACKET_SIZE, 0, sizeof(rq->cmd) - CDROM_PACKET_SIZE);
>>  
>> -	rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ? WRITE : READ,
>> -			     __GFP_WAIT);
>> -	rq->errors = 0;
>> -	rq->rq_disk = pd->bdev->bd_disk;
>> -	rq->bio = NULL;
>> -	rq->buffer = NULL;
>>  	rq->timeout = 60*HZ;
>> -	rq->data = cgc->buffer;
>> -	rq->data_len = cgc->buflen;
>> -	rq->sense = sense;
>> -	memset(sense, 0, sizeof(sense));
>> -	rq->sense_len = 0;
>>  	rq->cmd_type = REQ_TYPE_BLOCK_PC;
>>  	rq->cmd_flags |= REQ_HARDBARRIER;
>>  	if (cgc->quiet)
>>  		rq->cmd_flags |= REQ_QUIET;
>> -	memcpy(rq->cmd, cgc->cmd, CDROM_PACKET_SIZE);
>> -	if (sizeof(rq->cmd) > CDROM_PACKET_SIZE)
>> -		memset(rq->cmd + CDROM_PACKET_SIZE, 0, sizeof(rq->cmd) - CDROM_PACKET_SIZE);
>> -	rq->cmd_len = COMMAND_SIZE(rq->cmd[0]);
>> -
>> -	rq->ref_count++;
>> -	rq->end_io_data = &wait;
>> -	rq->end_io = blk_end_sync_rq;
>> -	elv_add_request(q, rq, ELEVATOR_INSERT_BACK, 1);
>> -	generic_unplug_device(q);
>> -	wait_for_completion(&wait);
>> -
>> -	if (rq->errors)
>> -		err = -EIO;
>>  
>> +	blk_execute_rq(rq->q, pd->bdev->bd_disk, rq, 0);
>> +	ret = rq->errors;
>> +out:
>>  	blk_put_request(rq);
>> -	return err;
>> +	return ret;
>>  }
>>  
>>  /*
>> -
>> 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
> I'm afraid this might not be enough because of code in drivers/ide/ide-cd.c. It does IO off of request->data.
> 
> [background]
> pkt_generic_packet and ton of other places mainly cd(s), floppy(s), and other ide code paths,
> are using what I call BLACK requests. They put some data on request->buffer or request->data
> stick it in the Q and than advance on them later down the ladder. Remove of "buffer" and "data" from
> struct request will reveal all these places. At one time I had plans to do just that. But 1/2 way through I gave
> up, it is too risky, too much Hardware that I don't have, that needs checking.
> 
> below patch combined with your patch might get a bit closer for this code path.
> At struct request I have changed the name of "data" member to "user_data". than changed the code paths that used
> "data" as IO to use request->buffer instead. This is just as bad but is a more common practice.
> 
> I suspect there is a problem with what I did in scsi_lib.c Christoph please check me out with the new BUG_ON.
> Mainly what you need from below is only the code in ide-cd.c.
> (And there are 3-4 places that do exactly like pkt_generic_packet, though I'm not sure they end up through SCSI.
> At first I thought this code doesn't either)
> 
[patch snipped]

Christoph's patch fixed the BUG, while Boaz's patch didn't fix anything 
(both tested with kernel 2.6.16-rc6-mm2). 

Please note I don't use ide-cd, I use libata+pata_via+sr_mod.

Boaz, when you wrote "below patch combined with your patch...", did you mean 
your patch should be applied on top of Christoph's one ? In this case, it fails with:
   patching file drivers/block/pktcdvd.c
   Hunk #1 FAILED at 778.
   1 out of 1 hunk FAILED -- rejects in file drivers/block/pktcdvd.c

This BUG (http://bugzilla.kernel.org/show_bug.cgi?id=7667) still happens with 
2.6.20-rc2-mm1.

Fortunately, Christoph Hellwig's patch 
(http://bugzilla.kernel.org/show_bug.cgi?id=7667#c5) still fix it.

Reply-To: hch@lst.de

On Tue, Dec 12, 2006 at 02:26:00PM +0100, Christoph Hellwig wrote:
> On Tue, Dec 12, 2006 at 11:38:42AM +0100, Christoph Hellwig wrote:
> > This is because the packet driver tries to send down read/write
> > BLOCK_PC commands that don't use a bio and do not use sg lists.
> > As part of the patch you mentioned I added strict assertations for that
> > case because the scsi layer doesn't handle those anymore.
> > 
> > The right fix is to replace all the packet_command stuff in the packet
> > driver by scsi_execute() which needs to be lifted from scsi code to
> > the block code for that.  I'll prepare a patch this weekend unless
> > someone beets me in doing that work.
> 
> Please try the patch below to fix the bug for now.  It's not the
> full way to a generic execute block pc infrastcuture but should fix
> the bug for the time beeing:

Given that we now have a tester that confirms this fix can we please
put this into 2.6.20 so we avoid a regression that makes pktcdvd
entirely unusable?


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/block/pktcdvd.c
===================================================================
--- linux-2.6.orig/drivers/block/pktcdvd.c	2006-12-12 11:30:45.000000000 +0100
+++ linux-2.6/drivers/block/pktcdvd.c	2006-12-12 14:23:37.000000000 +0100
@@ -765,47 +765,34 @@
  */
 static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *cgc)
 {
-	char sense[SCSI_SENSE_BUFFERSIZE];
-	request_queue_t *q;
+	request_queue_t *q = bdev_get_queue(pd->bdev);
 	struct request *rq;
-	DECLARE_COMPLETION_ONSTACK(wait);
-	int err = 0;
+	int ret = 0;
 
-	q = bdev_get_queue(pd->bdev);
+	rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ?
+			     WRITE : READ, __GFP_WAIT);
+
+	if (cgc->buflen) {
+		if (blk_rq_map_kern(q, rq, cgc->buffer, cgc->buflen, __GFP_WAIT))
+			goto out;
+	}
+
+	rq->cmd_len = COMMAND_SIZE(rq->cmd[0]);
+	memcpy(rq->cmd, cgc->cmd, CDROM_PACKET_SIZE);
+	if (sizeof(rq->cmd) > CDROM_PACKET_SIZE)
+		memset(rq->cmd + CDROM_PACKET_SIZE, 0, sizeof(rq->cmd) - CDROM_PACKET_SIZE);
 
-	rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ? WRITE : READ,
-			     __GFP_WAIT);
-	rq->errors = 0;
-	rq->rq_disk = pd->bdev->bd_disk;
-	rq->bio = NULL;
-	rq->buffer = NULL;
 	rq->timeout = 60*HZ;
-	rq->data = cgc->buffer;
-	rq->data_len = cgc->buflen;
-	rq->sense = sense;
-	memset(sense, 0, sizeof(sense));
-	rq->sense_len = 0;
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 	rq->cmd_flags |= REQ_HARDBARRIER;
 	if (cgc->quiet)
 		rq->cmd_flags |= REQ_QUIET;
-	memcpy(rq->cmd, cgc->cmd, CDROM_PACKET_SIZE);
-	if (sizeof(rq->cmd) > CDROM_PACKET_SIZE)
-		memset(rq->cmd + CDROM_PACKET_SIZE, 0, sizeof(rq->cmd) - CDROM_PACKET_SIZE);
-	rq->cmd_len = COMMAND_SIZE(rq->cmd[0]);
-
-	rq->ref_count++;
-	rq->end_io_data = &wait;
-	rq->end_io = blk_end_sync_rq;
-	elv_add_request(q, rq, ELEVATOR_INSERT_BACK, 1);
-	generic_unplug_device(q);
-	wait_for_completion(&wait);
-
-	if (rq->errors)
-		err = -EIO;
 
+	blk_execute_rq(rq->q, pd->bdev->bd_disk, rq, 0);
+	ret = rq->errors;
+out:
 	blk_put_request(rq);
-	return err;
+	return ret;
 }
 
 /*

Reply-To: petero2@telia.com

On Tue, 2 Jan 2007, Christoph Hellwig wrote:

> On Tue, Dec 12, 2006 at 02:26:00PM +0100, Christoph Hellwig wrote:
>> On Tue, Dec 12, 2006 at 11:38:42AM +0100, Christoph Hellwig wrote:
>>> This is because the packet driver tries to send down read/write
>>> BLOCK_PC commands that don't use a bio and do not use sg lists.
>>> As part of the patch you mentioned I added strict assertations for that
>>> case because the scsi layer doesn't handle those anymore.
>>>
>>> The right fix is to replace all the packet_command stuff in the packet
>>> driver by scsi_execute() which needs to be lifted from scsi code to
>>> the block code for that.  I'll prepare a patch this weekend unless
>>> someone beets me in doing that work.
>>
>> Please try the patch below to fix the bug for now.  It's not the
>> full way to a generic execute block pc infrastcuture but should fix
>> the bug for the time beeing:
>
> Given that we now have a tester that confirms this fix can we please
> put this into 2.6.20 so we avoid a regression that makes pktcdvd
> entirely unusable?

Works for me using an IDE drive too. Thanks.

> Signed-off-by: Christoph Hellwig <hch@lst.de>

Acked-by: Peter Osterlund <petero2@telia.com>