| Message ID | 20210501135727.17747-3-LinoSanfilippo@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for TPM interrupt handling | expand |
What the heck is "simplification" and what that has to do with fixing anything? I don't understand your terminology. On Sat, May 01, 2021 at 03:57:25PM +0200, Lino Sanfilippo wrote: > Currently the TPM (default) locality is claimed and released for each > access to the TPM registers which require a claimed locality. This results > in locality claim/release combos at various code places. For interrupt > handling we also need such a combo in the interrupt handler (for clearing > the interrupts) which makes the locality handling even more complicated > since now we also have to synchronize concurrent accesses in process and in > interrupt context. > > Since currently the driver uses only one locality anyway, avoid the > increasing complexity by claiming it once at driver startup and only > releasing it at driver shutdown. > > Due to the simplifications the functions tpm_request_locality() and > tpm_relinquish_locality() are not needed any more an can be removed. > > As a side-effect these modifications fix a bug which results in the > following warning when using TPM 2: > > TPM returned invalid status > WARNING: CPU: 0 PID: 874 at drivers/char/tpm/tpm_tis_core.c:249 tpm_tis_status+0xbc/0xc8 [tpm_tis_core] > Modules linked in: tpm_tis_spi tpm_tis_core tpm sha256_generic cfg80211 rfkill 8021q garp stp llc spidev v3d gpu_sched vc4 bcm2835_codec(C) cec raspberrypi_hwmon drm_kms_helper drm bcm2835_isp(C) v4l2_mem2mem bcm2835_v4l2(C) bcm2835_mmal_vchiq(C) videobuf2_vmalloc videobuf2_dma_contig snd_bcm2835(C) videobuf2_memops drm_panel_orientation_quirks videobuf2_v4l2 videobuf2_common snd_soc_core snd_compress videodev snd_pcm_dmaengine spi_bcm2835 snd_pcm mc vc_sm_cma(C) snd_timer snd syscopyarea rpivid_mem sysfillrect sysimgblt fb_sys_fops backlight uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: tpm] > CPU: 0 PID: 874 Comm: kworker/u8:1 Tainted: G WC 5.11.0-rc2-LS-HOME+ #1 > Hardware name: Raspberry Pi Compute Module 4 Rev 1.0 (DT) > Workqueue: events_unbound async_run_entry_fn > pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--) > pc : tpm_tis_status+0xbc/0xc8 [tpm_tis_core] > lr : tpm_tis_status+0xbc/0xc8 [tpm_tis_core] > sp : ffffffc0127e38f0 > x29: ffffffc0127e38f0 x28: ffffffc011238000 > x27: 0000000000000016 x26: 000000000000017a > x25: 0000000000000014 x24: ffffff8047f60000 > x23: 0000000000000000 x22: 0000000000000016 > x21: ffffff8044e8a480 x20: 0000000000000000 > x19: ffffffc011238000 x18: ffffffc011238948 > x17: 0000000000000000 x16: 0000000000000000 > x15: ffffffc01141be81 x14: ffffffffffffffff > x13: ffffffc01141be7e x12: ffffffffffffffff > x11: ffffff807fb797f0 x10: 00000000000019b0 > x9 : ffffffc0127e38f0 x8 : 766e692064656e72 > x7 : 0000000000000000 x6 : ffffffc011239000 > x5 : ffffff807fb628b8 x4 : 0000000000000000 > x3 : 0000000000000027 x2 : 0000000000000000 > x1 : 6818b6f22fdef800 x0 : 0000000000000000 > Call trace: > tpm_tis_status+0xbc/0xc8 [tpm_tis_core] > tpm_tis_send_data+0x58/0x250 [tpm_tis_core] > tpm_tis_send_main+0x50/0x128 [tpm_tis_core] > tpm_tis_send+0x4c/0xe0 [tpm_tis_core] > tpm_transmit+0xd0/0x350 [tpm] > tpm_transmit_cmd+0x3c/0xc0 [tpm] > tpm2_get_tpm_pt+0x124/0x1e8 [tpm] > tpm_tis_probe_irq_single+0x198/0x364 [tpm_tis_core] > tpm_tis_core_init+0x304/0x520 [tpm_tis_core] > tpm_tis_spi_init+0x5c/0x78 [tpm_tis_spi] > tpm_tis_spi_probe+0x80/0x98 [tpm_tis_spi] > tpm_tis_spi_driver_probe+0x4c/0x60 [tpm_tis_spi] > spi_probe+0x84/0xf0 > really_probe+0x118/0x420 > driver_probe_device+0x5c/0xc0 > __driver_attach_async_helper+0x64/0x68 > async_run_entry_fn+0x48/0x150 > process_one_work+0x15c/0x4d0 > worker_thread+0x50/0x490 > kthread+0x118/0x150 > ret_from_fork+0x10/0x1c > > The reason for this issue is that in case of TPM 2 function > tpm2_get_timeouts() which executes a TPM command is called without a > claimed locality. Since with this patch the locality is taken once at > driver startup and never released before shutdown the issue does not occur > any more. Please rather create fix that fixes the issue exactly in the code path. I don't want to worry what other things this might do "as a side-effect". Also, lacks the explanation why this patch fixes the issue. > Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()") > Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> > --- > drivers/char/tpm/tpm-chip.c | 40 --------------------------------- > drivers/char/tpm/tpm_tis_core.c | 35 +++++++++-------------------- > include/linux/tpm.h | 3 --- > 3 files changed, 10 insertions(+), 68 deletions(-) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index ddaeceb7e109..a09b6523261e 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -32,35 +32,6 @@ struct class *tpm_class; > struct class *tpmrm_class; > dev_t tpm_devt; > > -static int tpm_request_locality(struct tpm_chip *chip) > -{ > - int rc; > - > - if (!chip->ops->request_locality) > - return 0; > - > - rc = chip->ops->request_locality(chip, 0); > - if (rc < 0) > - return rc; > - > - chip->locality = rc; > - return 0; > -} > - > -static void tpm_relinquish_locality(struct tpm_chip *chip) > -{ > - int rc; > - > - if (!chip->ops->relinquish_locality) > - return; > - > - rc = chip->ops->relinquish_locality(chip, chip->locality); > - if (rc) > - dev_err(&chip->dev, "%s: : error %d\n", __func__, rc); > - > - chip->locality = -1; > -} > - > static int tpm_cmd_ready(struct tpm_chip *chip) > { > if (!chip->ops->cmd_ready) > @@ -103,17 +74,8 @@ int tpm_chip_start(struct tpm_chip *chip) > > tpm_clk_enable(chip); > > - if (chip->locality == -1) { > - ret = tpm_request_locality(chip); > - if (ret) { > - tpm_clk_disable(chip); > - return ret; > - } > - } > - > ret = tpm_cmd_ready(chip); > if (ret) { > - tpm_relinquish_locality(chip); > tpm_clk_disable(chip); > return ret; > } > @@ -133,7 +95,6 @@ EXPORT_SYMBOL_GPL(tpm_chip_start); > void tpm_chip_stop(struct tpm_chip *chip) > { > tpm_go_idle(chip); > - tpm_relinquish_locality(chip); > tpm_clk_disable(chip); > } > EXPORT_SYMBOL_GPL(tpm_chip_stop); > @@ -392,7 +353,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, > goto out; > } > > - chip->locality = -1; > return chip; > > out: > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index a12992ae2a3e..f892b1ae46f2 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -626,9 +626,6 @@ static int probe_itpm(struct tpm_chip *chip) > if (vendor != TPM_VID_INTEL) > return 0; > > - if (request_locality(chip, 0) != 0) > - return -EBUSY; > - > rc = tpm_tis_send_data(chip, cmd_getticks, len); > if (rc == 0) > goto out; > @@ -647,7 +644,6 @@ static int probe_itpm(struct tpm_chip *chip) > > out: > tpm_tis_ready(chip); > - release_locality(chip, priv->locality); > > return rc; > } > @@ -707,22 +703,13 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) > const char *desc = "attempting to generate an interrupt"; > u32 cap2; > cap_t cap; > - int ret; > > /* TPM 2.0 */ > if (chip->flags & TPM_CHIP_FLAG_TPM2) > return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc); > > /* TPM 1.2 */ > - ret = request_locality(chip, 0); > - if (ret < 0) > - return ret; > - > - ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0); > - > - release_locality(chip, 0); > - > - return ret; > + return tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0); > } > > /* Register the IRQ and issue a command that will cause an interrupt. If an > @@ -836,6 +823,7 @@ void tpm_tis_remove(struct tpm_chip *chip) > > tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt); > > + release_locality(chip, 0); > tpm_tis_clkrun_enable(chip, false); > > if (priv->ilb_base_addr) > @@ -963,6 +951,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > goto out_err; > } > > + rc = request_locality(chip, 0); > + if (rc) > + goto out_err; > + > + rc = tpm_chip_start(chip); > + if (rc) > + goto out_err; > + > /* Take control of the TPM's interrupt hardware and shut it off */ > rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask); > if (rc < 0) > @@ -973,9 +969,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > intmask &= ~TPM_GLOBAL_INT_ENABLE; > tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); > > - rc = tpm_chip_start(chip); > - if (rc) > - goto out_err; > rc = tpm2_probe(chip); > tpm_chip_stop(chip); > if (rc) > @@ -1036,15 +1029,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > * to make sure it works. May as well use that command to set the > * proper timeouts for the driver. > */ > - > - rc = request_locality(chip, 0); > - if (rc < 0) > - goto out_err; > - > rc = tpm_get_timeouts(chip); > - > - release_locality(chip, 0); > - > if (rc) { > dev_err(dev, "Could not get TPM timeouts and durations\n"); > rc = -ENODEV; > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > index aa11fe323c56..7a68832b14bb 100644 > --- a/include/linux/tpm.h > +++ b/include/linux/tpm.h > @@ -167,9 +167,6 @@ struct tpm_chip { > u32 last_cc; > u32 nr_commands; > u32 *cc_attrs_tbl; > - > - /* active locality */ > - int locality; > }; > > #define TPM_HEADER_SIZE 10 > -- > 2.31.1 > /Jarkko
Hi, On 03.05.21 at 17:50, Jarkko Sakkinen wrote: > What the heck is "simplification" and what that has to do with fixing > anything? I don't understand your terminology. The intention for this patch is not to fix anything. Please read the cover letter and the commit message. This patch is about making the locality handling easier by not claiming/releasing it multiple times over the driver life time, but claiming it once at driver startup and only releasing it at driver shutdown. Right now we have locality request/release combos in - probe_itpm() - tpm_tis_gen_interrupt() - tpm_tis_core_init() - tpm_chip_start() and there is still one combo missing for - tpm2_get_timeouts() which is the reason why we get the "TPM returned invalid status" bug in case of TPM2 (and this is the bug which is _incidentally_ fixed by this patch, see below). And if we are going to enable interrupts, we have to introduce yet another combo, for accessing the status register in the interrupt handler, since TPM 2.0 requires holding the locality for writing to the status register. That makes 6 different code places in which we take and release the locality. With this patch applied we only take the locality at one place. Furthermore with interrupts enabled we dont have to claim the locality for each handler execution, saving us countless claim/release combinations at runtime. Hence the term "simplification" which is perfectly justified IMO. So again, this patch is "only" in preparation for the next patch when interrupts are actually enabled and we would have to take the locality in the interrupt handler without this patch. > On Sat, May 01, 2021 at 03:57:25PM +0200, Lino Sanfilippo wrote: >> WARNING: CPU: 0 PID: 874 at drivers/char/tpm/tpm_tis_core.c:249 tpm_tis_status+0xbc/0xc8 [tpm_tis_core] >> Modules linked in: tpm_tis_spi tpm_tis_core tpm sha256_generic cfg80211 rfkill 8021q garp stp llc spidev v3d gpu_sched vc4 bcm2835_codec(C) cec raspberrypi_hwmon drm_kms_helper drm bcm2835_isp(C) v4l2_mem2mem bcm2835_v4l2(C) bcm2835_mmal_vchiq(C) videobuf2_vmalloc videobuf2_dma_contig snd_bcm2835(C) videobuf2_memops drm_panel_orientation_quirks videobuf2_v4l2 videobuf2_common snd_soc_core snd_compress videodev snd_pcm_dmaengine spi_bcm2835 snd_pcm mc vc_sm_cma(C) snd_timer snd syscopyarea rpivid_mem sysfillrect sysimgblt fb_sys_fops backlight uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: tpm] >> CPU: 0 PID: 874 Comm: kworker/u8:1 Tainted: G WC 5.11.0-rc2-LS-HOME+ #1 >> Hardware name: Raspberry Pi Compute Module 4 Rev 1.0 (DT) >> Workqueue: events_unbound async_run_entry_fn >> pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--) >> pc : tpm_tis_status+0xbc/0xc8 [tpm_tis_core] >> lr : tpm_tis_status+0xbc/0xc8 [tpm_tis_core] >> sp : ffffffc0127e38f0 >> x29: ffffffc0127e38f0 x28: ffffffc011238000 >> x27: 0000000000000016 x26: 000000000000017a >> x25: 0000000000000014 x24: ffffff8047f60000 >> x23: 0000000000000000 x22: 0000000000000016 >> x21: ffffff8044e8a480 x20: 0000000000000000 >> x19: ffffffc011238000 x18: ffffffc011238948 >> x17: 0000000000000000 x16: 0000000000000000 >> x15: ffffffc01141be81 x14: ffffffffffffffff >> x13: ffffffc01141be7e x12: ffffffffffffffff >> x11: ffffff807fb797f0 x10: 00000000000019b0 >> x9 : ffffffc0127e38f0 x8 : 766e692064656e72 >> x7 : 0000000000000000 x6 : ffffffc011239000 >> x5 : ffffff807fb628b8 x4 : 0000000000000000 >> x3 : 0000000000000027 x2 : 0000000000000000 >> x1 : 6818b6f22fdef800 x0 : 0000000000000000 >> Call trace: >> tpm_tis_status+0xbc/0xc8 [tpm_tis_core] >> tpm_tis_send_data+0x58/0x250 [tpm_tis_core] >> tpm_tis_send_main+0x50/0x128 [tpm_tis_core] >> tpm_tis_send+0x4c/0xe0 [tpm_tis_core] >> tpm_transmit+0xd0/0x350 [tpm] >> tpm_transmit_cmd+0x3c/0xc0 [tpm] >> tpm2_get_tpm_pt+0x124/0x1e8 [tpm] >> tpm_tis_probe_irq_single+0x198/0x364 [tpm_tis_core] >> tpm_tis_core_init+0x304/0x520 [tpm_tis_core] >> tpm_tis_spi_init+0x5c/0x78 [tpm_tis_spi] >> tpm_tis_spi_probe+0x80/0x98 [tpm_tis_spi] >> tpm_tis_spi_driver_probe+0x4c/0x60 [tpm_tis_spi] >> spi_probe+0x84/0xf0 >> really_probe+0x118/0x420 >> driver_probe_device+0x5c/0xc0 >> __driver_attach_async_helper+0x64/0x68 >> async_run_entry_fn+0x48/0x150 >> process_one_work+0x15c/0x4d0 >> worker_thread+0x50/0x490 >> kthread+0x118/0x150 >> ret_from_fork+0x10/0x1c >> >> The reason for this issue is that in case of TPM 2 function >> tpm2_get_timeouts() which executes a TPM command is called without a >> claimed locality. Since with this patch the locality is taken once at >> driver startup and never released before shutdown the issue does not occur >> any more. > > Please rather create fix that fixes the issue exactly in the code path. > I don't want to worry what other things this might do "as a side-effect". As explained above this patch is not meant to fix a bug in the first place but rather fixes the bug above incidentally by the locality handling reimplementation. > Also, lacks the explanation why this patch fixes the issue. > The explanation is there: "The reason for this issue is that in case of TPM 2 function tpm2_get_timeouts() which executes a TPM command is called without a claimed locality. Since with this patch the locality is taken once at driver startup and never released before shutdown the issue does not occur any more." I really dont know how to describe this more clear. Lino
On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote: > Hi, > > On 03.05.21 at 17:50, Jarkko Sakkinen wrote: > > What the heck is "simplification" and what that has to do with fixing > > anything? I don't understand your terminology. > > > The intention for this patch is not to fix anything. Please read the cover > letter and the commit message. > This patch is about making the locality handling easier by not claiming/releasing > it multiple times over the driver life time, but claiming it once at driver > startup and only releasing it at driver shutdown. > > Right now we have locality request/release combos in > > - probe_itpm() > - tpm_tis_gen_interrupt() > - tpm_tis_core_init() > - tpm_chip_start() > > and there is still one combo missing for > > - tpm2_get_timeouts() > > which is the reason why we get the "TPM returned invalid status" bug in case > of TPM2 (and this is the bug which is _incidentally_ fixed by this patch, see > below). > > And if we are going to enable interrupts, we have to introduce yet another combo, > for accessing the status register in the interrupt handler, since TPM 2.0 > requires holding the locality for writing to the status register. That makes > 6 different code places in which we take and release the locality. > > With this patch applied we only take the locality at one place. Furthermore > with interrupts enabled we dont have to claim the locality for each handler > execution, saving us countless claim/release combinations at runtime. > > Hence the term "simplification" which is perfectly justified IMO. > > So again, this patch is "only" in preparation for the next patch when interrupts > are actually enabled and we would have to take the locality in the interrupt > handler without this patch. So: what problem this patch does solve? /Jarkko
Hi guys, On Thu, 2021-05-06 at 04:47 +0300, Jarkko Sakkinen wrote: > On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote: > > Hi, > > > > On 03.05.21 at 17:50, Jarkko Sakkinen wrote: > > > What the heck is "simplification" and what that has to do with fixing > > > anything? I don't understand your terminology. > > > > > > The intention for this patch is not to fix anything. Please read the cover > > letter and the commit message. > > This patch is about making the locality handling easier by not > > claiming/releasing > > it multiple times over the driver life time, but claiming it once at driver > > startup and only releasing it at driver shutdown. > > > > Right now we have locality request/release combos in > > > > - probe_itpm() > > - tpm_tis_gen_interrupt() > > - tpm_tis_core_init() > > - tpm_chip_start() > > > > and there is still one combo missing for > > > > - tpm2_get_timeouts() > > > > which is the reason why we get the "TPM returned invalid status" bug in case > > of TPM2 (and this is the bug which is _incidentally_ fixed by this patch, > > see > > below). > > > > And if we are going to enable interrupts, we have to introduce yet another > > combo, > > for accessing the status register in the interrupt handler, since TPM 2.0 > > requires holding the locality for writing to the status register. That makes > > 6 different code places in which we take and release the locality. > > > > With this patch applied we only take the locality at one place. Furthermore > > with interrupts enabled we dont have to claim the locality for each handler > > execution, saving us countless claim/release combinations at runtime. > > > > Hence the term "simplification" which is perfectly justified IMO. > > > > So again, this patch is "only" in preparation for the next patch when > > interrupts > > are actually enabled and we would have to take the locality in the interrupt > > handler without this patch. > > So: what problem this patch does solve? > > /Jarkko > first, thank you very much, Lino, for working on this! I've been debugging issues with the tis driver in the last days and was about to start with the same approach as yours when I luckily discovered your patch! Jarkko, while I agree, that the commit message is not optimal, Lino tried hard to explain what the problems with the current code are and how they are / can be fixed. Further, I too don't see why simplification / optimization is such a bad thing. This driver is actually a very good example. I had a hard time, too, figuring out what's going on there. A clean rewrite is a very valid approach here IMO. It's not "polishing for nothing", as you described it, but actually solving problems. Interrupt detection is broken for years now and finally a volunteer worked on a solution. Don't you think this should be valued? Let's get this problem sorted out :-) Lino, I'd be happy to test the patches, when you have time and interest to work on this again! Thanks, Michael
On Thu, Mar 24, 2022 at 06:04:23PM +0100, Michael Niewöhner wrote: > Hi guys, > > On Thu, 2021-05-06 at 04:47 +0300, Jarkko Sakkinen wrote: > > On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote: > > > Hi, > > > > > > On 03.05.21 at 17:50, Jarkko Sakkinen wrote: > > > > What the heck is "simplification" and what that has to do with fixing > > > > anything? I don't understand your terminology. > > > > > > > > > The intention for this patch is not to fix anything. Please read the cover > > > letter and the commit message. > > > This patch is about making the locality handling easier by not > > > claiming/releasing > > > it multiple times over the driver life time, but claiming it once at driver > > > startup and only releasing it at driver shutdown. > > > > > > Right now we have locality request/release combos in > > > > > > - probe_itpm() > > > - tpm_tis_gen_interrupt() > > > - tpm_tis_core_init() > > > - tpm_chip_start() > > > > > > and there is still one combo missing for > > > > > > - tpm2_get_timeouts() > > > > > > which is the reason why we get the "TPM returned invalid status" bug in case > > > of TPM2 (and this is the bug which is _incidentally_ fixed by this patch, > > > see > > > below). > > > > > > And if we are going to enable interrupts, we have to introduce yet another > > > combo, > > > for accessing the status register in the interrupt handler, since TPM 2.0 > > > requires holding the locality for writing to the status register. That makes > > > 6 different code places in which we take and release the locality. > > > > > > With this patch applied we only take the locality at one place. Furthermore > > > with interrupts enabled we dont have to claim the locality for each handler > > > execution, saving us countless claim/release combinations at runtime. > > > > > > Hence the term "simplification" which is perfectly justified IMO. > > > > > > So again, this patch is "only" in preparation for the next patch when > > > interrupts > > > are actually enabled and we would have to take the locality in the interrupt > > > handler without this patch. > > > > So: what problem this patch does solve? > > > > /Jarkko > > > > first, thank you very much, Lino, for working on this! I've been debugging > issues with the tis driver in the last days and was about to start with the same > approach as yours when I luckily discovered your patch! > > Jarkko, while I agree, that the commit message is not optimal, Lino tried hard > to explain what the problems with the current code are and how they are / can be > fixed. Further, I too don't see why simplification / optimization is such a bad > thing. This driver is actually a very good example. I had a hard time, too, > figuring out what's going on there. A clean rewrite is a very valid approach > here IMO. It's not "polishing for nothing", as you described it, but actually > solving problems. > > Interrupt detection is broken for years now and finally a volunteer worked on a > solution. Don't you think this should be valued? Let's get this problem sorted > out :-) > > Lino, I'd be happy to test the patches, when you have time and interest to work > on this again! > > Thanks, Michael It's quite easy to test them out. Both fixes are in the mainline GIT tree. E.g. give a shot rc1, and please report if any issues persists to: linux-integrity@vger.kernel.org BR, Jarkko
On Fri, 2022-03-25 at 04:14 +0200, Jarkko Sakkinen wrote: > On Thu, Mar 24, 2022 at 06:04:23PM +0100, Michael Niewöhner wrote: > > Hi guys, > > > > On Thu, 2021-05-06 at 04:47 +0300, Jarkko Sakkinen wrote: > > > On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote: > > > > Hi, > > > > > > > > On 03.05.21 at 17:50, Jarkko Sakkinen wrote: > > > > > What the heck is "simplification" and what that has to do with fixing > > > > > anything? I don't understand your terminology. > > > > > > > > > > > > The intention for this patch is not to fix anything. Please read the > > > > cover > > > > letter and the commit message. > > > > This patch is about making the locality handling easier by not > > > > claiming/releasing > > > > it multiple times over the driver life time, but claiming it once at > > > > driver > > > > startup and only releasing it at driver shutdown. > > > > > > > > Right now we have locality request/release combos in > > > > > > > > - probe_itpm() > > > > - tpm_tis_gen_interrupt() > > > > - tpm_tis_core_init() > > > > - tpm_chip_start() > > > > > > > > and there is still one combo missing for > > > > > > > > - tpm2_get_timeouts() > > > > > > > > which is the reason why we get the "TPM returned invalid status" bug in > > > > case > > > > of TPM2 (and this is the bug which is _incidentally_ fixed by this > > > > patch, > > > > see > > > > below). > > > > > > > > And if we are going to enable interrupts, we have to introduce yet > > > > another > > > > combo, > > > > for accessing the status register in the interrupt handler, since TPM > > > > 2.0 > > > > requires holding the locality for writing to the status register. That > > > > makes > > > > 6 different code places in which we take and release the locality. > > > > > > > > With this patch applied we only take the locality at one place. > > > > Furthermore > > > > with interrupts enabled we dont have to claim the locality for each > > > > handler > > > > execution, saving us countless claim/release combinations at runtime. > > > > > > > > Hence the term "simplification" which is perfectly justified IMO. > > > > > > > > So again, this patch is "only" in preparation for the next patch when > > > > interrupts > > > > are actually enabled and we would have to take the locality in the > > > > interrupt > > > > handler without this patch. > > > > > > So: what problem this patch does solve? > > > > > > /Jarkko > > > > > > > first, thank you very much, Lino, for working on this! I've been debugging > > issues with the tis driver in the last days and was about to start with the > > same > > approach as yours when I luckily discovered your patch! > > > > Jarkko, while I agree, that the commit message is not optimal, Lino tried > > hard > > to explain what the problems with the current code are and how they are / > > can be > > fixed. Further, I too don't see why simplification / optimization is such a > > bad > > thing. This driver is actually a very good example. I had a hard time, too, > > figuring out what's going on there. A clean rewrite is a very valid approach > > here IMO. It's not "polishing for nothing", as you described it, but > > actually > > solving problems. > > > > Interrupt detection is broken for years now and finally a volunteer worked > > on a > > solution. Don't you think this should be valued? Let's get this problem > > sorted > > out :-) > > > > Lino, I'd be happy to test the patches, when you have time and interest to > > work > > on this again! > > > > Thanks, Michael > > It's quite easy to test them out. Both fixes are in the mainline GIT tree. > E.g. give a shot rc1, and please report if any issues persists to: > > linux-integrity@vger.kernel.org > > BR, Jarkko I don't see Linos patches on mainline. Also, the series included four patches: [PATCH v3 0/4] Fixes for TPM interrupt handling [PATCH v3 1/4] tpm: Use a threaded interrupt handler [PATCH v3 2/4] tpm: Simplify locality handling [PATCH v3 3/4] tpm: Fix test for interrupts [PATCH v3 4/4] tpm: Only enable supported irqs Three of them are relevant for the interrupt problem, which is still present in mainline, as these patches were refused: [PATCH v3 1/4] tpm: Use a threaded interrupt handler [PATCH v3 2/4] tpm: Simplify locality handling [PATCH v3 3/4] tpm: Fix test for interrupts Michael
Hi Michael, On 25.03.22 at 13:32, Michael Niewöhner wrote: >>> >>> Lino, I'd be happy to test the patches, when you have time and interest to >>> work >>> on this again! >>> >>> Thanks, Michael >> >> It's quite easy to test them out. Both fixes are in the mainline GIT tree. >> E.g. give a shot rc1, and please report if any issues persists to: >> >> linux-integrity@vger.kernel.org >> >> BR, Jarkko > > I don't see Linos patches on mainline. Also, the series included four patches: > [PATCH v3 0/4] Fixes for TPM interrupt handling > [PATCH v3 1/4] tpm: Use a threaded interrupt handler > [PATCH v3 2/4] tpm: Simplify locality handling > [PATCH v3 3/4] tpm: Fix test for interrupts > [PATCH v3 4/4] tpm: Only enable supported irqs > > Three of them are relevant for the interrupt problem, which is still present in > mainline, as these patches were refused: > [PATCH v3 1/4] tpm: Use a threaded interrupt handler > [PATCH v3 2/4] tpm: Simplify locality handling > [PATCH v3 3/4] tpm: Fix test for interrupts > > Michael > You are right, the interrupts are still not working in the mainline kernel. I would gladly make another attempt to fix this but rather step by step than in a series that tries to fix (different) things at once. A first step could be to have a sleepable context for the interrupt handling, since in case of SPI the accesses to the irq status register may sleep. I sent a patch for this purpose once, but it seems to have gone lost: https://lore.kernel.org/all/20210620023444.14684-1-LinoSanfilippo@gmx.de/ Best regards, Lino
Hi Lino, On Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote: > > Hi Michael, > > On 25.03.22 at 13:32, Michael Niewöhner wrote: > > > > > > > > Lino, I'd be happy to test the patches, when you have time and interest > > > > to > > > > work > > > > on this again! > > > > > > > > Thanks, Michael > > > > > > It's quite easy to test them out. Both fixes are in the mainline GIT tree. > > > E.g. give a shot rc1, and please report if any issues persists to: > > > > > > linux-integrity@vger.kernel.org > > > > > > BR, Jarkko > > > > I don't see Linos patches on mainline. Also, the series included four > > patches: > > [PATCH v3 0/4] Fixes for TPM interrupt handling > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler > > [PATCH v3 2/4] tpm: Simplify locality handling > > [PATCH v3 3/4] tpm: Fix test for interrupts > > [PATCH v3 4/4] tpm: Only enable supported irqs > > > > Three of them are relevant for the interrupt problem, which is still present > > in > > mainline, as these patches were refused: > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler > > [PATCH v3 2/4] tpm: Simplify locality handling > > [PATCH v3 3/4] tpm: Fix test for interrupts > > > > Michael > > > > You are right, the interrupts are still not working in the mainline kernel. > I would gladly make another attempt to fix this but rather step by step > than in a series that tries to fix (different) things at once. IMHO a series is perfectly fine, as it's easier to show *why* single changes are required (like already done in v3 0/4). One just has to actually *read* what's written there. Ahem. It's up to you, though. > > A first step could be to have a sleepable context for the interrupt handling, > since in case of SPI the accesses to the irq status register may sleep. > > I sent a patch for this purpose once, but it seems to have gone lost: > > https://lore.kernel.org/all/20210620023444.14684-1-LinoSanfilippo@gmx.de/ > Jarkko, looks like you've already tested that patch on your NUC? > > Best regards, > Lino > Michael
On Fri, Mar 25, 2022 at 01:32:25PM +0100, Michael Niewöhner wrote: > On Fri, 2022-03-25 at 04:14 +0200, Jarkko Sakkinen wrote: > > On Thu, Mar 24, 2022 at 06:04:23PM +0100, Michael Niewöhner wrote: > > > Hi guys, > > > > > > On Thu, 2021-05-06 at 04:47 +0300, Jarkko Sakkinen wrote: > > > > On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote: > > > > > Hi, > > > > > > > > > > On 03.05.21 at 17:50, Jarkko Sakkinen wrote: > > > > > > What the heck is "simplification" and what that has to do with fixing > > > > > > anything? I don't understand your terminology. > > > > > > > > > > > > > > > The intention for this patch is not to fix anything. Please read the > > > > > cover > > > > > letter and the commit message. > > > > > This patch is about making the locality handling easier by not > > > > > claiming/releasing > > > > > it multiple times over the driver life time, but claiming it once at > > > > > driver > > > > > startup and only releasing it at driver shutdown. > > > > > > > > > > Right now we have locality request/release combos in > > > > > > > > > > - probe_itpm() > > > > > - tpm_tis_gen_interrupt() > > > > > - tpm_tis_core_init() > > > > > - tpm_chip_start() > > > > > > > > > > and there is still one combo missing for > > > > > > > > > > - tpm2_get_timeouts() > > > > > > > > > > which is the reason why we get the "TPM returned invalid status" bug in > > > > > case > > > > > of TPM2 (and this is the bug which is _incidentally_ fixed by this > > > > > patch, > > > > > see > > > > > below). > > > > > > > > > > And if we are going to enable interrupts, we have to introduce yet > > > > > another > > > > > combo, > > > > > for accessing the status register in the interrupt handler, since TPM > > > > > 2.0 > > > > > requires holding the locality for writing to the status register. That > > > > > makes > > > > > 6 different code places in which we take and release the locality. > > > > > > > > > > With this patch applied we only take the locality at one place. > > > > > Furthermore > > > > > with interrupts enabled we dont have to claim the locality for each > > > > > handler > > > > > execution, saving us countless claim/release combinations at runtime. > > > > > > > > > > Hence the term "simplification" which is perfectly justified IMO. > > > > > > > > > > So again, this patch is "only" in preparation for the next patch when > > > > > interrupts > > > > > are actually enabled and we would have to take the locality in the > > > > > interrupt > > > > > handler without this patch. > > > > > > > > So: what problem this patch does solve? > > > > > > > > /Jarkko > > > > > > > > > > first, thank you very much, Lino, for working on this! I've been debugging > > > issues with the tis driver in the last days and was about to start with the > > > same > > > approach as yours when I luckily discovered your patch! > > > > > > Jarkko, while I agree, that the commit message is not optimal, Lino tried > > > hard > > > to explain what the problems with the current code are and how they are / > > > can be > > > fixed. Further, I too don't see why simplification / optimization is such a > > > bad > > > thing. This driver is actually a very good example. I had a hard time, too, > > > figuring out what's going on there. A clean rewrite is a very valid approach > > > here IMO. It's not "polishing for nothing", as you described it, but > > > actually > > > solving problems. > > > > > > Interrupt detection is broken for years now and finally a volunteer worked > > > on a > > > solution. Don't you think this should be valued? Let's get this problem > > > sorted > > > out :-) > > > > > > Lino, I'd be happy to test the patches, when you have time and interest to > > > work > > > on this again! > > > > > > Thanks, Michael > > > > It's quite easy to test them out. Both fixes are in the mainline GIT tree. > > E.g. give a shot rc1, and please report if any issues persists to: > > > > linux-integrity@vger.kernel.org > > > > BR, Jarkko > > I don't see Linos patches on mainline. Also, the series included four patches: > [PATCH v3 0/4] Fixes for TPM interrupt handling > [PATCH v3 1/4] tpm: Use a threaded interrupt handler > [PATCH v3 2/4] tpm: Simplify locality handling > [PATCH v3 3/4] tpm: Fix test for interrupts > [PATCH v3 4/4] tpm: Only enable supported irqs > > Three of them are relevant for the interrupt problem, which is still present in > mainline, as these patches were refused: > [PATCH v3 1/4] tpm: Use a threaded interrupt handler > [PATCH v3 2/4] tpm: Simplify locality handling > [PATCH v3 3/4] tpm: Fix test for interrupts > > Michael There was some unaddressed feedback, most of not that hard to address. I'm waiting for v4. BR, Jarkko
On Sat, Mar 26, 2022 at 04:24:55AM +0100, Lino Sanfilippo wrote: > > Hi Michael, > > On 25.03.22 at 13:32, Michael Niewöhner wrote: > >>> > >>> Lino, I'd be happy to test the patches, when you have time and interest to > >>> work > >>> on this again! > >>> > >>> Thanks, Michael > >> > >> It's quite easy to test them out. Both fixes are in the mainline GIT tree. > >> E.g. give a shot rc1, and please report if any issues persists to: > >> > >> linux-integrity@vger.kernel.org > >> > >> BR, Jarkko > > > > I don't see Linos patches on mainline. Also, the series included four patches: > > [PATCH v3 0/4] Fixes for TPM interrupt handling > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler > > [PATCH v3 2/4] tpm: Simplify locality handling > > [PATCH v3 3/4] tpm: Fix test for interrupts > > [PATCH v3 4/4] tpm: Only enable supported irqs > > > > Three of them are relevant for the interrupt problem, which is still present in > > mainline, as these patches were refused: > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler > > [PATCH v3 2/4] tpm: Simplify locality handling > > [PATCH v3 3/4] tpm: Fix test for interrupts > > > > Michael > > > > You are right, the interrupts are still not working in the mainline kernel. > I would gladly make another attempt to fix this but rather step by step > than in a series that tries to fix (different) things at once. > > A first step could be to have a sleepable context for the interrupt handling, > since in case of SPI the accesses to the irq status register may sleep. > > I sent a patch for this purpose once, but it seems to have gone lost: > > https://lore.kernel.org/all/20210620023444.14684-1-LinoSanfilippo@gmx.de/ > > > Best regards, > Lino This is clearly my bad I'll test this ASAP. BR, Jarkko
n Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote: > > Hi Michael, > > On 25.03.22 at 13:32, Michael Niewöhner wrote: > > > > > > > > Lino, I'd be happy to test the patches, when you have time and interest to > > > > work > > > > on this again! > > > > > > > > Thanks, Michael > > > > > > It's quite easy to test them out. Both fixes are in the mainline GIT tree. > > > E.g. give a shot rc1, and please report if any issues persists to: > > > > > > linux-integrity@vger.kernel.org > > > > > > BR, Jarkko > > > > I don't see Linos patches on mainline. Also, the series included four patches: > > [PATCH v3 0/4] Fixes for TPM interrupt handling > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler > > [PATCH v3 2/4] tpm: Simplify locality handling > > [PATCH v3 3/4] tpm: Fix test for interrupts > > [PATCH v3 4/4] tpm: Only enable supported irqs > > > > Three of them are relevant for the interrupt problem, which is still present in > > mainline, as these patches were refused: > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler > > [PATCH v3 2/4] tpm: Simplify locality handling > > [PATCH v3 3/4] tpm: Fix test for interrupts > > > > Michael > > > > You are right, the interrupts are still not working in the mainline kernel. > I would gladly make another attempt to fix this but rather step by step > than in a series that tries to fix (different) things at once. > > A first step could be to have a sleepable context for the interrupt handling, > since in case of SPI the accesses to the irq status register may sleep. > > I sent a patch for this purpose once, but it seems to have gone lost: > > https://lore.kernel.org/all/20210620023444.14684-1-LinoSanfilippo@gmx.de/ > > > Best regards, > Lino I went these through one by one. # Above linked patch Boolean parameters are considered bad. I.e. use named flags instead. This is for above linked patch. # [PATCH v3 3/4] tpm: Fix test for interrupts 1. Please remove "unnecessarily complicated" sentence because it cannot be evaluated. It's your opinion, which might perhaps be correct, but it is irrelevant for any possible patch description. 2. There's no such thing as "fix by re-implementation". Please explain instead code change is relevant for the bug fix. 3. If set_bit() et al necessarily to fix a possible race condition you need to have a separate patch for that. To move forward, start with a better summary such as "tpm: move interrupt test to tpm_tis_probe_irq_single()" I'd also either revert the change for flags, or alternatively move it to separate patch explaining race condition. Otherwise, there's no argument of saying that using set_bit() is more proper. This will make the change more localized. # [PATCH v3 2/4] tpm: Simplify locality handling "As a side-effect these modifications fix a bug which results in the following warning when using TPM 2:" Generally speaking, the simplifications should be done on top of code that does not have known bugs, even if the simplification renders out the bug. This is because then we have code that have potentially unknown unknown bugs. I hope you see my point. The problem with these patches were then and is still that they intermix bug fixes and other modifications and thus cannot be taken in. BR, Jarkko
On Wed, 2022-04-20 at 08:30 +0300, Jarkko Sakkinen wrote: > n Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote: > > > > Hi Michael, > > > > On 25.03.22 at 13:32, Michael Niewöhner wrote: > > > > > > > > > > Lino, I'd be happy to test the patches, when you have time and interest to > > > > > work > > > > > on this again! > > > > > > > > > > Thanks, Michael > > > > > > > > It's quite easy to test them out. Both fixes are in the mainline GIT tree. > > > > E.g. give a shot rc1, and please report if any issues persists to: > > > > > > > > linux-integrity@vger.kernel.org > > > > > > > > BR, Jarkko > > > > > > I don't see Linos patches on mainline. Also, the series included four patches: > > > [PATCH v3 0/4] Fixes for TPM interrupt handling > > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler > > > [PATCH v3 2/4] tpm: Simplify locality handling > > > [PATCH v3 3/4] tpm: Fix test for interrupts > > > [PATCH v3 4/4] tpm: Only enable supported irqs > > > > > > Three of them are relevant for the interrupt problem, which is still present in > > > mainline, as these patches were refused: > > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler > > > [PATCH v3 2/4] tpm: Simplify locality handling > > > [PATCH v3 3/4] tpm: Fix test for interrupts > > > > > > Michael > > > > > > > You are right, the interrupts are still not working in the mainline kernel. > > I would gladly make another attempt to fix this but rather step by step > > than in a series that tries to fix (different) things at once. > > > > A first step could be to have a sleepable context for the interrupt handling, > > since in case of SPI the accesses to the irq status register may sleep. > > > > I sent a patch for this purpose once, but it seems to have gone lost: > > > > https://lore.kernel.org/all/20210620023444.14684-1-LinoSanfilippo@gmx.de/ > > > > > > Best regards, > > Lino > > I went these through one by one. > > # Above linked patch > > Boolean parameters are considered bad. I.e. use named flags > instead. This is for above linked patch. > > # [PATCH v3 3/4] tpm: Fix test for interrupts > > 1. Please remove "unnecessarily complicated" sentence because > it cannot be evaluated. It's your opinion, which might perhaps > be correct, but it is irrelevant for any possible patch > description. > 2. There's no such thing as "fix by re-implementation". Please > explain instead code change is relevant for the bug fix. > 3. If set_bit() et al necessarily to fix a possible race condition > you need to have a separate patch for that. > > To move forward, start with a better summary such as > > "tpm: move interrupt test to tpm_tis_probe_irq_single()" > > I'd also either revert the change for flags, or alternatively > move it to separate patch explaining race condition. Otherwise, > there's no argument of saying that using set_bit() is more > proper. This will make the change more localized. > > > # [PATCH v3 2/4] tpm: Simplify locality handling > > "As a side-effect these modifications fix a bug which results in the > following warning when using TPM 2:" > > Generally speaking, the simplifications should be done on top of code > that does not have known bugs, even if the simplification renders out > the bug. This is because then we have code that have potentially unknown > unknown bugs. > > I hope you see my point. The problem with these patches were then > and is still that they intermix bug fixes and other modifications and > thus cannot be taken in. I.e. to move forward create first localized fixes, and only after those clean ups if there is point. Removing code (like in 2/4) is not a bug fix fo anything. This not to say that some changes would be illegit, I'm only saying that the patches are badly scoped. BR, Jarkko
Hi, On 20.04.22 at 07:30, Jarkko Sakkinen wrote: > n Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote: >> >> Hi Michael, >> >> On 25.03.22 at 13:32, Michael Niewöhner wrote: >>>>> >>>>> Lino, I'd be happy to test the patches, when you have time and interest to >>>>> work >>>>> on this again! >>>>> >>>>> Thanks, Michael >>>> >>>> It's quite easy to test them out. Both fixes are in the mainline GIT tree. >>>> E.g. give a shot rc1, and please report if any issues persists to: >>>> >>>> linux-integrity@vger.kernel.org >>>> >>>> BR, Jarkko >>> >>> I don't see Linos patches on mainline. Also, the series included four patches: >>> [PATCH v3 0/4] Fixes for TPM interrupt handling >>> [PATCH v3 1/4] tpm: Use a threaded interrupt handler >>> [PATCH v3 2/4] tpm: Simplify locality handling >>> [PATCH v3 3/4] tpm: Fix test for interrupts >>> [PATCH v3 4/4] tpm: Only enable supported irqs >>> >>> Three of them are relevant for the interrupt problem, which is still present in >>> mainline, as these patches were refused: >>> [PATCH v3 1/4] tpm: Use a threaded interrupt handler >>> [PATCH v3 2/4] tpm: Simplify locality handling >>> [PATCH v3 3/4] tpm: Fix test for interrupts >>> >>> Michael >>> >> >> You are right, the interrupts are still not working in the mainline kernel. >> I would gladly make another attempt to fix this but rather step by step >> than in a series that tries to fix (different) things at once. >> >> A first step could be to have a sleepable context for the interrupt handling, >> since in case of SPI the accesses to the irq status register may sleep. >> >> I sent a patch for this purpose once, but it seems to have gone lost: >> >> https://lore.kernel.org/all/20210620023444.14684-1-LinoSanfilippo@gmx.de/ >> >> >> Best regards, >> Lino > > I went these through one by one> > # Above linked patch > > Boolean parameters are considered bad. I.e. use named flags > instead. This is for above linked patch. Ok, we could extend tpm_tis_flags by a flag "TPM_TIS_USE_THREADED_IRQ" for this. > > # [PATCH v3 3/4] tpm: Fix test for interrupts > > 1. Please remove "unnecessarily complicated" sentence because > it cannot be evaluated. It's your opinion, which might perhaps > be correct, but it is irrelevant for any possible patch > description. > 2. There's no such thing as "fix by re-implementation". Please > explain instead code change is relevant for the bug fix. > 3. If set_bit() et al necessarily to fix a possible race condition > you need to have a separate patch for that. > > To move forward, start with a better summary such as > > "tpm: move interrupt test to tpm_tis_probe_irq_single()" > > I'd also either revert the change for flags, or alternatively > move it to separate patch explaining race condition. Otherwise, > there's no argument of saying that using set_bit() is more > proper. This will make the change more localized. > Ok, I will split the fix for the irq test into two patches then. > > # [PATCH v3 2/4] tpm: Simplify locality handling > > "As a side-effect these modifications fix a bug which results in the > following warning when using TPM 2:" > > Generally speaking, the simplifications should be done on top of code > that does not have known bugs, even if the simplification renders out > the bug. This is because then we have code that have potentially unknown > unknown bugs. > > I hope you see my point. The problem with these patches were then > and is still that they intermix bug fixes and other modifications and > thus cannot be taken in. > Yes, I can see that point. > BR, Jarkko > Thanks a lot for the review. I will prepare new patches with the suggested changes. Best regards, Lino
On Sun, 2022-04-24 at 04:22 +0200, Lino Sanfilippo wrote: > > Hi, > > On 20.04.22 at 07:30, Jarkko Sakkinen wrote: > > n Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote: > > > > > > Hi Michael, > > > > > > On 25.03.22 at 13:32, Michael Niewöhner wrote: > > > > > > > > > > > > Lino, I'd be happy to test the patches, when you have time and interest to > > > > > > work > > > > > > on this again! > > > > > > > > > > > > Thanks, Michael > > > > > > > > > > It's quite easy to test them out. Both fixes are in the mainline GIT tree. > > > > > E.g. give a shot rc1, and please report if any issues persists to: > > > > > > > > > > linux-integrity@vger.kernel.org > > > > > > > > > > BR, Jarkko > > > > > > > > I don't see Linos patches on mainline. Also, the series included four patches: > > > > [PATCH v3 0/4] Fixes for TPM interrupt handling > > > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler > > > > [PATCH v3 2/4] tpm: Simplify locality handling > > > > [PATCH v3 3/4] tpm: Fix test for interrupts > > > > [PATCH v3 4/4] tpm: Only enable supported irqs > > > > > > > > Three of them are relevant for the interrupt problem, which is still present in > > > > mainline, as these patches were refused: > > > > [PATCH v3 1/4] tpm: Use a threaded interrupt handler > > > > [PATCH v3 2/4] tpm: Simplify locality handling > > > > [PATCH v3 3/4] tpm: Fix test for interrupts > > > > > > > > Michael > > > > > > > > > > You are right, the interrupts are still not working in the mainline kernel. > > > I would gladly make another attempt to fix this but rather step by step > > > than in a series that tries to fix (different) things at once. > > > > > > A first step could be to have a sleepable context for the interrupt handling, > > > since in case of SPI the accesses to the irq status register may sleep. > > > > > > I sent a patch for this purpose once, but it seems to have gone lost: > > > > > > https://lore.kernel.org/all/20210620023444.14684-1-LinoSanfilippo@gmx.de/ > > > > > > > > > Best regards, > > > Lino > > > > I went these through one by one> > > # Above linked patch > > > > Boolean parameters are considered bad. I.e. use named flags > > instead. This is for above linked patch. > > Ok, we could extend tpm_tis_flags by a flag "TPM_TIS_USE_THREADED_IRQ" > for this. > > > > > # [PATCH v3 3/4] tpm: Fix test for interrupts > > > > 1. Please remove "unnecessarily complicated" sentence because > > it cannot be evaluated. It's your opinion, which might perhaps > > be correct, but it is irrelevant for any possible patch > > description. > > 2. There's no such thing as "fix by re-implementation". Please > > explain instead code change is relevant for the bug fix. > > 3. If set_bit() et al necessarily to fix a possible race condition > > you need to have a separate patch for that. > > > > To move forward, start with a better summary such as > > > > "tpm: move interrupt test to tpm_tis_probe_irq_single()" > > > > I'd also either revert the change for flags, or alternatively > > move it to separate patch explaining race condition. Otherwise, > > there's no argument of saying that using set_bit() is more > > proper. This will make the change more localized. > > > > Ok, I will split the fix for the irq test into two patches then. > > > > > # [PATCH v3 2/4] tpm: Simplify locality handling > > > > "As a side-effect these modifications fix a bug which results in the > > following warning when using TPM 2:" > > > > Generally speaking, the simplifications should be done on top of code > > that does not have known bugs, even if the simplification renders out > > the bug. This is because then we have code that have potentially unknown > > unknown bugs. > > > > I hope you see my point. The problem with these patches were then > > and is still that they intermix bug fixes and other modifications and > > thus cannot be taken in. > > > > Yes, I can see that point. > > > BR, Jarkko > > > > Thanks a lot for the review. I will prepare new patches with the suggested > changes. Yeah, I mean the point being: it's OK to suggest clean ups but with bug fixes you should aim for the lowest common denominator as far as you possibly can. BR, Jarkko
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index ddaeceb7e109..a09b6523261e 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -32,35 +32,6 @@ struct class *tpm_class; struct class *tpmrm_class; dev_t tpm_devt; -static int tpm_request_locality(struct tpm_chip *chip) -{ - int rc; - - if (!chip->ops->request_locality) - return 0; - - rc = chip->ops->request_locality(chip, 0); - if (rc < 0) - return rc; - - chip->locality = rc; - return 0; -} - -static void tpm_relinquish_locality(struct tpm_chip *chip) -{ - int rc; - - if (!chip->ops->relinquish_locality) - return; - - rc = chip->ops->relinquish_locality(chip, chip->locality); - if (rc) - dev_err(&chip->dev, "%s: : error %d\n", __func__, rc); - - chip->locality = -1; -} - static int tpm_cmd_ready(struct tpm_chip *chip) { if (!chip->ops->cmd_ready) @@ -103,17 +74,8 @@ int tpm_chip_start(struct tpm_chip *chip) tpm_clk_enable(chip); - if (chip->locality == -1) { - ret = tpm_request_locality(chip); - if (ret) { - tpm_clk_disable(chip); - return ret; - } - } - ret = tpm_cmd_ready(chip); if (ret) { - tpm_relinquish_locality(chip); tpm_clk_disable(chip); return ret; } @@ -133,7 +95,6 @@ EXPORT_SYMBOL_GPL(tpm_chip_start); void tpm_chip_stop(struct tpm_chip *chip) { tpm_go_idle(chip); - tpm_relinquish_locality(chip); tpm_clk_disable(chip); } EXPORT_SYMBOL_GPL(tpm_chip_stop); @@ -392,7 +353,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev, goto out; } - chip->locality = -1; return chip; out: diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index a12992ae2a3e..f892b1ae46f2 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -626,9 +626,6 @@ static int probe_itpm(struct tpm_chip *chip) if (vendor != TPM_VID_INTEL) return 0; - if (request_locality(chip, 0) != 0) - return -EBUSY; - rc = tpm_tis_send_data(chip, cmd_getticks, len); if (rc == 0) goto out; @@ -647,7 +644,6 @@ static int probe_itpm(struct tpm_chip *chip) out: tpm_tis_ready(chip); - release_locality(chip, priv->locality); return rc; } @@ -707,22 +703,13 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) const char *desc = "attempting to generate an interrupt"; u32 cap2; cap_t cap; - int ret; /* TPM 2.0 */ if (chip->flags & TPM_CHIP_FLAG_TPM2) return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc); /* TPM 1.2 */ - ret = request_locality(chip, 0); - if (ret < 0) - return ret; - - ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0); - - release_locality(chip, 0); - - return ret; + return tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0); } /* Register the IRQ and issue a command that will cause an interrupt. If an @@ -836,6 +823,7 @@ void tpm_tis_remove(struct tpm_chip *chip) tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt); + release_locality(chip, 0); tpm_tis_clkrun_enable(chip, false); if (priv->ilb_base_addr) @@ -963,6 +951,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, goto out_err; } + rc = request_locality(chip, 0); + if (rc) + goto out_err; + + rc = tpm_chip_start(chip); + if (rc) + goto out_err; + /* Take control of the TPM's interrupt hardware and shut it off */ rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask); if (rc < 0) @@ -973,9 +969,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, intmask &= ~TPM_GLOBAL_INT_ENABLE; tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); - rc = tpm_chip_start(chip); - if (rc) - goto out_err; rc = tpm2_probe(chip); tpm_chip_stop(chip); if (rc) @@ -1036,15 +1029,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, * to make sure it works. May as well use that command to set the * proper timeouts for the driver. */ - - rc = request_locality(chip, 0); - if (rc < 0) - goto out_err; - rc = tpm_get_timeouts(chip); - - release_locality(chip, 0); - if (rc) { dev_err(dev, "Could not get TPM timeouts and durations\n"); rc = -ENODEV; diff --git a/include/linux/tpm.h b/include/linux/tpm.h index aa11fe323c56..7a68832b14bb 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -167,9 +167,6 @@ struct tpm_chip { u32 last_cc; u32 nr_commands; u32 *cc_attrs_tbl; - - /* active locality */ - int locality; }; #define TPM_HEADER_SIZE 10
Currently the TPM (default) locality is claimed and released for each access to the TPM registers which require a claimed locality. This results in locality claim/release combos at various code places. For interrupt handling we also need such a combo in the interrupt handler (for clearing the interrupts) which makes the locality handling even more complicated since now we also have to synchronize concurrent accesses in process and in interrupt context. Since currently the driver uses only one locality anyway, avoid the increasing complexity by claiming it once at driver startup and only releasing it at driver shutdown. Due to the simplifications the functions tpm_request_locality() and tpm_relinquish_locality() are not needed any more an can be removed. As a side-effect these modifications fix a bug which results in the following warning when using TPM 2: TPM returned invalid status WARNING: CPU: 0 PID: 874 at drivers/char/tpm/tpm_tis_core.c:249 tpm_tis_status+0xbc/0xc8 [tpm_tis_core] Modules linked in: tpm_tis_spi tpm_tis_core tpm sha256_generic cfg80211 rfkill 8021q garp stp llc spidev v3d gpu_sched vc4 bcm2835_codec(C) cec raspberrypi_hwmon drm_kms_helper drm bcm2835_isp(C) v4l2_mem2mem bcm2835_v4l2(C) bcm2835_mmal_vchiq(C) videobuf2_vmalloc videobuf2_dma_contig snd_bcm2835(C) videobuf2_memops drm_panel_orientation_quirks videobuf2_v4l2 videobuf2_common snd_soc_core snd_compress videodev snd_pcm_dmaengine spi_bcm2835 snd_pcm mc vc_sm_cma(C) snd_timer snd syscopyarea rpivid_mem sysfillrect sysimgblt fb_sys_fops backlight uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: tpm] CPU: 0 PID: 874 Comm: kworker/u8:1 Tainted: G WC 5.11.0-rc2-LS-HOME+ #1 Hardware name: Raspberry Pi Compute Module 4 Rev 1.0 (DT) Workqueue: events_unbound async_run_entry_fn pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--) pc : tpm_tis_status+0xbc/0xc8 [tpm_tis_core] lr : tpm_tis_status+0xbc/0xc8 [tpm_tis_core] sp : ffffffc0127e38f0 x29: ffffffc0127e38f0 x28: ffffffc011238000 x27: 0000000000000016 x26: 000000000000017a x25: 0000000000000014 x24: ffffff8047f60000 x23: 0000000000000000 x22: 0000000000000016 x21: ffffff8044e8a480 x20: 0000000000000000 x19: ffffffc011238000 x18: ffffffc011238948 x17: 0000000000000000 x16: 0000000000000000 x15: ffffffc01141be81 x14: ffffffffffffffff x13: ffffffc01141be7e x12: ffffffffffffffff x11: ffffff807fb797f0 x10: 00000000000019b0 x9 : ffffffc0127e38f0 x8 : 766e692064656e72 x7 : 0000000000000000 x6 : ffffffc011239000 x5 : ffffff807fb628b8 x4 : 0000000000000000 x3 : 0000000000000027 x2 : 0000000000000000 x1 : 6818b6f22fdef800 x0 : 0000000000000000 Call trace: tpm_tis_status+0xbc/0xc8 [tpm_tis_core] tpm_tis_send_data+0x58/0x250 [tpm_tis_core] tpm_tis_send_main+0x50/0x128 [tpm_tis_core] tpm_tis_send+0x4c/0xe0 [tpm_tis_core] tpm_transmit+0xd0/0x350 [tpm] tpm_transmit_cmd+0x3c/0xc0 [tpm] tpm2_get_tpm_pt+0x124/0x1e8 [tpm] tpm_tis_probe_irq_single+0x198/0x364 [tpm_tis_core] tpm_tis_core_init+0x304/0x520 [tpm_tis_core] tpm_tis_spi_init+0x5c/0x78 [tpm_tis_spi] tpm_tis_spi_probe+0x80/0x98 [tpm_tis_spi] tpm_tis_spi_driver_probe+0x4c/0x60 [tpm_tis_spi] spi_probe+0x84/0xf0 really_probe+0x118/0x420 driver_probe_device+0x5c/0xc0 __driver_attach_async_helper+0x64/0x68 async_run_entry_fn+0x48/0x150 process_one_work+0x15c/0x4d0 worker_thread+0x50/0x490 kthread+0x118/0x150 ret_from_fork+0x10/0x1c The reason for this issue is that in case of TPM 2 function tpm2_get_timeouts() which executes a TPM command is called without a claimed locality. Since with this patch the locality is taken once at driver startup and never released before shutdown the issue does not occur any more. Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()") Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> --- drivers/char/tpm/tpm-chip.c | 40 --------------------------------- drivers/char/tpm/tpm_tis_core.c | 35 +++++++++-------------------- include/linux/tpm.h | 3 --- 3 files changed, 10 insertions(+), 68 deletions(-)