diff mbox series

[11/25] scsi: move get_scsi_ml_byte() to scsi_priv.h

Message ID 20221208105947.2399894-12-niklas.cassel@wdc.com
State New
Headers show
Series Add Command Duration Limits support | expand

Commit Message

Niklas Cassel Dec. 8, 2022, 10:59 a.m. UTC
Move get_scsi_ml_byte() to scsi_priv.h since both scsi_lib.c and
scsi_error.c will need to use this helper in a follow-up patch.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/scsi/scsi_lib.c  | 5 -----
 drivers/scsi/scsi_priv.h | 5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Mike Christie Dec. 8, 2022, 11:58 p.m. UTC | #1
On 12/8/22 4:59 AM, Niklas Cassel wrote:
> Move get_scsi_ml_byte() to scsi_priv.h since both scsi_lib.c and
> scsi_error.c will need to use this helper in a follow-up patch.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  drivers/scsi/scsi_lib.c  | 5 -----
>  drivers/scsi/scsi_priv.h | 5 +++++
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 9ed1ebcb7443..d243ebc5ad54 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -579,11 +579,6 @@ static bool scsi_end_request(struct request *req, blk_status_t error,
>  	return false;
>  }
>  
> -static inline u8 get_scsi_ml_byte(int result)
> -{
> -	return (result >> 8) & 0xff;
> -}
> -
>  /**
>   * scsi_result_to_blk_status - translate a SCSI result code into blk_status_t
>   * @result:	scsi error code
> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> index 96284a0e13fe..4f97e126c6fb 100644
> --- a/drivers/scsi/scsi_priv.h
> +++ b/drivers/scsi/scsi_priv.h
> @@ -29,6 +29,11 @@ enum scsi_ml_status {
>  	SCSIML_STAT_TGT_FAILURE		= 0x04,	/* Permanent target failure */
>  };
>  
> +static inline u8 get_scsi_ml_byte(int result)
> +{
> +	return (result >> 8) & 0xff;
> +}
> +

I made a mistake in the naming of this function. The get_* functions take a
scsi_cmnd. The ones without the "get_" prefix take the scsi_cmnd->result.

Could you rename it to scsi_ml_byte() when you move it so the mistake does not
get used in multiple places?
Niklas Cassel Dec. 28, 2022, 8:41 p.m. UTC | #2
On Thu, Dec 08, 2022 at 05:58:01PM -0600, Mike Christie wrote:
> On 12/8/22 4:59 AM, Niklas Cassel wrote:
> > Move get_scsi_ml_byte() to scsi_priv.h since both scsi_lib.c and
> > scsi_error.c will need to use this helper in a follow-up patch.
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > ---
> >  drivers/scsi/scsi_lib.c  | 5 -----
> >  drivers/scsi/scsi_priv.h | 5 +++++
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index 9ed1ebcb7443..d243ebc5ad54 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -579,11 +579,6 @@ static bool scsi_end_request(struct request *req, blk_status_t error,
> >  	return false;
> >  }
> >  
> > -static inline u8 get_scsi_ml_byte(int result)
> > -{
> > -	return (result >> 8) & 0xff;
> > -}
> > -
> >  /**
> >   * scsi_result_to_blk_status - translate a SCSI result code into blk_status_t
> >   * @result:	scsi error code
> > diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> > index 96284a0e13fe..4f97e126c6fb 100644
> > --- a/drivers/scsi/scsi_priv.h
> > +++ b/drivers/scsi/scsi_priv.h
> > @@ -29,6 +29,11 @@ enum scsi_ml_status {
> >  	SCSIML_STAT_TGT_FAILURE		= 0x04,	/* Permanent target failure */
> >  };
> >  
> > +static inline u8 get_scsi_ml_byte(int result)
> > +{
> > +	return (result >> 8) & 0xff;
> > +}
> > +
> 
> I made a mistake in the naming of this function. The get_* functions take a
> scsi_cmnd. The ones without the "get_" prefix take the scsi_cmnd->result.
> 
> Could you rename it to scsi_ml_byte() when you move it so the mistake does not
> get used in multiple places?

Hello Mike,

In scsi.h we have:
#define status_byte(result) (result & 0xff)
#define host_byte(result)   (((result) >> 16) & 0xff)

In scsi_cmnd.h we have:
static inline u8 get_status_byte(struct scsi_cmnd *cmd)
static inline u8 get_host_byte(struct scsi_cmnd *cmd)

So I see your point.


I understand that you intentionally didn't put the ML byte getter in scsi.h,
as you intentionally didn't want a LLDD to be able to get the SCSI ML byte.

However, isn't the important thing that that a LLDD can't *set* the SCSI ML
byte?
Does it really matter if a LLDD can *get* the SCSI ML byte?

If you think that it is fine that a LLD can get the SCSI ML byte,
then perhaps we should move the getter to scsi.h instead, such that
it is defined together with the other getters (which lacks a get_ prefix).


