| 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
| ||
|
Message-ID: <cb4bf44e-1399-277f-807b-1a7f26f80c1c@amd.com>
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&data=04%7C01%7Cm
>>> ario.limonciello%40amd.com%7Ca32a202d437544cd2cbb08d9ef9112c0%7C3d
>>> d8961fe4884e608e11a82d994e183d%7C0%7C0%7C637804228648002522%7CU
>>> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
>>> 6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=CbfImBnwc8uV1L5QRBuV
>>> PLkP72wpS9yif1UbUwykhNI%3D&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&data=04%7C01%7CMario.Limonciello%40amd.com%7Cf25ec205dd2e46253a2208d9f0519b39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805055570772355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=POXfSI626inWB7k73CqJF3IMp6y31r%2F%2BugdCXOkvydo%3D&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&data=04%7C01%7CMario.Limonciello%40amd.com%7Cf25ec205dd2e46253a2208d9f0519b39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805055570772355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=vqX1NjUKE0STzvmKSWtJxhrrdQnH%2BmqAiXGuqNMQAt0%3D&reserved=0
>
> [2]:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2F20200&data=04%7C01%7CMario.Limonciello%40amd.com%7Cf25ec205dd2e46253a2208d9f0519b39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805055570772355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=oz3K76NGXrp41wq%2B5QcPUtJG%2BfqxCGOTVojfYb%2BMIlw%3D&reserved=0
>
> [3]:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpci-ids.ucw.cz%2Fread%2FPC%2F1022%2F7900&data=04%7C01%7CMario.Limonciello%40amd.com%7Cf25ec205dd2e46253a2208d9f0519b39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805055570772355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Y32YS5rIxBXTyNQ8VOuTxBTbfsOipBcIPAvmhBhSz7E%3D&reserved=0
>
Powered by blists - more mailing lists