Received-SPF: pass (google.com: domain of linux-kernel+bounces-60627-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <18761e12-822a-16b4-fdc5-0ac889b1693c@intel.com>
Date: Sat, 10 Feb 2024 17:49:34 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.11.0
Subject: Re: [PATCH] dmaengine: idxd: Clear Event Log head in idxd upon
 completion of the Enable Device command
Content-Language: en-US
To: Dave Jiang <dave.jiang@intel.com>, Vinod Koul <vkoul@kernel.org>
CC: <dmaengine@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>,
	Lingyan Guo <lingyan.guo@intel.com>
References: <20240209191851.1050501-1-fenghua.yu@intel.com>
 <36895817-8f71-461a-93e0-5db1a39cd3c4@intel.com>
From: Fenghua Yu <fenghua.yu@intel.com>
In-Reply-To: <36895817-8f71-461a-93e0-5db1a39cd3c4@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aitwN0NBUVUwakZ0aVQ4NFV0cVNmdkFSOE1MSVRoV3gycnBSc3pHOUtUME9U?=
 =?utf-8?B?WERJU0UrWVUxTXRzdGRHTXhsSk4rSlFaWVhoakZoRUkvWEgxWnVNRHpCTm9G?=
 =?utf-8?B?R2hiTlZLMXF2ZHQ2SWpCd0Ewc2lOQ3FxRUl3SFdjVkxycENzcjVCY0hoN0tW?=
 =?utf-8?B?ZWZ0WXRsNFlscWh5cUVjWHlnU01mUHQrMFFlQlppVUU4NUtsRWhlVUIrWVJ3?=
 =?utf-8?B?T25NbzM1SnNhUHExTzBrR2RWamVxcGQ5NVFFMFVnZzhYdHhPQlhCbjNZRGlN?=
 =?utf-8?B?cGR3WlpaOTIvZDdYTUJIK3pvUHE5WWl4WlRybUxYSjBlTzc5TzVLZElOd0RP?=
 =?utf-8?B?SXpiNHhVUEdDT2M4OVVJQjIvQjNadlp2aFlmNDV0RkxvV2t2TWt0S09Pbk1i?=
 =?utf-8?B?cXF2d0xYWURBNE5jaDcvbjBaSDBwSHJ3alZTYnp0L1FlckYrQ2ptTGlXVUhh?=
 =?utf-8?B?dUlPODN3YTl0eitSWjNIU2lQamJnc0hCcXVrREo1ME1lWUJNQWJmdENLN2p0?=
 =?utf-8?B?Tnl2cE54cXZGcjVXSktiR21RQWxWc1FUQ0J4OFF0MFBoUUpieTVnam1iSVl1?=
 =?utf-8?B?WVJlVVRQYVRzT1p4MmVVVnhvNFlweDVPL1JkV2JiZEZ2TVc1UDkyRitkVTVF?=
 =?utf-8?B?YXZvVnBCcVFKNWcycmtpZUQyL3YrbUFPWnJ2ZU45bDVXSEtjbVhjZ25OQnN5?=
 =?utf-8?B?OUFtcGRXNlFFVzB1OTdWci96dGxldHZSc0NqOTBSaUYvVEFVeldZR2N1ZFJC?=
 =?utf-8?B?N0JiWkRlcWs0KzlLTndGYTBDeVBUM1VIWEwvQ1RBdng2cGlaZXB4NmhnSDg5?=
 =?utf-8?B?cDJjaFpVQUNDa0pQdE9TaGk1TFVDdWk3RGNmWHp5WU55a1F0aVkvbTJsUDhZ?=
 =?utf-8?B?dTBSSXRFaWg1eGVvaU9DdU1sSXloNDFtUzJxOTYxQnY5bUtuZ1BoVTBMZU5T?=
 =?utf-8?B?NVpUUGdBUGJMOFZVb3BDTXoyMEtKMURyMGdkL0JFcnZsajg5L09zSURLc0hv?=
 =?utf-8?B?MXBuVUQzdVJ6dDlmUFMvekluNE9TeDdENU1OTTZhNVRLdmxjK0xjY1lZWTND?=
 =?utf-8?B?L3FaYm5WN0hFTytTeTllVU0xWS9pMEFNODdxU21Udm10ZjU3VG1NS3JnMmxB?=
 =?utf-8?B?NEwrcis4amt1Szg1SGtiZk10QVRzNHBvQVdhaHVhTWx0VW4wVlJLazVyN3pY?=
 =?utf-8?B?OCtieG1MR1h4TWZYKzZWdC8vcWZHNnhGL2d5aFZNeFdhVVFEOHc5VzNWRXpo?=
 =?utf-8?B?eUZDczB2enY2aEtxYmo4YS9jMk10OGtkcVltMFRSSFJHRHVHcllnWHd4UGlK?=
 =?utf-8?B?TTlSaG1yN0U4VDNvUzd6N3VKVzF2R2V5Y0lFaGczSEhGUllsOS94VFV5aU9F?=
 =?utf-8?B?L2doQzRoYlRGa3E1QTJXRjVxZXdmekQyVWFzclpEV3Zhd1lsZy8zTGJTSi9x?=
 =?utf-8?B?MjJ6Vzd5Mklyc3EzUU1GcGpBNlFTSUJYdHZJMU5XVHBtNHQ1eTIvdlJ3enJD?=
 =?utf-8?B?ZVE0Mkx2ZS9uRk0zb3gvS0JmNDJOVFdPbU9XcXpid1pHbGR6MEhwZU5yVDBs?=
 =?utf-8?B?QUxVOUdXNENjb0dUNnFRcldpQWJlRTBtNDE3RmF5VzhkN2dzYzVZV20yUW1m?=
 =?utf-8?B?K0MzYXBqQ3RERWF4aENHR3gxMEloZ2k0ZVh0MWxvUUdpa0FuRjB6VmsvRjNS?=
 =?utf-8?B?Y0M3cC9wNmpOZmVOYUN2V0VHaU5LN3pkR1VaUG5ML0VWREgrQVI2VUFrQ3lW?=
 =?utf-8?B?R2h3UFdHMGdXOTNYOFNOdGRNVUhlSlQrMUVDUksrQ2ZSYVJHcUZkOXlNL3N6?=
 =?utf-8?B?b05CSHRQTHllZFpQUlEwWG1vY3ZscFpndDJ4OTc5ZDUzbndOd0dIQjRkU0R5?=
 =?utf-8?B?L0hpWnkrMHEwTHF6ZW9WZ3NuQW9ZaDNLZXI1YVIvcjB1NWNKYUNRN05jU25K?=
 =?utf-8?B?WU5yc1pvL01RMzNvYTMzcTJpdE1nRTVxTDVLTnFIU3RKM0RpTjdCeDZrKzJm?=
 =?utf-8?B?L2hKanBqNjFUT2pWRzgzbFczZFczUDRyVEFjdGtjN0FqWEVwUGFKQ2tKMTlq?=
 =?utf-8?B?c0VFOXV5MDFtcVQ0cWRIUTZBcXJ2TlJQWWpqNHhjZkxKa0xsbG1VOHJReFJF?=
 =?utf-8?Q?emf2fWCvgDixpsOWYxRkvh/an?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e91aebd2-baf9-45c9-f60d-08dc2aa3c180
X-MS-Exchange-CrossTenant-AuthSource: IA1PR11MB6097.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Feb 2024 01:50:00.0236
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: JQqHU6yIbobdtcjRX0Ca2c0f8oPSDzAh3OdUyfSa/3GHngA44C1UHsLtD8mt1UcKMnT2Gxkj2cYT4vJPRrSEeA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB7686
X-OriginatorOrg: intel.com

Hi, Dave,

On 2/9/24 12:17, Dave Jiang wrote:
> 
> 
> On 2/9/24 12:18 PM, Fenghua Yu wrote:
>> If Event Log is supported, upon completion of the Enable Device command,
>> the Event Log head in the variable idxd->evl->head should be cleared to
>> match the state of the EVLSTATUS register. But the variable is not reset
>> currently, leading mismatch of the variable and the register state.
>> The mismatch causes incorrect processing of Event Log entries.
>>
>> Fix the issue by clearing the variable after completion of the command.
> 
> Should this be done in idxd_device_clear_state() instead?

If clear evl->head in idxd_device_clear_state(), evl->head still 
mismatches head in EVLSTATUS in some cases.

For exmample, when a few event log entries are logged and then device is 
disabled, head in EVLSTATUS is still a valid non-zero value. Clearing 
evl->head in idxd_device_clear_state() when disabling device makes 
evl->head and head in EVLSTATUS mismatched.

I haven't thought a failure test case when they mismatch in these cases 
though.

But while thinking evl->head more, I wonder why is it even needed?

head of event log can always be read from EVLSTATUS instead of from its 
shadow evl->head. And reading head from EVLSTATUS won't degrade 
performance because tail is always read from EVLSTATUS whenever head is 
read (no matter from evl->head or from EVLSATUS).

To avoid any mismatch issue/trouble, I think the right fix is to remove 
head definition in struct idxd_evl and always read head from EVLSTATUS.

Do you think this is the right fix?

Thanks.

-Fenghua