[Intel-wired-lan] [PATCH v3] e1000e: PCIm function state support
Neftin, Sasha
sasha.neftin at intel.com
Wed Jul 10 14:53:35 UTC 2019
On 6/28/2019 11:57, Paul Menzel wrote:
> Dear Vitaly,
>
>
> Thank you for the patch. Some suggestions below.
>
> 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?
>
>> Signed-off-by: Vitaly Lifshits <vitaly.lifshits at 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?
>
>> +
>> #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?
>
>> 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?
>
>> + 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
>
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
>
Paul, thanks for your comments. I worked with Vitalik on this - we will
address your suggestions and re-submit the patch.
More information about the Intel-wired-lan
mailing list