Skip to contentSkip to conversation

libata: implement new initialization model w/ iomap support, take 2

PATCH In review Tejun Heo 3 participants 50 messages Aug 19, 2006 – Sep 19, 2006

Subsystems (2)
lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/1 libata: implement new initialization model w/ iomap support, take 2

Hello, all.

This is the second take of new-init-model patchset. This patchset implements new init model for libata LLDs. Changes from the last take[L] are...

* Adapted to drivers/ata relocation, SAS update and pending grand renaming.

* legacy / pci helpers made modular and lives in their own file libata-legacy.c and libata-pci.c respectively.

* IRQ helpers made modular.

* MSIX IRQ helpers added.

* Static legacy ports table removed as Alan pointed out.

Legacy handling is moularized into libata-legacy.c and PCI helpers use the module. If other bus helpers wanna use it, it just has to select CONFIG_ATA_LEGACY and call helpers as PCI does.

Instead of subclassing ata_host and make ata_legacy_host and/or ata_pci_host, I simply added two needed fields - legacy_flags and pci_flags. Rationale: that's all that's needed && with further modularization, we'll probably need some form of multiple-inheritence which can't be done with C-style wrapping-up subclassing. IMHO, waiting and seeing how things develop and updating basic data structure organization accordingly is the way to go. So, the minimal approach on this patchset.

Now that it's merged after such a long wait, I really didn't want to break SAS support and kept probe_ent and ata_port_init() for SAS helpers, so there's a bit of duality there ATM.

Brian, can you please take a look at how LLDs use new init functions and see if SAS can use the same approach? If you pass NULL as @sht to ata_host_alloc(), it won't associate ports as part of Scsi_Host just as ata_sas_port_alloc() does and ata_host_free() will do the right thing when freeing a host allocated that way.

sata_sil24.c is a pretty straight-forward example. If you can't determine the number of ports when allocating host, please take a look at how ahci.c initializes its host.

The intention was to allow SAS to use all the regular init/deinit functions just as other LLDs. If something doesn't seem to be right, please let me know.

Jeff, I privately did PATA support on sata_promise.c and it looks good and works well (tested with PDC20375). If this patchset gets ACKed, I'll post that patch against sata_promise.c.

The new init model has the following benefits.

* Consistency across different LLDs

Provide helpers such that common codes live in one place and init sequence doesn't have to be creative.

* Flexibility and easy code sharing with suspend/resume

Instead of using probe_ent, allocate host_set and initialize it directly. This removes many restrictions of probe_ent and allows LLDs to share init/remove codes with suspend/resume easily as they use the same target data structure.

IRQ management is also moved out of libata-core into libata-pci helpers which LLDs can use or override at will.

* More bookkeeping in libata-core, less in LLDDs

Acquired resources are tracked by libata-core and helpers. e.g. LLD doesn't have to remember which resources it has acquired, it can simply tell libata to release all resources on error path and driver detachments. This simplies LLDs and fixes several dangling resource and double-free bugs.

* Support for iomap

As with all other PCI resources, both PIO and mmio areas are managed by libata PCI helpers. iomaps are automatically created on IO resource acqusition and destroyed on release. LLDs can simply request which BARs it intends to use. The rest is handled by libata PCI helpers.

This patchset has been tested on

* ata_piix : ICH7R, most combinations and w/ induced resource
allocation errors
* ahci : ICH7R
* sata_sil : sil3112, sil3114
* sata_sil24 : sil3124, sil3132
* sata_via : vt6420 (via C3)
* pdc_adma : as broken as previous
* sata_promise : PDC20375, tested w/ PATA port too (PATA bits not
included in this post)

If you have any controller which use one of the following drivers, please test and report.

* sata_svw
* sata_sx4
* sata_uli
* sata_vsc
* sata_sis
* sata_qstor
* sata_nv (I can do this later)
* sata_mv

If this patchset is agreed upon, I'll update #pata-drivers accordingly.

This patchset is against

[U] upstream
[1] + grand-renaming patch
[2] + pci-quirks-CONFIG_ATA-fixup patch

This patchset is avaliable in the following git tree.

http://htj.dyndns.org/git/?p=libata-tj.git;a=shortlog;h=new-init
git://htj.dyndns.org/libata-tj new-init

Thanks.

--
tejun

[L] http://article.gmane.org/gmane.linux.ide/12437
[U] 421b20b9c44b3fcdc07d6af875dad5ae86fad014
[1] http://htj.dyndns.org/git/?p=libata-tj.git;a=commit;h=a8e3784a8a479e3cf82a17cb25419b05f916d53f
[2] http://article.gmane.org/gmane.linux.ide/12733

author
#1 lore raw

PATCH 1/20 libata: kill ata_host_stop()

As all the current LLDs are PCI, ata_pci_host_stop() should be used, which BTW is effectively identical to ata_host_stop(). Convert all LLDs to use ata_pci_host_stop() and kill ata_host_stop().

---
 drivers/ata/ata_piix.c    |    2 +-
 drivers/ata/libata-core.c |    7 -------
 drivers/ata/sata_mv.c     |    2 +-
 drivers/ata/sata_sis.c    |    2 +-
 drivers/ata/sata_uli.c    |    2 +-
 drivers/ata/sata_via.c    |    2 +-
 include/linux/libata.h    |    1 -
 7 files changed, 5 insertions(+), 13 deletions(-)
drivers/ata/ata_piix.c +1 -1
index a44c431..0cb48a4 100644
@@ -934,7 +934,7 @@ static void piix_host_stop(struct ata_ho
 {
 	struct piix_host_priv *hpriv = host->private_data;
 
-	ata_host_stop(host);
+	ata_pci_host_stop(host);
 
 	kfree(hpriv);
 }
drivers/ata/libata-core.c +0 -7
index 1c93154..e8bbbaa 100644
@@ -5207,12 +5207,6 @@ void ata_port_stop (struct ata_port *ap)
 	ata_pad_free(ap, dev);
 }
 
-void ata_host_stop (struct ata_host *host)
-{
-	if (host->mmio_base)
-		iounmap(host->mmio_base);
-}
-
 /**
  *	ata_dev_init - Initialize an ata_device structure
  *	@dev: Device structure to initialize
@@ -6056,7 +6050,6 @@ EXPORT_SYMBOL_GPL(ata_altstatus);
 EXPORT_SYMBOL_GPL(ata_exec_command);
 EXPORT_SYMBOL_GPL(ata_port_start);
 EXPORT_SYMBOL_GPL(ata_port_stop);
-EXPORT_SYMBOL_GPL(ata_host_stop);
 EXPORT_SYMBOL_GPL(ata_interrupt);
 EXPORT_SYMBOL_GPL(ata_mmio_data_xfer);
 EXPORT_SYMBOL_GPL(ata_pio_data_xfer);
drivers/ata/sata_mv.c +1 -1
index 34f1939..92ee209 100644
@@ -828,7 +828,7 @@ static void mv_host_stop(struct ata_host
 		pci_intx(pdev, 0);
 	}
 	kfree(hpriv);
-	ata_host_stop(host);
+	ata_pci_host_stop(host);
 }
 
 static inline void mv_priv_free(struct mv_port_priv *pp, struct device *dev)
drivers/ata/sata_sis.c +1 -1
index 9b17375..167f9b6 100644
@@ -123,7 +123,7 @@ static const struct ata_port_operations 
 	.scr_write		= sis_scr_write,
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= ata_host_stop,
+	.host_stop		= ata_pci_host_stop,
 };
 
 static struct ata_port_info sis_port_info = {
drivers/ata/sata_uli.c +1 -1
index 8fc6e80..227f983 100644
@@ -123,7 +123,7 @@ static const struct ata_port_operations 
 
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= ata_host_stop,
+	.host_stop		= ata_pci_host_stop,
 };
 
 static struct ata_port_info uli_port_info = {
drivers/ata/sata_via.c +1 -1
index cd77f03..f9ec293 100644
@@ -138,7 +138,7 @@ static const struct ata_port_operations 
 
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= ata_host_stop,
+	.host_stop		= ata_pci_host_stop,
 };
 
 static struct ata_port_info svia_port_info = {
include/linux/libata.h +0 -1
index 563885c..8fd8851 100644
@@ -741,7 +741,6 @@ extern u8 ata_altstatus(struct ata_port 
 extern void ata_exec_command(struct ata_port *ap, const struct ata_taskfile *tf);
 extern int ata_port_start (struct ata_port *ap);
 extern void ata_port_stop (struct ata_port *ap);
-extern void ata_host_stop (struct ata_host *host);
 extern irqreturn_t ata_interrupt (int irq, void *dev_instance, struct pt_regs *regs);
 extern void ata_mmio_data_xfer(struct ata_device *adev, unsigned char *buf,
 			       unsigned int buflen, int write_data);
Signature · 1 line
1.4.1.1
#2 lore raw

Re: [PATCH 1/20] libata: kill ata_host_stop()

Quoted 5 lines · Tejun Heo

As all the current LLDs are PCI, ata_pci_host_stop() should be used, which BTW is effectively identical to ata_host_stop(). Convert all LLDs to use ata_pci_host_stop() and kill ata_host_stop().

This patch is broken because now several drivers which do not do

mmio_base = pci_iomap()

now unconditionally call pci_iounmap().

Jeff

author
#3 lore raw

Re: [PATCH 1/20] libata: kill ata_host_stop()

Quoted 5 lines · Jeff Garzik
Quoted 4 lines · Tejun Heo

As all the current LLDs are PCI, ata_pci_host_stop() should be used, which BTW is effectively identical to ata_host_stop(). Convert all LLDs to use ata_pci_host_stop() and kill ata_host_stop().

Quoted 7 lines
Quoted 1 line

This patch is broken because now several drivers which do not do

mmio_base = pci_iomap()

now unconditionally call pci_iounmap().

I thought pci_iounmap() allowed NULL input. i386 and x86_64 look safe but NULL arg doesn't seem to be supported explicitly. I'll add 'if (host->mmio_base)' to ata_pci_host_stop().

Thanks.

Signature · 1 line
tejun
#4 lore raw

Re: [PATCH 1/20] libata: kill ata_host_stop()

In reply to Tejun Heo
Quoted 3 lines · Tejun Heo

As all the current LLDs are PCI, ata_pci_host_stop() should be used, which BTW is effectively identical to ata_host_stop(). Convert all LLDs to use ata_pci_host_stop() and kill ata_host_stop().

NAK, they are not equivalent replacements:

* ata_host_stop checks mmio_base before calling iounmap. Most of the drivers you change do not use ioremap (thus it's really a no-op call for them)

* ata_pci_host_stop not only unmaps unconditionally, it calls pci_iounmap(). Only drivers which have previously used pci_iomap() should use this helper.

This patch may make sense later in the patch series, when ata_pci_host_stop presumably works for all iomap-converted drivers, but not really as the first patch?

author
#5 lore raw

Re: [PATCH 1/20] libata: kill ata_host_stop()

Quoted 5 lines · Jeff Garzik
Quoted 3 lines · Tejun Heo

As all the current LLDs are PCI, ata_pci_host_stop() should be used, which BTW is effectively identical to ata_host_stop(). Convert all LLDs to use ata_pci_host_stop() and kill ata_host_stop().

Quoted 7 lines

NAK, they are not equivalent replacements:

* ata_host_stop checks mmio_base before calling iounmap. Most of the drivers you change do not use ioremap (thus it's really a no-op call for them)

* ata_pci_host_stop not only unmaps unconditionally, it calls pci_iounmap(). Only drivers which have previously used pci_iomap() should use this helper.

This patch may make sense later in the patch series, when ata_pci_host_stop presumably works for all iomap-converted drivers, but not really as the first patch?

It's the first patch because I wanted to free the function name ata_host_stop(), which will be used for another purpose in new init model. As the new use isn't a port op, there shouldn't be too much confusion.

Hmmm.. I'll push this backward and use different name for it.

Thanks.

Signature · 1 line
tejun
author
#6 lore raw

PATCH 2/20 libata: implement ata_host_start/stop()

In reply to Tejun Heo

Implement [de-]init helpers ata_host_start/stop(). start() will start all ports in the host while stop() does the opposite. These helpers use ATA_PFLAG_STARTED to track whether a port is started or not and thus can be called without side effect.

ata_host_stop() replaces half of ata_scsi_release() and the other half is merged into ata_host_detach().

This patch introduces the follwing behavior changes.

* ->port_start/stop() can be omitted. This will be used to simplify port initialization.

* ->port_disable() callback is reordered w.r.t. freeing IRQs. The callback's roll and usefulness are questionable. All LLDs use ata_port_disable() and libata-core directly calls ata_port_disable() in several places. ->port_disable() invocation during port deinit doesn't provide any feature. All are already done by EH. The relocation doesn't cause any real behavior change.

This is a part of efforts to improve [de-]init paths and simplify LLDs.

---
 drivers/ata/ahci.c        |    9 +--
 drivers/ata/libata-core.c |  129 +++++++++++++++++++++++++++++----------------
 include/linux/libata.h    |    5 +-
 3 files changed, 91 insertions(+), 52 deletions(-)
drivers/ata/ahci.c +3 -6
index 3f1106f..eb6ceb3 100644
@@ -1643,13 +1643,10 @@ static void ahci_remove_one (struct pci_
 	have_msi = hpriv->flags & AHCI_FLAG_MSI;
 	free_irq(host->irq, host);
 
-	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap = host->ports[i];
-
-		ata_scsi_release(ap->scsi_host);
-		scsi_host_put(ap->scsi_host);
-	}
+	ata_host_stop(host);
 
+	for (i = 0; i < host->n_ports; i++)
+		scsi_host_put(host->ports[i]->scsi_host);
 	kfree(hpriv);
 	pci_iounmap(pdev, host->mmio_base);
 	kfree(host);
drivers/ata/libata-core.c +84 -45

189 lines not on this page · raw message

include/linux/libata.h +4 -1
index 8fd8851..e098d32 100644
@@ -184,6 +184,8 @@ enum {
 	ATA_PFLAG_SUSPENDED	= (1 << 17), /* port is suspended (power) */
 	ATA_PFLAG_PM_PENDING	= (1 << 18), /* PM operation pending */
 
+	ATA_PFLAG_STARTED	= (1 << 24), /* port has been started */
+
 	/* struct ata_queued_cmd flags */
 	ATA_QCFLAG_ACTIVE	= (1 << 0), /* cmd not yet ack'd to scsi lyer */
 	ATA_QCFLAG_SG		= (1 << 1), /* have s/g table? */
@@ -688,15 +690,16 @@ extern int ata_pci_device_suspend(struct
 extern int ata_pci_device_resume(struct pci_dev *pdev);
 extern int ata_pci_clear_simplex(struct pci_dev *pdev);
 #endif /* CONFIG_PCI */
+extern int ata_host_start(struct ata_host *host);
 extern int ata_device_add(const struct ata_probe_ent *ent);
 extern void ata_port_detach(struct ata_port *ap);
+extern void ata_host_stop(struct ata_host *host);
 extern void ata_host_init(struct ata_host *, struct device *,
 			  unsigned long, const struct ata_port_operations *);
 extern void ata_host_remove(struct ata_host *host);
 extern int ata_scsi_detect(struct scsi_host_template *sht);
 extern int ata_scsi_ioctl(struct scsi_device *dev, int cmd, void __user *arg);
 extern int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *));
-extern int ata_scsi_release(struct Scsi_Host *host);
 extern void ata_sas_port_destroy(struct ata_port *);
 extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
 					   struct ata_port_info *, struct Scsi_Host *);
Signature · 1 line
1.4.1.1
author
#7 lore raw

PATCH 3/20 libata: implement ata_host_detach() and ata_host_free()

In reply to Tejun Heo

Implement and use ata_host_detach() and ata_host_free(). ata_host_detach() detaches all ports in the host and supercedes ata_port_detach(). ata_host_free() makes sure all ports are stopped (LLDs may stop ports beforehand if necessary), frees all ports and the host itself. ata_host_free() can also be used in the error handling path of init functions.

This patch moves several memory deallocations but doesn't introduce any real behavior changes.

This is a part of efforts to improve [de-]init paths and simplify LLDs.

---
 drivers/ata/ahci.c        |   13 +++------
 drivers/ata/libata-core.c |   65 +++++++++++++++++++++++++++++++++++++--------
 include/linux/libata.h    |    3 +-
 3 files changed, 60 insertions(+), 21 deletions(-)
drivers/ata/ahci.c +5 -8
index eb6ceb3..08ecc69 100644
@@ -1634,30 +1634,27 @@ static void ahci_remove_one (struct pci_
 	struct device *dev = pci_dev_to_dev(pdev);
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct ahci_host_priv *hpriv = host->private_data;
-	unsigned int i;
 	int have_msi;
 
-	for (i = 0; i < host->n_ports; i++)
-		ata_port_detach(host->ports[i]);
+	ata_host_detach(host);
 
 	have_msi = hpriv->flags & AHCI_FLAG_MSI;
 	free_irq(host->irq, host);
 
 	ata_host_stop(host);
-
-	for (i = 0; i < host->n_ports; i++)
-		scsi_host_put(host->ports[i]->scsi_host);
-	kfree(hpriv);
 	pci_iounmap(pdev, host->mmio_base);
-	kfree(host);
 
 	if (have_msi)
 		pci_disable_msi(pdev);
 	else
 		pci_intx(pdev, 0);
+
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
 	dev_set_drvdata(dev, NULL);
+
+	ata_host_free(host);
+	kfree(hpriv);
 }
 
 static int __init ahci_init(void)
drivers/ata/libata-core.c +53 -12

117 lines not on this page · raw message

include/linux/libata.h +2 -1
index e098d32..5263e6b 100644
@@ -692,8 +692,9 @@ extern int ata_pci_clear_simplex(struct 
 #endif /* CONFIG_PCI */
 extern int ata_host_start(struct ata_host *host);
 extern int ata_device_add(const struct ata_probe_ent *ent);
-extern void ata_port_detach(struct ata_port *ap);
+extern void ata_host_detach(struct ata_host *host);
 extern void ata_host_stop(struct ata_host *host);
+extern void ata_host_free(struct ata_host *host);
 extern void ata_host_init(struct ata_host *, struct device *,
 			  unsigned long, const struct ata_port_operations *);
 extern void ata_host_remove(struct ata_host *host);
Signature · 1 line
1.4.1.1
#8 lore raw

Re: [PATCH 3/20] libata: implement ata_host_detach() and ata_host_free()

ACK patches 2-3

author
#9 lore raw

PATCH 4/20 libata: separate out ata_host_alloc() and ata_host_attach()

In reply to Tejun Heo

Separate out ata_host_alloc(), ata_host_add_ports() and ata_host_attach() from ata_device_add(). These functions will be used by new LLD init model where LLD allocates host directly, then intializes and attaches it instead of going through probe_ent.

ata_host_alloc() can be called with NULL @sht which indicates that Scsi_Host association is handled by the caller. This will be used by SAS.

This patch does not introduce behavior changes.

---
 drivers/ata/libata-core.c |  451 ++++++++++++++++++++++++++++++---------------
 include/linux/libata.h    |    7 +
 2 files changed, 312 insertions(+), 146 deletions(-)
drivers/ata/libata-core.c +305 -146
index d7bf4a1..eeb9dc6 100644
@@ -5243,11 +5243,15 @@ void ata_dev_init(struct ata_device *dev
  *	ata_port_init - Initialize an ata_port structure
  *	@ap: Structure to initialize
  *	@host: Collection of hosts to which @ap belongs
- *	@ent: Probe information provided by low-level driver
- *	@port_no: Port number associated with this ata_port
+ *	@ent: Probe information provided by low-level driver (optional)
+ *	@port_no: Port number associated with this ata_port (optional)
  *
  *	Initialize a new ata_port structure.
  *
+ *	@ent and @port_no are to support sas interface.  These will be
+ *	removed once sas is converted to use new alloc/init/attach
+ *	interface.
+ *
  *	LOCKING:
  *	Inherited from caller.
  */
@@ -5261,13 +5265,17 @@ void ata_port_init(struct ata_port *ap, 
 	ap->id = ata_unique_id++;
 	ap->ctl = ATA_DEVCTL_OBS;
 	ap->host = host;
-	ap->dev = ent->dev;
-	ap->port_no = port_no;
-	ap->pio_mask = ent->pio_mask;
-	ap->mwdma_mask = ent->mwdma_mask;
-	ap->udma_mask = ent->udma_mask;
-	ap->flags |= ent->port_flags;
-	ap->ops = ent->port_ops;
+	ap->dev = host->dev;
+
+	if (ent) {
+		ap->port_no = port_no;
+		ap->pio_mask = ent->pio_mask;
+		ap->mwdma_mask = ent->mwdma_mask;
+		ap->udma_mask = ent->udma_mask;
+		ap->flags |= ent->port_flags;
+		ap->ops = ent->port_ops;
+	}
+
 	ap->hw_sata_spd_limit = UINT_MAX;
 	ap->active_tag = ATA_TAG_POISON;
 	ap->last_ctl = 0xFF;
@@ -5287,10 +5295,7 @@ #endif
 	INIT_LIST_HEAD(&ap->eh_done_q);
 	init_waitqueue_head(&ap->eh_wait_q);
 
-	/* set cable type */
 	ap->cbl = ATA_CBL_NONE;
-	if (ap->flags & ATA_FLAG_SATA)
-		ap->cbl = ATA_CBL_SATA;
 
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *dev = &ap->device[i];
@@ -5304,7 +5309,12 @@ #ifdef ATA_IRQ_TRAP
 	ap->stats.idle_irq = 1;
 #endif
 
-	memcpy(&ap->ioaddr, &ent->port[port_no], sizeof(struct ata_ioports));
+	if (ent) {
+		if (ap->flags & ATA_FLAG_SATA)
+			ap->cbl = ATA_CBL_SATA;
+		memcpy(&ap->ioaddr, &ent->port[port_no],
+		       sizeof(struct ata_ioports));
+	}
 }
 
 /**
@@ -5329,47 +5339,140 @@ static void ata_port_init_shost(struct a
 }
 
 /**
- *	ata_port_add - Attach low-level ATA driver to system
- *	@ent: Information provided by low-level driver
- *	@host: Collections of ports to which we add
- *	@port_no: Port number associated with this host
+ *	ata_port_alloc - allocate and initialize basic ATA port resources
+ *	@host: ATA host this allocated port belongs to
+ *	@sht: template for SCSI host (can be NULL)
  *
- *	Attach low-level ATA driver to system.
+ *	Allocate and initialize basic ATA port resources.
  *
- *	LOCKING:
- *	PCI/etc. bus probe sem.
+ *	If @sht is NULL, SCSI host is not allocated nor freed later
+ *	with the port.  The caller is responsible for associating SCSI
+ *	host.  This is for SAS.
  *
  *	RETURNS:
- *	New ata_port on success, for NULL on error.
+ *	Allocate ATA port on success, NULL on failure.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
  */
-static struct ata_port * ata_port_add(const struct ata_probe_ent *ent,
-				      struct ata_host *host,
-				      unsigned int port_no)
+static struct ata_port *ata_port_alloc(struct ata_host *host,
+				       struct scsi_host_template *sht)
 {
-	struct Scsi_Host *shost;
 	struct ata_port *ap;
 
 	DPRINTK("ENTER\n");
 
-	if (!ent->port_ops->error_handler &&
-	    !(ent->port_flags & (ATA_FLAG_SATA_RESET | ATA_FLAG_SRST))) {
-		printk(KERN_ERR "ata%u: no reset mechanism available\n",
-		       port_no);
-		return NULL;
+	if (sht) {
+		struct Scsi_Host *shost;
+
+		shost = scsi_host_alloc(sht, sizeof(struct ata_port));
+		if (!shost)
+			return NULL;
+
+		shost->transportt = &ata_scsi_transport_template;
+
+		ap = ata_shost_to_port(shost);
+		ata_port_init_shost(ap, shost);
+
+		ap->pflags |= ATA_PFLAG_SHOST_OWNER;
+	} else {
+		ap = kzalloc(sizeof(*ap), GFP_KERNEL);
+		if (!ap)
+			return NULL;
 	}
 
-	shost = scsi_host_alloc(ent->sht, sizeof(struct ata_port));
-	if (!shost)
+	ata_port_init(ap, host, NULL, 0);
+
+	return ap;
+}
+
+/**
+ *	ata_host_alloc - allocate and init basic ATA host resources
+ *	@dev: generic device this host is associated with
+ *	@sht: template for SCSI host
+ *	@n_ports: number of ATA ports associated with this host (can be 0)
+ *
+ *	Allocate and initialize basic ATA host resources.  LLD calls
+ *	this function to allocate a host, initializes it fully and
+ *	attaches it using ata_host_attach().
+ *
+ *	RETURNS:
+ *	Allocate ATA host on success, NULL on failure.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+struct ata_host *ata_host_alloc(struct device *dev,
+				struct scsi_host_template *sht, int n_ports)
+{
+	struct ata_host *host;
+	size_t sz;
+
+	DPRINTK("ENTER\n");
+
+	/* alloc a container for our list of ATA ports (buses) */
+	sz = sizeof(struct ata_host) + n_ports * sizeof(void *);
+	if (n_ports == 0)
+		sz += ATA_MAX_PORTS * sizeof(void *);
+
+	host = kzalloc(sz, GFP_KERNEL);
+	if (!host)
 		return NULL;
 
-	shost->transportt = &ata_scsi_transport_template;
+	spin_lock_init(&host->lock);
+	host->dev = dev;
 
-	ap = ata_shost_to_port(shost);
+	if (ata_host_add_ports(host, sht, n_ports))
+		goto err_free;
 
-	ata_port_init(ap, host, ent, port_no);
-	ata_port_init_shost(ap, shost);
+	return host;
 
-	return ap;
+ err_free:
+	ata_host_free(host);
+	return NULL;
+}
+
+/**
+ *	ata_host_add_ports - allocate ports to zero-port host
+ *	@host: target ATA host
+ *	@sht: template for SCSI host
+ *	@n_ports: number of ATA ports associated with this host
+ *
+ *	Allocate @n_ports ports and associate them with @host.
+ *	@n_ports must be less than or equal to ATA_MAX_PORTS and @host
+ *	must have been allocated with zero port.  This function is
+ *	used by LLDs which can't determine number of ports at host
+ *	allocation time.
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+int ata_host_add_ports(struct ata_host *host,
+		       struct scsi_host_template *sht, int n_ports)
+{
+	int i;
+
+	if (host->n_ports || n_ports > ATA_MAX_PORTS)
+		return -EINVAL;
+
+	/* allocate ports bound to this host */
+	for (i = 0; i < n_ports; i++) {
+		struct ata_port *ap;
+
+		ap = ata_port_alloc(host, sht);
+		if (!ap)
+			return -ENOMEM;
+
+		ap->port_no = i;
+		host->ports[i] = ap;
+	}
+
+	host->n_ports = n_ports;
+
+	return 0;
 }
 
 /**
@@ -5401,6 +5504,9 @@ void ata_host_init(struct ata_host *host
  *	ap->pflags, so this function can be called multiple times.
  *	Ports are guaranteed to get started only once.
  *
+ *	This function also initializes ap->cbl according to
+ *	ATA_FLAG_SATA if cable type isn't set yet.
+ *
  *	LOCKING:
  *	Inherited from calling layer (may sleep).
  *
@@ -5414,6 +5520,11 @@ int ata_host_start(struct ata_host *host
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 
+		/* set cable type */
+		if (ap->cbl == ATA_CBL_NONE && (ap->flags & ATA_FLAG_SATA))
+			ap->cbl = ATA_CBL_SATA;
+
+		/* start ports */
 		if (ap->pflags & ATA_PFLAG_STARTED)
 			continue;
 
@@ -5430,6 +5541,126 @@ int ata_host_start(struct ata_host *host
 }
 
 /**
+ *	ata_host_attach - attach initialized ATA host
+ *	@host: ATA host to attach
+ *
+ *	Attach initialized ATA host.  @host is allocated using
+ *	ata_host_alloc() and fully initialized by LLD.  This function
+ *	registers @host with ATA and SCSI layers and probe attached
+ *	devices.
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+int ata_host_attach(struct ata_host *host)
+{
+	int i, rc;
+
+	/* make sure ports are started */
+	rc = ata_host_start(host);
+	if (rc)
+		return rc;
+
+	/* perform each probe synchronously */
+	DPRINTK("probe begin\n");
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+		int irq_line = host->irq;
+		unsigned long xfer_mode_mask;
+		u32 scontrol;
+		int rc;
+
+		/* report the secondary IRQ for second channel legacy */
+		if (i == 1 && host->irq2)
+			irq_line = host->irq2;
+
+		xfer_mode_mask = (ap->udma_mask << ATA_SHIFT_UDMA) |
+				 (ap->mwdma_mask << ATA_SHIFT_MWDMA) |
+				 (ap->pio_mask << ATA_SHIFT_PIO);
+
+		/* print per-port info to dmesg */
+		if (ap->ops != &ata_dummy_port_ops)
+			ata_port_printk(ap, KERN_INFO, "%cATA max %s cmd 0x%lu "
+					"ctl 0x%lX bmdma 0x%lX irq %d\n",
+					ap->flags & ATA_FLAG_SATA ? 'S' : 'P',
+					ata_mode_string(xfer_mode_mask),
+					ap->ioaddr.cmd_addr,
+					ap->ioaddr.ctl_addr,
+					ap->ioaddr.bmdma_addr, irq_line);
+		else
+			ata_port_printk(ap, KERN_INFO, "DUMMY port\n");
+
+		/* init sata_spd_limit to the current value */
+		if (sata_scr_read(ap, SCR_CONTROL, &scontrol) == 0) {
+			int spd = (scontrol >> 4) & 0xf;
+			ap->hw_sata_spd_limit &= (1 << spd) - 1;
+		}
+		ap->sata_spd_limit = ap->hw_sata_spd_limit;
+
+		rc = scsi_add_host(ap->scsi_host, host->dev);
+		if (rc) {
+			ata_port_printk(ap, KERN_ERR, "scsi_add_host failed\n");
+			/* FIXME: do something useful here */
+			/* FIXME: handle unconditional calls to
+			 * scsi_scan_host and ata_host_remove, below,
+			 * at the very least
+			 */
+		}
+
+		if (ap->ops->error_handler) {
+			struct ata_eh_info *ehi = &ap->eh_info;
+			unsigned long flags;
+
+			ata_port_probe(ap);
+
+			/* kick EH for boot probing */
+			spin_lock_irqsave(ap->lock, flags);
+
+			ehi->probe_mask = (1 << ATA_MAX_DEVICES) - 1;
+			ehi->action |= ATA_EH_SOFTRESET;
+			ehi->flags |= ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET;
+
+			ap->pflags |= ATA_PFLAG_LOADING;
+			ata_port_schedule_eh(ap);
+
+			spin_unlock_irqrestore(ap->lock, flags);
+
+			/* wait for EH to finish */
+			ata_port_wait_eh(ap);
+		} else {
+			DPRINTK("ata%u: bus probe begin\n", ap->id);
+			rc = ata_bus_probe(ap);
+			DPRINTK("ata%u: bus probe end\n", ap->id);
+
+			if (rc) {
+				/* FIXME: do something useful here?
+				 * Current libata behavior will
+				 * tear down everything when
+				 * the module is removed
+				 * or the h/w is unplugged.
+				 */
+			}
+		}
+	}
+
+	/* probes are done, now scan each port's disk(s) */
+	DPRINTK("host probe begin\n");
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+
+		ata_scsi_scan_host(ap);
+	}
+
+	dev_set_drvdata(host->dev, host);
+
+	return 0;
+}
+
+
+/**
  *	ata_device_add - Register hardware device with ATA and SCSI layers
  *	@ent: Probe information describing hardware device to be registered
  *
@@ -5455,61 +5686,48 @@ int ata_device_add(const struct ata_prob
 	int rc;
 
 	DPRINTK("ENTER\n");
-	/* alloc a container for our list of ATA ports (buses) */
-	host = kzalloc(sizeof(struct ata_host) +
-		       (ent->n_ports * sizeof(void *)), GFP_KERNEL);
+
+	if (!ent->port_ops->error_handler &&
+	    !(ent->port_flags & (ATA_FLAG_SATA_RESET | ATA_FLAG_SRST))) {
+		dev_printk(KERN_ERR, dev, "no reset mechanism available\n");
+		return 0;
+	}
+
+	/* allocate host */
+	host = ata_host_alloc(dev, ent->sht, ent->n_ports);
 	if (!host)
 		return 0;
 
-	ata_host_init(host, dev, ent->_host_flags, ent->port_ops);
-	host->n_ports = ent->n_ports;
 	host->irq = ent->irq;
 	host->irq2 = ent->irq2;
 	host->mmio_base = ent->mmio_base;
 	host->private_data = ent->private_data;
+	host->ops = ent->port_ops;
+	host->flags = ent->_host_flags;
 
-	/* register each port bound to this device */
 	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap;
-		unsigned long xfer_mode_mask;
-		int irq_line = ent->irq;
-
-		ap = ata_port_add(ent, host, i);
-		if (!ap)
-			goto err_out;
-
-		host->ports[i] = ap;
+		struct ata_port *ap = host->ports[i];
 
 		/* dummy? */
 		if (ent->dummy_port_mask & (1 << i)) {
-			ata_port_printk(ap, KERN_INFO, "DUMMY\n");
 			ap->ops = &ata_dummy_port_ops;
 			continue;
 		}
 
-		/* Report the secondary IRQ for second channel legacy */
-		if (i == 1 && ent->irq2)
-			irq_line = ent->irq2;
-
-		xfer_mode_mask =(ap->udma_mask << ATA_SHIFT_UDMA) |
-				(ap->mwdma_mask << ATA_SHIFT_MWDMA) |
-				(ap->pio_mask << ATA_SHIFT_PIO);
+		ap->pio_mask = ent->pio_mask;
+		ap->mwdma_mask = ent->mwdma_mask;
+		ap->udma_mask = ent->udma_mask;
+		ap->flags |= ent->port_flags;
+		ap->ops = ent->port_ops;
 
-		/* print per-port info to dmesg */
-		ata_port_printk(ap, KERN_INFO, "%cATA max %s cmd 0x%lX "
-				"ctl 0x%lX bmdma 0x%lX irq %d\n",
-				ap->flags & ATA_FLAG_SATA ? 'S' : 'P',
-				ata_mode_string(xfer_mode_mask),
-				ap->ioaddr.cmd_addr,
-				ap->ioaddr.ctl_addr,
-				ap->ioaddr.bmdma_addr,
-				irq_line);
+		memcpy(&ap->ioaddr, &ent->port[ap->port_no],
+		       sizeof(struct ata_ioports));
 	}
 
 	/* start ports */
 	rc = ata_host_start(host);
 	if (rc)
