Re: [PATCH 4/7] drm/msm/dp: fix aux-bus EP lifetime

Dmitry Baryshkov
2022-09-12T18:10:17Z

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: stable@vger.kernel.org # 5.19
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
> 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