* [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
@ 2019-03-12 19:50 Paul Thomas
2019-03-12 20:05 ` Paul Thomas
2019-03-12 21:34 ` Keller, Jacob E
0 siblings, 2 replies; 12+ messages in thread
From: Paul Thomas @ 2019-03-12 19:50 UTC (permalink / raw)
To: netdev; +Cc: Paul Thomas
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)) {
+ 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
2019-03-12 19:50 [PATCH] Check for SKBTX_HW_TSTAMP in macb driver Paul Thomas
@ 2019-03-12 20:05 ` Paul Thomas
2019-03-12 21:37 ` Keller, Jacob E
2019-03-12 21:34 ` Keller, Jacob E
1 sibling, 1 reply; 12+ messages in thread
From: Paul Thomas @ 2019-03-12 20:05 UTC (permalink / raw)
To: netdev
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
2019-03-12 19:50 [PATCH] Check for SKBTX_HW_TSTAMP in macb driver Paul Thomas
2019-03-12 20:05 ` Paul Thomas
@ 2019-03-12 21:34 ` Keller, Jacob E
2019-03-12 21:39 ` Keller, Jacob E
1 sibling, 1 reply; 12+ messages in thread
From: Keller, Jacob E @ 2019-03-12 21:34 UTC (permalink / raw)
To: Paul Thomas, netdev
> -----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
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
2019-03-12 20:05 ` Paul Thomas
@ 2019-03-12 21:37 ` Keller, Jacob E
2019-03-13 5:59 ` Harini Katakam
0 siblings, 1 reply; 12+ messages in thread
From: Keller, Jacob E @ 2019-03-12 21:37 UTC (permalink / raw)
To: Paul Thomas, netdev
> -----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
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
2019-03-12 21:34 ` Keller, Jacob E
@ 2019-03-12 21:39 ` Keller, Jacob E
2019-03-12 22:05 ` Paul Thomas
0 siblings, 1 reply; 12+ messages in thread
From: Keller, Jacob E @ 2019-03-12 21:39 UTC (permalink / raw)
To: Keller, Jacob E, Paul Thomas, netdev
> -----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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
2019-03-12 21:39 ` Keller, Jacob E
@ 2019-03-12 22:05 ` Paul Thomas
2019-03-12 23:07 ` Keller, Jacob E
0 siblings, 1 reply; 12+ messages in thread
From: Paul Thomas @ 2019-03-12 22:05 UTC (permalink / raw)
To: Keller, Jacob E; +Cc: netdev
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
2019-03-12 22:05 ` Paul Thomas
@ 2019-03-12 23:07 ` Keller, Jacob E
2019-03-12 23:30 ` Paul Thomas
0 siblings, 1 reply; 12+ messages in thread
From: Keller, Jacob E @ 2019-03-12 23:07 UTC (permalink / raw)
To: Paul Thomas; +Cc: netdev
> -----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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
2019-03-12 23:07 ` Keller, Jacob E
@ 2019-03-12 23:30 ` Paul Thomas
2019-03-13 5:40 ` Harini Katakam
0 siblings, 1 reply; 12+ messages in thread
From: Paul Thomas @ 2019-03-12 23:30 UTC (permalink / raw)
To: Keller, Jacob E; +Cc: netdev
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
2019-03-12 23:30 ` Paul Thomas
@ 2019-03-13 5:40 ` Harini Katakam
2019-03-13 14:46 ` Keller, Jacob E
0 siblings, 1 reply; 12+ messages in thread
From: Harini Katakam @ 2019-03-13 5:40 UTC (permalink / raw)
To: Paul Thomas; +Cc: Keller, Jacob E, netdev
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
2019-03-12 21:37 ` Keller, Jacob E
@ 2019-03-13 5:59 ` Harini Katakam
2019-03-13 14:50 ` Keller, Jacob E
0 siblings, 1 reply; 12+ messages in thread
From: Harini Katakam @ 2019-03-13 5:59 UTC (permalink / raw)
To: Keller, Jacob E; +Cc: Paul Thomas, netdev
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
2019-03-13 5:40 ` Harini Katakam
@ 2019-03-13 14:46 ` Keller, Jacob E
0 siblings, 0 replies; 12+ messages in thread
From: Keller, Jacob E @ 2019-03-13 14:46 UTC (permalink / raw)
To: Harini Katakam, Paul Thomas; +Cc: netdev
> -----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
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH] Check for SKBTX_HW_TSTAMP in macb driver
2019-03-13 5:59 ` Harini Katakam
@ 2019-03-13 14:50 ` Keller, Jacob E
0 siblings, 0 replies; 12+ messages in thread
From: Keller, Jacob E @ 2019-03-13 14:50 UTC (permalink / raw)
To: Harini Katakam; +Cc: Paul Thomas, netdev
> -----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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-03-13 14:50 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-12 19:50 [PATCH] Check for SKBTX_HW_TSTAMP in macb driver Paul Thomas
2019-03-12 20:05 ` Paul Thomas
2019-03-12 21:37 ` Keller, Jacob E
2019-03-13 5:59 ` Harini Katakam
2019-03-13 14:50 ` Keller, Jacob E
2019-03-12 21:34 ` Keller, Jacob E
2019-03-12 21:39 ` Keller, Jacob E
2019-03-12 22:05 ` Paul Thomas
2019-03-12 23:07 ` Keller, Jacob E
2019-03-12 23:30 ` Paul Thomas
2019-03-13 5:40 ` Harini Katakam
2019-03-13 14:46 ` Keller, Jacob E
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.