[PATCH 0/2] nvme-pci: Log failures to probe

[PATCH 0/2] nvme-pci: Log failures to probe

[Gerd Bayer]

Hi,

occasionally, an NVMe drive fails to reply to MMIO reads despite the PCI
link being operational. In these situations (obviously not on the
boot-device :) it is preferred to have this reported in the kernel log
instead of deducing that something is wrong by comparing expectated vs.
observed number of nvme devices.

 - 1/2 adds a clear indication that the device driver failed to probe an
   NVMe drive and will not register a nvme device.
 - 2/2 adds a debug log that allows to further zoom in on the exact
   failure

I've split this into two patches, since 2/2 might be less important wrt
to back-ports, etc.

Thanks,
Gerd

Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
---
Gerd Bayer (2):
      nvme-pci: Print error message on failure in nvme_probe
      nvme-pci: Add debug message on fail to read CSTS

 drivers/nvme/host/pci.c | 3 +++
 1 file changed, 3 insertions(+)
---
base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada
change-id: 20251015-nvme_probefail-3976120a4719

Best regards,
-- 
Gerd Bayer <gbayer@linux.ibm.com>

[PATCH 1/2] nvme-pci: Print error message on failure in nvme_probe

[Gerd Bayer]

Add a new error message like
nvme nvme0: probe failed on 2004:00:00.0 (result: -19)
that makes failures to probe visible in the kernel log.

Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
---
 drivers/nvme/host/pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index c916176bd9f058b49e6e6768675711df52b15765..7544c4bac2c4a230d32cf729abb9e94bf93a921f 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3519,6 +3519,8 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 out_dev_unmap:
 	nvme_dev_unmap(dev);
 out_uninit_ctrl:
+	dev_err(dev->ctrl.device, "probe failed on %s (result: %d)\n",
+		dev_name(&pdev->dev), result);
 	nvme_uninit_ctrl(&dev->ctrl);
 out_put_ctrl:
 	nvme_put_ctrl(&dev->ctrl);

-- 
2.48.1

[PATCH 2/2] nvme-pci: Add debug message on fail to read CSTS

[Gerd Bayer]

Add a debug log spelling out that reading the CSTS register failed - to
distinguish this from other reasons for ENODEV.

Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
---
 drivers/nvme/host/pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 7544c4bac2c4a230d32cf729abb9e94bf93a921f..c3b0a9d56ac1f32f67ce0890becd6425eb80ebad 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2906,6 +2906,7 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 	pci_set_master(pdev);
 
 	if (readl(dev->bar + NVME_REG_CSTS) == -1) {
+		dev_dbg(dev->ctrl.device, "Read CSTS register failed");
 		result = -ENODEV;
 		goto disable;
 	}

-- 
2.48.1

Re: [PATCH 2/2] nvme-pci: Add debug message on fail to read CSTS

[Chaitanya Kulkarni]

On 10/20/25 08:29, Gerd Bayer wrote:
> Add a debug log spelling out that reading the CSTS register failed - to
> distinguish this from other reasons for ENODEV.
>
> Signed-off-by: Gerd Bayer<gbayer@linux.ibm.com>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 1/2] nvme-pci: Print error message on failure in nvme_probe

[Chaitanya Kulkarni]

On 10/20/25 08:29, Gerd Bayer wrote:
> Add a new error message like
> nvme nvme0: probe failed on 2004:00:00.0 (result: -19)
> that makes failures to probe visible in the kernel log.
>
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> ---
>   drivers/nvme/host/pci.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index c916176bd9f058b49e6e6768675711df52b15765..7544c4bac2c4a230d32cf729abb9e94bf93a921f 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -3519,6 +3519,8 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   out_dev_unmap:
>   	nvme_dev_unmap(dev);
>   out_uninit_ctrl:
> +	dev_err(dev->ctrl.device, "probe failed on %s (result: %d)\n",
> +		dev_name(&pdev->dev), result);

is it really have to be dev_err ? or dev_dbg would work ?


>   	nvme_uninit_ctrl(&dev->ctrl);
>   out_put_ctrl:
>   	nvme_put_ctrl(&dev->ctrl);
>

-ck

Re: [PATCH 1/2] nvme-pci: Print error message on failure in nvme_probe

[Gerd Bayer]

