Re: [RFC PATCH v3 1/8] tpm: Add a single table for all TPM wait limits

[Surendran Kanagaraj <surenkj@amazon.com>]

Hi Jarkko,
Thanks for the review.

On Fri, Nov 13, 2026 at 09:45:13PM +0200, Jarkko Sakkinen wrote:
> On Thu, Nov 12, 2026 at 04:38:39PM +0000, Surendran Kanagaraj wrote:
> > The hook sets values with tpm_limits_driver_set() or from a quirk
> > table through tpm_limits_apply_quirks(). tpm_poll_timeout() waits on
>
> OK, but ...
>
> I.e. I'm not fully following what is the diffence between these call
> paths.

They are used from the same place. tpm_limits_update(), which is called
from tpm*_get_timeouts(), builds the table. The driver hook
->adjust_limits() runs from tpm_limits_update() and uses
tpm_limits_apply_quirks() to apply the quirks. tpm_limits_apply_quirks()
finds the matching quirk entry and calls tpm_limits_driver_set() to
update the limit.

tpm_limits_update(...)
{
    tpm_limits_stage_defaults(&p, family);
    if (reported)
        tpm_limits_stage_reported(&p, reported);
    if (chip->ops->adjust_limits)
        chip->ops->adjust_limits(chip, &p);
    tpm_limits_stage_validate(&p);
    tpm_limits_commit(chip, &p);
}

Hope this clarifies.

> > +/*
> > + * Handles time limits for a TPM chip.
>
> I'd extend this to just have brief description how this works. Just a
> high-level overview.

Ack. I will add a short overview in the next rev.

> > +static int tpm_poll(struct tpm_chip *chip, u32 timeout_us,
> > +		    enum tpm_limit limit,
> > +		    bool (*cond)(struct tpm_chip *chip, void *arg), void *arg)
> > [...]
> > +int tpm_poll_timeout(struct tpm_chip *chip, enum tpm_limit limit,
> > +		     bool (*cond)(struct tpm_chip *chip, void *arg), void *arg)
> > +{
> > +	if (WARN_ON_ONCE(limit >= TPM_NR_LIMITS))
> > +		return -EINVAL;
> > +
> > +	return tpm_poll(chip, tpm_limits_us(chip, limit), limit, cond, arg);
> > +}
> > +EXPORT_SYMBOL_GPL(tpm_poll_timeout);
> > [...]
> > +int tpm_poll_timeout_us(struct tpm_chip *chip, u32 timeout_us,
> > +			bool (*cond)(struct tpm_chip *chip, void *arg),
> > +			void *arg)
> > +{
> > +	return tpm_poll(chip, timeout_us, TPM_NR_LIMITS, cond, arg);
> > +}
>
> I did not check all the details but could this be reorganized into:
>
> 1. __tpm_poll(): the same as tpm_poll() but exported and defined in
>    drivers/char/tpm.h.
> 2. tpm_poll(): a single static inline function declared fully at
>    drivers/char/tpm.h
>
> __tpm_poll() would e.g., make a nicer trace point for a kprobe or
> kretprobe.

Good point. __tpm_poll() then becomes the single kprobe point. Will do
this in the next rev.

> > +/* per-device overrides based on id the driver defines e.g. DID/VID */
> > +struct tpm_limits_quirk {
> > +	u32 id;				/* driver-defined device identity */
> > +	u32 time_us[TPM_NR_LIMITS];	/* 0 leaves the limit alone */
> > +};
>
> I think it would make more sense to fully initialize the database per
> device, if that is possible.
>
> It is a non-concern if two TPM drivers do some redundant work, as it
> is not a real-world production use case :-)
>
> I'm just wondering if this quirk/entry division.
>
> I.e. in my world view chip owns its limits (not e.g., tpm2_space).

Makes sense. The quirk table is just a convenience, a common table fmt
for drivers, and the chip still owns the table. Without it the driver
calls tpm_limits_driver_set() for each limit it changes. I am happy to
drop it in the next rev.

Thanks,
Surendran