* handling MAC set by user space in reset_resume() of r8152
@ 2022-07-27 11:39 Oliver Neukum
2022-07-27 18:34 ` Andrew Lunn
0 siblings, 1 reply; 5+ messages in thread
From: Oliver Neukum @ 2022-07-27 11:39 UTC (permalink / raw)
To: Hayes Wang; +Cc: USB list, netdev
[-- Attachment #1: Type: text/plain, Size: 236 bytes --]
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
[-- Attachment #2: 0001-r8152-restore-external-MAC-in-reset_resume.patch --]
[-- Type: text/x-patch, Size: 3349 bytes --]
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: handling MAC set by user space in reset_resume() of r8152
2022-07-27 11:39 handling MAC set by user space in reset_resume() of r8152 Oliver Neukum
@ 2022-07-27 18:34 ` Andrew Lunn
2022-07-28 8:40 ` Hayes Wang
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Lunn @ 2022-07-27 18:34 UTC (permalink / raw)
To: Oliver Neukum; +Cc: Hayes Wang, USB list, netdev
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: handling MAC set by user space in reset_resume() of r8152
2022-07-27 18:34 ` Andrew Lunn
@ 2022-07-28 8:40 ` Hayes Wang
2022-07-28 8:54 ` Oliver Neukum
0 siblings, 1 reply; 5+ messages in thread
From: Hayes Wang @ 2022-07-28 8:40 UTC (permalink / raw)
To: Oliver Neukum; +Cc: USB list, netdev
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: handling MAC set by user space in reset_resume() of r8152
2022-07-28 8:40 ` Hayes Wang
@ 2022-07-28 8:54 ` Oliver Neukum
2022-07-28 9:41 ` Hayes Wang
0 siblings, 1 reply; 5+ messages in thread
From: Oliver Neukum @ 2022-07-28 8:54 UTC (permalink / raw)
To: Hayes Wang, Oliver Neukum; +Cc: USB list, netdev
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: handling MAC set by user space in reset_resume() of r8152
2022-07-28 8:54 ` Oliver Neukum
@ 2022-07-28 9:41 ` Hayes Wang
0 siblings, 0 replies; 5+ messages in thread
From: Hayes Wang @ 2022-07-28 9:41 UTC (permalink / raw)
To: Oliver Neukum; +Cc: USB list, netdev
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-28 9:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-27 11:39 handling MAC set by user space in reset_resume() of r8152 Oliver Neukum
2022-07-27 18:34 ` Andrew Lunn
2022-07-28 8:40 ` Hayes Wang
2022-07-28 8:54 ` Oliver Neukum
2022-07-28 9:41 ` Hayes Wang
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).