RE: [PATCH 09/14] nvmet: Add AEN configuration support
From: Verkamp, Daniel <hidden>
Date: 2018-05-29 17:15:34
Also in:
linux-nvme
[...]
quoted hunk ↗ jump to hunk
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c index 82d521f60a43..20d9ce5064f8 100644--- a/drivers/nvme/target/admin-cmd.c +++ b/drivers/nvme/target/admin-cmd.c@@ -432,6 +432,16 @@ static void nvmet_execute_set_features(structnvmet_req *req) req->sq->ctrl->kato =3D DIV_ROUND_UP(val32, 1000); nvmet_set_result(req, req->sq->ctrl->kato); break; + val32 =3D le32_to_cpu(req->cmd->common.cdw10[1]); + if (val32 & ~NVMET_AEN_SUPPORTED) { + status =3D NVME_SC_INVALID_FIELD | NVME_SC_DNR; + break; + }
This looks overly restrictive - a host sending a Set Features with e.g. the= health critical warning bits set in CDW11 will get a failure. As far as I= can tell, this isn't allowed by the spec; Set Features - Asynchronous Eve= nt Configuration and the health log page have been mandatory since NVMe 1.0= , and presumably support for the corresponding health log page related AER = bits is also mandatory (these were the only bits available in NVMe 1.0). I= think it should be fine to just allow the user to set any (valid) combinat= ion of bits here, while still only triggering the NS Changed notification. Thanks, -- Daniel