Re: [PATCH v2 2/2] PCI: Don't disable PF's memory decoding when enabling SRIOV

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Mon, Oct 24, 2016 at 10:51:13PM -0500, Bjorn Helgaas wrote:
>On Tue, Oct 25, 2016 at 12:47:28PM +1100, Gavin Shan wrote:
>> On Mon, Oct 24, 2016 at 09:03:16AM -0500, Bjorn Helgaas wrote:
>> >On Mon, Oct 24, 2016 at 10:28:02AM +1100, Gavin Shan wrote:
>> >> On Fri, Oct 21, 2016 at 11:50:34AM -0500, Bjorn Helgaas wrote:
>> >> >On Fri, Sep 30, 2016 at 09:47:50AM +1000, Gavin Shan wrote:

.../...

>
>That specific case (pci_enable_device() followed by
>pci_update_resource()) should *not* work.  pci_enable_device() is
>normally called by a driver's .probe() method, and after we call a
>.probe() method, the PCI core shouldn't touch the device at all
>because there's no means of mutual exclusion between the driver and
>the PCI core.
>
>I think pci_update_resource() should only be called in situations
>where the caller already knows that nobody is using the device.  For
>regular PCI BARs, that doesn't necessarily mean PCI_COMMAND_MEMORY is
>turned off, because firmware leaves PCI_COMMAND_MEMORY enabled for
>many devices, even though nobody is using them.
>
>Anyway, I think that's a project for another day.  That's too much to
>tackle for the limited problem you're trying to solve.
>

Bjorn, it's all about discussion. Please take your time and reply when
you have bandwidth.

Well, some drivers break the order and expects the relaxed order to work.
One example is drivers/char/agp/efficeon-agp.c::agp_efficeon_probe(). I
didn't check all usage cases.

I think it's hard for user, who calls pci_update_resource(), to know
that nobody is using the devcie (limited to memory BARs as we concern).
The memory write is usually non-posted transaction and it can be on
the way to target device when pci_update_resource() is called. So which
one tranaction will be completed first (disabling memory decoding and
memory write). I guess it can happen even with the mutual exclusion,
especially on SMP system. Yes, the situation is worse without the
synchronization.

.../...

>> 
>> Yeah, it would be the solution to have. If you agree, I will post
>> updatd version according to this: Clearing PCI_SRIOV_CTRL_MSE when
>> updating IOV BARs. The bit won't be touched if pdev->mmio_always_on
>> is true.
>
>I think you should ignore pdev->mmio_always_on for IOV BARs.
>mmio_always_on is basically a workaround for devices that either don't
>follow the spec or where we didn't completely understand the problem.
>I don't think there's any reason to set mmio_always_on for SR-IOV
>devices.
>

Agree, thanks for the comments again. I will post updated version
shortly.

Thanks,
Gavin