Re: [PATCH] Add proper module ID tables to Adaptec aic7[9x]xx drivers

[Luben Tuikov <luben_tuikov@adaptec.com>]

Andi Kleen wrote:
> On Wed, Oct 06, 2004 at 11:47:33PM +0400, Michael Tokarev wrote:
> 
>>[Answering to an old post -- rehashing the topic.
>> I didn't trim the email.  See my comments below.]
>>
>>On Mon, 21 Jun 2004 20:10:47 +0400, Sergey Vlasov wrote:
>>
>>>On Mon, Jun 21, 2004 at 06:24:40PM +0200, Andi Kleen wrote:
>>>
>>>
>>>>On Mon, 21 Jun 2004 17:44:08 +0400
>>>>Sergey Vlasov <vsu@altlinux.ru> wrote:
>>>>
>>>>
>>>>>On Mon, 21 Jun 2004 16:14:41 +0200 Andi Kleen wrote:
>>>>>
>>>>>
>>>>>
>>>>>>This is needed for 2.6 hotplug where the driver is autoloaded. When you 
>>>>>>have
>>>>>>multiple conflicting entries the hotplug module loader usually loads
>>>>>>the first one listed, which may be correct or may be not.
>>>>>>
>>>>>>With these changes the drivers announce the correct PCI IDs.
>>>>>
>>>>>Unfortunately, the patch does not seem to be correct :(
>>>>>
>>>>>struct pci_device_id does not have the mask field, therefore the
>>>>>ID_9005_GENERIC_MASK restriction cannot be specified other than by
>>>>>listing all 16 possible IDs as separate entries.  Your patch adds only
>>>>>one entry, thus losing 15 other possible IDs.
>>>>
>>>>Hmm, good point.  Thanks for catching this.
>>>>
>>>>Here is a new patch.  Does this one look better?
>>>
>>>It fixes the above problem, but it's hard to say that the patch is
>>>correct without carefully checking all the tables.  I'm trying to
>>>hack up something to autogenerate the pci_device_id table from the
>>>aic7xxx internal table.
>>>
>>>BTW, ID_AIC7810 and ID_AIC7815 probably should not be in the PCI ID
>>>table at all - ahc_raid_setup() just prints "RAID functionality
>>>unsupported" for them.  And some more IDs generated by ID16 are
>>>really rejected by the driver due to the (ID_9005_SISL_ID,
>>>ID_9005_SISL_MASK) exclusion entry.
>>
>>I found several "OEM" (on-board) aic79xx controllers (mostly on
>>HP boxes) that breaks after the above-mentioned change, which
>>found it's way into 2.6.8 kernel.  For example, HP ProLiant ML
> 
> 
> I don't think it's merged in mainline, no. Or at least 2.6.9rc3 
> doesn't have it.
> 
> 
> 
>>machines and others.  The end-result is that the driver, which
>>worked just fine before, does not "detect" the card anymore.
>>
>>On one of such machines, AIC7902 U320 (rev 03) card is shown
>>as device=9005:801f (identified by the driver), but subsystem
>>id is 103c:103c, wich gets interpreted by pci.ids as
>>"Hewlett-Packard Company: Unknown device 103c".  It does
>>not look like a right thing to do, but HP knows better it
>>seems.
>>
>>Also, I tried to guess what all those new PCI ID macros does
>>in the driver, but that's quite a challenge: deeply-nested
>>macros with non-obvious bit manipulation... ;)   So I can't
> 
> 
> They just try to match large groups without too much typing....
> 
> 
>>produce a patch right now for this problem.  Either way,
>>the resulting PCI table does not look right:
>>
>>$ fgrep aic79xx /lib/modules/`uname -r`/modules.pcimap | wc -l
>>32
>>$ fgrep aic79xx /lib/modules/`uname -r`/modules.pcimap | sort -u | wc -l
>>15
>>
>>Devices 9005:800f and 9005:801e are listed with *:* subsystem,
>>while the rest (incl. 9005:801f -- the one shown above) specifies
>>several non-wildcard subsystem entries, nothing to match 103c
>>(HP).
> 
> 
> If I remember the patch correctly it only matches the Adaptec vendor id. 
> 
> For HP there will need to be an special entry (probably needs some
> reverse engineering from the original code) 
> 
> 
>>Maybe just list all subsystems as wildcards?
> 
> 
> No, i don't think that's a good idea.
> 
> At least the vendors should be listed explicitely. 
> Otherwise hotplug autoloading is unhappy because bogus modules 
> get loaded all the time.
> 
> I can go over it again at some point, but currently I don't 
> have too much time, so it would be good if someone else would
> tackle it.
> 
> Luben, can you just do a proper pci_id table please? You probably
> know the requirements best.

Yes, no problem. (It'd be tricky but I'll try to augment it properly.)

	Luben

> -Andi
>