| Message ID | 20230421021609.7730-1-nancy.lin@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/mediatek: fix uninitialized symbol | expand |
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>
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 > >
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;
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
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
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
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;
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(-)