| Message ID | 132d2a41a089905de3147b4656e350608aa7fd6f.1326523495.git.dvhart@linux.intel.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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?
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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).
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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.
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?
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... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ;-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 */
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(-)