LibATA support for long sectors

LibATA support for long sectors

[Matthew Wilcox]

ATA-8 defines two different methods for increasing the length of
sectors.  Long Physical Sectors increase the native sector size
of the disc while pretending to the OS that sectors are still 512 bytes.
Long Logical Sectors increase the length of the reported sector.  This
patch series adds support for both.  It does not add support for
non-power-of-2 sector sizes as they have not been fully specified yet.

[PATCH] ata: Define new commands from ATA8

[Matthew Wilcox]

From: Matthew Wilcox <willy@linux.intel.com>

Later patches require knowledge of some commands which aren't currently
defined.  While I'm at it, add all the commands that are in ATA8 and
reorder the existing commands to be in the same order as ATA8.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
---
 include/linux/ata.h |   97 ++++++++++++++++++++++++++++++++++----------------
 1 files changed, 66 insertions(+), 31 deletions(-)

diff --git a/include/linux/ata.h b/include/linux/ata.h
index 1c622e2..3815431 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -149,58 +149,93 @@ enum {
 	ATA_REG_IRQ		= ATA_REG_NSECT,
 
 	/* ATA device commands */
-	ATA_CMD_DEV_RESET	= 0x08, /* ATAPI device reset */
+	ATA_CMD_CFA_ERASE_SECTORS = 0xC0,
+	ATA_CMD_CFA_REQUEST_EXT_ERROR = 0x03,
+	ATA_CMD_CFA_TRANSLATE_SECTOR = 0x87,
+	ATA_CMD_CFA_WRITE_MULTI_WITHOUT_ERASE = 0xCD,
+	ATA_CMD_CFA_WRITE_SECTORS_WITHOUT_ERASE = 0x38,
+	ATA_CMD_CHK_MEDIA_CARD_TYPE = 0xD1,
 	ATA_CMD_CHK_POWER	= 0xE5, /* check power mode */
-	ATA_CMD_STANDBY		= 0xE2, /* place in standby power mode */
-	ATA_CMD_IDLE		= 0xE3, /* place in idle power mode */
+	ATA_CMD_CONFIG_STREAM	= 0x51,
+	ATA_CMD_CONF_OVERLAY	= 0xB1,
+	ATA_CMD_DEV_RESET	= 0x08, /* ATAPI device reset */
+	ATA_CMD_DLOAD_MCODE	= 0x92,
 	ATA_CMD_EDD		= 0x90,	/* execute device diagnostic */
 	ATA_CMD_FLUSH		= 0xE7,
 	ATA_CMD_FLUSH_EXT	= 0xEA,
 	ATA_CMD_ID_ATA		= 0xEC,
 	ATA_CMD_ID_ATAPI	= 0xA1,
-	ATA_CMD_READ		= 0xC8,
-	ATA_CMD_READ_EXT	= 0x25,
-	ATA_CMD_WRITE		= 0xCA,
-	ATA_CMD_WRITE_EXT	= 0x35,
-	ATA_CMD_WRITE_FUA_EXT	= 0x3D,
+	ATA_CMD_IDLE		= 0xE3, /* place in idle power mode */
+	ATA_CMD_IDLEIMMEDIATE	= 0xE1,
+	ATA_CMD_NVCACHE		= 0xB6,
+	ATA_CMD_NOP		= 0x00,
+	ATA_CMD_PACKET		= 0xA0,
+	ATA_CMD_PMP_READ	= 0xE4, /* aka READ BUFFER */
+	ATA_CMD_READ		= 0xC8, /* aka READ DMA */
+	ATA_CMD_READ_EXT	= 0x25, /* aka READ DMA EXT */
+	ATA_CMD_READ_QUEUED	= 0xC7,
+	ATA_CMD_READ_QUEUED_EXT	= 0x26,
 	ATA_CMD_FPDMA_READ	= 0x60,
-	ATA_CMD_FPDMA_WRITE	= 0x61,
-	ATA_CMD_PIO_READ	= 0x20,
-	ATA_CMD_PIO_READ_EXT	= 0x24,
-	ATA_CMD_PIO_WRITE	= 0x30,
-	ATA_CMD_PIO_WRITE_EXT	= 0x34,
+	ATA_CMD_READ_LOG_EXT	= 0x2F,
+	ATA_CMD_READ_LOG_DMA_EXT= 0x47,
 	ATA_CMD_READ_MULTI	= 0xC4,
 	ATA_CMD_READ_MULTI_EXT	= 0x29,
-	ATA_CMD_WRITE_MULTI	= 0xC5,
-	ATA_CMD_WRITE_MULTI_EXT	= 0x39,
-	ATA_CMD_WRITE_MULTI_FUA_EXT = 0xCE,
-	ATA_CMD_SET_FEATURES	= 0xEF,
-	ATA_CMD_SET_MULTI	= 0xC6,
-	ATA_CMD_PACKET		= 0xA0,
-	ATA_CMD_VERIFY		= 0x40,
-	ATA_CMD_VERIFY_EXT	= 0x42,
-	ATA_CMD_STANDBYNOW1	= 0xE0,
-	ATA_CMD_IDLEIMMEDIATE	= 0xE1,
-	ATA_CMD_SLEEP		= 0xE6,
-	ATA_CMD_INIT_DEV_PARAMS	= 0x91,
 	ATA_CMD_READ_NATIVE_MAX	= 0xF8,
 	ATA_CMD_READ_NATIVE_MAX_EXT = 0x27,
+	ATA_CMD_PIO_READ	= 0x20, /* aka READ SECTOR(S) */
+	ATA_CMD_PIO_READ_EXT	= 0x24, /* aka READ SECTOR(S) EXT */
+	ATA_CMD_READ_STREAM_DMA_EXT = 0x2A,
+	ATA_CMD_READ_STREAM_EXT	= 0x2B,
+	ATA_CMD_VERIFY		= 0x40, /* aka READ VERIFY SECTOR(S) */
+	ATA_CMD_VERIFY_EXT	= 0x42, /* aka READ VERIFY SECTOR(S) EXT */
+	ATA_CMD_SEC_DISABLE_PASSWORD = 0xF6,
+	ATA_CMD_SEC_ERASE_PREPARE = 0xF3,
+	ATA_CMD_SEC_ERASE_UNIT	= 0xF4,
+	ATA_CMD_SEC_FREEZE_LOCK	= 0xF5,
+	ATA_CMD_SEC_SET_PASSWORD = 0xF1,
+	ATA_CMD_SEC_UNLOCK	= 0xF2,
+	ATA_CMD_SERVICE		= 0xA2,
+	ATA_CMD_SET_FEATURES	= 0xEF,
 	ATA_CMD_SET_MAX		= 0xF9,
 	ATA_CMD_SET_MAX_EXT	= 0x37,
-	ATA_CMD_READ_LOG_EXT	= 0x2f,
-	ATA_CMD_PMP_READ	= 0xE4,
-	ATA_CMD_PMP_WRITE	= 0xE8,
-	ATA_CMD_CONF_OVERLAY	= 0xB1,
-	ATA_CMD_SEC_FREEZE_LOCK	= 0xF5,
+	ATA_CMD_SET_MULTI	= 0xC6,
+	ATA_CMD_SLEEP		= 0xE6,
+	ATA_CMD_SMART		= 0xB0,
+	ATA_CMD_STANDBY		= 0xE2, /* place in standby power mode */
+	ATA_CMD_STANDBYNOW1	= 0xE0,
+	ATA_CMD_TRUSTED_NON_DATA = 0x5B,
+	ATA_CMD_TRUSTED_RECEIVE	= 0x5C,
+	ATA_CMD_TRUSTED_RECEIVE_DMA = 0x5D,
+	ATA_CMD_TRUSTED_SEND	= 0x5E,
+	ATA_CMD_TRUSTED_SEND_DMA = 0x5F,
+	ATA_CMD_PMP_WRITE	= 0xE8, /* aka WRITE BUFFER */
+	ATA_CMD_WRITE		= 0xCA, /* aka WRITE DMA */
+	ATA_CMD_WRITE_EXT	= 0x35, /* aka WRITE DMA EXT */
+	ATA_CMD_WRITE_FUA_EXT	= 0x3D, /* aka WRITE DMA FUA EXT */
+	ATA_CMD_WRITE_DMA_QUEUED = 0xCC,
+	ATA_CMD_WRITE_DMA_QUEUED_EXT = 0x36,
+	ATA_CMD_WRITE_DMA_QUEUED_FUA_EXT = 0x3E,
+	ATA_CMD_FPDMA_WRITE	= 0x61, /* aka WRITE FPDMA QUEUED */
+	ATA_CMD_WRITE_LOG_EXT	= 0x3F,
+	ATA_CMD_WRITE_LOG_DMA_EXT = 0x57,
+	ATA_CMD_WRITE_MULTI	= 0xC5,
+	ATA_CMD_WRITE_MULTI_EXT	= 0x39,
+	ATA_CMD_WRITE_MULTI_FUA_EXT = 0xCE,
+	ATA_CMD_PIO_WRITE	= 0x30, /* aka WRITE SECTOR(S) */
+	ATA_CMD_PIO_WRITE_EXT	= 0x34, /* aka WRITE SECTOR(S) EXT */
+	ATA_CMD_WRITE_STREAM_DMA_EXT = 0x3A,
+	ATA_CMD_WRITE_STREAM_EXT = 0x3B,
+	ATA_CMD_WRITE_UNCORRECTABLE_EXT = 0x45,
 
 	/* READ_LOG_EXT pages */
 	ATA_LOG_SATA_NCQ	= 0x10,
 
-	/* READ/WRITE LONG (obsolete) */
+	/* Obsolete */
 	ATA_CMD_READ_LONG	= 0x22,
 	ATA_CMD_READ_LONG_ONCE	= 0x23,
 	ATA_CMD_WRITE_LONG	= 0x32,
 	ATA_CMD_WRITE_LONG_ONCE	= 0x33,
+	ATA_CMD_INIT_DEV_PARAMS	= 0x91,
 
 	/* SETFEATURES stuff */
 	SETFEATURES_XFER	= 0x03,
-- 
1.5.6.3

Re: [PATCH] ata: Define new commands from ATA8

[Greg Freemyer]

Updated comment below.

On Tue, Aug 5, 2008 at 1:26 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> From: Matthew Wilcox <willy@linux.intel.com>
>
> Later patches require knowledge of some commands which aren't currently
> defined.  While I'm at it, add all the commands that are in ATA8 and
> reorder the existing commands to be in the same order as ATA8.
>
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> ---
>  include/linux/ata.h |   97 ++++++++++++++++++++++++++++++++++----------------
>  1 files changed, 66 insertions(+), 31 deletions(-)
>
> diff --git a/include/linux/ata.h b/include/linux/ata.h
> index 1c622e2..3815431 100644
> --- a/include/linux/ata.h
> +++ b/include/linux/ata.h
> @@ -149,58 +149,93 @@ enum {
>        ATA_REG_IRQ             = ATA_REG_NSECT,
>
>        /* ATA device commands */
> -       ATA_CMD_DEV_RESET       = 0x08, /* ATAPI device reset */
> +       ATA_CMD_CFA_ERASE_SECTORS = 0xC0,
> +       ATA_CMD_CFA_REQUEST_EXT_ERROR = 0x03,
> +       ATA_CMD_CFA_TRANSLATE_SECTOR = 0x87,
> +       ATA_CMD_CFA_WRITE_MULTI_WITHOUT_ERASE = 0xCD,
> +       ATA_CMD_CFA_WRITE_SECTORS_WITHOUT_ERASE = 0x38,
> +       ATA_CMD_CHK_MEDIA_CARD_TYPE = 0xD1,
>        ATA_CMD_CHK_POWER       = 0xE5, /* check power mode */
> -       ATA_CMD_STANDBY         = 0xE2, /* place in standby power mode */
> -       ATA_CMD_IDLE            = 0xE3, /* place in idle power mode */
> +       ATA_CMD_CONFIG_STREAM   = 0x51,
> +       ATA_CMD_CONF_OVERLAY    = 0xB1,
> +       ATA_CMD_DEV_RESET       = 0x08, /* ATAPI device reset */
> +       ATA_CMD_DLOAD_MCODE     = 0x92,
>        ATA_CMD_EDD             = 0x90, /* execute device diagnostic */
>        ATA_CMD_FLUSH           = 0xE7,
>        ATA_CMD_FLUSH_EXT       = 0xEA,
>        ATA_CMD_ID_ATA          = 0xEC,
>        ATA_CMD_ID_ATAPI        = 0xA1,
> -       ATA_CMD_READ            = 0xC8,
> -       ATA_CMD_READ_EXT        = 0x25,
> -       ATA_CMD_WRITE           = 0xCA,
> -       ATA_CMD_WRITE_EXT       = 0x35,
> -       ATA_CMD_WRITE_FUA_EXT   = 0x3D,
> +       ATA_CMD_IDLE            = 0xE3, /* place in idle power mode */
> +       ATA_CMD_IDLEIMMEDIATE   = 0xE1,
> +       ATA_CMD_NVCACHE         = 0xB6,
> +       ATA_CMD_NOP             = 0x00,
> +       ATA_CMD_PACKET          = 0xA0,
> +       ATA_CMD_PMP_READ        = 0xE4, /* aka READ BUFFER */
> +       ATA_CMD_READ            = 0xC8, /* aka READ DMA */
> +       ATA_CMD_READ_EXT        = 0x25, /* aka READ DMA EXT */
> +       ATA_CMD_READ_QUEUED     = 0xC7,
> +       ATA_CMD_READ_QUEUED_EXT = 0x26,
>        ATA_CMD_FPDMA_READ      = 0x60,
> -       ATA_CMD_FPDMA_WRITE     = 0x61,
> -       ATA_CMD_PIO_READ        = 0x20,
> -       ATA_CMD_PIO_READ_EXT    = 0x24,
> -       ATA_CMD_PIO_WRITE       = 0x30,
> -       ATA_CMD_PIO_WRITE_EXT   = 0x34,
> +       ATA_CMD_READ_LOG_EXT    = 0x2F,
> +       ATA_CMD_READ_LOG_DMA_EXT= 0x47,
>        ATA_CMD_READ_MULTI      = 0xC4,
>        ATA_CMD_READ_MULTI_EXT  = 0x29,
> -       ATA_CMD_WRITE_MULTI     = 0xC5,
> -       ATA_CMD_WRITE_MULTI_EXT = 0x39,
> -       ATA_CMD_WRITE_MULTI_FUA_EXT = 0xCE,
> -       ATA_CMD_SET_FEATURES    = 0xEF,
> -       ATA_CMD_SET_MULTI       = 0xC6,
> -       ATA_CMD_PACKET          = 0xA0,
> -       ATA_CMD_VERIFY          = 0x40,
> -       ATA_CMD_VERIFY_EXT      = 0x42,
> -       ATA_CMD_STANDBYNOW1     = 0xE0,
> -       ATA_CMD_IDLEIMMEDIATE   = 0xE1,
> -       ATA_CMD_SLEEP           = 0xE6,
> -       ATA_CMD_INIT_DEV_PARAMS = 0x91,
>        ATA_CMD_READ_NATIVE_MAX = 0xF8,
>        ATA_CMD_READ_NATIVE_MAX_EXT = 0x27,
> +       ATA_CMD_PIO_READ        = 0x20, /* aka READ SECTOR(S) */
> +       ATA_CMD_PIO_READ_EXT    = 0x24, /* aka READ SECTOR(S) EXT */
> +       ATA_CMD_READ_STREAM_DMA_EXT = 0x2A,
> +       ATA_CMD_READ_STREAM_EXT = 0x2B,
> +       ATA_CMD_VERIFY          = 0x40, /* aka READ VERIFY SECTOR(S) */
> +       ATA_CMD_VERIFY_EXT      = 0x42, /* aka READ VERIFY SECTOR(S) EXT */
> +       ATA_CMD_SEC_DISABLE_PASSWORD = 0xF6,
> +       ATA_CMD_SEC_ERASE_PREPARE = 0xF3,
> +       ATA_CMD_SEC_ERASE_UNIT  = 0xF4,
> +       ATA_CMD_SEC_FREEZE_LOCK = 0xF5,
> +       ATA_CMD_SEC_SET_PASSWORD = 0xF1,
> +       ATA_CMD_SEC_UNLOCK      = 0xF2,
> +       ATA_CMD_SERVICE         = 0xA2,
> +       ATA_CMD_SET_FEATURES    = 0xEF,
>        ATA_CMD_SET_MAX         = 0xF9,
>        ATA_CMD_SET_MAX_EXT     = 0x37,
> -       ATA_CMD_READ_LOG_EXT    = 0x2f,
> -       ATA_CMD_PMP_READ        = 0xE4,
> -       ATA_CMD_PMP_WRITE       = 0xE8,
> -       ATA_CMD_CONF_OVERLAY    = 0xB1,
> -       ATA_CMD_SEC_FREEZE_LOCK = 0xF5,
> +       ATA_CMD_SET_MULTI       = 0xC6,
> +       ATA_CMD_SLEEP           = 0xE6,
> +       ATA_CMD_SMART           = 0xB0,
> +       ATA_CMD_STANDBY         = 0xE2, /* place in standby power mode */
> +       ATA_CMD_STANDBYNOW1     = 0xE0,
> +       ATA_CMD_TRUSTED_NON_DATA = 0x5B,
> +       ATA_CMD_TRUSTED_RECEIVE = 0x5C,
> +       ATA_CMD_TRUSTED_RECEIVE_DMA = 0x5D,
> +       ATA_CMD_TRUSTED_SEND    = 0x5E,
> +       ATA_CMD_TRUSTED_SEND_DMA = 0x5F,
> +       ATA_CMD_PMP_WRITE       = 0xE8, /* aka WRITE BUFFER */
> +       ATA_CMD_WRITE           = 0xCA, /* aka WRITE DMA */
> +       ATA_CMD_WRITE_EXT       = 0x35, /* aka WRITE DMA EXT */
> +       ATA_CMD_WRITE_FUA_EXT   = 0x3D, /* aka WRITE DMA FUA EXT */
> +       ATA_CMD_WRITE_DMA_QUEUED = 0xCC,
> +       ATA_CMD_WRITE_DMA_QUEUED_EXT = 0x36,
> +       ATA_CMD_WRITE_DMA_QUEUED_FUA_EXT = 0x3E,
> +       ATA_CMD_FPDMA_WRITE     = 0x61, /* aka WRITE FPDMA QUEUED */
> +       ATA_CMD_WRITE_LOG_EXT   = 0x3F,
> +       ATA_CMD_WRITE_LOG_DMA_EXT = 0x57,
> +       ATA_CMD_WRITE_MULTI     = 0xC5,
> +       ATA_CMD_WRITE_MULTI_EXT = 0x39,
> +       ATA_CMD_WRITE_MULTI_FUA_EXT = 0xCE,
> +       ATA_CMD_PIO_WRITE       = 0x30, /* aka WRITE SECTOR(S) */
> +       ATA_CMD_PIO_WRITE_EXT   = 0x34, /* aka WRITE SECTOR(S) EXT */
> +       ATA_CMD_WRITE_STREAM_DMA_EXT = 0x3A,
> +       ATA_CMD_WRITE_STREAM_EXT = 0x3B,
> +       ATA_CMD_WRITE_UNCORRECTABLE_EXT = 0x45,
>
>        /* READ_LOG_EXT pages */
>        ATA_LOG_SATA_NCQ        = 0x10,
>
> -       /* READ/WRITE LONG (obsolete) */
> +       /* Obsolete */

Since these diagnostic commands could be confused with the new "long
sector" commands, should the comment say:

/* Obsolete LBA28 diagnostic commands, in LBA48 replaced by
ATA_WRITE_UNC_EXT, etc. */

>        ATA_CMD_READ_LONG       = 0x22,
>        ATA_CMD_READ_LONG_ONCE  = 0x23,
>        ATA_CMD_WRITE_LONG      = 0x32,
>        ATA_CMD_WRITE_LONG_ONCE = 0x33,
> +       ATA_CMD_INIT_DEV_PARAMS = 0x91,
>
>        /* SETFEATURES stuff */
>        SETFEATURES_XFER        = 0x03,
> --
> 1.5.6.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Greg Freemyer
Litigation Triage Solutions Specialist
http://www.linkedin.com/in/gregfreemyer
First 99 Days Litigation White Paper -
http://www.norcrossgroup.com/forms/whitepapers/99%20Days%20whitepaper.pdf

The Norcross Group
The Intersection of Evidence & Technology
http://www.norcrossgroup.com

[PATCH] ata: Add support for Long Logical Sectors and Long Physical Sectors

[Matthew Wilcox]

From: Matthew Wilcox <willy@linux.intel.com>

ATA 8 adds support for devices that have sector sizes larger than 512
bytes.  We record the sector size in the ata_device and use it instead
of the ATA_SECT_SIZE when the data transfer is a multiple of sectors.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
---
 drivers/ata/libata-core.c |  127 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata-scsi.c |   52 +++++++++++++-----
 include/linux/libata.h    |    4 +-
 3 files changed, 168 insertions(+), 15 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 5ba96c5..8f38d0c 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -446,6 +446,106 @@ void ata_tf_from_fis(const u8 *fis, struct ata_taskfile *tf)
 	tf->hob_nsect	= fis[13];
 }
 
+/**
+ * ata_sect_size - Returns the sector size to use for a command
+ * @command: The ATA command byte
+ * @dev_sect_size: The size of the drive's sectors
+ *
+ * Some commands are specified to transfer (a multiple of) 512 bytes of data
+ * while others transfer a multiple of the number of bytes in a sector.  This
+ * function knows which commands transfer how much data.
+ */
+unsigned ata_sect_size(u8 command, unsigned dev_sect_size)
+{
+	switch (command) {
+	case ATA_CMD_CFA_TRANSLATE_SECTOR:
+	case ATA_CMD_CFA_WRITE_MULTI_WITHOUT_ERASE:
+	case ATA_CMD_CFA_WRITE_SECTORS_WITHOUT_ERASE:
+	case ATA_CMD_READ:
+	case ATA_CMD_READ_EXT:
+	case ATA_CMD_READ_QUEUED:
+	case ATA_CMD_READ_QUEUED_EXT:
+	case ATA_CMD_FPDMA_READ:
+	case ATA_CMD_READ_MULTI:
+	case ATA_CMD_READ_MULTI_EXT:
+	case ATA_CMD_PIO_READ:
+	case ATA_CMD_PIO_READ_EXT:
+	case ATA_CMD_READ_STREAM_DMA_EXT:
+	case ATA_CMD_READ_STREAM_EXT:
+	case ATA_CMD_VERIFY:
+	case ATA_CMD_VERIFY_EXT: /* XXX: not listed in rev 5 */
+	case ATA_CMD_WRITE:
+	case ATA_CMD_WRITE_EXT:
+	case ATA_CMD_WRITE_FUA_EXT:
+	case ATA_CMD_WRITE_DMA_QUEUED:
+	case ATA_CMD_WRITE_DMA_QUEUED_EXT:
+	case ATA_CMD_WRITE_DMA_QUEUED_FUA_EXT:
+	case ATA_CMD_FPDMA_WRITE:
+	case ATA_CMD_WRITE_MULTI:
+	case ATA_CMD_WRITE_MULTI_EXT:
+	case ATA_CMD_WRITE_MULTI_FUA_EXT:
+	case ATA_CMD_PIO_WRITE:
+	case ATA_CMD_PIO_WRITE_EXT:
+	case ATA_CMD_WRITE_STREAM_DMA_EXT:
+	case ATA_CMD_WRITE_STREAM_EXT:
+		return dev_sect_size;
+
+	default:
+		if (printk_ratelimit())
+			printk("Unknown ata cmd %d, assuming 512 byte "
+				"sector size\n", command);
+	case ATA_CMD_CFA_ERASE_SECTORS:
+	case ATA_CMD_CFA_REQUEST_EXT_ERROR:
+	case ATA_CMD_CHK_MEDIA_CARD_TYPE:
+	case ATA_CMD_CHK_POWER:
+	case ATA_CMD_CONFIG_STREAM:
+	case ATA_CMD_CONF_OVERLAY:
+	case ATA_CMD_DEV_RESET:
+	case ATA_CMD_DLOAD_MCODE:
+	case ATA_CMD_EDD:
+	case ATA_CMD_FLUSH:
+	case ATA_CMD_FLUSH_EXT:
+	case ATA_CMD_ID_ATA:
+	case ATA_CMD_ID_ATAPI:
+	case ATA_CMD_IDLE:
+	case ATA_CMD_IDLEIMMEDIATE:
+	case ATA_CMD_NVCACHE:
+	case ATA_CMD_NOP:
+	case ATA_CMD_PACKET:
+	case ATA_CMD_PMP_READ:
+	case ATA_CMD_READ_LOG_EXT:
+	case ATA_CMD_READ_LOG_DMA_EXT:
+	case ATA_CMD_READ_NATIVE_MAX:
+	case ATA_CMD_READ_NATIVE_MAX_EXT:
+	case ATA_CMD_SEC_DISABLE_PASSWORD:
+	case ATA_CMD_SEC_ERASE_PREPARE:
+	case ATA_CMD_SEC_ERASE_UNIT:
+	case ATA_CMD_SEC_FREEZE_LOCK:
+	case ATA_CMD_SEC_SET_PASSWORD:
+	case ATA_CMD_SEC_UNLOCK:
+	case ATA_CMD_SERVICE:
+	case ATA_CMD_SET_FEATURES:
+	case ATA_CMD_SET_MAX:
+	case ATA_CMD_SET_MAX_EXT:
+	case ATA_CMD_SET_MULTI:
+	case ATA_CMD_SLEEP:
+	case ATA_CMD_SMART:
+	case ATA_CMD_STANDBY:
+	case ATA_CMD_STANDBYNOW1:
+	case ATA_CMD_TRUSTED_NON_DATA:
+	case ATA_CMD_TRUSTED_RECEIVE:
+	case ATA_CMD_TRUSTED_RECEIVE_DMA:
+	case ATA_CMD_TRUSTED_SEND:
+	case ATA_CMD_TRUSTED_SEND_DMA:
+	case ATA_CMD_PMP_WRITE:
+	case ATA_CMD_WRITE_LOG_EXT:
+	case ATA_CMD_WRITE_LOG_DMA_EXT:
+	case ATA_CMD_WRITE_UNCORRECTABLE_EXT:
+	case ATA_CMD_INIT_DEV_PARAMS:
+		return ATA_SECT_SIZE;
+	}
+}
+
 static const u8 ata_rw_cmds[] = {
 	/* pio multi */
 	ATA_CMD_READ_MULTI,
@@ -1190,6 +1290,25 @@ static u64 ata_id_n_sectors(const u16 *id)
 	}
 }
 
