[RFC PATCH] nvme: replace lazy sgl warn with informational message

[RFC PATCH] nvme: replace lazy sgl warn with informational message

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Instead of warning about the device's SGL capabilities the first time we
consider using them with a passthrough command, print out what the
device is capable of during initialization. While we're at it, print
other interesting capabilities that may be useful to know when you just
have a dmesg. The ouput format was inspired by other existing subsystems
like pci.

And example of the new message when used with a default qemu nvme
emulated device:

  nvme nvme0: sgl+ meta-sgl+ sed- vwc+ apst- dsm+ wz+

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c  | 20 ++++++++++++++++++++
 drivers/nvme/host/ioctl.c | 11 ++---------
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 2bcd9f710cb65..7535a9b9d2afa 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3440,6 +3440,22 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 	return ret;
 }
 
+#define ENABLED(cond) ((cond) ? '+' : '-')
+
+static void nvme_ctrl_caps(struct nvme_ctrl *ctrl)
+{
+	dev_info(ctrl->device,
+		"sgl%c meta-sgl%c sed%c vwc%c apst%c dsm%c wz%c\n",
+		ENABLED(nvme_ctrl_sgl_supported(ctrl)),
+		ENABLED(nvme_ctrl_meta_sgl_supported(ctrl)),
+		ENABLED(!!ctrl->opal_dev),
+		ENABLED(ctrl->vwc & NVME_CTRL_VWC_PRESENT),
+		ENABLED(ctrl->apst_enabled),
+		ENABLED(ctrl->oncs & NVME_CTRL_ONCS_DSM),
+		ENABLED(ctrl->oncs & NVME_CTRL_ONCS_WRITE_ZEROES)
+	);
+}
+
 /*
  * Initialize the cached copies of the Identify data and various controller
  * register in our nvme_ctrl structure.  This should be called as soon as
@@ -3489,6 +3505,10 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl, bool was_suspended)
 	}
 
 	clear_bit(NVME_CTRL_DIRTY_CAPABILITY, &ctrl->flags);
+
+	if (!ctrl->identified)
+		nvme_ctrl_caps(ctrl);
+
 	ctrl->identified = true;
 
 	nvme_start_keep_alive(ctrl);
diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index e8930146847af..2b5b650443229 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -125,15 +125,8 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 	struct bio *bio = NULL;
 	int ret;
 
-	if (!nvme_ctrl_sgl_supported(ctrl))
-		dev_warn_once(ctrl->device, "using unchecked data buffer\n");
-	if (has_metadata) {
-		if (!supports_metadata)
-			return -EINVAL;
-		if (!nvme_ctrl_meta_sgl_supported(ctrl))
-			dev_warn_once(ctrl->device,
-				      "using unchecked metadata buffer\n");
-	}
+	if (has_metadata && !supports_metadata)
+		return -EINVAL;
 
 	if (ioucmd && (ioucmd->flags & IORING_URING_CMD_FIXED)) {
 		struct iov_iter iter;
-- 
2.43.5

Re: [RFC PATCH] nvme: replace lazy sgl warn with informational message

[Christoph Hellwig]

On Thu, Jan 30, 2025 at 09:51:31AM -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Instead of warning about the device's SGL capabilities the first time we
> consider using them with a passthrough command, print out what the
> device is capable of during initialization. While we're at it, print
> other interesting capabilities that may be useful to know when you just
> have a dmesg. The ouput format was inspired by other existing subsystems
> like pci.
> 
> And example of the new message when used with a default qemu nvme
> emulated device:
> 
>   nvme nvme0: sgl+ meta-sgl+ sed- vwc+ apst- dsm+ wz+

Printing out the capabilities sounds really useful.  But I don't see
how this is useful for replacing the warning about the unchecked
passthrough?

Re: [RFC PATCH] nvme: replace lazy sgl warn with informational message

[Sagi Grimberg]

On 31/01/2025 9:48, Christoph Hellwig wrote:
> On Thu, Jan 30, 2025 at 09:51:31AM -0800, Keith Busch wrote:
>> From: Keith Busch <kbusch@kernel.org>
>>
>> Instead of warning about the device's SGL capabilities the first time we
>> consider using them with a passthrough command, print out what the
>> device is capable of during initialization. While we're at it, print
>> other interesting capabilities that may be useful to know when you just
>> have a dmesg. The ouput format was inspired by other existing subsystems
>> like pci.
>>
>> And example of the new message when used with a default qemu nvme
>> emulated device:
>>
>>    nvme nvme0: sgl+ meta-sgl+ sed- vwc+ apst- dsm+ wz+
> Printing out the capabilities sounds really useful.  But I don't see
> how this is useful for replacing the warning about the unchecked
> passthrough?
>

I agree, I'd keep the warn_once messages.

Re: [RFC PATCH] nvme: replace lazy sgl warn with informational message

[Keith Busch]

On Fri, Jan 31, 2025 at 10:06:24AM +0200, Sagi Grimberg wrote:
> On 31/01/2025 9:48, Christoph Hellwig wrote:
> > On Thu, Jan 30, 2025 at 09:51:31AM -0800, Keith Busch wrote:
> > > From: Keith Busch <kbusch@kernel.org>
> > > 
> > > Instead of warning about the device's SGL capabilities the first time we
> > > consider using them with a passthrough command, print out what the
> > > device is capable of during initialization. While we're at it, print
> > > other interesting capabilities that may be useful to know when you just
> > > have a dmesg. The ouput format was inspired by other existing subsystems
> > > like pci.
> > > 
> > > And example of the new message when used with a default qemu nvme
> > > emulated device:
> > > 
> > >    nvme nvme0: sgl+ meta-sgl+ sed- vwc+ apst- dsm+ wz+
> > Printing out the capabilities sounds really useful.  But I don't see
> > how this is useful for replacing the warning about the unchecked
> > passthrough?

I thought this patch could remove the message because it tells you
upfront what you'll get if you use the passthrough interface. The
existing message tells you what you're getting once you actually try to
use it, so they felt a bit redundant.

> I agree, I'd keep the warn_once messages.

Okay. The problem with the message is that it's a new warn, but nothing
has really changed as far as what the driver supports, so having it at
the "warn" level caused some alarm. Any concern if it's an info_once
instead? I just want to avoid repeat explanations on its implications;
I've experienced several offlist discussions about it already.

Re: [RFC PATCH] nvme: replace lazy sgl warn with informational message

[Christoph Hellwig]

On Fri, Jan 31, 2025 at 10:54:46AM -0700, Keith Busch wrote:
> I thought this patch could remove the message because it tells you
> upfront what you'll get if you use the passthrough interface. The
> existing message tells you what you're getting once you actually try to
> use it, so they felt a bit redundant.
> 
> > I agree, I'd keep the warn_once messages.
> 
> Okay. The problem with the message is that it's a new warn, but nothing
> has really changed as far as what the driver supports, so having it at
> the "warn" level caused some alarm. Any concern if it's an info_once
> instead? I just want to avoid repeat explanations on its implications;
> I've experienced several offlist discussions about it already.

Well, we do want to warn, right?  To make sure all the current
administratative uses of passthrough switch the sysfs interfaces we
added over time.