[PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Daniel Danzberger]

Fixes a NULL pointer access when registering a switch device that has
not been defined via DTS.

This might happen when the switch is used on a platform like x86 that
doesn't use DTS and instantiates devices in platform specific init code.

Signed-off-by: Daniel Danzberger <dd@embedd.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 9545aed905f5..525e13d9e39c 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1678,7 +1678,7 @@ static int ksz_check_device_id(struct ksz_device *dev)
 	dt_chip_data = of_device_get_match_data(dev->dev);
 
 	/* Check for Device Tree and Chip ID */
-	if (dt_chip_data->chip_id != dev->chip_id) {
+	if (dt_chip_data && dt_chip_data->chip_id != dev->chip_id) {
 		dev_err(dev->dev,
 			"Device tree specifies chip %s but found %s, please fix it!\n",
 			dt_chip_data->dev_name, dev->info->dev_name);
-- 
2.39.2

Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Vladimir Oltean]

Hello Daniel,

On Mon, Dec 04, 2023 at 04:43:15PM +0100, Daniel Danzberger wrote:
> Fixes a NULL pointer access when registering a switch device that has
> not been defined via DTS.
> 
> This might happen when the switch is used on a platform like x86 that
> doesn't use DTS and instantiates devices in platform specific init code.
> 
> Signed-off-by: Daniel Danzberger <dd@embedd.com>
> ---
>  drivers/net/dsa/microchip/ksz_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 9545aed905f5..525e13d9e39c 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1678,7 +1678,7 @@ static int ksz_check_device_id(struct ksz_device *dev)
>  	dt_chip_data = of_device_get_match_data(dev->dev);
>  
>  	/* Check for Device Tree and Chip ID */
> -	if (dt_chip_data->chip_id != dev->chip_id) {
> +	if (dt_chip_data && dt_chip_data->chip_id != dev->chip_id) {
>  		dev_err(dev->dev,
>  			"Device tree specifies chip %s but found %s, please fix it!\n",
>  			dt_chip_data->dev_name, dev->info->dev_name);
> -- 
> 2.39.2
> 
> 

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?

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.

Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Daniel Danzberger]

Hi,

On Mon, 2023-12-04 at 19:43 +0200, Vladimir Oltean wrote:
> Hello Daniel,
> 
> On Mon, Dec 04, 2023 at 04:43:15PM +0100, Daniel Danzberger wrote:
> > Fixes a NULL pointer access when registering a switch device that has
> > not been defined via DTS.
> > 
> > This might happen when the switch is used on a platform like x86 that
> > doesn't use DTS and instantiates devices in platform specific init code.
> > 
> > Signed-off-by: Daniel Danzberger <dd@embedd.com>
> > ---
> >  drivers/net/dsa/microchip/ksz_common.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> > index 9545aed905f5..525e13d9e39c 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.c
> > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > @@ -1678,7 +1678,7 @@ static int ksz_check_device_id(struct ksz_device *dev)
> >  	dt_chip_data = of_device_get_match_data(dev->dev);
> >  
> >  	/* Check for Device Tree and Chip ID */
> > -	if (dt_chip_data->chip_id != dev->chip_id) {
> > +	if (dt_chip_data && dt_chip_data->chip_id != dev->chip_id) {
> >  		dev_err(dev->dev,
> >  			"Device tree specifies chip %s but found %s, please fix it!\n",
> >  			dt_chip_data->dev_name, dev->info->dev_name);
> > -- 
> > 2.39.2
> > 
> > 
> 
> 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@embedd.com>");
MODULE_DESCRIPTION("T2T NGR421 platform driver");
MODULE_LICENSE("GPL v2");
--

> 
> 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.
Which is what the DSA code at net/dsa/dsa2.c expects in:
 static int dsa_switch_parse(struct dsa_switch *ds, struct dsa_chip_data *cd)

-- 
Regards

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

Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Vladimir Oltean]

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@embedd.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)

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?

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;

What is your dev->chip_id before and after this? The code dereferences
the first 4 bytes of struct dsa_chip_data as if it was the chip_id field
of struct ksz_platform_data. There is a bunch of code that depends on
dev->chip_id at runtime.

Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Daniel Danzberger]

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@embedd.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

Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Vladimir Oltean]

