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.1 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,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 74897C433DF for ; Wed, 8 Jul 2020 13:44:44 +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 30ACC206E9 for ; Wed, 8 Jul 2020 13:44:44 +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="sjlGBpKw" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 30ACC206E9 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=brQORkRQVoW86Vd2dwJ6DM7wxRjLg/Con/ir+NemjeY=; b=sjlGBpKweVZ5Xxs1QDcBWnnMP QDGeriGqr8E/YHIlmkHIDnbiGrDxEqgzBb+Y0LPxa/zSma4QJ7VdlZLh0Y0iUcSPKL5lMfV1qVPKO HhxiPi1hQoHjl8rdTyZCXSym+5oKovxcQrJJjINohybow45FwzjKoSDAmIkRbHoVWxH8j6aIZmRE8 9KojCCfY0v3XFvc7VT2k7CIvb5k06r09WMNVzU8i4s9YA3EqRBjMieDb+kxoZ9IoinueZ2+QJD2Kk ciXQRmvLJkEVND/lw47DdYQz6nowbAxhCBax/c1r+ALnPqX+6/THKb9IYlRzDionHx0FCl5JFStCv F/ERtbnFA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jtAKw-0001jt-SL; Wed, 08 Jul 2020 13:42:10 +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 1jtAKs-0001jQ-TZ for linux-arm-kernel@lists.infradead.org; Wed, 08 Jul 2020 13:42:08 +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 8B3911FB; Wed, 8 Jul 2020 06:42:01 -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 617E53F237; Wed, 8 Jul 2020 06:42:00 -0700 (PDT) Date: Wed, 8 Jul 2020 14:41:50 +0100 From: Cristian Marussi To: Arnd Bergmann Subject: Re: [GIT PULL] firmware: arm_scmi: updates for v5.9 Message-ID: <20200708134150.GA17585@e119603-lin.cambridge.arm.com> References: <20200706165336.40800-1-sudeep.holla@arm.com> <20200707080410.GC10953@bogus> <20200707083300.GA29665@e119603-lin.cambridge.arm.com> <20200707151240.GA16845@e119603-lin.cambridge.arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200707151240.GA16845@e119603-lin.cambridge.arm.com> 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-20200708_094207_063001_06119675 X-CRM114-Status: GOOD ( 56.16 ) 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: Kevin Hilman , SoC Team , ARM SoC Team , cristian.marussi@arm.com, Sudeep Holla , Olof Johansson , ALKML 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 On Tue, Jul 07, 2020 at 04:12:40PM +0100, Cristian Marussi wrote: > Hi Arnd, > > On Tue, Jul 07, 2020 at 11:37:47AM +0200, Arnd Bergmann wrote: > > On Tue, Jul 7, 2020 at 10:33 AM Cristian Marussi > > wrote: > > > 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. > > > > I would suggest first changing the structure to avoid the internal padding by > > moving the payld_sz ahead of the evt_id, or by changing evt_id and payld_sz > > to both to be u32 members. > > > > The size_t seemed mostly confusing because you went with the fixed-size 'u64' > > for the timestamp instead of using the abstract ktime_t type (which is > > also 64 bit > > but signed). > > In fact in an earlier iteration of this series I was using ktime_t, then I had automously > the brilliant (:D) idea of switching to u64: the (probably bogus) reason for this was that > I realized that ktime_t was signed and given that this timestamp represent the time, in > bootime ns, at which the event was received in the RX ISR, I thought that having 64 bits was > better than 63....but given that this field is not really even architected in the spec but > simply an implementation aid if the user wanted to measure the latency on his side I think > I can simply stick to ktime_t or use unsigned long at least. > > The reason behind the other fixed-size u8 fields in scmi_event_header is instead directly > 'inspired' by length of the events SCMI messages as defined by the current spec, but the real > underlying reason (as for the unneeded __packed) was to stick to the minimum spec-required > sizes so as to minimize the number of bytes to be memcopied in and out of the kfifo, since, > during the external review some remarks were raised about one other redundant memcpy that > would have led to an average unneeded copy of a dozen bytes on each transfer: so, after I > refactored the code to remove such memcpy, I thought was sensible to shrink the header as > much as possible to keep memcopied bytes to the minimum. > > Probably it's not worth the effort and I could just stick to unsigned char/int/long here > while still dropping the __packed. > > > > > > > > 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 ? > > > > Both, kind of. Again, using only u64/u32 types in a structure tells the reader > > that this is supposed to be a fixed structure layout even if that's not what > > you meant, but having implied padding makes it look like you got that wrong > > by accident. > > > > Ah, understood. > > > Using the abstract ktime_t here or 'unsigned int' or 'unsigned long' types > > would again make it clearer to a casual reader that this is a kernel-internal > > structure with no constraints on the layout, while adding a comment about > > padding or something like a 'u32 __pad' member would convey that you > > have thought about the layout and intentionally left it at that. > > > > I think these structures are more the second type, since they are in a > > header called scmi_protocol.h and passed around with an explicit > > length, so explicit padding is probably best. I would also try to avoid > > the 'bool' type for the same reason and make that a 'u8' member. > > > > Reviewing this u32/u64 usage here instead I think it's wrong; the whole reason > of event reports was to convey into the user callbacks some event-specific meaningful > information in a standard format about the events WITHOUT exposing directly the > protocol-message structure: so that a standard report is generated during the event-dispatch > from the raw content in scmi_evemt_header, possibly dropping any non user-interesting event > fields, and without the user-callback need to have knowledge/decoding of the protocol > internals but just the report layout. > > Using u32/u64 sized report-fields, inspired by the protocol-event-msg spec, is in fact at odd > with this idea and hurts back compatibility, since if an already released spec should change > in a some way, (even though we clearly know for sure that in our perfectly civilized > world this simply cannot happen...o_O), say changing one u16 field into a u8 (or viceversa) we > would be in trouble supporting this across possible multiple spec-versions, so maybe just > sticking here to a generic unsigned char/int/long also for the report fields would guard us > against near-future issues. Does it makes sense ? > > Not really a short reply...but thanks to have raised this point so I could rethink to all of this. > > Cristian > This 4 patches series posted now: https://lore.kernel.org/linux-arm-kernel/20200708122248.52771-1-cristian.marussi@arm.com/ Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-08 12:22 ` [PATCH 1/4] firmware: arm_scmi: Remove zero-length array in SCMI Notifications 2020-07-08 12:22 ` [PATCH 2/4] firmware: arm_scmi: Remove unneeded __packed attribute Cristian Marussi 2020-07-08 12:22 ` [PATCH 3/4] firmware: arm_scmi: Fix scmi_event_header fields typing Cristian Marussi 2020-07-08 12:22 ` [PATCH 4/4] firmware: arm_scmi: Remove fixed size typing from event reports Cristian Marussi is meant to address the above issues (hopefully) Thanks Cristian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel