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

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri at resnulli.us>
>Sent: Wednesday, August 2, 2023 8:57 AM
>
>Tue, Aug 01, 2023 at 04:50:44PM CEST, arkadiusz.kubalewski at intel.com wrote:
>>>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?
>
>Looks this is bothering you, sorry about that.
>

Just poining out.

>
>>
>>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.
>
>You are not answering my question. I asked about how the flag helps is
>you do unregister dpll devices/pins and you free related resources in
>the correct order. Because that is why you claim you need this flag.
>

I do not claim such thing, actually opposite, I said it helps a bit
but the reason for existence is different, yet you are still trying to
imply me this.

>I'm tired of this. Keep your driver tangled for all I care, I'm trying
>to help you, obviously you are not interested.
>

With review you are doing great job and many thanks for that.

Already said it multiple times, the main reason of flag existence is not a
use in the callback but to determine successful dpll initialization.
As there is no need to call unregister on anything if it was not successfully
registered.

>
>>
>>>
>>>>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.
>
>How exactly your flag helps you in this scenario? It does not.
>

In this scenario it helps because it fails the callbacks when dpll subsystem
was partially initialized and callbacks can be already invoked, but in fact
the dpll initialization is not yet finished in the driver, and there will always
be the time between first and second dpll registration where we might wait for
the mutex to become available on dpll core part.

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

The global mutex for accessing the data works just fine, but it is slow.
Maybe we could improve this by using rwlock instead.

Thank you!
Arkadiusz

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