Re: [PATCH] media: vsp1: tidyup VI6_HGT_LBn_H() macro

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Kieran,

On Fri, Dec 13, 2019 at 12:55 PM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
> On 12/12/2019 07:33, Greg Kroah-Hartman wrote:
> > On Wed, Dec 11, 2019 at 09:58:11PM +0000, Kieran Bingham wrote:
> >> +Greg, +Sasha to opine on the merit of whether this should go to stable
> >> trees (for my future learning and understanding more so than this
> >> specific case)
> >>
> >> On 11/12/2019 17:58, Laurent Pinchart wrote:
> >>> On Wed, Dec 11, 2019 at 12:59:57PM +0000, Kieran Bingham wrote:
> >>>> On 11/12/2019 01:55, Kuninori Morimoto wrote:
> >>>>>
> >>>>> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >>>>>
> >>>>> The address of VSP2_VI6_HGT_LBx_H are
> >>>>>   VSP2_VI6_HGT_LB0_H : 0x3428
> >>>>>   VSP2_VI6_HGT_LB1_H : 0x3430
> >>>>>   VSP2_VI6_HGT_LB2_H : 0x3438
> >>>>>   VSP2_VI6_HGT_LB3_H : 0x3440
> >>>>>
> >>>>> Thus, VI6_HGT_LBn_H() macro should start from 0x3420 instead of 0x3430.
> >>>>> This patch fixup it.
> >>
> >> s/fixup/fixes/
> >>
> >>
> >>>> I think this deserves a fixes tag:
> >>>>
> >>>> Fixes: 26e0ca22c3b8 ("[media] v4l: Renesas R-Car VSP1 driver")
> >>>
> >>> Given that this macro is not used, we could argue that it doesn't fix
> >>> anything yet :-) I'd rather avoid having this backported to stable
> >>> kernels as it's not useful to have it there, and thus not add a Fixes
> >>
> >> I'm sorry - I'm not sure I can agree here, Do you know that no one will
> >> use this macro when they back port the HGT functionality to an LTSI kernel?
> >>
> >> We know the Renesas BSP uses LTSI kernels, and the very nature of the
> >> fact that this typo has been spotted by the Renesas BSP team suggests
> >> that they are indeed looking at/using this functionality ...
> >>
> >> (Ok, so maybe they will thus apply the fix themselves, but that's not my
> >> point, and if they 'have' to apply the fix - it should be in stable?)
> >>
> >> It feels a bit presumptuous to state that we shouldn't fix this because
> >> /we/ don't utilise it yet, when this issue is in mainline regardless ...
> >
> > Nothing should be in the kernel tree that is not already used by
> > something in that specific kernel tree.  We don't care about out-of-tree
> > code, and especially for stable kernel patches, it does not matter in
> > the least.
>
> So perhaps this patch should actually remove this macro rather than fix it?

IMHO removing all unused register and register bit definitions from drivers
would not improve them. On the contrary.
These also serve as a kind of documentation, as low-level documentation
about the hardware is not always available publicly.

> > If you have out-of-tree code, you are on your own here, sorry.
> >
> > So no, no backporting of stuff that no one actually uses in the codebase
> > itself.
>
> Ok understood, It was really the 'the macro exists in the kernel, but is
> wrong' part that got me.

There is a difference between code that is not used, and register definitions.

> Along with the fact that we now have various automated machinery that
> would likely pick this patch up and backport it anyway?
>
> (Sasha, is that assumption accurate? Or would you/your system have
> identified that this macro is not used?)

IMHO the real danger lies in not backporting this, and forgetting to
backport this fix when future code that does use this definition is
backported.
This may happen +5 years from now, and the feature may be backported to
a 10-year old LTS(i) kernel, causing subtle issues in your 8 year old car...

Basically we can do 4 things:
  1. Fix the buggy definition, and not backport the fix,
      => may trigger the issue above.
  2. idem, but backport the fix,
  3. Remove the buggy definition, and not backport the removal,
      => may trigger the issue above, if people miss-backport the re-addition.
      => people may just revert the removal when they need the definition.
  4. idem, but backport the removal.
      => same as 3.

So I am in favor of fixing the buggy definition, with a Fixes tag, so
stable will pick it up, and anyone else (if any not using LTS) can look
for Fixes tags and know what fixes to backport.

Of course, all of this can be avoided by using the One Way Of Linux ;-)
https://society.oftrolls.com/@geert/102984898449908163

Just my 2€c.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds