[PATCH] ks8842: Add module param for setting mac address

[PATCH] ks8842: Add module param for setting mac address

[Richard Röjfors]

This patch adds a module parameter for setting the MAC address.

To ensure this MAC address is used, the MAC address is written
after each hardware reset.

Signed-off-by: Richard Röjfors <richard.rojfors@pelagicore.com>
---
diff --git a/drivers/net/ks8842.c b/drivers/net/ks8842.c
index 5c45cb5..a013662 100644
--- a/drivers/net/ks8842.c
+++ b/drivers/net/ks8842.c
@@ -1,5 +1,5 @@
 /*
- * ks8842_main.c timberdale KS8842 ethernet driver
+ * ks8842.c timberdale KS8842 ethernet driver
  * Copyright (c) 2009 Intel Corporation
  *
  * This program is free software; you can redistribute it and/or modify
@@ -119,6 +119,8 @@ struct ks8842_adapter {
 	struct platform_device *pdev;
 };
 
+static u8 __devinitdata macaddr[ETH_ALEN];
+
 static inline void ks8842_select_bank(struct ks8842_adapter *adapter, u16 bank)
 {
 	iowrite16(bank, adapter->hw_addr + REG_SELECT_BANK);
@@ -302,6 +304,20 @@ static void ks8842_read_mac_addr(struct ks8842_adapter *adapter, u8 *dest)
 	ks8842_write16(adapter, 39, mac, REG_MACAR3);
 }
 
+static void ks8842_write_mac_addr(struct ks8842_adapter *adapter, u8 *mac)
+{
+	unsigned long flags;
+	unsigned i;
+
+	spin_lock_irqsave(&adapter->lock, flags);
+	for (i = 0; i < ETH_ALEN; i++) {
+		ks8842_write8(adapter, 2, mac[ETH_ALEN - i - 1], REG_MARL + i);
+		ks8842_write8(adapter, 39, mac[ETH_ALEN - i - 1],
+			REG_MACAR1 + i);
+	}
+	spin_unlock_irqrestore(&adapter->lock, flags);
+}
+
 static inline u16 ks8842_tx_fifo_space(struct ks8842_adapter *adapter)
 {
 	return ks8842_read16(adapter, 16, REG_TXMIR) & 0x1fff;
@@ -520,6 +536,8 @@ static int ks8842_open(struct net_device *netdev)
 	/* reset the HW */
 	ks8842_reset_hw(adapter);
 
+	ks8842_write_mac_addr(adapter, netdev->dev_addr);
+
 	ks8842_update_link_status(netdev, adapter);
 
 	err = request_irq(adapter->irq, ks8842_irq, IRQF_SHARED, DRV_NAME,
@@ -567,10 +585,8 @@ static netdev_tx_t ks8842_xmit_frame(struct sk_buff *skb,
 static int ks8842_set_mac(struct net_device *netdev, void *p)
 {
 	struct ks8842_adapter *adapter = netdev_priv(netdev);
-	unsigned long flags;
 	struct sockaddr *addr = p;
 	char *mac = (u8 *)addr->sa_data;
-	int i;
 
 	dev_dbg(&adapter->pdev->dev, "%s: entry\n", __func__);
 
@@ -579,13 +595,7 @@ static int ks8842_set_mac(struct net_device *netdev, void *p)
 
 	memcpy(netdev->dev_addr, mac, netdev->addr_len);
 
-	spin_lock_irqsave(&adapter->lock, flags);
-	for (i = 0; i < ETH_ALEN; i++) {
-		ks8842_write8(adapter, 2, mac[ETH_ALEN - i - 1], REG_MARL + i);
-		ks8842_write8(adapter, 39, mac[ETH_ALEN - i - 1],
-			REG_MACAR1 + i);
-	}
-	spin_unlock_irqrestore(&adapter->lock, flags);
+	ks8842_write_mac_addr(adapter, mac);
 	return 0;
 }
 
@@ -604,6 +614,8 @@ static void ks8842_tx_timeout(struct net_device *netdev)
 
 	ks8842_reset_hw(adapter);
 
+	ks8842_write_mac_addr(adapter, netdev->dev_addr);
+
 	ks8842_update_link_status(netdev, adapter);
 }
 
@@ -627,6 +639,7 @@ static int __devinit ks8842_probe(struct platform_device *pdev)
 	struct net_device *netdev;
 	struct ks8842_adapter *adapter;
 	u16 id;
+	unsigned i;
 
 	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!request_mem_region(iomem->start, resource_size(iomem), DRV_NAME))
@@ -657,7 +670,20 @@ static int __devinit ks8842_probe(struct platform_device *pdev)
 	netdev->netdev_ops = &ks8842_netdev_ops;
 	netdev->ethtool_ops = &ks8842_ethtool_ops;
 
-	ks8842_read_mac_addr(adapter, netdev->dev_addr);
+	/* Check if a mac address was given */
+	for (i = 0; i < netdev->addr_len; i++)
+		if (macaddr[i] != 0)
+			break;
+
+	if (i < netdev->addr_len)
+		/* an address was passed, use it */
+		memcpy(netdev->dev_addr, macaddr, netdev->addr_len);
+	else {
+		ks8842_read_mac_addr(adapter, netdev->dev_addr);
+
+		if (!is_valid_ether_addr(netdev->dev_addr))
+			random_ether_addr(netdev->dev_addr);
+	}
 
 	id = ks8842_read16(adapter, 32, REG_SW_ID_AND_ENABLE);
 
@@ -723,6 +749,10 @@ static void __exit ks8842_exit(void)
 module_init(ks8842_init);
 module_exit(ks8842_exit);
 
+/* accept MAC address of the form macaddr=0x08,0x00,0x20,0x30,0x40,0x50 */
+module_param_array(macaddr, byte, NULL, 0);
+MODULE_PARM_DESC(macaddr, "KS8842 MAC address to set");
+
 MODULE_DESCRIPTION("Timberdale KS8842 ethernet driver");
 MODULE_AUTHOR("Mocean Laboratories <info@mocean-labs.com>");
 MODULE_LICENSE("GPL v2");

Re: [PATCH] ks8842: Add module param for setting mac address

[Ben Hutchings]

On Sun, 2010-04-18 at 19:25 +0200, Richard Röjfors wrote:
> This patch adds a module parameter for setting the MAC address.
> 
> To ensure this MAC address is used, the MAC address is written
> after each hardware reset.
[...]

This is not an accepted way of setting the MAC address.  The accepted
ways to initialise a network controller's address are:

1. a. Controller reads it from dedicated NVRAM.  Driver reads it from
      controller.
   b. Driver reads it from dedicated NVRAM.
2. Platform firmware or boot loader passes platform data (OpenFirmware,
   device tree, etc.) to the kernel, which includes the assigned MAC
   address.  Driver uses kernel functions to read it from platform data.
3. Platform firmware or boot loader programs it into the controller.
   Driver reads it from the controller.
4. Driver generates random address.

In any case, userland can change the MAC address later.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] ks8842: Add module param for setting mac address

[Richard Röjfors]

On 04/18/2010 09:50 PM, Ben Hutchings wrote:
> On Sun, 2010-04-18 at 19:25 +0200, Richard Röjfors wrote:
>> This patch adds a module parameter for setting the MAC address.
>>
>> To ensure this MAC address is used, the MAC address is written
>> after each hardware reset.
> [...]
>
> This is not an accepted way of setting the MAC address.

I agree it's not the cleanest way of doing this, I saw  some of the
other drivers do like this.

>  The accepted ways to initialise a network controller's address are:
>
> 1. a. Controller reads it from dedicated NVRAM.  Driver reads it from
>        controller.
>     b. Driver reads it from dedicated NVRAM.

Not possible with the current hardware.

> 2. Platform firmware or boot loader passes platform data (OpenFirmware,
>     device tree, etc.) to the kernel, which includes the assigned MAC
>     address.  Driver uses kernel functions to read it from platform data.

