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

[Jarkko Sakkinen <jarkko@kernel.org>]

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