Message-ID: <130dd1d2-d37c-d266-3fa2-923359b7bc33@amd.com>
Date:   Mon, 6 Feb 2023 11:22:40 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH v2] HID: amd_sfh: if no sensors are enabled, clean up
To:     Mario Limonciello <mario.limonciello@amd.com>,
        Basavaraj Natikar <basavaraj.natikar@amd.com>,
        Jiri Kosina <jkosina@suse.cz>
Cc:     Xaver Hugl <xaver.hugl@gmail.com>, Jiri Kosina <jikos@kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230203220850.13924-1-mario.limonciello@amd.com>
Content-Language: en-US
From:   Basavaraj Natikar <bnatikar@amd.com>
In-Reply-To: <20230203220850.13924-1-mario.limonciello@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?am1SZ3JpL0U1ZUZMcGRaak45YmFVdStGVk0wZENPM3ByUExaMU5qUnF2OEYw?=
 =?utf-8?B?aUplTEUzNDJGYzFCZC9tSjRNUUVYOElvRUJzQmdVZmsvNjREaHFwcjZMbVRC?=
 =?utf-8?B?Y2orcFR3d1Y3K05TVFo3VTdwNFliMzB2b1JFVklMdDFWODBhQ3RjY0c3dFVv?=
 =?utf-8?B?MGluSS9QWDRhaitjeVdlVnlxbWN0S0RtMnlWaG9BeFJXMEk0VHFyRStuTjhG?=
 =?utf-8?B?UjgvYmVTWXhMT3RBN1NVY3VtR1VxSUFxOUR2aDBHcHdUNXpxcDBWK2pyUWV0?=
 =?utf-8?B?c1lKcmNMeURpcGsxRFVqMWh6ajVyd1ZJS0ZzN0FqSGJNalgrSnFMb09wbnVX?=
 =?utf-8?B?TTlndWNsU1pMNUtvV0ZiZHh5MVFWZWY3SjV5UDVvaVBXQ1BGTzVsU1pScXZl?=
 =?utf-8?B?QlN4OFdaRFNMUm54K1ZUbzZuZmFCREN0SzdZRWlNcXcyd01LNldiVXAycDhY?=
 =?utf-8?B?SkFScE56TEJzZHhoSS9YV2pxa3c4RjltOXdIOVNVVCtvN1l2MU9WSFNQWGEw?=
 =?utf-8?B?eHZDZThsMElBQkQ5QkV0UllHZXUwNXNKR2tFd2dpc2MyS0ZWZjZNNHhidGxm?=
 =?utf-8?B?K3BtdkI1ZTNBQ1pPZUN3R2YrcnFCQUpESk1NL1pnTFdvcDBYbEVoaTgyYktC?=
 =?utf-8?B?cWZYcFZvRmpCWXpIRnJhSzNlK0pTRjRJUHRrN01tM1ZVWW9mcjA5dHBxaWVB?=
 =?utf-8?B?cnBMOWhaakRvNS8xTWZzbTRaeWNZWGpzZHJja3NtbWtpMndUaFZCMnpoS1RR?=
 =?utf-8?B?U1oyelhVc1FUalJrQmEvQXJOTlh1NFp0R0JkV0w4dGp3bDRBKzFYdlU1WjZ3?=
 =?utf-8?B?UlN6RzRZUjdWYWdzUXR5NitHRGFvSEhtWW1jYVBLV0hKWWk2WUVOSEhRazJ2?=
 =?utf-8?B?NEhxWDNPdFl3cWdMakFOTUFHeDR4YXpXMVpuQ2tSV1daaVFzVTYxSERuMDVO?=
 =?utf-8?B?c2JzQ2RZTXM2bWVWeC9KUm1SWHdBZDBlYnRTTkZXMW1XTjl4d3E2cS9WWHB3?=
 =?utf-8?B?V3V3d2MyaGZNa1NqZDY4RWR4RXNnQUZYRW9pUG9sS1B1eURlcXZER0hxbmMv?=
 =?utf-8?B?STFKNkZzbzlTTHpRVUFWbFhyMVhndWhJUm9IVW1OdVRNWSsxOTNCR3lyaWo1?=
 =?utf-8?B?amtmdTMveGJzZDNUOEkvRktsSStrZEhPUnlxWUZzb1Z5WWtTRytRYTM3RTJO?=
 =?utf-8?B?Q3lsdGErc1pObXhRWWdmU2pPWmdQSnd0NHM1dGdJOUxocDN0OHB2V2tLMFQr?=
 =?utf-8?B?RVg2bUpxcE82cUVSLytjSGh1N3U1L255eVNsem5QRGNVS2I4ZHhleFZMRGx1?=
 =?utf-8?B?U3NQclFIaVhxaTFYUVJMelh5bS91YjFZR1VjMTV5dmc4bjEvMkpPa3U1RkU4?=
 =?utf-8?B?d1JUS0xGK1YyczUvZzFLS3h3UWp1MTdrM0IzRUx5a0NJanFRYlZWOHl0RW1S?=
 =?utf-8?B?SWoyWVFlazArNXlyY2MvVXoyM0NscUw1QnhMSytYckgyOWp1T2dIdUlaRnh1?=
 =?utf-8?B?TkV5SzA5eUhHUDRURUh0R0gvK0xBUUZVNU9IcWpKRTR1VDRmaTNtNk1NT1F4?=
 =?utf-8?B?RlNPY2lYdEhiRlVNSTRuOTkzNjIyOUxBOUVZd2I0SWVEY204cCttakdZbUph?=
 =?utf-8?B?N3F6ZURXU2hvM3BDSVFRMlBUWm9uU2czb2RPMWFyendmakdXOU1yTEtjejh5?=
 =?utf-8?B?VG8rbTVyK3FZZmlqbWhTL0s4ZjhEd2hLcUZ3b20rMlFqelZIczVwUngxNCty?=
 =?utf-8?B?dkoxVVlRcFlGVHhBa0lOM2dTRkw3REpiVGgveGQ4Q253SE9XUk1rQVRuWDU0?=
 =?utf-8?B?U2RDUTFQVWxXQkM1TWVORnZDUXRKSGNlK1dweEJpc0g2THdkeWFXZVc3bURS?=
 =?utf-8?B?NVdLVnRrck1SY25ucGJGTkRxUFpjL2FQdnMrcDlFMkJQRVl5VG5PcGRWQjVZ?=
 =?utf-8?B?N2RSK0xLVmxEYlg0cDI5YWxDd2FQZVJFYlYrNEplemtXZ2hIdGoxYXlMRytQ?=
 =?utf-8?B?UjF4UUlPMWZIdmlldDQ4eFVZeVhzRkpBTmhESTJtRWR3U3RWUjVkNEorMjRO?=
 =?utf-8?B?a2tUSkZKMG5yNDN4dm13QTcyakxXcmxWeGR1bUZLNFZFMjNadDcxTGY5VjVW?=
 =?utf-8?Q?yFtdKruQZiJeDiOVVmf3V0sc3?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2b267dbe-caf6-4ecc-bd41-08db08066254
X-MS-Exchange-CrossTenant-AuthSource: DM4PR12MB5040.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Feb 2023 05:52:51.9508
 (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: P/gUT3aC353iD5HgB9AqbZmAZek6O4D+b+SuE1j6vFmlSrsteGxSiJyY3R18QnmxvFR5+83f7bEo6tddN/llcg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB6253
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 2/4/2023 3:38 AM, Mario Limonciello wrote:
> It was reported that commit b300667b33b2 ("HID: amd_sfh: Disable the
> interrupt for all command") had caused increased resume time on HP Envy
> x360.
>
> Before this commit 3 sensors were reported, but they were not actually
> functional.  After this commit the sensors are no longer reported, but
> also the resume time increased.
>
> To avoid this problem explicitly look for the number of disabled sensors.
> If all the sensors are disabled, clean everything up.
>
> Fixes: b300667b33b2 ("HID: amd_sfh: Disable the interrupt for all command")
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2115
> Reported-by: Xaver Hugl <xaver.hugl@gmail.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v1->v2:
>  * Use a boolean parameter instead
> ---
>  drivers/hid/amd-sfh-hid/amd_sfh_client.c | 13 +++++++++++--
>  drivers/hid/amd-sfh-hid/amd_sfh_hid.h    |  1 +
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
> index 1fb0f7105fb21..c751d12f5df89 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_client.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
> @@ -227,6 +227,7 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev *privdata)
>  	cl_data->num_hid_devices = amd_mp2_get_sensor_num(privdata, &cl_data->sensor_idx[0]);
>  	if (cl_data->num_hid_devices == 0)
>  		return -ENODEV;
> +	cl_data->is_any_sensor_enabled = false;
>  
>  	INIT_DELAYED_WORK(&cl_data->work, amd_sfh_work);
>  	INIT_DELAYED_WORK(&cl_data->work_buffer, amd_sfh_work_buffer);
> @@ -287,6 +288,7 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev *privdata)
>  		status = amd_sfh_wait_for_response
>  				(privdata, cl_data->sensor_idx[i], SENSOR_ENABLED);
>  		if (status == SENSOR_ENABLED) {
> +			cl_data->is_any_sensor_enabled = true;
>  			cl_data->sensor_sts[i] = SENSOR_ENABLED;
>  			rc = amdtp_hid_probe(cl_data->cur_hid_dev, cl_data);
>  			if (rc) {
> @@ -301,19 +303,26 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev *privdata)
>  					cl_data->sensor_sts[i]);
>  				goto cleanup;
>  			}
> +		} else {
> +			cl_data->sensor_sts[i] = SENSOR_DISABLED;
> +			dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n",
> +				cl_data->sensor_idx[i],
> +				get_sensor_name(cl_data->sensor_idx[i]),
> +				cl_data->sensor_sts[i]);
>  		}
>  		dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n",
>  			cl_data->sensor_idx[i], get_sensor_name(cl_data->sensor_idx[i]),
>  			cl_data->sensor_sts[i]);
>  	}
> -	if (mp2_ops->discovery_status && mp2_ops->discovery_status(privdata) == 0) {
> +	if (!cl_data->is_any_sensor_enabled ||
> +	   (mp2_ops->discovery_status && mp2_ops->discovery_status(privdata) == 0)) {
>  		amd_sfh_hid_client_deinit(privdata);
>  		for (i = 0; i < cl_data->num_hid_devices; i++) {
>  			devm_kfree(dev, cl_data->feature_report[i]);
>  			devm_kfree(dev, in_data->input_report[i]);
>  			devm_kfree(dev, cl_data->report_descr[i]);
>  		}
> -		dev_warn(dev, "Failed to discover, sensors not enabled\n");
> +		dev_warn(dev, "Failed to discover, sensors not enabled is %d\n", cl_data->is_any_sensor_enabled);
>  		return -EOPNOTSUPP;
>  	}
>  	schedule_delayed_work(&cl_data->work_buffer, msecs_to_jiffies(AMD_SFH_IDLE_LOOP));
> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_hid.h b/drivers/hid/amd-sfh-hid/amd_sfh_hid.h
> index 3754fb423e3ae..528036892c9d2 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_hid.h
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_hid.h
> @@ -32,6 +32,7 @@ struct amd_input_data {
>  struct amdtp_cl_data {
>  	u8 init_done;
>  	u32 cur_hid_dev;
> +	bool is_any_sensor_enabled;
>  	u32 hid_dev_count;
>  	u32 num_hid_devices;
>  	struct device_info *hid_devices;

Changes looks good to me

Acked-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>

Thanks,
--
Basavaraj