On Wed, Feb 27, 2019 at 11:06:14AM +0200, Gal Pressman wrote:
> >>>> +static struct scatterlist *efa_vmalloc_buf_to_sg(u64 *buf, int page_cnt)
> >>>> +{
> >>>> + struct scatterlist *sglist;
> >>>> + struct page *pg;
> >>>> + int i;
> >>>> +
> >>>> + sglist = kcalloc(page_cnt, sizeof(*sglist), GFP_KERNEL);
> >>>> + if (!sglist)
> >>>> + return NULL;
> >>>> + sg_init_table(sglist, page_cnt);
> >>>> + for (i = 0; i < page_cnt; i++) {
> >>>> + pg = vmalloc_to_page(buf);
> >>>> + if (!pg)
> >>>> + goto err;
> >>>> + WARN_ON_ONCE(PageHighMem(pg));
> >>>
> >>> Is this WARN_ON_ONCE() really an error that needs to be handled?
> >>
> >> AFAIK, there is no way we can actually get a higemem page here.
> >> The WARN is here from early dev days, it should probably be removed.
> >>
> >>>
> >>>
> >>>> + sg_set_page(&sglist[i], pg, EFA_PAGE_SIZE, 0);
> >>>> + buf = (u64 *)((u8 *)buf + EFA_PAGE_SIZE);
> >
> > Why do you need special EFA_PAGE_SIZE? Isn't PAGE_SIZE enough for you?
>
> EFA_PAGE_SIZE represents the device page size.
Something is wrong here then, as vmalloc_to_page returns something
that is *at most* PAGE_SIZE long. You can't go around and pass that
into sg_set_page with EFA_PAGE_SIZE > PAGE_SIZE.
Maybe this code is wrongly assuming PAGE_SIZE > EFA_PAGE_SIZE
?
What you need here is to make the scatter list out of PAGE_SIZE blocks
and then use Shiraz's work to fragment it into EFA_PAGE_SIZE blocks
for building your pbl. dma_map_sg should use the largest sgls possible
for efficiency.
Also, I think there are now several places in the kernel converting
vmalloc/kmalloc regions into scatterlist tables, it probably warrents
adding a shared helper in lib/scatterlist.c that does a good and
proper job of this.
> >>>> + buf = (u64 *)((u8 *)buf + EFA_PAGE_SIZE);
Yuk
buf += EFA_PAGE_SIZE/sizeof(*buf);
Jason
