Re: [PATCH 1/3] nvme: fix nvme_pr_* status code parsing

[Weiwen Hu <huweiwen@linux.alibaba.com>]

On Wed, May 29, 2024 at 03:41:37PM -0600, Keith Busch wrote:
> On Wed, May 29, 2024 at 07:18:36PM +0000, Chaitanya Kulkarni wrote:
> > On 5/29/24 05:22, Weiwen Hu wrote:
> > > Fix the parsing if extra status bits (e.g. MORE) is present.
> > >
> > > Renamed nvme_sc_to_pr_err to nvme_status_to_pr_err to better match its
> > > semantic.
> > >
> > > Fixes: 7fb42780d06c ("nvme: Convert NVMe errors to PR errors")
> > > Signed-off-by: Weiwen Hu <huweiwen@linux.alibaba.com>
> > > ---
> > >   drivers/nvme/host/pr.c | 10 +++++-----
> > >   1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
> > > index e05571b2a1b0..25e23cdba151 100644
> > > --- a/drivers/nvme/host/pr.c
> > > +++ b/drivers/nvme/host/pr.c
> > > @@ -72,12 +72,12 @@ static int nvme_send_ns_pr_command(struct nvme_ns *ns, struct nvme_command *c,
> > >   	return nvme_submit_sync_cmd(ns->queue, c, data, data_len);
> > >   }
> > >   
> > > -static int nvme_sc_to_pr_err(int nvme_sc)
> > > +static int nvme_status_to_pr_err(int nvme_status)
> > 
> > do you really have to make this helper name longer ?
> > nvme_sc matches the NVME_SC and makes it easy to grep when
> > looking for symbols and error codes, but if everyone is okay with
> > this then sure go ahead ...

I change it specifically because nvme_sc matches NVME_SC_* too good. That makes
people think nvme_sc can be compared to NVME_SC_* directly, but it can not
actually.  We may think of another name, but "sc" is not appropriate IMO.

"sc" specifically refer to the last 8 bit in the status field in NVMe spec, and
nvme_is_path_error() also use "status" as its parameter name. So I think
"status" can be a good choice.

> 
> Yeah, I'd agree. In fact, just resend with only the "& 0x7ff" masking
> for the "Fixes" part of this, and that can go in for 6.10. Potential
> enhancments can come after.

OK, resent the first patch. I will resent the refactors after the discussions
here are concluded.
 
> > > -	switch (nvme_sc) {
> > > +	switch (nvme_status & 0x7ff) {
> > >   	case NVME_SC_SUCCESS:
> > >   		return PR_STS_SUCCESS;
> > >   	case NVME_SC_RESERVATION_CONFLICT: