[PATCH] drm: make drm_get_format_name atomic/irq safe again

[Rob Clark]

On Fri, Nov 4, 2016 at 12:27 PM, Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
> On Fri, Nov 04, 2016 at 11:32:45AM -0400, Rob Clark wrote:
>> Fixes: 90844f00049e9f42573fd31d7c32e8fd31d3fd07
>>
>>     drm: make drm_get_format_name thread-safe
>>
>>     Signed-off-by: Eric Engestrom <eric at engestrom.ch>
>>     [danvet: Clarify that the returned pointer must be freed with
>>     kfree().]
>>     Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>>
>> Note: I think we need to be a bit careful about follow-up audits of
>> callers of this..  now that you need to kfree the return value I think
>> it is fairly easy for new patches to introduce new callers which leak
>> the return value.  We probably should have left the function as-is and
>> introduce a new variant, or something like that.
>>
>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>> ---
>>  drivers/gpu/drm/drm_fourcc.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
>> index cbb8b77..2be9ea8 100644
>> --- a/drivers/gpu/drm/drm_fourcc.c
>> +++ b/drivers/gpu/drm/drm_fourcc.c
>> @@ -87,7 +87,10 @@ EXPORT_SYMBOL(drm_mode_legacy_fb_format);
>>   */
>>  char *drm_get_format_name(uint32_t format)
>>  {
>> -     char *buf = kmalloc(32, GFP_KERNEL);
>> +     char *buf = kmalloc(32, GFP_ATOMIC);
>> +
>> +     if (!buf)
>> +             return NULL;
>>
>>       snprintf(buf, 32,
>>                "%c%c%c%c %s-endian (0x%08x)",
>
> Why aren't we using kasprintf()?
>
> Or we could have just made the caller provide the buffer...

I guess kasprintf() would do the job (although still not address the
fact that we still always do the sprintf bits, rather than only doing
it when debug logs are enabled)..  caller provided buffer would be
better.  And make it more obvious that something needs to be fixed
when merging/rebasing patches that started life before this change
landed.

I still kinda like the idea of making vsprintf know about fourcc's
with a new format string, and just making drm_get_format_name() go
away.

But I don't really have time atm to re-work all the callers of
drm_get_format_name().  So I guess unless someone else does, I'd go w/
kasprintf() for now.

BR,
-R

>> --
>> 2.7.4
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC