Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Can Guo <cang@codeaurora.org>
To: Stanley Chu <stanley.chu@mediatek.com>
Cc: Bart Van Assche <bvanassche@acm.org>,
	linux-scsi@vger.kernel.org, andy.teng@mediatek.com,
	jejb@linux.ibm.com, chun-hung.wu@mediatek.com,
	kuohong.wang@mediatek.com, linux-kernel@vger.kernel.org,
	Avri Altman <Avri.Altman@wdc.com>,
	linux-mediatek@lists.infradead.org, peter.wang@mediatek.com,
	alim.akhtar@samsung.com, martin.petersen@oracle.com,
	matthias.bgg@gmail.com, asutoshd@codeaurora.org,
	chaotian.jing@mediatek.com, cc.chou@mediatek.com,
	linux-arm-kernel@lists.infradead.org, beanhuo@micron.com
Subject: Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification
Date: Mon, 03 Aug 2020 13:14:02 +0800
Message-ID: <3b144ed6897483d1ae3ced6de2dfc64c@codeaurora.org> (raw)
In-Reply-To: <1596423655.32283.7.camel@mtkswgap22>

Hi Stanley,

On 2020-08-03 11:00, Stanley Chu wrote:
> Hi Can,
> 
> On Sat, 2020-08-01 at 07:17 +0800, Can Guo wrote:
>> Hi Bart,
>> 
>> On 2020-08-01 00:51, Bart Van Assche wrote:
>> > On 2020-07-31 01:00, Can Guo wrote:
>> >> AFAIK, sychronization of scsi_done is not a problem here, because scsi
>> >> layer
>> >> use the atomic state, namely SCMD_STATE_COMPLETE, of a scsi cmd to
>> >> prevent
>> >> the concurrency of abort and real completion of it.
>> >>
>> >> Check func scsi_times_out(), hope it helps.
>> >>
>> >> enum blk_eh_timer_return scsi_times_out(struct request *req)
>> >> {
>> >> ...
>> >>         if (rtn == BLK_EH_DONE) {
>> >>                 /*
>> >>                  * Set the command to complete first in order to
>> >> prevent
>> >> a real
>> >>                  * completion from releasing the command while error
>> >> handling
>> >>                  * is using it. If the command was already completed,
>> >> then the
>> >>                  * lower level driver beat the timeout handler, and it
>> >> is safe
>> >>                  * to return without escalating error recovery.
>> >>                  *
>> >>                  * If timeout handling lost the race to a real
>> >> completion, the
>> >>                  * block layer may ignore that due to a fake timeout
>> >> injection,
>> >>                  * so return RESET_TIMER to allow error handling
>> >> another
>> >> shot
>> >>                  * at this command.
>> >>                  */
>> >>                 if (test_and_set_bit(SCMD_STATE_COMPLETE,
>> >> &scmd->state))
>> >>                         return BLK_EH_RESET_TIMER;
>> >>                 if (scsi_abort_command(scmd) != SUCCESS) {
>> >>                         set_host_byte(scmd, DID_TIME_OUT);
>> >>                         scsi_eh_scmd_add(scmd);
>> >>                 }
>> >>         }
>> >> }
>> >
>> > I am familiar with this mechanism. My concern is that both the regular
>> > completion path and the abort handler must call scsi_dma_unmap() before
>> > calling cmd->scsi_done(cmd). I don't see how
>> > test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state) could prevent that
>> > the regular completion path and the abort handler call scsi_dma_unmap()
>> > concurrently since both calls happen before the SCMD_STATE_COMPLETE bit
>> > is set?
>> >
>> > Thanks,
>> >
>> > Bart.
>> 
>> For scsi_dma_unmap() part, that is true - we should make it serialized
>> with
>> any other completion paths. I've found it during my fault injection
>> test, so
>> I've made a patch to fix it, but it only comes in my next error 
>> recovery
>> enhancement patch series. Please check the attachment.
>> 
> 
> Your patch looks good to me.
> 
> I have the same idea before but I found that calling scsi_done() (by
> __ufshcd_transfer_req_compl()) in ufshcd_abort() in old kernel (e.g.,
> 4.14) will cause issues but it has been resolved by introduced
> SCMD_STATE_COMPLETE flag in newer kernel. So your patch makes sense.
> 
> Would you mind sending out this draft patch as a formal patch together
> with my patch to fix issues in ufshcd_abort()? Our patches are aimed to
> fix cases that host/device reset eventually not being triggered by the
> result of ufshcd_abort(), for example, command is aborted successfully
> or command is not pending in device with its doorbell also cleared.
> 
> Thanks,
> Stanley Chu
> 

I don't quite actually follow your fix here and I didn't test the 
similar
fault injection scenario like you do here, so I am not sure if I should
just absorb your fix into mine. How about I put my fix in my current 
error
recovery patch series (maybe in next version of it) and you can give 
your
review. So you can still go with your fix as it is. Mine will be picked 
up
later by Martin. What do you think?

Thanks,

Can Guo.

>> Thanks,
>> 
>> Can Guo.
>> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-24 14:02 Stanley Chu
2020-07-27 11:18 ` Avri Altman
2020-07-31  1:30   ` Stanley Chu
2020-07-31  4:06     ` Bart Van Assche
2020-07-31  8:00       ` Can Guo
2020-07-31 16:51         ` Bart Van Assche
2020-07-31 23:17           ` Can Guo
2020-08-03  3:00             ` Stanley Chu
2020-08-03  5:14               ` Can Guo [this message]
2020-08-03  5:27                 ` Stanley Chu
2020-08-03  3:12             ` Bart Van Assche
2020-08-03  5:07               ` Can Guo
2020-08-04 10:01 ` Can Guo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3b144ed6897483d1ae3ced6de2dfc64c@codeaurora.org \
    --to=cang@codeaurora.org \
    --cc=Avri.Altman@wdc.com \
    --cc=alim.akhtar@samsung.com \
    --cc=andy.teng@mediatek.com \
    --cc=asutoshd@codeaurora.org \
    --cc=beanhuo@micron.com \
    --cc=bvanassche@acm.org \
    --cc=cc.chou@mediatek.com \
    --cc=chaotian.jing@mediatek.com \
    --cc=chun-hung.wu@mediatek.com \
    --cc=jejb@linux.ibm.com \
    --cc=kuohong.wang@mediatek.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=matthias.bgg@gmail.com \
    --cc=peter.wang@mediatek.com \
    --cc=stanley.chu@mediatek.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git