linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] ata: MODE SELECT implementation
@ 2012-07-05  9:40 Paolo Bonzini
  2012-07-05  9:40 ` [PATCH v2 1/2] ata: support MODE SENSE request for changeable and default parameters Paolo Bonzini
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Paolo Bonzini @ 2012-07-05  9:40 UTC (permalink / raw)
  To: linux-kernel, linux-ide

This is a revised version of the MODE SELECT implementation from yesterday,
augmented with support for changeable parameter requests in MODE SENSE.

Paolo Bonzini (2):
  ata: support MODE SENSE request for changeable parameters
  ata: implement MODE SELECT command

 drivers/ata/libata-scsi.c |  242 +++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 221 insertions(+), 21 deletions(-)


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/2] ata: support MODE SENSE request for changeable and default parameters
  2012-07-05  9:40 [PATCH v2 0/2] ata: MODE SELECT implementation Paolo Bonzini
@ 2012-07-05  9:40 ` Paolo Bonzini
  2012-07-05  9:40 ` [PATCH v2 2/2] ata: implement MODE SELECT command Paolo Bonzini
  2012-07-26  7:25 ` [PATCH v2 0/2] ata: MODE SELECT implementation Paolo Bonzini
  2 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2012-07-05  9:40 UTC (permalink / raw)
  To: linux-kernel, linux-ide; +Cc: Sergei Shtylyov, Jeff Garzik

Since the next patch will introduce support for MODE SELECT, it
makes sense to start advertising which bits are actually changeable.
For now, the answer is none.

Default parameters can also be reported, they are simply the same
as the current parameters.

Cc: Sergei Shtylyov <sshtylyov@mvista.com>
Cc: Jeff Garzik <jgarzik@pobox.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/ata/libata-scsi.c |   59 ++++++++++++++++++++++++++++++++------------
 1 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 41cde45..d5aeb26 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2206,9 +2206,33 @@ static unsigned int ata_scsiop_noop(struct ata_scsi_args *args, u8 *rbuf)
 }
 
 /**
+ *	modecpy - Prepare response for MODE SENSE
+ *	@dest: output buffer
+ *	@src: data being copied
+ *	@n: length of mode page
+ *	@changeable: whether changeable parameters are requested
+ *
+ *	Generate a generic MODE SENSE page for either current or changeable
+ *	parameters.
+ *
+ *	LOCKING:
+ *	None.
+ */
+static void modecpy(u8 *dest, const u8 *src, int n, bool changeable)
+{
+	if (changeable) {
+		memcpy(dest, src, 2);
+		memset(dest + 2, 0, n - 2);
+	} else {
+		memcpy(dest, src, n);
+	}
+}
+
+/**
  *	ata_msense_caching - Simulate MODE SENSE caching info page
  *	@id: device IDENTIFY data
  *	@buf: output buffer
+ *	@changeable: whether changeable parameters are requested
  *
  *	Generate a caching info page, which conditionally indicates
  *	write caching to the SCSI layer, depending on device
@@ -2217,12 +2241,12 @@ static unsigned int ata_scsiop_noop(struct ata_scsi_args *args, u8 *rbuf)
  *	LOCKING:
  *	None.
  */
-static unsigned int ata_msense_caching(u16 *id, u8 *buf)
+static unsigned int ata_msense_caching(u16 *id, u8 *buf, bool changeable)
 {
-	memcpy(buf, def_cache_mpage, sizeof(def_cache_mpage));
-	if (ata_id_wcache_enabled(id))
+	modecpy(buf, def_cache_mpage, sizeof(def_cache_mpage), changeable);
+	if (!changeable && ata_id_wcache_enabled(id))
 		buf[2] |= (1 << 2);	/* write cache enable */
-	if (!ata_id_rahead_enabled(id))
+	if (!changeable && !ata_id_rahead_enabled(id))
 		buf[12] |= (1 << 5);	/* disable read ahead */
 	return sizeof(def_cache_mpage);
 }
