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

On 10/17/2018 05:54 PM, Winkler, Tomas wrote:

quoted

quoted

quoted

   	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.

quoted

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.
 

quoted

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