Device-managed resources allocated post component bind must be tied to
the lifetime of the aggregate DRM device or they will not necessarily be
released when binding of the aggregate device is deferred.
This can lead resource leaks or failure to bind the aggregate device
when binding is later retried and a second attempt to allocate the
resources is made.
For the DP aux-bus, an attempt to populate the bus a second time will
simply fail ("DP AUX EP device already populated").
Fix this by amending the DP aux interface and tying the lifetime of the
EP device to the DRM device rather than DP controller platform device.
Fixes: c3bf8e21b38a ("drm/msm/dp: Add eDP support via aux_bus")
Cc: [email protected] # 5.19
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/gpu/drm/bridge/parade-ps8640.c | 2 +-
drivers/gpu/drm/display/drm_dp_aux_bus.c | 5 +++--
drivers/gpu/drm/msm/dp/dp_display.c | 3 ++-
include/drm/display/drm_dp_aux_bus.h | 6 +++---
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index d7483c13c569..6127979370cb 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -719,7 +719,7 @@ static int ps8640_probe(struct i2c_client *client)
if (ret)
return ret;
- ret = devm_of_dp_aux_populate_bus(&ps_bridge->aux, ps8640_bridge_link_panel);
+ ret = devm_of_dp_aux_populate_bus(dev, &ps_bridge->aux, ps8640_bridge_link_panel);
/*
* If devm_of_dp_aux_populate_bus() returns -ENODEV then it's up to
diff --git a/drivers/gpu/drm/display/drm_dp_aux_bus.c b/drivers/gpu/drm/display/drm_dp_aux_bus.c
index f5741b45ca07..2706f2cf82f7 100644
--- a/drivers/gpu/drm/display/drm_dp_aux_bus.c
+++ b/drivers/gpu/drm/display/drm_dp_aux_bus.c
@@ -322,6 +322,7 @@ static void of_dp_aux_depopulate_bus_void(void *data)
/**
* devm_of_dp_aux_populate_bus() - devm wrapper for of_dp_aux_populate_bus()
+ * @dev: Device to tie the lifetime of the EP devices to
* @aux: The AUX channel whose device we want to populate
* @done_probing: Callback functions to call after EP device finishes probing.
* Will not be called if there are no EP devices and this
@@ -333,7 +334,7 @@ static void of_dp_aux_depopulate_bus_void(void *data)
* no children. The done_probing() function won't be called in that
* case.
*/
-int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
+int devm_of_dp_aux_populate_bus(struct device *dev, struct drm_dp_aux *aux,
int (*done_probing)(struct drm_dp_aux *aux))
{
int ret;
@@ -342,7 +343,7 @@ int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
if (ret)
return ret;
- return devm_add_action_or_reset(aux->dev,
+ return devm_add_action_or_reset(dev,
of_dp_aux_depopulate_bus_void, aux);
}
EXPORT_SYMBOL_GPL(devm_of_dp_aux_populate_bus);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index ba557328710a..e1aa6355bbf6 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1559,7 +1559,8 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
* panel driver is probed asynchronously but is the best we
* can do without a bigger driver reorganization.
*/
- rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
+ rc = devm_of_dp_aux_populate_ep_devices(dp->drm_dev->dev,
+ dp_priv->aux);
of_node_put(aux_bus);
if (rc)
goto error;
diff --git a/include/drm/display/drm_dp_aux_bus.h b/include/drm/display/drm_dp_aux_bus.h
index 8a0a486383c5..a4063aa7fc40 100644
--- a/include/drm/display/drm_dp_aux_bus.h
+++ b/include/drm/display/drm_dp_aux_bus.h
@@ -47,7 +47,7 @@ static inline struct dp_aux_ep_driver *to_dp_aux_ep_drv(struct device_driver *dr
int of_dp_aux_populate_bus(struct drm_dp_aux *aux,
int (*done_probing)(struct drm_dp_aux *aux));
void of_dp_aux_depopulate_bus(struct drm_dp_aux *aux);
-int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
+int devm_of_dp_aux_populate_bus(struct device *dev, struct drm_dp_aux *aux,
int (*done_probing)(struct drm_dp_aux *aux));
/* Deprecated versions of the above functions. To be removed when no callers. */
@@ -61,11 +61,11 @@ static inline int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
return (ret != -ENODEV) ? ret : 0;
}
-static inline int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
+static inline int devm_of_dp_aux_populate_ep_devices(struct device *dev, struct drm_dp_aux *aux)
{
int ret;
- ret = devm_of_dp_aux_populate_bus(aux, NULL);
+ ret = devm_of_dp_aux_populate_bus(dev, aux, NULL);
/* New API returns -ENODEV for no child case; adapt to old assumption */
return (ret != -ENODEV) ? ret : 0;
--
2.35.1
On 12/09/2022 18:40, Johan Hovold wrote:
> Device-managed resources allocated post component bind must be tied to
> the lifetime of the aggregate DRM device or they will not necessarily be
> released when binding of the aggregate device is deferred.
>
> This can lead resource leaks or failure to bind the aggregate device
> when binding is later retried and a second attempt to allocate the
> resources is made.
>
> For the DP aux-bus, an attempt to populate the bus a second time will
> simply fail ("DP AUX EP device already populated").
>
> Fix this by amending the DP aux interface and tying the lifetime of the
> EP device to the DRM device rather than DP controller platform device.
Doug, could you please take a look?
For me this is another reminder/pressure point that we should populate
the AUX BUS from the probe(), before binding the components together.
>
> Fixes: c3bf8e21b38a ("drm/msm/dp: Add eDP support via aux_bus")
> Cc: [email protected] # 5.19
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/gpu/drm/bridge/parade-ps8640.c | 2 +-
> drivers/gpu/drm/display/drm_dp_aux_bus.c | 5 +++--
> drivers/gpu/drm/msm/dp/dp_display.c | 3 ++-
> include/drm/display/drm_dp_aux_bus.h | 6 +++---
> 4 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index d7483c13c569..6127979370cb 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -719,7 +719,7 @@ static int ps8640_probe(struct i2c_client *client)
> if (ret)
> return ret;
>
> - ret = devm_of_dp_aux_populate_bus(&ps_bridge->aux, ps8640_bridge_link_panel);
> + ret = devm_of_dp_aux_populate_bus(dev, &ps_bridge->aux, ps8640_bridge_link_panel);
>
> /*
> * If devm_of_dp_aux_populate_bus() returns -ENODEV then it's up to
> diff --git a/drivers/gpu/drm/display/drm_dp_aux_bus.c b/drivers/gpu/drm/display/drm_dp_aux_bus.c
> index f5741b45ca07..2706f2cf82f7 100644
> --- a/drivers/gpu/drm/display/drm_dp_aux_bus.c
> +++ b/drivers/gpu/drm/display/drm_dp_aux_bus.c
> @@ -322,6 +322,7 @@ static void of_dp_aux_depopulate_bus_void(void *data)
>
> /**
> * devm_of_dp_aux_populate_bus() - devm wrapper for of_dp_aux_populate_bus()
> + * @dev: Device to tie the lifetime of the EP devices to
> * @aux: The AUX channel whose device we want to populate
> * @done_probing: Callback functions to call after EP device finishes probing.
> * Will not be called if there are no EP devices and this
> @@ -333,7 +334,7 @@ static void of_dp_aux_depopulate_bus_void(void *data)
> * no children. The done_probing() function won't be called in that
> * case.
> */
> -int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
> +int devm_of_dp_aux_populate_bus(struct device *dev, struct drm_dp_aux *aux,
> int (*done_probing)(struct drm_dp_aux *aux))
> {
> int ret;
> @@ -342,7 +343,7 @@ int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
> if (ret)
> return ret;
>
> - return devm_add_action_or_reset(aux->dev,
> + return devm_add_action_or_reset(dev,
> of_dp_aux_depopulate_bus_void, aux);
> }
> EXPORT_SYMBOL_GPL(devm_of_dp_aux_populate_bus);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index ba557328710a..e1aa6355bbf6 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1559,7 +1559,8 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
> * panel driver is probed asynchronously but is the best we
> * can do without a bigger driver reorganization.
> */
> - rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
> + rc = devm_of_dp_aux_populate_ep_devices(dp->drm_dev->dev,
> + dp_priv->aux);
> of_node_put(aux_bus);
> if (rc)
> goto error;
> diff --git a/include/drm/display/drm_dp_aux_bus.h b/include/drm/display/drm_dp_aux_bus.h
> index 8a0a486383c5..a4063aa7fc40 100644
> --- a/include/drm/display/drm_dp_aux_bus.h
> +++ b/include/drm/display/drm_dp_aux_bus.h
> @@ -47,7 +47,7 @@ static inline struct dp_aux_ep_driver *to_dp_aux_ep_drv(struct device_driver *dr
> int of_dp_aux_populate_bus(struct drm_dp_aux *aux,
> int (*done_probing)(struct drm_dp_aux *aux));
> void of_dp_aux_depopulate_bus(struct drm_dp_aux *aux);
> -int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
> +int devm_of_dp_aux_populate_bus(struct device *dev, struct drm_dp_aux *aux,
> int (*done_probing)(struct drm_dp_aux *aux));
>
> /* Deprecated versions of the above functions. To be removed when no callers. */
> @@ -61,11 +61,11 @@ static inline int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
> return (ret != -ENODEV) ? ret : 0;
> }
>
> -static inline int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
> +static inline int devm_of_dp_aux_populate_ep_devices(struct device *dev, struct drm_dp_aux *aux)
> {
> int ret;
>
> - ret = devm_of_dp_aux_populate_bus(aux, NULL);
> + ret = devm_of_dp_aux_populate_bus(dev, aux, NULL);
>
> /* New API returns -ENODEV for no child case; adapt to old assumption */
> return (ret != -ENODEV) ? ret : 0;
--
With best wishes
Dmitry
On 9/12/22 1:10 PM, Dmitry Baryshkov wrote:
> On 12/09/2022 18:40, Johan Hovold wrote:
>> Device-managed resources allocated post component bind must be tied to
>> the lifetime of the aggregate DRM device or they will not necessarily be
>> released when binding of the aggregate device is deferred.
>>
>> This can lead resource leaks or failure to bind the aggregate device
>> when binding is later retried and a second attempt to allocate the
>> resources is made.
>>
>> For the DP aux-bus, an attempt to populate the bus a second time will
>> simply fail ("DP AUX EP device already populated").
>>
>> Fix this by amending the DP aux interface and tying the lifetime of the
>> EP device to the DRM device rather than DP controller platform device.
>
> Doug, could you please take a look?
>
> For me this is another reminder/pressure point that we should populate
> the AUX BUS from the probe(), before binding the components together.
>
>>
>> Fixes: c3bf8e21b38a ("drm/msm/dp: Add eDP support via aux_bus")
>> Cc: [email protected] # 5.19
>> Signed-off-by: Johan Hovold <[email protected]>
>> ---
>> drivers/gpu/drm/bridge/parade-ps8640.c | 2 +-
>> drivers/gpu/drm/display/drm_dp_aux_bus.c | 5 +++--
>> drivers/gpu/drm/msm/dp/dp_display.c | 3 ++-
>> include/drm/display/drm_dp_aux_bus.h | 6 +++---
>> 4 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c
>> b/drivers/gpu/drm/bridge/parade-ps8640.c
>> index d7483c13c569..6127979370cb 100644
>> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
>> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
>> @@ -719,7 +719,7 @@ static int ps8640_probe(struct i2c_client *client)
>> if (ret)
>> return ret;
>> - ret = devm_of_dp_aux_populate_bus(&ps_bridge->aux,
>> ps8640_bridge_link_panel);
>> + ret = devm_of_dp_aux_populate_bus(dev, &ps_bridge->aux,
>> ps8640_bridge_link_panel);
>> /*
>> * If devm_of_dp_aux_populate_bus() returns -ENODEV then it's
>> up to
>> diff --git a/drivers/gpu/drm/display/drm_dp_aux_bus.c
>> b/drivers/gpu/drm/display/drm_dp_aux_bus.c
>> index f5741b45ca07..2706f2cf82f7 100644
>> --- a/drivers/gpu/drm/display/drm_dp_aux_bus.c
>> +++ b/drivers/gpu/drm/display/drm_dp_aux_bus.c
>> @@ -322,6 +322,7 @@ static void of_dp_aux_depopulate_bus_void(void
>> *data)
>> /**
>> * devm_of_dp_aux_populate_bus() - devm wrapper for
>> of_dp_aux_populate_bus()
>> + * @dev: Device to tie the lifetime of the EP devices to
>> * @aux: The AUX channel whose device we want to populate
>> * @done_probing: Callback functions to call after EP device
>> finishes probing.
>> * Will not be called if there are no EP devices and
>> this
>> @@ -333,7 +334,7 @@ static void of_dp_aux_depopulate_bus_void(void
>> *data)
>> * no children. The done_probing() function won't be called
>> in that
>> * case.
>> */
>> -int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
>> +int devm_of_dp_aux_populate_bus(struct device *dev, struct
>> drm_dp_aux *aux,
>> int (*done_probing)(struct drm_dp_aux *aux))
>> {
>> int ret;
>> @@ -342,7 +343,7 @@ int devm_of_dp_aux_populate_bus(struct drm_dp_aux
>> *aux,
>> if (ret)
>> return ret;
>> - return devm_add_action_or_reset(aux->dev,
>> + return devm_add_action_or_reset(dev,
>> of_dp_aux_depopulate_bus_void, aux);
>> }
>> EXPORT_SYMBOL_GPL(devm_of_dp_aux_populate_bus);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index ba557328710a..e1aa6355bbf6 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1559,7 +1559,8 @@ static int dp_display_get_next_bridge(struct
>> msm_dp *dp)
>> * panel driver is probed asynchronously but is the best we
>> * can do without a bigger driver reorganization.
>> */
>> - rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
>> + rc = devm_of_dp_aux_populate_ep_devices(dp->drm_dev->dev,
>> + dp_priv->aux);
>> of_node_put(aux_bus);
>> if (rc)
>> goto error;
>> diff --git a/include/drm/display/drm_dp_aux_bus.h
>> b/include/drm/display/drm_dp_aux_bus.h
>> index 8a0a486383c5..a4063aa7fc40 100644
>> --- a/include/drm/display/drm_dp_aux_bus.h
>> +++ b/include/drm/display/drm_dp_aux_bus.h
>> @@ -47,7 +47,7 @@ static inline struct dp_aux_ep_driver
>> *to_dp_aux_ep_drv(struct device_driver *dr
>> int of_dp_aux_populate_bus(struct drm_dp_aux *aux,
>> int (*done_probing)(struct drm_dp_aux *aux));
>> void of_dp_aux_depopulate_bus(struct drm_dp_aux *aux);
>> -int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
>> +int devm_of_dp_aux_populate_bus(struct device *dev, struct
>> drm_dp_aux *aux,
>> int (*done_probing)(struct drm_dp_aux *aux));
>> /* Deprecated versions of the above functions. To be removed when
>> no callers. */
>> @@ -61,11 +61,11 @@ static inline int
>> of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
>> return (ret != -ENODEV) ? ret : 0;
>> }
>> -static inline int devm_of_dp_aux_populate_ep_devices(struct
>> drm_dp_aux *aux)
>> +static inline int devm_of_dp_aux_populate_ep_devices(struct device
>> *dev, struct drm_dp_aux *aux)
>> {
>> int ret;
>> - ret = devm_of_dp_aux_populate_bus(aux, NULL);
>> + ret = devm_of_dp_aux_populate_bus(dev, aux, NULL);
>> /* New API returns -ENODEV for no child case; adapt to old
>> assumption */
>> return (ret != -ENODEV) ? ret : 0;
>
This breaks builds which have ti-sn65dsi86 included:
drivers/gpu/drm/bridge/ti-sn65dsi86.c:628:50: error: passing argument 1
of 'devm_of_dp_aux_populate_ep_devices' from incompatible argument type.
As well,
drivers/gpu/drm/bridge/ti-sn65dsi86.c:628:15: error: too few arguments
to function 'devm_of_dp_aux_populate_ep_devices'
--steev
Hi Johan,
I love your patch! Yet something to improve:
[auto build test ERROR on next-20220912]
[also build test ERROR on v6.0-rc5]
[cannot apply to drm-misc/drm-misc-next drm/drm-next drm-intel/for-linux-next drm-tip/drm-tip linus/master v6.0-rc5 v6.0-rc4 v6.0-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Johan-Hovold/drm-msm-probe-deferral-fixes/20220912-234351
base: 044b771be9c5de9d817dfafb829d2f049c71c3b4
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220913/[email protected]/config)
compiler: arceb-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/458c96e19570036b3dd6e48d91f0bf6f67b996fa
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Johan-Hovold/drm-msm-probe-deferral-fixes/20220912-234351
git checkout 458c96e19570036b3dd6e48d91f0bf6f67b996fa
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/gpu/drm/bridge/ti-sn65dsi86.c: In function 'ti_sn_aux_probe':
>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:632:50: error: passing argument 1 of 'devm_of_dp_aux_populate_ep_devices' from incompatible pointer type [-Werror=incompatible-pointer-types]
632 | ret = devm_of_dp_aux_populate_ep_devices(&pdata->aux);
| ^~~~~~~~~~~
| |
| struct drm_dp_aux *
In file included from drivers/gpu/drm/bridge/ti-sn65dsi86.c:26:
include/drm/display/drm_dp_aux_bus.h:64:69: note: expected 'struct device *' but argument is of type 'struct drm_dp_aux *'
64 | static inline int devm_of_dp_aux_populate_ep_devices(struct device *dev, struct drm_dp_aux *aux)
| ~~~~~~~~~~~~~~~^~~
>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:632:15: error: too few arguments to function 'devm_of_dp_aux_populate_ep_devices'
632 | ret = devm_of_dp_aux_populate_ep_devices(&pdata->aux);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/drm/display/drm_dp_aux_bus.h:64:19: note: declared here
64 | static inline int devm_of_dp_aux_populate_ep_devices(struct device *dev, struct drm_dp_aux *aux)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
--
drivers/gpu/drm/bridge/analogix/anx7625.c: In function 'anx7625_i2c_probe':
>> drivers/gpu/drm/bridge/analogix/anx7625.c:2654:44: error: passing argument 1 of 'devm_of_dp_aux_populate_ep_devices' from incompatible pointer type [-Werror=incompatible-pointer-types]
2654 | devm_of_dp_aux_populate_ep_devices(&platform->aux);
| ^~~~~~~~~~~~~~
| |
| struct drm_dp_aux *
In file included from drivers/gpu/drm/bridge/analogix/anx7625.c:24:
include/drm/display/drm_dp_aux_bus.h:64:69: note: expected 'struct device *' but argument is of type 'struct drm_dp_aux *'
64 | static inline int devm_of_dp_aux_populate_ep_devices(struct device *dev, struct drm_dp_aux *aux)
| ~~~~~~~~~~~~~~~^~~
>> drivers/gpu/drm/bridge/analogix/anx7625.c:2654:9: error: too few arguments to function 'devm_of_dp_aux_populate_ep_devices'
2654 | devm_of_dp_aux_populate_ep_devices(&platform->aux);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/drm/display/drm_dp_aux_bus.h:64:19: note: declared here
64 | static inline int devm_of_dp_aux_populate_ep_devices(struct device *dev, struct drm_dp_aux *aux)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/devm_of_dp_aux_populate_ep_devices +632 drivers/gpu/drm/bridge/ti-sn65dsi86.c
77674e722f4b27 Laurent Pinchart 2021-06-24 620
77674e722f4b27 Laurent Pinchart 2021-06-24 621 static int ti_sn_aux_probe(struct auxiliary_device *adev,
77674e722f4b27 Laurent Pinchart 2021-06-24 622 const struct auxiliary_device_id *id)
77674e722f4b27 Laurent Pinchart 2021-06-24 623 {
77674e722f4b27 Laurent Pinchart 2021-06-24 624 struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
77674e722f4b27 Laurent Pinchart 2021-06-24 625 int ret;
77674e722f4b27 Laurent Pinchart 2021-06-24 626
77674e722f4b27 Laurent Pinchart 2021-06-24 627 pdata->aux.name = "ti-sn65dsi86-aux";
77674e722f4b27 Laurent Pinchart 2021-06-24 628 pdata->aux.dev = &adev->dev;
77674e722f4b27 Laurent Pinchart 2021-06-24 629 pdata->aux.transfer = ti_sn_aux_transfer;
77674e722f4b27 Laurent Pinchart 2021-06-24 630 drm_dp_aux_init(&pdata->aux);
77674e722f4b27 Laurent Pinchart 2021-06-24 631
77674e722f4b27 Laurent Pinchart 2021-06-24 @632 ret = devm_of_dp_aux_populate_ep_devices(&pdata->aux);
77674e722f4b27 Laurent Pinchart 2021-06-24 633 if (ret)
77674e722f4b27 Laurent Pinchart 2021-06-24 634 return ret;
77674e722f4b27 Laurent Pinchart 2021-06-24 635
77674e722f4b27 Laurent Pinchart 2021-06-24 636 /*
77674e722f4b27 Laurent Pinchart 2021-06-24 637 * The eDP to MIPI bridge parts don't work until the AUX channel is
77674e722f4b27 Laurent Pinchart 2021-06-24 638 * setup so we don't add it in the main driver probe, we add it now.
77674e722f4b27 Laurent Pinchart 2021-06-24 639 */
77674e722f4b27 Laurent Pinchart 2021-06-24 640 return ti_sn65dsi86_add_aux_device(pdata, &pdata->bridge_aux, "bridge");
77674e722f4b27 Laurent Pinchart 2021-06-24 641 }
77674e722f4b27 Laurent Pinchart 2021-06-24 642
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Tue, Sep 13, 2022 at 07:35:15AM +0100, Doug Anderson wrote:
> Hi,
>
> On Mon, Sep 12, 2022 at 7:10 PM Dmitry Baryshkov
> <[email protected]> wrote:
> >
> > On 12/09/2022 18:40, Johan Hovold wrote:
> > > Device-managed resources allocated post component bind must be tied to
> > > the lifetime of the aggregate DRM device or they will not necessarily be
> > > released when binding of the aggregate device is deferred.
> > >
> > > This can lead resource leaks or failure to bind the aggregate device
> > > when binding is later retried and a second attempt to allocate the
> > > resources is made.
> > >
> > > For the DP aux-bus, an attempt to populate the bus a second time will
> > > simply fail ("DP AUX EP device already populated").
> > >
> > > Fix this by amending the DP aux interface and tying the lifetime of the
> > > EP device to the DRM device rather than DP controller platform device.
> >
> > Doug, could you please take a look?
> >
> > For me this is another reminder/pressure point that we should populate
> > the AUX BUS from the probe(), before binding the components together.
>
> Aside from the kernel robot complaints, I'm not necessarily convinced.
> I think we know that the AUX DP stuff in MSM-DP is fragile right now
> and Qualcomm has promised to clean it up. This really feels like a
> band-aid and is really a sign that we're populating the AUX DP bus in
> the wrong place in Qualcomm's code. As you said, if we moved this to
> probe(), which is the plan in the promised cleanup, then it wouldn't
> be a problem.
Right, but that appears to be non-trivial judging from the discussions
you had back when the offending patch was merged:
https://lore.kernel.org/lkml/CAD=FV=X+QvjwoT2zGP82KW4kD0oMUY6ZgCizSikNX_Uj8dNDqA@mail.gmail.com/t/#u
> As far as I know Qualcomm has queued this cleanup behind their current
> PSR work (though it's never been clear why both can't be worked on at
> the same time) and the PSR work was stalled because they couldn't
> figure out what caused the glitching I reported. It's still on my nag
> list that I bring up with them every week...
>
> In any case, if a band-aid is urgent, maybe you could just call
> of_dp_aux_populate_bus() directly in Qualcomm code and you could add
> your own devm_add_action_or_reset() on the DRM device.
Yeah, that's probably better. I apparently missed a bunch of users of
devm_of_dp_aux_populate_ep_devices() after searching for
devm_of_dp_aux_populate_bus() instead. Judging from a quick glance all
of these populate the bus at probe, so Qualcomm indeed appears to be
the odd bird here.
But the bug is real, in mainline and needs to be fixed, so rolling a
custom devm action indeed should to be the right thing to do here (if
only to have a smaller fix).
Johan
Hi,
On Mon, Sep 12, 2022 at 7:10 PM Dmitry Baryshkov
<[email protected]> wrote:
>
> On 12/09/2022 18:40, Johan Hovold wrote:
> > Device-managed resources allocated post component bind must be tied to
> > the lifetime of the aggregate DRM device or they will not necessarily be
> > released when binding of the aggregate device is deferred.
> >
> > This can lead resource leaks or failure to bind the aggregate device
> > when binding is later retried and a second attempt to allocate the
> > resources is made.
> >
> > For the DP aux-bus, an attempt to populate the bus a second time will
> > simply fail ("DP AUX EP device already populated").
> >
> > Fix this by amending the DP aux interface and tying the lifetime of the
> > EP device to the DRM device rather than DP controller platform device.
>
> Doug, could you please take a look?
>
> For me this is another reminder/pressure point that we should populate
> the AUX BUS from the probe(), before binding the components together.
Aside from the kernel robot complaints, I'm not necessarily convinced.
I think we know that the AUX DP stuff in MSM-DP is fragile right now
and Qualcomm has promised to clean it up. This really feels like a
band-aid and is really a sign that we're populating the AUX DP bus in
the wrong place in Qualcomm's code. As you said, if we moved this to
probe(), which is the plan in the promised cleanup, then it wouldn't
be a problem.
As far as I know Qualcomm has queued this cleanup behind their current
PSR work (though it's never been clear why both can't be worked on at
the same time) and the PSR work was stalled because they couldn't
figure out what caused the glitching I reported. It's still on my nag
list that I bring up with them every week...
In any case, if a band-aid is urgent, maybe you could just call
of_dp_aux_populate_bus() directly in Qualcomm code and you could add
your own devm_add_action_or_reset() on the DRM device.
On Mon, Sep 12, 2022 at 04:55:58PM -0500, Steev Klimaszewski wrote:
>
> On 9/12/22 1:10 PM, Dmitry Baryshkov wrote:
> > On 12/09/2022 18:40, Johan Hovold wrote:
> >> Device-managed resources allocated post component bind must be tied to
> >> the lifetime of the aggregate DRM device or they will not necessarily be
> >> released when binding of the aggregate device is deferred.
> >>
> >> This can lead resource leaks or failure to bind the aggregate device
> >> when binding is later retried and a second attempt to allocate the
> >> resources is made.
> >>
> >> For the DP aux-bus, an attempt to populate the bus a second time will
> >> simply fail ("DP AUX EP device already populated").
> >>
> >> Fix this by amending the DP aux interface and tying the lifetime of the
> >> EP device to the DRM device rather than DP controller platform device.
> >
> > Doug, could you please take a look?
> >
> > For me this is another reminder/pressure point that we should populate
> > the AUX BUS from the probe(), before binding the components together.
> >
> >>
> >> Fixes: c3bf8e21b38a ("drm/msm/dp: Add eDP support via aux_bus")
> >> Cc: [email protected] # 5.19
> >> Signed-off-by: Johan Hovold <[email protected]>
> >> ---
> >> drivers/gpu/drm/bridge/parade-ps8640.c | 2 +-
> >> drivers/gpu/drm/display/drm_dp_aux_bus.c | 5 +++--
> >> drivers/gpu/drm/msm/dp/dp_display.c | 3 ++-
> >> include/drm/display/drm_dp_aux_bus.h | 6 +++---
> >> 4 files changed, 9 insertions(+), 7 deletions(-)
> This breaks builds which have ti-sn65dsi86 included:
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c:628:50: error: passing argument 1
> of 'devm_of_dp_aux_populate_ep_devices' from incompatible argument type.
>
> As well,
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c:628:15: error: too few arguments
> to function 'devm_of_dp_aux_populate_ep_devices'
Thanks for reporting this. I messed up and apparently only grepped for
devm_of_dp_aux_populate_bus() and not the
devm_of_dp_aux_populate_ep_devices() wrapper when searching for users.
Johan