Re: [Patch net-next v2 0/9] net: dsa: microchip: add support for phylink mac config and link up

[Oleksij Rempel <o.rempel@pengutronix.de>]

On Tue, Aug 30, 2022 at 12:58:30PM +0300, Vladimir Oltean wrote:
> Hello,
> 
> On Tue, Aug 30, 2022 at 08:15:59AM +0000, Arun.Ramadoss@microchip.com wrote:
...
> > Hi Oleksij,
> > Is this Bug related to fix in 
> > https://lore.kernel.org/lkml/20220829105810.577903823@linuxfoundation.org/
> > . 
> > It is observed in ksz8794 switch. I think after applying this bug fix
> > patch it should work. I don't have ksz8 series to test. I ran the
> > regression only for ksz9 series switches. 
> 
> I find it unlikely that the cited patch will fix a NULL pointer
> dereference in ksz_get_gbit(). But rather, some pointer to a structure
> is NULL, and we then dereference a member located at its offset 0x5, no?
> 
> My eyes are on this:
> 
> 	const u8 *bitval = dev->info->xmii_ctrl1;
> 
> 		data8 |= FIELD_PREP(P_GMII_1GBIT_M, bitval[P_GMII_NOT_1GBIT]);
> 							   ~~~~~~~~~~~~~~~~
> 							   this is coincidentally
> 							   also 5

ack.

> See, looking at the struct ksz_chip_data[] array element for KSZ8873
> that Oleksij mentions as broken, I do not see xmii_ctrl1 and xmii_ctrl2
> as being pointers to anything.
> 
> 	[KSZ8830] = {
> 		.chip_id = KSZ8830_CHIP_ID,
> 		.dev_name = "KSZ8863/KSZ8873",
> 		.num_vlans = 16,
> 		.num_alus = 0,
> 		.num_statics = 8,
> 		.cpu_ports = 0x4,	/* can be configured as cpu port */
> 		.port_cnt = 3,
> 		.ops = &ksz8_dev_ops,
> 		.mib_names = ksz88xx_mib_names,
> 		.mib_cnt = ARRAY_SIZE(ksz88xx_mib_names),
> 		.reg_mib_cnt = MIB_COUNTER_NUM,
> 		.regs = ksz8863_regs,
> 		.masks = ksz8863_masks,
> 		.shifts = ksz8863_shifts,
> 		.supports_mii = {false, false, true},
> 		.supports_rmii = {false, false, true},
> 		.internal_phy = {true, true, false},
> 	},
> 
> Should we point them to ksz8795_xmii_ctrl0 and ksz8795_xmii_ctrl1? I don't know.
> Could you find out what these should be set to?

xmii_ctrl0/1 are missing and it make no sense to add it.
KSZ8873 switch is controlling CPU port MII configuration over global,
not port based register.

I'll better define separate ops for this chip.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |