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 for Android: free password hash cracker in your pocket

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

Message-ID: <DM6PR11MB4657C0DA91583D92697324BC9B0AA@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Tue, 1 Aug 2023 14:50:44 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
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

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Monday, July 31, 2023 2:20 PM
>
>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.
>

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.

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

>
>>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!
>
>
>>
>>>
>>>>+	mutex_destroy(&pf->dplls.lock);
>>>>+}
>
>
>[...]