netdev - Re: [Patch net-next v1 04/12] net: dsa: microchip: ptp: Manipulating absolute time using ptp hw clock

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2d36cc5c34a995f680177a52e4659ad07a31ab8f.camel@microchip.com>
Date:   Fri, 2 Dec 2022 09:40:17 +0000
From:   <Arun.Ramadoss@...rochip.com>
To:     <olteanv@...il.com>
CC:     <andrew@...n.ch>, <linux-kernel@...r.kernel.org>,
        <UNGLinuxDriver@...rochip.com>, <vivien.didelot@...il.com>,
        <linux@...linux.org.uk>, <ceggers@...i.de>,
        <Tristram.Ha@...rochip.com>, <f.fainelli@...il.com>,
        <kuba@...nel.org>, <edumazet@...gle.com>, <pabeni@...hat.com>,
        <richardcochran@...il.com>, <netdev@...r.kernel.org>,
        <Woojung.Huh@...rochip.com>, <davem@...emloft.net>
Subject: Re: [Patch net-next v1 04/12] net: dsa: microchip: ptp: Manipulating
 absolute time using ptp hw clock

Hi Vladimir,
Thanks for the review comment.

On Thu, 2022-12-01 at 03:04 +0200, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Mon, Nov 28, 2022 at 04:02:19PM +0530, Arun Ramadoss wrote:
> > diff --git a/drivers/net/dsa/microchip/ksz_ptp.c
> > b/drivers/net/dsa/microchip/ksz_ptp.c
> > index 184aa57a8489..415522ef4ce9 100644
> > --- a/drivers/net/dsa/microchip/ksz_ptp.c
> > +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> > @@ -200,6 +209,12 @@ static int ksz_ptp_settime(struct
> > ptp_clock_info *ptp,
> >               goto error_return;
> > 
> >       ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_LOAD_TIME,
> > PTP_LOAD_TIME);
> > +     if (ret)
> > +             goto error_return;
> > +
> > +     spin_lock_bh(&ptp_data->clock_lock);
> 
> Why disable bottom halves? Where is the bottom half that this races
> with?

The interrupts are added in the following patches in the series. During
the deferred packet timestamping, partial timestamps are reconstructed
to absolute time using the ptp_data->clock_time in the bottom halves.
So we need this spin_lock_bh.

> 
> > +     ptp_data->clock_time = *ts;
> > +     spin_unlock_bh(&ptp_data->clock_lock);
> > 
> >  error_return:
> >       mutex_unlock(&ptp_data->lock);
> >  }
> > 
> > +/*  Function is pointer to the do_aux_work in the ptp_clock
> > capability */
> > +static long ksz_ptp_do_aux_work(struct ptp_clock_info *ptp)
> > +{
> > +     struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
> > +     struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
> > +     struct timespec64 ts;
> > +
> > +     mutex_lock(&ptp_data->lock);
> > +     _ksz_ptp_gettime(dev, &ts);
> > +     mutex_unlock(&ptp_data->lock);
> 
> Why don't you call ksz_ptp_gettime(ptp, &ts) directly?

I will use ksz_ptp_gettime directly.

> 
> > +
> > +     spin_lock_bh(&ptp_data->clock_lock);
> > +     ptp_data->clock_time = ts;
> > +     spin_unlock_bh(&ptp_data->clock_lock);
> > +
> > +     return HZ;  /* reschedule in 1 second */
> > +}
> > +
> >  static int ksz_ptp_start_clock(struct ksz_device *dev)
> >  {
> > -     return ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_CLK_ENABLE,
> > PTP_CLK_ENABLE);
> > +     struct ksz_ptp_data *ptp_data = &dev->ptp_data;
> > +     int ret;
> > +
> > +     ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_CLK_ENABLE,
> > PTP_CLK_ENABLE);
> > +     if (ret)
> > +             return ret;
> > +
> > +     spin_lock_bh(&ptp_data->clock_lock);
> > +     ptp_data->clock_time.tv_sec = 0;
> > +     ptp_data->clock_time.tv_nsec = 0;
> > +     spin_unlock_bh(&ptp_data->clock_lock);
> 
> Does ksz_ptp_start_clock() race with anything? The PTP clock has not
> even been registered by the time this has been called. This is
> literally
> an example of the "spin_lock_init(); spin_lock();" antipattern.

Yes, this function is called before PTP clock registeration. I will
remove the spin_lock for here.

> 
> > +
> > +     return 0;
> >  }