2022-01-25 16:36:11

by John Garry

Subject: [PATCH 01/16] scsi: libsas: Use enum for response frame DATAPRES field

As defined in table 126 of the SAS spec 1.1, use an enum for the DATAPRES
field, which makes reading the code easier.

Signed-off-by: John Garry <[email protected]>
---
drivers/scsi/libsas/sas_task.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/libsas/sas_task.c b/drivers/scsi/libsas/sas_task.c
index 2966ead1d421..7240fd22b154 100644
--- a/drivers/scsi/libsas/sas_task.c
+++ b/drivers/scsi/libsas/sas_task.c
@@ -7,6 +7,12 @@
#include <scsi/sas.h>
#include <scsi/libsas.h>

+enum {
+ NO_DATA = 0,
+ RESPONSE_DATA = 1,
+ SENSE_DATA = 2,
+};
+
/* fill task_status_struct based on SSP response frame */
void sas_ssp_task_response(struct device *dev, struct sas_task *task,
struct ssp_response_iu *iu)
@@ -15,11 +21,11 @@ void sas_ssp_task_response(struct device *dev, struct sas_task *task,

tstat->resp = SAS_TASK_COMPLETE;

- if (iu->datapres == 0)
+ if (iu->datapres == NO_DATA)
tstat->stat = iu->status;
- else if (iu->datapres == 1)
+ else if (iu->datapres == RESPONSE_DATA)
tstat->stat = iu->resp_data[3];
- else if (iu->datapres == 2) {
+ else if (iu->datapres == SENSE_DATA) {
tstat->stat = SAS_SAM_STAT_CHECK_CONDITION;
tstat->buf_valid_size =
min_t(int, SAS_STATUS_BUF_SIZE,
--
2.26.2

2022-01-25 16:50:58

by Jinpu Wang

Subject: Re: [PATCH 01/16] scsi: libsas: Use enum for response frame DATAPRES field

On Tue, Jan 25, 2022 at 12:38 PM John Garry <[email protected]> wrote:
>
> As defined in table 126 of the SAS spec 1.1, use an enum for the DATAPRES
> field, which makes reading the code easier.
>
> Signed-off-by: John Garry <[email protected]>
Reviewed-by: Jack Wang <[email protected]>
Thx!
> ---
> drivers/scsi/libsas/sas_task.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_task.c b/drivers/scsi/libsas/sas_task.c
> index 2966ead1d421..7240fd22b154 100644
> --- a/drivers/scsi/libsas/sas_task.c
> +++ b/drivers/scsi/libsas/sas_task.c
> @@ -7,6 +7,12 @@
> #include <scsi/sas.h>
> #include <scsi/libsas.h>
>
> +enum {
> + NO_DATA = 0,
> + RESPONSE_DATA = 1,
> + SENSE_DATA = 2,
> +};
> +
> /* fill task_status_struct based on SSP response frame */
> void sas_ssp_task_response(struct device *dev, struct sas_task *task,
> struct ssp_response_iu *iu)
> @@ -15,11 +21,11 @@ void sas_ssp_task_response(struct device *dev, struct sas_task *task,
>
> tstat->resp = SAS_TASK_COMPLETE;
>
> - if (iu->datapres == 0)
> + if (iu->datapres == NO_DATA)
> tstat->stat = iu->status;
> - else if (iu->datapres == 1)
> + else if (iu->datapres == RESPONSE_DATA)
> tstat->stat = iu->resp_data[3];
> - else if (iu->datapres == 2) {
> + else if (iu->datapres == SENSE_DATA) {
> tstat->stat = SAS_SAM_STAT_CHECK_CONDITION;
> tstat->buf_valid_size =
> min_t(int, SAS_STATUS_BUF_SIZE,
> --
> 2.26.2
>

2022-01-27 17:58:43

by Christoph Hellwig

Subject: Re: [PATCH 01/16] scsi: libsas: Use enum for response frame DATAPRES field

On Tue, Jan 25, 2022 at 07:32:37PM +0800, John Garry wrote:
> - if (iu->datapres == 0)
> + if (iu->datapres == NO_DATA)
> tstat->stat = iu->status;
> - else if (iu->datapres == 1)
> + else if (iu->datapres == RESPONSE_DATA)
> tstat->stat = iu->resp_data[3];
> - else if (iu->datapres == 2) {
> + else if (iu->datapres == SENSE_DATA) {

Maybe use a switch here to make it more obvious?

2022-01-27 18:01:16

by John Garry

Subject: Re: [PATCH 01/16] scsi: libsas: Use enum for response frame DATAPRES field

On 27/01/2022 10:19, Christoph Hellwig wrote:
> On Tue, Jan 25, 2022 at 07:32:37PM +0800, John Garry wrote:
>> - if (iu->datapres == 0)
>> + if (iu->datapres == NO_DATA)
>> tstat->stat = iu->status;
>> - else if (iu->datapres == 1)
>> + else if (iu->datapres == RESPONSE_DATA)
>> tstat->stat = iu->resp_data[3];
>> - else if (iu->datapres == 2) {
>> + else if (iu->datapres == SENSE_DATA) {
> Maybe use a switch here to make it more obvious?
> .

ok, I can include that suggestion.

Thanks,
John

2022-01-27 20:03:42

by chenxiang (M)

Subject: Re: [PATCH 01/16] scsi: libsas: Use enum for response frame DATAPRES field

Hi John,


在 2022/1/25 19:32, John Garry 写道:
> As defined in table 126 of the SAS spec 1.1, use an enum for the DATAPRES
> field, which makes reading the code easier.
>
> Signed-off-by: John Garry <[email protected]>
> ---
> drivers/scsi/libsas/sas_task.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_task.c b/drivers/scsi/libsas/sas_task.c
> index 2966ead1d421..7240fd22b154 100644
> --- a/drivers/scsi/libsas/sas_task.c
> +++ b/drivers/scsi/libsas/sas_task.c
> @@ -7,6 +7,12 @@
> #include <scsi/sas.h>
> #include <scsi/libsas.h>
>
> +enum {
> + NO_DATA = 0,
> + RESPONSE_DATA = 1,
> + SENSE_DATA = 2,
> +};
> +

I find that iu->datapres is also used in other drivers with 0/1/2, and
maybe it is worth to replace all of them with those enum.

2 290
/home/chenxiang/kernel_next/kernel-dev/drivers/scsi/aic94xx/aic94xx_tmf.c
<<<unknown>>>
if (ru->datapres == 1)
5 1055
/home/chenxiang/kernel_next/kernel-dev/drivers/scsi/isci/request.c
<<<unknown>>>
if (datapres == 1 || datapres == 2) {
6 1740
/home/chenxiang/kernel_next/kernel-dev/drivers/scsi/isci/request.c
<<<unknown>>>
if (resp_iu->datapres == 0x01 ||
7 1741
/home/chenxiang/kernel_next/kernel-dev/drivers/scsi/isci/request.c
<<<unknown>>>
resp_iu->datapres == 0x02) {
17 1641
/home/chenxiang/kernel_next/kernel-dev/drivers/scsi/mvsas/mv_sas.c
<<<unknown>>>
iu->datapres = 2;

> /* fill task_status_struct based on SSP response frame */
> void sas_ssp_task_response(struct device *dev, struct sas_task *task,
> struct ssp_response_iu *iu)
> @@ -15,11 +21,11 @@ void sas_ssp_task_response(struct device *dev, struct sas_task *task,
>
> tstat->resp = SAS_TASK_COMPLETE;
>
> - if (iu->datapres == 0)
> + if (iu->datapres == NO_DATA)
> tstat->stat = iu->status;
> - else if (iu->datapres == 1)
> + else if (iu->datapres == RESPONSE_DATA)
> tstat->stat = iu->resp_data[3];
> - else if (iu->datapres == 2) {
> + else if (iu->datapres == SENSE_DATA) {
> tstat->stat = SAS_SAM_STAT_CHECK_CONDITION;
> tstat->buf_valid_size =
> min_t(int, SAS_STATUS_BUF_SIZE,

2022-01-27 20:07:23

by John Garry

Subject: Re: [PATCH 01/16] scsi: libsas: Use enum for response frame DATAPRES field

On 27/01/2022 11:31, chenxiang (M) wrote:
>> +
>
> I find that iu->datapres is also used in other drivers with 0/1/2, and
> maybe it is worth to replace all of them with those enum.
>
>   2    290
> /home/chenxiang/kernel_next/kernel-dev/drivers/scsi/aic94xx/aic94xx_tmf.c <<<unknown>>>
>
>              if (ru->datapres == 1)
> 5   1055
> /home/chenxiang/kernel_next/kernel-dev/drivers/scsi/isci/request.c
> <<<unknown>>>
>              if (datapres == 1 || datapres == 2) {
>    6   1740
> /home/chenxiang/kernel_next/kernel-dev/drivers/scsi/isci/request.c
> <<<unknown>>>
>              if (resp_iu->datapres == 0x01 ||
>    7   1741
> /home/chenxiang/kernel_next/kernel-dev/drivers/scsi/isci/request.c
> <<<unknown>>>
>                  resp_iu->datapres == 0x02) {
>  17   1641
> /home/chenxiang/kernel_next/kernel-dev/drivers/scsi/mvsas/mv_sas.c
> <<<unknown>>>
>              iu->datapres = 2;
>
>>   /* fill task_status_struct based o

OK, I can move that enum to libsas.h and use in those drivers.

But I will also check that they are not duplicating code already in libsas.h

Thanks,
John