[PATCH v2] drm/mediatek: fix uninitialized symbol

[PATCH v2] drm/mediatek: fix uninitialized symbol

[Nancy.Lin]

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;
+
 	priv = devm_kzalloc(comp->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
-- 
2.18.0

[PATCH v2] drm/mediatek: fix uninitialized symbol

[Nancy.Lin]

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;
+
 	priv = devm_kzalloc(comp->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
-- 
2.18.0

[PATCH v2] drm/mediatek: fix uninitialized symbol

[Nancy.Lin]

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;
+
 	priv = devm_kzalloc(comp->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[AngeloGioacchino Del Regno]

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>

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[AngeloGioacchino Del Regno]

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>

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[AngeloGioacchino Del Regno]

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>



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[Fei Shao]

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
>
>

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[Fei Shao]

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
>
>

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[Fei Shao]

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
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[CK Hu (胡俊光)]

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;

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[CK Hu (胡俊光)]

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;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[CK Hu (胡俊光)]

[-- Attachment #1: Type: text/html, Size: 3620 bytes --]

[-- Attachment #2: Type: text/plain, Size: 1787 bytes --]

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;

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[Fei Shao]

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

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[Fei Shao]

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

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[Fei Shao]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[CK Hu (胡俊光)]

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

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[CK Hu (胡俊光)]

[-- Attachment #1: Type: text/html, Size: 4207 bytes --]

[-- Attachment #2: Type: text/plain, Size: 1771 bytes --]

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

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[CK Hu (胡俊光)]

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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[Nancy Lin (林欣螢)]

[-- Attachment #1: Type: text/html, Size: 5225 bytes --]

[-- Attachment #2: Type: text/plain, Size: 2133 bytes --]

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

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[Nancy Lin (林欣螢)]

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

Re: [PATCH v2] drm/mediatek: fix uninitialized symbol

[Nancy Lin (林欣螢)]

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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel