[PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Abel Vesa]

The assignment of the of_node to the aux bridge needs to mark the
of_node as reused as well, otherwise resource providers like pinctrl will
report a gpio as already requested by a different device when both pinconf
and gpios property are present.
Fix that by using the device_set_of_node_from_dev() helper instead.

Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers")
Cc: stable@vger.kernel.org      # 6.8
Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
Changes in v2:
- Re-worded commit to be more explicit of what it fixes, as Johan suggested
- Used device_set_of_node_from_dev() helper, as per Johan's suggestion
- Added Fixes tag and cc'ed stable
- Link to v1: https://lore.kernel.org/r/20241017-drm-aux-bridge-mark-of-node-reused-v1-1-7cd5702bb4f2@linaro.org
---
 drivers/gpu/drm/bridge/aux-bridge.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/aux-bridge.c b/drivers/gpu/drm/bridge/aux-bridge.c
index b29980f95379ec7af873ed6e0fb79a9abb663c7b..295e9d031e2dc86cbfd2a7350718fca181c99487 100644
--- a/drivers/gpu/drm/bridge/aux-bridge.c
+++ b/drivers/gpu/drm/bridge/aux-bridge.c
@@ -58,9 +58,10 @@ int drm_aux_bridge_register(struct device *parent)
 	adev->id = ret;
 	adev->name = "aux_bridge";
 	adev->dev.parent = parent;
-	adev->dev.of_node = of_node_get(parent->of_node);
 	adev->dev.release = drm_aux_bridge_release;
 
+	device_set_of_node_from_dev(&adev->dev, parent);
+
 	ret = auxiliary_device_init(adev);
 	if (ret) {
 		ida_free(&drm_aux_bridge_ida, adev->id);

---
base-commit: d61a00525464bfc5fe92c6ad713350988e492b88
change-id: 20241017-drm-aux-bridge-mark-of-node-reused-5c2ee740ff19

Best regards,
-- 
Abel Vesa <abel.vesa@linaro.org>

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Dmitry Baryshkov]

On Fri, Oct 18, 2024 at 03:49:34PM +0300, Abel Vesa wrote:
> The assignment of the of_node to the aux bridge needs to mark the
> of_node as reused as well, otherwise resource providers like pinctrl will
> report a gpio as already requested by a different device when both pinconf
> and gpios property are present.
> Fix that by using the device_set_of_node_from_dev() helper instead.
> 
> Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers")
> Cc: stable@vger.kernel.org      # 6.8
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
> Changes in v2:
> - Re-worded commit to be more explicit of what it fixes, as Johan suggested
> - Used device_set_of_node_from_dev() helper, as per Johan's suggestion
> - Added Fixes tag and cc'ed stable
> - Link to v1: https://lore.kernel.org/r/20241017-drm-aux-bridge-mark-of-node-reused-v1-1-7cd5702bb4f2@linaro.org
> ---
>  drivers/gpu/drm/bridge/aux-bridge.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Neil Armstrong]

On 18/10/2024 14:49, Abel Vesa wrote:
> The assignment of the of_node to the aux bridge needs to mark the
> of_node as reused as well, otherwise resource providers like pinctrl will
> report a gpio as already requested by a different device when both pinconf
> and gpios property are present.
> Fix that by using the device_set_of_node_from_dev() helper instead.
> 
> Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers")
> Cc: stable@vger.kernel.org      # 6.8
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
> Changes in v2:
> - Re-worded commit to be more explicit of what it fixes, as Johan suggested
> - Used device_set_of_node_from_dev() helper, as per Johan's suggestion
> - Added Fixes tag and cc'ed stable
> - Link to v1: https://lore.kernel.org/r/20241017-drm-aux-bridge-mark-of-node-reused-v1-1-7cd5702bb4f2@linaro.org
> ---
>   drivers/gpu/drm/bridge/aux-bridge.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/aux-bridge.c b/drivers/gpu/drm/bridge/aux-bridge.c
> index b29980f95379ec7af873ed6e0fb79a9abb663c7b..295e9d031e2dc86cbfd2a7350718fca181c99487 100644
> --- a/drivers/gpu/drm/bridge/aux-bridge.c
> +++ b/drivers/gpu/drm/bridge/aux-bridge.c
> @@ -58,9 +58,10 @@ int drm_aux_bridge_register(struct device *parent)
>   	adev->id = ret;
>   	adev->name = "aux_bridge";
>   	adev->dev.parent = parent;
> -	adev->dev.of_node = of_node_get(parent->of_node);
>   	adev->dev.release = drm_aux_bridge_release;
>   
> +	device_set_of_node_from_dev(&adev->dev, parent);
> +
>   	ret = auxiliary_device_init(adev);
>   	if (ret) {
>   		ida_free(&drm_aux_bridge_ida, adev->id);
> 
> ---
> base-commit: d61a00525464bfc5fe92c6ad713350988e492b88
> change-id: 20241017-drm-aux-bridge-mark-of-node-reused-5c2ee740ff19
> 
> Best regards,

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Johan Hovold]

On Fri, Oct 18, 2024 at 03:49:34PM +0300, Abel Vesa wrote:
> The assignment of the of_node to the aux bridge needs to mark the
> of_node as reused as well, otherwise resource providers like pinctrl will
> report a gpio as already requested by a different device when both pinconf
> and gpios property are present.

I don't think you need a gpio property for that to happen, right? And
this causes probe to fail IIRC?

> Fix that by using the device_set_of_node_from_dev() helper instead.
> 
> Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers")

This is not the commit that introduced the issue.

> Cc: stable@vger.kernel.org      # 6.8

I assume there are no existing devicetrees that need this since then we
would have heard about it sooner. Do we still need to backport it?

When exactly are you hitting this?

> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
> Changes in v2:
> - Re-worded commit to be more explicit of what it fixes, as Johan suggested
> - Used device_set_of_node_from_dev() helper, as per Johan's suggestion
> - Added Fixes tag and cc'ed stable
> - Link to v1: https://lore.kernel.org/r/20241017-drm-aux-bridge-mark-of-node-reused-v1-1-7cd5702bb4f2@linaro.org

Patch itself looks good now.

Johan

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Neil Armstrong]

Hi,

On Fri, 18 Oct 2024 15:49:34 +0300, Abel Vesa wrote:
> The assignment of the of_node to the aux bridge needs to mark the
> of_node as reused as well, otherwise resource providers like pinctrl will
> report a gpio as already requested by a different device when both pinconf
> and gpios property are present.
> Fix that by using the device_set_of_node_from_dev() helper instead.
> 
> 
> [...]

Thanks, Applied to https://gitlab.freedesktop.org/drm/misc/kernel.git (drm-misc-fixes)

[1/1] drm/bridge: Fix assignment of the of_node of the parent to aux bridge
      https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/85e444a68126a631221ae32c63fce882bb18a262

-- 
Neil

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Sui Jingfeng]

Hi,

On 2024/10/21 21:08, Neil Armstrong wrote:
> Hi,
>
> On Fri, 18 Oct 2024 15:49:34 +0300, Abel Vesa wrote:
>> The assignment of the of_node to the aux bridge needs to mark the
>> of_node as reused as well, otherwise resource providers like pinctrl will
>> report a gpio as already requested by a different device when both pinconf
>> and gpios property are present.
>> Fix that by using the device_set_of_node_from_dev() helper instead.
>>
>>
>> [...]
> Thanks, Applied to https://gitlab.freedesktop.org/drm/misc/kernel.git (drm-misc-fixes)


It's quite impolite to force push patches that still under reviewing,
this prevent us to know what exactly its solves.

This also prevent us from finding a better solution.

> [1/1] drm/bridge: Fix assignment of the of_node of the parent to aux bridge
>        https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/85e444a68126a631221ae32c63fce882bb18a262
>
-- 
Best regards,
Sui

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Sui Jingfeng]

Hi,

On 2024/10/18 23:43, Dmitry Baryshkov wrote:
> On Fri, Oct 18, 2024 at 03:49:34PM +0300, Abel Vesa wrote:
>> The assignment of the of_node to the aux bridge needs to mark the
>> of_node as reused as well, otherwise resource providers like pinctrl will
>> report a gpio as already requested by a different device when both pinconf
>> and gpios property are present.
>> Fix that by using the device_set_of_node_from_dev() helper instead.
>>
>> Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers")
>> Cc: stable@vger.kernel.org      # 6.8
>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>> ---
>> Changes in v2:
>> - Re-worded commit to be more explicit of what it fixes, as Johan suggested
>> - Used device_set_of_node_from_dev() helper, as per Johan's suggestion
>> - Added Fixes tag and cc'ed stable
>> - Link to v1: https://lore.kernel.org/r/20241017-drm-aux-bridge-mark-of-node-reused-v1-1-7cd5702bb4f2@linaro.org
>> ---
>>   drivers/gpu/drm/bridge/aux-bridge.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


Technically speaking, your driver just move the burden to its caller.
Because this driver requires its user call drm_aux_bridge_register()
to create an AUX child device manually, you need it call ida_alloc()
to generate a unique id.

Functions symbols still have to leak to other subsystems, which is
not really preserve coding sharing.

What's worse, the action that allocating unique device id traditionally
is the duty of driver core. Why breaks (so called) perfect device driver
model by moving that out of core. Especially in the DT world that the
core knows very well how to populate device instance and manage the
reference counter.

HPD handling is traditionally belongs to connector, create standalone
driver like this one *abuse* to both Maxime's simple bridge driver and
Laurent's display-connector bridge driver or drm_bridge_connector or
whatever. Why those work can't satisfy you? At least, their drivers
are able to passing the mode setting states to the next bridge.

Basically those AUX drivers implementation abusing the definition of
bridge, abusing the definition of connector and abusing the DT.
Its just manually populate instances across drivers.

  

-- 
Best regards,
Sui

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Laurent Pinchart]

