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 <yong.wu@mediatek.com>
---
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
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 >
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 <yong.wu@mediatek.com> > --- > 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 <yong.wu@mediatek.com> > > --- > > 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) > >
© 2016 - 2024 Red Hat, Inc.