Date:	Tue, 24 Feb 2015 14:55:58 -0800
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Rafał Miłecki <zajec5@...il.com>,
	Andy Gospodarek <gospo@...ulusnetworks.com>
CC:	"David S. Miller" <davem@...emloft.net>,
	Network Development <netdev@...r.kernel.org>,
	Jonas Gorski <jogo@...nwrt.org>,
	Hauke Mehrtens <hauke@...ke-m.de>,
	Felix Fietkau <nbd@...nwrt.org>, Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH] net: phy: b53: switchdev driver for Broadcom BCM53xx
 switches

On 24/02/15 14:50, Rafał Miłecki wrote:
> On 24 February 2015 at 23:30, Andy Gospodarek <gospo@...ulusnetworks.com> wrote:
>> On Tue, Feb 24, 2015 at 06:42:07PM +0100, Rafał Miłecki wrote:
>>> BCM53xx is series of Broadcom Ethernet switches that can be found in
>>> various (mostly home) routers.
>>> They are quite simple switches with mainly just support for:
>>> 1) Tagging incoming packets (PVID)
>>> 2) Untagging outgoing packets
>>> 3) Forwarding all packets across a single VLAN
>>>
>>> This driver is split into common code (module) and bus specific code.
>>> Right now only PHY (MDIO) support is included, other could follow after
>>> accepting this driver. It was successfully tested on BCM4706 SoC with
>>> BCM53125.
>>>
>>> You could notice it's yet another try of submitting b53 driver. This
>>> time it was modified to use recently introduced switchdev API which
>>> hopefully make it possible to accept it mainline.
>>
>> I must confess I do not know the entire history of this driver, but I
>> have a few comments....
> 
> I guess previous discussions were mostly focusing on API (like
> swconfig), so a good review of b53 is still highly wanted!
> 
> 
>>> diff --git a/drivers/net/phy/b53/b53_common.c b/drivers/net/phy/b53/b53_common.c
>>> new file mode 100644
>>> index 0000000..fce6b71
>>> --- /dev/null
>>> +++ b/drivers/net/phy/b53/b53_common.c
>> [...]
>>> +
>>> +/*****************
>>> + * Net device ops
>>> + *****************/
>>> +
>>> +static netdev_tx_t b53_port_xmit(struct sk_buff *skb, struct net_device *dev)
>>> +{
>>> +     dev_kfree_skb(skb);
>>> +     dev->stats.tx_dropped++;
>>> +
>>> +     return NETDEV_TX_OK;
>>> +}
>>> +
>>> (...)
>>> +
>>> +static const struct net_device_ops b53_port_netdev_ops = {
>>> +     .ndo_start_xmit                 = b53_port_xmit,
>>> +     .ndo_vlan_rx_add_vid            = b53_port_vlan_rx_add_vid,
>>> +     .ndo_vlan_rx_kill_vid           = b53_port_vlan_rx_kill_vid,
>>> +     .ndo_switch_parent_id_get       = b53_port_switch_parent_id_get,
>>> +     .ndo_bridge_setlink             = b53_port_bridge_setlink,
>>> +};
>>
>> I see there is an xmit function, but no napi registration at all.
>>
>> Is this simply because the SoC ethernet dev is doing all the frame rx?
>> Is is able to demux the frames correctly, so it is clear which swXpY
>> received the frame?  That will be necessary to ensure that standard
>> applications that want to open sockets directly on the needed interfaces
>> can still function properly.
> 
> If you take a look at b53_port_xmit you'll start understanding why
> there is no NAPI ;)
> 
> As said in the commit message, these switches are really simple
> devices. We can't actually send packets to the particular ports
> (unless something has changed in the more recent hardware).

These switches all support Broadcom tags, so you could use your host CPU
Ethernet MAC to send/receive packets to/from specific ports of the
switch, and then this is just like DSA, but everything that you say
below is true.

> We also
> don't receive any packets from the switch itself! Really, switch
> doesn't give as any IRQ when receiving frame, it doesn't put it in any
> memory (DMA) we could access.
> So we can't even consider knowing which port has received the frame.

If you ever wanted to use Broadcom tags, your CPU Ethernet MAC would
receive packets and be able to deliver them to per-port net_devices, but
the physical packets are indeed received from your host CPU Ethernet MAC
and transfered by it into DRAM.

> 
> So if we want to get any packets from switch port N we have to:
> 1) Assign some PVID to the port N
> 2) Put port N and "CPU port" in the same VLAN (matching PVID)
> 3) Starting with above switch will tag every packet received on port N
> and forward it to the "CPU port"
> 4) Listen on the Ethernet device connected to the "CPU port" for
> packets with a proper VLAN ID
> 
> Now with the above example there are 2 most common scenarios you may
> want to consider:
> 
> 1) Having full control over every port
> It requires assigning unique VLAN for every single port. Any switching
> (forwarding packets) has to be done in software then.
> 
> 2) Having few groups of ports
> This is the most common case for home routers. You usually want to
> have group of "LAN" ports when you don't really care which port
> provided a given packet. This also allows using switch for offloading
> traffic between machines the same ports group.

With that use case in mind, yes, your driver is doing exactly that,
allowing you to define VLAN membership for particular ports within a
group, all of this gets sent to the host CPU, where the IP routing happens.

> 
> So all this driver really does it allowing us to *configure* the
> switch. It doesn't handle any RX/TX.
> 


-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists