Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

Hi,

On Wednesday 04 March 2009, Borislav Petkov wrote:
> ... since some do not transfer any data from the drive and
> rq->buffer is unset leading to an OOPS when checking VM translations
> (CONFIG_DEBUG_VIRTUAL).
> 
> Tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>

Thanks for fixing it but I worry that the patch is not entirely correct
(why o why did you have to throw in unrelated changes...?)

also ideally there should have been two patches:
- minimal bugfix for the buggy patch
- unification/cleanup of PIO sg setup

> ---
>  drivers/ide/ide-atapi.c  |    9 ++++++++-
>  drivers/ide/ide-cd.c     |    4 ----
>  drivers/ide/ide-floppy.c |    4 ----
>  3 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
> index a5596a6..ef78cbf 100644
> --- a/drivers/ide/ide-atapi.c
> +++ b/drivers/ide/ide-atapi.c
> @@ -570,7 +570,7 @@ static ide_startstop_t ide_transfer_pc(ide_drive_t *drive)
>  
>  ide_startstop_t ide_issue_pc(ide_drive_t *drive, struct ide_cmd *cmd)
>  {
> -	struct ide_atapi_pc *pc;
> +	struct ide_atapi_pc *uninitialized_var(pc);
>  	ide_hwif_t *hwif = drive->hwif;
>  	ide_expiry_t *expiry = NULL;
>  	struct request *rq = hwif->rq;
> @@ -587,6 +587,7 @@ ide_startstop_t ide_issue_pc(ide_drive_t *drive, struct ide_cmd *cmd)
>  
>  		if (drive->dma)
>  			drive->dma = !ide_dma_prepare(drive, cmd);
> +

?

>  	} else {
>  		pc = drive->pc;
>  
> @@ -614,6 +615,12 @@ ide_startstop_t ide_issue_pc(ide_drive_t *drive, struct ide_cmd *cmd)
>  						       : WAIT_TAPE_CMD;
>  	}
>  
> +	if (drive->media != ide_tape &&
> +	      !drive->dma && (blk_fs_request(rq) || rq->data_len)) {
> +		ide_init_sg_cmd(cmd, blk_rq_bytes(rq));
> +		ide_map_sg(drive, cmd);
> +	}
> +
>  	ide_init_packet_cmd(cmd, tf_flags, bcount, drive->dma);
>  
>  	(void)do_rw_taskfile(drive, cmd);
> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> index 091d414..106cacb 100644
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -916,10 +916,6 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
>  
>  	cmd.rq = rq;
>  
> -	ide_init_sg_cmd(&cmd,
> -		blk_fs_request(rq) ? (rq->nr_sectors << 9) : rq->data_len);
> -	ide_map_sg(drive, &cmd);
> -
>  	return ide_issue_pc(drive, &cmd);
>  out_end:
>  	nsectors = rq->hard_nr_sectors;
> diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
> index 2f8f453..b91deef 100644
> --- a/drivers/ide/ide-floppy.c
> +++ b/drivers/ide/ide-floppy.c
> @@ -281,10 +281,6 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
>  		cmd.tf_flags |= IDE_TFLAG_WRITE;
>  
>  	cmd.rq = rq;
> -
> -	ide_init_sg_cmd(&cmd, pc->req_xfer);

There was a reason for using ->req_xfer.

I don't see where rq->data_len is set for REQ_TYPE_SPECIAL requests
(ide_queue_pc_tail() and friends)?

> -	ide_map_sg(drive, &cmd);
> -
>  	pc->rq = rq;
>  
>  	return ide_floppy_issue_pc(drive, &cmd, pc);