lkml.org 
[lkml]   [2022]   [Aug]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH -next 1/3] PCI: fix double put_device() in error case in pci_create_root_bus()
[+cc Rob]

On Thu, Aug 25, 2022 at 08:27:51PM +0800, Yang Yingliang wrote:
> If device_add() fails in pci_register_host_bridge(), the brigde device will
> be put once, and it will be put again in error path of pci_create_root_bus().
> Move the put_device() from pci_create_root_bus() to pci_register_host_bridge()
> to fix this problem. And use device_unregister() instead of del_device() and
> put_device().

s/brigde/bridge/

> Fixes: 9885440b16b8 ("PCI: Fix pci_host_bridge struct device release/free handling")

If you're fixing a commit from somebody else, please always cc: the
author because the author can help review the change.

> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
> drivers/pci/probe.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index c5286b027f00..e500eb9d6468 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1027,7 +1027,7 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>
> unregister:
> put_device(&bridge->dev);
> - device_del(&bridge->dev);
> + device_unregister(&bridge->dev);

I don't understand this part. device_unregister() looks like this:

void device_unregister(struct device *dev)
{
device_del(dev);
put_device(dev);
}

So this calls put_device(&bridge->dev) twice, doesn't it?

The "unregister" label looks poorly named. We only get there if
device_register() *failed*. We shouldn't need to unregister anything
in that case.

> free:
> kfree(bus);
> @@ -3037,13 +3037,9 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>
> error = pci_register_host_bridge(bridge);
> if (error < 0)
> - goto err_out;
> + return NULL;
>
> return bridge->bus;
> -
> -err_out:
> - put_device(&bridge->dev);
> - return NULL;

This part looks right to me. The get_device() is in
pci_register_host_bridge(), and if pci_register_host_bridge() returns
failure, I think it should first do the corresponding put_device().

> }
> EXPORT_SYMBOL_GPL(pci_create_root_bus);
>
> --
> 2.25.1
>

\
 
 \ /
  Last update: 2022-08-26 23:15    [W:0.054 / U:1.580 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site