| Message ID | 1619394440-30646-2-git-send-email-LinoSanfilippo@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for TPM interrupt handling | expand |
On 4/25/21 7:47 PM, Lino Sanfilippo wrote: > Interrupt handling at least includes reading and writing the interrupt > status register from the interrupt routine. However over SPI those accesses > require a sleepable context, since a mutex is used in the concerning > functions. > For this reason request a threaded interrupt handler which is running in > (sleepable) process context. > > Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
On Mon, Apr 26, 2021 at 01:47:17AM +0200, Lino Sanfilippo wrote: > Interrupt handling at least includes reading and writing the interrupt > status register from the interrupt routine. However over SPI those accesses > require a sleepable context, since a mutex is used in the concerning > functions. > For this reason request a threaded interrupt handler which is running in > (sleepable) process context. > > Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> > --- > drivers/char/tpm/tpm_tis_core.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index e7d1eab..0959559 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -781,8 +781,10 @@ 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 (devm_request_threaded_irq(chip->dev.parent, irq, NULL, > + tis_int_handler, IRQF_ONESHOT | flags, > + dev_name(&chip->dev), chip) != 0) { > dev_info(&chip->dev, "Unable to request irq: %d for probe\n", > irq); > return -1; > -- > 2.7.4 > Why? https://elixir.bootlin.com/linux/v5.12/source/drivers/char/tpm/tpm_tis_core.c#L670 I don't see anything that sleeps there. /Jarkko1
Hi, On 28.04.21 at 01:53, Jarkko Sakkinen wrote: > On Mon, Apr 26, 2021 at 01:47:17AM +0200, Lino Sanfilippo wrote: >> Interrupt handling at least includes reading and writing the interrupt >> status register from the interrupt routine. However over SPI those accesses >> require a sleepable context, since a mutex is used in the concerning >> functions. >> For this reason request a threaded interrupt handler which is running in >> (sleepable) process context. >> >> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> >> --- >> drivers/char/tpm/tpm_tis_core.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c >> index e7d1eab..0959559 100644 >> --- a/drivers/char/tpm/tpm_tis_core.c >> +++ b/drivers/char/tpm/tpm_tis_core.c >> @@ -781,8 +781,10 @@ 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 (devm_request_threaded_irq(chip->dev.parent, irq, NULL, >> + tis_int_handler, IRQF_ONESHOT | flags, >> + dev_name(&chip->dev), chip) != 0) { >> dev_info(&chip->dev, "Unable to request irq: %d for probe\n", >> irq); >> return -1; >> -- >> 2.7.4 >> > > Why? > > https://elixir.bootlin.com/linux/v5.12/source/drivers/char/tpm/tpm_tis_core.c#L670 > > I don't see anything that sleeps there. > > /Jarkko1 > The problem are the register read/write functions which we use to access the status register in the interrupt handler. In case of SPI they result in taking the spi_bus_lock which is a mutex. E.g tpm_tis_spi_read32: tpm_tis_spi_read_bytes->tpm_tis_spi_transfer->spi_bus_lock->mutex_lock Using a threaded interrupt handler seemed to me the easiest way to avoid this issue. Regards, Lino
On Thu, Apr 29, 2021 at 12:37:40AM +0200, Lino Sanfilippo wrote: > > Hi, > > On 28.04.21 at 01:53, Jarkko Sakkinen wrote: > > On Mon, Apr 26, 2021 at 01:47:17AM +0200, Lino Sanfilippo wrote: > >> Interrupt handling at least includes reading and writing the interrupt > >> status register from the interrupt routine. However over SPI those accesses > >> require a sleepable context, since a mutex is used in the concerning > >> functions. > >> For this reason request a threaded interrupt handler which is running in > >> (sleepable) process context. > >> > >> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> > >> --- > >> drivers/char/tpm/tpm_tis_core.c | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > >> index e7d1eab..0959559 100644 > >> --- a/drivers/char/tpm/tpm_tis_core.c > >> +++ b/drivers/char/tpm/tpm_tis_core.c > >> @@ -781,8 +781,10 @@ 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 (devm_request_threaded_irq(chip->dev.parent, irq, NULL, > >> + tis_int_handler, IRQF_ONESHOT | flags, > >> + dev_name(&chip->dev), chip) != 0) { > >> dev_info(&chip->dev, "Unable to request irq: %d for probe\n", > >> irq); > >> return -1; > >> -- > >> 2.7.4 > >> > > > > Why? > > > > https://elixir.bootlin.com/linux/v5.12/source/drivers/char/tpm/tpm_tis_core.c#L670 > > > > I don't see anything that sleeps there. > > > > /Jarkko1 > > > > The problem are the register read/write functions which we use to access the status register in > the interrupt handler. In case of SPI they result in taking the spi_bus_lock which is a mutex. > > E.g tpm_tis_spi_read32: tpm_tis_spi_read_bytes->tpm_tis_spi_transfer->spi_bus_lock->mutex_lock > > Using a threaded interrupt handler seemed to me the easiest way to avoid this issue. > > Regards, > Lino > > > This is a sentence that you should delete: "However over SPI those accesses require a sleepable context, since a mutex is used in the concerning functions. " It neither explains anything who and why sort of stuff. Why don't you put intead something like "Inside tpm_int_handler(), tpm_tis_read32() and tpm_tis_write32() are invoked. The SPI subsystem requires mutex for I/O, which means that the calls ought not to be used inside interrupt context." (I did not check typos). Generally speaking, commit message is as, if not more important than the code change. /Jarkko
Hi, On 29.04.21 at 08:58, Jarkko Sakkinen wrote: > > This is a sentence that you should delete: > > "However over SPI those accesses require a sleepable context, since a > mutex is used in the concerning functions. " > > It neither explains anything who and why sort of stuff. > > Why don't you put intead something like > > "Inside tpm_int_handler(), tpm_tis_read32() and tpm_tis_write32() are > invoked. The SPI subsystem requires mutex for I/O, which means that the > calls ought not to be used inside interrupt context." > > (I did not check typos). Generally speaking, commit message is as, if not > more important than the code change. > > /Jarkko > ok, I will rephrase this in the next patch version. Regards, Lino
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index e7d1eab..0959559 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -781,8 +781,10 @@ 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 (devm_request_threaded_irq(chip->dev.parent, irq, NULL, + tis_int_handler, IRQF_ONESHOT | flags, + dev_name(&chip->dev), chip) != 0) { dev_info(&chip->dev, "Unable to request irq: %d for probe\n", irq); return -1;
Interrupt handling at least includes reading and writing the interrupt status register from the interrupt routine. However over SPI those accesses require a sleepable context, since a mutex is used in the concerning functions. For this reason request a threaded interrupt handler which is running in (sleepable) process context. Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> --- drivers/char/tpm/tpm_tis_core.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)