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
