Re: [Qemu-devel] [PATCH v5 1/4] net/rocker: Remove the dead error handling
On Wed, 24 May 2017 08:01:47 -0400 (EDT) Marcel Apfelbaum <mar...@redhat.com> wrote: > - Original Message - > > From: "Markus Armbruster" <arm...@redhat.com> > > To: "Philippe Mathieu-Daudé" <f4...@amsat.org> > > Cc: qemu-devel@nongnu.org, "Mao Zhongyi" <maozy.f...@cn.fujitsu.com>, > > j...@resnulli.us, jasow...@redhat.com, "Michael > > S. Tsirkin" <m...@redhat.com>, "Marcel Apfelbaum" <mar...@redhat.com> > > Sent: Wednesday, May 24, 2017 8:35:04 AM > > Subject: Re: [Qemu-devel] [PATCH v5 1/4] net/rocker: Remove the dead error > > handling > > > > Philippe Mathieu-Daudé <f4...@amsat.org> writes: > > > > > Hi Markus, > > > > > > On 05/23/2017 06:27 AM, Markus Armbruster wrote: > > > [...] > > >> There's one more cleanup opportunity: > > >> > > > [...] > > >>> if (pci_dma_read(dev, le64_to_cpu(info->desc.buf_addr), info->buf, > > >>> size)) { > > >>> return NULL; > > >>> } > > >> > > >> None of the pci_dma_read() calls outside rocker check the return value. > > >> Just as well, because it always returns 0. Please clean this up in a > > >> separate followup patch. > > > > > > It may be the correct way to do it but this sounds like we are missing > > > something somewhere... pci_dma_read() calls pci_dma_rw() which always > > > returns 0. Why not let it returns void? It is inlined and never used > > > by address. Else we should document why returning 0 is correct, and > > > what is the reason to not use a void prototype. > > > > > > pci_dma_rw() calls dma_memory_rw() which does return a boolean value, > > > false on success (MEMTX_OK) and true on error > > > (MEMTX_ERROR/DECODE_ERROR) > > > > PCI question. Michael, Marcel? > > > > Hi Markus, > > Looking at the git history, pci_dma_rw used to call cpu_physical_memory_rw > which, at that time (commit ec17457), returned void. Since the interface > dictated > to return int, 0 is returned as "always OK". > > The callers to pci_dma_read did not bother to check it for obvious reasons > (even if they should). > > In the meantime the memory API has changed to allow returning errors, but > since the callers of > pci_dma_rw don't check the return value, why bother to update the PCI DMA? > > History aside (and my speculations above), it seems the right move is to > update > the return value and check it by callers, but honestly I don't have any idea > if the emulated devices expect pci dma to fail. > Adding Paolo and David for more insights. It seems to me that PCI DMA transfers ought to be able to fail, and devices ought to be able to handle that (to a limited extent). After all, what will happen if you try to DMA to PCI addresses that simply aren't mapped. Or which are in the domain of a vIOMMU which wither hasn't mapped those addreses, or has them mapped read-only (meaning host-to-device only in this context). -- David Gibson <dgib...@redhat.com> Principal Software Engineer, Virtualization, Red Hat pgpQEQtzVxx2q.pgp Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v5 1/4] net/rocker: Remove the dead error handling
- Original Message - > From: "Markus Armbruster" <arm...@redhat.com> > To: "Philippe Mathieu-Daudé" <f4...@amsat.org> > Cc: qemu-devel@nongnu.org, "Mao Zhongyi" <maozy.f...@cn.fujitsu.com>, > j...@resnulli.us, jasow...@redhat.com, "Michael > S. Tsirkin" <m...@redhat.com>, "Marcel Apfelbaum" <mar...@redhat.com> > Sent: Wednesday, May 24, 2017 8:35:04 AM > Subject: Re: [Qemu-devel] [PATCH v5 1/4] net/rocker: Remove the dead error > handling > > Philippe Mathieu-Daudé <f4...@amsat.org> writes: > > > Hi Markus, > > > > On 05/23/2017 06:27 AM, Markus Armbruster wrote: > > [...] > >> There's one more cleanup opportunity: > >> > > [...] > >>> if (pci_dma_read(dev, le64_to_cpu(info->desc.buf_addr), info->buf, > >>> size)) { > >>> return NULL; > >>> } > >> > >> None of the pci_dma_read() calls outside rocker check the return value. > >> Just as well, because it always returns 0. Please clean this up in a > >> separate followup patch. > > > > It may be the correct way to do it but this sounds like we are missing > > something somewhere... pci_dma_read() calls pci_dma_rw() which always > > returns 0. Why not let it returns void? It is inlined and never used > > by address. Else we should document why returning 0 is correct, and > > what is the reason to not use a void prototype. > > > > pci_dma_rw() calls dma_memory_rw() which does return a boolean value, > > false on success (MEMTX_OK) and true on error > > (MEMTX_ERROR/DECODE_ERROR) > > PCI question. Michael, Marcel? > Hi Markus, Looking at the git history, pci_dma_rw used to call cpu_physical_memory_rw which, at that time (commit ec17457), returned void. Since the interface dictated to return int, 0 is returned as "always OK". The callers to pci_dma_read did not bother to check it for obvious reasons (even if they should). In the meantime the memory API has changed to allow returning errors, but since the callers of pci_dma_rw don't check the return value, why bother to update the PCI DMA? History aside (and my speculations above), it seems the right move is to update the return value and check it by callers, but honestly I don't have any idea if the emulated devices expect pci dma to fail. Adding Paolo and David for more insights. Thanks, Marcel
Re: [Qemu-devel] [PATCH v5 1/4] net/rocker: Remove the dead error handling
Philippe Mathieu-Daudéwrites: > Hi Markus, > > On 05/23/2017 06:27 AM, Markus Armbruster wrote: > [...] >> There's one more cleanup opportunity: >> > [...] >>> if (pci_dma_read(dev, le64_to_cpu(info->desc.buf_addr), info->buf, >>> size)) { >>> return NULL; >>> } >> >> None of the pci_dma_read() calls outside rocker check the return value. >> Just as well, because it always returns 0. Please clean this up in a >> separate followup patch. > > It may be the correct way to do it but this sounds like we are missing > something somewhere... pci_dma_read() calls pci_dma_rw() which always > returns 0. Why not let it returns void? It is inlined and never used > by address. Else we should document why returning 0 is correct, and > what is the reason to not use a void prototype. > > pci_dma_rw() calls dma_memory_rw() which does return a boolean value, > false on success (MEMTX_OK) and true on error > (MEMTX_ERROR/DECODE_ERROR) PCI question. Michael, Marcel?
Re: [Qemu-devel] [PATCH v5 1/4] net/rocker: Remove the dead error handling
Hi Markus, On 05/23/2017 06:27 AM, Markus Armbruster wrote: [...] There's one more cleanup opportunity: [...] if (pci_dma_read(dev, le64_to_cpu(info->desc.buf_addr), info->buf, size)) { return NULL; } None of the pci_dma_read() calls outside rocker check the return value. Just as well, because it always returns 0. Please clean this up in a separate followup patch. It may be the correct way to do it but this sounds like we are missing something somewhere... pci_dma_read() calls pci_dma_rw() which always returns 0. Why not let it returns void? It is inlined and never used by address. Else we should document why returning 0 is correct, and what is the reason to not use a void prototype. pci_dma_rw() calls dma_memory_rw() which does return a boolean value, false on success (MEMTX_OK) and true on error (MEMTX_ERROR/DECODE_ERROR) Regards, Phil.
Re: [Qemu-devel] [PATCH v5 1/4] net/rocker: Remove the dead error handling
Mao Zhongyiwrites: > Hi, Markus > > > On 05/23/2017 05:27 PM, Markus Armbruster wrote: >> Mao Zhongyi writes: >> >>> Memory allocation functions like world_alloc, desc_ring_alloc etc, >>> they are all wrappers around g_malloc, g_new etc. But g_malloc and >>> similar functions doesn't return null. Because they ignore the fact >> >> don't > > Will I need to make a separated patch to fix it? or when you merge to > help me repair? Perhaps Jason can touch it up on commit. > Thanks a lot. > >> >>> that g_malloc() of 0 bytes returns null. So error checks for these >>> allocation failure are superfluous. Now, remove them entirely. >>> >>> Signed-off-by: Mao Zhongyi >> >> Reviewed-by: Markus Armbruster > > Thanks for your quick review:) You're welcome. >> There's one more cleanup opportunity: >> >>> diff --git a/hw/net/rocker/rocker_desc.c b/hw/net/rocker/rocker_desc.c >>> index ac02797..d0df89a 100644 >>> --- a/hw/net/rocker/rocker_desc.c >>> +++ b/hw/net/rocker/rocker_desc.c >>> @@ -65,10 +65,6 @@ char *desc_get_buf(DescInfo *info, bool read_only) >>> info->buf_size = size; >>> } >>> >>> -if (!info->buf) { >>> -return NULL; >>> -} >>> - >>> if (pci_dma_read(dev, le64_to_cpu(info->desc.buf_addr), info->buf, >>> size)) { >>> return NULL; >>> } >> >> None of the pci_dma_read() calls outside rocker check the return value. >> Just as well, because it always returns 0. Please clean this up in a >> separate followup patch. > > Thanks for the reminder. I just read the code, it's true. > Will fix it right away. Thanks!
Re: [Qemu-devel] [PATCH v5 1/4] net/rocker: Remove the dead error handling
Hi, Markus On 05/23/2017 05:27 PM, Markus Armbruster wrote: Mao Zhongyiwrites: Memory allocation functions like world_alloc, desc_ring_alloc etc, they are all wrappers around g_malloc, g_new etc. But g_malloc and similar functions doesn't return null. Because they ignore the fact don't Will I need to make a separated patch to fix it? or when you merge to help me repair? Thanks a lot. that g_malloc() of 0 bytes returns null. So error checks for these allocation failure are superfluous. Now, remove them entirely. Signed-off-by: Mao Zhongyi Reviewed-by: Markus Armbruster Thanks for your quick review:) There's one more cleanup opportunity: diff --git a/hw/net/rocker/rocker_desc.c b/hw/net/rocker/rocker_desc.c index ac02797..d0df89a 100644 --- a/hw/net/rocker/rocker_desc.c +++ b/hw/net/rocker/rocker_desc.c @@ -65,10 +65,6 @@ char *desc_get_buf(DescInfo *info, bool read_only) info->buf_size = size; } -if (!info->buf) { -return NULL; -} - if (pci_dma_read(dev, le64_to_cpu(info->desc.buf_addr), info->buf, size)) { return NULL; } None of the pci_dma_read() calls outside rocker check the return value. Just as well, because it always returns 0. Please clean this up in a separate followup patch. Thanks for the reminder. I just read the code, it's true. Will fix it right away. Mao
Re: [Qemu-devel] [PATCH v5 1/4] net/rocker: Remove the dead error handling
Mao Zhongyiwrites: > Memory allocation functions like world_alloc, desc_ring_alloc etc, > they are all wrappers around g_malloc, g_new etc. But g_malloc and > similar functions doesn't return null. Because they ignore the fact don't > that g_malloc() of 0 bytes returns null. So error checks for these > allocation failure are superfluous. Now, remove them entirely. > > Signed-off-by: Mao Zhongyi Reviewed-by: Markus Armbruster There's one more cleanup opportunity: > diff --git a/hw/net/rocker/rocker_desc.c b/hw/net/rocker/rocker_desc.c > index ac02797..d0df89a 100644 > --- a/hw/net/rocker/rocker_desc.c > +++ b/hw/net/rocker/rocker_desc.c > @@ -65,10 +65,6 @@ char *desc_get_buf(DescInfo *info, bool read_only) > info->buf_size = size; > } > > -if (!info->buf) { > -return NULL; > -} > - > if (pci_dma_read(dev, le64_to_cpu(info->desc.buf_addr), info->buf, > size)) { > return NULL; > } None of the pci_dma_read() calls outside rocker check the return value. Just as well, because it always returns 0. Please clean this up in a separate followup patch. [...]
[Qemu-devel] [PATCH v5 1/4] net/rocker: Remove the dead error handling
Memory allocation functions like world_alloc, desc_ring_alloc etc, they are all wrappers around g_malloc, g_new etc. But g_malloc and similar functions doesn't return null. Because they ignore the fact that g_malloc() of 0 bytes returns null. So error checks for these allocation failure are superfluous. Now, remove them entirely. Signed-off-by: Mao Zhongyi--- hw/net/rocker/rocker.c| 46 --- hw/net/rocker/rocker_desc.c | 10 -- hw/net/rocker/rocker_fp.c | 4 hw/net/rocker/rocker_of_dpa.c | 20 --- hw/net/rocker/rocker_world.c | 12 +-- 5 files changed, 5 insertions(+), 87 deletions(-) diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c index 6e70fdd..d01ba9d 100644 --- a/hw/net/rocker/rocker.c +++ b/hw/net/rocker/rocker.c @@ -239,10 +239,6 @@ static int tx_consume(Rocker *r, DescInfo *info) } iov[iovcnt].iov_len = frag_len; iov[iovcnt].iov_base = g_malloc(frag_len); -if (!iov[iovcnt].iov_base) { -err = -ROCKER_ENOMEM; -goto err_no_mem; -} if (pci_dma_read(dev, frag_addr, iov[iovcnt].iov_base, iov[iovcnt].iov_len)) { @@ -262,7 +258,6 @@ static int tx_consume(Rocker *r, DescInfo *info) err_too_many_frags: err_bad_io: -err_no_mem: err_bad_attr: for (i = 0; i < ROCKER_TX_FRAGS_MAX; i++) { g_free(iov[i].iov_base); @@ -674,10 +669,6 @@ int rx_produce(World *world, uint32_t pport, */ data = g_malloc(data_size); -if (!data) { -err = -ROCKER_ENOMEM; -goto out; -} iov_to_buf(iov, iovcnt, 0, data, data_size); pci_dma_write(dev, frag_addr, data, data_size); g_free(data); @@ -722,11 +713,6 @@ static void rocker_test_dma_ctrl(Rocker *r, uint32_t val) buf = g_malloc(r->test_dma_size); -if (!buf) { -DPRINTF("test dma buffer alloc failed"); -return; -} - switch (val) { case ROCKER_TEST_DMA_CTRL_CLEAR: memset(buf, 0, r->test_dma_size); @@ -1313,13 +1299,6 @@ static int pci_rocker_init(PCIDevice *dev) r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r); -for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) { -if (!r->worlds[i]) { -err = -ENOMEM; -goto err_world_alloc; -} -} - if (!r->world_name) { r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA])); } @@ -1396,9 +1375,6 @@ static int pci_rocker_init(PCIDevice *dev) } r->rings = g_new(DescRing *, rocker_pci_ring_count(r)); -if (!r->rings) { -goto err_rings_alloc; -} /* Rings are ordered like this: * - command ring @@ -1410,14 +1386,9 @@ static int pci_rocker_init(PCIDevice *dev) * . */ -err = -ENOMEM; for (i = 0; i < rocker_pci_ring_count(r); i++) { DescRing *ring = desc_ring_alloc(r, i); -if (!ring) { -goto err_ring_alloc; -} - if (i == ROCKER_RING_CMD) { desc_ring_set_consume(ring, cmd_consume, ROCKER_MSIX_VEC_CMD); } else if (i == ROCKER_RING_EVENT) { @@ -1437,10 +1408,6 @@ static int pci_rocker_init(PCIDevice *dev) fp_port_alloc(r, r->name, >fp_start_macaddr, i, >fp_ports_peers[i]); -if (!port) { -goto err_port_alloc; -} - r->fp_port[i] = port; fp_port_set_world(port, r->world_dflt); } @@ -1449,25 +1416,12 @@ static int pci_rocker_init(PCIDevice *dev) return 0; -err_port_alloc: -for (--i; i >= 0; i--) { -FpPort *port = r->fp_port[i]; -fp_port_free(port); -} -i = rocker_pci_ring_count(r); -err_ring_alloc: -for (--i; i >= 0; i--) { -desc_ring_free(r->rings[i]); -} -g_free(r->rings); -err_rings_alloc: err_duplicate: rocker_msix_uninit(r); err_msix_init: object_unparent(OBJECT(>msix_bar)); object_unparent(OBJECT(>mmio)); err_world_type_by_name: -err_world_alloc: for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) { if (r->worlds[i]) { world_free(r->worlds[i]); diff --git a/hw/net/rocker/rocker_desc.c b/hw/net/rocker/rocker_desc.c index ac02797..d0df89a 100644 --- a/hw/net/rocker/rocker_desc.c +++ b/hw/net/rocker/rocker_desc.c @@ -65,10 +65,6 @@ char *desc_get_buf(DescInfo *info, bool read_only) info->buf_size = size; } -if (!info->buf) { -return NULL; -} - if (pci_dma_read(dev, le64_to_cpu(info->desc.buf_addr), info->buf, size)) { return NULL; } @@ -144,9 +140,6 @@ bool desc_ring_set_size(DescRing *ring, uint32_t size) ring->head = ring->tail = 0; ring->info = g_renew(DescInfo, ring->info, size); -if (!ring->info) { -return false; -} memset(ring->info, 0, size * sizeof(DescInfo)); @@ -347,9 +340,6 @@ DescRing