Re: [PATCH 2/2 v2] scsi: add scsi trace core function and put trace points

[Kei Tokunaga <tokunaga.keiich@jp.fujitsu.com>]

Martin K. Petersen wrote:
>>>>>> "Kei" == Kei Tokunaga <tokunaga.keiich@jp.fujitsu.com> writes:
> 
> I'm traveling so I won't have time to look at this closely until next
> week.  However, this caught my eye:
> 
> +static const char *
> +scsi_trace_varlen(struct trace_seq *p, unsigned char *cdb, int len)
> +{
> +	switch (SERVICE_ACTION(cdb)) {
> +	case READ_32:
> +	case WRITE_32:
> +		/* if protection is enabled */
> +		if (((cdb[10] >> 5) & 0x7) == 1)
> +			return scsi_trace_rw32(p, cdb, len);
> +		/* fall through */
> +	default:
> +		return scsi_trace_misc(p, cdb, len);
> +	}
> +}
> 
> It is not a requirement that a 32-byte READ/WRITE request must have
> PROTECT set.  So that if statement is bogus.

Yes.  I agree that the if statement is not necessary here.  I'll
fix it.  Thanks for having your time for the review.

Kei