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

[Jiri Pirko <jiri@resnulli.us>]

Wed, Aug 02, 2023 at 05:48:43PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Wednesday, August 2, 2023 8:57 AM
>>
>>Tue, Aug 01, 2023 at 04:50:44PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>>>From: Jiri Pirko <jiri@resnulli.us>
>>>>Sent: Monday, July 31, 2023 2:20 PM
>>>>
>>>>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.
>>>>
>>>
>>>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.

So use it only for this, nothing else. Use it only to check during
cleanup that you need to do the cleanup as init was previously done.


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

Draw it to me, please, where exatly there is a problem. I'm still
convinced that with the proper ordering of init/cleanup flows,
you'll get all you need, without any flag use.


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

"it is slow" is quite vague description of what's wrong with the
locking.


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