>-----Original Message-----
>From: Richard Cochran <richardcochran@xxxxxxxxx>
>Sent: Thursday, May 16, 2019 5:33 PM
>To: Y.b. Lu <yangbo.lu@xxxxxxx>
>Cc: netdev@xxxxxxxxxxxxxxx; David Miller <davem@xxxxxxxxxxxxx>; Claudiu
>Manoil <claudiu.manoil@xxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Rob
>Herring <robh+dt@xxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux-arm-
>kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>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;
[...]
}
>
> 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.
>> 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.
Thanks,
Claudiu
