Wed, Aug 02, 2023 at 05:48:43PM CEST, arkadiusz.kubalewski@intel.com wrote:

quoted

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:

quoted

quoted

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:

quoted

quoted

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:

quoted

From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

[...]

quoted

quoted

quoted

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

quoted

[...]

[...]

quoted

quoted

quoted

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

quoted

quoted

This is about consistent approach in ice driver.

quoted

quoted

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.

quoted

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.

quoted

quoted

quoted

quoted

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.

quoted

quoted

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

quoted

quoted

Thank you!
Arkadiusz

quoted

Thanks!

quoted

quoted

quoted

+	mutex_destroy(&pf->dplls.lock);
+}

[...]