linux-kernel - Re: [PATCH] libata: fix combined mode (was Re: Happy New Year (and v2.6.20-rc3 released))

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20070103003635.626cdfb3@localhost.localdomain>
Date:	Wed, 3 Jan 2007 00:36:35 +0000
From:	Alan <alan@...rguk.ukuu.org.uk>
To:	Jeff Garzik <jgarzik@...ox.com>
Cc:	Linus Torvalds <torvalds@...l.org>,
	Alessandro Suardi <alessandro.suardi@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] libata: fix combined mode (was Re: Happy New Year (and
 v2.6.20-rc3 released))

> 	1) Programmatically reserve /all/ resources associated with
> 	   our PCI device
> 	2) Manually reserve resources associated with our PCI device,
> 	   but are not listed in struct pci_dev.

Which it doesn't actually do. You reserve 0x1F0-0x1F7 but forget the
other register. BTW on unload you forget to release 0x1F0-7 too. I've not
fixed that as its not a regression just a bug. It's just another symptom
of a pile of code trying to do things the wrong way, in the wrong place.

> But then 2.6.21 goes back to:
> 
> 	1) Programmatically reserve /all/ resources associated with
> 	   our PCI device
> 	2) Manually reserve resources associated with our PCI device,
> 	   but are not listed in struct pci_dev.

Nope. You are either very confused about how PCI bus resources work or
you are trying to implement the future code in a very very peculiar way 8)

Remember with the resource tree now correct all the resources for an IDE
controller *are* in the pci_dev struct properly - the special cases are
all gone in libata and in drivers/ide.

Once combined mode is fixed not to abuse resources (and it originally
did it that way for a good reason I grant and am not criticising that) the
entire management for legacy mode, mixed mode and native mode resources
for an ATA device (including 0x170, 0x3F6 and other wacky magic) becomes

	if (pci_request_regions(pdev, "libata")) ...

You'll note:
- No special cases for differing modes
- No libata knowledge of PCI legacy mapping rules and addresses
- The death of the magic ATA_PRIMARY/SECONDARY constants and their magic
numbers
- Support for platforms that map legacy space differently
- Trivial cleanup from failure unlike the current code

all in one line. This will also fix all the existing bugs where unloading
a libata driver fails to free resources as pci_release_regions() will also
now do the correct thing.

*That* is one key reason why getting the PCI resource map right is so
important. We turn fifty lines of bug ridden hard to debug code into one
line of code that actually does more than the original, and gets it
right. For free we get the leaked resources after rmmod fixed, we get the
mixed mode resources fixed, we get all this stuff for free. We get to
shoot a chunk of code in drivers/ide if we want as well.

If we want to keep a combined mode in 2.6.21 with drivers/ide (which
seems dumb as libata has progressed far beyond the need for it) then the
-mm tree has a pci_request_regions_mask() function which we can push
to .21 development and we end up with five lines not three for these
cases.

Make sense ?

Alan
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/