[PATCH 0/5] tpm: assorted fixes and cleanups

[PATCH 0/5] tpm: assorted fixes and cleanups

[Pei Xiao]

This series fixes several issues found during a code review of
drivers/char/tpm:

  1. tpm_ppi: tpm_show_ppi_response() stores its return value in an
     acpi_status (a u32 typedef), so error codes such as -EINVAL reach
     user space as huge positive values.  Declare the variable as
     ssize_t to match the show callback's return type.

  2. tpm_nsc: tpm_nsc_remove() doubles as the release callback of the
     hand-created platform device and dereferences the chip drvdata
     unconditionally; init failures before tpmm_chip_alloc() crash
     module load.  Return early when the chip has not been created.

  3. tpm_nsc: the cleanup runs twice on module exit because
     tpm_nsc_remove() is both the explicit cleanup and the device
     release callback; the second run operates on an already freed
     chip.  Stop overriding the release callback, which also stops the
     platform object allocation from leaking.

  4. tpm_dev: a zero-length read() discards a pending response,
     breaking the command/response pairing of the TPM character
     devices, although POSIX requires zero-count reads to have no side
     effects.  Return early on a zero count.

  5. tpm-interface: make the tpm_init() error messages consistently
     prefixed with "tpm: " and report the actual failure of
     tpm_dev_common_init().

The series was prepared with AI assistance (GLM-5.3); every change was
reviewed against the code by hand.

Pei Xiao (5):
  tpm: tpm_ppi: fix wrong error code returned to user space
  tpm: tpm_nsc: fix NULL pointer dereference on init failure
  tpm: tpm_nsc: stop using the cleanup callback as dev.release
  tpm: fix zero-length read discarding the pending response
  tpm: fix log messages in tpm_init()

 drivers/char/tpm/tpm-dev-common.c | 3 +++
 drivers/char/tpm/tpm-interface.c  | 6 +++---
 drivers/char/tpm/tpm_nsc.c        | 8 ++++++--
 drivers/char/tpm/tpm_ppi.c        | 2 +-
 4 files changed, 13 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/5] tpm: tpm_ppi: fix wrong error code returned to user space

[Pei Xiao]

tpm_show_ppi_response() stores its return value in an acpi_status,
a typedef of u32.  Both error paths of the function (-EINVAL on a
malformed _DSM package, -EFAULT on a non-zero operation return
code) end up as huge positive values when returned as ssize_t, so
user space cannot detect the failure with the usual "ret < 0"
check.

Declare the variable as ssize_t to match the show callback's
return type.

Fixes: 84b1667dea23 ("ACPI / TPM: replace open-coded _DSM code with helper functions")
Assisted-by: GLM-5.3
Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
---
 drivers/char/tpm/tpm_ppi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c
