Re: [PATCH v11 7/9] nvmet-passthru: Add passthru code to process commands

[Logan Gunthorpe <logang@deltatee.com>]

Thanks for the Review!

On 2020-02-26 4:28 p.m., Sagi Grimberg wrote:
> This looks questionable... There are tons of features that doesn't
> make sense here like hmb, temperature stuff, irq stuff, timestamps,
> reservations etc... passing-through these will have confusing
> semantics.. Maybe white-list what actually makes sense to passthru?

Yes, I agree a white-list here probably makes sense. I'll try to come up
with a list of features to start that whitelist, though my list might be
a bit different from yours: I don't see why temperature or timestamps
can't be passed through.

Also note: Christoph was advocating against the whitelist for the
commands, though, I agree with you that it is the most sensible approach.

>> +        break;
>> +    case nvme_admin_identify:
>> +        switch (req->cmd->identify.cns) {
>> +        case NVME_ID_CNS_CTRL:
>> +            req->execute = nvmet_passthru_execute_cmd;
>> +            req->p.end_req = nvmet_passthru_override_id_ctrl;
>> +            return NVME_SC_SUCCESS;
>> +        case NVME_ID_CNS_NS:
>> +            req->execute = nvmet_passthru_execute_cmd;
>> +            req->p.end_req = nvmet_passthru_override_id_ns;
>> +            return NVME_SC_SUCCESS;
> 
> Aren't you missing NVME_ID_CNS_NS_DESC_LIST? and
> NVME_ID_CNS_NS_ACTIVE_LIST?

Well no, seeing they can be passed through the default path.... But in
the light of the comment below, yes.

>> +        default:
>> +            return nvmet_setup_passthru_command(req);
>> +        }
> 
> Also here, all the namespace management stuff has questionable
> semantics in my mind...

Yes, I agree with that. I'll make the change in the next revision.

Logan

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme