| Message ID | 20230614-topic-camss_grpah-v2-1-57d9d5e49038@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] media: camss: Intepret OF graph connections more sensibly | expand |
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
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
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 --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);
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,