I like this in general, but some comments below:
> cmnd->rw.opcode = op;
> + cmnd->rw.flags = 0;
> + cmnd->rw.command_id = 0;
The command_id is always set in nvme_setup_cmd, so no need to clear it
here.
> cmnd->rw.nsid = cpu_to_le32(ns->head->ns_id);
> + cmnd->rw.rsvd2 = 0;
There should be no need to clear this reserved space.
> + cmnd->rw.metadata = 0;
> cmnd->rw.slba = cpu_to_le64(nvme_sect_to_lba(ns, blk_rq_pos(req)));
> cmnd->rw.length = cpu_to_le16((blk_rq_bytes(req) >> ns->lba_shift) - 1);
> + cmnd->rw.reftag = 0;
> + cmnd->rw.apptag = 0;
> + cmnd->rw.appmask = 0;
All these PI fields are reserved when PI isn't enabled using the control
field. So I think we can stop clearing reftag here entirely, and move
clearing the apptag and appmask down next to the reftag assignment.
>
> +static void nvme_clear_cmd(struct request *req)
> +{
> + if (!(req->rq_flags & RQF_DONTPREP)) {
> + nvme_clear_nvme_request(req);
> + memset(nvme_req(req)->cmd, 0, sizeof(struct nvme_command));
> + }
> +}
> +
> blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req)
> {
> struct nvme_command *cmd = nvme_req(req)->cmd;
> struct nvme_ctrl *ctrl = nvme_req(req)->ctrl;
> blk_status_t ret = BLK_STS_OK;
>
> - if (!(req->rq_flags & RQF_DONTPREP)) {
> - nvme_clear_nvme_request(req);
> - memset(cmd, 0, sizeof(*cmd));
> - }
The nvme_clear_nvme_request is not done unconditionally for the read
and write commands below, which does the wrong thing. So I think we want
to keep it here and just move the memset.
I think the best way is to split this patch into two:
1) remove the memset here and do it unconditionally in the individual
nvme_setup_* handlers, and document there why the extra memsets don't
hurt (no partial completions unlikey SCSI)
2) optimize the read/write case
