Re: [PATCH v4 3/3] v4l: async: add subnotifier to subdevices

[Niklas Söderlund]

Hi Hans,

On 2017-07-18 17:06:15 +0200, Hans Verkuil wrote:
> On 18/07/17 16:47, Niklas Söderlund wrote:
> >>>  void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> >>>  {
> >>> - struct v4l2_subdev *sd, *tmp;
> >>> + struct v4l2_subdev *sd, *tmp, **subdev;
> >>>   unsigned int notif_n_subdev = notifier->num_subdevs;
> >>>   unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS);
> >>>   struct device **dev;
> >>> @@ -217,6 +293,12 @@ void v4l2_async_notifier_unregister(struct 
> >>> v4l2_async_notifier *notifier)
> >>>   "Failed to allocate device cache!\n");
> >>>   }
> >>>  
> >>> + subdev = kvmalloc_array(n_subdev, sizeof(*subdev), GFP_KERNEL);
> >>> + if (!dev) {
> >>> + dev_err(notifier->v4l2_dev->dev,
> >>> + "Failed to allocate subdevice cache!\n");
> >>> + }
> >>> +
> >>
> >> How about making a little struct:
> >>
> >>struct whatever {
> >>struct device *dev;
> >>struct v4l2_subdev *sd;
> >>};
> >>
> >> and allocate an array of that. Only need to call kvmalloc_array once.
> > 
> > Neat idea, will do so for next version.
> > 
> >>
> >> Some comments after the dev_err of why you ignore the failed memory 
> >> allocation
> >> and what the consequences of that are would be helpful. It is unexpected 
> >> code,
> >> and that needs documentation.
> > 
> > I agree that it's unexpected and I don't know the reason for it, I was 
> > just mimic the existing behavior. If you are OK with it I be more then 
> > happy to add patch to this series returning -ENOMEM if the allocation 
> > failed as Geert pointed out if this allocation fails I think we are in a 
> > lot of trouble anyhow...
> > 
> > Let me know what you think, but I don't think I can add a comment 
> > explaining why the function don't simply abort on failure since I don't 
> > understand it myself.
> 
> So you don't understand the device_release_driver/device_attach reprobing bit 
> either?
> 
> I did some digging and found this thread:
> 
> http://lkml.iu.edu/hypermail/linux/kernel/1210.2/00713.html
> 
> It explains the reason for this.

Nice, thanks for digging this out.

> 
> I'm pretty sure Greg K-H never saw this code :-)
> 
> Looking in drivers/base/bus.c I see this function: device_reprobe().
> 
> I think we need to use that instead.

I have now looked at device_reprobe() and unfortunately it can't be used 
in v4l2_async_notifier_unregister(). This is because some v4l2 drivers 
call v4l2_async_notifier_unregister() in there remove functions leading 
to call chains similar to this:

SyS_delete_module()
  rcar_vin_driver_exit()
platform_driver_unregister()
  driver_unregister()
bus_remove_driver()
  driver_detach()
device_lock(dev->parent); <- Here the lock is taken
device_release_driver_internal()
  platform_drv_remove()
rcar_vin_remove()
  v4l2_async_notifier_unregister()
device_reprobe()
  device_lock(dev->parent); <- Here we dead lock

So we are stuck with calling device_release_driver() and device_attach() 
directly from v4l2-async.

> 
> Regards,
> 
>   Hans

-- 
Regards,
Niklas Söderlund

Re: [PATCH v4 3/3] v4l: async: add subnotifier to subdevices

[Hans Verkuil]

