| 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
| ||
|
Date: Sun, 4 Jun 2023 19:19:45 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Christian Lamparter <chunkeey@...il.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "luizluca@...il.com"
<luizluca@...il.com>, "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
"andrew@...n.ch" <andrew@...n.ch>, "olteanv@...il.com" <olteanv@...il.com>,
"f.fainelli@...il.com" <f.fainelli@...il.com>
Subject: Re: [PATCH v1] net: dsa: realtek: rtl8365mb: add missing case for
digital interface 0
On Sun, Jun 04, 2023 at 03:01:27PM +0200, Christian Lamparter wrote:
> On 6/4/23 13:13, Alvin Šipraga wrote:
> > On Sat, Jun 03, 2023 at 12:53:48AM +0200, Christian Lamparter wrote:
> > > when bringing up the switch on a Netgear WNDAP660, I observed that
> > > no traffic got passed from the RTL8363 to the ethernet interface...
> >
> > Could you share the chip ID/version you read out from this RTL8363SB? I haven't
> > seen this part number but maybe it's equivalent to some other known switch.
>
> Sure Chip ID is 0x6000 and Chip Version is 0x1000. The label on the physical chip itself:
>
> RTL8363SB
> B8E77P2
> GC17 TAIWAN
Thanks, please include when you send the patch which adds the chip_info!
>
> I also have a preliminary patch that just adds the switch to the
> rtl8365mb_chip_info table. (The -CG came from Googling after RTL8363SB)
> ---
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -519,6 +519,19 @@ struct rtl8365mb_chip_info {
> /* Chip info for each supported switch in the family */
> #define PHY_INTF(_mode) (RTL8365MB_PHY_INTERFACE_MODE_ ## _mode)
> static const struct rtl8365mb_chip_info rtl8365mb_chip_infos[] = {
> + {
> + .name = "RTL8363SB-CG",
> + .chip_id = 0x6000,
> + .chip_ver = 0x1000,
> + .extints = {
> + { 5, 0, PHY_INTF(MII) | PHY_INTF(TMII) |
> + PHY_INTF(RMII) | PHY_INTF(RGMII) },
> + { 6, 1, PHY_INTF(MII) | PHY_INTF(TMII) |
> + PHY_INTF(RMII) | PHY_INTF(RGMII) },
> + },
> + .jam_table = rtl8365mb_init_jam_8365mb_vc,
> + .jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc),
> + },
> {
> .name = "RTL8365MB-VC",
> .chip_id = 0x6367,
> ---
>
> currently, the WNDAP660 works with the out-of-tree rtl8367b.c from openwrt:
> <https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/apm821xx/dts/netgear-wndap6x0.dtsi>
>
> | rtl8367b {
> | compatible = "realtek,rtl8367b";
> | cpu_port = <5>;
> | realtek,extif0 = <1 2 1 1 1 1 1 1 2>;
> | mii-bus = <&mdio0>;
> | };
>
> <https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/apm821xx/dts/netgear-wndap660.dts>
>
> > > Turns out, this was because the dropped case for
> > > RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(0) that
> > > got deleted by accident.
> >
> > Could you show where exactly this macro is called with 0 as an argument? AFAICT
> > this patch doesn't change anything, as the macro is called in only one place
> > with rtl8365mb_extint::id as an argument, but these id fields are statically
> > populated in rtl8365mb_chip_info and I only see values 1 or 2 there.
> >
> > If you are introducing support for a new switch, why not just use a value of 1
> > instead? The macro will then map to ..._REG0 as you desire.
>
> No, Value "1" sadly does not work. Other macros like
> RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_MASK(_extint) and
> RTL8365MB_EXT_RGMXF_REG(_extint) do support "0" just as before. i.e:
>
> <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/dsa/realtek/rtl8365mb.c#n224>
> #define RTL8365MB_EXT_RGMXF_REG(_extint) \
> ((_extint) == 0 ? RTL8365MB_EXT_RGMXF_REG0 : \
> (_extint) == 1 ? RTL8365MB_EXT_RGMXF_REG1 : \
> (_extint) == 2 ? RTL8365MB_EXT_RGMXF_REG2 : \
> 0x0
>
> The patch "net: dsa: realtek: rtl8365mb: rename extport to extint" mentioned
> removed:
>
> -#define RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(_extport) \
> - (RTL8365MB_DIGITAL_INTERFACE_SELECT_REG0 + \
> - ((_extport) >> 1) * (0x13C3 - 0x1305))
>
> and replaced it with:
>
> +#define RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(_extint) \
> + ((_extint) == 1 ? RTL8365MB_DIGITAL_INTERFACE_SELECT_REG0 : \
> + (_extint) == 2 ? RTL8365MB_DIGITAL_INTERFACE_SELECT_REG1 : \
> + 0x0)
>
> so with the old RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(0) evaluated to
> (RTL8365MB_DIGITAL_INTERFACE_SELECT_REG0+(0) (which is 0x1305) and
> since the patch RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(0) evaluates to
> 0. So the driver writes to somewhere it shouldn't (in my RTL8363SB)
> case.
Ah yes, I see what you mean now. OK, so 0 is indeed valid. Please include this
extra info in your v2 message :)
Also I suggest marking REG0 as EXT0 and EXT1, something like this:
#define RTL8365MB_DIGITAL_INTERFACE_SELECT_REG0 0x1305 /* EXT0/EXT1 */
>
> so that's why I said it was "by accident" in the commit message.
> Since the other macros stayed intact.
Yes, agree. Do you agree with me though that this doesn't warrant backporting to
stable as there is no functional change with just the patch on its own?
i.e. this should be part of a broader series adding RTL8363SB-CG support
targetting net-next. (The Fixes: tag is absolutely fine ofc - stable maintainers
will tell you that this does not necessarily mean it should go in stable, that's
what cc: stable@...r is for). If so please add [PATCH v2 net-next] so it goes in
the right place.
>
> From what I can gleam, Luiz patch mentions at the end:
> "[...] and ext_id 0 does not seem to be used as well for this family."
>
> Looking around in todays OpenWrt's various DTS. There are these devices:
>
> extif0:
> TP-Link WR2543-v1
> SFR Neufbox 6 (Sercomm)
> Edimax BR-6475nD
> Samsung CY-SWR1100
> (Netgear WNDAP660 + WNDAP620)
Hm, hard to comment without knowing the exact chip. But if you couldn't get it
to work with 1 or 2, I guess 0 is correct. The vendor sources refer to an EXT0
here and there after all.
Kind regards,
Alvin
>
> extif1:
> Asus RT-N56U
> D-Link DIR-645
> TP-Link Archer C2 v1
> I-O DATA WN-AC733GR3
>
> extif2:
> ZyXEL Keenetic Viva
>
> Since this discovery, I do now have something that sort of works.
> If you have a different values for extif0/export1, I sure can adapt
> them no problem.
>
> Cheers,
> Christian
Powered by blists - more mailing lists