From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sylwester Nawrocki Date: Wed, 31 Oct 2012 23:25:46 +0000 Subject: Re: [PATCH 2/2] media: V4L2: support asynchronous subdevice registration Message-Id: <5091B37A.30509@gmail.com> List-Id: References: <508D4F79.2000204@gmail.com> <5091AF97.7010804@gmail.com> In-Reply-To: <5091AF97.7010804@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Guennadi Liakhovetski Cc: Greg Kroah-Hartman , Hans Verkuil , Sylwester Nawrocki , Laurent Pinchart , Magnus Damm , linux-sh@vger.kernel.org, Linux Media Mailing List On 11/01/2012 12:09 AM, Sylwester Nawrocki wrote: > Hi Guennadi, > > On 10/29/2012 08:52 AM, Guennadi Liakhovetski wrote: >>>>> +/* >>>>> + * Typically this function will be called during bridge driver probing. It >>>>> + * installs bus notifiers to handle asynchronously probing subdevice drivers. >>>>> + * Once the bridge driver probing completes, subdevice drivers, waiting in >>>>> + * EPROBE_DEFER state are re-probed, at which point they get their platform >>>>> + * data, which allows them to complete probing. >>>>> + */ >>>>> +int v4l2_async_group_probe(struct v4l2_async_group *group) >>>>> +{ >>>>> + struct v4l2_async_subdev *asd, *tmp; >>>>> + bool i2c_used = false, platform_used = false; >>>>> + int ret; >>>>> + >>>>> + /* This group is inactive so far - no notifiers yet */ >>>>> + list_for_each_entry_safe(asd, tmp,&group->group, list) { >>>>> + if (asd->sdpd.subdev) { >>>>> + /* Simulate a BIND event */ >>>>> + if (group->bind_cb) >>>>> + group->bind_cb(group, asd); >>>>> + >>> >>> Still we can't be sure at this moment asd->sdpd.subdev's driver is >>> valid and not unloaded, can we ? >>> >>> In the case when a sub-device driver is probed after the host driver >>> (a caller of this function) I assume doing >>> >>> asd->sdpd.subdev = i2c_get_clientdata(to_i2c_client(dev)); >>> ... >>> ret = v4l2_device_register_subdev(v4l2_dev, asd->sdpd.subdev); >>> >>> is safe, because it is done in the i2c bus notifier callback itself, >>> i.e. under device_lock(dev). >>> >>> But for these already probed sub-devices, how do we prevent races from >>> subdev module unloading ? By not setting CONFIG_MODULE_UNLOAD?... ;) >> >> Right, I also think there's a race there. I have a solution for it - in >> the current mainline version of sh_mobile_ceu_camera.c look at the code >> around the line >> >> err = bus_register_notifier(&platform_bus_type,&wait.notifier); >> >> sh_mobile_ceu_probe(). I think, that guarantees, that we either lock the >> module _safely_ in memory per try_module_get(dev->driver->owner) or get >> notified, that the module is unavailable. It looks ugly, but I don't have >> a better solution ATM. We could do the same here too. > > IMHO even "ugly" solution is better than completely ignoring the problem. > > I have some doubts whether your method eliminates the race issue. Firstly, > shouldn't the bus_notify callback [1] be active on BUS_NOTIFY_UNBIND_DRIVER, > rather than US_NOTIFY_UNBOUND_DRIVER ? Upon US_NOTIFY_UNBOUND_DRIVER > dev->driver is already NULL and still it is being referenced in a call to > try_module_get() (line 2224, [1]). > > Secondly, what guarantees that before bus_register_notifier() call [1], > we are not already after blocking_notifier_call_chain() (line 504, [2]) > which means we miss the notification and the sub-device driver is going > away together with its module under our feet ? Hmm, please ignore that one, of course in this case dev->driver is NULL and branch after this line if (!csi2_pdev->dev.driver) { is entered. > [1] http://lxr.linux.no/#linux+v3.6/drivers/media/video/sh_mobile_ceu_camera.c#L2055 > [2] http://lxr.linux.no/#linux+v3.6/drivers/base/dd.c#L478 > > -- > Thanks, > Sylwester From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-ea0-f174.google.com ([209.85.215.174]:38752 "EHLO mail-ea0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754314Ab2JaXZw (ORCPT ); Wed, 31 Oct 2012 19:25:52 -0400 Message-ID: <5091B37A.30509@gmail.com> Date: Thu, 01 Nov 2012 00:25:46 +0100 From: Sylwester Nawrocki MIME-Version: 1.0 To: Guennadi Liakhovetski CC: Greg Kroah-Hartman , Hans Verkuil , Sylwester Nawrocki , Laurent Pinchart , Magnus Damm , linux-sh@vger.kernel.org, Linux Media Mailing List Subject: Re: [PATCH 2/2] media: V4L2: support asynchronous subdevice registration References: <508D4F79.2000204@gmail.com> <5091AF97.7010804@gmail.com> In-Reply-To: <5091AF97.7010804@gmail.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: On 11/01/2012 12:09 AM, Sylwester Nawrocki wrote: > Hi Guennadi, > > On 10/29/2012 08:52 AM, Guennadi Liakhovetski wrote: >>>>> +/* >>>>> + * Typically this function will be called during bridge driver probing. It >>>>> + * installs bus notifiers to handle asynchronously probing subdevice drivers. >>>>> + * Once the bridge driver probing completes, subdevice drivers, waiting in >>>>> + * EPROBE_DEFER state are re-probed, at which point they get their platform >>>>> + * data, which allows them to complete probing. >>>>> + */ >>>>> +int v4l2_async_group_probe(struct v4l2_async_group *group) >>>>> +{ >>>>> + struct v4l2_async_subdev *asd, *tmp; >>>>> + bool i2c_used = false, platform_used = false; >>>>> + int ret; >>>>> + >>>>> + /* This group is inactive so far - no notifiers yet */ >>>>> + list_for_each_entry_safe(asd, tmp,&group->group, list) { >>>>> + if (asd->sdpd.subdev) { >>>>> + /* Simulate a BIND event */ >>>>> + if (group->bind_cb) >>>>> + group->bind_cb(group, asd); >>>>> + >>> >>> Still we can't be sure at this moment asd->sdpd.subdev's driver is >>> valid and not unloaded, can we ? >>> >>> In the case when a sub-device driver is probed after the host driver >>> (a caller of this function) I assume doing >>> >>> asd->sdpd.subdev = i2c_get_clientdata(to_i2c_client(dev)); >>> ... >>> ret = v4l2_device_register_subdev(v4l2_dev, asd->sdpd.subdev); >>> >>> is safe, because it is done in the i2c bus notifier callback itself, >>> i.e. under device_lock(dev). >>> >>> But for these already probed sub-devices, how do we prevent races from >>> subdev module unloading ? By not setting CONFIG_MODULE_UNLOAD?... ;) >> >> Right, I also think there's a race there. I have a solution for it - in >> the current mainline version of sh_mobile_ceu_camera.c look at the code >> around the line >> >> err = bus_register_notifier(&platform_bus_type,&wait.notifier); >> >> sh_mobile_ceu_probe(). I think, that guarantees, that we either lock the >> module _safely_ in memory per try_module_get(dev->driver->owner) or get >> notified, that the module is unavailable. It looks ugly, but I don't have >> a better solution ATM. We could do the same here too. > > IMHO even "ugly" solution is better than completely ignoring the problem. > > I have some doubts whether your method eliminates the race issue. Firstly, > shouldn't the bus_notify callback [1] be active on BUS_NOTIFY_UNBIND_DRIVER, > rather than US_NOTIFY_UNBOUND_DRIVER ? Upon US_NOTIFY_UNBOUND_DRIVER > dev->driver is already NULL and still it is being referenced in a call to > try_module_get() (line 2224, [1]). > > Secondly, what guarantees that before bus_register_notifier() call [1], > we are not already after blocking_notifier_call_chain() (line 504, [2]) > which means we miss the notification and the sub-device driver is going > away together with its module under our feet ? Hmm, please ignore that one, of course in this case dev->driver is NULL and branch after this line if (!csi2_pdev->dev.driver) { is entered. > [1] http://lxr.linux.no/#linux+v3.6/drivers/media/video/sh_mobile_ceu_camera.c#L2055 > [2] http://lxr.linux.no/#linux+v3.6/drivers/base/dd.c#L478 > > -- > Thanks, > Sylwester