On Wed, Oct 06, 2021 at 05:49:29AM +0000, Kelvin.Cao@xxxxxxxxxxxxx wrote:
> On Tue, 2021-10-05 at 21:33 -0500, Bjorn Helgaas wrote:
> > On Wed, Oct 06, 2021 at 12:37:02AM +0000, Kelvin.Cao@xxxxxxxxxxxxx
> > wrote:
> > > On Tue, 2021-10-05 at 15:11 -0500, Bjorn Helgaas wrote:
> > > > On Mon, Oct 04, 2021 at 08:51:06PM +0000,
> > > > Kelvin.Cao@xxxxxxxxxxxxx
> > > > wrote:
> > > > > On Sat, 2021-10-02 at 10:11 -0500, Bjorn Helgaas wrote:
> > > > > > I *thought* the problem was that the PCIe Memory Read
> > > > > > failed and the Root Complex fabricated ~0 data to complete
> > > > > > the CPU read. But now I'm not sure, because it sounds
> > > > > > like it might be that the PCIe transaction succeeds, but
> > > > > > it reads data that hasn't been updated by the firmware,
> > > > > > i.e., it reads 'in progress' because firmware hasn't
> > > > > > updated it to 'done'.
> > > > >
> > > > > The original message was sort of misleading. After a
> > > > > firmware reset, CPU getting ~0 for the PCIe Memory Read
> > > > > doesn't explain the hang. In a MRPC execution (DMA MRPC
> > > > > mode), the MRPC status which is located in the host memory,
> > > > > gets initialized by the CPU and updated/finalized by the
> > > > > firmware. In the situation of a firmware reset, any MRPC
> > > > > initiated afterwards will not get the status updated by the
> > > > > firmware per the reason you pointed out above (or similar,
> > > > > to my understanding, firmware can no longer DMA data to host
> > > > > memory in such cases), therefore the MRPC execution will
> > > > > never end.
> > > >
> > > > I'm glad this makes sense to you, because it still doesn't to
> > > > me.
> > > >
> > > > check_access() does an MMIO read to something in BAR0. If
> > > > that read returns ~0, it means either the PCIe Memory Read was
> > > > successful and the Switchtec device supplied ~0 data (maybe
> > > > because firmware has not initialized that part of the BAR) or
> > > > the PCIe Memory Read failed and the root complex fabricated
> > > > the ~0 data.
> > > >
> > > > I'd like to know which one is happening so we can clarify the
> > > > commit log text about "MRPC command executions hang
> > > > indefinitely" and "host wil fail all GAS reads." It's not
> > > > clear whether these are PCIe protocol issues or
> > > > driver/firmware interaction issues.
> > >
> > > I think it's the latter case, the ~0 data was fabricated by the
> > > root complex, as the MMIO read in check_access() always returns
> > > ~0 until a reboot or a rescan happens.
> >
> > If the root complex fabricates ~0, that means a PCIe transaction
> > failed, i.e., the device didn't respond. Rescan only does config
> > reads and writes. Why should that cause the PCIe transactions to
> > magically start working?
>
> I took a closer look. What I observed was like this. A firmware
> reset cleared some CSR settings including the MSE and MBE bits and
> the Base Address Registers. With a rescan (removing the switch to
> which the management EP was binded from root port and rescan), the
> management EP was re-enumerated and driver was re-probed, so that
> the settings cleared by the firmware reset was properly setup again,
> therefore PCIe transactions start working.
I think what you just said is that
- the driver asked the firmware to reset the device
- the firmware did reset the device, which cleared Memory Space
Enable
- nothing restored the device config after the reset, so Memory
Space Enable remains cleared
- the driver does MMIO reads to figure out when the reset has
completed
- the device doesn't respond to the PCIe Memory Reads because Memory
Space Enable is cleared
- the root complex sees a timeout or error completion and fabricates
~0 data for the CPU read
- the driver sees ~0 data from the MMIO read and thinks the device
or firmware is hung
If that's all true, I think the patch is sort of a band-aid that
doesn't fix the problem at all but only makes the driver's response to
it marginally better. But the device is still unusable until a rescan
or reboot.
So I think we should drop this patch and do something to restore the
device state after the reset.
Bjorn
