| Message ID | 20230822231510.2263255-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] tpm: Enable hwrng only for Pluton on AMD CPUs | expand |
Dear Jarkko, Thank you for your patch. Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen: > The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for > all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. On the > reported systems the TPM doesn't reply at bootup and returns back the > command code. This makes the TPM fail probe. > > Since only Microsoft Pluton is the only known combination of AMD CPU and > fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin > aware of this, print also info message to the klog. > > Cc: stable@vger.kernel.org > Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") > Reported-by: Todd Brandt <todd.e.brandt@intel.com> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> Mario’s patch also had the three reporters below listed: Reported-by: Patrick Steinhardt <ps@pks.im> Reported-by: Ronan Pigott <ronan@rjp.ie> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > --- > v3: > * Forgot to amend config flags. > v2: > * CONFIG_X86 > * Removed "Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>" > * Removed "Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>" > --- > drivers/char/tpm/tpm_crb.c | 33 ++++++++------------------------- > 1 file changed, 8 insertions(+), 25 deletions(-) > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > index 65ff4d2fbe8d..ea085b14ab7c 100644 > --- a/drivers/char/tpm/tpm_crb.c > +++ b/drivers/char/tpm/tpm_crb.c > @@ -463,28 +463,6 @@ static bool crb_req_canceled(struct tpm_chip *chip, u8 status) > return (cancel & CRB_CANCEL_INVOKE) == CRB_CANCEL_INVOKE; > } > > -static int crb_check_flags(struct tpm_chip *chip) > -{ > - u32 val; > - int ret; > - > - ret = crb_request_locality(chip, 0); > - if (ret) > - return ret; > - > - ret = tpm2_get_tpm_pt(chip, TPM2_PT_MANUFACTURER, &val, NULL); > - if (ret) > - goto release; > - > - if (val == 0x414D4400U /* AMD */) > - chip->flags |= TPM_CHIP_FLAG_HWRNG_DISABLED; > - > -release: > - crb_relinquish_locality(chip, 0); > - > - return ret; > -} > - > static const struct tpm_class_ops tpm_crb = { > .flags = TPM_OPS_AUTO_STARTUP, > .status = crb_status, > @@ -827,9 +805,14 @@ static int crb_acpi_add(struct acpi_device *device) > if (rc) > goto out; > > - rc = crb_check_flags(chip); > - if (rc) > - goto out; > +#ifdef CONFIG_X86 > + /* A quirk for https://www.amd.com/en/support/kb/faq/pa-410 */ > + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD && > + priv->sm != ACPI_TPM2_COMMAND_BUFFER_WITH_PLUTON) { > + dev_info(dev, "Disabling hwrng\n"); A more elaborate log message would be helpful for the user. Maybe: Disabling hwrng in AMD's fTPM to avoid stutter (AMD article PA 410) > + chip->flags |= TPM_CHIP_FLAG_HWRNG_DISABLED; > + } > +#endif /* CONFIG_X86 */ > > rc = tpm_chip_register(chip); > Kind regards, Paul
On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote: > Dear Jarkko, > > > Thank you for your patch. > > > Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen: > > The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for > > all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. On the > > reported systems the TPM doesn't reply at bootup and returns back the > > command code. This makes the TPM fail probe. > > > > Since only Microsoft Pluton is the only known combination of AMD CPU and > > fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin > > aware of this, print also info message to the klog. > > > > Cc: stable@vger.kernel.org > > Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") > > Reported-by: Todd Brandt <todd.e.brandt@intel.com> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > Mario’s patch also had the three reporters below listed: > > Reported-by: Patrick Steinhardt <ps@pks.im> > Reported-by: Ronan Pigott <ronan@rjp.ie> > Reported-by: Raymond Jay Golo <rjgolo@gmail.com> The problem here is that checkpatch throws three warnings: WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report #19: Reported-by: Patrick Steinhardt <ps@pks.im> Reported-by: Ronan Pigott <ronan@rjp.ie> WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report #20: Reported-by: Ronan Pigott <ronan@rjp.ie> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report #21: Reported-by: Raymond Jay Golo <rjgolo@gmail.com> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> Since bugzilla is not part of the documented process afaik, I used this field as the guideline: Reported: 2023-08-17 20:59 UTC by Todd Brandt How otherwise I should interpret kernel bugzilla? In any case new version is still needed as the commit message must contain a mention of "Lenovo Legion Y540" as the stimulus for doing this code change in the first place. BR, Jarkko
On 8/23/2023 12:40, Jarkko Sakkinen wrote: > On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote: >> Dear Jarkko, >> >> >> Thank you for your patch. >> >> >> Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen: >>> The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for >>> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. On the >>> reported systems the TPM doesn't reply at bootup and returns back the >>> command code. This makes the TPM fail probe. >>> >>> Since only Microsoft Pluton is the only known combination of AMD CPU and >>> fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin >>> aware of this, print also info message to the klog. >>> >>> Cc: stable@vger.kernel.org >>> Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") >>> Reported-by: Todd Brandt <todd.e.brandt@intel.com> >>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 >>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> >> >> Mario’s patch also had the three reporters below listed: >> >> Reported-by: Patrick Steinhardt <ps@pks.im> >> Reported-by: Ronan Pigott <ronan@rjp.ie> >> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > The problem here is that checkpatch throws three warnings: > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > #19: > Reported-by: Patrick Steinhardt <ps@pks.im> > Reported-by: Ronan Pigott <ronan@rjp.ie> > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > #20: > Reported-by: Ronan Pigott <ronan@rjp.ie> > Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > #21: > Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > FWIW I observed the same checkpatch warning when I submitted my version of the patch. I figured it's better to ignore the warning and attribute everyone who reported the issue affected them. If nothing else it gives more people to pull in and check any future fixes if there is a regression caused by this patch that forces it to be reverted. > Since bugzilla is not part of the documented process afaik, I used this > field as the guideline: > > Reported: 2023-08-17 20:59 UTC by Todd Brandt > > How otherwise I should interpret kernel bugzilla? > > In any case new version is still needed as the commit message must > contain a mention of "Lenovo Legion Y540" as the stimulus for doing > this code change in the first place. > > BR, Jarkko
[Cc: +Andy, +Joe] Dear Jarkko, dear Andy, dear Joe, Am 23.08.23 um 19:40 schrieb Jarkko Sakkinen: > On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote: >> Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen: >>> The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for >>> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. On the >>> reported systems the TPM doesn't reply at bootup and returns back the >>> command code. This makes the TPM fail probe. >>> >>> Since only Microsoft Pluton is the only known combination of AMD CPU and >>> fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin >>> aware of this, print also info message to the klog. >>> >>> Cc: stable@vger.kernel.org >>> Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") >>> Reported-by: Todd Brandt <todd.e.brandt@intel.com> >>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 >>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> >> >> Mario’s patch also had the three reporters below listed: >> >> Reported-by: Patrick Steinhardt <ps@pks.im> >> Reported-by: Ronan Pigott <ronan@rjp.ie> >> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > The problem here is that checkpatch throws three warnings: > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > #19: > Reported-by: Patrick Steinhardt <ps@pks.im> > Reported-by: Ronan Pigott <ronan@rjp.ie> > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > #20: > Reported-by: Ronan Pigott <ronan@rjp.ie> > Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > #21: > Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > Since bugzilla is not part of the documented process afaik, I used this > field as the guideline: > > Reported: 2023-08-17 20:59 UTC by Todd Brandt > > How otherwise I should interpret kernel bugzilla? How is the proper process to add more than one reporter (so they are noted and also added to CC), so that checkpatch.pl does not complain? Kind regards, Paul > In any case new version is still needed as the commit message must > contain a mention of "Lenovo Legion Y540" as the stimulus for doing > this code change in the first place. > > BR, Jarkko
On Wed, 2023-08-23 at 21:24 +0200, Paul Menzel wrote: > [Cc: +Andy, +Joe] > > > Dear Jarkko, dear Andy, dear Joe, > > > Am 23.08.23 um 19:40 schrieb Jarkko Sakkinen: > > On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote: > > > > Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen: > > > > The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for > > > > all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. On the > > > > reported systems the TPM doesn't reply at bootup and returns back the > > > > command code. This makes the TPM fail probe. > > > > > > > > Since only Microsoft Pluton is the only known combination of AMD CPU and > > > > fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin > > > > aware of this, print also info message to the klog. > > > > > > > > Cc: stable@vger.kernel.org > > > > Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") > > > > Reported-by: Todd Brandt <todd.e.brandt@intel.com> > > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > > > Mario’s patch also had the three reporters below listed: > > > > > > Reported-by: Patrick Steinhardt <ps@pks.im> > > > Reported-by: Ronan Pigott <ronan@rjp.ie> > > > Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > > > The problem here is that checkpatch throws three warnings: > > > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > #19: > > Reported-by: Patrick Steinhardt <ps@pks.im> > > Reported-by: Ronan Pigott <ronan@rjp.ie> > > > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > #20: > > Reported-by: Ronan Pigott <ronan@rjp.ie> > > Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > #21: > > Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > Since bugzilla is not part of the documented process afaik, I used this > > field as the guideline: > > > > Reported: 2023-08-17 20:59 UTC by Todd Brandt > > > > How otherwise I should interpret kernel bugzilla? > > How is the proper process to add more than one reporter (so they are > noted and also added to CC), so that checkpatch.pl does not complain? > > > Kind regards, > > Paul > > > > In any case new version is still needed as the commit message must > > contain a mention of "Lenovo Legion Y540" as the stimulus for doing > > this code change in the first place. > > > > BR, Jarkko Well, if it's really desired to allow multiple consecutive reported-by: lines, maybe: --- scripts/checkpatch.pl | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 528f619520eb9..5b5c11ad04087 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -3179,6 +3179,8 @@ sub process { if (!defined $lines[$linenr]) { WARN("BAD_REPORTED_BY_LINK", "Reported-by: should be immediately followed by Closes: with a URL to the report\n" . $herecurr . "\n"); + } elsif ($rawlines[$linenr] =~ /^\s*reported(?:|-and-tested)-by:/i) { + ; } elsif ($rawlines[$linenr] !~ /^closes:\s*/i) { WARN("BAD_REPORTED_BY_LINK", "Reported-by: should be immediately followed by Closes: with a URL to the report\n" . $herecurr . $rawlines[$linenr] . "\n");
On Wed Aug 23, 2023 at 9:58 PM EEST, Mario Limonciello wrote: > On 8/23/2023 12:40, Jarkko Sakkinen wrote: > > On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote: > >> Dear Jarkko, > >> > >> > >> Thank you for your patch. > >> > >> > >> Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen: > >>> The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for > >>> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. On the > >>> reported systems the TPM doesn't reply at bootup and returns back the > >>> command code. This makes the TPM fail probe. > >>> > >>> Since only Microsoft Pluton is the only known combination of AMD CPU and > >>> fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin > >>> aware of this, print also info message to the klog. > >>> > >>> Cc: stable@vger.kernel.org > >>> Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") > >>> Reported-by: Todd Brandt <todd.e.brandt@intel.com> > >>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 > >>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > >> > >> Mario’s patch also had the three reporters below listed: > >> > >> Reported-by: Patrick Steinhardt <ps@pks.im> > >> Reported-by: Ronan Pigott <ronan@rjp.ie> > >> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > > > The problem here is that checkpatch throws three warnings: > > > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > #19: > > Reported-by: Patrick Steinhardt <ps@pks.im> > > Reported-by: Ronan Pigott <ronan@rjp.ie> > > > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > #20: > > Reported-by: Ronan Pigott <ronan@rjp.ie> > > Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > #21: > > Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > FWIW I observed the same checkpatch warning when I submitted my version > of the patch. I figured it's better to ignore the warning and attribute > everyone who reported the issue affected them. OK so: 1. checkpatch.pl is part of the kernel process. 2. Bugzilla is not part of the kernel process. Why emphasis on 1? BR, Jarkko
On Wed Aug 23, 2023 at 10:24 PM EEST, Paul Menzel wrote: > [Cc: +Andy, +Joe] > > > Dear Jarkko, dear Andy, dear Joe, > > > Am 23.08.23 um 19:40 schrieb Jarkko Sakkinen: > > On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote: > > >> Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen: > >>> The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for > >>> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. On the > >>> reported systems the TPM doesn't reply at bootup and returns back the > >>> command code. This makes the TPM fail probe. > >>> > >>> Since only Microsoft Pluton is the only known combination of AMD CPU and > >>> fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin > >>> aware of this, print also info message to the klog. > >>> > >>> Cc: stable@vger.kernel.org > >>> Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") > >>> Reported-by: Todd Brandt <todd.e.brandt@intel.com> > >>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 > >>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > >> > >> Mario’s patch also had the three reporters below listed: > >> > >> Reported-by: Patrick Steinhardt <ps@pks.im> > >> Reported-by: Ronan Pigott <ronan@rjp.ie> > >> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > > > The problem here is that checkpatch throws three warnings: > > > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > #19: > > Reported-by: Patrick Steinhardt <ps@pks.im> > > Reported-by: Ronan Pigott <ronan@rjp.ie> > > > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > #20: > > Reported-by: Ronan Pigott <ronan@rjp.ie> > > Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > #21: > > Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > Since bugzilla is not part of the documented process afaik, I used this > > field as the guideline: > > > > Reported: 2023-08-17 20:59 UTC by Todd Brandt > > > > How otherwise I should interpret kernel bugzilla? > > How is the proper process to add more than one reporter (so they are > noted and also added to CC), so that checkpatch.pl does not complain? I have no idea. I actually tried all sorts of combinations with no luck. Since it exists and is out there, the process documentation should really bring up some clarity to the kernel bugzilla. BR, Jarkko
On Thu Aug 24, 2023 at 7:43 AM EEST, Joe Perches wrote: > On Wed, 2023-08-23 at 21:24 +0200, Paul Menzel wrote: > > [Cc: +Andy, +Joe] > > > > > > Dear Jarkko, dear Andy, dear Joe, > > > > > > Am 23.08.23 um 19:40 schrieb Jarkko Sakkinen: > > > On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote: > > > > > > Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen: > > > > > The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for > > > > > all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. On the > > > > > reported systems the TPM doesn't reply at bootup and returns back the > > > > > command code. This makes the TPM fail probe. > > > > > > > > > > Since only Microsoft Pluton is the only known combination of AMD CPU and > > > > > fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin > > > > > aware of this, print also info message to the klog. > > > > > > > > > > Cc: stable@vger.kernel.org > > > > > Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") > > > > > Reported-by: Todd Brandt <todd.e.brandt@intel.com> > > > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 > > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > > > > > Mario’s patch also had the three reporters below listed: > > > > > > > > Reported-by: Patrick Steinhardt <ps@pks.im> > > > > Reported-by: Ronan Pigott <ronan@rjp.ie> > > > > Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > > > > > The problem here is that checkpatch throws three warnings: > > > > > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > > #19: > > > Reported-by: Patrick Steinhardt <ps@pks.im> > > > Reported-by: Ronan Pigott <ronan@rjp.ie> > > > > > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > > #20: > > > Reported-by: Ronan Pigott <ronan@rjp.ie> > > > Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > > > > > WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > > #21: > > > Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > > > Since bugzilla is not part of the documented process afaik, I used this > > > field as the guideline: > > > > > > Reported: 2023-08-17 20:59 UTC by Todd Brandt > > > > > > How otherwise I should interpret kernel bugzilla? > > > > How is the proper process to add more than one reporter (so they are > > noted and also added to CC), so that checkpatch.pl does not complain? > > > > > > Kind regards, > > > > Paul > > > > > > > In any case new version is still needed as the commit message must > > > contain a mention of "Lenovo Legion Y540" as the stimulus for doing > > > this code change in the first place. > > > > > > BR, Jarkko > > Well, if it's really desired to allow multiple consecutive reported-by: > lines, maybe: > --- > scripts/checkpatch.pl | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 528f619520eb9..5b5c11ad04087 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -3179,6 +3179,8 @@ sub process { > if (!defined $lines[$linenr]) { > WARN("BAD_REPORTED_BY_LINK", > "Reported-by: should be immediately followed by Closes: with a URL to the report\n" . $herecurr . "\n"); > + } elsif ($rawlines[$linenr] =~ /^\s*reported(?:|-and-tested)-by:/i) { > + ; > } elsif ($rawlines[$linenr] !~ /^closes:\s*/i) { > WARN("BAD_REPORTED_BY_LINK", > "Reported-by: should be immediately followed by Closes: with a URL to the report\n" . $herecurr . $rawlines[$linenr] . "\n"); Kind of opposing this because: 1. Bugzilla has a reporter field. 2. The request is now, if I understood this correctly, to add reported-by field to all people who have left a comment. 3. There is a field for the reporter, which points out to a single person. Why all the possible commenters and not the creator of the report? BR, Jarkko
On 8/27/2023 13:12, Jarkko Sakkinen wrote: > On Wed Aug 23, 2023 at 9:58 PM EEST, Mario Limonciello wrote: >> On 8/23/2023 12:40, Jarkko Sakkinen wrote: >>> On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote: >>>> Dear Jarkko, >>>> >>>> >>>> Thank you for your patch. >>>> >>>> >>>> Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen: >>>>> The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for >>>>> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. On the >>>>> reported systems the TPM doesn't reply at bootup and returns back the >>>>> command code. This makes the TPM fail probe. >>>>> >>>>> Since only Microsoft Pluton is the only known combination of AMD CPU and >>>>> fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin >>>>> aware of this, print also info message to the klog. >>>>> >>>>> Cc: stable@vger.kernel.org >>>>> Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") >>>>> Reported-by: Todd Brandt <todd.e.brandt@intel.com> >>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 >>>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> >>>> >>>> Mario’s patch also had the three reporters below listed: >>>> >>>> Reported-by: Patrick Steinhardt <ps@pks.im> >>>> Reported-by: Ronan Pigott <ronan@rjp.ie> >>>> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> >>> >>> The problem here is that checkpatch throws three warnings: >>> >>> WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report >>> #19: >>> Reported-by: Patrick Steinhardt <ps@pks.im> >>> Reported-by: Ronan Pigott <ronan@rjp.ie> >>> >>> WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report >>> #20: >>> Reported-by: Ronan Pigott <ronan@rjp.ie> >>> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> >>> >>> WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report >>> #21: >>> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> >>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> >>> >> >> FWIW I observed the same checkpatch warning when I submitted my version >> of the patch. I figured it's better to ignore the warning and attribute >> everyone who reported the issue affected them. > > OK so: > > 1. checkpatch.pl is part of the kernel process. > 2. Bugzilla is not part of the kernel process. > > Why emphasis on 1? > > BR, Jarkko The reason I submitted it this way is because of this quote from the documentation [1]. "Check your patches with the patch style checker prior to submission (scripts/checkpatch.pl). Note, though, that the style checker should be viewed as a guide, not as a replacement for human judgment. If your code looks better with a violation then its probably best left alone." I wanted the patch to capture and attribute all those that reported it not just the "first one". Like I said previously, it's better to have a collection of people to ping to notify if something needs to be reverted. [1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#style-check-your-changes
On Wed, Aug 23, 2023 at 02:15:10AM +0300, Jarkko Sakkinen wrote: > The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for > all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. On the > reported systems the TPM doesn't reply at bootup and returns back the > command code. This makes the TPM fail probe. > > Since only Microsoft Pluton is the only known combination of AMD CPU and > fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin > aware of this, print also info message to the klog. > > Cc: stable@vger.kernel.org > Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") > Reported-by: Todd Brandt <todd.e.brandt@intel.com> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > --- > v3: > * Forgot to amend config flags. > v2: > * CONFIG_X86 > * Removed "Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>" > * Removed "Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>" > --- > drivers/char/tpm/tpm_crb.c | 33 ++++++++------------------------- > 1 file changed, 8 insertions(+), 25 deletions(-) > It looks like stable should be pinged as well. I saw a report yesterday for Fedora where someone is seeing issue where the tpm device no longer shows up with a 6.4.11 kernel. That kernel pulled in commit 554b841d4703. It pulled in a couple other tpm commits, and there is practically no info in the bug at this point, but I'm guessing the probe is failing due to 554b841d4703. > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > index 65ff4d2fbe8d..ea085b14ab7c 100644 > --- a/drivers/char/tpm/tpm_crb.c > +++ b/drivers/char/tpm/tpm_crb.c > @@ -463,28 +463,6 @@ static bool crb_req_canceled(struct tpm_chip *chip, u8 status) > return (cancel & CRB_CANCEL_INVOKE) == CRB_CANCEL_INVOKE; > } > > -static int crb_check_flags(struct tpm_chip *chip) > -{ > - u32 val; > - int ret; > - > - ret = crb_request_locality(chip, 0); > - if (ret) > - return ret; > - > - ret = tpm2_get_tpm_pt(chip, TPM2_PT_MANUFACTURER, &val, NULL); > - if (ret) > - goto release; > - > - if (val == 0x414D4400U /* AMD */) > - chip->flags |= TPM_CHIP_FLAG_HWRNG_DISABLED; > - > -release: > - crb_relinquish_locality(chip, 0); > - > - return ret; > -} > - > static const struct tpm_class_ops tpm_crb = { > .flags = TPM_OPS_AUTO_STARTUP, > .status = crb_status, > @@ -827,9 +805,14 @@ static int crb_acpi_add(struct acpi_device *device) > if (rc) > goto out; > > - rc = crb_check_flags(chip); > - if (rc) > - goto out; > +#ifdef CONFIG_X86 > + /* A quirk for https://www.amd.com/en/support/kb/faq/pa-410 */ > + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD && > + priv->sm != ACPI_TPM2_COMMAND_BUFFER_WITH_PLUTON) { > + dev_info(dev, "Disabling hwrng\n"); > + chip->flags |= TPM_CHIP_FLAG_HWRNG_DISABLED; > + } > +#endif /* CONFIG_X86 */ > > rc = tpm_chip_register(chip); > > -- > 2.39.2 >
[CCing the regression list, as it should be in the loop for regressions: https://docs.kernel.org/admin-guide/reporting-regressions.html] On 28.08.23 02:35, Mario Limonciello wrote: > On 8/27/2023 13:12, Jarkko Sakkinen wrote: >> On Wed Aug 23, 2023 at 9:58 PM EEST, Mario Limonciello wrote: >>> On 8/23/2023 12:40, Jarkko Sakkinen wrote: >>>> On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote: >>>>> Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen: >>>>>> The vendor check introduced by commit 554b841d4703 ("tpm: Disable >>>>>> RNG for >>>>>> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. >>>>>> On the >>>>>> reported systems the TPM doesn't reply at bootup and returns back the >>>>>> command code. This makes the TPM fail probe. >>>>>> >>>>>> Since only Microsoft Pluton is the only known combination of AMD >>>>>> CPU and >>>>>> fTPM from other vendor, disable hwrng otherwise. In order to make >>>>>> sysadmin >>>>>> aware of this, print also info message to the klog. >>>>>> >>>>>> Cc: stable@vger.kernel.org >>>>>> Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") >>>>>> Reported-by: Todd Brandt <todd.e.brandt@intel.com> >>>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 >>>>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> >>>>> >>>>> Mario’s patch also had the three reporters below listed: >>>>> >>>>> Reported-by: Patrick Steinhardt <ps@pks.im> >>>>> Reported-by: Ronan Pigott <ronan@rjp.ie> >>>>> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> >>>> >>>> The problem here is that checkpatch throws three warnings: >>>> >>>> WARNING: Reported-by: should be immediately followed by Closes: with >>>> a URL to the report Note, those are warnings (and not an errors) on purpose and the text says "should" (and not "must"), so this is IMHO something in this case can be ignored, as Mario indicated in his reply. But I write for a different reason: this seems to become a regression that is annoying quite a few people (in 6.5 and 6.4.y afaics), so it would be good to get the fix merged to mainline rather sooner than later. Are these warnings and the mentioning of affected machines in the patch descriptions the only remaining problems, or is there anything else that needs to be addressed? Ciao, Thorsten (just back from vacation and catching up) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page. #regzbot poke
On Wed, Aug 23, 2023 at 02:15:10AM +0300, Jarkko Sakkinen wrote: > The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for > all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. On the > reported systems the TPM doesn't reply at bootup and returns back the > command code. This makes the TPM fail probe. > > Since only Microsoft Pluton is the only known combination of AMD CPU and > fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin > aware of this, print also info message to the klog. > > Cc: stable@vger.kernel.org > Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") > Reported-by: Todd Brandt <todd.e.brandt@intel.com> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > --- > v3: > * Forgot to amend config flags. > v2: > * CONFIG_X86 > * Removed "Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>" > * Removed "Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>" > --- > drivers/char/tpm/tpm_crb.c | 33 ++++++++------------------------- > 1 file changed, 8 insertions(+), 25 deletions(-) > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > index 65ff4d2fbe8d..ea085b14ab7c 100644 > --- a/drivers/char/tpm/tpm_crb.c > +++ b/drivers/char/tpm/tpm_crb.c > @@ -463,28 +463,6 @@ static bool crb_req_canceled(struct tpm_chip *chip, u8 status) > return (cancel & CRB_CANCEL_INVOKE) == CRB_CANCEL_INVOKE; > } > > -static int crb_check_flags(struct tpm_chip *chip) > -{ > - u32 val; > - int ret; > - > - ret = crb_request_locality(chip, 0); > - if (ret) > - return ret; > - > - ret = tpm2_get_tpm_pt(chip, TPM2_PT_MANUFACTURER, &val, NULL); > - if (ret) > - goto release; > - > - if (val == 0x414D4400U /* AMD */) > - chip->flags |= TPM_CHIP_FLAG_HWRNG_DISABLED; > - > -release: > - crb_relinquish_locality(chip, 0); > - > - return ret; > -} > - > static const struct tpm_class_ops tpm_crb = { > .flags = TPM_OPS_AUTO_STARTUP, > .status = crb_status, > @@ -827,9 +805,14 @@ static int crb_acpi_add(struct acpi_device *device) > if (rc) > goto out; > > - rc = crb_check_flags(chip); > - if (rc) > - goto out; > +#ifdef CONFIG_X86 > + /* A quirk for https://www.amd.com/en/support/kb/faq/pa-410 */ > + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD && > + priv->sm != ACPI_TPM2_COMMAND_BUFFER_WITH_PLUTON) { > + dev_info(dev, "Disabling hwrng\n"); > + chip->flags |= TPM_CHIP_FLAG_HWRNG_DISABLED; > + } > +#endif /* CONFIG_X86 */ > > rc = tpm_chip_register(chip); > > -- > 2.39.2 >
> On Aug 29, 2023, at 12:03 PM, Jerry Snitselaar <jsnitsel@redhat.com> wrote: > > On Wed, Aug 23, 2023 at 02:15:10AM +0300, Jarkko Sakkinen wrote: >> The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for >> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. On the >> reported systems the TPM doesn't reply at bootup and returns back the >> command code. This makes the TPM fail probe. >> >> Since only Microsoft Pluton is the only known combination of AMD CPU and >> fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin >> aware of this, print also info message to the klog. >> >> Cc: stable@vger.kernel.org >> Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") >> Reported-by: Todd Brandt <todd.e.brandt@intel.com> >> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 >> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> >> --- >> v3: >> * Forgot to amend config flags. >> v2: >> * CONFIG_X86 >> * Removed "Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>" >> * Removed "Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>" >> --- >> drivers/char/tpm/tpm_crb.c | 33 ++++++++------------------------- >> 1 file changed, 8 insertions(+), 25 deletions(-) >> > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> It looks like the Fedora folks are getting more reports of the issue.
[CCing Linus, as this triggered my "this is moving to slowly" threshold, as (a) the initial report was two weeks ago by now (b) a fix seems within reach for nearly as long (c) the problem seems to annoy quite a few people, as the culprit of this regression made it into 6.5 and was picked up for 6.1.y and 6.4.y (rightfully so I'd say, as it fixes an earlier regression)] On 29.08.23 10:38, Linux regression tracking (Thorsten Leemhuis) wrote: > On 28.08.23 02:35, Mario Limonciello wrote: >> On 8/27/2023 13:12, Jarkko Sakkinen wrote: >>> On Wed Aug 23, 2023 at 9:58 PM EEST, Mario Limonciello wrote: >>>> On 8/23/2023 12:40, Jarkko Sakkinen wrote: >>>>> On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote: >>>>>> Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen: >>>>>>> The vendor check introduced by commit 554b841d4703 ("tpm: Disable >>>>>>> RNG for >>>>>>> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. >>>>>>> On the >>>>>>> reported systems the TPM doesn't reply at bootup and returns back the >>>>>>> command code. This makes the TPM fail probe. >>>>>>> >>>>>>> Since only Microsoft Pluton is the only known combination of AMD >>>>>>> CPU and >>>>>>> fTPM from other vendor, disable hwrng otherwise. In order to make >>>>>>> sysadmin >>>>>>> aware of this, print also info message to the klog. >>>>>>> >>>>>>> Cc: stable@vger.kernel.org >>>>>>> Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") >>>>>>> Reported-by: Todd Brandt <todd.e.brandt@intel.com> >>>>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 >>>>>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> >>>>>> >>>>>> Mario’s patch also had the three reporters below listed: >>>>>> >>>>>> Reported-by: Patrick Steinhardt <ps@pks.im> >>>>>> Reported-by: Ronan Pigott <ronan@rjp.ie> >>>>>> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > [...] this seems to become a regression > that is annoying quite a few people (in 6.5 and 6.4.y afaics), so it > would be good to get the fix merged to mainline rather sooner than > later. Are these warnings and the mentioning of affected machines in the > patch descriptions the only remaining problems, or is there anything > else that needs to be addressed? Hmmm. Quite a bit progress to fix the issue was made in the first week after Todd's report; Jarkko apparently even applied the earlier patch from Mario to his master branch: https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=b1a62d41bdc1d15b0641759717e8c3651f0a810c But since then (aka in the past week) there was not much progress. Wondering what's up here -- and if both patches are needed or just one of them (I suspect it's the latter). Checked lore and noticed that Jarkko was not much active in kernel land during the past few days; happens, *no worries at all*. But still would be good if this could be resolved rather sooner that later. Just not sure how to achieve that. Mario, could you maybe pick this up in case Jarkko doesn't show up soon soon? From an earlier message in the thread it sounded like all that was missing was a slightly improved patch description? Or am I missing something? Ciao, Thorsten (who feels bad that he's putting pressure on people; sorry for that, but that duty comes with the "regression tracker" hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page. #regzbot poke
On Mon Aug 28, 2023 at 3:35 AM EEST, Mario Limonciello wrote: > On 8/27/2023 13:12, Jarkko Sakkinen wrote: > > On Wed Aug 23, 2023 at 9:58 PM EEST, Mario Limonciello wrote: > >> On 8/23/2023 12:40, Jarkko Sakkinen wrote: > >>> On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote: > >>>> Dear Jarkko, > >>>> > >>>> > >>>> Thank you for your patch. > >>>> > >>>> > >>>> Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen: > >>>>> The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for > >>>>> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. On the > >>>>> reported systems the TPM doesn't reply at bootup and returns back the > >>>>> command code. This makes the TPM fail probe. > >>>>> > >>>>> Since only Microsoft Pluton is the only known combination of AMD CPU and > >>>>> fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin > >>>>> aware of this, print also info message to the klog. > >>>>> > >>>>> Cc: stable@vger.kernel.org > >>>>> Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") > >>>>> Reported-by: Todd Brandt <todd.e.brandt@intel.com> > >>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 > >>>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > >>>> > >>>> Mario’s patch also had the three reporters below listed: > >>>> > >>>> Reported-by: Patrick Steinhardt <ps@pks.im> > >>>> Reported-by: Ronan Pigott <ronan@rjp.ie> > >>>> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > >>> > >>> The problem here is that checkpatch throws three warnings: > >>> > >>> WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > >>> #19: > >>> Reported-by: Patrick Steinhardt <ps@pks.im> > >>> Reported-by: Ronan Pigott <ronan@rjp.ie> > >>> > >>> WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > >>> #20: > >>> Reported-by: Ronan Pigott <ronan@rjp.ie> > >>> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > >>> > >>> WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > >>> #21: > >>> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > >>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > >>> > >> > >> FWIW I observed the same checkpatch warning when I submitted my version > >> of the patch. I figured it's better to ignore the warning and attribute > >> everyone who reported the issue affected them. > > > > OK so: > > > > 1. checkpatch.pl is part of the kernel process. > > 2. Bugzilla is not part of the kernel process. > > > > Why emphasis on 1? > > > > BR, Jarkko > > The reason I submitted it this way is because of this quote from the > documentation [1]. > > "Check your patches with the patch style checker prior to submission > (scripts/checkpatch.pl). Note, though, that the style checker should be > viewed as a guide, not as a replacement for human judgment. If your code > looks better with a violation then its probably best left alone." > > I wanted the patch to capture and attribute all those that reported it > not just the "first one". Like I said previously, it's better to have a > collection of people to ping to notify if something needs to be reverted. > > [1] > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#style-check-your-changes Please denote also that kernel bugzilla is not mentioned in the page that you put as a reference, and only reporter in the LKML has been Todd. BR, Jarkko
On Mon Sep 4, 2023 at 9:00 PM EEST, Jarkko Sakkinen wrote: > On Mon Aug 28, 2023 at 3:35 AM EEST, Mario Limonciello wrote: > > On 8/27/2023 13:12, Jarkko Sakkinen wrote: > > > On Wed Aug 23, 2023 at 9:58 PM EEST, Mario Limonciello wrote: > > >> On 8/23/2023 12:40, Jarkko Sakkinen wrote: > > >>> On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote: > > >>>> Dear Jarkko, > > >>>> > > >>>> > > >>>> Thank you for your patch. > > >>>> > > >>>> > > >>>> Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen: > > >>>>> The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for > > >>>>> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. On the > > >>>>> reported systems the TPM doesn't reply at bootup and returns back the > > >>>>> command code. This makes the TPM fail probe. > > >>>>> > > >>>>> Since only Microsoft Pluton is the only known combination of AMD CPU and > > >>>>> fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin > > >>>>> aware of this, print also info message to the klog. > > >>>>> > > >>>>> Cc: stable@vger.kernel.org > > >>>>> Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") > > >>>>> Reported-by: Todd Brandt <todd.e.brandt@intel.com> > > >>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 > > >>>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > >>>> > > >>>> Mario’s patch also had the three reporters below listed: > > >>>> > > >>>> Reported-by: Patrick Steinhardt <ps@pks.im> > > >>>> Reported-by: Ronan Pigott <ronan@rjp.ie> > > >>>> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > >>> > > >>> The problem here is that checkpatch throws three warnings: > > >>> > > >>> WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > >>> #19: > > >>> Reported-by: Patrick Steinhardt <ps@pks.im> > > >>> Reported-by: Ronan Pigott <ronan@rjp.ie> > > >>> > > >>> WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > >>> #20: > > >>> Reported-by: Ronan Pigott <ronan@rjp.ie> > > >>> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > >>> > > >>> WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report > > >>> #21: > > >>> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > >>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > >>> > > >> > > >> FWIW I observed the same checkpatch warning when I submitted my version > > >> of the patch. I figured it's better to ignore the warning and attribute > > >> everyone who reported the issue affected them. > > > > > > OK so: > > > > > > 1. checkpatch.pl is part of the kernel process. > > > 2. Bugzilla is not part of the kernel process. > > > > > > Why emphasis on 1? > > > > > > BR, Jarkko > > > > The reason I submitted it this way is because of this quote from the > > documentation [1]. > > > > "Check your patches with the patch style checker prior to submission > > (scripts/checkpatch.pl). Note, though, that the style checker should be > > viewed as a guide, not as a replacement for human judgment. If your code > > looks better with a violation then its probably best left alone." > > > > I wanted the patch to capture and attribute all those that reported it > > not just the "first one". Like I said previously, it's better to have a > > collection of people to ping to notify if something needs to be reverted. > > > > [1] > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#style-check-your-changes > > Please denote also that kernel bugzilla is not mentioned in the page > that you put as a reference, and only reporter in the LKML has been > Todd. Also the bugzilla is ambiguous because in this thread I get a picture that any possible commenter is a reporter, and at the same time bugzilla has a *specific field* for a reporter. How do the comments and the field for the reporter relate, and how they should be interpreted? BR, Jarkko
On Wed Aug 30, 2023 at 9:25 PM EEST, Jerry Snitselaar wrote: > > > > On Aug 29, 2023, at 12:03 PM, Jerry Snitselaar <jsnitsel@redhat.com> wrote: > > > > On Wed, Aug 23, 2023 at 02:15:10AM +0300, Jarkko Sakkinen wrote: > >> The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for > >> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. On the > >> reported systems the TPM doesn't reply at bootup and returns back the > >> command code. This makes the TPM fail probe. > >> > >> Since only Microsoft Pluton is the only known combination of AMD CPU and > >> fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin > >> aware of this, print also info message to the klog. > >> > >> Cc: stable@vger.kernel.org > >> Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") > >> Reported-by: Todd Brandt <todd.e.brandt@intel.com> > >> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 > >> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > >> --- > >> v3: > >> * Forgot to amend config flags. > >> v2: > >> * CONFIG_X86 > >> * Removed "Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>" > >> * Removed "Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>" > >> --- > >> drivers/char/tpm/tpm_crb.c | 33 ++++++++------------------------- > >> 1 file changed, 8 insertions(+), 25 deletions(-) > >> > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > It looks like the Fedora folks are getting more reports of the issue. https://lore.kernel.org/linux-integrity/20230904202512.29825-1-jarkko@kernel.org/T/#u I have all the possible reported-by's. I still don't fully understand kernel bugzilla's role. I don't oppose having it but e.g. for me reporter has been traditionally someone who reports the bug in LKML, not in bugzilla. Also the ambiguity of the whole discussion has been over the top. E.g. why bugzilla even has a field for reporter if that is not *the* reporter at least according to this discussion? And in the case of this bug, the reporter in bugzilla was the same exact person who mailed about it to LKML. I'm actually cool with almost any policy, as long as there is at least some policy in existence. Pretty confusing exercise overally, and very time consuming for a maintainer. BR, Jarkko
On Fri Sep 1, 2023 at 11:49 AM EEST, Thorsten Leemhuis wrote: > [CCing Linus, as this triggered my "this is moving to slowly" threshold, > as (a) the initial report was two weeks ago by now (b) a fix seems > within reach for nearly as long (c) the problem seems to annoy quite a > few people, as the culprit of this regression made it into 6.5 and was > picked up for 6.1.y and 6.4.y (rightfully so I'd say, as it fixes an > earlier regression)] > > On 29.08.23 10:38, Linux regression tracking (Thorsten Leemhuis) wrote: > > On 28.08.23 02:35, Mario Limonciello wrote: > >> On 8/27/2023 13:12, Jarkko Sakkinen wrote: > >>> On Wed Aug 23, 2023 at 9:58 PM EEST, Mario Limonciello wrote: > >>>> On 8/23/2023 12:40, Jarkko Sakkinen wrote: > >>>>> On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote: > >>>>>> Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen: > >>>>>>> The vendor check introduced by commit 554b841d4703 ("tpm: Disable > >>>>>>> RNG for > >>>>>>> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. > >>>>>>> On the > >>>>>>> reported systems the TPM doesn't reply at bootup and returns back the > >>>>>>> command code. This makes the TPM fail probe. > >>>>>>> > >>>>>>> Since only Microsoft Pluton is the only known combination of AMD > >>>>>>> CPU and > >>>>>>> fTPM from other vendor, disable hwrng otherwise. In order to make > >>>>>>> sysadmin > >>>>>>> aware of this, print also info message to the klog. > >>>>>>> > >>>>>>> Cc: stable@vger.kernel.org > >>>>>>> Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") > >>>>>>> Reported-by: Todd Brandt <todd.e.brandt@intel.com> > >>>>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 > >>>>>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > >>>>>> > >>>>>> Mario’s patch also had the three reporters below listed: > >>>>>> > >>>>>> Reported-by: Patrick Steinhardt <ps@pks.im> > >>>>>> Reported-by: Ronan Pigott <ronan@rjp.ie> > >>>>>> Reported-by: Raymond Jay Golo <rjgolo@gmail.com> > > > > [...] this seems to become a regression > > that is annoying quite a few people (in 6.5 and 6.4.y afaics), so it > > would be good to get the fix merged to mainline rather sooner than > > later. Are these warnings and the mentioning of affected machines in the > > patch descriptions the only remaining problems, or is there anything > > else that needs to be addressed? > > Hmmm. Quite a bit progress to fix the issue was made in the first week > after Todd's report; Jarkko apparently even applied the earlier patch > from Mario to his master branch: > https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=b1a62d41bdc1d15b0641759717e8c3651f0a810c > But since then (aka in the past week) there was not much progress. > > Wondering what's up here -- and if both patches are needed or just one > of them (I suspect it's the latter). > > Checked lore and noticed that Jarkko was not much active in kernel land > during the past few days; happens, *no worries at all*. But still would > be good if this could be resolved rather sooner that later. Just not > sure how to achieve that. > > Mario, could you maybe pick this up in case Jarkko doesn't show up soon > soon? From an earlier message in the thread it sounded like all that was > missing was a slightly improved patch description? Or am I missing > something? > > Ciao, Thorsten (who feels bad that he's putting pressure on people; > sorry for that, but that duty comes with the "regression tracker" hat) > -- > Everything you wanna know about Linux kernel regression tracking: > https://linux-regtracking.leemhuis.info/about/#tldr > If I did something stupid, please tell me, as explained on that page. > > #regzbot poke Could it be possible to extend the actual kernel documentation to give at least some guidelines how a maintainer should deal with the bugzilla? I do not oppose having it but IMHO at least the basics should be in the actualy process documentation. You can even put a link to this URL to that. I posted a PR today with the hopefully final fix: https://lore.kernel.org/linux-integrity/20230904202512.29825-1-jarkko@kernel.org/T/#u BR, Jarkko
On 05.09.23 00:32, Jarkko Sakkinen wrote: > On Fri Sep 1, 2023 at 11:49 AM EEST, Thorsten Leemhuis wrote: >> On 29.08.23 10:38, Linux regression tracking (Thorsten Leemhuis) wrote: >>> On 28.08.23 02:35, Mario Limonciello wrote: >>>> On 8/27/2023 13:12, Jarkko Sakkinen wrote: >>>>> On Wed Aug 23, 2023 at 9:58 PM EEST, Mario Limonciello wrote: >>>>>> On 8/23/2023 12:40, Jarkko Sakkinen wrote: >>>>>>> On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote: >>>>>>>> Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen: >>>>>>>>> The vendor check introduced by commit 554b841d4703 ("tpm: Disable >>>>>>>>> RNG for >>>>>>>>> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. >>>>>>>>> On the >>>>>>>>> reported systems the TPM doesn't reply at bootup and returns back the >>>>>>>>> command code. This makes the TPM fail probe. > [...] >> Hmmm. Quite a bit progress to fix the issue was made in the first week >> after Todd's report; Jarkko apparently even applied the earlier patch >> from Mario to his master branch: >> https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=b1a62d41bdc1d15b0641759717e8c3651f0a810c >> But since then (aka in the past week) there was not much progress. Jarkko, many thx for picking this up and submitting it to Linus, much appreciated. Sorry again for prodding things, but I felt I had to. Hope you didn't mind too much. > Could it be possible to extend the actual kernel documentation > to give at least some guidelines how a maintainer should deal > with the bugzilla? I guess it's best if that is done by somebody that cares about bugzilla (I don't fall into that group[1]) and knows the official status. But FWIW, I wonder what you actually want to see documented. From https://lore.kernel.org/all/CVAC8VQPD3PK.1CBS5QTWDSS2C@suppilovahvero/ it sounds like you had trouble with Link:/Closes: tag and Reported-by. From what I can see I don't think bugzilla.kernel.org needs special documentation in that area: * just use Link:/Closes: to reports to public reports that might be helpful later in case somebody wants to look at the backstory of a commit, wherever those reports may be (lore, bugzilla.kernel.org, https://gitlab.freedesktop.org/drm/intel/-/issues, https://github.com/thesofproject/linux/issues, ...) * use Reported-by: to give credit to anyone that deserves it, as it is a nice way to say thx while motivate people to help again in the future. That usually will include the initial reporter, but might also include people that replied to a report from somebody else and helped perceptible with debugging or fixing. Ciao, Thorsten [1] I only sometimes help people that report regressions to bugzilla.kernel.org that otherwise would likely would fall through the cracks (among others because many reports are never forwarded to the proper developers otherwise).
On Tue Sep 5, 2023 at 3:01 PM EEST, Thorsten Leemhuis wrote: > On 05.09.23 00:32, Jarkko Sakkinen wrote: > > On Fri Sep 1, 2023 at 11:49 AM EEST, Thorsten Leemhuis wrote: > >> On 29.08.23 10:38, Linux regression tracking (Thorsten Leemhuis) wrote: > >>> On 28.08.23 02:35, Mario Limonciello wrote: > >>>> On 8/27/2023 13:12, Jarkko Sakkinen wrote: > >>>>> On Wed Aug 23, 2023 at 9:58 PM EEST, Mario Limonciello wrote: > >>>>>> On 8/23/2023 12:40, Jarkko Sakkinen wrote: > >>>>>>> On Wed Aug 23, 2023 at 11:23 AM EEST, Paul Menzel wrote: > >>>>>>>> Am 23.08.23 um 01:15 schrieb Jarkko Sakkinen: > >>>>>>>>> The vendor check introduced by commit 554b841d4703 ("tpm: Disable > >>>>>>>>> RNG for > >>>>>>>>> all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. > >>>>>>>>> On the > >>>>>>>>> reported systems the TPM doesn't reply at bootup and returns back the > >>>>>>>>> command code. This makes the TPM fail probe. > > [...] > >> Hmmm. Quite a bit progress to fix the issue was made in the first week > >> after Todd's report; Jarkko apparently even applied the earlier patch > >> from Mario to his master branch: > >> https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=b1a62d41bdc1d15b0641759717e8c3651f0a810c > >> But since then (aka in the past week) there was not much progress. > > Jarkko, many thx for picking this up and submitting it to Linus, much > appreciated. Sorry again for prodding things, but I felt I had to. Hope > you didn't mind too much. > > > Could it be possible to extend the actual kernel documentation > > to give at least some guidelines how a maintainer should deal > > with the bugzilla? > > I guess it's best if that is done by somebody that cares about bugzilla > (I don't fall into that group[1]) and knows the official status. > > But FWIW, I wonder what you actually want to see documented. From > https://lore.kernel.org/all/CVAC8VQPD3PK.1CBS5QTWDSS2C@suppilovahvero/ > it sounds like you had trouble with Link:/Closes: tag and Reported-by. > From what I can see I don't think bugzilla.kernel.org needs special > documentation in that area: > > * just use Link:/Closes: to reports to public reports that might be > helpful later in case somebody wants to look at the backstory of a > commit, wherever those reports may be (lore, bugzilla.kernel.org, > https://gitlab.freedesktop.org/drm/intel/-/issues, > https://github.com/thesofproject/linux/issues, ...) > > * use Reported-by: to give credit to anyone that deserves it, as it is > a nice way to say thx while motivate people to help again in the future. > That usually will include the initial reporter, but might also include > people that replied to a report from somebody else and helped > perceptible with debugging or fixing. I *kind of* agree with this but checkpatch.pl disagrees with this :-/ And it is an actual issue that bugzilla is hosted in kernel.org domain, and at the same time it is undocumented process. AFAIK anything that is not part of the process can be ignored in the process so *theoretically* anything put to kernel bugzilla ca be ignored. This is how it is with e.g. patchwork - some people use it, some people don't. Personally I think bugzilla, being user approachable system, should be better defined but *theoretically*, at least by the process, it can be fully ignored. This is where the main source of confusion inherits from, when you put your maintainer hat on. > Ciao, Thorsten > > [1] I only sometimes help people that report regressions to > bugzilla.kernel.org that otherwise would likely would fall through the > cracks (among others because many reports are never forwarded to the > proper developers otherwise). BR, Jarkko
On Mon Sep 11, 2023 at 1:40 PM EEST, Jarkko Sakkinen wrote: > Personally I think bugzilla, being user approachable system, should > be better defined but *theoretically*, at least by the process, it > can be fully ignored. I.e. I don't think it should be ignored :-) </disclaimer> BR, Jarkko
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index 65ff4d2fbe8d..ea085b14ab7c 100644 --- a/drivers/char/tpm/tpm_crb.c +++ b/drivers/char/tpm/tpm_crb.c @@ -463,28 +463,6 @@ static bool crb_req_canceled(struct tpm_chip *chip, u8 status) return (cancel & CRB_CANCEL_INVOKE) == CRB_CANCEL_INVOKE; } -static int crb_check_flags(struct tpm_chip *chip) -{ - u32 val; - int ret; - - ret = crb_request_locality(chip, 0); - if (ret) - return ret; - - ret = tpm2_get_tpm_pt(chip, TPM2_PT_MANUFACTURER, &val, NULL); - if (ret) - goto release; - - if (val == 0x414D4400U /* AMD */) - chip->flags |= TPM_CHIP_FLAG_HWRNG_DISABLED; - -release: - crb_relinquish_locality(chip, 0); - - return ret; -} - static const struct tpm_class_ops tpm_crb = { .flags = TPM_OPS_AUTO_STARTUP, .status = crb_status, @@ -827,9 +805,14 @@ static int crb_acpi_add(struct acpi_device *device) if (rc) goto out; - rc = crb_check_flags(chip); - if (rc) - goto out; +#ifdef CONFIG_X86 + /* A quirk for https://www.amd.com/en/support/kb/faq/pa-410 */ + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD && + priv->sm != ACPI_TPM2_COMMAND_BUFFER_WITH_PLUTON) { + dev_info(dev, "Disabling hwrng\n"); + chip->flags |= TPM_CHIP_FLAG_HWRNG_DISABLED; + } +#endif /* CONFIG_X86 */ rc = tpm_chip_register(chip);
The vendor check introduced by commit 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") doesn't work properly on a number of Intel fTPMs. On the reported systems the TPM doesn't reply at bootup and returns back the command code. This makes the TPM fail probe. Since only Microsoft Pluton is the only known combination of AMD CPU and fTPM from other vendor, disable hwrng otherwise. In order to make sysadmin aware of this, print also info message to the klog. Cc: stable@vger.kernel.org Fixes: 554b841d4703 ("tpm: Disable RNG for all AMD fTPMs") Reported-by: Todd Brandt <todd.e.brandt@intel.com> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217804 Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- v3: * Forgot to amend config flags. v2: * CONFIG_X86 * Removed "Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>" * Removed "Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>" --- drivers/char/tpm/tpm_crb.c | 33 ++++++++------------------------- 1 file changed, 8 insertions(+), 25 deletions(-)