| Message ID | 20200415130159.611361-1-its@irrelevant.dk |
|---|---|
| Headers | show |
| Series | nvme: refactoring and cleanups | expand |
On 4/15/20 3:01 PM, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/block/nvme.c | 52 +++++++++++++++++++++++++++---------------------- > 1 file changed, 29 insertions(+), 23 deletions(-) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index 4c28d75e0fc8..804f24719dce 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -1422,32 +1422,11 @@ static void nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev) > } > } > > -static void nvme_realize(PCIDevice *pci_dev, Error **errp) > +static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev) > { > - NvmeCtrl *n = NVME(pci_dev); > NvmeIdCtrl *id = &n->id_ctrl; > - Error *err = NULL; > + uint8_t *pci_conf = pci_dev->config; > > - int i; > - uint8_t *pci_conf; > - > - nvme_check_constraints(n, &err); > - if (err) { > - error_propagate(errp, err); > - return; > - } > - > - nvme_init_state(n); > - > - nvme_init_blk(n, &err); > - if (err) { > - error_propagate(errp, err); > - return; > - } > - > - nvme_init_pci(n, pci_dev); > - > - pci_conf = pci_dev->config; > id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID)); > id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID)); > strpadcpy((char *)id->mn, sizeof(id->mn), "QEMU NVMe Ctrl", ' '); > @@ -1481,6 +1460,33 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) > n->bar.vs = 0x00010200; > n->bar.intmc = n->bar.intms = 0; > > + Spurious empty lines (no need to repost!). To other reviewer, patch trivial to review with 'git-diff -W'. > +} > + > +static void nvme_realize(PCIDevice *pci_dev, Error **errp) > +{ > + NvmeCtrl *n = NVME(pci_dev); > + Error *err = NULL; > + > + int i; > + > + nvme_check_constraints(n, &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > + > + > + nvme_init_state(n); > + nvme_init_blk(n, &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > + > + nvme_init_pci(n, pci_dev); > + nvme_init_ctrl(n, pci_dev); > + > for (i = 0; i < n->num_namespaces; i++) { > nvme_init_namespace(n, &n->namespaces[i], &err); > if (err) { >
Patchew URL: https://patchew.org/QEMU/20200415130159.611361-1-its@irrelevant.dk/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [PATCH v2 00/16] nvme: refactoring and cleanups
Message-id: 20200415130159.611361-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
Switched to a new branch 'test'
a0e808d nvme: factor out controller identify setup
ea2d295 nvme: factor out cmb setup
c3c1e51 nvme: factor out pci setup
b4b6c55 nvme: factor out namespace setup
79229ae nvme: add namespace helpers
c7d0f2b nvme: factor out block backend setup
0802218 nvme: factor out device state setup
b407ffa nvme: factor out property/constraint checks
a7b4ac0 nvme: remove redundant cmbloc/cmbsz members
bf8d4cc nvme: add max_ioqpairs device parameter
eef1607 nvme: refactor nvme_addr_read
c063b77 nvme: use constants in identify
9c1ad75 nvme: move device parameters to separate struct
72124cf nvme: remove superfluous breaks
8cbaf9e nvme: rename trace events to pci_nvme
826b0ca nvme: fix pci doorbell size calculation
=== OUTPUT BEGIN ===
1/16 Checking commit 826b0caf1bed (nvme: fix pci doorbell size calculation)
2/16 Checking commit 8cbaf9e98e00 (nvme: rename trace events to pci_nvme)
3/16 Checking commit 72124cfc8e35 (nvme: remove superfluous breaks)
4/16 Checking commit 9c1ad75817e7 (nvme: move device parameters to separate struct)
ERROR: Macros with complex values should be enclosed in parenthesis
#182: 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, 182 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 c063b77e218e (nvme: use constants in identify)
6/16 Checking commit eef16074638e (nvme: refactor nvme_addr_read)
7/16 Checking commit bf8d4cc67458 (nvme: add max_ioqpairs device parameter)
8/16 Checking commit a7b4ac0a9cbe (nvme: remove redundant cmbloc/cmbsz members)
9/16 Checking commit b407ffae89f8 (nvme: factor out property/constraint checks)
10/16 Checking commit 0802218ca18b (nvme: factor out device state setup)
11/16 Checking commit c7d0f2b17c08 (nvme: factor out block backend setup)
12/16 Checking commit 79229aef5988 (nvme: add namespace helpers)
13/16 Checking commit b4b6c55cd5af (nvme: factor out namespace setup)
14/16 Checking commit c3c1e5121db6 (nvme: factor out pci setup)
15/16 Checking commit ea2d29507c1e (nvme: factor out cmb setup)
16/16 Checking commit a0e808d2fca5 (nvme: factor out controller identify setup)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200415130159.611361-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
On Apr 15 15:01, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > Changes since v1 > ~~~~~~~~~~~~~~~~ > * nvme: fix pci doorbell size calculation > - added some defines and a better comment (Philippe) > > * nvme: rename trace events to pci_nvme > - changed the prefix from nvme_dev to pci_nvme (Philippe) > > * nvme: add max_ioqpairs device parameter > - added a deprecation comment. I doubt this will go in until 5.1, so > changed it to "deprecated from 5.1" (Philippe) > > * nvme: factor out property/constraint checks > * nvme: factor out block backend setup > - changed to return void and propagate errors in proper QEMU style > (Philippe) > > * nvme: add namespace helpers > - use the helper immediately (Philippe) > > * nvme: factor out pci setup > - removed setting of vendor and device id which is already inherited > from nvme_class_init() (Philippe) > > * nvme: factor out cmb setup > - add lost comment (Philippe) > > > Klaus Jensen (16): > nvme: fix pci doorbell size calculation > nvme: rename trace events to pci_nvme > 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 | 433 ++++++++++++++++++++++++------------------ > hw/block/nvme.h | 36 +++- > hw/block/trace-events | 172 ++++++++--------- > include/block/nvme.h | 8 + > 4 files changed, 372 insertions(+), 277 deletions(-) > > -- > 2.26.0 > Hi Keith, You have acked most of this previously, but not in it's most recent state. Since a good bunch of the refactoring patches have been split up and changed, only a small subset of the patches still carry your Acked-by. The 'nvme: fix pci doorbell size calculation' and 'nvme: add max_ioqpairs device parameter' are new since your ack and given their nature a review from you would be nice :) Thanks, Klaus
The series looks good to me.
Reviewed-by: Keith Busch <kbusch@kernel.org>
On Apr 21 02:38, Keith Busch wrote: > The series looks good to me. > > Reviewed-by: Keith Busch <kbusch@kernel.org> Thanks for the review Keith! Kevin, should I rebase this on block-next? I think it might have some conflicts with the PMR patch that went in previously. Philippe, then I can also change the *err to *local_err ;)
Am 21.04.2020 um 08:38 hat Klaus Birkelund Jensen geschrieben: > On Apr 21 02:38, Keith Busch wrote: > > The series looks good to me. > > > > Reviewed-by: Keith Busch <kbusch@kernel.org> > > Thanks for the review Keith! > > Kevin, should I rebase this on block-next? I think it might have some > conflicts with the PMR patch that went in previously. The series doesn't apply at the moment anyway, I assume it's because of the PMR patch. So yes, I would appreciate a rebase. Kevin
On Wed, 2020-04-15 at 15:01 +0200, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > Changes since v1 > ~~~~~~~~~~~~~~~~ > * nvme: fix pci doorbell size calculation > - added some defines and a better comment (Philippe) > > * nvme: rename trace events to pci_nvme > - changed the prefix from nvme_dev to pci_nvme (Philippe) > > * nvme: add max_ioqpairs device parameter > - added a deprecation comment. I doubt this will go in until 5.1, so > changed it to "deprecated from 5.1" (Philippe) > > * nvme: factor out property/constraint checks > * nvme: factor out block backend setup > - changed to return void and propagate errors in proper QEMU style > (Philippe) > > * nvme: add namespace helpers > - use the helper immediately (Philippe) > > * nvme: factor out pci setup > - removed setting of vendor and device id which is already inherited > from nvme_class_init() (Philippe) > > * nvme: factor out cmb setup > - add lost comment (Philippe) > > > Klaus Jensen (16): > nvme: fix pci doorbell size calculation > nvme: rename trace events to pci_nvme > 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 | 433 ++++++++++++++++++++++++------------------ > hw/block/nvme.h | 36 +++- > hw/block/trace-events | 172 ++++++++--------- > include/block/nvme.h | 8 + > 4 files changed, 372 insertions(+), 277 deletions(-) > Should I also review the V7 series or I should wait for V8 which will not include these cleanups? Best regards, Maxim Levitsky
On Apr 21 19:24, Maxim Levitsky wrote: > Should I also review the V7 series or I should wait for V8 which will > not include these cleanups? Hi Maxim, Just wait for another series - I don't think I will post a v8, I will chop op the series into smaller ones instead. Most patches will hopefully not change too much, so should keep your Reviewed-by's ;) Thanks, Klaus
From: Klaus Jensen <k.jensen@samsung.com> Changes since v1 ~~~~~~~~~~~~~~~~ * nvme: fix pci doorbell size calculation - added some defines and a better comment (Philippe) * nvme: rename trace events to pci_nvme - changed the prefix from nvme_dev to pci_nvme (Philippe) * nvme: add max_ioqpairs device parameter - added a deprecation comment. I doubt this will go in until 5.1, so changed it to "deprecated from 5.1" (Philippe) * nvme: factor out property/constraint checks * nvme: factor out block backend setup - changed to return void and propagate errors in proper QEMU style (Philippe) * nvme: add namespace helpers - use the helper immediately (Philippe) * nvme: factor out pci setup - removed setting of vendor and device id which is already inherited from nvme_class_init() (Philippe) * nvme: factor out cmb setup - add lost comment (Philippe) Klaus Jensen (16): nvme: fix pci doorbell size calculation nvme: rename trace events to pci_nvme 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 | 433 ++++++++++++++++++++++++------------------ hw/block/nvme.h | 36 +++- hw/block/trace-events | 172 ++++++++--------- include/block/nvme.h | 8 + 4 files changed, 372 insertions(+), 277 deletions(-)