| Message ID | 20220509080559.4381-2-LinoSanfilippo@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TPM irq fixes | expand |
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. > --- > 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
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 >
On Wed, May 11, 2022 at 09:18:39PM +0200, Lino Sanfilippo wrote: > 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)? There's zero mention about specific lock you are talking about. Providing the basic knowledge what you are trying to do is the whole point of the commit message in the first place. I'd presume this patch is related to the use bus_lock_mutex but it is fully ingored here. BR, Jarkko
Hi, On 13.05.22 at 20:08, Jarkko Sakkinen wrote: > On Wed, May 11, 2022 at 09:18:39PM +0200, Lino Sanfilippo wrote: >> 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)? > > There's zero mention about specific lock you are talking about. Providing > the basic knowledge what you are trying to do is the whole point of the > commit message in the first place. I'd presume this patch is related to the > use bus_lock_mutex but it is fully ingored here. > Ok, understood. I will amend the commit message to make more clear that reading and writing registers from the interrupt handler results in a call to tpm_tis_spi_transfer() which uses the bus_lock_mutex of the spi device and thus requires a sleepable context. Regards, Lino > BR, Jarkko >
On Mon, 2022-05-16 at 21:52 +0200, Lino Sanfilippo wrote: > Hi, > > On 13.05.22 at 20:08, Jarkko Sakkinen wrote: > > On Wed, May 11, 2022 at 09:18:39PM +0200, Lino Sanfilippo wrote: > > > 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)? > > > > There's zero mention about specific lock you are talking about. Providing > > the basic knowledge what you are trying to do is the whole point of the > > commit message in the first place. I'd presume this patch is related to the > > use bus_lock_mutex but it is fully ingored here. > > > > Ok, understood. I will amend the commit message to make more clear that > reading and writing registers from the interrupt handler results in > a call to tpm_tis_spi_transfer() which uses the bus_lock_mutex of the > spi device and thus requires a sleepable context. Yeah, please be always as explicit as possible, so that it is impossible to get it wrong. Then a reader of your patch saves time from seeking e.g. the current name of the data structure. Just dumb things down as far as you can. Commit messages have a dual function: 1. They *lower* the barrier to look into a code change, which helps the patches get any attention. 2. Proper commit messages save tons of time from the maintainer, when you have revisit years old commits, e.g. when bisecting a bug. Comparing to e.g. Github the key difference is this: in Github you have commits and issues. In kernel commit is both issue and the commit bundled together. Therefore, every commit also needs to have a "bug report" included. BR, Jarkko
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);