* Re: [PATCH v2] drm/mediatek: fix uninitialized symbol
[not found] <20230421021609.7730-1-nancy.lin@mediatek.com>
@ 2023-04-24 7:04 ` AngeloGioacchino Del Regno
2023-07-13 3:34 ` Fei Shao
2023-07-14 9:27 ` CK Hu (胡俊光)
2 siblings, 0 replies; 6+ messages in thread
From: AngeloGioacchino Del Regno @ 2023-04-24 7:04 UTC (permalink / raw)
To: Nancy.Lin, Chun-Kuang Hu, Philipp Zabel, Matthias Brugger
Cc: David Airlie, Daniel Vetter, dri-devel, linux-mediatek,
linux-kernel, linux-arm-kernel, clang-built-linux,
Project_Global_Chrome_Upstream_Group, singo.chang
Il 21/04/23 04:16, Nancy.Lin ha scritto:
> fix Smatch static checker warning
> - uninitialized symbol comp_pdev in mtk_ddp_comp_init.
>
> Fixes: 0d9eee9118b7 ("drm/mediatek: Add drm ovl_adaptor sub driver for MT8195")
> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] drm/mediatek: fix uninitialized symbol
[not found] <20230421021609.7730-1-nancy.lin@mediatek.com>
2023-04-24 7:04 ` [PATCH v2] drm/mediatek: fix uninitialized symbol AngeloGioacchino Del Regno
@ 2023-07-13 3:34 ` Fei Shao
2023-07-14 9:27 ` CK Hu (胡俊光)
2 siblings, 0 replies; 6+ messages in thread
From: Fei Shao @ 2023-07-13 3:34 UTC (permalink / raw)
To: Nancy.Lin
Cc: Chun-Kuang Hu, Philipp Zabel, Matthias Brugger,
AngeloGioacchino Del Regno, David Airlie, Daniel Vetter,
dri-devel, linux-mediatek, linux-kernel, linux-arm-kernel,
clang-built-linux, Project_Global_Chrome_Upstream_Group,
singo.chang
On Fri, Apr 21, 2023 at 10:16 AM Nancy.Lin <nancy.lin@mediatek.com> wrote:
>
> fix Smatch static checker warning
> - uninitialized symbol comp_pdev in mtk_ddp_comp_init.
>
> Fixes: 0d9eee9118b7 ("drm/mediatek: Add drm ovl_adaptor sub driver for MT8195")
> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
Reviewed-by: Fei Shao <fshao@chromium.org>
This seems to be unnoticed and I just want to get some attention for
it. Any action items here?
Regards,
Fei
> ---
> v2: add Fixes tag
> ---
> drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> index f114da4d36a9..e987ac4481bc 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -546,7 +546,7 @@ unsigned int mtk_drm_find_possible_crtc_by_comp(struct drm_device *drm,
> int mtk_ddp_comp_init(struct device_node *node, struct mtk_ddp_comp *comp,
> unsigned int comp_id)
> {
> - struct platform_device *comp_pdev;
> + struct platform_device *comp_pdev = NULL;
> enum mtk_ddp_comp_type type;
> struct mtk_ddp_comp_dev *priv;
> #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> @@ -588,6 +588,9 @@ int mtk_ddp_comp_init(struct device_node *node, struct mtk_ddp_comp *comp,
> type == MTK_DSI)
> return 0;
>
> + if (!comp_pdev)
> + return -EPROBE_DEFER;
> +
> priv = devm_kzalloc(comp->dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> return -ENOMEM;
> --
> 2.18.0
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] drm/mediatek: fix uninitialized symbol
[not found] <20230421021609.7730-1-nancy.lin@mediatek.com>
2023-04-24 7:04 ` [PATCH v2] drm/mediatek: fix uninitialized symbol AngeloGioacchino Del Regno
2023-07-13 3:34 ` Fei Shao
@ 2023-07-14 9:27 ` CK Hu (胡俊光)
2023-07-17 3:59 ` Fei Shao
2 siblings, 1 reply; 6+ messages in thread
From: CK Hu (胡俊光) @ 2023-07-14 9:27 UTC (permalink / raw)
To: p.zabel, Nancy Lin (林欣螢),
matthias.bgg, chunkuang.hu, angelogioacchino.delregno
Cc: dri-devel, linux-kernel, linux-mediatek, linux-arm-kernel,
clang-built-linux, Singo Chang (張興國),
Project_Global_Chrome_Upstream_Group
Hi, Nancy:
On Fri, 2023-04-21 at 10:16 +0800, Nancy.Lin wrote:
> fix Smatch static checker warning
> - uninitialized symbol comp_pdev in mtk_ddp_comp_init.
>
> Fixes: 0d9eee9118b7 ("drm/mediatek: Add drm ovl_adaptor sub driver
> for MT8195")
> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> ---
> v2: add Fixes tag
> ---
> drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> index f114da4d36a9..e987ac4481bc 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -546,7 +546,7 @@ unsigned int
> mtk_drm_find_possible_crtc_by_comp(struct drm_device *drm,
> int mtk_ddp_comp_init(struct device_node *node, struct mtk_ddp_comp
> *comp,
> unsigned int comp_id)
> {
> - struct platform_device *comp_pdev;
> + struct platform_device *comp_pdev = NULL;
> enum mtk_ddp_comp_type type;
> struct mtk_ddp_comp_dev *priv;
> #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> @@ -588,6 +588,9 @@ int mtk_ddp_comp_init(struct device_node *node,
> struct mtk_ddp_comp *comp,
> type == MTK_DSI)
> return 0;
>
> + if (!comp_pdev)
> + return -EPROBE_DEFER;
In line 566, the statement is
if (nodo) {
comp_pdev = ...
}
The comment says that only ovl_adaptoer has no device node, so the
checking should be
if (type != MTK_DISP_OVL_ADAPTOR) {
comp_pdev = ...
}
and later it would return when type = MTK_DISP_OVL_ADAPTOR,
so there would be no problem of uninitialized symbol.
Regards,
CK
> +
> priv = devm_kzalloc(comp->dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> return -ENOMEM;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] drm/mediatek: fix uninitialized symbol
2023-07-14 9:27 ` CK Hu (胡俊光)
@ 2023-07-17 3:59 ` Fei Shao
2023-07-20 7:52 ` CK Hu (胡俊光)
0 siblings, 1 reply; 6+ messages in thread
From: Fei Shao @ 2023-07-17 3:59 UTC (permalink / raw)
To: CK Hu (胡俊光)
Cc: p.zabel, Nancy Lin (林欣螢),
matthias.bgg, chunkuang.hu, angelogioacchino.delregno, dri-devel,
linux-kernel, linux-mediatek, linux-arm-kernel,
clang-built-linux, Singo Chang (張興國),
Project_Global_Chrome_Upstream_Group
Hi CK,
On Fri, Jul 14, 2023 at 5:27 PM CK Hu (胡俊光) <ck.hu@mediatek.com> wrote:
>
> Hi, Nancy:
>
> On Fri, 2023-04-21 at 10:16 +0800, Nancy.Lin wrote:
snip
>
> In line 566, the statement is
>
> if (nodo) {
> comp_pdev = ...
> }
>
> The comment says that only ovl_adaptoer has no device node, so the
> checking should be
>
> if (type != MTK_DISP_OVL_ADAPTOR) {
> comp_pdev = ...
> }
>
> and later it would return when type = MTK_DISP_OVL_ADAPTOR,
> so there would be no problem of uninitialized symbol.
That sounds fair, but IIUC what Nancy tries to resolve here is the
false-positive Smatch warning.
How about this: given the `if (node)` block was exclusively added for
ovl_adaptor in [1], plus the init function will immediately return
after that in this case, it should be safe to do the following
```
/* Not all drm components have a DTS device node... */
if (node == NULL)
return 0;
comp_pdev = of_find_device_by_node(node);
...
if (type == MTK_DISP_AAL ||
...
```
which is equivalent to adding a `node == NULL` check before [1].
This should suppress the Smatch warning because `comp_pdev` will be
(again) unconditionally assigned to something, and the `type ==
MTK_DISP_OVL_ADAPTOR` line can be dropped also (optional?).
[1]: commit 0d9eee9118b7 ("drm/mediatek: Add drm ovl_adaptor sub
driver for MT8195")
Regards,
Fei
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] drm/mediatek: fix uninitialized symbol
2023-07-17 3:59 ` Fei Shao
@ 2023-07-20 7:52 ` CK Hu (胡俊光)
2023-08-03 8:32 ` Nancy Lin (林欣螢)
0 siblings, 1 reply; 6+ messages in thread
From: CK Hu (胡俊光) @ 2023-07-20 7:52 UTC (permalink / raw)
To: fshao
Cc: linux-mediatek, linux-kernel,
Singo Chang (張興國),
chunkuang.hu, Nancy Lin (林欣螢),
p.zabel, dri-devel, Project_Global_Chrome_Upstream_Group,
linux-arm-kernel, clang-built-linux, matthias.bgg,
angelogioacchino.delregno
Hi, Fei:
On Mon, 2023-07-17 at 11:59 +0800, Fei Shao wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Hi CK,
>
> On Fri, Jul 14, 2023 at 5:27 PM CK Hu (胡俊光) <ck.hu@mediatek.com>
> wrote:
> >
> > Hi, Nancy:
> >
> > On Fri, 2023-04-21 at 10:16 +0800, Nancy.Lin wrote:
> snip
> >
> > In line 566, the statement is
> >
> > if (nodo) {
> > comp_pdev = ...
> > }
> >
> > The comment says that only ovl_adaptoer has no device node, so the
> > checking should be
> >
> > if (type != MTK_DISP_OVL_ADAPTOR) {
> > comp_pdev = ...
> > }
> >
> > and later it would return when type = MTK_DISP_OVL_ADAPTOR,
> > so there would be no problem of uninitialized symbol.
>
> That sounds fair, but IIUC what Nancy tries to resolve here is the
> false-positive Smatch warning.
> How about this: given the `if (node)` block was exclusively added for
> ovl_adaptor in [1], plus the init function will immediately return
> after that in this case, it should be safe to do the following
>
> ```
> /* Not all drm components have a DTS device node... */
> if (node == NULL)
> return 0;
>
> comp_pdev = of_find_device_by_node(node);
> ...
>
> if (type == MTK_DISP_AAL ||
> ...
> ```
>
> which is equivalent to adding a `node == NULL` check before [1].
> This should suppress the Smatch warning because `comp_pdev` will be
> (again) unconditionally assigned to something, and the `type ==
> MTK_DISP_OVL_ADAPTOR` line can be dropped also (optional?).
This solution also looks good to me.
Regards,
CK
>
> [1]: commit 0d9eee9118b7 ("drm/mediatek: Add drm ovl_adaptor sub
> driver for MT8195")
>
> Regards,
> Fei
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] drm/mediatek: fix uninitialized symbol
2023-07-20 7:52 ` CK Hu (胡俊光)
@ 2023-08-03 8:32 ` Nancy Lin (林欣螢)
0 siblings, 0 replies; 6+ messages in thread
From: Nancy Lin (林欣螢) @ 2023-08-03 8:32 UTC (permalink / raw)
To: CK Hu (胡俊光), fshao
Cc: linux-kernel, linux-mediatek,
Singo Chang (張興國),
chunkuang.hu, p.zabel, dri-devel,
Project_Global_Chrome_Upstream_Group, linux-arm-kernel,
clang-built-linux, matthias.bgg, angelogioacchino.delregno
Hi CK and Fei,
Thanks for the review.
On Thu, 2023-07-20 at 07:52 +0000, CK Hu (胡俊光) wrote:
> Hi, Fei:
>
> On Mon, 2023-07-17 at 11:59 +0800, Fei Shao wrote:
> >
> > External email : Please do not click links or open attachments
> > until
> > you have verified the sender or the content.
> > Hi CK,
> >
> > On Fri, Jul 14, 2023 at 5:27 PM CK Hu (胡俊光) <ck.hu@mediatek.com>
> > wrote:
> > >
> > > Hi, Nancy:
> > >
> > > On Fri, 2023-04-21 at 10:16 +0800, Nancy.Lin wrote:
> >
> > snip
> > >
> > > In line 566, the statement is
> > >
> > > if (nodo) {
> > > comp_pdev = ...
> > > }
> > >
> > > The comment says that only ovl_adaptoer has no device node, so
> > > the
> > > checking should be
> > >
> > > if (type != MTK_DISP_OVL_ADAPTOR) {
> > > comp_pdev = ...
> > > }
> > >
> > > and later it would return when type = MTK_DISP_OVL_ADAPTOR,
> > > so there would be no problem of uninitialized symbol.
> >
> > That sounds fair, but IIUC what Nancy tries to resolve here is the
> > false-positive Smatch warning.
> > How about this: given the `if (node)` block was exclusively added
> > for
> > ovl_adaptor in [1], plus the init function will immediately return
> > after that in this case, it should be safe to do the following
> >
> > ```
> > /* Not all drm components have a DTS device node... */
> > if (node == NULL)
> > return 0;
> >
> > comp_pdev = of_find_device_by_node(node);
> > ...
> >
> > if (type == MTK_DISP_AAL ||
> > ...
> > ```
> >
> > which is equivalent to adding a `node == NULL` check before [1].
> > This should suppress the Smatch warning because `comp_pdev` will be
> > (again) unconditionally assigned to something, and the `type ==
> > MTK_DISP_OVL_ADAPTOR` line can be dropped also (optional?).
>
> This solution also looks good to me.
>
> Regards,
> CK
>
I will send the next version of modifications based on your
suggestions.
Thanks,
Nancy
> >
> > [1]: commit 0d9eee9118b7 ("drm/mediatek: Add drm ovl_adaptor sub
> > driver for MT8195")
> >
> > Regards,
> > Fei
^ permalink raw reply [flat|nested] 6+ messages in thread