On Thu, Jan 5, 2023 at 7:02 PM Jason A. Donenfeld <Jason@xxxxxxxxx> wrote:
>
> In lieu of actually fixing the underlying bug, just allow system suspend
> to continue, so that laptops still go to sleep fine. Later, this can be
> reverted when the real bug is fixed.
So the patch looks fine to me, but since there's still the ChromeOS
discussion pending I'll wait for that to finish.
Perhaps re-send or at least remind me if/when it does?
Also, a query about the printout:
> + if (rc)
> + pr_err("Unable to suspend tpm-%d (error %d), but continuing system suspend\n",
> + chip->dev_num, rc);
so I suspect that 99% of the time the dev_num isn't actually all that
useful, but what *might* be useful is which tpm driver it is.
Just comparing the error dmesg output you had:
..
tpm tpm0: Error (28) sending savestate before suspend
tpm_tis 00:08: PM: __pnp_bus_suspend(): tpm_pm_suspend+0x0/0x80 returns 28
..
that "tpm tpm0" output is kind of useless compared to the "tpm_tis 00:08" one.
So I think "dev_err(dev, ...)" would be more useful here.
Finally - and maybe I'm just being difficult here, I will note here
again that TPM2 devices don't have this issue, because the TPM2 path
for suspend doesn't do any of this at all.
It just does
tpm_transmit_cmd(..);
with a TPM2_CC_SHUTDOWN TPM_SU_STATE command, and doesn't even check
the return value. In fact, the tpm2 code *used* to have this comment:
/* In places where shutdown command is sent there's no much we can do
* except print the error code on a system failure.
*/
if (rc < 0 && rc != -EPIPE)
dev_warn(&chip->dev, "transmit returned %d while
stopping the TPM",
rc);
but it was summarily removed when doing some re-organization around
buffer handling.
So just by looking at what tpm2 does, I'm not 100% convinced that tpm1
should do this dance at all.
But having a dev_err() is probably a good idea at least as a transitional thing.
Linus
