[PATCH] Fix e1000e with Intel 82572EI that has no hardware timestamp support

[PATCH] Fix e1000e with Intel 82572EI that has no hardware timestamp support

[Koehrer Mathias (ETAS/ESW5)]

With the Intel 82527EI (driver: e1000e) there is an issue when running 
the ptpd2 program, that leads to a kernel oops.
The reason is here that in e1000_xmit_frame() a work queue will be 
scheduled that has not been initialized in this case.
The work queue "tx_hwstamp_work" will only be initialized if 
adapter->flags & FLAG_HAS_HW_TIMESTAMP set.
This check is missing in e1000_xmit_frame().

The following patch adds the missing check.

Signed-off-by: Mathias Koehrer <mathias.koehrer@etas.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-3.12.26/drivers/net/ethernet/intel/e1000e/netdev.c
===================================================================
--- linux-3.12.26.orig/drivers/net/ethernet/intel/e1000e/netdev.c	2014-08-04 10:56:56.000000000 +0200
+++ linux-3.12.26/drivers/net/ethernet/intel/e1000e/netdev.c	2014-08-06 16:22:10.000000000 +0200
@@ -5549,8 +5549,9 @@
 	count = e1000_tx_map(tx_ring, skb, first, adapter->tx_fifo_limit,
 			     nr_frags);
 	if (count) {
-		if (unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
-			     !adapter->tx_hwtstamp_skb)) {
+		if ((adapter->flags & FLAG_HAS_HW_TIMESTAMP) && 
+		    unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
+		    !adapter->tx_hwtstamp_skb)) {
 			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 			tx_flags |= E1000_TX_FLAGS_HWTSTAMP;
 			adapter->tx_hwtstamp_skb = skb_get(skb);

-- 

Re: [PATCH] Fix e1000e with Intel 82572EI that has no hardware timestamp support

[Richard Cochran]

On Wed, Aug 06, 2014 at 02:32:16PM +0000, Koehrer Mathias (ETAS/ESW5) wrote:
> With the Intel 82527EI (driver: e1000e) there is an issue when running 
> the ptpd2 program, that leads to a kernel oops.
> The reason is here that in e1000_xmit_frame() a work queue will be 
> scheduled that has not been initialized in this case.
> The work queue "tx_hwstamp_work" will only be initialized if 
> adapter->flags & FLAG_HAS_HW_TIMESTAMP set.
> This check is missing in e1000_xmit_frame().

Why all the ragged lines?  Better to use an editor that knows how to
justify a paragraph, more like this:

With the Intel 82527EI (driver: e1000e) there is an issue when running
the ptpd2 program, that leads to a kernel oops.  The reason is here
that in e1000_xmit_frame() a work queue will be scheduled that has not
been initialized in this case.  The work queue "tx_hwstamp_work" will
only be initialized if adapter->flags & FLAG_HAS_HW_TIMESTAMP set.
This check is missing in e1000_xmit_frame().

That not only looks nicer, but it is also more readable.

Thanks,
Richard

Re: [PATCH] Fix e1000e with Intel 82572EI that has no hardware timestamp support

[Richard Cochran]

On Wed, Aug 06, 2014 at 03:20:43PM -0400, Nick Krause wrote:
> My question was as follows after reading the thread here is this patch
> good or do I need to edit it
> some more based on the thread and only putting part of the if
> statement in an unlikely statement.

Huh? Are you Mathias Koehrer?

I was only complaining about the formatting of the change log 
comment. As far as the code goes, please see Alexander Duyck's
emails.

Thanks,
Richard

Re: [PATCH] Fix e1000e with Intel 82572EI that has no hardware timestamp support

[Richard Cochran]

On Wed, Aug 06, 2014 at 03:38:18PM -0400, Nick Krause wrote:
> I checked them and this patch seems fine to me.

No, it isn't fine. It ignores Alexander's comments.  Mathias, can you
please fix it?

Thanks,
Richard

[PATCH] Fix e1000e with Intel 82572EI that has no hardware timestamp support

[Koehrer Mathias (ETAS/ESW5)]

With the Intel 82527EI (driver: e1000e) there is an issue when running
the ptpd2 program, that leads to a kernel oops.  The reason is here that
in e1000_xmit_frame() a work queue will be scheduled that has not been
initialized in this case.  The work queue "tx_hwstamp_work" will only be
initialized if adapter->flags & FLAG_HAS_HW_TIMESTAMP set.  This check
is missing in e1000_xmit_frame().

The following patch adds the missing check.

Signed-off-by: Mathias Koehrer <mathias.koehrer@etas.com>

---
 drivers/net/ethernet/intel/e1000e/netdev.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-3.12.26/drivers/net/ethernet/intel/e1000e/netdev.c
===================================================================
--- linux-3.12.26.orig/drivers/net/ethernet/intel/e1000e/netdev.c	2014-08-04 10:56:56.000000000 +0200
+++ linux-3.12.26/drivers/net/ethernet/intel/e1000e/netdev.c	2014-08-07 08:05:17.000000000 +0200
@@ -5550,7 +5550,8 @@
 			     nr_frags);
 	if (count) {
 		if (unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
-			     !adapter->tx_hwtstamp_skb)) {
+			     !adapter->tx_hwtstamp_skb &&
+			     (adapter->flags & FLAG_HAS_HW_TIMESTAMP))) {
 			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 			tx_flags |= E1000_TX_FLAGS_HWTSTAMP;
 			adapter->tx_hwtstamp_skb = skb_get(skb);

-- 

Re: [PATCH] Fix e1000e with Intel 82572EI that has no hardware timestamp support

[Jeff Kirsher]

On Wed, Aug 6, 2014 at 11:10 PM, Koehrer Mathias (ETAS/ESW5)
<mathias.koehrer@etas.com> wrote:
> With the Intel 82527EI (driver: e1000e) there is an issue when running
> the ptpd2 program, that leads to a kernel oops.  The reason is here that
> in e1000_xmit_frame() a work queue will be scheduled that has not been
> initialized in this case.  The work queue "tx_hwstamp_work" will only be
> initialized if adapter->flags & FLAG_HAS_HW_TIMESTAMP set.  This check
> is missing in e1000_xmit_frame().
>
> The following patch adds the missing check.
>
> Signed-off-by: Mathias Koehrer <mathias.koehrer@etas.com>
>
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> Index: linux-3.12.26/drivers/net/ethernet/intel/e1000e/netdev.c
> ===================================================================
> --- linux-3.12.26.orig/drivers/net/ethernet/intel/e1000e/netdev.c       2014-08-04 10:56:56.000000000 +0200
> +++ linux-3.12.26/drivers/net/ethernet/intel/e1000e/netdev.c    2014-08-07 08:05:17.000000000 +0200
> @@ -5550,7 +5550,8 @@
>                              nr_frags);
>         if (count) {
>                 if (unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> -                            !adapter->tx_hwtstamp_skb)) {
> +                            !adapter->tx_hwtstamp_skb &&
> +                            (adapter->flags & FLAG_HAS_HW_TIMESTAMP))) {
>                         skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
>                         tx_flags |= E1000_TX_FLAGS_HWTSTAMP;
>                         adapter->tx_hwtstamp_skb = skb_get(skb);
>

So is this v2 of your patch?  and you are ignoring Alex's last suggestion to do:

  unlikely(skb->tx_flags & HW_TSTAMP) &&
  (adapter->flags & HAS_HW_TSTAMP) &&
  !adapter->tx_hwtstamp_skb

Just wondering....

-- 
Cheers,
Jeff

[PATCH] Fix e1000e with Intel 82572EI that has no hardware timestamp support

[Koehrer Mathias (ETAS/ESW5)]

With the Intel 82527EI (driver: e1000e) there is an issue when running 
the ptpd2 program, that leads to a kernel oops.
The reason is here that in e1000_xmit_frame() a work queue will be 
scheduled that has not been initialized in this case.
The work queue "tx_hwstamp_work" will only be initialized if 
adapter->flags & FLAG_HAS_HW_TIMESTAMP set.
This check is missing in e1000_xmit_frame().

The following patch adds the missing check.

---
 drivers/net/ethernet/intel/e1000e/netdev.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-3.12.26/drivers/net/ethernet/intel/e1000e/netdev.c
===================================================================
--- linux-3.12.26.orig/drivers/net/ethernet/intel/e1000e/netdev.c	2014-08-04 10:56:56.000000000 +0200
+++ linux-3.12.26/drivers/net/ethernet/intel/e1000e/netdev.c	2014-08-06 16:22:10.000000000 +0200
@@ -5549,8 +5549,9 @@
 	count = e1000_tx_map(tx_ring, skb, first, adapter->tx_fifo_limit,
 			     nr_frags);
 	if (count) {
-		if (unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
-			     !adapter->tx_hwtstamp_skb)) {
+		if ((adapter->flags & FLAG_HAS_HW_TIMESTAMP) && 
+		    unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
+		    !adapter->tx_hwtstamp_skb)) {
 			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 			tx_flags |= E1000_TX_FLAGS_HWTSTAMP;
 			adapter->tx_hwtstamp_skb = skb_get(skb);

-- 

Re: [PATCH] Fix e1000e with Intel 82572EI that has no hardware timestamp support

[Sergei Shtylyov]

On 8/6/2014 6:27 PM, Koehrer Mathias (ETAS/ESW5) wrote:

> With the Intel 82527EI (driver: e1000e) there is an issue when running
> the ptpd2 program, that leads to a kernel oops.
> The reason is here that in e1000_xmit_frame() a work queue will be
> scheduled that has not been initialized in this case.
> The work queue "tx_hwstamp_work" will only be initialized if
> adapter->flags & FLAG_HAS_HW_TIMESTAMP set.
> This check is missing in e1000_xmit_frame().

> The following patch adds the missing check.

    OK, but this time you forgot your sign-off. :-)

WBR, Sergei

Re: [PATCH] Fix e1000e with Intel 82572EI that has no hardware timestamp support

[Henrique de Moraes Holschuh]

On Wed, Aug 6, 2014, at 11:30, Sergei Shtylyov wrote:
> On 8/6/2014 6:27 PM, Koehrer Mathias (ETAS/ESW5) wrote:
> 
> > With the Intel 82527EI (driver: e1000e) there is an issue when running
> > the ptpd2 program, that leads to a kernel oops.
> > The reason is here that in e1000_xmit_frame() a work queue will be
> > scheduled that has not been initialized in this case.
> > The work queue "tx_hwstamp_work" will only be initialized if
> > adapter->flags & FLAG_HAS_HW_TIMESTAMP set.
> > This check is missing in e1000_xmit_frame().
> 
> > The following patch adds the missing check.
> 
>     OK, but this time you forgot your sign-off. :-)

Also, shouldn't the added test be inside the unlikely() ?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] Fix e1000e with Intel 82572EI that has no hardware timestamp support

[Alexander Duyck]

On 08/06/2014 08:09 AM, Henrique de Moraes Holschuh wrote:
> On Wed, Aug 6, 2014, at 11:30, Sergei Shtylyov wrote:
>> On 8/6/2014 6:27 PM, Koehrer Mathias (ETAS/ESW5) wrote:
>>
>>> With the Intel 82527EI (driver: e1000e) there is an issue when running
>>> the ptpd2 program, that leads to a kernel oops.
>>> The reason is here that in e1000_xmit_frame() a work queue will be
>>> scheduled that has not been initialized in this case.
>>> The work queue "tx_hwstamp_work" will only be initialized if
>>> adapter->flags & FLAG_HAS_HW_TIMESTAMP set.
>>> This check is missing in e1000_xmit_frame().
>>
>>> The following patch adds the missing check.
>>
>>     OK, but this time you forgot your sign-off. :-)
> 
> Also, shouldn't the added test be inside the unlikely() ?
> 

That shouldn't be necessary, but it might be better to place the new
check after the unlikely.  So you do the unlikely check first, then the
adapter check to see if we support Tx timestamping.  That way we can
avoid any unnecessary checks in the adapter structure.

Thanks,

Alex

Re: [PATCH] Fix e1000e with Intel 82572EI that has no hardware timestamp support

[Alexander Duyck]

On 08/06/2014 08:21 AM, Alexander Duyck wrote:
> On 08/06/2014 08:09 AM, Henrique de Moraes Holschuh wrote:
>> On Wed, Aug 6, 2014, at 11:30, Sergei Shtylyov wrote:
>>> On 8/6/2014 6:27 PM, Koehrer Mathias (ETAS/ESW5) wrote:
>>>
>>>> With the Intel 82527EI (driver: e1000e) there is an issue when running
>>>> the ptpd2 program, that leads to a kernel oops.
>>>> The reason is here that in e1000_xmit_frame() a work queue will be
>>>> scheduled that has not been initialized in this case.
>>>> The work queue "tx_hwstamp_work" will only be initialized if
>>>> adapter->flags & FLAG_HAS_HW_TIMESTAMP set.
>>>> This check is missing in e1000_xmit_frame().
>>>
>>>> The following patch adds the missing check.
>>>
>>>     OK, but this time you forgot your sign-off. :-)
>>
>> Also, shouldn't the added test be inside the unlikely() ?
>>
> 
> That shouldn't be necessary, but it might be better to place the new
> check after the unlikely.  So you do the unlikely check first, then the
> adapter check to see if we support Tx timestamping.  That way we can
> avoid any unnecessary checks in the adapter structure.
> 
> Thanks,
> 
> Alex
> 