-		goto err_out;
+		goto err_free_host;
 
 	/* freeze */
 	for (i = 0; i < host->n_ports; i++) {
@@ -5526,7 +5744,7 @@ int ata_device_add(const struct ata_prob
 	if (rc) {
 		dev_printk(KERN_ERR, dev, "irq %lu request failed: %d\n",
 			   ent->irq, rc);
-		goto err_out;
+		goto err_free_host;
 	}
 
 	/* do we have a second IRQ for the other channel, eg legacy mode */
@@ -5540,86 +5758,22 @@ int ata_device_add(const struct ata_prob
 		if (rc) {
 			dev_printk(KERN_ERR, dev, "irq %lu request failed: %d\n",
 				   ent->irq2, rc);
-			goto err_out_free_irq;
+			goto err_free_irq;
 		}
 	}
 
-	/* perform each probe synchronously */
-	DPRINTK("probe begin\n");
-	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap = host->ports[i];
-		u32 scontrol;
-		int rc;
-
-		/* init sata_spd_limit to the current value */
-		if (sata_scr_read(ap, SCR_CONTROL, &scontrol) == 0) {
-			int spd = (scontrol >> 4) & 0xf;
-			ap->hw_sata_spd_limit &= (1 << spd) - 1;
-		}
-		ap->sata_spd_limit = ap->hw_sata_spd_limit;
-
-		rc = scsi_add_host(ap->scsi_host, dev);
-		if (rc) {
-			ata_port_printk(ap, KERN_ERR, "scsi_add_host failed\n");
-			/* FIXME: do something useful here */
-			/* FIXME: handle unconditional calls to
-			 * scsi_scan_host and ata_host_remove, below,
-			 * at the very least
-			 */
-		}
-
-		if (ap->ops->error_handler) {
-			struct ata_eh_info *ehi = &ap->eh_info;
-			unsigned long flags;
-
-			ata_port_probe(ap);
-
-			/* kick EH for boot probing */
-			spin_lock_irqsave(ap->lock, flags);
-
-			ehi->probe_mask = (1 << ATA_MAX_DEVICES) - 1;
-			ehi->action |= ATA_EH_SOFTRESET;
-			ehi->flags |= ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET;
-
-			ap->pflags |= ATA_PFLAG_LOADING;
-			ata_port_schedule_eh(ap);
-
-			spin_unlock_irqrestore(ap->lock, flags);
-
-			/* wait for EH to finish */
-			ata_port_wait_eh(ap);
-		} else {
-			DPRINTK("ata%u: bus probe begin\n", ap->id);
-			rc = ata_bus_probe(ap);
-			DPRINTK("ata%u: bus probe end\n", ap->id);
-
-			if (rc) {
-				/* FIXME: do something useful here?
-				 * Current libata behavior will
-				 * tear down everything when
-				 * the module is removed
-				 * or the h/w is unplugged.
-				 */
-			}
-		}
-	}
-
-	/* probes are done, now scan each port's disk(s) */
-	DPRINTK("host probe begin\n");
-	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap = host->ports[i];
-
-		ata_scsi_scan_host(ap);
-	}
-
-	dev_set_drvdata(dev, host);
+	rc = ata_host_attach(host);
+	if (rc)
+		goto err_free_irq2;
 
-	VPRINTK("EXIT, returning %u\n", ent->n_ports);
-	return ent->n_ports; /* success */
+	return host->n_ports; /* success */
 
-err_out_free_irq:
+ err_free_irq2:
+	if (ent->irq2)
+		free_irq(ent->irq2, host);
+ err_free_irq:
 	free_irq(ent->irq, host);
-err_out:
+ err_free_host:
 	ata_host_stop(host);
 	ata_host_free(host);
 	VPRINTK("EXIT, returning 0\n");
@@ -5755,8 +5909,10 @@ void ata_host_free(struct ata_host *host
 	/* free */
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
-		if (ap)
+		if (ap->pflags & ATA_PFLAG_SHOST_OWNER)
 			scsi_host_put(ap->scsi_host);
+		else
+			kfree(ap);
 	}
 
 	kfree(host);
@@ -6108,7 +6264,10 @@ EXPORT_SYMBOL_GPL(ata_dummy_port_ops);
 EXPORT_SYMBOL_GPL(ata_std_bios_param);
 EXPORT_SYMBOL_GPL(ata_std_ports);
 EXPORT_SYMBOL_GPL(ata_host_init);
+EXPORT_SYMBOL_GPL(ata_host_alloc);
+EXPORT_SYMBOL_GPL(ata_host_add_ports);
 EXPORT_SYMBOL_GPL(ata_host_start);
+EXPORT_SYMBOL_GPL(ata_host_attach);
 EXPORT_SYMBOL_GPL(ata_device_add);
 EXPORT_SYMBOL_GPL(ata_host_detach);
 EXPORT_SYMBOL_GPL(ata_host_stop);
include/linux/libata.h +7 -0
index 5263e6b..5e8160a 100644
@@ -185,6 +185,7 @@ enum {
 	ATA_PFLAG_PM_PENDING	= (1 << 18), /* PM operation pending */
 
 	ATA_PFLAG_STARTED	= (1 << 24), /* port has been started */
+	ATA_PFLAG_SHOST_OWNER	= (1 << 25), /* owner of ap->scsi_host */
 
 	/* struct ata_queued_cmd flags */
 	ATA_QCFLAG_ACTIVE	= (1 << 0), /* cmd not yet ack'd to scsi lyer */
@@ -690,7 +691,13 @@ extern int ata_pci_device_suspend(struct
 extern int ata_pci_device_resume(struct pci_dev *pdev);
 extern int ata_pci_clear_simplex(struct pci_dev *pdev);
 #endif /* CONFIG_PCI */
+extern struct ata_host *ata_host_alloc(struct device *dev,
+				       struct scsi_host_template *sht,
+				       int n_ports);
+extern int ata_host_add_ports(struct ata_host *host,
+			      struct scsi_host_template *sht, int n_ports);
 extern int ata_host_start(struct ata_host *host);
+extern int ata_host_attach(struct ata_host *host);
 extern int ata_device_add(const struct ata_probe_ent *ent);
 extern void ata_host_detach(struct ata_host *host);
 extern void ata_host_stop(struct ata_host *host);
Signature · 1 line
1.4.1.1
#10 lore raw

Re: [PATCH 4/20] libata: separate out ata_host_alloc() and ata_host_attach()

Quoted 5 lines · Tejun Heo

Separate out ata_host_alloc(), ata_host_add_ports() and ata_host_attach() from ata_device_add(). These functions will be used by new LLD init model where LLD allocates host directly, then intializes and attaches it instead of going through probe_ent.

Quoted 7 lines

ata_host_alloc() can be called with NULL @sht which indicates that Scsi_Host association is handled by the caller. This will be used by SAS.

This patch does not introduce behavior changes.

ACK in general, but minor nits are found in comments below.

Also, to make reviewing and git bisect easier, it would be nice to split ata_host_attach() separation into another patch.

Quoted 10 lines
---
 drivers/ata/libata-core.c |  451 ++++++++++++++++++++++++++++++---------------
 include/linux/libata.h    |    7 +
 2 files changed, 312 insertions(+), 146 deletions(-)
drivers/ata/libata-core.c +0 -0
index d7bf4a1..eeb9dc6 100644

drivers/ata/libata-core.c void ata_dev_init(struct ata_device *dev · line 5247

Quoted 7 lines
drivers/ata/libata-core.c +2 -2
@@ -5243,11 +5243,15 @@ void ata_dev_init(struct ata_device *dev
  *	ata_port_init - Initialize an ata_port structure
  *	@ap: Structure to initialize
  *	@host: Collection of hosts to which @ap belongs
- *	@ent: Probe information provided by low-level driver
- *	@port_no: Port number associated with this ata_port
+ *	@ent: Probe information provided by low-level driver (optional)
+ *	@port_no: Port number associated with this ata_port (optional)

I would perhaps note that port_no is require iff ent != NULL

Quoted 195 lines
drivers/ata/libata-core.c +112 -35
  *	Initialize a new ata_port structure.
  *
+ *	@ent and @port_no are to support sas interface.  These will be
+ *	removed once sas is converted to use new alloc/init/attach
+ *	interface.
+ *
  *	LOCKING:
  *	Inherited from caller.
  */
@@ -5261,13 +5265,17 @@ void ata_port_init(struct ata_port *ap, 
 	ap->id = ata_unique_id++;
 	ap->ctl = ATA_DEVCTL_OBS;
 	ap->host = host;
-	ap->dev = ent->dev;
-	ap->port_no = port_no;
-	ap->pio_mask = ent->pio_mask;
-	ap->mwdma_mask = ent->mwdma_mask;
-	ap->udma_mask = ent->udma_mask;
-	ap->flags |= ent->port_flags;
-	ap->ops = ent->port_ops;
+	ap->dev = host->dev;
+
+	if (ent) {
+		ap->port_no = port_no;
+		ap->pio_mask = ent->pio_mask;
+		ap->mwdma_mask = ent->mwdma_mask;
+		ap->udma_mask = ent->udma_mask;
+		ap->flags |= ent->port_flags;
+		ap->ops = ent->port_ops;
+	}
+
 	ap->hw_sata_spd_limit = UINT_MAX;
 	ap->active_tag = ATA_TAG_POISON;
 	ap->last_ctl = 0xFF;
@@ -5287,10 +5295,7 @@ #endif
 	INIT_LIST_HEAD(&ap->eh_done_q);
 	init_waitqueue_head(&ap->eh_wait_q);
 
-	/* set cable type */
 	ap->cbl = ATA_CBL_NONE;
-	if (ap->flags & ATA_FLAG_SATA)
-		ap->cbl = ATA_CBL_SATA;
 
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *dev = &ap->device[i];
@@ -5304,7 +5309,12 @@ #ifdef ATA_IRQ_TRAP
 	ap->stats.idle_irq = 1;
 #endif
 
-	memcpy(&ap->ioaddr, &ent->port[port_no], sizeof(struct ata_ioports));
+	if (ent) {
+		if (ap->flags & ATA_FLAG_SATA)
+			ap->cbl = ATA_CBL_SATA;
+		memcpy(&ap->ioaddr, &ent->port[port_no],
+		       sizeof(struct ata_ioports));
+	}
 }
 
 /**
@@ -5329,47 +5339,140 @@ static void ata_port_init_shost(struct a
 }
 
 /**
- *	ata_port_add - Attach low-level ATA driver to system
- *	@ent: Information provided by low-level driver
- *	@host: Collections of ports to which we add
- *	@port_no: Port number associated with this host
+ *	ata_port_alloc - allocate and initialize basic ATA port resources
+ *	@host: ATA host this allocated port belongs to
+ *	@sht: template for SCSI host (can be NULL)
  *
- *	Attach low-level ATA driver to system.
+ *	Allocate and initialize basic ATA port resources.
  *
- *	LOCKING:
- *	PCI/etc. bus probe sem.
+ *	If @sht is NULL, SCSI host is not allocated nor freed later
+ *	with the port.  The caller is responsible for associating SCSI
+ *	host.  This is for SAS.
  *
  *	RETURNS:
- *	New ata_port on success, for NULL on error.
+ *	Allocate ATA port on success, NULL on failure.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
  */
-static struct ata_port * ata_port_add(const struct ata_probe_ent *ent,
-				      struct ata_host *host,
-				      unsigned int port_no)
+static struct ata_port *ata_port_alloc(struct ata_host *host,
+				       struct scsi_host_template *sht)
 {
-	struct Scsi_Host *shost;
 	struct ata_port *ap;
 
 	DPRINTK("ENTER\n");
 
-	if (!ent->port_ops->error_handler &&
-	    !(ent->port_flags & (ATA_FLAG_SATA_RESET | ATA_FLAG_SRST))) {
-		printk(KERN_ERR "ata%u: no reset mechanism available\n",
-		       port_no);
-		return NULL;
+	if (sht) {
+		struct Scsi_Host *shost;
+
+		shost = scsi_host_alloc(sht, sizeof(struct ata_port));
+		if (!shost)
+			return NULL;
+
+		shost->transportt = &ata_scsi_transport_template;
+
+		ap = ata_shost_to_port(shost);
+		ata_port_init_shost(ap, shost);
+
+		ap->pflags |= ATA_PFLAG_SHOST_OWNER;
+	} else {
+		ap = kzalloc(sizeof(*ap), GFP_KERNEL);
+		if (!ap)
+			return NULL;
 	}
 
-	shost = scsi_host_alloc(ent->sht, sizeof(struct ata_port));
-	if (!shost)
+	ata_port_init(ap, host, NULL, 0);
+
+	return ap;
+}
+
+/**
+ *	ata_host_alloc - allocate and init basic ATA host resources
+ *	@dev: generic device this host is associated with
+ *	@sht: template for SCSI host
+ *	@n_ports: number of ATA ports associated with this host (can be 0)
+ *
+ *	Allocate and initialize basic ATA host resources.  LLD calls
+ *	this function to allocate a host, initializes it fully and
+ *	attaches it using ata_host_attach().
+ *
+ *	RETURNS:
+ *	Allocate ATA host on success, NULL on failure.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+struct ata_host *ata_host_alloc(struct device *dev,
+				struct scsi_host_template *sht, int n_ports)
+{
+	struct ata_host *host;
+	size_t sz;
+
+	DPRINTK("ENTER\n");
+
+	/* alloc a container for our list of ATA ports (buses) */
+	sz = sizeof(struct ata_host) + n_ports * sizeof(void *);
+	if (n_ports == 0)
+		sz += ATA_MAX_PORTS * sizeof(void *);
+
+	host = kzalloc(sz, GFP_KERNEL);
+	if (!host)
 		return NULL;
 
-	shost->transportt = &ata_scsi_transport_template;
+	spin_lock_init(&host->lock);
+	host->dev = dev;
 
-	ap = ata_shost_to_port(shost);
+	if (ata_host_add_ports(host, sht, n_ports))
+		goto err_free;
 
-	ata_port_init(ap, host, ent, port_no);
-	ata_port_init_shost(ap, shost);
+	return host;
 
-	return ap;
+ err_free:
+	ata_host_free(host);
+	return NULL;
+}
+
+/**
+ *	ata_host_add_ports - allocate ports to zero-port host
+ *	@host: target ATA host
+ *	@sht: template for SCSI host
+ *	@n_ports: number of ATA ports associated with this host
+ *
+ *	Allocate @n_ports ports and associate them with @host.
+ *	@n_ports must be less than or equal to ATA_MAX_PORTS and @host
+ *	must have been allocated with zero port.  This function is
+ *	used by LLDs which can't determine number of ports at host
+ *	allocation time.
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ *

drivers/ata/libata-core.c static void ata_port_init_shost(struct a · line 5459

Quoted 10 lines
drivers/ata/libata-core.c +10 -0
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+int ata_host_add_ports(struct ata_host *host,
+		       struct scsi_host_template *sht, int n_ports)
+{
+	int i;
+
+	if (host->n_ports || n_ports > ATA_MAX_PORTS)
+		return -EINVAL;

For what code path would host->n_ports be non-zero?

Quoted 356 lines
drivers/ata/libata-core.c +174 -108
+	/* allocate ports bound to this host */
+	for (i = 0; i < n_ports; i++) {
+		struct ata_port *ap;
+
+		ap = ata_port_alloc(host, sht);
+		if (!ap)
+			return -ENOMEM;
+
+		ap->port_no = i;
+		host->ports[i] = ap;
+	}
+
+	host->n_ports = n_ports;
+
+	return 0;
 }
 
 /**
@@ -5401,6 +5504,9 @@ void ata_host_init(struct ata_host *host
  *	ap->pflags, so this function can be called multiple times.
  *	Ports are guaranteed to get started only once.
  *
+ *	This function also initializes ap->cbl according to
+ *	ATA_FLAG_SATA if cable type isn't set yet.
+ *
  *	LOCKING:
  *	Inherited from calling layer (may sleep).
  *
@@ -5414,6 +5520,11 @@ int ata_host_start(struct ata_host *host
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 
+		/* set cable type */
+		if (ap->cbl == ATA_CBL_NONE && (ap->flags & ATA_FLAG_SATA))
+			ap->cbl = ATA_CBL_SATA;
+
+		/* start ports */
 		if (ap->pflags & ATA_PFLAG_STARTED)
 			continue;
 
@@ -5430,6 +5541,126 @@ int ata_host_start(struct ata_host *host
 }
 
 /**
+ *	ata_host_attach - attach initialized ATA host
+ *	@host: ATA host to attach
+ *
+ *	Attach initialized ATA host.  @host is allocated using
+ *	ata_host_alloc() and fully initialized by LLD.  This function
+ *	registers @host with ATA and SCSI layers and probe attached
+ *	devices.
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+int ata_host_attach(struct ata_host *host)
+{
+	int i, rc;
+
+	/* make sure ports are started */
+	rc = ata_host_start(host);
+	if (rc)
+		return rc;
+
+	/* perform each probe synchronously */
+	DPRINTK("probe begin\n");
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+		int irq_line = host->irq;
+		unsigned long xfer_mode_mask;
+		u32 scontrol;
+		int rc;
+
+		/* report the secondary IRQ for second channel legacy */
+		if (i == 1 && host->irq2)
+			irq_line = host->irq2;
+
+		xfer_mode_mask = (ap->udma_mask << ATA_SHIFT_UDMA) |
+				 (ap->mwdma_mask << ATA_SHIFT_MWDMA) |
+				 (ap->pio_mask << ATA_SHIFT_PIO);
+
+		/* print per-port info to dmesg */
+		if (ap->ops != &ata_dummy_port_ops)
+			ata_port_printk(ap, KERN_INFO, "%cATA max %s cmd 0x%lu "
+					"ctl 0x%lX bmdma 0x%lX irq %d\n",
+					ap->flags & ATA_FLAG_SATA ? 'S' : 'P',
+					ata_mode_string(xfer_mode_mask),
+					ap->ioaddr.cmd_addr,
+					ap->ioaddr.ctl_addr,
+					ap->ioaddr.bmdma_addr, irq_line);
+		else
+			ata_port_printk(ap, KERN_INFO, "DUMMY port\n");
+
+		/* init sata_spd_limit to the current value */
+		if (sata_scr_read(ap, SCR_CONTROL, &scontrol) == 0) {
+			int spd = (scontrol >> 4) & 0xf;
+			ap->hw_sata_spd_limit &= (1 << spd) - 1;
+		}
+		ap->sata_spd_limit = ap->hw_sata_spd_limit;
+
+		rc = scsi_add_host(ap->scsi_host, host->dev);
+		if (rc) {
+			ata_port_printk(ap, KERN_ERR, "scsi_add_host failed\n");
+			/* FIXME: do something useful here */
+			/* FIXME: handle unconditional calls to
+			 * scsi_scan_host and ata_host_remove, below,
+			 * at the very least
+			 */
+		}
+
+		if (ap->ops->error_handler) {
+			struct ata_eh_info *ehi = &ap->eh_info;
+			unsigned long flags;
+
+			ata_port_probe(ap);
+
+			/* kick EH for boot probing */
+			spin_lock_irqsave(ap->lock, flags);
+
+			ehi->probe_mask = (1 << ATA_MAX_DEVICES) - 1;
+			ehi->action |= ATA_EH_SOFTRESET;
+			ehi->flags |= ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET;
+
+			ap->pflags |= ATA_PFLAG_LOADING;
+			ata_port_schedule_eh(ap);
+
+			spin_unlock_irqrestore(ap->lock, flags);
+
+			/* wait for EH to finish */
+			ata_port_wait_eh(ap);
+		} else {
+			DPRINTK("ata%u: bus probe begin\n", ap->id);
+			rc = ata_bus_probe(ap);
+			DPRINTK("ata%u: bus probe end\n", ap->id);
+
+			if (rc) {
+				/* FIXME: do something useful here?
+				 * Current libata behavior will
+				 * tear down everything when
+				 * the module is removed
+				 * or the h/w is unplugged.
+				 */
+			}
+		}
+	}
+
+	/* probes are done, now scan each port's disk(s) */
+	DPRINTK("host probe begin\n");
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+
+		ata_scsi_scan_host(ap);
+	}
+
+	dev_set_drvdata(host->dev, host);
+
+	return 0;
+}
+
+
+/**
  *	ata_device_add - Register hardware device with ATA and SCSI layers
  *	@ent: Probe information describing hardware device to be registered
  *
@@ -5455,61 +5686,48 @@ int ata_device_add(const struct ata_prob
 	int rc;
 
 	DPRINTK("ENTER\n");
-	/* alloc a container for our list of ATA ports (buses) */
-	host = kzalloc(sizeof(struct ata_host) +
-		       (ent->n_ports * sizeof(void *)), GFP_KERNEL);
+
+	if (!ent->port_ops->error_handler &&
+	    !(ent->port_flags & (ATA_FLAG_SATA_RESET | ATA_FLAG_SRST))) {
+		dev_printk(KERN_ERR, dev, "no reset mechanism available\n");
+		return 0;
+	}
+
+	/* allocate host */
+	host = ata_host_alloc(dev, ent->sht, ent->n_ports);
 	if (!host)
 		return 0;
 
-	ata_host_init(host, dev, ent->_host_flags, ent->port_ops);
-	host->n_ports = ent->n_ports;
 	host->irq = ent->irq;
 	host->irq2 = ent->irq2;
 	host->mmio_base = ent->mmio_base;
 	host->private_data = ent->private_data;
+	host->ops = ent->port_ops;
+	host->flags = ent->_host_flags;
 
-	/* register each port bound to this device */
 	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap;
-		unsigned long xfer_mode_mask;
-		int irq_line = ent->irq;
-
-		ap = ata_port_add(ent, host, i);
-		if (!ap)
-			goto err_out;
-
-		host->ports[i] = ap;
+		struct ata_port *ap = host->ports[i];
 
 		/* dummy? */
 		if (ent->dummy_port_mask & (1 << i)) {
-			ata_port_printk(ap, KERN_INFO, "DUMMY\n");
 			ap->ops = &ata_dummy_port_ops;
 			continue;
 		}
 
-		/* Report the secondary IRQ for second channel legacy */
-		if (i == 1 && ent->irq2)
-			irq_line = ent->irq2;
-
-		xfer_mode_mask =(ap->udma_mask << ATA_SHIFT_UDMA) |
-				(ap->mwdma_mask << ATA_SHIFT_MWDMA) |
-				(ap->pio_mask << ATA_SHIFT_PIO);
+		ap->pio_mask = ent->pio_mask;
+		ap->mwdma_mask = ent->mwdma_mask;
+		ap->udma_mask = ent->udma_mask;
+		ap->flags |= ent->port_flags;
+		ap->ops = ent->port_ops;
 
-		/* print per-port info to dmesg */
-		ata_port_printk(ap, KERN_INFO, "%cATA max %s cmd 0x%lX "
-				"ctl 0x%lX bmdma 0x%lX irq %d\n",
-				ap->flags & ATA_FLAG_SATA ? 'S' : 'P',
-				ata_mode_string(xfer_mode_mask),
-				ap->ioaddr.cmd_addr,
-				ap->ioaddr.ctl_addr,
-				ap->ioaddr.bmdma_addr,
-				irq_line);
+		memcpy(&ap->ioaddr, &ent->port[ap->port_no],
+		       sizeof(struct ata_ioports));
 	}
 
 	/* start ports */
 	rc = ata_host_start(host);
 	if (rc)
-		goto err_out;
+		goto err_free_host;
 
 	/* freeze */
 	for (i = 0; i < host->n_ports; i++) {
@@ -5526,7 +5744,7 @@ int ata_device_add(const struct ata_prob
 	if (rc) {
 		dev_printk(KERN_ERR, dev, "irq %lu request failed: %d\n",
 			   ent->irq, rc);
-		goto err_out;
+		goto err_free_host;
 	}
 
 	/* do we have a second IRQ for the other channel, eg legacy mode */
@@ -5540,86 +5758,22 @@ int ata_device_add(const struct ata_prob
 		if (rc) {
 			dev_printk(KERN_ERR, dev, "irq %lu request failed: %d\n",
 				   ent->irq2, rc);
-			goto err_out_free_irq;
+			goto err_free_irq;
 		}
 	}
 
-	/* perform each probe synchronously */
-	DPRINTK("probe begin\n");
-	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap = host->ports[i];
-		u32 scontrol;
-		int rc;
-
-		/* init sata_spd_limit to the current value */
-		if (sata_scr_read(ap, SCR_CONTROL, &scontrol) == 0) {
-			int spd = (scontrol >> 4) & 0xf;
-			ap->hw_sata_spd_limit &= (1 << spd) - 1;
-		}
-		ap->sata_spd_limit = ap->hw_sata_spd_limit;
-
-		rc = scsi_add_host(ap->scsi_host, dev);
-		if (rc) {
-			ata_port_printk(ap, KERN_ERR, "scsi_add_host failed\n");
-			/* FIXME: do something useful here */
-			/* FIXME: handle unconditional calls to
-			 * scsi_scan_host and ata_host_remove, below,
-			 * at the very least
-			 */
-		}
-
-		if (ap->ops->error_handler) {
-			struct ata_eh_info *ehi = &ap->eh_info;
-			unsigned long flags;
-
-			ata_port_probe(ap);
-
-			/* kick EH for boot probing */
-			spin_lock_irqsave(ap->lock, flags);
-
-			ehi->probe_mask = (1 << ATA_MAX_DEVICES) - 1;
-			ehi->action |= ATA_EH_SOFTRESET;
-			ehi->flags |= ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET;
-
-			ap->pflags |= ATA_PFLAG_LOADING;
-			ata_port_schedule_eh(ap);
-
-			spin_unlock_irqrestore(ap->lock, flags);
-
-			/* wait for EH to finish */
-			ata_port_wait_eh(ap);
-		} else {
-			DPRINTK("ata%u: bus probe begin\n", ap->id);
-			rc = ata_bus_probe(ap);
-			DPRINTK("ata%u: bus probe end\n", ap->id);
-
-			if (rc) {
-				/* FIXME: do something useful here?
-				 * Current libata behavior will
-				 * tear down everything when
-				 * the module is removed
-				 * or the h/w is unplugged.
-				 */
-			}
-		}
-	}
-
-	/* probes are done, now scan each port's disk(s) */
-	DPRINTK("host probe begin\n");
-	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap = host->ports[i];
-
-		ata_scsi_scan_host(ap);
-	}
-
-	dev_set_drvdata(dev, host);
+	rc = ata_host_attach(host);
+	if (rc)
+		goto err_free_irq2;
 
-	VPRINTK("EXIT, returning %u\n", ent->n_ports);
-	return ent->n_ports; /* success */
+	return host->n_ports; /* success */
 
-err_out_free_irq:
+ err_free_irq2:
+	if (ent->irq2)
+		free_irq(ent->irq2, host);
+ err_free_irq:
 	free_irq(ent->irq, host);
-err_out:
+ err_free_host:
 	ata_host_stop(host);
 	ata_host_free(host);
 	VPRINTK("EXIT, returning 0\n");

