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

[Andy Shevchenko]

On Thu, May 30, 2024 at 8:52 AM D, Lakshmi Sowjanya
<lakshmi.sowjanya.d@xxxxxxxxx> wrote:
> > -----Original Message-----
> > From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > 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@xxxxxxxxxxxxxxx>
> > > > Sent: Monday, May 13, 2024 4:49 PM
> > > > On Mon, May 13, 2024 at 04:08:11PM +0530,
> > > > lakshmi.sowjanya.d@xxxxxxxxx
> > > > 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