* [PATCH v2 0/3] char: tpm: Adjust cr50_i2c locking mechanism
@ 2022-11-03 14:54 Jan Dabros
2022-11-03 14:54 ` [PATCH v2 1/3] char: tpm: Protect tpm_pm_suspend with locks Jan Dabros
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Jan Dabros @ 2022-11-03 14:54 UTC (permalink / raw)
To: linux-integrity, jarkko, peterhuewe, jgg, gregkh, arnd
Cc: rrangel, timvp, apronin, mw, upstream, jsd
This patchset aims to add support for a platforms with cr50(tpm) i2c
chip shared across two CPUs. We need to provide a mechanism, which will
allow to synchronize accesses on a TPM-transaction boundaries.
First commit in a patchset is a generic fix for an issue encountered
during suspend stress test.
Next two patches are modifying cr50 to use generic callbacks for
locality management and then leverage this solution to apply i2c bus
locking on a TPM-operation level.
v1->v2:
* Get back with tpm_cr50_check_locality to return 0 instead of locality
* Return from functions which get (locality < 0)
Jan Dabros (3):
char: tpm: Protect tpm_pm_suspend with locks
char: tpm: cr50: Use generic request/relinquish locality ops
char: tpm: cr50: Move i2c locking to request/relinquish locality ops
drivers/char/tpm/tpm-interface.c | 5 +-
drivers/char/tpm/tpm_tis_i2c_cr50.c | 126 ++++++++++++++++++----------
2 files changed, 85 insertions(+), 46 deletions(-)
--
2.38.1.273.g43a17bfeac-goog
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH v2 1/3] char: tpm: Protect tpm_pm_suspend with locks 2022-11-03 14:54 [PATCH v2 0/3] char: tpm: Adjust cr50_i2c locking mechanism Jan Dabros @ 2022-11-03 14:54 ` Jan Dabros 2022-11-06 19:48 ` Jarkko Sakkinen 2022-11-03 14:54 ` [PATCH v2 2/3] char: tpm: cr50: Use generic request/relinquish locality ops Jan Dabros 2022-11-03 14:54 ` [PATCH v2 3/3] char: tpm: cr50: Move i2c locking to " Jan Dabros 2 siblings, 1 reply; 18+ messages in thread From: Jan Dabros @ 2022-11-03 14:54 UTC (permalink / raw) To: linux-integrity, jarkko, peterhuewe, jgg, gregkh, arnd Cc: rrangel, timvp, apronin, mw, upstream, jsd Currently tpm transactions are executed unconditionally in tpm_pm_suspend() function, what may lead to races with other tpm accessors in the system. Add proper locking mechanisms by calling tpm_try_get_ops() which is a wrapper on tpm_chip_start(). Signed-off-by: Jan Dabros <jsd@semihalf.com> --- drivers/char/tpm/tpm-interface.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 1621ce8187052..d69905233aff2 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -401,13 +401,14 @@ int tpm_pm_suspend(struct device *dev) !pm_suspend_via_firmware()) goto suspended; - if (!tpm_chip_start(chip)) { + rc = tpm_try_get_ops(chip); + if (!rc) { if (chip->flags & TPM_CHIP_FLAG_TPM2) tpm2_shutdown(chip, TPM2_SU_STATE); else rc = tpm1_pm_suspend(chip, tpm_suspend_pcr); - tpm_chip_stop(chip); + tpm_put_ops(chip); } suspended: -- 2.38.1.273.g43a17bfeac-goog ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] char: tpm: Protect tpm_pm_suspend with locks 2022-11-03 14:54 ` [PATCH v2 1/3] char: tpm: Protect tpm_pm_suspend with locks Jan Dabros @ 2022-11-06 19:48 ` Jarkko Sakkinen 2022-11-07 8:45 ` Jan Dąbroś 0 siblings, 1 reply; 18+ messages in thread From: Jarkko Sakkinen @ 2022-11-06 19:48 UTC (permalink / raw) To: Jan Dabros Cc: linux-integrity, peterhuewe, jgg, gregkh, arnd, rrangel, timvp, apronin, mw, upstream On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote: > Currently tpm transactions are executed unconditionally in > tpm_pm_suspend() function, what may lead to races with other tpm > accessors in the system. > > Add proper locking mechanisms by calling tpm_try_get_ops() which is a > wrapper on tpm_chip_start(). > > Signed-off-by: Jan Dabros <jsd@semihalf.com> AFAIK processes are freezed before suspend callbacks are called, and the callbacks are called sequentially. I have no idea what is meant by "TPM accessor" here. Please describe the concurrency scenario in the commit message where the race could happen, if it is hard to reproduce, and add an appropriate fixes tag. BR, Jarkko ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] char: tpm: Protect tpm_pm_suspend with locks 2022-11-06 19:48 ` Jarkko Sakkinen @ 2022-11-07 8:45 ` Jan Dąbroś 2022-11-07 16:35 ` Jarkko Sakkinen 0 siblings, 1 reply; 18+ messages in thread From: Jan Dąbroś @ 2022-11-07 8:45 UTC (permalink / raw) To: Jarkko Sakkinen Cc: linux-integrity, peterhuewe, jgg, gregkh, arnd, rrangel, timvp, apronin, mw, upstream niedz., 6 lis 2022 o 20:49 Jarkko Sakkinen <jarkko@kernel.org> napisał(a): > > On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote: > > Currently tpm transactions are executed unconditionally in > > tpm_pm_suspend() function, what may lead to races with other tpm > > accessors in the system. > > > > Add proper locking mechanisms by calling tpm_try_get_ops() which is a > > wrapper on tpm_chip_start(). > > > > Signed-off-by: Jan Dabros <jsd@semihalf.com> > > AFAIK processes are freezed before suspend callbacks are called, and > the callbacks are called sequentially. I have no idea what is meant > by "TPM accessor" here. User space processes are freezed before suspend, but kernel threads are not freezable by default. In my particular case it was a hwrng thread started from drivers/char/hw_random/core.c - I was referring to it as "TPM accessor". For sure I should be more precise in a commit msg. > Please describe the concurrency scenario in the commit message where the > race could happen, if it is hard to reproduce, and add an appropriate fixes > tag. I will describe my scenario in more detail in the next version. Regarding the "fixes" tag - I'm not too familiar with it, but looking at the kernel submission guide, "fixes" should be used either when there was a particular commit in the past which introduced the bug or if a patch fixes an already logged bug entry (so that one can paste URL). In my case both are not applicable, so please advise what exactly I should put after this tag? Best Regards, Jan ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] char: tpm: Protect tpm_pm_suspend with locks 2022-11-07 8:45 ` Jan Dąbroś @ 2022-11-07 16:35 ` Jarkko Sakkinen 2022-11-28 17:04 ` Jason A. Donenfeld 0 siblings, 1 reply; 18+ messages in thread From: Jarkko Sakkinen @ 2022-11-07 16:35 UTC (permalink / raw) To: Jan Dąbroś Cc: linux-integrity, peterhuewe, jgg, gregkh, arnd, rrangel, timvp, apronin, mw, upstream On Mon, Nov 07, 2022 at 09:45:41AM +0100, Jan Dąbroś wrote: > niedz., 6 lis 2022 o 20:49 Jarkko Sakkinen <jarkko@kernel.org> napisał(a): > > > > On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote: > > > Currently tpm transactions are executed unconditionally in > > > tpm_pm_suspend() function, what may lead to races with other tpm > > > accessors in the system. > > > > > > Add proper locking mechanisms by calling tpm_try_get_ops() which is a > > > wrapper on tpm_chip_start(). > > > > > > Signed-off-by: Jan Dabros <jsd@semihalf.com> > > > > AFAIK processes are freezed before suspend callbacks are called, and > > the callbacks are called sequentially. I have no idea what is meant > > by "TPM accessor" here. > > User space processes are freezed before suspend, but kernel threads > are not freezable by default. In my particular case it was a hwrng > thread started from drivers/char/hw_random/core.c - I was referring to > it as "TPM accessor". For sure I should be more precise in a commit > msg. OK, great. > > > Please describe the concurrency scenario in the commit message where the > > race could happen, if it is hard to reproduce, and add an appropriate fixes > > tag. > > I will describe my scenario in more detail in the next version. > Regarding the "fixes" tag - I'm not too familiar with it, but looking > at the kernel submission guide, "fixes" should be used either when > there was a particular commit in the past which introduced the bug or > if a patch fixes an already logged bug entry (so that one can paste > URL). In my case both are not applicable, so please advise what > exactly I should put after this tag? It is Fixed: <12 character prefix of the hash> ("short summary") It should point out to the commit, which introduced the issue/bug. > Best Regards, > Jan BR, Jarkko ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] char: tpm: Protect tpm_pm_suspend with locks 2022-11-07 16:35 ` Jarkko Sakkinen @ 2022-11-28 17:04 ` Jason A. Donenfeld 2022-11-28 17:07 ` Jason A. Donenfeld 2022-11-28 17:11 ` Vlastimil Babka 0 siblings, 2 replies; 18+ messages in thread From: Jason A. Donenfeld @ 2022-11-28 17:04 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Jan Dąbroś, linux-integrity, peterhuewe, jgg, gregkh, arnd, rrangel, timvp, apronin, mw, upstream, Vlastimil Babka Hi Jarkko, On Mon, Nov 07, 2022 at 06:35:25PM +0200, Jarkko Sakkinen wrote: > On Mon, Nov 07, 2022 at 09:45:41AM +0100, Jan Dąbroś wrote: > > niedz., 6 lis 2022 o 20:49 Jarkko Sakkinen <jarkko@kernel.org> napisał(a): > > > > > > On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote: > > > > Currently tpm transactions are executed unconditionally in > > > > tpm_pm_suspend() function, what may lead to races with other tpm > > > > accessors in the system. > > > > > > > > Add proper locking mechanisms by calling tpm_try_get_ops() which is a > > > > wrapper on tpm_chip_start(). > > > > > > > > Signed-off-by: Jan Dabros <jsd@semihalf.com> > > > > > > AFAIK processes are freezed before suspend callbacks are called, and > > > the callbacks are called sequentially. I have no idea what is meant > > > by "TPM accessor" here. > > > > User space processes are freezed before suspend, but kernel threads > > are not freezable by default. In my particular case it was a hwrng > > thread started from drivers/char/hw_random/core.c - I was referring to > > it as "TPM accessor". For sure I should be more precise in a commit > > msg. > > OK, great. > > > > > > Please describe the concurrency scenario in the commit message where the > > > race could happen, if it is hard to reproduce, and add an appropriate fixes > > > tag. > > > > I will describe my scenario in more detail in the next version. > > Regarding the "fixes" tag - I'm not too familiar with it, but looking > > at the kernel submission guide, "fixes" should be used either when > > there was a particular commit in the past which introduced the bug or > > if a patch fixes an already logged bug entry (so that one can paste > > URL). In my case both are not applicable, so please advise what > > exactly I should put after this tag? > > It is > > Fixed: <12 character prefix of the hash> ("short summary") > > It should point out to the commit, which introduced the issue/bug. Somebody just reported a bug to me in hwrng stuff, which I traced down to this lack of tpm_try_get_ops(), and then I coded up the exact same patch as Jan's here, saw it fixed the problem, and then realized it was already submitted in that exact form. Because of some recent hwrng changes, this bug will be triggered more often than it was before. So, even though Jan hasn't submitted a v+1, do you think you could take this 1/3 commit, fix up the commit message or whatever you need there, and get this in for 6.1-rc8? It's an important fix. Thanks, Jason ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] char: tpm: Protect tpm_pm_suspend with locks 2022-11-28 17:04 ` Jason A. Donenfeld @ 2022-11-28 17:07 ` Jason A. Donenfeld 2022-11-28 19:46 ` Vlastimil Babka 2022-11-28 17:11 ` Vlastimil Babka 1 sibling, 1 reply; 18+ messages in thread From: Jason A. Donenfeld @ 2022-11-28 17:07 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Jan Dąbroś, linux-integrity, peterhuewe, jgg, gregkh, arnd, rrangel, timvp, apronin, mw, upstream, Vlastimil Babka On Mon, Nov 28, 2022 at 6:04 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Hi Jarkko, > > On Mon, Nov 07, 2022 at 06:35:25PM +0200, Jarkko Sakkinen wrote: > > On Mon, Nov 07, 2022 at 09:45:41AM +0100, Jan Dąbroś wrote: > > > niedz., 6 lis 2022 o 20:49 Jarkko Sakkinen <jarkko@kernel.org> napisał(a): > > > > > > > > On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote: > > > > > Currently tpm transactions are executed unconditionally in > > > > > tpm_pm_suspend() function, what may lead to races with other tpm > > > > > accessors in the system. > > > > > > > > > > Add proper locking mechanisms by calling tpm_try_get_ops() which is a > > > > > wrapper on tpm_chip_start(). > > > > > > > > > > Signed-off-by: Jan Dabros <jsd@semihalf.com> > > > > > > > > AFAIK processes are freezed before suspend callbacks are called, and > > > > the callbacks are called sequentially. I have no idea what is meant > > > > by "TPM accessor" here. > > > > > > User space processes are freezed before suspend, but kernel threads > > > are not freezable by default. In my particular case it was a hwrng > > > thread started from drivers/char/hw_random/core.c - I was referring to > > > it as "TPM accessor". For sure I should be more precise in a commit > > > msg. > > > > OK, great. > > > > > > > > > Please describe the concurrency scenario in the commit message where the > > > > race could happen, if it is hard to reproduce, and add an appropriate fixes > > > > tag. > > > > > > I will describe my scenario in more detail in the next version. > > > Regarding the "fixes" tag - I'm not too familiar with it, but looking > > > at the kernel submission guide, "fixes" should be used either when > > > there was a particular commit in the past which introduced the bug or > > > if a patch fixes an already logged bug entry (so that one can paste > > > URL). In my case both are not applicable, so please advise what > > > exactly I should put after this tag? > > > > It is > > > > Fixed: <12 character prefix of the hash> ("short summary") > > > > It should point out to the commit, which introduced the issue/bug. > > Somebody just reported a bug to me in hwrng stuff, which I traced down > to this lack of tpm_try_get_ops(), and then I coded up the exact same > patch as Jan's here, saw it fixed the problem, and then realized it was > already submitted in that exact form. > > Because of some recent hwrng changes, this bug will be triggered more > often than it was before. > > So, even though Jan hasn't submitted a v+1, do you think you could take > this 1/3 commit, fix up the commit message or whatever you need there, > and get this in for 6.1-rc8? It's an important fix. > > Thanks, > Jason And if it helps, here's some more metadata: Tested-by: Jason A. Donenfeld <Jason@zx2c4.com> Reported-by: Vlastimil Babka <vbabka@suse.cz> Link: https://lore.kernel.org/all/c5ba47ef-393f-1fba-30bd-1230d1b4b592@suse.cz/ Cc: stable@vger.kernel.org ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] char: tpm: Protect tpm_pm_suspend with locks 2022-11-28 17:07 ` Jason A. Donenfeld @ 2022-11-28 19:46 ` Vlastimil Babka 2022-11-28 19:55 ` Jason A. Donenfeld 0 siblings, 1 reply; 18+ messages in thread From: Vlastimil Babka @ 2022-11-28 19:46 UTC (permalink / raw) To: Jason A. Donenfeld, Jarkko Sakkinen Cc: Jan Dąbroś, linux-integrity, peterhuewe, jgg, gregkh, arnd, rrangel, timvp, apronin, mw, upstream On 11/28/22 18:07, Jason A. Donenfeld wrote: > On Mon, Nov 28, 2022 at 6:04 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: >> > > >> > > > Please describe the concurrency scenario in the commit message where the >> > > > race could happen, if it is hard to reproduce, and add an appropriate fixes >> > > > tag. >> > > >> > > I will describe my scenario in more detail in the next version. >> > > Regarding the "fixes" tag - I'm not too familiar with it, but looking >> > > at the kernel submission guide, "fixes" should be used either when >> > > there was a particular commit in the past which introduced the bug or >> > > if a patch fixes an already logged bug entry (so that one can paste >> > > URL). In my case both are not applicable, so please advise what >> > > exactly I should put after this tag? >> > >> > It is >> > >> > Fixed: <12 character prefix of the hash> ("short summary") >> > >> > It should point out to the commit, which introduced the issue/bug. >> >> Somebody just reported a bug to me in hwrng stuff, which I traced down >> to this lack of tpm_try_get_ops(), and then I coded up the exact same >> patch as Jan's here, saw it fixed the problem, and then realized it was >> already submitted in that exact form. >> >> Because of some recent hwrng changes, this bug will be triggered more >> often than it was before. >> >> So, even though Jan hasn't submitted a v+1, do you think you could take >> this 1/3 commit, fix up the commit message or whatever you need there, >> and get this in for 6.1-rc8? It's an important fix. >> >> Thanks, >> Jason > > And if it helps, here's some more metadata: > > Tested-by: Jason A. Donenfeld <Jason@zx2c4.com> > Reported-by: Vlastimil Babka <vbabka@suse.cz> You can make that: Reported-and-tested-by: Vlastimil Babka <vbabka@suse.cz> > Link: https://lore.kernel.org/all/c5ba47ef-393f-1fba-30bd-1230d1b4b592@suse.cz/ > Cc: stable@vger.kernel.org Not an expert here but looks like a valid Fixes: would be Fixes: e891db1a18bf ("tpm: turn on TPM on suspend for TPM 1.x") The 6.1-rc1 commit 3b006c439d58d ("hwrng: core - start hwrng kthread also for untrusted sources") likely exposed the bug in my case, but it wouldn't be the real root cause. But it makes it prudent to get the fix to 6.1. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] char: tpm: Protect tpm_pm_suspend with locks 2022-11-28 19:46 ` Vlastimil Babka @ 2022-11-28 19:55 ` Jason A. Donenfeld 0 siblings, 0 replies; 18+ messages in thread From: Jason A. Donenfeld @ 2022-11-28 19:55 UTC (permalink / raw) To: Vlastimil Babka Cc: Jarkko Sakkinen, Jan Dąbroś, linux-integrity, peterhuewe, jgg, gregkh, arnd, rrangel, timvp, apronin, mw, upstream On Mon, Nov 28, 2022 at 8:46 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 11/28/22 18:07, Jason A. Donenfeld wrote: > > On Mon, Nov 28, 2022 at 6:04 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > >> > > > >> > > > Please describe the concurrency scenario in the commit message where the > >> > > > race could happen, if it is hard to reproduce, and add an appropriate fixes > >> > > > tag. > >> > > > >> > > I will describe my scenario in more detail in the next version. > >> > > Regarding the "fixes" tag - I'm not too familiar with it, but looking > >> > > at the kernel submission guide, "fixes" should be used either when > >> > > there was a particular commit in the past which introduced the bug or > >> > > if a patch fixes an already logged bug entry (so that one can paste > >> > > URL). In my case both are not applicable, so please advise what > >> > > exactly I should put after this tag? > >> > > >> > It is > >> > > >> > Fixed: <12 character prefix of the hash> ("short summary") > >> > > >> > It should point out to the commit, which introduced the issue/bug. > >> > >> Somebody just reported a bug to me in hwrng stuff, which I traced down > >> to this lack of tpm_try_get_ops(), and then I coded up the exact same > >> patch as Jan's here, saw it fixed the problem, and then realized it was > >> already submitted in that exact form. > >> > >> Because of some recent hwrng changes, this bug will be triggered more > >> often than it was before. > >> > >> So, even though Jan hasn't submitted a v+1, do you think you could take > >> this 1/3 commit, fix up the commit message or whatever you need there, > >> and get this in for 6.1-rc8? It's an important fix. > >> > >> Thanks, > >> Jason > > > > And if it helps, here's some more metadata: > > > > Tested-by: Jason A. Donenfeld <Jason@zx2c4.com> > > Reported-by: Vlastimil Babka <vbabka@suse.cz> > > You can make that: > Reported-and-tested-by: Vlastimil Babka <vbabka@suse.cz> > > > Link: https://lore.kernel.org/all/c5ba47ef-393f-1fba-30bd-1230d1b4b592@suse.cz/ > > Cc: stable@vger.kernel.org > > Not an expert here but looks like a valid Fixes: would be > Fixes: e891db1a18bf ("tpm: turn on TPM on suspend for TPM 1.x") > > The 6.1-rc1 commit 3b006c439d58d ("hwrng: core - start hwrng kthread also > for untrusted sources") likely exposed the bug in my case, but it wouldn't > be the real root cause. But it makes it prudent to get the fix to 6.1. Since this thread went dormant, and we're already pretty close to the end of the cycle, I'm going to resend this with the commit message fixed, as Jarkko asked at the beginning of the month. Jan can then send whatever needs fixing in 2/3 and 3/3 whenever he gets around to it. I'll keep the original authorship intact of course. Jason ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] char: tpm: Protect tpm_pm_suspend with locks 2022-11-28 17:04 ` Jason A. Donenfeld 2022-11-28 17:07 ` Jason A. Donenfeld @ 2022-11-28 17:11 ` Vlastimil Babka 1 sibling, 0 replies; 18+ messages in thread From: Vlastimil Babka @ 2022-11-28 17:11 UTC (permalink / raw) To: Jason A. Donenfeld, Jarkko Sakkinen Cc: Jan Dąbroś, linux-integrity, peterhuewe, jgg, gregkh, arnd, rrangel, timvp, apronin, mw, upstream On 11/28/22 18:04, Jason A. Donenfeld wrote: > Hi Jarkko, > > On Mon, Nov 07, 2022 at 06:35:25PM +0200, Jarkko Sakkinen wrote: >> On Mon, Nov 07, 2022 at 09:45:41AM +0100, Jan Dąbroś wrote: >> > niedz., 6 lis 2022 o 20:49 Jarkko Sakkinen <jarkko@kernel.org> napisał(a): >> > > >> > > On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote: >> > > > Currently tpm transactions are executed unconditionally in >> > > > tpm_pm_suspend() function, what may lead to races with other tpm >> > > > accessors in the system. >> > > > >> > > > Add proper locking mechanisms by calling tpm_try_get_ops() which is a >> > > > wrapper on tpm_chip_start(). >> > > > >> > > > Signed-off-by: Jan Dabros <jsd@semihalf.com> >> > > >> > > AFAIK processes are freezed before suspend callbacks are called, and >> > > the callbacks are called sequentially. I have no idea what is meant >> > > by "TPM accessor" here. >> > >> > User space processes are freezed before suspend, but kernel threads >> > are not freezable by default. In my particular case it was a hwrng >> > thread started from drivers/char/hw_random/core.c - I was referring to >> > it as "TPM accessor". For sure I should be more precise in a commit >> > msg. >> >> OK, great. >> >> > >> > > Please describe the concurrency scenario in the commit message where the >> > > race could happen, if it is hard to reproduce, and add an appropriate fixes >> > > tag. >> > >> > I will describe my scenario in more detail in the next version. >> > Regarding the "fixes" tag - I'm not too familiar with it, but looking >> > at the kernel submission guide, "fixes" should be used either when >> > there was a particular commit in the past which introduced the bug or >> > if a patch fixes an already logged bug entry (so that one can paste >> > URL). In my case both are not applicable, so please advise what >> > exactly I should put after this tag? >> >> It is >> >> Fixed: <12 character prefix of the hash> ("short summary") >> >> It should point out to the commit, which introduced the issue/bug. > > Somebody just reported a bug to me in hwrng stuff, which I traced down > to this lack of tpm_try_get_ops(), and then I coded up the exact same > patch as Jan's here, saw it fixed the problem, and then realized it was > already submitted in that exact form. > > Because of some recent hwrng changes, this bug will be triggered more > often than it was before. > > So, even though Jan hasn't submitted a v+1, do you think you could take > this 1/3 commit, fix up the commit message or whatever you need there, > and get this in for 6.1-rc8? It's an important fix. For the record, the bug report is here: https://lore.kernel.org/all/c5ba47ef-393f-1fba-30bd-1230d1b4b592@suse.cz/ (patch author Jan and TPM maintainers were Cc'd) FWIW I tested the patch 1/3 on top of rc7 and didn't reproduce the issue. But also had no luck of reproducing it on plain rc7 yet, so it doesn't say much, unfortunately. > Thanks, > Jason ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/3] char: tpm: cr50: Use generic request/relinquish locality ops 2022-11-03 14:54 [PATCH v2 0/3] char: tpm: Adjust cr50_i2c locking mechanism Jan Dabros 2022-11-03 14:54 ` [PATCH v2 1/3] char: tpm: Protect tpm_pm_suspend with locks Jan Dabros @ 2022-11-03 14:54 ` Jan Dabros 2022-11-07 9:20 ` Jarkko Sakkinen 2022-11-18 14:09 ` Guenter Roeck 2022-11-03 14:54 ` [PATCH v2 3/3] char: tpm: cr50: Move i2c locking to " Jan Dabros 2 siblings, 2 replies; 18+ messages in thread From: Jan Dabros @ 2022-11-03 14:54 UTC (permalink / raw) To: linux-integrity, jarkko, peterhuewe, jgg, gregkh, arnd Cc: rrangel, timvp, apronin, mw, upstream, jsd Instead of using static functions tpm_cr50_request_locality and tpm_cr50_release_locality register callbacks from tpm class chip->ops created for this purpose. Signed-off-by: Jan Dabros <jsd@semihalf.com> --- drivers/char/tpm/tpm_tis_i2c_cr50.c | 106 ++++++++++++++++++---------- 1 file changed, 70 insertions(+), 36 deletions(-) diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c index 77cea5b31c6e4..517d8410d7da0 100644 --- a/drivers/char/tpm/tpm_tis_i2c_cr50.c +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c @@ -17,6 +17,7 @@ */ #include <linux/acpi.h> +#include <linux/bug.h> #include <linux/completion.h> #include <linux/i2c.h> #include <linux/interrupt.h> @@ -35,6 +36,7 @@ #define TPM_CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C errors */ #define TPM_CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs between retries on I2C */ #define TPM_CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs between retries on I2C */ +#define TPM_CR50_I2C_DEFAULT_LOC 0 #define TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4)) #define TPM_I2C_STS(l) (0x0001 | ((l) << 4)) @@ -286,20 +288,21 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer, } /** - * tpm_cr50_check_locality() - Verify TPM locality 0 is active. + * tpm_cr50_check_locality() - Verify if required TPM locality is active. * @chip: A TPM chip. + * @loc: Locality to be verified * * Return: * - 0: Success. * - -errno: A POSIX error code. */ -static int tpm_cr50_check_locality(struct tpm_chip *chip) +static int tpm_cr50_check_locality(struct tpm_chip *chip, int loc) { u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY; u8 buf; int rc; - rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf)); + rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf)); if (rc < 0) return rc; @@ -312,48 +315,57 @@ static int tpm_cr50_check_locality(struct tpm_chip *chip) /** * tpm_cr50_release_locality() - Release TPM locality. * @chip: A TPM chip. - * @force: Flag to force release if set. + * @loc: Locality to be released + * + * Return: + * - 0: Success. + * - -errno: A POSIX error code. */ -static void tpm_cr50_release_locality(struct tpm_chip *chip, bool force) +static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc) { u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING; - u8 addr = TPM_I2C_ACCESS(0); + u8 addr = TPM_I2C_ACCESS(loc); u8 buf; + int rc; - if (tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf)) < 0) - return; + rc = tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf)); + if (rc < 0) + return rc; - if (force || (buf & mask) == mask) { + if ((buf & mask) == mask) { buf = TPM_ACCESS_ACTIVE_LOCALITY; - tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf)); + rc = tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf)); } + + return rc; } /** - * tpm_cr50_request_locality() - Request TPM locality 0. + * tpm_cr50_request_locality() - Request TPM locality. * @chip: A TPM chip. + * @loc: Locality to be requested. * * Return: - * - 0: Success. + * - loc: Success. * - -errno: A POSIX error code. */ -static int tpm_cr50_request_locality(struct tpm_chip *chip) +static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc) { u8 buf = TPM_ACCESS_REQUEST_USE; unsigned long stop; int rc; - if (!tpm_cr50_check_locality(chip)) - return 0; + if (!tpm_cr50_check_locality(chip, loc)) + return loc; - rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf)); + rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf)); if (rc < 0) return rc; stop = jiffies + chip->timeout_a; do { - if (!tpm_cr50_check_locality(chip)) - return 0; + if (!tpm_cr50_check_locality(chip, loc)) + return loc; msleep(TPM_CR50_TIMEOUT_SHORT_MS); } while (time_before(jiffies, stop)); @@ -374,7 +386,12 @@ static u8 tpm_cr50_i2c_tis_status(struct tpm_chip *chip) { u8 buf[4]; - if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(0), buf, sizeof(buf)) < 0) + if (chip->locality < 0){ + WARN_ONCE(1, "Incorrect tpm locality value\n"); + return 0; + } + + if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf)) < 0) return 0; return buf[0]; @@ -390,7 +407,12 @@ static void tpm_cr50_i2c_tis_set_ready(struct tpm_chip *chip) { u8 buf[4] = { TPM_STS_COMMAND_READY }; - tpm_cr50_i2c_write(chip, TPM_I2C_STS(0), buf, sizeof(buf)); + if (chip->locality < 0) { + WARN_ONCE(1, "Incorrect tpm locality value\n"); + return; + } + + tpm_cr50_i2c_write(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf)); msleep(TPM_CR50_TIMEOUT_SHORT_MS); } @@ -420,7 +442,7 @@ static int tpm_cr50_i2c_get_burst_and_status(struct tpm_chip *chip, u8 mask, stop = jiffies + chip->timeout_b; do { - if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(0), buf, sizeof(buf)) < 0) { + if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf)) < 0) { msleep(TPM_CR50_TIMEOUT_SHORT_MS); continue; } @@ -454,10 +476,15 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len) u8 mask = TPM_STS_VALID | TPM_STS_DATA_AVAIL; size_t burstcnt, cur, len, expected; - u8 addr = TPM_I2C_DATA_FIFO(0); + u8 addr = TPM_I2C_DATA_FIFO(chip->locality); u32 status; int rc; + if (chip->locality < 0) { + WARN_ONCE(1, "Incorrect tpm locality value\n"); + return -EINVAL; + } + if (buf_len < TPM_HEADER_SIZE) return -EINVAL; @@ -516,7 +543,6 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len) goto out_err; } - tpm_cr50_release_locality(chip, false); return cur; out_err: @@ -524,7 +550,6 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len) if (tpm_cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY) tpm_cr50_i2c_tis_set_ready(chip); - tpm_cr50_release_locality(chip, false); return rc; } @@ -546,9 +571,10 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) u32 status; int rc; - rc = tpm_cr50_request_locality(chip); - if (rc < 0) - return rc; + if (chip->locality < 0) { + WARN_ONCE(1, "Incorrect tpm locality value\n"); + return -EINVAL; + } /* Wait until TPM is ready for a command */ stop = jiffies + chip->timeout_b; @@ -578,7 +604,8 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) * that is inserted by tpm_cr50_i2c_write() */ limit = min_t(size_t, burstcnt - 1, len); - rc = tpm_cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(0), &buf[sent], limit); + rc = tpm_cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(chip->locality), + &buf[sent], limit); if (rc < 0) { dev_err(&chip->dev, "Write failed\n"); goto out_err; @@ -599,7 +626,7 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) } /* Start the TPM command */ - rc = tpm_cr50_i2c_write(chip, TPM_I2C_STS(0), tpm_go, + rc = tpm_cr50_i2c_write(chip, TPM_I2C_STS(chip->locality), tpm_go, sizeof(tpm_go)); if (rc < 0) { dev_err(&chip->dev, "Start command failed\n"); @@ -612,7 +639,6 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) if (tpm_cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY) tpm_cr50_i2c_tis_set_ready(chip); - tpm_cr50_release_locality(chip, false); return rc; } @@ -651,6 +677,8 @@ static const struct tpm_class_ops cr50_i2c = { .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID, .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID, .req_canceled = &tpm_cr50_i2c_req_canceled, + .request_locality = &tpm_cr50_request_locality, + .relinquish_locality = &tpm_cr50_release_locality, }; #ifdef CONFIG_ACPI @@ -686,6 +714,7 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client) u32 vendor; u8 buf[4]; int rc; + int loc; if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) return -ENODEV; @@ -728,24 +757,30 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client) TPM_CR50_TIMEOUT_NOIRQ_MS); } - rc = tpm_cr50_request_locality(chip); - if (rc < 0) { + loc = tpm_cr50_request_locality(chip, TPM_CR50_I2C_DEFAULT_LOC); + if (loc < 0) { dev_err(dev, "Could not request locality\n"); return rc; } /* Read four bytes from DID_VID register */ - rc = tpm_cr50_i2c_read(chip, TPM_I2C_DID_VID(0), buf, sizeof(buf)); + rc = tpm_cr50_i2c_read(chip, TPM_I2C_DID_VID(loc), buf, sizeof(buf)); if (rc < 0) { dev_err(dev, "Could not read vendor id\n"); - tpm_cr50_release_locality(chip, true); + if (tpm_cr50_release_locality(chip, loc)) + dev_err(dev, "Could not release locality\n"); + return rc; + } + + rc = tpm_cr50_release_locality(chip, loc); + if (rc) { + dev_err(dev, "Could not release locality\n"); return rc; } vendor = le32_to_cpup((__le32 *)buf); if (vendor != TPM_CR50_I2C_DID_VID && vendor != TPM_TI50_I2C_DID_VID) { dev_err(dev, "Vendor ID did not match! ID was %08x\n", vendor); - tpm_cr50_release_locality(chip, true); return -ENODEV; } @@ -774,7 +809,6 @@ static void tpm_cr50_i2c_remove(struct i2c_client *client) } tpm_chip_unregister(chip); - tpm_cr50_release_locality(chip, true); } static SIMPLE_DEV_PM_OPS(cr50_i2c_pm, tpm_pm_suspend, tpm_pm_resume); -- 2.38.1.273.g43a17bfeac-goog ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] char: tpm: cr50: Use generic request/relinquish locality ops 2022-11-03 14:54 ` [PATCH v2 2/3] char: tpm: cr50: Use generic request/relinquish locality ops Jan Dabros @ 2022-11-07 9:20 ` Jarkko Sakkinen 2022-11-07 9:41 ` Jan Dąbroś 2022-11-18 14:09 ` Guenter Roeck 1 sibling, 1 reply; 18+ messages in thread From: Jarkko Sakkinen @ 2022-11-07 9:20 UTC (permalink / raw) To: Jan Dabros Cc: linux-integrity, peterhuewe, jgg, gregkh, arnd, rrangel, timvp, apronin, mw, upstream On Thu, Nov 03, 2022 at 03:54:49PM +0100, Jan Dabros wrote: > Instead of using static functions tpm_cr50_request_locality and > tpm_cr50_release_locality register callbacks from tpm class chip->ops > created for this purpose. > > Signed-off-by: Jan Dabros <jsd@semihalf.com> I get that architecturally using the standard callbacks is a good idea. Still, you should explicitly document the gain because the existing code is working and field tested. > --- > drivers/char/tpm/tpm_tis_i2c_cr50.c | 106 ++++++++++++++++++---------- > 1 file changed, 70 insertions(+), 36 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c > index 77cea5b31c6e4..517d8410d7da0 100644 > --- a/drivers/char/tpm/tpm_tis_i2c_cr50.c > +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c > @@ -17,6 +17,7 @@ > */ > > #include <linux/acpi.h> > +#include <linux/bug.h> > #include <linux/completion.h> > #include <linux/i2c.h> > #include <linux/interrupt.h> > @@ -35,6 +36,7 @@ > #define TPM_CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C errors */ > #define TPM_CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs between retries on I2C */ > #define TPM_CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs between retries on I2C */ > +#define TPM_CR50_I2C_DEFAULT_LOC 0 > > #define TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4)) > #define TPM_I2C_STS(l) (0x0001 | ((l) << 4)) > @@ -286,20 +288,21 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer, > } > > /** > - * tpm_cr50_check_locality() - Verify TPM locality 0 is active. > + * tpm_cr50_check_locality() - Verify if required TPM locality is active. > * @chip: A TPM chip. > + * @loc: Locality to be verified > * > * Return: > * - 0: Success. > * - -errno: A POSIX error code. > */ > -static int tpm_cr50_check_locality(struct tpm_chip *chip) > +static int tpm_cr50_check_locality(struct tpm_chip *chip, int loc) > { > u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY; > u8 buf; > int rc; > > - rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf)); > + rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf)); > if (rc < 0) > return rc; > > @@ -312,48 +315,57 @@ static int tpm_cr50_check_locality(struct tpm_chip *chip) > /** > * tpm_cr50_release_locality() - Release TPM locality. > * @chip: A TPM chip. > - * @force: Flag to force release if set. > + * @loc: Locality to be released > + * > + * Return: > + * - 0: Success. > + * - -errno: A POSIX error code. > */ > -static void tpm_cr50_release_locality(struct tpm_chip *chip, bool force) > +static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc) > { > u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING; > - u8 addr = TPM_I2C_ACCESS(0); > + u8 addr = TPM_I2C_ACCESS(loc); > u8 buf; > + int rc; > > - if (tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf)) < 0) > - return; > + rc = tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf)); > + if (rc < 0) > + return rc; > > - if (force || (buf & mask) == mask) { > + if ((buf & mask) == mask) { > buf = TPM_ACCESS_ACTIVE_LOCALITY; > - tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf)); > + rc = tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf)); > } > + > + return rc; > } > > /** > - * tpm_cr50_request_locality() - Request TPM locality 0. > + * tpm_cr50_request_locality() - Request TPM locality. > * @chip: A TPM chip. > + * @loc: Locality to be requested. > * > * Return: > - * - 0: Success. > + * - loc: Success. > * - -errno: A POSIX error code. > */ > -static int tpm_cr50_request_locality(struct tpm_chip *chip) > +static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc) > { > u8 buf = TPM_ACCESS_REQUEST_USE; > unsigned long stop; > int rc; > > - if (!tpm_cr50_check_locality(chip)) > - return 0; > + if (!tpm_cr50_check_locality(chip, loc)) > + return loc; > > - rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf)); > + rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf)); > if (rc < 0) > return rc; > > stop = jiffies + chip->timeout_a; > do { > - if (!tpm_cr50_check_locality(chip)) > - return 0; > + if (!tpm_cr50_check_locality(chip, loc)) > + return loc; > > msleep(TPM_CR50_TIMEOUT_SHORT_MS); > } while (time_before(jiffies, stop)); > @@ -374,7 +386,12 @@ static u8 tpm_cr50_i2c_tis_status(struct tpm_chip *chip) > { > u8 buf[4]; > > - if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(0), buf, sizeof(buf)) < 0) > + if (chip->locality < 0){ > + WARN_ONCE(1, "Incorrect tpm locality value\n"); Never ever add WARN() for a success case. It can ultimately crash the whole system, if panic_on_warn is enabled. Since this is a success case, judging from the return value, at most you should use pr_info() here. > + return 0; > + } > + > + if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf)) < 0) > return 0; > > return buf[0]; > @@ -390,7 +407,12 @@ static void tpm_cr50_i2c_tis_set_ready(struct tpm_chip *chip) > { > u8 buf[4] = { TPM_STS_COMMAND_READY }; > > - tpm_cr50_i2c_write(chip, TPM_I2C_STS(0), buf, sizeof(buf)); > + if (chip->locality < 0) { > + WARN_ONCE(1, "Incorrect tpm locality value\n"); > + return; > + } > + > + tpm_cr50_i2c_write(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf)); > msleep(TPM_CR50_TIMEOUT_SHORT_MS); > } > > @@ -420,7 +442,7 @@ static int tpm_cr50_i2c_get_burst_and_status(struct tpm_chip *chip, u8 mask, > stop = jiffies + chip->timeout_b; > > do { > - if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(0), buf, sizeof(buf)) < 0) { > + if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(chip->locality), buf, sizeof(buf)) < 0) { > msleep(TPM_CR50_TIMEOUT_SHORT_MS); > continue; > } > @@ -454,10 +476,15 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len) > > u8 mask = TPM_STS_VALID | TPM_STS_DATA_AVAIL; > size_t burstcnt, cur, len, expected; > - u8 addr = TPM_I2C_DATA_FIFO(0); > + u8 addr = TPM_I2C_DATA_FIFO(chip->locality); > u32 status; > int rc; > > + if (chip->locality < 0) { > + WARN_ONCE(1, "Incorrect tpm locality value\n"); > + return -EINVAL; > + } > + > if (buf_len < TPM_HEADER_SIZE) > return -EINVAL; > > @@ -516,7 +543,6 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len) > goto out_err; > } > > - tpm_cr50_release_locality(chip, false); > return cur; > > out_err: > @@ -524,7 +550,6 @@ static int tpm_cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len) > if (tpm_cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY) > tpm_cr50_i2c_tis_set_ready(chip); > > - tpm_cr50_release_locality(chip, false); > return rc; > } > > @@ -546,9 +571,10 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > u32 status; > int rc; > > - rc = tpm_cr50_request_locality(chip); > - if (rc < 0) > - return rc; > + if (chip->locality < 0) { > + WARN_ONCE(1, "Incorrect tpm locality value\n"); > + return -EINVAL; > + } > > /* Wait until TPM is ready for a command */ > stop = jiffies + chip->timeout_b; > @@ -578,7 +604,8 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > * that is inserted by tpm_cr50_i2c_write() > */ > limit = min_t(size_t, burstcnt - 1, len); > - rc = tpm_cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(0), &buf[sent], limit); > + rc = tpm_cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(chip->locality), > + &buf[sent], limit); > if (rc < 0) { > dev_err(&chip->dev, "Write failed\n"); > goto out_err; > @@ -599,7 +626,7 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > } > > /* Start the TPM command */ > - rc = tpm_cr50_i2c_write(chip, TPM_I2C_STS(0), tpm_go, > + rc = tpm_cr50_i2c_write(chip, TPM_I2C_STS(chip->locality), tpm_go, > sizeof(tpm_go)); > if (rc < 0) { > dev_err(&chip->dev, "Start command failed\n"); > @@ -612,7 +639,6 @@ static int tpm_cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > if (tpm_cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY) > tpm_cr50_i2c_tis_set_ready(chip); > > - tpm_cr50_release_locality(chip, false); > return rc; > } > > @@ -651,6 +677,8 @@ static const struct tpm_class_ops cr50_i2c = { > .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID, > .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID, > .req_canceled = &tpm_cr50_i2c_req_canceled, > + .request_locality = &tpm_cr50_request_locality, > + .relinquish_locality = &tpm_cr50_release_locality, > }; > > #ifdef CONFIG_ACPI > @@ -686,6 +714,7 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client) > u32 vendor; > u8 buf[4]; > int rc; > + int loc; > > if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) > return -ENODEV; > @@ -728,24 +757,30 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client) > TPM_CR50_TIMEOUT_NOIRQ_MS); > } > > - rc = tpm_cr50_request_locality(chip); > - if (rc < 0) { > + loc = tpm_cr50_request_locality(chip, TPM_CR50_I2C_DEFAULT_LOC); > + if (loc < 0) { > dev_err(dev, "Could not request locality\n"); > return rc; > } > > /* Read four bytes from DID_VID register */ > - rc = tpm_cr50_i2c_read(chip, TPM_I2C_DID_VID(0), buf, sizeof(buf)); > + rc = tpm_cr50_i2c_read(chip, TPM_I2C_DID_VID(loc), buf, sizeof(buf)); > if (rc < 0) { > dev_err(dev, "Could not read vendor id\n"); > - tpm_cr50_release_locality(chip, true); > + if (tpm_cr50_release_locality(chip, loc)) > + dev_err(dev, "Could not release locality\n"); > + return rc; > + } > + > + rc = tpm_cr50_release_locality(chip, loc); > + if (rc) { > + dev_err(dev, "Could not release locality\n"); > return rc; > } > > vendor = le32_to_cpup((__le32 *)buf); > if (vendor != TPM_CR50_I2C_DID_VID && vendor != TPM_TI50_I2C_DID_VID) { > dev_err(dev, "Vendor ID did not match! ID was %08x\n", vendor); > - tpm_cr50_release_locality(chip, true); > return -ENODEV; > } > > @@ -774,7 +809,6 @@ static void tpm_cr50_i2c_remove(struct i2c_client *client) > } > > tpm_chip_unregister(chip); > - tpm_cr50_release_locality(chip, true); > } > > static SIMPLE_DEV_PM_OPS(cr50_i2c_pm, tpm_pm_suspend, tpm_pm_resume); > -- > 2.38.1.273.g43a17bfeac-goog > BR, Jarkko ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] char: tpm: cr50: Use generic request/relinquish locality ops 2022-11-07 9:20 ` Jarkko Sakkinen @ 2022-11-07 9:41 ` Jan Dąbroś 0 siblings, 0 replies; 18+ messages in thread From: Jan Dąbroś @ 2022-11-07 9:41 UTC (permalink / raw) To: Jarkko Sakkinen Cc: linux-integrity, peterhuewe, jgg, gregkh, arnd, rrangel, timvp, apronin, mw, upstream > On Thu, Nov 03, 2022 at 03:54:49PM +0100, Jan Dabros wrote: > > Instead of using static functions tpm_cr50_request_locality and > > tpm_cr50_release_locality register callbacks from tpm class chip->ops > > created for this purpose. > > > > Signed-off-by: Jan Dabros <jsd@semihalf.com> > > I get that architecturally using the standard callbacks is a good idea. > Still, you should explicitly document the gain because the existing code > is working and field tested. ACK. I will mention here about the overall idea I have. > > > --- > > drivers/char/tpm/tpm_tis_i2c_cr50.c | 106 ++++++++++++++++++---------- > > 1 file changed, 70 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c > > index 77cea5b31c6e4..517d8410d7da0 100644 > > --- a/drivers/char/tpm/tpm_tis_i2c_cr50.c > > +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c > > @@ -17,6 +17,7 @@ > > */ > > > > #include <linux/acpi.h> > > +#include <linux/bug.h> > > #include <linux/completion.h> > > #include <linux/i2c.h> > > #include <linux/interrupt.h> > > @@ -35,6 +36,7 @@ > > #define TPM_CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C errors */ > > #define TPM_CR50_I2C_RETRY_DELAY_LO 55 /* Min usecs between retries on I2C */ > > #define TPM_CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs between retries on I2C */ > > +#define TPM_CR50_I2C_DEFAULT_LOC 0 > > > > #define TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4)) > > #define TPM_I2C_STS(l) (0x0001 | ((l) << 4)) > > @@ -286,20 +288,21 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer, > > } > > > > /** > > - * tpm_cr50_check_locality() - Verify TPM locality 0 is active. > > + * tpm_cr50_check_locality() - Verify if required TPM locality is active. > > * @chip: A TPM chip. > > + * @loc: Locality to be verified > > * > > * Return: > > * - 0: Success. > > * - -errno: A POSIX error code. > > */ > > -static int tpm_cr50_check_locality(struct tpm_chip *chip) > > +static int tpm_cr50_check_locality(struct tpm_chip *chip, int loc) > > { > > u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY; > > u8 buf; > > int rc; > > > > - rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf)); > > + rc = tpm_cr50_i2c_read(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf)); > > if (rc < 0) > > return rc; > > > > @@ -312,48 +315,57 @@ static int tpm_cr50_check_locality(struct tpm_chip *chip) > > /** > > * tpm_cr50_release_locality() - Release TPM locality. > > * @chip: A TPM chip. > > - * @force: Flag to force release if set. > > + * @loc: Locality to be released > > + * > > + * Return: > > + * - 0: Success. > > + * - -errno: A POSIX error code. > > */ > > -static void tpm_cr50_release_locality(struct tpm_chip *chip, bool force) > > +static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc) > > { > > u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING; > > - u8 addr = TPM_I2C_ACCESS(0); > > + u8 addr = TPM_I2C_ACCESS(loc); > > u8 buf; > > + int rc; > > > > - if (tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf)) < 0) > > - return; > > + rc = tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf)); > > + if (rc < 0) > > + return rc; > > > > - if (force || (buf & mask) == mask) { > > + if ((buf & mask) == mask) { > > buf = TPM_ACCESS_ACTIVE_LOCALITY; > > - tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf)); > > + rc = tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf)); > > } > > + > > + return rc; > > } > > > > /** > > - * tpm_cr50_request_locality() - Request TPM locality 0. > > + * tpm_cr50_request_locality() - Request TPM locality. > > * @chip: A TPM chip. > > + * @loc: Locality to be requested. > > * > > * Return: > > - * - 0: Success. > > + * - loc: Success. > > * - -errno: A POSIX error code. > > */ > > -static int tpm_cr50_request_locality(struct tpm_chip *chip) > > +static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc) > > { > > u8 buf = TPM_ACCESS_REQUEST_USE; > > unsigned long stop; > > int rc; > > > > - if (!tpm_cr50_check_locality(chip)) > > - return 0; > > + if (!tpm_cr50_check_locality(chip, loc)) > > + return loc; > > > > - rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(0), &buf, sizeof(buf)); > > + rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf)); > > if (rc < 0) > > return rc; > > > > stop = jiffies + chip->timeout_a; > > do { > > - if (!tpm_cr50_check_locality(chip)) > > - return 0; > > + if (!tpm_cr50_check_locality(chip, loc)) > > + return loc; > > > > msleep(TPM_CR50_TIMEOUT_SHORT_MS); > > } while (time_before(jiffies, stop)); > > @@ -374,7 +386,12 @@ static u8 tpm_cr50_i2c_tis_status(struct tpm_chip *chip) > > { > > u8 buf[4]; > > > > - if (tpm_cr50_i2c_read(chip, TPM_I2C_STS(0), buf, sizeof(buf)) < 0) > > + if (chip->locality < 0){ > > + WARN_ONCE(1, "Incorrect tpm locality value\n"); > > Never ever add WARN() for a success case. It can ultimately crash the whole > system, if panic_on_warn is enabled. > > Since this is a success case, judging from the return value, at most you > should use pr_info() here. ACK. Best Regards, Jan ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] char: tpm: cr50: Use generic request/relinquish locality ops 2022-11-03 14:54 ` [PATCH v2 2/3] char: tpm: cr50: Use generic request/relinquish locality ops Jan Dabros 2022-11-07 9:20 ` Jarkko Sakkinen @ 2022-11-18 14:09 ` Guenter Roeck 1 sibling, 0 replies; 18+ messages in thread From: Guenter Roeck @ 2022-11-18 14:09 UTC (permalink / raw) To: Jan Dabros Cc: linux-integrity, jarkko, peterhuewe, jgg, gregkh, arnd, rrangel, timvp, apronin, mw, upstream On Thu, Nov 03, 2022 at 03:54:49PM +0100, Jan Dabros wrote: > Instead of using static functions tpm_cr50_request_locality and > tpm_cr50_release_locality register callbacks from tpm class chip->ops > created for this purpose. > > Signed-off-by: Jan Dabros <jsd@semihalf.com> > --- [ ... ] > #ifdef CONFIG_ACPI > @@ -686,6 +714,7 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client) > u32 vendor; > u8 buf[4]; > int rc; > + int loc; > > if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) > return -ENODEV; > @@ -728,24 +757,30 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client) > TPM_CR50_TIMEOUT_NOIRQ_MS); > } > > - rc = tpm_cr50_request_locality(chip); > - if (rc < 0) { > + loc = tpm_cr50_request_locality(chip, TPM_CR50_I2C_DEFAULT_LOC); > + if (loc < 0) { > dev_err(dev, "Could not request locality\n"); > return rc; As reported by 0-day and Dan Carpenter: return loc; Guenter ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 3/3] char: tpm: cr50: Move i2c locking to request/relinquish locality ops 2022-11-03 14:54 [PATCH v2 0/3] char: tpm: Adjust cr50_i2c locking mechanism Jan Dabros 2022-11-03 14:54 ` [PATCH v2 1/3] char: tpm: Protect tpm_pm_suspend with locks Jan Dabros 2022-11-03 14:54 ` [PATCH v2 2/3] char: tpm: cr50: Use generic request/relinquish locality ops Jan Dabros @ 2022-11-03 14:54 ` Jan Dabros 2022-11-07 9:22 ` Jarkko Sakkinen 2 siblings, 1 reply; 18+ messages in thread From: Jan Dabros @ 2022-11-03 14:54 UTC (permalink / raw) To: linux-integrity, jarkko, peterhuewe, jgg, gregkh, arnd Cc: rrangel, timvp, apronin, mw, upstream, jsd Move i2c locking primitives to request_locality and relinquish_locality callbacks, what effectively blocks TPM bus for the whole duration of logical TPM operation. With this in place, cr50-equipped TPM may be shared with external CPUs - assuming that underneath i2c controller driver is aware of this setup (see i2c-designware-amdpsp as an example). Signed-off-by: Jan Dabros <jsd@semihalf.com> --- drivers/char/tpm/tpm_tis_i2c_cr50.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c index 517d8410d7da0..f8a67fc2382cc 100644 --- a/drivers/char/tpm/tpm_tis_i2c_cr50.c +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c @@ -202,8 +202,6 @@ static int tpm_cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t }; int rc; - i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); - /* Prepare for completion interrupt */ tpm_cr50_i2c_enable_tpm_irq(chip); @@ -222,7 +220,6 @@ static int tpm_cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t out: tpm_cr50_i2c_disable_tpm_irq(chip); - i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); if (rc < 0) return rc; @@ -264,8 +261,6 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer, priv->buf[0] = addr; memcpy(priv->buf + 1, buffer, len); - i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); - /* Prepare for completion interrupt */ tpm_cr50_i2c_enable_tpm_irq(chip); @@ -279,7 +274,6 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer, out: tpm_cr50_i2c_disable_tpm_irq(chip); - i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); if (rc < 0) return rc; @@ -323,6 +317,7 @@ static int tpm_cr50_check_locality(struct tpm_chip *chip, int loc) */ static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc) { + struct i2c_client *client = to_i2c_client(chip->dev.parent); u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING; u8 addr = TPM_I2C_ACCESS(loc); u8 buf; @@ -330,13 +325,15 @@ static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc) rc = tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf)); if (rc < 0) - return rc; + goto unlock_out; if ((buf & mask) == mask) { buf = TPM_ACCESS_ACTIVE_LOCALITY; rc = tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf)); } +unlock_out: + i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); return rc; } @@ -351,16 +348,19 @@ static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc) */ static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc) { + struct i2c_client *client = to_i2c_client(chip->dev.parent); u8 buf = TPM_ACCESS_REQUEST_USE; unsigned long stop; int rc; + i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); + if (!tpm_cr50_check_locality(chip, loc)) return loc; rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf)); if (rc < 0) - return rc; + goto unlock_out; stop = jiffies + chip->timeout_a; do { @@ -370,7 +370,11 @@ static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc) msleep(TPM_CR50_TIMEOUT_SHORT_MS); } while (time_before(jiffies, stop)); - return -ETIMEDOUT; + rc = -ETIMEDOUT; + +unlock_out: + i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); + return rc; } /** -- 2.38.1.273.g43a17bfeac-goog ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] char: tpm: cr50: Move i2c locking to request/relinquish locality ops 2022-11-03 14:54 ` [PATCH v2 3/3] char: tpm: cr50: Move i2c locking to " Jan Dabros @ 2022-11-07 9:22 ` Jarkko Sakkinen 2022-11-07 9:47 ` Jan Dąbroś 0 siblings, 1 reply; 18+ messages in thread From: Jarkko Sakkinen @ 2022-11-07 9:22 UTC (permalink / raw) To: Jan Dabros Cc: linux-integrity, peterhuewe, jgg, gregkh, arnd, rrangel, timvp, apronin, mw, upstream On Thu, Nov 03, 2022 at 03:54:50PM +0100, Jan Dabros wrote: > Move i2c locking primitives to request_locality and relinquish_locality > callbacks, what effectively blocks TPM bus for the whole duration of > logical TPM operation. > > With this in place, cr50-equipped TPM may be shared with external CPUs - > assuming that underneath i2c controller driver is aware of this setup > (see i2c-designware-amdpsp as an example). Nit: s/CPUs/peripherals/ ? I'm not sure why you want to emphasize external chips having CPU. > > Signed-off-by: Jan Dabros <jsd@semihalf.com> > --- > drivers/char/tpm/tpm_tis_i2c_cr50.c | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c > index 517d8410d7da0..f8a67fc2382cc 100644 > --- a/drivers/char/tpm/tpm_tis_i2c_cr50.c > +++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c > @@ -202,8 +202,6 @@ static int tpm_cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t > }; > int rc; > > - i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); > - > /* Prepare for completion interrupt */ > tpm_cr50_i2c_enable_tpm_irq(chip); > > @@ -222,7 +220,6 @@ static int tpm_cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, size_t > > out: > tpm_cr50_i2c_disable_tpm_irq(chip); > - i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); > > if (rc < 0) > return rc; > @@ -264,8 +261,6 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer, > priv->buf[0] = addr; > memcpy(priv->buf + 1, buffer, len); > > - i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); > - > /* Prepare for completion interrupt */ > tpm_cr50_i2c_enable_tpm_irq(chip); > > @@ -279,7 +274,6 @@ static int tpm_cr50_i2c_write(struct tpm_chip *chip, u8 addr, u8 *buffer, > > out: > tpm_cr50_i2c_disable_tpm_irq(chip); > - i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); > > if (rc < 0) > return rc; > @@ -323,6 +317,7 @@ static int tpm_cr50_check_locality(struct tpm_chip *chip, int loc) > */ > static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc) > { > + struct i2c_client *client = to_i2c_client(chip->dev.parent); > u8 mask = TPM_ACCESS_VALID | TPM_ACCESS_REQUEST_PENDING; > u8 addr = TPM_I2C_ACCESS(loc); > u8 buf; > @@ -330,13 +325,15 @@ static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc) > > rc = tpm_cr50_i2c_read(chip, addr, &buf, sizeof(buf)); > if (rc < 0) > - return rc; > + goto unlock_out; > > if ((buf & mask) == mask) { > buf = TPM_ACCESS_ACTIVE_LOCALITY; > rc = tpm_cr50_i2c_write(chip, addr, &buf, sizeof(buf)); > } > > +unlock_out: > + i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); > return rc; > } > > @@ -351,16 +348,19 @@ static int tpm_cr50_release_locality(struct tpm_chip *chip, int loc) > */ > static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc) > { > + struct i2c_client *client = to_i2c_client(chip->dev.parent); > u8 buf = TPM_ACCESS_REQUEST_USE; > unsigned long stop; > int rc; > > + i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); > + > if (!tpm_cr50_check_locality(chip, loc)) > return loc; > > rc = tpm_cr50_i2c_write(chip, TPM_I2C_ACCESS(loc), &buf, sizeof(buf)); > if (rc < 0) > - return rc; > + goto unlock_out; > > stop = jiffies + chip->timeout_a; > do { > @@ -370,7 +370,11 @@ static int tpm_cr50_request_locality(struct tpm_chip *chip, int loc) > msleep(TPM_CR50_TIMEOUT_SHORT_MS); > } while (time_before(jiffies, stop)); > > - return -ETIMEDOUT; > + rc = -ETIMEDOUT; > + > +unlock_out: > + i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); > + return rc; > } > > /** > -- > 2.38.1.273.g43a17bfeac-goog > BR, Jarkko ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] char: tpm: cr50: Move i2c locking to request/relinquish locality ops 2022-11-07 9:22 ` Jarkko Sakkinen @ 2022-11-07 9:47 ` Jan Dąbroś 2022-11-07 16:35 ` Jarkko Sakkinen 0 siblings, 1 reply; 18+ messages in thread From: Jan Dąbroś @ 2022-11-07 9:47 UTC (permalink / raw) To: Jarkko Sakkinen Cc: linux-integrity, peterhuewe, jgg, gregkh, arnd, rrangel, timvp, apronin, mw, upstream > On Thu, Nov 03, 2022 at 03:54:50PM +0100, Jan Dabros wrote: > > Move i2c locking primitives to request_locality and relinquish_locality > > callbacks, what effectively blocks TPM bus for the whole duration of > > logical TPM operation. > > > > With this in place, cr50-equipped TPM may be shared with external CPUs - > > assuming that underneath i2c controller driver is aware of this setup > > (see i2c-designware-amdpsp as an example). > > Nit: s/CPUs/peripherals/ ? I'm not sure why you want to emphasize external > chips having CPU. My point was that we can have a multi-controller setup and each controller may work with the TPM chip, so let me s/CPUs/controllers/. Best Regards, Jan ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] char: tpm: cr50: Move i2c locking to request/relinquish locality ops 2022-11-07 9:47 ` Jan Dąbroś @ 2022-11-07 16:35 ` Jarkko Sakkinen 0 siblings, 0 replies; 18+ messages in thread From: Jarkko Sakkinen @ 2022-11-07 16:35 UTC (permalink / raw) To: Jan Dąbroś Cc: linux-integrity, peterhuewe, jgg, gregkh, arnd, rrangel, timvp, apronin, mw, upstream On Mon, Nov 07, 2022 at 10:47:44AM +0100, Jan Dąbroś wrote: > > On Thu, Nov 03, 2022 at 03:54:50PM +0100, Jan Dabros wrote: > > > Move i2c locking primitives to request_locality and relinquish_locality > > > callbacks, what effectively blocks TPM bus for the whole duration of > > > logical TPM operation. > > > > > > With this in place, cr50-equipped TPM may be shared with external CPUs - > > > assuming that underneath i2c controller driver is aware of this setup > > > (see i2c-designware-amdpsp as an example). > > > > Nit: s/CPUs/peripherals/ ? I'm not sure why you want to emphasize external > > chips having CPU. > > My point was that we can have a multi-controller setup and each > controller may work with the TPM chip, so let me s/CPUs/controllers/. ack BR, Jarkko ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2022-11-28 19:55 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-03 14:54 [PATCH v2 0/3] char: tpm: Adjust cr50_i2c locking mechanism Jan Dabros 2022-11-03 14:54 ` [PATCH v2 1/3] char: tpm: Protect tpm_pm_suspend with locks Jan Dabros 2022-11-06 19:48 ` Jarkko Sakkinen 2022-11-07 8:45 ` Jan Dąbroś 2022-11-07 16:35 ` Jarkko Sakkinen 2022-11-28 17:04 ` Jason A. Donenfeld 2022-11-28 17:07 ` Jason A. Donenfeld 2022-11-28 19:46 ` Vlastimil Babka 2022-11-28 19:55 ` Jason A. Donenfeld 2022-11-28 17:11 ` Vlastimil Babka 2022-11-03 14:54 ` [PATCH v2 2/3] char: tpm: cr50: Use generic request/relinquish locality ops Jan Dabros 2022-11-07 9:20 ` Jarkko Sakkinen 2022-11-07 9:41 ` Jan Dąbroś 2022-11-18 14:09 ` Guenter Roeck 2022-11-03 14:54 ` [PATCH v2 3/3] char: tpm: cr50: Move i2c locking to " Jan Dabros 2022-11-07 9:22 ` Jarkko Sakkinen 2022-11-07 9:47 ` Jan Dąbroś 2022-11-07 16:35 ` Jarkko Sakkinen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.