From: Ping-Ke Shih <pkshih@realtek.com>
To: Dmitry Antipov <dmantipov@yandex.ru>
Cc: Tom Rix <trix@redhat.com>, Kalle Valo <kvalo@kernel.org>,
"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: RE: [PATCH] wifi: rtw89: fix clang-specific -Wvoid-pointer-to-enum-cast
Date: Fri, 20 Oct 2023 03:15:08 +0000 [thread overview]
Message-ID: <661e7dffafdb403abc2c4cab23c7d1ed@realtek.com> (raw)
In-Reply-To: <20231019093805.66324-1-dmantipov@yandex.ru>
> -----Original Message-----
> From: Dmitry Antipov <dmantipov@yandex.ru>
> Sent: Thursday, October 19, 2023 5:38 PM
> To: Ping-Ke Shih <pkshih@realtek.com>
> Cc: Tom Rix <trix@redhat.com>; Kalle Valo <kvalo@kernel.org>; linux-wireless@vger.kernel.org; Dmitry
> Antipov <dmantipov@yandex.ru>
> Subject: [PATCH] wifi: rtw89: fix clang-specific -Wvoid-pointer-to-enum-cast
Please point out "fw_element" in subject, so it would be clear to know what
we are dealing with.
>
> When compiling with clang-18, I've noticed the following:
>
> drivers/net/wireless/realtek/rtw89/fw.c:389:28: warning: cast to smaller
> integer type 'enum rtw89_fw_type' from 'const void *' [-Wvoid-pointer-to-enum-cast]
> 389 | enum rtw89_fw_type type = (enum rtw89_fw_type)data;
> | ^~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/wireless/realtek/rtw89/fw.c:569:13: warning: cast to smaller
> integer type 'enum rtw89_rf_path' from 'const void *' [-Wvoid-pointer-to-enum-cast]
> 569 | rf_path = (enum rtw89_rf_path)data;
> | ^~~~~~~~~~~~~~~~~~~~~~~~
>
> So avoid brutal everything-to-const-void-and-back casts, introduce
> 'union rtw89_fw_element_data' to pass parameters to element handler
> callbacks, and adjust all of the related bits accordingly. Compile
> tested only.
>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> drivers/net/wireless/realtek/rtw89/fw.c | 81 +++++++++++++++----------
> 1 file changed, 50 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw89/fw.c b/drivers/net/wireless/realtek/rtw89/fw.c
> index 7cfcf536d6fe..13a98c44d304 100644
> --- a/drivers/net/wireless/realtek/rtw89/fw.c
> +++ b/drivers/net/wireless/realtek/rtw89/fw.c
> @@ -13,6 +13,20 @@
> #include "reg.h"
> #include "util.h"
>
> +union rtw89_fw_element_data {
"fw_element_data" could be confusing, because it looks like data from firmware file.
Prefer "fw_element_arg" or "fw_element_var", because the certain handler can handle
various cases of fw_element_id.
Then, third argument of handler becomes 'const union rtw89_fw_element_arg arg'.
> + size_t offset;
> + enum rtw89_rf_path path;
> + enum rtw89_fw_type type;
'rf_path' and 'fw_type' would be clearer.
> +};
> +
> +struct rtw89_fw_element_handler {
> + int (*fn)(struct rtw89_dev *rtwdev,
> + const struct rtw89_fw_element_hdr *elm,
> + const union rtw89_fw_element_data data);
> + const union rtw89_fw_element_data data;
> + const char *name;
> +};
> +
> static void rtw89_fw_c2h_cmd_handle(struct rtw89_dev *rtwdev,
> struct sk_buff *skb);
> static int rtw89_h2c_tx_and_wait(struct rtw89_dev *rtwdev, struct sk_buff *skb,
> @@ -384,9 +398,9 @@ int __rtw89_fw_recognize(struct rtw89_dev *rtwdev, enum rtw89_fw_type type,
> static
> int __rtw89_fw_recognize_from_elm(struct rtw89_dev *rtwdev,
> const struct rtw89_fw_element_hdr *elm,
> - const void *data)
> + const union rtw89_fw_element_data data)
> {
> - enum rtw89_fw_type type = (enum rtw89_fw_type)data;
> + enum rtw89_fw_type type = data.type;
> struct rtw89_fw_suit *fw_suit;
>
> fw_suit = rtw89_fw_suit_get(rtwdev, type);
> @@ -542,7 +556,7 @@ int rtw89_fw_recognize(struct rtw89_dev *rtwdev)
> static
> int rtw89_build_phy_tbl_from_elm(struct rtw89_dev *rtwdev,
> const struct rtw89_fw_element_hdr *elm,
> - const void *data)
> + const union rtw89_fw_element_data data)
> {
> struct rtw89_fw_elm_info *elm_info = &rtwdev->fw.elm_info;
> struct rtw89_phy_table *tbl;
> @@ -566,7 +580,7 @@ int rtw89_build_phy_tbl_from_elm(struct rtw89_dev *rtwdev,
> case RTW89_FW_ELEMENT_ID_RADIO_B:
> case RTW89_FW_ELEMENT_ID_RADIO_C:
> case RTW89_FW_ELEMENT_ID_RADIO_D:
> - rf_path = (enum rtw89_rf_path)data;
> + rf_path = data.path;
> idx = elm->u.reg2.idx;
>
> elm_info->rf_radio[idx] = tbl;
> @@ -604,10 +618,10 @@ int rtw89_build_phy_tbl_from_elm(struct rtw89_dev *rtwdev,
> static
> int rtw89_fw_recognize_txpwr_from_elm(struct rtw89_dev *rtwdev,
> const struct rtw89_fw_element_hdr *elm,
> - const void *data)
> + union rtw89_fw_element_data data)
const union rtw89_fw_element_data data
miss 'const' here.
> {
> const struct __rtw89_fw_txpwr_element *txpwr_elm = &elm->u.txpwr;
> - const unsigned long offset = (const unsigned long)data;
> + const unsigned long offset = data.offset;
> struct rtw89_efuse *efuse = &rtwdev->efuse;
> struct rtw89_txpwr_conf *conf;
>
> @@ -644,64 +658,69 @@ int rtw89_fw_recognize_txpwr_from_elm(struct rtw89_dev *rtwdev,
> return 0;
> }
>
> -struct rtw89_fw_element_handler {
> - int (*fn)(struct rtw89_dev *rtwdev,
> - const struct rtw89_fw_element_hdr *elm, const void *data);
> - const void *data;
> - const char *name;
> -};
> -
> static const struct rtw89_fw_element_handler __fw_element_handlers[] = {
> [RTW89_FW_ELEMENT_ID_BBMCU0] = {__rtw89_fw_recognize_from_elm,
> - (const void *)RTW89_FW_BBMCU0, NULL},
> + { .type = RTW89_FW_BBMCU0 }, NULL},
> [RTW89_FW_ELEMENT_ID_BBMCU1] = {__rtw89_fw_recognize_from_elm,
> - (const void *)RTW89_FW_BBMCU1, NULL},
> - [RTW89_FW_ELEMENT_ID_BB_REG] = {rtw89_build_phy_tbl_from_elm, NULL, "BB"},
> - [RTW89_FW_ELEMENT_ID_BB_GAIN] = {rtw89_build_phy_tbl_from_elm, NULL, NULL},
> + { .type = RTW89_FW_BBMCU1 }, NULL},
> + [RTW89_FW_ELEMENT_ID_BB_REG] = {rtw89_build_phy_tbl_from_elm,
> + { 0 }, "BB"},
Can we just '{}' instead? And, straighten it to single one line as before, like
[RTW89_FW_ELEMENT_ID_BB_REG] = {rtw89_build_phy_tbl_from_elm, {], "BB"},
> + [RTW89_FW_ELEMENT_ID_BB_GAIN] = {rtw89_build_phy_tbl_from_elm,
> + { 0 }, NULL},
> [RTW89_FW_ELEMENT_ID_RADIO_A] = {rtw89_build_phy_tbl_from_elm,
> - (const void *)RF_PATH_A, "radio A"},
> + { .path = RF_PATH_A }, "radio A"},
> [RTW89_FW_ELEMENT_ID_RADIO_B] = {rtw89_build_phy_tbl_from_elm,
> - (const void *)RF_PATH_B, NULL},
> + { .path = RF_PATH_B }, NULL},
> [RTW89_FW_ELEMENT_ID_RADIO_C] = {rtw89_build_phy_tbl_from_elm,
> - (const void *)RF_PATH_C, NULL},
> + { .path = RF_PATH_C }, NULL},
> [RTW89_FW_ELEMENT_ID_RADIO_D] = {rtw89_build_phy_tbl_from_elm,
> - (const void *)RF_PATH_D, NULL},
> - [RTW89_FW_ELEMENT_ID_RF_NCTL] = {rtw89_build_phy_tbl_from_elm, NULL, "NCTL"},
> + { .path = RF_PATH_D }, NULL},
> + [RTW89_FW_ELEMENT_ID_RF_NCTL] = {rtw89_build_phy_tbl_from_elm,
> + { 0 }, "NCTL"},
> [RTW89_FW_ELEMENT_ID_TXPWR_BYRATE] = {
> rtw89_fw_recognize_txpwr_from_elm,
> - (const void *)offsetof(struct rtw89_rfe_data, byrate.conf), "TXPWR",
> + { .offset = offsetof(struct rtw89_rfe_data, byrate.conf) },
> + "TXPWR",
Prefer keeping original single-one-line style.
> },
> [RTW89_FW_ELEMENT_ID_TXPWR_LMT_2GHZ] = {
> rtw89_fw_recognize_txpwr_from_elm,
> - (const void *)offsetof(struct rtw89_rfe_data, lmt_2ghz.conf), NULL,
> + { .offset = offsetof(struct rtw89_rfe_data, lmt_2ghz.conf) },
> + NULL,
> },
> [RTW89_FW_ELEMENT_ID_TXPWR_LMT_5GHZ] = {
> rtw89_fw_recognize_txpwr_from_elm,
> - (const void *)offsetof(struct rtw89_rfe_data, lmt_5ghz.conf), NULL,
> + { .offset = offsetof(struct rtw89_rfe_data, lmt_5ghz.conf) },
> + NULL,
> },
> [RTW89_FW_ELEMENT_ID_TXPWR_LMT_6GHZ] = {
> rtw89_fw_recognize_txpwr_from_elm,
> - (const void *)offsetof(struct rtw89_rfe_data, lmt_6ghz.conf), NULL,
> + { .offset = offsetof(struct rtw89_rfe_data, lmt_6ghz.conf) },
> + NULL,
> },
> [RTW89_FW_ELEMENT_ID_TXPWR_LMT_RU_2GHZ] = {
> rtw89_fw_recognize_txpwr_from_elm,
> - (const void *)offsetof(struct rtw89_rfe_data, lmt_ru_2ghz.conf), NULL,
> + { .offset = offsetof(struct rtw89_rfe_data, lmt_ru_2ghz.conf) },
> + NULL,
> },
> [RTW89_FW_ELEMENT_ID_TXPWR_LMT_RU_5GHZ] = {
> rtw89_fw_recognize_txpwr_from_elm,
> - (const void *)offsetof(struct rtw89_rfe_data, lmt_ru_5ghz.conf), NULL,
> + { .offset = offsetof(struct rtw89_rfe_data, lmt_ru_5ghz.conf) },
> + NULL,
> },
> [RTW89_FW_ELEMENT_ID_TXPWR_LMT_RU_6GHZ] = {
> rtw89_fw_recognize_txpwr_from_elm,
> - (const void *)offsetof(struct rtw89_rfe_data, lmt_ru_6ghz.conf), NULL,
> + { .offset = offsetof(struct rtw89_rfe_data, lmt_ru_6ghz.conf) },
> + NULL,
> },
> [RTW89_FW_ELEMENT_ID_TX_SHAPE_LMT] = {
> rtw89_fw_recognize_txpwr_from_elm,
> - (const void *)offsetof(struct rtw89_rfe_data, tx_shape_lmt.conf), NULL,
> + { .offset = offsetof(struct rtw89_rfe_data, tx_shape_lmt.conf) },
> + NULL,
> },
> [RTW89_FW_ELEMENT_ID_TX_SHAPE_LMT_RU] = {
> rtw89_fw_recognize_txpwr_from_elm,
> - (const void *)offsetof(struct rtw89_rfe_data, tx_shape_lmt_ru.conf), NULL,
> + { .offset = offsetof(struct rtw89_rfe_data, tx_shape_lmt_ru.conf) },
> + NULL,
> },
> };
>
> --
> 2.41.0
next prev parent reply other threads:[~2023-10-20 3:15 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-19 9:38 [PATCH] wifi: rtw89: fix clang-specific -Wvoid-pointer-to-enum-cast Dmitry Antipov
2023-10-20 3:15 ` Ping-Ke Shih [this message]
2023-10-20 4:09 ` [PATCH] [v2] wifi: rtw89: cleanup firmware elements parsing Dmitry Antipov
2023-10-20 5:54 ` Ping-Ke Shih
2023-10-25 9:09 ` Kalle Valo
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=661e7dffafdb403abc2c4cab23c7d1ed@realtek.com \
--to=pkshih@realtek.com \
--cc=dmantipov@yandex.ru \
--cc=kvalo@kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=trix@redhat.com \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.