Re: [PATCH 1/3] thunderbolt: Read DP IN adapter first two dwords in one go

[Brad Campbell]

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@xxxxxxxxxxxxxxx>

Tested-by: Brad Campbell <lists2009@xxxxxxxxxxxxxxx>

> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> ---
>   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 */