| 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 linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CALHRZuoVtuHLFjwW_bJsWxVFYN=PYxwsj+YabNH4p=v82u-MVA@mail.gmail.com>
Date: Mon, 13 Jul 2020 11:40:34 +0530
From: sundeep subbaraya <sundeep.lkml@...il.com>
To: Richard Cochran <richardcochran@...il.com>
Cc: David Miller <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
sgoutham@...vell.com, Subbaraya Sundeep <sbhatta@...vell.com>,
Aleksey Makarov <amakarov@...vell.com>
Subject: Re: [PATCH v3 net-next 3/3] octeontx2-pf: Add support for PTP clock
Hi Richard,
On Thu, Jul 9, 2020 at 9:32 PM Richard Cochran <richardcochran@...il.com> wrote:
>
> On Thu, Jul 09, 2020 at 06:57:01PM +0530, sundeep.lkml@...il.com wrote:
>
> > @@ -1736,6 +1751,143 @@ static void otx2_reset_task(struct work_struct *work)
> > netif_trans_update(pf->netdev);
> > }
> >
> > +static int otx2_config_hw_rx_tstamp(struct otx2_nic *pfvf, bool enable)
> > +{
> > + struct msg_req *req;
> > + int err;
> > +
> > + if (pfvf->flags & OTX2_FLAG_RX_TSTAMP_ENABLED && enable)
> > + return 0;
>
> It appears that nothing protects pfvf->flags from concurrent access.
> Please double check and correct if needed.
>
Please correct me if am wrong ndo_open, ndo_close and ndo_ioctl are
protected by rtnl lock so it is okay here. But there is a reset task
in the driver
which accesses this flag too hence lock is required b/w those. I will fix this.
> > + mutex_lock(&pfvf->mbox.lock);
> > + if (enable)
> > + req = otx2_mbox_alloc_msg_cgx_ptp_rx_enable(&pfvf->mbox);
> > + else
> > + req = otx2_mbox_alloc_msg_cgx_ptp_rx_disable(&pfvf->mbox);
> > + if (!req) {
> > + mutex_unlock(&pfvf->mbox.lock);
> > + return -ENOMEM;
> > + }
> > +
> > + err = otx2_sync_mbox_msg(&pfvf->mbox);
> > + if (err) {
> > + mutex_unlock(&pfvf->mbox.lock);
> > + return err;
> > + }
> > +
> > + mutex_unlock(&pfvf->mbox.lock);
> > + if (enable)
> > + pfvf->flags |= OTX2_FLAG_RX_TSTAMP_ENABLED;
> > + else
> > + pfvf->flags &= ~OTX2_FLAG_RX_TSTAMP_ENABLED;
> > + return 0;
> > +}
> > +
> > +static int otx2_config_hw_tx_tstamp(struct otx2_nic *pfvf, bool enable)
> > +{
> > + struct msg_req *req;
> > + int err;
> > +
> > + if (pfvf->flags & OTX2_FLAG_TX_TSTAMP_ENABLED && enable)
> > + return 0;
>
> Again, please check concurrency here.
>
> > +
> > + mutex_lock(&pfvf->mbox.lock);
> > + if (enable)
> > + req = otx2_mbox_alloc_msg_nix_lf_ptp_tx_enable(&pfvf->mbox);
> > + else
> > + req = otx2_mbox_alloc_msg_nix_lf_ptp_tx_disable(&pfvf->mbox);
> > + if (!req) {
> > + mutex_unlock(&pfvf->mbox.lock);
> > + return -ENOMEM;
> > + }
> > +
> > + err = otx2_sync_mbox_msg(&pfvf->mbox);
> > + if (err) {
> > + mutex_unlock(&pfvf->mbox.lock);
> > + return err;
> > + }
> > +
> > + mutex_unlock(&pfvf->mbox.lock);
> > + if (enable)
> > + pfvf->flags |= OTX2_FLAG_TX_TSTAMP_ENABLED;
> > + else
> > + pfvf->flags &= ~OTX2_FLAG_TX_TSTAMP_ENABLED;
> > + return 0;
> > +}
> > +
> > +static int otx2_config_hwtstamp(struct net_device *netdev, struct ifreq *ifr)
> > +{
> > + struct otx2_nic *pfvf = netdev_priv(netdev);
> > + struct hwtstamp_config config;
> > +
> > + if (!pfvf->ptp)
> > + return -ENODEV;
> > +
> > + if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
> > + return -EFAULT;
> > +
> > + /* reserved for future extensions */
> > + if (config.flags)
> > + return -EINVAL;
> > +
> > + switch (config.tx_type) {
> > + case HWTSTAMP_TX_OFF:
> > + otx2_config_hw_tx_tstamp(pfvf, false);
> > + break;
> > + case HWTSTAMP_TX_ON:
> > + otx2_config_hw_tx_tstamp(pfvf, true);
> > + break;
> > + default:
> > + return -ERANGE;
> > + }
> > +
> > + switch (config.rx_filter) {
> > + case HWTSTAMP_FILTER_NONE:
> > + otx2_config_hw_rx_tstamp(pfvf, false);
> > + break;
> > + case HWTSTAMP_FILTER_ALL:
> > + case HWTSTAMP_FILTER_SOME:
> > + case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
> > + case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
> > + case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> > + case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> > + case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> > + case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> > + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> > + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> > + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
> > + case HWTSTAMP_FILTER_PTP_V2_EVENT:
> > + case HWTSTAMP_FILTER_PTP_V2_SYNC:
> > + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> > + otx2_config_hw_rx_tstamp(pfvf, true);
> > + config.rx_filter = HWTSTAMP_FILTER_ALL;
> > + break;
> > + default:
> > + return -ERANGE;
> > + }
> > +
> > + memcpy(&pfvf->tstamp, &config, sizeof(config));
> > +
> > + return copy_to_user(ifr->ifr_data, &config,
> > + sizeof(config)) ? -EFAULT : 0;
> > +}
> > +
> > +static int otx2_ioctl(struct net_device *netdev, struct ifreq *req, int cmd)
> > +{
> > + struct otx2_nic *pfvf = netdev_priv(netdev);
> > + struct hwtstamp_config *cfg = &pfvf->tstamp;
> > +
>
> Need to test phy_has_hwtstamp() here and pass ioctl to PHY if true.
>
For this platform PHY is taken care of by firmware hence it is not
possible.
> > + switch (cmd) {
> > + case SIOCSHWTSTAMP:
> > + return otx2_config_hwtstamp(netdev, req);
> > + case SIOCGHWTSTAMP:
> > + return copy_to_user(req->ifr_data, cfg,
> > + sizeof(*cfg)) ? -EFAULT : 0;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +}
> > +
> > static const struct net_device_ops otx2_netdev_ops = {
> > .ndo_open = otx2_open,
> > .ndo_stop = otx2_stop,
>
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ptp.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ptp.c
> > new file mode 100644
> > index 0000000..28058bd
> > --- /dev/null
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ptp.c
> > @@ -0,0 +1,208 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Marvell OcteonTx2 PTP support for ethernet driver */
> > +
> > +#include "otx2_common.h"
> > +#include "otx2_ptp.h"
> > +
> > +static int otx2_ptp_adjfine(struct ptp_clock_info *ptp_info, long scaled_ppm)
> > +{
> > + struct otx2_ptp *ptp = container_of(ptp_info, struct otx2_ptp,
> > + ptp_info);
> > + struct ptp_req *req;
> > + int err;
> > +
> > + if (!ptp->nic)
> > + return -ENODEV;
> > +
> > + req = otx2_mbox_alloc_msg_ptp_op(&ptp->nic->mbox);
> > + if (!req)
> > + return -ENOMEM;
> > +
> > + req->op = PTP_OP_ADJFINE;
> > + req->scaled_ppm = scaled_ppm;
> > +
> > + err = otx2_sync_mbox_msg(&ptp->nic->mbox);
> > + if (err)
> > + return err;
> > +
> > + return 0;
> > +}
> > +
> > +static u64 ptp_cc_read(const struct cyclecounter *cc)
> > +{
> > + struct otx2_ptp *ptp = container_of(cc, struct otx2_ptp, cycle_counter);
> > + struct ptp_req *req;
> > + struct ptp_rsp *rsp;
> > + int err;
> > +
> > + if (!ptp->nic)
> > + return 0;
> > +
> > + req = otx2_mbox_alloc_msg_ptp_op(&ptp->nic->mbox);
> > + if (!req)
> > + return 0;
> > +
> > + req->op = PTP_OP_GET_CLOCK;
> > +
> > + err = otx2_sync_mbox_msg(&ptp->nic->mbox);
> > + if (err)
> > + return 0;
> > +
> > + rsp = (struct ptp_rsp *)otx2_mbox_get_rsp(&ptp->nic->mbox.mbox, 0,
> > + &req->hdr);
> > + if (IS_ERR(rsp))
> > + return 0;
> > +
> > + return rsp->clk;
> > +}
> > +
> > +static int otx2_ptp_adjtime(struct ptp_clock_info *ptp_info, s64 delta)
> > +{
> > + struct otx2_ptp *ptp = container_of(ptp_info, struct otx2_ptp,
> > + ptp_info);
> > + struct otx2_nic *pfvf = ptp->nic;
> > +
> > + mutex_lock(&pfvf->mbox.lock);
> > + timecounter_adjtime(&ptp->time_counter, delta);
> > + mutex_unlock(&pfvf->mbox.lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int otx2_ptp_gettime(struct ptp_clock_info *ptp_info,
> > + struct timespec64 *ts)
> > +{
> > + struct otx2_ptp *ptp = container_of(ptp_info, struct otx2_ptp,
> > + ptp_info);
> > + struct otx2_nic *pfvf = ptp->nic;
> > + u64 nsec;
> > +
> > + mutex_lock(&pfvf->mbox.lock);
> > + nsec = timecounter_read(&ptp->time_counter);
> > + mutex_unlock(&pfvf->mbox.lock);
> > +
> > + *ts = ns_to_timespec64(nsec);
> > +
> > + return 0;
> > +}
> > +
> > +static int otx2_ptp_settime(struct ptp_clock_info *ptp_info,
> > + const struct timespec64 *ts)
> > +{
> > + struct otx2_ptp *ptp = container_of(ptp_info, struct otx2_ptp,
> > + ptp_info);
> > + struct otx2_nic *pfvf = ptp->nic;
> > + u64 nsec;
> > +
> > + nsec = timespec64_to_ns(ts);
> > +
> > + mutex_lock(&pfvf->mbox.lock);
> > + timecounter_init(&ptp->time_counter, &ptp->cycle_counter, nsec);
> > + mutex_unlock(&pfvf->mbox.lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int otx2_ptp_enable(struct ptp_clock_info *ptp_info,
> > + struct ptp_clock_request *rq, int on)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +int otx2_ptp_init(struct otx2_nic *pfvf)
> > +{
> > + struct otx2_ptp *ptp_ptr;
> > + struct cyclecounter *cc;
> > + struct ptp_req *req;
> > + int err;
> > +
> > + mutex_lock(&pfvf->mbox.lock);
> > + /* check if PTP block is available */
> > + req = otx2_mbox_alloc_msg_ptp_op(&pfvf->mbox);
> > + if (!req) {
> > + mutex_unlock(&pfvf->mbox.lock);
> > + return -ENOMEM;
> > + }
> > +
> > + req->op = PTP_OP_GET_CLOCK;
> > +
> > + err = otx2_sync_mbox_msg(&pfvf->mbox);
> > + if (err) {
> > + mutex_unlock(&pfvf->mbox.lock);
> > + return err;
> > + }
> > + mutex_unlock(&pfvf->mbox.lock);
> > +
> > + ptp_ptr = kzalloc(sizeof(*ptp_ptr), GFP_KERNEL);
> > + if (!ptp_ptr) {
> > + err = -ENOMEM;
> > + goto error;
> > + }
> > +
> > + ptp_ptr->nic = pfvf;
> > +
> > + cc = &ptp_ptr->cycle_counter;
> > + cc->read = ptp_cc_read;
> > + cc->mask = CYCLECOUNTER_MASK(64);
> > + cc->mult = 1;
> > + cc->shift = 0;
> > +
> > + timecounter_init(&ptp_ptr->time_counter, &ptp_ptr->cycle_counter,
> > + ktime_to_ns(ktime_get_real()));
> > +
> > + ptp_ptr->ptp_info = (struct ptp_clock_info) {
> > + .owner = THIS_MODULE,
> > + .name = "OcteonTX2 PTP",
> > + .max_adj = 1000000000ull,
> > + .n_ext_ts = 0,
> > + .n_pins = 0,
> > + .pps = 0,
> > + .adjfine = otx2_ptp_adjfine,
> > + .adjtime = otx2_ptp_adjtime,
> > + .gettime64 = otx2_ptp_gettime,
> > + .settime64 = otx2_ptp_settime,
> > + .enable = otx2_ptp_enable,
> > + };
> > +
> > + ptp_ptr->ptp_clock = ptp_clock_register(&ptp_ptr->ptp_info, pfvf->dev);
> > + if (IS_ERR(ptp_ptr->ptp_clock)) {
> > + err = PTR_ERR(ptp_ptr->ptp_clock);
> > + kfree(ptp_ptr);
> > + goto error;
> > + }
>
> You need to handle NULL here.
>
> * ptp_clock_register() - register a PTP hardware clock driver
> *
> * @info: Structure describing the new clock.
> * @parent: Pointer to the parent device of the new clock.
> *
> * Returns a valid pointer on success or PTR_ERR on failure. If PHC
> * support is missing at the configuration level, this function
> * returns NULL, and drivers are expected to gracefully handle that
> * case separately.
>
Okay. Will fix it.
> > +
> > + pfvf->ptp = ptp_ptr;
> > +
> > +error:
> > + return err;
> > +}
> > +
> > +void otx2_ptp_destroy(struct otx2_nic *pfvf)
> > +{
> > + struct otx2_ptp *ptp = pfvf->ptp;
> > +
> > + if (!ptp)
> > + return;
> > +
> > + ptp_clock_unregister(ptp->ptp_clock);
> > + kfree(ptp);
> > + pfvf->ptp = NULL;
> > +}
> > +
> > +int otx2_ptp_clock_index(struct otx2_nic *pfvf)
> > +{
> > + if (!pfvf->ptp)
> > + return -ENODEV;
> > +
> > + return ptp_clock_index(pfvf->ptp->ptp_clock);
> > +}
> > +
> > +int otx2_ptp_tstamp2time(struct otx2_nic *pfvf, u64 tstamp, u64 *tsns)
> > +{
> > + if (!pfvf->ptp)
> > + return -ENODEV;
> > +
> > + *tsns = timecounter_cyc2time(&pfvf->ptp->time_counter, tstamp);
> > +
> > + return 0;
> > +}
>
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > index 3a5b34a..1f90426 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > @@ -16,6 +16,7 @@
> > #include "otx2_common.h"
> > #include "otx2_struct.h"
> > #include "otx2_txrx.h"
> > +#include "otx2_ptp.h"
> >
> > #define CQE_ADDR(CQ, idx) ((CQ)->cqe_base + ((CQ)->cqe_size * (idx)))
> >
> > @@ -81,8 +82,11 @@ static void otx2_snd_pkt_handler(struct otx2_nic *pfvf,
> > int budget, int *tx_pkts, int *tx_bytes)
> > {
> > struct nix_send_comp_s *snd_comp = &cqe->comp;
> > + struct skb_shared_hwtstamps ts;
> > struct sk_buff *skb = NULL;
> > + u64 timestamp, tsns;
> > struct sg_list *sg;
> > + int err;
> >
> > if (unlikely(snd_comp->status) && netif_msg_tx_err(pfvf))
> > net_err_ratelimited("%s: TX%d: Error in send CQ status:%x\n",
> > @@ -94,6 +98,18 @@ static void otx2_snd_pkt_handler(struct otx2_nic *pfvf,
> > if (unlikely(!skb))
> > return;
> >
> > + if (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) {
>
> SKBTX_IN_PROGRESS may be set by the PHY, so you need to test whether
> time stamping is enabled in your MAC driver as well.
>
In our case PHY will not set it and the pfvf/MAC driver sets it.
Thanks for review,
Sundeep
> > + timestamp = ((u64 *)sq->timestamps->base)[snd_comp->sqe_id];
> > + if (timestamp != 1) {
> > + err = otx2_ptp_tstamp2time(pfvf, timestamp, &tsns);
> > + if (!err) {
> > + memset(&ts, 0, sizeof(ts));
> > + ts.hwtstamp = ns_to_ktime(tsns);
> > + skb_tstamp_tx(skb, &ts);
> > + }
> > + }
> > + }
> > +
> > *tx_bytes += skb->len;
> > (*tx_pkts)++;
> > otx2_dma_unmap_skb_frags(pfvf, sg);
>
> Thanks,
> Richard
Powered by blists - more mailing lists