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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Mon, Oct 12, 2026 at 12:23:05PM +0200, Stefano Garzarella wrote:
> On Sun, Oct 11, 2026 at 02:54:57AM +0300, Jarkko Sakkinen wrote:
> > In all the pre-existing call sites both @iomem and @iobase_ptr are either
> 
> Do you mean @iores ?
> 
> > both NULL or have a valid address.
> > 
> > Add an invariant for the expeced conditions and remove a dead basic block.
> > The second block is dead given that if @iomem turns out not to be NULL,
> > neither can't be @iobase_ptr.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > v2:
> > - Updated the commit message in order to clarify things better.
> > - Updated the invariant in order to address checkpatch.pl --strict
> >   errors.
> >  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..833226966fa2 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 != !iobase_ptr)
> > +		return IOMEM_ERR_PTR(-EINVAL);
> > +
> >  	if (!iores)
> >  		return devm_ioremap_resource(dev, &new_res);
> > 
> > -	if (!*iobase_ptr) {
> 
> Sorry to insist, but in the commit description we talk about @iobase_ptr 
> can't be null, and I agree, but here we were checking the value of the 
> pointer, so IMO this code was not dead.
> 
> But again, I know nothing on this code. So, if you think it's correct go 
> head, I'm surely missing something.

Ugh, you're actually right, and this is not, and should not have sent
this. And it is for lazy init. 

I need to sanity check my test environment because I thought I was
testing this with swtpm emulated tpm_crb but tests passed, and this
should not work.

Sorry and thanks :-)

Just wanted to make it explicit that I was making somehting extremely
stupid.

> 
> Thanks,
> Stefano
> 
> > -		*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
> > 
> 

Br, Jarkko