Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7c0623f8-5274-c7ee-71a7-3e0fab918f97@amd.com>
Date:   Fri, 23 Sep 2022 11:08:30 -0500
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [PATCH v2] thunderbolt: Explicitly enable lane adapter hotplug
 events at startup
Content-Language: en-US
To:     Mika Westerberg <mika.westerberg@linux.intel.com>
Cc:     Andreas Noever <andreas.noever@gmail.com>,
        Michael Jamet <michael.jamet@intel.com>,
        Yehezkel Bernat <YehezkelShB@gmail.com>,
        Mehta Sanju <Sanju.Mehta@amd.com>, stable@vger.kernel.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220922160730.898-1-mario.limonciello@amd.com>
 <Yy15gKzHyMcitY/N@black.fi.intel.com>
From:   "Limonciello, Mario" <mario.limonciello@amd.com>
In-Reply-To: <Yy15gKzHyMcitY/N@black.fi.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cXVnRDZKMFhYTWt3SkNTMjFNWWY2b3ptZTMvbGdOb1AyNUUzT3c4cXFHNTQ5?=
 =?utf-8?B?NWs0UldUcVdrWmdWMU0yNjNlK1kxUWFPU3JnRGhkbkRGT0R5V0hMWXVGTFhK?=
 =?utf-8?B?eVBDOGcraXZ6OGhPcGNuR3k4ZG1oOGlwYzViNC9kOXZLZkxmRGFoWFRzdmk4?=
 =?utf-8?B?MmlQeklidC9VNk96aVF5VmZKbVF2Wisva00vNVJMMld2S1NsdGtjQkw2UUgv?=
 =?utf-8?B?N0FiQWwyTmRoNStGRWM4U0cxeldpd1RNN2t5czhuTlVDZDNZUExCdnB1eVE0?=
 =?utf-8?B?OVQ4MjF0R3M2U2RwU3ZEaVIxVXRBanYxN3lhMVNyMHdydk95WndpZlM4UE0r?=
 =?utf-8?B?NkNEcTJhbWZXM1BHT1J6MEs1YmxMSW9XMkRPeGhKMGdRNDZ1M3d1RDBwR1da?=
 =?utf-8?B?SzA2Z0o5eEhyc0NvT3ZLR1RNaTZvdzJ3R3FQbDBZcUs1eDlMZVZCcmlNQlM3?=
 =?utf-8?B?Z3luZHVQcDJYdnoyVVRGNEpOVXlzZTZtd3JiNWNVNVNWSVhMMG1YVDlnRzc5?=
 =?utf-8?B?OWpsWjQ2OWxvem5OVC9ZY3JSWFFqRHVkWnBvNVYxUjZVOXpFdUUyak9ITkVY?=
 =?utf-8?B?S1pJMW81WTRJZk1zbkhXYS9ITnlJTUIwR0RNbWpiWEVBYXlZYkRTTTBROUhi?=
 =?utf-8?B?TWNNWFBJQnc3QzBhWTkxYUY0SEVrTnpCVnFERllhdEVPckpRd2FTR2JqYjNq?=
 =?utf-8?B?UnkrTXBJRjFyekdPMzJreTMxUzdHcFdGYkt6ZUJ5aEtVdkY2dUZEUWUyUm9Z?=
 =?utf-8?B?K28ycUxta1ZyMWh3cFZ2U21sS2dYWkZESWtIRXNBZkllOExPUllsZGhrZXF6?=
 =?utf-8?B?NlpWMWdHMmdSOFhjcjVudlRFN1E4TDl1UmF1YXhLVVE5amFoZU45SDVoN2Yy?=
 =?utf-8?B?akx5RWQ4Tnpld2tLRDJhMFJxc01OT2pMYzdzMHBWcVN6MmJpK1lVbzgyUWU2?=
 =?utf-8?B?R0ludGFOSlAvUFlKSm9OVmZPUTByejc2bHdxRGRteWtVeG90QUlHbXhrTndC?=
 =?utf-8?B?SkE2aStrRGJlamV3bFh6SXdvOGhrdkNtbWQ0ckRKREZVY1NodEJzbStpRFA2?=
 =?utf-8?B?UkRSUTdlVDgvU0ZTRXNheUZJOWpDd3RhaFdNV0Y3VDAzZEI2b2F5N0Jpemhx?=
 =?utf-8?B?c01BekNOY3JwajhPZWwrMno5Qkl0ditTZGlRcGZBRW9kT25tTkJMVHVva2ly?=
 =?utf-8?B?aE5oTUcxeVdHbWhPQWRnMEhpSXVtall5SURIaUZBa2R2TStHTzlWd2NHVFNk?=
 =?utf-8?B?MjZxWnNSbFg0aG1rUE9oclF6YjkrTU5MRTJ0UEZTamJjREZuV1U2L3A1Q1Iz?=
 =?utf-8?B?dGpRL2lzQnBJOVEvVGJVM2dpcy9YVFl5Yy8xM2xvM2FVaGliME1vMDl2aDlB?=
 =?utf-8?B?cXNLNnJEM0dSelU3Q3V0QzEzdXZoa1Z4TnZEbkdMNDZvRmVrMVZDaXBZL3FO?=
 =?utf-8?B?S1VlVE04b29JaXhOOWM4cEZ3czNnelVrLzZ5a2pRUXN3cWFrbFhlRGYyNUE3?=
 =?utf-8?B?TXJxajZtcEUvQXQwaTd2M0FYOXRPY2MvUHZtbWpuMy8vSjIyc2hwTEF3NkYy?=
 =?utf-8?B?QXRjbHRkeXMvcTVNcmg3VGxYekI5MDJLZjF2UE8vQkljaGQ5UXBxWjNhZDBS?=
 =?utf-8?B?ZUliaHJuQ3RvRVNyTXUyWC9NSFhFVjA5YUxCRGpkRjRVN0tldkxzSmQ3SDY0?=
 =?utf-8?B?cHBEd2F3MTBSQlRWRk5TalFaamUwOGY5aDhnTDFIOUJURWZuQ0duSmtUWlhV?=
 =?utf-8?B?dEhOV3Rmc1JwdEViRE9Eb2taLzBDdnMwMS9tUzNDVzJIek1pQlEzSlhpS0h2?=
 =?utf-8?B?R01YYllBYm9KZXpNM3FPN3l5ckJpMmJMSms0QWF5TGx3clZjZzNTakN2SGww?=
 =?utf-8?B?RzNhaHBNVzM1SC9LaW16akw1Y2o4SVZFeTMyOThRRFFSTkRka3dOSHA2SEtM?=
 =?utf-8?B?eGpybkdOTDAvdVhwV3hkWUNLNkxDd0JzMllsQ0JpTVhjalVEb1IySVN3NUxL?=
 =?utf-8?B?SGl5aDI5dE5NczVJYVN3bDh2NCtPeVRIL3MwanovOXN1U1FSY1UyTFJSYXBL?=
 =?utf-8?B?ZXZubks1R2xSQmUwZFdPYkd4VVBiMDd3M1BJb0hCSW9TS0k2S3h4Ym1Jc2lR?=
 =?utf-8?Q?7SC43u8vl3LiqjLWSq1ENfnPI?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bced7454-1bf3-4140-5d70-08da9d7ddca7
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Sep 2022 16:08:32.6181
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 9G6rVn8wbz44/DiLVRjAyAXJFBke8FdiGQtRLWWRH8fXgv2OavXmNgtI7Acu/geTj+OUff6x3Y74I9V5flBm2w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4042
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,SPF_HELO_PASS,
        SPF_PASS autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 9/23/2022 04:16, Mika Westerberg wrote:
