[PATCH] libata: add host_set->next for legacy two host sets case

[PATCH] libata: add host_set->next for legacy two host sets case

[Tejun Heo]

For a legacy ATA controller, libata registers two separate host sets.
There was no connection between the two hosts making it impossible to
traverse all ports related to the controller.  This patch adds
host_set->next which points to the second host_set and implements
ata_host_set_for_all_ports() which traverses all ports associated with
the controller.  This fixes the following bugs.

* On device removal, all ports hanging off the device are properly
  detached.  Prior to this patch, ports on the first host_set weren't
  detached casuing oops on driver unloading.

* On device removal, both host sets are freed

This will also be used by new power management code to suspend and
resume all ports of a controller.  host_set/port representation will
be improved to handle legacy controllers better and this host_set
linking hack will go away with it.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-bmdma.c |   15 +++++++++++++--
 drivers/scsi/libata-core.c  |   39 +++++++++++++++++++++++++++++++++------
 include/linux/libata.h      |    3 ++-
 3 files changed, 48 insertions(+), 9 deletions(-)

b1d1062e1b5b1de896b687118ed9a1926fd6f3b0
diff --git a/drivers/scsi/libata-bmdma.c b/drivers/scsi/libata-bmdma.c
index 4bc0537..13fab97 100644
--- a/drivers/scsi/libata-bmdma.c
+++ b/drivers/scsi/libata-bmdma.c
@@ -1076,10 +1076,21 @@ int ata_pci_init_one (struct pci_dev *pd
 
 	/* FIXME: check ata_device_add return */
 	if (legacy_mode) {
-		if (legacy_mode & (1 << 0))
+		struct device *dev = &pdev->dev;
+		struct ata_host_set *host_set = NULL;
+
+		if (legacy_mode & (1 << 0)) {
 			ata_device_add(probe_ent);
-		if (legacy_mode & (1 << 1))
+			host_set = dev_get_drvdata(dev);
+		}
+
+		if (legacy_mode & (1 << 1)) {
 			ata_device_add(probe_ent2);
+			if (host_set) {
+				host_set->next = dev_get_drvdata(dev);
+				dev_set_drvdata(dev, host_set);
+			}
+		}
 	} else
 		ata_device_add(probe_ent);
 
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 014855e..357f253 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -94,6 +94,32 @@ MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
 
+static struct ata_port *__ata_host_set_next_port(struct ata_port *ap,
+						 int *p_idx,
+						 struct ata_host_set *host_set)
+{
+	int idx = ++(*p_idx);
+
+	if (idx >= host_set->n_ports && host_set->next) {
+		idx -= host_set->n_ports;
+		host_set = host_set->next;
+	}
+
+	if (idx < host_set->n_ports)
+		return host_set->ports[idx];
+
+	return NULL;
+}
+
+/* No, it's not for_each_port.  The following macro traverses not only
+ * host_set->ports but also host_set->next->ports.  Don't use it
+ * unless you know what you're doing.  It will go away once legacy
+ * host handling is improved.
+ */
+#define ata_host_set_for_all_ports(ap, idx, host_set) \
+	for (ap = host_set->ports[0], idx = 0; ap; \
+	     ap = __ata_host_set_next_port(ap, &idx, host_set))
+
 /**
  *	ata_tf_to_fis - Convert ATA taskfile to SATA FIS structure
  *	@tf: Taskfile to convert
@@ -5510,16 +5536,15 @@ void ata_port_detach(struct ata_port *ap
 
 void ata_host_set_remove(struct ata_host_set *host_set)
 {
-	unsigned int i;
+	struct ata_port *ap;
+	int i;
 
-	for (i = 0; i < host_set->n_ports; i++)
-		ata_port_detach(host_set->ports[i]);
+	ata_host_set_for_all_ports(ap, i, host_set)
+		ata_port_detach(ap);
 
 	free_irq(host_set->irq, host_set);
 
-	for (i = 0; i < host_set->n_ports; i++) {
-		struct ata_port *ap = host_set->ports[i];
-
+	ata_host_set_for_all_ports(ap, i, host_set) {
 		ata_scsi_release(ap->host);
 
 		if ((ap->flags & ATA_FLAG_NO_LEGACY) == 0) {
@@ -5537,6 +5562,8 @@ void ata_host_set_remove(struct ata_host
 	if (host_set->ops->host_stop)
 		host_set->ops->host_stop(host_set);
 
+	if (host_set->next)
+		kfree(host_set->next);
 	kfree(host_set);
 }
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 61eea57..f03b866 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -356,7 +356,8 @@ struct ata_host_set {
 	unsigned long		flags;
 	int			simplex_claimed;	/* Keep seperate in case we
 							   ever need to do this locked */
-	struct ata_port *	ports[0];
+	struct ata_host_set	*next;		/* for legacy mode */
+	struct ata_port		*ports[0];
 };
 
 struct ata_queued_cmd {
-- 
1.3.2

Re: [PATCH] libata: add host_set->next for legacy two host sets case

[Jeff Garzik]

Tejun Heo wrote:
> For a legacy ATA controller, libata registers two separate host sets.
> There was no connection between the two hosts making it impossible to
> traverse all ports related to the controller.  This patch adds
> host_set->next which points to the second host_set and implements
> ata_host_set_for_all_ports() which traverses all ports associated with
> the controller.  This fixes the following bugs.
> 
> * On device removal, all ports hanging off the device are properly
>   detached.  Prior to this patch, ports on the first host_set weren't
>   detached casuing oops on driver unloading.
> 
> * On device removal, both host sets are freed
> 
> This will also be used by new power management code to suspend and
> resume all ports of a controller.  host_set/port representation will
> be improved to handle legacy controllers better and this host_set
> linking hack will go away with it.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

NAK.  You don't want to iterate through multiple host sets, _inside_ of 
function ata_host_set_remove().  ata_host_set_remove() is called with a 
single host_set, and is designed to remove only a single host set.

The concept of a host_set list is OK, but I would rather that 
ata_pci_remove_one() be updated to something like

	host_set = get-drvdata(...)

	while (host_set)
		tmp = host_set
		host_set = host_set->next
		ata_host_set_remove(tmp)

Re: [PATCH] libata: add host_set->next for legacy two host sets case

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> For a legacy ATA controller, libata registers two separate host sets.
>> There was no connection between the two hosts making it impossible to
>> traverse all ports related to the controller.  This patch adds
>> host_set->next which points to the second host_set and implements
>> ata_host_set_for_all_ports() which traverses all ports associated with
>> the controller.  This fixes the following bugs.
>>
>> * On device removal, all ports hanging off the device are properly
>>   detached.  Prior to this patch, ports on the first host_set weren't
>>   detached casuing oops on driver unloading.
>>
>> * On device removal, both host sets are freed
>>
>> This will also be used by new power management code to suspend and
>> resume all ports of a controller.  host_set/port representation will
>> be improved to handle legacy controllers better and this host_set
>> linking hack will go away with it.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> NAK.  You don't want to iterate through multiple host sets, _inside_ of 
> function ata_host_set_remove().  ata_host_set_remove() is called with a 
> single host_set, and is designed to remove only a single host set.
> 
> The concept of a host_set list is OK, but I would rather that 
> ata_pci_remove_one() be updated to something like
> 
>     host_set = get-drvdata(...)
> 
>     while (host_set)
>         tmp = host_set
>         host_set = host_set->next
>         ata_host_set_remove(tmp)

I wanted to hide the second host_set behind the first one as there will 
be only one host_set in the future.  But, okay, will update as you said. 
  That should be less confusing.

-- 
tejun

[PATCH] libata: add host_set->next for legacy two host_sets case, take #2

[Tejun Heo]

For a legacy ATA controller, libata registers two separate host sets.
There was no connection between the two hosts making it impossible to
traverse all ports related to the controller.  This patch adds
host_set->next which points to the second host_set and makes
ata_host_set_remove() remove all associated host_sets and ports.

* On device removal, all ports hanging off the device are properly
  detached.  Prior to this patch, ports on the first host_set weren't
  detached casuing oops on driver unloading.

* On device removal, both host_sets are properly freed

This will also be used by new power management code to suspend and
resume all ports of a controller.  host_set/port representation will
be improved to handle legacy controllers better and this host_set
linking will go away with it.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-bmdma.c |   15 +++++++++++++--
 drivers/scsi/libata-core.c  |   44 +++++++++++++++++++++++--------------------
 include/linux/libata.h      |    3 ++-
 3 files changed, 39 insertions(+), 23 deletions(-)

b53ff57b968b2b6f07ae091d0d7d29f1e422fb3a
diff --git a/drivers/scsi/libata-bmdma.c b/drivers/scsi/libata-bmdma.c
index 4bc0537..13fab97 100644
--- a/drivers/scsi/libata-bmdma.c
+++ b/drivers/scsi/libata-bmdma.c
@@ -1076,10 +1076,21 @@ int ata_pci_init_one (struct pci_dev *pd
 
 	/* FIXME: check ata_device_add return */
 	if (legacy_mode) {
-		if (legacy_mode & (1 << 0))
+		struct device *dev = &pdev->dev;
+		struct ata_host_set *host_set = NULL;
+
+		if (legacy_mode & (1 << 0)) {
 			ata_device_add(probe_ent);
-		if (legacy_mode & (1 << 1))
+			host_set = dev_get_drvdata(dev);
+		}
+
+		if (legacy_mode & (1 << 1)) {
 			ata_device_add(probe_ent2);
+			if (host_set) {
+				host_set->next = dev_get_drvdata(dev);
+				dev_set_drvdata(dev, host_set);
+			}
+		}
 	} else
 		ata_device_add(probe_ent);
 
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 014855e..43f88ef 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -5507,37 +5507,41 @@ void ata_port_detach(struct ata_port *ap
  *	LOCKING:
  *	Inherited from calling layer (may sleep).
  */
-
 void ata_host_set_remove(struct ata_host_set *host_set)
 {
-	unsigned int i;
+	while (host_set) {
+		struct ata_host_set *next;
+		unsigned int i;
 
-	for (i = 0; i < host_set->n_ports; i++)
-		ata_port_detach(host_set->ports[i]);
+		for (i = 0; i < host_set->n_ports; i++)
+			ata_port_detach(host_set->ports[i]);
 
-	free_irq(host_set->irq, host_set);
+		free_irq(host_set->irq, host_set);
 
-	for (i = 0; i < host_set->n_ports; i++) {
-		struct ata_port *ap = host_set->ports[i];
+		for (i = 0; i < host_set->n_ports; i++) {
+			struct ata_port *ap = host_set->ports[i];
 
-		ata_scsi_release(ap->host);
+			ata_scsi_release(ap->host);
 
-		if ((ap->flags & ATA_FLAG_NO_LEGACY) == 0) {
-			struct ata_ioports *ioaddr = &ap->ioaddr;
+			if ((ap->flags & ATA_FLAG_NO_LEGACY) == 0) {
+				struct ata_ioports *ioaddr = &ap->ioaddr;
 
-			if (ioaddr->cmd_addr == 0x1f0)
-				release_region(0x1f0, 8);
-			else if (ioaddr->cmd_addr == 0x170)
-				release_region(0x170, 8);
-		}
+				if (ioaddr->cmd_addr == 0x1f0)
+					release_region(0x1f0, 8);
+				else if (ioaddr->cmd_addr == 0x170)
+					release_region(0x170, 8);
+			}
 
-		scsi_host_put(ap->host);
-	}
+			scsi_host_put(ap->host);
+		}
 
-	if (host_set->ops->host_stop)
-		host_set->ops->host_stop(host_set);
+		if (host_set->ops->host_stop)
+			host_set->ops->host_stop(host_set);
 
-	kfree(host_set);
+		next = host_set->next;
+		kfree(host_set);
+		host_set = next;
+	}
 }
 
 /**
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 61eea57..f03b866 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -356,7 +356,8 @@ struct ata_host_set {
 	unsigned long		flags;
 	int			simplex_claimed;	/* Keep seperate in case we
 							   ever need to do this locked */
-	struct ata_port *	ports[0];
+	struct ata_host_set	*next;		/* for legacy mode */
+	struct ata_port		*ports[0];
 };
 
 struct ata_queued_cmd {
-- 
1.3.2

Re: [PATCH] libata: add host_set->next for legacy two host_sets case, take #2

[Jeff Garzik]

Tejun Heo wrote:
> For a legacy ATA controller, libata registers two separate host sets.
> There was no connection between the two hosts making it impossible to
> traverse all ports related to the controller.  This patch adds
> host_set->next which points to the second host_set and makes
> ata_host_set_remove() remove all associated host_sets and ports.
> 
> * On device removal, all ports hanging off the device are properly
>   detached.  Prior to this patch, ports on the first host_set weren't
>   detached casuing oops on driver unloading.
> 
> * On device removal, both host_sets are properly freed
> 
> This will also be used by new power management code to suspend and
> resume all ports of a controller.  host_set/port representation will
> be improved to handle legacy controllers better and this host_set
> linking will go away with it.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

Same objection as before:  ata_host_set_remove() should only remove 
_one_ host_set.  Therefore, that function is ignorant of the linked 
list, and the caller is the one that is forced to manage the list.

	Jeff

Re: [PATCH] libata: add host_set->next for legacy two host_sets case, take #2

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> For a legacy ATA controller, libata registers two separate host sets.
>> There was no connection between the two hosts making it impossible to
>> traverse all ports related to the controller.  This patch adds
>> host_set->next which points to the second host_set and makes
>> ata_host_set_remove() remove all associated host_sets and ports.
>>
>> * On device removal, all ports hanging off the device are properly
>>   detached.  Prior to this patch, ports on the first host_set weren't
>>   detached casuing oops on driver unloading.
>>
>> * On device removal, both host_sets are properly freed
>>
>> This will also be used by new power management code to suspend and
>> resume all ports of a controller.  host_set/port representation will
>> be improved to handle legacy controllers better and this host_set
>> linking will go away with it.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> Same objection as before:  ata_host_set_remove() should only remove 
> _one_ host_set.  Therefore, that function is ignorant of the linked 
> list, and the caller is the one that is forced to manage the list.

Duh... I must be smoking something.  I thought I moved that loop out of 
that function.  :-(  Will submit again.

-- 
tejun

[PATCH] libata: add host_set->next for legacy two host_sets case, take #3

[Tejun Heo]

For a legacy ATA controller, libata registers two separate host sets.
There was no connection between the two hosts making it impossible to
traverse all ports related to the controller.  This patch adds
host_set->next which points to the second host_set and makes
ata_pci_remove_one() remove all associated host_sets.

* On device removal, all ports hanging off the device are properly
  detached.  Prior to this patch, ports on the first host_set weren't
  detached casuing oops on driver unloading.

* On device removal, both host_sets are properly freed

This will also be used by new power management code to suspend and
resume all ports of a controller.  host_set/port representation will
be improved to handle legacy controllers better and this host_set
linking will go away with it.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-bmdma.c |   15 +++++++++++++--
 drivers/scsi/libata-core.c  |    4 ++++
 include/linux/libata.h      |    3 ++-
 3 files changed, 19 insertions(+), 3 deletions(-)

d785d4a8e1eb5210dc351585610244567a0d5d70
diff --git a/drivers/scsi/libata-bmdma.c b/drivers/scsi/libata-bmdma.c
index 4bc0537..13fab97 100644
--- a/drivers/scsi/libata-bmdma.c
+++ b/drivers/scsi/libata-bmdma.c
@@ -1076,10 +1076,21 @@ int ata_pci_init_one (struct pci_dev *pd
 
 	/* FIXME: check ata_device_add return */
 	if (legacy_mode) {
-		if (legacy_mode & (1 << 0))
+		struct device *dev = &pdev->dev;
+		struct ata_host_set *host_set = NULL;
+
+		if (legacy_mode & (1 << 0)) {
 			ata_device_add(probe_ent);
-		if (legacy_mode & (1 << 1))
+			host_set = dev_get_drvdata(dev);
+		}
+
+		if (legacy_mode & (1 << 1)) {
 			ata_device_add(probe_ent2);
+			if (host_set) {
+				host_set->next = dev_get_drvdata(dev);
+				dev_set_drvdata(dev, host_set);
+			}
+		}
 	} else
 		ata_device_add(probe_ent);
 
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 014855e..d73cb36 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -5621,8 +5621,12 @@ void ata_pci_remove_one (struct pci_dev 
 {
 	struct device *dev = pci_dev_to_dev(pdev);
 	struct ata_host_set *host_set = dev_get_drvdata(dev);
+	struct ata_host_set *host_set2 = host_set->next;
 
 	ata_host_set_remove(host_set);
+	if (host_set2)
+		ata_host_set_remove(host_set2);
+
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
 	dev_set_drvdata(dev, NULL);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 61eea57..f03b866 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -356,7 +356,8 @@ struct ata_host_set {
 	unsigned long		flags;
 	int			simplex_claimed;	/* Keep seperate in case we
 							   ever need to do this locked */
-	struct ata_port *	ports[0];
+	struct ata_host_set	*next;		/* for legacy mode */
+	struct ata_port		*ports[0];
 };
 
 struct ata_queued_cmd {
-- 
1.3.2

Re: [PATCH] libata: add host_set->next for legacy two host_sets case, take #3

[Jeff Garzik]

Tejun Heo wrote:
> For a legacy ATA controller, libata registers two separate host sets.
> There was no connection between the two hosts making it impossible to
> traverse all ports related to the controller.  This patch adds
> host_set->next which points to the second host_set and makes
> ata_pci_remove_one() remove all associated host_sets.
> 
> * On device removal, all ports hanging off the device are properly
>   detached.  Prior to this patch, ports on the first host_set weren't
>   detached casuing oops on driver unloading.
> 
> * On device removal, both host_sets are properly freed
> 
> This will also be used by new power management code to suspend and
> resume all ports of a controller.  host_set/port representation will
> be improved to handle legacy controllers better and this host_set
> linking will go away with it.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

applied

Re: [PATCH] libata: add host_set->next for legacy two host_sets case, take #3

[Albert Lee]

Tejun Heo wrote:
> For a legacy ATA controller, libata registers two separate host sets.
> There was no connection between the two hosts making it impossible to
> traverse all ports related to the controller.  This patch adds
> host_set->next which points to the second host_set and makes
> ata_pci_remove_one() remove all associated host_sets.
> 
> * On device removal, all ports hanging off the device are properly
>   detached.  Prior to this patch, ports on the first host_set weren't
>   detached casuing oops on driver unloading.
> 
> * On device removal, both host_sets are properly freed
> 
> This will also be used by new power management code to suspend and
> resume all ports of a controller.  


> host_set/port representation will
> be improved to handle legacy controllers better and this host_set
> linking will go away with it.
> 

Hi Tejun,

Hmm, the current patch looks more like a temporary solution. The legacy mode
ATA_HOST_SIMPLEX host_set->flags could be assigned to both legacy ports.
In the long term, have one host_set for both legacy ports
instead of two host_sets for both legacy ports can fix the problem.

A patch was submitted by Unicorn, but it looks not good/elegant enough. :(
It will be nice if you have plan for furthur legacy two host_sets case fixes...

Thanks,

Albert

Re: [PATCH] libata: add host_set->next for legacy two host_sets case, take #3

[Tejun Heo]

Albert Lee wrote:
> Hmm, the current patch looks more like a temporary solution. The legacy mode
> ATA_HOST_SIMPLEX host_set->flags could be assigned to both legacy ports.
> In the long term, have one host_set for both legacy ports
> instead of two host_sets for both legacy ports can fix the problem.
> 
> A patch was submitted by Unicorn, but it looks not good/elegant enough. :(
> It will be nice if you have plan for furthur legacy two host_sets case fixes...

Hello, Albert.

Yeap, this host_set->next thing is a temporary solution and we need to 
handle legacy ports in one host_set.  I've seen Unicorn's patches.  I 
agree that those are in the right direction but we seem to need more 
clean up in driver initialization code.  I'll look into it once the next 
iteration of PMP patches are finished.

Thanks.

-- 
tejun

Re: [PATCH] libata: add host_set->next for legacy two host_sets case, take #3

[Albert Lee]

Tejun Heo wrote:
> Albert Lee wrote:
> 
>> Hmm, the current patch looks more like a temporary solution. The
>> legacy mode
>> ATA_HOST_SIMPLEX host_set->flags could be assigned to both legacy ports.
>> In the long term, have one host_set for both legacy ports
>> instead of two host_sets for both legacy ports can fix the problem.
>>
>> A patch was submitted by Unicorn, but it looks not good/elegant
>> enough. :(
>> It will be nice if you have plan for furthur legacy two host_sets case
>> fixes...
> 
> 
> Hello, Albert.
> 
> Yeap, this host_set->next thing is a temporary solution and we need to
> handle legacy ports in one host_set.  I've seen Unicorn's patches.  I
> agree that those are in the right direction but we seem to need more
> clean up in driver initialization code.  I'll look into it once the next
> iteration of PMP patches are finished.

Great to hear that. Looking forward to it. :)
--
albert

Re: [PATCH] libata: add host_set->next for legacy two host_sets case, take #3

[Alan Cox]

Ar Mer, 2006-06-28 am 19:42 +0900, ysgrifennodd Tejun Heo:
> clean up in driver initialization code.  I'll look into it once the next 
> iteration of PMP patches are finished.

This is what I have now on the host_set front which removes the chaining
of host sets and fixes simplex as well as cleaning up some other cases. 

Alan

diff --git a/drivers/scsi/libata-bmdma.c b/drivers/scsi/libata-bmdma.c
index 004e1a0..b1dfa8c 100644
--- a/drivers/scsi/libata-bmdma.c
+++ b/drivers/scsi/libata-bmdma.c
@@ -881,7 +881,7 @@ ata_pci_init_native_mode(struct pci_dev 
 		if (bmdma) {
 			bmdma += 8;
 			if(inb(bmdma + 2) & 0x80)
-			probe_ent->host_set_flags |= ATA_HOST_SIMPLEX;
+				probe_ent->host_set_flags |= ATA_HOST_SIMPLEX;
 			probe_ent->port[p].bmdma_addr = bmdma;
 		}
 		ata_std_ports(&probe_ent->port[p]);
@@ -894,45 +894,55 @@ ata_pci_init_native_mode(struct pci_dev 
 
 
 static struct ata_probe_ent *ata_pci_init_legacy_port(struct pci_dev *pdev,
-				struct ata_port_info *port, int port_num)
+				struct ata_port_info **port, int port_mask)
 {
 	struct ata_probe_ent *probe_ent;
-	unsigned long bmdma;
+	unsigned long bmdma = pci_resource_start(pdev, 4);
+
+	int port_num = 0;
 
-	probe_ent = ata_probe_ent_alloc(pci_dev_to_dev(pdev), port);
+	probe_ent = ata_probe_ent_alloc(pci_dev_to_dev(pdev), port[0]);
 	if (!probe_ent)
 		return NULL;
 
 	probe_ent->legacy_mode = 1;
 	probe_ent->n_ports = 1;
-	probe_ent->hard_port_no = port_num;
-	probe_ent->private_data = port->private_data;
+	probe_ent->hard_port_no = 0;
+	probe_ent->private_data = port[0]->private_data;
 
-	switch(port_num)
-	{
-		case 0:
-			probe_ent->irq = 14;
-			probe_ent->port[0].cmd_addr = 0x1f0;
-			probe_ent->port[0].altstatus_addr =
-			probe_ent->port[0].ctl_addr = 0x3f6;
-			break;
-		case 1:
-			probe_ent->irq = 15;
-			probe_ent->port[0].cmd_addr = 0x170;
-			probe_ent->port[0].altstatus_addr =
-			probe_ent->port[0].ctl_addr = 0x376;
-			break;
+	if (port_mask & ATA_PORT_PRIMARY) {
+		probe_ent->irq = 14;
+		probe_ent->port[port_num].cmd_addr = ATA_PRIMARY_CMD;
+		probe_ent->port[port_num].altstatus_addr =
+		probe_ent->port[port_num].ctl_addr = ATA_PRIMARY_CTL;
+		if (bmdma) {
+			probe_ent->port[0].bmdma_addr = bmdma;
+			if (inb(bmdma + 2) & 0x80)
+				probe_ent->host_set_flags |= ATA_HOST_SIMPLEX;
+		}
+		ata_std_ports(&probe_ent->port[port_num]);
+		port_num ++;
 	}
-
-	bmdma = pci_resource_start(pdev, 4);
-	if (bmdma != 0) {
-		bmdma += 8 * port_num;
-		probe_ent->port[0].bmdma_addr = bmdma;
-		if (inb(bmdma + 2) & 0x80)
-			probe_ent->host_set_flags |= ATA_HOST_SIMPLEX;
+	if (port_mask & ATA_PORT_SECONDARY) {
+		if (port_num == 1)
+			probe_ent->irq2 = 15;
+		else {
+			/* Secondary only. IRQ 15 only and "first" port is port 1 */
+			probe_ent->irq = 15;
+			probe_ent->hard_port_no = 1;
+		}
+		probe_ent->port[port_num].cmd_addr = ATA_SECONDARY_CMD;
+		probe_ent->port[port_num].altstatus_addr =
+		probe_ent->port[port_num].ctl_addr = ATA_SECONDARY_CTL;
+		port_num ++;
+		if (bmdma) {
+			probe_ent->port[port_num].bmdma_addr = bmdma + 8;
+			if (inb(bmdma + 10) & 0x80)
+				probe_ent->host_set_flags |= ATA_HOST_SIMPLEX;
+		}
+		ata_std_ports(&probe_ent->port[port_num]);
+		port_num ++;
 	}
-	ata_std_ports(&probe_ent->port[0]);
-
 	return probe_ent;
 }
 
@@ -951,6 +961,10 @@ static struct ata_probe_ent *ata_pci_ini
  *	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).
  *
@@ -961,7 +975,7 @@ static struct ata_probe_ent *ata_pci_ini
 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, *probe_ent2 = NULL;
+	struct ata_probe_ent *probe_ent = NULL;
 	struct ata_port_info *port[2];
 	u8 tmp8, mask;
 	unsigned int legacy_mode = 0;
@@ -1010,35 +1024,34 @@ int ata_pci_init_one (struct pci_dev *pd
 		goto err_out;
 	}
 
-	/* FIXME: Should use platform specific mappers for legacy port ranges */
 	if (legacy_mode) {
-		if (!request_region(0x1f0, 8, "libata")) {
+		if (!request_region(ATA_PRIMARY_CMD, 8, "libata")) {
 			struct resource *conflict, res;
-			res.start = 0x1f0;
-			res.end = 0x1f0 + 8 - 1;
+			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 |= (1 << 0);
+				legacy_mode |= ATA_PORT_PRIMARY;
 			else {
 				disable_dev_on_err = 0;
-				printk(KERN_WARNING "ata: 0x1f0 IDE port busy\n");
+				printk(KERN_WARNING "ata: 0x%0X IDE port busy\n", ATA_PRIMARY_CMD);
 			}
 		} else
-			legacy_mode |= (1 << 0);
+			legacy_mode |= ATA_PORT_PRIMARY;
 
-		if (!request_region(0x170, 8, "libata")) {
+		if (!request_region(ATA_SECONDARY_CMD, 8, "libata")) {
 			struct resource *conflict, res;
-			res.start = 0x170;
-			res.end = 0x170 + 8 - 1;
+			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 |= (1 << 1);
+				legacy_mode |= ATA_PORT_SECONDARY;
 			else {
 				disable_dev_on_err = 0;
-				printk(KERN_WARNING "ata: 0x170 IDE port busy\n");
+				printk(KERN_WARNING "ata: 0x%X IDE port busy\n", ATA_SECONDARY_CMD);
 			}
 		} else
-			legacy_mode |= (1 << 1);
+			legacy_mode |= ATA_PORT_SECONDARY;
 	}
 
 	/* we have legacy mode, but all ports are unavailable */
@@ -1056,17 +1069,14 @@ int ata_pci_init_one (struct pci_dev *pd
 		goto err_out_regions;
 
 	if (legacy_mode) {
-		if (legacy_mode & (1 << 0))
-			probe_ent = ata_pci_init_legacy_port(pdev, port[0], 0);
-		if (legacy_mode & (1 << 1))
-			probe_ent2 = ata_pci_init_legacy_port(pdev, port[1], 1);
+		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 && !probe_ent2) {
+	if (!probe_ent) {
 		rc = -ENOMEM;
 		goto err_out_regions;
 	}
@@ -1074,35 +1084,17 @@ int ata_pci_init_one (struct pci_dev *pd
 	pci_set_master(pdev);
 
 	/* FIXME: check ata_device_add return */
-	if (legacy_mode) {
-		struct device *dev = &pdev->dev;
-		struct ata_host_set *host_set = NULL;
-
-		if (legacy_mode & (1 << 0)) {
-			ata_device_add(probe_ent);
-			host_set = dev_get_drvdata(dev);
-		}
-
-		if (legacy_mode & (1 << 1)) {
-			ata_device_add(probe_ent2);
-			if (host_set) {
-				host_set->next = dev_get_drvdata(dev);
-				dev_set_drvdata(dev, host_set);
-			}
-		}
-	} else
-		ata_device_add(probe_ent);
+	ata_device_add(probe_ent);
 
 	kfree(probe_ent);
-	kfree(probe_ent2);
 
 	return 0;
 
 err_out_regions:
-	if (legacy_mode & (1 << 0))
-		release_region(0x1f0, 8);
-	if (legacy_mode & (1 << 1))
-		release_region(0x170, 8);
+	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)
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 6c66877..d48b93e 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -5196,6 +5196,7 @@ static void ata_host_init(struct ata_por
 	ap->host_set = host_set;
 	ap->dev = ent->dev;
 	ap->port_no = port_no;
+
 	ap->hard_port_no =
 		ent->legacy_mode ? ent->hard_port_no : port_no;
 	ap->pio_mask = ent->pio_mask;
@@ -5331,6 +5332,7 @@ int ata_device_add(const struct ata_prob
 	host_set->dev = dev;
 	host_set->n_ports = ent->n_ports;
 	host_set->irq = ent->irq;
+	host_set->irq2 = ent->irq2;
 	host_set->mmio_base = ent->mmio_base;
 	host_set->private_data = ent->private_data;
 	host_set->ops = ent->port_ops;
@@ -5340,16 +5342,23 @@ int ata_device_add(const struct ata_prob
 	for (i = 0; i < ent->n_ports; i++) {
 		struct ata_port *ap;
 		unsigned long xfer_mode_mask;
+		int irq_line = ent->irq;
+
 
 		ap = ata_host_add(ent, host_set, i);
 		if (!ap)
 			goto err_out;
 
+		/* Report the secondary IRQ for second channel legacy */
+		if (i == 1 && ent->irq2)
+			irq_line = ent->irq2;
+
 		host_set->ports[i] = ap;
 		xfer_mode_mask =(ap->udma_mask << ATA_SHIFT_UDMA) |
 				(ap->mwdma_mask << ATA_SHIFT_MWDMA) |
 				(ap->pio_mask << ATA_SHIFT_PIO);
 
+		/* FIXME: maybe print both IRQ lines ? */
 		/* 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 %lu\n",
@@ -5369,7 +5378,7 @@ int ata_device_add(const struct ata_prob
 	if (!count)
 		goto err_free_ret;
 
-	/* obtain irq, that is shared between channels */
+	/* obtain irq, that may be shared between channels */
 	rc = request_irq(ent->irq, ent->port_ops->irq_handler, ent->irq_flags,
 			 DRV_NAME, host_set);
 	if (rc) {
@@ -5378,6 +5387,21 @@ int ata_device_add(const struct ata_prob
 		goto err_out;
 	}
 
+	/* do we have a second IRQ for the other channel, eg legacy mode */
+	if (ent->irq2) {
+		/* We will get weird core code crashes later if this is true 
+		   so trap it now */
+		BUG_ON(ent->irq == ent->irq2);
+
+		rc = request_irq(ent->irq2, ent->port_ops->irq_handler, ent->irq_flags,
+			 DRV_NAME, host_set);
+		if (rc) {
+			dev_printk(KERN_ERR, dev, "irq %lu request failed: %d\n",
+				   ent->irq2, rc);
+			goto err_out;
+		}
+	}
+
 	/* perform each probe synchronously */
 	DPRINTK("probe begin\n");
 	for (i = 0; i < count; i++) {
@@ -5538,6 +5562,8 @@ void ata_host_set_remove(struct ata_host
 		ata_port_detach(host_set->ports[i]);
 
 	free_irq(host_set->irq, host_set);
+	if (host_set->irq2)
+		free_irq(host_set->irq2, host_set);
 
 	for (i = 0; i < host_set->n_ports; i++) {
 		struct ata_port *ap = host_set->ports[i];
@@ -5547,10 +5573,11 @@ void ata_host_set_remove(struct ata_host
 		if ((ap->flags & ATA_FLAG_NO_LEGACY) == 0) {
 			struct ata_ioports *ioaddr = &ap->ioaddr;
 
-			if (ioaddr->cmd_addr == 0x1f0)
-				release_region(0x1f0, 8);
-			else if (ioaddr->cmd_addr == 0x170)
-				release_region(0x170, 8);
+			/* 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);
 		}
 
 		scsi_host_put(ap->host);
@@ -5643,11 +5670,8 @@ void ata_pci_remove_one (struct pci_dev 
 {
 	struct device *dev = pci_dev_to_dev(pdev);
 	struct ata_host_set *host_set = dev_get_drvdata(dev);
-	struct ata_host_set *host_set2 = host_set->next;
 
 	ata_host_set_remove(host_set);
-	if (host_set2)
-		ata_host_set_remove(host_set2);
 
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
diff --git a/include/asm-alpha/libata-portmap.h b/include/asm-alpha/libata-portmap.h
new file mode 100644
index 0000000..9202fd0
--- /dev/null
+++ b/include/asm-alpha/libata-portmap.h
@@ -0,0 +1,12 @@
+#ifndef __ASM_GENERIC_LIBATA_PORTMAP_H
+#define __ASM_GENERIC_LIBATA_PORTMAP_H
+
+#define ATA_PRIMARY_CMD		0x1F0
+#define ATA_PRIMARY_CTL		0x3F6
+#define ATA_PRIMARY_IRQ		14
+
+#define ATA_SECONDARY_CMD	0x170
+#define ATA_SECONDARY_CTL	0x376
+#define ATA_SECONDARY_IRQ	15
+
+#endif
diff --git a/include/asm-i386/libata-portmap.h b/include/asm-i386/libata-portmap.h
new file mode 100644
index 0000000..9202fd0
--- /dev/null
+++ b/include/asm-i386/libata-portmap.h
@@ -0,0 +1,12 @@
+#ifndef __ASM_GENERIC_LIBATA_PORTMAP_H
+#define __ASM_GENERIC_LIBATA_PORTMAP_H
+
+#define ATA_PRIMARY_CMD		0x1F0
+#define ATA_PRIMARY_CTL		0x3F6
+#define ATA_PRIMARY_IRQ		14
+
+#define ATA_SECONDARY_CMD	0x170
+#define ATA_SECONDARY_CTL	0x376
+#define ATA_SECONDARY_IRQ	15
+
+#endif
diff --git a/include/asm-x86_64/libata-portmap.h b/include/asm-x86_64/libata-portmap.h
new file mode 100644
index 0000000..9202fd0
--- /dev/null
+++ b/include/asm-x86_64/libata-portmap.h
@@ -0,0 +1,12 @@
+#ifndef __ASM_GENERIC_LIBATA_PORTMAP_H
+#define __ASM_GENERIC_LIBATA_PORTMAP_H
+
+#define ATA_PRIMARY_CMD		0x1F0
+#define ATA_PRIMARY_CTL		0x3F6
+#define ATA_PRIMARY_IRQ		14
+
+#define ATA_SECONDARY_CMD	0x170
+#define ATA_SECONDARY_CTL	0x376
+#define ATA_SECONDARY_IRQ	15
+
+#endif
diff --git a/include/linux/libata.h b/include/linux/libata.h
index a14ebdb..4c05fee 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -35,6 +35,8 @@
 #include <linux/workqueue.h>
 #include <scsi/scsi_host.h>
 
+#include <asm/libata-portmap.h>
+
 /*
  * compile-time options: to be removed as soon as all the drivers are
  * converted to the new debugging mechanism
@@ -339,6 +341,7 @@ struct ata_probe_ent {
 	unsigned int		udma_mask;
 	unsigned int		legacy_mode;
 	unsigned long		irq;
+	unsigned long		irq2;
 	unsigned int		irq_flags;
 	unsigned long		host_flags;
 	unsigned long		host_set_flags;
@@ -350,6 +353,7 @@ struct ata_host_set {
 	spinlock_t		lock;
 	struct device 		*dev;
 	unsigned long		irq;
+	unsigned long		irq2;
 	void __iomem		*mmio_base;
 	unsigned int		n_ports;
 	void			*private_data;
@@ -357,7 +361,6 @@ struct ata_host_set {
 	unsigned long		flags;
 	int			simplex_claimed;	/* Keep seperate in case we
 							   ever need to do this locked */
-	struct ata_host_set	*next;		/* for legacy mode */
 	struct ata_port		*ports[0];
 };
 

Re: [PATCH] libata: add host_set->next for legacy two host_sets case, take #3

[Alan Cox]

Ar Mer, 2006-06-28 am 18:27 +0800, ysgrifennodd Albert Lee:
> Hmm, the current patch looks more like a temporary solution. The legacy mode
> ATA_HOST_SIMPLEX host_set->flags could be assigned to both legacy ports.
> In the long term, have one host_set for both legacy ports
> instead of two host_sets for both legacy ports can fix the problem.

I'm diddling with this at the moment. Following up on Jeffs request that
there should be a single host_set in such cases since its the same pci
device.

Alan