From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932916AbcHXNuL (ORCPT ); Wed, 24 Aug 2016 09:50:11 -0400 Received: from mout.kundenserver.de ([212.227.126.133]:53425 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756294AbcHXNuI (ORCPT ); Wed, 24 Aug 2016 09:50:08 -0400 Date: Wed, 24 Aug 2016 15:49:34 +0200 From: Alban Bedel To: Hans Verkuil Cc: Alban Bedel , Javier Martinez Canillas , linux-media@vger.kernel.org, Mauro Carvalho Chehab , Sakari Ailus , Bryan Wu , linux-kernel@vger.kernel.org Subject: Re: [PATCH] [media] v4l2-async: Always unregister the subdev on failure Message-ID: <20160824154934.07f75979@avionic-0020> In-Reply-To: <38e4b736-b053-05e0-112b-550411ecb56c@xs4all.nl> References: <1462981201-14768-1-git-send-email-alban.bedel@avionic-design.de> <429cc087-85e3-7bfa-b0b6-ab9434e5d47c@osg.samsung.com> <20160511183252.6270d740@avionic-0020> <38e4b736-b053-05e0-112b-550411ecb56c@xs4all.nl> Organization: Avionic Design X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; boundary="Sig_/AcTbSniZ/wTXtYlT40_Sr8_"; protocol="application/pgp-signature" X-Provags-ID: V03:K0:A1BZePKQRmvVMN4Eh9Li5NacGpnD6rElqBPH/Sh7lkLNHakT6Nl GK9yhNMLtXX35vcheKzCUib6XvGJK7ykkmoenHWDyBBuPPB+Di1yXlb/7i4Fb1EkL7tbM07 OMFnNZA92rBiMIxFzC2EIsZoz8RyDnJsQj7qlXscEZxPoPwM/uQbvNoW3BiTbIIcsLimKC9 qyk8QrLoRpbalNxjEna3Q== X-UI-Out-Filterresults: notjunk:1;V01:K0:1J3gLHpeqGE=:0w5Vc46XNRElq3v72sZlY6 rgVLsWHe5sGaaOLNytRhQQI8LlkCKEdjrM+Ri/+krf3FvdoCQNpH5+eVhp+FoPrqWySd/A1z8 2EK2gLzWma55YPKDUDdOOL5qezd5ivf3+vbTjRVbQqmSAz1WAHKf2N0aw1qPxaFbCTyzdc6GB UUQDvgmf85a/7jmpCS0ivL2YOH5b6063hEwr69Fhq3K8fOop0r6FgGx6W8XqE8WwoOennSsT7 qTA8syXzaLRxQd8jUY5ROluexLkCkvJ6CdAGVML+QpiZAaekureZTX5F/qXAYjv7WnzuBPude 5j3IaDCrqFd+TIaPlwCerBG7v8JfqBdWy8MP3ASlUgsXp4N41uCa7ShTbx1xKyR9xNLcKpC5Z O5XX5bpD+sLwJ8AI4WMSA+mCrHlsEKYUM1AFTf0r1b4g9zFuuglQMNPU7WonfJDR1TncLRtp2 Qv62Vcl6wCYINy9+Xh0lSl9C+RAivusY5bEWg4aRrXy6EAorQsel5RmY/X4Sd8xvYmZOJmorI 7+KAifH/XBalk4P0XLYfDsUcgv2nMFbNZd8k7Q2Uj9LofOVn88tzWtvGjoc7sX29vxWUAi90t ecplo7M7OSS/7EAuPbPaYfJfu5JeftxhmOxPsi7/IobRMdBSKkMV6iGwHYjJbI3yYZrlrw4Ep IcvW/4UcoYUisdlD8lWszNLn1+LASqlI7I9+rNy6qdTOmoAZG+uNtStI6WPvB3Ju3AHE= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --Sig_/AcTbSniZ/wTXtYlT40_Sr8_ Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Fri, 1 Jul 2016 13:55:44 +0200 Hans Verkuil wrote: > On 05/11/2016 06:32 PM, Alban Bedel wrote: > > On Wed, 11 May 2016 12:22:44 -0400 > > Javier Martinez Canillas wrote: > > =20 > >> Hello Alban, > >> > >> On 05/11/2016 11:40 AM, Alban Bedel wrote: =20 > >>> In v4l2_async_test_notify() if the registered_async callback or the > >>> complete notifier returns an error the subdev is not unregistered. > >>> This leave paths where v4l2_async_register_subdev() can fail but > >>> leave the subdev still registered. > >>> > >>> Add the required calls to v4l2_device_unregister_subdev() to plug > >>> these holes. > >>> > >>> Signed-off-by: Alban Bedel > >>> --- > >>> drivers/media/v4l2-core/v4l2-async.c | 10 ++++++++-- > >>> 1 file changed, 8 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l= 2-core/v4l2-async.c > >>> index ceb28d4..43393f8 100644 > >>> --- a/drivers/media/v4l2-core/v4l2-async.c > >>> +++ b/drivers/media/v4l2-core/v4l2-async.c > >>> @@ -121,13 +121,19 @@ static int v4l2_async_test_notify(struct v4l2_a= sync_notifier *notifier, > >>> =20 > >>> ret =3D v4l2_subdev_call(sd, core, registered_async); > >>> if (ret < 0 && ret !=3D -ENOIOCTLCMD) { > >>> + v4l2_device_unregister_subdev(sd); > >>> if (notifier->unbind) > >>> notifier->unbind(notifier, sd, asd); > >>> return ret; > >>> } > >>> =20 > >>> - if (list_empty(¬ifier->waiting) && notifier->complete) > >>> - return notifier->complete(notifier); > >>> + if (list_empty(¬ifier->waiting) && notifier->complete) { > >>> + ret =3D notifier->complete(notifier); > >>> + if (ret < 0) { > >>> + v4l2_device_unregister_subdev(sd); =20 > >> > >> Isn't a call to notifier->unbind() missing here as well? > >> > >> Also, I think the error path is becoming too duplicated and complex, so > >> maybe we can have a single error path and use goto labels as is common > >> in Linux? For example something like the following (not tested) can be > >> squashed on top of your change: =20 > >=20 > > Yes, that look better. I'll test it and report tomorrow. =20 >=20 > I haven't heard anything back about this. Did you manage to test it? Yes, that's working fine. Sorry for the delay, I'm sending the v2 patch. Alban --Sig_/AcTbSniZ/wTXtYlT40_Sr8_ Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXvaXuAAoJEHSUmkuduC285AUQANhHvlzcasSofAqH4eHyyrUY wM4whFXW9djk6FnM++djql7Au10dlCDk4u36mo0hq13fa6OM81o/OeXb+SifK9mO 64mH9C0YreGLlXeJ9VOO1REDd3Scu4L5i9Hpra2DxYkkTQS0CC+uAuZ5kWp25csT NyV69kHw4NDJZdzzha0ABfjniEQq/aD0vZnTITjhZs2Trkl8zRaiXEePhRMqUUGn 4agczXV82kW89FCmAVXSNOqHP/pWko7sIWtFNnFbwo42bCbxRNbIcEXh+ZA+EVSq pwX9gG1d5vnFxH3gyDeTeDY2nk/e2gQunYxKHrGERN6S+Xd0dNwrDP/2lDz+Ke05 qDdu8cxuCbxv91gng3sqmACC0qR/z6VIThiS01SCgRzMLOv0ZuF8Rj/Ii3kvdmrN WME0kxTfpim3+bdHRUo+GZhcT5eEGVU7MqcyA/ysjS1NEF5+SAZa9lW2XEDsJK9i 4nzfy2o6N0VF6bWWjPYt8bwN24+i4KYKcstmHxCIRDxskJi3VEakoXGoO04Ecow7 bwTXroOni2xgoyGEX16lIfZBdl4WWgLZeLN0kU+LzPOrknbbu13EjDHa3W2wA2tb 8aG5Of5d3bqMb/f/di/RztVYA2NpO4PcXM8D1/iacXiMn0Mq0ePrbpxzdmTAgnXR AKzc/oitUEwM4xEdYwBd =JFl1 -----END PGP SIGNATURE----- --Sig_/AcTbSniZ/wTXtYlT40_Sr8_--