Re: [PATCH 2/7] tpm: reorganize sysfs files

[Dave Hansen]

On Mon, 2006-04-03 at 11:42 -0500, Kylene Jo Hall wrote:
>  ssize_t tpm_show_pcrs(struct device *dev, struct device_attribute *attr,
>  		      char *buf)
>  {
> -	u8 data[READ_PCR_RESULT_SIZE];
> +	u8 data[30];

Is this correct?  Are you guaranteed that this data read will never,
ever exceed 30 bytes?  Any reason it shouldn't be a variable?

>  	ssize_t len;
>  	int i, j, num_pcrs;
>  	__be32 index;
> @@ -150,26 +659,28 @@ ssize_t tpm_show_pcrs(struct device *dev
>  	if (chip == NULL)
>  		return -ENODEV;
>  
> -	memcpy(data, cap_pcr, sizeof(cap_pcr));
> +	memcpy(data, tpm_cap, sizeof(tpm_cap));
> +	data[TPM_CAP_IDX] = TPM_CAP_PROP;
> +	data[TPM_CAP_SUBCAP_IDX] = TPM_CAP_PROP_PCR;
> +
>  	if ((len = tpm_transmit(chip, data, sizeof(data)))
> -	    < CAP_PCR_RESULT_SIZE) {
> +	    <= TPM_ERROR_SIZE) {
>  		dev_dbg(chip->dev, "A TPM error (%d) occurred "
> -				"attempting to determine the number of PCRS\n",
> -			be32_to_cpu(*((__be32 *) (data + 6))));
> +			"attempting to determine the number of PCRS\n",
> +			be32_to_cpu(*((__be32 *) (data + TPM_RET_CODE_IDX))));
>  		return 0;
>  	}

I know this is old code, but I see this little 

	be32_to_cpu(*((__be32 *) (data + TPM_RET_CODE_IDX))));

snippet at least twice.  It is also a bit hard to read.  Seems likt it
would be a great candidate for a little helper function.

Come to think of it, that entire if() sequence appears to be repeated
quite a few times.

>  	num_pcrs = be32_to_cpu(*((__be32 *) (data + 14)));
> -
>  	for (i = 0; i < num_pcrs; i++) {
>  		memcpy(data, pcrread, sizeof(pcrread));
>  		index = cpu_to_be32(i);
>  		memcpy(data + 10, &index, 4);
>  		if ((len = tpm_transmit(chip, data, sizeof(data)))
> -		    < READ_PCR_RESULT_SIZE){
> +		    <= TPM_ERROR_SIZE) {
>  			dev_dbg(chip->dev, "A TPM error (%d) occurred"
>  				" attempting to read PCR %d of %d\n",
> -				be32_to_cpu(*((__be32 *) (data + 6))),
> +				be32_to_cpu(*((__be32 *) (data + TPM_RET_CODE_IDX))),
>  				i, num_pcrs);
>  			goto out;
>  		}
> @@ -208,11 +724,11 @@ ssize_t tpm_show_pubek(struct device *de
>  
>  	memcpy(data, readpubek, sizeof(readpubek));
>  
> -	if ((len = tpm_transmit(chip, data, READ_PUBEK_RESULT_SIZE)) <
> -	    READ_PUBEK_RESULT_SIZE) {
> +	if ((len = tpm_transmit(chip, data, READ_PUBEK_RESULT_SIZE)) <=
> +	    TPM_ERROR_SIZE) {
>  		dev_dbg(chip->dev, "A TPM error (%d) occurred "
> -				"attempting to read the PUBEK\n",
> -			    be32_to_cpu(*((__be32 *) (data + 6))));
> +			"attempting to read the PUBEK\n",
> +			be32_to_cpu(*((__be32 *) (data + TPM_RET_CODE_IDX))));
>  		rc = 0;
>  		goto out;
>  	}
> @@ -250,29 +284,20 @@ out:
>  }
>  EXPORT_SYMBOL_GPL(tpm_show_pubek);
>  
> -#define CAP_VER_RESULT_SIZE 18
> +#define CAP_VERSION_1_1 6
> +#define CAP_VERSION_IDX 13
>  static const u8 cap_version[] = {
>  	0, 193,			/* TPM_TAG_RQU_COMMAND */
>  	0, 0, 0, 18,		/* length */
>  	0, 0, 0, 101,		/* TPM_ORD_GetCapability */
> -	0, 0, 0, 6,
> +	0, 0, 0, 0,
>  	0, 0, 0, 0
>  };
>  
> -#define CAP_MANUFACTURER_RESULT_SIZE 18
> -static const u8 cap_manufacturer[] = {
> -	0, 193,			/* TPM_TAG_RQU_COMMAND */
> -	0, 0, 0, 22,		/* length */
> -	0, 0, 0, 101,		/* TPM_ORD_GetCapability */
> -	0, 0, 0, 5,
> -	0, 0, 0, 4,
> -	0, 0, 1, 3
> -};
> -
>  ssize_t tpm_show_caps(struct device *dev, struct device_attribute *attr,
>  		      char *buf)
>  {
> -	u8 data[sizeof(cap_manufacturer)];
> +	u8 data[30];
>  	ssize_t len;
>  	char *str = buf;
>  
> @@ -282,26 +793,37 @@ ssize_t tpm_show_caps(struct device *dev
>  	if (chip == NULL)
>  		return -ENODEV;
>  
> -	memcpy(data, cap_manufacturer, sizeof(cap_manufacturer));
> +	memcpy(data, tpm_cap, sizeof(tpm_cap));
> +	data[TPM_CAP_IDX] = TPM_CAP_PROP;
> +	data[TPM_CAP_SUBCAP_IDX] = TPM_CAP_PROP_MANUFACTURER;
>  
> -	if ((len = tpm_transmit(chip, data, sizeof(data))) <
> -	    CAP_MANUFACTURER_RESULT_SIZE)
> -		return len;
> +	if ((len = tpm_transmit(chip, data, sizeof(data))) <=
> +	    TPM_ERROR_SIZE) {
> +		dev_dbg(chip->dev, "A TPM error (%d) occurred "
> +			"attempting to determine the manufacturer\n",
> +			be32_to_cpu(*((__be32 *) (data + TPM_RET_CODE_IDX))));
> +		return 0;
> +	}

Since you're going through and modifying these, it might be nice to
change them to the more normal (and readable) style of 


	len = tpm_transmit(chip, data, sizeof(data));
	if (len < CAP_MANUFACTURER_RESULT_SIZE)
		return len;

Note that that doesn't even increase the number of lines of code.

-- Dave

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/