@@ -2230,30 +2254,33 @@ static unsigned int ata_msense_caching(u16 *id, u8 *buf)
 /**
  *	ata_msense_ctl_mode - Simulate MODE SENSE control mode page
  *	@buf: output buffer
+ *	@changeable: whether changeable parameters are requested
  *
  *	Generate a generic MODE SENSE control mode page.
  *
  *	LOCKING:
  *	None.
  */
-static unsigned int ata_msense_ctl_mode(u8 *buf)
+static unsigned int ata_msense_ctl_mode(u8 *buf, bool changeable)
 {
-	memcpy(buf, def_control_mpage, sizeof(def_control_mpage));
+	modecpy(buf, def_control_mpage, sizeof(def_control_mpage), changeable);
 	return sizeof(def_control_mpage);
 }
 
 /**
  *	ata_msense_rw_recovery - Simulate MODE SENSE r/w error recovery page
  *	@buf: output buffer
+ *	@changeable: whether changeable parameters are requested
  *
  *	Generate a generic MODE SENSE r/w error recovery page.
  *
  *	LOCKING:
  *	None.
  */
-static unsigned int ata_msense_rw_recovery(u8 *buf)
+static unsigned int ata_msense_rw_recovery(u8 *buf, bool changeable)
 {
-	memcpy(buf, def_rw_recovery_mpage, sizeof(def_rw_recovery_mpage));
+	modecpy(buf, def_rw_recovery_mpage, sizeof(def_rw_recovery_mpage),
+		changeable);
 	return sizeof(def_rw_recovery_mpage);
 }
 
@@ -2317,11 +2344,11 @@ static unsigned int ata_scsiop_mode_sense(struct ata_scsi_args *args, u8 *rbuf)
 	page_control = scsicmd[2] >> 6;
 	switch (page_control) {
 	case 0: /* current */
+	case 1: /* changeable */
+	case 2: /* defaults */
 		break;  /* supported */
 	case 3: /* saved */
 		goto saving_not_supp;
-	case 1: /* changeable */
-	case 2: /* defaults */
 	default:
 		goto invalid_fld;
 	}
@@ -2342,21 +2369,21 @@ static unsigned int ata_scsiop_mode_sense(struct ata_scsi_args *args, u8 *rbuf)
 
 	switch(pg) {
 	case RW_RECOVERY_MPAGE:
-		p += ata_msense_rw_recovery(p);
+		p += ata_msense_rw_recovery(p, page_control == 1);
 		break;
 
 	case CACHE_MPAGE:
-		p += ata_msense_caching(args->id, p);
+		p += ata_msense_caching(args->id, p, page_control == 1);
 		break;
 
 	case CONTROL_MPAGE:
-		p += ata_msense_ctl_mode(p);
+		p += ata_msense_ctl_mode(p, page_control == 1);
 		break;
 
 	case ALL_MPAGES:
-		p += ata_msense_rw_recovery(p);
-		p += ata_msense_caching(args->id, p);
-		p += ata_msense_ctl_mode(p);
+		p += ata_msense_rw_recovery(p, page_control == 1);
+		p += ata_msense_caching(args->id, p, page_control == 1);
+		p += ata_msense_ctl_mode(p, page_control == 1);
 		break;
 
 	default:		/* invalid page code */
-- 
1.7.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] ata: implement MODE SELECT command
  2012-07-05  9:40 [PATCH v2 0/2] ata: MODE SELECT implementation Paolo Bonzini
  2012-07-05  9:40 ` [PATCH v2 1/2] ata: support MODE SENSE request for changeable and default parameters Paolo Bonzini
@ 2012-07-05  9:40 ` Paolo Bonzini
  2012-07-05 11:42   ` Sergei Shtylyov
  2012-07-26  7:25 ` [PATCH v2 0/2] ata: MODE SELECT implementation Paolo Bonzini
  2 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2012-07-05  9:40 UTC (permalink / raw)
  To: linux-kernel, linux-ide; +Cc: Sergei Shtylyov, Jeff Garzik

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.  The set of changeable parameters
answered by MODE SENSE is also adjusted accordingly.

