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]

Message-ID: <20070508113735.GB32055@xi.wantstofly.org>
Date:	Tue, 8 May 2007 13:37:35 +0200
From:	Lennert Buytenhek <buytenh@...tstofly.org>
To:	Krzysztof Halasa <khc@...waw.pl>
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

On Tue, May 08, 2007 at 03:19:22AM +0200, Krzysztof Halasa wrote:

> diff --git a/arch/arm/mach-ixp4xx/ixdp425-setup.c b/arch/arm/mach-ixp4xx/ixdp425-setup.c
> index ec4f079..f20d39d 100644
> --- a/arch/arm/mach-ixp4xx/ixdp425-setup.c
> +++ b/arch/arm/mach-ixp4xx/ixdp425-setup.c
> @@ -101,10 +101,35 @@ static struct platform_device ixdp425_uart = {
>  	.resource		= ixdp425_uart_resources
>  };
>  
> +/* 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.

> +#ifndef __ARMEB__
> +#warning Little endian mode not supported
> +#endif

Yay. :-)  /me hides

> +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.

Didn't review the rest -- not sure whether I'm looking at the latest
version.
-
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/