diff mbox series

[v3,2/2] hw/nvme: cleanup error reporting in nvme_init_pci()

Message ID 20221110220805.26816-3-its@irrelevant.dk
State New
Headers show
Series hw/nvme: errp fixes | expand

Commit Message

Klaus Jensen Nov. 10, 2022, 10:08 p.m. UTC
From: Klaus Jensen <k.jensen@samsung.com>

Replace the local Error variable with errp and ERRP_GUARD() and change
the return value to bool.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

Comments

Philippe Mathieu-Daudé Nov. 11, 2022, 11:40 a.m. UTC | #1
On 10/11/22 23:08, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Replace the local Error variable with errp and ERRP_GUARD() and change
> the return value to bool.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/nvme/ctrl.c | 23 ++++++++++-------------
>   1 file changed, 10 insertions(+), 13 deletions(-)


> @@ -7388,14 +7387,12 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
>       }
>       ret = msix_init(pci_dev, n->params.msix_qsize,
>                       &n->bar0, 0, msix_table_offset,
> -                    &n->bar0, 0, msix_pba_offset, 0, &err);
> -    if (ret < 0) {
> -        if (ret == -ENOTSUP) {
> -            warn_report_err(err);
> -        } else {
> -            error_propagate(errp, err);
> -            return ret;
> -        }
> +                    &n->bar0, 0, msix_pba_offset, 0, errp);
> +    if (ret == -ENOTSUP) {
> +        warn_report_err(*errp);

Why only report ENOTSUP in particular?

> +        *errp = NULL;
> +    } else if (ret < 0) {
 > +        return false;

Is that normal to ignore:

-   error_setg(errp, "The number of MSI-X vectors is invalid");
     return -EINVAL;

-   error_setg(errp, "table & pba overlap, or they don't fit in BARs,"
                      " or don't align");
     return -EINVAL;

Or possible future error added in msix_init()?
Klaus Jensen Nov. 11, 2022, 12:32 p.m. UTC | #2
On Nov 11 12:40, Philippe Mathieu-Daudé wrote:
> On 10/11/22 23:08, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Replace the local Error variable with errp and ERRP_GUARD() and change
> > the return value to bool.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >   hw/nvme/ctrl.c | 23 ++++++++++-------------
> >   1 file changed, 10 insertions(+), 13 deletions(-)
> 
> 
> > @@ -7388,14 +7387,12 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
> >       }
> >       ret = msix_init(pci_dev, n->params.msix_qsize,
> >                       &n->bar0, 0, msix_table_offset,
> > -                    &n->bar0, 0, msix_pba_offset, 0, &err);
> > -    if (ret < 0) {
> > -        if (ret == -ENOTSUP) {
> > -            warn_report_err(err);
> > -        } else {
> > -            error_propagate(errp, err);
> > -            return ret;
> > -        }
> > +                    &n->bar0, 0, msix_pba_offset, 0, errp);
> > +    if (ret == -ENOTSUP) {
> > +        warn_report_err(*errp);
> 
> Why only report ENOTSUP in particular?
> 

Because the error is beneign (it's just a notice that MSI-X isnt
available on the platform).

> > +        *errp = NULL;
> > +    } else if (ret < 0) {
> > +        return false;
> 
> Is that normal to ignore:
> 
> -   error_setg(errp, "The number of MSI-X vectors is invalid");
>     return -EINVAL;
> 
> -   error_setg(errp, "table & pba overlap, or they don't fit in BARs,"
>                      " or don't align");
>     return -EINVAL;
> 
> Or possible future error added in msix_init()?

It is not ignored, it is passed up to the caller. On any other error,
returning false will cause device realization to fail and the error
(i.e. invalid vectors or overlap) be reported.
Philippe Mathieu-Daudé Nov. 11, 2022, 12:56 p.m. UTC | #3
On 11/11/22 13:32, Klaus Jensen wrote:
> On Nov 11 12:40, Philippe Mathieu-Daudé wrote:
>> On 10/11/22 23:08, Klaus Jensen wrote:
>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>
>>> Replace the local Error variable with errp and ERRP_GUARD() and change
>>> the return value to bool.
>>>
>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>> ---
>>>    hw/nvme/ctrl.c | 23 ++++++++++-------------
>>>    1 file changed, 10 insertions(+), 13 deletions(-)
>>
>>
>>> @@ -7388,14 +7387,12 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
>>>        }
>>>        ret = msix_init(pci_dev, n->params.msix_qsize,
>>>                        &n->bar0, 0, msix_table_offset,
>>> -                    &n->bar0, 0, msix_pba_offset, 0, &err);
>>> -    if (ret < 0) {
>>> -        if (ret == -ENOTSUP) {
>>> -            warn_report_err(err);
>>> -        } else {
>>> -            error_propagate(errp, err);
>>> -            return ret;
>>> -        }
>>> +                    &n->bar0, 0, msix_pba_offset, 0, errp);
>>> +    if (ret == -ENOTSUP) {
>>> +        warn_report_err(*errp);
>>
>> Why only report ENOTSUP in particular?
>>
> 
> Because the error is beneign (it's just a notice that MSI-X isnt
> available on the platform).
> 
>>> +        *errp = NULL;
>>> +    } else if (ret < 0) {
>>> +        return false;
>>
>> Is that normal to ignore:
>>
>> -   error_setg(errp, "The number of MSI-X vectors is invalid");
>>      return -EINVAL;
>>
>> -   error_setg(errp, "table & pba overlap, or they don't fit in BARs,"
>>                       " or don't align");
>>      return -EINVAL;
>>
>> Or possible future error added in msix_init()?
> 
> It is not ignored, it is passed up to the caller. On any other error,
> returning false will cause device realization to fail and the error
> (i.e. invalid vectors or overlap) be reported.

Indeed, I didn't review carefully enough.

Maybe in the first case explicit with /* Convert the error as a simple 
warning */ and in the second /* Propagate to caller */.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
diff mbox series

Patch

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index a5c0a5fa6ce2..7d855523bb93 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -7343,15 +7343,14 @@  static int nvme_add_pm_capability(PCIDevice *pci_dev, uint8_t offset)
     return 0;
 }
 
-static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
+static bool nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
 {
+    ERRP_GUARD();
     uint8_t *pci_conf = pci_dev->config;
     uint64_t bar_size;
     unsigned msix_table_offset, msix_pba_offset;
     int ret;
 
-    Error *err = NULL;
-
     pci_conf[PCI_INTERRUPT_PIN] = 1;
     pci_config_set_prog_interface(pci_conf, 0x2);
 
@@ -7388,14 +7387,12 @@  static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
     }
     ret = msix_init(pci_dev, n->params.msix_qsize,
                     &n->bar0, 0, msix_table_offset,
-                    &n->bar0, 0, msix_pba_offset, 0, &err);
-    if (ret < 0) {
-        if (ret == -ENOTSUP) {
-            warn_report_err(err);
-        } else {
-            error_propagate(errp, err);
-            return ret;
-        }
+                    &n->bar0, 0, msix_pba_offset, 0, errp);
+    if (ret == -ENOTSUP) {
+        warn_report_err(*errp);
+        *errp = NULL;
+    } else if (ret < 0) {
+        return false;
     }
 
     nvme_update_msixcap_ts(pci_dev, n->conf_msix_qsize);
@@ -7412,7 +7409,7 @@  static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
         nvme_init_sriov(n, pci_dev, 0x120);
     }
 
-    return 0;
+    return true;
 }
 
 static void nvme_init_subnqn(NvmeCtrl *n)
@@ -7588,7 +7585,7 @@  static void nvme_realize(PCIDevice *pci_dev, Error **errp)
         return;
     }
     nvme_init_state(n);
-    if (nvme_init_pci(n, pci_dev, errp)) {
+    if (!nvme_init_pci(n, pci_dev, errp)) {
         return;
     }
     nvme_init_ctrl(n, pci_dev);