| Message ID | 20221207061815.7404-5-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
Hi Vikram, On 07/12/2022 06:18, Vikram Garhwal wrote: > Dynamic programming ops will modify the dt_host and there might be other > function which are browsing the dt_host at the same time. To avoid the race > conditions, adding rwlock for browsing the dt_host. Looking at the user below, it is not entirely clear what the lock is actually protecting. For instance... > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > --- > xen/common/device_tree.c | 27 +++++++++++++++++++++++++++ > xen/include/xen/device_tree.h | 6 ++++++ > 2 files changed, 33 insertions(+) > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index acf26a411d..51ee2a5edf 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -140,6 +140,8 @@ const struct dt_property *dt_find_property(const struct dt_device_node *np, > if ( !np ) > return NULL; > > + read_lock(&dt_host->lock); > + > for ( pp = np->properties; pp; pp = pp->next ) > { > if ( dt_prop_cmp(pp->name, name) == 0 ) > @@ -150,6 +152,7 @@ const struct dt_property *dt_find_property(const struct dt_device_node *np, > } > } > > + read_unlock(&dt_host->lock); > return pp; > } > > @@ -336,11 +339,14 @@ struct dt_device_node *dt_find_node_by_name(struct dt_device_node *from, > struct dt_device_node *np; > struct dt_device_node *dt; > > + read_lock(&dt_host->lock); > + > dt = from ? from->allnext : dt_host; > dt_for_each_device_node(dt, np) > if ( np->name && (dt_node_cmp(np->name, name) == 0) ) > break; > > + read_unlock(&dt_host->lock); > return np; ... I was expecting the read lock to also protect the value returned from being freed. But this is not the case. Cheers,
Hi Julien, On 12/7/22 8:31 AM, Julien Grall wrote: > Hi Vikram, > > On 07/12/2022 06:18, Vikram Garhwal wrote: >> Dynamic programming ops will modify the dt_host and there might be >> other >> function which are browsing the dt_host at the same time. To avoid >> the race >> conditions, adding rwlock for browsing the dt_host. > > Looking at the user below, it is not entirely clear what the lock is > actually protecting. For instance... Purpose of the lock was to protect the read/scanning of dt_host while we remove the add/nodes. This lock is also used when nodes are added/removed in "[XEN][RFC PATCH v4 12/16]: static int remove_nodes(const struct overlay_track *tracker)". > >> >> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> >> --- >> xen/common/device_tree.c | 27 +++++++++++++++++++++++++++ >> xen/include/xen/device_tree.h | 6 ++++++ >> 2 files changed, 33 insertions(+) >> >> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c >> index acf26a411d..51ee2a5edf 100644 >> --- a/xen/common/device_tree.c >> +++ b/xen/common/device_tree.c >> @@ -140,6 +140,8 @@ const struct dt_property *dt_find_property(const >> struct dt_device_node *np, >> if ( !np ) >> return NULL; >> + read_lock(&dt_host->lock); >> + >> for ( pp = np->properties; pp; pp = pp->next ) >> { >> if ( dt_prop_cmp(pp->name, name) == 0 ) >> @@ -150,6 +152,7 @@ const struct dt_property *dt_find_property(const >> struct dt_device_node *np, >> } >> } >> + read_unlock(&dt_host->lock); >> return pp; >> } >> @@ -336,11 +339,14 @@ struct dt_device_node >> *dt_find_node_by_name(struct dt_device_node *from, >> struct dt_device_node *np; >> struct dt_device_node *dt; >> + read_lock(&dt_host->lock); >> + >> dt = from ? from->allnext : dt_host; >> dt_for_each_device_node(dt, np) >> if ( np->name && (dt_node_cmp(np->name, name) == 0) ) >> break; >> + read_unlock(&dt_host->lock); >> return np; > > ... I was expecting the read lock to also protect the value returned > from being freed. But this is not the case. > Okay. Shall I remove the lock from here and perhaps add it when dt_find_node_by_name() and other related functions are called? > Cheers, >
Hi, On 01/02/2023 17:05, Vikram Garhwal wrote: > On 12/7/22 8:31 AM, Julien Grall wrote: >>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> >>> --- >>> xen/common/device_tree.c | 27 +++++++++++++++++++++++++++ >>> xen/include/xen/device_tree.h | 6 ++++++ >>> 2 files changed, 33 insertions(+) >>> >>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c >>> index acf26a411d..51ee2a5edf 100644 >>> --- a/xen/common/device_tree.c >>> +++ b/xen/common/device_tree.c >>> @@ -140,6 +140,8 @@ const struct dt_property *dt_find_property(const >>> struct dt_device_node *np, >>> if ( !np ) >>> return NULL; >>> + read_lock(&dt_host->lock); >>> + >>> for ( pp = np->properties; pp; pp = pp->next ) >>> { >>> if ( dt_prop_cmp(pp->name, name) == 0 ) >>> @@ -150,6 +152,7 @@ const struct dt_property *dt_find_property(const >>> struct dt_device_node *np, >>> } >>> } >>> + read_unlock(&dt_host->lock); >>> return pp; >>> } >>> @@ -336,11 +339,14 @@ struct dt_device_node >>> *dt_find_node_by_name(struct dt_device_node *from, >>> struct dt_device_node *np; >>> struct dt_device_node *dt; >>> + read_lock(&dt_host->lock); >>> + >>> dt = from ? from->allnext : dt_host; >>> dt_for_each_device_node(dt, np) >>> if ( np->name && (dt_node_cmp(np->name, name) == 0) ) >>> break; >>> + read_unlock(&dt_host->lock); >>> return np; >> >> ... I was expecting the read lock to also protect the value returned >> from being freed. But this is not the case. >> > Okay. Shall I remove the lock from here and perhaps add it when > dt_find_node_by_name() and other related functions are called? It is a possibility. I would need to see the end result to be sure a lock is actually suitable. Maybe we will end up to need a refcounting instead if we keep the DT node for longer. Cheers,
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index acf26a411d..51ee2a5edf 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -140,6 +140,8 @@ const struct dt_property *dt_find_property(const struct dt_device_node *np, if ( !np ) return NULL; + read_lock(&dt_host->lock); + for ( pp = np->properties; pp; pp = pp->next ) { if ( dt_prop_cmp(pp->name, name) == 0 ) @@ -150,6 +152,7 @@ const struct dt_property *dt_find_property(const struct dt_device_node *np, } } + read_unlock(&dt_host->lock); return pp; } @@ -336,11 +339,14 @@ struct dt_device_node *dt_find_node_by_name(struct dt_device_node *from, struct dt_device_node *np; struct dt_device_node *dt; + read_lock(&dt_host->lock); + dt = from ? from->allnext : dt_host; dt_for_each_device_node(dt, np) if ( np->name && (dt_node_cmp(np->name, name) == 0) ) break; + read_unlock(&dt_host->lock); return np; } @@ -350,11 +356,14 @@ struct dt_device_node *dt_find_node_by_type(struct dt_device_node *from, struct dt_device_node *np; struct dt_device_node *dt; + read_lock(&dt_host->lock); + dt = from ? from->allnext : dt_host; dt_for_each_device_node(dt, np) if ( np->type && (dt_node_cmp(np->type, type) == 0) ) break; + read_unlock(&dt_host->lock); return np; } @@ -363,10 +372,13 @@ struct dt_device_node *device_tree_find_node_by_path(struct dt_device_node *dt, { struct dt_device_node *np; + read_lock(&dt_host->lock); + dt_for_each_device_node(dt, np) if ( np->full_name && (dt_node_cmp(np->full_name, path) == 0) ) break; + read_unlock(&dt_host->lock); return np; } @@ -450,6 +462,8 @@ dt_find_compatible_node(struct dt_device_node *from, struct dt_device_node *np; struct dt_device_node *dt; + read_lock(&dt_host->lock); + dt = from ? from->allnext : dt_host; dt_for_each_device_node(dt, np) { @@ -460,6 +474,7 @@ dt_find_compatible_node(struct dt_device_node *from, break; } + read_unlock(&dt_host->lock); return np; } @@ -470,13 +485,19 @@ dt_find_matching_node(struct dt_device_node *from, struct dt_device_node *np; struct dt_device_node *dt; + read_lock(&dt_host->lock); + dt = from ? from->allnext : dt_host; dt_for_each_device_node(dt, np) { if ( dt_match_node(matches, np) ) + { + read_unlock(&dt_host->lock); return np; + } } + read_unlock(&dt_host->lock); return NULL; } @@ -1052,10 +1073,13 @@ struct dt_device_node *dt_find_node_by_phandle(dt_phandle handle) { struct dt_device_node *np; + read_lock(&dt_host->lock); + dt_for_each_device_node(dt_host, np) if ( np->phandle == handle ) break; + read_unlock(&dt_host->lock); return np; } @@ -2102,6 +2126,9 @@ int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes) dt_dprintk(" <- unflatten_device_tree()\n"); + /* Init r/w lock for host device tree. */ + rwlock_init(&dt_host->lock); + return 0; } diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h index 51e251b0b4..bafc898f1c 100644 --- a/xen/include/xen/device_tree.h +++ b/xen/include/xen/device_tree.h @@ -18,6 +18,7 @@ #include <xen/string.h> #include <xen/types.h> #include <xen/list.h> +#include <xen/rwlock.h> #define DEVICE_TREE_MAX_DEPTH 16 @@ -106,6 +107,11 @@ struct dt_device_node { struct list_head domain_list; struct device dev; + + /* + * Lock that protects r/w updates to unflattened device tree i.e. dt_host. + */ + rwlock_t lock; }; #define dt_to_dev(dt_node) (&(dt_node)->dev)
Dynamic programming ops will modify the dt_host and there might be other function which are browsing the dt_host at the same time. To avoid the race conditions, adding rwlock for browsing the dt_host. Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> --- xen/common/device_tree.c | 27 +++++++++++++++++++++++++++ xen/include/xen/device_tree.h | 6 ++++++ 2 files changed, 33 insertions(+)