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

[Jiri Pirko <jiri@resnulli.us>]

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.


>
>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'm tired of this. Keep your driver tangled for all I care, I'm trying
to help you, obviously you are not interested.


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


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

Why?


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