Smarter Kconfig help

Smarter Kconfig help

[Russell King - ARM Linux admin]

Guys,

We need to be smarter when writing Kconfig help.  I'm just going
through updating my build trees with the results of 5.0 development,
and a number of the help texts are next to useless.  For example,

PVPANIC - is this something that should be enabled for a host or
guest kernel?  Answer: you have to read the driver code to find out.

IMX_IRQSTEER - which i.MX SoCs does this apply to?  The help text
just says:

  "Support for the i.MX IRQSTEER interrupt multiplexer/remapper."

which doesn't say which SoCs this should be enabled for - it turns
out that grepping for the driver's DT compatible string, none of
the 32-bit ARM cores have support for this, yet we still default
it to enabled there.  It seems the help text should at the very
least tell the user that this is not applicable to i.MX SoCs with
32-bit ARM cores.

I'm sure there's many other instances of this... I suspect that
it's caused by review concentrating mostly on the technical aspects
of the code and the Kconfig help text just gets forgotten about.

Can we at least improve these two options please?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Smarter Kconfig help

[Mark Rutland]

On Tue, Mar 05, 2019 at 05:31:12PM +0000, Russell King - ARM Linux admin wrote:
> Guys,

Hi Russell,

> We need to be smarter when writing Kconfig help.  I'm just going
> through updating my build trees with the results of 5.0 development,
> and a number of the help texts are next to useless.  For example,
> 
> PVPANIC - is this something that should be enabled for a host or
> guest kernel?  Answer: you have to read the driver code to find out.

When I looked at the help text:

    This driver provides support for the pvpanic device.  pvpanic is
    a paravirtualized device provided by QEMU; it lets a virtual machine
    (guest) communicate panic events to the host.

... it seemed clear to me that this was for a guest, given the text says
QEMU provides the device. I guess you read that as meaning QEMU asks the
host kernel to provide the device to the guest?

Do you have a suggestion for how to word that unambiguously?

> IMX_IRQSTEER - which i.MX SoCs does this apply to?  The help text
> just says:
> 
>   "Support for the i.MX IRQSTEER interrupt multiplexer/remapper."
> 
> which doesn't say which SoCs this should be enabled for - it turns
> out that grepping for the driver's DT compatible string, none of
> the 32-bit ARM cores have support for this, yet we still default
> it to enabled there.  It seems the help text should at the very
> least tell the user that this is not applicable to i.MX SoCs with
> 32-bit ARM cores.
> 
> I'm sure there's many other instances of this... I suspect that
> it's caused by review concentrating mostly on the technical aspects
> of the code and the Kconfig help text just gets forgotten about.

Just to be clear, in general what you want is for Kconfig help to be
clearer about *when* an option is relevant, right?

I'll try to bear that in mind when reviewing in future.

Thanks,
Mark.

Re: Smarter Kconfig help

[Lucas Stach]

Hi Russell,

Am Dienstag, den 05.03.2019, 17:31 +0000 schrieb Russell King - ARM Linux admin:
> Guys,
> 
> We need to be smarter when writing Kconfig help.  I'm just going
> through updating my build trees with the results of 5.0 development,
> and a number of the help texts are next to useless.  For example,
> 
> PVPANIC - is this something that should be enabled for a host or
> guest kernel?  Answer: you have to read the driver code to find out.
> 
> IMX_IRQSTEER - which i.MX SoCs does this apply to?  The help text
> just says:
> 
>   "Support for the i.MX IRQSTEER interrupt multiplexer/remapper."
> 
> which doesn't say which SoCs this should be enabled for - it turns
> out that grepping for the driver's DT compatible string, none of
> the 32-bit ARM cores have support for this, yet we still default
> it to enabled there.  It seems the help text should at the very
> least tell the user that this is not applicable to i.MX SoCs with
> 32-bit ARM cores.
> 
> I'm sure there's many other instances of this... I suspect that
> it's caused by review concentrating mostly on the technical aspects
> of the code and the Kconfig help text just gets forgotten about.
> 
> Can we at least improve these two options please?

