| 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, 22 Aug 2021 23:25:18 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Andrew Lunn <andrew@...n.ch>,
Alvin Šipraga <alvin@...s.dk>
CC: Linus Walleij <linus.walleij@...aro.org>,
Vivien Didelot <vivien.didelot@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
Vladimir Oltean <olteanv@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>,
Michael Rasmussen <MIR@...g-olufsen.dk>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb
subdriver for RTL8365MB-VC
Hi Andrew,
On 8/23/21 1:04 AM, Andrew Lunn wrote:
>> Signed-off-by: Alvin Šipraga <alsi@...g-olufsen.dk>
>> Co-developed-by: Michael Rasmussen <mir@...g-olufsen.dk>
>> Signed-off-by: Michael Rasmussen <mir@...g-olufsen.dk>
>
> Hi Alvin
>
> Since you are submitting the patch, your SOB goes last.
Will fix in v2.
>
>> +/* Port mapping macros
>> + *
>> + * PORT_NUM_x2y: map a port number from domain x to domain y
>> + * PORT_MASK_x2y: map a port mask from domain x to domain y
>> + *
>> + * L = logical port domain, i.e. dsa_port.index
>> + * P = physical port domain, used by the Realtek ASIC for port indexing;
>> + * for ports with internal PHYs, this is also the PHY index
>> + * E = extension port domain, used by the Realtek ASIC for managing EXT ports
>> + *
>> + * The terminology is borrowed from the vendor driver. The extension port domain
>> + * is mostly used to navigate the labyrinthine layout of EXT port configuration
>> + * registers and is not considered intuitive by the author.
>> + *
>> + * Unless a function is accessing chip registers, it should be using the logical
>> + * port domain. Moreover, function arguments for port numbers and port masks
>> + * must always be in the logical domain. The conversion must be done as close as
>> + * possible to the register access to avoid chaos.
>> + *
>> + * The mappings vary between chips in the family supported by this driver. Here
>> + * is an example of the mapping for the RTL8365MB-VC:
>> + *
>> + * L | P | E | remark
>> + * ---+---+---+--------
>> + * 0 | 0 | | user port
>> + * 1 | 1 | | user port
>> + * 2 | 2 | | user port
>> + * 3 | 3 | | user port
>> + * 4 | 6 | 1 | extension (CPU) port
>
> Did you consider not bothering with this. Just always use the Physical
> port number? The DSA framework does not care if there are ports
> missing. If it makes the code simpler, ignore the logical number, and
> just enforce that the missing ports are not used, by returning -EINVAL
> in the port_enable() callback.
Nice, I didn't know I could do that. I'll review this before sending v2,
although I might end up keeping it if it leads to sticky logic elsewhere
in the code. In general the PORT_{NUM,MASK}_L2P() calls are not too painfil.
Just to clarify, this means I should set the physical port number in the
reg field of the device tree? i.e.:
port@4 {
reg = <6>; /* <--- instead of <4>? */
label = "cpu";
ethernet = <&fec1>;
phy-mode = "rgmii-id";
fixed-link {
speed = <1000>;
full-duplex;
pause;
};
};
>
>> +/* Interrupt control register - enable or disable specific interrupt types */
>> +#define RTL8365MB_INTR_CTRL 0x1101
>> +#define RTL8365MB_INTR_CTRL_SLIENT_START_2_MASK 0x1000
>> +#define RTL8365MB_INTR_CTRL_SLIENT_START_MASK 0x800
>> +#define RTL8365MB_INTR_CTRL_ACL_ACTION_MASK 0x200
>> +#define RTL8365MB_INTR_CTRL_CABLE_DIAG_FIN_MASK 0x100
>> +#define RTL8365MB_INTR_CTRL_INTERRUPT_8051_MASK 0x80
>> +#define RTL8365MB_INTR_CTRL_LOOP_DETECTION_MASK 0x40
>> +#define RTL8365MB_INTR_CTRL_GREEN_TIMER_MASK 0x20
>> +#define RTL8365MB_INTR_CTRL_SPECIAL_CONGEST_MASK 0x10
>> +#define RTL8365MB_INTR_CTRL_SPEED_CHANGE_MASK 0x8
>> +#define RTL8365MB_INTR_CTRL_LEARN_OVER_MASK 0x4
>> +#define RTL8365MB_INTR_CTRL_METER_EXCEEDED_MASK 0x2
>> +#define RTL8365MB_INTR_CTRL_LINK_CHANGE_MASK 0x1
>> +
>> +
>> +/* Interrupt status register */
>> +#define RTL8365MB_INTR_STATUS_REG 0x1102
>> +#define RTL8365MB_INTR_STATUS_SLIENT_START_2_MASK 0x1000
>> +#define RTL8365MB_INTR_STATUS_SLIENT_START_MASK 0x800
>> +#define RTL8365MB_INTR_STATUS_ACL_ACTION_MASK 0x200
>> +#define RTL8365MB_INTR_STATUS_CABLE_DIAG_FIN_MASK 0x100
>> +#define RTL8365MB_INTR_STATUS_INTERRUPT_8051_MASK 0x80
>> +#define RTL8365MB_INTR_STATUS_LOOP_DETECTION_MASK 0x40
>> +#define RTL8365MB_INTR_STATUS_GREEN_TIMER_MASK 0x20
>> +#define RTL8365MB_INTR_STATUS_SPECIAL_CONGEST_MASK 0x10
>> +#define RTL8365MB_INTR_STATUS_SPEED_CHANGE_MASK 0x8
>> +#define RTL8365MB_INTR_STATUS_LEARN_OVER_MASK 0x4
>> +#define RTL8365MB_INTR_STATUS_METER_EXCEEDED_MASK 0x2
>> +#define RTL8365MB_INTR_STATUS_LINK_CHANGE_MASK 0x1
>> +#define RTL8365MB_INTR_STATUS_ALL_MASK \
>
> Interrupt control and status registers are generally identical. So you
> don't need to define the values twice.
Will remove in v2.
>
> +static void rtl8365mb_phylink_validate(struct dsa_switch *ds, int port,
>> + unsigned long *supported,
>> + struct phylink_link_state *state)
>> +{
>> + struct realtek_smi *smi = ds->priv;
>> + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0 };
>> +
>> + /* include/linux/phylink.h says:
>> + * When @state->interface is %PHY_INTERFACE_MODE_NA, phylink
>> + * expects the MAC driver to return all supported link modes.
>> + */
>> + if (state->interface != PHY_INTERFACE_MODE_NA &&
>> + !rtl8365mb_phy_mode_supported(ds, port, state->interface)) {
>> + dev_err(smi->dev, "phy mode %s is unsupported on port %d\n",
>> + phy_modes(state->interface), port);
>> + linkmode_zero(supported);
>> + return;
>> + }
>> +
>> + phylink_set_port_modes(mask);
>> +
>> + phylink_set(mask, Autoneg);
>> + phylink_set(mask, Pause);
>> + phylink_set(mask, Asym_Pause);
>> +
>> + phylink_set(mask, 10baseT_Half);
>> + phylink_set(mask, 10baseT_Full);
>> + phylink_set(mask, 100baseT_Half);
>> + phylink_set(mask, 100baseT_Full);
>> + phylink_set(mask, 1000baseT_Full);
>> + phylink_set(mask, 1000baseT_Half);
>
> Does the documentation actually mention 1000baseT_Half? Often it is
> not implemented.
I think you're right that it isn't supported - although the
documentation I have is very terse. I'll remove it in v2.
>
>> +static int rtl8365mb_port_enable(struct dsa_switch *ds, int port,
>> + struct phy_device *phy)
>> +{
>> + struct realtek_smi *smi = ds->priv;
>> + int val;
>> +
>> + if (dsa_is_user_port(ds, port)) {
>> + /* Power up the internal PHY and restart autonegotiation */
>> + val = rtl8365mb_phy_read(smi, port, MII_BMCR);
>> + if (val < 0)
>> + return val;
>> +
>> + val &= ~BMCR_PDOWN;
>> + val |= BMCR_ANRESTART;
>> +
>> + return rtl8365mb_phy_write(smi, port, MII_BMCR, val);
>> + }
>
> There should not be any need to do this. phylib should do it. In
> generally, you should not need to touch any PHY registers, you just
> need to allow access to the PHY registers.
Will phylib also the opposite when the interface is administratively put
down (e.g. ip link set dev swp2 down)? The switch doesn't seem to have
any other handle for stopping the flow of packets from a port except to
power down the internal PHY, hence why I put this in for port_disable.
For port_enable I just did the opposite but I can see why it's not
necessary.
>
> I want to take another look at the interrupt code. But this looks
> pretty nice in general.
>
> Andrew
>
Powered by blists - more mailing lists