Re: [PATCH v4] nvme: reject keep-alive passthrough on non-fabrics

[Christoph Hellwig <hch@lst.de>]

On Fri, May 22, 2026 at 01:32:38PM -0600, Keith Busch wrote:
> On Fri, May 22, 2026 at 12:26:39PM -0400, Chao Shi wrote:
> > +/*
> > + * Some Set Features commands change controller behaviour that the driver is
> > + * not prepared to handle on every transport.  Reject such commands from
> > + * userspace passthrough rather than letting them put the controller into a
> > + * state the driver cannot deal with.  The list can be extended as other
> > + * problematic features are identified.
> > + */
> > +static bool nvme_passthru_cmd_allowed(struct nvme_ctrl *ctrl,
> > +				      struct nvme_ns *ns,
> > +				      struct nvme_command *c)
> > +{
> > +	/*
> > +	 * This only filters admin commands (ns == NULL).  I/O commands share
> > +	 * the opcode space with admin commands - Dataset Management is 0x09,
> > +	 * the same value as Set Features - so they must not be inspected here.
> > +	 */
> > +	if (ns || c->common.opcode != nvme_admin_set_features)
> > +		return true;
> > +
> > +	switch (le32_to_cpu(c->common.cdw10) & 0xff) {
> > +	case NVME_FEAT_KATO:
> > +		/*
> > +		 * Keep Alive is optional on PCIe (NVMe 2.0a 5.27.1.12) and the
> > +		 * driver only arms keep-alive for fabrics.  Enabling it on
> > +		 * other transports starts a keep-alive command the driver is
> > +		 * not set up for and harms idle power states, so reject it.
> > +		 */
> > +		return ctrl->ops->flags & NVME_F_FABRICS;
> > +	default:
> > +		return true;
> > +	}
> > +}
> 
> This doesn't need to be its own function. You can add these checks to
> the existing nvme_cmd_allowed():

Sorry for only catching this.  Adding it to the common function is of
course good, but I think it's grown to a size where splitting that common
code into sub-helper as suggested probably helps readability.