Re: [PATCH v7 13/24] wfx: add hif_tx*.c/hif_tx*.h

6 messages, 4 authors, 2021-10-05 · open the first message on its own page

Re: [PATCH v7 13/24] wfx: add hif_tx*.c/hif_tx*.h

From: Kalle Valo <hidden>
Date: 2021-10-05 06:12:58

Pali Rohár [off-list ref] writes:
On Friday 01 October 2021 17:17:52 Jérôme Pouiller wrote:
quoted
On Friday 1 October 2021 11:55:33 CEST Kalle Valo wrote:
quoted
CAUTION: This email originated from outside of the organization.
Do not click links or open attachments unless you recognize the
sender and know the content is safe.


Jerome Pouiller [off-list ref] writes:
quoted
From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
[...]
quoted
--- /dev/null
+++ b/drivers/net/wireless/silabs/wfx/hif_tx_mib.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Implementation of the host-to-chip MIBs of the hardware API.
+ *
+ * Copyright (c) 2017-2020, Silicon Laboratories, Inc.
+ * Copyright (c) 2010, ST-Ericsson
+ * Copyright (C) 2010, ST-Ericsson SA
+ */
+#ifndef WFX_HIF_TX_MIB_H
+#define WFX_HIF_TX_MIB_H
+
+struct wfx_vif;
+struct sk_buff;
+
+int hif_set_output_power(struct wfx_vif *wvif, int val);
+int hif_set_beacon_wakeup_period(struct wfx_vif *wvif,
+                              unsigned int dtim_interval,
+                              unsigned int listen_interval);
+int hif_set_rcpi_rssi_threshold(struct wfx_vif *wvif,
+                             int rssi_thold, int rssi_hyst);
+int hif_get_counters_table(struct wfx_dev *wdev, int vif_id,
+                        struct hif_mib_extended_count_table *arg);
+int hif_set_macaddr(struct wfx_vif *wvif, u8 *mac);
+int hif_set_rx_filter(struct wfx_vif *wvif,
+                   bool filter_bssid, bool fwd_probe_req);
+int hif_set_beacon_filter_table(struct wfx_vif *wvif, int tbl_len,
+                             const struct hif_ie_table_entry *tbl);
+int hif_beacon_filter_control(struct wfx_vif *wvif,
+                           int enable, int beacon_count);
+int hif_set_operational_mode(struct wfx_dev *wdev, enum
hif_op_power_mode mode);
+int hif_set_template_frame(struct wfx_vif *wvif, struct sk_buff *skb,
+                        u8 frame_type, int init_rate);
+int hif_set_mfp(struct wfx_vif *wvif, bool capable, bool required);
+int hif_set_block_ack_policy(struct wfx_vif *wvif,
+                          u8 tx_tid_policy, u8 rx_tid_policy);
+int hif_set_association_mode(struct wfx_vif *wvif, int ampdu_density,
+                          bool greenfield, bool short_preamble);
+int hif_set_tx_rate_retry_policy(struct wfx_vif *wvif,
+                              int policy_index, u8 *rates);
+int hif_keep_alive_period(struct wfx_vif *wvif, int period);
+int hif_set_arp_ipv4_filter(struct wfx_vif *wvif, int idx, __be32 *addr);
+int hif_use_multi_tx_conf(struct wfx_dev *wdev, bool enable);
+int hif_set_uapsd_info(struct wfx_vif *wvif, unsigned long val);
+int hif_erp_use_protection(struct wfx_vif *wvif, bool enable);
+int hif_slot_time(struct wfx_vif *wvif, int val);
+int hif_wep_default_key_id(struct wfx_vif *wvif, int val);
+int hif_rts_threshold(struct wfx_vif *wvif, int val);
"wfx_" prefix missing from quite a few functions.
I didn't know it was mandatory to prefix all the functions with the
same prefix.
I don't know either if this is mandatory or not, for example I do not
have any recollection what Linus and other maintainers think of this. I
just personally think it's good practise to use driver prefix ("wfx_")
in all non-static functions.