Actually you might want to even take it one step further and break out
the test as follows (note this is only pseudo-code:
  unlikely(skb->tx_flags & HW_TSTAMP) &&
  (adapter->flags & HAS_HW_TSTAMP) &&
  !adapter->tx_hwtstamp_skb

There isn't much point in wrapping the entire statement in unlikely, by
wrapping just the first test the rest of the conditional checks can be
pushed into the branch which will likely require a jump to some remote
block of code and should streamline the hot-path.

Thanks,

Alex

Re: [PATCH] Fix e1000e with Intel 82572EI that has no hardware timestamp support

[Alexander Duyck]

On 08/06/2014 12:03 PM, Nick Krause wrote:
> On Wed, Aug 6, 2014 at 11:26 AM, Alexander Duyck
> <alexander.h.duyck@intel.com> wrote:
>> On 08/06/2014 08:21 AM, Alexander Duyck wrote:
>>> On 08/06/2014 08:09 AM, Henrique de Moraes Holschuh wrote:
>>>> On Wed, Aug 6, 2014, at 11:30, Sergei Shtylyov wrote:
>>>>> On 8/6/2014 6:27 PM, Koehrer Mathias (ETAS/ESW5) wrote:
>>>>>
>>>>>> With the Intel 82527EI (driver: e1000e) there is an issue when running
>>>>>> the ptpd2 program, that leads to a kernel oops.
>>>>>> The reason is here that in e1000_xmit_frame() a work queue will be
>>>>>> scheduled that has not been initialized in this case.
>>>>>> The work queue "tx_hwstamp_work" will only be initialized if
>>>>>> adapter->flags & FLAG_HAS_HW_TIMESTAMP set.
>>>>>> This check is missing in e1000_xmit_frame().
>>>>>
>>>>>> The following patch adds the missing check.
>>>>>
>>>>>     OK, but this time you forgot your sign-off. :-)
>>>>
>>>> Also, shouldn't the added test be inside the unlikely() ?
>>>>
>>>
>>> That shouldn't be necessary, but it might be better to place the new
>>> check after the unlikely.  So you do the unlikely check first, then the
>>> adapter check to see if we support Tx timestamping.  That way we can
>>> avoid any unnecessary checks in the adapter structure.
>>>
>>> Thanks,
>>>
>>> Alex
>>>
>>
>>
>> Actually you might want to even take it one step further and break out
>> the test as follows (note this is only pseudo-code:
>>   unlikely(skb->tx_flags & HW_TSTAMP) &&
>>   (adapter->flags & HAS_HW_TSTAMP) &&
>>   !adapter->tx_hwtstamp_skb
>>
>> There isn't much point in wrapping the entire statement in unlikely, by
>> wrapping just the first test the rest of the conditional checks can be
>> pushed into the branch which will likely require a jump to some remote
>> block of code and should streamline the hot-path.
>>
>> Thanks,
>>
>> Alex
> 
> Alex,
> Has this patch been resent or not. If not I can clean it up and sent
> as my own patch against linus's tree.
> Regards NIck
> 

Nick,

Please give Mathias a chance to complete his own patch.  Also this is
would be against Dave Miller's net tree, not Linus's tree.

Thanks,

Alex

[PATCH] Fix e1000e with Intel 82572EI that has no hardware timestamp support

[Koehrer Mathias (ETAS/ESW5)]

With the Intel 82527EI (driver: e1000e) there is an issue when running the ptpd2 program, that leads to a kernel oops.
The reason is here that in e1000_xmit_frame() a work queue will be scheduled that has not been initialized in this case.
The work queue "tx_hwstamp_work" will only be initialized if adapter->flags & FLAG_HAS_HW_TIMESTAMP set.
This check is missing in e1000_xmit_frame().

The following patch adds the missing check.

Signed-off-by: Mathias Koehrer <mathias.koehrer@etas.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-3.12.26/drivers/net/ethernet/intel/e1000e/netdev.c
===================================================================
--- linux-3.12.26.orig/drivers/net/ethernet/intel/e1000e/netdev.c       2014-08-04 10:56:56.000000000 +0200
+++ linux-3.12.26/drivers/net/ethernet/intel/e1000e/netdev.c    2014-08-06 15:15:42.000000000 +0200
@@ -5549,7 +5549,8 @@
        count = e1000_tx_map(tx_ring, skb, first, adapter->tx_fifo_limit,
                             nr_frags);
        if (count) {
-               if (unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
+               if ((adapter->flags & FLAG_HAS_HW_TIMESTAMP) &&
+                        unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
                             !adapter->tx_hwtstamp_skb)) {
                        skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
                        tx_flags |= E1000_TX_FLAGS_HWTSTAMP;


-- 

Re: [PATCH] Fix e1000e with Intel 82572EI that has no hardware timestamp support

[Sergei Shtylyov]

Hello.

On 8/6/2014 5:56 PM, Koehrer Mathias (ETAS/ESW5) wrote:

> With the Intel 82527EI (driver: e1000e) there is an issue when running the ptpd2 program, that leads to a kernel oops.
> The reason is here that in e1000_xmit_frame() a work queue will be scheduled that has not been initialized in this case.
> The work queue "tx_hwstamp_work" will only be initialized if adapter->flags & FLAG_HAS_HW_TIMESTAMP set.
> This check is missing in e1000_xmit_frame().

   Please wrap your changelog at 80 columns or less.

> The following patch adds the missing check.

> Signed-off-by: Mathias Koehrer <mathias.koehrer@etas.com>
> ---
>   drivers/net/ethernet/intel/e1000e/netdev.c |    3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

> Index: linux-3.12.26/drivers/net/ethernet/intel/e1000e/netdev.c
> ===================================================================
> --- linux-3.12.26.orig/drivers/net/ethernet/intel/e1000e/netdev.c       2014-08-04 10:56:56.000000000 +0200
> +++ linux-3.12.26/drivers/net/ethernet/intel/e1000e/netdev.c    2014-08-06 15:15:42.000000000 +0200
> @@ -5549,7 +5549,8 @@
>          count = e1000_tx_map(tx_ring, skb, first, adapter->tx_fifo_limit,
>                               nr_frags);
>          if (count) {
> -               if (unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&

    Your mailer has corrupted the patch -- all tabs were converted to spaces.

> +               if ((adapter->flags & FLAG_HAS_HW_TIMESTAMP) &&
> +                        unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&

    When you break the line with the *if* statement, continuation lines should 
start on a column right after ( above, according to the networking coding style.

WBR, Sergei