From: Dave Jiang <dave.jiang@intel.com>
To: Linus Torvalds <torvalds@linux-foundation.org>,
Dan Williams <dan.j.williams@intel.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
"linux-nvdimm@lists.01.org" <linux-nvdimm@lists.01.org>
Subject: Re: [GIT PULL]: libnvdimm updates for v4.19-rc1
Date: Mon, 20 Aug 2018 10:21:43 -0700 [thread overview]
Message-ID: <4b2eaec3-7dd7-5b68-d0e1-3d0834b83c8b@intel.com> (raw)
In-Reply-To: <CA+55aFy18-r8FM614FWo8o0T2yOSy9rvv1hX50EOij84v1n1-g@mail.gmail.com>
On 08/18/2018 04:15 PM, Linus Torvalds wrote:
> On Fri, Aug 17, 2018 at 9:17 AM Jiang, Dave <dave.jiang@intel.com> wrote:
>>
>> Please pull to receive libnvdimm contributions for v4.19-rc1
>
> So I don't care about the libnvdimm code itself, but when you guys add
> code to the core mm/ code, I start looking.
>
> And when I then see shit like this:
>
> if (is_zone_device_page(p))
> tk->size_shift = ilog2(dev_pagemap_mapping_size(p, vma));
>
> I go "No".
>
> There's two issues with this:
>
> - the damn thing can return 0, which would be an error for ilog2, and
> the result is undefined
>
> You never check for errors. There's a check for tk->size_shift ==
> 0, but is that actually the guaranteed return value of ilog2(0)? No.
>
> - there is exactly one user of dev_pagemap_mapping_size(), and the above is it.
>
> Why the hell didn't that function just return the number of bits to
> begin with?
>
> I do not care if you screw up your own particular driver that much.
>
> But when I see a pull request with complete and utter garbage in the
> core mm part, I will not pull.
>
> This is not acceptable.
>
> Pulled, merge conflict fixed, and then immediately unpulled again.
>
> I do not want to *EVER* see these kinds of patches to core MM code.
> And I'm not gfoing to pull these patches or anythinig that looks like
> it has any trace of this shit.
>
> I get upset, because dammit, I expect better. I don't want to go "oh,
> this changes core code, let's just skim over the patches" and
> immediately find something fundamentally broken like this.
>
> Linus
>
Linus,
I have addressed the mistake by changing the function directly to return
PXX_SHIFT instead of size and removed the offending ilog2() call. I have
pushed the branch to kernel.org nvdimm tree for linux-next soaking. If
it's acceptable to you I'll send the pull request again later this week
for 4.19 inclusion. Thank you!
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
prev parent reply other threads:[~2018-08-20 17:21 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-17 16:17 [GIT PULL]: libnvdimm updates for v4.19-rc1 Jiang, Dave
2018-08-18 23:15 ` Linus Torvalds
2018-08-19 5:08 ` Dan Williams
2018-08-20 17:21 ` Dave Jiang [this message]
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=4b2eaec3-7dd7-5b68-d0e1-3d0834b83c8b@intel.com \
--to=dave.jiang@intel.com \
--cc=dan.j.williams@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nvdimm@lists.01.org \
--cc=torvalds@linux-foundation.org \
/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).