Am 07.06.2017 um 01:10 schrieb Bjorn Helgaas:
[SNIP]
What if the driver did something like this:
pci_disable_decoding(dev, IORESOURCE_MEM);
pci_release_resource(dev, 2);
pci_resize_bar(dev, bar, size);
pci_assign_resources(dev);
pci_enable_decoding(dev, IORESOURCE_MEM);
That would require adding pci_enable/disable_decoding() to the driver
API, along with the requirement that the driver discard and remap
some resources after pci_enable_decoding(). I think
pci_enable_decoding() would look much like the existing
pci_enable_resources() except taking a resource type instead of a
bitmask.
This is pretty close to what we already do. I'm going to send out an
updated version of the patch in a minute.
One difference that I still have in this patch is
+ pci_read_config_word(adev->pdev, PCI_COMMAND, &cmd);
+ pci_write_config_word(adev->pdev, PCI_COMMAND,
+ cmd & ~PCI_COMMAND_MEMORY);
in the driver instead of "pci_disable_decoding(dev, IORESOURCE_MEM);"
I thought that introducing a new interface for this two lines would be
overkill, but if you find it cleaner to add the new interface I can
change that.
I would *prefer* if we released and reassigned all resources, because
then the core has complete flexibility to move things around, and it's
easy to document that pci_assign_resources() means you have to
reread/remap everything.
I've tried this and it doesn't work at all, surprisingly the I/O ports
turned out to be not the first problem I've ran into.
Releasing all resources means that we also try to release the
0xa000-0xbffff (VGA) and 0xc0000-0xdffff (VBIOS) ranges.
They can be reassigned, but somehow that seems to cause problems later on.
If the driver only releases specified BARs, the pci_assign_resources()
interface becomes "you need to reread/remap the BAR you resized, plus
any other BARs you released". It's a little more complicated to
describe and more dependent on previous driver actions.
How about the driver releases all resources it wants to move, including
the resized and reallocates them after the resize is done?
But releasing only the specified BAR will make your life easier and
help with the fact that multiple drivers might be using the same BAR
(I have to raise my eyebrows at that), so I think I'm OK with it. And
it would also side-step the "can't restore previous state" problem.
I agree that it is a bit more documentation work to describe how the
interface works, but it is clearly less problematic during runtime.
Please take a look at the new version of the patches and let me know
what you think.
Regards,
Christian.
It's an "interesting" asymmetry that pci_enable_device() turns on BAR
decoding but doesn't touch bus mastering, while pci_disable_device()
turns off bus mastering but doesn't touch BAR decoding.
Bjorn
