| Message ID | 20190111180523.27862-12-afd@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc ION cleanups and adding unmapped heap | expand |
Hi Andrew, On Fri, Jan 11, 2019 at 12:05:20PM -0600, Andrew F. Davis wrote: > The heap name can be used for debugging but otherwise does not seem > to be required and no other part of the code will fail if left NULL > except here. We can make it required and check for it at some point, > for now lets just prevent this from causing a NULL pointer exception. I'm not so keen on this one. In the "new" API with heap querying, the name string is the only way to identify the heap. I think Laura mentioned at XDC2017 that it was expected that userspace should use the strings to find the heap they want. I'd actually be in favor of making the string a more strict UAPI than allowing it to be empty (at least, if heap name strings is the API we decide on for identifying heaps - which is another discussion). Cheers, -Brian > > Signed-off-by: Andrew F. Davis <afd@ti.com> > --- > drivers/staging/android/ion/ion.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c > index bba5f682bc25..14e48f6eb734 100644 > --- a/drivers/staging/android/ion/ion.c > +++ b/drivers/staging/android/ion/ion.c > @@ -467,7 +467,7 @@ static int ion_query_heaps(struct ion_heap_query *query) > max_cnt = query->cnt; > > plist_for_each_entry(heap, &dev->heaps, node) { > - strncpy(hdata.name, heap->name, MAX_HEAP_NAME); > + strncpy(hdata.name, heap->name ?: "(null)", MAX_HEAP_NAME); > hdata.name[sizeof(hdata.name) - 1] = '\0'; > hdata.type = heap->type; > hdata.heap_id = heap->id; > -- > 2.19.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 1/16/19 9:28 AM, Brian Starkey wrote: > Hi Andrew, > > On Fri, Jan 11, 2019 at 12:05:20PM -0600, Andrew F. Davis wrote: >> The heap name can be used for debugging but otherwise does not seem >> to be required and no other part of the code will fail if left NULL >> except here. We can make it required and check for it at some point, >> for now lets just prevent this from causing a NULL pointer exception. > > I'm not so keen on this one. In the "new" API with heap querying, the > name string is the only way to identify the heap. I think Laura > mentioned at XDC2017 that it was expected that userspace should use > the strings to find the heap they want. > Right now the names are only for debug. I accidentally left the name null once and got a kernel crash. This is the only spot where it is needed so I fixed it up. The other option is to make the name mandatory and properly error out, I don't want to do that right now until the below discussion is had to see if names really do matter or not. > I'd actually be in favor of making the string a more strict UAPI than > allowing it to be empty (at least, if heap name strings is the API we > decide on for identifying heaps - which is another discussion). > I would think identifying heaps by name would be less portable, but as you said that is a whole different discussion.. Thanks, Andrew > Cheers, > -Brian > >> >> Signed-off-by: Andrew F. Davis <afd@ti.com> >> --- >> drivers/staging/android/ion/ion.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c >> index bba5f682bc25..14e48f6eb734 100644 >> --- a/drivers/staging/android/ion/ion.c >> +++ b/drivers/staging/android/ion/ion.c >> @@ -467,7 +467,7 @@ static int ion_query_heaps(struct ion_heap_query *query) >> max_cnt = query->cnt; >> >> plist_for_each_entry(heap, &dev->heaps, node) { >> - strncpy(hdata.name, heap->name, MAX_HEAP_NAME); >> + strncpy(hdata.name, heap->name ?: "(null)", MAX_HEAP_NAME); >> hdata.name[sizeof(hdata.name) - 1] = '\0'; >> hdata.type = heap->type; >> hdata.heap_id = heap->id; >> -- >> 2.19.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 1/16/19 9:12 AM, Andrew F. Davis wrote: > On 1/16/19 9:28 AM, Brian Starkey wrote: >> Hi Andrew, >> >> On Fri, Jan 11, 2019 at 12:05:20PM -0600, Andrew F. Davis wrote: >>> The heap name can be used for debugging but otherwise does not seem >>> to be required and no other part of the code will fail if left NULL >>> except here. We can make it required and check for it at some point, >>> for now lets just prevent this from causing a NULL pointer exception. >> >> I'm not so keen on this one. In the "new" API with heap querying, the >> name string is the only way to identify the heap. I think Laura >> mentioned at XDC2017 that it was expected that userspace should use >> the strings to find the heap they want. >> > > Right now the names are only for debug. I accidentally left the name > null once and got a kernel crash. This is the only spot where it is > needed so I fixed it up. The other option is to make the name mandatory > and properly error out, I don't want to do that right now until the > below discussion is had to see if names really do matter or not. > Yes, the heap names are part of the query API and are the expected way to identify individual heaps for the API at the moment so having a null heap name is incorrect. The heap name seemed like the best way to identify heaps to userspace but if you have an alternative proposal I'd be interested. Thanks, Laura >
On 1/18/19 1:53 PM, Laura Abbott wrote: > On 1/16/19 9:12 AM, Andrew F. Davis wrote: >> On 1/16/19 9:28 AM, Brian Starkey wrote: >>> Hi Andrew, >>> >>> On Fri, Jan 11, 2019 at 12:05:20PM -0600, Andrew F. Davis wrote: >>>> The heap name can be used for debugging but otherwise does not seem >>>> to be required and no other part of the code will fail if left NULL >>>> except here. We can make it required and check for it at some point, >>>> for now lets just prevent this from causing a NULL pointer exception. >>> >>> I'm not so keen on this one. In the "new" API with heap querying, the >>> name string is the only way to identify the heap. I think Laura >>> mentioned at XDC2017 that it was expected that userspace should use >>> the strings to find the heap they want. >>> >> >> Right now the names are only for debug. I accidentally left the name >> null once and got a kernel crash. This is the only spot where it is >> needed so I fixed it up. The other option is to make the name mandatory >> and properly error out, I don't want to do that right now until the >> below discussion is had to see if names really do matter or not. >> > > Yes, the heap names are part of the query API and are the expected > way to identify individual heaps for the API at the moment so having > a null heap name is incorrect. The heap name seemed like the best way > to identify heaps to userspace but if you have an alternative proposal > I'd be interested. > Not sure I have a better proposal right now, I'll re-work this patch to force heap names to be populated before ion_device_add_heap() instead. (do you think that function name is now is a misnomer? how do you feel about renaming that to just ion_add_heap()?) Andrew > Thanks, > Laura > >> > >
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index bba5f682bc25..14e48f6eb734 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -467,7 +467,7 @@ static int ion_query_heaps(struct ion_heap_query *query) max_cnt = query->cnt; plist_for_each_entry(heap, &dev->heaps, node) { - strncpy(hdata.name, heap->name, MAX_HEAP_NAME); + strncpy(hdata.name, heap->name ?: "(null)", MAX_HEAP_NAME); hdata.name[sizeof(hdata.name) - 1] = '\0'; hdata.type = heap->type; hdata.heap_id = heap->id;
The heap name can be used for debugging but otherwise does not seem to be required and no other part of the code will fail if left NULL except here. We can make it required and check for it at some point, for now lets just prevent this from causing a NULL pointer exception. Signed-off-by: Andrew F. Davis <afd@ti.com> --- drivers/staging/android/ion/ion.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)