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

[PATCH v2] 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
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) {
-		*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 v2] tpm_crb: Remove dead code from crb_map_res()

[Stefano Garzarella]

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.

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
> 

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

[Jarkko Sakkinen]

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

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

[Jarkko Sakkinen]

On Mon, Oct 12, 2026 at 04:31:45PM +0300, Jarkko Sakkinen wrote:
> 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.

Probably too much multitasking lately with all these bugs.

Lessons learned is that from now on I keep my own contributions
and reviewing in separate sessions. Things have gotten much
busier than they used to be so need to adapt and learn how
to have focus on all actitivies :-)

Br, Jarkko

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

[Stefano Garzarella]

On Mon, Oct 12, 2026 at 04:53:52PM +0300, Jarkko Sakkinen wrote:
>On Mon, Oct 12, 2026 at 04:31:45PM +0300, Jarkko Sakkinen wrote:
>> 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.

Yeah, a lazy init makes sense.

>>
>> 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 :-)

You're welcome!

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

Don't beat yourself up, it happens to all of us! Thanks for taking the 
time to check and clarify!

>
>Probably too much multitasking lately with all these bugs.
>
>Lessons learned is that from now on I keep my own contributions
>and reviewing in separate sessions. Things have gotten much
>busier than they used to be so need to adapt and learn how
>to have focus on all actitivies :-)

Yeah, it's something that's happening here too, and it's a real pain to 
deal with! :-(

Thanks,
Stefano