| Message ID | 161728744762.2474040.11009693084215696415.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | CXL Port Enumeration | expand |
On Thu, 1 Apr 2021 07:30:47 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > In preparation for sharing cxl.h with other generic CXL consumers, > move / consolidate some of the memory device specifics to mem.h. > > Reviewed-by: Ben Widawsky <ben.widawsky@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Hi Dan, Would be good to see something in this patch description saying why you chose to have mem.h rather than push the defines down into mem.c (which from the current code + patch set looks like the more logical thing to do). As a side note, docs for struct cxl_mem need a fix as they cover enabled_commands which at somepoint got shortened to enabled_cmds Jonathan > --- > drivers/cxl/cxl.h | 57 ------------------------------------ > drivers/cxl/mem.c | 25 +--------------- > drivers/cxl/mem.h | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 86 insertions(+), 81 deletions(-) > create mode 100644 drivers/cxl/mem.h > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 6f14838c2d25..2e3bdacb32e7 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -34,62 +34,5 @@ > #define CXLDEV_MBOX_BG_CMD_STATUS_OFFSET 0x18 > #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20 > > -/* CXL 2.0 8.2.8.5.1.1 Memory Device Status Register */ > -#define CXLMDEV_STATUS_OFFSET 0x0 > -#define CXLMDEV_DEV_FATAL BIT(0) > -#define CXLMDEV_FW_HALT BIT(1) > -#define CXLMDEV_STATUS_MEDIA_STATUS_MASK GENMASK(3, 2) > -#define CXLMDEV_MS_NOT_READY 0 > -#define CXLMDEV_MS_READY 1 > -#define CXLMDEV_MS_ERROR 2 > -#define CXLMDEV_MS_DISABLED 3 > -#define CXLMDEV_READY(status) \ > - (FIELD_GET(CXLMDEV_STATUS_MEDIA_STATUS_MASK, status) == \ > - CXLMDEV_MS_READY) > -#define CXLMDEV_MBOX_IF_READY BIT(4) > -#define CXLMDEV_RESET_NEEDED_MASK GENMASK(7, 5) > -#define CXLMDEV_RESET_NEEDED_NOT 0 > -#define CXLMDEV_RESET_NEEDED_COLD 1 > -#define CXLMDEV_RESET_NEEDED_WARM 2 > -#define CXLMDEV_RESET_NEEDED_HOT 3 > -#define CXLMDEV_RESET_NEEDED_CXL 4 > -#define CXLMDEV_RESET_NEEDED(status) \ > - (FIELD_GET(CXLMDEV_RESET_NEEDED_MASK, status) != \ > - CXLMDEV_RESET_NEEDED_NOT) > - > -struct cxl_memdev; > -/** > - * struct cxl_mem - A CXL memory device > - * @pdev: The PCI device associated with this CXL device. > - * @regs: IO mappings to the device's MMIO > - * @status_regs: CXL 2.0 8.2.8.3 Device Status Registers > - * @mbox_regs: CXL 2.0 8.2.8.4 Mailbox Registers > - * @memdev_regs: CXL 2.0 8.2.8.5 Memory Device Registers > - * @payload_size: Size of space for payload > - * (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register) > - * @mbox_mutex: Mutex to synchronize mailbox access. > - * @firmware_version: Firmware version for the memory device. > - * @enabled_commands: Hardware commands found enabled in CEL. > - * @pmem_range: Persistent memory capacity information. > - * @ram_range: Volatile memory capacity information. > - */ > -struct cxl_mem { > - struct pci_dev *pdev; > - void __iomem *regs; > - struct cxl_memdev *cxlmd; > - > - void __iomem *status_regs; > - void __iomem *mbox_regs; > - void __iomem *memdev_regs; > - > - size_t payload_size; > - struct mutex mbox_mutex; /* Protects device mailbox and firmware */ > - char firmware_version[0x10]; > - unsigned long *enabled_cmds; > - > - struct range pmem_range; > - struct range ram_range; > -}; > - > extern struct bus_type cxl_bus_type; > #endif /* __CXL_H__ */ > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index 244cb7d89678..45871ef65152 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -12,6 +12,7 @@ > #include <linux/io-64-nonatomic-lo-hi.h> > #include "pci.h" > #include "cxl.h" > +#include "mem.h" > > /** > * DOC: cxl mem > @@ -29,12 +30,6 @@ > * - Handle and manage error conditions. > */ > > -/* > - * An entire PCI topology full of devices should be enough for any > - * config > - */ > -#define CXL_MEM_MAX_DEVS 65536 > - > #define cxl_doorbell_busy(cxlm) \ > (readl((cxlm)->mbox_regs + CXLDEV_MBOX_CTRL_OFFSET) & \ > CXLDEV_MBOX_CTRL_DOORBELL) > @@ -91,24 +86,6 @@ struct mbox_cmd { > #define CXL_MBOX_SUCCESS 0 > }; > > -/** > - * struct cxl_memdev - CXL bus object representing a Type-3 Memory Device > - * @dev: driver core device object > - * @cdev: char dev core object for ioctl operations > - * @cxlm: pointer to the parent device driver data > - * @ops_active: active user of @cxlm in ops handlers > - * @ops_dead: completion when all @cxlm ops users have exited > - * @id: id number of this memdev instance. > - */ > -struct cxl_memdev { > - struct device dev; > - struct cdev cdev; > - struct cxl_mem *cxlm; > - struct percpu_ref ops_active; > - struct completion ops_dead; > - int id; > -}; > - Why move this stuff? As far as I could tell, at the end of this patch set this is still only used within mem.c. > static int cxl_mem_major; > static DEFINE_IDA(cxl_memdev_ida); > static struct dentry *cxl_debugfs; > diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h > new file mode 100644 > index 000000000000..daa9aba0e218 > --- /dev/null > +++ b/drivers/cxl/mem.h > @@ -0,0 +1,85 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* Copyright(c) 2020-2021 Intel Corporation. */ > +#ifndef __CXL_MEM_H__ > +#define __CXL_MEM_H__ > + > +/* CXL 2.0 8.2.8.5.1.1 Memory Device Status Register */ > +#define CXLMDEV_STATUS_OFFSET 0x0 > +#define CXLMDEV_DEV_FATAL BIT(0) > +#define CXLMDEV_FW_HALT BIT(1) > +#define CXLMDEV_STATUS_MEDIA_STATUS_MASK GENMASK(3, 2) > +#define CXLMDEV_MS_NOT_READY 0 > +#define CXLMDEV_MS_READY 1 > +#define CXLMDEV_MS_ERROR 2 > +#define CXLMDEV_MS_DISABLED 3 > +#define CXLMDEV_READY(status) \ > + (FIELD_GET(CXLMDEV_STATUS_MEDIA_STATUS_MASK, status) == \ > + CXLMDEV_MS_READY) > +#define CXLMDEV_MBOX_IF_READY BIT(4) > +#define CXLMDEV_RESET_NEEDED_MASK GENMASK(7, 5) > +#define CXLMDEV_RESET_NEEDED_NOT 0 > +#define CXLMDEV_RESET_NEEDED_COLD 1 > +#define CXLMDEV_RESET_NEEDED_WARM 2 > +#define CXLMDEV_RESET_NEEDED_HOT 3 > +#define CXLMDEV_RESET_NEEDED_CXL 4 > +#define CXLMDEV_RESET_NEEDED(status) \ > + (FIELD_GET(CXLMDEV_RESET_NEEDED_MASK, status) != \ > + CXLMDEV_RESET_NEEDED_NOT) > + > +/* > + * An entire PCI topology full of devices should be enough for any > + * config > + */ > +#define CXL_MEM_MAX_DEVS 65536 > + > +/** > + * struct cxl_memdev - CXL bus object representing a Type-3 Memory Device > + * @dev: driver core device object > + * @cdev: char dev core object for ioctl operations > + * @cxlm: pointer to the parent device driver data > + * @ops_active: active user of @cxlm in ops handlers > + * @ops_dead: completion when all @cxlm ops users have exited > + * @id: id number of this memdev instance. > + */ > +struct cxl_memdev { > + struct device dev; > + struct cdev cdev; > + struct cxl_mem *cxlm; > + struct percpu_ref ops_active; > + struct completion ops_dead; > + int id; > +}; > + > +/** > + * struct cxl_mem - A CXL memory device > + * @pdev: The PCI device associated with this CXL device. > + * @regs: IO mappings to the device's MMIO > + * @status_regs: CXL 2.0 8.2.8.3 Device Status Registers > + * @mbox_regs: CXL 2.0 8.2.8.4 Mailbox Registers > + * @memdev_regs: CXL 2.0 8.2.8.5 Memory Device Registers > + * @payload_size: Size of space for payload > + * (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register) > + * @mbox_mutex: Mutex to synchronize mailbox access. > + * @firmware_version: Firmware version for the memory device. > + * @enabled_commands: Hardware commands found enabled in CEL. @enabled_cmds: > + * @pmem_range: Persistent memory capacity information. > + * @ram_range: Volatile memory capacity information. > + */ > +struct cxl_mem { > + struct pci_dev *pdev; > + void __iomem *regs; > + struct cxl_memdev *cxlmd; > + > + void __iomem *status_regs; > + void __iomem *mbox_regs; > + void __iomem *memdev_regs; > + > + size_t payload_size; > + struct mutex mbox_mutex; /* Protects device mailbox and firmware */ > + char firmware_version[0x10]; > + unsigned long *enabled_cmds; > + > + struct range pmem_range; > + struct range ram_range; > +}; > +#endif /* __CXL_MEM_H__ */ >
On Tue, Apr 6, 2021 at 10:47 AM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Thu, 1 Apr 2021 07:30:47 -0700 > Dan Williams <dan.j.williams@intel.com> wrote: > > > In preparation for sharing cxl.h with other generic CXL consumers, > > move / consolidate some of the memory device specifics to mem.h. > > > > Reviewed-by: Ben Widawsky <ben.widawsky@intel.com> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > Hi Dan, > > Would be good to see something in this patch description saying > why you chose to have mem.h rather than push the defines down > into mem.c (which from the current code + patch set looks like > the more logical thing to do). The main motivation was least privilege access to memory-device details, so they had to move out of cxl.h. As to why move them in to a new mem.h instead of piling more into mem.c that's just a personal organizational style choice to aid review. I tend to go to headers first and read data structure definitions before reading the implementation, and having that all in one place is cleaner than interspersed with implementation details in the C code. It's all still private to drivers/cxl/ so I don't see any "least privilege" concerns with moving it there. Does that satisfy your concern? If yes, I'll add the above to v3. > As a side note, docs for struct cxl_mem need a fix as they cover > enabled_commands which at somepoint got shortened to enabled_cmds Thanks, will fix.
On Tue, Apr 13, 2021 at 5:18 PM Dan Williams <dan.j.williams@intel.com> wrote: > > On Tue, Apr 6, 2021 at 10:47 AM Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: > > > > On Thu, 1 Apr 2021 07:30:47 -0700 > > Dan Williams <dan.j.williams@intel.com> wrote: > > > > > In preparation for sharing cxl.h with other generic CXL consumers, > > > move / consolidate some of the memory device specifics to mem.h. > > > > > > Reviewed-by: Ben Widawsky <ben.widawsky@intel.com> > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > > Hi Dan, > > > > Would be good to see something in this patch description saying > > why you chose to have mem.h rather than push the defines down > > into mem.c (which from the current code + patch set looks like > > the more logical thing to do). > > The main motivation was least privilege access to memory-device > details, so they had to move out of cxl.h. As to why move them in to a > new mem.h instead of piling more into mem.c that's just a personal > organizational style choice to aid review. I tend to go to headers > first and read data structure definitions before reading the > implementation, and having that all in one place is cleaner than > interspersed with implementation details in the C code. It's all still > private to drivers/cxl/ so I don't see any "least privilege" concerns > with moving it there. > > Does that satisfy your concern? > > If yes, I'll add the above to v3. Oh, another thing it helps is the information content of diffstats to distinguish definition changes from implementation development.
On Tue, 13 Apr 2021 17:42:37 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > On Tue, Apr 13, 2021 at 5:18 PM Dan Williams <dan.j.williams@intel.com> wrote: > > > > On Tue, Apr 6, 2021 at 10:47 AM Jonathan Cameron > > <Jonathan.Cameron@huawei.com> wrote: > > > > > > On Thu, 1 Apr 2021 07:30:47 -0700 > > > Dan Williams <dan.j.williams@intel.com> wrote: > > > > > > > In preparation for sharing cxl.h with other generic CXL consumers, > > > > move / consolidate some of the memory device specifics to mem.h. > > > > > > > > Reviewed-by: Ben Widawsky <ben.widawsky@intel.com> > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > > > > Hi Dan, > > > > > > Would be good to see something in this patch description saying > > > why you chose to have mem.h rather than push the defines down > > > into mem.c (which from the current code + patch set looks like > > > the more logical thing to do). > > > > The main motivation was least privilege access to memory-device > > details, so they had to move out of cxl.h. As to why move them in to a > > new mem.h instead of piling more into mem.c that's just a personal > > organizational style choice to aid review. I tend to go to headers > > first and read data structure definitions before reading the > > implementation, and having that all in one place is cleaner than > > interspersed with implementation details in the C code. It's all still > > private to drivers/cxl/ so I don't see any "least privilege" concerns > > with moving it there. > > > > Does that satisfy your concern? > > > > If yes, I'll add the above to v3. > > Oh, another thing it helps is the information content of diffstats to > distinguish definition changes from implementation development. I go the other way style wise, but agree it doesn't really matter for local headers included from few other files. Adding a above to comment will at least avoid anyone else (or forgetful me) raising question on v3. Jonathan
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 6f14838c2d25..2e3bdacb32e7 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -34,62 +34,5 @@ #define CXLDEV_MBOX_BG_CMD_STATUS_OFFSET 0x18 #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20 -/* CXL 2.0 8.2.8.5.1.1 Memory Device Status Register */ -#define CXLMDEV_STATUS_OFFSET 0x0 -#define CXLMDEV_DEV_FATAL BIT(0) -#define CXLMDEV_FW_HALT BIT(1) -#define CXLMDEV_STATUS_MEDIA_STATUS_MASK GENMASK(3, 2) -#define CXLMDEV_MS_NOT_READY 0 -#define CXLMDEV_MS_READY 1 -#define CXLMDEV_MS_ERROR 2 -#define CXLMDEV_MS_DISABLED 3 -#define CXLMDEV_READY(status) \ - (FIELD_GET(CXLMDEV_STATUS_MEDIA_STATUS_MASK, status) == \ - CXLMDEV_MS_READY) -#define CXLMDEV_MBOX_IF_READY BIT(4) -#define CXLMDEV_RESET_NEEDED_MASK GENMASK(7, 5) -#define CXLMDEV_RESET_NEEDED_NOT 0 -#define CXLMDEV_RESET_NEEDED_COLD 1 -#define CXLMDEV_RESET_NEEDED_WARM 2 -#define CXLMDEV_RESET_NEEDED_HOT 3 -#define CXLMDEV_RESET_NEEDED_CXL 4 -#define CXLMDEV_RESET_NEEDED(status) \ - (FIELD_GET(CXLMDEV_RESET_NEEDED_MASK, status) != \ - CXLMDEV_RESET_NEEDED_NOT) - -struct cxl_memdev; -/** - * struct cxl_mem - A CXL memory device - * @pdev: The PCI device associated with this CXL device. - * @regs: IO mappings to the device's MMIO - * @status_regs: CXL 2.0 8.2.8.3 Device Status Registers - * @mbox_regs: CXL 2.0 8.2.8.4 Mailbox Registers - * @memdev_regs: CXL 2.0 8.2.8.5 Memory Device Registers - * @payload_size: Size of space for payload - * (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register) - * @mbox_mutex: Mutex to synchronize mailbox access. - * @firmware_version: Firmware version for the memory device. - * @enabled_commands: Hardware commands found enabled in CEL. - * @pmem_range: Persistent memory capacity information. - * @ram_range: Volatile memory capacity information. - */ -struct cxl_mem { - struct pci_dev *pdev; - void __iomem *regs; - struct cxl_memdev *cxlmd; - - void __iomem *status_regs; - void __iomem *mbox_regs; - void __iomem *memdev_regs; - - size_t payload_size; - struct mutex mbox_mutex; /* Protects device mailbox and firmware */ - char firmware_version[0x10]; - unsigned long *enabled_cmds; - - struct range pmem_range; - struct range ram_range; -}; - extern struct bus_type cxl_bus_type; #endif /* __CXL_H__ */ diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index 244cb7d89678..45871ef65152 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -12,6 +12,7 @@ #include <linux/io-64-nonatomic-lo-hi.h> #include "pci.h" #include "cxl.h" +#include "mem.h" /** * DOC: cxl mem @@ -29,12 +30,6 @@ * - Handle and manage error conditions. */ -/* - * An entire PCI topology full of devices should be enough for any - * config - */ -#define CXL_MEM_MAX_DEVS 65536 - #define cxl_doorbell_busy(cxlm) \ (readl((cxlm)->mbox_regs + CXLDEV_MBOX_CTRL_OFFSET) & \ CXLDEV_MBOX_CTRL_DOORBELL) @@ -91,24 +86,6 @@ struct mbox_cmd { #define CXL_MBOX_SUCCESS 0 }; -/** - * struct cxl_memdev - CXL bus object representing a Type-3 Memory Device - * @dev: driver core device object - * @cdev: char dev core object for ioctl operations - * @cxlm: pointer to the parent device driver data - * @ops_active: active user of @cxlm in ops handlers - * @ops_dead: completion when all @cxlm ops users have exited - * @id: id number of this memdev instance. - */ -struct cxl_memdev { - struct device dev; - struct cdev cdev; - struct cxl_mem *cxlm; - struct percpu_ref ops_active; - struct completion ops_dead; - int id; -}; - static int cxl_mem_major; static DEFINE_IDA(cxl_memdev_ida); static struct dentry *cxl_debugfs; diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h new file mode 100644 index 000000000000..daa9aba0e218 --- /dev/null +++ b/drivers/cxl/mem.h @@ -0,0 +1,85 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* Copyright(c) 2020-2021 Intel Corporation. */ +#ifndef __CXL_MEM_H__ +#define __CXL_MEM_H__ + +/* CXL 2.0 8.2.8.5.1.1 Memory Device Status Register */ +#define CXLMDEV_STATUS_OFFSET 0x0 +#define CXLMDEV_DEV_FATAL BIT(0) +#define CXLMDEV_FW_HALT BIT(1) +#define CXLMDEV_STATUS_MEDIA_STATUS_MASK GENMASK(3, 2) +#define CXLMDEV_MS_NOT_READY 0 +#define CXLMDEV_MS_READY 1 +#define CXLMDEV_MS_ERROR 2 +#define CXLMDEV_MS_DISABLED 3 +#define CXLMDEV_READY(status) \ + (FIELD_GET(CXLMDEV_STATUS_MEDIA_STATUS_MASK, status) == \ + CXLMDEV_MS_READY) +#define CXLMDEV_MBOX_IF_READY BIT(4) +#define CXLMDEV_RESET_NEEDED_MASK GENMASK(7, 5) +#define CXLMDEV_RESET_NEEDED_NOT 0 +#define CXLMDEV_RESET_NEEDED_COLD 1 +#define CXLMDEV_RESET_NEEDED_WARM 2 +#define CXLMDEV_RESET_NEEDED_HOT 3 +#define CXLMDEV_RESET_NEEDED_CXL 4 +#define CXLMDEV_RESET_NEEDED(status) \ + (FIELD_GET(CXLMDEV_RESET_NEEDED_MASK, status) != \ + CXLMDEV_RESET_NEEDED_NOT) + +/* + * An entire PCI topology full of devices should be enough for any + * config + */ +#define CXL_MEM_MAX_DEVS 65536 + +/** + * struct cxl_memdev - CXL bus object representing a Type-3 Memory Device + * @dev: driver core device object + * @cdev: char dev core object for ioctl operations + * @cxlm: pointer to the parent device driver data + * @ops_active: active user of @cxlm in ops handlers + * @ops_dead: completion when all @cxlm ops users have exited + * @id: id number of this memdev instance. + */ +struct cxl_memdev { + struct device dev; + struct cdev cdev; + struct cxl_mem *cxlm; + struct percpu_ref ops_active; + struct completion ops_dead; + int id; +}; + +/** + * struct cxl_mem - A CXL memory device + * @pdev: The PCI device associated with this CXL device. + * @regs: IO mappings to the device's MMIO + * @status_regs: CXL 2.0 8.2.8.3 Device Status Registers + * @mbox_regs: CXL 2.0 8.2.8.4 Mailbox Registers + * @memdev_regs: CXL 2.0 8.2.8.5 Memory Device Registers + * @payload_size: Size of space for payload + * (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register) + * @mbox_mutex: Mutex to synchronize mailbox access. + * @firmware_version: Firmware version for the memory device. + * @enabled_commands: Hardware commands found enabled in CEL. + * @pmem_range: Persistent memory capacity information. + * @ram_range: Volatile memory capacity information. + */ +struct cxl_mem { + struct pci_dev *pdev; + void __iomem *regs; + struct cxl_memdev *cxlmd; + + void __iomem *status_regs; + void __iomem *mbox_regs; + void __iomem *memdev_regs; + + size_t payload_size; + struct mutex mbox_mutex; /* Protects device mailbox and firmware */ + char firmware_version[0x10]; + unsigned long *enabled_cmds; + + struct range pmem_range; + struct range ram_range; +}; +#endif /* __CXL_MEM_H__ */