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

[Sanjay R Mehta <sanmehta@amd.com>]

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);
	}
}