Re: [PATCH v4 1/6] tpm, tpm_tis_spi: Request threaded irq

From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
To: Jarkko Sakkinen <jarkko@kernel.org>
Cc: peterhuewe@gmx.de, jgg@ziepe.ca, stefanb@linux.vnet.ibm.com,
	linux@mniewoehner.de, linux-integrity@vger.kernel.org,
	linux-kernel@vger.kernel.org, lukas@wunner.de,
	p.rosenberger@kunbus.com,
	Lino Sanfilippo <l.sanfilippo@kunbus.com>
Subject: Re: [PATCH v4 1/6] tpm, tpm_tis_spi: Request threaded irq
Date: Wed, 11 May 2022 21:18:39 +0200	[thread overview]
Message-ID: <486cec01-ec02-3f11-0b81-037e0700c503@gmx.de> (raw)
In-Reply-To: <YnucgDH3I87RI8PN@kernel.org>

Hi,

On 11.05.22 at 13:22, Jarkko Sakkinen wrote:
> On Mon, May 09, 2022 at 10:05:54AM +0200, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> Interrupt handling at least includes reading and writing the interrupt
>> status register within the interrupt routine. Since accesses over the SPI
>> bus are synchronized by a mutex, request a threaded interrupt handler to
>> ensure a sleepable context during interrupt processing.
>>
>> Fixes: 1a339b658d9d ("tpm_tis_spi: Pass the SPI IRQ down to the driver")
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>
> When you state that it needs a sleepable context, you should bring a
> context why it needs it. This not to disregard the code change overally but
> you cannot make even the most obvious claim without backing data.
>

so what kind of backing data do you have in mind? Would it help to emphasize more
that the irq handler is running in hard irq context in the current code and thus
must not access registers over SPI since SPI uses a mutex (I consider it as basic
knowledge that a mutex must not be taken in hard irq context)?

Regards,
Lino

>> ---
>>  drivers/char/tpm/tpm_tis_core.c     | 15 +++++++++++++--
>>  drivers/char/tpm/tpm_tis_core.h     |  1 +
>>  drivers/char/tpm/tpm_tis_spi_main.c |  5 +++--
>>  3 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index dc56b976d816..52369ef39b03 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -747,8 +747,19 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
>>  	int rc;
>>  	u32 int_status;
>>
>> -	if (devm_request_irq(chip->dev.parent, irq, tis_int_handler, flags,
>> -			     dev_name(&chip->dev), chip) != 0) {
>> +
>> +	if (priv->flags & TPM_TIS_USE_THREADED_IRQ) {
>> +		rc = devm_request_threaded_irq(chip->dev.parent, irq, NULL,
>> +					       tis_int_handler,
>> +					       IRQF_ONESHOT | flags,
>> +					       dev_name(&chip->dev),
>> +					       chip);
>> +	} else {
>> +		rc = devm_request_irq(chip->dev.parent, irq, tis_int_handler,
>> +				      flags, dev_name(&chip->dev), chip);
>> +	}
>> +
>> +	if (rc) {
>>  		dev_info(&chip->dev, "Unable to request irq: %d for probe\n",
>>  			 irq);
>>  		return -1;
>> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
>> index 3be24f221e32..43b724e55192 100644
>> --- a/drivers/char/tpm/tpm_tis_core.h
>> +++ b/drivers/char/tpm/tpm_tis_core.h
>> @@ -86,6 +86,7 @@ enum tis_defaults {
>>  enum tpm_tis_flags {
>>  	TPM_TIS_ITPM_WORKAROUND		= BIT(0),
>>  	TPM_TIS_INVALID_STATUS		= BIT(1),
>> +	TPM_TIS_USE_THREADED_IRQ	= BIT(2),
>>  };
>>
>>  struct tpm_tis_data {
>> diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
>> index 184396b3af50..f56613f2946f 100644
>> --- a/drivers/char/tpm/tpm_tis_spi_main.c
>> +++ b/drivers/char/tpm/tpm_tis_spi_main.c
>> @@ -223,9 +223,10 @@ static int tpm_tis_spi_probe(struct spi_device *dev)
>>  	phy->flow_control = tpm_tis_spi_flow_control;
>>
>>  	/* If the SPI device has an IRQ then use that */
>> -	if (dev->irq > 0)
>> +	if (dev->irq > 0) {
>>  		irq = dev->irq;
>> -	else
>> +		phy->priv.flags |= TPM_TIS_USE_THREADED_IRQ;
>> +	} else
>>  		irq = -1;
>>
>>  	init_completion(&phy->ready);
>> --
>> 2.36.0
>>
>
>
> BR, Jarkko
>