drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 6 +++--- drivers/staging/rtl8723bs/hal/hal_com.c | 7 ++++--- drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 12 ++++++++---- drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 2 +- 4 files changed, 16 insertions(+), 11 deletions(-)
As part of TODO file for future work, use dyn debug api for
remaining printk statements.
Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>
---
I didn't use the netdev_dbg() over drivers/staging/rtl8723bs/hal/hal_com.c
because I noticed now that rtw_dump_raw_rssi_info() and the hal file is a
little broken with -DDBG_RX_SIGNAL_DISPLAY_RAW_DATA, maybe we can
fix that in a next patch.
Suggesting to keep the pr_debug() in the hal file for now.
Tks and regards.
---
Changelog
v3 additional changes to netdev_dbg() at rtl8723b_hal_init.c file
v2 https://lore.kernel.org/linux-staging/20241022031825.309568-1-rodrigo.gobbi.7@gmail.com/T/#m4c2796a796d7e5b456975365147c51d7977e9e81
v1 https://lore.kernel.org/lkml/2024101608-daycare-exterior-31fd@gregkh/T/#m1b2b4fdb8a5eec605983c12ca211d394b66cc79f
---
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 6 +++---
drivers/staging/rtl8723bs/hal/hal_com.c | 7 ++++---
drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 12 ++++++++----
drivers/staging/rtl8723bs/os_dep/sdio_intf.c | 2 +-
4 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index bbdd5fce28a1..ac5066db4e78 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -1870,10 +1870,10 @@ unsigned int OnAction_sa_query(struct adapter *padapter, union recv_frame *precv
if (0) {
int pp;
- printk("pattrib->pktlen = %d =>", pattrib->pkt_len);
+ netdev_dbg(padapter->pnetdev, "pattrib->pktlen = %d =>", pattrib->pkt_len);
for (pp = 0; pp < pattrib->pkt_len; pp++)
- printk(" %02x ", pframe[pp]);
- printk("\n");
+ pr_cont(" %02x ", pframe[pp]);
+ pr_cont("\n");
}
return _SUCCESS;
diff --git a/drivers/staging/rtl8723bs/hal/hal_com.c b/drivers/staging/rtl8723bs/hal/hal_com.c
index faa6ed2b320d..5994e574ae99 100644
--- a/drivers/staging/rtl8723bs/hal/hal_com.c
+++ b/drivers/staging/rtl8723bs/hal/hal_com.c
@@ -909,10 +909,11 @@ void rtw_dump_raw_rssi_info(struct adapter *padapter)
for (rf_path = 0; rf_path < pHalData->NumTotalRFPath; rf_path++) {
if (!isCCKrate) {
- printk(", rx_ofdm_pwr:%d(dBm), rx_ofdm_snr:%d(dB)\n",
- psample_pkt_rssi->ofdm_pwr[rf_path], psample_pkt_rssi->ofdm_snr[rf_path]);
+ pr_debug(", rx_ofdm_pwr:%d(dBm), rx_ofdm_snr:%d(dB)\n",
+ psample_pkt_rssi->ofdm_pwr[rf_path],
+ psample_pkt_rssi->ofdm_snr[rf_path]);
} else {
- printk("\n");
+ pr_debug("\n");
}
}
}
diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
index 46962b003d17..a8ffa219fb2a 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
@@ -60,7 +60,8 @@ static int _BlockWrite(struct adapter *padapter, void *buffer, u32 buffSize)
for (i = 0; i < blockCount_p1; i++) {
ret = rtw_write32(padapter, (FW_8723B_START_ADDRESS + i * blockSize_p1), *((u32 *)(bufferPtr + i * blockSize_p1)));
if (ret == _FAIL) {
- printk("====>%s %d i:%d\n", __func__, __LINE__, i);
+ netdev_dbg(padapter->pnetdev, "write failed at %s %d, block:%d\n",
+ __func__, __LINE__, i);
goto exit;
}
}
@@ -83,7 +84,8 @@ static int _BlockWrite(struct adapter *padapter, void *buffer, u32 buffSize)
ret = rtw_write8(padapter, (FW_8723B_START_ADDRESS + offset + i), *(bufferPtr + offset + i));
if (ret == _FAIL) {
- printk("====>%s %d i:%d\n", __func__, __LINE__, i);
+ netdev_dbg(padapter->pnetdev, "write failed at %s %d, block:%d\n",
+ __func__, __LINE__, i);
goto exit;
}
}
@@ -125,7 +127,8 @@ static int _WriteFW(struct adapter *padapter, void *buffer, u32 size)
ret = _PageWrite(padapter, page, bufferPtr+offset, MAX_DLFW_PAGE_SIZE);
if (ret == _FAIL) {
- printk("====>%s %d\n", __func__, __LINE__);
+ netdev_dbg(padapter->pnetdev, "page write failed at %s %d\n",
+ __func__, __LINE__);
goto exit;
}
}
@@ -136,7 +139,8 @@ static int _WriteFW(struct adapter *padapter, void *buffer, u32 size)
ret = _PageWrite(padapter, page, bufferPtr+offset, remainSize);
if (ret == _FAIL) {
- printk("====>%s %d\n", __func__, __LINE__);
+ netdev_dbg(padapter->pnetdev, "remaining page write failed at %s %d\n",
+ __func__, __LINE__);
goto exit;
}
}
diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
index d18fde4e5d6c..b845089e8d8e 100644
--- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
+++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
@@ -72,7 +72,7 @@ static int sdio_alloc_irq(struct dvobj_priv *dvobj)
err = sdio_claim_irq(func, &sd_sync_int_hdl);
if (err) {
dvobj->drv_dbg.dbg_sdio_alloc_irq_error_cnt++;
- printk(KERN_CRIT "%s: sdio_claim_irq FAIL(%d)!\n", __func__, err);
+ pr_crit("%s: sdio_claim_irq FAIL(%d)!\n", __func__, err);
} else {
dvobj->drv_dbg.dbg_sdio_alloc_irq_cnt++;
dvobj->irq_alloc = 1;
--
2.34.1On Wed, Oct 23, 2024 at 08:11:55PM -0300, Rodrigo Gobbi wrote:
> As part of TODO file for future work, use dyn debug api for
> remaining printk statements.
>
> Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>
> ---
> I didn't use the netdev_dbg() over drivers/staging/rtl8723bs/hal/hal_com.c
> because I noticed now that rtw_dump_raw_rssi_info() and the hal file is a
> little broken with -DDBG_RX_SIGNAL_DISPLAY_RAW_DATA, maybe we can
> fix that in a next patch.
How is it broken?
> diff --git a/drivers/staging/rtl8723bs/hal/hal_com.c b/drivers/staging/rtl8723bs/hal/hal_com.c
> index faa6ed2b320d..5994e574ae99 100644
> --- a/drivers/staging/rtl8723bs/hal/hal_com.c
> +++ b/drivers/staging/rtl8723bs/hal/hal_com.c
> @@ -909,10 +909,11 @@ void rtw_dump_raw_rssi_info(struct adapter *padapter)
>
> for (rf_path = 0; rf_path < pHalData->NumTotalRFPath; rf_path++) {
> if (!isCCKrate) {
> - printk(", rx_ofdm_pwr:%d(dBm), rx_ofdm_snr:%d(dB)\n",
> - psample_pkt_rssi->ofdm_pwr[rf_path], psample_pkt_rssi->ofdm_snr[rf_path]);
> + pr_debug(", rx_ofdm_pwr:%d(dBm), rx_ofdm_snr:%d(dB)\n",
> + psample_pkt_rssi->ofdm_pwr[rf_path],
> + psample_pkt_rssi->ofdm_snr[rf_path]);
> } else {
> - printk("\n");
> + pr_debug("\n");
Just delete this line.
> }
> }
> }
> diff --git a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> index d18fde4e5d6c..b845089e8d8e 100644
> --- a/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> +++ b/drivers/staging/rtl8723bs/os_dep/sdio_intf.c
> @@ -72,7 +72,7 @@ static int sdio_alloc_irq(struct dvobj_priv *dvobj)
> err = sdio_claim_irq(func, &sd_sync_int_hdl);
> if (err) {
> dvobj->drv_dbg.dbg_sdio_alloc_irq_error_cnt++;
> - printk(KERN_CRIT "%s: sdio_claim_irq FAIL(%d)!\n", __func__, err);
> + pr_crit("%s: sdio_claim_irq FAIL(%d)!\n", __func__, err);
^^^^^^^?
regards,
dan carpenter
First, tks for the answer, Dan.
> little broken with -DDBG_RX_SIGNAL_DISPLAY_RAW_DATA, maybe we can
> fix that in a next patch.
> How is it broken?
make -j<> ./drivers/staging/rtl8723bs/r8723bs.ko EXTRA_CFLAGS="-DDBG_RX_SIGNAL_DISPLAY_RAW_DATA"
drivers/staging/rtl8723bs/hal/hal_com.c: In function ‘rtw_store_phy_info’:
drivers/staging/rtl8723bs/hal/hal_com.c:927:43: error: ‘PODM_PHY_INFO_T’ undeclared (first use in this function)
927 | struct odm_phy_info *pPhyInfo = (PODM_PHY_INFO_T)(&pattrib->phy_info);
| ^~~~~~~~~~~~~~~
drivers/staging/rtl8723bs/hal/hal_com.c:927:43: note: each undeclared identifier is reported only once for each function it appears in
drivers/staging/rtl8723bs/hal/hal_com.c:940:73: error: ‘struct odm_phy_info’ has no member named ‘RxPwr’; did you mean ‘rx_pwr’?
940 | psample_pkt_rssi->ofdm_pwr[rf_path] = pPhyInfo->RxPwr[rf_path];
| ^~~~~
| rx_pwr
drivers/staging/rtl8723bs/hal/hal_com.c:941:71: error: ‘struct odm_phy_info’ has no member named ‘RxSNR’
941 | psample_pkt_rssi->ofdm_snr[rf_path] = pPhyInfo->RxSNR[rf_path];
Actually it's not exaclty in the same line of pr_debug/netdev_dbg replacement. Do you think
we can do the replacement at:
> + pr_debug(", rx_ofdm_pwr:%d(dBm), rx_ofdm_snr:%d(dB)\n",...
regardless of this build errors? I mean, fixing it here in this patch
would not be related to the purpose of this simple patch.
> Just delete this line.
Ok.
> - printk(KERN_CRIT "%s: sdio_claim_irq FAIL(%d)!\n", __func__, err);
> + pr_crit("%s: sdio_claim_irq FAIL(%d)!\n", __func__, err);
> ^^^^^^^?
Originally, I didn't replace this because at dvobj of sdio_alloc_irq(struct dvobj_priv *dvobj)
there are two adapter fields:
struct dvobj_priv {
struct adapter *if1; /* PRIMARY_ADAPTER */
...
struct adapter *padapters;
...
}
Checking the "counter part" function of sdio_alloc_irq(), the sdio_free_irq() (which is not used),
the if1 (primary) is used for debug purpose:
netdev_err(dvobj->if1->pnetdev...
And checking the initialization path, if1 should be ok at this point.
But since I can't test this, do you suggest to replace anyway?
Tks and regards.
On Thu, Oct 24, 2024 at 07:55:12PM -0300, Rodrigo Gobbi wrote:
> First, tks for the answer, Dan.
>
> > little broken with -DDBG_RX_SIGNAL_DISPLAY_RAW_DATA, maybe we can
> > fix that in a next patch.
> > How is it broken?
>
> make -j<> ./drivers/staging/rtl8723bs/r8723bs.ko EXTRA_CFLAGS="-DDBG_RX_SIGNAL_DISPLAY_RAW_DATA"
Why are you pasing -DDBG_RX_SIGNAL_DISPLAY_RAW_DATA? Is there some document
somewhere which says to do that? Normally we would just say "Nothing ever
enables DDBG_RX_SIGNAL_DISPLAY_RAW_DATA so just delete that code" and delete
it. But if there is some external document which says to enable it, and it's
useful stuff then maybe we should keep it?
>
> drivers/staging/rtl8723bs/hal/hal_com.c: In function ‘rtw_store_phy_info’:
> drivers/staging/rtl8723bs/hal/hal_com.c:927:43: error: ‘PODM_PHY_INFO_T’ undeclared (first use in this function)
> 927 | struct odm_phy_info *pPhyInfo = (PODM_PHY_INFO_T)(&pattrib->phy_info);
> | ^~~~~~~~~~~~~~~
> drivers/staging/rtl8723bs/hal/hal_com.c:927:43: note: each undeclared identifier is reported only once for each function it appears in
> drivers/staging/rtl8723bs/hal/hal_com.c:940:73: error: ‘struct odm_phy_info’ has no member named ‘RxPwr’; did you mean ‘rx_pwr’?
> 940 | psample_pkt_rssi->ofdm_pwr[rf_path] = pPhyInfo->RxPwr[rf_path];
> | ^~~~~
> | rx_pwr
> drivers/staging/rtl8723bs/hal/hal_com.c:941:71: error: ‘struct odm_phy_info’ has no member named ‘RxSNR’
> 941 | psample_pkt_rssi->ofdm_snr[rf_path] = pPhyInfo->RxSNR[rf_path];
>
> Actually it's not exaclty in the same line of pr_debug/netdev_dbg replacement. Do you think
> we can do the replacement at:
>
This has nothing to do with pr_debug/netdev_dbg replacement as you say.
On the other hand, how useful can DDBG_RX_SIGNAL_DISPLAY_RAW_DATA be if it
doesn't compile? If you sent a patch to delete it, we will apply that patch.
> > + pr_debug(", rx_ofdm_pwr:%d(dBm), rx_ofdm_snr:%d(dB)\n",...
>
> regardless of this build errors? I mean, fixing it here in this patch
> would not be related to the purpose of this simple patch.
>
> > Just delete this line.
> Ok.
>
> > - printk(KERN_CRIT "%s: sdio_claim_irq FAIL(%d)!\n", __func__, err);
> > + pr_crit("%s: sdio_claim_irq FAIL(%d)!\n", __func__, err);
> > ^^^^^^^?
>
> Originally, I didn't replace this because at dvobj of sdio_alloc_irq(struct dvobj_priv *dvobj)
> there are two adapter fields:
>
> struct dvobj_priv {
> struct adapter *if1; /* PRIMARY_ADAPTER */
> ...
> struct adapter *padapters;
> ...
> }
>
> Checking the "counter part" function of sdio_alloc_irq(), the sdio_free_irq() (which is not used),
> the if1 (primary) is used for debug purpose:
If it's not used, just delete it?
>
> netdev_err(dvobj->if1->pnetdev...
>
> And checking the initialization path, if1 should be ok at this point.
> But since I can't test this, do you suggest to replace anyway?
Most drivers/staging patches are not tested before sending. The printk
conversions are a leading source of bugs. The common bug with these conversions
looks like this:
if (!dev)
netdev_err(dev, "no device\n");
Anyway, it's fine to send untested patches so long as you've looked at the
initialization path and it's okay as you said.
regards,
dan carpenter
> Why are you pasing -DDBG_RX_SIGNAL_DISPLAY_RAW_DATA? > ... But if there is some external document which says to enable it, and it's > useful stuff then maybe we should keep it? I've enabled this in an arbitrary way just to check the rtw_dump_raw_rssi_info() because I was doing the debug replacement within the isolated code. Looking again, I didn't find any documentation about this and the history of some files, the #ifdef macro is very old...from 7years ago. We can remove this for sure. > If it's not used, just delete it? Ok, I'll do it in a next patch too. > ...as you've looked at the > initialization path and it's okay as you said. Ok, I'll send a v4. Tks again and regards!
© 2016 - 2024 Red Hat, Inc.