Message-ID: <b309ba69b9e97f8e681f814fda1bb069e11e367d.camel@embedd.com>
Date: Tue, 05 Dec 2023 10:08:39 +0100
From: Daniel Danzberger <dd@...edd.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: woojung.huh@...rochip.com, UNGLinuxDriver@...rochip.com, 
	netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>, Florian Fainelli
	 <f.fainelli@...il.com>
Subject: Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on
 platform init

On Tue, 2023-12-05 at 10:36 +0200, Vladimir Oltean wrote:
> On Tue, Dec 05, 2023 at 09:00:39AM +0100, Daniel Danzberger wrote:
> > > Is this all that's necessary for instantiating the ksz driver through
> > > ds->dev->platform_data? I suppose not, so can you post it all, please?
> > Yes, that NULL pointer was the only issue I encountered.
> > 
> > Here is the module I use to instantiate the switch, which works fine so far in our
> > linux v6.1 x86_64 builds:
> > --
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > #include <linux/i2c.h>
> > #include <linux/netdevice.h>
> > #include <net/dsa.h>
> > 
> > static struct i2c_client *i2cdev;
> > 
> > static struct dsa_chip_data ksz9477_dsa = {
> > 	.port_names = {
> > 		"cpu",
> > 		"lan1",
> > 		"lan2",
> > 		"lan3",
> > 		"lan4"
> > 	}
> > };
> > 
> > static struct i2c_board_info t2t_ngr421_i2c_board_info = {
> > 	I2C_BOARD_INFO("ksz9477-switch", 0x5f),
> > 	.platform_data	= &ksz9477_dsa,
> > };
> > 
> > static int __init t2t_ngr421_platform_init(void)
> > {
> > 	struct i2c_adapter *adapter = i2c_get_adapter(11);
> > 	struct net_device *netdev_cpu = NULL, *nd;
> > 
> > 	if (adapter == NULL) {
> > 		pr_err("t2t-ngr421: Missing FT260 I2C Adapter");
> > 		return -ENODEV;
> > 	}
> > 
> > 	read_lock(&dev_base_lock);
> > 	for_each_netdev(&init_net, nd) {
> > 		if (!strcmp(nd->name, "eth0")) {
> > 			netdev_cpu = nd;
> > 			break;
> > 		}
> > 	}
> > 	read_unlock(&dev_base_lock);
> > 
> > 	if (netdev_cpu == NULL) {
> > 		pr_err("t2t-ngr421: Missing netdev eth0");
> > 		return -ENODEV;
> > 	}
> > 		
> > 	ksz9477_dsa.netdev[0] = &netdev_cpu->dev;
> > 	i2cdev = i2c_new_client_device(adapter, &t2t_ngr421_i2c_board_info);
> > 	return i2cdev ? 0 : -ENODEV;
> > }
> > 
> > static void t2t_ngr421_platform_deinit(void)
> > {
> > 	if (i2cdev)
> > 		i2c_unregister_device(i2cdev);
> > }
> > 
> > module_init(t2t_ngr421_platform_init);
> > module_exit(t2t_ngr421_platform_deinit);
> > 
> > MODULE_AUTHOR("Daniel Danzberger <dd@...edd.com>");
> > MODULE_DESCRIPTION("T2T NGR421 platform driver");
> > MODULE_LICENSE("GPL v2");
> > --
> 
> Pfff, I hate that "eth0" search. If you have a udev naming rule and the
> driver is built as a module, you break it. Although you don't even need
> that. Insert a USB to Ethernet adapter and all bets are off regarding
> which one is eth0 and which one is eth1. It's good as prototyping code
> and not much more.
> 
> Admittedly, that's the only thing that DSA offers currently when there's
> no firmware description of the switch, but I think it wouldn't even be
> that hard to do better. Someone needs to take a close look at Marcin
> Wojtas' work of converting DSA to fwnode APIs
> https://lore.kernel.org/netdev/20230116173420.1278704-1-mw@semihalf.com/
> then we could replace the platform_data with software nodes and references.
> That should actually be in our own best interest as maintainers, since
> it should unify the handling of the 2 probing cases in the DSA core.
> I might be able to find some time to do that early next year.
> 
> Except for dsa_loop_bdinfo.c which is easy to test by anyone, I don't
> see any other board init code physically present in mainline. So please
> do _not_ submit the board code, so I can pretend I didn't see it, and
> for the responsibility of converting it to the new API to fall on you :)
> 
> (or, of course, you may want to take on the bigger task yourself ahead
> of me, case in which your board code, edited to use fwnode_create_software_node(),
> would be perfectly welcome in mainline)
That module is just a way to instantiate the switch on a piece of fixed custom hardware glued
together on my desk. It's never meant to be upstream. I just posted it as an example on how the
switch can be instantiated via 'i2c_new_client_device' instead of DTS.

> 
> But let's do something about the ksz driver's use of the platform_data
> structures, since it wasn't even on my radar of something that might be
> able to support that use case. More below.
> 
> > > Looking at dsa_switch_probe() -> dsa_switch_parse(), it expects
> > > ds->dev->platform_data to contain a struct dsa_chip_data. This is in
> > > contrast with ksz_spi.c, ksz9477_i2c.c and ksz8863_smi.c, which expect
> > > the dev->platform_data to have the struct ksz_platform_data type.
> > > But struct ksz_platform_data does not contain struct dsa_chip_data as
> > > first element.
> > 
> > Noticed that as well.
> > But hence the 'struct ksz_platform_data' isn't used anywhere, I passed (see module above)
> > 'struct
> > dsa_chip_data' directly.
> 
> What do you mean struct ksz_platform_data isn't used anywhere? What about this?
{
	u8 id1, id2, id4;
	u16 id16;
	u32 id32;
	int ret;

	/* read chip id */
	ret = ksz_read16(dev, REG_CHIP_ID0, &id16);
	if (ret)
		return ret;

>  isn't used anywhere? What about this?
> 
> int ksz_switch_register(struct ksz_device *dev)
> {
> 	const struct ksz_chip_data *info;
> 	struct device_node *port, *ports;
> 	phy_interface_t interface;
> 	unsigned int port_num;
> 	int ret;
> 	int i;
> 
> 	if (dev->pdata)
> 		dev->chip_id = dev->pdata->chip_id;
> 
> with dev->pdata assigned like this:
> 
> static int ksz9477_i2c_probe(struct i2c_client *i2c)
> {
> 	struct ksz_device *dev;
> 
> 	dev = ksz_switch_alloc(&i2c->dev, i2c);
> 	if (!dev)
> 		return -ENOMEM;
> 
> 	if (i2c->dev.platform_data)
> 		dev->pdata = i2c->dev.platform_data;

The pointer is only copied around, but ksz_platform_data is never actually accessed in any
meaningful way. The chip_id assigned from DTS or platform_data doesn't even seem to be respected
anywhere in the decision making.

Right at 'ksz_switch_register', 'ksz_switch_detect' is called and overwrites 'dev->chip_id' with the
id read from the hardware:

static int ksz_switch_detect(struct ksz_device *dev)
{
	u8 id1, id2, id4;
	u16 id16;
	u32 id32;
	int ret;

	/* read chip id */
	ret = ksz_read16(dev, REG_CHIP_ID0, &id16);
	if (ret)
		return ret;


> 

-- 
Regards

Daniel Danzberger
embeDD GmbH, Alter Postplatz 2, CH-6370 Stans

Powered by blists - more mailing lists