On 3/24/22 09:25, Limonciello, Mario wrote: > [Public] > >> On 3/24/22 08:04, Limonciello, Mario wrote: >>> [Public] >>> >>>> On Mon, Mar 21, 2022 at 11:57 PM Damien Le Moal >>>> <damien.lemoal@xxxxxxxxxxxxxxxxxx> wrote: >>>>> >>>>> * Rename ahci_board_mobile to board_ahci_low_power to be more >>>> descriptive >>>>> of the feature as that is also used on PC and server AHCI adapters, >>>>> from Mario. >>>>> >>>>> Mario Limonciello (3): >>>>> ata: ahci: Rename board_ahci_mobile >>>>> ata: ahci: Rename `AHCI_HFLAG_IS_MOBILE` >>>>> ata: ahci: Rename CONFIG_SATA_LPM_MOBILE_POLICY configuration >> item >>>> >>>> So I've pulled this, but it's worth noting that particularly renaming >>>> that CONFIG option was probably not a good idea. >>>> >>>> Why? >>>> >>>> Because it means that people silently lose their old values. And it has that >>>> >>>> range 0 4 >>>> default 0 >>>> >>>> with 4 being explicitly marked as very dangerous - but at least Fedora >>>> seems to actually have a default of 3 in their kernels: >>>> >>>> /boot/config-5.16.13-200.fc35.x86_64: >>>> CONFIG_SATA_MOBILE_LPM_POLICY=3 >>>> >>>> so that "default 0" may actually not be the right one. >>>> >>>> Now, we're at the point where few enough people likely care about ATA, >>>> but the corollary to that is also that these kinds of changes can >>>> cause user pain that then developers have *no* idea about. >>>> Particularly when the pain ends up being caused by some subtle default >>>> config option silently changing that nobody even thought about. >>>> >>>> Now, that "default 0" is probably the only safe default - and I don't >>>> dispute that part. But I also suspect that Fedora chose that value '3' >>>> because it probably makes a noticeable power use difference on at >>>> least some platforms. >>>> >>>> I don't know. But I doubt really *anybody* knows, so renaming them and >>>> silently likely changing config options for some less-than-critical >>>> reason is just not a great idea. >>>> >>>> Linus >>> >>> Thanks for pointing out the subtlety of renaming a configuration option hides >>> problems because people don't see the new config option and pick the default. >>> I wouldn't call this configuration option rename critical, so if you chose to >> revert >>> it I would understand. >>> >>> However I think you raise a good point that if distros are picking different >> "default" >>> values and keeping them there a long time that the value in the upstream >> kernel >>> is probably not right anymore. A while back that default made sense because >> all the >>> power saving stuff was risky at the time. It's pretty well baked now. >>> >>> So maybe a logical thing is to keep this change and send a follow up that also >> changes >>> the default to 3? If you're supportive of that I'll send something to Damien to >> do that. >> >> Mario, let's check what other distros do first before deciding. Fedora for >> sure has a default of 3 and I have never seen any issue with it (and I >> have been using Fedora for a long time with many different drives). >> >> Not sure what distro you are using, but if it is not Fedora, please check. >> We should check at least Debian, Ubuntu, SUSE, RHEL and CentOS. I can >> check some other minor ones too as I know users. >> > > I use Ubuntu mostly, and have a variety of distro kernels installed: > > config-5.13.0-25-generic:CONFIG_SATA_MOBILE_LPM_POLICY=3 > config-5.14.0-1029-oem:CONFIG_SATA_MOBILE_LPM_POLICY=3 > config-5.4.0-60-generic:CONFIG_SATA_MOBILE_LPM_POLICY=3 OK. Thanks. > > From a box with a random RHEL8 kernel I see: > > config-4.18.0-372.2.1.el8.x86_64:CONFIG_SATA_MOBILE_LPM_POLICY=0 > > However I believe that is because RH uses tuned to set policies like this. > They have use cases from embedded up through datacenter. > For example you can see their "desktop" profile overrides it to > to medium_power: > > https://github.com/redhat-performance/tuned/commit/adffbb0ca1537277d5344661e05a705af2520c89 OK. I will check CentOS too, but it should be the same as RHEL. > > Downloaded a random Debian 5.10 kernel package and extracted it: > $ grep LPM config-5.10.0-10-amd64 > CONFIG_SATA_MOBILE_LPM_POLICY=3 Yep, just checked that too. Both Debian stable and testing have 3 as the default. > > Checked arch from their source tree and they also set 3: > https://github.com/archlinux/svntogit-packages/blob/packages/linux/trunk/config#L2798 > > Not sure where to check SUSE. Checking this one now. -- Damien Le Moal Western Digital Research
