[PATCH 1/2] nvme-auth: unlock mutex in one place only

[PATCH 1/2] nvme-auth: unlock mutex in one place only

[Mark O'Donovan]

Signed-off-by: Mark O'Donovan <shiftee@posteo.net>
---
 drivers/nvme/host/auth.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index daf5d144a8ea..e1a98647c3a2 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -758,12 +758,11 @@ static void nvme_queue_auth_work(struct work_struct *work)
 		__func__, chap->qid);
 	mutex_lock(&ctrl->dhchap_auth_mutex);
 	ret = nvme_auth_dhchap_setup_host_response(ctrl, chap);
+	mutex_unlock(&ctrl->dhchap_auth_mutex);
 	if (ret) {
-		mutex_unlock(&ctrl->dhchap_auth_mutex);
 		chap->error = ret;
 		goto fail2;
 	}
-	mutex_unlock(&ctrl->dhchap_auth_mutex);
 
 	/* DH-HMAC-CHAP Step 3: send reply */
 	dev_dbg(ctrl->device, "%s: qid %d send reply\n",
-- 
2.39.2

[PATCH 2/2] nvme-auth: set explanation code for failure2 msgs

[Mark O'Donovan]

These error cases were not setting an auth-failure-reason-code-explanation.
This means an AUTH_Failure2 message will be sent with an explanation value
of 0 which is a reserved value.

Signed-off-by: Mark O'Donovan <shiftee@posteo.net>
---
 drivers/nvme/host/auth.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index e1a98647c3a2..094f37e11921 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -760,6 +760,7 @@ static void nvme_queue_auth_work(struct work_struct *work)
 	ret = nvme_auth_dhchap_setup_host_response(ctrl, chap);
 	mutex_unlock(&ctrl->dhchap_auth_mutex);
 	if (ret) {
+		chap->status = NVME_AUTH_DHCHAP_FAILURE_FAILED;
 		chap->error = ret;
 		goto fail2;
 	}
@@ -776,6 +777,7 @@ static void nvme_queue_auth_work(struct work_struct *work)
 	tl = ret;
 	ret = nvme_auth_submit(ctrl, chap->qid, chap->buf, tl, true);
 	if (ret) {
+		chap->status = NVME_AUTH_DHCHAP_FAILURE_FAILED;
 		chap->error = ret;
 		goto fail2;
 	}
@@ -811,6 +813,7 @@ static void nvme_queue_auth_work(struct work_struct *work)
 		ret = nvme_auth_dhchap_setup_ctrl_response(ctrl, chap);
 		if (ret) {
 			mutex_unlock(&ctrl->dhchap_auth_mutex);
+			chap->status = NVME_AUTH_DHCHAP_FAILURE_FAILED;
 			chap->error = ret;
 			goto fail2;
 		}
@@ -830,8 +833,10 @@ static void nvme_queue_auth_work(struct work_struct *work)
 			__func__, chap->qid);
 		tl = nvme_auth_set_dhchap_success2_data(ctrl, chap);
 		ret = nvme_auth_submit(ctrl, chap->qid, chap->buf, tl, true);
-		if (ret)
+		if (ret) {
+			chap->status = NVME_AUTH_DHCHAP_FAILURE_FAILED;
 			chap->error = ret;
+		}
 	}
 	if (!ret) {
 		chap->error = 0;
-- 
2.39.2

Re: [PATCH 2/2] nvme-auth: set explanation code for failure2 msgs

[Christoph Hellwig]

On Thu, Jul 27, 2023 at 01:47:48PM +0000, Mark O'Donovan wrote:
> These error cases were not setting an auth-failure-reason-code-explanation.
> This means an AUTH_Failure2 message will be sent with an explanation value
> of 0 which is a reserved value.

I'll leave the final decision to Hannes, but shouldn't we find a common
place, either behind a label or in the body of the function to set
chap->status?  Having to add this in a lot of error labels doesn't feel
very maintainable.

Re: [PATCH 2/2] nvme-auth: set explanation code for failure2 msgs

[Hannes Reinecke]

On 7/31/23 07:58, Christoph Hellwig wrote:
> On Thu, Jul 27, 2023 at 01:47:48PM +0000, Mark O'Donovan wrote:
>> These error cases were not setting an auth-failure-reason-code-explanation.
>> This means an AUTH_Failure2 message will be sent with an explanation value
>> of 0 which is a reserved value.
> 
> I'll leave the final decision to Hannes, but shouldn't we find a common
> place, either behind a label or in the body of the function to set
> chap->status?  Having to add this in a lot of error labels doesn't feel
> very maintainable.
> 
Yeah; the whole AUTH_XXX errors are ever so cumbersome as we don't have 
a good way to passing them up the stack. Plus we can't transport errors
during connect, making it doubly pointless.
Maybe a printk() somewhere. I'll check.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH 1/2] nvme-auth: unlock mutex in one place only

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/2] nvme-auth: unlock mutex in one place only

[Markus Elfring]

Are imperative change descriptions still preferred?

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc3#n94


Will a cover letter become helpful also for the presented small patch series?

See also:
https://kernelnewbies.org/PatchSeries

Regards,
Markus

Re: [PATCH 1/2] nvme-auth: unlock mutex in one place only

[Christoph Hellwig]

On Mon, Jul 31, 2023 at 02:00:37PM +0200, Markus Elfring wrote:
> Are imperative change descriptions still preferred?

It doesn't fucking matter.  Please stop spamming nvme contributors.

Re: [1/2] nvme-auth: unlock mutex in one place only

[Markus Elfring]

>> Are imperative change descriptions still preferred?
>
> It doesn't fucking matter.

Do you exaggerate here?

If not:
Does such a feedback really indicate that you would intentionally like to
disagree with specific requirements from the Linux development documentation
(for a discussed patch)?


> Please stop spamming nvme contributors.
Why do you interpret a few change suggestions in this direction?

Regards,
Markus

Re: [1/2] nvme-auth: unlock mutex in one place only

[Keith Busch]

On Mon, Jul 31, 2023 at 07:46:25PM +0200, Markus Elfring wrote:
> >> Are imperative change descriptions still preferred?
> >
> > It doesn't fucking matter.
> 
> Do you exaggerate here?
> 
> If not:
> Does such a feedback really indicate that you would intentionally like to
> disagree with specific requirements from the Linux development documentation
> (for a discussed patch)?

The subject is already in the imperative voice, and there's really
nothing furthur to elaborate that would help justify this patch's
inclusion. I'm not sure what point you're trying to make here.

Re: [1/2] nvme-auth: unlock mutex in one place only

[Markus Elfring]

> The subject is already in the imperative voice, and there's really
> nothing furthur to elaborate that would help justify this patch's
> inclusion. I'm not sure what point you're trying to make here.

Does the concrete requirement for the usage of “imperative mood”
mean also that a reasonable subject would be insufficient alone
without a corresponding description (or “change log”)?

Regards,
Markus

Re: [1/2] nvme-auth: unlock mutex in one place only

[Keith Busch]

On Mon, Jul 31, 2023 at 08:37:12PM +0200, Markus Elfring wrote:
> Does the concrete requirement for the usage of “imperative mood”
> mean also that a reasonable subject would be insufficient alone
> without a corresponding description (or “change log”)?

The doc you linked to is a simple guide for people new to the process:

  "This text is a collection of suggestions"

It doesn't claim to be a list of concrete requirements.

Re: [PATCH 1/2] nvme-auth: unlock mutex in one place only

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>