While I totally agree that the irqsteer driver should only default Y on
64bit i.MX SoCs, I'm not really sure what to do about the Kconfig help
text.

Enumerating SoCs in the Kconfig of a IP block driver is always prone to
get outdated. Just as the first example I've been able to come up with,
the MMC_SDHCI_ESDHC_IMX help text says: "This selects the Freescale
eSDHC/uSDHC controller support found on i.MX25, i.MX35 i.MX5x and
i.MX6x.", while the same IP block is to be found on i.MX7 and various
i.MX8.

For the Kconfig user, who needs to decide if an option is relevant,
outdated SoC information is probably just as bad as no information at
all.

Regards,
Lucas

Re: Smarter Kconfig help

[Russell King - ARM Linux admin]

On Wed, Mar 06, 2019 at 10:45:52AM +0100, Lucas Stach wrote:
> Hi Russell,
> 
> Am Dienstag, den 05.03.2019, 17:31 +0000 schrieb Russell King - ARM Linux admin:
> > Guys,
> > 
> > We need to be smarter when writing Kconfig help.  I'm just going
> > through updating my build trees with the results of 5.0 development,
> > and a number of the help texts are next to useless.  For example,
> > 
> > PVPANIC - is this something that should be enabled for a host or
> > guest kernel?  Answer: you have to read the driver code to find out.
> > 
> > IMX_IRQSTEER - which i.MX SoCs does this apply to?  The help text
> > just says:
> > 
> >   "Support for the i.MX IRQSTEER interrupt multiplexer/remapper."
> > 
> > which doesn't say which SoCs this should be enabled for - it turns
> > out that grepping for the driver's DT compatible string, none of
> > the 32-bit ARM cores have support for this, yet we still default
> > it to enabled there.  It seems the help text should at the very
> > least tell the user that this is not applicable to i.MX SoCs with
> > 32-bit ARM cores.
> > 
> > I'm sure there's many other instances of this... I suspect that
> > it's caused by review concentrating mostly on the technical aspects
> > of the code and the Kconfig help text just gets forgotten about.
> > 
> > Can we at least improve these two options please?
> 
> While I totally agree that the irqsteer driver should only default Y on
> 64bit i.MX SoCs, I'm not really sure what to do about the Kconfig help
> text.
> 
> Enumerating SoCs in the Kconfig of a IP block driver is always prone to
> get outdated. Just as the first example I've been able to come up with,
> the MMC_SDHCI_ESDHC_IMX help text says: "This selects the Freescale
> eSDHC/uSDHC controller support found on i.MX25, i.MX35 i.MX5x and
> i.MX6x.", while the same IP block is to be found on i.MX7 and various
> i.MX8.
> 
> For the Kconfig user, who needs to decide if an option is relevant,
> outdated SoC information is probably just as bad as no information at
> all.

How about "This option does not apply to AArch32 based SoCs" or
"This option should be enabled for i.MX7 and later SoCs" ?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Smarter Kconfig help

[Andy Shevchenko]

On Wed, Mar 6, 2019 at 11:52 AM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
> On Wed, Mar 06, 2019 at 10:45:52AM +0100, Lucas Stach wrote:
> > Am Dienstag, den 05.03.2019, 17:31 +0000 schrieb Russell King - ARM Linux admin:

> > While I totally agree that the irqsteer driver should only default Y on
> > 64bit i.MX SoCs, I'm not really sure what to do about the Kconfig help
> > text.
> >
> > Enumerating SoCs in the Kconfig of a IP block driver is always prone to
> > get outdated. Just as the first example I've been able to come up with,
> > the MMC_SDHCI_ESDHC_IMX help text says: "This selects the Freescale
> > eSDHC/uSDHC controller support found on i.MX25, i.MX35 i.MX5x and
> > i.MX6x.", while the same IP block is to be found on i.MX7 and various
> > i.MX8.
> >
> > For the Kconfig user, who needs to decide if an option is relevant,
> > outdated SoC information is probably just as bad as no information at
> > all.
>
> How about "This option does not apply to AArch32 based SoCs" or