Kind regards,
Niklas
Mike Christie Dec. 29, 2022, 6:55 p.m. UTC | #3
On 12/28/22 2:41 PM, Niklas Cassel wrote:
> On Thu, Dec 08, 2022 at 05:58:01PM -0600, Mike Christie wrote:
>> On 12/8/22 4:59 AM, Niklas Cassel wrote:
>>> Move get_scsi_ml_byte() to scsi_priv.h since both scsi_lib.c and
>>> scsi_error.c will need to use this helper in a follow-up patch.
>>>
>>> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>>> ---
>>>  drivers/scsi/scsi_lib.c  | 5 -----
>>>  drivers/scsi/scsi_priv.h | 5 +++++
>>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>>> index 9ed1ebcb7443..d243ebc5ad54 100644
>>> --- a/drivers/scsi/scsi_lib.c
>>> +++ b/drivers/scsi/scsi_lib.c
>>> @@ -579,11 +579,6 @@ static bool scsi_end_request(struct request *req, blk_status_t error,
>>>  	return false;
>>>  }
>>>  
>>> -static inline u8 get_scsi_ml_byte(int result)
>>> -{
>>> -	return (result >> 8) & 0xff;
>>> -}
>>> -
>>>  /**
>>>   * scsi_result_to_blk_status - translate a SCSI result code into blk_status_t
>>>   * @result:	scsi error code
>>> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
>>> index 96284a0e13fe..4f97e126c6fb 100644
>>> --- a/drivers/scsi/scsi_priv.h
>>> +++ b/drivers/scsi/scsi_priv.h
>>> @@ -29,6 +29,11 @@ enum scsi_ml_status {
>>>  	SCSIML_STAT_TGT_FAILURE		= 0x04,	/* Permanent target failure */
>>>  };
>>>  
>>> +static inline u8 get_scsi_ml_byte(int result)
>>> +{
>>> +	return (result >> 8) & 0xff;
>>> +}
>>> +
>>
>> I made a mistake in the naming of this function. The get_* functions take a
>> scsi_cmnd. The ones without the "get_" prefix take the scsi_cmnd->result.
>>
>> Could you rename it to scsi_ml_byte() when you move it so the mistake does not
>> get used in multiple places?
> 
> Hello Mike,
> 
> In scsi.h we have:
> #define status_byte(result) (result & 0xff)
> #define host_byte(result)   (((result) >> 16) & 0xff)
> 
> In scsi_cmnd.h we have:
> static inline u8 get_status_byte(struct scsi_cmnd *cmd)
> static inline u8 get_host_byte(struct scsi_cmnd *cmd)
> 
> So I see your point.
> 
> 
> I understand that you intentionally didn't put the ML byte getter in scsi.h,
> as you intentionally didn't want a LLDD to be able to get the SCSI ML byte.
> 
> However, isn't the important thing that that a LLDD can't *set* the SCSI ML
> byte?
> Does it really matter if a LLDD can *get* the SCSI ML byte?

We don't want LLDDs to get the ml byte and do something with it because I think
it's kind of just a hack to avoid parsing the result/sense multiple times, and
the drivers have no need for accessing it.

If you put it in scsi_cmnd.h they can still do:

if (get_scsi_ml_byte(cmd))
	do something;

which we don't want.
Niklas Cassel Dec. 29, 2022, 8:19 p.m. UTC | #4
On Thu, Dec 29, 2022 at 12:55:47PM -0600, Mike Christie wrote:
> On 12/28/22 2:41 PM, Niklas Cassel wrote:
> > I understand that you intentionally didn't put the ML byte getter in scsi.h,
> > as you intentionally didn't want a LLDD to be able to get the SCSI ML byte.
> > 
> > However, isn't the important thing that that a LLDD can't *set* the SCSI ML
> > byte?
> > Does it really matter if a LLDD can *get* the SCSI ML byte?
> 
> We don't want LLDDs to get the ml byte and do something with it because I think
> it's kind of just a hack to avoid parsing the result/sense multiple times, and
> the drivers have no need for accessing it.
> 
> If you put it in scsi_cmnd.h they can still do:
> 
> if (get_scsi_ml_byte(cmd))
> 	do something;
> 
> which we don't want.
> 

Ok, when I send a V2 of this patch, it will be identical to this patch,
with the exception that get_scsi_ml_byte() will be renamed to
scsi_ml_byte(). (Parameters will be the same.)


Kind regards,
Niklas
diff mbox series

Patch

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9ed1ebcb7443..d243ebc5ad54 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -579,11 +579,6 @@  static bool scsi_end_request(struct request *req, blk_status_t error,
 	return false;
 }
 
-static inline u8 get_scsi_ml_byte(int result)
-{
-	return (result >> 8) & 0xff;
-}
-
 /**
  * scsi_result_to_blk_status - translate a SCSI result code into blk_status_t
  * @result:	scsi error code
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 96284a0e13fe..4f97e126c6fb 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -29,6 +29,11 @@  enum scsi_ml_status {
 	SCSIML_STAT_TGT_FAILURE		= 0x04,	/* Permanent target failure */
 };
 
+static inline u8 get_scsi_ml_byte(int result)
+{
+	return (result >> 8) & 0xff;
+}
+
 /*
  * Scsi Error Handler Flags
  */