Linux Virtualization - Dec 2017 - [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add

On Wed, 20 Dec 2017 12:26:25 +0800
weiping zhang <zwp10758 at gmail.com> wrote:

[you used a different mail address in your From: than in your s-o-b:;
same for the other patches]
> In order to make caller do a simple cleanup, we split device_register
> into device_initialize and device_add. device_initialize always sucess,
s/success/succeeds/
> the caller can always use put_device when fail to register virtio_device
"so the caller can always use put_device when register_virtio_device
failed,"
> no matter fail at ida_simple_get or at device_add.
"no matter whether it failed..."
> 
> Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>
> Suggested-by: Cornelia Huck <cohuck at redhat.com>
> ---
>  drivers/virtio/virtio.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index bf7ff39..3c9f211 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -303,11 +303,21 @@ void unregister_virtio_driver(struct virtio_driver
*driver)
>  }
>  EXPORT_SYMBOL_GPL(unregister_virtio_driver);
>  
> +/**
> + * register_virtio_device - register virtio device
> + * @dev        : virtio device interested
"virtio device to be registered"
> + *
> + * If an error occurs, the caller must use put_device, instead of kfree,
because
> + * device_initialize and device_add will increase @dev->dev's
reference count.
That's not correct: It's not because of device_add increasing the
reference count (it releases it again on failure), but because another
code path may have obtained a reference.

What about:

"On error, the caller must call put_device on &@dev->dev (and not
kfree), as another code path may have obtained a reference to @dev."
> + *
> + * Returns: 0 on suceess, -error on failure
> + */
>  int register_virtio_device(struct virtio_device *dev)
>  {
>  	int err;
>  
>  	dev->dev.bus = &virtio_bus;
> +	device_initialize(&dev->dev);
>  
>  	/* Assign a unique device index and hence name. */
>  	err = ida_simple_get(&virtio_index_ida, 0, 0, GFP_KERNEL);
> @@ -330,9 +340,11 @@ int register_virtio_device(struct virtio_device *dev)
>  
>  	INIT_LIST_HEAD(&dev->vqs);
>  
> -	/* device_register() causes the bus infrastructure to look for a
> -	 * matching driver. */
> -	err = device_register(&dev->dev);
> +	/*
> +	 * device_add() causes the bus infrastructure to look for a matching
> +	 * driver.
FWIW, I would just have done s/device_register/device_add/ in the
comment, but this is ok as well.
> +	 */
> +	err = device_add(&dev->dev);
>  	if (err)
>  		ida_simple_remove(&virtio_index_ida, dev->index);
>  out:
Your code change is fine.

2017-12-20 23:53 GMT+08:00 Cornelia Huck <cohuck at
redhat.com>:
> On Wed, 20 Dec 2017 12:26:25 +0800
> weiping zhang <zwp10758 at gmail.com> wrote:
>
> [you used a different mail address in your From: than in your s-o-b:;
> same for the other patches]
>
>> In order to make caller do a simple cleanup, we split device_register
>> into device_initialize and device_add. device_initialize always sucess,
>
> s/success/succeeds/
>
>> the caller can always use put_device when fail to register
virtio_device
>
> "so the caller can always use put_device when register_virtio_device
> failed,"
>
>> no matter fail at ida_simple_get or at device_add.
>
> "no matter whether it failed..."
>
>> + *
>> + * If an error occurs, the caller must use put_device, instead of
kfree, because
>> + * device_initialize and device_add will increase @dev->dev's
reference count.
>
> That's not correct: It's not because of device_add increasing the
> reference count (it releases it again on failure), but because another
> code path may have obtained a reference.
>
> What about:
>
> "On error, the caller must call put_device on &@dev->dev (and
not
> kfree), as another code path may have obtained a reference to @dev."
>
It's good to understand, further more dev->name may has a bit mem leak.
anyway, I'll correct all comments at V5. Thanks very
much.
>> + *
>> + * Returns: 0 on suceess, -error on failure
>> + */
>>  int register_virtio_device(struct virtio_device *dev)

2017-12-21 10:37 GMT+08:00 weiping zhang <zwp10758 at
gmail.com>:
> 2017-12-20 23:53 GMT+08:00 Cornelia Huck <cohuck at redhat.com>:
>> On Wed, 20 Dec 2017 12:26:25 +0800
>> weiping zhang <zwp10758 at gmail.com> wrote:
>>
>> [you used a different mail address in your From: than in your s-o-b:;
>> same for the other patches]
a wrong setting of my email client, correct it next time,
thanks.
>>> In order to make caller do a simple cleanup, we split
device_register
>>> into device_initialize and device_add. device_initialize always
sucess,
>>
>> s/success/succeeds/
>>
>>> the caller can always use put_device when fail to register
virtio_device
>>
>> "so the caller can always use put_device when
register_virtio_device
>> failed,"
>>
>>> no matter fail at ida_simple_get or at device_add.
>>
>> "no matter whether it failed..."
>>
>>> + *
>>> + * If an error occurs, the caller must use put_device, instead of
kfree, because
>>> + * device_initialize and device_add will increase
@dev->dev's reference count.
>>
>> That's not correct: It's not because of device_add increasing
the
>> reference count (it releases it again on failure), but because another
>> code path may have obtained a reference.
>>
>> What about:
>>
>> "On error, the caller must call put_device on &@dev->dev
(and not
>> kfree), as another code path may have obtained a reference to
@dev."
>>
> It's good to understand, further more dev->name may has a bit mem
leak.
> anyway, I'll correct all comments at V5. Thanks very much.
>>> + *
>>> + * Returns: 0 on suceess, -error on failure
>>> + */
>>>  int register_virtio_device(struct virtio_device *dev)