| Message ID | 20241018-drm-aux-bridge-mark-of-node-reused-v2-1-aeed1b445c7d@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/bridge: Fix assignment of the of_node of the parent to aux bridge | expand |
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>
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>
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
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
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 >
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.
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.
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 > > >
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 >>
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
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
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 >>> >
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
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
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.
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
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
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
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
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 >>> >
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
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.
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 >
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.
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.
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);
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(-) --- base-commit: d61a00525464bfc5fe92c6ad713350988e492b88 change-id: 20241017-drm-aux-bridge-mark-of-node-reused-5c2ee740ff19 Best regards,