linux-kernel - Re: [PATCH v4 1/3] drm/bridge/synopsys: dsi: stop clobbering drvdata

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171205185651.GA136446@google.com>
Date:   Tue, 5 Dec 2017 10:56:52 -0800
From:   Brian Norris <briannorris@...omium.org>
To:     Nickey Yang <nickey.yang@...k-chips.com>
Cc:     Philippe CORNU <philippe.cornu@...com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "heiko@...ech.de" <heiko@...ech.de>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "airlied@...ux.ie" <airlied@...ux.ie>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-rockchip@...ts.infradead.org" 
        <linux-rockchip@...ts.infradead.org>,
        "laurent.pinchart@...asonboard.com" 
        <laurent.pinchart@...asonboard.com>,
        "seanpaul@...omium.org" <seanpaul@...omium.org>,
        "mka@...omium.org" <mka@...omium.org>,
        "hoegsberg@...il.com" <hoegsberg@...il.com>,
        "architt@...eaurora.org" <architt@...eaurora.org>,
        Yannick FERTRE <yannick.fertre@...com>,
        "hl@...k-chips.com" <hl@...k-chips.com>,
        "zyw@...k-chips.com" <zyw@...k-chips.com>,
        "xbl@...k-chips.com" <xbl@...k-chips.com>
Subject: Re: [PATCH v4 1/3] drm/bridge/synopsys: dsi: stop clobbering drvdata

Hi Nickey,

On Tue, Dec 05, 2017 at 05:14:11PM +0800, Nickey Yang wrote:
> On 2017年12月01日 18:07, Philippe CORNU wrote:
> >On 12/01/2017 10:11 AM, Nickey Yang wrote:
> >>On 2017年12月01日 16:32, Philippe CORNU wrote:
> >>>I am sorry to say that but you can not add my "Acked-by" to this patch
> >>>because this code is different from the "original" one from Brian (which
> >>>got my "Acked-by").
> >>I'm sorry I didn't think much about it, Thank you for correcting me.
> >>>Sometimes it is not an issue because differences are not important but
> >>>in this particular case, the code is really different: you have remove
> >>>platform_set_drvdata() & platform_get_drvdata() in the stm part.
> >>>
> >>>Could you please go back to the original code or propose me an updated
> >>>version of this code.
> >>Could you help update new version of this code(stm part) and then test on
> >>stm platform?
> >I think you can simply goes back to the original version from Brian (see
> >the discussion thread in https://patchwork.kernel.org/patch/10078493/)
> >unless you have specific/good reasons for modifying the code as you did.
> mmm,I'm sorry, I feel a little puzzled. Do you means we should abandon
> Brian's patch (https://patchwork.kernel.org/patch/10078493/)?
> I think we need to adjust stm part because  dw_mipi_dsi_stm.c calls
> bridge's drivers if we want merge Brian's patch.

It's really simple. Your code is different from the patch I sent, and in
a way that Philippe did not like. I'll highlight it again below:

> >>>>diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> >>>>index e5b6310..80f9950 100644
> >>>>--- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> >>>>+++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> >>>>@@ -66,6 +66,7 @@ enum dsi_color {
> >>>>    struct dw_mipi_dsi_stm {
> >>>>    	void __iomem *base;
> >>>>    	struct clk *pllref_clk;
> >>>>+	struct dw_mipi_dsi *dmd;
> >>>>    };
> >>>>    static inline void dsi_write(struct dw_mipi_dsi_stm *dsi, u32 reg, u32 val)
> >>>>@@ -318,10 +319,11 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev)
> >>>>    	dw_mipi_dsi_stm_plat_data.base = dsi->base;
> >>>>    	dw_mipi_dsi_stm_plat_data.priv_data = dsi;
> >>>>-	ret = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data);
> >>>>-	if (ret) {
> >>>>+	dsi->dmd = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data);
> >>>>+	if (IS_ERR(dsi->dmd)) {
> >>>>    		DRM_ERROR("Failed to initialize mipi dsi host\n");
> >>>>    		clk_disable_unprepare(dsi->pllref_clk);
> >>>>+		return PTR_ERR(dsi->dmd);
> >>>>    	}
> >>>>    	return ret;
> >>>>@@ -332,7 +334,7 @@ static int dw_mipi_dsi_stm_remove(struct platform_device *pdev)
> >>>>    	struct dw_mipi_dsi_stm *dsi = dw_mipi_dsi_stm_plat_data.priv_data;
> >>>>    	clk_disable_unprepare(dsi->pllref_clk);
> >>>>-	dw_mipi_dsi_remove(pdev);
> >>>>+	dw_mipi_dsi_remove(dsi->dmd);
> >>>>    	return 0;
> >>>>    }
 
Above is your diff for dw_mipi_dsi-stm.c. Particularly, notice that
remove() is directly referencing the static dw_mipi_dsi_stm_plat_data
struct.

If you look back at my patch [1] you'll see that you're missing hunks
like this:

 static int dw_mipi_dsi_stm_remove(struct platform_device *pdev)
 {
-	struct dw_mipi_dsi_stm *dsi = dw_mipi_dsi_stm_plat_data.priv_data;
+	struct dw_mipi_dsi_stm *dsi = platform_get_drvdata(pdev);
 
 	clk_disable_unprepare(dsi->pllref_clk);
[...]

Brian

[1] https://patchwork.kernel.org/patch/10078493/