From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 28318C388F7 for ; Tue, 10 Nov 2020 13:38:58 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D60C0207D3 for ; Tue, 10 Nov 2020 13:38:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730617AbgKJNi5 (ORCPT ); Tue, 10 Nov 2020 08:38:57 -0500 Received: from mx2.suse.de ([195.135.220.15]:35390 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730534AbgKJNi4 (ORCPT ); Tue, 10 Nov 2020 08:38:56 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 7AAC6ACDB; Tue, 10 Nov 2020 13:38:54 +0000 (UTC) Message-ID: <25933d5863cd6ddb98dea25bdedf342ebd063480.camel@suse.de> Subject: Re: [PATCH v3 01/11] firmware: raspberrypi: Introduce devm_rpi_firmware_get() From: Nicolas Saenz Julienne To: Bartosz Golaszewski Cc: Uwe =?ISO-8859-1?Q?Kleine-K=F6nig?= , LKML , Florian Fainelli , Ray Jui , Scott Branden , bcm-kernel-feedback-list@broadcom.com, linux-pwm@vger.kernel.org, arm-soc , linux-devicetree , wahrenst@gmx.net, Linux Input , Dmitry Torokhov , Greg KH , devel@driverdev.osuosl.org, Philipp Zabel , linux-gpio , Linus Walleij , linux-clk , Stephen Boyd , linux-rpi-kernel@lists.infradead.org, Andy Shevchenko Date: Tue, 10 Nov 2020 14:38:52 +0100 In-Reply-To: References: <20201104103938.1286-1-nsaenzjulienne@suse.de> <20201104103938.1286-2-nsaenzjulienne@suse.de> <47eaba0bc71c6e23bff87b8a01cebf0c6d12efd0.camel@suse.de> Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="=-aSQnVn7Q9FP2OogzR6s5" User-Agent: Evolution 3.36.5 MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org --=-aSQnVn7Q9FP2OogzR6s5 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Bartosz, thanks for the feedback. On Thu, 2020-11-05 at 10:42 +0100, Bartosz Golaszewski wrote: > On Thu, Nov 5, 2020 at 10:28 AM Nicolas Saenz Julienne > wrote: > > Hi Bartosz, thanks for the review. > >=20 > > On Thu, 2020-11-05 at 10:13 +0100, Bartosz Golaszewski wrote: > > > > +/** > > > > + * devm_rpi_firmware_get - Get pointer to rpi_firmware structure. > > > > + * @firmware_node: Pointer to the firmware Device Tree node. > > > > + * > > > > + * Returns NULL is the firmware device is not ready. > > > > + */ > > > > +struct rpi_firmware *devm_rpi_firmware_get(struct device *dev, > > > > + struct device_node *firm= ware_node) > > > > +{ > > > > + struct platform_device *pdev =3D of_find_device_by_node(fir= mware_node); > > > > + struct rpi_firmware *fw; > > > > + > > > > + if (!pdev) > > > > + return NULL; > > > > + > > > > + fw =3D platform_get_drvdata(pdev); > > > > + if (!fw) > > > > + return NULL; > > > > + > > > > + if (!refcount_inc_not_zero(&fw->consumers)) > > > > + return NULL; > > > > + > > > > + if (devm_add_action_or_reset(dev, rpi_firmware_put, fw)) > > > > + return NULL; > > > > + > > > > + return fw; > > > > +} > > > > +EXPORT_SYMBOL_GPL(devm_rpi_firmware_get); > > >=20 > > > Usually I'd expect the devres variant to simply call > > > rpi_firmware_get() and then schedule a release callback which would > > > call whatever function is the release counterpart for it currently. > > > Devres actions are for drivers which want to schedule some more > > > unusual tasks at driver detach. Any reason for designing it this way? > >=20 > > Yes, see patch #8 where I get rid of rpi_firmware_get() altogether afte= r > > converting all users to devres. Since there is no use for the vanilla v= ersion > > of the function anymore, I figured it'd be better to merge everything i= nto > > devm_rpi_firmware_get(). That said it's not something I have strong fee= lings > > about. > >=20 >=20 > I see. So the previous version didn't really have any reference > counting and it leaked the reference returned by > of_find_device_by_node(), got it. Could you just clarify for me the > logic behind the wait_queue in rpi_firmware_remove()? If the firmware > driver gets detached and remove() stops on the wait_queue - it will be > stuck until the last user releases the firmware. I'm not sure this is > correct. Yes, that's what I meant to implement. > I'd prefer to see a kref with a release callback and remove > would simply decrease the kref and return. Each user would do the same > and then after the last user is detached the firmware would be > destroyed. Sounds good to me. I'll update it. > Don't we really have some centralized firmware subsystem that would handl= e this? Sadly no, this is an RPi specific thing, it doesn't live behind a standard = like other firmware based protocols (for ex. SCMI), and evolves as the needs ari= se. Regards, Nicolas --=-aSQnVn7Q9FP2OogzR6s5 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEErOkkGDHCg2EbPcGjlfZmHno8x/4FAl+ql+wACgkQlfZmHno8 x/5TFQgArzH6eU5ljiN7do5NqV1SI7f2HoX88NazrWiPc8Ixl7QT4jfzWnZeyiBn 31OdfWDVQeexADs3RDEvq/o5SSxNP+FDGlnzm9PiYaKPWcGdOpe8wW9wggXest4N MVtyqksGQlf3MuItqI4HgP/aAhB8EKnYHTVrku2tAPR9cNliVmeusFWsPWIYXSYc IcX61cPnzFkqU56k7aNrk452Kme6XDFDi2eD2DXAzHNlSHiQOH5ZQPKBmFUkaCDL xP/T5PwL+YwF3ZWO2sU6voMp96QfiO8R/LYt215dIzlmTmdKcIC7scqEkr4HRSJp 9SW2n981ery3AA1wXoyGhLMJMilzcQ== =CaET -----END PGP SIGNATURE----- --=-aSQnVn7Q9FP2OogzR6s5--