On Thu, Jul 16, 2009 at 6:48 PM, Alan Cox<alan@xxxxxxxxxxxxxxxxxxx> wrote: >> >> + pci_read_config_word(pdev, PCI_COMMAND, &cmd); >> >> + if (rsrc & (VGA_RSRC_LEGACY_IO | VGA_RSRC_NORMAL_IO)) >> >> + cmd |= PCI_COMMAND_IO; >> >> + if (rsrc & (VGA_RSRC_LEGACY_MEM | VGA_RSRC_NORMAL_MEM)) >> >> + cmd |= PCI_COMMAND_MEMORY; >> >> + pci_write_config_word(pdev, PCI_COMMAND, cmd); >> > >> > Locking question - what locks this lot against hotplug also touching >> > bridge settings ? >> >> well here we just bang on device config space registers which means we >> can probably >> race against lots of other things that rmw the PCI_COMMAND not just hotplug. >> >> Perhaps we need some sort per device PCI command space lock, >> granted this still means we race against anyone directly hacking it >> behind our backs. > > I suspect the right thing to do is to move that function into the > drivers/pci code and lock it properly there. That would keep all the > locking detail internal and private (and someone else's problem ;)) I'll have a look, Jesse any ideas? the hotplug bridge bashing looks unfun. I noticed a fair few drivers seem to bash these things, and really pci_enable_device could possible race with hotplug. > >> >> + pdev = vga_default_device(); >> > >> > What if the BIOS provided device was hot unplugged ? >> >> we just use the pdev as a cookie, if it was hot unplugged we'll >> have gotten a callback to remove it from the VGA device list >> and the lookup which happens 5 lines later inside the spinlock >> will fail. > > What if I inserted a new device - the allocator might give me a new > device with the same pointer if its reusing the same slab entry for that > size. Unlikely but given a zillion boxes this starts to occur 8( A zillion boxes with hotplug VGA devices, I wish :-), but I'll fix it up to do the pci_dev_get/puts. > >> >> + /* We have a conflict, we wait until somebody kicks the >> >> + * work queue. Currently we have one work queue that we >> > >> > If two drivers own half the resources and both are waiting for the rest >> > what handles the deadlock >> > > > Not commented on - but a serious question would be "do we actually care > enough or are there really devices with just I/O and just vga memory > access used ?" > Oops yes I want to avoid doing anything except VGA resources on/off I don't think the granularity matters, will talk to BenH tomorrow see what his original idea was. >> > PCI device refcounting ? >> >> Again its just used a cookie for a later lookup in our vgadev array, >> its gone away it'll have been removed from the array, >> >> We could use pci_dev_get/pci_dev_put I suppose but its only used as >> a cookie so far. > > Your cookie validity is suspect I fear. Also holding the device reference > means you stop that exact set of resources being reissued too early and > you (or clients) scribbling on them through unfortunate timing. So I > think you actually do need to grab references properly, Doesn't make the > code much more complex that I can see. I'll fix it seems like a good plan just in case. Dave. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
