[PATCH 0/2] [media] exynos4-is: Trivial fixes for DT port/endpoint parse logic

[PATCH 0/2] [media] exynos4-is: Trivial fixes for DT port/endpoint parse logic

[Javier Martinez Canillas]

Hello,

This series have two trivial fixes for issues that I noticed while
reading as a reference the driver's functions that parse the graph
port and endpoints nodes.

It was only compile tested because I don't have access to a Exynos4
hardware to test the DT parsing, but the patches are very simple.

Best regards,
Javier


Javier Martinez Canillas (2):
  [media] exynos4-is: Put node before s5pcsis_parse_dt() return error
  [media] exynos4-is: FIMC port parse should fail if there's no endpoint

 drivers/media/platform/exynos4-is/media-dev.c | 2 +-
 drivers/media/platform/exynos4-is/mipi-csis.c | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
2.5.0

[PATCH 1/2] [media] exynos4-is: Put node before s5pcsis_parse_dt() return error

[Javier Martinez Canillas]

The MIPI CSIS DT parse function return an -ENXIO errno if the port #
is outside of the supported values. But it doesn't call of_node_put()
to decrement the node's reference counter, that's incremented inside
the of_graph_get_next_endpoint() function that was called before.

Instead of just returning, go to the error path that already does it.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/media/platform/exynos4-is/mipi-csis.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
index bd5c46c3d4b7..bf954424e7be 100644
--- a/drivers/media/platform/exynos4-is/mipi-csis.c
+++ b/drivers/media/platform/exynos4-is/mipi-csis.c
@@ -757,8 +757,10 @@ static int s5pcsis_parse_dt(struct platform_device *pdev,
 		goto err;
 
 	state->index = endpoint.base.port - FIMC_INPUT_MIPI_CSI2_0;
-	if (state->index >= CSIS_MAX_ENTITIES)
-		return -ENXIO;
+	if (state->index >= CSIS_MAX_ENTITIES) {
+		ret = -ENXIO;
+		goto err;
+	}
 
 	/* Get MIPI CSI-2 bus configration from the endpoint node. */
 	of_property_read_u32(node, "samsung,csis-hs-settle",
-- 
2.5.0

[PATCH 2/2] [media] exynos4-is: FIMC port parse should fail if there's no endpoint

[Javier Martinez Canillas]

The fimc_md_parse_port_node() function return 0 if an endpoint node is
not found but according to Documentation/devicetree/bindings/graph.txt,
a port must always have at least one enpoint.

So return an -EINVAL errno code to the caller instead, so it knows that
the port node parse failed due an invalid Device Tree description.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---

 drivers/media/platform/exynos4-is/media-dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index feb521f28e14..06f3d75c9a0e 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -394,7 +394,7 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
 	/* Assume here a port node can have only one endpoint node. */
 	ep = of_get_next_child(port, NULL);
 	if (!ep)
-		return 0;
+		return -EINVAL;
 
 	ret = v4l2_of_parse_endpoint(ep, &endpoint);
 	if (ret) {
-- 
2.5.0

Re: [PATCH 2/2] [media] exynos4-is: FIMC port parse should fail if there's no endpoint

[Sylwester Nawrocki]

On 03/04/2016 09:20 PM, Javier Martinez Canillas wrote:
> The fimc_md_parse_port_node() function return 0 if an endpoint node is
> not found but according to Documentation/devicetree/bindings/graph.txt,
> a port must always have at least one enpoint.
> 
> So return an -EINVAL errno code to the caller instead, so it knows that
> the port node parse failed due an invalid Device Tree description.

I don't think it is forbidden to have a port node in device tree
containing no endpoint nodes. Empty port node means only that,
for example, a subsystem has a port/bus for connecting external
devices but nothing is actually connected to it.

In case of Exynos CSIS it might not be so useful to have an empty
port node specified in some top level *.dtsi file and only
the endpoints specified in a board specific dts file. Nevertheless,
I wouldn't be saying in general a port node must always have some
endpoint node defined.

I could apply this patch as it doesn't do any harm considering
existing dts files in the kernel tree (arch/arm/boot/dts/
exynos4412-trats2.dts), but the commit description would need to
be changed.

-- 
Thanks,
Sylwester

Re: [PATCH 2/2] [media] exynos4-is: FIMC port parse should fail if there's no endpoint

[Javier Martinez Canillas]

Hello Sylwester,

On Fri, Mar 11, 2016 at 10:03 AM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> On 03/04/2016 09:20 PM, Javier Martinez Canillas wrote:
>> The fimc_md_parse_port_node() function return 0 if an endpoint node is
>> not found but according to Documentation/devicetree/bindings/graph.txt,
>> a port must always have at least one enpoint.
>>
>> So return an -EINVAL errno code to the caller instead, so it knows that
>> the port node parse failed due an invalid Device Tree description.
>
> I don't think it is forbidden to have a port node in device tree
> containing no endpoint nodes. Empty port node means only that,
> for example, a subsystem has a port/bus for connecting external
> devices but nothing is actually connected to it.
>

That's not what I understood by reading both
Documentation/devicetree/bindings/media/video-interfaces.txt and
Documentation/devicetree/bindings/graph.txt but maybe these are not
that clear about it or I just failed to parse the english.

> In case of Exynos CSIS it might not be so useful to have an empty
> port node specified in some top level *.dtsi file and only
> the endpoints specified in a board specific dts file. Nevertheless,
> I wouldn't be saying in general a port node must always have some
> endpoint node defined.
>

Ok, but if that is valid then I believe that at the very least
Documentation/devicetree/bindings/media/samsung-fimc.txt should
explicitly mention which (sub)nodes are optional and which are
required so the DT parsing logic could follow what's documented there.

> I could apply this patch as it doesn't do any harm considering
> existing dts files in the kernel tree (arch/arm/boot/dts/
> exynos4412-trats2.dts), but the commit description would need to
> be changed.
>

I don't mind if you want to change the commit message but if those
nodes are really optional then a follow-up should be to update the DT
binding docs to make that clear IMHO.

> --
> Thanks,
> Sylwester
> --

Best regards,
Javier

Re: [PATCH 2/2] [media] exynos4-is: FIMC port parse should fail if there's no endpoint

[Javier Martinez Canillas]

Hello Sylwester,

On 03/11/2016 10:03 AM, Sylwester Nawrocki wrote:
> On 03/04/2016 09:20 PM, Javier Martinez Canillas wrote:
>> The fimc_md_parse_port_node() function return 0 if an endpoint node is
>> not found but according to Documentation/devicetree/bindings/graph.txt,
>> a port must always have at least one enpoint.
>>
>> So return an -EINVAL errno code to the caller instead, so it knows that
>> the port node parse failed due an invalid Device Tree description.
> 
> I don't think it is forbidden to have a port node in device tree
> containing no endpoint nodes. Empty port node means only that,
> for example, a subsystem has a port/bus for connecting external
> devices but nothing is actually connected to it.
> 
> In case of Exynos CSIS it might not be so useful to have an empty
> port node specified in some top level *.dtsi file and only
> the endpoints specified in a board specific dts file. Nevertheless,
> I wouldn't be saying in general a port node must always have some
> endpoint node defined.
> 

You are right, I asked Laurent and he confirms what you said that
it's possible to have ports with no endpoints. I still think the
DT binding docs could be more clear but that's a separate issue.

> I could apply this patch as it doesn't do any harm considering
> existing dts files in the kernel tree (arch/arm/boot/dts/
> exynos4412-trats2.dts), but the commit description would need to
> be changed.
> 

No worries, the current code is correct if endpoints are optional
and this patch is wrong so it should not be applied.
Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 0/2] [media] exynos4-is: Trivial fixes for DT port/endpoint parse logic

[Krzysztof Kozlowski]

2016-03-05 5:20 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
> Hello,
>
> This series have two trivial fixes for issues that I noticed while
> reading as a reference the driver's functions that parse the graph
> port and endpoints nodes.
>
> It was only compile tested because I don't have access to a Exynos4
> hardware to test the DT parsing, but the patches are very simple.

Not directly related, but similar: my previous two patches for missing
of_node_put [0] are unfortunately still waiting. Although I have
Exynos4 boards, but I don't have infrastructure/scripts to test it.

Best regards,
Krzysztof

[0] https://patchwork.linuxtv.org/patch/32707/

Re: [PATCH 0/2] [media] exynos4-is: Trivial fixes for DT port/endpoint parse logic

[Sylwester Nawrocki]

Hi Javier, Krzysztof,

On 03/05/2016 05:35 AM, Krzysztof Kozlowski wrote:
> 2016-03-05 5:20 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>> > Hello,
>> >
>> > This series have two trivial fixes for issues that I noticed while
>> > reading as a reference the driver's functions that parse the graph
>> > port and endpoints nodes.
>> >
>> > It was only compile tested because I don't have access to a Exynos4
>> > hardware to test the DT parsing, but the patches are very simple.
>
> Not directly related, but similar: my previous two patches for missing
> of_node_put [0] are unfortunately still waiting. Although I have
> Exynos4 boards, but I don't have infrastructure/scripts to test it.
> 
> Best regards,
> Krzysztof
> 
> [0] https://patchwork.linuxtv.org/patch/32707/

Thanks for the patches, I've delegated them to myself and I'm going to
review/apply them this week.

--
Regards,
Sylwester

Re: [PATCH 0/2] [media] exynos4-is: Trivial fixes for DT port/endpoint parse logic

[Javier Martinez Canillas]

Hello Sylwester,

On 03/07/2016 06:24 AM, Sylwester Nawrocki wrote:
> Hi Javier, Krzysztof,
> 
> On 03/05/2016 05:35 AM, Krzysztof Kozlowski wrote:
>> 2016-03-05 5:20 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>>>> Hello,
>>>>
>>>> This series have two trivial fixes for issues that I noticed while
>>>> reading as a reference the driver's functions that parse the graph
>>>> port and endpoints nodes.
>>>>
>>>> It was only compile tested because I don't have access to a Exynos4
>>>> hardware to test the DT parsing, but the patches are very simple.
>>
>> Not directly related, but similar: my previous two patches for missing
>> of_node_put [0] are unfortunately still waiting. Although I have
>> Exynos4 boards, but I don't have infrastructure/scripts to test it.
>>
>> Best regards,
>> Krzysztof
>>

I've reviewed Krzysztof and looks good to me.

>> [0] https://patchwork.linuxtv.org/patch/32707/
> 
> Thanks for the patches, I've delegated them to myself and I'm going to
> review/apply them this week.
>

Thanks, I just noticed another similar issue in the driver now and is
that fimc_is_parse_sensor_config() uses the same struct device_node *
for looking up the I2C sensor, port and endpoint and thus not doing a
of_node_put() for all the nodes on the error path.

I think the right fix is to have a separate struct device_node * for
each so their reference counter cand be incremented and decremented.

> --
> Regards,
> Sylwester
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 0/2] [media] exynos4-is: Trivial fixes for DT port/endpoint parse logic

[Sylwester Nawrocki]

On 03/07/2016 03:30 PM, Javier Martinez Canillas wrote:
> Thanks, I just noticed another similar issue in the driver now and is
> that fimc_is_parse_sensor_config() uses the same struct device_node *
> for looking up the I2C sensor, port and endpoint and thus not doing a
> of_node_put() for all the nodes on the error path.
> 
> I think the right fix is to have a separate struct device_node * for
> each so their reference counter can be incremented and decremented.

Yes, the node reference count is indeed not handled very well there,
feel free to submit a patch to fix that bug.

-- 
Regards,
Sylwester

Re: [PATCH 0/2] [media] exynos4-is: Trivial fixes for DT port/endpoint parse logic

[Javier Martinez Canillas]

Hello Sylwester,

On Fri, Mar 11, 2016 at 10:09 AM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> On 03/07/2016 03:30 PM, Javier Martinez Canillas wrote:
>> Thanks, I just noticed another similar issue in the driver now and is
>> that fimc_is_parse_sensor_config() uses the same struct device_node *
>> for looking up the I2C sensor, port and endpoint and thus not doing a
>> of_node_put() for all the nodes on the error path.
>>
>> I think the right fix is to have a separate struct device_node * for
>> each so their reference counter can be incremented and decremented.
>
> Yes, the node reference count is indeed not handled very well there,
> feel free to submit a patch to fix that bug.
>

Ok, I'll post a patch to fix that once all the in-flight patches land
to avoid merge conflicts.

> --
> Regards,
> Sylwester

Best regards,
Javier