[PATCH - for 6.4] tpm: tpm_tis: Disable interrupts for AEON UPX-i11

[PATCH - for 6.4] tpm: tpm_tis: Disable interrupts for AEON UPX-i11

[Peter Ujfalusi]

The interrupts initially works on the device but they will stop arriving
after about 200 interrupts.

On system reboot/shutdown this will cause a long wait (120000 jiffies).

The interrupts on this device got enabled by commit
e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")

Prior to this point the interrupts were not enabled on this machine.

Complements: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
Hi,

This patch applies on top of mainline since 6.4-rc1 takes about 2 minutes to
reboot on this machine, linux-next have
e7d3e5c4b1dd tpm/tpm_tis: Disable interrupts for more Lenovo devices

I'm not sure if I shouold send this on top of next or mainline is fine, please
let me know the preferred way to get this to 6.4.

Regards,
Peter

 drivers/char/tpm/tpm_tis.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 7af389806643..aad682c2ab21 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -122,6 +122,13 @@ static const struct dmi_system_id tpm_tis_dmi_table[] = {
 			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T490s"),
 		},
 	},
+	{
+		.callback = tpm_tis_disable_irq,
+		.ident = "UPX-TGL",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "AAEON"),
+		},
+	},
 	{}
 };
 
-- 
2.40.1

Re: [PATCH - for 6.4] tpm: tpm_tis: Disable interrupts for AEON UPX-i11

[Péter Ujfalusi]

On 17/05/2023 15:29, Peter Ujfalusi wrote:
> The interrupts initially works on the device but they will stop arriving
> after about 200 interrupts.
> 
> On system reboot/shutdown this will cause a long wait (120000 jiffies).
> 
> The interrupts on this device got enabled by commit
> e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")
> 
> Prior to this point the interrupts were not enabled on this machine.
> 
> Complements: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> ---
> Hi,
> 
> This patch applies on top of mainline since 6.4-rc1 takes about 2 minutes to
> reboot on this machine, linux-next have
> e7d3e5c4b1dd tpm/tpm_tis: Disable interrupts for more Lenovo devices
> 
> I'm not sure if I shouold send this on top of next or mainline is fine, please
> let me know the preferred way to get this to 6.4.

In 6.3 the kernel prints this on boot:
# dmesg -w | grep tpm
tpm_tis MSFT0101:00: 2.0 TPM (device-id 0x1B, rev-id 22)
tpm tpm0: [Firmware Bug]: TPM interrupt not working, polling instead

It is interesting that with 6.4 most of the times the interrupts got
enabled (without this patch) resulting stall during reboot/shutdown but
there are few boots when the driver falls back to polling and thus the
TPM driver works.

The command which 'locks' the system is TPM2_CC_SHUTDOWN, it is given
TPM_UNDEFINED as duration index by tpm2_ordinal_duration_index().

> 
> Regards,
> Peter
> 
>  drivers/char/tpm/tpm_tis.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 7af389806643..aad682c2ab21 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -122,6 +122,13 @@ static const struct dmi_system_id tpm_tis_dmi_table[] = {
>  			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T490s"),
>  		},
>  	},
> +	{
> +		.callback = tpm_tis_disable_irq,
> +		.ident = "UPX-TGL",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +		},
> +	},
>  	{}
>  };
>  

-- 
Péter

Re: [PATCH - for 6.4] tpm: tpm_tis: Disable interrupts for AEON UPX-i11

[Jarkko Sakkinen]

On Wed May 17, 2023 at 3:29 PM EEST, Peter Ujfalusi wrote:
> The interrupts initially works on the device but they will stop arriving
> after about 200 interrupts.
>
> On system reboot/shutdown this will cause a long wait (120000 jiffies).
>
> The interrupts on this device got enabled by commit
> e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")
>
> Prior to this point the interrupts were not enabled on this machine.
>
> Complements: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")

Complements -> Fixes

> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> ---
> Hi,
>
> This patch applies on top of mainline since 6.4-rc1 takes about 2 minutes to
> reboot on this machine, linux-next have
> e7d3e5c4b1dd tpm/tpm_tis: Disable interrupts for more Lenovo devices
>
> I'm not sure if I shouold send this on top of next or mainline is fine, please
> let me know the preferred way to get this to 6.4.
>
> Regards,
> Peter
>
>  drivers/char/tpm/tpm_tis.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 7af389806643..aad682c2ab21 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -122,6 +122,13 @@ static const struct dmi_system_id tpm_tis_dmi_table[] = {
>  			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T490s"),
>  		},
>  	},
> +	{
> +		.callback = tpm_tis_disable_irq,
> +		.ident = "UPX-TGL",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +		},
> +	},
>  	{}
>  };
>  
> -- 
> 2.40.1

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

Re: [PATCH - for 6.4] tpm: tpm_tis: Disable interrupts for AEON UPX-i11

[Jarkko Sakkinen]

On Thu May 18, 2023 at 9:37 PM EEST, Jarkko Sakkinen wrote:
> On Wed May 17, 2023 at 3:29 PM EEST, Peter Ujfalusi wrote:
> > The interrupts initially works on the device but they will stop arriving
> > after about 200 interrupts.
> >
> > On system reboot/shutdown this will cause a long wait (120000 jiffies).
> >
> > The interrupts on this device got enabled by commit
> > e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")
> >
> > Prior to this point the interrupts were not enabled on this machine.
> >
> > Complements: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")
>
> Complements -> Fixes
>
> > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> > ---
> > Hi,
> >
> > This patch applies on top of mainline since 6.4-rc1 takes about 2 minutes to
> > reboot on this machine, linux-next have
> > e7d3e5c4b1dd tpm/tpm_tis: Disable interrupts for more Lenovo devices
> >
> > I'm not sure if I shouold send this on top of next or mainline is fine, please
> > let me know the preferred way to get this to 6.4.
> >
> > Regards,
> > Peter
> >
> >  drivers/char/tpm/tpm_tis.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> > index 7af389806643..aad682c2ab21 100644
> > --- a/drivers/char/tpm/tpm_tis.c
> > +++ b/drivers/char/tpm/tpm_tis.c
> > @@ -122,6 +122,13 @@ static const struct dmi_system_id tpm_tis_dmi_table[] = {
> >  			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T490s"),
> >  		},
> >  	},
> > +	{
> > +		.callback = tpm_tis_disable_irq,
> > +		.ident = "UPX-TGL",
> > +		.matches = {
> > +			DMI_MATCH(DMI_SYS_VENDOR, "AAEON"),
> > +		},
> > +	},
> >  	{}
> >  };
> >  
> > -- 
> > 2.40.1
>
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

I adjusted the commit message a bit:

https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=d46f47575cf97d397fbe8a6150353f41d2917936

BR, Jarkko

Re: [PATCH - for 6.4] tpm: tpm_tis: Disable interrupts for AEON UPX-i11

[Jerry Snitselaar]

On Wed, May 17, 2023 at 03:29:31PM +0300, Peter Ujfalusi wrote:
> The interrupts initially works on the device but they will stop arriving
> after about 200 interrupts.
> 
> On system reboot/shutdown this will cause a long wait (120000 jiffies).
> 
> The interrupts on this device got enabled by commit
> e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")
> 
> Prior to this point the interrupts were not enabled on this machine.
> 
> Complements: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> ---
> Hi,
> 
> This patch applies on top of mainline since 6.4-rc1 takes about 2 minutes to
> reboot on this machine, linux-next have
> e7d3e5c4b1dd tpm/tpm_tis: Disable interrupts for more Lenovo devices
> 
> I'm not sure if I shouold send this on top of next or mainline is fine, please
> let me know the preferred way to get this to 6.4.
> 
> Regards,
> Peter
> 
>  drivers/char/tpm/tpm_tis.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 7af389806643..aad682c2ab21 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -122,6 +122,13 @@ static const struct dmi_system_id tpm_tis_dmi_table[] = {
>  			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T490s"),
>  		},
>  	},
> +	{
> +		.callback = tpm_tis_disable_irq,
> +		.ident = "UPX-TGL",

Is this the product version returned by dmidecode? If yes,
then the entry could be made more specific by adding a
DMI_MATCH(DMI_PRODUCT_VERSION, "UPX-TGL"), and only disable
for this device instead of any system that matches the vendor
AAEON.

> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "AAEON"),
> +		},
> +	},
>  	{}
>  };
>  
> -- 
> 2.40.1
> 

