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=-11.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable 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 8174DC433E1 for ; Mon, 13 Jul 2020 13:09:14 +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 4BF8120738 for ; Mon, 13 Jul 2020 13:09:14 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="k65PGzcQ" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4BF8120738 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=sBoVdMCl8BX0l2okjTQIddrKaTwCoN53l9VsYYdeFQQ=; b=k65PGzcQ0zU+IRoYC+OKXD7h/ fVhcr2k6TrWnCGR3hUDG/aA39lOGrU5xi0gUy8gqNKWs0SQM67qKY2WSjKwKTSQgOngPlB0Sdeqds jto37r31kPBeKtYCsJzWCxDu/jwoGwUn7ySrRvseG6GUZOuN6cmivjwo+ty2Sf18RipfdaEwudZA4 +7DVxaoiYCxX4LBVMbBU5oJqzGCJanhUodh3r7gi2vyr/QbLE0YYSGKkGqNQyzGAPjFMOz71v5uM9 mk1SVrpo6hjFQ3Nzc4/OlPHciTHwYqll1zZNE98ydY1NavZ/QIOlKJQH9Ms9WqCypKQp/FJnNV/hE 2TSNDBC9A==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1juyBZ-0004Og-53; Mon, 13 Jul 2020 13:07:57 +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 1juyBW-0004NS-Eh for linux-arm-kernel@lists.infradead.org; Mon, 13 Jul 2020 13:07:55 +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 C7C7630E; Mon, 13 Jul 2020 06:07:52 -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 EF5443F887; Mon, 13 Jul 2020 06:07:51 -0700 (PDT) Date: Mon, 13 Jul 2020 14:07:49 +0100 From: Cristian Marussi To: Steven Price Subject: Re: [PATCH 2/3] firmware: arm_scmi: Remove unneeded __packed attribute Message-ID: <20200713130749.GA31938@e119603-lin.cambridge.arm.com> References: <20200710133919.39792-1-cristian.marussi@arm.com> <20200710133919.39792-2-cristian.marussi@arm.com> <751ee628-ff38-a383-5832-aab4905af32b@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <751ee628-ff38-a383-5832-aab4905af32b@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-20200713_090754_597084_F0A1479B X-CRM114-Status: GOOD ( 20.61 ) 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: , Cc: arnd@arndb.de, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, sudeep.holla@arm.com Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Steven thanks for the review. On Mon, Jul 13, 2020 at 12:20:43PM +0100, Steven Price wrote: > On 10/07/2020 14:39, Cristian Marussi wrote: > >Remove __packed attribute from struct scmi_event_header. > > > >Signed-off-by: Cristian Marussi > = > A drive-by review. But this doesn't look safe to me. sizeof(struct > scmi_event_header) is used in several places and this change will modify > that from 13 to 16, but leave the structure members at the same offset (as > the members are naturally aligned). In particular the na=EFve header size= is > now bigger than the offset to payld. > = > What is the justification for __packed being 'unneeded'? > = Arnd pointed out at first that this structure in the original series had a = mix of fixed and non-fixed types in its fields and that the __packed rendered some= fields misaglined. Removing that as it is, in fact left also some unexplained implicit padding= which is at odd for a struct containing fixed-sized types. In a following fix in the series I have indeed moved this struct's fields = and others to generic non fixed type fields and shuffled around the fields to avoid mi= salignment and implicit internal padding (except for the trailing padding due to the v= ariable size array) It was probably better to squash also this patch in that following patch. This structure is used internally to push variable-sized (through the means= of the payld[]) events descriptors through a fifo from the ISR to the deferred workqueus, s= o that's whhy I originally thought to avoid to carry around unneeded padding into the fifos= and use the __packed. On the correctness side, as you pointed out, the header with padding is now= 16 so when I push thorugh the kfifos this header and the payload there's a hole in the= data as represented in the fifo buffer as such @end_of hdr+payld kfifo writes: kifo_in(fifo, h, sizeof(*h)) + kfifo_in(fifo, payld, h->payld_sz) 0 14 16 +-------+----+------------ |header - pad| payload... ------------------------- ^ | .payld (Note that header and payload comes from two distinct place so I have push = it with two kfifo_in() in order to avoid a redundant memcpy on an intermediate buffer to collate t= hem...thing that was pointed out as undesirable in a review) and when I read it back from the fifo such hole is just transparently overw= ritten: @header read: kfifo_out(fifo, h, sizeof(*h)) 0 14 16 +-------+----+-------------- |header - pad| = ---------------------------- @payload_read: kfifo_out(fifo, h->payld, h->payld_sz) 0 14 = +-------+----+-------------- |header | payload.... ---------------------------- ^ | .payld So since anyway the drawback of packing is that the misaglined access poten= tially slows down the reads, I was not sure anymore it was worth to pack and misalign, and, given= that it seemed not to be liked so much, I dropped it and moved to generic non-fixed types with= out packing. A better (and shorter) explanation of all of the above is possibly needed (= but I'd still prefer the fixed sized typing and __packed 'holeless' approach...) Thanks Cristian > Steve > = > >--- > > drivers/firmware/arm_scmi/notify.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > >diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_s= cmi/notify.c > >index c4d006cfde88..752415367305 100644 > >--- a/drivers/firmware/arm_scmi/notify.c > >+++ b/drivers/firmware/arm_scmi/notify.c > >@@ -258,7 +258,7 @@ struct scmi_event_header { > > u8 evt_id; > > size_t payld_sz; > > u8 payld[]; > >-} __packed; > >+}; > > struct scmi_registered_event; > > > = _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel