| Message ID | 20210928235635.1348330-1-willmcvicker@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | arm64: Kconfig: Update ARCH_EXYNOS select configs | expand |
On 29/09/2021 01:56, Will McVicker wrote: > This is v2 of the series of patches that modularizes a number of core > ARCH_EXYNOS drivers. Based off of the feedback from the v1 series, I have > modularized all of the drivers that are removed from the ARCH_EXYNOS > series of "select XXX". This includes setting the following configs as > tristate: > > * COMMON_CLK_SAMSUNG > * EXYNOS_ARM64_COMMON_CLK > * PINCTRL_SAMSUNG > * PINCTRL_EXYNOS > * EXYNOS_PMU_ARM64 > * EXYNOS_PM_DOMAINS > > Additionally, it introduces the config EXYNOS_PMU_ARM64 and EXYNOS_PMU_ARM > which was previously EXYNOS_PMU and EXYNOS_PMU_ARM_DRIVERS respectively. > The reason for these new configs is because we are not able to easily > modularize the ARMv7 PMU driver due to built-in arch dependencies on > pmu_base_addr under arch/arm/mach-exynos/*. So the new configs split up > the ARM and ARM64 portions into two separate configs. > > Overall, these drivers didn't require much refactoring and converted to > modules relatively easily. However, due to my lack of exynos hardware, I > was not able to boot test these changes. I'm mostly concerned about the > CLK_OF_DECLARE() changes having dependencies on early timers. So I'm > requesting help for testing these changes on the respective hardware. > These are all not tested at all? In such case, since these are not trivial changes, please mark the series as RFT. I will not be able to test these for some days, so it must wait. Best regards, Krzysztof
On Wed, Sep 29, 2021 at 6:02 AM Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 29/09/2021 01:56, Will McVicker wrote: > > This is v2 of the series of patches that modularizes a number of core > > ARCH_EXYNOS drivers. Based off of the feedback from the v1 series, I have > > modularized all of the drivers that are removed from the ARCH_EXYNOS > > series of "select XXX". This includes setting the following configs as > > tristate: > > > > * COMMON_CLK_SAMSUNG > > * EXYNOS_ARM64_COMMON_CLK > > * PINCTRL_SAMSUNG > > * PINCTRL_EXYNOS > > * EXYNOS_PMU_ARM64 > > * EXYNOS_PM_DOMAINS > > > > Additionally, it introduces the config EXYNOS_PMU_ARM64 and EXYNOS_PMU_ARM > > which was previously EXYNOS_PMU and EXYNOS_PMU_ARM_DRIVERS respectively. > > The reason for these new configs is because we are not able to easily > > modularize the ARMv7 PMU driver due to built-in arch dependencies on > > pmu_base_addr under arch/arm/mach-exynos/*. So the new configs split up > > the ARM and ARM64 portions into two separate configs. > > > > Overall, these drivers didn't require much refactoring and converted to > > modules relatively easily. However, due to my lack of exynos hardware, I > > was not able to boot test these changes. I'm mostly concerned about the > > CLK_OF_DECLARE() changes having dependencies on early timers. So I'm > > requesting help for testing these changes on the respective hardware. > > > > These are all not tested at all? In such case, since these are not > trivial changes, please mark the series as RFT. > > I will not be able to test these for some days, so it must wait. > > > Best regards, > Krzysztof +Cc Arnd and Olof, Hi Krzysztof, To avoid the scrambled conversation from the first patchset, I'm going to address all your general questions here in the cover letter thread so that it's easier for everyone to follow and reference in the future. >What is more, it seems you entirely ignored Geert's comments. I pointed >attention to it last time and you just said you will send v2 instead of >joining discussion. > >It's a NAK for this reason - ignoring what Geert brought: you just broke >distro configs for Exynos. First off I did want to chime into the discussion from the previous patchset, but I felt that Lee and Saravana addressed all your concerns regarding the intent and feasibility. You also made it clear what the next steps were that I needed to take. >Please also explain why Exynos is so special that we deviate from the >policy for all SoC that critical SoC-related drivers have to be enabled >(built-in or as module). I am not actually changing ANY default build configurations here and I'm not removing any existing configuration. I tried to make it pretty clear in my original patch series commit messages that none of my changes modify the default behavior. The .config is the same with and without my patches. All of these drivers remain enabled as built-in. So if there is a distro that requires all of these drivers to be built-in, then they can continue as is without noticing any difference. IOW, all of these changes are/should be backwards compatible. I really appreciate yours and John Stultz's comments regarding including the "why" in my commit message wording. I will spend more time on the next series on trying to write a more meaningful commit message, but before that we can surely discuss the "why" here. As mentioned by Lee and Saravana, our common goal is to make it easier for everyone to contribute upstream. In particular, this series of patches is laying the ground work for distros to have more flexibility in supporting a wider range of platforms without forcing everyone to include unnecessary drivers. You said that upstream supports a generic kernel, but I argue that the upstream "generic" arm64 kernel can't be considered generic if it builds in SoC specific drivers that can be modules. This patch series is addressing exactly that -- allow distros to move SoC specific drivers out of the core kernel and into modules. Ultimately, our goal is to be able to directly develop with the upstream kernel on new and old SoCs by not including SoC specific drivers in our generic kernel distro. This helps the upstream community in a number of ways: (1) It makes the ARM64 generic kernel smaller by converting more drivers into modules (2) It makes it a lot easier for everyone to develop upstream if they can directly use the upstream kernel without carrying downstream changes. >Even if there was, I think it is good to have dependencies like >ARCH_EXYNOS, as they let us partition the (19000, as Arnd said recently) >Kconfig symbols into better manageable groups. Without these, we cannot >do better than "depends on ARM || ARM64 || COMPILE_TEST". My patch series still keeps the dependencies on ARCH_EXYNOS. I am totally fine with "depends on ARCH_EXYNOS" and totally fine with "default ARCH_EXYNOS". The problem we have is that ARCH_EXYNOS forcefully selects SoC specific drivers to be built-in because it just adds more and more SoC-specific drivers to a generic kernel. I know you are asking for me to only push changes that have proven to work. The theory behind these changes has been proven downstream on other devices and I'm more than willing to help debug any issues that arise out of this patch series, but since I don't have the hardware myself I do need help with device testing these changes. We are not trying to trick upstream in anyway to accept something that is not functional or going to hurt the upstream community. I am more than willing to help upstream and am totally willing to work with upstream to verify all of these changes before they are accepted (feel free to send me any dusty, unused hardware lying around if you want the extra help with device testing). I hope that helps clarifies things! I will address other patch specific comments in those threads as well. Thanks, Will
On 29/09/2021 21:48, Will McVicker wrote: > On Wed, Sep 29, 2021 at 6:02 AM Krzysztof Kozlowski > <krzysztof.kozlowski@canonical.com> wrote: >> >> On 29/09/2021 01:56, Will McVicker wrote: >>> This is v2 of the series of patches that modularizes a number of core >>> ARCH_EXYNOS drivers. Based off of the feedback from the v1 series, I have >>> modularized all of the drivers that are removed from the ARCH_EXYNOS >>> series of "select XXX". This includes setting the following configs as >>> tristate: >>> >>> * COMMON_CLK_SAMSUNG >>> * EXYNOS_ARM64_COMMON_CLK >>> * PINCTRL_SAMSUNG >>> * PINCTRL_EXYNOS >>> * EXYNOS_PMU_ARM64 >>> * EXYNOS_PM_DOMAINS >>> >>> Additionally, it introduces the config EXYNOS_PMU_ARM64 and EXYNOS_PMU_ARM >>> which was previously EXYNOS_PMU and EXYNOS_PMU_ARM_DRIVERS respectively. >>> The reason for these new configs is because we are not able to easily >>> modularize the ARMv7 PMU driver due to built-in arch dependencies on >>> pmu_base_addr under arch/arm/mach-exynos/*. So the new configs split up >>> the ARM and ARM64 portions into two separate configs. >>> >>> Overall, these drivers didn't require much refactoring and converted to >>> modules relatively easily. However, due to my lack of exynos hardware, I >>> was not able to boot test these changes. I'm mostly concerned about the >>> CLK_OF_DECLARE() changes having dependencies on early timers. So I'm >>> requesting help for testing these changes on the respective hardware. >>> >> >> These are all not tested at all? In such case, since these are not >> trivial changes, please mark the series as RFT. >> >> I will not be able to test these for some days, so it must wait. >> >> >> Best regards, >> Krzysztof > > +Cc Arnd and Olof, > > Hi Krzysztof, > > To avoid the scrambled conversation from the first patchset, I'm going > to address all your general questions here in the cover letter thread > so that it's easier for everyone to follow and reference in the > future. > >> What is more, it seems you entirely ignored Geert's comments. I pointed >> attention to it last time and you just said you will send v2 instead of >> joining discussion. >> >> It's a NAK for this reason - ignoring what Geert brought: you just broke >> distro configs for Exynos. > > First off I did want to chime into the discussion from the previous > patchset, but I felt that Lee and Saravana addressed all your concerns > regarding the intent and feasibility. You also made it clear what the > next steps were that I needed to take. One of the steps was problem with distros using everything as modules. They should not receive these drivers as modules. Reminder: these are essential drivers and all Exynos platforms must have them as built-in (at least till someone really tests this on multiple setups). > >> Please also explain why Exynos is so special that we deviate from the >> policy for all SoC that critical SoC-related drivers have to be enabled >> (built-in or as module). > > I am not actually changing ANY default build configurations here and > I'm not removing any existing configuration. You are changing not default, but selectability which is part of the enforced configuration to make platforms working. The distros do not always choose defaults but rather all as modules. Kernel configuration is huge and complex, so by mistake they could now even disable potentially essential driver. There is no need to disable for example essential clock driver on a supported Exynos platform. > I tried to make it pretty > clear in my original patch series commit messages that none of my > changes modify the default behavior. The .config is the same with and > without my patches. All of these drivers remain enabled as built-in. > So if there is a distro that requires all of these drivers to be > built-in, then they can continue as is without noticing any > difference. IOW, all of these changes are/should be backwards > compatible. I was not referring to default neither to backwards compatibility. Please explain why Exynos is special that it does not require essential drivers to be selected as built-in. For example why aren't same changes done for Renesas? Is that now a new global approach that all SoC drivers should be allowed to be disabled for ARCH_XXX? > > I really appreciate yours and John Stultz's comments regarding > including the "why" in my commit message wording. I will spend more > time on the next series on trying to write a more meaningful commit > message, but before that we can surely discuss the "why" here. > > As mentioned by Lee and Saravana, our common goal is to make it easier > for everyone to contribute upstream. In particular, this series of > patches is laying the ground work for distros to have more flexibility > in supporting a wider range of platforms without forcing everyone to > include unnecessary drivers. The drivers are usually necessary. Actually, you admitted you didn't test patchset, so how do you even know that they are unnecessary? How do you judge? > You said that upstream supports a generic > kernel, but I argue that the upstream "generic" arm64 kernel can't be > considered generic if it builds in SoC specific drivers that can be > modules. Good point, but since having them as modules was not tested, I consider it as theoretical topic. > This patch series is addressing exactly that -- allow distros > to move SoC specific drivers out of the core kernel and into modules. > Ultimately, our goal is to be able to directly develop with the > upstream kernel on new and old SoCs by not including SoC specific > drivers in our generic kernel distro. This helps the upstream > community in a number of ways: > > (1) It makes the ARM64 generic kernel smaller by converting more > drivers into modules > (2) It makes it a lot easier for everyone to develop upstream if they > can directly use the upstream kernel without carrying downstream > changes. I don't understand the point (2) here. Anyone can use upstream kernel for supported and unsupported platforms. How upstream benefits from a change affecting supported platforms made for unsupported, downstream platforms. > >> Even if there was, I think it is good to have dependencies like >> ARCH_EXYNOS, as they let us partition the (19000, as Arnd said recently) >> Kconfig symbols into better manageable groups. Without these, we cannot >> do better than "depends on ARM || ARM64 || COMPILE_TEST". > > My patch series still keeps the dependencies on ARCH_EXYNOS. I am > totally fine with "depends on ARCH_EXYNOS" and totally fine with > "default ARCH_EXYNOS". The problem we have is that ARCH_EXYNOS > forcefully selects SoC specific drivers to be built-in because it just > adds more and more SoC-specific drivers to a generic kernel. The selected drivers are essential for supported platforms. We don't even know what are these unsupported, downstream platforms you want customize kernel for. They cannot be audited, cannot be compared. Therefore I don't agree with calling it a "problem" that we select *necessary* drivers for supported platforms. It's by design - supported platforms should receive them without ability to remove. If you want to change it, let me paste from previous discussion: Affecting upstream platforms just because vendor/downstream does not want to mainline some code is unacceptable. Please upstream your drivers and DTS. Everyone else are working like this. NXP, Renesas, Xilinx, TI, Rockchip, AllWinner. Samsung or Google is not special to receive an exception for this. > > I know you are asking for me to only push changes that have proven to > work. Yep, tested. > The theory behind these changes has been proven downstream on > other devices and I'm more than willing to help debug any issues that > arise out of this patch series, but since I don't have the hardware > myself I do need help with device testing these changes. Downstream uses very specific Linux "distro" or fork - Android - with changes not present in others. Although tests on other (unsupported/unupstreamed) devices is good, but it's not sufficient. For example I guess that half or most of Odroid devices are running standard Linux distro (Arch, Ubuntu). You also mentioned downstream devices but without actually ever defining them. Please be more specific. What SoC, what hardware? Best regards, Krzysztof
On Thu, Sep 30, 2021 at 8:15 AM Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > On 29/09/2021 21:48, Will McVicker wrote: > > On Wed, Sep 29, 2021 at 6:02 AM Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > >> What is more, it seems you entirely ignored Geert's comments. I pointed > >> attention to it last time and you just said you will send v2 instead of > >> joining discussion. > >> > >> It's a NAK for this reason - ignoring what Geert brought: you just broke > >> distro configs for Exynos. > > > > First off I did want to chime into the discussion from the previous > > patchset, but I felt that Lee and Saravana addressed all your concerns > > regarding the intent and feasibility. You also made it clear what the > > next steps were that I needed to take. > > One of the steps was problem with distros using everything as modules. > They should not receive these drivers as modules. > Reminder: these are essential drivers and all Exynos platforms must have > them as built-in (at least till someone really tests this on multiple > setups). Agreed. I absolutely love the work of the GKI developers to turn more drivers into loadable modules, but the "correctness-first" principle is not up for negotiation. If you are uncomfortable with the code or the amount of testing because you think it breaks something, you should reject the patches. Moving core platform functionality is fundamentally hard and it can go wrong in all possible ways where it used to work by accident because the init order was fixed. > >> Please also explain why Exynos is so special that we deviate from the > >> policy for all SoC that critical SoC-related drivers have to be enabled > >> (built-in or as module). > > > > I am not actually changing ANY default build configurations here and > > I'm not removing any existing configuration. > > You are changing not default, but selectability which is part of the > enforced configuration to make platforms working. The distros do not > always choose defaults but rather all as modules. Kernel configuration > is huge and complex, so by mistake they could now even disable > potentially essential driver. There is no need to disable for example > essential clock driver on a supported Exynos platform. I'm not overly worried about the defaults. If the drivers work as loadable modules, I'm happy with them being loadable modules in distros. If they don't work this way, then the patches are broken and should not get merged. I don't even mind having essential drivers that can be turned off, since we already have a ton of those (e.g. serial ports on most platforms). It's up to distros to know which drivers to enable, though having either reasonable defaults or fail-safe Kconfig dependencies (e.g. making it impossible to turn off but allowing modules) is clearly best. > > I tried to make it pretty > > clear in my original patch series commit messages that none of my > > changes modify the default behavior. The .config is the same with and > > without my patches. All of these drivers remain enabled as built-in. > > So if there is a distro that requires all of these drivers to be > > built-in, then they can continue as is without noticing any > > difference. IOW, all of these changes are/should be backwards > > compatible. > > I was not referring to default neither to backwards compatibility. > Please explain why Exynos is special that it does not require essential > drivers to be selected as built-in. For example why aren't same changes > done for Renesas? > > Is that now a new global approach that all SoC drivers should be allowed > to be disabled for ARCH_XXX? I wouldn't enforce it either way across platforms. I would prefer drivers to be loadable modules where possible (and tested), rather than selected by the platform Kconfig. If you want to ensure the exynos drivers are impossible to turn into a nonworking state, that's up to you. > > You said that upstream supports a generic > > kernel, but I argue that the upstream "generic" arm64 kernel can't be > > considered generic if it builds in SoC specific drivers that can be > > modules. > > Good point, but since having them as modules was not tested, I consider > it as theoretical topic. I actually disagree strongly with labelling the kernel as "non-generic" just because it requires platform specific support to be built-in rather than a loadable module. This has never been possible on any platform I'm aware of, and likely never will, except for minor variations of an existing platform. Look at x86 as an example: there are less than a dozen SoC platforms supported and they are incredibly similar hardware-wise, but the kernel is anything but "generic" in the sense that was mentioned above. Most of the platform specific drivers in arch/x86/platform and the corresponding bits in drivers/{irqchip,clocksource,iommu} are always built-in, and a lot more is hardwired in architecture code as PCI quirks or conditional on cpuid or dmi firmware checks. > >> Even if there was, I think it is good to have dependencies like > >> ARCH_EXYNOS, as they let us partition the (19000, as Arnd said recently) > >> Kconfig symbols into better manageable groups. Without these, we cannot > >> do better than "depends on ARM || ARM64 || COMPILE_TEST". > > > > My patch series still keeps the dependencies on ARCH_EXYNOS. I am > > totally fine with "depends on ARCH_EXYNOS" and totally fine with > > "default ARCH_EXYNOS". The problem we have is that ARCH_EXYNOS > > forcefully selects SoC specific drivers to be built-in because it just > > adds more and more SoC-specific drivers to a generic kernel. > > The selected drivers are essential for supported platforms. We don't > even know what are these unsupported, downstream platforms you want > customize kernel for. They cannot be audited, cannot be compared. > > Therefore I don't agree with calling it a "problem" that we select > *necessary* drivers for supported platforms. It's by design - supported > platforms should receive them without ability to remove. > > If you want to change it, let me paste from previous discussion: > > Affecting upstream platforms just because vendor/downstream does not > want to mainline some code is unacceptable. Please upstream your drivers > and DTS. Agreed. I understand that it would be convenient for SoC vendors to never have to upstream their platform code again, and that Android would benefit from this in the short run. From my upstream perspective, this is absolutely a non-goal. If it becomes easier as a side-effect of making the kernel more modular, that's fine. The actual goal should be to get more people to contribute upstream so devices run code that has been reviewed and integrated into new kernels. > > I know you are asking for me to only push changes that have proven to > > work. > > Yep, tested. I'm generally fine with "obviously correct" ones as well, but it's up to you to categorize them ;-) Arnd
I've taken the liberty of cherry-picking some of the points you have reiteratted a few times. Hopefully I can help to address them adequently. On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > Reminder: these are essential drivers and all Exynos platforms must have > them as built-in (at least till someone really tests this on multiple > setups). > Therefore I don't agree with calling it a "problem" that we select > *necessary* drivers for supported platforms. It's by design - supported > platforms should receive them without ability to remove. > The selected drivers are essential for supported platforms. SoC specific drivers are only essential/necessary/required in images designed to execute solely on a platform that requires them. For a kernel image which is designed to be generic i.e. one that has the ability to boot on vast array of platforms, the drivers simply have to be *available*. Forcing all H/W drivers that are only *potentially* utilised on *some* platforms as core binary built-ins doesn't make any technical sense. The two most important issues this causes are image size and a lack of configurability/flexibility relating to real-world application i.e. the one issue we already agreed upon; H/W or features that are too new (pre-release). Bloating a generic kernel with potentially hundreds of unnecessary drivers that will never be executed in the vast majority of instances doesn't achieve anything. If we have a kernel image that has the ability to boot on 10's of architectures which have 10's of platforms each, that's a whole host of unused/wasted executable space. In order for vendors to work more closely with upstream, they need the ability to over-ride a *few* drivers to supplement them with some functionality which they believe provides them with a competitive edge (I think you called this "value-add" before) prior to the release of a device. This is a requirement that cannot be worked around. By insisting on drivers (most of which will not be executed in the vast majority of cases) to be built-in, you are insisting on a downstream kernel fork, which all of us would like to avoid [0]. [0] Full disclosure: part of my role at Linaro is to keep the Android kernel running as close to Mainline as possible and encourage/push the upstream-first mantra, hence my involvement with this and other sets. I assure you all intentions are good and honourable. If you haven't already seen it, please see Todd's most recent update on the goals and status of GKI: Article: https://tinyurl.com/saaen3sp Video: https://youtu.be/O_lCFGinFPM > We don't even know what are these unsupported, downstream platforms > you want customize kernel for. They cannot be audited, cannot be > compared. Affecting upstream platforms just because > vendor/downstream does not want to mainline some code is > unacceptable. Please upstream your drivers and DTS. > You also mentioned downstream devices but without actually ever defining > them. Please be more specific. What SoC, what hardware? Accepting changes based on the proviso that vendors upstream all of their work-in-progress solutions is an unfair position. We already discussed why upstreaming support for bleeding edge H/W and functionality is unrealistic in terms of competitive advantage. Besides, we might not be talking about new silicon at all (I don't believe anyone alluded to that). The flexibility in configuration simply allows for generic upstream drivers to be swapped out for ones which may have more or slightly different functionality (that can't be publicly shared until release). > Please explain why Exynos is special that it does not require essential > drivers to be selected as built-in. For example why aren't same changes > done for Renesas? > Everyone else are working like this. NXP, Renesas, Xilinx, TI, Rockchip, > AllWinner. Samsung or Google is not special to receive an exception for > this. Exynos isn't special in this regard. This applies to any vendor who releases Android images and wishes to be solve all of the issues the GKI project addresses (please read the article above for more about this). I truly hope this has helped to align my thoughts with yours.
On Thu, 30 Sep 2021, Arnd Bergmann wrote: > On Thu, Sep 30, 2021 at 8:15 AM Krzysztof Kozlowski > <krzysztof.kozlowski@canonical.com> wrote: > > On 29/09/2021 21:48, Will McVicker wrote: > > > On Wed, Sep 29, 2021 at 6:02 AM Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > >> What is more, it seems you entirely ignored Geert's comments. I pointed > > >> attention to it last time and you just said you will send v2 instead of > > >> joining discussion. > > >> > > >> It's a NAK for this reason - ignoring what Geert brought: you just broke > > >> distro configs for Exynos. > > > > > > First off I did want to chime into the discussion from the previous > > > patchset, but I felt that Lee and Saravana addressed all your concerns > > > regarding the intent and feasibility. You also made it clear what the > > > next steps were that I needed to take. > > > > One of the steps was problem with distros using everything as modules. > > They should not receive these drivers as modules. > > Reminder: these are essential drivers and all Exynos platforms must have > > them as built-in (at least till someone really tests this on multiple > > setups). > > Agreed. I absolutely love the work of the GKI developers to turn more > drivers into loadable modules, but the "correctness-first" principle is > not up for negotiation. If you are uncomfortable with the code or the > amount of testing because you think it breaks something, you should > reject the patches. Moving core platform functionality is fundamentally > hard and it can go wrong in all possible ways where it used to work > by accident because the init order was fixed. > > > >> Please also explain why Exynos is so special that we deviate from the > > >> policy for all SoC that critical SoC-related drivers have to be enabled > > >> (built-in or as module). > > > > > > I am not actually changing ANY default build configurations here and > > > I'm not removing any existing configuration. > > > > You are changing not default, but selectability which is part of the > > enforced configuration to make platforms working. The distros do not > > always choose defaults but rather all as modules. Kernel configuration > > is huge and complex, so by mistake they could now even disable > > potentially essential driver. There is no need to disable for example > > essential clock driver on a supported Exynos platform. > > I'm not overly worried about the defaults. If the drivers work as loadable > modules, I'm happy with them being loadable modules in distros. > If they don't work this way, then the patches are broken and should > not get merged. > > I don't even mind having essential drivers that can be turned off, > since we already have a ton of those (e.g. serial ports on most platforms). > It's up to distros to know which drivers to enable, though having > either reasonable defaults or fail-safe Kconfig dependencies (e.g. > making it impossible to turn off but allowing modules) is clearly > best. > > > > I tried to make it pretty > > > clear in my original patch series commit messages that none of my > > > changes modify the default behavior. The .config is the same with and > > > without my patches. All of these drivers remain enabled as built-in. > > > So if there is a distro that requires all of these drivers to be > > > built-in, then they can continue as is without noticing any > > > difference. IOW, all of these changes are/should be backwards > > > compatible. > > > > I was not referring to default neither to backwards compatibility. > > Please explain why Exynos is special that it does not require essential > > drivers to be selected as built-in. For example why aren't same changes > > done for Renesas? > > > > Is that now a new global approach that all SoC drivers should be allowed > > to be disabled for ARCH_XXX? > > I wouldn't enforce it either way across platforms. I would prefer drivers > to be loadable modules where possible (and tested), rather than > selected by the platform Kconfig. If you want to ensure the exynos > drivers are impossible to turn into a nonworking state, that's up to you. > > > > You said that upstream supports a generic > > > kernel, but I argue that the upstream "generic" arm64 kernel can't be > > > considered generic if it builds in SoC specific drivers that can be > > > modules. > > > > Good point, but since having them as modules was not tested, I consider > > it as theoretical topic. > > I actually disagree strongly with labelling the kernel as "non-generic" > just because it requires platform specific support to be built-in rather than > a loadable module. This has never been possible on any platform > I'm aware of, and likely never will, except for minor variations of > an existing platform. > > Look at x86 as an example: there are less than a dozen SoC platforms > supported and they are incredibly similar hardware-wise, but the kernel > is anything but "generic" in the sense that was mentioned above. > Most of the platform specific drivers in arch/x86/platform and the > corresponding bits in drivers/{irqchip,clocksource,iommu} are always > built-in, and a lot more is hardwired in architecture code as PCI > quirks or conditional on cpuid or dmi firmware checks. > > > >> Even if there was, I think it is good to have dependencies like > > >> ARCH_EXYNOS, as they let us partition the (19000, as Arnd said recently) > > >> Kconfig symbols into better manageable groups. Without these, we cannot > > >> do better than "depends on ARM || ARM64 || COMPILE_TEST". > > > > > > My patch series still keeps the dependencies on ARCH_EXYNOS. I am > > > totally fine with "depends on ARCH_EXYNOS" and totally fine with > > > "default ARCH_EXYNOS". The problem we have is that ARCH_EXYNOS > > > forcefully selects SoC specific drivers to be built-in because it just > > > adds more and more SoC-specific drivers to a generic kernel. > > > > The selected drivers are essential for supported platforms. We don't > > even know what are these unsupported, downstream platforms you want > > customize kernel for. They cannot be audited, cannot be compared. > > > > Therefore I don't agree with calling it a "problem" that we select > > *necessary* drivers for supported platforms. It's by design - supported > > platforms should receive them without ability to remove. > > > > If you want to change it, let me paste from previous discussion: > > > > Affecting upstream platforms just because vendor/downstream does not > > want to mainline some code is unacceptable. Please upstream your drivers > > and DTS. > > Agreed. I understand that it would be convenient for SoC vendors to > never have to upstream their platform code again, and that Android > would benefit from this in the short run. > > From my upstream perspective, this is absolutely a non-goal. If it becomes > easier as a side-effect of making the kernel more modular, that's fine. > The actual goal should be to get more people to contribute upstream so > devices run code that has been reviewed and integrated into new kernels. > > > > I know you are asking for me to only push changes that have proven to > > > work. > > > > Yep, tested. > > I'm generally fine with "obviously correct" ones as well, but it's up to > you to categorize them ;-) Arnd, FWIW, I agree with all of your points. Krzysztof, It sounds like a lack of testing is your main concern. How can I help here? What H/W do I need to be able to fully test this?
On Thu, Sep 30, 2021 at 11:01 AM Arnd Bergmann <arnd@arndb.de> wrote: > On Thu, Sep 30, 2021 at 8:15 AM Krzysztof Kozlowski > <krzysztof.kozlowski@canonical.com> wrote: > > On 29/09/2021 21:48, Will McVicker wrote: > > > On Wed, Sep 29, 2021 at 6:02 AM Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > >> What is more, it seems you entirely ignored Geert's comments. I pointed > > >> attention to it last time and you just said you will send v2 instead of > > >> joining discussion. > > >> > > >> It's a NAK for this reason - ignoring what Geert brought: you just broke > > >> distro configs for Exynos. > > > > > > First off I did want to chime into the discussion from the previous > > > patchset, but I felt that Lee and Saravana addressed all your concerns > > > regarding the intent and feasibility. You also made it clear what the > > > next steps were that I needed to take. > > > > One of the steps was problem with distros using everything as modules. > > They should not receive these drivers as modules. > > Reminder: these are essential drivers and all Exynos platforms must have > > them as built-in (at least till someone really tests this on multiple > > setups). > > Agreed. I absolutely love the work of the GKI developers to turn more > drivers into loadable modules, but the "correctness-first" principle is > not up for negotiation. If you are uncomfortable with the code or the > amount of testing because you think it breaks something, you should > reject the patches. Moving core platform functionality is fundamentally > hard and it can go wrong in all possible ways where it used to work > by accident because the init order was fixed. Exactly. And patches to change this that haven't been tested at all should be very clearly marked that way. > > > You said that upstream supports a generic > > > kernel, but I argue that the upstream "generic" arm64 kernel can't be > > > considered generic if it builds in SoC specific drivers that can be > > > modules. > > > > Good point, but since having them as modules was not tested, I consider > > it as theoretical topic. > > I actually disagree strongly with labelling the kernel as "non-generic" > just because it requires platform specific support to be built-in rather than > a loadable module. This has never been possible on any platform > I'm aware of, and likely never will, except for minor variations of > an existing platform. There seem to be two different meanings for a "generic" kernel: 1. To me, a generic kernel is a kernel that can boot on a variety of hardware platforms. and will operate correctly with all expected functionality. That is, it should include[1] drivers for all that hardware. Examples of this are arm/multi_v7_defconfig and arm64/defconfig. 2. To GKI, a generic kernel is a minimal kernel that doesn't include any[2] hardware-specific drivers, and has loadable modules for all[2] hardware-specific drivers. Before loading these modules, it can run an application on the CPU, but it cannot do any I/O to real hardware. [1] Developers usually want the drivers builtin, distros want them modular. [2] On arm/arm64, this must rely on the GIC and architectured timer being built-in (AFAIK these cannot be modules yet), and thus precludes booting on Cortex A9 and older that lack GIC and/or architectured timer as their primary interrupt controller and timer. I'm wondering how this works on other architectures with less-standardized interrupt controllers and timers? > Look at x86 as an example: there are less than a dozen SoC platforms > supported and they are incredibly similar hardware-wise, but the kernel > is anything but "generic" in the sense that was mentioned above. > Most of the platform specific drivers in arch/x86/platform and the > corresponding bits in drivers/{irqchip,clocksource,iommu} are always > built-in, and a lot more is hardwired in architecture code as PCI > quirks or conditional on cpuid or dmi firmware checks. Indeed. X86 has less variability in the hardware than other architectures, and has all the critical drivers built-in or in firmware. Where there have been big differences (remember Voyager?), x86 never had the option to have a generic (in the meaning of 1 above) kernel (but James did try, cfr. https://lwn.net/Articles/328469/). Gr{oetje,eeting}s, Geert
Hi Lee, On Thu, Sep 30, 2021 at 11:23 AM Lee Jones <lee.jones@linaro.org> wrote: > I've taken the liberty of cherry-picking some of the points you have > reiteratted a few times. Hopefully I can help to address them > adequently. > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > Reminder: these are essential drivers and all Exynos platforms must have > > them as built-in (at least till someone really tests this on multiple > > setups). > > > Therefore I don't agree with calling it a "problem" that we select > > *necessary* drivers for supported platforms. It's by design - supported > > platforms should receive them without ability to remove. > > > The selected drivers are essential for supported platforms. > > SoC specific drivers are only essential/necessary/required in > images designed to execute solely on a platform that requires them. Why? > For a kernel image which is designed to be generic i.e. one that has > the ability to boot on vast array of platforms, the drivers simply > have to be *available*. If the drivers are really essential/necessary/required, this precludes running the generic kernel image on the platform that requires them, making the kernel not sufficiently generic. > Forcing all H/W drivers that are only *potentially* utilised on *some* > platforms as core binary built-ins doesn't make any technical sense. > The two most important issues this causes are image size and a lack of > configurability/flexibility relating to real-world application i.e. > the one issue we already agreed upon; H/W or features that are too > new (pre-release). True, if "potentially". If not potentially, they must be included. > Bloating a generic kernel with potentially hundreds of unnecessary > drivers that will never be executed in the vast majority of instances > doesn't achieve anything. If we have a kernel image that has the > ability to boot on 10's of architectures which have 10's of platforms > each, that's a whole host of unused/wasted executable space. The key here is if the driver is required or not to use the platform, and why it is required. If the requirement comes from some deficiency in the kernel code or config system, it should be fixed, if possible. And the fix should be tested. If it cannot be fixed, the driver should be included, else it would preclude running the generic kernel on the affected platform. > In order for vendors to work more closely with upstream, they need the > ability to over-ride a *few* drivers to supplement them with some > functionality which they believe provides them with a competitive edge > (I think you called this "value-add" before) prior to the release of a > device. This is a requirement that cannot be worked around. > > By insisting on drivers (most of which will not be executed in the > vast majority of cases) to be built-in, you are insisting on a > downstream kernel fork, which all of us would like to avoid [0]. > > [0] Full disclosure: part of my role at Linaro is to keep the Android > kernel running as close to Mainline as possible and encourage/push the > upstream-first mantra, hence my involvement with this and other sets. > I assure you all intentions are good and honourable. If you haven't > already seen it, please see Todd's most recent update on the goals and > status of GKI: > > Article: https://tinyurl.com/saaen3sp > Video: https://youtu.be/O_lCFGinFPM > > > We don't even know what are these unsupported, downstream platforms > > you want customize kernel for. They cannot be audited, cannot be > > compared. Affecting upstream platforms just because > > vendor/downstream does not want to mainline some code is > > unacceptable. Please upstream your drivers and DTS. > > > You also mentioned downstream devices but without actually ever defining > > them. Please be more specific. What SoC, what hardware? > > Accepting changes based on the proviso that vendors upstream all of > their work-in-progress solutions is an unfair position. We already > discussed why upstreaming support for bleeding edge H/W and > functionality is unrealistic in terms of competitive advantage. > > Besides, we might not be talking about new silicon at all (I don't > believe anyone alluded to that). The flexibility in configuration > simply allows for generic upstream drivers to be swapped out for ones > which may have more or slightly different functionality (that can't be > publicly shared until release). The replacement drivers are already a downstream kernel fork, which you would like to avoid? Gr{oetje,eeting}s, Geert
On 30/09/2021 11:30, Lee Jones wrote: > On Thu, 30 Sep 2021, Arnd Bergmann wrote: > >> On Thu, Sep 30, 2021 at 8:15 AM Krzysztof Kozlowski >> <krzysztof.kozlowski@canonical.com> wrote: >>> On 29/09/2021 21:48, Will McVicker wrote: >>>> On Wed, Sep 29, 2021 at 6:02 AM Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: >>>>> What is more, it seems you entirely ignored Geert's comments. I pointed >>>>> attention to it last time and you just said you will send v2 instead of >>>>> joining discussion. >>>>> >>>>> It's a NAK for this reason - ignoring what Geert brought: you just broke >>>>> distro configs for Exynos. >>>> >>>> First off I did want to chime into the discussion from the previous >>>> patchset, but I felt that Lee and Saravana addressed all your concerns >>>> regarding the intent and feasibility. You also made it clear what the >>>> next steps were that I needed to take. >>> >>> One of the steps was problem with distros using everything as modules. >>> They should not receive these drivers as modules. >>> Reminder: these are essential drivers and all Exynos platforms must have >>> them as built-in (at least till someone really tests this on multiple >>> setups). >> >> Agreed. I absolutely love the work of the GKI developers to turn more >> drivers into loadable modules, but the "correctness-first" principle is >> not up for negotiation. If you are uncomfortable with the code or the >> amount of testing because you think it breaks something, you should >> reject the patches. Moving core platform functionality is fundamentally >> hard and it can go wrong in all possible ways where it used to work >> by accident because the init order was fixed. >> >>>>> Please also explain why Exynos is so special that we deviate from the >>>>> policy for all SoC that critical SoC-related drivers have to be enabled >>>>> (built-in or as module). >>>> >>>> I am not actually changing ANY default build configurations here and >>>> I'm not removing any existing configuration. >>> >>> You are changing not default, but selectability which is part of the >>> enforced configuration to make platforms working. The distros do not >>> always choose defaults but rather all as modules. Kernel configuration >>> is huge and complex, so by mistake they could now even disable >>> potentially essential driver. There is no need to disable for example >>> essential clock driver on a supported Exynos platform. >> >> I'm not overly worried about the defaults. If the drivers work as loadable >> modules, I'm happy with them being loadable modules in distros. >> If they don't work this way, then the patches are broken and should >> not get merged. >> >> I don't even mind having essential drivers that can be turned off, >> since we already have a ton of those (e.g. serial ports on most platforms). >> It's up to distros to know which drivers to enable, though having >> either reasonable defaults or fail-safe Kconfig dependencies (e.g. >> making it impossible to turn off but allowing modules) is clearly >> best. >> >>>> I tried to make it pretty >>>> clear in my original patch series commit messages that none of my >>>> changes modify the default behavior. The .config is the same with and >>>> without my patches. All of these drivers remain enabled as built-in. >>>> So if there is a distro that requires all of these drivers to be >>>> built-in, then they can continue as is without noticing any >>>> difference. IOW, all of these changes are/should be backwards >>>> compatible. >>> >>> I was not referring to default neither to backwards compatibility. >>> Please explain why Exynos is special that it does not require essential >>> drivers to be selected as built-in. For example why aren't same changes >>> done for Renesas? >>> >>> Is that now a new global approach that all SoC drivers should be allowed >>> to be disabled for ARCH_XXX? >> >> I wouldn't enforce it either way across platforms. I would prefer drivers >> to be loadable modules where possible (and tested), rather than >> selected by the platform Kconfig. If you want to ensure the exynos >> drivers are impossible to turn into a nonworking state, that's up to you. >> >>>> You said that upstream supports a generic >>>> kernel, but I argue that the upstream "generic" arm64 kernel can't be >>>> considered generic if it builds in SoC specific drivers that can be >>>> modules. >>> >>> Good point, but since having them as modules was not tested, I consider >>> it as theoretical topic. >> >> I actually disagree strongly with labelling the kernel as "non-generic" >> just because it requires platform specific support to be built-in rather than >> a loadable module. This has never been possible on any platform >> I'm aware of, and likely never will, except for minor variations of >> an existing platform. >> >> Look at x86 as an example: there are less than a dozen SoC platforms >> supported and they are incredibly similar hardware-wise, but the kernel >> is anything but "generic" in the sense that was mentioned above. >> Most of the platform specific drivers in arch/x86/platform and the >> corresponding bits in drivers/{irqchip,clocksource,iommu} are always >> built-in, and a lot more is hardwired in architecture code as PCI >> quirks or conditional on cpuid or dmi firmware checks. >> >>>>> Even if there was, I think it is good to have dependencies like >>>>> ARCH_EXYNOS, as they let us partition the (19000, as Arnd said recently) >>>>> Kconfig symbols into better manageable groups. Without these, we cannot >>>>> do better than "depends on ARM || ARM64 || COMPILE_TEST". >>>> >>>> My patch series still keeps the dependencies on ARCH_EXYNOS. I am >>>> totally fine with "depends on ARCH_EXYNOS" and totally fine with >>>> "default ARCH_EXYNOS". The problem we have is that ARCH_EXYNOS >>>> forcefully selects SoC specific drivers to be built-in because it just >>>> adds more and more SoC-specific drivers to a generic kernel. >>> >>> The selected drivers are essential for supported platforms. We don't >>> even know what are these unsupported, downstream platforms you want >>> customize kernel for. They cannot be audited, cannot be compared. >>> >>> Therefore I don't agree with calling it a "problem" that we select >>> *necessary* drivers for supported platforms. It's by design - supported >>> platforms should receive them without ability to remove. >>> >>> If you want to change it, let me paste from previous discussion: >>> >>> Affecting upstream platforms just because vendor/downstream does not >>> want to mainline some code is unacceptable. Please upstream your drivers >>> and DTS. >> >> Agreed. I understand that it would be convenient for SoC vendors to >> never have to upstream their platform code again, and that Android >> would benefit from this in the short run. >> >> From my upstream perspective, this is absolutely a non-goal. If it becomes >> easier as a side-effect of making the kernel more modular, that's fine. >> The actual goal should be to get more people to contribute upstream so >> devices run code that has been reviewed and integrated into new kernels. >> >>>> I know you are asking for me to only push changes that have proven to >>>> work. >>> >>> Yep, tested. >> >> I'm generally fine with "obviously correct" ones as well, but it's up to >> you to categorize them ;-) Thanks Arnd! > > Arnd, > > FWIW, I agree with all of your points.> > Krzysztof, > > It sounds like a lack of testing is your main concern. > > How can I help here? What H/W do I need to be able to fully test this? The changes here need to be tested on affected platforms (ARMv7 and ARMv8), when built as a modules on some types of regular distros (e.g. Arch, Ubuntu). From each of such boot I would be happy to see number of new dmesg warnings/errors plus number of probe deferrals. Since the drivers could be switched to modules (and some distros might do it), they might be hit by surprise regressions in boot performance due to probe deferrals. This should be also checked on these platforms. Geert pointed out before that clocks in many cases are not optional - driver needs them and will wait defer. Assuming of course that boot succeeds. Minor differences in boot speed should not be a problem, I think, because distro anyway chosen all-module approach so it accepts the penalty. Best regards, Krzysztof
On 30/09/2021 11:23, Lee Jones wrote: > I've taken the liberty of cherry-picking some of the points you have > reiteratted a few times. Hopefully I can help to address them > adequently. > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: >> Reminder: these are essential drivers and all Exynos platforms must have >> them as built-in (at least till someone really tests this on multiple >> setups). > >> Therefore I don't agree with calling it a "problem" that we select >> *necessary* drivers for supported platforms. It's by design - supported >> platforms should receive them without ability to remove. > >> The selected drivers are essential for supported platforms. > > SoC specific drivers are only essential/necessary/required in > images designed to execute solely on a platform that requires them. > For a kernel image which is designed to be generic i.e. one that has > the ability to boot on vast array of platforms, the drivers simply > have to be *available*. By "essential", I meant drivers which are needed for supported platform to boot properly. Also without significant performance penalty due to probe deferrals. Therefore if someone selects ARCH_EXYNOS in mainline kernel, it means he/she wants such devices to be working fine with generic kernel. This is the difference with term "drivers have to be available". > > Forcing all H/W drivers that are only *potentially* utilised on *some* > platforms as core binary built-ins doesn't make any technical sense. > The two most important issues this causes are image size and a lack of > configurability/flexibility relating to real-world application i.e. > the one issue we already agreed upon; H/W or features that are too > new (pre-release). Geert responded here. If drivers are essential for supported platforms to boot, having them built-in has technical sense. For other drivers not and we do not discuss such cases. > > Bloating a generic kernel with potentially hundreds of unnecessary > drivers that will never be executed in the vast majority of instances > doesn't achieve anything. If we have a kernel image that has the > ability to boot on 10's of architectures which have 10's of platforms > each, that's a whole host of unused/wasted executable space. Geert responded here. > > In order for vendors to work more closely with upstream, they need the > ability to over-ride a *few* drivers to supplement them with some > functionality which they believe provides them with a competitive edge > (I think you called this "value-add" before) prior to the release of a > device. This is a requirement that cannot be worked around. > > By insisting on drivers (most of which will not be executed in the > vast majority of cases) to be built-in, you are insisting on a > downstream kernel fork, which all of us would like to avoid [0]. The same with all the DTS and hundreds of drivers you keep out of tree. > > [0] Full disclosure: part of my role at Linaro is to keep the Android > kernel running as close to Mainline as possible and encourage/push the > upstream-first mantra, hence my involvement with this and other sets. > I assure you all intentions are good and honourable. If you haven't > already seen it, please see Todd's most recent update on the goals and > status of GKI: > > Article: https://tinyurl.com/saaen3sp > Video: https://youtu.be/O_lCFGinFPM > >> We don't even know what are these unsupported, downstream platforms >> you want customize kernel for. They cannot be audited, cannot be >> compared. Affecting upstream platforms just because >> vendor/downstream does not want to mainline some code is >> unacceptable. Please upstream your drivers and DTS. > >> You also mentioned downstream devices but without actually ever defining >> them. Please be more specific. What SoC, what hardware? > > Accepting changes based on the proviso that vendors upstream all of > their work-in-progress solutions is an unfair position. You twisted (or ignored) my words here. I said before - sacrificing any mainline platform (e.g. reliable boot for distro) for an out-of-tree vendor which does no want to upstream drivers is the unfair position. One of the incentives of upstreaming is to be able to shape kernel and be considered in kernel upstream decisions. That's the privileged for upstreamed platforms - they have an impact. Vendor decides to stay out - fine, vendor's choice. You loose impact. Any sad words like "oh upstream does not want to change for me" should receive a message: "please join the upstream :), so we will change together". > We already > discussed why upstreaming support for bleeding edge H/W and > functionality is unrealistic in terms of competitive advantage. Nope, my last point was not responded to. You said that there is no point for vendor to upstream bleeding edge HW. Then you said that there is also little point to upstream older HW. Basically following this approach you agree that vendor does not have to do anything because it is inconvenient for him. However upstream has to adapt to downstream vendor, right? No, this is *unfair to all the platforms we upstreamed*. > > Besides, we might not be talking about new silicon at all (I don't > believe anyone alluded to that). The flexibility in configuration > simply allows for generic upstream drivers to be swapped out for ones > which may have more or slightly different functionality (that can't be > publicly shared until release). > >> Please explain why Exynos is special that it does not require essential >> drivers to be selected as built-in. For example why aren't same changes >> done for Renesas? > >> Everyone else are working like this. NXP, Renesas, Xilinx, TI, Rockchip, >> AllWinner. Samsung or Google is not special to receive an exception for >> this. > > Exynos isn't special in this regard. This applies to any vendor who > releases Android images and wishes to be solve all of the issues the > GKI project addresses (please read the article above for more about > this). We have then slightly different goals, because you are driven by Android release images and this is why you are less interested in NXP and Renesas. Only some vendors should receive such changes? No, in upstream we are not focusing on any specific distro and there are other uses of Exynos (and NXP and Renesas) platforms. Therefore the choice/policy and overall design tries to match all vendors, not only some subset convenient to Android. If Android (or some vendor) wants to have exception for a specific driver/platform, it must do it in upstream way, by contributing, not by changing to match downstream needs while ignoring other users. Best regards, Krzysztof
On Thu, 30 Sep 2021, Geert Uytterhoeven wrote: > Hi Lee, > > On Thu, Sep 30, 2021 at 11:23 AM Lee Jones <lee.jones@linaro.org> wrote: > > I've taken the liberty of cherry-picking some of the points you have > > reiteratted a few times. Hopefully I can help to address them > > adequently. > > > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > > Reminder: these are essential drivers and all Exynos platforms must have > > > them as built-in (at least till someone really tests this on multiple > > > setups). > > > > > Therefore I don't agree with calling it a "problem" that we select > > > *necessary* drivers for supported platforms. It's by design - supported > > > platforms should receive them without ability to remove. > > > > > The selected drivers are essential for supported platforms. > > > > SoC specific drivers are only essential/necessary/required in > > images designed to execute solely on a platform that requires them. > > Why? Because without them the image wouldn't functional on any level. But you're right, there is still no requirement for it to be built-in. > > For a kernel image which is designed to be generic i.e. one that has > > the ability to boot on vast array of platforms, the drivers simply > > have to be *available*. > > If the drivers are really essential/necessary/required, this precludes > running the generic kernel image on the platform that requires them, > making the kernel not sufficiently generic. If they are not at all present, then yes. However that is not what is being suggested. The essential functionality will be provided. Just not built-in. > > Forcing all H/W drivers that are only *potentially* utilised on *some* > > platforms as core binary built-ins doesn't make any technical sense. > > The two most important issues this causes are image size and a lack of > > configurability/flexibility relating to real-world application i.e. > > the one issue we already agreed upon; H/W or features that are too > > new (pre-release). > > True, if "potentially". If not potentially, they must be included. I'm not sure what you're trying to say here. Would you mind elaborating? > > Bloating a generic kernel with potentially hundreds of unnecessary > > drivers that will never be executed in the vast majority of instances > > doesn't achieve anything. If we have a kernel image that has the > > ability to boot on 10's of architectures which have 10's of platforms > > each, that's a whole host of unused/wasted executable space. > > The key here is if the driver is required or not to use the platform, > and why it is required. If the requirement comes from some deficiency > in the kernel code or config system, it should be fixed, if possible. > And the fix should be tested. > If it cannot be fixed, the driver should be included, else it would > preclude running the generic kernel on the affected platform. Sorry, I'm not following. > > In order for vendors to work more closely with upstream, they need the > > ability to over-ride a *few* drivers to supplement them with some > > functionality which they believe provides them with a competitive edge > > (I think you called this "value-add" before) prior to the release of a > > device. This is a requirement that cannot be worked around. > > > > By insisting on drivers (most of which will not be executed in the > > vast majority of cases) to be built-in, you are insisting on a > > downstream kernel fork, which all of us would like to avoid [0]. > > > > [0] Full disclosure: part of my role at Linaro is to keep the Android > > kernel running as close to Mainline as possible and encourage/push the > > upstream-first mantra, hence my involvement with this and other sets. > > I assure you all intentions are good and honourable. If you haven't > > already seen it, please see Todd's most recent update on the goals and > > status of GKI: > > > > Article: https://tinyurl.com/saaen3sp > > Video: https://youtu.be/O_lCFGinFPM > > > > > We don't even know what are these unsupported, downstream platforms > > > you want customize kernel for. They cannot be audited, cannot be > > > compared. Affecting upstream platforms just because > > > vendor/downstream does not want to mainline some code is > > > unacceptable. Please upstream your drivers and DTS. > > > > > You also mentioned downstream devices but without actually ever defining > > > them. Please be more specific. What SoC, what hardware? > > > > Accepting changes based on the proviso that vendors upstream all of > > their work-in-progress solutions is an unfair position. We already > > discussed why upstreaming support for bleeding edge H/W and > > functionality is unrealistic in terms of competitive advantage. > > > > Besides, we might not be talking about new silicon at all (I don't > > believe anyone alluded to that). The flexibility in configuration > > simply allows for generic upstream drivers to be swapped out for ones > > which may have more or slightly different functionality (that can't be > > publicly shared until release). > > The replacement drivers are already a downstream kernel fork, which you > would like to avoid? Avoid, yes, absolutely. However, in the real world of competitive tech, other than in extreme cases such as; fully open source, community driven, cute embedded hobby platforms, there will always be changes required on-top of upstream projects. Even as upstream kernel developers we need to understand and respect that.
2021年9月30日(木) 18:23 Lee Jones <lee.jones@linaro.org>: > > I've taken the liberty of cherry-picking some of the points you have > reiteratted a few times. Hopefully I can help to address them > adequently. > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > Reminder: these are essential drivers and all Exynos platforms must have > > them as built-in (at least till someone really tests this on multiple > > setups). > > > Therefore I don't agree with calling it a "problem" that we select > > *necessary* drivers for supported platforms. It's by design - supported > > platforms should receive them without ability to remove. > > > The selected drivers are essential for supported platforms. > > SoC specific drivers are only essential/necessary/required in > images designed to execute solely on a platform that requires them. > For a kernel image which is designed to be generic i.e. one that has > the ability to boot on vast array of platforms, the drivers simply > have to be *available*. > > Forcing all H/W drivers that are only *potentially* utilised on *some* > platforms as core binary built-ins doesn't make any technical sense. > The two most important issues this causes are image size and a lack of > configurability/flexibility relating to real-world application i.e. > the one issue we already agreed upon; H/W or features that are too > new (pre-release). > > Bloating a generic kernel with potentially hundreds of unnecessary > drivers that will never be executed in the vast majority of instances > doesn't achieve anything. If we have a kernel image that has the > ability to boot on 10's of architectures which have 10's of platforms > each, that's a whole host of unused/wasted executable space. > > In order for vendors to work more closely with upstream, they need the > ability to over-ride a *few* drivers to supplement them with some > functionality which they believe provides them with a competitive edge > (I think you called this "value-add" before) prior to the release of a > device. This is a requirement that cannot be worked around. [Chiming in as a clock driver sub-maintainer and someone who spent a non-insignificant part of his life on SoC driver bring-up - not as a Google employee.] I'd argue that the proper way for them to achieve it would be to extend the upstream frameworks and/or existing drivers with appropriate APIs to allow their downstream modules to plug into what's already available upstream. Best regards, Tomasz
Hi Lee, On Thu, Sep 30, 2021 at 12:56 PM Lee Jones <lee.jones@linaro.org> wrote: > On Thu, 30 Sep 2021, Geert Uytterhoeven wrote: > > On Thu, Sep 30, 2021 at 11:23 AM Lee Jones <lee.jones@linaro.org> wrote: > > > I've taken the liberty of cherry-picking some of the points you have > > > reiteratted a few times. Hopefully I can help to address them > > > adequently. > > > > > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > > > Reminder: these are essential drivers and all Exynos platforms must have > > > > them as built-in (at least till someone really tests this on multiple > > > > setups). > > > > > > > Therefore I don't agree with calling it a "problem" that we select > > > > *necessary* drivers for supported platforms. It's by design - supported > > > > platforms should receive them without ability to remove. > > > > > > > The selected drivers are essential for supported platforms. > > > > > > SoC specific drivers are only essential/necessary/required in > > > images designed to execute solely on a platform that requires them. > > > > Why? > > Because without them the image wouldn't functional on any level. > > But you're right, there is still no requirement for it to be built-in. > > > > For a kernel image which is designed to be generic i.e. one that has > > > the ability to boot on vast array of platforms, the drivers simply > > > have to be *available*. > > > > If the drivers are really essential/necessary/required, this precludes > > running the generic kernel image on the platform that requires them, > > making the kernel not sufficiently generic. > > If they are not at all present, then yes. However that is not what is > being suggested. The essential functionality will be provided. Just > not built-in. I really meant "essential/necessary/required to be built-in". > > > Forcing all H/W drivers that are only *potentially* utilised on *some* > > > platforms as core binary built-ins doesn't make any technical sense. > > > The two most important issues this causes are image size and a lack of > > > configurability/flexibility relating to real-world application i.e. > > > the one issue we already agreed upon; H/W or features that are too > > > new (pre-release). > > > > True, if "potentially". If not potentially, they must be included. > > I'm not sure what you're trying to say here. Would you mind elaborating? It was a comment to your "*potentially* utilised on *some* platforms". It is clear they are not used on the other ("not *some*") platforms, but your sentence was unclear whether they are always or only sometimes used on "*some*" platforms. "always" => "not potentially" "sometimes" => "potentially". I hope this makes it more clear. > > > Bloating a generic kernel with potentially hundreds of unnecessary > > > drivers that will never be executed in the vast majority of instances > > > doesn't achieve anything. If we have a kernel image that has the > > > ability to boot on 10's of architectures which have 10's of platforms > > > each, that's a whole host of unused/wasted executable space. > > > > The key here is if the driver is required or not to use the platform, > > and why it is required. If the requirement comes from some deficiency > > in the kernel code or config system, it should be fixed, if possible. > > And the fix should be tested. > > If it cannot be fixed, the driver should be included, else it would > > preclude running the generic kernel on the affected platform. > > Sorry, I'm not following. It all depends on why the driver is "required to be built-in". Depending on the reason behind that requirement, the driver can be changed from built-in to modular without ill effects on functionality. Gr{oetje,eeting}s, Geert
Hi Tomasz, On Thu, Sep 30, 2021 at 1:01 PM Tomasz Figa <tomasz.figa@gmail.com> wrote: > 2021年9月30日(木) 18:23 Lee Jones <lee.jones@linaro.org>: > > I've taken the liberty of cherry-picking some of the points you have > > reiteratted a few times. Hopefully I can help to address them > > adequently. > > > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > > Reminder: these are essential drivers and all Exynos platforms must have > > > them as built-in (at least till someone really tests this on multiple > > > setups). > > > > > Therefore I don't agree with calling it a "problem" that we select > > > *necessary* drivers for supported platforms. It's by design - supported > > > platforms should receive them without ability to remove. > > > > > The selected drivers are essential for supported platforms. > > > > SoC specific drivers are only essential/necessary/required in > > images designed to execute solely on a platform that requires them. > > For a kernel image which is designed to be generic i.e. one that has > > the ability to boot on vast array of platforms, the drivers simply > > have to be *available*. > > > > Forcing all H/W drivers that are only *potentially* utilised on *some* > > platforms as core binary built-ins doesn't make any technical sense. > > The two most important issues this causes are image size and a lack of > > configurability/flexibility relating to real-world application i.e. > > the one issue we already agreed upon; H/W or features that are too > > new (pre-release). > > > > Bloating a generic kernel with potentially hundreds of unnecessary > > drivers that will never be executed in the vast majority of instances > > doesn't achieve anything. If we have a kernel image that has the > > ability to boot on 10's of architectures which have 10's of platforms > > each, that's a whole host of unused/wasted executable space. > > > > In order for vendors to work more closely with upstream, they need the > > ability to over-ride a *few* drivers to supplement them with some > > functionality which they believe provides them with a competitive edge > > (I think you called this "value-add" before) prior to the release of a > > device. This is a requirement that cannot be worked around. > > [Chiming in as a clock driver sub-maintainer and someone who spent a > non-insignificant part of his life on SoC driver bring-up - not as a > Google employee.] > > I'd argue that the proper way for them to achieve it would be to > extend the upstream frameworks and/or existing drivers with > appropriate APIs to allow their downstream modules to plug into what's > already available upstream. Yes, that's one possible solution. We do have to be careful this would not just become some fishy API to touch deeply buried internals, and for which there are no valid use cases in upstream. Gr{oetje,eeting}s, Geert
On Thu, 30 Sep 2021, Tomasz Figa wrote: > 2021年9月30日(木) 18:23 Lee Jones <lee.jones@linaro.org>: > > > > I've taken the liberty of cherry-picking some of the points you have > > reiteratted a few times. Hopefully I can help to address them > > adequently. > > > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > > Reminder: these are essential drivers and all Exynos platforms must have > > > them as built-in (at least till someone really tests this on multiple > > > setups). > > > > > Therefore I don't agree with calling it a "problem" that we select > > > *necessary* drivers for supported platforms. It's by design - supported > > > platforms should receive them without ability to remove. > > > > > The selected drivers are essential for supported platforms. > > > > SoC specific drivers are only essential/necessary/required in > > images designed to execute solely on a platform that requires them. > > For a kernel image which is designed to be generic i.e. one that has > > the ability to boot on vast array of platforms, the drivers simply > > have to be *available*. > > > > Forcing all H/W drivers that are only *potentially* utilised on *some* > > platforms as core binary built-ins doesn't make any technical sense. > > The two most important issues this causes are image size and a lack of > > configurability/flexibility relating to real-world application i.e. > > the one issue we already agreed upon; H/W or features that are too > > new (pre-release). > > > > Bloating a generic kernel with potentially hundreds of unnecessary > > drivers that will never be executed in the vast majority of instances > > doesn't achieve anything. If we have a kernel image that has the > > ability to boot on 10's of architectures which have 10's of platforms > > each, that's a whole host of unused/wasted executable space. > > > > In order for vendors to work more closely with upstream, they need the > > ability to over-ride a *few* drivers to supplement them with some > > functionality which they believe provides them with a competitive edge > > (I think you called this "value-add" before) prior to the release of a > > device. This is a requirement that cannot be worked around. > > [Chiming in as a clock driver sub-maintainer and someone who spent a > non-insignificant part of his life on SoC driver bring-up - not as a > Google employee.] > > I'd argue that the proper way for them to achieve it would be to > extend the upstream frameworks and/or existing drivers with > appropriate APIs to allow their downstream modules to plug into what's > already available upstream. Is that the same as exporting symbols to framework APIs? Since this is already a method GKI uses to allow external modules to interact with the core kernel/frameworks. However, it's not possible to upstream these without an upstream user for each one.
On Thu, 30 Sep 2021, Geert Uytterhoeven wrote: > Hi Lee, > > On Thu, Sep 30, 2021 at 12:56 PM Lee Jones <lee.jones@linaro.org> wrote: > > On Thu, 30 Sep 2021, Geert Uytterhoeven wrote: > > > On Thu, Sep 30, 2021 at 11:23 AM Lee Jones <lee.jones@linaro.org> wrote: > > > > I've taken the liberty of cherry-picking some of the points you have > > > > reiteratted a few times. Hopefully I can help to address them > > > > adequently. > > > > > > > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > > > > Reminder: these are essential drivers and all Exynos platforms must have > > > > > them as built-in (at least till someone really tests this on multiple > > > > > setups). > > > > > > > > > Therefore I don't agree with calling it a "problem" that we select > > > > > *necessary* drivers for supported platforms. It's by design - supported > > > > > platforms should receive them without ability to remove. > > > > > > > > > The selected drivers are essential for supported platforms. > > > > > > > > SoC specific drivers are only essential/necessary/required in > > > > images designed to execute solely on a platform that requires them. > > > > > > Why? > > > > Because without them the image wouldn't functional on any level. > > > > But you're right, there is still no requirement for it to be built-in. > > > > > > For a kernel image which is designed to be generic i.e. one that has > > > > the ability to boot on vast array of platforms, the drivers simply > > > > have to be *available*. > > > > > > If the drivers are really essential/necessary/required, this precludes > > > running the generic kernel image on the platform that requires them, > > > making the kernel not sufficiently generic. > > > > If they are not at all present, then yes. However that is not what is > > being suggested. The essential functionality will be provided. Just > > not built-in. > > I really meant "essential/necessary/required to be built-in". Then I agree with you. My position is that if they don't *have* to be built-in, then why force it? > > > > Forcing all H/W drivers that are only *potentially* utilised on *some* > > > > platforms as core binary built-ins doesn't make any technical sense. > > > > The two most important issues this causes are image size and a lack of > > > > configurability/flexibility relating to real-world application i.e. > > > > the one issue we already agreed upon; H/W or features that are too > > > > new (pre-release). > > > > > > True, if "potentially". If not potentially, they must be included. > > > > I'm not sure what you're trying to say here. Would you mind elaborating? > > It was a comment to your "*potentially* utilised on *some* platforms". > It is clear they are not used on the other ("not *some*") platforms, but your > sentence was unclear whether they are always or only sometimes used on > "*some*" platforms. > "always" => "not potentially" > "sometimes" => "potentially". > > I hope this makes it more clear. Not really, but I'll try to clean mine up: The aim is to have a single kernel (image + modules) that can be booted on a plethora of platforms. For the sake of argument say 10. Let's also say that each of the platforms are equal and will be booted the same amount of times. Taking the example above, when I say that the H/W specific drivers will only be *potentially* utilised, I mean that they will only be bound and probed 1/10 times i.e. when booted on the associated platform. Which means that in the vast majority of boots (9/10) they will lie dormant, taking up unnecessary space. Another way to say this would be; the kernel needs to have the capability to boot all of the supported platforms, but it will only ever be utilised on one at a time. > > > > Bloating a generic kernel with potentially hundreds of unnecessary > > > > drivers that will never be executed in the vast majority of instances > > > > doesn't achieve anything. If we have a kernel image that has the > > > > ability to boot on 10's of architectures which have 10's of platforms > > > > each, that's a whole host of unused/wasted executable space. > > > > > > The key here is if the driver is required or not to use the platform, > > > and why it is required. If the requirement comes from some deficiency > > > in the kernel code or config system, it should be fixed, if possible. > > > And the fix should be tested. > > > If it cannot be fixed, the driver should be included, else it would > > > preclude running the generic kernel on the affected platform. > > > > Sorry, I'm not following. > > It all depends on why the driver is "required to be built-in". > Depending on the reason behind that requirement, the driver can be > changed from built-in to modular without ill effects on functionality. Absolutely. There are cases where drivers simply can't be built as modules. These unavoidable situations are legitimate use-cases and the technology/ code-base will have to work around these as required. The argument here is that if they can be separated and have been shown to work well in either use-case, then it is my opinion that placing an artificial barrier up based mostly on politics is not the correct approach.
2021年9月30日(木) 20:51 Lee Jones <lee.jones@linaro.org>: > > On Thu, 30 Sep 2021, Tomasz Figa wrote: > > > 2021年9月30日(木) 18:23 Lee Jones <lee.jones@linaro.org>: > > > > > > I've taken the liberty of cherry-picking some of the points you have > > > reiteratted a few times. Hopefully I can help to address them > > > adequently. > > > > > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > > > Reminder: these are essential drivers and all Exynos platforms must have > > > > them as built-in (at least till someone really tests this on multiple > > > > setups). > > > > > > > Therefore I don't agree with calling it a "problem" that we select > > > > *necessary* drivers for supported platforms. It's by design - supported > > > > platforms should receive them without ability to remove. > > > > > > > The selected drivers are essential for supported platforms. > > > > > > SoC specific drivers are only essential/necessary/required in > > > images designed to execute solely on a platform that requires them. > > > For a kernel image which is designed to be generic i.e. one that has > > > the ability to boot on vast array of platforms, the drivers simply > > > have to be *available*. > > > > > > Forcing all H/W drivers that are only *potentially* utilised on *some* > > > platforms as core binary built-ins doesn't make any technical sense. > > > The two most important issues this causes are image size and a lack of > > > configurability/flexibility relating to real-world application i.e. > > > the one issue we already agreed upon; H/W or features that are too > > > new (pre-release). > > > > > > Bloating a generic kernel with potentially hundreds of unnecessary > > > drivers that will never be executed in the vast majority of instances > > > doesn't achieve anything. If we have a kernel image that has the > > > ability to boot on 10's of architectures which have 10's of platforms > > > each, that's a whole host of unused/wasted executable space. > > > > > > In order for vendors to work more closely with upstream, they need the > > > ability to over-ride a *few* drivers to supplement them with some > > > functionality which they believe provides them with a competitive edge > > > (I think you called this "value-add" before) prior to the release of a > > > device. This is a requirement that cannot be worked around. > > > > [Chiming in as a clock driver sub-maintainer and someone who spent a > > non-insignificant part of his life on SoC driver bring-up - not as a > > Google employee.] > > > > I'd argue that the proper way for them to achieve it would be to > > extend the upstream frameworks and/or existing drivers with > > appropriate APIs to allow their downstream modules to plug into what's > > already available upstream. > > Is that the same as exporting symbols to framework APIs? > > Since this is already a method GKI uses to allow external modules to > interact with the core kernel/frameworks. However, it's not possible > to upstream these without an upstream user for each one. Not necessary the core frameworks, could also be changing the ways the existing drivers register to allow additional drivers to extend the functionality rather than completely overwrite them. It's really hard to tell what the right way would be without knowing the exact things they find missing in the upstream drivers. As for upstream users, this is exactly the point - upstream is a bidirectional effort, one takes from it and should contribute things back. Generally, the subsystems being mentioned here are so basic (clock, pinctrl, rtc), that I really can't imagine what kind of rocket science one might want to hide for competitive reasons... If it's for an entire SoC, I wonder why Intel and AMD don't have similar concerns and contribute support for their newest hardware far before the release.
On 30/09/2021 14:10, Tomasz Figa wrote: > 2021年9月30日(木) 20:51 Lee Jones <lee.jones@linaro.org>: >> >> On Thu, 30 Sep 2021, Tomasz Figa wrote: >> >> >> Is that the same as exporting symbols to framework APIs? >> >> Since this is already a method GKI uses to allow external modules to >> interact with the core kernel/frameworks. However, it's not possible >> to upstream these without an upstream user for each one. > > Not necessary the core frameworks, could also be changing the ways the > existing drivers register to allow additional drivers to extend the > functionality rather than completely overwrite them. Yes, the first user could be within the kernel after modifying some of the drivers. > It's really hard > to tell what the right way would be without knowing the exact things > they find missing in the upstream drivers. As for upstream users, this > is exactly the point - upstream is a bidirectional effort, one takes > from it and should contribute things back. > > Generally, the subsystems being mentioned here are so basic (clock, > pinctrl, rtc), that I really can't imagine what kind of rocket science > one might want to hide for competitive reasons... If it's for an > entire SoC, I wonder why Intel and AMD don't have similar concerns and > contribute support for their newest hardware far before the release. Lee used the argument of not-disclosing-edge-hw but I also don't see much of it in the case of few drivers needed to be overridden. Just bunch of registers for the same stuff we have sine 8 years. Rather the vendor does not want to commit effort towards upstreaming these... Best regards, Krzysztof
On 30/09/2021 11:23, Lee Jones wrote: > [0] Full disclosure: part of my role at Linaro is to keep the Android > kernel running as close to Mainline as possible and encourage/push the > upstream-first mantra, hence my involvement with this and other sets. > I assure you all intentions are good and honourable. If you haven't > already seen it, please see Todd's most recent update on the goals and > status of GKI: > > Article: https://tinyurl.com/saaen3sp > Video: https://youtu.be/O_lCFGinFPM > Side topic, why this patchset is in your scope or Will's/Google's scope? Just drop it from Android main kernel, it will not be your problem. I mean, really, you don't need this patchset in your tree at all. The only platform which needs it, the only platform which will loose something will be one specific vendor. Therefore this will be an incentive for them to join both discussions and upstream development. :) Best regards, Krzysztof
On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > On 30/09/2021 11:23, Lee Jones wrote: > > I've taken the liberty of cherry-picking some of the points you have > > reiteratted a few times. Hopefully I can help to address them > > adequently. > > > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > >> Reminder: these are essential drivers and all Exynos platforms must have > >> them as built-in (at least till someone really tests this on multiple > >> setups). > > > >> Therefore I don't agree with calling it a "problem" that we select > >> *necessary* drivers for supported platforms. It's by design - supported > >> platforms should receive them without ability to remove. > > > >> The selected drivers are essential for supported platforms. > > > > SoC specific drivers are only essential/necessary/required in > > images designed to execute solely on a platform that requires them. > > For a kernel image which is designed to be generic i.e. one that has > > the ability to boot on vast array of platforms, the drivers simply > > have to be *available*. > > By "essential", I meant drivers which are needed for supported platform > to boot properly. Yes, I know what you meant by essential. My comment still stands. As long as they are provided, it should work. > Also without significant performance penalty due to > probe deferrals. Agreed. We will try to find a way to test this. > > Forcing all H/W drivers that are only *potentially* utilised on *some* > > platforms as core binary built-ins doesn't make any technical sense. > > The two most important issues this causes are image size and a lack of > > configurability/flexibility relating to real-world application i.e. > > the one issue we already agreed upon; H/W or features that are too > > new (pre-release). > > Geert responded here. If drivers are essential for supported platforms > to boot, having them built-in has technical sense. We're going to have to agree to disagree here. None of us are currently aware of any technical reasons why these particular drivers have to be built-in. Hopefully we can prove this out with testing. > > By insisting on drivers (most of which will not be executed in the > > vast majority of cases) to be built-in, you are insisting on a > > downstream kernel fork, which all of us would like to avoid [0]. > > The same with all the DTS and hundreds of drivers you keep out of tree. I do not keep any drivers out of tree. :) > >> We don't even know what are these unsupported, downstream platforms > >> you want customize kernel for. They cannot be audited, cannot be > >> compared. Affecting upstream platforms just because > >> vendor/downstream does not want to mainline some code is > >> unacceptable. > >> > >> Please upstream your drivers and DTS. > > > > Accepting changes based on the proviso that vendors upstream all of > > their work-in-progress solutions is an unfair position. > > You twisted (or ignored) my words here. I said before - sacrificing any > mainline platform (e.g. reliable boot for distro) for an out-of-tree > vendor which does no want to upstream drivers is the unfair position. > One of the incentives of upstreaming is to be able to shape kernel and > be considered in kernel upstream decisions. That's the privileged for > upstreamed platforms - they have an impact. > > Vendor decides to stay out - fine, vendor's choice. You loose impact. > Any sad words like "oh upstream does not want to change for me" should > receive a message: "please join the upstream :), so we will change > together". No one is proposing to sacrifice anything. This change is believed to be benign. > > We already > > discussed why upstreaming support for bleeding edge H/W and > > functionality is unrealistic in terms of competitive advantage. > > Nope, my last point was not responded to. You said that there is no > point for vendor to upstream bleeding edge HW. Then you said that there > is also little point to upstream older HW. The driver in question is already upstream. It is my hope that the vendor will realign with Mainline and upstream the differences (providing the current competitive edge). Although I have no real influence in this regard, as I have no contact with them. > Basically following this approach you agree that vendor does not have to > do anything because it is inconvenient for him. > > However upstream has to adapt to downstream vendor, right? > > No, this is *unfair to all the platforms we upstreamed*. The point here is that this change should not be a determent to anyone. > > Exynos isn't special in this regard. This applies to any vendor who > > releases Android images and wishes to be solve all of the issues the > > GKI project addresses (please read the article above for more about > > this). > > We have then slightly different goals, because you are driven by Android > release images and this is why you are less interested in NXP and > Renesas. Only some vendors should receive such changes? No, in upstream > we are not focusing on any specific distro and there are other uses of > Exynos (and NXP and Renesas) platforms. Therefore the choice/policy and > overall design tries to match all vendors, not only some subset > convenient to Android. > > If Android (or some vendor) wants to have exception for a specific > driver/platform, it must do it in upstream way, by contributing, not by > changing to match downstream needs while ignoring other users. No users are being ignored. No damaging changes are being proposed. This has little to do with Android and everything to do with the possibly of a more generic kernel. Moving drivers out of platform code and into /drivers along with enabling components as modules has been an on-going effort for many years now. I'm struggling to see how this is different.
On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > It sounds like a lack of testing is your main concern. > > > > How can I help here? What H/W do I need to be able to fully test this? > > The changes here need to be tested on affected platforms (ARMv7 and > ARMv8), when built as a modules on some types of regular distros (e.g. > Arch, Ubuntu). From each of such boot I would be happy to see number of > new dmesg warnings/errors plus number of probe deferrals. > > Since the drivers could be switched to modules (and some distros might > do it), they might be hit by surprise regressions in boot performance > due to probe deferrals. This should be also checked on these platforms. > Geert pointed out before that clocks in many cases are not optional - > driver needs them and will wait defer. > > Assuming of course that boot succeeds. Minor differences in boot speed > should not be a problem, I think, because distro anyway chosen > all-module approach so it accepts the penalty. Do you have any suggestions in terms of devboards?
On 30/09/2021 14:34, Lee Jones wrote: > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: >>> It sounds like a lack of testing is your main concern. >>> >>> How can I help here? What H/W do I need to be able to fully test this? >> >> The changes here need to be tested on affected platforms (ARMv7 and >> ARMv8), when built as a modules on some types of regular distros (e.g. >> Arch, Ubuntu). From each of such boot I would be happy to see number of >> new dmesg warnings/errors plus number of probe deferrals. >> >> Since the drivers could be switched to modules (and some distros might >> do it), they might be hit by surprise regressions in boot performance >> due to probe deferrals. This should be also checked on these platforms. >> Geert pointed out before that clocks in many cases are not optional - >> driver needs them and will wait defer. >> >> Assuming of course that boot succeeds. Minor differences in boot speed >> should not be a problem, I think, because distro anyway chosen >> all-module approach so it accepts the penalty. > > Do you have any suggestions in terms of devboards? Minimal set: 1. Something with Exynos4 (e.g. Odroid U3 with Exynos4412). 2. Something with Exynos 5422/5800 (e.g. Odroid XU3/XU4/HC1/MC1 or Chromebook Peach Pi). 3. Exynos5433 - TM2 or TM2E. Boards are not widely available, so we need to rely on provided testing by Samsung. What would be good is to also test Exynos3 boards, but this is also not widely available. Best regards, Krzysztof
On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > On 30/09/2021 11:23, Lee Jones wrote: > > [0] Full disclosure: part of my role at Linaro is to keep the Android > > kernel running as close to Mainline as possible and encourage/push the > > upstream-first mantra, hence my involvement with this and other sets. > > I assure you all intentions are good and honourable. If you haven't > > already seen it, please see Todd's most recent update on the goals and > > status of GKI: > > > > Article: https://tinyurl.com/saaen3sp > > Video: https://youtu.be/O_lCFGinFPM > > > > Side topic, why this patchset is in your scope or Will's/Google's scope? > Just drop it from Android main kernel, it will not be your problem. I > mean, really, you don't need this patchset in your tree at all. The only > platform which needs it, the only platform which will loose something > will be one specific vendor. Therefore this will be an incentive for > them to join both discussions and upstream development. :) How would they fix this besides upstreaming support for unreleased work-in-progress H/W? Haven't I explained this several times already? :)
On Thu, 30 Sep 2021, Tomasz Figa wrote: > 2021年9月30日(木) 20:51 Lee Jones <lee.jones@linaro.org>: > > > > On Thu, 30 Sep 2021, Tomasz Figa wrote: > > > > > 2021年9月30日(木) 18:23 Lee Jones <lee.jones@linaro.org>: > > > > > > > > I've taken the liberty of cherry-picking some of the points you have > > > > reiteratted a few times. Hopefully I can help to address them > > > > adequently. > > > > > > > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > > > > Reminder: these are essential drivers and all Exynos platforms must have > > > > > them as built-in (at least till someone really tests this on multiple > > > > > setups). > > > > > > > > > Therefore I don't agree with calling it a "problem" that we select > > > > > *necessary* drivers for supported platforms. It's by design - supported > > > > > platforms should receive them without ability to remove. > > > > > > > > > The selected drivers are essential for supported platforms. > > > > > > > > SoC specific drivers are only essential/necessary/required in > > > > images designed to execute solely on a platform that requires them. > > > > For a kernel image which is designed to be generic i.e. one that has > > > > the ability to boot on vast array of platforms, the drivers simply > > > > have to be *available*. > > > > > > > > Forcing all H/W drivers that are only *potentially* utilised on *some* > > > > platforms as core binary built-ins doesn't make any technical sense. > > > > The two most important issues this causes are image size and a lack of > > > > configurability/flexibility relating to real-world application i.e. > > > > the one issue we already agreed upon; H/W or features that are too > > > > new (pre-release). > > > > > > > > Bloating a generic kernel with potentially hundreds of unnecessary > > > > drivers that will never be executed in the vast majority of instances > > > > doesn't achieve anything. If we have a kernel image that has the > > > > ability to boot on 10's of architectures which have 10's of platforms > > > > each, that's a whole host of unused/wasted executable space. > > > > > > > > In order for vendors to work more closely with upstream, they need the > > > > ability to over-ride a *few* drivers to supplement them with some > > > > functionality which they believe provides them with a competitive edge > > > > (I think you called this "value-add" before) prior to the release of a > > > > device. This is a requirement that cannot be worked around. > > > > > > [Chiming in as a clock driver sub-maintainer and someone who spent a > > > non-insignificant part of his life on SoC driver bring-up - not as a > > > Google employee.] > > > > > > I'd argue that the proper way for them to achieve it would be to > > > extend the upstream frameworks and/or existing drivers with > > > appropriate APIs to allow their downstream modules to plug into what's > > > already available upstream. > > > > Is that the same as exporting symbols to framework APIs? > > > > Since this is already a method GKI uses to allow external modules to > > interact with the core kernel/frameworks. However, it's not possible > > to upstream these without an upstream user for each one. > > Not necessary the core frameworks, could also be changing the ways the > existing drivers register to allow additional drivers to extend the > functionality rather than completely overwrite them. It's really hard > to tell what the right way would be without knowing the exact things > they find missing in the upstream drivers. As for upstream users, this > is exactly the point - upstream is a bidirectional effort, one takes > from it and should contribute things back. > > Generally, the subsystems being mentioned here are so basic (clock, > pinctrl, rtc), that I really can't imagine what kind of rocket science > one might want to hide for competitive reasons... If it's for an > entire SoC, I wonder why Intel and AMD don't have similar concerns and > contribute support for their newest hardware far before the release. I don't have visibility into the driver-overrides I'm afraid. I do know that code-space can be a problem though. So any way we can make the core binary smaller (i.e. remove anything that can be built as a module) will have a positive effect.
On 30/09/2021 14:39, Lee Jones wrote: > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > >> On 30/09/2021 11:23, Lee Jones wrote: >>> [0] Full disclosure: part of my role at Linaro is to keep the Android >>> kernel running as close to Mainline as possible and encourage/push the >>> upstream-first mantra, hence my involvement with this and other sets. >>> I assure you all intentions are good and honourable. If you haven't >>> already seen it, please see Todd's most recent update on the goals and >>> status of GKI: >>> >>> Article: https://tinyurl.com/saaen3sp >>> Video: https://youtu.be/O_lCFGinFPM >>> >> >> Side topic, why this patchset is in your scope or Will's/Google's scope? >> Just drop it from Android main kernel, it will not be your problem. I >> mean, really, you don't need this patchset in your tree at all. The only >> platform which needs it, the only platform which will loose something >> will be one specific vendor. Therefore this will be an incentive for >> them to join both discussions and upstream development. :) > > How would they fix this besides upstreaming support for unreleased > work-in-progress H/W? > > Haven't I explained this several times already? :) Either that way or the same as Will's doing but that's not my question. I understand you flush the queue of your GKI patches to be closer to upstream. Reduce the backlog/burden. you can achieve your goal by simply dropping such patch and making it not your problem. :) Best regards, Krzysztof
On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > On 30/09/2021 14:39, Lee Jones wrote: > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > > >> On 30/09/2021 11:23, Lee Jones wrote: > >>> [0] Full disclosure: part of my role at Linaro is to keep the Android > >>> kernel running as close to Mainline as possible and encourage/push the > >>> upstream-first mantra, hence my involvement with this and other sets. > >>> I assure you all intentions are good and honourable. If you haven't > >>> already seen it, please see Todd's most recent update on the goals and > >>> status of GKI: > >>> > >>> Article: https://tinyurl.com/saaen3sp > >>> Video: https://youtu.be/O_lCFGinFPM > >>> > >> > >> Side topic, why this patchset is in your scope or Will's/Google's scope? > >> Just drop it from Android main kernel, it will not be your problem. I > >> mean, really, you don't need this patchset in your tree at all. The only > >> platform which needs it, the only platform which will loose something > >> will be one specific vendor. Therefore this will be an incentive for > >> them to join both discussions and upstream development. :) > > > > How would they fix this besides upstreaming support for unreleased > > work-in-progress H/W? > > > > Haven't I explained this several times already? :) > > Either that way or the same as Will's doing but that's not my question. > I understand you flush the queue of your GKI patches to be closer to > upstream. Reduce the backlog/burden. you can achieve your goal by simply > dropping such patch and making it not your problem. :) git reset --hard mainline/master # job done - tea break :) Seriously though, we wish to encourage the use of GKI so all vendors can enjoy the benefits of more easily updateable/secure code-bases. I can't see how pushing back on seamlessly benign changes would benefit them or anyone else.
Hi Lee, On Thu, Sep 30, 2021 at 2:08 PM Lee Jones <lee.jones@linaro.org> wrote: > On Thu, 30 Sep 2021, Geert Uytterhoeven wrote: > > On Thu, Sep 30, 2021 at 12:56 PM Lee Jones <lee.jones@linaro.org> wrote: > > > On Thu, 30 Sep 2021, Geert Uytterhoeven wrote: > > > > On Thu, Sep 30, 2021 at 11:23 AM Lee Jones <lee.jones@linaro.org> wrote: > > > > > I've taken the liberty of cherry-picking some of the points you have > > > > > reiteratted a few times. Hopefully I can help to address them > > > > > adequently. > > > > > > > > > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > > > > > Reminder: these are essential drivers and all Exynos platforms must have > > > > > > them as built-in (at least till someone really tests this on multiple > > > > > > setups). > > > > > > > > > > > Therefore I don't agree with calling it a "problem" that we select > > > > > > *necessary* drivers for supported platforms. It's by design - supported > > > > > > platforms should receive them without ability to remove. > > > > > > > > > > > The selected drivers are essential for supported platforms. > > > > > > > > > > SoC specific drivers are only essential/necessary/required in > > > > > images designed to execute solely on a platform that requires them. > > > > > > > > Why? > > > > > > Because without them the image wouldn't functional on any level. > > > > > > But you're right, there is still no requirement for it to be built-in. > > > > > > > > For a kernel image which is designed to be generic i.e. one that has > > > > > the ability to boot on vast array of platforms, the drivers simply > > > > > have to be *available*. > > > > > > > > If the drivers are really essential/necessary/required, this precludes > > > > running the generic kernel image on the platform that requires them, > > > > making the kernel not sufficiently generic. > > > > > > If they are not at all present, then yes. However that is not what is > > > being suggested. The essential functionality will be provided. Just > > > not built-in. > > > > I really meant "essential/necessary/required to be built-in". > > Then I agree with you. My position is that if they don't *have* to be > built-in, then why force it? > > > > > > Forcing all H/W drivers that are only *potentially* utilised on *some* > > > > > platforms as core binary built-ins doesn't make any technical sense. > > > > > The two most important issues this causes are image size and a lack of > > > > > configurability/flexibility relating to real-world application i.e. > > > > > the one issue we already agreed upon; H/W or features that are too > > > > > new (pre-release). > > > > > > > > True, if "potentially". If not potentially, they must be included. > > > > > > I'm not sure what you're trying to say here. Would you mind elaborating? > > > > It was a comment to your "*potentially* utilised on *some* platforms". > > It is clear they are not used on the other ("not *some*") platforms, but your > > sentence was unclear whether they are always or only sometimes used on > > "*some*" platforms. > > "always" => "not potentially" > > "sometimes" => "potentially". > > > > I hope this makes it more clear. > > Not really, but I'll try to clean mine up: > > The aim is to have a single kernel (image + modules) that can be > booted on a plethora of platforms. For the sake of argument say 10. > Let's also say that each of the platforms are equal and will be booted > the same amount of times. > > Taking the example above, when I say that the H/W specific drivers > will only be *potentially* utilised, I mean that they will only be > bound and probed 1/10 times i.e. when booted on the associated > platform. Which means that in the vast majority of boots (9/10) they > will lie dormant, taking up unnecessary space. > > Another way to say this would be; the kernel needs to have the > capability to boot all of the supported platforms, but it will only > ever be utilised on one at a time. That's true even for drivers for "generic" hardware, right? E.g. arm64 selects ARM_GIC and ARM_GIC_V3, where most (all?) platforms have at most one of them. > > > > > Bloating a generic kernel with potentially hundreds of unnecessary > > > > > drivers that will never be executed in the vast majority of instances > > > > > doesn't achieve anything. If we have a kernel image that has the > > > > > ability to boot on 10's of architectures which have 10's of platforms > > > > > each, that's a whole host of unused/wasted executable space. > > > > > > > > The key here is if the driver is required or not to use the platform, > > > > and why it is required. If the requirement comes from some deficiency > > > > in the kernel code or config system, it should be fixed, if possible. > > > > And the fix should be tested. > > > > If it cannot be fixed, the driver should be included, else it would > > > > preclude running the generic kernel on the affected platform. > > > > > > Sorry, I'm not following. > > > > It all depends on why the driver is "required to be built-in". > > Depending on the reason behind that requirement, the driver can be > > changed from built-in to modular without ill effects on functionality. > > Absolutely. > > There are cases where drivers simply can't be built as modules. These > unavoidable situations are legitimate use-cases and the technology/ > code-base will have to work around these as required. > > The argument here is that if they can be separated and have been shown > to work well in either use-case, then it is my opinion that placing an > artificial barrier up based mostly on politics is not the correct > approach. Agreed. Gr{oetje,eeting}s, Geert
Hi Lee, On Thu, Sep 30, 2021 at 3:29 PM Lee Jones <lee.jones@linaro.org> wrote: > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > On 30/09/2021 14:39, Lee Jones wrote: > > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > >> On 30/09/2021 11:23, Lee Jones wrote: > > >>> [0] Full disclosure: part of my role at Linaro is to keep the Android > > >>> kernel running as close to Mainline as possible and encourage/push the > > >>> upstream-first mantra, hence my involvement with this and other sets. > > >>> I assure you all intentions are good and honourable. If you haven't > > >>> already seen it, please see Todd's most recent update on the goals and > > >>> status of GKI: > > >>> > > >>> Article: https://tinyurl.com/saaen3sp > > >>> Video: https://youtu.be/O_lCFGinFPM > > >>> > > >> > > >> Side topic, why this patchset is in your scope or Will's/Google's scope? > > >> Just drop it from Android main kernel, it will not be your problem. I > > >> mean, really, you don't need this patchset in your tree at all. The only > > >> platform which needs it, the only platform which will loose something > > >> will be one specific vendor. Therefore this will be an incentive for > > >> them to join both discussions and upstream development. :) > > > > > > How would they fix this besides upstreaming support for unreleased > > > work-in-progress H/W? > > > > > > Haven't I explained this several times already? :) > > > > Either that way or the same as Will's doing but that's not my question. > > I understand you flush the queue of your GKI patches to be closer to > > upstream. Reduce the backlog/burden. you can achieve your goal by simply > > dropping such patch and making it not your problem. :) > > git reset --hard mainline/master # job done - tea break :) > > Seriously though, we wish to encourage the use of GKI so all vendors > can enjoy the benefits of more easily updateable/secure code-bases. > > I can't see how pushing back on seamlessly benign changes would > benefit them or anyone else. I like your wording ;-) Indeed, seamlessly benign changes, which are (1) not tested, and (2) some believed by the platform maintainer to break the platform. What can possibly go wrong? ;-) Gr{oetje,eeting}s, Geert
On Thu, 30 Sep 2021, Geert Uytterhoeven wrote: > Hi Lee, > > On Thu, Sep 30, 2021 at 3:29 PM Lee Jones <lee.jones@linaro.org> wrote: > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > > On 30/09/2021 14:39, Lee Jones wrote: > > > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > > >> On 30/09/2021 11:23, Lee Jones wrote: > > > >>> [0] Full disclosure: part of my role at Linaro is to keep the Android > > > >>> kernel running as close to Mainline as possible and encourage/push the > > > >>> upstream-first mantra, hence my involvement with this and other sets. > > > >>> I assure you all intentions are good and honourable. If you haven't > > > >>> already seen it, please see Todd's most recent update on the goals and > > > >>> status of GKI: > > > >>> > > > >>> Article: https://tinyurl.com/saaen3sp > > > >>> Video: https://youtu.be/O_lCFGinFPM > > > >>> > > > >> > > > >> Side topic, why this patchset is in your scope or Will's/Google's scope? > > > >> Just drop it from Android main kernel, it will not be your problem. I > > > >> mean, really, you don't need this patchset in your tree at all. The only > > > >> platform which needs it, the only platform which will loose something > > > >> will be one specific vendor. Therefore this will be an incentive for > > > >> them to join both discussions and upstream development. :) > > > > > > > > How would they fix this besides upstreaming support for unreleased > > > > work-in-progress H/W? > > > > > > > > Haven't I explained this several times already? :) > > > > > > Either that way or the same as Will's doing but that's not my question. > > > I understand you flush the queue of your GKI patches to be closer to > > > upstream. Reduce the backlog/burden. you can achieve your goal by simply > > > dropping such patch and making it not your problem. :) > > > > git reset --hard mainline/master # job done - tea break :) > > > > Seriously though, we wish to encourage the use of GKI so all vendors > > can enjoy the benefits of more easily updateable/secure code-bases. > > > > I can't see how pushing back on seamlessly benign changes would > > benefit them or anyone else. > > I like your wording ;-) > > Indeed, seamlessly benign changes, which are (1) not tested, and (2) > some believed by the platform maintainer to break the platform. > What can possibly go wrong? ;-) William has already shown a willingness to test the series. There is already a downstream proof-of-concept of this working. I am hopeful. :)
Hi Lee, On Thu, Sep 30, 2021 at 6:21 PM Lee Jones <lee.jones@linaro.org> wrote: > On Thu, 30 Sep 2021, Geert Uytterhoeven wrote: > > On Thu, Sep 30, 2021 at 3:29 PM Lee Jones <lee.jones@linaro.org> wrote: > > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > > > On 30/09/2021 14:39, Lee Jones wrote: > > > > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > > > >> On 30/09/2021 11:23, Lee Jones wrote: > > > > >>> [0] Full disclosure: part of my role at Linaro is to keep the Android > > > > >>> kernel running as close to Mainline as possible and encourage/push the > > > > >>> upstream-first mantra, hence my involvement with this and other sets. > > > > >>> I assure you all intentions are good and honourable. If you haven't > > > > >>> already seen it, please see Todd's most recent update on the goals and > > > > >>> status of GKI: > > > > >>> > > > > >>> Article: https://tinyurl.com/saaen3sp > > > > >>> Video: https://youtu.be/O_lCFGinFPM > > > > >>> > > > > >> > > > > >> Side topic, why this patchset is in your scope or Will's/Google's scope? > > > > >> Just drop it from Android main kernel, it will not be your problem. I > > > > >> mean, really, you don't need this patchset in your tree at all. The only > > > > >> platform which needs it, the only platform which will loose something > > > > >> will be one specific vendor. Therefore this will be an incentive for > > > > >> them to join both discussions and upstream development. :) > > > > > > > > > > How would they fix this besides upstreaming support for unreleased > > > > > work-in-progress H/W? > > > > > > > > > > Haven't I explained this several times already? :) > > > > > > > > Either that way or the same as Will's doing but that's not my question. > > > > I understand you flush the queue of your GKI patches to be closer to > > > > upstream. Reduce the backlog/burden. you can achieve your goal by simply > > > > dropping such patch and making it not your problem. :) > > > > > > git reset --hard mainline/master # job done - tea break :) > > > > > > Seriously though, we wish to encourage the use of GKI so all vendors > > > can enjoy the benefits of more easily updateable/secure code-bases. > > > > > > I can't see how pushing back on seamlessly benign changes would > > > benefit them or anyone else. > > > > I like your wording ;-) > > > > Indeed, seamlessly benign changes, which are (1) not tested, and (2) > > some believed by the platform maintainer to break the platform. > > What can possibly go wrong? ;-) > > William has already shown a willingness to test the series. I'm looking forward to the results! Gr{oetje,eeting}s, Geert
On Thu, Sep 30, 2021 at 9:26 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Lee, > > On Thu, Sep 30, 2021 at 6:21 PM Lee Jones <lee.jones@linaro.org> wrote: > > On Thu, 30 Sep 2021, Geert Uytterhoeven wrote: > > > On Thu, Sep 30, 2021 at 3:29 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > > > > On 30/09/2021 14:39, Lee Jones wrote: > > > > > > On Thu, 30 Sep 2021, Krzysztof Kozlowski wrote: > > > > > >> On 30/09/2021 11:23, Lee Jones wrote: > > > > > >>> [0] Full disclosure: part of my role at Linaro is to keep the Android > > > > > >>> kernel running as close to Mainline as possible and encourage/push the > > > > > >>> upstream-first mantra, hence my involvement with this and other sets. > > > > > >>> I assure you all intentions are good and honourable. If you haven't > > > > > >>> already seen it, please see Todd's most recent update on the goals and > > > > > >>> status of GKI: > > > > > >>> > > > > > >>> Article: https://tinyurl.com/saaen3sp > > > > > >>> Video: https://youtu.be/O_lCFGinFPM > > > > > >>> > > > > > >> > > > > > >> Side topic, why this patchset is in your scope or Will's/Google's scope? > > > > > >> Just drop it from Android main kernel, it will not be your problem. I > > > > > >> mean, really, you don't need this patchset in your tree at all. The only > > > > > >> platform which needs it, the only platform which will loose something > > > > > >> will be one specific vendor. Therefore this will be an incentive for > > > > > >> them to join both discussions and upstream development. :) > > > > > > > > > > > > How would they fix this besides upstreaming support for unreleased > > > > > > work-in-progress H/W? > > > > > > > > > > > > Haven't I explained this several times already? :) > > > > > > > > > > Either that way or the same as Will's doing but that's not my question. > > > > > I understand you flush the queue of your GKI patches to be closer to > > > > > upstream. Reduce the backlog/burden. you can achieve your goal by simply > > > > > dropping such patch and making it not your problem. :) > > > > > > > > git reset --hard mainline/master # job done - tea break :) > > > > > > > > Seriously though, we wish to encourage the use of GKI so all vendors > > > > can enjoy the benefits of more easily updateable/secure code-bases. > > > > > > > > I can't see how pushing back on seamlessly benign changes would > > > > benefit them or anyone else. > > > > > > I like your wording ;-) > > > > > > Indeed, seamlessly benign changes, which are (1) not tested, and (2) > > > some believed by the platform maintainer to break the platform. > > > What can possibly go wrong? ;-) > > > > William has already shown a willingness to test the series. > > I'm looking forward to the results! > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds Hi Krzysztof and Geert, Thanks for all the responses! There are a few things I want to respond to. Since this thread is 32 responses deep, I'm copying and pasting the parts I'm responding to. I hope that isn't an issue. >You are changing not default, but selectability which is part of the >enforced configuration to make platforms working. The distros do not >always choose defaults but rather all as modules. I'm guessing you are referring to distros using the "allmodconfig". I agree that this would affect their platforms in the sense that they would get a module vs built-in, but obviously that is the choice they are making by using "allmodconfig". I also agree that my changes should not break any distros. I definitely don't want you or anyone else to accept these without them being tested and verified. >I don't understand the point (2) here. Anyone can use upstream kernel >for supported and unsupported platforms. How upstream benefits from a >change affecting supported platforms made for unsupported, downstream >platforms. We believe that if we make it easier for SoC vendors to directly use the upstream kernel during bring-up and during the development stages of their project, then that will decrease the friction of working with upstream (less downstream changes) and increase the upstream contributions. As mentioned in the LPC talk by Todd, the Android kernel team is constantly pushing to reduce the number of out-of-tree changes we carry so that we can ultimately use the upstream kernel directly. >You also mentioned downstream devices but without actually ever defining >them. Please be more specific. What SoC, what hardware? Saravana provided 2 platforms in his response. The first one is hikey960 devboard and the second one is the Pixel 5 which uses the Qualcomm sm7250 chipset. In particular the Pixel 5 is running a fully modular kernel. It has 320+ modules including interrupt controllers, timers, pinctrl and clocks. To address your question regarding if Exynos is special? No, Exynos isn't special. There is a long road ahead of us to get to where we'd ultimately like to be and ARCH_EXYNOS happens to be along the way. Currently, we are working through the arm64 ARCH_XXX configs that are supported by Android to make sure that all drivers included by any ARCH_XXX are modularized if they can be which benefits all parties. From my understanding upstream does support modularizing drivers and we can help with that. I believe this also kind of addresses Geert's comments regarding what a generic kernel is. We look at it as a kernel that only includes the absolutely necessary SoC drivers as built-in. Obviously we can't go back and change older hardware like Cortex A9 to support this, but we can do our best to support future SoCs. Lastly, you mentioned a couple of times that we should just disable ARCH_EXYNOS and move on. At the moment we are not able to do that because some SoC specific drivers actually do need to be built-in to the kernel (tainting it's generic-ness unfortunately) and are protected by these ARCH_XXX configs. One example that you or Geert pointed out is the early console serial driver which Greg did try to decouple from ARCH_EXYNOS but was turned down. There are likely other reasons as well that I don't know of off hand. I hope Lee and I answered all your questions. Ultimately, I think we all understand what is being debated here and it sounds like we all agree that we are okay with modules as long as (1) they are proven to work as both built-in and as modules and (2) can't be disabled if they are required by the platform, but can be built-in or modular. Let me know if I misunderstood that. Regarding the testing, I will look around for a devboard that I can test my clock driver changes on since those are the most contested ones. I obviously am not going to be able to find all the HW variants you mentioned, but I will try to find an ARM64 Exynos7 or Exynos5433 devboard since my patchset only modifies the clocks for these SoCs which seem to be the most contested. Once I provide some testing results, I'm hoping someone upstream can help verify that on the other affected hardware. Of course this will take some time though. In the meantime, I will try to push some of these easier patches that don't need extensive testing, but are required before modularizing the drivers. Thanks, Will
On Thu, Sep 30, 2021 at 09:10:31PM +0900, Tomasz Figa wrote: > Generally, the subsystems being mentioned here are so basic (clock, > pinctrl, rtc), that I really can't imagine what kind of rocket science > one might want to hide for competitive reasons... If it's for an > entire SoC, I wonder why Intel and AMD don't have similar concerns and > contribute support for their newest hardware far before the release. There is no reason at all, and to be honest this whole discussion with these bullshit arguments from the Google/Linaro/SoC vendor crowd just shows how on crack these people are, and shows a good example of why we should not support these models at all. There is no good reason to "overide" uptream functionality EVER. Stop digging yourselves into your ever bigger holes and just f***king contribute upstream NOW. Just as we always have we should not give you more rope to shoot yoursel while ausing us extra overhead.
On Thu, Sep 30, 2021 at 01:39:35PM +0100, Lee Jones wrote: > How would they fix this besides upstreaming support for unreleased > work-in-progress H/W? > > Haven't I explained this several times already? :) No you haven't. Mostly likely because there is absolutely no good explanation. And if google/Linaro think they want to create hooks for stupid vendor modules we need to do whatever we can to make your life as miserable as possible.
On Thu, Sep 30, 2021 at 9:02 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Thu, Sep 30, 2021 at 09:10:31PM +0900, Tomasz Figa wrote: > > Generally, the subsystems being mentioned here are so basic (clock, > > pinctrl, rtc), that I really can't imagine what kind of rocket science > > one might want to hide for competitive reasons... If it's for an > > entire SoC, I wonder why Intel and AMD don't have similar concerns and > > contribute support for their newest hardware far before the release. > > There is no reason at all, and to be honest this whole discussion with > these bullshit arguments from the Google/Linaro/SoC vendor crowd just > shows how on crack these people are, and shows a good example of why > we should not support these models at all. There is no good reason > to "overide" uptream functionality EVER. Stop digging yourselves into > your ever bigger holes and just f***king contribute upstream NOW. Just > as we always have we should not give you more rope to shoot yoursel > while ausing us extra overhead. Maybe you need to read up the code of conduct again. -Saravana
On Wed, Sep 29, 2021 at 12:48 PM Will McVicker <willmcvicker@google.com> wrote: > > On Wed, Sep 29, 2021 at 6:02 AM Krzysztof Kozlowski > <krzysztof.kozlowski@canonical.com> wrote: > > > > On 29/09/2021 01:56, Will McVicker wrote: > > > This is v2 of the series of patches that modularizes a number of core > > > ARCH_EXYNOS drivers. Based off of the feedback from the v1 series, I have > > > modularized all of the drivers that are removed from the ARCH_EXYNOS > > > series of "select XXX". This includes setting the following configs as > > > tristate: > > > > > > * COMMON_CLK_SAMSUNG > > > * EXYNOS_ARM64_COMMON_CLK > > > * PINCTRL_SAMSUNG > > > * PINCTRL_EXYNOS > > > * EXYNOS_PMU_ARM64 > > > * EXYNOS_PM_DOMAINS > > > > > > Additionally, it introduces the config EXYNOS_PMU_ARM64 and EXYNOS_PMU_ARM > > > which was previously EXYNOS_PMU and EXYNOS_PMU_ARM_DRIVERS respectively. > > > The reason for these new configs is because we are not able to easily > > > modularize the ARMv7 PMU driver due to built-in arch dependencies on > > > pmu_base_addr under arch/arm/mach-exynos/*. So the new configs split up > > > the ARM and ARM64 portions into two separate configs. > > > > > > Overall, these drivers didn't require much refactoring and converted to > > > modules relatively easily. However, due to my lack of exynos hardware, I > > > was not able to boot test these changes. I'm mostly concerned about the > > > CLK_OF_DECLARE() changes having dependencies on early timers. So I'm > > > requesting help for testing these changes on the respective hardware. > > > > > > > These are all not tested at all? In such case, since these are not > > trivial changes, please mark the series as RFT. > > > > I will not be able to test these for some days, so it must wait. > > > > > > Best regards, > > Krzysztof > > +Cc Arnd and Olof, > > Hi Krzysztof, > > To avoid the scrambled conversation from the first patchset, I'm going > to address all your general questions here in the cover letter thread > so that it's easier for everyone to follow and reference in the > future. This patchset shouldn't go in. GKI is a fantastic effort, since it finally seems like Google has the backbone to put pressure on the vendors to upstream all their stuff. This patcheset dilutes and undermines all of that by opening up a truck-size loophole, reducing the impact of GKI, and overall removes leverage to get vendors to do the right thing. It's against our interest as a community to have this happen, since there's no other reasonably justifiable reason to do this. -Olof
On Thu, Sep 30, 2021 at 09:52:12PM -0700, Saravana Kannan wrote:
> Maybe you need to read up the code of conduct again.
*plonk*
On Thu, Sep 30, 2021 at 9:52 PM Olof Johansson <olof@lixom.net> wrote: > > On Wed, Sep 29, 2021 at 12:48 PM Will McVicker <willmcvicker@google.com> wrote: > > > > On Wed, Sep 29, 2021 at 6:02 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@canonical.com> wrote: > > > > > > On 29/09/2021 01:56, Will McVicker wrote: > > > > This is v2 of the series of patches that modularizes a number of core > > > > ARCH_EXYNOS drivers. Based off of the feedback from the v1 series, I have > > > > modularized all of the drivers that are removed from the ARCH_EXYNOS > > > > series of "select XXX". This includes setting the following configs as > > > > tristate: > > > > > > > > * COMMON_CLK_SAMSUNG > > > > * EXYNOS_ARM64_COMMON_CLK > > > > * PINCTRL_SAMSUNG > > > > * PINCTRL_EXYNOS > > > > * EXYNOS_PMU_ARM64 > > > > * EXYNOS_PM_DOMAINS > > > > > > > > Additionally, it introduces the config EXYNOS_PMU_ARM64 and EXYNOS_PMU_ARM > > > > which was previously EXYNOS_PMU and EXYNOS_PMU_ARM_DRIVERS respectively. > > > > The reason for these new configs is because we are not able to easily > > > > modularize the ARMv7 PMU driver due to built-in arch dependencies on > > > > pmu_base_addr under arch/arm/mach-exynos/*. So the new configs split up > > > > the ARM and ARM64 portions into two separate configs. > > > > > > > > Overall, these drivers didn't require much refactoring and converted to > > > > modules relatively easily. However, due to my lack of exynos hardware, I > > > > was not able to boot test these changes. I'm mostly concerned about the > > > > CLK_OF_DECLARE() changes having dependencies on early timers. So I'm > > > > requesting help for testing these changes on the respective hardware. > > > > > > > > > > These are all not tested at all? In such case, since these are not > > > trivial changes, please mark the series as RFT. > > > > > > I will not be able to test these for some days, so it must wait. > > > > > > > > > Best regards, > > > Krzysztof > > > > +Cc Arnd and Olof, > > > > Hi Krzysztof, > > > > To avoid the scrambled conversation from the first patchset, I'm going > > to address all your general questions here in the cover letter thread > > so that it's easier for everyone to follow and reference in the > > future. > > This patchset shouldn't go in. > > GKI is a fantastic effort, since it finally seems like Google has the > backbone to put pressure on the vendors to upstream all their stuff. > > This patcheset dilutes and undermines all of that by opening up a > truck-size loophole, reducing the impact of GKI, and overall removes > leverage to get vendors to do the right thing. > > It's against our interest as a community to have this happen, since > there's no other reasonably justifiable reason to do this. Oolf, Geert, Krzysztof, Arnd, I skimmed through the emails and you all make a lot of good points. It looks like you all at least like the idea of being able to have a minimal generic kernel where everything that can be a module is a module. Please correct me if I'm wrong on that. I was thinking about this patch series and I was wondering if it'd be good to come at it from the other end. Instead of taking the mostly builtin generic kernel and trying to rip out drivers as modules (and not having enough hardware to test them all) and hitting all these issues, we could come at it from the other end. A config like ARM64_MINIMAL_GENERIC_KERNEL that's off by default. But if it's selected, all the "selects" done by the various ARCH_XXX are not done any more. Something like: ARCH_XXX select XXX_CLK1 if !ARM64_MINIMAL_GENERIC_KERNEL select XXX_PINCTRL1 if !ARM64_MINIMAL_GENERIC_KERNEL ARCH_YYY select YYY_CLK1 if !ARM64_MINIMAL_GENERIC_KERNEL select YYY_PINCTRL1 if !ARM64_MINIMAL_GENERIC_KERNEL And ARM64_MINIMAL_GENERIC_KERNEL itself would select the absolutely mandatory stuff that can never be made into a module like the GIC, architectured timer (as Geert mentioned) and UART early console driver. I'm not sure if ARM32 has an equivalent to the standardized GIC and arch timer. Basically the minimal kernel would need a timer for the scheduler tick and IRQ controller to get the timer IRQ and the fixed clock driver if the archtimer uses one to get its frequency and the early UART console is pointless as a module (so build it in to allow debugging/development). And then all new drivers, we should make sure are implemented as tristate drivers. And we can go back and slowly work on converting existing drivers to modules (community effort -- not one person or entity) -- at least the ones where the author has hardware or ones where the change is very likely to be correct and someone else is willing to test it. We'll never be able to support some/all ARM32 (do they even have a GIC/arch timer standard?), but at least for ARM64, this seems like a viable goal. This way, we'll keep the existing model working, while working on a fully modular kernel from the other end. Thoughts? -Saravana
On Thu, Sep 30, 2021 at 10:24 PM Saravana Kannan <saravanak@google.com> wrote: > > On Thu, Sep 30, 2021 at 9:52 PM Olof Johansson <olof@lixom.net> wrote: > > > > On Wed, Sep 29, 2021 at 12:48 PM Will McVicker <willmcvicker@google.com> wrote: > > > > > > On Wed, Sep 29, 2021 at 6:02 AM Krzysztof Kozlowski > > > <krzysztof.kozlowski@canonical.com> wrote: > > > > > > > > On 29/09/2021 01:56, Will McVicker wrote: > > > > > This is v2 of the series of patches that modularizes a number of core > > > > > ARCH_EXYNOS drivers. Based off of the feedback from the v1 series, I have > > > > > modularized all of the drivers that are removed from the ARCH_EXYNOS > > > > > series of "select XXX". This includes setting the following configs as > > > > > tristate: > > > > > > > > > > * COMMON_CLK_SAMSUNG > > > > > * EXYNOS_ARM64_COMMON_CLK > > > > > * PINCTRL_SAMSUNG > > > > > * PINCTRL_EXYNOS > > > > > * EXYNOS_PMU_ARM64 > > > > > * EXYNOS_PM_DOMAINS > > > > > > > > > > Additionally, it introduces the config EXYNOS_PMU_ARM64 and EXYNOS_PMU_ARM > > > > > which was previously EXYNOS_PMU and EXYNOS_PMU_ARM_DRIVERS respectively. > > > > > The reason for these new configs is because we are not able to easily > > > > > modularize the ARMv7 PMU driver due to built-in arch dependencies on > > > > > pmu_base_addr under arch/arm/mach-exynos/*. So the new configs split up > > > > > the ARM and ARM64 portions into two separate configs. > > > > > > > > > > Overall, these drivers didn't require much refactoring and converted to > > > > > modules relatively easily. However, due to my lack of exynos hardware, I > > > > > was not able to boot test these changes. I'm mostly concerned about the > > > > > CLK_OF_DECLARE() changes having dependencies on early timers. So I'm > > > > > requesting help for testing these changes on the respective hardware. > > > > > > > > > > > > > These are all not tested at all? In such case, since these are not > > > > trivial changes, please mark the series as RFT. > > > > > > > > I will not be able to test these for some days, so it must wait. > > > > > > > > > > > > Best regards, > > > > Krzysztof > > > > > > +Cc Arnd and Olof, > > > > > > Hi Krzysztof, > > > > > > To avoid the scrambled conversation from the first patchset, I'm going > > > to address all your general questions here in the cover letter thread > > > so that it's easier for everyone to follow and reference in the > > > future. > > > > This patchset shouldn't go in. > > > > GKI is a fantastic effort, since it finally seems like Google has the > > backbone to put pressure on the vendors to upstream all their stuff. > > > > This patcheset dilutes and undermines all of that by opening up a > > truck-size loophole, reducing the impact of GKI, and overall removes > > leverage to get vendors to do the right thing. > > > > It's against our interest as a community to have this happen, since > > there's no other reasonably justifiable reason to do this. > > Oolf, Geert, Krzysztof, Arnd, So close. > I skimmed through the emails and you all make a lot of good points. I skimmed through this email and I think it adds a lot of new complexity and fragility to solve a problem that doesn't really exist for upstream, adding yet more config parameter combinations to build and test for. A much more valuable approach would be to work towards being able to free up memory by un-probed drivers at the end of boot. That would possibly benefit all platforms on all architectures. -Olof
On Thu, Sep 30, 2021 at 10:36 PM Olof Johansson <olof@lixom.net> wrote: > > On Thu, Sep 30, 2021 at 10:24 PM Saravana Kannan <saravanak@google.com> wrote: > > > > On Thu, Sep 30, 2021 at 9:52 PM Olof Johansson <olof@lixom.net> wrote: > > > > > > On Wed, Sep 29, 2021 at 12:48 PM Will McVicker <willmcvicker@google.com> wrote: > > > > > > > > On Wed, Sep 29, 2021 at 6:02 AM Krzysztof Kozlowski > > > > <krzysztof.kozlowski@canonical.com> wrote: > > > > > > > > > > On 29/09/2021 01:56, Will McVicker wrote: > > > > > > This is v2 of the series of patches that modularizes a number of core > > > > > > ARCH_EXYNOS drivers. Based off of the feedback from the v1 series, I have > > > > > > modularized all of the drivers that are removed from the ARCH_EXYNOS > > > > > > series of "select XXX". This includes setting the following configs as > > > > > > tristate: > > > > > > > > > > > > * COMMON_CLK_SAMSUNG > > > > > > * EXYNOS_ARM64_COMMON_CLK > > > > > > * PINCTRL_SAMSUNG > > > > > > * PINCTRL_EXYNOS > > > > > > * EXYNOS_PMU_ARM64 > > > > > > * EXYNOS_PM_DOMAINS > > > > > > > > > > > > Additionally, it introduces the config EXYNOS_PMU_ARM64 and EXYNOS_PMU_ARM > > > > > > which was previously EXYNOS_PMU and EXYNOS_PMU_ARM_DRIVERS respectively. > > > > > > The reason for these new configs is because we are not able to easily > > > > > > modularize the ARMv7 PMU driver due to built-in arch dependencies on > > > > > > pmu_base_addr under arch/arm/mach-exynos/*. So the new configs split up > > > > > > the ARM and ARM64 portions into two separate configs. > > > > > > > > > > > > Overall, these drivers didn't require much refactoring and converted to > > > > > > modules relatively easily. However, due to my lack of exynos hardware, I > > > > > > was not able to boot test these changes. I'm mostly concerned about the > > > > > > CLK_OF_DECLARE() changes having dependencies on early timers. So I'm > > > > > > requesting help for testing these changes on the respective hardware. > > > > > > > > > > > > > > > > These are all not tested at all? In such case, since these are not > > > > > trivial changes, please mark the series as RFT. > > > > > > > > > > I will not be able to test these for some days, so it must wait. > > > > > > > > > > > > > > > Best regards, > > > > > Krzysztof > > > > > > > > +Cc Arnd and Olof, > > > > > > > > Hi Krzysztof, > > > > > > > > To avoid the scrambled conversation from the first patchset, I'm going > > > > to address all your general questions here in the cover letter thread > > > > so that it's easier for everyone to follow and reference in the > > > > future. > > > > > > This patchset shouldn't go in. > > > > > > GKI is a fantastic effort, since it finally seems like Google has the > > > backbone to put pressure on the vendors to upstream all their stuff. > > > > > > This patcheset dilutes and undermines all of that by opening up a > > > truck-size loophole, reducing the impact of GKI, and overall removes > > > leverage to get vendors to do the right thing. > > > > > > It's against our interest as a community to have this happen, since > > > there's no other reasonably justifiable reason to do this. Are you saying that modularizing drivers is opening up a loophole? How is this different from Krysztof pushing changes to modularize the Exynos ChipId driver just last week [1]. I understand the push back on "these aren't tested yet" and I agree that we should not merge them until they are (I've re-iterated that multiple times and have requested for testing help multiple times since I can't get my hands on any Exynos arm64 hardware), but are you saying that if I gather the test data to prove that these drivers can actually be made into modules that you will still deny them out of the interest of the community? [1] https://lore.kernel.org/linux-samsung-soc/4aee1b0d-91a1-75ac-d2b7-6dab3d7a301f@kernel.org/T/#t --Will > > > > > Oolf, Geert, Krzysztof, Arnd, > > So close. > > > I skimmed through the emails and you all make a lot of good points. > > I skimmed through this email and I think it adds a lot of new > complexity and fragility to solve a problem that doesn't really exist > for upstream, adding yet more config parameter combinations to build > and test for. > > A much more valuable approach would be to work towards being able to > free up memory by un-probed drivers at the end of boot. That would > possibly benefit all platforms on all architectures. > > > -Olof
On Thu, Sep 30, 2021 at 10:36 PM Olof Johansson <olof@lixom.net> wrote: > > On Thu, Sep 30, 2021 at 10:24 PM Saravana Kannan <saravanak@google.com> wrote: > > > > On Thu, Sep 30, 2021 at 9:52 PM Olof Johansson <olof@lixom.net> wrote: > > > > > > On Wed, Sep 29, 2021 at 12:48 PM Will McVicker <willmcvicker@google.com> wrote: > > > > > > > > On Wed, Sep 29, 2021 at 6:02 AM Krzysztof Kozlowski > > > > <krzysztof.kozlowski@canonical.com> wrote: > > > > > > > > > > On 29/09/2021 01:56, Will McVicker wrote: > > > > > > This is v2 of the series of patches that modularizes a number of core > > > > > > ARCH_EXYNOS drivers. Based off of the feedback from the v1 series, I have > > > > > > modularized all of the drivers that are removed from the ARCH_EXYNOS > > > > > > series of "select XXX". This includes setting the following configs as > > > > > > tristate: > > > > > > > > > > > > * COMMON_CLK_SAMSUNG > > > > > > * EXYNOS_ARM64_COMMON_CLK > > > > > > * PINCTRL_SAMSUNG > > > > > > * PINCTRL_EXYNOS > > > > > > * EXYNOS_PMU_ARM64 > > > > > > * EXYNOS_PM_DOMAINS > > > > > > > > > > > > Additionally, it introduces the config EXYNOS_PMU_ARM64 and EXYNOS_PMU_ARM > > > > > > which was previously EXYNOS_PMU and EXYNOS_PMU_ARM_DRIVERS respectively. > > > > > > The reason for these new configs is because we are not able to easily > > > > > > modularize the ARMv7 PMU driver due to built-in arch dependencies on > > > > > > pmu_base_addr under arch/arm/mach-exynos/*. So the new configs split up > > > > > > the ARM and ARM64 portions into two separate configs. > > > > > > > > > > > > Overall, these drivers didn't require much refactoring and converted to > > > > > > modules relatively easily. However, due to my lack of exynos hardware, I > > > > > > was not able to boot test these changes. I'm mostly concerned about the > > > > > > CLK_OF_DECLARE() changes having dependencies on early timers. So I'm > > > > > > requesting help for testing these changes on the respective hardware. > > > > > > > > > > > > > > > > These are all not tested at all? In such case, since these are not > > > > > trivial changes, please mark the series as RFT. > > > > > > > > > > I will not be able to test these for some days, so it must wait. > > > > > > > > > > > > > > > Best regards, > > > > > Krzysztof > > > > > > > > +Cc Arnd and Olof, > > > > > > > > Hi Krzysztof, > > > > > > > > To avoid the scrambled conversation from the first patchset, I'm going > > > > to address all your general questions here in the cover letter thread > > > > so that it's easier for everyone to follow and reference in the > > > > future. > > > > > > This patchset shouldn't go in. > > > > > > GKI is a fantastic effort, since it finally seems like Google has the > > > backbone to put pressure on the vendors to upstream all their stuff. > > > > > > This patcheset dilutes and undermines all of that by opening up a > > > truck-size loophole, reducing the impact of GKI, and overall removes > > > leverage to get vendors to do the right thing. > > > > > > It's against our interest as a community to have this happen, since > > > there's no other reasonably justifiable reason to do this. > > > > Oolf, Geert, Krzysztof, Arnd, > > So close. I'm sorry, it's pretty late here and I'm sleepy and messed it up. > > > I skimmed through the emails and you all make a lot of good points. > > I skimmed through this email and I think it adds a lot of new > complexity and fragility to solve a problem that doesn't really exist > for upstream, adding yet more config parameter combinations to build > and test for. How is this not an upstream problem? Having a minimal kernel with as many drivers as modules is of interest to upstream. And what's the complexity in having a config to easily disable a bunch of configs? The new config gives a clear config against which new platforms/systems should be developed against. > > A much more valuable approach would be to work towards being able to > free up memory by un-probed drivers at the end of boot. That would > possibly benefit all platforms on all architectures. Sure it would help memory after boot, but it won't help with size on "disk", kernel load time, etc. And some of the devices have very tight boot requirements. Think battery operated outdoor cameras for example. -Saravana
On Thu, Sep 30, 2021 at 11:03 PM Saravana Kannan <saravanak@google.com> wrote: > > On Thu, Sep 30, 2021 at 10:36 PM Olof Johansson <olof@lixom.net> wrote: > > > > On Thu, Sep 30, 2021 at 10:24 PM Saravana Kannan <saravanak@google.com> wrote: > > > > > > On Thu, Sep 30, 2021 at 9:52 PM Olof Johansson <olof@lixom.net> wrote: > > > > > > > > On Wed, Sep 29, 2021 at 12:48 PM Will McVicker <willmcvicker@google.com> wrote: > > > > > > > > > > On Wed, Sep 29, 2021 at 6:02 AM Krzysztof Kozlowski > > > > > <krzysztof.kozlowski@canonical.com> wrote: > > > > > > > > > > > > On 29/09/2021 01:56, Will McVicker wrote: > > > > > > > This is v2 of the series of patches that modularizes a number of core > > > > > > > ARCH_EXYNOS drivers. Based off of the feedback from the v1 series, I have > > > > > > > modularized all of the drivers that are removed from the ARCH_EXYNOS > > > > > > > series of "select XXX". This includes setting the following configs as > > > > > > > tristate: > > > > > > > > > > > > > > * COMMON_CLK_SAMSUNG > > > > > > > * EXYNOS_ARM64_COMMON_CLK > > > > > > > * PINCTRL_SAMSUNG > > > > > > > * PINCTRL_EXYNOS > > > > > > > * EXYNOS_PMU_ARM64 > > > > > > > * EXYNOS_PM_DOMAINS > > > > > > > > > > > > > > Additionally, it introduces the config EXYNOS_PMU_ARM64 and EXYNOS_PMU_ARM > > > > > > > which was previously EXYNOS_PMU and EXYNOS_PMU_ARM_DRIVERS respectively. > > > > > > > The reason for these new configs is because we are not able to easily > > > > > > > modularize the ARMv7 PMU driver due to built-in arch dependencies on > > > > > > > pmu_base_addr under arch/arm/mach-exynos/*. So the new configs split up > > > > > > > the ARM and ARM64 portions into two separate configs. > > > > > > > > > > > > > > Overall, these drivers didn't require much refactoring and converted to > > > > > > > modules relatively easily. However, due to my lack of exynos hardware, I > > > > > > > was not able to boot test these changes. I'm mostly concerned about the > > > > > > > CLK_OF_DECLARE() changes having dependencies on early timers. So I'm > > > > > > > requesting help for testing these changes on the respective hardware. > > > > > > > > > > > > > > > > > > > These are all not tested at all? In such case, since these are not > > > > > > trivial changes, please mark the series as RFT. > > > > > > > > > > > > I will not be able to test these for some days, so it must wait. > > > > > > > > > > > > > > > > > > Best regards, > > > > > > Krzysztof > > > > > > > > > > +Cc Arnd and Olof, > > > > > > > > > > Hi Krzysztof, > > > > > > > > > > To avoid the scrambled conversation from the first patchset, I'm going > > > > > to address all your general questions here in the cover letter thread > > > > > so that it's easier for everyone to follow and reference in the > > > > > future. > > > > > > > > This patchset shouldn't go in. > > > > > > > > GKI is a fantastic effort, since it finally seems like Google has the > > > > backbone to put pressure on the vendors to upstream all their stuff. > > > > > > > > This patcheset dilutes and undermines all of that by opening up a > > > > truck-size loophole, reducing the impact of GKI, and overall removes > > > > leverage to get vendors to do the right thing. > > > > > > > > It's against our interest as a community to have this happen, since > > > > there's no other reasonably justifiable reason to do this. > > > > > > Oolf, Geert, Krzysztof, Arnd, > > > > So close. > > I'm sorry, it's pretty late here and I'm sleepy and messed it up. This email thread will be here in the morning too, this is the last reply I will make tonight myself. > > > I skimmed through the emails and you all make a lot of good points. > > > > I skimmed through this email and I think it adds a lot of new > > complexity and fragility to solve a problem that doesn't really exist > > for upstream, adding yet more config parameter combinations to build > > and test for. > > How is this not an upstream problem? Having a minimal kernel with as > many drivers as modules is of interest to upstream. And what's the > complexity in having a config to easily disable a bunch of configs? > The new config gives a clear config against which new > platforms/systems should be developed against. The approach for general kernel upstream has been to have the built-in drivers needed to reach rootfs and runtime load the rest from there. For downstream embedded kernels, the right answer is to just exclude the drivers you don't need. I agree, some of this is probably useful work but 1) It was posted as a burden on the maintainers of the legacy platforms to test and make sure it's not broken 2) It regresses distro configs, which we do care about 3) Based on the above, it's unclear whether the people pushing for this patchset cares about the existing platforms, they're looking to solve a different problem I didn't go back and look at who and where, but the person who started justifying this work with making it easier for out-of-tree vendors to integrate their non-contributed code killed this patchset. Would the out-of-tree argument on its own kill it? No, but the above complexities/cost, plus the actual intent behind the patchset did. > > A much more valuable approach would be to work towards being able to > > free up memory by un-probed drivers at the end of boot. That would > > possibly benefit all platforms on all architectures. > > Sure it would help memory after boot, but it won't help with size on > "disk", kernel load time, etc. And some of the devices have very tight > boot requirements. Think battery operated outdoor cameras for example. I would question the choices made if someone ships a camera with a generic kernel (without a generic filesystem which at that point also needs sufficient storage such that the kernel image size becomes a secondary consideration). Once your storage is that constrained the balance shifts towards a custom minimal config. -Olof
On Thu, Sep 30, 2021 at 11:27 PM Olof Johansson <olof@lixom.net> wrote: > > On Thu, Sep 30, 2021 at 11:03 PM Saravana Kannan <saravanak@google.com> wrote: > > > > On Thu, Sep 30, 2021 at 10:36 PM Olof Johansson <olof@lixom.net> wrote: > > > > > > On Thu, Sep 30, 2021 at 10:24 PM Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > On Thu, Sep 30, 2021 at 9:52 PM Olof Johansson <olof@lixom.net> wrote: > > > > > > > > > > On Wed, Sep 29, 2021 at 12:48 PM Will McVicker <willmcvicker@google.com> wrote: > > > > > > > > > > > > On Wed, Sep 29, 2021 at 6:02 AM Krzysztof Kozlowski > > > > > > <krzysztof.kozlowski@canonical.com> wrote: > > > > > > > > > > > > > > On 29/09/2021 01:56, Will McVicker wrote: > > > > > > > > This is v2 of the series of patches that modularizes a number of core > > > > > > > > ARCH_EXYNOS drivers. Based off of the feedback from the v1 series, I have > > > > > > > > modularized all of the drivers that are removed from the ARCH_EXYNOS > > > > > > > > series of "select XXX". This includes setting the following configs as > > > > > > > > tristate: > > > > > > > > > > > > > > > > * COMMON_CLK_SAMSUNG > > > > > > > > * EXYNOS_ARM64_COMMON_CLK > > > > > > > > * PINCTRL_SAMSUNG > > > > > > > > * PINCTRL_EXYNOS > > > > > > > > * EXYNOS_PMU_ARM64 > > > > > > > > * EXYNOS_PM_DOMAINS > > > > > > > > > > > > > > > > Additionally, it introduces the config EXYNOS_PMU_ARM64 and EXYNOS_PMU_ARM > > > > > > > > which was previously EXYNOS_PMU and EXYNOS_PMU_ARM_DRIVERS respectively. > > > > > > > > The reason for these new configs is because we are not able to easily > > > > > > > > modularize the ARMv7 PMU driver due to built-in arch dependencies on > > > > > > > > pmu_base_addr under arch/arm/mach-exynos/*. So the new configs split up > > > > > > > > the ARM and ARM64 portions into two separate configs. > > > > > > > > > > > > > > > > Overall, these drivers didn't require much refactoring and converted to > > > > > > > > modules relatively easily. However, due to my lack of exynos hardware, I > > > > > > > > was not able to boot test these changes. I'm mostly concerned about the > > > > > > > > CLK_OF_DECLARE() changes having dependencies on early timers. So I'm > > > > > > > > requesting help for testing these changes on the respective hardware. > > > > > > > > > > > > > > > > > > > > > > These are all not tested at all? In such case, since these are not > > > > > > > trivial changes, please mark the series as RFT. > > > > > > > > > > > > > > I will not be able to test these for some days, so it must wait. > > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > Krzysztof > > > > > > > > > > > > +Cc Arnd and Olof, > > > > > > > > > > > > Hi Krzysztof, > > > > > > > > > > > > To avoid the scrambled conversation from the first patchset, I'm going > > > > > > to address all your general questions here in the cover letter thread > > > > > > so that it's easier for everyone to follow and reference in the > > > > > > future. > > > > > > > > > > This patchset shouldn't go in. > > > > > > > > > > GKI is a fantastic effort, since it finally seems like Google has the > > > > > backbone to put pressure on the vendors to upstream all their stuff. > > > > > > > > > > This patcheset dilutes and undermines all of that by opening up a > > > > > truck-size loophole, reducing the impact of GKI, and overall removes > > > > > leverage to get vendors to do the right thing. > > > > > > > > > > It's against our interest as a community to have this happen, since > > > > > there's no other reasonably justifiable reason to do this. > > > > > > > > Oolf, Geert, Krzysztof, Arnd, > > > > > > So close. > > > > I'm sorry, it's pretty late here and I'm sleepy and messed it up. > > This email thread will be here in the morning too, this is the last > reply I will make tonight myself. > > > > > I skimmed through the emails and you all make a lot of good points. > > > > > > I skimmed through this email and I think it adds a lot of new > > > complexity and fragility to solve a problem that doesn't really exist > > > for upstream, adding yet more config parameter combinations to build > > > and test for. > > > > How is this not an upstream problem? Having a minimal kernel with as > > many drivers as modules is of interest to upstream. And what's the > > complexity in having a config to easily disable a bunch of configs? > > The new config gives a clear config against which new > > platforms/systems should be developed against. > > The approach for general kernel upstream has been to have the built-in > drivers needed to reach rootfs and runtime load the rest from there. Correction: The intent for our own multiconfigs have been this. If people want to go more granular and allow for modules of their core drivers, we have never pushed back (but the rest of the argument still stands). > For downstream embedded kernels, the right answer is to just exclude > the drivers you don't need. > > I agree, some of this is probably useful work but > 1) It was posted as a burden on the maintainers of the legacy > platforms to test and make sure it's not broken > 2) It regresses distro configs, which we do care about > 3) Based on the above, it's unclear whether the people pushing for > this patchset cares about the existing platforms, they're looking to > solve a different problem > > I didn't go back and look at who and where, but the person who started > justifying this work with making it easier for out-of-tree vendors to > integrate their non-contributed code killed this patchset. > > Would the out-of-tree argument on its own kill it? No, but the above > complexities/cost, plus the actual intent behind the patchset did. > > > > A much more valuable approach would be to work towards being able to > > > free up memory by un-probed drivers at the end of boot. That would > > > possibly benefit all platforms on all architectures. > > > > Sure it would help memory after boot, but it won't help with size on > > "disk", kernel load time, etc. And some of the devices have very tight > > boot requirements. Think battery operated outdoor cameras for example. > > I would question the choices made if someone ships a camera with a > generic kernel (without a generic filesystem which at that point also > needs sufficient storage such that the kernel image size becomes a > secondary consideration). Once your storage is that constrained the > balance shifts towards a custom minimal config. > > > -Olof
On 01/10/2021 07:59, Will McVicker wrote: > On Thu, Sep 30, 2021 at 10:36 PM Olof Johansson <olof@lixom.net> wrote: >> >>>> >>>> GKI is a fantastic effort, since it finally seems like Google has the >>>> backbone to put pressure on the vendors to upstream all their stuff. >>>> >>>> This patcheset dilutes and undermines all of that by opening up a >>>> truck-size loophole, reducing the impact of GKI, and overall removes >>>> leverage to get vendors to do the right thing. >>>> >>>> It's against our interest as a community to have this happen, since >>>> there's no other reasonably justifiable reason to do this. > > Are you saying that modularizing drivers is opening up a loophole? How > is this different from Krysztof pushing changes to modularize the > Exynos ChipId driver just last week [1]. Modularizing drivers, which can work as modules or even can be disabled because they are not essential for platform boot, is not opening loophole and is helping upstream platforms. Modularizing everything, even essential drivers, because downstream does not want to contribute rest of its drivers, is not beneficial to the upstream project. Since downstream does want to contribute its platforms and drivers, it decides to change mainline project to fits its needs. Only its needs, not others. I was repeating this multiple times - there is no point, no incentive for the mainline to allow disabling essential SoC drivers. It's only downstream interest without any benefit to the upstream. Best regards, Krzysztof
Hi Saravana, On Fri, Oct 1, 2021 at 7:24 AM Saravana Kannan <saravanak@google.com> wrote: > I skimmed through the emails and you all make a lot of good points. It > looks like you all at least like the idea of being able to have a > minimal generic kernel where everything that can be a module is a > module. Please correct me if I'm wrong on that. > > I was thinking about this patch series and I was wondering if it'd be > good to come at it from the other end. Instead of taking the mostly > builtin generic kernel and trying to rip out drivers as modules (and > not having enough hardware to test them all) and hitting all these > issues, we could come at it from the other end. > > A config like ARM64_MINIMAL_GENERIC_KERNEL that's off by default. But > if it's selected, all the "selects" done by the various ARCH_XXX are > not done any more. Something like: > > ARCH_XXX > select XXX_CLK1 if !ARM64_MINIMAL_GENERIC_KERNEL > select XXX_PINCTRL1 if !ARM64_MINIMAL_GENERIC_KERNEL > > ARCH_YYY > select YYY_CLK1 if !ARM64_MINIMAL_GENERIC_KERNEL > select YYY_PINCTRL1 if !ARM64_MINIMAL_GENERIC_KERNEL > > And ARM64_MINIMAL_GENERIC_KERNEL itself would select the absolutely > mandatory stuff that can never be made into a module like the GIC, > architectured timer (as Geert mentioned) and UART early console > driver. I'm not sure if ARM32 has an equivalent to the standardized While the UART early console can work (assuming the related hardware setup has been done by the boot loader), the actual serial driver usually cannot, as it relies on clocks, PM Domains, pin control, which won't be available until the corresponding modular drivers are loaded. Actually earlycon is a debug feature, so I'm wondering if you actually want that in your GKI kernel? > GIC and arch timer. Basically the minimal kernel would need a timer > for the scheduler tick and IRQ controller to get the timer IRQ and the > fixed clock driver if the archtimer uses one to get its frequency and > the early UART console is pointless as a module (so build it in to > allow debugging/development). > > And then all new drivers, we should make sure are implemented as > tristate drivers. And we can go back and slowly work on converting > existing drivers to modules (community effort -- not one person or > entity) -- at least the ones where the author has hardware or ones > where the change is very likely to be correct and someone else is > willing to test it. We'll never be able to support some/all ARM32 (do > they even have a GIC/arch timer standard?), but at least for ARM64, > this seems like a viable goal. Cortex-A7/A15 and later have GIC and architectured timer, so it should work for contemporary systems. Cortex-A9 systems may have GIC, and TWD and/or Global Timer (but I've seen SoCs where the interrupt for the latter was not wired :-(. What are the plans for other architectures? I've seen similar patches being applied for e.g. MIPS. Gr{oetje,eeting}s, Geert
On Fri, Oct 1, 2021 at 10:19 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Fri, Oct 1, 2021 at 7:24 AM Saravana Kannan <saravanak@google.com> wrote: > > GIC and arch timer. Basically the minimal kernel would need a timer > > for the scheduler tick and IRQ controller to get the timer IRQ and the > > fixed clock driver if the archtimer uses one to get its frequency and > > the early UART console is pointless as a module (so build it in to > > allow debugging/development). > > > > And then all new drivers, we should make sure are implemented as > > tristate drivers. And we can go back and slowly work on converting > > existing drivers to modules (community effort -- not one person or > > entity) -- at least the ones where the author has hardware or ones > > where the change is very likely to be correct and someone else is > > willing to test it. We'll never be able to support some/all ARM32 (do > > they even have a GIC/arch timer standard?), but at least for ARM64, > > this seems like a viable goal. > > Cortex-A7/A15 and later have GIC and architectured timer, so it should > work for contemporary systems. > Cortex-A9 systems may have GIC, and TWD and/or Global Timer (but I've > seen SoCs where the interrupt for the latter was not wired :-(. There are a number of well-known examples even with 64-bit chips or Cortex-A7/A15 based SoCs that can't use the architected timer, irqchip or iommu. Apple M1, Broadcom BCM283x, Samsung Exynos5 and some Hisilicon server parts come to mind, I'm sure there are more. > What are the plans for other architectures? > I've seen similar patches being applied for e.g. MIPS. There is some work in the more actively maintained MIPS platforms to make those behave more like Arm/powerpc/riscv/m68k platforms, using a single image and moving drivers into modules. Most MIPS platforms seem unlikely to get updated to this, and will continue to require a SoC specific kernel binary forever, similar to the renesas superh platforms. Most of the less common architectures (arc, csky, hexagon, nios2, xtensa, microblaze, nds32, openrisc, sparc/leon) are way behind that though, and generally don't work at all without out-of-tree code. Arnd
On Fri, Oct 1, 2021 at 7:36 AM Olof Johansson <olof@lixom.net> wrote: > A much more valuable approach would be to work towards being able to > free up memory by un-probed drivers at the end of boot. That would > possibly benefit all platforms on all architectures. This would be really neat. Also the ages-old problem of discarding unreferenced strings which apparently still isn't possible would be great to have solved. E.g. to couple strings to the file/module it belongs with and discard them if the code is not probed. Yours, Linus Walleij
Hi Olof, On Fri, Oct 1, 2021 at 7:36 AM Olof Johansson <olof@lixom.net> wrote: > A much more valuable approach would be to work towards being able to > free up memory by un-probed drivers at the end of boot. That would > possibly benefit all platforms on all architectures. We used to have such a functionality in arch/ppc (not arch/powerpc!), where code/data could be tagged __prep, __chrp, or __pmac, to put it in a special section, and to be freed with initdata when unused. It was removed in v2.6.15[1], as the savings weren't worth the hassle. In a more fragmented space like arm the memory lost due to alignment of the sections would be even more substantial. Another problem is to know when is the end of the boot, especially with deferred probing. [1] 6c45ab992e4299c8 ("[PATCH] powerpc: Remove section free() and linker script bits") Gr{oetje,eeting}s, Geert
On Fri, Oct 1, 2021 at 8:02 AM Saravana Kannan <saravanak@google.com> wrote: > > A much more valuable approach would be to work towards being able to > > free up memory by un-probed drivers at the end of boot. That would > > possibly benefit all platforms on all architectures. > > Sure it would help memory after boot, but it won't help with size on > "disk", kernel load time, etc. And some of the devices have very tight > boot requirements. Think battery operated outdoor cameras for example. I think we can draw a clear line (or several lines) between devices that boot from strictly constrained NOR flash and those that run a platform-independent kernel. Also, when I look at a distro kernel, I see over 5000 kernel modules that need to be stored on disk, but only a small fraction of those are platform specific while most are for general-purpose pluggable devices, network features or file systems that could be used on any system. The vmlinux file is clearly too big and includes too much stuff that should be in loadable modules, but I'm not really that worried about disk space for the platform specific code. Arnd
On Fri, 01 Oct 2021, Arnd Bergmann wrote: > The vmlinux file is clearly too big and includes too much stuff that should > be in loadable modules This for me is the crux of the matter. The ability to replace modules was only brought to light as an "and also, this is possible". However in retrospect, given the attention this has received, it probably shouldn't have even mentioned, as it's not that important. We should focus on the benefits of making parts of the kernel modular if technically possible. The most prominent of those is core binary size, since this has a direct impact on boot-time and RAM usage. Reclaiming dead code after boot is certainly one way to tackle part of the problem. Ensuring that it's not even loaded into RAM in the first place is a better more encompassing solution to both issues IMHO.
On Fri, Oct 1, 2021 at 2:01 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Fri, Oct 1, 2021 at 10:19 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Fri, Oct 1, 2021 at 7:24 AM Saravana Kannan <saravanak@google.com> wrote: > > > GIC and arch timer. Basically the minimal kernel would need a timer > > > for the scheduler tick and IRQ controller to get the timer IRQ and the > > > fixed clock driver if the archtimer uses one to get its frequency and > > > the early UART console is pointless as a module (so build it in to > > > allow debugging/development). > > > > > > And then all new drivers, we should make sure are implemented as > > > tristate drivers. And we can go back and slowly work on converting > > > existing drivers to modules (community effort -- not one person or > > > entity) -- at least the ones where the author has hardware or ones > > > where the change is very likely to be correct and someone else is > > > willing to test it. We'll never be able to support some/all ARM32 (do > > > they even have a GIC/arch timer standard?), but at least for ARM64, > > > this seems like a viable goal. > > > > Cortex-A7/A15 and later have GIC and architectured timer, so it should > > work for contemporary systems. > > Cortex-A9 systems may have GIC, and TWD and/or Global Timer (but I've > > seen SoCs where the interrupt for the latter was not wired :-(. > > There are a number of well-known examples even with 64-bit chips or > Cortex-A7/A15 based SoCs that can't use the architected timer, > irqchip or iommu. > > Apple M1, Broadcom BCM283x, Samsung Exynos5 and > some Hisilicon server parts come to mind, I'm sure there > are more. There's also more and more movement towards having coprocessors with standardized interfaces dealing with this functionality. We're currently at the point where they have coprocessors with non-standardized interfaces, and it's useful to keep encouraging convergence in this area to everybody's benefit. I don't find it particularly useful to make life easier for the custom solutions at the expense of others like this patchset does, when that's (just beyond? on?) the horizon. > > What are the plans for other architectures? > > I've seen similar patches being applied for e.g. MIPS. > > There is some work in the more actively maintained MIPS > platforms to make those behave more like Arm/powerpc/riscv/m68k > platforms, using a single image and moving drivers into modules. > Most MIPS platforms seem unlikely to get updated to this, > and will continue to require a SoC specific kernel binary forever, > similar to the renesas superh platforms. Most of the less > common architectures (arc, csky, hexagon, nios2, xtensa, > microblaze, nds32, openrisc, sparc/leon) are way behind that > though, and generally don't work at all without out-of-tree > code. One of the arguments for needing some of these core drivers in-kernel is that some platforms boot at very conservative DVFS operating points, to a degree that you really want to turn up the CPU clocks fairly early during boot. If you don't have the drivers built-in, you can't do that and/or you create possible fragile or awkward inter-module dependencies with deferred probing, etc. We do care about boot time enough to prefer to just build them in for this reason. If vmlinux binary size is a concern, maybe it's time to consider splitting the drivers into a bare-minimum piece that's not a module for early setup, and the rest that can be loaded post-boot. -Olof
On Fri, Oct 1, 2021 at 5:31 AM Lee Jones <lee.jones@linaro.org> wrote: > > On Fri, 01 Oct 2021, Arnd Bergmann wrote: > > The vmlinux file is clearly too big and includes too much stuff that should > > be in loadable modules > > This for me is the crux of the matter. > > The ability to replace modules was only brought to light as an "and > also, this is possible". However in retrospect, given the attention > this has received, it probably shouldn't have even mentioned, as it's > not that important. Too late, unfortunately. I would actually argue that given the benefit of needing more vendor engagement upstream for GKI to be smooth, it's in our interest to welcome those engagements and make the most of it and help vendors get there, and it's against those interests to make it easier to be out-of-tree if it comes at the expense of our in-tree users and maintainers which this does. > We should focus on the benefits of making parts of the kernel modular > if technically possible. The most prominent of those is core binary > size, since this has a direct impact on boot-time and RAM usage. The way forward here should be to focus on the problem that needs to be solved (vmlinux size) and not overly fixate on whether this patchset is what needs to be merged to reach there, given the downsides observed. I'm not saying let's not improve vmlinux binary size, but this particular approach isn't appealing. > Reclaiming dead code after boot is certainly one way to tackle part of > the problem. Ensuring that it's not even loaded into RAM in the first > place is a better more encompassing solution to both issues IMHO. See my reply to Arnd; the reason some of these drivers aren't modules today is because they are needed during early boot to bring the platform to a stable operating point. Work on fixing the binary size is terrific, but this approach seems to be shortsighted and it's been done in a way that rubs the maintainers the wrong way. -Olof
On Fri, Oct 1, 2021 at 4:59 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Olof, > > On Fri, Oct 1, 2021 at 7:36 AM Olof Johansson <olof@lixom.net> wrote: > > A much more valuable approach would be to work towards being able to > > free up memory by un-probed drivers at the end of boot. That would > > possibly benefit all platforms on all architectures. > > We used to have such a functionality in arch/ppc (not arch/powerpc!), > where code/data could be tagged __prep, __chrp, or __pmac, to put it > in a special section, and to be freed with initdata when unused. It > was removed in v2.6.15[1], as the savings weren't worth the hassle. > In a more fragmented space like arm the memory lost due to alignment > of the sections would be even more substantial. Yeah, the balance between per-platform code size and overall kernel code size shifted over time to a point where it wasn't as meaningful on ppc. > Another problem is to know when is the end of the boot, especially > with deferred probing. Most of this code either has a module_init() or an initcall that actually registers the drivers and/or probes for the platform and does the work. This means you can have a late equivalent hook/initcall that determines whether this path ended up being probed/used. If it wasn't, you can then unregister and flag the corresponding memory to be freed at the end, and would take out the heuristics and guessing on needing to do it automatically from the code path that's doing said freeing. -Olof
On Fri, Oct 1, 2021 at 9:00 AM Olof Johansson <olof@lixom.net> wrote: > > On Fri, Oct 1, 2021 at 4:59 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > Hi Olof, > > > > On Fri, Oct 1, 2021 at 7:36 AM Olof Johansson <olof@lixom.net> wrote: > > > A much more valuable approach would be to work towards being able to > > > free up memory by un-probed drivers at the end of boot. That would > > > possibly benefit all platforms on all architectures. > > > > We used to have such a functionality in arch/ppc (not arch/powerpc!), > > where code/data could be tagged __prep, __chrp, or __pmac, to put it > > in a special section, and to be freed with initdata when unused. It > > was removed in v2.6.15[1], as the savings weren't worth the hassle. > > In a more fragmented space like arm the memory lost due to alignment > > of the sections would be even more substantial. > > Yeah, the balance between per-platform code size and overall kernel > code size shifted over time to a point where it wasn't as meaningful > on ppc. > > > Another problem is to know when is the end of the boot, especially > > with deferred probing. > > Most of this code either has a module_init() or an initcall that > actually registers the drivers and/or probes for the platform and does > the work. > > This means you can have a late equivalent hook/initcall that > determines whether this path ended up being probed/used. If it wasn't, > you can then unregister and flag the corresponding memory to be freed > at the end, and would take out the heuristics and guessing on needing > to do it automatically from the code path that's doing said freeing. > > > -Olof First off, I appreciate the constructive conversations and I understand the ask here. So I'd like to close the "we don't want this" and "this isn't possible" conversation. We have already proven downstream that it is in fact possible to modularize these drivers on other SoCs (mentioned earlier if you missed it) and I'd like to direct the conversation towards verifying/testing here instead of negatively arguing about how SoC vendors aren't upstreaming their drivers. I think everyone understands that, but unfortunately I have no control over that even though I would love everyone to work upstream directly. I am fine with forcing these drivers to always be enabled in some form upstream even though it doesn't really make much sense for a generic kernel that will run on Qualcomm, Exynos, Mediatek, (you name it) SoC devices. I thought about how to do this yesterday and wasn't able to come up with a proper solution that didn't always force this driver to be a module when CONFIG_MODULES is enabled. For example, if I do this below, then we will be forcing all builds to use CONFIG_XXX as a module if they want just one driver as a module. config XXX tristate "blah blah" if COMPILE_TEST default m if (ARCH_XXX && MODULES) default ARCH_XXX The best I was able to come up with was this below which would allow the driver to be a module or built-in; however, obviously it lets you disable it in EXPERT mode. config XXX tristate "blah blah" if COMPILE_TEST || EXPERT default ARCH_XXX Let me know if you have a better solution that doesn't force the driver to be a module when CONFIG_MODULES=y. Saravana did propose a MINIMUM_ARM64_GENERIC_KERNEL config that could solve this, but that too was shot down. Thanks, Will
On Fri, Oct 1, 2021 at 9:51 AM Will McVicker <willmcvicker@google.com> wrote: > > On Fri, Oct 1, 2021 at 9:00 AM Olof Johansson <olof@lixom.net> wrote: > > > > On Fri, Oct 1, 2021 at 4:59 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > Hi Olof, > > > > > > On Fri, Oct 1, 2021 at 7:36 AM Olof Johansson <olof@lixom.net> wrote: > > > > A much more valuable approach would be to work towards being able to > > > > free up memory by un-probed drivers at the end of boot. That would > > > > possibly benefit all platforms on all architectures. > > > > > > We used to have such a functionality in arch/ppc (not arch/powerpc!), > > > where code/data could be tagged __prep, __chrp, or __pmac, to put it > > > in a special section, and to be freed with initdata when unused. It > > > was removed in v2.6.15[1], as the savings weren't worth the hassle. > > > In a more fragmented space like arm the memory lost due to alignment > > > of the sections would be even more substantial. > > > > Yeah, the balance between per-platform code size and overall kernel > > code size shifted over time to a point where it wasn't as meaningful > > on ppc. > > > > > Another problem is to know when is the end of the boot, especially > > > with deferred probing. > > > > Most of this code either has a module_init() or an initcall that > > actually registers the drivers and/or probes for the platform and does > > the work. > > > > This means you can have a late equivalent hook/initcall that > > determines whether this path ended up being probed/used. If it wasn't, > > you can then unregister and flag the corresponding memory to be freed > > at the end, and would take out the heuristics and guessing on needing > > to do it automatically from the code path that's doing said freeing. > > > > > > -Olof > > First off, I appreciate the constructive conversations and I > understand the ask here. So I'd like to close the "we don't want this" > and "this isn't possible" conversation. We have already proven > downstream that it is in fact possible to modularize these drivers on > other SoCs (mentioned earlier if you missed it) and I'd like to direct > the conversation towards verifying/testing here instead of negatively > arguing about how SoC vendors aren't upstreaming their drivers. I > think everyone understands that, but unfortunately I have no control > over that even though I would love everyone to work upstream directly. > > I am fine with forcing these drivers to always be enabled in some form > upstream even though it doesn't really make much sense for a generic > kernel that will run on Qualcomm, Exynos, Mediatek, (you name it) SoC > devices. I thought about how to do this yesterday and wasn't able to > come up with a proper solution that didn't always force this driver to > be a module when CONFIG_MODULES is enabled. This line of reasoning: "I couldn't think of a better option" made us merge a userspace ABI some time ago that within a few months was replaced with a better solution. In that case it was the kernel headers bundling with a build (extending the vmlinux size by a lot), that seemed to have no concerns about binary growth. Not all that far after that went in, the BPF folks came up with a solid solution for CO-RE by introducing BTF, etc. So, the argument "I can't think of a better solution" is a local maxima that we shouldn't settle for if there's a likely better global maxima available with a bit more time/effort. If we say "this problem is worth solving but this doesn't seem to be the solution we want to go for" we might actually be better off long-term. -Olof
On Fri, Oct 1, 2021 at 10:16 AM Olof Johansson <olof@lixom.net> wrote: > > On Fri, Oct 1, 2021 at 9:51 AM Will McVicker <willmcvicker@google.com> wrote: > > > > On Fri, Oct 1, 2021 at 9:00 AM Olof Johansson <olof@lixom.net> wrote: > > > > > > On Fri, Oct 1, 2021 at 4:59 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > > > Hi Olof, > > > > > > > > On Fri, Oct 1, 2021 at 7:36 AM Olof Johansson <olof@lixom.net> wrote: > > > > > A much more valuable approach would be to work towards being able to > > > > > free up memory by un-probed drivers at the end of boot. That would > > > > > possibly benefit all platforms on all architectures. > > > > > > > > We used to have such a functionality in arch/ppc (not arch/powerpc!), > > > > where code/data could be tagged __prep, __chrp, or __pmac, to put it > > > > in a special section, and to be freed with initdata when unused. It > > > > was removed in v2.6.15[1], as the savings weren't worth the hassle. > > > > In a more fragmented space like arm the memory lost due to alignment > > > > of the sections would be even more substantial. > > > > > > Yeah, the balance between per-platform code size and overall kernel > > > code size shifted over time to a point where it wasn't as meaningful > > > on ppc. > > > > > > > Another problem is to know when is the end of the boot, especially > > > > with deferred probing. > > > > > > Most of this code either has a module_init() or an initcall that > > > actually registers the drivers and/or probes for the platform and does > > > the work. > > > > > > This means you can have a late equivalent hook/initcall that > > > determines whether this path ended up being probed/used. If it wasn't, > > > you can then unregister and flag the corresponding memory to be freed > > > at the end, and would take out the heuristics and guessing on needing > > > to do it automatically from the code path that's doing said freeing. > > > > > > > > > -Olof > > > > First off, I appreciate the constructive conversations and I > > understand the ask here. So I'd like to close the "we don't want this" > > and "this isn't possible" conversation. We have already proven > > downstream that it is in fact possible to modularize these drivers on > > other SoCs (mentioned earlier if you missed it) and I'd like to direct > > the conversation towards verifying/testing here instead of negatively > > arguing about how SoC vendors aren't upstreaming their drivers. I > > think everyone understands that, but unfortunately I have no control > > over that even though I would love everyone to work upstream directly. > > > > I am fine with forcing these drivers to always be enabled in some form > > upstream even though it doesn't really make much sense for a generic > > kernel that will run on Qualcomm, Exynos, Mediatek, (you name it) SoC > > devices. I thought about how to do this yesterday and wasn't able to > > come up with a proper solution that didn't always force this driver to > > be a module when CONFIG_MODULES is enabled. > > This line of reasoning: "I couldn't think of a better option" made us > merge a userspace ABI some time ago that within a few months was > replaced with a better solution. In that case it was the kernel > headers bundling with a build (extending the vmlinux size by a lot), > that seemed to have no concerns about binary growth. Not all that far > after that went in, the BPF folks came up with a solid solution for > CO-RE by introducing BTF, etc. > > So, the argument "I can't think of a better solution" is a local > maxima that we shouldn't settle for if there's a likely better global > maxima available with a bit more time/effort. If we say "this problem > is worth solving but this doesn't seem to be the solution we want to > go for" we might actually be better off long-term. > > > -Olof If the answer is, "we don't have a solution for that" then that's fine. I'm not an expert at Kconfig configurations and am asking if there is already a way to handle this. To me it sounded like there might be a solution already due to this policy of "we don't allow disabling drivers that are essential." I'll wait for Krysztof to respond (or someone who has a potential solution) before I dig into this deeper. --Will
On Fri, Oct 1, 2021 at 8:27 AM Olof Johansson <olof@lixom.net> wrote: > > On Fri, Oct 1, 2021 at 2:01 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Fri, Oct 1, 2021 at 10:19 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Fri, Oct 1, 2021 at 7:24 AM Saravana Kannan <saravanak@google.com> wrote: > > > > GIC and arch timer. Basically the minimal kernel would need a timer > > > > for the scheduler tick and IRQ controller to get the timer IRQ and the > > > > fixed clock driver if the archtimer uses one to get its frequency and > > > > the early UART console is pointless as a module (so build it in to > > > > allow debugging/development). > > > > > > > > And then all new drivers, we should make sure are implemented as > > > > tristate drivers. And we can go back and slowly work on converting > > > > existing drivers to modules (community effort -- not one person or > > > > entity) -- at least the ones where the author has hardware or ones > > > > where the change is very likely to be correct and someone else is > > > > willing to test it. We'll never be able to support some/all ARM32 (do > > > > they even have a GIC/arch timer standard?), but at least for ARM64, > > > > this seems like a viable goal. > > > > > > Cortex-A7/A15 and later have GIC and architectured timer, so it should > > > work for contemporary systems. > > > Cortex-A9 systems may have GIC, and TWD and/or Global Timer (but I've > > > seen SoCs where the interrupt for the latter was not wired :-(. > > > > There are a number of well-known examples even with 64-bit chips or > > Cortex-A7/A15 based SoCs that can't use the architected timer, > > irqchip or iommu. > > > > Apple M1, Broadcom BCM283x, Samsung Exynos5 and > > some Hisilicon server parts come to mind, I'm sure there > > are more. > > There's also more and more movement towards having coprocessors with > standardized interfaces dealing with this functionality. We're > currently at the point where they have coprocessors with > non-standardized interfaces, and it's useful to keep encouraging > convergence in this area to everybody's benefit. I don't find it > particularly useful to make life easier for the custom solutions at > the expense of others like this patchset does, when that's (just > beyond? on?) the horizon. > > > > What are the plans for other architectures? > > > I've seen similar patches being applied for e.g. MIPS. > > > > There is some work in the more actively maintained MIPS > > platforms to make those behave more like Arm/powerpc/riscv/m68k > > platforms, using a single image and moving drivers into modules. > > Most MIPS platforms seem unlikely to get updated to this, > > and will continue to require a SoC specific kernel binary forever, > > similar to the renesas superh platforms. Most of the less > > common architectures (arc, csky, hexagon, nios2, xtensa, > > microblaze, nds32, openrisc, sparc/leon) are way behind that > > though, and generally don't work at all without out-of-tree > > code. > > One of the arguments for needing some of these core drivers in-kernel > is that some platforms boot at very conservative DVFS operating > points, to a degree that you really want to turn up the CPU clocks > fairly early during boot. > > If you don't have the drivers built-in, you can't do that and/or you > create possible fragile or awkward inter-module dependencies with > deferred probing, etc. We do care about boot time enough to prefer to > just build them in for this reason. Go look at a Pixel 5, we got this working just fine with all these drivers as modules and we definitely care about boot time. You just need to load your CPU freq driver and the other ones it needs early on. And with fw_devlink=on (default in upstream), there's hardly any deferred probing. > If vmlinux binary size is a concern, maybe it's time to consider > splitting the drivers into a bare-minimum piece that's not a module > for early setup, and the rest that can be loaded post-boot. Isn't this literally what I was suggesting with my ARM64_MINIMAL_GENERIC_KERNEL suggestion? Build in all the bare minimum drivers that are needed before module loading can happen? You'd just select them all under that config. And any existing platform that wants to use it would break up their drivers into modules and switch to it. -Saravana
2021年10月2日(土) 4:26 Saravana Kannan <saravanak@google.com>: > > On Fri, Oct 1, 2021 at 8:27 AM Olof Johansson <olof@lixom.net> wrote: > > > > On Fri, Oct 1, 2021 at 2:01 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > > On Fri, Oct 1, 2021 at 10:19 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Fri, Oct 1, 2021 at 7:24 AM Saravana Kannan <saravanak@google.com> wrote: > > > > > GIC and arch timer. Basically the minimal kernel would need a timer > > > > > for the scheduler tick and IRQ controller to get the timer IRQ and the > > > > > fixed clock driver if the archtimer uses one to get its frequency and > > > > > the early UART console is pointless as a module (so build it in to > > > > > allow debugging/development). > > > > > > > > > > And then all new drivers, we should make sure are implemented as > > > > > tristate drivers. And we can go back and slowly work on converting > > > > > existing drivers to modules (community effort -- not one person or > > > > > entity) -- at least the ones where the author has hardware or ones > > > > > where the change is very likely to be correct and someone else is > > > > > willing to test it. We'll never be able to support some/all ARM32 (do > > > > > they even have a GIC/arch timer standard?), but at least for ARM64, > > > > > this seems like a viable goal. > > > > > > > > Cortex-A7/A15 and later have GIC and architectured timer, so it should > > > > work for contemporary systems. > > > > Cortex-A9 systems may have GIC, and TWD and/or Global Timer (but I've > > > > seen SoCs where the interrupt for the latter was not wired :-(. > > > > > > There are a number of well-known examples even with 64-bit chips or > > > Cortex-A7/A15 based SoCs that can't use the architected timer, > > > irqchip or iommu. > > > > > > Apple M1, Broadcom BCM283x, Samsung Exynos5 and > > > some Hisilicon server parts come to mind, I'm sure there > > > are more. > > > > There's also more and more movement towards having coprocessors with > > standardized interfaces dealing with this functionality. We're > > currently at the point where they have coprocessors with > > non-standardized interfaces, and it's useful to keep encouraging > > convergence in this area to everybody's benefit. I don't find it > > particularly useful to make life easier for the custom solutions at > > the expense of others like this patchset does, when that's (just > > beyond? on?) the horizon. > > > > > > What are the plans for other architectures? > > > > I've seen similar patches being applied for e.g. MIPS. > > > > > > There is some work in the more actively maintained MIPS > > > platforms to make those behave more like Arm/powerpc/riscv/m68k > > > platforms, using a single image and moving drivers into modules. > > > Most MIPS platforms seem unlikely to get updated to this, > > > and will continue to require a SoC specific kernel binary forever, > > > similar to the renesas superh platforms. Most of the less > > > common architectures (arc, csky, hexagon, nios2, xtensa, > > > microblaze, nds32, openrisc, sparc/leon) are way behind that > > > though, and generally don't work at all without out-of-tree > > > code. > > > > One of the arguments for needing some of these core drivers in-kernel > > is that some platforms boot at very conservative DVFS operating > > points, to a degree that you really want to turn up the CPU clocks > > fairly early during boot. > > > > If you don't have the drivers built-in, you can't do that and/or you > > create possible fragile or awkward inter-module dependencies with > > deferred probing, etc. We do care about boot time enough to prefer to > > just build them in for this reason. > > Go look at a Pixel 5, we got this working just fine with all these > drivers as modules and we definitely care about boot time. You just > need to load your CPU freq driver and the other ones it needs early > on. And with fw_devlink=on (default in upstream), there's hardly any > deferred probing. > > > If vmlinux binary size is a concern, maybe it's time to consider > > splitting the drivers into a bare-minimum piece that's not a module > > for early setup, and the rest that can be loaded post-boot. > > Isn't this literally what I was suggesting with my > ARM64_MINIMAL_GENERIC_KERNEL suggestion? Build in all the bare minimum > drivers that are needed before module loading can happen? You'd just > select them all under that config. And any existing platform that > wants to use it would break up their drivers into modules and switch > to it. I think the point here is that pinctrl and clk are considered a part of "bare minimum" for Exynos.
On Fri, Oct 1, 2021 at 12:26 PM Saravana Kannan <saravanak@google.com> wrote: > > On Fri, Oct 1, 2021 at 8:27 AM Olof Johansson <olof@lixom.net> wrote: > > > > On Fri, Oct 1, 2021 at 2:01 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > > On Fri, Oct 1, 2021 at 10:19 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Fri, Oct 1, 2021 at 7:24 AM Saravana Kannan <saravanak@google.com> wrote: > > > > > GIC and arch timer. Basically the minimal kernel would need a timer > > > > > for the scheduler tick and IRQ controller to get the timer IRQ and the > > > > > fixed clock driver if the archtimer uses one to get its frequency and > > > > > the early UART console is pointless as a module (so build it in to > > > > > allow debugging/development). > > > > > > > > > > And then all new drivers, we should make sure are implemented as > > > > > tristate drivers. And we can go back and slowly work on converting > > > > > existing drivers to modules (community effort -- not one person or > > > > > entity) -- at least the ones where the author has hardware or ones > > > > > where the change is very likely to be correct and someone else is > > > > > willing to test it. We'll never be able to support some/all ARM32 (do > > > > > they even have a GIC/arch timer standard?), but at least for ARM64, > > > > > this seems like a viable goal. > > > > > > > > Cortex-A7/A15 and later have GIC and architectured timer, so it should > > > > work for contemporary systems. > > > > Cortex-A9 systems may have GIC, and TWD and/or Global Timer (but I've > > > > seen SoCs where the interrupt for the latter was not wired :-(. > > > > > > There are a number of well-known examples even with 64-bit chips or > > > Cortex-A7/A15 based SoCs that can't use the architected timer, > > > irqchip or iommu. > > > > > > Apple M1, Broadcom BCM283x, Samsung Exynos5 and > > > some Hisilicon server parts come to mind, I'm sure there > > > are more. > > > > There's also more and more movement towards having coprocessors with > > standardized interfaces dealing with this functionality. We're > > currently at the point where they have coprocessors with > > non-standardized interfaces, and it's useful to keep encouraging > > convergence in this area to everybody's benefit. I don't find it > > particularly useful to make life easier for the custom solutions at > > the expense of others like this patchset does, when that's (just > > beyond? on?) the horizon. > > > > > > What are the plans for other architectures? > > > > I've seen similar patches being applied for e.g. MIPS. > > > > > > There is some work in the more actively maintained MIPS > > > platforms to make those behave more like Arm/powerpc/riscv/m68k > > > platforms, using a single image and moving drivers into modules. > > > Most MIPS platforms seem unlikely to get updated to this, > > > and will continue to require a SoC specific kernel binary forever, > > > similar to the renesas superh platforms. Most of the less > > > common architectures (arc, csky, hexagon, nios2, xtensa, > > > microblaze, nds32, openrisc, sparc/leon) are way behind that > > > though, and generally don't work at all without out-of-tree > > > code. > > > > One of the arguments for needing some of these core drivers in-kernel > > is that some platforms boot at very conservative DVFS operating > > points, to a degree that you really want to turn up the CPU clocks > > fairly early during boot. > > > > If you don't have the drivers built-in, you can't do that and/or you > > create possible fragile or awkward inter-module dependencies with > > deferred probing, etc. We do care about boot time enough to prefer to > > just build them in for this reason. > > Go look at a Pixel 5, we got this working just fine with all these > drivers as modules and we definitely care about boot time. You just > need to load your CPU freq driver and the other ones it needs early > on. And with fw_devlink=on (default in upstream), there's hardly any > deferred probing. Unfortunately these problems are usually easier to fix on new platforms, especially during new product development. The hard part is making sure you haven't regressed any of the legacy platforms when you're changing the implementation for them as well. > > If vmlinux binary size is a concern, maybe it's time to consider > > splitting the drivers into a bare-minimum piece that's not a module > > for early setup, and the rest that can be loaded post-boot. > > Isn't this literally what I was suggesting with my > ARM64_MINIMAL_GENERIC_KERNEL suggestion? Build in all the bare minimum > drivers that are needed before module loading can happen? You'd just > select them all under that config. And any existing platform that > wants to use it would break up their drivers into modules and switch > to it. Do you understand the implications of your proposal on the complexity for those who care about making sure different config combinations keep building and working, i.e. both the minimal-generic-kernel and the regular generic version? You've doubled the testing workload for all of those folks. It's a different mindset from when you mostly need to care about your one config and platform. -Olof