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 <[email protected]>
> ---
> 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 (胡俊光) <[email protected]> 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 (胡俊光) <[email protected]>
> 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 (胡俊光) <[email protected]>
> > 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