On Tue, Dec 05, 2023 at 10:08:39AM +0100, Daniel Danzberger wrote:
> 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.

You're free to use the code in whichever way you want. I was just pointing
out that non-mainline code gets no attention in terms of converting after
breaking changes in the API.

For the networking subsystem there are 2 git trees: net-next for new
features and net for bug fixes which get backported to stable kernels.
In Documentation/process/stable-kernel-rules.rst there are some guidelines
about what constitutes a bug, but my understanding of it is that if we
can't point to some mainline code that is broken, then it's not a bug
and the change goes to net-next (aka work pending for v6.8).

So ok, it's not mainline, perfectly fair, but I want to be clear about
the implication in terms of reduced level of effort in helping to
maintain such support.

Usually, patches are designated by the author in the email title as
"[PATCH net-next]" or "[PATCH net]", and to help the backport, one
should also add a Fixes: tag in case of a bug (search the git log for
examples). You didn't make your intent clear, so I suppose you don't
have a clear expectation of how the patch should be handled. The default
in this case is "net-next".

> 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.

The one assigned from DTS gets respected in ksz_check_device_id(). If
different from the detected one, the driver fails to probe. Incidentally,
that's also the snag that you hit in your platform_data case.
of_device_get_match_data() doesn't work, so the driver isn't able to
compare the detected switch ID with a pre-established switch ID that it
expects.

Your patch implies it's ok to go along with whatever is detected, which
makes the code paths slightly different. If there is an early error in
the SPI/I2C bus communication, it will be detected for sure in the OF
case, but it might or might not get detected in the platform_data case.
It all depends on what we read and the branches that ksz_switch_detect()
takes.

> 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;

Ok. It would be nice to remove the bogus and confusing handling of
struct ksz_platform_data. If someone who has the hardware to test and
make sure nothing breaks (aka you) could do it, it would be even better.

Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Vladimir Oltean]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

On Mon, Dec 04, 2023 at 04:43:15PM +0100, Daniel Danzberger wrote:
> Fixes a NULL pointer access when registering a switch device that has
> not been defined via DTS.
> 
> This might happen when the switch is used on a platform like x86 that
> doesn't use DTS and instantiates devices in platform specific init code.
> 
> Signed-off-by: Daniel Danzberger <dd@embedd.com>
> ---

I'm sorry, I just don't like the state in which your patch leaves the
driver. Would you mind testing this attached patch instead?

[-- Attachment #2: 0001-net-dsa-microchip-properly-support-platform_data-pro.patch --]
[-- Type: text/x-diff, Size: 4069 bytes --]

From dfc42178eabe54d0bf76440f0721b920702a78f3 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Tue, 5 Dec 2023 12:00:05 +0200
Subject: [PATCH net-next] net: dsa: microchip: properly support platform_data
 probing

The ksz driver has bits and pieces of platform_data probing support, but
it doesn't work.

The conventional thing to do is to have an encapsulating structure for
struct dsa_chip_data that gets put into dev->platform_data. This driver
expects a struct ksz_platform_data, but that doesn't contain a struct
dsa_chip_data as first element, which will obviously not work with
dsa_switch_probe() -> dsa_switch_parse().

Pointing dev->platform_data to a struct dsa_chip_data directly is in
principle possible, but that doesn't work either. The driver has
ksz_switch_detect() to read the device ID from hardware, followed by
ksz_check_device_id() to compare it against a predetermined expected
value. This protects against early errors in the SPI/I2C communication.
With platform_data, the mechanism in ksz_check_device_id() doesn't work
and even leads to NULL pointer dereferences, since of_device_get_match_data()
doesn't work in that probe path.

So obviously, the platform_data support is actually missing, and the
existing handling of struct ksz_platform_data is bogus. Complete the
support by adding a struct dsa_chip_data as first element, and fixing up
ksz_check_device_id() to pick up the platform_data instead of the
unavailable of_device_get_match_data().

The early dev->chip_id assignment from ksz_switch_register() is also
bogus, because ksz_switch_detect() sets it to an initial value. So
remove it.

Also, ksz_platform_data :: enabled_ports isn't used anywhere, delete it.

Link: https://lore.kernel.org/netdev/20231204154315.3906267-1-dd@embedd.com/
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/microchip/ksz_common.c      | 21 +++++++++++++--------
 include/linux/platform_data/microchip-ksz.h |  4 +++-
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 9545aed905f5..db1bbcf3a5f2 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1673,15 +1673,23 @@ static const struct ksz_chip_data *ksz_lookup_info(unsigned int prod_num)
 
 static int ksz_check_device_id(struct ksz_device *dev)
 {
-	const struct ksz_chip_data *dt_chip_data;
+	const struct ksz_chip_data *expected_chip_data;
+	u32 expected_chip_id;
 
-	dt_chip_data = of_device_get_match_data(dev->dev);
+	if (dev->pdata) {
+		expected_chip_id = dev->pdata->chip_id;
+		expected_chip_data = ksz_lookup_info(expected_chip_id);
+		if (WARN_ON(!expected_chip_data))
+			return -ENODEV;
+	} else {
+		expected_chip_data = of_device_get_match_data(dev->dev);
+		expected_chip_id = expected_chip_data->chip_id;
+	}
 
-	/* Check for Device Tree and Chip ID */
-	if (dt_chip_data->chip_id != dev->chip_id) {
+	if (expected_chip_id != dev->chip_id) {
 		dev_err(dev->dev,
 			"Device tree specifies chip %s but found %s, please fix it!\n",
-			dt_chip_data->dev_name, dev->info->dev_name);
+			expected_chip_data->dev_name, dev->info->dev_name);
 		return -ENODEV;
 	}
 
@@ -4156,9 +4164,6 @@ int ksz_switch_register(struct ksz_device *dev)
 	int ret;
 	int i;
 
-	if (dev->pdata)
-		dev->chip_id = dev->pdata->chip_id;
-
 	dev->reset_gpio = devm_gpiod_get_optional(dev->dev, "reset",
 						  GPIOD_OUT_LOW);
 	if (IS_ERR(dev->reset_gpio))
diff --git a/include/linux/platform_data/microchip-ksz.h b/include/linux/platform_data/microchip-ksz.h
index ea1cc6d829e9..6480bf4af0fb 100644
--- a/include/linux/platform_data/microchip-ksz.h
+++ b/include/linux/platform_data/microchip-ksz.h
@@ -20,10 +20,12 @@
 #define __MICROCHIP_KSZ_H
 
 #include <linux/types.h>
+#include <linux/platform_data/dsa.h>
 
 struct ksz_platform_data {
+	/* Must be first such that dsa_register_switch() can access it */
+	struct dsa_chip_data cd;
 	u32 chip_id;
-	u16 enabled_ports;
 };
 
 #endif
-- 
2.34.1

Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Daniel Danzberger]

[-- Attachment #1: Type: text/plain, Size: 933 bytes --]

On Tue, 2023-12-05 at 12:12 +0200, Vladimir Oltean wrote:
> On Mon, Dec 04, 2023 at 04:43:15PM +0100, Daniel Danzberger wrote:
> > Fixes a NULL pointer access when registering a switch device that has
> > not been defined via DTS.
> > 
> > This might happen when the switch is used on a platform like x86 that
> > doesn't use DTS and instantiates devices in platform specific init code.
> > 
> > Signed-off-by: Daniel Danzberger <dd@embedd.com>
> > ---
> 
> I'm sorry, I just don't like the state in which your patch leaves the
> driver. Would you mind testing this attached patch instead?
Works fine. I could however only test the platform_data path, not the DTS path.

I would also move the 'enum ksz_chip_id' to the platform include, so instantiating code can use the
enums to set ksz_platform_data.chip_id. (See attached patch)

-- 
Regards

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

[-- Attachment #2: 0001-net-dsa-microchip-properly-support-platform_data-pro.patch --]
[-- Type: text/x-patch, Size: 5460 bytes --]

From 6217924a0127d073ac753c862a62c752320aa151 Mon Sep 17 00:00:00 2001
From: Daniel Danzberger <dd@embedd.com>
Date: Tue, 5 Dec 2023 12:08:16 +0100
Subject: [PATCH] net: dsa: microchip: properly support platform_data probing

The ksz driver has bits and pieces of platform_data probing support, but
it doesn't work.

The conventional thing to do is to have an encapsulating structure for
struct dsa_chip_data that gets put into dev->platform_data. This driver
expects a struct ksz_platform_data, but that doesn't contain a struct
dsa_chip_data as first element, which will obviously not work with
dsa_switch_probe() -> dsa_switch_parse().

Pointing dev->platform_data to a struct dsa_chip_data directly is in
principle possible, but that doesn't work either. The driver has
ksz_switch_detect() to read the device ID from hardware, followed by
ksz_check_device_id() to compare it against a predetermined expected
value. This protects against early errors in the SPI/I2C communication.
With platform_data, the mechanism in ksz_check_device_id() doesn't work
and even leads to NULL pointer dereferences, since of_device_get_match_data()
doesn't work in that probe path.

So obviously, the platform_data support is actually missing, and the
existing handling of struct ksz_platform_data is bogus. Complete the
support by adding a struct dsa_chip_data as first element, and fixing up
ksz_check_device_id() to pick up the platform_data instead of the
unavailable of_device_get_match_data().

The early dev->chip_id assignment from ksz_switch_register() is also
bogus, because ksz_switch_detect() sets it to an initial value. So
remove it.

Also, ksz_platform_data :: enabled_ports isn't used anywhere, delete it.

---
 drivers/net/dsa/microchip/ksz_common.c      | 21 ++++++++++++--------
 drivers/net/dsa/microchip/ksz_common.h      | 19 +-----------------
 include/linux/platform_data/microchip-ksz.h | 22 ++++++++++++++++++++-
 3 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index dc9eea3c8..248dc034d 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1457,15 +1457,23 @@ static const struct ksz_chip_data *ksz_lookup_info(unsigned int prod_num)
 
 static int ksz_check_device_id(struct ksz_device *dev)
 {
-	const struct ksz_chip_data *dt_chip_data;
+	const struct ksz_chip_data *expected_chip_data;
+	u32 expected_chip_id;
 
-	dt_chip_data = of_device_get_match_data(dev->dev);
+	if (dev->pdata) {
+		expected_chip_id = dev->pdata->chip_id;
+		expected_chip_data = ksz_lookup_info(expected_chip_id);
+		if (WARN_ON(!expected_chip_data))
+			return -ENODEV;
+	} else {
+		expected_chip_data = of_device_get_match_data(dev->dev);
+		expected_chip_id = expected_chip_data->chip_id;
+	}
 
-	/* Check for Device Tree and Chip ID */
-	if (dt_chip_data->chip_id != dev->chip_id) {
+	if (expected_chip_id != dev->chip_id) {
 		dev_err(dev->dev,
 			"Device tree specifies chip %s but found %s, please fix it!\n",
-			dt_chip_data->dev_name, dev->info->dev_name);
+			expected_chip_data->dev_name, dev->info->dev_name);
 		return -ENODEV;
 	}
 
@@ -2900,9 +2908,6 @@ int ksz_switch_register(struct ksz_device *dev)
 	int ret;
 	int i;
 
-	if (dev->pdata)
-		dev->chip_id = dev->pdata->chip_id;
-
 	dev->reset_gpio = devm_gpiod_get_optional(dev->dev, "reset",
 						  GPIOD_OUT_LOW);
 	if (IS_ERR(dev->reset_gpio))
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index d1b2db8e6..fe1525ffe 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -14,6 +14,7 @@
 #include <linux/regmap.h>
 #include <net/dsa.h>
 #include <linux/irq.h>
+#include <linux/platform_data/microchip-ksz.h>
 
 #define KSZ_MAX_NUM_PORTS 8
 
@@ -162,24 +163,6 @@ enum ksz_model {
 	LAN9374,
 };
 
-enum ksz_chip_id {
-	KSZ8563_CHIP_ID = 0x8563,
-	KSZ8795_CHIP_ID = 0x8795,
-	KSZ8794_CHIP_ID = 0x8794,
-	KSZ8765_CHIP_ID = 0x8765,
-	KSZ8830_CHIP_ID = 0x8830,
-	KSZ9477_CHIP_ID = 0x00947700,
-	KSZ9896_CHIP_ID = 0x00989600,
-	KSZ9897_CHIP_ID = 0x00989700,
-	KSZ9893_CHIP_ID = 0x00989300,
-	KSZ9567_CHIP_ID = 0x00956700,
-	LAN9370_CHIP_ID = 0x00937000,
-	LAN9371_CHIP_ID = 0x00937100,
-	LAN9372_CHIP_ID = 0x00937200,
-	LAN9373_CHIP_ID = 0x00937300,
-	LAN9374_CHIP_ID = 0x00937400,
-};
-
 enum ksz_regs {
 	REG_IND_CTRL_0,
 	REG_IND_DATA_8,
diff --git a/include/linux/platform_data/microchip-ksz.h b/include/linux/platform_data/microchip-ksz.h
index ea1cc6d82..3f24a7a44 100644
--- a/include/linux/platform_data/microchip-ksz.h
+++ b/include/linux/platform_data/microchip-ksz.h
@@ -20,10 +20,30 @@
 #define __MICROCHIP_KSZ_H
 
 #include <linux/types.h>
+#include <linux/platform_data/dsa.h>
+
+enum ksz_chip_id {
+	KSZ8563_CHIP_ID = 0x8563,
+	KSZ8795_CHIP_ID = 0x8795,
+	KSZ8794_CHIP_ID = 0x8794,
+	KSZ8765_CHIP_ID = 0x8765,
+	KSZ8830_CHIP_ID = 0x8830,
+	KSZ9477_CHIP_ID = 0x00947700,
+	KSZ9896_CHIP_ID = 0x00989600,
+	KSZ9897_CHIP_ID = 0x00989700,
+	KSZ9893_CHIP_ID = 0x00989300,
+	KSZ9567_CHIP_ID = 0x00956700,
+	LAN9370_CHIP_ID = 0x00937000,
+	LAN9371_CHIP_ID = 0x00937100,
+	LAN9372_CHIP_ID = 0x00937200,
+	LAN9373_CHIP_ID = 0x00937300,
+	LAN9374_CHIP_ID = 0x00937400,
+};
 
 struct ksz_platform_data {
+	/* Must be first such that dsa_register_switch() can access it */
+	struct dsa_chip_data cd;
 	u32 chip_id;
-	u16 enabled_ports;
 };
 
 #endif
-- 
2.39.2

Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Vladimir Oltean]

On Tue, Dec 05, 2023 at 12:44:41PM +0100, Daniel Danzberger wrote:
> On Tue, 2023-12-05 at 12:12 +0200, Vladimir Oltean wrote:
> > On Mon, Dec 04, 2023 at 04:43:15PM +0100, Daniel Danzberger wrote:
> > > Fixes a NULL pointer access when registering a switch device that has
> > > not been defined via DTS.
> > > 
> > > This might happen when the switch is used on a platform like x86 that
> > > doesn't use DTS and instantiates devices in platform specific init code.
> > > 
> > > Signed-off-by: Daniel Danzberger <dd@embedd.com>
> > > ---
> > 
> > I'm sorry, I just don't like the state in which your patch leaves the
> > driver. Would you mind testing this attached patch instead?
> Works fine. I could however only test the platform_data path, not the DTS path.
> 
> I would also move the 'enum ksz_chip_id' to the platform include, so instantiating code can use the
> enums to set ksz_platform_data.chip_id. (See attached patch)

Ok, looks alright. Code movement will have to be a separate patch 1/2,
to not pollute the actual code changes from 2/2.

Also, please keep my authorship information and sign off. You can post
the 2 patches as a v2 of this series (separate thread). General info:
when submitting somebody else's patch you have to add your sign off at
the end of the commit, immediately after his, so that it is apparent in
the git log that he didn't submit it himself.

The change doesn't functionally affect the DT probing path (it just
renames variables), so it's reasonable to assume that it won't break
things.

Thanks.

Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Vladimir Oltean]

On Tue, Dec 05, 2023 at 02:04:21PM +0200, Vladimir Oltean wrote:
> You can post the 2 patches as a v2 of this series (separate thread).

Also, for v2 please remember to use scripts/get_maintainer.pl more
diligently. Would it have been a busier period, I would have probably
not noticed your patch.

Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Vladimir Oltean]

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.

I was just thinking, the KSZ9477 has internal PHYs on ports 0-4, and an
internal MDIO bus registered in ksz_mdio_register(). The bus registration
won't work without OF, since it returns early when not finding
of_get_child_by_name(dev->dev->of_node, "mdio").

Don't you need the internal PHY ports to work?

Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Daniel Danzberger]

