Re: [PATCH wireless-next v2 06/31] wifi: mm81x: add core.h

[Lachlan Hodges]

> > +static inline u32 mm81x_fle32_to_cpu(u32 v)
> > +{
> > +	return le32_to_cpu((__force __le32)v);
> > +}
> > +
> > +static inline u16 mm81x_fle16_to_cpu(u16 v)
> > +{
> > +	return le16_to_cpu((__force __le16)v);
> > +}
> 
> The whole __force thing here seems odd, why isn't the input 'v' just
> __le16?
> 
> This goes with all the FW loader thing - but that also has all __force.
> I'd argue it'd be better to just have separate FW-endian (little endian)
> and host-endian structures, even if that duplicates the structure
> definitions, but it'll have sparse actually checking the fields were all
> converted correctly rather than casting a little endian structure to
> host endian and then doing the conversions:
> 
>                 mm81x_elf_phdr *p = (mm81x_elf_phdr *)(fw->data + ehdr.e_phoff +
>                                                        i * ehdr.e_phentsize);
> 
>                 phdr.p_type = le32_to_cpu((__force __le32)p->p_type);

Having separate structs for the hw structs definitely seems better:

	struct mm81x_yaps_hw_status_registers status_regs;
	struct mm81x_yaps_hw_status_registers_le status_regs_le;

However, since we have to do some elf parsing, I assume due to
the size of all the S1G reg rules, it seems best to do something
similar but it means we have to define our own le elf structs:

#define mm81x_elf_ehdr Elf32_Ehdr
#define mm81x_elf_shdr Elf32_Shdr
#define mm81x_elf_phdr Elf32_Phdr

struct mm81x_elf32_ehdr_le {
	unsigned char e_ident[EI_NIDENT];
	__le16 e_type;
	__le16 e_machine;
	__le32 e_version;
	__le32 e_entry;
	__le32 e_phoff;
	__le32 e_shoff;
	__le32 e_flags;
	__le16 e_ehsize;
	__le16 e_phentsize;
	__le16 e_phnum;
	__le16 e_shentsize;
	__le16 e_shnum;
	__le16 e_shstrndx;
} __packed;

...

since the kernel (AFAICT) only has a single host endian structure.
However I am a bit cautious about redefining elf sections here since
I cannot find much precedent in the tree. it does mean we don't need
to use __force. Of course we could also do what you wrote above and
convert in-place using __force to the host endian struct. Do you have
any preferences here? Seems most wireless drivers have much simpler
TLV followed by blob firmware loaders.

lachlan