Skip to contentSkip to conversation

2.6.x libata fixes (mostly)

PATCH In review Jeff Garzik 5 participants 13 messages Feb 23, 2005 – Feb 28, 2005

Subsystems (2)
SCSI libata
lore
Share & tools
lore

Jeff Garzik

author
#0 lore raw

PATCH 1/1 2.6.x libata fixes (mostly)

This BK push includes additional hardware support, but that's only because it's (a) obviously low impact and (b) it was in the queue.

Far more important are:

1) API additions, to fix a severe bug: advanced drivers such as AHCI were directly bitbanging --non-existent-- PCI IDE registers, causing an oops, because of function calls buried deep within libata.

Solution: add the necessary hooks that should have existed all along, for this functionality.

2) Fix stomping on active devices, if pci_request_regions() fails. It is rather more urgent for libata than other situations, since we must deal with PCI devices that secretly export ISA device regions (legacy IDE 0x1f0, 0x170).

3) Fix command queue leak, if SCSI->ATA translation fails.

Please do a

bk pull bk://gkernel.bkbits.net/libata-2.6

This will update the following files:

 drivers/scsi/Kconfig        |    8 
 drivers/scsi/Makefile       |    1 
 drivers/scsi/ahci.c         |   18 +
 drivers/scsi/ata_piix.c     |    4 
 drivers/scsi/libata-core.c  |  115 ++++++-
 drivers/scsi/libata-scsi.c  |    1 
 drivers/scsi/libata.h       |    1 
 drivers/scsi/sata_nv.c      |   10 
 drivers/scsi/sata_promise.c |    8 
 drivers/scsi/sata_qstor.c   |  700 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/sata_sil.c     |   10 
 drivers/scsi/sata_sis.c     |   10 
 drivers/scsi/sata_svw.c     |   10 
 drivers/scsi/sata_sx4.c     |    8 
 drivers/scsi/sata_uli.c     |   10 
 drivers/scsi/sata_via.c     |  213 +++++++++----
 drivers/scsi/sata_vsc.c     |   10 
 include/linux/libata.h      |   64 ----
 18 files changed, 1058 insertions(+), 143 deletions(-)

through these ChangeSets:

<[email protected]> (05/02/23 1.2043)
[libata] Add missing hooks, to avoid oops in advanced SATA drivers

Advanced SATA drivers should not (and cannot) use the basic PCI IDE hooks for checking the Status and Error registers, as these registers are either in non-standard locations, or simply don't exist.

In the error handling path, libata was unconditionally calling some PCI IDE hardware bitbanging functions, which would cause an oops in the AHCI driver and any other advanced libata driver.

<[email protected]> (05/02/18 1.2040)
[PATCH] libata: fix command queue leak when xlat_func fails

ata_scsi_translate allocates from the libata command queue by calling ata_scsi_qc_new. If xlat_func returns non-zero, control jumps to err_out which fails to free the allocated command. Fix is to add a new API to free unused commands.

Signed-off-by John W. Linville <[email protected]>
Signed-off-by Jeff Garzik <[email protected]>

<[email protected]> (05/02/17 1.2035.16.1)
[libata] add ->bmdma_{stop,status} hooks

The timeout/error handling path was assuming that the hardware in question was PCI IDE BMDMA-like, which is incorrect in a few cases.

Turn direct function calls into two new hooks.

<[email protected]> (05/02/17 1.2038)
[PATCH] sata_qstor: new basic driver for Pacific Digital

This is a new basic libata SATA driver
for the Pacific Digital QStor SATA/RAID card.

It features ordinary per-drive SATA with single-request DMA for R/W commands, and PIO for everything else.

It currently does not implement any of the hardware RAID support, nor hot-plug, nor the tagged/native command-queuing features.

On the other hand, it is small and simple as a result.

Signed-off-by Mark Lord <[email protected]>
Signed-off-by Jeff Garzik <[email protected]>

<[email protected]> (05/02/13 1.2037)
[libata] do not call pci_disable_device() for certain errors

If PCI request regions fails, then someone else is using the hardware we wish to use. For that one case, calling pci_disable_device() is rather rude.

<[email protected]> (04/11/16 1.1938.367.2)
[libata sata_via] add support for VT6421 SATA

<[email protected]> (04/11/16 1.1938.367.1)
[libata sata_via] minor cleanups

Preparation for addition of VT6421 support. Mostly moving bits of code into discrete functions.

drivers/scsi/Kconfig +8 -0
diff -Nru a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
@@ -457,6 +457,14 @@
 
 	  If unsure, say N.
 
+config SCSI_SATA_QSTOR
+	tristate "Pacific Digital SATA QStor support"
+	depends on SCSI_SATA && PCI
+	help
+	  This option enables support for Pacific Digital Serial ATA QStor.
+
+	  If unsure, say N.
+
 config SCSI_SATA_SX4
 	tristate "Promise SATA SX4 support"
 	depends on SCSI_SATA && PCI && EXPERIMENTAL
