Re: [PATCH v2 05/14] powerpc/kernel/iommu: Add new iommu_table_in_use() helper

[Leonardo Bras <leobras.c@gmail.com>]

Hello Alexey, thanks for the feedback!

On Tue, 2020-09-29 at 13:57 +1000, Alexey Kardashevskiy wrote:
> 
> On 12/09/2020 03:07, Leonardo Bras wrote:
> > diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> > index ffb2637dc82b..c838da3d8f32 100644
> > --- a/arch/powerpc/kernel/iommu.c
> > +++ b/arch/powerpc/kernel/iommu.c
> > @@ -655,34 +655,21 @@ static void iommu_table_reserve_pages(struct iommu_table *tbl,
> >   	if (tbl->it_offset == 0)
> >   		set_bit(0, tbl->it_map);
> >   
> > 
> > 
> > 
> > +	/* Check if res_start..res_end is a valid range in the table */
> > +	if (res_start >= res_end || res_start < tbl->it_offset ||
> > +	    res_end > (tbl->it_offset + tbl->it_size)) {
> > +		tbl->it_reserved_start = tbl->it_offset;
> > +		tbl->it_reserved_end = tbl->it_offset;
> 
> 
> This silently ignores overlapped range of the reserved area and the 
> window which does not seem right.

Humm, that makes sense.
Would it be better to do something like this?

if (res_start < tbl->it_offset) 
	res_start = tbl->it_offset;

if (res_end > (tbl->it_offset + tbl->it_size))
	res_end = tbl->it_offset + tbl->it_size;

if (res_start >= res_end) {
	tbl->it_reserved_start = tbl->it_offset;
	tbl->it_reserved_end = tbl->it_offset;
	return;
}


> > +		return;
> > +	}
> > +
> >   	tbl->it_reserved_start = res_start;
> >   	tbl->it_reserved_end = res_end;

> >   -	/* Check if res_start..res_end isn't empty and overlaps the table */
> > -	if (res_start && res_end &&
> > -			(tbl->it_offset + tbl->it_size < res_start ||
> > -			 res_end < tbl->it_offset))
> > -		return;
> > -
> >   	for (i = tbl->it_reserved_start; i < tbl->it_reserved_end; ++i)
> >   		set_bit(i - tbl->it_offset, tbl->it_map);
> >   }
> > +bool iommu_table_in_use(struct iommu_table *tbl)
> > +{
> > +	unsigned long start = 0, end;
> > +
> > +	/* ignore reserved bit0 */
> > +	if (tbl->it_offset == 0)
> > +		start = 1;
> > +	end = tbl->it_reserved_start - tbl->it_offset;
> > +	if (find_next_bit(tbl->it_map, end, start) != end)
> > +		return true;
> > +
> > +	start = tbl->it_reserved_end - tbl->it_offset;
> > +	end = tbl->it_size;
> > +	return find_next_bit(tbl->it_map, end, start) != end;
> > +
> 
> Unnecessary empty line.

Sure, removing. 
Thanks!