On Sun, Aug 20, 2017 at 01:02:09AM +0530, Oza Oza wrote:
> On Sat, Aug 19, 2017 at 11:56 PM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > I think you should do something like this instead so you don't do the
> > MMIO read any more times than necessary:
> >
> > static u32 iproc_pcie_cfg_retry(void __iomem *cfg_data_p)
> > {
> > u32 data;
> > int timeout = CFG_RETRY_STATUS_TIMEOUT_US;
> >
> > data = readl(cfg_data_p);
> > while (data == CFG_RETRY_STATUS && timeout--) {
> > udelay(1);
> > data = readl(cfg_data_p);
> > }
> >
> > if (data == CFG_RETRY_STATUS)
> > data = 0xffffffff;
> > return data;
> > }
> >
> > static int iproc_pcie_config_read(...)
> > {
> > u32 data;
> >
> > ...
> > data = iproc_pcie_cfg_retry(cfg_data_p);
> > if (size <= 2)
> > *val = (data >> (8 * (where & 3))) & ((1 << (size * 8)) - 1);
> >
> > In the event of a timeout, this returns PCIBIOS_SUCCESSFUL and
> > 0xffffffff data. That's what most other platforms do, and most
> > callers of the PCI config accessors check for that data instead of
> > checking the return code to see whether the access was successful.
> >
> I see one problem with this.
> we have Samsung NVMe which exposes 64-bit IO BAR.
> 0xffff0001.
>
> and if you see __pci_read_base which does following in sequence.
>
> pci_read_config_dword(dev, pos, &l);
> pci_write_config_dword(dev, pos, l | mask);
> pci_read_config_dword(dev, pos, &sz);
> pci_write_config_dword(dev, pos, l);
>
> returning 0xffffffff would not be correct in that case.
> even if callers retry, they will end up in a loop if caller retries.
>
> hence I was returning 0xffff0001, it is upto the upper layer to treat
> it as data or not.
In your patch, I don't think the upper layer will ever see 0xffff0001.
iproc_pcie_config_read() only updates *data if iproc_pcie_cfg_retry()
returns PCIBIOS_SUCCESSFUL. And iproc_pcie_cfg_retry() only returns
PCIBIOS_SUCCESSFUL if it reads something other than 0xffff0001.
Even if you *did* return 0xffff0001 to upper layers, I think we'd have
a problem. Here's an example scenario. If we do a Function-Level
Reset, pcie_flr() starts the reset, then calls pci_flr_wait() to wait
until the device becomes ready again. pci_flr_wait() thinks that any
PCI_COMMAND value other than 0xffffffff means the device is ready.
If the device returns CRS status and you return 0xffff0001 when
pci_flr_wait() reads PCI_COMMAND, it thinks that means the device is
ready, but it's not.
> let me know if this sounds like a problem to you as well.
>
> so in my opinion returning 0xffffffff is not an option.
>
> > For example, pci_flr_wait() assumes that if a read of PCI_COMMAND
> > returns ~0, it's because the device isn't ready yet, and we should
> > wait and retry.