Negative is usually error prone.

> "This option should be enabled for i.MX7 and later SoCs" ?

Why it should be? It can / could be I guess.

-- 
With Best Regards,
Andy Shevchenko

Re: Smarter Kconfig help

[Russell King - ARM Linux admin]

On Wed, Mar 06, 2019 at 12:49:40PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 6, 2019 at 11:52 AM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > On Wed, Mar 06, 2019 at 10:45:52AM +0100, Lucas Stach wrote:
> > > Am Dienstag, den 05.03.2019, 17:31 +0000 schrieb Russell King - ARM Linux admin:
> 
> > > While I totally agree that the irqsteer driver should only default Y on
> > > 64bit i.MX SoCs, I'm not really sure what to do about the Kconfig help
> > > text.
> > >
> > > Enumerating SoCs in the Kconfig of a IP block driver is always prone to
> > > get outdated. Just as the first example I've been able to come up with,
> > > the MMC_SDHCI_ESDHC_IMX help text says: "This selects the Freescale
> > > eSDHC/uSDHC controller support found on i.MX25, i.MX35 i.MX5x and
> > > i.MX6x.", while the same IP block is to be found on i.MX7 and various
> > > i.MX8.
> > >
> > > For the Kconfig user, who needs to decide if an option is relevant,
> > > outdated SoC information is probably just as bad as no information at
> > > all.
> >
> > How about "This option does not apply to AArch32 based SoCs" or
> 
> Negative is usually error prone.
> 
> > "This option should be enabled for i.MX7 and later SoCs" ?
> 
> Why it should be? It can / could be I guess.

I've no idea.  I'm just making suggestions - if you have anything
better, let's hear it.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Smarter Kconfig help

[Russell King - ARM Linux admin]

On Wed, Mar 06, 2019 at 11:34:36AM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Mar 06, 2019 at 12:49:40PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 6, 2019 at 11:52 AM Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > > On Wed, Mar 06, 2019 at 10:45:52AM +0100, Lucas Stach wrote:
> > > > Am Dienstag, den 05.03.2019, 17:31 +0000 schrieb Russell King - ARM Linux admin:
> > 
> > > > While I totally agree that the irqsteer driver should only default Y on
> > > > 64bit i.MX SoCs, I'm not really sure what to do about the Kconfig help
> > > > text.
> > > >
> > > > Enumerating SoCs in the Kconfig of a IP block driver is always prone to
> > > > get outdated. Just as the first example I've been able to come up with,
> > > > the MMC_SDHCI_ESDHC_IMX help text says: "This selects the Freescale
> > > > eSDHC/uSDHC controller support found on i.MX25, i.MX35 i.MX5x and
> > > > i.MX6x.", while the same IP block is to be found on i.MX7 and various
> > > > i.MX8.
> > > >
> > > > For the Kconfig user, who needs to decide if an option is relevant,
> > > > outdated SoC information is probably just as bad as no information at
> > > > all.
> > >
> > > How about "This option does not apply to AArch32 based SoCs" or
> > 
> > Negative is usually error prone.
> > 
> > > "This option should be enabled for i.MX7 and later SoCs" ?
> > 
> > Why it should be? It can / could be I guess.
> 
> I've no idea.  I'm just making suggestions - if you have anything
> better, let's hear it.

In case it isn't clear, this is the *exact* point here - I don't know
whether this option should be enabled for iMX6 or not, and the only
way I found out was to grep the dts files in arch/arm/boot/dts for
the driver's compatible string.  What that reveals is that *no* 32-bit
dts files contain the compatible string, and so I summise that *no*
32-bit iMX SoC should have this driver enabled.

Given that I don't know for certain, _I_ can't write a proper help
text for this - I can only make suggestions which _might_ fit what
the reality actually is.

The excuse that "we can't list the explicit SoCs" to me seems to be
a very lame excuse for a poor, one line help text that says absolutely
nothing that can't already be gathered from the option line itself.
So the current help text might as well be deleted - it's that "useful".

The best that I can come up with right now, given what little I know
from grepping the 32-bit DTS files, is that the help text should at
least indicate that it *isn't* applicable to 32-bit SoCs, or if you
prefer, *is* applicable to 64-bit SoCs.  Beyond that, I have no
information to formulate a better suggestion.

This is precisely why I say that we need to be smarter about Kconfig
help text: if it gives no useful information, it might as well not
even exist, and we might as well be honest and have Kconfig say that
there is no help available for the option.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Smarter Kconfig help

[Russell King - ARM Linux admin]

Hi Mark,

On Wed, Mar 06, 2019 at 09:35:21AM +0000, Mark Rutland wrote:
> On Tue, Mar 05, 2019 at 05:31:12PM +0000, Russell King - ARM Linux admin wrote:
> > Guys,
> 
> Hi Russell,
> 
> > We need to be smarter when writing Kconfig help.  I'm just going
> > through updating my build trees with the results of 5.0 development,
> > and a number of the help texts are next to useless.  For example,
> > 
> > PVPANIC - is this something that should be enabled for a host or
> > guest kernel?  Answer: you have to read the driver code to find out.
> 
> When I looked at the help text:
> 
>     This driver provides support for the pvpanic device.  pvpanic is
>     a paravirtualized device provided by QEMU; it lets a virtual machine
>     (guest) communicate panic events to the host.
> 
> ... it seemed clear to me that this was for a guest, given the text says
> QEMU provides the device. I guess you read that as meaning QEMU asks the
> host kernel to provide the device to the guest?

Yes - that's exactly where the confusion was.  It could be something
like a tap network device to allow a guest access to a facility on the
host, or it could be something that the guest kernel uses to communicate
with its host environment.

> Do you have a suggestion for how to word that unambiguously?

It used to be normal to include a sugestion in the help text that
guided when an option should be enabled.  So, adding something like:

    "Say Y here if you are building a kernel for a virtual machine."

would make it clear.  This used to be standard throughout the kernel,
but it seems in recent years, this has been omitted.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Smarter Kconfig help

[Enrico Weigelt, metux IT consult]

On 06.03.19 13:42, Russell King - ARM Linux admin wrote:

