From: Klaus Jensen <[email protected]>
When using shadow doorbells, the event index and the doorbell values are
written to host memory. Prior to this patch, the values written would
erroneously be written in host endianness. This causes trouble on
big-endian platforms. Fix this by adding missing endian conversions.
This issue was noticed by Guenter while testing various big-endian
platforms under QEMU[1]. A similar fix required for hw/nvme in QEMU is
up for review as well[2].
[1]: https://lore.kernel.org/qemu-devel/[email protected]/
[2]: https://lore.kernel.org/qemu-devel/[email protected]/
Fixes: f9f38e33389c ("nvme: improve performance for virtual NVMe devices")
Reported-by: Guenter Roeck <[email protected]>
Signed-off-by: Klaus Jensen <[email protected]>
---
drivers/nvme/host/pci.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index f005e8569266..d12d4c3cb6d0 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -347,7 +347,7 @@ static bool nvme_dbbuf_update_and_check_event(u16 value, u32 *dbbuf_db,
volatile u32 *dbbuf_ei)
{
if (dbbuf_db) {
- u16 old_value;
+ u16 old_value, event_idx;
/*
* Ensure that the queue is written before updating
@@ -355,8 +355,8 @@ static bool nvme_dbbuf_update_and_check_event(u16 value, u32 *dbbuf_db,
*/
wmb();
- old_value = *dbbuf_db;
- *dbbuf_db = value;
+ old_value = le32_to_cpu(*dbbuf_db);
+ *dbbuf_db = cpu_to_le32(value);
/*
* Ensure that the doorbell is updated before reading the event
@@ -366,7 +366,9 @@ static bool nvme_dbbuf_update_and_check_event(u16 value, u32 *dbbuf_db,
*/
mb();
- if (!nvme_dbbuf_need_event(*dbbuf_ei, value, old_value))
+ event_idx = le32_to_cpu(*dbbuf_ei);
+
+ if (!nvme_dbbuf_need_event(event_idx, value, old_value))
return false;
}
--
2.38.1
Hi Klaus,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on hch-configfs/for-next]
[also build test WARNING on linus/master v6.1 next-20221208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Klaus-Jensen/nvme-pci-fix-doorbell-buffer-value-endianness/20221212-230406
base: git://git.infradead.org/users/hch/configfs.git for-next
patch link: https://lore.kernel.org/r/20221212150243.41283-1-its%40irrelevant.dk
patch subject: [PATCH] nvme-pci: fix doorbell buffer value endianness
config: microblaze-randconfig-s041-20221211
compiler: microblaze-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/1909d9281256482585e9ba9a34bd284684dfe6a1
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Klaus-Jensen/nvme-pci-fix-doorbell-buffer-value-endianness/20221212-230406
git checkout 1909d9281256482585e9ba9a34bd284684dfe6a1
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/nvme/host/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> drivers/nvme/host/pci.c:355:29: sparse: sparse: cast to restricted __le32
>> drivers/nvme/host/pci.c:356:27: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [usertype] @@ got restricted __le32 [usertype] @@
drivers/nvme/host/pci.c:356:27: sparse: expected unsigned int [usertype]
drivers/nvme/host/pci.c:356:27: sparse: got restricted __le32 [usertype]
drivers/nvme/host/pci.c:366:29: sparse: sparse: cast to restricted __le32
vim +355 drivers/nvme/host/pci.c
341
342 /* Update dbbuf and return true if an MMIO is required */
343 static bool nvme_dbbuf_update_and_check_event(u16 value, u32 *dbbuf_db,
344 volatile u32 *dbbuf_ei)
345 {
346 if (dbbuf_db) {
347 u16 old_value, event_idx;
348
349 /*
350 * Ensure that the queue is written before updating
351 * the doorbell in memory
352 */
353 wmb();
354
> 355 old_value = le32_to_cpu(*dbbuf_db);
> 356 *dbbuf_db = cpu_to_le32(value);
357
358 /*
359 * Ensure that the doorbell is updated before reading the event
360 * index from memory. The controller needs to provide similar
361 * ordering to ensure the envent index is updated before reading
362 * the doorbell.
363 */
364 mb();
365
366 event_idx = le32_to_cpu(*dbbuf_ei);
367
368 if (!nvme_dbbuf_need_event(event_idx, value, old_value))
369 return false;
370 }
371
372 return true;
373 }
374
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Mon, Dec 12, 2022 at 04:02:43PM +0100, Klaus Jensen wrote:
> - old_value = *dbbuf_db;
> - *dbbuf_db = value;
> + old_value = le32_to_cpu(*dbbuf_db);
> + *dbbuf_db = cpu_to_le32(value);
As the buildbot noticed, this now means dbbuf_dbs needs to be
a __le32, and the endianess annoations will have to wee through
quite a few places.
On Dec 13 09:50, Christoph Hellwig wrote:
> On Mon, Dec 12, 2022 at 04:02:43PM +0100, Klaus Jensen wrote:
> > - old_value = *dbbuf_db;
> > - *dbbuf_db = value;
> > + old_value = le32_to_cpu(*dbbuf_db);
> > + *dbbuf_db = cpu_to_le32(value);
>
> As the buildbot noticed, this now means dbbuf_dbs needs to be
> a __le32, and the endianess annoations will have to wee through
> quite a few places.
>
Yes, I'll fix it :)