| Message ID | 20221110220805.26816-3-its@irrelevant.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/nvme: errp fixes | expand |
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()?
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.
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 --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);