Any opinions from others? Greg?
quoted
With the rule of 80-columns, I think I will have to change a bunch of
code :( .
I think that new drivers can use 100 characters per line.
That's my understanding as well.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v7 13/24] wfx: add hif_tx*.c/hif_tx*.h

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: 2021-10-05 06:44:25

On Tue, Oct 05, 2021 at 09:12:27AM +0300, Kalle Valo wrote:
Pali Rohár [off-list ref] writes:
quoted
On Friday 01 October 2021 17:17:52 Jérôme Pouiller wrote:
quoted
On Friday 1 October 2021 11:55:33 CEST Kalle Valo wrote:
quoted
CAUTION: This email originated from outside of the organization.
Do not click links or open attachments unless you recognize the
sender and know the content is safe.


Jerome Pouiller [off-list ref] writes:
quoted
From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
[...]
quoted
--- /dev/null
+++ b/drivers/net/wireless/silabs/wfx/hif_tx_mib.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Implementation of the host-to-chip MIBs of the hardware API.
+ *
+ * Copyright (c) 2017-2020, Silicon Laboratories, Inc.
+ * Copyright (c) 2010, ST-Ericsson
+ * Copyright (C) 2010, ST-Ericsson SA
+ */
+#ifndef WFX_HIF_TX_MIB_H
+#define WFX_HIF_TX_MIB_H
+
+struct wfx_vif;
+struct sk_buff;
+
+int hif_set_output_power(struct wfx_vif *wvif, int val);
+int hif_set_beacon_wakeup_period(struct wfx_vif *wvif,
+                              unsigned int dtim_interval,
+                              unsigned int listen_interval);
+int hif_set_rcpi_rssi_threshold(struct wfx_vif *wvif,
+                             int rssi_thold, int rssi_hyst);
+int hif_get_counters_table(struct wfx_dev *wdev, int vif_id,
+                        struct hif_mib_extended_count_table *arg);
+int hif_set_macaddr(struct wfx_vif *wvif, u8 *mac);
+int hif_set_rx_filter(struct wfx_vif *wvif,
+                   bool filter_bssid, bool fwd_probe_req);
+int hif_set_beacon_filter_table(struct wfx_vif *wvif, int tbl_len,
+                             const struct hif_ie_table_entry *tbl);
+int hif_beacon_filter_control(struct wfx_vif *wvif,
+                           int enable, int beacon_count);
+int hif_set_operational_mode(struct wfx_dev *wdev, enum
hif_op_power_mode mode);
+int hif_set_template_frame(struct wfx_vif *wvif, struct sk_buff *skb,
+                        u8 frame_type, int init_rate);
+int hif_set_mfp(struct wfx_vif *wvif, bool capable, bool required);
+int hif_set_block_ack_policy(struct wfx_vif *wvif,
+                          u8 tx_tid_policy, u8 rx_tid_policy);
+int hif_set_association_mode(struct wfx_vif *wvif, int ampdu_density,
+                          bool greenfield, bool short_preamble);
+int hif_set_tx_rate_retry_policy(struct wfx_vif *wvif,
+                              int policy_index, u8 *rates);
+int hif_keep_alive_period(struct wfx_vif *wvif, int period);
+int hif_set_arp_ipv4_filter(struct wfx_vif *wvif, int idx, __be32 *addr);
+int hif_use_multi_tx_conf(struct wfx_dev *wdev, bool enable);
+int hif_set_uapsd_info(struct wfx_vif *wvif, unsigned long val);
+int hif_erp_use_protection(struct wfx_vif *wvif, bool enable);
+int hif_slot_time(struct wfx_vif *wvif, int val);
+int hif_wep_default_key_id(struct wfx_vif *wvif, int val);
+int hif_rts_threshold(struct wfx_vif *wvif, int val);
"wfx_" prefix missing from quite a few functions.
I didn't know it was mandatory to prefix all the functions with the
same prefix.
I don't know either if this is mandatory or not, for example I do not
have any recollection what Linus and other maintainers think of this. I
just personally think it's good practise to use driver prefix ("wfx_")
in all non-static functions.

Any opinions from others? Greg?
For static functions, pick what you want.

For global functions, like this, use a common prefix that indicates the
driver as you are now playing in the global namespace of a 30 million
line project.
quoted
quoted
With the rule of 80-columns, I think I will have to change a bunch of
code :( .
I think that new drivers can use 100 characters per line.
That's my understanding as well.
Yes, that's fine.

thanks,

greg k-h

Re: [PATCH v7 13/24] wfx: add hif_tx*.c/hif_tx*.h

From: Jérôme Pouiller <jerome.pouiller@silabs.com>
Date: 2021-10-05 08:17:49

On Tuesday 5 October 2021 08:12:27 CEST Kalle Valo wrote:
Pali Rohár [off-list ref] writes:
quoted
On Friday 01 October 2021 17:17:52 Jérôme Pouiller wrote:
quoted
On Friday 1 October 2021 11:55:33 CEST Kalle Valo wrote:
quoted
Jerome Pouiller [off-list ref] writes:
quoted
From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
[...]
quoted
--- /dev/null
+++ b/drivers/net/wireless/silabs/wfx/hif_tx_mib.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Implementation of the host-to-chip MIBs of the hardware API.
+ *
+ * Copyright (c) 2017-2020, Silicon Laboratories, Inc.
+ * Copyright (c) 2010, ST-Ericsson
+ * Copyright (C) 2010, ST-Ericsson SA
+ */
+#ifndef WFX_HIF_TX_MIB_H
+#define WFX_HIF_TX_MIB_H
+
+struct wfx_vif;
+struct sk_buff;
+
+int hif_set_output_power(struct wfx_vif *wvif, int val);
+int hif_set_beacon_wakeup_period(struct wfx_vif *wvif,
+                              unsigned int dtim_interval,
+                              unsigned int listen_interval);
+int hif_set_rcpi_rssi_threshold(struct wfx_vif *wvif,
+                             int rssi_thold, int rssi_hyst);
+int hif_get_counters_table(struct wfx_dev *wdev, int vif_id,
+                        struct hif_mib_extended_count_table *arg);
+int hif_set_macaddr(struct wfx_vif *wvif, u8 *mac);
+int hif_set_rx_filter(struct wfx_vif *wvif,
+                   bool filter_bssid, bool fwd_probe_req);
+int hif_set_beacon_filter_table(struct wfx_vif *wvif, int tbl_len,
+                             const struct hif_ie_table_entry *tbl);
+int hif_beacon_filter_control(struct wfx_vif *wvif,
+                           int enable, int beacon_count);
+int hif_set_operational_mode(struct wfx_dev *wdev, enum
hif_op_power_mode mode);
+int hif_set_template_frame(struct wfx_vif *wvif, struct sk_buff *skb,
+                        u8 frame_type, int init_rate);
+int hif_set_mfp(struct wfx_vif *wvif, bool capable, bool required);
+int hif_set_block_ack_policy(struct wfx_vif *wvif,
+                          u8 tx_tid_policy, u8 rx_tid_policy);
+int hif_set_association_mode(struct wfx_vif *wvif, int ampdu_density,
+                          bool greenfield, bool short_preamble);
+int hif_set_tx_rate_retry_policy(struct wfx_vif *wvif,
+                              int policy_index, u8 *rates);
+int hif_keep_alive_period(struct wfx_vif *wvif, int period);
+int hif_set_arp_ipv4_filter(struct wfx_vif *wvif, int idx, __be32 *addr);
+int hif_use_multi_tx_conf(struct wfx_dev *wdev, bool enable);
+int hif_set_uapsd_info(struct wfx_vif *wvif, unsigned long val);
+int hif_erp_use_protection(struct wfx_vif *wvif, bool enable);
+int hif_slot_time(struct wfx_vif *wvif, int val);
+int hif_wep_default_key_id(struct wfx_vif *wvif, int val);
+int hif_rts_threshold(struct wfx_vif *wvif, int val);
"wfx_" prefix missing from quite a few functions.
I didn't know it was mandatory to prefix all the functions with the
same prefix.
I don't know either if this is mandatory or not, for example I do not
have any recollection what Linus and other maintainers think of this. I
just personally think it's good practise to use driver prefix ("wfx_")
in all non-static functions.
What about structs (especially all the structs from hif_api.*.h)? Do you
think I should also prefix them with wfx_? 

quoted
quoted
With the rule of 80-columns, I think I will have to change a bunch of
code :( .
I think that new drivers can use 100 characters per line.
That's my understanding as well.
:)

-- 
Jérôme Pouiller

Re: [PATCH v7 13/24] wfx: add hif_tx*.c/hif_tx*.h

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: 2021-10-05 08:21:37

On Tue, Oct 05, 2021 at 10:17:32AM +0200, Jérôme Pouiller wrote:
On Tuesday 5 October 2021 08:12:27 CEST Kalle Valo wrote:
quoted
Pali Rohár [off-list ref] writes:
quoted
On Friday 01 October 2021 17:17:52 Jérôme Pouiller wrote:
quoted
On Friday 1 October 2021 11:55:33 CEST Kalle Valo wrote:
quoted
Jerome Pouiller [off-list ref] writes:
quoted
From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
[...]
quoted
--- /dev/null
+++ b/drivers/net/wireless/silabs/wfx/hif_tx_mib.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Implementation of the host-to-chip MIBs of the hardware API.
+ *
+ * Copyright (c) 2017-2020, Silicon Laboratories, Inc.
+ * Copyright (c) 2010, ST-Ericsson
+ * Copyright (C) 2010, ST-Ericsson SA
+ */
+#ifndef WFX_HIF_TX_MIB_H
+#define WFX_HIF_TX_MIB_H
+
+struct wfx_vif;
+struct sk_buff;
+
+int hif_set_output_power(struct wfx_vif *wvif, int val);
+int hif_set_beacon_wakeup_period(struct wfx_vif *wvif,
+                              unsigned int dtim_interval,
+                              unsigned int listen_interval);
+int hif_set_rcpi_rssi_threshold(struct wfx_vif *wvif,
+                             int rssi_thold, int rssi_hyst);
+int hif_get_counters_table(struct wfx_dev *wdev, int vif_id,
+                        struct hif_mib_extended_count_table *arg);
+int hif_set_macaddr(struct wfx_vif *wvif, u8 *mac);
+int hif_set_rx_filter(struct wfx_vif *wvif,
+                   bool filter_bssid, bool fwd_probe_req);
+int hif_set_beacon_filter_table(struct wfx_vif *wvif, int tbl_len,
+                             const struct hif_ie_table_entry *tbl);
+int hif_beacon_filter_control(struct wfx_vif *wvif,
+                           int enable, int beacon_count);
+int hif_set_operational_mode(struct wfx_dev *wdev, enum
hif_op_power_mode mode);
+int hif_set_template_frame(struct wfx_vif *wvif, struct sk_buff *skb,
+                        u8 frame_type, int init_rate);
+int hif_set_mfp(struct wfx_vif *wvif, bool capable, bool required);
+int hif_set_block_ack_policy(struct wfx_vif *wvif,
+                          u8 tx_tid_policy, u8 rx_tid_policy);
+int hif_set_association_mode(struct wfx_vif *wvif, int ampdu_density,
+                          bool greenfield, bool short_preamble);
+int hif_set_tx_rate_retry_policy(struct wfx_vif *wvif,
+                              int policy_index, u8 *rates);
+int hif_keep_alive_period(struct wfx_vif *wvif, int period);
+int hif_set_arp_ipv4_filter(struct wfx_vif *wvif, int idx, __be32 *addr);
+int hif_use_multi_tx_conf(struct wfx_dev *wdev, bool enable);
+int hif_set_uapsd_info(struct wfx_vif *wvif, unsigned long val);
+int hif_erp_use_protection(struct wfx_vif *wvif, bool enable);
+int hif_slot_time(struct wfx_vif *wvif, int val);
+int hif_wep_default_key_id(struct wfx_vif *wvif, int val);
+int hif_rts_threshold(struct wfx_vif *wvif, int val);
"wfx_" prefix missing from quite a few functions.
I didn't know it was mandatory to prefix all the functions with the
same prefix.
I don't know either if this is mandatory or not, for example I do not
have any recollection what Linus and other maintainers think of this. I
just personally think it's good practise to use driver prefix ("wfx_")
in all non-static functions.
What about structs (especially all the structs from hif_api.*.h)? Do you
think I should also prefix them with wfx_? 
Why would they _not_ have wfx_ as a prefix if they only pertain to this
driver?

thanks,

greg k-h

Re: [PATCH v7 13/24] wfx: add hif_tx*.c/hif_tx*.h

From: Jérôme Pouiller <jerome.pouiller@silabs.com>
Date: 2021-10-05 09:18:58

On Tuesday 5 October 2021 10:21:27 CEST Greg Kroah-Hartman wrote:
On Tue, Oct 05, 2021 at 10:17:32AM +0200, Jérôme Pouiller wrote:
quoted
On Tuesday 5 October 2021 08:12:27 CEST Kalle Valo wrote:
quoted
Pali Rohár [off-list ref] writes:
quoted
On Friday 01 October 2021 17:17:52 Jérôme Pouiller wrote:
quoted
On Friday 1 October 2021 11:55:33 CEST Kalle Valo wrote:
quoted
Jerome Pouiller [off-list ref] writes:
quoted
From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
[...]
quoted
--- /dev/null
+++ b/drivers/net/wireless/silabs/wfx/hif_tx_mib.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Implementation of the host-to-chip MIBs of the hardware API.
+ *
+ * Copyright (c) 2017-2020, Silicon Laboratories, Inc.
+ * Copyright (c) 2010, ST-Ericsson
+ * Copyright (C) 2010, ST-Ericsson SA
+ */
+#ifndef WFX_HIF_TX_MIB_H
+#define WFX_HIF_TX_MIB_H
+
+struct wfx_vif;
+struct sk_buff;
+
+int hif_set_output_power(struct wfx_vif *wvif, int val);
+int hif_set_beacon_wakeup_period(struct wfx_vif *wvif,
+                              unsigned int dtim_interval,
+                              unsigned int listen_interval);
+int hif_set_rcpi_rssi_threshold(struct wfx_vif *wvif,
+                             int rssi_thold, int rssi_hyst);
+int hif_get_counters_table(struct wfx_dev *wdev, int vif_id,
+                        struct hif_mib_extended_count_table *arg);
+int hif_set_macaddr(struct wfx_vif *wvif, u8 *mac);
+int hif_set_rx_filter(struct wfx_vif *wvif,
+                   bool filter_bssid, bool fwd_probe_req);
+int hif_set_beacon_filter_table(struct wfx_vif *wvif, int tbl_len,
+                             const struct hif_ie_table_entry *tbl);
+int hif_beacon_filter_control(struct wfx_vif *wvif,
+                           int enable, int beacon_count);
+int hif_set_operational_mode(struct wfx_dev *wdev, enum
hif_op_power_mode mode);
+int hif_set_template_frame(struct wfx_vif *wvif, struct sk_buff *skb,
+                        u8 frame_type, int init_rate);
+int hif_set_mfp(struct wfx_vif *wvif, bool capable, bool required);
+int hif_set_block_ack_policy(struct wfx_vif *wvif,
+                          u8 tx_tid_policy, u8 rx_tid_policy);
+int hif_set_association_mode(struct wfx_vif *wvif, int ampdu_density,
+                          bool greenfield, bool short_preamble);
+int hif_set_tx_rate_retry_policy(struct wfx_vif *wvif,
+                              int policy_index, u8 *rates);
+int hif_keep_alive_period(struct wfx_vif *wvif, int period);
+int hif_set_arp_ipv4_filter(struct wfx_vif *wvif, int idx, __be32 *addr);
+int hif_use_multi_tx_conf(struct wfx_dev *wdev, bool enable);
+int hif_set_uapsd_info(struct wfx_vif *wvif, unsigned long val);
+int hif_erp_use_protection(struct wfx_vif *wvif, bool enable);
+int hif_slot_time(struct wfx_vif *wvif, int val);
+int hif_wep_default_key_id(struct wfx_vif *wvif, int val);
+int hif_rts_threshold(struct wfx_vif *wvif, int val);
"wfx_" prefix missing from quite a few functions.
I didn't know it was mandatory to prefix all the functions with the
same prefix.
I don't know either if this is mandatory or not, for example I do not
have any recollection what Linus and other maintainers think of this. I
just personally think it's good practise to use driver prefix ("wfx_")
in all non-static functions.
What about structs (especially all the structs from hif_api.*.h)? Do you
think I should also prefix them with wfx_?
Why would they _not_ have wfx_ as a prefix if they only pertain to this
driver?
hmmm... to keep identifiers small and readable? I find
"wfx_hif_set_tx_rate_retry_policy" a bit long.

Don't worry, I don't want to debate the rules. I am going to apply them.

-- 
Jérôme Pouiller

Re: [PATCH v7 13/24] wfx: add hif_tx*.c/hif_tx*.h

From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-10-05 14:02:51

On Tue, 05 Oct 2021 09:12:27 +0300 Kalle Valo wrote:
quoted
quoted
I didn't know it was mandatory to prefix all the functions with the
same prefix.  
I don't know either if this is mandatory or not, for example I do not
have any recollection what Linus and other maintainers think of this. I
just personally think it's good practise to use driver prefix ("wfx_")
in all non-static functions.
I'd even say all functions. The prefixes are usually 3 chars, it's no
hassle to add and makes reading the code and looking at stack traces
much more intuitive for people who are not intimately familiar with 
the code.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help