* [bug report] nvme-pci: implement host memory buffer support
@ 2017-07-17 10:34 Dan Carpenter
2017-07-17 23:34 ` Keith Busch
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2017-07-17 10:34 UTC (permalink / raw)
Hello Christoph Hellwig,
The patch 87ad72a59a38: "nvme-pci: implement host memory buffer
support" from May 12, 2017, leads to the following static checker
warning:
drivers/nvme/host/pci.c:1624 nvme_alloc_host_mem()
warn: should we be adding 'chunk_size' of the min_t value?
drivers/nvme/host/pci.c
1602 static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
1603 {
1604 struct nvme_host_mem_buf_desc *descs;
1605 u32 chunk_size, max_entries;
1606 int i = 0;
1607 void **bufs;
1608 u64 size = 0, tmp;
1609
1610 /* start big and work our way down */
1611 chunk_size = min(preferred, (u64)PAGE_SIZE << MAX_ORDER);
1612 retry:
1613 tmp = (preferred + chunk_size - 1);
1614 do_div(tmp, chunk_size);
1615 max_entries = tmp;
1616 descs = kcalloc(max_entries, sizeof(*descs), GFP_KERNEL);
1617 if (!descs)
1618 goto out;
1619
1620 bufs = kcalloc(max_entries, sizeof(*bufs), GFP_KERNEL);
1621 if (!bufs)
1622 goto out_free_descs;
1623
1624 for (size = 0; size < preferred; size += chunk_size) {
^^^^^^^^^^^^^^^^^^
The static checker is saying that maybe we should be doing
"size += len;" here. On the last iteration through the loop, then len
can be less than chunk_size.
1625 u32 len = min_t(u64, chunk_size, preferred - size);
1626 dma_addr_t dma_addr;
1627
1628 bufs[i] = dma_alloc_attrs(dev->dev, len, &dma_addr, GFP_KERNEL,
1629 DMA_ATTR_NO_KERNEL_MAPPING | DMA_ATTR_NO_WARN);
1630 if (!bufs[i])
1631 break;
1632
1633 descs[i].addr = cpu_to_le64(dma_addr);
1634 descs[i].size = cpu_to_le32(len / dev->ctrl.page_size);
1635 i++;
1636 }
1637
1638 if (!size || (min && size < min)) {
1639 dev_warn(dev->ctrl.device,
1640 "failed to allocate host memory buffer.\n");
1641 goto out_free_bufs;
1642 }
1643
1644 dev_info(dev->ctrl.device,
1645 "allocated %lld MiB host memory buffer.\n",
1646 size >> ilog2(SZ_1M));
1647 dev->nr_host_mem_descs = i;
1648 dev->host_mem_size = size;
^^^^^^^^^^^^^^^^^^^^^^^^^
Which means that we would be recording a slightly larger than actual
size in ->host_mem_size. I don't know the code well enough to say if
this matters.
1649 dev->host_mem_descs = descs;
1650 dev->host_mem_desc_bufs = bufs;
1651 return 0;
1652
1653 out_free_bufs:
1654 while (--i >= 0) {
1655 size_t size = le32_to_cpu(descs[i].size) * dev->ctrl.page_size;
1656
1657 dma_free_coherent(dev->dev, size, bufs[i],
1658 le64_to_cpu(descs[i].addr));
1659 }
1660
1661 kfree(bufs);
1662 out_free_descs:
1663 kfree(descs);
1664 out:
1665 /* try a smaller chunk size if we failed early */
1666 if (chunk_size >= PAGE_SIZE * 2 && (i == 0 || size < min)) {
1667 chunk_size /= 2;
1668 goto retry;
1669 }
1670 dev->host_mem_descs = NULL;
1671 return -ENOMEM;
1672 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* [bug report] nvme-pci: implement host memory buffer support
2017-07-17 10:34 [bug report] nvme-pci: implement host memory buffer support Dan Carpenter
@ 2017-07-17 23:34 ` Keith Busch
0 siblings, 0 replies; 2+ messages in thread
From: Keith Busch @ 2017-07-17 23:34 UTC (permalink / raw)
On Mon, Jul 17, 2017@01:34:04PM +0300, Dan Carpenter wrote:
> Hello Christoph Hellwig,
>
> The patch 87ad72a59a38: "nvme-pci: implement host memory buffer
> support" from May 12, 2017, leads to the following static checker
> warning:
>
> drivers/nvme/host/pci.c:1624 nvme_alloc_host_mem()
> warn: should we be adding 'chunk_size' of the min_t value?
>
> drivers/nvme/host/pci.c
> 1602 static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
> 1603 {
> 1604 struct nvme_host_mem_buf_desc *descs;
> 1605 u32 chunk_size, max_entries;
> 1606 int i = 0;
> 1607 void **bufs;
> 1608 u64 size = 0, tmp;
> 1609
> 1610 /* start big and work our way down */
> 1611 chunk_size = min(preferred, (u64)PAGE_SIZE << MAX_ORDER);
> 1612 retry:
> 1613 tmp = (preferred + chunk_size - 1);
> 1614 do_div(tmp, chunk_size);
> 1615 max_entries = tmp;
> 1616 descs = kcalloc(max_entries, sizeof(*descs), GFP_KERNEL);
> 1617 if (!descs)
> 1618 goto out;
> 1619
> 1620 bufs = kcalloc(max_entries, sizeof(*bufs), GFP_KERNEL);
> 1621 if (!bufs)
> 1622 goto out_free_descs;
> 1623
> 1624 for (size = 0; size < preferred; size += chunk_size) {
> ^^^^^^^^^^^^^^^^^^
> The static checker is saying that maybe we should be doing
> "size += len;" here. On the last iteration through the loop, then len
> can be less than chunk_size.
>
> 1625 u32 len = min_t(u64, chunk_size, preferred - size);
> 1626 dma_addr_t dma_addr;
> 1627
> 1628 bufs[i] = dma_alloc_attrs(dev->dev, len, &dma_addr, GFP_KERNEL,
> 1629 DMA_ATTR_NO_KERNEL_MAPPING | DMA_ATTR_NO_WARN);
> 1630 if (!bufs[i])
> 1631 break;
> 1632
> 1633 descs[i].addr = cpu_to_le64(dma_addr);
> 1634 descs[i].size = cpu_to_le32(len / dev->ctrl.page_size);
> 1635 i++;
> 1636 }
> 1637
> 1638 if (!size || (min && size < min)) {
> 1639 dev_warn(dev->ctrl.device,
> 1640 "failed to allocate host memory buffer.\n");
> 1641 goto out_free_bufs;
> 1642 }
> 1643
> 1644 dev_info(dev->ctrl.device,
> 1645 "allocated %lld MiB host memory buffer.\n",
> 1646 size >> ilog2(SZ_1M));
> 1647 dev->nr_host_mem_descs = i;
> 1648 dev->host_mem_size = size;
> ^^^^^^^^^^^^^^^^^^^^^^^^^
> Which means that we would be recording a slightly larger than actual
> size in ->host_mem_size. I don't know the code well enough to say if
> this matters.
Thanks for the report. It looks like the driver could be telling the
device it has more memory reserved for it than reality. There is no HMB
descriptor for that memory, so we're safe from corrupting anything, but I
think a device may choose to return an error if the total HSIZE and list
entries' BSIZE don't add up. I'll send a patch to correct this.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-07-17 23:34 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-17 10:34 [bug report] nvme-pci: implement host memory buffer support Dan Carpenter
2017-07-17 23:34 ` Keith Busch
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox