| Message ID | 20201027135547.374946-4-philmd@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | block/nvme: Fix Aarch64 host | expand |
On Tue, Oct 27, 2020 at 02:55:25PM +0100, Philippe Mathieu-Daudé wrote: > Instead of displaying warning on stderr, use warn_report() > which also displays it on the monitor. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > block/nvme.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/block/nvme.c b/block/nvme.c > index 739a0a700cb..6f1d7f9b2a1 100644 > --- a/block/nvme.c > +++ b/block/nvme.c > @@ -399,8 +399,8 @@ static bool nvme_process_completion(NVMeQueuePair *q) > } > cid = le16_to_cpu(c->cid); Not related to your patch, but it stands out as odd that this is treated as an endian type. The field is just an opaque cookie, so there shouldn't be a need for byte swapping. It in fact looks like this is broken on a big-endian host, as the swaping on submission uses a 32-bit value. Won't that truncate the relavant bits? > if (cid == 0 || cid > NVME_QUEUE_SIZE) { > - fprintf(stderr, "Unexpected CID in completion queue: %" PRIu32 "\n", > - cid); > + warn_report("NVMe: Unexpected CID in completion queue: %"PRIu32", " > + "queue size: %u", cid, NVME_QUEUE_SIZE); > continue; > } > trace_nvme_complete_command(s, q->index, cid);
On 10/27/20 3:45 PM, Keith Busch wrote: > On Tue, Oct 27, 2020 at 02:55:25PM +0100, Philippe Mathieu-Daudé wrote: >> Instead of displaying warning on stderr, use warn_report() >> which also displays it on the monitor. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> block/nvme.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/block/nvme.c b/block/nvme.c >> index 739a0a700cb..6f1d7f9b2a1 100644 >> --- a/block/nvme.c >> +++ b/block/nvme.c >> @@ -399,8 +399,8 @@ static bool nvme_process_completion(NVMeQueuePair *q) >> } >> cid = le16_to_cpu(c->cid); > > Not related to your patch, but it stands out as odd that this is treated > as an endian type. The field is just an opaque cookie, so there shouldn't > be a need for byte swapping. It in fact looks like this is broken on a > big-endian host, as the swaping on submission uses a 32-bit value. Won't > that truncate the relavant bits? You are right, thanks for having a look and catching this bug :) I suppose we never tested on big-endian host yet. > >> if (cid == 0 || cid > NVME_QUEUE_SIZE) { >> - fprintf(stderr, "Unexpected CID in completion queue: %" PRIu32 "\n", >> - cid); >> + warn_report("NVMe: Unexpected CID in completion queue: %"PRIu32", " >> + "queue size: %u", cid, NVME_QUEUE_SIZE); >> continue; >> } >> trace_nvme_complete_command(s, q->index, cid); >
On 10/27/20 4:33 PM, Philippe Mathieu-Daudé wrote: > On 10/27/20 3:45 PM, Keith Busch wrote: >> On Tue, Oct 27, 2020 at 02:55:25PM +0100, Philippe Mathieu-Daudé wrote: >>> Instead of displaying warning on stderr, use warn_report() >>> which also displays it on the monitor. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> block/nvme.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/block/nvme.c b/block/nvme.c >>> index 739a0a700cb..6f1d7f9b2a1 100644 >>> --- a/block/nvme.c >>> +++ b/block/nvme.c >>> @@ -399,8 +399,8 @@ static bool nvme_process_completion(NVMeQueuePair *q) >>> } >>> cid = le16_to_cpu(c->cid); >> >> Not related to your patch, but it stands out as odd that this is treated >> as an endian type. The field is just an opaque cookie, so there shouldn't >> be a need for byte swapping. It in fact looks like this is broken on a >> big-endian host, as the swaping on submission uses a 32-bit value. Won't >> that truncate the relavant bits? > > You are right, thanks for having a look and catching this bug :) > > I suppose we never tested on big-endian host yet. FYI we barely have 64-bit testing on x86_64 and aarch64.
Hi Philippe, On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote: > Instead of displaying warning on stderr, use warn_report() > which also displays it on the monitor. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks Eric > --- > block/nvme.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/block/nvme.c b/block/nvme.c > index 739a0a700cb..6f1d7f9b2a1 100644 > --- a/block/nvme.c > +++ b/block/nvme.c > @@ -399,8 +399,8 @@ static bool nvme_process_completion(NVMeQueuePair *q) > } > cid = le16_to_cpu(c->cid); > if (cid == 0 || cid > NVME_QUEUE_SIZE) { > - fprintf(stderr, "Unexpected CID in completion queue: %" PRIu32 "\n", > - cid); > + warn_report("NVMe: Unexpected CID in completion queue: %"PRIu32", " > + "queue size: %u", cid, NVME_QUEUE_SIZE); > continue; > } > trace_nvme_complete_command(s, q->index, cid); >
On Tue, Oct 27, 2020 at 02:55:25PM +0100, Philippe Mathieu-Daudé wrote: > Instead of displaying warning on stderr, use warn_report() > which also displays it on the monitor. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > block/nvme.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff --git a/block/nvme.c b/block/nvme.c index 739a0a700cb..6f1d7f9b2a1 100644 --- a/block/nvme.c +++ b/block/nvme.c @@ -399,8 +399,8 @@ static bool nvme_process_completion(NVMeQueuePair *q) } cid = le16_to_cpu(c->cid); if (cid == 0 || cid > NVME_QUEUE_SIZE) { - fprintf(stderr, "Unexpected CID in completion queue: %" PRIu32 "\n", - cid); + warn_report("NVMe: Unexpected CID in completion queue: %"PRIu32", " + "queue size: %u", cid, NVME_QUEUE_SIZE); continue; } trace_nvme_complete_command(s, q->index, cid);
Instead of displaying warning on stderr, use warn_report() which also displays it on the monitor. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- block/nvme.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)