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
