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

[Jiri Pirko]

Sat, Jul 29, 2023 at 01:03:59AM CEST, arkadiusz.kubalewski at intel.com wrote:
>>From: Jiri Pirko <jiri at resnulli.us>
>>Sent: Friday, July 21, 2023 1:39 PM
>>
>>Thu, Jul 20, 2023 at 11:19:01AM CEST, vadim.fedorenko at linux.dev wrote:
>>>From: Arkadiusz Kubalewski <arkadiusz.kubalewski at 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);
>>>+}


[...]