Re: Smarter Kconfig help
On Wed 2019-03-06 10:45:52, Lucas Stach wrote: > Hi Russell, > > Am Dienstag, den 05.03.2019, 17:31 + 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 signature.asc Description: Digital signature
Re: Smarter Kconfig help
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
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
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 i...@metux.net -- +49-151-27565287
Re: Smarter Kconfig help
Hi Mark, On Wed, Mar 06, 2019 at 09:35:21AM +, Mark Rutland wrote: > On Tue, Mar 05, 2019 at 05:31:12PM +, 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
On Wed, Mar 06, 2019 at 11:34:36AM +, 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 > > wrote: > > > On Wed, Mar 06, 2019 at 10:45:52AM +0100, Lucas Stach wrote: > > > > Am Dienstag, den 05.03.2019, 17:31 + 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
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 > wrote: > > On Wed, Mar 06, 2019 at 10:45:52AM +0100, Lucas Stach wrote: > > > Am Dienstag, den 05.03.2019, 17:31 + 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
On Wed, Mar 6, 2019 at 11:52 AM Russell King - ARM Linux admin wrote: > On Wed, Mar 06, 2019 at 10:45:52AM +0100, Lucas Stach wrote: > > Am Dienstag, den 05.03.2019, 17:31 + 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
On Wed, Mar 06, 2019 at 10:45:52AM +0100, Lucas Stach wrote: > Hi Russell, > > Am Dienstag, den 05.03.2019, 17:31 + 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
Hi Russell, Am Dienstag, den 05.03.2019, 17:31 + 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
On Tue, Mar 05, 2019 at 05:31:12PM +, 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.
Smarter Kconfig help
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
