| Message ID | 2397d98d-e373-1740-eb5f-8fe795a0352a@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | handling MAC set by user space in reset_resume() of r8152 | expand |
On Wed, Jul 27, 2022 at 01:39:43PM +0200, Oliver Neukum wrote: > Hi, > > looking at the driver it looks to me like the address > provided to ndo_set_mac_address() is thrown away after use. > That looks problematic to me, because reset_resume() > should redo the operation. > What do you think? > > Regards > Oliver > From 19fc972a5cc98197bc81a7c56dd5d68e3fdfc36b Mon Sep 17 00:00:00 2001 > From: Oliver Neukum <oneukum@suse.com> > Date: Wed, 27 Jul 2022 13:29:42 +0200 > Subject: [PATCH] r8152: restore external MAC in reset_resume > > If user space has set the MAC of the interface, > reset_resume() must restore that setting rather > than redetermine the MAC like if te interface > is probed regularly. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > --- > drivers/net/usb/r8152.c | 34 +++++++++++++++++++++++++--------- > 1 file changed, 25 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c > index 0f6efaabaa32..5cf74b984655 100644 > --- a/drivers/net/usb/r8152.c > +++ b/drivers/net/usb/r8152.c > @@ -923,6 +923,7 @@ struct r8152 { > atomic_t rx_count; > > bool eee_en; > + bool external_mac; > int intr_interval; > u32 saved_wolopts; > u32 msg_enable; > @@ -933,6 +934,8 @@ struct r8152 { > u32 rx_copybreak; > u32 rx_pending; > u32 fc_pause_on, fc_pause_off; > + /* for reset_resume */ > + struct sockaddr saved_addr; > > unsigned int pipe_in, pipe_out, pipe_intr, pipe_ctrl_in, pipe_ctrl_out; > > @@ -1574,6 +1577,7 @@ static int __rtl8152_set_mac_address(struct net_device *netdev, void *p, > mutex_lock(&tp->control); > > eth_hw_addr_set(netdev, addr->sa_data); > + memcpy(&tp->saved_addr, addr, sizeof(tp->saved_addr)); Do you need a copy in tp? I would expect the MAC address stored in netdev by eth_hw_addr_set() is still there after the resume? Andrew
Oliver Neukum <oneukum@suse.com> > > Date: Wed, 27 Jul 2022 13:29:42 +0200 > > Subject: [PATCH] r8152: restore external MAC in reset_resume > > > > If user space has set the MAC of the interface, > > reset_resume() must restore that setting rather > > than redetermine the MAC like if te interface > > is probed regularly. I think this patch conflicts with commit 25766271e42f ("r8152: Refresh MAC address during USBDEVFS_RESET"). The results would be changed. Besides, I don't understand why you set tp->external_mac = false in rtl8152_down(). Best Regards, Hayes
On 28.07.22 10:40, Hayes Wang wrote: > Oliver Neukum <oneukum@suse.com> >>> Date: Wed, 27 Jul 2022 13:29:42 +0200 >>> Subject: [PATCH] r8152: restore external MAC in reset_resume >>> >>> If user space has set the MAC of the interface, >>> reset_resume() must restore that setting rather >>> than redetermine the MAC like if te interface >>> is probed regularly. > > I think this patch conflicts with commit 25766271e42f ("r8152: Refresh > MAC address during USBDEVFS_RESET"). The results would be changed. Argh. OK, thank you. Do you agree that a manually set MAC needs to be kept even through a pre/post_reset() and reset_resume(), while a MAC passed through needs to be reevaluated at pre/post_reset() but not at reset_resume() > Besides, I don't understand why you set tp->external_mac = false > in rtl8152_down(). Frankly I need to undo the effect of ndo_set_mac_address() at some time, but it is unclear to me how to return a network interface to its "native" MAC. Any ideas? Regards Oliver
Oliver Neukum <oneukum@suse.com> [...] > OK, thank you. Do you agree that a manually set MAC needs to be kept > even through a pre/post_reset() and reset_resume(), while a MAC passed > through needs to be reevaluated at pre/post_reset() but not at > reset_resume() Yes. I think a user wishes to keep it. > > Besides, I don't understand why you set tp->external_mac = false > > in rtl8152_down(). > > Frankly I need to undo the effect of ndo_set_mac_address() > at some time, but it is unclear to me how to return a network > interface to its "native" MAC. > Any ideas? I am not sure. Is it necessary? If I change the MAC address, I don't hope it is recovered unexpectedly unless the system is rebooted or shutdown. Best Regards, Hayes
From 19fc972a5cc98197bc81a7c56dd5d68e3fdfc36b Mon Sep 17 00:00:00 2001 From: Oliver Neukum <oneukum@suse.com> Date: Wed, 27 Jul 2022 13:29:42 +0200 Subject: [PATCH] r8152: restore external MAC in reset_resume If user space has set the MAC of the interface, reset_resume() must restore that setting rather than redetermine the MAC like if te interface is probed regularly. Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/net/usb/r8152.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index 0f6efaabaa32..5cf74b984655 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -923,6 +923,7 @@ struct r8152 { atomic_t rx_count; bool eee_en; + bool external_mac; int intr_interval; u32 saved_wolopts; u32 msg_enable; @@ -933,6 +934,8 @@ struct r8152 { u32 rx_copybreak; u32 rx_pending; u32 fc_pause_on, fc_pause_off; + /* for reset_resume */ + struct sockaddr saved_addr; unsigned int pipe_in, pipe_out, pipe_intr, pipe_ctrl_in, pipe_ctrl_out; @@ -1574,6 +1577,7 @@ static int __rtl8152_set_mac_address(struct net_device *netdev, void *p, mutex_lock(&tp->control); eth_hw_addr_set(netdev, addr->sa_data); + memcpy(&tp->saved_addr, addr, sizeof(tp->saved_addr)); ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CRWECR, CRWECR_CONFIG); pla_ocp_write(tp, PLA_IDR, BYTE_EN_SIX_BYTES, 8, addr->sa_data); @@ -1589,7 +1593,13 @@ static int __rtl8152_set_mac_address(struct net_device *netdev, void *p, static int rtl8152_set_mac_address(struct net_device *netdev, void *p) { - return __rtl8152_set_mac_address(netdev, p, false); + int rv; + struct r8152 *tp = netdev_priv(netdev); + + rv = __rtl8152_set_mac_address(netdev, p, false); + if (!rv) + tp->external_mac = true; + return rv; } /* Devices containing proper chips can support a persistent @@ -1676,10 +1686,14 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa) static int determine_ethernet_addr(struct r8152 *tp, struct sockaddr *sa) { struct net_device *dev = tp->netdev; - int ret; + int ret = 0; sa->sa_family = dev->type; + if (tp->external_mac) { + ether_addr_copy(sa->sa_data, dev->dev_addr); + return ret; + } ret = eth_platform_get_mac_address(&tp->udev->dev, sa->sa_data); if (ret < 0) { if (tp->version == RTL_VER_01) { @@ -1713,17 +1727,18 @@ static int determine_ethernet_addr(struct r8152 *tp, struct sockaddr *sa) static int set_ethernet_addr(struct r8152 *tp, bool in_resume) { struct net_device *dev = tp->netdev; - struct sockaddr sa; - int ret; + int ret = 0; - ret = determine_ethernet_addr(tp, &sa); - if (ret < 0) - return ret; + if (!tp->external_mac) { + ret = determine_ethernet_addr(tp, &tp->saved_addr); + if (ret < 0) + return ret; + } if (tp->version == RTL_VER_01) - eth_hw_addr_set(dev, sa.sa_data); + eth_hw_addr_set(dev, tp->saved_addr.sa_data); else - ret = __rtl8152_set_mac_address(dev, &sa, in_resume); + ret = __rtl8152_set_mac_address(dev, &tp->saved_addr, in_resume); return ret; } @@ -6225,6 +6240,7 @@ static void rtl8152_down(struct r8152 *tp) r8152_aldps_en(tp, false); r8152b_enter_oob(tp); r8152_aldps_en(tp, true); + tp->external_mac = false; } static void rtl8153_up(struct r8152 *tp) -- 2.35.3