[PATCH] tpm_tis: don't flush never initialized work

[PATCH] tpm_tis: don't flush never initialized work

[Jan Beulich]

tpm_tis_core_init() may fail before tpm_tis_probe_irq_single() is
called, in which case tpm_tis_remove() unconditionally calling
flush_work() is triggering a warning for .func still being NULL.

Fixes: 481c2d14627d ("tpm,tpm_tis: Disable interrupts after 1000 unhandled IRQs")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
An alternative would be to move INIT_WORK(), but where to put it is far
more difficult to tell for an outsider than simply making the flush call
conditional.

--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1022,7 +1022,8 @@ void tpm_tis_remove(struct tpm_chip *chi
 		interrupt = 0;
 
 	tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt);
-	flush_work(&priv->free_irq_work);
+	if (priv->free_irq_work.func)
+		flush_work(&priv->free_irq_work);
 
 	tpm_tis_clkrun_enable(chip, false);
 

Re: [PATCH] tpm_tis: don't flush never initialized work

[Jarkko Sakkinen]

On Thu Sep 14, 2023 at 5:28 PM EEST, Jan Beulich wrote:
> tpm_tis_core_init() may fail before tpm_tis_probe_irq_single() is
> called, in which case tpm_tis_remove() unconditionally calling
> flush_work() is triggering a warning for .func still being NULL.
>
> Fixes: 481c2d14627d ("tpm,tpm_tis: Disable interrupts after 1000 unhandled IRQs")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> An alternative would be to move INIT_WORK(), but where to put it is far
> more difficult to tell for an outsider than simply making the flush call
> conditional.
>
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -1022,7 +1022,8 @@ void tpm_tis_remove(struct tpm_chip *chi
>  		interrupt = 0;
>  
>  	tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt);
> -	flush_work(&priv->free_irq_work);
> +	if (priv->free_irq_work.func)
> +		flush_work(&priv->free_irq_work);
>  
>  	tpm_tis_clkrun_enable(chip, false);
>  

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

Jan, I'm about to leave to vacation but will be back after next week.

Do you think that the fix can hold up unti that?

The feature is opt-in as I documented to kernel-parameters.txt:

	tpm_tis.interrupts= [HW,TPM]
			Enable interrupts for the MMIO based physical layer
			for the FIFO interface. By default it is set to false
			(0). For more information about TPM hardware interfaces
			defined by Trusted Computing Group (TCG) see
			https://trustedcomputinggroup.org/resource/pc-client-platform-tpm-profile-ptp-specification/

I'd prefer to apply this right after I'm back and not on rush but
will listen to other reasoning :-) Thanks.

BR, Jarkko

Re: [PATCH] tpm_tis: don't flush never initialized work

[Jan Beulich]

On 14.09.2023 17:19, Jarkko Sakkinen wrote:
> On Thu Sep 14, 2023 at 5:28 PM EEST, Jan Beulich wrote:
>> tpm_tis_core_init() may fail before tpm_tis_probe_irq_single() is
>> called, in which case tpm_tis_remove() unconditionally calling
>> flush_work() is triggering a warning for .func still being NULL.
>>
>> Fixes: 481c2d14627d ("tpm,tpm_tis: Disable interrupts after 1000 unhandled IRQs")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> An alternative would be to move INIT_WORK(), but where to put it is far
>> more difficult to tell for an outsider than simply making the flush call
>> conditional.
>>
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -1022,7 +1022,8 @@ void tpm_tis_remove(struct tpm_chip *chi
>>  		interrupt = 0;
>>  
>>  	tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt);
>> -	flush_work(&priv->free_irq_work);
>> +	if (priv->free_irq_work.func)
>> +		flush_work(&priv->free_irq_work);
>>  
>>  	tpm_tis_clkrun_enable(chip, false);
>>  
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> Jan, I'm about to leave to vacation but will be back after next week.
> 
> Do you think that the fix can hold up unti that?

There's no rush from my pov, as I have helped myself. Nevertheless ...

> The feature is opt-in as I documented to kernel-parameters.txt:
> 
> 	tpm_tis.interrupts= [HW,TPM]
> 			Enable interrupts for the MMIO based physical layer
> 			for the FIFO interface. By default it is set to false
> 			(0). For more information about TPM hardware interfaces
> 			defined by Trusted Computing Group (TCG) see
> 			https://trustedcomputinggroup.org/resource/pc-client-platform-tpm-profile-ptp-specification/

... I'm not doing anything non-default. The issue here is that part of
interrupt cleanup occurs without interrupt setup having happened. So
I'm inclined to think that the warning is independent of (and perhaps
more likely to observe without) use of that optional functionality.

Jan

Re: [PATCH] tpm_tis: don't flush never initialized work

[Jarkko Sakkinen]

On Thu Sep 14, 2023 at 6:29 PM EEST, Jan Beulich wrote:
> On 14.09.2023 17:19, Jarkko Sakkinen wrote:
> > On Thu Sep 14, 2023 at 5:28 PM EEST, Jan Beulich wrote:
> >> tpm_tis_core_init() may fail before tpm_tis_probe_irq_single() is
> >> called, in which case tpm_tis_remove() unconditionally calling
> >> flush_work() is triggering a warning for .func still being NULL.
> >>
> >> Fixes: 481c2d14627d ("tpm,tpm_tis: Disable interrupts after 1000 unhandled IRQs")
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> An alternative would be to move INIT_WORK(), but where to put it is far
> >> more difficult to tell for an outsider than simply making the flush call
> >> conditional.
> >>
> >> --- a/drivers/char/tpm/tpm_tis_core.c
> >> +++ b/drivers/char/tpm/tpm_tis_core.c
> >> @@ -1022,7 +1022,8 @@ void tpm_tis_remove(struct tpm_chip *chi
> >>  		interrupt = 0;
> >>  
> >>  	tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt);
> >> -	flush_work(&priv->free_irq_work);
> >> +	if (priv->free_irq_work.func)
> >> +		flush_work(&priv->free_irq_work);
> >>  
> >>  	tpm_tis_clkrun_enable(chip, false);
> >>  
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > Jan, I'm about to leave to vacation but will be back after next week.
> > 
> > Do you think that the fix can hold up unti that?
>
> There's no rush from my pov, as I have helped myself. Nevertheless ...
>
> > The feature is opt-in as I documented to kernel-parameters.txt:
> > 
> > 	tpm_tis.interrupts= [HW,TPM]
> > 			Enable interrupts for the MMIO based physical layer
> > 			for the FIFO interface. By default it is set to false
> > 			(0). For more information about TPM hardware interfaces
> > 			defined by Trusted Computing Group (TCG) see
> > 			https://trustedcomputinggroup.org/resource/pc-client-platform-tpm-profile-ptp-specification/
>
> ... I'm not doing anything non-default. The issue here is that part of
> interrupt cleanup occurs without interrupt setup having happened. So
> I'm inclined to think that the warning is independent of (and perhaps
> more likely to observe without) use of that optional functionality.
>
> Jan

Ah, you're absolutely right!

I'll apply and test this properly after I'm back. Thanks for finding
the bug and fixing  it.

BR, Jrakko

Ping: [PATCH] tpm_tis: don't flush never initialized work

[Jan Beulich]

On 14.09.2023 17:29, Jan Beulich wrote:
> On 14.09.2023 17:19, Jarkko Sakkinen wrote:
>> On Thu Sep 14, 2023 at 5:28 PM EEST, Jan Beulich wrote:
>>> tpm_tis_core_init() may fail before tpm_tis_probe_irq_single() is
>>> called, in which case tpm_tis_remove() unconditionally calling
>>> flush_work() is triggering a warning for .func still being NULL.
>>>
>>> Fixes: 481c2d14627d ("tpm,tpm_tis: Disable interrupts after 1000 unhandled IRQs")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> An alternative would be to move INIT_WORK(), but where to put it is far
>>> more difficult to tell for an outsider than simply making the flush call
>>> conditional.
>>>
>>> --- a/drivers/char/tpm/tpm_tis_core.c
>>> +++ b/drivers/char/tpm/tpm_tis_core.c
>>> @@ -1022,7 +1022,8 @@ void tpm_tis_remove(struct tpm_chip *chi
>>>  		interrupt = 0;
>>>  
>>>  	tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt);
>>> -	flush_work(&priv->free_irq_work);
>>> +	if (priv->free_irq_work.func)
>>> +		flush_work(&priv->free_irq_work);
>>>  
>>>  	tpm_tis_clkrun_enable(chip, false);
>>>  
>>
>> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
>>
>> Jan, I'm about to leave to vacation but will be back after next week.
>>
>> Do you think that the fix can hold up unti that?
> 
> There's no rush from my pov, as I have helped myself.

