lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 20 May 2019 03:25:45 +0000 From: "Y.b. Lu" <yangbo.lu@....com> To: Claudiu Manoil <claudiu.manoil@....com>, Richard Cochran <richardcochran@...il.com> CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, David Miller <davem@...emloft.net>, Shawn Guo <shawnguo@...nel.org>, Rob Herring <robh+dt@...nel.org>, "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org> Subject: RE: [PATCH 1/3] enetc: add hardware timestamping support Hi, > -----Original Message----- > From: Claudiu Manoil > Sent: Thursday, May 16, 2019 11:31 PM > To: Richard Cochran <richardcochran@...il.com>; Y.b. Lu > <yangbo.lu@....com> > Cc: netdev@...r.kernel.org; David Miller <davem@...emloft.net>; Shawn > Guo <shawnguo@...nel.org>; Rob Herring <robh+dt@...nel.org>; > devicetree@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; > linux-kernel@...r.kernel.org > Subject: RE: [PATCH 1/3] enetc: add hardware timestamping support > > > >-----Original Message----- > >From: Richard Cochran <richardcochran@...il.com> > >Sent: Thursday, May 16, 2019 5:33 PM > >To: Y.b. Lu <yangbo.lu@....com> > >Cc: netdev@...r.kernel.org; David Miller <davem@...emloft.net>; Claudiu > >Manoil <claudiu.manoil@....com>; Shawn Guo <shawnguo@...nel.org>; > Rob > >Herring <robh+dt@...nel.org>; devicetree@...r.kernel.org; linux-arm- > >kernel@...ts.infradead.org; linux-kernel@...r.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
Powered by blists - more mailing lists