> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Monday, May 27, 2024 8:04 PM
> To: D, Lakshmi Sowjanya <lakshmi.sowjanya.d@xxxxxxxxx>
> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; tglx@xxxxxxxxxxxxx;
> jstultz@xxxxxxxxxx; giometti@xxxxxxxxxxxx; corbet@xxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> doc@xxxxxxxxxxxxxxx; intel-wired-lan@xxxxxxxxxxxxxxxx; Dong, Eddie
> <eddie.dong@xxxxxxxxx>; Hall, Christopher S <christopher.s.hall@xxxxxxxxx>;
> Brandeburg, Jesse <jesse.brandeburg@xxxxxxxxx>; davem@xxxxxxxxxxxxx;
> alexandre.torgue@xxxxxxxxxxx; joabreu@xxxxxxxxxxxx;
> mcoquelin.stm32@xxxxxxxxx; perex@xxxxxxxx; linux-sound@xxxxxxxxxxxxxxx;
> Nguyen, Anthony L <anthony.l.nguyen@xxxxxxxxx>;
> peter.hilber@xxxxxxxxxxxxxxx; N, Pandith <pandith.n@xxxxxxxxx>; Mohan,
> Subramanian <subramanian.mohan@xxxxxxxxx>; T R, Thejesh Reddy
> <thejesh.reddy.t.r@xxxxxxxxx>
> Subject: Re: [PATCH v8 10/12] pps: generators: Add PPS Generator TIO Driver
>
> 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.
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");
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
>
Regards
Sowjanya
