linux-kernel - Re: [PATCH 1/2] ahci: Add Green Sardine vendor ID as board_ahci

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 15 Feb 2022 20:56:44 -0600
From:   "Limonciello, Mario" <mario.limonciello@....com>
To:     Paul Menzel <pmenzel@...gen.mpg.de>,
        Hans de Goede <hdegoede@...hat.com>
Cc:     linux-ide@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
        Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@....com>
Subject: Re: [PATCH 1/2] ahci: Add Green Sardine vendor ID as
 board_ahci_mobile

On 2/15/2022 01:05, Paul Menzel wrote:
> Dear Mario,
> 
> 
> Am 14.02.22 um 17:07 schrieb Limonciello, Mario:
> 
>> +Hans
>>
>>> (For the records, is part of Linux since 5.16-rc2 (commit 
>>> 1527f69204fe).)
>>>
>>> Am 12.11.21 um 21:15 schrieb Mario Limonciello:
>>>> AMD requires that the SATA controller be configured for devsleep in 
>>>> order
>>>> for S0i3 entry to work properly.
>>>>
>>>> commit b1a9585cc396 ("ata: ahci: Enable DEVSLP by default on x86 with
>>>> SLP_S0") sets up a kernel policy to enable devsleep on Intel mobile
>>>> platforms that are using s0ix.  Add the PCI ID for the SATA 
>>>> controller in
>>>> Green Sardine platforms to extend this policy by default for AMD based
>>>> systems using s0i3 as well.
>>>>
>>>> Cc: Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@....com>
>>>> BugLink:
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugz
>>> illa.kernel.org%2Fshow_bug.cgi%3Fid%3D214091&amp;data=04%7C01%7Cm
>>> ario.limonciello%40amd.com%7Ca32a202d437544cd2cbb08d9ef9112c0%7C3d
>>> d8961fe4884e608e11a82d994e183d%7C0%7C0%7C637804228648002522%7CU
>>> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
>>> 6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=CbfImBnwc8uV1L5QRBuV
>>> PLkP72wpS9yif1UbUwykhNI%3D&amp;reserved=0
> 
> You have to love Microsoft Outlook.
> 
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>>>> ---
>>>>    drivers/ata/ahci.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>>> index d60f34718b5d..1e1167e725a4 100644
>>>> --- a/drivers/ata/ahci.c
>>>> +++ b/drivers/ata/ahci.c
>>>> @@ -438,6 +438,7 @@ static const struct pci_device_id ahci_pci_tbl[] 
>>>> = {
>>>>        /* AMD */
>>>>        { PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
>>>>        { PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>>>> +    { PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green 
>>>> Sardine */
>>>
>>> Aren't 0x7900 and 0x7901 the same device only in different modes? I
>>> wonder, why different boards and different comments are used.
>>
>> No they aren't the same device in different modes.
>>
>> Reference:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fen%2Fsupport%2Ftech-docs%2Fprocessor-programming-reference-ppr-for-amd-family-19h-model-51h-revision-a1&amp;data=04%7C01%7CMario.Limonciello%40amd.com%7Cf25ec205dd2e46253a2208d9f0519b39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805055570772355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=POXfSI626inWB7k73CqJF3IMp6y31r%2F%2BugdCXOkvydo%3D&amp;reserved=0 
>>
>> Page 33 has a table.
> 
> That table misses 0x7900h. (Where can I find it?) coreboot has 0x7900 
> defined for Cezanne:
> 
>      src/include/device/pci_ids.h:#define PCI_DEVICE_ID_AMD_CZ_SATA  0x7900
>      src/soc/amd/stoneyridge/include/soc/pci_devs.h: * SATA_IDE_IDEVID 
>               0x7900
> 

You can see that 0x7900 was introduced in the kernel back in 2013; well 
in advance of the controller used in Cezanne.

So I really don't believe that CZ in that context for coreboot means 
Cezanne.  It's from an earlier product.  I haven't referenced any older 
documentation to confirm anything but if I were to venture a guess based 
on those names you pasted it's probably introduced with Carrizo and 
continues to be used up until Stoney Ridge.


> The PCI database too [3]:
> 
>> FCH SATA Controller [IDE mode]

Just FYI the strings in the PCI database is based on empirical user 
submissions.  You shouldn't rely upon it to tell you what generations 
different devices are in.

For example I found recently on some family 19h laptops that they claim 
to have a family 17h DMIC according to GNOME Settings which stems back 
to a string that was in the PCI database.

When I encountered this I submitted a correction to make it more 
generic, but I'm sure there are plenty more like this.

> 
> 
>>> Additionally, the device 0x7901 is also present in desktop systems like
>>> Dell OptiPlex 5055 and MSI B350 MORTAR. Is `board_ahci_mobile` the right
>>> board then? Or should the flag `AHCI_HFLAG_IS_MOBILE` be renamed to
>>> avoid confusion?
>>
>> Are you having a problem or just want clarity in the enum definition?
> 
> It’s more about clarity, and understanding the two different entries.
> 
>> This was introduced by Hans in commit ebb82e3c79d2a to set LPM policy 
>> properly
>> for a number of mobile devices.
>>
>> My opinion here is that the policy being for "mobile" devices only is 
>> short sighted as power
>> consumption policy on desktops is also relevant as OEMs ship desktops 
>> that need to meet
>> various power regulations for those too.
>>
>> So I would be in the camp for renaming the flags.
> 
> Why can’t the LPM policy, if available(?), not be set for `board_ahci` 
> by default, so `board_ahci_mobile` could be removed?
> 
>>>>        /* AMD is using RAID class only for ahci controllers */
>>>>        { PCI_VENDOR_ID_AMD, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
>>>>          PCI_CLASS_STORAGE_RAID << 8, 0xffffff, board_ahci },
> 
> 
> Kind regards,
> 
> Paul
> 
> 
> [1]: 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2F10418&amp;data=04%7C01%7CMario.Limonciello%40amd.com%7Cf25ec205dd2e46253a2208d9f0519b39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805055570772355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=vqX1NjUKE0STzvmKSWtJxhrrdQnH%2BmqAiXGuqNMQAt0%3D&amp;reserved=0 
> 
> [2]: 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2F20200&amp;data=04%7C01%7CMario.Limonciello%40amd.com%7Cf25ec205dd2e46253a2208d9f0519b39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805055570772355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=oz3K76NGXrp41wq%2B5QcPUtJG%2BfqxCGOTVojfYb%2BMIlw%3D&amp;reserved=0 
> 
> [3]: 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpci-ids.ucw.cz%2Fread%2FPC%2F1022%2F7900&amp;data=04%7C01%7CMario.Limonciello%40amd.com%7Cf25ec205dd2e46253a2208d9f0519b39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805055570772355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Y32YS5rIxBXTyNQ8VOuTxBTbfsOipBcIPAvmhBhSz7E%3D&amp;reserved=0 
>