[Intel-wired-lan] [PATCHv2] ixgbe: Wait for 1ms, not 1us, after RST

[Intel-wired-lan] [PATCHv2] ixgbe: Wait for 1ms, not 1us, after RST

[Peter Hurley]

On 10/27/2015 02:35 PM, ND Linux CI Server wrote:
> Greetings,
> 
> This email is automatically generated by ND's Linux Patch Testing framework
> based on aiaiai. I have performed some automatic testing of a patch (series)
> you submitted to intel-wired-lan at lists.osuosl.org
> 
> The following contains output of any tests which failed to pass, and might be
> the result of developer error. The tests performed include but may not be
> limited to checkpatch.pl, bisection testing, compilation on a default kernel
> config, coccinelle scripts, cppcheck, and smatch.
> 
> If you have received this email in error, or believe that aiaiai has detected a
> false positive, please email Jacob Keller <jacob.e.keller@intel.com>.

False positive.

As long as the delay is at least 1ms (which is guaranteed), slightly longer
delays (relative to the existing reset delay of 100ms) are not harmful.

Use of usleep_range() would be unnecessary overkill for the purpose.

Regards,
Peter Hurley


> ---
> 
> I have tested your changes
> 
> [Intel-wired-lan] [PATCHv2] ixgbe: Wait for 1ms, not 1us, after RST
> 
> Project: net (net-current development queue)
> 
> Configurations: intel_defconfig,x86
> 
> Tested the patch(es) on top of the following commits:
> 505b857 ixgbe: Reset interface after enabling SR-IOV
> ce9d9b8 net: sysctl: fix a kmemleak warning
> 1acea4f ppp: fix pppoe_dev deletion condition in pppoe_release()
> f6b8dec9 af_key: fix two typos
> 
> --------------------------------------------------------------------------------
> 
> Successfully built configuration "intel_defconfig,x86", no issues.
> 
> --------------------------------------------------------------------------------
> 
> checkpatch.pl has some complaints:
> 
> --------------------------------------------------------------------------------
> 
> checkpatch.pl results for patch "[PATCH] ixgbe: Wait for 1ms, not 1us, after RST"
> 
> WARNING:MSLEEP: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> #29: FILE: drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c:119:
> +	msleep(1);
> 
> total: 0 errors, 1 warnings, 0 checks, 13 lines checked
> 
> --------------------------------------------------------------------------------

Re: [Intel-wired-lan] [PATCHv2] ixgbe: Wait for 1ms, not 1us, after RST

[Peter Hurley]

On 10/27/2015 02:35 PM, ND Linux CI Server wrote:
> Greetings,
> 
> This email is automatically generated by ND's Linux Patch Testing framework
> based on aiaiai. I have performed some automatic testing of a patch (series)
> you submitted to intel-wired-lan@lists.osuosl.org
> 
> The following contains output of any tests which failed to pass, and might be
> the result of developer error. The tests performed include but may not be
> limited to checkpatch.pl, bisection testing, compilation on a default kernel
> config, coccinelle scripts, cppcheck, and smatch.
> 
> If you have received this email in error, or believe that aiaiai has detected a
> false positive, please email Jacob Keller <jacob.e.keller@intel.com>.

False positive.

As long as the delay is at least 1ms (which is guaranteed), slightly longer
delays (relative to the existing reset delay of 100ms) are not harmful.

Use of usleep_range() would be unnecessary overkill for the purpose.

Regards,
Peter Hurley


> ---
> 
> I have tested your changes
> 
> [Intel-wired-lan] [PATCHv2] ixgbe: Wait for 1ms, not 1us, after RST
> 
> Project: net (net-current development queue)
> 
> Configurations: intel_defconfig,x86
> 
> Tested the patch(es) on top of the following commits:
> 505b857 ixgbe: Reset interface after enabling SR-IOV
> ce9d9b8 net: sysctl: fix a kmemleak warning
> 1acea4f ppp: fix pppoe_dev deletion condition in pppoe_release()
> f6b8dec9 af_key: fix two typos
> 
> --------------------------------------------------------------------------------
> 
> Successfully built configuration "intel_defconfig,x86", no issues.
> 
> --------------------------------------------------------------------------------
> 
> checkpatch.pl has some complaints:
> 
> --------------------------------------------------------------------------------
> 
> checkpatch.pl results for patch "[PATCH] ixgbe: Wait for 1ms, not 1us, after RST"
> 
> WARNING:MSLEEP: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> #29: FILE: drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c:119:
> +	msleep(1);
> 
> total: 0 errors, 1 warnings, 0 checks, 13 lines checked
> 
> --------------------------------------------------------------------------------

[Intel-wired-lan] [PATCHv2] ixgbe: Wait for 1ms, not 1us, after RST

[Keller, Jacob E]

On Tue, 2015-10-27 at 18:45 -0400, Peter Hurley wrote:
> On 10/27/2015 02:35 PM, ND Linux CI Server wrote:
> > Greetings,
> > 
> > This email is automatically generated by ND's Linux Patch Testing
> > framework
> > based on aiaiai. I have performed some automatic testing of a patch
> > (series)
> > you submitted to intel-wired-lan at lists.osuosl.org
> > 
> > The following contains output of any tests which failed to pass,
> > and might be
> > the result of developer error. The tests performed include but may
> > not be
> > limited to checkpatch.pl, bisection testing, compilation on a
> > default kernel
> > config, coccinelle scripts, cppcheck, and smatch.
> > 
> > If you have received this email in error, or believe that aiaiai
> > has detected a
> > false positive, please email Jacob Keller <jacob.e.keller@intel.com
> > >.
> 
> False positive.
> 
> As long as the delay is at least 1ms (which is guaranteed), slightly
> longer
> delays (relative to the existing reset delay of 100ms) are not
> harmful.
> 
> Use of usleep_range() would be unnecessary overkill for the purpose.
> 
> Regards,
> Peter Hurley


