Received-SPF: pass (google.com: domain of linux-kernel+bounces-61997-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <c643b6a3-0f41-416f-9268-3072f76939e3@intel.com>
Date: Mon, 12 Feb 2024 09:39:24 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] dmaengine: idxd: Clear Event Log head in idxd upon
 completion of the Enable Device command
To: Fenghua Yu <fenghua.yu@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>
 <18761e12-822a-16b4-fdc5-0ac889b1693c@intel.com>
Content-Language: en-US
From: Dave Jiang <dave.jiang@intel.com>
In-Reply-To: <18761e12-822a-16b4-fdc5-0ac889b1693c@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 2/10/24 6:49 PM, Fenghua Yu wrote:
> 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?

I was to avoid register reads during event processing. But if you think there's no performance impact then feel free to read directly from the register. 

> 
> Thanks.
> 
> -Fenghua