| Message ID | 20220428062430.31010-1-paul.gortmaker@windriver.com |
|---|---|
| Headers | show |
| Series | platform: allow ATOM PMC code to be optional | expand |
On Thu, Apr 28, 2022 at 02:24:26AM -0400, Paul Gortmaker wrote: > A few months back I was doing a test build for "defconfig-like" COTS > hardware and happened to notice some pmc-atom stuff being built. Fine, > I thought - the defconfig isn't minimal - we all know that - what Kconfig > do I use to turn that off? Well, imagine my surprise when I found out > you couldn't turn it [Atom Power Management Controller] code off! Turning it off on BayTrail and CherryTrail devices will be devastating to the users' experience. And plenty of cheap tablets are exactly made on that SoCs. > Normally we all agree to not use "default y" unless unavoidable, but > somehow this was added as "def_bool y" which is even worse ; you can > see the Kconfig setting but there is *no* way you can turn it off. > > After investigating, I found no reason why the atom code couldn't be > opt-out with just minor changes that the original addition overlooked. > > And so this series addreses that. I tried to be sensitive to not > break any existing configs in the process, but the defconfig will > now intentionally be different and exclude this atom specific code. > > Using a defconfig on today's linux-next, here is the delta summary: > > $ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep add/remove > add/remove: 0/410 grow/shrink: 0/7 up/down: 0/-47659 (-47659) > add/remove: 0/105 grow/shrink: 0/1 up/down: 0/-6848 (-6848) > add/remove: 0/56 grow/shrink: 0/1 up/down: 0/-10155 (-10155) > > $ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep Total > Total: Before=13626994, After=13579335, chg -0.35% > Total: Before=3572137, After=3565289, chg -0.19% > Total: Before=1235335, After=1225180, chg -0.82% > > It is hard to reclaim anything against the inevitable growth in > footprint, so I'd say we should be glad to take whatever we can. > > Boot tested defconfig on today's linux-next on older non-atom COTS. I believe this needs an extensive test done by Hans who possesses a lot of problematic devices that require that module to be present. Note to all your patches, please, use --cc option instead of putting noisy lines in the each of the commit messages. For myself I created a "smart" script [1] to avoid bothering with that. Feel free to use, modify, send PRs back to improve. [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
[Re: [PATCH 0/4] platform: allow ATOM PMC code to be optional] On 28/04/2022 (Thu 13:12) Andy Shevchenko wrote: > On Thu, Apr 28, 2022 at 02:24:26AM -0400, Paul Gortmaker wrote: > > A few months back I was doing a test build for "defconfig-like" COTS > > hardware and happened to notice some pmc-atom stuff being built. Fine, > > I thought - the defconfig isn't minimal - we all know that - what Kconfig > > do I use to turn that off? Well, imagine my surprise when I found out > > you couldn't turn it [Atom Power Management Controller] code off! > > Turning it off on BayTrail and CherryTrail devices will be devastating > to the users' experience. And plenty of cheap tablets are exactly made > on that SoCs. Sure, but I could say the same thing for DRM_I915 and millions of desktop PC users - yet we still give all the other non i915 users the option to be able to turn it off. Pick any other Kconfig value you like and the same thing holds true. Just so we are on the same page - I want to give the option to let people opt out, and at the same time not break existing users. If you think the defconfig default of being off is too risky, then I am OK with that and we can just start by exposing the option with "default y". So, to that end - are you OK with exposing the Kconfig so people can opt out, or are you 100% against exposing the Kconfig at all? That obviously has the most impact on what I do or don't do next. > > Normally we all agree to not use "default y" unless unavoidable, but > > somehow this was added as "def_bool y" which is even worse ; you can > > see the Kconfig setting but there is *no* way you can turn it off. > > > > After investigating, I found no reason why the atom code couldn't be > > opt-out with just minor changes that the original addition overlooked. > > > > And so this series addreses that. I tried to be sensitive to not > > break any existing configs in the process, but the defconfig will > > now intentionally be different and exclude this atom specific code. > > > > Using a defconfig on today's linux-next, here is the delta summary: > > > > $ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep add/remove > > add/remove: 0/410 grow/shrink: 0/7 up/down: 0/-47659 (-47659) > > add/remove: 0/105 grow/shrink: 0/1 up/down: 0/-6848 (-6848) > > add/remove: 0/56 grow/shrink: 0/1 up/down: 0/-10155 (-10155) > > > > $ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep Total > > Total: Before=13626994, After=13579335, chg -0.35% > > Total: Before=3572137, After=3565289, chg -0.19% > > Total: Before=1235335, After=1225180, chg -0.82% > > > > It is hard to reclaim anything against the inevitable growth in > > footprint, so I'd say we should be glad to take whatever we can. > > > > Boot tested defconfig on today's linux-next on older non-atom COTS. > > I believe this needs an extensive test done by Hans who possesses a lot of > problematic devices that require that module to be present. Input from Hans is 100% welcome - but maybe again if we just consider using "default y" even though it isn't typical - then your concerns are not as extensive? > Note to all your patches, please, use --cc option instead of putting noisy > lines in the each of the commit messages. For myself I created a "smart" > script [1] to avoid bothering with that. Feel free to use, modify, send PRs > back to improve. Thanks - I'm used to explicitly capturing who was looped into the discussion. But I hadn't considered how things have evolved so that in certain circumstances that might be considered just noise with the wider audience we see in the typical patch sets of today. Assuming you are OK with exposing the option as a choice, I'll make things "default y" in v2 to ensure we've minimized risk to existing users who rely on it, but wait a while for others like Hans to put in their input. I'll probably follow up in the individual patches to ask for clarification if I'm not clear on what you had in mind. Thanks, Paul. -- > > [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh > > -- > With Best Regards, > Andy Shevchenko > >
On Thu, Apr 28, 2022 at 02:11:31PM -0400, Paul Gortmaker wrote: > [Re: [PATCH 0/4] platform: allow ATOM PMC code to be optional] On 28/04/2022 (Thu 13:12) Andy Shevchenko wrote: > > On Thu, Apr 28, 2022 at 02:24:26AM -0400, Paul Gortmaker wrote: > > > A few months back I was doing a test build for "defconfig-like" COTS > > > hardware and happened to notice some pmc-atom stuff being built. Fine, > > > I thought - the defconfig isn't minimal - we all know that - what Kconfig > > > do I use to turn that off? Well, imagine my surprise when I found out > > > you couldn't turn it [Atom Power Management Controller] code off! > > > > Turning it off on BayTrail and CherryTrail devices will be devastating > > to the users' experience. And plenty of cheap tablets are exactly made > > on that SoCs. > > Sure, but I could say the same thing for DRM_I915 and millions of > desktop PC users - yet we still give all the other non i915 users the > option to be able to turn it off. Pick any other Kconfig value you like > and the same thing holds true. > > Just so we are on the same page - I want to give the option to let > people opt out, and at the same time not break existing users. If you > think the defconfig default of being off is too risky, then I am OK with > that and we can just start by exposing the option with "default y". > > So, to that end - are you OK with exposing the Kconfig so people can > opt out, or are you 100% against exposing the Kconfig at all? That > obviously has the most impact on what I do or don't do next. > > > > Normally we all agree to not use "default y" unless unavoidable, but > > > somehow this was added as "def_bool y" which is even worse ; you can > > > see the Kconfig setting but there is *no* way you can turn it off. > > > > > > After investigating, I found no reason why the atom code couldn't be > > > opt-out with just minor changes that the original addition overlooked. > > > > > > And so this series addreses that. I tried to be sensitive to not > > > break any existing configs in the process, but the defconfig will > > > now intentionally be different and exclude this atom specific code. > > > > > > Using a defconfig on today's linux-next, here is the delta summary: > > > > > > $ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep add/remove > > > add/remove: 0/410 grow/shrink: 0/7 up/down: 0/-47659 (-47659) > > > add/remove: 0/105 grow/shrink: 0/1 up/down: 0/-6848 (-6848) > > > add/remove: 0/56 grow/shrink: 0/1 up/down: 0/-10155 (-10155) > > > > > > $ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep Total > > > Total: Before=13626994, After=13579335, chg -0.35% > > > Total: Before=3572137, After=3565289, chg -0.19% > > > Total: Before=1235335, After=1225180, chg -0.82% > > > > > > It is hard to reclaim anything against the inevitable growth in > > > footprint, so I'd say we should be glad to take whatever we can. > > > > > > Boot tested defconfig on today's linux-next on older non-atom COTS. > > > > I believe this needs an extensive test done by Hans who possesses a lot of > > problematic devices that require that module to be present. > > Input from Hans is 100% welcome - but maybe again if we just consider > using "default y" even though it isn't typical - then your concerns are > not as extensive? > > > Note to all your patches, please, use --cc option instead of putting noisy > > lines in the each of the commit messages. For myself I created a "smart" > > script [1] to avoid bothering with that. Feel free to use, modify, send PRs > > back to improve. > > Thanks - I'm used to explicitly capturing who was looped into the > discussion. But I hadn't considered how things have evolved so that in > certain circumstances that might be considered just noise with the wider > audience we see in the typical patch sets of today. > > Assuming you are OK with exposing the option as a choice, I'll make > things "default y" in v2 to ensure we've minimized risk to existing > users who rely on it, but wait a while for others like Hans to put in > their input. I'll probably follow up in the individual patches to ask > for clarification if I'm not clear on what you had in mind. My main concern is the existing users' experience. Opting-out the option is a good to have, I'm not against it.
Hi, On 4/28/22 20:11, Paul Gortmaker wrote: > [Re: [PATCH 0/4] platform: allow ATOM PMC code to be optional] On 28/04/2022 (Thu 13:12) Andy Shevchenko wrote: > >> On Thu, Apr 28, 2022 at 02:24:26AM -0400, Paul Gortmaker wrote: >>> A few months back I was doing a test build for "defconfig-like" COTS >>> hardware and happened to notice some pmc-atom stuff being built. Fine, >>> I thought - the defconfig isn't minimal - we all know that - what Kconfig >>> do I use to turn that off? Well, imagine my surprise when I found out >>> you couldn't turn it [Atom Power Management Controller] code off! >> >> Turning it off on BayTrail and CherryTrail devices will be devastating >> to the users' experience. And plenty of cheap tablets are exactly made >> on that SoCs. > > Sure, but I could say the same thing for DRM_I915 and millions of > desktop PC users - yet we still give all the other non i915 users the > option to be able to turn it off. Pick any other Kconfig value you like > and the same thing holds true. > > Just so we are on the same page - I want to give the option to let > people opt out, and at the same time not break existing users. If you > think the defconfig default of being off is too risky, then I am OK with > that and we can just start by exposing the option with "default y". > > So, to that end - are you OK with exposing the Kconfig so people can > opt out, or are you 100% against exposing the Kconfig at all? That > obviously has the most impact on what I do or don't do next. > >>> Normally we all agree to not use "default y" unless unavoidable, but >>> somehow this was added as "def_bool y" which is even worse ; you can >>> see the Kconfig setting but there is *no* way you can turn it off. >>> >>> After investigating, I found no reason why the atom code couldn't be >>> opt-out with just minor changes that the original addition overlooked. >>> >>> And so this series addreses that. I tried to be sensitive to not >>> break any existing configs in the process, but the defconfig will >>> now intentionally be different and exclude this atom specific code. >>> >>> Using a defconfig on today's linux-next, here is the delta summary: >>> >>> $ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep add/remove >>> add/remove: 0/410 grow/shrink: 0/7 up/down: 0/-47659 (-47659) >>> add/remove: 0/105 grow/shrink: 0/1 up/down: 0/-6848 (-6848) >>> add/remove: 0/56 grow/shrink: 0/1 up/down: 0/-10155 (-10155) >>> >>> $ ./scripts/bloat-o-meter -c ../pmc-atom-pre/vmlinux ../pmc-atom-post/vmlinux|grep Total >>> Total: Before=13626994, After=13579335, chg -0.35% >>> Total: Before=3572137, After=3565289, chg -0.19% >>> Total: Before=1235335, After=1225180, chg -0.82% >>> >>> It is hard to reclaim anything against the inevitable growth in >>> footprint, so I'd say we should be glad to take whatever we can. >>> >>> Boot tested defconfig on today's linux-next on older non-atom COTS. >> >> I believe this needs an extensive test done by Hans who possesses a lot of >> problematic devices that require that module to be present. > > Input from Hans is 100% welcome - but maybe again if we just consider > using "default y" even though it isn't typical - then your concerns are > not as extensive? I have no objection against allowing disabling the PMC_ATOM Kconfig option. As for users breaking support for BYT/CHT setups because they forget to enable this, without X86_INTEL_LPSS being enabled BYT/CHT are pretty much broken anyways and since patch 2/4 adds a "select PMC_ATOM" to the X86_INTEL_LPSS Kconfig option I'm not really worried about that. I'm afraid this patch-set might break some randomconfig builds though, but I cannot see anything obviously causing such breakage here, so I think it would be fine to just merge this series as is and then see if we get any breakage reports. Andy, are you ok with me moving ahead and merging this series as is? Regards, Hans
On Mon, May 02, 2022 at 04:30:57PM +0200, Hans de Goede wrote: > On 4/28/22 20:11, Paul Gortmaker wrote: ... > As for users breaking support for BYT/CHT setups because they forget > to enable this, without X86_INTEL_LPSS being enabled BYT/CHT are pretty > much broken anyways and since patch 2/4 adds a "select PMC_ATOM" to the > X86_INTEL_LPSS Kconfig option I'm not really worried about that. > > I'm afraid this patch-set might break some randomconfig builds though, > but I cannot see anything obviously causing such breakage here, so > I think it would be fine to just merge this series as is and then > see if we get any breakage reports. > > Andy, are you ok with me moving ahead and merging this series as is? It seems as is can't be fulfilled due to your own comment, but in general I'm not objecting the idea. So, go ahead if you feel it's ready.
Hi, On 5/2/22 18:05, Andy Shevchenko wrote: > On Mon, May 02, 2022 at 04:30:57PM +0200, Hans de Goede wrote: >> On 4/28/22 20:11, Paul Gortmaker wrote: > > ... > >> As for users breaking support for BYT/CHT setups because they forget >> to enable this, without X86_INTEL_LPSS being enabled BYT/CHT are pretty >> much broken anyways and since patch 2/4 adds a "select PMC_ATOM" to the >> X86_INTEL_LPSS Kconfig option I'm not really worried about that. >> >> I'm afraid this patch-set might break some randomconfig builds though, >> but I cannot see anything obviously causing such breakage here, so >> I think it would be fine to just merge this series as is and then >> see if we get any breakage reports. >> >> Andy, are you ok with me moving ahead and merging this series as is? > > It seems as is can't be fulfilled due to your own comment, but in general I'm > not objecting the idea. So, go ahead if you feel it's ready. Right, my later comment to just replace PMC_ATOM with X86_INTEL_LPSS supersedes this. I'll send out a patch with that approach so that this can get some comments / review. Regards, Hans