Re: [PATCH] net: usb: r8152: Add MAC passthrough support for more Lenovo Docks

[Henning Schild]

Am Wed, 5 Jan 2022 01:40:42 +0800
schrieb Aaron Ma <aaron.ma@xxxxxxxxxxxxx>:

> On 1/5/22 01:07, Henning Schild wrote:
> > Am Tue, 4 Jan 2022 06:53:26 -0800
> > schrieb Jakub Kicinski <kuba@xxxxxxxxxx>:
> >   
> >> On Tue, 4 Jan 2022 12:38:14 +0100 Henning Schild wrote:  
> >>> This patch is wrong and taking the MAC inheritance way too far.
> >>> Now any USB Ethernet dongle connected to a Lenovo USB Hub will go
> >>> into inheritance (which is meant for docks).
> >>>
> >>> It means that such dongles plugged directly into the laptop will
> >>> do that, or travel adaptors/hubs which are not "active docks".
> >>>
> >>> I have USB-Ethernet dongles on two desks and both stopped working
> >>> as expected because they took the main MAC, even with it being
> >>> used at the same time. The inheritance should (if at all) only be
> >>> done for clearly identified docks and only for one r8152 instance
> >>> ... not all. Maybe even double checking if that main PHY is
> >>> "plugged" and monitoring it to back off as soon as it is.
> >>>
> >>> With this patch applied users can not use multiple ethernet
> >>> devices anymore ... if some of them are r8152 and connected to
> >>> "Lenovo" ... which is more than likely!
> >>>
> >>> Reverting that patch solved my problem, but i later went to
> >>> disabling that very questionable BIOS feature to disable things
> >>> for good without having to patch my kernel.
> >>>
> >>> I strongly suggest to revert that. And if not please drop the
> >>> defines of  
> >>>> -		case DEVICE_ID_THINKPAD_THUNDERBOLT3_DOCK_GEN2:
> >>>> -		case DEVICE_ID_THINKPAD_USB_C_DOCK_GEN2:  
> >>>
> >>> And instead of crapping out with "(unnamed net_device)
> >>> (uninitialized): Invalid header when reading pass-thru MAC addr"
> >>> when the BIOS feature is turned off, one might want to check
> >>> DSDT/WMT1/ITEM/"MACAddressPassThrough" which is my best for asking
> >>> the BIOS if the feature is wanted.  
> >>
> >> Thank you for the report!
> >>
> >> Aaron, will you be able to fix this quickly? 5.16 is about to be
> >> released.  
> > 
> > If you guys agree with a revert and potentially other actions, i
> > would be willing to help. In any case it is not super-urgent since
> > we can maybe agree an regression and push it back into stable
> > kernels.
> > 
> > I first wanted to place the report and see how people would react
> > ... if you guys agree that this is a bug and the inheritance is
> > going "way too far".
> > 
> > But i would only do some repairs on the surface, the feature itself
> > is horrific to say the least and i am very happy with that BIOS
> > switch to ditch it for good. Giving the MAC out is something a dock
> > physically blocking the original PHY could do ... but year ... only
> > once and it might be pretty hard to say which r8152 is built-in
> > from the hub and which is plugged in additionally in that very hub.
> > Not to mention multiple hubs of the same type ... in a nice USB-C
> > chain. 
> 
> Yes, it's expected to be a mess if multiple r8152 are attached to
> Lenovo USB-C/TBT docks. The issue had been discussed for several
> times in LKML. Either lose this feature or add potential risk for
> multiple r8152.
> 
> The idea is to make the Dock work which only ship with one r8152.
> It's really hard to say r8152 is from dock or another plugin one.
> 
> If revert this patch, then most users with the original shipped dock
> may lose this feature. That's the problem this patch try to fix.
> 
> For now I suggest to disable it in BIOS if you got multiple r8152.

I can do that. But as i expect that to be coming from "managed devices"
where IT departments dream that they can identify a machine by its MAC
... those uses likely can not.

So there should maybe be an additional module param, even if it
defaults to "on" as well.

Henning

> Let me try to make some changes to limit this feature in one r8152.
> 
> Aaron
> 
> 
> > MAC spoofing is something NetworkManager and others can take care
> > of, or udev ... doing that in the driver is ... spooky.
> > 
> > regards,
> > Henning