| Message ID | 20220124082255.86223-1-Ajish.Koshy@microchip.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | scsi: pm80xx: Fix double completion for SATA devices | expand |
On Mon, Jan 24, 2022 at 9:20 AM Ajish Koshy <Ajish.Koshy@microchip.com> wrote: > > 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! > > There are some events for which we get > sata_completions, for some needs recovery > procedure and for others abort. > > All the tasks must be completed via > sata_completion() path. > > Have just removed the task done related code > from sata_events(). > For tasks where we don't get completions, > let top layer call abort() to abort the > command post timeout. > > Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com> > Signed-off-by: Viswas G <Viswas.G@microchip.com> > --- > drivers/scsi/pm8001/pm8001_hwi.c | 18 ------------------ > drivers/scsi/pm8001/pm80xx_hwi.c | 26 -------------------------- > 2 files changed, 44 deletions(-) > > diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c > index c814e5071712..9ec310b795c3 100644 > --- a/drivers/scsi/pm8001/pm8001_hwi.c > +++ b/drivers/scsi/pm8001/pm8001_hwi.c > @@ -2692,7 +2692,6 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb) > u32 tag = le32_to_cpu(psataPayload->tag); > u32 port_id = le32_to_cpu(psataPayload->port_id); > u32 dev_id = le32_to_cpu(psataPayload->device_id); > - unsigned long flags; > > if (event) > pm8001_dbg(pm8001_ha, FAIL, "SATA EVENT 0x%x\n", event); > @@ -2724,8 +2723,6 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb) > ts->resp = SAS_TASK_COMPLETE; > ts->stat = SAS_DATA_OVERRUN; > ts->residual = 0; > - if (pm8001_dev) > - atomic_dec(&pm8001_dev->running_req); > break; > case IO_XFER_ERROR_BREAK: > pm8001_dbg(pm8001_ha, IO, "IO_XFER_ERROR_BREAK\n"); > @@ -2767,7 +2764,6 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb) > IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS); > ts->resp = SAS_TASK_COMPLETE; > ts->stat = SAS_QUEUE_FULL; > - pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag); > return; > } > break; > @@ -2853,20 +2849,6 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb) > ts->stat = SAS_OPEN_TO; > break; > } > - spin_lock_irqsave(&t->task_state_lock, flags); > - t->task_state_flags &= ~SAS_TASK_STATE_PENDING; > - t->task_state_flags &= ~SAS_TASK_AT_INITIATOR; > - t->task_state_flags |= SAS_TASK_STATE_DONE; > - if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { > - spin_unlock_irqrestore(&t->task_state_lock, flags); > - pm8001_dbg(pm8001_ha, FAIL, > - "task 0x%p done with io_status 0x%x resp 0x%x stat 0x%x but aborted by upper layer!\n", > - t, event, ts->resp, ts->stat); > - pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > - } else { > - spin_unlock_irqrestore(&t->task_state_lock, flags); > - pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag); > - } > } > > /*See the comments for mpi_ssp_completion */ > diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c > index 0849ecc913c7..1e573be04407 100644 > --- a/drivers/scsi/pm8001/pm80xx_hwi.c > +++ b/drivers/scsi/pm8001/pm80xx_hwi.c > @@ -2821,7 +2821,6 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, > u32 tag = le32_to_cpu(psataPayload->tag); > u32 port_id = le32_to_cpu(psataPayload->port_id); > u32 dev_id = le32_to_cpu(psataPayload->device_id); > - unsigned long flags; > > if (event) > pm8001_dbg(pm8001_ha, FAIL, "SATA EVENT 0x%x\n", event); > @@ -2854,8 +2853,6 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, > ts->resp = SAS_TASK_COMPLETE; > ts->stat = SAS_DATA_OVERRUN; > ts->residual = 0; > - if (pm8001_dev) > - atomic_dec(&pm8001_dev->running_req); > break; > case IO_XFER_ERROR_BREAK: > pm8001_dbg(pm8001_ha, IO, "IO_XFER_ERROR_BREAK\n"); > @@ -2904,11 +2901,6 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, > IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS); > ts->resp = SAS_TASK_COMPLETE; > ts->stat = SAS_QUEUE_FULL; > - spin_unlock_irqrestore(&circularQ->oq_lock, > - circularQ->lock_flags); > - pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag); > - spin_lock_irqsave(&circularQ->oq_lock, > - circularQ->lock_flags); > return; > } > break; > @@ -3008,24 +3000,6 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, > ts->stat = SAS_OPEN_TO; > break; > } > - spin_lock_irqsave(&t->task_state_lock, flags); > - t->task_state_flags &= ~SAS_TASK_STATE_PENDING; > - t->task_state_flags &= ~SAS_TASK_AT_INITIATOR; > - t->task_state_flags |= SAS_TASK_STATE_DONE; > - if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { > - spin_unlock_irqrestore(&t->task_state_lock, flags); > - pm8001_dbg(pm8001_ha, FAIL, > - "task 0x%p done with io_status 0x%x resp 0x%x stat 0x%x but aborted by upper layer!\n", > - t, event, ts->resp, ts->stat); > - pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > - } else { > - spin_unlock_irqrestore(&t->task_state_lock, flags); > - spin_unlock_irqrestore(&circularQ->oq_lock, > - circularQ->lock_flags); > - pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag); > - spin_lock_irqsave(&circularQ->oq_lock, > - circularQ->lock_flags); > - } > } > > /*See the comments for mpi_ssp_completion */ > -- > 2.27.0 >
> > > > 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. 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. 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
On Tue, Jan 25, 2022 at 11:58 AM <Ajish.Koshy@microchip.com> wrote: > > > > > > > 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. > 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? 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 >
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 > >
On Thu, Jan 27, 2022 at 1:08 PM <Ajish.Koshy@microchip.com> 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@ionos.com> > > > > > 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 > > > >
Ajish,
> For SATA devices, correct the double completion issue.
Applied to 5.17/scsi-fixes, thanks!
On Mon, 24 Jan 2022 13:52:55 +0530, Ajish Koshy wrote: > For SATA devices, correct the double > completion issue. > > Current code handles completions for sata > devices in mpi_sata_completion() and > mpi_sata_event(). > > [...] Applied to 5.17/scsi-fixes, thanks! [1/1] scsi: pm80xx: Fix double completion for SATA devices https://git.kernel.org/mkp/scsi/c/c26b85ea1636
diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c index c814e5071712..9ec310b795c3 100644 --- a/drivers/scsi/pm8001/pm8001_hwi.c +++ b/drivers/scsi/pm8001/pm8001_hwi.c @@ -2692,7 +2692,6 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb) u32 tag = le32_to_cpu(psataPayload->tag); u32 port_id = le32_to_cpu(psataPayload->port_id); u32 dev_id = le32_to_cpu(psataPayload->device_id); - unsigned long flags; if (event) pm8001_dbg(pm8001_ha, FAIL, "SATA EVENT 0x%x\n", event); @@ -2724,8 +2723,6 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb) ts->resp = SAS_TASK_COMPLETE; ts->stat = SAS_DATA_OVERRUN; ts->residual = 0; - if (pm8001_dev) - atomic_dec(&pm8001_dev->running_req); break; case IO_XFER_ERROR_BREAK: pm8001_dbg(pm8001_ha, IO, "IO_XFER_ERROR_BREAK\n"); @@ -2767,7 +2764,6 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb) IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS); ts->resp = SAS_TASK_COMPLETE; ts->stat = SAS_QUEUE_FULL; - pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag); return; } break; @@ -2853,20 +2849,6 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb) ts->stat = SAS_OPEN_TO; break; } - spin_lock_irqsave(&t->task_state_lock, flags); - t->task_state_flags &= ~SAS_TASK_STATE_PENDING; - t->task_state_flags &= ~SAS_TASK_AT_INITIATOR; - t->task_state_flags |= SAS_TASK_STATE_DONE; - if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { - spin_unlock_irqrestore(&t->task_state_lock, flags); - pm8001_dbg(pm8001_ha, FAIL, - "task 0x%p done with io_status 0x%x resp 0x%x stat 0x%x but aborted by upper layer!\n", - t, event, ts->resp, ts->stat); - pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); - } else { - spin_unlock_irqrestore(&t->task_state_lock, flags); - pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag); - } } /*See the comments for mpi_ssp_completion */ diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c index 0849ecc913c7..1e573be04407 100644 --- a/drivers/scsi/pm8001/pm80xx_hwi.c +++ b/drivers/scsi/pm8001/pm80xx_hwi.c @@ -2821,7 +2821,6 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, u32 tag = le32_to_cpu(psataPayload->tag); u32 port_id = le32_to_cpu(psataPayload->port_id); u32 dev_id = le32_to_cpu(psataPayload->device_id); - unsigned long flags; if (event) pm8001_dbg(pm8001_ha, FAIL, "SATA EVENT 0x%x\n", event); @@ -2854,8 +2853,6 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, ts->resp = SAS_TASK_COMPLETE; ts->stat = SAS_DATA_OVERRUN; ts->residual = 0; - if (pm8001_dev) - atomic_dec(&pm8001_dev->running_req); break; case IO_XFER_ERROR_BREAK: pm8001_dbg(pm8001_ha, IO, "IO_XFER_ERROR_BREAK\n"); @@ -2904,11 +2901,6 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS); ts->resp = SAS_TASK_COMPLETE; ts->stat = SAS_QUEUE_FULL; - spin_unlock_irqrestore(&circularQ->oq_lock, - circularQ->lock_flags); - pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag); - spin_lock_irqsave(&circularQ->oq_lock, - circularQ->lock_flags); return; } break; @@ -3008,24 +3000,6 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, ts->stat = SAS_OPEN_TO; break; } - spin_lock_irqsave(&t->task_state_lock, flags); - t->task_state_flags &= ~SAS_TASK_STATE_PENDING; - t->task_state_flags &= ~SAS_TASK_AT_INITIATOR; - t->task_state_flags |= SAS_TASK_STATE_DONE; - if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) { - spin_unlock_irqrestore(&t->task_state_lock, flags); - pm8001_dbg(pm8001_ha, FAIL, - "task 0x%p done with io_status 0x%x resp 0x%x stat 0x%x but aborted by upper layer!\n", - t, event, ts->resp, ts->stat); - pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); - } else { - spin_unlock_irqrestore(&t->task_state_lock, flags); - spin_unlock_irqrestore(&circularQ->oq_lock, - circularQ->lock_flags); - pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag); - spin_lock_irqsave(&circularQ->oq_lock, - circularQ->lock_flags); - } } /*See the comments for mpi_ssp_completion */