Re: [Outreachy kernel] [PATCH 1/4] Staging: rtl8192u: ieee80211: Use netdev_dbg() for debug messages.

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 30 Mar 2020 09:01:19 -0400
Sam Muhammed <jane.pnx9@gmail.com> wrote:

> On Mon, 2020-03-30 at 09:59 +0200, Stefano Brivio wrote:
> > On Thu, 26 Mar 2020 11:02:35 -0400
> > Sam Muhammed <jane.pnx9@gmail.com> wrote:
> >   
> > > +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> > > @@ -436,7 +436,7 @@ int ieee80211_wx_set_encode(struct ieee80211_device *ieee,
> > >  	if (ieee->reset_on_keychange &&
> > >  	    ieee->iw_mode != IW_MODE_INFRA &&
> > >  	    ieee->reset_port && ieee->reset_port(dev)) {
> > > -		printk(KERN_DEBUG "%s: reset_port failed\n", dev->name);
> > > +		netdev_dbg(ieee->dev, "reset_port failed\n");  
> > 
> > I guess you should stick to 'dev' here -- it's the same as ieee->dev,
> > but reset_port() is invoked on 'dev', so it would be less confusing.
> 
> Thank You for pointing this out! this got me hesitant but i thought i
> could be more explicit?

Hmm, I see. Look, these would be my points, in order of priority:

1. ieee->reset_port(dev), port reset failed on dev, nothing else

2. it's nice to have a 'dev' pointer representing the device instead of
   having to spell out ieee->dev every time in this function,
   especially as the container is called 'ieee', which has a very weak
   meaning

2. having a 'dev' variable is useless, ieee->dev leaves no room for
   interpretation and wondering (when was 'dev' assigned? where does it
   come from?)

So if I understand correctly you are referring to the second "2."
point. About those two points, I really have no preference. I also
couldn't decide myself. I guess I'd instinctively go for "ieee->dev"
everywhere.

However, I guess point 1. is arguably the most important here.

> Now since this change is already been made, i guess i should make this
> a new patch?

Ouch, I missed that. Yes.

> is there any information i should say to tie it with this series? or
> should be treated like a new separate patch?

It's a separate patch. You can use the Fixes: tag as documented in
submitting-patches.rst, but this is really subject to interpretation.

My personal interpretation is that it's not a functional fix and the
Fixes: tag should be omitted, even though you should refer to the merged
commit in the message of the new change.

Some maintainers want it no matter what. I don't recall having ever
sent a patch directly to Greg, so I don't know :) Try without :)

-- 
Stefano