On Fri, Oct 26, 2018 at 9:57 AM Boris Brezillon
[off-list ref] wrote:

On Fri, 26 Oct 2018 09:43:25 +0200
Arnd Bergmann [off-list ref] wrote:

quoted

On Thu, Oct 25, 2018 at 6:30 PM Boris Brezillon
[off-list ref] wrote:

quoted

On Thu, 25 Oct 2018 18:13:51 +0200 Arnd Bergmann [off-list ref] wrote:
On Thu, Oct 25, 2018 at 6:07 PM Boris Brezillon [off-list ref] wrote:

quoted

quoted

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.

quoted

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.

quoted

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