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

[Winkler, Tomas]

> 
> 
> 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.
>
This would be ideal,  but that's happen more than often that the fix cannot be applied w/o changes to older kernels.
I can send that patch, are you able to test it? Frankly nobody complained about it so I'm not sure what to do.
 
> > 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.
That's correct 
> 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.
That's not correct,   I missed that.  Either someone can test the TPM 2.0 device  or I need to revert it for nuvoton, in the next patch.  

Thanks
Tomas