index c9793a3d986d..949fb7055bea 100644
--- a/drivers/char/tpm/tpm_ppi.c
+++ b/drivers/char/tpm/tpm_ppi.c
@@ -234,7 +234,7 @@ static ssize_t tpm_show_ppi_response(struct device *dev,
 				     struct device_attribute *attr,
 				     char *buf)
 {
-	acpi_status status = -EINVAL;
+	ssize_t status = -EINVAL;
 	union acpi_object *obj, *ret_obj;
 	u64 req, res;
 	struct tpm_chip *chip = to_tpm_chip(dev);
-- 
2.25.1

[PATCH 2/5] tpm: tpm_nsc: fix NULL pointer dereference on init failure

[Pei Xiao]

tpm_nsc_remove() is used as the release callback of the hand-created
platform device and dereferences the chip drvdata unconditionally.
If init fails before tpmm_chip_alloc() (e.g. request_region() cannot
claim the ports), the error path drops the last device reference and
the release callback runs with chip == NULL, crashing module init.

Return early when the chip has not been created yet.

Fixes: afb5abc262e9 ("tpm: two-phase chip management functions")
Assisted-by: GLM-5.3
Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
---
 drivers/char/tpm/tpm_nsc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_nsc.c b/drivers/char/tpm/tpm_nsc.c
index 879ac88f5783..46a52dc99b14 100644
--- a/drivers/char/tpm/tpm_nsc.c
+++ b/drivers/char/tpm/tpm_nsc.c
@@ -259,7 +259,12 @@ static struct platform_device *pdev = NULL;
 static void tpm_nsc_remove(struct device *dev)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
-	struct tpm_nsc_priv *priv = dev_get_drvdata(&chip->dev);
+	struct tpm_nsc_priv *priv;
+
+	if (!chip)
+		return;
+
+	priv = dev_get_drvdata(&chip->dev);
 
 	tpm_chip_unregister(chip);
 	release_region(priv->base, 2);
-- 
2.25.1

[PATCH 3/5] tpm: tpm_nsc: stop using the cleanup callback as dev.release

[Pei Xiao]

tpm_nsc_remove() is called explicitly from cleanup_nsc() and also
runs as the platform device release callback on the final
platform_device_put(), so the cleanup executes twice on module
exit; the second invocation operates on a chip that has already
been freed by the devm cleanup, and the I/O region is released
twice.

Overwriting the release callback installed by
platform_device_alloc() also keeps platform_device_release() from
running, leaking the platform object allocation.

Leave the default release callback in place; the explicit call in
cleanup_nsc() remains the single cleanup point.

Fixes: 570302a31149 ("[PATCH] tpm: move nsc driver off pci_dev")
Assisted-by: GLM-5.3
Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
---
 drivers/char/tpm/tpm_nsc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_nsc.c b/drivers/char/tpm/tpm_nsc.c
index 46a52dc99b14..bcfa2a1a208a 100644
--- a/drivers/char/tpm/tpm_nsc.c
+++ b/drivers/char/tpm/tpm_nsc.c
@@ -327,7 +327,6 @@ static int __init init_nsc(void)
 
 	pdev->num_resources = 0;
 	pdev->dev.driver = &nsc_drv.driver;
-	pdev->dev.release = tpm_nsc_remove;
 
 	if ((rc = platform_device_add(pdev)) < 0)
 		goto err_put_dev;
-- 
2.25.1

[PATCH 4/5] tpm: fix zero-length read discarding the pending response

[Pei Xiao]

POSIX requires that a read() with a count of zero returns zero and
has no other effects.  tpm_common_read() treats such a call as a
consumed response: it marks the pending response as read and drops
it, so the response can never be retrieved; subsequent reads return
zero and the next write() is allowed to overwrite the response
buffer, silently breaking the command/response pairing of the TPM
character devices.

Return early when the caller passes a zero count, leaving any
pending response untouched for the next read.  A zero-length read
will not report a deferred asynchronous error; POSIX permits read()
to skip error detection for a zero count.

Fixes: 9488585b21be ("tpm: add support for partial reads")
Assisted-by: GLM-5.3
Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
---
 drivers/char/tpm/tpm-dev-common.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index f942c0c8e402..6569212dc6b8 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -134,6 +134,9 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
 	ssize_t ret_size = 0;
 	int rc;
 
+	if (!size)
+		return 0;
+
 	mutex_lock(&priv->buffer_mutex);
 
 	if (priv->response_length) {
-- 
2.25.1

[PATCH 5/5] tpm: fix log messages in tpm_init()

[Pei Xiao]

Make the error messages consistently prefixed with "tpm: ", and fix
the tpm_dev_common_init() failure message which was wrongly copied
from the previous step.

Assisted-by: GLM-5.3
Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
---
 drivers/char/tpm/tpm-interface.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1ccdbde98b69..666a1c654c02 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -524,13 +524,13 @@ static int __init tpm_init(void)
 
 	rc = class_register(&tpm_class);
 	if (rc) {
-		pr_err("couldn't create tpm class\n");
+		pr_err("tpm: couldn't create tpm class\n");
 		return rc;
 	}
 
 	rc = class_register(&tpmrm_class);
 	if (rc) {
-		pr_err("couldn't create tpmrm class\n");
+		pr_err("tpm: couldn't create tpmrm class\n");
 		goto out_destroy_tpm_class;
 	}
 
@@ -542,7 +542,7 @@ static int __init tpm_init(void)
 
 	rc = tpm_dev_common_init();
 	if (rc) {
-		pr_err("tpm: failed to allocate char dev region\n");
+		pr_err("tpm: failed to allocate TPM workqueue\n");
 		goto out_unreg_chrdev;
 	}
 
-- 
2.25.1

Re: [PATCH 5/5] tpm: fix log messages in tpm_init()

[Jarkko Sakkinen]

On Sat, Oct 03, 2026 at 04:27:55PM +0800, Pei Xiao wrote:
> Make the error messages consistently prefixed with "tpm: ", and fix
> the tpm_dev_common_init() failure message which was wrongly copied
> from the previous step.
> 
> Assisted-by: GLM-5.3
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> ---
>  drivers/char/tpm/tpm-interface.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 1ccdbde98b69..666a1c654c02 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -524,13 +524,13 @@ static int __init tpm_init(void)
>  
>  	rc = class_register(&tpm_class);
>  	if (rc) {
> -		pr_err("couldn't create tpm class\n");
> +		pr_err("tpm: couldn't create tpm class\n");
>  		return rc;
>  	}
>  
>  	rc = class_register(&tpmrm_class);
>  	if (rc) {
> -		pr_err("couldn't create tpmrm class\n");
> +		pr_err("tpm: couldn't create tpmrm class\n");
>  		goto out_destroy_tpm_class;
>  	}
>  
> @@ -542,7 +542,7 @@ static int __init tpm_init(void)
>  
>  	rc = tpm_dev_common_init();
>  	if (rc) {
> -		pr_err("tpm: failed to allocate char dev region\n");
> +		pr_err("tpm: failed to allocate TPM workqueue\n");
>  		goto out_unreg_chrdev;
>  	}
>  
> -- 
> 2.25.1
> 

I NAK this one. It is not fixing anything.

Br, Jarkko

Re: [PATCH 1/5] tpm: tpm_ppi: fix wrong error code returned to user space

[Jarkko Sakkinen]

On Sat, Oct 03, 2026 at 04:27:51PM +0800, Pei Xiao wrote:
> tpm_show_ppi_response() stores its return value in an acpi_status,
> a typedef of u32.  Both error paths of the function (-EINVAL on a
> malformed _DSM package, -EFAULT on a non-zero operation return
> code) end up as huge positive values when returned as ssize_t, so
> user space cannot detect the failure with the usual "ret < 0"
> check.

I have no idea what you mean by huge value and why that would be
a problem, and overall this is disconnected from the code change.

Two's complement value is correctly stored in status up until the
ssize_t cast, which results the value to be zero-extended, and
as a result corrupt the negative values.

> 
> Declare the variable as ssize_t to match the show callback's
> return type.
> 
> Fixes: 84b1667dea23 ("ACPI / TPM: replace open-coded _DSM code with helper functions")
> Assisted-by: GLM-5.3
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> ---
>  drivers/char/tpm/tpm_ppi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c
> index c9793a3d986d..949fb7055bea 100644
> --- a/drivers/char/tpm/tpm_ppi.c
> +++ b/drivers/char/tpm/tpm_ppi.c
> @@ -234,7 +234,7 @@ static ssize_t tpm_show_ppi_response(struct device *dev,
>  				     struct device_attribute *attr,
>  				     char *buf)
>  {
> -	acpi_status status = -EINVAL;
> +	ssize_t status = -EINVAL;
>  	union acpi_object *obj, *ret_obj;
>  	u64 req, res;
>  	struct tpm_chip *chip = to_tpm_chip(dev);
> -- 
> 2.25.1
> 

Br, Jarkko

Re: [PATCH 2/5] tpm: tpm_nsc: fix NULL pointer dereference on init failure

[Jarkko Sakkinen]

On Sat, Oct 03, 2026 at 04:27:52PM +0800, Pei Xiao wrote:
> tpm_nsc_remove() is used as the release callback of the hand-created
> platform device and dereferences the chip drvdata unconditionally.
> If init fails before tpmm_chip_alloc() (e.g. request_region() cannot
> claim the ports), the error path drops the last device reference and
> the release callback runs with chip == NULL, crashing module init.
> 
> Return early when the chip has not been created yet.
> 
> Fixes: afb5abc262e9 ("tpm: two-phase chip management functions")
> Assisted-by: GLM-5.3
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> ---
>  drivers/char/tpm/tpm_nsc.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm_nsc.c b/drivers/char/tpm/tpm_nsc.c
> index 879ac88f5783..46a52dc99b14 100644
> --- a/drivers/char/tpm/tpm_nsc.c
> +++ b/drivers/char/tpm/tpm_nsc.c
> @@ -259,7 +259,12 @@ static struct platform_device *pdev = NULL;
>  static void tpm_nsc_remove(struct device *dev)
>  {
>  	struct tpm_chip *chip = dev_get_drvdata(dev);
> -	struct tpm_nsc_priv *priv = dev_get_drvdata(&chip->dev);
> +	struct tpm_nsc_priv *priv;
> +
> +	if (!chip)
> +		return;
> +
> +	priv = dev_get_drvdata(&chip->dev);
>  
>  	tpm_chip_unregister(chip);
>  	release_region(priv->base, 2);
> -- 
> 2.25.1
> 

I can apply this, thanks.

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Br, Jarkko

Re: [PATCH 3/5] tpm: tpm_nsc: stop using the cleanup callback as dev.release

[Jarkko Sakkinen]

On Sat, Oct 03, 2026 at 04:27:53PM +0800, Pei Xiao wrote:
> tpm_nsc_remove() is called explicitly from cleanup_nsc() and also
> runs as the platform device release callback on the final
> platform_device_put(), so the cleanup executes twice on module
> exit; the second invocation operates on a chip that has already
> been freed by the devm cleanup, and the I/O region is released
> twice.
> 
> Overwriting the release callback installed by
> platform_device_alloc() also keeps platform_device_release() from
> running, leaking the platform object allocation.
> 
> Leave the default release callback in place; the explicit call in
> cleanup_nsc() remains the single cleanup point.
> 
> Fixes: 570302a31149 ("[PATCH] tpm: move nsc driver off pci_dev")
> Assisted-by: GLM-5.3
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> ---
>  drivers/char/tpm/tpm_nsc.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm_nsc.c b/drivers/char/tpm/tpm_nsc.c
> index 46a52dc99b14..bcfa2a1a208a 100644
> --- a/drivers/char/tpm/tpm_nsc.c
> +++ b/drivers/char/tpm/tpm_nsc.c
> @@ -327,7 +327,6 @@ static int __init init_nsc(void)
>  
>  	pdev->num_resources = 0;
>  	pdev->dev.driver = &nsc_drv.driver;
> -	pdev->dev.release = tpm_nsc_remove;
>  
>  	if ((rc = platform_device_add(pdev)) < 0)
>  		goto err_put_dev;
> -- 
> 2.25.1
> 

This is fine too, thanks.

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Br, Jarkko

Re: [PATCH 5/5] tpm: fix log messages in tpm_init()

[Pei Xiao]

在 2026/10/5 11:46, Jarkko Sakkinen 写道:
> On Sat, Oct 03, 2026 at 04:27:55PM +0800, Pei Xiao wrote:
>> Make the error messages consistently prefixed with "tpm: ", and fix
>> the tpm_dev_common_init() failure message which was wrongly copied
>> from the previous step.
>>
>> Assisted-by: GLM-5.3
>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
>> ---
>>  drivers/char/tpm/tpm-interface.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index 1ccdbde98b69..666a1c654c02 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -524,13 +524,13 @@ static int __init tpm_init(void)
>>  
>>  	rc = class_register(&tpm_class);
>>  	if (rc) {
>> -		pr_err("couldn't create tpm class\n");
>> +		pr_err("tpm: couldn't create tpm class\n");
>>  		return rc;
>>  	}
>>  
>>  	rc = class_register(&tpmrm_class);
>>  	if (rc) {
>> -		pr_err("couldn't create tpmrm class\n");
>> +		pr_err("tpm: couldn't create tpmrm class\n");
>>  		goto out_destroy_tpm_class;
>>  	}
>>  
>> @@ -542,7 +542,7 @@ static int __init tpm_init(void)
>>  
>>  	rc = tpm_dev_common_init();
>>  	if (rc) {
>> -		pr_err("tpm: failed to allocate char dev region\n");
>> +		pr_err("tpm: failed to allocate TPM workqueue\n");
>>  		goto out_unreg_chrdev;
>>  	}
>>  
>> -- 
>> 2.25.1
>>
> 
> I NAK this one. It is not fixing anything.
Hmm, this is a cleanup, not a fix—just a very minor change to an error
log/print. I noticed it was duplicated (with the alloc_chrdev_region
error print), which made it impossible to tell which function call had
failed (it might actually never be executed). So I just brought up this
cleanup along the way.

Pei.
Thanks!>
> Br, Jarkko

Re: [PATCH 4/5] tpm: fix zero-length read discarding the pending response

[Jarkko Sakkinen]

On Sat, Oct 03, 2026 at 04:27:54PM +0800, Pei Xiao wrote:
> POSIX requires that a read() with a count of zero returns zero and
> has no other effects.  tpm_common_read() treats such a call as a
> consumed response: it marks the pending response as read and drops
> it, so the response can never be retrieved; subsequent reads return
> zero and the next write() is allowed to overwrite the response
> buffer, silently breaking the command/response pairing of the TPM
> character devices.
> 
> Return early when the caller passes a zero count, leaving any
> pending response untouched for the next read.  A zero-length read
> will not report a deferred asynchronous error; POSIX permits read()
> to skip error detection for a zero count.
> 
> Fixes: 9488585b21be ("tpm: add support for partial reads")
> Assisted-by: GLM-5.3
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> ---
>  drivers/char/tpm/tpm-dev-common.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index f942c0c8e402..6569212dc6b8 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -134,6 +134,9 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
>  	ssize_t ret_size = 0;
>  	int rc;
>  
> +	if (!size)
> +		return 0;
> +
>  	mutex_lock(&priv->buffer_mutex);
>  
>  	if (priv->response_length) {
> -- 
> 2.25.1
> 

This look good to me,  thanks.

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Br, Jarkko

Re: [PATCH 5/5] tpm: fix log messages in tpm_init()

[Jarkko Sakkinen]

On Mon, Oct 05, 2026 at 12:23:31PM +0800, Pei Xiao wrote:
> 
> 
> 在 2026/10/5 11:46, Jarkko Sakkinen 写道:
> > On Sat, Oct 03, 2026 at 04:27:55PM +0800, Pei Xiao wrote:
> >> Make the error messages consistently prefixed with "tpm: ", and fix
> >> the tpm_dev_common_init() failure message which was wrongly copied
> >> from the previous step.
> >>
> >> Assisted-by: GLM-5.3
> >> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> >> ---
> >>  drivers/char/tpm/tpm-interface.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> >> index 1ccdbde98b69..666a1c654c02 100644
> >> --- a/drivers/char/tpm/tpm-interface.c
> >> +++ b/drivers/char/tpm/tpm-interface.c
> >> @@ -524,13 +524,13 @@ static int __init tpm_init(void)
> >>  
> >>  	rc = class_register(&tpm_class);
> >>  	if (rc) {
> >> -		pr_err("couldn't create tpm class\n");
> >> +		pr_err("tpm: couldn't create tpm class\n");
> >>  		return rc;
> >>  	}
> >>  
> >>  	rc = class_register(&tpmrm_class);
> >>  	if (rc) {
> >> -		pr_err("couldn't create tpmrm class\n");
> >> +		pr_err("tpm: couldn't create tpmrm class\n");
> >>  		goto out_destroy_tpm_class;
> >>  	}
> >>  
> >> @@ -542,7 +542,7 @@ static int __init tpm_init(void)
> >>  
> >>  	rc = tpm_dev_common_init();
> >>  	if (rc) {
> >> -		pr_err("tpm: failed to allocate char dev region\n");
> >> +		pr_err("tpm: failed to allocate TPM workqueue\n");
> >>  		goto out_unreg_chrdev;
> >>  	}
> >>  
> >> -- 
> >> 2.25.1
> >>
> > 
> > I NAK this one. It is not fixing anything.
> Hmm, this is a cleanup, not a fix—just a very minor change to an error
> log/print. I noticed it was duplicated (with the alloc_chrdev_region
> error print), which made it impossible to tell which function call had
> failed (it might actually never be executed). So I just brought up this
> cleanup along the way.

If there is patch that is coming along the way, it is patch that should
not be sent because:

1. It wastes also everyone else's time.
2. Lack of understanding of cause and effect because by definition
   you have no idea what you are submitting. E

Pure clean ups per se are already something that is usually best to NAK
but this patch is not a clean up.

I mean the path is doing arbitrary log message changes. That is not
harmless change as you enforce your arbitrary preferences also for few
billion other users. 

Br, Jarkko

Re: [PATCH 5/5] tpm: fix log messages in tpm_init()

[Pei Xiao]

在 2026/10/5 14:05, Jarkko Sakkinen 写道:
> On Mon, Oct 05, 2026 at 12:23:31PM +0800, Pei Xiao wrote:
>>
>>
>> 在 2026/10/5 11:46, Jarkko Sakkinen 写道:
>>> On Sat, Oct 03, 2026 at 04:27:55PM +0800, Pei Xiao wrote:
>>>> Make the error messages consistently prefixed with "tpm: ", and fix
>>>> the tpm_dev_common_init() failure message which was wrongly copied
>>>> from the previous step.
>>>>
>>>> Assisted-by: GLM-5.3
>>>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
>>>> ---
>>>>  drivers/char/tpm/tpm-interface.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>>>> index 1ccdbde98b69..666a1c654c02 100644
>>>> --- a/drivers/char/tpm/tpm-interface.c
>>>> +++ b/drivers/char/tpm/tpm-interface.c
>>>> @@ -524,13 +524,13 @@ static int __init tpm_init(void)
>>>>  
>>>>  	rc = class_register(&tpm_class);
>>>>  	if (rc) {
>>>> -		pr_err("couldn't create tpm class\n");
>>>> +		pr_err("tpm: couldn't create tpm class\n");
>>>>  		return rc;
>>>>  	}
>>>>  
>>>>  	rc = class_register(&tpmrm_class);
>>>>  	if (rc) {
>>>> -		pr_err("couldn't create tpmrm class\n");
>>>> +		pr_err("tpm: couldn't create tpmrm class\n");
>>>>  		goto out_destroy_tpm_class;
>>>>  	}
>>>>  
>>>> @@ -542,7 +542,7 @@ static int __init tpm_init(void)
>>>>  
>>>>  	rc = tpm_dev_common_init();
>>>>  	if (rc) {
>>>> -		pr_err("tpm: failed to allocate char dev region\n");
>>>> +		pr_err("tpm: failed to allocate TPM workqueue\n");
>>>>  		goto out_unreg_chrdev;
>>>>  	}
>>>>  
>>>> -- 
>>>> 2.25.1
>>>>
>>>
>>> I NAK this one. It is not fixing anything.
>> Hmm, this is a cleanup, not a fix—just a very minor change to an error
>> log/print. I noticed it was duplicated (with the alloc_chrdev_region
>> error print), which made it impossible to tell which function call had
>> failed (it might actually never be executed). So I just brought up this
>> cleanup along the way.
> 
> If there is patch that is coming along the way, it is patch that should
> not be sent because:
> 
> 1. It wastes also everyone else's time.
> 2. Lack of understanding of cause and effect because by definition
>    you have no idea what you are submitting. E
> 
> Pure clean ups per se are already something that is usually best to NAK
> but this patch is not a clean up.
> 
> I mean the path is doing arbitrary log message changes. That is not
> harmless change as you enforce your arbitrary preferences also for few
> billion other users. 
OK, got it. Thanks.
> 
> Br, Jarkko

Re: [PATCH 5/5] tpm: fix log messages in tpm_init()

[Jarkko Sakkinen]

On Mon, Oct 05, 2026 at 09:05:25AM +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 05, 2026 at 12:23:31PM +0800, Pei Xiao wrote:
> > 
> > 
> > 在 2026/10/5 11:46, Jarkko Sakkinen 写道:
> > > On Sat, Oct 03, 2026 at 04:27:55PM +0800, Pei Xiao wrote:
> > >> Make the error messages consistently prefixed with "tpm: ", and fix
> > >> the tpm_dev_common_init() failure message which was wrongly copied
> > >> from the previous step.
> > >>
> > >> Assisted-by: GLM-5.3
> > >> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> > >> ---
> > >>  drivers/char/tpm/tpm-interface.c | 6 +++---
> > >>  1 file changed, 3 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > >> index 1ccdbde98b69..666a1c654c02 100644
> > >> --- a/drivers/char/tpm/tpm-interface.c
> > >> +++ b/drivers/char/tpm/tpm-interface.c
> > >> @@ -524,13 +524,13 @@ static int __init tpm_init(void)
> > >>  
> > >>  	rc = class_register(&tpm_class);
> > >>  	if (rc) {
> > >> -		pr_err("couldn't create tpm class\n");
> > >> +		pr_err("tpm: couldn't create tpm class\n");
> > >>  		return rc;
> > >>  	}
> > >>  
> > >>  	rc = class_register(&tpmrm_class);
> > >>  	if (rc) {
> > >> -		pr_err("couldn't create tpmrm class\n");
> > >> +		pr_err("tpm: couldn't create tpmrm class\n");
> > >>  		goto out_destroy_tpm_class;
> > >>  	}
> > >>  
> > >> @@ -542,7 +542,7 @@ static int __init tpm_init(void)
> > >>  
> > >>  	rc = tpm_dev_common_init();
> > >>  	if (rc) {
> > >> -		pr_err("tpm: failed to allocate char dev region\n");
> > >> +		pr_err("tpm: failed to allocate TPM workqueue\n");
> > >>  		goto out_unreg_chrdev;
> > >>  	}
> > >>  
> > >> -- 
> > >> 2.25.1
> > >>
> > > 
> > > I NAK this one. It is not fixing anything.
> > Hmm, this is a cleanup, not a fix—just a very minor change to an error
> > log/print. I noticed it was duplicated (with the alloc_chrdev_region
> > error print), which made it impossible to tell which function call had
> > failed (it might actually never be executed). So I just brought up this
> > cleanup along the way.
> 
> If there is patch that is coming along the way, it is patch that should
> not be sent because:
> 
> 1. It wastes also everyone else's time.
> 2. Lack of understanding of cause and effect because by definition
>    you have no idea what you are submitting. E
> 
> Pure clean ups per se are already something that is usually best to NAK
> but this patch is not a clean up.
> 
> I mean the path is doing arbitrary log message changes. That is not
> harmless change as you enforce your arbitrary preferences also for few
> billion other users. 

Well, maybe just machines but anyhow :-)

Br, Jarkko

Re: [PATCH 1/5] tpm: tpm_ppi: fix wrong error code returned to user space

[Pei Xiao]

在 2026/10/5 12:16, Jarkko Sakkinen 写道:
> On Sat, Oct 03, 2026 at 04:27:51PM +0800, Pei Xiao wrote:
>> tpm_show_ppi_response() stores its return value in an acpi_status,
>> a typedef of u32.  Both error paths of the function (-EINVAL on a
>> malformed _DSM package, -EFAULT on a non-zero operation return
>> code) end up as huge positive values when returned as ssize_t, so
>> user space cannot detect the failure with the usual "ret < 0"
>> check.
> 
> I have no idea what you mean by huge value and why that would be
> a problem, and overall this is disconnected from the code change.
> 
> Two's complement value is correctly stored in status up until the
> ssize_t cast, which results the value to be zero-extended, and
> as a result corrupt the negative values.
How does the following Git commit message look：

tpm: tpm_ppi: fix zero-extension of negative error codes

tpm_show_ppi_response() keeps its return value in an acpi_status,
a typedef of u32.  The two's complement of the error code is stored
correctly there, but on return the value is converted to ssize_t and
zero-extended, so the sign is lost: user space receives 0xFFFFFFEA
(4294967274) instead of -EINVAL, which breaks the usual "ret < 0" error
check.
Declare the variable as ssize_t so that negative values survive the
conversion.
> 
>>
>> Declare the variable as ssize_t to match the show callback's
>> return type.
>>
>> Fixes: 84b1667dea23 ("ACPI / TPM: replace open-coded _DSM code with helper functions")
>> Assisted-by: GLM-5.3
>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
>> ---
>>  drivers/char/tpm/tpm_ppi.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c
>> index c9793a3d986d..949fb7055bea 100644
>> --- a/drivers/char/tpm/tpm_ppi.c
>> +++ b/drivers/char/tpm/tpm_ppi.c
>> @@ -234,7 +234,7 @@ static ssize_t tpm_show_ppi_response(struct device *dev,
>>  				     struct device_attribute *attr,
>>  				     char *buf)
>>  {
>> -	acpi_status status = -EINVAL;
>> +	ssize_t status = -EINVAL;
>>  	union acpi_object *obj, *ret_obj;
>>  	u64 req, res;
>>  	struct tpm_chip *chip = to_tpm_chip(dev);
>> -- 
>> 2.25.1
>>
> 
> Br, Jarkko

Re: [PATCH 1/5] tpm: tpm_ppi: fix wrong error code returned to user space

[Jarkko Sakkinen]

On Mon, Oct 05, 2026 at 02:17:23PM +0800, Pei Xiao wrote:
> 
> 
> 在 2026/10/5 12:16, Jarkko Sakkinen 写道:
> > On Sat, Oct 03, 2026 at 04:27:51PM +0800, Pei Xiao wrote:
> >> tpm_show_ppi_response() stores its return value in an acpi_status,
> >> a typedef of u32.  Both error paths of the function (-EINVAL on a
> >> malformed _DSM package, -EFAULT on a non-zero operation return
> >> code) end up as huge positive values when returned as ssize_t, so
> >> user space cannot detect the failure with the usual "ret < 0"
> >> check.
> > 
> > I have no idea what you mean by huge value and why that would be
> > a problem, and overall this is disconnected from the code change.
> > 
> > Two's complement value is correctly stored in status up until the
> > ssize_t cast, which results the value to be zero-extended, and
> > as a result corrupt the negative values.
> How does the following Git commit message look：
> 
> tpm: tpm_ppi: fix zero-extension of negative error codes
> 
> tpm_show_ppi_response() keeps its return value in an acpi_status,
> a typedef of u32.  The two's complement of the error code is stored
> correctly there, but on return the value is converted to ssize_t and
> zero-extended, so the sign is lost: user space receives 0xFFFFFFEA
> (4294967274) instead of -EINVAL, which breaks the usual "ret < 0" error
> check.
> Declare the variable as ssize_t so that negative values survive the
> conversion.

Works for me. It does not have to be perfect as long as it points out
to the right direction.

> > 
> >>
> >> Declare the variable as ssize_t to match the show callback's
> >> return type.
> >>
> >> Fixes: 84b1667dea23 ("ACPI / TPM: replace open-coded _DSM code with helper functions")
> >> Assisted-by: GLM-5.3
> >> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> >> ---
> >>  drivers/char/tpm/tpm_ppi.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c
> >> index c9793a3d986d..949fb7055bea 100644
> >> --- a/drivers/char/tpm/tpm_ppi.c
> >> +++ b/drivers/char/tpm/tpm_ppi.c
> >> @@ -234,7 +234,7 @@ static ssize_t tpm_show_ppi_response(struct device *dev,
> >>  				     struct device_attribute *attr,
> >>  				     char *buf)
> >>  {
> >> -	acpi_status status = -EINVAL;
> >> +	ssize_t status = -EINVAL;
> >>  	union acpi_object *obj, *ret_obj;
> >>  	u64 req, res;
> >>  	struct tpm_chip *chip = to_tpm_chip(dev);
> >> -- 
> >> 2.25.1
> >>
> > 
> > Br, Jarkko
> 

Br, Jarkko