Re: [PATCH net-next v5 2/2] net: thunderx: add timestamping support

2018-01-08 Thread Aleksey Makarov 


On 12.12.2017 05:32, Richard Cochran wrote:
> On Mon, Dec 11, 2017 at 05:14:31PM +0300, Aleksey Makarov wrote:
>> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h 
>> b/drivers/net/ethernet/cavium/thunder/nic.h
>> index 4a02e618e318..204b234beb9d 100644
>> --- a/drivers/net/ethernet/cavium/thunder/nic.h
>> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
>> @@ -263,6 +263,8 @@ struct nicvf_drv_stats {
>>  struct u64_stats_sync   syncp;
>>  };
>>  
>> +struct cavium_ptp;
>> +
>>  struct nicvf {
>>  struct nicvf*pnicvf;
>>  struct net_device   *netdev;
>> @@ -312,6 +314,12 @@ struct nicvf {
>>  struct tasklet_struct   qs_err_task;
>>  struct work_struct  reset_task;
>>  
>> +/* PTP timestamp */
>> +struct cavium_ptp   *ptp_clock;
>> +boolhw_rx_tstamp;
>> +struct sk_buff  *ptp_skb;
>> +atomic_ttx_ptp_skbs;
> 
> It is disturbing that the above two fields are set in different
> places.  Shouldn't they be unified into one logical lock?

No, they should not as they are not quite related.

`tx_ptp_skbs` is set when the hardware is sending a packet that requires
timestamping.  Cavium hardware can not process more than one
such packet at once so this is set each time the driver submits
a packet that requires timestamping to the send queue and clears
each time it receives the entry on the completion queue saying
that such packet was sent.

So `tx_ptp_skbs` prevents driver from submitting more than one
packet that requires timestamping to the hardware for transmitting.

When that packet is sent, hardware inserts two entries to
the completion queue.  First is the regular CQE_TYPE_SEND entry
that signals that the packet was sent.  The second is CQE_TYPE_SEND_PTP
that contains the actual timestamp for that packet.

`ptp_skb` is initialized in the handler for the CQE_TYPE_SEND
entry and is used and zeroed in the handler for the CQE_TYPE_SEND_PTP
entry.

So `ptp_skb` is used to hold the pointer to the packet between
the calls to CQE_TYPE_SEND and CQE_TYPE_SEND_PTP handlers.

I am going to add those comments to the sources, fix other issues and
send v6 in short time.

Thank you
Aleksey Makarov

> Here you clear them together:
> 
>> +static void nicvf_snd_ptp_handler(struct net_device *netdev,
>> +  struct cqe_send_t *cqe_tx)
>> +{
>> +struct nicvf *nic = netdev_priv(netdev);
>> +struct skb_shared_hwtstamps ts;
>> +u64 ns;
>> +
>> +nic = nic->pnicvf;
>> +
>> +/* Sync for 'ptp_skb' */
>> +smp_rmb();
>> +
>> +/* New timestamp request can be queued now */
>> +atomic_set(>tx_ptp_skbs, 0);
>> +
>> +/* Check for timestamp requested skb */
>> +if (!nic->ptp_skb)
>> +return;
>> +
>> +/* Check if timestamping is timedout, which is set to 10us */
>> +if (cqe_tx->send_status == CQ_TX_ERROP_TSTMP_TIMEOUT ||
>> +cqe_tx->send_status == CQ_TX_ERROP_TSTMP_CONFLICT)
>> +goto no_tstamp;
>> +
>> +/* Get the timestamp */
>> +memset(, 0, sizeof(ts));
>> +ns = cavium_ptp_tstamp2time(nic->ptp_clock, cqe_tx->ptp_timestamp);
>> +ts.hwtstamp = ns_to_ktime(ns);
>> +skb_tstamp_tx(nic->ptp_skb, );
>> +
>> +no_tstamp:
>> +/* Free the original skb */
>> +dev_kfree_skb_any(nic->ptp_skb);
>> +nic->ptp_skb = NULL;
>> +/* Sync 'ptp_skb' */
>> +smp_wmb();
>> +}
>> +
> 
> but here you set the one:
> 
>> @@ -657,7 +697,12 @@ static void nicvf_snd_pkt_handler(struct net_device 
>> *netdev,
>>  prefetch(skb);
>>  (*tx_pkts)++;
>>  *tx_bytes += skb->len;
>> -napi_consume_skb(skb, budget);
>> +/* If timestamp is requested for this skb, don't free it */
>> +if (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS &&
>> +!nic->pnicvf->ptp_skb)
>> +nic->pnicvf->ptp_skb = skb;
>> +else
>> +napi_consume_skb(skb, budget);
>>  sq->skbuff[cqe_tx->sqe_ptr] = (u64)NULL;
>>  } else {
>>  /* In case of SW TSO on 88xx, only last segment will have
> 
> here you clear one:
> 
>> @@ -1319,12 +1382,28 @@ int nicvf_stop(struct net_device *netdev)
>>  
>>  nicvf_free_cq_poll(nic);
>>  
>> +/* Free any pending SKB saved to receive timestamp */
>> +if (nic->ptp_skb) {
>> +dev_kfree_skb_any(nic->ptp_skb);
>> +nic->ptp_skb = NULL;
>> +}
>> +
>>  /* Clear multiqset info */
>>  nic->pnicvf = nic;
>>  
>>  return 0;
>>  }
> 
> here you clear both:
> 
>> @@ -1394,6 +1473,12 @@ int nicvf_open(struct net_device *netdev)
>>  if (nic->sqs_mode)
>>  nicvf_get_primary_vf_struct(nic);
>>  
>> +/* Configure PTP timestamp */
>> +if (nic->ptp_clock)
>> +nicvf_config_hw_rx_tstamp(nic, nic->hw_rx_tstamp);
>> +atomic_set(>tx_ptp_skbs, 0);
>> +nic->ptp_skb = NULL;
>> +
>>  /* Configure

Re: [PATCH net-next v5 2/2] net: thunderx: add timestamping support

2018-01-08 Thread Aleksey Makarov 


On 12.12.2017 05:32, Richard Cochran wrote:
> On Mon, Dec 11, 2017 at 05:14:31PM +0300, Aleksey Makarov wrote:
>> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h 
>> b/drivers/net/ethernet/cavium/thunder/nic.h
>> index 4a02e618e318..204b234beb9d 100644
>> --- a/drivers/net/ethernet/cavium/thunder/nic.h
>> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
>> @@ -263,6 +263,8 @@ struct nicvf_drv_stats {
>>  struct u64_stats_sync   syncp;
>>  };
>>  
>> +struct cavium_ptp;
>> +
>>  struct nicvf {
>>  struct nicvf*pnicvf;
>>  struct net_device   *netdev;
>> @@ -312,6 +314,12 @@ struct nicvf {
>>  struct tasklet_struct   qs_err_task;
>>  struct work_struct  reset_task;
>>  
>> +/* PTP timestamp */
>> +struct cavium_ptp   *ptp_clock;
>> +boolhw_rx_tstamp;
>> +struct sk_buff  *ptp_skb;
>> +atomic_ttx_ptp_skbs;
> 
> It is disturbing that the above two fields are set in different
> places.  Shouldn't they be unified into one logical lock?

No, they should not as they are not quite related.

`tx_ptp_skbs` is set when the hardware is sending a packet that requires
timestamping.  Cavium hardware can not process more than one
such packet at once so this is set each time the driver submits
a packet that requires timestamping to the send queue and clears
each time it receives the entry on the completion queue saying
that such packet was sent.

So `tx_ptp_skbs` prevents driver from submitting more than one
packet that requires timestamping to the hardware for transmitting.

When that packet is sent, hardware inserts two entries to
the completion queue.  First is the regular CQE_TYPE_SEND entry
that signals that the packet was sent.  The second is CQE_TYPE_SEND_PTP
that contains the actual timestamp for that packet.

`ptp_skb` is initialized in the handler for the CQE_TYPE_SEND
entry and is used and zeroed in the handler for the CQE_TYPE_SEND_PTP
entry.

So `ptp_skb` is used to hold the pointer to the packet between
the calls to CQE_TYPE_SEND and CQE_TYPE_SEND_PTP handlers.

I am going to add those comments to the sources, fix other issues and
send v6 in short time.

Thank you
Aleksey Makarov

> Here you clear them together:
> 
>> +static void nicvf_snd_ptp_handler(struct net_device *netdev,
>> +  struct cqe_send_t *cqe_tx)
>> +{
>> +struct nicvf *nic = netdev_priv(netdev);
>> +struct skb_shared_hwtstamps ts;
>> +u64 ns;
>> +
>> +nic = nic->pnicvf;
>> +
>> +/* Sync for 'ptp_skb' */
>> +smp_rmb();
>> +
>> +/* New timestamp request can be queued now */
>> +atomic_set(>tx_ptp_skbs, 0);
>> +
>> +/* Check for timestamp requested skb */
>> +if (!nic->ptp_skb)
>> +return;
>> +
>> +/* Check if timestamping is timedout, which is set to 10us */
>> +if (cqe_tx->send_status == CQ_TX_ERROP_TSTMP_TIMEOUT ||
>> +cqe_tx->send_status == CQ_TX_ERROP_TSTMP_CONFLICT)
>> +goto no_tstamp;
>> +
>> +/* Get the timestamp */
>> +memset(, 0, sizeof(ts));
>> +ns = cavium_ptp_tstamp2time(nic->ptp_clock, cqe_tx->ptp_timestamp);
>> +ts.hwtstamp = ns_to_ktime(ns);
>> +skb_tstamp_tx(nic->ptp_skb, );
>> +
>> +no_tstamp:
>> +/* Free the original skb */
>> +dev_kfree_skb_any(nic->ptp_skb);
>> +nic->ptp_skb = NULL;
>> +/* Sync 'ptp_skb' */
>> +smp_wmb();
>> +}
>> +
> 
> but here you set the one:
> 
>> @@ -657,7 +697,12 @@ static void nicvf_snd_pkt_handler(struct net_device 
>> *netdev,
>>  prefetch(skb);
>>  (*tx_pkts)++;
>>  *tx_bytes += skb->len;
>> -napi_consume_skb(skb, budget);
>> +/* If timestamp is requested for this skb, don't free it */
>> +if (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS &&
>> +!nic->pnicvf->ptp_skb)
>> +nic->pnicvf->ptp_skb = skb;
>> +else
>> +napi_consume_skb(skb, budget);
>>  sq->skbuff[cqe_tx->sqe_ptr] = (u64)NULL;
>>  } else {
>>  /* In case of SW TSO on 88xx, only last segment will have
> 
> here you clear one:
> 
>> @@ -1319,12 +1382,28 @@ int nicvf_stop(struct net_device *netdev)
>>  
>>  nicvf_free_cq_poll(nic);
>>  
>> +/* Free any pending SKB saved to receive timestamp */
>> +if (nic->ptp_skb) {
>> +dev_kfree_skb_any(nic->ptp_skb);
>> +nic->ptp_skb = NULL;
>> +}
>> +
>>  /* Clear multiqset info */
>>  nic->pnicvf = nic;
>>  
>>  return 0;
>>  }
> 
> here you clear both:
> 
>> @@ -1394,6 +1473,12 @@ int nicvf_open(struct net_device *netdev)
>>  if (nic->sqs_mode)
>>  nicvf_get_primary_vf_struct(nic);
>>  
>> +/* Configure PTP timestamp */
>> +if (nic->ptp_clock)
>> +nicvf_config_hw_rx_tstamp(nic, nic->hw_rx_tstamp);
>> +atomic_set(>tx_ptp_skbs, 0);
>> +nic->ptp_skb = NULL;
>> +
>>  /* Configure

Re: [PATCH net-next v5 2/2] net: thunderx: add timestamping support

2017-12-12 Thread Joe Perches 
On Mon, 2017-12-11 at 15:36 -0800, Richard Cochran wrote:
> On Mon, Dec 11, 2017 at 05:14:31PM +0300, Aleksey Makarov wrote:
> > @@ -880,6 +889,46 @@ static void nic_pause_frame(struct nicpf *nic, int vf, 
> > struct pfc *cfg)
> > }
> >  }
> >  
> > +/* Enable or disable HW timestamping by BGX for pkts received on a LMAC */
> > +static void nic_config_timestamp(struct nicpf *nic, int vf, struct set_ptp 
> > *ptp)
> > +{
> > +   struct pkind_cfg *pkind;
> > +   u8 lmac, bgx_idx;
> > +   u64 pkind_val, pkind_idx;
> > +
> > +   if (vf >= nic->num_vf_en)
> > +   return;
> > +
> > +   bgx_idx = NIC_GET_BGX_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
> > +   lmac = NIC_GET_LMAC_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
> > +
> > +   pkind_idx = lmac + bgx_idx * MAX_LMAC_PER_BGX;
> > +   pkind_val = nic_reg_read(nic, NIC_PF_PKIND_0_15_CFG | (pkind_idx << 3));
> > +   pkind = (struct pkind_cfg *)_val;
> > +
> > +   if (ptp->enable && !pkind->hdr_sl) {
> > +   /* Skiplen to exclude 8byte timestamp while parsing pkt
> > +* If not configured, will result in L2 errors.
> > +*/
> > +   pkind->hdr_sl = 4;
> > +   /* Adjust max packet length allowed */
> > +   pkind->maxlen += (pkind->hdr_sl * 2);

Are all compilers smart enough to set this to 8?
I rather doubt a compiler is even allowed to.

Re: [PATCH net-next v5 2/2] net: thunderx: add timestamping support

2017-12-12 Thread Joe Perches 
On Mon, 2017-12-11 at 15:36 -0800, Richard Cochran wrote:
> On Mon, Dec 11, 2017 at 05:14:31PM +0300, Aleksey Makarov wrote:
> > @@ -880,6 +889,46 @@ static void nic_pause_frame(struct nicpf *nic, int vf, 
> > struct pfc *cfg)
> > }
> >  }
> >  
> > +/* Enable or disable HW timestamping by BGX for pkts received on a LMAC */
> > +static void nic_config_timestamp(struct nicpf *nic, int vf, struct set_ptp 
> > *ptp)
> > +{
> > +   struct pkind_cfg *pkind;
> > +   u8 lmac, bgx_idx;
> > +   u64 pkind_val, pkind_idx;
> > +
> > +   if (vf >= nic->num_vf_en)
> > +   return;
> > +
> > +   bgx_idx = NIC_GET_BGX_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
> > +   lmac = NIC_GET_LMAC_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
> > +
> > +   pkind_idx = lmac + bgx_idx * MAX_LMAC_PER_BGX;
> > +   pkind_val = nic_reg_read(nic, NIC_PF_PKIND_0_15_CFG | (pkind_idx << 3));
> > +   pkind = (struct pkind_cfg *)_val;
> > +
> > +   if (ptp->enable && !pkind->hdr_sl) {
> > +   /* Skiplen to exclude 8byte timestamp while parsing pkt
> > +* If not configured, will result in L2 errors.
> > +*/
> > +   pkind->hdr_sl = 4;
> > +   /* Adjust max packet length allowed */
> > +   pkind->maxlen += (pkind->hdr_sl * 2);

Are all compilers smart enough to set this to 8?
I rather doubt a compiler is even allowed to.

Re: [PATCH net-next v5 2/2] net: thunderx: add timestamping support

2017-12-11 Thread Richard Cochran 
On Mon, Dec 11, 2017 at 05:14:31PM +0300, Aleksey Makarov wrote:
> @@ -880,6 +889,46 @@ static void nic_pause_frame(struct nicpf *nic, int vf, 
> struct pfc *cfg)
>   }
>  }
>  
> +/* Enable or disable HW timestamping by BGX for pkts received on a LMAC */
> +static void nic_config_timestamp(struct nicpf *nic, int vf, struct set_ptp 
> *ptp)
> +{
> + struct pkind_cfg *pkind;
> + u8 lmac, bgx_idx;
> + u64 pkind_val, pkind_idx;
> +
> + if (vf >= nic->num_vf_en)
> + return;
> +
> + bgx_idx = NIC_GET_BGX_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
> + lmac = NIC_GET_LMAC_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
> +
> + pkind_idx = lmac + bgx_idx * MAX_LMAC_PER_BGX;
> + pkind_val = nic_reg_read(nic, NIC_PF_PKIND_0_15_CFG | (pkind_idx << 3));
> + pkind = (struct pkind_cfg *)_val;
> +
> + if (ptp->enable && !pkind->hdr_sl) {
> + /* Skiplen to exclude 8byte timestamp while parsing pkt
> +  * If not configured, will result in L2 errors.
> +  */
> + pkind->hdr_sl = 4;
> + /* Adjust max packet length allowed */
> + pkind->maxlen += (pkind->hdr_sl * 2);
> + bgx_config_timestamping(nic->node, bgx_idx, lmac, true);
> + nic_reg_write(nic,
> +   NIC_PF_RX_ETYPE_0_7 | (1 << 3),
> +   (ETYPE_ALG_ENDPARSE << 16) | ETH_P_1588);

don't need three lines for this function call.

> + } else if (!ptp->enable && pkind->hdr_sl) {
> + pkind->maxlen -= (pkind->hdr_sl * 2);
> + pkind->hdr_sl = 0;
> + bgx_config_timestamping(nic->node, bgx_idx, lmac, false);
> + nic_reg_write(nic,
> +   NIC_PF_RX_ETYPE_0_7 | (1 << 3),
> +   (1ULL << 16) | ETH_P_8021Q); /* reset value */

here neither.  Also avoid comment on the LHS.  If 1<<16 means "reset"
then just define a macro.

> + }
> +
> + nic_reg_write(nic, NIC_PF_PKIND_0_15_CFG | (pkind_idx << 3), pkind_val);
> +}
> +

Thanks,
Richard

Re: [PATCH net-next v5 2/2] net: thunderx: add timestamping support

2017-12-11 Thread Richard Cochran 
On Mon, Dec 11, 2017 at 05:14:31PM +0300, Aleksey Makarov wrote:
> @@ -880,6 +889,46 @@ static void nic_pause_frame(struct nicpf *nic, int vf, 
> struct pfc *cfg)
>   }
>  }
>  
> +/* Enable or disable HW timestamping by BGX for pkts received on a LMAC */
> +static void nic_config_timestamp(struct nicpf *nic, int vf, struct set_ptp 
> *ptp)
> +{
> + struct pkind_cfg *pkind;
> + u8 lmac, bgx_idx;
> + u64 pkind_val, pkind_idx;
> +
> + if (vf >= nic->num_vf_en)
> + return;
> +
> + bgx_idx = NIC_GET_BGX_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
> + lmac = NIC_GET_LMAC_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
> +
> + pkind_idx = lmac + bgx_idx * MAX_LMAC_PER_BGX;
> + pkind_val = nic_reg_read(nic, NIC_PF_PKIND_0_15_CFG | (pkind_idx << 3));
> + pkind = (struct pkind_cfg *)_val;
> +
> + if (ptp->enable && !pkind->hdr_sl) {
> + /* Skiplen to exclude 8byte timestamp while parsing pkt
> +  * If not configured, will result in L2 errors.
> +  */
> + pkind->hdr_sl = 4;
> + /* Adjust max packet length allowed */
> + pkind->maxlen += (pkind->hdr_sl * 2);
> + bgx_config_timestamping(nic->node, bgx_idx, lmac, true);
> + nic_reg_write(nic,
> +   NIC_PF_RX_ETYPE_0_7 | (1 << 3),
> +   (ETYPE_ALG_ENDPARSE << 16) | ETH_P_1588);

don't need three lines for this function call.

> + } else if (!ptp->enable && pkind->hdr_sl) {
> + pkind->maxlen -= (pkind->hdr_sl * 2);
> + pkind->hdr_sl = 0;
> + bgx_config_timestamping(nic->node, bgx_idx, lmac, false);
> + nic_reg_write(nic,
> +   NIC_PF_RX_ETYPE_0_7 | (1 << 3),
> +   (1ULL << 16) | ETH_P_8021Q); /* reset value */

here neither.  Also avoid comment on the LHS.  If 1<<16 means "reset"
then just define a macro.

> + }
> +
> + nic_reg_write(nic, NIC_PF_PKIND_0_15_CFG | (pkind_idx << 3), pkind_val);
> +}
> +

Thanks,
Richard

Re: [PATCH net-next v5 2/2] net: thunderx: add timestamping support

2017-12-11 Thread Richard Cochran 
On Mon, Dec 11, 2017 at 05:14:31PM +0300, Aleksey Makarov wrote:
> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h 
> b/drivers/net/ethernet/cavium/thunder/nic.h
> index 4a02e618e318..204b234beb9d 100644
> --- a/drivers/net/ethernet/cavium/thunder/nic.h
> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> @@ -263,6 +263,8 @@ struct nicvf_drv_stats {
>   struct u64_stats_sync   syncp;
>  };
>  
> +struct cavium_ptp;
> +
>  struct nicvf {
>   struct nicvf*pnicvf;
>   struct net_device   *netdev;
> @@ -312,6 +314,12 @@ struct nicvf {
>   struct tasklet_struct   qs_err_task;
>   struct work_struct  reset_task;
>  
> + /* PTP timestamp */
> + struct cavium_ptp   *ptp_clock;
> + boolhw_rx_tstamp;
> + struct sk_buff  *ptp_skb;
> + atomic_ttx_ptp_skbs;

It is disturbing that the above two fields are set in different
places.  Shouldn't they be unified into one logical lock?

Here you clear them together:

> +static void nicvf_snd_ptp_handler(struct net_device *netdev,
> +   struct cqe_send_t *cqe_tx)
> +{
> + struct nicvf *nic = netdev_priv(netdev);
> + struct skb_shared_hwtstamps ts;
> + u64 ns;
> +
> + nic = nic->pnicvf;
> +
> + /* Sync for 'ptp_skb' */
> + smp_rmb();
> +
> + /* New timestamp request can be queued now */
> + atomic_set(>tx_ptp_skbs, 0);
> +
> + /* Check for timestamp requested skb */
> + if (!nic->ptp_skb)
> + return;
> +
> + /* Check if timestamping is timedout, which is set to 10us */
> + if (cqe_tx->send_status == CQ_TX_ERROP_TSTMP_TIMEOUT ||
> + cqe_tx->send_status == CQ_TX_ERROP_TSTMP_CONFLICT)
> + goto no_tstamp;
> +
> + /* Get the timestamp */
> + memset(, 0, sizeof(ts));
> + ns = cavium_ptp_tstamp2time(nic->ptp_clock, cqe_tx->ptp_timestamp);
> + ts.hwtstamp = ns_to_ktime(ns);
> + skb_tstamp_tx(nic->ptp_skb, );
> +
> +no_tstamp:
> + /* Free the original skb */
> + dev_kfree_skb_any(nic->ptp_skb);
> + nic->ptp_skb = NULL;
> + /* Sync 'ptp_skb' */
> + smp_wmb();
> +}
> +

but here you set the one:

> @@ -657,7 +697,12 @@ static void nicvf_snd_pkt_handler(struct net_device 
> *netdev,
>   prefetch(skb);
>   (*tx_pkts)++;
>   *tx_bytes += skb->len;
> - napi_consume_skb(skb, budget);
> + /* If timestamp is requested for this skb, don't free it */
> + if (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS &&
> + !nic->pnicvf->ptp_skb)
> + nic->pnicvf->ptp_skb = skb;
> + else
> + napi_consume_skb(skb, budget);
>   sq->skbuff[cqe_tx->sqe_ptr] = (u64)NULL;
>   } else {
>   /* In case of SW TSO on 88xx, only last segment will have

here you clear one:

> @@ -1319,12 +1382,28 @@ int nicvf_stop(struct net_device *netdev)
>  
>   nicvf_free_cq_poll(nic);
>  
> + /* Free any pending SKB saved to receive timestamp */
> + if (nic->ptp_skb) {
> + dev_kfree_skb_any(nic->ptp_skb);
> + nic->ptp_skb = NULL;
> + }
> +
>   /* Clear multiqset info */
>   nic->pnicvf = nic;
>  
>   return 0;
>  }

here you clear both:

> @@ -1394,6 +1473,12 @@ int nicvf_open(struct net_device *netdev)
>   if (nic->sqs_mode)
>   nicvf_get_primary_vf_struct(nic);
>  
> + /* Configure PTP timestamp */
> + if (nic->ptp_clock)
> + nicvf_config_hw_rx_tstamp(nic, nic->hw_rx_tstamp);
> + atomic_set(>tx_ptp_skbs, 0);
> + nic->ptp_skb = NULL;
> +
>   /* Configure receive side scaling and MTU */
>   if (!nic->sqs_mode) {
>   nicvf_rss_init(nic);

here you set the other:

> @@ -1385,6 +1388,29 @@ nicvf_sq_add_hdr_subdesc(struct nicvf *nic, struct 
> snd_queue *sq, int qentry,
>   hdr->inner_l3_offset = skb_network_offset(skb) - 2;
>   this_cpu_inc(nic->pnicvf->drv_stats->tx_tso);
>   }
> +
> + /* Check if timestamp is requested */
> + if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
> + skb_tx_timestamp(skb);
> + return;
> + }
> +
> + /* Tx timestamping not supported along with TSO, so ignore request */
> + if (skb_shinfo(skb)->gso_size)
> + return;
> +
> + /* HW supports only a single outstanding packet to timestamp */
> + if (!atomic_add_unless(>pnicvf->tx_ptp_skbs, 1, 1))
> + return;
> +
> + /* Mark the SKB for later reference */
> + skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +
> + /* Finally enable timestamp generation
> +  * Since 'post_cqe' is also set, two CQEs will be posted
> +  * for this packet i.e CQE_TYPE_SEND and CQE_TYPE_SEND_PTP.
> +  */
> + hdr->tstmp = 1;
>  }

and so it is completely non-obvious whether this is race free or not.

Thanks,
Richard

Re: [PATCH net-next v5 2/2] net: thunderx: add timestamping support

2017-12-11 Thread Richard Cochran 
On Mon, Dec 11, 2017 at 05:14:31PM +0300, Aleksey Makarov wrote:
> diff --git a/drivers/net/ethernet/cavium/thunder/nic.h 
> b/drivers/net/ethernet/cavium/thunder/nic.h
> index 4a02e618e318..204b234beb9d 100644
> --- a/drivers/net/ethernet/cavium/thunder/nic.h
> +++ b/drivers/net/ethernet/cavium/thunder/nic.h
> @@ -263,6 +263,8 @@ struct nicvf_drv_stats {
>   struct u64_stats_sync   syncp;
>  };
>  
> +struct cavium_ptp;
> +
>  struct nicvf {
>   struct nicvf*pnicvf;
>   struct net_device   *netdev;
> @@ -312,6 +314,12 @@ struct nicvf {
>   struct tasklet_struct   qs_err_task;
>   struct work_struct  reset_task;
>  
> + /* PTP timestamp */
> + struct cavium_ptp   *ptp_clock;
> + boolhw_rx_tstamp;
> + struct sk_buff  *ptp_skb;
> + atomic_ttx_ptp_skbs;

It is disturbing that the above two fields are set in different
places.  Shouldn't they be unified into one logical lock?

Here you clear them together:

> +static void nicvf_snd_ptp_handler(struct net_device *netdev,
> +   struct cqe_send_t *cqe_tx)
> +{
> + struct nicvf *nic = netdev_priv(netdev);
> + struct skb_shared_hwtstamps ts;
> + u64 ns;
> +
> + nic = nic->pnicvf;
> +
> + /* Sync for 'ptp_skb' */
> + smp_rmb();
> +
> + /* New timestamp request can be queued now */
> + atomic_set(>tx_ptp_skbs, 0);
> +
> + /* Check for timestamp requested skb */
> + if (!nic->ptp_skb)
> + return;
> +
> + /* Check if timestamping is timedout, which is set to 10us */
> + if (cqe_tx->send_status == CQ_TX_ERROP_TSTMP_TIMEOUT ||
> + cqe_tx->send_status == CQ_TX_ERROP_TSTMP_CONFLICT)
> + goto no_tstamp;
> +
> + /* Get the timestamp */
> + memset(, 0, sizeof(ts));
> + ns = cavium_ptp_tstamp2time(nic->ptp_clock, cqe_tx->ptp_timestamp);
> + ts.hwtstamp = ns_to_ktime(ns);
> + skb_tstamp_tx(nic->ptp_skb, );
> +
> +no_tstamp:
> + /* Free the original skb */
> + dev_kfree_skb_any(nic->ptp_skb);
> + nic->ptp_skb = NULL;
> + /* Sync 'ptp_skb' */
> + smp_wmb();
> +}
> +

but here you set the one:

> @@ -657,7 +697,12 @@ static void nicvf_snd_pkt_handler(struct net_device 
> *netdev,
>   prefetch(skb);
>   (*tx_pkts)++;
>   *tx_bytes += skb->len;
> - napi_consume_skb(skb, budget);
> + /* If timestamp is requested for this skb, don't free it */
> + if (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS &&
> + !nic->pnicvf->ptp_skb)
> + nic->pnicvf->ptp_skb = skb;
> + else
> + napi_consume_skb(skb, budget);
>   sq->skbuff[cqe_tx->sqe_ptr] = (u64)NULL;
>   } else {
>   /* In case of SW TSO on 88xx, only last segment will have

here you clear one:

> @@ -1319,12 +1382,28 @@ int nicvf_stop(struct net_device *netdev)
>  
>   nicvf_free_cq_poll(nic);
>  
> + /* Free any pending SKB saved to receive timestamp */
> + if (nic->ptp_skb) {
> + dev_kfree_skb_any(nic->ptp_skb);
> + nic->ptp_skb = NULL;
> + }
> +
>   /* Clear multiqset info */
>   nic->pnicvf = nic;
>  
>   return 0;
>  }

here you clear both:

> @@ -1394,6 +1473,12 @@ int nicvf_open(struct net_device *netdev)
>   if (nic->sqs_mode)
>   nicvf_get_primary_vf_struct(nic);
>  
> + /* Configure PTP timestamp */
> + if (nic->ptp_clock)
> + nicvf_config_hw_rx_tstamp(nic, nic->hw_rx_tstamp);
> + atomic_set(>tx_ptp_skbs, 0);
> + nic->ptp_skb = NULL;
> +
>   /* Configure receive side scaling and MTU */
>   if (!nic->sqs_mode) {
>   nicvf_rss_init(nic);

here you set the other:

> @@ -1385,6 +1388,29 @@ nicvf_sq_add_hdr_subdesc(struct nicvf *nic, struct 
> snd_queue *sq, int qentry,
>   hdr->inner_l3_offset = skb_network_offset(skb) - 2;
>   this_cpu_inc(nic->pnicvf->drv_stats->tx_tso);
>   }
> +
> + /* Check if timestamp is requested */
> + if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
> + skb_tx_timestamp(skb);
> + return;
> + }
> +
> + /* Tx timestamping not supported along with TSO, so ignore request */
> + if (skb_shinfo(skb)->gso_size)
> + return;
> +
> + /* HW supports only a single outstanding packet to timestamp */
> + if (!atomic_add_unless(>pnicvf->tx_ptp_skbs, 1, 1))
> + return;
> +
> + /* Mark the SKB for later reference */
> + skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +
> + /* Finally enable timestamp generation
> +  * Since 'post_cqe' is also set, two CQEs will be posted
> +  * for this packet i.e CQE_TYPE_SEND and CQE_TYPE_SEND_PTP.
> +  */
> + hdr->tstmp = 1;
>  }

and so it is completely non-obvious whether this is race free or not.

Thanks,
Richard

[PATCH net-next v5 2/2] net: thunderx: add timestamping support

2017-12-11 Thread Aleksey Makarov 
From: Sunil Goutham 

This adds timestamping support for both receive and transmit
paths. On the receive side no filters are supported i.e either
all pkts will get a timestamp appended infront of the packet or none.
On the transmit side HW doesn't support timestamp insertion but
only generates a separate CQE with transmitted packet's timestamp.
Also HW supports only one packet at a time for timestamping on the
transmit side.

Signed-off-by: Sunil Goutham 
Signed-off-by: Aleksey Makarov 
---
 drivers/net/ethernet/cavium/Kconfig|   1 +
 drivers/net/ethernet/cavium/thunder/nic.h  |  15 ++
 drivers/net/ethernet/cavium/thunder/nic_main.c |  58 ++-
 drivers/net/ethernet/cavium/thunder/nic_reg.h  |   1 +
 .../net/ethernet/cavium/thunder/nicvf_ethtool.c|  29 +++-
 drivers/net/ethernet/cavium/thunder/nicvf_main.c   | 173 -
 drivers/net/ethernet/cavium/thunder/nicvf_queues.c |  26 
 drivers/net/ethernet/cavium/thunder/thunder_bgx.c  |  29 
 drivers/net/ethernet/cavium/thunder/thunder_bgx.h  |   4 +
 9 files changed, 330 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/cavium/Kconfig 
b/drivers/net/ethernet/cavium/Kconfig
index 96586c0b4490..043e3c11c42b 100644
--- a/drivers/net/ethernet/cavium/Kconfig
+++ b/drivers/net/ethernet/cavium/Kconfig
@@ -27,6 +27,7 @@ config THUNDER_NIC_PF
 
 config THUNDER_NIC_VF
tristate "Thunder Virtual function driver"
+   imply CAVIUM_PTP
depends on 64BIT
---help---
  This driver supports Thunder's NIC virtual function
diff --git a/drivers/net/ethernet/cavium/thunder/nic.h 
b/drivers/net/ethernet/cavium/thunder/nic.h
index 4a02e618e318..204b234beb9d 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -263,6 +263,8 @@ struct nicvf_drv_stats {
struct u64_stats_sync   syncp;
 };
 
+struct cavium_ptp;
+
 struct nicvf {
struct nicvf*pnicvf;
struct net_device   *netdev;
@@ -312,6 +314,12 @@ struct nicvf {
struct tasklet_struct   qs_err_task;
struct work_struct  reset_task;
 
+   /* PTP timestamp */
+   struct cavium_ptp   *ptp_clock;
+   boolhw_rx_tstamp;
+   struct sk_buff  *ptp_skb;
+   atomic_ttx_ptp_skbs;
+
/* Interrupt coalescing settings */
u32 cq_coalesce_usecs;
u32 msg_enable;
@@ -371,6 +379,7 @@ struct nicvf {
 #defineNIC_MBOX_MSG_LOOPBACK   0x16/* Set interface in 
loopback */
 #defineNIC_MBOX_MSG_RESET_STAT_COUNTER 0x17/* Reset statistics 
counters */
 #defineNIC_MBOX_MSG_PFC0x18/* Pause frame control 
*/
+#defineNIC_MBOX_MSG_PTP_CFG0x19/* HW packet timestamp 
*/
 #defineNIC_MBOX_MSG_CFG_DONE   0xF0/* VF configuration 
done */
 #defineNIC_MBOX_MSG_SHUTDOWN   0xF1/* VF is being shutdown 
*/
 
@@ -521,6 +530,11 @@ struct pfc {
u8fc_tx;
 };
 
+struct set_ptp {
+   u8msg;
+   bool  enable;
+};
+
 /* 128 bit shared memory between PF and each VF */
 union nic_mbx {
struct { u8 msg; }  msg;
@@ -540,6 +554,7 @@ union nic_mbx {
struct set_loopback lbk;
struct reset_stat_cfg   reset_stat;
struct pfc  pfc;
+   struct set_ptp  ptp;
 };
 
 #define NIC_NODE_ID_MASK   0x03
diff --git a/drivers/net/ethernet/cavium/thunder/nic_main.c 
b/drivers/net/ethernet/cavium/thunder/nic_main.c
index 8f1dd55b3e08..4c1c5414a162 100644
--- a/drivers/net/ethernet/cavium/thunder/nic_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nic_main.c
@@ -426,13 +426,22 @@ static void nic_init_hw(struct nicpf *nic)
/* Enable backpressure */
nic_reg_write(nic, NIC_PF_BP_CFG, (1ULL << 6) | 0x03);
 
-   /* TNS and TNS bypass modes are present only on 88xx */
+   /* TNS and TNS bypass modes are present only on 88xx
+* Also offset of this CSR has changed in 81xx and 83xx.
+*/
if (nic->pdev->subsystem_device == PCI_SUBSYS_DEVID_88XX_NIC_PF) {
/* Disable TNS mode on both interfaces */
nic_reg_write(nic, NIC_PF_INTF_0_1_SEND_CFG,
- (NIC_TNS_BYPASS_MODE << 7) | BGX0_BLOCK);
+ (NIC_TNS_BYPASS_MODE << 7) |
+ BGX0_BLOCK | (1ULL << 16));
nic_reg_write(nic, NIC_PF_INTF_0_1_SEND_CFG | (1 << 8),
- (NIC_TNS_BYPASS_MODE << 7) | BGX1_BLOCK);
+ (NIC_TNS_BYPASS_MODE << 7) |
+ BGX1_BLOCK | (1ULL << 16));
+   } else {
+   /* Configure timestamp generation timeout to 10us */
+   for (i = 0; i < nic->hw->bgx_cnt; i++)
+

[PATCH net-next v5 2/2] net: thunderx: add timestamping support

2017-12-11 Thread Aleksey Makarov 
From: Sunil Goutham 

This adds timestamping support for both receive and transmit
paths. On the receive side no filters are supported i.e either
all pkts will get a timestamp appended infront of the packet or none.
On the transmit side HW doesn't support timestamp insertion but
only generates a separate CQE with transmitted packet's timestamp.
Also HW supports only one packet at a time for timestamping on the
transmit side.

Signed-off-by: Sunil Goutham 
Signed-off-by: Aleksey Makarov 
---
 drivers/net/ethernet/cavium/Kconfig|   1 +
 drivers/net/ethernet/cavium/thunder/nic.h  |  15 ++
 drivers/net/ethernet/cavium/thunder/nic_main.c |  58 ++-
 drivers/net/ethernet/cavium/thunder/nic_reg.h  |   1 +
 .../net/ethernet/cavium/thunder/nicvf_ethtool.c|  29 +++-
 drivers/net/ethernet/cavium/thunder/nicvf_main.c   | 173 -
 drivers/net/ethernet/cavium/thunder/nicvf_queues.c |  26 
 drivers/net/ethernet/cavium/thunder/thunder_bgx.c  |  29 
 drivers/net/ethernet/cavium/thunder/thunder_bgx.h  |   4 +
 9 files changed, 330 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/cavium/Kconfig 
b/drivers/net/ethernet/cavium/Kconfig
index 96586c0b4490..043e3c11c42b 100644
--- a/drivers/net/ethernet/cavium/Kconfig
+++ b/drivers/net/ethernet/cavium/Kconfig
@@ -27,6 +27,7 @@ config THUNDER_NIC_PF
 
 config THUNDER_NIC_VF
tristate "Thunder Virtual function driver"
+   imply CAVIUM_PTP
depends on 64BIT
---help---
  This driver supports Thunder's NIC virtual function
diff --git a/drivers/net/ethernet/cavium/thunder/nic.h 
b/drivers/net/ethernet/cavium/thunder/nic.h
index 4a02e618e318..204b234beb9d 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -263,6 +263,8 @@ struct nicvf_drv_stats {
struct u64_stats_sync   syncp;
 };
 
+struct cavium_ptp;
+
 struct nicvf {
struct nicvf*pnicvf;
struct net_device   *netdev;
@@ -312,6 +314,12 @@ struct nicvf {
struct tasklet_struct   qs_err_task;
struct work_struct  reset_task;
 
+   /* PTP timestamp */
+   struct cavium_ptp   *ptp_clock;
+   boolhw_rx_tstamp;
+   struct sk_buff  *ptp_skb;
+   atomic_ttx_ptp_skbs;
+
/* Interrupt coalescing settings */
u32 cq_coalesce_usecs;
u32 msg_enable;
@@ -371,6 +379,7 @@ struct nicvf {
 #defineNIC_MBOX_MSG_LOOPBACK   0x16/* Set interface in 
loopback */
 #defineNIC_MBOX_MSG_RESET_STAT_COUNTER 0x17/* Reset statistics 
counters */
 #defineNIC_MBOX_MSG_PFC0x18/* Pause frame control 
*/
+#defineNIC_MBOX_MSG_PTP_CFG0x19/* HW packet timestamp 
*/
 #defineNIC_MBOX_MSG_CFG_DONE   0xF0/* VF configuration 
done */
 #defineNIC_MBOX_MSG_SHUTDOWN   0xF1/* VF is being shutdown 
*/
 
@@ -521,6 +530,11 @@ struct pfc {
u8fc_tx;
 };
 
+struct set_ptp {
+   u8msg;
+   bool  enable;
+};
+
 /* 128 bit shared memory between PF and each VF */
 union nic_mbx {
struct { u8 msg; }  msg;
@@ -540,6 +554,7 @@ union nic_mbx {
struct set_loopback lbk;
struct reset_stat_cfg   reset_stat;
struct pfc  pfc;
+   struct set_ptp  ptp;
 };
 
 #define NIC_NODE_ID_MASK   0x03
diff --git a/drivers/net/ethernet/cavium/thunder/nic_main.c 
b/drivers/net/ethernet/cavium/thunder/nic_main.c
index 8f1dd55b3e08..4c1c5414a162 100644
--- a/drivers/net/ethernet/cavium/thunder/nic_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nic_main.c
@@ -426,13 +426,22 @@ static void nic_init_hw(struct nicpf *nic)
/* Enable backpressure */
nic_reg_write(nic, NIC_PF_BP_CFG, (1ULL << 6) | 0x03);
 
-   /* TNS and TNS bypass modes are present only on 88xx */
+   /* TNS and TNS bypass modes are present only on 88xx
+* Also offset of this CSR has changed in 81xx and 83xx.
+*/
if (nic->pdev->subsystem_device == PCI_SUBSYS_DEVID_88XX_NIC_PF) {
/* Disable TNS mode on both interfaces */
nic_reg_write(nic, NIC_PF_INTF_0_1_SEND_CFG,
- (NIC_TNS_BYPASS_MODE << 7) | BGX0_BLOCK);
+ (NIC_TNS_BYPASS_MODE << 7) |
+ BGX0_BLOCK | (1ULL << 16));
nic_reg_write(nic, NIC_PF_INTF_0_1_SEND_CFG | (1 << 8),
- (NIC_TNS_BYPASS_MODE << 7) | BGX1_BLOCK);
+ (NIC_TNS_BYPASS_MODE << 7) |
+ BGX1_BLOCK | (1ULL << 16));
+   } else {
+   /* Configure timestamp generation timeout to 10us */
+   for (i = 0; i < nic->hw->bgx_cnt; i++)
+   nic_reg_write(nic, NIC_PF_INTFX_SEND_CFG | (i <<