On Thu, Oct 31, 2024 at 12:45:24AM +0800, Sui Jingfeng wrote:
> Hi,
> 
> On 2024/10/18 23:43, Dmitry Baryshkov wrote:
> > On Fri, Oct 18, 2024 at 03:49:34PM +0300, Abel Vesa wrote:
> >> The assignment of the of_node to the aux bridge needs to mark the
> >> of_node as reused as well, otherwise resource providers like pinctrl will
> >> report a gpio as already requested by a different device when both pinconf
> >> and gpios property are present.
> >> Fix that by using the device_set_of_node_from_dev() helper instead.
> >>
> >> Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers")
> >> Cc: stable@vger.kernel.org      # 6.8
> >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> >> ---
> >> Changes in v2:
> >> - Re-worded commit to be more explicit of what it fixes, as Johan suggested
> >> - Used device_set_of_node_from_dev() helper, as per Johan's suggestion
> >> - Added Fixes tag and cc'ed stable
> >> - Link to v1: https://lore.kernel.org/r/20241017-drm-aux-bridge-mark-of-node-reused-v1-1-7cd5702bb4f2@linaro.org
> >> ---
> >>   drivers/gpu/drm/bridge/aux-bridge.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Technically speaking, your driver just move the burden to its caller.
> Because this driver requires its user call drm_aux_bridge_register()
> to create an AUX child device manually, you need it call ida_alloc()
> to generate a unique id.

There's a relevant discussion for a ti-sn65dsi86 patch, see
https://lore.kernel.org/r/20241030102846.GB14276@pendragon.ideasonboard.com

I agree it shouldn't be the responsibility of each caller to generate
unique IDs.

> Functions symbols still have to leak to other subsystems, which is
> not really preserve coding sharing.
> 
> What's worse, the action that allocating unique device id traditionally
> is the duty of driver core. Why breaks (so called) perfect device driver
> model by moving that out of core. Especially in the DT world that the
> core knows very well how to populate device instance and manage the
> reference counter.
> 
> HPD handling is traditionally belongs to connector, create standalone
> driver like this one *abuse* to both Maxime's simple bridge driver and
> Laurent's display-connector bridge driver or drm_bridge_connector or
> whatever. Why those work can't satisfy you? At least, their drivers
> are able to passing the mode setting states to the next bridge.
> 
> Basically those AUX drivers implementation abusing the definition of
> bridge, abusing the definition of connector and abusing the DT.
> Its just manually populate instances across drivers.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Neil Armstrong]

On 30/10/2024 17:45, Sui Jingfeng wrote:
> Hi,
> 
> On 2024/10/18 23:43, Dmitry Baryshkov wrote:
>> On Fri, Oct 18, 2024 at 03:49:34PM +0300, Abel Vesa wrote:
>>> The assignment of the of_node to the aux bridge needs to mark the
>>> of_node as reused as well, otherwise resource providers like pinctrl will
>>> report a gpio as already requested by a different device when both pinconf
>>> and gpios property are present.
>>> Fix that by using the device_set_of_node_from_dev() helper instead.
>>>
>>> Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers")
>>> Cc: stable@vger.kernel.org      # 6.8
>>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>>> ---
>>> Changes in v2:
>>> - Re-worded commit to be more explicit of what it fixes, as Johan suggested
>>> - Used device_set_of_node_from_dev() helper, as per Johan's suggestion
>>> - Added Fixes tag and cc'ed stable
>>> - Link to v1: https://lore.kernel.org/r/20241017-drm-aux-bridge-mark-of-node-reused-v1-1-7cd5702bb4f2@linaro.org
>>> ---
>>>   drivers/gpu/drm/bridge/aux-bridge.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> 
> Technically speaking, your driver just move the burden to its caller.
> Because this driver requires its user call drm_aux_bridge_register()
> to create an AUX child device manually, you need it call ida_alloc()
> to generate a unique id.
> 
> Functions symbols still have to leak to other subsystems, which is
> not really preserve coding sharing.

???

> 
> What's worse, the action that allocating unique device id traditionally
> is the duty of driver core. Why breaks (so called) perfect device driver
> model by moving that out of core. Especially in the DT world that the
> core knows very well how to populate device instance and manage the
> reference counter.

This has nothing to do with DT, auxiliary device is a nice way to actually
use the driver model to handle devices sub-functions without overloading
drivers. It's still young and we need to collectively solve some issues,
but it's now agreed auxiliary device helps designing multi-functions drivers.

> 
> HPD handling is traditionally belongs to connector, create standalone
> driver like this one *abuse* to both Maxime's simple bridge driver and
> Laurent's display-connector bridge driver or drm_bridge_connector or
> whatever. Why those work can't satisfy you? At least, their drivers
> are able to passing the mode setting states to the next bridge.

HPD handling is now shared along all the bridges, because it corresponds
to a reality.

It simply takes in account complex uses-cases like Type-C Altmode where
we need to describe the connection between the DP controller and the
Type-C retimers/muxes and properly propagate HPD events to synchronize
all the chain.

> 
> Basically those AUX drivers implementation abusing the definition of
> bridge, abusing the definition of connector and abusing the DT.
> Its just manually populate instances across drivers.

It abuses nothing, the DT representation of the full signal path
in the Type-C complex was required by DT bindings maintainers.

The fact we can describe an element of the Type-C Altmode DP
path is very handy, and we have the full control of the data
path unlike x86 platforms where all this handling is hidden in
closed firmwares.

