RE: [PATCH v6 03/20] tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c
From: Winkler, Tomas
Date: Wed Oct 17 2018 - 11:28:35 EST
> 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.
> 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.