linux-kernel - Re: [PATCH 2/3] Staging: olpc_dcon: replace some magic numbers

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]

Date:	Fri, 16 Aug 2013 11:54:19 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Jens Frederich <jfrederich@...il.com>
Cc:	devel@...verdev.osuosl.org, Greg KH <gregkh@...uxfoundation.org>,
	Jon Nettleton <jon.nettleton@...il.com>,
	Daniel Drake <dsd@...top.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] Staging: olpc_dcon: replace some magic numbers

On Fri, Aug 16, 2013 at 09:40:38AM +0200, Jens Frederich wrote:
> On Fri, Aug 16, 2013 at 9:13 AM, Dan Carpenter <dan.carpenter@...cle.com> wrote:
> > On Thu, Aug 15, 2013 at 09:34:55PM +0200, Jens Frederich wrote:
> >> The 0x42 initialize squence 0x101 is wrong.  According to
> >> the specification Bit 8 is reserved, thus not in use.
> >> I removed it.
> >
> > Really these code changes should be in a separate patch and labeled
> > "Don't set reserved bit." instead of hidden away inside a cleanup
> > patch.
> >
> 
> The patch is applied. Still, good to know. It's not so easy to find the
> right patch granularity as newbie.
> 

Yeah.  Staging is for educating people about kernel process as much
as it is about writing code.

The rule here is "Don't mix code changes into a cleanup patches."
What we want is if you have a bug then you can look through
`git log --oneline` output and guess which patch introduced the bug.
This patch is a cleanup patch so it shouldn't introduce any code
changes or any bugs.

Meanwhile, if you are making a code change you can make any cleanups
you need to in order to do the change.  Also if there is an existing
checkpatch warning on any of the lines you touch, then that's ok to
fix as well.  Or if there are tiny related changes than that's fine.

There are three problems with big patches:
1) It breaks the --oneline summary to mix two things into one patch.
2) It makes the patch harder to review.  For example, sometimes
   people fix a bug and rename 10 variables as well.
3) The more lines your patch is, the more chance there is that we
   will reject it based on one of those lines.  You don't like
   redoing patches and we don't like making people redo them.  So
   small patches are better and put the more controversial ones at
   the end so the first patches can be applied.

No one totally agrees what "small closely related cleanups" means so
it's better to be conservative.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/