* [PATCH v2 1/5] iommu/mediatek: Use dev_err_probe to mute probe_defer err log
2022-06-16 5:41 [PATCH v2 0/5] iommu/mediatek: Improve safety from dts Yong Wu
@ 2022-06-16 5:41 ` Yong Wu
2022-06-16 13:28 ` Matthias Brugger
2022-06-16 5:42 ` [PATCH v2 2/5] iommu/mediatek: Add error path for loop of mm_dts_parse Yong Wu
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Yong Wu @ 2022-06-16 5:41 UTC (permalink / raw)
To: Joerg Roedel, Matthias Brugger, Will Deacon
Cc: iommu, linux-mediatek, linux-arm-kernel, linux-kernel, Yong Wu,
AngeloGioacchino Del Regno, mingyuan.ma, yf.wang, libo.kang,
chengci.xu, youlin.pei, anan.sun, xueqi.zhang, Guenter Roeck,
Dan Carpenter
Mute the probe defer log:
[ 2.654806] mtk-iommu 14018000.iommu: mm dts parse fail(-517).
[ 2.656168] mtk-iommu 1c01f000.iommu: mm dts parse fail(-517).
Fixes: d2e9a1102cfc ("iommu/mediatek: Contain MM IOMMU flow with the MM TYPE")
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
---
drivers/iommu/mtk_iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index bb9dd92c9898..3b2489e8a6dd 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1204,7 +1204,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM)) {
ret = mtk_iommu_mm_dts_parse(dev, &match, data);
if (ret) {
- dev_err(dev, "mm dts parse fail(%d).", ret);
+ dev_err_probe(dev, ret, "mm dts parse fail.");
goto out_runtime_disable;
}
} else if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_INFRA) &&
--
2.18.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/5] iommu/mediatek: Use dev_err_probe to mute probe_defer err log
2022-06-16 5:41 ` [PATCH v2 1/5] iommu/mediatek: Use dev_err_probe to mute probe_defer err log Yong Wu
@ 2022-06-16 13:28 ` Matthias Brugger
0 siblings, 0 replies; 16+ messages in thread
From: Matthias Brugger @ 2022-06-16 13:28 UTC (permalink / raw)
To: Yong Wu, Joerg Roedel, Will Deacon
Cc: iommu, linux-mediatek, linux-arm-kernel, linux-kernel,
AngeloGioacchino Del Regno, mingyuan.ma, yf.wang, libo.kang,
chengci.xu, youlin.pei, anan.sun, xueqi.zhang, Guenter Roeck,
Dan Carpenter
On 16/06/2022 07:41, Yong Wu wrote:
> Mute the probe defer log:
>
> [ 2.654806] mtk-iommu 14018000.iommu: mm dts parse fail(-517).
> [ 2.656168] mtk-iommu 1c01f000.iommu: mm dts parse fail(-517).
>
> Fixes: d2e9a1102cfc ("iommu/mediatek: Contain MM IOMMU flow with the MM TYPE")
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Reviewed-by: Guenter Roeck <groeck@chromium.org>
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> ---
> drivers/iommu/mtk_iommu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index bb9dd92c9898..3b2489e8a6dd 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -1204,7 +1204,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM)) {
> ret = mtk_iommu_mm_dts_parse(dev, &match, data);
> if (ret) {
> - dev_err(dev, "mm dts parse fail(%d).", ret);
> + dev_err_probe(dev, ret, "mm dts parse fail.");
> goto out_runtime_disable;
> }
> } else if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_INFRA) &&
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/5] iommu/mediatek: Add error path for loop of mm_dts_parse
2022-06-16 5:41 [PATCH v2 0/5] iommu/mediatek: Improve safety from dts Yong Wu
2022-06-16 5:41 ` [PATCH v2 1/5] iommu/mediatek: Use dev_err_probe to mute probe_defer err log Yong Wu
@ 2022-06-16 5:42 ` Yong Wu
2022-06-16 8:59 ` Robin Murphy
2022-06-16 13:49 ` Matthias Brugger
2022-06-16 5:42 ` [PATCH v2 3/5] iommu/mediatek: Validate number of phandles associated with "mediatek, larbs" Yong Wu
` (2 subsequent siblings)
4 siblings, 2 replies; 16+ messages in thread
From: Yong Wu @ 2022-06-16 5:42 UTC (permalink / raw)
To: Joerg Roedel, Matthias Brugger, Will Deacon
Cc: iommu, linux-mediatek, linux-arm-kernel, linux-kernel, Yong Wu,
AngeloGioacchino Del Regno, mingyuan.ma, yf.wang, libo.kang,
chengci.xu, youlin.pei, anan.sun, xueqi.zhang, Guenter Roeck,
Dan Carpenter
The mtk_iommu_mm_dts_parse will parse the smi larbs nodes. if the i+1
larb is parsed fail(return -EINVAL), we should of_node_put for the 0..i
larbs. In the fail path, one of_node_put matches with of_parse_phandle in
it.
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 | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 3b2489e8a6dd..ab24078938bf 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1071,12 +1071,12 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
plarbdev = of_find_device_by_node(larbnode);
if (!plarbdev) {
- of_node_put(larbnode);
- return -ENODEV;
+ ret = -ENODEV;
+ goto err_larbnode_put;
}
if (!plarbdev->dev.driver) {
- of_node_put(larbnode);
- return -EPROBE_DEFER;
+ ret = -EPROBE_DEFER;
+ goto err_larbnode_put;
}
data->larb_imu[id].dev = &plarbdev->dev;
@@ -1107,9 +1107,20 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
if (!link) {
dev_err(dev, "Unable to link %s.\n", dev_name(data->smicomm_dev));
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_larbnode_put;
}
return 0;
+
+err_larbnode_put:
+ while (i--) {
+ larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
+ if (larbnode && of_device_is_available(larbnode)) {
+ of_node_put(larbnode);
+ of_node_put(larbnode);
+ }
+ }
+ return ret;
}
static int mtk_iommu_probe(struct platform_device *pdev)
--
2.18.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/5] iommu/mediatek: Add error path for loop of mm_dts_parse
2022-06-16 5:42 ` [PATCH v2 2/5] iommu/mediatek: Add error path for loop of mm_dts_parse Yong Wu
@ 2022-06-16 8:59 ` Robin Murphy
2022-06-16 10:08 ` Yong Wu
2022-06-16 13:49 ` Matthias Brugger
1 sibling, 1 reply; 16+ messages in thread
From: Robin Murphy @ 2022-06-16 8:59 UTC (permalink / raw)
To: Yong Wu, Joerg Roedel, Matthias Brugger, Will Deacon
Cc: iommu, linux-mediatek, linux-arm-kernel, linux-kernel,
AngeloGioacchino Del Regno, mingyuan.ma, yf.wang, libo.kang,
chengci.xu, youlin.pei, anan.sun, xueqi.zhang, Guenter Roeck,
Dan Carpenter
On 2022-06-16 06:42, Yong Wu wrote:
> The mtk_iommu_mm_dts_parse will parse the smi larbs nodes. if the i+1
> larb is parsed fail(return -EINVAL), we should of_node_put for the 0..i
> larbs. In the fail path, one of_node_put matches with of_parse_phandle in
> it.
>
> 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 | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 3b2489e8a6dd..ab24078938bf 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -1071,12 +1071,12 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
>
> plarbdev = of_find_device_by_node(larbnode);
> if (!plarbdev) {
> - of_node_put(larbnode);
> - return -ENODEV;
> + ret = -ENODEV;
> + goto err_larbnode_put;
> }
> if (!plarbdev->dev.driver) {
> - of_node_put(larbnode);
> - return -EPROBE_DEFER;
> + ret = -EPROBE_DEFER;
> + goto err_larbnode_put;
> }
> data->larb_imu[id].dev = &plarbdev->dev;
>
> @@ -1107,9 +1107,20 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
> DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
> if (!link) {
> dev_err(dev, "Unable to link %s.\n", dev_name(data->smicomm_dev));
> - return -EINVAL;
> + ret = -EINVAL;
> + goto err_larbnode_put;
> }
> return 0;
> +
> +err_larbnode_put:
> + while (i--) {
> + larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
> + if (larbnode && of_device_is_available(larbnode)) {
> + of_node_put(larbnode);
> + of_node_put(larbnode);
> + }
This looks a bit awkward - could we not just iterate through
data->larb_imu and put dev->of_node for each valid dev?
Also, of_find_device_by_node() takes a reference on the struct device
itself, so strictly we should be doing put_device() on those as well if
we're bailing out.
Robin.
> + }
> + return ret;
> }
>
> static int mtk_iommu_probe(struct platform_device *pdev)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/5] iommu/mediatek: Add error path for loop of mm_dts_parse
2022-06-16 8:59 ` Robin Murphy
@ 2022-06-16 10:08 ` Yong Wu
2022-06-16 10:31 ` Robin Murphy
0 siblings, 1 reply; 16+ messages in thread
From: Yong Wu @ 2022-06-16 10:08 UTC (permalink / raw)
To: Robin Murphy
Cc: Joerg Roedel, Matthias Brugger, Will Deacon, iommu,
linux-mediatek, linux-arm-kernel, linux-kernel,
AngeloGioacchino Del Regno, mingyuan.ma, yf.wang, libo.kang,
chengci.xu, youlin.pei, anan.sun, xueqi.zhang, Guenter Roeck,
Dan Carpenter
On Thu, 2022-06-16 at 09:59 +0100, Robin Murphy wrote:
> On 2022-06-16 06:42, Yong Wu wrote:
> > The mtk_iommu_mm_dts_parse will parse the smi larbs nodes. if the
> > i+1
> > larb is parsed fail(return -EINVAL), we should of_node_put for the
> > 0..i
> > larbs. In the fail path, one of_node_put matches with
> > of_parse_phandle in
> > it.
> >
> > 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 | 21 ++++++++++++++++-----
> > 1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 3b2489e8a6dd..ab24078938bf 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -1071,12 +1071,12 @@ static int mtk_iommu_mm_dts_parse(struct
> > device *dev, struct component_match **m
> >
> > plarbdev = of_find_device_by_node(larbnode);
> > if (!plarbdev) {
> > - of_node_put(larbnode);
> > - return -ENODEV;
> > + ret = -ENODEV;
> > + goto err_larbnode_put;
> > }
> > if (!plarbdev->dev.driver) {
> > - of_node_put(larbnode);
> > - return -EPROBE_DEFER;
> > + ret = -EPROBE_DEFER;
> > + goto err_larbnode_put;
> > }
> > data->larb_imu[id].dev = &plarbdev->dev;
> >
> > @@ -1107,9 +1107,20 @@ static int mtk_iommu_mm_dts_parse(struct
> > device *dev, struct component_match **m
> > DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
> > if (!link) {
> > dev_err(dev, "Unable to link %s.\n", dev_name(data-
> > >smicomm_dev));
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto err_larbnode_put;
> > }
> > return 0;
> > +
> > +err_larbnode_put:
> > + while (i--) {
> > + larbnode = of_parse_phandle(dev->of_node,
> > "mediatek,larbs", i);
> > + if (larbnode && of_device_is_available(larbnode)) {
> > + of_node_put(larbnode);
> > + of_node_put(larbnode);
> > + }
>
> This looks a bit awkward - could we not just iterate through
> data->larb_imu and put dev->of_node for each valid dev?
It should work. Thanks very much.
>
> Also, of_find_device_by_node() takes a reference on the struct
> device
> itself, so strictly we should be doing put_device() on those as well
> if we're bailing out.
Thanks for this hint. A new reference for me. I will add it.
>
> Robin.
>
> > + }
> > + return ret;
> > }
> >
> > static int mtk_iommu_probe(struct platform_device *pdev)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/5] iommu/mediatek: Add error path for loop of mm_dts_parse
2022-06-16 10:08 ` Yong Wu
@ 2022-06-16 10:31 ` Robin Murphy
2022-06-23 1:54 ` Yong Wu
0 siblings, 1 reply; 16+ messages in thread
From: Robin Murphy @ 2022-06-16 10:31 UTC (permalink / raw)
To: Yong Wu
Cc: Joerg Roedel, Matthias Brugger, Will Deacon, iommu,
linux-mediatek, linux-arm-kernel, linux-kernel,
AngeloGioacchino Del Regno, mingyuan.ma, yf.wang, libo.kang,
chengci.xu, youlin.pei, anan.sun, xueqi.zhang, Guenter Roeck,
Dan Carpenter
On 2022-06-16 11:08, Yong Wu wrote:
> On Thu, 2022-06-16 at 09:59 +0100, Robin Murphy wrote:
>> On 2022-06-16 06:42, Yong Wu wrote:
>>> The mtk_iommu_mm_dts_parse will parse the smi larbs nodes. if the
>>> i+1
>>> larb is parsed fail(return -EINVAL), we should of_node_put for the
>>> 0..i
>>> larbs. In the fail path, one of_node_put matches with
>>> of_parse_phandle in
>>> it.
>>>
>>> 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 | 21 ++++++++++++++++-----
>>> 1 file changed, 16 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
>>> index 3b2489e8a6dd..ab24078938bf 100644
>>> --- a/drivers/iommu/mtk_iommu.c
>>> +++ b/drivers/iommu/mtk_iommu.c
>>> @@ -1071,12 +1071,12 @@ static int mtk_iommu_mm_dts_parse(struct
>>> device *dev, struct component_match **m
>>>
>>> plarbdev = of_find_device_by_node(larbnode);
>>> if (!plarbdev) {
>>> - of_node_put(larbnode);
>>> - return -ENODEV;
>>> + ret = -ENODEV;
>>> + goto err_larbnode_put;
>>> }
>>> if (!plarbdev->dev.driver) {
>>> - of_node_put(larbnode);
>>> - return -EPROBE_DEFER;
>>> + ret = -EPROBE_DEFER;
>>> + goto err_larbnode_put;
>>> }
>>> data->larb_imu[id].dev = &plarbdev->dev;
>>>
>>> @@ -1107,9 +1107,20 @@ static int mtk_iommu_mm_dts_parse(struct
>>> device *dev, struct component_match **m
>>> DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
>>> if (!link) {
>>> dev_err(dev, "Unable to link %s.\n", dev_name(data-
>>>> smicomm_dev));
>>> - return -EINVAL;
>>> + ret = -EINVAL;
>>> + goto err_larbnode_put;
>>> }
>>> return 0;
>>> +
>>> +err_larbnode_put:
>>> + while (i--) {
>>> + larbnode = of_parse_phandle(dev->of_node,
>>> "mediatek,larbs", i);
>>> + if (larbnode && of_device_is_available(larbnode)) {
>>> + of_node_put(larbnode);
>>> + of_node_put(larbnode);
>>> + }
>>
>> This looks a bit awkward - could we not just iterate through
>> data->larb_imu and put dev->of_node for each valid dev?
>
> It should work. Thanks very much.
>
>>
>> Also, of_find_device_by_node() takes a reference on the struct
>> device
>> itself, so strictly we should be doing put_device() on those as well
>> if we're bailing out.
>
> Thanks for this hint. A new reference for me. I will add it.
In fact, thinking about it some more we may as well do the of_node_put()
unconditionally immediately after the of_find_device_by_node() call, so
then it's *only* the device references we'd need to worry about cleaning
up in the failure path.
Robin.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/5] iommu/mediatek: Add error path for loop of mm_dts_parse
2022-06-16 10:31 ` Robin Murphy
@ 2022-06-23 1:54 ` Yong Wu
2022-06-23 10:08 ` Robin Murphy
0 siblings, 1 reply; 16+ messages in thread
From: Yong Wu @ 2022-06-23 1:54 UTC (permalink / raw)
To: Robin Murphy
Cc: Joerg Roedel, Matthias Brugger, Will Deacon, iommu,
linux-mediatek, linux-arm-kernel, linux-kernel,
AngeloGioacchino Del Regno, mingyuan.ma, yf.wang, libo.kang,
chengci.xu, youlin.pei, anan.sun, xueqi.zhang, Guenter Roeck,
Dan Carpenter
On Thu, 2022-06-16 at 11:31 +0100, Robin Murphy wrote:
> On 2022-06-16 11:08, Yong Wu wrote:
> > On Thu, 2022-06-16 at 09:59 +0100, Robin Murphy wrote:
> > > On 2022-06-16 06:42, Yong Wu wrote:
> > > > The mtk_iommu_mm_dts_parse will parse the smi larbs nodes. if
> > > > the
> > > > i+1
> > > > larb is parsed fail(return -EINVAL), we should of_node_put for
> > > > the
> > > > 0..i
> > > > larbs. In the fail path, one of_node_put matches with
> > > > of_parse_phandle in
> > > > it.
> > > >
> > > > 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 | 21 ++++++++++++++++-----
> > > > 1 file changed, 16 insertions(+), 5 deletions(-)
[snip..]
> > > > +err_larbnode_put:
> > > > + while (i--) {
> > > > + larbnode = of_parse_phandle(dev->of_node,
> > > > "mediatek,larbs", i);
> > > > + if (larbnode &&
> > > > of_device_is_available(larbnode)) {
> > > > + of_node_put(larbnode);
> > > > + of_node_put(larbnode);
> > > > + }
> > >
> > > This looks a bit awkward - could we not just iterate through
> > > data->larb_imu and put dev->of_node for each valid dev?
> >
> > It should work. Thanks very much.
> >
> > >
> > > Also, of_find_device_by_node() takes a reference on the struct
> > > device
> > > itself, so strictly we should be doing put_device() on those as
> > > well
> > > if we're bailing out.
> >
> > Thanks for this hint. A new reference for me. I will add it.
>
> In fact, thinking about it some more we may as well do the
> of_node_put()
> unconditionally immediately after the of_find_device_by_node() call,
of_node_put is called in component_release_of in the normal case, thus
we shouldn't call of_node_put unconditionally. Right?
(Sorry for reply late)
> so
> then it's *only* the device references we'd need to worry about
> cleaning
> up in the failure path.
>
> Robin.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/5] iommu/mediatek: Add error path for loop of mm_dts_parse
2022-06-23 1:54 ` Yong Wu
@ 2022-06-23 10:08 ` Robin Murphy
0 siblings, 0 replies; 16+ messages in thread
From: Robin Murphy @ 2022-06-23 10:08 UTC (permalink / raw)
To: Yong Wu
Cc: Joerg Roedel, Matthias Brugger, Will Deacon, iommu,
linux-mediatek, linux-arm-kernel, linux-kernel,
AngeloGioacchino Del Regno, mingyuan.ma, yf.wang, libo.kang,
chengci.xu, youlin.pei, anan.sun, xueqi.zhang, Guenter Roeck,
Dan Carpenter
On 2022-06-23 02:54, Yong Wu wrote:
> On Thu, 2022-06-16 at 11:31 +0100, Robin Murphy wrote:
>> On 2022-06-16 11:08, Yong Wu wrote:
>>> On Thu, 2022-06-16 at 09:59 +0100, Robin Murphy wrote:
>>>> On 2022-06-16 06:42, Yong Wu wrote:
>>>>> The mtk_iommu_mm_dts_parse will parse the smi larbs nodes. if
>>>>> the
>>>>> i+1
>>>>> larb is parsed fail(return -EINVAL), we should of_node_put for
>>>>> the
>>>>> 0..i
>>>>> larbs. In the fail path, one of_node_put matches with
>>>>> of_parse_phandle in
>>>>> it.
>>>>>
>>>>> 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 | 21 ++++++++++++++++-----
>>>>> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> [snip..]
>
>>>>> +err_larbnode_put:
>>>>> + while (i--) {
>>>>> + larbnode = of_parse_phandle(dev->of_node,
>>>>> "mediatek,larbs", i);
>>>>> + if (larbnode &&
>>>>> of_device_is_available(larbnode)) {
>>>>> + of_node_put(larbnode);
>>>>> + of_node_put(larbnode);
>>>>> + }
>>>>
>>>> This looks a bit awkward - could we not just iterate through
>>>> data->larb_imu and put dev->of_node for each valid dev?
>>>
>>> It should work. Thanks very much.
>>>
>>>>
>>>> Also, of_find_device_by_node() takes a reference on the struct
>>>> device
>>>> itself, so strictly we should be doing put_device() on those as
>>>> well
>>>> if we're bailing out.
>>>
>>> Thanks for this hint. A new reference for me. I will add it.
>>
>> In fact, thinking about it some more we may as well do the
>> of_node_put()
>> unconditionally immediately after the of_find_device_by_node() call,
>
> of_node_put is called in component_release_of in the normal case, thus
> we shouldn't call of_node_put unconditionally. Right?
As it stands, yes. However I'm also figuring that we could just use
component_match_add() there, and probably also switch to
component_compare_dev as the the comparison, since we've already
resolved the larb device, and it is the device itself that we're
interested in here, rather than just its of_node.
I *think* this idea could end up with simpler code overall, but as
always, feel free to ignore the suggestion if you think it wouldn't make
enough difference to be worth the bother.
Thanks,
Robin.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/5] iommu/mediatek: Add error path for loop of mm_dts_parse
2022-06-16 5:42 ` [PATCH v2 2/5] iommu/mediatek: Add error path for loop of mm_dts_parse Yong Wu
2022-06-16 8:59 ` Robin Murphy
@ 2022-06-16 13:49 ` Matthias Brugger
2022-06-23 2:12 ` Yong Wu
1 sibling, 1 reply; 16+ messages in thread
From: Matthias Brugger @ 2022-06-16 13:49 UTC (permalink / raw)
To: Yong Wu, Joerg Roedel, Will Deacon
Cc: iommu, linux-mediatek, linux-arm-kernel, linux-kernel,
AngeloGioacchino Del Regno, mingyuan.ma, yf.wang, libo.kang,
chengci.xu, youlin.pei, anan.sun, xueqi.zhang, Guenter Roeck,
Dan Carpenter
On 16/06/2022 07:42, Yong Wu wrote:
> The mtk_iommu_mm_dts_parse will parse the smi larbs nodes. if the i+1
> larb is parsed fail(return -EINVAL), we should of_node_put for the 0..i
> larbs. In the fail path, one of_node_put matches with of_parse_phandle in
> it.
>
> 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 | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 3b2489e8a6dd..ab24078938bf 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -1071,12 +1071,12 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
>
Don't we need to call the goto also on error case of:
larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
Regards,
Matthias
> plarbdev = of_find_device_by_node(larbnode);
> if (!plarbdev) {
> - of_node_put(larbnode);
> - return -ENODEV;
> + ret = -ENODEV;
> + goto err_larbnode_put;
> }
> if (!plarbdev->dev.driver) {
> - of_node_put(larbnode);
> - return -EPROBE_DEFER;
> + ret = -EPROBE_DEFER;
> + goto err_larbnode_put;
> }
> data->larb_imu[id].dev = &plarbdev->dev;
>
> @@ -1107,9 +1107,20 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
> DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
> if (!link) {
> dev_err(dev, "Unable to link %s.\n", dev_name(data->smicomm_dev));
> - return -EINVAL;
> + ret = -EINVAL;
> + goto err_larbnode_put;
> }
> return 0;
> +
> +err_larbnode_put:
> + while (i--) {
> + larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
> + if (larbnode && of_device_is_available(larbnode)) {
> + of_node_put(larbnode);
> + of_node_put(larbnode);
> + }
> + }
> + return ret;
> }
>
> static int mtk_iommu_probe(struct platform_device *pdev)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/5] iommu/mediatek: Add error path for loop of mm_dts_parse
2022-06-16 13:49 ` Matthias Brugger
@ 2022-06-23 2:12 ` Yong Wu
0 siblings, 0 replies; 16+ messages in thread
From: Yong Wu @ 2022-06-23 2:12 UTC (permalink / raw)
To: Matthias Brugger
Cc: Joerg Roedel, Will Deacon, iommu, linux-mediatek,
linux-arm-kernel, linux-kernel, AngeloGioacchino Del Regno,
mingyuan.ma, yf.wang, libo.kang, chengci.xu, youlin.pei,
anan.sun, xueqi.zhang, Guenter Roeck, Dan Carpenter
On Thu, 2022-06-16 at 15:49 +0200, Matthias Brugger wrote:
>
> On 16/06/2022 07:42, Yong Wu wrote:
> > The mtk_iommu_mm_dts_parse will parse the smi larbs nodes. if the
> > i+1
> > larb is parsed fail(return -EINVAL), we should of_node_put for the
> > 0..i
> > larbs. In the fail path, one of_node_put matches with
> > of_parse_phandle in
> > it.
> >
> > 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 | 21 ++++++++++++++++-----
> > 1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 3b2489e8a6dd..ab24078938bf 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -1071,12 +1071,12 @@ static int mtk_iommu_mm_dts_parse(struct
> > device *dev, struct component_match **m
> >
>
> Don't we need to call the goto also on error case of:
>
> larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
Thanks very much.
exactly right. I will add in next version.
> Regards,
> Matthias
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 3/5] iommu/mediatek: Validate number of phandles associated with "mediatek, larbs"
2022-06-16 5:41 [PATCH v2 0/5] iommu/mediatek: Improve safety from dts Yong Wu
2022-06-16 5:41 ` [PATCH v2 1/5] iommu/mediatek: Use dev_err_probe to mute probe_defer err log Yong Wu
2022-06-16 5:42 ` [PATCH v2 2/5] iommu/mediatek: Add error path for loop of mm_dts_parse Yong Wu
@ 2022-06-16 5:42 ` Yong Wu
2022-06-16 5:42 ` [PATCH v2 4/5] iommu/mediatek: Improve safety for mediatek, smi property in larb nodes Yong Wu
2022-06-16 5:42 ` [PATCH v2 5/5] iommu/mediatek: Remove a unused "mapping" which is only for v1 Yong Wu
4 siblings, 0 replies; 16+ messages in thread
From: Yong Wu @ 2022-06-16 5:42 UTC (permalink / raw)
To: Joerg Roedel, Matthias Brugger, Will Deacon
Cc: iommu, linux-mediatek, linux-arm-kernel, linux-kernel, Yong Wu,
AngeloGioacchino Del Regno, mingyuan.ma, yf.wang, libo.kang,
chengci.xu, youlin.pei, anan.sun, xueqi.zhang, Guenter Roeck,
Dan Carpenter
From: Guenter Roeck <groeck@chromium.org>
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 <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
drivers/iommu/mtk_iommu.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index ab24078938bf..a869d4aee7b3 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1052,6 +1052,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,10 @@ 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) {
+ ret = -EINVAL;
+ goto err_larbnode_put;
+ }
plarbdev = of_find_device_by_node(larbnode);
if (!plarbdev) {
@@ -1078,6 +1084,11 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
ret = -EPROBE_DEFER;
goto err_larbnode_put;
}
+
+ if (data->larb_imu[id].dev) {
+ ret = -EEXIST;
+ goto err_larbnode_put;
+ }
data->larb_imu[id].dev = &plarbdev->dev;
component_match_add_release(dev, match, component_release_of,
--
2.18.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 4/5] iommu/mediatek: Improve safety for mediatek, smi property in larb nodes
2022-06-16 5:41 [PATCH v2 0/5] iommu/mediatek: Improve safety from dts Yong Wu
` (2 preceding siblings ...)
2022-06-16 5:42 ` [PATCH v2 3/5] iommu/mediatek: Validate number of phandles associated with "mediatek, larbs" Yong Wu
@ 2022-06-16 5:42 ` Yong Wu
2022-06-16 5:42 ` [PATCH v2 5/5] iommu/mediatek: Remove a unused "mapping" which is only for v1 Yong Wu
4 siblings, 0 replies; 16+ messages in thread
From: Yong Wu @ 2022-06-16 5:42 UTC (permalink / raw)
To: Joerg Roedel, Matthias Brugger, Will Deacon
Cc: iommu, linux-mediatek, linux-arm-kernel, linux-kernel, Yong Wu,
AngeloGioacchino Del Regno, mingyuan.ma, yf.wang, libo.kang,
chengci.xu, youlin.pei, anan.sun, xueqi.zhang, Guenter Roeck,
Dan Carpenter
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 <groeck@chromium.org>
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
drivers/iommu/mtk_iommu.c | 49 ++++++++++++++++++++++++++-------------
1 file changed, 33 insertions(+), 16 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index a869d4aee7b3..5e86fd48928a 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1044,7 +1044,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;
struct device_link *link;
int i, larb_nr, ret;
@@ -1056,6 +1056,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);
@@ -1091,27 +1092,43 @@ static int mtk_iommu_mm_dts_parse(struct device *dev, struct component_match **m
}
data->larb_imu[id].dev = &plarbdev->dev;
+ /* 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_larbnode_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;
+
+ if (!frst_avail_smicomm_node) {
+ frst_avail_smicomm_node = smicomm_node;
+ } else if (frst_avail_smicomm_node != smicomm_node) {
+ dev_err(dev, "mediatek,smi is not right @larb%d.", id);
+ of_node_put(smicomm_node);
+ ret = -EINVAL;
+ goto err_larbnode_put;
+ } else {
+ of_node_put(smicomm_node);
+ }
+
component_match_add_release(dev, match, component_release_of,
component_compare_of, larbnode);
}
- /* 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;
-
- plarbdev = of_find_device_by_node(smicomm_node);
- of_node_put(smicomm_node);
+ plarbdev = of_find_device_by_node(frst_avail_smicomm_node);
+ of_node_put(frst_avail_smicomm_node);
data->smicomm_dev = &plarbdev->dev;
link = device_link_add(data->smicomm_dev, dev,
--
2.18.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 5/5] iommu/mediatek: Remove a unused "mapping" which is only for v1
2022-06-16 5:41 [PATCH v2 0/5] iommu/mediatek: Improve safety from dts Yong Wu
` (3 preceding siblings ...)
2022-06-16 5:42 ` [PATCH v2 4/5] iommu/mediatek: Improve safety for mediatek, smi property in larb nodes Yong Wu
@ 2022-06-16 5:42 ` Yong Wu
2022-06-16 12:03 ` AngeloGioacchino Del Regno
2022-06-16 13:56 ` Matthias Brugger
4 siblings, 2 replies; 16+ messages in thread
From: Yong Wu @ 2022-06-16 5:42 UTC (permalink / raw)
To: Joerg Roedel, Matthias Brugger, Will Deacon
Cc: iommu, linux-mediatek, linux-arm-kernel, linux-kernel, Yong Wu,
AngeloGioacchino Del Regno, mingyuan.ma, yf.wang, libo.kang,
chengci.xu, youlin.pei, anan.sun, xueqi.zhang, Guenter Roeck,
Dan Carpenter
Just remove a unused variable that only is for mtk_iommu_v1.
Fixes: 9485a04a5bb9 ("iommu/mediatek: Separate mtk_iommu_data for v1 and v2")
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
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 5e86fd48928a..e65e705d9fc1 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 5/5] iommu/mediatek: Remove a unused "mapping" which is only for v1
2022-06-16 5:42 ` [PATCH v2 5/5] iommu/mediatek: Remove a unused "mapping" which is only for v1 Yong Wu
@ 2022-06-16 12:03 ` AngeloGioacchino Del Regno
2022-06-16 13:56 ` Matthias Brugger
1 sibling, 0 replies; 16+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-06-16 12:03 UTC (permalink / raw)
To: Yong Wu, Joerg Roedel, Matthias Brugger, Will Deacon
Cc: iommu, linux-mediatek, linux-arm-kernel, linux-kernel,
mingyuan.ma, yf.wang, libo.kang, chengci.xu, youlin.pei,
anan.sun, xueqi.zhang, Guenter Roeck, Dan Carpenter
Il 16/06/22 07:42, Yong Wu ha scritto:
> Just remove a unused variable that only is for mtk_iommu_v1.
>
> Fixes: 9485a04a5bb9 ("iommu/mediatek: Separate mtk_iommu_data for v1 and v2")
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
The title isn't immediately clear, looks like you're removing some mapping, not
a struct member...
Perhaps... iommu/mediatek: Remove unused "mapping" member from mtk_iommu_data ?
After clarifying the commit title:
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Cheers,
Angelo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 5/5] iommu/mediatek: Remove a unused "mapping" which is only for v1
2022-06-16 5:42 ` [PATCH v2 5/5] iommu/mediatek: Remove a unused "mapping" which is only for v1 Yong Wu
2022-06-16 12:03 ` AngeloGioacchino Del Regno
@ 2022-06-16 13:56 ` Matthias Brugger
1 sibling, 0 replies; 16+ messages in thread
From: Matthias Brugger @ 2022-06-16 13:56 UTC (permalink / raw)
To: Yong Wu, Joerg Roedel, Will Deacon
Cc: iommu, linux-mediatek, linux-arm-kernel, linux-kernel,
AngeloGioacchino Del Regno, mingyuan.ma, yf.wang, libo.kang,
chengci.xu, youlin.pei, anan.sun, xueqi.zhang, Guenter Roeck,
Dan Carpenter
On 16/06/2022 07:42, Yong Wu wrote:
> Just remove a unused variable that only is for mtk_iommu_v1.
>
> Fixes: 9485a04a5bb9 ("iommu/mediatek: Separate mtk_iommu_data for v1 and v2")
It does not fix a bug, so no fixes tag here needed.
With that:
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
> 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 5e86fd48928a..e65e705d9fc1 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 */
>
> /*
^ permalink raw reply [flat|nested] 16+ messages in thread