| Message ID | 1412712189-1234-15-git-send-email-christophe-h.ricard@st.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Oct 07, 2014 at 10:03:07PM +0200, Christophe Ricard wrote: > When sending data in tpm_stm_i2c_send, each loop iteration send buf. > Send buf + i instead as the goal of this for loop is to send a number > of byte from buf that fit in burstcnt. Once those byte are sent, we are > supposed to send the next ones. So, this driver never really worked? I'm guessing sending a larger command (take ownership, for example) will exceed the burst count and just blow up? This should be marked for stable (please see Documentation/stable_kernel_rules.txt) Also, please make it the first patch in your series so it applies cleanly to older kernels. Reviewed-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com> > drivers/char/tpm/tpm_i2c_stm_st33.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c > index 8d32ade..de9f12e 100644 > +++ b/drivers/char/tpm/tpm_i2c_stm_st33.c > @@ -480,7 +480,7 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf, > if (burstcnt < 0) > return burstcnt; > size = min_t(int, len - i - 1, burstcnt); > - r = i2c_write_data(tpm_dev, TPM_DATA_FIFO, buf, size); > + r = i2c_write_data(tpm_dev, TPM_DATA_FIFO, buf + i, size); > if (r < 0) > goto out_err; > ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Hi Jason, In fact this driver worked because the TPM burstcnt was always big enought to support all TPM commands. In other term burstcnt > len. No problem for making making this patch as number 1 in this series. Best Regards Christophe On 08/10/2014 00:04, Jason Gunthorpe wrote: > On Tue, Oct 07, 2014 at 10:03:07PM +0200, Christophe Ricard wrote: >> When sending data in tpm_stm_i2c_send, each loop iteration send buf. >> Send buf + i instead as the goal of this for loop is to send a number >> of byte from buf that fit in burstcnt. Once those byte are sent, we are >> supposed to send the next ones. > So, this driver never really worked? I'm guessing sending a larger > command (take ownership, for example) will exceed the burst count and > just blow up? > > This should be marked for stable (please see > Documentation/stable_kernel_rules.txt) > > Also, please make it the first patch in your series so it applies > cleanly to older kernels. > > Reviewed-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > >> Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com> >> drivers/char/tpm/tpm_i2c_stm_st33.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c >> index 8d32ade..de9f12e 100644 >> +++ b/drivers/char/tpm/tpm_i2c_stm_st33.c >> @@ -480,7 +480,7 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf, >> if (burstcnt < 0) >> return burstcnt; >> size = min_t(int, len - i - 1, burstcnt); >> - r = i2c_write_data(tpm_dev, TPM_DATA_FIFO, buf, size); >> + r = i2c_write_data(tpm_dev, TPM_DATA_FIFO, buf + i, size); >> if (r < 0) >> goto out_err; >> ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
I would just add an additional comment to my previous message. After reviewing Documentation/stable_kernel_rules.txt, it looks like it is *not* necessary to send this patch marked as stable because of the following rule: "- It must fix a real bug that bothers people (not a, "This could be a problem..." type thing)." As argued previously this patch is more an algorithm fix luckily never seen with the current ST33 I2C TPM devices. I looks like a "This could be a problem..." Do you agree with this ? Best Regards Christophe 2014-10-08 7:31 GMT+02:00 Christophe RICARD <christophe.ricard@gmail.com>: > Hi Jason, > > In fact this driver worked because the TPM burstcnt was always big enought > to support all TPM commands. > In other term burstcnt > len. > > No problem for making making this patch as number 1 in this series. > > Best Regards > Christophe > > On 08/10/2014 00:04, Jason Gunthorpe wrote: > >> On Tue, Oct 07, 2014 at 10:03:07PM +0200, Christophe Ricard wrote: >> >>> When sending data in tpm_stm_i2c_send, each loop iteration send buf. >>> Send buf + i instead as the goal of this for loop is to send a number >>> of byte from buf that fit in burstcnt. Once those byte are sent, we are >>> supposed to send the next ones. >>> >> So, this driver never really worked? I'm guessing sending a larger >> command (take ownership, for example) will exceed the burst count and >> just blow up? >> >> This should be marked for stable (please see >> Documentation/stable_kernel_rules.txt) >> >> Also, please make it the first patch in your series so it applies >> cleanly to older kernels. >> >> Reviewed-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> >> >> Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com> >>> drivers/char/tpm/tpm_i2c_stm_st33.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c >>> b/drivers/char/tpm/tpm_i2c_stm_st33.c >>> index 8d32ade..de9f12e 100644 >>> +++ b/drivers/char/tpm/tpm_i2c_stm_st33.c >>> @@ -480,7 +480,7 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, >>> unsigned char *buf, >>> if (burstcnt < 0) >>> return burstcnt; >>> size = min_t(int, len - i - 1, burstcnt); >>> - r = i2c_write_data(tpm_dev, TPM_DATA_FIFO, buf, size); >>> + r = i2c_write_data(tpm_dev, TPM_DATA_FIFO, buf + i, >>> size); >>> if (r < 0) >>> goto out_err; >>> >>> >> > ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
On Wed, Oct 08, 2014 at 09:38:39AM +0200, Christophe Ricard wrote: > I would just add an additional comment to my previous message. > After reviewing Documentation/stable_kernel_rules.txt, it looks like it > is *not* necessary to send this patch marked as stable because of the > following rule: > "- It must fix a real bug that bothers people (not a, "This could be a > problem..." type thing)." > As argued previously this patch is more an algorithm fix luckily never > seen with the current ST33 I2C TPM devices. > I looks like a "This could be a problem..." > Do you agree with this ? Yes, please clarify your patch description with something like: 'This never happens in practice because the burst count is XXX bytes and the largest TPM command is YYY bytes.' Jason ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c index 8d32ade..de9f12e 100644 --- a/drivers/char/tpm/tpm_i2c_stm_st33.c +++ b/drivers/char/tpm/tpm_i2c_stm_st33.c @@ -480,7 +480,7 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf, if (burstcnt < 0) return burstcnt; size = min_t(int, len - i - 1, burstcnt); - r = i2c_write_data(tpm_dev, TPM_DATA_FIFO, buf, size); + r = i2c_write_data(tpm_dev, TPM_DATA_FIFO, buf + i, size); if (r < 0) goto out_err;
When sending data in tpm_stm_i2c_send, each loop iteration send buf. Send buf + i instead as the goal of this for loop is to send a number of byte from buf that fit in burstcnt. Once those byte are sent, we are supposed to send the next ones. Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com> --- drivers/char/tpm/tpm_i2c_stm_st33.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)