On Tue, 2023-12-05 at 18:55 +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.
> 
> I was just thinking, the KSZ9477 has internal PHYs on ports 0-4, and an
> internal MDIO bus registered in ksz_mdio_register(). The bus registration
> won't work without OF, since it returns early when not finding
> of_get_child_by_name(dev->dev->of_node, "mdio").
Interesting, I did not notice that.
After the NULL pointer issue was fixed the switch just worked.
> 
> Don't you need the internal PHY ports to work?
For now the switch seems to run just fine, with port 0 being the CPU port and 1-4 being used as
regular switch ports.
I will do some more testing this week however...

And probably checkout some DTS files that use that switch to see what other options I might need
when going the platform_data path.

> 

-- 
Regards

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

Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Vladimir Oltean]

On Tue, Dec 05, 2023 at 06:33:18PM +0100, Daniel Danzberger wrote:
> On Tue, 2023-12-05 at 18:55 +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.
> > 
> > I was just thinking, the KSZ9477 has internal PHYs on ports 0-4, and an
> > internal MDIO bus registered in ksz_mdio_register(). The bus registration
> > won't work without OF, since it returns early when not finding
> > of_get_child_by_name(dev->dev->of_node, "mdio").
> Interesting, I did not notice that.
> After the NULL pointer issue was fixed the switch just worked.
> > 
> > Don't you need the internal PHY ports to work?
> For now the switch seems to run just fine, with port 0 being the CPU port and 1-4 being used as
> regular switch ports.
> I will do some more testing this week however...

