linux-kernel - Re: [PATCH v2] net/r8169: Remove the firmware of RTL8111D

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] [day] [month] [year] [list]

Date:	Sun, 12 Dec 2010 04:25:59 +0000
From:	Ben Hutchings <benh@...ian.org>
To:	Hayes Wang <hayeswang@...ltek.com>
Cc:	romieu@...zoreil.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] net/r8169: Remove the firmware of RTL8111D

On Tue, 2010-12-07 at 15:19 +0000, Hayes Wang wrote:
> Remove the firmware of RTL8111D from the kernel.
> The binary file of firmware would be moved to linux-firmware repository.
> The firmwares are rtl_nic/rtl8168d-1.fw and rtl_nic/rtl8168d-2.fw.
> The driver would load the firmware through request_firmware. The driver
> would just go along if the firmware couldn't be found. However, it is
> suggested to be done with the suitable firmware.
> 
> Signed-off-by: Hayes Wang <hayeswang@...ltek.com>
> ---
>  drivers/net/r8169.c |  796 ++++++++-------------------------------------------
>  1 files changed, 117 insertions(+), 679 deletions(-)
>  mode change 100644 => 100755 drivers/net/r8169.c
> 
> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> old mode 100644
> new mode 100755

Please do not make source files executable.

> index 7d33ef4..e0df607
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c
[...]
> +static void
> +rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw)
> +{
> +	void __iomem *ioaddr = tp->mmio_addr;
> +	u32 *phytable = (u32 *)fw->data;

This line should use __le32 not u32 to make it clearer which byte order
is being used.

> +	u32 action;
> +	size_t len = fw->size / sizeof(*phytable);
> +	u32 regno, data;
> +
> +	while (len-- > 0 && *phytable != 0) {
> +		action = le32_to_cpu(*phytable);
> +		regno = (action & 0x0FFF0000) >> 16;
> +		data = action & 0x0000FFFF;
> +
> +		switch(action & 0xF0000000) {
> +		case PHY_WRITE:
> +			mdio_write(ioaddr, regno, data);
> +			phytable++;
> +			break;
> +		default:
> +			netif_err(tp, probe, tp->dev,
> +				  "Unknown action 0x%08x\n", action);
> +			return;
> +		}
> +	}
[...]

I think should validate the action types before starting.  Otherwise it
could begin loading the firmware and then abort (without even returning
an error code) which would be worse than not loading it at all.

Other than that, this is good, especially the addition of comments for
some of the register initialisation sequences.

Ben.

-- 
Ben Hutchings, Debian Developer and kernel team member


Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)