legacy libata IDE missing reservation

legacy libata IDE missing reservation

[Jeff Garzik]

FYI:  I noticed this weekend that libata does not reserve the legacy
IDE control block I/O port region, like it does the command block
I/O region.

	Jeff

[PATCH] libata: claim ctl port for legacy SFF ports

[Tejun Heo]

libata used to claim only cmd ports for legacy SFF ports.  Claim ctl
ports too.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
This has been this way as long as I can remember.  It's almost
pleasant to see that it survived through all the devres/init rework.
I guess it's time for a change.

 drivers/ata/libata-sff.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index e35d134..99a9f98 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -712,6 +712,7 @@ int ata_pci_prepare_native_host(struct pci_dev *pdev,
 struct ata_legacy_devres {
 	unsigned int	mask;
 	unsigned long	cmd_port[2];
+	unsigned long	ctl_port[2];
 	void __iomem *	cmd_addr[2];
 	void __iomem *	ctl_addr[2];
 	unsigned int	irq[2];
@@ -764,12 +765,16 @@ static int ata_init_legacy_port(struct ata_port *ap,
 		ctl_port = ATA_SECONDARY_CTL;
 	}
 
-	/* request cmd_port */
+	/* request cmd and ctl port */
 	if (request_region(cmd_port, 8, "libata"))
 		legacy_dr->cmd_port[port_no] = cmd_port;
-	else {
+
+	if (request_region(ctl_port, 8, "libata"))
+		legacy_dr->ctl_port[port_no] = ctl_port;
+
+	if (!legacy_dr->cmd_port[port_no] || !legacy_dr->ctl_port[port_no]) {
 		dev_printk(KERN_WARNING, host->dev,
-			   "0x%0lX IDE port busy\n", cmd_port);
+			   "0x%0lX/0x%0lX IDE port busy\n", cmd_port, ctl_port);
 		return -EBUSY;
 	}
 

Re: [PATCH] libata: claim ctl port for legacy SFF ports

[Alan Cox]

On Fri, 8 Jun 2007 20:41:19 +0900
Tejun Heo <htejun@gmail.com> wrote:

> libata used to claim only cmd ports for legacy SFF ports.  Claim ctl
> ports too.

Just use pci resource functions. The quirk handlers do the rest for you
already.

Re: [PATCH] libata: claim ctl port for legacy SFF ports

[Tejun Heo]

Alan Cox wrote:
> On Fri, 8 Jun 2007 20:41:19 +0900
> Tejun Heo <htejun@gmail.com> wrote:
> 
>> libata used to claim only cmd ports for legacy SFF ports.  Claim ctl
>> ports too.
> 
> Just use pci resource functions. The quirk handlers do the rest for you
> already.

It was that way to allow legacy SFF code to be used in non-PCI
environment.  Is it gonna?

BTW, the original patch is broken and I tested the wrong kernel.
Brewing fixed one now.

-- 
tejun

[PATCH] libata: claim legacy SFF ctl ioport in legacy_dr

[Tejun Heo]

For legacy SFF port, cmd ioport is claimed in legacy_dr while ctl
ioport is claimed using resource-managed pci_request_region().  Move
ctl port claiming into legacy_dr for consistency.  This also makes
legacy SFF code easier to use for non-PCI device.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
ctl ports *were* claimed - this is why my wrong kernel test looked
fine.  It was just done in a different place.

Thanks.

 drivers/ata/libata-sff.c |   32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index e35d134..36d5f38 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -712,6 +712,7 @@ int ata_pci_prepare_native_host(struct pci_dev *pdev,
 struct ata_legacy_devres {
 	unsigned int	mask;
 	unsigned long	cmd_port[2];
+	unsigned long	ctl_port[2];
 	void __iomem *	cmd_addr[2];
 	void __iomem *	ctl_addr[2];
 	unsigned int	irq[2];
@@ -746,6 +747,8 @@ static void ata_legacy_release(struct device *gdev, void *res)
 			ioport_unmap(this->ctl_addr[i]);
 		if (this->cmd_port[i])
 			release_region(this->cmd_port[i], 8);
+		if (this->ctl_port[i])
+			release_region(this->ctl_port[i], 1);
 	}
 }
 
@@ -764,7 +767,7 @@ static int ata_init_legacy_port(struct ata_port *ap,
 		ctl_port = ATA_SECONDARY_CTL;
 	}
 
-	/* request cmd_port */
+	/* request and map cmd/ctl ports */
 	if (request_region(cmd_port, 8, "libata"))
 		legacy_dr->cmd_port[port_no] = cmd_port;
 	else {
@@ -773,15 +776,30 @@ static int ata_init_legacy_port(struct ata_port *ap,
 		return -EBUSY;
 	}
 
-	/* iomap cmd and ctl ports */
 	legacy_dr->cmd_addr[port_no] = ioport_map(cmd_port, 8);
-	legacy_dr->ctl_addr[port_no] = ioport_map(ctl_port, 1);
-	if (!legacy_dr->cmd_addr[port_no] || !legacy_dr->ctl_addr[port_no]) {
+	if (!legacy_dr->cmd_addr[port_no]) {
 		dev_printk(KERN_WARNING, host->dev,
-			   "failed to map cmd/ctl ports\n");
+			   "failed to map cmd port\n");
 		return -ENOMEM;
 	}
 
+	if (ctl_port) {
+		if (request_region(ctl_port, 1, "libata"))
+			legacy_dr->ctl_port[port_no] = ctl_port;
+		else {
+			dev_printk(KERN_WARNING, host->dev,
+				   "0x%0lX IDE port busy\n", ctl_port);
+			return -EBUSY;
+		}
+
+		legacy_dr->ctl_addr[port_no] = ioport_map(ctl_port, 1);
+		if (!legacy_dr->ctl_addr[port_no]) {
+			dev_printk(KERN_WARNING, host->dev,
+				   "failed to map ctl port\n");
+			return -ENOMEM;
+		}
+	}
+
 	/* init IO addresses */
 	ap->ioaddr.cmd_addr = legacy_dr->cmd_addr[port_no];
 	ap->ioaddr.altstatus_addr = legacy_dr->ctl_addr[port_no];
@@ -1029,10 +1047,6 @@ int ata_pci_init_one(struct pci_dev *pdev,
 			pcim_pin_device(pdev);
 		if (rc)
 			goto err_out;
-
-		/* request respective PCI regions, may fail */
-		rc = pci_request_region(pdev, 1, DRV_NAME);
-		rc = pci_request_region(pdev, 3, DRV_NAME);
 	}
 
 	/* init BMDMA, may fail */

Re: [PATCH] libata: claim ctl port for legacy SFF ports

[Alan Cox]

> It was that way to allow legacy SFF code to be used in non-PCI
> environment.  Is it gonna?

Probably best to keep that in the affected driver - we have two drivers
at the moment which know bits about ISA legacy and I'm currently folding
them into one. I don't think they can usefully use the libata-sff PCI/IDE
helper code - not all of it anyway.

Re: [PATCH] libata: claim ctl port for legacy SFF ports

[Jeff Garzik]

On Fri, Jun 08, 2007 at 08:41:19PM +0900, Tejun Heo wrote:
> libata used to claim only cmd ports for legacy SFF ports.  Claim ctl
> ports too.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
> This has been this way as long as I can remember.  It's almost
> pleasant to see that it survived through all the devres/init rework.
> I guess it's time for a change.

pci_request_regions() should claims everything, if Alan did his job :)

All this goes through ata_pci_init_one(), which guarantees it's PCI IDE.

	Jeff

Re: [PATCH] libata: claim ctl port for legacy SFF ports

[Tejun Heo]

Jeff Garzik wrote:
> On Fri, Jun 08, 2007 at 08:41:19PM +0900, Tejun Heo wrote:
>> libata used to claim only cmd ports for legacy SFF ports.  Claim ctl
>> ports too.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> ---
>> This has been this way as long as I can remember.  It's almost
>> pleasant to see that it survived through all the devres/init rework.
>> I guess it's time for a change.
> 
> pci_request_regions() should claims everything, if Alan did his job :)
> 
> All this goes through ata_pci_init_one(), which guarantees it's PCI IDE.

Allowing legacy to be used on non-PCI systems was your idea!  Alright,
then we can drop a lot of junk from legacy SFF code.  Will prep another
patch.  Please ignore two patches in this thread.

-- 
tejun

Re: [PATCH] libata: claim ctl port for legacy SFF ports

[Jeff Garzik]

On Fri, Jun 08, 2007 at 11:12:01PM +0900, Tejun Heo wrote:
> Jeff Garzik wrote:
> > On Fri, Jun 08, 2007 at 08:41:19PM +0900, Tejun Heo wrote:
> >> libata used to claim only cmd ports for legacy SFF ports.  Claim ctl
> >> ports too.
> >>
> >> Signed-off-by: Tejun Heo <htejun@gmail.com>
> >> ---
> >> This has been this way as long as I can remember.  It's almost
> >> pleasant to see that it survived through all the devres/init rework.
> >> I guess it's time for a change.
> > 
> > pci_request_regions() should claims everything, if Alan did his job :)
> > 
> > All this goes through ata_pci_init_one(), which guarantees it's PCI IDE.
> 
> Allowing legacy to be used on non-PCI systems was your idea!  Alright,
> then we can drop a lot of junk from legacy SFF code.  Will prep another
> patch.  Please ignore two patches in this thread.

Alan seems to be ignoring that idea, so it turned out to be useless...  :/

	Jeff

Re: [PATCH] libata: claim ctl port for legacy SFF ports

[Alan Cox]

> > Allowing legacy to be used on non-PCI systems was your idea!  Alright,
> > then we can drop a lot of junk from legacy SFF code.  Will prep another
> > patch.  Please ignore two patches in this thread.
> 
> Alan seems to be ignoring that idea, so it turned out to be useless...  :/

For the init code stuff it turns out to be useless, for the rest of the
general chip kicking it turns out to be very useful indeed.

Alan

Re: [PATCH] libata: claim ctl port for legacy SFF ports

[Jeff Garzik]

On Fri, Jun 08, 2007 at 04:33:37PM +0100, Alan Cox wrote:
> > > Allowing legacy to be used on non-PCI systems was your idea!  Alright,
> > > then we can drop a lot of junk from legacy SFF code.  Will prep another
> > > patch.  Please ignore two patches in this thread.
> > 
> > Alan seems to be ignoring that idea, so it turned out to be useless...  :/
> 
> For the init code stuff it turns out to be useless, for the rest of the
> general chip kicking it turns out to be very useful indeed.

Unless I've completely lost the thread, it's the only init code that
is being discussed and patched...

	Jeff