| Message ID | 20191001102905.21680-2-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Commit | fd5c46b754d4799afda8dcdd6851e0390aa4961a |
| Headers | show |
| Series | thunderbolt: Fixes for few reported issues | expand |
On 1/10/19 6:29 pm, Mika Westerberg wrote: > When we discover existing DP tunnels the code checks whether DP IN > adapter port is enabled by calling tb_dp_port_is_enabled() before it > continues the discovery process. On Light Ridge (gen 1) controller > reading only the first dword of the DP IN config space causes subsequent > access to the same DP IN port path config space to fail or return > invalid data as can be seen in the below splat: > > thunderbolt 0000:07:00.0: CFG_ERROR(0:d): Invalid config space or offset > Call Trace: > tb_cfg_read+0xb9/0xd0 > __tb_path_deactivate_hop+0x98/0x210 > tb_path_activate+0x228/0x7d0 > tb_tunnel_restart+0x95/0x200 > tb_handle_hotplug+0x30e/0x630 > process_one_work+0x1b4/0x340 > worker_thread+0x44/0x3d0 > kthread+0xeb/0x120 > ? process_one_work+0x340/0x340 > ? kthread_park+0xa0/0xa0 > ret_from_fork+0x1f/0x30 > > If both DP In adapter config dwords are read in one go the issue does > not reproduce. This is likely firmware bug but we can work it around by > always reading the two dwords in one go. There should be no harm for > other controllers either so can do it unconditionally. > > Link: https://lkml.org/lkml/2019/8/28/160 > Reported-by: Brad Campbell <lists2009@fnarfbargle.com> Tested-by: Brad Campbell <lists2009@fnarfbargle.com> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/thunderbolt/switch.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c > index 410bf1bceeee..8e712fbf8233 100644 > --- a/drivers/thunderbolt/switch.c > +++ b/drivers/thunderbolt/switch.c > @@ -896,12 +896,13 @@ int tb_dp_port_set_hops(struct tb_port *port, unsigned int video, > */ > bool tb_dp_port_is_enabled(struct tb_port *port) > { > - u32 data; > + u32 data[2]; > > - if (tb_port_read(port, &data, TB_CFG_PORT, port->cap_adap, 1)) > + if (tb_port_read(port, data, TB_CFG_PORT, port->cap_adap, > + ARRAY_SIZE(data))) > return false; > > - return !!(data & (TB_DP_VIDEO_EN | TB_DP_AUX_EN)); > + return !!(data[0] & (TB_DP_VIDEO_EN | TB_DP_AUX_EN)); > } > > /** > @@ -914,19 +915,21 @@ bool tb_dp_port_is_enabled(struct tb_port *port) > */ > int tb_dp_port_enable(struct tb_port *port, bool enable) > { > - u32 data; > + u32 data[2]; > int ret; > > - ret = tb_port_read(port, &data, TB_CFG_PORT, port->cap_adap, 1); > + ret = tb_port_read(port, data, TB_CFG_PORT, port->cap_adap, > + ARRAY_SIZE(data)); > if (ret) > return ret; > > if (enable) > - data |= TB_DP_VIDEO_EN | TB_DP_AUX_EN; > + data[0] |= TB_DP_VIDEO_EN | TB_DP_AUX_EN; > else > - data &= ~(TB_DP_VIDEO_EN | TB_DP_AUX_EN); > + data[0] &= ~(TB_DP_VIDEO_EN | TB_DP_AUX_EN); > > - return tb_port_write(port, &data, TB_CFG_PORT, port->cap_adap, 1); > + return tb_port_write(port, data, TB_CFG_PORT, port->cap_adap, > + ARRAY_SIZE(data)); > } > > /* switch utility functions */ >
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c index 410bf1bceeee..8e712fbf8233 100644 --- a/drivers/thunderbolt/switch.c +++ b/drivers/thunderbolt/switch.c @@ -896,12 +896,13 @@ int tb_dp_port_set_hops(struct tb_port *port, unsigned int video, */ bool tb_dp_port_is_enabled(struct tb_port *port) { - u32 data; + u32 data[2]; - if (tb_port_read(port, &data, TB_CFG_PORT, port->cap_adap, 1)) + if (tb_port_read(port, data, TB_CFG_PORT, port->cap_adap, + ARRAY_SIZE(data))) return false; - return !!(data & (TB_DP_VIDEO_EN | TB_DP_AUX_EN)); + return !!(data[0] & (TB_DP_VIDEO_EN | TB_DP_AUX_EN)); } /** @@ -914,19 +915,21 @@ bool tb_dp_port_is_enabled(struct tb_port *port) */ int tb_dp_port_enable(struct tb_port *port, bool enable) { - u32 data; + u32 data[2]; int ret; - ret = tb_port_read(port, &data, TB_CFG_PORT, port->cap_adap, 1); + ret = tb_port_read(port, data, TB_CFG_PORT, port->cap_adap, + ARRAY_SIZE(data)); if (ret) return ret; if (enable) - data |= TB_DP_VIDEO_EN | TB_DP_AUX_EN; + data[0] |= TB_DP_VIDEO_EN | TB_DP_AUX_EN; else - data &= ~(TB_DP_VIDEO_EN | TB_DP_AUX_EN); + data[0] &= ~(TB_DP_VIDEO_EN | TB_DP_AUX_EN); - return tb_port_write(port, &data, TB_CFG_PORT, port->cap_adap, 1); + return tb_port_write(port, data, TB_CFG_PORT, port->cap_adap, + ARRAY_SIZE(data)); } /* switch utility functions */
When we discover existing DP tunnels the code checks whether DP IN adapter port is enabled by calling tb_dp_port_is_enabled() before it continues the discovery process. On Light Ridge (gen 1) controller reading only the first dword of the DP IN config space causes subsequent access to the same DP IN port path config space to fail or return invalid data as can be seen in the below splat: thunderbolt 0000:07:00.0: CFG_ERROR(0:d): Invalid config space or offset Call Trace: tb_cfg_read+0xb9/0xd0 __tb_path_deactivate_hop+0x98/0x210 tb_path_activate+0x228/0x7d0 tb_tunnel_restart+0x95/0x200 tb_handle_hotplug+0x30e/0x630 process_one_work+0x1b4/0x340 worker_thread+0x44/0x3d0 kthread+0xeb/0x120 ? process_one_work+0x340/0x340 ? kthread_park+0xa0/0xa0 ret_from_fork+0x1f/0x30 If both DP In adapter config dwords are read in one go the issue does not reproduce. This is likely firmware bug but we can work it around by always reading the two dwords in one go. There should be no harm for other controllers either so can do it unconditionally. Link: https://lkml.org/lkml/2019/8/28/160 Reported-by: Brad Campbell <lists2009@fnarfbargle.com> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/thunderbolt/switch.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)