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 > > 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 > >