drivers/scsi/Makefile +1 -0
diff -Nru a/drivers/scsi/Makefile b/drivers/scsi/Makefile
@@ -125,6 +125,7 @@
 obj-$(CONFIG_SCSI_SATA_SVW)	+= libata.o sata_svw.o
 obj-$(CONFIG_SCSI_ATA_PIIX)	+= libata.o ata_piix.o
 obj-$(CONFIG_SCSI_SATA_PROMISE)	+= libata.o sata_promise.o
+obj-$(CONFIG_SCSI_SATA_QSTOR)	+= libata.o sata_qstor.o
 obj-$(CONFIG_SCSI_SATA_SIL)	+= libata.o sata_sil.o
 obj-$(CONFIG_SCSI_SATA_VIA)	+= libata.o sata_via.o
 obj-$(CONFIG_SCSI_SATA_VITESSE)	+= libata.o sata_vsc.o
drivers/scsi/ahci.c +16 -2
diff -Nru a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
@@ -179,6 +179,7 @@
 static void ahci_host_stop(struct ata_host_set *host_set);
 static void ahci_qc_prep(struct ata_queued_cmd *qc);
 static u8 ahci_check_status(struct ata_port *ap);
+static u8 ahci_check_err(struct ata_port *ap);
 static inline int ahci_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc);
 
 static Scsi_Host_Template ahci_sht = {

@@ -204,6 +205,8 @@
 	.port_disable		= ata_port_disable,
 
 	.check_status		= ahci_check_status,
+	.check_altstatus	= ahci_check_status,
+	.check_err		= ahci_check_err,
 	.dev_select		= ata_noop_dev_select,
 
 	.phy_reset		= ahci_phy_reset,

@@ -452,6 +455,13 @@
 	return readl(mmio + PORT_TFDATA) & 0xFF;
 }
 
+static u8 ahci_check_err(struct ata_port *ap)
+{
+	void *mmio = (void *) ap->ioaddr.cmd_addr;
+
+	return (readl(mmio + PORT_TFDATA) >> 8) & 0xFF;
+}
+
 static void ahci_fill_sg(struct ata_queued_cmd *qc)
 {
 	struct ahci_port_priv *pp = qc->ap->private_data;

@@ -940,6 +950,7 @@
 	unsigned long base;
 	void *mmio_base;
 	unsigned int board_idx = (unsigned int) ent->driver_data;
+	int pci_dev_busy = 0;
 	int rc;
 
 	VPRINTK("ENTER\n");

@@ -952,8 +963,10 @@
 		return rc;
 
 	rc = pci_request_regions(pdev, DRV_NAME);
-	if (rc)
+	if (rc) {
+		pci_dev_busy = 1;
 		goto err_out;
+	}
 
 	pci_enable_intx(pdev);
 

@@ -1015,7 +1028,8 @@
 err_out_regions:
 	pci_release_regions(pdev);
 err_out:
-	pci_disable_device(pdev);
+	if (!pci_dev_busy)
+		pci_disable_device(pdev);
 	return rc;
 }
drivers/scsi/ata_piix.c +4 -0
diff -Nru a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
@@ -138,6 +138,8 @@
 
 	.bmdma_setup		= ata_bmdma_setup,
 	.bmdma_start		= ata_bmdma_start,
+	.bmdma_stop		= ata_bmdma_stop,
+	.bmdma_status		= ata_bmdma_status,
 	.qc_prep		= ata_qc_prep,
 	.qc_issue		= ata_qc_issue_prot,
 

@@ -163,6 +165,8 @@
 
 	.bmdma_setup		= ata_bmdma_setup,
 	.bmdma_start		= ata_bmdma_start,
+	.bmdma_stop		= ata_bmdma_stop,
+	.bmdma_status		= ata_bmdma_status,
 	.qc_prep		= ata_qc_prep,
 	.qc_issue		= ata_qc_issue_prot,
drivers/scsi/libata-core.c +101 -14

228 lines not on this page · raw message

drivers/scsi/libata-scsi.c +1 -0
diff -Nru a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
@@ -701,6 +701,7 @@
 	return;
 
 err_out:
+	ata_qc_free(qc);
 	ata_bad_cdb(cmd, done);
 	DPRINTK("EXIT - badcmd\n");
 }
drivers/scsi/libata.h +1 -0
diff -Nru a/drivers/scsi/libata.h b/drivers/scsi/libata.h
@@ -37,6 +37,7 @@
 /* libata-core.c */
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,
 				      struct ata_device *dev);