What does "regular switch ports" mean? L2 forwarding between them?
Could you give us an "ip addr" output?

> 
> And probably checkout some DTS files that use that switch to see what other options I might need
> when going the platform_data path.

The platform_data code path has reduced functionality, at some point you
will get some pushback to help kickstart the software node conversion.

Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Daniel Danzberger]

On Tue, 2023-12-05 at 20:17 +0200, Vladimir Oltean wrote:
> On Tue, Dec 05, 2023 at 06:33:18PM +0100, Daniel Danzberger wrote:
> > On Tue, 2023-12-05 at 18:55 +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.
> > > 
> > > I was just thinking, the KSZ9477 has internal PHYs on ports 0-4, and an
> > > internal MDIO bus registered in ksz_mdio_register(). The bus registration
> > > won't work without OF, since it returns early when not finding
> > > of_get_child_by_name(dev->dev->of_node, "mdio").
> > Interesting, I did not notice that.
> > After the NULL pointer issue was fixed the switch just worked.
> > > 
> > > Don't you need the internal PHY ports to work?
> > For now the switch seems to run just fine, with port 0 being the CPU port and 1-4 being used as
> > regular switch ports.
> > I will do some more testing this week however...
> 
> What does "regular switch ports" mean? L2 forwarding between them?
> Could you give us an "ip addr" output?
root@t2t-ngr421:~# ip a s
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host proto kernel_lo 
       valid_lft forever preferred_lft forever
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1502 qdisc mq state UP group default qlen 1000
    link/ether 00:13:95:55:cf:d7 brd ff:ff:ff:ff:ff:ff
    inet6 fe80::213:95ff:fe55:cfd7/64 scope link proto kernel_ll 
       valid_lft forever preferred_lft forever
