On Mon, Jun 19, 2017 at 2:43 AM, PrasannaKumar Muralidharan wrote:
> On 19 June 2017 at 11:51, Herbert Xu wrote:
>> On Sun, Jun 18, 2017 at 10:00:17PM -0700, Mike Frysinger wrote:
>>>
>>> in order to make tpm-rng react in the way you're implying, the TPM
>>> subsystem would need to add a notification chain for transitions from
>>> none<->some devices, then tpm-rng could subscribe to that, and during
>>> those transition points, it would call hwrng_register/hwrng_unregister
>>> to make itself visible accordingly to the hwrng subsystem. maybe
>>> someone on the TPM side would be interested in writing all that logic,
>>> but it sounds excessive for this minor usage. the current tpm-rng
>>> driver is *extremely* simple -- it's 3 funcs, each of which are 1
>>> line.
>>
>> It's simple and it's broken, as far as the way it hooks into the
>> hwrng is concerned.
>
> *********************************************************************************************
> diff --git a/drivers/char/hw_random/tpm-rng.c b/drivers/char/hw_random/tpm-rng.c
> index d6d4482..4861b35 100644
> --- a/drivers/char/hw_random/tpm-rng.c
> +++ b/drivers/char/hw_random/tpm-rng.c
> @@ -22,6 +22,10 @@
> #include <linux/tpm.h>
>
> #define MODULE_NAME "tpm-rng"
> +#define MAX_RETRIES 30
> +
> +static struct delayed_work check_tpm_work;
> +static int retry_count;
>
> static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> {
> @@ -33,9 +37,27 @@ static struct hwrng tpm_rng = {
> .read = tpm_rng_read,
> };
>
> +static void check_tpm_presence(struct work_struct *work)
> +{
> + u8 data = 0;
> + if (tpm_get_random(TPM_ANY_NUM, &data, 1) > 0) {
> + hwrng_register(&tpm_rng);
> + } else {
> + if (retry_count < MAX_RETRIES) {
> + retry_count++;
> + schedule_delayed_work(&check_tpm_work, HZ * 10);
> + } else {
> + pr_err("Could not find any TPM chip, not
> registering rng");
> + }
> + }
> +}
> +
> static int __init rng_init(void)
> {
> - return hwrng_register(&tpm_rng);
> + INIT_DELAYED_WORK(&check_tpm_work, check_tpm_presence);
> + check_tpm_presence(NULL);
> +
> + return 0;
> }
> module_init(rng_init);
> *********************************************************************************************
>
> Why not something like this? Patch is completely untested. If this
> idea seems useful I can clean the code but would require help in
> testing.
first, that's not how deferred device probing works in the kernel.
drivers shouldn't be doing their own sleeping. but we can ignore that
because no amount of delay/retries will work -- TPMs can come & go at
anytime via hotplugging or module loading/unloading. so the only way
to pull it off would be to do something like what i described --
extending the tpm framework so that it can signal children to come
up/go down.
imo, standing all of that up is over-engineering and not worth the
effort, so i'm not going to do it. but maybe you can convince some of
the TPM maintainers it's worthwhile.
-mike
