netdev - Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4B5871F2.9090005@grandegger.com>
Date:	Thu, 21 Jan 2010 16:25:38 +0100
From:	Wolfgang Grandegger <wg@...ndegger.com>
To:	David Miller <davem@...emloft.net>
CC:	agust@...x.de, netdev@...r.kernel.org, dzu@...x.de, wd@...x.de,
	jcrigby@...il.com, kosmo@...ihalf.com, linuxppc-dev@...abs.org,
	grant.likely@...retlab.ca
Subject: Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to
 fs_enet driver

David Miller wrote:
> From: Anatolij Gustschin <agust@...x.de>
> Date: Thu, 21 Jan 2010 03:13:18 +0100
> 
>>  struct fec_info {
>> -	fec_t __iomem *fecp;
>> +	void __iomem *fecp;

To avoid confusion, the name "base_addr" seems more appropriate as it's
just used to calculate register offsets and for iomap/unmap.

>  ...
>>  /* write */
>> -#define FW(_fecp, _reg, _v) __fs_out32(&(_fecp)->fec_ ## _reg, (_v))
>> +#define FW(_regp, _reg, _v) __fs_out32((_regp)->fec_ ## _reg, (_v))
>  ...
>> +/* register address macros */
>> +#define fec_reg_addr(_type, _reg) \
>> +	(fep->fec.rtbl->fec_##_reg = (u32 __iomem *)((u32)fep->fec.fecp + \
>> +				(u32)&((__typeof__(_type) *)NULL)->fec_##_reg))
>> +
>> +#define fec_reg_mpc8xx(_reg) \
>> +	fec_reg_addr(struct mpc8xx_fec, _reg)
>> +
>> +#define fec_reg_mpc5121(_reg) \
>> +	fec_reg_addr(struct mpc5121_fec, _reg)
> 
> This is a step backwards in my view.
> 
> If you use the "fec_t __iomem *" type for the register
> pointer, you simply use &p->fecp->XXX to get the I/O
> address of register XXX and that's what you pass to
> the appropriate I/O accessor routines.
> 
> Now you've made it typeless, and then you have to walk
> through all of these contortions to get the offset.
> 
> I don't want to apply this, sorry...

The major problem that Anatolij tries to solve are the different
register layouts of the supported SOCs, MPC52xx and MPC8xx. They use the
same registers but at *different* offsets. Therefore we cannot handle
this with a single "fec_t" struct to allow building a single kernel
image. Instead it's handled by filling a table with register addresses:

	if (of_device_is_compatible(ofdev->node, "fsl,mpc5121-fec")) {
		fep->fec.fec_id = FS_ENET_MPC5121_FEC;
		fec_reg_mpc5121(ievent);
		fec_reg_mpc5121(imask);
		...
	} else {
		fec_reg_mpc8xx(ievent);
		fec_reg_mpc8xx(imask);
		...
	}

Do you see a more clever solution to this problem? Nevertheless, the
code could be improved by using "offsetof", I think.

Wolfgang.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html