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
