Message ID | 20190312195053.21741-1-pthomas8589@gmail.com |
---|---|
State | RFC |
Delegated to: | David Miller |
Headers | show |
Series | Check for SKBTX_HW_TSTAMP in macb driver | expand |
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
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),