Re: [PATCH 12/20] scsi: merge print_opcode_name()

[Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>]

(2014/09/03 19:06), Hannes Reinecke wrote:
> Instead of having two versions of print_opcode_name() we
> should be consolidating them into one version.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/scsi/constants.c | 90 +++++++++++++++++++-----------------------------
>   1 file changed, 35 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
> index 364349c..3aee43b 100644
> --- a/drivers/scsi/constants.c
> +++ b/drivers/scsi/constants.c
> @@ -30,6 +30,11 @@
>   #define THIRD_PARTY_COPY_IN 0x84
>   
>   
> +struct sa_name_list {
> +	int cmd;
> +	const struct value_name_pair *arr;
> +	int arr_sz;
> +};
>   
>   #ifdef CONFIG_SCSI_CONSTANTS
>   static const char * cdb_byte0_names[] = {
> @@ -244,12 +249,6 @@ static const struct value_name_pair variable_length_arr[] = {
>   };
>   #define VARIABLE_LENGTH_SZ ARRAY_SIZE(variable_length_arr)
>   
> -struct sa_name_list {
> -	int cmd;
> -	const struct value_name_pair *arr;
> -	int arr_sz;
> -};
> -
>   static struct sa_name_list sa_names_arr[] = {
>   	{VARIABLE_LENGTH_CMD, variable_length_arr, VARIABLE_LENGTH_SZ},
>   	{MAINTENANCE_IN, maint_in_arr, MAINT_IN_SZ},
> @@ -267,6 +266,28 @@ static struct sa_name_list sa_names_arr[] = {
>   };
>   #define SA_NAME_LIST_SZ ARRAY_SIZE(sa_names_arr)

We can delete SA_NAME_LIST_SZ because we define it out of
CONFIG_SCSI_CONSTANTS.

The others look good.

Thanks,
Yoshihiro YUNOMAE

> +#else /* ifndef CONFIG_SCSI_CONSTANTS */
> +static const char * cdb_byte0_names[] = NULL;
> +
> +static struct sa_name_list sa_names_arr[] = {
> +	{VARIABLE_LENGTH_CMD, NULL, 0},
> +	{MAINTENANCE_IN, NULL, 0},
> +	{MAINTENANCE_OUT, NULL, 0},
> +	{PERSISTENT_RESERVE_IN, NULL, 0},
> +	{PERSISTENT_RESERVE_OUT, NULL, 0},
> +	{SERVICE_ACTION_IN_12, NULL, 0},
> +	{SERVICE_ACTION_OUT_12, NULL, 0},
> +	{SERVICE_ACTION_BIDIRECTIONAL, NULL, 0},
> +	{SERVICE_ACTION_IN_16, NULL, 0},
> +	{SERVICE_ACTION_OUT_16, NULL, 0},
> +	{THIRD_PARTY_COPY_IN, NULL, 0},
> +	{THIRD_PARTY_COPY_OUT, NULL, 0},
> +	{0, NULL, 0},
> +};
> +#endif /* CONFIG_SCSI_CONSTANTS */
> +#define SA_NAME_LIST_SZ ARRAY_SIZE(sa_names_arr)
> +#define CDB_BYTE0_NAMES_SZ ARRAY_SIZE(cdb_byte0_names)
> +
>   static bool scsi_opcode_sa_name(int cmd, int service_action,
>   				const char **sa_name)
>   {
> @@ -316,11 +337,14 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len)
>   
>   	if (!scsi_opcode_sa_name(cdb0, sa, &name)) {
>   		if (cdb0 < 0xc0) {
> -			name = cdb_byte0_names[cdb0];
> -			if (name)
> -				printk("%s", name);
> -			else
> -				printk("cdb[0]=0x%x (reserved)", cdb0);
> +			if (CDB_BYTE0_NAMES_SZ) {
> +				name = cdb_byte0_names[cdb0];
> +				if (name)
> +					printk("%s", name);
> +				else
> +					printk("cdb[0]=0x%x (reserved)", cdb0);
> +			} else
> +				printk("cdb[0]=0x%x", cdb0);
>   		} else
>   			printk("cdb[0]=0x%x (vendor)", cdb0);
>   	} else {
> @@ -334,50 +358,6 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len)
>   	}
>   }
>   
> -#else /* ifndef CONFIG_SCSI_CONSTANTS */
> -
> -static void print_opcode_name(unsigned char * cdbp, int cdb_len)
> -{
> -	int sa, len, cdb0;
> -
> -	cdb0 = cdbp[0];
> -	switch(cdb0) {
> -	case VARIABLE_LENGTH_CMD:
> -		len = scsi_varlen_cdb_length(cdbp);
> -		if (len < 10) {
> -			printk("short opcode=0x%x command, len=%d "
> -			       "ext_len=%d", cdb0, len, cdb_len);
> -			break;
> -		}
> -		sa = (cdbp[8] << 8) + cdbp[9];
> -		printk("cdb[0]=0x%x, sa=0x%x", cdb0, sa);
> -		if (len != cdb_len)
> -			printk(", in_cdb_len=%d, ext_len=%d", len, cdb_len);
> -		break;
> -	case MAINTENANCE_IN:
> -	case MAINTENANCE_OUT:
> -	case PERSISTENT_RESERVE_IN:
> -	case PERSISTENT_RESERVE_OUT:
> -	case SERVICE_ACTION_IN_12:
> -	case SERVICE_ACTION_OUT_12:
> -	case SERVICE_ACTION_BIDIRECTIONAL:
> -	case SERVICE_ACTION_IN_16:
> -	case SERVICE_ACTION_OUT_16:
> -	case THIRD_PARTY_COPY_IN:
> -	case THIRD_PARTY_COPY_OUT:
> -		sa = cdbp[1] & 0x1f;
> -		printk("cdb[0]=0x%x, sa=0x%x", cdb0, sa);
> -		break;
> -	default:
> -		if (cdb0 < 0xc0)
> -			printk("cdb[0]=0x%x", cdb0);
> -		else
> -			printk("cdb[0]=0x%x (vendor)", cdb0);
> -		break;
> -	}
> -}
> -#endif
> -
>   void __scsi_print_command(unsigned char *cdb)
>   {
>   	int k, len;
> 

-- 
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae.ez@hitachi.com