drivers/ata/libata-core.c void ata_host_free(struct ata_host *host · line 5915

Quoted 8 lines
drivers/ata/libata-core.c +3 -1
@@ -5755,8 +5909,10 @@ void ata_host_free(struct ata_host *host
 	/* free */
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
-		if (ap)
+		if (ap->pflags & ATA_PFLAG_SHOST_OWNER)
 			scsi_host_put(ap->scsi_host);
+		else
+			kfree(ap);

Do we ever have a reason to care about ap->scsi_host in the SAS case?

It seems like we could kill ATA_PFLAG_SHOST_OWNER, and just test scsi_host for NULL.

author
#11 lore raw

Re: [PATCH 4/20] libata: separate out ata_host_alloc() and ata_host_attach()

Quoted 3 lines · Jeff Garzik

ACK in general, but minor nits are found in comments below.

Also, to make reviewing and git bisect easier, it would be nice to split ata_host_attach() separation into another patch.

Okay, will try.

Quoted 5 lines
Quoted 5 lines
 drivers/ata/libata-core.c |  451 
++++++++++++++++++++++++++++++---------------
 include/linux/libata.h    |    7 +
 2 files changed, 312 insertions(+), 146 deletions(-)
Quoted 14 lines

drivers/ata/libata-core.c void ata_dev_init(struct ata_device *dev · line 5247

Quoted 12 lines
drivers/ata/libata-core.c +2 -2
index d7bf4a1..eeb9dc6 100644
@@ -5243,11 +5243,15 @@ void ata_dev_init(struct ata_device *dev
  *    ata_port_init - Initialize an ata_port structure
  *    @ap: Structure to initialize
  *    @host: Collection of hosts to which @ap belongs
- *    @ent: Probe information provided by low-level driver
- *    @port_no: Port number associated with this ata_port
+ *    @ent: Probe information provided by low-level driver (optional)
+ *    @port_no: Port number associated with this ata_port (optional)

I would perhaps note that port_no is require iff ent != NULL

Will do.

Quoted 9 lines

drivers/ata/libata-core.c static void ata_port_init_shost(struct a · line 5459

Quoted 7 lines
drivers/ata/libata-core.c +7 -0
+int ata_host_add_ports(struct ata_host *host,
+               struct scsi_host_template *sht, int n_ports)
+{
+    int i;
+
+    if (host->n_ports || n_ports > ATA_MAX_PORTS)
+        return -EINVAL;

For what code path would host->n_ports be non-zero?

It's just an error check as this part of code is not very flexible (yet).

Quoted 12 lines

drivers/ata/libata-core.c void ata_host_free(struct ata_host *host · line 5915

Quoted 9 lines
drivers/ata/libata-core.c +3 -1
@@ -5755,8 +5909,10 @@ void ata_host_free(struct ata_host *host
     /* free */
     for (i = 0; i < host->n_ports; i++) {
         struct ata_port *ap = host->ports[i];
-        if (ap)
+        if (ap->pflags & ATA_PFLAG_SHOST_OWNER)
             scsi_host_put(ap->scsi_host);
+        else
+            kfree(ap);

Do we ever have a reason to care about ap->scsi_host in the SAS case?

Quoted 1 line

It seems like we could kill ATA_PFLAG_SHOST_OWNER, and just test scsi_host for NULL.

I thought SAS might want to use the field to back link ATA port to its SCSI host. If that's not necessary, I'll drop the flag.

Thanks.

Signature · 1 line
tejun
author
#12 lore raw

PATCH 5/20 libata: implement several LLD init helpers

In reply to Tejun Heo

Implement the following init helpers.

* ata_host_alloc_pinfo() : alloc host, init with port_info
* ata_host_alloc_pinfo_ar() : alloc host, init with port_info array
* ata_host_add_ports_pinfo() : add ports to host, init with port_info
* ata_host_add_ports_pinfo_ar() : add ports to host, init with port_info array
* ata_host_request_irq_marker() : prep host for IRQ, request IRQ with marker
* ata_host_request_irq() : prep host for IRQ, request IRQ
* ata_host_free_irqs_marker() : free IRQs with the given marker
* ata_host_free_irqs() : free all IRQs

libata core layer keeps track of allocated IRQs and can free subset designated by @marker or all of them.

These helpers will be used in new LLD init model.

---
 drivers/ata/libata-core.c |  300 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata.h      |    6 +
 include/linux/libata.h    |   19 ++-
 3 files changed, 321 insertions(+), 4 deletions(-)
drivers/ata/libata-core.c +300 -0
index eeb9dc6..b293d47 100644
@@ -59,6 +59,13 @@ #include <asm/byteorder.h>
 
 #include "libata.h"
 
+struct ata_irq {
+	struct list_head node;
+	unsigned int irq;
+	void *dev_id;
+	void *marker;
+};
+
 /* debounce timing parameters in msecs { interval, duration, timeout } */
 const unsigned long sata_deb_timing_normal[]		= {   5,  100, 2000 };
 const unsigned long sata_deb_timing_hotplug[]		= {  25,  500, 2000 };
@@ -5421,6 +5428,7 @@ struct ata_host *ata_host_alloc(struct d
 
 	spin_lock_init(&host->lock);
 	host->dev = dev;
+	INIT_LIST_HEAD(&host->irq_list);
 
 	if (ata_host_add_ports(host, sht, n_ports))
 		goto err_free;
@@ -5475,6 +5483,147 @@ int ata_host_add_ports(struct ata_host *
 	return 0;
 }
 
+static void __ata_host_init_pinfo(struct ata_host *host,
+				  const struct ata_port_info **pinfo,
+				  int n_ports, int pi_is_ar)
+{
+	int i;
+
+	if (host->private_data == NULL)
+		host->private_data = pinfo[0]->private_data;
+	host->ops = pinfo[0]->port_ops;
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+		const struct ata_port_info *pi;
+
+		if (pi_is_ar)
+			pi = pinfo[i];
+		else
+			pi = pinfo[0];
+
+		ap->pio_mask = pi->pio_mask;
+		ap->mwdma_mask = pi->mwdma_mask;
+		ap->udma_mask = pi->udma_mask;
+		ap->flags |= pi->flags;
+		ap->ops = pi->port_ops;
+
+		WARN_ON(pi->private_data &&
+			pi->private_data != host->private_data);
+	}
+}
+
+/**
+ *	ata_host_alloc_pinfo - allocate host and init with port_info
+ *	@dev: generic device this host is associated with
+ *	@pinfo: ATA port_info to initialize host with
+ *	@n_ports: number of ATA ports attached to this host
+ *
+ *	Allocate ATA host and initialize with info from @pi.
+ *
+ *	RETURNS:
+ *	Allocate ATA host on success, NULL on failure.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+struct ata_host *ata_host_alloc_pinfo(struct device *dev,
+				      const struct ata_port_info *pinfo,
+				      int n_ports)
+{
+	struct ata_host *host;
+
+	if (!n_ports)
+		return NULL;
+
+	host = ata_host_alloc(dev, pinfo->sht, n_ports);
+	if (host)
+		__ata_host_init_pinfo(host, &pinfo, n_ports, 0);
+	return host;
+}
+
+/**
+ *	ata_host_alloc_pinfo_ar - alloc host and init with port_info ar
+ *	@dev: generic device this host is associated with
+ *	@pinfo_ar: array of ATA port_info to initialize host with
+ *	@n_ports: number of ATA ports attached to this host
+ *
+ *	Allocate ATA host and initialize with info from @pinfo_ar.
+ *
+ *	RETURNS:
+ *	Allocate ATA host on success, NULL on failure.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+struct ata_host *ata_host_alloc_pinfo_ar(struct device *dev,
+					 const struct ata_port_info **pinfo_ar,
+					 int n_ports)
+{
+	struct ata_host *host;
+
+	if (!n_ports)
+		return NULL;
+
+	host = ata_host_alloc(dev, pinfo_ar[0]->sht, n_ports);
+	if (host)
+		__ata_host_init_pinfo(host, pinfo_ar, n_ports, 1);
+	return host;
+}
+
+/**
+ *	ata_host_add_ports_pinfo - add ports and init with port_info
+ *	@host: target ATA host
+ *	@pinfo: ATA port_info to initialize host with
+ *	@n_ports: number of ATA ports attached to this host
+ *
+ *	Add @n_ports ports to @host and initialize @host with
+ *	info from @pinfo.
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+int ata_host_add_ports_pinfo(struct ata_host *host,
+			     const struct ata_port_info *pinfo, int n_ports)
+{
+	int rc;
+
+	rc = ata_host_add_ports(host, pinfo->sht, n_ports);
+	if (rc == 0)
+		__ata_host_init_pinfo(host, &pinfo, n_ports, 0);
+	return rc;
+}
+
+/**
+ *	ata_host_add_ports_pinfo_ar - add ports and init with port_info ar
+ *	@host: target ATA host
+ *	@pinfo_ar: array of ATA port_info to initialize host with
+ *	@n_ports: number of ATA ports attached to this host
+ *
+ *	Add @n_ports ports to @host and initialize @host with
+ *	info from @pinfo_ar.
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+int ata_host_add_ports_pinfo_ar(struct ata_host *host,
+				const struct ata_port_info **pinfo_ar,
+				int n_ports)
+{
+	int rc;
+
+	rc = ata_host_add_ports(host, pinfo_ar[0]->sht, n_ports);
+	if (rc == 0)
+		__ata_host_init_pinfo(host, pinfo_ar, n_ports, 1);
+	return rc;
+}
+
 /**
  *	ata_sas_host_init - Initialize a host struct
  *	@host:	host to initialize
@@ -5494,6 +5643,7 @@ void ata_host_init(struct ata_host *host
 	host->dev = dev;
 	host->flags = flags;
 	host->ops = ops;
+	INIT_LIST_HEAD(&host->irq_list);
 }
 
 /**
@@ -5541,6 +5691,108 @@ int ata_host_start(struct ata_host *host
 }
 
 /**
+ *	ata_host_request_irq_marker - request IRQ helper
+ *	@host: ATA host requesting IRQ for
+ *	@irq: IRQ to request
+ *	@handler: IRQ handler
+ *	@irq_flags: IRQ flags
+ *	@dev_id: IRQ dev_id
+ *	@marker: marker
+ *	@p_reason: out arg for error message (can be NULL)
+ *
+ *	Freeze all ports and request IRQ with given parameters.
+ *	devname for the IRQ will be the name of the associated LLD.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ *
+ *	RETURNS:
+ *	Return value of request_irq().
+ */
+int ata_host_request_irq_marker(struct ata_host *host, unsigned int irq,
+			irqreturn_t (*handler)(int, void *, struct pt_regs *),
+			unsigned int irq_flags, void *dev_id, void *marker,
+			const char **p_reason)
+{
+	struct ata_irq *airq = NULL;
+	const char *reason;
+	int i, rc;
+
+	/* make sure ports are started */
+	rc = ata_host_start(host);
+	if (rc) {
+		reason = "failed to start host";
+		goto err;
+	}
+
+	/* freeze before requesting IRQ */
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+
+		if (!(ap->pflags & ATA_PFLAG_FROZEN)) {
+			ata_chk_status(ap);
+			host->ops->irq_clear(ap);
+			ata_eh_freeze_port(ap);
+		}
+	}
+
+	/* request irq */
+	airq = kzalloc(sizeof(*airq), GFP_KERNEL);
+	if (!airq) {
+		reason = "failed to allocate ata_irq";
+		rc = -ENOMEM;
+		goto err;
+	}
+
+	rc = request_irq(irq, handler, irq_flags, DRV_NAME, dev_id);
+	if (rc) {
+		reason = "failed to request IRQ";
+		goto err;
+	}
+
+	airq->irq = irq;
+	airq->dev_id = dev_id;
+	airq->marker = marker;
+	list_add_tail(&airq->node, &host->irq_list);
+
+	return rc;
+
+ err:
+	kfree(airq);
+	if (p_reason)
+		*p_reason = reason;
+	return rc;
+}
+
+/**
+ *	ata_host_request_irq - request IRQ helper
+ *	@host: ATA host requesting IRQ for
+ *	@irq: IRQ to request
+ *	@handler: IRQ handler
+ *	@irq_flags: IRQ flags
+ *	@dev_id: IRQ dev_id
+ *	@p_reason: out arg for error message (can be NULL)
+ *
+ *	Freeze all ports and request IRQ with given parameters.
+ *	devname for the IRQ will be the name of the associated LLD.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ *
+ *	RETURNS:
+ *	Return value of request_irq().
+ */
+int ata_host_request_irq(struct ata_host *host, unsigned int irq,
+			 irqreturn_t (*handler)(int, void *, struct pt_regs *),
+			 unsigned int irq_flags, void *dev_id,
+			 const char **p_reason)
+{
+	return ata_host_request_irq_marker(host, irq, handler, irq_flags,
+					   dev_id, ata_host_request_irq,
+					   p_reason);
+}
+
+/**
  *	ata_host_attach - attach initialized ATA host
  *	@host: ATA host to attach
  *
@@ -5886,6 +6138,48 @@ void ata_host_stop(struct ata_host *host
 	}
 }
 
+static void __ata_host_free_irqs(struct ata_host *host, void **markerp)
+{
+	struct ata_irq *airq, *tmp;
+
+	list_for_each_entry_safe(airq, tmp, &host->irq_list, node) {
+		if (!markerp || airq->marker == *markerp) {
+			list_del(&airq->node);
+			free_irq(airq->irq, airq->dev_id);
+			kfree(airq);
+		}
+	}
+}
+
+/**
+ *	ata_host_free_irqs_marker - free the IRQs with matching marker
+ *	@host: target ATA host
+ *	@marker: marker to match
+ *
+ *	Free IRQs @host is holding and marked with @marker.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+void ata_host_free_irqs_marker(struct ata_host *host, void *marker)
+{
+	__ata_host_free_irqs(host, &marker);
+}
+
+/**
+ *	ata_host_free_irqs - free all IRQs an ATA host is holding
+ *	@host: target ATA host
+ *
+ *	Free all IRQs @host is holding.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+void ata_host_free_irqs(struct ata_host *host)
+{
+	__ata_host_free_irqs(host, NULL);
+}
+
 /**
  *	ata_host_free - Release a host
  *	@host: ATA host set to be freed
@@ -6266,11 +6560,17 @@ EXPORT_SYMBOL_GPL(ata_std_ports);
 EXPORT_SYMBOL_GPL(ata_host_init);
 EXPORT_SYMBOL_GPL(ata_host_alloc);
 EXPORT_SYMBOL_GPL(ata_host_add_ports);
+EXPORT_SYMBOL_GPL(ata_host_alloc_pinfo);
+EXPORT_SYMBOL_GPL(ata_host_alloc_pinfo_ar);
+EXPORT_SYMBOL_GPL(ata_host_add_ports_pinfo);
+EXPORT_SYMBOL_GPL(ata_host_add_ports_pinfo_ar);
 EXPORT_SYMBOL_GPL(ata_host_start);
+EXPORT_SYMBOL_GPL(ata_host_request_irq);
 EXPORT_SYMBOL_GPL(ata_host_attach);
 EXPORT_SYMBOL_GPL(ata_device_add);
 EXPORT_SYMBOL_GPL(ata_host_detach);
 EXPORT_SYMBOL_GPL(ata_host_stop);
+EXPORT_SYMBOL_GPL(ata_host_free_irqs);
 EXPORT_SYMBOL_GPL(ata_host_free);
 EXPORT_SYMBOL_GPL(ata_host_remove);
 EXPORT_SYMBOL_GPL(ata_sg_init);
drivers/ata/libata.h +5 -1
index a5ecb71..1d24254 100644
@@ -73,7 +73,11 @@ extern void ata_port_init(struct ata_por
 			  const struct ata_probe_ent *ent, unsigned int port_no);
 extern struct ata_probe_ent *ata_probe_ent_alloc(struct device *dev,
 						 const struct ata_port_info *port);
-
+extern int ata_host_request_irq_marker(struct ata_host *host, unsigned int irq,
+			irqreturn_t (*handler)(int, void *, struct pt_regs *),
+			unsigned int irq_flags, void *dev_id, void *marker,
+			const char **p_reason);
+extern void ata_host_free_irqs_marker(struct ata_host *host, void *marker);
 
 /* libata-scsi.c */
 extern struct scsi_transport_template ata_scsi_transport_template;
include/linux/libata.h +16 -3
index 5e8160a..b050517 100644
@@ -378,6 +378,7 @@ struct ata_host {
 	unsigned long		flags;
 	int			simplex_claimed;	/* Keep seperate in case we
 							   ever need to do this locked */
+	struct list_head	irq_list;	/* list of acquired irqs */
 	struct ata_port		*ports[0];
 };
 
@@ -692,15 +693,27 @@ extern int ata_pci_device_resume(struct 
 extern int ata_pci_clear_simplex(struct pci_dev *pdev);
 #endif /* CONFIG_PCI */
 extern struct ata_host *ata_host_alloc(struct device *dev,
-				       struct scsi_host_template *sht,
-				       int n_ports);
+			struct scsi_host_template *sht, int n_ports);
 extern int ata_host_add_ports(struct ata_host *host,
-			      struct scsi_host_template *sht, int n_ports);
+			struct scsi_host_template *sht, int n_ports);
+extern struct ata_host *ata_host_alloc_pinfo(struct device *dev,
+			const struct ata_port_info *pinfo, int n_ports);
+extern struct ata_host *ata_host_alloc_pinfo_ar(struct device *dev,
+			const struct ata_port_info **pinfo_ar, int n_ports);
+extern int ata_host_add_ports_pinfo(struct ata_host *host,
+			const struct ata_port_info *pinfo, int n_ports);
+extern int ata_host_add_ports_pinfo_ar(struct ata_host *host,
+			const struct ata_port_info **pinfo_ar, int n_ports);
+extern int ata_host_request_irq(struct ata_host *host, unsigned int irq,
+			irqreturn_t (*handler)(int, void *, struct pt_regs *),
+			unsigned int irq_flags, void *dev_id,
+			const char **p_reason);
 extern int ata_host_start(struct ata_host *host);
 extern int ata_host_attach(struct ata_host *host);
 extern int ata_device_add(const struct ata_probe_ent *ent);
 extern void ata_host_detach(struct ata_host *host);
 extern void ata_host_stop(struct ata_host *host);
+extern void ata_host_free_irqs(struct ata_host *host);
 extern void ata_host_free(struct ata_host *host);
 extern void ata_host_init(struct ata_host *, struct device *,
 			  unsigned long, const struct ata_port_operations *);
Signature · 1 line
1.4.1.1
#13 lore raw

Re: [PATCH 5/20] libata: implement several LLD init helpers

Quoted 16 lines · Tejun Heo
drivers/ata/libata-core.c +16 -0
+static void __ata_host_init_pinfo(struct ata_host *host,
+				  const struct ata_port_info **pinfo,
+				  int n_ports, int pi_is_ar)
+{
+	int i;
+
+	if (host->private_data == NULL)
+		host->private_data = pinfo[0]->private_data;
+	host->ops = pinfo[0]->port_ops;
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+		const struct ata_port_info *pi;
+
+		if (pi_is_ar)
+			pi = pinfo[i];

drivers/ata/libata-core.c int ata_host_add_ports(struct ata_host * · line 5509

Quoted 10 lines
drivers/ata/libata-core.c +8 -0
+		else
+			pi = pinfo[0];
+
 
 
+		ap->pio_mask = pi->pio_mask;
+		ap->mwdma_mask = pi->mwdma_mask;
+		ap->udma_mask = pi->udma_mask;
+		ap->flags |= pi->flags;
+		ap->ops = pi->port_ops;

Could this bit be moved into ata_port_init and possibly change the parameter list of ata_port_init to something like:

void ata_port_init(struct ata_port *ap, struct ata_host *host,
const struct ata_port_info *pinfo, unsigned int port_no)

Then we could get rid of the struct ata_probe_ent stuff completely.

Brian

Quoted 364 lines
drivers/ata/libata-core.c +268 -0
+
+		WARN_ON(pi->private_data &&
+			pi->private_data != host->private_data);
+	}
+}
+
+/**
+ *	ata_host_alloc_pinfo - allocate host and init with port_info
+ *	@dev: generic device this host is associated with
+ *	@pinfo: ATA port_info to initialize host with
+ *	@n_ports: number of ATA ports attached to this host
+ *
+ *	Allocate ATA host and initialize with info from @pi.
+ *
+ *	RETURNS:
+ *	Allocate ATA host on success, NULL on failure.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+struct ata_host *ata_host_alloc_pinfo(struct device *dev,
+				      const struct ata_port_info *pinfo,
+				      int n_ports)
+{
+	struct ata_host *host;
+
+	if (!n_ports)
+		return NULL;
+
+	host = ata_host_alloc(dev, pinfo->sht, n_ports);
+	if (host)
+		__ata_host_init_pinfo(host, &pinfo, n_ports, 0);
+	return host;
+}
+
+/**
+ *	ata_host_alloc_pinfo_ar - alloc host and init with port_info ar
+ *	@dev: generic device this host is associated with
+ *	@pinfo_ar: array of ATA port_info to initialize host with
+ *	@n_ports: number of ATA ports attached to this host
+ *
+ *	Allocate ATA host and initialize with info from @pinfo_ar.
+ *
+ *	RETURNS:
+ *	Allocate ATA host on success, NULL on failure.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+struct ata_host *ata_host_alloc_pinfo_ar(struct device *dev,
+					 const struct ata_port_info **pinfo_ar,
+					 int n_ports)
+{
+	struct ata_host *host;
+
+	if (!n_ports)
+		return NULL;
+
+	host = ata_host_alloc(dev, pinfo_ar[0]->sht, n_ports);
+	if (host)
+		__ata_host_init_pinfo(host, pinfo_ar, n_ports, 1);
+	return host;
+}
+
+/**
+ *	ata_host_add_ports_pinfo - add ports and init with port_info
+ *	@host: target ATA host
+ *	@pinfo: ATA port_info to initialize host with
+ *	@n_ports: number of ATA ports attached to this host
+ *
+ *	Add @n_ports ports to @host and initialize @host with
+ *	info from @pinfo.
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+int ata_host_add_ports_pinfo(struct ata_host *host,
+			     const struct ata_port_info *pinfo, int n_ports)
+{
+	int rc;
+
+	rc = ata_host_add_ports(host, pinfo->sht, n_ports);
+	if (rc == 0)
+		__ata_host_init_pinfo(host, &pinfo, n_ports, 0);
+	return rc;
+}
+
+/**
+ *	ata_host_add_ports_pinfo_ar - add ports and init with port_info ar
+ *	@host: target ATA host
+ *	@pinfo_ar: array of ATA port_info to initialize host with
+ *	@n_ports: number of ATA ports attached to this host
+ *
+ *	Add @n_ports ports to @host and initialize @host with
+ *	info from @pinfo_ar.
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+int ata_host_add_ports_pinfo_ar(struct ata_host *host,
+				const struct ata_port_info **pinfo_ar,
+				int n_ports)
+{
+	int rc;
+
+	rc = ata_host_add_ports(host, pinfo_ar[0]->sht, n_ports);
+	if (rc == 0)
+		__ata_host_init_pinfo(host, pinfo_ar, n_ports, 1);
+	return rc;
+}
+
 /**
  *	ata_sas_host_init - Initialize a host struct
  *	@host:	host to initialize
@@ -5494,6 +5643,7 @@ void ata_host_init(struct ata_host *host
 	host->dev = dev;
 	host->flags = flags;
 	host->ops = ops;
+	INIT_LIST_HEAD(&host->irq_list);
 }
 
 /**
@@ -5541,6 +5691,108 @@ int ata_host_start(struct ata_host *host
 }
 
 /**
+ *	ata_host_request_irq_marker - request IRQ helper
+ *	@host: ATA host requesting IRQ for
+ *	@irq: IRQ to request
+ *	@handler: IRQ handler
+ *	@irq_flags: IRQ flags
+ *	@dev_id: IRQ dev_id
+ *	@marker: marker
+ *	@p_reason: out arg for error message (can be NULL)
+ *
+ *	Freeze all ports and request IRQ with given parameters.
+ *	devname for the IRQ will be the name of the associated LLD.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ *
+ *	RETURNS:
+ *	Return value of request_irq().
+ */
+int ata_host_request_irq_marker(struct ata_host *host, unsigned int irq,
+			irqreturn_t (*handler)(int, void *, struct pt_regs *),
+			unsigned int irq_flags, void *dev_id, void *marker,
+			const char **p_reason)
+{
+	struct ata_irq *airq = NULL;
+	const char *reason;
+	int i, rc;
+
+	/* make sure ports are started */
+	rc = ata_host_start(host);
+	if (rc) {
+		reason = "failed to start host";
+		goto err;
+	}
+
+	/* freeze before requesting IRQ */
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+
+		if (!(ap->pflags & ATA_PFLAG_FROZEN)) {
+			ata_chk_status(ap);
+			host->ops->irq_clear(ap);
+			ata_eh_freeze_port(ap);
+		}
+	}
+
+	/* request irq */
+	airq = kzalloc(sizeof(*airq), GFP_KERNEL);
+	if (!airq) {
+		reason = "failed to allocate ata_irq";
+		rc = -ENOMEM;
+		goto err;
+	}
+
+	rc = request_irq(irq, handler, irq_flags, DRV_NAME, dev_id);
+	if (rc) {
+		reason = "failed to request IRQ";
+		goto err;
+	}
+
+	airq->irq = irq;
+	airq->dev_id = dev_id;
+	airq->marker = marker;
+	list_add_tail(&airq->node, &host->irq_list);
+
+	return rc;
+
+ err:
+	kfree(airq);
+	if (p_reason)
+		*p_reason = reason;
+	return rc;
+}
+
+/**
+ *	ata_host_request_irq - request IRQ helper
+ *	@host: ATA host requesting IRQ for
+ *	@irq: IRQ to request
+ *	@handler: IRQ handler
+ *	@irq_flags: IRQ flags
+ *	@dev_id: IRQ dev_id
+ *	@p_reason: out arg for error message (can be NULL)
+ *
+ *	Freeze all ports and request IRQ with given parameters.
+ *	devname for the IRQ will be the name of the associated LLD.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ *
+ *	RETURNS:
+ *	Return value of request_irq().
+ */
+int ata_host_request_irq(struct ata_host *host, unsigned int irq,
+			 irqreturn_t (*handler)(int, void *, struct pt_regs *),
+			 unsigned int irq_flags, void *dev_id,
+			 const char **p_reason)
+{
+	return ata_host_request_irq_marker(host, irq, handler, irq_flags,
+					   dev_id, ata_host_request_irq,
+					   p_reason);
+}
+
+/**
  *	ata_host_attach - attach initialized ATA host
  *	@host: ATA host to attach
  *
@@ -5886,6 +6138,48 @@ void ata_host_stop(struct ata_host *host
 	}
 }
 
+static void __ata_host_free_irqs(struct ata_host *host, void **markerp)
+{
+	struct ata_irq *airq, *tmp;
+
+	list_for_each_entry_safe(airq, tmp, &host->irq_list, node) {
+		if (!markerp || airq->marker == *markerp) {
+			list_del(&airq->node);
+			free_irq(airq->irq, airq->dev_id);
+			kfree(airq);
+		}
+	}
+}
+
+/**
+ *	ata_host_free_irqs_marker - free the IRQs with matching marker
+ *	@host: target ATA host
+ *	@marker: marker to match
+ *
+ *	Free IRQs @host is holding and marked with @marker.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+void ata_host_free_irqs_marker(struct ata_host *host, void *marker)
+{
+	__ata_host_free_irqs(host, &marker);
+}
+
+/**
+ *	ata_host_free_irqs - free all IRQs an ATA host is holding
+ *	@host: target ATA host
+ *
+ *	Free all IRQs @host is holding.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+void ata_host_free_irqs(struct ata_host *host)
+{
+	__ata_host_free_irqs(host, NULL);
+}
+
 /**
  *	ata_host_free - Release a host
  *	@host: ATA host set to be freed
@@ -6266,11 +6560,17 @@ EXPORT_SYMBOL_GPL(ata_std_ports);
 EXPORT_SYMBOL_GPL(ata_host_init);
 EXPORT_SYMBOL_GPL(ata_host_alloc);
 EXPORT_SYMBOL_GPL(ata_host_add_ports);
+EXPORT_SYMBOL_GPL(ata_host_alloc_pinfo);
+EXPORT_SYMBOL_GPL(ata_host_alloc_pinfo_ar);
+EXPORT_SYMBOL_GPL(ata_host_add_ports_pinfo);
+EXPORT_SYMBOL_GPL(ata_host_add_ports_pinfo_ar);
 EXPORT_SYMBOL_GPL(ata_host_start);
+EXPORT_SYMBOL_GPL(ata_host_request_irq);
 EXPORT_SYMBOL_GPL(ata_host_attach);
 EXPORT_SYMBOL_GPL(ata_device_add);
 EXPORT_SYMBOL_GPL(ata_host_detach);
 EXPORT_SYMBOL_GPL(ata_host_stop);
+EXPORT_SYMBOL_GPL(ata_host_free_irqs);
 EXPORT_SYMBOL_GPL(ata_host_free);
 EXPORT_SYMBOL_GPL(ata_host_remove);
 EXPORT_SYMBOL_GPL(ata_sg_init);
drivers/ata/libata.h +5 -1
index a5ecb71..1d24254 100644
@@ -73,7 +73,11 @@ extern void ata_port_init(struct ata_por
 			  const struct ata_probe_ent *ent, unsigned int port_no);
 extern struct ata_probe_ent *ata_probe_ent_alloc(struct device *dev,
 						 const struct ata_port_info *port);
-
+extern int ata_host_request_irq_marker(struct ata_host *host, unsigned int irq,
+			irqreturn_t (*handler)(int, void *, struct pt_regs *),
+			unsigned int irq_flags, void *dev_id, void *marker,
+			const char **p_reason);
+extern void ata_host_free_irqs_marker(struct ata_host *host, void *marker);
 
 /* libata-scsi.c */
 extern struct scsi_transport_template ata_scsi_transport_template;
include/linux/libata.h +16 -3
index 5e8160a..b050517 100644
@@ -378,6 +378,7 @@ struct ata_host {
 	unsigned long		flags;
 	int			simplex_claimed;	/* Keep seperate in case we
 							   ever need to do this locked */
+	struct list_head	irq_list;	/* list of acquired irqs */
 	struct ata_port		*ports[0];
 };
 
@@ -692,15 +693,27 @@ extern int ata_pci_device_resume(struct 
 extern int ata_pci_clear_simplex(struct pci_dev *pdev);
 #endif /* CONFIG_PCI */
 extern struct ata_host *ata_host_alloc(struct device *dev,
-				       struct scsi_host_template *sht,
-				       int n_ports);
+			struct scsi_host_template *sht, int n_ports);
 extern int ata_host_add_ports(struct ata_host *host,
-			      struct scsi_host_template *sht, int n_ports);
+			struct scsi_host_template *sht, int n_ports);
+extern struct ata_host *ata_host_alloc_pinfo(struct device *dev,
+			const struct ata_port_info *pinfo, int n_ports);
+extern struct ata_host *ata_host_alloc_pinfo_ar(struct device *dev,
+			const struct ata_port_info **pinfo_ar, int n_ports);
+extern int ata_host_add_ports_pinfo(struct ata_host *host,
+			const struct ata_port_info *pinfo, int n_ports);
+extern int ata_host_add_ports_pinfo_ar(struct ata_host *host,
+			const struct ata_port_info **pinfo_ar, int n_ports);
+extern int ata_host_request_irq(struct ata_host *host, unsigned int irq,
+			irqreturn_t (*handler)(int, void *, struct pt_regs *),
+			unsigned int irq_flags, void *dev_id,
+			const char **p_reason);
 extern int ata_host_start(struct ata_host *host);
 extern int ata_host_attach(struct ata_host *host);
 extern int ata_device_add(const struct ata_probe_ent *ent);
 extern void ata_host_detach(struct ata_host *host);
 extern void ata_host_stop(struct ata_host *host);
+extern void ata_host_free_irqs(struct ata_host *host);
 extern void ata_host_free(struct ata_host *host);
 extern void ata_host_init(struct ata_host *, struct device *,
 			  unsigned long, const struct ata_port_operations *);
Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center
author
#14 lore raw

Re: [PATCH 5/20] libata: implement several LLD init helpers

Hello, Brian.

Quoted 32 lines · Brian King
Quoted 16 lines · Tejun Heo
drivers/ata/libata-core.c +16 -0
+static void __ata_host_init_pinfo(struct ata_host *host,
+				  const struct ata_port_info **pinfo,
+				  int n_ports, int pi_is_ar)
+{
+	int i;
+
+	if (host->private_data == NULL)
+		host->private_data = pinfo[0]->private_data;
+	host->ops = pinfo[0]->port_ops;
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+		const struct ata_port_info *pi;
+
+		if (pi_is_ar)
+			pi = pinfo[i];

drivers/ata/libata-core.c int ata_host_add_ports(struct ata_host * · line 5509

Quoted 10 lines
drivers/ata/libata-core.c +8 -0
+		else
+			pi = pinfo[0];
+
 
 
+		ap->pio_mask = pi->pio_mask;
+		ap->mwdma_mask = pi->mwdma_mask;
+		ap->udma_mask = pi->udma_mask;
+		ap->flags |= pi->flags;
+		ap->ops = pi->port_ops;

Could this bit be moved into ata_port_init and possibly change the parameter list of ata_port_init to something like:

Quoted 4 lines

void ata_port_init(struct ata_port *ap, struct ata_host *host,
const struct ata_port_info *pinfo, unsigned int port_no)

Then we could get rid of the struct ata_probe_ent stuff completely.

Hmmm... ata_port_init() is still exported only to support ata_sas_* helpers. Once the SAS helpers are converted to use new model, this function will be made static or merged into ata_host_alloc(). In the new init model, initialization is done by...

1. allocate the host and associated ports with ata_host_alloc(). If port number cannot be determined at this point. ports can be added later. The returned ata_host has only the basic fields, which assume the same initial values regardless of the user, initialized.

2. initialize ata_host and associated ports appropriately. During this ata_host_add_ports() can be used to add ports if host has no ports yet.

3. attach the initialized ata_host using ata_host_attach().

The ata_host_alloc_pinfo[_ar]() functions are just helpers which perform #1 and part of #2 as ata_port_info is handy for many LLDs.

For SAS helpers, I don't think using pinfo helpers is necessary. Just initializing the above fields manually in SAS helper or SAS LLD should do. The pinfo helpers are specifically for this purpose and doesn't allow delayed port addition.

Also, note that probe_ent is still there only for SAS helpers. If you take a look at normal SATA init path, no probe_ent is used. Only ata_port_init() deals with it and usual SATA path calls it with null probe_ent.

Thanks.

Signature · 1 line
tejun
#15 lore raw

Re: [PATCH 5/20] libata: implement several LLD init helpers

Quoted 48 lines · Tejun Heo

Hello, Brian.

Quoted 30 lines · Brian King
Quoted 15 lines · Tejun Heo
drivers/ata/libata-core.c +15 -0
+static void __ata_host_init_pinfo(struct ata_host *host,
+				  const struct ata_port_info **pinfo,
+				  int n_ports, int pi_is_ar)
+{
+	int i;
+
+	if (host->private_data == NULL)
+		host->private_data = pinfo[0]->private_data;
+	host->ops = pinfo[0]->port_ops;
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+		const struct ata_port_info *pi;
+
+		if (pi_is_ar)

drivers/ata/libata-core.c int ata_host_add_ports(struct ata_host * · line 5509

Quoted 10 lines
drivers/ata/libata-core.c +9 -0
+			pi = pinfo[i];
+		else
+			pi = pinfo[0];
+
 
+		ap->pio_mask = pi->pio_mask;
+		ap->mwdma_mask = pi->mwdma_mask;
+		ap->udma_mask = pi->udma_mask;
+		ap->flags |= pi->flags;
+		ap->ops = pi->port_ops;

Could this bit be moved into ata_port_init and possibly change the parameter list of ata_port_init to something like:

Quoted 4 lines

void ata_port_init(struct ata_port *ap, struct ata_host *host,
const struct ata_port_info *pinfo, unsigned int port_no)

Then we could get rid of the struct ata_probe_ent stuff completely.

Hmmm... ata_port_init() is still exported only to support ata_sas_* helpers. Once the SAS helpers are converted to use new model, this function will be made static or merged into ata_host_alloc(). In the new init model, initialization is done by...

1. allocate the host and associated ports with ata_host_alloc(). If port number cannot be determined at this point. ports can be added later. The returned ata_host has only the basic fields, which assume the same initial values regardless of the user, initialized.

Quoted 4 lines

2. initialize ata_host and associated ports appropriately. During this ata_host_add_ports() can be used to add ports if host has no ports yet.

3. attach the initialized ata_host using ata_host_attach().

Are you suggesting SAS would follow this host initialization model as well? ata_host_attach currently does not work for SAS since it calls scsi_add_host for each ata port, but that is easily fixed. I think the broader question is whether or not SAS should be calling ata_host_attach to probe all "ports" on the SAS adapter. Since for SAS, the concept of a SATA "port" is really a virtual and very dynamic thing, I'm inclined to think SAS users calling ata_host_attach to scan all "ports" might not be the best approach.

Brian

Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center
#16 lore raw

Re: [PATCH 5/20] libata: implement several LLD init helpers

In reply to Tejun Heo
Quoted 86 lines · Tejun Heo
drivers/ata/libata-core.c +86 -0
+struct ata_host *ata_host_alloc_pinfo(struct device *dev,
+				      const struct ata_port_info *pinfo,
+				      int n_ports)
+{
+	struct ata_host *host;
+
+	if (!n_ports)
+		return NULL;
+
+	host = ata_host_alloc(dev, pinfo->sht, n_ports);
+	if (host)
+		__ata_host_init_pinfo(host, &pinfo, n_ports, 0);
+	return host;
+}
+
+/**
+ *	ata_host_alloc_pinfo_ar - alloc host and init with port_info ar
+ *	@dev: generic device this host is associated with
+ *	@pinfo_ar: array of ATA port_info to initialize host with
+ *	@n_ports: number of ATA ports attached to this host
+ *
+ *	Allocate ATA host and initialize with info from @pinfo_ar.
+ *
+ *	RETURNS:
+ *	Allocate ATA host on success, NULL on failure.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+struct ata_host *ata_host_alloc_pinfo_ar(struct device *dev,
+					 const struct ata_port_info **pinfo_ar,
+					 int n_ports)
+{
+	struct ata_host *host;
+
+	if (!n_ports)
+		return NULL;
+
+	host = ata_host_alloc(dev, pinfo_ar[0]->sht, n_ports);
+	if (host)
+		__ata_host_init_pinfo(host, pinfo_ar, n_ports, 1);
+	return host;
+}
+
+/**
+ *	ata_host_add_ports_pinfo - add ports and init with port_info
+ *	@host: target ATA host
+ *	@pinfo: ATA port_info to initialize host with
+ *	@n_ports: number of ATA ports attached to this host
+ *
+ *	Add @n_ports ports to @host and initialize @host with
+ *	info from @pinfo.
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+int ata_host_add_ports_pinfo(struct ata_host *host,
+			     const struct ata_port_info *pinfo, int n_ports)
+{
+	int rc;
+
+	rc = ata_host_add_ports(host, pinfo->sht, n_ports);
+	if (rc == 0)
+		__ata_host_init_pinfo(host, &pinfo, n_ports, 0);
+	return rc;
+}
+
+/**
+ *	ata_host_add_ports_pinfo_ar - add ports and init with port_info ar
+ *	@host: target ATA host
+ *	@pinfo_ar: array of ATA port_info to initialize host with
+ *	@n_ports: number of ATA ports attached to this host
+ *
+ *	Add @n_ports ports to @host and initialize @host with
+ *	info from @pinfo_ar.
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+int ata_host_add_ports_pinfo_ar(struct ata_host *host,

drivers/ata/libata-core.c int ata_host_add_ports(struct ata_host * · line 5625

Quoted 10 lines
drivers/ata/libata-core.c +10 -0
+				const struct ata_port_info **pinfo_ar,
+				int n_ports)
+{
+	int rc;
+
+	rc = ata_host_add_ports(host, pinfo_ar[0]->sht, n_ports);
+	if (rc == 0)
+		__ata_host_init_pinfo(host, pinfo_ar, n_ports, 1);
+	return rc;
+}

Just implement a single version for each case (array and non-array). The LLDD can pass in an indication of whether or not to copy the first element into succeeding elements.

Quoted 123 lines
drivers/ata/libata-core.c +105 -0
@@ -5494,6 +5643,7 @@ void ata_host_init(struct ata_host *host
 	host->dev = dev;
 	host->flags = flags;
 	host->ops = ops;
+	INIT_LIST_HEAD(&host->irq_list);
 }
 
 /**
@@ -5541,6 +5691,108 @@ int ata_host_start(struct ata_host *host
 }
 
 /**
+ *	ata_host_request_irq_marker - request IRQ helper
+ *	@host: ATA host requesting IRQ for
+ *	@irq: IRQ to request
+ *	@handler: IRQ handler
+ *	@irq_flags: IRQ flags
+ *	@dev_id: IRQ dev_id
+ *	@marker: marker
+ *	@p_reason: out arg for error message (can be NULL)
+ *
+ *	Freeze all ports and request IRQ with given parameters.
+ *	devname for the IRQ will be the name of the associated LLD.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ *
+ *	RETURNS:
+ *	Return value of request_irq().
+ */
+int ata_host_request_irq_marker(struct ata_host *host, unsigned int irq,
+			irqreturn_t (*handler)(int, void *, struct pt_regs *),
+			unsigned int irq_flags, void *dev_id, void *marker,
+			const char **p_reason)
+{
+	struct ata_irq *airq = NULL;
+	const char *reason;
+	int i, rc;
+
+	/* make sure ports are started */
+	rc = ata_host_start(host);
+	if (rc) {
+		reason = "failed to start host";
+		goto err;
+	}
+
+	/* freeze before requesting IRQ */
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+
+		if (!(ap->pflags & ATA_PFLAG_FROZEN)) {
+			ata_chk_status(ap);
+			host->ops->irq_clear(ap);
+			ata_eh_freeze_port(ap);
+		}
+	}
+
+	/* request irq */
+	airq = kzalloc(sizeof(*airq), GFP_KERNEL);
+	if (!airq) {
+		reason = "failed to allocate ata_irq";
+		rc = -ENOMEM;
+		goto err;
+	}
+
+	rc = request_irq(irq, handler, irq_flags, DRV_NAME, dev_id);
+	if (rc) {
+		reason = "failed to request IRQ";
+		goto err;
+	}
+
+	airq->irq = irq;
+	airq->dev_id = dev_id;
+	airq->marker = marker;
+	list_add_tail(&airq->node, &host->irq_list);
+
+	return rc;
+
+ err:
+	kfree(airq);
+	if (p_reason)
+		*p_reason = reason;
+	return rc;
+}
+
+/**
+ *	ata_host_request_irq - request IRQ helper
+ *	@host: ATA host requesting IRQ for
+ *	@irq: IRQ to request
+ *	@handler: IRQ handler
+ *	@irq_flags: IRQ flags
+ *	@dev_id: IRQ dev_id
+ *	@p_reason: out arg for error message (can be NULL)
+ *
+ *	Freeze all ports and request IRQ with given parameters.
+ *	devname for the IRQ will be the name of the associated LLD.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ *
+ *	RETURNS:
+ *	Return value of request_irq().
+ */
+int ata_host_request_irq(struct ata_host *host, unsigned int irq,
+			 irqreturn_t (*handler)(int, void *, struct pt_regs *),
+			 unsigned int irq_flags, void *dev_id,
+			 const char **p_reason)
+{
+	return ata_host_request_irq_marker(host, irq, handler, irq_flags,
+					   dev_id, ata_host_request_irq,
+					   p_reason);
+}
+
+/**
  *	ata_host_attach - attach initialized ATA host
  *	@host: ATA host to attach
  *
@@ -5886,6 +6138,48 @@ void ata_host_stop(struct ata_host *host
 	}
 }
 
+static void __ata_host_free_irqs(struct ata_host *host, void **markerp)
+{

drivers/ata/libata-core.c void ata_host_stop(struct ata_host *host · line 6152

Quoted 10 lines
drivers/ata/libata-core.c +10 -0
+	struct ata_irq *airq, *tmp;
+
+	list_for_each_entry_safe(airq, tmp, &host->irq_list, node) {
+		if (!markerp || airq->marker == *markerp) {
+			list_del(&airq->node);
+			free_irq(airq->irq, airq->dev_id);
+			kfree(airq);
+		}
+	}
+}

Ugh, I just don't like this at all. I would much rather have a hook or entry point where the LLDD is given the capability to call request_irq() itself, in some exotic situations. Then, helpers provided by libata can handle the common cases. That's much more modular than the above.

Jeff

author
#17 lore raw

Re: [PATCH 5/20] libata: implement several LLD init helpers

Quoted 5 lines · Jeff Garzik
Quoted 5 lines
drivers/ata/libata-core.c +5 -0
+int ata_host_add_ports_pinfo_ar(struct ata_host *host,
+                const struct ata_port_info **pinfo_ar,
+                int n_ports)
+{
+    int rc;
Quoted 8 lines

drivers/ata/libata-core.c int ata_host_add_ports(struct ata_host * · line 5625

Quoted 6 lines
drivers/ata/libata-core.c +6 -0
+
+    rc = ata_host_add_ports(host, pinfo_ar[0]->sht, n_ports);
+    if (rc == 0)
+        __ata_host_init_pinfo(host, pinfo_ar, n_ports, 1);
+    return rc;
+}

Just implement a single version for each case (array and non-array). The LLDD can pass in an indication of whether or not to copy the first element into succeeding elements.

Okay.

Quoted 6 lines
Quoted 6 lines
drivers/ata/libata-core.c +6 -0
+static void __ata_host_free_irqs(struct ata_host *host, void **markerp)
+{
+    struct ata_irq *airq, *tmp;
+
+    list_for_each_entry_safe(airq, tmp, &host->irq_list, node) {
+        if (!markerp || airq->marker == *markerp) {
Quoted 8 lines

drivers/ata/libata-core.c void ata_host_stop(struct ata_host *host · line 6152

Quoted 6 lines
drivers/ata/libata-core.c +6 -0
+            list_del(&airq->node);
+            free_irq(airq->irq, airq->dev_id);
+            kfree(airq);
+        }
+    }
+}

Ugh, I just don't like this at all. I would much rather have a hook or entry point where the LLDD is given the capability to call request_irq() itself, in some exotic situations. Then, helpers provided by libata can handle the common cases. That's much more modular than the above.

What I was trying to do was to make libata keep track of allocated resources including IRQ handlers. While implementing this patch, I've located several bugs failing to free / or freeing twice resources in the init-failure path or detach path.

With iomap, multiple IRQ handlers and different IRQ modes, the resource management is getting more complex. Bugs acquiring the resources are easy to catch but the other way around is not. So, I wanted to push all the 'free's into libata helpers such that LLDs can call single or a few libata free helpers and be done with it - or, more importantly, the free code can be boilerplate part shared across all/most LLDs.

That's where the above IRQ handler list comes from. The marker thing is to make different libata core modules (pci, legacy, etc...) deallocate its own and completely hidden from LLDs.

I'd like to know your opinion on this.

Thanks.

Signature · 1 line
tejun
author
#18 lore raw

PATCH 6/20 libata: implement legacy ATA init helpers

In reply to Tejun Heo

Implement the following legacy ATA init helpers.

* ata_legacy_acquire_resources() : acquire legacy ATA resources
* ata_legacy_release_resources() : release legacy ATA resources
* ata_legacy_init_ports() : init legacy port addresses
* ata_legacy_request_irqs() : request legacy ATA IRQs
* ata_legacy_free_irqs() : free legacy ATA IRQs

These helpers can be used independently or called implictly from other bus helpers which has support for legacy ATA device for compatibility (e.g. PCI).

---
 drivers/ata/Kconfig         |    5 +
 drivers/ata/Makefile        |    3 +
 drivers/ata/libata-legacy.c |  241 +++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata.h        |   11 ++
 include/linux/libata.h      |   15 ++-
 5 files changed, 273 insertions(+), 2 deletions(-)
drivers/ata/Kconfig +5 -0
index 2109d75..1a98559 100644
@@ -15,6 +15,11 @@ config ATA
 	  that "speaks" the ATA protocol, also called ATA controller),
 	  because you will be asked for it.
 
+config ATA_LEGACY
+	bool
+	depends on ATA
+	default n
+
 config SATA_AHCI
 	tristate "AHCI SATA support"
 	depends on ATA && PCI
drivers/ata/Makefile +3 -0
index c17ae9c..61d3e76 100644
@@ -19,3 +19,6 @@ obj-$(CONFIG_PDC_ADMA)		+= pdc_adma.o
 
 libata-objs	:= libata-core.o libata-scsi.o libata-bmdma.o libata-eh.o
 
+ifeq ($(CONFIG_ATA_LEGACY),y)
+libata-objs	+= libata-legacy.o
+endif
drivers/ata/libata-legacy.c new file +241 -0

241 lines not on this page · raw message

drivers/ata/libata.h +11 -0
index 1d24254..0601c4a 100644
@@ -117,6 +117,17 @@ extern void ata_schedule_scsi_eh(struct 
 extern void ata_scsi_dev_rescan(void *data);
 extern int ata_bus_probe(struct ata_port *ap);
 
+/* libata-legacy.c */
+extern int ata_legacy_acquire_resources(struct ata_host *host,
+					const char **p_reason);
+extern void ata_legacy_release_resources(struct ata_host *host);
+extern void ata_legacy_init_ports(struct ata_host *host);
+extern int ata_legacy_request_irqs(struct ata_host *host,
+		irqreturn_t (* const * handler)(int, void *, struct pt_regs *),
+		const unsigned int *irq_flags, void * const *dev_id,
+		const char **p_reason);
+extern void ata_legacy_free_irqs(struct ata_host *host);
+
 /* libata-eh.c */
 extern enum scsi_eh_timer_return ata_scsi_timed_out(struct scsi_cmnd *cmd);
 extern void ata_scsi_error(struct Scsi_Host *host);
include/linux/libata.h +13 -2
index b050517..9535f54 100644
@@ -200,8 +200,16 @@ enum {
 	ATA_QCFLAG_EH_SCHEDULED = (1 << 18), /* EH scheduled (obsolete) */
 
 	/* host set flags */
-	ATA_HOST_SIMPLEX	= (1 << 0),	/* Host is simplex, one DMA channel per host only */
-	
+	ATA_HOST_SIMPLEX	= (1 << 0), /* host is simplex, one DMA channel per host only */
+	ATA_HOST_DEV_BUSY	= (1 << 1), /* host device was busy on init */
+
+	/* legacy flags */
+	/* BIT 0 and 1 are reserved for ATA_PORT_PRIMARY and SECONDARY */
+	ATA_LEGACY_MASK		= (1 << 0) | (1 << 1),
+
+	ATA_LEGACY_RES_PRI	= (1 << 8),
+	ATA_LEGACY_RES_SEC	= (1 << 9),
+
 	/* various lengths of time */
 	ATA_TMOUT_BOOT		= 30 * HZ,	/* heuristic */
 	ATA_TMOUT_BOOT_QUICK	= 7 * HZ,	/* heuristic */
@@ -376,8 +384,11 @@ struct ata_host {
 	void			*private_data;
 	const struct ata_port_operations *ops;
 	unsigned long		flags;
+
 	int			simplex_claimed;	/* Keep seperate in case we
 							   ever need to do this locked */
+	unsigned int		legacy_flags;
+
 	struct list_head	irq_list;	/* list of acquired irqs */
 	struct ata_port		*ports[0];
 };
Signature · 1 line
1.4.1.1
#19 lore raw

Re: [PATCH 6/20] libata: implement legacy ATA init helpers

Quoted 8 lines · Tejun Heo

Implement the following legacy ATA init helpers.

* ata_legacy_acquire_resources() : acquire legacy ATA resources
* ata_legacy_release_resources() : release legacy ATA resources
* ata_legacy_init_ports() : init legacy port addresses
* ata_legacy_request_irqs() : request legacy ATA IRQs
* ata_legacy_free_irqs() : free legacy ATA IRQs

Quoted 5 lines

These helpers can be used independently or called implictly from other bus helpers which has support for legacy ATA device for compatibility (e.g. PCI).

The irq part will change based on comments from previous email

Quoted 76 lines
drivers/ata/Kconfig +5 -0
index 2109d75..1a98559 100644
@@ -15,6 +15,11 @@ config ATA
 	  that "speaks" the ATA protocol, also called ATA controller),
 	  because you will be asked for it.
 
+config ATA_LEGACY
+	bool
+	depends on ATA
+	default n
+
 config SATA_AHCI
 	tristate "AHCI SATA support"
 	depends on ATA && PCI
drivers/ata/Makefile +3 -0
index c17ae9c..61d3e76 100644
@@ -19,3 +19,6 @@ obj-$(CONFIG_PDC_ADMA)		+= pdc_adma.o
 
 libata-objs	:= libata-core.o libata-scsi.o libata-bmdma.o libata-eh.o
 
+ifeq ($(CONFIG_ATA_LEGACY),y)
+libata-objs	+= libata-legacy.o
+endif
drivers/ata/libata-legacy.c new file +43 -0
new file mode 100644
index 0000000..00b70b0
@@ -0,0 +1,241 @@
+/*
+ *  libata-legacy.c - helper library for legacy ATA
+ *
+ *  Maintained by:  Jeff Garzik <[email protected]>
+ *    		    Please ALWAYS copy [email protected]
+ *		    on emails.
+ *
+ *  Copyright 2006 Tejun Heo <[email protected]>
+ *
+ *
+ *  This program is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU General Public License as
+ *  published by the Free Software Foundation; either version 2, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; see the file COPYING.  If not, write to
+ *  the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139,
+ *  USA.
+ *
+ *
+ *  libata documentation is available via 'make {ps|pdf}docs',
+ *  as Documentation/DocBook/libata.*
+ *
+ *  Hardware documentation available from http://www.t13.org/ and
+ *  http://www.sata-io.org/
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/libata.h>
+
+#include "libata.h"
+
+static unsigned long ata_legacy_addr(int port, int sel)
+{
+	if (port == 0) {
+		if (sel == 0)

drivers/ata/libata-legacy.c line 53

Quoted 10 lines
drivers/ata/libata-legacy.c new file +10 -0
+			return ATA_PRIMARY_CMD;
+		else
+			return ATA_PRIMARY_CTL;
+	} else {
+		if (sel == 0)
+			return ATA_SECONDARY_CMD;
+		else
+			return ATA_SECONDARY_CTL;
+	}
+}

insufficient long term, but matches current code so ACK for now.

This will change depending on platform, and for easier x86 chipsets

Quoted 11 lines
drivers/ata/libata-legacy.c new file +11 -0
+static int ata_legacy_acquire_port(struct ata_port *ap)
+{
+	struct ata_host *host = ap->host;
+	unsigned long cmd_addr = ata_legacy_addr(ap->port_no, 0);
+
+	if (request_region(cmd_addr, 8, "libata") != NULL)
+		host->legacy_flags |= ATA_LEGACY_RES_PRI << ap->port_no;
+	else {
+		struct resource *conflict, res;
+
+		res.start = cmd_addr;

drivers/ata/libata-legacy.c line 75

Quoted 10 lines
drivers/ata/libata-legacy.c new file +10 -0
+		res.end = cmd_addr + 8 - 1;
+		conflict = ____request_resource(&ioport_resource, &res);
+
+		if (strcmp(conflict->name, "libata")) {
+			printk(KERN_WARNING "ata: 0x%0lX IDE port busy\n",
+			       cmd_addr);
+			host->flags |= ATA_HOST_DEV_BUSY;
+			return -EBUSY;
+		}
+		printk("ata: 0x%0lX IDE port preallocated\n", cmd_addr);

obviously needs updating based on the patch I just applied to libata#upstream from Arnaud @ Mandriva

Quoted 188 lines
drivers/ata/libata-legacy.c new file +161 -0
+/**
+ *	ata_legacy_acquire_resources - acquire legacy ATA resources
+ *	@host: target ATA host
+ *	@p_reason: out arg for error message (can be NULL)
+ *
+ *	Acquire legacy ATA resources for ports specified by
+ *	ATA_PORT_PRIMARY/SECONDARY mask in host->legacy_flags.  If the
+ *	port is busy, this function makes the port dummy instead of
+ *	failing.  This is to allow legacy ports to be driven by other
+ *	drivers.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ */
+int ata_legacy_acquire_resources(struct ata_host *host, const char **p_reason)
+{
+	int i;
+
+	for (i = 0; i < 2; i++) {
+		if (!(host->legacy_flags & (ATA_PORT_PRIMARY << i)))
+			continue;
+		BUG_ON(i >= host->n_ports);
+
+		if (ata_legacy_acquire_port(host->ports[i]))
+			host->ports[i]->ops = &ata_dummy_port_ops;
+	}
+
+	return 0;
+}
+
+/**
+ *	ata_legacy_release_resources - release legacy ATA resources
+ *	@host: target ATA host
+ *
+ *	Free all legacy ATA resources @host is holding.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+void ata_legacy_release_resources(struct ata_host *host)
+{
+	int i;
+
+	for (i = 0; i < 2; i++) {
+		if (host->legacy_flags & (ATA_LEGACY_RES_PRI << i)) {
+			release_region(ata_legacy_addr(i, 0), 8);
+			host->legacy_flags &= ~(ATA_LEGACY_RES_PRI << i);
+		}
+	}
+}
+
+/**
+ *	ata_legacy_init_ports - initialize legacy ATA port addresses
+ *	@host: target ATA host
+ *
+ *	Initialize legacy ATA port addresses for non-dummy legacy
+ *	ports in @host.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer.
+ */
+void ata_legacy_init_ports(struct ata_host *host)
+{
+	int i;
+
+	for (i = 0; i < 2; i++) {
+		struct ata_port *ap = host->ports[i];
+
+		if (host->legacy_flags & (ATA_PORT_PRIMARY << i) &&
+		    !ata_port_is_dummy(ap)) {
+			struct ata_ioports *ioaddr = &ap->ioaddr;
+			unsigned long cmd_addr = ata_legacy_addr(i, 0);
+			unsigned long ctl_addr = ata_legacy_addr(i, 1);
+
+			ioaddr->cmd_addr = cmd_addr;
+			ioaddr->altstatus_addr = ctl_addr;
+			ioaddr->ctl_addr = ctl_addr;
+			ata_std_ports(ioaddr);
+		}
+	}
+}
+
+/**
+ *	ata_legacy_request_irqs - request legacy ATA IRQs
+ *	@host: target ATA host
+ *	@handler: array of IRQ handlers
+ *	@irq_flags: array of IRQ flags
+ *	@dev_id: array of IRQ dev_ids
+ *	@p_reason: out arg for error message (can be NULL)
+ *
+ *	Request legacy IRQs for non-dummy legacy ports in @host.  All
+ *	IRQ parameters are passed as array to allow ports to have
+ *	separate IRQ handlers.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ */
+int ata_legacy_request_irqs(struct ata_host *host,
+		irqreturn_t (* const * handler)(int, void *, struct pt_regs *),
+		const unsigned int *irq_flags, void * const *dev_id,
+		const char **p_reason)
+{
+	const unsigned int legacy_irq[] = { 14, 15 };
+	const char *reason;
+	int i, rc;
+
+	for (i = 0; i < 2; i++) {
+		struct ata_port *ap = host->ports[i];
+		unsigned int irq = legacy_irq[i];
+
+		if (!(host->legacy_flags & (ATA_PORT_PRIMARY << i)) ||
+		    ata_port_is_dummy(ap))
+			continue;
+
+		if (!handler[i]) {
+			reason = "NULL handler";
+			rc = -EINVAL;
+			goto err;
+		}
+
+		rc = ata_host_request_irq_marker(host, irq,
+					handler[i], irq_flags[i], dev_id[i],
+					ata_legacy_request_irqs, &reason);
+		if (rc)
+			goto err;
+
+		/* only for info printing */
+		if (i == 0)
+			host->irq = irq;
+		else
+			host->irq2 = irq;
+	}
+
+	return 0;
+
+ err:
+	ata_legacy_free_irqs(host);
+	if (p_reason)
+		*p_reason = reason;
+	return rc;
+}
+
+/**
+ *	ata_legacy_free_irqs - free legacy ATA IRQs
+ *	@host: target ATA host
+ *
+ *	Free all legacy ATA IRQs of @host.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+void ata_legacy_free_irqs(struct ata_host *host)
+{
+	ata_host_free_irqs_marker(host, ata_legacy_request_irqs);
+}
drivers/ata/libata.h +11 -0
index 1d24254..0601c4a 100644
@@ -117,6 +117,17 @@ extern void ata_schedule_scsi_eh(struct 
 extern void ata_scsi_dev_rescan(void *data);
 extern int ata_bus_probe(struct ata_port *ap);
 
+/* libata-legacy.c */
+extern int ata_legacy_acquire_resources(struct ata_host *host,
+					const char **p_reason);
+extern void ata_legacy_release_resources(struct ata_host *host);
+extern void ata_legacy_init_ports(struct ata_host *host);
+extern int ata_legacy_request_irqs(struct ata_host *host,
+		irqreturn_t (* const * handler)(int, void *, struct pt_regs *),
+		const unsigned int *irq_flags, void * const *dev_id,
+		const char **p_reason);
+extern void ata_legacy_free_irqs(struct ata_host *host);
+
 /* libata-eh.c */
 extern enum scsi_eh_timer_return ata_scsi_timed_out(struct scsi_cmnd *cmd);
 extern void ata_scsi_error(struct Scsi_Host *host);
include/linux/libata.h +0 -0
index b050517..9535f54 100644

include/linux/libata.h enum { · line 204

Quoted 7 lines
include/linux/libata.h +2 -2
@@ -200,8 +200,16 @@ enum {
 	ATA_QCFLAG_EH_SCHEDULED = (1 << 18), /* EH scheduled (obsolete) */
 
 	/* host set flags */
-	ATA_HOST_SIMPLEX	= (1 << 0),	/* Host is simplex, one DMA channel per host only */
-	
+	ATA_HOST_SIMPLEX	= (1 << 0), /* host is simplex, one DMA channel per host only */
+	ATA_HOST_DEV_BUSY	= (1 << 1), /* host device was busy on init */

this flag is set but never used. doesn't seem necessary anyway... just return an error code.

author
#20 lore raw

PATCH 7/20 libata: implement PCI ATA init helpers

In reply to Tejun Heo

Implement the following PCI ATA init helpers

* ata_pci_legacy_mask() : obtain legacy mask from PCI header
* ata_pci_set_dma_mask() : PCI DMA mask set helper
* ata_pci_acquire_resources() : acquire generic PCI resources
* ata_pci_release_resources() : release generic PCI resources
* ata_pci_init_ports() : init native PCI ATA port addresses
* ata_pci_request_irq() : request ATA PCI IRQ with MSI/INTX handling
* ata_pci_free_irq() : free ATA PCI IRQ
* ata_pci_request_msix_irqs() : request ATA PCI MSIX IRQs
* ata_pci_free_msix_irqs() : free ATA PCI MSIX IRQs
* ata_pci_host_destroy() : PCI ATA host self-destruct

Legacy handling isn't yet integrated.

These helpers will be used in new LLD init model.

---
 drivers/ata/Kconfig      |   26 +--
 drivers/ata/Makefile     |    4 
 drivers/ata/libata-pci.c |  442 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/libata.h   |   35 ++++
 4 files changed, 491 insertions(+), 16 deletions(-)
drivers/ata/Kconfig +10 -16
index 1a98559..38a3f79 100644
@@ -20,9 +20,16 @@ config ATA_LEGACY
 	depends on ATA
 	default n
 
+config ATA_PCI
+	bool
+	depends on ATA && PCI
+	default y
+	select ATA_LEGACY
+
+if ATA_PCI
+
 config SATA_AHCI
 	tristate "AHCI SATA support"
-	depends on ATA && PCI
 	help
 	  This option enables support for AHCI Serial ATA.
 
@@ -30,7 +37,6 @@ config SATA_AHCI
 
 config SATA_SVW
 	tristate "ServerWorks Frodo / Apple K2 SATA support"
-	depends on ATA && PCI
 	help
 	  This option enables support for Broadcom/Serverworks/Apple K2
 	  SATA support.
@@ -39,7 +45,6 @@ config SATA_SVW
 
 config ATA_PIIX
 	tristate "Intel PIIX/ICH SATA support"
-	depends on ATA && PCI
 	help
 	  This option enables support for ICH5/6/7/8 Serial ATA.
 	  If PATA support was enabled previously, this enables
@@ -49,7 +54,6 @@ config ATA_PIIX
 
 config SATA_MV
 	tristate "Marvell SATA support (HIGHLY EXPERIMENTAL)"
-	depends on ATA && PCI && EXPERIMENTAL
 	help
 	  This option enables support for the Marvell Serial ATA family.
 	  Currently supports 88SX[56]0[48][01] chips.
@@ -58,7 +62,6 @@ config SATA_MV
 
 config SATA_NV
 	tristate "NVIDIA SATA support"
-	depends on ATA && PCI
 	help
 	  This option enables support for NVIDIA Serial ATA.
 
@@ -66,7 +69,6 @@ config SATA_NV
 
 config PDC_ADMA
 	tristate "Pacific Digital ADMA support"
-	depends on ATA && PCI
 	help
 	  This option enables support for Pacific Digital ADMA controllers
 
@@ -74,7 +76,6 @@ config PDC_ADMA
 
 config SATA_QSTOR
 	tristate "Pacific Digital SATA QStor support"
-	depends on ATA && PCI
 	help
 	  This option enables support for Pacific Digital Serial ATA QStor.
 
@@ -82,7 +83,6 @@ config SATA_QSTOR
 
 config SATA_PROMISE
 	tristate "Promise SATA TX2/TX4 support"
-	depends on ATA && PCI
 	help
 	  This option enables support for Promise Serial ATA TX2/TX4.
 
@@ -90,7 +90,6 @@ config SATA_PROMISE
 
 config SATA_SX4
 	tristate "Promise SATA SX4 support"
-	depends on ATA && PCI && EXPERIMENTAL
 	help
 	  This option enables support for Promise Serial ATA SX4.
 
@@ -98,7 +97,6 @@ config SATA_SX4
 
 config SATA_SIL
 	tristate "Silicon Image SATA support"
-	depends on ATA && PCI
 	help
 	  This option enables support for Silicon Image Serial ATA.
 
@@ -106,7 +104,6 @@ config SATA_SIL
 
 config SATA_SIL24
 	tristate "Silicon Image 3124/3132 SATA support"
-	depends on ATA && PCI
 	help
 	  This option enables support for Silicon Image 3124/3132 Serial ATA.
 
@@ -114,7 +111,6 @@ config SATA_SIL24
 
 config SATA_SIS
 	tristate "SiS 964/180 SATA support"
-	depends on ATA && PCI
 	help
 	  This option enables support for SiS Serial ATA 964/180.
 
@@ -122,7 +118,6 @@ config SATA_SIS
 
 config SATA_ULI
 	tristate "ULi Electronics SATA support"
-	depends on ATA && PCI
 	help
 	  This option enables support for ULi Electronics SATA.
 
@@ -130,7 +125,6 @@ config SATA_ULI
 
 config SATA_VIA
 	tristate "VIA SATA support"
-	depends on ATA && PCI
 	help
 	  This option enables support for VIA Serial ATA.
 
@@ -138,7 +132,6 @@ config SATA_VIA
 
 config SATA_VITESSE
 	tristate "VITESSE VSC-7174 / INTEL 31244 SATA support"
-	depends on ATA && PCI
 	help
 	  This option enables support for Vitesse VSC7174 and Intel 31244 Serial ATA.
 
@@ -149,5 +142,6 @@ config SATA_INTEL_COMBINED
 	depends on IDE=y && !BLK_DEV_IDE_SATA && (SATA_AHCI || ATA_PIIX)
 	default y
 
-endmenu
+endif
 
+endmenu
drivers/ata/Makefile +4 -0
index 61d3e76..72ce589 100644
@@ -22,3 +22,7 @@ libata-objs	:= libata-core.o libata-scsi
 ifeq ($(CONFIG_ATA_LEGACY),y)
 libata-objs	+= libata-legacy.o
 endif
+
+ifeq ($(CONFIG_ATA_PCI),y)
+libata-objs	+= libata-pci.o
+endif
drivers/ata/libata-pci.c new file +442 -0

442 lines not on this page · raw message

include/linux/libata.h +35 -0
index 9535f54..f25fc53 100644
@@ -210,6 +210,15 @@ enum {
 	ATA_LEGACY_RES_PRI	= (1 << 8),
 	ATA_LEGACY_RES_SEC	= (1 << 9),
 
+	/* PCI flags */
+	ATA_PCI_INTX		= (1 << 0), /* do INTX */
+	ATA_PCI_MSI		= (1 << 1), /* PCI IRQ via MSI */
+
+	ATA_PCI_RES_GEN		= (1 << 8), /* generic PCI resources */
+	ATA_PCI_RES_INTX	= (1 << 9), /* did INTX */
+	ATA_PCI_RES_MSI		= (1 << 10), /* is MSI */
+	ATA_PCI_RES_MSIX	= (1 << 11), /* is MSIX, INTX is implied */
+
 	/* various lengths of time */
 	ATA_TMOUT_BOOT		= 30 * HZ,	/* heuristic */
 	ATA_TMOUT_BOOT_QUICK	= 7 * HZ,	/* heuristic */
@@ -374,6 +383,14 @@ struct ata_probe_ent {
 	void			*private_data;
 };
 
+struct ata_msix_entry {
+	u16			entry;		/* in arg, msix table index */
+	u16			vector;		/* out arg, mapped vector */
+	irqreturn_t		(*handler)(int, void *, struct pt_regs *);
+	unsigned int		flags;
+	void			*dev_id;
+};
+
 struct ata_host {
 	spinlock_t		lock;
 	struct device 		*dev;
@@ -388,6 +405,7 @@ struct ata_host {
 	int			simplex_claimed;	/* Keep seperate in case we
 							   ever need to do this locked */
 	unsigned int		legacy_flags;
+	unsigned int		pci_flags;
 
 	struct list_head	irq_list;	/* list of acquired irqs */
 	struct ata_port		*ports[0];
@@ -694,6 +712,23 @@ extern int ata_dev_revalidate(struct ata
 extern void ata_port_disable(struct ata_port *);
 extern void ata_std_ports(struct ata_ioports *ioaddr);
 #ifdef CONFIG_PCI
+extern unsigned int ata_pci_legacy_mask(struct pci_dev *pdev);
+extern int ata_pci_set_dma_mask(struct pci_dev *pdev, u64 dma_mask,
+				const char **p_reason);
+extern int ata_pci_acquire_resources(struct ata_host *host, u64 dma_mask,
+				     const char **p_reason);
+extern void ata_pci_release_resources(struct ata_host *host);
+extern void ata_pci_init_ports(struct ata_host *host);
+extern int ata_pci_request_irq(struct ata_host *host,
+			irqreturn_t (*handler)(int, void *, struct pt_regs *),
+			unsigned int irq_flags, void *dev_id,
+			const char **p_reason);
+extern void ata_pci_free_irq(struct ata_host *host);
+extern int ata_pci_request_msix_irqs(struct ata_host *host,
+			      struct ata_msix_entry *ata_msix_tbl, int nr_irqs,
+			      const char **p_reason);
+extern void ata_pci_free_msix_irqs(struct ata_host *host);
+extern void ata_pci_host_destroy(struct ata_host *host);
 extern int ata_pci_init_one (struct pci_dev *pdev, struct ata_port_info **port_info,
 			     unsigned int n_ports);
 extern void ata_pci_remove_one (struct pci_dev *pdev);
Signature · 1 line
1.4.1.1
#21 lore raw

Re: [PATCH 7/20] libata: implement PCI ATA init helpers

Quoted 28 lines · Tejun Heo
drivers/ata/libata-pci.c new file +28 -0
+int ata_pci_set_dma_mask(struct pci_dev *pdev, u64 dma_mask,
+			 const char **p_reason)
+{
+	const char *reason;
+	int rc = 0;
+
+	if (dma_mask == DMA_64BIT_MASK) {
+		if (pci_set_dma_mask(pdev, DMA_64BIT_MASK) == 0) {
+			rc = pci_set_consistent_dma_mask(pdev, DMA_64BIT_MASK);
+			if (rc) {
+				rc = pci_set_consistent_dma_mask(pdev,
+							DMA_32BIT_MASK);
+				if (rc) {
+					reason = "64-bit DMA enable failed";
+					goto err;
+				}
+			}
+		} else {
+			rc = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
+			if (rc) {
+				reason = "32-bit DMA enable failed";
+				goto err;
+			}
+			rc = pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK);
+			if (rc) {
+				reason = "32-bit consistent DMA enable failed";
+				goto err;
+			}

drivers/ata/libata-pci.c line 122

Quoted 10 lines
drivers/ata/libata-pci.c new file +10 -0
+		}
+	} else if (dma_mask) {
+		reason = "failed to set DMA mask";
+		rc = pci_set_dma_mask(pdev, dma_mask);
+		if (rc)
+			goto err;
+		rc = pci_set_consistent_dma_mask(pdev, dma_mask);
+		if (rc)
+			goto err;
+	}

else return -EINVAL, I suppose

Quoted 123 lines
drivers/ata/libata-pci.c new file +123 -0
+	return 0;
+
+ err:
+	if (p_reason)
+		*p_reason = reason;
+	return rc;
+}
+
+/**
+ *	ata_pci_acquire_resources - acquire default PCI resources
+ *	@host: target ATA host to acquire PCI resources for
+ *	@dma_mask: DMA mask
+ *	@p_reason: out arg for error message (can be NULL)
+ *
+ *	Acquire default ATA PCI resources.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ */
+int ata_pci_acquire_resources(struct ata_host *host, u64 dma_mask,
+			      const char **p_reason)
+{
+	struct pci_dev *pdev = to_pci_dev(host->dev);
+	const char *reason;
+	int rc;
+
+	/* acquire generic resources */
+
+	/* FIXME: Really for ATA it isn't safe because the device may
+	 * be multi-purpose and we want to leave it alone if it was
+	 * already enabled. Secondly for shared use as Arjan says we
+	 * want refcounting
+	 *
+	 * Checking dev->is_enabled is insufficient as this is not set
+	 * at boot for the primary video which is BIOS enabled
+         */
+	rc = pci_enable_device(pdev);
+	if (rc) {
+		reason = "failed to enable PCI device";
+		goto err;
+	}
+
+	rc = pci_request_regions(pdev, DRV_NAME);
+	if (rc) {
+		host->flags |= ATA_HOST_DEV_BUSY;
+		reason = "failed to request PCI regions";
+		goto err;
+	}
+
+	host->pci_flags |= ATA_PCI_RES_GEN;
+
+	/* set DMA mask */
+	/* FIXME: If we get no DMA mask we should fall back to PIO */
+	rc = ata_pci_set_dma_mask(pdev, dma_mask, &reason);
+	if (rc)
+		goto err;
+
+	return 0;
+
+ err:
+	ata_pci_release_resources(host);
+	if (p_reason)
+		*p_reason = reason;
+	return rc;
+}
+
+/**
+ *	ata_pci_release_resources - release default PCI resources
+ *	@host: target ATA host to release PCI resources for
+ *
+ *	Release default ATA PCI resources.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+void ata_pci_release_resources(struct ata_host *host)
+{
+	struct pci_dev *pdev = to_pci_dev(host->dev);
+
+	if (host->pci_flags & ATA_PCI_RES_GEN) {
+		pci_release_regions(pdev);
+
+		if (!(host->flags & ATA_HOST_DEV_BUSY))
+			pci_disable_device(pdev);
+
+		host->pci_flags &= ~ATA_PCI_RES_GEN;
+	}
+}
+
+/**
+ *	ata_pci_init_ports - initialize PCI ATA port addresses
+ *	@host: target ATA host
+ *
+ *	Initialize native ATA port TF addresses and PCI BMDMA
+ *	addresses for both legacy and native ports.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer.
+ */
+void ata_pci_init_ports(struct ata_host *host)
+{
+	struct pci_dev *pdev = to_pci_dev(host->dev);
+	int i;
+
+	/* initialize native TF and BMDMA */
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+		struct ata_ioports *ioaddr = &ap->ioaddr;
+		unsigned long cmd_addr, ctl_addr, bmdma_addr;
+
+		cmd_addr = pci_resource_start(pdev, ap->port_no * 2);
+		ctl_addr = pci_resource_start(pdev, ap->port_no * 2 + 1) |
+			ATA_PCI_CTL_OFS;
+		bmdma_addr = pci_resource_start(pdev, 4);
+
+		if (!((host->legacy_flags & ATA_LEGACY_MASK) & (1 << i))) {
+			ioaddr->cmd_addr = cmd_addr;
+			ioaddr->altstatus_addr = ctl_addr;
+			ioaddr->ctl_addr = ctl_addr;
+			ata_std_ports(ioaddr);

drivers/ata/libata-pci.c line 256

Quoted 10 lines
drivers/ata/libata-pci.c new file +10 -0
+		}
+
+		if (bmdma_addr) {
+			bmdma_addr += ap->port_no * 8;
+			if (inb(bmdma_addr + 2) & 0x80)
+				host->flags |= ATA_HOST_SIMPLEX;
+			ioaddr->bmdma_addr = bmdma_addr;
+		}
+	}
+}

I don't see where the legacy resource handling is?

For PCI devices in legacy, or mixed legacy+native mode, the above code should be calling into ata-legacy.c to reserve resources.

author
#22 lore raw

PATCH 8/20 libata: reimplement ata_pci_init_one() using new init helpers

In reply to Tejun Heo

Reimplement ata_pci_init_one() using new init helpers. New implementation is split into to functions - ata_pci_host_prepare() and ata_pci_init_one(). ata_pci_host_prepare() prepares host for the specified pdev. The split is to give LLDs more flexibility.

ata_pci_host_prepare() can handle all combinations of native and legacy ports, but ata_pci_init_one() still applies the same restriction as before for compatibility.

New implementation properly releases legacy resources on remove. Other than that, there's no behavior change.

---
 drivers/ata/libata-bmdma.c |  204 --------------------------------------------
 drivers/ata/libata-core.c  |    1 
 drivers/ata/libata-pci.c   |  191 +++++++++++++++++++++++++++++++++++++++++
 include/linux/libata.h     |    4 +
 4 files changed, 195 insertions(+), 205 deletions(-)
drivers/ata/libata-bmdma.c +0 -204
index 7605028..672df5a 100644
@@ -865,210 +865,6 @@ ata_pci_init_native_mode(struct pci_dev 
 	return probe_ent;
 }
 
-
-static struct ata_probe_ent *ata_pci_init_legacy_port(struct pci_dev *pdev,
-				struct ata_port_info **port, int port_mask)
-{
-	struct ata_probe_ent *probe_ent;
-	unsigned long bmdma = pci_resource_start(pdev, 4);
-
-	probe_ent = ata_probe_ent_alloc(pci_dev_to_dev(pdev), port[0]);
-	if (!probe_ent)
-		return NULL;
-
-	probe_ent->n_ports = 2;
-	probe_ent->private_data = port[0]->private_data;
-
-	if (port_mask & ATA_PORT_PRIMARY) {
-		probe_ent->irq = 14;
-		probe_ent->port[0].cmd_addr = ATA_PRIMARY_CMD;
-		probe_ent->port[0].altstatus_addr =
-		probe_ent->port[0].ctl_addr = ATA_PRIMARY_CTL;
-		if (bmdma) {
-			probe_ent->port[0].bmdma_addr = bmdma;
-			if (inb(bmdma + 2) & 0x80)
-				probe_ent->_host_flags |= ATA_HOST_SIMPLEX;
-		}
-		ata_std_ports(&probe_ent->port[0]);
-	} else
-		probe_ent->dummy_port_mask |= ATA_PORT_PRIMARY;
-
-	if (port_mask & ATA_PORT_SECONDARY) {
-		if (probe_ent->irq)
-			probe_ent->irq2 = 15;
-		else
-			probe_ent->irq = 15;
-		probe_ent->port[1].cmd_addr = ATA_SECONDARY_CMD;
-		probe_ent->port[1].altstatus_addr =
-		probe_ent->port[1].ctl_addr = ATA_SECONDARY_CTL;
-		if (bmdma) {
-			probe_ent->port[1].bmdma_addr = bmdma + 8;
-			if (inb(bmdma + 10) & 0x80)
-				probe_ent->_host_flags |= ATA_HOST_SIMPLEX;
-		}
-		ata_std_ports(&probe_ent->port[1]);
-	} else
-		probe_ent->dummy_port_mask |= ATA_PORT_SECONDARY;
-
-	return probe_ent;
-}
-
-
-/**
- *	ata_pci_init_one - Initialize/register PCI IDE host controller
- *	@pdev: Controller to be initialized
- *	@port_info: Information from low-level host driver
- *	@n_ports: Number of ports attached to host controller
- *
- *	This is a helper function which can be called from a driver's
- *	xxx_init_one() probe function if the hardware uses traditional
- *	IDE taskfile registers.
- *
- *	This function calls pci_enable_device(), reserves its register
- *	regions, sets the dma mask, enables bus master mode, and calls
- *	ata_device_add()
- *
- *	ASSUMPTION:
- *	Nobody makes a single channel controller that appears solely as
- *	the secondary legacy port on PCI.
- *
- *	LOCKING:
- *	Inherited from PCI layer (may sleep).
- *
- *	RETURNS:
- *	Zero on success, negative on errno-based value on error.
- */
-
-int ata_pci_init_one (struct pci_dev *pdev, struct ata_port_info **port_info,
-		      unsigned int n_ports)
-{
-	struct ata_probe_ent *probe_ent = NULL;
-	struct ata_port_info *port[2];
-	u8 tmp8, mask;
-	unsigned int legacy_mode = 0;
-	int disable_dev_on_err = 1;
-	int rc;
-
-	DPRINTK("ENTER\n");
-
-	port[0] = port_info[0];
-	if (n_ports > 1)
-		port[1] = port_info[1];
-	else
-		port[1] = port[0];
-
-	if ((port[0]->flags & ATA_FLAG_NO_LEGACY) == 0
-	    && (pdev->class >> 8) == PCI_CLASS_STORAGE_IDE) {
-		/* TODO: What if one channel is in native mode ... */
-		pci_read_config_byte(pdev, PCI_CLASS_PROG, &tmp8);
-		mask = (1 << 2) | (1 << 0);
-		if ((tmp8 & mask) != mask)
-			legacy_mode = (1 << 3);
-	}
-
-	/* FIXME... */
-	if ((!legacy_mode) && (n_ports > 2)) {
-		printk(KERN_ERR "ata: BUG: native mode, n_ports > 2\n");
-		n_ports = 2;
-		/* For now */
-	}
-
-	/* FIXME: Really for ATA it isn't safe because the device may be
-	   multi-purpose and we want to leave it alone if it was already
-	   enabled. Secondly for shared use as Arjan says we want refcounting
-
-	   Checking dev->is_enabled is insufficient as this is not set at
-	   boot for the primary video which is BIOS enabled
-         */
-
-	rc = pci_enable_device(pdev);
-	if (rc)
-		return rc;
-
-	rc = pci_request_regions(pdev, DRV_NAME);
-	if (rc) {
-		disable_dev_on_err = 0;
-		goto err_out;
-	}
-
-	if (legacy_mode) {
-		if (!request_region(ATA_PRIMARY_CMD, 8, "libata")) {
-			struct resource *conflict, res;
-			res.start = ATA_PRIMARY_CMD;
-			res.end = ATA_PRIMARY_CMD + 8 - 1;
-			conflict = ____request_resource(&ioport_resource, &res);
-			if (!strcmp(conflict->name, "libata"))
-				legacy_mode |= ATA_PORT_PRIMARY;
-			else {
-				disable_dev_on_err = 0;
-				printk(KERN_WARNING "ata: 0x%0X IDE port busy\n", ATA_PRIMARY_CMD);
-			}
-		} else
-			legacy_mode |= ATA_PORT_PRIMARY;
-
-		if (!request_region(ATA_SECONDARY_CMD, 8, "libata")) {
-			struct resource *conflict, res;
-			res.start = ATA_SECONDARY_CMD;
-			res.end = ATA_SECONDARY_CMD + 8 - 1;
-			conflict = ____request_resource(&ioport_resource, &res);
-			if (!strcmp(conflict->name, "libata"))
-				legacy_mode |= ATA_PORT_SECONDARY;
-			else {
-				disable_dev_on_err = 0;
-				printk(KERN_WARNING "ata: 0x%X IDE port busy\n", ATA_SECONDARY_CMD);
-			}
-		} else
-			legacy_mode |= ATA_PORT_SECONDARY;
-	}
-
-	/* we have legacy mode, but all ports are unavailable */
-	if (legacy_mode == (1 << 3)) {
-		rc = -EBUSY;
-		goto err_out_regions;
-	}
-
-	/* FIXME: If we get no DMA mask we should fall back to PIO */
-	rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
-	if (rc)
-		goto err_out_regions;
-	rc = pci_set_consistent_dma_mask(pdev, ATA_DMA_MASK);
-	if (rc)
-		goto err_out_regions;
-
-	if (legacy_mode) {
-		probe_ent = ata_pci_init_legacy_port(pdev, port, legacy_mode);
-	} else {
-		if (n_ports == 2)
-			probe_ent = ata_pci_init_native_mode(pdev, port, ATA_PORT_PRIMARY | ATA_PORT_SECONDARY);
-		else
-			probe_ent = ata_pci_init_native_mode(pdev, port, ATA_PORT_PRIMARY);
-	}
-	if (!probe_ent) {
-		rc = -ENOMEM;
-		goto err_out_regions;
-	}
-
-	pci_set_master(pdev);
-
-	/* FIXME: check ata_device_add return */
-	ata_device_add(probe_ent);
-
-	kfree(probe_ent);
-
-	return 0;
-
-err_out_regions:
-	if (legacy_mode & ATA_PORT_PRIMARY)
-		release_region(ATA_PRIMARY_CMD, 8);
-	if (legacy_mode & ATA_PORT_SECONDARY)
-		release_region(ATA_SECONDARY_CMD, 8);
-	pci_release_regions(pdev);
-err_out:
-	if (disable_dev_on_err)
-		pci_disable_device(pdev);
-	return rc;
-}
-
 /**
  *	ata_pci_clear_simplex	-	attempt to kick device out of simplex
  *	@pdev: PCI device
drivers/ata/libata-core.c +0 -1
index b293d47..0509935 100644
@@ -6651,7 +6651,6 @@ #ifdef CONFIG_PCI
 EXPORT_SYMBOL_GPL(pci_test_config_bits);
 EXPORT_SYMBOL_GPL(ata_pci_host_stop);
 EXPORT_SYMBOL_GPL(ata_pci_init_native_mode);
-EXPORT_SYMBOL_GPL(ata_pci_init_one);
 EXPORT_SYMBOL_GPL(ata_pci_remove_one);
 EXPORT_SYMBOL_GPL(ata_pci_device_do_suspend);
 EXPORT_SYMBOL_GPL(ata_pci_device_do_resume);
drivers/ata/libata-pci.c +191 -0

217 lines not on this page · raw message

include/linux/libata.h +4 -0
index f25fc53..3705d09 100644
@@ -728,6 +728,10 @@ extern int ata_pci_request_msix_irqs(str
 			      struct ata_msix_entry *ata_msix_tbl, int nr_irqs,
 			      const char **p_reason);
 extern void ata_pci_free_msix_irqs(struct ata_host *host);
+extern int ata_pci_host_prepare(struct pci_dev *pdev,
+				struct ata_port_info **pinfo_ar,
+				unsigned int mask, unsigned int legacy_mask,
+				struct ata_host **r_host);
 extern void ata_pci_host_destroy(struct ata_host *host);
 extern int ata_pci_init_one (struct pci_dev *pdev, struct ata_port_info **port_info,
 			     unsigned int n_ports);
Signature · 1 line
1.4.1.1
#23 lore raw

Re: [PATCH 8/20] libata: reimplement ata_pci_init_one() using new init helpers

Tejun Heo wrote:

Nevermind previous question about legacy resources.

It does seem like the change is split across these two patches though...
so the previous patch is incorrect without this one.

Quoted 5 lines
drivers/ata/libata-pci.c +5 -0
+int ata_pci_init_one(struct pci_dev *pdev, struct ata_port_info **pinfo_ar,
+		     unsigned int n_ports)
+
+{
+	unsigned int mask = ATA_PORT_PRIMARY | ATA_PORT_SECONDARY;

drivers/ata/libata-pci.c void ata_pci_host_destroy(struct ata_hos · line 596

Quoted 10 lines
drivers/ata/libata-pci.c +10 -0
+	unsigned int legacy_mask = 0;
+	struct ata_host *host;
+	int rc;
+
+	DPRINTK("ENTER\n");
+
+	/* legacy mode? */
+	if (!(pinfo_ar[0]->flags & ATA_FLAG_NO_LEGACY))
+		legacy_mask = ata_pci_legacy_mask(pdev);
+

NAK -- you can't do anything with the PCI device before calling pci_enable_device()

author
#24 lore raw

Re: [PATCH 8/20] libata: reimplement ata_pci_init_one() using new init helpers

Agreed on all your comments for 6 & 7.

Quoted 6 lines · Jeff Garzik

Tejun Heo wrote:

Nevermind previous question about legacy resources.

It does seem like the change is split across these two patches though...
so the previous patch is incorrect without this one.

Yeap, that was more of logical devision and the code isn't yet activated.

Quoted 10 lines
Quoted 10 lines

+int ata_pci_init_one(struct pci_dev *pdev, struct ata_port_info **pinfo_ar,

drivers/ata/libata-pci.c +8 -0
+             unsigned int n_ports)
+
+{
+    unsigned int mask = ATA_PORT_PRIMARY | ATA_PORT_SECONDARY;
+    unsigned int legacy_mask = 0;
+    struct ata_host *host;
+    int rc;
+
Quoted 8 lines

drivers/ata/libata-pci.c void ata_pci_host_destroy(struct ata_hos · line 596

Quoted 6 lines
drivers/ata/libata-pci.c +6 -0
+    DPRINTK("ENTER\n");
+
+    /* legacy mode? */
+    if (!(pinfo_ar[0]->flags & ATA_FLAG_NO_LEGACY))
+        legacy_mask = ata_pci_legacy_mask(pdev);
+

NAK -- you can't do anything with the PCI device before calling pci_enable_device()

This is what we're currently doing in ata_pci_init_one(). I thought the assumption wass that any device which can act as a legacy device must have been initialized by BIOS and thus is safe to access. If not, moving this backward isn't difficult. FWIW, ata_pci_legacy_mask() only performs PCI-configuration access.

Thanks.

Signature · 1 line
tejun
#25 lore raw

Re: [PATCH 8/20] libata: reimplement ata_pci_init_one() using new init helpers

Quoted 4 lines · Tejun Heo
Quoted 1 line · Jeff Garzik

NAK -- you can't do anything with the PCI device before calling pci_enable_device()

This is what we're currently doing in ata_pci_init_one(). I thought the

You just found a bug in the current code :)

author
#26 lore raw

PATCH 10/20 libata: reimplement ata_pci_remove_one() using new PCI init helpers

In reply to Tejun Heo

Reimplement ata_pci_remove_one() using new PCI init helpers.

* drvdata clearing is moved to ata_host_detach().

* if (!ops->host_stop) mmio_base is iounmapped. This makes mmio_base release libata-pci's responsibility and will ease following host_stop and iomap updates. This behavior is temporary.

* unused ata_host_remove() is killed.

---
 drivers/ata/libata-core.c |   72 +--------------------------------------------
 drivers/ata/libata-pci.c  |   29 ++++++++++++++++++
 include/linux/libata.h    |    1 -
 3 files changed, 31 insertions(+), 71 deletions(-)
drivers/ata/libata-core.c +2 -70
index 0509935..ad32d35 100644
@@ -6108,6 +6108,8 @@ void ata_host_detach(struct ata_host *ho
 
 	for (i = 0; i < host->n_ports; i++)
 		ata_port_detach(host->ports[i]);
+
+	dev_set_drvdata(host->dev, NULL);
 }
 
 /**
@@ -6212,48 +6214,6 @@ void ata_host_free(struct ata_host *host
 	kfree(host);
 }
 
-/**
- *	ata_host_remove - PCI layer callback for device removal
- *	@host: ATA host set that was removed
- *
- *	Unregister all objects associated with this host set. Free those
- *	objects.
- *
- *	LOCKING:
- *	Inherited from calling layer (may sleep).
- */
-void ata_host_remove(struct ata_host *host)
-{
-	unsigned int i;
-
-	ata_host_detach(host);
-
-	free_irq(host->irq, host);
-	if (host->irq2)
-		free_irq(host->irq2, host);
-
-	ata_host_stop(host);
-
-	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap = host->ports[i];
-
-		if ((ap->flags & ATA_FLAG_NO_LEGACY) == 0) {
-			struct ata_ioports *ioaddr = &ap->ioaddr;
-
-			/* FIXME: Add -ac IDE pci mods to remove these special cases */
-			if (ioaddr->cmd_addr == ATA_PRIMARY_CMD)
-				release_region(ATA_PRIMARY_CMD, 8);
-			else if (ioaddr->cmd_addr == ATA_SECONDARY_CMD)
-				release_region(ATA_SECONDARY_CMD, 8);
-		}
-	}
-
-	if (host->ops->host_stop)
-		host->ops->host_stop(host);
-
-	ata_host_free(host);
-}
-
 struct ata_probe_ent *
 ata_probe_ent_alloc(struct device *dev, const struct ata_port_info *port)
 {
@@ -6315,32 +6275,6 @@ void ata_pci_host_stop (struct ata_host 
 	pci_iounmap(pdev, host->mmio_base);
 }
 
-/**
- *	ata_pci_remove_one - PCI layer callback for device removal
- *	@pdev: PCI device that was removed
- *
- *	PCI layer indicates to libata via this hook that
- *	hot-unplug or module unload event has occurred.
- *	Handle this by unregistering all objects associated
- *	with this PCI device.  Free those objects.  Then finally
- *	release PCI resources and disable device.
- *
- *	LOCKING:
- *	Inherited from PCI layer (may sleep).
- */
-
-void ata_pci_remove_one (struct pci_dev *pdev)
-{
-	struct device *dev = pci_dev_to_dev(pdev);
-	struct ata_host *host = dev_get_drvdata(dev);
-
-	ata_host_remove(host);
-
-	pci_release_regions(pdev);
-	pci_disable_device(pdev);
-	dev_set_drvdata(dev, NULL);
-}
-
 /* move to PCI subsystem */
 int pci_test_config_bits(struct pci_dev *pdev, const struct pci_bits *bits)
 {
@@ -6572,7 +6506,6 @@ EXPORT_SYMBOL_GPL(ata_host_detach);
 EXPORT_SYMBOL_GPL(ata_host_stop);
 EXPORT_SYMBOL_GPL(ata_host_free_irqs);
 EXPORT_SYMBOL_GPL(ata_host_free);
-EXPORT_SYMBOL_GPL(ata_host_remove);
 EXPORT_SYMBOL_GPL(ata_sg_init);
 EXPORT_SYMBOL_GPL(ata_sg_init_one);
 EXPORT_SYMBOL_GPL(ata_hsm_move);
@@ -6651,7 +6584,6 @@ #ifdef CONFIG_PCI
 EXPORT_SYMBOL_GPL(pci_test_config_bits);
 EXPORT_SYMBOL_GPL(ata_pci_host_stop);
 EXPORT_SYMBOL_GPL(ata_pci_init_native_mode);
-EXPORT_SYMBOL_GPL(ata_pci_remove_one);
 EXPORT_SYMBOL_GPL(ata_pci_device_do_suspend);
 EXPORT_SYMBOL_GPL(ata_pci_device_do_resume);
 EXPORT_SYMBOL_GPL(ata_pci_device_suspend);
drivers/ata/libata-pci.c +29 -0
index 9649a8c..6e2c454 100644
@@ -619,6 +619,34 @@ int ata_pci_init_one(struct pci_dev *pde
 	return rc;
 }
 
+/**
+ *	ata_pci_remove_one - PCI layer callback for device removal
+ *	@pdev: PCI device that was removed
+ *
+ *	PCI layer indicates to libata via this hook that
+ *	hot-unplug or module unload event has occurred.
+ *	Handle this by unregistering all objects associated
+ *	with this PCI device.  Free those objects.  Then finally
+ *	release PCI resources and disable device.
+ *
+ *	LOCKING:
+ *	Inherited from PCI layer (may sleep).
+ */
+void ata_pci_remove_one(struct pci_dev *pdev)
+{
+	struct device *dev = pci_dev_to_dev(pdev);
+	struct ata_host *host = dev_get_drvdata(dev);
+
+	ata_host_detach(host);
+
+	if (host->mmio_base) {
+		ata_host_stop(host);
+		pci_iounmap(pdev, host->mmio_base);
+	}
+
+	ata_pci_host_destroy(host);
+}
+
 EXPORT_SYMBOL_GPL(ata_pci_legacy_mask);
 EXPORT_SYMBOL_GPL(ata_pci_set_dma_mask);
 EXPORT_SYMBOL_GPL(ata_pci_acquire_resources);
@@ -631,3 +659,4 @@ EXPORT_SYMBOL_GPL(ata_pci_free_msix_irqs
 EXPORT_SYMBOL_GPL(ata_pci_host_prepare);
 EXPORT_SYMBOL_GPL(ata_pci_host_destroy);
 EXPORT_SYMBOL_GPL(ata_pci_init_one);
+EXPORT_SYMBOL_GPL(ata_pci_remove_one);
include/linux/libata.h +0 -1
index 3705d09..e96c257 100644
@@ -767,7 +767,6 @@ extern void ata_host_free_irqs(struct at
 extern void ata_host_free(struct ata_host *host);
 extern void ata_host_init(struct ata_host *, struct device *,
 			  unsigned long, const struct ata_port_operations *);
-extern void ata_host_remove(struct ata_host *host);
 extern int ata_scsi_detect(struct scsi_host_template *sht);
 extern int ata_scsi_ioctl(struct scsi_device *dev, int cmd, void __user *arg);
 extern int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *));
Signature · 1 line
1.4.1.1
author
#27 lore raw

PATCH 11/20 libata: use remove_one() for deinit instead of ->host_stop()

In reply to Tejun Heo

Update LLDs such that they use ->remove_one() for deinit instead of ->host_stop(). This is how deinit is usually done in other drivers, more in line with how callbacks are used in libata, symmetric with initialization, and gives LLDs more flexibility.

As mmio_base release is libata-pci's responsibility, LLDs which used the default ata_pci_host_stop() can simply switch to stock ata_pci_remove_one().

This patch fixes the following bugs.

* ata_piix doesn't free hpriv on removal
* sata_sil24 unmaps port_base twice on removal
* sata_uli doesn't free hpriv on removal

---
 drivers/ata/ahci.c         |   20 +-------------------
 drivers/ata/ata_piix.c     |   18 +++++++-----------
 drivers/ata/pdc_adma.c     |   30 +++++++++++++++---------------
 drivers/ata/sata_mv.c      |   40 +++++++++++-----------------------------
 drivers/ata/sata_nv.c      |   26 +++++++++++++-------------
 drivers/ata/sata_promise.c |   26 +++++++++++---------------
 drivers/ata/sata_qstor.c   |   27 +++++++++++++--------------
 drivers/ata/sata_sil.c     |    3 +--
 drivers/ata/sata_sil24.c   |   30 +++++++++++++++++-------------
 drivers/ata/sata_sis.c     |    1 -
 drivers/ata/sata_svw.c     |    1 -
 drivers/ata/sata_sx4.c     |   36 ++++++++++++++++++++----------------
 drivers/ata/sata_uli.c     |   13 +++++++++++--
 drivers/ata/sata_via.c     |    1 -
 drivers/ata/sata_vsc.c     |    1 -
 15 files changed, 120 insertions(+), 153 deletions(-)
drivers/ata/ahci.c +1 -19
index 80b47e5..e8f8977 100644
@@ -1576,26 +1576,8 @@ static void ahci_remove_one (struct pci_
 	struct device *dev = pci_dev_to_dev(pdev);
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct ahci_host_priv *hpriv = host->private_data;
-	int have_msi;
 
-	ata_host_detach(host);
-
-	have_msi = host->pci_flags & ATA_PCI_RES_MSI;
-	free_irq(host->irq, host);
-
-	ata_host_stop(host);
-	pci_iounmap(pdev, host->mmio_base);
-
-	if (have_msi)
-		pci_disable_msi(pdev);
-	else
-		pci_intx(pdev, 0);
-
-	pci_release_regions(pdev);
-	pci_disable_device(pdev);
-	dev_set_drvdata(dev, NULL);
-
-	ata_host_free(host);
+	ata_pci_remove_one(pdev);
 	kfree(hpriv);
 }
 
drivers/ata/ata_piix.c +7 -11
index 0cb48a4..f13513c 100644
@@ -149,9 +149,8 @@ struct piix_host_priv {
 	const struct piix_map_db *map_db;
 };
 
-static int piix_init_one (struct pci_dev *pdev,
-				    const struct pci_device_id *ent);
-static void piix_host_stop(struct ata_host *host);
+static int piix_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
+static void piix_remove_one(struct pci_dev *pdev);
 static void piix_set_piomode (struct ata_port *ap, struct ata_device *adev);
 static void piix_set_dmamode (struct ata_port *ap, struct ata_device *adev);
 static void piix_pata_error_handler(struct ata_port *ap);
@@ -205,7 +204,7 @@ static struct pci_driver piix_pci_driver
 	.name			= DRV_NAME,
 	.id_table		= piix_pci_tbl,
 	.probe			= piix_init_one,
-	.remove			= ata_pci_remove_one,
+	.remove			= piix_remove_one,
 	.suspend		= ata_pci_device_suspend,
 	.resume			= ata_pci_device_resume,
 };
@@ -260,7 +259,6 @@ static const struct ata_port_operations 
 
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= piix_host_stop,
 };
 
 static const struct ata_port_operations piix_sata_ops = {
@@ -290,7 +288,6 @@ static const struct ata_port_operations 
 
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= piix_host_stop,
 };
 
 static const struct piix_map_db ich5_map_db = {
@@ -930,13 +927,12 @@ static int piix_init_one (struct pci_dev
 	return ata_pci_init_one(pdev, ppinfo, 2);
 }
 
-static void piix_host_stop(struct ata_host *host)
+static void piix_remove_one(struct pci_dev *pdev)
 {
-	struct piix_host_priv *hpriv = host->private_data;
+	struct ata_host *host = dev_get_drvdata(&pdev->dev);
 
-	ata_pci_host_stop(host);
-
-	kfree(hpriv);
+	kfree(host->private_data);
+	ata_pci_remove_one(pdev);
 }
 
 static int __init piix_init(void)
drivers/ata/pdc_adma.c +15 -15
index ab5d06b..d999235 100644
@@ -122,10 +122,10 @@ struct adma_port_priv {
 	adma_state_t		state;
 };
 
-static int adma_ata_init_one (struct pci_dev *pdev,
-				const struct pci_device_id *ent);
+static int adma_ata_init_one(struct pci_dev *pdev,
+			     const struct pci_device_id *ent);
+static void adma_ata_remove_one(struct pci_dev *pdev);
 static int adma_port_start(struct ata_port *ap);
-static void adma_host_stop(struct ata_host *host);
 static void adma_port_stop(struct ata_port *ap);
 static void adma_phy_reset(struct ata_port *ap);
 static void adma_qc_prep(struct ata_queued_cmd *qc);
@@ -170,7 +170,6 @@ static const struct ata_port_operations 
 	.irq_clear		= adma_irq_clear,
 	.port_start		= adma_port_start,
 	.port_stop		= adma_port_stop,
-	.host_stop		= adma_host_stop,
 	.bmdma_stop		= adma_bmdma_stop,
 	.bmdma_status		= adma_bmdma_status,
 };
@@ -199,7 +198,7 @@ static struct pci_driver adma_ata_pci_dr
 	.name			= DRV_NAME,
 	.id_table		= adma_ata_pci_tbl,
 	.probe			= adma_ata_init_one,
-	.remove			= ata_pci_remove_one,
+	.remove			= adma_ata_remove_one,
 };
 
 static int adma_check_atapi_dma(struct ata_queued_cmd *qc)
@@ -593,16 +592,6 @@ static void adma_port_stop(struct ata_po
 	ata_port_stop(ap);
 }
 
-static void adma_host_stop(struct ata_host *host)
-{
-	unsigned int port_no;
-
-	for (port_no = 0; port_no < ADMA_PORTS; ++port_no)
-		adma_reset_engine(ADMA_REGS(host->mmio_base, port_no));
-
-	ata_pci_host_stop(host);
-}
-
 static void adma_host_init(unsigned int chip_id, struct ata_host *host)
 {
 	unsigned int port_no;
@@ -689,6 +678,17 @@ static int adma_ata_init_one(struct pci_
 	return rc;
 }
 
+static void adma_ata_remove_one(struct pci_dev *pdev)
+{
+	struct ata_host *host = dev_get_drvdata(&pdev->dev);
+	unsigned int port_no;
+
+	for (port_no = 0; port_no < ADMA_PORTS; ++port_no)
+		adma_reset_engine(ADMA_REGS(host->mmio_base, port_no));
+
+	ata_pci_remove_one(pdev);
+}
+
 static int __init adma_ata_init(void)
 {
 	return pci_register_driver(&adma_ata_pci_driver);
drivers/ata/sata_mv.c +11 -29
index 71e877c..9a87937 100644
@@ -341,7 +341,6 @@ static u32 mv5_scr_read(struct ata_port 
 static void mv5_scr_write(struct ata_port *ap, unsigned int sc_reg_in, u32 val);
 static void mv_phy_reset(struct ata_port *ap);
 static void __mv_phy_reset(struct ata_port *ap, int can_sleep);
-static void mv_host_stop(struct ata_host *host);
 static int mv_port_start(struct ata_port *ap);
 static void mv_port_stop(struct ata_port *ap);
 static void mv_qc_prep(struct ata_queued_cmd *qc);
@@ -349,6 +348,7 @@ static void mv_qc_prep_iie(struct ata_qu
 static unsigned int mv_qc_issue(struct ata_queued_cmd *qc);
 static void mv_eng_timeout(struct ata_port *ap);
 static int mv_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
+static void mv_remove_one(struct pci_dev *pdev);
 
 static void mv5_phy_errata(struct mv_host_priv *hpriv, void __iomem *mmio,
 			   unsigned int port);
@@ -415,7 +415,6 @@ static const struct ata_port_operations 
 
 	.port_start		= mv_port_start,
 	.port_stop		= mv_port_stop,
-	.host_stop		= mv_host_stop,
 };
 
 static const struct ata_port_operations mv6_ops = {
@@ -442,7 +441,6 @@ static const struct ata_port_operations 
 
 	.port_start		= mv_port_start,
 	.port_stop		= mv_port_stop,
-	.host_stop		= mv_host_stop,
 };
 
 static const struct ata_port_operations mv_iie_ops = {
@@ -468,7 +466,6 @@ static const struct ata_port_operations 
 
 	.port_start		= mv_port_start,
 	.port_stop		= mv_port_stop,
-	.host_stop		= mv_host_stop,
 };
 
 static const struct ata_port_info mv_port_info[] = {
@@ -545,7 +542,7 @@ static struct pci_driver mv_pci_driver =
 	.name			= DRV_NAME,
 	.id_table		= mv_pci_tbl,
 	.probe			= mv_init_one,
-	.remove			= ata_pci_remove_one,
+	.remove			= mv_remove_one,
 };
 
 static const struct mv_hw_ops mv5xxx_ops = {
@@ -801,30 +798,6 @@ static void mv_scr_write(struct ata_port
 	}
 }
 
-/**
- *      mv_host_stop - Host specific cleanup/stop routine.
- *      @host: host data structure
- *
- *      Disable ints, cleanup host memory, call general purpose
- *      host_stop.
- *
- *      LOCKING:
- *      Inherited from caller.
- */
-static void mv_host_stop(struct ata_host *host)
-{
-	struct mv_host_priv *hpriv = host->private_data;
-	struct pci_dev *pdev = to_pci_dev(host->dev);
-
-	if (host->pci_flags & ATA_PCI_RES_MSI) {
-		pci_disable_msi(pdev);
-	} else {
-		pci_intx(pdev, 0);
-	}
-	kfree(hpriv);
-	ata_pci_host_stop(host);
-}
-
 static inline void mv_priv_free(struct mv_port_priv *pp, struct device *dev)
 {
 	dma_free_coherent(dev, MV_PORT_PRIV_DMA_SZ, pp->crpb, pp->crpb_dma);
@@ -2406,6 +2379,15 @@ static int mv_init_one(struct pci_dev *p
 	return rc;
 }
 
+static void mv_remove_one(struct pci_dev *pdev)
+{
+	struct ata_host *host = dev_get_drvdata(&pdev->dev);
+	struct mv_host_priv *hpriv = host->private_data;
+
+	ata_pci_remove_one(pdev);
+	kfree(hpriv);
+}
+
 static int __init mv_init(void)
 {
 	return pci_register_driver(&mv_pci_driver);
drivers/ata/sata_nv.c +13 -13
index f5b94af..59ee99a 100644
@@ -80,8 +80,8 @@ enum {
 	NV_MCP_SATA_CFG_20_SATA_SPACE_EN = 0x04,
 };
 
-static int nv_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
-static void nv_ck804_host_stop(struct ata_host *host);
+static int nv_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
+static void nv_remove_one(struct pci_dev *pdev);
 static u32 nv_scr_read (struct ata_port *ap, unsigned int sc_reg);
 static void nv_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
 
@@ -145,7 +145,7 @@ static struct pci_driver nv_pci_driver =
 	.name			= DRV_NAME,
 	.id_table		= nv_pci_tbl,
 	.probe			= nv_init_one,
-	.remove			= ata_pci_remove_one,
+	.remove			= nv_remove_one,
 };
 
 static struct scsi_host_template nv_sht = {
@@ -189,7 +189,6 @@ static const struct ata_port_operations 
 	.scr_write		= nv_scr_write,
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= ata_pci_host_stop,
 };
 
 static const struct ata_port_operations nv_nf2_ops = {
@@ -215,7 +214,6 @@ static const struct ata_port_operations 
 	.scr_write		= nv_scr_write,
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= ata_pci_host_stop,
 };
 
 static const struct ata_port_operations nv_ck804_ops = {
@@ -241,7 +239,6 @@ static const struct ata_port_operations 
 	.scr_write		= nv_scr_write,
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= nv_ck804_host_stop,
 };
 
 static struct ata_port_info nv_port_info[] = {
@@ -564,17 +561,20 @@ static int nv_init_one (struct pci_dev *
 	return rc;
 }
 
-static void nv_ck804_host_stop(struct ata_host *host)
+static void nv_remove_one(struct pci_dev *pdev)
 {
-	struct pci_dev *pdev = to_pci_dev(host->dev);
-	u8 regval;
+	struct ata_host *host = dev_get_drvdata(&pdev->dev);
 
 	/* disable SATA space for CK804 */
-	pci_read_config_byte(pdev, NV_MCP_SATA_CFG_20, &regval);
-	regval &= ~NV_MCP_SATA_CFG_20_SATA_SPACE_EN;
-	pci_write_config_byte(pdev, NV_MCP_SATA_CFG_20, regval);
+	if (host->ops == &nv_ck804_ops) {
+		u8 regval;
+
+		pci_read_config_byte(pdev, NV_MCP_SATA_CFG_20, &regval);
+		regval &= ~NV_MCP_SATA_CFG_20_SATA_SPACE_EN;
+		pci_write_config_byte(pdev, NV_MCP_SATA_CFG_20, regval);
+	}
 
-	ata_pci_host_stop(host);
+	ata_pci_remove_one(pdev);
 }
 
 static int __init nv_init(void)
drivers/ata/sata_promise.c +11 -15
index 6cae44a..92571f7 100644
@@ -97,7 +97,8 @@ struct pdc_host_priv {
 
 static u32 pdc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg);
 static void pdc_sata_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
-static int pdc_ata_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
+static int pdc_ata_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
+static void pdc_ata_remove_one(struct pci_dev *pdev);
 static void pdc_eng_timeout(struct ata_port *ap);
 static int pdc_port_start(struct ata_port *ap);
 static void pdc_port_stop(struct ata_port *ap);
@@ -108,7 +109,6 @@ static void pdc_tf_load_mmio(struct ata_
 static void pdc_exec_command_mmio(struct ata_port *ap, const struct ata_taskfile *tf);
 static void pdc_irq_clear(struct ata_port *ap);
 static unsigned int pdc_qc_issue_prot(struct ata_queued_cmd *qc);
-static void pdc_host_stop(struct ata_host *host);
 
 
 static struct scsi_host_template pdc_ata_sht = {
@@ -149,7 +149,6 @@ static const struct ata_port_operations 
 	.scr_write		= pdc_sata_scr_write,
 	.port_start		= pdc_port_start,
 	.port_stop		= pdc_port_stop,
-	.host_stop		= pdc_host_stop,
 };
 
 static const struct ata_port_operations pdc_pata_ops = {
@@ -170,7 +169,6 @@ static const struct ata_port_operations 
 
 	.port_start		= pdc_port_start,
 	.port_stop		= pdc_port_stop,
-	.host_stop		= pdc_host_stop,
 };
 
 /* Use bits 30-31 of port_flags to encode available port numbers.
@@ -300,7 +298,7 @@ static struct pci_driver pdc_ata_pci_dri
 	.name			= DRV_NAME,
 	.id_table		= pdc_ata_pci_tbl,
 	.probe			= pdc_ata_init_one,
-	.remove			= ata_pci_remove_one,
+	.remove			= pdc_ata_remove_one,
 };
 
 
@@ -350,16 +348,6 @@ static void pdc_port_stop(struct ata_por
 }
 
 
-static void pdc_host_stop(struct ata_host *host)
-{
-	struct pdc_host_priv *hp = host->private_data;
-
-	ata_pci_host_stop(host);
-
-	kfree(hp);
-}
-
-
 static void pdc_reset_port(struct ata_port *ap)
 {
 	void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr + PDC_CTLSTAT;
@@ -789,6 +777,14 @@ static int pdc_ata_init_one(struct pci_d
 	return rc;
 }
 
+static void pdc_ata_remove_one(struct pci_dev *pdev)
+{
+	struct ata_host *host = dev_get_drvdata(&pdev->dev);
+	struct pdc_host_priv *hp = host->private_data;
+
+	ata_pci_remove_one(pdev);
+	kfree(hp);
+}
 
 static int __init pdc_ata_init(void)
 {
drivers/ata/sata_qstor.c +13 -14
index 48ed01f..c564ac4 100644
@@ -114,8 +114,8 @@ struct qs_port_priv {
 static u32 qs_scr_read (struct ata_port *ap, unsigned int sc_reg);
 static void qs_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
 static int qs_ata_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
+static void qs_ata_remove_one(struct pci_dev *pdev);
 static int qs_port_start(struct ata_port *ap);
-static void qs_host_stop(struct ata_host *host);
 static void qs_port_stop(struct ata_port *ap);
 static void qs_phy_reset(struct ata_port *ap);
 static void qs_qc_prep(struct ata_queued_cmd *qc);
@@ -163,7 +163,6 @@ static const struct ata_port_operations 
 	.scr_write		= qs_scr_write,
 	.port_start		= qs_port_start,
 	.port_stop		= qs_port_stop,
-	.host_stop		= qs_host_stop,
 	.bmdma_stop		= qs_bmdma_stop,
 	.bmdma_status		= qs_bmdma_status,
 };
@@ -193,7 +192,7 @@ static struct pci_driver qs_ata_pci_driv
 	.name			= DRV_NAME,
 	.id_table		= qs_ata_pci_tbl,
 	.probe			= qs_ata_init_one,
-	.remove			= ata_pci_remove_one,
+	.remove			= qs_ata_remove_one,
 };
 
 static int qs_check_atapi_dma(struct ata_queued_cmd *qc)
@@ -541,17 +540,6 @@ static void qs_port_stop(struct ata_port
 	ata_port_stop(ap);
 }
 
-static void qs_host_stop(struct ata_host *host)
-{
-	void __iomem *mmio_base = host->mmio_base;
-	struct pci_dev *pdev = to_pci_dev(host->dev);
-
-	writeb(0, mmio_base + QS_HCT_CTRL); /* disable host interrupts */
-	writeb(QS_CNFG3_GSRST, mmio_base + QS_HCF_CNFG3); /* global reset */
-
-	pci_iounmap(pdev, mmio_base);
-}
-
 static void qs_host_init(unsigned int chip_id, struct ata_host *host)
 {
 	void __iomem *mmio_base = host->mmio_base;
@@ -688,6 +676,17 @@ static int qs_ata_init_one(struct pci_de
 	return rc;
 }
 
+static void qs_ata_remove_one(struct pci_dev *pdev)
+{
+	struct ata_host *host = dev_get_drvdata(&pdev->dev);
+	void __iomem *mmio_base = host->mmio_base;
+
+	writeb(0, mmio_base + QS_HCT_CTRL); /* disable host interrupts */
+	writeb(QS_CNFG3_GSRST, mmio_base + QS_HCF_CNFG3); /* global reset */
+
+	ata_pci_remove_one(pdev);
+}
+
 static int __init qs_ata_init(void)
 {
 	return pci_register_driver(&qs_ata_pci_driver);
drivers/ata/sata_sil.c +1 -2
index 75c611a..3619f86 100644
@@ -108,7 +108,7 @@ enum {
 	SIL_QUIRK_UDMA5MAX	= (1 << 1),
 };
 
-static int sil_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
+static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
 static int sil_pci_device_resume(struct pci_dev *pdev);
 static void sil_dev_config(struct ata_port *ap, struct ata_device *dev);
 static u32 sil_scr_read (struct ata_port *ap, unsigned int sc_reg);
@@ -204,7 +204,6 @@ static const struct ata_port_operations 
 	.scr_write		= sil_scr_write,
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= ata_pci_host_stop,
 };
 
 static const struct ata_port_info sil_port_info[] = {
drivers/ata/sata_sil24.c +17 -13
index 59351b3..3c6d668 100644
@@ -336,8 +336,8 @@ static void sil24_error_handler(struct a
 static void sil24_post_internal_cmd(struct ata_queued_cmd *qc);
 static int sil24_port_start(struct ata_port *ap);
 static void sil24_port_stop(struct ata_port *ap);
-static void sil24_host_stop(struct ata_host *host);
 static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
+static void sil24_remove_one(struct pci_dev *pdev);
 static int sil24_pci_device_resume(struct pci_dev *pdev);
 
 static const struct pci_device_id sil24_pci_tbl[] = {
@@ -353,7 +353,7 @@ static struct pci_driver sil24_pci_drive
 	.name			= DRV_NAME,
 	.id_table		= sil24_pci_tbl,
 	.probe			= sil24_init_one,
-	.remove			= ata_pci_remove_one, /* safe? */
+	.remove			= sil24_remove_one,
 	.suspend		= ata_pci_device_suspend,
 	.resume			= sil24_pci_device_resume,
 };
@@ -405,7 +405,6 @@ static const struct ata_port_operations 
 
 	.port_start		= sil24_port_start,
 	.port_stop		= sil24_port_stop,
-	.host_stop		= sil24_host_stop,
 };
 
 /*
@@ -982,16 +981,6 @@ static void sil24_port_stop(struct ata_p
 	kfree(pp);
 }
 
-static void sil24_host_stop(struct ata_host *host)
-{
-	struct sil24_host_priv *hpriv = host->private_data;
-	struct pci_dev *pdev = to_pci_dev(host->dev);
-
-	pci_iounmap(pdev, hpriv->host_base);
-	pci_iounmap(pdev, hpriv->port_base);
-	kfree(hpriv);
-}
-
 static void sil24_init_controller(struct ata_host *host)
 {
 	struct pci_dev *pdev = to_pci_dev(host->dev);
@@ -1154,6 +1143,21 @@ static int sil24_init_one(struct pci_dev
 	return rc;
 }
 
+static void sil24_remove_one(struct pci_dev *pdev)
+{
+	struct ata_host *host = dev_get_drvdata(&pdev->dev);
+	struct sil24_host_priv *hpriv = host->private_data;
+
+	ata_host_detach(host);
+	ata_host_stop(host);
+
+	pci_iounmap(pdev, hpriv->host_base);
+	pci_iounmap(pdev, hpriv->port_base);
+
+	ata_pci_host_destroy(host);
+	kfree(hpriv);
+}
+
 static int sil24_pci_device_resume(struct pci_dev *pdev)
 {
 	struct ata_host *host = dev_get_drvdata(&pdev->dev);
drivers/ata/sata_sis.c +0 -1
index 6bf4533..56ad81d 100644
@@ -122,7 +122,6 @@ static const struct ata_port_operations 
 	.scr_write		= sis_scr_write,
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= ata_pci_host_stop,
 };
 
 static struct ata_port_info sis_port_info = {
drivers/ata/sata_svw.c +0 -1
index b12b288..79c526d 100644
@@ -329,7 +329,6 @@ static const struct ata_port_operations 
 	.scr_write		= k2_sata_scr_write,
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= ata_pci_host_stop,
 };
 
 static void k2_sata_setup_port(struct ata_ioports *port, unsigned long base)
drivers/ata/sata_sx4.c +20 -16
index 5b94e53..794dbed 100644
@@ -151,7 +151,9 @@ struct pdc_host_priv {
 };
 
 
-static int pdc_sata_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
+static int pdc_sata_init_one(struct pci_dev *pdev,
+			     const struct pci_device_id *ent);
+static void pdc_sata_remove_one(struct pci_dev *pdev);
 static void pdc_eng_timeout(struct ata_port *ap);
 static void pdc_20621_phy_reset (struct ata_port *ap);
 static int pdc_port_start(struct ata_port *ap);
@@ -159,7 +161,6 @@ static void pdc_port_stop(struct ata_por
 static void pdc20621_qc_prep(struct ata_queued_cmd *qc);
 static void pdc_tf_load_mmio(struct ata_port *ap, const struct ata_taskfile *tf);
 static void pdc_exec_command_mmio(struct ata_port *ap, const struct ata_taskfile *tf);
-static void pdc20621_host_stop(struct ata_host *host);
 static unsigned int pdc20621_dimm_init(struct ata_host *hs);
 static int pdc20621_detect_dimm(struct ata_host *hs);
 static unsigned int pdc20621_i2c_read(struct ata_host *hs,
@@ -209,7 +210,6 @@ static const struct ata_port_operations 
 	.irq_clear		= pdc20621_irq_clear,
 	.port_start		= pdc_port_start,
 	.port_stop		= pdc_port_stop,
-	.host_stop		= pdc20621_host_stop,
 };
 
 static const struct ata_port_info pdc_port_info[] = {
@@ -238,22 +238,10 @@ static struct pci_driver pdc_sata_pci_dr
 	.name			= DRV_NAME,
 	.id_table		= pdc_sata_pci_tbl,
 	.probe			= pdc_sata_init_one,
-	.remove			= ata_pci_remove_one,
+	.remove			= pdc_sata_remove_one,
 };
 
 
-static void pdc20621_host_stop(struct ata_host *host)
-{
-	struct pci_dev *pdev = to_pci_dev(host->dev);
-	struct pdc_host_priv *hpriv = host->private_data;
-	void __iomem *dimm_mmio = hpriv->dimm_mmio;
-
-	pci_iounmap(pdev, dimm_mmio);
-	kfree(hpriv);
-
-	pci_iounmap(pdev, host->mmio_base);
-}
-
 static int pdc_port_start(struct ata_port *ap)
 {
 	struct device *dev = ap->host->dev;
@@ -1456,6 +1444,22 @@ static int pdc_sata_init_one(struct pci_
 }
 
 
+static void pdc_sata_remove_one(struct pci_dev *pdev)
+{
+	struct ata_host *host = dev_get_drvdata(&pdev->dev);
+	struct pdc_host_priv *hpriv = host->private_data;
+
+	ata_host_detach(host);
+	ata_host_stop(host);
+
+	pci_iounmap(pdev, host->mmio_base);
+	pci_iounmap(pdev, hpriv->dimm_mmio);
+
+	ata_pci_host_destroy(host);
+	kfree(hpriv);
+}
+
+
 static int __init pdc_sata_init(void)
 {
 	return pci_register_driver(&pdc_sata_pci_driver);
drivers/ata/sata_uli.c +11 -2
index b1498bb..540bdd5 100644
@@ -57,6 +57,7 @@ struct uli_priv {
 };
 
 static int uli_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
+static void uli_remove_one(struct pci_dev *pdev);
 static u32 uli_scr_read (struct ata_port *ap, unsigned int sc_reg);
 static void uli_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
 
@@ -72,7 +73,7 @@ static struct pci_driver uli_pci_driver 
 	.name			= DRV_NAME,
 	.id_table		= uli_pci_tbl,
 	.probe			= uli_init_one,
-	.remove			= ata_pci_remove_one,
+	.remove			= uli_remove_one,
 };
 
 static struct scsi_host_template uli_sht = {
@@ -122,7 +123,6 @@ static const struct ata_port_operations 
 
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= ata_pci_host_stop,
 };
 
 static struct ata_port_info uli_port_info = {
@@ -278,6 +278,15 @@ static int uli_init_one(struct pci_dev *
 	return rc;
 }
 
+static void uli_remove_one(struct pci_dev *pdev)
+{
+	struct ata_host *host = dev_get_drvdata(&pdev->dev);
+	struct uli_priv *hpriv = host->private_data;
+
+	ata_pci_remove_one(pdev);
+	kfree(hpriv);
+}
+
 static int __init uli_init(void)
 {
 	return pci_register_driver(&uli_pci_driver);
drivers/ata/sata_via.c +0 -1
index fc85b59..db7b09e 100644
@@ -137,7 +137,6 @@ static const struct ata_port_operations 
 
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= ata_pci_host_stop,
 };
 
 static struct ata_port_info svia_port_info = {
drivers/ata/sata_vsc.c +0 -1
index 2fa3e7e..ac00d17 100644
@@ -307,7 +307,6 @@ static const struct ata_port_operations 
 	.scr_write		= vsc_sata_scr_write,
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= ata_pci_host_stop,
 };
 
 static const struct ata_port_info vsc_sata_port_info = {
Signature · 1 line
1.4.1.1
#28 lore raw

Re: [PATCH 11/20] libata: use remove_one() for deinit instead of ->host_stop()

in general #10 and #11 look OK, modulo the comments just made on #9

author
#29 lore raw

PATCH 12/20 libata: kill old init helpers

In reply to Tejun Heo

Kill old init helpers ata_pci_init_native_mode() and ata_device_add().

---
 drivers/ata/libata-bmdma.c |   68 ------------------------
 drivers/ata/libata-core.c  |  123 --------------------------------------------
 include/linux/libata.h     |    3 -
 3 files changed, 0 insertions(+), 194 deletions(-)
drivers/ata/libata-bmdma.c +0 -68
index 672df5a..0f62a6e 100644
@@ -798,74 +798,6 @@ void ata_bmdma_post_internal_cmd(struct 
 
 #ifdef CONFIG_PCI
 /**
- *	ata_pci_init_native_mode - Initialize native-mode driver
- *	@pdev:  pci device to be initialized
- *	@port:  array[2] of pointers to port info structures.
- *	@ports: bitmap of ports present
- *
- *	Utility function which allocates and initializes an
- *	ata_probe_ent structure for a standard dual-port
- *	PIO-based IDE controller.  The returned ata_probe_ent
- *	structure can be passed to ata_device_add().  The returned
- *	ata_probe_ent structure should then be freed with kfree().
- *
- *	The caller need only pass the address of the primary port, the
- *	secondary will be deduced automatically. If the device has non
- *	standard secondary port mappings this function can be called twice,
- *	once for each interface.
- */
-
-struct ata_probe_ent *
-ata_pci_init_native_mode(struct pci_dev *pdev, struct ata_port_info **port, int ports)
-{
-	struct ata_probe_ent *probe_ent =
-		ata_probe_ent_alloc(pci_dev_to_dev(pdev), port[0]);
-	int p = 0;
-	unsigned long bmdma;
-
-	if (!probe_ent)
-		return NULL;
-
-	probe_ent->irq = pdev->irq;
-	probe_ent->irq_flags = IRQF_SHARED;
-	probe_ent->private_data = port[0]->private_data;
-
-	if (ports & ATA_PORT_PRIMARY) {
-		probe_ent->port[p].cmd_addr = pci_resource_start(pdev, 0);
-		probe_ent->port[p].altstatus_addr =
-		probe_ent->port[p].ctl_addr =
-			pci_resource_start(pdev, 1) | ATA_PCI_CTL_OFS;
-		bmdma = pci_resource_start(pdev, 4);
-		if (bmdma) {
-			if (inb(bmdma + 2) & 0x80)
-				probe_ent->_host_flags |= ATA_HOST_SIMPLEX;
-			probe_ent->port[p].bmdma_addr = bmdma;
-		}
-		ata_std_ports(&probe_ent->port[p]);
-		p++;
-	}
-
-	if (ports & ATA_PORT_SECONDARY) {
-		probe_ent->port[p].cmd_addr = pci_resource_start(pdev, 2);
-		probe_ent->port[p].altstatus_addr =
-		probe_ent->port[p].ctl_addr =
-			pci_resource_start(pdev, 3) | ATA_PCI_CTL_OFS;
-		bmdma = pci_resource_start(pdev, 4);
-		if (bmdma) {
-			bmdma += 8;
-			if(inb(bmdma + 2) & 0x80)
-				probe_ent->_host_flags |= ATA_HOST_SIMPLEX;
-			probe_ent->port[p].bmdma_addr = bmdma;
-		}
-		ata_std_ports(&probe_ent->port[p]);
-		p++;
-	}
-
-	probe_ent->n_ports = p;
-	return probe_ent;
-}
-
-/**
  *	ata_pci_clear_simplex	-	attempt to kick device out of simplex
  *	@pdev: PCI device
  *
drivers/ata/libata-core.c +0 -123

141 lines not on this page · raw message

include/linux/libata.h +0 -3
index e96c257..e274121 100644
@@ -760,7 +760,6 @@ extern int ata_host_request_irq(struct a
 			const char **p_reason);
 extern int ata_host_start(struct ata_host *host);
 extern int ata_host_attach(struct ata_host *host);
-extern int ata_device_add(const struct ata_probe_ent *ent);
 extern void ata_host_detach(struct ata_host *host);
 extern void ata_host_stop(struct ata_host *host);
 extern void ata_host_free_irqs(struct ata_host *host);
@@ -900,8 +899,6 @@ struct pci_bits {
 };
 
 extern void ata_pci_host_stop (struct ata_host *host);
-extern struct ata_probe_ent *
-ata_pci_init_native_mode(struct pci_dev *pdev, struct ata_port_info **port, int portmask);
 extern int pci_test_config_bits(struct pci_dev *pdev, const struct pci_bits *bits);
 extern unsigned long ata_pci_default_filter(const struct ata_port *, struct ata_device *, unsigned long);
 #endif /* CONFIG_PCI */
Signature · 1 line
1.4.1.1
author
#30 lore raw

PATCH 13/20 libata: kill unused ->host_stop() operation and related functions

In reply to Tejun Heo

Now that ata_port_operations->host_stop() is replaced by pci_driver->remove_one(), kill the method and related functions.

---
 drivers/ata/libata-core.c |    9 ---------
 include/linux/libata.h    |    3 ---
 2 files changed, 0 insertions(+), 12 deletions(-)
drivers/ata/libata-core.c +0 -9
index 1778256..4b85c88 100644
@@ -6146,14 +6146,6 @@ void ata_std_ports(struct ata_ioports *i
 
 
 #ifdef CONFIG_PCI
-
-void ata_pci_host_stop (struct ata_host *host)
-{
-	struct pci_dev *pdev = to_pci_dev(host->dev);
-
-	pci_iounmap(pdev, host->mmio_base);
-}
-
 /* move to PCI subsystem */
 int pci_test_config_bits(struct pci_dev *pdev, const struct pci_bits *bits)
 {
@@ -6460,7 +6452,6 @@ EXPORT_SYMBOL_GPL(ata_timing_merge);
 
 #ifdef CONFIG_PCI
 EXPORT_SYMBOL_GPL(pci_test_config_bits);
-EXPORT_SYMBOL_GPL(ata_pci_host_stop);
 EXPORT_SYMBOL_GPL(ata_pci_device_do_suspend);
 EXPORT_SYMBOL_GPL(ata_pci_device_do_resume);
 EXPORT_SYMBOL_GPL(ata_pci_device_suspend);
include/linux/libata.h +0 -3
index e274121..c5200c0 100644
@@ -647,8 +647,6 @@ struct ata_port_operations {
 	int (*port_start) (struct ata_port *ap);
 	void (*port_stop) (struct ata_port *ap);
 
-	void (*host_stop) (struct ata_host *host);
-
 	void (*bmdma_stop) (struct ata_queued_cmd *qc);
 	u8   (*bmdma_status) (struct ata_port *ap);
 };
@@ -898,7 +896,6 @@ struct pci_bits {
 	unsigned long		val;
 };
 
-extern void ata_pci_host_stop (struct ata_host *host);
 extern int pci_test_config_bits(struct pci_dev *pdev, const struct pci_bits *bits);
 extern unsigned long ata_pci_default_filter(const struct ata_port *, struct ata_device *, unsigned long);
 #endif /* CONFIG_PCI */
Signature · 1 line
1.4.1.1
#31 lore raw

Re: [PATCH 13/20] libata: kill unused ->host_stop() operation and related functions

ACK #12/#13 as obvious

author
#32 lore raw

PATCH 14/20 libata: use LLD name where possible

In reply to Tejun Heo

When acquiring resources, use LLD name where possible.

---
 drivers/ata/libata-core.c   |   12 +++++++++++-
 drivers/ata/libata-legacy.c |    2 +-
 drivers/ata/libata-pci.c    |    2 +-
 drivers/ata/libata.h        |    1 +
 4 files changed, 14 insertions(+), 3 deletions(-)
drivers/ata/libata-core.c +11 -1
index 4b85c88..96709de 100644
@@ -103,6 +103,16 @@ MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
 
+const char *ata_drv_name(struct ata_host *host)
+{
+	const char *name = dev_driver_string(host->dev);
+
+	if (name[0] != '\0')
+		return name;
+
+	return DRV_NAME;
+}
+
 /**
  *	ata_tf_to_fis - Convert ATA taskfile to SATA FIS structure
  *	@tf: Taskfile to convert
@@ -5744,7 +5754,7 @@ int ata_host_request_irq_marker(struct a
 		goto err;
 	}
 
-	rc = request_irq(irq, handler, irq_flags, DRV_NAME, dev_id);
+	rc = request_irq(irq, handler, irq_flags, ata_drv_name(host), dev_id);
 	if (rc) {
 		reason = "failed to request IRQ";
 		goto err;
drivers/ata/libata-legacy.c +1 -1
index 00b70b0..c519602 100644
@@ -57,7 +57,7 @@ static int ata_legacy_acquire_port(struc
 	struct ata_host *host = ap->host;
 	unsigned long cmd_addr = ata_legacy_addr(ap->port_no, 0);
 
-	if (request_region(cmd_addr, 8, "libata") != NULL)
+	if (request_region(cmd_addr, 8, ata_drv_name(host)) != NULL)
 		host->legacy_flags |= ATA_LEGACY_RES_PRI << ap->port_no;
 	else {
 		struct resource *conflict, res;
drivers/ata/libata-pci.c +1 -1
index 6e2c454..cb61679 100644
@@ -166,7 +166,7 @@ int ata_pci_acquire_resources(struct ata
 		goto err;
 	}
 
-	rc = pci_request_regions(pdev, DRV_NAME);
+	rc = pci_request_regions(pdev, ata_drv_name(host));
 	if (rc) {
 		host->flags |= ATA_HOST_DEV_BUSY;
 		reason = "failed to request PCI regions";
drivers/ata/libata.h +1 -0
index 0601c4a..fcb7a66 100644
@@ -39,6 +39,7 @@ struct ata_scsi_args {
 };
 
 /* libata-core.c */
+extern const char *ata_drv_name(struct ata_host *host);
 extern struct workqueue_struct *ata_aux_wq;
 extern int atapi_enabled;
 extern int atapi_dmadir;
Signature · 1 line
1.4.1.1
#33 lore raw

Re: [PATCH 14/20] libata: use LLD name where possible

ACK

author
#34 lore raw

PATCH 15/20 libata: move ->irq_handler from port_ops to port_info

In reply to Tejun Heo

libata-core now doesn't care about irq_handler. It's responsibility of LLD and helpers. The only left user is PCI legacy/native init helpers. Move ->irq_handler to ata_port_info.

---
 drivers/ata/ata_piix.c   |   10 ++++++++--
 drivers/ata/libata-pci.c |    2 +-
 include/linux/libata.h   |    3 ++-
 3 files changed, 11 insertions(+), 4 deletions(-)
drivers/ata/ata_piix.c +8 -2
index f13513c..21f19f2 100644
@@ -254,7 +254,6 @@ static const struct ata_port_operations 
 	.error_handler		= piix_pata_error_handler,
 	.post_internal_cmd	= ata_bmdma_post_internal_cmd,
 
-	.irq_handler		= ata_interrupt,
 	.irq_clear		= ata_bmdma_irq_clear,
 
 	.port_start		= ata_port_start,
@@ -283,7 +282,6 @@ static const struct ata_port_operations 
 	.error_handler		= piix_sata_error_handler,
 	.post_internal_cmd	= ata_bmdma_post_internal_cmd,
 
-	.irq_handler		= ata_interrupt,
 	.irq_clear		= ata_bmdma_irq_clear,
 
 	.port_start		= ata_port_start,
@@ -368,6 +366,7 @@ #else
 #endif
 		.udma_mask	= ATA_UDMA_MASK_40C,
 		.port_ops	= &piix_pata_ops,
+		.irq_handler	= ata_interrupt,
 	},
 
 	/* ich5_pata */
@@ -382,6 +381,7 @@ #else
 #endif
 		.udma_mask	= 0x3f, /* udma0-5 */
 		.port_ops	= &piix_pata_ops,
+		.irq_handler	= ata_interrupt,
 	},
 
 	/* ich5_sata */
@@ -392,6 +392,7 @@ #endif
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
 		.port_ops	= &piix_sata_ops,
+		.irq_handler	= ata_interrupt,
 	},
 
 	/* i6300esb_sata */
@@ -403,6 +404,7 @@ #endif
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
 		.port_ops	= &piix_sata_ops,
+		.irq_handler	= ata_interrupt,
 	},
 
 	/* ich6_sata */
@@ -414,6 +416,7 @@ #endif
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
 		.port_ops	= &piix_sata_ops,
+		.irq_handler	= ata_interrupt,
 	},
 
 	/* ich6_sata_ahci */
@@ -426,6 +429,7 @@ #endif
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
 		.port_ops	= &piix_sata_ops,
+		.irq_handler	= ata_interrupt,
 	},
 
 	/* ich6m_sata_ahci */
@@ -438,6 +442,7 @@ #endif
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
 		.port_ops	= &piix_sata_ops,
+		.irq_handler	= ata_interrupt,
 	},
 
 	/* ich8_sata_ahci */
@@ -450,6 +455,7 @@ #endif
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f,	/* udma0-6 */
 		.port_ops	= &piix_sata_ops,
+		.irq_handler	= ata_interrupt,
 	},
 };
 
drivers/ata/libata-pci.c +1 -1
index cb61679..997bd14 100644
@@ -494,7 +494,7 @@ int ata_pci_host_prepare(struct pci_dev 
 
 	/* request legacy IRQs */
 	for (i = 0; i < 2; i++) {
-		handler[i] = pi[i]->port_ops->irq_handler;
+		handler[i] = pi[i]->irq_handler;
 		dev_id[i] = host;
 		legacy_irq_flags[i] = 0;
 	}
include/linux/libata.h +2 -1
index c5200c0..791fa97 100644
@@ -634,7 +634,6 @@ struct ata_port_operations {
 	void (*error_handler) (struct ata_port *ap);
 	void (*post_internal_cmd) (struct ata_queued_cmd *qc);
 
-	irqreturn_t (*irq_handler)(int, void *, struct pt_regs *);
 	void (*irq_clear) (struct ata_port *);
 
 	u32 (*scr_read) (struct ata_port *ap, unsigned int sc_reg);
@@ -659,6 +658,8 @@ struct ata_port_info {
 	unsigned long		udma_mask;
 	const struct ata_port_operations *port_ops;
 	void 			*private_data;
+	/* fields for BMDMA init */
+	irqreturn_t (*irq_handler)(int, void *, struct pt_regs *);
 };
 
 struct ata_timing {
Signature · 1 line
1.4.1.1
#35 lore raw

Re: [PATCH 15/20] libata: move ->irq_handler from port_ops to port_info

ideally we don't ever want to store info on the irq handler at all...

author
#36 lore raw

PATCH 16/20 libata: make ata_host_alloc() take care of hpriv alloc/free

In reply to Tejun Heo

Transfer responsibility to allocate/init hpriv from LLD/port_info to host allocation. All host allocation/prepare routines now take @hpriv_hz and initialize host->private_data accordingly. hpriv area is aligned to __alignof__(long long) and zeroed.

As hpriv handling is moved to host allocation, pinfo->private_data is removed. The only user of pinfo->private_data was ata_pci_init_one() which is updated to take @hpriv argument directly. This change makes port_info carry one less host-wide info - which is good because mismatched scope causes confusion and bug (e.g. ata_piix private_data init bug).

---
 drivers/ata/ahci.c         |   29 ++++++++---------------------
 drivers/ata/ata_piix.c     |    9 +++------
 drivers/ata/libata-core.c  |   36 ++++++++++++++++++++++--------------
 drivers/ata/libata-pci.c   |   12 ++++++++----
 drivers/ata/pdc_adma.c     |    2 +-
 drivers/ata/sata_mv.c      |   27 +++++++--------------------
 drivers/ata/sata_nv.c      |    2 +-
 drivers/ata/sata_promise.c |   28 ++++++++--------------------
 drivers/ata/sata_qstor.c   |    4 ++--
 drivers/ata/sata_sil.c     |    2 +-
 drivers/ata/sata_sil24.c   |   19 ++++++++-----------
 drivers/ata/sata_sis.c     |    2 +-
 drivers/ata/sata_svw.c     |    2 +-
 drivers/ata/sata_sx4.c     |   19 ++++++++-----------
 drivers/ata/sata_uli.c     |   31 +++++++++----------------------
 drivers/ata/sata_via.c     |    3 +--
 drivers/ata/sata_vsc.c     |    3 ++-
 include/linux/libata.h     |   14 ++++++++------
 18 files changed, 99 insertions(+), 145 deletions(-)
drivers/ata/ahci.c +8 -21
index e8f8977..1ad49fe 100644
@@ -214,7 +214,6 @@ static int ahci_port_suspend(struct ata_
 static int ahci_port_resume(struct ata_port *ap);
 static int ahci_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg);
 static int ahci_pci_device_resume(struct pci_dev *pdev);
-static void ahci_remove_one (struct pci_dev *pdev);
 
 static struct scsi_host_template ahci_sht = {
 	.module			= THIS_MODULE,
@@ -374,7 +373,7 @@ static struct pci_driver ahci_pci_driver
 	.probe			= ahci_init_one,
 	.suspend		= ahci_pci_device_suspend,
 	.resume			= ahci_pci_device_resume,
-	.remove			= ahci_remove_one,
+	.remove			= ata_pci_remove_one,
 };
 
 
@@ -1453,9 +1452,9 @@ static int ahci_init_one(struct pci_dev 
 {
 	static int printed_version;
 	const struct ata_port_info *pinfo = &ahci_port_info[ent->driver_data];
-	struct ata_host *host = NULL;
-	struct ahci_host_priv *hpriv = NULL;
+	struct ata_host *host;
 	void __iomem *mmio_base = NULL;
+	struct ahci_host_priv *hpriv;
 	const char *reason;
 	u64 dma_mask;
 	u32 cap;
@@ -1479,15 +1478,14 @@ static int ahci_init_one(struct pci_dev 
 			return -ENODEV;
 	}
 
-	/* allocate host and private_data */
-	host = ata_host_alloc(&pdev->dev, NULL, 0);
-	hpriv = kzalloc(sizeof(*hpriv), GFP_KERNEL);
-	if (!host || !hpriv) {
-		reason = "failed to allocate host and private_data";
+	/* allocate host */
+	host = ata_host_alloc(&pdev->dev, NULL, 0, sizeof(*hpriv));
+	if (!host) {
+		reason = "failed to allocate host";
 		rc = -ENOMEM;
 		goto err;
 	}
-	host->private_data = hpriv;
+	hpriv = host->private_data;
 
 	/* acquire ATA PCI resources */
 	rc = ata_pci_acquire_resources(host, 0, &reason);
@@ -1565,22 +1563,11 @@ static int ahci_init_one(struct pci_dev 
 	if (mmio_base)
 		pci_iounmap(pdev, mmio_base);
 	ata_pci_host_destroy(host);
-	kfree(hpriv);
 
 	dev_printk(KERN_ERR, &pdev->dev, "%s (error=%d)\n", reason, rc);
 	return rc;
 }
 
-static void ahci_remove_one (struct pci_dev *pdev)
-{
-	struct device *dev = pci_dev_to_dev(pdev);
-	struct ata_host *host = dev_get_drvdata(dev);
-	struct ahci_host_priv *hpriv = host->private_data;
-
-	ata_pci_remove_one(pdev);
-	kfree(hpriv);
-}
-
 static int __init ahci_init(void)
 {
 	return pci_register_driver(&ahci_pci_driver);
drivers/ata/ata_piix.c +3 -6
index 21f19f2..7826bdf 100644
@@ -805,9 +805,9 @@ static void __devinit piix_init_pcs(stru
 
 static void __devinit piix_init_sata_map(struct pci_dev *pdev,
 					 struct ata_port_info *pinfo,
+					 struct piix_host_priv *hpriv,
 					 const struct piix_map_db *map_db)
 {
-	struct piix_host_priv *hpriv = pinfo[0].private_data;
 	const unsigned int *map;
 	int i, invalid_map = 0;
 	u8 map_value;
@@ -831,7 +831,6 @@ static void __devinit piix_init_sata_map
 		case IDE:
 			WARN_ON((i & 1) || map[i + 1] != IDE);
 			pinfo[i / 2] = piix_port_info[ich5_pata];
-			pinfo[i / 2].private_data = hpriv;
 			i++;
 			printk(" IDE IDE");
 			break;
@@ -890,8 +889,6 @@ static int piix_init_one (struct pci_dev
 
 	port_info[0] = piix_port_info[ent->driver_data];
 	port_info[1] = piix_port_info[ent->driver_data];
-	port_info[0].private_data = hpriv;
-	port_info[1].private_data = hpriv;
 
 	port_flags = port_info[0].flags;
 
@@ -907,7 +904,7 @@ static int piix_init_one (struct pci_dev
 
 	/* Initialize SATA map */
 	if (port_flags & ATA_FLAG_SATA) {
-		piix_init_sata_map(pdev, port_info,
+		piix_init_sata_map(pdev, port_info, hpriv,
 				   piix_map_db_table[ent->driver_data]);
 		piix_init_pcs(pdev, piix_map_db_table[ent->driver_data]);
 	}
@@ -930,7 +927,7 @@ static int piix_init_one (struct pci_dev
 		port_info[1].mwdma_mask = 0;
 		port_info[1].udma_mask = 0;
 	}
-	return ata_pci_init_one(pdev, ppinfo, 2);
+	return ata_pci_init_one(pdev, ppinfo, 2, hpriv);
 }
 
 static void piix_remove_one(struct pci_dev *pdev)
drivers/ata/libata-core.c +22 -14
index 96709de..457ee4a 100644
@@ -5408,11 +5408,16 @@ static struct ata_port *ata_port_alloc(s
  *	@dev: generic device this host is associated with
  *	@sht: template for SCSI host
  *	@n_ports: number of ATA ports associated with this host (can be 0)
+ *	@hpriv_sz: size of host private data
  *
  *	Allocate and initialize basic ATA host resources.  LLD calls
  *	this function to allocate a host, initializes it fully and
  *	attaches it using ata_host_attach().
  *
+ *	If @hpriv_sz is not zero, host private data of the specified
+ *	size is allocated together with host.  Host private data is
+ *	aligned to __alignof__(long long).
+ *
  *	RETURNS:
  *	Allocate ATA host on success, NULL on failure.
  *
@@ -5420,22 +5425,28 @@ static struct ata_port *ata_port_alloc(s
  *	Inherited from calling layer (may sleep).
  */
 struct ata_host *ata_host_alloc(struct device *dev,
-				struct scsi_host_template *sht, int n_ports)
+				struct scsi_host_template *sht, int n_ports,
+				size_t hpriv_sz)
 {
 	struct ata_host *host;
-	size_t sz;
+	size_t hs_sz;
 
 	DPRINTK("ENTER\n");
 
 	/* alloc a container for our list of ATA ports (buses) */
-	sz = sizeof(struct ata_host) + n_ports * sizeof(void *);
+	hs_sz = sizeof(struct ata_host) + n_ports * sizeof(void *);
 	if (n_ports == 0)
-		sz += ATA_MAX_PORTS * sizeof(void *);
+		hs_sz += ATA_MAX_PORTS * sizeof(void *);
+	if (hpriv_sz)
+		hs_sz = ALIGN(hs_sz, __alignof__(long long));
 
-	host = kzalloc(sz, GFP_KERNEL);
+	host = kzalloc(hs_sz + hpriv_sz, GFP_KERNEL);
 	if (!host)
 		return NULL;
 
+	if (hpriv_sz)
+		host->private_data = (void *)host + hs_sz;
+
 	spin_lock_init(&host->lock);
 	host->dev = dev;
 	INIT_LIST_HEAD(&host->irq_list);
@@ -5499,8 +5510,6 @@ static void __ata_host_init_pinfo(struct
 {
 	int i;
 
-	if (host->private_data == NULL)
-		host->private_data = pinfo[0]->private_data;
 	host->ops = pinfo[0]->port_ops;
 
 	for (i = 0; i < host->n_ports; i++) {
@@ -5517,9 +5526,6 @@ static void __ata_host_init_pinfo(struct
 		ap->udma_mask = pi->udma_mask;
 		ap->flags |= pi->flags;
 		ap->ops = pi->port_ops;
-
-		WARN_ON(pi->private_data &&
-			pi->private_data != host->private_data);
 	}
 }
 
@@ -5528,6 +5534,7 @@ static void __ata_host_init_pinfo(struct
  *	@dev: generic device this host is associated with
  *	@pinfo: ATA port_info to initialize host with
  *	@n_ports: number of ATA ports attached to this host
+ *	@hpriv_sz: size of host private data
  *
  *	Allocate ATA host and initialize with info from @pi.
  *
@@ -5539,14 +5546,14 @@ static void __ata_host_init_pinfo(struct
  */
 struct ata_host *ata_host_alloc_pinfo(struct device *dev,
 				      const struct ata_port_info *pinfo,
-				      int n_ports)
+				      int n_ports, size_t hpriv_sz)
 {
 	struct ata_host *host;
 
 	if (!n_ports)
 		return NULL;
 
-	host = ata_host_alloc(dev, pinfo->sht, n_ports);
+	host = ata_host_alloc(dev, pinfo->sht, n_ports, hpriv_sz);
 	if (host)
 		__ata_host_init_pinfo(host, &pinfo, n_ports, 0);
 	return host;
@@ -5557,6 +5564,7 @@ struct ata_host *ata_host_alloc_pinfo(st
  *	@dev: generic device this host is associated with
  *	@pinfo_ar: array of ATA port_info to initialize host with
  *	@n_ports: number of ATA ports attached to this host
+ *	@hpriv_sz: size of host private data
  *
  *	Allocate ATA host and initialize with info from @pinfo_ar.
  *
@@ -5568,14 +5576,14 @@ struct ata_host *ata_host_alloc_pinfo(st
  */
 struct ata_host *ata_host_alloc_pinfo_ar(struct device *dev,
 					 const struct ata_port_info **pinfo_ar,
-					 int n_ports)
+					 int n_ports, size_t hpriv_sz)
 {
 	struct ata_host *host;
 
 	if (!n_ports)
 		return NULL;
 
-	host = ata_host_alloc(dev, pinfo_ar[0]->sht, n_ports);
+	host = ata_host_alloc(dev, pinfo_ar[0]->sht, n_ports, hpriv_sz);
 	if (host)
 		__ata_host_init_pinfo(host, pinfo_ar, n_ports, 1);
 	return host;
drivers/ata/libata-pci.c +8 -4
index 997bd14..7fee3c0 100644
@@ -412,6 +412,7 @@ void ata_pci_free_msix_irqs(struct ata_h
  *	@pinfo_ar: array of pointers to ATA port_info
  *	@mask: available port mask
  *	@legacy_mask: legacy port mask
+ *	@hpriv_sz: size of host private data
  *	@r_host: out arg for prepared ATA host
  *
  *	Allocate and initialize ATA host for PCI ATA device @pdev.
@@ -428,7 +429,7 @@ void ata_pci_free_msix_irqs(struct ata_h
  */
 int ata_pci_host_prepare(struct pci_dev *pdev, struct ata_port_info **pinfo_ar,
 			 unsigned int mask, unsigned int legacy_mask,
-			 struct ata_host **r_host)
+			 size_t hpriv_sz, struct ata_host **r_host)
 {
 	struct ata_host *host = NULL;
 	struct ata_port_info dummy_pinfo;
@@ -459,7 +460,7 @@ int ata_pci_host_prepare(struct pci_dev 
 	}
 
 	/* allocate host */
-	host = ata_host_alloc_pinfo_ar(&pdev->dev, pi, n_ports);
+	host = ata_host_alloc_pinfo_ar(&pdev->dev, pi, n_ports, hpriv_sz);
 	if (!host) {
 		reason = "failed to allocate host";
 		rc = -ENOMEM;
@@ -564,6 +565,7 @@ void ata_pci_host_destroy(struct ata_hos
  *	@pdev: Controller to be initialized
  *	@pinfo_ar: Information from low-level host driver
  *	@n_ports: Number of ports attached to host controller
+ *	@hpriv: host private data
  *
  *	This is a helper function which can be called from a driver's
  *	xxx_init_one() probe function if the hardware uses traditional
@@ -580,7 +582,7 @@ void ata_pci_host_destroy(struct ata_hos
  *	Zero on success, negative on errno-based value on error.
  */
 int ata_pci_init_one(struct pci_dev *pdev, struct ata_port_info **pinfo_ar,
-		     unsigned int n_ports)
+		     unsigned int n_ports, void *hpriv)
 
 {
 	unsigned int mask = ATA_PORT_PRIMARY | ATA_PORT_SECONDARY;
@@ -607,10 +609,12 @@ int ata_pci_init_one(struct pci_dev *pde
 	}
 
 	/* prep */
-	rc = ata_pci_host_prepare(pdev, pinfo_ar, mask, legacy_mask, &host);
+	rc = ata_pci_host_prepare(pdev, pinfo_ar, mask, legacy_mask, 0, &host);
 	if (rc)
 		return rc;
 
+	host->private_data = hpriv;
+
 	/* attach */
 	rc = ata_host_attach(host);
 	if (rc)
drivers/ata/pdc_adma.c +1 -1
index d999235..f21e0db 100644
@@ -621,7 +621,7 @@ static int adma_ata_init_one(struct pci_
 	/* alloc host */
 	host = ata_host_alloc_pinfo(&pdev->dev,
 				    &adma_port_info[board_idx],
-				    ADMA_PORTS);
+				    ADMA_PORTS, 0);
 	if (!host) {
 		reason = "failed to allocate host";
 		rc = -ENOMEM;
drivers/ata/sata_mv.c +7 -20
index 9a87937..865cfd8 100644
@@ -348,7 +348,6 @@ static void mv_qc_prep_iie(struct ata_qu
 static unsigned int mv_qc_issue(struct ata_queued_cmd *qc);
 static void mv_eng_timeout(struct ata_port *ap);
 static int mv_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
-static void mv_remove_one(struct pci_dev *pdev);
 
 static void mv5_phy_errata(struct mv_host_priv *hpriv, void __iomem *mmio,
 			   unsigned int port);
@@ -542,7 +541,7 @@ static struct pci_driver mv_pci_driver =
 	.name			= DRV_NAME,
 	.id_table		= mv_pci_tbl,
 	.probe			= mv_init_one,
-	.remove			= mv_remove_one,
+	.remove			= ata_pci_remove_one,
 };
 
 static const struct mv_hw_ops mv5xxx_ops = {
@@ -2308,8 +2307,7 @@ static int mv_init_one(struct pci_dev *p
 	static int printed_version = 0;
 	unsigned int board_idx = (unsigned int)ent->driver_data;
 	const struct ata_port_info *pinfo = &mv_port_info[board_idx];
-	struct ata_host *host = NULL;
-	struct mv_host_priv *hpriv = NULL;
+	struct ata_host *host;
 	void __iomem *mmio_base = NULL;
 	const char *reason;
 	int n_ports, rc;
@@ -2317,17 +2315,16 @@ static int mv_init_one(struct pci_dev *p
 	if (!printed_version++)
 		dev_printk(KERN_INFO, &pdev->dev, "version " DRV_VERSION "\n");
 
-	/* alloc host and hpriv */
+	/* alloc host */
 	n_ports = MV_PORTS_PER_HC * mv_get_hc_count(pinfo->flags);
 
-	host = ata_host_alloc_pinfo(&pdev->dev, pinfo, n_ports);
-	hpriv = kzalloc(sizeof(*hpriv), GFP_KERNEL);
-	if (!host || !hpriv) {
-		reason = "failed to allocate host and private_data";
+	host = ata_host_alloc_pinfo(&pdev->dev, pinfo, n_ports,
+				    sizeof(struct mv_host_priv));
+	if (!host) {
+		reason = "failed to allocate host";
 		rc = -ENOMEM;
 		goto err;
 	}
-	host->private_data = hpriv;
 
 	/* acquire generic ATA PCI resources */
 	rc = ata_pci_acquire_resources(host, DMA_64BIT_MASK, &reason);
@@ -2373,21 +2370,11 @@ static int mv_init_one(struct pci_dev *p
 	if (mmio_base)
 		pci_iounmap(pdev, mmio_base);
 	ata_pci_host_destroy(host);
-	kfree(hpriv);
 
 	dev_printk(KERN_ERR, &pdev->dev, "%s (error=%d)\n", reason, rc);
 	return rc;
 }
 
-static void mv_remove_one(struct pci_dev *pdev)
-{
-	struct ata_host *host = dev_get_drvdata(&pdev->dev);
-	struct mv_host_priv *hpriv = host->private_data;
-
-	ata_pci_remove_one(pdev);
-	kfree(hpriv);
-}
-
 static int __init mv_init(void)
 {
 	return pci_register_driver(&mv_pci_driver);
drivers/ata/sata_nv.c +1 -1
index 59ee99a..00894a0 100644
@@ -499,7 +499,7 @@ static int nv_init_one (struct pci_dev *
 	/* alloc host */
 	host = ata_host_alloc_pinfo(&pdev->dev,
 				    &nv_port_info[ent->driver_data],
-				    NV_PORTS);
+				    NV_PORTS, 0);
 	if (!host) {
 		reason = "failed to allocate host";
 		rc = -ENOMEM;
drivers/ata/sata_promise.c +8 -20
index 92571f7..6e33abb 100644
@@ -98,7 +98,6 @@ struct pdc_host_priv {
 static u32 pdc_sata_scr_read (struct ata_port *ap, unsigned int sc_reg);
 static void pdc_sata_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
 static int pdc_ata_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
-static void pdc_ata_remove_one(struct pci_dev *pdev);
 static void pdc_eng_timeout(struct ata_port *ap);
 static int pdc_port_start(struct ata_port *ap);
 static void pdc_port_stop(struct ata_port *ap);
@@ -298,7 +297,7 @@ static struct pci_driver pdc_ata_pci_dri
 	.name			= DRV_NAME,
 	.id_table		= pdc_ata_pci_tbl,
 	.probe			= pdc_ata_init_one,
-	.remove			= pdc_ata_remove_one,
+	.remove			= ata_pci_remove_one,
 };
 
 
@@ -693,8 +692,8 @@ static int pdc_ata_init_one(struct pci_d
 	static int printed_version;
 	unsigned int board_idx = (unsigned int) ent->driver_data;
 	const struct ata_port_info *pinfo = &pdc_port_info[board_idx];
-	struct ata_host *host = NULL;
-	struct pdc_host_priv *hp = NULL;
+	struct ata_host *host;
+	struct pdc_host_priv *hp;
 	void __iomem *mmio_base = NULL;
 	unsigned long base;
 	const char *reason;
@@ -703,16 +702,15 @@ static int pdc_ata_init_one(struct pci_d
 	if (!printed_version++)
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
 
-	/* allocate host and private_data */
+	/* allocate host */
 	host = ata_host_alloc_pinfo(&pdev->dev, pinfo,
-				    PDC_FLAG2NPORTS(pinfo->flags));
-	hp = kzalloc(sizeof(*hp), GFP_KERNEL);
-	if (!host || !hp) {
-		reason = "failed to allocate host and private_data";
+				    PDC_FLAG2NPORTS(pinfo->flags), sizeof(*hp));
+	if (!host) {
+		reason = "failed to allocate host";
 		rc = -ENOMEM;
 		goto err;
 	}
-	host->private_data = hp;
+	hp = host->private_data;
 
 	/* acquire generic ATA PCI resources */
 	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, &reason);
@@ -771,21 +769,11 @@ static int pdc_ata_init_one(struct pci_d
 	if (mmio_base)
 		pci_iounmap(pdev, mmio_base);
 	ata_pci_host_destroy(host);
-	kfree(hp);
 
 	dev_printk(KERN_ERR, &pdev->dev, "%s (error=%d)\n", reason, rc);
 	return rc;
 }
 
-static void pdc_ata_remove_one(struct pci_dev *pdev)
-{
-	struct ata_host *host = dev_get_drvdata(&pdev->dev);
-	struct pdc_host_priv *hp = host->private_data;
-
-	ata_pci_remove_one(pdev);
-	kfree(hp);
-}
-
 static int __init pdc_ata_init(void)
 {
 	return pci_register_driver(&pdc_ata_pci_driver);
drivers/ata/sata_qstor.c +2 -2
index c564ac4..420eb58 100644
@@ -608,8 +608,8 @@ static int qs_ata_init_one(struct pci_de
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
 
 	/* alloc host */
-	host = ata_host_alloc_pinfo(&pdev->dev,
-				    &qs_port_info[board_idx], QS_PORTS);
+	host = ata_host_alloc_pinfo(&pdev->dev, &qs_port_info[board_idx],
+				    QS_PORTS, 0);
 	if (!host) {
 		reason = "failed to allocate host";
 		rc = -ENOMEM;
drivers/ata/sata_sil.c +1 -1
index 3619f86..9cabbb5 100644
@@ -618,7 +618,7 @@ static int sil_init_one (struct pci_dev 
 	/* alloc host */
 	host = ata_host_alloc_pinfo(&pdev->dev,
 				    &sil_port_info[ent->driver_data],
-				    (ent->driver_data == sil_3114) ? 4 : 2);
+				    (ent->driver_data == sil_3114) ? 4 : 2, 0);
 	if (!host) {
 		reason = "failed to allocate host";
 		rc = -ENOMEM;
drivers/ata/sata_sil24.c +8 -11
index 3c6d668..9f1ad0f 100644
@@ -1046,8 +1046,8 @@ static int sil24_init_one(struct pci_dev
 	static int printed_version = 0;
 	unsigned int board_id = (unsigned int)ent->driver_data;
 	struct ata_port_info *pinfo = &sil24_port_info[board_id];
-	struct ata_host *host = NULL;
-	struct sil24_host_priv *hpriv = NULL;
+	struct ata_host *host;
+	struct sil24_host_priv *hpriv;
 	void __iomem *host_base = NULL;
 	void __iomem *port_base = NULL;
 	const char *reason;
@@ -1057,17 +1057,16 @@ static int sil24_init_one(struct pci_dev
 	if (!printed_version++)
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
 
-	/* alloc host and hpriv */
+	/* alloc host */
 	host = ata_host_alloc_pinfo(&pdev->dev, pinfo,
-				    SIL24_FLAG2NPORTS(pinfo->flags));
-	hpriv = kzalloc(sizeof(*hpriv), GFP_KERNEL);
-
-	if (!host || !hpriv) {
-		reason = "failed to allocate host and private_data";
+				    SIL24_FLAG2NPORTS(pinfo->flags),
+				    sizeof(*hpriv));
+	if (!host) {
+		reason = "failed to allocate host";
 		rc = -ENOMEM;
 		goto err;
 	}
-	host->private_data = hpriv;
+	hpriv = host->private_data;
 
 	/* acquire generic ATA PCI resources */
 	rc = ata_pci_acquire_resources(host, DMA_64BIT_MASK, &reason);
@@ -1137,7 +1136,6 @@ static int sil24_init_one(struct pci_dev
 	if (port_base)
 		pci_iounmap(pdev, port_base);
 	ata_pci_host_destroy(host);
-	kfree(hpriv);
 
 	dev_printk(KERN_ERR, &pdev->dev, "%s (error=%d)\n", reason, rc);
 	return rc;
@@ -1155,7 +1153,6 @@ static void sil24_remove_one(struct pci_
 	pci_iounmap(pdev, hpriv->port_base);
 
 	ata_pci_host_destroy(host);
-	kfree(hpriv);
 }
 
 static int sil24_pci_device_resume(struct pci_dev *pdev)
drivers/ata/sata_sis.c +1 -1
index 56ad81d..a696693 100644
@@ -247,7 +247,7 @@ static int sis_init_one (struct pci_dev 
 		dev_printk(KERN_INFO, &pdev->dev, "version " DRV_VERSION "\n");
 
 	/* alloc host */
-	host = ata_host_alloc_pinfo(&pdev->dev, &sis_port_info, 2);
+	host = ata_host_alloc_pinfo(&pdev->dev, &sis_port_info, 2, 0);
 	if (!host) {
 		reason = "failed to allocate host";
 		rc = -ENOMEM;
drivers/ata/sata_svw.c +1 -1
index 79c526d..d51e860 100644
@@ -386,7 +386,7 @@ static int k2_sata_init_one (struct pci_
 	 * ent->driver_data and the same is done here.  Verify the bug
 	 * and fix.
 	 */
-	host = ata_host_alloc_pinfo(&pdev->dev, &k2_sata_port_info, 4);
+	host = ata_host_alloc_pinfo(&pdev->dev, &k2_sata_port_info, 4, 0);
 	if (!host) {
 		reason = "failed to allocate host";
 		rc = -ENOMEM;
drivers/ata/sata_sx4.c +8 -11
index 794dbed..4a235ab 100644
@@ -1353,8 +1353,8 @@ static int pdc_sata_init_one(struct pci_
 {
 	static int printed_version;
 	unsigned int board_idx = (unsigned int) ent->driver_data;
-	struct ata_host *host = NULL;
-	struct pdc_host_priv *hpriv = NULL;
+	struct ata_host *host;
+	struct pdc_host_priv *hpriv;
 	void __iomem *mmio_base = NULL;
 	void __iomem *dimm_mmio = NULL;
 	unsigned long base;
@@ -1364,17 +1364,16 @@ static int pdc_sata_init_one(struct pci_
 	if (!printed_version++)
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
 
-	/* alloc host and hpriv */
+	/* alloc host */
 	host = ata_host_alloc_pinfo(&pdev->dev,
-				    &pdc_port_info[board_idx], 4);
-	hpriv = kzalloc(sizeof(*hpriv), GFP_KERNEL);
-
-	if (!host || !hpriv) {
-		reason = "failed to allocate host and private_data";
+					    &pdc_port_info[board_idx],
+					    4, sizeof(*hpriv));
+	if (!host) {
+		reason = "failed to allocate host";
 		rc = -ENOMEM;
 		goto err;
 	}
-	host->private_data = hpriv;
+	hpriv = host->private_data;
 
 	/* acquire generic ATA PCI resources */
 	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, &reason);
@@ -1437,7 +1436,6 @@ static int pdc_sata_init_one(struct pci_
 	if (dimm_mmio)
 		pci_iounmap(pdev, dimm_mmio);
 	ata_pci_host_destroy(host);
-	kfree(hpriv);
 
 	dev_printk(KERN_ERR, &pdev->dev, "%s (error=%d)\n", reason, rc);
 	return rc;
@@ -1456,7 +1454,6 @@ static void pdc_sata_remove_one(struct p
 	pci_iounmap(pdev, hpriv->dimm_mmio);
 
 	ata_pci_host_destroy(host);
-	kfree(hpriv);
 }
 
 
drivers/ata/sata_uli.c +9 -22
index 540bdd5..5ffdc2b 100644
@@ -57,7 +57,6 @@ struct uli_priv {
 };
 
 static int uli_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
-static void uli_remove_one(struct pci_dev *pdev);
 static u32 uli_scr_read (struct ata_port *ap, unsigned int sc_reg);
 static void uli_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
 
@@ -73,7 +72,7 @@ static struct pci_driver uli_pci_driver 
 	.name			= DRV_NAME,
 	.id_table		= uli_pci_tbl,
 	.probe			= uli_init_one,
-	.remove			= uli_remove_one,
+	.remove			= ata_pci_remove_one,
 };
 
 static struct scsi_host_template uli_sht = {
@@ -184,8 +183,8 @@ static int uli_init_one(struct pci_dev *
 {
 	static int printed_version;
 	unsigned int board_idx = (unsigned int) ent->driver_data;
-	struct ata_host *host = NULL;
-	struct uli_priv *hpriv = NULL;
+	struct ata_host *host;
+	struct uli_priv *hpriv;
 	struct ata_ioports *ioaddr;
 	const char *reason;
 	int rc;
@@ -193,17 +192,16 @@ static int uli_init_one(struct pci_dev *
 	if (!printed_version++)
 		dev_printk(KERN_INFO, &pdev->dev, "version " DRV_VERSION "\n");
 
-	/* alloc host and hpriv */
+	/* alloc host */
 	host = ata_host_alloc_pinfo(&pdev->dev, &uli_port_info,
-				    board_idx == uli_5287 ? 4 : 2);
-	hpriv = kzalloc(sizeof(*hpriv), GFP_KERNEL);
-
-	if (!host || !hpriv) {
-		reason = "failed to allocate host and private_data";
+					    board_idx == uli_5287 ? 4 : 2,
+					    sizeof(*hpriv));
+	if (!host) {
+		reason = "failed to allocate host";
 		rc = -ENOMEM;
 		goto err;
 	}
-	host->private_data = hpriv;
+	hpriv = host->private_data;
 
 	/* acquire generic ATA PCI resources */
 	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, &reason);
@@ -272,21 +270,10 @@ static int uli_init_one(struct pci_dev *
 
  err:
 	ata_pci_host_destroy(host);
-	kfree(hpriv);
-
 	dev_printk(KERN_ERR, &pdev->dev, "%s (error=%d)\n", reason, rc);
 	return rc;
 }
 
-static void uli_remove_one(struct pci_dev *pdev)
-{
-	struct ata_host *host = dev_get_drvdata(&pdev->dev);
-	struct uli_priv *hpriv = host->private_data;
-
-	ata_pci_remove_one(pdev);
-	kfree(hpriv);
-}
-
 static int __init uli_init(void)
 {
 	return pci_register_driver(&uli_pci_driver);
drivers/ata/sata_via.c +1 -2
index db7b09e..f0bc20e 100644
@@ -254,8 +254,7 @@ static int svia_init_one(struct pci_dev 
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
 
 	/* alloc host */
-	host = ata_host_alloc_pinfo(&pdev->dev, &svia_port_info,
-				    N_PORTS);
+	host = ata_host_alloc_pinfo(&pdev->dev, &svia_port_info, N_PORTS, 0);
 	if (!host) {
 		reason = "failed to allocate host";
 		rc = -ENOMEM;
drivers/ata/sata_vsc.c +2 -1
index ac00d17..71ea8d9 100644
@@ -357,7 +357,8 @@ static int __devinit vsc_sata_init_one (
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
 
 	/* alloc host */
-	host = ata_host_alloc_pinfo(&pdev->dev, &vsc_sata_port_info, 4);
+	host = ata_host_alloc_pinfo(&pdev->dev, &vsc_sata_port_info,
+					    4, 0);
 	if (!host) {
 		reason = "failed to allocate host";
 		rc = -ENOMEM;
include/linux/libata.h +8 -6
index 791fa97..f72cc73 100644
@@ -657,7 +657,6 @@ struct ata_port_info {
 	unsigned long		mwdma_mask;
 	unsigned long		udma_mask;
 	const struct ata_port_operations *port_ops;
-	void 			*private_data;
 	/* fields for BMDMA init */
 	irqreturn_t (*irq_handler)(int, void *, struct pt_regs *);
 };
@@ -730,10 +729,10 @@ extern void ata_pci_free_msix_irqs(struc
 extern int ata_pci_host_prepare(struct pci_dev *pdev,
 				struct ata_port_info **pinfo_ar,
 				unsigned int mask, unsigned int legacy_mask,
-				struct ata_host **r_host);
+				size_t hpriv_sz, struct ata_host **r_host);
 extern void ata_pci_host_destroy(struct ata_host *host);
 extern int ata_pci_init_one (struct pci_dev *pdev, struct ata_port_info **port_info,
-			     unsigned int n_ports);
+			     unsigned int n_ports, void *hpriv);
 extern void ata_pci_remove_one (struct pci_dev *pdev);
 extern void ata_pci_device_do_suspend(struct pci_dev *pdev, pm_message_t mesg);
 extern void ata_pci_device_do_resume(struct pci_dev *pdev);
@@ -742,13 +741,16 @@ extern int ata_pci_device_resume(struct 
 extern int ata_pci_clear_simplex(struct pci_dev *pdev);
 #endif /* CONFIG_PCI */
 extern struct ata_host *ata_host_alloc(struct device *dev,
-			struct scsi_host_template *sht, int n_ports);
+			struct scsi_host_template *sht, int n_ports,
+			size_t hpriv_sz);
 extern int ata_host_add_ports(struct ata_host *host,
 			struct scsi_host_template *sht, int n_ports);
 extern struct ata_host *ata_host_alloc_pinfo(struct device *dev,
-			const struct ata_port_info *pinfo, int n_ports);
+			const struct ata_port_info *pinfo, int n_ports,
+			size_t hpriv_sz);
 extern struct ata_host *ata_host_alloc_pinfo_ar(struct device *dev,
-			const struct ata_port_info **pinfo_ar, int n_ports);
+			const struct ata_port_info **pinfo_ar, int n_ports,
+			size_t hpriv_sz);
 extern int ata_host_add_ports_pinfo(struct ata_host *host,
 			const struct ata_port_info *pinfo, int n_ports);
 extern int ata_host_add_ports_pinfo_ar(struct ata_host *host,
Signature · 1 line
1.4.1.1
#37 lore raw

Re: [PATCH 16/20] libata: make ata_host_alloc() take care of hpriv alloc/free

Quoted 5 lines · Tejun Heo

Transfer responsibility to allocate/init hpriv from LLD/port_info to host allocation. All host allocation/prepare routines now take @hpriv_hz and initialize host->private_data accordingly. hpriv area is aligned to __alignof__(long long) and zeroed.

Quoted 8 lines

As hpriv handling is moved to host allocation, pinfo->private_data is removed. The only user of pinfo->private_data was ata_pci_init_one() which is updated to take @hpriv argument directly. This change makes port_info carry one less host-wide info - which is good because mismatched scope causes confusion and bug (e.g. ata_piix private_data init bug).

A nice model to follow is netdev_priv() and the alloc_netdev() allocation code...

author
#38 lore raw

PATCH 17/20 libata: make ata_pci_acquire_resources() handle iomap

In reply to Tejun Heo

Make ata_pci_acquire_resources() handle iomap. @bar_mask is added and for each set bit respective PCI BAR is iomapped and stored in host->iomap[BAR#]. All iomaps stored in host->iomap[] are freed by ata_pci_release_resources().

Also, acquire_resources() also map legacy IO areas and store them in host->legacy_iomap[p][a] where @p is port number (0=PRIMARY/1=SECONDARY) and @a indicates CMD or CTL.

This function only implements the feature.

Took hints from iomap patch by Jeff Garzik <[email protected]>.

---
 drivers/ata/ahci.c          |    2 +-
 drivers/ata/libata-legacy.c |   34 ++++++++++++++++++++++++++++++----
 drivers/ata/libata-pci.c    |   38 +++++++++++++++++++++++++++++++++++---
 drivers/ata/pdc_adma.c      |    2 +-
 drivers/ata/sata_mv.c       |    2 +-
 drivers/ata/sata_nv.c       |    2 +-
 drivers/ata/sata_promise.c  |    2 +-
 drivers/ata/sata_qstor.c    |    2 +-
 drivers/ata/sata_sil.c      |    2 +-
 drivers/ata/sata_sil24.c    |    2 +-
 drivers/ata/sata_sis.c      |    2 +-
 drivers/ata/sata_svw.c      |    2 +-
 drivers/ata/sata_sx4.c      |    2 +-
 drivers/ata/sata_uli.c      |    2 +-
 drivers/ata/sata_via.c      |    2 +-
 drivers/ata/sata_vsc.c      |    2 +-
 include/linux/libata.h      |    4 ++++
 17 files changed, 83 insertions(+), 21 deletions(-)
drivers/ata/ahci.c +1 -1
index 1ad49fe..92d0394 100644
@@ -1488,7 +1488,7 @@ static int ahci_init_one(struct pci_dev 
 	hpriv = host->private_data;
 
 	/* acquire ATA PCI resources */
-	rc = ata_pci_acquire_resources(host, 0, &reason);
+	rc = ata_pci_acquire_resources(host, 0, 0, &reason);
 	if (rc)
 		goto err;
 
drivers/ata/libata-legacy.c +30 -4
index c519602..eac4d06 100644
@@ -52,10 +52,12 @@ static unsigned long ata_legacy_addr(int
 	}
 }
 
-static int ata_legacy_acquire_port(struct ata_port *ap)
+static int ata_legacy_acquire_port(struct ata_port *ap, const char **p_reason)
 {
 	struct ata_host *host = ap->host;
+	void __iomem **legacy_iomap = host->legacy_iomap[ap->port_no];
 	unsigned long cmd_addr = ata_legacy_addr(ap->port_no, 0);
+	unsigned long ctl_addr = ata_legacy_addr(ap->port_no, 1);
 
 	if (request_region(cmd_addr, 8, ata_drv_name(host)) != NULL)
 		host->legacy_flags |= ATA_LEGACY_RES_PRI << ap->port_no;
@@ -75,6 +77,13 @@ static int ata_legacy_acquire_port(struc
 		printk("ata: 0x%0lX IDE port preallocated\n", cmd_addr);
 	}
 
+	legacy_iomap[0] = ioport_map(cmd_addr, 8);
+	legacy_iomap[1] = ioport_map(ctl_addr, 4);
+	if (!legacy_iomap[0] || (ctl_addr && !legacy_iomap[1])) {
+		*p_reason = "failed to iomap legacy TF";
+		return -ENOMEM;
+	}
+
 	return 0;
 }
 
@@ -97,18 +106,28 @@ static int ata_legacy_acquire_port(struc
  */
 int ata_legacy_acquire_resources(struct ata_host *host, const char **p_reason)
 {
-	int i;
+	const char *reason;
+	int i, rc;
 
 	for (i = 0; i < 2; i++) {
 		if (!(host->legacy_flags & (ATA_PORT_PRIMARY << i)))
 			continue;
 		BUG_ON(i >= host->n_ports);
 
-		if (ata_legacy_acquire_port(host->ports[i]))
+		rc = ata_legacy_acquire_port(host->ports[i], &reason);
+		if (rc == -EBUSY)
 			host->ports[i]->ops = &ata_dummy_port_ops;
+		else if (rc)
+			goto err;
 	}
 
 	return 0;
+
+ err:
+	if (p_reason)
+		*p_reason = reason;
+	ata_legacy_release_resources(host);
+	return rc;
 }
 
 /**
@@ -122,9 +141,16 @@ int ata_legacy_acquire_resources(struct 
  */
 void ata_legacy_release_resources(struct ata_host *host)
 {
-	int i;
+	void __iomem *(*legacy_iomap)[2] = host->legacy_iomap;
+	int i, j;
 
 	for (i = 0; i < 2; i++) {
+		for (j = 0; j < 2; j++)
+			if (legacy_iomap[i][j]) {
+				ioport_unmap(legacy_iomap[i][j]);
+				legacy_iomap[i][j] = NULL;
+			}
+
 		if (host->legacy_flags & (ATA_LEGACY_RES_PRI << i)) {
 			release_region(ata_legacy_addr(i, 0), 8);
 			host->legacy_flags &= ~(ATA_LEGACY_RES_PRI << i);
drivers/ata/libata-pci.c +35 -3
index 7fee3c0..18a1952 100644
@@ -133,6 +133,7 @@ int ata_pci_set_dma_mask(struct pci_dev 
  *	ata_pci_acquire_resources - acquire default PCI resources
  *	@host: target ATA host to acquire PCI resources for
  *	@dma_mask: DMA mask
+ *	@bar_mask: mask of PCI bars to iomap
  *	@p_reason: out arg for error message (can be NULL)
  *
  *	Acquire default ATA PCI resources.
@@ -144,11 +145,12 @@ int ata_pci_set_dma_mask(struct pci_dev 
  *	0 on success, -errno otherwise.
  */
 int ata_pci_acquire_resources(struct ata_host *host, u64 dma_mask,
-			      const char **p_reason)
+			      unsigned int bar_mask, const char **p_reason)
 {
 	struct pci_dev *pdev = to_pci_dev(host->dev);
+	void __iomem **iomap = host->iomap;
 	const char *reason;
-	int rc;
+	int i, rc;
 
 	/* acquire generic resources */
 
@@ -181,6 +183,28 @@ int ata_pci_acquire_resources(struct ata
 	if (rc)
 		goto err;
 
+	/* iomap PCI BARs */
+	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+		if (!(bar_mask & (1 << i)))
+			continue;
+
+		WARN_ON(iomap[i]);
+
+		if (!pci_resource_start(pdev, i) ||
+		    !pci_resource_len(pdev, i)) {
+			reason = "iomap requested for null PCI BAR";
+			rc = -EIO;
+			goto err;
+		}
+
+		iomap[i] = pci_iomap(pdev, i, 0);
+		if (!iomap[i]) {
+			rc = -ENOMEM;
+			reason = "failed to iomap PCI BAR";
+			goto err;
+		}
+	}
+
 	return 0;
 
  err:
@@ -202,6 +226,14 @@ int ata_pci_acquire_resources(struct ata
 void ata_pci_release_resources(struct ata_host *host)
 {
 	struct pci_dev *pdev = to_pci_dev(host->dev);
+	void __iomem **iomap = host->iomap;
+	int i;
+
+	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
+		if (iomap[i]) {
+			pci_iounmap(pdev, iomap[i]);
+			iomap[i] = NULL;
+		}
 
 	if (host->pci_flags & ATA_PCI_RES_GEN) {
 		pci_release_regions(pdev);
@@ -470,7 +502,7 @@ int ata_pci_host_prepare(struct pci_dev 
 	/* acquire PCI and legacy resources and init host accordingly */
 	host->legacy_flags |= legacy_mask & ATA_LEGACY_MASK;
 
-	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, &reason);
+	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, 0, &reason);
 	if (rc)
 		goto err;
 
drivers/ata/pdc_adma.c +1 -1
index f21e0db..7f0c104 100644
@@ -629,7 +629,7 @@ static int adma_ata_init_one(struct pci_
 	}
 
 	/* acquire generic ATA PCI resources */
-	rc = ata_pci_acquire_resources(host, DMA_32BIT_MASK, &reason);
+	rc = ata_pci_acquire_resources(host, DMA_32BIT_MASK, 0, &reason);
 	if (rc)
 		goto err;
 
drivers/ata/sata_mv.c +1 -1
index 865cfd8..ea09e7e 100644
@@ -2327,7 +2327,7 @@ static int mv_init_one(struct pci_dev *p
 	}
 
 	/* acquire generic ATA PCI resources */
-	rc = ata_pci_acquire_resources(host, DMA_64BIT_MASK, &reason);
+	rc = ata_pci_acquire_resources(host, DMA_64BIT_MASK, 0, &reason);
 	if (rc)
 		goto err;
 
drivers/ata/sata_nv.c +1 -1
index 00894a0..f9b3602 100644
@@ -507,7 +507,7 @@ static int nv_init_one (struct pci_dev *
 	}
 
 	/* acquire generic ATA PCI resources */
-	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, &reason);
+	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, 0, &reason);
 	if (rc)
 		goto err;
 
drivers/ata/sata_promise.c +1 -1
index 6e33abb..e384d80 100644
@@ -713,7 +713,7 @@ static int pdc_ata_init_one(struct pci_d
 	hp = host->private_data;
 
 	/* acquire generic ATA PCI resources */
-	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, &reason);
+	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, 0, &reason);
 	if (rc)
 		goto err;
 
drivers/ata/sata_qstor.c +1 -1
index 420eb58..dc8d88c 100644
@@ -617,7 +617,7 @@ static int qs_ata_init_one(struct pci_de
 	}
 
 	/* acquire generic ATA PCI resources */
-	rc = ata_pci_acquire_resources(host, 0, &reason);
+	rc = ata_pci_acquire_resources(host, 0, 0, &reason);
 	if (rc)
 		goto err;
 
drivers/ata/sata_sil.c +1 -1
index 9cabbb5..dec3c91 100644
@@ -626,7 +626,7 @@ static int sil_init_one (struct pci_dev 
 	}
 
 	/* acquire generic ATA PCI resources */
-	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, &reason);
+	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, 0, &reason);
 	if (rc)
 		goto err;
 
drivers/ata/sata_sil24.c +1 -1
index 9f1ad0f..269dee0 100644
@@ -1069,7 +1069,7 @@ static int sil24_init_one(struct pci_dev
 	hpriv = host->private_data;
 
 	/* acquire generic ATA PCI resources */
-	rc = ata_pci_acquire_resources(host, DMA_64BIT_MASK, &reason);
+	rc = ata_pci_acquire_resources(host, DMA_64BIT_MASK, 0, &reason);
 	if (rc)
 		goto err;
 
drivers/ata/sata_sis.c +1 -1
index a696693..0869501 100644
@@ -255,7 +255,7 @@ static int sis_init_one (struct pci_dev 
 	}
 
 	/* acquire generic ATA PCI resources and init PCI host */
-	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, &reason);
+	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, 0, &reason);
 	if (rc)
 		goto err;
 
drivers/ata/sata_svw.c +1 -1
index d51e860..e16d1c7 100644
@@ -394,7 +394,7 @@ static int k2_sata_init_one (struct pci_
 	}
 
 	/* acquire generic ATA PCI resources */
-	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, &reason);
+	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, 0, &reason);
 	if (rc)
 		goto err;
 
drivers/ata/sata_sx4.c +1 -1
index 4a235ab..3a0257d 100644
@@ -1376,7 +1376,7 @@ static int pdc_sata_init_one(struct pci_
 	hpriv = host->private_data;
 
 	/* acquire generic ATA PCI resources */
-	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, &reason);
+	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, 0, &reason);
 	if (rc)
 		goto err;
 
drivers/ata/sata_uli.c +1 -1
index 5ffdc2b..3a9723e 100644
@@ -204,7 +204,7 @@ static int uli_init_one(struct pci_dev *
 	hpriv = host->private_data;
 
 	/* acquire generic ATA PCI resources */
-	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, &reason);
+	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, 0, &reason);
 	if (rc)
 		goto err;
 
drivers/ata/sata_via.c +1 -1
index f0bc20e..2a7513d 100644
@@ -262,7 +262,7 @@ static int svia_init_one(struct pci_dev 
 	}
 
 	/* acquire generic ATA PCI resources */
-	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, &reason);
+	rc = ata_pci_acquire_resources(host, ATA_DMA_MASK, 0, &reason);
 	if (rc)
 		goto err;
 
drivers/ata/sata_vsc.c +1 -1
index 71ea8d9..153403f 100644
@@ -368,7 +368,7 @@ static int __devinit vsc_sata_init_one (
 	/* Acquire generic ATA PCI resources.  Use 32 bit DMA mask,
 	 * because 64 bit address support is poor.
 	 */
-	rc = ata_pci_acquire_resources(host, DMA_32BIT_MASK, &reason);
+	rc = ata_pci_acquire_resources(host, DMA_32BIT_MASK, 0, &reason);
 	if (rc)
 		goto err;
 
include/linux/libata.h +4 -0
index f72cc73..ed02ae9 100644
@@ -407,6 +407,9 @@ struct ata_host {
 	unsigned int		legacy_flags;
 	unsigned int		pci_flags;
 
+	void __iomem		*legacy_iomap[2][2];
+	void __iomem		*iomap[DEVICE_COUNT_RESOURCE];
+
 	struct list_head	irq_list;	/* list of acquired irqs */
 	struct ata_port		*ports[0];
 };
@@ -714,6 +717,7 @@ extern unsigned int ata_pci_legacy_mask(
 extern int ata_pci_set_dma_mask(struct pci_dev *pdev, u64 dma_mask,
 				const char **p_reason);
 extern int ata_pci_acquire_resources(struct ata_host *host, u64 dma_mask,
+				     unsigned int bar_mask,
 				     const char **p_reason);
 extern void ata_pci_release_resources(struct ata_host *host);
 extern void ata_pci_init_ports(struct ata_host *host);
Signature · 1 line
1.4.1.1
#39 lore raw

Re: [PATCH 17/20] libata: make ata_pci_acquire_resources() handle iomap

Quoted 32 lines · Tejun Heo
drivers/ata/libata-pci.c +22 -0
@@ -181,6 +183,28 @@ int ata_pci_acquire_resources(struct ata
 	if (rc)
 		goto err;
 
+	/* iomap PCI BARs */
+	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+		if (!(bar_mask & (1 << i)))
+			continue;
+
+		WARN_ON(iomap[i]);
+
+		if (!pci_resource_start(pdev, i) ||
+		    !pci_resource_len(pdev, i)) {
+			reason = "iomap requested for null PCI BAR";
+			rc = -EIO;
+			goto err;
+		}
+
+		iomap[i] = pci_iomap(pdev, i, 0);
+		if (!iomap[i]) {
+			rc = -ENOMEM;
+			reason = "failed to iomap PCI BAR";
+			goto err;
+		}
+	}
+
 	return 0;
 
  err:
@@ -202,6 +226,14 @@ int ata_pci_acquire_resources(struct ata
 void ata_pci_release_resources(struct ata_host *host)
 {

drivers/ata/libata-pci.c int ata_pci_acquire_resources(struct ata · line 237

Quoted 10 lines
drivers/ata/libata-pci.c +8 -0
 	struct pci_dev *pdev = to_pci_dev(host->dev);
+	void __iomem **iomap = host->iomap;
+	int i;
+
+	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
+		if (iomap[i]) {
+			pci_iounmap(pdev, iomap[i]);
+			iomap[i] = NULL;
+		}
 

NAK, you only should map the stuff you are going to use.

Mapped resources is finite, and you definitely don't want to map certain resources on certain controllers -- sometimes they are a memory area that's too large to be mapped, even.

author
#40 lore raw

Re: [PATCH 17/20] libata: make ata_pci_acquire_resources() handle iomap

Quoted 8 lines · Jeff Garzik

drivers/ata/libata-pci.c int ata_pci_acquire_resources(struct ata · line 189

Quoted 4 lines
drivers/ata/libata-pci.c +4 -0
+    /* iomap PCI BARs */
+    for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+        if (!(bar_mask & (1 << i)))
+            continue;

NAK, you only should map the stuff you are going to use.

Mapped resources is finite, and you definitely don't want to map certain resources on certain controllers -- sometimes they are a memory area that's too large to be mapped, even.

Yeap, that's what @bar_mask and the above if () continue; block is for.
Or am I misunderstanding you?

Thanks.

Signature · 1 line
tejun
#41 lore raw

Re: [PATCH 17/20] libata: make ata_pci_acquire_resources() handle iomap

Quoted 6 lines · Tejun Heo
Quoted 5 lines · Jeff Garzik
Quoted 4 lines
drivers/ata/libata-pci.c +4 -0
+    /* iomap PCI BARs */
+    for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+        if (!(bar_mask & (1 << i)))
+            continue;
Quoted 6 lines
Quoted 3 lines

NAK, you only should map the stuff you are going to use.

Mapped resources is finite, and you definitely don't want to map certain resources on certain controllers -- sometimes they are a memory area that's too large to be mapped, even.

Yeap, that's what @bar_mask and the above if () continue; block is for.
Or am I misunderstanding you?

Oops, I missed that part. Ignore my comment...

author
#42 lore raw

PATCH 19/20 libata: kill unused ATA_FLAG_MMIO

In reply to Tejun Heo

Kill unused ATA_FLAG_MMIO.

---
 drivers/ata/ahci.c         |    6 ++----
 drivers/ata/pdc_adma.c     |    3 +--
 drivers/ata/sata_mv.c      |    4 ++--
 drivers/ata/sata_promise.c |    3 +--
 drivers/ata/sata_qstor.c   |    2 +-
 drivers/ata/sata_sil.c     |    2 +-
 drivers/ata/sata_sil24.c   |    4 ++--
 drivers/ata/sata_svw.c     |    3 +--
 drivers/ata/sata_sx4.c     |    4 ++--
 drivers/ata/sata_vsc.c     |    3 +--
 include/linux/libata.h     |    1 -
 11 files changed, 14 insertions(+), 21 deletions(-)
drivers/ata/ahci.c +2 -4
index 2ecb87c..b78878b 100644
@@ -271,8 +271,7 @@ static const struct ata_port_info ahci_p
 	{
 		.sht		= &ahci_sht,
 		.flags		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-				  ATA_FLAG_SKIP_D2H_BSY,
+				  ATA_FLAG_PIO_DMA | ATA_FLAG_SKIP_D2H_BSY,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,
@@ -281,8 +280,7 @@ static const struct ata_port_info ahci_p
 	{
 		.sht		= &ahci_sht,
 		.flags		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-				  ATA_FLAG_SKIP_D2H_BSY |
+				  ATA_FLAG_PIO_DMA | ATA_FLAG_SKIP_D2H_BSY |
 				  AHCI_FLAG_RESET_NEEDS_CLO | AHCI_FLAG_NO_NCQ,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
drivers/ata/pdc_adma.c +1 -2
index 503f66a..7814261 100644
@@ -187,8 +187,7 @@ static struct ata_port_info adma_port_in
 	{
 		.sht		= &adma_ata_sht,
 		.flags		= ATA_FLAG_SLAVE_POSS | ATA_FLAG_SRST |
-				  ATA_FLAG_NO_LEGACY | ATA_FLAG_MMIO |
-				  ATA_FLAG_PIO_POLLING,
+				  ATA_FLAG_NO_LEGACY | ATA_FLAG_PIO_POLLING,
 		.pio_mask	= 0x10, /* pio4 */
 		.udma_mask	= 0x1f, /* udma0-4 */
 		.port_ops	= &adma_ata_ops,
drivers/ata/sata_mv.c +2 -2
index 3a9aa5e..33988bd 100644
@@ -92,8 +92,8 @@ enum {
 	MV_FLAG_DUAL_HC		= (1 << 30),  /* two SATA Host Controllers */
 	MV_FLAG_IRQ_COALESCE	= (1 << 29),  /* IRQ coalescing capability */
 	MV_COMMON_FLAGS		= (ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				   ATA_FLAG_SATA_RESET | ATA_FLAG_MMIO |
-				   ATA_FLAG_NO_ATAPI | ATA_FLAG_PIO_POLLING),
+				   ATA_FLAG_SATA_RESET | ATA_FLAG_NO_ATAPI |
+				   ATA_FLAG_PIO_POLLING),
 	MV_6XXX_FLAGS		= MV_FLAG_IRQ_COALESCE,
 
 	CRQB_FLAG_READ		= (1 << 0),
drivers/ata/sata_promise.c +1 -2
index f3ffb05..112418a 100644
@@ -83,8 +83,7 @@ enum {
 	PDC_HAS_PATA		= (1 << 1), /* PDC20375/20575 has PATA */
 
 	PDC_COMMON_FLAGS	= ATA_FLAG_NO_LEGACY | ATA_FLAG_SRST |
-				  ATA_FLAG_MMIO | ATA_FLAG_NO_ATAPI |
-				  ATA_FLAG_PIO_POLLING,
+				  ATA_FLAG_NO_ATAPI | ATA_FLAG_PIO_POLLING,
 };
 
 
drivers/ata/sata_qstor.c +1 -1
index edcfc75..6f94988 100644
@@ -176,7 +176,7 @@ static const struct ata_port_info qs_por
 		.flags		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 				  ATA_FLAG_SATA_RESET |
 				  //FIXME ATA_FLAG_SRST |
-				  ATA_FLAG_MMIO | ATA_FLAG_PIO_POLLING,
+				  ATA_FLAG_PIO_POLLING,
 		.pio_mask	= 0x10, /* pio4 */
 		.udma_mask	= 0x7f, /* udma0-6 */
 		.port_ops	= &qs_ata_ops,
drivers/ata/sata_sil.c +1 -1
index 90a6caa..d397cf5 100644
@@ -59,7 +59,7 @@ enum {
 	SIL_FLAG_MOD15WRITE	= (1 << 30),
 
 	SIL_DFL_PORT_FLAGS	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_MMIO | ATA_FLAG_HRST_TO_RESUME,
+				  ATA_FLAG_HRST_TO_RESUME,
 
 	/*
 	 * Controller IDs
drivers/ata/sata_sil24.c +2 -2
index 9a7ad16..e6b23ae 100644
@@ -232,8 +232,8 @@ enum {
 
 	/* host flags */
 	SIL24_COMMON_FLAGS	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
-				  ATA_FLAG_NCQ | ATA_FLAG_SKIP_D2H_BSY,
+				  ATA_FLAG_PIO_DMA | ATA_FLAG_NCQ |
+				  ATA_FLAG_SKIP_D2H_BSY,
 	SIL24_FLAG_PCIX_IRQ_WOC	= (1 << 24), /* IRQ loss errata on PCI-X */
 
 	IRQ_STAT_4PORTS		= 0xf,
drivers/ata/sata_svw.c +1 -2
index 3b7c1de..2cedc80 100644
@@ -352,8 +352,7 @@ static void k2_sata_setup_port(struct at
 
 static const struct ata_port_info k2_sata_port_info = {
 	.sht			= &k2_sata_sht,
-	.flags			= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_MMIO,
+	.flags			= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY,
 	/* We don't care much about the PIO/UDMA masks, but the core
 	 * won't like us if we don't fill these
 	 */
drivers/ata/sata_sx4.c +2 -2
index 6054915..c7324a8 100644
@@ -217,8 +217,8 @@ static const struct ata_port_info pdc_po
 	{
 		.sht		= &pdc_sata_sht,
 		.flags		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
-				  ATA_FLAG_NO_ATAPI | ATA_FLAG_PIO_POLLING,
+				  ATA_FLAG_SRST | ATA_FLAG_NO_ATAPI |
+				  ATA_FLAG_PIO_POLLING,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
drivers/ata/sata_vsc.c +1 -2
index eb2887f..4add2ad 100644
@@ -314,8 +314,7 @@ static const struct ata_port_operations 
 
 static const struct ata_port_info vsc_sata_port_info = {
 	.sht		= &vsc_sata_sht,
-	.flags		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
-			  ATA_FLAG_MMIO,
+	.flags		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY,
 	/* We don't care much about the PIO/UDMA masks, but the core
 	 * won't like us if we don't fill these.
 	 */
include/linux/libata.h +0 -1
index 7efd84b..0341495 100644
@@ -149,7 +149,6 @@ enum {
 					    /* (doesn't imply presence) */
 	ATA_FLAG_SATA		= (1 << 1),
 	ATA_FLAG_NO_LEGACY	= (1 << 2), /* no legacy mode check */
-	ATA_FLAG_MMIO		= (1 << 3), /* use MMIO, not PIO */
 	ATA_FLAG_SRST		= (1 << 4), /* (obsolete) use ATA SRST, not E.D.D. */
 	ATA_FLAG_SATA_RESET	= (1 << 5), /* (obsolete) use COMRESET */
 	ATA_FLAG_NO_ATAPI	= (1 << 6), /* No ATAPI support */
Signature · 1 line
1.4.1.1
author
#43 lore raw

PATCH 20/20 libata: move scattered PCI ATA functions into liata-pci.c

In reply to Tejun Heo

Move remaining PCI ATA functions in libata-bmdma.c and libata-core.c into libata-pci.c

---
 drivers/ata/libata-bmdma.c |   40 ---------------
 drivers/ata/libata-core.c  |   89 ---------------------------------
 drivers/ata/libata-pci.c   |  117 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 117 insertions(+), 129 deletions(-)
drivers/ata/libata-bmdma.c +0 -40
index b8b393f..66f935a 100644
@@ -444,43 +444,3 @@ void ata_bmdma_post_internal_cmd(struct 
 {
 	ata_bmdma_stop(qc);
 }
-
-#ifdef CONFIG_PCI
-/**
- *	ata_pci_clear_simplex	-	attempt to kick device out of simplex
- *	@pdev: PCI device
- *
- *	Some PCI ATA devices report simplex mode but in fact can be told to
- *	enter non simplex mode. This implements the neccessary logic to
- *	perform the task on such devices. Calling it on other devices will
- *	have -undefined- behaviour.
- */
-
-int ata_pci_clear_simplex(struct pci_dev *pdev)
-{
-	unsigned long bmdma = pci_resource_start(pdev, 4);
-	u8 simplex;
-
-	if (bmdma == 0)
-		return -ENOENT;
-
-	simplex = inb(bmdma + 0x02);
-	outb(simplex & 0x60, bmdma + 0x02);
-	simplex = inb(bmdma + 0x02);
-	if (simplex & 0x80)
-		return -EOPNOTSUPP;
-	return 0;
-}
-
-unsigned long ata_pci_default_filter(const struct ata_port *ap, struct ata_device *adev, unsigned long xfer_mask)
-{
-	/* Filter out DMA modes if the device has been configured by
-	   the BIOS as PIO only */
-
-	if (ap->ioaddr.bmdma_addr == 0)
-		xfer_mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);
-	return xfer_mask;
-}
-
-#endif /* CONFIG_PCI */
-
drivers/ata/libata-core.c +0 -89

101 lines not on this page · raw message

drivers/ata/libata-pci.c +117 -0

132 lines not on this page · raw message

Signature · 1 line
1.4.1.1
#44 lore raw

Re: [PATCH 20/20] libata: move scattered PCI ATA functions into liata-pci.c

the rest of the patches seem mostly sane. more comments on next iteration of patchset...

#45 lore raw
In reply to Tejun Heo
Quoted 5 lines · Tejun Heo

Brian, can you please take a look at how LLDs use new init functions and see if SAS can use the same approach? If you pass NULL as @sht to ata_host_alloc(), it won't associate ports as part of Scsi_Host just as ata_sas_port_alloc() does and ata_host_free() will do the right thing when freeing a host allocated that way.

I started looking through your patch set and have a few comments.

The idea of a static number of "ata ports" per ata host in SAS doesn't really work. Since you can have ATA devices under a SAS expander, the number of possible ATA devices that can be attached to a SAS adapter can be rather large and can change depending on the SAS fabric. If libata ever needed to iterate over the ata_port's for a SAS ata_host, then we would probably need to convert the static array of ata_ports to a linked list, allowing it to be more dynamic.

Object lifetime rules also have me concerned. Currently, for SAS, there are a couple objects that libata is concerned with. The first is an ata_host_set, which I am allocating as part of the scsi_host struct, so it inherits the object lifetime rules of that. The second is the ata_port, which I allocate and free in target_alloc/target_destroy, so I get refcounting for free there as well. Your patch set introduces an ata_host struct, which is kmalloc'ed and doesn't inherit any of the above refcounting.

Quoted 7 lines

sata_sil24.c is a pretty straight-forward example. If you can't determine the number of ports when allocating host, please take a look at how ahci.c initializes its host.

The intention was to allow SAS to use all the regular init/deinit functions just as other LLDs. If something doesn't seem to be right, please let me know.

I think it can use bits of it, but I think the actual device discovery process is better initiated by the SAS layer. The SAS layer knows what devices are out there when it does discovery and can tell libata about them.

I'm still looking through your patch set, but wanted to get a few comments out...

Brian

Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center
author
#46 lore raw

Hello,

Quoted 9 lines · Brian King
Quoted 5 lines · Tejun Heo

Brian, can you please take a look at how LLDs use new init functions and see if SAS can use the same approach? If you pass NULL as @sht to ata_host_alloc(), it won't associate ports as part of Scsi_Host just as ata_sas_port_alloc() does and ata_host_free() will do the right thing when freeing a host allocated that way.

I started looking through your patch set and have a few comments.

Quoted 7 lines

The idea of a static number of "ata ports" per ata host in SAS doesn't really work. Since you can have ATA devices under a SAS expander, the number of possible ATA devices that can be attached to a SAS adapter can be rather large and can change depending on the SAS fabric. If libata ever needed to iterate over the ata_port's for a SAS ata_host, then we would probably need to convert the static array of ata_ports to a linked list, allowing it to be more dynamic.

Making the array dynamically-sized isn't difficult at all; however, the current libata code assumes that ata_host->ports[] array is packed - ie. no intervening empty entry. Can SAS keep this restriction or does it need more flexibility?

Quoted 8 lines

Object lifetime rules also have me concerned. Currently, for SAS, there are a couple objects that libata is concerned with. The first is an ata_host_set, which I am allocating as part of the scsi_host struct, so it inherits the object lifetime rules of that. The second is the ata_port, which I allocate and free in target_alloc/target_destroy, so I get refcounting for free there as well. Your patch set introduces an ata_host struct, which is kmalloc'ed and doesn't inherit any of the above refcounting.

Actually, ata_host is ata_host_set. It's just renamed recently.

cca3974e48607c3775dc73b544a5700b2e37c21a in libata-dev#upstream

Hmmm.... I was kind of hoping SAS could use ata_host_alloc() and store its pointer and then later release it w/ ata_host_free(), hmmm.. maybe you can call ata_host_free from ->slave_destroy?. That gives libata more control over the host structure (e.g. if we implement dynamic ports array, it needs to be freed too). Port lifetime rules aren't changed by these updates and host free does need some changes but IMHO that shouldn't be difficult.

Quoted 4 lines
Quoted 4 lines

sata_sil24.c is a pretty straight-forward example. If you can't determine the number of ports when allocating host, please take a look at how ahci.c initializes its host.

Quoted 8 lines
Quoted 3 lines

The intention was to allow SAS to use all the regular init/deinit functions just as other LLDs. If something doesn't seem to be right, please let me know.

I think it can use bits of it, but I think the actual device discovery process is better initiated by the SAS layer. The SAS layer knows what devices are out there when it does discovery and can tell libata about them.

Hmmm.... I see. Something like ata_dev_attach(adev) after initialized by SAS maybe?

Signature · 1 line
tejun
#47 lore raw
Quoted 9 lines · Tejun Heo
Quoted 7 lines · Brian King

The idea of a static number of "ata ports" per ata host in SAS doesn't really work. Since you can have ATA devices under a SAS expander, the number of possible ATA devices that can be attached to a SAS adapter can be rather large and can change depending on the SAS fabric. If libata ever needed to iterate over the ata_port's for a SAS ata_host, then we would probably need to convert the static array of ata_ports to a linked list, allowing it to be more dynamic.

Quoted 4 lines

Making the array dynamically-sized isn't difficult at all; however, the current libata code assumes that ata_host->ports[] array is packed - ie. no intervening empty entry. Can SAS keep this restriction or does it need more flexibility?

This could be made to work with the addition of a new API. Rather than having just ata_sas_port_destroy, we would need ata_sas_port_delete and ata_sas_port_free. ata_sas_port_delete would remove the port from the ata_host and do everything except free the ata port since there could still be references to it. Then ata_sas_port_free would get called once all the references were deleted.

Quoted 10 lines
Quoted 8 lines

Object lifetime rules also have me concerned. Currently, for SAS, there are a couple objects that libata is concerned with. The first is an ata_host_set, which I am allocating as part of the scsi_host struct, so it inherits the object lifetime rules of that. The second is the ata_port, which I allocate and free in target_alloc/target_destroy, so I get refcounting for free there as well. Your patch set introduces an ata_host struct, which is kmalloc'ed and doesn't inherit any of the above refcounting.

Actually, ata_host is ata_host_set. It's just renamed recently.

Right. I saw that.

Quoted 9 lines

cca3974e48607c3775dc73b544a5700b2e37c21a in libata-dev#upstream

Hmmm.... I was kind of hoping SAS could use ata_host_alloc() and store its pointer and then later release it w/ ata_host_free(), hmmm.. maybe you can call ata_host_free from ->slave_destroy?. That gives libata more control over the host structure (e.g. if we implement dynamic ports array, it needs to be freed too). Port lifetime rules aren't changed by these updates and host free does need some changes but IMHO that shouldn't be difficult.

The current design for SAS is to have a single ata_host per scsi host. This means the ata_host is really tied to the object lifetime rules of the scsi host. In the current SAS code, ata_host_set does not get allocated as a separate piece of memory, but rather as part of the scsi_host object. This means the memory for the ata_host doesn't get freed until the last reference to the scsi host is released. Making ata_host a separate allocation changes these rules and forces the caller to know when to free the ata_host memory, which they currently do not know.

In order to do what you are proposing, I think we would need to add some refcounting to the ata_host object. Each allocated ata_port would get a reference to its parent ata_host and would put a reference when the port is freed. Otherwise I am concerned that we would end up in the situation where the ata_host is freed before all the ata ports and the scsi_host is freed. It might be appropriate to create an ata_host class device and an ata_port class device...

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

sata_sil24.c is a pretty straight-forward example. If you can't determine the number of ports when allocating host, please take a look at how ahci.c initializes its host.

The intention was to allow SAS to use all the regular init/deinit functions just as other LLDs. If something doesn't seem to be right, please let me know.

Quoted 7 lines
Quoted 4 lines

I think it can use bits of it, but I think the actual device discovery process is better initiated by the SAS layer. The SAS layer knows what devices are out there when it does discovery and can tell libata about them.

Hmmm.... I see. Something like ata_dev_attach(adev) after initialized by SAS maybe?

Possibly. Are you proposing that ata_dev_attach would then end up calling scsi_add_device after doing the ATA initialization stuff?

Brian

Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center
author
#48 lore raw

Hello, Brian.

Quoted 8 lines · Brian King
Quoted 1 line

Making the array dynamically-sized isn't difficult at all; however, the current libata code assumes that ata_host->ports[] array is packed - ie. no intervening empty entry. Can SAS keep this restriction or does it need more flexibility?

This could be made to work with the addition of a new API. Rather than having just ata_sas_port_destroy, we would need ata_sas_port_delete and ata_sas_port_free. ata_sas_port_delete would remove the port from the ata_host and do everything except free the ata port since there could still be references to it. Then ata_sas_port_free would get called once all the references were deleted.

I see.

Quoted 10 lines
Quoted 1 line

Hmmm.... I was kind of hoping SAS could use ata_host_alloc() and store its pointer and then later release it w/ ata_host_free(), hmmm.. maybe you can call ata_host_free from ->slave_destroy?. That gives libata more control over the host structure (e.g. if we implement dynamic ports array, it needs to be freed too). Port lifetime rules aren't changed by these updates and host free does need some changes but IMHO that shouldn't be difficult.

The current design for SAS is to have a single ata_host per scsi host. This means the ata_host is really tied to the object lifetime rules of the scsi host. In the current SAS code, ata_host_set does not get allocated as a separate piece of memory, but rather as part of the scsi_host object. This means the memory for the ata_host doesn't get freed until the last reference to the scsi host is released. Making ata_host a separate allocation changes these rules and forces the caller to know when to free the ata_host memory, which they currently do not know.

You're right. I was confused that ->slave_destroy() is called on host release. SCSI doesn't have host release callback. What do you think about adding a host release callback? That should make things easier and it is generally useful.

Quoted 7 lines

In order to do what you are proposing, I think we would need to add some refcounting to the ata_host object. Each allocated ata_port would get a reference to its parent ata_host and would put a reference when the port is freed. Otherwise I am concerned that we would end up in the situation where the ata_host is freed before all the ata ports and the scsi_host is freed. It might be appropriate to create an ata_host class device and an ata_port class device...

Or we can just keep ata_host_init() and let sas allocate ata_host as part of SCSI host and free all dynamic stuff when all ports are detached. It's a bit hacky but should work for the time being.

Quoted 4 lines
Quoted 1 line

Hmmm.... I see. Something like ata_dev_attach(adev) after initialized by SAS maybe?

Possibly. Are you proposing that ata_dev_attach would then end up calling scsi_add_device after doing the ATA initialization stuff?

For SAS, libata isn't controlling SCSI host, so I think it's more logical to leave SCSI devices to SAS too. Would that make much difference?

Thanks.

Signature · 1 line
tejun
#49 lore raw
Quoted 4 lines · Tejun Heo

You're right. I was confused that ->slave_destroy() is called on host release. SCSI doesn't have host release callback. What do you think about adding a host release callback? That should make things easier and it is generally useful.

Sounds reasonable.

Quoted 11 lines
Quoted 7 lines

In order to do what you are proposing, I think we would need to add some refcounting to the ata_host object. Each allocated ata_port would get a reference to its parent ata_host and would put a reference when the port is freed. Otherwise I am concerned that we would end up in the situation where the ata_host is freed before all the ata ports and the scsi_host is freed. It might be appropriate to create an ata_host class device and an ata_port class device...

Or we can just keep ata_host_init() and let sas allocate ata_host as part of SCSI host and free all dynamic stuff when all ports are detached. It's a bit hacky but should work for the time being.

I think that's a fine approach for now.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Hmmm.... I see. Something like ata_dev_attach(adev) after initialized by SAS maybe?

Possibly. Are you proposing that ata_dev_attach would then end up calling scsi_add_device after doing the ATA initialization stuff?

For SAS, libata isn't controlling SCSI host, so I think it's more logical to leave SCSI devices to SAS too.

I agree.

Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center