[PATCH] v4l2: v4l2-fh: v4l2_fh_is_singular should use list head to test

[PATCH] v4l2: v4l2-fh: v4l2_fh_is_singular should use list head to test

[Scott Jiang]

list_is_singular accepts a list head to test whether a list has just one entry.
fh->list is the entry, fh->vdev->fh_list is the list head.

Signed-off-by: Scott Jiang <scott.jiang.linux@gmail.com>
---
 drivers/media/video/v4l2-fh.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
index 9e3fc04..8292c4a 100644
--- a/drivers/media/video/v4l2-fh.c
+++ b/drivers/media/video/v4l2-fh.c
@@ -113,7 +113,7 @@ int v4l2_fh_is_singular(struct v4l2_fh *fh)
 	if (fh == NULL || fh->vdev == NULL)
 		return 0;
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
-	is_singular = list_is_singular(&fh->list);
+	is_singular = list_is_singular(&fh->vdev->fh_list);
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
 	return is_singular;
 }
-- 
1.7.0.4

Re: [PATCH] v4l2: v4l2-fh: v4l2_fh_is_singular should use list head to test

[Sakari Ailus]

Hi Scott,

Thanks for the patch.

On Wed, Dec 21, 2011 at 10:30:54AM -0500, Scott Jiang wrote:
> list_is_singular accepts a list head to test whether a list has just one entry.
> fh->list is the entry, fh->vdev->fh_list is the list head.
> 
> Signed-off-by: Scott Jiang <scott.jiang.linux@gmail.com>
> ---
>  drivers/media/video/v4l2-fh.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
> index 9e3fc04..8292c4a 100644
> --- a/drivers/media/video/v4l2-fh.c
> +++ b/drivers/media/video/v4l2-fh.c
> @@ -113,7 +113,7 @@ int v4l2_fh_is_singular(struct v4l2_fh *fh)
>  	if (fh == NULL || fh->vdev == NULL)
>  		return 0;
>  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> -	is_singular = list_is_singular(&fh->list);
> +	is_singular = list_is_singular(&fh->vdev->fh_list);
>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>  	return is_singular;
>  }

Is there an issue that this patch resolves, or am I missing something? As
far as I can see, the list_is_singular() test returns the same result
whether you are testing a list item which is part of the list, or its head
in struct video_device.

Kind regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [PATCH] v4l2: v4l2-fh: v4l2_fh_is_singular should use list head to test

[Scott Jiang]

2012/1/4 Sakari Ailus <sakari.ailus@iki.fi>:
> Hi Scott,
>
> Thanks for the patch.
>
> On Wed, Dec 21, 2011 at 10:30:54AM -0500, Scott Jiang wrote:
>> list_is_singular accepts a list head to test whether a list has just one entry.
>> fh->list is the entry, fh->vdev->fh_list is the list head.
>>
>> Signed-off-by: Scott Jiang <scott.jiang.linux@gmail.com>
>> ---
>>  drivers/media/video/v4l2-fh.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
>> index 9e3fc04..8292c4a 100644
>> --- a/drivers/media/video/v4l2-fh.c
>> +++ b/drivers/media/video/v4l2-fh.c
>> @@ -113,7 +113,7 @@ int v4l2_fh_is_singular(struct v4l2_fh *fh)
>>       if (fh == NULL || fh->vdev == NULL)
>>               return 0;
>>       spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>> -     is_singular = list_is_singular(&fh->list);
>> +     is_singular = list_is_singular(&fh->vdev->fh_list);
>>       spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>>       return is_singular;
>>  }
>
> Is there an issue that this patch resolves, or am I missing something? As
> far as I can see, the list_is_singular() test returns the same result
> whether you are testing a list item which is part of the list, or its head
> in struct video_device.
>
Yes, the result is the same. But I don't think it's a good example
because it may abuse this api.
Can anybody figure out what this api needs you to pass in?  I confess
I am not sure about that.

Scott

Re: [PATCH] v4l2: v4l2-fh: v4l2_fh_is_singular should use list head to test

[Sakari Ailus]

