lkml.org 
[lkml]   [2022]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 3/3] tty: n_gsm: add parameter negotiation support
On Tue, 25 Oct 2022, Starke, Daniel wrote:

> > > + n1 = FIELD_GET(PN_N_FIELD_N1, le16_to_cpu(params->n_bits));
> >
> > Should this be using get_unaligned...()?
>
> Is this really necessary if the structure is already __packed? I did not
> receive any warning by the compiler.

It would be arch dependent to begin with. But honestly, I'm not entirely
certain here myself.

Documentation/core-api/unaligned-memory-access.rst claims compiler would
indeed do extra work to ensure access of unaligned member in a packed
struct is handled ok. But then you call le16_to_cpu() for the member field
which is full of cast magic so I'd be a bit hesitant to claim the
knowledge about the unalignment is carried all the way down there through
those casts.

Other subtle detail is the reply side struct which is allocated from stack
and with packed compiler is allowed (I don't know if it does that or not)
to make the struct unaligned as well (so perhaps put_unaligned would be
necessary there too if packed is retained).

If you want my recommendation, I'd just remove the packed altogether from
the struct because there seems to be no natural holes in it, use
get_unaligned for the receive side, and add this build time check:

static_assert(sizeof(struct gsm_dlci_param_bits) == 8);

If lkp builds all its current archs fine with that static_assert(), I'd be
pretty sure the struct that the unpacked struct is ok on all archs. Would
it ever stop being true on any arch/compiler setting, the assert would
catch it right away.


--
i.

\
 
 \ /
  Last update: 2022-10-25 13:51    [W:0.052 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site