> Hi Mario,
> 
> On Thu, Sep 22, 2022 at 11:07:29AM -0500, Mario Limonciello wrote:
>> Software that has run before the USB4 CM in Linux runs may have disabled
>> hotplug events for a given lane adapter.
>>
>> Other CMs such as that one distributed with Windows 11 will enable hotplug
>> events. Do the same thing in the Linux CM which fixes hotplug events on
>> "AMD Pink Sardine".
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> v1->v2:
>>   * Only send second patch as first was merged already
>>   * s/usb4_enable_hotplug/usb4_port_hotplug_enable/
>>   * Clarify intended users in documentation comment
>>   * Only call for lane adapters
>>   * Add stable tag
>>
>>   drivers/thunderbolt/switch.c  |  4 ++++
>>   drivers/thunderbolt/tb.h      |  1 +
>>   drivers/thunderbolt/tb_regs.h |  1 +
>>   drivers/thunderbolt/usb4.c    | 20 ++++++++++++++++++++
>>   4 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
>> index 77d7f07ca075..3213239d12c8 100644
>> --- a/drivers/thunderbolt/switch.c
>> +++ b/drivers/thunderbolt/switch.c
>> @@ -778,6 +778,10 @@ static int tb_init_port(struct tb_port *port)
>>   
>>   			if (!tb_port_read(port, &hop, TB_CFG_HOPS, 0, 2))
>>   				port->ctl_credits = hop.initial_credits;
>> +
>> +			res = usb4_port_hotplug_enable(port);
>> +			if (res)
> 
> I think this does not belong here in tb_init_port(). This is called from
> both FW and SW CM paths and we don't want to confuse the FW CM more than
> necessary ;-)
> 
> So instead I think this should be added to tb_plug_events_active().
> 

