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

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)

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

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)

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)

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.

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.

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.