On Tue, Feb 28, 2023 at 8:05 PM Maxime Ripard <maxime@xxxxxxxxxx> wrote: > > 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. Okay. So, even if we manually use drm_of_dsi_find_panel_or_bridge in mipi_dsi_host_ops.attach and drm_panel_bridge_remove() in mipi_dsi_host_ops.detach KMS doesn't aware of it and is still unsafe. - am I correct? Jagan.
