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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello,

On Fri, Dec 13, 2019 at 02:22:56PM +0100, Geert Uytterhoeven wrote:
> On Fri, Dec 13, 2019 at 12:55 PM Kieran Bingham 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.

You 2¢ resulted in the Fixed: line being added to the patch in my tree
:-)

-- 
Regards,

Laurent Pinchart