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

[Eric Engestrom <eric.engestrom@imgtec.com>]

On Friday, 2016-11-04 13:12:51 -0400, Rob Clark wrote:
> On Fri, Nov 4, 2016 at 12:27 PM, Ville Syrjälä
> <ville.syrjala@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@engestrom.ch>
> >>     [danvet: Clarify that the returned pointer must be freed with
> >>     kfree().]
> >>     Signed-off-by: Daniel Vetter <daniel.vetter@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@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;

Unrelated bug, thanks for that fix too :)

> >>
> >>       snprintf(buf, 32,
> >>                "%c%c%c%c %s-endian (0x%08x)",
> >
> > Why aren't we using kasprintf()?

Because I didn't know it was a thing :(
Patch using it incoming.

> >
> > 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.

That sounds cleaner to me indeed, I'll send a patch doing that tonight.
Any idea for the name? drm_get_format_name_safe(uint32_t, char*)?
I could also keep the same name and rely on the function signature
change to make sure any merging of the old function call will break the
compilation and be noticed that way.

Looks like I'm obviously not qualified to choose which is the best
option, so I'll leave that to you guys :)

Cheers,
  Eric
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel