Re: [PATCH] ata: implement MODE SELECT command

[Sergei Shtylyov <sshtylyov@mvista.com>]

Hello.

On 07/04/2012 03:13 PM, Paolo Bonzini wrote:

> The cache_type file in sysfs lets users configure the disk cache in
> write-through or write-back modes.  However, ata disks do not support
> writing to the file because they do not implement the MODE SELECT
> command.

> This patch adds a translation from MODE SELECT (for the caching page
> only) to the ATA SET FEATURES command.

> Cc: Jeff Garzik <jgarzik@pobox.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  drivers/ata/libata-scsi.c |  147 +++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 142 insertions(+), 5 deletions(-)

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 41cde45..e7702d3 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3081,6 +3081,144 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>  }
>  
>  /**
> + *	ata_mselect_caching - Simulate MODE SELECT for caching info page
> + *	@tf: taskfile to be filled
> + *	@buf: input buffer
> + *	@len: number of valid bytes in the input buffer
> + *
> + *	Prepare a taskfile to modify caching information for the device.
> + *
> + *	LOCKING:
> + *	None.
> + */
> +static unsigned int ata_mselect_caching(struct ata_taskfile *tf, const u8 *buf,
> +					int len)
> +{
> +	u8 wce;

   Empty line after declaration block wouldn't hurt...

> +	if (len == 0)
> +		return 1;
> +
> +	/*
> +	 * The first two bytes are a header, so offsets here are 2 less
> +	 * than in ata_msense_caching.
> +	 */
> +	wce = buf[0] & (1 << 2);

   Perhaps it's worth denying the command if the other page fields dfifer from
'def_cache_mpage' (i.e. all zeros)?

> +/**
> + *	ata_scsi_mode_select_xlat - Translate MODE SELECT 6, 10 commands
> + *	@qc: Storage for translated ATA taskfile
> + *
> + *	Converts a MODE SELECT command to an ATA SET FEATURES taskfile.
> + *	Assume this is invoked for direct access devices (e.g. disks) only.
> + *	There should be no block descriptor for other device types.
> + *
> + *	LOCKING:
> + *	spin_lock_irqsave(host lock)
> + */
> +static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
> +{
> +	struct scsi_cmnd *scmd = qc->scsicmd;
> +	struct ata_taskfile *tf = &qc->tf;
> +	const u8 *cdb = scmd->cmnd;
> +	const u8 *p;
> +	u8 pg, spg;
> +	unsigned six_byte, pg_len;
> +	int len;
> +
> +	VPRINTK("ENTER\n");
> +
> +	/*
> +	 * Get the command buffer.
> +	 */
> +	if (!scsi_sg_count(scmd))
> +		goto invalid_fld;
> +
> +	p = page_address(sg_page(scsi_sglist(scmd)));
> +
> +	six_byte = (cdb[0] == MODE_SELECT);
> +	if (six_byte) {
> +		if (scmd->cmd_len < 5)

   Not 6?

> +			goto invalid_fld;
> +
> +		len = cdb[4];
> +	} else {
> +		if (scmd->cmd_len < 9)

   Not 10?
   And ata_scsiop_mode_sense() don't seem to check this...
   And I doubt "Invalid field in CDB" is a proper sense code in this situation.

> +			goto invalid_fld;
> +
> +		len = (cdb[7] << 8) | cdb[8];
> +	}

> +	/*
> +	 * No mode subpages supported (yet) but asking for _all_
> +	 * subpages may be valid
> +	 */
> +	if (spg && (spg != ALL_SUB_MPAGES))
> +		goto invalid_fld;
> +	if (pg_len > len)
> +		goto invalid_fld;

   It's valid to have buffer size less than data size.

> +	switch (pg) {
> +	case CACHE_MPAGE:
> +		if (ata_mselect_caching(tf, p, pg_len))
> +			goto invalid_fld;
> +		break;
> +
> +	default:		/* invalid page code */
> +		goto invalid_fld;

   Page code is not a part of CDB, hence the sense code you'll return doesn't
fit there.

> +	}
> +	return 0;
> +
> + invalid_fld:
> +	ata_scsi_set_sense(scmd, ILLEGAL_REQUEST, 0x24, 0x0);
> +	/* "Invalid field in cbd" */
> +	return 1;

MBR, Sergei