On Mon, 2025-10-20 at 23:35 +0000, Chaitanya Kulkarni wrote:
> On 10/20/25 08:29, Gerd Bayer wrote:
> > Add a new error message like
> > nvme nvme0: probe failed on 2004:00:00.0 (result: -19)
> > that makes failures to probe visible in the kernel log.
> > 
> > Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> > ---
> >   drivers/nvme/host/pci.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> > index
> > c916176bd9f058b49e6e6768675711df52b15765..7544c4bac2c4a230d32cf729a
> > bb9e94bf93a921f 100644
> > --- a/drivers/nvme/host/pci.c
> > +++ b/drivers/nvme/host/pci.c
> > @@ -3519,6 +3519,8 @@ static int nvme_probe(struct pci_dev *pdev,
> > const struct pci_device_id *id)
> >   out_dev_unmap:
> >   	nvme_dev_unmap(dev);
> >   out_uninit_ctrl:
> > +	dev_err(dev->ctrl.device, "probe failed on %s (result:
> > %d)\n",
> > +		dev_name(&pdev->dev), result);
> 
> is it really have to be dev_err ? or dev_dbg would work ?

I've almost overlooked this question, since I'm not subscribed to the
list - found it on lore.

dev_dbg is too weak IMHO. I assmume most installations don't log debug-
level and would require a repro with dynamic_debug turned on. My intent
was to highlight the abnormal behavior even in default kernel configs.
I was following the example in mlx5_core that produces error-level
messages when probe fails.

Thanks,
Gerd

> 
> 
> >   	nvme_uninit_ctrl(&dev->ctrl);
> >   out_put_ctrl:
> >   	nvme_put_ctrl(&dev->ctrl);
> > 
> 
> -ck
> 

Re: [PATCH 1/2] nvme-pci: Print error message on failure in nvme_probe

[Wilfred Mallawa]

On Mon, 2025-10-20 at 17:29 +0200, Gerd Bayer wrote:
> Add a new error message like
> nvme nvme0: probe failed on 2004:00:00.0 (result: -19)
> that makes failures to probe visible in the kernel log.
> 
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> ---
>  drivers/nvme/host/pci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index
> c916176bd9f058b49e6e6768675711df52b15765..7544c4bac2c4a230d32cf729abb
> 9e94bf93a921f 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -3519,6 +3519,8 @@ static int nvme_probe(struct pci_dev *pdev,
> const struct pci_device_id *id)
>  out_dev_unmap:
>  	nvme_dev_unmap(dev);
>  out_uninit_ctrl:
> +	dev_err(dev->ctrl.device, "probe failed on %s (result:
> %d)\n",
> +		dev_name(&pdev->dev), result);
>  	nvme_uninit_ctrl(&dev->ctrl);
>  out_put_ctrl:
>  	nvme_put_ctrl(&dev->ctrl);
Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>

Wilfred

Re: [PATCH 2/2] nvme-pci: Add debug message on fail to read CSTS

[Wilfred Mallawa]

