[PATCH] nvme-pci: Harden drive presence detect in nvme_dev_disable()

[PATCH] nvme-pci: Harden drive presence detect in nvme_dev_disable()

[Stefan Roese]

On our ZynqMP system we observe, that a NVMe drive that resets itself
while doing a firmware update causes a Kernel crash like this:

[ 67.720772] pcieport 0000:02:02.0: pciehp: Slot(2): Link Down
[ 67.720783] pcieport 0000:02:02.0: pciehp: Slot(2): Card not present
[ 67.720795] nvme 0000:04:00.0: PME# disabled
[ 67.720849] Internal error: synchronous external abort: 96000010 [#1] PREEMPT SMP
[ 67.720853] nwl-pcie fd0e0000.pcie: Slave error

Analysis: When nvme_dev_disable() is called because of this PCIe hotplug
event, pci_is_enabled() is still true. And accessing the NVMe drive
which is currently not available as it's in reboot process causes this
"synchronous external abort" on this ARM64 platform.

This patch adds the pci_device_is_present() check as well, which returns
false in this "Card not present" hot-plug case. With this change, the
NVMe driver does not try to access the NVMe registers any more and the
FW update finishes without any problems.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Keith Busch <kbusch@kernel.org>
Cc: Jens Axboe <axboe@fb.com>
Cc: Christoph Hellwig <hch@lst.de>
---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 97afeb898b25..c764a2a1d7f1 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2473,7 +2473,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 
 	mutex_lock(&dev->shutdown_lock);
-	if (pci_is_enabled(pdev)) {
+	if (pci_device_is_present(pdev) && pci_is_enabled(pdev)) {
 		u32 csts = readl(dev->bar + NVME_REG_CSTS);
 
 		if (dev->ctrl.state == NVME_CTRL_LIVE ||
-- 
2.36.0

Re: [PATCH] nvme-pci: Harden drive presence detect in nvme_dev_disable()

[Christoph Hellwig]

Thanks,

applied to nvme-5.19.

Re: [PATCH] nvme-pci: Harden drive presence detect in nvme_dev_disable()

[Max Gurtovoy]

On 5/6/2022 1:15 PM, Stefan Roese wrote:
> On our ZynqMP system we observe, that a NVMe drive that resets itself
> while doing a firmware update causes a Kernel crash like this:
>
> [ 67.720772] pcieport 0000:02:02.0: pciehp: Slot(2): Link Down
> [ 67.720783] pcieport 0000:02:02.0: pciehp: Slot(2): Card not present
> [ 67.720795] nvme 0000:04:00.0: PME# disabled
> [ 67.720849] Internal error: synchronous external abort: 96000010 [#1] PREEMPT SMP
> [ 67.720853] nwl-pcie fd0e0000.pcie: Slave error
>
> Analysis: When nvme_dev_disable() is called because of this PCIe hotplug
> event, pci_is_enabled() is still true. And accessing the NVMe drive
> which is currently not available as it's in reboot process causes this
> "synchronous external abort" on this ARM64 platform.
>
> This patch adds the pci_device_is_present() check as well, which returns
> false in this "Card not present" hot-plug case. With this change, the
> NVMe driver does not try to access the NVMe registers any more and the
> FW update finishes without any problems.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Keith Busch <kbusch@kernel.org>
> Cc: Jens Axboe <axboe@fb.com>
> Cc: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/nvme/host/pci.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 97afeb898b25..c764a2a1d7f1 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2473,7 +2473,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>   	struct pci_dev *pdev = to_pci_dev(dev->dev);
>   
>   	mutex_lock(&dev->shutdown_lock);
> -	if (pci_is_enabled(pdev)) {
> +	if (pci_device_is_present(pdev) && pci_is_enabled(pdev)) {

sound weird to me that pci device can be enabled but not present.

Also why we get hot unplug event for FW upgrade ?

is this the correct behavior ?

>   		u32 csts = readl(dev->bar + NVME_REG_CSTS);
>   
>   		if (dev->ctrl.state == NVME_CTRL_LIVE ||

Re: [PATCH] nvme-pci: Harden drive presence detect in nvme_dev_disable()

[Keith Busch]

On Thu, May 12, 2022 at 04:03:39PM +0300, Max Gurtovoy wrote:
> On 5/6/2022 1:15 PM, Stefan Roese wrote:
> > On our ZynqMP system we observe, that a NVMe drive that resets itself
> > while doing a firmware update causes a Kernel crash like this:
> > 
> > [ 67.720772] pcieport 0000:02:02.0: pciehp: Slot(2): Link Down
> > [ 67.720783] pcieport 0000:02:02.0: pciehp: Slot(2): Card not present
> > [ 67.720795] nvme 0000:04:00.0: PME# disabled
> > [ 67.720849] Internal error: synchronous external abort: 96000010 [#1] PREEMPT SMP
> > [ 67.720853] nwl-pcie fd0e0000.pcie: Slave error
> > 
> > Analysis: When nvme_dev_disable() is called because of this PCIe hotplug
> > event, pci_is_enabled() is still true. And accessing the NVMe drive
> > which is currently not available as it's in reboot process causes this
> > "synchronous external abort" on this ARM64 platform.
> > 
> > This patch adds the pci_device_is_present() check as well, which returns
> > false in this "Card not present" hot-plug case. With this change, the
> > NVMe driver does not try to access the NVMe registers any more and the
> > FW update finishes without any problems.
> > 
> > Signed-off-by: Stefan Roese <sr@denx.de>
> > Cc: Keith Busch <kbusch@kernel.org>
> > Cc: Jens Axboe <axboe@fb.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > ---
> >   drivers/nvme/host/pci.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> > index 97afeb898b25..c764a2a1d7f1 100644
> > --- a/drivers/nvme/host/pci.c
> > +++ b/drivers/nvme/host/pci.c
> > @@ -2473,7 +2473,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
> >   	struct pci_dev *pdev = to_pci_dev(dev->dev);
> >   	mutex_lock(&dev->shutdown_lock);
> > -	if (pci_is_enabled(pdev)) {
> > +	if (pci_device_is_present(pdev) && pci_is_enabled(pdev)) {
> 
> sound weird to me that pci device can be enabled but not present.

pci_is_enabled() just checks a driver reference count. It doesn't actually
query the physical status of the device's link it references, but
pci_device_is_present() will do that.
 
> Also why we get hot unplug event for FW upgrade ?

It might happen if the device requires a Subsystem Reset to activate the new
firmware.
 
> is this the correct behavior ?

I don't know if the "synchronous external abort" is normal, or how severe of a
problem it indicates, but we normally expect accessing registers on an
inaccessible device to be mostly harmless.

> >   		u32 csts = readl(dev->bar + NVME_REG_CSTS);
> >   		if (dev->ctrl.state == NVME_CTRL_LIVE ||

Re: [PATCH] nvme-pci: Harden drive presence detect in nvme_dev_disable()

[Max Gurtovoy]

On 5/12/2022 5:33 PM, Keith Busch wrote:
> On Thu, May 12, 2022 at 04:03:39PM +0300, Max Gurtovoy wrote:
>> On 5/6/2022 1:15 PM, Stefan Roese wrote:
>>> On our ZynqMP system we observe, that a NVMe drive that resets itself
>>> while doing a firmware update causes a Kernel crash like this:
>>>
>>> [ 67.720772] pcieport 0000:02:02.0: pciehp: Slot(2): Link Down
>>> [ 67.720783] pcieport 0000:02:02.0: pciehp: Slot(2): Card not present
>>> [ 67.720795] nvme 0000:04:00.0: PME# disabled
>>> [ 67.720849] Internal error: synchronous external abort: 96000010 [#1] PREEMPT SMP
>>> [ 67.720853] nwl-pcie fd0e0000.pcie: Slave error
>>>
>>> Analysis: When nvme_dev_disable() is called because of this PCIe hotplug
>>> event, pci_is_enabled() is still true. And accessing the NVMe drive
>>> which is currently not available as it's in reboot process causes this
>>> "synchronous external abort" on this ARM64 platform.
>>>
>>> This patch adds the pci_device_is_present() check as well, which returns
>>> false in this "Card not present" hot-plug case. With this change, the
>>> NVMe driver does not try to access the NVMe registers any more and the
>>> FW update finishes without any problems.
>>>
>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>> Cc: Keith Busch <kbusch@kernel.org>
>>> Cc: Jens Axboe <axboe@fb.com>
>>> Cc: Christoph Hellwig <hch@lst.de>
>>> ---
>>>    drivers/nvme/host/pci.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>>> index 97afeb898b25..c764a2a1d7f1 100644
>>> --- a/drivers/nvme/host/pci.c
>>> +++ b/drivers/nvme/host/pci.c
>>> @@ -2473,7 +2473,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>>>    	struct pci_dev *pdev = to_pci_dev(dev->dev);
>>>    	mutex_lock(&dev->shutdown_lock);
>>> -	if (pci_is_enabled(pdev)) {
>>> +	if (pci_device_is_present(pdev) && pci_is_enabled(pdev)) {
>> sound weird to me that pci device can be enabled but not present.
> pci_is_enabled() just checks a driver reference count. It doesn't actually
> query the physical status of the device's link it references, but
> pci_device_is_present() will do that.
>   
>> Also why we get hot unplug event for FW upgrade ?
> It might happen if the device requires a Subsystem Reset to activate the new
> firmware.

but isn't the reset something that the admin should do ?

some power-cycle or cold-reboot or other reset.

In the reported case the device resets itself. I'm not sure it's expected.

BTW, for sure the fix is good for the hot unplug case but FW reset 
shouldn't cause this scenario IMO.

>   
>> is this the correct behavior ?
> I don't know if the "synchronous external abort" is normal, or how severe of a
> problem it indicates, but we normally expect accessing registers on an
> inaccessible device to be mostly harmless.
>
>>>    		u32 csts = readl(dev->bar + NVME_REG_CSTS);
>>>    		if (dev->ctrl.state == NVME_CTRL_LIVE ||

Re: [PATCH] nvme-pci: Harden drive presence detect in nvme_dev_disable()

[Keith Busch]

On Fri, May 13, 2022 at 04:07:18AM +0300, Max Gurtovoy wrote:
> On 5/12/2022 5:33 PM, Keith Busch wrote:
> > It might happen if the device requires a Subsystem Reset to activate the new
> > firmware.
> 
> but isn't the reset something that the admin should do ?
> 
> some power-cycle or cold-reboot or other reset.
> 
> In the reported case the device resets itself. I'm not sure it's expected.

I'm not familiar with this device or the procedure used to update the firmware,
but I'm aware many vendors still provide their firmware bundled within their
own tooling, so simply running that utility could do all sorts of things
including a device link reset.

If the device is resetting itself without a user or driver app initiating it,
though, that would be bad behavior outside the spec. But if this harmless patch
improves interop regardless of device behavior, then it should be okay to
include.

> BTW, for sure the fix is good for the hot unplug case but FW reset shouldn't
> cause this scenario IMO.

Yeah, the patch is fine as far as I'm concerned, though it is impossible to
prevent all register reads executing concurrently with any random link-down
event. The commit log indicates reading registers during such an event causes a
kernel crash, so a more complete fix probably needs to come from the platform
level.

Re: [PATCH] nvme-pci: Harden drive presence detect in nvme_dev_disable()

[Stefan Roese]

On 13.05.22 04:00, Keith Busch wrote:
> On Fri, May 13, 2022 at 04:07:18AM +0300, Max Gurtovoy wrote:
>> On 5/12/2022 5:33 PM, Keith Busch wrote:
>>> It might happen if the device requires a Subsystem Reset to activate the new
>>> firmware.
>>
>> but isn't the reset something that the admin should do ?
>>
>> some power-cycle or cold-reboot or other reset.
>>
>> In the reported case the device resets itself. I'm not sure it's expected.
> 
> I'm not familiar with this device or the procedure used to update the firmware,
> but I'm aware many vendors still provide their firmware bundled within their
> own tooling, so simply running that utility could do all sorts of things
> including a device link reset.

We are using standard nvme-cli tools for the FW update process, like:
nvme fw-download ...
nvme fw-commit ...

> If the device is resetting itself without a user or driver app initiating it,
> though, that would be bad behavior outside the spec. But if this harmless patch
> improves interop regardless of device behavior, then it should be okay to
> include.

Yes. The device resetting itself seems to be outside of the spec. Still
we somehow need to handle this situation.

>> BTW, for sure the fix is good for the hot unplug case but FW reset shouldn't
>> cause this scenario IMO.
> 
> Yeah, the patch is fine as far as I'm concerned, though it is impossible to
> prevent all register reads executing concurrently with any random link-down
> event. The commit log indicates reading registers during such an event causes a
> kernel crash, so a more complete fix probably needs to come from the platform
> level.

Agreed. Even with this patch applied, we have still seen Kernel crashes
after the "nvme fw-commit", when the drive reset'ed itself twice (IIRC).
We were able to work around these issues by removing the nvme modules
exactly when the PCIe device was removed (via HP event) for now.

Thanks,
Stefan