Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v4 1/6] tpm, tpm_tis_spi: Request threaded irq
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>
References: <20220509080559.4381-1-LinoSanfilippo@gmx.de>
 <20220509080559.4381-2-LinoSanfilippo@gmx.de> <YnucgDH3I87RI8PN@kernel.org>
From:   Lino Sanfilippo <LinoSanfilippo@gmx.de>
Message-ID: <486cec01-ec02-3f11-0b81-037e0700c503@gmx.de>
Date:   Wed, 11 May 2022 21:18:39 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <YnucgDH3I87RI8PN@kernel.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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 S=
PI
>> bus are synchronized by a mutex, request a threaded interrupt handler t=
o
>> 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 emphasi=
ze more
that the irq handler is running in hard irq context in the current code an=
d thus
must not access registers over SPI since SPI uses a mutex (I consider it a=
s 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_chi=
p *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) !=3D 0) {
>> +
>> +	if (priv->flags & TPM_TIS_USE_THREADED_IRQ) {
>> +		rc =3D devm_request_threaded_irq(chip->dev.parent, irq, NULL,
>> +					       tis_int_handler,
>> +					       IRQF_ONESHOT | flags,
>> +					       dev_name(&chip->dev),
>> +					       chip);
>> +	} else {
>> +		rc =3D 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		=3D BIT(0),
>>  	TPM_TIS_INVALID_STATUS		=3D BIT(1),
>> +	TPM_TIS_USE_THREADED_IRQ	=3D 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 *de=
v)
>>  	phy->flow_control =3D tpm_tis_spi_flow_control;
>>
>>  	/* If the SPI device has an IRQ then use that */
>> -	if (dev->irq > 0)
>> +	if (dev->irq > 0) {
>>  		irq =3D dev->irq;
>> -	else
>> +		phy->priv.flags |=3D TPM_TIS_USE_THREADED_IRQ;
>> +	} else
>>  		irq =3D -1;
>>
>>  	init_completion(&phy->ready);
>> --
>> 2.36.0
>>
>
>
> BR, Jarkko
>