Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180625155750.GA14306@jordon-HP-15-Notebook-PC>
 <CALCETrU3dS5t714BYqkxEYgkbJaESK6NP1Rsn3xq3En38yjJzQ@mail.gmail.com> <CAFqt6zZHo3frivz0Qjj7v2ifF-p+6530Bb8HZqO3iPrL9kemuA@mail.gmail.com>
In-Reply-To: <CAFqt6zZHo3frivz0Qjj7v2ifF-p+6530Bb8HZqO3iPrL9kemuA@mail.gmail.com>
From:   Andy Lutomirski <luto@kernel.org>
Date:   Mon, 25 Jun 2018 09:39:47 -0700
Message-ID: <CALCETrUTeVfj7n29AHK6Pa=caqmZOwqtZrBNawXKa_GBvCw8XQ@mail.gmail.com>
Subject: Re: [PATCH] x86/vdso: Change return type to vm_fault_t for fault handlers
To:     jrdr.linux@gmail.com
Cc:     Matthew Wilcox <willy@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>, X86 ML <x86@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, brajeswar.linux@gmail.com,
        sabyasachi.linux@gmail.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jun 25, 2018 at 9:15 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
>
> On Mon, Jun 25, 2018 at 9:41 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> > On Mon, Jun 25, 2018 at 8:55 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
> >>
> >> Use new return type vm_fault_t for fault handler. For
> >> now, this is just documenting that the function returns
> >> a VM_FAULT value rather than an errno. Once all instances
> >> are converted, vm_fault_t will become a distinct type.
> >
> > Whoa there..  Your commit message makes it sound like you're just
> > changing the return type, but:
> >
> >>                 if (tsc_pg && vclock_was_used(VCLOCK_HVCLOCK))
> >> -                       ret = vm_insert_pfn(vma, vmf->address,
> >> +                       ret = vmf_insert_pfn(vma, vmf->address,
> >>                                             vmalloc_to_pfn(tsc_pg));
> >>         }
> >>
> >> -       if (ret == 0 || ret == -EBUSY)
> >> -               return VM_FAULT_NOPAGE;
> >> -
> >> -       return VM_FAULT_SIGBUS;
> >> +       return ret;
> >
> > You're refactoring the code, too.
> >
> > Please fix your changelog.
>
>
> I have mentioned it.
>
> ********************
> Ref-> commit 1c8f422059ae ("mm: change return type to vm_fault_t")
>
> Previously vm_insert_pfn() returns err which has to
> mapped into VM_FAULT_* type. The new function
> vmf_insert_pfn() will replace this inefficiency by
> returning VM_FAULT_* type.
> *********************

The second line of your changelog says "For now, this is just
documenting...".  Please don't make future readers read all the way
through the whole changelog and try to reconcile conflicting sentences
to figure out what the patch does.

I read your changelog, then I read through the diff to make sure that
you were really just changing the return type (as your changelog
seemed to claim), and I decided that your patch was incorrect.  As the
maintainer of this code, this means that you've made my work harder
than it should be, so NAK.  Please improve your changelog and
resubmit.

--Andy