>
> > > > /*
> > > > * Protected with mmap_sem in write mode as multiple tasks
> > > > * might race to install the same page.
> > > > */
> > > > mmap_write_lock(alloc->mm);
> > > > - if (binder_get_installed_page(lru_page))
> > > > + if (binder_get_installed_page(lru_page)) {
> > > > + ret = 1;
> > >
> > > That is not a valid error value :(
> > >
> > > > goto out;
> > > > + }
> > > >
> > > > if (!alloc->vma) {
> > > > pr_err("%d: %s failed, no vma\n", alloc->pid, __func__);
> > > > @@ -241,7 +251,6 @@ static int binder_install_single_page(struct binder_alloc *alloc,
> > > > goto out;
> > > > }
> > > >
> > > > - page = alloc_page(GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO);
> > > > if (!page) {
> > > > pr_err("%d: failed to allocate page\n", alloc->pid);
> > > > ret = -ENOMEM;
> > > > @@ -252,7 +261,6 @@ static int binder_install_single_page(struct binder_alloc *alloc,
> > > > if (ret) {
> > > > pr_err("%d: %s failed to insert page at offset %lx with %d\n",
> > > > alloc->pid, __func__, addr - alloc->buffer, ret);
> > > > - __free_page(page);
> > > > ret = -ENOMEM;
> > > > goto out;
> > > > }
> > > > @@ -262,7 +270,9 @@ static int binder_install_single_page(struct binder_alloc *alloc,
> > > > out:
> > > > mmap_write_unlock(alloc->mm);
> > > > mmput_async(alloc->mm);
> > > > - return ret;
> > > > + if (ret && page)
> > > > + __free_page(page);
> > > > + return ret < 0 ? ret : 0;
> > >
> > > Please only use ? : for when you have to, otherwise please spell it out
> > > with a normal if statement:
> > > if (ret < 0)
> > > return ret;
> > > return 0;
> > >
> > > But, this is abusing the fact that you set "ret = 1" above, which is
> > > going to trip someone up in the future as that is NOT a normal coding
> > > pattern we have in the kernel, sorry.
> > >
> > > If you insist on this change, please rework it to not have that type of
> > > "positive means one thing, 0 means another, and negative means yet
> > > something else" please.
> >
> > I was trying to consolidate all free_page() calls into one place. Otherwise,
> > we would need multiple free_page() calls. I'm perfectly fine with having more
> > free_page() calls in both the ret = 1 and ret < 0 paths. In that case,
> > the ret = 1
> > path can be removed if you prefer.
>
> Remember, we write code for people first, and compilers second. You
> have to maintain this code for the next 10+ years, make it _VERY_
> obvious what is happening and how it works as you will be coming back to
> it and not remembering what was made for what reason at all.
no objection to this.
>
> thanks,
>
> greg k-h
Thanks
Barry
