Messages in this thread | | | Subject | Re: [PATCH] net: hns: Fix object reference leaks in hns_dsaf_roce_reset() | From | John Garry <> | Date | Fri, 15 Feb 2019 12:00:11 +0000 |
| |
On 15/02/2019 11:25, Salil Mehta wrote: >> From: John Garry >> Sent: Friday, February 15, 2019 10:52 AM >> >> On 14/02/2019 06:41, Huang Zijiang wrote: >>> The of_find_device_by_node() takes a reference to the underlying device >>> structure, we should release that reference. >> >> of_find_device_by_node() is not called for every path, so is this change proper: > > > It looks okay to me and with the suggested device reference is being > released from every possible error leg in the function. Could you be > more specific which error path is not being addressed? > > > >> /* find the platform device corresponding to fwnode */ >> if (is_of_node(dsaf_fwnode)) { >> pdev = of_find_device_by_node(to_of_node(dsaf_fwnode)); > > > This will get the reference to the device, which needs to be > released later. > > >> } else if (is_acpi_device_node(dsaf_fwnode)) { >> pdev = hns_dsaf_find_platform_device(dsaf_fwnode); > > > This will also get the reference to the device when bus_find_device() > gets called and returns the 'device'. Therefore, this needs to be > released as well using put_device()
OK, I see. That's non-obvious.
And so the commit message was simply incomplete.
>
So who finally drops this reference? This patch only seems to release on error path. I checked the callers and they don't seem to.
John
> > >> } else { >> pr_err("fwnode is neither OF or ACPI type\n"); >> return -EINVAL; >> } >> >> /* check if we were a success in fetching pdev */ >> if (!pdev) { >> pr_err("couldn't find platform device for node\n"); >> return -ENODEV; >> } >> >> /* retrieve the dsaf_device from the driver data */ >> dsaf_dev = dev_get_drvdata(&pdev->dev); >> if (!dsaf_dev) { >> dev_err(&pdev->dev, "dsaf_dev is NULL\n"); >> return -ENODEV; >> } >> >> John > > > [...] > >>> --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c >>> +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c >>> @@ -3081,6 +3081,7 @@ int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool dereset) >>> dsaf_dev = dev_get_drvdata(&pdev->dev); >>> if (!dsaf_dev) { >>> dev_err(&pdev->dev, "dsaf_dev is NULL\n"); >>> + put_device(&pdev->dev); > > > This looks okay. > > >>> return -ENODEV; >>> } >>> >>> @@ -3088,6 +3089,7 @@ int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool dereset) >>> if (AE_IS_VER1(dsaf_dev->dsaf_ver)) { >>> dev_err(dsaf_dev->dev, "%s v1 chip doesn't support RoCE!\n", >>> dsaf_dev->ae_dev.name); >>> + put_device(&pdev->dev); > > > This looks okay. > > > Salil. > > . >
| |