On Fri, Oct 26, 2018 at 9:57 AM Boris Brezillon
<boris.brezillon@xxxxxxxxxxx> wrote:
> On Fri, 26 Oct 2018 09:43:25 +0200
> Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> > On Thu, Oct 25, 2018 at 6:30 PM Boris Brezillon
> > <boris.brezillon@xxxxxxxxxxx> wrote:
> > > On Thu, 25 Oct 2018 18:13:51 +0200 Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > > On Thu, Oct 25, 2018 at 6:07 PM Boris Brezillon <boris.brezillon@xxxxxxxxxxx> 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
