Re: [PATCH v2 2/2] platform/x86: x86-android-tablets: Add Vexia EDU ATLA 10 EC battery driver

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2575 bytes --]

On Tue, 3 Dec 2024, Andy Shevchenko wrote:

> On Mon, Dec 2, 2024 at 11:48 PM Hans de Goede <hdegoede@redhat.com> 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.

Things will break spectacularly if such shenanigans are more than a myth.
It's not like they can change ABIs just like that without impacting real 
code. Essential networking headers such as tcp.h and ip.h do not use 
packed despite being literally wire formats. The day compiler people would 
break natural alignment means no padding, world will stop so chances of 
that happening are practically nil.

I admit though networking structs tend to be dword sized so Andy's 
point about sizeof() has more validity in it. My tests seemed to indicate 
it would be based on the alignment requirements of members within the 
struct so all __le16 would not need extra tail padding. But unlike the
natural alignment case, I've not seen the ABI/spec language about that so 
it seems acceptable in this case to use __packed, just in case.

-- 
 i.