| Message ID | 20170829121623.24761-4-sergey.matyukevich.os@quantenna.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bab5dac73c08e0a18717756c4aff6417279bf2f9 |
| Delegated to: | Kalle Valo |
| Headers | show |
On 08/29/2017 05:16 AM, Sergey Matyukevich wrote: > NULL is not a special type of success here but a error pointer. > So it makes sense to check against NULL in qtnf_map_bar > and return error code. > > Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com> > --- On a first glance not immediately obvious what is logically changed here, is it so that pr_debug() would not print NULL pointer?
On 08/29/2017 05:16 AM, Sergey Matyukevich wrote: > NULL is not a special type of success here but a error pointer. > So it makes sense to check against NULL in qtnf_map_bar > and return error code. > > Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com> > --- On a first glance not immediately obvious what is logically changed here, is it so that pr_debug() would not print NULL pointer?
On Tue, Aug 29, 2017 at 07:13:35PM -0700, Igor Mitsyanko wrote: > On 08/29/2017 05:16 AM, Sergey Matyukevich wrote: > > NULL is not a special type of success here but a error pointer. > > So it makes sense to check against NULL in qtnf_map_bar > > and return error code. > > > > Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com> > > --- > > On a first glance not immediately obvious what is logically changed here, is > it so that pr_debug() would not print NULL pointer? There is no actual bug here: all the mappings and error checks are in place. This is more about coding style problem: when function return both NULL and error pointeres, then the NULL is supposed to be a special type of success return. In this particular case NULL is just yet another failure.
diff --git a/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c b/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c index fd552d64f943..bfbcd0bf75bf 100644 --- a/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c +++ b/drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c @@ -184,8 +184,10 @@ static void __iomem *qtnf_map_bar(struct qtnf_pcie_bus_priv *priv, u8 index) return IOMEM_ERR_PTR(ret); busaddr = pci_resource_start(priv->pdev, index); - vaddr = pcim_iomap_table(priv->pdev)[index]; len = pci_resource_len(priv->pdev, index); + vaddr = pcim_iomap_table(priv->pdev)[index]; + if (!vaddr) + return IOMEM_ERR_PTR(-ENOMEM); pr_debug("BAR%u vaddr=0x%p busaddr=%pad len=%u\n", index, vaddr, &busaddr, (int)len); @@ -248,19 +250,19 @@ static int qtnf_pcie_init_memory(struct qtnf_pcie_bus_priv *priv) int ret = -ENOMEM; priv->sysctl_bar = qtnf_map_bar(priv, QTN_SYSCTL_BAR); - if (IS_ERR_OR_NULL(priv->sysctl_bar)) { + if (IS_ERR(priv->sysctl_bar)) { pr_err("failed to map BAR%u\n", QTN_SYSCTL_BAR); return ret; } priv->dmareg_bar = qtnf_map_bar(priv, QTN_DMA_BAR); - if (IS_ERR_OR_NULL(priv->dmareg_bar)) { + if (IS_ERR(priv->dmareg_bar)) { pr_err("failed to map BAR%u\n", QTN_DMA_BAR); return ret; } priv->epmem_bar = qtnf_map_bar(priv, QTN_SHMEM_BAR); - if (IS_ERR_OR_NULL(priv->epmem_bar)) { + if (IS_ERR(priv->epmem_bar)) { pr_err("failed to map BAR%u\n", QTN_SHMEM_BAR); return ret; }
NULL is not a special type of success here but a error pointer. So it makes sense to check against NULL in qtnf_map_bar and return error code. Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com> --- drivers/net/wireless/quantenna/qtnfmac/pearl/pcie.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)