Re: [PULL 04/53] hw/cxl: Add clear poison mailbox command support.
Peter Maydell wrote: > Ping! This looks like it should be an easy one-liner fix > for a Coverity-detected read-from-bogus-memory bug -- > could one of the CXL folks have a look at it and send > a patch, please ? Done. Jonathan could you double check I only compile tested. I think you are correct and apologies for not seeing your report earlier. Ira
[PATCH] hw/cxl: Fix read from bogus memory
Peter and coverity report:
We've passed '' to address_space_write(), which means "read
from the address on the stack where the function argument 'data'
lives", so instead of writing 64 bytes of data to the guest ,
we'll write 64 bytes which start with a host pointer value and
then continue with whatever happens to be on the host stack
after that.
Indeed the intention was to write 64 bytes of data at the address given.
Fix the parameter to address_space_write().
Reported-by: Peter Maydell
Link:
https://lore.kernel.org/all/cafeaca-u4sytgwtksb__y+_+0o2-wwarntm3x8wnhvl1wfh...@mail.gmail.com/
Fixes: 6bda41a69bdc ("hw/cxl: Add clear poison mailbox command support.")
Cc: Jonathan Cameron
Signed-off-by: Ira Weiny
---
Compile tested only. Jonathan please double check me.
---
hw/mem/cxl_type3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 3e42490b6ce8..582412d9925f 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1025,7 +1025,7 @@ static bool set_cacheline(CXLType3Dev *ct3d, uint64_t
dpa_offset, uint8_t *data)
as = >hostpmem_as;
}
-address_space_write(as, dpa_offset, MEMTXATTRS_UNSPECIFIED, ,
+address_space_write(as, dpa_offset, MEMTXATTRS_UNSPECIFIED, data,
CXL_CACHE_LINE_SIZE);
return true;
}
---
base-commit: 3b2fe44bb7f605f179e5e7feb2c13c2eb3abbb80
change-id: 20240531-fix-poison-set-cacheline-e32bc1e74b27
Best regards,
--
Ira Weiny
[GIT PULL] NVDIMM and DAX for 6.10
Hi Linus, Please pull from
git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git
tags/libnvdimm-for-6.10
... to get code updates for the nvdimm tree. These have been in
linux-next for a couple of weeks. The changes include removing duplicate
code and updating the nvdimm tree to the current kernel interfaces such as
using const for struct device_type and changing the platform remove
callback signature.
Thank you,
Ira Weiny
---
The following changes since commit ed30a4a51bb196781c8058073ea720133a65596f:
Linux 6.9-rc5 (2024-04-21 12:35:54 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git
tags/libnvdimm-for-6.10
for you to fetch changes up to 41147b006be2174b825a54b0620ecf4cc7ec5c84:
dax: remove redundant assignment to variable rc (2024-04-25 14:11:11 -0700)
Updates for nvdimm for 6.10
Code cleanups, remove duplicate code, and updates for current
interfaces.
Christoph Hellwig (2):
nvdimm: remove nd_integrity_init
nvdimm/btt: always set max_integrity_segments
Colin Ian King (1):
dax: remove redundant assignment to variable rc
Ricardo B. Marliere (1):
dax: constify the struct device_type usage
Shivaprasad G Bhat (1):
powerpc/papr_scm: Move duplicate definitions to common header files
Uwe Kleine-König (1):
ndtest: Convert to platform remove callback returning void
MAINTAINERS| 2 +
arch/powerpc/platforms/pseries/papr_scm.c | 43 +--
drivers/dax/bus.c | 3 +-
drivers/nvdimm/btt.c | 12 --
drivers/nvdimm/core.c | 30 -
drivers/nvdimm/nd.h| 1 -
include/linux/papr_scm.h | 49 ++
.../uapi/asm => include/uapi/linux}/papr_pdsm.h| 0
tools/testing/nvdimm/test/ndtest.c | 7 ++--
tools/testing/nvdimm/test/ndtest.h | 31 --
10 files changed, 66 insertions(+), 112 deletions(-)
create mode 100644 include/linux/papr_scm.h
rename {arch/powerpc/include/uapi/asm => include/uapi/linux}/papr_pdsm.h (100%)
Re: [ndctl PATCH v2] cxl/test: use max_available_extent in cxl-destroy-region
alison.schofield@ wrote: > From: Alison Schofield > > Using .size in decoder selection can lead to a set_size failure with > these error messages: > > cxl region: create_region: region8: set_size failed: Numerical result out of > range > > [] cxl_core:alloc_hpa:555: cxl region8: HPA allocation error (-34) for > size:0x2000 in CXL Window 0 [mem 0xf01000-0xf04fff flags > 0x200] > > Use max_available_extent for decoder selection instead. > > The test overlooked the region creation failure because the not 'null' > comparison succeeds when cxl create-region command emits nothing. > Use the ! comparator when checking the create-region result. > > When checking the ram_size output of cxl-list add a check for empty. > > Signed-off-by: Alison Schofield Reviewed-by: Ira Weiny
Re: [PATCH v2 21/25] nvdimm: virtio_pmem: drop owner assignment
Krzysztof Kozlowski wrote:
> virtio core already sets the .owner, so driver does not need to.
>
> Acked-by: Dave Jiang
Acked-by: Ira Weiny
> Signed-off-by: Krzysztof Kozlowski
> ---
>
> Depends on the first patch.
> ---
> drivers/nvdimm/virtio_pmem.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c
> index 4ceced5cefcf..c9b97aeabf85 100644
> --- a/drivers/nvdimm/virtio_pmem.c
> +++ b/drivers/nvdimm/virtio_pmem.c
> @@ -151,7 +151,6 @@ static struct virtio_driver virtio_pmem_driver = {
> .feature_table = features,
> .feature_table_size = ARRAY_SIZE(features),
> .driver.name= KBUILD_MODNAME,
> - .driver.owner = THIS_MODULE,
> .id_table = id_table,
> .validate = virtio_pmem_validate,
> .probe = virtio_pmem_probe,
>
> --
> 2.34.1
>
Re: [PATCH v2 21/25] nvdimm: virtio_pmem: drop owner assignment
Krzysztof Kozlowski wrote:
> virtio core already sets the .owner, so driver does not need to.
>
> Acked-by: Dave Jiang
Acked-by: Ira Weiny
> Signed-off-by: Krzysztof Kozlowski
> ---
>
> Depends on the first patch.
> ---
> drivers/nvdimm/virtio_pmem.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c
> index 4ceced5cefcf..c9b97aeabf85 100644
> --- a/drivers/nvdimm/virtio_pmem.c
> +++ b/drivers/nvdimm/virtio_pmem.c
> @@ -151,7 +151,6 @@ static struct virtio_driver virtio_pmem_driver = {
> .feature_table = features,
> .feature_table_size = ARRAY_SIZE(features),
> .driver.name= KBUILD_MODNAME,
> - .driver.owner = THIS_MODULE,
> .id_table = id_table,
> .validate = virtio_pmem_validate,
> .probe = virtio_pmem_probe,
>
> --
> 2.34.1
>
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
Markus Armbruster wrote: > fan writes: > > > On Wed, Apr 24, 2024 at 03:09:52PM +0200, Markus Armbruster wrote: > >> nifan@gmail.com writes: > >> > >> > From: Fan Ni > >> > > >> > Since fabric manager emulation is not supported yet, the change > >> > implements > >> > the functions to add/release dynamic capacity extents as QMP interfaces. > >> > >> Will fabric manager emulation obsolete these commands? > > > > If in the future, fabric manager emulation supports commands for dynamic > > capacity > > extent add/release, it is possible we do not need the commands. > > But it seems not to happen soon, we need the qmp commands for the > > end-to-end test with kernel DCD support. > > I asked because if the commands are temporary testing aids, they should > probably be declared unstable. Even if they are permanent testing aids, > unstable might be the right choice. This is for the CXL maintainers to > decide. > > What does "unstable" mean? docs/devel/qapi-code-gen.rst: "Interfaces so > marked may be withdrawn or changed incompatibly in future releases." > > Management applications need stable interfaces. Libvirt developers > generally refuse to touch anything in QMP that's declared unstable. > > Human users and their ad hoc scripts appreciate stability, but they > don't need it nearly as much as management applications do. > > A stability promise increases the maintenance burden. By how much is > unclear. In other words, by promising stability, the maintainers take > on risk. Are the CXL maintainers happy to accept the risk here? > Ah... All great points. Outside of CXL development I don't think there is a strong need for them to be stable. I would like to see more than ad hoc scripts use them though. So I don't think they are going to be changed without some thought though. Ira [snip]
Re: [PATCH v3 1/2] cxl/core: correct length of DPA field masks
Shiyang Ruan wrote: > > > 在 2024/4/24 5:04, Ira Weiny 写道: > > Alison Schofield wrote: > >> On Wed, Apr 17, 2024 at 03:50:52PM +0800, Shiyang Ruan wrote: > > > > [snip] > > > >>> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h > >>> index e5f13260fc52..cdfce932d5b1 100644 > >>> --- a/drivers/cxl/core/trace.h > >>> +++ b/drivers/cxl/core/trace.h > >>> @@ -253,7 +253,7 @@ TRACE_EVENT(cxl_generic_event, > >>>* DRAM Event Record > >>>* CXL rev 3.0 section 8.2.9.2.1.2; Table 8-44 > >>>*/ > >>> -#define CXL_DPA_FLAGS_MASK 0x3F > >>> +#define CXL_DPA_FLAGS_MASK 0x3FULL > >>> #define CXL_DPA_MASK(~CXL_DPA_FLAGS_MASK) > >>> > >>> #define CXL_DPA_VOLATILEBIT(0) > >> > >> This works but I'm thinking this is the time to convene on one > >> CXL_EVENT_DPA_MASK for both all CXL events, rather than having > >> cxl_poison event be different. > >> > >> I prefer how poison defines it: > >> > >> cxlmem.h:#define CXL_POISON_START_MASK GENMASK_ULL(63, 6) > >> > >> Can we rename that CXL_EVENT_DPA_MASK and use for all events? > > cxlmem.h:CXL_POISON_START_MASK is defined for MBOX commands (poison > record, the lower 3 bits indicate "Error Source"), but CXL_DPA_MASK here > is for events. They have different meaning though their values are > same. So, I don't think we should consolidate them. By definition the DPA in all these payloads can't use the lower 6 bits. We are defining a mask to get the DPA. This has nothing to do with what may be stored in the other 6 bits. Defining a common DPA mask is correct per both sections of the spec. > > > > > Ah! Great catch. I dont' know why I only masked off the 2 used bits. > > Per spec, the lowest 2 bits of CXL event's DPA are defined for "Volatile > or not" and "not repairable". So there is no mistake here. I appreciate your not calling out my code as a bug. :-D However, bits [5:2] are also Reserved. So it is incorrect to mask off only the lower 2 bits. Even though the reserved bits must be 0 per the spec, it is still better to properly mask all reserved bits because they are by definition not part of the DPA. Could you create a common macro for the next version of the patch? Thanks, Ira > > > That was short sighted of me. > > > > Yes we should consolidate these. > > Ira > > -- > Thanks, > Ruan. >
Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents
Markus Armbruster wrote:
> nifan@gmail.com writes:
>
> > From: Fan Ni
> >
> > Since fabric manager emulation is not supported yet, the change implements
> > the functions to add/release dynamic capacity extents as QMP interfaces.
>
> Will fabric manager emulation obsolete these commands?
I don't think so. In the development of the kernel, I see these being
valuable to do CI and regression testing without the complexity of an FM.
Ira
>
> > Note: we skips any FM issued extent release request if the exact extent
> > does not exist in the extent list of the device. We will loose the
> > restriction later once we have partial release support in the kernel.
> >
> > 1. Add dynamic capacity extents:
> >
> > For example, the command to add two continuous extents (each 128MiB long)
> > to region 0 (starting at DPA offset 0) looks like below:
> >
> > { "execute": "qmp_capabilities" }
> >
> > { "execute": "cxl-add-dynamic-capacity",
> > "arguments": {
> > "path": "/machine/peripheral/cxl-dcd0",
> > "region-id": 0,
> > "extents": [
> > {
> > "dpa": 0,
> > "len": 134217728
> > },
> > {
> > "dpa": 134217728,
> > "len": 134217728
> > }
> > ]
> > }
> > }
> >
> > 2. Release dynamic capacity extents:
> >
> > For example, the command to release an extent of size 128MiB from region 0
> > (DPA offset 128MiB) look like below:
> >
> > { "execute": "cxl-release-dynamic-capacity",
> > "arguments": {
> > "path": "/machine/peripheral/cxl-dcd0",
> > "region-id": 0,
> > "extents": [
> > {
> > "dpa": 134217728,
> > "len": 134217728
> > }
> > ]
> > }
> > }
> >
> > Signed-off-by: Fan Ni
>
> [...]
>
> > diff --git a/qapi/cxl.json b/qapi/cxl.json
> > index 8cc4c72fa9..2645004666 100644
> > --- a/qapi/cxl.json
> > +++ b/qapi/cxl.json
> > @@ -19,13 +19,16 @@
> > #
> > # @fatal: Fatal Event Log
> > #
> > +# @dyncap: Dynamic Capacity Event Log
> > +#
> > # Since: 8.1
> > ##
> > { 'enum': 'CxlEventLog',
> >'data': ['informational',
> > 'warning',
> > 'failure',
> > - 'fatal']
> > + 'fatal',
> > + 'dyncap']
>
> We tend to avoid abbreviations in QMP identifiers: dynamic-capacity.
>
> > }
> >
> > ##
> > @@ -361,3 +364,59 @@
> > ##
> > {'command': 'cxl-inject-correctable-error',
> > 'data': {'path': 'str', 'type': 'CxlCorErrorType'}}
> > +
> > +##
> > +# @CXLDCExtentRecord:
>
> Such traffic jams of capital letters are hard to read.
>
> What does DC mean?
>
> > +#
> > +# Record of a single extent to add/release
> > +#
> > +# @offset: offset to the start of the region where the extent to be
> > operated
>
> Blank line here, please
>
> > +# @len: length of the extent
> > +#
> > +# Since: 9.0
> > +##
> > +{ 'struct': 'CXLDCExtentRecord',
> > + 'data': {
> > + 'offset':'uint64',
> > + 'len': 'uint64'
> > + }
> > +}
> > +
> > +##
> > +# @cxl-add-dynamic-capacity:
> > +#
> > +# Command to start add dynamic capacity extents flow. The device will
>
> I think we're missing an article here. Is it "a flow" or "the flow"?
>
> > +# have to acknowledged the acceptance of the extents before they are
> > usable.
>
> to acknowledge
>
> docs/devel/qapi-code-gen.rst:
>
> For legibility, wrap text paragraphs so every line is at most 70
> characters long.
>
> Separate sentences with two spaces.
>
> > +#
> > +# @path: CXL DCD canonical QOM path
>
> What is a CXL DCD? Is it a device?
>
> I'd prefer @qom-path, unless you can make a consistency argument for
> @path.
>
> > +# @region-id: id of the region where the extent to add
>
> What's a region, and how do they get their IDs?
>
> > +# @extents: Extents to add
>
> Blank lines between argument descriptions, please.
>
> > +#
> > +# Since : 9.0
>
> 9.1
>
> > +##
> > +{ 'command': 'cxl-add-dynamic-capacity',
> > + 'data': { 'path': 'str',
> > +'region-id': 'uint8',
> > +'extents': [ 'CXLDCExtentRecord' ]
> > + }
> > +}
> > +
> > +##
> > +# @cxl-release-dynamic-capacity:
> > +#
> > +# Command to start release dynamic capacity extents flow. The host will
>
> Article again.
>
> The host? In cxl-add-dynamic-capacity's doc comment, it's the device.
>
> > +# need to respond to indicate that it has released the capacity before it
> > +# is made unavailable for read and write and can be re-added.
>
> Is "and can be re-added" relevant here?
>
> > +#
> > +# @path: CXL DCD canonical QOM path
> > +# @region-id: id of the region where the extent to release
> > +# @extents: Extents to release
> > +#
> > +# Since : 9.0
>
> 9.1
>
> > +##
> > +{ 'command': 'cxl-release-dynamic-capacity',
> > + 'data': { 'path': 'str',
> > +'region-id': 'uint8',
> > +'extents': [ 'CXLDCExtentRecord' ]
> > + }
> > +}
>
Re: [PATCH v3 1/2] cxl/core: correct length of DPA field masks
Alison Schofield wrote: > On Wed, Apr 17, 2024 at 03:50:52PM +0800, Shiyang Ruan wrote: [snip] > > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h > > index e5f13260fc52..cdfce932d5b1 100644 > > --- a/drivers/cxl/core/trace.h > > +++ b/drivers/cxl/core/trace.h > > @@ -253,7 +253,7 @@ TRACE_EVENT(cxl_generic_event, > > * DRAM Event Record > > * CXL rev 3.0 section 8.2.9.2.1.2; Table 8-44 > > */ > > -#define CXL_DPA_FLAGS_MASK 0x3F > > +#define CXL_DPA_FLAGS_MASK 0x3FULL > > #define CXL_DPA_MASK (~CXL_DPA_FLAGS_MASK) > > > > #define CXL_DPA_VOLATILE BIT(0) > > This works but I'm thinking this is the time to convene on one > CXL_EVENT_DPA_MASK for both all CXL events, rather than having > cxl_poison event be different. > > I prefer how poison defines it: > > cxlmem.h:#define CXL_POISON_START_MASK GENMASK_ULL(63, 6) > > Can we rename that CXL_EVENT_DPA_MASK and use for all events? Ah! Great catch. I dont' know why I only masked off the 2 used bits. That was short sighted of me. Yes we should consolidate these. Ira
Re: [PATCH v3 2/2] cxl/core: add poison creation event handler
case CXL_EVENT_TRANSACTION_INJECT_POISON:
> + cxl_event_handle_poison(cxlmd, dpa);
> + break;
> + default:
> + break;
> + }
> + }
> +}
> +
> +void cxl_event_handle_record(struct cxl_memdev *cxlmd,
> + enum cxl_event_log_type type,
> + enum cxl_event_type event_type,
> + const uuid_t *uuid, union cxl_event *evt)
> +{
> + if (event_type == CXL_CPER_EVENT_GEN_MEDIA) {
> trace_cxl_general_media(cxlmd, type, >gen_media);
> - else if (event_type == CXL_CPER_EVENT_DRAM)
> + cxl_event_handle_general_media(cxlmd, type, >gen_media);
> + } else if (event_type == CXL_CPER_EVENT_DRAM) {
> trace_cxl_dram(cxlmd, type, >dram);
> - else if (event_type == CXL_CPER_EVENT_MEM_MODULE)
> + cxl_event_handle_dram(cxlmd, type, >dram);
> + } else if (event_type == CXL_CPER_EVENT_MEM_MODULE)
> trace_cxl_memory_module(cxlmd, type, >mem_module);
> else
> trace_cxl_generic_event(cxlmd, type, uuid, >generic);
> }
> -EXPORT_SYMBOL_NS_GPL(cxl_event_trace_record, CXL);
> +EXPORT_SYMBOL_NS_GPL(cxl_event_handle_record, CXL);
>
Why all the churn with changing the names of functions?
Ira
>
> -static void __cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> - enum cxl_event_log_type type,
> - struct cxl_event_record_raw *record)
> +static void __cxl_event_handle_record(struct cxl_memdev *cxlmd,
> + enum cxl_event_log_type type,
> + struct cxl_event_record_raw *record)
> {
> enum cxl_event_type ev_type = CXL_CPER_EVENT_GENERIC;
> const uuid_t *uuid = >id;
> @@ -867,7 +958,7 @@ static void __cxl_event_trace_record(const struct
> cxl_memdev *cxlmd,
> else if (uuid_equal(uuid, _EVENT_MEM_MODULE_UUID))
> ev_type = CXL_CPER_EVENT_MEM_MODULE;
>
> - cxl_event_trace_record(cxlmd, type, ev_type, uuid, >event);
> + cxl_event_handle_record(cxlmd, type, ev_type, uuid, >event);
> }
>
> static int cxl_clear_event_record(struct cxl_memdev_state *mds,
> @@ -979,8 +1070,8 @@ static void cxl_mem_get_records_log(struct
> cxl_memdev_state *mds,
> break;
>
> for (i = 0; i < nr_rec; i++)
> - __cxl_event_trace_record(cxlmd, type,
> - >records[i]);
> + __cxl_event_handle_record(cxlmd, type,
> + >records[i]);
>
> if (payload->flags & CXL_GET_EVENT_FLAG_OVERFLOW)
> trace_cxl_overflow(cxlmd, type, payload);
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 36cee9c30ceb..ba1347de5651 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -822,10 +822,10 @@ void set_exclusive_cxl_commands(struct cxl_memdev_state
> *mds,
> void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
> unsigned long *cmds);
> void cxl_mem_get_event_records(struct cxl_memdev_state *mds, u32 status);
> -void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
> - enum cxl_event_log_type type,
> - enum cxl_event_type event_type,
> - const uuid_t *uuid, union cxl_event *evt);
> +void cxl_event_handle_record(struct cxl_memdev *cxlmd,
> + enum cxl_event_log_type type,
> + enum cxl_event_type event_type,
> + const uuid_t *uuid, union cxl_event *evt);
> int cxl_set_timestamp(struct cxl_memdev_state *mds);
> int cxl_poison_state_init(struct cxl_memdev_state *mds);
> int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
> diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> index 03fa6d50d46f..8189bed76c12 100644
> --- a/include/linux/cxl-event.h
> +++ b/include/linux/cxl-event.h
> @@ -23,6 +23,20 @@ struct cxl_event_generic {
> u8 data[CXL_EVENT_RECORD_DATA_LENGTH];
> } __packed;
>
> +/*
> + * Event transaction type
> + * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
> + */
> +enum cxl_event_transaction_type {
> + CXL_EVENT_TRANSACTION_UNKNOWN = 0X00,
> + CXL_EVENT_TRANSACTION_READ,
> + CXL_EVENT_TRANSACTION_WRITE,
> + CXL_EVENT_TRANSACTION_SCAN_MEDIA,
> + CXL_EVENT_TRANSACTION_INJECT_POISON,
> + CXL_EVENT_TRANSACTION_MEDIA_SCRUB,
> + CXL_EVENT_TRANSACTION_MEDIA_MANAGEMENT,
> +};
> +
> /*
> * General Media Event Record
> * CXL rev 3.0 Section 8.2.9.2.1.1; Table 8-43
> @@ -33,7 +47,7 @@ struct cxl_event_gen_media {
> __le64 phys_addr;
> u8 descriptor;
> u8 type;
> - u8 transaction_type;
> + u8 transaction_type;/* enum cxl_event_transaction_type */
> u8 validity_flags[2];
> u8 channel;
> u8 rank;
> @@ -52,7 +66,7 @@ struct cxl_event_dram {
> __le64 phys_addr;
> u8 descriptor;
> u8 type;
> - u8 transaction_type;
> + u8 transaction_type;/* enum cxl_event_transaction_type */
> u8 validity_flags[2];
> u8 channel;
> u8 rank;
> --
> 2.34.1
>
Re: [PATCH v3 1/2] cxl/core: correct length of DPA field masks
Shiyang Ruan wrote:
> The length of Physical Address in General Media Event Record/DRAM Event
> Record is 64-bit, so the field mask should be defined as such length.
> Otherwise, this causes cxl_general_media and cxl_dram tracepoints to
> mask off the upper-32-bits of DPA addresses. The cxl_poison event is
> unaffected.
>
> If userspace was doing its own DPA-to-HPA translation this could lead to
> incorrect page retirement decisions, but there is no known consumer
> (like rasdaemon) of this event today.
>
> Fixes: d54a531a430b ("cxl/mem: Trace General Media Event Record")
> Cc:
> Cc: Dan Williams
> Cc: Davidlohr Bueso
> Cc: Jonathan Cameron
> Cc: Ira Weiny
Apologies I thought I saw this go in before. But perhaps it was a
different mask.
Reviewed-by: Ira Weiny
> Signed-off-by: Shiyang Ruan
> ---
> drivers/cxl/core/trace.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index e5f13260fc52..cdfce932d5b1 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h
> @@ -253,7 +253,7 @@ TRACE_EVENT(cxl_generic_event,
> * DRAM Event Record
> * CXL rev 3.0 section 8.2.9.2.1.2; Table 8-44
> */
> -#define CXL_DPA_FLAGS_MASK 0x3F
> +#define CXL_DPA_FLAGS_MASK 0x3FULL
> #define CXL_DPA_MASK (~CXL_DPA_FLAGS_MASK)
>
> #define CXL_DPA_VOLATILE BIT(0)
> --
> 2.34.1
>
[MOPO] Wtb Rockwell freedom of speech
Hi there. I am looking for the 28x 40 Norman Rockwell WW2 bond poster for Freedom of Speech. One of the four of the series. It comes in three sizes but I am specifically looking for this medium size. (Of course I have the small and the large version ). Please send me a condition and a price. Thanks, Ira Sent via mobile.Please excuse typos and autocorrects. Visit the MoPo Mailing List Web Site at www.filmfan.com ___ How to UNSUBSCRIBE from the MoPo Mailing List Send a message addressed to: lists...@listserv.american.edu In the BODY of your message type: SIGNOFF MOPO-L The author of this message is solely responsible for its content.
Re: [RFC PATCH v2 00/19] PKS write protected page tables
Edgecombe, Rick P wrote: > On Thu, 2024-03-14 at 09:27 -0700, Kees Cook wrote: > > On Mon, Aug 30, 2021 at 04:59:08PM -0700, Rick Edgecombe wrote: > > > This is a second RFC for the PKS write protected tables concept. > > > I'm sharing to > > > show the progress to interested people. I'd also appreciate any > > > comments, > > > especially on the direct map page table protection solution (patch > > > 17). > > > > *thread necromancy* > > > > Hi, > > > > Where does this series stand? I don't think it ever got merged? > > There are sort of three components to this: > 1. Basic PKS support. It was dropped after the main use case was > rejected (pmem stray write protection). This was the main reason it got dropped. > 2. Solution for applying direct map permissions efficiently. This > includes avoiding excessive kernel shootdowns, as well as avoiding > direct map fragmentation. rppt continued to look at the fragmentation > part of the problem and ended up arguing that it actually isn't an > issue [0]. Regardless, the shootdown problem remains for usages like > PKS tables that allocate so frequently. There is an attempt to address > both in this series. But given the above, there may be lots of debate > and opinions. > 3. The actual protection of the PKS tables (most of this series). It > got paused when I started to work on CET. In the meantime 1 was > dropped, and 2 is still open(?). So there is more to work through now, > then when it was dropped. > > If anyone wants to pick it up, it is fine by me. I can help with > reviews. I can help with reviews as well, Ira > > > [0] https://lwn.net/Articles/931406/
Re: [GNC] Inconsistent MySQL databases
Good point and indeed doing it that way gives consistent sizes. I’ll let the client developer know. > On Mar 1, 2024, at 9:24 AM, Derek Atkins wrote: > > Hi, > > On Fri, March 1, 2024 9:21 am, Ira Fuchs wrote: >> My MySQL client shows tables sizes. > > I would never trust that different versions of databases would store it > the same way.. > > Use a Query to see how much data is in the table. For example: > > select count(*) from XXX; > > This will tell you how many rows are in the table, which will be MUCH more > accurate that looking at "table size." > > -derek > >> >>> On Mar 1, 2024, at 9:02 AM, Derek Atkins wrote: >>> >>> What queries are you running to compare the two databases? >>> >>> -derek >>> >>> On Fri, March 1, 2024 8:53 am, Ira Fuchs wrote: >>>> They are precisely the same. No changes have been made during this >>>> test. >>>> One difference is that one MySQL is running 8.0.22 and the other is >>>> 8.0.36 >>>> but that should not affect the gnucash data table row numbers. >>>> >>>>> On Mar 1, 2024, at 8:38 AM, Derek Atkins wrote: >>>>> >>>>> Hi >>>>> >>>>> On Fri, March 1, 2024 8:30 am, Ira Fuchs wrote: >>>>>> I have 2 instances of MySQL to which I save copies of my Gnucash >>>>>> data. >>>>>> I >>>>>> have gnucash open with an xml file and I do a save to the first >>>>>> instance >>>>>> and then I reopen the XML file and Save again, this time to another >>>>>> MySQL. >>>>>> In both cases the save proceeds normally after warning the file will >>>>>> be >>>>>> overwritten. I then examine the 2 files and in particular the number >>>>>> of >>>>>> records in the transactions, slots, and splits tables and find that >>>>>> they >>>>>> are different by 1000s of rows. (It doesn’t matter if I do the save >>>>>> from >>>>>> xml->MySQL(1)-MySQL(2) or xml->MySQL(1)-XML->MYSQL(2). >>>>>> Can anyone explain to me why there would be any difference in these 2 >>>>>> SQL >>>>>> files? >>>>> >>>>> I cannot explain why they are different, however, in case #2 have you >>>>> compared the two XML files to ensure they are the same? >>>>> >>>>> -derek >>>>> >>>>>> Please remember to CC this list on all your replies. >>>>>> You can do this by using Reply-To-List or Reply-All. >>>>> >>>>> -derek >>>>> >>>>> -- >>>>> Derek Atkins 617-623-3745 >>>>> de...@ihtfp.com www.ihtfp.com >>>>> Computer and Internet Security Consultant >>>>> >>>> >>>> >>> >>> >>> -- >>> Derek Atkins 617-623-3745 >>> de...@ihtfp.com www.ihtfp.com >>> Computer and Internet Security Consultant >>> >> >> > > > -- > Derek Atkins 617-623-3745 > de...@ihtfp.com <mailto:de...@ihtfp.com> www.ihtfp.com > <http://www.ihtfp.com/> > Computer and Internet Security Consultant ___ gnucash-user mailing list gnucash-user@gnucash.org To update your subscription preferences or to unsubscribe: https://lists.gnucash.org/mailman/listinfo/gnucash-user - Please remember to CC this list on all your replies. You can do this by using Reply-To-List or Reply-All.
Re: [GNC] Inconsistent MySQL databases
My MySQL client shows tables sizes. > On Mar 1, 2024, at 9:02 AM, Derek Atkins wrote: > > What queries are you running to compare the two databases? > > -derek > > On Fri, March 1, 2024 8:53 am, Ira Fuchs wrote: >> They are precisely the same. No changes have been made during this test. >> One difference is that one MySQL is running 8.0.22 and the other is 8.0.36 >> but that should not affect the gnucash data table row numbers. >> >>> On Mar 1, 2024, at 8:38 AM, Derek Atkins wrote: >>> >>> Hi >>> >>> On Fri, March 1, 2024 8:30 am, Ira Fuchs wrote: >>>> I have 2 instances of MySQL to which I save copies of my Gnucash data. >>>> I >>>> have gnucash open with an xml file and I do a save to the first >>>> instance >>>> and then I reopen the XML file and Save again, this time to another >>>> MySQL. >>>> In both cases the save proceeds normally after warning the file will be >>>> overwritten. I then examine the 2 files and in particular the number of >>>> records in the transactions, slots, and splits tables and find that >>>> they >>>> are different by 1000s of rows. (It doesn’t matter if I do the save >>>> from >>>> xml->MySQL(1)-MySQL(2) or xml->MySQL(1)-XML->MYSQL(2). >>>> Can anyone explain to me why there would be any difference in these 2 >>>> SQL >>>> files? >>> >>> I cannot explain why they are different, however, in case #2 have you >>> compared the two XML files to ensure they are the same? >>> >>> -derek >>> >>>> Please remember to CC this list on all your replies. >>>> You can do this by using Reply-To-List or Reply-All. >>> >>> -derek >>> >>> -- >>> Derek Atkins 617-623-3745 >>> de...@ihtfp.com www.ihtfp.com >>> Computer and Internet Security Consultant >>> >> >> > > > -- > Derek Atkins 617-623-3745 > de...@ihtfp.com www.ihtfp.com > Computer and Internet Security Consultant > ___ gnucash-user mailing list gnucash-user@gnucash.org To update your subscription preferences or to unsubscribe: https://lists.gnucash.org/mailman/listinfo/gnucash-user - Please remember to CC this list on all your replies. You can do this by using Reply-To-List or Reply-All.
Re: [GNC] Inconsistent MySQL databases
They are precisely the same. No changes have been made during this test. One difference is that one MySQL is running 8.0.22 and the other is 8.0.36 but that should not affect the gnucash data table row numbers. > On Mar 1, 2024, at 8:38 AM, Derek Atkins wrote: > > Hi > > On Fri, March 1, 2024 8:30 am, Ira Fuchs wrote: >> I have 2 instances of MySQL to which I save copies of my Gnucash data. I >> have gnucash open with an xml file and I do a save to the first instance >> and then I reopen the XML file and Save again, this time to another MySQL. >> In both cases the save proceeds normally after warning the file will be >> overwritten. I then examine the 2 files and in particular the number of >> records in the transactions, slots, and splits tables and find that they >> are different by 1000s of rows. (It doesn’t matter if I do the save from >> xml->MySQL(1)-MySQL(2) or xml->MySQL(1)-XML->MYSQL(2). >> Can anyone explain to me why there would be any difference in these 2 SQL >> files? > > I cannot explain why they are different, however, in case #2 have you > compared the two XML files to ensure they are the same? > > -derek > >> Please remember to CC this list on all your replies. >> You can do this by using Reply-To-List or Reply-All. > > -derek > > -- > Derek Atkins 617-623-3745 > de...@ihtfp.com www.ihtfp.com > Computer and Internet Security Consultant > ___ gnucash-user mailing list gnucash-user@gnucash.org To update your subscription preferences or to unsubscribe: https://lists.gnucash.org/mailman/listinfo/gnucash-user - Please remember to CC this list on all your replies. You can do this by using Reply-To-List or Reply-All.
[GNC] Inconsistent MySQL databases
I have 2 instances of MySQL to which I save copies of my Gnucash data. I have gnucash open with an xml file and I do a save to the first instance and then I reopen the XML file and Save again, this time to another MySQL. In both cases the save proceeds normally after warning the file will be overwritten. I then examine the 2 files and in particular the number of records in the transactions, slots, and splits tables and find that they are different by 1000s of rows. (It doesn’t matter if I do the save from xml->MySQL(1)-MySQL(2) or xml->MySQL(1)-XML->MYSQL(2). Can anyone explain to me why there would be any difference in these 2 SQL files? ___ gnucash-user mailing list gnucash-user@gnucash.org To update your subscription preferences or to unsubscribe: https://lists.gnucash.org/mailman/listinfo/gnucash-user - Please remember to CC this list on all your replies. You can do this by using Reply-To-List or Reply-All.
[GNC] Saving to MySQL 8.0.36
I am trying to save my Gbucash data to MySQl v8.0.36 on a newly installed MySQL instance on WIndwos 10. Gnucash is running on a Mac and I also have a MySQL instance on that machine running v .8.0.22. I have no problem saving the gnucash data to the local MySQL instance but when I try to save it to the WIndows instance, I get a non-specific error. My MySQL client can login from the Mac to the Sinwows instance without problem so I know that I have both connectivity and the correct userid/passwd. Can anyone please help me to diagnose why gnucash will not save to the Windows instance. I also tried creating a user entry for the same userid but specifying the Mac ip and Standard authentication. This also failed to work. What am I missing? ___ gnucash-user mailing list gnucash-user@gnucash.org To update your subscription preferences or to unsubscribe: https://lists.gnucash.org/mailman/listinfo/gnucash-user - Please remember to CC this list on all your replies. You can do this by using Reply-To-List or Reply-All.
Re: [MOPO] 29 Years!
Happy Birthday I love this corner of the internet. We should plan a party next year for 30!!! Ira Posterpalace.com (Yes. After years of talking about it. I finally have started to sell some of my collection and moon light in the poster dealer space. I have so much to learn still.).And lots more posters to list. Sent via mobile.Please excuse typos and autocorrects. > On Feb 24, 2024, at 11:23 AM, Tom Martin > wrote: > > CAUTION: This email originated from outside of PBS. Do not click links or > open attachments unless you recognize the sender and know the content is safe. > > > wow Scott Congrats and thank You so much for 29 years > o f allowing me to network with some of the most interesting people > onthe internet, and Movie poster industryand entertainment biz// > I am not sure when i was 1st on however i got on Ebay on Jan 14,1999 > > and we boughta commodore computer in i Think 1884 > before Mopo itwas newspaers only likethe Big Reel, Classic Images. Comic > buyers guide, and Movie collectors workd..I remeber Davusd Kusmoto wrote > articles in Movie collectors workd wheni 1st asvertised it was called > Film collectors world > > I started my biz in may 1977 from a office in Toledo OHIO as i was a > booking agentand musician... > the friendships I have made are countless from MOPO > > and as the owner Scott Burns your patience and kindness and civility is > unmatched in any media or political situation I've ever witnessed your > management skills and brevity is very impressive > You are one of the best examples of a well grounded person that I've > ever experienced on the Internet or in business anywhere thank you so > much for everything you have done and God bless you and your family > > all the MOPO Family I can only say from the bottom of my heart you have > been like brothers and sisters to me and psychiatrist and counselors and > thanks for putting up with all my various comments on life and the > motion picture industry over the years for me you have been Lifesavers > where I could discuss many topics and ways to deal with life and the > motion picture industry entertainment industry > Even when there has been discussions all of that I don't agree with it > is discussed in a fair and equitable way for everyone and even though we > never come to conclusions on many areas we have all learned to give > respect to each other and love so thank you Mopo for the kindness and > brotherhood you've extended to me over the last 29 years or whatever > > I wish everyone peace and love and good health and harmony and it's been > a very interesting ride in life may you all have happiness and health > and friendship > Keep up the good work Scott Burns and again thank you so much for > allowing me to be on your very beautiful news group,, LOVE YOU ALL AND > IT HAS BEEN MY COFFEE SHOP TO VISIT > WHENI CAN SEE OR HAVE NOT BEENIN HOSPITALS OR IN A TOUGH TIME > > > > On 2024-02-24 10:09, Scott Burns wrote: > > ALL THE BEST > TOM > HOLLYWOOD DREAM FACTORY® > SINCE 1977 > > >> Fellow MoPo’ers: >> >> Time is moving way too fast….MoPo marks its 29th birthday today. >> That’s amazing longevity for an Internet group. >> >> February 24, 1995 was the day of the first official MoPo post via the >> American University listserv. >> >> As is my custom, here’s my annual recognition of those 11 first-day >> MoPo members: Mahtab Moayeri, Michael Danese, Rob Ellis, Donna >> Tschetter, Goh Kai Shen, Evan Zweifel, George Nichol, Jeff Static >> (using AOL name Static555), Cindy Nemeth-Johannes, Adam Ehrlich, and >> myself. Michael, Rob and Evan are still with the group. >> >> Our subscriber numbers aren’t what they once were, but I’ll take >> quality over quantity any day. >> >> Thanks again to American University for keeping the listserv up and >> running and thanks to all of you for being here. >> >> Scott >> MoPo List Owner >> >> - >> >> To unsubscribe from the MoPo-L list, click the following link: >> >> https://urldefense.com/v3/__https://listserv.american.edu/scripts/wa-american.exe?SUBED1=MoPo-L=1__;!!Ppj8HQ!PVaSxqUjczkxafNEdVvYgJ1fsCggCaNHipYUJ7GZFoyvLu6tS_t0jdyUHr9gRNOXLkBffpwAm-iUzbLxpbcUhGUor6mL45pQDpw$ >> [1] >> >> Links: >> -- >> [1] >> https://urldefense.com/v3/__https://listserv.american.edu/scripts/wa-american.exe?SUBED1=MoPo-L=1__;!!Ppj8HQ!PVaSxqUjczkxafNEdVvYgJ1fsCggCaNHipYUJ7GZFoyvLu6tS_t0jdyUHr9gRNOXLkBffpwAm-iUzbLxpbcUhGUor6mL45pQDpw$ > > > >Visit the MoPo Mailing List Web Site at > https://urld
Re: [marxmail] At what stage do United Frontists vs Trump pull the plug on Biden?
A statement I posted on my Facebook page: IMO: Only the most hardened Biden Puppet will deny the obvious truth that Joe Biden is mentally and physically deteriorating and should not be a candidate for president. This is what happens when you're a puppet; you deny reality due to your undying loyalty to a specific political party or candidate. This is what Biden Puppets have in common with Trump Puppets. They know he is too old to run but would never say it out loud (except maybe to whisper it under the sheets to an intimate partner). Here’s the thing though; Bidens' age will not affect the outcome of the election. For those that are politically engaged and understand the threat that Trump poses, Bidens’ age is just an inconvenience, a small problem that might result in a less than enthusiastic vote, but a vote none the less. Can the same be said for Bidens’ full-throated support for Israels’ genocidal bombing of Gaza? I would say, emphatically NO. A person has no control over the aging process. As everyone my age knows, we don't consciously decide to get old... it just happens. Making a conscious choice to provide weapons, money and political cover for Netanyahu and his right-wing extremist government is something else altogether. Those deliberate actions mean that he is aiding and abetting the expulsion and mass murder of 2.3 million men, women and children. THAT is the issue that will cost the Democratic Party the election in 2024, and rightfully so. I have no illusions about what a second Trump administration will mean for civil rights, labor rights, political discourse, political violence and international relations during his time in office and probably well beyond. Nevertheless, like millions of other Americans, I will sit this election out. The choice between voting for a genocidal octogenarian or a Mussolini wannabe is no choice at all. Let the chips fall where they may. NO CEASEFIRE, NO VOTE IW -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#29022): https://groups.io/g/marxmail/message/29022 Mute This Topic: https://groups.io/mt/104479559/21656 -=-=- POSTING RULES & NOTES #1 YOU MUST clip all extraneous text when replying to a message. #2 This mail-list, like most, is publicly & permanently archived. #3 Subscribe and post under an alias if #2 is a concern. #4 Do not exceed five posts a day. -=-=- Group Owner: marxmail+ow...@groups.io Unsubscribe: https://groups.io/g/marxmail/leave/8674936/21656/1316126222/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [PATCH v2] acpi/ghes: Prevent sleeping with spinlock held
Jonathan Cameron wrote: > On Wed, 14 Feb 2024 17:33:18 -0500 > Steven Rostedt wrote: > > > On Wed, 14 Feb 2024 14:19:19 -0800 > > Ira Weiny wrote: > > > > > > > Jonathan Cameron wrote: > > > > > > > > > > > So I'm thinking this is a won't fix - wait for the printk rework to > > > > > > land and > > > > > > assume this will be resolved as well? > > > > > > > > > > That pretty much sums up what I was about to say ;-) > > > > > > > > > > tp_printk is more of a hack and not to be used sparingly. With the > > > > > right > > > > > trace events it can hang the machine. > > > > > > > > > > So, you can use your internal patch locally, but I would recommend > > > > > waiting > > > > > for the new printk changes to land. > > > > > > Steven, Do you think that will land in 6.9? > > > > > > > > > > > > > I'm really hoping that will be soon! > > > > > > > > > I may be like Jon Corbet predicting RT will land in mainline if I do. > > > > -- Steve > > > > > Agreed. Don't wait on printk fixes landing. (Well unless you are sure > it's the year of the Linux desktop.) Reverting is fine for 6.8 > if you and Dan feel it's unwise to take this forwards (all the distros > will backport it anyway and 6.8 isn't an LTS so no great rush) > so fine to just queue it up again for 6.9 with this fix. > > As Steve said, tp_printk is a hack (a very useful one) and > hopefully no one runs it in production. OMG... I did not realize what tp_printk() was exactly. I should have looked closer. Do we have evidence of its use in production? I would love to not have to revert/respin, Ira
Re: [PATCH v2] acpi/ghes: Prevent sleeping with spinlock held
Ira Weiny wrote:
> Jonathan Cameron wrote:
> > On Wed, 14 Feb 2024 10:23:10 -0500
> > Steven Rostedt wrote:
> >
> > > On Wed, 14 Feb 2024 12:11:53 +
> > > Jonathan Cameron wrote:
> > >
> > > > So I'm thinking this is a won't fix - wait for the printk rework to
> > > > land and
> > > > assume this will be resolved as well?
> > >
> > > That pretty much sums up what I was about to say ;-)
> > >
> > > tp_printk is more of a hack and not to be used sparingly. With the right
> > > trace events it can hang the machine.
> > >
> > > So, you can use your internal patch locally, but I would recommend waiting
> > > for the new printk changes to land.
>
> Steven, Do you think that will land in 6.9?
>
> > >
> > > I'm really hoping that will be soon!
> > >
> > > -- Steve
> >
> > Thanks Steve,
> >
> > Ira's fix is needed for other valid locking reasons - this was 'just
> > another'
> > lock debugging report that came up whilst testing it.
> >
> > For this patch (not a potential additional one that we aren't going to do ;)
> >
> > Tested-by: Jonathan Cameron
> > Reviewed-by: Jonathan Cameron
>
> Jonathan,
>
> Again thanks for the testing! However, Dan and I just discussed this and
> he has an uneasy feeling about going forward with this for 6.8 final.
>
> If we revert the following patch I can squash this fix and wait for the
> tp_printk() fix to land in 6.9 and resubmit.
>
> Dan here is the patch which backs out the actual bug:
>
> Fixes: 671a794c33c6 ("acpi/ghes: Process CXL Component Events")
Unfortunately this is not the only patch.
We need to revert this too:
Fixes: dc97f6344f20 ("cxl/pci: Register for and process CPER events")
And then revert ...
Fixes: 671a794c33c6 ("acpi/ghes: Process CXL Component Events")
... but there is a conflict.
Dan, below is the correct revert patch. Let me know if you need more.
Ira
commit 807fbe9cac9b190dab83e3ff377a30d18859c8ab
Author: Ira Weiny
Date: Wed Feb 14 15:25:24 2024 -0800
Revert "acpi/ghes: Process CXL Component Events"
This reverts commit 671a794c33c6e048ca5cedd5ad6af44d52d5d7e5.
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index fe825a432c5b..ab2a82cb1b0b 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -26,7 +26,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -674,52 +673,6 @@ static void ghes_defer_non_standard_event(struct
acpi_hest_generic_data *gdata,
schedule_work(>work);
}
-/*
- * Only a single callback can be registered for CXL CPER events.
- */
-static DECLARE_RWSEM(cxl_cper_rw_sem);
-static cxl_cper_callback cper_callback;
-
-static void cxl_cper_post_event(enum cxl_event_type event_type,
- struct cxl_cper_event_rec *rec)
-{
- if (rec->hdr.length <= sizeof(rec->hdr) ||
- rec->hdr.length > sizeof(*rec)) {
- pr_err(FW_WARN "CXL CPER Invalid section length (%u)\n",
- rec->hdr.length);
- return;
- }
-
- if (!(rec->hdr.validation_bits & CPER_CXL_COMP_EVENT_LOG_VALID)) {
- pr_err(FW_WARN "CXL CPER invalid event\n");
- return;
- }
-
- guard(rwsem_read)(_cper_rw_sem);
- if (cper_callback)
- cper_callback(event_type, rec);
-}
-
-int cxl_cper_register_callback(cxl_cper_callback callback)
-{
- guard(rwsem_write)(_cper_rw_sem);
- if (cper_callback)
- return -EINVAL;
- cper_callback = callback;
- return 0;
-}
-EXPORT_SYMBOL_NS_GPL(cxl_cper_register_callback, CXL);
-
-int cxl_cper_unregister_callback(cxl_cper_callback callback)
-{
- guard(rwsem_write)(_cper_rw_sem);
- if (callback != cper_callback)
- return -EINVAL;
- cper_callback = NULL;
- return 0;
-}
-EXPORT_SYMBOL_NS_GPL(cxl_cper_unregister_callback, CXL);
-
static bool ghes_do_proc(struct ghes *ghes,
const struct acpi_hest_generic_status *estatus)
{
@@ -754,22 +707,6 @@ static bool ghes_do_proc(struct ghes *ghes,
}
else if (guid_equal(sec_type, _SEC_PROC_ARM)) {
queued = ghes_handle_arm_hw_error(gdata, sev, sync);
- } else if (guid_equal(sec_type, _SEC_CXL_GEN_MEDIA_GUID)) {
- struct cxl_cper_event_rec *rec =
- acpi_hest_get_payload(gdata);
-
- cxl_cper_post_event(CXL_CPER_EVENT_GEN_MEDIA, rec);
- } else if (guid_equal(sec
Re: [PATCH v2] acpi/ghes: Prevent sleeping with spinlock held
Jonathan Cameron wrote:
> On Wed, 14 Feb 2024 10:23:10 -0500
> Steven Rostedt wrote:
>
> > On Wed, 14 Feb 2024 12:11:53 +
> > Jonathan Cameron wrote:
> >
> > > So I'm thinking this is a won't fix - wait for the printk rework to land
> > > and
> > > assume this will be resolved as well?
> >
> > That pretty much sums up what I was about to say ;-)
> >
> > tp_printk is more of a hack and not to be used sparingly. With the right
> > trace events it can hang the machine.
> >
> > So, you can use your internal patch locally, but I would recommend waiting
> > for the new printk changes to land.
Steven, Do you think that will land in 6.9?
> >
> > I'm really hoping that will be soon!
> >
> > -- Steve
>
> Thanks Steve,
>
> Ira's fix is needed for other valid locking reasons - this was 'just another'
> lock debugging report that came up whilst testing it.
>
> For this patch (not a potential additional one that we aren't going to do ;)
>
> Tested-by: Jonathan Cameron
> Reviewed-by: Jonathan Cameron
Jonathan,
Again thanks for the testing! However, Dan and I just discussed this and
he has an uneasy feeling about going forward with this for 6.8 final.
If we revert the following patch I can squash this fix and wait for the
tp_printk() fix to land in 6.9 and resubmit.
Dan here is the patch which backs out the actual bug:
Fixes: 671a794c33c6 ("acpi/ghes: Process CXL Component Events")
Thanks everyone,
Ira
Re: [PATCH v2] acpi/ghes: Prevent sleeping with spinlock held
Jonathan Cameron wrote: > On Wed, 14 Feb 2024 10:23:10 -0500 > Steven Rostedt wrote: > > > On Wed, 14 Feb 2024 12:11:53 + > > Jonathan Cameron wrote: > > > > > So I'm thinking this is a won't fix - wait for the printk rework to land > > > and > > > assume this will be resolved as well? > > > > That pretty much sums up what I was about to say ;-) > > > > tp_printk is more of a hack and not to be used sparingly. With the right > > trace events it can hang the machine. > > > > So, you can use your internal patch locally, but I would recommend waiting > > for the new printk changes to land. I'm really hoping that will be soon! > > > > -- Steve > > Thanks Steve, > > Ira's fix is needed for other valid locking reasons - this was 'just another' > lock debugging report that came up whilst testing it. > > For this patch (not a potential additional one that we aren't going to do ;) > > Tested-by: Jonathan Cameron > Reviewed-by: Jonathan Cameron Thanks Jonathan! I really appreciate the testing, Ira
Re: [PATCH v2] acpi/ghes: Prevent sleeping with spinlock held
Ira Weiny wrote:
> Jonathan Cameron wrote:
> > On Tue, 06 Feb 2024 14:15:32 -0800
> > Ira Weiny wrote:
> >
> >
[snip]
> >
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 9ff8a439d674..7ee45f22f56f 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -2957,7 +2957,7 @@ static void output_printk(struct trace_event_buffer
> > *fbuffer)
> > iter->ent = fbuffer->entry;
> > event_call->event.funcs->trace(iter, 0, event);
> > trace_seq_putc(>seq, 0);
> > - printk("%s", iter->seq.buffer);
> > + printk_deferred("%s", iter->seq.buffer);
> >
> > raw_spin_unlock_irqrestore(_iter_lock, flags);
> > }
> >
> > My assumption is similar views will apply here to Peter Zijlstra's comment
> > on
> > not using printk_deferred.
> >
> > https://lore.kernel.org/all/20231010141244.gm...@noisy.programming.kicks-ass.net/
> >
> > Note I also tried the hacks Peter links to from there. They trip issues in
> > the initial
> > CPER print - so I assume not appropriate here.
> >
> > So I'm thinking this is a won't fix - wait for the printk rework to land and
> > assume this will be resolved as well?
> >
>
> Or could we avoid the situation entirely by using a static call?
>
> The work queue still needs to be created because of the atomicness of
> ghes_do_proc() but it avoids cxl_cper_rw_sem.
>
> I think the hardest thing may be writing the commit message to explain all
> this. :-(
>
Never mind, I already went down that path. I was surprised I did not
mention it in this commit message. I did in V1. :-(
"A static call was considered but ARM does not select HAVE_STATIC_CALL
and in that case setting the function pointer uses a RW semaphore."
--
https://lore.kernel.org/all/20240202-cxl-cper-smatch-v1-1-7a4103c7f...@intel.com/
Should have carried that through.
So should we revert ...
Fixes: 671a794c33c6 ("acpi/ghes: Process CXL Component Events")
... and wait for the printk fix or just move forward with this patch?
Sorry for the noise,
Ira
Re: [PATCH v2] acpi/ghes: Prevent sleeping with spinlock held
Jonathan Cameron wrote:
> On Tue, 06 Feb 2024 14:15:32 -0800
> Ira Weiny wrote:
>
> > Smatch caught that cxl_cper_post_event() is called with a spinlock held
> > or preemption disabled.[1] The callback takes the device lock to
> > perform address translation and therefore might sleep. The record data
> > is released back to BIOS in ghes_clear_estatus() which requires it to be
> > copied for use in the workqueue.
> >
> > Copy the record to a lockless list and schedule a work item to process
> > the record outside of atomic context.
> >
> > [1]
> > https://lore.kernel.org/all/b963c490-2c13-4b79-bbe7-34c6568423c7@moroto.mountain/
> >
> > Reported-by: Dan Carpenter
> > Signed-off-by: Ira Weiny
>
> +CC tracing folk for the splat below (the second one as first one is fixed!)
>
> Lock debugging is slow (on an emulated machine) :(
> Testing with my gitlab.com/jic23/qemu cxl-2024-02-05-draft branch
> which is only one I've put out with the FW first injection patches so far
>
> For reference without this patch I got a nice spat identifying the original
> bug.
> So far so good.
>
> With this patch (and tp_printk in command line and trace points enabled)
> [ 193.048229] {1}[Hardware Error]: Hardware error from APEI Generic Hardware
> Error Source: 1
> [ 193.049636] {1}[Hardware Error]: event severity: recoverable
> [ 193.050472] {1}[Hardware Error]: Error 0, type: recoverable
> [ 193.051337] {1}[Hardware Error]: section type: unknown,
> fbcd0a77-c260-417f-85a9-088b1621eba6
> [ 193.052270] {1}[Hardware Error]: section length: 0x90
> [ 193.053402] {1}[Hardware Error]: : 0090 0007
> 0d938086
> [ 193.055036] {1}[Hardware Error]: 0010: 000e 0005
>
> [ 193.058592] {1}[Hardware Error]: 0020: 0180 1626fa24
> 17b3b158 $.&.X...
> [ 193.062289] {1}[Hardware Error]: 0030:
>
> [ 193.065959] {1}[Hardware Error]: 0040: 07d0 0fc00307
> 05210300 ..!.
> [ 193.069782] {1}[Hardware Error]: 0050: 7269 6d207361 00326d65
> ..iras mem2.
> [ 193.072760] {1}[Hardware Error]: 0060:
>
> [ 193.074062] {1}[Hardware Error]: 0070:
>
> [ 193.075346] {1}[Hardware Error]: 0080:
>
>
> I rebased after this so now we get the smaller print - but that's not really
> relevant here.
>
> [ 193.086589] cxl_general_media: memdev=mem1 host=:0e:00.0 serial=5
> log=Warning : time=1707903675590441508
> uuid=fbcd0a77-c260-417f-85a9-088b1621eba6 len=128 flags='0x1' handle=0
> related_handle=0 maint_op_class=0 : dpa=7c0 dpa_flags='0x10'
> descriptor='UNCORRECTABLE_EVENT|THRESHOLD_EVENT|POISON_LIST_OVERFLOW'
> type='0x3' transaction_type='0xc0' channel=3 rank=33 device=5 comp_id=69 72
> 61 73 20 6d 65 6d 32 00 00 00 00 00 00 00
> validity_flags='CHANNEL|RANK|DEVICE|COMPONENT'
> [ 193.087181]
>
>
>
> [ 193.087361] =
> [ 193.087399] [ BUG: Invalid wait context ]
> [ 193.087504] 6.8.0-rc3 #1200 Not tainted
> [ 193.087660] -
> [ 193.087858] kworker/3:0/31 is trying to lock:
> [ 193.087966] c0ce1898 (_lock_key){-.-.}-{3:3}, at:
> pl011_console_write+0x148/0x1c8
> [ 193.089754] other info that might help us debug this:
> [ 193.089820] context-{5:5}[ 193.089900] 8 locks held by kworker/3:0/31:
> [ 193.089990] #0: c0018738 ((wq_completion)events){+.+.}-{0:0}, at:
> process_one_work+0x154/0x500
> [ 193.090439] #1: 800083793de0 (cxl_cper_work){+.+.}-{0:0}, at:
> process_one_work+0x154/0x500
> [ 193.090718] #2: 800082883310 (cxl_cper_rw_sem){}-{4:4}, at:
> cxl_cper_work_fn+0x2c/0xb0
> [ 193.091019] #3: c0a7b1a8 (>mutex){}-{4:4}, at:
> pci_dev_lock+0x28/0x48
> [ 193.091431] #4: 800082738f18 (tracepoint_iter_lock){}-{2:2}, at:
> trace_event_buffer_commit+0xd8/0x2c8
> [ 193.091772] #5: 8000826b3ce8 (console_lock){+.+.}-{0:0}, at:
> vprintk_emit+0x124/0x398
> [ 193.092031] #6: 8000826b3d40 (console_srcu){}-{0:0}, at:
> console_flush_all+0x88/0x4b0
> [ 193.092363] #7: 8000826b3ef8 (conso
Re: [PATCH] libnvdimm: Fix ACPI_NFIT in BLK_DEV_PMEM help
Peter Robinson wrote:
> The ACPI_NFIT config option is described incorrectly as the
> inverse NFIT_ACPI, which doesn't exist, so update the help
> to the actual config option.
>
> Fixes: 18da2c9ee41a0 ("libnvdimm, pmem: move pmem to drivers/nvdimm/")
I don't think this warrants a fixes tag as this commit has been around
since 2015 and has not bothered anyone. But the change is valid for the
next merge window.
> Signed-off-by: Peter Robinson
> ---
> drivers/nvdimm/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
> index 77b06d54cc62e..fde3e17c836c8 100644
> --- a/drivers/nvdimm/Kconfig
> +++ b/drivers/nvdimm/Kconfig
> @@ -24,7 +24,7 @@ config BLK_DEV_PMEM
> select ND_PFN if NVDIMM_PFN
> help
> Memory ranges for PMEM are described by either an NFIT
> - (NVDIMM Firmware Interface Table, see CONFIG_NFIT_ACPI), a
> + (NVDIMM Firmware Interface Table, see CONFIG_ACPI_NFIT), a
> non-standard OEM-specific E820 memory type (type-12, see
> CONFIG_X86_PMEM_LEGACY), or it is manually specified by the
> 'memmap=nn[KMG]!ss[KMG]' kernel command line (see
> --
> 2.43.0
>
Re: [PATCH] bswap.h: Fix const_le64() macro
Peter Maydell wrote:
> The const_le64() macro introduced in commit 845d80a8c7b187 turns out
> to have a bug which means that on big-endian systems the compiler
> complains if the argument isn't already a 64-bit type. This hasn't
> caused a problem yet, because there are no in-tree uses, but it
> means it's not possible for anybody to add one without it failing CI.
>
> This example is from an attempted use of it with the argument '0',
> from the s390 CI runner's gcc:
>
> ../block/blklogwrites.c: In function ‘blk_log_writes_co_do_log’:
> ../include/qemu/bswap.h:148:36: error: left shift count >= width of
> type [-Werror=shift-count-overflow]
> 148 | _x) & 0x00ffU) << 56) | \
> | ^~
> ../block/blklogwrites.c:409:27: note: in expansion of macro ‘const_le64’
> 409 | .nr_entries = const_le64(0),
> | ^~
> ../include/qemu/bswap.h:149:36: error: left shift count >= width of
> type [-Werror=shift-count-overflow]
> 149 | (((_x) & 0xff00U) << 40) | \
> | ^~
> ../block/blklogwrites.c:409:27: note: in expansion of macro ‘const_le64’
> 409 | .nr_entries = const_le64(0),
> | ^~
> cc1: all warnings being treated as errors
>
> Fix this by making all the constants in the macro have the ULL
> suffix. This will cause them all to be 64-bit integers, which means
> the result of the logical & will also be an unsigned 64-bit type,
> even if the input to the macro is a smaller type, and so the shifts
> will be in range.
>
> Fixes: 845d80a8c7b187 ("qemu/bswap: Add const_le64()")
> Signed-off-by: Peter Maydell
Thanks!
Reviewed-by: Ira Weiny
Re: [marxmail] PSL ups the crowd-inflation factor
While there, I estimated 25K-50K. After viewing aerial photos, I revised that number to 100K max. Of course, these estimates are always difficult but 400K? No way. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#28532): https://groups.io/g/marxmail/message/28532 Mute This Topic: https://groups.io/mt/103794305/21656 -=-=- POSTING RULES & NOTES #1 YOU MUST clip all extraneous text when replying to a message. #2 This mail-list, like most, is publicly & permanently archived. #3 Subscribe and post under an alias if #2 is a concern. #4 Do not exceed five posts a day. -=-=- Group Owner: marxmail+ow...@groups.io Unsubscribe: https://groups.io/g/marxmail/leave/8674936/21656/1316126222/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[GIT PULL] NVDIMM/NFIT changes for 6.8
Hi Linus, please pull from: git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git tags/libnvdimm-for-6.8 ... to get updates to the nvdimm tree. They are a mix of bug fixes and updates to interfaces used by nvdimm. Updates to interfaces include: Use the new scope based management Remove deprecated ida interfaces Update to sysfs_emit() Fixup kdoc comments They have all been in -next more than 6 days with no reported issues. --- The following changes since commit 610a9b8f49fbcf1100716370d3b5f6f884a2835a: Linux 6.7-rc8 (2023-12-31 12:51:25 -0800) are available in the Git repository at: g...@gitolite.kernel.org:pub/scm/linux/kernel/git/nvdimm/nvdimm.git tags/libnvdimm-for-6.8 for you to fetch changes up to a085a5eb6594a3ebe5c275e9c2c2d341f686c23c: acpi/nfit: Use sysfs_emit() for all attributes (2024-01-03 12:21:37 -0800) libnvdimm updates for v6.8 - updates to deprecated and changed interfaces - use new cleanup.h features - use new ida interface - kdoc fixes Christophe JAILLET (1): nvdimm: Remove usage of the deprecated ida_simple_xx() API Dan Williams (1): acpi/nfit: Use sysfs_emit() for all attributes Dinghao Liu (1): nvdimm-btt: simplify code with the scope based resource management Michal Wilczynski (1): ACPI: NFIT: Use cleanup.h helpers instead of devm_*() Randy Dunlap (3): nvdimm/btt: fix btt_blk_cleanup() kernel-doc nvdimm/dimm_devs: fix kernel-doc for function params nvdimm/namespace: fix kernel-doc for function params drivers/acpi/nfit/core.c| 65 +++-- drivers/nvdimm/btt.c| 15 -- drivers/nvdimm/btt_devs.c | 6 ++-- drivers/nvdimm/bus.c| 4 +-- drivers/nvdimm/dax_devs.c | 4 +-- drivers/nvdimm/dimm_devs.c | 17 --- drivers/nvdimm/namespace_devs.c | 19 drivers/nvdimm/pfn_devs.c | 4 +-- 8 files changed, 71 insertions(+), 63 deletions(-)
Re: [PATCH 3/3] nvdimm/namespace: fix kernel-doc for function params
Randy Dunlap wrote: > > > On 12/21/23 14:32, Ira Weiny wrote: > > Randy Dunlap wrote: > > > > [snip] > > > >> @@ -1656,8 +1664,10 @@ static int select_pmem_id(struct nd_regi > >> /** > >> * create_namespace_pmem - validate interleave set labelling, retrieve > >> label0 > >> * @nd_region: region with mappings to validate > >> - * @nspm: target namespace to create > >> + * @nd_mapping: container of dpa-resource-root + labels > >> * @nd_label: target pmem namespace label to evaluate > >> + * > >> + * Returns: the created device on success or -errno on error > > > > NIT: should this be ERR_PTR(-errno) on error? > > Oh, for sure. Thanks for catching that. I'll clean this up as well if you are good with me changing patch 2/3 as well. Ira
Re: [PATCH 2/3] nvdimm/dimm_devs: fix kernel-doc for function params
Ira Weiny wrote: > Randy Dunlap wrote: [snip] > > diff -- a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c > > --- a/drivers/nvdimm/dimm_devs.c > > +++ b/drivers/nvdimm/dimm_devs.c > > @@ -53,7 +53,10 @@ static int validate_dimm(struct nvdimm_d > > > > /** > > * nvdimm_init_nsarea - determine the geometry of a dimm's namespace area > > - * @nvdimm: dimm to initialize > > + * @ndd: dimm to initialize > > + * > > + * Returns: %0 if the area is already valid, -errno on error, > > This is good... > > > ...otherwise an > > + * ND_CMD_* status code. > > I don't see where any of the ->ndctl() calls return an ND_CMD_* status > code. What am I missing? If you agree that this should be dropped I can clean it up as I'm trying to prep the nvdimm tree now and was hoping to take this series. Ira > > The rest looks good, > Ira
Re: [PATCH 3/3] nvdimm/namespace: fix kernel-doc for function params
Randy Dunlap wrote: [snip] > @@ -1656,8 +1664,10 @@ static int select_pmem_id(struct nd_regi > /** > * create_namespace_pmem - validate interleave set labelling, retrieve label0 > * @nd_region: region with mappings to validate > - * @nspm: target namespace to create > + * @nd_mapping: container of dpa-resource-root + labels > * @nd_label: target pmem namespace label to evaluate > + * > + * Returns: the created device on success or -errno on error NIT: should this be ERR_PTR(-errno) on error? Generally good to me though. Reviewed-by: Ira Weiny > */ > static struct device *create_namespace_pmem(struct nd_region *nd_region, > struct nd_mapping *nd_mapping,
Re: [PATCH 2/3] nvdimm/dimm_devs: fix kernel-doc for function params
Randy Dunlap wrote:
> Adjust kernel-doc notation to prevent warnings when using -Wall.
>
> dimm_devs.c:59: warning: Function parameter or member 'ndd' not described in
> 'nvdimm_init_nsarea'
> dimm_devs.c:59: warning: Excess function parameter 'nvdimm' description in
> 'nvdimm_init_nsarea'
> dimm_devs.c:59: warning: No description found for return value of
> 'nvdimm_init_nsarea'
> dimm_devs.c:728: warning: No description found for return value of
> 'nd_pmem_max_contiguous_dpa'
> dimm_devs.c:773: warning: No description found for return value of
> 'nd_pmem_available_dpa'
> dimm_devs.c:844: warning: Function parameter or member 'ndd' not described in
> 'nvdimm_allocated_dpa'
> dimm_devs.c:844: warning: Excess function parameter 'nvdimm' description in
> 'nvdimm_allocated_dpa'
> dimm_devs.c:844: warning: No description found for return value of
> 'nvdimm_allocated_dpa'
>
> Signed-off-by: Randy Dunlap
> Cc: Dan Williams
> Cc: Vishal Verma
> Cc: Dave Jiang
> Cc: Ira Weiny
> Cc: nvd...@lists.linux.dev
> ---
> drivers/nvdimm/dimm_devs.c | 14 --
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff -- a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c
> --- a/drivers/nvdimm/dimm_devs.c
> +++ b/drivers/nvdimm/dimm_devs.c
> @@ -53,7 +53,10 @@ static int validate_dimm(struct nvdimm_d
>
> /**
> * nvdimm_init_nsarea - determine the geometry of a dimm's namespace area
> - * @nvdimm: dimm to initialize
> + * @ndd: dimm to initialize
> + *
> + * Returns: %0 if the area is already valid, -errno on error,
This is good...
> ...otherwise an
> + * ND_CMD_* status code.
I don't see where any of the ->ndctl() calls return an ND_CMD_* status
code. What am I missing?
The rest looks good,
Ira
> */
> int nvdimm_init_nsarea(struct nvdimm_drvdata *ndd)
> {
> @@ -722,6 +725,9 @@ static unsigned long dpa_align(struct nd
> * contiguous unallocated dpa range.
> * @nd_region: constrain available space check to this reference region
> * @nd_mapping: container of dpa-resource-root + labels
> + *
> + * Returns: %0 if there is an alignment error, otherwise the max
> + * unallocated dpa range
> */
> resource_size_t nd_pmem_max_contiguous_dpa(struct nd_region *nd_region,
> struct nd_mapping *nd_mapping)
> @@ -767,6 +773,8 @@ resource_size_t nd_pmem_max_contiguous_d
> *
> * Validate that a PMEM label, if present, aligns with the start of an
> * interleave set.
> + *
> + * Returns: %0 if there is an alignment error, otherwise the unallocated dpa
> */
> resource_size_t nd_pmem_available_dpa(struct nd_region *nd_region,
> struct nd_mapping *nd_mapping)
> @@ -836,8 +844,10 @@ struct resource *nvdimm_allocate_dpa(str
>
> /**
> * nvdimm_allocated_dpa - sum up the dpa currently allocated to this label_id
> - * @nvdimm: container of dpa-resource-root + labels
> + * @ndd: container of dpa-resource-root + labels
> * @label_id: dpa resource name of the form pmem-
> + *
> + * Returns: sum of the dpa allocated to the label_id
> */
> resource_size_t nvdimm_allocated_dpa(struct nvdimm_drvdata *ndd,
> struct nd_label_id *label_id)
Re: [PATCH 1/3] nvdimm/btt: fix btt_blk_cleanup() kernel-doc
Randy Dunlap wrote: > Correct the function parameters to prevent kernel-doc warnings: > > btt.c:1567: warning: Function parameter or member 'nd_region' not described > in 'btt_init' > btt.c:1567: warning: Excess function parameter 'maxlane' description in > 'btt_init' > > Signed-off-by: Randy Dunlap > Cc: Vishal Verma > Cc: Dan Williams > Cc: Dave Jiang > Cc: Ira Weiny Reviewed-by: Ira Weiny > Cc: nvd...@lists.linux.dev > --- > drivers/nvdimm/btt.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff -- a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c > --- a/drivers/nvdimm/btt.c > +++ b/drivers/nvdimm/btt.c > @@ -1550,7 +1550,7 @@ static void btt_blk_cleanup(struct btt * > * @rawsize: raw size in bytes of the backing device > * @lbasize: lba size of the backing device > * @uuid:A uuid for the backing device - this is stored on media > - * @maxlane: maximum number of parallel requests the device can handle > + * @nd_region:nd_region for the REGION device > * > * Initialize a Block Translation Table on a backing device to provide > * single sector power fail atomicity.
Re: [PATCH v3] ACPI: NFIT: Use cleanup.h helpers instead of devm_*()
Rafael J. Wysocki wrote: > On Wed, Oct 18, 2023 at 6:28 AM Dan Williams wrote: > > > > Michal Wilczynski wrote: > > > The new cleanup.h facilities that arrived in v6.5-rc1 can replace the > > > the usage of devm semantics in acpi_nfit_init_interleave_set(). That > > > routine appears to only be using devm to avoid goto statements. The > > > new __free() annotation at variable declaration time can achieve the same > > > effect more efficiently. > > > > > > There is no end user visible side effects of this patch, I was > > > motivated to send this cleanup to practice using the new helpers. > > > > > > Suggested-by: Dave Jiang > > > Suggested-by: Andy Shevchenko > > > Reviewed-by: Dave Jiang > > > Reviewed-by: Andy Shevchenko > > > Signed-off-by: Michal Wilczynski > > > --- > > > > > > Dan, would you like me to give you credit for the changelog changes > > > with Co-developed-by tag ? > > > > Nope, this looks good to me, thanks for fixing it up. > > > > Reviewed-by: Dan Williams > > Are you going to apply it too, or should I take it? I'm prepping the nvdimm tree for 6.8. I will take it. Ira
Re: [TLS] Adoption call for 'TLS 1.2 Feature Freeze'
+1 to Tim - tell the reader explicitly that they will only ever get PQC w/ TLS 1.3 or higher. Cheers, - Ira On Thu, Dec 21, 2023, 12:34 PM Tim Hollebeek wrote: > I personally think this point is important enough to be made explicitly > instead of implicitly. > > > > If we want to communicate loudly and clearly that post-quantum > cryptography is NEVER coming to TLS 1.2, we need to explicitly say that. > > > > Otherwise people will say “I know you said TLS 1.2 was frozen, but > post-quantum cryptography isn’t a feature, it’s a critical security > vulnerability that needs to be patched regardless of any freezes.” > > > > The answer will be and needs to be: “No, we told you clearly and > explicitly that post-quantum cryptography would require moving to TLS 1.3 > or later”. > > > > -Tim > > > > *From:* TLS *On Behalf Of *Hannes Tschofenig > *Sent:* Monday, December 11, 2023 12:06 PM > *To:* Salz, Rich ; Hannes Tschofenig > ; Bas Westerbaan 40cloudflare@dmarc.ietf.org>; Deirdre Connolly < > durumcrustu...@gmail.com> > *Cc:* TLS@ietf.org > *Subject:* Re: [TLS] Adoption call for 'TLS 1.2 Feature Freeze' > > > > Hi Rich, > > > > that is implied by a "feature freeze". No reason to highlight PQC (even > though it is a hype topic right now). > > > > Ciao > > Hannes > > > > Am 11.12.2023 um 17:18 schrieb Salz, Rich: > > 1. I consider Section 3 "Implications for post-quantum > cryptography" misplaced. I suggest to delete the section > > 2. The motivation for the draft is unrelated to developments with > PQC. > > The point is to explain to people that we are going to need PQ crypto, and > it **will not be a 1.2 enhancement** > > > > > > ___ > > TLS mailing list > > TLS@ietf.org > > https://www.ietf.org/mailman/listinfo/tls > > ___ > TLS mailing list > TLS@ietf.org > https://www.ietf.org/mailman/listinfo/tls > ___ TLS mailing list TLS@ietf.org https://www.ietf.org/mailman/listinfo/tls
Re: [PATCH] [v2] nvdimm-btt: fix several memleaks
dinghao.liu@ wrote:
> > Ira Weiny wrote:
> > > Dinghao Liu wrote:
> >
> > [snip]
> >
> > -static int btt_freelist_init(struct arena_info *arena)
> > +static int btt_freelist_init(struct device *dev, struct arena_info *arena)
> >
> > Both struct arena_info and struct btt contain references to struct nd_btt
> > which is the device you are passing down this call stack.
> >
> > Just use the device in the arena/btt rather than passing a device
> > structure. That makes the code easier to read and ensures that the device
> > associated with this arena or btt is used.
>
> Thanks for this suggestion! I will fix this in the v3 patch.
>
> > [snip]
> > > >
> > > > -static int btt_maplocks_init(struct arena_info *arena)
> > > > +static int btt_maplocks_init(struct device *dev, struct arena_info
> > > > *arena)
> > > > {
> > > > u32 i;
> > > >
> > > > - arena->map_locks = kcalloc(arena->nfree, sizeof(struct
> > > > aligned_lock),
> > > > + arena->map_locks = devm_kcalloc(dev, arena->nfree,
> > > > sizeof(struct aligned_lock),
> > > > GFP_KERNEL);
> > > > if (!arena->map_locks)
> > > > return -ENOMEM;
> > > > @@ -805,9 +805,6 @@ static void free_arenas(struct btt *btt)
> > > >
> > > > list_for_each_entry_safe(arena, next, >arena_list, list) {
> > > > list_del(>list);
> > > > - kfree(arena->rtt);
> > > > - kfree(arena->map_locks);
> > > > - kfree(arena->freelist);
> > >
> > > This does not quite work.
> > >
> > > free_arenas() is used in the error paths of create_arenas() and
> > > discover_arenas(). In those cases devm_kfree() is probably a better way
> > > to clean up this.
>
> Here I'm a little confused about when devm_kfree() should be used.
Over all it should be used whenever memory is allocated for the lifetime
of the device.
> Code in btt_init() implies that resources allocated by devm_* could be
> auto freed in both error and success paths of probe/attach (e.g., btt
> allocated by devm_kzalloc is never freed by devm_kfree).
> Using devm_kfree() in free_arenas() is ok for me, but I want to know
> whether not using devm_kfree() constitutes a bug.
Unfortunately I'm not familiar enough with this code to know for sure.
After my quick checks before I thought it was. But each time I look at it
I get confused. This is why I was thinking maybe not using devm_*() and
using no_free_ptr() may be a better solution to make sure things don't
leak without changing the success path (which is likely working fine
because no bugs have been found.)
>
> > >
> > > However...
> > >
> > > > debugfs_remove_recursive(arena->debugfs_dir);
> > > > kfree(arena);
> > >
> > > Why can't arena be allocated with devm_*?
> > >
> > > We need to change this up a bit more to handle the error path vs regular
> > > device shutdown free (automatic devm frees).
> >
>
> At first, I think the use of arena is correct. Therefore, allocating arena
> with devm_* should be a code style optimization. However, I rechecked
> discover_arenas and found arena might also be leaked (e.g., if
> alloc_arena() fails in the second loop, the previously allocated
> resources in the loop is leaked). The correct code could be found in
> create_arenas(), where free_arenas is called on failure of
> alloc_arena().
Yea I've not reached that level of detail in my analysis.
>
> To fix this issue, I think it's better to change the 'goto out_super'
> tag to 'goto out'. We could also use devm_* for arena to simplify the
> error path in discover_arenas().
I think it is your call at this point as I don't have time to dig in more
than I have. Sorry.
>
> > We might want to look at using no_free_ptr() in this code. See this
> > patch[1] for an example of how to inhibit the cleanup and pass the
> > pointer on when the function succeeds.
> >
> > [1]
> > https://lore.kernel.org/all/170261791914.1714654.6447680285357545638.st...@dwillia2-xfh.jf.intel.com/
> >
> > Ira
>
> Thanks for this example. But it seems that no_free_ptr() is used to
> handle the scope based resource management. Changes in this patch does
> not introduce this feature. Do I understand this correctly?
You do understand but I was thinking that perhaps using no_free_ptr()
rather than devm_*() might be an easier way to fix this bug without trying
to decode the lifetime of everything.
Ira
>
> Regards,
> Dinghao
Re: [PATCH] [v2] nvdimm-btt: fix several memleaks
Ira Weiny wrote:
> Dinghao Liu wrote:
[snip]
> >
> > -static int btt_maplocks_init(struct arena_info *arena)
> > +static int btt_maplocks_init(struct device *dev, struct arena_info *arena)
> > {
> > u32 i;
> >
> > - arena->map_locks = kcalloc(arena->nfree, sizeof(struct aligned_lock),
> > + arena->map_locks = devm_kcalloc(dev, arena->nfree, sizeof(struct
> > aligned_lock),
> > GFP_KERNEL);
> > if (!arena->map_locks)
> > return -ENOMEM;
> > @@ -805,9 +805,6 @@ static void free_arenas(struct btt *btt)
> >
> > list_for_each_entry_safe(arena, next, >arena_list, list) {
> > list_del(>list);
> > - kfree(arena->rtt);
> > - kfree(arena->map_locks);
> > - kfree(arena->freelist);
>
> This does not quite work.
>
> free_arenas() is used in the error paths of create_arenas() and
> discover_arenas(). In those cases devm_kfree() is probably a better way
> to clean up this.
>
> However...
>
> > debugfs_remove_recursive(arena->debugfs_dir);
> > kfree(arena);
>
> Why can't arena be allocated with devm_*?
>
> We need to change this up a bit more to handle the error path vs regular
> device shutdown free (automatic devm frees).
We might want to look at using no_free_ptr() in this code. See this
patch[1] for an example of how to inhibit the cleanup and pass the pointer
on when the function succeeds.
[1]
https://lore.kernel.org/all/170261791914.1714654.6447680285357545638.st...@dwillia2-xfh.jf.intel.com/
Ira
Re: [PATCH] [v2] nvdimm-btt: fix several memleaks
Dinghao Liu wrote:
> Resources allocated by kcalloc() in btt_freelist_init(),
> btt_rtt_init(), and btt_maplocks_init() are not correctly
> released in their callers when an error happens. For
> example, when an error happens in btt_freelist_init(), its
> caller discover_arenas() will directly free arena, which makes
> arena->freelist a leaked memory. Fix these memleaks by using
> devm_kcalloc() to make the memory auto-freed on driver detach.
Thanks for this work!
>
> Fixes: 5212e11fde4d ("nd_btt: atomic sector updates")
> Signed-off-by: Dinghao Liu
> ---
>
> Changelog:
>
> v2: -Use devm_kcalloc() to fix the memleaks.
> -Fix the potential leaked memory in btt_rtt_init()
> and btt_maplocks_init().
> ---
> drivers/nvdimm/btt.c | 35 ---
> 1 file changed, 16 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index d5593b0dc700..c55231f42617 100644
> --- a/drivers/nvdimm/btt.c
> +++ b/drivers/nvdimm/btt.c
> @@ -531,13 +531,13 @@ static int arena_clear_freelist_error(struct arena_info
> *arena, u32 lane)
> return ret;
> }
>
> -static int btt_freelist_init(struct arena_info *arena)
> +static int btt_freelist_init(struct device *dev, struct arena_info *arena)
Both struct arena_info and struct btt contain references to struct nd_btt
which is the device you are passing down this call stack.
Just use the device in the arena/btt rather than passing a device
structure. That makes the code easier to read and ensures that the device
associated with this arena or btt is used.
> {
> int new, ret;
> struct log_entry log_new;
> u32 i, map_entry, log_oldmap, log_newmap;
>
> - arena->freelist = kcalloc(arena->nfree, sizeof(struct free_entry),
> + arena->freelist = devm_kcalloc(dev, arena->nfree, sizeof(struct
> free_entry),
... devm_kcalloc(>nd_btt.dev, ...)
> GFP_KERNEL);
> if (!arena->freelist)
> return -ENOMEM;
> @@ -718,20 +718,20 @@ static int log_set_indices(struct arena_info *arena)
> return 0;
> }
>
> -static int btt_rtt_init(struct arena_info *arena)
> +static int btt_rtt_init(struct device *dev, struct arena_info *arena)
> {
> - arena->rtt = kcalloc(arena->nfree, sizeof(u32), GFP_KERNEL);
> + arena->rtt = devm_kcalloc(dev, arena->nfree, sizeof(u32), GFP_KERNEL);
> if (arena->rtt == NULL)
> return -ENOMEM;
>
> return 0;
> }
>
> -static int btt_maplocks_init(struct arena_info *arena)
> +static int btt_maplocks_init(struct device *dev, struct arena_info *arena)
> {
> u32 i;
>
> - arena->map_locks = kcalloc(arena->nfree, sizeof(struct aligned_lock),
> + arena->map_locks = devm_kcalloc(dev, arena->nfree, sizeof(struct
> aligned_lock),
> GFP_KERNEL);
> if (!arena->map_locks)
> return -ENOMEM;
> @@ -805,9 +805,6 @@ static void free_arenas(struct btt *btt)
>
> list_for_each_entry_safe(arena, next, >arena_list, list) {
> list_del(>list);
> - kfree(arena->rtt);
> - kfree(arena->map_locks);
> - kfree(arena->freelist);
This does not quite work.
free_arenas() is used in the error paths of create_arenas() and
discover_arenas(). In those cases devm_kfree() is probably a better way
to clean up this.
However...
> debugfs_remove_recursive(arena->debugfs_dir);
> kfree(arena);
Why can't arena be allocated with devm_*?
We need to change this up a bit more to handle the error path vs regular
device shutdown free (automatic devm frees).
Ira
Re: [PATCH] driver core: Add a guard() definition for the device_lock()
Dan Williams wrote:
> At present there are ~200 usages of device_lock() in the kernel. Some of
> those usages lead to "goto unlock;" patterns which have proven to be
> error prone. Define a "device" guard() definition to allow for those to
> be cleaned up and prevent new ones from appearing.
>
> Link:
> http://lore.kernel.org/r/657897453dda8_269bd29...@dwillia2-mobl3.amr.corp.intel.com.notmuch
> Link:
> http://lore.kernel.org/r/6577b0c2a02df_a04c529...@dwillia2-xfh.jf.intel.com.notmuch
> Cc: Vishal Verma
> Cc: Ira Weiny
Reviewed-by: Ira Weiny
> Cc: Peter Zijlstra
> Cc: Greg Kroah-Hartman
> Cc: Andrew Morton
> Signed-off-by: Dan Williams
> ---
> Hi Greg,
>
> I wonder if you might include this change in v6.7-rc to ease some patch
> sets alternately going through my tree and Andrew's tree. Those
> discussions are linked above. Alternately I can can just take it through
> my tree with your ack and the other use case can circle back to it in
> the v6.9 cycle.
>
> I considered also defining a __free() helper similar to __free(mutex),
> but I think "__free(device)" would be a surprising name for something
> that drops a lock. Also, I like the syntax of guard(device) over
> something like guard(device_lock) since a 'struct device *' is the
> argument, not a lock type, but I'm open to your or Peter's thoughts on
> the naming.
>
> include/linux/device.h |2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index d7a72a8749ea..6c83294395ac 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1007,6 +1007,8 @@ static inline void device_unlock(struct device *dev)
> mutex_unlock(>mutex);
> }
>
> +DEFINE_GUARD(device, struct device *, device_lock(_T), device_unlock(_T))
> +
> static inline void device_lock_assert(struct device *dev)
> {
> lockdep_assert_held(>mutex);
>
Re: [PATCH v4 2/3] dax/bus: Introduce guard(device) for device_{lock,unlock} flows
Vishal Verma wrote: > Introduce a guard(device) macro to lock a 'struct device', and unlock it > automatically when going out of scope using Scope Based Resource > Management semantics. A lot of the sysfs attribute writes in > drivers/dax/bus.c benefit from a cleanup using these, so change these > where applicable. > > Cc: Joao Martins > Suggested-by: Dan Williams > Signed-off-by: Vishal Verma > Reviewed-by: Ira Weiny
Re: [PATCH] nvdimm-btt: fix a potential memleak in btt_freelist_init
dinghao.liu@ wrote: > > Dave Jiang wrote: [snip] > > That said, this patch does not completely fix freelist from leaking in the > > following error path. > > > > discover_arenas() > > btt_freelist_init() -> ok (memory allocated) > > btt_rtt_init() -> fail > > goto out; > > (leak because arena is not yet on btt->arena_list) > > OR > > btt_maplocks_init() -> fail > > goto out; > > (leak because arena is not yet on btt->arena_list) > > > > Thanks for pointing out this issue! I rechecked discover_arenas() and found > that btt_rtt_init() may also trigger a memleak for the same reason as > btt_freelist_init(). Also, I checked another call trace: > > btt_init() -> btt_meta_init() -> btt_maplocks_init() > > I think there is a memleak if btt_maplocks_init() succeeds but an error > happens in btt_init() after btt_meta_init() (e.g., when btt_blk_init() > returns an error). Therefore, we may need to fix three functions. Yea I think we need to trace this code better. This is why devm_ is nice for memory allocated for the life of the device. > > > This error could be fixed by adding to arena_list earlier but devm_*() > > also takes care of this without having to worry about that logic. > > > > On normal operation all of this memory can be free'ed with the > > corresponding devm_kfree() and/or devm_add_action_*() calls if arenas come > > and go. I'm not sure off the top of my head. > > > > In addition, looking at this code. discover_arenas() could make use of > > the scoped based management for struct btt_sb *super! > > > > Dinghao would you be willing to submit a series of 2 or 3 patches to fix > > the above issues? > > > > Sure. Currently I plan to send 2 patches as follows: > 1. Using devm_kcalloc() to replace kcalloc() in btt_freelist_init(), >btt_rtt_init(), and btt_maplocks_init(), and removing the corresponding >kfree in free_arenas(). I checked some uses of devm_kcalloc() and it >seems that we need not to call devm_kfree(). The memory is automatically >freed on driver detach, right? On device put yes. So if these allocations are scoped to the life of the device there would be no reason to call devm_kfree() on them at all. I was not sure if they got reallocated at some point or not. > 2. Using the scoped based management for struct btt_sb *super (not a bug, >but it could improve the code). Good! > > I'm not quite sure whether my understanding or bug fixing plan is correct. > If there are any issues, please correct me, thanks! The above sounds right. Ira
Re: [PATCH] nvdimm-btt: fix a potential memleak in btt_freelist_init
Dave Jiang wrote:
>
[snip]
First off thanks for the patch. This code seems to have a few things to
clean up.
>
> On 12/6/23 20:43, Dinghao Liu wrote:
> > When an error happens in btt_freelist_init(), its caller
> > discover_arenas() will directly free arena, which makes
> > arena->freelist allocated in btt_freelist_init() a leaked
> > memory. Fix this by freeing arena->freelist in all error
> > handling paths of btt_freelist_init().
> >
> > Fixes: 5212e11fde4d ("nd_btt: atomic sector updates")
> > Signed-off-by: Dinghao Liu
>
> How about use the new scope based resource management and we can avoid the
> goto mess altogether?
> https://lwn.net/Articles/934679/
>
The freelist is returned as part of arena. I've not traced both paths of
btt_freelist_init() completely but devm_kcalloc() looks like a better
solution here because this memory needs to live past the function scope.
That said, this patch does not completely fix freelist from leaking in the
following error path.
discover_arenas()
btt_freelist_init() -> ok (memory allocated)
btt_rtt_init() -> fail
goto out;
(leak because arena is not yet on btt->arena_list)
OR
btt_maplocks_init() -> fail
goto out;
(leak because arena is not yet on btt->arena_list)
This error could be fixed by adding to arena_list earlier but devm_*()
also takes care of this without having to worry about that logic.
On normal operation all of this memory can be free'ed with the
corresponding devm_kfree() and/or devm_add_action_*() calls if arenas come
and go. I'm not sure off the top of my head.
In addition, looking at this code. discover_arenas() could make use of
the scoped based management for struct btt_sb *super!
Dinghao would you be willing to submit a series of 2 or 3 patches to fix
the above issues?
Thanks!
Ira
Re: [PATCH] tools/testing/nvdimm: Add compile-test coverage for ndtest
Dan Williams wrote:
> Greg lamented:
> "Ick, sorry about that, obviously this test isn't actually built by any
> bots :("
>
> A quick and dirty way to prevent this problem going forward is to always
> compile ndtest.ko whenever nfit_test is built. While this still does not
> expose the test code to any of the known build bots, it at least makes
> it the case that anyone that runs the x86 tests also compiles the
> powerpc test.
>
> I.e. the Intel NVDIMM maintainers are less likely to fall into this hole
> in the future.
>
> Link: http://lore.kernel.org/r/2023112729-aids-drainable-5744@gregkh
> Cc: Greg KH
> Cc: Yi Zhang
> Cc: Dave Jiang
> Cc: Ira Weiny
Reviewed-by: Ira Weiny
Re: [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
Verma, Vishal L wrote:
[snip]
> > > > +# Find a memory device to create regions on to test the destroy
> > > > +readarray -t mems < <("$CXL" list -b cxl_test -M | jq -r '.[].memdev')
> > > > +for mem in ${mems[@]}; do
> > > > + ramsize=$($CXL list -m $mem | jq -r '.[].ram_size')
> > > > + if [ "$ramsize" == "null" ]; then
> > > > + continue
> > > > + fi
> > > > + decoder=$($CXL list -b cxl_test -D -d root -m "$mem" |
> > > > + jq -r ".[] |
> > > > + select(.volatile_capable == true) |
> > > > + select(.nr_targets == 1) |
> > > > + select(.size >= ${ramsize}) |
> > > > + .decoder")
> > > > + if [[ $decoder ]]; then
> > > > + check_destroy_ram $mem $decoder
> > > > + check_destroy_devdax $mem $decoder
> > > > + break
> > > > + fi
> > > > +done
> > >
> > > Does this need to check results of the region disable & destroy?
> > >
> > > Did the regression this is after leave a trace in the dmesg log,
> > > so checking that is all that's needed?
> > >
> >
> > The regression causes
> >
> > check_destroy_devdax()
> > $CXL disable-region $region
> >
> > to fail. That command failure will exit with an error which causes the
> > test script to exit with that error as well.
> >
> > At least that is what happened when I used this to test the fix. I'll
> > defer to Vishal if there is a more explicit or better way to check for
> > that cxl-cli command to fail.
> >
> Correct, the set -e will cause the script to abort with an error exit
> code whenever a command fails.
>
> I do wonder if we need this new test - with Dave's patch here[1],
I'm not sure.
> destroy-region and disable-region both use the same helper that
> performs the libdaxctl checks.
>
> cxl-create-region.sh already has flows that create a region and then
> destroy it. Those should now cover this case as well yeah?
I thought it would have but I don't think it covers the case where the dax
device is not system ram (the default when creating a region).
Ira
Re: [PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
Alison Schofield wrote:
> On Thu, Nov 30, 2023 at 08:06:13PM -0800, Ira Weiny wrote:
[snip]
> > +
> > +check_destroy_devdax()
> > +{
> > + mem=$1
> > + decoder=$2
> > +
> > + region=$($CXL create-region -d "$decoder" -m "$mem" | jq -r ".region")
> > + if [ "$region" == "null" ]; then
> > + err "$LINENO"
> > + fi
> > + $CXL enable-region "$region"
> > +
> > + dax=$($CXL list -X -r "$region" | jq -r ".[].daxregion.devices" | jq -r
> > '.[].chardev')
> > +
> > + $DAXCTL reconfigure-device -m devdax "$dax"
> > +
> > + $CXL disable-region $region
> > + $CXL destroy-region $region
> > +}
> > +
> > +# Find a memory device to create regions on to test the destroy
> > +readarray -t mems < <("$CXL" list -b cxl_test -M | jq -r '.[].memdev')
> > +for mem in ${mems[@]}; do
> > +ramsize=$($CXL list -m $mem | jq -r '.[].ram_size')
> > +if [ "$ramsize" == "null" ]; then
> > +continue
> > +fi
> > +decoder=$($CXL list -b cxl_test -D -d root -m "$mem" |
> > + jq -r ".[] |
> > + select(.volatile_capable == true) |
> > + select(.nr_targets == 1) |
> > + select(.size >= ${ramsize}) |
> > + .decoder")
> > +if [[ $decoder ]]; then
> > + check_destroy_ram $mem $decoder
> > + check_destroy_devdax $mem $decoder
> > +break
> > +fi
> > +done
>
> Does this need to check results of the region disable & destroy?
>
> Did the regression this is after leave a trace in the dmesg log,
> so checking that is all that's needed?
>
The regression causes
check_destroy_devdax()
$CXL disable-region $region
to fail. That command failure will exit with an error which causes the
test script to exit with that error as well.
At least that is what happened when I used this to test the fix. I'll
defer to Vishal if there is a more explicit or better way to check for
that cxl-cli command to fail.
Ira
RE: [PATCH v2] daxctl: Remove unused memory_zone and mem_zone
Xiao Yang (Fujitsu) wrote: > Ping. ^_^ > > -Original Message- > From: Xiao Yang > Sent: 2023年8月11日 9:16 > To: vishal.l.ve...@intel.com; fan...@gmx.us; nvdimm@lists.linux.dev > Cc: linux-...@vger.kernel.org; Yang, Xiao/杨 晓 > Subject: [PATCH v2] daxctl: Remove unused memory_zone and mem_zone > > The enum memory_zone definition and mem_zone variable have never been used so > remove them. NIT: I don't know that they have never been used but they certainly seem to have been moved to the library. > > Signed-off-by: Xiao Yang Reviewed-by: Ira Weiny
[PATCH ndctl RESEND 2/2] cxl/region: Fix memory device teardown in disable-region
When a region is requested to be disabled, memory devices are normally
automatically torn down. Commit 9399aa667ab0 prevents tear down if
memory is online without a force flag. However, daxctl_dev_get_memory()
may return NULL if the memory device in question is not system-ram
capable as is the case for a region with only devdax devices. Such
devices do not need to be off-lined explicitly.
Skip non-system-ram devices rather than error the operation.
Fixes: 9399aa667ab0 ("cxl/region: Add -f option for disable-region")
Cc: Dave Jiang
Signed-off-by: Ira Weiny
---
cxl/region.c | 3 +++
daxctl/lib/libdaxctl.c | 4 ++--
daxctl/lib/libdaxctl.sym | 5 +
daxctl/libdaxctl.h | 1 +
4 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/cxl/region.c b/cxl/region.c
index 5cbbf2749e2d..44ac76b001e9 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -805,6 +805,9 @@ static int disable_region(struct cxl_region *region)
goto out;
daxctl_dev_foreach(dax_region, dev) {
+ if (!daxctl_dev_is_system_ram_capable(dev))
+ continue;
+
mem = daxctl_dev_get_memory(dev);
if (!mem)
return -ENXIO;
diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
index 4f9aba0b09f2..9fbefe2e8329 100644
--- a/daxctl/lib/libdaxctl.c
+++ b/daxctl/lib/libdaxctl.c
@@ -385,7 +385,7 @@ static bool device_model_is_dax_bus(struct daxctl_dev *dev)
return false;
}
-static int dev_is_system_ram_capable(struct daxctl_dev *dev)
+DAXCTL_EXPORT int daxctl_dev_is_system_ram_capable(struct daxctl_dev *dev)
{
const char *devname = daxctl_dev_get_devname(dev);
struct daxctl_ctx *ctx = daxctl_dev_get_ctx(dev);
@@ -432,7 +432,7 @@ static struct daxctl_memory *daxctl_dev_alloc_mem(struct
daxctl_dev *dev)
char buf[SYSFS_ATTR_SIZE];
int node_num;
- if (!dev_is_system_ram_capable(dev))
+ if (!daxctl_dev_is_system_ram_capable(dev))
return NULL;
mem = calloc(1, sizeof(*mem));
diff --git a/daxctl/lib/libdaxctl.sym b/daxctl/lib/libdaxctl.sym
index fe68fd0a9cde..309881196c86 100644
--- a/daxctl/lib/libdaxctl.sym
+++ b/daxctl/lib/libdaxctl.sym
@@ -99,3 +99,8 @@ global:
daxctl_set_config_path;
daxctl_get_config_path;
} LIBDAXCTL_8;
+
+LIBDAXCTL_10 {
+global:
+ daxctl_dev_is_system_ram_capable;
+} LIBDAXCTL_9;
diff --git a/daxctl/libdaxctl.h b/daxctl/libdaxctl.h
index 6876037a9427..53c6bbdae5c3 100644
--- a/daxctl/libdaxctl.h
+++ b/daxctl/libdaxctl.h
@@ -77,6 +77,7 @@ int daxctl_dev_will_auto_online_memory(struct daxctl_dev
*dev);
int daxctl_dev_has_online_memory(struct daxctl_dev *dev);
struct daxctl_memory;
+int daxctl_dev_is_system_ram_capable(struct daxctl_dev *dev);
struct daxctl_memory *daxctl_dev_get_memory(struct daxctl_dev *dev);
struct daxctl_dev *daxctl_memory_get_dev(struct daxctl_memory *mem);
const char *daxctl_memory_get_node_path(struct daxctl_memory *mem);
--
2.42.0
[PATCH ndctl RESEND 1/2] ndctl/test: Add destroy region test
Commit 9399aa667ab0 ("cxl/region: Add -f option for disable-region")
introduced a regression when destroying a region.
Add a tests for destroying a region.
Cc: Dave Jiang
Signed-off-by: Ira Weiny
---
test/cxl-destroy-region.sh | 76 ++
test/meson.build | 2 ++
2 files changed, 78 insertions(+)
diff --git a/test/cxl-destroy-region.sh b/test/cxl-destroy-region.sh
new file mode 100644
index ..251720a98688
--- /dev/null
+++ b/test/cxl-destroy-region.sh
@@ -0,0 +1,76 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2023 Intel Corporation. All rights reserved.
+
+. $(dirname $0)/common
+
+rc=77
+
+set -ex
+
+trap 'err $LINENO' ERR
+
+check_prereq "jq"
+
+modprobe -r cxl_test
+modprobe cxl_test
+rc=1
+
+check_destroy_ram()
+{
+ mem=$1
+ decoder=$2
+
+ region=$($CXL create-region -d "$decoder" -m "$mem" | jq -r ".region")
+ if [ "$region" == "null" ]; then
+ err "$LINENO"
+ fi
+ $CXL enable-region "$region"
+
+ # default is memory is system-ram offline
+ $CXL disable-region $region
+ $CXL destroy-region $region
+}
+
+check_destroy_devdax()
+{
+ mem=$1
+ decoder=$2
+
+ region=$($CXL create-region -d "$decoder" -m "$mem" | jq -r ".region")
+ if [ "$region" == "null" ]; then
+ err "$LINENO"
+ fi
+ $CXL enable-region "$region"
+
+ dax=$($CXL list -X -r "$region" | jq -r ".[].daxregion.devices" | jq -r
'.[].chardev')
+
+ $DAXCTL reconfigure-device -m devdax "$dax"
+
+ $CXL disable-region $region
+ $CXL destroy-region $region
+}
+
+# Find a memory device to create regions on to test the destroy
+readarray -t mems < <("$CXL" list -b cxl_test -M | jq -r '.[].memdev')
+for mem in ${mems[@]}; do
+ramsize=$($CXL list -m $mem | jq -r '.[].ram_size')
+if [ "$ramsize" == "null" ]; then
+continue
+fi
+decoder=$($CXL list -b cxl_test -D -d root -m "$mem" |
+ jq -r ".[] |
+ select(.volatile_capable == true) |
+ select(.nr_targets == 1) |
+ select(.size >= ${ramsize}) |
+ .decoder")
+if [[ $decoder ]]; then
+ check_destroy_ram $mem $decoder
+ check_destroy_devdax $mem $decoder
+break
+fi
+done
+
+check_dmesg "$LINENO"
+
+modprobe -r cxl_test
diff --git a/test/meson.build b/test/meson.build
index 2706fa5d633c..126d663dfce2 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -158,6 +158,7 @@ cxl_xor_region = find_program('cxl-xor-region.sh')
cxl_update_firmware = find_program('cxl-update-firmware.sh')
cxl_events = find_program('cxl-events.sh')
cxl_poison = find_program('cxl-poison.sh')
+cxl_destroy_region = find_program('cxl-destroy-region.sh')
tests = [
[ 'libndctl', libndctl, 'ndctl' ],
@@ -188,6 +189,7 @@ tests = [
[ 'cxl-xor-region.sh', cxl_xor_region, 'cxl' ],
[ 'cxl-events.sh', cxl_events, 'cxl' ],
[ 'cxl-poison.sh', cxl_poison, 'cxl' ],
+ [ 'cxl-destroy-region.sh', cxl_destroy_region, 'cxl' ],
]
if get_option('destructive').enabled()
--
2.42.0
[PATCH ndctl RESEND 0/2] ndctl: Fix ups for region destroy
The patch to force device tear down on region disable caused a regression for regions without system ram. Add a test for such a case and a fix to the patch. Signed-off-by: Ira Weiny --- [djiang: RESEND with proper mailing lists.] --- Ira Weiny (2): ndctl/test: Add destroy region test cxl/region: Fix memory device teardown in disable-region cxl/region.c | 3 ++ daxctl/lib/libdaxctl.c | 4 +-- daxctl/lib/libdaxctl.sym | 5 +++ daxctl/libdaxctl.h | 1 + test/cxl-destroy-region.sh | 76 ++ test/meson.build | 2 ++ 6 files changed, 89 insertions(+), 2 deletions(-) --- base-commit: cbf049039482a56c2b66ede3e10d5e9c652890b7 change-id: 20231130-fix-region-destroy-a85ad1fde87b Best regards, -- Ira Weiny
Re: [TLS] Call to Move RFC 8773 from Experimental to Standards Track
Hi, Approve. Cheers, - Ira On Wed, Nov 29, 2023 at 10:51 AM Joseph Salowey wrote: > RFC 8773 (TLS 1.3 Extension for Certificate-Based Authentication with an > External Pre-Shared Key) was originally published as experimental due to > lack of implementations. As part of implementation work for the EMU > workitem draft-ietf-emu-bootstrapped-tls which uses RFC 8773 there is > ongoing implementation work. Since the implementation status of RFC 8773 is > changing, this is a consensus call to move RFC 8773 to standards track as > reflected in [RFC8773bis]( > https://datatracker.ietf.org/doc/draft-ietf-tls-8773bis). This will also > help avoid downref for the EMU draft. Please indicate if you approve of or > object to this transition to standards track status by December 15, 2023. > > Thanks, > > Joe, Sean, and Deirdre > ___ > TLS mailing list > TLS@ietf.org > https://www.ietf.org/mailman/listinfo/tls > ___ TLS mailing list TLS@ietf.org https://www.ietf.org/mailman/listinfo/tls
Re: [PATCH] ndtest: fix typo class_regster -> class_register
Greg KH wrote: > On Mon, Nov 27, 2023 at 10:00:14AM -0700, Dave Jiang wrote: > > > > [snip] > Ick, sorry about that, obviously this test isn't actually built by any > bots :( Indeed. I'm looking through the tests I ran now. > > I'll go queue this up now, Thanks, Ira
Re: [PATCH v3 0/9] Enabling DCD emulation support in Qemu
nifan.cxl@ wrote: > From: Fan Ni > > > The patch series are based on Jonathan's branch cxl-2023-09-26. Finally getting around to trying this new series and the patch series does not seem to apply on top of this branch? Just to verify is this the top commit this work was based on? d4edf131bbac [jonathan/cxl-2023-09-26] cxl/vendor: SK hynix Niagara Multi-Headed SLD Device I seem to have found some issue with CDAT checksumming[1] which I'm not quite sure about. I went ahead and pulled your latest work from: https://github.com/moking/qemu-jic-clone.git dcd-dev abe893944bb3 hw/mem/cxl_type3: Add dpa range validation for accesses to dc regions It still has this same problem. Before I dig into this, is this the latest dcd branch? Has anything changed in how you specify DCD devices on the qemu command line with this latest work? Here is what I have: ... -device cxl-type3,bus=hb0rp0,memdev=cxl-mem0,num-dc-regions=2,nonvolatile-dc-memdev=cxl-dc-mem0,id=cxl-dev0,lsa=cxl-lsa0,sn=0 -device cxl-type3,bus=hb0rp1,memdev=cxl-mem1,num-dc-regions=2,nonvolatile-dc-memdev=cxl-dc-mem1,id=cxl-dev1,lsa=cxl-lsa1,sn=1 -device cxl-type3,bus=hb1rp0,memdev=cxl-mem2,num-dc-regions=2,nonvolatile-dc-memdev=cxl-dc-mem2,id=cxl-dev2,lsa=cxl-lsa2,sn=2 -device cxl-type3,bus=hb1rp1,memdev=cxl-mem3,num-dc-regions=2,nonvolatile-dc-memdev=cxl-dc-mem3,id=cxl-dev3,lsa=cxl-lsa3,sn=3 ... Ira [1] https://lore.kernel.org/all/20231116-fix-cdat-devm-free-v1-1-b148b4070...@intel.com/ > The main changes include, > 1. Update cxl_find_dc_region to detect the case the range of the extent cross > multiple DC regions. > 2. Add comments to explain the checks performed in function > cxl_detect_malformed_extent_list. (Jonathan) > 3. Minimize the checks in cmd_dcd_add_dyn_cap_rsp.(Jonathan) > 4. Update total_extent_count in add/release dynamic capacity response > function. > (Ira and Jorgen Hansen). > 5. Fix the logic issue in test_bits and renamed it to > test_any_bits_set to clear its function. > 6. Add pending extent list for dc extent add event. > 7. When add extent response is received, use the pending-to-add list to > verify the extents are valid. > 8. Add test_any_bits_set and cxl_insert_extent_to_extent_list declaration to > cxl_device.h so it can be used in different files. > 9. Updated ct3d_qmp_cxl_event_log_enc to include dynamic capacity event > log type. > 10. Extract the functionality to delete extent from extent list to a helper > function. > 11. Move the update of the bitmap which reflects which blocks are backed with > dc extents from the moment when a dc extent is offered to the moment when it > is accepted from the host. > 12. Free dc_name after calling address_space_init to avoid memory leak when > returning early. (Nathan) > 13. Add code to detect and reject QMP requests without any extents. (Jonathan) > 14. Add code to detect and reject QMP requests where the extent len is 0. > 15. Change the QMP interface and move the region-id out of extents and now > each command only takes care of extent add/release request in a single > region. (Jonathan) > 16. Change the region bitmap length from decode_len to len. > 17. Rename "dpa" to "offset" in the add/release dc extent qmp interface. > (Jonathan) > 18. Block any dc extent release command if the exact extent is not already in > the extent list of the device. > > The code is tested together with Ira's kernel DCD support: > https://github.com/weiny2/linux-kernel/tree/dcd-v3-2023-10-30 > > Cover letter from v2 is here: > https://lore.kernel.org/linux-cxl/20230724162313.34196-1-fan...@samsung.com/T/#m63039621087023691c9749a0af1212deb5549ddf > > Last version (v2) is here: > https://lore.kernel.org/linux-cxl/20230725183939.2741025-1-fan...@samsung.com/ > > More DCD related discussions are here: > https://lore.kernel.org/linux-cxl/650cc29ab3f64_50d07294e7@iweiny-mobl.notmuch/ > > > > Fan Ni (9): > hw/cxl/cxl-mailbox-utils: Add dc_event_log_size field to output > payload of identify memory device command > hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative > and mailbox command support > include/hw/cxl/cxl_device: Rename mem_size as static_mem_size for > type3 memory devices > hw/mem/cxl_type3: Add support to create DC regions to type3 memory > devices > hw/mem/cxl_type3: Add host backend and address space handling for DC > regions > hw/mem/cxl_type3: Add DC extent list representative and get DC extent > list mailbox support > hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release > dynamic capacity response > hw/cxl/events: Add qmp interfaces to add/release dynamic capacity > extents > hw/mem
[GIT PULL] NVDIMM for 6.7
Hi Linus, please pull from: git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git tags/libnvdimm-for-6.7 ... to get updates to the nvdimm tree. They are a mix of bug fixes and updates to interfaces used by nvdimm. Bug fixes include: Fix a sleep during spinlock in PREEMPT_RT kernels when getting a BTT lane Check for kstrdup memory failure in region probe Use the correct variables in the nvdimm_badblocks_populate() kdoc block Updates to interfaces include: Use devm_kstrdup to manage memory better Allow class structures to be declared at build time Start using __counted_by to help with bounds checking Remove use of the deprecated strncpy They have all been in -next for more than a week with no reported issues. --- The following changes since commit 6465e260f48790807eef06b583b38ca9789b6072: Linux 6.6-rc3 (2023-09-24 14:31:13 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git tags/libnvdimm-for-6.7 for you to fetch changes up to 9ea459e477dc09370cdd8ee13b61aefe8cd1f20a: libnvdimm: remove kernel-doc warnings: (2023-10-18 09:48:05 -0700) libnvdimm updates for v6.7 - updates to deprecated and changed interfaces - bug/kdoc fixes Chen Ni (1): libnvdimm/of_pmem: Use devm_kstrdup instead of kstrdup and check its return value Greg Kroah-Hartman (1): testing: nvdimm: make struct class structures constant Justin Stitt (1): dax: refactor deprecated strncpy Kees Cook (1): libnvdimm: Annotate struct nd_region with __counted_by Tomas Glozar (1): nd_btt: Make BTT lanes preemptible Zhu Wang (1): libnvdimm: remove kernel-doc warnings: drivers/dax/bus.c | 2 +- drivers/nvdimm/badrange.c | 4 ++-- drivers/nvdimm/nd.h| 2 +- drivers/nvdimm/of_pmem.c | 8 +++- drivers/nvdimm/region_devs.c | 10 +- tools/testing/nvdimm/test/ndtest.c | 17 + tools/testing/nvdimm/test/nfit.c | 14 +++--- 7 files changed, 32 insertions(+), 25 deletions(-)
Re: [PATCH] Install Notify() handler before getting NFIT table
Rafael J. Wysocki wrote:
> On Thu, Oct 19, 2023 at 2:57 PM chenxiang wrote:
> >
> > From: Xiang Chen
> >
> > If there is no NFIT at startup, it will return 0 immediately in function
> > acpi_nfit_add() and will not install Notify() handler. If hotplugging
> > a nvdimm device later, it will not be identified as there is no Notify()
> > handler.
>
> Yes, this is a change in behavior that shouldn't have been made.
>
> > So move handler installing before getting NFI table in function
> > acpi_nfit_add() to avoid above issue.
>
> And the fix is correct if I'm not mistaken.
>
> I can still queue it up for 6.6 if that's fine with everyone. Dan?
That is fine with me. Vishal, Dave Jiang, and I are wrangling the nvdimm
tree these days. I've prepared 6.7 already so I'll ignore this.
Ira
>
> > Fixes: dcca12ab62a2 ("ACPI: NFIT: Install Notify() handler directly")
> > Signed-off-by: Xiang Chen
> > ---
> > drivers/acpi/nfit/core.c | 22 +++---
> > 1 file changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> > index 3826f49..9923855 100644
> > --- a/drivers/acpi/nfit/core.c
> > +++ b/drivers/acpi/nfit/core.c
> > @@ -3339,6 +3339,16 @@ static int acpi_nfit_add(struct acpi_device *adev)
> > acpi_size sz;
> > int rc = 0;
> >
> > + rc = acpi_dev_install_notify_handler(adev, ACPI_DEVICE_NOTIFY,
> > +acpi_nfit_notify, adev);
> > + if (rc)
> > + return rc;
> > +
> > + rc = devm_add_action_or_reset(dev, acpi_nfit_remove_notify_handler,
> > + adev);
> > + if (rc)
> > + return rc;
> > +
> > status = acpi_get_table(ACPI_SIG_NFIT, 0, );
> > if (ACPI_FAILURE(status)) {
> > /* The NVDIMM root device allows OS to trigger enumeration
> > of
> > @@ -3386,17 +3396,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
> > if (rc)
> > return rc;
> >
> > - rc = devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
> > - if (rc)
> > - return rc;
> > -
> > - rc = acpi_dev_install_notify_handler(adev, ACPI_DEVICE_NOTIFY,
> > -acpi_nfit_notify, adev);
> > - if (rc)
> > - return rc;
> > -
> > - return devm_add_action_or_reset(dev,
> > acpi_nfit_remove_notify_handler,
> > - adev);
> > + return devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
> > }
> >
> > static void acpi_nfit_update_notify(struct device *dev, acpi_handle handle)
> > --
>
Re: [PATCH] testing: nvdimm: make struct class structures constant
Greg Kroah-Hartman wrote: > Now that the driver core allows for struct class to be in read-only > memory, we should make all 'class' structures declared at build time > placing them into read-only memory, instead of having to be dynamically > allocated at runtime. > > Cc: Dan Williams > Cc: Vishal Verma > Cc: Dave Jiang > Cc: Ira Weiny Tested-by: Ira Weiny Reviewed-by: Ira Weiny > Signed-off-by: Greg Kroah-Hartman > --- > tools/testing/nvdimm/test/ndtest.c | 17 + > tools/testing/nvdimm/test/nfit.c | 14 +++--- > 2 files changed, 16 insertions(+), 15 deletions(-)
[PATCH ndctl] ndctl/cxl/region: Report max size for region creation
When creating a region if the size exceeds the max an error is printed.
However, the max available space is not reported which makes it harder
to determine what is wrong.
Add the max size available to the output error.
Signed-off-by: Ira Weiny
---
cxl/region.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/cxl/region.c b/cxl/region.c
index bcd703956207..cb6a547990fb 100644
--- a/cxl/region.c
+++ b/cxl/region.c
@@ -623,8 +623,8 @@ static int create_region(struct cxl_ctx *ctx, int *count,
}
if (!default_size && size > max_extent) {
log_err(,
- "%s: region size %#lx exceeds max available space\n",
- cxl_decoder_get_devname(p->root_decoder), size);
+ "%s: region size %#lx exceeds max available space
(%#lx)\n",
+ cxl_decoder_get_devname(p->root_decoder), size,
max_extent);
return -ENOSPC;
}
---
base-commit: a871e6153b11fe63780b37cdcb1eb347b296095c
change-id: 20230926-max-size-create-region-1f57ff3bc53c
Best regards,
--
Ira Weiny
[PATCH ndctl RESEND] test/cxl-event: Skip cxl event testing if cxl-test is not available
CXL event testing is only appropriate when the cxl-test modules are available. Return error 77 (skip) if cxl-test modules are not available. Reported-by: Dave Jiang Signed-off-by: Ira Weiny --- Changes for resend: - iweiny: properly cc the mailing lists --- test/cxl-events.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/cxl-events.sh b/test/cxl-events.sh index 33b68daa6ade..fe702bf98ad4 100644 --- a/test/cxl-events.sh +++ b/test/cxl-events.sh @@ -10,6 +10,8 @@ num_fatal_expected=2 num_failure_expected=16 num_info_expected=3 +rc=77 + set -ex trap 'err $LINENO' ERR @@ -18,6 +20,7 @@ check_prereq "jq" modprobe -r cxl_test modprobe cxl_test +rc=1 dev_path="/sys/bus/platform/devices" --- base-commit: a871e6153b11fe63780b37cdcb1eb347b296095c change-id: 20230925-skip-cxl-events-7f16052b9c4e Best regards, -- Ira Weiny
Re: [PATCH v2] nd_btt: Make BTT lanes preemptible
Tomas Glozar wrote:
> nd_region_acquire_lane uses get_cpu, which disables preemption. This is
> an issue on PREEMPT_RT kernels, since btt_write_pg and also
> nd_region_acquire_lane itself take a spin lock, resulting in BUG:
> sleeping function called from invalid context.
>
> Fix the issue by replacing get_cpu with smp_process_id and
> migrate_disable when needed. This makes BTT operations preemptible, thus
> permitting the use of spin_lock.
>
> BUG example occurring when running ndctl tests on PREEMPT_RT kernel:
>
> BUG: sleeping function called from invalid context at
> kernel/locking/spinlock_rt.c:48
> in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 4903, name:
> libndctl
> preempt_count: 1, expected: 0
> RCU nest depth: 0, expected: 0
> Preemption disabled at:
> [] nd_region_acquire_lane+0x15/0x90 [libnvdimm]
> Call Trace:
>
> dump_stack_lvl+0x8e/0xb0
> __might_resched+0x19b/0x250
> rt_spin_lock+0x4c/0x100
> ? btt_write_pg+0x2d7/0x500 [nd_btt]
> btt_write_pg+0x2d7/0x500 [nd_btt]
> ? local_clock_noinstr+0x9/0xc0
> btt_submit_bio+0x16d/0x270 [nd_btt]
> __submit_bio+0x48/0x80
> __submit_bio_noacct+0x7e/0x1e0
> submit_bio_wait+0x58/0xb0
> __blkdev_direct_IO_simple+0x107/0x240
> ? inode_set_ctime_current+0x51/0x110
> ? __pfx_submit_bio_wait_endio+0x10/0x10
> blkdev_write_iter+0x1d8/0x290
> vfs_write+0x237/0x330
> ...
>
>
> Fixes: 5212e11fde4d ("nd_btt: atomic sector updates")
> Signed-off-by: Tomas Glozar
Thanks for the clarification.
Reviewed-by: Ira Weiny
Re: [PATCH] nd_btt: Make BTT lanes preemptible
Tomas Glozar wrote:
> čt 14. 9. 2023 v 22:18 odesílatel Ira Weiny napsal:
> > Is the bug in 1 of 2 places?
> >
> > 1) When btt_write_pg()->lock_map() (when the number of lanes is < number
> >of cpus) and the lane is acquired is called?
> >
> > *or*
> >
> > 2) When nd_region_acquire_lane() internally trys to take it's lock?
> >
> > A copy/paste of the BUG observed would have been more clear I think.
> >
>
> The BUG was observed on btt_write_pg()->lock_map(), but I assume the
> BUG will also happen on the lock in nd_region_acquire_lane, since that
> is also a spin lock, i.e. a sleeping lock on RT.
>
> BUG observed in dmesg when running ndctl tests on RT kernel without the patch:
Thanks for clarifying. Could you respin the patch with the text below?
That would have saved me a lot of time digging to see what the code path
was.
...
BUG: sleeping function called from invalid context at
kernel/locking/spinlock_rt.c:48
in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 4903, name:
libndctl
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
1 lock held by libndctl/4903:
#0: 8c184a270060 (>map_locks[i].lock){+.+.}-{2:2}, at:
btt_write_pg+0x2d7/0x500 [nd_btt]
Preemption disabled at:
[] nd_region_acquire_lane+0x15/0x90 [libnvdimm]
Call Trace:
dump_stack_lvl+0x8e/0xb0
__might_resched+0x19b/0x250
rt_spin_lock+0x4c/0x100
? btt_write_pg+0x2d7/0x500 [nd_btt]
btt_write_pg+0x2d7/0x500 [nd_btt]
? local_clock_noinstr+0x9/0xc0
btt_submit_bio+0x16d/0x270 [nd_btt]
__submit_bio+0x48/0x80
__submit_bio_noacct+0x7e/0x1e0
submit_bio_wait+0x58/0xb0
__blkdev_direct_IO_simple+0x107/0x240
? inode_set_ctime_current+0x51/0x110
? __pfx_submit_bio_wait_endio+0x10/0x10
blkdev_write_iter+0x1d8/0x290
vfs_write+0x237/0x330
...
With a respin including this trace:
Reviewed-by: Ira Weiny
Re: [PATCH 2/3] hw/mem/cxl_type3: Add missing copyright and license notice
Jonathan Cameron wrote: > On Mon, 18 Sep 2023 17:31:38 +0100 > Peter Maydell wrote: > > > On Mon, 18 Sept 2023 at 16:04, Jonathan Cameron > > wrote: > > > > > > This has been missing from the start. Assume it should match > > > with cxl/cxl-component-utils.c as both were part of early > > > postings from Ben. > > > > Sounds plausible -- is there an Intel person who could give us > > an acked-by for this? While we are at it; what about .../hw/mem/cxl_type3_stubs.c? > > > > (Ideally we wouldn't have let more gpl-2-only code into the > > codebase without a rationale...) I'm curious about this statement. Does the qemu project not want gpl v2 only code? I agree with Jonathan that this is the intention of Ben's initial submission; so from that PoV. Acked-by: Ira Weiny Going forward I'd like to better understand the qemu communities view. Thanks, Ira > > > > I've +CC'd the kernel CXL maintainers from Intel a few of whom > have also contributed some of the QEMU CXL code. > Hopefully someone can ack. > > > > Suggested-by: Philippe Mathieu-Daudé > > > Signed-off-by: Jonathan Cameron > > > --- > > > hw/mem/cxl_type3.c | 9 + > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > > > index c5855d4e7d..ad3f0f6a9d 100644 > > > --- a/hw/mem/cxl_type3.c > > > +++ b/hw/mem/cxl_type3.c > > > @@ -1,3 +1,12 @@ > > > +/* > > > + * CXL Type 3 (memory expander) device > > > + * > > > + * Copyright(C) 2020 Intel Corporation. > > > + * > > > + * This work is licensed under the terms of the GNU GPL, version 2. See > > > the > > > + * COPYING file in the top-level directory. > > > + */ > > > + > > > #include "qemu/osdep.h" > > > #include "qemu/units.h" > > > #include "qemu/error-report.h" > > > > -- PMM > > >
Re: [PATCH v3] libnvdimm/of_pmem: Use devm_kstrdup instead of kstrdup and check its return value
Chen Ni wrote:
> Use devm_kstrdup() instead of kstrdup() and check its return value to
> avoid memory leak.
>
> Fixes: 49bddc73d15c ("libnvdimm/of_pmem: Provide a unique name for bus
> provider")
> Signed-off-by: Chen Ni
Reviewed-by: Ira Weiny
> ---
> Changelog:
>
> v2 -> v3:
>
> 1. Use devm_kstrdup() instead of kstrdup()
>
> v1 -> v2:
>
> 1. Add a fixes tag.
> 2. Update commit message.
> ---
> drivers/nvdimm/of_pmem.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> index 1b9f5b8a6167..5765674b36f2 100644
> --- a/drivers/nvdimm/of_pmem.c
> +++ b/drivers/nvdimm/of_pmem.c
> @@ -30,7 +30,13 @@ static int of_pmem_region_probe(struct platform_device
> *pdev)
> if (!priv)
> return -ENOMEM;
>
> - priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
> + priv->bus_desc.provider_name = devm_kstrdup(>dev, pdev->name,
> + GFP_KERNEL);
> + if (!priv->bus_desc.provider_name) {
> + kfree(priv);
> + return -ENOMEM;
> + }
> +
> priv->bus_desc.module = THIS_MODULE;
> priv->bus_desc.of_node = np;
>
> --
> 2.25.1
>
Re: [PATCH v2 2/2] ACPI: NFIT: use struct_size() helper
Yu Liao wrote:
> Make use of the struct_size() helper instead of an open-coded version,
> in order to avoid any potential type mistakes or integer overflows that,
> in the worst scenario, could lead to heap overflows.
>
> Signed-off-by: Yu Liao
> Reviewed-by: Dave Jiang
Reviewed-by: Ira Weiny
> ---
> drivers/acpi/nfit/core.c | 10 --
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 305f590c54a8..2f7217600307 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -712,8 +712,7 @@ static bool add_spa(struct acpi_nfit_desc *acpi_desc,
> }
> }
>
> - nfit_spa = devm_kzalloc(dev, sizeof(*nfit_spa) + sizeof_spa(spa),
> - GFP_KERNEL);
> + nfit_spa = devm_kzalloc(dev, struct_size(nfit_spa, spa, 1), GFP_KERNEL);
> if (!nfit_spa)
> return false;
> INIT_LIST_HEAD(_spa->list);
> @@ -741,7 +740,7 @@ static bool add_memdev(struct acpi_nfit_desc *acpi_desc,
> return true;
> }
>
> - nfit_memdev = devm_kzalloc(dev, sizeof(*nfit_memdev) + sizeof(*memdev),
> + nfit_memdev = devm_kzalloc(dev, struct_size(nfit_memdev, memdev, 1),
> GFP_KERNEL);
> if (!nfit_memdev)
> return false;
> @@ -812,8 +811,7 @@ static bool add_dcr(struct acpi_nfit_desc *acpi_desc,
> return true;
> }
>
> - nfit_dcr = devm_kzalloc(dev, sizeof(*nfit_dcr) + sizeof(*dcr),
> - GFP_KERNEL);
> + nfit_dcr = devm_kzalloc(dev, struct_size(nfit_dcr, dcr, 1), GFP_KERNEL);
> if (!nfit_dcr)
> return false;
> INIT_LIST_HEAD(_dcr->list);
> @@ -855,7 +853,7 @@ static size_t sizeof_idt(struct acpi_nfit_interleave *idt)
> {
> if (idt->header.length < sizeof(*idt))
> return 0;
> - return sizeof(*idt) + sizeof(u32) * idt->line_count;
> + return struct_size(idt, line_offset, idt->line_count);
> }
>
> static bool add_idt(struct acpi_nfit_desc *acpi_desc,
> --
> 2.25.1
>
Re: [PATCH v2 1/2] ACPI: NFIT: Fix incorrect calculation of idt size
Yu Liao wrote:
> acpi_nfit_interleave's field 'line_offset' is switched to flexible array [1],
> but sizeof_idt() still calculates the size in the form of 1-element array.
>
> Therefore, fix incorrect calculation in sizeof_idt().
>
> [1] https://lore.kernel.org/lkml/2652195.BddDVKsqQX@kreacher/
>
> Fixes: 2a5ab99847bd ("ACPICA: struct acpi_nfit_interleave: Replace 1-element
> array with flexible array")
> Cc: sta...@vger.kernel.org # v6.4+
> Signed-off-by: Yu Liao
> Reviewed-by: Dave Jiang
Reviewed-by: Ira Weiny
> ---
> v1 -> v2: add Dave's review tag and cc nvd...@lists.linux.dev
> ---
> drivers/acpi/nfit/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 07204d482968..305f590c54a8 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -855,7 +855,7 @@ static size_t sizeof_idt(struct acpi_nfit_interleave *idt)
> {
> if (idt->header.length < sizeof(*idt))
> return 0;
> - return sizeof(*idt) + sizeof(u32) * (idt->line_count - 1);
> + return sizeof(*idt) + sizeof(u32) * idt->line_count;
> }
>
> static bool add_idt(struct acpi_nfit_desc *acpi_desc,
> --
> 2.25.1
>
Re: [PATCH] nd_btt: Make BTT lanes preemptible
Tomáš Glozar wrote:
> From: Tomas Glozar
>
> nd_region_acquire_lane uses get_cpu, which disables preemption. This is
> an issue on PREEMPT_RT kernels, since btt_write_pg and also
> nd_region_acquire_lane itself take a spin lock, resulting in BUG:
> sleeping function called from invalid context.
Is the bug in 1 of 2 places?
1) When btt_write_pg()->lock_map() (when the number of lanes is < number
of cpus) and the lane is acquired is called?
*or*
2) When nd_region_acquire_lane() internally trys to take it's lock?
A copy/paste of the BUG observed would have been more clear I think.
Regardless I *think* this is ok but I'm worried I don't fully understand
what the problem is.
Ira
>
> Fix the issue by replacing get_cpu with smp_process_id and
> migrate_disable when needed. This makes BTT operations preemptible, thus
> permitting the use of spin_lock.
>
> Fixes: 5212e11fde4d ("nd_btt: atomic sector updates")
> Signed-off-by: Tomas Glozar
> ---
> drivers/nvdimm/region_devs.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> index 0a81f87f6f6c..e2f1fb99707f 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -939,7 +939,8 @@ unsigned int nd_region_acquire_lane(struct nd_region
> *nd_region)
> {
> unsigned int cpu, lane;
>
> - cpu = get_cpu();
> + migrate_disable();
> + cpu = smp_processor_id();
> if (nd_region->num_lanes < nr_cpu_ids) {
> struct nd_percpu_lane *ndl_lock, *ndl_count;
>
> @@ -958,16 +959,15 @@ EXPORT_SYMBOL(nd_region_acquire_lane);
> void nd_region_release_lane(struct nd_region *nd_region, unsigned int lane)
> {
> if (nd_region->num_lanes < nr_cpu_ids) {
> - unsigned int cpu = get_cpu();
> + unsigned int cpu = smp_processor_id();
> struct nd_percpu_lane *ndl_lock, *ndl_count;
>
> ndl_count = per_cpu_ptr(nd_region->lane, cpu);
> ndl_lock = per_cpu_ptr(nd_region->lane, lane);
> if (--ndl_count->count == 0)
> spin_unlock(_lock->lock);
> - put_cpu();
> }
> - put_cpu();
> + migrate_enable();
> }
> EXPORT_SYMBOL(nd_region_release_lane);
>
> --
> 2.39.3
>
Re: [PATCH v3] libnvdimm/of_pmem: Use devm_kstrdup instead of kstrdup and check its return value
Dave Jiang wrote:
>
>
> On 9/14/23 00:03, Chen Ni wrote:
[snip]
> > diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> > index 1b9f5b8a6167..5765674b36f2 100644
> > --- a/drivers/nvdimm/of_pmem.c
> > +++ b/drivers/nvdimm/of_pmem.c
> > @@ -30,7 +30,13 @@ static int of_pmem_region_probe(struct platform_device
> > *pdev)
> > if (!priv)
> > return -ENOMEM;
> >
> > - priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
> > + priv->bus_desc.provider_name = devm_kstrdup(>dev, pdev->name,
> > + GFP_KERNEL);
> > + if (!priv->bus_desc.provider_name) {
> > + kfree(priv);
>
> I wonder if priv should be allocated with devm_kzalloc() instead to reduce
> the resource management burden.
I think it could be but this is the driver and I wonder if leaving the
allocation around until the platform device goes away was undesirable for
some reason?
Ira
Re: [PATCH v2] nvdimm: of_pmem: Check return value and add kfree for kstrdup
Chen Ni wrote:
> Check the return value of kstrdup() and add kfree() for kstrdup() to
> avoid memory leak.
>
> Fixes: 49bddc73d15c ("libnvdimm/of_pmem: Provide a unique name for bus
> provider")
> Signed-off-by: Chen Ni
> ---
> Changelog:
>
> v1 -> v2:
>
> 1.Add a fixes tag.
> 2.Update commit message.
> ---
> drivers/nvdimm/of_pmem.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> index 10dbdcdfb9ce..fe6edb7e6631 100644
> --- a/drivers/nvdimm/of_pmem.c
> +++ b/drivers/nvdimm/of_pmem.c
> @@ -31,11 +31,17 @@ static int of_pmem_region_probe(struct platform_device
> *pdev)
> return -ENOMEM;
>
> priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
Could this be done with a devm_kstrdup()?
> + if (!priv->bus_desc.provider_name) {
> + kfree(priv);
> + return -ENOMEM;
> + }
> +
> priv->bus_desc.module = THIS_MODULE;
> priv->bus_desc.of_node = np;
>
> priv->bus = bus = nvdimm_bus_register(>dev, >bus_desc);
> if (!bus) {
> + kfree(priv->bus_desc.provider_name);
If not using devm_kstrdup() don't you need a kfree in
of_pmem_region_remove?
Ira
> kfree(priv);
> return -ENODEV;
> }
> --
> 2.25.1
>
Re: [PATCH ndctl v2] ndctl/cxl/test: Add CXL event test
Verma, Vishal L wrote: > On Mon, 2023-07-31 at 16:53 -0700, Ira Weiny wrote: > > Previously CXL event testing was run by hand. This reduces testing > > Reduces or increases / improves? Or did you mean running by hand > reduced coverage. Running by hand reduces test coverage. > > Maybe this can read "Improve testing coverage and address a lack of > automated regression testing by adding a unit test for this" Sounds good. > > (no need to respin, I can fixup when applying, just making sure I'm not > misinterpreting what you meant to say). Thanks, Ira > > > coverage including a lack of regression testing. > > > > Add a CXL event test as part of the meson test infrastructure. Passing > > is predicated on receiving the appropriate number of errors in each log. > > Individual event values are not checked. > > > > Signed-off-by: Ira Weiny > > --- > > Changes in v2: > > [djiang] run shellcheck and fix as needed > > > > [vishal] quote variables > > > > [vishal] skip test if event_trigger is not available > > > > [vishal] remove dead code > > > > [vishal] explicitly use the first memdev returned from cxl-cli > > > > [vishal] store trace output in a variable > > > > [vishal] simplify grep statement looking for results > > > > [vishal] use variables for expected values > > > > - Link to v1: > > https://lore.kernel.org/r/20230726-cxl-event-v1-1-1cf8cb02b...@intel.com > > --- > > test/cxl-events.sh | 76 > > ++ > > test/meson.build | 2 ++ > > 2 files changed, 78 insertions(+) > > > Thanks for the rev, everything else looks good.
[PATCH ndctl v2] ndctl/cxl/test: Add CXL event test
Previously CXL event testing was run by hand. This reduces testing
coverage including a lack of regression testing.
Add a CXL event test as part of the meson test infrastructure. Passing
is predicated on receiving the appropriate number of errors in each log.
Individual event values are not checked.
Signed-off-by: Ira Weiny
---
Changes in v2:
[djiang] run shellcheck and fix as needed
[vishal] quote variables
[vishal] skip test if event_trigger is not available
[vishal] remove dead code
[vishal] explicitly use the first memdev returned from cxl-cli
[vishal] store trace output in a variable
[vishal] simplify grep statement looking for results
[vishal] use variables for expected values
- Link to v1:
https://lore.kernel.org/r/20230726-cxl-event-v1-1-1cf8cb02b...@intel.com
---
test/cxl-events.sh | 76 ++
test/meson.build | 2 ++
2 files changed, 78 insertions(+)
diff --git a/test/cxl-events.sh b/test/cxl-events.sh
new file mode 100644
index ..33b68daa6ade
--- /dev/null
+++ b/test/cxl-events.sh
@@ -0,0 +1,76 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2023 Intel Corporation. All rights reserved.
+
+. "$(dirname "$0")/common"
+
+# Results expected
+num_overflow_expected=1
+num_fatal_expected=2
+num_failure_expected=16
+num_info_expected=3
+
+set -ex
+
+trap 'err $LINENO' ERR
+
+check_prereq "jq"
+
+modprobe -r cxl_test
+modprobe cxl_test
+
+dev_path="/sys/bus/platform/devices"
+
+test_cxl_events()
+{
+ memdev="$1"
+
+ if [ ! -f "${dev_path}/${memdev}/event_trigger" ]; then
+ echo "TEST: Kernel does not support test event trigger"
+ exit 77
+ fi
+
+ echo "TEST: triggering $memdev"
+ echo 1 > "${dev_path}/${memdev}/event_trigger"
+}
+
+readarray -t memdevs < <("$CXL" list -b cxl_test -Mi | jq -r '.[].host')
+
+echo "TEST: Prep event trace"
+echo "" > /sys/kernel/tracing/trace
+echo 1 > /sys/kernel/tracing/events/cxl/enable
+echo 1 > /sys/kernel/tracing/tracing_on
+
+test_cxl_events "${memdevs[0]}"
+
+echo 0 > /sys/kernel/tracing/tracing_on
+
+echo "TEST: Events seen"
+trace_out=$(cat /sys/kernel/tracing/trace)
+
+num_overflow=$(grep -c "cxl_overflow" <<< "${trace_out}")
+num_fatal=$(grep -c "log=Fatal" <<< "${trace_out}")
+num_failure=$(grep -c "log=Failure" <<< "${trace_out}")
+num_info=$(grep -c "log=Informational" <<< "${trace_out}")
+echo " LOG (Expected) : (Found)"
+echo " overflow ($num_overflow_expected) : $num_overflow"
+echo " Fatal ($num_fatal_expected) : $num_fatal"
+echo " Failure ($num_failure_expected) : $num_failure"
+echo " Informational ($num_info_expected) : $num_info"
+
+if [ "$num_overflow" -ne $num_overflow_expected ]; then
+ err "$LINENO"
+fi
+if [ "$num_fatal" -ne $num_fatal_expected ]; then
+ err "$LINENO"
+fi
+if [ "$num_failure" -ne $num_failure_expected ]; then
+ err "$LINENO"
+fi
+if [ "$num_info" -ne $num_info_expected ]; then
+ err "$LINENO"
+fi
+
+check_dmesg "$LINENO"
+
+modprobe -r cxl_test
diff --git a/test/meson.build b/test/meson.build
index a956885f6df6..a33255bde1a8 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -155,6 +155,7 @@ cxl_sysfs = find_program('cxl-region-sysfs.sh')
cxl_labels = find_program('cxl-labels.sh')
cxl_create_region = find_program('cxl-create-region.sh')
cxl_xor_region = find_program('cxl-xor-region.sh')
+cxl_events = find_program('cxl-events.sh')
tests = [
[ 'libndctl', libndctl, 'ndctl' ],
@@ -183,6 +184,7 @@ tests = [
[ 'cxl-labels.sh', cxl_labels,'cxl' ],
[ 'cxl-create-region.sh', cxl_create_region, 'cxl' ],
[ 'cxl-xor-region.sh', cxl_xor_region, 'cxl' ],
+ [ 'cxl-events.sh', cxl_events, 'cxl' ],
]
if get_option('destructive').enabled()
---
base-commit: 2fd570a0ed788b1bd0971dfdb1466a5dbcb79775
change-id: 20230726-cxl-event-dc00a2f94b60
Best regards,
--
Ira Weiny
Re: [PATCH ndctl] ndctl/cxl/test: Add CXL event test
Verma, Vishal L wrote:
> On Thu, 2023-07-27 at 14:21 -0700, Ira Weiny wrote:
> > Previously CXL event testing was run by hand. This reduces testing
> > coverage including a lack of regression testing.
> >
> > Add a CXL test as part of the meson test infrastructure. Passing is
> > predicated on receiving the appropriate number of errors in each log.
> > Individual event values are not checked.
> >
> > Signed-off-by: Ira Weiny
> > ---
> > test/cxl-events.sh | 68
> > ++
> > test/meson.build | 2 ++
> > 2 files changed, 70 insertions(+)
>
> Hi Ira,
>
> Thanks for adding this test. Just a few minor comments below, otherwise
> looks good.
Thanks!
>
> >
> > diff --git a/test/cxl-events.sh b/test/cxl-events.sh
> > new file mode 100644
> > index ..f51046ec39ad
> > --- /dev/null
> > +++ b/test/cxl-events.sh
> > @@ -0,0 +1,68 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (C) 2023 Intel Corporation. All rights reserved.
> > +
> > +. $(dirname $0)/common
> > +
> > +set -ex
> > +
> > +trap 'err $LINENO' ERR
> > +
> > +check_prereq "jq"
> > +
> > +modprobe -r cxl_test
> > +modprobe cxl_test
> > +
> > +dev_path="/sys/bus/platform/devices"
> > +
> > +test_cxl_events()
> > +{
> > + memdev="$1"
> > +
> > + echo "TEST: triggering $memdev"
> > + echo 1 > $dev_path/$memdev/event_trigger
>
> Quote the "$variables" here. We don't expect spaces in the path in this
> case, so it will still work, but it is good practice to always quote
> everything.
Done.
>
> We might also need a test to see if this file exists first. For kernels
> that don't have this support, we probably want to print a message and
> skip the test (return '77').
Good idea.
>
> > +}
> > +
> > +readarray -t memdevs < <("$CXL" list -b cxl_test -Mi | jq -r '.[].host')
> > +
> > +echo "TEST: Prep event trace"
> > +echo "" > /sys/kernel/tracing/trace
> > +echo 1 > /sys/kernel/tracing/events/cxl/enable
> > +echo 1 > /sys/kernel/tracing/tracing_on
> > +
> > +# Only need to test 1 device
> > +#for memdev in ${memdevs[@]}; do
> > +#done
>
> Probably just remove the commented out loop, if we need to test more
> than one memdev in the future, it is easy enough to add back then.
Done.
>
> > +
> > +test_cxl_events "$memdevs"
>
> Shouldn't use "$memdevs" here since it is an array. If you want to pass
> in just the first memdev, use "${memdevs[0]}"
Ah yea caught my hack ;-) done.
>
> > +
> > +echo 0 > /sys/kernel/tracing/tracing_on
> > +
> > +echo "TEST: Events seen"
> > +cat /sys/kernel/tracing/trace
> > +num_overflow=$(grep "cxl_overflow" /sys/kernel/tracing/trace | wc -l)
> > +num_fatal=$(grep "log=Fatal" /sys/kernel/tracing/trace | wc -l)
> > +num_failure=$(grep "log=Failure" /sys/kernel/tracing/trace | wc -l)
> > +num_info=$(grep "log=Informational" /sys/kernel/tracing/trace | wc -l)
>
> Minor nit, but you can 'grep -c' instead of 'grep | wc -l'
Ok Done.
>
> Also could put /sys/kernel/tracing/trace into a variable just for
> readability since it is used many times.
Done.
>
> > +echo " LOG (Expected) : (Found)"
> > +echo " overflow ( 1) : $num_overflow"
> > +echo " Fatal ( 2) : $num_fatal"
> > +echo " Failure (16) : $num_failure"
> > +echo " Informational ( 3) : $num_info"
> > +
> > +if [ "$num_overflow" -ne 1 ]; then
> > + err "$LINENO"
> > +fi
> > +if [ "$num_fatal" -ne 2 ]; then
> > + err "$LINENO"
> > +fi
> > +if [ "$num_failure" -ne 16 ]; then
> > + err "$LINENO"
> > +fi
> > +if [ "$num_info" -ne 3 ]; then
> > + err "$LINENO"
> > +fi
>
> Perhaps define variables for each of the expected nums, that way there
> is only one spot to change in case the numbers change in the future.
Good idea, done.
V2 on it's way soon, thanks for looking,
Ira
>
> > +
> > +check_dmesg "$LINENO"
> > +
> > +modprobe -r cxl_test
> > diff --git a/test/meson.build b/test/meson.build
> > index a956885f6df6..a33255bde1a8 100644
> > --- a/test/meson.build
> > +++ b/test/meson.build
> > @@ -155,6 +155,7 @@ cxl_sysfs = find_program('cxl-region-sysfs.sh')
> > cxl_labels = find_program('cxl-labels.sh')
> > cxl_create_region = find_program('cxl-create-region.sh')
> > cxl_xor_region = find_program('cxl-xor-region.sh')
> > +cxl_events = find_program('cxl-events.sh')
> >
> > tests = [
> > [ 'libndctl', libndctl, 'ndctl' ],
> > @@ -183,6 +184,7 @@ tests = [
> > [ 'cxl-labels.sh', cxl_labels, 'cxl' ],
> > [ 'cxl-create-region.sh', cxl_create_region, 'cxl' ],
> > [ 'cxl-xor-region.sh', cxl_xor_region, 'cxl' ],
> > + [ 'cxl-events.sh', cxl_events, 'cxl' ],
> > ]
> >
> > if get_option('destructive').enabled()
> >
> > ---
> > base-commit: 2fd570a0ed788b1bd0971dfdb1466a5dbcb79775
> > change-id: 20230726-cxl-event-dc00a2f94b60
> >
> > Best regards,
>
Re: Pump actuation lag when raising brew lever
Hello David, Monday, July 31, 2023, 11:44:06 AM, you wrote: > Well, I tried to replace the relay, but the original board is kind of > broken now...ooops > The espresso machine is in Austria working just fine with the new > electronic box, and I am in Sweden during the summer, so I don't see > any reason to to more repairs right now. > Thank you for the support :) Oh, shipping to California and back is probably not reasonable. -- Ira -- You received this message because you are subscribed to the Google Groups "Brewtus" group. To unsubscribe from this group and stop receiving emails from it, send an email to brewtus+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/brewtus/1466333578.20230731114540%40gmail.com.
Re: [PATCH ndctl] ndctl/cxl/test: Add CXL event test
Dave Jiang wrote: > > > On 7/27/23 14:21, Ira Weiny wrote: > > Previously CXL event testing was run by hand. This reduces testing > > coverage including a lack of regression testing. > > > > Add a CXL test as part of the meson test infrastructure. Passing is > > predicated on receiving the appropriate number of errors in each log. > > Individual event values are not checked. > > > > Signed-off-by: Ira Weiny > > LGTM. Have you tried running shellcheck? Never heard of it... searching... This? ShellCheck.x86_64 : Shell script analysis tool > > Reviewed-by: Dave Jiang Thanks, Ira
[PATCH ndctl] ndctl/cxl/test: Add CXL event test
Previously CXL event testing was run by hand. This reduces testing
coverage including a lack of regression testing.
Add a CXL test as part of the meson test infrastructure. Passing is
predicated on receiving the appropriate number of errors in each log.
Individual event values are not checked.
Signed-off-by: Ira Weiny
---
test/cxl-events.sh | 68 ++
test/meson.build | 2 ++
2 files changed, 70 insertions(+)
diff --git a/test/cxl-events.sh b/test/cxl-events.sh
new file mode 100644
index ..f51046ec39ad
--- /dev/null
+++ b/test/cxl-events.sh
@@ -0,0 +1,68 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2023 Intel Corporation. All rights reserved.
+
+. $(dirname $0)/common
+
+set -ex
+
+trap 'err $LINENO' ERR
+
+check_prereq "jq"
+
+modprobe -r cxl_test
+modprobe cxl_test
+
+dev_path="/sys/bus/platform/devices"
+
+test_cxl_events()
+{
+ memdev="$1"
+
+ echo "TEST: triggering $memdev"
+ echo 1 > $dev_path/$memdev/event_trigger
+}
+
+readarray -t memdevs < <("$CXL" list -b cxl_test -Mi | jq -r '.[].host')
+
+echo "TEST: Prep event trace"
+echo "" > /sys/kernel/tracing/trace
+echo 1 > /sys/kernel/tracing/events/cxl/enable
+echo 1 > /sys/kernel/tracing/tracing_on
+
+# Only need to test 1 device
+#for memdev in ${memdevs[@]}; do
+#done
+
+test_cxl_events "$memdevs"
+
+echo 0 > /sys/kernel/tracing/tracing_on
+
+echo "TEST: Events seen"
+cat /sys/kernel/tracing/trace
+num_overflow=$(grep "cxl_overflow" /sys/kernel/tracing/trace | wc -l)
+num_fatal=$(grep "log=Fatal" /sys/kernel/tracing/trace | wc -l)
+num_failure=$(grep "log=Failure" /sys/kernel/tracing/trace | wc -l)
+num_info=$(grep "log=Informational" /sys/kernel/tracing/trace | wc -l)
+echo " LOG (Expected) : (Found)"
+echo " overflow ( 1) : $num_overflow"
+echo " Fatal ( 2) : $num_fatal"
+echo " Failure (16) : $num_failure"
+echo " Informational ( 3) : $num_info"
+
+if [ "$num_overflow" -ne 1 ]; then
+ err "$LINENO"
+fi
+if [ "$num_fatal" -ne 2 ]; then
+ err "$LINENO"
+fi
+if [ "$num_failure" -ne 16 ]; then
+ err "$LINENO"
+fi
+if [ "$num_info" -ne 3 ]; then
+ err "$LINENO"
+fi
+
+check_dmesg "$LINENO"
+
+modprobe -r cxl_test
diff --git a/test/meson.build b/test/meson.build
index a956885f6df6..a33255bde1a8 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -155,6 +155,7 @@ cxl_sysfs = find_program('cxl-region-sysfs.sh')
cxl_labels = find_program('cxl-labels.sh')
cxl_create_region = find_program('cxl-create-region.sh')
cxl_xor_region = find_program('cxl-xor-region.sh')
+cxl_events = find_program('cxl-events.sh')
tests = [
[ 'libndctl', libndctl, 'ndctl' ],
@@ -183,6 +184,7 @@ tests = [
[ 'cxl-labels.sh', cxl_labels,'cxl' ],
[ 'cxl-create-region.sh', cxl_create_region, 'cxl' ],
[ 'cxl-xor-region.sh', cxl_xor_region, 'cxl' ],
+ [ 'cxl-events.sh', cxl_events, 'cxl' ],
]
if get_option('destructive').enabled()
---
base-commit: 2fd570a0ed788b1bd0971dfdb1466a5dbcb79775
change-id: 20230726-cxl-event-dc00a2f94b60
Best regards,
--
Ira Weiny
Re: [Qemu RFC 0/7] Early enabling of DCD emulation in Qemu
Fan Ni wrote:
> On Thu, May 11, 2023 at 05:56:40PM +, Fan Ni wrote:
>
> FYI.
>
> I have updated the patch series and sent out again.
>
> I suggested anyone who are interested in DCD and using this patch series to
> use the new series. Quite a few things has been fixed.
>
> https://lore.kernel.org/linux-cxl/20230724162313.34196-1-fan...@samsung.com/T/#t
>
> Also, if you want to use the code repo directly, you can try
>
> https://github.com/moking/qemu-dcd-preview-latest/tree/dcd-dev
Thanks for the branch!
I took a quick look and I don't see a resolution to the problem I
mentioned with non DCD devices being supported.[1]
[1] https://lore.kernel.org/all/6483946e8152f_f1132294a2@iweiny-mobl.notmuch/
Did you fix this in a different way? If I don't add DC to my mem devices they
don't get probed properly. I'm still looking into this with your new branch,
but I don't think DC commands should be in the CEL if the device does not
support it.
Also I get a build warning on this branch I had to fix[3] as my build is
treating warnings as errors.[2]
I don't think this fix is technically necessary as 'list' should never be NULL
that I can see. But might be nice to check or just use my fix.
I'll try and get to a review once I get the DCD stuff out on the list again.
Ira
[2]
../hw/mem/cxl_type3.c: In function
‘qmp_cxl_process_dynamic_capacity_event.constprop’:
../hw/mem/cxl_type3.c:2063:28: error: ‘rid’ may be used uninitialized
[-Werror=maybe-uninitialized]
2063 | dCap.updated_region_id = rid;
| ~~~^
../hw/mem/cxl_type3.c:1987:13: note: ‘rid’ was declared here
1987 | uint8_t rid;
| ^~~
cc1: all warnings being treated as errors
[3]
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index e67328780407..d25e6064f6c9 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1984,7 +1984,7 @@ static void qmp_cxl_process_dynamic_capacity_event(const
char *path,
CXLDCExtentRecordList *list = records;
CXLDCExtent_raw *extents;
uint64_t dpa, len;
-uint8_t rid;
+uint8_t rid = 0;
int i;
if (!obj) {
Re: [PATCH] Remove unnecessary calls to kmap{,_local_page}() when acquiring pages using GFP_DMA32.
Sumitra Sharma wrote: > The GFP_DMA32 uses the DMA32 zone to satisfy the allocation > requests. Therefore, pages allocated with GFP_DMA32 cannot > come from Highmem. > > Avoid using calls to kmap() / kunmap() as the kmap() is being > deprecated [1]. > > Avoid using calls to kmap_local_page() / kunmap_local() as the > code does not depends on the implicit disable of migration of > local mappings and is, in fact, an unnecessary overhead for > the main code [2]. > > Hence, use a plain page_address() directly in the > psb_mmu_alloc_pd function. > > [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.we...@intel.com/ > [2]: https://lwn.net/Articles/836503/ > > Suggested-by: Ira Weiny Reviewed-by: Ira Weiny > Signed-off-by: Sumitra Sharma > --- > drivers/gpu/drm/gma500/mmu.c | 11 +++ > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/gma500/mmu.c b/drivers/gpu/drm/gma500/mmu.c > index a70b01ccdf70..1a44dd062fd1 100644 > --- a/drivers/gpu/drm/gma500/mmu.c > +++ b/drivers/gpu/drm/gma500/mmu.c > @@ -184,20 +184,15 @@ struct psb_mmu_pd *psb_mmu_alloc_pd(struct > psb_mmu_driver *driver, > pd->invalid_pte = 0; > } > > - v = kmap_local_page(pd->dummy_pt); > + v = page_address(pd->dummy_pt); > for (i = 0; i < (PAGE_SIZE / sizeof(uint32_t)); ++i) > v[i] = pd->invalid_pte; > > - kunmap_local(v); > - > - v = kmap_local_page(pd->p); > + v = page_address(pd->p); > for (i = 0; i < (PAGE_SIZE / sizeof(uint32_t)); ++i) > v[i] = pd->invalid_pde; > > - kunmap_local(v); > - > - clear_page(kmap(pd->dummy_page)); > - kunmap(pd->dummy_page); > + clear_page(page_address(pd->dummy_page)); > > pd->tables = vmalloc_user(sizeof(struct psb_mmu_pt *) * 1024); > if (!pd->tables) > -- > 2.25.1 >
Re: [Qemu PATCH 0/9] Enabling DCD emulation support in Qemu
nifan@ wrote: > From: Fan Ni > > The patch series provides dynamic capacity device (DCD) emulation in Qemu. > More specifically, it provides the following functionalities: > 1. Extended type3 memory device to support DC regions and extents. > 2. Implemented DCD related mailbox command support in CXL r3.0: 8.2.9.8.9. > 3. ADD QMP interfaces for adding and releasing DC extents to simulate FM > functions for DCD described in cxl r3.0: 7.6.7.6.5 and 7.6.7.6.6. > 4. Add new ct3d properties for DCD devices (host backend, number of dc > regions, etc.) > 5. Add read/write support from/to DC regions of the device. > 6. Add mechanism to validate accessed to DC region address space. > Sorry I see them in this other thread now. https://lore.kernel.org/all/sg2pr06mb3397f3e74a083607f7492fa4b2...@sg2pr06mb3397.apcprd06.prod.outlook.com/ Thanks, Ira > A more detailed description can be found from the previously posted RFC[1]. > > Compared to the previously posted RFC[1], following changes have been made: > 1. Rebased the code on top of Jonathan's branch > https://gitlab.com/jic23/qemu/-/tree/cxl-2023-05-25. > 2. Extracted the rename of mem_size to a separated patch.(Jonathan) > 3. Reordered the patch series to improve its readability.(Jonathan) > 4. Split the validation of accesses to DC region address space as a separate > patch. > 5. Redesigned the QMP interfaces for adding and releasing DC extents to make > them easier to understand and act like existing QMP interfaces (like the > interface for cxl-inject-uncorrectable-errors). (Jonathan) > 6. Updated dvsec range register setting to support DCD devices without static > capacity. > 7. Fixed other issues mentioned in the comments (Jonathan Fontenot). > 8. Fixed the format issues and checked with checkpatch.pl under qemu code dir. > > > The code is tested with the DCD patch series at the kernel side[2]. The test > is similar to those mentioned in the cover letter of [1]. > > > [1]: https://lore.kernel.org/all/20230511175609.2091136-1-fan...@samsung.com/ > [2]: > https://lore.kernel.org/linux-cxl/649da378c28a3_968bb29420@iweiny-mobl.notmuch/T/#t > > Fan Ni (9): > hw/cxl/cxl-mailbox-utils: Add dc_event_log_size field to output > payload of identify memory device command > hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative > and mailbox command support > include/hw/cxl/cxl_device: Rename mem_size as static_mem_size for > type3 memory devices > hw/mem/cxl_type3: Add support to create DC regions to type3 memory > devices > hw/mem/cxl_type3: Add host backend and address space handling for DC > regions > hw/mem/cxl_type3: Add DC extent list representative and get DC extent > list mailbox support > hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release > dynamic capacity response > hw/cxl/events: Add qmp interfaces to add/release dynamic capacity > extents > hw/mem/cxl_type3: Add dpa range validation for accesses to dc regions > > hw/cxl/cxl-mailbox-utils.c | 421 +++- > hw/mem/cxl_type3.c | 539 +--- > hw/mem/cxl_type3_stubs.c| 6 + > include/hw/cxl/cxl_device.h | 49 +++- > include/hw/cxl/cxl_events.h | 16 ++ > qapi/cxl.json | 49 > 6 files changed, 1034 insertions(+), 46 deletions(-) > > -- > 2.39.2 >
Re: [Qemu PATCH 0/9] Enabling DCD emulation support in Qemu
nifan@ wrote: > From: Fan Ni > > The patch series provides dynamic capacity device (DCD) emulation in Qemu. I don't the patches on the list. https://lore.kernel.org/all/sg2pr06mb33976bb3f9c47cbe08f02d09b2...@sg2pr06mb3397.apcprd06.prod.outlook.com/ Did they get sent? Ira > More specifically, it provides the following functionalities: > 1. Extended type3 memory device to support DC regions and extents. > 2. Implemented DCD related mailbox command support in CXL r3.0: 8.2.9.8.9. > 3. ADD QMP interfaces for adding and releasing DC extents to simulate FM > functions for DCD described in cxl r3.0: 7.6.7.6.5 and 7.6.7.6.6. > 4. Add new ct3d properties for DCD devices (host backend, number of dc > regions, etc.) > 5. Add read/write support from/to DC regions of the device. > 6. Add mechanism to validate accessed to DC region address space. > > A more detailed description can be found from the previously posted RFC[1]. > > Compared to the previously posted RFC[1], following changes have been made: > 1. Rebased the code on top of Jonathan's branch > https://gitlab.com/jic23/qemu/-/tree/cxl-2023-05-25. > 2. Extracted the rename of mem_size to a separated patch.(Jonathan) > 3. Reordered the patch series to improve its readability.(Jonathan) > 4. Split the validation of accesses to DC region address space as a separate > patch. > 5. Redesigned the QMP interfaces for adding and releasing DC extents to make > them easier to understand and act like existing QMP interfaces (like the > interface for cxl-inject-uncorrectable-errors). (Jonathan) > 6. Updated dvsec range register setting to support DCD devices without static > capacity. > 7. Fixed other issues mentioned in the comments (Jonathan Fontenot). > 8. Fixed the format issues and checked with checkpatch.pl under qemu code dir. > > > The code is tested with the DCD patch series at the kernel side[2]. The test > is similar to those mentioned in the cover letter of [1]. > > > [1]: https://lore.kernel.org/all/20230511175609.2091136-1-fan...@samsung.com/ > [2]: > https://lore.kernel.org/linux-cxl/649da378c28a3_968bb29420@iweiny-mobl.notmuch/T/#t > > Fan Ni (9): > hw/cxl/cxl-mailbox-utils: Add dc_event_log_size field to output > payload of identify memory device command > hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative > and mailbox command support > include/hw/cxl/cxl_device: Rename mem_size as static_mem_size for > type3 memory devices > hw/mem/cxl_type3: Add support to create DC regions to type3 memory > devices > hw/mem/cxl_type3: Add host backend and address space handling for DC > regions > hw/mem/cxl_type3: Add DC extent list representative and get DC extent > list mailbox support > hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release > dynamic capacity response > hw/cxl/events: Add qmp interfaces to add/release dynamic capacity > extents > hw/mem/cxl_type3: Add dpa range validation for accesses to dc regions > > hw/cxl/cxl-mailbox-utils.c | 421 +++- > hw/mem/cxl_type3.c | 539 +--- > hw/mem/cxl_type3_stubs.c| 6 + > include/hw/cxl/cxl_device.h | 49 +++- > include/hw/cxl/cxl_events.h | 16 ++ > qapi/cxl.json | 49 > 6 files changed, 1034 insertions(+), 46 deletions(-) > > -- > 2.39.2 >
Re: [PATCH v3] libnvdimm/of_pmem: Replace kstrdup with devm_kstrdup and add check
Jiasheng Jiang wrote:
> Replace kstrdup() with devm_kstrdup() to avoid memory leak and
> add check for the return value of the devm_kstrdup() to avoid
> NULL pointer dereference
>
> Fixes: 49bddc73d15c ("libnvdimm/of_pmem: Provide a unique name for bus
> provider")
> Signed-off-by: Jiasheng Jiang
LTGM
Reviewed-by: Ira Weiny
> ---
> Changelog:
>
> v2 -> v3:
>
> 1. Correct the usage of devm_kstrdup().
>
> v1 -> v2:
>
> 1. Replace kstrdup() with devm_kstrdup().
> ---
> drivers/nvdimm/of_pmem.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> index 10dbdcdfb9ce..1d2b1ab5b737 100644
> --- a/drivers/nvdimm/of_pmem.c
> +++ b/drivers/nvdimm/of_pmem.c
> @@ -30,7 +30,12 @@ static int of_pmem_region_probe(struct platform_device
> *pdev)
> if (!priv)
> return -ENOMEM;
>
> - priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
> + priv->bus_desc.provider_name = devm_kstrdup(>dev, pdev->name,
> GFP_KERNEL);
> + if (!priv->bus_desc.provider_name) {
> + kfree(priv);
> + return -ENOMEM;
> + }
> +
> priv->bus_desc.module = THIS_MODULE;
> priv->bus_desc.of_node = np;
>
> --
> 2.25.1
>
[MOPO] Visited Movie Poster Archives
I was in New Orleans for the Hearing Loss Association of America annual convention.I were hearing aids and I have a cochlear implant. So I took the opportunity to visit Ed at the Movie Poster Archives! So much fun to visit his gallery.I even picked up a few stills.Impressive work he is doing. Anyone know of Galleries I can visit in Kentucky? Next stop as we visit the bourbon trail. Ira Sent via mobile.Please excuse typos and autocorrects. Visit the MoPo Mailing List Web Site at www.filmfan.com ___ How to UNSUBSCRIBE from the MoPo Mailing List Send a message addressed to: lists...@listserv.american.edu In the BODY of your message type: SIGNOFF MOPO-L The author of this message is solely responsible for its content.
[GNC] Upgraded to v5.2- now can't save
I just upgraded to v5.2 and I can no longer save changes. I get this error: Could not write to file /Users/if/Documents/GnuCash folder/ gnucash_transactions.gnucash.20181105195638.gnucash. 20211205203910.gnucash. Check that you have permission to write to this file and that there is sufficient space to create it. I have gone back to v4.13. ___ gnucash-user mailing list gnucash-user@gnucash.org To update your subscription preferences or to unsubscribe: https://lists.gnucash.org/mailman/listinfo/gnucash-user - Please remember to CC this list on all your replies. You can do this by using Reply-To-List or Reply-All.
Re: [PATCH] libnvdimm/of_pmem: Add check and kfree for kstrdup
Jiasheng Jiang wrote: > On Wed, Jun 21, 2023 at 00:04:36 +0800, Ira Weiny wrote: > > Ira Weiny wrote: > >> Jiasheng Jiang wrote: [snip] > >> > >> Nice catch! > >> > >> However, this free needs to happen in of_pmem_region_remove() as well. > > > > Looks like the mail from my phone had html in it. Sorry for that. > > > > This would be better with devm_kstrdup() and then we don't have to worry > > about the kfree at all. > > Looks good. > I have submitted a new patch "libnvdimm/of_pmem: Replace kstrdup with > devm_kstrdup and add check". > Since the titie has been modified, I did not submitted a v2. Ah ok... But looks like we will need a v3. See the other email. Thanks again for trying to fix this, Ira
Re: [PATCH] libnvdimm/of_pmem: Replace kstrdup with devm_kstrdup and add check
Jiasheng Jiang wrote:
> Replace kstrdup() with devm_kstrdup() to avoid memory leak and
> add check for the return value of the devm_kstrdup() to avoid
> NULL pointer dereference
>
> Fixes: 49bddc73d15c ("libnvdimm/of_pmem: Provide a unique name for bus
> provider")
> Signed-off-by: Jiasheng Jiang
V2? references to the first fix and review?
> ---
> drivers/nvdimm/of_pmem.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> index 10dbdcdfb9ce..5106dfe0147b 100644
> --- a/drivers/nvdimm/of_pmem.c
> +++ b/drivers/nvdimm/of_pmem.c
> @@ -30,7 +30,12 @@ static int of_pmem_region_probe(struct platform_device
> *pdev)
> if (!priv)
> return -ENOMEM;
>
> - priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
> + priv->bus_desc.provider_name = devm_kstrdup(pdev->name, GFP_KERNEL);
Again, thanks for finding and trying to fix this but you did not even compile
test this. :-(
/**
* devm_kstrdup - Allocate resource managed space and
*copy an existing string into that.
* @dev: Device to allocate memory for
* @s: the string to duplicate
* @gfp: the GFP mask used in the devm_kmalloc() call when
* allocating memory
* RETURNS:
* Pointer to allocated string on success, NULL on failure.
*/
char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp)
^^
??
Ira
> + if (!priv->bus_desc.provider_name) {
> + kfree(priv);
> + return -ENOMEM;
> + }
> +
> priv->bus_desc.module = THIS_MODULE;
> priv->bus_desc.of_node = np;
>
> --
> 2.25.1
>
[MOPO] WTB Scooby Doo and Space Jam
Hello Mopo! Well, after years and years of collecting posters. Mostly from work. . I have started to sell a few and opened an online store. Posterpalace.com. Big thanks to Sue Heim for all the friendly tips. It’s still a hobby, and I still have a full time job. But I figure when I retire in the next decade, this will keep me busy. So one of my first sales was DESTROYED by UPS. A Bruce tube that was absolutely crushed/punctured/tore the poster inside. Thanks UPS. And I want to help replace the posters I sold. A 2002 Scooby Doo Cast poster. And a original Space Jam with Bugs and Jordan. Anyone have any they would sell me? Please be kind to my store. I have lots and lots to learn. Lots more posters to list. Of course any constructive feedback is always appreciated. Thanks, Ira Visit the MoPo Mailing List Web Site at www.filmfan.com ___ How to UNSUBSCRIBE from the MoPo Mailing List Send a message addressed to: lists...@listserv.american.edu In the BODY of your message type: SIGNOFF MOPO-L The author of this message is solely responsible for its content.
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
Thomas Hellström (Intel) wrote: > > I think one thing worth mentioning in the context of this patch is that > IIRC kmap_local_page() will block offlining of the mapping CPU until > kunmap_local(), so while I haven't seen any guidelines around the usage > of this api for long-held mappings, I figure it's wise to keep the > mapping duration short, or at least avoid sleeping with a > kmap_local_page() map active. > > I figured, while page compression is probably to be considered "slow" > it's probably not slow enough to motivate kmap() instead of > kmap_local_page(), but if anyone feels differently, perhaps it should be > considered. What you say is all true. But remember the mappings are only actually created on a HIGHMEM system. HIGHMEM systems are increasingly rare. Also they must suffer such performance issues because there is just no other way around supporting them. Also Sumitra, and our kmap conversion project in general, is focusing on not using kmap* if at all possible. Thus the reason V1 tried to use page_address(). Could we guarantee the i915 driver is excluded from all HIGHMEM systems? > > With that said, my Reviewed-by: still stands. Thanks! Ira
Re: [Intel-gfx] [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
Thomas Hellström (Intel) wrote: > > I think one thing worth mentioning in the context of this patch is that > IIRC kmap_local_page() will block offlining of the mapping CPU until > kunmap_local(), so while I haven't seen any guidelines around the usage > of this api for long-held mappings, I figure it's wise to keep the > mapping duration short, or at least avoid sleeping with a > kmap_local_page() map active. > > I figured, while page compression is probably to be considered "slow" > it's probably not slow enough to motivate kmap() instead of > kmap_local_page(), but if anyone feels differently, perhaps it should be > considered. What you say is all true. But remember the mappings are only actually created on a HIGHMEM system. HIGHMEM systems are increasingly rare. Also they must suffer such performance issues because there is just no other way around supporting them. Also Sumitra, and our kmap conversion project in general, is focusing on not using kmap* if at all possible. Thus the reason V1 tried to use page_address(). Could we guarantee the i915 driver is excluded from all HIGHMEM systems? > > With that said, my Reviewed-by: still stands. Thanks! Ira
Re: [PATCH] libnvdimm/of_pmem: Add check and kfree for kstrdup
Ira Weiny wrote:
> Jiasheng Jiang wrote:
> > Add check for the return value of kstrdup() and return the error
> > if it fails in order to avoid NULL pointer dereference.
> > Moreover, use kfree() in the later error handling in order to avoid
> > memory leak.
> >
> > Fixes: 49bddc73d15c ("libnvdimm/of_pmem: Provide a unique name for bus
> > provider")
> > Signed-off-by: Jiasheng Jiang
> > ---
> > drivers/nvdimm/of_pmem.c | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> > index 10dbdcdfb9ce..fe6edb7e6631 100644
> > --- a/drivers/nvdimm/of_pmem.c
> > +++ b/drivers/nvdimm/of_pmem.c
> > @@ -31,11 +31,17 @@ static int of_pmem_region_probe(struct platform_device
> > *pdev)
> > return -ENOMEM;
> >
> > priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
> > + if (!priv->bus_desc.provider_name) {
> > + kfree(priv);
> > + return -ENOMEM;
> > + }
> > +
> > priv->bus_desc.module = THIS_MODULE;
> > priv->bus_desc.of_node = np;
> >
> > priv->bus = bus = nvdimm_bus_register(>dev, >bus_desc);
> > if (!bus) {
> > + kfree(priv->bus_desc.provider_name);
>
> Nice catch!
>
> However, this free needs to happen in of_pmem_region_remove() as well.
Looks like the mail from my phone had html in it. Sorry for that.
This would be better with devm_kstrdup() and then we don't have to worry
about the kfree at all.
Ira
Re: [PATCH] libnvdimm/of_pmem: Add check and kfree for kstrdup
Jiasheng Jiang wrote:
> Add check for the return value of kstrdup() and return the error
> if it fails in order to avoid NULL pointer dereference.
> Moreover, use kfree() in the later error handling in order to avoid
> memory leak.
>
> Fixes: 49bddc73d15c ("libnvdimm/of_pmem: Provide a unique name for bus
> provider")
> Signed-off-by: Jiasheng Jiang
> ---
> drivers/nvdimm/of_pmem.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
> index 10dbdcdfb9ce..fe6edb7e6631 100644
> --- a/drivers/nvdimm/of_pmem.c
> +++ b/drivers/nvdimm/of_pmem.c
> @@ -31,11 +31,17 @@ static int of_pmem_region_probe(struct platform_device
> *pdev)
> return -ENOMEM;
>
> priv->bus_desc.provider_name = kstrdup(pdev->name, GFP_KERNEL);
> + if (!priv->bus_desc.provider_name) {
> + kfree(priv);
> + return -ENOMEM;
> + }
> +
> priv->bus_desc.module = THIS_MODULE;
> priv->bus_desc.of_node = np;
>
> priv->bus = bus = nvdimm_bus_register(>dev, >bus_desc);
> if (!bus) {
> + kfree(priv->bus_desc.provider_name);
Nice catch!
However, this free needs to happen in of_pmem_region_remove() as well.
Ira
> kfree(priv);
> return -ENODEV;
> }
> --
> 2.25.1
>
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
Sumitra Sharma wrote: > On Sun, Jun 18, 2023 at 11:11:08AM -0700, Ira Weiny wrote: > > Sumitra Sharma wrote: > > > kmap() has been deprecated in favor of the kmap_local_page() > > > due to high cost, restricted mapping space, the overhead of a > > > global lock for synchronization, and making the process sleep > > > in the absence of free slots. > > > > > > kmap_local_page() is faster than kmap() and offers thread-local > > > and CPU-local mappings, take pagefaults in a local kmap region > > > and preserves preemption by saving the mappings of outgoing tasks > > > and restoring those of the incoming one during a context switch. > > > > > > The mapping is kept thread local in the function > > > “i915_vma_coredump_create” in i915_gpu_error.c > > > > > > Therefore, replace kmap() with kmap_local_page(). > > > > > > Suggested-by: Ira Weiny > > > > > > > NIT: No need for the line break between Suggested-by and your signed off > > line. > > > > Hi Ira, > > What does NIT stand for? Shorthand for 'nitpicking'. "giving too much attention to details that are not important, especially as a way of criticizing: " - https://dictionary.cambridge.org/dictionary/english/nitpicking Via email this is a way for authors of an email to indicate something is technically wrong but while nicely acknowledging that it is not very significant and could be seen as overly critical. For this particular comment I'm showing something to pay attention to next time but that was not a big deal this time around. > > Thank you. I will take care about the line breaks. > > > > Signed-off-by: Sumitra Sharma > > > --- > > > > > > Changes in v2: > > > - Replace kmap() with kmap_local_page(). > > > > Generally it is customary to attribute a change like this to those who > > suggested it in a V1 review. > > > > For example: > > > > - Tvrtko/Thomas: Use kmap_local_page() instead of page_address() > > > > Also I don't see Thomas on the new email list. Since he took the time to > > review V1 he might want to check this version out. I've added him to the > > 'To:' list. > > > > Also a link to V1 is nice. B4 formats it like this: > > > > - Link to v1: > > https://lore.kernel.org/all/20230614123556.ga381...@sumitra.com/ > > > > All that said the code looks good to me. So with the above changes. > > > > Reviewed-by: Ira Weiny > > > > I have noted down the points mentioned above. Thank you again. > > I am not supposed to create another version of this patch for > adding the above mentions, as you and Thomas both gave this patch > a reviewed-by tag. Right? > Based on this response[*] from Tvrtko I think this version can move through without a v3. Thanks! Ira [*] https://lore.kernel.org/all/bcb0a1d2-cd4d-a56f-1ee6-7ccfdd2f7...@linux.intel.com/ Thanks all! I'll just re-send the patch for our CI, since it didn't get picked up automatically (stuck in moderation perhaps), with all r-b tags added and extra line space removed and merge it if results will be green. Regards, Tvrtko > > Thanks & regards > Sumitra > > PS: I am new to the open source vocabulary terms. > > > > - Change commit subject and message. > > > > > > drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c > > > b/drivers/gpu/drm/i915/i915_gpu_error.c > > > index f020c0086fbc..bc41500eedf5 100644 > > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > > > @@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt *gt, > > > > > > drm_clflush_pages(, 1); > > > > > > - s = kmap(page); > > > + s = kmap_local_page(page); > > > ret = compress_page(compress, s, dst, false); > > > - kunmap(page); > > > + kunmap_local(s); > > > > > > drm_clflush_pages(, 1); > > > > > > -- > > > 2.25.1 > > > > > > >
Re: [Intel-gfx] [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
Sumitra Sharma wrote: > On Sun, Jun 18, 2023 at 11:11:08AM -0700, Ira Weiny wrote: > > Sumitra Sharma wrote: > > > kmap() has been deprecated in favor of the kmap_local_page() > > > due to high cost, restricted mapping space, the overhead of a > > > global lock for synchronization, and making the process sleep > > > in the absence of free slots. > > > > > > kmap_local_page() is faster than kmap() and offers thread-local > > > and CPU-local mappings, take pagefaults in a local kmap region > > > and preserves preemption by saving the mappings of outgoing tasks > > > and restoring those of the incoming one during a context switch. > > > > > > The mapping is kept thread local in the function > > > “i915_vma_coredump_create” in i915_gpu_error.c > > > > > > Therefore, replace kmap() with kmap_local_page(). > > > > > > Suggested-by: Ira Weiny > > > > > > > NIT: No need for the line break between Suggested-by and your signed off > > line. > > > > Hi Ira, > > What does NIT stand for? Shorthand for 'nitpicking'. "giving too much attention to details that are not important, especially as a way of criticizing: " - https://dictionary.cambridge.org/dictionary/english/nitpicking Via email this is a way for authors of an email to indicate something is technically wrong but while nicely acknowledging that it is not very significant and could be seen as overly critical. For this particular comment I'm showing something to pay attention to next time but that was not a big deal this time around. > > Thank you. I will take care about the line breaks. > > > > Signed-off-by: Sumitra Sharma > > > --- > > > > > > Changes in v2: > > > - Replace kmap() with kmap_local_page(). > > > > Generally it is customary to attribute a change like this to those who > > suggested it in a V1 review. > > > > For example: > > > > - Tvrtko/Thomas: Use kmap_local_page() instead of page_address() > > > > Also I don't see Thomas on the new email list. Since he took the time to > > review V1 he might want to check this version out. I've added him to the > > 'To:' list. > > > > Also a link to V1 is nice. B4 formats it like this: > > > > - Link to v1: > > https://lore.kernel.org/all/20230614123556.ga381...@sumitra.com/ > > > > All that said the code looks good to me. So with the above changes. > > > > Reviewed-by: Ira Weiny > > > > I have noted down the points mentioned above. Thank you again. > > I am not supposed to create another version of this patch for > adding the above mentions, as you and Thomas both gave this patch > a reviewed-by tag. Right? > Based on this response[*] from Tvrtko I think this version can move through without a v3. Thanks! Ira [*] https://lore.kernel.org/all/bcb0a1d2-cd4d-a56f-1ee6-7ccfdd2f7...@linux.intel.com/ Thanks all! I'll just re-send the patch for our CI, since it didn't get picked up automatically (stuck in moderation perhaps), with all r-b tags added and extra line space removed and merge it if results will be green. Regards, Tvrtko > > Thanks & regards > Sumitra > > PS: I am new to the open source vocabulary terms. > > > > - Change commit subject and message. > > > > > > drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c > > > b/drivers/gpu/drm/i915/i915_gpu_error.c > > > index f020c0086fbc..bc41500eedf5 100644 > > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > > > @@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt *gt, > > > > > > drm_clflush_pages(, 1); > > > > > > - s = kmap(page); > > > + s = kmap_local_page(page); > > > ret = compress_page(compress, s, dst, false); > > > - kunmap(page); > > > + kunmap_local(s); > > > > > > drm_clflush_pages(, 1); > > > > > > -- > > > 2.25.1 > > > > > > >
Re: GRS setup for MONOPLEX to SYSPLEX
Yes but the monoplex needs to communicate over CTCs to the SYSPLEX. Sent from my iPhone > On Jun 19, 2023, at 10:25 AM, Paul Gorlinsky wrote: > > We have a sandbox system that references TSO user and ISV catalogs and > datasets. The sandbox has a unique master catalog but shares virtually > everything else. > > It does have access to the couple facility that the Sysplex is using ... But > it is not a member of the Sysplex. > > Is it possible for a monoplex z/OS system to share DASD, user cats, etc., > with a SYSPLEX via GRS management and coupling facility? > > Thanks > > -- > For IBM-MAIN subscribe / signoff / archive access instructions, > send email to lists...@listserv.ua.edu with the message: INFO IBM-MAIN -- For IBM-MAIN subscribe / signoff / archive access instructions, send email to lists...@listserv.ua.edu with the message: INFO IBM-MAIN
Re: [Intel-gfx] [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
Sumitra Sharma wrote: > kmap() has been deprecated in favor of the kmap_local_page() > due to high cost, restricted mapping space, the overhead of a > global lock for synchronization, and making the process sleep > in the absence of free slots. > > kmap_local_page() is faster than kmap() and offers thread-local > and CPU-local mappings, take pagefaults in a local kmap region > and preserves preemption by saving the mappings of outgoing tasks > and restoring those of the incoming one during a context switch. > > The mapping is kept thread local in the function > “i915_vma_coredump_create” in i915_gpu_error.c > > Therefore, replace kmap() with kmap_local_page(). > > Suggested-by: Ira Weiny > NIT: No need for the line break between Suggested-by and your signed off line. > Signed-off-by: Sumitra Sharma > --- > > Changes in v2: > - Replace kmap() with kmap_local_page(). Generally it is customary to attribute a change like this to those who suggested it in a V1 review. For example: - Tvrtko/Thomas: Use kmap_local_page() instead of page_address() Also I don't see Thomas on the new email list. Since he took the time to review V1 he might want to check this version out. I've added him to the 'To:' list. Also a link to V1 is nice. B4 formats it like this: - Link to v1: https://lore.kernel.org/all/20230614123556.ga381...@sumitra.com/ All that said the code looks good to me. So with the above changes. Reviewed-by: Ira Weiny > - Change commit subject and message. > > drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c > b/drivers/gpu/drm/i915/i915_gpu_error.c > index f020c0086fbc..bc41500eedf5 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt *gt, > > drm_clflush_pages(, 1); > > - s = kmap(page); > + s = kmap_local_page(page); > ret = compress_page(compress, s, dst, false); > - kunmap(page); > + kunmap_local(s); > > drm_clflush_pages(, 1); > > -- > 2.25.1 >
Re: [PATCH v2] drm/i915: Replace kmap() with kmap_local_page()
Sumitra Sharma wrote: > kmap() has been deprecated in favor of the kmap_local_page() > due to high cost, restricted mapping space, the overhead of a > global lock for synchronization, and making the process sleep > in the absence of free slots. > > kmap_local_page() is faster than kmap() and offers thread-local > and CPU-local mappings, take pagefaults in a local kmap region > and preserves preemption by saving the mappings of outgoing tasks > and restoring those of the incoming one during a context switch. > > The mapping is kept thread local in the function > “i915_vma_coredump_create” in i915_gpu_error.c > > Therefore, replace kmap() with kmap_local_page(). > > Suggested-by: Ira Weiny > NIT: No need for the line break between Suggested-by and your signed off line. > Signed-off-by: Sumitra Sharma > --- > > Changes in v2: > - Replace kmap() with kmap_local_page(). Generally it is customary to attribute a change like this to those who suggested it in a V1 review. For example: - Tvrtko/Thomas: Use kmap_local_page() instead of page_address() Also I don't see Thomas on the new email list. Since he took the time to review V1 he might want to check this version out. I've added him to the 'To:' list. Also a link to V1 is nice. B4 formats it like this: - Link to v1: https://lore.kernel.org/all/20230614123556.ga381...@sumitra.com/ All that said the code looks good to me. So with the above changes. Reviewed-by: Ira Weiny > - Change commit subject and message. > > drivers/gpu/drm/i915/i915_gpu_error.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c > b/drivers/gpu/drm/i915/i915_gpu_error.c > index f020c0086fbc..bc41500eedf5 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -1164,9 +1164,9 @@ i915_vma_coredump_create(const struct intel_gt *gt, > > drm_clflush_pages(, 1); > > - s = kmap(page); > + s = kmap_local_page(page); > ret = compress_page(compress, s, dst, false); > - kunmap(page); > + kunmap_local(s); > > drm_clflush_pages(, 1); > > -- > 2.25.1 >
Re: [PATCH 3/4] dax: Introduce alloc_dev_dax_id()
Dan Williams wrote:
> Ira Weiny wrote:
> > Dan Williams wrote:
> > > The reference counting of dax_region objects is needlessly complicated,
> > > has lead to confusion [1], and has hidden a bug [2]. Towards cleaning up
> > > that mess introduce alloc_dev_dax_id() to minimize the holding of a
> > > dax_region reference to only what dev_dax_release() needs, the
> > > dax_region->ida.
> > >
> > > Part of the reason for the mess was the design to dereference a
> > > dax_region in all cases in free_dev_dax_id() even if the id was
> > > statically assigned by the upper level dax_region driver. Remove the
> > > need to call "is_static(dax_region)" by tracking whether the id is
> > > dynamic directly in the dev_dax instance itself.
> > >
> > > With that flag the dax_region pinning and release per dev_dax instance
> > > can move to alloc_dev_dax_id() and free_dev_dax_id() respectively.
> > >
> > > A follow-on cleanup address the unnecessary references in the dax_region
> > > setup and drivers.
> > >
> > > Fixes: 0f3da14a4f05 ("device-dax: introduce 'seed' devices")
> > > Link:
> > > http://lore.kernel.org/r/20221203095858.612027-1-liuyongqian...@huawei.com
> > > [1]
> > > Link:
> > > http://lore.kernel.org/r/3cf0890b-4eb0-e70e-cd9c-2ecc3d496...@hpe.com [2]
> > > Reported-by: Yongqiang Liu
> > > Reported-by: Paul Cassella
> > > Reported-by: Ira Weiny
> > > Signed-off-by: Dan Williams
> [..]
> > > @@ -1326,6 +1340,7 @@ struct dev_dax *devm_create_dev_dax(struct
> > > dev_dax_data *data)
> > > if (!dev_dax)
> > > return ERR_PTR(-ENOMEM);
> > >
> > > + dev_dax->region = dax_region;
> >
> > Overall I like that this reference is not needed to be carried and/or
> > managed by the callers.
> >
> > However, here you are referencing the dax_region from the dev_dax in an
> > unrelated place to where the reference matters (in id management).
> >
> > Could alloc_dev_dax_id() change to:
> >
> > static int alloc_dev_dax_id(struct dev_dax *dev_dax, struct dax_region
> > *dax_region)
> > {
> > ...
> > }
> >
> > Then make this assignment next to where the kref is taken so it is clear
> > that this is the only user of the reference?
> >
> > I did not pick up on the fact this reference was only needed to free the
> > id at all in reviewing the code and I think this would make it even more
> > clear.
>
> I hesitate only for symmetry reasons. I.e. that there are many interfaces in
> this file, in addition to free_dev_dax_id(), where @dax_region is
> implicitly retrieved from the @dev_dax.
Ok but the reason we need this extra reference and for the dax_region to
live this long is because the ida within the dax_region. Otherwise the
normal device references would be enough, right?
Regardless, I've convinced myself this is ok.
Reviewed-by: Ira Weiny
Ira
Re: [PATCH] dax: include bus.h for definition of run_dax()
Ben Dooks wrote: > The run_dax() prototype is defined in "bus.h" but drivers/dax/super.c > does not include this. Include bus.h to silece the following sparse > warning: > > drivers/dax/super.c:337:6: warning: symbol 'run_dax' was not declared. Should > it be static? A different version of this fix has already been queued up. https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/commit/?h=libnvdimm-fixes=2d5153526f929838b0912ded26862840f72745f4 Thank you anyway, Ira
Re: [PATCH] nvdimm: make nd_class variable static
Ben Dooks wrote: > The nd_class is not used outside of drivers/nvdimm/bus.c and thus sparse > is generating the following warning. Remove this by making it static: > > drivers/nvdimm/bus.c:28:14: warning: symbol 'nd_class' was not declared. > Should it be static? Reviewed-by: Ira Weiny > Signed-off-by: Ben Dooks > --- > drivers/nvdimm/bus.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c > index 954dbc105fc8..5852fe290523 100644 > --- a/drivers/nvdimm/bus.c > +++ b/drivers/nvdimm/bus.c > @@ -25,7 +25,7 @@ > > int nvdimm_major; > static int nvdimm_bus_major; > -struct class *nd_class; > +static struct class *nd_class; > static DEFINE_IDA(nd_ida); > > static int to_nd_device_type(const struct device *dev) > -- > 2.39.2 >
[MOPO] UPS Strike Coming?
Better get all your shipping done before July 31 if you use UPS…. https://www.washingtonpost.com/business/2023/06/15/ups-strike-vote-teamsters/ Thousands of Teamster UPS drivers across the United States are expected to authorize a strike Friday, bringing the country a step closer to what would be among the largest work stoppages in decades. An authorization vote does not does not mean that a strike will happen, but it gives Teamsters union leaders the right to call for a national walkout if a contract deal is not reached in negotiations before July 31. It would be the first strike to threaten the nation’s transportation infrastructure system since the last time UPS employees went on strike 26 years ago and the largest of any industry since the 1950s. Visit the MoPo Mailing List Web Site at www.filmfan.com ___ How to UNSUBSCRIBE from the MoPo Mailing List Send a message addressed to: lists...@listserv.american.edu In the BODY of your message type: SIGNOFF MOPO-L The author of this message is solely responsible for its content.
Re: [Qemu RFC 0/7] Early enabling of DCD emulation in Qemu
ni...@outlook.com wrote:
> The 06/08/2023 08:43, Ira Weiny wrote:
> > Shesha Bhushan Sreenivasamurthy wrote:
[snip]
>
> Hi Ira & Shesha,
> FYI. I reabased my patch series on top of the above branch and created a new
> branch here:
>
> https://github.com/moking/qemu-dcd-preview-latest/tree/dcd-preview
Thanks!
>
> It passes the same tests as shown here:
> https://lore.kernel.org/linux-cxl/6481f70fca5c2_c82be29440@iweiny-mobl.notmuch/T/#m76f6e85ce3d7292b1982960eb22086ee03922166
I've not gotten very far with this testing. But I did find that regular
type 3 devices don't work with this change. I used the patch below to get
this working. Was there something I was missing to configure a non-DCD
device?
I don't particularly like adding another bool to this call stack. Seems
like this calls for a flags field but I want to move on to DCD work so I
hacked this in.
Ira
commit ed27935044dcbd2c6ba71f8411b218621f3f4167
Author: Ira Weiny
Date: Fri Jun 9 13:56:33 2023 -0700
hw/mem/cxl_type3: Exclude DCD from CEL when type3 is not DCD
Per CXL 3.0 9.13.3 Dynamic Capacity Device (DCD) when the type 3 memory
device does not have DCD support the CEL should not include DCD
configuration commands.
If the number of DC regions supported is 0 skip the DCD commands in the
CEL.
Applies on top of Fan Ni's work here:
https://github.com/moking/qemu-dcd-preview-latest/tree/dcd-preview
Not-yet-Signed-off-by: Ira Weiny
diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
index a4a2c6a80004..262e35935563 100644
--- a/hw/cxl/cxl-device-utils.c
+++ b/hw/cxl/cxl-device-utils.c
@@ -288,7 +288,7 @@ static void mailbox_reg_init_common(CXLDeviceState
*cxl_dstate)
static void memdev_reg_init_common(CXLDeviceState *cxl_dstate) { }
-void cxl_device_register_init_common(CXLDeviceState *cxl_dstate)
+void cxl_device_register_init_common(CXLDeviceState *cxl_dstate, bool is_dcd)
{
uint64_t *cap_hdrs = cxl_dstate->caps_reg_state64;
const int cap_count = 3;
@@ -307,7 +307,7 @@ void cxl_device_register_init_common(CXLDeviceState
*cxl_dstate)
cxl_device_cap_init(cxl_dstate, MEMORY_DEVICE, 0x4000, 1);
memdev_reg_init_common(cxl_dstate);
-cxl_initialize_mailbox(cxl_dstate, false);
+cxl_initialize_mailbox(cxl_dstate, false, is_dcd);
}
void cxl_device_register_init_swcci(CXLDeviceState *cxl_dstate)
@@ -329,7 +329,7 @@ void cxl_device_register_init_swcci(CXLDeviceState
*cxl_dstate)
cxl_device_cap_init(cxl_dstate, MEMORY_DEVICE, 0x4000, 1);
memdev_reg_init_common(cxl_dstate);
-cxl_initialize_mailbox(cxl_dstate, true);
+cxl_initialize_mailbox(cxl_dstate, true, false);
}
uint64_t cxl_device_get_timestamp(CXLDeviceState *cxl_dstate)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 93b26e717c94..80e9cb9a8f04 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1526,7 +1526,8 @@ static void bg_timercb(void *opaque)
}
}
-void cxl_initialize_mailbox(CXLDeviceState *cxl_dstate, bool switch_cci)
+void cxl_initialize_mailbox(CXLDeviceState *cxl_dstate, bool switch_cci,
+bool is_dcd)
{
if (!switch_cci) {
cxl_dstate->cxl_cmd_set = cxl_cmd_set;
@@ -1534,6 +1535,9 @@ void cxl_initialize_mailbox(CXLDeviceState *cxl_dstate,
bool switch_cci)
cxl_dstate->cxl_cmd_set = cxl_cmd_set_sw;
}
for (int set = 0; set < 256; set++) {
+if (!is_dcd && set == DCD_CONFIG) {
+continue;
+}
for (int cmd = 0; cmd < 256; cmd++) {
if (cxl_dstate->cxl_cmd_set[set][cmd].handler) {
struct cxl_cmd *c = _dstate->cxl_cmd_set[set][cmd];
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 329e8b5915b3..e6e6e125990c 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1276,9 +1276,11 @@ static void ct3d_reset(DeviceState *dev)
CXLType3Dev *ct3d = CXL_TYPE3(dev);
uint32_t *reg_state = ct3d->cxl_cstate.crb.cache_mem_registers;
uint32_t *write_msk = ct3d->cxl_cstate.crb.cache_mem_regs_write_mask;
+bool is_dcd;
cxl_component_register_init_common(reg_state, write_msk,
CXL2_TYPE3_DEVICE);
-cxl_device_register_init_common(>cxl_dstate);
+is_dcd = (ct3d->dc.num_regions != 0);
+cxl_device_register_init_common(>cxl_dstate, is_dcd);
}
static Property ct3_props[] = {
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 1ccddcca7d0d..4621bba4f533 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -233,7 +233,7 @@ typedef struct cxl_device_state {
void cxl_device_register_block_init(Object *obj, CXLDeviceState *dev);
/* Set up default values for the register block */
-void cxl_device_register_init_common(CXLDeviceState *dev);
+void cxl_device_register_init_common(CXLDeviceState *dev, bool
Re: [Qemu RFC 0/7] Early enabling of DCD emulation in Qemu
Shesha Bhushan Sreenivasamurthy wrote: > Hi Fan, >I am implementing DCD FMAPI commands and planning to start pushing changes > to the below branch. That requires the contributions you have made. Can your > changes be pushed to the below branch ? > > https://gitlab.com/jic23/qemu/-/tree/cxl-2023-05-25 This is the branch I'm trying to use as well. Thanks, Ira
