[PATCH v3] thunderbolt: Add DP out resource when DP tunnel is discovered.

[PATCH v3] thunderbolt: Add DP out resource when DP tunnel is discovered.

[Sanjay R Mehta]

From: Sanjay R Mehta <sanju.mehta@amd.com>

If the boot firmware implements a connection manager of its
own it may create a DP tunnel and will be handed off to Linux
CM, but the DP out resource is not saved in the dp_resource
list.

This patch adds tunnelled DP out port to the dp_resource list
once the DP tunnel is discovered.

Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>

---
v3: make tb_dp_resource_available_discovered as static function.

v2: Re-ordering the function declaration as per Greg's comment.
---
 drivers/thunderbolt/tb.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 9a3214f..53abce3 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -105,6 +105,21 @@ static void tb_remove_dp_resources(struct tb_switch *sw)
 	}
 }
 
+static void tb_dp_resource_available_discovered(struct tb *tb, struct tb_port *port)
+{
+	struct tb_cm *tcm = tb_priv(tb);
+	struct tb_port *p;
+
+	list_for_each_entry(p, &tcm->dp_resources, list) {
+		if (p == port)
+			return;
+	}
+
+	tb_port_dbg(port, "DP %s resource available discovered\n",
+		    tb_port_is_dpin(port) ? "IN" : "OUT");
+	list_add_tail(&port->list, &tcm->dp_resources);
+}
+
 static void tb_switch_discover_tunnels(struct tb_switch *sw,
 				       struct list_head *list,
 				       bool alloc_hopids)
