On Mon, Dec 2, 2024 at 11:48 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> On 2-Dec-24 7:45 PM, Andy Shevchenko wrote:
> > On Mon, Dec 02, 2024 at 08:34:01PM +0200, Ilpo Järvinen wrote:
> >> On Sat, 16 Nov 2024, Hans de Goede wrote:
...
> >>> +struct atla10_ec_battery_state {
> >>> + u8 status; /* Using ACPI Battery spec status bits */
> >>> + u8 capacity; /* Percent */
Then an obvious remark based on Hans' reply, why are these internal
kernel types and not external ones, i.e. __u8?
> >>> + __le16 charge_now_mAh;
> >>> + __le16 voltage_now_mV;
> >>> + __le16 current_now_mA;
> >>> + __le16 charge_full_mAh;
> >>> + __le16 temp; /* centi degrees Celsius */
> >>> +} __packed;
> >>> +
> >>> +struct atla10_ec_battery_info {
> >>> + __le16 charge_full_design_mAh;
> >>> + __le16 voltage_now_mV; /* Should be design voltage, but is not ? */
> >>> + __le16 charge_full_design2_mAh;
> >>> +} __packed;
> >>
> >> Both struct have only naturally aligned members. Why is __packed needed?
> >
> > Wouldn't the second one give sizeof() == 8 rather than 6? Sorry, my memory
> > about this in C is always flaky.
>
> That might be one way how things could go wrong, yes.
>
> To answer Ilpo's original question: these structures represent
> the on wire format, hence also the __le16 use and the __packed
> is there to disable any possible compiler shenanigans.
>
> I basically always add __packed to structures representing
> hw memory / wire formats just to be sure.
--
With Best Regards,
Andy Shevchenko