Cc: Sergei Shtylyov <sshtylyov@mvista.com>
Cc: Jeff Garzik <jgarzik@pobox.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/ata/libata-scsi.c |  185 +++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 179 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index d5aeb26..1cb88df 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2244,7 +2244,7 @@ static void modecpy(u8 *dest, const u8 *src, int n, bool changeable)
 static unsigned int ata_msense_caching(u16 *id, u8 *buf, bool changeable)
 {
 	modecpy(buf, def_cache_mpage, sizeof(def_cache_mpage), changeable);
-	if (!changeable && ata_id_wcache_enabled(id))
+	if (changeable || ata_id_wcache_enabled(id))
 		buf[2] |= (1 << 2);	/* write cache enable */
 	if (!changeable && !ata_id_rahead_enabled(id))
 		buf[12] |= (1 << 5);	/* disable read ahead */
@@ -3108,6 +3108,179 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 }
 
 /**
+ *	ata_mselect_caching - Simulate MODE SELECT for caching info page
+ *	@qc: Storage for translated ATA taskfile
+ *	@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 int ata_mselect_caching(struct ata_queued_cmd *qc,
+			       const u8 *buf, int len)
+{
+	struct ata_taskfile *tf = &qc->tf;
+	struct ata_device *dev = qc->dev;
+	char mpage[CACHE_MPAGE_LEN];
+	u8 wce;
+
+	/*
+	 * The first two bytes of def_cache_mpage are a header, so offsets
+	 * in mpage are off by 2 compared to buf.  Same for len.
+	 */
+
+	if (len != CACHE_MPAGE_LEN - 2)
+		return -EINVAL;
+
+	wce = buf[0] & (1 << 2);
+
+	/*
+	 * Check that read-only bits are not modified.
+	 */
+	ata_msense_caching(dev->id, mpage, false);
+	mpage[2] &= ~(1 << 2);
+	mpage[2] |= wce;
+	if (memcmp(mpage + 2, buf, CACHE_MPAGE_LEN - 2) != 0)
+		return -EINVAL;
+
+	tf->flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
+	tf->protocol = ATA_PROT_NODATA;
+	tf->nsect = 0;
+	tf->command = ATA_CMD_SET_FEATURES;
+	tf->feature = wce ? SETFEATURES_WC_ON : SETFEATURES_WC_OFF;
+	return 0;
+}
+
+/**
+ *	ata_scsiop_mode_select - Simulate 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;
+	const u8 *cdb = scmd->cmnd;
+	const u8 *p;
+	u8 pg, spg;
+	unsigned six_byte, pg_len, hdr_len;
+	int len;
+
+	VPRINTK("ENTER\n");
+
+	six_byte = (cdb[0] == MODE_SELECT);
+	if (six_byte) {
+		if (scmd->cmd_len < 5)
+			goto invalid_fld;
+
+		len = cdb[4];
+	} else {
+		if (scmd->cmd_len < 9)
+			goto invalid_fld;
+
+		len = (cdb[7] << 8) + cdb[8];
+	}
+
+	/* We only support PF=1, SP=0.  */
+	if ((cdb[1] & 0x11) != 0x10)
+		goto invalid_fld;
+
+	/* Test early for possible overrun.  */
+	if (!scsi_sg_count(scmd) || scsi_sglist(scmd)->length < len)
+		goto invalid_param_len;
+
+	p = page_address(sg_page(scsi_sglist(scmd)));
+
+	/* Move past header and block descriptors.  */
+	if (six_byte)
+		hdr_len = p[3] + 4;
+	else
+		hdr_len = (p[6] << 8) + p[7] + 8;
+
+	if (len < hdr_len)
+		goto invalid_param_len;
+
+	len -= hdr_len;
+	p += hdr_len;
+	if (len == 0)
+		goto skip;
+
+	/* Parse both possible formats for the mode page headers.  */
+	pg = p[0] & 0x3f;
+	if (p[0] & 0x40) {
+		if (len < 4)
+			goto invalid_param_len;
+
+		spg = p[1];
+		pg_len = (p[2] << 8) | p[3];
+		p += 4;
+		len -= 4;
+	} else {
+		if (len < 2)
+			goto invalid_param_len;
+
+		spg = 0;
+		pg_len = p[1];
+		p += 2;
+		len -= 2;
+	}
+
+	/*
+	 * Only one page has changeable data, so we only support setting one
+	 * page at a time.
+	 */
+	if (len < pg_len)
+		goto invalid_param;
+
+	/*
+	 * No mode subpages supported (yet) but asking for _all_
+	 * subpages may be valid
+	 */
+	if (spg && (spg != ALL_SUB_MPAGES))
+		goto invalid_param;
+	if (pg_len > len)
+		goto invalid_param_len;
+
+	switch (pg) {
+	case CACHE_MPAGE:
+		if (ata_mselect_caching(qc, p, pg_len) < 0)
+			goto invalid_param;
+		break;
+
+	default:		/* invalid page code */
+		goto invalid_param;
+	}
+
+	return 0;
+
+ invalid_fld:
+	/* "Invalid field in CDB" */
+	ata_scsi_set_sense(scmd, ILLEGAL_REQUEST, 0x24, 0x0);
+	return 1;
+
+ invalid_param:
+	/* "Invalid field in parameter list" */
+	ata_scsi_set_sense(scmd, ILLEGAL_REQUEST, 0x26, 0x0);
+	return 1;
+
+ invalid_param_len:
+	/* "Parameter list length error" */
+	ata_scsi_set_sense(scmd, ILLEGAL_REQUEST, 0x1a, 0x0);
+	return 1;
+
+ skip:
+	scmd->result = SAM_STAT_GOOD;
+	return 1;
+}
+
+/**
  *	ata_get_xlat_func - check if SCSI to ATA translation is possible
  *	@dev: ATA device
  *	@cmd: SCSI command opcode to consider
@@ -3147,6 +3320,11 @@ static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd)
 	case ATA_16:
 		return ata_scsi_pass_thru;
 
+	case MODE_SELECT:
+	case MODE_SELECT_10:
+		return ata_scsi_mode_select_xlat;
+		break;
+
 	case START_STOP:
 		return ata_scsi_start_stop_xlat;
 	}
@@ -3339,11 +3517,6 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
 		ata_scsi_rbuf_fill(&args, ata_scsiop_mode_sense);
 		break;
 
-	case MODE_SELECT:	/* unconditionally return */
-	case MODE_SELECT_10:	/* bad-field-in-cdb */
-		ata_scsi_invalid_field(cmd);
-		break;
-
 	case READ_CAPACITY:
 		ata_scsi_rbuf_fill(&args, ata_scsiop_read_cap);
 		break;
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] ata: implement MODE SELECT command
  2012-07-05  9:40 ` [PATCH v2 2/2] ata: implement MODE SELECT command Paolo Bonzini
@ 2012-07-05 11:42   ` Sergei Shtylyov
  2012-07-05 12:05     ` Paolo Bonzini
  0 siblings, 1 reply; 9+ messages in thread
From: Sergei Shtylyov @ 2012-07-05 11:42 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, linux-ide, Jeff Garzik

Hello.

On 05-07-2012 13:40, 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.  The set of changeable parameters
> answered by MODE SENSE is also adjusted accordingly.

> Cc: Sergei Shtylyov <sshtylyov@mvista.com>
> Cc: Jeff Garzik <jgarzik@pobox.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   drivers/ata/libata-scsi.c |  185 +++++++++++++++++++++++++++++++++++++++++++--
>   1 files changed, 179 insertions(+), 6 deletions(-)

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index d5aeb26..1cb88df 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
[...]
> +/**
> + *	ata_scsiop_mode_select - Simulate 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;
> +	const u8 *cdb = scmd->cmnd;
> +	const u8 *p;
> +	u8 pg, spg;
> +	unsigned six_byte, pg_len, hdr_len;
> +	int len;
> +
> +	VPRINTK("ENTER\n");
> +
> +	six_byte = (cdb[0] == MODE_SELECT);
> +	if (six_byte) {
> +		if (scmd->cmd_len < 5)
> +			goto invalid_fld;

    Strictly speaking, it should be "invalid phase" error.

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

    The same.

> +
> +		len = (cdb[7] << 8) + cdb[8];
> +	}
> +	/* Test early for possible overrun.  */
> +	if (!scsi_sg_count(scmd) || scsi_sglist(scmd)->length < len)
> +		goto invalid_param_len;

     Strictly speaking, it's "data underrun" error.

> +	p = page_address(sg_page(scsi_sglist(scmd)));

    What if the S/G list spans multiple non-consecutive pages?

> +
> +	/* Move past header and block descriptors.  */
> +	if (six_byte)
> +		hdr_len = p[3] + 4;
> +	else
> +		hdr_len = (p[6] << 8) + p[7] + 8;
> +
> +	if (len < hdr_len)
> +		goto invalid_param_len;
> +
> +	len -= hdr_len;
> +	p += hdr_len;
> +	if (len == 0)
> +		goto skip;
> +
> +	/* Parse both possible formats for the mode page headers.  */
> +	pg = p[0] & 0x3f;
> +	if (p[0] & 0x40) {
> +		if (len < 4)
> +			goto invalid_param_len;
> +
> +		spg = p[1];
> +		pg_len = (p[2] << 8) | p[3];
> +		p += 4;
> +		len -= 4;
> +	} else {
> +		if (len < 2)
> +			goto invalid_param_len;
> +
> +		spg = 0;
> +		pg_len = p[1];
> +		p += 2;
> +		len -= 2;
> +	}
> +
> +	/*
> +	 * Only one page has changeable data, so we only support setting one
> +	 * page at a time.
> +	 */
> +	if (len < pg_len)
> +		goto invalid_param;

     Not 'invalid_param_len'?

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

    Rather "paramater not supported" (0x26/0x01)...

> +	if (pg_len > len)
> +		goto invalid_param_len;

    We have just checked this.

> +	switch (pg) {
> +	case CACHE_MPAGE:
> +		if (ata_mselect_caching(qc, p, pg_len) < 0)
> +			goto invalid_param;

    Rather "parameter not supported"?

> +		break;
> +
> +	default:		/* invalid page code */
> +		goto invalid_param;

    Rather "paramater not supported"...

> +	}
> +
> +	return 0;

MBR, Sergei

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] ata: implement MODE SELECT command
  2012-07-05 11:42   ` Sergei Shtylyov
