Re: [PATCH] fence: Use smp_mb__before_atomic()

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Jun 05, 2014 at 07:51:10AM -0400, Rob Clark wrote:
> On Wed, Jun 4, 2014 at 1:49 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Wed, Jun 04, 2014 at 03:28:33PM +0200, Thierry Reding wrote:
> >> On Wed, Jun 04, 2014 at 04:57:07PM +0530, Sumit Semwal wrote:
> >> > Hi Greg,
> >> >
> >> >
> >> > On 30 May 2014 21:38, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >> > > On Fri, May 30, 2014 at 10:15:05AM +0200, Thierry Reding wrote:
> >> > >> On Wed, May 28, 2014 at 01:51:45PM -0700, Greg Kroah-Hartman wrote:
> >> > >> > On Wed, May 28, 2014 at 04:26:32PM +0200, Thierry Reding wrote:
> >> > >> > > From: Thierry Reding <treding@nvidia.com>
> >> > >> > >
> >> > >> > > Commit febdbfe8a91c (arch: Prepare for smp_mb__{before,after}_atomic())
> >> > >> > > deprecated the smp_mb__{before,after}_{atomic,clear}_{dec,inc,bit}*()
> >> > >> > > functions in favour of the unified smp_mb__{before,after}_atomic().
> >> > >> > >
> >> > >> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> >> > >> > > ---
> >> > >> > >  drivers/base/fence.c | 4 ++--
> >> > >> >
> >> > >> > Where does this file come from?  I've not seen it before, and it's not
> >> > >> > in my tree.
> >> > >>
> >> > >> I think it came in through Sumit's tree and it's only in linux-next I
> >> > >> believe.
> >> > >
> >> > > Odd, linux-next is for merging things in Linus's next release.
> >> > >
> >> > > And as I have never seen this code that will end up being my
> >> > > responsibility to maintain, it seems strange that it will be merged in
> >> > > the next kernel development cycle.
> >> > >
> >> > > What broke down here with our review process that required something to
> >> > > be merged without at least a cc: to me?
> >> >
> >> > This is a new file added by Maarten's patches [1], that got reviewed
> >> > on dri-devel and other mailing lists. Since it was quite closely
> >> > associated with dma-buf, I figured I should take it through the
> >> > dma-buf tree.
> >> >
> >> > I am sorry I didn't notice that you weren't CC'ed on these patches -
> >> > Sincere apologies, since I should've noticed that during the patch
> >> > review process - I would take part of the blame here as well  :(
> >> >
> >> > I do realize now that atleast on my part, I should've asked you before
> >> > taking it through the dma-buf tree - I will make sure things like this
> >> > don't happen again through me.
> >> >
> >> > May I request you to help us handle this - would it help if we add
> >> > Maarten as the maintainer for this file? Any other suggestions?
> >>
> >> Perhaps something like the following would help?
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index fb39c9c3f0c1..d582f54adec8 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -2867,7 +2867,9 @@ L:        linux-media@vger.kernel.org
> >>  L:   dri-devel@lists.freedesktop.org
> >>  L:   linaro-mm-sig@lists.linaro.org
> >>  F:   drivers/base/dma-buf*
> >> +F:   drivers/base/fence.c
> >>  F:   include/linux/dma-buf*
> >> +F:   include/linux/fence.h
> >>  F:   Documentation/dma-buf-sharing.txt
> >>  T:   git git://git.linaro.org/people/sumitsemwal/linux-dma-buf.git
> >> @@ -2936,6 +2938,8 @@ T:        git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
> >>  S:   Supported
> >>  F:   Documentation/kobject.txt
> >>  F:   drivers/base/
> >> +X:   drivers/base/dma-buf*
> >> +X:   drivers/base/fence.c
> >>  F:   fs/sysfs/
> >>  F:   fs/debugfs/
> >>  F:   include/linux/kobj*
> >>
> >> That removes Greg from the list generated by get_maintainer.pl for
> >> anything that touches the DMA-BUF files.
> >
> > That doesn't really work for most people, I'll still be "responsible"
> > for the code.
> >
> >> Thinking about it, perhaps moving DMA-BUF into its own subdirectory
> >> would be an option too, to make the separation more obvious.
> >
> > That might be best for some of this.
> >
> > But again, why is the fence.c code needed at all anyway?  I'm not sold
> > on that.
> 
> Fence serves as a way to synchronize between (for example) multiple
> asynchronous gpu's.  There is definitely a need for this.  Otherwise
> performance for optimus/prime type setups is going to suck.

What's wrong with the 'sync' code in the drivers/staging/android/
directory?  I thought that is what that codebase was supposed to be
doing.

> I thought we had added something under Documentation/ about it, but I
> can't find it now (although possibly looking at the wrong trees)..
> there is at least a bit of a description in the commit msg:
> 
>   https://lkml.org/lkml/2014/2/24/602

Ah, so no documenation, no discussion, and you want to just throw it in
a directory I am responsible for?  That sounds like a major rush job...

> I don't think the question about whether we need something like fence
> to augment dma-buf is really in doubt.  Maybe it should live somewhere
> else, I'm not sure.  But it makes sense for it to live wherever
> dma-buf does, as they are intended to work together.

Ok, then let's give it a proper review cycle, and notify everyone
involved.  It doesn't look like this happened at all.  We don't add core
primitives to the kernel without a lot of discussion and agreement.  And
we sure don't add them without telling the person who owns the directory
(again, my pet peeve, I know...)

greg k-h