| Message ID | 20220129012702.3220704-1-weiyongjun1@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cc4598cf179ff636d7634008045905a88480bb88 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net/fsl: xgmac_mdio: fix return value check in xgmac_mdio_probe() | expand |
On Sat, Jan 29, 2022 at 01:27, Wei Yongjun <weiyongjun1@huawei.com> wrote: > In case of error, the function devm_ioremap() returns NULL pointer > not ERR_PTR(). The IS_ERR() test in the return value check should > be replaced with NULL test. > > Fixes: 1d14eb15dc2c ("net/fsl: xgmac_mdio: Use managed device resources") > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com> Sorry about that. I started out by using devm_ioremap_resource, which uses the in-band error signaling, and forgot to match the guard when I changed it. I see that this was reported by your CI, do you mind me asking what it is running in the back-end? At least my version of sparse does not seem to catch this.
> On Sat, Jan 29, 2022 at 01:27, Wei Yongjun <weiyongjun1@huawei.com> wrote: >> In case of error, the function devm_ioremap() returns NULL pointer >> not ERR_PTR(). The IS_ERR() test in the return value check should >> be replaced with NULL test. >> >> Fixes: 1d14eb15dc2c ("net/fsl: xgmac_mdio: Use managed device resources") >> Reported-by: Hulk Robot <hulkci@huawei.com> >> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com> > > Sorry about that. I started out by using devm_ioremap_resource, which > uses the in-band error signaling, and forgot to match the guard when I > changed it. > > I see that this was reported by your CI, do you mind me asking what it > is running in the back-end? At least my version of sparse does not seem > to catch this. It was reported by coccinelle with follow script: @@ expression ret, E; @@ ret = \(devm_ioport_map\| devm_ioremap\| devm_ioremap_wc\| devm_irq_alloc_generic_chip\| devm_kasprintf\| devm_kcalloc\| devm_kmalloc\| devm_kmalloc_array\| devm_kmemdup\| devm_kstrdup\| devm_kzalloc\| \)(...); ... when != ret = E ( - IS_ERR(ret) + !ret | - !IS_ERR(ret) + ret | - PTR_ERR(ret) + -ENOMEM ) It seems smatch also can report this. Regards, Wei Yongjun
Hello: This patch was applied to netdev/net-next.git (master) by David S. Miller <davem@davemloft.net>: On Sat, 29 Jan 2022 01:27:02 +0000 you wrote: > In case of error, the function devm_ioremap() returns NULL pointer > not ERR_PTR(). The IS_ERR() test in the return value check should > be replaced with NULL test. > > Fixes: 1d14eb15dc2c ("net/fsl: xgmac_mdio: Use managed device resources") > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > > [...] Here is the summary with links: - [net-next] net/fsl: xgmac_mdio: fix return value check in xgmac_mdio_probe() https://git.kernel.org/netdev/net-next/c/cc4598cf179f You are awesome, thank you!
On Sun, Jan 30, 2022 at 08:58:55AM +0800, weiyongjun (A) wrote: > > > On Sat, Jan 29, 2022 at 01:27, Wei Yongjun <weiyongjun1@huawei.com> wrote: > > > In case of error, the function devm_ioremap() returns NULL pointer > > > not ERR_PTR(). The IS_ERR() test in the return value check should > > > be replaced with NULL test. > > > > > > Fixes: 1d14eb15dc2c ("net/fsl: xgmac_mdio: Use managed device resources") > > > Reported-by: Hulk Robot <hulkci@huawei.com> > > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > > Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com> > > > > Sorry about that. I started out by using devm_ioremap_resource, which > > uses the in-band error signaling, and forgot to match the guard when I > > changed it. > > > > I see that this was reported by your CI, do you mind me asking what it > > is running in the back-end? At least my version of sparse does not seem > > to catch this. > > > It was reported by coccinelle with follow script: > > > @@ > expression ret, E; > @@ > ret = \(devm_ioport_map\| > devm_ioremap\| > devm_ioremap_wc\| > devm_irq_alloc_generic_chip\| > devm_kasprintf\| > devm_kcalloc\| > devm_kmalloc\| > devm_kmalloc_array\| > devm_kmemdup\| > devm_kstrdup\| > devm_kzalloc\| > \)(...); > ... when != ret = E > ( > - IS_ERR(ret) > + !ret > | > - !IS_ERR(ret) > + ret > | > - PTR_ERR(ret) > + -ENOMEM > ) > > > > It seems smatch also can report this. Yeah. I had this patch in my postponed messages but you beat me to sending it. The Smatch check for this requires you to have the cross function database built. I should update it to remove that requirement for at least the common functions that you have listed. regards, dan carpenter
diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c index d38d0c372585..264162049c6d 100644 --- a/drivers/net/ethernet/freescale/xgmac_mdio.c +++ b/drivers/net/ethernet/freescale/xgmac_mdio.c @@ -335,8 +335,8 @@ static int xgmac_mdio_probe(struct platform_device *pdev) priv = bus->priv; priv->mdio_base = devm_ioremap(&pdev->dev, res->start, resource_size(res)); - if (IS_ERR(priv->mdio_base)) - return PTR_ERR(priv->mdio_base); + if (!priv->mdio_base) + return -ENOMEM; /* For both ACPI and DT cases, endianness of MDIO controller * needs to be specified using "little-endian" property.
In case of error, the function devm_ioremap() returns NULL pointer not ERR_PTR(). The IS_ERR() test in the return value check should be replaced with NULL test. Fixes: 1d14eb15dc2c ("net/fsl: xgmac_mdio: Use managed device resources") Reported-by: Hulk Robot <hulkci@huawei.com> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> --- drivers/net/ethernet/freescale/xgmac_mdio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)