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