Has this possibly fallen through the cracks?

Jan

Re: Ping: [PATCH] tpm_tis: don't flush never initialized work

[Jarkko Sakkinen]

On Wed May 29, 2024 at 10:47 AM EEST, Jan Beulich wrote:
> Has this possibly fallen through the cracks?

Not possibly that is what exactly has happened, sorry.

I tweaked a bit the commit message:

commit 2c0943eba0bd765e1e4a90234e669a26a9304b74 (HEAD -> master)
Author: Jan Beulich <jbeulich@suse.com>
Date:   Wed May 29 15:23:25 2024 +0300

    tpm_tis: Do *not* flush uninitialized work

    tpm_tis_core_init() may fail before tpm_tis_probe_irq_single() is
    called, in which case tpm_tis_remove() unconditionally calling
    flush_work() is triggering a warning for .func still being NULL.

    Cc: stable@vger.kernel.org # v6.5+
    Fixes: 481c2d14627d ("tpm,tpm_tis: Disable interrupts after 1000 unhandled IRQs")
    Signed-off-by: Jan Beulich <jbeulich@suse.com>
    Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
    Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

Does this make sense to you?

> Jan

BR, Jarkko

Re: Ping: [PATCH] tpm_tis: don't flush never initialized work

[Jan Beulich]

On 29.05.2024 14:26, Jarkko Sakkinen wrote:
> On Wed May 29, 2024 at 10:47 AM EEST, Jan Beulich wrote:
>> Has this possibly fallen through the cracks?
> 
> Not possibly that is what exactly has happened, sorry.
> 
> I tweaked a bit the commit message:
> 
> commit 2c0943eba0bd765e1e4a90234e669a26a9304b74 (HEAD -> master)
> Author: Jan Beulich <jbeulich@suse.com>
> Date:   Wed May 29 15:23:25 2024 +0300
> 
>     tpm_tis: Do *not* flush uninitialized work
> 
>     tpm_tis_core_init() may fail before tpm_tis_probe_irq_single() is
>     called, in which case tpm_tis_remove() unconditionally calling
>     flush_work() is triggering a warning for .func still being NULL.
> 
>     Cc: stable@vger.kernel.org # v6.5+
>     Fixes: 481c2d14627d ("tpm,tpm_tis: Disable interrupts after 1000 unhandled IRQs")
>     Signed-off-by: Jan Beulich <jbeulich@suse.com>
>     Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
>     Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> Does this make sense to you?

Sure, that's entirely up to you.

Jan

Re: Ping: [PATCH] tpm_tis: don't flush never initialized work

[Jarkko Sakkinen]

On Wed May 29, 2024 at 3:32 PM EEST, Jan Beulich wrote:
> On 29.05.2024 14:26, Jarkko Sakkinen wrote:
> > On Wed May 29, 2024 at 10:47 AM EEST, Jan Beulich wrote:
> >> Has this possibly fallen through the cracks?
> > 
> > Not possibly that is what exactly has happened, sorry.
> > 
> > I tweaked a bit the commit message:
> > 
> > commit 2c0943eba0bd765e1e4a90234e669a26a9304b74 (HEAD -> master)
> > Author: Jan Beulich <jbeulich@suse.com>
> > Date:   Wed May 29 15:23:25 2024 +0300
> > 
> >     tpm_tis: Do *not* flush uninitialized work
> > 
> >     tpm_tis_core_init() may fail before tpm_tis_probe_irq_single() is
> >     called, in which case tpm_tis_remove() unconditionally calling
> >     flush_work() is triggering a warning for .func still being NULL.
> > 
> >     Cc: stable@vger.kernel.org # v6.5+
> >     Fixes: 481c2d14627d ("tpm,tpm_tis: Disable interrupts after 1000 unhandled IRQs")
> >     Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >     Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> >     Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > Does this make sense to you?
>
> Sure, that's entirely up to you.

Ok, cool, it should mirror soon to linux-next. Earliest time to PR it is
next week (rc3) because I sent one already yesterday.

> Jan

BR, Jarkko