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

Steev Klimaszewski <steev@xxxxxxxx> · Mon, 12 Sep 2022 16:55:58 -0500

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: stable@xxxxxxxxxxxxxxx      # 5.19
Signed-off-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
---
  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








