netdev - Re: [PATCH v8 10/12] pps: generators: Add PPS Generator TIO Driver

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: <CAHp75Vevif+oX8Lq9D90ekTSixC6Q2Mfr38HrgVhzq0ab-COyQ@mail.gmail.com>
Date: Thu, 30 May 2024 11:38:40 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: "D, Lakshmi Sowjanya" <lakshmi.sowjanya.d@...el.com>
Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	"tglx@...utronix.de" <tglx@...utronix.de>, "jstultz@...gle.com" <jstultz@...gle.com>, 
	"giometti@...eenne.com" <giometti@...eenne.com>, "corbet@....net" <corbet@....net>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>, 
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, 
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>, 
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>, "Dong, Eddie" <eddie.dong@...el.com>, 
	"Hall, Christopher S" <christopher.s.hall@...el.com>, 
	"Brandeburg, Jesse" <jesse.brandeburg@...el.com>, "davem@...emloft.net" <davem@...emloft.net>, 
	"alexandre.torgue@...s.st.com" <alexandre.torgue@...s.st.com>, 
	"joabreu@...opsys.com" <joabreu@...opsys.com>, 
	"mcoquelin.stm32@...il.com" <mcoquelin.stm32@...il.com>, "perex@...ex.cz" <perex@...ex.cz>, 
	"linux-sound@...r.kernel.org" <linux-sound@...r.kernel.org>, 
	"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, 
	"peter.hilber@...nsynergy.com" <peter.hilber@...nsynergy.com>, "N, Pandith" <pandith.n@...el.com>, 
	"Mohan, Subramanian" <subramanian.mohan@...el.com>, 
	"T R, Thejesh Reddy" <thejesh.reddy.t.r@...el.com>
Subject: Re: [PATCH v8 10/12] pps: generators: Add PPS Generator TIO Driver

On Thu, May 30, 2024 at 8:52 AM D, Lakshmi Sowjanya
<lakshmi.sowjanya.d@...el.com> wrote:
> > -----Original Message-----
> > From: Andy Shevchenko <andy.shevchenko@...il.com>
> > Sent: Monday, May 27, 2024 8:04 PM
> > Mon, May 27, 2024 at 11:48:54AM +0000, D, Lakshmi Sowjanya kirjoitti:
> > > > -----Original Message-----
> > > > From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > > > Sent: Monday, May 13, 2024 4:49 PM
> > > > On Mon, May 13, 2024 at 04:08:11PM +0530,
> > > > lakshmi.sowjanya.d@...el.com
> > > > wrote:

...

> > > > > +static ssize_t enable_store(struct device *dev, struct
> > > > > +device_attribute
> > > > *attr, const char *buf,
> > > > > +                           size_t count)
> > > > > +{
> > > > > +       struct pps_tio *tio = dev_get_drvdata(dev);
> > > > > +       bool enable;
> > > > > +       int err;
> > > >
> > > > (1)
> > > >
> > > > > +       err = kstrtobool(buf, &enable);
> > > > > +       if (err)
> > > > > +               return err;
> > > > > +
> > > > > +       guard(spinlock_irqsave)(&tio->lock);
> > > > > +       if (enable && !tio->enabled) {
> > > >
> > > > > +               if (!timekeeping_clocksource_has_base(CSID_X86_ART)) {
> > > > > +                       dev_err(tio->dev, "PPS cannot be started as clock is
> > > > not related
> > > > > +to ART");
> > > >
> > > > Why not simply dev_err(dev, ...)?
> > > >
> > > > > +                       return -EPERM;
> > > > > +               }
> > > >
> > > > I'm wondering if we can move this check to (1) above.
> > > > Because currently it's a good question if we are able to stop PPS
> > > > which was run by somebody else without this check done.
> > >
> > > Do you mean can someone stop the signal without this check?
> > > Yes, this check is not required to stop.  So, I feel it need not be moved to (1).
> > >
> > > Please, correct me if my understanding is wrong.
> >
> > So, there is a possibility to have a PPS being run (by somebody else) even if there
> > is no ART provided?
> >
> > If "yes", your check is wrong to begin with. If "no", my suggestion is correct, i.e.
> > there is no need to stop something that can't be started at all.
>
> It is a "no", PPS doesn't start without ART.
>
> We can move the check to (1), but it would always be checking for ART even in case of disable.

Please do,

> Code readability is better with this approach.
>
>         struct pps_tio *tio = dev_get_drvdata(dev);
>         bool enable;
>         int err;
>
>         if (!timekeeping_clocksource_has_base(CSID_X86_ART)) {
>                 dev_err(dev, "PPS cannot be started as clock is not related to ART");

started --> used

>                 return -EPERM;
>         }
>
>         err = kstrtobool(buf, &enable);
>         if (err)
>                 return err;
>
> > > > I.o.w. this sounds too weird to me and reading the code doesn't give
> > > > any hint if it's even possible. And if it is, are we supposed to
> > > > touch that since it was definitely *not* us who ran it.
> > >
> > > Yes, we are not restricting on who can stop/start the signal.
> >
> > See above. It's not about this kind of restriction.
> >
> > > > > +               pps_tio_direction_output(tio);
> > > > > +               hrtimer_start(&tio->timer, first_event(tio),
> > > > HRTIMER_MODE_ABS);
> > > > > +               tio->enabled = true;
> > > > > +       } else if (!enable && tio->enabled) {
> > > > > +               hrtimer_cancel(&tio->timer);
> > > > > +               pps_tio_disable(tio);
> > > > > +               tio->enabled = false;
> > > > > +       }
> > > > > +       return count;
> > > > > +}

-- 
With Best Regards,
Andy Shevchenko