+/*
+ * ATA supports sector sizes up to 2^33 - 1.  The reported sector size may
+ * not be a power of two.  The extra bytes are used for user-visible data
+ * integrity calculations.  Note this is not the same as the ECC which is
+ * accessed through the SCT Command Transport or READ / WRITE LONG.
+ */
+static u64 ata_id_sect_size(const u16 *id)
+{
+	u16 word_106 = id[106];
+	u64 sz;
+
+	if ((word_106 & 0xc000) != 0x4000)
+		return ATA_SECT_SIZE;
+	if (!(word_106 & (1 << 12)))
+		return ATA_SECT_SIZE;
+	sz = (id[117] | ((u64)id[118] << 16)) * 2;
+	return sz;
+}
+
 u64 ata_tf_to_lba48(const struct ata_taskfile *tf)
 {
 	u64 sectors = 0;
@@ -2196,6 +2315,7 @@ int ata_dev_configure(struct ata_device *dev)
 	dev->max_sectors = 0;
 	dev->cdb_len = 0;
 	dev->n_sectors = 0;
+	dev->sect_size = ATA_SECT_SIZE;
 	dev->cylinders = 0;
 	dev->heads = 0;
 	dev->sectors = 0;
@@ -2235,6 +2355,13 @@ int ata_dev_configure(struct ata_device *dev)
 		}
 
 		dev->n_sectors = ata_id_n_sectors(id);
+		dev->sect_size = ata_id_sect_size(id);
+		if (dev->sect_size > 0xffffffffULL) {
+			ata_dev_printk(dev, KERN_ERR, "sector size larger than "
+						"4GB not supported.\n");
+			rc = -EINVAL;
+			goto err_out_nosup;
+		}
 
 		if (dev->id[59] & 0x100)
 			dev->multi_count = dev->id[59] & 0xff;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index b9d3ba4..145ea08 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -49,7 +49,6 @@
 
 #include "libata.h"
 
-#define SECTOR_SIZE		512
 #define ATA_SCSI_RBUF_SIZE	4096
 
 static DEFINE_SPINLOCK(ata_scsi_rbuf_lock);
@@ -347,7 +346,7 @@ static int ata_get_identity(struct scsi_device *sdev, void __user *arg)
 
 /**
  *	ata_cmd_ioctl - Handler for HDIO_DRIVE_CMD ioctl
- *	@scsidev: Device to which we are issuing command
+ *	@sdev: Device to which we are issuing command
  *	@arg: User provided data for issuing command
  *
  *	LOCKING:
@@ -356,7 +355,7 @@ static int ata_get_identity(struct scsi_device *sdev, void __user *arg)
  *	RETURNS:
  *	Zero on success, negative errno on error.
  */
