This patchset contains misc improve patches. Mainly to improve safety from
invalid dts input.
Change notes:
v4: 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 | 115 +++++++++++++++++++++++++++-----------
1 file changed, 83 insertions(+), 32 deletions(-)
--
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]>
---
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 f2560ce72b6c..9c5902207bef 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1066,19 +1066,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
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]>
---
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 7e363b1f24df..f2560ce72b6c 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1041,7 +1041,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;
@@ -1072,12 +1072,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. */
@@ -1095,12 +1097,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 -EINVAL;
+ 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
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 f63d4210043d..21195ac060f1 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1048,6 +1048,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;
@@ -1066,6 +1068,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);
@@ -1073,6 +1080,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
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 ea387c762509..b06c91f5a9b2 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -221,10 +221,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
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]>
---
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 21195ac060f1..ea387c762509 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1040,7 +1040,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;
@@ -1052,6 +1052,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);
@@ -1092,29 +1093,49 @@ 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) {
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;
-
- 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) {
ret = -EINVAL;
goto err_larbdev_put;
--
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 0..i 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;"
That means set data->larb_imu[id].dev before the error path.
then we don't need "platform_device_put(plarbdev)" again while
probe_defer case. All depend on "put_device" in 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]>
---
drivers/iommu/mtk_iommu.c | 42 ++++++++++++++++++++++++++++-----------
1 file changed, 30 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 9c5902207bef..f63d4210043d 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1053,8 +1053,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);
@@ -1067,14 +1069,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);
@@ -1082,8 +1086,10 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
/* Get smi-(sub)-common dev from the last larb. */
smi_subcomm_node = of_parse_phandle(larbnode, "mediatek,smi", 0);
- if (!smi_subcomm_node)
- return -EINVAL;
+ 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
@@ -1097,8 +1103,10 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
pcommdev = of_find_device_by_node(smicomm_node);
of_node_put(smicomm_node);
- if (!pcommdev)
- return -EINVAL;
+ if (!pcommdev) {
+ ret = -EINVAL;
+ goto err_larbdev_put;
+ }
data->smicomm_dev = &pcommdev->dev;
link = device_link_add(data->smicomm_dev, dev,
@@ -1106,9 +1114,19 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
platform_device_put(pcommdev);
if (!link) {
dev_err(dev, "Unable to link %s.\n", dev_name(data->smicomm_dev));
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_larbdev_put;
}
return 0;
+
+err_larbdev_put:
+ /* id may be not linear mapping, loop whole the array */
+ for (i = 0; i < MTK_LARB_NR_MAX; 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
On Wed, Aug 24, 2022 at 02:43:03PM +0800, 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 0..i 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;"
> That means set data->larb_imu[id].dev before the error path.
> then we don't need "platform_device_put(plarbdev)" again while
> probe_defer case. All depend on "put_device" in the error path in error
> cases.
I don't understand what you're saying here. There is still a
platform_device_put(plarbdev) on the success path after
component_match_add().
So if we fail when i == 2 then we do:
put_device(data->larb_imu[2].dev);
But for the previous iterations has both platform_device_put()
and put_device() called for them.
regards,
dan carpenter
Il 24/08/22 08:43, Yong Wu ha scritto:
> 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]>
Il 24/08/22 08:43, Yong Wu ha scritto:
> 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]>
Il 24/08/22 08:43, Yong Wu ha scritto:
> 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 0..i 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;"
> That means set data->larb_imu[id].dev before the error path.
> then we don't need "platform_device_put(plarbdev)" again while
> probe_defer case. All depend on "put_device" in 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]>
> ---
> drivers/iommu/mtk_iommu.c | 42 ++++++++++++++++++++++++++++-----------
> 1 file changed, 30 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 9c5902207bef..f63d4210043d 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -1053,8 +1053,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);
> @@ -1067,14 +1069,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);
> @@ -1082,8 +1086,10 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
>
> /* Get smi-(sub)-common dev from the last larb. */
> smi_subcomm_node = of_parse_phandle(larbnode, "mediatek,smi", 0);
> - if (!smi_subcomm_node)
> - return -EINVAL;
> + 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
> @@ -1097,8 +1103,10 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
>
> pcommdev = of_find_device_by_node(smicomm_node);
> of_node_put(smicomm_node);
> - if (!pcommdev)
> - return -EINVAL;
> + if (!pcommdev) {
> + ret = -EINVAL;
> + goto err_larbdev_put;
> + }
> data->smicomm_dev = &pcommdev->dev;
>
> link = device_link_add(data->smicomm_dev, dev,
> @@ -1106,9 +1114,19 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
> platform_device_put(pcommdev);
> if (!link) {
> dev_err(dev, "Unable to link %s.\n", dev_name(data->smicomm_dev));
> - return -EINVAL;
> + ret = -EINVAL;
> + goto err_larbdev_put;
> }
> return 0;
> +
> +err_larbdev_put:
> + /* id may be not linear mapping, loop whole the array */
> + for (i = 0; i < MTK_LARB_NR_MAX; i++) {
Since there may be a case in which the mapping is linear and we're doing teardown,
I think it would be sensible to loop the other way around instead, from
MTK_LARB_NR_MAX to 0.
Everything else looks good to me.
Cheers,
Angelo
> + 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 Tue, 2022-08-30 at 10:14 +0200, AngeloGioacchino Del Regno wrote:
> Il 24/08/22 08:43, Yong Wu ha scritto:
> > 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 0..i 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;"
> > That means set data->larb_imu[id].dev before the error path.
> > then we don't need "platform_device_put(plarbdev)" again while
> > probe_defer case. All depend on "put_device" in 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]>
> > ---
> > drivers/iommu/mtk_iommu.c | 42 ++++++++++++++++++++++++++++----
> > -------
> > 1 file changed, 30 insertions(+), 12 deletions(-)
[...]
> > +
> > +err_larbdev_put:
> > + /* id may be not linear mapping, loop whole the array */
> > + for (i = 0; i < MTK_LARB_NR_MAX; i++) {
>
> Since there may be a case in which the mapping is linear and we're
> doing teardown,
> I think it would be sensible to loop the other way around instead,
> from
> MTK_LARB_NR_MAX to 0.
Thanks very much. I will change from MTK_LARB_NR_MAX to 0.
>
> Everything else looks good to me.
>
> Cheers,
> Angelo
>
> > + 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)
>
>
Hi Dan,
Thanks very much for your review:)
On Tue, 2022-08-30 at 11:32 +0300, Dan Carpenter wrote:
> On Wed, Aug 24, 2022 at 02:43:03PM +0800, 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 0..i 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;"
> > That means set data->larb_imu[id].dev before the error path.
> > then we don't need "platform_device_put(plarbdev)" again while
> > probe_defer case. All depend on "put_device" in the error path
> > in error
> > cases.
>
> I don't understand what you're saying here. There is still a
> platform_device_put(plarbdev) on the success path after
> component_match_add().
>
> So if we fail when i == 2 then we do:
>
> put_device(data->larb_imu[2].dev);
>
> But for the previous iterations has both platform_device_put()
> and put_device() called for them.
Sorry for this. Right! For the goto outside the loop, it did put twice.
I will fix this.
>
> regards,
> dan carpenter
>