+extern void ata_qc_free(struct ata_queued_cmd *qc);
 extern int ata_qc_issue(struct ata_queued_cmd *qc);
 extern int ata_check_atapi_dma(struct ata_queued_cmd *qc);
 extern void ata_dev_select(struct ata_port *ap, unsigned int device,
drivers/scsi/sata_nv.c +8 -2
diff -Nru a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
@@ -217,6 +217,8 @@
 	.phy_reset		= sata_phy_reset,
 	.bmdma_setup		= ata_bmdma_setup,
 	.bmdma_start		= ata_bmdma_start,
+	.bmdma_stop		= ata_bmdma_stop,
+	.bmdma_status		= ata_bmdma_status,
 	.qc_prep		= ata_qc_prep,
 	.qc_issue		= ata_qc_issue_prot,
 	.eng_timeout		= ata_eng_timeout,

@@ -332,6 +334,7 @@
 	struct nv_host *host;
 	struct ata_port_info *ppi;
 	struct ata_probe_ent *probe_ent;
+	int pci_dev_busy = 0;
 	int rc;
 	u32 bar;
 

@@ -350,8 +353,10 @@
 		goto err_out;
 
 	rc = pci_request_regions(pdev, DRV_NAME);
-	if (rc)
+	if (rc) {
+		pci_dev_busy = 1;
 		goto err_out_disable;
+	}
 
 	rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
 	if (rc)

@@ -427,7 +432,8 @@
 err_out_regions:
 	pci_release_regions(pdev);
 err_out_disable:
-	pci_disable_device(pdev);
+	if (!pci_dev_busy)
+		pci_disable_device(pdev);
 err_out:
 	return rc;
 }
drivers/scsi/sata_promise.c +6 -2
diff -Nru a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
@@ -556,6 +556,7 @@
 	unsigned long base;
 	void *mmio_base;
 	unsigned int board_idx = (unsigned int) ent->driver_data;
+	int pci_dev_busy = 0;
 	int rc;
 
 	if (!printed_version++)

@@ -570,8 +571,10 @@
 		return rc;
 
 	rc = pci_request_regions(pdev, DRV_NAME);
-	if (rc)
+	if (rc) {
+		pci_dev_busy = 1;
 		goto err_out;
+	}
 
 	rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
 	if (rc)

@@ -650,7 +653,8 @@
 err_out_regions:
 	pci_release_regions(pdev);
 err_out:
-	pci_disable_device(pdev);
+	if (!pci_dev_busy)
+		pci_disable_device(pdev);
 	return rc;
 }
drivers/scsi/sata_qstor.c new file +700 -0

700 lines not on this page · raw message

drivers/scsi/sata_sil.c +8 -2
diff -Nru a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
@@ -139,6 +139,8 @@
 	.post_set_mode		= sil_post_set_mode,
 	.bmdma_setup            = ata_bmdma_setup,
 	.bmdma_start            = ata_bmdma_start,
+	.bmdma_stop		= ata_bmdma_stop,
+	.bmdma_status		= ata_bmdma_status,
 	.qc_prep		= ata_qc_prep,
 	.qc_issue		= ata_qc_issue_prot,
 	.eng_timeout		= ata_eng_timeout,

@@ -336,6 +338,7 @@
 	void *mmio_base;
 	int rc;
 	unsigned int i;
+	int pci_dev_busy = 0;
 	u32 tmp, irq_mask;
 
 	if (!printed_version++)

@@ -350,8 +353,10 @@
 		return rc;
 
 	rc = pci_request_regions(pdev, DRV_NAME);
-	if (rc)
+	if (rc) {
+		pci_dev_busy = 1;
 		goto err_out;
+	}
 
 	rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
 	if (rc)

@@ -438,7 +443,8 @@
 err_out_regions:
 	pci_release_regions(pdev);
 err_out:
-	pci_disable_device(pdev);
+	if (!pci_dev_busy)
+		pci_disable_device(pdev);
 	return rc;
 }
drivers/scsi/sata_sis.c +8 -2
diff -Nru a/drivers/scsi/sata_sis.c b/drivers/scsi/sata_sis.c
@@ -102,6 +102,8 @@
 	.phy_reset		= sata_phy_reset,
 	.bmdma_setup            = ata_bmdma_setup,
 	.bmdma_start            = ata_bmdma_start,
+	.bmdma_stop		= ata_bmdma_stop,
+	.bmdma_status		= ata_bmdma_status,
 	.qc_prep		= ata_qc_prep,
 	.qc_issue		= ata_qc_issue_prot,
 	.eng_timeout		= ata_eng_timeout,

@@ -200,14 +202,17 @@
 	int rc;
 	u32 genctl;
 	struct ata_port_info *ppi;
+	int pci_dev_busy = 0;
 
 	rc = pci_enable_device(pdev);
 	if (rc)
 		return rc;
 
 	rc = pci_request_regions(pdev, DRV_NAME);
-	if (rc)
+	if (rc) {
+		pci_dev_busy = 1;
 		goto err_out;
+	}
 
 	rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
 	if (rc)

@@ -259,7 +264,8 @@
 	pci_release_regions(pdev);
 
 err_out:
-	pci_disable_device(pdev);
+	if (!pci_dev_busy)
+		pci_disable_device(pdev);
 	return rc;
 
 }
drivers/scsi/sata_svw.c +8 -2
diff -Nru a/drivers/scsi/sata_svw.c b/drivers/scsi/sata_svw.c
@@ -301,6 +301,8 @@
 	.phy_reset		= sata_phy_reset,
 	.bmdma_setup		= k2_bmdma_setup_mmio,
 	.bmdma_start		= k2_bmdma_start_mmio,
+	.bmdma_stop		= ata_bmdma_stop,
+	.bmdma_status		= ata_bmdma_status,
 	.qc_prep		= ata_qc_prep,
 	.qc_issue		= ata_qc_issue_prot,
 	.eng_timeout		= ata_eng_timeout,

@@ -338,6 +340,7 @@
 	struct ata_probe_ent *probe_ent = NULL;
 	unsigned long base;
 	void *mmio_base;
+	int pci_dev_busy = 0;
 	int rc;
 
 	if (!printed_version++)

@@ -359,8 +362,10 @@
 
 	/* Request PCI regions */
 	rc = pci_request_regions(pdev, DRV_NAME);
-	if (rc)
+	if (rc) {
+		pci_dev_busy = 1;
 		goto err_out;
+	}
 
 	rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
 	if (rc)

@@ -433,7 +438,8 @@
 err_out_regions:
 	pci_release_regions(pdev);
 err_out:
-	pci_disable_device(pdev);
+	if (!pci_dev_busy)
+		pci_disable_device(pdev);
 	return rc;
 }
drivers/scsi/sata_sx4.c +6 -2
diff -Nru a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
@@ -1366,6 +1366,7 @@
 	void *mmio_base, *dimm_mmio = NULL;
 	struct pdc_host_priv *hpriv = NULL;
 	unsigned int board_idx = (unsigned int) ent->driver_data;
+	int pci_dev_busy = 0;
 	int rc;
 
 	if (!printed_version++)

@@ -1380,8 +1381,10 @@
 		return rc;
 
 	rc = pci_request_regions(pdev, DRV_NAME);
-	if (rc)
+	if (rc) {
+		pci_dev_busy = 1;
 		goto err_out;
+	}
 
 	rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
 	if (rc)

@@ -1471,7 +1474,8 @@
 err_out_regions:
 	pci_release_regions(pdev);
 err_out:
-	pci_disable_device(pdev);
+	if (!pci_dev_busy)
+		pci_disable_device(pdev);
 	return rc;
 }
drivers/scsi/sata_uli.c +8 -2
diff -Nru a/drivers/scsi/sata_uli.c b/drivers/scsi/sata_uli.c
@@ -97,6 +97,8 @@
 
 	.bmdma_setup            = ata_bmdma_setup,
 	.bmdma_start            = ata_bmdma_start,
+	.bmdma_stop		= ata_bmdma_stop,
+	.bmdma_status		= ata_bmdma_status,
 	.qc_prep		= ata_qc_prep,
 	.qc_issue		= ata_qc_issue_prot,
 

@@ -185,14 +187,17 @@
 	struct ata_port_info *ppi;
 	int rc;
 	unsigned int board_idx = (unsigned int) ent->driver_data;
+	int pci_dev_busy = 0;
 
 	rc = pci_enable_device(pdev);
 	if (rc)
 		return rc;
 
 	rc = pci_request_regions(pdev, DRV_NAME);
-	if (rc)
+	if (rc) {
+		pci_dev_busy = 1;
 		goto err_out;
+	}
 
 	rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
 	if (rc)

@@ -260,7 +265,8 @@
 	pci_release_regions(pdev);
 
 err_out:
-	pci_disable_device(pdev);
+	if (!pci_dev_busy)
+		pci_disable_device(pdev);
 	return rc;
 
 }
drivers/scsi/sata_via.c +157 -56

292 lines not on this page · raw message

drivers/scsi/sata_vsc.c +8 -2
diff -Nru a/drivers/scsi/sata_vsc.c b/drivers/scsi/sata_vsc.c
@@ -217,6 +217,8 @@
 	.phy_reset		= sata_phy_reset,
 	.bmdma_setup            = ata_bmdma_setup,
 	.bmdma_start            = ata_bmdma_start,
+	.bmdma_stop		= ata_bmdma_stop,
+	.bmdma_status		= ata_bmdma_status,
 	.qc_prep		= ata_qc_prep,
 	.qc_issue		= ata_qc_issue_prot,
 	.eng_timeout		= ata_eng_timeout,

@@ -255,6 +257,7 @@
 	static int printed_version;
 	struct ata_probe_ent *probe_ent = NULL;
 	unsigned long base;
+	int pci_dev_busy = 0;
 	void *mmio_base;
 	int rc;
 