@ 2012-07-05 12:05     ` Paolo Bonzini
  2012-07-05 19:42       ` Sergei Shtylyov
  0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2012-07-05 12:05 UTC (permalink / raw)
  To: Sergei Shtylyov; +Cc: linux-kernel, linux-ide, Jeff Garzik

Il 05/07/2012 13:42, Sergei Shtylyov ha scritto:
>> +    six_byte = (cdb[0] == MODE_SELECT);
>> +    if (six_byte) {
>> +        if (scmd->cmd_len < 5)
>> +            goto invalid_fld;
> 
>    Strictly speaking, it should be "invalid phase" error.
> 
>> +
>> +        len = cdb[4];
>> +    } else {
>> +        if (scmd->cmd_len < 9)
>> +            goto invalid_fld;
> 
>    The same.
> 
>> +
>> +        len = (cdb[7] << 8) + cdb[8];
>> +    }
>> +    /* Test early for possible overrun.  */
>> +    if (!scsi_sg_count(scmd) || scsi_sglist(scmd)->length < len)
>> +        goto invalid_param_len;
> 
>     Strictly speaking, it's "data underrun" error.

The exact errors don't really matter, they shouldn't happen at all.  But
they're important so that we don't access uninitialized memory in case
they do.  Existing xlat functions are similarly hand-wavy.

>> +    p = page_address(sg_page(scsi_sglist(scmd)));
> 
>    What if the S/G list spans multiple non-consecutive pages?

Hmm, with a large number of block descriptors we could indeed span more
than one page.  On the other hand, we can just fail with invalid_param
if there is more than one block descriptor, so we're guaranteed not to
look beyond the first page (4 bytes for the header, 8 bytes for the
block descriptor, 4 bytes for the mode page header, 10 bytes for the
mode page).

>> +
>> +    /* Move past header and block descriptors.  */
>> +    if (six_byte)
>> +        hdr_len = p[3] + 4;
>> +    else
>> +        hdr_len = (p[6] << 8) + p[7] + 8;
>> +
>> +    if (len < hdr_len)
>> +        goto invalid_param_len;
>> +
>> +    len -= hdr_len;
>> +    p += hdr_len;
>> +    if (len == 0)
>> +        goto skip;
>> +
>> +    /* Parse both possible formats for the mode page headers.  */
>> +    pg = p[0] & 0x3f;
>> +    if (p[0] & 0x40) {
>> +        if (len < 4)
>> +            goto invalid_param_len;
>> +
>> +        spg = p[1];
>> +        pg_len = (p[2] << 8) | p[3];
>> +        p += 4;
>> +        len -= 4;
>> +    } else {
>> +        if (len < 2)
>> +            goto invalid_param_len;
>> +
>> +        spg = 0;
>> +        pg_len = p[1];
>> +        p += 2;
>> +        len -= 2;
>> +    }
>> +
>> +    /*
>> +     * Only one page has changeable data, so we only support setting one
>> +     * page at a time.
>> +     */
>> +    if (len < pg_len)
>> +        goto invalid_param;
> 
>     Not 'invalid_param_len'?

No (it is more like a shortcut for the "default" case of the switch
statement below), but it should be "if (len > pg_len)" rather than <.
It's also clearer to move it closer to the end of the function.

>> +
>> +    /*
>> +     * No mode subpages supported (yet) but asking for _all_
>> +     * subpages may be valid
>> +     */
>> +    if (spg && (spg != ALL_SUB_MPAGES))
>> +        goto invalid_param;
> 
>    Rather "paramater not supported" (0x26/0x01)...

SCSI spec begs to differ... there is no reference to that sense code in
the whole SPC spec.

>> +    if (pg_len > len)
>> +        goto invalid_param_len;
> 
>    We have just checked this.

See above.

>> +    switch (pg) {
>> +    case CACHE_MPAGE:
>> +        if (ata_mselect_caching(qc, p, pg_len) < 0)
>> +            goto invalid_param;
> 
>    Rather "parameter not supported"?

Same here, SCSI spec says "invalid field in parameter list", I obey.

>> +        break;
>> +
>> +    default:        /* invalid page code */
>> +        goto invalid_param;
> 
>    Rather "paramater not supported"...

Same here too.

Thanks for the review.

Paolo

>> +    }
>> +
>> +    return 0;
> 
> MBR, Sergei



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] ata: implement MODE SELECT command
  2012-07-05 12:05     ` Paolo Bonzini
@ 2012-07-05 19:42       ` Sergei Shtylyov
  0 siblings, 0 replies; 9+ messages in thread
From: Sergei Shtylyov @ 2012-07-05 19:42 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, linux-ide, Jeff Garzik

Hello.

On 07/05/2012 04:05 PM, Paolo Bonzini wrote:

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

>>    Rather "paramater not supported" (0x26/0x01)...

> SCSI spec begs to differ... there is no reference to that sense code in
> the whole SPC spec.

   Right you are, I was just picking the more fitting ASC/ASCQ from the table
instead of reading the command description itself. :-<

