> On Thu, Nov 03, 2022 at 03:54:49PM +0100, Jan Dabros wrote:
> > Instead of using static functions tpm_cr50_request_locality and
> > tpm_cr50_release_locality register callbacks from tpm class chip->ops
> > created for this purpose.
> >
> > Signed-off-by: Jan Dabros <jsd@xxxxxxxxxxxx>
>
> I get that architecturally using the standard callbacks is a good idea.
> Still, you should explicitly document the gain because the existing code
> is working and field tested.
ACK. I will mention here about the overall idea I have.
>
> > ---
> > drivers/char/tpm/tpm_tis_i2c_cr50.c | 106 ++++++++++++++++++----------
> > 1 file changed, 70 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > index 77cea5b31c6e4..517d8410d7da0 100644
> > --- a/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
> > @@ -17,6 +17,7 @@
> > */
> >
> > #include <linux/acpi.h>
> > +#include <linux/bug.h>
> > #include <linux/completion.h>
> > #include <linux/i2c.h>
> > #include <linux/interrupt.h>
> > @@ -35,6 +36,7 @@
> > #define TPM_CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C errors */
> > #define TPM_CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs between retries on I2C */
> > #define TPM_CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs between retries on I2C */
> > +#define TPM_CR50_I2C_DEFAULT_LOC 0
> >
> > #define TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4))
> > #define TPM_I2C_STS(l) (0x0001 | ((l) << 4))
> > @@ -286,20 +288,21 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer,
> > }
> >
> > /**
> > - * 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.
> > * - -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;
> >
> > @@ -312,48 +315,57 @@ static int tpm_cr50_check_locality(struct tpm_chip *chip)
> > /**
> > * tpm_cr50_release_locality() - Release TPM locality.
> > * @chip: A TPM chip.
> > - * @force: Flag to force release if set.
> > + * @loc: Locality to be released
> > + *
> > + * Return:
> > + * - 0: Success.
> > + * - -errno: A POSIX error code.
> > */
> > -static void tpm_cr50_release_locality(struct tpm_chip *chip, bool force)
> > +static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc)
> > {
> > u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING;
> > - u8 addr = TPM_I2C_ACCESS(0);
> > + u8 addr = TPM_I2C_ACCESS(loc);
> > u8 buf;
> > + int rc;
> >
> > - if (tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf)) < 0)
> > - return;
> > + rc = tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf));
> > + if (rc < 0)
> > + return rc;
> >
> > - if (force || (buf & mask) == mask) {
> > + if ((buf & mask) == mask) {
> > buf = TPM_ACCESS_ACTIVE_LOCALITY;
> > - tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf));
> > + rc = tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf));
> > }
> > +
> > + return rc;
> > }
> >
> > /**
> > - * 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.
> > * - -errno: A POSIX error code.
> > */
> > -static int tpm_cr50_request_locality(struct tpm_chip *chip)
> > +static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc)
> > {
> > u8 buf = TPM_ACCESS_REQUEST_USE;
> > unsigned long stop;
> > int rc;
> >
> > - if (!tpm_cr50_check_locality(chip))
> > - return 0;
> > + if (!tpm_cr50_check_locality(chip, loc))
> > + return loc;
> >
> > - rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf));
> > + rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf));
> > if (rc < 0)
> > return rc;
> >
> > stop = jiffies + chip->timeout_a;
> > do {
> > - if (!tpm_cr50_check_locality(chip))
> > - return 0;
> > + if (!tpm_cr50_check_locality(chip, loc))
> > + return loc;
> >
> > msleep(TPM_CR50_TIMEOUT_SHORT_MS);
> > } while (time_before(jiffies, stop));
> > @@ -374,7 +386,12 @@ 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)
> > + if (chip->locality < 0){
> > + WARN_ONCE(1, "Incorrect tpm locality value\n");
>
> Never ever add WARN() for a success case. It can ultimately crash the whole
> system, if panic_on_warn is enabled.
>
> Since this is a success case, judging from the return value, at most you
> should use pr_info() here.
ACK.
Best Regards,
Jan
