| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 28 Feb 2020 12:03:32 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Vadym Kochan <vadym.kochan@...ision.eu>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"David S . Miller" <davem@...emloft.net>,
Oleksandr Mazur <oleksandr.mazur@...ision.eu>,
Taras Chornyi <taras.chornyi@...ision.eu>,
Serhiy Boiko <serhiy.boiko@...ision.eu>,
Andrii Savka <andrii.savka@...ision.eu>,
Volodymyr Mytnyk <volodymyr.mytnyk@...ision.eu>,
Serhiy Pshyk <serhiy.pshyk@...ision.eu>
Subject: Re: [RFC net-next 1/3] net: marvell: prestera: Add Switchdev driver
for Prestera family ASIC device 98DX325x (AC3x)
Fri, Feb 28, 2020 at 09:06:02AM CET, vadym.kochan@...ision.eu wrote:
>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@...ision.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@...ision.eu>
>> >Signed-off-by: Andrii Savka <andrii.savka@...ision.eu>
>> >Signed-off-by: Oleksandr Mazur <oleksandr.mazur@...ision.eu>
>> >Signed-off-by: Serhiy Boiko <serhiy.boiko@...ision.eu>
>> >Signed-off-by: Serhiy Pshyk <serhiy.pshyk@...ision.eu>
>> >Signed-off-by: Taras Chornyi <taras.chornyi@...ision.eu>
>> >Signed-off-by: Volodymyr Mytnyk <volodymyr.mytnyk@...ision.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.
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 :/
>
>>
>>
>> >+({ \
>> >+ struct mvsw_msg_common_response __re; \
>> >+ (fw_send_req_resp(_sw, _t, _req, &__re)); \
>> >+})
>> >+
>
>Regards,
>Vadym Kochan,
Powered by blists - more mailing lists