Re: Misaligned Access

Re: Misaligned Access

[Vineet Gupta]

+CC lkml, Arnd : subject matter expert

On 11/21/18 10:06 AM, Vitor Soares wrote:
> I use the follow function to get data from a RX Fifo.
>
>
> 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);

So the semantics are reading the same fifo register N times, to get the N words,
hence read*s*l is appropriate. That however expects the buffer to be 4 bytes
aligned, hence your issue. You can't possibly use the reads*b* as we want the

The obvious but crude hack is to use a temp array for readsl and then copy over
using memcpy, but I'm sure there are better ways, @Arnd ? To summarize is issue is
a driver triggering unaligned access due to the misinteraction of API (driver get
an unaligned u8 *) which goes against expectations of io accessor readl  (needed
since the register contents are 4 bytes)


>      if (nbytes & 3) {
>          u32 tmp;
>
>          readsl(master->regs + RX_TX_DATA_PORT, &tmp, 1);
>          memcpy(bytes + (nbytes & ~3), &tmp, nbytes & 3);
>      }
> }
>
>
> and the pointer u8 *bytes is what is unaligned and breaks when inside of 
> realdsl() it does:
>
>      *buf++ = x;
>
>
> Note that the u8 *bytes pointer is the __u8 *buf of the i2c_msg struct.

Re: Misaligned Access

[Arnd Bergmann]

On Wed, Nov 21, 2018 at 8:42 PM Vineet Gupta <vineet.gupta1@synopsys.com> wrote:
>
> +CC lkml, Arnd : subject matter expert
>
> On 11/21/18 10:06 AM, Vitor Soares wrote:
> > I use the follow function to get data from a RX Fifo.
> >
> >
> > 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);
>
> So the semantics are reading the same fifo register N times, to get the N words,
> hence read*s*l is appropriate. That however expects the buffer to be 4 bytes
> aligned, hence your issue. You can't possibly use the reads*b* as we want the
>
> The obvious but crude hack is to use a temp array for readsl and then copy over
> using memcpy, but I'm sure there are better ways, @Arnd ? To summarize is issue is
> a driver triggering unaligned access due to the misinteraction of API (driver get
> an unaligned u8 *) which goes against expectations of io accessor readl  (needed
> since the register contents are 4 bytes)

Is this again on ARC or some other architecture that cannot do unaligned
access to normal RAM? On ARMv7 or x86, you should never see a problem
because the CPU handles misaligned writes. On ARMv4/v5, the readsl()
implementation internally aligns the access to the output buffer so it
will work correctly.

      Arnd

Re: Misaligned Access

[Vineet Gupta]

On 11/21/18 12:12 PM, Arnd Bergmann wrote:
> On Wed, Nov 21, 2018 at 8:42 PM Vineet Gupta <vineet.gupta1@synopsys.com> wrote:
>> +CC lkml, Arnd : subject matter expert
>>
>> On 11/21/18 10:06 AM, Vitor Soares wrote:
>>> I use the follow function to get data from a RX Fifo.
>>>
>>>
>>> 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);
>> So the semantics are reading the same fifo register N times, to get the N words,
>> hence read*s*l is appropriate. That however expects the buffer to be 4 bytes
>> aligned, hence your issue. You can't possibly use the reads*b* as we want the
>>
>> The obvious but crude hack is to use a temp array for readsl and then copy over
>> using memcpy, but I'm sure there are better ways, @Arnd ? To summarize is issue is
>> a driver triggering unaligned access due to the misinteraction of API (driver get
>> an unaligned u8 *) which goes against expectations of io accessor readl  (needed
>> since the register contents are 4 bytes)
> Is this again on ARC or some other architecture that cannot do unaligned
> access to normal RAM? On ARMv7 or x86, you should never see a problem
> because the CPU handles misaligned writes. On ARMv4/v5, the readsl()
> implementation internally aligns the access to the output buffer so it
> will work correctly.

This is indeed on ARC: on ARC700 unaligned access to RAM was never supported and
on HS38x cores, it is configurable, so the API probably needs to support both cases.

Thx,
-Vineet

Re: Misaligned Access

[Arnd Bergmann]

On Wed, Nov 21, 2018 at 9:15 PM Vineet Gupta <vineet.gupta1@synopsys.com> wrote:
>
> On 11/21/18 12:12 PM, Arnd Bergmann wrote:
> > On Wed, Nov 21, 2018 at 8:42 PM Vineet Gupta <vineet.gupta1@synopsys.com> wrote:
> >> +CC lkml, Arnd : subject matter expert
> >>
> >> On 11/21/18 10:06 AM, Vitor Soares wrote:
> >>> I use the follow function to get data from a RX Fifo.
> >>>
> >>>
> >>> 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);
> >> So the semantics are reading the same fifo register N times, to get the N words,
> >> hence read*s*l is appropriate. That however expects the buffer to be 4 bytes
> >> aligned, hence your issue. You can't possibly use the reads*b* as we want the
> >>
> >> The obvious but crude hack is to use a temp array for readsl and then copy over
> >> using memcpy, but I'm sure there are better ways, @Arnd ? To summarize is issue is
> >> a driver triggering unaligned access due to the misinteraction of API (driver get
> >> an unaligned u8 *) which goes against expectations of io accessor readl  (needed
> >> since the register contents are 4 bytes)
> > Is this again on ARC or some other architecture that cannot do unaligned
> > access to normal RAM? On ARMv7 or x86, you should never see a problem
> > because the CPU handles misaligned writes. On ARMv4/v5, the readsl()
> > implementation internally aligns the access to the output buffer so it
> > will work correctly.
>
> This is indeed on ARC: on ARC700 unaligned access to RAM was never supported and
> on HS38x cores, it is configurable, so the API probably needs to support both cases.

Ok, then I think you need to overwrite readsl() with a variant that
uses put_unaligned() instead the plain pointer dereference for the
output.

      Arnd