* [PATCH] tpm_crb: Remove dead code from crb_map_res() @ 2026-09-01 14:29 Jarkko Sakkinen 2026-09-01 16:06 ` Stefano Garzarella 0 siblings, 1 reply; 5+ messages in thread From: Jarkko Sakkinen @ 2026-09-01 14:29 UTC (permalink / raw) To: linux-integrity Cc: Jarkko Sakkinen, Peter Huewe, Jason Gunthorpe, Rafael J. Wysocki, Stuart Yoder, Chu Guangqing, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tpm_crb: Remove dead code from crb_map_res() 2026-09-01 14:29 [PATCH] tpm_crb: Remove dead code from crb_map_res() Jarkko Sakkinen @ 2026-09-01 16:06 ` Stefano Garzarella 2026-09-01 16:55 ` Jarkko Sakkinen 0 siblings, 1 reply; 5+ messages in thread From: Stefano Garzarella @ 2026-09-01 16:06 UTC (permalink / raw) To: Jarkko Sakkinen Cc: linux-integrity, Peter Huewe, Jason Gunthorpe, Rafael J. Wysocki, Stuart Yoder, Chu Guangqing, linux-kernel 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 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tpm_crb: Remove dead code from crb_map_res() 2026-09-01 16:06 ` Stefano Garzarella @ 2026-09-01 16:55 ` Jarkko Sakkinen 2026-09-02 8:54 ` Stefano Garzarella 0 siblings, 1 reply; 5+ messages in thread From: Jarkko Sakkinen @ 2026-09-01 16:55 UTC (permalink / raw) To: Stefano Garzarella Cc: linux-integrity, Peter Huewe, Jason Gunthorpe, Rafael J. Wysocki, Stuart Yoder, Chu Guangqing, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tpm_crb: Remove dead code from crb_map_res() 2026-09-01 16:55 ` Jarkko Sakkinen @ 2026-09-02 8:54 ` Stefano Garzarella 2026-09-09 16:01 ` Jarkko Sakkinen 0 siblings, 1 reply; 5+ messages in thread From: Stefano Garzarella @ 2026-09-02 8:54 UTC (permalink / raw) To: Jarkko Sakkinen Cc: linux-integrity, Peter Huewe, Jason Gunthorpe, Rafael J. Wysocki, Stuart Yoder, Chu Guangqing, linux-kernel 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); > } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tpm_crb: Remove dead code from crb_map_res() 2026-09-02 8:54 ` Stefano Garzarella @ 2026-09-09 16:01 ` Jarkko Sakkinen 0 siblings, 0 replies; 5+ messages in thread From: Jarkko Sakkinen @ 2026-09-09 16:01 UTC (permalink / raw) To: Stefano Garzarella Cc: linux-integrity, Peter Huewe, Jason Gunthorpe, Rafael J. Wysocki, Stuart Yoder, Chu Guangqing, linux-kernel On Wed, Sep 02, 2026 at 10:54:32AM +0200, Stefano Garzarella wrote: > 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); > > } > Thanks for the remarks and ack for receiving this ;-) I'll move this to my TODO-folder and read it with thought some days from now. BR, Jarkko ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-09-09 16:01 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-09-01 14:29 [PATCH] tpm_crb: Remove dead code from crb_map_res() Jarkko Sakkinen 2026-09-01 16:06 ` Stefano Garzarella 2026-09-01 16:55 ` Jarkko Sakkinen 2026-09-02 8:54 ` Stefano Garzarella 2026-09-09 16:01 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®