* Re: [RFC][PATCH] libata: enable SATA disk fua detection on default
From: Michael Tokarev @ 2012-07-03 20:11 UTC (permalink / raw)
To: Jeff Garzik, linux-kernel, linux-ide, linux-scsi, Zheng Liu
In-Reply-To: <20120703090441.GA4804@gmail.com>
On 03.07.2012 13:04, Zheng Liu wrote:
> Hi Jeff,
>
> Sorry for bothering you. Could you please review this patch, or give me
> some feedbacks. If this patch can be applied, I can rebase it to the
> lastest version of mainline kernel. Thank you.
[]
>> -int libata_fua = 0;
>> +int libata_fua = 1;
I guess it can be enabled after LOTS of testing of various drives out
there, which shows that no current drives have issues with FUA anymore,
and after building a black-list of devices which misbehave. It is quite
a big project, I think. But what it gives us in return?
(I've no idea if this is the right answer, speaking of myself only)
Thanks,
/mjt
^ permalink raw reply
* RE:FOREIGN TRANSFER DEPARTMENT
From: Janet Yellen @ 2012-07-03 23:14 UTC (permalink / raw)
To: Recipients
[-- Attachment #1: Mail message body --]
[-- Type: text/plain, Size: 114 bytes --]
It has come to our notice that you have been defrauded of your hard earned money by fraudsters, View attached...
[-- Attachment #2: full details.rtf --]
[-- Type: application/octet-stream, Size: 2116 bytes --]
^ permalink raw reply
* Re: [RFC][PATCH] libata: enable SATA disk fua detection on default
From: Zheng Liu @ 2012-07-04 2:47 UTC (permalink / raw)
To: Michael Tokarev
Cc: Jeff Garzik, linux-kernel, linux-ide, linux-scsi, Zheng Liu
In-Reply-To: <4FF351F8.1000003@msgid.tls.msk.ru>
Hi Michael,
On Wed, Jul 04, 2012 at 12:11:36AM +0400, Michael Tokarev wrote:
> On 03.07.2012 13:04, Zheng Liu wrote:
> > Hi Jeff,
> >
> > Sorry for bothering you. Could you please review this patch, or give me
> > some feedbacks. If this patch can be applied, I can rebase it to the
> > lastest version of mainline kernel. Thank you.
> []
> >> -int libata_fua = 0;
> >> +int libata_fua = 1;
>
> I guess it can be enabled after LOTS of testing of various drives out
> there, which shows that no current drives have issues with FUA anymore,
> and after building a black-list of devices which misbehave. It is quite
> a big project, I think. But what it gives us in return?
>
> (I've no idea if this is the right answer, speaking of myself only)
Thanks for the reply. Indeed it is quite a big project but we enable
FUA feature for SAS disk. Is there any differences?
Regards,
Zheng
^ permalink raw reply
* Re: [RFC][PATCH] libata: enable SATA disk fua detection on default
From: Michael Tokarev @ 2012-07-04 6:36 UTC (permalink / raw)
To: Jeff Garzik, linux-kernel, linux-ide, linux-scsi, Zheng Liu
In-Reply-To: <20120704024715.GA17095@gmail.com>
On 04.07.2012 06:47, Zheng Liu wrote:
[]
>> I guess it can be enabled after LOTS of testing of various drives out
>> there, which shows that no current drives have issues with FUA anymore,
>> and after building a black-list of devices which misbehave. It is quite
>> a big project, I think. But what it gives us in return?
>>
>> (I've no idea if this is the right answer, speaking of myself only)
>
> Thanks for the reply. Indeed it is quite a big project but we enable
> FUA feature for SAS disk. Is there any differences?
Yes, there's a very big difference with SAS disks. Even in parallel SCSI
world DPO/FUA has been enabled since the day it has been implemented IIRC,
because, apparently, hardware raid controllers enabled it too. In other
words, it has been tested and proved to be working even before linux
implementation. When first SAS disks started appearing, DPO/FUA were
enabled for them in linux already -- at that time any breakage were
solely due to the particular disk model, and were easy to blacklist,
if necessary, since only a few disk models were in production.
With SATA disks, initial hardware implementation proved to be more
non-functional than functional, ie, initially there were more drives
with non-working FUA. I have a few not-so-old SATA drives here which
behaves strangely when FUA is enabled (I don't remember exact details,
but I had to disable FA again after I tried to enable it once, the
system started behaving not as good as before). So, for SATA drives,
we've exactly the opposite picture: we've some proof that "generally,
drives dislikes FUA", and now when fua has been disabled for a lot
of drives and users, turning it on by default needs lots of testing.
But I ask again: what is the benefit of turning FUA on to start with?
Thanks,
/mjt
^ permalink raw reply
* Re: [RFC][PATCH] libata: enable SATA disk fua detection on default
From: Zheng Liu @ 2012-07-04 7:06 UTC (permalink / raw)
To: Michael Tokarev
Cc: Jeff Garzik, linux-kernel, linux-ide, linux-scsi, Zheng Liu
In-Reply-To: <4FF3E478.50609@msgid.tls.msk.ru>
On Wed, Jul 04, 2012 at 10:36:40AM +0400, Michael Tokarev wrote:
> > Thanks for the reply. Indeed it is quite a big project but we enable
> > FUA feature for SAS disk. Is there any differences?
>
> Yes, there's a very big difference with SAS disks. Even in parallel SCSI
> world DPO/FUA has been enabled since the day it has been implemented IIRC,
> because, apparently, hardware raid controllers enabled it too. In other
> words, it has been tested and proved to be working even before linux
> implementation. When first SAS disks started appearing, DPO/FUA were
> enabled for them in linux already -- at that time any breakage were
> solely due to the particular disk model, and were easy to blacklist,
> if necessary, since only a few disk models were in production.
>
> With SATA disks, initial hardware implementation proved to be more
> non-functional than functional, ie, initially there were more drives
> with non-working FUA. I have a few not-so-old SATA drives here which
> behaves strangely when FUA is enabled (I don't remember exact details,
> but I had to disable FA again after I tried to enable it once, the
> system started behaving not as good as before). So, for SATA drives,
> we've exactly the opposite picture: we've some proof that "generally,
> drives dislikes FUA", and now when fua has been disabled for a lot
> of drives and users, turning it on by default needs lots of testing.
>
> But I ask again: what is the benefit of turning FUA on to start with?
Thanks for your clarification. :-)
Turning FUA on can reduce the overhead of flushes AFAIK. In our product
system we have a lot of SATA disks with FUA, but we must add a boot
parameter 'libata.fua=1' to enable it. Meanwhile there already has a
number of SATA disks that have supported this feature. So I think maybe
we can enable it.
Regards,
Zheng
^ permalink raw reply
* [PATCH] ata: implement MODE SELECT command
From: Paolo Bonzini @ 2012-07-04 11:13 UTC (permalink / raw)
To: linux-kernel, linux-ide; +Cc: 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.
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;
+ 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);
+
+ 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_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)
+ goto invalid_fld;
+
+ len = cdb[4];
+ } else {
+ if (scmd->cmd_len < 9)
+ goto invalid_fld;
+
+ len = (cdb[7] << 8) | cdb[8];
+ }
+
+ /* Test early for possible overrun. */
+ if (scsi_sglist(scmd)->length < len)
+ goto invalid_fld;
+
+ p += six_byte ? 4 : 8;
+ len -= six_byte ? 4 : 8;
+
+ /* We only support PF=1, SP=0. */
+ if ((cdb[1] & 0x11) != 0x10)
+ goto invalid_fld;
+
+ if (len == 0)
+ goto skip;
+
+ /* Parse both possible formats for the mode page headers. */
+ pg = p[0] & 0x3f;
+ if (p[0] & 0x40) {
+ spg = p[1];
+ pg_len = (p[2] << 8) | p[3];
+ p += 4;
+ len -= 4;
+ } else {
+ spg = 0;
+ pg_len = p[1];
+ p += 2;
+ len -= 2;
+ }
+
+ /* We only support setting one page at a time. */
+ if (len != pg_len)
+ goto invalid_fld;
+
+ /*
+ * 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;
+
+ switch (pg) {
+ case CACHE_MPAGE:
+ if (ata_mselect_caching(tf, p, pg_len))
+ goto invalid_fld;
+ break;
+
+ default: /* invalid page code */
+ goto invalid_fld;
+ }
+ return 0;
+
+ invalid_fld:
+ ata_scsi_set_sense(scmd, ILLEGAL_REQUEST, 0x24, 0x0);
+ /* "Invalid field in cbd" */
+ 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
@@ -3120,6 +3257,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;
}
@@ -3312,11 +3454,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
* Re: [PATCH] ata: implement MODE SELECT command
From: Sergei Shtylyov @ 2012-07-04 15:38 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, linux-ide, Jeff Garzik
In-Reply-To: <1341400436-2546-1-git-send-email-pbonzini@redhat.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
^ permalink raw reply
* solved: `softreset failed (device not ready)` gone with `CONFIG_SATA_PMP=n` (was: ASUS M2A-VM (SB600): AHCI setting in BIOS; AHCI and UDMA and `softreset failed (device not ready)`)
From: Paul Menzel @ 2012-07-04 15:33 UTC (permalink / raw)
To: linux-ide; +Cc: Robert Hancock
In-Reply-To: <4F27523F.90200@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5625 bytes --]
Dear Linux folks,
this is a follow up for my message from January [7].
Am Montag, den 30.01.2012, 20:30 -0600 schrieb Robert Hancock:
> On 01/16/2012 04:03 AM, Paul Menzel wrote:
[…]
> > Jan 13 02:13:15 joe kernel: [ 1.640626] ata1.00: ATAPI: TOSHIBA DVD-ROM SD-M1712, 1004, max UDMA/33
> > Jan 13 02:13:15 joe kernel: [ 1.656502] ata1.00: configured for UDMA/33
> > Jan 13 02:13:15 joe kernel: [ 1.796038] ata6: SATA link down (SStatus 0 SControl 300)
> > Jan 13 02:13:15 joe kernel: [ 1.796076] ata5: SATA link down (SStatus 0 SControl 300)
> > Jan 13 02:13:15 joe kernel: [ 1.796132] ata4: SATA link down (SStatus 0 SControl 300)
> > Jan 13 02:13:15 joe kernel: [ 1.808018] usb 2-1: new low speed USB device number 2 using ohci_hcd
> > Jan 13 02:13:15 joe kernel: [ 1.968017] ata3: softreset failed (device not ready)
> > Jan 13 02:13:15 joe kernel: [ 1.968063] ata3: applying PMP SRST workaround and retrying
> >
> > This seems to be related to a hardware bug in the SB600 chipset [3]. The
> > discussion [4] indicates that setting `CONFIG_SATA_PMP=n` fixes this
> > issue. Although reading the option description [5] I do not understand
> > what it does and if it is advisable to disable it.
>
> Turning that off would prevent you from using any SATA port multipliers
> (usually found in things like multi-drive external enclosures). The
> kernel already worked around the issue so I don't think you need to
> worry about this.
[…]
To make the story short the board was switched from ASUS M2A-VM (SB600) [8]
to ASRock A780FullHD (AMD 780G/SB700) [9]. The drive is still a Western
Digital WD20EARS-60MVWB0 connected to the SATA port.
New about the ASRock A780FullHD is also that some kind of AHCI BIOS ROM
is executed after the BIOS. So maybe there is a separate controller
included in the board. I do not know. I only know this adds another 5
seconds to the boot process. ;-)
Anyway I still saw the error `softreset failed (device not ready)` in
the Linux kernel ring buffer. This is shown at every boot at 2 seconds
after Linux is executed and during every resume.
$ dmesg | grep ata1
[ 1.722407] ata1: SATA max UDMA/133 abar m1024@0xfddff800 port 0xfddff900 irq 22
[ 2.212024] ata1: softreset failed (device not ready)
[ 2.212063] ata1: applying PMP SRST workaround and retrying
[ 2.384049] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[ 2.390552] ata1.00: ATA-8: WDC WD20EARS-60MVWB0, 51.0AB51, max UDMA/100
[ 2.390554] ata1.00: 3907029168 sectors, multi 0: LBA48 NCQ (depth 31/32), AA
[ 2.396516] ata1.00: configured for UDMA/100
[18757.377500] ata1: exception Emask 0x40 SAct 0x0 SErr 0x800 action 0x7
[18757.377512] ata1: SError: { HostInt }
[18757.377526] ata1: hard resetting link
[18757.868051] ata1: softreset failed (device not ready)
[18757.868063] ata1: applying PMP SRST workaround and retrying
[18758.040049] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[18758.052521] ata1.00: configured for UDMA/100
[18758.068039] ata1: EH complete
[18771.044020] ata1: softreset failed (device not ready)
[18774.560018] ata1: softreset failed (device not ready)
[18774.560020] ata1: applying PMP SRST workaround and retrying
[18774.732029] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[18774.744487] ata1.00: configured for UDMA/100
Building Linux myself and disabling SATA Port Multiplier support
CONFIG_SATA_PMP=n
the message is gone now.
$ dmesg | grep ata1
[ 1.683069] ata1: SATA max UDMA/133 abar m1024@0xfddff800 port 0xfddff900 irq 22
[ 2.000192] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[ 2.006761] ata1.00: ATA-8: WDC WD20EARS-60MVWB0, 51.0AB51, max UDMA/100
[ 2.006763] ata1.00: 3907029168 sectors, multi 0: LBA48 NCQ (depth 31/32), AA
[ 2.012806] ata1.00: configured for UDMA/100
[ 148.124029] ata1: link is slow to respond, please be patient (ready=0)
This was not there before and is after resume, but I guess this message
can be attributed to the drive needing time to spin up.
[ 152.772019] ata1: COMRESET failed (errno=-16)
[ 155.780033] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[ 155.792109] ata1.00: configured for UDMA/100
Could the error message »softreset failed (device not ready)« be
reworded, since `CONFIG_SATA_PMP=y` is used at least in Debian for the
default Linux kernel image?
softreset failed (device not ready): no Port Multiplier available, might be AMD chipset bug we will work around
Now trying to reword it myself, it is kind of hard, but hopefully we can
come up with something.
Thanks,
Paul
> > [1] https://en.wikipedia.org/wiki/Advanced_Host_Controller_Interface
> > [2] http://www.intel.com/technology/serialata/ahci.htm
> > [3] https://bugzilla.redhat.com/show_bug.cgi?id=468800
> > [4] http://www.linuxquestions.org/questions/linux-kernel-70/ata4-softreset-failed-device-not-ready-865155/
> > [5] http://cateee.net/lkddb/web-lkddb/SATA_PMP.html
> > [6] http://www.spinics.net/lists/linux-ide/msg41224.html
[7] http://www.spinics.net/lists/linux-ide/msg42654.html
[8] http://www.asus.com/Motherboards/AMD_AM2/M2AVM/
[9] http://www.asrock.com/mb/overview.asp?model=a780fullhd
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH] ata: implement MODE SELECT command
From: Paolo Bonzini @ 2012-07-04 16:03 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: linux-kernel, linux-ide, Jeff Garzik
In-Reply-To: <4FF4637A.6060901@mvista.com>
Il 04/07/2012 17:38, Sergei Shtylyov ha scritto:
> 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)?
I thought about it, but it seemed a useless complication.
>> +/**
>> + * 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?
This is just protecting the next access. The last byte is unused.
>> + 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...
Yeah, but other xlat functions do this, see ata_scsi_start_stop_xlat. I
wasn't sure why, so I went for the safer option.
> 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.
Perhaps for MODE SENSE, but not for MODE SELECT. Otherwise, where do
you get the mode data from?
Anyhow, checking buffer size vs. data size is done early, in
+ /* Test early for possible overrun. */
+ if (scsi_sglist(scmd)->length < len)
+ goto invalid_fld;
not here. What this check is doing, is ensuring that the page data does
not extend past the end of the buffer.
>> + 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.
Yes, invalid field in parameter list probably would be better. Let me
know if I should respin.
Thanks for the review!
Paolo
>> + }
>> + return 0;
>> +
>> + invalid_fld:
>> + ata_scsi_set_sense(scmd, ILLEGAL_REQUEST, 0x24, 0x0);
>> + /* "Invalid field in cbd" */
>> + return 1;
>
> MBR, Sergei
>
^ permalink raw reply
* Re: [PATCH] ata: implement MODE SELECT command
From: Sergei Shtylyov @ 2012-07-04 17:08 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, linux-ide, Jeff Garzik
In-Reply-To: <1341400436-2546-1-git-send-email-pbonzini@redhat.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
[...]
> + switch (pg) {
> + case CACHE_MPAGE:
> + if (ata_mselect_caching(tf, p, pg_len))
> + goto invalid_fld;
Error is in the data, not CDB, so returning "invalied field in CDB" doesn't
make sense...
> + break;
> +
> + default: /* invalid page code */
> + goto invalid_fld;
> + }
> + return 0;
> +
> + invalid_fld:
> + ata_scsi_set_sense(scmd, ILLEGAL_REQUEST, 0x24, 0x0);
> + /* "Invalid field in cbd" */
s/cbd/CDB/
MBR, Sergei
^ permalink raw reply
* Re: [PATCH] ata: implement MODE SELECT command
From: Sergei Shtylyov @ 2012-07-04 17:20 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, linux-ide, Jeff Garzik
In-Reply-To: <4FF46951.7050808@redhat.com>
On 07/04/2012 08:03 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)?
> I thought about it, but it seemed a useless complication.
If you bothered to implement that, go all the way. :-)
>>> +/**
>>> + * 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)
>>> +{
[...]
>>> + /*
>>> + * 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.
> Perhaps for MODE SENSE, but not for MODE SELECT. Otherwise, where do
> you get the mode data from?
They simply don't change?
> Anyhow, checking buffer size vs. data size is done early, in
> + /* Test early for possible overrun. */
> + if (scsi_sglist(scmd)->length < len)
> + goto invalid_fld;
> not here. What this check is doing, is ensuring that the page data does
> not extend past the end of the buffer.
That's what I say is valid. You can get incomplete data, according to the
buffer size. Though maybe it was only my impression...
>>> + 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.
> Yes, invalid field in parameter list probably would be better.
Or "parameter not supported" (0x26/0x01).
> Let me know if I should respin.
I'd prefer if you respin.
> Thanks for the review!
> Paolo
MBR, Sergei
^ permalink raw reply
* Re: [PATCH] ata: implement MODE SELECT command
From: Paolo Bonzini @ 2012-07-04 19:34 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: linux-kernel, linux-ide, Jeff Garzik
In-Reply-To: <4FF47B71.4030603@mvista.com>
Il 04/07/2012 19:20, Sergei Shtylyov ha scritto:
>> > Perhaps for MODE SENSE, but not for MODE SELECT. Otherwise, where do
>> > you get the mode data from?
> They simply don't change?
The standard says "If the parameter list length results in the
truncation of any mode parameter header, mode parameter block
descriptor(s), or mode page, then the command shall be terminated with
CHECK CONDITION status, with the sense key set to ILLEGAL REQUEST, and
the additional sense code set to PARAMETER LIST LENGTH ERROR".
Paolo
^ permalink raw reply
* very important
From: Ulrich Claypole @ 2012-07-04 13:03 UTC (permalink / raw)
Greetings,
I have a huge financial transfer proposal for you, just get back for more
details.
Thanks,
Barriser Ulrich Claypole.
^ permalink raw reply
* [PATCH v2 0/2] ata: MODE SELECT implementation
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
* [PATCH v2 1/2] ata: support MODE SENSE request for changeable and default parameters
From: Paolo Bonzini @ 2012-07-05 9:40 UTC (permalink / raw)
To: linux-kernel, linux-ide; +Cc: Sergei Shtylyov, Jeff Garzik
In-Reply-To: <1341481235-12708-1-git-send-email-pbonzini@redhat.com>
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
* [PATCH v2 2/2] ata: implement MODE SELECT command
From: Paolo Bonzini @ 2012-07-05 9:40 UTC (permalink / raw)
To: linux-kernel, linux-ide; +Cc: Sergei Shtylyov, Jeff Garzik
In-Reply-To: <1341481235-12708-1-git-send-email-pbonzini@redhat.com>
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
* Re: [PATCH v2 2/2] ata: implement MODE SELECT command
From: Sergei Shtylyov @ 2012-07-05 11:42 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, linux-ide, Jeff Garzik
In-Reply-To: <1341481235-12708-3-git-send-email-pbonzini@redhat.com>
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
* Re: [PATCH v2 2/2] ata: implement MODE SELECT command
From: Paolo Bonzini @ 2012-07-05 12:05 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: linux-kernel, linux-ide, Jeff Garzik
In-Reply-To: <4FF57DAA.8010405@mvista.com>
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
* [PATCH v3 0/2] ata: MODE SELECT implementation
From: Paolo Bonzini @ 2012-07-05 12:18 UTC (permalink / raw)
To: linux-kernel, linux-ide; +Cc: sshtylyov, jgarzik
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
* [PATCH v3 1/2] ata: support MODE SENSE request for changeable and default parameters
From: Paolo Bonzini @ 2012-07-05 12:18 UTC (permalink / raw)
To: linux-kernel, linux-ide; +Cc: sshtylyov, jgarzik
In-Reply-To: <1341490701-15348-1-git-send-email-pbonzini@redhat.com>
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
* [PATCH v3 2/2] ata: implement MODE SELECT command
From: Paolo Bonzini @ 2012-07-05 12:18 UTC (permalink / raw)
To: linux-kernel, linux-ide; +Cc: sshtylyov, jgarzik
In-Reply-To: <1341490701-15348-1-git-send-email-pbonzini@redhat.com>
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>
---
v2->v3: ensure that only the first page of the sg list is
accessed
drivers/ata/libata-scsi.c | 194 +++++++++++++++++++++++++++++++++++++++++++--
1 files changed, 188 insertions(+), 6 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index d5aeb26..e13cf0c 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,188 @@ 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, bd_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];
+ hdr_len = 4;
+ } else {
+ if (scmd->cmd_len < 9)
+ goto invalid_fld;
+
+ len = (cdb[7] << 8) + cdb[8];
+ hdr_len = 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 (len < hdr_len)
+ goto invalid_param_len;
+
+ if (six_byte)
+ bd_len = p[3];
+ else
+ bd_len = (p[6] << 8) + p[7];
+
+ len -= hdr_len;
+ p += hdr_len;
+ if (len < bd_len)
+ goto invalid_param_len;
+ if (bd_len != 0 && bd_len != 8)
+ goto invalid_param;
+
+ len -= bd_len;
+ p += bd_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;
+ }
+
+ /*
+ * 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;
+ }
+
+ /*
+ * Only one page has changeable data, so we only support setting one
+ * page at a time.
+ */
+ if (len > pg_len)
+ 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 +3329,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 +3526,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
* [Bug 15445] pata_jmicron incorrectly downgrades CompactFlash to UDMA/33
From: bugzilla-daemon @ 2012-07-05 15:43 UTC (permalink / raw)
To: linux-ide
In-Reply-To: <bug-15445-11633@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=15445
Alan <alan@lxorguk.ukuu.org.uk> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
CC| |alan@lxorguk.ukuu.org.uk
Resolution| |DOCUMENTED
--
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [PATCH v2 2/2] ata: implement MODE SELECT command
From: Sergei Shtylyov @ 2012-07-05 19:42 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, linux-ide, Jeff Garzik
In-Reply-To: <4FF58320.1050606@redhat.com>
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
* Hi! I want to meet an interesting man. You seem to suit the conditions perfectly...
From: Carline Shumaker @ 2012-07-06 11:19 UTC (permalink / raw)
To: jahve@vip.cybercity.dk
Hello pretty!
I am sure we'll be good friends. I am Carline, there is so much I can tell u;)
I saw your photos and I consider u to be quite magnetic.
It's interesting what u have to tell! :)
^ permalink raw reply
* [PATCH] ata: pata_imx: Convert to clk_prepare_enable/clk_disable_unprepare
From: Fabio Estevam @ 2012-07-07 22:34 UTC (permalink / raw)
To: jgarzik; +Cc: arnaud.patard, kernel, linux-ide, Fabio Estevam
From: Fabio Estevam <fabio.estevam@freescale.com>
With the new i.mx clock framework, we need to use clk_prepare_enable/clk_disable_unprepare.
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
drivers/ata/pata_imx.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/ata/pata_imx.c b/drivers/ata/pata_imx.c
index c5af97f..87bb05b 100644
--- a/drivers/ata/pata_imx.c
+++ b/drivers/ata/pata_imx.c
@@ -118,7 +118,7 @@ static int __devinit pata_imx_probe(struct platform_device *pdev)
return PTR_ERR(priv->clk);
}
- clk_enable(priv->clk);
+ clk_prepare_enable(priv->clk);
host = ata_host_alloc(&pdev->dev, 1);
if (!host)
@@ -162,7 +162,7 @@ static int __devinit pata_imx_probe(struct platform_device *pdev)
&pata_imx_sht);
free_priv:
- clk_disable(priv->clk);
+ clk_disable_unprepare(priv->clk);
clk_put(priv->clk);
return -ENOMEM;
}
@@ -176,7 +176,7 @@ static int __devexit pata_imx_remove(struct platform_device *pdev)
__raw_writel(0, priv->host_regs + PATA_IMX_ATA_INT_EN);
- clk_disable(priv->clk);
+ clk_disable_unprepare(priv->clk);
clk_put(priv->clk);
return 0;
@@ -194,7 +194,7 @@ static int pata_imx_suspend(struct device *dev)
__raw_writel(0, priv->host_regs + PATA_IMX_ATA_INT_EN);
priv->ata_ctl =
__raw_readl(priv->host_regs + PATA_IMX_ATA_CONTROL);
- clk_disable(priv->clk);
+ clk_disable_unprepare(priv->clk);
}
return ret;
@@ -205,7 +205,7 @@ static int pata_imx_resume(struct device *dev)
struct ata_host *host = dev_get_drvdata(dev);
struct pata_imx_priv *priv = host->private_data;
- clk_enable(priv->clk);
+ clk_prepare_enable(priv->clk);
__raw_writel(priv->ata_ctl, priv->host_regs + PATA_IMX_ATA_CONTROL);
--
1.7.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox