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. > + 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
