Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v6 03/20] tpm: factor out tpm 1.x duration calculation to
 tpm1-cmd.c
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>
References: <20181017064613.19746-1-tomas.winkler@intel.com>
 <20181017064613.19746-4-tomas.winkler@intel.com>
 <05fea3e6-91a3-b508-83c0-d8f06d6bbd5c@linux.ibm.com>
 <5B8DA87D05A7694D9FA63FD143655C1B9D9F4691@hasmsx109.ger.corp.intel.com>
From:   Nayna Jain <nayna@linux.ibm.com>
Date:   Wed, 17 Oct 2018 20:37:21 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.2
MIME-Version: 1.0
In-Reply-To: <5B8DA87D05A7694D9FA63FD143655C1B9D9F4691@hasmsx109.ger.corp.intel.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Message-Id: <650c00b1-d53e-27e8-3e1f-94e34c2ae2e9@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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
>