On Mon, 2025-10-20 at 17:29 +0200, Gerd Bayer wrote:
> Add a debug log spelling out that reading the CSTS register failed -
> to
> distinguish this from other reasons for ENODEV.
> 
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> ---
>  drivers/nvme/host/pci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index
> 7544c4bac2c4a230d32cf729abb9e94bf93a921f..c3b0a9d56ac1f32f67ce0890bec
> d6425eb80ebad 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2906,6 +2906,7 @@ static int nvme_pci_enable(struct nvme_dev
> *dev)
>  	pci_set_master(pdev);
>  
>  	if (readl(dev->bar + NVME_REG_CSTS) == -1) {
> +		dev_dbg(dev->ctrl.device, "Read CSTS register
> failed");
>  		result = -ENODEV;
>  		goto disable;
>  	}
Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>

Wilfred

Re: [PATCH 1/2] nvme-pci: Print error message on failure in nvme_probe

[Christoph Hellwig]

On Mon, Oct 20, 2025 at 05:29:07PM +0200, Gerd Bayer wrote:
> Add a new error message like
> nvme nvme0: probe failed on 2004:00:00.0 (result: -19)
> that makes failures to probe visible in the kernel log.

Is that really a thing drivers are expected to do?  If it is generally
usefull I'd expect it to be in the driver core.

Also for nvme most "interesting" errors happen in the reset workqueue
and not in ->probe itself.

Re: [PATCH 2/2] nvme-pci: Add debug message on fail to read CSTS

[Christoph Hellwig]

On Mon, Oct 20, 2025 at 05:29:08PM +0200, Gerd Bayer wrote:
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 7544c4bac2c4a230d32cf729abb9e94bf93a921f..c3b0a9d56ac1f32f67ce0890becd6425eb80ebad 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2906,6 +2906,7 @@ static int nvme_pci_enable(struct nvme_dev *dev)
>  	pci_set_master(pdev);
>  
>  	if (readl(dev->bar + NVME_REG_CSTS) == -1) {
> +		dev_dbg(dev->ctrl.device, "Read CSTS register failed");

Should this say "reading" instead of "read"?  Also most (but not
all) nvme errors don't start with capitalized words, like most drivers
as far as I can tell.

Re: [PATCH 1/2] nvme-pci: Print error message on failure in nvme_probe

[Greg Kroah-Hartman]

On Wed, Oct 22, 2025 at 08:26:34AM +0200, Christoph Hellwig wrote:
> On Mon, Oct 20, 2025 at 05:29:07PM +0200, Gerd Bayer wrote:
> > Add a new error message like
> > nvme nvme0: probe failed on 2004:00:00.0 (result: -19)
> > that makes failures to probe visible in the kernel log.
> 
> Is that really a thing drivers are expected to do?  If it is generally
> usefull I'd expect it to be in the driver core.

We have that already, dev_err_probe(), no need to create
yet-another-version of that.

thanks,

greg k-h

Re: [PATCH 1/2] nvme-pci: Print error message on failure in nvme_probe

[Gerd Bayer]

On Wed, 2025-10-22 at 09:48 +0200, Greg Kroah-Hartman wrote:
> On Wed, Oct 22, 2025 at 08:26:34AM +0200, Christoph Hellwig wrote:
> > On Mon, Oct 20, 2025 at 05:29:07PM +0200, Gerd Bayer wrote:
> > > Add a new error message like
> > > nvme nvme0: probe failed on 2004:00:00.0 (result: -19)
> > > that makes failures to probe visible in the kernel log.
> > 
> > Is that really a thing drivers are expected to do?  If it is generally
> > usefull I'd expect it to be in the driver core.
> 
> We have that already, dev_err_probe(), no need to create
> yet-another-version of that.

So I take this as an implict answer to the question, if drivers are
expected to do this or the driver core: Drivers.


Before learning about dev_err_probe() I was sampling a few drivers'
probe functions and got inconclusive results regarding their verbosity
regarding logging errors in probe. Interestingly, none of my samples
used dev_err_probe()...

While for most drivers it may be obvious that in most sytem
configurations it will not go unnoticed that e.g. the graphics
controller failed to probe - there may be other
components/configurations and a centralized error reporting e.g. in
local_pci_probe() would have its benefits. But then with so many
drivers already using dev_err_probe(), we don't want to report this
twice, now.

Thus, I'm going to convert my patch over for a v2.

Thanks,
Gerd

Re: [PATCH 2/2] nvme-pci: Add debug message on fail to read CSTS

[Gerd Bayer]

On Wed, 2025-10-22 at 08:27 +0200, Christoph Hellwig wrote:
> On Mon, Oct 20, 2025 at 05:29:08PM +0200, Gerd Bayer wrote:
> > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> > index 7544c4bac2c4a230d32cf729abb9e94bf93a921f..c3b0a9d56ac1f32f67ce0890becd6425eb80ebad 100644
> > --- a/drivers/nvme/host/pci.c
> > +++ b/drivers/nvme/host/pci.c
> > @@ -2906,6 +2906,7 @@ static int nvme_pci_enable(struct nvme_dev *dev)
> >  	pci_set_master(pdev);
> >  
> >  	if (readl(dev->bar + NVME_REG_CSTS) == -1) {
> > +		dev_dbg(dev->ctrl.device, "Read CSTS register failed");
> 
> Should this say "reading" instead of "read"?  Also most (but not
> all) nvme errors don't start with capitalized words, like most drivers
> as far as I can tell.

I'll change that in a v2.

Thanks,
Gerd