> In case it isn't clear, this is the *exact* point here - I don't know> whether this option should be enabled for iMX6 or not, and the only>
way I found out was to grep the dts files in arch/arm/boot/dts for> the
driver's compatible string.  What that reveals is that *no* 32-bit> dts
files contain the compatible string, and so I summise that *no*> 32-bit
iMX SoC should have this driver enabled.
The problem is a bit more generic. I often have to spend lots of time
to find out which configs to enable on a specific board, to get certain
features (eg. network, sata, display, gpu, ...). Even worse: many
options require other stuff enabled before even showing up. And when
disabling unneeded stuff, it leaves lots of other things enabled.
(we don't have some `apt autoremove` kconfig counterpart :().

I've decided to cope w/ that on a higher level and written a little
config generator tool for that - here you can enable high level
features (eg. 'network' or 'display', etc) and it will generate the
actual .config:

	https://github.com/metux/kmct

> The excuse that "we can't list the explicit SoCs" to me seems to be> a very lame excuse

Maybe this actually means "nobody here volounteered to actually maintain
these help texts" ?

> The best that I can come up with right now, given what little I know> from grepping the 32-bit DTS files, is that the help text should at>
least indicate that it *isn't* applicable to 32-bit SoCs, or if you>
prefer, *is* applicable to 64-bit SoCs.  Beyond that, I have no>
information to formulate a better suggestion.
Perhaps just fix the text based on your knowledge and send a patch to
the maintainers. They'll propably tell you if it's incorrect.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: Smarter Kconfig help

[Russell King - ARM Linux admin]

On Wed, Mar 06, 2019 at 09:16:02PM +0100, Enrico Weigelt, metux IT consult wrote:
> On 06.03.19 13:42, Russell King - ARM Linux admin wrote:
> 
> > In case it isn't clear, this is the *exact* point here - I don't know> whether this option should be enabled for iMX6 or not, and the only>
> way I found out was to grep the dts files in arch/arm/boot/dts for> the
> driver's compatible string.  What that reveals is that *no* 32-bit> dts
> files contain the compatible string, and so I summise that *no*> 32-bit
> iMX SoC should have this driver enabled.
> The problem is a bit more generic. I often have to spend lots of time
> to find out which configs to enable on a specific board, to get certain
> features (eg. network, sata, display, gpu, ...). Even worse: many
> options require other stuff enabled before even showing up. And when
> disabling unneeded stuff, it leaves lots of other things enabled.
> (we don't have some `apt autoremove` kconfig counterpart :().
> 
> I've decided to cope w/ that on a higher level and written a little
> config generator tool for that - here you can enable high level
> features (eg. 'network' or 'display', etc) and it will generate the
> actual .config:
> 
> 	https://github.com/metux/kmct
> 
> > The excuse that "we can't list the explicit SoCs" to me seems to be> a very lame excuse
> 
> Maybe this actually means "nobody here volounteered to actually maintain
> these help texts" ?
> 
> > The best that I can come up with right now, given what little I know> from grepping the 32-bit DTS files, is that the help text should at>
> least indicate that it *isn't* applicable to 32-bit SoCs, or if you>
> prefer, *is* applicable to 64-bit SoCs.  Beyond that, I have no>
> information to formulate a better suggestion.
> Perhaps just fix the text based on your knowledge and send a patch to
> the maintainers. They'll propably tell you if it's incorrect.

Frankly, no.  I don't want to be going round endlessly writing help
texts.

We need the effort to be properly distributed - we need those who
_know_ the feature they're developing to write a proper help text.
One way to achieve that is to make a proper informative help text
a pre-requisit of accepting the code.

The quality of the help text is just as important as the quality of
the code, and we really should be paying the same amount of attention
to both.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: Smarter Kconfig help

[Randy Dunlap]

On 3/6/19 12:22 PM, Russell King - ARM Linux admin wrote:
> On Wed, Mar 06, 2019 at 09:16:02PM +0100, Enrico Weigelt, metux IT consult wrote:
>> On 06.03.19 13:42, Russell King - ARM Linux admin wrote:
>>
>>> In case it isn't clear, this is the *exact* point here - I don't know> whether this option should be enabled for iMX6 or not, and the only>
>> way I found out was to grep the dts files in arch/arm/boot/dts for> the
>> driver's compatible string.  What that reveals is that *no* 32-bit> dts
>> files contain the compatible string, and so I summise that *no*> 32-bit
>> iMX SoC should have this driver enabled.
>> The problem is a bit more generic. I often have to spend lots of time
>> to find out which configs to enable on a specific board, to get certain
>> features (eg. network, sata, display, gpu, ...). Even worse: many
>> options require other stuff enabled before even showing up. And when
>> disabling unneeded stuff, it leaves lots of other things enabled.
>> (we don't have some `apt autoremove` kconfig counterpart :().
>>
>> I've decided to cope w/ that on a higher level and written a little
>> config generator tool for that - here you can enable high level
>> features (eg. 'network' or 'display', etc) and it will generate the
>> actual .config:
>>
>> 	https://github.com/metux/kmct
>>
>>> The excuse that "we can't list the explicit SoCs" to me seems to be> a very lame excuse
>>
>> Maybe this actually means "nobody here volounteered to actually maintain
>> these help texts" ?
>>
>>> The best that I can come up with right now, given what little I know> from grepping the 32-bit DTS files, is that the help text should at>
>> least indicate that it *isn't* applicable to 32-bit SoCs, or if you>
>> prefer, *is* applicable to 64-bit SoCs.  Beyond that, I have no>
>> information to formulate a better suggestion.
>> Perhaps just fix the text based on your knowledge and send a patch to
>> the maintainers. They'll propably tell you if it's incorrect.
> 
> Frankly, no.  I don't want to be going round endlessly writing help
> texts.
> 
> We need the effort to be properly distributed - we need those who
> _know_ the feature they're developing to write a proper help text.
> One way to achieve that is to make a proper informative help text
> a pre-requisit of accepting the code.

Ack.

> The quality of the help text is just as important as the quality of
> the code, and we really should be paying the same amount of attention
> to both.

It goes much further than this IMHO, such as:

- #including the needed header files and not #including header files that
  are not used.

- using the correct Kconfig dependencies and selects

- testing builds with multiple kconfig settings (as applicable)

- builds should be clean, and that means without newly added warnings as
  well as build errors

I.e., the build testing that the 0day kernel robot does and that Arnd
and I do and that a few other people do should not catch nearly as many build
problems as they do.  The developer who knows the code should put due
diligence into the entire package, not just the (driver) source code and
building/testing with one default config.


Documentation/process/submit-checklist.rst has a more thorough list.


-- 
~Randy

Re: Smarter Kconfig help

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2303 bytes --]

On Wed 2019-03-06 10:45:52, Lucas Stach wrote:
> Hi Russell,
> 
> Am Dienstag, den 05.03.2019, 17:31 +0000 schrieb Russell King - ARM Linux admin:
> > Guys,
> > 
> > We need to be smarter when writing Kconfig help.  I'm just going
> > through updating my build trees with the results of 5.0 development,
> > and a number of the help texts are next to useless.  For example,
> > 
> > PVPANIC - is this something that should be enabled for a host or
> > guest kernel?  Answer: you have to read the driver code to find out.
> > 
> > IMX_IRQSTEER - which i.MX SoCs does this apply to?  The help text
> > just says:
> > 
> >   "Support for the i.MX IRQSTEER interrupt multiplexer/remapper."
> > 
> > which doesn't say which SoCs this should be enabled for - it turns
> > out that grepping for the driver's DT compatible string, none of
> > the 32-bit ARM cores have support for this, yet we still default
> > it to enabled there.  It seems the help text should at the very
> > least tell the user that this is not applicable to i.MX SoCs with
> > 32-bit ARM cores.
> > 
> > I'm sure there's many other instances of this... I suspect that
> > it's caused by review concentrating mostly on the technical aspects
> > of the code and the Kconfig help text just gets forgotten about.
> > 
> > Can we at least improve these two options please?
> 
> While I totally agree that the irqsteer driver should only default Y on
> 64bit i.MX SoCs, I'm not really sure what to do about the Kconfig help
> text.
> 
> Enumerating SoCs in the Kconfig of a IP block driver is always prone to
> get outdated. Just as the first example I've been able to come up with,
> the MMC_SDHCI_ESDHC_IMX help text says: "This selects the Freescale
> eSDHC/uSDHC controller support found on i.MX25, i.MX35 i.MX5x and
> i.MX6x.", while the same IP block is to be found on i.MX7 and various
> i.MX8.

"This selects the Freescale eSDHC/uSDHC controller support found on
i.MX25, i.MX35 i.MX5x, i.MX6x and later SoCs.".

Really, there are way too many silicon blocks, and you can't expect
Kconfig user to know them all...

Best regards,

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]