Re: [PATCH v6 03/20] tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c

From: Nayna Jain <nayna@linux.ibm.com>
To: "Winkler, Tomas" <tomas.winkler@intel.com>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	Jason Gunthorpe <jgg@ziepe.ca>,
	Nayna Jain <nayna@linux.vnet.ibm.com>,
	"Usyskin, Alexander" <alexander.usyskin@intel.com>,
	"Struk, Tadeusz" <tadeusz.struk@intel.com>,
	"linux-integrity@vger.kernel.org"
	<linux-integrity@vger.kernel.org>,
	"linux-security-module@vger.kernel.org" 
	<linux-security-module@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Mimi Zohar <zohar@linux.ibm.com>
Subject: Re: [PATCH v6 03/20] tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c
Date: Wed, 17 Oct 2018 20:37:21 +0530	[thread overview]
Message-ID: <650c00b1-d53e-27e8-3e1f-94e34c2ae2e9@linux.ibm.com> (raw)
In-Reply-To: <5B8DA87D05A7694D9FA63FD143655C1B9D9F4691@hasmsx109.ger.corp.intel.com>

On 10/17/2018 05:54 PM, Winkler, Tomas wrote:
>>
>>>    	ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
>>> -	rc = i2c_nuvoton_wait_for_data_avail(chip,
>>> -					     tpm_calc_ordinal_duration(chip,
>>> -								       ordinal),
>>> -					     &priv->read_queue);
>>> +	duration = tpm1_calc_ordinal_duration(chip, ordinal);
>>
>> This version of the patch didn't address my previous comment - "The original
>> code in the nuvoton driver does not differentiate between TPM 1.2 and TPM
>> 2.0 as it does in tpm_tis_core.c.
>> Before making any changes, I would first fix it, so that it could easily be
>> backported. Only then do the refactoring."
> This patch doesn't change the original behavior, just change the name of the function,  so there is no regression.
> I would suggest there is another bug in those drivers/devices that is orthogonal to this refactoring and should not block this from merging.

The problem is that you are inadvertently fixing a bug without realizing 
it - [Patch 04/20]. Bug fixes should be addressed independently
of this change, so that they can be backported properly.

> According to what you say  it can call just tpm_calc_oridnal_duration() instead of tpm1_calc_ordinal_duration(chip, ordinal),
> but I prefer that someone that has those devices will do that change on top of this series as  I cannot test it.

The problem is:

1. This patch calls tpm1_calc_ordinal_duration for both the TPM 1.2 and 
TPM 2.0.
2. In the next patch, it adds a new function tpm_calc_ordinal_duration 
as a wrapper for both the TPM 1.2 and TPM 2.0. After this change when it 
calls tpm_calc_ordinal_duration(),
it now calls different functions for TPM 1.2 and TPM 2.0. This is a 
change in behavior.

Thanks & Regards,
    - Nayna

> Thanks
> Tomas
>