From: Arnd Bergmann <arnd@arndb.de> To: Boris Brezillon <boris.brezillon@bootlin.com> Cc: Wolfram Sang <wsa@the-dreams.de>, Linux I2C <linux-i2c@vger.kernel.org>, Jonathan Corbet <corbet@lwn.net>, "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>, gregkh <gregkh@linuxfoundation.org>, Przemyslaw Sroka <psroka@cadence.com>, Arkadiusz Golec <agolec@cadence.com>, Alan Douglas <adouglas@cadence.com>, Bartosz Folta <bfolta@cadence.com>, Damian Kos <dkos@cadence.com>, Alicja Jurasik-Urbaniak <alicja@cadence.com>, Cyprian Wronka <cwronka@cadence.com>, Suresh Punnoose <sureshp@cadence.com>, Rafal Ciepiela <rafalc@cadence.com>, Thomas Petazzoni <thomas.petazzoni@bootlin.com>, Nishanth Menon <nm@ti.com>, Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>, Mark Rutland <mark.rutland@arm.com>, Ian Campbell <ijc+devicetree@he> Subject: Re: [PATCH v9 6/9] i3c: master: Add driver for Cadence IP Date: Fri, 26 Oct 2018 12:01:52 +0200 [thread overview] Message-ID: <CAK8P3a0_En6e6ORvNqxprBcsT3+NmFO8nFnQ3nNiv-5Hq2LjrQ@mail.gmail.com> (raw) In-Reply-To: <20181026095707.3cd9b511@bbrezillon> On Fri, Oct 26, 2018 at 9:57 AM Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Fri, 26 Oct 2018 09:43:25 +0200 > Arnd Bergmann <arnd@arndb.de> wrote: > > > On Thu, Oct 25, 2018 at 6:30 PM Boris Brezillon > > <boris.brezillon@bootlin.com> wrote: > > > On Thu, 25 Oct 2018 18:13:51 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > > > On Thu, Oct 25, 2018 at 6:07 PM Boris Brezillon <boris.brezillon@bootlin.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Arnd Bergmann <arnd@arndb.de> To: Boris Brezillon <boris.brezillon@bootlin.com> Cc: Wolfram Sang <wsa@the-dreams.de>, Linux I2C <linux-i2c@vger.kernel.org>, Jonathan Corbet <corbet@lwn.net>, "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>, gregkh <gregkh@linuxfoundation.org>, Przemyslaw Sroka <psroka@cadence.com>, Arkadiusz Golec <agolec@cadence.com>, Alan Douglas <adouglas@cadence.com>, Bartosz Folta <bfolta@cadence.com>, Damian Kos <dkos@cadence.com>, Alicja Jurasik-Urbaniak <alicja@cadence.com>, Cyprian Wronka <cwronka@cadence.com>, Suresh Punnoose <sureshp@cadence.com>, Rafal Ciepiela <rafalc@cadence.com>, Thomas Petazzoni <thomas.petazzoni@bootlin.com>, Nishanth Menon <nm@ti.com>, Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>, Mark Rutland <mark.rutland@arm.com>, Ian Campbell <ijc+devicetree@hellion.org.uk>, Kumar Gala <galak@codeaurora.org>, DTML <devicetree@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Vitor Soares <Vitor.Soares@synopsys.com>, Geert Uytterhoeven <geert@linux-m68k.org>, Linus Walleij <linus.walleij@linaro.org>, Xiang Lin <Xiang.Lin@synaptics.com>, "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>, Sekhar Nori <nsekhar@ti.com>, Przemyslaw Gaj <pgaj@cadence.com>, Peter Rosin <peda@axentia.se>, Mike Shettel <mshettel@codeaurora.org>, Stephen Boyd <swboyd@chromium.org>, Joe Perches <joe@perches.com> Subject: Re: [PATCH v9 6/9] i3c: master: Add driver for Cadence IP Date: Fri, 26 Oct 2018 12:01:52 +0200 [thread overview] Message-ID: <CAK8P3a0_En6e6ORvNqxprBcsT3+NmFO8nFnQ3nNiv-5Hq2LjrQ@mail.gmail.com> (raw) In-Reply-To: <20181026095707.3cd9b511@bbrezillon> On Fri, Oct 26, 2018 at 9:57 AM Boris Brezillon <boris.brezillon@bootlin.com> wrote: > On Fri, 26 Oct 2018 09:43:25 +0200 > Arnd Bergmann <arnd@arndb.de> wrote: > > > On Thu, Oct 25, 2018 at 6:30 PM Boris Brezillon > > <boris.brezillon@bootlin.com> wrote: > > > On Thu, 25 Oct 2018 18:13:51 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > > > On Thu, Oct 25, 2018 at 6:07 PM Boris Brezillon <boris.brezillon@bootlin.com> 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
next prev parent reply other threads:[~2018-10-26 10:01 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-10-22 13:33 [PATCH v9 0/9] Add the I3C subsystem Boris Brezillon 2018-10-22 13:33 ` Boris Brezillon 2018-10-22 13:33 ` [PATCH v9 1/9] i3c: Add core I3C infrastructure Boris Brezillon 2018-10-22 13:33 ` Boris Brezillon 2018-10-22 13:33 ` [PATCH v9 2/9] docs: driver-api: Add I3C documentation Boris Brezillon 2018-10-22 13:33 ` Boris Brezillon 2018-10-22 13:33 ` [PATCH v9 3/9] i3c: Add sysfs ABI spec Boris Brezillon 2018-10-22 13:33 ` Boris Brezillon 2018-10-22 13:33 ` [PATCH v9 4/9] dt-bindings: i3c: Document core bindings Boris Brezillon 2018-10-22 13:33 ` Boris Brezillon 2018-10-22 20:45 ` Rob Herring 2018-10-22 20:45 ` Rob Herring 2018-10-22 13:34 ` [PATCH v9 5/9] MAINTAINERS: Add myself as the I3C subsystem maintainer Boris Brezillon 2018-10-22 13:34 ` Boris Brezillon 2018-10-22 13:34 ` [PATCH v9 6/9] i3c: master: Add driver for Cadence IP Boris Brezillon 2018-10-22 13:34 ` Boris Brezillon 2018-10-24 18:20 ` Boris Brezillon 2018-10-24 18:20 ` Boris Brezillon 2018-10-24 20:25 ` Grygorii Strashko 2018-10-24 20:25 ` Grygorii Strashko 2018-10-24 21:04 ` Boris Brezillon 2018-10-24 21:04 ` Boris Brezillon 2018-10-24 22:43 ` Grygorii Strashko 2018-10-24 22:43 ` Grygorii Strashko 2018-10-24 22:52 ` Boris Brezillon 2018-10-24 22:52 ` Boris Brezillon 2018-10-25 15:30 ` Arnd Bergmann 2018-10-25 15:30 ` Arnd Bergmann 2018-10-25 16:07 ` Boris Brezillon 2018-10-25 16:07 ` Boris Brezillon 2018-10-25 16:13 ` Arnd Bergmann 2018-10-25 16:13 ` Arnd Bergmann 2018-10-25 16:30 ` Boris Brezillon 2018-10-25 16:30 ` Boris Brezillon 2018-10-26 7:43 ` Arnd Bergmann 2018-10-26 7:43 ` Arnd Bergmann 2018-10-26 7:57 ` Boris Brezillon 2018-10-26 7:57 ` Boris Brezillon 2018-10-26 10:01 ` Arnd Bergmann [this message] 2018-10-26 10:01 ` Arnd Bergmann 2018-10-26 12:46 ` Boris Brezillon 2018-10-26 12:46 ` Boris Brezillon 2018-10-26 13:21 ` Arnd Bergmann 2018-10-26 13:21 ` Arnd Bergmann 2018-10-22 13:34 ` [PATCH v9 7/9] dt-bindings: i3c: Document Cadence I3C master bindings Boris Brezillon 2018-10-22 13:34 ` Boris Brezillon 2018-10-22 13:34 ` [PATCH v9 8/9] gpio: Add a driver for Cadence I3C GPIO expander Boris Brezillon 2018-10-22 13:34 ` Boris Brezillon 2018-10-22 13:34 ` [PATCH v9 9/9] dt-bindings: gpio: Add bindings for Cadence I3C gpio expander Boris Brezillon 2018-10-22 13:34 ` Boris Brezillon
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAK8P3a0_En6e6ORvNqxprBcsT3+NmFO8nFnQ3nNiv-5Hq2LjrQ@mail.gmail.com \ --to=arnd@arndb.de \ --cc=adouglas@cadence.com \ --cc=agolec@cadence.com \ --cc=alicja@cadence.com \ --cc=bfolta@cadence.com \ --cc=boris.brezillon@bootlin.com \ --cc=corbet@lwn.net \ --cc=cwronka@cadence.com \ --cc=dkos@cadence.com \ --cc=gregkh@linuxfoundation.org \ --cc=ijc+devicetree@he \ --cc=linux-doc@vger.kernel.org \ --cc=linux-i2c@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=nm@ti.com \ --cc=pawel.moll@arm.com \ --cc=psroka@cadence.com \ --cc=rafalc@cadence.com \ --cc=robh+dt@kernel.org \ --cc=sureshp@cadence.com \ --cc=thomas.petazzoni@bootlin.com \ --cc=wsa@the-dreams.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.