linux-kernel - Re: [PATCH] Intel IXP4xx network drivers v.2

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]

Date:	Tue, 08 May 2007 16:31:12 +0200
From:	Krzysztof Halasa <khc@...waw.pl>
To:	Lennert Buytenhek <buytenh@...tstofly.org>
Cc:	Michael-Luke Jones <mlj28@....ac.uk>,
	Jeff Garzik <jeff@...zik.org>, netdev@...r.kernel.org,
	lkml <linux-kernel@...r.kernel.org>,
	Russell King <rmk@....linux.org.uk>,
	ARM Linux Mailing List 
	<linux-arm-kernel@...ts.arm.linux.org.uk>
Subject: Re: [PATCH] Intel IXP4xx network drivers v.2 - Ethernet and HSS

Lennert Buytenhek <buytenh@...tstofly.org> writes:

>> +/* Built-in 10/100 Ethernet MAC interfaces */
>> +static struct mac_plat_info ixdp425_plat_mac[] = {
>> +	{
>> +		.phy		= 0,
>> +		.rxq		= 3,
>> +	}, {
>> +		.phy		= 1,
>> +		.rxq		= 4,
>> +	}
>> +};
>
> As with Christian's driver (I'm feeling like a bit of a broken record
> here :-), putting knowledge of which queue to use (which is firmware-
> specific) in the _board_ support file is almost certainly wrong.
>
> I would just put the port number in there, and let the ethernet
> driver map the port number to the hardware queue number.  After all,
> the ethernet driver knows which queues the firmware uses, while the
> board support code doesn't.

No, quite the opposite. The board code knows its set of hardware
interfaces etc. and can let Ethernet driver use, say, HSS queues.
The driver can't know that.

It would make sense if we had many queues, but it doesn't seem
the case (perhaps the upper queues could be used for some
purposes, but Intel's code doesn't use them either and they
probably know better).

>> +static inline void set_regbits(u32 bits, u32 __iomem *reg)
>> +{
>> +	__raw_writel(__raw_readl(reg) | bits, reg);
>> +}
>> +static inline void clr_regbits(u32 bits, u32 __iomem *reg)
>> +{
>> +	__raw_writel(__raw_readl(reg) & ~bits, reg);
>> +}
>
> I generally discourage the use of such wrappers, as it often makes
> people forget that the set and clear operations are not atomic, and
> it ignores the fact that some of the other bits in the register you
> are modifying might have side-effects.

Without them the code in question is hardly readable, I pick the need
to remember about non-atomicity and possible side effects instead :-)

I've outlined the current versions in a separate mail, generally
2 patches are marked "v.2" and one "v.3".
-- 
Krzysztof Halasa
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/