On Tue, Jan 21, 2014 at 3:01 AM, George Spelvin <linux@xxxxxxxxxxx> wrote: >>> I massaged it a bit to fit my personal idea of "cleaner". >>> The biggest logic change is the elimination of the fn_mapped >>> variable from quirk_map_multi_requester_ids. > >> Thanks, that was detritus left over from debugging. > > The code still does the exact same thing; I just derive the value from > (fn_map & ((1<<fn)-1)) if it's required. I see that now - was over excited and tired last night. >>> I also got rid of the "fn_map << fn" logic (which is never false). > >> What if function 0 is not present? There were cases where the Marvell >> 9172 only used function 1 (when only one port is in use). I don't >> think it's safe to assume that function 0 is present when you're >> trying to compensate for broken devices. > > Er... can you explain? I did not change what the code *does* in the > slightest, only the was it figures out what to do. > > I took this it out because it does nothing; it can be statically proved > that the expression is always non-zero. > > Because "fn_map" is a non-zero 8-bit value and "fn" is an integer in > the range 0-7, the value "fn_map << fn" is a non-zero 15-bit value. > 15 bits can't overflow (even on a PDP-11 with 16-bit ints), so this is > a non-zero value, and the loop will never terminate because of it. > > It sure looked like a bug to me, but I couldn't figure out what it > was supposed to do. Can you enlighten me? "fn_map >> fn" (shifting > right rather than left) achieves early termination of the loop, but > isn't essential. OK, I understand the problem now. Early exit was the intended effect and for some reason I thought the new version could exit too early, but it seems fine. >>> + fn_map &= ~(1<<PCI_FUNC(pdev->devfn)); /* Skip the normal case */ > >> Do you really find this "cleaner", as in more readable? > > I find it easier to think about fn_map as the bitmap of functions to > be operated on. Rather than have two conditions in the following loop. I see that now. > The confusing thing is "why are we skipping that function?" (Because this > is a quirk and the "normal case" has already been handled.) It would be > nicer if all the domain_context_mapping_one calls were grouped together. Sure, but since this was the first non-trivial patch I submitted, I tried to keep it non-intrusive and simple. > I would like to see if there's a clean way to combine this with phantom > function support (which I haven't find the code for yet). I'm willing to believe that if phantom functions haven't shown up and caused problems yet, it probably doesn't need fixing in a hurry. SR-IOV seems like a better and more common way for devices to use multiple lightweight functions. > >>> + for (i = pci_dev_dma_source_map; i->devfn; i++) { > >> The bug that Martin Oehrling pointed out in the ticket is still there. > > Ah, thank you; I had missed that that. Yes, it should be testing > i->vendor. You really get devices using the wrong slot as well as > function? Wow! > > Using multiple functions is at least anticipated in the PCI spec, > even if they do it wrong. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html