Re: [PATCH net-next v2] net: dsa: Mock-up driver

[David Miller]

From: Florian Fainelli 
Date: Thu, 30 Mar 2017 18:43:21 -0700

> This patch adds support for a DSA mock-up driver which essentially does
> the following:
> 
> - registers/unregisters 4 fixed PHYs to the slave network devices
> - uses eth0 (configurable) as the master netdev
> - registers the switch as a fixed MDIO device against the fixed MDIO bus
>   at address 31
> - includes dynamic debug prints for dsa_switch_ops functions that can be
>   enabled to get call traces
> 
> This is a good way to test modular builds as well as exercise the DSA
> APIs without requiring access to real hardware. This does not test the
> data-path, although this could be added later on.
> 
> Signed-off-by: Florian Fainelli 
> ---
> Changes in v2;
> 
> - removed an unnecessary change to include/linux/fixed_phy.h

This looks extremely useful, applied, thanks!

Re: [PATCH net-next v2] net: dsa: Mock-up driver

[Andrew Lunn]

> Actually we do not, because netdev_uses_dsa() returns true only when
> dst->rcv is different from NULL. When dst->rcv is NULL we completely
> bypass the DSA hook in eth_type_trans() and everything is well, the
> master network device is the one receiving packets.

static inline bool netdev_uses_dsa(struct net_device *dev)
{
#if IS_ENABLED(CONFIG_NET_DSA)
if (dev->dsa_ptr != NULL)
return dsa_uses_tagged_protocol(dev->dsa_ptr);
#endif
return false;
}

and

static inline bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
{
return dst->rcv != NULL;
}

Yep, i just needed to dig further...

Thanks
Andrew

Re: [PATCH net-next v2] net: dsa: Mock-up driver

[Florian Fainelli]

Hi Andrew,

On 03/31/2017 09:06 AM, Andrew Lunn wrote:
> Hi Florian
> 
>> +static enum dsa_tag_protocol dsa_loop_get_protocol(struct dsa_switch *ds)
>> +{
>> +dev_dbg(ds->dev, "%s\n", __func__);
>> +
>> +return DSA_TAG_PROTO_NONE;
>> +}
> 
> I'm wondering how safe this is:
> 
> static const struct dsa_device_ops none_ops = {
> .xmit   = dsa_slave_notag_xmit,
> .rcv= NULL,
> };
> 
> /*
>  * If the CPU connects to this switch, set the switch tree
>  * tagging protocol to the preferred tagging format of this
>  * switch.
>  */
> if (dst->cpu_switch == ds) {
> enum dsa_tag_protocol tag_protocol;
> 
> tag_protocol = ops->get_tag_protocol(ds);
> dst->tag_ops = dsa_resolve_tag_protocol(tag_protocol);
> if (IS_ERR(dst->tag_ops))
> return PTR_ERR(dst->tag_ops);
> 
> dst->rcv = dst->tag_ops->rcv;
> }
> 
> 
> static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>   struct packet_type *pt, struct net_device *orig_dev)
> {
> struct dsa_switch_tree *dst = dev->dsa_ptr;
> 
> if (unlikely(dst == NULL)) {
> kfree_skb(skb);
> return 0;
> }
> 
> return dst->rcv(skb, dev, pt, orig_dev);
> }
> 
> static struct packet_type dsa_pack_type __read_mostly = {
> .type   = cpu_to_be16(ETH_P_XDSA),
> .func   = dsa_switch_rcv,
> };
> 
> It looks like when a frame is received, we are going to dereference a
> NULL pointer.

Actually we do not, because netdev_uses_dsa() returns true only when
dst->rcv is different from NULL. When dst->rcv is NULL we completely
bypass the DSA hook in eth_type_trans() and everything is well, the
master network device is the one receiving packets.

This is actually the intended behavior for netdev_uses_dsa() because it
really tells whether there is a DSA tagging protocol set-up and that is
what NIC drivers (e.g: bcmsysport) would care about.

Thanks!
-- 
Florian

Re: [PATCH net-next v2] net: dsa: Mock-up driver

[Andrew Lunn]

Hi Florian

> +static enum dsa_tag_protocol dsa_loop_get_protocol(struct dsa_switch *ds)
> +{
> + dev_dbg(ds->dev, "%s\n", __func__);
> +
> + return DSA_TAG_PROTO_NONE;
> +}

I'm wondering how safe this is:

static const struct dsa_device_ops none_ops = {
.xmit   = dsa_slave_notag_xmit,
.rcv= NULL,
};

/*
 * If the CPU connects to this switch, set the switch tree
 * tagging protocol to the preferred tagging format of this
 * switch.
 */
if (dst->cpu_switch == ds) {
enum dsa_tag_protocol tag_protocol;

tag_protocol = ops->get_tag_protocol(ds);
dst->tag_ops = dsa_resolve_tag_protocol(tag_protocol);
if (IS_ERR(dst->tag_ops))
return PTR_ERR(dst->tag_ops);

dst->rcv = dst->tag_ops->rcv;
}


static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
  struct packet_type *pt, struct net_device *orig_dev)
{
struct dsa_switch_tree *dst = dev->dsa_ptr;

if (unlikely(dst == NULL)) {
kfree_skb(skb);
return 0;
}

return dst->rcv(skb, dev, pt, orig_dev);
}

static struct packet_type dsa_pack_type __read_mostly = {
.type   = cpu_to_be16(ETH_P_XDSA),
.func   = dsa_switch_rcv,
};

It looks like when a frame is received, we are going to dereference a
NULL pointer.

Either we need a NOP rcv function, or we don't register dsa_pack_type
if rcv is NULL.

   Andrew