Re: [PATCH v5 02/11] powerpc/kernel/iommu: Add new iommu_table_in_use() helper

[Alexey Kardashevskiy]

On 20/07/2021 15:38, Leonardo Brás wrote:
> Hello Fred, thanks for this feedback!
>
> Sorry if I miss anything, this snippet was written for v1 over an year
> ago, and I have not taken a look at it ever since.
>
> On Mon, 2021-07-19 at 15:53 +0200, Frederic Barrat wrote:
> > On 16/07/2021 10:27, Leonardo Bras wrote:
> > > @@ -1099,18 +1105,13 @@ int iommu_take_ownership(struct iommu_table
> > > *tbl)
> > >          for (i = 0; i < tbl->nr_pools; i++)
> > >                  spin_lock_nest_lock(&tbl->pools[i].lock, &tbl-
> > > > large_pool.lock);
> > >    
> > > -       iommu_table_release_pages(tbl);
> > > -
> > > -       if (!bitmap_empty(tbl->it_map, tbl->it_size)) {
> > > +       if (iommu_table_in_use(tbl)) {
> > >                  pr_err("iommu_tce: it_map is not empty");
> > >                  ret = -EBUSY;
> > > -               /* Undo iommu_table_release_pages, i.e. restore
> > > bit#0, etc */
> > > -               iommu_table_reserve_pages(tbl, tbl-
> > > > it_reserved_start,
> > > -                               tbl->it_reserved_end);
> > > -       } else {
> > > -               memset(tbl->it_map, 0xff, sz);
> > >          }
> > >    
> > > +       memset(tbl->it_map, 0xff, sz);
> > > +
> >
> >
> > So if the table is not empty, we fail (EBUSY) but we now also
> > completely
> > overwrite the bitmap. It was in an unexpected state, but we're making
> > it
> > worse. Or am I missing something?
>
> IIRC there was a reason to do that at the time, but TBH I don't really
> remember it, and by looking at the code right now you seem to be
> correct about this causing trouble.
>
> I will send a v6 fixing it soon.
> Please review the remaining patches for some issue I may be missing.
>
> Alexey, any comments on that?


Agree with Fred, this is a bug, EBUSY is not that unexpected :-/ Thanks,


> >     Fred
>
> Again, thank you for reviewing Fred!
> Best regards,
> Leonardo Bras

--
Alexey