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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 04, 2016 at 01:50:25PM -0400, Rob Clark wrote:
> On Fri, Nov 4, 2016 at 1:32 PM, Eric Engestrom
> <eric.engestrom@imgtec.com> wrote:
> > 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.
> 
> Thanks
> 
> >> >
> >> > 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.
> 
> I think if fxn signature changes, we can keep the same name.  What I
> get nervous about is fxn sig+name stays same but semantics change ;-)
> 
> btw, if this isn't what you were already planning, I'd suggest:
> 
>    char *drm_get_format_name(uint32_t, char *);

I would model it after snprintf(): drm_get_format_name(buf,size,format)

Although I guess you could have it return the string instead of
int for extra ease of use.

But yeah, having the ability to register custom printf conversions would
be pretty nice.

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