linux-kernel - Re: [PATCH v2 2/3] drm/rockchip: Add ROCKCHIP DW MIPI DSI controller driver

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: <20171128052400.GA14640@google.com>
Date:   Mon, 27 Nov 2017 21:24:02 -0800
From:   Brian Norris <briannorris@...omium.org>
To:     Nickey Yang <nickey.yang@...k-chips.com>
Cc:     mark.yao@...k-chips.com, robh+dt@...nel.org, heiko@...ech.de,
        mark.rutland@....com, airlied@...ux.ie,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-rockchip@...ts.infradead.org, seanpaul@...omium.org,
        mka@...omium.org, hoegsberg@...il.com, architt@...eaurora.org,
        philippe.cornu@...com, yannick.fertre@...com, hl@...k-chips.com,
        zyw@...k-chips.com, xbl@...k-chips.com
Subject: Re: [PATCH v2 2/3] drm/rockchip: Add ROCKCHIP DW MIPI DSI controller
 driver

Hi Nickey,

Thanks for the quick version 2. You've fixed most of the the problems I
pointed out, but you've missed at least one.

On Tue, Nov 28, 2017 at 09:55:24AM +0800, Nickey Yang wrote:
> Add the ROCKCHIP DSI controller driver that uses the Synopsys DesignWare
> MIPI DSI host controller bridge.
> 
> Signed-off-by: Nickey Yang <nickey.yang@...k-chips.com>
> ---

I suppose you've included a changelog in the cover letter (thanks!), but
sometimes it helps to go here too, to give an individual history of each
patch.

>  drivers/gpu/drm/rockchip/Kconfig                |    2 +-
>  drivers/gpu/drm/rockchip/Makefile               |    2 +-
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c          | 1349 -----------------------
>  drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c |  756 +++++++++++++
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c     |    2 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h     |    2 +-
>  6 files changed, 760 insertions(+), 1353 deletions(-)
>  delete mode 100644 drivers/gpu/drm/rockchip/dw-mipi-dsi.c
>  create mode 100644 drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c
> 


> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c
> new file mode 100644
> index 0000000..c919d4f
> --- /dev/null
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c
> @@ -0,0 +1,756 @@
[...]
> +static int dw_mipi_dsi_rockchip_bind(struct device *dev,
> +				     struct device *master,
> +				     void *data)
> +{
> +	const struct of_device_id *of_id =
> +			of_match_device(dw_mipi_dsi_rockchip_dt_ids, dev);
> +	const struct rockchip_dw_dsi_chip_data *cdata = of_id->data;
> +	struct dw_mipi_dsi_rockchip *dsi = dev_get_drvdata(dev);

Hmm, so I see 2 instances of 'dev_get_drvdata()' in this driver, and one
of them is here. I never see a 'dev_set_drvdata()', so that can't
possibly be useful.

In this case...you've actually moved all the 'probe()' logic into
bind(), so there's no driver data to carry over here. And instead, you
just clobber this 'dsi = ...' assignment a few lines below:

> +	struct resource *res;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct drm_device *drm_dev = data;
> +	struct device_node *np = dev->of_node;
> +	int ret;
> +
> +	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);

^^^ See here.

So if you don't actually need to stash anything in drvdata between
probe() and bind(), then you might get away without my patch:

[PATCH] drm/bridge/synopsis: stop clobbering drvdata
https://patchwork.kernel.org/patch/10078493/

There's one other instance to review though:

> +	if (!dsi)
> +		return -ENOMEM;
> +
[...]

> +static void dw_mipi_dsi_rockchip_unbind(struct device *dev,
> +					struct device *master,
> +					void *data)
> +{
> +	struct dw_mipi_dsi_rockchip *dsi = dev_get_drvdata(dev);

^^ That's wrong. You either want to do your own 'dev_set_drvdata()'
somewhere (and make sure the bridge driver doesn't clobber it, e.g.,
with my patch above), or else you need to convince the bridge driver to
give us back something like its 'priv_data' pointer
(dsi->plat_data->priv_data).

Also, don't you need to call dw_mipi_dsi_unbind() here?

> +	clk_disable_unprepare(dsi->pllref_clk);
> +}
> +
> +static const struct component_ops dw_mipi_dsi_rockchip_ops = {
> +	.bind	= dw_mipi_dsi_rockchip_bind,
> +	.unbind	= dw_mipi_dsi_rockchip_unbind,
> +};

...

Brian