linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vadym Kochan <vadym.kochan@plvision.eu>
To: Jiri Pirko <jiri@resnulli.us>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"David S . Miller" <davem@davemloft.net>,
	Oleksandr Mazur <oleksandr.mazur@plvision.eu>,
	Taras Chornyi <taras.chornyi@plvision.eu>,
	Serhiy Boiko <serhiy.boiko@plvision.eu>,
	Andrii Savka <andrii.savka@plvision.eu>,
	Volodymyr Mytnyk <volodymyr.mytnyk@plvision.eu>,
	Serhiy Pshyk <serhiy.pshyk@plvision.eu>
Subject: Re: [RFC net-next 1/3] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX325x (AC3x)
Date: Fri, 28 Feb 2020 08:06:02 +0000	[thread overview]
Message-ID: <20200228080554.GA17929@plvision.eu> (raw)
In-Reply-To: <20200227142259.GF26061@nanopsycho>

Hi Jiri,

On Thu, Feb 27, 2020 at 03:22:59PM +0100, Jiri Pirko wrote:
> Tue, Feb 25, 2020 at 05:30:54PM CET, vadym.kochan@plvision.eu 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>
> >---

[SNIP]

> >+};
> >+
> >+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.
> 
> 
> >+({								\
> >+	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.

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

Regards,
Vadym Kochan,

  reply	other threads:[~2020-02-28  8:06 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-25 16:30 [RFC net-next 0/3] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX326x (AC3x) Vadym Kochan
2020-02-25 16:30 ` [RFC net-next 1/3] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX325x (AC3x) Vadym Kochan
2020-02-25 22:05   ` Andrew Lunn
2020-02-26 15:54   ` Jiri Pirko
2020-02-27 21:32     ` Vadym Kochan
2020-02-27 21:43       ` Andrew Lunn
2020-02-27 23:50         ` Vadym Kochan
2020-02-28  6:36           ` Jiri Pirko
2020-02-28 14:03             ` Andrew Lunn
2020-02-28  6:34       ` Jiri Pirko
2020-02-28  9:44         ` Vadym Kochan
2020-02-28 10:59           ` Jiri Pirko
2020-02-27 14:22   ` Jiri Pirko
2020-02-28  8:06     ` Vadym Kochan [this message]
2020-02-28 11:03       ` Jiri Pirko
2020-02-28  4:22   ` Florian Fainelli
2020-02-28  8:17     ` Vadym Kochan
2020-03-05 14:49   ` Ido Schimmel
2020-05-02 15:20     ` Vadym Kochan
2020-05-05  4:01       ` Vadym Kochan
2020-02-25 16:30 ` [RFC net-next 2/3] net: marvell: prestera: Add PCI interface support Vadym Kochan
2020-02-25 20:49   ` Andrew Lunn
2020-02-27 11:05   ` Jiri Pirko
2020-02-28 16:54     ` Vadym Kochan
2020-02-29  7:58       ` Jiri Pirko
2020-03-01  2:12         ` Jakub Kicinski
2020-02-25 16:30 ` [RFC net-next 3/3] dt-bindings: marvell,prestera: Add address mapping for Prestera Switchdev PCIe driver Vadym Kochan
2020-02-28  4:24   ` Florian Fainelli
2020-02-25 22:12 ` [RFC net-next 0/3] net: marvell: prestera: Add Switchdev driver for Prestera family ASIC device 98DX326x (AC3x) Andrew Lunn
2020-02-25 22:45 ` Chris Packham
2020-02-28 16:50   ` Vadym Kochan
2020-02-26 15:44 ` Jiri Pirko
2020-02-26 16:38 ` Roopa Prabhu
2020-03-05 15:01 ` Ido Schimmel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200228080554.GA17929@plvision.eu \
    --to=vadym.kochan@plvision.eu \
    --cc=andrii.savka@plvision.eu \
    --cc=davem@davemloft.net \
    --cc=jiri@resnulli.us \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=oleksandr.mazur@plvision.eu \
    --cc=serhiy.boiko@plvision.eu \
    --cc=serhiy.pshyk@plvision.eu \
    --cc=taras.chornyi@plvision.eu \
    --cc=volodymyr.mytnyk@plvision.eu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).