Re: [PATCH v2 1/2] nvmet: return Invalid Field for invalid NSIDs in Set Features 82h

[Nilay Shroff <nilay@linux.ibm.com>]

On 9/21/26 12:23 PM, Christoph Hellwig wrote:
> On Sun, Sep 20, 2026 at 04:37:08PM +0530, Nilay Shroff wrote:
>> On 9/20/26 9:04 AM, Guixin Liu wrote:
>>> diff --git a/drivers/nvme/target/pr.c b/drivers/nvme/target/pr.c
>>> index 0948a690a1c0..a36c732883d4 100644
>>> --- a/drivers/nvme/target/pr.c
>>> +++ b/drivers/nvme/target/pr.c
>>> @@ -51,6 +51,8 @@ u16 nvmet_set_feat_resv_notif_mask(struct nvmet_req *req, u32 mask)
>>>      	if (nsid != U32_MAX) {
>>>    		status = nvmet_req_find_ns(req);
>>> +		if (status == (NVME_SC_INVALID_NS | NVME_STATUS_DNR))
>>> +			return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
>>>    		if (status)
>>>    			return status;
>>>    		if (!req->ns->pr.enable)
>>> -- 
>>
>> This looks good to me. However, to avoid hand-coding the same error
>> return code multiple times in the function and to make it a bit more
>> readable, how about modifying it as follows?
> 
> I don't understand how that helps.  If we really want to refactor,
> we could add a version of nvmet_req_find_ns that does it, but
> that feels like a bit of overkill.
> 
I agree that adding a separate helper just for this would be unnecessary.

My suggestion was mainly for readability and to avoid repeating the same
NVME_SC_INVALID_FIELD | NVME_STATUS_DNR error code and keep the error
handling in one place. I also found the two status checks a little
less straightforward to read. When applying the proposed patch, the
resulting code looks like below:

u16 nvmet_get_feat_resv_notif_mask(struct nvmet_req *req)
{
	u16 status;

	status = nvmet_req_find_ns(req);
	if (status == (NVME_SC_INVALID_NS | NVME_STATUS_DNR))
		return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
	if (status)
		return status;

	if (!req->ns->pr.enable)
		return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;

	nvmet_set_result(req, READ_ONCE(req->ns->pr.notify_mask));
	return status;
}

That said, this is mostly a matter of personal preference rather than
a functional concern, so I'm fine with keeping the current version if
everyone agrees.

>> 		if (status == (NVME_SC_INVALID_NS | NVME_STATUS_DNR))
> 
> Still no need for the inner braces here in all versions.
> 
I think Guixin has already replied to this comment in another
email. The inner braces are required here otherwise compiler
would complain:

drivers/nvme/target/pr.c:80:35: error: | has lower precedence than ==; == will be evaluated first [-Werror,-Wparentheses]
    80 |         if (status == NVME_SC_INVALID_NS | NVME_STATUS_DNR)
       |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^


Thanks,
--Nilay