| Message ID | 20221208105947.2399894-12-niklas.cassel@wdc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add Command Duration Limits support | expand |
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?
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
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.
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 --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 */
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(-)