| Message ID | 1524763162-4865-8-git-send-email-brad@nextdimension.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Brad, On 04/26/18 19:19, Brad Love wrote: > config-mycompat.h is for overriding macros which are incorrectly > enabled on certain kernels by the build system. The file should be > left empty, unless build errors are encountered for a kernel. The > file is removed by distclean, therefore should be externally > sourced, before the build process starts, when required. > > In standard operation the file is empty, but if a particular kernel has > incorrectly enabled options defined this allows them to be undefined. Can you give an example where this will be used? FYI: I've committed patches 1-6, but I don't quite understand when this patch is needed. With "for overriding macros which are incorrectly enabled on certain kernels" do you mean when distros do backports of features from later kernels? Regards, Hans > > Signed-off-by: Brad Love <brad@nextdimension.cc> > --- > v4l/Makefile | 3 ++- > v4l/compat.h | 7 +++++++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/v4l/Makefile b/v4l/Makefile > index 270a624..ee18d11 100644 > --- a/v4l/Makefile > +++ b/v4l/Makefile > @@ -273,6 +273,7 @@ links:: > @find ../linux/drivers/misc -name '*.[ch]' -type f -print0 | xargs -0n 255 ln -sf --target-directory=. > > config-compat.h:: $(obj)/.version .myconfig scripts/make_config_compat.pl > + -touch $(obj)/config-mycompat.h > perl scripts/make_config_compat.pl $(SRCDIR) $(obj)/.myconfig $(obj)/config-compat.h > > kernel-links makelinks:: > @@ -298,7 +299,7 @@ clean:: > distclean:: clean > -rm -f .version .*.o.flags .*.o.d *.mod.gcno Makefile.media \ > Kconfig Kconfig.kern .config .config.cmd .myconfig \ > - .kconfig.dep > + .kconfig.dep config-mycompat.h > -rm -rf .tmp_versions .tmp*.ver .tmp*.o .*.gcno .cache.mk > -rm -f scripts/lxdialog scripts/kconfig > @find .. -name '*.orig' -exec rm '{}' \; > diff --git a/v4l/compat.h b/v4l/compat.h > index 87ce401..db48fdf 100644 > --- a/v4l/compat.h > +++ b/v4l/compat.h > @@ -8,6 +8,13 @@ > #include <linux/version.h> > > #include "config-compat.h" > +/* config-mycompat.h is for overriding #defines which > + * are incorrectly enabled on certain kernels. The file > + * should be left empty, unless build errors are encountered > + * for a kernel. The file is removed by distclean, therefore > + * should be externally sourced, before compilation, when required. > + */ > +#include "config-mycompat.h" > > #ifndef SZ_512 > #define SZ_512 0x00000200 >
Hi Hans, On 2018-05-11 09:41, Hans Verkuil wrote: > Hi Brad, > > On 04/26/18 19:19, Brad Love wrote: >> config-mycompat.h is for overriding macros which are incorrectly >> enabled on certain kernels by the build system. The file should be >> left empty, unless build errors are encountered for a kernel. The >> file is removed by distclean, therefore should be externally >> sourced, before the build process starts, when required. >> >> In standard operation the file is empty, but if a particular kernel has >> incorrectly enabled options defined this allows them to be undefined. > Can you give an example where this will be used? > > FYI: I've committed patches 1-6, but I don't quite understand when this patch > is needed. > > With "for overriding macros which are incorrectly enabled on certain kernels" > do you mean when distros do backports of features from later kernels? > > Regards, > > Hans Apologies if I was not very clear. Yes, this is for use in kernels/distros whose maintainers have integrated various backports, and which the media_build system does not pick up on for whatever reason. At that point there are options defined in config-compat.h, which enable backports in compat.h, but which already exist in the target kernel. For example, on the device I'm working on right now, in kernel 3.10, I have to supply the following three options in config-mycompat.h or modify the tree and stuff them right into the top of compat.h: #undef NEED_WRITEL_RELAXED #undef NEED_PM_RUNTIME_GET #undef NEED_PFN_TO_PHYS The above disables those three media_build backports and allows everything to build. It seems there is quite often at least one backport I must disable, and some target kernels require multiple backports disabled. Cheers, Brad > >> Signed-off-by: Brad Love <brad@nextdimension.cc> >> --- >> v4l/Makefile | 3 ++- >> v4l/compat.h | 7 +++++++ >> 2 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/v4l/Makefile b/v4l/Makefile >> index 270a624..ee18d11 100644 >> --- a/v4l/Makefile >> +++ b/v4l/Makefile >> @@ -273,6 +273,7 @@ links:: >> @find ../linux/drivers/misc -name '*.[ch]' -type f -print0 | xargs -0n 255 ln -sf --target-directory=. >> >> config-compat.h:: $(obj)/.version .myconfig scripts/make_config_compat.pl >> + -touch $(obj)/config-mycompat.h >> perl scripts/make_config_compat.pl $(SRCDIR) $(obj)/.myconfig $(obj)/config-compat.h >> >> kernel-links makelinks:: >> @@ -298,7 +299,7 @@ clean:: >> distclean:: clean >> -rm -f .version .*.o.flags .*.o.d *.mod.gcno Makefile.media \ >> Kconfig Kconfig.kern .config .config.cmd .myconfig \ >> - .kconfig.dep >> + .kconfig.dep config-mycompat.h >> -rm -rf .tmp_versions .tmp*.ver .tmp*.o .*.gcno .cache.mk >> -rm -f scripts/lxdialog scripts/kconfig >> @find .. -name '*.orig' -exec rm '{}' \; >> diff --git a/v4l/compat.h b/v4l/compat.h >> index 87ce401..db48fdf 100644 >> --- a/v4l/compat.h >> +++ b/v4l/compat.h >> @@ -8,6 +8,13 @@ >> #include <linux/version.h> >> >> #include "config-compat.h" >> +/* config-mycompat.h is for overriding #defines which >> + * are incorrectly enabled on certain kernels. The file >> + * should be left empty, unless build errors are encountered >> + * for a kernel. The file is removed by distclean, therefore >> + * should be externally sourced, before compilation, when required. >> + */ >> +#include "config-mycompat.h" >> >> #ifndef SZ_512 >> #define SZ_512 0x00000200 >>
On 05/11/18 17:08, Brad Love wrote: > Hi Hans, > > > On 2018-05-11 09:41, Hans Verkuil wrote: >> Hi Brad, >> >> On 04/26/18 19:19, Brad Love wrote: >>> config-mycompat.h is for overriding macros which are incorrectly >>> enabled on certain kernels by the build system. The file should be >>> left empty, unless build errors are encountered for a kernel. The >>> file is removed by distclean, therefore should be externally >>> sourced, before the build process starts, when required. >>> >>> In standard operation the file is empty, but if a particular kernel has >>> incorrectly enabled options defined this allows them to be undefined. >> Can you give an example where this will be used? >> >> FYI: I've committed patches 1-6, but I don't quite understand when this patch >> is needed. >> >> With "for overriding macros which are incorrectly enabled on certain kernels" >> do you mean when distros do backports of features from later kernels? >> >> Regards, >> >> Hans > > > Apologies if I was not very clear. Yes, this is for use in > kernels/distros whose maintainers have integrated various backports, and > which the media_build system does not pick up on for whatever reason. At > that point there are options defined in config-compat.h, which enable > backports in compat.h, but which already exist in the target kernel. > > For example, on the device I'm working on right now, in kernel 3.10, I > have to supply the following three options in config-mycompat.h or > modify the tree and stuff them right into the top of compat.h: > > #undef NEED_WRITEL_RELAXED > #undef NEED_PM_RUNTIME_GET > #undef NEED_PFN_TO_PHYS > > > The above disables those three media_build backports and allows > everything to build. It seems there is quite often at least one backport > I must disable, and some target kernels require multiple backports disabled. OK, that's what I thought. Can you post a v2 of this patch with this explanation included? Both in the commit log and in the compat.h comment. That should make it clear what the purpose of this file is. Regards, Hans > > Cheers, > > Brad > > > >> >>> Signed-off-by: Brad Love <brad@nextdimension.cc> >>> --- >>> v4l/Makefile | 3 ++- >>> v4l/compat.h | 7 +++++++ >>> 2 files changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/v4l/Makefile b/v4l/Makefile >>> index 270a624..ee18d11 100644 >>> --- a/v4l/Makefile >>> +++ b/v4l/Makefile >>> @@ -273,6 +273,7 @@ links:: >>> @find ../linux/drivers/misc -name '*.[ch]' -type f -print0 | xargs -0n 255 ln -sf --target-directory=. >>> >>> config-compat.h:: $(obj)/.version .myconfig scripts/make_config_compat.pl >>> + -touch $(obj)/config-mycompat.h >>> perl scripts/make_config_compat.pl $(SRCDIR) $(obj)/.myconfig $(obj)/config-compat.h >>> >>> kernel-links makelinks:: >>> @@ -298,7 +299,7 @@ clean:: >>> distclean:: clean >>> -rm -f .version .*.o.flags .*.o.d *.mod.gcno Makefile.media \ >>> Kconfig Kconfig.kern .config .config.cmd .myconfig \ >>> - .kconfig.dep >>> + .kconfig.dep config-mycompat.h >>> -rm -rf .tmp_versions .tmp*.ver .tmp*.o .*.gcno .cache.mk >>> -rm -f scripts/lxdialog scripts/kconfig >>> @find .. -name '*.orig' -exec rm '{}' \; >>> diff --git a/v4l/compat.h b/v4l/compat.h >>> index 87ce401..db48fdf 100644 >>> --- a/v4l/compat.h >>> +++ b/v4l/compat.h >>> @@ -8,6 +8,13 @@ >>> #include <linux/version.h> >>> >>> #include "config-compat.h" >>> +/* config-mycompat.h is for overriding #defines which >>> + * are incorrectly enabled on certain kernels. The file >>> + * should be left empty, unless build errors are encountered >>> + * for a kernel. The file is removed by distclean, therefore >>> + * should be externally sourced, before compilation, when required. >>> + */ >>> +#include "config-mycompat.h" >>> >>> #ifndef SZ_512 >>> #define SZ_512 0x00000200 >>> > >
Hi Hans, On 2018-05-11 10:11, Hans Verkuil wrote: > On 05/11/18 17:08, Brad Love wrote: >> Hi Hans, >> >> >> On 2018-05-11 09:41, Hans Verkuil wrote: >>> Hi Brad, >>> >>> On 04/26/18 19:19, Brad Love wrote: >>>> config-mycompat.h is for overriding macros which are incorrectly >>>> enabled on certain kernels by the build system. The file should be >>>> left empty, unless build errors are encountered for a kernel. The >>>> file is removed by distclean, therefore should be externally >>>> sourced, before the build process starts, when required. >>>> >>>> In standard operation the file is empty, but if a particular kernel has >>>> incorrectly enabled options defined this allows them to be undefined. >>> Can you give an example where this will be used? >>> >>> FYI: I've committed patches 1-6, but I don't quite understand when this patch >>> is needed. >>> >>> With "for overriding macros which are incorrectly enabled on certain kernels" >>> do you mean when distros do backports of features from later kernels? >>> >>> Regards, >>> >>> Hans >> >> Apologies if I was not very clear. Yes, this is for use in >> kernels/distros whose maintainers have integrated various backports, and >> which the media_build system does not pick up on for whatever reason. At >> that point there are options defined in config-compat.h, which enable >> backports in compat.h, but which already exist in the target kernel. >> >> For example, on the device I'm working on right now, in kernel 3.10, I >> have to supply the following three options in config-mycompat.h or >> modify the tree and stuff them right into the top of compat.h: >> >> #undef NEED_WRITEL_RELAXED >> #undef NEED_PM_RUNTIME_GET >> #undef NEED_PFN_TO_PHYS >> >> >> The above disables those three media_build backports and allows >> everything to build. It seems there is quite often at least one backport >> I must disable, and some target kernels require multiple backports disabled. > OK, that's what I thought. Can you post a v2 of this patch with this explanation > included? Both in the commit log and in the compat.h comment. > > That should make it clear what the purpose of this file is. > > Regards, > > Hans Will do now. > >> Cheers, >> >> Brad >> >> >> >>>> Signed-off-by: Brad Love <brad@nextdimension.cc> >>>> --- >>>> v4l/Makefile | 3 ++- >>>> v4l/compat.h | 7 +++++++ >>>> 2 files changed, 9 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/v4l/Makefile b/v4l/Makefile >>>> index 270a624..ee18d11 100644 >>>> --- a/v4l/Makefile >>>> +++ b/v4l/Makefile >>>> @@ -273,6 +273,7 @@ links:: >>>> @find ../linux/drivers/misc -name '*.[ch]' -type f -print0 | xargs -0n 255 ln -sf --target-directory=. >>>> >>>> config-compat.h:: $(obj)/.version .myconfig scripts/make_config_compat.pl >>>> + -touch $(obj)/config-mycompat.h >>>> perl scripts/make_config_compat.pl $(SRCDIR) $(obj)/.myconfig $(obj)/config-compat.h >>>> >>>> kernel-links makelinks:: >>>> @@ -298,7 +299,7 @@ clean:: >>>> distclean:: clean >>>> -rm -f .version .*.o.flags .*.o.d *.mod.gcno Makefile.media \ >>>> Kconfig Kconfig.kern .config .config.cmd .myconfig \ >>>> - .kconfig.dep >>>> + .kconfig.dep config-mycompat.h >>>> -rm -rf .tmp_versions .tmp*.ver .tmp*.o .*.gcno .cache.mk >>>> -rm -f scripts/lxdialog scripts/kconfig >>>> @find .. -name '*.orig' -exec rm '{}' \; >>>> diff --git a/v4l/compat.h b/v4l/compat.h >>>> index 87ce401..db48fdf 100644 >>>> --- a/v4l/compat.h >>>> +++ b/v4l/compat.h >>>> @@ -8,6 +8,13 @@ >>>> #include <linux/version.h> >>>> >>>> #include "config-compat.h" >>>> +/* config-mycompat.h is for overriding #defines which >>>> + * are incorrectly enabled on certain kernels. The file >>>> + * should be left empty, unless build errors are encountered >>>> + * for a kernel. The file is removed by distclean, therefore >>>> + * should be externally sourced, before compilation, when required. >>>> + */ >>>> +#include "config-mycompat.h" >>>> >>>> #ifndef SZ_512 >>>> #define SZ_512 0x00000200 >>>> >>
Hello Brad! > and which the media_build system does not pick up on for whatever > reason. Maybe it would be better to analyse why "make_config_compat.pl" selects wrongly the compatibility code. > It seems there is quite often at least one backport I must disable, > and some target kernels require multiple backports disabled. This sounds strange. media-build should handle those cases correctly in my opinion and should be fixed. At least we should check why this happens. Patch 7/7 sounds like a workaround for me. If there is really no other solution, than we need to implement this possibility for distro maintainers. On the other hand, why is media-build used by distro maintainers at all? I thought distro Kernels are built with a full tree and thus doesn't need media-build. BR, Jasmin
Hi Jasmin, On 2018-05-11 15:02, Jasmin J. wrote: > Hello Brad! > >> and which the media_build system does not pick up on for whatever >> reason. > Maybe it would be better to analyse why "make_config_compat.pl" selects > wrongly the compatibility code. I've found several reasons, but the one I seem to encounter often is the symbols are located in files that the config_compat script does not check for. Some symbols have moved between revisions, so a check that works in 4.2 will fail in 3.10 and will also fail in 3.2. I've also seen not-found symbols defined in arch/ code, which makes it a little difficult to add additional paths to the search. There is also the case of a maintainer who puts their backports wherever they felt like and just glued things together in their build. I'm not averse to handling this other ways, but while I was looking at fixing make_config_compat.pl it just seemed that the script would have to be made more complex. The search strategy would have to change to include additional search paths, possibly depending on kernel version as well as support to find symbols in the arch code, while making sure the symbol was found in the correct target arch. I know this is a workaround, but I have to do this 'workaround' in some form for almost every package I maintain. > >> It seems there is quite often at least one backport I must disable, >> and some target kernels require multiple backports disabled. > This sounds strange. media-build should handle those cases correctly > in my opinion and should be fixed. > At least we should check why this happens. > > Patch 7/7 sounds like a workaround for me. > If there is really no other solution, than we need to implement this > possibility for distro maintainers. This is not for distro maintainers. I am the lead engineer at Hauppauge and and a responsibility of mine is the support our hardware on the largest amount of systems and architectures possible. I work with kernels anywhere from 3.2 to 4.15, all provided by either manufacturers or distributions. Some are close to mainline, others not so much. > > On the other hand, why is media-build used by distro maintainers at all? > I thought distro Kernels are built with a full tree and thus doesn't > need media-build. > > BR, > Jasmin I am not a distro maintainer. What I do is maintain and provide out of tree driver packages for a large variety of systems, as well as in tree integrations of the entire media tree for an assortment of Ubuntu kernels. I have no influence over how a maintainer or distro publisher organizes source code. I just take their tree and either compile the media_build system out of tree, or integrate it completely. It is not very often I can get *random_arch* kernel tree from *random_manufacturer* and media_build 'just works'. Like I said in the cover-letter, I'm totally open to better ways of handling this. I am just honestly tired of having to fudge with things before every new build, dirtying up the media_build tree. I like things reproducible and clean and this seems to be the only thing left preventing that. Cheers, Brad
Hello Brad! THX for this clarification! So you tried already to fix the config_compat script and I agree with you that it is difficult for you because of the various Kernels and distributions you have to maintain. Then your workaround is indeed a possibility to use media-build to build your modules out of tree for all the combinations you have. Maybe in the future other people may use this also. BR, Jasmin
Hi Jasmin, On 2018-05-11 16:43, Jasmin J. wrote: > Hello Brad! > > THX for this clarification! > > So you tried already to fix the config_compat script and I agree with you that > it is difficult for you because of the various Kernels and distributions you > have to maintain. > > Then your workaround is indeed a possibility to use media-build to build your > modules out of tree for all the combinations you have. Maybe in the future > other people may use this also. > > BR, > Jasmin Please do check out my v2. Hans also had questions and concerns over the intended usage, so I did my best to explain it thoroughly in the v2 commit message along with inside of compat.h. I hope it is clear enough now to understand for anyone who might need the feature in the future. Cheers, Brad
diff --git a/v4l/Makefile b/v4l/Makefile index 270a624..ee18d11 100644 --- a/v4l/Makefile +++ b/v4l/Makefile @@ -273,6 +273,7 @@ links:: @find ../linux/drivers/misc -name '*.[ch]' -type f -print0 | xargs -0n 255 ln -sf --target-directory=. config-compat.h:: $(obj)/.version .myconfig scripts/make_config_compat.pl + -touch $(obj)/config-mycompat.h perl scripts/make_config_compat.pl $(SRCDIR) $(obj)/.myconfig $(obj)/config-compat.h kernel-links makelinks:: @@ -298,7 +299,7 @@ clean:: distclean:: clean -rm -f .version .*.o.flags .*.o.d *.mod.gcno Makefile.media \ Kconfig Kconfig.kern .config .config.cmd .myconfig \ - .kconfig.dep + .kconfig.dep config-mycompat.h -rm -rf .tmp_versions .tmp*.ver .tmp*.o .*.gcno .cache.mk -rm -f scripts/lxdialog scripts/kconfig @find .. -name '*.orig' -exec rm '{}' \; diff --git a/v4l/compat.h b/v4l/compat.h index 87ce401..db48fdf 100644 --- a/v4l/compat.h +++ b/v4l/compat.h @@ -8,6 +8,13 @@ #include <linux/version.h> #include "config-compat.h" +/* config-mycompat.h is for overriding #defines which + * are incorrectly enabled on certain kernels. The file + * should be left empty, unless build errors are encountered + * for a kernel. The file is removed by distclean, therefore + * should be externally sourced, before compilation, when required. + */ +#include "config-mycompat.h" #ifndef SZ_512 #define SZ_512 0x00000200
config-mycompat.h is for overriding macros which are incorrectly enabled on certain kernels by the build system. The file should be left empty, unless build errors are encountered for a kernel. The file is removed by distclean, therefore should be externally sourced, before the build process starts, when required. In standard operation the file is empty, but if a particular kernel has incorrectly enabled options defined this allows them to be undefined. Signed-off-by: Brad Love <brad@nextdimension.cc> --- v4l/Makefile | 3 ++- v4l/compat.h | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-)