| Message ID | 20221028235705.32890-1-fan.ni@samsung.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [ndctl] Adding cxl_memdev_get_firmware_version | expand |
Fan Ni wrote: > The function for retrieving firmware version is named > `cxl_memdev_get_firmware_verison`, updated to > `cxl_memdev_get_firmware_version'. > keeping `cxl_memdev_get_firmware_verison` for library compatibility. > > Signed-off-by: Fan Ni <fan.ni@samsung.com> > --- > Documentation/cxl/lib/libcxl.txt | 1 + > cxl/lib/libcxl.c | 5 +++++ > cxl/lib/libcxl.sym | 5 +++++ > cxl/libcxl.h | 1 + > 4 files changed, 12 insertions(+) > > diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt > index fd2962a..5a8d980 100644 > --- a/Documentation/cxl/lib/libcxl.txt > +++ b/Documentation/cxl/lib/libcxl.txt > @@ -69,6 +69,7 @@ int cxl_memdev_get_minor(struct cxl_memdev *memdev); > unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev); > unsigned long long cxl_memdev_get_ram_size(struct cxl_memdev *memdev); > const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev); > +const char *cxl_memdev_get_firmware_version(struct cxl_memdev *memdev); > size_t cxl_memdev_get_label_size(struct cxl_memdev *memdev); > int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev); > int cxl_memdev_get_numa_node(struct cxl_memdev *memdev); > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c > index e8c5d44..1b7e9d2 100644 > --- a/cxl/lib/libcxl.c > +++ b/cxl/lib/libcxl.c > @@ -1267,6 +1267,11 @@ CXL_EXPORT const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev > return memdev->firmware_version; > } > > +CXL_EXPORT const char *cxl_memdev_get_firmware_version(struct cxl_memdev *memdev) > +{ > + return memdev->firmware_version; > +} > + It occurs to me that a new export is not needed for this... > static void bus_invalidate(struct cxl_bus *bus) > { > struct cxl_ctx *ctx = cxl_bus_get_ctx(bus); > diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym > index 8bb91e0..d46c50e 100644 > --- a/cxl/lib/libcxl.sym > +++ b/cxl/lib/libcxl.sym > @@ -217,3 +217,8 @@ global: > cxl_decoder_get_max_available_extent; > cxl_decoder_get_region; > } LIBCXL_2; > + > +LIBCXL_4 { > +global: > + cxl_memdev_get_firmware_version; > +} LIBCXL_3; > diff --git a/cxl/libcxl.h b/cxl/libcxl.h > index 9fe4e99..7951e78 100644 > --- a/cxl/libcxl.h > +++ b/cxl/libcxl.h > @@ -48,6 +48,7 @@ struct cxl_ctx *cxl_memdev_get_ctx(struct cxl_memdev *memdev); > unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev); > unsigned long long cxl_memdev_get_ram_size(struct cxl_memdev *memdev); > const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev); > +const char *cxl_memdev_get_firmware_version(struct cxl_memdev *memdev); ...just make this the following instead: /* ABI spelling mistakes are forever */ static inline const char *cxl_memdev_get_firmware_version(struct cxl_memdev *memdev) { return cxl_memdev_get_firmware_verison(memdev); }
On Fri, 28 Oct 2022, Fan Ni wrote: >The function for retrieving firmware version is named >`cxl_memdev_get_firmware_verison`, updated to >`cxl_memdev_get_firmware_version'. >keeping `cxl_memdev_get_firmware_verison` for library compatibility. > Yikes, good catch! While at it there are a few other typos (not abi, fortunately :) diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt index fd2962ad78b5..66c1d14ec5e9 100644 --- a/Documentation/cxl/lib/libcxl.txt +++ b/Documentation/cxl/lib/libcxl.txt @@ -110,7 +110,7 @@ may appear in the topology that were not previously enumerable. NOTE: cxl_memdev_disable_invalidate() will force disable the memdev regardless of whether the memory provided by the device is in active use -by the operating system. Callers take responisbility for assuring that +by the operating system. Callers take responsibility for assuring that it is safe to disable the memory device. Otherwise, this call can be as destructive as ripping a DIMM out of a running system. Like all other libcxl calls that mutate the system state or divulge security sensitive @@ -327,7 +327,7 @@ const char *cxl_dport_get_physical_node(struct cxl_dport *dport); int cxl_dport_get_id(struct cxl_dport *dport); bool cxl_dport_maps_memdev(struct cxl_dport *dport, struct cxl_memdev *memdev); ---- -The id of a dport is the hardware idenfifier used by an upstream port to +The id of a dport is the hardware identifier used by an upstream port to reference a downstream port. The physical node of a dport is only available for platform firmware defined downstream ports and alias the companion object, like a PCI host bridge, in the PCI device hierarchy.
On Fri, 2022-10-28 at 17:15 -0700, Dan Williams wrote: > Fan Ni wrote: >> Subject: RE: [ndctl PATCH] Adding cxl_memdev_get_firmware_version General note - please make sure to denote a revision correctly with something like [PATCH v2] in the subject line. > > The function for retrieving firmware version is named > > `cxl_memdev_get_firmware_verison`, updated to > > `cxl_memdev_get_firmware_version'. > > keeping `cxl_memdev_get_firmware_verison` for library compatibility. > > > > Signed-off-by: Fan Ni <fan.ni@samsung.com> > > --- > > Documentation/cxl/lib/libcxl.txt | 1 + > > cxl/lib/libcxl.c | 5 +++++ > > cxl/lib/libcxl.sym | 5 +++++ > > cxl/libcxl.h | 1 + > > 4 files changed, 12 insertions(+) > > > > diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt > > index fd2962a..5a8d980 100644 > > --- a/Documentation/cxl/lib/libcxl.txt > > +++ b/Documentation/cxl/lib/libcxl.txt > > @@ -69,6 +69,7 @@ int cxl_memdev_get_minor(struct cxl_memdev *memdev); > > unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev); > > unsigned long long cxl_memdev_get_ram_size(struct cxl_memdev *memdev); > > const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev); > > +const char *cxl_memdev_get_firmware_version(struct cxl_memdev *memdev); > > size_t cxl_memdev_get_label_size(struct cxl_memdev *memdev); > > int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev); > > int cxl_memdev_get_numa_node(struct cxl_memdev *memdev); > > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c > > index e8c5d44..1b7e9d2 100644 > > --- a/cxl/lib/libcxl.c > > +++ b/cxl/lib/libcxl.c > > @@ -1267,6 +1267,11 @@ CXL_EXPORT const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev > > return memdev->firmware_version; > > } > > > > +CXL_EXPORT const char *cxl_memdev_get_firmware_version(struct cxl_memdev *memdev) > > +{ > > + return memdev->firmware_version; > > +} > > + > > It occurs to me that a new export is not needed for this... > > > static void bus_invalidate(struct cxl_bus *bus) > > { > > struct cxl_ctx *ctx = cxl_bus_get_ctx(bus); > > diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym > > index 8bb91e0..d46c50e 100644 > > --- a/cxl/lib/libcxl.sym > > +++ b/cxl/lib/libcxl.sym > > @@ -217,3 +217,8 @@ global: > > cxl_decoder_get_max_available_extent; > > cxl_decoder_get_region; > > } LIBCXL_2; > > + > > +LIBCXL_4 { > > +global: > > + cxl_memdev_get_firmware_version; > > +} LIBCXL_3; > > diff --git a/cxl/libcxl.h b/cxl/libcxl.h > > index 9fe4e99..7951e78 100644 > > --- a/cxl/libcxl.h > > +++ b/cxl/libcxl.h > > @@ -48,6 +48,7 @@ struct cxl_ctx *cxl_memdev_get_ctx(struct cxl_memdev *memdev); > > unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev); > > unsigned long long cxl_memdev_get_ram_size(struct cxl_memdev *memdev); > > const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev); > > +const char *cxl_memdev_get_firmware_version(struct cxl_memdev *memdev); > > ...just make this the following instead: > > /* ABI spelling mistakes are forever */ > static inline const char *cxl_memdev_get_firmware_version(struct cxl_memdev *memdev) > { > return cxl_memdev_get_firmware_verison(memdev); > } Agreed with this.
On Tue, Nov 01, 2022 at 07:44:21AM +0000, Verma, Vishal L wrote: Thanks Vishal & Dan for the comments. I will update the patch and send it out for review. Fan > On Fri, 2022-10-28 at 17:15 -0700, Dan Williams wrote: > > Fan Ni wrote: > > >> Subject: RE: [ndctl PATCH] Adding cxl_memdev_get_firmware_version > > General note - please make sure to denote a revision correctly with > something like [PATCH v2] in the subject line. > > > > The function for retrieving firmware version is named > > > `cxl_memdev_get_firmware_verison`, updated to > > > `cxl_memdev_get_firmware_version'. > > > keeping `cxl_memdev_get_firmware_verison` for library compatibility. > > > > > > Signed-off-by: Fan Ni <fan.ni@samsung.com> > > > --- > > > Documentation/cxl/lib/libcxl.txt | 1 + > > > cxl/lib/libcxl.c | 5 +++++ > > > cxl/lib/libcxl.sym | 5 +++++ > > > cxl/libcxl.h | 1 + > > > 4 files changed, 12 insertions(+) > > > > > > diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt > > > index fd2962a..5a8d980 100644 > > > --- a/Documentation/cxl/lib/libcxl.txt > > > +++ b/Documentation/cxl/lib/libcxl.txt > > > @@ -69,6 +69,7 @@ int cxl_memdev_get_minor(struct cxl_memdev *memdev); > > > unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev); > > > unsigned long long cxl_memdev_get_ram_size(struct cxl_memdev *memdev); > > > const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev); > > > +const char *cxl_memdev_get_firmware_version(struct cxl_memdev *memdev); > > > size_t cxl_memdev_get_label_size(struct cxl_memdev *memdev); > > > int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev); > > > int cxl_memdev_get_numa_node(struct cxl_memdev *memdev); > > > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c > > > index e8c5d44..1b7e9d2 100644 > > > --- a/cxl/lib/libcxl.c > > > +++ b/cxl/lib/libcxl.c > > > @@ -1267,6 +1267,11 @@ CXL_EXPORT const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev > > > return memdev->firmware_version; > > > } > > > > > > +CXL_EXPORT const char *cxl_memdev_get_firmware_version(struct cxl_memdev *memdev) > > > +{ > > > + return memdev->firmware_version; > > > +} > > > + > > > > It occurs to me that a new export is not needed for this... > > > > > static void bus_invalidate(struct cxl_bus *bus) > > > { > > > struct cxl_ctx *ctx = cxl_bus_get_ctx(bus); > > > diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym > > > index 8bb91e0..d46c50e 100644 > > > --- a/cxl/lib/libcxl.sym > > > +++ b/cxl/lib/libcxl.sym > > > @@ -217,3 +217,8 @@ global: > > > cxl_decoder_get_max_available_extent; > > > cxl_decoder_get_region; > > > } LIBCXL_2; > > > + > > > +LIBCXL_4 { > > > +global: > > > + cxl_memdev_get_firmware_version; > > > +} LIBCXL_3; > > > diff --git a/cxl/libcxl.h b/cxl/libcxl.h > > > index 9fe4e99..7951e78 100644 > > > --- a/cxl/libcxl.h > > > +++ b/cxl/libcxl.h > > > @@ -48,6 +48,7 @@ struct cxl_ctx *cxl_memdev_get_ctx(struct cxl_memdev *memdev); > > > unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev); > > > unsigned long long cxl_memdev_get_ram_size(struct cxl_memdev *memdev); > > > const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev); > > > +const char *cxl_memdev_get_firmware_version(struct cxl_memdev *memdev); > > > > ...just make this the following instead: > > > > /* ABI spelling mistakes are forever */ > > static inline const char *cxl_memdev_get_firmware_version(struct cxl_memdev *memdev) > > { > > return cxl_memdev_get_firmware_verison(memdev); > > } > > Agreed with this. >
On Mon, Oct 31, 2022 at 10:25:30AM -0700, Davidlohr Bueso wrote: Thanks David. I will include the fix of the typos you mentioned in the next patch. Fan > On Fri, 28 Oct 2022, Fan Ni wrote: > > > The function for retrieving firmware version is named > > `cxl_memdev_get_firmware_verison`, updated to > > `cxl_memdev_get_firmware_version'. > > keeping `cxl_memdev_get_firmware_verison` for library compatibility. > > > > Yikes, good catch! > > While at it there are a few other typos (not abi, fortunately :) > > diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt > index fd2962ad78b5..66c1d14ec5e9 100644 > --- a/Documentation/cxl/lib/libcxl.txt > +++ b/Documentation/cxl/lib/libcxl.txt > @@ -110,7 +110,7 @@ may appear in the topology that were not previously enumerable. > > NOTE: cxl_memdev_disable_invalidate() will force disable the memdev > regardless of whether the memory provided by the device is in active use > -by the operating system. Callers take responisbility for assuring that > +by the operating system. Callers take responsibility for assuring that > it is safe to disable the memory device. Otherwise, this call can be as > destructive as ripping a DIMM out of a running system. Like all other > libcxl calls that mutate the system state or divulge security sensitive > @@ -327,7 +327,7 @@ const char *cxl_dport_get_physical_node(struct cxl_dport *dport); > int cxl_dport_get_id(struct cxl_dport *dport); > bool cxl_dport_maps_memdev(struct cxl_dport *dport, struct cxl_memdev *memdev); > ---- > -The id of a dport is the hardware idenfifier used by an upstream port to > +The id of a dport is the hardware identifier used by an upstream port to > reference a downstream port. The physical node of a dport is only > available for platform firmware defined downstream ports and alias the > companion object, like a PCI host bridge, in the PCI device hierarchy.
diff --git a/Documentation/cxl/lib/libcxl.txt b/Documentation/cxl/lib/libcxl.txt index fd2962a..5a8d980 100644 --- a/Documentation/cxl/lib/libcxl.txt +++ b/Documentation/cxl/lib/libcxl.txt @@ -69,6 +69,7 @@ int cxl_memdev_get_minor(struct cxl_memdev *memdev); unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev); unsigned long long cxl_memdev_get_ram_size(struct cxl_memdev *memdev); const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev); +const char *cxl_memdev_get_firmware_version(struct cxl_memdev *memdev); size_t cxl_memdev_get_label_size(struct cxl_memdev *memdev); int cxl_memdev_nvdimm_bridge_active(struct cxl_memdev *memdev); int cxl_memdev_get_numa_node(struct cxl_memdev *memdev); diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c index e8c5d44..1b7e9d2 100644 --- a/cxl/lib/libcxl.c +++ b/cxl/lib/libcxl.c @@ -1267,6 +1267,11 @@ CXL_EXPORT const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev return memdev->firmware_version; } +CXL_EXPORT const char *cxl_memdev_get_firmware_version(struct cxl_memdev *memdev) +{ + return memdev->firmware_version; +} + static void bus_invalidate(struct cxl_bus *bus) { struct cxl_ctx *ctx = cxl_bus_get_ctx(bus); diff --git a/cxl/lib/libcxl.sym b/cxl/lib/libcxl.sym index 8bb91e0..d46c50e 100644 --- a/cxl/lib/libcxl.sym +++ b/cxl/lib/libcxl.sym @@ -217,3 +217,8 @@ global: cxl_decoder_get_max_available_extent; cxl_decoder_get_region; } LIBCXL_2; + +LIBCXL_4 { +global: + cxl_memdev_get_firmware_version; +} LIBCXL_3; diff --git a/cxl/libcxl.h b/cxl/libcxl.h index 9fe4e99..7951e78 100644 --- a/cxl/libcxl.h +++ b/cxl/libcxl.h @@ -48,6 +48,7 @@ struct cxl_ctx *cxl_memdev_get_ctx(struct cxl_memdev *memdev); unsigned long long cxl_memdev_get_pmem_size(struct cxl_memdev *memdev); unsigned long long cxl_memdev_get_ram_size(struct cxl_memdev *memdev); const char *cxl_memdev_get_firmware_verison(struct cxl_memdev *memdev); +const char *cxl_memdev_get_firmware_version(struct cxl_memdev *memdev); size_t cxl_memdev_get_label_size(struct cxl_memdev *memdev); int cxl_memdev_disable_invalidate(struct cxl_memdev *memdev); int cxl_memdev_enable(struct cxl_memdev *memdev);
The function for retrieving firmware version is named `cxl_memdev_get_firmware_verison`, updated to `cxl_memdev_get_firmware_version'. keeping `cxl_memdev_get_firmware_verison` for library compatibility. Signed-off-by: Fan Ni <fan.ni@samsung.com> --- Documentation/cxl/lib/libcxl.txt | 1 + cxl/lib/libcxl.c | 5 +++++ cxl/lib/libcxl.sym | 5 +++++ cxl/libcxl.h | 1 + 4 files changed, 12 insertions(+)