@@ -274,8 +277,10 @@
 	}
 
 	rc = pci_request_regions(pdev, DRV_NAME);
-	if (rc)
+	if (rc) {
+		pci_dev_busy = 1;
 		goto err_out;
+	}
 
 	/*
 	 * Use 32 bit DMA mask, because 64 bit address support is poor.

@@ -352,7 +357,8 @@
 err_out_regions:
 	pci_release_regions(pdev);
 err_out:
-	pci_disable_device(pdev);
+	if (!pci_dev_busy)
+		pci_disable_device(pdev);
 	return rc;
 }
include/linux/libata.h +9 -55

105 lines not on this page · raw message

#1 lore raw

drivers/scsi/ahci.c line 460

Quoted 9 lines · Jeff Garzik

This BK push includes additional hardware support, but that's only because it's (a) obviously low impact and (b) it was in the queue.

drivers/scsi/ahci.c +3 -0
 
+static u8 ahci_check_err(struct ata_port *ap)
+{
+	void *mmio = (void *) ap->ioaddr.cmd_addr;

void __iomem *

drivers/scsi/libata-core.c line 2777

Quoted 7 lines
drivers/scsi/ahci.c +1 -0
+	return (readl(mmio + PORT_TFDATA) >> 8) & 0xFF;
drivers/scsi/libata-core.c +2 -0
 
+ *	ata_qc_free - free unused ata_queued_cmd
+ *	@qc: Command to complete

"Command to free"?

--- /dev/null
+++ b/drivers/scsi/sata_qstor.c

drivers/scsi/libata-core.c

Quoted 8 lines
drivers/scsi/libata-core.c +6 -0
+	u8 *prd = pp->pkt + QS_CPB_BYTES;
 
+	for (nelem = 0; nelem < qc->n_elem; nelem++,sg++) {
+		u64 addr;
+		u32 len;
 
+		addr = sg_dma_address(sg);
+		*(u64 *)prd = cpu_to_le64(addr);

*(__le64 *) prd

drivers/scsi/libata-core.c

Quoted 4 lines
drivers/scsi/libata-core.c +3 -0
+		prd += sizeof(u64);
 
+		len = sg_dma_len(sg);
+		*(u32 *)prd = cpu_to_le32(len);

*(__le32 *) prd

drivers/scsi/libata-core.c

Quoted 1 line
drivers/scsi/libata-core.c +1 -0
+		prd += sizeof(u64);

Should this be "prd += sizeof(u32)"? Looks suspicious.

drivers/scsi/libata-core.c

Quoted 7 lines
drivers/scsi/libata-core.c +5 -0
+static void qs_qc_prep(struct ata_queued_cmd *qc)
+{
 
+	*(u32 *)(&buf[ 4]) = cpu_to_le32(qc->nsect * ATA_SECT_SIZE);
+	*(u32 *)(&buf[ 8]) = cpu_to_le32(qc->n_elem);
 
+	*(u64 *)(&buf[16]) = cpu_to_le64(addr);

__le* again...

drivers/scsi/libata-core.c

Quoted 9 lines
drivers/scsi/libata-core.c +6 -0
+static void qs_ata_setup_port(struct ata_ioports *port, unsigned long base)
+{
+	port->cmd_addr		=
 
+	port->error_addr	=
 
+	port->status_addr	=
 
+	port->altstatus_addr	=

Oo-oops...

drivers/scsi/libata-core.c

Quoted 8 lines
drivers/scsi/libata-core.c +7 -0
+static int qs_set_dma_masks(struct pci_dev *pdev, void __iomem *mmio_base)
+{
 
+	if (have_64bit_bus &&
+	    !pci_set_dma_mask(pdev, 0xffffffffffffffffULL)) {
+		rc = pci_set_consistent_dma_mask(pdev, 0xffffffffffffffffULL);
+		if (rc) {
+			rc = pci_set_consistent_dma_mask(pdev, 0xffffffffULL);

We already have DMA_{32,64}BIT_MASK.

drivers/scsi/libata-core.c

Quoted 4 lines
drivers/scsi/libata-core.c +3 -0
+	} else {
+		rc = pci_set_dma_mask(pdev, 0xffffffffULL);
 
+		rc = pci_set_consistent_dma_mask(pdev, 0xffffffffULL);

Alexey

#2 lore raw
Quoted 11 lines · Alexey Dobriyan

drivers/scsi/sata_qstor.c line 456

Quoted 9 lines
drivers/scsi/sata_qstor.c new file +6 -0
+static void qs_ata_setup_port(struct ata_ioports *port, unsigned long base)
+{
+	port->cmd_addr		=
 
+	port->error_addr	=
 
+	port->status_addr	=
 
+	port->altstatus_addr	=

Oo-oops...

Too much snipping and time to sleep for me, sorry. :-(

Alexey

#4 lore raw
In reply to Alexey Dobriyan
Quoted 9 lines · Alexey Dobriyan

drivers/scsi/sata_qstor.c line 264

Quoted 2 lines · Jeff Garzik
drivers/scsi/sata_qstor.c new file +2 -0
+		addr = sg_dma_address(sg);
+		*(u64 *)prd = cpu_to_le64(addr);

*(__le64 *) prd

Quoted 8 lines

drivers/scsi/sata_qstor.c line 268

Quoted 5 lines
drivers/scsi/sata_qstor.c new file +3 -0
+		prd += sizeof(u64);
 
 
+		len = sg_dma_len(sg);
+		*(u32 *)prd = cpu_to_le32(len);

*(__le32 *) prd

If I am not totally mistaken this is not gcc4 friendly code. (lvalue thing...) Wouldn't it be better to prevent double patching?

--
Prakash Punnoor

formerly known as Prakash K. Cheemplavam

#5 lore raw

Prakash> If I am not totally mistaken this is not gcc4 friendly Prakash> code. (lvalue thing...)

Actually you misread the code slightly. It's a little subtle, but code like

		*(__le32 *)prd = cpu_to_le32(len);

is not using a cast as an lvalue. It's dereferencing a cast and as such is totally correct, idiomatic and clean C.

- Roland

#6 lore raw
Quoted 10 lines · Roland Dreier

Prakash> If I am not totally mistaken this is not gcc4 friendly Prakash> code. (lvalue thing...)

Actually you misread the code slightly. It's a little subtle, but code like

		*(__le32 *)prd = cpu_to_le32(len);

is not using a cast as an lvalue. It's dereferencing a cast and as such is totally correct, idiomatic and clean C.

OK, thanks for clearing that up. Obviously my C knowledge needs to be improved...

--
Prakash Punnoor

formerly known as Prakash K. Cheemplavam

#7 lore raw

PATCH 1/1 libata: sata_qstor cosmetic fixes

In reply to Alexey Dobriyan

Minor patch for new 2.6.xx sata_qstor driver attached, as per Alexey's fine-toothed comb! :)

Signed-off-by Mark Lord <[email protected]>

Cheers

Signature · 3 lines
Mark Lord
Real-Time Remedies Inc.
[email protected]
drivers/scsi/sata_qstor.c renamed from drivers/scsi/sata_qstor.c.orig +10 -10
@@ -261,11 +261,11 @@
 		u32 len;
 
 		addr = sg_dma_address(sg);
-		*(u64 *)prd = cpu_to_le64(addr);
+		*(__le64 *)prd = cpu_to_le64(addr);
 		prd += sizeof(u64);
 
 		len = sg_dma_len(sg);
-		*(u32 *)prd = cpu_to_le32(len);
+		*(__le32 *)prd = cpu_to_le32(len);
 		prd += sizeof(u64);
 
 		VPRINTK("PRD[%u] = (0x%llX, 0x%X)\n", nelem,

@@ -298,10 +298,10 @@
 	/* host control block (HCB) */
 	buf[ 0] = QS_HCB_HDR;
 	buf[ 1] = hflags;
