| Message ID | 1357074661-15784-1-git-send-email-khoroshilov@ispras.ru (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Tuesday 01 January 2013 22:11:01 Alexey Khoroshilov wrote: > If pci_set_dma_mask() or pci_set_consistent_dma_mask() fails in p54p_probe(), > it breaks off initialization, deallocates all resources, but returns zero. > > The patch implements proper error code propagation. Uh, Thanks! But wait, I think there's another return 0 in the error path. See p54pci.c @ line 558: mem_len = pci_resource_len(pdev, 0); if (mem_len < sizeof(...)) { dev_err(...) goto err_disabled_dev; } Do you think you can add a err = -EINVAL; before the goto too? [I wonder why this wasn't found by the verification project as well? Could it be that pci_resource_len(...) < sizeof(...) is somehow always true and this is a dead branch?] > Found by Linux Driver Verification project (linuxtesting.org). > > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> Acked-by: Christian Lamparter <chunkeey@googlemail.com> Regards, Christian > --- > drivers/net/wireless/p54/p54pci.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/p54/p54pci.c b/drivers/net/wireless/p54/p54pci.c > index 933e5d9..fef69ea 100644 > --- a/drivers/net/wireless/p54/p54pci.c > +++ b/drivers/net/wireless/p54/p54pci.c > @@ -568,8 +568,10 @@ static int p54p_probe(struct pci_dev *pdev, > goto err_disable_dev; > } > > - if (pci_set_dma_mask(pdev, DMA_BIT_MASK(32)) || > - pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32))) { > + err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32)); > + if (!err) > + err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)); > + if (err) { > dev_err(&pdev->dev, "No suitable DMA available\n"); > goto err_free_reg; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/02/2013 01:45 AM, Christian Lamparter wrote: > On Tuesday 01 January 2013 22:11:01 Alexey Khoroshilov wrote: >> If pci_set_dma_mask() or pci_set_consistent_dma_mask() fails in p54p_probe(), >> it breaks off initialization, deallocates all resources, but returns zero. >> >> The patch implements proper error code propagation. > Uh, Thanks! > > But wait, I think there's another return 0 in the error > path. See p54pci.c @ line 558: > > mem_len = pci_resource_len(pdev, 0); > if (mem_len < sizeof(...)) { > dev_err(...) > goto err_disabled_dev; > } > > Do you think you can add a err = -EINVAL; before the goto too? You are right! But I would say -ENODEV is more popular error code in this case. > [I wonder why this wasn't found by the verification project as > well? Could it be that pci_resource_len(...) < sizeof(...) is > somehow always true and this is a dead branch?] Actually it was found, but I have no direct access to the results at the moment. My fault. Would you like I resend the patch to fix both? -- Best regards, Alexey -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 01 January 2013 23:44:38 Alexey Khoroshilov wrote: > On 01/02/2013 01:45 AM, Christian Lamparter wrote: > > On Tuesday 01 January 2013 22:11:01 Alexey Khoroshilov wrote: > >> If pci_set_dma_mask() or pci_set_consistent_dma_mask() fails in p54p_probe(), > >> it breaks off initialization, deallocates all resources, but returns zero. > >> > >> The patch implements proper error code propagation. > > Uh, Thanks! > > > > But wait, I think there's another return 0 in the error > > path. See p54pci.c @ line 558: > > > > mem_len = pci_resource_len(pdev, 0); > > if (mem_len < sizeof(...)) { > > dev_err(...) > > goto err_disabled_dev; > > } > > > > Do you think you can add a err = -EINVAL; before the goto too? > You are right! But I would say -ENODEV is more popular error code in > this case. pci_* functions seem to use a lot of -EIO too. Either way shouldn't really matter. So, let's make it -ENODEV. > > [I wonder why this wasn't found by the verification project as > > well? Could it be that pci_resource_len(...) < sizeof(...) is > > somehow always true and this is a dead branch?] > Actually it was found, but I have no direct access to the results at the > moment. My fault. > > Would you like I resend the patch to fix both? :) Don't forget the ACKed-by tag from the previous reply! Thanks Christian -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/p54/p54pci.c b/drivers/net/wireless/p54/p54pci.c index 933e5d9..fef69ea 100644 --- a/drivers/net/wireless/p54/p54pci.c +++ b/drivers/net/wireless/p54/p54pci.c @@ -568,8 +568,10 @@ static int p54p_probe(struct pci_dev *pdev, goto err_disable_dev; } - if (pci_set_dma_mask(pdev, DMA_BIT_MASK(32)) || - pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32))) { + err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32)); + if (!err) + err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)); + if (err) { dev_err(&pdev->dev, "No suitable DMA available\n"); goto err_free_reg; }
If pci_set_dma_mask() or pci_set_consistent_dma_mask() fails in p54p_probe(), it breaks off initialization, deallocates all resources, but returns zero. The patch implements proper error code propagation. Found by Linux Driver Verification project (linuxtesting.org). Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> --- drivers/net/wireless/p54/p54pci.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)