Received: by 2002:ac0:aa62:0:0:0:0:0 with SMTP id w31-v6csp565963ima; Fri, 26 Oct 2018 03:02:52 -0700 (PDT) X-Google-Smtp-Source: AJdET5cn0jWjYk1ZWltmSzZbxh1bExL8vTP6NgVtWn+0s+TWfVYh9n/gX53p0sxYNAHtVk8jeMnF X-Received: by 2002:a65:60c9:: with SMTP id r9-v6mr2810105pgv.285.1540548172497; Fri, 26 Oct 2018 03:02:52 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1540548172; cv=none; d=google.com; s=arc-20160816; b=tQVM7E3S8sexF1rQOzGi5+3XF09KOAiH/1C2pvzh1vxAYIoUQzuAtX7CflSu8lIrBm K4CHjfse+9DKmstFaGS161LB85U6S2k2r077aFoYnDq0/jR5IH9ae70HLR8IWb7w3jEw Qv1OKlVm2PoViUNDbFHJGAj5qlzDW6gYJvKrZtp3Bzec0k/ffw5BwDGd7ZJN+teviEpF r9+t1OCKwE2u99Qdx/Z9G5hwcBWE7s+iZJx34O3Mzye0Cw6jdBZLrh0YRElD+u1SHbDu T1uJSuGKts7toJM+GNlaFDFXvpTAF8tSA+j9F0XdHeOMG+1oWz+/AfQTpucc66WfueWj x/kg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version; bh=Qqxi1PfP84Gp/aXmT/PseO8oQFHIbI2H9A+ahIcGjKc=; b=acFakpZWfdAd44NthTdkrItJA9GT9eQplxmcP8fmCUvve/lkCbUFgpOKwZMNEnY9ne 2EwrPzEvA9S7ivmfFy0tQYY1maE4C4UFZRfYvAqSAONRnmssIbwXEzTwa8ct6yoV6hIj LpVbBXd+BHpq5AanWNHA4vW0Hirq8Wdgi8XKKQ467axYSvmtrMOH270PWToTuvwliGM7 wz7BzG98V7EhWhS5gnfCndKDDq8Yf3bUsiLwdVE7OuZP2NZZEq3apVmjl8UPIgdHjQaO 7I22G94Odt1l3b9qR5oqgiFOhBPFyRYWfX/EtwZT1BdmhRFfmW7nqfOHgbFl0oSrbfXH HY4w== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id r19-v6si11129005pgj.438.2018.10.26.03.02.35; Fri, 26 Oct 2018 03:02:52 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727203AbeJZSih (ORCPT + 99 others); Fri, 26 Oct 2018 14:38:37 -0400 Received: from mail-qt1-f193.google.com ([209.85.160.193]:43932 "EHLO mail-qt1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726128AbeJZSih (ORCPT ); Fri, 26 Oct 2018 14:38:37 -0400 Received: by mail-qt1-f193.google.com with SMTP id q41-v6so519589qtq.10; Fri, 26 Oct 2018 03:02:09 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Qqxi1PfP84Gp/aXmT/PseO8oQFHIbI2H9A+ahIcGjKc=; b=XLgF5pBXNLOfePp16/ozBjMElUYxuG3UyCxqyqOuXQi2Bstx9mxy8w/CO+u/mhcDzv tA7vep6gprpmw/mKv+W69os14s2TbUhEj/i4iZUpgCigXuh9eA0pY166tz8Ji/d+3d5P MTBs/lDkwv7riakdo/XlCJtfPmMHSew7C1Mb19gA38IZqh8NNTU6ftghiBFfnUTKL5j2 7e4uM7DrYpkhrH7JmCGm5TLHDb95soLqARMx778FzwbP3Ekb1oezyRsdzKYAid2bvf7i 00cXWDPOec8KolVLKn6L2AM1j+FGI4MRexHzBRO9IzKCjTA0yIlbXL8NzG+q6gMYrB/C IB9w== X-Gm-Message-State: AGRZ1gJwKTVpF81/XKLuT8DtNZAPvXL3IukLDua5N2UB1WlDSFSUsMTQ GdOkVmdj/p2gWTVSKSPZRf5JwWSSRJxwDTEy8Gg= X-Received: by 2002:ac8:1d11:: with SMTP id d17-v6mr2433707qtl.343.1540548129294; Fri, 26 Oct 2018 03:02:09 -0700 (PDT) MIME-Version: 1.0 References: <20181022133404.2061-1-boris.brezillon@bootlin.com> <20181022133404.2061-7-boris.brezillon@bootlin.com> <20181024202048.7e3534f7@bbrezillon> <20181025180720.1790f6a1@bbrezillon> <20181025183005.3c0fa452@bbrezillon> <20181026095707.3cd9b511@bbrezillon> In-Reply-To: <20181026095707.3cd9b511@bbrezillon> From: Arnd Bergmann Date: Fri, 26 Oct 2018 12:01:52 +0200 Message-ID: Subject: Re: [PATCH v9 6/9] i3c: master: Add driver for Cadence IP To: Boris Brezillon Cc: Wolfram Sang , Linux I2C , Jonathan Corbet , "open list:DOCUMENTATION" , gregkh , Przemyslaw Sroka , Arkadiusz Golec , Alan Douglas , Bartosz Folta , Damian Kos , Alicja Jurasik-Urbaniak , Cyprian Wronka , Suresh Punnoose , Rafal Ciepiela , Thomas Petazzoni , Nishanth Menon , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , DTML , Linux Kernel Mailing List , Vitor Soares , Geert Uytterhoeven , Linus Walleij , Xiang Lin , "open list:GPIO SUBSYSTEM" , Sekhar Nori , Przemyslaw Gaj , Peter Rosin , Mike Shettel , Stephen Boyd , Joe Perches Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Oct 26, 2018 at 9:57 AM Boris Brezillon wrote: > On Fri, 26 Oct 2018 09:43:25 +0200 > Arnd Bergmann wrote: > > > On Thu, Oct 25, 2018 at 6:30 PM Boris Brezillon > > wrote: > > > On Thu, 25 Oct 2018 18:13:51 +0200 Arnd Bergmann wrote: > > > On Thu, Oct 25, 2018 at 6:07 PM Boris Brezillon wrote: > > > > > On Thu, 25 Oct 2018 17:30:26 +0200 > > Ok. Is i3c_master_send_ccc_cmd_locked() what implements the public > > interfaces then, or is this something else? > > i3c_master_send_ccc_cmd_locked() calls master->ops->send_ccc_cmd(), so > it's part of the master controller interface. > > > > > If you place a buffer on the stack, it is not DMA capable, but > > it is guaranteed to be at least 32-bit word aligned, and should > > not cause an exception in readsl(), unless it starts with a couple of > > (not multiple of four) extra bytes that are not sent to the devices. > > Is that what happens here? > > Here is the report I received from Vitor: > > " > Hi Boris, > > > I'm trying this new patch-set version but I get some issues when use > readsl() function. > > Basically the system complain about memory alignment. > > > +static int i3c_master_getpid_locked(struct i3c_master_controller *master, > > + struct i3c_device_info *info) > > +{ > > + struct i3c_ccc_getpid getpid; > > at this point the getpid struct it is already unaligned with > > i3c_master_getpid_locked:1129 getpid_add=0x9a249c7a > > > + struct i3c_ccc_cmd_dest dest = { > > + .addr = info->dyn_addr, > > + .payload.len = sizeof(struct i3c_ccc_getpid), > > + .payload.data = &getpid, > > + }; > > +} > > + > > and them when > > static void dw_i3c_master_read_rx_fifo(struct dw_i3c_master *master, > u8 *bytes, int nbytes) > { > readsl(master->regs + RX_TX_DATA_PORT, bytes, nbytes / 4); > ... > } Ok, I spent an hour chasing the ARM implementation and finding no way this could go wrong here. I see that 'struct i3c_ccc_getpid' may be misaligned on the stack (it normally won't be), and that the ARM readsl() has a lot of extra code to handle unaligned output. However, the dump that Vitor reports > [ECR ]: 0x00230400 => Misaligned r/w from 0x9a249c7a > [EFA ]: 0x9a249c7a > [BLINK ]: dw_i3c_master_irq_handler+0x200/0x2fc [dw_i3c_master] Is from an arch/arc kernel that uses asm-generic/io.h, and that stores the output using a u32 pointer: static inline void readsl(const volatile void __iomem *addr, void *buffer, unsigned int count) { if (count) { u32 *buf = buffer; do { u32 x = __raw_readl(addr); *buf++ = x; } while (--count); } } This is apparently not allowed on ARC when 'buffer' is unaligned. I think what we need here is to use put_unaligned() instead of the pointer dereference. For architectures that can do unaligned accesses, the result is the same, but for ARC it will fix the problem. > > One way to address this might be to always bounce any > > messages that are less than a cache line through a > > (pre-)kmallocated buffer, and require any longer messages > > to be cache capable. This could also solve the issue with > > readsl(), but it would be a rather confusing user interface. > > > > Another option might be to have separate interfaces for > > "short" and "long" messages at the API level and have > > distinct rules for those: short would always be bounced > > by the i3c code, and long puts restrictions on the buffer > > location. > > Hm, let's keep the API simple. I'll just mandate that all payload bufs > passed to i3c_master_send_ccc_cmd_locked() be dynamically allocated. Ok. What about i2c commands sent to the same i3c controller then? Do we need to copy those to satisfy the requirements of the i3c layer? Arnd