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
