Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
> On Thu, Aug 27, 2015 at 7:11 AM, wrote: >>> On Tue, Aug 25, 2015 at 7:36 AM, wrote: > On Aug 21, 2015 3:10 PM, "Yaniv Gardi" wrote: >> >> Add a write memory barrier to make sure descriptors prepared are >> actually >> written to memory before ringing the doorbell. We have also added >> the >> write memory barrier after ringing the doorbell register so that >> controller sees the new request immediately. >> >> Signed-off-by: Yaniv Gardi >> >> --- >> drivers/scsi/ufs/ufshcd.c | 6 ++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index fef0660..876148b 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba, >> unsigned int task_tag) >> ufshcd_clk_scaling_start_busy(hba); >> __set_bit(task_tag, &hba->outstanding_reqs); >> ufshcd_writel(hba, 1 << task_tag, >> REG_UTP_TRANSFER_REQ_DOOR_BELL); >> + /* Make sure that doorbell is committed immediately */ >> + wmb(); > > Is this really necessary? Is there a measurable difference? I'm not sure if there is a measurable difference, but as the Door-Bell register is the one that actually responsible for the HW execution of the requests, anyhow, it's recommended to its value will be written instantly to the memory. >>> >>> A barrier doesn't guarantee speed, only ordering. Unless you can >>> measure the difference, you should not have it. >> >> Rob, >> let me have an example: >> context#1 updates outstanding_reqs variable and write(DOOR_BELL) >> context#2 upon interrupt of a request completion the following happens: >> report completion on each one of the bits in: >> outstanding_reqs ^ read(DOOR_BELL); >> >> 0. let's assume the DOOR_BELL = 0x1 (which means 1 active request in >> slot 0) >> 1. context#1: update the DOOR_BELL to be 0x3; (2 active requests: in >> slot >> 0 and 1) >> 2. the new value 0x3 is still not written to the DR so DORR_BELL is >> still >> 0x1, but outstanding_reqs is already updated = 0x3 >> 3. the request in slot 0 just completed, and interrupt happens, so >> DORR_BELL is now 0 (request in slot 0 completed) >> 4. context#2: outstanding_reqs ^ read(DOOR_BELL) = 0x3 ^ 0x0 = 0x3 => >> wrong conclusion since the request in slot 1 never completed, and >> actually >> never started. > > Barriers alone will never solve this problem. They may narrow the > window possibly, but the problem is still there. What you have to have > is a spinlock around all accesses to both outstanding_reqs and > doorbell register. And guess what, spinlocks have appropriate barriers > to ensure visibility of what they protect. Or perhaps the h/w provides > another way to signal what slots have completed. Using the same > register for doorbell and completion status is not ideal. > can i assume spin_lock_irqsave() and spin_unlock_irqrestore() both provide barriers ? i couldn't find the barrier instruction when following the call chain... > Rob > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
On Thu, Aug 27, 2015 at 7:11 AM, wrote: >> On Tue, Aug 25, 2015 at 7:36 AM, wrote: On Aug 21, 2015 3:10 PM, "Yaniv Gardi" wrote: > > Add a write memory barrier to make sure descriptors prepared are > actually > written to memory before ringing the doorbell. We have also added the > write memory barrier after ringing the doorbell register so that > controller sees the new request immediately. > > Signed-off-by: Yaniv Gardi > > --- > drivers/scsi/ufs/ufshcd.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index fef0660..876148b 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba, > unsigned int task_tag) > ufshcd_clk_scaling_start_busy(hba); > __set_bit(task_tag, &hba->outstanding_reqs); > ufshcd_writel(hba, 1 << task_tag, > REG_UTP_TRANSFER_REQ_DOOR_BELL); > + /* Make sure that doorbell is committed immediately */ > + wmb(); Is this really necessary? Is there a measurable difference? >>> >>> I'm not sure if there is a measurable difference, but as the Door-Bell >>> register is the one that actually responsible for the HW execution of >>> the >>> requests, anyhow, it's recommended to its value will be written >>> instantly to the memory. >> >> A barrier doesn't guarantee speed, only ordering. Unless you can >> measure the difference, you should not have it. > > Rob, > let me have an example: > context#1 updates outstanding_reqs variable and write(DOOR_BELL) > context#2 upon interrupt of a request completion the following happens: > report completion on each one of the bits in: > outstanding_reqs ^ read(DOOR_BELL); > > 0. let's assume the DOOR_BELL = 0x1 (which means 1 active request in slot 0) > 1. context#1: update the DOOR_BELL to be 0x3; (2 active requests: in slot > 0 and 1) > 2. the new value 0x3 is still not written to the DR so DORR_BELL is still > 0x1, but outstanding_reqs is already updated = 0x3 > 3. the request in slot 0 just completed, and interrupt happens, so > DORR_BELL is now 0 (request in slot 0 completed) > 4. context#2: outstanding_reqs ^ read(DOOR_BELL) = 0x3 ^ 0x0 = 0x3 => > wrong conclusion since the request in slot 1 never completed, and actually > never started. Barriers alone will never solve this problem. They may narrow the window possibly, but the problem is still there. What you have to have is a spinlock around all accesses to both outstanding_reqs and doorbell register. And guess what, spinlocks have appropriate barriers to ensure visibility of what they protect. Or perhaps the h/w provides another way to signal what slots have completed. Using the same register for doorbell and completion status is not ideal. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
On Thu, Aug 27, 2015 at 7:28 AM, wrote: >>> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" wrote: Add a write memory barrier to make sure descriptors prepared are actually written to memory before ringing the doorbell. We have also added the write memory barrier after ringing the doorbell register so that controller sees the new request immediately. [...] + /* Make sure descriptors are ready before ringing the doorbell */ + wmb(); >>> >>> The writel for the doorbell will do a barrier first. (I didn't check >>> what exactly ufshcd_writel does, but that is why I don't like these >>> private access wrappers.) > > the barrier here is important and a must. > before it we prepare descriptors. > after it we write to DOOR-BELL. > (and after the DOOR-BELL we have another one.) > if we remove it, we might get the DOOR-BELL written, before the > descriptors written. If you dig into what writel does, you will see that what the code here ends up being is: descriptor setup wmb() __iomwb() -> wmb on arm64 doorbell register write So explain why you need 2 barriers. Barriers in a driver are a red flag. Usually they are not needed, but sometimes they are. You have to be able to explain why if they are. Pretty much every descriptor based DMA device works as you describe and has the ordering problem. Because of that, the core code takes care of this. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
>> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" wrote: >>> >>> Add a write memory barrier to make sure descriptors prepared are >>> actually >>> written to memory before ringing the doorbell. We have also added the >>> write memory barrier after ringing the doorbell register so that >>> controller sees the new request immediately. >>> >>> Signed-off-by: Yaniv Gardi >>> >>> --- >>> drivers/scsi/ufs/ufshcd.c | 6 ++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >>> index fef0660..876148b 100644 >>> --- a/drivers/scsi/ufs/ufshcd.c >>> +++ b/drivers/scsi/ufs/ufshcd.c >>> @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba, >>> unsigned int task_tag) >>> ufshcd_clk_scaling_start_busy(hba); >>> __set_bit(task_tag, &hba->outstanding_reqs); >>> ufshcd_writel(hba, 1 << task_tag, >>> REG_UTP_TRANSFER_REQ_DOOR_BELL); >>> + /* Make sure that doorbell is committed immediately */ >>> + wmb(); >> >> Is this really necessary? Is there a measurable difference? > > I'm not sure if there is a measurable difference, but as the Door-Bell > register is the one that actually responsible for the HW execution of the > requests, anyhow, it's recommended to its value will be written > instantly to the memory. > > Also, as the Interrupt context reads this register, and compare it to the > SW mirroring value (hba->outstanding_reqs) in order to realize what > requests are already completed, it's important to get the correct value > by reading this register, otherwise we might realize a request completion > while it was never even submitted. > >> >>> } >>> >>> /** >>> @@ -1418,6 +1420,8 @@ static int ufshcd_queuecommand(struct Scsi_Host >>> *host, struct scsi_cmnd *cmd) >>> goto out; >>> } >>> >>> + /* Make sure descriptors are ready before ringing the doorbell >>> */ >>> + wmb(); >> >> The writel for the doorbell will do a barrier first. (I didn't check >> what exactly ufshcd_writel does, but that is why I don't like these >> private access wrappers.) the barrier here is important and a must. before it we prepare descriptors. after it we write to DOOR-BELL. (and after the DOOR-BELL we have another one.) if we remove it, we might get the DOOR-BELL written, before the descriptors written. >> >>> /* issue command to the controller */ >>> spin_lock_irqsave(hba->host->host_lock, flags); >>> ufshcd_send_command(hba, tag); >>> @@ -1627,6 +1631,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba >>> *hba, >>> >>> hba->dev_cmd.complete = &wait; >>> >>> + /* Make sure descriptors are ready before ringing the doorbell >>> */ >>> + wmb(); >>> spin_lock_irqsave(hba->host->host_lock, flags); >>> ufshcd_send_command(hba, tag); >>> spin_unlock_irqrestore(hba->host->host_lock, flags); >>> -- >>> 1.8.5.2 >>> >>> -- >>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a >>> member of Code Aurora Forum, hosted by The Linux Foundation >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
> On Tue, Aug 25, 2015 at 7:36 AM, wrote: >>> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" wrote: Add a write memory barrier to make sure descriptors prepared are actually written to memory before ringing the doorbell. We have also added the write memory barrier after ringing the doorbell register so that controller sees the new request immediately. Signed-off-by: Yaniv Gardi --- drivers/scsi/ufs/ufshcd.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index fef0660..876148b 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag) ufshcd_clk_scaling_start_busy(hba); __set_bit(task_tag, &hba->outstanding_reqs); ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL); + /* Make sure that doorbell is committed immediately */ + wmb(); >>> >>> Is this really necessary? Is there a measurable difference? >> >> I'm not sure if there is a measurable difference, but as the Door-Bell >> register is the one that actually responsible for the HW execution of >> the >> requests, anyhow, it's recommended to its value will be written >> instantly to the memory. > > A barrier doesn't guarantee speed, only ordering. Unless you can > measure the difference, you should not have it. Rob, let me have an example: context#1 updates outstanding_reqs variable and write(DOOR_BELL) context#2 upon interrupt of a request completion the following happens: report completion on each one of the bits in: outstanding_reqs ^ read(DOOR_BELL); 0. let's assume the DOOR_BELL = 0x1 (which means 1 active request in slot 0) 1. context#1: update the DOOR_BELL to be 0x3; (2 active requests: in slot 0 and 1) 2. the new value 0x3 is still not written to the DR so DORR_BELL is still 0x1, but outstanding_reqs is already updated = 0x3 3. the request in slot 0 just completed, and interrupt happens, so DORR_BELL is now 0 (request in slot 0 completed) 4. context#2: outstanding_reqs ^ read(DOOR_BELL) = 0x3 ^ 0x0 = 0x3 => wrong conclusion since the request in slot 1 never completed, and actually never started. > >> Also, as the Interrupt context reads this register, and compare it to >> the >> SW mirroring value (hba->outstanding_reqs) in order to realize what >> requests are already completed, it's important to get the correct value >> by reading this register, otherwise we might realize a request >> completion >> while it was never even submitted. > > If a register read can pass a register write out of order, then your > h/w is broken. Plus what if the interrupt occurs before the barrier. > > Rob > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
On Tue, Aug 25, 2015 at 7:36 AM, wrote: >> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" wrote: >>> >>> Add a write memory barrier to make sure descriptors prepared are >>> actually >>> written to memory before ringing the doorbell. We have also added the >>> write memory barrier after ringing the doorbell register so that >>> controller sees the new request immediately. >>> >>> Signed-off-by: Yaniv Gardi >>> >>> --- >>> drivers/scsi/ufs/ufshcd.c | 6 ++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >>> index fef0660..876148b 100644 >>> --- a/drivers/scsi/ufs/ufshcd.c >>> +++ b/drivers/scsi/ufs/ufshcd.c >>> @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba, >>> unsigned int task_tag) >>> ufshcd_clk_scaling_start_busy(hba); >>> __set_bit(task_tag, &hba->outstanding_reqs); >>> ufshcd_writel(hba, 1 << task_tag, >>> REG_UTP_TRANSFER_REQ_DOOR_BELL); >>> + /* Make sure that doorbell is committed immediately */ >>> + wmb(); >> >> Is this really necessary? Is there a measurable difference? > > I'm not sure if there is a measurable difference, but as the Door-Bell > register is the one that actually responsible for the HW execution of the > requests, anyhow, it's recommended to its value will be written > instantly to the memory. A barrier doesn't guarantee speed, only ordering. Unless you can measure the difference, you should not have it. > Also, as the Interrupt context reads this register, and compare it to the > SW mirroring value (hba->outstanding_reqs) in order to realize what > requests are already completed, it's important to get the correct value > by reading this register, otherwise we might realize a request completion > while it was never even submitted. If a register read can pass a register write out of order, then your h/w is broken. Plus what if the interrupt occurs before the barrier. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" wrote: >> >> Add a write memory barrier to make sure descriptors prepared are >> actually >> written to memory before ringing the doorbell. We have also added the >> write memory barrier after ringing the doorbell register so that >> controller sees the new request immediately. >> >> Signed-off-by: Yaniv Gardi >> >> --- >> drivers/scsi/ufs/ufshcd.c | 6 ++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index fef0660..876148b 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba, >> unsigned int task_tag) >> ufshcd_clk_scaling_start_busy(hba); >> __set_bit(task_tag, &hba->outstanding_reqs); >> ufshcd_writel(hba, 1 << task_tag, >> REG_UTP_TRANSFER_REQ_DOOR_BELL); >> + /* Make sure that doorbell is committed immediately */ >> + wmb(); > > Is this really necessary? Is there a measurable difference? I'm not sure if there is a measurable difference, but as the Door-Bell register is the one that actually responsible for the HW execution of the requests, anyhow, it's recommended to its value will be written instantly to the memory. Also, as the Interrupt context reads this register, and compare it to the SW mirroring value (hba->outstanding_reqs) in order to realize what requests are already completed, it's important to get the correct value by reading this register, otherwise we might realize a request completion while it was never even submitted. > >> } >> >> /** >> @@ -1418,6 +1420,8 @@ static int ufshcd_queuecommand(struct Scsi_Host >> *host, struct scsi_cmnd *cmd) >> goto out; >> } >> >> + /* Make sure descriptors are ready before ringing the doorbell >> */ >> + wmb(); > > The writel for the doorbell will do a barrier first. (I didn't check > what exactly ufshcd_writel does, but that is why I don't like these > private access wrappers.) > >> /* issue command to the controller */ >> spin_lock_irqsave(hba->host->host_lock, flags); >> ufshcd_send_command(hba, tag); >> @@ -1627,6 +1631,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba >> *hba, >> >> hba->dev_cmd.complete = &wait; >> >> + /* Make sure descriptors are ready before ringing the doorbell >> */ >> + wmb(); >> spin_lock_irqsave(hba->host->host_lock, flags); >> ufshcd_send_command(hba, tag); >> spin_unlock_irqrestore(hba->host->host_lock, flags); >> -- >> 1.8.5.2 >> >> -- >> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a >> member of Code Aurora Forum, hosted by The Linux Foundation > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
On Aug 21, 2015 3:10 PM, "Yaniv Gardi" wrote: > > Add a write memory barrier to make sure descriptors prepared are actually > written to memory before ringing the doorbell. We have also added the > write memory barrier after ringing the doorbell register so that > controller sees the new request immediately. > > Signed-off-by: Yaniv Gardi > > --- > drivers/scsi/ufs/ufshcd.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index fef0660..876148b 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned > int task_tag) > ufshcd_clk_scaling_start_busy(hba); > __set_bit(task_tag, &hba->outstanding_reqs); > ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL); > + /* Make sure that doorbell is committed immediately */ > + wmb(); Is this really necessary? Is there a measurable difference? > } > > /** > @@ -1418,6 +1420,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, > struct scsi_cmnd *cmd) > goto out; > } > > + /* Make sure descriptors are ready before ringing the doorbell */ > + wmb(); The writel for the doorbell will do a barrier first. (I didn't check what exactly ufshcd_writel does, but that is why I don't like these private access wrappers.) > /* issue command to the controller */ > spin_lock_irqsave(hba->host->host_lock, flags); > ufshcd_send_command(hba, tag); > @@ -1627,6 +1631,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba, > > hba->dev_cmd.complete = &wait; > > + /* Make sure descriptors are ready before ringing the doorbell */ > + wmb(); > spin_lock_irqsave(hba->host->host_lock, flags); > ufshcd_send_command(hba, tag); > spin_unlock_irqrestore(hba->host->host_lock, flags); > -- > 1.8.5.2 > > -- > QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of > Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
