* [PATCH] tpm: Add flag to use default cancellation policy
@ 2022-09-07 16:43 Eddie James
2022-09-08 5:22 ` Jarkko Sakkinen
0 siblings, 1 reply; 5+ messages in thread
From: Eddie James @ 2022-09-07 16:43 UTC (permalink / raw)
To: linux-integrity
Cc: linux-kernel, peterhuewe, jarkko, jgg, joel, Alexander.Steffen,
Eddie James
The check for cancelled request depends on the VID of the chip, but
some chips share VID which shouldn't share their cancellation
behavior. This is the case for the Nuvoton NPCT75X, which should use
the default cancellation check, not the Winbond one.
To avoid changing the existing behavior, add a new flag to indicate
that the chip should use the default cancellation check and set it
for the I2C TPM2 TIS driver.
Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
drivers/char/tpm/tpm_tis_core.c | 18 ++++++++++--------
drivers/char/tpm/tpm_tis_core.h | 1 +
drivers/char/tpm/tpm_tis_i2c.c | 1 +
3 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 757623bacfd5..175e75337395 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -682,15 +682,17 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
{
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
- switch (priv->manufacturer_id) {
- case TPM_VID_WINBOND:
- return ((status == TPM_STS_VALID) ||
- (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
- case TPM_VID_STM:
- return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
- default:
- return (status == TPM_STS_COMMAND_READY);
+ if (!test_bit(TPM_TIS_DEFAULT_CANCELLATION, &priv->flags)) {
+ switch (priv->manufacturer_id) {
+ case TPM_VID_WINBOND:
+ return ((status == TPM_STS_VALID) ||
+ (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
+ case TPM_VID_STM:
+ return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
+ }
}
+
+ return status == TPM_STS_COMMAND_READY;
}
static irqreturn_t tis_int_handler(int dummy, void *dev_id)
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 66a5a13cd1df..b68479e0de10 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -86,6 +86,7 @@ enum tis_defaults {
enum tpm_tis_flags {
TPM_TIS_ITPM_WORKAROUND = BIT(0),
TPM_TIS_INVALID_STATUS = BIT(1),
+ TPM_TIS_DEFAULT_CANCELLATION = BIT(2),
};
struct tpm_tis_data {
diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
index 0692510dfcab..6722588e0217 100644
--- a/drivers/char/tpm/tpm_tis_i2c.c
+++ b/drivers/char/tpm/tpm_tis_i2c.c
@@ -329,6 +329,7 @@ static int tpm_tis_i2c_probe(struct i2c_client *dev,
if (!phy->io_buf)
return -ENOMEM;
+ set_bit(TPM_TIS_DEFAULT_CANCELLATION, &phy->priv.flags);
phy->i2c_client = dev;
/* must precede all communication with the tpm */
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] tpm: Add flag to use default cancellation policy
2022-09-07 16:43 [PATCH] tpm: Add flag to use default cancellation policy Eddie James
@ 2022-09-08 5:22 ` Jarkko Sakkinen
2022-09-08 13:53 ` Eddie James
0 siblings, 1 reply; 5+ messages in thread
From: Jarkko Sakkinen @ 2022-09-08 5:22 UTC (permalink / raw)
To: Eddie James
Cc: linux-integrity, linux-kernel, peterhuewe, jarkko, jgg, joel,
Alexander.Steffen
On Wed, Sep 07, 2022 at 11:43:17AM -0500, Eddie James wrote:
> The check for cancelled request depends on the VID of the chip, but
> some chips share VID which shouldn't share their cancellation
> behavior. This is the case for the Nuvoton NPCT75X, which should use
> the default cancellation check, not the Winbond one.
> To avoid changing the existing behavior, add a new flag to indicate
> that the chip should use the default cancellation check and set it
> for the I2C TPM2 TIS driver.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
> drivers/char/tpm/tpm_tis_core.c | 18 ++++++++++--------
> drivers/char/tpm/tpm_tis_core.h | 1 +
> drivers/char/tpm/tpm_tis_i2c.c | 1 +
> 3 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 757623bacfd5..175e75337395 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -682,15 +682,17 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
> {
> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>
> - switch (priv->manufacturer_id) {
> - case TPM_VID_WINBOND:
> - return ((status == TPM_STS_VALID) ||
> - (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
> - case TPM_VID_STM:
> - return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
> - default:
> - return (status == TPM_STS_COMMAND_READY);
> + if (!test_bit(TPM_TIS_DEFAULT_CANCELLATION, &priv->flags)) {
> + switch (priv->manufacturer_id) {
> + case TPM_VID_WINBOND:
> + return ((status == TPM_STS_VALID) ||
> + (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
> + case TPM_VID_STM:
> + return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
> + }
Why there is no default: ?
> }
> +
> + return status == TPM_STS_COMMAND_READY;
> }
>
> static irqreturn_t tis_int_handler(int dummy, void *dev_id)
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index 66a5a13cd1df..b68479e0de10 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -86,6 +86,7 @@ enum tis_defaults {
> enum tpm_tis_flags {
> TPM_TIS_ITPM_WORKAROUND = BIT(0),
> TPM_TIS_INVALID_STATUS = BIT(1),
> + TPM_TIS_DEFAULT_CANCELLATION = BIT(2),
> };
>
> struct tpm_tis_data {
> diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
> index 0692510dfcab..6722588e0217 100644
> --- a/drivers/char/tpm/tpm_tis_i2c.c
> +++ b/drivers/char/tpm/tpm_tis_i2c.c
> @@ -329,6 +329,7 @@ static int tpm_tis_i2c_probe(struct i2c_client *dev,
> if (!phy->io_buf)
> return -ENOMEM;
>
> + set_bit(TPM_TIS_DEFAULT_CANCELLATION, &phy->priv.flags);
What if you just zeroed manufacturer ID?
> phy->i2c_client = dev;
>
> /* must precede all communication with the tpm */
> --
> 2.31.1
>
BR, Jarkko
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tpm: Add flag to use default cancellation policy
2022-09-08 5:22 ` Jarkko Sakkinen
@ 2022-09-08 13:53 ` Eddie James
2022-09-28 5:10 ` Joel Stanley
0 siblings, 1 reply; 5+ messages in thread
From: Eddie James @ 2022-09-08 13:53 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, linux-kernel, peterhuewe, jarkko, jgg, joel,
Alexander.Steffen
On 9/8/22 00:22, Jarkko Sakkinen wrote:
> On Wed, Sep 07, 2022 at 11:43:17AM -0500, Eddie James wrote:
>> The check for cancelled request depends on the VID of the chip, but
>> some chips share VID which shouldn't share their cancellation
>> behavior. This is the case for the Nuvoton NPCT75X, which should use
>> the default cancellation check, not the Winbond one.
>> To avoid changing the existing behavior, add a new flag to indicate
>> that the chip should use the default cancellation check and set it
>> for the I2C TPM2 TIS driver.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>> drivers/char/tpm/tpm_tis_core.c | 18 ++++++++++--------
>> drivers/char/tpm/tpm_tis_core.h | 1 +
>> drivers/char/tpm/tpm_tis_i2c.c | 1 +
>> 3 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
>> index 757623bacfd5..175e75337395 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -682,15 +682,17 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
>> {
>> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>
>> - switch (priv->manufacturer_id) {
>> - case TPM_VID_WINBOND:
>> - return ((status == TPM_STS_VALID) ||
>> - (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
>> - case TPM_VID_STM:
>> - return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
>> - default:
>> - return (status == TPM_STS_COMMAND_READY);
>> + if (!test_bit(TPM_TIS_DEFAULT_CANCELLATION, &priv->flags)) {
>> + switch (priv->manufacturer_id) {
>> + case TPM_VID_WINBOND:
>> + return ((status == TPM_STS_VALID) ||
>> + (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
>> + case TPM_VID_STM:
>> + return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
>> + }
> Why there is no default: ?
Well I didn't want to duplicate the line "status ==
TPM_STS_COMMAND_READY" in the default case and for the flagged case. So
now the switch just falls through for default. I can add default: break
instead
>
>> }
>> +
>> + return status == TPM_STS_COMMAND_READY;
>> }
>>
>> static irqreturn_t tis_int_handler(int dummy, void *dev_id)
>> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
>> index 66a5a13cd1df..b68479e0de10 100644
>> --- a/drivers/char/tpm/tpm_tis_core.h
>> +++ b/drivers/char/tpm/tpm_tis_core.h
>> @@ -86,6 +86,7 @@ enum tis_defaults {
>> enum tpm_tis_flags {
>> TPM_TIS_ITPM_WORKAROUND = BIT(0),
>> TPM_TIS_INVALID_STATUS = BIT(1),
>> + TPM_TIS_DEFAULT_CANCELLATION = BIT(2),
>> };
>>
>> struct tpm_tis_data {
>> diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
>> index 0692510dfcab..6722588e0217 100644
>> --- a/drivers/char/tpm/tpm_tis_i2c.c
>> +++ b/drivers/char/tpm/tpm_tis_i2c.c
>> @@ -329,6 +329,7 @@ static int tpm_tis_i2c_probe(struct i2c_client *dev,
>> if (!phy->io_buf)
>> return -ENOMEM;
>>
>> + set_bit(TPM_TIS_DEFAULT_CANCELLATION, &phy->priv.flags);
> What if you just zeroed manufacturer ID?
It's already zero there, and gets set to the VID as part of the core
init function.
Thanks,
Eddie
>
>> phy->i2c_client = dev;
>>
>> /* must precede all communication with the tpm */
>> --
>> 2.31.1
>>
> BR, Jarkko
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tpm: Add flag to use default cancellation policy
2022-09-08 13:53 ` Eddie James
@ 2022-09-28 5:10 ` Joel Stanley
2022-09-30 21:49 ` Jarkko Sakkinen
0 siblings, 1 reply; 5+ messages in thread
From: Joel Stanley @ 2022-09-28 5:10 UTC (permalink / raw)
To: Eddie James
Cc: Jarkko Sakkinen, linux-integrity, linux-kernel, peterhuewe,
jarkko, jgg, Alexander.Steffen
On Thu, 8 Sept 2022 at 13:53, Eddie James <eajames@linux.ibm.com> wrote:
>
>
> On 9/8/22 00:22, Jarkko Sakkinen wrote:
> > On Wed, Sep 07, 2022 at 11:43:17AM -0500, Eddie James wrote:
> >> The check for cancelled request depends on the VID of the chip, but
> >> some chips share VID which shouldn't share their cancellation
> >> behavior. This is the case for the Nuvoton NPCT75X, which should use
> >> the default cancellation check, not the Winbond one.
> >> To avoid changing the existing behavior, add a new flag to indicate
> >> that the chip should use the default cancellation check and set it
> >> for the I2C TPM2 TIS driver.
> >>
> >> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> >> ---
> >> drivers/char/tpm/tpm_tis_core.c | 18 ++++++++++--------
> >> drivers/char/tpm/tpm_tis_core.h | 1 +
> >> drivers/char/tpm/tpm_tis_i2c.c | 1 +
> >> 3 files changed, 12 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> >> index 757623bacfd5..175e75337395 100644
> >> --- a/drivers/char/tpm/tpm_tis_core.c
> >> +++ b/drivers/char/tpm/tpm_tis_core.c
> >> @@ -682,15 +682,17 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
> >> {
> >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> >>
> >> - switch (priv->manufacturer_id) {
> >> - case TPM_VID_WINBOND:
> >> - return ((status == TPM_STS_VALID) ||
> >> - (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
> >> - case TPM_VID_STM:
> >> - return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
> >> - default:
> >> - return (status == TPM_STS_COMMAND_READY);
> >> + if (!test_bit(TPM_TIS_DEFAULT_CANCELLATION, &priv->flags)) {
> >> + switch (priv->manufacturer_id) {
> >> + case TPM_VID_WINBOND:
> >> + return ((status == TPM_STS_VALID) ||
> >> + (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
> >> + case TPM_VID_STM:
> >> + return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
> >> + }
> > Why there is no default: ?
>
>
> Well I didn't want to duplicate the line "status ==
> TPM_STS_COMMAND_READY" in the default case and for the flagged case. So
> now the switch just falls through for default. I can add default: break
> instead
This code was in the original patch series submitted by Nuvoton:
https://lore.kernel.org/r/20211104140211.6258-3-amirmizi6@gmail.com
Perhaps something like that would be better?
>
>
> >
> >> }
> >> +
> >> + return status == TPM_STS_COMMAND_READY;
> >> }
> >>
> >> static irqreturn_t tis_int_handler(int dummy, void *dev_id)
> >> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> >> index 66a5a13cd1df..b68479e0de10 100644
> >> --- a/drivers/char/tpm/tpm_tis_core.h
> >> +++ b/drivers/char/tpm/tpm_tis_core.h
> >> @@ -86,6 +86,7 @@ enum tis_defaults {
> >> enum tpm_tis_flags {
> >> TPM_TIS_ITPM_WORKAROUND = BIT(0),
> >> TPM_TIS_INVALID_STATUS = BIT(1),
> >> + TPM_TIS_DEFAULT_CANCELLATION = BIT(2),
> >> };
> >>
> >> struct tpm_tis_data {
> >> diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
> >> index 0692510dfcab..6722588e0217 100644
> >> --- a/drivers/char/tpm/tpm_tis_i2c.c
> >> +++ b/drivers/char/tpm/tpm_tis_i2c.c
> >> @@ -329,6 +329,7 @@ static int tpm_tis_i2c_probe(struct i2c_client *dev,
> >> if (!phy->io_buf)
> >> return -ENOMEM;
> >>
> >> + set_bit(TPM_TIS_DEFAULT_CANCELLATION, &phy->priv.flags);
> > What if you just zeroed manufacturer ID?
>
>
> It's already zero there, and gets set to the VID as part of the core
> init function.
>
>
> Thanks,
>
> Eddie
>
>
> >
> >> phy->i2c_client = dev;
> >>
> >> /* must precede all communication with the tpm */
> >> --
> >> 2.31.1
> >>
> > BR, Jarkko
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tpm: Add flag to use default cancellation policy
2022-09-28 5:10 ` Joel Stanley
@ 2022-09-30 21:49 ` Jarkko Sakkinen
0 siblings, 0 replies; 5+ messages in thread
From: Jarkko Sakkinen @ 2022-09-30 21:49 UTC (permalink / raw)
To: Joel Stanley
Cc: Eddie James, linux-integrity, linux-kernel, peterhuewe, jarkko,
jgg, Alexander.Steffen
On Wed, Sep 28, 2022 at 05:10:25AM +0000, Joel Stanley wrote:
> On Thu, 8 Sept 2022 at 13:53, Eddie James <eajames@linux.ibm.com> wrote:
> >
> >
> > On 9/8/22 00:22, Jarkko Sakkinen wrote:
> > > On Wed, Sep 07, 2022 at 11:43:17AM -0500, Eddie James wrote:
> > >> The check for cancelled request depends on the VID of the chip, but
> > >> some chips share VID which shouldn't share their cancellation
> > >> behavior. This is the case for the Nuvoton NPCT75X, which should use
> > >> the default cancellation check, not the Winbond one.
> > >> To avoid changing the existing behavior, add a new flag to indicate
> > >> that the chip should use the default cancellation check and set it
> > >> for the I2C TPM2 TIS driver.
> > >>
> > >> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> > >> ---
> > >> drivers/char/tpm/tpm_tis_core.c | 18 ++++++++++--------
> > >> drivers/char/tpm/tpm_tis_core.h | 1 +
> > >> drivers/char/tpm/tpm_tis_i2c.c | 1 +
> > >> 3 files changed, 12 insertions(+), 8 deletions(-)
> > >>
> > >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > >> index 757623bacfd5..175e75337395 100644
> > >> --- a/drivers/char/tpm/tpm_tis_core.c
> > >> +++ b/drivers/char/tpm/tpm_tis_core.c
> > >> @@ -682,15 +682,17 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
> > >> {
> > >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> > >>
> > >> - switch (priv->manufacturer_id) {
> > >> - case TPM_VID_WINBOND:
> > >> - return ((status == TPM_STS_VALID) ||
> > >> - (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
> > >> - case TPM_VID_STM:
> > >> - return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
> > >> - default:
> > >> - return (status == TPM_STS_COMMAND_READY);
> > >> + if (!test_bit(TPM_TIS_DEFAULT_CANCELLATION, &priv->flags)) {
> > >> + switch (priv->manufacturer_id) {
> > >> + case TPM_VID_WINBOND:
> > >> + return ((status == TPM_STS_VALID) ||
> > >> + (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
> > >> + case TPM_VID_STM:
> > >> + return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
> > >> + }
> > > Why there is no default: ?
> >
> >
> > Well I didn't want to duplicate the line "status ==
> > TPM_STS_COMMAND_READY" in the default case and for the flagged case. So
> > now the switch just falls through for default. I can add default: break
> > instead
>
> This code was in the original patch series submitted by Nuvoton:
>
> https://lore.kernel.org/r/20211104140211.6258-3-amirmizi6@gmail.com
>
> Perhaps something like that would be better?
The current patch could have
default:
/* fall-through */
break;
BR, Jarkko
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-09-30 21:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-07 16:43 [PATCH] tpm: Add flag to use default cancellation policy Eddie James
2022-09-08 5:22 ` Jarkko Sakkinen
2022-09-08 13:53 ` Eddie James
2022-09-28 5:10 ` Joel Stanley
2022-09-30 21:49 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).