On Mon, Nov 02, 2020 at 11:03:47AM -0500, Tom Rini wrote:
> On Mon, Nov 02, 2020 at 11:56:07AM +1100, David Gibson wrote:
> > On Fri, Oct 30, 2020 at 02:56:58PM -0400, Tom Rini wrote:
> > > Start with commit 6dcb8ba4 "libfdt: Add helpers for accessing unaligned
> > > words" which introduced changes to support unaligned reads for ARM
> > > platforms and 11738cf01f15 "libfdt: Don't use memcpy to handle unaligned
> > > reads on ARM" to improve the performance of these helpers, libfdt has
> > > defaulted to assuming that unaligned memory access could be a fatal
> > > problem.
> > >
> > > Upon further discussion on the mailing list, we leave the improved
> > > unaligned-safe memory load functions available if needed, but go back to
> > > using fdt{32,64}_to_cpu() for access as generally platforms handle
> > > unaligned access safely and there is still a sizable performance and
> > > size impact of using the always-safe helpers in all cases.
> >
> > I think the basic change is ok, but there's some details I'd like to
> > see changed.
> >
> > First, I'd like to see an alignment check added to fdt_probe_ro_().
>
> OK, that's not hard. Do you want a new error code or FDT_ERR_INTERNAL
> or something else?
It definitely shouldn't be FDT_ERR_INTERNAL - that's always supposed
to indicate a bug within libfdt itself (basicaly anything that returns
FDT_ERR_INTERNAL wants to be an assert(), but actually using assert()
would pull in complex dependencies we don't want).
/me looks through the error list
Yeah, nothing there really suits, so we'll need to add a new one for
this.
> > > Signed-off-by: Tom Rini <trini@xxxxxxxxxxxx>
> > > ---
> > > fdtget.c | 2 +-
> > > libfdt/fdt_ro.c | 20 ++++++++++----------
> > > libfdt/libfdt.h | 2 +-
> > > 3 files changed, 12 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/fdtget.c b/fdtget.c
> > > index 777582e2d45f..7cee28718cbc 100644
> > > --- a/fdtget.c
> > > +++ b/fdtget.c
> > > @@ -62,7 +62,7 @@ static int show_cell_list(struct display_info *disp, const char *data, int len,
> > > for (i = 0; i < len; i += size, p += size) {
> > > if (i)
> > > printf(" ");
> > > - value = size == 4 ? fdt32_ld((const fdt32_t *)p) :
> > > + value = size == 4 ? fdt32_to_cpu(*(const fdt32_t *)p) :
> >
> > Second, getting rid of these ugly open-coded constructions was an
> > additional reason I went to using fdt32_ld() everywhere. So I'd like
> > to see both an assume-aligned and an unaligned-safe helper, rather
> > than open-coding the load-and-byteswap everywhere.
>
> OK, I think I've got an idea.
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
