[PATCH] tpm_crb: Remove dead code from crb_map_res()

[PATCH] tpm_crb: Remove dead code from crb_map_res()

[Jarkko Sakkinen]

In all the pre-existing call sites both @iomem and @iobase_ptr are
either NULL or non-NULL.

Thus, add invariant for this and remove a dead basic block.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 drivers/char/tpm/tpm_crb.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index ceb4100ba400..e7a61f36c58b 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -570,15 +570,12 @@ static void __iomem *crb_map_res(struct device *dev, struct resource *iores,
 	if (start != new_res.start)
 		return IOMEM_ERR_PTR(-EINVAL);
 
+	if ((iores == NULL) != (iobase_ptr == NULL))
+		return IOMEM_ERR_PTR(-EINVAL);
+
 	if (!iores)
 		return devm_ioremap_resource(dev, &new_res);
 
-	if (!*iobase_ptr) {
-		*iobase_ptr = devm_ioremap_resource(dev, iores);
-		if (IS_ERR(*iobase_ptr))
-			return *iobase_ptr;
-	}
-
 	return *iobase_ptr + (new_res.start - iores->start);
 }
 
-- 
2.47.3

Re: [PATCH] tpm_crb: Remove dead code from crb_map_res()

[Stefano Garzarella]

On Tue, Sep 01, 2026 at 05:29:46PM +0300, Jarkko Sakkinen wrote:
>In all the pre-existing call sites both @iomem and @iobase_ptr are
>either NULL or non-NULL.

I don't know this code, but I'm a bit worried about iobase_ptr and 
*iobase_ptr. IIUC it is true that iobase_ptr and iores are either NULL 
or non-NULL, but here we are removing the case where *iobase_ptr is 
NULL.

Now looking at crb_map_io(), IIUC iobase_array is initialized with NULL 
pointers and the code we are removing was the only one initializing 
those pointers IIUC, or am I missing something?

Thanks,
Stefano

>
>Thus, add invariant for this and remove a dead basic block.
>
>Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>---
> drivers/char/tpm/tpm_crb.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
>index ceb4100ba400..e7a61f36c58b 100644
>--- a/drivers/char/tpm/tpm_crb.c
>+++ b/drivers/char/tpm/tpm_crb.c
>@@ -570,15 +570,12 @@ static void __iomem *crb_map_res(struct device *dev, struct resource *iores,
> 	if (start != new_res.start)
> 		return IOMEM_ERR_PTR(-EINVAL);
>
>+	if ((iores == NULL) != (iobase_ptr == NULL))
>+		return IOMEM_ERR_PTR(-EINVAL);
>+
> 	if (!iores)
> 		return devm_ioremap_resource(dev, &new_res);
>
>-	if (!*iobase_ptr) {
>-		*iobase_ptr = devm_ioremap_resource(dev, iores);
>-		if (IS_ERR(*iobase_ptr))
>-			return *iobase_ptr;
>-	}
>-
> 	return *iobase_ptr + (new_res.start - iores->start);
> }
>
>-- 
>2.47.3
>
>

Re: [PATCH] tpm_crb: Remove dead code from crb_map_res()

[Jarkko Sakkinen]

On Tue, Sep 01, 2026 at 06:06:40PM +0200, Stefano Garzarella wrote:
> On Tue, Sep 01, 2026 at 05:29:46PM +0300, Jarkko Sakkinen wrote:
> > In all the pre-existing call sites both @iomem and @iobase_ptr are
> > either NULL or non-NULL.
> 
> I don't know this code, but I'm a bit worried about iobase_ptr and
> *iobase_ptr. IIUC it is true that iobase_ptr and iores are either NULL or
> non-NULL, but here we are removing the case where *iobase_ptr is NULL.
> 
> Now looking at crb_map_io(), IIUC iobase_array is initialized with NULL
> pointers and the code we are removing was the only one initializing those
> pointers IIUC, or am I missing something?

crb_map_io() sets both to non-NULL value, or leaves both as NULL.

crb_map_pluton() explicitly calls both explicitly with NULL.

If anything else will arrive too crb_map_res, that'd be unexpected
input, which without this patch will go unnoticed and will lead to
undefined behavior.

Not sure what is the argument here really.

> 
> Thanks,
> Stefano

BR, Jarkko

Re: [PATCH] tpm_crb: Remove dead code from crb_map_res()

[Stefano Garzarella]

On Tue, Sep 01, 2026 at 07:55:27PM +0300, Jarkko Sakkinen wrote:
>On Tue, Sep 01, 2026 at 06:06:40PM +0200, Stefano Garzarella wrote:
>> On Tue, Sep 01, 2026 at 05:29:46PM +0300, Jarkko Sakkinen wrote:
>> > In all the pre-existing call sites both @iomem and @iobase_ptr are
>> > either NULL or non-NULL.
>>
>> I don't know this code, but I'm a bit worried about iobase_ptr and
>> *iobase_ptr. IIUC it is true that iobase_ptr and iores are either NULL or
>> non-NULL, but here we are removing the case where *iobase_ptr is NULL.
>>
>> Now looking at crb_map_io(), IIUC iobase_array is initialized with NULL
>> pointers and the code we are removing was the only one initializing those
>> pointers IIUC, or am I missing something?
>
>crb_map_io() sets both to non-NULL value, or leaves both as NULL.
>
>crb_map_pluton() explicitly calls both explicitly with NULL.
>
>If anything else will arrive too crb_map_res, that'd be unexpected
>input, which without this patch will go unnoticed and will lead to
>undefined behavior.

This is clear, and it's what is done in the first hunk, what is not 
clear to me is why removing the second hunk.

>
>Not sure what is the argument here really.

Sorry, I should have commented in the diff:

>diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
>index ceb4100ba400..e7a61f36c58b 100644
>--- a/drivers/char/tpm/tpm_crb.c
>+++ b/drivers/char/tpm/tpm_crb.c
>@@ -570,15 +570,12 @@ static void __iomem *crb_map_res(struct device *dev, struct resource *iores,
> 	if (start != new_res.start)
> 		return IOMEM_ERR_PTR(-EINVAL);
>
>+	if ((iores == NULL) != (iobase_ptr == NULL))
>+		return IOMEM_ERR_PTR(-EINVAL);
>+

This makes sense to me.

> 	if (!iores)
> 		return devm_ioremap_resource(dev, &new_res);
>
>-	if (!*iobase_ptr) {
>-		*iobase_ptr = devm_ioremap_resource(dev, iores);
>-		if (IS_ERR(*iobase_ptr))
>-			return *iobase_ptr;
>-	}
>-

This is unclear to me, here we are checking if the value stored in 
iobase_ptr is NULL (so something different from the check we are adding 
above, but appropriate because it only makes sense when both are 
non-NULL). If the value stored in the pointer is NULL we are setting it.
Looking at the code, I can't see any other point where that values 
(iobase_array[]) are initialized, but again, I don't know this code, so 
I may missing something.

Stefano

> 	return *iobase_ptr + (new_res.start - iores->start);
> }