[openib-general] [PATCH 5/6] Use pci_find_ht_capability() in drivers/pci/quirks.c

[ebiederm at xmission.com]

Brice Goglin <brice at myri.com> writes:
>
> How do you want to detect the following loop in pci_find_capability()
> without changing the API?
>     any cap -> any cap -> one HT cap -> any cap -> back to first HT cap
> When looking for a HT cap, pci_find_capability() will always succeed, it
> will never loop forever. But, pci_find_ht_capability() will loop forever
> if it is looking for a different HT cap than the one we have in the chain.

Ok.  Reading the code now..

pci_find_capability will never loop forever precisely we have the ttl
check in __pci_find_next_cap, which underlies both pci_find_capability,
and pci_find_next_capability.  While it is a little silly we can use
the exact same logic on the hypertransport side.

What I was actually thinking and this may almost be better is to perform
a preemptive walk pci_find_capability and see if there is a loop and
issue a warning and refuse to deal with the device if that is the case.

Or being more generous have pci_find_capability or some similar function
do a walk and find the end of the chain, as defined be either the point
the chain terminates, or the point where the chain loops back in on itself.
At which point you only need the ttl check once.

One of the above two solutions is probably preferable so that higher
level interfaces don't have to think about infinite loops.

> When we add a new function looping on top of the existing functions, we
> can always find a brain-damaged chain which makes the new function loop
> forever while the existing ones do not... That's why I had a new TTL in
> msi_ht_cap_enabled() in drivers/pci/quirks.c even if I was using
> pci_find_next_capability() which has its own TTL.
>
> Anyway, I agree that protecting against bugs that we've never seen
> before is not that important. I was just thinking that protecting is
> very easy, while debugging might be boring once we find such a broken
> PCI chain :)

Right, and I agree that it makes sense to be a little paranoid especially
in generic code.  I just don't think it makes sense to make the user
of the interface care that we are being paranoid.

Eric