-int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
+int ata_cmd_ioctl(struct scsi_device *sdev, void __user *arg)
 {
 	int rc = 0;
 	u8 scsi_cmd[MAX_COMMAND_SIZE];
@@ -378,7 +377,8 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
 	memset(scsi_cmd, 0, sizeof(scsi_cmd));
 
 	if (args[3]) {
-		argsize = SECTOR_SIZE * args[3];
+		unsigned sect_size = ata_sect_size(args[0], sdev->sector_size);
+		argsize = sect_size * args[3];
 		argbuf = kmalloc(argsize, GFP_KERNEL);
 		if (argbuf == NULL) {
 			rc = -ENOMEM;
@@ -410,7 +410,7 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
 
 	/* Good values for timeout and retries?  Values below
 	   from scsi_ioctl_send_command() for default case... */
-	cmd_result = scsi_execute(scsidev, scsi_cmd, data_dir, argbuf, argsize,
+	cmd_result = scsi_execute(sdev, scsi_cmd, data_dir, argbuf, argsize,
 				  sensebuf, (10*HZ), 5, 0);
 
 	if (driver_byte(cmd_result) == DRIVER_SENSE) {/* sense data available */
@@ -1493,6 +1493,7 @@ nothing_to_do:
 static unsigned int ata_scsi_rw_xlat(struct ata_queued_cmd *qc)
 {
 	struct scsi_cmnd *scmd = qc->scsicmd;
+	struct ata_device *dev = qc->dev;
 	const u8 *cdb = scmd->cmnd;
 	unsigned int tf_flags = 0;
 	u64 block;
@@ -1549,9 +1550,9 @@ static unsigned int ata_scsi_rw_xlat(struct ata_queued_cmd *qc)
 		goto nothing_to_do;
 
 	qc->flags |= ATA_QCFLAG_IO;
-	qc->nbytes = n_block * ATA_SECT_SIZE;
+	qc->nbytes = n_block * dev->sect_size;
 
-	rc = ata_build_rw_tf(&qc->tf, qc->dev, block, n_block, tf_flags,
+	rc = ata_build_rw_tf(&qc->tf, dev, block, n_block, tf_flags,
 			     qc->tag);
 	if (likely(rc == 0))
 		return 0;
@@ -2217,10 +2218,25 @@ saving_not_supp:
  */
 static unsigned int ata_scsiop_read_cap(struct ata_scsi_args *args, u8 *rbuf)
 {
-	u64 last_lba = args->dev->n_sectors - 1; /* LBA of the last block */
+	struct ata_device *dev = args->dev;
+	u64 last_lba = dev->n_sectors - 1; /* LBA of the last block */
+	u32 sector_size;
+	u8 log_per_phys = 1;
+	u16 first_sector_offset = 0;
+	u16 word_106 = dev->id[106];
 
 	VPRINTK("ENTER\n");
 
+	if ((word_106 & 0xc000) == 0x4000) {
+		/* Number and offset of logical sectors per physical sector */
+		if (word_106 & (1 << 13))
+			log_per_phys = word_106 & 0xf;
+		if ((dev->id[209] & 0xc000) == 0x4000)
+			first_sector_offset = dev->id[209] & 0x3fff;
+	}
+
+	sector_size = dev->sect_size;
+
 	if (args->cmd->cmnd[0] == READ_CAPACITY) {
 		if (last_lba >= 0xffffffffULL)
 			last_lba = 0xffffffff;
@@ -2231,9 +2247,10 @@ static unsigned int ata_scsiop_read_cap(struct ata_scsi_args *args, u8 *rbuf)
 		rbuf[2] = last_lba >> (8 * 1);
 		rbuf[3] = last_lba;
 
-		/* sector size */
-		rbuf[6] = ATA_SECT_SIZE >> 8;
-		rbuf[7] = ATA_SECT_SIZE & 0xff;
+		rbuf[4] = sector_size >> (8 * 3);
+		rbuf[5] = sector_size >> (8 * 2);
+		rbuf[6] = sector_size >> (8 * 1);
+		rbuf[7] = sector_size;
 	} else {
 		/* sector count, 64-bit */
 		rbuf[0] = last_lba >> (8 * 7);
@@ -2246,8 +2263,15 @@ static unsigned int ata_scsiop_read_cap(struct ata_scsi_args *args, u8 *rbuf)
 		rbuf[7] = last_lba;
 
 		/* sector size */
-		rbuf[10] = ATA_SECT_SIZE >> 8;
-		rbuf[11] = ATA_SECT_SIZE & 0xff;
+		rbuf[8] = sector_size >> (8 * 3);
+		rbuf[9] = sector_size >> (8 * 2);
+		rbuf[10] = sector_size >> (8 * 1);
+		rbuf[11] = sector_size;
+
+		rbuf[12] = 0;
+		rbuf[13] = log_per_phys;
+		rbuf[14] = first_sector_offset >> 8;
+		rbuf[15] = first_sector_offset;
 	}
 
 	return 0;
@@ -2721,7 +2745,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 	}
 
 	/* READ/WRITE LONG use a non-standard sect_size */
-	qc->sect_size = ATA_SECT_SIZE;
+	qc->sect_size = ata_sect_size(tf->command, dev->sect_size);
 	switch (tf->command) {
 	case ATA_CMD_READ_LONG:
 	case ATA_CMD_READ_LONG_ONCE:
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 06b8033..fcf84d1 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -553,8 +553,8 @@ struct ata_ering {
 struct ata_device {
 	struct ata_link		*link;
 	unsigned int		devno;		/* 0 or 1 */
-	unsigned long		flags;		/* ATA_DFLAG_xxx */
 	unsigned int		horkage;	/* List of broken features */
+	unsigned long		flags;		/* ATA_DFLAG_xxx */
 	struct scsi_device	*sdev;		/* attached SCSI device */
 #ifdef CONFIG_ATA_ACPI
 	acpi_handle		acpi_handle;
@@ -562,6 +562,7 @@ struct ata_device {
 #endif
 	/* n_sector is used as CLEAR_OFFSET, read comment above CLEAR_OFFSET */
 	u64			n_sectors;	/* size of device, if ATA */
+	u64			sect_size;	/* Logical, not physical */
 	unsigned int		class;		/* ATA_DEV_xxx */
 
 	u8			pio_mode;
@@ -956,6 +957,7 @@ extern unsigned int ata_do_dev_read_id(struct ata_device *dev,
 					struct ata_taskfile *tf, u16 *id);
 extern void ata_qc_complete(struct ata_queued_cmd *qc);
 extern int ata_qc_complete_multiple(struct ata_port *ap, u32 qc_active);
+extern unsigned ata_sect_size(u8 command, unsigned dev_sect_size);
 extern void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd,
 			      void (*done)(struct scsi_cmnd *));
 extern int ata_std_bios_param(struct scsi_device *sdev,
-- 
1.5.6.3

Re: [PATCH] ata: Add support for Long Logical Sectors and Long Physical Sectors

[Alan Cox]

> + * Some commands are specified to transfer (a multiple of) 512 bytes of data
> + * while others transfer a multiple of the number of bytes in a sector.  This
> + * function knows which commands transfer how much data.

	static u32 ata_sector_or_block[]={...};

	if (test_bit(tf->cmd, &ata_sector_or_block))

looks so much more elegant than a giant switch statement and I suspect
produces far better code

> + * ATA supports sector sizes up to 2^33 - 1.  The reported sector size may
> + * not be a power of two.  The extra bytes are used for user-visible data
> + * integrity calculations.  Note this is not the same as the ECC which is
> + * accessed through the SCT Command Transport or READ / WRITE LONG.
> + */
> +static u64 ata_id_sect_size(const u16 *id)

word 106 is not defined in early ATA standards so it would be wise to
check that ATA8 is reported by the drive - and trust the relevant bits
for ATA7/8 as appropriate.

The drivers also need to know when a command is being issued which is a
funny shape as some hardware cannot do it because the internal state
machine knows the sector size and other stuff seems to need the internal
FIFO turning off or other things whacked repeatedly on the head to make
it work.

You also don't need to bother listing all the commands that don't have
data transfer phases as a sector size is meaningless there so we
shouldn't even bother doing a lookup for them. Indeed the more I look at
this the more it seems that keeping long complex ever out of date arrays
is the wrong way to do it.

We will also need a driver flag to indicate support in that HBA but that
is pretty trivial to add

Alan

Re: [PATCH] ata: Add support for Long Logical Sectors and Long Physical Sectors

[Matthew Wilcox]

On Tue, Aug 05, 2008 at 09:46:51PM +0100, Alan Cox wrote:
> > + * Some commands are specified to transfer (a multiple of) 512 bytes of data
> > + * while others transfer a multiple of the number of bytes in a sector.  This
> > + * function knows which commands transfer how much data.
> 
> 	static u32 ata_sector_or_block[]={...};
> 
> 	if (test_bit(tf->cmd, &ata_sector_or_block))
> 
> looks so much more elegant than a giant switch statement and I suspect
> produces far better code

Probably ... I did consider it, but I think I was too influenced by the
existing READ/WRITE LONG code.

> > + * ATA supports sector sizes up to 2^33 - 1.  The reported sector size may
> > + * not be a power of two.  The extra bytes are used for user-visible data
> > + * integrity calculations.  Note this is not the same as the ECC which is
> > + * accessed through the SCT Command Transport or READ / WRITE LONG.
> > + */
> > +static u64 ata_id_sect_size(const u16 *id)
> 
> word 106 is not defined in early ATA standards so it would be wise to
> check that ATA8 is reported by the drive - and trust the relevant bits
> for ATA7/8 as appropriate.

I'm not sure that's necessary.  The spec says to check whether words are
valid by doing the & 0xc000 == 0x4000 test.

> The drivers also need to know when a command is being issued which is a
> funny shape as some hardware cannot do it because the internal state
> machine knows the sector size and other stuff seems to need the internal
> FIFO turning off or other things whacked repeatedly on the head to make
> it work.

Yes, I would expect some driver work to be required.  It only gets worse
once we implement the DIX-equivalent.  How do you suggest would be a
good migration path?  We could have the driver set a flag, or call into
the driver from the midlayer to check whether it can cope with a
particular sector size.

> You also don't need to bother listing all the commands that don't have
> data transfer phases as a sector size is meaningless there so we
> shouldn't even bother doing a lookup for them. Indeed the more I look at
> this the more it seems that keeping long complex ever out of date arrays
> is the wrong way to do it.

I didn't want to change behaviour.  We currently set sect_size to 512
for all commands (except READ LONG), and I didn't want to change that
for non-data commands.  I agree with you that it's not relevant.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] ata: Add support for Long Logical Sectors and Long Physical Sectors

[Alan Cox]

On Tue, 5 Aug 2008 20:22:55 -0600
Matthew Wilcox <matthew@wil.cx> wrote:

> On Tue, Aug 05, 2008 at 09:46:51PM +0100, Alan Cox wrote:
> > > + * Some commands are specified to transfer (a multiple of) 512 bytes of data
> > > + * while others transfer a multiple of the number of bytes in a sector.  This
> > > + * function knows which commands transfer how much data.
> > 
> > 	static u32 ata_sector_or_block[]={...};
> > 
> > 	if (test_bit(tf->cmd, &ata_sector_or_block))
> > 
> > looks so much more elegant than a giant switch statement and I suspect
> > produces far better code
> 
> Probably ... I did consider it, but I think I was too influenced by the
> existing READ/WRITE LONG code.
> 
> > > + * ATA supports sector sizes up to 2^33 - 1.  The reported sector size may
> > > + * not be a power of two.  The extra bytes are used for user-visible data
> > > + * integrity calculations.  Note this is not the same as the ECC which is
> > > + * accessed through the SCT Command Transport or READ / WRITE LONG.
> > > + */
> > > +static u64 ata_id_sect_size(const u16 *id)
> > 
> > word 106 is not defined in early ATA standards so it would be wise to
> > check that ATA8 is reported by the drive - and trust the relevant bits
> > for ATA7/8 as appropriate.
> 
> I'm not sure that's necessary.  The spec says to check whether words are
> valid by doing the & 0xc000 == 0x4000 test.

What early spec says what state word 106 is in ? Healthy paranoia is a
good idea in the IDE world because its all a bit murky in the early days
and you get some quite strange ident data from early devices - one reason
for 0xC000 = 0x4000 is that some early drives use 0xFFFF for unknown words
for example!

> good migration path?  We could have the driver set a flag, or call into
> the driver from the midlayer to check whether it can cope with a
> particular sector size.

On the driver side I need to know so I can control the FIFO so I guess
knowing when you start/end planning to use large sector sizes. The driver
could do it per command but the cost is almost certainly not worth it as
I'd expect us to stick to a size. A driver method would do the trick
nicely if it could return -EOPNOTSUPP or similar.

Alan

Re: [PATCH] ata: Add support for Long Logical Sectors and Long Physical Sectors

[Matthew Wilcox]

On Wed, Aug 06, 2008 at 10:06:47AM +0100, Alan Cox wrote:
> > I'm not sure that's necessary.  The spec says to check whether words are
> > valid by doing the & 0xc000 == 0x4000 test.
> 
> What early spec says what state word 106 is in ? Healthy paranoia is a
> good idea in the IDE world because its all a bit murky in the early days
> and you get some quite strange ident data from early devices - one reason
> for 0xC000 = 0x4000 is that some early drives use 0xFFFF for unknown words
> for example!

ATA-1 says that word 106 is reserved (and further that reserved means
the drive shall return 0).  I don't have a spec earlier than that.

I suspect the ATA spec writers are assuming that no drive puts random
bits there.  all-0 and all-1 make sense, but alternating 0 and 1 are
unlikely.  At least, it's good enough for us for checking words 48, 76, 78,
83, 84 and 87, so I'm not sure why 106 would be different.

> > good migration path?  We could have the driver set a flag, or call into
> > the driver from the midlayer to check whether it can cope with a
> > particular sector size.
> 
> On the driver side I need to know so I can control the FIFO so I guess
> knowing when you start/end planning to use large sector sizes. The driver
> could do it per command but the cost is almost certainly not worth it as
> I'd expect us to stick to a size. A driver method would do the trick
> nicely if it could return -EOPNOTSUPP or similar.

Obviously it is going to change per command -- because different
commands have different sizes.  I was thinking that we could call the
driver to see if it can handle a particular sector size right after we
get the IDENTIFY data.  

Something like this, perhaps:

diff --git a/drivers/ata/ata_ram.c b/drivers/ata/ata_ram.c
index 7479b69..7c30e97 100644
--- a/drivers/ata/ata_ram.c
+++ b/drivers/ata/ata_ram.c
@@ -595,11 +595,17 @@ static bool ata_ram_qc_fill_rtf(struct ata_queued_cmd *qc)
 	return true;
 }
 
+static bool ata_ram_sector_size_supported(struct ata_device *dev)
+{
+	return 1;
+}
+
 static struct ata_port_operations ata_ram_port_ops = {
 	.qc_fill_rtf = ata_ram_qc_fill_rtf,
 	.qc_prep = ata_ram_qc_prep,
 	.qc_issue = ata_ram_qc_issue,
 	.error_handler = ata_ram_error_handler,
+	.sector_size_supported = ata_ram_sector_size_supported,
 };
 
 static struct scsi_host_template ata_ram_template = {
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 8f38d0c..28f16fb 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2236,6 +2236,20 @@ static void ata_dev_config_ncq(struct ata_device *dev,
 		snprintf(desc, desc_sz, "NCQ (depth %d/%d)", hdepth, ddepth);
 }
 
+static int ata_check_sect_size(struct ata_device *dev)
+{
+	/* Every device can handle 512 byte sectors */
+	if (dev->sect_size == 512)
+		return 0;
+	/* Linux doesn't handle sectors larger than 4GB.  This may be a
+	 * problem around 2050 or so.  Deal with it then.  */
+	if (dev->sect_size > 0xffffffffULL)
+		return -EINVAL;
+	if (!dev->link->ap->ops->sector_size_supported)
+		return -EINVAL;
+	return dev->link->ap->ops->sector_size_supported(dev) ? 0 : -EINVAL;
+}
+
 /**
  *	ata_dev_configure - Configure the specified ATA/ATAPI device
  *	@dev: Target device to configure
@@ -2356,10 +2370,10 @@ int ata_dev_configure(struct ata_device *dev)
 
 		dev->n_sectors = ata_id_n_sectors(id);
 		dev->sect_size = ata_id_sect_size(id);
-		if (dev->sect_size > 0xffffffffULL) {
-			ata_dev_printk(dev, KERN_ERR, "sector size larger than "
-						"4GB not supported.\n");
-			rc = -EINVAL;
+		rc = ata_check_sect_size(dev);
+		if (rc) {
+			ata_dev_printk(dev, KERN_ERR, "sector size %lld not "
+					"supported.\n", dev->sect_size);
 			goto err_out_nosup;
 		}
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index fcf84d1..16a2132 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -754,6 +754,7 @@ struct ata_port_operations {
 	unsigned int (*read_id)(struct ata_device *dev, struct ata_taskfile *tf, u16 *id);
 
 	void (*dev_config)(struct ata_device *dev);
+	bool (*sector_size_supported)(struct ata_device *dev);
 
 	void (*freeze)(struct ata_port *ap);
 	void (*thaw)(struct ata_port *ap);

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] ata: Add support for Long Logical Sectors and Long Physical Sectors

[Alan Cox]

> Obviously it is going to change per command -- because different
> commands have different sizes.  I was thinking that we could call the
> driver to see if it can handle a particular sector size right after we
> get the IDENTIFY data.  

The drivers need to know if you are going to be using odd sizes regularly
so they can pick between

	-	I do this fine who cares (most chips)
	-	Er uh wtf its not 512 byts (some chip state machines)
	-	FIFO off (performance hit) for this disk
	-	FIFO managed for the odd command thats a funny size
	-	Various other levels of software managed controller
		thumping

It's not a passive thing and we'd want to do it post identify on the
drive pair as it'll often need per channel decisions (eg on FIFO)

Alan

Re: [PATCH] ata: Add support for Long Logical Sectors and Long Physical Sectors

[Matthew Wilcox]

On Wed, Aug 06, 2008 at 03:13:02PM +0100, Alan Cox wrote:
> > Obviously it is going to change per command -- because different
> > commands have different sizes.  I was thinking that we could call the
> > driver to see if it can handle a particular sector size right after we
> > get the IDENTIFY data.  
> 
> The drivers need to know if you are going to be using odd sizes regularly
> so they can pick between

It's up to the drive to report the number of sectors it uses.  After
that all regular read/write commands will be doing that size.  Unless
we're in some very bizarre situation, that will be the majority of
accesses to the device.

> 	-	I do this fine who cares (most chips)

Fine, returns 1.

> 	-	Er uh wtf its not 512 byts (some chip state machines)

Fine, returns 0.

> 	-	FIFO off (performance hit) for this disk

Might want to print a message explaining why performance is going to
suck and return 1.

> 	-	FIFO managed for the odd command thats a funny size

... but it's not the odd command, it's going to be the vast majority of
them.

> 	-	Various other levels of software managed controller
> 		thumping
> 
> It's not a passive thing and we'd want to do it post identify on the
> drive pair as it'll often need per channel decisions (eg on FIFO)

Why can't you just disable the (controller) FIFO whenever any drive
reports != 512 byte sectors?

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] ata: Add support for Long Logical Sectors and Long Physical Sectors

[Alan Cox]

> > 	-	FIFO managed for the odd command thats a funny size
> 
> ... but it's not the odd command, it's going to be the vast majority of
> them.

Put a 512 byte and a 2K sector sized disk on the same channel and its
probably nearer 50/50.

> 
> > 	-	Various other levels of software managed controller
> > 		thumping
> > 
> > It's not a passive thing and we'd want to do it post identify on the
> > drive pair as it'll often need per channel decisions (eg on FIFO)
> 
> Why can't you just disable the (controller) FIFO whenever any drive
> reports != 512 byte sectors?

Because I'd like to do better than that - and FIFO is only one of the
cases. I think your proposal basically works - providing you identify
both drives before worrying about sector sizes. Most of our other logic
is done that way - the drives are all identified then the controller makes
decisions.

The setup time method then either vetos the disk or accepts it
The existing mode_filter can be used to implement stuff like "PIO only no
DMA" cases
And if we can do it but have problems the qc_issue or device select
methods can be hooked to do the work.

Alan

Re: [PATCH] ata: Add support for Long Logical Sectors and Long Physical Sectors

[Jeff Garzik]

Matthew Wilcox wrote:
> From: Matthew Wilcox <willy@linux.intel.com>
> 
> ATA 8 adds support for devices that have sector sizes larger than 512
> bytes.  We record the sector size in the ata_device and use it instead
> of the ATA_SECT_SIZE when the data transfer is a multiple of sectors.
> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> ---
>  drivers/ata/libata-core.c |  127 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/ata/libata-scsi.c |   52 +++++++++++++-----
>  include/linux/libata.h    |    4 +-
>  3 files changed, 168 insertions(+), 15 deletions(-)

Was this ever updated regarding Alan's final comments?

I only have this patch (the one initiating the email sub-thread).

	Jeff

Re: [PATCH] ata: Add support for Long Logical Sectors and Long Physical Sectors

[Matthew Wilcox]

On Sun, Aug 17, 2008 at 07:19:45PM -0400, Jeff Garzik wrote:
> Matthew Wilcox wrote:
> >From: Matthew Wilcox <willy@linux.intel.com>
> >
> >ATA 8 adds support for devices that have sector sizes larger than 512
> >bytes.  We record the sector size in the ata_device and use it instead
> >of the ATA_SECT_SIZE when the data transfer is a multiple of sectors.
> >
> >Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> >---
> > drivers/ata/libata-core.c |  127 
> > +++++++++++++++++++++++++++++++++++++++++++++
> > drivers/ata/libata-scsi.c |   52 +++++++++++++-----
> > include/linux/libata.h    |    4 +-
> > 3 files changed, 168 insertions(+), 15 deletions(-)
> 
> Was this ever updated regarding Alan's final comments?
> 
> I only have this patch (the one initiating the email sub-thread).

I didn't update it fully; I moved onto something else.  I didn't change
the first of the two patches, so I'll just include the new version of
the second one in this mail.  I think it addresses all of Alan's comments.
I'll have a git tree up soon for you to pull from.

commit dfde7a888bae3b36113b19f37e9edb29be9ae803
Author: Matthew Wilcox <willy@linux.intel.com>
Date:   Tue Aug 5 02:25:57 2008 -0400

    ata: Add support for Long Logical Sectors and Long Physical Sectors
    
    ATA 8 permits devices that have sector sizes larger than 512 bytes.
    We support this by recording the sector size in the ata_device and use
    it instead of the ATA_SECT_SIZE when the data transfer is a multiple
    of sectors.  Drivers must indicate their support for sector sizes other
    than 512 by implementing the 'sector_size_supported' port operation.
    
    Signed-off-by: Matthew Wilcox <willy@linux.intel.com>

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 5ba96c5..31e359e 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -446,6 +446,113 @@ void ata_tf_from_fis(const u8 *fis, struct ata_taskfile *tf)
 	tf->hob_nsect	= fis[13];
 }
 
+/**
+ * ata_sect_size - Returns the sector size to use for a command
+ * @command: The ATA command byte
+ * @dev_sect_size: The size of the drive's sectors
+ *
+ * Some commands are specified to transfer (a multiple of) 512 bytes of data
+ * while others transfer a multiple of the number of bytes in a sector.  This
+ * function knows which commands transfer how much data.
+ */
+unsigned ata_sect_size(u8 command, unsigned dev_sect_size)
+{
+	enum {
+		UNKNOWN = 0,
+		FIVE_TWELVE = 1,
+		DEVICE = 2,
+	};
+
+	static const char command_sect_size[256] = {
+		[ ATA_CMD_CFA_TRANSLATE_SECTOR ] = DEVICE,
+		[ ATA_CMD_CFA_WRITE_MULTI_WITHOUT_ERASE ] = DEVICE,
+		[ ATA_CMD_CFA_WRITE_SECTORS_WITHOUT_ERASE ] = DEVICE,
+		[ ATA_CMD_READ ] = DEVICE,
+		[ ATA_CMD_READ_EXT ] = DEVICE,
+		[ ATA_CMD_READ_QUEUED ] = DEVICE,
+		[ ATA_CMD_READ_QUEUED_EXT ] = DEVICE,
+		[ ATA_CMD_FPDMA_READ ] = DEVICE,
+		[ ATA_CMD_READ_MULTI ] = DEVICE,
+		[ ATA_CMD_READ_MULTI_EXT ] = DEVICE,
+		[ ATA_CMD_PIO_READ ] = DEVICE,
+		[ ATA_CMD_PIO_READ_EXT ] = DEVICE,
+		[ ATA_CMD_READ_STREAM_DMA_EXT ] = DEVICE,
+		[ ATA_CMD_READ_STREAM_EXT ] = DEVICE,
+		[ ATA_CMD_VERIFY ] = DEVICE,
+		[ ATA_CMD_VERIFY_EXT ] = DEVICE, /* XXX: not listed in rev 5 */
+		[ ATA_CMD_WRITE ] = DEVICE,
+		[ ATA_CMD_WRITE_EXT ] = DEVICE,
+		[ ATA_CMD_WRITE_FUA_EXT ] = DEVICE,
+		[ ATA_CMD_WRITE_DMA_QUEUED ] = DEVICE,
+		[ ATA_CMD_WRITE_DMA_QUEUED_EXT ] = DEVICE,
+		[ ATA_CMD_WRITE_DMA_QUEUED_FUA_EXT ] = DEVICE,
+		[ ATA_CMD_FPDMA_WRITE ] = DEVICE,
+		[ ATA_CMD_WRITE_MULTI ] = DEVICE,
+		[ ATA_CMD_WRITE_MULTI_EXT ] = DEVICE,
+		[ ATA_CMD_WRITE_MULTI_FUA_EXT ] = DEVICE,
+		[ ATA_CMD_PIO_WRITE ] = DEVICE,
+		[ ATA_CMD_PIO_WRITE_EXT ] = DEVICE,
+		[ ATA_CMD_WRITE_STREAM_DMA_EXT ] = DEVICE,
+		[ ATA_CMD_WRITE_STREAM_EXT ] = DEVICE,
+		[ ATA_CMD_CFA_ERASE_SECTORS ] = FIVE_TWELVE,
+		[ ATA_CMD_CFA_REQUEST_EXT_ERROR ] = FIVE_TWELVE,
+		[ ATA_CMD_CHK_MEDIA_CARD_TYPE ] = FIVE_TWELVE,
+		[ ATA_CMD_CHK_POWER ] = FIVE_TWELVE,
+		[ ATA_CMD_CONFIG_STREAM ] = FIVE_TWELVE,
+		[ ATA_CMD_CONF_OVERLAY ] = FIVE_TWELVE,
+		[ ATA_CMD_DEV_RESET ] = FIVE_TWELVE,
+		[ ATA_CMD_DLOAD_MCODE ] = FIVE_TWELVE,
+		[ ATA_CMD_EDD ] = FIVE_TWELVE,
+		[ ATA_CMD_FLUSH ] = FIVE_TWELVE,
+		[ ATA_CMD_FLUSH_EXT ] = FIVE_TWELVE,
+		[ ATA_CMD_ID_ATA ] = FIVE_TWELVE,
+		[ ATA_CMD_ID_ATAPI ] = FIVE_TWELVE,
+		[ ATA_CMD_IDLE ] = FIVE_TWELVE,
+		[ ATA_CMD_IDLEIMMEDIATE ] = FIVE_TWELVE,
+		[ ATA_CMD_NVCACHE ] = FIVE_TWELVE,
+		[ ATA_CMD_NOP ] = FIVE_TWELVE,
+		[ ATA_CMD_PACKET ] = FIVE_TWELVE,
+		[ ATA_CMD_PMP_READ ] = FIVE_TWELVE,
+		[ ATA_CMD_READ_LOG_EXT ] = FIVE_TWELVE,
+		[ ATA_CMD_READ_LOG_DMA_EXT ] = FIVE_TWELVE,
+		[ ATA_CMD_READ_NATIVE_MAX ] = FIVE_TWELVE,
+		[ ATA_CMD_READ_NATIVE_MAX_EXT ] = FIVE_TWELVE,
+		[ ATA_CMD_SEC_DISABLE_PASSWORD ] = FIVE_TWELVE,
+		[ ATA_CMD_SEC_ERASE_PREPARE ] = FIVE_TWELVE,
+		[ ATA_CMD_SEC_ERASE_UNIT ] = FIVE_TWELVE,
+		[ ATA_CMD_SEC_FREEZE_LOCK ] = FIVE_TWELVE,
+		[ ATA_CMD_SEC_SET_PASSWORD ] = FIVE_TWELVE,
+		[ ATA_CMD_SEC_UNLOCK ] = FIVE_TWELVE,
+		[ ATA_CMD_SERVICE ] = FIVE_TWELVE,
+		[ ATA_CMD_SET_FEATURES ] = FIVE_TWELVE,
+		[ ATA_CMD_SET_MAX ] = FIVE_TWELVE,
+		[ ATA_CMD_SET_MAX_EXT ] = FIVE_TWELVE,
+		[ ATA_CMD_SET_MULTI ] = FIVE_TWELVE,
+		[ ATA_CMD_SLEEP ] = FIVE_TWELVE,
+		[ ATA_CMD_SMART ] = FIVE_TWELVE,
+		[ ATA_CMD_STANDBY ] = FIVE_TWELVE,
+		[ ATA_CMD_STANDBYNOW1 ] = FIVE_TWELVE,
+		[ ATA_CMD_TRUSTED_NON_DATA ] = FIVE_TWELVE,
+		[ ATA_CMD_TRUSTED_RECEIVE ] = FIVE_TWELVE,
+		[ ATA_CMD_TRUSTED_RECEIVE_DMA ] = FIVE_TWELVE,
+		[ ATA_CMD_TRUSTED_SEND ] = FIVE_TWELVE,
+		[ ATA_CMD_TRUSTED_SEND_DMA ] = FIVE_TWELVE,
+		[ ATA_CMD_PMP_WRITE ] = FIVE_TWELVE,
+		[ ATA_CMD_WRITE_LOG_EXT ] = FIVE_TWELVE,
+		[ ATA_CMD_WRITE_LOG_DMA_EXT ] = FIVE_TWELVE,
+		[ ATA_CMD_WRITE_UNCORRECTABLE_EXT ] = FIVE_TWELVE,
+		[ ATA_CMD_INIT_DEV_PARAMS ] = FIVE_TWELVE,
+	};
+
+	char size = command_sect_size[command];
+	if (size == DEVICE)
+		return dev_sect_size;
+	if (size == UNKNOWN && printk_ratelimit())
+		printk("Unknown ata cmd %d, assuming 512 byte sector size\n",
+				command);
+	return 512;
+}
+
 static const u8 ata_rw_cmds[] = {
 	/* pio multi */
 	ATA_CMD_READ_MULTI,
@@ -1190,6 +1297,25 @@ static u64 ata_id_n_sectors(const u16 *id)
 	}
 }
 
+/*
+ * ATA supports sector sizes up to 2^33 - 1.  The reported sector size may
+ * not be a power of two.  The extra bytes are used for user-visible data
+ * integrity calculations.  Note this is not the same as the ECC which is
+ * accessed through the SCT Command Transport or READ / WRITE LONG.
+ */
+static u64 ata_id_sect_size(const u16 *id)
+{
+	u16 word_106 = id[106];
+	u64 sz;
+
+	if ((word_106 & 0xc000) != 0x4000)
+		return ATA_SECT_SIZE;
+	if (!(word_106 & (1 << 12)))
+		return ATA_SECT_SIZE;
+	sz = (id[117] | ((u64)id[118] << 16)) * 2;
+	return sz;
+}
+
 u64 ata_tf_to_lba48(const struct ata_taskfile *tf)
 {
 	u64 sectors = 0;
@@ -2117,6 +2243,20 @@ static void ata_dev_config_ncq(struct ata_device *dev,
 		snprintf(desc, desc_sz, "NCQ (depth %d/%d)", hdepth, ddepth);
 }
 
+static int ata_check_sect_size(struct ata_device *dev)
+{
+	/* Every device can handle 512 byte sectors */
+	if (dev->sect_size == 512)
+		return 0;
+	/* Linux doesn't handle sectors above 4GB.  This may be a problem
+	 * around 2050 or so.  Deal with it then.  */
+	if (dev->sect_size > 0xffffffffULL)
+		return -EINVAL;
+	if (!dev->link->ap->ops->sector_size_supported)
+		return -EINVAL;
+	return dev->link->ap->ops->sector_size_supported(dev) ? 0 : -EINVAL;
+}
+
 /**
  *	ata_dev_configure - Configure the specified ATA/ATAPI device
  *	@dev: Target device to configure
@@ -2196,6 +2336,7 @@ int ata_dev_configure(struct ata_device *dev)
 	dev->max_sectors = 0;
 	dev->cdb_len = 0;
 	dev->n_sectors = 0;
+	dev->sect_size = ATA_SECT_SIZE;
 	dev->cylinders = 0;
 	dev->heads = 0;
 	dev->sectors = 0;
@@ -2235,6 +2376,13 @@ int ata_dev_configure(struct ata_device *dev)
 		}
 
 		dev->n_sectors = ata_id_n_sectors(id);
+		dev->sect_size = ata_id_sect_size(id);
+		rc = ata_check_sect_size(dev);
+		if (rc) {
+			ata_dev_printk(dev, KERN_ERR, "sector size %lld not "
+					"supported.\n", dev->sect_size);
+			goto err_out_nosup;
+		}
 
 		if (dev->id[59] & 0x100)
 			dev->multi_count = dev->id[59] & 0xff;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index b9d3ba4..145ea08 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -49,7 +49,6 @@
 
 #include "libata.h"
 
-#define SECTOR_SIZE		512
 #define ATA_SCSI_RBUF_SIZE	4096
 
 static DEFINE_SPINLOCK(ata_scsi_rbuf_lock);
@@ -347,7 +346,7 @@ static int ata_get_identity(struct scsi_device *sdev, void __user *arg)
 
 /**
  *	ata_cmd_ioctl - Handler for HDIO_DRIVE_CMD ioctl
- *	@scsidev: Device to which we are issuing command
+ *	@sdev: Device to which we are issuing command
  *	@arg: User provided data for issuing command
  *
  *	LOCKING:
@@ -356,7 +355,7 @@ static int ata_get_identity(struct scsi_device *sdev, void __user *arg)
  *	RETURNS:
  *	Zero on success, negative errno on error.
  */
-int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
+int ata_cmd_ioctl(struct scsi_device *sdev, void __user *arg)
 {
 	int rc = 0;
 	u8 scsi_cmd[MAX_COMMAND_SIZE];
@@ -378,7 +377,8 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
 	memset(scsi_cmd, 0, sizeof(scsi_cmd));
 
 	if (args[3]) {
-		argsize = SECTOR_SIZE * args[3];
+		unsigned sect_size = ata_sect_size(args[0], sdev->sector_size);
+		argsize = sect_size * args[3];
 		argbuf = kmalloc(argsize, GFP_KERNEL);
 		if (argbuf == NULL) {
 			rc = -ENOMEM;
@@ -410,7 +410,7 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
 
 	/* Good values for timeout and retries?  Values below
 	   from scsi_ioctl_send_command() for default case... */
-	cmd_result = scsi_execute(scsidev, scsi_cmd, data_dir, argbuf, argsize,
+	cmd_result = scsi_execute(sdev, scsi_cmd, data_dir, argbuf, argsize,
 				  sensebuf, (10*HZ), 5, 0);
 
 	if (driver_byte(cmd_result) == DRIVER_SENSE) {/* sense data available */
@@ -1493,6 +1493,7 @@ nothing_to_do:
 static unsigned int ata_scsi_rw_xlat(struct ata_queued_cmd *qc)
 {
 	struct scsi_cmnd *scmd = qc->scsicmd;
+	struct ata_device *dev = qc->dev;
 	const u8 *cdb = scmd->cmnd;
 	unsigned int tf_flags = 0;
 	u64 block;
@@ -1549,9 +1550,9 @@ static unsigned int ata_scsi_rw_xlat(struct ata_queued_cmd *qc)
 		goto nothing_to_do;
 
 	qc->flags |= ATA_QCFLAG_IO;
-	qc->nbytes = n_block * ATA_SECT_SIZE;
+	qc->nbytes = n_block * dev->sect_size;
 
-	rc = ata_build_rw_tf(&qc->tf, qc->dev, block, n_block, tf_flags,
+	rc = ata_build_rw_tf(&qc->tf, dev, block, n_block, tf_flags,
 			     qc->tag);
 	if (likely(rc == 0))
 		return 0;
@@ -2217,10 +2218,25 @@ saving_not_supp:
  */
 static unsigned int ata_scsiop_read_cap(struct ata_scsi_args *args, u8 *rbuf)
 {
-	u64 last_lba = args->dev->n_sectors - 1; /* LBA of the last block */
+	struct ata_device *dev = args->dev;
+	u64 last_lba = dev->n_sectors - 1; /* LBA of the last block */
+	u32 sector_size;
+	u8 log_per_phys = 1;
+	u16 first_sector_offset = 0;
+	u16 word_106 = dev->id[106];
 
 	VPRINTK("ENTER\n");
 
+	if ((word_106 & 0xc000) == 0x4000) {
+		/* Number and offset of logical sectors per physical sector */
+		if (word_106 & (1 << 13))
+			log_per_phys = word_106 & 0xf;
+		if ((dev->id[209] & 0xc000) == 0x4000)
+			first_sector_offset = dev->id[209] & 0x3fff;
+	}
+
+	sector_size = dev->sect_size;
+
 	if (args->cmd->cmnd[0] == READ_CAPACITY) {
 		if (last_lba >= 0xffffffffULL)
 			last_lba = 0xffffffff;
@@ -2231,9 +2247,10 @@ static unsigned int ata_scsiop_read_cap(struct ata_scsi_args *args, u8 *rbuf)
 		rbuf[2] = last_lba >> (8 * 1);
 		rbuf[3] = last_lba;
 
-		/* sector size */
-		rbuf[6] = ATA_SECT_SIZE >> 8;
-		rbuf[7] = ATA_SECT_SIZE & 0xff;
+		rbuf[4] = sector_size >> (8 * 3);
+		rbuf[5] = sector_size >> (8 * 2);
+		rbuf[6] = sector_size >> (8 * 1);
+		rbuf[7] = sector_size;
 	} else {
 		/* sector count, 64-bit */
 		rbuf[0] = last_lba >> (8 * 7);
@@ -2246,8 +2263,15 @@ static unsigned int ata_scsiop_read_cap(struct ata_scsi_args *args, u8 *rbuf)
 		rbuf[7] = last_lba;
 
 		/* sector size */
-		rbuf[10] = ATA_SECT_SIZE >> 8;
-		rbuf[11] = ATA_SECT_SIZE & 0xff;
+		rbuf[8] = sector_size >> (8 * 3);
+		rbuf[9] = sector_size >> (8 * 2);
+		rbuf[10] = sector_size >> (8 * 1);
+		rbuf[11] = sector_size;
+
+		rbuf[12] = 0;
+		rbuf[13] = log_per_phys;
+		rbuf[14] = first_sector_offset >> 8;
+		rbuf[15] = first_sector_offset;
 	}
 
 	return 0;
@@ -2721,7 +2745,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 	}
 
 	/* READ/WRITE LONG use a non-standard sect_size */
-	qc->sect_size = ATA_SECT_SIZE;
+	qc->sect_size = ata_sect_size(tf->command, dev->sect_size);
 	switch (tf->command) {
 	case ATA_CMD_READ_LONG:
 	case ATA_CMD_READ_LONG_ONCE:
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 06b8033..16a2132 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -553,8 +553,8 @@ struct ata_ering {
 struct ata_device {
 	struct ata_link		*link;
 	unsigned int		devno;		/* 0 or 1 */
-	unsigned long		flags;		/* ATA_DFLAG_xxx */
 	unsigned int		horkage;	/* List of broken features */
+	unsigned long		flags;		/* ATA_DFLAG_xxx */
 	struct scsi_device	*sdev;		/* attached SCSI device */
 #ifdef CONFIG_ATA_ACPI
 	acpi_handle		acpi_handle;
@@ -562,6 +562,7 @@ struct ata_device {
 #endif
 	/* n_sector is used as CLEAR_OFFSET, read comment above CLEAR_OFFSET */
 	u64			n_sectors;	/* size of device, if ATA */
+	u64			sect_size;	/* Logical, not physical */
 	unsigned int		class;		/* ATA_DEV_xxx */
 
 	u8			pio_mode;
@@ -753,6 +754,7 @@ struct ata_port_operations {
 	unsigned int (*read_id)(struct ata_device *dev, struct ata_taskfile *tf, u16 *id);
 
 	void (*dev_config)(struct ata_device *dev);
+	bool (*sector_size_supported)(struct ata_device *dev);
 
 	void (*freeze)(struct ata_port *ap);
 	void (*thaw)(struct ata_port *ap);
@@ -956,6 +958,7 @@ extern unsigned int ata_do_dev_read_id(struct ata_device *dev,
 					struct ata_taskfile *tf, u16 *id);
 extern void ata_qc_complete(struct ata_queued_cmd *qc);
 extern int ata_qc_complete_multiple(struct ata_port *ap, u32 qc_active);
+extern unsigned ata_sect_size(u8 command, unsigned dev_sect_size);
 extern void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd,
 			      void (*done)(struct scsi_cmnd *));
 extern int ata_std_bios_param(struct scsi_device *sdev,

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] ata: Add support for Long Logical Sectors and Long Physical Sectors

[Alan Cox]

> +	static const char command_sect_size[256] = {
> +		[ ATA_CMD_CFA_TRANSLATE_SECTOR ] = DEVICE,

Should be a bit array so we can do a single fast test_bit()

> +	if (size == UNKNOWN && printk_ratelimit())
> +		printk("Unknown ata cmd %d, assuming 512 byte sector size\n",
> +				command);

or two bits ;)


Rest looks ok. I'll probably send a follow up patch to make most pata
drivers return "no" to anything but 512 until we can test them with real
devices.

Alan

Re: [PATCH] ata: Add support for Long Logical Sectors and Long Physical Sectors

[Matthew Wilcox]

On Mon, Aug 18, 2008 at 07:06:29PM +0100, Alan Cox wrote:
> > +	static const char command_sect_size[256] = {
> > +		[ ATA_CMD_CFA_TRANSLATE_SECTOR ] = DEVICE,
> 
> Should be a bit array so we can do a single fast test_bit()
> 
> > +	if (size == UNKNOWN && printk_ratelimit())
> > +		printk("Unknown ata cmd %d, assuming 512 byte sector size\n",
> > +				command);
> 
> or two bits ;)

;-)  How about two 256-bit arrays?

The first is set for "Use device sector size", the second is set for
"Yes, we know about this command, don't warn".  The most common commands
(reads and writes) will hit the first test and never try the second test.

> Rest looks ok. I'll probably send a follow up patch to make most pata
> drivers return "no" to anything but 512 until we can test them with real
> devices.

Why do we need to do that?  Leaving sector_size_supported() unset means
exactly "no to anything but 512".  I'll be quite surprised if we see
PATA drives supporting 4096 or 520 byte sectors anyway.  It seems like
a premium feature.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] ata: Add support for Long Logical Sectors and Long Physical Sectors

[Alan Cox]

> Why do we need to do that?  Leaving sector_size_supported() unset means
> exactly "no to anything but 512".  I'll be quite surprised if we see
> PATA drives supporting 4096 or 520 byte sectors anyway.  It seems like
> a premium feature.

Good point on the former - yes it defaults sane now so that doesn't need
doing. For the sector size remember a lot of PATA chips appear with SATA
bridges at times - especially promise and highpoint.

Re: [PATCH] ata: Add support for Long Logical Sectors and Long Physical Sectors

[Matthew Wilcox]

On Mon, Aug 18, 2008 at 07:06:29PM +0100, Alan Cox wrote:
> > +	static const char command_sect_size[256] = {
> > +		[ ATA_CMD_CFA_TRANSLATE_SECTOR ] = DEVICE,
> 
> Should be a bit array so we can do a single fast test_bit()

... err ... any idea what the syntax might be for initialising a bit
array at compile time?  Do I have to use an initialiser function at
runtime?

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."