Re: [PATCH 13/18] ide-floppy: replace pc->c with rq->cmd

[Bartlomiej Zolnierkiewicz]

On Thursday 12 June 2008, Borislav Petkov wrote:
> This is the first one in the series attempting to phase out ide_atapi_pc and use
> block request structs. Also, the pc request type is now REQ_TYPE_BLOCK_PC. As a result,
> idefloppy_blockpc_cmd becomes unused and can go.

Do not attack too many dragons at once or they will slay you... ;)

IOW this patch mixes too many changes (some _really_ non-trivial)
in one go which results in rather bad outcome...

[...]

> diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
> index b368943..ffebf83 100644
> --- a/drivers/ide/ide-floppy.c
> +++ b/drivers/ide/ide-floppy.c
> @@ -217,7 +217,7 @@ static int idefloppy_end_request(ide_drive_t *drive, int uptodate, int nsecs)
>  	/* Why does this happen? */
>  	if (!rq)
>  		return 0;
> -	if (!blk_special_request(rq)) {
> +	if (!blk_pc_request(rq)) {
>  		/* our real local end request function */
>  		ide_end_request(drive, uptodate, nsecs);
>  		return 0;
> @@ -282,13 +282,14 @@ static void idefloppy_update_buffers(ide_drive_t *drive,
>   * pass through the driver.
>   */
>  static void idefloppy_queue_pc_head(ide_drive_t *drive, struct ide_atapi_pc *pc,
> -		struct request *rq)
> +		struct request *rq, unsigned char *cmd)
>  {
>  	struct ide_floppy_obj *floppy = drive->driver_data;
>  
>  	blk_rq_init(NULL, rq);
>  	rq->buffer = (char *) pc;
> -	rq->cmd_type = REQ_TYPE_SPECIAL;
> +	rq->cmd_type = REQ_TYPE_BLOCK_PC;
> +	memcpy(rq->cmd, cmd, 12);
>  	rq->cmd_flags |= REQ_PREEMPT;
>  	rq->rq_disk = floppy->disk;
>  	ide_do_drive_cmd(drive, rq);

REQUEST SENSE command is switched from REQ_TYPE_SPECIAL to REQ_TYPE_BLOCK_PC
which should belong to a separate patch and is premature IMHO (the other
ATAPI drivers seem to still use REQ_TYPE_SPECIAL so probably it makes sense
to unify/move REQUEST_SENSE handling to ide-atapi.c first).

> @@ -323,10 +324,10 @@ static void ide_floppy_callback(ide_drive_t *drive)
>  	if (floppy->failed_pc == pc)
>  		floppy->failed_pc = NULL;
>  
> -	if (pc->c[0] == GPCMD_READ_10 || pc->c[0] == GPCMD_WRITE_10 ||
> -	    (pc->rq && blk_pc_request(pc->rq)))
> +	if (pc->rq->cmd[0] == GPCMD_READ_10 || pc->rq->cmd[0] == GPCMD_WRITE_10
> +			|| (pc->rq && blk_pc_request(pc->rq)))
>  		uptodate = 1; /* FIXME */

This actually seems to break REQUEST SENSE handling
(please note the blk_pc_request() check above)... :(

> -	else if (pc->c[0] == GPCMD_REQUEST_SENSE) {
> +	else if (pc->rq->cmd[0] == GPCMD_REQUEST_SENSE) {

[...]

> @@ -592,25 +601,6 @@ static void idefloppy_create_rw_cmd(idefloppy_floppy_t *floppy,
>  	pc->flags |= PC_FLAG_DMA_OK;
>  }
>  
> -static void idefloppy_blockpc_cmd(idefloppy_floppy_t *floppy,
> -		struct ide_atapi_pc *pc, struct request *rq)
> -{
> -	idefloppy_init_pc(pc);
> -	memcpy(pc->c, rq->cmd, sizeof(pc->c));
> -	pc->rq = rq;
> -	pc->b_count = rq->data_len;
> -	if (rq->data_len && rq_data_dir(rq) == WRITE)
> -		pc->flags |= PC_FLAG_WRITING;
> -	pc->buf = rq->data;
> -	if (rq->bio)
> -		pc->flags |= PC_FLAG_DMA_OK;
> -	/*
> -	 * possibly problematic, doesn't look like ide-floppy correctly
> -	 * handled scattered requests if dma fails...
> -	 */
> -	pc->req_xfer = pc->buf_size = rq->data_len;
> -}

[...]

> @@ -644,12 +634,9 @@ static ide_startstop_t idefloppy_do_request(ide_drive_t *drive,
>  		}
>  		pc = idefloppy_next_pc_storage(drive);
>  		idefloppy_create_rw_cmd(floppy, pc, rq, block);
> -	} else if (blk_special_request(rq)) {
> +	} else if (blk_pc_request(rq))
>  		pc = (struct ide_atapi_pc *) rq->buffer;
> -	} else if (blk_pc_request(rq)) {
> -		pc = idefloppy_next_pc_storage(drive);
> -		idefloppy_blockpc_cmd(floppy, pc, rq);
> -	} else {
> +	else {
>  		blk_dump_rq_flags(rq,
>  			"ide-floppy: unsupported command in queue");
>  		idefloppy_end_request(drive, 0, 0);

Also the above changes seem to break handling of blk_pc_request() requests
(i.e. SG_IO ioctl) because they will be no longer initialized properly.

OTOH the main change (pc->c to rq->cmd) looks OK (only minor complaint is
that 'u8' is both shorter and more readable than 'unsigned short')...

[ I had to also skip patches #16-17 because of skipping this patch. ]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/