If you have an issue with the aux-bridge design please open a separate
thread, because the actual patch has nothing to do with aux devices or DRM
bridge implementation.

Please do not respond to this thread except concerning this fix.

Neil

> 
> 
> 

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Neil Armstrong]

On 30/10/2024 15:49, Sui Jingfeng wrote:
> Hi,
> 
> On 2024/10/21 21:08, Neil Armstrong wrote:
>> Hi,
>>
>> On Fri, 18 Oct 2024 15:49:34 +0300, Abel Vesa wrote:
>>> The assignment of the of_node to the aux bridge needs to mark the
>>> of_node as reused as well, otherwise resource providers like pinctrl will
>>> report a gpio as already requested by a different device when both pinconf
>>> and gpios property are present.
>>> Fix that by using the device_set_of_node_from_dev() helper instead.
>>>
>>>
>>> [...]
>> Thanks, Applied to https://gitlab.freedesktop.org/drm/misc/kernel.git (drm-misc-fixes)
> 
> 
> It's quite impolite to force push patches that still under reviewing,
> this prevent us to know what exactly its solves.

It's quite explicit.

> 
> This also prevent us from finding a better solution.

Better solution of ? This needed to be fixed and backported to stable,
if there's desire to redesign the driver, then it should be discussed in a separate thread.

> 
>> [1/1] drm/bridge: Fix assignment of the of_node of the parent to aux bridge
>>        https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/85e444a68126a631221ae32c63fce882bb18a262
>>

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Johan Hovold]

On Thu, Oct 31, 2024 at 01:31:47PM +0100, Neil Armstrong wrote:
> On 30/10/2024 15:49, Sui Jingfeng wrote:
> > On 2024/10/21 21:08, Neil Armstrong wrote:
> >> On Fri, 18 Oct 2024 15:49:34 +0300, Abel Vesa wrote:
> >>> The assignment of the of_node to the aux bridge needs to mark the
> >>> of_node as reused as well, otherwise resource providers like pinctrl will
> >>> report a gpio as already requested by a different device when both pinconf
> >>> and gpios property are present.
> >>> Fix that by using the device_set_of_node_from_dev() helper instead.
> >>>
> >>>
> >>> [...]
> >> Thanks, Applied to https://gitlab.freedesktop.org/drm/misc/kernel.git (drm-misc-fixes)
> > 
> > 
> > It's quite impolite to force push patches that still under reviewing,
> > this prevent us to know what exactly its solves.
> 
> It's quite explicit.

It's still disrespectful and prevents reviewers' work from being
acknowledged as I told you off-list when you picked up the patch.

You said it would not happen again, and I had better things to do so I
let this one pass, but now it seems you insist that you did nothing
wrong here.

We do development in public and we should have had that discussion in
public, if only so that no one thinks I'm ok with this.

Johan

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Johan Hovold]

On Mon, Oct 21, 2024 at 09:23:24AM +0200, Johan Hovold wrote:
> On Fri, Oct 18, 2024 at 03:49:34PM +0300, Abel Vesa wrote:
> > The assignment of the of_node to the aux bridge needs to mark the
> > of_node as reused as well, otherwise resource providers like pinctrl will
> > report a gpio as already requested by a different device when both pinconf
> > and gpios property are present.
> 
> I don't think you need a gpio property for that to happen, right? And
> this causes probe to fail IIRC?
> 
> > Fix that by using the device_set_of_node_from_dev() helper instead.
> > 
> > Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers")
> 
> This is not the commit that introduced the issue.
> 
> > Cc: stable@vger.kernel.org      # 6.8
> 
> I assume there are no existing devicetrees that need this since then we
> would have heard about it sooner. Do we still need to backport it?
> 
> When exactly are you hitting this?

Abel, even if Neil decided to give me the finger here, please answer the
above so that it's recorded in the archives at least.

Johan

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Sui Jingfeng]

Hi, Dears maintainers

On 2024/10/31 20:31, Neil Armstrong wrote:
> On 30/10/2024 15:49, Sui Jingfeng wrote:
>> Hi,
>>
>> On 2024/10/21 21:08, Neil Armstrong wrote:
>>> Hi,
>>>
>>> On Fri, 18 Oct 2024 15:49:34 +0300, Abel Vesa wrote:
>>>> The assignment of the of_node to the aux bridge needs to mark the
>>>> of_node as reused as well, otherwise resource providers like 
>>>> pinctrl will
>>>> report a gpio as already requested by a different device when both 
>>>> pinconf
>>>> and gpios property are present.
>>>> Fix that by using the device_set_of_node_from_dev() helper instead.
>>>>
>>>>
>>>> [...]
>>> Thanks, Applied to 
>>> https://gitlab.freedesktop.org/drm/misc/kernel.git (drm-misc-fixes)
>>
>>
>> It's quite impolite to force push patches that still under reviewing,
>> this prevent us to know what exactly its solves.
>
> It's quite explicit.
>
>>
>> This also prevent us from finding a better solution.
>
> Better solution of ? This needed to be fixed and backported to stable,

We were thinking about

1) if possible to add a proper DT binding for those drives.

Or alternatively, as Laurent pointed out that

