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
