diff mbox series

[03/25] block/nvme: Report warning with warn_report()

Message ID 20201027135547.374946-4-philmd@redhat.com (mailing list archive)
State New, archived
Headers show
Series block/nvme: Fix Aarch64 host | expand

Commit Message

Philippe Mathieu-Daudé Oct. 27, 2020, 1:55 p.m. UTC
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(-)

Comments

Keith Busch Oct. 27, 2020, 2:45 p.m. UTC | #1
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);
Philippe Mathieu-Daudé Oct. 27, 2020, 3:33 p.m. UTC | #2
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);
>
Philippe Mathieu-Daudé Oct. 27, 2020, 3:54 p.m. UTC | #3
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.
Auger Eric Oct. 28, 2020, 10:22 a.m. UTC | #4
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);
>
Stefan Hajnoczi Oct. 28, 2020, 3:47 p.m. UTC | #5
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 mbox series

Patch

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);