3: lan1@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master br-lan state UP group
default qlen 1000
    link/ether 00:13:95:55:cf:d7 brd ff:ff:ff:ff:ff:ff
4: lan2@eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc noqueue master br-lan state
LOWERLAYERDOWN group default qlen 1000
    link/ether 00:13:95:55:cf:d7 brd ff:ff:ff:ff:ff:ff
5: lan3@eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc noqueue master br-lan state
LOWERLAYERDOWN group default qlen 1000
    link/ether 00:13:95:55:cf:d7 brd ff:ff:ff:ff:ff:ff
6: lan4@eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc noqueue master br-lan state
LOWERLAYERDOWN group default qlen 1000
    link/ether 00:13:95:55:cf:d7 brd ff:ff:ff:ff:ff:ff
7: br-lan: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
    link/ether 00:13:95:55:cf:d7 brd ff:ff:ff:ff:ff:ff
    inet 192.168.20.1/24 brd 192.168.20.255 scope global br-lan
       valid_lft forever preferred_lft forever
    inet6 fd4e:2bf5:cc74::1/60 scope global noprefixroute 
       valid_lft forever preferred_lft forever
    inet6 fe80::213:95ff:fe55:cfd7/64 scope link proto kernel_ll 
       valid_lft forever preferred_lft forever

