Re: [Nouveau] [PATCH v2 16/27] drm/dp_mst: Refactor pdt setup/teardown, add more locking
On Wed, Sep 25, 2019 at 05:00:00PM -0400, Lyude Paul wrote:
> On Wed, 2019-09-25 at 15:27 -0400, Sean Paul wrote:
> > On Tue, Sep 03, 2019 at 04:45:54PM -0400, Lyude Paul wrote:
> > > Since we're going to be implementing suspend/resume reprobing very soon,
> > > we need to make sure we are extra careful to ensure that our locking
> > > actually protects the topology state where we expect it to. Turns out
> > > this isn't the case with drm_dp_port_setup_pdt() and
> > > drm_dp_port_teardown_pdt(), both of which change port->mstb without
> > > grabbing >lock.
> > >
> > > Additionally, since most callers of these functions are just using it to
> > > teardown the port's previous PDT and setup a new one we can simplify
> > > things a bit and combine drm_dp_port_setup_pdt() and
> > > drm_dp_port_teardown_pdt() into a single function:
> > > drm_dp_port_set_pdt(). This function also handles actually ensuring that
> > > we grab the correct locks when we need to modify port->mstb.
> > >
> > > Cc: Juston Li
> > > Cc: Imre Deak
> > > Cc: Ville Syrjälä
> > > Cc: Harry Wentland
> > > Cc: Daniel Vetter
> > > Signed-off-by: Lyude Paul
> > > ---
> > > drivers/gpu/drm/drm_dp_mst_topology.c | 181 +++---
> > > include/drm/drm_dp_mst_helper.h | 6 +-
> > > 2 files changed, 110 insertions(+), 77 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index d1610434a0cb..9944ef2ce885 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -1487,24 +1487,6 @@ drm_dp_mst_topology_put_mstb(struct
> > > drm_dp_mst_branch *mstb)
> > > kref_put(>topology_kref, drm_dp_destroy_mst_branch_device);
> > > }
> > >
> > > -static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int
> > > old_pdt)
> > > -{
> > > - struct drm_dp_mst_branch *mstb;
> > > -
> > > - switch (old_pdt) {
> > > - case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > > - case DP_PEER_DEVICE_SST_SINK:
> > > - /* remove i2c over sideband */
> > > - drm_dp_mst_unregister_i2c_bus(>aux);
> > > - break;
> > > - case DP_PEER_DEVICE_MST_BRANCHING:
> > > - mstb = port->mstb;
> > > - port->mstb = NULL;
> > > - drm_dp_mst_topology_put_mstb(mstb);
> > > - break;
> > > - }
> > > -}
> > > -
> > > static void drm_dp_destroy_port(struct kref *kref)
> > > {
> > > struct drm_dp_mst_port *port =
> > > @@ -1714,38 +1696,79 @@ static u8 drm_dp_calculate_rad(struct
> > > drm_dp_mst_port *port,
> > > return parent_lct + 1;
> > > }
> > >
> > > -/*
> > > - * return sends link address for new mstb
> > > - */
> > > -static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port)
> > > +static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt)
> > > {
> > > - int ret;
> > > - u8 rad[6], lct;
> > > - bool send_link = false;
> > > + struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> > > + struct drm_dp_mst_branch *mstb;
> > > + u8 rad[8], lct;
> > > + int ret = 0;
> > > +
> > > + if (port->pdt == new_pdt)
> >
> > Shouldn't we also ensure that access to port->pdt is also locked?
> >
>
> It's specifically port->mstb that needs to be protected under lock. We don't
> use port->pdt for traversing the topology at all, so keeping it under
> connection_mutex is sufficient.
>
I hadn't gotten to the connection_mutex patch yet when I made that comment :)
Reviewed-by: Sean Paul
> > Sean
> >
> > > + return 0;
> > > +
> > > + /* Teardown the old pdt, if there is one */
> > > + switch (port->pdt) {
> > > + case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > > + case DP_PEER_DEVICE_SST_SINK:
> > > + /*
> > > + * If the new PDT would also have an i2c bus, don't bother
> > > + * with reregistering it
> > > + */
> > > + if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> > > + new_pdt == DP_PEER_DEVICE_SST_SINK) {
> > > + port->pdt = new_pdt;
> > > + return 0;
> > > + }
> > > +
> > > + /* remove i2c over sideband */
> > > + drm_dp_mst_unregister_i2c_bus(>aux);
> > > + break;
> > > + case DP_PEER_DEVICE_MST_BRANCHING:
> > > + mutex_lock(>lock);
> > > + drm_dp_mst_topology_put_mstb(port->mstb);
> > > + port->mstb = NULL;
> > > + mutex_unlock(>lock);
> > > + break;
> > > + }
> > > +
> > > + port->pdt = new_pdt;
> > > switch (port->pdt) {
> > > case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > > case DP_PEER_DEVICE_SST_SINK:
> > > /* add i2c over sideband */
> > > ret = drm_dp_mst_register_i2c_bus(>aux);
> > > break;
> > > +
> > > case DP_PEER_DEVICE_MST_BRANCHING:
> > > lct = drm_dp_calculate_rad(port, rad);
> > > + mstb = drm_dp_add_mst_branch_device(lct, rad);
> > > + if (!mstb) {
> > > + ret = -ENOMEM;
> > > +
Re: [Nouveau] [PATCH v2 16/27] drm/dp_mst: Refactor pdt setup/teardown, add more locking
On Wed, 2019-09-25 at 15:27 -0400, Sean Paul wrote:
> On Tue, Sep 03, 2019 at 04:45:54PM -0400, Lyude Paul wrote:
> > Since we're going to be implementing suspend/resume reprobing very soon,
> > we need to make sure we are extra careful to ensure that our locking
> > actually protects the topology state where we expect it to. Turns out
> > this isn't the case with drm_dp_port_setup_pdt() and
> > drm_dp_port_teardown_pdt(), both of which change port->mstb without
> > grabbing >lock.
> >
> > Additionally, since most callers of these functions are just using it to
> > teardown the port's previous PDT and setup a new one we can simplify
> > things a bit and combine drm_dp_port_setup_pdt() and
> > drm_dp_port_teardown_pdt() into a single function:
> > drm_dp_port_set_pdt(). This function also handles actually ensuring that
> > we grab the correct locks when we need to modify port->mstb.
> >
> > Cc: Juston Li
> > Cc: Imre Deak
> > Cc: Ville Syrjälä
> > Cc: Harry Wentland
> > Cc: Daniel Vetter
> > Signed-off-by: Lyude Paul
> > ---
> > drivers/gpu/drm/drm_dp_mst_topology.c | 181 +++---
> > include/drm/drm_dp_mst_helper.h | 6 +-
> > 2 files changed, 110 insertions(+), 77 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index d1610434a0cb..9944ef2ce885 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -1487,24 +1487,6 @@ drm_dp_mst_topology_put_mstb(struct
> > drm_dp_mst_branch *mstb)
> > kref_put(>topology_kref, drm_dp_destroy_mst_branch_device);
> > }
> >
> > -static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int
> > old_pdt)
> > -{
> > - struct drm_dp_mst_branch *mstb;
> > -
> > - switch (old_pdt) {
> > - case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > - case DP_PEER_DEVICE_SST_SINK:
> > - /* remove i2c over sideband */
> > - drm_dp_mst_unregister_i2c_bus(>aux);
> > - break;
> > - case DP_PEER_DEVICE_MST_BRANCHING:
> > - mstb = port->mstb;
> > - port->mstb = NULL;
> > - drm_dp_mst_topology_put_mstb(mstb);
> > - break;
> > - }
> > -}
> > -
> > static void drm_dp_destroy_port(struct kref *kref)
> > {
> > struct drm_dp_mst_port *port =
> > @@ -1714,38 +1696,79 @@ static u8 drm_dp_calculate_rad(struct
> > drm_dp_mst_port *port,
> > return parent_lct + 1;
> > }
> >
> > -/*
> > - * return sends link address for new mstb
> > - */
> > -static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port)
> > +static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt)
> > {
> > - int ret;
> > - u8 rad[6], lct;
> > - bool send_link = false;
> > + struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> > + struct drm_dp_mst_branch *mstb;
> > + u8 rad[8], lct;
> > + int ret = 0;
> > +
> > + if (port->pdt == new_pdt)
>
> Shouldn't we also ensure that access to port->pdt is also locked?
>
It's specifically port->mstb that needs to be protected under lock. We don't
use port->pdt for traversing the topology at all, so keeping it under
connection_mutex is sufficient.
> Sean
>
> > + return 0;
> > +
> > + /* Teardown the old pdt, if there is one */
> > + switch (port->pdt) {
> > + case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > + case DP_PEER_DEVICE_SST_SINK:
> > + /*
> > +* If the new PDT would also have an i2c bus, don't bother
> > +* with reregistering it
> > +*/
> > + if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> > + new_pdt == DP_PEER_DEVICE_SST_SINK) {
> > + port->pdt = new_pdt;
> > + return 0;
> > + }
> > +
> > + /* remove i2c over sideband */
> > + drm_dp_mst_unregister_i2c_bus(>aux);
> > + break;
> > + case DP_PEER_DEVICE_MST_BRANCHING:
> > + mutex_lock(>lock);
> > + drm_dp_mst_topology_put_mstb(port->mstb);
> > + port->mstb = NULL;
> > + mutex_unlock(>lock);
> > + break;
> > + }
> > +
> > + port->pdt = new_pdt;
> > switch (port->pdt) {
> > case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > case DP_PEER_DEVICE_SST_SINK:
> > /* add i2c over sideband */
> > ret = drm_dp_mst_register_i2c_bus(>aux);
> > break;
> > +
> > case DP_PEER_DEVICE_MST_BRANCHING:
> > lct = drm_dp_calculate_rad(port, rad);
> > + mstb = drm_dp_add_mst_branch_device(lct, rad);
> > + if (!mstb) {
> > + ret = -ENOMEM;
> > + DRM_ERROR("Failed to create MSTB for port %p", port);
> > + goto out;
> > + }
> >
> > - port->mstb = drm_dp_add_mst_branch_device(lct, rad);
> > - if (port->mstb) {
> > - port->mstb->mgr = port->mgr;
> > - port->mstb->port_parent =
Re: [Nouveau] [PATCH v2 16/27] drm/dp_mst: Refactor pdt setup/teardown, add more locking
On Tue, Sep 03, 2019 at 04:45:54PM -0400, Lyude Paul wrote:
> Since we're going to be implementing suspend/resume reprobing very soon,
> we need to make sure we are extra careful to ensure that our locking
> actually protects the topology state where we expect it to. Turns out
> this isn't the case with drm_dp_port_setup_pdt() and
> drm_dp_port_teardown_pdt(), both of which change port->mstb without
> grabbing >lock.
>
> Additionally, since most callers of these functions are just using it to
> teardown the port's previous PDT and setup a new one we can simplify
> things a bit and combine drm_dp_port_setup_pdt() and
> drm_dp_port_teardown_pdt() into a single function:
> drm_dp_port_set_pdt(). This function also handles actually ensuring that
> we grab the correct locks when we need to modify port->mstb.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 181 +++---
> include/drm/drm_dp_mst_helper.h | 6 +-
> 2 files changed, 110 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index d1610434a0cb..9944ef2ce885 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1487,24 +1487,6 @@ drm_dp_mst_topology_put_mstb(struct drm_dp_mst_branch
> *mstb)
> kref_put(>topology_kref, drm_dp_destroy_mst_branch_device);
> }
>
> -static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int
> old_pdt)
> -{
> - struct drm_dp_mst_branch *mstb;
> -
> - switch (old_pdt) {
> - case DP_PEER_DEVICE_DP_LEGACY_CONV:
> - case DP_PEER_DEVICE_SST_SINK:
> - /* remove i2c over sideband */
> - drm_dp_mst_unregister_i2c_bus(>aux);
> - break;
> - case DP_PEER_DEVICE_MST_BRANCHING:
> - mstb = port->mstb;
> - port->mstb = NULL;
> - drm_dp_mst_topology_put_mstb(mstb);
> - break;
> - }
> -}
> -
> static void drm_dp_destroy_port(struct kref *kref)
> {
> struct drm_dp_mst_port *port =
> @@ -1714,38 +1696,79 @@ static u8 drm_dp_calculate_rad(struct drm_dp_mst_port
> *port,
> return parent_lct + 1;
> }
>
> -/*
> - * return sends link address for new mstb
> - */
> -static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port)
> +static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt)
> {
> - int ret;
> - u8 rad[6], lct;
> - bool send_link = false;
> + struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> + struct drm_dp_mst_branch *mstb;
> + u8 rad[8], lct;
> + int ret = 0;
> +
> + if (port->pdt == new_pdt)
Shouldn't we also ensure that access to port->pdt is also locked?
Sean
> + return 0;
> +
> + /* Teardown the old pdt, if there is one */
> + switch (port->pdt) {
> + case DP_PEER_DEVICE_DP_LEGACY_CONV:
> + case DP_PEER_DEVICE_SST_SINK:
> + /*
> + * If the new PDT would also have an i2c bus, don't bother
> + * with reregistering it
> + */
> + if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> + new_pdt == DP_PEER_DEVICE_SST_SINK) {
> + port->pdt = new_pdt;
> + return 0;
> + }
> +
> + /* remove i2c over sideband */
> + drm_dp_mst_unregister_i2c_bus(>aux);
> + break;
> + case DP_PEER_DEVICE_MST_BRANCHING:
> + mutex_lock(>lock);
> + drm_dp_mst_topology_put_mstb(port->mstb);
> + port->mstb = NULL;
> + mutex_unlock(>lock);
> + break;
> + }
> +
> + port->pdt = new_pdt;
> switch (port->pdt) {
> case DP_PEER_DEVICE_DP_LEGACY_CONV:
> case DP_PEER_DEVICE_SST_SINK:
> /* add i2c over sideband */
> ret = drm_dp_mst_register_i2c_bus(>aux);
> break;
> +
> case DP_PEER_DEVICE_MST_BRANCHING:
> lct = drm_dp_calculate_rad(port, rad);
> + mstb = drm_dp_add_mst_branch_device(lct, rad);
> + if (!mstb) {
> + ret = -ENOMEM;
> + DRM_ERROR("Failed to create MSTB for port %p", port);
> + goto out;
> + }
>
> - port->mstb = drm_dp_add_mst_branch_device(lct, rad);
> - if (port->mstb) {
> - port->mstb->mgr = port->mgr;
> - port->mstb->port_parent = port;
> - /*
> - * Make sure this port's memory allocation stays
> - * around until its child MSTB releases it
> - */
> - drm_dp_mst_get_port_malloc(port);
> + mutex_lock(>lock);
> + port->mstb = mstb;
> + mstb->mgr =
[Nouveau] [PATCH v2 16/27] drm/dp_mst: Refactor pdt setup/teardown, add more locking
Since we're going to be implementing suspend/resume reprobing very soon,
we need to make sure we are extra careful to ensure that our locking
actually protects the topology state where we expect it to. Turns out
this isn't the case with drm_dp_port_setup_pdt() and
drm_dp_port_teardown_pdt(), both of which change port->mstb without
grabbing >lock.
Additionally, since most callers of these functions are just using it to
teardown the port's previous PDT and setup a new one we can simplify
things a bit and combine drm_dp_port_setup_pdt() and
drm_dp_port_teardown_pdt() into a single function:
drm_dp_port_set_pdt(). This function also handles actually ensuring that
we grab the correct locks when we need to modify port->mstb.
Cc: Juston Li
Cc: Imre Deak
Cc: Ville Syrjälä
Cc: Harry Wentland
Cc: Daniel Vetter
Signed-off-by: Lyude Paul
---
drivers/gpu/drm/drm_dp_mst_topology.c | 181 +++---
include/drm/drm_dp_mst_helper.h | 6 +-
2 files changed, 110 insertions(+), 77 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index d1610434a0cb..9944ef2ce885 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1487,24 +1487,6 @@ drm_dp_mst_topology_put_mstb(struct drm_dp_mst_branch
*mstb)
kref_put(>topology_kref, drm_dp_destroy_mst_branch_device);
}
-static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int old_pdt)
-{
- struct drm_dp_mst_branch *mstb;
-
- switch (old_pdt) {
- case DP_PEER_DEVICE_DP_LEGACY_CONV:
- case DP_PEER_DEVICE_SST_SINK:
- /* remove i2c over sideband */
- drm_dp_mst_unregister_i2c_bus(>aux);
- break;
- case DP_PEER_DEVICE_MST_BRANCHING:
- mstb = port->mstb;
- port->mstb = NULL;
- drm_dp_mst_topology_put_mstb(mstb);
- break;
- }
-}
-
static void drm_dp_destroy_port(struct kref *kref)
{
struct drm_dp_mst_port *port =
@@ -1714,38 +1696,79 @@ static u8 drm_dp_calculate_rad(struct drm_dp_mst_port
*port,
return parent_lct + 1;
}
-/*
- * return sends link address for new mstb
- */
-static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port)
+static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt)
{
- int ret;
- u8 rad[6], lct;
- bool send_link = false;
+ struct drm_dp_mst_topology_mgr *mgr = port->mgr;
+ struct drm_dp_mst_branch *mstb;
+ u8 rad[8], lct;
+ int ret = 0;
+
+ if (port->pdt == new_pdt)
+ return 0;
+
+ /* Teardown the old pdt, if there is one */
+ switch (port->pdt) {
+ case DP_PEER_DEVICE_DP_LEGACY_CONV:
+ case DP_PEER_DEVICE_SST_SINK:
+ /*
+* If the new PDT would also have an i2c bus, don't bother
+* with reregistering it
+*/
+ if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
+ new_pdt == DP_PEER_DEVICE_SST_SINK) {
+ port->pdt = new_pdt;
+ return 0;
+ }
+
+ /* remove i2c over sideband */
+ drm_dp_mst_unregister_i2c_bus(>aux);
+ break;
+ case DP_PEER_DEVICE_MST_BRANCHING:
+ mutex_lock(>lock);
+ drm_dp_mst_topology_put_mstb(port->mstb);
+ port->mstb = NULL;
+ mutex_unlock(>lock);
+ break;
+ }
+
+ port->pdt = new_pdt;
switch (port->pdt) {
case DP_PEER_DEVICE_DP_LEGACY_CONV:
case DP_PEER_DEVICE_SST_SINK:
/* add i2c over sideband */
ret = drm_dp_mst_register_i2c_bus(>aux);
break;
+
case DP_PEER_DEVICE_MST_BRANCHING:
lct = drm_dp_calculate_rad(port, rad);
+ mstb = drm_dp_add_mst_branch_device(lct, rad);
+ if (!mstb) {
+ ret = -ENOMEM;
+ DRM_ERROR("Failed to create MSTB for port %p", port);
+ goto out;
+ }
- port->mstb = drm_dp_add_mst_branch_device(lct, rad);
- if (port->mstb) {
- port->mstb->mgr = port->mgr;
- port->mstb->port_parent = port;
- /*
-* Make sure this port's memory allocation stays
-* around until its child MSTB releases it
-*/
- drm_dp_mst_get_port_malloc(port);
+ mutex_lock(>lock);
+ port->mstb = mstb;
+ mstb->mgr = port->mgr;
+ mstb->port_parent = port;
- send_link = true;
- }
+ /*
+* Make sure this port's memory allocation stays
+* around until its child MSTB
