| Message ID | 20221108153342.18979-1-albert.zhou.50@gmail.com |
|---|---|
| Headers | show |
| Series | Update r8152 to version two | expand |
On Wed, Nov 09, 2022 at 02:33:38AM +1100, Albert Zhou wrote: > Reverse commit 744d49daf8bd ("net: move netif_set_gso_max helpers") by > moving the functions netif_set_gso_max* back to netdevice.h so that the > updated R8152 v2 driver can use them. > > Signed-off-by: Albert Zhou <albert.zhou.50@gmail.com> > --- > include/linux/netdevice.h | 21 +++++++++++++++++++++ > net/core/dev.h | 21 --------------------- > 2 files changed, 21 insertions(+), 21 deletions(-) No, use the helpers that are there to be used instead. This should not be needed to be done, fix up the driver you are adding instead. thanks, greg k-h
Albert Zhou <albert.zhou.50@gmail.com> : > This patch integrates the version-two r8152 drivers from Realtek into > the kernel. I am new to kernel development, so apologies if I make > newbie mistakes. While it makes sense to minimize differences betwenn Realtek's in-house driver and kernel r8152 driver, it does not mean that the out-of-tree driver is suitable for a straight kernel inclusion. If you want things to move forward in a not too painful way, you should split the more than 650 ko patch into smaller, more focused patches (huge patches also makes bisection mildly effective btw). In its current form, the submission is imho a bit abrasive to review. [...] > Albert Zhou (5): > net: move back netif_set_gso_max helpers > r8152: update to version two This code misuses mutex in {read, write}_mii_word. It includes code and data that should be moved to firmware files. > r8152: remove backwards compatibility Backwards compatibility code should had been avoided in the first place. [...] > r8152: remove redundant code Same thing.
On 9/11/22 07:50, Jakub Kicinski wrote: > On Wed, 9 Nov 2022 02:33:37 +1100 Albert Zhou wrote: >> This patch integrates the version-two r8152 drivers from Realtek into >> the kernel. I am new to kernel development, so apologies if I make >> newbie mistakes. >> >> I have tested the updated module in v6.1 on my machine, without any >> issues. > > What are you trying to achieve? Copy pasting 18k LoC into the kernel > in a single patch is definitely not the way we do development. If there > are features missing in the upstream driver, or you see discrepancies > in the operation - please prepare targeted patches. Hi Jakub, The version-one r8152 module, for some reason, cannot maintain high data-transfer speeds. I personally experienced this problem myself, when I bought a new USB-C to ethernet adapter. The version-two module fixes this issue. I was searching for a way to include version two in the kernel, so that future users would not have this issue. Being new to this process, I na�vely contacted Greg, who advised me to send a patch through. Unfortunately my attempts have not met your standard. I will try and persist because I would like to improve the kernel in the best way. Sincerely, Albert Zhou
On 9/11/22 04:28, Greg KH wrote: >> // SPDX-License-Identifier: GPL-2.0-only >> /* >> - * Copyright (c) 2014 Realtek Semiconductor Corp. All rights reserved. >> + * Copyright (c) 2021 Realtek Semiconductor Corp. All rights reserved. >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License >> + * version 2 as published by the Free Software Foundation. > To start with, this is not correct. Don't add back license boiler-plate > code. Hi Greg, My apologies, I was unaware of this. This can be easily removed. > > And you just changed the copyright notice incorrectly, that is not ok. > When I replaced the version-one code with the version-two code, I assumed the authors' copyright would be correct. What is the correct copyright notice? >> + * >> + * This product is covered by one or more of the following patents: >> + * US6,570,884, US6,115,776, and US6,327,625. > Oh wow. That's playing with fire... Do you believe this prohibits the code from being in the kernel? I had assumed these refer to the hardware, the ethernet adapter, since the code is covered by GPLv2. > > thanks, > > greg k-h
On 9/11/22 08:03, Francois Romieu wrote: > Albert Zhou <albert.zhou.50@gmail.com> : >> This patch integrates the version-two r8152 drivers from Realtek into >> the kernel. I am new to kernel development, so apologies if I make >> newbie mistakes. > > While it makes sense to minimize differences betwenn Realtek's in-house > driver and kernel r8152 driver, it does not mean that the out-of-tree > driver is suitable for a straight kernel inclusion. > > If you want things to move forward in a not too painful way, you should > split the more than 650 ko patch into smaller, more focused patches > (huge patches also makes bisection mildly effective btw). > > In its current form, the submission is imho a bit abrasive to review. > Hi Francois, Thanks for the feedback. As I was not involved in the development, it will be difficult for me to achieve this. I will try contact the developers at Realtek to see if they can assist. I was under the false impression, that indeed the Realtek driver could simply be included in the kernel. But now I see that is not the case. Best, Albert Zhou > [...] >> Albert Zhou (5): >> net: move back netif_set_gso_max helpers >> r8152: update to version two > > This code misuses mutex in {read, write}_mii_word. > > It includes code and data that should be moved to firmware files. > >> r8152: remove backwards compatibility > > Backwards compatibility code should had been avoided in the first > place. > > [...] >> r8152: remove redundant code > > Same thing. >
On Wed, Nov 09, 2022 at 03:50:59PM +1100, Albert Zhou wrote: > On 9/11/22 04:28, Greg KH wrote: > > > // SPDX-License-Identifier: GPL-2.0-only > > > /* > > > - * Copyright (c) 2014 Realtek Semiconductor Corp. All rights reserved. > > > + * Copyright (c) 2021 Realtek Semiconductor Corp. All rights reserved. > > > + * > > > + * This program is free software; you can redistribute it and/or > > > + * modify it under the terms of the GNU General Public License > > > + * version 2 as published by the Free Software Foundation. > > To start with, this is not correct. Don't add back license boiler-plate > > code. > > Hi Greg, > > My apologies, I was unaware of this. This can be easily removed. > > > > > And you just changed the copyright notice incorrectly, that is not ok. > > > > When I replaced the version-one code with the version-two code, I assumed > the authors' copyright would be correct. What is the correct copyright > notice? The correct way would be to list all years that the copyright was asserted for the file. Your patch removed the copyright notice for an older year, which isn't ok. But the larger issue here is that just wholesale replacing the in-tree driver with an out-of-tree one isn't going to work. As others have pointed out, you need to break the changes up into one-patch-per-logical-change and drag the driver forward that way. The easiest way for you to do this is to clean up the out-of-tree driver on its own, removing all the backwards compatibility stuff, and then try to figure out what features are different and add them to the in-kernel driver, one by one. It's not an easy task, but as you have the hardware to test with, should be doable. good luck! > > > + * > > > + * This product is covered by one or more of the following patents: > > > + * US6,570,884, US6,115,776, and US6,327,625. > > Oh wow. That's playing with fire... > > Do you believe this prohibits the code from being in the kernel? No I do not. It's just not something that is normally advertised in the kernel for obvious reasons :) thanks, greg k-h
Albert Zhou <albert.zhou.50@gmail.com> > Sent: Tuesday, November 8, 2022 11:34 PM > Subject: [PATCH net-next RFC 0/5] Update r8152 to version two > > This patch integrates the version-two r8152 drivers from Realtek into > the kernel. I am new to kernel development, so apologies if I make > newbie mistakes. The Realtek's in-house driver doesn't satisfy the rules or requests of Linux kernel, so I don't think you could use it to replace the Linux kernel r8152 driver. The version is used to distinguish between the Realtek's in-house driver and Linux kernel r8152 driver. It doesn't mean you have to use version 2, even the version 2 may contain more experiment features. Best Regards, Hayes > I have tested the updated module in v6.1 on my machine, without any > issues. > > A final note, when I removed all the code for earlier kernel versions, > the header r8152_compatibility.h reduced dramatically in size. This > leads me to suspect that some of the headers like <linux/init.h> are no > longer needed. However, I left them in there just in case.
On 9/11/22 22:02, Hayes Wang wrote: > Albert Zhou<albert.zhou.50@gmail.com> >> Sent: Tuesday, November 8, 2022 11:34 PM >> Subject: [PATCH net-next RFC 0/5] Update r8152 to version two >> >> This patch integrates the version-two r8152 drivers from Realtek into >> the kernel. I am new to kernel development, so apologies if I make >> newbie mistakes. > The Realtek's in-house driver doesn't satisfy the rules or requests of > Linux kernel, so I don't think you could use it to replace the Linux > kernel r8152 driver. > > The version is used to distinguish between the Realtek's in-house driver > and Linux kernel r8152 driver. It doesn't mean you have to use version 2, > even the version 2 may contain more experiment features. > > Best Regards, > Hayes > Thanks for clarifying this, and thanks Greg for your advice in the previous message. I will follow Greg's advice and try and identify that bits of code that fixes my issue of having throttled data-transfer speed via ethernet. If you have some kind of in-house changelog that you could make available to me, I would be grateful. Best, Albert
On Wed, 9 Nov 2022 15:43:21 +1100 Albert Zhou wrote: > The version-one r8152 module, for some reason, cannot maintain high > data-transfer speeds. I personally experienced this problem myself, when > I bought a new USB-C to ethernet adapter. The version-two module fixes > this issue. I see, perhaps it'd be possible to zero in on how the datapath of the driver is implemented?
On 10/11/22 05:40, Jakub Kicinski wrote: > On Wed, 9 Nov 2022 15:43:21 +1100 Albert Zhou wrote: >> The version-one r8152 module, for some reason, cannot maintain high >> data-transfer speeds. I personally experienced this problem myself, when >> I bought a new USB-C to ethernet adapter. The version-two module fixes >> this issue. > > I see, perhaps it'd be possible to zero in on how the datapath of > the driver is implemented? Hi all, After a lot of testing, I have come to the conclusion that the reason for the speed difference between the v1 and v2 driver is actually the firmware. The v2 driver doesn't even #include <linux/firmware.h>; so it doesn't load the standard firmware in /lib/firmware/rtl_nic. It seems the firmware is actually written in the source and loaded directly. Since firmware is not even part of the kernel, it's probably inappropriate to submit a patch that modifies the firmware of a device, would that be correct? If that's the case, it's probably best if Realtek can update the firmware on the linux firmware git. Not sure otherwise how to proceed. Any suggestions? Best, Albert
On Thu, 17 Nov 2022 02:13:49 +1100 Albert Zhou wrote: > After a lot of testing, I have come to the conclusion that the reason > for the speed difference between the v1 and v2 driver is actually the > firmware. > > The v2 driver doesn't even #include <linux/firmware.h>; so it doesn't > load the standard firmware in /lib/firmware/rtl_nic. It seems the > firmware is actually written in the source and loaded directly. > > Since firmware is not even part of the kernel, it's probably > inappropriate to submit a patch that modifies the firmware of a device, > would that be correct? > > If that's the case, it's probably best if Realtek can update the > firmware on the linux firmware git. > > Not sure otherwise how to proceed. Any suggestions? Getting Realtek to update the FW would definitely be the best avenue. If that fails you can try to update it yourself, but it's always a bit tricky because of licensing. You may need to invent your own file format :S