Re: [RFC net-next 1/3] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX325x (AC3x)

[Vadym Kochan <vadym.kochan@plvision.eu>]

Hi Florian,

On Thu, Feb 27, 2020 at 08:22:02PM -0800, Florian Fainelli wrote:
> 
> 
> On 2/25/2020 8:30 AM, Vadym Kochan wrote:
> > Marvell Prestera 98DX326x integrates up to 24 ports of 1GbE with 8
> > ports of 10GbE uplinks or 2 ports of 40Gbps stacking for a largely
> > wireless SMB deployment.
> > 
> > This driver implementation includes only L1 & basic L2 support.
> > 
> > The core Prestera switching logic is implemented in prestera.c, there is
> > an intermediate hw layer between core logic and firmware. It is
> > implemented in prestera_hw.c, the purpose of it is to encapsulate hw
> > related logic, in future there is a plan to support more devices with
> > different HW related configurations.
> > 
> > The following Switchdev features are supported:
> > 
> >     - VLAN-aware bridge offloading
> >     - VLAN-unaware bridge offloading
> >     - FDB offloading (learning, ageing)
> >     - Switchport configuration
> > 
> > Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
> > Signed-off-by: Andrii Savka <andrii.savka@plvision.eu>
> > Signed-off-by: Oleksandr Mazur <oleksandr.mazur@plvision.eu>
> > Signed-off-by: Serhiy Boiko <serhiy.boiko@plvision.eu>
> > Signed-off-by: Serhiy Pshyk <serhiy.pshyk@plvision.eu>
> > Signed-off-by: Taras Chornyi <taras.chornyi@plvision.eu>
> > Signed-off-by: Volodymyr Mytnyk <volodymyr.mytnyk@plvision.eu>
> 
> Very little to pick on, the driver is nice and clean, great job!
> 
> > ---
> 
> [snip]
> 
> > +#define PORT_STATS_CACHE_TIMEOUT_MS	(msecs_to_jiffies(1000))
> > +#define PORT_STATS_CNT	(sizeof(struct mvsw_pr_port_stats) / sizeof(u64))
> 
> All entries in mvsw_pr_port_stats are u64 so you can use ARRAY_SIZE() here.
> 
> [snip]
> 
> > +
> > +	err = register_netdev(net_dev);
> > +	if (err)
> > +		goto err_register_netdev;
> > +
> > +	list_add(&port->list, &sw->port_list);
> 
> As soon as you publish the network device it can be used by notifiers,
> user-space etc, better do this as the last operation.
> 
> [snip]
> 
> > +int mvsw_pr_hw_port_stats_get(const struct mvsw_pr_port *port,
> > +			      struct mvsw_pr_port_stats *stats)
> > +{
> > +	struct mvsw_msg_port_stats_ret resp;
> > +	struct mvsw_msg_port_attr_cmd req = {
> > +		.attr = MVSW_MSG_PORT_ATTR_STATS,
> > +		.port = port->hw_id,
> > +		.dev = port->dev_id
> > +	};
> > +	u64 *hw_val = resp.stats;
> > +	int err;
> > +
> > +	err = fw_send_req_resp(port->sw, MVSW_MSG_TYPE_PORT_ATTR_GET,
> > +			       &req, &resp);
> > +	if (err)
> > +		return err;
> > +
> > +	stats->good_octets_received = hw_val[MVSW_PORT_GOOD_OCTETS_RCV_CNT];
> 
> This seems error prone and not scaling really well, since all stats
> member are u64 and they are ordered in the same way as the response, is
> not a memcpy() sufficient here?
> -- 

The reason for this is that struct mvsw_pr_port_stats and struct
mvsw_msg_port_stats_ret has very different usage context, struct
mvsw_pr_port_stats might have different layout, like additional fields
which is needed for the higher layer, so I think it would be better to
fill it member by member which has related one received from the
firmware. So, what I mean is to avoid mixing data transfer objects with
the generic ones. I am totally agree that memcpy looks more simpler, but
it may bring bugs because the generic stats struct may be differ from the
one which is used for transmission.

> Florian

Regards,
Vadym Kochan