Received-SPF: pass (google.com: domain of linux-kernel+bounces-195076-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240513103813.5666-1-lakshmi.sowjanya.d@intel.com>
 <20240513103813.5666-11-lakshmi.sowjanya.d@intel.com> <ZkH3GP2b9WTz9W3W@smile.fi.intel.com>
 <CY8PR11MB7364D1C85099E4337408EBAFC4F02@CY8PR11MB7364.namprd11.prod.outlook.com>
 <ZlSZ63ST-Pj9CwCh@surfacebook.localdomain> <CY8PR11MB7364118081A77973A9504C4CC4F32@CY8PR11MB7364.namprd11.prod.outlook.com>
In-Reply-To: <CY8PR11MB7364118081A77973A9504C4CC4F32@CY8PR11MB7364.namprd11.prod.outlook.com>
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Thu, 30 May 2024 11:38:40 +0300
Message-ID: <CAHp75Vevif+oX8Lq9D90ekTSixC6Q2Mfr38HrgVhzq0ab-COyQ@mail.gmail.com>
Subject: Re: [PATCH v8 10/12] pps: generators: Add PPS Generator TIO Driver
To: "D, Lakshmi Sowjanya" <lakshmi.sowjanya.d@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>, 
	"tglx@linutronix.de" <tglx@linutronix.de>, "jstultz@google.com" <jstultz@google.com>, 
	"giometti@enneenne.com" <giometti@enneenne.com>, "corbet@lwn.net" <corbet@lwn.net>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "x86@kernel.org" <x86@kernel.org>, 
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>, 
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>, 
	"intel-wired-lan@lists.osuosl.org" <intel-wired-lan@lists.osuosl.org>, "Dong, Eddie" <eddie.dong@intel.com>, 
	"Hall, Christopher S" <christopher.s.hall@intel.com>, 
	"Brandeburg, Jesse" <jesse.brandeburg@intel.com>, "davem@davemloft.net" <davem@davemloft.net>, 
	"alexandre.torgue@foss.st.com" <alexandre.torgue@foss.st.com>, 
	"joabreu@synopsys.com" <joabreu@synopsys.com>, 
	"mcoquelin.stm32@gmail.com" <mcoquelin.stm32@gmail.com>, "perex@perex.cz" <perex@perex.cz>, 
	"linux-sound@vger.kernel.org" <linux-sound@vger.kernel.org>, 
	"Nguyen, Anthony L" <anthony.l.nguyen@intel.com>, 
	"peter.hilber@opensynergy.com" <peter.hilber@opensynergy.com>, "N, Pandith" <pandith.n@intel.com>, 
	"Mohan, Subramanian" <subramanian.mohan@intel.com>, 
	"T R, Thejesh Reddy" <thejesh.reddy.t.r@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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

..

> > > > > +static ssize_t enable_store(struct device *dev, struct
> > > > > +device_attribute
> > > > *attr, const char *buf,
> > > > > +                           size_t count)
> > > > > +{
> > > > > +       struct pps_tio *tio =3D dev_get_drvdata(dev);
> > > > > +       bool enable;
> > > > > +       int err;
> > > >
> > > > (1)
> > > >
> > > > > +       err =3D kstrtobool(buf, &enable);
> > > > > +       if (err)
> > > > > +               return err;
> > > > > +
> > > > > +       guard(spinlock_irqsave)(&tio->lock);
> > > > > +       if (enable && !tio->enabled) {
> > > >
> > > > > +               if (!timekeeping_clocksource_has_base(CSID_X86_AR=
T)) {
> > > > > +                       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 m=
oved to (1).
> > >
> > > Please, correct me if my understanding is wrong.
> >
> > So, there is a possibility to have a PPS being run (by somebody else) e=
ven 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 eve=
n in case of disable.

Please do,

> Code readability is better with this approach.
>
>         struct pps_tio *tio =3D 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 relat=
ed to ART");

started --> used

>                 return -EPERM;
>         }
>
>         err =3D kstrtobool(buf, &enable);
>         if (err)
>                 return err;
>
> > > > I.o.w. this sounds too weird to me and reading the code doesn't giv=
e
> > > > 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 =3D true;
> > > > > +       } else if (!enable && tio->enabled) {
> > > > > +               hrtimer_cancel(&tio->timer);
> > > > > +               pps_tio_disable(tio);
> > > > > +               tio->enabled =3D false;
> > > > > +       }
> > > > > +       return count;
> > > > > +}

--=20
With Best Regards,
Andy Shevchenko