The problem with that location is that tb_plug_events_active() is called 
from tb_switch_configure() which is before tb_switch_add() is called.
tb_switch_add() calls tb_init_port() which reads port->config for the 
first time.

So if this is only to be called in tb_switch_configure() it means 
reading port->config "earlier" too.

So it definitely needs to be called in tb_init_port() or a later 
function but before the device is announced to satisfy only running on 
the appropriate port types.

tb_init_port() or tb_switch_add feels like the right place to me.  How 
about leaving it where it is but guarding with a "if 
(!tb_switch_is_icm())" to avoid the risk to the FW CM case?

> Otherwise looks good to me.
> 
>> +				return res;
>>   		}
>>   		if (!port->ctl_credits)
>>   			port->ctl_credits = 2;
>> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
>> index 5db76de40cc1..332159f984fc 100644
>> --- a/drivers/thunderbolt/tb.h
>> +++ b/drivers/thunderbolt/tb.h
>> @@ -1174,6 +1174,7 @@ int usb4_switch_add_ports(struct tb_switch *sw);
>>   void usb4_switch_remove_ports(struct tb_switch *sw);
>>   
>>   int usb4_port_unlock(struct tb_port *port);
>> +int usb4_port_hotplug_enable(struct tb_port *port);
>>   int usb4_port_configure(struct tb_port *port);
>>   void usb4_port_unconfigure(struct tb_port *port);
>>   int usb4_port_configure_xdomain(struct tb_port *port);
>> diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
>> index 166054110388..bbe38b2d9057 100644
>> --- a/drivers/thunderbolt/tb_regs.h
>> +++ b/drivers/thunderbolt/tb_regs.h
>> @@ -308,6 +308,7 @@ struct tb_regs_port_header {
>>   #define ADP_CS_5				0x05
>>   #define ADP_CS_5_LCA_MASK			GENMASK(28, 22)
>>   #define ADP_CS_5_LCA_SHIFT			22
>> +#define ADP_CS_5_DHP				BIT(31)
>>   
>>   /* TMU adapter registers */
>>   #define TMU_ADP_CS_3				0x03
>> diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c
>> index 3a2e7126db9d..f0b5a8f1ed3a 100644
>> --- a/drivers/thunderbolt/usb4.c
>> +++ b/drivers/thunderbolt/usb4.c
>> @@ -1046,6 +1046,26 @@ int usb4_port_unlock(struct tb_port *port)
>>   	return tb_port_write(port, &val, TB_CFG_PORT, ADP_CS_4, 1);
>>   }
>>   
>> +/**
>> + * usb4_port_hotplug_enable() - Enables hotplug for a port
>> + * @port: USB4 port to operate on
>> + *
>> + * Enables hot plug events on a given port. This is only intended
>> + * to be used on lane, DP-IN, and DP-OUT adapters.
>> + */
>> +int usb4_port_hotplug_enable(struct tb_port *port)
>> +{
>> +	int ret;
>> +	u32 val;
>> +
>> +	ret = tb_port_read(port, &val, TB_CFG_PORT, ADP_CS_5, 1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	val &= ~ADP_CS_5_DHP;
>> +	return tb_port_write(port, &val, TB_CFG_PORT, ADP_CS_5, 1);
>> +}
>> +
>>   static int usb4_port_set_configured(struct tb_port *port, bool configured)
>>   {
>>   	int ret;
>> -- 
>> 2.34.1