Re: [PATCH v2] nvme: allow 64-bit results in passthru commands

[Marta Rybczynska]

----- On 22 Aug, 2019, at 02:06, Christoph Hellwig hch@xxxxxx wrote:

> On Fri, Aug 16, 2019 at 11:47:21AM +0200, Marta Rybczynska wrote:
>> It is not possible to get 64-bit results from the passthru commands,
>> what prevents from getting for the Capabilities (CAP) property value.
>> 
>> As a result, it is not possible to implement IOL's NVMe Conformance
>> test 4.3 Case 1 for Fabrics targets [1] (page 123).
>> 
>> This issue has been already discussed [2], but without a solution.
>> 
>> This patch solves the problem by adding new ioctls with a new
>> passthru structure, including 64-bit results. The older ioctls stay
>> unchanged.
> 
> Ok, with my idea not being suitable I think I'm fine with this approach, a
> little nitpick below:
> 
>> +static bool is_admin_cmd(unsigned int cmd)
>> +{
>> +	if ((cmd == NVME_IOCTL_ADMIN_CMD) || (cmd == NVME_IOCTL_ADMIN64_CMD))
>> +		return true;
>> +	return false;
>> +}
> 
> No need for the inner braces.  But I'm actually not sure the current
> code structure is very suitable for extending it.
> 
>> +
>>  static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
>>  		unsigned int cmd, unsigned long arg)
>>  {
>> @@ -1418,13 +1473,13 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t
>> mode,
>>  	 * seperately and drop the ns SRCU reference early.  This avoids a
>>  	 * deadlock when deleting namespaces using the passthrough interface.
>>  	 */
>> -	if (cmd == NVME_IOCTL_ADMIN_CMD || is_sed_ioctl(cmd)) {
>> +	if (is_admin_cmd(cmd) || is_sed_ioctl(cmd)) {
> 
> So maybe for this check we should have a is_ctrl_iocl() helper instead
> that includes the is_sed_ioctl check.
> 
>>  		struct nvme_ctrl *ctrl = ns->ctrl;
>>  
>>  		nvme_get_ctrl(ns->ctrl);
>>  		nvme_put_ns_from_disk(head, srcu_idx);
>>  
>> -		if (cmd == NVME_IOCTL_ADMIN_CMD)
>> +		if (is_admin_cmd(cmd))
>>  			ret = nvme_user_cmd(ctrl, NULL, argp);
>>  		else
>>  			ret = sed_ioctl(ctrl->opal_dev, cmd, argp);
> 
> And then we can move this whole branch into a helper function,
> which then switches on the ioctl cmd, with sed_ioctl as the fallback.

Do you mean something like this?

+static bool is_ctrl_ioctl(unsigned int cmd)
+{
+	if (cmd == NVME_IOCTL_ADMIN_CMD || cmd == NVME_IOCTL_ADMIN64_CMD)
+		return true;
+	if (is_sed_ioctl(cmd))
+		return true;
+	return false;
+}
+
+static int nvme_handle_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd,
+				  void __user *argp,
+				  struct nvme_ns_head *head,
+				  int srcu_idx)
+{
+	struct nvme_ctrl *ctrl = ns->ctrl;
+	int ret;
+
+	nvme_get_ctrl(ns->ctrl);
+	nvme_put_ns_from_disk(head, srcu_idx);
+
+	switch (cmd) {
+	case NVME_IOCTL_ADMIN_CMD:
+		ret = nvme_user_cmd(ctrl, NULL, argp);
+		break;
+	case NVME_IOCTL_ADMIN64_CMD:
+		ret = nvme_user_cmd64(ctrl, NULL, argp);
+		break;
+	default:
+		ret = sed_ioctl(ctrl->opal_dev, cmd, argp);
+		break;
+	}
+	nvme_put_ctrl(ctrl);
+	return ret;
+}
+
 static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 		unsigned int cmd, unsigned long arg)
 {
@@ -1418,20 +1501,8 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 	 * seperately and drop the ns SRCU reference early.  This avoids a
 	 * deadlock when deleting namespaces using the passthrough interface.
 	 */
-	if (cmd == NVME_IOCTL_ADMIN_CMD || is_sed_ioctl(cmd)) {
-		struct nvme_ctrl *ctrl = ns->ctrl;
-
-		nvme_get_ctrl(ns->ctrl);
-		nvme_put_ns_from_disk(head, srcu_idx);
-
-		if (cmd == NVME_IOCTL_ADMIN_CMD)
-			ret = nvme_user_cmd(ctrl, NULL, argp);
-		else
-			ret = sed_ioctl(ctrl->opal_dev, cmd, argp);
-
-		nvme_put_ctrl(ctrl);
-		return ret;
-	}
+	if (is_ctrl_ioctl(cmd))
+		return nvme_handle_ctrl_ioctl(ns, cmd, argp, head, srcu_idx);
 
 	switch (cmd) {
 	case NVME_IOCTL_ID:

Regards,
Marta