| 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
| ||
|
Date: Mon, 24 Apr 2023 16:23:17 -0700
From: Abhinav Kumar <quic_abhinavk@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Marijn Suijten <marijn.suijten@...ainline.org>
CC: Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>,
<~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/24/2023 3:54 PM, Dmitry Baryshkov wrote:
> On Tue, 25 Apr 2023 at 01:03, Marijn Suijten
> <marijn.suijten@...ainline.org> wrote:
>>
>> On 2023-04-21 16:25:15, Abhinav Kumar wrote:
>>>
>>>
>>> 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.
>>
>> I mainly intended to keep original validation where _intf_offset would
>> skip INTF_NONE, and error out. RM init is hence expected to filter out
>> INTF_NONE instead of running into that `-EINVAL`, which I maintained
>> here.
>>
>> If you think there won't be another caller of dpu_hw_intf_init, and that
>> such validation is hence excessive, I can remove it in a followup v3.
>
> I'd prefer to see the checks at dpu_rm to be dropped.
> dpu_hw_intf_init() (and other dpu_hw_foo_init() functions) should be
> self-contained. If they can not init HW block (e.g. because the index
> is out of the boundaries), they should return an error.
>
They already do that today because even without this it will call into
_intf_offset() and that will bail out for INTF_NONE.
I feel this is a duplicated check because the caller with the loop needs
to validate the index before passing it to dpu_hw_intf_init() otherwise
the loop will get broken at the first return of the error and rest of
the blocks will also not be initialized.
>>
>> - Marijn
>>
>>>> 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>
>
>
>
Powered by blists - more mailing lists