driverdev-devel.linuxdriverproject.org archive mirror
 help / color / mirror / Atom feed
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Dinghao Liu <dinghao.liu@zju.edu.cn>
Cc: devel@driverdev.osuosl.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	kjlu@umn.edu, linux-kernel@vger.kernel.org,
	Julia Lawall <julia.lawall@inria.fr>,
	Stefano Brivio <sbrivio@redhat.com>,
	Shreeya Patel <shreeya.patel23498@gmail.com>,
	Larry Finger <Larry.Finger@lwfinger.net>
Subject: Re: [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode
Date: Mon, 27 Jul 2020 16:23:51 +0300	[thread overview]
Message-ID: <20200727132351.GF1913@kadam> (raw)
In-Reply-To: <20200727114451.GA1913@kadam>

I review things in the order that they appear in my inbox so I hadn't
seen Greg and Larry's comments.  You've now stumbled into an area of
politics where you have conflicting reviews...  :P  Fortunately, we're
all of us reasonable people.

I think your patch is correct in that it is what the original coder
intended.  You just need to sell the patch correctly in the commit
message.  The real danger of the original code would be if "authmode" is
accidentally 0x30 or 0xdd just because it was uninitialized.  Setting it
to zero ensures that it is not and it also matches how this is handled
in the rtl8723bs driver.  This matches the original author's intention.

So:

1) Re-write the commit message.

    The variable authmode can be uninitialized.  The danger would be
    if it equals _WPA_IE_ID_ (0xdd) or _WPA2_IE_ID_ (0x33).  We can
    avoid this by setting it to zero instead.  This is the approach that
    was used in the rtl8723bs driver.

2) Add a fixes tag.
   Fixes: 7b464c9fa5cc ("staging: r8188eu: Add files for new driver - part 4")

3) Change the commit to Larry's style with the "else if" and "else".
   Sometimes people just disagree about style so it's easiest to go with
   whatever the maintainer (Larry) wants.  It's not worth debating one
   way or the other so just redo it.

Then resend.  Google for "how to send a v2 patch" to get the right
format.

regards,
dan carpenter


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

  reply	other threads:[~2020-07-27 13:24 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-24 12:29 [PATCH] Staging: rtl8188eu: rtw_mlme: Fix uninitialized variable authmode Dinghao Liu
2020-07-24 13:28 ` Greg Kroah-Hartman
2020-07-24 17:56   ` Larry Finger
2020-07-27 13:39     ` dinghao.liu
2020-07-27 11:44 ` Dan Carpenter
2020-07-27 13:23   ` Dan Carpenter [this message]
2020-07-27 14:45     ` dinghao.liu

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=20200727132351.GF1913@kadam \
    --to=dan.carpenter@oracle.com \
    --cc=Larry.Finger@lwfinger.net \
    --cc=devel@driverdev.osuosl.org \
    --cc=dinghao.liu@zju.edu.cn \
    --cc=gregkh@linuxfoundation.org \
    --cc=julia.lawall@inria.fr \
    --cc=kjlu@umn.edu \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sbrivio@redhat.com \
    --cc=shreeya.patel23498@gmail.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).