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@xxxxxxxxxx> >> --- >> 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@xxxxxxxxxx >> --- >> 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