-	*(u32 *)(&buf[ 4]) = cpu_to_le32(qc->nsect * ATA_SECT_SIZE);
-	*(u32 *)(&buf[ 8]) = cpu_to_le32(qc->n_elem);
+	*(__le32 *)(&buf[ 4]) = cpu_to_le32(qc->nsect * ATA_SECT_SIZE);
+	*(__le32 *)(&buf[ 8]) = cpu_to_le32(qc->n_elem);
 	addr = ((u64)pp->pkt_dma) + QS_CPB_BYTES;
-	*(u64 *)(&buf[16]) = cpu_to_le64(addr);
+	*(__le64 *)(&buf[16]) = cpu_to_le64(addr);
 
 	/* device control block (DCB) */
 	buf[24] = QS_DCB_HDR;

@@ -566,10 +566,10 @@
 	int rc, have_64bit_bus = (bus_info & QS_HPHY_64BIT);
 
 	if (have_64bit_bus &&
-	    !pci_set_dma_mask(pdev, 0xffffffffffffffffULL)) {
-		rc = pci_set_consistent_dma_mask(pdev, 0xffffffffffffffffULL);
+	    !pci_set_dma_mask(pdev, DMA_64BIT_MASK)) {
+		rc = pci_set_consistent_dma_mask(pdev, DMA_64BIT_MASK);
 		if (rc) {
-			rc = pci_set_consistent_dma_mask(pdev, 0xffffffffULL);
+			rc = pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK);
 			if (rc) {
 				printk(KERN_ERR DRV_NAME
 					"(%s): 64-bit DMA enable failed\n",

@@ -578,14 +578,14 @@
 			}
 		}
 	} else {
-		rc = pci_set_dma_mask(pdev, 0xffffffffULL);
+		rc = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
 		if (rc) {
 			printk(KERN_ERR DRV_NAME
 				"(%s): 32-bit DMA enable failed\n",
 				pci_name(pdev));
 			return rc;
 		}
-		rc = pci_set_consistent_dma_mask(pdev, 0xffffffffULL);
+		rc = pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK);
 		if (rc) {
 			printk(KERN_ERR DRV_NAME
 				"(%s): 32-bit consistent DMA enable failed\n",
author
#8 lore raw

Re: [PATCH 2.6.11+ sata_qstor] libata: sata_qstor cosmetic fixes

Quoted 4 lines · Mark Lord

Minor patch for new 2.6.xx sata_qstor driver attached, as per Alexey's fine-toothed comb! :)

Signed-off-by Mark Lord <[email protected]>

Cool, I'll throw this into libata-2.6 soonish.

Jeff

author
#9 lore raw

Re: [PATCH 2.6.11+ sata_qstor] libata: sata_qstor cosmetic fixes

In reply to Mark Lord
Quoted 4 lines · Mark Lord

Minor patch for new 2.6.xx sata_qstor driver attached, as per Alexey's fine-toothed comb! :)

Signed-off-by Mark Lord <[email protected]>

I had to apply this manually, since your mailer "corrupts" the patch by encoding text/plain as base64. Please fix your mailer...

The ideal is an inline patch, rather than an attachment anyway. e.g.

To: ...
From: ...
Subject: ...
<blank line>
Patch description
Patch

cat'd to 'sendmail -t'. Sendmail (or another MTA which provides a /usr/sbin/sendmail wrapper) will automatically fill in other headers like Message-ID and Date.

Jeff

#10 lore raw

Re: [PATCH 2.6.11+ sata_qstor] libata: sata_qstor cosmetic fixes

Quoted 8 lines · Jeff Garzik
Quoted 4 lines · Mark Lord

Minor patch for new 2.6.xx sata_qstor driver attached, as per Alexey's fine-toothed comb! :)

Signed-off-by Mark Lord <[email protected]>

I had to apply this manually, since your mailer "corrupts" the patch by encoding text/plain as base64. Please fix your mailer...

Mmm.. first time that's ever happened here. It's not the mailer, but the mime-types, which must have gotten confused by the ".patch1" extension, as opposed to the normal ".patch".

Here it is again, renamed to avoid the faulty mime handling.

Signed-off-by Mark Lord <[email protected]>

Cheers

Signature · 3 lines
Mark Lord
Real-Time Remedies Inc.
[email protected]
drivers/scsi/sata_qstor.c renamed from drivers/scsi/sata_qstor.c.orig +10 -10
@@ -261,11 +261,11 @@
 		u32 len;
 
 		addr = sg_dma_address(sg);
-		*(u64 *)prd = cpu_to_le64(addr);
+		*(__le64 *)prd = cpu_to_le64(addr);
 		prd += sizeof(u64);
 
 		len = sg_dma_len(sg);
-		*(u32 *)prd = cpu_to_le32(len);
+		*(__le32 *)prd = cpu_to_le32(len);
 		prd += sizeof(u64);
 
 		VPRINTK("PRD[%u] = (0x%llX, 0x%X)\n", nelem,

@@ -298,10 +298,10 @@
 	/* host control block (HCB) */
 	buf[ 0] = QS_HCB_HDR;
 	buf[ 1] = hflags;
-	*(u32 *)(&buf[ 4]) = cpu_to_le32(qc->nsect * ATA_SECT_SIZE);
-	*(u32 *)(&buf[ 8]) = cpu_to_le32(qc->n_elem);
+	*(__le32 *)(&buf[ 4]) = cpu_to_le32(qc->nsect * ATA_SECT_SIZE);
+	*(__le32 *)(&buf[ 8]) = cpu_to_le32(qc->n_elem);
 	addr = ((u64)pp->pkt_dma) + QS_CPB_BYTES;
-	*(u64 *)(&buf[16]) = cpu_to_le64(addr);
+	*(__le64 *)(&buf[16]) = cpu_to_le64(addr);
 
 	/* device control block (DCB) */
 	buf[24] = QS_DCB_HDR;

@@ -566,10 +566,10 @@
 	int rc, have_64bit_bus = (bus_info & QS_HPHY_64BIT);
 
 	if (have_64bit_bus &&
-	    !pci_set_dma_mask(pdev, 0xffffffffffffffffULL)) {
-		rc = pci_set_consistent_dma_mask(pdev, 0xffffffffffffffffULL);
+	    !pci_set_dma_mask(pdev, DMA_64BIT_MASK)) {
+		rc = pci_set_consistent_dma_mask(pdev, DMA_64BIT_MASK);
 		if (rc) {
-			rc = pci_set_consistent_dma_mask(pdev, 0xffffffffULL);
+			rc = pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK);
 			if (rc) {
 				printk(KERN_ERR DRV_NAME
 					"(%s): 64-bit DMA enable failed\n",

@@ -578,14 +578,14 @@
 			}
 		}
 	} else {
-		rc = pci_set_dma_mask(pdev, 0xffffffffULL);
+		rc = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
 		if (rc) {
 			printk(KERN_ERR DRV_NAME
 				"(%s): 32-bit DMA enable failed\n",
 				pci_name(pdev));
 			return rc;
 		}
-		rc = pci_set_consistent_dma_mask(pdev, 0xffffffffULL);
+		rc = pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK);
 		if (rc) {
 			printk(KERN_ERR DRV_NAME
 				"(%s): 32-bit consistent DMA enable failed\n",
author
#11 lore raw

Re: [PATCH 2.6.11+ sata_qstor] libata: sata_qstor cosmetic fixes

Quoted 13 lines · Mark Lord
Quoted 9 lines · Jeff Garzik
Quoted 4 lines · Mark Lord

Minor patch for new 2.6.xx sata_qstor driver attached, as per Alexey's fine-toothed comb! :)

Signed-off-by Mark Lord <[email protected]>

I had to apply this manually, since your mailer "corrupts" the patch by encoding text/plain as base64. Please fix your mailer...

Quoted 8 lines

Mmm.. first time that's ever happened here. It's not the mailer, but the mime-types, which must have gotten confused by the ".patch1" extension, as opposed to the normal ".patch".

Here it is again, renamed to avoid the faulty mime handling.

Signed-off-by Mark Lord <[email protected]>

Came through OK this time, thanks.

Jeff

#12 lore raw

PATCH 1/1 sata_qstor: eh_timeout fix

In reply to Mark Lord

Here is an update to sata_qstor.c to enable full/proper register access during eh_timeout handling.

Patch is against 2.6.11-rc5 + earlier sata_qstor cosmetic patch.

Signed-off-by Mark Lord <[email protected]>
Signature · 3 lines
Mark Lord
Real-Time Remedies Inc.
[email protected]
drivers/scsi/sata_qstor.c +24 -6
@@ -38,7 +38,7 @@
 #include <linux/libata.h>
 
 #define DRV_NAME	"sata_qstor"
-#define DRV_VERSION	"0.03"
+#define DRV_VERSION	"0.04"
 
 enum {
 	QS_PORTS		= 4,

@@ -120,6 +120,7 @@
 static void qs_bmdma_stop(struct ata_port *ap);
 static u8 qs_bmdma_status(struct ata_port *ap);
 static void qs_irq_clear(struct ata_port *ap);
+static void qs_eng_timeout(struct ata_port *ap);
 
 static Scsi_Host_Template qs_ata_sht = {
 	.module			= THIS_MODULE,

@@ -152,7 +153,7 @@
 	.phy_reset		= qs_phy_reset,
 	.qc_prep		= qs_qc_prep,
 	.qc_issue		= qs_qc_issue,
-	.eng_timeout		= ata_eng_timeout,
+	.eng_timeout		= qs_eng_timeout,
 	.irq_handler		= qs_intr,
 	.irq_clear		= qs_irq_clear,
 	.scr_read		= qs_scr_read,

@@ -212,7 +213,7 @@
 	/* nothing */
 }
 
-static void qs_enter_reg_mode(struct ata_port *ap)
+static inline void qs_enter_reg_mode(struct ata_port *ap)
 {
 	u8 __iomem *chan = ap->host_set->mmio_base + (ap->port_no * 0x4000);
 

@@ -220,17 +221,34 @@
 	readb(chan + QS_CCT_CTR0);        /* flush */
 }
 
-static void qs_phy_reset(struct ata_port *ap)
+static inline void qs_reset_channel_logic(struct ata_port *ap)
 {
 	u8 __iomem *chan = ap->host_set->mmio_base + (ap->port_no * 0x4000);
-	struct qs_port_priv *pp = ap->private_data;
 
-	pp->state = qs_state_idle;
 	writeb(QS_CTR1_RCHN, chan + QS_CCT_CTR1);
+	readb(chan + QS_CCT_CTR0);        /* flush */
 	qs_enter_reg_mode(ap);
+}
+
+static void qs_phy_reset(struct ata_port *ap)
+{
+	struct qs_port_priv *pp = ap->private_data;
+
+	pp->state = qs_state_idle;
+	qs_reset_channel_logic(ap);
 	sata_phy_reset(ap);
 }
 
+static void qs_eng_timeout(struct ata_port *ap)
+{
+	struct qs_port_priv *pp = ap->private_data;
+
+	if (pp->state != qs_state_idle) /* healthy paranoia */
+		pp->state = qs_state_mmio;
+	qs_reset_channel_logic(ap);
+	ata_eng_timeout(ap);
+}
+
 static u32 qs_scr_read (struct ata_port *ap, unsigned int sc_reg)
 {
 	if (sc_reg > SCR_CONTROL)