On Tue, Feb 28, 2023 at 06:04:39PM +0530, Jagan Teki wrote: > On Tue, Feb 28, 2023 at 5:40 PM Maxime Ripard <maxime@xxxxxxxxxx> wrote: > > > > On Mon, Feb 27, 2023 at 08:41:22PM +0100, Marek Vasut wrote: > > > > If we go ahead with no need for DRM-managed helper at the moment, then > > > > find the panel hook in host.attach and then drop 2/18. > > > > > > The panel lookup must happen in .bind/.probe for exynos/imx respectively , > > > that's really all that is required here. Then you can drop 1,2,3/18 and get > > > this series applied (I hope) . > > > > > > Can you implement just this one change ? > > > > > > There is no need to use drmm_* helper for now, that can be improved later if > > > possible. > > > > Yeah... The drmm helper isn't needed per se, but not using it will > > create a use-after-free pattern that is very easy to miss. > > > > I'd really prefer not to add a new helper that favors an unsafe pattern, > > but the driver seems to have a whole bunch of them anyway so it's not > > really a big deal. > > > > Which also raises another question: if it's code that is only really > > relevant in the context of that driver, why are you creating a helper > > for it in the first place? > > I can answer this question as I did add these helpers. > > 1. DSI-specific helper added since it is a good candidate for > common/helper code, based on the comments in V9 by Marek. V > https://patchwork.kernel.org/project/dri-devel/patch/20221209152343.180139-8-jagan@xxxxxxxxxxxxxxxxxxxx/ > > So, I have added this to the common drm_of code in V10. > https://patchwork.kernel.org/project/dri-devel/patch/20221214125907.376148-2-jagan@xxxxxxxxxxxxxxxxxxxx/ > > 2. DRM-managed discussion was commented on in V11 by you, so from > where all discussion moved. > https://patchwork.kernel.org/project/dri-devel/patch/20230123151212.269082-3-jagan@xxxxxxxxxxxxxxxxxxxx/ > > 1) helper wouldn't be an unsafe helper as it can reuse many DSI > drivers but 2) helper might be an unsafe helper at the moment. You're wrong. The first one is unsafe, for the same reason than the devm one you did is unsafe: the assumption everyone will get (and the one you implemented in your driver) is that the bridge reference will be put back at remove time. The DRM/KMS structures however are free'd only when the last user closes the file descriptor of the KMS device, so your driver functions will get called after remove has been called. If you are using the panel or bridge in any of your KMS hooks implementations, this is now a use-after-free. The drmm variant is safe though, because drmm actions run not when the device is removed but when the DRM device is last closed. Maxime
Attachment:
signature.asc
Description: PGP signature
