From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.0 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PULL_REQUEST,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4B0A2C433E0 for ; Tue, 7 Jul 2020 08:35:28 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 156202063A for ; Tue, 7 Jul 2020 08:35:27 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="X6ch0Shv" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 156202063A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=j19fspxqeI79nebnzc0CBbBKzyLXYO6oMkdonmh92CE=; b=X6ch0ShvYLs1faF1x0fHjp/tN teAaJDwWaYfjjEkxlxl63/GYIwVicR2O8m9Id0dmJtQ1+TyT2YEIvyG5EzKg9MPB/QD8jBwauuxVD WKSPczlPCBYXsAr53C5WQvHtyWgudSrHx5Fs5x6YxyVBBI7dS68nxyl66LbZwHb5WHFsD5SrQSbAp +lGVxyzaiDSaTiY7cdFNQ5uu6UPhNba6pXQurvaR88mQLPIuVQqKcZfrAk5siPqIPTPfFYtJQexog ec3NNLxaEozQxeoyTwEAxoJp6VF1Wu7cRMCOeHJ4MDwYOJTCthGaJ5UZgUlb5R4T0/0yLv0yHA1Af vTkNie0NA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jsj2w-0002I5-HJ; Tue, 07 Jul 2020 08:33:46 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jsj2t-0002Hh-8q for linux-arm-kernel@lists.infradead.org; Tue, 07 Jul 2020 08:33:44 +0000 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 5109531B; Tue, 7 Jul 2020 01:33:36 -0700 (PDT) Received: from e119603-lin.cambridge.arm.com (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 4438A3F68F; Tue, 7 Jul 2020 01:33:35 -0700 (PDT) Date: Tue, 7 Jul 2020 09:33:24 +0100 From: Cristian Marussi To: Sudeep Holla , arnd@arndb.de Subject: Re: [GIT PULL] firmware: arm_scmi: updates for v5.9 Message-ID: <20200707083300.GA29665@e119603-lin.cambridge.arm.com> References: <20200706165336.40800-1-sudeep.holla@arm.com> <20200707080410.GC10953@bogus> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200707080410.GC10953@bogus> User-Agent: Mutt/1.5.24 (2015-08-30) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200707_043343_440830_3F7D560D X-CRM114-Status: GOOD ( 34.18 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , List-Id: Cc: Olof Johansson , SoC Team , ARM SoC Team , ALKML , Kevin Hilman Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi On Tue, Jul 07, 2020 at 09:04:10AM +0100, Sudeep Holla wrote: > Hi Arnd, > > On Mon, Jul 06, 2020 at 09:23:46PM +0200, Arnd Bergmann wrote: > > On Mon, Jul 6, 2020 at 6:53 PM Sudeep Holla wrote: > > > > > > The following changes since commit b3a9e3b9622ae10064826dccb4f7a52bd88c7407: > > > > > > Linux 5.8-rc1 (2020-06-14 12:45:04 -0700) > > > > > > are available in the Git repository at: > > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git tags/scmi-updates-5.9 > > > > > > for you to fetch changes up to 585dfab3fb80e67b3a54790b3d5ef2991feb3950: > > > > > > firmware: arm_scmi: Add base notifications support (2020-07-01 17:07:26 +0100) > > > > > > ---------------------------------------------------------------- > > > ARM SCMI/SCPI updates for v5.9 > > > > > > The main addition for this time is the support for platform notifications. > > > SCMI protocol specification allows the platform to signal events to the > > > interested agents via notification messages. We are adding support for > > > the dispatch and delivery of such notifications to the interested users > > > inside the kernel. > > > > > > Other than that, there are minor changes like checking and using the > > > fast_switch capability quering the firmware instead of doing it > > > unconditionally(using polling mode transfer), cosmetic trace update and > > > use of HAVE_ARM_SMCCC_DISCOVERY instead of ARM_PSCI_FW. > > > > I haven't pulled this yet, as I noticed one data structure definition that > > seems very odd: > > > > struct scmi_event_header { > > u64 timestamp; > > u8 evt_id; > > size_t payld_sz; > > u8 payld[]; > > } __packed; > > > > This is an odd mix of fixed-length fields (u64) and variable length > > fields (size_t is different on 32-bit machines), out of which at least > > one is misaligned because of the __packed attribute. > > > > Agreed, my mistake. I did mention to get rid of __packed in earlier version > and completely missed to observe in later versions. > This structure is used only internally to the SCMI Notifications machinery to describe and push the events from the RX ISR to the deferred worked through the kfifos, so the size_t seemed a good fit given it represents a length and the struct is just an internal helper. The reason for the unneeded __packed was because it seemed odd to me to push around padding through the internal fifos, but it is not needed and it would hurt perfomance indeed due to the forced misalignment: I'll drop it. > > There are others that are just slightly odd: > > > > struct scmi_reset_issued_report { > > u64 timestamp; > > u32 agent_id; > > u32 domain_id; > > u32 reset_state; > > /* four bytes padding */ > > }; > > > > struct scmi_perf_level_report { > > u64 timestamp; > > u32 agent_id; > > u32 domain_id; > > u32 performance_level; > > /* four bytes padding */ > > }; > > > > struct scmi_base_error_report { > > u64 timestamp; > > u32 agent_id; > > bool fatal; > > /* 1 byte padding */ > > u16 cmd_count; > > u64 reports[0]; > > }; > > > > as this includes four implied padding bytes at the end. I could not figure > > out exactly what the guarantees for interface stability on either of > > them are, but if these get passed between the kernel and some other > > code (firmware or user space), or might be in the future, then I'd suggest > > redefining them in a way that avoids those oddities. > > > > These structures are not shared across kernel and userspace/firmware. It > is entirely constructed by kernel for other users within kernel. > > Cristian, correct me if I am wrong ? Or add more info/clarity if it > helps the discussion here. Correct, these structs are just common per-event descriptors built by the notifications core while dispatching events and passed to the interested users (SCMI drivers) as an argument to their notifier_block registered callback to provide specific info about the received event. Not sure though, Arnd, if you added the padding comments above just to highlight the possible issue here or if you want them added ? By the way there's also a residual zero-lenght array definition in base report, I'll remove that too. > > > Once this has been clarified, please just add any further patches > > (if needed) on top of the existing branch and send a new pull request. > > > > Thanks > Thanks Cristian > -- > Regards, > Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel