I219 on HP EliteOne 840 All in One cannot work after s2idle resume
when the link speed is Gigabit, Wake-on-LAN is enabled and then set
the link down before suspend. No issue found when requesting driver
to configure S0ix. Add workround to let ADP_I219_LM17 use the dirver
configured S0ix.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216926
Signed-off-by: Jiajia Liu <[email protected]>
---
It's regarding the bug above, it looks it's causued by the ME S0ix.
And is there a method to make the ME S0ix path work?
drivers/net/ethernet/intel/e1000e/netdev.c | 25 ++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 04acd1a992fa..7ee759dbd09d 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -6330,6 +6330,23 @@ static void e1000e_flush_lpic(struct pci_dev *pdev)
pm_runtime_put_sync(netdev->dev.parent);
}
+static u16 me_s0ix_blacklist[] = {
+ E1000_DEV_ID_PCH_ADP_I219_LM17,
+ 0
+};
+
+static bool e1000e_check_me_s0ix_blacklist(const struct e1000_adapter *adapter)
+{
+ u16 *list;
+
+ for (list = me_s0ix_blacklist; *list; list++) {
+ if (*list == adapter->pdev->device)
+ return true;
+ }
+
+ return false;
+}
+
/* S0ix implementation */
static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
{
@@ -6337,6 +6354,9 @@ static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
u32 mac_data;
u16 phy_data;
+ if (e1000e_check_me_s0ix_blacklist(adapter))
+ goto req_driver;
+
if (er32(FWSM) & E1000_ICH_FWSM_FW_VALID &&
hw->mac.type >= e1000_pch_adp) {
/* Request ME configure the device for S0ix */
@@ -6346,6 +6366,7 @@ static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
trace_e1000e_trace_mac_register(mac_data);
ew32(H2ME, mac_data);
} else {
+req_driver:
/* Request driver configure the device to S0ix */
/* Disable the periodic inband message,
* don't request PCIe clock in K1 page770_17[10:9] = 10b
@@ -6488,6 +6509,9 @@ static void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter)
u16 phy_data;
u32 i = 0;
+ if (e1000e_check_me_s0ix_blacklist(adapter))
+ goto req_driver;
+
if (er32(FWSM) & E1000_ICH_FWSM_FW_VALID &&
hw->mac.type >= e1000_pch_adp) {
/* Keep the GPT clock enabled for CSME */
@@ -6523,6 +6547,7 @@ static void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter)
else
e_dbg("DPG_EXIT_DONE cleared after %d msec\n", i * 10);
} else {
+req_driver:
/* Request driver unconfigure the device from S0ix */
/* Disable the Dynamic Power Gating in the MAC */
--
2.30.2
On 1/17/2023 2:26 AM, Jiajia Liu wrote:
> I219 on HP EliteOne 840 All in One cannot work after s2idle resume
> when the link speed is Gigabit, Wake-on-LAN is enabled and then set
> the link down before suspend. No issue found when requesting driver
> to configure S0ix. Add workround to let ADP_I219_LM17 use the dirver
> configured S0ix.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216926
> Signed-off-by: Jiajia Liu <[email protected]>
> ---
>
> It's regarding the bug above, it looks it's causued by the ME S0ix.
> And is there a method to make the ME S0ix path work?
>
No idea. It does seem better to disable S0ix if it doesn't work properly
first though...
> drivers/net/ethernet/intel/e1000e/netdev.c | 25 ++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 04acd1a992fa..7ee759dbd09d 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -6330,6 +6330,23 @@ static void e1000e_flush_lpic(struct pci_dev *pdev)
> pm_runtime_put_sync(netdev->dev.parent);
> }
>
> +static u16 me_s0ix_blacklist[] = {
> + E1000_DEV_ID_PCH_ADP_I219_LM17,
> + 0
> +};
> +
> +static bool e1000e_check_me_s0ix_blacklist(const struct e1000_adapter *adapter)
> +{
> + u16 *list;
> +
> + for (list = me_s0ix_blacklist; *list; list++) {
> + if (*list == adapter->pdev->device)
> + return true;
> + }
> +
> + return false;
> +}
The name of this function seems odd..? "check_me"? It also seems like we
could just do a simple switch/case on the device ID or similar.
Maybe: "e1000e_device_supports_s0ix"?
> +
> /* S0ix implementation */
> static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
> {
> @@ -6337,6 +6354,9 @@ static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
> u32 mac_data;
> u16 phy_data;
>
> + if (e1000e_check_me_s0ix_blacklist(adapter))
> + goto req_driver;
> +
> if (er32(FWSM) & E1000_ICH_FWSM_FW_VALID &&
> hw->mac.type >= e1000_pch_adp) {
> /* Request ME configure the device for S0ix */
The related code also seems to already perform some set of mac checks
here...
> @@ -6346,6 +6366,7 @@ static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
> trace_e1000e_trace_mac_register(mac_data);
> ew32(H2ME, mac_data);
> } else {
> +req_driver:> /* Request driver configure the device to S0ix */
> /* Disable the periodic inband message,
> * don't request PCIe clock in K1 page770_17[10:9] = 10b
> @@ -6488,6 +6509,9 @@ static void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter)
> u16 phy_data;
> u32 i = 0;
>
> + if (e1000e_check_me_s0ix_blacklist(adapter))
> + goto req_driver;
> +
Why not just combine this check into the statement below rather than
adding a goto?
> if (er32(FWSM) & E1000_ICH_FWSM_FW_VALID &&
> hw->mac.type >= e1000_pch_adp) {
> /* Keep the GPT clock enabled for CSME */
> @@ -6523,6 +6547,7 @@ static void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter)
> else
> e_dbg("DPG_EXIT_DONE cleared after %d msec\n", i * 10);
> } else {
> +req_driver:
> /* Request driver unconfigure the device from S0ix */
>
> /* Disable the Dynamic Power Gating in the MAC */
On 1/17/2023 21:34, Jacob Keller wrote:
>
>
> On 1/17/2023 2:26 AM, Jiajia Liu wrote:
>> I219 on HP EliteOne 840 All in One cannot work after s2idle resume
>> when the link speed is Gigabit, Wake-on-LAN is enabled and then set
>> the link down before suspend. No issue found when requesting driver
>> to configure S0ix. Add workround to let ADP_I219_LM17 use the dirver
>> configured S0ix.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216926
>> Signed-off-by: Jiajia Liu <[email protected]>
>> ---
>>
>> It's regarding the bug above, it looks it's causued by the ME S0ix.
>> And is there a method to make the ME S0ix path work?
No. This is a fragile approach. ME must get the message from us
(unconfigure the device from s0ix). Otherwise, ME will continue to
access LAN resources and the controller could get stuck.
I see two ways:
1. you always can skip s0ix flow by priv_flag
2. Especially in this case (HP platform) - please, contact HP (what is
the ME version on this system, and how was it released...). HP will open
a ticket with Intel. (then we can involve the ME team)
>>
>
> No idea. It does seem better to disable S0ix if it doesn't work properly
> first though...
>
>> drivers/net/ethernet/intel/e1000e/netdev.c | 25 ++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
>> index 04acd1a992fa..7ee759dbd09d 100644
>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>> @@ -6330,6 +6330,23 @@ static void e1000e_flush_lpic(struct pci_dev *pdev)
>> pm_runtime_put_sync(netdev->dev.parent);
>> }
>>
>> +static u16 me_s0ix_blacklist[] = {
>> + E1000_DEV_ID_PCH_ADP_I219_LM17,
>> + 0
>> +};
>> +
>> +static bool e1000e_check_me_s0ix_blacklist(const struct e1000_adapter *adapter)
>> +{
>> + u16 *list;
>> +
>> + for (list = me_s0ix_blacklist; *list; list++) {
>> + if (*list == adapter->pdev->device)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>
> The name of this function seems odd..? "check_me"? It also seems like we
> could just do a simple switch/case on the device ID or similar.
>
> Maybe: "e1000e_device_supports_s0ix"?
>
>> +
>> /* S0ix implementation */
>> static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
>> {
>> @@ -6337,6 +6354,9 @@ static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
>> u32 mac_data;
>> u16 phy_data;
>>
>> + if (e1000e_check_me_s0ix_blacklist(adapter))
>> + goto req_driver;
>> +
>> if (er32(FWSM) & E1000_ICH_FWSM_FW_VALID &&
>> hw->mac.type >= e1000_pch_adp) {
>> /* Request ME configure the device for S0ix */
>
>
> The related code also seems to already perform some set of mac checks
> here...
>
>> @@ -6346,6 +6366,7 @@ static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
>> trace_e1000e_trace_mac_register(mac_data);
>> ew32(H2ME, mac_data);
>> } else {
>> +req_driver:> /* Request driver configure the device to S0ix */
>> /* Disable the periodic inband message,
>> * don't request PCIe clock in K1 page770_17[10:9] = 10b
>> @@ -6488,6 +6509,9 @@ static void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter)
>> u16 phy_data;
>> u32 i = 0;
>>
>> + if (e1000e_check_me_s0ix_blacklist(adapter))
>> + goto req_driver;
>> +
>
> Why not just combine this check into the statement below rather than
> adding a goto?
>
>> if (er32(FWSM) & E1000_ICH_FWSM_FW_VALID &&
>> hw->mac.type >= e1000_pch_adp) {
>> /* Keep the GPT clock enabled for CSME */
>> @@ -6523,6 +6547,7 @@ static void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter)
>> else
>> e_dbg("DPG_EXIT_DONE cleared after %d msec\n", i * 10);
>> } else {
>> +req_driver:
>> /* Request driver unconfigure the device from S0ix */
>>
>> /* Disable the Dynamic Power Gating in the MAC */
> _______________________________________________
> Intel-wired-lan mailing list
> [email protected]
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
On Wed, Jan 18, 2023 at 1:20 PM Neftin, Sasha <[email protected]> wrote:
>
> On 1/17/2023 21:34, Jacob Keller wrote:
> >
> >
> > On 1/17/2023 2:26 AM, Jiajia Liu wrote:
> >> I219 on HP EliteOne 840 All in One cannot work after s2idle resume
> >> when the link speed is Gigabit, Wake-on-LAN is enabled and then set
> >> the link down before suspend. No issue found when requesting driver
> >> to configure S0ix. Add workround to let ADP_I219_LM17 use the dirver
> >> configured S0ix.
> >>
> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216926
> >> Signed-off-by: Jiajia Liu <[email protected]>
> >> ---
> >>
> >> It's regarding the bug above, it looks it's causued by the ME S0ix.
> >> And is there a method to make the ME S0ix path work?
> No. This is a fragile approach. ME must get the message from us
> (unconfigure the device from s0ix). Otherwise, ME will continue to
> access LAN resources and the controller could get stuck.
> I see two ways:
> 1. you always can skip s0ix flow by priv_flag
> 2. Especially in this case (HP platform) - please, contact HP (what is
> the ME version on this system, and how was it released...). HP will open
> a ticket with Intel. (then we can involve the ME team)
HP released BIOS including ME firmware on their website HP.com at
https://support.hp.com/my-en/drivers/selfservice/hp-eliteone-840-23.8-inch-g9-all-in-one-desktop-pc/2101132389.
There is upgrade interface on the BIOS setup menu which can connect
HP.com and upgrade to newer BIOS.
The initial ME version was v16.0.15.1735 from BIOS 02.03.04.
Then I upgraded to the latest one v16.1.25.1932v3 from BIOS 02.06.01
released on Nov 28, 2022. Both of them can produce this issue.
I have only one setup. Is it possible to try on your system which has the
same I219-LM to see if it's platform specific or not?
> >>
> >
> > No idea. It does seem better to disable S0ix if it doesn't work properly
> > first though...
> >
> >> drivers/net/ethernet/intel/e1000e/netdev.c | 25 ++++++++++++++++++++++
> >> 1 file changed, 25 insertions(+)
> >>
> >> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> >> index 04acd1a992fa..7ee759dbd09d 100644
> >> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> >> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> >> @@ -6330,6 +6330,23 @@ static void e1000e_flush_lpic(struct pci_dev *pdev)
> >> pm_runtime_put_sync(netdev->dev.parent);
> >> }
> >>
> >> +static u16 me_s0ix_blacklist[] = {
> >> + E1000_DEV_ID_PCH_ADP_I219_LM17,
> >> + 0
> >> +};
> >> +
> >> +static bool e1000e_check_me_s0ix_blacklist(const struct e1000_adapter *adapter)
> >> +{
> >> + u16 *list;
> >> +
> >> + for (list = me_s0ix_blacklist; *list; list++) {
> >> + if (*list == adapter->pdev->device)
> >> + return true;
> >> + }
> >> +
> >> + return false;
> >> +}
> >
> > The name of this function seems odd..? "check_me"? It also seems like we
> > could just do a simple switch/case on the device ID or similar.
> >
> > Maybe: "e1000e_device_supports_s0ix"?
> >
> >> +
> >> /* S0ix implementation */
> >> static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
> >> {
> >> @@ -6337,6 +6354,9 @@ static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
> >> u32 mac_data;
> >> u16 phy_data;
> >>
> >> + if (e1000e_check_me_s0ix_blacklist(adapter))
> >> + goto req_driver;
> >> +
> >> if (er32(FWSM) & E1000_ICH_FWSM_FW_VALID &&
> >> hw->mac.type >= e1000_pch_adp) {
> >> /* Request ME configure the device for S0ix */
> >
> >
> > The related code also seems to already perform some set of mac checks
> > here...
> >
> >> @@ -6346,6 +6366,7 @@ static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
> >> trace_e1000e_trace_mac_register(mac_data);
> >> ew32(H2ME, mac_data);
> >> } else {
> >> +req_driver:> /* Request driver configure the device to S0ix */
> >> /* Disable the periodic inband message,
> >> * don't request PCIe clock in K1 page770_17[10:9] = 10b
> >> @@ -6488,6 +6509,9 @@ static void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter)
> >> u16 phy_data;
> >> u32 i = 0;
> >>
> >> + if (e1000e_check_me_s0ix_blacklist(adapter))
> >> + goto req_driver;
> >> +
> >
> > Why not just combine this check into the statement below rather than
> > adding a goto?
> >
> >> if (er32(FWSM) & E1000_ICH_FWSM_FW_VALID &&
> >> hw->mac.type >= e1000_pch_adp) {
> >> /* Keep the GPT clock enabled for CSME */
> >> @@ -6523,6 +6547,7 @@ static void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter)
> >> else
> >> e_dbg("DPG_EXIT_DONE cleared after %d msec\n", i * 10);
> >> } else {
> >> +req_driver:
> >> /* Request driver unconfigure the device from S0ix */
> >>
> >> /* Disable the Dynamic Power Gating in the MAC */
> > _______________________________________________
> > Intel-wired-lan mailing list
> > [email protected]
> > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
>
On 1/18/2023 11:08, Jia Liu wrote:
> On Wed, Jan 18, 2023 at 1:20 PM Neftin, Sasha <[email protected]> wrote:
>>
>> On 1/17/2023 21:34, Jacob Keller wrote:
>>>
>>>
>>> On 1/17/2023 2:26 AM, Jiajia Liu wrote:
>>>> I219 on HP EliteOne 840 All in One cannot work after s2idle resume
>>>> when the link speed is Gigabit, Wake-on-LAN is enabled and then set
>>>> the link down before suspend. No issue found when requesting driver
>>>> to configure S0ix. Add workround to let ADP_I219_LM17 use the dirver
>>>> configured S0ix.
>>>>
>>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216926
>>>> Signed-off-by: Jiajia Liu <[email protected]>
>>>> ---
>>>>
>>>> It's regarding the bug above, it looks it's causued by the ME S0ix.
>>>> And is there a method to make the ME S0ix path work?
>> No. This is a fragile approach. ME must get the message from us
>> (unconfigure the device from s0ix). Otherwise, ME will continue to
>> access LAN resources and the controller could get stuck.
>> I see two ways:
>> 1. you always can skip s0ix flow by priv_flag
>> 2. Especially in this case (HP platform) - please, contact HP (what is
>> the ME version on this system, and how was it released...). HP will open
>> a ticket with Intel. (then we can involve the ME team)
>
> HP released BIOS including ME firmware on their website HP.com at
> https://support.hp.com/my-en/drivers/selfservice/hp-eliteone-840-23.8-inch-g9-all-in-one-desktop-pc/2101132389.
> There is upgrade interface on the BIOS setup menu which can connect
> HP.com and upgrade to newer BIOS.
>
> The initial ME version was v16.0.15.1735 from BIOS 02.03.04.
> Then I upgraded to the latest one v16.1.25.1932v3 from BIOS 02.06.01
> released on Nov 28, 2022. Both of them can produce this issue.
>
> I have only one setup. Is it possible to try on your system which has the
> same I219-LM to see if it's platform specific or not?
Yes, s0ix flows works on our platforms.
>
>>>>
>>>
>>> No idea. It does seem better to disable S0ix if it doesn't work properly
>>> first though...
>>>
>>>> drivers/net/ethernet/intel/e1000e/netdev.c | 25 ++++++++++++++++++++++
>>>> 1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
>>>> index 04acd1a992fa..7ee759dbd09d 100644
>>>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>>>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>>>> @@ -6330,6 +6330,23 @@ static void e1000e_flush_lpic(struct pci_dev *pdev)
>>>> pm_runtime_put_sync(netdev->dev.parent);
>>>> }
>>>>
>>>> +static u16 me_s0ix_blacklist[] = {
>>>> + E1000_DEV_ID_PCH_ADP_I219_LM17,
>>>> + 0
>>>> +};
>>>> +
>>>> +static bool e1000e_check_me_s0ix_blacklist(const struct e1000_adapter *adapter)
>>>> +{
>>>> + u16 *list;
>>>> +
>>>> + for (list = me_s0ix_blacklist; *list; list++) {
>>>> + if (*list == adapter->pdev->device)
>>>> + return true;
>>>> + }
>>>> +
>>>> + return false;
>>>> +}
>>>
>>> The name of this function seems odd..? "check_me"? It also seems like we
>>> could just do a simple switch/case on the device ID or similar.
>>>
>>> Maybe: "e1000e_device_supports_s0ix"?
>>>
>>>> +
>>>> /* S0ix implementation */
>>>> static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
>>>> {
>>>> @@ -6337,6 +6354,9 @@ static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
>>>> u32 mac_data;
>>>> u16 phy_data;
>>>>
>>>> + if (e1000e_check_me_s0ix_blacklist(adapter))
>>>> + goto req_driver;
>>>> +
>>>> if (er32(FWSM) & E1000_ICH_FWSM_FW_VALID &&
>>>> hw->mac.type >= e1000_pch_adp) {
>>>> /* Request ME configure the device for S0ix */
>>>
>>>
>>> The related code also seems to already perform some set of mac checks
>>> here...
>>>
>>>> @@ -6346,6 +6366,7 @@ static void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter)
>>>> trace_e1000e_trace_mac_register(mac_data);
>>>> ew32(H2ME, mac_data);
>>>> } else {
>>>> +req_driver:> /* Request driver configure the device to S0ix */
>>>> /* Disable the periodic inband message,
>>>> * don't request PCIe clock in K1 page770_17[10:9] = 10b
>>>> @@ -6488,6 +6509,9 @@ static void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter)
>>>> u16 phy_data;
>>>> u32 i = 0;
>>>>
>>>> + if (e1000e_check_me_s0ix_blacklist(adapter))
>>>> + goto req_driver;
>>>> +
>>>
>>> Why not just combine this check into the statement below rather than
>>> adding a goto?
>>>
>>>> if (er32(FWSM) & E1000_ICH_FWSM_FW_VALID &&
>>>> hw->mac.type >= e1000_pch_adp) {
>>>> /* Keep the GPT clock enabled for CSME */
>>>> @@ -6523,6 +6547,7 @@ static void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter)
>>>> else
>>>> e_dbg("DPG_EXIT_DONE cleared after %d msec\n", i * 10);
>>>> } else {
>>>> +req_driver:
>>>> /* Request driver unconfigure the device from S0ix */
>>>>
>>>> /* Disable the Dynamic Power Gating in the MAC */
>>> _______________________________________________
>>> Intel-wired-lan mailing list
>>> [email protected]
>>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
>>