libnvme API design

[Daniel Wagner]

On Fri, Oct 10, 2025 at 09:26:58AM +0200, Christoph Hellwig wrote:
> On Thu, Oct 09, 2025 at 01:07:47PM +0200, Daniel Wagner wrote:
> > Hi Christoph,
> > 
> > As you have suggested the command API should be inverted, that is the
> > only stable API will be nvme_submit_admin_passthru and struct
> > nvme_passthru_cmd and a bunch of helpers to initialize the cmd.
> > 
> > I've done updated a bunch of the identify commands and below is the
> > result. I think we could reduce the number of helpers a bit,
> > 
> >   nvme_nvm_init_identify_ns vs nvme_nvm_init_identify_ns_csi.
> > 
> > Is this what you had in mind
> 
> Yes.
> 
> > (obviously I haven't gone the whole way to
> > macros only, I like some type safty)?
> 
> What actual type safety do we actually gain here?

Not much, just via the argument types. I agree this argument is a bit
mood. The other point I missed to mention, is that the inline function
allow us to write code not worrying about the context it is called from
,no variable name collission. But then this might not be a big thing
either. I just prefer inline function over macros :)

> > static inline
> > void nvme_nvm_init_identify_ns(struct nvme_passthru_cmd *cmd,
> > 			       __u32 nsid, struct nvme_id_ns *ns)
> > {
> > 	nvme_nvm_init_identify(cmd,
> > 			       NVME_IDENTIFY_CNS_NS,
> > 			       ns, sizeof(*ns));
> > 	cmd->nsid = nsid;
> 
> Given that identify includes nsid and just specified how it is
> cleared to zero for the non-ns commands, maybe pass a nsid to
> nvme_nvm_init_identify to simplify things?

Okay, the majority of the currently existing helpers do use nsid so that
seem common enough.

> > 	cmd->cdw11 |=  NVME_SET(nvmsetid, IDENTIFY_CDW11_CNSSPECID);
> 
> And NVME_SET still feels very oddly name..

It's on the TODO list but we might address this at the same time indeed.

Is it just the name or the macro itself?

Something like NVME_FIELD_PREP, REG_FIELD_PREP, FIELD_ENCODE,
PACK_FIELD?

Yeah I am struggling coming up with good names...


Just for reference what v1 currently defines:

#define NVME_GET(value, name) \
	(((value) >> NVME_##name##_SHIFT) & NVME_##name##_MASK)

#define NVME_SET(value, name) \
	(((__u32)(value) & NVME_##name##_MASK) << NVME_##name##_SHIFT)

#define NVME_CHECK(value, name, check) ((value) == NVME_##name##_##check)

#define NVME_VAL(name) (NVME_##name##_MASK << NVME_##name##_SHIFT)


enum nvme_cmd_dword_fields {
	NVME_DEVICE_SELF_TEST_CDW10_STC_SHIFT			= 0,
	NVME_DEVICE_SELF_TEST_CDW10_STC_MASK			= 0xf,
	NVME_DIRECTIVE_CDW11_DOPER_SHIFT			= 0,
	NVME_DIRECTIVE_CDW11_DTYPE_SHIFT			= 8,
	NVME_DIRECTIVE_CDW11_DPSEC_SHIFT			= 16,
	NVME_DIRECTIVE_CDW11_DOPER_MASK				= 0xff,
	NVME_DIRECTIVE_CDW11_DTYPE_MASK				= 0xff,
	NVME_DIRECTIVE_CDW11_DPSEC_MASK				= 0xffff,
        [...]
};