MBR, Sergei

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/2] ata: MODE SELECT implementation
  2012-07-05  9:40 [PATCH v2 0/2] ata: MODE SELECT implementation Paolo Bonzini
  2012-07-05  9:40 ` [PATCH v2 1/2] ata: support MODE SENSE request for changeable and default parameters Paolo Bonzini
  2012-07-05  9:40 ` [PATCH v2 2/2] ata: implement MODE SELECT command Paolo Bonzini
@ 2012-07-26  7:25 ` Paolo Bonzini
  2012-07-26 13:47   ` Jeff Garzik
  2 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2012-07-26  7:25 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: linux-kernel, linux-ide

Il 05/07/2012 11:40, Paolo Bonzini ha scritto:
> This is a revised version of the MODE SELECT implementation from yesterday,
> augmented with support for changeable parameter requests in MODE SENSE.
> 
> Paolo Bonzini (2):
>   ata: support MODE SENSE request for changeable parameters
>   ata: implement MODE SELECT command
> 
>  drivers/ata/libata-scsi.c |  242 +++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 221 insertions(+), 21 deletions(-)
> 

Ping, no love for these? :)

Paolo


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/2] ata: MODE SELECT implementation
  2012-07-26  7:25 ` [PATCH v2 0/2] ata: MODE SELECT implementation Paolo Bonzini
@ 2012-07-26 13:47   ` Jeff Garzik
  2012-07-26 13:55     ` Paolo Bonzini
  0 siblings, 1 reply; 9+ messages in thread
From: Jeff Garzik @ 2012-07-26 13:47 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, linux-ide

On 07/26/2012 03:25 AM, Paolo Bonzini wrote:
> Il 05/07/2012 11:40, Paolo Bonzini ha scritto:
>> This is a revised version of the MODE SELECT implementation from yesterday,
>> augmented with support for changeable parameter requests in MODE SENSE.
>>
>> Paolo Bonzini (2):
>>    ata: support MODE SENSE request for changeable parameters
>>    ata: implement MODE SELECT command
>>
>>   drivers/ata/libata-scsi.c |  242 +++++++++++++++++++++++++++++++++++++++++----
>>   1 files changed, 221 insertions(+), 21 deletions(-)
>>
>
> Ping, no love for these? :)

These will go in #upstream for the next kernel...

	Jeff






^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/2] ata: MODE SELECT implementation
  2012-07-26 13:47   ` Jeff Garzik
@ 2012-07-26 13:55     ` Paolo Bonzini
  0 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2012-07-26 13:55 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: linux-kernel, linux-ide

Il 26/07/2012 15:47, Jeff Garzik ha scritto:
>>>
>>>
>>> Paolo Bonzini (2):
>>>    ata: support MODE SENSE request for changeable parameters
>>>    ata: implement MODE SELECT command
>>>
>>>   drivers/ata/libata-scsi.c |  242
>>> +++++++++++++++++++++++++++++++++++++++++----
>>>   1 files changed, 221 insertions(+), 21 deletions(-)
>>>
>>
>> Ping, no love for these? :)
> 
> These will go in #upstream for the next kernel...

Ok, thanks (BTW I pinged v2 but there is a v3 too).

Paolo


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2012-07-26 13:56 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-05  9:40 [PATCH v2 0/2] ata: MODE SELECT implementation Paolo Bonzini
2012-07-05  9:40 ` [PATCH v2 1/2] ata: support MODE SENSE request for changeable and default parameters Paolo Bonzini
2012-07-05  9:40 ` [PATCH v2 2/2] ata: implement MODE SELECT command Paolo Bonzini
2012-07-05 11:42   ` Sergei Shtylyov
2012-07-05 12:05     ` Paolo Bonzini
2012-07-05 19:42       ` Sergei Shtylyov
2012-07-26  7:25 ` [PATCH v2 0/2] ata: MODE SELECT implementation Paolo Bonzini
2012-07-26 13:47   ` Jeff Garzik
2012-07-26 13:55     ` Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).