Re: Build fail in drivers/gpu/drm/i915/display/intel_tc.c

[Imre Deak]

On Fri, Nov 10, 2023 at 09:00:21AM +0000, David Laight wrote:
> From: Linus Torvalds
> > Sent: 10 November 2023 00:52
> > 
> > On Thu, 9 Nov 2023 at 15:34, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> > >
> > > The compiler warn should be fixed/suppressed by:
> > > https://lore.kernel.org/all/20231026125636.5080-1-nirmoy.das@xxxxxxxxx
> > 
> > Ugh, so now it's a dynamic allocation, wasting memory, and a pointer
> > to it, using as much memory as the array did in the first place.
> > 
> > All because of a pointless warning that was a false positive - and was
> > always harmless anyway, since snprintf() is safe (ie it was only a
> > "might be truncated").
> 
> That entire warning for snprintf() is a false positive.
> The ones that are likely to overflow unexpectedly are the ones
> with a "%s" format for a 'char *' pointer where there is no
> implied length.
> 
> The same check for printf() using the implied buffer length
> probably does make sense.
> 
> I don't even think there is a way of avoiding it on a case by case
> basis - apart from passing both the buffer address and length
> to an inline asm that the compiler has to assume might change
> the values, but that tends to generate an extra 'mov' instruction
> for no good reason at all.
> 
> > 
> > Please don't do this. Either do that ((tc_port & 7) + 1) suggestion of
> > David's, or just do '%c' and make the expression be
> > 
> >   '1' + tc_port
> > 
> > which should be fine since I915_MAX_PORTS is 8 or whatever.

Ok, the patch above was merged already to drm-tip, but I agree not to
use kasprintf for this. The above looks ok and there is already
tc_port_name() for this, would just need to export that from
intel_display.h.

I can follow up with a patch for this, or if you or David wanted to do
that please send a patch to the intel-gfx@xxxxxxxxxxxxxxxxxxxxx list.

Thanks,
Imre

> If I'd though for 2 seconds that is what I'd have done.
> But I wanted to get something through the compiler.
> 
> > I do wonder why those ports are printed out as '1-8', when the 'enum
> > port' is PORT_A..I.
> 
> They look like TC_PORT_[1..6] to me - the enum values are 0..5
> which is why there is a 'random' '+ 1'.
> 
> 	David
> 
> > 
> > So it would actually have made more sense to print them out as %c with
> > the expression being
> > 
> >    'A'+tc_port
> > 
> > but I guess by now people might depend on the 1..8 naming?
> > 
> >              Linus
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)