Check for SKBTX_HW_TSTAMP in macb driver

Make sure SKBTX_HW_TSTAMP (i.e. SOF_TIMESTAMPING_TX_HARDWARE) has been enabled for this skb
This is a concept for discussion, more testing is needed.
It does fix the issue where normal socks that aren't expecting a timestamp will not wake
up on select.
---
 drivers/net/ethernet/cadence/macb_main.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

On Tue, Mar 12, 2019 at 3:51 PM Paul Thomas <pthomas8589@gmail.com> wrote:
>
> Make sure SKBTX_HW_TSTAMP (i.e. SOF_TIMESTAMPING_TX_HARDWARE) has been enabled for this skb
> This is a concept for discussion, more testing is needed.
> It does fix the issue where normal socks that aren't expecting a timestamp will not wake
> up on select.
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index ad099fd01b45..b2f184fc1370 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -898,11 +898,13 @@ static void macb_tx_interrupt(struct macb_queue *queue)
>
>                         /* First, update TX stats if needed */
>                         if (skb) {
> -                               if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {
> -                                       /* skb now belongs to timestamp buffer
> -                                        * and will be removed later
> -                                        */
I think the above does the same thing as if CONFIG_MACB_USE_HWSTAMP is
undefined regarding cleanup, so there is no extra cleanup if the
gem_ptp_do_txstamp() path isn't taken, but I wasn't sure about the
"skb now belongs to the timestamp buffer" if we don't go down that
path.
> -                                       tx_skb->skb = NULL;
> +                               if(unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
This looks like how other drivers are doing things
> +                                       if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {
> +                                               /* skb now belongs to timestamp buffer
> +                                                * and will be removed later
> +                                                */
> +                                               tx_skb->skb = NULL;
> +                                       }
>                                 }
>                                 netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n",
>                                             macb_tx_ring_wrap(bp, tail),
> --
> 2.17.1
>

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Paul Thomas
> Sent: Tuesday, March 12, 2019 12:51 PM
> To: netdev@vger.kernel.org
> Cc: Paul Thomas <pthomas8589@gmail.com>
> Subject: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
> 
> Make sure SKBTX_HW_TSTAMP (i.e. SOF_TIMESTAMPING_TX_HARDWARE) has been
> enabled for this skb
> This is a concept for discussion, more testing is needed.
> It does fix the issue where normal socks that aren't expecting a timestamp will not
> wake
> up on select.
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c
> b/drivers/net/ethernet/cadence/macb_main.c
> index ad099fd01b45..b2f184fc1370 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -898,11 +898,13 @@ static void macb_tx_interrupt(struct macb_queue *queue)
> 
>  			/* First, update TX stats if needed */
>  			if (skb) {
> -				if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {
> -					/* skb now belongs to timestamp buffer
> -					 * and will be removed later
> -					 */
> -					tx_skb->skb = NULL;
> +				if(unlikely(skb_shinfo(skb)->tx_flags &
> SKBTX_HW_TSTAMP)) {

This should be && for logical AND. Technically a bitwise & will likely work, but it's not what you intended.

Thanks,
Jake

> +					if (gem_ptp_do_txstamp(queue, skb, desc)
> == 0) {
> +						/* skb now belongs to timestamp
> buffer
> +						 * and will be removed later
> +						 */
> +						tx_skb->skb = NULL;
> +					}
>  				}
>  				netdev_vdbg(bp->dev, "skb %u (data %p) TX
> complete\n",
>  					    macb_tx_ring_wrap(bp, tail),
> --
> 2.17.1

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Paul Thomas
> Sent: Tuesday, March 12, 2019 1:05 PM
> To: netdev@vger.kernel.org
> Subject: Re: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
> 
> On Tue, Mar 12, 2019 at 3:51 PM Paul Thomas <pthomas8589@gmail.com> wrote:
> >
> > Make sure SKBTX_HW_TSTAMP (i.e. SOF_TIMESTAMPING_TX_HARDWARE) has
> been enabled for this skb
> > This is a concept for discussion, more testing is needed.
> > It does fix the issue where normal socks that aren't expecting a timestamp will not
> wake
> > up on select.
> > ---
> >  drivers/net/ethernet/cadence/macb_main.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c
> b/drivers/net/ethernet/cadence/macb_main.c
> > index ad099fd01b45..b2f184fc1370 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -898,11 +898,13 @@ static void macb_tx_interrupt(struct macb_queue
> *queue)
> >
> >                         /* First, update TX stats if needed */
> >                         if (skb) {
> > -                               if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {
> > -                                       /* skb now belongs to timestamp buffer
> > -                                        * and will be removed later
> > -                                        */
> I think the above does the same thing as if CONFIG_MACB_USE_HWSTAMP is
> undefined regarding cleanup, so there is no extra cleanup if the
> gem_ptp_do_txstamp() path isn't taken, but I wasn't sure about the
> "skb now belongs to the timestamp buffer" if we don't go down that
> path.

The path they're taking here seems a bit weird... I suspect the function gem_ptp_do_txstamp is doing something.

Normally the driver should use something like skb_get to obtain a reference to the skb.

However, the main goal of this patch is correct.

Thanks,
Jake

> > -                                       tx_skb->skb = NULL;
> > +                               if(unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
> This looks like how other drivers are doing things
> > +                                       if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {
> > +                                               /* skb now belongs to timestamp buffer
> > +                                                * and will be removed later
> > +                                                */
> > +                                               tx_skb->skb = NULL;
> > +                                       }
> >                                 }
> >                                 netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n",
> >                                             macb_tx_ring_wrap(bp, tail),
> > --
> > 2.17.1
> >

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Keller, Jacob E
> Sent: Tuesday, March 12, 2019 2:35 PM
> To: Paul Thomas <pthomas8589@gmail.com>; netdev@vger.kernel.org
> Subject: RE: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
> 
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> > Behalf Of Paul Thomas
> > Sent: Tuesday, March 12, 2019 12:51 PM
> > To: netdev@vger.kernel.org
> > Cc: Paul Thomas <pthomas8589@gmail.com>
> > Subject: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
> >
> > Make sure SKBTX_HW_TSTAMP (i.e. SOF_TIMESTAMPING_TX_HARDWARE) has
> been
> > enabled for this skb
> > This is a concept for discussion, more testing is needed.
> > It does fix the issue where normal socks that aren't expecting a timestamp will not
> > wake
> > up on select.
> > ---
> >  drivers/net/ethernet/cadence/macb_main.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c
> > b/drivers/net/ethernet/cadence/macb_main.c
> > index ad099fd01b45..b2f184fc1370 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -898,11 +898,13 @@ static void macb_tx_interrupt(struct macb_queue
> *queue)
> >
> >  			/* First, update TX stats if needed */
> >  			if (skb) {
> > -				if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {
> > -					/* skb now belongs to timestamp buffer
> > -					 * and will be removed later
> > -					 */
> > -					tx_skb->skb = NULL;
> > +				if(unlikely(skb_shinfo(skb)->tx_flags &
> > SKBTX_HW_TSTAMP)) {
> 
> This should be && for logical AND. Technically a bitwise & will likely work, but it's not
> what you intended.
> 
> Thanks,
> Jake
> 

Sorry, I misread the code.

You have two conditionals inside, and I misread where you were doing the checking of the SKBTX_HW_TSTAMP flag.

I would do the following :

if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HWTSTAMP) &&
    gem_ptp_do_txstamp(queue, skb, desc)) {
  ....
}

Rather than have two nested if statements, relying on the short circuiting to exit early.

The patch looks technically correct to me now that I'm reading it properly.

Jake

> > +					if (gem_ptp_do_txstamp(queue, skb, desc)
> > == 0) {
> > +						/* skb now belongs to timestamp
> > buffer
> > +						 * and will be removed later
> > +						 */
> > +						tx_skb->skb = NULL;
> > +					}
> >  				}
> >  				netdev_vdbg(bp->dev, "skb %u (data %p) TX
> > complete\n",
> >  					    macb_tx_ring_wrap(bp, tail),
> > --
> > 2.17.1

Hi Jake, thanks for all the help and for looking at this!

>
> You have two conditionals inside, and I misread where you were doing the checking of the SKBTX_HW_TSTAMP flag.
>
> I would do the following :
>
> if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HWTSTAMP) &&
>     gem_ptp_do_txstamp(queue, skb, desc)) {
I guess I was thinking the call to gem_ptp_do_txstamp() could be
avoided in many cases, but either way is fine with me..
>   ....
> }
>
-Paul

> -----Original Message-----
> From: Paul Thomas [mailto:pthomas8589@gmail.com]
> Sent: Tuesday, March 12, 2019 3:05 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
> 
> Hi Jake, thanks for all the help and for looking at this!
> 
> >
> > You have two conditionals inside, and I misread where you were doing the checking
> of the SKBTX_HW_TSTAMP flag.
> >
> > I would do the following :
> >
> > if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HWTSTAMP) &&
> >     gem_ptp_do_txstamp(queue, skb, desc)) {
> I guess I was thinking the call to gem_ptp_do_txstamp() could be
> avoided in many cases, but either way is fine with me..

The call will be avoided by virtue of short-curcuit boolean AND. If the first check is false, then the entire section is skipped without evaluating the second condition.

Thanks,
Jake

> >   ....
> > }
> >
> -Paul

On Tue, Mar 12, 2019 at 7:07 PM Keller, Jacob E
<jacob.e.keller@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Paul Thomas [mailto:pthomas8589@gmail.com]
> > Sent: Tuesday, March 12, 2019 3:05 PM
> > To: Keller, Jacob E <jacob.e.keller@intel.com>
> > Cc: netdev@vger.kernel.org
> > Subject: Re: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
> >
> > Hi Jake, thanks for all the help and for looking at this!
> >
> > >
> > > You have two conditionals inside, and I misread where you were doing the checking
> > of the SKBTX_HW_TSTAMP flag.
> > >
> > > I would do the following :
> > >
> > > if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HWTSTAMP) &&
> > >     gem_ptp_do_txstamp(queue, skb, desc)) {
> > I guess I was thinking the call to gem_ptp_do_txstamp() could be
> > avoided in many cases, but either way is fine with me..
>
> The call will be avoided by virtue of short-curcuit boolean AND. If the first check is false, then the entire section is skipped without evaluating the second condition.
Cool! good to know.

-Paul

> Thanks,
> Jake
>
> > >   ....
> > > }
> > >
> > -Paul

Hi Paul, Jake,
On Wed, Mar 13, 2019 at 5:01 AM Paul Thomas <pthomas8589@gmail.com> wrote:
>
> On Tue, Mar 12, 2019 at 7:07 PM Keller, Jacob E
> <jacob.e.keller@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Paul Thomas [mailto:pthomas8589@gmail.com]
> > > Sent: Tuesday, March 12, 2019 3:05 PM
> > > To: Keller, Jacob E <jacob.e.keller@intel.com>
> > > Cc: netdev@vger.kernel.org
> > > Subject: Re: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
> > >
> > > Hi Jake, thanks for all the help and for looking at this!
> > >
> > > >
> > > > You have two conditionals inside, and I misread where you were doing the checking
> > > of the SKBTX_HW_TSTAMP flag.
> > > >
> > > > I would do the following :
> > > >
> > > > if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HWTSTAMP) &&
> > > >     gem_ptp_do_txstamp(queue, skb, desc)) {

Thanks for looking into this. Shouldn't this be:
 if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HWTSTAMP) &&
      (gem_ptp_do_txstamp(queue, skb, desc) == 0)) {

Regards,
Harini

Hi Paul, Jake,
On Wed, Mar 13, 2019 at 3:08 AM Keller, Jacob E
<jacob.e.keller@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> > Behalf Of Paul Thomas
> > Sent: Tuesday, March 12, 2019 1:05 PM
> > To: netdev@vger.kernel.org
> > Subject: Re: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
> >
> > On Tue, Mar 12, 2019 at 3:51 PM Paul Thomas <pthomas8589@gmail.com> wrote:
> > >
<snip>
> > >                         /* First, update TX stats if needed */
> > >                         if (skb) {
> > > -                               if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {
> > > -                                       /* skb now belongs to timestamp buffer
> > > -                                        * and will be removed later
> > > -                                        */
> > I think the above does the same thing as if CONFIG_MACB_USE_HWSTAMP is
> > undefined regarding cleanup, so there is no extra cleanup if the
> > gem_ptp_do_txstamp() path isn't taken, but I wasn't sure about the
> > "skb now belongs to the timestamp buffer" if we don't go down that
> > path.
>
> The path they're taking here seems a bit weird... I suspect the function gem_ptp_do_txstamp is doing something.
>
> Normally the driver should use something like skb_get to obtain a reference to the skb.

Just FYI, this is the historical reason for the skb implementation here:
https://patchwork.kernel.org/patch/9473937/

Regards,
Harini

> -----Original Message-----
> From: Harini Katakam [mailto:harinik@xilinx.com]
> Sent: Tuesday, March 12, 2019 10:40 PM
> To: Paul Thomas <pthomas8589@gmail.com>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
> 
> Hi Paul, Jake,
> On Wed, Mar 13, 2019 at 5:01 AM Paul Thomas <pthomas8589@gmail.com> wrote:
> >
> > On Tue, Mar 12, 2019 at 7:07 PM Keller, Jacob E
> > <jacob.e.keller@intel.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Paul Thomas [mailto:pthomas8589@gmail.com]
> > > > Sent: Tuesday, March 12, 2019 3:05 PM
> > > > To: Keller, Jacob E <jacob.e.keller@intel.com>
> > > > Cc: netdev@vger.kernel.org
> > > > Subject: Re: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
> > > >
> > > > Hi Jake, thanks for all the help and for looking at this!
> > > >
> > > > >
> > > > > You have two conditionals inside, and I misread where you were doing the
> checking
> > > > of the SKBTX_HW_TSTAMP flag.
> > > > >
> > > > > I would do the following :
> > > > >
> > > > > if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HWTSTAMP) &&
> > > > >     gem_ptp_do_txstamp(queue, skb, desc)) {
> 
> Thanks for looking into this. Shouldn't this be:
>  if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HWTSTAMP) &&
>       (gem_ptp_do_txstamp(queue, skb, desc) == 0)) {
> 
> Regards,
> Harini

Yes, you're correct, because gem_ptp_do_txstamp doesn't use the "non-zero = error code" pattern.

Thanks,
Jake

> -----Original Message-----
> From: Harini Katakam [mailto:harinik@xilinx.com]
> Sent: Tuesday, March 12, 2019 11:00 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Paul Thomas <pthomas8589@gmail.com>; netdev@vger.kernel.org
> Subject: Re: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
> 
> Hi Paul, Jake,
> On Wed, Mar 13, 2019 at 3:08 AM Keller, Jacob E
> <jacob.e.keller@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> > > Behalf Of Paul Thomas
> > > Sent: Tuesday, March 12, 2019 1:05 PM
> > > To: netdev@vger.kernel.org
> > > Subject: Re: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
> > >
> > > On Tue, Mar 12, 2019 at 3:51 PM Paul Thomas <pthomas8589@gmail.com>
> wrote:
> > > >
> <snip>
> > > >                         /* First, update TX stats if needed */
> > > >                         if (skb) {
> > > > -                               if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {
> > > > -                                       /* skb now belongs to timestamp buffer
> > > > -                                        * and will be removed later
> > > > -                                        */
> > > I think the above does the same thing as if CONFIG_MACB_USE_HWSTAMP is
> > > undefined regarding cleanup, so there is no extra cleanup if the
> > > gem_ptp_do_txstamp() path isn't taken, but I wasn't sure about the
> > > "skb now belongs to the timestamp buffer" if we don't go down that
> > > path.
> >
> > The path they're taking here seems a bit weird... I suspect the function
> gem_ptp_do_txstamp is doing something.
> >
> > Normally the driver should use something like skb_get to obtain a reference to the
> skb.
> 
> Just FYI, this is the historical reason for the skb implementation here:
> https://patchwork.kernel.org/patch/9473937/
> 
> Regards,
> Harini

I'm not sure I follow that logic, but I obviously haven't dug into what gem_ptp_do_txstamp does, or what calls its making.

Either way, I think this patch is a strict improvement on what was previously happening. It can be someone else's responsibility to change how the skb is handled if they find an issue with it.

Thanks,
Jake