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

[Nayna Jain]

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