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

[Jarkko Sakkinen]

On Mon, May 09, 2022 at 10:05:54AM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>

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.

> ---
>  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