linux-kernel - Re: [PATCH v2 3/3] drm/msm/dpu: Pass catalog pointers directly from RM instead of IDs

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: <50d22e0c-84b3-0678-eb06-30fb66fd24cf@quicinc.com>
Date:   Fri, 21 Apr 2023 16:25:15 -0700
From:   Abhinav Kumar <quic_abhinavk@...cinc.com>
To:     Marijn Suijten <marijn.suijten@...ainline.org>,
        Rob Clark <robdclark@...il.com>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Sean Paul <sean@...rly.run>, David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>
CC:     <~postmarketos/upstreaming@...ts.sr.ht>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Martin Botka <martin.botka@...ainline.org>,
        Jami Kettunen <jami.kettunen@...ainline.org>,
        Jordan Crouse <jordan@...micpenguin.net>,
        <linux-arm-msm@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
        <freedreno@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/3] drm/msm/dpu: Pass catalog pointers directly from
 RM instead of IDs



On 4/21/2023 1:53 PM, Marijn Suijten wrote:
> The Resource Manager already iterates over all available blocks from the
> catalog, only to pass their ID to a dpu_hw_xxx_init() function which
> uses an _xxx_offset() helper to search for and find the exact same
> catalog pointer again to initialize the block with, fallible error
> handling and all.
> 
> Instead, pass const pointers to the catalog entries directly to these
> _init functions and drop the for loops entirely, saving on both
> readability complexity and unnecessary cycles at boot.
> 
> Signed-off-by: Marijn Suijten <marijn.suijten@...ainline.org>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>

Overall, a nice cleanup!

One comment below.

> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c        | 37 +++++----------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h        | 14 ++++----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c        | 32 +++---------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h        | 11 +++----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.c       | 38 ++++-----------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.h       | 12 +++----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h |  2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c       | 40 ++++++-----------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h       | 12 +++----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c         | 38 ++++-----------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.h         | 10 +++---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_merge3d.c    | 33 +++----------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_merge3d.h    | 14 ++++----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c   | 33 +++----------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h   | 14 ++++----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c       | 39 ++++------------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h       | 12 +++----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.c       | 33 +++----------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.h       | 11 +++----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c         | 33 ++++---------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h         | 11 +++----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c           | 17 +++++-----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c            | 18 +++++-----
>   23 files changed, 139 insertions(+), 375 deletions(-)
> 

<snipped>

> -struct dpu_hw_intf *dpu_hw_intf_init(enum dpu_intf idx,
> -		void __iomem *addr,
> -		const struct dpu_mdss_cfg *m)
> +struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
> +		void __iomem *addr)
>   {
>   	struct dpu_hw_intf *c;
> -	const struct dpu_intf_cfg *cfg;
> +
> +	if (cfg->type == INTF_NONE) {
> +		pr_err("Cannot create interface hw object for INTF_NONE type\n");
> +		return ERR_PTR(-EINVAL);
> +	}

The caller of dpu_hw_intf_init which is the RM already has protection 
for INTF_NONE, see below

         for (i = 0; i < cat->intf_count; i++) {
                 struct dpu_hw_intf *hw;
                 const struct dpu_intf_cfg *intf = &cat->intf[i];

                 if (intf->type == INTF_NONE) {
                         DPU_DEBUG("skip intf %d with type none\n", i);
                         continue;
                 }
                 if (intf->id < INTF_0 || intf->id >= INTF_MAX) {
                         DPU_ERROR("skip intf %d with invalid id\n", 
intf->id);
                         continue;
                 }
                 hw = dpu_hw_intf_init(intf->id, mmio, cat);

So this part can be dropped.

>   
>   	c = kzalloc(sizeof(*c), GFP_KERNEL);
>   	if (!c)
>   		return ERR_PTR(-ENOMEM);
>   
> -	cfg = _intf_offset(idx, m, addr, &c->hw);
> -	if (IS_ERR_OR_NULL(cfg)) {
> -		kfree(c);
> -		pr_err("failed to create dpu_hw_intf %d\n", idx);
> -		return ERR_PTR(-EINVAL);
> -	}
> +	c->hw.blk_addr = addr + cfg->base;
> +	c->hw.log_mask = DPU_DBG_MASK_INTF;
>   

<snipped>