Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <a4a9ec24a6d4a646e22fb5c63e285d4d1bb50a0d.camel@redhat.com>
Subject: Re: [PATCH v3 3/4] drm_dp_mst_topology: export two functions
From:   Lyude Paul <lyude@redhat.com>
Reply-To: lyude@redhat.com
To:     Sam McNally <sammc@chromium.org>,
        LKML <linux-kernel@vger.kernel.org>
Cc:     Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@linux.ie>,
        Hans Verkuil <hverkuil@xs4all.nl>,
        Hans Verkuil <hans.verkuil@cisco.com>,
        dri-devel@lists.freedesktop.org
Date:   Mon, 01 Feb 2021 17:02:55 -0500
In-Reply-To: <20200923121320.v3.3.I6c917ff82a1d2ae2253a3bd4d750d58879387cac@changeid>
References: <20200923121320.v3.1.I8693156f555875e5c8342e86ab37ce968dfdd277@changeid>
         <20200923121320.v3.3.I6c917ff82a1d2ae2253a3bd4d750d58879387cac@changeid>
Organization: Red Hat
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.3 (3.38.3-1.fc33) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Wed, 2020-09-23 at 12:13 +1000, Sam McNally wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> These are required for the CEC MST support.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Signed-off-by: Sam McNally <sammc@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/drm_dp_mst_topology.c | 6 ++----
>  include/drm/drm_dp_mst_helper.h       | 4 ++++
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 0d753201adbd..c783a2a1c114 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -62,8 +62,6 @@ struct drm_dp_pending_up_req {
>  static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr,
>                                   char *buf);
>  
> -static void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port);
> -
>  static int drm_dp_dpcd_write_payload(struct drm_dp_mst_topology_mgr *mgr,
>                                      int id,
>                                      struct drm_dp_payload *payload);
> @@ -1864,7 +1862,7 @@ static void drm_dp_mst_topology_get_port(struct
> drm_dp_mst_port *port)
>   * drm_dp_mst_topology_try_get_port()
>   * drm_dp_mst_topology_get_port()
>   */
> -static void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port)
> +void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port)

Mhhhhhh-can you think of some way around this? I really don't think it's a good
idea for us to be exposing topology references to things as-is, the thing is
they're really meant to be used for critical sections in code where it'd become
very painful to deal with an mst port disappearing from under us. Outside of MST
helpers, everyone else should be dealing with the expectation that these things
can disappear as a result of hotplugs at any moment.

Note that we do expose malloc refs, but that's intentional as holding a malloc
ref to something doesn't cause it to stay around even when it's unplugged - it
just stops it from being unallocated.


>  {
>         topology_ref_history_lock(port->mgr);
>  
> @@ -1935,7 +1933,7 @@ drm_dp_mst_topology_get_port_validated_locked(struct
> drm_dp_mst_branch *mstb,
>         return NULL;
>  }
>  
> -static struct drm_dp_mst_port *
> +struct drm_dp_mst_port *
>  drm_dp_mst_topology_get_port_validated(struct drm_dp_mst_topology_mgr *mgr,
>                                        struct drm_dp_mst_port *port)
>  {
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index c7c79e0ced18..d036222e0d64 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -754,6 +754,10 @@ drm_dp_mst_detect_port(struct drm_connector *connector,
>                        struct drm_dp_mst_topology_mgr *mgr,
>                        struct drm_dp_mst_port *port);
>  
> +struct drm_dp_mst_port *drm_dp_mst_topology_get_port_validated
> +(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);
> +void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port);
> +
>  struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct
> drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);
>  
>  

-- 
Sincerely,
   Lyude Paul (she/her)
   Software Engineer at Red Hat
   
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!