| Message ID | E1nc7F6-004lEo-Be@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next] net: dsa: b53: convert to phylink_pcs | expand |
On 4/6/22 08:06, Russell King (Oracle) wrote: > Convert B53 to use phylink_pcs for the serdes rather than hooking it > into the MAC-layer callbacks. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > Hi Florian, > > Please can you test this patch? Thanks. Did not spend much time debugging this as I had to do something else but here is what I got: [ 1.909223] b53-srab-switch 18036000.ethernet-switch: SerDes lane 0, model: 1, rev B0 (OUI: 0x0143bff0) [ 1.918956] 8<--- cut here --- [ 1.922119] Unable to handle kernel NULL pointer dereference at virtual address 0000012c [ 1.930473] [0000012c] *pgd=00000000 [ 1.934177] Internal error: Oops: 805 [#1] SMP ARM [ 1.939124] Modules linked in: [ 1.942277] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc1 #8 [ 1.948744] Hardware name: Broadcom Northstar Plus SoC [ 1.954041] PC is at b53_serdes_init+0x1d8/0x1e0 [ 1.958815] LR is at _printk_rb_static_descs+0x0/0x6000 [ 1.964218] pc : [<c0851e08>] lr : [<c1b425d8>] psr: 60000013 [ 1.970678] sp : e0821d08 ip : 00005ff4 fp : e0821d3c [ 1.976064] r10: c102d3cc r9 : c0d6a628 r8 : 00000143 [ 1.981448] r7 : 0000012c r6 : 00004281 r5 : 00000000 r4 : c4636b40 [ 1.988177] r3 : c0d6b4ac r2 : 00000000 r1 : 0000003c r0 : 00000000 [ 1.994906] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none [ 2.002273] Control: 10c5387d Table: 6000404a DAC: 00000051 [ 2.008195] Register r0 information: NULL pointer [ 2.013050] Register r1 information: non-paged memory [ 2.018265] Register r2 information: NULL pointer [ 2.023112] Register r3 information: non-slab/vmalloc memory [ 2.028955] Register r4 information: slab kmalloc-256 start c4636b00 pointer offset 64 size 256 [ 2.037941] Register r5 information: NULL pointer [ 2.042789] Register r6 information: non-paged memory [ 2.048004] Register r7 information: non-paged memory [ 2.053218] Register r8 information: non-paged memory [ 2.058424] Register r9 information: non-slab/vmalloc memory [ 2.064266] Register r10 information: non-slab/vmalloc memory [ 2.070198] Register r11 information: 2-page vmalloc region starting at 0xe0820000 allocated at kernel_clone+0xa4/0x3d8 [ 2.081335] Register r12 information: non-paged memory [ 2.086639] Process swapper/0 (pid: 1, stack limit = 0x(ptrval)) [ 2.092840] Stack: (0xe0821d08 to 0xe0822000) [ 2.097340] 1d00: 00000042 00000000 0143bff0 c05d689c e0821d3c c4636b40 [ 2.105782] 1d20: c4636a40 00000005 c4636b40 c4636a40 e0821d74 e0821d40 c0851128 c0851c3c [ 2.114222] 1d40: c0f9a998 c21abc10 c0761620 00000000 c21abc10 c1bec070 c1c3b528 00000000 [ 2.122662] 1d60: c1157854 c1c4f000 e0821d94 e0821d78 c0747708 c0850ed8 00000000 c21abc10 [ 2.131103] 1d80: c1bec070 c1c3b528 e0821dbc e0821d98 c07446ec c07476a8 c07551f0 c0753e00 [ 2.139542] 1da0: c21abc10 c1bec070 c1c3b528 c21abc10 e0821dec e0821dc0 c0744a38 c0744594 [ 2.147982] 1dc0: c21abc10 c1bec070 c1c83288 c1c8328c c1bec070 c21abc10 00000000 c1157854 [ 2.156423] 1de0: e0821e14 e0821df0 c0744be0 c0744994 c21abc10 c21abc54 c1bec070 c1bd7910 [ 2.164863] 1e00: 00000000 c1157854 e0821e34 e0821e18 c074545c c0744ba8 00000000 c1bec070 [ 2.173303] 1e20: c0745364 c1bd7910 e0821e64 e0821e38 c07421d4 c0745370 00000000 c2128c58 [ 2.181744] 1e40: c216d134 32ea7446 e0821e74 c1bec070 c45f2700 00000000 e0821e74 e0821e68 [ 2.190185] 1e60: c074400c c074215c e0821e9c e0821e78 c07438e8 c0743fec c102d3e0 c0652620 [ 2.198624] 1e80: c1bec070 00000000 00000007 c2180000 e0821eb4 e0821ea0 c07460b0 c0743774 [ 2.207065] 1ea0: c1c2bb40 c112d33c e0821ec4 e0821eb8 c074736c c0746024 e0821ed4 e0821ec8 [ 2.215506] 1ec0: c112d360 c074734c e0821f4c e0821ed8 c0102300 c112d348 c0fa28d8 c0fa28b8 [ 2.223946] 1ee0: c0fa2904 c0fbaf00 00000000 c0fa2894 00000006 00000006 c1c36440 c11005cc [ 2.232386] 1f00: c1046b18 00000000 0000011c 00000000 c1101554 c20d9fd2 c20d9fdd 32ea7446 [ 2.240826] 1f20: c018bd80 32ea7446 c20d9f80 c1190248 c20d9f80 00000007 c1157834 0000011c [ 2.249267] 1f40: e0821f94 e0821f50 c110161c c01022b4 00000006 00000006 00000000 c11005cc [ 2.257707] 1f60: c10eda18 c11005cc 00000000 c1b04d00 c0c9bf20 00000000 00000000 00000000 [ 2.266147] 1f80: 00000000 00000000 e0821fac e0821f98 c0c9bf48 c1101454 00000000 c0c9bf20 [ 2.274588] 1fa0: 00000000 e0821fb0 c0100148 c0c9bf2c 00000000 00000000 00000000 00000000 [ 2.283027] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 2.291467] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000 [ 2.299905] Backtrace: [ 2.302423] b53_serdes_init from b53_srab_probe+0x25c/0x2b0 [ 2.308269] r8:c4636a40 r7:c4636b40 r6:00000005 r5:c4636a40 r4:c4636b40 [ 2.315181] b53_srab_probe from platform_probe+0x6c/0xcc [ 2.320759] r10:c1c4f000 r9:c1157854 r8:00000000 r7:c1c3b528 r6:c1bec070 r5:c21abc10 [ 2.328837] r4:00000000 [ 2.331445] platform_probe from really_probe+0x164/0x400 [ 2.337029] r7:c1c3b528 r6:c1bec070 r5:c21abc10 r4:00000000 [ 2.342868] really_probe from __driver_probe_device+0xb0/0x214 [ 2.348982] r7:c21abc10 r6:c1c3b528 r5:c1bec070 r4:c21abc10 [ 2.354821] __driver_probe_device from driver_probe_device+0x44/0xd4 [ 2.361473] r9:c1157854 r8:00000000 r7:c21abc10 r6:c1bec070 r5:c1c8328c r4:c1c83288 [ 2.369462] driver_probe_device from __driver_attach+0xf8/0x1dc [ 2.375666] r9:c1157854 r8:00000000 r7:c1bd7910 r6:c1bec070 r5:c21abc54 r4:c21abc10 [ 2.383654] __driver_attach from bus_for_each_dev+0x84/0xd0 [ 2.389498] r7:c1bd7910 r6:c0745364 r5:c1bec070 r4:00000000 [ 2.395329] bus_for_each_dev from driver_attach+0x2c/0x30 [ 2.400993] r6:00000000 r5:c45f2700 r4:c1bec070 [ 2.405749] driver_attach from bus_add_driver+0x180/0x21c [ 2.411412] bus_add_driver from driver_register+0x98/0x128 [ 2.417167] r7:c2180000 r6:00000007 r5:00000000 r4:c1bec070 [ 2.423007] driver_register from __platform_driver_register+0x2c/0x34 [ 2.429746] r5:c112d33c r4:c1c2bb40 [ 2.433427] __platform_driver_register from b53_srab_driver_init+0x24/0x28 [ 2.440622] b53_srab_driver_init from do_one_initcall+0x58/0x210 [ 2.446920] do_one_initcall from kernel_init_freeable+0x1d4/0x230 [ 2.453313] r8:0000011c r7:c1157834 r6:00000007 r5:c20d9f80 r4:c1190248 [ 2.460227] kernel_init_freeable from kernel_init+0x28/0x140 [ 2.466170] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0c9bf20 [ 2.474249] r4:c1b04d00 [ 2.476857] kernel_init from ret_from_fork+0x14/0x2c [ 2.482070] Exception stack(0xe0821fb0 to 0xe0821ff8) [ 2.487279] 1fa0: 00000000 00000000 00000000 00000000 [ 2.495720] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 2.504160] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 [ 2.510986] r5:c0c9bf20 r4:00000000 [ 2.514671] Code: e30b34ac e34c30d6 e0070791 e3a00000 (e7823007) [ 2.520987] ---[ end trace 0000000000000000 ]---
On Thu, Apr 07, 2022 at 03:55:35PM -0700, Florian Fainelli wrote: > On 4/6/22 08:06, Russell King (Oracle) wrote: > > Convert B53 to use phylink_pcs for the serdes rather than hooking it > > into the MAC-layer callbacks. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > Hi Florian, > > > > Please can you test this patch? Thanks. > > Did not spend much time debugging this as I had to do something else but > here is what I got: > > [ 1.909223] b53-srab-switch 18036000.ethernet-switch: SerDes lane 0, > model: 1, rev B0 (OUI: 0x0143bff0) > [ 1.918956] 8<--- cut here --- > [ 1.922119] Unable to handle kernel NULL pointer dereference at virtual > address 0000012c Thanks - it looks like the problem is dev->port has not been allocated at the point where b53_serdes_init is called, causing this null pointer deref. It's allocated in b53_switch_register(), which is also where the driver works out how many ports there are. I don't see an easy way to allocate this array earlier... so, where can we save a pointer to the PCS. I guess as this is specific to srab, we could put it in struct b53_srab_port_priv - but then we'll need b53_phylink_mac_select_pcs() to call into the e.g. the srab layer.
diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c index 77501f9c5915..9c7abe4ee51a 100644 --- a/drivers/net/dsa/b53/b53_common.c +++ b/drivers/net/dsa/b53/b53_common.c @@ -1354,46 +1354,27 @@ static void b53_phylink_get_caps(struct dsa_switch *ds, int port, config->legacy_pre_march2020 = false; } -int b53_phylink_mac_link_state(struct dsa_switch *ds, int port, - struct phylink_link_state *state) +static struct phylink_pcs *b53_phylink_mac_select_pcs(struct dsa_switch *ds, + int port, + phy_interface_t interface) { struct b53_device *dev = ds->priv; - int ret = -EOPNOTSUPP; - if ((phy_interface_mode_is_8023z(state->interface) || - state->interface == PHY_INTERFACE_MODE_SGMII) && - dev->ops->serdes_link_state) - ret = dev->ops->serdes_link_state(dev, port, state); + if (dev->ports[port].pcs.pcs.ops && + (phy_interface_mode_is_8023z(interface) || + interface == PHY_INTERFACE_MODE_SGMII)) + return &dev->ports[port].pcs.pcs; - return ret; + return NULL; } -EXPORT_SYMBOL(b53_phylink_mac_link_state); void b53_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode, const struct phylink_link_state *state) { - struct b53_device *dev = ds->priv; - - if (mode == MLO_AN_PHY || mode == MLO_AN_FIXED) - return; - - if ((phy_interface_mode_is_8023z(state->interface) || - state->interface == PHY_INTERFACE_MODE_SGMII) && - dev->ops->serdes_config) - dev->ops->serdes_config(dev, port, mode, state); } EXPORT_SYMBOL(b53_phylink_mac_config); -void b53_phylink_mac_an_restart(struct dsa_switch *ds, int port) -{ - struct b53_device *dev = ds->priv; - - if (dev->ops->serdes_an_restart) - dev->ops->serdes_an_restart(dev, port); -} -EXPORT_SYMBOL(b53_phylink_mac_an_restart); - void b53_phylink_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode, phy_interface_t interface) @@ -2269,9 +2250,8 @@ static const struct dsa_switch_ops b53_switch_ops = { .phy_write = b53_phy_write16, .adjust_link = b53_adjust_link, .phylink_get_caps = b53_phylink_get_caps, - .phylink_mac_link_state = b53_phylink_mac_link_state, + .phylink_mac_select_pcs = b53_phylink_mac_select_pcs, .phylink_mac_config = b53_phylink_mac_config, - .phylink_mac_an_restart = b53_phylink_mac_an_restart, .phylink_mac_link_down = b53_phylink_mac_link_down, .phylink_mac_link_up = b53_phylink_mac_link_up, .port_enable = b53_enable_port, diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h index 3085b6cc7d40..83cdca443567 100644 --- a/drivers/net/dsa/b53/b53_priv.h +++ b/drivers/net/dsa/b53/b53_priv.h @@ -21,7 +21,7 @@ #include <linux/kernel.h> #include <linux/mutex.h> -#include <linux/phy.h> +#include <linux/phylink.h> #include <linux/etherdevice.h> #include <net/dsa.h> @@ -29,7 +29,6 @@ struct b53_device; struct net_device; -struct phylink_link_state; struct b53_io_ops { int (*read8)(struct b53_device *dev, u8 page, u8 reg, u8 *value); @@ -49,12 +48,6 @@ struct b53_io_ops { void (*phylink_get_caps)(struct b53_device *dev, int port, struct phylink_config *config); u8 (*serdes_map_lane)(struct b53_device *dev, int port); - int (*serdes_link_state)(struct b53_device *dev, int port, - struct phylink_link_state *state); - void (*serdes_config)(struct b53_device *dev, int port, - unsigned int mode, - const struct phylink_link_state *state); - void (*serdes_an_restart)(struct b53_device *dev, int port); void (*serdes_link_set)(struct b53_device *dev, int port, unsigned int mode, phy_interface_t interface, bool link_up); @@ -85,10 +78,22 @@ enum { BCM7278_DEVICE_ID = 0x7278, }; +struct b53_pcs { + struct phylink_pcs pcs; + struct b53_device *dev; + int port; +}; + +static inline struct b53_pcs *pcs_to_b53_pcs(struct phylink_pcs *pcs) +{ + return container_of(pcs, struct b53_pcs, pcs); +} + #define B53_N_PORTS 9 #define B53_N_PORTS_25 6 struct b53_port { + struct b53_pcs pcs; u16 vlan_ctl_mask; struct ethtool_eee eee; }; @@ -336,12 +341,9 @@ int b53_br_flags(struct dsa_switch *ds, int port, struct netlink_ext_ack *extack); int b53_setup_devlink_resources(struct dsa_switch *ds); void b53_port_event(struct dsa_switch *ds, int port); -int b53_phylink_mac_link_state(struct dsa_switch *ds, int port, - struct phylink_link_state *state); void b53_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode, const struct phylink_link_state *state); -void b53_phylink_mac_an_restart(struct dsa_switch *ds, int port); void b53_phylink_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode, phy_interface_t interface); diff --git a/drivers/net/dsa/b53/b53_serdes.c b/drivers/net/dsa/b53/b53_serdes.c index 555e5b372321..32af78f1438b 100644 --- a/drivers/net/dsa/b53/b53_serdes.c +++ b/drivers/net/dsa/b53/b53_serdes.c @@ -60,31 +60,40 @@ static u16 b53_serdes_read(struct b53_device *dev, u8 lane, return b53_serdes_read_blk(dev, offset, block); } -void b53_serdes_config(struct b53_device *dev, int port, unsigned int mode, - const struct phylink_link_state *state) +static int b53_serdes_config(struct phylink_pcs *pcs, unsigned int mode, + phy_interface_t interface, + const unsigned long *advertising, + bool permit_pause_to_mac) { - u8 lane = b53_serdes_map_lane(dev, port); + struct b53_device *dev = pcs_to_b53_pcs(pcs)->dev; + int port = pcs_to_b53_pcs(pcs)->port; u16 reg; + u8 lane; + lane = b53_serdes_map_lane(dev, port); if (lane == B53_INVALID_LANE) - return; + return 0; reg = b53_serdes_read(dev, lane, B53_SERDES_DIGITAL_CONTROL(1), SERDES_DIGITAL_BLK); - if (state->interface == PHY_INTERFACE_MODE_1000BASEX) + if (interface == PHY_INTERFACE_MODE_1000BASEX) reg |= FIBER_MODE_1000X; else reg &= ~FIBER_MODE_1000X; b53_serdes_write(dev, lane, B53_SERDES_DIGITAL_CONTROL(1), SERDES_DIGITAL_BLK, reg); + + return 0; } -EXPORT_SYMBOL(b53_serdes_config); -void b53_serdes_an_restart(struct b53_device *dev, int port) +static void b53_serdes_an_restart(struct phylink_pcs *pcs) { - u8 lane = b53_serdes_map_lane(dev, port); + struct b53_device *dev = pcs_to_b53_pcs(pcs)->dev; + int port = pcs_to_b53_pcs(pcs)->port; u16 reg; + u8 lane; + lane = b53_serdes_map_lane(dev, port); if (lane == B53_INVALID_LANE) return; @@ -94,16 +103,20 @@ void b53_serdes_an_restart(struct b53_device *dev, int port) b53_serdes_write(dev, lane, B53_SERDES_MII_REG(MII_BMCR), SERDES_MII_BLK, reg); } -EXPORT_SYMBOL(b53_serdes_an_restart); -int b53_serdes_link_state(struct b53_device *dev, int port, - struct phylink_link_state *state) +static void b53_serdes_get_state(struct phylink_pcs *pcs, + struct phylink_link_state *state) { - u8 lane = b53_serdes_map_lane(dev, port); + struct b53_device *dev = pcs_to_b53_pcs(pcs)->dev; + int port = pcs_to_b53_pcs(pcs)->port; u16 dig, bmsr; + u8 lane; - if (lane == B53_INVALID_LANE) - return 1; + lane = b53_serdes_map_lane(dev, port); + if (lane == B53_INVALID_LANE) { + state->link = false; + return; + } dig = b53_serdes_read(dev, lane, B53_SERDES_DIGITAL_STATUS, SERDES_DIGITAL_BLK); @@ -133,10 +146,7 @@ int b53_serdes_link_state(struct b53_device *dev, int port, state->pause |= MLO_PAUSE_RX; if (dig & PAUSE_RESOLUTION_TX_SIDE) state->pause |= MLO_PAUSE_TX; - - return 0; } -EXPORT_SYMBOL(b53_serdes_link_state); void b53_serdes_link_set(struct b53_device *dev, int port, unsigned int mode, phy_interface_t interface, bool link_up) @@ -158,6 +168,12 @@ void b53_serdes_link_set(struct b53_device *dev, int port, unsigned int mode, } EXPORT_SYMBOL(b53_serdes_link_set); +static const struct phylink_pcs_ops b53_pcs_ops = { + .pcs_get_state = b53_serdes_get_state, + .pcs_config = b53_serdes_config, + .pcs_an_restart = b53_serdes_an_restart, +}; + void b53_serdes_phylink_get_caps(struct b53_device *dev, int port, struct phylink_config *config) { @@ -212,6 +228,8 @@ int b53_serdes_init(struct b53_device *dev, int port) (id0 >> SERDES_ID0_REV_NUM_SHIFT) & SERDES_ID0_REV_NUM_MASK, (u32)msb << 16 | lsb); + dev->ports[port].pcs.pcs.ops = &b53_pcs_ops; + return 0; } EXPORT_SYMBOL(b53_serdes_init); diff --git a/drivers/net/dsa/b53/b53_serdes.h b/drivers/net/dsa/b53/b53_serdes.h index f47d5caa7557..c372ba588994 100644 --- a/drivers/net/dsa/b53/b53_serdes.h +++ b/drivers/net/dsa/b53/b53_serdes.h @@ -107,12 +107,6 @@ static inline u8 b53_serdes_map_lane(struct b53_device *dev, int port) return dev->ops->serdes_map_lane(dev, port); } -int b53_serdes_get_link(struct b53_device *dev, int port); -int b53_serdes_link_state(struct b53_device *dev, int port, - struct phylink_link_state *state); -void b53_serdes_config(struct b53_device *dev, int port, unsigned int mode, - const struct phylink_link_state *state); -void b53_serdes_an_restart(struct b53_device *dev, int port); void b53_serdes_link_set(struct b53_device *dev, int port, unsigned int mode, phy_interface_t interface, bool link_up); void b53_serdes_phylink_get_caps(struct b53_device *dev, int port, diff --git a/drivers/net/dsa/b53/b53_srab.c b/drivers/net/dsa/b53/b53_srab.c index c51b716657db..af5172c4660f 100644 --- a/drivers/net/dsa/b53/b53_srab.c +++ b/drivers/net/dsa/b53/b53_srab.c @@ -492,9 +492,6 @@ static const struct b53_io_ops b53_srab_ops = { .phylink_get_caps = b53_srab_phylink_get_caps, #if IS_ENABLED(CONFIG_B53_SERDES) .serdes_map_lane = b53_srab_serdes_map_lane, - .serdes_link_state = b53_serdes_link_state, - .serdes_config = b53_serdes_config, - .serdes_an_restart = b53_serdes_an_restart, .serdes_link_set = b53_serdes_link_set, #endif };
Convert B53 to use phylink_pcs for the serdes rather than hooking it into the MAC-layer callbacks. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- Hi Florian, Please can you test this patch? Thanks. drivers/net/dsa/b53/b53_common.c | 38 ++++++----------------- drivers/net/dsa/b53/b53_priv.h | 24 ++++++++------- drivers/net/dsa/b53/b53_serdes.c | 52 +++++++++++++++++++++----------- drivers/net/dsa/b53/b53_serdes.h | 6 ---- drivers/net/dsa/b53/b53_srab.c | 3 -- 5 files changed, 57 insertions(+), 66 deletions(-)