On Wed, Jan 02, 2019 at 05:02:27PM -0700, Jason Gunthorpe wrote:
> On Mon, Dec 24, 2018 at 04:32:23PM -0600, Shiraz Saleem wrote:
> > This helper iterates through the SG list to find the best page size to use
> > from a bitmap of HW supported page sizes. Drivers that support multiple
> > page sizes, but not mixed pages in an MR can use this API.
> >
[..]
> > +static unsigned int ib_umem_find_pg_bit(unsigned long phyaddr,
> > + unsigned long supported_pgsz)
> > +{
> > + unsigned long num_zeroes;
> > + unsigned long pgsz;
> > +
> > + /* Trailing zero bits in the address */
> > + num_zeroes = __ffs(phyaddr);
> > +
> > + /* Find page bit such that phyaddr is aligned to the highest supported
> > + * HW page size
> > + */
> > + pgsz = supported_pgsz & (BIT_ULL(num_zeroes + 1) - 1);
> > + if (!pgsz)
> > + return __ffs(supported_pgsz);
> > +
> > + return (fls64(pgsz) - 1);
>
> extra brackets
OK.
> > +/**
> > + * ib_umem_find_single_pg_size - Find best HW page size to use for this MR
> > + * @umem: umem struct
> > + * @supported_pgsz: bitmask of HW supported page sizes
> > + * @uvirt_addr: user-space virtual MR base address
> > + *
> > + * This helper is intended for HW that support multiple page
> > + * sizes but can do only a single page size in an MR.
>
> Returns 0 if the umem requires page sizes not supported by the driver
> to be mapped. Drivers always supporting PAGE_SIZE will never see a 0
> result.
OK.
> > + */
> > +unsigned long ib_umem_find_single_pg_size(struct ib_umem *umem,
> > + unsigned long supported_pgsz,
> > + unsigned long uvirt_addr)
> > +{
> > + struct scatterlist *sg;
> > + unsigned int pg_bit_sg, min_pg_bit, best_pg_bit;
> > + int i;
> > +
> > + if (!supported_pgsz)
> > + return 0;
>
> Maybe like:
>
> if (WARN_ON(!(BIT_ULL(PAGE_SHIFT) & supported_pgsz)))
> return 0;
>
> ?
>
> Drivers are not allowed to not support the system page size.
>
> And also I guess it would be reasonable to have something like
>
> /* Driver only supports the system page size, so no further checks required */
> if (supported_pgsz >> PAGE_SHIFT == 1)
> return PAGE_SIZE;
>
Yes, these 2 checks are warranted.
Shiraz