The lan* devices are then bridged together:

root@t2t-ngr421:~# brctl show
bridge name	bridge id		STP enabled	interfaces
br-lan		7fff.00139555cfd7	no		lan4
							lan2
							lan3
							lan1


-- 
Regards

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

Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Vladimir Oltean]

On Tue, Dec 05, 2023 at 11:15:58PM +0100, Daniel Danzberger wrote:
> The lan* devices are then bridged together:
> 
> root@t2t-ngr421:~# brctl show
> bridge name	bridge id		STP enabled	interfaces
> br-lan		7fff.00139555cfd7	no		lan4
> 							lan2
> 							lan3
> 							lan1

Ok, so it forwards traffic because the ports are bridged, not that it
does that by default.

I see in your output that the internal PHY network interfaces are
registered and functional, and now I know how that works.

ksz_mdio_register() is indeed bypassed by the lack of OF as I suspected,
so it's not that function which is responsible for creating the MDIO bus.
But this piece of code from dsa_switch_setup() will kick in:

	if (!ds->user_mii_bus && ds->ops->phy_read) {
		ds->user_mii_bus = mdiobus_alloc();
		if (!ds->user_mii_bus) {
			err = -ENOMEM;
			goto teardown;
		}

		dsa_user_mii_bus_init(ds);

		dn = of_get_child_by_name(ds->dev->of_node, "mdio");

		err = of_mdiobus_register(ds->user_mii_bus, dn);
		of_node_put(dn);
		if (err < 0)
			goto free_user_mii_bus;
	}

aka DSA will set up a ds->user_mii_bus which calls into ds->ops->phy_read()
and ds->ops->phy_write() to access the internal PHYs. Although this bus
may be OF-based (if "dn" above is non-NULL), the of_mdiobus_register()
implementation simply calls mdiobus_register() if there is no OF node.

So, surprisingly, there is enough redundancy between DSA mechanisms that
platform_data kinda works.

Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Andrew Lunn]

> So, surprisingly, there is enough redundancy between DSA mechanisms that
> platform_data kinda works.

I have x86 platforms using mv88e6xxx with platform data. Simple
systems do work, the platforms i have only make use of the internal
PHYs. This is partially because of history. DSA is older than the
adoption of DT. The mv88e6xxx driver and DSA used to be purely
platform data driven, and we have not yet broken that.

	Andrew

Re: [PATCH] net: dsa: microchip: fix NULL pointer dereference on platform init

[Vladimir Oltean]

On Wed, Dec 06, 2023 at 04:26:52PM +0100, Andrew Lunn wrote:
> > So, surprisingly, there is enough redundancy between DSA mechanisms that
> > platform_data kinda works.
> 
> I have x86 platforms using mv88e6xxx with platform data. Simple
> systems do work, the platforms i have only make use of the internal
> PHYs. This is partially because of history. DSA is older than the
> adoption of DT. The mv88e6xxx driver and DSA used to be purely
> platform data driven, and we have not yet broken that.
> 
> 	Andrew

I'm not saying that platform_data did not have its time and place, but
Device Tree clearly won, and platform_data is a rarely untested minority
nowadays.

We don't have to break platform_data in mv88e6xxx and in the DSA core
all of a sudden. It can coexist with software nodes for a while, as
alternative solutions to the same problem.