On 5/4/2023 10:50 AM, Jason Gunthorpe wrote:
Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
On Fri, Apr 21, 2023 at 06:06:39PM -0700, Brett Creeley wrote:
+static int
+pds_vfio_dma_map_lm_file(struct device *dev, enum dma_data_direction dir,
+ struct pds_vfio_lm_file *lm_file)
+{
+ struct pds_lm_sg_elem *sgl, *sge;
+ struct scatterlist *sg;
+ dma_addr_t sgl_addr;
+ size_t sgl_size;
+ int err;
+ int i;
+
+ if (!lm_file)
+ return -EINVAL;
+
+ /* dma map file pages */
+ err = dma_map_sgtable(dev, &lm_file->sg_table, dir, 0);
+ if (err)
+ return err;
+
+ lm_file->num_sge = lm_file->sg_table.nents;
+
+ /* alloc sgl */
+ sgl_size = lm_file->num_sge * sizeof(struct pds_lm_sg_elem);
+ sgl = kzalloc(sgl_size, GFP_KERNEL);
+ if (!sgl) {
+ err = -ENOMEM;
+ goto err_alloc_sgl;
+ }
+
+ sgl_addr = dma_map_single(dev, sgl, sgl_size, DMA_TO_DEVICE);
+ if (dma_mapping_error(dev, sgl_addr)) {
+ err = -EIO;
+ goto err_map_sgl;
+ }
+
+ lm_file->sgl = sgl;
+ lm_file->sgl_addr = sgl_addr;
+
+ /* fill sgl */
+ sge = sgl;
+ for_each_sgtable_dma_sg(&lm_file->sg_table, sg, i) {
+ sge->addr = cpu_to_le64(sg_dma_address(sg));
+ sge->len = cpu_to_le32(sg_dma_len(sg));
+ dev_dbg(dev, "addr = %llx, len = %u\n", sge->addr, sge->len);
+ sge++;
+ }
This sequence is in the wrong order, the dma_map_single() has to be
after the data is written to the memory as it synchronizes the caches
on some arches
This is an easy enough fix and I'll keep this in mind moving forward.
Thanks for pointing it out.
+
+ return 0;
+
+err_map_sgl:
+ kfree(sgl);
+err_alloc_sgl:
+ dma_unmap_sgtable(dev, &lm_file->sg_table, dir, 0);
+ return err;
And why is the goto error unwind in a messed up order? Error unwinds
should always be strictly in the opposite order of the success
path. Check them all when you are fixing the "come from" notation
Jason
Yeah, this wasn't on purpose. Thanks again for pointing things like this
out. As you noted I will go through the entire series to make sure the
goto labels are named by what they are freeing/clearing and also that
they are in the correct opposite order.
Thanks again for the review.
Brett
