| Message ID | 1472113162-26915-1-git-send-email-dev@kresin.me (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Aug 25, 2016 at 10:19:22AM +0200, Mathias Kresin wrote: > The EEPROM used on some CPEs has for every device the same generic > ralink mac in EEPROM and needs to be overridden. I don't know what is CPE, but even if I would know that, I most likely sill will not understand that description. > +++ b/drivers/net/wireless/ralink/rt2x00/rt2400pci.c > @@ -1459,10 +1459,7 @@ static int rt2400pci_validate_eeprom(struct rt2x00_dev *rt2x00dev) > * Start validation of the data that has been read. > */ > mac = rt2x00_eeprom_addr(rt2x00dev, EEPROM_MAC_ADDR_0); > - if (!is_valid_ether_addr(mac)) { > - eth_random_addr(mac); > - rt2x00_eeprom_dbg(rt2x00dev, "MAC: %pM\n", mac); > - } > + rt2x00lib_set_mac_address(rt2x00dev, mac); <snip> > +#include <linux/of.h> > +#include <linux/of_net.h> > > #include "rt2x00.h" > #include "rt2x00lib.h" > @@ -931,6 +933,21 @@ static void rt2x00lib_rate(struct ieee80211_rate *entry, > entry->flags |= IEEE80211_RATE_SHORT_PREAMBLE; > } > > +void rt2x00lib_set_mac_address(struct rt2x00_dev *rt2x00dev, u8 *eeprom_mac_addr) > +{ > + const char *mac_addr; > + > + mac_addr = of_get_mac_address(rt2x00dev->dev->of_node); Shouldn't use dev_of_node(&rt2x00dev->dev) and check against NULL ? > + if (mac_addr) > + ether_addr_copy(eeprom_mac_addr, mac_addr); Why we do not read MAC from EEPROM if fail to get it from of_node? Thanks Stanislaw
2016-08-25 11:33 GMT+02:00 Stanislaw Gruszka <sgruszka@redhat.com>: > > On Thu, Aug 25, 2016 at 10:19:22AM +0200, Mathias Kresin wrote: > > The EEPROM used on some CPEs has for every device the same generic > > ralink mac in EEPROM and needs to be overridden. > > I don't know what is CPE, but even if I would know that, I most likely > sill will not understand that description. Well, seams to me the commit message can be improved. If a v2 is required or a v2 is required because of the commit message, I'll take care of it. CPE = Customer Premises Equipment or the small plastic box from your ISP at home. The whole point of the patch is that the MAC stored in the wifi EEPROM might not be unique and need to be overridden. I'm aware of three different "home router", where each model has the same generic ralink MAC address stored in the wifi EEPROM. This can cause nasty issues. > > +++ b/drivers/net/wireless/ralink/rt2x00/rt2400pci.c > > @@ -1459,10 +1459,7 @@ static int rt2400pci_validate_eeprom(struct rt2x00_dev *rt2x00dev) > > * Start validation of the data that has been read. > > */ > > mac = rt2x00_eeprom_addr(rt2x00dev, EEPROM_MAC_ADDR_0); > > - if (!is_valid_ether_addr(mac)) { > > - eth_random_addr(mac); > > - rt2x00_eeprom_dbg(rt2x00dev, "MAC: %pM\n", mac); > > - } > > + rt2x00lib_set_mac_address(rt2x00dev, mac); > <snip> > > +#include <linux/of.h> > > +#include <linux/of_net.h> > > > > #include "rt2x00.h" > > #include "rt2x00lib.h" > > @@ -931,6 +933,21 @@ static void rt2x00lib_rate(struct ieee80211_rate *entry, > > entry->flags |= IEEE80211_RATE_SHORT_PREAMBLE; > > } > > > > +void rt2x00lib_set_mac_address(struct rt2x00_dev *rt2x00dev, u8 *eeprom_mac_addr) > > +{ > > + const char *mac_addr; > > + > > + mac_addr = of_get_mac_address(rt2x00dev->dev->of_node); > > Shouldn't use dev_of_node(&rt2x00dev->dev) and check against NULL ? Not sure if dev_of_node() is meant to be used by every driver. Or at least the function is only used by base stuff and not by any driver. The NULL check doesn't seam to me required. The of_node is finally passed to __of_find_property which does the NULL check before using of_node. Mathias
On Thu, Aug 25, 2016 at 01:12:22PM +0200, Mathias Kresin wrote: > 2016-08-25 11:33 GMT+02:00 Stanislaw Gruszka <sgruszka@redhat.com>: > > > > On Thu, Aug 25, 2016 at 10:19:22AM +0200, Mathias Kresin wrote: > > > The EEPROM used on some CPEs has for every device the same generic > > > ralink mac in EEPROM and needs to be overridden. > > > > I don't know what is CPE, but even if I would know that, I most likely > > sill will not understand that description. > > Well, seams to me the commit message can be improved. If a v2 is > required or a v2 is required because of the commit message, I'll take > care of it. Please do. > CPE = Customer Premises Equipment or the small plastic box from your > ISP at home. The whole point of the patch is that the MAC stored in > the wifi EEPROM might not be unique and need to be overridden. I'm > aware of three different "home router", where each model has the same > generic ralink MAC address stored in the wifi EEPROM. This can cause > nasty issues. I think we still want MAC from EEPROM instead of random one on systems without OF. Otherwise we could just use random MAC every time, but this does not seems lika a good idea. Can we check against that particular MAC that repeats on those CPEs and if it match use random one ? Or use some other identification to find out that EEPROM MAC is not good ? > > Shouldn't use dev_of_node(&rt2x00dev->dev) and check against NULL ? > > Not sure if dev_of_node() is meant to be used by every driver. Or at > least the function is only used by base stuff and not by any driver. > > The NULL check doesn't seam to me required. The of_node is finally > passed to __of_find_property which does the NULL check before using > of_node. Ok. Thanks Stanislaw
2016-08-25 15:19 GMT+02:00 Stanislaw Gruszka <sgruszka@redhat.com>: > On Thu, Aug 25, 2016 at 01:12:22PM +0200, Mathias Kresin wrote: >> 2016-08-25 11:33 GMT+02:00 Stanislaw Gruszka <sgruszka@redhat.com>: >> > On Thu, Aug 25, 2016 at 10:19:22AM +0200, Mathias Kresin wrote: >> CPE = Customer Premises Equipment or the small plastic box from your >> ISP at home. The whole point of the patch is that the MAC stored in >> the wifi EEPROM might not be unique and need to be overridden. I'm >> aware of three different "home router", where each model has the same >> generic ralink MAC address stored in the wifi EEPROM. This can cause >> nasty issues. > > I think we still want MAC from EEPROM instead of random one on systems > without OF. Otherwise we could just use random MAC every time, but this > does not seems lika a good idea. Either I got you wrong, the code does something different than I intended/tested or you misread the code. The mac address stored in the EEPROM is only overridden in case: a) a mac address is defined in the device tree. b) invalid/no mac stored in EEPROM => random one If none of the above is true, the EEPROM mac will be used. What I've added is a). Everything else is the same as before. > Can we check against that particular MAC that repeats on those CPEs and > if it match use random one ? Or use some other identification to find > out that EEPROM MAC is not good ? IMHO this would be a wonky approach. I had a look at the EEPROM MACs from two of the affected devices and spotted just now a coherence with the used wifi chip: VGV7510KW22 (RT3062F): 00:0C:43:30:62:00 ARV7506 (RT3060F): 00:0C:43:30:60:00 IMHO, it would be reckless to assume that it's the same for other affected models. You know, ODMs doing silly things. Personally I prefer to deal with the issue in device tree rather than forcing a random MAC address. This way I can set the MAC used with the stock firmware to fix the issue. Mathias
On Thu, Aug 25, 2016 at 05:03:06PM +0200, Mathias Kresin wrote: > 2016-08-25 15:19 GMT+02:00 Stanislaw Gruszka <sgruszka@redhat.com>: > > On Thu, Aug 25, 2016 at 01:12:22PM +0200, Mathias Kresin wrote: > >> 2016-08-25 11:33 GMT+02:00 Stanislaw Gruszka <sgruszka@redhat.com>: > >> > On Thu, Aug 25, 2016 at 10:19:22AM +0200, Mathias Kresin wrote: > > >> CPE = Customer Premises Equipment or the small plastic box from your > >> ISP at home. The whole point of the patch is that the MAC stored in > >> the wifi EEPROM might not be unique and need to be overridden. I'm > >> aware of three different "home router", where each model has the same > >> generic ralink MAC address stored in the wifi EEPROM. This can cause > >> nasty issues. > > > > I think we still want MAC from EEPROM instead of random one on systems > > without OF. Otherwise we could just use random MAC every time, but this > > does not seems lika a good idea. > > Either I got you wrong, the code does something different than I > intended/tested or you misread the code. I misread the patch. It's ok (except the changelog). Thanks Stanislaw
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2400pci.c b/drivers/net/wireless/ralink/rt2x00/rt2400pci.c index 155f343..085c5b4 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2400pci.c +++ b/drivers/net/wireless/ralink/rt2x00/rt2400pci.c @@ -1459,10 +1459,7 @@ static int rt2400pci_validate_eeprom(struct rt2x00_dev *rt2x00dev) * Start validation of the data that has been read. */ mac = rt2x00_eeprom_addr(rt2x00dev, EEPROM_MAC_ADDR_0); - if (!is_valid_ether_addr(mac)) { - eth_random_addr(mac); - rt2x00_eeprom_dbg(rt2x00dev, "MAC: %pM\n", mac); - } + rt2x00lib_set_mac_address(rt2x00dev, mac); rt2x00_eeprom_read(rt2x00dev, EEPROM_ANTENNA, &word); if (word == 0xffff) { diff --git a/drivers/net/wireless/ralink/rt2x00/rt2500pci.c b/drivers/net/wireless/ralink/rt2x00/rt2500pci.c index 2553cdd..9832fd5 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2500pci.c +++ b/drivers/net/wireless/ralink/rt2x00/rt2500pci.c @@ -1585,10 +1585,7 @@ static int rt2500pci_validate_eeprom(struct rt2x00_dev *rt2x00dev) * Start validation of the data that has been read. */ mac = rt2x00_eeprom_addr(rt2x00dev, EEPROM_MAC_ADDR_0); - if (!is_valid_ether_addr(mac)) { - eth_random_addr(mac); - rt2x00_eeprom_dbg(rt2x00dev, "MAC: %pM\n", mac); - } + rt2x00lib_set_mac_address(rt2x00dev, mac); rt2x00_eeprom_read(rt2x00dev, EEPROM_ANTENNA, &word); if (word == 0xffff) { diff --git a/drivers/net/wireless/ralink/rt2x00/rt2500usb.c b/drivers/net/wireless/ralink/rt2x00/rt2500usb.c index 2d64611..cd3ab5a 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2500usb.c +++ b/drivers/net/wireless/ralink/rt2x00/rt2500usb.c @@ -1349,10 +1349,7 @@ static int rt2500usb_validate_eeprom(struct rt2x00_dev *rt2x00dev) * Start validation of the data that has been read. */ mac = rt2x00_eeprom_addr(rt2x00dev, EEPROM_MAC_ADDR_0); - if (!is_valid_ether_addr(mac)) { - eth_random_addr(mac); - rt2x00_eeprom_dbg(rt2x00dev, "MAC: %pM\n", mac); - } + rt2x00lib_set_mac_address(rt2x00dev, mac); rt2x00_eeprom_read(rt2x00dev, EEPROM_ANTENNA, &word); if (word == 0xffff) { diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c index bf3f0a3..59c49af 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c +++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c @@ -6919,10 +6919,7 @@ static int rt2800_validate_eeprom(struct rt2x00_dev *rt2x00dev) * Start validation of the data that has been read. */ mac = rt2800_eeprom_addr(rt2x00dev, EEPROM_MAC_ADDR_0); - if (!is_valid_ether_addr(mac)) { - eth_random_addr(mac); - rt2x00_eeprom_dbg(rt2x00dev, "MAC: %pM\n", mac); - } + rt2x00lib_set_mac_address(rt2x00dev, mac); rt2800_eeprom_read(rt2x00dev, EEPROM_NIC_CONF0, &word); if (word == 0xffff) { diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00.h b/drivers/net/wireless/ralink/rt2x00/rt2x00.h index f68d492..aa3d4cee 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2x00.h +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00.h @@ -1403,6 +1403,7 @@ static inline void rt2x00debug_dump_frame(struct rt2x00_dev *rt2x00dev, */ u32 rt2x00lib_get_bssidx(struct rt2x00_dev *rt2x00dev, struct ieee80211_vif *vif); +void rt2x00lib_set_mac_address(struct rt2x00_dev *rt2x00dev, u8 *eeprom_mac_addr); /* * Interrupt context handlers. diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c index 4e0c565..d659250 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c @@ -26,6 +26,8 @@ #include <linux/module.h> #include <linux/slab.h> #include <linux/log2.h> +#include <linux/of.h> +#include <linux/of_net.h> #include "rt2x00.h" #include "rt2x00lib.h" @@ -931,6 +933,21 @@ static void rt2x00lib_rate(struct ieee80211_rate *entry, entry->flags |= IEEE80211_RATE_SHORT_PREAMBLE; } +void rt2x00lib_set_mac_address(struct rt2x00_dev *rt2x00dev, u8 *eeprom_mac_addr) +{ + const char *mac_addr; + + mac_addr = of_get_mac_address(rt2x00dev->dev->of_node); + if (mac_addr) + ether_addr_copy(eeprom_mac_addr, mac_addr); + + if (!is_valid_ether_addr(eeprom_mac_addr)) { + eth_random_addr(eeprom_mac_addr); + rt2x00_eeprom_dbg(rt2x00dev, "MAC: %pM\n", eeprom_mac_addr); + } +} +EXPORT_SYMBOL_GPL(rt2x00lib_set_mac_address); + static int rt2x00lib_probe_hw_modes(struct rt2x00_dev *rt2x00dev, struct hw_mode_spec *spec) { diff --git a/drivers/net/wireless/ralink/rt2x00/rt61pci.c b/drivers/net/wireless/ralink/rt2x00/rt61pci.c index 03013eb..5306a3b 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt61pci.c +++ b/drivers/net/wireless/ralink/rt2x00/rt61pci.c @@ -2413,10 +2413,7 @@ static int rt61pci_validate_eeprom(struct rt2x00_dev *rt2x00dev) * Start validation of the data that has been read. */ mac = rt2x00_eeprom_addr(rt2x00dev, EEPROM_MAC_ADDR_0); - if (!is_valid_ether_addr(mac)) { - eth_random_addr(mac); - rt2x00_eeprom_dbg(rt2x00dev, "MAC: %pM\n", mac); - } + rt2x00lib_set_mac_address(rt2x00dev, mac); rt2x00_eeprom_read(rt2x00dev, EEPROM_ANTENNA, &word); if (word == 0xffff) { diff --git a/drivers/net/wireless/ralink/rt2x00/rt73usb.c b/drivers/net/wireless/ralink/rt2x00/rt73usb.c index c1397a6..1a29c4d 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt73usb.c +++ b/drivers/net/wireless/ralink/rt2x00/rt73usb.c @@ -1766,10 +1766,7 @@ static int rt73usb_validate_eeprom(struct rt2x00_dev *rt2x00dev) * Start validation of the data that has been read. */ mac = rt2x00_eeprom_addr(rt2x00dev, EEPROM_MAC_ADDR_0); - if (!is_valid_ether_addr(mac)) { - eth_random_addr(mac); - rt2x00_eeprom_dbg(rt2x00dev, "MAC: %pM\n", mac); - } + rt2x00lib_set_mac_address(rt2x00dev, mac); rt2x00_eeprom_read(rt2x00dev, EEPROM_ANTENNA, &word); if (word == 0xffff) {
The EEPROM used on some CPEs has for every device the same generic ralink mac in EEPROM and needs to be overridden. Signed-off-by: Mathias Kresin <dev@kresin.me> --- drivers/net/wireless/ralink/rt2x00/rt2400pci.c | 5 +---- drivers/net/wireless/ralink/rt2x00/rt2500pci.c | 5 +---- drivers/net/wireless/ralink/rt2x00/rt2500usb.c | 5 +---- drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 5 +---- drivers/net/wireless/ralink/rt2x00/rt2x00.h | 1 + drivers/net/wireless/ralink/rt2x00/rt2x00dev.c | 17 +++++++++++++++++ drivers/net/wireless/ralink/rt2x00/rt61pci.c | 5 +---- drivers/net/wireless/ralink/rt2x00/rt73usb.c | 5 +---- 8 files changed, 24 insertions(+), 24 deletions(-)