Received-SPF: pass (google.com: domain of linux-kernel+bounces-194958-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: "D, Lakshmi Sowjanya" <lakshmi.sowjanya.d@intel.com>
To: Andy Shevchenko <andy.shevchenko@gmail.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>
Subject: RE: [PATCH v8 10/12] pps: generators: Add PPS Generator TIO Driver
Thread-Topic: [PATCH v8 10/12] pps: generators: Add PPS Generator TIO Driver
Thread-Index: AQHapSHXUIkwGaeDCkGimvlL+UU2cbGVBGIAgBXrFfCAAEwvgIAEIpPA
Date: Thu, 30 May 2024 05:52:22 +0000
Message-ID: <CY8PR11MB7364118081A77973A9504C4CC4F32@CY8PR11MB7364.namprd11.prod.outlook.com>
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>
In-Reply-To: <ZlSZ63ST-Pj9CwCh@surfacebook.localdomain>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?4/gc4AuBADE/NRjcGtPgJdXdIDDGWPr7Uwy6+C4l+bHbL/jHV3cQ8SZ42ndp?=
 =?us-ascii?Q?CfWy3/QJ/7PNGHOTLcD7Xw5IMDlR7Qeu88/pHckQLnft2e2EfFQKKeFCowRh?=
 =?us-ascii?Q?gT5bMPFwmxiK6hArJAbpeCxocjneqeRB3H+w+9Vg3TFUqD0msL83QFalC2sL?=
 =?us-ascii?Q?yeE8UJ4ZnGwXk5YmxN5Wml3GiwdJNfYjO8SncoXE6+gaTpL7zERoYzqA9s08?=
 =?us-ascii?Q?TQrZhu9ECitB5l0MNIvMZ3E9mBd+gxFUU4Ta1Cf6O8SbBFrvkIoVvxeSKM83?=
 =?us-ascii?Q?XvscuR5c/jH8dqGtwv06qXrrSKfcGGwYvD+O/gUCvwaF9RiG3vGijbWqLOZJ?=
 =?us-ascii?Q?AmdcFRjBYXNbcs2mdDTTk/47Fp3zEs66tWNAUbSDNVIZtkqwGa9C50/MSy89?=
 =?us-ascii?Q?z+p8VO2h84918G/3kLvObBuKibUmQYvghd18ASJn9+vcgSRslXn/YkGYPYnG?=
 =?us-ascii?Q?ynY/aorUSuyomapQ2y/ywjJDGCp5381t/JeEMa6u/kGmfgDMDAe0+VjuLPOl?=
 =?us-ascii?Q?HEGalqfHy7LP35CRHNpHhaiceKZrZTi0ryK6OtY4e8W0OuJRFoHAUPMFQdD0?=
 =?us-ascii?Q?KfMTtgIl9gqqwGMuqnlF5CXc+/4qb6cmEynPHASXxItSn1pK+Vq9EirVd6nu?=
 =?us-ascii?Q?uGf9vGus8F1bbWRVHZ1/H8igubPeGUhsPsw2aXwFT8CJr6t1VJdvSTPpkE13?=
 =?us-ascii?Q?F9jpY9zDcuYwhQfBVUWcgWByoXz3y0uHzugNIZ05L8WTEKWNjZIh2heWTVaX?=
 =?us-ascii?Q?q9S2eETJqMkOMsAOsOsxTCNFOjcVHj2UU+O9VSuyfGO8cAYfqysWN7xFEf2Q?=
 =?us-ascii?Q?LV818MZkLihNJ6vNXiJvl2AoTnhT/Xh3NuFNwQt+EU9pM2YBQScitf7qL61W?=
 =?us-ascii?Q?TstRX1jdI4UZe0LsVn2iFEzlBQ8y7dbYD2UgypcshJkQTvgPg8Bq+H/eSmIi?=
 =?us-ascii?Q?y4AhYQGBt0HDxsB4imj/T1If1pHx5O87FPReHjYBVWI7FKjY0U7FPMFtNeiH?=
 =?us-ascii?Q?zYOr0dampjJBZqXwnmoTaAC4eeM66vNXvAlu0HtcbeCE7tgBLvIWfWgEBU9t?=
 =?us-ascii?Q?h9aE39NzwT9u8z7Kmr6WNrUk5wIPSmRxVyQ9+tUWJqGzYNXGiYFl11i/u0et?=
 =?us-ascii?Q?DRPFLdDqZqMAmotct4ApoEMSK1e1hhXfigNXqPbXgGQ0T8OeiQ4H13+5nQFA?=
 =?us-ascii?Q?pMHbd3sm8cirMIvZC5t+iJVP60feQYQbLpM/Oi8eSmvf5qYxttlySNbw0Gv3?=
 =?us-ascii?Q?+TIUKYyd8TZTC51iU01sNWkjgRVq0FirQP0LawXgm/lgmeZFVNPVSgYqOA16?=
 =?us-ascii?Q?7Rsw4t05eU2eUl6e284vH7YVQ7aQ+wKcRToLWjuL0+z6KAVtpU+k/bsSrXDW?=
 =?us-ascii?Q?ewrZDP/mQzmMYV4h6l6VD78Q6WtVFv/z/F3vD9yiSZLfXEcbtx6llydBVFT7?=
 =?us-ascii?Q?0md7yewqpMkr4qNop2ZSxfuLs38oL8FLncDLzaDiZ1KkEqhqmGLnPgD7f/K8?=
 =?us-ascii?Q?xW8hKPWceaaM4Qad8yROEqSiMHGBgnkVHV+EbiEauL56Nd09TmR/qOCdoiT0?=
 =?us-ascii?Q?ikaK/N6zeQXwtjXcJnjJ/RsaTBE1s64Wj8Ec7QxOZKRWnc3ThlnUJSm1Ysx1?=
 =?us-ascii?Q?pg=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CY8PR11MB7364.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e5005cd1-c896-4da7-3947-08dc806cacb6
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 May 2024 05:52:22.1989
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: J6+p97mequNQpAEEVdjcinzrqNbu/HZ06A3+wgVaE/mKyelkMXdzp1Gn+X15qo3lp4gXNOFKPvoUoJVPof+GkbD0Vr2EEsCFxFFaiCHgz1c=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB8514
X-OriginatorOrg: intel.com


> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Monday, May 27, 2024 8:04 PM
> To: D, Lakshmi Sowjanya <lakshmi.sowjanya.d@intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; tglx@linutronix.=
de;
> jstultz@google.com; giometti@enneenne.com; corbet@lwn.net; linux-
> kernel@vger.kernel.org; x86@kernel.org; netdev@vger.kernel.org; linux-
> doc@vger.kernel.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;
> alexandre.torgue@foss.st.com; joabreu@synopsys.com;
> mcoquelin.stm32@gmail.com; perex@perex.cz; linux-sound@vger.kernel.org;
> Nguyen, Anthony L <anthony.l.nguyen@intel.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>
> Subject: Re: [PATCH v8 10/12] pps: generators: Add PPS Generator TIO Driv=
er
>=20
> 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:
>=20
> ...
>=20
> > > > +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_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 mov=
ed to (1).
> >
> > Please, correct me if my understanding is wrong.
>=20
> So, there is a possibility to have a PPS being run (by somebody else) eve=
n if there
> is no ART provided?
>=20
> If "yes", your check is wrong to begin with. If "no", my suggestion is co=
rrect, 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.=20
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 related=
 to ART");
                return -EPERM;=20
        }

        err =3D kstrtobool(buf, &enable);
        if (err)
                return err;

>=20
> > > 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.
>=20
> See above. It's not about this kind of restriction.
>=20
> > > > +		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
>=20

Regards
Sowjanya