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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZMem35OUQiQmB9Vd@nanopsycho>
Date: Mon, 31 Jul 2023 14:19:43 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
Cc: Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
	Jakub Kicinski <kuba@...nel.org>,
	Jonathan Lemon <jonathan.lemon@...il.com>,
	Paolo Abeni <pabeni@...hat.com>,
	"Olech, Milena" <milena.olech@...el.com>,
	"Michalik, Michal" <michal.michalik@...el.com>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
	poros <poros@...hat.com>, mschmidt <mschmidt@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
	Bart Van Assche <bvanassche@....org>
Subject: Re: [PATCH 09/11] ice: implement dpll interface to control cgu

Sat, Jul 29, 2023 at 01:03:59AM CEST, arkadiusz.kubalewski@...el.com wrote:
>>From: Jiri Pirko <jiri@...nulli.us>
>>Sent: Friday, July 21, 2023 1:39 PM
>>
>>Thu, Jul 20, 2023 at 11:19:01AM CEST, vadim.fedorenko@...ux.dev wrote:
>>>From: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.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.


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


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

Thanks!


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


[...]