From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> To: Tomas Winkler <tomas.winkler@intel.com> Cc: Jason Gunthorpe <jgg@ziepe.ca>, Alexander Usyskin <alexander.usyskin@intel.com>, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2 v3] tpm: cmd_ready command can be issued only after granting locality Date: Mon, 19 Feb 2018 13:27:00 +0200 [thread overview] Message-ID: <20180219112700.yfoywzegqzrpynlk@linux.intel.com> (raw) In-Reply-To: <20180214134319.4400-2-tomas.winkler@intel.com> On Wed, Feb 14, 2018 at 03:43:18PM +0200, Tomas Winkler wrote: > if (need_locality && chip->ops->relinquish_locality) { > - chip->ops->relinquish_locality(chip, chip->locality); > + /* this coud be on error path, don't override error code */ > + int l_rc = chip->ops->relinquish_locality(chip, chip->locality); All local variable declarations must be in the beginning of the function. > + > + if (l_rc) { > + dev_err(&chip->dev, "%s: relinquish_locality: error %d\n", > + __func__, l_rc); > + rc = l_rc; > + } Your comment about not overriding error code is incorrect. The value of 'rc' should be never overridden, which kind of supports to "just print" behavior that we had for a locality error. Is your fix somehow dependent on changing relinquish_locality() behavior? If not, please remove this change. If you want to contribute such behavioral change, you should make a separate patch of it. Now it's like a trojan horse bundled inside a bug fix. /Jarkko
WARNING: multiple messages have this Message-ID (diff)
From: jarkko.sakkinen@linux.intel.com (Jarkko Sakkinen) To: linux-security-module@vger.kernel.org Subject: [PATCH 1/2 v3] tpm: cmd_ready command can be issued only after granting locality Date: Mon, 19 Feb 2018 13:27:00 +0200 [thread overview] Message-ID: <20180219112700.yfoywzegqzrpynlk@linux.intel.com> (raw) In-Reply-To: <20180214134319.4400-2-tomas.winkler@intel.com> On Wed, Feb 14, 2018 at 03:43:18PM +0200, Tomas Winkler wrote: > if (need_locality && chip->ops->relinquish_locality) { > - chip->ops->relinquish_locality(chip, chip->locality); > + /* this coud be on error path, don't override error code */ > + int l_rc = chip->ops->relinquish_locality(chip, chip->locality); All local variable declarations must be in the beginning of the function. > + > + if (l_rc) { > + dev_err(&chip->dev, "%s: relinquish_locality: error %d\n", > + __func__, l_rc); > + rc = l_rc; > + } Your comment about not overriding error code is incorrect. The value of 'rc' should be never overridden, which kind of supports to "just print" behavior that we had for a locality error. Is your fix somehow dependent on changing relinquish_locality() behavior? If not, please remove this change. If you want to contribute such behavioral change, you should make a separate patch of it. Now it's like a trojan horse bundled inside a bug fix. /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2018-02-19 11:27 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-02-14 13:43 [PATCH 0/2 v3] tpm: fix locality and power saving handling Tomas Winkler 2018-02-14 13:43 ` Tomas Winkler 2018-02-14 13:43 ` [PATCH 1/2 v3] tpm: cmd_ready command can be issued only after granting locality Tomas Winkler 2018-02-14 13:43 ` Tomas Winkler 2018-02-19 11:27 ` Jarkko Sakkinen [this message] 2018-02-19 11:27 ` Jarkko Sakkinen 2018-02-19 11:43 ` Winkler, Tomas 2018-02-19 11:43 ` Winkler, Tomas 2018-02-20 14:57 ` Jarkko Sakkinen 2018-02-20 14:57 ` Jarkko Sakkinen 2018-02-20 20:26 ` Winkler, Tomas 2018-02-20 20:26 ` Winkler, Tomas 2018-02-20 23:03 ` Jarkko Sakkinen 2018-02-20 23:03 ` Jarkko Sakkinen 2018-02-20 14:12 ` Jarkko Sakkinen 2018-02-20 14:12 ` Jarkko Sakkinen 2018-02-14 13:43 ` [PATCH 2/2 v3] tpm: separate cmd_ready/go_idle from runtime_pm Tomas Winkler 2018-02-14 13:43 ` Tomas Winkler 2018-02-19 11:52 ` Jarkko Sakkinen 2018-02-19 11:52 ` Jarkko Sakkinen 2018-02-20 14:13 ` Jarkko Sakkinen 2018-02-20 14:13 ` Jarkko Sakkinen 2018-03-05 18:08 ` Jarkko Sakkinen 2018-03-05 18:08 ` Jarkko Sakkinen 2018-03-23 8:12 ` Jarkko Sakkinen 2018-03-23 8:12 ` Jarkko Sakkinen
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180219112700.yfoywzegqzrpynlk@linux.intel.com \ --to=jarkko.sakkinen@linux.intel.com \ --cc=alexander.usyskin@intel.com \ --cc=jgg@ziepe.ca \ --cc=linux-integrity@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-security-module@vger.kernel.org \ --cc=tomas.winkler@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.