| Message ID | 20211120000250.1663391-20-ben.widawsky@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add drivers for CXL ports and mem devices | expand |
Hi Ben,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 53989fad1286e652ea3655ae3367ba698da8d2ff]
url: https://github.com/0day-ci/linux/commits/Ben-Widawsky/Add-drivers-for-CXL-ports-and-mem-devices/20211120-080513
base: 53989fad1286e652ea3655ae3367ba698da8d2ff
config: riscv-buildonly-randconfig-r001-20211119 (attached as .config)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/0day-ci/linux/commit/3f74c99d751a24a4c12ba76c23b68c2832f805e1
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Ben-Widawsky/Add-drivers-for-CXL-ports-and-mem-devices/20211120-080513
git checkout 3f74c99d751a24a4c12ba76c23b68c2832f805e1
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/cxl/pci.c:469:7: warning: variable 'size' set but not used [-Wunused-but-set-variable]
u64 size;
^
drivers/cxl/pci.c:516:7: warning: variable 'size' set but not used [-Wunused-but-set-variable]
u64 size;
^
>> drivers/cxl/pci.c:673:13: warning: variable 'cxlmd' is uninitialized when used here [-Wuninitialized]
dev_warn(&cxlmd->dev, "No component registers (%d)\n", rc);
^~~~~
include/linux/dev_printk.h:146:49: note: expanded from macro 'dev_warn'
dev_printk_index_wrap(_dev_warn, KERN_WARNING, dev, dev_fmt(fmt), ##__VA_ARGS__)
^~~
include/linux/dev_printk.h:110:11: note: expanded from macro 'dev_printk_index_wrap'
_p_func(dev, fmt, ##__VA_ARGS__); \
^~~
drivers/cxl/pci.c:630:26: note: initialize the variable 'cxlmd' to silence this warning
struct cxl_memdev *cxlmd;
^
= NULL
3 warnings generated.
vim +/cxlmd +673 drivers/cxl/pci.c
625
626 static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
627 {
628 struct cxl_endpoint_dvsec_info *info;
629 struct cxl_register_map map;
630 struct cxl_memdev *cxlmd;
631 struct cxl_dev_state *cxlds;
632 int rc;
633
634 /*
635 * Double check the anonymous union trickery in struct cxl_regs
636 * FIXME switch to struct_group()
637 */
638 BUILD_BUG_ON(offsetof(struct cxl_regs, memdev) !=
639 offsetof(struct cxl_regs, device_regs.memdev));
640
641 rc = pcim_enable_device(pdev);
642 if (rc)
643 return rc;
644
645 cxlds = cxl_dev_state_create(&pdev->dev);
646 if (IS_ERR(cxlds))
647 return PTR_ERR(cxlds);
648
649 cxlds->device_dvsec = pci_find_dvsec_capability(pdev,
650 PCI_DVSEC_VENDOR_ID_CXL,
651 CXL_DVSEC_PCIE_DEVICE);
652 if (!cxlds->device_dvsec)
653 dev_warn(&pdev->dev,
654 "Device DVSEC not present. Expect limited functionality.\n");
655 else
656 cxlds->wait_media_ready = wait_for_media_ready;
657
658 rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
659 if (rc)
660 return rc;
661
662 rc = cxl_map_regs(cxlds, &map);
663 if (rc)
664 return rc;
665
666 /*
667 * If the component registers can't be found, the cxl_pci driver may
668 * still be useful for management functions so don't return an error.
669 */
670 cxlds->component_reg_phys = CXL_RESOURCE_NONE;
671 rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
672 if (rc)
> 673 dev_warn(&cxlmd->dev, "No component registers (%d)\n", rc);
674 else
675 cxlds->component_reg_phys = cxl_reg_block(pdev, &map);
676
677 rc = cxl_pci_setup_mailbox(cxlds);
678 if (rc)
679 return rc;
680
681 rc = cxl_enumerate_cmds(cxlds);
682 if (rc)
683 return rc;
684
685 rc = cxl_dev_state_identify(cxlds);
686 if (rc)
687 return rc;
688
689 rc = cxl_mem_create_range_info(cxlds);
690 if (rc)
691 return rc;
692
693 info = dvsec_ranges(cxlds);
694 if (IS_ERR(info))
695 dev_err(&pdev->dev,
696 "Failed to get DVSEC range information (%ld)\n",
697 PTR_ERR(info));
698 else
699 cxlds->info = info;
700
701 cxlmd = devm_cxl_add_memdev(cxlds);
702 if (IS_ERR(cxlmd))
703 return PTR_ERR(cxlmd);
704
705 if (range_len(&cxlds->pmem_range) && IS_ENABLED(CONFIG_CXL_PMEM))
706 rc = devm_cxl_add_nvdimm(&pdev->dev, cxlmd);
707
708 return rc;
709 }
710
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Fri, 19 Nov 2021 16:02:46 -0800 Ben Widawsky <ben.widawsky@intel.com> wrote: > The component register base address is enumerated and stored in the > cxl_device_state structure for use by the endpoint driver. > > Component register base addresses are obtained through PCI mechanisms. > As such it makes most sense for the cxl_pci driver to obtain that > address. In order to reuse the port driver for enumerating decoder > resources for an endpoint, it is desirable to be able to add the > endpoint as a port. The endpoint does know of the cxlds and can pull the > component register base from there and pass it along to port creation. This feels like a lot of explanation in for trivial caching of an address. I'm not sure you need to be that detailed, though I guess it does no real harm. Another one where I'm unsure why we are muddling on after an error... > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > Changes since RFCv2: > This patch was originally named, "cxl/core: Store component register > base for memdevs". It plumbed the component registers through memdev > creation. After more work, it became apparent we needed to plumb other > stuff from the pci driver, so going forward, cxlds will just be > referenced by the cxl_mem driver. This also allows us to ignore the > change needed to cxl_test > > - Rework patch to store the base in cxlds > - Remove defunct comment (Dan) > --- > drivers/cxl/cxlmem.h | 2 ++ > drivers/cxl/pci.c | 11 +++++++++++ > 2 files changed, 13 insertions(+) > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index a9424dd4e5c3..b1d753541f4e 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -134,6 +134,7 @@ struct cxl_endpoint_dvsec_info { > * @next_volatile_bytes: volatile capacity change pending device reset > * @next_persistent_bytes: persistent capacity change pending device reset > * @info: Cached DVSEC information about the device. > + * @component_reg_phys: register base of component registers > * @mbox_send: @dev specific transport for transmitting mailbox commands > * > * See section 8.2.9.5.2 Capacity Configuration and Label Storage for > @@ -165,6 +166,7 @@ struct cxl_dev_state { > u64 next_persistent_bytes; > > struct cxl_endpoint_dvsec_info *info; > + resource_size_t component_reg_phys; > > int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd); > int (*wait_media_ready)(struct cxl_dev_state *cxlds); > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index f1a68bfe5f77..a8e375950514 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -663,6 +663,17 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (rc) > return rc; > > + /* > + * If the component registers can't be found, the cxl_pci driver may > + * still be useful for management functions so don't return an error. > + */ > + cxlds->component_reg_phys = CXL_RESOURCE_NONE; > + rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT, &map); > + if (rc) > + dev_warn(&cxlmd->dev, "No component registers (%d)\n", rc); > + else > + cxlds->component_reg_phys = cxl_reg_block(pdev, &map); > + > rc = cxl_pci_setup_mailbox(cxlds); > if (rc) > return rc;
On 21-11-22 17:11:42, Jonathan Cameron wrote: > On Fri, 19 Nov 2021 16:02:46 -0800 > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > The component register base address is enumerated and stored in the > > cxl_device_state structure for use by the endpoint driver. > > > > Component register base addresses are obtained through PCI mechanisms. > > As such it makes most sense for the cxl_pci driver to obtain that > > address. In order to reuse the port driver for enumerating decoder > > resources for an endpoint, it is desirable to be able to add the > > endpoint as a port. The endpoint does know of the cxlds and can pull the > > component register base from there and pass it along to port creation. > > This feels like a lot of explanation in for trivial caching of an address. > I'm not sure you need to be that detailed, though I guess it does no real > harm. It is mostly to articulate that cxl_pci is responsible for PCI stuff, and cxl_mem is responsible for CXL.mem stuff, and that's why we didn't just do this work in cxl_mem (which indeed was what I originally did). > > Another one where I'm unsure why we are muddling on after an error... Same motivation as the other - mailbox can still be used even if the media isn't available. > > > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > --- > > Changes since RFCv2: > > This patch was originally named, "cxl/core: Store component register > > base for memdevs". It plumbed the component registers through memdev > > creation. After more work, it became apparent we needed to plumb other > > stuff from the pci driver, so going forward, cxlds will just be > > referenced by the cxl_mem driver. This also allows us to ignore the > > change needed to cxl_test > > > > - Rework patch to store the base in cxlds > > - Remove defunct comment (Dan) > > --- > > drivers/cxl/cxlmem.h | 2 ++ > > drivers/cxl/pci.c | 11 +++++++++++ > > 2 files changed, 13 insertions(+) > > > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > > index a9424dd4e5c3..b1d753541f4e 100644 > > --- a/drivers/cxl/cxlmem.h > > +++ b/drivers/cxl/cxlmem.h > > @@ -134,6 +134,7 @@ struct cxl_endpoint_dvsec_info { > > * @next_volatile_bytes: volatile capacity change pending device reset > > * @next_persistent_bytes: persistent capacity change pending device reset > > * @info: Cached DVSEC information about the device. > > + * @component_reg_phys: register base of component registers > > * @mbox_send: @dev specific transport for transmitting mailbox commands > > * > > * See section 8.2.9.5.2 Capacity Configuration and Label Storage for > > @@ -165,6 +166,7 @@ struct cxl_dev_state { > > u64 next_persistent_bytes; > > > > struct cxl_endpoint_dvsec_info *info; > > + resource_size_t component_reg_phys; > > > > int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd); > > int (*wait_media_ready)(struct cxl_dev_state *cxlds); > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > > index f1a68bfe5f77..a8e375950514 100644 > > --- a/drivers/cxl/pci.c > > +++ b/drivers/cxl/pci.c > > @@ -663,6 +663,17 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > if (rc) > > return rc; > > > > + /* > > + * If the component registers can't be found, the cxl_pci driver may > > + * still be useful for management functions so don't return an error. > > + */ > > + cxlds->component_reg_phys = CXL_RESOURCE_NONE; > > + rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT, &map); > > + if (rc) > > + dev_warn(&cxlmd->dev, "No component registers (%d)\n", rc); > > + else > > + cxlds->component_reg_phys = cxl_reg_block(pdev, &map); > > + > > rc = cxl_pci_setup_mailbox(cxlds); > > if (rc) > > return rc; >
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index a9424dd4e5c3..b1d753541f4e 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -134,6 +134,7 @@ struct cxl_endpoint_dvsec_info { * @next_volatile_bytes: volatile capacity change pending device reset * @next_persistent_bytes: persistent capacity change pending device reset * @info: Cached DVSEC information about the device. + * @component_reg_phys: register base of component registers * @mbox_send: @dev specific transport for transmitting mailbox commands * * See section 8.2.9.5.2 Capacity Configuration and Label Storage for @@ -165,6 +166,7 @@ struct cxl_dev_state { u64 next_persistent_bytes; struct cxl_endpoint_dvsec_info *info; + resource_size_t component_reg_phys; int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd); int (*wait_media_ready)(struct cxl_dev_state *cxlds); diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index f1a68bfe5f77..a8e375950514 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -663,6 +663,17 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (rc) return rc; + /* + * If the component registers can't be found, the cxl_pci driver may + * still be useful for management functions so don't return an error. + */ + cxlds->component_reg_phys = CXL_RESOURCE_NONE; + rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT, &map); + if (rc) + dev_warn(&cxlmd->dev, "No component registers (%d)\n", rc); + else + cxlds->component_reg_phys = cxl_reg_block(pdev, &map); + rc = cxl_pci_setup_mailbox(cxlds); if (rc) return rc;
The component register base address is enumerated and stored in the cxl_device_state structure for use by the endpoint driver. Component register base addresses are obtained through PCI mechanisms. As such it makes most sense for the cxl_pci driver to obtain that address. In order to reuse the port driver for enumerating decoder resources for an endpoint, it is desirable to be able to add the endpoint as a port. The endpoint does know of the cxlds and can pull the component register base from there and pass it along to port creation. Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> --- Changes since RFCv2: This patch was originally named, "cxl/core: Store component register base for memdevs". It plumbed the component registers through memdev creation. After more work, it became apparent we needed to plumb other stuff from the pci driver, so going forward, cxlds will just be referenced by the cxl_mem driver. This also allows us to ignore the change needed to cxl_test - Rework patch to store the base in cxlds - Remove defunct comment (Dan) --- drivers/cxl/cxlmem.h | 2 ++ drivers/cxl/pci.c | 11 +++++++++++ 2 files changed, 13 insertions(+)