On 18/07/17 16:39, Niklas Söderlund wrote:
> Hi Hans,
>
> Thanks for your feedback.
>
> On 2017-07-18 16:22:14 +0200, Hans Verkuil wrote:
>> On 17/07/17 18:59, Niklas Söderlund wrote:
>>> There is no good reason to hold the list_lock when reprobing the devices
>>> and it prevents a clean implementation of subdevice notifiers. Move the
>>> actual release of the devices outside of the loop which requires the
>>> lock to be held.
>>>
>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
>>> ---
>>> drivers/media/v4l2-core/v4l2-async.c | 29 ++++++++++-------------------
>>> 1 file changed, 10 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>>> index 0acf288d7227ba97..8fc84f7962386ddd 100644
>>> --- a/drivers/media/v4l2-core/v4l2-async.c
>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>>> @@ -206,7 +206,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>>> unsigned int notif_n_subdev = notifier->num_subdevs;
>>> unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS);
>>> struct device **dev;
>>> - int i = 0;
>>> + int i, count = 0;
>>>
>>> if (!notifier->v4l2_dev)
>>> return;
>>> @@ -222,37 +222,28 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>>> list_del(¬ifier->list);
>>>
>>> list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) {
>>> - struct device *d;
>>> -
>>> - d = get_device(sd->dev);
>>> + if (dev)
>>> + dev[count] = get_device(sd->dev);
>>> + count++;
>>>
>>> if (notifier->unbind)
>>> notifier->unbind(notifier, sd, sd->asd);
>>>
>>> v4l2_async_cleanup(sd);
>>> + }
>>>
>>> - /* If we handled USB devices, we'd have to lock the parent too */
>>> - device_release_driver(d);
>>> + mutex_unlock(&list_lock);
>>>
>>> - /*
>>> - * Store device at the device cache, in order to call
>>> - * put_device() on the final step
>>> - */
>>> + for (i = 0; i < count; i++) {
>>> + /* If we handled USB devices, we'd have to lock the parent too */
>>> if (dev)
>>> - dev[i++] = d;
>>> - else
>>> - put_device(d);
>>> + device_release_driver(dev[i]);
>>
>> This changes the behavior. If the alloc failed, then at least put_device was still called.
>> Now that no longer happens.
>
> Yes, but also changes the behavior to also only call get_device() if the
> allocation was successful. So the behavior is kept the same as far as I
> understands it.
Ah, I missed that. Sorry about that.
But regardless of that the device_release_driver(d) isn't called anymore.
It's not clear at all to me whether that is a problem or not.
>
>>
>> Frankly I don't understand this code, it is in desperate need of some comments explaining
>> this whole reprobing thing.
>
> I agree that the code is in need of comments, but I feel a patch that
> separates the v4l2-async work from the re-probing work is a step in the
> right direction :-)
Would it help to simplify this function to:
dev = kvmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL);
if (!dev) {
dev_err(notifier->v4l2_dev->dev,
"Failed to allocate device cache!\n");
mutex_lock(&list_lock);
list_del(¬ifier->list);
/* this assumes device_release_driver(d) isn't necessary */
list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) {
if (notifier->unbind)
notifier->unbind(notifier, sd, sd->asd);
v4l2_async_cleanup(sd);
}
mutex_unlock(&list_lock);
return;
}
...and here the code where dev is non-NULL...
Yes, there is some code duplication, but it is a lot easier to understand.
Regards,
Hans
>
>>
>> I have this strong feeling that this function needs to be reworked.
>
> I also strongly agree with this.
>
>>
>> Regards,
>>
>> Hans
>>
>>> }
>>>
>>> - mutex_unlock(&list_lock);
>>> -
>>> /*
>>> * Call device_attach() to reprobe devices
>>> - *
>>> - * NOTE: If dev allocation fails, i is 0, and the whole loop won't be
>>> - * executed.
>>> */
>>> - while (i--) {
>>> + for (i = 0; dev && i < count; i++) {
>>> struct device *d = dev[i];
>>>
>>> if (d && device_attach(d) < 0) {
>>>
>>
>
