Re: [RFC PATCH v7 5/8] ice: implement dpll interface to control cgu

Thu, May 18, 2023 at 06:06:03PM CEST, arkadiusz.kubalewski@intel.com wrote:

quoted

From: Jiri Pirko <jiri@resnulli.us>
Sent: Tuesday, May 16, 2023 8:26 AM

Tue, May 16, 2023 at 12:07:57AM CEST, arkadiusz.kubalewski@intel.com wrote:

quoted

quoted

From: Jiri Pirko <jiri@resnulli.us>
Sent: Wednesday, May 3, 2023 2:19 PM

Fri, Apr 28, 2023 at 02:20:06AM CEST, vadfed@meta.com wrote:

quoted

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

[...]

quoted

quoted

quoted

quoted

+			pins[i].pin = NULL;
+			return -ENOMEM;
+		}
+		if (cgu) {
+			ret = dpll_pin_register(pf->dplls.eec.dpll,
+						pins[i].pin,
+						ops, pf, NULL);
+			if (ret)
+				return ret;
+			ret = dpll_pin_register(pf->dplls.pps.dpll,
+						pins[i].pin,
+						ops, pf, NULL);
+			if (ret)
+				return ret;

You have to call dpll_pin_unregister(pf->dplls.eec.dpll, pins[i].pin, ..)
here.

No, in case of error, the caller releases everything

ice_dpll_release_all(..).


How does ice_dpll_release_all() where you failed? If you need to
unregister one or both or none? I know that in ice you have odd ways to
handle error paths in general, but this one clearly seems to be broken.

It doesn't have to, as release all would release all anyway.
Leaving it for now.

So you call dpll_pin_unregister() even for the pin that was not
registered before? How is that even remotely correct?

Fix your error paths, please. I don't understand the resistance here :)

[...]