linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
@ 2012-01-14  6:44 Darren Hart
  2012-01-14  8:14 ` David Miller
  2012-01-14  8:15 ` David Miller
  0 siblings, 2 replies; 23+ messages in thread
From: Darren Hart @ 2012-01-14  6:44 UTC (permalink / raw)
  To: Linux Kernel Mailing List
  Cc: Darren Hart, Arjan van de Ven, Alan Cox, Tomoya MORINAGA,
	Jeff Kirsher, David S. Miller, Paul Gortmaker, Jon Mason, netdev

If the MAC is invalid or not implemented, use a randomly generated one rather
than failing the probe. Store the generated addr in a new sw_mac array in the
pch_gbe_mac_info structure. Take care to allow for assigning the MAC via
ifconfig by reusing sw_addr to store an assigned mac if probe populated it with
a random one (otherwise the assignment would rely on the ROM and the reset would
fail to write a valid MAC to the rx filter).

Tested on two platforms, one with a valid MAC, the other without a MAC. The
real MAC is used if present, a randomly generated one otherwise. Both are
capable of changing the MAC with ifconfig. They successfully get an IP over
DHCP and pass a simple ping and login over ssh test.

This does not make any attempt to address a missing or invalid MAC for the
pch_phub driver.

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
CC: Arjan van de Ven <arjan@linux.intel.com>
CC: Alan Cox <alan@linux.intel.com>
CC: Tomoya MORINAGA <tomoya.rohm@gmail.com>
CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> (commit_signer:1/3=33%,commit_signer:1/8=12%)
CC: "David S. Miller" <davem@davemloft.net>
CC: Paul Gortmaker <paul.gortmaker@windriver.com>
CC: Jon Mason <jdmason@kudzu.us>
CC: netdev@vger.kernel.org
---
 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    |    3 ++
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |   25 ++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
index a09a071..3a451a9 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
@@ -356,6 +356,8 @@ struct pch_gbe_functions {
 /**
  * struct pch_gbe_mac_info - MAC information
  * @addr[6]:		Store the MAC address
+ * @sw_addr[6]:		Store a random or specified MAC address if the
+ *			ROM is invalid or missing.
  * @fc:			Mode of flow control
  * @fc_autoneg:		Auto negotiation enable for flow control setting
  * @tx_fc_enable:	Enable flag of Transmit flow control
@@ -367,6 +369,7 @@ struct pch_gbe_functions {
  */
 struct pch_gbe_mac_info {
 	u8 addr[6];
+	u8 sw_addr[6];
 	u8 fc;
 	u8 fc_autoneg;
 	u8 tx_fc_enable;
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 964e9c0..6453a71 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -116,6 +116,16 @@ s32 pch_gbe_mac_read_mac_addr(struct pch_gbe_hw *hw)
 {
 	u32  adr1a, adr1b;
 
+	/*
+	 * If we generated a random mac address during probe, then the one in
+	 * the ROM is either invalid or missing, use the generated one instead.
+	 */
+	if (is_valid_ether_addr(hw->mac.sw_addr)) {
+		memcpy(hw->mac.addr, hw->mac.sw_addr, 6);
+		pr_debug("hw->mac.addr : %pM (using random generated addr)\n", hw->mac.addr);
+		return 0;
+	}
+
 	adr1a = ioread32(&hw->reg->mac_adr[0].high);
 	adr1b = ioread32(&hw->reg->mac_adr[0].low);
 
@@ -2036,6 +2046,8 @@ static int pch_gbe_set_mac(struct net_device *netdev, void *addr)
 		ret_val = -EADDRNOTAVAIL;
 	} else {
 		memcpy(netdev->dev_addr, skaddr->sa_data, netdev->addr_len);
+		if (is_valid_ether_addr(adapter->hw.mac.sw_addr))
+			memcpy(adapter->hw.mac.sw_addr, skaddr->sa_data, netdev->addr_len);
 		memcpy(adapter->hw.mac.addr, skaddr->sa_data, netdev->addr_len);
 		pch_gbe_mac_mar_set(&adapter->hw, adapter->hw.mac.addr, 0);
 		ret_val = 0;
@@ -2444,6 +2456,19 @@ static int pch_gbe_probe(struct pci_dev *pdev,
 	pch_gbe_set_ethtool_ops(netdev);
 
 	pch_gbe_mac_load_mac_addr(&adapter->hw);
+
+	/*
+	 * Try to read the MAC address. If it is invalid (or just missing),
+	 * generate a random one to use from here on out.
+	 */
+	pch_gbe_mac_read_mac_addr(&adapter->hw);
+	if (!is_valid_ether_addr(adapter->hw.mac.addr)) {
+		dev_err(&pdev->dev, "Invalid MAC address, "
+		                    "using a random generated one.\n");
+		random_ether_addr(adapter->hw.mac.sw_addr);
+		memcpy(adapter->hw.mac.addr, adapter->hw.mac.sw_addr, netdev->addr_len);
+	}
+
 	pch_gbe_mac_reset_hw(&adapter->hw);
 
 	/* setup the private structure */
-- 
1.7.6.5


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-14  6:44 [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe Darren Hart
@ 2012-01-14  8:14 ` David Miller
  2012-01-14 15:54   ` Darren Hart
  2012-01-14  8:15 ` David Miller
  1 sibling, 1 reply; 23+ messages in thread
From: David Miller @ 2012-01-14  8:14 UTC (permalink / raw)
  To: dvhart
  Cc: linux-kernel, arjan, alan, tomoya.rohm, jeffrey.t.kirsher,
	paul.gortmaker, jdmason, netdev

From: Darren Hart <dvhart@linux.intel.com>
Date: Fri, 13 Jan 2012 22:44:55 -0800

> If the MAC is invalid or not implemented, use a randomly generated one rather
> than failing the probe. Store the generated addr in a new sw_mac array in the
> pch_gbe_mac_info structure. Take care to allow for assigning the MAC via
> ifconfig by reusing sw_addr to store an assigned mac if probe populated it with
> a random one (otherwise the assignment would rely on the ROM and the reset would
> fail to write a valid MAC to the rx filter).
> 
> Tested on two platforms, one with a valid MAC, the other without a MAC. The
> real MAC is used if present, a randomly generated one otherwise. Both are
> capable of changing the MAC with ifconfig. They successfully get an IP over
> DHCP and pass a simple ping and login over ssh test.
> 
> This does not make any attempt to address a missing or invalid MAC for the
> pch_phub driver.
> 
> Signed-off-by: Darren Hart <dvhart@linux.intel.com>

I don't want to see code like this added if it's "just in case."

Please correct any hardware that hasn't shipped yet or is alpha/beta
hardware in testing, so that we don't need stuff like this.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-14  6:44 [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe Darren Hart
  2012-01-14  8:14 ` David Miller
@ 2012-01-14  8:15 ` David Miller
  2012-01-14  8:18   ` Cong Wang
                     ` (2 more replies)
  1 sibling, 3 replies; 23+ messages in thread
From: David Miller @ 2012-01-14  8:15 UTC (permalink / raw)
  To: dvhart
  Cc: linux-kernel, arjan, alan, tomoya.rohm, jeffrey.t.kirsher,
	paul.gortmaker, jdmason, netdev

From: Darren Hart <dvhart@linux.intel.com>
Date: Fri, 13 Jan 2012 22:44:55 -0800

> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> (commit_signer:1/3=33%,commit_signer:1/8=12%)

BTW, please try to figure out where this "commit_signer" stuff came from.

It ended up corrupting the CC: list of your posting too.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-14  8:15 ` David Miller
@ 2012-01-14  8:18   ` Cong Wang
  2012-01-14 15:45   ` Darren Hart
  2012-01-16 15:22   ` Nick Bowler
  2 siblings, 0 replies; 23+ messages in thread
From: Cong Wang @ 2012-01-14  8:18 UTC (permalink / raw)
  To: David Miller
  Cc: dvhart, linux-kernel, arjan, alan, tomoya.rohm,
	jeffrey.t.kirsher, paul.gortmaker, jdmason, netdev

On 01/14/2012 04:15 PM, David Miller wrote:
> From: Darren Hart<dvhart@linux.intel.com>
> Date: Fri, 13 Jan 2012 22:44:55 -0800
>
>> CC: Jeff Kirsher<jeffrey.t.kirsher@intel.com>  (commit_signer:1/3=33%,commit_signer:1/8=12%)
>
> BTW, please try to figure out where this "commit_signer" stuff came from.
>
> It ended up corrupting the CC: list of your posting too.

It is from `./scripts/get_maintainer.pl`, and can be suppressed by 
adding '--norolestats' option. :)

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-14  8:15 ` David Miller
  2012-01-14  8:18   ` Cong Wang
@ 2012-01-14 15:45   ` Darren Hart
  2012-01-16 15:22   ` Nick Bowler
  2 siblings, 0 replies; 23+ messages in thread
From: Darren Hart @ 2012-01-14 15:45 UTC (permalink / raw)
  To: David Miller
  Cc: linux-kernel, arjan, alan, tomoya.rohm, jeffrey.t.kirsher,
	paul.gortmaker, jdmason, netdev



On 01/14/2012 12:15 AM, David Miller wrote:
> From: Darren Hart <dvhart@linux.intel.com>
> Date: Fri, 13 Jan 2012 22:44:55 -0800
> 
>> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> (commit_signer:1/3=33%,commit_signer:1/8=12%)
> 
> BTW, please try to figure out where this "commit_signer" stuff came from.
> 
> It ended up corrupting the CC: list of your posting too.

Sorry about that, I caught it too late.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-14  8:14 ` David Miller
@ 2012-01-14 15:54   ` Darren Hart
  2012-01-14 19:56     ` David Miller
  0 siblings, 1 reply; 23+ messages in thread
From: Darren Hart @ 2012-01-14 15:54 UTC (permalink / raw)
  To: David Miller
  Cc: linux-kernel, arjan, alan, tomoya.rohm, jeffrey.t.kirsher,
	paul.gortmaker, jdmason, netdev



On 01/14/2012 12:14 AM, David Miller wrote:
> From: Darren Hart <dvhart@linux.intel.com>
> Date: Fri, 13 Jan 2012 22:44:55 -0800
> 
>> If the MAC is invalid or not implemented, use a randomly generated one rather
>> than failing the probe. Store the generated addr in a new sw_mac array in the
>> pch_gbe_mac_info structure. Take care to allow for assigning the MAC via
>> ifconfig by reusing sw_addr to store an assigned mac if probe populated it with
>> a random one (otherwise the assignment would rely on the ROM and the reset would
>> fail to write a valid MAC to the rx filter).
>>
>> Tested on two platforms, one with a valid MAC, the other without a MAC. The
>> real MAC is used if present, a randomly generated one otherwise. Both are
>> capable of changing the MAC with ifconfig. They successfully get an IP over
>> DHCP and pass a simple ping and login over ssh test.
>>
>> This does not make any attempt to address a missing or invalid MAC for the
>> pch_phub driver.
>>
>> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> 
> I don't want to see code like this added if it's "just in case."

I don't disagree, unfortunately it is not "just in case". The Inforce
Goldstein QSeven Module on the Portwell PQ7-C100XL carrier board are
already available and do not implement the MAC EEPROM in hardware.

> 
> Please correct any hardware that hasn't shipped yet or is alpha/beta
> hardware in testing, so that we don't need stuff like this.

I saw that the use of random_ether_addr is fairly prevalent, and
attempted a roughly similar sort of approach to others I had seen. Do
you consider all of those to be "necessary evils" or are there
legitimate situations for its use?

In any case, with existing hardware out there that is unusable with the
current pch_gbe driver, can we consider this workaround for inclusion?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-14 15:54   ` Darren Hart
@ 2012-01-14 19:56     ` David Miller
  2012-01-14 20:29       ` Joe Perches
  2012-01-14 21:46       ` Alan Cox
  0 siblings, 2 replies; 23+ messages in thread
From: David Miller @ 2012-01-14 19:56 UTC (permalink / raw)
  To: dvhart
  Cc: linux-kernel, arjan, alan, tomoya.rohm, jeffrey.t.kirsher,
	paul.gortmaker, jdmason, netdev

From: Darren Hart <dvhart@linux.intel.com>
Date: Sat, 14 Jan 2012 07:54:27 -0800

>> Please correct any hardware that hasn't shipped yet or is alpha/beta
>> hardware in testing, so that we don't need stuff like this.
> 
> I saw that the use of random_ether_addr is fairly prevalent, and
> attempted a roughly similar sort of approach to others I had seen. Do
> you consider all of those to be "necessary evils" or are there
> legitimate situations for its use?
> 
> In any case, with existing hardware out there that is unusable with the
> current pch_gbe driver, can we consider this workaround for inclusion?

I fear that people are just going to add this random MAC stuff way too
easily, it's a spreading disease.

Ship functional hardware instead.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-14 19:56     ` David Miller
@ 2012-01-14 20:29       ` Joe Perches
  2012-01-14 21:46       ` Alan Cox
  1 sibling, 0 replies; 23+ messages in thread
From: Joe Perches @ 2012-01-14 20:29 UTC (permalink / raw)
  To: David Miller
  Cc: dvhart, linux-kernel, arjan, alan, tomoya.rohm,
	jeffrey.t.kirsher, paul.gortmaker, jdmason, netdev

On Sat, 2012-01-14 at 11:56 -0800, David Miller wrote:
> From: Darren Hart <dvhart@linux.intel.com>
> Date: Sat, 14 Jan 2012 07:54:27 -0800
> > In any case, with existing hardware out there that is unusable with the
> > current pch_gbe driver, can we consider this workaround for inclusion?
> I fear that people are just going to add this random MAC stuff way too
> easily, it's a spreading disease.
> Ship functional hardware instead.

Good advice, but that's _hard_.

Working systems are nearly always some combination
of hardware/firmware/software defect workarounds.

Anyway, perhaps setting random_ether_addr like this
should be in some generic routine with a standardized
"avoid buying bad hardware" output logging message.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-14 19:56     ` David Miller
  2012-01-14 20:29       ` Joe Perches
@ 2012-01-14 21:46       ` Alan Cox
  2012-01-14 22:36         ` Darren Hart
  1 sibling, 1 reply; 23+ messages in thread
From: Alan Cox @ 2012-01-14 21:46 UTC (permalink / raw)
  To: David Miller
  Cc: dvhart, linux-kernel, arjan, alan, tomoya.rohm,
	jeffrey.t.kirsher, paul.gortmaker, jdmason, netdev

> I fear that people are just going to add this random MAC stuff way too
> easily, it's a spreading disease.
> 
> Ship functional hardware instead.

See "choir, preaching to the".

The stuff is out there and it's not Darren's fault !

Alan

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-14 21:46       ` Alan Cox
@ 2012-01-14 22:36         ` Darren Hart
  2012-01-16  7:38           ` Tomoya MORINAGA
  0 siblings, 1 reply; 23+ messages in thread
From: Darren Hart @ 2012-01-14 22:36 UTC (permalink / raw)
  To: Alan Cox
  Cc: David Miller, linux-kernel, arjan, alan, tomoya.rohm,
	jeffrey.t.kirsher, paul.gortmaker, jdmason, netdev



On 01/14/2012 01:46 PM, Alan Cox wrote:
>> I fear that people are just going to add this random MAC stuff way too
>> easily, it's a spreading disease.
>>
>> Ship functional hardware instead.
> 
> See "choir, preaching to the".
> 
> The stuff is out there and it's not Darren's fault !
> 
> Alan


So perhaps I should provide some more context into why I'm sending these
patches. The following hardware is currently available and the existing
Linux support is confined to a Timesys Fedora-Based pre-installed image
which requires user intervention to write a MAC using an old, not
upstreamed, modified ioh_gbe_mac driver.

The board documentation is available here:
http://www.inforcecomputing.com/SYS940X_ECX.html

In particular, see:
http://www.inforcecomputing.com/proddls/SYS940X-01_UserGuide_001329.pdf
and:
http://www.inforcecomputing.com/proddls/BLDK2_Kern_2.6.29-10_and_2.6.29-12_for_SYS940X-1_1304.pdf

I felt this patch made this hardware more accessible by allowing it to
work with current kernels without ugly userspace hacks. It is also
following precedent set by existing drivers.

So while I completely agree with the sentiment "Ship functional
hardware", this wasn't a product I was involved with, I'm just trying to
make a bad situation better. As Alan alludes to above, I do actually
spend a good deal of time trying to improve hardware to avoid this kind
of thing (it's an amazingly difficult task).

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-14 22:36         ` Darren Hart
@ 2012-01-16  7:38           ` Tomoya MORINAGA
  2012-01-16 12:31             ` Alan Cox
  2012-01-16 15:38             ` Darren Hart
  0 siblings, 2 replies; 23+ messages in thread
From: Tomoya MORINAGA @ 2012-01-16  7:38 UTC (permalink / raw)
  To: Darren Hart
  Cc: Alan Cox, David Miller, linux-kernel, arjan, alan,
	jeffrey.t.kirsher, paul.gortmaker, jdmason, netdev

2012/1/16 Darren Hart <darren.hart@intel.com>:
> I have since resolved this particular issue. I did not disable the
> pcieqos driver I forward ported. With that disabled, pch_phub works as
> expected.
Yes, you can not use both pcieqos and pch_phub at the same time.
Because pcieqos is previous version of pch_phub which is upstreamed.


> Which is to say it lists pch_mac, reads all 0's, and does
> nothing on write (since the MAC ROM doesn't exist). Please see the patch
> thread from Friday to address this using a random mac if the ROM is
> missing or invalid.
Saving MAC address into external ROM is generic method, I think.
Though I know the ROM-less system using eg20t-pch, however I think
this system is not common.
So, I think pch_gbe shouldn't have auto-mac address assignment.

BTW, as you know, a use can write MAC address using sysfs file system
like below.
echo aa:bb:cc:dd:ee:ff > pch_mac

thanks,
tomoya

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-16  7:38           ` Tomoya MORINAGA
@ 2012-01-16 12:31             ` Alan Cox
  2012-01-16 15:42               ` Darren Hart
  2012-01-16 15:38             ` Darren Hart
  1 sibling, 1 reply; 23+ messages in thread
From: Alan Cox @ 2012-01-16 12:31 UTC (permalink / raw)
  To: Tomoya MORINAGA
  Cc: Darren Hart, Alan Cox, David Miller, linux-kernel, arjan,
	jeffrey.t.kirsher, paul.gortmaker, jdmason, netdev

> Saving MAC address into external ROM is generic method, I think.
> Though I know the ROM-less system using eg20t-pch, however I think
> this system is not common.
> So, I think pch_gbe shouldn't have auto-mac address assignment.

The problem is the module load fails for those cases. You cannot load
the module and use the standard ifconfig eth0 hw aa:bb:cc:dd:ee:ff
interface. The better fix might be to make sure it loads.

So change from

        memcpy(netdev->dev_addr, adapter->hw.mac.addr,
        netdev->addr_len);   
        if (!is_valid_ether_addr(netdev->dev_addr)) {
                dev_err(&pdev->dev, "Invalid MAC Address\n");
                ret = -EIO; 
                goto err_free_adapter;
        }

to just printing a warning, and check the current address when a user
tries to ifconfig it up and refuse to allow the port to go active.

Alan

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-14  8:15 ` David Miller
  2012-01-14  8:18   ` Cong Wang
  2012-01-14 15:45   ` Darren Hart
@ 2012-01-16 15:22   ` Nick Bowler
  2012-01-16 23:04     ` David Miller
  2 siblings, 1 reply; 23+ messages in thread
From: Nick Bowler @ 2012-01-16 15:22 UTC (permalink / raw)
  To: David Miller
  Cc: dvhart, linux-kernel, arjan, alan, tomoya.rohm,
	jeffrey.t.kirsher, paul.gortmaker, jdmason, netdev

On 2012-01-14 00:15 -0800, David Miller wrote:
> From: Darren Hart <dvhart@linux.intel.com>
> Date: Fri, 13 Jan 2012 22:44:55 -0800
> 
> > CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> (commit_signer:1/3=33%,commit_signer:1/8=12%)
> 
> BTW, please try to figure out where this "commit_signer" stuff came from.
> 
> It ended up corrupting the CC: list of your posting too.

While it ended up on the CC line, the result is still a valid and
correct email address per RFC 822.

Cheers,
-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-16  7:38           ` Tomoya MORINAGA
  2012-01-16 12:31             ` Alan Cox
@ 2012-01-16 15:38             ` Darren Hart
  1 sibling, 0 replies; 23+ messages in thread
From: Darren Hart @ 2012-01-16 15:38 UTC (permalink / raw)
  To: Tomoya MORINAGA
  Cc: Alan Cox, David Miller, linux-kernel, arjan, alan,
	jeffrey.t.kirsher, paul.gortmaker, jdmason, netdev



On 01/15/2012 11:38 PM, Tomoya MORINAGA wrote:
> 2012/1/16 Darren Hart <darren.hart@intel.com>:
>> I have since resolved this particular issue. I did not disable the
>> pcieqos driver I forward ported. With that disabled, pch_phub works as
>> expected.
> Yes, you can not use both pcieqos and pch_phub at the same time.
> Because pcieqos is previous version of pch_phub which is upstreamed.

Right, they both claim the same PCI ID.

> 
>> Which is to say it lists pch_mac, reads all 0's, and does
>> nothing on write (since the MAC ROM doesn't exist). Please see the patch
>> thread from Friday to address this using a random mac if the ROM is
>> missing or invalid.
> Saving MAC address into external ROM is generic method, I think.
> Though I know the ROM-less system using eg20t-pch, however I think
> this system is not common.
> So, I think pch_gbe shouldn't have auto-mac address assignment.
> 
> BTW, as you know, a use can write MAC address using sysfs file system
> like below.
> echo aa:bb:cc:dd:ee:ff > pch_mac

Right, this doesn't work on the ROM-less system. At least, the
subsequent read returns all 0's. The same is true with the phub-util-mac
and pcieqos. I believe this is due to pci_map_rom failing.

Also, if you don't build the driver as a module, then the above still
isn't sufficient as the pci probe fails and the device isn't created.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-16 12:31             ` Alan Cox
@ 2012-01-16 15:42               ` Darren Hart
  2012-01-16 16:07                 ` Arjan van de Ven
  0 siblings, 1 reply; 23+ messages in thread
From: Darren Hart @ 2012-01-16 15:42 UTC (permalink / raw)
  To: Alan Cox
  Cc: Tomoya MORINAGA, Alan Cox, David Miller, linux-kernel, arjan,
	jeffrey.t.kirsher, paul.gortmaker, jdmason, netdev



On 01/16/2012 04:31 AM, Alan Cox wrote:
>> Saving MAC address into external ROM is generic method, I think.
>> Though I know the ROM-less system using eg20t-pch, however I think
>> this system is not common.
>> So, I think pch_gbe shouldn't have auto-mac address assignment.
> 
> The problem is the module load fails for those cases. You cannot load
> the module and use the standard ifconfig eth0 hw aa:bb:cc:dd:ee:ff
> interface. The better fix might be to make sure it loads.
> 
> So change from
> 
>         memcpy(netdev->dev_addr, adapter->hw.mac.addr,
>         netdev->addr_len);   
>         if (!is_valid_ether_addr(netdev->dev_addr)) {
>                 dev_err(&pdev->dev, "Invalid MAC Address\n");
>                 ret = -EIO; 
>                 goto err_free_adapter;
>         }
> 
> to just printing a warning, and check the current address when a user
> tries to ifconfig it up and refuse to allow the port to go active.

I can go this route I suppose. I don't really understand the objection
to the use of a random mac addr in the special case given the prevalence
of this approach within existing drivers.

One reason I don't care for this alternative approach is that this
particular hardware is targeted at embedded use where we can't assume a
full init system is available, etc. It can be made to work of course, it
just isn't as automated.

David, would you orefer/accept an alternative patch which allows the
driver to load without a MAC address so the user can set it via ifconfig
after boot?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-16 15:42               ` Darren Hart
@ 2012-01-16 16:07                 ` Arjan van de Ven
  2012-01-16 16:20                   ` David Laight
  0 siblings, 1 reply; 23+ messages in thread
From: Arjan van de Ven @ 2012-01-16 16:07 UTC (permalink / raw)
  To: Darren Hart
  Cc: Alan Cox, Tomoya MORINAGA, Alan Cox, David Miller, linux-kernel,
	jeffrey.t.kirsher, paul.gortmaker, jdmason, netdev

On 1/16/2012 7:42 AM, Darren Hart wrote:

> One reason I don't care for this alternative approach is that this
> particular hardware is targeted at embedded use where we can't assume a
> full init system is available, etc. It can be made to work of course, it
> just isn't as automated.
> 

the tricky thing with embedded hw like this is that all devices might
end up with the same, read-only filesystem, so storing the mac on the FS
and then loading it from there into the HW is... suboptimal.

Would be very nice if busybox had a command that would check the mac
from each IF, and created the random mac from userspace automatically...


^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-16 16:07                 ` Arjan van de Ven
@ 2012-01-16 16:20                   ` David Laight
  2012-01-16 16:35                     ` Arjan van de Ven
  2012-01-16 21:06                     ` Rick Jones
  0 siblings, 2 replies; 23+ messages in thread
From: David Laight @ 2012-01-16 16:20 UTC (permalink / raw)
  To: Arjan van de Ven, Darren Hart
  Cc: Alan Cox, Tomoya MORINAGA, Alan Cox, David Miller, linux-kernel,
	jeffrey.t.kirsher, paul.gortmaker, jdmason, netdev

 
> the tricky thing with embedded hw like this is that all devices might
> end up with the same, read-only filesystem, so storing the 
> mac on the FS
> and then loading it from there into the HW is... suboptimal.
> 
> Would be very nice if busybox had a command that would check the mac
> from each IF, and created the random mac from userspace 
> automatically...

Since multiple interfaces on a single system are unlikely
to be connected to the same LAN segment, it doesn't really
matter if they use same MAC address.

For a long time sun solaris systems used the same MAC
address (based on the system id) on all their ethernet
interfaces.

But yes, you don't want a 'random' number generator that
might give the same value for cloned systems.
You also really want a 'manufacturer id' for 'random address'
so that they can't collide with anuything using real addresses.

	David



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-16 16:20                   ` David Laight
@ 2012-01-16 16:35                     ` Arjan van de Ven
  2012-01-16 16:44                       ` Mark Brown
  2012-01-16 21:06                     ` Rick Jones
  1 sibling, 1 reply; 23+ messages in thread
From: Arjan van de Ven @ 2012-01-16 16:35 UTC (permalink / raw)
  To: David Laight
  Cc: Darren Hart, Alan Cox, Tomoya MORINAGA, Alan Cox, David Miller,
	linux-kernel, jeffrey.t.kirsher, paul.gortmaker, jdmason, netdev

On 1/16/2012 8:20 AM, David Laight wrote:
>  
>> the tricky thing with embedded hw like this is that all devices might
>> end up with the same, read-only filesystem, so storing the 
>> mac on the FS
>> and then loading it from there into the HW is... suboptimal.
>>
>> Would be very nice if busybox had a command that would check the mac
>> from each IF, and created the random mac from userspace 
>> automatically...
> 
> Since multiple interfaces on a single system are unlikely
> to be connected to the same LAN segment, it doesn't really
> matter if they use same MAC address.

I think you missed the point. All embedded devices have the same fs, so
if you have 2 boxes of the same model/brand on the same network, they'd
have the same MAC. That's generally frowned upon by network
administrators ;-)

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-16 16:35                     ` Arjan van de Ven
@ 2012-01-16 16:44                       ` Mark Brown
  2012-01-16 17:02                         ` Alan Cox
  0 siblings, 1 reply; 23+ messages in thread
From: Mark Brown @ 2012-01-16 16:44 UTC (permalink / raw)
  To: Arjan van de Ven
  Cc: David Laight, Darren Hart, Alan Cox, Tomoya MORINAGA, Alan Cox,
	David Miller, linux-kernel, jeffrey.t.kirsher, paul.gortmaker,
	jdmason, netdev

On Mon, Jan 16, 2012 at 08:35:09AM -0800, Arjan van de Ven wrote:
> On 1/16/2012 8:20 AM, David Laight wrote:

> > Since multiple interfaces on a single system are unlikely
> > to be connected to the same LAN segment, it doesn't really
> > matter if they use same MAC address.

> I think you missed the point. All embedded devices have the same fs, so
> if you have 2 boxes of the same model/brand on the same network, they'd
> have the same MAC. That's generally frowned upon by network
> administrators ;-)

This is generally handled by putting the MAC into a board-specific bit
of flash that isn't part the main firmware (for example, the bootloader
configuration) - for a lot of applications random generation doesn't
help that much as entropy is hard to come by.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-16 16:44                       ` Mark Brown
@ 2012-01-16 17:02                         ` Alan Cox
  0 siblings, 0 replies; 23+ messages in thread
From: Alan Cox @ 2012-01-16 17:02 UTC (permalink / raw)
  To: Mark Brown
  Cc: Arjan van de Ven, David Laight, Darren Hart, Tomoya MORINAGA,
	Alan Cox, David Miller, linux-kernel, jeffrey.t.kirsher,
	paul.gortmaker, jdmason, netdev

> bootloader configuration) - for a lot of applications random
> generation doesn't help that much as entropy is hard to come by.

This is an x86 platform - plenty of entropy from the tsc etc.


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-16 16:20                   ` David Laight
  2012-01-16 16:35                     ` Arjan van de Ven
@ 2012-01-16 21:06                     ` Rick Jones
  1 sibling, 0 replies; 23+ messages in thread
From: Rick Jones @ 2012-01-16 21:06 UTC (permalink / raw)
  To: David Laight
  Cc: Arjan van de Ven, Darren Hart, Alan Cox, Tomoya MORINAGA,
	Alan Cox, David Miller, linux-kernel, jeffrey.t.kirsher,
	paul.gortmaker, jdmason, netdev

On 01/16/2012 08:20 AM, David Laight wrote:
> Since multiple interfaces on a single system are unlikely
> to be connected to the same LAN segment, it doesn't really
> matter if they use same MAC address.
>
> For a long time sun solaris systems used the same MAC
> address (based on the system id) on all their ethernet
> interfaces.

And there was a perhaps small, but certainly non-trivial stream of 
people asking how to change that because their system(s) were indeed 
multiple ports connected to the same broadcast domain.

Long since then Sun changed their defaults, and we've gotten things like 
bonding, so I think any assumption that a system with multiple ports 
will not have then connected to the same broadcast domain is brittle at 
best.

rick jones

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-16 15:22   ` Nick Bowler
@ 2012-01-16 23:04     ` David Miller
  2012-01-16 23:07       ` Darren Hart
  0 siblings, 1 reply; 23+ messages in thread
From: David Miller @ 2012-01-16 23:04 UTC (permalink / raw)
  To: nbowler
  Cc: dvhart, linux-kernel, arjan, alan, tomoya.rohm,
	jeffrey.t.kirsher, paul.gortmaker, jdmason, netdev

From: Nick Bowler <nbowler@elliptictech.com>
Date: Mon, 16 Jan 2012 10:22:39 -0500

> On 2012-01-14 00:15 -0800, David Miller wrote:
>> From: Darren Hart <dvhart@linux.intel.com>
>> Date: Fri, 13 Jan 2012 22:44:55 -0800
>> 
>> > CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> (commit_signer:1/3=33%,commit_signer:1/8=12%)
>> 
>> BTW, please try to figure out where this "commit_signer" stuff came from.
>> 
>> It ended up corrupting the CC: list of your posting too.
> 
> While it ended up on the CC line, the result is still a valid and
> correct email address per RFC 822.

I didn't say it wasn't an "RFC compliant" email address, I just said
it's complete garbage.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe
  2012-01-16 23:04     ` David Miller
@ 2012-01-16 23:07       ` Darren Hart
  0 siblings, 0 replies; 23+ messages in thread
From: Darren Hart @ 2012-01-16 23:07 UTC (permalink / raw)
  To: David Miller
  Cc: nbowler, linux-kernel, arjan, alan, tomoya.rohm,
	jeffrey.t.kirsher, paul.gortmaker, jdmason, netdev



On 01/16/2012 03:04 PM, David Miller wrote:
> From: Nick Bowler <nbowler@elliptictech.com>
> Date: Mon, 16 Jan 2012 10:22:39 -0500
> 
>> On 2012-01-14 00:15 -0800, David Miller wrote:
>>> From: Darren Hart <dvhart@linux.intel.com>
>>> Date: Fri, 13 Jan 2012 22:44:55 -0800
>>>
>>>> CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com> (commit_signer:1/3=33%,commit_signer:1/8=12%)
>>>
>>> BTW, please try to figure out where this "commit_signer" stuff came from.
>>>
>>> It ended up corrupting the CC: list of your posting too.
>>
>> While it ended up on the CC line, the result is still a valid and
>> correct email address per RFC 822.
> 
> I didn't say it wasn't an "RFC compliant" email address, I just said
> it's complete garbage.

Indeed. An oversight on my part and I've addressed it. Apologies for the
noise.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2012-01-16 23:07 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-14  6:44 [PATCH] pch_gbe: Use a randomly generated MAC instead of failing probe Darren Hart
2012-01-14  8:14 ` David Miller
2012-01-14 15:54   ` Darren Hart
2012-01-14 19:56     ` David Miller
2012-01-14 20:29       ` Joe Perches
2012-01-14 21:46       ` Alan Cox
2012-01-14 22:36         ` Darren Hart
2012-01-16  7:38           ` Tomoya MORINAGA
2012-01-16 12:31             ` Alan Cox
2012-01-16 15:42               ` Darren Hart
2012-01-16 16:07                 ` Arjan van de Ven
2012-01-16 16:20                   ` David Laight
2012-01-16 16:35                     ` Arjan van de Ven
2012-01-16 16:44                       ` Mark Brown
2012-01-16 17:02                         ` Alan Cox
2012-01-16 21:06                     ` Rick Jones
2012-01-16 15:38             ` Darren Hart
2012-01-14  8:15 ` David Miller
2012-01-14  8:18   ` Cong Wang
2012-01-14 15:45   ` Darren Hart
2012-01-16 15:22   ` Nick Bowler
2012-01-16 23:04     ` David Miller
2012-01-16 23:07       ` Darren Hart

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).