Re: [PATCH 09/16] dma-direct: Support PCI P2PDMA pages in dma-direct map_sg

[John Hubbard]

On 5/3/21 9:55 AM, Logan Gunthorpe wrote:
...
> > The same thing can be achieved with fewer lines and a bit more clarity.
> > Can we please do it like this instead:
> >
> > 	for_each_sg(sgl, sg, nents, i) {
> > 		if (sg_is_pci_p2pdma(sg))
> > 			sg_unmark_pci_p2pdma(sg);
> > 		else
> > 			dma_direct_unmap_page(dev, sg->dma_address,
> > 					      sg_dma_len(sg), dir, attrs);
> > 	}
>
> That's debatable (the way I did it emphasizes the common case). But I'll
> consider changing it.

Thanks for considering it.

> > Also here, a block comment for the function would be nice. How about
> > approximately this:
> >
> > /*
> >    * Maps each SG segment. Returns the number of entries mapped, or 0 upon
> >    * failure. If any entry could not be mapped, then no entries are mapped.
> >    */
> >
> > I'll stop complaining about the pre-existing return code conventions,
> > since by now you know what I was thinking of saying. :)
>
> Not really part of this patchset... Seems like if you think there should
> be a comment like that here, you should send a patch. But this patch
> starts returning a negative value here.

OK, that's fine. Like you say, that comment is rather beyond this patchset.

> > >    	for_each_sg(sgl, sg, nents, i) {
> > > +		if (is_pci_p2pdma_page(sg_page(sg))) {
> > > +			ret = pci_p2pdma_map_segment(&p2pdma_state, dev, sg,
> > > +						     attrs);
> > > +			if (ret < 0) {
> > > +				goto out_unmap;
> > > +			} else if (ret) {
> > > +				ret = 0;
> > > +				continue;
> >
> > Is this a bug? If neither of those "if" branches fires (ret == 0), then
> > the code (probably unintentionally) falls through and continues on to
> > attempt to call dma_direct_map_page()--despite being a PCI_P2PDMA page!
>
> No, it's not a bug. Per the documentation of pci_p2pdma_map_segment(),
> if it returns zero the segment should be mapped normally. P2PDMA pages
> must be mapped with physical addresses (or IOVA addresses) if the TLPS
> for the transaction will go through the host bridge.

Could we maybe put a little comment there, to that effect? It would be
nice to call out that point, especially since it is common to miss one
case (negative, 0, positive) when handling return values. Sort of like
we used to put "// fallthrough" in the case statements. Not a big deal
of course.

> > See below for suggestions:
> >
> > > +			}
> > > +		}
> > > +
> > >    		sg->dma_address = dma_direct_map_page(dev, sg_page(sg),
> > >    				sg->offset, sg->length, dir, attrs);
> > >    		if (sg->dma_address == DMA_MAPPING_ERROR)
> >
> > This is another case in which "continue" is misleading and not as good
> > as "else". Because unless I'm wrong above, you really only want to take
> > one path *or* the other.
>
> No, per above, it's not one path or the other. If it's a P2PDMA page it
> may still need to be mapped normally.

Right. That follows.

thanks,
--
John Hubbard
NVIDIA