2) Invent some extra techniques to move the idr allocation
    procedure back to the AUX bus core. Make the core maintained
    device ID happens can help to reduce some boilerplate.

And those really deserve yet an another deeper thinking? no?
    

> if there's desire to redesign the driver, then it should be discussed 
> in a separate thread.
>
No, please don't misunderstanding. We are admire your work
and we both admit that this patch is a valid fix.

But I think Johan do need more times to understand what exactly
the real problem is. We do need times to investigate new method.
This bug can be a good chance to verify/test new ideas,
at the least, allow us to talk and to discussion.


>>
>>> [1/1] drm/bridge: Fix assignment of the of_node of the parent to aux 
>>> bridge
>>> https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/85e444a68126a631221ae32c63fce882bb18a262
>>>
>
-- 
Best regards,
Sui

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Sui Jingfeng]

Hi,

On 2024/10/31 22:02, Johan Hovold wrote:
> On Thu, Oct 31, 2024 at 01:31:47PM +0100, Neil Armstrong wrote:
>> On 30/10/2024 15:49, Sui Jingfeng wrote:
>>> On 2024/10/21 21:08, Neil Armstrong wrote:
>>>> On Fri, 18 Oct 2024 15:49:34 +0300, Abel Vesa wrote:
>>>>> The assignment of the of_node to the aux bridge needs to mark the
>>>>> of_node as reused as well, otherwise resource providers like pinctrl will
>>>>> report a gpio as already requested by a different device when both pinconf
>>>>> and gpios property are present.
>>>>> Fix that by using the device_set_of_node_from_dev() helper instead.
>>>>>
>>>>>
>>>>> [...]
>>>> Thanks, Applied to https://gitlab.freedesktop.org/drm/misc/kernel.git (drm-misc-fixes)
>>>
>>> It's quite impolite to force push patches that still under reviewing,
>>> this prevent us to know what exactly its solves.
>> It's quite explicit.
> It's still disrespectful and prevents reviewers' work from being
> acknowledged as I told you off-list when you picked up the patch.
>
> You said it would not happen again, and I had better things to do so I
> let this one pass, but now it seems you insist that you did nothing
> wrong here.
>
> We do development in public and we should have had that discussion in
> public, if only so that no one thinks I'm ok with this.


Yeah, extremely correct, Johan!

While I am really don't know why a child device have to
share the referencing of the OF device node with its parent device?
Is possible to pass a child device node via the platform data to reference?

I means that, in DT systems, the child device can easily
have(find) its own device node to attached.
I'm imagining that it probably should be belong to the USB
connector device node or something like that.

Sorry, I'm confused. I understand that you also might be busy.
I think I probably should go back alone to think for a while.


> Johan

-- 
Best regards,
Sui

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Dmitry Baryshkov]

On Mon, 21 Oct 2024 at 10:23, Johan Hovold <johan@kernel.org> wrote:
>
> On Fri, Oct 18, 2024 at 03:49:34PM +0300, Abel Vesa wrote:
> > The assignment of the of_node to the aux bridge needs to mark the
> > of_node as reused as well, otherwise resource providers like pinctrl will
> > report a gpio as already requested by a different device when both pinconf
> > and gpios property are present.
>
> I don't think you need a gpio property for that to happen, right? And
> this causes probe to fail IIRC?

No, just having a pinctrl property in the bridge device is enough.
Without this fix when the aux subdevice is being bound to the driver,
the pinctrl_bind_pins() will attempt to bind pins, which are already
in use by the actual bridge device.

>
> > Fix that by using the device_set_of_node_from_dev() helper instead.
> >
> > Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers")
>
> This is not the commit that introduced the issue.
>
> > Cc: stable@vger.kernel.org      # 6.8
>
> I assume there are no existing devicetrees that need this since then we
> would have heard about it sooner. Do we still need to backport it?
>
> When exactly are you hitting this?
>
> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> > Changes in v2:
> > - Re-worded commit to be more explicit of what it fixes, as Johan suggested
> > - Used device_set_of_node_from_dev() helper, as per Johan's suggestion
> > - Added Fixes tag and cc'ed stable
> > - Link to v1: https://lore.kernel.org/r/20241017-drm-aux-bridge-mark-of-node-reused-v1-1-7cd5702bb4f2@linaro.org
>
> Patch itself looks good now.
>
> Johan



-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Abel Vesa]

On 24-10-31 15:05:52, Johan Hovold wrote:
> On Mon, Oct 21, 2024 at 09:23:24AM +0200, Johan Hovold wrote:
> > On Fri, Oct 18, 2024 at 03:49:34PM +0300, Abel Vesa wrote:
> > > The assignment of the of_node to the aux bridge needs to mark the
> > > of_node as reused as well, otherwise resource providers like pinctrl will
> > > report a gpio as already requested by a different device when both pinconf
> > > and gpios property are present.
> > 
> > I don't think you need a gpio property for that to happen, right? And
> > this causes probe to fail IIRC?

Yes, I think this is actually because of the pinctrl property in the node,
so no gpio needed.

Yes, probe fails.

> > 
> > > Fix that by using the device_set_of_node_from_dev() helper instead.
> > > 
> > > Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers")
> > 
> > This is not the commit that introduced the issue.

