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

[Stefano Garzarella <sgarzare@redhat.com>]

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