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@xxxxxxx>
>>> ---
>>> 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.