The proper fixes tag here is actually:

Fixes: 2a04739139b2 ("drm/bridge: add transparent bridge helper")

> > 
> > > Cc: stable@vger.kernel.org      # 6.8

> > 
> > I assume there are no existing devicetrees that need this since then we
> > would have heard about it sooner. Do we still need to backport it?

None of the DTs I managed to scan seem to have this problem.

Maybe backporting it is not worth it then.

> > 
> > When exactly are you hitting this?

Here is one of the examples.

[    5.768283] x1e80100-tlmm f100000.pinctrl: error -EINVAL: pin-185 (aux_bridge.aux_bridge.3)
[    5.768289] x1e80100-tlmm f100000.pinctrl: error -EINVAL: could not request pin 185 (GPIO_185) from group gpio185 on device f100000.pinctrl
[    5.768293] aux_bridge.aux_bridge aux_bridge.aux_bridge.3: Error applying setting, reverse things back

> 
> Abel, even if Neil decided to give me the finger here, please answer the
> above so that it's recorded in the archives at least.
> 
> Johan
> 

Sorry for not replying in time before the patch was merge.

Abel.

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Johan Hovold]

On Thu, Oct 31, 2024 at 11:06:38PM +0800, Sui Jingfeng wrote:

> But I think Johan do need more times to understand what exactly
> the real problem is. We do need times to investigate new method.

No, I know perfectly well what the (immediate) problem is here (I was
the one adding support for the of_node_reused flag some years back).

I just wanted to make sure that the commit message was correct and
complete before merging (and also to figure out whether this particular
patch needed to be backported).

Johan

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Johan Hovold]

On Thu, Oct 31, 2024 at 05:48:24PM +0200, Dmitry Baryshkov wrote:
> On Mon, 21 Oct 2024 at 10:23, Johan Hovold <johan@kernel.org> wrote:
> >
> > On Fri, Oct 18, 2024 at 03:49:34PM +0300, Abel Vesa wrote:
> > > The assignment of the of_node to the aux bridge needs to mark the
> > > of_node as reused as well, otherwise resource providers like pinctrl will
> > > report a gpio as already requested by a different device when both pinconf
> > > and gpios property are present.
> >
> > I don't think you need a gpio property for that to happen, right? And
> > this causes probe to fail IIRC?
> 
> No, just having a pinctrl property in the bridge device is enough.
> Without this fix when the aux subdevice is being bound to the driver,
> the pinctrl_bind_pins() will attempt to bind pins, which are already
> in use by the actual bridge device.

Right, and IIRC it then fails to probe as well. This is information that
should have been in the commit message.

Johan

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Johan Hovold]

On Thu, Oct 31, 2024 at 06:13:45PM +0200, Abel Vesa wrote:
> On 24-10-31 15:05:52, Johan Hovold wrote:
> > On Mon, Oct 21, 2024 at 09:23:24AM +0200, Johan Hovold wrote:
> > > On Fri, Oct 18, 2024 at 03:49:34PM +0300, Abel Vesa wrote:

> > > > Cc: stable@vger.kernel.org      # 6.8
> 
> > > I assume there are no existing devicetrees that need this since then we
> > > would have heard about it sooner. Do we still need to backport it?
> 
> None of the DTs I managed to scan seem to have this problem.
> 
> Maybe backporting it is not worth it then.

Thanks for confirming. Which (new) driver and DT are you seeing this
with?

> > > When exactly are you hitting this?
> 
> Here is one of the examples.
> 
> [    5.768283] x1e80100-tlmm f100000.pinctrl: error -EINVAL: pin-185 (aux_bridge.aux_bridge.3)
> [    5.768289] x1e80100-tlmm f100000.pinctrl: error -EINVAL: could not request pin 185 (GPIO_185) from group gpio185 on device f100000.pinctrl
> [    5.768293] aux_bridge.aux_bridge aux_bridge.aux_bridge.3: Error applying setting, reverse things back

I meant with which driver and DT you hit this with.

> > Abel, even if Neil decided to give me the finger here, please answer the
> > above so that it's recorded in the archives at least.

> Sorry for not replying in time before the patch was merge.

That's not your fault.

Johan

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Sui Jingfeng]

On 2024/11/1 00:23, Johan Hovold wrote:
> On Thu, Oct 31, 2024 at 11:06:38PM +0800, Sui Jingfeng wrote:
>
>> But I think Johan do need more times to understand what exactly
>> the real problem is. We do need times to investigate new method.
> No, I know perfectly well what the (immediate) problem is here (I was
> the one adding support for the of_node_reused flag some years back).
>
> I just wanted to make sure that the commit message was correct and
> complete before merging (and also to figure out whether this particular
> patch needed to be backported).


Well under such a design, having the child device sharing the 'OF' device
node with it parent device means that one parent device can *only*
create one AUX bridge child device.

Since If you create two or more child AUX bridge, *all* of them will
call devm_drm_of_get_bridge(&auxdev->dev, auxdev->dev.of_node, 0, 0),
then we will *contend* the same next bridge resource.

Because of the 'auxdev->dev.of_node' is same for all its instance.
While other display bridges seems don't has such limitations.


> Johan