On Thu, Jan 05, 2012 at 10:52:02AM +0800, Scott Jiang wrote:
> 2012/1/4 Sakari Ailus <sakari.ailus@iki.fi>:
> > Hi Scott,
> >
> > Thanks for the patch.
> >
> > On Wed, Dec 21, 2011 at 10:30:54AM -0500, Scott Jiang wrote:
> >> list_is_singular accepts a list head to test whether a list has just one entry.
> >> fh->list is the entry, fh->vdev->fh_list is the list head.
> >>
> >> Signed-off-by: Scott Jiang <scott.jiang.linux@gmail.com>
> >> ---
> >>  drivers/media/video/v4l2-fh.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
> >> index 9e3fc04..8292c4a 100644
> >> --- a/drivers/media/video/v4l2-fh.c
> >> +++ b/drivers/media/video/v4l2-fh.c
> >> @@ -113,7 +113,7 @@ int v4l2_fh_is_singular(struct v4l2_fh *fh)
> >>       if (fh == NULL || fh->vdev == NULL)
> >>               return 0;
> >>       spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> >> -     is_singular = list_is_singular(&fh->list);
> >> +     is_singular = list_is_singular(&fh->vdev->fh_list);
> >>       spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> >>       return is_singular;
> >>  }
> >
> > Is there an issue that this patch resolves, or am I missing something? As
> > far as I can see, the list_is_singular() test returns the same result
> > whether you are testing a list item which is part of the list, or its head
> > in struct video_device.
> >
> Yes, the result is the same. But I don't think it's a good example
> because it may abuse this api.
> Can anybody figure out what this api needs you to pass in?  I confess
> I am not sure about that.

That's true; it's more correct (and intuitive as well) to use the real list
head for the purpose. But if the implementation really changed I bet a huge
number of other things would break as well.

Acked-by: Sakari Ailus <sakari.ailus@iki.fi>

Hans: you wrote the patch adding this code (dfddb244); what do you think?

Regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [PATCH] v4l2: v4l2-fh: v4l2_fh_is_singular should use list head to test

[Mauro Carvalho Chehab]

On 05-01-2012 05:57, Sakari Ailus wrote:
> On Thu, Jan 05, 2012 at 10:52:02AM +0800, Scott Jiang wrote:
>> 2012/1/4 Sakari Ailus <sakari.ailus@iki.fi>:
>>> Hi Scott,
>>>
>>> Thanks for the patch.
>>>
>>> On Wed, Dec 21, 2011 at 10:30:54AM -0500, Scott Jiang wrote:
>>>> list_is_singular accepts a list head to test whether a list has just one entry.
>>>> fh->list is the entry, fh->vdev->fh_list is the list head.
>>>>
>>>> Signed-off-by: Scott Jiang <scott.jiang.linux@gmail.com>
>>>> ---
>>>>  drivers/media/video/v4l2-fh.c |    2 +-
>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
>>>> index 9e3fc04..8292c4a 100644
>>>> --- a/drivers/media/video/v4l2-fh.c
>>>> +++ b/drivers/media/video/v4l2-fh.c
>>>> @@ -113,7 +113,7 @@ int v4l2_fh_is_singular(struct v4l2_fh *fh)
>>>>       if (fh == NULL || fh->vdev == NULL)
>>>>               return 0;
>>>>       spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>>>> -     is_singular = list_is_singular(&fh->list);
>>>> +     is_singular = list_is_singular(&fh->vdev->fh_list);
>>>>       spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>>>>       return is_singular;
>>>>  }
>>>
>>> Is there an issue that this patch resolves, or am I missing something? As
>>> far as I can see, the list_is_singular() test returns the same result
>>> whether you are testing a list item which is part of the list, or its head
>>> in struct video_device.
>>>
>> Yes, the result is the same. But I don't think it's a good example
>> because it may abuse this api.
>> Can anybody figure out what this api needs you to pass in?  I confess
>> I am not sure about that.
> 
> That's true; it's more correct (and intuitive as well) to use the real list
> head for the purpose. But if the implementation really changed I bet a huge
> number of other things would break as well.
> 
> Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
> 
> Hans: you wrote the patch adding this code (dfddb244); what do you think?

All those list functions can operate on any node of the list, since the
list is circular. So, there's not a real "head" for the list.

The function implementation shows that:

static inline void INIT_LIST_HEAD(struct list_head *list)
{
        list->next = list;
        list->prev = list;
}

...

static inline int list_is_singular(const struct list_head *head)
{
        return !list_empty(head) && (head->next == head->prev);
}

So, I prefer to not change it, _unless_ for some reason, you hit
a bug on it (for example, by not having one of the list pointers
filled).

Regards,
Mauro
> 
> Regards,
>