> > /**
> > - * tpm_cr50_check_locality() - Verify TPM locality 0 is active.
> > + * tpm_cr50_check_locality() - Verify if required TPM locality is active.
> > * @chip: A TPM chip.
> > + * @loc: Locality to be verified
> > *
> > * Return:
> > - * - 0: Success.
> > + * - loc: Success.
> > * - -errno: A POSIX error code.
> > */
> > -static int tpm_cr50_check_locality(struct tpm_chip *chip)
> > +static int tpm_cr50_check_locality(struct tpm_chip *chip, int loc)
> > {
> > u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY;
> > u8 buf;
> > int rc;
> >
> > - rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
> > + rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf));
> > if (rc < 0)
> > return rc;
> >
> > if ((buf & mask) == mask)
> > - return 0;
> > + return loc;
> >
> > return -EIO;
> > }
>
> Why is it useful to return the same `loc` value that was passed in,
> rather than just returning `0`? The caller already knows the value of
> `loc`, so they aren't being told anything new.
>
> I think this should continue to return `0` for success.
I agree, I should keep this as it was.
>
>
> > - * tpm_cr50_request_locality() - Request TPM locality 0.
> > + * tpm_cr50_request_locality() - Request TPM locality.
> > * @chip: A TPM chip.
> > + * @loc: Locality to be requested.
> > *
> > * Return:
> > - * - 0: Success.
> > + * - loc: Success.
>
> Same as above. Return `0`.
The case here is that .request_locality callback should return active
locality. This value is assigned to chip->locality inside
tpm_request_locality() [drivers/char/tpm/tpm-chip.c].
>
> > @@ -374,7 +386,9 @@ static u8 tpm_cr50_i2c_tis_status(struct tpm_chip *chip)
> > {
> > u8 buf[4];
> >
> > - if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(0), buf, sizeof(buf)) < 0)
> > + WARN_ONCE((chip->locality < 0), "Incorrect tpm locality value\n");
>
> For each of these ` WARN_ONCE((chip->locality < 0), ...).`, can it
> return immediately rather than attempting to continue using an invalid
> locality value? Do the following commands have a chance of succeeding
> with the invalid value?
I agree - it makes more sense to return immediately instead of trying
to send invalid configuration over i2c.
Best Regards,
Jan
