>> +static void nvme_pci_sgl_set_data_addr(struct nvme_sgl_desc *sge,
>> + dma_addr_t addr, u32 len)
>> +{
>> + sge->addr = cpu_to_le64(addr);
>> + sge->length = cpu_to_le32(len);
>> + sge->type = NVME_SGL_FMT_DATA_DESC << 4;
>> +}
>> +
>> static void nvme_pci_sgl_set_data(struct nvme_sgl_desc *sge,
>> struct blk_dma_iter *iter)
>> {
>> - sge->addr = cpu_to_le64(iter->addr);
>> - sge->length = cpu_to_le32(iter->len);
>> - sge->type = NVME_SGL_FMT_DATA_DESC << 4;
>> + nvme_pci_sgl_set_data_addr(sge, iter->addr, iter->len);
>> }
>
> The naming is a bit confusing (and me passing the iter to
> nvme_pci_sgl_set_data is probably at faul for that). So maybe
> spin out a prep patch to rename the old nvme_pci_sgl_set_data
> to nvme_pci_dma_iter_set_sgl or so, and then add the new one
> as nvme_pci_sgl_set_data (as before the dma_iter conversion).
>
Thanks for the detailed review! Will split the rename into a prep patch.
>>
>> +static unsigned int nvme_pci_dmabuf_sgl_nents(struct request *req,
>> + dma_addr_t *first_dma,
>> + u32 *first_len)
>
> This is a really good example why the aligning to the opening braces
> produces totally unreadble code..
>
> But I also don't understand what the use case for this function
> is to start with. struct sg_table tells us how many segments
> exist on the DMA side in the nents member, which should be just
> fine for the SGL threshold calculation.
sg_table->nents covers the entire exported buffer (<=1GiB), while a
request only covers a subrange[bi_offset, bi_offset+payload). Using
nents would overcount the request's segments.
>
>> +{
>> + struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>> + struct bio *bio = req->bio;
>> + struct nvme_dmabuf_map *map = to_nvme_dmabuf_map(bio->bi_dmabuf_map);
>> + size_t length = blk_rq_payload_bytes(req);
>> + struct nvme_sgl_desc *sg_list = NULL;
>> + dma_addr_t sgl_dma = 0, last_end = 0;
>> + unsigned int mapped = 0;
>> + unsigned long tmp;
>> + struct scatterlist *sg;
>> + size_t offset, remaining;
>> + bool have = false;
>> +
>> + if (!entries)
>> + return BLK_STS_IOERR;
>> + if (entries > NVME_MAX_SEGS)
>> + return BLK_STS_AGAIN;
>
> Given that the block layer enforced data in rw/command and the
> max_segments limit, why do we need the extra check here?
A dmabuf bio reports nsegs=1 (bio_split_io_at) to the block layer, so
max_segments isn't enforced against the SG entries actually spanned by
the request. Hence the explicit check.
>
>> + continue;
>> + }
>> +
>> + addr += offset;
>> + sg_len -= offset;
>> + offset = 0;
>> +
>> + while (sg_len && remaining) {
>
> These can't be false on the first iteration, so maybe turn this into
> a do {} while loop?
Will do.
>
>> + u32 chunk = min_t(size_t, remaining, sg_len);
>> +
>> + if (have && last_end == addr) {
>> + u32 old = le32_to_cpu(sg_list[mapped -
>> 1].length);
>> +
>> + sg_list[mapped - 1].length = cpu_to_le32(old +
>> chunk);
>
> Overly long line.
>
>> + } else {
>> + if (WARN_ON_ONCE(mapped == entries))
>> + goto err_free;
>> + nvme_pci_sgl_set_data_addr(&sg_list[mapped++],
>> + addr, chunk);
>> + }
>
> Why do we need this merging? dma_map_sg should have already done
> any interesting merging, or am I missing something?
Will drop it.
