lkml.org 
[lkml]   [2009]   [Jul]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 1/2] vga: implements VGA arbitration on Linux
    From
    On Thu, Jul 16, 2009 at 6:48 PM, Alan Cox<alan@lxorguk.ukuu.org.uk> 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-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2009-07-16 12:41    [W:0.034 / U:87.540 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site