From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 78B1522DF95; Thu, 12 Jun 2025 10:31:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1749724268; cv=none; b=BX7OGBK3xLNgKUCfPPXqWTCtweNhkLwVBH1S6OJyr4noIjX1M+/Hjf/bZdtwHfsTQT95JQXyfzzfVIqOAktU8xiat83YkbEo4FV0yqJBtHnqTfTfkGbtgrM2Weqv+XO8I8ZXuwQeAnGktJVwZxpo+6m5HR3WrH2mXOJoVcdCgrI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1749724268; c=relaxed/simple; bh=TbqSp7G1dTO/lGn9z7FA7XGh/pw6w/fZljlWQsM6/2w=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ioEzpiXv6AuS3T/0uYrxbtLUc/Mcp4wjI3Z7zDyyVxrzou9TecW8g4VrR4t7iSo0MLKIu2DgG1/WivjPQ7h4UDFOFdOl9OzBXCBKJHAvoN6xxsmhwVnD8yqoLHOsKZs9NRouJji37BhlZM7/HC8OUKgjrIU2MZacn6bCwdVh4YY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 912B11424; Thu, 12 Jun 2025 03:30:45 -0700 (PDT) Received: from pluto (usa-sjc-mx-foss1.foss.arm.com [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 18DDD3F59E; Thu, 12 Jun 2025 03:31:03 -0700 (PDT) Date: Thu, 12 Jun 2025 11:31:01 +0100 From: Cristian Marussi To: Peng Fan Cc: Cristian Marussi , Sudeep Holla , "Rafael J. Wysocki" , Viresh Kumar , arm-scmi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, Peng Fan Subject: Re: [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform Message-ID: References: <20250611-scmi-perf-v1-0-df2b548ba77c@nxp.com> <20250611-cherubic-solemn-toucanet-aac5af@sudeepholla> <20250612034351.GA7552@nxa18884-linux> Precedence: bulk X-Mailing-List: linux-pm@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250612034351.GA7552@nxa18884-linux> On Thu, Jun 12, 2025 at 11:43:52AM +0800, Peng Fan wrote: > On Wed, Jun 11, 2025 at 02:33:37PM +0100, Cristian Marussi wrote: > >On Wed, Jun 11, 2025 at 01:17:11PM +0100, Sudeep Holla wrote: > >> On Wed, Jun 11, 2025 at 03:52:42PM +0800, Peng Fan (OSS) wrote: > >> > PERFORMANCE_NOTIFY_LIMITS and PERFORMANCE_NOTIFY_LEVEL are optional > >> > commands. If use these commands on platforms that not support the two, > >> > there is error log: > >> > SCMI Notifications - Failed to ENABLE events for key:13000008 ! > >> > scmi-cpufreq scmi_dev.4: failed to register for limits change notifier for domain 8 > >> > > >> > > > >Hi, > > > >I had a quick look/refresh to this stuff from years ago... > > > >...wont be so short to explain :P > > > >In general when you register a notifier_block for some SCMI events, > >the assumption was that a driver using proto_X_ops could want to register > >NOT only for proto_X events BUT also for other protos...in such a case you > >are NOT guaranteed that such other proto_Y was initialized when your > >driver probes and tries to register the notifier...indeed it could be > >that such proto_Y could be a module that has still to be loaded ! > > > >...in this scenario you can end-up quickly in a hell of probe-dependency > >if you write a driver asking for SCMI events that can or cannot be still > >readily available when the driver probes... > > > >....so the decision was to simply place such notifier registration requests > >on hold on a pending list...whenever the needed missing protocol is > >loaded/inialized the notifier registration is completed...if the proto_Y > >never arrives nothing happens...and your driver caller can probe > >successfully anyway... > > > >This means in such a corner-case the notifier registration is sort of > >asynchonous and eventual errors detected later, when the protocol is > >finally initialized and notifiers finalized, cannot be easily reported > >(BUT I think we could improve on this ... thinking about this...) > > > >...BUT.... > > > >....this is NOT our case NOR the most common case...the usual scenario, > >like cpufreq, is that a driver using proto_X_ops tries to register for > >that same proto_X events and in such a case we can detect that such > >domain is unsupported and fail while avoiding to send any message indeed.... > > > >....so....:P...while I was going through this rabbit-hole....this issues > >started to feel familiar...O_o.... > > > >... indeed I realized that the function that you (Peng) now invoke to > >set the per-domain perf_limit_notify flag was introduced just for these > >reasons to check and avoid such situation for all protocols in the core: > > > > > >commit 8733e86a80f5a7abb7b4b6ca3f417b32c3eb68e3 > >Author: Cristian Marussi > >Date: Mon Feb 12 12:32:23 2024 +0000 > > > > firmware: arm_scmi: Check for notification support > > > > When registering protocol events, use the optional .is_notify_supported > > callback provided by the protocol to check if that specific notification > > type is available for that particular resource on the running system, > > marking it as unsupported otherwise. > > > > Then, when a notification enable request is received, return an error if > > it was previously marked as unsuppported, so avoiding to send a needless > > notification enable command and check the returned value for failure. > > > > Signed-off-by: Cristian Marussi > > Link: https://lore.kernel.org/r/20240212123233.1230090-2-cristian.marussi@arm.com > > Signed-off-by: Sudeep Holla > > > > > >...so my suspect is that we are ALREADY avoiding to send unneeded > >messages when a domain does NOT support notifications for ALL > >protocols...it is just that we are a bit too noisy... > > > >@Peng do you observe the message being sent instead ? (so maybe the > >above has a bug...) or it is just the message ? > > Just the message. > > arm-scmi arm-scmi.0.auto: SCMI Notifications - Notification NOT supported - proto_id:19 evt_id:0 src_id:8 > SCMI Notifications - Failed to ENABLE events for key:13000008 ! > scmi-cpufreq scmi_dev.4: failed to register for limits change notifier for domain 8 > > It just make user has a feeling that there must be something wrong, especially > those not know the internals. Yes indeed it is too much noisy... > > And from the error message, "Failed to ENABLE events for key..", we not > know which protocol, and whether notification supported. > > I was thinking to propogate the error value, but __scmi_enable_evt > always use -EINVAL if not success. > I suppose, if you want also to save cycles that you could mark internally a protocol, at init time, as NOT suporting notifs (if you can detect that no domain is supported OR the related notfs commands are NOT available) and then bailing out early with -ENOTOPSUPP when trying to register a new notifier (amd muting all the errs to dbgs....) so that the caller can warn if wanted or not... > > > >> I wonder if it makes sense to quiesce the warnings from the core if the > >> platform doesn't support notifications. > > If not quiesce, we might need to make it clear from the error message, > saying whether X events are supported for Y protocols or not, not just > a "Failed to ENABLE events for key.." > Yes that was a very early and primitve errors message of mine...my bad :D Thanks, Cristian