Fri, Feb 28, 2020 at 09:06:02AM CET, vadym.kochan@plvision.eu wrote:

Hi Jiri,

On Thu, Feb 27, 2020 at 03:22:59PM +0100, Jiri Pirko wrote:

quoted

Tue, Feb 25, 2020 at 05:30:54PM CET, vadym.kochan@plvision.eu wrote:

quoted

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 <redacted>
Signed-off-by: Andrii Savka <redacted>
Signed-off-by: Oleksandr Mazur <redacted>
Signed-off-by: Serhiy Boiko <redacted>
Signed-off-by: Serhiy Pshyk <redacted>
Signed-off-by: Taras Chornyi <redacted>
Signed-off-by: Volodymyr Mytnyk <redacted>
---

[SNIP]

quoted

quoted

+};
+
+struct mvsw_msg_cmd {
+	u32 type;
+} __packed __aligned(4);
+
+struct mvsw_msg_ret {
+	struct mvsw_msg_cmd cmd;
+	u32 status;
+} __packed __aligned(4);
+
+struct mvsw_msg_common_request {
+	struct mvsw_msg_cmd cmd;
+} __packed __aligned(4);
+
+struct mvsw_msg_common_response {
+	struct mvsw_msg_ret ret;
+} __packed __aligned(4);
+
+union mvsw_msg_switch_param {
+	u32 ageing_timeout;
+};
+
+struct mvsw_msg_switch_attr_cmd {
+	struct mvsw_msg_cmd cmd;
+	union mvsw_msg_switch_param param;
+} __packed __aligned(4);
+
+struct mvsw_msg_switch_init_ret {
+	struct mvsw_msg_ret ret;
+	u32 port_count;
+	u32 mtu_max;
+	u8  switch_id;
+	u8  mac[ETH_ALEN];
+} __packed __aligned(4);
+
+struct mvsw_msg_port_autoneg_param {
+	u64 link_mode;
+	u8  enable;
+	u8  fec;
+};
+
+struct mvsw_msg_port_cap_param {
+	u64 link_mode;
+	u8  type;
+	u8  fec;
+	u8  transceiver;
+};
+
+union mvsw_msg_port_param {
+	u8  admin_state;
+	u8  oper_state;
+	u32 mtu;
+	u8  mac[ETH_ALEN];
+	u8  accept_frm_type;
+	u8  learning;
+	u32 speed;
+	u8  flood;
+	u32 link_mode;
+	u8  type;
+	u8  duplex;
+	u8  fec;
+	u8  mdix;
+	struct mvsw_msg_port_autoneg_param autoneg;
+	struct mvsw_msg_port_cap_param cap;
+};
+
+struct mvsw_msg_port_attr_cmd {
+	struct mvsw_msg_cmd cmd;
+	u32 attr;
+	u32 port;
+	u32 dev;
+	union mvsw_msg_port_param param;
+} __packed __aligned(4);
+
+struct mvsw_msg_port_attr_ret {
+	struct mvsw_msg_ret ret;
+	union mvsw_msg_port_param param;
+} __packed __aligned(4);
+
+struct mvsw_msg_port_stats_ret {
+	struct mvsw_msg_ret ret;
+	u64 stats[MVSW_PORT_CNT_MAX];
+} __packed __aligned(4);
+
+struct mvsw_msg_port_info_cmd {
+	struct mvsw_msg_cmd cmd;
+	u32 port;
+} __packed __aligned(4);
+
+struct mvsw_msg_port_info_ret {
+	struct mvsw_msg_ret ret;
+	u32 hw_id;
+	u32 dev_id;
+	u16 fp_id;
+} __packed __aligned(4);
+
+struct mvsw_msg_vlan_cmd {
+	struct mvsw_msg_cmd cmd;
+	u32 port;
+	u32 dev;
+	u16 vid;
+	u8  is_member;
+	u8  is_tagged;
+} __packed __aligned(4);
+
+struct mvsw_msg_fdb_cmd {
+	struct mvsw_msg_cmd cmd;
+	u32 port;
+	u32 dev;
+	u8  mac[ETH_ALEN];
+	u16 vid;
+	u8  dynamic;
+	u32 flush_mode;
+} __packed __aligned(4);
+
+struct mvsw_msg_event {
+	u16 type;
+	u16 id;
+} __packed __aligned(4);
+
+union mvsw_msg_event_fdb_param {
+	u8 mac[ETH_ALEN];
+};
+
+struct mvsw_msg_event_fdb {
+	struct mvsw_msg_event id;
+	u32 port_id;
+	u32 vid;
+	union mvsw_msg_event_fdb_param param;
+} __packed __aligned(4);
+
+union mvsw_msg_event_port_param {
+	u32 oper_state;
+};
+
+struct mvsw_msg_event_port {
+	struct mvsw_msg_event id;
+	u32 port_id;
+	union mvsw_msg_event_port_param param;
+} __packed __aligned(4);
+
+struct mvsw_msg_bridge_cmd {
+	struct mvsw_msg_cmd cmd;
+	u32 port;
+	u32 dev;
+	u16 bridge;
+} __packed __aligned(4);
+
+struct mvsw_msg_bridge_ret {
+	struct mvsw_msg_ret ret;
+	u16 bridge;
+} __packed __aligned(4);
+
+#define fw_check_resp(_response)	\
+({								\
+	int __er = 0;						\
+	typeof(_response) __r = (_response);			\
+	if (__r->ret.cmd.type != MVSW_MSG_TYPE_ACK)		\
+		__er = -EBADE;					\
+	else if (__r->ret.status != MVSW_MSG_ACK_OK)		\
+		__er = -EINVAL;					\
+	(__er);							\
+})
+
+#define __fw_send_req_resp(_switch, _type, _request, _response, _wait)	\

Please try to avoid doing functions in macros like this one and the
previous one.

quoted

+({								\
+	int __e;						\
+	typeof(_switch) __sw = (_switch);			\
+	typeof(_request) __req = (_request);			\
+	typeof(_response) __resp = (_response);			\
+	__req->cmd.type = (_type);				\
+	__e = __sw->dev->send_req(__sw->dev,			\
+		(u8 *)__req, sizeof(*__req),			\
+		(u8 *)__resp, sizeof(*__resp),			\
+		_wait);						\
+	if (!__e)						\
+		__e = fw_check_resp(__resp);			\
+	(__e);							\
+})
+
+#define fw_send_req_resp(_sw, _t, _req, _resp)	\
+	__fw_send_req_resp(_sw, _t, _req, _resp, 0)
+
+#define fw_send_req_resp_wait(_sw, _t, _req, _resp, _wait)	\
+	__fw_send_req_resp(_sw, _t, _req, _resp, _wait)
+
+#define fw_send_req(_sw, _t, _req)	\

This should be function, not define

Yeah, I understand your point, but here was the reason:

all packed structs which defined here in prestera_hw.c
are used for transmission request/return to/from the firmware, each of
the struct requires to have req/ret member (depends if it is request or
return from the firmware), and the purpose of the macro is to avoid case
when someone can forget to add req/ret member to the new such structure.

You should refactor your code to make this cleaner. Define acting as
a function working with random structures counting with 2 member names in
them, that is not nice :/

quoted

quoted

+({							\
+	struct mvsw_msg_common_response __re;		\
+	(fw_send_req_resp(_sw, _t, _req, &__re));	\
+})
+

Regards,
Vadym Kochan,