Re: [PATCH 09/11] ice: implement dpll interface to control cgu

From: Jiri Pirko <jiri@resnulli.us>
To: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@intel.com>
Cc: Vadim Fedorenko <vadim.fedorenko@linux.dev>,
	Jakub Kicinski <kuba@kernel.org>,
	Jonathan Lemon <jonathan.lemon@gmail.com>,
	Paolo Abeni <pabeni@redhat.com>,
	"Olech, Milena" <milena.olech@intel.com>,
	"Michalik, Michal" <michal.michalik@intel.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>, poros <poros@redhat.com>,
	mschmidt <mschmidt@redhat.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
	Bart Van Assche <bvanassche@acm.org>
Subject: Re: [PATCH 09/11] ice: implement dpll interface to control cgu
Date: Mon, 31 Jul 2023 14:19:43 +0200	[thread overview]
Message-ID: <ZMem35OUQiQmB9Vd@nanopsycho> (raw)
In-Reply-To: <DM6PR11MB46571657F0DF87765DAB32FE9B06A@DM6PR11MB4657.namprd11.prod.outlook.com>

Sat, Jul 29, 2023 at 01:03:59AM CEST, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Friday, July 21, 2023 1:39 PM
>>
>>Thu, Jul 20, 2023 at 11:19:01AM CEST, vadim.fedorenko@linux.dev wrote:
>>>From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>

[...]

>>>+static int ice_dpll_cb_lock(struct ice_pf *pf, struct netlink_ext_ack
>>>*extack)
>>>+{
>>>+	int i;
>>>+
>>>+	for (i = 0; i < ICE_DPLL_LOCK_TRIES; i++) {
>>>+		if (!test_bit(ICE_FLAG_DPLL, pf->flags)) {
>>
>>And again, as I already told you, this flag checking is totally
>>pointless. See below my comment to ice_dpll_init()/ice_dpll_deinit().
>>
>
>This is not pointless, will explain below.
>
>>
>>
>
>[...]
>

[...]

>>>+void ice_dpll_deinit(struct ice_pf *pf)
>>>+{
>>>+	bool cgu = ice_is_feature_supported(pf, ICE_F_CGU);
>>>+
>>>+	if (!test_bit(ICE_FLAG_DPLL, pf->flags))
>>>+		return;
>>>+	clear_bit(ICE_FLAG_DPLL, pf->flags);
>>>+
>>>+	ice_dpll_deinit_pins(pf, cgu);
>>>+	ice_dpll_deinit_dpll(pf, &pf->dplls.pps, cgu);
>>>+	ice_dpll_deinit_dpll(pf, &pf->dplls.eec, cgu);
>>>+	ice_dpll_deinit_info(pf);
>>>+	if (cgu)
>>>+		ice_dpll_deinit_worker(pf);
>>
>>Could you please order the ice_dpll_deinit() to be symmetrical to
>>ice_dpll_init()? Then, you can drop ICE_FLAG_DPLL flag entirely, as the
>>ice_dpll_periodic_work() function is the only reason why you need it
>>currently.
>>
>
>Not true.
>The feature flag is common approach in ice. If the feature was successfully

The fact that something is common does not necessarily mean it is
correct. 0 value argument.

>initialized the flag is set. It allows to determine if deinit of the feature
>is required on driver unload.
>
>Right now the check for the flag is not only in kworker but also in each
>callback, if the flag were cleared the data shall be not accessed by callbacks.

Could you please draw me a scenario when this could actually happen?
It is just a matter of ordering. Unregister dpll device/pins before you
cleanup the related resources and you don't need this ridiculous flag.

>I know this is not required, but it helps on loading and unloading the driver,
>thanks to that, spam of pin-get dump is not slowing the driver load/unload.

? Could you plese draw me a scenario how such thing may actually happen?

Thanks!

>
>>
>>>+	mutex_destroy(&pf->dplls.lock);
>>>+}

[...]