lkml.org 
[lkml]   [2019]   [May]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 1/3] enetc: add hardware timestamping support
Date
Hi,

> -----Original Message-----
> From: Claudiu Manoil
> Sent: Thursday, May 16, 2019 11:31 PM
> To: Richard Cochran <richardcochran@gmail.com>; Y.b. Lu
> <yangbo.lu@nxp.com>
> Cc: netdev@vger.kernel.org; David Miller <davem@davemloft.net>; Shawn
> Guo <shawnguo@kernel.org>; Rob Herring <robh+dt@kernel.org>;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org
> Subject: RE: [PATCH 1/3] enetc: add hardware timestamping support
>
>
> >-----Original Message-----
> >From: Richard Cochran <richardcochran@gmail.com>
> >Sent: Thursday, May 16, 2019 5:33 PM
> >To: Y.b. Lu <yangbo.lu@nxp.com>
> >Cc: netdev@vger.kernel.org; David Miller <davem@davemloft.net>; Claudiu
> >Manoil <claudiu.manoil@nxp.com>; Shawn Guo <shawnguo@kernel.org>;
> Rob
> >Herring <robh+dt@kernel.org>; devicetree@vger.kernel.org; linux-arm-
> >kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> >Subject: Re: [PATCH 1/3] enetc: add hardware timestamping support
> >
> >On Thu, May 16, 2019 at 09:59:08AM +0000, Y.b. Lu wrote:
> >
> [...]
> >
> >> static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int
> >> napi_budget) {
> >> struct net_device *ndev = tx_ring->ndev;
> >> + struct enetc_ndev_priv *priv = netdev_priv(ndev);
> >> int tx_frm_cnt = 0, tx_byte_cnt = 0;
> >> struct enetc_tx_swbd *tx_swbd;
> >> + union enetc_tx_bd *txbd;
> >> + bool do_tstamp;
> >> int i, bds_to_clean;
> >> + u64 tstamp = 0;
> >
> >Please keep in reverse Christmas tree order as much as possible:
>
> For the xmass tree part, Yangbo, better move the priv and txbd declarations
> inside the scope of the if() {} block where they are actually used, i.e.:
>
> if (unlikely(tx_swbd->check_wb)) {
> struct enetc_ndev_priv *priv = netdev_priv(ndev);
> union enetc_tx_bd *txbd;
> [...]
> }
>

[Y.b. Lu] Will do that.

> >
> > union enetc_tx_bd *txbd;
> > int i, bds_to_clean;
> > bool do_tstamp;
> > u64 tstamp = 0;
> >
> >> i = tx_ring->next_to_clean;
> >> tx_swbd = &tx_ring->tx_swbd[i];
> >> bds_to_clean = enetc_bd_ready_count(tx_ring, i);
> >>
> >> + do_tstamp = false;
> >> +
> >> while (bds_to_clean && tx_frm_cnt < ENETC_DEFAULT_TX_WORK) {
> >> bool is_eof = !!tx_swbd->skb;
> >>
> >> + if (unlikely(tx_swbd->check_wb)) {
> >> + txbd = ENETC_TXBD(*tx_ring, i);
> >> +
> >> + if (!(txbd->flags & ENETC_TXBD_FLAGS_W))
> >> + goto no_wb;
> >> +
> >> + if (tx_swbd->do_tstamp) {
> >> + enetc_get_tx_tstamp(&priv->si->hw, txbd,
> >> + &tstamp);
> >> + do_tstamp = true;
> >> + }
> >> + }
> >> +no_wb:
> >
> >This goto seems strange and unnecessary. How about this instead?
> >
> > if (txbd->flags & ENETC_TXBD_FLAGS_W &&
> > tx_swbd->do_tstamp) {
> > enetc_get_tx_tstamp(&priv->si->hw, txbd, &tstamp);
> > do_tstamp = true;
> > }
> >
>
> Absolutely, somehow I missed this. I guess the intention was to be able to
> support multiple
> if() blocks for the writeback case (W flag set) but the code is much better off
> without the goto.

[Y.b. Lu] Will use this to support current single tstamp writeback case.

>
> >> enetc_unmap_tx_buff(tx_ring, tx_swbd);
> >> if (is_eof) {
> >> + if (unlikely(do_tstamp)) {
> >> + enetc_tstamp_tx(tx_swbd->skb, tstamp);
> >> + do_tstamp = false;
> >> + }
> >> napi_consume_skb(tx_swbd->skb, napi_budget);
> >> tx_swbd->skb = NULL;
> >> }
> >> @@ -167,6 +169,11 @@ struct enetc_cls_rule {
> >>
> >> #define ENETC_MAX_BDR_INT 2 /* fixed to max # of available cpus */
> >>
> >> +enum enetc_hw_features {
> >
> >This is a poor choice of name. It sounds like it describes HW
> >capabilities, but you use it to track whether a feature is requested at
> >run time.
> >
> >> + ENETC_F_RX_TSTAMP = BIT(0),
> >> + ENETC_F_TX_TSTAMP = BIT(1),
> >> +};
> >> +
> >> struct enetc_ndev_priv {
> >> struct net_device *ndev;
> >> struct device *dev; /* dma-mapping device */ @@ -178,6 +185,7 @@
> >> struct enetc_ndev_priv {
> >> u16 rx_bd_count, tx_bd_count;
> >>
> >> u16 msg_enable;
> >> + int hw_features;
> >
> >This is also poorly named. How about "tstamp_request" instead?
> >
>
> This ndev_priv variable was intended to gather flags for all the active h/w
> related features, i.e. keeping count of what h/w offloads are enabled for the
> current device (at least for those that don't have already a netdev_features_t
> flag).
> I wouldn't waste an int for 2 timestamp flags, I'd rather have a more generic
> name.
> Maybe active_offloads then?
>
> Anyway, the name can be changed later too, when other offloads will be
> added.

[Y.b. Lu] How about using active_offloads, and add TODO comments in enum enetc_active_offloads?

>
> Thanks,
> Claudiu

\
 
 \ /
  Last update: 2019-05-20 05:27    [W:0.056 / U:1.936 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site