-- 
Best regards,
Sui

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Sui Jingfeng]

On 2024/10/31 20:31, Neil Armstrong wrote:
> On 30/10/2024 15:49, Sui Jingfeng wrote:
>> Hi,
>>
>> On 2024/10/21 21:08, Neil Armstrong wrote:
>>> Hi,
>>>
>>> On Fri, 18 Oct 2024 15:49:34 +0300, Abel Vesa wrote:
>>>> The assignment of the of_node to the aux bridge needs to mark the
>>>> of_node as reused as well, otherwise resource providers like 
>>>> pinctrl will
>>>> report a gpio as already requested by a different device when both 
>>>> pinconf
>>>> and gpios property are present.
>>>> Fix that by using the device_set_of_node_from_dev() helper instead.
>>>>
>>>>
>>>> [...]
>>> Thanks, Applied to 
>>> https://gitlab.freedesktop.org/drm/misc/kernel.git (drm-misc-fixes)
>>
>>
>> It's quite impolite to force push patches that still under reviewing,
>> this prevent us to know what exactly its solves.
>
> It's quite explicit.
>

Auxiliary bus emphasis on *compartmentalize*, layer, and distribute
domain-specific concerns via *Linux device-driver model*.

Reusing(or sharing) of_node by multiple devices proved that the two
subsystems are still tangled together somehow. Which is fundamentally
violate the philosophy of compartmentalization.

The way that driver operated is not via Linux device-driver model either,
lots of those kind things happens quite implicitly.

But I think beautiful things associated behind this might also be voided,
that's it.


>>
>> This also prevent us from finding a better solution.
>
> Better solution of ? This needed to be fixed and backported to stable,
> if there's desire to redesign the driver, then it should be discussed 
> in a separate thread.
>
>>
>>> [1/1] drm/bridge: Fix assignment of the of_node of the parent to aux 
>>> bridge
>>> https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/85e444a68126a631221ae32c63fce882bb18a262
>>>
>
-- 
Best regards,
Sui

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Johan Hovold]

On Fri, Nov 01, 2024 at 11:49:07AM +0800, Sui Jingfeng wrote:
> On 2024/11/1 00:23, Johan Hovold wrote:
> > On Thu, Oct 31, 2024 at 11:06:38PM +0800, Sui Jingfeng wrote:
> >
> >> But I think Johan do need more times to understand what exactly
> >> the real problem is. We do need times to investigate new method.

> > No, I know perfectly well what the (immediate) problem is here (I was
> > the one adding support for the of_node_reused flag some years back).
> >
> > I just wanted to make sure that the commit message was correct and
> > complete before merging (and also to figure out whether this particular
> > patch needed to be backported).
> 
> Well under such a design, having the child device sharing the 'OF' device
> node with it parent device means that one parent device can *only*
> create one AUX bridge child device.
> 
> Since If you create two or more child AUX bridge, *all* of them will
> call devm_drm_of_get_bridge(&auxdev->dev, auxdev->dev.of_node, 0, 0),
> then we will *contend* the same next bridge resource.
> 
> Because of the 'auxdev->dev.of_node' is same for all its instance.
> While other display bridges seems don't has such limitations.

Oh, I'm not saying that there cannot be further issues with the design
or implementation here. And perhaps fixing those would make the
immediate issue Abel was trying to address go away.

Johan

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Laurent Pinchart]

On Fri, Nov 01, 2024 at 11:49:07AM +0800, Sui Jingfeng wrote:
> 
> On 2024/11/1 00:23, Johan Hovold wrote:
> > On Thu, Oct 31, 2024 at 11:06:38PM +0800, Sui Jingfeng wrote:
> >
> >> But I think Johan do need more times to understand what exactly
> >> the real problem is. We do need times to investigate new method.
> > No, I know perfectly well what the (immediate) problem is here (I was
> > the one adding support for the of_node_reused flag some years back).
> >
> > I just wanted to make sure that the commit message was correct and
> > complete before merging (and also to figure out whether this particular
> > patch needed to be backported).
> 
> Well under such a design, having the child device sharing the 'OF' device
> node with it parent device means that one parent device can *only*
> create one AUX bridge child device.
> 
> Since If you create two or more child AUX bridge, *all* of them will
> call devm_drm_of_get_bridge(&auxdev->dev, auxdev->dev.of_node, 0, 0),
> then we will *contend* the same next bridge resource.
> 
> Because of the 'auxdev->dev.of_node' is same for all its instance.
> While other display bridges seems don't has such limitations.

Brainstorming a bit, I wonder if we could create a swnode for the
auxiliary device, instead of reusing the parent's OF node. This would
require switching the DRM OF-based APIs to fwnode, but that's easy and
mostly a mechanical change.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Abel Vesa]

On 24-10-31 17:33:35, Johan Hovold wrote:
> On Thu, Oct 31, 2024 at 06:13:45PM +0200, Abel Vesa wrote:
> > On 24-10-31 15:05:52, Johan Hovold wrote:
> > > On Mon, Oct 21, 2024 at 09:23:24AM +0200, Johan Hovold wrote:
> > > > On Fri, Oct 18, 2024 at 03:49:34PM +0300, Abel Vesa wrote:
> 
> > > > > Cc: stable@vger.kernel.org      # 6.8
> > 
> > > > I assume there are no existing devicetrees that need this since then we
> > > > would have heard about it sooner. Do we still need to backport it?
> > 
> > None of the DTs I managed to scan seem to have this problem.
> > 
> > Maybe backporting it is not worth it then.
> 
> Thanks for confirming. Which (new) driver and DT are you seeing this
> with?

