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?

This is about consistent approach in ice driver.

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.

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.

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

Thank you!
Arkadiusz

Thanks!

quoted

quoted

quoted

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

[...]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel