| Message ID | 20250402172134.7751-1-msuchanek@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tpm: tis: Increase the default for timeouts B and C | expand |
On Wed, Apr 02, 2025 at 07:21:30PM +0200, Michal Suchanek wrote: >With some Infineon chips the timeouts in tpm_tis_send_data (both B and >C) can reach up to about 2250 ms. > >Extend the timeout duration to accommodate this. The problem here is the bump of timeout_c is going to interact poorly with the Infineon errata workaround, as now we'll wait 4s instead of 200ms to detect the stuck status change. (Also shouldn't timeout_c already end up as 750ms, as it's max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_C), and TIS_SHORT_TIMEOUT is 750 vs 200 for TPM2_TIMEOUT_C? That doesn't seem to be borne out by your logs, nor my results.) >Link: https://lore.kernel.org/linux-integrity/Z5pI07m0Muapyu9w@kitsune.suse.cz/ >Signed-off-by: Michal Suchanek <msuchanek@suse.de> >--- >An alternative would be to add an entry to vendor_timeout_overrides but >I do not know how to determine the chip IDs to put into this table. >--- > drivers/char/tpm/tpm_tis_core.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h >index 970d02c337c7..1ff565be2175 100644 >--- a/drivers/char/tpm/tpm_tis_core.h >+++ b/drivers/char/tpm/tpm_tis_core.h >@@ -54,7 +54,7 @@ enum tis_int_flags { > enum tis_defaults { > TIS_MEM_LEN = 0x5000, > TIS_SHORT_TIMEOUT = 750, /* ms */ >- TIS_LONG_TIMEOUT = 2000, /* 2 sec */ >+ TIS_LONG_TIMEOUT = 4000, /* 2 sec */ > TIS_TIMEOUT_MIN_ATML = 14700, /* usecs */ > TIS_TIMEOUT_MAX_ATML = 15000, /* usecs */ > }; >@@ -64,7 +64,7 @@ enum tis_defaults { > */ > #define TIS_TIMEOUT_A_MAX max_t(int, TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_A) > #define TIS_TIMEOUT_B_MAX max_t(int, TIS_LONG_TIMEOUT, TPM2_TIMEOUT_B) >-#define TIS_TIMEOUT_C_MAX max_t(int, TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_C) >+#define TIS_TIMEOUT_C_MAX max_t(int, TIS_LONG_TIMEOUT, TPM2_TIMEOUT_C) > #define TIS_TIMEOUT_D_MAX max_t(int, TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_D) > > #define TPM_ACCESS(l) (0x0000 | ((l) << 12)) >-- >2.47.1 > J.
On Wed, Apr 02, 2025 at 06:45:40PM +0100, Jonathan McDowell wrote: > On Wed, Apr 02, 2025 at 07:21:30PM +0200, Michal Suchanek wrote: > > With some Infineon chips the timeouts in tpm_tis_send_data (both B and > > C) can reach up to about 2250 ms. > > > > Extend the timeout duration to accommodate this. > > The problem here is the bump of timeout_c is going to interact poorly with > the Infineon errata workaround, as now we'll wait 4s instead of 200ms to > detect the stuck status change. Yes, that's problematic. Is it possible to detect the errata by anything other than waiting for the timeout to expire? > > (Also shouldn't timeout_c already end up as 750ms, as it's > max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_C), and TIS_SHORT_TIMEOUT is 750 vs 200 > for TPM2_TIMEOUT_C? That doesn't seem to be borne out by your logs, nor my > results.) Indeed, it should be 750ms but the logs show 200ms. I do not see where it could get reduced, nor any significan difference between the mainline code and the kernel I am using in this area. Thanks Michal > > > Link: https://lore.kernel.org/linux-integrity/Z5pI07m0Muapyu9w@kitsune.suse.cz/ > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > > --- > > An alternative would be to add an entry to vendor_timeout_overrides but > > I do not know how to determine the chip IDs to put into this table. > > --- > > drivers/char/tpm/tpm_tis_core.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h > > index 970d02c337c7..1ff565be2175 100644 > > --- a/drivers/char/tpm/tpm_tis_core.h > > +++ b/drivers/char/tpm/tpm_tis_core.h > > @@ -54,7 +54,7 @@ enum tis_int_flags { > > enum tis_defaults { > > TIS_MEM_LEN = 0x5000, > > TIS_SHORT_TIMEOUT = 750, /* ms */ > > - TIS_LONG_TIMEOUT = 2000, /* 2 sec */ > > + TIS_LONG_TIMEOUT = 4000, /* 2 sec */ > > TIS_TIMEOUT_MIN_ATML = 14700, /* usecs */ > > TIS_TIMEOUT_MAX_ATML = 15000, /* usecs */ > > }; > > @@ -64,7 +64,7 @@ enum tis_defaults { > > */ > > #define TIS_TIMEOUT_A_MAX max_t(int, TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_A) > > #define TIS_TIMEOUT_B_MAX max_t(int, TIS_LONG_TIMEOUT, TPM2_TIMEOUT_B) > > -#define TIS_TIMEOUT_C_MAX max_t(int, TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_C) > > +#define TIS_TIMEOUT_C_MAX max_t(int, TIS_LONG_TIMEOUT, TPM2_TIMEOUT_C) > > #define TIS_TIMEOUT_D_MAX max_t(int, TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_D) > > > > #define TPM_ACCESS(l) (0x0000 | ((l) << 12)) > > -- > > 2.47.1 > > > > J. > > -- > ... "Tom's root boot is the Linux world equivalent of a 'get out of jail > free' card. The man is a *hero*." -- Simon Brooke, ucol.
On Wed, Apr 02, 2025 at 10:07:40PM +0200, Michal Suchánek wrote: > On Wed, Apr 02, 2025 at 06:45:40PM +0100, Jonathan McDowell wrote: > > On Wed, Apr 02, 2025 at 07:21:30PM +0200, Michal Suchanek wrote: > > > With some Infineon chips the timeouts in tpm_tis_send_data (both B and > > > C) can reach up to about 2250 ms. > > > > > > Extend the timeout duration to accommodate this. > > > > The problem here is the bump of timeout_c is going to interact poorly with > > the Infineon errata workaround, as now we'll wait 4s instead of 200ms to > > detect the stuck status change. > > Yes, that's problematic. Is it possible to detect the errata by anything > other than waiting for the timeout to expire? > > > > > (Also shouldn't timeout_c already end up as 750ms, as it's > > max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_C), and TIS_SHORT_TIMEOUT is 750 vs 200 > > for TPM2_TIMEOUT_C? That doesn't seem to be borne out by your logs, nor my > > results.) > > Indeed, it should be 750ms but the logs show 200ms. I do not see > where it could get reduced, nor any significan difference between the > mainline code and the kernel I am using in this area. This would come from drivers/char/tpm/tpm2-cmd.c: chip->timeout_c = msecs_to_jiffies(TPM2_TIMEOUT_C); include/linux/tpm.h: TPM2_TIMEOUT_C = 200, So this would need also adjusting. Thanks Michal > > Thanks > > Michal > > > > > > Link: https://lore.kernel.org/linux-integrity/Z5pI07m0Muapyu9w@kitsune.suse.cz/ > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > > > --- > > > An alternative would be to add an entry to vendor_timeout_overrides but > > > I do not know how to determine the chip IDs to put into this table. > > > --- > > > drivers/char/tpm/tpm_tis_core.h | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h > > > index 970d02c337c7..1ff565be2175 100644 > > > --- a/drivers/char/tpm/tpm_tis_core.h > > > +++ b/drivers/char/tpm/tpm_tis_core.h > > > @@ -54,7 +54,7 @@ enum tis_int_flags { > > > enum tis_defaults { > > > TIS_MEM_LEN = 0x5000, > > > TIS_SHORT_TIMEOUT = 750, /* ms */ > > > - TIS_LONG_TIMEOUT = 2000, /* 2 sec */ > > > + TIS_LONG_TIMEOUT = 4000, /* 2 sec */ > > > TIS_TIMEOUT_MIN_ATML = 14700, /* usecs */ > > > TIS_TIMEOUT_MAX_ATML = 15000, /* usecs */ > > > }; > > > @@ -64,7 +64,7 @@ enum tis_defaults { > > > */ > > > #define TIS_TIMEOUT_A_MAX max_t(int, TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_A) > > > #define TIS_TIMEOUT_B_MAX max_t(int, TIS_LONG_TIMEOUT, TPM2_TIMEOUT_B) > > > -#define TIS_TIMEOUT_C_MAX max_t(int, TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_C) > > > +#define TIS_TIMEOUT_C_MAX max_t(int, TIS_LONG_TIMEOUT, TPM2_TIMEOUT_C) > > > #define TIS_TIMEOUT_D_MAX max_t(int, TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_D) > > > > > > #define TPM_ACCESS(l) (0x0000 | ((l) << 12)) > > > -- > > > 2.47.1 > > > > > > > J. > > > > -- > > ... "Tom's root boot is the Linux world equivalent of a 'get out of jail > > free' card. The man is a *hero*." -- Simon Brooke, ucol.
On Wed, Apr 02, 2025 at 10:07:39PM +0200, Michal Suchánek wrote: >On Wed, Apr 02, 2025 at 06:45:40PM +0100, Jonathan McDowell wrote: >> On Wed, Apr 02, 2025 at 07:21:30PM +0200, Michal Suchanek wrote: >> > With some Infineon chips the timeouts in tpm_tis_send_data (both B and >> > C) can reach up to about 2250 ms. >> > >> > Extend the timeout duration to accommodate this. >> >> The problem here is the bump of timeout_c is going to interact poorly with >> the Infineon errata workaround, as now we'll wait 4s instead of 200ms to >> detect the stuck status change. > >Yes, that's problematic. Is it possible to detect the errata by anything >other than waiting for the timeout to expire? Not that I'm aware of, nor have seen in my experimentation. It's a "stuck" status, so the timeout is how it's detected. OOI, have you tried back porting the fixes that are in mainline for 6.15 to your frankenkernel? I _think_ the errata fix might end up resolving at least the timeout for valid for you, as a side effect? We're currently rolling them out across our fleet, but I don't have enough runtime yet to be sure they've sorted all the timeout instances we see. J.
On Thu, Apr 03, 2025 at 12:00:36PM +0100, Jonathan McDowell wrote: > On Wed, Apr 02, 2025 at 10:07:39PM +0200, Michal Suchánek wrote: > > On Wed, Apr 02, 2025 at 06:45:40PM +0100, Jonathan McDowell wrote: > > > On Wed, Apr 02, 2025 at 07:21:30PM +0200, Michal Suchanek wrote: > > > > With some Infineon chips the timeouts in tpm_tis_send_data (both B and > > > > C) can reach up to about 2250 ms. > > > > > > > > Extend the timeout duration to accommodate this. > > > > > > The problem here is the bump of timeout_c is going to interact poorly with > > > the Infineon errata workaround, as now we'll wait 4s instead of 200ms to > > > detect the stuck status change. > > > > Yes, that's problematic. Is it possible to detect the errata by anything > > other than waiting for the timeout to expire? > > Not that I'm aware of, nor have seen in my experimentation. It's a "stuck" > status, so the timeout is how it's detected. > > OOI, have you tried back porting the fixes that are in mainline for 6.15 to > your frankenkernel? I _think_ the errata fix might end up resolving at least > the timeout for valid for you, as a side effect? We're currently rolling > them out across our fleet, but I don't have enough runtime yet to be sure > they've sorted all the timeout instances we see. When was that merged? The change I see is that sometimes EAGAIN is returned instead of ETIME but based on the previous discussion this is unlikely to help. Thanks Michal > > J. > > -- > /-\ | He's weird? It's ok, I'm fluent in > |@/ Debian GNU/Linux Developer | weird. > \- |
On Thu, Apr 03, 2025 at 01:56:37PM +0200, Michal Suchánek wrote: >On Thu, Apr 03, 2025 at 12:00:36PM +0100, Jonathan McDowell wrote: >> On Wed, Apr 02, 2025 at 10:07:39PM +0200, Michal Suchánek wrote: >> > On Wed, Apr 02, 2025 at 06:45:40PM +0100, Jonathan McDowell wrote: >> > > On Wed, Apr 02, 2025 at 07:21:30PM +0200, Michal Suchanek wrote: >> > > > With some Infineon chips the timeouts in tpm_tis_send_data (both B and >> > > > C) can reach up to about 2250 ms. >> > > > >> > > > Extend the timeout duration to accommodate this. >> > > >> > > The problem here is the bump of timeout_c is going to interact poorly with >> > > the Infineon errata workaround, as now we'll wait 4s instead of 200ms to >> > > detect the stuck status change. >> > >> > Yes, that's problematic. Is it possible to detect the errata by anything >> > other than waiting for the timeout to expire? >> >> Not that I'm aware of, nor have seen in my experimentation. It's a "stuck" >> status, so the timeout is how it's detected. >> >> OOI, have you tried back porting the fixes that are in mainline for 6.15 to >> your frankenkernel? I _think_ the errata fix might end up resolving at least >> the timeout for valid for you, as a side effect? We're currently rolling >> them out across our fleet, but I don't have enough runtime yet to be sure >> they've sorted all the timeout instances we see. > >When was that merged? It hit Linus' tree last Friday I believe. >The change I see is that sometimes EAGAIN is returned instead of ETIME >but based on the previous discussion this is unlikely to help. That sounds like you might have picked up the version with the typo that I posted to the list; it got fixed up before making it to mainline. The two patches I've backported locally are in mainline as: 7146dffa875cd00e7a7f918e1fce79c7593ac1fa tpm, tpm_tis: Fix timeout handling when waiting for TPM status de9e33df7762abbfc2a1568291f2c3a3154c6a9d tpm, tpm_tis: Workaround failed command reception on Infineon devices J.
On Thu, Apr 03, 2025 at 02:00:26PM +0100, Jonathan McDowell wrote: > On Thu, Apr 03, 2025 at 01:56:37PM +0200, Michal Suchánek wrote: > > On Thu, Apr 03, 2025 at 12:00:36PM +0100, Jonathan McDowell wrote: > > > On Wed, Apr 02, 2025 at 10:07:39PM +0200, Michal Suchánek wrote: > > > > On Wed, Apr 02, 2025 at 06:45:40PM +0100, Jonathan McDowell wrote: > > > > > On Wed, Apr 02, 2025 at 07:21:30PM +0200, Michal Suchanek wrote: > > > > > > With some Infineon chips the timeouts in tpm_tis_send_data (both B and > > > > > > C) can reach up to about 2250 ms. > > > > > > > > > > > > Extend the timeout duration to accommodate this. > > > > > > > > > > The problem here is the bump of timeout_c is going to interact poorly with > > > > > the Infineon errata workaround, as now we'll wait 4s instead of 200ms to > > > > > detect the stuck status change. > > > > > > > > Yes, that's problematic. Is it possible to detect the errata by anything > > > > other than waiting for the timeout to expire? > > > > > > Not that I'm aware of, nor have seen in my experimentation. It's a "stuck" > > > status, so the timeout is how it's detected. > > > > > > OOI, have you tried back porting the fixes that are in mainline for 6.15 to > > > your frankenkernel? I _think_ the errata fix might end up resolving at least > > > the timeout for valid for you, as a side effect? We're currently rolling > > > them out across our fleet, but I don't have enough runtime yet to be sure > > > they've sorted all the timeout instances we see. > > > > When was that merged? > > It hit Linus' tree last Friday I believe. > > > The change I see is that sometimes EAGAIN is returned instead of ETIME > > but based on the previous discussion this is unlikely to help. > > That sounds like you might have picked up the version with the typo that I > posted to the list; it got fixed up before making it to mainline. The two > patches I've backported locally are in mainline as: > > 7146dffa875cd00e7a7f918e1fce79c7593ac1fa tpm, tpm_tis: Fix timeout handling when waiting for TPM status > de9e33df7762abbfc2a1568291f2c3a3154c6a9d tpm, tpm_tis: Workaround failed command reception on Infineon devices Indeed, it adds a retry in tpm_send_main as well. That might work, needs some testing on the affected hardware. With that changing only the B timeout should suffice. Thanks Michal
On Wed, Apr 02, 2025 at 07:21:30PM +0200, Michal Suchanek wrote: > With some Infineon chips the timeouts in tpm_tis_send_data (both B and > C) can reach up to about 2250 ms. > > Extend the timeout duration to accommodate this. > > Link: https://lore.kernel.org/linux-integrity/Z5pI07m0Muapyu9w@kitsune.suse.cz/ > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > --- > An alternative would be to add an entry to vendor_timeout_overrides but > I do not know how to determine the chip IDs to put into this table. > --- > drivers/char/tpm/tpm_tis_core.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h > index 970d02c337c7..1ff565be2175 100644 > --- a/drivers/char/tpm/tpm_tis_core.h > +++ b/drivers/char/tpm/tpm_tis_core.h > @@ -54,7 +54,7 @@ enum tis_int_flags { > enum tis_defaults { > TIS_MEM_LEN = 0x5000, > TIS_SHORT_TIMEOUT = 750, /* ms */ > - TIS_LONG_TIMEOUT = 2000, /* 2 sec */ > + TIS_LONG_TIMEOUT = 4000, /* 2 sec */ /* 4 secs */ > TIS_TIMEOUT_MIN_ATML = 14700, /* usecs */ > TIS_TIMEOUT_MAX_ATML = 15000, /* usecs */ > }; > @@ -64,7 +64,7 @@ enum tis_defaults { > */ > #define TIS_TIMEOUT_A_MAX max_t(int, TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_A) > #define TIS_TIMEOUT_B_MAX max_t(int, TIS_LONG_TIMEOUT, TPM2_TIMEOUT_B) > -#define TIS_TIMEOUT_C_MAX max_t(int, TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_C) > +#define TIS_TIMEOUT_C_MAX max_t(int, TIS_LONG_TIMEOUT, TPM2_TIMEOUT_C) > #define TIS_TIMEOUT_D_MAX max_t(int, TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_D) > > #define TPM_ACCESS(l) (0x0000 | ((l) << 12)) > -- > 2.47.1 > > BR, Jarkko
On Wed, Apr 02, 2025 at 06:45:40PM +0100, Jonathan McDowell wrote: > On Wed, Apr 02, 2025 at 07:21:30PM +0200, Michal Suchanek wrote: > > With some Infineon chips the timeouts in tpm_tis_send_data (both B and > > C) can reach up to about 2250 ms. > > > > Extend the timeout duration to accommodate this. > > The problem here is the bump of timeout_c is going to interact poorly with > the Infineon errata workaround, as now we'll wait 4s instead of 200ms to > detect the stuck status change. > > (Also shouldn't timeout_c already end up as 750ms, as it's > max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_C), and TIS_SHORT_TIMEOUT is 750 vs 200 > for TPM2_TIMEOUT_C? That doesn't seem to be borne out by your logs, nor my > results.) Just noticed that the commit did not end up having fixes etc. tags: https://web.git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=de9e33df7762abbfc2a1568291f2c3a3154c6a9d Should we forward to stable? BR, Jarkko
On Thu, Apr 03, 2025 at 09:45:21PM +0300, Jarkko Sakkinen wrote: >On Wed, Apr 02, 2025 at 06:45:40PM +0100, Jonathan McDowell wrote: >> On Wed, Apr 02, 2025 at 07:21:30PM +0200, Michal Suchanek wrote: >> > With some Infineon chips the timeouts in tpm_tis_send_data (both B and >> > C) can reach up to about 2250 ms. >> > >> > Extend the timeout duration to accommodate this. >> >> The problem here is the bump of timeout_c is going to interact poorly with >> the Infineon errata workaround, as now we'll wait 4s instead of 200ms to >> detect the stuck status change. >> >> (Also shouldn't timeout_c already end up as 750ms, as it's >> max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_C), and TIS_SHORT_TIMEOUT is 750 vs 200 >> for TPM2_TIMEOUT_C? That doesn't seem to be borne out by your logs, nor my >> results.) > >Just noticed that the commit did not end up having fixes etc. tags: > >https://web.git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=de9e33df7762abbfc2a1568291f2c3a3154c6a9d > >Should we forward to stable? It's a TPM bug rather than a kernel issue, so I don't think there's a valid Fixes: for it, but it's certainly stable material in my mind. J.
On Thu, Apr 03, 2025 at 09:43:19PM +0100, Jonathan McDowell wrote: > On Thu, Apr 03, 2025 at 09:45:21PM +0300, Jarkko Sakkinen wrote: > > On Wed, Apr 02, 2025 at 06:45:40PM +0100, Jonathan McDowell wrote: > > > On Wed, Apr 02, 2025 at 07:21:30PM +0200, Michal Suchanek wrote: > > > > With some Infineon chips the timeouts in tpm_tis_send_data (both B and > > > > C) can reach up to about 2250 ms. > > > > > > > > Extend the timeout duration to accommodate this. > > > > > > The problem here is the bump of timeout_c is going to interact poorly with > > > the Infineon errata workaround, as now we'll wait 4s instead of 200ms to > > > detect the stuck status change. > > > > > > (Also shouldn't timeout_c already end up as 750ms, as it's > > > max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_C), and TIS_SHORT_TIMEOUT is 750 vs 200 > > > for TPM2_TIMEOUT_C? That doesn't seem to be borne out by your logs, nor my > > > results.) > > > > Just noticed that the commit did not end up having fixes etc. tags: > > > > https://web.git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=de9e33df7762abbfc2a1568291f2c3a3154c6a9d > > > > Should we forward to stable? > > It's a TPM bug rather than a kernel issue, so I don't think there's a valid > Fixes: for it, but it's certainly stable material in my mind. In the more general sense of Fixes: indicating where the fix is applicable it would be any kernel that supports TPM2. Thanks Michal
On Fri, Apr 04, 2025 at 09:51:29AM +0200, Michal Suchánek wrote: > On Thu, Apr 03, 2025 at 09:43:19PM +0100, Jonathan McDowell wrote: > > On Thu, Apr 03, 2025 at 09:45:21PM +0300, Jarkko Sakkinen wrote: > > > On Wed, Apr 02, 2025 at 06:45:40PM +0100, Jonathan McDowell wrote: > > > > On Wed, Apr 02, 2025 at 07:21:30PM +0200, Michal Suchanek wrote: > > > > > With some Infineon chips the timeouts in tpm_tis_send_data (both B and > > > > > C) can reach up to about 2250 ms. > > > > > > > > > > Extend the timeout duration to accommodate this. > > > > > > > > The problem here is the bump of timeout_c is going to interact poorly with > > > > the Infineon errata workaround, as now we'll wait 4s instead of 200ms to > > > > detect the stuck status change. > > > > > > > > (Also shouldn't timeout_c already end up as 750ms, as it's > > > > max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_C), and TIS_SHORT_TIMEOUT is 750 vs 200 > > > > for TPM2_TIMEOUT_C? That doesn't seem to be borne out by your logs, nor my > > > > results.) > > > > > > Just noticed that the commit did not end up having fixes etc. tags: > > > > > > https://web.git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=de9e33df7762abbfc2a1568291f2c3a3154c6a9d > > > > > > Should we forward to stable? > > > > It's a TPM bug rather than a kernel issue, so I don't think there's a valid > > Fixes: for it, but it's certainly stable material in my mind. > > In the more general sense of Fixes: indicating where the fix is > applicable it would be any kernel that supports TPM2. I tried applying the patch on 6.1-stable: ~/work/kernel.org/stable/linux tags/v6.1.132 $ git am -3 ~/Downloads/infineon.patch Applying: tpm, tpm_tis: Workaround failed command reception on Infineon devices Using index info to reconstruct a base tree... M drivers/char/tpm/tpm_tis_core.c M drivers/char/tpm/tpm_tis_core.h M include/linux/tpm.h Falling back to patching base and 3-way merge... Auto-merging include/linux/tpm.h Auto-merging drivers/char/tpm/tpm_tis_core.h Auto-merging drivers/char/tpm/tpm_tis_core.c If no counter-opinions, I'd add: stable@vger.kernel.org # v6.1+ I based this on Bookworm kernel. > > Thanks > > Michal BR, Jarkko
On Fri, Apr 04, 2025 at 11:10:12AM +0300, Jarkko Sakkinen wrote: >On Fri, Apr 04, 2025 at 09:51:29AM +0200, Michal Suchánek wrote: >> On Thu, Apr 03, 2025 at 09:43:19PM +0100, Jonathan McDowell wrote: >> > On Thu, Apr 03, 2025 at 09:45:21PM +0300, Jarkko Sakkinen wrote: >> > > On Wed, Apr 02, 2025 at 06:45:40PM +0100, Jonathan McDowell wrote: >> > > > On Wed, Apr 02, 2025 at 07:21:30PM +0200, Michal Suchanek wrote: >> > > > > With some Infineon chips the timeouts in tpm_tis_send_data (both B and >> > > > > C) can reach up to about 2250 ms. >> > > > > >> > > > > Extend the timeout duration to accommodate this. >> > > > >> > > > The problem here is the bump of timeout_c is going to interact poorly with >> > > > the Infineon errata workaround, as now we'll wait 4s instead of 200ms to >> > > > detect the stuck status change. >> > > > >> > > > (Also shouldn't timeout_c already end up as 750ms, as it's >> > > > max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_C), and TIS_SHORT_TIMEOUT is 750 vs 200 >> > > > for TPM2_TIMEOUT_C? That doesn't seem to be borne out by your logs, nor my >> > > > results.) >> > > >> > > Just noticed that the commit did not end up having fixes etc. tags: >> > > >> > > https://web.git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=de9e33df7762abbfc2a1568291f2c3a3154c6a9d >> > > >> > > Should we forward to stable? >> > >> > It's a TPM bug rather than a kernel issue, so I don't think there's a valid >> > Fixes: for it, but it's certainly stable material in my mind. >> >> In the more general sense of Fixes: indicating where the fix is >> applicable it would be any kernel that supports TPM2. > >I tried applying the patch on 6.1-stable: > >~/work/kernel.org/stable/linux tags/v6.1.132 >$ git am -3 ~/Downloads/infineon.patch >Applying: tpm, tpm_tis: Workaround failed command reception on Infineon devices >Using index info to reconstruct a base tree... >M drivers/char/tpm/tpm_tis_core.c >M drivers/char/tpm/tpm_tis_core.h >M include/linux/tpm.h >Falling back to patching base and 3-way merge... >Auto-merging include/linux/tpm.h >Auto-merging drivers/char/tpm/tpm_tis_core.h >Auto-merging drivers/char/tpm/tpm_tis_core.c > >If no counter-opinions, I'd add: > >stable@vger.kernel.org # v6.1+ > >I based this on Bookworm kernel. It looks like Sasha has already autoselected it for 6.1, 6.6, 6.12, 6.13 + 6.14. J.
On Fri, Apr 04, 2025 at 10:31:18AM +0100, Jonathan McDowell wrote: > On Fri, Apr 04, 2025 at 11:10:12AM +0300, Jarkko Sakkinen wrote: > > On Fri, Apr 04, 2025 at 09:51:29AM +0200, Michal Suchánek wrote: > > > On Thu, Apr 03, 2025 at 09:43:19PM +0100, Jonathan McDowell wrote: > > > > On Thu, Apr 03, 2025 at 09:45:21PM +0300, Jarkko Sakkinen wrote: > > > > > On Wed, Apr 02, 2025 at 06:45:40PM +0100, Jonathan McDowell wrote: > > > > > > On Wed, Apr 02, 2025 at 07:21:30PM +0200, Michal Suchanek wrote: > > > > > > > With some Infineon chips the timeouts in tpm_tis_send_data (both B and > > > > > > > C) can reach up to about 2250 ms. > > > > > > > > > > > > > > Extend the timeout duration to accommodate this. > > > > > > > > > > > > The problem here is the bump of timeout_c is going to interact poorly with > > > > > > the Infineon errata workaround, as now we'll wait 4s instead of 200ms to > > > > > > detect the stuck status change. > > > > > > > > > > > > (Also shouldn't timeout_c already end up as 750ms, as it's > > > > > > max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_C), and TIS_SHORT_TIMEOUT is 750 vs 200 > > > > > > for TPM2_TIMEOUT_C? That doesn't seem to be borne out by your logs, nor my > > > > > > results.) > > > > > > > > > > Just noticed that the commit did not end up having fixes etc. tags: > > > > > > > > > > https://web.git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=de9e33df7762abbfc2a1568291f2c3a3154c6a9d > > > > > > > > > > Should we forward to stable? > > > > > > > > It's a TPM bug rather than a kernel issue, so I don't think there's a valid > > > > Fixes: for it, but it's certainly stable material in my mind. > > > > > > In the more general sense of Fixes: indicating where the fix is > > > applicable it would be any kernel that supports TPM2. > > > > I tried applying the patch on 6.1-stable: > > > > ~/work/kernel.org/stable/linux tags/v6.1.132 > > $ git am -3 ~/Downloads/infineon.patch > > Applying: tpm, tpm_tis: Workaround failed command reception on Infineon devices > > Using index info to reconstruct a base tree... > > M drivers/char/tpm/tpm_tis_core.c > > M drivers/char/tpm/tpm_tis_core.h > > M include/linux/tpm.h > > Falling back to patching base and 3-way merge... > > Auto-merging include/linux/tpm.h > > Auto-merging drivers/char/tpm/tpm_tis_core.h > > Auto-merging drivers/char/tpm/tpm_tis_core.c > > > > If no counter-opinions, I'd add: > > > > stable@vger.kernel.org # v6.1+ > > > > I based this on Bookworm kernel. > > It looks like Sasha has already autoselected it for 6.1, 6.6, 6.12, 6.13 + > 6.14. Right! I can see also those mails, and exactly the version range I would have proposed :-) Perfect, thanks Sasha! > > J. > > -- > How does it work? I don't know but it does! > This .sig brought to you by the letter R and the number 21 > Product of the Republic of HuggieTag > BR, Jarkko
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h index 970d02c337c7..1ff565be2175 100644 --- a/drivers/char/tpm/tpm_tis_core.h +++ b/drivers/char/tpm/tpm_tis_core.h @@ -54,7 +54,7 @@ enum tis_int_flags { enum tis_defaults { TIS_MEM_LEN = 0x5000, TIS_SHORT_TIMEOUT = 750, /* ms */ - TIS_LONG_TIMEOUT = 2000, /* 2 sec */ + TIS_LONG_TIMEOUT = 4000, /* 2 sec */ TIS_TIMEOUT_MIN_ATML = 14700, /* usecs */ TIS_TIMEOUT_MAX_ATML = 15000, /* usecs */ }; @@ -64,7 +64,7 @@ enum tis_defaults { */ #define TIS_TIMEOUT_A_MAX max_t(int, TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_A) #define TIS_TIMEOUT_B_MAX max_t(int, TIS_LONG_TIMEOUT, TPM2_TIMEOUT_B) -#define TIS_TIMEOUT_C_MAX max_t(int, TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_C) +#define TIS_TIMEOUT_C_MAX max_t(int, TIS_LONG_TIMEOUT, TPM2_TIMEOUT_C) #define TIS_TIMEOUT_D_MAX max_t(int, TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_D) #define TPM_ACCESS(l) (0x0000 | ((l) << 12))
With some Infineon chips the timeouts in tpm_tis_send_data (both B and C) can reach up to about 2250 ms. Extend the timeout duration to accommodate this. Link: https://lore.kernel.org/linux-integrity/Z5pI07m0Muapyu9w@kitsune.suse.cz/ Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- An alternative would be to add an entry to vendor_timeout_overrides but I do not know how to determine the chip IDs to put into this table. --- drivers/char/tpm/tpm_tis_core.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)