netdev - Re: [PATCH net-next 07/13] net: dsa: rtl8365mb: rename rtl8365mb to rtl8367c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 17 Dec 2021 12:15:07 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Luiz Angelo Daros de Luca <luizluca@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "olteanv@...il.com" <olteanv@...il.com>,
        "arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next 07/13] net: dsa: rtl8365mb: rename rtl8365mb to
 rtl8367c

On 12/17/21 08:24, Luiz Angelo Daros de Luca wrote:
>> Hi Luiz,
> Hi Alvin,
> 
>> On 12/16/21 21:13, luizluca@...il.com wrote:
>>> From: Luiz Angelo Daros de Luca <luizluca@...il.com>
>>>
>>> rtl8365mb refers to a single device supported by the driver.
>>> The rtl8367c does not refer to any real device, but it is the
>>> driver version name used by Realtek.
>>>
>>> Tested-by: Arınç ÜNAL <arinc.unal@...nc9.com>
>>> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
>>> ---
>>>    drivers/net/dsa/realtek/Kconfig       |    9 +-
>>>    drivers/net/dsa/realtek/Makefile      |    2 +-
>>>    drivers/net/dsa/realtek/realtek-smi.c |    4 +-
>>>    drivers/net/dsa/realtek/realtek.h     |    2 +-
>>>    drivers/net/dsa/realtek/rtl8367c.c    | 1321 ++++++++++++-------------
>>>    drivers/net/phy/realtek.c             |    2 +-
>>>    6 files changed, 666 insertions(+), 674 deletions(-)
>>>
>>
>> Is the rename really necessary? My logic in naming it rtl8365mb was
>> simply that it was the first hardware to be supported by the driver,
>> which was more meaningful than Realtek's fictitious rtl8367c. This seems
>> to be common practice in the kernel, and bulk renames don't normally
>> bring much value.
>>
>> I think the vendor's naming scheme is confusing at best, so it's better
>> to stick to real things in the kernel.
> 
> Yes, it is quite confusing. I just know that the last digit is the
> number of ports and NB
> seems to indicate a switch that does not "switch" (user ports does not
> share a broadcast
> domain). RTL8365MB-VC does seem to be the first one "switch" in the
> rtl8367c supported list.
> 
> I don't have any strong preference which name it will have. I'm really
> not an expert in
> the Realtek product line. My guess is that Realtek has some kind of
> "driver/device generation",

I don't think the current name is really hurting anybody. And as Arınç 
says, it’s a huge chunk of change with no real benefits.

Besides, not all renaming seems consistent - what is mb to mean here?:

-	struct rtl8365mb *mb = priv->chip_data;
+	struct rtl8367c *mb = priv->chip_data;

The naming scheme for rtl8366rb is also mysterious - why not fix that 
too? Is that rtl8367b?

Honestly it seems like more effort than it is worth. The comments at the 
top of the driver should be sufficient to explain to any future 
developer what exactly is going on. If something is unclear, why not 
just add/remove some lines there?

Since you don't feel strongly about the name, I would suggest you drop 
the renaming from your MDIO/RTL8367S series for now. It will also make 
the review process a bit easier.

	Alvin

> because I also see rtl8367b, and rtl8367d. I used rtl8367c as it is
> the name used by Realtek
> API and Programming Guide. I saw it referenced in, arduino and uboot
> and out-of-tree linux
> drivers. I really don't know the best name but, if we use a real
> product name, I suggest using
> the full name, including suffixes because Realtek could launch a new
> RTL8365MB (with a
> different suffix or even without one) for a different incompatible
> chip. And that will be even more
> confusing. We could also create our own fake sequence (rtl83xx-1,
> rtl83xx-2,...) but it is normally
> better to adopt an in-use standard than to create a new one.
> 
> I do care about names but I simply don't have the knowledge to have a
> say. I think there are
> plenty of experts on this list. I would also love to hear from someone
> from Realtek to suggest
>   a good name. Does anyone have a contact?
> 
> Regards