| Message ID | 20200528151912.1.Id689a39ce8d1ec6f29f4287277ad977ff4f57d7d@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | tpm_tis_spi: Don't send anything during flow control | expand |
Dear Douglas, Thank you for the patch. Am 29.05.20 um 00:19 schrieb Douglas Anderson: > During flow control we are just reading from the TPM, yet our spi_xfer > has the tx_buf and rx_buf both non-NULL which means we're requesting a > full duplex transfer. > > SPI is always somewhat of a full duplex protocol anyway and in theory > the other side shouldn't really be looking at what we're sending it > during flow control, but it's still a bit ugly to be sending some > "random" data when we shouldn't. > > The default tpm_tis_spi_flow_control() tries to address this by > setting 'phy->iobuf[0] = 0'. This partially avoids the problem of > sending "random" data, but since our tx_buf and rx_buf both point to > the same place I believe there is the potential of us sending the > TPM's previous byte back to it if we hit the retry loop. > > Another flow control implementation, cr50_spi_flow_control(), doesn't > address this at all. > > Let's clean this up and just make the tx_buf NULL before we call > flow_control(). Not only does this ensure that we're not sending any > "random" bytes but it also possibly could make the SPI controller > behave in a slightly more optimal way. > > NOTE: no actual observed problems are fixed by this patch--it's was > just made based on code inspection. s/it's was/it was/ Were you able to test this? Maybe in the “Chromebook QA arsenal”? Are you already running it in production on Google Chrome OS devices? > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/char/tpm/tpm_tis_spi_main.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c > index d96755935529..8d2c581a93c6 100644 > --- a/drivers/char/tpm/tpm_tis_spi_main.c > +++ b/drivers/char/tpm/tpm_tis_spi_main.c > @@ -53,8 +53,6 @@ static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy, > > if ((phy->iobuf[3] & 0x01) == 0) { > // handle SPI wait states > - phy->iobuf[0] = 0; > - > for (i = 0; i < TPM_RETRY; i++) { > spi_xfer->len = 1; > spi_message_init(&m); > @@ -104,6 +102,8 @@ int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len, > if (ret < 0) > goto exit; > > + /* Flow control transfers are receive only */ > + spi_xfer.tx_buf = NULL; > ret = phy->flow_control(phy, &spi_xfer); > if (ret < 0) > goto exit; > @@ -113,9 +113,8 @@ int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len, > spi_xfer.delay.value = 5; > spi_xfer.delay.unit = SPI_DELAY_UNIT_USECS; > > - if (in) { > - spi_xfer.tx_buf = NULL; > - } else if (out) { > + if (out) { > + spi_xfer.tx_buf = phy->iobuf; > spi_xfer.rx_buf = NULL; > memcpy(phy->iobuf, out, transfer_len); > out += transfer_len; Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de> Kind regards, Paul
Hi, On Fri, May 29, 2020 at 1:33 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > Dear Douglas, > > > Thank you for the patch. > > Am 29.05.20 um 00:19 schrieb Douglas Anderson: > > During flow control we are just reading from the TPM, yet our spi_xfer > > has the tx_buf and rx_buf both non-NULL which means we're requesting a > > full duplex transfer. > > > > SPI is always somewhat of a full duplex protocol anyway and in theory > > the other side shouldn't really be looking at what we're sending it > > during flow control, but it's still a bit ugly to be sending some > > "random" data when we shouldn't. > > > > The default tpm_tis_spi_flow_control() tries to address this by > > setting 'phy->iobuf[0] = 0'. This partially avoids the problem of > > sending "random" data, but since our tx_buf and rx_buf both point to > > the same place I believe there is the potential of us sending the > > TPM's previous byte back to it if we hit the retry loop. > > > > Another flow control implementation, cr50_spi_flow_control(), doesn't > > address this at all. > > > > Let's clean this up and just make the tx_buf NULL before we call > > flow_control(). Not only does this ensure that we're not sending any > > "random" bytes but it also possibly could make the SPI controller > > behave in a slightly more optimal way. > > > > NOTE: no actual observed problems are fixed by this patch--it's was > > just made based on code inspection. > > s/it's was/it was/ I'll assume this is easier to get fixed up when applying the patch, but happy to send a v2 if requested. > Were you able to test this? Maybe in the “Chromebook QA arsenal”? Are > you already running it in production on Google Chrome OS devices? I've tested it locally but not much more than that. As far as I can tell it doesn't break anything or fix anything but still seems like a good change to make. ;-) Generally Chrome OS tries to work with an "upstream first" philosophy. That has lots of benefits, but one downside is it means that patches get sent upstream before they've actually landed anywhere. That being said, even if we landed this today we'd get limited testing because of the Chrome OS kernel trees only the Chrome OS 5.4 tree uses the upstream driver for Cr50 (the one SPI-connected security chip that Chromebooks use). Cr50 originally did not get developed as "upstream first", so all the old codebases have the old downstream driver. This problem doesn't affect the old downstream driver which had its own "struct spi_transfer". If anything it makes the upstream code behave more like the old downstream driver. If you want to see the old downstream driver: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.19/drivers/char/tpm/cr50_spi.c#172 Another note is that I don't have access to any SPI-connected chips that are _not_ Cr50. That means that half of this change is totally untested but looks sane. If anyone can test it that'd be great. :-) > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > drivers/char/tpm/tpm_tis_spi_main.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c > > index d96755935529..8d2c581a93c6 100644 > > --- a/drivers/char/tpm/tpm_tis_spi_main.c > > +++ b/drivers/char/tpm/tpm_tis_spi_main.c > > @@ -53,8 +53,6 @@ static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy, > > > > if ((phy->iobuf[3] & 0x01) == 0) { > > // handle SPI wait states > > - phy->iobuf[0] = 0; > > - > > for (i = 0; i < TPM_RETRY; i++) { > > spi_xfer->len = 1; > > spi_message_init(&m); > > @@ -104,6 +102,8 @@ int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len, > > if (ret < 0) > > goto exit; > > > > + /* Flow control transfers are receive only */ > > + spi_xfer.tx_buf = NULL; > > ret = phy->flow_control(phy, &spi_xfer); > > if (ret < 0) > > goto exit; > > @@ -113,9 +113,8 @@ int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len, > > spi_xfer.delay.value = 5; > > spi_xfer.delay.unit = SPI_DELAY_UNIT_USECS; > > > > - if (in) { > > - spi_xfer.tx_buf = NULL; > > - } else if (out) { > > + if (out) { > > + spi_xfer.tx_buf = phy->iobuf; > > spi_xfer.rx_buf = NULL; > > memcpy(phy->iobuf, out, transfer_len); > > out += transfer_len; > > Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de> Thanks for reviewing! :-) -Doug
On Thu, May 28, 2020 at 03:19:30PM -0700, Douglas Anderson wrote: > During flow control we are just reading from the TPM, yet our spi_xfer > has the tx_buf and rx_buf both non-NULL which means we're requesting a > full duplex transfer. > > SPI is always somewhat of a full duplex protocol anyway and in theory > the other side shouldn't really be looking at what we're sending it > during flow control, but it's still a bit ugly to be sending some > "random" data when we shouldn't. > > The default tpm_tis_spi_flow_control() tries to address this by > setting 'phy->iobuf[0] = 0'. This partially avoids the problem of > sending "random" data, but since our tx_buf and rx_buf both point to > the same place I believe there is the potential of us sending the > TPM's previous byte back to it if we hit the retry loop. > > Another flow control implementation, cr50_spi_flow_control(), doesn't > address this at all. > > Let's clean this up and just make the tx_buf NULL before we call > flow_control(). Not only does this ensure that we're not sending any > "random" bytes but it also possibly could make the SPI controller > behave in a slightly more optimal way. > > NOTE: no actual observed problems are fixed by this patch--it's was > just made based on code inspection. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/char/tpm/tpm_tis_spi_main.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c > index d96755935529..8d2c581a93c6 100644 > --- a/drivers/char/tpm/tpm_tis_spi_main.c > +++ b/drivers/char/tpm/tpm_tis_spi_main.c > @@ -53,8 +53,6 @@ static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy, > > if ((phy->iobuf[3] & 0x01) == 0) { > // handle SPI wait states > - phy->iobuf[0] = 0; > - Why this should be removed? /Jarkko
Hi, On Sun, May 31, 2020 at 6:47 PM Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote: > > On Thu, May 28, 2020 at 03:19:30PM -0700, Douglas Anderson wrote: > > During flow control we are just reading from the TPM, yet our spi_xfer > > has the tx_buf and rx_buf both non-NULL which means we're requesting a > > full duplex transfer. > > > > SPI is always somewhat of a full duplex protocol anyway and in theory > > the other side shouldn't really be looking at what we're sending it > > during flow control, but it's still a bit ugly to be sending some > > "random" data when we shouldn't. > > > > The default tpm_tis_spi_flow_control() tries to address this by > > setting 'phy->iobuf[0] = 0'. This partially avoids the problem of > > sending "random" data, but since our tx_buf and rx_buf both point to > > the same place I believe there is the potential of us sending the > > TPM's previous byte back to it if we hit the retry loop. > > > > Another flow control implementation, cr50_spi_flow_control(), doesn't > > address this at all. > > > > Let's clean this up and just make the tx_buf NULL before we call > > flow_control(). Not only does this ensure that we're not sending any > > "random" bytes but it also possibly could make the SPI controller > > behave in a slightly more optimal way. > > > > NOTE: no actual observed problems are fixed by this patch--it's was > > just made based on code inspection. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > drivers/char/tpm/tpm_tis_spi_main.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c > > index d96755935529..8d2c581a93c6 100644 > > --- a/drivers/char/tpm/tpm_tis_spi_main.c > > +++ b/drivers/char/tpm/tpm_tis_spi_main.c > > @@ -53,8 +53,6 @@ static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy, > > > > if ((phy->iobuf[3] & 0x01) == 0) { > > // handle SPI wait states > > - phy->iobuf[0] = 0; > > - > > Why this should be removed? As far as I can tell the only purpose of that was to make sure we were sending 0. Specifically "tx_buf" "rx_buf" both point to "phy->iobuf" so setting the first byte to 0 here made sure that we weren't sending out "random" data but were instead sending a 0. After my change "tx_buf" is NULL so we don't need to do this--the controller should take charge of sending nothing on the lines (AKA sending a zero). Does that answer your question, or were you worried about us needing to init iobuf[0] to 0 in some other case? -Doug
On Mon, Jun 01, 2020 at 03:54:03PM -0700, Doug Anderson wrote: > Does that answer your question, or were you worried about us needing > to init iobuf[0] to 0 in some other case? > > -Doug No, but it should be treated as a bug fix for CR50 implementation i.e. for 797c0113c9a481d4554988d70b5b52fae657262f, or is there some reason why it shouldn't? /Jarkko
Hi, On Thu, Jun 4, 2020 at 2:40 AM Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote: > > On Mon, Jun 01, 2020 at 03:54:03PM -0700, Doug Anderson wrote: > > Does that answer your question, or were you worried about us needing > > to init iobuf[0] to 0 in some other case? > > > > -Doug > > No, but it should be treated as a bug fix for CR50 implementation i.e. > for 797c0113c9a481d4554988d70b5b52fae657262f, or is there some reason > why it shouldn't? As talked about in the commit message, I think this is a slight cleanup for non-Cr50 too. Specifically if we end up running through the TPM_RETRY loop a second time we weren't re-initting "phy->iobuf[0] = 0;" That means that the 2nd time through the loop we were actually sending the TPM back the byte that the TPM sent us the first time through the loop. Presumably this doesn't matter much, but it still feels nicer not to be sending the TPM's bytes back to it when we're not really supposed to. Also, as mentioned in the commit message, I haven't observed this fixing any problems. I only came up with it from code inspection while trying to track something else down. -Doug
On Thu, Jun 04, 2020 at 06:48:58AM -0700, Doug Anderson wrote: > Hi, > > On Thu, Jun 4, 2020 at 2:40 AM Jarkko Sakkinen > <jarkko.sakkinen@linux.intel.com> wrote: > > > > On Mon, Jun 01, 2020 at 03:54:03PM -0700, Doug Anderson wrote: > > > Does that answer your question, or were you worried about us needing > > > to init iobuf[0] to 0 in some other case? > > > > > > -Doug > > > > No, but it should be treated as a bug fix for CR50 implementation i.e. > > for 797c0113c9a481d4554988d70b5b52fae657262f, or is there some reason > > why it shouldn't? > > As talked about in the commit message, I think this is a slight > cleanup for non-Cr50 too. Specifically if we end up running through > the TPM_RETRY loop a second time we weren't re-initting "phy->iobuf[0] > = 0;" That means that the 2nd time through the loop we were actually > sending the TPM back the byte that the TPM sent us the first time > through the loop. > > Presumably this doesn't matter much, but it still feels nicer not to > be sending the TPM's bytes back to it when we're not really supposed > to. > > Also, as mentioned in the commit message, I haven't observed this > fixing any problems. I only came up with it from code inspection > while trying to track something else down. Thanks, I'm happy how it is. Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko
diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c index d96755935529..8d2c581a93c6 100644 --- a/drivers/char/tpm/tpm_tis_spi_main.c +++ b/drivers/char/tpm/tpm_tis_spi_main.c @@ -53,8 +53,6 @@ static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy, if ((phy->iobuf[3] & 0x01) == 0) { // handle SPI wait states - phy->iobuf[0] = 0; - for (i = 0; i < TPM_RETRY; i++) { spi_xfer->len = 1; spi_message_init(&m); @@ -104,6 +102,8 @@ int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len, if (ret < 0) goto exit; + /* Flow control transfers are receive only */ + spi_xfer.tx_buf = NULL; ret = phy->flow_control(phy, &spi_xfer); if (ret < 0) goto exit; @@ -113,9 +113,8 @@ int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len, spi_xfer.delay.value = 5; spi_xfer.delay.unit = SPI_DELAY_UNIT_USECS; - if (in) { - spi_xfer.tx_buf = NULL; - } else if (out) { + if (out) { + spi_xfer.tx_buf = phy->iobuf; spi_xfer.rx_buf = NULL; memcpy(phy->iobuf, out, transfer_len); out += transfer_len;
During flow control we are just reading from the TPM, yet our spi_xfer has the tx_buf and rx_buf both non-NULL which means we're requesting a full duplex transfer. SPI is always somewhat of a full duplex protocol anyway and in theory the other side shouldn't really be looking at what we're sending it during flow control, but it's still a bit ugly to be sending some "random" data when we shouldn't. The default tpm_tis_spi_flow_control() tries to address this by setting 'phy->iobuf[0] = 0'. This partially avoids the problem of sending "random" data, but since our tx_buf and rx_buf both point to the same place I believe there is the potential of us sending the TPM's previous byte back to it if we hit the retry loop. Another flow control implementation, cr50_spi_flow_control(), doesn't address this at all. Let's clean this up and just make the tx_buf NULL before we call flow_control(). Not only does this ensure that we're not sending any "random" bytes but it also possibly could make the SPI controller behave in a slightly more optimal way. NOTE: no actual observed problems are fixed by this patch--it's was just made based on code inspection. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/char/tpm/tpm_tis_spi_main.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)