netdev - Re: [net-next 1/9] i40e: poll firmware slower

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141107145819.00007fa0@unknown>
Date:	Fri, 7 Nov 2014 14:58:19 -0800
From:	Jesse Brandeburg <jesse.brandeburg@...el.com>
To:	David Laight <David.Laight@...LAB.COM>
Cc:	'Jeff Kirsher' <jeffrey.t.kirsher@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	Kamil Krawczyk <kamil.krawczyk@...el.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"nhorman@...hat.com" <nhorman@...hat.com>,
	"sassmann@...hat.com" <sassmann@...hat.com>,
	"jogreene@...hat.com" <jogreene@...hat.com>,
	jesse.brandeburg@...el.com
Subject: Re: [net-next 1/9] i40e: poll firmware slower

Thanks for the review David, comments follow.

On Fri, 7 Nov 2014 09:40:08 +0000
David Laight <David.Laight@...LAB.COM> wrote:

> From: Jeff Kirsher
> > From: Kamil Krawczyk <kamil.krawczyk@...el.com>
> > 
> > The code was polling the firmware tail register for completion every
> > 10 microseconds, which is way faster than the firmware can respond.
> > This changes the poll interval to 1ms, which reduces polling CPU
> > utilization, and the number of times we loop.
> 
> Are you sure the code path is allowed to sleep?

Yes, we are never (should never be) in interrupt context when calling
these routines.

> 
> > The maximum delay is still 100ms.
> 
> Actually it is now likely to be up to 200ms or more.
> You could convert the maximum delay check to one that
> looks at jiffies - but maybe it doesn't matter.

Thats okay, this is all init or reset or shutdown level code.  If the
delay goes up it won't hurt anything.

> 
> > --- a/drivers/net/ethernet/intel/i40e/i40e_adminq.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_adminq.c
> > @@ -853,7 +853,6 @@ i40e_status i40e_asq_send_command(struct i40e_hw *hw,
> >  	 */
> >  	if (!details->async && !details->postpone) {
> >  		u32 total_delay = 0;
> > -		u32 delay_len = 10;
> > 
> >  		do {
> >  			/* AQ designers suggest use of head for better
> > @@ -862,8 +861,8 @@ i40e_status i40e_asq_send_command(struct i40e_hw *hw,
> >  			if (i40e_asq_done(hw))
> >  				break;
> >  			/* ugh! delay while spin_lock */
> 
> The comment is not right any more.

yes it should have been removed.

> 
> > -			udelay(delay_len);
> > -			total_delay += delay_len;
> > +			usleep_range(1000, 2000);
> > +			total_delay++;
> >  		} while (total_delay < hw->aq.asq_cmd_timeout);
> >  	}
> > 
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq.h
> > b/drivers/net/ethernet/intel/i40e/i40e_adminq.h
> > index ba38a89..df0bd09 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_adminq.h
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_adminq.h
> > @@ -141,7 +141,7 @@ static inline int i40e_aq_rc_to_posix(u16 aq_rc)
> > 
> >  /* general information */
> >  #define I40E_AQ_LARGE_BUF	512
> > -#define I40E_ASQ_CMD_TIMEOUT	100000  /* usecs */
> > +#define I40E_ASQ_CMD_TIMEOUT	100  /* msecs */
> 
> It looks like this value is written to asq_cmd_timeout, that makes
> be wonder whether anything else can change it - otherwise the compile
> time constant would be used.
> Changing the units has broken anything else that modifies the value.

I pretty much agree with you, but I can tell you why it's there.
Currently nothing in the code changes it.  The code was designed such
that it can run on hardware requiring different timeouts.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html