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

[Ben Hutchings]

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@xxxxxxxxxxx>
> ---
>  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

Attachment: signature.asc
Description: This is a digitally signed message part