On Thu, Jan 27, 2022 at 1:08 PM <Ajish.Koshy@xxxxxxxxxxxxx> wrote: > > Hi Jinpu, > > > > > > > > > > > For SATA devices, correct the double completion issue. > > > > > > > > > > Current code handles completions for sata devices in > > > > > mpi_sata_completion() and mpi_sata_event(). > > > > > > > > > > But at the time when any sata event happens, for almost all the > > > > > event types, the command is still in the target. It is wrong to > > > > > complete the task in sata_event(). > > > > > > > > Is it also an issue for SSP? what is the side effect, the current > > > > code will lead to (w/o this patch)? > > > > why SATA is different? > > > > > > > > Thanks! > > > > > > Thanks Jinpu. > > > > > > Yes, same issue with the SSP event path too. We need similar changes > > > there also. > > > > > If that's the case, probably it should fix in same patch. > > We do have plans to post the changes for > ssp event path too. Will be handled > in a separate patch. > > This event/completion issue came to light only > recently when firmware for one of the SATA > device generated IO_XFER_ERROR_NAK_RECEIVED > event and later firmware attempted to the > complete the same command. > > > > > > My understanding here on side effects without this patch- Here is the > > > expectation of the firmware. > > > The controller sends a sata_event notification to indicate that the > > > I/O is not finished (may be pending in the controller or in the remote > > > target) and that the host must take additional action. The host action > > > required depends on the event received. > > > > > > There are cases, where commands need special handling mentioned as per > > > the spec. > > > > > > But for most of the event types, the host must reset the SATA drive > > > and abort the pending I/O in the controller by sending abort command > > > > > > Till here inside the firmware all the resource are intact, tag, > > > memory, etc. > > > > > > Now in the host side, once we receive the event, host will free its > > > own resources, tag, dma memory, etc. by calling its own completion. > > > > > > So at this point firmware has not freed its own the resources and host > > > freed its resources corresponding to the given command/tag. > > > > > > For example, in the case of IO_XFER_ERROR_NAK_RECEIVED sata event. > > > Here firmware will keep retrying the command. There are chances the > > > corresponding command may get successful and firmware may give > > > response through sata completion. > > > > > > Since we received the sata_event, we will complete the command. > > > Host will free its own resouces, tags, dma memory, etc. At this point > > > when we received this event, command was still pending in the target. > > > > > > In this case since we freed the DMA memory, while firmware completes > > > the command, it will generate IO_XFER_READ_COMPL_ERR event (pcie > > error > > > based) for the same tag/ command since we already freed the DMA > > > memory/resources. > > My impression was the FW should either generate SATA_EVENT or > > SATA_COPMLETION, but not both, was this behavior changed? > > I too had same impression when started to look into > pm80xx code. But that is not the case after going > through the firmware spec. 'During events the command > is still with the firmware.' > > In some cases we may get SATA_EVENT as well as > SATA_COMPLETION for the same command > i.e. event() first and then completion(). > > Thanks, > Ajish Ok, if the firmware spec states that, then I think it's the right change. Please check and fix SSP case too. Thanks Acked-by: Jack Wang <jinpu.wang@xxxxxxxxx> > > > > > Thanks! > > > > > > > > That's one side effect right now. > > > > > > Another instance I can see is during I/O, host will free the tag once > > > we receive an event, same tag/command is still pending in the firmware > > > and post this event host re-allocates this tag to some other new > > > request. This may lead to some un-expected situation. > > > > > > That is what I can visualize right now as side effects without this > > > patch. > > > > > > Thanks, > > > Ajish > > > >
