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);
}
