Re: [PATCH v3 2/4] tpm: Implement tpm_chip_find() for other subsystems to find a TPM chip

[Jason Gunthorpe]

On Fri, Jun 22, 2018 at 04:45:47PM -0400, Stefan Berger wrote:
> On 06/22/2018 04:43 PM, Jason Gunthorpe wrote:
> >On Fri, Jun 22, 2018 at 12:46:11PM -0400, Stefan Berger wrote:
> >>Implement tpm_chip_find() for other subsystems to find a TPM chip and
> >>get a reference to that chip.
> >>
> >>Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx>
> >>  drivers/char/tpm/tpm-chip.c | 27 +++++++++++++++++++++++++++
> >>  include/linux/tpm.h         |  5 +++++
> >>  2 files changed, 32 insertions(+)
> >>
> >>diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> >>index 4e83695af068..2520555b1e17 100644
> >>+++ b/drivers/char/tpm/tpm-chip.c
> >>@@ -81,6 +81,33 @@ void tpm_put_ops(struct tpm_chip *chip)
> >>  EXPORT_SYMBOL_GPL(tpm_put_ops);
> >>  /**
> >>+ * tpm_chip_find() - find a TPM chip and get a reference to it
> >>+ */
> >>+struct tpm_chip *tpm_chip_find(void)
> >I feel like this should be called 'tpm_default_chip()'
> >
> >>+{
> >>+	struct tpm_chip *chip, *res = NULL;
> >>+	int chip_num = 0;
> >>+	int chip_prev;
> >>+
> >>+	mutex_lock(&idr_lock);
> >>+
> >>+	do {
> >>+		chip_prev = chip_num;
> >>+		chip = idr_get_next(&dev_nums_idr, &chip_num);
> >>+		if (chip) {
> >>+			get_device(&chip->dev);
> >>+			res = chip;
> >>+			break;
> >>+		}
> >>+	} while (chip_prev != chip_num);
> >>+
> >>+	mutex_unlock(&idr_lock);
> >And what was tpm_chip_find_get should just call this function..
> And then after that each time tpm_get_ops() ?

It is best to keep the tpm_chip_find_get/tpm_put_ops for internal use
as it also manages the kref lifetime of chip in a subtle way, it
relies on the ops lock not the kref to keep the memory valid when it
has a NULL parameter..

Something like this:

struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip)
{
    int rc;

    if (chip) {
          if (!tpm_try_get_ops(chip))
	      return NULL;
	  return chip;
    }

    chip = tpm_default_chip();
    rc = tpm_try_get_ops(chip));
    put_device(&chip->dev);

    if (rc)
           return NULL;
    return chip;
}

Jason