diff mbox series

[v2] media: camss: Intepret OF graph connections more sensibly

Message ID 20230614-topic-camss_grpah-v2-1-57d9d5e49038@linaro.org
State New
Headers
Series [v2] media: camss: Intepret OF graph connections more sensibly |

Commit Message

Konrad Dybcio July 15, 2023, 3:37 p.m. UTC 
  Not all endpoints of camss have to be populated. In fact, most of the
time they shouldn't be as n-th auxilliary cameras are usually ewaste.

Don't fail probing the entire camss even even one endpoint is not
linked and throw an error when none is found.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Changes in v2:
- Use if-else instead of the ternary operator (Bryan)
- Drop "RFC"
- Link to v1: https://lore.kernel.org/r/20230614-topic-camss_grpah-v1-1-5f4b516310fa@linaro.org
---
 drivers/media/platform/qcom/camss/camss.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)


---
base-commit: 7c2878be573282a9961c359b806ccf70afe1a6b6
change-id: 20230614-topic-camss_grpah-39f9a4f7420c

Best regards,

Comments

Johan Hovold July 17, 2023, 10:48 a.m. UTC | #1 
On Sat, Jul 15, 2023 at 05:37:52PM +0200, Konrad Dybcio wrote:
> Not all endpoints of camss have to be populated. In fact, most of the
> time they shouldn't be as n-th auxilliary cameras are usually ewaste.
> 
> Don't fail probing the entire camss even even one endpoint is not
> linked and throw an error when none is found.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> Changes in v2:
> - Use if-else instead of the ternary operator (Bryan)
> - Drop "RFC"
> - Link to v1: https://lore.kernel.org/r/20230614-topic-camss_grpah-v1-1-5f4b516310fa@linaro.org
> ---
>  drivers/media/platform/qcom/camss/camss.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 1ef26aea3eae..8b75197fa5d7 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1084,9 +1084,8 @@ static int camss_of_parse_ports(struct camss *camss)
>  
>  		remote = of_graph_get_remote_port_parent(node);
>  		if (!remote) {
> -			dev_err(dev, "Cannot get remote parent\n");
> -			ret = -EINVAL;
> -			goto err_cleanup;
> +			of_node_put(node);

This is broken and could potentially lead to a use after free.

Specifically, the iteration macro already takes care of putting this
reference.

> +			continue;
>  		}
>  
>  		csd = v4l2_async_nf_add_fwnode(&camss->notifier,
> @@ -1105,7 +1104,10 @@ static int camss_of_parse_ports(struct camss *camss)
>  		num_subdevs++;
>  	}
>  
> -	return num_subdevs;
> +	if (num_subdevs)
> +		return num_subdevs;
> +
> +	return -EINVAL;

Please change this so that you test for the error condition rather than
its inverse for symmetry. That is

	if (!num_subdevs)
		return -EINVAL;

	return num_subdevs;

Returning EINVAL (invalid argument) is perhaps not the best choice, but
the driver already does so here and in other places so keeping it for
now should be fine.

>  err_cleanup:
>  	of_node_put(node);

Johan
Konrad Dybcio July 17, 2023, 12:40 p.m. UTC | #2 
On 17.07.2023 12:48, Johan Hovold wrote:
> On Sat, Jul 15, 2023 at 05:37:52PM +0200, Konrad Dybcio wrote:
>> Not all endpoints of camss have to be populated. In fact, most of the
>> time they shouldn't be as n-th auxilliary cameras are usually ewaste.
>>
>> Don't fail probing the entire camss even even one endpoint is not
>> linked and throw an error when none is found.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>> Changes in v2:
>> - Use if-else instead of the ternary operator (Bryan)
>> - Drop "RFC"
>> - Link to v1: https://lore.kernel.org/r/20230614-topic-camss_grpah-v1-1-5f4b516310fa@linaro.org
>> ---
>>  drivers/media/platform/qcom/camss/camss.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
>> index 1ef26aea3eae..8b75197fa5d7 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -1084,9 +1084,8 @@ static int camss_of_parse_ports(struct camss *camss)
>>  
>>  		remote = of_graph_get_remote_port_parent(node);
>>  		if (!remote) {
>> -			dev_err(dev, "Cannot get remote parent\n");
>> -			ret = -EINVAL;
>> -			goto err_cleanup;
>> +			of_node_put(node);
> 
> This is broken and could potentially lead to a use after free.
> 
> Specifically, the iteration macro already takes care of putting this
> reference.
/**
 * for_each_endpoint_of_node - iterate over every endpoint in a device node
 * @parent: parent device node containing ports and endpoints
 * @child: loop variable pointing to the current endpoint node
 *
 * When breaking out of the loop, of_node_put(child) has to be called manually.
 */

> 
>> +			continue;
>>  		}
>>  
>>  		csd = v4l2_async_nf_add_fwnode(&camss->notifier,
>> @@ -1105,7 +1104,10 @@ static int camss_of_parse_ports(struct camss *camss)
>>  		num_subdevs++;
>>  	}
>>  
>> -	return num_subdevs;
>> +	if (num_subdevs)
>> +		return num_subdevs;
>> +
>> +	return -EINVAL;
> 
> Please change this so that you test for the error condition rather than
> its inverse for symmetry. That is
> 
> 	if (!num_subdevs)
> 		return -EINVAL;
> 
> 	return num_subdevs;
Right, this makes more sense

Konrad
> 
> Returning EINVAL (invalid argument) is perhaps not the best choice, but
> the driver already does so here and in other places so keeping it for
> now should be fine.
> 
>>  err_cleanup:
>>  	of_node_put(node);
> 
> Johan
Johan Hovold July 17, 2023, 12:45 p.m. UTC | #3 
On Mon, Jul 17, 2023 at 02:40:05PM +0200, Konrad Dybcio wrote:
> On 17.07.2023 12:48, Johan Hovold wrote:
> > On Sat, Jul 15, 2023 at 05:37:52PM +0200, Konrad Dybcio wrote:

> >> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> >> index 1ef26aea3eae..8b75197fa5d7 100644
> >> --- a/drivers/media/platform/qcom/camss/camss.c
> >> +++ b/drivers/media/platform/qcom/camss/camss.c
> >> @@ -1084,9 +1084,8 @@ static int camss_of_parse_ports(struct camss *camss)
> >>  
> >>  		remote = of_graph_get_remote_port_parent(node);
> >>  		if (!remote) {
> >> -			dev_err(dev, "Cannot get remote parent\n");
> >> -			ret = -EINVAL;
> >> -			goto err_cleanup;
> >> +			of_node_put(node);
> > 
> > This is broken and could potentially lead to a use after free.
> > 
> > Specifically, the iteration macro already takes care of putting this
> > reference.

> /**
>  * for_each_endpoint_of_node - iterate over every endpoint in a device node
>  * @parent: parent device node containing ports and endpoints
>  * @child: loop variable pointing to the current endpoint node
>  *
>  * When breaking out of the loop, of_node_put(child) has to be called manually.
>  */

Please read this comment you just pasted here again as it seems you did
not understand it.

> > 
> >> +			continue;

And again, please remember to trim your replies. Including context after
your reply is almost always wrong.

Johan
diff mbox series

Patch

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 1ef26aea3eae..8b75197fa5d7 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1084,9 +1084,8 @@  static int camss_of_parse_ports(struct camss *camss)
 
 		remote = of_graph_get_remote_port_parent(node);
 		if (!remote) {
-			dev_err(dev, "Cannot get remote parent\n");
-			ret = -EINVAL;
-			goto err_cleanup;
+			of_node_put(node);
+			continue;
 		}
 
 		csd = v4l2_async_nf_add_fwnode(&camss->notifier,
@@ -1105,7 +1104,10 @@  static int camss_of_parse_ports(struct camss *camss)
 		num_subdevs++;
 	}
 
-	return num_subdevs;
+	if (num_subdevs)
+		return num_subdevs;
+
+	return -EINVAL;
 
 err_cleanup:
 	of_node_put(node);