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

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri at resnulli.us>
>Sent: Monday, July 31, 2023 2:20 PM
>
>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.
>

Like using functions that unwrap netlink attributes as unsigned when
they are in fact enums with possibility of being signed?

This is about consistent approach in ice driver.

>
>>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.
>

Flag allows to determine if dpll was successfully initialized and do proper
deinit on rmmod only if it was initialized. That's all.

>
>>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?

First of all I said it is not required.

I already draw you this with above sentence.
You need spam pin-get asynchronously and unload driver, what is not clear?
Basically mutex in dpll is a bottleneck, with multiple requests waiting for
mutex there is low change of driver getting mutex when doing unregisters.

We actually need to redesign the mutex in dpll core/netlink, but I guess after
initial submission.

Thank you!
Arkadiusz

>
>Thanks!
>
>
>>
>>>
>>>>+	mutex_destroy(&pf->dplls.lock);
>>>>+}
>
>
>[...]