Re: [PATCH 02/15] ide-tape: remove back-to-back REQUEST_SENSE detection

[Tejun Heo <tj@kernel.org>]

Hello,

Borislav Petkov wrote:
> I hit that too when debugging an ide-tape problem a user has
> (http://bugzilla.kernel.org/show_bug.cgi?id=12874). However, this is not the
> proper solution since, currently, ide-tape stuffs all packet commands in
> rq->buffer or rq->special now after your changes. It has to get them out of
> there in the ->do_request callback and set drive->pc to point to the current
> packet command in flight through the IRQ handler. And since ide_tape_issue_pc()
> is called by the ->do_request callback we should have the drive->pc always
> valid.
> 
> How about something like that instead:
> 
> diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> index 4e6181c..171dbcd 100644
> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -792,6 +792,9 @@ static ide_startstop_t
> idetape_do_request(ide_drive_t *drive,
>  	struct request *postponed_rq = tape->postponed_rq;
>  	u8 stat;
> 
> +	if (rq->cmd_type == REQ_TYPE_SPECIAL)
> +		drive->pc = (struct ide_atapi_pc *) rq->buffer;
> +
>  	debug_log(DBG_SENSE, "sector: %llu, nr_sectors: %lu,"
>  			" current_nr_sectors: %u\n",
>  			(unsigned long long)rq->sector, rq->nr_sectors,

I don't really care one way or the other but the error condition
itself looked somewhat pointless to me, so I just killed it.  Is the
error messasge really worth guaranteeing drive->pc can be accessed
after completion?  That's a nasty and fragile guarantee.  If such
check is really necessary, the proper way to do it would be recording
whether the last command was request sense in some persistent data
structure not trying to access a data structure which the code doesn't
own anymore.

Thanks.

-- 
tejun