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] [day] [month] [year] [list]
Message-ID: <001101dae31f$d5ef44d0$81cdce70$@samsung.com>
Date: Wed, 31 Jul 2024 17:01:23 +0900
From: "Manjae Cho" <manjae.cho@...sung.com>
To: "'Greg KH'" <gregkh@...uxfoundation.org>
Cc: <linux-staging@...ts.linux.dev>, <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] Improve MAR register definition and usage for rtl8723

> On Wed, Jul 31, 2024 at 03:55:05PM +0900, Manjae Cho wrote:
> > > On Wed, Jul 31, 2024 at 12:50:54AM +0900, Manjae Cho wrote:
> > > > This patch improves the usage of the MAR register by updating the
> > > > relevant macro definitions and ensuring consistent usage across
> > > > the codebase.
> > > >
> > > > Signed-off-by: Manjae Cho <manjae.cho@...sung.com>
> > > >
> > > > ---
> > > >  drivers/staging/rtl8723bs/hal/sdio_halinit.c    | 4 ++--
> > > >  drivers/staging/rtl8723bs/include/hal_com_reg.h | 3 +++
> > > >  2 files changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/rtl8723bs/hal/sdio_halinit.c
> > > > b/drivers/staging/rtl8723bs/hal/sdio_halinit.c
> > > > index c9cd6578f7f8..9493562c1619 100644
> > > > --- a/drivers/staging/rtl8723bs/hal/sdio_halinit.c
> > > > +++ b/drivers/staging/rtl8723bs/hal/sdio_halinit.c
> > > > @@ -380,8 +380,8 @@ static void _InitWMACSetting(struct adapter
> > > *padapter)
> > > >  	rtw_write32(padapter, REG_RCR, pHalData->ReceiveConfig);
> > > >
> > > >  	/*  Accept all multicast address */
> > > > -	rtw_write32(padapter, REG_MAR, 0xFFFFFFFF);
> > > > -	rtw_write32(padapter, REG_MAR + 4, 0xFFFFFFFF);
> > > > +	rtw_write32(padapter, MAR0, 0xFFFFFFFF);
> > > > +	rtw_write32(padapter, MAR4, 0xFFFFFFFF);
> > > >
> > > >  	/*  Accept all data frames */
> > > >  	value16 = 0xFFFF;
> > > > diff --git a/drivers/staging/rtl8723bs/include/hal_com_reg.h
> > > > b/drivers/staging/rtl8723bs/include/hal_com_reg.h
> > > > index 9a02ae69d7a4..baf326d53a46 100644
> > > > --- a/drivers/staging/rtl8723bs/include/hal_com_reg.h
> > > > +++ b/drivers/staging/rtl8723bs/include/hal_com_reg.h
> > > > @@ -151,6 +151,9 @@
> > > >  #define REG_BSSID
0x0618
> > > >  #define REG_MAR
> > 0x0620
> > > >
> > > > +#define MAR0						REG_MAR
> > > 	/* Multicast Address Register, Offset 0x0620-0x0623 */
> > >
> > > Why redefine this value again?  What is wrong with using it as
> "REG_MAR"?
> > > Is this fixing anything or making anything more consistent somewhere?
> > > It's only used in one place that I can see.
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > Dear Greg,
> >
> > Thank you for your feedback. I appreciate your point about the current
> > usage of REG_MAR. While it's true that it's only used in one place
> > currently, I believe there's value in making this change for the
> following reasons:
> >
> >  - Consistency: Other similar registers in the codebase use this
pattern.
> > For example, we have IDR0 and IDR4 for MACID registers. Adding MAR0
> > and MAR4 brings consistency to our register naming convention.
> >
> >  - Clarity: The +4 offset in "REG_MAR + 4" isn't immediately clear
> > without context. MAR4 makes it explicit that we're dealing with the
> > next 4 bytes of the Multicast Address Register.
> >
> >  - If we need to use these registers elsewhere in the future, having
> > clear, specific names will make the code more readable.
> 
> You aren't going to use them elsewhere, worry about this then, not now.
> 
> > However, I understand if you feel this change doesn't provide enough
> > benefit to justify inclusion. If you prefer, I could modify the patch
> > to keep the REG_MAR usage but add comments for clarity:
> >
> >     /* Multicast Address Register */
> >     rtw_write32(padapter, REG_MAR, 0xFFFFFFFF);     /* Offset 0x0620-
> 0x0623
> > */
> >     rtw_write32(padapter, REG_MAR + 4, 0xFFFFFFFF); /* Offset
> > 0x0624-0x0627 */
> 
> That seems a lot more sane and simpler.
> 
> thanks,
> 
> greg k-h

Dear Greg,

Thank you for your guidance. I appreciate your perspective on keeping the
code simple and addressing current needs rather than potential future uses.

I agree that adding comments for clarity is a more straightforward approach.
I'll revise the patch accordingly

I'll submit this updated patch shortly. Thank you again for your time and
feedback.

Thank you
Best Regards

Manjae Cho




Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