Re: [PATCH v9 17/19] drm/virtio: kconfig: Fix recursive dependency issue.

[Emil Velikov]

Hi Peter,

On 6 October 2016 at 11:48, Peter Griffin <peter.griffin@xxxxxxxxxx> wrote:
> Hi Emil,
>
> On Wed, 21 Sep 2016, Emil Velikov wrote:
>
>> On 20 September 2016 at 09:32, Peter Griffin <peter.griffin@xxxxxxxxxx> wrote:
>> > Hi Emil,
>> >
>> > On Tue, 20 Sep 2016, Emil Velikov wrote:
>> >
>> >> On 5 September 2016 at 14:16, Peter Griffin <peter.griffin@xxxxxxxxxx> wrote:
>> >> > ST_SLIM_REMOTEPROC must select REMOTEPROC, which exposes the following
>> >> > recursive dependency.
>> >
>> >
>> >> >
>> >> From a humble skim through remoteproc, drm and a few other subsystems
>> >> I think the above is wrong. All the drivers (outside of remoteproc),
>> >> that I've seen, depend on the core component, they don't select it.
>> >
>> > I will let Bjorn comment on the remoteproc subsystem Kconfig design, and
>> > why it is like it is.
>> >
>> > For this particular SLIM_RPROC I have added it to Kconfig in keeping with all
>> > the other drivers in the remoteproc subsystem which has exposed this recursive
>> > dependency issue.
>> >
>> > For this particular kconfig symbol a quick grep reveals more drivers in
>> > the kernel using 'select', than 'depend on'
>> >
>> > git grep "select VIRTIO" | wc -l
>> > 14
>> >
>> > git grep "depends on VIRTIO" | wc -l
>> > 10
>> >
>> Might be worth taking a closer look into these at some point.
>
> VIRTIO has no dependencies, and is a non visible symbol. Its Kconfig help also
> mentions that drivers should select it.
>
This is a (un)fortunate detail cannot/should not overrule the other
arguments I've mentioned, should it ?

>>
>> >
>> >> Furthermore most places explicitly hide the drivers from the menu if
>> >> the core component isn't enabled.
>> >
>> > Remoteproc subsystem takes a different approach, the core code is only enabled
>> > if a driver which relies on it is enabled. This IMHO makes sense, as
>> > remoteproc is not widely used (only a few particular ARM SoC's).
>> >
>> > It is true that for subsystems which rely on the core component being
>> > explicitly enabled, they often tend to hide drivers which depend on it
>> > from the menu unless it is. This also makes sense.
>> >
>> >>
>> >> Is there something that requires such a different/unusual behaviour in
>> >> remoteproc ?
>> >>
>> >
>> > I'm not sure it is that unusual...looking at config USB, it selects USB_COMMON in
>> > mfd subsystem, client drivers select MFD_CORE.
>> >
>> On the USB case I'm not sure what the reasoning behind the USB vs
>> USB_COMMON split. In seems that one could just fold them, but that's
>> another topic. On the MFD side... it follows the select {,mis,ab}use.
>> With one (the only one?) MFD driver not using/selecting MFD_CORE doing
>> it's own version of mfd_add_devices... which could be reworked,
>> possibly.
>
> Much like selecting VIRTIO in this patch, MFD_CORE is a non visible symbol
> with no dependencies so it matches the documentation Jani referenced.
>
> I personally think the approach taken makes sense, as why would you want to have
> a CONFIG_MFD_CORE=y symbol being enabled unless you actually have a MFD device
> which uses it also enabled in your kernel?
>
> It seems to me a good solution to make the client drivers select the core
> component so that it only gets enabled if at least one driver requires it.
> This avoids having useless core code which will never be used compiled into the
> kernel and in the end a smaller kernel size for a given kernel configuration (better
> cache use etc etc).
>
>> > I've added Arnd to this thread, as I've seen lots of Kconfig patches from him
>> > recently, maybe he has some thoughts on whether this is the correct fix or not?
>> >
>> Ack. Fwiw, I believe that the reasoning put by Jani is perfeclty
>> reasonable, but it'll be great to hear others as well.
>
> Yes me to. However I don't think anything in this patch is at odds with the
> documentation Jani has referenced.
>
It case it's not clear, let me elaborate:

Yes, using depend might not be the most user-friendly thing to do and
in the long term we might want to move to select.
Yes, I agree with the argument about symbol visibility but that is not
the only contributing factor.

If one wants to pick on specific users which opt for $driver select
$core they must do the same for $driver depends on $core. Check the
number 'in favour" of each case and draw their conclusions ;-)

In particular: both MFD_CORE and USB_COMMON, are _optional_ as only
some drivers depends on them. Thus giving them as an example is
wrong/irrelevant, I'm afraid.

Thanks
Emil