Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 7 Oct 2021 16:23:17 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Kelvin.Cao@microchip.com
Cc:     kurt.schwemmer@microsemi.com, bhelgaas@google.com,
        kelvincao@outlook.com, logang@deltatee.com,
        linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
Subject: Re: [PATCH 1/5] PCI/switchtec: Error out MRPC execution when no GAS
 access
Message-ID: <20211007212317.GA1268429@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <54263d552d05f2fae706e83aa4c2b31b1983b0e2.camel@microchip.com>
Precedence: bulk

On Wed, Oct 06, 2021 at 09:27:49PM +0000, Kelvin.Cao@microchip.com wrote:
> On Wed, 2021-10-06 at 15:20 -0500, Bjorn Helgaas wrote:
> > On Wed, Oct 06, 2021 at 07:00:55PM +0000, Kelvin.Cao@microchip.com
> > wrote:

> > So wait, you mean you just intentionally ask the firmware to
> > reset, knowing that the device will be unusable until the user
> > reboots or does a manual rescan?  And the way to improve this is
> > for the driver to report an error to the user instead of hanging?
> > I *guess* that might be some sort of improvement, but seems like a
> > pretty small one.
> 
> Yes, however, I believe it's something our users really like to
> have...  With this, they can do their user space
> programming/scripting more easily in a synchronous fashion.
> 
> > >   - The firwmare crashes and doesn't respond, which normally is
> > >   the reason for users to issue a firmware reset command to try
> > >   to recover it via either the driver or a sideband interface.
> > >   The firmware may not be able to recover by a reset in some
> > >   extream situations like hardware errors, so that an error
> > >   return is probably all the users can get before another level
> > >   of recovery happens.
> > > 
> > > So I'd think this patch is still making the driver better in
> > > some way.

OK.  I still think the fact that all these different mechanisms can
reset the device behind your back and make the switch and anything on
the other side of it just stop working is ..., well, let's just say
it's quite surprising to me.

Well, at least this isn't quite so much a mystery anymore and maybe we
can improve the commit log.  E.g., maybe something like this:

  A firmware hard reset may be initiated by various mechanisms
  including a UART interface, TWI sideband interface from BMC, MRPC
  command from userspace, etc.  The switchtec management driver is
  unaware of these resets.

  The reset clears PCI state including the BARs and Memory Space
  Enable bits, so the device no longer responds to the MMIO accesses
  the driver uses to operate it.

  MMIO reads to the device will fail with a PCIe error.  When the root
  complex handles that error, it typically fabricates ~0 data to
  complete the CPU read.

  Check for this sort of error by reading the device ID from MMIO
  space.  This ID can never be ~0, so if we see that value, it
  probably means the PCIe Memory Read failed and we should return an
  error indication to the application using the switchtec driver.