Feel free to ignore this then.

Regards,
Jake

Re: [Intel-wired-lan] [PATCHv2] ixgbe: Wait for 1ms, not 1us, after RST

[Keller, Jacob E]

On Tue, 2015-10-27 at 18:45 -0400, Peter Hurley wrote:
> On 10/27/2015 02:35 PM, ND Linux CI Server wrote:
> > Greetings,
> > 
> > This email is automatically generated by ND's Linux Patch Testing
> > framework
> > based on aiaiai. I have performed some automatic testing of a patch
> > (series)
> > you submitted to intel-wired-lan@lists.osuosl.org
> > 
> > The following contains output of any tests which failed to pass,
> > and might be
> > the result of developer error. The tests performed include but may
> > not be
> > limited to checkpatch.pl, bisection testing, compilation on a
> > default kernel
> > config, coccinelle scripts, cppcheck, and smatch.
> > 
> > If you have received this email in error, or believe that aiaiai
> > has detected a
> > false positive, please email Jacob Keller <jacob.e.keller@intel.com
> > >.
> 
> False positive.
> 
> As long as the delay is at least 1ms (which is guaranteed), slightly
> longer
> delays (relative to the existing reset delay of 100ms) are not
> harmful.
> 
> Use of usleep_range() would be unnecessary overkill for the purpose.
> 
> Regards,
> Peter Hurley


Feel free to ignore this then.

Regards,
Jake

Re: [Intel-wired-lan] [PATCHv2] ixgbe: Wait for 1ms, not 1us, after RST

[Keller, Jacob E]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1324 bytes --]

On Tue, 2015-10-27 at 18:45 -0400, Peter Hurley wrote:
> On 10/27/2015 02:35 PM, ND Linux CI Server wrote:
> > Greetings,
> > 
> > This email is automatically generated by ND's Linux Patch Testing
> > framework
> > based on aiaiai. I have performed some automatic testing of a patch
> > (series)
> > you submitted to intel-wired-lan@lists.osuosl.org
> > 
> > The following contains output of any tests which failed to pass,
> > and might be
> > the result of developer error. The tests performed include but may
> > not be
> > limited to checkpatch.pl, bisection testing, compilation on a
> > default kernel
> > config, coccinelle scripts, cppcheck, and smatch.
> > 
> > If you have received this email in error, or believe that aiaiai
> > has detected a
> > false positive, please email Jacob Keller <jacob.e.keller@intel.com
> > >.
> 
> False positive.
> 
> As long as the delay is at least 1ms (which is guaranteed), slightly
> longer
> delays (relative to the existing reset delay of 100ms) are not
> harmful.
> 
> Use of usleep_range() would be unnecessary overkill for the purpose.
> 
> Regards,
> Peter Hurley


Feel free to ignore this then.

Regards,
Jakeÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[Intel-wired-lan] [PATCH] ixgbe: Wait for 1ms, not 1us, after RST

[Peter Hurley]

Hi Dan,

On 10/26/2015 08:16 PM, dan.streetman at canonical.com wrote:
> From: Dan Streetman <dan.streetman@canonical.com>
> 
> The driver currently waits 1us after issuing a RST, but the spec
> requires it to wait 1ms.
> 
> Signed-off-by: Dan Streetman <dan.streetman@canonical.com>
> Signed-off-by: Dan Streetman <ddstreet@ieee.org>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
> index 4e75843..147bc65 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
> @@ -113,7 +113,12 @@ mac_reset_top:
>  
>  	/* Poll for reset bit to self-clear indicating reset is complete */
>  	for (i = 0; i < 10; i++) {
> -		udelay(1);
> +		/* sec 8.2.4.1.1 :
> +		 * programmers must wait approximately 1 ms after setting before
> +		 * attempting to check if the bit has cleared or to access (read
> +		 * or write) any other device register.
> +		 */
> +		mdelay(1);

Since ixgbe_reset_hw_x540() goes on to msleep(100) immediately after this
busy-wait loop, this should instead be:

		msleep(1);

Regards,
Peter Hurley


>  		ctrl = IXGBE_READ_REG(hw, IXGBE_CTRL);
>  		if (!(ctrl & IXGBE_CTRL_RST_MASK))
>  			break;
> 

[Intel-wired-lan] [PATCHv2] ixgbe: Wait for 1ms, not 1us, after RST

[Dan Streetman]

The driver currently waits 1us after issuing a RST, but the spec
requires it to wait 1ms.  This adds a msleep(1) before polling the
reset bit.

Signed-off-by: Dan Streetman <dan.streetman@canonical.com>
Signed-off-by: Dan Streetman <ddstreet@ieee.org>
---
changes since v1:
 use msleep(1) instead of mdelay(1), per Peter Hurley
 move msleep(1) out of for loop - only msleep once, leave udelay(1)
   inside for loop
 use spec sec title instead of number, per Don Skidmore

 drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
index 4e75843..02cfa1e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
@@ -111,6 +111,13 @@ mac_reset_top:
 	IXGBE_WRITE_REG(hw, IXGBE_CTRL, ctrl);
 	IXGBE_WRITE_FLUSH(hw);
 
+	/* From the spec "General Control Registers - Device Control Register":
+	 * "...programmers must wait approximately 1 ms after setting before
+	 *  attempting to check if the bit has cleared or to access (read
+	 *  or write) any other device register."
+	 */
+	msleep(1);
+
 	/* Poll for reset bit to self-clear indicating reset is complete */
 	for (i = 0; i < 10; i++) {
 		udelay(1);
-- 
2.5.0