On our system (X86 based) the ks8842 is connected via a FPGA, the FPGA is
connected via PCI express. The system has a standard BIOS.
In linux we have a MFD driver chunking up the PCI memory space into
platform devices. In that case we would need to feed the data as a param
to the MFD driver which copies into the platform data of the ks8842, would
be doable.

> 3. Platform firmware or boot loader programs it into the controller.
>     Driver reads it from the controller.

We use standard BIOS and boot loaders from the X86 distros -> more or less
not doable.

> 4. Driver generates random address.

That's the current fallback if none is given and the random address
in the chip isn't valid.

>
> In any case, userland can change the MAC address later.

If we don't need to have a known MAC before the root FS is mounted.

--Richard

Re: [PATCH] ks8842: Add module param for setting mac address

[David Miller]

From: Richard Röjfors <richard.rojfors@pelagicore.com>
Date: Sun, 18 Apr 2010 19:25:57 +0200

> This patch adds a module parameter for setting the MAC address.
> 
> To ensure this MAC address is used, the MAC address is written
> after each hardware reset.
> 
> Signed-off-by: Richard Röjfors <richard.rojfors@pelagicore.com>

Your driver isn't unique, or special, and it doesn't need
to do special things to handle this situation.

Either probe the information from somewhere, or use a random
MAC address.

I'm not applying this.

Re: [PATCH] ks8842: Add module param for setting mac address

[Richard Röjfors]

On 04/19/2010 06:18 AM, David Miller wrote:
> From: Richard Röjfors<richard.rojfors@pelagicore.com>
> Date: Sun, 18 Apr 2010 19:25:57 +0200
>
>> This patch adds a module parameter for setting the MAC address.
>>
>> To ensure this MAC address is used, the MAC address is written
>> after each hardware reset.
>>
>> Signed-off-by: Richard Röjfors<richard.rojfors@pelagicore.com>
>
> Your driver isn't unique, or special, and it doesn't need
> to do special things to handle this situation.

I posted a new patch where the MAC address is passed via
platform data, hope that's an accepted way.

Thanks,
--Richard

Re: [PATCH] ks8842: Add module param for setting mac address

[David Miller]

From: Richard Röjfors <richard.rojfors@pelagicore.com>
Date: Mon, 19 Apr 2010 08:16:29 +0200

> I posted a new patch where the MAC address is passed via
> platform data, hope that's an accepted way.

I saw also that you mentioned that there is no real
way to probe this information.

Therefore I worry that your plan might be to simply
use some kernel command line or module option somewhere
else to fill in this platform_device value.

Is that what you plan to do?

Re: [PATCH] ks8842: Add module param for setting mac address

[Richard Röjfors]

On 04/19/2010 08:36 AM, David Miller wrote:
> From: Richard Röjfors<richard.rojfors@pelagicore.com>
> Date: Mon, 19 Apr 2010 08:16:29 +0200
>
>> I posted a new patch where the MAC address is passed via
>> platform data, hope that's an accepted way.
>
> I saw also that you mentioned that there is no real
> way to probe this information.
>
> Therefore I worry that your plan might be to simply
> use some kernel command line or module option somewhere
> else to fill in this platform_device value.
>
> Is that what you plan to do?

In the lab; yes.
In the end, there will be a flash available to store
system wide parameters.

--Richard

Re: [PATCH] ks8842: Add module param for setting mac address

[David Miller]

From: Richard Röjfors <richard.rojfors@pelagicore.com>
Date: Mon, 19 Apr 2010 08:46:58 +0200

> On 04/19/2010 08:36 AM, David Miller wrote:
>> From: Richard Röjfors<richard.rojfors@pelagicore.com>
>> Date: Mon, 19 Apr 2010 08:16:29 +0200
>>
>>> I posted a new patch where the MAC address is passed via
>>> platform data, hope that's an accepted way.
>>
>> I saw also that you mentioned that there is no real
>> way to probe this information.
>>
>> Therefore I worry that your plan might be to simply
>> use some kernel command line or module option somewhere
>> else to fill in this platform_device value.
>>
>> Is that what you plan to do?
> 
> In the lab; yes.
> In the end, there will be a flash available to store
> system wide parameters.

Ok, your long term plan is fine I guess.