Re: [PATCH] s5p-fimc: Fix platform entities registration

[Sylwester Nawrocki <snjw23@gmail.com>]

On 10/25/2012 02:42 PM, Sylwester Nawrocki wrote:
> Hi Laurent,
> 
> On 10/25/2012 01:35 PM, Laurent Pinchart wrote:
>> On Thursday 25 October 2012 11:06:56 Sylwester Nawrocki wrote:
>>> Make sure there is no v4l2_device_unregister_subdev() call
>>> on a subdev which wasn't registered.
>>
>> I'm not implying that this fix is bad, but doesn't the V4L2 core already
>> handle this ? v4l2_device_unregister_subdev() returns immediately without
>> doing anything if the subdev hasn't been registered.
> 
> Indeed, the patch summary might be a bit misleading and incomplete.
> I of course wanted to make sure the platform subdevs are not treated
> as registered when any part of v4l2_device_register_subdev() fails.
> 
> 
> Looking at function v4l2_device_register_subdev(), I'm wondering whether
> line
>   159         sd->v4l2_dev = v4l2_dev;
> 
> shouldn't be moved right before
> 
>   190         spin_lock(&v4l2_dev->lock);
> 
> so sd->v4l2_dev is set only if we return 0 in this function ?

Hmm, no, that would be wrong. Since sd->v4l2_dev needs to be initialized
for sd->internal_ops->registered() and sd->internal_ops->registered() ops.

Still, it is possible that a subdev has the v4l2_dev field initialized and 
is not added to the v4l2_device list of subdevs (v4l2_dev->subdevs). Then 
function v4l2_device_unregister_subdev() checks for valid sd->v4l2_dev and
attempts to remove (not yet added) subdev from v4l2_dev->subdevs.

This subdev (un)registration code seems buggy, unless I'm missing something...

> Since in function v4l2_device_unregister_subdev() there is a check like
> 
>   259         /* return if it isn't registered */
>   260         if (sd == NULL || sd->v4l2_dev == NULL)
>   261                 return;
> 
> i.e. if subdev is not really registered, e.g. internal .registered
> op fails, it should be NULL.
> 
> In my case sd wasn't null since this structure was embedded in
> other one.

--
Regards,
Sylwester