| Message ID | 20220728191851.30402-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC] r8152: pass through needs to be singular | expand |
On Thu, Jul 28, 2022 at 09:18:51PM +0200, Oliver Neukum wrote: > If multiple devices are connected only one of them > is allowed to get the pass through MAC Is that true? Ethernet switches often use the same MAC address on multiple ports. It is not inherently broken to have the same MAC address on multiple interfaces. We also need to watch out for regressions. There could be users who do have the same MAC address on multiple interfaces, and it works for them. With this change it is a 50/50 chance they no longer get the MAC address they expect, depending on probe order. We know this implementation of pass through it very broken, but unfortunately, it is there, and we have to follow the normal regression rules, even if we really would like to throw it all out. What exactly is your problem which you are trying to fix? Andrew
On 29.07.22 00:11, Andrew Lunn wrote: > On Thu, Jul 28, 2022 at 09:18:51PM +0200, Oliver Neukum wrote: >> If multiple devices are connected only one of them >> is allowed to get the pass through MAC > > Is that true? Ethernet switches often use the same MAC address on > multiple ports. It is not inherently broken to have the same MAC > address on multiple interfaces. Well, yes, but for a host to assigning the same MAC by default is a bug. The whole point of MACs is kind of that they be unique. For the same reason I fixed usbnet to not hand out the same random address. In theory I have broken setups which relied on people reconnecting a device getting the same MAC. > We know this implementation of pass through it very broken, but > unfortunately, it is there, and we have to follow the normal > regression rules, even if we really would like to throw it all out. True. Nevertheless, do we really want to say that we dislike a design so much that we are not fixing bugs? > What exactly is your problem which you are trying to fix? Adressing the comment Hayes made when reset_resume() was fixed from a deadlock, that it still assigns wrong MACs. I feel that before I fix keeping the correct address I better make sure the MAC is sane in the first place. Regards Oliver
> True. Nevertheless, do we really want to say that we dislike a design > so much that we are not fixing bugs? I'm not sure we can fix it. Part of that long thread about why this whole concept is broken is that we have no idea which interface is the one which should give the MAC address to. If we change it to only give out the MAC address once, all we really do is change it from one bug to another bug. > > What exactly is your problem which you are trying to fix? > Adressing the comment Hayes made when reset_resume() was fixed > from a deadlock, that it still assigns wrong MACs. I feel that > before I fix keeping the correct address I better make sure the > MAC is sane in the first place. I would say that reset_resume() should restore whatever the MAC address was before the suspend. It does not matter if the MAC address is not unique. As far as i know, the kernel never prevents the user assigning the same MAC address on multiple interfaces via ip link set. So it could actually be a user choice. Andrew
Oliver Neukum <oneukum@suse.com> > Sent: Friday, July 29, 2022 3:19 AM [...] > @@ -1608,6 +1622,12 @@ static int vendor_mac_passthru_addr_read(struct > r8152 *tp, struct sockaddr *sa) > acpi_object_type mac_obj_type; > int mac_strlen; > > + mutex_lock(&pass_through_lock); > + > + if (!holder_of_pass_through) { > + ret = -EBUSY; > + goto failout; > + } Excuse me. I have one question. When is the holder_of_pass_through set? The default value of holder_of_pass_through is NULL, so it seems the holder_of_pass_through would never be set. Best Regards, Hayes > if (tp->lenovo_macpassthru) { > mac_obj_name = "\\MACA"; > mac_obj_type = ACPI_TYPE_STRING; > @@ -1621,7 +1641,8 @@ static int vendor_mac_passthru_addr_read(struct > r8152 *tp, struct sockaddr *sa) > if ((ocp_data & PASS_THRU_MASK) != 1) { > netif_dbg(tp, probe, tp->netdev, > "No efuse for RTL8153-AD MAC pass through\n"); > - return -ENODEV; > + ret = -ENODEV; > + goto failout; > } > } else { > /* test for RTL8153-BND and RTL8153-BD */ > @@ -1629,7 +1650,8 @@ static int vendor_mac_passthru_addr_read(struct > r8152 *tp, struct sockaddr *sa) > if ((ocp_data & BND_MASK) == 0 && (ocp_data & BD_MASK) > == 0) { > netif_dbg(tp, probe, tp->netdev, > "Invalid variant for MAC pass through\n"); > - return -ENODEV; > + ret = -ENODEV; > + goto failout; > } > } > > @@ -1641,8 +1663,10 @@ static int vendor_mac_passthru_addr_read(struct > r8152 *tp, struct sockaddr *sa) > /* returns _AUXMAC_#AABBCCDDEEFF# */ > status = acpi_evaluate_object(NULL, mac_obj_name, NULL, &buffer); > obj = (union acpi_object *)buffer.pointer; > - if (!ACPI_SUCCESS(status)) > - return -ENODEV; > + if (!ACPI_SUCCESS(status)) { > + ret = -ENODEV; > + goto failout; > + } > if (obj->type != mac_obj_type || obj->string.length != mac_strlen) { > netif_warn(tp, probe, tp->netdev, > "Invalid buffer for pass-thru MAC addr: (%d, %d)\n", > @@ -1670,6 +1694,10 @@ static int vendor_mac_passthru_addr_read(struct > r8152 *tp, struct sockaddr *sa) > > amacout: > kfree(obj); > +failout: > + if (!ret) > + holder_of_pass_through = tp; > + mutex_unlock(&pass_through_lock); > return ret; > }
On 03.08.22 05:48, Hayes Wang wrote: > Oliver Neukum <oneukum@suse.com> >> Sent: Friday, July 29, 2022 3:19 AM > [...] >> @@ -1608,6 +1622,12 @@ static int vendor_mac_passthru_addr_read(struct >> r8152 *tp, struct sockaddr *sa) >> acpi_object_type mac_obj_type; >> int mac_strlen; >> >> + mutex_lock(&pass_through_lock); >> + >> + if (!holder_of_pass_through) { >> + ret = -EBUSY; >> + goto failout; >> + } > > Excuse me. I have one question. > When is the holder_of_pass_through set? > The default value of holder_of_pass_through is NULL, so > it seems the holder_of_pass_through would never be set. Hi, here in vendor_mac_passthru_addr_read() >> amacout: >> kfree(obj); >> +failout: >> + if (!ret) >> + holder_of_pass_through = tp; >> + mutex_unlock(&pass_through_lock); >> return ret; >> } Regards Oliver
On 02.08.22 15:31, Andrew Lunn wrote: >> True. Nevertheless, do we really want to say that we dislike a design >> so much that we are not fixing bugs? > > I'm not sure we can fix it. Part of that long thread about why this > whole concept is broken is that we have no idea which interface is the > one which should give the MAC address to. If we change it to only give > out the MAC address once, all we really do is change it from one bug > to another bug. I am afraid I have to beg to differ. We have a couple of related issues here. Obviously whoever designed MAC pass-through did not consider the case of somebody using two docking stations at the same time. While pass-through is used I agree that this is unsolvable. Yet connecting two (or even more) docking stations to a host is within spec. They are USB devices (partially) and if they contain a NIC it is clear what we have to do. We would operate them with 1. a MAC contained on the device 2. if the device contains no MAC, we'd use a random MAC, but a different one per device. User space can assign any MAC it wants to, but we are talking defaults here. Now, the question arises whether we let another feature interfere with that. And then I must say, if we do that and we have decided to take a feature that does so, we'll have to do it in a way that stays within spec. Yet I am sorry, you cannot give out the same MAC to two devices at the same time, if you want to do so. So while the bug in the driver derives from a stupid design, it nevertheless is a bug and my patch fixes it. Optimally? No, of course not, the design is broken. >>> What exactly is your problem which you are trying to fix? >> Adressing the comment Hayes made when reset_resume() was fixed >> from a deadlock, that it still assigns wrong MACs. I feel that >> before I fix keeping the correct address I better make sure the >> MAC is sane in the first place. > > I would say that reset_resume() should restore whatever the MAC > address was before the suspend. The problem is that these devices reread the passed through MAC at post_reset(). Do you want reset_resume() to act differently? > It does not matter if the MAC address > is not unique. As far as i know, the kernel never prevents the user > assigning the same MAC address on multiple interfaces via ip link set. > So it could actually be a user choice. Iff the user does so. Until then it is a kernel bug. Regards Oliver
> I am afraid I have to beg to differ. > > We have a couple of related issues here. Obviously whoever designed > MAC pass-through did not consider the case of somebody using two > docking stations at the same time. While pass-through is used I agree > that this is unsolvable. > > Yet connecting two (or even more) docking stations to a host is > within spec. They are USB devices (partially) and if they contain > a NIC it is clear what we have to do. > We would operate them with > 1. a MAC contained on the device > 2. if the device contains no MAC, we'd use a random MAC, but a > different one per device. User space can assign any MAC it wants to, > but we are talking defaults here. > > Now, the question arises whether we let another feature interfere > with that. And then I must say, if we do that and we have decided > to take a feature that does so, we'll have to do it in a way that > stays within spec. Yet I am sorry, you cannot give out the same > MAC to two devices at the same time, if you want to do so. > > So while the bug in the driver derives from a stupid design, > it nevertheless is a bug and my patch fixes it. Optimally? > No, of course not, the design is broken. The problem is regressions. Current code will put the MAC address on both. I guess most users just have one dock with a cabled and the second is unused. Both will get the same MAC address, the DHCP server will recognise the MAC address and give out the expected IP address. If you change it to only give the MAC address out once, you get into a race condition, which device probes first, gets the MAC, and gives the expected MAC to the DHCP server? You are going to see regressions. > >>> What exactly is your problem which you are trying to fix? > >> Adressing the comment Hayes made when reset_resume() was fixed > >> from a deadlock, that it still assigns wrong MACs. I feel that > >> before I fix keeping the correct address I better make sure the > >> MAC is sane in the first place. > > > > I would say that reset_resume() should restore whatever the MAC > > address was before the suspend. > > The problem is that these devices reread the passed through MAC > at post_reset(). Do you want reset_resume() to act differently? I would expect whatever MAC address is in the netdev structure to be put on the interface at resume. That should of been the MAC it was using before suspend. And by doing that, you bypass all the discussion about where it came from. Andrew
On 04.08.22 15:24, Andrew Lunn wrote: > The problem is regressions. Current code will put the MAC address on > both. I guess most users just have one dock with a cabled and the > second is unused. Both will get the same MAC address, the DHCP server > will recognise the MAC address and give out the expected IP address. 1. That is a rather esoteric situation in which a bug is exploited 2. We are at a philosophical point where I need to argue that fixing a bug is necessarily a change in behavior. > I would expect whatever MAC address is in the netdev structure to be > put on the interface at resume. That should of been the MAC it was > using before suspend. And by doing that, you bypass all the discussion > about where it came from. Debatable, but that's not what the driver does. It reacquires the MAC from the firmware. I want to change that. Though obviously that changes behavior. I am sorry, but at some point a bug is a bug, even if some people like it. Regards Oliver
> I am sorry, but at some point a bug is a bug, even if some people like > it. So i guess we go with what we said last time. We can merge a change, but if anybody reports a regression, it will be reverted. And if i remember correctly, we did revert some stuff in this area. But i think it would be better to consider RTL8153-BND, RTL8153-BD and RTL8153-AD as poisoned, and make new designs with a different device. Andrew
Oliver Neukum <oneukum@suse.com> > Sent: Thursday, August 4, 2022 4:58 PM [...] > >> + if (!holder_of_pass_through) { > >> + ret = -EBUSY; > >> + goto failout; > >> + } > > > > Excuse me. I have one question. > > When is the holder_of_pass_through set? > > The default value of holder_of_pass_through is NULL, so > > it seems the holder_of_pass_through would never be set. > > > Hi, > > here in vendor_mac_passthru_addr_read() I mean that holder_of_pass_through is NULL, so you set ret = -EBUSY and goto failout. However, holder_of_pass_through is set only if ret == 0. That is, holder_of_pass_through = tp would never occur, because ret is equal -EBUSY. The default value of holder_of_pass_through is NULL, so it has no chance to be set. Right? Best Regards, Hayes > >> amacout: > >> kfree(obj); > >> +failout: > >> + if (!ret) > >> + holder_of_pass_through = tp; > >> + mutex_unlock(&pass_through_lock); > >> return ret; > >> }
On 08.08.22 09:04, Hayes Wang wrote: > Oliver Neukum <oneukum@suse.com> >> Sent: Thursday, August 4, 2022 4:58 PM > [...] >>>> + if (!holder_of_pass_through) { >>>> + ret = -EBUSY; >>>> + goto failout; >>>> + } >>> >>> Excuse me. I have one question. >>> When is the holder_of_pass_through set? >>> The default value of holder_of_pass_through is NULL, so >>> it seems the holder_of_pass_through would never be set. >> >> >> Hi, >> >> here in vendor_mac_passthru_addr_read() > > I mean that holder_of_pass_through is NULL, so you set ret = -EBUSY and goto failout. > However, holder_of_pass_through is set only if ret == 0. That is, > holder_of_pass_through = tp would never occur, because ret is equal -EBUSY. > The default value of holder_of_pass_through is NULL, so it has no chance to be set. > Right? > Hi, sorry, you are right. I made a "!" in error. I'll correct that. Regards Oliver
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index 0f6efaabaa32..15f695398284 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -1193,6 +1193,9 @@ static unsigned int agg_buf_sz = 16384; #define RTL_LIMITED_TSO_SIZE (size_to_mtu(agg_buf_sz) - sizeof(struct tx_desc)) +static struct r8152 *holder_of_pass_through = NULL; +static DEFINE_MUTEX(pass_through_lock); + static int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void *data) { @@ -1587,8 +1590,19 @@ static int __rtl8152_set_mac_address(struct net_device *netdev, void *p, return ret; } +static void give_up_pass_through(struct r8152 *tp) +{ + mutex_lock(&pass_through_lock); + if (tp == holder_of_pass_through) + holder_of_pass_through = NULL; + mutex_unlock(&pass_through_lock); +} + static int rtl8152_set_mac_address(struct net_device *netdev, void *p) { + struct r8152 *tp = netdev_priv(netdev); + + give_up_pass_through(tp); return __rtl8152_set_mac_address(netdev, p, false); } @@ -1608,6 +1622,12 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa) acpi_object_type mac_obj_type; int mac_strlen; + mutex_lock(&pass_through_lock); + + if (!holder_of_pass_through) { + ret = -EBUSY; + goto failout; + } if (tp->lenovo_macpassthru) { mac_obj_name = "\\MACA"; mac_obj_type = ACPI_TYPE_STRING; @@ -1621,7 +1641,8 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa) if ((ocp_data & PASS_THRU_MASK) != 1) { netif_dbg(tp, probe, tp->netdev, "No efuse for RTL8153-AD MAC pass through\n"); - return -ENODEV; + ret = -ENODEV; + goto failout; } } else { /* test for RTL8153-BND and RTL8153-BD */ @@ -1629,7 +1650,8 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa) if ((ocp_data & BND_MASK) == 0 && (ocp_data & BD_MASK) == 0) { netif_dbg(tp, probe, tp->netdev, "Invalid variant for MAC pass through\n"); - return -ENODEV; + ret = -ENODEV; + goto failout; } } @@ -1641,8 +1663,10 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa) /* returns _AUXMAC_#AABBCCDDEEFF# */ status = acpi_evaluate_object(NULL, mac_obj_name, NULL, &buffer); obj = (union acpi_object *)buffer.pointer; - if (!ACPI_SUCCESS(status)) - return -ENODEV; + if (!ACPI_SUCCESS(status)) { + ret = -ENODEV; + goto failout; + } if (obj->type != mac_obj_type || obj->string.length != mac_strlen) { netif_warn(tp, probe, tp->netdev, "Invalid buffer for pass-thru MAC addr: (%d, %d)\n", @@ -1670,6 +1694,10 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa) amacout: kfree(obj); +failout: + if (!ret) + holder_of_pass_through = tp; + mutex_unlock(&pass_through_lock); return ret; } @@ -8287,6 +8315,7 @@ static int rtl8152_pre_reset(struct usb_interface *intf) tp->rtl_ops.disable(tp); mutex_unlock(&tp->control); } + give_up_pass_through(tp); return 0; } @@ -9802,6 +9831,7 @@ static void rtl8152_disconnect(struct usb_interface *intf) cancel_delayed_work_sync(&tp->hw_phy_work); if (tp->rtl_ops.unload) tp->rtl_ops.unload(tp); + give_up_pass_through(tp); rtl8152_release_firmware(tp); free_netdev(tp->netdev); }
If multiple devices are connected only one of them is allowed to get the pass through MAC Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/net/usb/r8152.c | 38 ++++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-)