Re: [Bugme-new] [Bug 7667] New: BUG at drivers/scsi/scsi_lib.c:1118 caused by "pktsetup dvd /dev/sr0"

[Laurent Riffard <laurent.riffard@free.fr>]

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

-- 
laurent
-
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