Re: [PATCH v2 3/3] tty: n_gsm: add parameter negotiation support

[Ilpo Järvinen]

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.