Skip to contentSkip to conversation

ks8842: Add module param for setting mac address

PATCH In review Richard Röjfors 3 participants 8 messages Apr 18, 2010 – Apr 19, 2010

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ks8842: Add module param for setting mac address

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.

---

drivers/net/ks8842.c +41 -11
index 5c45cb5..a013662 100644
@@ -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 <[email protected]>");
 MODULE_LICENSE("GPL v2");
#1 lore raw
Quoted 4 lines · 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.

[...]

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.

Signature · 3 lines
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.
#2 lore raw
Quoted 8 lines · Ben Hutchings
Quoted 4 lines · 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.

[...]

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 1 line

4. Driver generates random address.

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

Quoted 2 lines

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

maintainer
#3 lore raw
In reply to Richard Röjfors

From: Richard Röjfors <[email protected]>
Date: Sun, 18 Apr 2010 19:25:57 +0200

Quoted 6 lines

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.

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.

maintainer
#5 lore raw
In reply to Richard Röjfors

From: Richard Röjfors <[email protected]>
Date: Mon, 19 Apr 2010 08:16:29 +0200

Quoted 2 lines

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?

#4 lore raw
In reply to David Miller
Quoted 5 lines · David Miller

From: Richard Röjfors<[email protected]>
Date: Sun, 18 Apr 2010 19:25:57 +0200

Quoted 2 lines

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

Quoted 7 lines
Quoted 4 lines

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

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

#6 lore raw
In reply to David Miller
Quoted 6 lines · David Miller

From: Richard Röjfors<[email protected]>
Date: Mon, 19 Apr 2010 08:16:29 +0200

Quoted 2 lines

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

Quoted 8 lines

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

maintainer
#7 lore raw

From: Richard Röjfors <[email protected]>
Date: Mon, 19 Apr 2010 08:46:58 +0200

Quoted 14 lines · David Miller
Quoted 13 lines

From: Richard Röjfors<[email protected]>
Date: Mon, 19 Apr 2010 08:16:29 +0200

Quoted 2 lines

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.

Quoted 5 lines
Quoted 1 line

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.