Message-ID: <DM6PR11MB4657DAB437CE3760FC1CE8C89B09A@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Fri, 4 Aug 2023 08:58:05 +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: Thursday, August 3, 2023 10:03 AM
>
>Wed, Aug 02, 2023 at 05:48:43PM CEST, arkadiusz.kubalewski@...el.com wrote:
>>>From: Jiri Pirko <jiri@...nulli.us>
>>>Sent: Wednesday, August 2, 2023 8:57 AM
>>>
>>>Tue, Aug 01, 2023 at 04:50:44PM CEST, arkadiusz.kubalewski@...el.com
>>>wrote:
>>>>>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?
>>>
>>>Looks this is bothering you, sorry about that.
>>>
>>
>>Just poining out.
>>
>>>
>>>>
>>>>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.
>>>
>>>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.
>>
>>>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.
>

Ok, will do.

>
>>As there is no need to call unregister on anything if it was not
>successfully
>>registered.
>>
>>>
>>>>
>>>>>
>>>>>>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.
>

But I never said there is some issue, was saying from the beginning
"helping a bit" and "not required". Sorry I don't know how to draw this
other than above.

As agreed, will fix and use it only to deinit, let's move on, it is not
required :)

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

I mean serialized access to dpll is something that might be the issue in the
OS with multiple pins/devices and tools monitoring them, no hard data so far.


Thank you!
Arkadiusz

>
>>
>>Thank you!
>>Arkadiusz
>>
>>>
>>>>
>>>>Thank you!
>>>>Arkadiusz
>>>>
>>>>>
>>>>>Thanks!
>>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>+	mutex_destroy(&pf->dplls.lock);
>>>>>>>>+}
>>>>>
>>>>>
>>>>>[...]

Powered by blists - more mailing lists