Re: [PATCH 01/12] mfd: Eberspaecher Flexcard PMC II Carrier Board support

[Holger Dengler]

On 12/14/2016 09:38 AM, Arnd Bergmann wrote:
> On Wednesday, December 14, 2016 1:11:42 AM CET Holger Dengler wrote:
>>
>> diff --git a/include/uapi/linux/flexcard.h b/include/uapi/linux/flexcard.h
>> new file mode 100644
>> index 0000000..4e9f07b4
>> --- /dev/null
>> +++ b/include/uapi/linux/flexcard.h
>> @@ -0,0 +1,64 @@
> 
> Why is this exported to user space?

The registers of bar0 can be accessed from userspace via mmap and the structures in this header file describe the register layout in bar0.

>> +
>> +#include <linux/types.h>
>> +
>> +struct fc_version {
>> +	__u8	dev;
>> +	__u8	min;
>> +	__u8	maj;
>> +	__u8	reserved;
>> +} __packed;
> 
> The __packed attribute is redundant here as all members
> are just one byte anyway.

Both structures (struct fc_version and struct fc_bar0_conf) describe the layout of the registers in bar0. Therefore it must be guaranteed (on all architectures) that the compiler don't insert any spare-parts in the structures. My current understanding is, that the __packed attribute has to be used exactly for such a case. But I will check this again and if it is not required, I'll remove it.

> 
>> +/* PCI BAR 0: Flexcard configuration */
>> +struct fc_bar0_conf {
>> +	__u32 r1;			/* 000 */
>> +	struct fc_version fc_fw_ver;	/* 004 */
>> +	struct fc_version fc_hw_ver;	/* 008 */
>> +	__u32 r2[3];			/* 00c */
>> +	__u64 fc_sn;			/* 018 */
>> +	__u32 fc_uid;			/* 020 */
>> +	__u32 r3[7];			/* 024 */
>> +	__u32 fc_lic[6];		/* 040 */
>> +	__u32 fc_slic[6];		/* 058 */
>> +	__u32 trig_ctrl1;		/* 070 */
>> +	__u32 r4;			/* 074 */
>> +	__u32 trig_ctrl2;		/* 078 */
>> +	__u32 r5[22];			/* 07c */
>> +	__u32 amreg;			/* 0d4 */
>> +	__u32 tiny_stat;		/* 0d8 */
>> +	__u32 r6[5];			/* 0dc */
>> +	__u32 can_dat_cnt;		/* 0f0 */
>> +	__u32 can_err_cnt;		/* 0f4 */
>> +	__u32 fc_data_cnt;		/* 0f8 */
>> +	__u32 r7;			/* 0fc */
>> +	__u32 fc_rocr;			/* 100 */
>> +	__u32 r8;			/* 104 */
>> +	__u32 pg_ctrl;			/* 108 */
>> +	__u32 pg_term;			/* 10c */
>> +	__u32 r9;			/* 110 */
>> +	__u32 irs;			/* 114 */
>> +	__u32 fr_tx_cnt;		/* 118 */
>> +	__u32 irc;			/* 11c */
>> +	__u64 pcnt;			/* 120 */
>> +	__u32 r10;			/* 128 */
>> +	__u32 nmv_cnt;			/* 12c */
>> +	__u32 info_cnt;			/* 130 */
>> +	__u32 stat_trg_cnt;		/* 134 */
>> +	__u32 r11;			/* 138 */
>> +	__u32 fr_rx_cnt;		/* 13c */
>> +} __packed;
> 
> Here the __packed attribute is probably harmful, it prevents you
> from accessing the members using 32-bit sized accesses and forces
> bytewise accesses on some architectures, which tends to do the
> wrong thing on MMIO.

Is this also true, if the base address for this struct is always 32bit-alligned (because it is the base address of bar0)?

> 
> 	Arnd
> 

-- 
Gruß,
Holger Dengler
--
phone: +49 7556 25 999 14; fax: +49 7556 25 999 99

Attachment: signature.asc
Description: OpenPGP digital signature