Re: [PATCH] Bugfix for handling of shadow doorbell buffer.

2018-08-17 Thread Christoph Hellwig
On Tue, Aug 14, 2018 at 06:35:16PM -0700, Michal Wnukowski wrote: > > The other side in this case is not actual controller hardware, but > virtual one (the regular hardware should rely on normal MMIO > doorbells). There could very much be real hardware there. We've made it clear in the spec

Re: [PATCH] Bugfix for handling of shadow doorbell buffer.

2018-08-17 Thread Christoph Hellwig
On Tue, Aug 14, 2018 at 06:35:16PM -0700, Michal Wnukowski wrote: > > The other side in this case is not actual controller hardware, but > virtual one (the regular hardware should rely on normal MMIO > doorbells). There could very much be real hardware there. We've made it clear in the spec

Re: [PATCH] Bugfix for handling of shadow doorbell buffer.

2018-08-14 Thread Linus Torvalds
On Tue, Aug 14, 2018 at 6:35 PM Michal Wnukowski wrote: > > I got confused after comaring disassembly of this code with and > without volatile keyword. Thanks for the correction. Note that _usually_, the "volatile" has absolutely no impact. When there is one read in the source code, it's almost

Re: [PATCH] Bugfix for handling of shadow doorbell buffer.

2018-08-14 Thread Linus Torvalds
On Tue, Aug 14, 2018 at 6:35 PM Michal Wnukowski wrote: > > I got confused after comaring disassembly of this code with and > without volatile keyword. Thanks for the correction. Note that _usually_, the "volatile" has absolutely no impact. When there is one read in the source code, it's almost

Re: [PATCH] Bugfix for handling of shadow doorbell buffer.

2018-08-14 Thread Michal Wnukowski
On 08/14/2018 04:16 PM, Linus Torvalds wrote: > On Tue, Aug 14, 2018 at 03:17:35PM -0700, Michal Wnukowski wrote: >> >> With memory barrier in place, the volatile keyword around *dbbuf_ei is >> redundant. > > No. The memory barrier enforces _ordering_, but it doesn't enforce > that the

Re: [PATCH] Bugfix for handling of shadow doorbell buffer.

2018-08-14 Thread Michal Wnukowski
On 08/14/2018 04:16 PM, Linus Torvalds wrote: > On Tue, Aug 14, 2018 at 03:17:35PM -0700, Michal Wnukowski wrote: >> >> With memory barrier in place, the volatile keyword around *dbbuf_ei is >> redundant. > > No. The memory barrier enforces _ordering_, but it doesn't enforce > that the

Re: [PATCH] Bugfix for handling of shadow doorbell buffer.

2018-08-14 Thread Keith Busch
On Tue, Aug 14, 2018 at 04:16:41PM -0700, Linus Torvalds wrote: > On Tue, Aug 14, 2018 at 3:56 PM Keith Busch > wrote: > > > > You just want to ensure the '*dbbuf_db = value' isn't reordered, right? > > The order dependency might be more obvious if done as: > > > > WRITE_ONCE(*dbbuf_db,

Re: [PATCH] Bugfix for handling of shadow doorbell buffer.

2018-08-14 Thread Keith Busch
On Tue, Aug 14, 2018 at 04:16:41PM -0700, Linus Torvalds wrote: > On Tue, Aug 14, 2018 at 3:56 PM Keith Busch > wrote: > > > > You just want to ensure the '*dbbuf_db = value' isn't reordered, right? > > The order dependency might be more obvious if done as: > > > > WRITE_ONCE(*dbbuf_db,

Re: [PATCH] Bugfix for handling of shadow doorbell buffer.

2018-08-14 Thread Linus Torvalds
Guys, you're both wrong. On Tue, Aug 14, 2018 at 03:17:35PM -0700, Michal Wnukowski wrote: > > With memory barrier in place, the volatile keyword around *dbbuf_ei is > redundant. No. The memory barrier enforces _ordering_, but it doesn't enforce that the accesses are only done once. So when you

Re: [PATCH] Bugfix for handling of shadow doorbell buffer.

2018-08-14 Thread Linus Torvalds
Guys, you're both wrong. On Tue, Aug 14, 2018 at 03:17:35PM -0700, Michal Wnukowski wrote: > > With memory barrier in place, the volatile keyword around *dbbuf_ei is > redundant. No. The memory barrier enforces _ordering_, but it doesn't enforce that the accesses are only done once. So when you

Re: [PATCH] Bugfix for handling of shadow doorbell buffer.

2018-08-14 Thread Keith Busch
On Tue, Aug 14, 2018 at 03:17:35PM -0700, Michal Wnukowski wrote: > /* Update dbbuf and return true if an MMIO is required */ > static bool nvme_dbbuf_update_and_check_event(u16 value, u32 *dbbuf_db, > - volatile u32 *dbbuf_ei) > +

Re: [PATCH] Bugfix for handling of shadow doorbell buffer.

2018-08-14 Thread Keith Busch
On Tue, Aug 14, 2018 at 03:17:35PM -0700, Michal Wnukowski wrote: > /* Update dbbuf and return true if an MMIO is required */ > static bool nvme_dbbuf_update_and_check_event(u16 value, u32 *dbbuf_db, > - volatile u32 *dbbuf_ei) > +

[PATCH] Bugfix for handling of shadow doorbell buffer.

2018-08-14 Thread Michal Wnukowski
This patch adds full memory barrier into nvme_dbbuf_update_and_check_event function to ensure that the shadow doorbell is written before reading EventIdx from memory. This is a critical bugfix for initial patch that added support for shadow doorbell into NVMe driver

[PATCH] Bugfix for handling of shadow doorbell buffer.

2018-08-14 Thread Michal Wnukowski
This patch adds full memory barrier into nvme_dbbuf_update_and_check_event function to ensure that the shadow doorbell is written before reading EventIdx from memory. This is a critical bugfix for initial patch that added support for shadow doorbell into NVMe driver