Hi Jean,
On Wed. 23 Oct. 2024, 02:04, Jean Delvare <jdelvare@xxxxxxx> wrote:
> Hi Vincent,
>
> On Tue, 22 Oct 2024 22:22:05 +0900, Vincent MAILHOL wrote:
> > On Tue. 22 Oct. 2024 at 20:06, Jean Delvare <jdelvare@xxxxxxx> wrote:
> > > Since commit 0166dc11be91 ("of: make CONFIG_OF user selectable"), OF
> > > can be enabled on all architectures. Therefore depending on
> > > COMPILE_TEST as an alternative is no longer needed.
> >
> > I understand the motivation behind this patch, but for me, as a
> > maintainer, it becomes more work when I want to do a compile test.
> > Before I would have needed to only select COMPILE_TEST but now, I
> > would need to remember to also select OF for that driver to appear in
> > the menuconfig.
>
> Not sure what your typical process looks like, but if you need to
> test-build drivers on a regular basis, why don't you have
> CONFIG_COMPILE_TEST always enabled? Switching back and forth must be a
> hassle.
The point is that I do not need to test build all drivers on a regular
basis. The CAN subsystem is a small tree with not so many activities.
And this OF thing is a pitfall (in which I have fallen into it in the
past…)
> About CONFIG_OF, well you probably want it enabled regardless. For one
> thing, there are at least 940 drivers in the kernel tree which depend on
> OF, so you are already missing a lot of test build coverage if you have
> it disabled.
The CAN subtree for which I care has only four drivers with OF, only
two of which are not automatically selected by COMPILE_TEST:
$ git grep "depends.* OF" drivers/net/can/
drivers/net/can/Kconfig: depends on OF || COLDFIRE || COMPILE_TEST
drivers/net/can/Kconfig: depends on OF && HAS_DMA && HAS_IOMEM
drivers/net/can/ctucanfd/Kconfig: depends on HAS_IOMEM && OF
drivers/net/can/rockchip/Kconfig: depends on OF || COMPILE_TEST
> For another, if test-building a driver which would normally depend on
> OF, but with OF disabled (as you do for rockchip_canfd at the moment,
> as I understand it), then you may not be building the full driver. Part
> of the code could be stripped out at build time because the compiler
> notices that it is unreachable. Apparently this isn't the case of the
> rockchip_canfd driver though.
>
> > Well, I am not strongly against this simplification, but, wouldn't it
> > be good to make COMPILE_TEST automatically select OF then? Looking at
> > the description of COMPILE_TEST, I read:
> >
> > If you are a developer and want to build everything available, say Y here.
>
> As a side note, I don't think this is a proper description of how to
> use this option. If you "want to build everything available" then what
> you want to do is "make allmodconfig", not manually enabling all
> drivers in "make menuconfig". The purpose of manually selecting
> COMPILE_TEST would presumably be to test-build a specific driver or set
> of drivers.
Ack. It would be helpful to rephrase that description.
> > So having COMPILE_TEST automatically select OF looks sane to me as it
> > goes in the direction of "building everything". If this makes sense, I
> > can send a patch for this. Thoughts?
>
> My initial reaction was that this would be an artificial dependency,
> which seems wrong.
>
> However, considering that the purpose of COMPILE_TEST is essentially to
> let the user build drivers from other architectures, and that enabling
> OF on an architecture which doesn't have it enabled by default
> essentially lets you select a whole lot of drivers from other
> architectures... I must admit that both options indeed share a
> purpose.
>
> But I still have a concern. Some drivers can be built with or without
> OF (they support both OF and non-OF devices). If someone wants to
> test-build such a driver on a different architecture (and therefore
> must enable COMPILE_TEST) and wants to ensure that it builds fine with
> and without OF, then if COMPILE_TEST selects OF as you suggested, they
> would no longer be able to test the CONFIG_OF=n case.
>
> So I'm afraid we can't actually do that.
Ack, this concern seems valid.
Thanks for your time and your good explanations.
I guess it is now my duty to not forget anymore to also enable OF when
doing compile tests. With this said:
Reviewed-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
Yours sincerely,
Vincent Mailhol
