This patchset contains misc improve patches. Mainly to improve safety from
invalid dts input.
Change notes:
v7: Rebase on v6.1-rc1. Change nothing.
v6: https://lore.kernel.org/linux-mediatek/[email protected]/
No change code. just reword the commit message in [3/6] from Angelo.
v5: https://lore.kernel.org/linux-mediatek/[email protected]/
a) Loop from MTK_LARB_NR_MAX in the error path from Angelo.
b) Fix the redundant put_device for the error patch outside the loop from dan.
v4: https://lore.kernel.org/linux-mediatek/[email protected]/
a) Just remove the first patch about dev_err_probe since it was merged.
b) Rebase v6.0-rc1
v3: https://lore.kernel.org/linux-mediatek/[email protected]/
a) Add platform_device_put from Robin.
b) Use component_match_add instead component_match_add_release suggested from Robin.
v2: https://lore.kernel.org/linux-mediatek/[email protected]/
a) Rebase on v5.19-rc1.
b) Add a New patch [5/5] just remove a variable that only is for v1.
v1: https://lore.kernel.org/linux-mediatek/[email protected]/
Base on linux-next-20220510.
the improve safety from dts is base on:
https://lore.kernel.org/linux-mediatek/[email protected]/
Guenter Roeck (1):
iommu/mediatek: Validate number of phandles associated with
"mediatek,larbs"
Yong Wu (5):
iommu/mediatek: Add platform_device_put for recovering the device
refcnt
iommu/mediatek: Use component_match_add
iommu/mediatek: Add error path for loop of mm_dts_parse
iommu/mediatek: Improve safety for mediatek,smi property in larb nodes
iommu/mediatek: Remove unused "mapping" member from mtk_iommu_data
drivers/iommu/mtk_iommu.c | 106 +++++++++++++++++++++++++++-----------
1 file changed, 76 insertions(+), 30 deletions(-)
--
2.18.0
Add platform_device_put to match with of_find_device_by_node.
Meanwhile, I add a new variable "pcommdev" which is for smi common device.
Otherwise, "platform_device_put(plarbdev)" for smi-common dev may be not
readable. And add a checking for whether pcommdev is NULL.
Fixes: d2e9a1102cfc ("iommu/mediatek: Contain MM IOMMU flow with the MM TYPE")
Signed-off-by: Yong Wu <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/iommu/mtk_iommu.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 5a4e00e4bbbc..3189b585725f 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1043,7 +1043,7 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
struct mtk_iommu_data *data)
{
struct device_node *larbnode, *smicomm_node, *smi_subcomm_node;
- struct platform_device *plarbdev;
+ struct platform_device *plarbdev, *pcommdev;
struct device_link *link;
int i, larb_nr, ret;
@@ -1074,12 +1074,14 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
}
if (!plarbdev->dev.driver) {
of_node_put(larbnode);
+ platform_device_put(plarbdev);
return -EPROBE_DEFER;
}
data->larb_imu[id].dev = &plarbdev->dev;
component_match_add_release(dev, match, component_release_of,
component_compare_of, larbnode);
+ platform_device_put(plarbdev);
}
/* Get smi-(sub)-common dev from the last larb. */
@@ -1097,12 +1099,15 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
else
smicomm_node = smi_subcomm_node;
- plarbdev = of_find_device_by_node(smicomm_node);
+ pcommdev = of_find_device_by_node(smicomm_node);
of_node_put(smicomm_node);
- data->smicomm_dev = &plarbdev->dev;
+ if (!pcommdev)
+ return -ENODEV;
+ data->smicomm_dev = &pcommdev->dev;
link = device_link_add(data->smicomm_dev, dev,
DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
+ platform_device_put(pcommdev);
if (!link) {
dev_err(dev, "Unable to link %s.\n", dev_name(data->smicomm_dev));
return -EINVAL;
--
2.18.0
In order to simplify the error patch(avoid call of_node_put), Use
component_match_add instead component_match_add_release since we are only
interested in the "device" here. Then we could always call of_node_put in
normal path.
Strictly this is not a fixes patch, but it is a prepare for adding the
error path, thus I add a Fixes tag too.
Fixes: d2e9a1102cfc ("iommu/mediatek: Contain MM IOMMU flow with the MM TYPE")
Suggested-by: Robin Murphy <[email protected]>
Signed-off-by: Yong Wu <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/iommu/mtk_iommu.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 3189b585725f..38112ad87d70 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1068,19 +1068,17 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
id = i;
plarbdev = of_find_device_by_node(larbnode);
- if (!plarbdev) {
- of_node_put(larbnode);
+ of_node_put(larbnode);
+ if (!plarbdev)
return -ENODEV;
- }
+
if (!plarbdev->dev.driver) {
- of_node_put(larbnode);
platform_device_put(plarbdev);
return -EPROBE_DEFER;
}
data->larb_imu[id].dev = &plarbdev->dev;
- component_match_add_release(dev, match, component_release_of,
- component_compare_of, larbnode);
+ component_match_add(dev, match, component_compare_dev, &plarbdev->dev);
platform_device_put(plarbdev);
}
--
2.18.0
The mtk_iommu_mm_dts_parse will parse the smi larbs nodes. if the i+1
larb is parsed fail, we should put_device for the i..0 larbs.
There are two places need to comment:
1) The larbid may be not linear mapping, we should loop whole
the array in the error path.
2) I move this line position: "data->larb_imu[id].dev = &plarbdev->dev;"
before "if (!plarbdev->dev.driver)", That means set
data->larb_imu[id].dev before the error path. then we don't need
"platform_device_put(plarbdev)" again in probe_defer case. All depend
on "put_device" of the error path in error cases.
Fixes: d2e9a1102cfc ("iommu/mediatek: Contain MM IOMMU flow with the MM TYPE")
Signed-off-by: Yong Wu <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/iommu/mtk_iommu.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 38112ad87d70..912322494bc0 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1055,8 +1055,10 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
u32 id;
larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
- if (!larbnode)
- return -EINVAL;
+ if (!larbnode) {
+ ret = -EINVAL;
+ goto err_larbdev_put;
+ }
if (!of_device_is_available(larbnode)) {
of_node_put(larbnode);
@@ -1069,14 +1071,16 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
plarbdev = of_find_device_by_node(larbnode);
of_node_put(larbnode);
- if (!plarbdev)
- return -ENODEV;
+ if (!plarbdev) {
+ ret = -ENODEV;
+ goto err_larbdev_put;
+ }
+ data->larb_imu[id].dev = &plarbdev->dev;
if (!plarbdev->dev.driver) {
- platform_device_put(plarbdev);
- return -EPROBE_DEFER;
+ ret = -EPROBE_DEFER;
+ goto err_larbdev_put;
}
- data->larb_imu[id].dev = &plarbdev->dev;
component_match_add(dev, match, component_compare_dev, &plarbdev->dev);
platform_device_put(plarbdev);
@@ -1111,6 +1115,15 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
return -EINVAL;
}
return 0;
+
+err_larbdev_put:
+ /* id may be not linear mapping, loop whole the array */
+ for (i = MTK_LARB_NR_MAX - 1; i >= 0; i++) {
+ if (!data->larb_imu[i].dev)
+ continue;
+ put_device(data->larb_imu[i].dev);
+ }
+ return ret;
}
static int mtk_iommu_probe(struct platform_device *pdev)
--
2.18.0
Just remove a unused variable that only is for mtk_iommu_v1.
Signed-off-by: Yong Wu <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Reviewed-by: Matthias Brugger <[email protected]>
---
drivers/iommu/mtk_iommu.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index f7ac102e343f..cb8df26ec71e 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -223,10 +223,7 @@ struct mtk_iommu_data {
struct device *smicomm_dev;
struct mtk_iommu_bank_data *bank;
-
- struct dma_iommu_mapping *mapping; /* For mtk_iommu_v1.c */
struct regmap *pericfg;
-
struct mutex mutex; /* Protect m4u_group/m4u_dom above */
/*
--
2.18.0
From: Guenter Roeck <[email protected]>
Fix the smatch warnings:
drivers/iommu/mtk_iommu.c:878 mtk_iommu_mm_dts_parse() error: uninitialized
symbol 'larbnode'.
If someone abuse the dtsi node(Don't follow the definition of dt-binding),
for example "mediatek,larbs" is provided as boolean property, "larb_nr"
will be zero and cause abnormal.
To fix this problem and improve the code safety, add some checking
for the invalid input from dtsi, e.g. checking the larb_nr/larbid valid
range, and avoid "mediatek,larb-id" property conflicts in the smi-larb
nodes.
Fixes: d2e9a1102cfc ("iommu/mediatek: Contain MM IOMMU flow with the MM TYPE")
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
Signed-off-by: Yong Wu <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/iommu/mtk_iommu.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 912322494bc0..9cbff48f03c0 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1050,6 +1050,8 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
larb_nr = of_count_phandle_with_args(dev->of_node, "mediatek,larbs", NULL);
if (larb_nr < 0)
return larb_nr;
+ if (larb_nr == 0 || larb_nr > MTK_LARB_NR_MAX)
+ return -EINVAL;
for (i = 0; i < larb_nr; i++) {
u32 id;
@@ -1068,6 +1070,11 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
ret = of_property_read_u32(larbnode, "mediatek,larb-id", &id);
if (ret)/* The id is consecutive if there is no this property */
id = i;
+ if (id >= MTK_LARB_NR_MAX) {
+ of_node_put(larbnode);
+ ret = -EINVAL;
+ goto err_larbdev_put;
+ }
plarbdev = of_find_device_by_node(larbnode);
of_node_put(larbnode);
@@ -1075,6 +1082,11 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
ret = -ENODEV;
goto err_larbdev_put;
}
+ if (data->larb_imu[id].dev) {
+ platform_device_put(plarbdev);
+ ret = -EEXIST;
+ goto err_larbdev_put;
+ }
data->larb_imu[id].dev = &plarbdev->dev;
if (!plarbdev->dev.driver) {
--
2.18.0
No functional change. Just improve safety from dts.
All the larbs that connect to one IOMMU must connect with the same
smi-common. This patch checks all the mediatek,smi property for each
larb, If their mediatek,smi are different, it will return fails.
Also avoid there is no available smi-larb nodes.
Suggested-by: Guenter Roeck <[email protected]>
Signed-off-by: Yong Wu <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/iommu/mtk_iommu.c | 53 +++++++++++++++++++++++++++------------
1 file changed, 37 insertions(+), 16 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 9cbff48f03c0..f7ac102e343f 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1042,7 +1042,7 @@ static const struct component_master_ops mtk_iommu_com_ops = {
static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **match,
struct mtk_iommu_data *data)
{
- struct device_node *larbnode, *smicomm_node, *smi_subcomm_node;
+ struct device_node *larbnode, *frst_avail_smicomm_node = NULL;
struct platform_device *plarbdev, *pcommdev;
struct device_link *link;
int i, larb_nr, ret;
@@ -1054,6 +1054,7 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
return -EINVAL;
for (i = 0; i < larb_nr; i++) {
+ struct device_node *smicomm_node, *smi_subcomm_node;
u32 id;
larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
@@ -1094,27 +1095,47 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
goto err_larbdev_put;
}
+ /* Get smi-(sub)-common dev from the last larb. */
+ smi_subcomm_node = of_parse_phandle(larbnode, "mediatek,smi", 0);
+ if (!smi_subcomm_node) {
+ ret = -EINVAL;
+ goto err_larbdev_put;
+ }
+
+ /*
+ * It may have two level smi-common. the node is smi-sub-common if it
+ * has a new mediatek,smi property. otherwise it is smi-commmon.
+ */
+ smicomm_node = of_parse_phandle(smi_subcomm_node, "mediatek,smi", 0);
+ if (smicomm_node)
+ of_node_put(smi_subcomm_node);
+ else
+ smicomm_node = smi_subcomm_node;
+
+ /*
+ * All the larbs that connect to one IOMMU must connect with the same
+ * smi-common.
+ */
+ if (!frst_avail_smicomm_node) {
+ frst_avail_smicomm_node = smicomm_node;
+ } else if (frst_avail_smicomm_node != smicomm_node) {
+ dev_err(dev, "mediatek,smi property is not right @larb%d.", id);
+ of_node_put(smicomm_node);
+ ret = -EINVAL;
+ goto err_larbdev_put;
+ } else {
+ of_node_put(smicomm_node);
+ }
+
component_match_add(dev, match, component_compare_dev, &plarbdev->dev);
platform_device_put(plarbdev);
}
- /* Get smi-(sub)-common dev from the last larb. */
- smi_subcomm_node = of_parse_phandle(larbnode, "mediatek,smi", 0);
- if (!smi_subcomm_node)
+ if (!frst_avail_smicomm_node)
return -EINVAL;
- /*
- * It may have two level smi-common. the node is smi-sub-common if it
- * has a new mediatek,smi property. otherwise it is smi-commmon.
- */
- smicomm_node = of_parse_phandle(smi_subcomm_node, "mediatek,smi", 0);
- if (smicomm_node)
- of_node_put(smi_subcomm_node);
- else
- smicomm_node = smi_subcomm_node;
-
- pcommdev = of_find_device_by_node(smicomm_node);
- of_node_put(smicomm_node);
+ pcommdev = of_find_device_by_node(frst_avail_smicomm_node);
+ of_node_put(frst_avail_smicomm_node);
if (!pcommdev)
return -ENODEV;
data->smicomm_dev = &pcommdev->dev;
--
2.18.0
On Tue, 2022-10-18 at 10:42 +0800, Yong Wu wrote:
> This patchset contains misc improve patches. Mainly to improve safety
> from
> invalid dts input.
>
> Change notes:
> v7: Rebase on v6.1-rc1. Change nothing.
Hi Joerg,
Gentle ping for this. Could you help review or apply this if it is ok
for you?
Thanks,
Yong
Hi Wu,
On Tue, Nov 22, 2022 at 01:13:01AM +0000, Yong Wu (吴勇) wrote:
> Gentle ping for this. Could you help review or apply this if it is ok
> for you?
I am waiting for Matthias' OK on this.
Regards,
Joerg
On 18/10/2022 04:42, Yong Wu wrote:
> In order to simplify the error patch(avoid call of_node_put), Use
> component_match_add instead component_match_add_release since we are only
> interested in the "device" here. Then we could always call of_node_put in
> normal path.
>
> Strictly this is not a fixes patch, but it is a prepare for adding the
> error path, thus I add a Fixes tag too.
>
> Fixes: d2e9a1102cfc ("iommu/mediatek: Contain MM IOMMU flow with the MM TYPE")
> Suggested-by: Robin Murphy <[email protected]>
> Signed-off-by: Yong Wu <[email protected]>
> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Reviewed-by: Matthias Brugger <[email protected]>
> ---
> drivers/iommu/mtk_iommu.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 3189b585725f..38112ad87d70 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -1068,19 +1068,17 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
> id = i;
>
> plarbdev = of_find_device_by_node(larbnode);
> - if (!plarbdev) {
> - of_node_put(larbnode);
> + of_node_put(larbnode);
> + if (!plarbdev)
> return -ENODEV;
> - }
> +
> if (!plarbdev->dev.driver) {
> - of_node_put(larbnode);
> platform_device_put(plarbdev);
> return -EPROBE_DEFER;
> }
> data->larb_imu[id].dev = &plarbdev->dev;
>
> - component_match_add_release(dev, match, component_release_of,
> - component_compare_of, larbnode);
> + component_match_add(dev, match, component_compare_dev, &plarbdev->dev);
> platform_device_put(plarbdev);
> }
>
On 18/10/2022 04:42, Yong Wu wrote:
> The mtk_iommu_mm_dts_parse will parse the smi larbs nodes. if the i+1
> larb is parsed fail, we should put_device for the i..0 larbs.
>
> There are two places need to comment:
> 1) The larbid may be not linear mapping, we should loop whole
> the array in the error path.
> 2) I move this line position: "data->larb_imu[id].dev = &plarbdev->dev;"
> before "if (!plarbdev->dev.driver)", That means set
> data->larb_imu[id].dev before the error path. then we don't need
> "platform_device_put(plarbdev)" again in probe_defer case. All depend
> on "put_device" of the error path in error cases.
>
> Fixes: d2e9a1102cfc ("iommu/mediatek: Contain MM IOMMU flow with the MM TYPE")
> Signed-off-by: Yong Wu <[email protected]>
> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Reviewed-by: Matthias Brugger <[email protected]>
> ---
> drivers/iommu/mtk_iommu.c | 27 ++++++++++++++++++++-------
> 1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 38112ad87d70..912322494bc0 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -1055,8 +1055,10 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
> u32 id;
>
> larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
> - if (!larbnode)
> - return -EINVAL;
> + if (!larbnode) {
> + ret = -EINVAL;
> + goto err_larbdev_put;
> + }
>
> if (!of_device_is_available(larbnode)) {
> of_node_put(larbnode);
> @@ -1069,14 +1071,16 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
>
> plarbdev = of_find_device_by_node(larbnode);
> of_node_put(larbnode);
> - if (!plarbdev)
> - return -ENODEV;
> + if (!plarbdev) {
> + ret = -ENODEV;
> + goto err_larbdev_put;
> + }
> + data->larb_imu[id].dev = &plarbdev->dev;
>
> if (!plarbdev->dev.driver) {
> - platform_device_put(plarbdev);
> - return -EPROBE_DEFER;
> + ret = -EPROBE_DEFER;
> + goto err_larbdev_put;
> }
> - data->larb_imu[id].dev = &plarbdev->dev;
>
> component_match_add(dev, match, component_compare_dev, &plarbdev->dev);
> platform_device_put(plarbdev);
> @@ -1111,6 +1115,15 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
> return -EINVAL;
> }
> return 0;
> +
> +err_larbdev_put:
> + /* id may be not linear mapping, loop whole the array */
> + for (i = MTK_LARB_NR_MAX - 1; i >= 0; i++) {
> + if (!data->larb_imu[i].dev)
> + continue;
> + put_device(data->larb_imu[i].dev);
> + }
> + return ret;
> }
>
> static int mtk_iommu_probe(struct platform_device *pdev)
On 18/10/2022 04:42, Yong Wu wrote:
> No functional change. Just improve safety from dts.
>
> All the larbs that connect to one IOMMU must connect with the same
> smi-common. This patch checks all the mediatek,smi property for each
> larb, If their mediatek,smi are different, it will return fails.
> Also avoid there is no available smi-larb nodes.
>
> Suggested-by: Guenter Roeck <[email protected]>
> Signed-off-by: Yong Wu <[email protected]>
> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Reviewed-by: Matthias Brugger <[email protected]>
> ---
> drivers/iommu/mtk_iommu.c | 53 +++++++++++++++++++++++++++------------
> 1 file changed, 37 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 9cbff48f03c0..f7ac102e343f 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -1042,7 +1042,7 @@ static const struct component_master_ops mtk_iommu_com_ops = {
> static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **match,
> struct mtk_iommu_data *data)
> {
> - struct device_node *larbnode, *smicomm_node, *smi_subcomm_node;
> + struct device_node *larbnode, *frst_avail_smicomm_node = NULL;
> struct platform_device *plarbdev, *pcommdev;
> struct device_link *link;
> int i, larb_nr, ret;
> @@ -1054,6 +1054,7 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
> return -EINVAL;
>
> for (i = 0; i < larb_nr; i++) {
> + struct device_node *smicomm_node, *smi_subcomm_node;
> u32 id;
>
> larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
> @@ -1094,27 +1095,47 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
> goto err_larbdev_put;
> }
>
> + /* Get smi-(sub)-common dev from the last larb. */
> + smi_subcomm_node = of_parse_phandle(larbnode, "mediatek,smi", 0);
> + if (!smi_subcomm_node) {
> + ret = -EINVAL;
> + goto err_larbdev_put;
> + }
> +
> + /*
> + * It may have two level smi-common. the node is smi-sub-common if it
> + * has a new mediatek,smi property. otherwise it is smi-commmon.
> + */
> + smicomm_node = of_parse_phandle(smi_subcomm_node, "mediatek,smi", 0);
> + if (smicomm_node)
> + of_node_put(smi_subcomm_node);
> + else
> + smicomm_node = smi_subcomm_node;
> +
> + /*
> + * All the larbs that connect to one IOMMU must connect with the same
> + * smi-common.
> + */
> + if (!frst_avail_smicomm_node) {
> + frst_avail_smicomm_node = smicomm_node;
> + } else if (frst_avail_smicomm_node != smicomm_node) {
> + dev_err(dev, "mediatek,smi property is not right @larb%d.", id);
> + of_node_put(smicomm_node);
> + ret = -EINVAL;
> + goto err_larbdev_put;
> + } else {
> + of_node_put(smicomm_node);
> + }
> +
> component_match_add(dev, match, component_compare_dev, &plarbdev->dev);
> platform_device_put(plarbdev);
> }
>
> - /* Get smi-(sub)-common dev from the last larb. */
> - smi_subcomm_node = of_parse_phandle(larbnode, "mediatek,smi", 0);
> - if (!smi_subcomm_node)
> + if (!frst_avail_smicomm_node)
> return -EINVAL;
>
> - /*
> - * It may have two level smi-common. the node is smi-sub-common if it
> - * has a new mediatek,smi property. otherwise it is smi-commmon.
> - */
> - smicomm_node = of_parse_phandle(smi_subcomm_node, "mediatek,smi", 0);
> - if (smicomm_node)
> - of_node_put(smi_subcomm_node);
> - else
> - smicomm_node = smi_subcomm_node;
> -
> - pcommdev = of_find_device_by_node(smicomm_node);
> - of_node_put(smicomm_node);
> + pcommdev = of_find_device_by_node(frst_avail_smicomm_node);
> + of_node_put(frst_avail_smicomm_node);
> if (!pcommdev)
> return -ENODEV;
> data->smicomm_dev = &pcommdev->dev;
On 18/10/2022 04:42, Yong Wu wrote:
> Add platform_device_put to match with of_find_device_by_node.
>
> Meanwhile, I add a new variable "pcommdev" which is for smi common device.
> Otherwise, "platform_device_put(plarbdev)" for smi-common dev may be not
> readable. And add a checking for whether pcommdev is NULL.
>
> Fixes: d2e9a1102cfc ("iommu/mediatek: Contain MM IOMMU flow with the MM TYPE")
> Signed-off-by: Yong Wu <[email protected]>
> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Reviewed-by: Matthias Brugger <[email protected]>
> ---
> drivers/iommu/mtk_iommu.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 5a4e00e4bbbc..3189b585725f 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -1043,7 +1043,7 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
> struct mtk_iommu_data *data)
> {
> struct device_node *larbnode, *smicomm_node, *smi_subcomm_node;
> - struct platform_device *plarbdev;
> + struct platform_device *plarbdev, *pcommdev;
> struct device_link *link;
> int i, larb_nr, ret;
>
> @@ -1074,12 +1074,14 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
> }
> if (!plarbdev->dev.driver) {
> of_node_put(larbnode);
> + platform_device_put(plarbdev);
> return -EPROBE_DEFER;
> }
> data->larb_imu[id].dev = &plarbdev->dev;
>
> component_match_add_release(dev, match, component_release_of,
> component_compare_of, larbnode);
> + platform_device_put(plarbdev);
> }
>
> /* Get smi-(sub)-common dev from the last larb. */
> @@ -1097,12 +1099,15 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
> else
> smicomm_node = smi_subcomm_node;
>
> - plarbdev = of_find_device_by_node(smicomm_node);
> + pcommdev = of_find_device_by_node(smicomm_node);
> of_node_put(smicomm_node);
> - data->smicomm_dev = &plarbdev->dev;
> + if (!pcommdev)
> + return -ENODEV;
> + data->smicomm_dev = &pcommdev->dev;
>
> link = device_link_add(data->smicomm_dev, dev,
> DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
> + platform_device_put(pcommdev);
> if (!link) {
> dev_err(dev, "Unable to link %s.\n", dev_name(data->smicomm_dev));
> return -EINVAL;
On 18/10/2022 04:42, Yong Wu wrote:
> From: Guenter Roeck <[email protected]>
>
> Fix the smatch warnings:
> drivers/iommu/mtk_iommu.c:878 mtk_iommu_mm_dts_parse() error: uninitialized
> symbol 'larbnode'.
>
> If someone abuse the dtsi node(Don't follow the definition of dt-binding),
> for example "mediatek,larbs" is provided as boolean property, "larb_nr"
> will be zero and cause abnormal.
>
> To fix this problem and improve the code safety, add some checking
> for the invalid input from dtsi, e.g. checking the larb_nr/larbid valid
> range, and avoid "mediatek,larb-id" property conflicts in the smi-larb
> nodes.
>
> Fixes: d2e9a1102cfc ("iommu/mediatek: Contain MM IOMMU flow with the MM TYPE")
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> Signed-off-by: Yong Wu <[email protected]>
> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Reviewed-by: Matthias Brugger <[email protected]>
> ---
> drivers/iommu/mtk_iommu.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 912322494bc0..9cbff48f03c0 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -1050,6 +1050,8 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
> larb_nr = of_count_phandle_with_args(dev->of_node, "mediatek,larbs", NULL);
> if (larb_nr < 0)
> return larb_nr;
> + if (larb_nr == 0 || larb_nr > MTK_LARB_NR_MAX)
> + return -EINVAL;
>
> for (i = 0; i < larb_nr; i++) {
> u32 id;
> @@ -1068,6 +1070,11 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
> ret = of_property_read_u32(larbnode, "mediatek,larb-id", &id);
> if (ret)/* The id is consecutive if there is no this property */
> id = i;
> + if (id >= MTK_LARB_NR_MAX) {
> + of_node_put(larbnode);
> + ret = -EINVAL;
> + goto err_larbdev_put;
> + }
>
> plarbdev = of_find_device_by_node(larbnode);
> of_node_put(larbnode);
> @@ -1075,6 +1082,11 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
> ret = -ENODEV;
> goto err_larbdev_put;
> }
> + if (data->larb_imu[id].dev) {
> + platform_device_put(plarbdev);
> + ret = -EEXIST;
> + goto err_larbdev_put;
> + }
> data->larb_imu[id].dev = &plarbdev->dev;
>
> if (!plarbdev->dev.driver) {
On Tue, Oct 18, 2022 at 10:42:52AM +0800, Yong Wu wrote:
> Guenter Roeck (1):
> iommu/mediatek: Validate number of phandles associated with
> "mediatek,larbs"
>
> Yong Wu (5):
> iommu/mediatek: Add platform_device_put for recovering the device
> refcnt
> iommu/mediatek: Use component_match_add
> iommu/mediatek: Add error path for loop of mm_dts_parse
> iommu/mediatek: Improve safety for mediatek,smi property in larb nodes
> iommu/mediatek: Remove unused "mapping" member from mtk_iommu_data
Applied, thanks.