linux-kernel - Re: staging: rtl8723bs: change remaining printk to proper api

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e12bc7eb-07ed-4bec-84f7-5b178ba802c2@stanley.mountain>
Date: Fri, 25 Oct 2024 10:19:33 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Rodrigo Gobbi <rodrigo.gobbi.7@...il.com>
Cc: gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
	linux-staging@...ts.linux.dev, philipp.g.hortmann@...il.com,
	~lkcamp/patches@...ts.sr.ht
Subject: Re: staging: rtl8723bs: change remaining printk to proper api

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