| Message ID | 1449881138-19985-1-git-send-email-mitch.a.williams@intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Jeff Kirsher |
| Headers | show |
> From: Intel-wired-lan [intel-wired-lan-bounces@lists.osuosl.org] on behalf of Mitch Williams [mitch.a.williams@intel.com] > Sent: Friday, December 11, 2015 4:45 PM > To: intel-wired-lan@lists.osuosl.org > Subject: [Intel-wired-lan] [next PATCH] igb/igbvf: don't give up > > The driver shouldn't just give up if it fails to get the hardware > mailbox lock. This can happen in a situation where the PF-VF > communication channel is heavily loaded and causes complete > communications failure between the PF and VF drivers. > > Add a counter and a delay. The driver will now retry ten times, waiting > one millsecond between retries. > Signed-off-by: Mitch Williams <mitch.a.williams@intel.com> > --- > drivers/net/ethernet/intel/igb/e1000_mbx.c | 18 ++++++++++++------ > drivers/net/ethernet/intel/igbvf/mbx.c | 20 +++++++++++++------- > 2 files changed, 25 insertions(+), 13 deletions(-) This is getting flagged by checkpatch for using udelay rather than usleep_range. But it does not seem to be considering it an error or even a warning, rather it just seems to come up as a "CHECK:". Is there any reason not to use usleep_range here? -------------------------------------------------------------------------------------------- u1458:[1]/usr/src/kernels/next-queue> git format-patch $item -1 --stdout|./scripts/checkpatch.pl - CHECK: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt #46: FILE: drivers/net/ethernet/intel/igb/e1000_mbx.c:337: + udelay(1000); CHECK: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt #77: FILE: drivers/net/ethernet/intel/igbvf/mbx.c:248: + udelay(1000); total: 0 errors, 0 warnings, 2 checks, 52 lines checked Your patch has style problems, please review. NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. u1458:[1]/usr/src/kernels/next-queue>
Aaron F <aaron.f.brown@intel.com> wrote:
> Is there any reason not to use usleep_range here?
Can the routines ever be called from any atomic context, such as an interrupt or while a lock is held? If so, you can't use any form of sleep.
--
Mark Rustad, Networking Division, Intel Corporation
Yes, these are called from interrupt context. So they can't sleep, no matter how tired they get. -Mitch > -----Original Message----- > From: Rustad, Mark D > Sent: Thursday, December 17, 2015 5:57 PM > To: Brown, Aaron F > Cc: Williams, Mitch A; intel-wired-lan@lists.osuosl.org > Subject: Re: [Intel-wired-lan] [next PATCH] igb/igbvf: don't give up > > Aaron F <aaron.f.brown@intel.com> wrote: > > > Is there any reason not to use usleep_range here? > > Can the routines ever be called from any atomic context, such as an > interrupt or while a lock is held? If so, you can't use any form of sleep. > > -- > Mark Rustad, Networking Division, Intel Corporation
> From: Williams, Mitch A > Sent: Friday, December 18, 2015 8:42 AM > To: Rustad, Mark D; Brown, Aaron F > Cc: intel-wired-lan@lists.osuosl.org > Subject: RE: [Intel-wired-lan] [next PATCH] igb/igbvf: don't give up > > Yes, these are called from interrupt context. So they can't sleep, no matter how tired they get. > -Mitch No matter HOW tired, bummer! Cause for me, sometimes I just can't stay awake another millisecond :) Tested-by: Aaron Brown <aaron.f.brown@intel.com> > > -----Original Message----- > > From: Rustad, Mark D > > Sent: Thursday, December 17, 2015 5:57 PM > > To: Brown, Aaron F > > Cc: Williams, Mitch A; intel-wired-lan@lists.osuosl.org > > Subject: Re: [Intel-wired-lan] [next PATCH] igb/igbvf: don't give up > > > > Aaron F <aaron.f.brown@intel.com> wrote: > > > > > Is there any reason not to use usleep_range here? > > > > Can the routines ever be called from any atomic context, such as an > > interrupt or while a lock is held? If so, you can't use any form of sleep. > > > > -- > > Mark Rustad, Networking Division, Intel Corporation
diff --git a/drivers/net/ethernet/intel/igb/e1000_mbx.c b/drivers/net/ethernet/intel/igb/e1000_mbx.c index 162cc49..10f5c9e 100644 --- a/drivers/net/ethernet/intel/igb/e1000_mbx.c +++ b/drivers/net/ethernet/intel/igb/e1000_mbx.c @@ -322,14 +322,20 @@ static s32 igb_obtain_mbx_lock_pf(struct e1000_hw *hw, u16 vf_number) { s32 ret_val = -E1000_ERR_MBX; u32 p2v_mailbox; + int count = 10; - /* Take ownership of the buffer */ - wr32(E1000_P2VMAILBOX(vf_number), E1000_P2VMAILBOX_PFU); + do { + /* Take ownership of the buffer */ + wr32(E1000_P2VMAILBOX(vf_number), E1000_P2VMAILBOX_PFU); - /* reserve mailbox for vf use */ - p2v_mailbox = rd32(E1000_P2VMAILBOX(vf_number)); - if (p2v_mailbox & E1000_P2VMAILBOX_PFU) - ret_val = 0; + /* reserve mailbox for vf use */ + p2v_mailbox = rd32(E1000_P2VMAILBOX(vf_number)); + if (p2v_mailbox & E1000_P2VMAILBOX_PFU) { + ret_val = 0; + break; + } + udelay(1000); + } while (count-- > 0); return ret_val; } diff --git a/drivers/net/ethernet/intel/igbvf/mbx.c b/drivers/net/ethernet/intel/igbvf/mbx.c index 7b6cb4c..01752f4 100644 --- a/drivers/net/ethernet/intel/igbvf/mbx.c +++ b/drivers/net/ethernet/intel/igbvf/mbx.c @@ -234,13 +234,19 @@ static s32 e1000_check_for_rst_vf(struct e1000_hw *hw) static s32 e1000_obtain_mbx_lock_vf(struct e1000_hw *hw) { s32 ret_val = -E1000_ERR_MBX; - - /* Take ownership of the buffer */ - ew32(V2PMAILBOX(0), E1000_V2PMAILBOX_VFU); - - /* reserve mailbox for VF use */ - if (e1000_read_v2p_mailbox(hw) & E1000_V2PMAILBOX_VFU) - ret_val = E1000_SUCCESS; + int count = 10; + + do { + /* Take ownership of the buffer */ + ew32(V2PMAILBOX(0), E1000_V2PMAILBOX_VFU); + + /* reserve mailbox for VF use */ + if (e1000_read_v2p_mailbox(hw) & E1000_V2PMAILBOX_VFU) { + ret_val = 0; + break; + } + udelay(1000); + } while (count-- > 0); return ret_val; }
The driver shouldn't just give up if it fails to get the hardware mailbox lock. This can happen in a situation where the PF-VF communication channel is heavily loaded and causes complete communications failure between the PF and VF drivers. Add a counter and a delay. The driver will now retry ten times, waiting one millsecond between retries. Signed-off-by: Mitch Williams <mitch.a.williams@intel.com> --- drivers/net/ethernet/intel/igb/e1000_mbx.c | 18 ++++++++++++------ drivers/net/ethernet/intel/igbvf/mbx.c | 20 +++++++++++++------- 2 files changed, 25 insertions(+), 13 deletions(-)