| Message ID | 20200415102445.564803-1-its@irrelevant.dk |
|---|---|
| Headers | show |
| Series | nvme: refactoring and cleanups | expand |
On 4/15/20 12:24 PM, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > --- > hw/block/nvme.c | 47 ++++++++++++++++++++++++++--------------------- > 1 file changed, 26 insertions(+), 21 deletions(-) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index f0989cbb4335..08f7ae0a48b3 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -1359,13 +1359,35 @@ static int nvme_init_blk(NvmeCtrl *n, Error **errp) > return 0; > } > > +static int nvme_init_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp) > +{ > + int64_t bs_size; > + NvmeIdNs *id_ns = &ns->id_ns; > + > + bs_size = blk_getlength(n->conf.blk); > + if (bs_size < 0) { > + error_setg_errno(errp, -bs_size, "could not get backing file size"); > + return -1; > + } > + > + n->ns_size = bs_size; > + > + id_ns->lbaf[0].ds = BDRV_SECTOR_BITS; > + id_ns->nsze = cpu_to_le64(nvme_ns_nlbas(n, ns)); > + > + /* no thin provisioning */ > + id_ns->ncap = id_ns->nsze; > + id_ns->nuse = id_ns->ncap; > + > + return 0; > +} > + > static void nvme_realize(PCIDevice *pci_dev, Error **errp) > { > NvmeCtrl *n = NVME(pci_dev); > NvmeIdCtrl *id = &n->id_ctrl; > > int i; > - int64_t bs_size; > uint8_t *pci_conf; > > if (nvme_check_constraints(n, errp)) { > @@ -1374,12 +1396,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) > > nvme_init_state(n); > > - bs_size = blk_getlength(n->conf.blk); > - if (bs_size < 0) { > - error_setg(errp, "could not get backing file size"); > - return; > - } > - > if (nvme_init_blk(n, errp)) { > return; > } > @@ -1390,8 +1406,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) > pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS); > pcie_endpoint_cap_init(pci_dev, 0x80); > > - n->ns_size = bs_size / (uint64_t)n->num_namespaces; I'm not sure this line belong to this patch. > - > memory_region_init_io(&n->iomem, OBJECT(n), &nvme_mmio_ops, n, > "nvme", n->reg_size); > pci_register_bar(pci_dev, 0, > @@ -1455,18 +1469,9 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) > } > > for (i = 0; i < n->num_namespaces; i++) { > - NvmeNamespace *ns = &n->namespaces[i]; > - NvmeIdNs *id_ns = &ns->id_ns; > - id_ns->nsfeat = 0; > - id_ns->nlbaf = 0; > - id_ns->flbas = 0; > - id_ns->mc = 0; > - id_ns->dpc = 0; > - id_ns->dps = 0; > - id_ns->lbaf[0].ds = BDRV_SECTOR_BITS; > - id_ns->ncap = id_ns->nuse = id_ns->nsze = > - cpu_to_le64(n->ns_size >> > - id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas)].ds); > + if (nvme_init_namespace(n, &n->namespaces[i], errp)) { > + return; > + } > } > } > >
On Apr 15 12:38, Philippe Mathieu-Daudé wrote: > On 4/15/20 12:24 PM, Klaus Jensen wrote: > > From: Klaus Jensen <k.jensen@samsung.com> > > > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > > --- > > hw/block/nvme.c | 47 ++++++++++++++++++++++++++--------------------- > > 1 file changed, 26 insertions(+), 21 deletions(-) > > > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > > index f0989cbb4335..08f7ae0a48b3 100644 > > --- a/hw/block/nvme.c > > +++ b/hw/block/nvme.c > > @@ -1359,13 +1359,35 @@ static int nvme_init_blk(NvmeCtrl *n, Error **errp) > > return 0; > > } > > +static int nvme_init_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp) > > +{ > > + int64_t bs_size; > > + NvmeIdNs *id_ns = &ns->id_ns; > > + > > + bs_size = blk_getlength(n->conf.blk); > > + if (bs_size < 0) { > > + error_setg_errno(errp, -bs_size, "could not get backing file size"); > > + return -1; > > + } > > + > > + n->ns_size = bs_size; > > + > > + id_ns->lbaf[0].ds = BDRV_SECTOR_BITS; > > + id_ns->nsze = cpu_to_le64(nvme_ns_nlbas(n, ns)); > > + > > + /* no thin provisioning */ > > + id_ns->ncap = id_ns->nsze; > > + id_ns->nuse = id_ns->ncap; > > + > > + return 0; > > +} > > + > > static void nvme_realize(PCIDevice *pci_dev, Error **errp) > > { > > NvmeCtrl *n = NVME(pci_dev); > > NvmeIdCtrl *id = &n->id_ctrl; > > int i; > > - int64_t bs_size; > > uint8_t *pci_conf; > > if (nvme_check_constraints(n, errp)) { > > @@ -1374,12 +1396,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) > > nvme_init_state(n); > > - bs_size = blk_getlength(n->conf.blk); > > - if (bs_size < 0) { > > - error_setg(errp, "could not get backing file size"); > > - return; > > - } > > - > > if (nvme_init_blk(n, errp)) { > > return; > > } > > @@ -1390,8 +1406,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) > > pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS); > > pcie_endpoint_cap_init(pci_dev, 0x80); > > - n->ns_size = bs_size / (uint64_t)n->num_namespaces; > > I'm not sure this line belong to this patch. > It does. It is already there in the middle of the realize function. It is moved to nvme_init_namespace as n->ns_size = bs_size; since only a single namespace can be configured anyway. I will remove the for-loop that initializes multiple namespaces as well.
On Apr 15 12:53, Klaus Birkelund Jensen wrote: > On Apr 15 12:38, Philippe Mathieu-Daudé wrote: > > > > I'm not sure this line belong to this patch. > > > > It does. It is already there in the middle of the realize function. It > is moved to nvme_init_namespace as > > n->ns_size = bs_size; > > since only a single namespace can be configured anyway. I will remove > the for-loop that initializes multiple namespaces as well. I'm gonna backtrack on that. Removing that for loop is just noise I think.
Patchew URL: https://patchew.org/QEMU/20200415102445.564803-1-its@irrelevant.dk/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [PATCH 00/16] nvme: refactoring and cleanups
Message-id: 20200415102445.564803-1-its@irrelevant.dk
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
2f7cc1f..73995d1 master -> master
- [tag update] patchew/20200414120935.12719-1-peter.maydell@linaro.org -> patchew/20200414120935.12719-1-peter.maydell@linaro.org
- [tag update] patchew/20200415074927.19897-1-armbru@redhat.com -> patchew/20200415074927.19897-1-armbru@redhat.com
- [tag update] patchew/20200415083048.14339-1-armbru@redhat.com -> patchew/20200415083048.14339-1-armbru@redhat.com
* [new tag] patchew/20200415130159.611361-1-its@irrelevant.dk -> patchew/20200415130159.611361-1-its@irrelevant.dk
Switched to a new branch 'test'
36a0670 nvme: factor out controller identify setup
0eba9ad nvme: factor out cmb setup
5b5feee nvme: factor out pci setup
577bb77 nvme: factor out namespace setup
9ac7fae nvme: add namespace helpers
ab9bbca nvme: factor out block backend setup
ba90026 nvme: factor out device state setup
b14d6d1 nvme: factor out property/constraint checks
35fc2dc nvme: remove redundant cmbloc/cmbsz members
802a9ab nvme: add max_ioqpairs device parameter
acf5b71 nvme: refactor nvme_addr_read
945aa26 nvme: use constants in identify
06aee41 nvme: move device parameters to separate struct
d1f279f nvme: remove superfluous breaks
e659f34 nvme: rename trace events to nvme_dev
f6d960a nvme: fix pci doorbell size calculation
=== OUTPUT BEGIN ===
1/16 Checking commit f6d960adefde (nvme: fix pci doorbell size calculation)
2/16 Checking commit e659f3474c29 (nvme: rename trace events to nvme_dev)
3/16 Checking commit d1f279f37080 (nvme: remove superfluous breaks)
4/16 Checking commit 06aee411cb67 (nvme: move device parameters to separate struct)
ERROR: Macros with complex values should be enclosed in parenthesis
#181: FILE: hw/block/nvme.h:6:
+#define DEFINE_NVME_PROPERTIES(_state, _props) \
+ DEFINE_PROP_STRING("serial", _state, _props.serial), \
+ DEFINE_PROP_UINT32("cmb_size_mb", _state, _props.cmb_size_mb, 0), \
+ DEFINE_PROP_UINT32("num_queues", _state, _props.num_queues, 64)
total: 1 errors, 0 warnings, 181 lines checked
Patch 4/16 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/16 Checking commit 945aa26e4b39 (nvme: use constants in identify)
6/16 Checking commit acf5b7156c93 (nvme: refactor nvme_addr_read)
7/16 Checking commit 802a9ab0f8c1 (nvme: add max_ioqpairs device parameter)
8/16 Checking commit 35fc2dcb7465 (nvme: remove redundant cmbloc/cmbsz members)
9/16 Checking commit b14d6d1afb3e (nvme: factor out property/constraint checks)
10/16 Checking commit ba9002645e0c (nvme: factor out device state setup)
11/16 Checking commit ab9bbcaa7af5 (nvme: factor out block backend setup)
12/16 Checking commit 9ac7faec3567 (nvme: add namespace helpers)
13/16 Checking commit 577bb7783e47 (nvme: factor out namespace setup)
14/16 Checking commit 5b5feee08d6d (nvme: factor out pci setup)
15/16 Checking commit 0eba9ade7b81 (nvme: factor out cmb setup)
16/16 Checking commit 36a0670099c1 (nvme: factor out controller identify setup)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200415102445.564803-1-its@irrelevant.dk/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
From: Klaus Jensen <k.jensen@samsung.com> Philippe suggested that I split up this already way too huge series (more than 50 patches now), so here goes. The first patch in this series fixes a small bug in the pci doorbell size calculation. Please consider cherry-picking this. The rest are refactorings. The "nvme: add max_ioqpairs device parameter" introduces the 'max_ioqpairs' device parameter, the meaning of which should be more intuitive than the existing 'num_queues' parameter. Klaus Jensen (16): nvme: fix pci doorbell size calculation nvme: rename trace events to nvme_dev nvme: remove superfluous breaks nvme: move device parameters to separate struct nvme: use constants in identify nvme: refactor nvme_addr_read nvme: add max_ioqpairs device parameter nvme: remove redundant cmbloc/cmbsz members nvme: factor out property/constraint checks nvme: factor out device state setup nvme: factor out block backend setup nvme: add namespace helpers nvme: factor out namespace setup nvme: factor out pci setup nvme: factor out cmb setup nvme: factor out controller identify setup hw/block/nvme.c | 436 ++++++++++++++++++++++++------------------ hw/block/nvme.h | 36 +++- hw/block/trace-events | 172 ++++++++--------- include/block/nvme.h | 8 + 4 files changed, 373 insertions(+), 279 deletions(-)