On 18/07/17 16:47, Niklas Söderlund wrote:
>>>  void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>>>  {
>>> -   struct v4l2_subdev *sd, *tmp;
>>> +   struct v4l2_subdev *sd, *tmp, **subdev;
>>> unsigned int notif_n_subdev = notifier->num_subdevs;
>>> unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS);
>>> struct device **dev;
>>> @@ -217,6 +293,12 @@ void v4l2_async_notifier_unregister(struct 
>>> v4l2_async_notifier *notifier)
>>> "Failed to allocate device cache!\n");
>>> }
>>>  
>>> +   subdev = kvmalloc_array(n_subdev, sizeof(*subdev), GFP_KERNEL);
>>> +   if (!dev) {
>>> +   dev_err(notifier->v4l2_dev->dev,
>>> +   "Failed to allocate subdevice cache!\n");
>>> +   }
>>> +
>>
>> How about making a little struct:
>>
>>  struct whatever {
>>  struct device *dev;
>>  struct v4l2_subdev *sd;
>>  };
>>
>> and allocate an array of that. Only need to call kvmalloc_array once.
> 
> Neat idea, will do so for next version.
> 
>>
>> Some comments after the dev_err of why you ignore the failed memory 
>> allocation
>> and what the consequences of that are would be helpful. It is unexpected 
>> code,
>> and that needs documentation.
> 
> I agree that it's unexpected and I don't know the reason for it, I was 
> just mimic the existing behavior. If you are OK with it I be more then 
> happy to add patch to this series returning -ENOMEM if the allocation 
> failed as Geert pointed out if this allocation fails I think we are in a 
> lot of trouble anyhow...
> 
> Let me know what you think, but I don't think I can add a comment 
> explaining why the function don't simply abort on failure since I don't 
> understand it myself.

So you don't understand the device_release_driver/device_attach reprobing bit 
either?

I did some digging and found this thread:

http://lkml.iu.edu/hypermail/linux/kernel/1210.2/00713.html

It explains the reason for this.

I'm pretty sure Greg K-H never saw this code :-)

Looking in drivers/base/bus.c I see this function: device_reprobe().

I think we need to use that instead.

Regards,

Hans

Re: [PATCH v4 3/3] v4l: async: add subnotifier to subdevices

[Niklas Söderlund]

Hi Hans,

Thanks for your feedback.

On 2017-07-18 16:22:43 +0200, Hans Verkuil wrote:
> On 17/07/17 18:59, Niklas Söderlund wrote:
> > Add a subdevice specific notifier which can be used by a subdevice
> > driver to compliment the master device notifier to extend the subdevice
> 
> compliment -> complement
> 
> Just one character difference, but a wildly different meaning :-)
> 
> Although it was very polite of the subdevice driver to compliment the
> master driver. Impeccable manners. :-)

It's all about good manners, is it not? But yes the next version of the 
series won't be this polite.

> 
> > discovery.
> > 
> > The master device registers the subdevices closest to itself in its
> > notifier while the subdevice(s) register notifiers for their closest
> > neighboring devices. Subdevice drivers configures a notifier at probe
> 
> configures -> configure
> 
> > time which are registered by the v4l2-async framework once the subdevice
> 
> are -> is
> 
> > itself is register, since it's only at this point the v4l2_dev is
> 
> register -> registered

Thanks.

> 
> > available to the subnotifier.
> > 
> > Using this incremental approach two problems can be solved:
> > 
> > 1. The master device no longer has to care how many devices exist in
> >the pipeline. It only needs to care about its closest subdevice and
> >arbitrary long pipelines can be created without having to adapt the
> >master device for each case.
> > 
> > 2. Subdevices which are represented as a single DT node but register
> >more than one subdevice can use this to improve the pipeline
> >discovery, since the subdevice driver is the only one who knows which
> >of its subdevices is linked with which subdevice of a neighboring DT
> >node.
> > 
> > To allow subdevices to provide its own list of subdevices to the
> 
> its -> their

Will fix.

