| Message ID | 20240410095603.502566-1-jtornosm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: usb: ax88179_178a: avoid writing the mac address before first reading | expand |
On Wed, 10 Apr 2024 11:55:49 +0200 Jose Ignacio Tornos Martinez wrote: > After the commit d2689b6a86b9 ("net: usb: ax88179_178a: avoid two > consecutive device resets"), reset operation, in which the default mac > address from the device is read, is not executed from bind operation and > the random address, that is pregenerated just in case, is direclty written > the first time in the device, so the default one from the device is not > even read. This writing is not dangerous because is volatile and the > default mac address is not missed. AFAICT the reset is synchronous to resume, right? I think you can use netif_device_detach() and netif_device_attach() to prevent getting called while suspended.
Hello Jakub, I have been trying to use netif_device_detach() and netif_device_attach() as conditions, but maybe I am misunderstanding you, becasue I think the detected problem is not related to suspend/resume process. Let me try to explain better (if considered, I can complete the patch explanation later): The issue happened at the initialization stage. At that moment, during normal rtnl_setlink call, the mac address is set and written in the device registers, but since the reset was not commanded previously, the mac address is not read from the device and without that, it always has the random address that is pre-generated just in case. After this, during open operation, the reset is commanded and the mac address is read, but as the device registers were modified, it reads the pregenerated random mac address and not the default mac address for the device. To fix, I am trying to protect this situtation, not allowing to write if the reset and the default mac address for the device is not previously read. I think it is easier in the driver because of the device condition. Thank you Best regards José Ignacio
On Mon, 15 Apr 2024 09:27:32 +0200 Jose Ignacio Tornos Martinez wrote: > The issue happened at the initialization stage. At that moment, during > normal rtnl_setlink call, the mac address is set and written in the device > registers, but since the reset was not commanded previously, the mac > address is not read from the device and without that, it always has the > random address that is pre-generated just in case. > After this, during open operation, the reset is commanded and the mac > address is read, but as the device registers were modified, it reads the > pregenerated random mac address and not the default mac address for the > device. Oh, I see, why don't we issue the reset and probe time, then?
Hello Jakub, My intention was to simplify it and reduce the delays, because two consecutive resets were used at initialization, one from bind (the one initially called during probe) and one from open. Indeed my initial idea was to let the one from bind/probe only but reset operation had to be removed from driver_info and that is the reason why the one from open was choosen. But the one from bind/probe is better in order to avoid the commented issue. Let me try with this in a second version. Thank you again Best regards José Ignacio
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c index 69169842fa2f..650bb7b6badf 100644 --- a/drivers/net/usb/ax88179_178a.c +++ b/drivers/net/usb/ax88179_178a.c @@ -174,6 +174,7 @@ struct ax88179_data { u32 wol_supported; u32 wolopts; u8 disconnecting; + u8 mac_address_read; }; struct ax88179_int_data { @@ -969,9 +970,12 @@ static int ax88179_change_mtu(struct net_device *net, int new_mtu) static int ax88179_set_mac_addr(struct net_device *net, void *p) { struct usbnet *dev = netdev_priv(net); + struct ax88179_data *ax179_data = dev->driver_priv; struct sockaddr *addr = p; int ret; + if (!ax179_data->mac_address_read) + return -EAGAIN; if (netif_running(net)) return -EBUSY; if (!is_valid_ether_addr(addr->sa_data)) @@ -1256,6 +1260,7 @@ static int ax88179_led_setting(struct usbnet *dev) static void ax88179_get_mac_addr(struct usbnet *dev) { + struct ax88179_data *ax179_data = dev->driver_priv; u8 mac[ETH_ALEN]; memset(mac, 0, sizeof(mac)); @@ -1281,6 +1286,9 @@ static void ax88179_get_mac_addr(struct usbnet *dev) ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, ETH_ALEN, ETH_ALEN, dev->net->dev_addr); + + ax179_data->mac_address_read = 1; + netdev_info(dev->net, "MAC address: %pM\n", dev->net->dev_addr); } static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
After the commit d2689b6a86b9 ("net: usb: ax88179_178a: avoid two consecutive device resets"), reset operation, in which the default mac address from the device is read, is not executed from bind operation and the random address, that is pregenerated just in case, is direclty written the first time in the device, so the default one from the device is not even read. This writing is not dangerous because is volatile and the default mac address is not missed. In order to avoid this, do not allow writing a mac address directly into the device, if the default mac address from the device has not been read yet. cc: stable@vger.kernel.org # 6.6+ Fixes: d2689b6a86b9 ("net: usb: ax88179_178a: avoid two consecutive device resets") Reported-by: Jarkko Palviainen <jarkko.palviainen@gmail.com> Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com> --- drivers/net/usb/ax88179_178a.c | 8 ++++++++ 1 file changed, 8 insertions(+)