The Parade PS8830 retimer and its DT node. The v3 of that patchset
will not trigger it unless the pinctrl properties are being added to the
retimer node.

> 
> > > > When exactly are you hitting this?
> > 
> > Here is one of the examples.
> > 
> > [    5.768283] x1e80100-tlmm f100000.pinctrl: error -EINVAL: pin-185 (aux_bridge.aux_bridge.3)
> > [    5.768289] x1e80100-tlmm f100000.pinctrl: error -EINVAL: could not request pin 185 (GPIO_185) from group gpio185 on device f100000.pinctrl
> > [    5.768293] aux_bridge.aux_bridge aux_bridge.aux_bridge.3: Error applying setting, reverse things back
> 
> I meant with which driver and DT you hit this with.
> 
> > > Abel, even if Neil decided to give me the finger here, please answer the
> > > above so that it's recorded in the archives at least.
> 
> > Sorry for not replying in time before the patch was merge.
> 
> That's not your fault.
> 
> Johan
> 

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Dmitry Baryshkov]

On Fri, 1 Nov 2024 at 11:20, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Fri, Nov 01, 2024 at 11:49:07AM +0800, Sui Jingfeng wrote:
> >
> > On 2024/11/1 00:23, Johan Hovold wrote:
> > > On Thu, Oct 31, 2024 at 11:06:38PM +0800, Sui Jingfeng wrote:
> > >
> > >> But I think Johan do need more times to understand what exactly
> > >> the real problem is. We do need times to investigate new method.
> > > No, I know perfectly well what the (immediate) problem is here (I was
> > > the one adding support for the of_node_reused flag some years back).
> > >
> > > I just wanted to make sure that the commit message was correct and
> > > complete before merging (and also to figure out whether this particular
> > > patch needed to be backported).
> >
> > Well under such a design, having the child device sharing the 'OF' device
> > node with it parent device means that one parent device can *only*
> > create one AUX bridge child device.
> >
> > Since If you create two or more child AUX bridge, *all* of them will
> > call devm_drm_of_get_bridge(&auxdev->dev, auxdev->dev.of_node, 0, 0),
> > then we will *contend* the same next bridge resource.
> >
> > Because of the 'auxdev->dev.of_node' is same for all its instance.
> > While other display bridges seems don't has such limitations.
>
> Brainstorming a bit, I wonder if we could create a swnode for the
> auxiliary device, instead of reusing the parent's OF node.

This will break bridge lookup which is performed by following the OF
graph links. So the aux bridges should use corresponding of_node or
fwnode.

> This would
> require switching the DRM OF-based APIs to fwnode, but that's easy and
> mostly a mechanical change.

-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge

[Laurent Pinchart]

On Fri, Nov 01, 2024 at 12:27:15PM +0200, Dmitry Baryshkov wrote:
> On Fri, 1 Nov 2024 at 11:20, Laurent Pinchart wrote:
> > On Fri, Nov 01, 2024 at 11:49:07AM +0800, Sui Jingfeng wrote:
> > > On 2024/11/1 00:23, Johan Hovold wrote:
> > > > On Thu, Oct 31, 2024 at 11:06:38PM +0800, Sui Jingfeng wrote:
> > > >
> > > >> But I think Johan do need more times to understand what exactly
> > > >> the real problem is. We do need times to investigate new method.
> > > > No, I know perfectly well what the (immediate) problem is here (I was
> > > > the one adding support for the of_node_reused flag some years back).
> > > >
> > > > I just wanted to make sure that the commit message was correct and
> > > > complete before merging (and also to figure out whether this particular
> > > > patch needed to be backported).
> > >
> > > Well under such a design, having the child device sharing the 'OF' device
> > > node with it parent device means that one parent device can *only*
> > > create one AUX bridge child device.
> > >
> > > Since If you create two or more child AUX bridge, *all* of them will
> > > call devm_drm_of_get_bridge(&auxdev->dev, auxdev->dev.of_node, 0, 0),
> > > then we will *contend* the same next bridge resource.
> > >
> > > Because of the 'auxdev->dev.of_node' is same for all its instance.
> > > While other display bridges seems don't has such limitations.
> >
> > Brainstorming a bit, I wonder if we could create a swnode for the
> > auxiliary device, instead of reusing the parent's OF node.
> 
> This will break bridge lookup which is performed by following the OF
> graph links. So the aux bridges should use corresponding of_node or
> fwnode.

We can also expand the lookup infrastructure and handle more platform
integration and driver architecture options. I'm not sure how it would
look like, but all these are in-kernel APIs, so they can be extended and
modified if needed.

> > This would
> > require switching the DRM OF-based APIs to fwnode, but that's easy and
> > mostly a mechanical change.

-- 
Regards,

Laurent Pinchart