| Message ID | 20240919104238.232704-4-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: dt overlay fixes | expand |
On Thu, 19 Sep 2024, Michal Orzel wrote: > The assumption stated in the comment that the code will never get there > is incorrect. It's enough for the target-path to be incorrect (i.e. user > error), which will lead to an incorrect overall node path and we will end > up in this "unreachable" place causing a failure in debug builds. > > Fixes: 0c0facdab6f5 ("xen/arm: Implement device tree node addition functionalities") > Signed-off-by: Michal Orzel <michal.orzel@amd.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/common/dt-overlay.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c > index 8606b14d1e8e..d18bd12bd38d 100644 > --- a/xen/common/dt-overlay.c > +++ b/xen/common/dt-overlay.c > @@ -596,11 +596,7 @@ static long add_nodes(struct overlay_track *tr, char **nodes_full_path) > overlay_node = dt_find_node_by_path_from(tr->dt_host_new, > nodes_full_path[j]); > if ( overlay_node == NULL ) > - { > - /* Sanity check. But code will never come here. */ > - ASSERT_UNREACHABLE(); > return -EFAULT; > - } > > /* > * Find previous and next node to overlay_node in dt_host_new. We will > -- > 2.37.6 >
Hi Michal, On 19/09/2024 12:42, Michal Orzel wrote: > The assumption stated in the comment that the code will never get there > is incorrect. It's enough for the target-path to be incorrect (i.e. user > error), which will lead to an incorrect overall node path and we will end > up in this "unreachable" place causing a failure in debug builds. Looking at the caller, nodes_full_path contain path that was computed from the overlay. So I would have assumed the path would be part of the new DT. What did I miss? > > Fixes: 0c0facdab6f5 ("xen/arm: Implement device tree node addition functionalities") > Signed-off-by: Michal Orzel <michal.orzel@amd.com> > --- > xen/common/dt-overlay.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c > index 8606b14d1e8e..d18bd12bd38d 100644 > --- a/xen/common/dt-overlay.c > +++ b/xen/common/dt-overlay.c > @@ -596,11 +596,7 @@ static long add_nodes(struct overlay_track *tr, char **nodes_full_path) > overlay_node = dt_find_node_by_path_from(tr->dt_host_new, > nodes_full_path[j]); > if ( overlay_node == NULL ) > - { > - /* Sanity check. But code will never come here. */ > - ASSERT_UNREACHABLE(); > return -EFAULT; > - } > > /* > * Find previous and next node to overlay_node in dt_host_new. We will Cheers,
Hi Julien, On 20/09/2024 10:35, Julien Grall wrote: > > > Hi Michal, > > On 19/09/2024 12:42, Michal Orzel wrote: >> The assumption stated in the comment that the code will never get there >> is incorrect. It's enough for the target-path to be incorrect (i.e. user >> error), which will lead to an incorrect overall node path and we will end >> up in this "unreachable" place causing a failure in debug builds. > > Looking at the caller, nodes_full_path contain path that was computed > from the overlay. So I would have assumed the path would be part of the > new DT. What did I miss? nodes_full_path contains paths to nodes by combining the name of the node with the target-path string. It's generated in overlay_get_nodes_info(). It's a simple <target-path> + '/' + <node-path> + '\0'. It does not have any dt logic as for paths. On the other hand libfdt contains advanced logic and can tweak the node path if needed. So for example if the target-path is "////" and node path is "foo@0", libfdt full path will be "/foo@0" (notice how it got rid of excessive slashes) but our nodes_full_path[foo] will be "////foo@0". The only place which can spot this difference is our check in add_nodes(). ~Michal
On 23/09/2024 11:44, Michal Orzel wrote: > Hi Julien, > > On 20/09/2024 10:35, Julien Grall wrote: >> >> >> Hi Michal, >> >> On 19/09/2024 12:42, Michal Orzel wrote: >>> The assumption stated in the comment that the code will never get there >>> is incorrect. It's enough for the target-path to be incorrect (i.e. user >>> error), which will lead to an incorrect overall node path and we will end >>> up in this "unreachable" place causing a failure in debug builds. >> >> Looking at the caller, nodes_full_path contain path that was computed >> from the overlay. So I would have assumed the path would be part of the >> new DT. What did I miss? > nodes_full_path contains paths to nodes by combining the name of the node with the target-path string. > It's generated in overlay_get_nodes_info(). It's a simple <target-path> + '/' + <node-path> + '\0'. It does > not have any dt logic as for paths. On the other hand libfdt contains advanced logic and can tweak the node > path if needed. So for example if the target-path is "////" and node path is "foo@0", libfdt full path will be "/foo@0" > (notice how it got rid of excessive slashes) but our nodes_full_path[foo] will be "////foo@0". The only place which can > spot this difference is our check in add_nodes(). Thanks for the clarification. This reads as we want to get the list of nodes using the libfdt logic rather than re-inventing our own. I would suggest to expand a bit more the commit message with the details you provided. With that: Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c index 8606b14d1e8e..d18bd12bd38d 100644 --- a/xen/common/dt-overlay.c +++ b/xen/common/dt-overlay.c @@ -596,11 +596,7 @@ static long add_nodes(struct overlay_track *tr, char **nodes_full_path) overlay_node = dt_find_node_by_path_from(tr->dt_host_new, nodes_full_path[j]); if ( overlay_node == NULL ) - { - /* Sanity check. But code will never come here. */ - ASSERT_UNREACHABLE(); return -EFAULT; - } /* * Find previous and next node to overlay_node in dt_host_new. We will
The assumption stated in the comment that the code will never get there is incorrect. It's enough for the target-path to be incorrect (i.e. user error), which will lead to an incorrect overall node path and we will end up in this "unreachable" place causing a failure in debug builds. Fixes: 0c0facdab6f5 ("xen/arm: Implement device tree node addition functionalities") Signed-off-by: Michal Orzel <michal.orzel@amd.com> --- xen/common/dt-overlay.c | 4 ---- 1 file changed, 4 deletions(-)