| Message ID | 1659587394-115256-1-git-send-email-Sanju.Mehta@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] thunderbolt: Add DP out resource when DP tunnel is discovered. | expand |
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.
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.
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.
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.
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 ;-)
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); } }
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.
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.
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: