[Intel-wired-lan] [PATCH v3] e1000e: PCIm function state support

[Paul Menzel <pmenzel@molgen.mpg.de>]

Dear Vitaly,


I am adding the list back, so that the Linux kernel experts can
chime and correct my answers/suggestions.


On 7/16/19 1:28 PM, Lifshits, Vitaly wrote:
> On 6/28/2019 11:57, Paul Menzel wrote:

>> On 06/25/19 16:39, Vitaly Lifshits wrote:
>>> Due to commit: 5d8682588605 ("[misc] mei: me: allow runtime
>>> ??????????? pm for platform with D0i3")
>> Do not indent it but integrate it into the line.
>>
>>> When disconnecting the cable and reconnecting it the NIC
>> s/When/when/
>>
>>> enters DMoff state. This caused wrong link indication
>>> and duplex mismatch. This bug is described in:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1689436
>> Isn?t there a tag Link: or Bugzilla: to mention these URLs?
>> Maybe add it below? (See `git log --grep bugzilla` for how this
>> is used.)
>>
>>> Checking PCIm function state and performing PHY reset after a
>>> timeout in watchdog task solves this issue.
>> In what data sheet is the function state described?
> PCIm function state isn't mentioned in the I219 data sheet.

It should be updated then. ;-)

> However the DMoff state (which is a pcim state) is mentioned in it.
> In I218 data sheet this state is mentioned.

Thanks. I found the data sheet.

https://datasheet.octopart.com/WGI218LM-S-LK3B-Intel-datasheet-76215422.pdf

>>> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>>> ---
>>>
>>> V2: Fixed typos in commit massage
>>> V3: Fixed minor typo
>>> ---
>>> ? drivers/net/ethernet/intel/e1000e/defines.h |? 3 +++
>>> ? drivers/net/ethernet/intel/e1000e/netdev.c? | 18 +++++++++++++++++-
>>> ? 2 files changed, 20 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/e1000e/defines.h b/drivers/net/ethernet/intel/e1000e/defines.h
>>> index fd550dee4982..13877fe300f1 100644
>>> --- a/drivers/net/ethernet/intel/e1000e/defines.h
>>> +++ b/drivers/net/ethernet/intel/e1000e/defines.h
>>> @@ -222,6 +222,9 @@
>>> ? #define E1000_STATUS_PHYRA????? 0x00000400????? /* PHY Reset Asserted */
>>> ? #define E1000_STATUS_GIO_MASTER_ENABLE??? 0x00080000??? /* Master Req status */
>>> ? +/* PCIm function state */
>>> +#define E1000_STATUS_PCIM_STATE???????? 0x40000000
>> Shouldn?t the name be something E1000_STATUS_PCIM_STATE_DMOFF?
> This bit indicates the pcim state if it is set then the device is in
> DMoff state.

Indeed. Shouldn?t the macro name be changed to my suggestion to better
describe it?s meaning?

>>> +
>>> ? #define HALF_DUPLEX 1
>>> ? #define FULL_DUPLEX 2
>>> ? diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
>>> index b081a1ef6859..c6a10fd30e4e 100644
>>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>>> @@ -5173,8 +5173,9 @@ static void e1000_watchdog_task(struct work_struct *work)
>>> ????? struct e1000_mac_info *mac = &adapter->hw.mac;
>>> ????? struct e1000_phy_info *phy = &adapter->hw.phy;
>>> ????? struct e1000_ring *tx_ring = adapter->tx_ring;
>>> +??? u32 dmoff_exit_timeout = 100, tries = 0;
>> Shouldn?t a macro be used for the time-out value?
> Just to make sure I understand you correctly, did you mean that I
> should set a defined value like DMOFF_EXIT_TIMEOUT 100?

Yes, that is what I meant. But I am no C or Linux expert, so I do not
know, if macros are wanted seeing that they do not have a type.

If you go with a C variable, it should be `unsigned int` and `const`?
I heard enums are an alternative to macros.

>>> ????? struct e1000_hw *hw = &adapter->hw;
>>> -??? u32 link, tctl;
>>> +??? u32 link, tctl, pcim_state;
>>> ? ????? if (test_bit(__E1000_DOWN, &adapter->state))
>>> ????????? return;
>>> @@ -5199,6 +5200,21 @@ static void e1000_watchdog_task(struct work_struct *work)
>>> ????????????? /* Cancel scheduled suspend requests. */
>>> ????????????? pm_runtime_resume(netdev->dev.parent);
>>> ? +??????????? /* Checking if MAC is in DMoff state*/
>>> +??????????? pcim_state = er32(STATUS);
>>> +??????????? while (pcim_state & E1000_STATUS_PCIM_STATE) {
>>> +??????????????? if (tries++ == dmoff_exit_timeout) {
>>> +??????????????????? e_dbg("Error in exiting dmoff\n");
>> Shouldn?t this be a user visible error message? If so, please elaborate and
>> mention the time-out.
>>
>>> Couldn?t exit DMoff after %i s. Your card might not work correctly,
>>> TIMEOUTMACRONAME
>>> +??????????????????? break;
>>> +??????????????? }
>>> +??????????????? usleep_range(10000, 20000);
>>> +??????????????? pcim_state = er32(STATUS);
>>> +
>>> +??????????????? /* Checking if MAC exited DMoff state */
>>> +??????????????? if (!(pcim_state & E1000_STATUS_PCIM_STATE))
>> If the macro name is more specific, the comment could be removed. If not,
>> the comment should use imperative mood (as below): Check if ?.
>>
>> Also can the while loop and if condition be refactored, as the condition
>> check if the same?
> The thing is that I don't want to perform phy_hw_reset if the device wasn't in this state at all.
> Does removing the if from here and using another one after the loop is a good solution for this?
> (By using tries variable)

If the if statement condition `!(pcim_state & E1000_STATUS_PCIM_STATE)` is
true, then the while loop condition is false, right? So the if statement can
at least be moved *outside* the loop (the compiler probably does it already).

Couldn?t you write it like below?

1.  do-while-loop: Saves one `pcim_state` assignment, but has a mandatory
    delay of `usleep_range`.

        do {
                if (tries++ == dmoff_exit_timeout) {
                        e_dbg("Error in exiting dmoff\n");
                        break;
                }
    
                pcim_state = er32(STATUS);
                usleep_range(10000, 20000);
        } while (pcim_state & E1000_STATUS_PCIM_STATE)
    
        if (!(pcim_state & E1000_STATUS_PCIM_STATE))
                e1000_phy_hw_reset(&adapter->hw);

2.  while-loop: Has an additional pcim_state assignment before the loop.

        pcim_state = er32(STATUS);
        while (pcim_state & E1000_STATUS_PCIM_STATE) {
                if (tries++ == dmoff_exit_timeout) {
                        e_dbg("Error in exiting dmoff\n");
                        break;
                }
        
                pcim_state = er32(STATUS);
                usleep_range(10000, 20000);
        }
        
        if (!(pcim_state & E1000_STATUS_PCIM_STATE))
                e1000_phy_hw_reset(&adapter->hw);

(I used your macro name. Should you decide to update it, it needs to
be updated of course.)

>>> +??????????????????? e1000_phy_hw_reset(&adapter->hw);
>>> +??????????? }
>>> +
>>> ????????????? /* update snapshot of PHY registers on LSC */
>>> ????????????? e1000_phy_read_status(adapter);
>>> ????????????? mac->ops.get_link_up_info(&adapter->hw,


Kind regards,

Paul

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5174 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20190719/b95368c7/attachment-0001.p7s>