Re: [PATCH - for 6.4] tpm: tpm_tis: Disable interrupts for AEON UPX-i11

[Jarkko Sakkinen]

On Thu May 18, 2023 at 9:50 PM EEST, Jerry Snitselaar wrote:
> On Wed, May 17, 2023 at 03:29:31PM +0300, Peter Ujfalusi wrote:
> > The interrupts initially works on the device but they will stop arriving
> > after about 200 interrupts.
> > 
> > On system reboot/shutdown this will cause a long wait (120000 jiffies).
> > 
> > The interrupts on this device got enabled by commit
> > e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")
> > 
> > Prior to this point the interrupts were not enabled on this machine.
> > 
> > Complements: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")
> > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> > ---
> > Hi,
> > 
> > This patch applies on top of mainline since 6.4-rc1 takes about 2 minutes to
> > reboot on this machine, linux-next have
> > e7d3e5c4b1dd tpm/tpm_tis: Disable interrupts for more Lenovo devices
> > 
> > I'm not sure if I shouold send this on top of next or mainline is fine, please
> > let me know the preferred way to get this to 6.4.
> > 
> > Regards,
> > Peter
> > 
> >  drivers/char/tpm/tpm_tis.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> > index 7af389806643..aad682c2ab21 100644
> > --- a/drivers/char/tpm/tpm_tis.c
> > +++ b/drivers/char/tpm/tpm_tis.c
> > @@ -122,6 +122,13 @@ static const struct dmi_system_id tpm_tis_dmi_table[] = {
> >  			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T490s"),
> >  		},
> >  	},
> > +	{
> > +		.callback = tpm_tis_disable_irq,
> > +		.ident = "UPX-TGL",
>
> Is this the product version returned by dmidecode? If yes,
> then the entry could be made more specific by adding a
> DMI_MATCH(DMI_PRODUCT_VERSION, "UPX-TGL"), and only disable
> for this device instead of any system that matches the vendor
> AAEON.

I can squash this to the commit I pushed (it is not yet mirrored
to linux-next), if I get the dmidecode info.

BR, Jarkko

Re: [PATCH - for 6.4] tpm: tpm_tis: Disable interrupts for AEON UPX-i11

[Péter Ujfalusi]

On 18/05/2023 21:53, Jarkko Sakkinen wrote:
> On Thu May 18, 2023 at 9:50 PM EEST, Jerry Snitselaar wrote:
>> On Wed, May 17, 2023 at 03:29:31PM +0300, Peter Ujfalusi wrote:
>>> The interrupts initially works on the device but they will stop arriving
>>> after about 200 interrupts.
>>>
>>> On system reboot/shutdown this will cause a long wait (120000 jiffies).
>>>
>>> The interrupts on this device got enabled by commit
>>> e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")
>>>
>>> Prior to this point the interrupts were not enabled on this machine.
>>>
>>> Complements: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")
>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
>>> ---
>>> Hi,
>>>
>>> This patch applies on top of mainline since 6.4-rc1 takes about 2 minutes to
>>> reboot on this machine, linux-next have
>>> e7d3e5c4b1dd tpm/tpm_tis: Disable interrupts for more Lenovo devices
>>>
>>> I'm not sure if I shouold send this on top of next or mainline is fine, please
>>> let me know the preferred way to get this to 6.4.
>>>
>>> Regards,
>>> Peter
>>>
>>>  drivers/char/tpm/tpm_tis.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
>>> index 7af389806643..aad682c2ab21 100644
>>> --- a/drivers/char/tpm/tpm_tis.c
>>> +++ b/drivers/char/tpm/tpm_tis.c
>>> @@ -122,6 +122,13 @@ static const struct dmi_system_id tpm_tis_dmi_table[] = {
>>>  			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T490s"),
>>>  		},
>>>  	},
>>> +	{
>>> +		.callback = tpm_tis_disable_irq,
>>> +		.ident = "UPX-TGL",
>>
>> Is this the product version returned by dmidecode? If yes,
>> then the entry could be made more specific by adding a
>> DMI_MATCH(DMI_PRODUCT_VERSION, "UPX-TGL"), and only disable
>> for this device instead of any system that matches the vendor
>> AAEON.

The version is used to differentiate the revisions of the UPX-i11
boards, and this issue present in all revisions.

> I can squash this to the commit I pushed (it is not yet mirrored
> to linux-next), if I get the dmidecode info.

System Information
        Manufacturer: AAEON
        Product Name: UPX-TGL01
        Version: V1.0
        Serial Number: Default string
        UUID: a300091d-fb1c-ce1c-1d30-0007328efc11
        Wake-up Type: Power Switch
        SKU Number: Default string
        Family: Default string

I have used this description as it it is used for SOF, probably
DMI_MATCH(DMI_PRODUCT_NAME, "UPX-TGL01")
should be added?

Oh, yes, the product name match should be there, we have TigerLake
specific matching, so SOF is looking for AAEON device with TGL.

Sorry for missing this.

> 
> BR, Jarkko

-- 
Péter

Re: [PATCH - for 6.4] tpm: tpm_tis: Disable interrupts for AEON UPX-i11

[Péter Ujfalusi]

On 18/05/2023 23:24, Péter Ujfalusi wrote:
> 
> The version is used to differentiate the revisions of the UPX-i11
> boards, and this issue present in all revisions.
> 
>> I can squash this to the commit I pushed (it is not yet mirrored
>> to linux-next), if I get the dmidecode info.
> 
> System Information
>         Manufacturer: AAEON
>         Product Name: UPX-TGL01
>         Version: V1.0
>         Serial Number: Default string
>         UUID: a300091d-fb1c-ce1c-1d30-0007328efc11
>         Wake-up Type: Power Switch
>         SKU Number: Default string
>         Family: Default string
> 
> I have used this description as it it is used for SOF, probably
> DMI_MATCH(DMI_PRODUCT_NAME, "UPX-TGL01")
> should be added?

Jarkko: I have tested that adding the
DMI_MATCH(DMI_PRODUCT_NAME, "UPX-TGL01")
works.

I would also do a small update to commit message: "120000 jiffies"  to
"120000 msec".
On my setup 120000 msec ends up to be 120000 jiffies.

How do you prefer to handle this?
I can send a v2 on top of linux-next / mainline
I can send a fixup patch which can be squashed to the patch you have in
your master branch atm
Or you add this line by yourself?

Either way is fine for me, whichever works best for you.

-- 
Péter

Re: [PATCH - for 6.4] tpm: tpm_tis: Disable interrupts for AEON UPX-i11

[Jarkko Sakkinen]

On Mon May 22, 2023 at 10:40 AM EEST, Péter Ujfalusi wrote:
>
> On 18/05/2023 23:24, Péter Ujfalusi wrote:
> > 
> > The version is used to differentiate the revisions of the UPX-i11
> > boards, and this issue present in all revisions.
> > 
> >> I can squash this to the commit I pushed (it is not yet mirrored
> >> to linux-next), if I get the dmidecode info.
> > 
> > System Information
> >         Manufacturer: AAEON
> >         Product Name: UPX-TGL01
> >         Version: V1.0
> >         Serial Number: Default string
> >         UUID: a300091d-fb1c-ce1c-1d30-0007328efc11
> >         Wake-up Type: Power Switch
> >         SKU Number: Default string
> >         Family: Default string
> > 
> > I have used this description as it it is used for SOF, probably
> > DMI_MATCH(DMI_PRODUCT_NAME, "UPX-TGL01")
> > should be added?
>
> Jarkko: I have tested that adding the
> DMI_MATCH(DMI_PRODUCT_NAME, "UPX-TGL01")
> works.
>
> I would also do a small update to commit message: "120000 jiffies"  to
> "120000 msec".
> On my setup 120000 msec ends up to be 120000 jiffies.
>
> How do you prefer to handle this?
> I can send a v2 on top of linux-next / mainline
> I can send a fixup patch which can be squashed to the patch you have in
> your master branch atm
> Or you add this line by yourself?
>
> Either way is fine for me, whichever works best for you.

If you want to send a follow-up please do, and I can pick it.

BR, Jarkko

Re: [PATCH - for 6.4] tpm: tpm_tis: Disable interrupts for AEON UPX-i11

[Jarkko Sakkinen]

On Thu, 2023-05-18 at 23:24 +0300, Péter Ujfalusi wrote:
> 
> On 18/05/2023 21:53, Jarkko Sakkinen wrote:
> > On Thu May 18, 2023 at 9:50 PM EEST, Jerry Snitselaar wrote:
> > > On Wed, May 17, 2023 at 03:29:31PM +0300, Peter Ujfalusi wrote:
> > > > The interrupts initially works on the device but they will stop arriving
> > > > after about 200 interrupts.
> > > > 
> > > > On system reboot/shutdown this will cause a long wait (120000 jiffies).
> > > > 
> > > > The interrupts on this device got enabled by commit
> > > > e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")
> > > > 
> > > > Prior to this point the interrupts were not enabled on this machine.
> > > > 
> > > > Complements: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")
> > > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> > > > ---
> > > > Hi,
> > > > 
> > > > This patch applies on top of mainline since 6.4-rc1 takes about 2 minutes to
> > > > reboot on this machine, linux-next have
> > > > e7d3e5c4b1dd tpm/tpm_tis: Disable interrupts for more Lenovo devices
> > > > 
> > > > I'm not sure if I shouold send this on top of next or mainline is fine, please
> > > > let me know the preferred way to get this to 6.4.
> > > > 
> > > > Regards,
> > > > Peter
> > > > 
> > > >  drivers/char/tpm/tpm_tis.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> > > > index 7af389806643..aad682c2ab21 100644
> > > > --- a/drivers/char/tpm/tpm_tis.c
> > > > +++ b/drivers/char/tpm/tpm_tis.c
> > > > @@ -122,6 +122,13 @@ static const struct dmi_system_id tpm_tis_dmi_table[] = {
> > > >  			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T490s"),
> > > >  		},
> > > >  	},
> > > > +	{
> > > > +		.callback = tpm_tis_disable_irq,
> > > > +		.ident = "UPX-TGL",
> > > 
> > > Is this the product version returned by dmidecode? If yes,
> > > then the entry could be made more specific by adding a
> > > DMI_MATCH(DMI_PRODUCT_VERSION, "UPX-TGL"), and only disable
> > > for this device instead of any system that matches the vendor
> > > AAEON.
> 
> The version is used to differentiate the revisions of the UPX-i11
> boards, and this issue present in all revisions.
> 
> > I can squash this to the commit I pushed (it is not yet mirrored
> > to linux-next), if I get the dmidecode info.
> 
> System Information
>         Manufacturer: AAEON
>         Product Name: UPX-TGL01
>         Version: V1.0
>         Serial Number: Default string
>         UUID: a300091d-fb1c-ce1c-1d30-0007328efc11
>         Wake-up Type: Power Switch
>         SKU Number: Default string
>         Family: Default string
> 
> I have used this description as it it is used for SOF, probably
> DMI_MATCH(DMI_PRODUCT_NAME, "UPX-TGL01")
> should be added?
> 
> Oh, yes, the product name match should be there, we have TigerLake
> specific matching, so SOF is looking for AAEON device with TGL.
> 
> Sorry for missing this.

I sent a PR with the original patch because I know that you've tested
it and it has worked for you.

BR, Jarkko