| Message ID | 20230513113137.97093-2-npiggin@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] mem_region: Add a local_free function | expand |
> On May 13, 2023, at 04:31, Nicholas Piggin <npiggin@gmail.com> wrote: > > free() asserts because local_alloc() doesn't allocate from the skiboot > heap region. Fix this by using local_free(). Well, there’s something I probably didn’t catch in review a while ago :) It may be possible to have valgrind pick up some of these in the unit tests too with some improved annotations and possible wrapping, as it can do warnings that the right kind of alloc/free was called. Although IIRC for *most* of the unit tests we end up not bringing in the skiboot allocator for ease of not going completely up the wall with looking too much like normal libc. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > hw/vas.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/vas.c b/hw/vas.c > index 96ca055c..aa3ae334 100644 > --- a/hw/vas.c > +++ b/hw/vas.c > @@ -428,7 +428,7 @@ static int init_uwcm(struct proc_chip *chip) > static inline void free_wcbs(struct proc_chip *chip) > { > if (chip->vas->wcbs) { > - free((void *)chip->vas->wcbs); > + local_free((void *)chip->vas->wcbs); > chip->vas->wcbs = 0ULL; > } > } > @@ -466,7 +466,7 @@ static int alloc_init_wcbs(struct proc_chip *chip) > return OPAL_SUCCESS; > > out: > - free((void *)wcbs); > + local_free((void *)wcbs); > return rc; > } > > -- > 2.40.1 > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
On Mon May 15, 2023 at 6:32 AM AEST, Stewart Smith wrote: > > > On May 13, 2023, at 04:31, Nicholas Piggin <npiggin@gmail.com> wrote: > > > > free() asserts because local_alloc() doesn't allocate from the skiboot > > heap region. Fix this by using local_free(). > > Well, there’s something I probably didn’t catch in review a while ago :) > > It may be possible to have valgrind pick up some of these in the unit tests too with some improved annotations and possible wrapping, as it can do warnings that the right kind of alloc/free was called. Although IIRC for *most* of the unit tests we end up not bringing in the skiboot allocator for ease of not going completely up the wall with looking too much like normal libc. The asserts in the allocator picked it up at least so if we had a test case that hit it it would explode. Trouble is the error path doesn't happen without some hardware or hostboot issue. I was mucking with HDAT (so I guess I deserve whatever I get). Thanks, Nick
diff --git a/hw/vas.c b/hw/vas.c index 96ca055c..aa3ae334 100644 --- a/hw/vas.c +++ b/hw/vas.c @@ -428,7 +428,7 @@ static int init_uwcm(struct proc_chip *chip) static inline void free_wcbs(struct proc_chip *chip) { if (chip->vas->wcbs) { - free((void *)chip->vas->wcbs); + local_free((void *)chip->vas->wcbs); chip->vas->wcbs = 0ULL; } } @@ -466,7 +466,7 @@ static int alloc_init_wcbs(struct proc_chip *chip) return OPAL_SUCCESS; out: - free((void *)wcbs); + local_free((void *)wcbs); return rc; }
free() asserts because local_alloc() doesn't allocate from the skiboot heap region. Fix this by using local_free(). Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- hw/vas.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)