* [PATCH] scsi/libata: Support variable-length cdb of ata pass-thru(32)-bug fixed.
From: Minwoo Im @ 2017-06-19 13:22 UTC (permalink / raw)
To: Tejun Heo, James E.J. Bottomley, Martin K. Petersen,
Bart Van Assche
Cc: linux-ide, linux-scsi, Minwoo Im
In-Reply-To: <1497697243-3724-1-git-send-email-dn3108@gmail.com>
Bug fixed in libata-scsi.c from previous patch.
Bit shifting of service action value in ata_scsi_var_len_cdb_xlat() was reversed.
I have tested a ata pass-thru(32) command with sg_io and it worked well.
I'm sorry for making a confusion.
Signed-off-by: Minwoo Im <dn3108@gmail.com>
---
drivers/ata/libata-core.c | 2 +-
drivers/ata/libata-scsi.c | 95 ++++++++++++++++++++++++++++++++++++++++++---
include/scsi/scsi_proto.h | 1 +
3 files changed, 91 insertions(+), 7 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index e157a0e..f1d3ba4 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2587,7 +2587,7 @@ int ata_dev_configure(struct ata_device *dev)
}
ata_dev_config_sense_reporting(dev);
ata_dev_config_zac(dev);
- dev->cdb_len = 16;
+ dev->cdb_len = 32;
}
/* ATAPI-specific feature tests */
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 49ba983..39f23e0 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3127,7 +3127,7 @@ static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
* ata_scsi_pass_thru - convert ATA pass-thru CDB to taskfile
* @qc: command structure to be initialized
*
- * Handles either 12 or 16-byte versions of the CDB.
+ * Handles either 12, 16, or 32-byte versions of the CDB.
*
* RETURNS:
* Zero on success, non-zero on failure.
@@ -3140,13 +3140,36 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
const u8 *cdb = scmd->cmnd;
u16 fp;
- if ((tf->protocol = ata_scsi_map_proto(cdb[1])) == ATA_PROT_UNKNOWN) {
+ /*
+ * if SCSI operation code in cdb[0] is ATA_12 or ATA_16,
+ * then cdb[1] will contain protocol of ATA PASS-THROUGH.
+ * otherwise, Its operation code shall be ATA_32(7Fh).
+ * in this case, cdb[10] will contain protocol of it.
+ * we call this command as a variable-length cdb.
+ */
+ if (cdb[0] == ATA_12 || cdb[0] == ATA_16)
+ tf->protocol = ata_scsi_map_proto(cdb[1]);
+ else
+ tf->protocol = ata_scsi_map_proto(cdb[10]);
+
+ if (tf->protocol == ATA_PROT_UNKNOWN) {
fp = 1;
goto invalid_fld;
}
- if (ata_is_ncq(tf->protocol) && (cdb[2] & 0x3) == 0)
- tf->protocol = ATA_PROT_NCQ_NODATA;
+ /*
+ * if protocol has a NCQ property and transfer length is 0,
+ * then the protocol will be marked as a NCQ_NODATA.
+ * in case of ATA_12 and ATA_16, cdb[2] has a t_length field.
+ * otherwise, cdb[11] will have a t_length field.
+ */
+ if (cdb[0] == ATA_12 || cdb[0] == ATA_16) {
+ if (ata_is_ncq(tf->protocol) && (cdb[2] & 0x3) == 0)
+ tf->protocol = ATA_PROT_NCQ_NODATA;
+ } else {
+ if (ata_is_ncq(tf->protocol) && (cdb[11] & 0x3) == 0)
+ tf->protocol = ATA_PROT_NCQ_NODATA;
+ }
/* enable LBA */
tf->flags |= ATA_TFLAG_LBA;
@@ -3181,7 +3204,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
tf->lbah = cdb[12];
tf->device = cdb[13];
tf->command = cdb[14];
- } else {
+ } else if (cdb[0] == ATA_12) {
/*
* 12-byte CDB - incapable of extended commands.
*/
@@ -3194,6 +3217,31 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
tf->lbah = cdb[7];
tf->device = cdb[8];
tf->command = cdb[9];
+ } else {
+ /*
+ * 32-byte CDB - may contain extended command fields.
+ *
+ * If that is the case, copy the upper byte register values.
+ */
+ if (cdb[10] & 0x01) {
+ tf->hob_feature = cdb[20];
+ tf->hob_nsect = cdb[22];
+ tf->hob_lbal = cdb[16];
+ tf->hob_lbam = cdb[15];
+ tf->hob_lbah = cdb[14];
+ tf->flags |= ATA_TFLAG_LBA48;
+ } else
+ tf->flags &= ~ATA_TFLAG_LBA48;
+
+ tf->feature = cdb[21];
+ tf->nsect = cdb[23];
+ tf->lbal = cdb[19];
+ tf->lbam = cdb[18];
+ tf->lbah = cdb[17];
+ tf->device = cdb[24];
+ tf->command = cdb[25];
+ tf->auxiliary = (cdb[28] << 24) | (cdb[29] << 16)
+ | (cdb[30] << 8) | cdb[31];
}
/* For NCQ commands copy the tag value */
@@ -4068,6 +4116,33 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
}
/**
+ * ata_scsi_var_len_cdb_xlat - SATL Variable Length CDB to Handler
+ * @qc: Command to be translated
+ *
+ * Translate a SCSI variable length CDB to specified commands.
+ * It checks a service action value in CDB to call corresponding handler.
+ *
+ * RETURNS:
+ * Zero on success, non-zero on failure
+ */
+
+static unsigned int ata_scsi_var_len_cdb_xlat(struct ata_queued_cmd *qc)
+{
+ struct scsi_cmnd *scmd = qc->scsicmd;
+ const u8 *cdb = scmd->cmnd;
+ const u16 sa = (cdb[8] << 8) | cdb[9]; /* service action */
+
+ // if service action represents a ata pass-thru(32) command,
+ // then pass it to ata_scsi_pass_thru handler.
+ if (sa == ATA_32)
+ return ata_scsi_pass_thru(qc);
+
+unspprt_sa:
+ /* unsupported service action */
+ return 1;
+}
+
+/**
* ata_get_xlat_func - check if SCSI to ATA translation is possible
* @dev: ATA device
* @cmd: SCSI command opcode to consider
@@ -4107,6 +4182,9 @@ 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 VARIABLE_LENGTH_CMD:
+ return ata_scsi_var_len_cdb_xlat;
+
case MODE_SELECT:
case MODE_SELECT_10:
return ata_scsi_mode_select_xlat;
@@ -4385,7 +4463,12 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
shost->max_id = 16;
shost->max_lun = 1;
shost->max_channel = 1;
- shost->max_cmd_len = 16;
+ /*
+ * SPC-3, SPC-4: Definition of CDB
+ * A CDB may have a fixed length of up to 16 bytes or
+ * variable length of between 12 and 260 bytes.
+ */
+ shost->max_cmd_len = 260;
/* Schedule policy is determined by ->qc_defer()
* callback and it needs to see every deferred qc.
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index ce78ec8..8545e34 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -164,6 +164,7 @@
#define WRITE_SAME_32 0x0d
/* Values for T10/04-262r7 */
+#define ATA_32 0x1ff0 /* 32-byte pass-thru, service action */
#define ATA_16 0x85 /* 16-byte pass-thru */
#define ATA_12 0xa1 /* 12-byte pass-thru */
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH] libata: do not initialise statics to 0 ERROR fix.
From: Tejun Heo @ 2017-06-19 17:38 UTC (permalink / raw)
To: Minwoo Im; +Cc: linux-ide
In-Reply-To: <1497682984-5554-1-git-send-email-dn3108@gmail.com>
Hello,
On Sat, Jun 17, 2017 at 04:03:04PM +0900, Minwoo Im wrote:
> ERROR: do not initialise statics to 0 in four lines in libata-core.c.
> Signed-off-by: Minwoo Im <dn3108@gmail.com>
I don't think there's much value in sweeping the code base with
checkpatch. We can fix them when changing code around them or there's
a meaningful gain from it (e.g. doc updates which improve doc
processing) but otherwise I think it's churns for churns' sake.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] ata: pata_octeon_cf: make of_device_ids const.
From: Tejun Heo @ 2017-06-19 18:04 UTC (permalink / raw)
To: Arvind Yadav; +Cc: b.zolnierkie, linux-ide, linux-kernel
In-Reply-To: <dd1977ad60634797aeb04d3785bafffa0ec53026.1497613944.git.arvind.yadav.cs@gmail.com>
On Fri, Jun 16, 2017 at 05:24:41PM +0530, Arvind Yadav wrote:
> of_device_ids are not supposed to change at runtime. All functions
> working with of_device_ids provided by <linux/of.h> work with const
> of_device_ids. So mark the non-const structs as const.
>
> File size before:
> text data bss dec hex filename
> 465 696 4 1165 48d drivers/ata/pata_octeon_cf.o
>
> File size after constify octeon_cf_match.
> text data bss dec hex filename
> 865 280 4 1149 47d drivers/ata/pata_octeon_cf.o
>
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
Applied to libata/for-4.13.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] ata: sata_rcar: make of_device_ids const.
From: Tejun Heo @ 2017-06-19 18:05 UTC (permalink / raw)
To: Arvind Yadav; +Cc: b.zolnierkie, linux-ide, linux-kernel
In-Reply-To: <474047fe3280ca103275247df6aa67288220a9d4.1497614383.git.arvind.yadav.cs@gmail.com>
On Fri, Jun 16, 2017 at 05:32:21PM +0530, Arvind Yadav wrote:
> of_device_ids are not supposed to change at runtime. All functions
> working with of_device_ids provided by <linux/of.h> work with const
> of_device_ids. So mark the non-const structs as const.
>
> File size before:
> text data bss dec hex filename
> 3946 2296 0 6242 1862 drivers/ata/sata_rcar.o
>
> File size after constify sata_rcar_match.
> text data bss dec hex filename
> 5554 696 0 6250 186a drivers/ata/sata_rcar.o
>
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
Applied to libata/for-4.13.
Thanks.
--
tejun
^ permalink raw reply
* Re: Reply Urgent
From: INFO @ 2017-06-19 19:39 UTC (permalink / raw)
Hello,
How are you doing? I have been sent to inform you that, We have an
inheritance of a deceased client with your surname. Contact Mr Andrew
Bailey Reply Email To: myinfand@gmail.com with your "Full Names" for
more info. Thanks for your understanding.
Reply ASAP thank you.
Melissa.
----------------------------------
Correo Corporativo Hospital Universitario del Valle E.S.E
*******************************************************************************
"Estamos re-dimensionandonos para crecer!"
******************************************************************************
^ permalink raw reply
* Re: [PATCH] libata: do not initialise statics to 0 ERROR fix.
From: Minwoo Im @ 2017-06-20 0:59 UTC (permalink / raw)
To: dn3108; +Cc: linux-ide
In-Reply-To: <20170619173835.GA12062@htj.duckdns.org>
On 6/20/17, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Sat, Jun 17, 2017 at 04:03:04PM +0900, Minwoo Im wrote:
>> ERROR: do not initialise statics to 0 in four lines in libata-core.c.
>> Signed-off-by: Minwoo Im <dn3108@gmail.com>
>
> I don't think there's much value in sweeping the code base with
> checkpatch. We can fix them when changing code around them or there's
> a meaningful gain from it (e.g. doc updates which improve doc
> processing) but otherwise I think it's churns for churns' sake.
>
> Thanks.
>
> --
> tejun
good
^ permalink raw reply
* Re: [PATCH] libata: do not initialise statics to 0 ERROR fix.
From: Minwoo Im @ 2017-06-20 1:03 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide
In-Reply-To: <20170619173835.GA12062@htj.duckdns.org>
On 6/20/17, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Sat, Jun 17, 2017 at 04:03:04PM +0900, Minwoo Im wrote:
>> ERROR: do not initialise statics to 0 in four lines in libata-core.c.
>> Signed-off-by: Minwoo Im <dn3108@gmail.com>
>
> I don't think there's much value in sweeping the code base with
> checkpatch. We can fix them when changing code around them or there's
> a meaningful gain from it (e.g. doc updates which improve doc
> processing) but otherwise I think it's churns for churns' sake.
>
> Thanks.
>
> --
> tejun
>
(resending in plain text to mail loop)
Hi Tejun,
I completely agree with your thought.
Thanks for giving your opinion.
Minwoo.
^ permalink raw reply
* [PATCH] libata: make function name in comment be matched with its own.
From: Minwoo Im @ 2017-06-20 10:21 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, Minwoo Im
the function name in comment used to be ata_scsiop_mode_select() for
ata_scsi_mode_select_xlat(). function comment has been updated.
Signed-off-by: Minwoo Im <dn3108@gmail.com>
---
drivers/ata/libata-scsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 49ba983..f80e415 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3909,7 +3909,7 @@ static int ata_mselect_control(struct ata_queued_cmd *qc,
}
/**
- * ata_scsiop_mode_select - Simulate MODE SELECT 6, 10 commands
+ * ata_scsi_mode_select_xlat - Simulate MODE SELECT 6, 10 commands
* @qc: Storage for translated ATA taskfile
*
* Converts a MODE SELECT command to an ATA SET FEATURES taskfile.
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH] libata: make function name in comment be matched with its own.
From: Tejun Heo @ 2017-06-20 17:37 UTC (permalink / raw)
To: Minwoo Im; +Cc: linux-ide
In-Reply-To: <1497954090-8130-1-git-send-email-dn3108@gmail.com>
On Tue, Jun 20, 2017 at 07:21:30PM +0900, Minwoo Im wrote:
> the function name in comment used to be ata_scsiop_mode_select() for
> ata_scsi_mode_select_xlat(). function comment has been updated.
>
> Signed-off-by: Minwoo Im <dn3108@gmail.com>
Applied to libata/for-4.13.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] scsi/libata: Support variable-length cdb of ata pass-thru(32).
From: Bart Van Assche @ 2017-06-21 18:52 UTC (permalink / raw)
To: jejb@linux.vnet.ibm.com, tj@kernel.org, dn3108@gmail.com,
martin.petersen@oracle.com
Cc: linux-scsi@vger.kernel.org, linux-ide@vger.kernel.org
In-Reply-To: <1497697243-3724-1-git-send-email-dn3108@gmail.com>
On Sat, 2017-06-17 at 20:00 +0900, Minwoo Im wrote:
> - if ((tf->protocol = ata_scsi_map_proto(cdb[1])) == ATA_PROT_UNKNOWN) {
> + /*
> + * if SCSI operation code in cdb[0] is ATA_12 or ATA_16,
> + * then cdb[1] will contain protocol of ATA PASS-THROUGH.
> + * otherwise, Its operation code shall be ATA_32(7Fh).
> + * in this case, cdb[10] will contain protocol of it.
> + * we call this command as a variable-length cdb.
> + */
> + if (cdb[0] == ATA_12 || cdb[0] == ATA_16)
> + tf->protocol = ata_scsi_map_proto(cdb[1]);
> + else
> + tf->protocol = ata_scsi_map_proto(cdb[10]);
> +
> + if (tf->protocol == ATA_PROT_UNKNOWN) {
> fp = 1;
> goto invalid_fld;
> }
Hello Minwoo,
Please consider introducing a variable (e.g. called "cdb_offset") in which the
value 9 is stored for 32-byte CDBs and 0 for 12-byte and 16-byte CDBs. That
will allow to rewrite the above code as follows:
tf->protocol = ata_scsi_map_proto(cdb[1 + cdb_offset])
I think that will allow to remove most "if (cdb[0] == ATA_12 || cdb[0] == ATA_16)"
statements introduced by this patch.
> + tf->auxiliary = (cdb[28] << 24) | (cdb[29] << 16)
> + | (cdb[30] << 8) | cdb[31];
Please use get_unaligned_be32() instead of open-coding it.
> + const u16 sa = (cdb[8] >> 8) | cdb[9]; /* service action */
Please use get_unaligned_be16() instead of open-coding it.
> @@ -4385,7 +4463,12 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
> shost->max_id = 16;
> shost->max_lun = 1;
> shost->max_channel = 1;
> - shost->max_cmd_len = 16;
> + /*
> + * SPC-3, SPC-4: Definition of CDB
> + * A CDB may have a fixed length of up to 16 bytes or
> + * variable length of between 12 and 260 bytes.
> + */
> + shost->max_cmd_len = 260;
Does ATA pass-through really support 260-byte CDBs or is the maximum CDB length
that is supported by ATA_32 perhaps 32 bytes?
Thanks,
Bart.
^ permalink raw reply
* [PATCH] ata: ftide010: fix resource printing
From: Arnd Bergmann @ 2017-06-21 22:00 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Tejun Heo, Linus Walleij
Cc: Arnd Bergmann, Hans Ulli Kroll, linux-ide, linux-kernel
The new driver uses an incorrect format string for resource_size_t:
drivers/ata/pata_ftide010.c: In function 'pata_ftide010_probe':
drivers/ata/pata_ftide010.c:520:17: warning: format '%x' expects argument of type 'unsigned int', but argument 5 has type 'resource_size_t {aka long long unsigned int}' [-Wformat=]
The nicest way to print the address is to pretty-print the resource
using %pR.
Fixes: be4e456ed3a5 ("ata: Add driver for Faraday Technology FTIDE010")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/ata/pata_ftide010.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/ata/pata_ftide010.c b/drivers/ata/pata_ftide010.c
index 7b7e417ba8ba..5d4b72e21161 100644
--- a/drivers/ata/pata_ftide010.c
+++ b/drivers/ata/pata_ftide010.c
@@ -517,8 +517,8 @@ static int pata_ftide010_probe(struct platform_device *pdev)
ata_sff_std_ports(ioaddr);
}
- dev_info(dev, "device ID %08x, irq %d, io base 0x%08x\n",
- readl(ftide->base + FTIDE010_IDE_DEVICE_ID), irq, res->start);
+ dev_info(dev, "device ID %08x, irq %d, reg %pR\n",
+ readl(ftide->base + FTIDE010_IDE_DEVICE_ID), irq, res);
ret = ata_host_activate(ftide->host, irq, ata_bmdma_interrupt,
0, &pata_ftide010_sht);
--
2.9.0
^ permalink raw reply related
* Re: [PATCH] ata: ftide010: fix resource printing
From: Tejun Heo @ 2017-06-21 22:06 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Bartlomiej Zolnierkiewicz, Linus Walleij, Hans Ulli Kroll,
linux-ide, linux-kernel
In-Reply-To: <20170621220115.4100076-1-arnd@arndb.de>
On Thu, Jun 22, 2017 at 12:00:55AM +0200, Arnd Bergmann wrote:
> The new driver uses an incorrect format string for resource_size_t:
>
> drivers/ata/pata_ftide010.c: In function 'pata_ftide010_probe':
> drivers/ata/pata_ftide010.c:520:17: warning: format '%x' expects argument of type 'unsigned int', but argument 5 has type 'resource_size_t {aka long long unsigned int}' [-Wformat=]
>
> The nicest way to print the address is to pretty-print the resource
> using %pR.
>
> Fixes: be4e456ed3a5 ("ata: Add driver for Faraday Technology FTIDE010")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Applied to libata/for-4.13.
Thanks.
--
Tejun
^ permalink raw reply
* AW: Wir geben jährlich Darlehen für 2% Zinsen aus
From: Bernhard Stöckl @ 2017-06-21 22:51 UTC (permalink / raw)
Wir vergeben Kredite mit einem Zinssatz von jährlich 2%.
Die Bearbeitung des Antrags erfolgt rasch, wir verlangen keine Gebühren, was sie beantragen werden wir annehmen. Wir bewilligen Kredite von bis zu 40 Millionen Euro und von mindestens 15.000 Euro. Sie können einen geschäftlichen oder privaten Kredit beantrage, wobei Sie bei Geschäftskrediten eine Zahlungsfrist von einem Jahr erhalten.
Anfragen bitte per Mail an: sunrisefunding01@hotmail.com
^ permalink raw reply
* [PATCH 2/3] libata: Add the AHCI_HFLAG_NO_WRITE_TO_RO flag
From: Doug Berger @ 2017-06-21 23:20 UTC (permalink / raw)
To: tj; +Cc: linux-ide, linux-kernel, Doug Berger
In-Reply-To: <20170621232014.27790-1-opendmb@gmail.com>
While most hardware will simply ignore a write to a read-only register,
some hardware will signal an abort if this occurs.
This commit introduces the flag AHCI_HFLAG_NO_WRITE_TO_RO to prevent the
AHCI library from attempting to write to the HOST_CAP, HOST_CAP2, and
HOST_PORTS_IMPL registers which may be read-only.
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
drivers/ata/ahci.h | 2 ++
drivers/ata/libahci.c | 3 ++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index ee176e4af97a..8b61123d2c3c 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -249,6 +249,8 @@ enum {
#endif
AHCI_HFLAG_WAKE_BEFORE_STOP = (1 << 22), /* wake before DMA stop */
AHCI_HFLAG_YES_ALPM = (1 << 23), /* force ALPM cap on */
+ AHCI_HFLAG_NO_WRITE_TO_RO = (1 << 24), /* don't write to read
+ only registers */
/* ap->flags bits */
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 4462f8a8cf2c..3e286d86ab42 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -945,7 +945,8 @@ int ahci_reset_controller(struct ata_host *host)
/* Some registers might be cleared on reset. Restore
* initial values.
*/
- ahci_restore_initial_config(host);
+ if (!(hpriv->flags & AHCI_HFLAG_NO_WRITE_TO_RO))
+ ahci_restore_initial_config(host);
} else
dev_info(host->dev, "skipping global host reset\n");
--
2.13.0
^ permalink raw reply related
* [PATCH 3/3] ata: ahci_brcm: Avoid writing to read-only registers
From: Doug Berger @ 2017-06-21 23:20 UTC (permalink / raw)
To: tj; +Cc: linux-ide, linux-kernel, Doug Berger
In-Reply-To: <20170621232014.27790-1-opendmb@gmail.com>
This commit makes use of the AHCI_HFLAG_YES_ALPM flag to prevent
the driver from writing to the read-only Host Capability register.
It also sets the AHCI_HFLAG_NO_WRITE_TO_RO flag to prevent the AHCI
library from writing to read-only registers.
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
drivers/ata/ahci_brcm.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 6f8a7341fa08..5936d1679bf3 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -39,7 +39,6 @@
#define PIODATA_ENDIAN_SHIFT 6
#define ENDIAN_SWAP_NONE 0
#define ENDIAN_SWAP_FULL 2
- #define OVERRIDE_HWINIT BIT(16)
#define SATA_TOP_CTRL_TP_CTRL 0x8
#define SATA_TOP_CTRL_PHY_CTRL 0xc
#define SATA_TOP_CTRL_PHY_CTRL_1 0x0
@@ -126,17 +125,13 @@ static inline void brcm_sata_writereg(u32 val, void __iomem *addr)
static void brcm_sata_alpm_init(struct ahci_host_priv *hpriv)
{
struct brcm_ahci_priv *priv = hpriv->plat_data;
- u32 bus_ctrl, port_ctrl, host_caps;
+ u32 port_ctrl, host_caps;
int i;
/* Enable support for ALPM */
- bus_ctrl = brcm_sata_readreg(priv->top_ctrl +
- SATA_TOP_CTRL_BUS_CTRL);
- brcm_sata_writereg(bus_ctrl | OVERRIDE_HWINIT,
- priv->top_ctrl + SATA_TOP_CTRL_BUS_CTRL);
host_caps = readl(hpriv->mmio + HOST_CAP);
- writel(host_caps | HOST_CAP_ALPM, hpriv->mmio);
- brcm_sata_writereg(bus_ctrl, priv->top_ctrl + SATA_TOP_CTRL_BUS_CTRL);
+ if (!(host_caps & HOST_CAP_ALPM))
+ hpriv->flags |= AHCI_HFLAG_YES_ALPM;
/*
* Adjust timeout to allow PLL sufficient time to lock while waking
@@ -360,6 +355,7 @@ static int brcm_ahci_probe(struct platform_device *pdev)
if (priv->quirks & BRCM_AHCI_QUIRK_NO_NCQ)
hpriv->flags |= AHCI_HFLAG_NO_NCQ;
+ hpriv->flags |= AHCI_HFLAG_NO_WRITE_TO_RO;
ret = ahci_platform_init_host(pdev, hpriv, &ahci_brcm_port_info,
&ahci_platform_sht);
--
2.13.0
^ permalink raw reply related
* [PATCH 0/3] libata: prevent writes to read-only registers
From: Doug Berger @ 2017-06-21 23:20 UTC (permalink / raw)
To: tj; +Cc: linux-ide, linux-kernel, Doug Berger
Recent Broadcom SoCs allow for the trapping of write accesses to
read-only registers. This is only useful if such accesses are
exceptional, so it is desirable to prevent such accesses in normal
operation. To that end, this set of commits proposes adding two
flags to the libata core.
The first allows for a quirk that exists in some Broadcom devices
that are capable of supporting Aggresive Link Power Management even
though it is not reported by their read-only capability register.
This removes a need for the Broadcom driver to write to it's
read-only capability register.
The second is a notification to the libata-core that it should not
write to any standard registers that are defined to be read-only.
The Broadcom driver is then modified to use these two general
purpose flags.
Doug Berger (3):
libata: Add the AHCI_HFLAG_YES_ALPM flag
libata: Add the AHCI_HFLAG_NO_WRITE_TO_RO flag
ata: ahci_brcm: Avoid writing to read-only registers
drivers/ata/ahci.h | 3 +++
drivers/ata/ahci_brcm.c | 12 ++++--------
drivers/ata/libahci.c | 8 +++++++-
3 files changed, 14 insertions(+), 9 deletions(-)
--
2.13.0
^ permalink raw reply
* [PATCH 1/3] libata: Add the AHCI_HFLAG_YES_ALPM flag
From: Doug Berger @ 2017-06-21 23:20 UTC (permalink / raw)
To: tj; +Cc: linux-ide, linux-kernel, Doug Berger
In-Reply-To: <20170621232014.27790-1-opendmb@gmail.com>
Some hardware is capable of supporting Aggresive Link Power Management
even though it is not indicated by the Host Capability register.
This commit adds the AHCI_HFLAG_YES_ALPM flag to the AHCI library to
allow indication of this quirk when the Host Capability register is
Read Only and therefore cannot be changed.
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
drivers/ata/ahci.h | 1 +
drivers/ata/libahci.c | 5 +++++
2 files changed, 6 insertions(+)
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 30f67a1a4f54..ee176e4af97a 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -248,6 +248,7 @@ enum {
AHCI_HFLAG_MULTI_MSI = 0,
#endif
AHCI_HFLAG_WAKE_BEFORE_STOP = (1 << 22), /* wake before DMA stop */
+ AHCI_HFLAG_YES_ALPM = (1 << 23), /* force ALPM cap on */
/* ap->flags bits */
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index b3a685ad9b87..4462f8a8cf2c 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -504,6 +504,11 @@ void ahci_save_initial_config(struct device *dev, struct ahci_host_priv *hpriv)
cap &= ~HOST_CAP_FBS;
}
+ if (!(cap & HOST_CAP_ALPM) && (hpriv->flags & AHCI_HFLAG_YES_ALPM)) {
+ dev_info(dev, "controller can do ALPM, turning on CAP_ALPM\n");
+ cap |= HOST_CAP_ALPM;
+ }
+
if (hpriv->force_port_map && port_map != hpriv->force_port_map) {
dev_info(dev, "forcing port_map 0x%x -> 0x%x\n",
port_map, hpriv->force_port_map);
--
2.13.0
^ permalink raw reply related
* let me know
From: CELINE @ 2017-06-21 13:58 UTC (permalink / raw)
To: Recipients
Hello, important charity foundation proposal to discuss with you, if you are interested please reply urgently for details.
with love,
CELINE
^ permalink raw reply
* Re: [PATCH 0/3] libata: prevent writes to read-only registers
From: Tejun Heo @ 2017-06-22 20:13 UTC (permalink / raw)
To: Doug Berger; +Cc: linux-ide, linux-kernel
In-Reply-To: <20170621232014.27790-1-opendmb@gmail.com>
On Wed, Jun 21, 2017 at 04:20:11PM -0700, Doug Berger wrote:
> Recent Broadcom SoCs allow for the trapping of write accesses to
> read-only registers. This is only useful if such accesses are
> exceptional, so it is desirable to prevent such accesses in normal
> operation. To that end, this set of commits proposes adding two
> flags to the libata core.
>
> The first allows for a quirk that exists in some Broadcom devices
> that are capable of supporting Aggresive Link Power Management even
> though it is not reported by their read-only capability register.
> This removes a need for the Broadcom driver to write to it's
> read-only capability register.
>
> The second is a notification to the libata-core that it should not
> write to any standard registers that are defined to be read-only.
>
> The Broadcom driver is then modified to use these two general
> purpose flags.
Applied 1-3 to libata/for-4.13.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] scsi/libata: Support variable-length cdb of ata pass-thru(32).
From: Minwoo Im @ 2017-06-23 16:50 UTC (permalink / raw)
To: Bart Van Assche, jejb@linux.vnet.ibm.com, tj@kernel.org,
martin.petersen@oracle.com
Cc: linux-scsi@vger.kernel.org, linux-ide@vger.kernel.org
In-Reply-To: <1498071149.2746.26.camel@wdc.com>
On Thu, Jun 22, 2017 at 3:52 AM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> On Sat, 2017-06-17 at 20:00 +0900, Minwoo Im wrote:
>> - if ((tf->protocol = ata_scsi_map_proto(cdb[1])) == ATA_PROT_UNKNOWN) {
>> + /*
>> + * if SCSI operation code in cdb[0] is ATA_12 or ATA_16,
>> + * then cdb[1] will contain protocol of ATA PASS-THROUGH.
>> + * otherwise, Its operation code shall be ATA_32(7Fh).
>> + * in this case, cdb[10] will contain protocol of it.
>> + * we call this command as a variable-length cdb.
>> + */
>> + if (cdb[0] == ATA_12 || cdb[0] == ATA_16)
>> + tf->protocol = ata_scsi_map_proto(cdb[1]);
>> + else
>> + tf->protocol = ata_scsi_map_proto(cdb[10]);
>> +
>> + if (tf->protocol == ATA_PROT_UNKNOWN) {
>> fp = 1;
>> goto invalid_fld;
>> }
>
> Hello Minwoo,
>
> Please consider introducing a variable (e.g. called "cdb_offset") in which the
> value 9 is stored for 32-byte CDBs and 0 for 12-byte and 16-byte CDBs. That
> will allow to rewrite the above code as follows:
>
> tf->protocol = ata_scsi_map_proto(cdb[1 + cdb_offset])
>
> I think that will allow to remove most "if (cdb[0] == ATA_12 || cdb[0] == ATA_16)"
> statements introduced by this patch.
Hello Bart,
I really appreciate that you gave me a simple way to handle 12, 16
and 32 bytes commands in a function above.
I have applied those things to my patch.
>
>> + tf->auxiliary = (cdb[28] << 24) | (cdb[29] << 16)
>> + | (cdb[30] << 8) | cdb[31];
>
> Please use get_unaligned_be32() instead of open-coding it.
>
>> + const u16 sa = (cdb[8] >> 8) | cdb[9]; /* service action */
>
> Please use get_unaligned_be16() instead of open-coding it.
I have applied "get_unaligned_be16 and 32" function to those parts.
Please refer a new patch below.
Thanks, again.
>
>> @@ -4385,7 +4463,12 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
>> shost->max_id = 16;
>> shost->max_lun = 1;
>> shost->max_channel = 1;
>> - shost->max_cmd_len = 16;
>> + /*
>> + * SPC-3, SPC-4: Definition of CDB
>> + * A CDB may have a fixed length of up to 16 bytes or
>> + * variable length of between 12 and 260 bytes.
>> + */
>> + shost->max_cmd_len = 260;
>
> Does ATA pass-through really support 260-byte CDBs or is the maximum CDB length
> that is supported by ATA_32 perhaps 32 bytes?
Here's my opinion about your question.
In perspective of SCSI host, I guess the max cmd len should be 260 bytes.
Because SPC says that, in case of variable-length command, it could be
from 12 to 260 bytes.
That's why I have applied 260 value to max_cmd_len of scsi host.
Please feel free to give any opinions about it.
>
> Thanks,
>
> Bart.
Thanks,
Minwoo.
Signed-off-by: Minwoo Im <dn3108@gmail.com>
---
drivers/ata/libata-core.c | 2 +-
drivers/ata/libata-scsi.c | 76 ++++++++++++++++++++++++++++++
++++++++++++---
include/scsi/scsi_proto.h | 1 +
3 files changed, 73 insertions(+), 6 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 2d83b8c..4777e76 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2587,7 +2587,7 @@ int ata_dev_configure(struct ata_device *dev)
}
ata_dev_config_sense_reporting(dev);
ata_dev_config_zac(dev);
- dev->cdb_len = 16;
+ dev->cdb_len = 32;
}
/* ATAPI-specific feature tests */
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 49ba983..d99c4e8 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3127,7 +3127,7 @@ static struct ata_device
*__ata_scsi_find_dev(struct ata_port *ap,
* ata_scsi_pass_thru - convert ATA pass-thru CDB to taskfile
* @qc: command structure to be initialized
*
- * Handles either 12 or 16-byte versions of the CDB.
+ * Handles either 12 16, or 32-byte versions of the CDB.
*
* RETURNS:
* Zero on success, non-zero on failure.
@@ -3139,13 +3139,19 @@ static unsigned int ata_scsi_pass_thru(struct
ata_queued_cmd *qc)
struct ata_device *dev = qc->dev;
const u8 *cdb = scmd->cmnd;
u16 fp;
+ u16 cdb_offset = 0;
- if ((tf->protocol = ata_scsi_map_proto(cdb[1])) == ATA_PROT_UNKNOWN) {
+ /* 7Fh variable length cmd means a ata pass-thru(32) */
+ if (cdb[0] == VARIABLE_LENGTH_CMD)
+ cdb_offset = 9;
+
+ if ((tf->protocol = ata_scsi_map_proto(cdb[1 + cdb_offset]))
+ == ATA_PROT_UNKNOWN) {
fp = 1;
goto invalid_fld;
}
- if (ata_is_ncq(tf->protocol) && (cdb[2] & 0x3) == 0)
+ if (ata_is_ncq(tf->protocol) && (cdb[2 + cdb_offset] & 0x3) == 0)
tf->protocol = ATA_PROT_NCQ_NODATA;
/* enable LBA */
@@ -3181,7 +3187,7 @@ static unsigned int ata_scsi_pass_thru(struct
ata_queued_cmd *qc)
tf->lbah = cdb[12];
tf->device = cdb[13];
tf->command = cdb[14];
- } else {
+ } else if (cdb[0] == ATA_12) {
/*
* 12-byte CDB - incapable of extended commands.
*/
@@ -3194,6 +3200,30 @@ static unsigned int ata_scsi_pass_thru(struct
ata_queued_cmd *qc)
tf->lbah = cdb[7];
tf->device = cdb[8];
tf->command = cdb[9];
+ } else {
+ /*
+ * 32-byte CDB - may contain extended command fields.
+ *
+ * If that is the case, copy the upper byte register values.
+ */
+ if (cdb[10] & 0x01) {
+ tf->hob_feature = cdb[20];
+ tf->hob_nsect = cdb[22];
+ tf->hob_lbal = cdb[16];
+ tf->hob_lbam = cdb[15];
+ tf->hob_lbah = cdb[14];
+ tf->flags |= ATA_TFLAG_LBA48;
+ } else
+ tf->flags &= ~ATA_TFLAG_LBA48;
+
+ tf->feature = cdb[21];
+ tf->nsect = cdb[23];
+ tf->lbal = cdb[19];
+ tf->lbam = cdb[18];
+ tf->lbah = cdb[17];
+ tf->device = cdb[24];
+ tf->command = cdb[25];
+ tf->auxiliary = get_unaligned_be32(&cdb[28]);
}
/* For NCQ commands copy the tag value */
@@ -4068,6 +4098,35 @@ static unsigned int
ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
}
/**
+ * ata_scsi_var_len_cdb_xlat - SATL variable length CDB to Handler
+ * @qc: Command to be translated
+ *
+ * Translate a SCSI variable length CDB to specified commands.
+ * It checks a service action value in CDB to call corresponding handler.
+ *
+ * RETURNS:
+ * Zero on success, non-zero on failure
+ *
+ */
+static unsigned int ata_scsi_var_len_cdb_xlat(struct ata_queued_cmd *qc)
+{
+ struct scsi_cmnd *scmd = qc->scsicmd;
+ const u8 *cdb = scmd->cmnd;
+ const u16 sa = get_unaligned_be16(&cdb[8]);
+
+ /*
+ * if service action represents a ata pass-thru(32) command,
+ * then pass it to ata_scsi_pass_thru handler.
+ */
+ if (sa == ATA_32)
+ return ata_scsi_pass_thru(qc);
+
+unspprt_sa:
+ /* unsupported service action */
+ return 1;
+}
+
+/**
* ata_get_xlat_func - check if SCSI to ATA translation is possible
* @dev: ATA device
* @cmd: SCSI command opcode to consider
@@ -4107,6 +4166,9 @@ 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 VARIABLE_LENGTH_CMD:
+ return ata_scsi_var_len_cdb_xlat;
+
case MODE_SELECT:
case MODE_SELECT_10:
return ata_scsi_mode_select_xlat;
@@ -4385,7 +4447,11 @@ int ata_scsi_add_hosts(struct ata_host *host,
struct scsi_host_template *sht)
shost->max_id = 16;
shost->max_lun = 1;
shost->max_channel = 1;
- shost->max_cmd_len = 16;
+ /*
+ * SPC says that CDB may have a fixed length of up to 16 bytes
+ * or variable length of between 12 and 260 bytes.
+ */
+ shost->max_cmd_len = 260;
/* Schedule policy is determined by ->qc_defer()
* callback and it needs to see every deferred qc.
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index ce78ec8..8545e34 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -164,6 +164,7 @@
#define WRITE_SAME_32 0x0d
/* Values for T10/04-262r7 */
+#define ATA_32 0x1ff0 /* 32-byte pass-thru,
service action */
#define ATA_16 0x85 /* 16-byte pass-thru */
#define ATA_12 0xa1 /* 12-byte pass-thru */
^ permalink raw reply related
* Re: [PATCH] scsi/libata: Support variable-length cdb of ata pass-thru(32).
From: Bart Van Assche @ 2017-06-23 17:02 UTC (permalink / raw)
To: jejb@linux.vnet.ibm.com, tj@kernel.org, dn3108@gmail.com,
martin.petersen@oracle.com
Cc: linux-scsi@vger.kernel.org, linux-ide@vger.kernel.org
In-Reply-To: <CAKv8XUU-k0dFjHAu9G2rM+is4AZw9BKfsFDh1zG0zwq=GzSVkg@mail.gmail.com>
On Sat, 2017-06-24 at 01:50 +0900, Minwoo Im wrote:
> On Thu, Jun 22, 2017 at 3:52 AM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> > > @@ -4385,7 +4463,12 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
> > > shost->max_id = 16;
> > > shost->max_lun = 1;
> > > shost->max_channel = 1;
> > > - shost->max_cmd_len = 16;
> > > + /*
> > > + * SPC-3, SPC-4: Definition of CDB
> > > + * A CDB may have a fixed length of up to 16 bytes or
> > > + * variable length of between 12 and 260 bytes.
> > > + */
> > > + shost->max_cmd_len = 260;
> >
> > Does ATA pass-through really support 260-byte CDBs or is the maximum CDB length
> > that is supported by ATA_32 perhaps 32 bytes?
>
> Here's my opinion about your question.
> In perspective of SCSI host, I guess the max cmd len should be 260 bytes.
> Because SPC says that, in case of variable-length command, it could be
> from 12 to 260 bytes.
> That's why I have applied 260 value to max_cmd_len of scsi host.
> Please feel free to give any opinions about it.
Hello Minwoo,
Table 172 in document sat4r06.pdf shows that the CDB of the ATA PASS-THROUGH (32)
command is exactly 32 bytes long. My conclusion from analyzing __ata_scsi_queuecmd()
is that the current version of that function does not support CDBs of more than
16 bytes. Since your patch adds support for a single command with 32-byte CDB I
am convinced that max_cmd_len should be set to 32 in ata_scsi_add_hosts().
Bart.
^ permalink raw reply
* Re: [PATCH] scsi/libata: Support variable-length cdb of ata pass-thru(32).
From: Bart Van Assche @ 2017-06-23 17:15 UTC (permalink / raw)
To: jejb@linux.vnet.ibm.com, tj@kernel.org, dn3108@gmail.com,
martin.petersen@oracle.com
Cc: linux-scsi@vger.kernel.org, linux-ide@vger.kernel.org
In-Reply-To: <CAKv8XUU-k0dFjHAu9G2rM+is4AZw9BKfsFDh1zG0zwq=GzSVkg@mail.gmail.com>
On Sat, 2017-06-24 at 01:50 +0900, Minwoo Im wrote:
> - * Handles either 12 or 16-byte versions of the CDB.
> + * Handles either 12 16, or 32-byte versions of the CDB.
Please insert a comma between "12" and "16" to avoid that this comment
gets confusing.
> + if ((tf->protocol = ata_scsi_map_proto(cdb[1 + cdb_offset]))
> + == ATA_PROT_UNKNOWN) {
Have you verified this patch with checkpatch? The recommended style is
*not* to use assignments in the expression that controls an if-statement
and also if a comparison expression has to be split to keep the comparison
operator at the end of the first line.
> - shost->max_cmd_len = 16;
> + /*
> + * SPC says that CDB may have a fixed length of up to 16 bytes
> + * or variable length of between 12 and 260 bytes.
> + */
> + shost->max_cmd_len = 260;
As mentioned before, I think a maximum CDB length of 260 is overkill.
> /* Values for T10/04-262r7 */
> +#define ATA_32 0x1ff0 /* 32-byte pass-thru,
> service action */
> #define ATA_16 0x85 /* 16-byte pass-thru */
> #define ATA_12 0xa1 /* 12-byte pass-thru */
Defining ATA_32 just above ATA_12 and ATA_16 is misleading because
the latter two are CDB opcodes while the former is a service action
code. Please move the definition of the ATA_32 service action code
one line up such that it appears at the end of the list of already
defined service codes, namely this list:
/* values for variable length command */
#define XDREAD_32 0x03
#define XDWRITE_32 0x04
#define XPWRITE_32 0x06
#define XDWRITEREAD_32 0x07
#define READ_32 0x09
#define VERIFY_32 0x0a
#define WRITE_32 0x0b
#define WRITE_SAME_32 0x0d
Thanks,
Bart.
^ permalink raw reply
* Re: [PATCH] scsi/libata: Support variable-length cdb of ata pass-thru(32).
From: Minwoo Im @ 2017-06-23 17:59 UTC (permalink / raw)
To: Bart Van Assche
Cc: jejb@linux.vnet.ibm.com, tj@kernel.org,
martin.petersen@oracle.com, linux-scsi@vger.kernel.org,
linux-ide@vger.kernel.org
In-Reply-To: <1498238136.2735.8.camel@wdc.com>
On Sat, Jun 24, 2017 at 2:15 AM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> On Sat, 2017-06-24 at 01:50 +0900, Minwoo Im wrote:
>> - * Handles either 12 or 16-byte versions of the CDB.
>> + * Handles either 12 16, or 32-byte versions of the CDB.
>
> Please insert a comma between "12" and "16" to avoid that this comment
> gets confusing.
I'm sorry for making a confusion for a mistyping. It's applied to patch.
>
>> + if ((tf->protocol = ata_scsi_map_proto(cdb[1 + cdb_offset]))
>> + == ATA_PROT_UNKNOWN) {
>
> Have you verified this patch with checkpatch? The recommended style is
> *not* to use assignments in the expression that controls an if-statement
> and also if a comparison expression has to be split to keep the comparison
> operator at the end of the first line.
I was trying to keep it in a way it used to be.
"Assignment expression separated and comparison expression in a line" applied.
>
>> - shost->max_cmd_len = 16;
>> + /*
>> + * SPC says that CDB may have a fixed length of up to 16 bytes
>> + * or variable length of between 12 and 260 bytes.
>> + */
>> + shost->max_cmd_len = 260;
>
> As mentioned before, I think a maximum CDB length of 260 is overkill.
I used to think of it only in perspective of SPC specification.
As you said, 32 would be enough for this patch which is only for ata
pass-thru(32).
>
>> /* Values for T10/04-262r7 */
>> +#define ATA_32 0x1ff0 /* 32-byte pass-thru,
>> service action */
>> #define ATA_16 0x85 /* 16-byte pass-thru */
>> #define ATA_12 0xa1 /* 12-byte pass-thru */
>
> Defining ATA_32 just above ATA_12 and ATA_16 is misleading because
> the latter two are CDB opcodes while the former is a service action
> code. Please move the definition of the ATA_32 service action code
> one line up such that it appears at the end of the list of already
> defined service codes, namely this list:
I completely agree with you. ATA_32 was moved to variable length command list.
I really appreciate those reviews for this patch.
Thanks,
Minwoo.
Signed-off-by: Minwoo Im <dn3108@gmail.com>
---
drivers/ata/libata-core.c | 2 +-
drivers/ata/libata-scsi.c | 72 ++++++++++++++++++++++++++++++
+++++++++++----
include/scsi/scsi_proto.h | 1 +
3 files changed, 69 insertions(+), 6 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 2d83b8c..4777e76 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2587,7 +2587,7 @@ int ata_dev_configure(struct ata_device *dev)
}
ata_dev_config_sense_reporting(dev);
ata_dev_config_zac(dev);
- dev->cdb_len = 16;
+ dev->cdb_len = 32;
}
/* ATAPI-specific feature tests */
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 49ba983..dc59860 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3127,7 +3127,7 @@ static struct ata_device
*__ata_scsi_find_dev(struct ata_port *ap,
* ata_scsi_pass_thru - convert ATA pass-thru CDB to taskfile
* @qc: command structure to be initialized
*
- * Handles either 12 or 16-byte versions of the CDB.
+ * Handles either 12, 16, or 32-byte versions of the CDB.
*
* RETURNS:
* Zero on success, non-zero on failure.
@@ -3139,13 +3139,19 @@ static unsigned int ata_scsi_pass_thru(struct
ata_queued_cmd *qc)
struct ata_device *dev = qc->dev;
const u8 *cdb = scmd->cmnd;
u16 fp;
+ u16 cdb_offset = 0;
- if ((tf->protocol = ata_scsi_map_proto(cdb[1])) == ATA_PROT_UNKNOWN) {
+ /* 7Fh variable length cmd means a ata pass-thru(32) */
+ if (cdb[0] == VARIABLE_LENGTH_CMD)
+ cdb_offset = 9;
+
+ tf->protocol = ata_scsi_map_proto(cdb[1 + cdb_offset]);
+ if (tf->protocol == ATA_PROT_UNKNOWN) {
fp = 1;
goto invalid_fld;
}
- if (ata_is_ncq(tf->protocol) && (cdb[2] & 0x3) == 0)
+ if (ata_is_ncq(tf->protocol) && (cdb[2 + cdb_offset] & 0x3) == 0)
tf->protocol = ATA_PROT_NCQ_NODATA;
/* enable LBA */
@@ -3181,7 +3187,7 @@ static unsigned int ata_scsi_pass_thru(struct
ata_queued_cmd *qc)
tf->lbah = cdb[12];
tf->device = cdb[13];
tf->command = cdb[14];
- } else {
+ } else if (cdb[0] == ATA_12) {
/*
* 12-byte CDB - incapable of extended commands.
*/
@@ -3194,6 +3200,30 @@ static unsigned int ata_scsi_pass_thru(struct
ata_queued_cmd *qc)
tf->lbah = cdb[7];
tf->device = cdb[8];
tf->command = cdb[9];
+ } else {
+ /*
+ * 32-byte CDB - may contain extended command fields.
+ *
+ * If that is the case, copy the upper byte register values.
+ */
+ if (cdb[10] & 0x01) {
+ tf->hob_feature = cdb[20];
+ tf->hob_nsect = cdb[22];
+ tf->hob_lbal = cdb[16];
+ tf->hob_lbam = cdb[15];
+ tf->hob_lbah = cdb[14];
+ tf->flags |= ATA_TFLAG_LBA48;
+ } else
+ tf->flags &= ~ATA_TFLAG_LBA48;
+
+ tf->feature = cdb[21];
+ tf->nsect = cdb[23];
+ tf->lbal = cdb[19];
+ tf->lbam = cdb[18];
+ tf->lbah = cdb[17];
+ tf->device = cdb[24];
+ tf->command = cdb[25];
+ tf->auxiliary = get_unaligned_be32(&cdb[28]);
}
/* For NCQ commands copy the tag value */
@@ -4068,6 +4098,35 @@ static unsigned int
ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
}
/**
+ * ata_scsi_var_len_cdb_xlat - SATL variable length CDB to Handler
+ * @qc: Command to be translated
+ *
+ * Translate a SCSI variable length CDB to specified commands.
+ * It checks a service action value in CDB to call corresponding handler.
+ *
+ * RETURNS:
+ * Zero on success, non-zero on failure
+ *
+ */
+static unsigned int ata_scsi_var_len_cdb_xlat(struct ata_queued_cmd *qc)
+{
+ struct scsi_cmnd *scmd = qc->scsicmd;
+ const u8 *cdb = scmd->cmnd;
+ const u16 sa = get_unaligned_be16(&cdb[8]);
+
+ /*
+ * if service action represents a ata pass-thru(32) command,
+ * then pass it to ata_scsi_pass_thru handler.
+ */
+ if (sa == ATA_32)
+ return ata_scsi_pass_thru(qc);
+
+unspprt_sa:
+ /* unsupported service action */
+ return 1;
+}
+
+/**
* ata_get_xlat_func - check if SCSI to ATA translation is possible
* @dev: ATA device
* @cmd: SCSI command opcode to consider
@@ -4107,6 +4166,9 @@ 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 VARIABLE_LENGTH_CMD:
+ return ata_scsi_var_len_cdb_xlat;
+
case MODE_SELECT:
case MODE_SELECT_10:
return ata_scsi_mode_select_xlat;
@@ -4385,7 +4447,7 @@ int ata_scsi_add_hosts(struct ata_host *host,
struct scsi_host_template *sht)
shost->max_id = 16;
shost->max_lun = 1;
shost->max_channel = 1;
- shost->max_cmd_len = 16;
+ shost->max_cmd_len = 32;
/* Schedule policy is determined by ->qc_defer()
* callback and it needs to see every deferred qc.
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index ce78ec8..06076b8 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -162,6 +162,7 @@
#define VERIFY_32 0x0a
#define WRITE_32 0x0b
#define WRITE_SAME_32 0x0d
+#define ATA_32 0x1ff0
/* Values for T10/04-262r7 */
#define ATA_16 0x85 /* 16-byte pass-thru */
^ permalink raw reply related
* Re: [PATCH] scsi/libata: Support variable-length cdb of ata pass-thru(32).
From: Bart Van Assche @ 2017-06-23 18:13 UTC (permalink / raw)
To: dn3108@gmail.com
Cc: jejb@linux.vnet.ibm.com, tj@kernel.org,
linux-scsi@vger.kernel.org, martin.petersen@oracle.com,
linux-ide@vger.kernel.org
In-Reply-To: <CAKv8XUWxJUxaWzJmJeSuOj7i7tv4_3w=iYmnNyxxGYXwnMwy4g@mail.gmail.com>
On Sat, 2017-06-24 at 02:59 +0900, Minwoo Im wrote:
> I completely agree with you. ATA_32 was moved to variable length command list.
Thanks for having addressed all my comments. Since as far as I'm familiar
with SCSI / ATA translation this patch looks fine to me, feel free to add
Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>
Please note the following:
- You will have to repost this patch with a proper description and as a
separate e-mail. See also
https://www.kernel.org/doc/Documentation/process/5.Posting.rst
- The maintainer for the libata subsystem is Tejun Heo so you will need his
approval before this patch can be merged.
Bart.
^ permalink raw reply
* [PATCH] libata: Support for an ATA PASS-THROUGH(32) command.
From: Minwoo Im @ 2017-06-23 18:41 UTC (permalink / raw)
To: Tejun Heo, James E.J. Bottomley, Martin K. Petersen
Cc: linux-ide, linux-scsi, Minwoo Im
SAT-4(SCSI/ATA Translation) supports for an ata pass-thru(32).
This patch will allow to translate an ata pass-thru(32) SCSI cmd
to an ATA cmd.
Signed-off-by: Minwoo Im <dn3108@gmail.com>
Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>
---
drivers/ata/libata-core.c | 2 +-
drivers/ata/libata-scsi.c | 72 +++++++++++++++++++++++++++++++++++++++++----
include/scsi/scsi_proto.h | 1 +
3 files changed, 69 insertions(+), 6 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 2d83b8c..4777e76 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2587,7 +2587,7 @@ int ata_dev_configure(struct ata_device *dev)
}
ata_dev_config_sense_reporting(dev);
ata_dev_config_zac(dev);
- dev->cdb_len = 16;
+ dev->cdb_len = 32;
}
/* ATAPI-specific feature tests */
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 49ba983..dc59860 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3127,7 +3127,7 @@ static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
* ata_scsi_pass_thru - convert ATA pass-thru CDB to taskfile
* @qc: command structure to be initialized
*
- * Handles either 12 or 16-byte versions of the CDB.
+ * Handles either 12, 16, or 32-byte versions of the CDB.
*
* RETURNS:
* Zero on success, non-zero on failure.
@@ -3139,13 +3139,19 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
struct ata_device *dev = qc->dev;
const u8 *cdb = scmd->cmnd;
u16 fp;
+ u16 cdb_offset = 0;
- if ((tf->protocol = ata_scsi_map_proto(cdb[1])) == ATA_PROT_UNKNOWN) {
+ /* 7Fh variable length cmd means a ata pass-thru(32) */
+ if (cdb[0] == VARIABLE_LENGTH_CMD)
+ cdb_offset = 9;
+
+ tf->protocol = ata_scsi_map_proto(cdb[1 + cdb_offset]);
+ if (tf->protocol == ATA_PROT_UNKNOWN) {
fp = 1;
goto invalid_fld;
}
- if (ata_is_ncq(tf->protocol) && (cdb[2] & 0x3) == 0)
+ if (ata_is_ncq(tf->protocol) && (cdb[2 + cdb_offset] & 0x3) == 0)
tf->protocol = ATA_PROT_NCQ_NODATA;
/* enable LBA */
@@ -3181,7 +3187,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
tf->lbah = cdb[12];
tf->device = cdb[13];
tf->command = cdb[14];
- } else {
+ } else if (cdb[0] == ATA_12) {
/*
* 12-byte CDB - incapable of extended commands.
*/
@@ -3194,6 +3200,30 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
tf->lbah = cdb[7];
tf->device = cdb[8];
tf->command = cdb[9];
+ } else {
+ /*
+ * 32-byte CDB - may contain extended command fields.
+ *
+ * If that is the case, copy the upper byte register values.
+ */
+ if (cdb[10] & 0x01) {
+ tf->hob_feature = cdb[20];
+ tf->hob_nsect = cdb[22];
+ tf->hob_lbal = cdb[16];
+ tf->hob_lbam = cdb[15];
+ tf->hob_lbah = cdb[14];
+ tf->flags |= ATA_TFLAG_LBA48;
+ } else
+ tf->flags &= ~ATA_TFLAG_LBA48;
+
+ tf->feature = cdb[21];
+ tf->nsect = cdb[23];
+ tf->lbal = cdb[19];
+ tf->lbam = cdb[18];
+ tf->lbah = cdb[17];
+ tf->device = cdb[24];
+ tf->command = cdb[25];
+ tf->auxiliary = get_unaligned_be32(&cdb[28]);
}
/* For NCQ commands copy the tag value */
@@ -4068,6 +4098,35 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
}
/**
+ * ata_scsi_var_len_cdb_xlat - SATL variable length CDB to Handler
+ * @qc: Command to be translated
+ *
+ * Translate a SCSI variable length CDB to specified commands.
+ * It checks a service action value in CDB to call corresponding handler.
+ *
+ * RETURNS:
+ * Zero on success, non-zero on failure
+ *
+ */
+static unsigned int ata_scsi_var_len_cdb_xlat(struct ata_queued_cmd *qc)
+{
+ struct scsi_cmnd *scmd = qc->scsicmd;
+ const u8 *cdb = scmd->cmnd;
+ const u16 sa = get_unaligned_be16(&cdb[8]);
+
+ /*
+ * if service action represents a ata pass-thru(32) command,
+ * then pass it to ata_scsi_pass_thru handler.
+ */
+ if (sa == ATA_32)
+ return ata_scsi_pass_thru(qc);
+
+unspprt_sa:
+ /* unsupported service action */
+ return 1;
+}
+
+/**
* ata_get_xlat_func - check if SCSI to ATA translation is possible
* @dev: ATA device
* @cmd: SCSI command opcode to consider
@@ -4107,6 +4166,9 @@ 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 VARIABLE_LENGTH_CMD:
+ return ata_scsi_var_len_cdb_xlat;
+
case MODE_SELECT:
case MODE_SELECT_10:
return ata_scsi_mode_select_xlat;
@@ -4385,7 +4447,7 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
shost->max_id = 16;
shost->max_lun = 1;
shost->max_channel = 1;
- shost->max_cmd_len = 16;
+ shost->max_cmd_len = 32;
/* Schedule policy is determined by ->qc_defer()
* callback and it needs to see every deferred qc.
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index ce78ec8..06076b8 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -162,6 +162,7 @@
#define VERIFY_32 0x0a
#define WRITE_32 0x0b
#define WRITE_SAME_32 0x0d
+#define ATA_32 0x1ff0
/* Values for T10/04-262r7 */
#define ATA_16 0x85 /* 16-byte pass-thru */
--
1.7.9.5
^ 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