netdev - Re: [PATCH net-next v2 1/5] ptp: Add cycles support for virtual clocks

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] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 10 Apr 2022 14:32:32 +0200
From:   Gerhard Engleder <gerhard@...leder-embedded.com>
To:     Richard Cochran <richardcochran@...il.com>, yangbo.lu@....com
Cc:     Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, mlichvar@...hat.com,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 1/5] ptp: Add cycles support for virtual clocks

> > @@ -225,6 +233,21 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
> >       mutex_init(&ptp->n_vclocks_mux);
> >       init_waitqueue_head(&ptp->tsev_wq);
> >
> > +     if (!ptp->info->getcycles64 && !ptp->info->getcyclesx64) {
>
> Please swap blocks, using non-negated logical test:
>
>         if (ptp->info->getcycles64 || ptp->info->getcyclesx64)

I will change it as suggested.

> > +             /* Free running cycle counter not supported, use time. */
> > +             ptp->info->getcycles64 = ptp_getcycles64;
> > +
> > +             if (ptp->info->gettimex64)
> > +                     ptp->info->getcyclesx64 = ptp->info->gettimex64;
> > +
> > +             if (ptp->info->getcrosststamp)
> > +                     ptp->info->getcrosscycles = ptp->info->getcrosststamp;
> > +     } else {
> > +             ptp->has_cycles = true;
> > +             if (!ptp->info->getcycles64 && ptp->info->getcyclesx64)
> > +                     ptp->info->getcycles64 = ptp_getcycles64;
> > +     }
> > +
> >       if (ptp->info->do_aux_work) {
> >               kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
> >               ptp->kworker = kthread_create_worker(0, "ptp%d", ptp->index);
>
>
> > @@ -231,10 +231,12 @@ static ssize_t n_vclocks_store(struct device *dev,
> >                       *(ptp->vclock_index + ptp->n_vclocks - i) = -1;
> >       }
> >
> > -     if (num == 0)
> > -             dev_info(dev, "only physical clock in use now\n");
> > -     else
> > -             dev_info(dev, "guarantee physical clock free running\n");
> > +     if (!ptp->has_cycles) {
>
> Not sure what this test means ...

I thought these dev_info() are useless if the free running cycle
counter is supported,
because the behavior of the physical clock does not change in this case.

> > +             if (num == 0)
> > +                     dev_info(dev, "only physical clock in use now\n");
>
> Shouldn't this one print even if has_cycles == false?

It will print if has_cycles == false.

In my opinion this dev_info() tells the user that the physical clock can be used
again. So it does not carry any interesting information if has_cycles == true.

> > +             else
> > +                     dev_info(dev, "guarantee physical clock free running\n");
> > +     }
> >
> >       ptp->n_vclocks = num;
> >       mutex_unlock(&ptp->n_vclocks_mux);

Thank you!

Gerhard