Re: [PATCH] tpm: Add flag to use default cancellation policy

[Jarkko Sakkinen <jarkko@kernel.org>]

On Wed, Sep 28, 2022 at 05:10:25AM +0000, Joel Stanley wrote:
> On Thu, 8 Sept 2022 at 13:53, Eddie James <eajames@linux.ibm.com> wrote:
> >
> >
> > On 9/8/22 00:22, Jarkko Sakkinen wrote:
> > > On Wed, Sep 07, 2022 at 11:43:17AM -0500, Eddie James wrote:
> > >> The check for cancelled request depends on the VID of the chip, but
> > >> some chips share VID which shouldn't share their cancellation
> > >> behavior. This is the case for the Nuvoton NPCT75X, which should use
> > >> the default cancellation check, not the Winbond one.
> > >> To avoid changing the existing behavior, add a new flag to indicate
> > >> that the chip should use the default cancellation check and set it
> > >> for the I2C TPM2 TIS driver.
> > >>
> > >> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> > >> ---
> > >>   drivers/char/tpm/tpm_tis_core.c | 18 ++++++++++--------
> > >>   drivers/char/tpm/tpm_tis_core.h |  1 +
> > >>   drivers/char/tpm/tpm_tis_i2c.c  |  1 +
> > >>   3 files changed, 12 insertions(+), 8 deletions(-)
> > >>
> > >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > >> index 757623bacfd5..175e75337395 100644
> > >> --- a/drivers/char/tpm/tpm_tis_core.c
> > >> +++ b/drivers/char/tpm/tpm_tis_core.c
> > >> @@ -682,15 +682,17 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
> > >>   {
> > >>      struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> > >>
> > >> -    switch (priv->manufacturer_id) {
> > >> -    case TPM_VID_WINBOND:
> > >> -            return ((status == TPM_STS_VALID) ||
> > >> -                    (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
> > >> -    case TPM_VID_STM:
> > >> -            return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
> > >> -    default:
> > >> -            return (status == TPM_STS_COMMAND_READY);
> > >> +    if (!test_bit(TPM_TIS_DEFAULT_CANCELLATION, &priv->flags)) {
> > >> +            switch (priv->manufacturer_id) {
> > >> +            case TPM_VID_WINBOND:
> > >> +                    return ((status == TPM_STS_VALID) ||
> > >> +                            (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
> > >> +            case TPM_VID_STM:
> > >> +                    return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
> > >> +            }
> > > Why there is no default: ?
> >
> >
> > Well I didn't want to duplicate the line "status ==
> > TPM_STS_COMMAND_READY" in the default case and for the flagged case. So
> > now the switch just falls through for default. I can add default: break
> > instead
> 
> This code was in the original patch series submitted by Nuvoton:
> 
> https://lore.kernel.org/r/20211104140211.6258-3-amirmizi6@gmail.com
> 
> Perhaps something like that would be better?

The current patch could have 

        default:
                /* fall-through */
                break;

BR, Jarkko