> 
> > v4l2-async framework v4l2_async_subdev_register_notifier() is added.
> > This new function must be called before the subdevice itself is
> > registered with the v4l2-async framework using
> > v4l2_async_register_subdev().
> > 
> > Signed-off-by: Niklas Söderlund 
> > ---
> >  Documentation/media/kapi/v4l2-subdev.rst |  12 +++
> >  drivers/media/v4l2-core/v4l2-async.c | 134 
> > +--
> >  include/media/v4l2-async.h   |  25 ++
> >  include/media/v4l2-subdev.h  |   5 ++
> >  4 files changed, 168 insertions(+), 8 deletions(-)
> > 
> > diff --git a/Documentation/media/kapi/v4l2-subdev.rst 
> > b/Documentation/media/kapi/v4l2-subdev.rst
> > index e1f0b726e438f963..5957176965a6a3ef 100644
> > --- a/Documentation/media/kapi/v4l2-subdev.rst
> > +++ b/Documentation/media/kapi/v4l2-subdev.rst
> > @@ -262,6 +262,18 @@ is called. After all subdevices have been located the 
> > .complete() callback is
> >  called. When a subdevice is removed from the system the .unbind() method is
> >  called. All three callbacks are optional.
> >  
> > +Subdevice drivers might in turn register subnotifier objects with an
> > +array of other subdevice descriptors that the subdevice needs for its
> > +own operation. Subnotifiers are an extension of the bridge drivers
> > +notifier to allow for a incremental registering and matching of
> > +subdevices. This is useful when a driver only has information about
> > +which subdevice is closest to itself and would require knowledge from the
> > +driver of that subdevice to know which other subdevice(s) lie beyond.
> > +By registering subnotifiers drivers can incrementally move the subdevice
> > +matching down the chain of drivers. This is performed using the
> > +:c:func:`v4l2_async_subdev_register_notifier` call which must be performed
> > +before registering the subdevice using 
> > :c:func:`v4l2_async_register_subdev`.
> > +
> >  V4L2 sub-device userspace API
> >  -
> >  
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> > b/drivers/media/v4l2-core/v4l2-async.c
> > index 8fc84f7962386ddd..558fb3ec07e7fba8 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -100,6 +100,61 @@ static struct v4l2_async_subdev 
> > *v4l2_async_belongs(struct v4l2_async_notifier *
> > return NULL;
> >  }
> >  
> > +static int v4l2_async_notifier_complete(struct v4l2_async_notifier 
> > *notifier)
> > +{
> > +   struct v4l2_subdev *sd, *tmp;
> > +
> > +   if (!notifier->num_subdevs)
> > +   return 0;
> > +
> > +   list_for_each_entry_safe(sd, tmp, >done, async_list) {
> > +   v4l2_async_notifier_complete(>subnotifier);
> > +   }
> 
> Curly brackets aren't needed here (didn't checkpatch complain about this?)

It did not, and I was unsure how to handle this since 
list_for_each_entry_safe() is a macro. Will drop them for next version 
provided checkpatch don't complain about it.

> 
> > +
> > +   if (notifier->complete)
> > +   return notifier->complete(notifier);
> > +

Re: [PATCH v4 3/3] v4l: async: add subnotifier to subdevices

[Hans Verkuil]

On 17/07/17 18:59, Niklas Söderlund wrote:
> Add a subdevice specific notifier which can be used by a subdevice
> driver to compliment the master device notifier to extend the subdevice

compliment -> complement

Just one character difference, but a wildly different meaning :-)

Although it was very polite of the subdevice driver to compliment the
master driver. Impeccable manners. :-)

> discovery.
> 
> The master device registers the subdevices closest to itself in its
> notifier while the subdevice(s) register notifiers for their closest
> neighboring devices. Subdevice drivers configures a notifier at probe

configures -> configure

> time which are registered by the v4l2-async framework once the subdevice

are -> is

> itself is register, since it's only at this point the v4l2_dev is

register -> registered

> available to the subnotifier.
> 
> Using this incremental approach two problems can be solved:
> 
> 1. The master device no longer has to care how many devices exist in
>the pipeline. It only needs to care about its closest subdevice and
>arbitrary long pipelines can be created without having to adapt the
>master device for each case.
> 
> 2. Subdevices which are represented as a single DT node but register
>more than one subdevice can use this to improve the pipeline
>discovery, since the subdevice driver is the only one who knows which
>of its subdevices is linked with which subdevice of a neighboring DT
>node.
> 
> To allow subdevices to provide its own list of subdevices to the

its -> their

> v4l2-async framework v4l2_async_subdev_register_notifier() is added.
> This new function must be called before the subdevice itself is
> registered with the v4l2-async framework using
> v4l2_async_register_subdev().
> 
> Signed-off-by: Niklas Söderlund 
> ---
>  Documentation/media/kapi/v4l2-subdev.rst |  12 +++
>  drivers/media/v4l2-core/v4l2-async.c | 134 
> +--
>  include/media/v4l2-async.h   |  25 ++
>  include/media/v4l2-subdev.h  |   5 ++
>  4 files changed, 168 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/media/kapi/v4l2-subdev.rst 
> b/Documentation/media/kapi/v4l2-subdev.rst
> index e1f0b726e438f963..5957176965a6a3ef 100644
> --- a/Documentation/media/kapi/v4l2-subdev.rst
> +++ b/Documentation/media/kapi/v4l2-subdev.rst
> @@ -262,6 +262,18 @@ is called. After all subdevices have been located the 
> .complete() callback is
>  called. When a subdevice is removed from the system the .unbind() method is
>  called. All three callbacks are optional.
>  
> +Subdevice drivers might in turn register subnotifier objects with an
> +array of other subdevice descriptors that the subdevice needs for its
> +own operation. Subnotifiers are an extension of the bridge drivers
> +notifier to allow for a incremental registering and matching of
> +subdevices. This is useful when a driver only has information about
> +which subdevice is closest to itself and would require knowledge from the
> +driver of that subdevice to know which other subdevice(s) lie beyond.
> +By registering subnotifiers drivers can incrementally move the subdevice
> +matching down the chain of drivers. This is performed using the
> +:c:func:`v4l2_async_subdev_register_notifier` call which must be performed
> +before registering the subdevice using :c:func:`v4l2_async_register_subdev`.
> +
>  V4L2 sub-device userspace API
>  -
>  
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index 8fc84f7962386ddd..558fb3ec07e7fba8 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -100,6 +100,61 @@ static struct v4l2_async_subdev 
> *v4l2_async_belongs(struct v4l2_async_notifier *
>   return NULL;
>  }
>  
> +static int v4l2_async_notifier_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct v4l2_subdev *sd, *tmp;
> +
> + if (!notifier->num_subdevs)
> + return 0;
> +
> + list_for_each_entry_safe(sd, tmp, >done, async_list) {
> + v4l2_async_notifier_complete(>subnotifier);
> + }

Curly brackets aren't needed here (didn't checkpatch complain about this?)

> +
> + if (notifier->complete)
> + return notifier->complete(notifier);
> +
> + return 0;
> +}
> +
> +static bool
> +v4l2_async_is_notifier_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct v4l2_subdev *sd, *tmp;
> +
> + if (!list_empty(>waiting))
> + return false;
> +
> + list_for_each_entry_safe(sd, tmp, >done, async_list) {
> + /* Don't consider empty subnotifiers */
> + if (!sd->subnotifier.num_subdevs)
> + continue;
> +
> + if (!v4l2_async_is_notifier_complete(>subnotifier))
> + return false;
> + }
> +
> + return true;
> +}
> +
> +static int

Re: [PATCH v4 3/3] v4l: async: add subnotifier to subdevices

[Niklas Söderlund]

Hi Geert,

Thanks for your feedback.

On 2017-07-18 09:11:19 +0200, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Mon, Jul 17, 2017 at 6:59 PM, Niklas Söderlund
>  wrote:
> > Add a subdevice specific notifier which can be used by a subdevice
> > driver to compliment the master device notifier to extend the subdevice
> > discovery.
> >
> > The master device registers the subdevices closest to itself in its
> > notifier while the subdevice(s) register notifiers for their closest
> > neighboring devices. Subdevice drivers configures a notifier at probe
> > time which are registered by the v4l2-async framework once the subdevice
> > itself is register, since it's only at this point the v4l2_dev is
> > available to the subnotifier.
> >
> > Using this incremental approach two problems can be solved:
> >
> > 1. The master device no longer has to care how many devices exist in
> >the pipeline. It only needs to care about its closest subdevice and
> >arbitrary long pipelines can be created without having to adapt the
> >master device for each case.
> >
> > 2. Subdevices which are represented as a single DT node but register
> >more than one subdevice can use this to improve the pipeline
> >discovery, since the subdevice driver is the only one who knows which
> >of its subdevices is linked with which subdevice of a neighboring DT
> >node.
> >
> > To allow subdevices to provide its own list of subdevices to the
> > v4l2-async framework v4l2_async_subdev_register_notifier() is added.
> > This new function must be called before the subdevice itself is
> > registered with the v4l2-async framework using
> > v4l2_async_register_subdev().
> >
> > Signed-off-by: Niklas Söderlund 
> 
> Thanks for your patch!
> 
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> 
> > @@ -217,6 +293,12 @@ void v4l2_async_notifier_unregister(struct 
> > v4l2_async_notifier *notifier)
> > "Failed to allocate device cache!\n");
> > }
> >
> > +   subdev = kvmalloc_array(n_subdev, sizeof(*subdev), GFP_KERNEL);
> > +   if (!dev) {
> 
> if (!subdev) {

Wops, copy-past coding strikes again! Will fix.

> 
> (noticed accidentally[*] :-)
> 
> > +   dev_err(notifier->v4l2_dev->dev,
> > +   "Failed to allocate subdevice cache!\n");
> > +   }
> > +
> > mutex_lock(_lock);
> >
> > list_del(>list);
> > @@ -224,6 +306,8 @@ void v4l2_async_notifier_unregister(struct 
> > v4l2_async_notifier *notifier)
> > list_for_each_entry_safe(sd, tmp, >done, async_list) {
> > if (dev)
> > dev[count] = get_device(sd->dev);
> > +   if (subdev)
> > +   subdev[count] = sd;
> 
> I don't like these "memory allocation fails, let's continue but check the
> pointer first"-games.
> Why not abort when the dev/subdev arrays cannot be allocated? It's not
> like the system is in a good state anyway.
> kmalloc() may have printed a big fat warning and invoked the OOM-killer
> already.

I agree, and I also don't like these "games". In my first attempt to 
work with this function I did remove the memory game, but then it hit me 
that if I did I would alter behavior of the function and I did not dare 
to do so.

Hopefully this can be addressed in the future if someone ever gets 
around to reworking the whole mess of re-probing devices which IMOH 
looks a but like a hack :-) I also ofc be happy to just remove the 
memory game from this function and as you suggest simply abort if the 
allocation fails, but I feel I need the blessing from the v4l2 community 
before doing so. Sometimes v4l2 do funny stuff for legacy reasons beyond 
my understanding.

> 
> [*] while checking if you perhaps removed the "dev" games in a later patch.
>  No, you added another one :-(
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds

-- 
Regards,
Niklas Söderlund

Re: [PATCH v4 3/3] v4l: async: add subnotifier to subdevices

[Geert Uytterhoeven]

Hi Niklas,

On Mon, Jul 17, 2017 at 6:59 PM, Niklas Söderlund
 wrote:
> Add a subdevice specific notifier which can be used by a subdevice
> driver to compliment the master device notifier to extend the subdevice
> discovery.
>
> The master device registers the subdevices closest to itself in its
> notifier while the subdevice(s) register notifiers for their closest
> neighboring devices. Subdevice drivers configures a notifier at probe
> time which are registered by the v4l2-async framework once the subdevice
> itself is register, since it's only at this point the v4l2_dev is
> available to the subnotifier.
>
> Using this incremental approach two problems can be solved:
>
> 1. The master device no longer has to care how many devices exist in
>the pipeline. It only needs to care about its closest subdevice and
>arbitrary long pipelines can be created without having to adapt the
>master device for each case.
>
> 2. Subdevices which are represented as a single DT node but register
>more than one subdevice can use this to improve the pipeline
>discovery, since the subdevice driver is the only one who knows which
>of its subdevices is linked with which subdevice of a neighboring DT
>node.
>
> To allow subdevices to provide its own list of subdevices to the
> v4l2-async framework v4l2_async_subdev_register_notifier() is added.
> This new function must be called before the subdevice itself is
> registered with the v4l2-async framework using
> v4l2_async_register_subdev().
>
> Signed-off-by: Niklas Söderlund 

Thanks for your patch!

> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c

> @@ -217,6 +293,12 @@ void v4l2_async_notifier_unregister(struct 
> v4l2_async_notifier *notifier)
> "Failed to allocate device cache!\n");
> }
>
> +   subdev = kvmalloc_array(n_subdev, sizeof(*subdev), GFP_KERNEL);
> +   if (!dev) {

if (!subdev) {

(noticed accidentally[*] :-)

> +   dev_err(notifier->v4l2_dev->dev,
> +   "Failed to allocate subdevice cache!\n");
> +   }
> +
> mutex_lock(_lock);
>
> list_del(>list);
> @@ -224,6 +306,8 @@ void v4l2_async_notifier_unregister(struct 
> v4l2_async_notifier *notifier)
> list_for_each_entry_safe(sd, tmp, >done, async_list) {
> if (dev)
> dev[count] = get_device(sd->dev);
> +   if (subdev)
> +   subdev[count] = sd;

I don't like these "memory allocation fails, let's continue but check the
pointer first"-games.
Why not abort when the dev/subdev arrays cannot be allocated? It's not
like the system is in a good state anyway.
kmalloc() may have printed a big fat warning and invoked the OOM-killer
already.

[*] while checking if you perhaps removed the "dev" games in a later patch.
 No, you added another one :-(

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds