Re: [PATCH net-next v2 08/10] net: dsa: Add support for platform data
On 01/13/2017 02:37 PM, Florian Fainelli wrote:
> On 01/13/2017 06:04 AM, Andrew Lunn wrote:
>>> index cd91070b5467..d326fc4afad7 100644
>>> --- a/net/dsa/dsa2.c
>>> +++ b/net/dsa/dsa2.c
>>> @@ -81,17 +81,23 @@ static void dsa_dst_del_ds(struct dsa_switch_tree *dst,
>>>
>>> static bool dsa_port_is_valid(struct dsa_port *port)
>>> {
>>> - return !!port->dn;
>>> + return !!(port->dn || port->name);
>>> }
>>
>> Does this clash with Viviens recent change to make names optional and
>> have the kernel assign it?
>
> So there were two ways to look at this, one was that could check here
> that ds->pd is assigned and port->name is assigned, which means that
> platform data has to provide valid port name. We can also eliminate this
> check entirely because we now support NULL names just fines.
Considering that the comment above struct dsa_chip_data::port_names in
net/dsa/dsa.h is pretty clear about the port_names usage, I am tempted
to keep the code as-is since without a name, for platform data, we would
not have a way to tell if a port is disabled or not.
Does that work for you?
--
Florian
Re: [PATCH net-next v2 08/10] net: dsa: Add support for platform data
On 01/13/2017 06:11 AM, Andrew Lunn wrote:
>> static int _dsa_register_switch(struct dsa_switch *ds, struct device *dev)
>> {
>> +struct dsa_chip_data *pdata = dev->platform_data;
>> struct device_node *np = dev->of_node;
>> struct dsa_switch_tree *dst;
>> struct device_node *ports;
>> u32 tree, index;
>> int i, err;
>>
>> -err = dsa_parse_member_dn(np, &tree, &index);
>> -if (err)
>> -return err;
>> +if (np) {
>> +err = dsa_parse_member_dn(np, &tree, &index);
>> +if (err)
>> +return err;
>>
>> -ports = dsa_get_ports(ds, np);
>> -if (IS_ERR(ports))
>> -return PTR_ERR(ports);
>> +ports = dsa_get_ports(ds, np);
>> +if (IS_ERR(ports))
>> +return PTR_ERR(ports);
>>
>> -err = dsa_parse_ports_dn(ports, ds);
>> -if (err)
>> -return err;
>> +err = dsa_parse_ports_dn(ports, ds);
>> +if (err)
>> +return err;
>> +} else {
>> +err = dsa_parse_member(pdata, &tree, &index);
>
Hello Andrew,
> Hi Florian
>
> Maybe it is hiding, but i don't see anywhere you check that pdata !=
> NULL.
You are right, there is not such a check, it should probably be added
early on.
>
> At least for x86 platforms, i don't expect we are booting using
> platform data like ARM systems used to do. I think it is more likely a
> glue module will be loaded. It looks up the MDIO bus and appends a
> platform data to an MDIO device. The switch driver then needs to load
> and use the platform data. But if things happen in a different order,
> it could be the switch driver probes before the glue driver, meaning
> pdata is NULL.
That's very valid, I will fix this, thanks!
>
> Do we even want to return -EPROBE_DEFERED?
I was trying to exercise that code path a little bit, but could not
quite make sense of what I was seeing, let me try again with more tracing.
--
Florian
Re: [PATCH net-next v2 08/10] net: dsa: Add support for platform data
On 01/13/2017 06:04 AM, Andrew Lunn wrote:
>> index cd91070b5467..d326fc4afad7 100644
>> --- a/net/dsa/dsa2.c
>> +++ b/net/dsa/dsa2.c
>> @@ -81,17 +81,23 @@ static void dsa_dst_del_ds(struct dsa_switch_tree *dst,
>>
>> static bool dsa_port_is_valid(struct dsa_port *port)
>> {
>> -return !!port->dn;
>> +return !!(port->dn || port->name);
>> }
>
> Does this clash with Viviens recent change to make names optional and
> have the kernel assign it?
So there were two ways to look at this, one was that could check here
that ds->pd is assigned and port->name is assigned, which means that
platform data has to provide valid port name. We can also eliminate this
check entirely because we now support NULL names just fines.
>
> I suppose you could use an name of "eth%d"? Is it worth adding a
> comment to the platform data structure?
Humm, that could be done, maybe for simplicity we can just let
net/dsa/dsa2.c assign names either based on what platform data provided,
or by falling back to eth%d.
Thanks!
--
Florian
Re: [PATCH net-next v2 08/10] net: dsa: Add support for platform data
> static int _dsa_register_switch(struct dsa_switch *ds, struct device *dev)
> {
> + struct dsa_chip_data *pdata = dev->platform_data;
> struct device_node *np = dev->of_node;
> struct dsa_switch_tree *dst;
> struct device_node *ports;
> u32 tree, index;
> int i, err;
>
> - err = dsa_parse_member_dn(np, &tree, &index);
> - if (err)
> - return err;
> + if (np) {
> + err = dsa_parse_member_dn(np, &tree, &index);
> + if (err)
> + return err;
>
> - ports = dsa_get_ports(ds, np);
> - if (IS_ERR(ports))
> - return PTR_ERR(ports);
> + ports = dsa_get_ports(ds, np);
> + if (IS_ERR(ports))
> + return PTR_ERR(ports);
>
> - err = dsa_parse_ports_dn(ports, ds);
> - if (err)
> - return err;
> + err = dsa_parse_ports_dn(ports, ds);
> + if (err)
> + return err;
> + } else {
> + err = dsa_parse_member(pdata, &tree, &index);
Hi Florian
Maybe it is hiding, but i don't see anywhere you check that pdata !=
NULL.
At least for x86 platforms, i don't expect we are booting using
platform data like ARM systems used to do. I think it is more likely a
glue module will be loaded. It looks up the MDIO bus and appends a
platform data to an MDIO device. The switch driver then needs to load
and use the platform data. But if things happen in a different order,
it could be the switch driver probes before the glue driver, meaning
pdata is NULL.
Do we even want to return -EPROBE_DEFERED?
Andrew
Re: [PATCH net-next v2 08/10] net: dsa: Add support for platform data
> index cd91070b5467..d326fc4afad7 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -81,17 +81,23 @@ static void dsa_dst_del_ds(struct dsa_switch_tree *dst,
>
> static bool dsa_port_is_valid(struct dsa_port *port)
> {
> - return !!port->dn;
> + return !!(port->dn || port->name);
> }
Does this clash with Viviens recent change to make names optional and
have the kernel assign it?
I suppose you could use an name of "eth%d"? Is it worth adding a
comment to the platform data structure?
Andrew