@@ -118,6 +133,7 @@ static void tb_switch_discover_tunnels(struct tb_switch *sw,
 		switch (port->config.type) {
 		case TB_TYPE_DP_HDMI_IN:
 			tunnel = tb_tunnel_discover_dp(tb, port, alloc_hopids);
+			tb_dp_resource_available_discovered(tb, tunnel->dst_port);
 			break;
 
 		case TB_TYPE_PCIE_DOWN:
-- 
2.7.4

Re: [PATCH v3] thunderbolt: Add DP out resource when DP tunnel is discovered.

[Mika Westerberg]

On Wed, Aug 03, 2022 at 11:29:54PM -0500, Sanjay R Mehta wrote:
> From: Sanjay R Mehta <sanju.mehta@amd.com>
> 
> If the boot firmware implements a connection manager of its
> own it may create a DP tunnel and will be handed off to Linux
> CM, but the DP out resource is not saved in the dp_resource
> list.
> 
> This patch adds tunnelled DP out port to the dp_resource list
> once the DP tunnel is discovered.
> 
> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> 
> ---
> v3: make tb_dp_resource_available_discovered as static function.

Hmm, I suggested this:

  Please call this tb_discover_dp_resources() make it static and call it
  right after tb_discover_tunnels() in tb_start() or in
  tb_discover_tunnels().      

Anything preventing you to do that? Or you missed my comment?

> v2: Re-ordering the function declaration as per Greg's comment.
> ---
>  drivers/thunderbolt/tb.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 9a3214f..53abce3 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -105,6 +105,21 @@ static void tb_remove_dp_resources(struct tb_switch *sw)
>  	}
>  }
>  
> +static void tb_dp_resource_available_discovered(struct tb *tb, struct tb_port *port)
> +{
> +	struct tb_cm *tcm = tb_priv(tb);
> +	struct tb_port *p;
> +
> +	list_for_each_entry(p, &tcm->dp_resources, list) {
> +		if (p == port)
> +			return;
> +	}
> +
> +	tb_port_dbg(port, "DP %s resource available discovered\n",
> +		    tb_port_is_dpin(port) ? "IN" : "OUT");
> +	list_add_tail(&port->list, &tcm->dp_resources);
> +}
> +
>  static void tb_switch_discover_tunnels(struct tb_switch *sw,
>  				       struct list_head *list,
>  				       bool alloc_hopids)
> @@ -118,6 +133,7 @@ static void tb_switch_discover_tunnels(struct tb_switch *sw,
>  		switch (port->config.type) {
>  		case TB_TYPE_DP_HDMI_IN:
>  			tunnel = tb_tunnel_discover_dp(tb, port, alloc_hopids);

Here tunnel can be NULL...

> +			tb_dp_resource_available_discovered(tb, tunnel->dst_port);

... so this will crash and burn.

Re: [PATCH v3] thunderbolt: Add DP out resource when DP tunnel is discovered.

[Sanjay R Mehta]

On 8/4/2022 12:00 PM, Mika Westerberg wrote:
> On Wed, Aug 03, 2022 at 11:29:54PM -0500, Sanjay R Mehta wrote:
>> From: Sanjay R Mehta <sanju.mehta@amd.com>
>>
>> If the boot firmware implements a connection manager of its
>> own it may create a DP tunnel and will be handed off to Linux
>> CM, but the DP out resource is not saved in the dp_resource
>> list.
>>
>> This patch adds tunnelled DP out port to the dp_resource list
>> once the DP tunnel is discovered.
>>
>> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>>
>> ---
>> v3: make tb_dp_resource_available_discovered as static function.
> 
> Hmm, I suggested this:
> 
>   Please call this tb_discover_dp_resources() make it static and call it
>   right after tb_discover_tunnels() in tb_start() or in
>   tb_discover_tunnels().      
> 
> Anything preventing you to do that? Or you missed my comment?
Sorry, I missed the comment of changing function name. I'll resend the
patch.
> 
>> v2: Re-ordering the function declaration as per Greg's comment.
>> ---
>>  drivers/thunderbolt/tb.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
>> index 9a3214f..53abce3 100644
>> --- a/drivers/thunderbolt/tb.c
>> +++ b/drivers/thunderbolt/tb.c
>> @@ -105,6 +105,21 @@ static void tb_remove_dp_resources(struct tb_switch *sw)
>>  	}
>>  }
>>  
>> +static void tb_dp_resource_available_discovered(struct tb *tb, struct tb_port *port)
>> +{
>> +	struct tb_cm *tcm = tb_priv(tb);
>> +	struct tb_port *p;
>> +
>> +	list_for_each_entry(p, &tcm->dp_resources, list) {
>> +		if (p == port)
>> +			return;
>> +	}
>> +
>> +	tb_port_dbg(port, "DP %s resource available discovered\n",
>> +		    tb_port_is_dpin(port) ? "IN" : "OUT");
>> +	list_add_tail(&port->list, &tcm->dp_resources);
>> +}
>> +
>>  static void tb_switch_discover_tunnels(struct tb_switch *sw,
>>  				       struct list_head *list,
>>  				       bool alloc_hopids)
>> @@ -118,6 +133,7 @@ static void tb_switch_discover_tunnels(struct tb_switch *sw,
>>  		switch (port->config.type) {
>>  		case TB_TYPE_DP_HDMI_IN:
>>  			tunnel = tb_tunnel_discover_dp(tb, port, alloc_hopids);
> 
> Here tunnel can be NULL...
> 
>> +			tb_dp_resource_available_discovered(tb, tunnel->dst_port);
> 
> ... so this will crash and burn.

Thanks. Agree, I will add check here and resend the patch.

Re: [PATCH v3] thunderbolt: Add DP out resource when DP tunnel is discovered.

[Mika Westerberg]

On Thu, Aug 04, 2022 at 12:34:09PM +0530, Sanjay R Mehta wrote:
> >>  			tunnel = tb_tunnel_discover_dp(tb, port, alloc_hopids);
> > 
> > Here tunnel can be NULL...
> > 
> >> +			tb_dp_resource_available_discovered(tb, tunnel->dst_port);
> > 
> > ... so this will crash and burn.
> 
> Thanks. Agree, I will add check here and resend the patch.

Please don't add the check here but move this to tb_start() as I
suggested.

Re: [PATCH v3] thunderbolt: Add DP out resource when DP tunnel is discovered.

[Sanjay R Mehta]

On 8/4/2022 12:38 PM, Mika Westerberg wrote:
> On Thu, Aug 04, 2022 at 12:34:09PM +0530, Sanjay R Mehta wrote:
>>>>  			tunnel = tb_tunnel_discover_dp(tb, port, alloc_hopids);
>>>
>>> Here tunnel can be NULL...
>>>
>>>> +			tb_dp_resource_available_discovered(tb, tunnel->dst_port);
>>>
>>> ... so this will crash and burn.
>>
>> Thanks. Agree, I will add check here and resend the patch.
> 
> Please don't add the check here but move this to tb_start() as I
> suggested.

Sure Mika.

As you earlier suggested to move this function to either "tb_start() or
tb_discover_tunnels()".


Since adding of this DP OUT resource is required for each DP tunnel,
hence I felt it will be better if I move this function in
tb_discover_tunnels() where we can avoid extra for loop for tunnel.

Below is the place how I am thinking of adding
"tb_discover_dp_resources()" function.


static void tb_discover_tunnels(struct tb *tb)
{
        struct tb_cm *tcm = tb_priv(tb);
        struct tb_tunnel *tunnel;

        tb_switch_discover_tunnels(tb->root_switch, &tcm->tunnel_list,
true);

        list_for_each_entry(tunnel, &tcm->tunnel_list, list) {
                if (tb_tunnel_is_pci(tunnel)) {
                        struct tb_switch *parent = tunnel->dst_port->sw;

                        while (parent != tunnel->src_port->sw) {
                                parent->boot = true;
                                parent = tb_switch_parent(parent);
                        }
                } else if (tb_tunnel_is_dp(tunnel)) {
                        /* Keep the domain from powering down */
                        tb_discover_dp_resources(tb, tunnel->dst_port);
                        pm_runtime_get_sync(&tunnel->src_port->sw->dev);
                        pm_runtime_get_sync(&tunnel->dst_port->sw->dev);
                }
        }
}


Does this make sense? Please suggest me if I have to do it other way.
Appreciate your help.

Re: [PATCH v3] thunderbolt: Add DP out resource when DP tunnel is discovered.

[Mika Westerberg]

On Thu, Aug 04, 2022 at 01:39:44PM +0530, Sanjay R Mehta wrote:
> 
> 
> On 8/4/2022 12:38 PM, Mika Westerberg wrote:
> > On Thu, Aug 04, 2022 at 12:34:09PM +0530, Sanjay R Mehta wrote:
> >>>>  			tunnel = tb_tunnel_discover_dp(tb, port, alloc_hopids);
> >>>
> >>> Here tunnel can be NULL...
> >>>
> >>>> +			tb_dp_resource_available_discovered(tb, tunnel->dst_port);
> >>>
> >>> ... so this will crash and burn.
> >>
> >> Thanks. Agree, I will add check here and resend the patch.
> > 
> > Please don't add the check here but move this to tb_start() as I
> > suggested.
> 
> Sure Mika.
> 
> As you earlier suggested to move this function to either "tb_start() or
> tb_discover_tunnels()".
> 
> 
> Since adding of this DP OUT resource is required for each DP tunnel,
> hence I felt it will be better if I move this function in
> tb_discover_tunnels() where we can avoid extra for loop for tunnel.
> 
> Below is the place how I am thinking of adding
> "tb_discover_dp_resources()" function.
> 
> 
> static void tb_discover_tunnels(struct tb *tb)
> {
>         struct tb_cm *tcm = tb_priv(tb);
>         struct tb_tunnel *tunnel;
> 
>         tb_switch_discover_tunnels(tb->root_switch, &tcm->tunnel_list,
> true);
> 
>         list_for_each_entry(tunnel, &tcm->tunnel_list, list) {
>                 if (tb_tunnel_is_pci(tunnel)) {
>                         struct tb_switch *parent = tunnel->dst_port->sw;
> 
>                         while (parent != tunnel->src_port->sw) {
>                                 parent->boot = true;
>                                 parent = tb_switch_parent(parent);
>                         }
>                 } else if (tb_tunnel_is_dp(tunnel)) {
>                         /* Keep the domain from powering down */
>                         tb_discover_dp_resources(tb, tunnel->dst_port);
>                         pm_runtime_get_sync(&tunnel->src_port->sw->dev);
>                         pm_runtime_get_sync(&tunnel->dst_port->sw->dev);
>                 }
>         }
> }
> 
> 
> Does this make sense? Please suggest me if I have to do it other way.
> Appreciate your help.

How about splitting this into tb_discover_dp_resources() that then calls
tb_discover_dp_resource() for a single router? Whatever is the simplest ;-)

Re: [PATCH v3] thunderbolt: Add DP out resource when DP tunnel is discovered.

[Sanjay R Mehta]

On 8/4/2022 2:01 PM, Mika Westerberg wrote:
> On Thu, Aug 04, 2022 at 01:39:44PM +0530, Sanjay R Mehta wrote:
>>
>>
>> On 8/4/2022 12:38 PM, Mika Westerberg wrote:
>>> On Thu, Aug 04, 2022 at 12:34:09PM +0530, Sanjay R Mehta wrote:
>>>>>>  			tunnel = tb_tunnel_discover_dp(tb, port, alloc_hopids);
>>>>>
>>>>> Here tunnel can be NULL...
>>>>>
>>>>>> +			tb_dp_resource_available_discovered(tb, tunnel->dst_port);
>>>>>
>>>>> ... so this will crash and burn.
>>>>
>>>> Thanks. Agree, I will add check here and resend the patch.
>>>
>>> Please don't add the check here but move this to tb_start() as I
>>> suggested.
>>
>> Sure Mika.
>>
>> As you earlier suggested to move this function to either "tb_start() or
>> tb_discover_tunnels()".
>>
>>
>> Since adding of this DP OUT resource is required for each DP tunnel,
>> hence I felt it will be better if I move this function in
>> tb_discover_tunnels() where we can avoid extra for loop for tunnel.
>>
>> Below is the place how I am thinking of adding
>> "tb_discover_dp_resources()" function.
>>
>>
>> static void tb_discover_tunnels(struct tb *tb)
>> {
>>         struct tb_cm *tcm = tb_priv(tb);
>>         struct tb_tunnel *tunnel;
>>
>>         tb_switch_discover_tunnels(tb->root_switch, &tcm->tunnel_list,
>> true);
>>
>>         list_for_each_entry(tunnel, &tcm->tunnel_list, list) {
>>                 if (tb_tunnel_is_pci(tunnel)) {
>>                         struct tb_switch *parent = tunnel->dst_port->sw;
>>
>>                         while (parent != tunnel->src_port->sw) {
>>                                 parent->boot = true;
>>                                 parent = tb_switch_parent(parent);
>>                         }
>>                 } else if (tb_tunnel_is_dp(tunnel)) {
>>                         /* Keep the domain from powering down */
>>                         tb_discover_dp_resources(tb, tunnel->dst_port);
>>                         pm_runtime_get_sync(&tunnel->src_port->sw->dev);
>>                         pm_runtime_get_sync(&tunnel->dst_port->sw->dev);
>>                 }
>>         }
>> }
>>
>>
>> Does this make sense? Please suggest me if I have to do it other way.
>> Appreciate your help.
> 
> How about splitting this into tb_discover_dp_resources() that then calls
> tb_discover_dp_resource() for a single router? Whatever is the simplest ;-)

You mean something as below & call it into tb_start() after
tb_discover_tunnels() ?

static void tb_discover_dp_resources(struct tb *tb)
{
        struct tb_cm *tcm = tb_priv(tb);
        struct tb_tunnel *tunnel;


        list_for_each_entry(tunnel, &tcm->tunnel_list, list) {
                if (tb_tunnel_is_dp(tunnel))
			tb_discover_dp_resource(tb, tunnel->dst_port);
	}
}

Re: [PATCH v3] thunderbolt: Add DP out resource when DP tunnel is discovered.

[Mika Westerberg]

On Thu, Aug 04, 2022 at 02:27:22PM +0530, Sanjay R Mehta wrote:
> You mean something as below & call it into tb_start() after
> tb_discover_tunnels() ?
> 
> static void tb_discover_dp_resources(struct tb *tb)
> {
>         struct tb_cm *tcm = tb_priv(tb);
>         struct tb_tunnel *tunnel;
> 
> 
>         list_for_each_entry(tunnel, &tcm->tunnel_list, list) {
>                 if (tb_tunnel_is_dp(tunnel))
> 			tb_discover_dp_resource(tb, tunnel->dst_port);
> 	}
> }
> 

Yes.

Re: [PATCH v3] thunderbolt: Add DP out resource when DP tunnel is discovered.

[Sanjay R Mehta]

On 8/4/2022 2:45 PM, Mika Westerberg wrote:
> On Thu, Aug 04, 2022 at 02:27:22PM +0530, Sanjay R Mehta wrote:
>> You mean something as below & call it into tb_start() after
>> tb_discover_tunnels() ?
>>
>> static void tb_discover_dp_resources(struct tb *tb)
>> {
>>         struct tb_cm *tcm = tb_priv(tb);
>>         struct tb_tunnel *tunnel;
>>
>>
>>         list_for_each_entry(tunnel, &tcm->tunnel_list, list) {
>>                 if (tb_tunnel_is_dp(tunnel))
>> 			tb_discover_dp_resource(tb, tunnel->dst_port);
>> 	}
>> }
>>
> 
> Yes.

Thanks. will send this as v3 resend.