A patch got applied to v4l bypassing v4l lists

A patch got applied to v4l bypassing v4l lists

[Guennadi Liakhovetski]

Hi Magnus, Paul,

just stumbled upon a patch

sh: sh_mobile ceu clock framework support

http://marc.info/?l=linux-sh&m\x122545217725877&w=2

with diffstat

 arch/sh/boards/board-ap325rxa.c            |    2 +-
 arch/sh/boards/mach-migor/setup.c          |    2 +-
 drivers/media/video/sh_mobile_ceu_camera.c |   20 +++++++++++++++++++-
 3 files changed, 21 insertions(+), 3 deletions(-)

that has been pulled through linux-sh ML and the sh tree without even 
being cc-ed to the v4l list, which wasn't a very good idea IMHO. Now this 
patch has to be manually "back-ported" to v4l hg repos using the 
"kernel-sync:" tag and only in part, because arch/sh directory is not in 
hg at all. Can we please avoid this in the future?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: A patch got applied to v4l bypassing v4l lists

[Paul Mundt]

On Thu, Dec 18, 2008 at 04:23:53PM +0100, Guennadi Liakhovetski wrote:
> Hi Magnus, Paul,
> 
> just stumbled upon a patch
> 
> sh: sh_mobile ceu clock framework support
> 
> http://marc.info/?l=linux-sh&m\x122545217725877&w=2
> 
> with diffstat
> 
>  arch/sh/boards/board-ap325rxa.c            |    2 +-
>  arch/sh/boards/mach-migor/setup.c          |    2 +-
>  drivers/media/video/sh_mobile_ceu_camera.c |   20 +++++++++++++++++++-
>  3 files changed, 21 insertions(+), 3 deletions(-)
> 
> that has been pulled through linux-sh ML and the sh tree without even 
> being cc-ed to the v4l list, which wasn't a very good idea IMHO. Now this 
> patch has to be manually "back-ported" to v4l hg repos using the 
> "kernel-sync:" tag and only in part, because arch/sh directory is not in 
> hg at all. Can we please avoid this in the future?
> 
It wasn't cc-ed to the v4l list because there was nothing v4l specific
about it. The patch in question likewise has a dependency on arch/sh/
changes, and it makes no sense to merge the v4l component out of order.

The last time v4l patches related to sh drivers got merged out of order
with the board support patches, both the driver and the boards were
broken for over a week. When there is something relevant to v4l, then the
list will be CCed, but I will not hold trivial patches hostage to
overzealous patchflow enforcement when the end result is likely to cause
more trouble than it prevents.

When touching files outside of arch/sh/, it is always a judgement call
whether it makes sense to include it through that subsystem's tree or
whether to simply merge it directly. In this case, given that it is an
sh-specific driver, and the changes are trivial in nature, with an
inherent dependency on other bits, I see no reason to force this change
through the v4l tree.

If you wish to be CC-ed on all trivial patches relating to sh drivers in
the v4l space, that is certainly something we can watch out for in the
future, but I will still be applying the patches where it makes sense.

Re: A patch got applied to v4l bypassing v4l lists

[Guennadi Liakhovetski]

On Fri, 19 Dec 2008, Paul Mundt wrote:

> On Thu, Dec 18, 2008 at 04:23:53PM +0100, Guennadi Liakhovetski wrote:
> > Hi Magnus, Paul,
> > 
> > just stumbled upon a patch
> > 
> > sh: sh_mobile ceu clock framework support
> > 
> > http://marc.info/?l=linux-sh&m\x122545217725877&w=2
> > 
> > with diffstat
> > 
> >  arch/sh/boards/board-ap325rxa.c            |    2 +-
> >  arch/sh/boards/mach-migor/setup.c          |    2 +-
> >  drivers/media/video/sh_mobile_ceu_camera.c |   20 +++++++++++++++++++-
> >  3 files changed, 21 insertions(+), 3 deletions(-)
> > 
> > that has been pulled through linux-sh ML and the sh tree without even 
> > being cc-ed to the v4l list, which wasn't a very good idea IMHO. Now this 
> > patch has to be manually "back-ported" to v4l hg repos using the 
> > "kernel-sync:" tag and only in part, because arch/sh directory is not in 
> > hg at all. Can we please avoid this in the future?
> > 
> It wasn't cc-ed to the v4l list because there was nothing v4l specific
> about it. The patch in question likewise has a dependency on arch/sh/
> changes, and it makes no sense to merge the v4l component out of order.
> 
> The last time v4l patches related to sh drivers got merged out of order
> with the board support patches, both the driver and the boards were
> broken for over a week. When there is something relevant to v4l, then the
> list will be CCed, but I will not hold trivial patches hostage to
> overzealous patchflow enforcement when the end result is likely to cause
> more trouble than it prevents.
> 
> When touching files outside of arch/sh/, it is always a judgement call
> whether it makes sense to include it through that subsystem's tree or
> whether to simply merge it directly. In this case, given that it is an
> sh-specific driver, and the changes are trivial in nature, with an
> inherent dependency on other bits, I see no reason to force this change
> through the v4l tree.
> 
> If you wish to be CC-ed on all trivial patches relating to sh drivers in
> the v4l space, that is certainly something we can watch out for in the
> future, but I will still be applying the patches where it makes sense.

I certainly understand, that the patch in question didn't contain any 
video specidic code, and you both are well able to justify its 
correctness, I'm just saying that because of the way v4l patches are 
handled it causes extra work, and not even knowing about such patches adds 
the necessity to search for them first - ok, thanks to git-blame it wasn't 
very difficult this time, but if the code had been removed, for instance, 
it could have been a bit trickier. So, yes, please, at least cc the v4l 
list on such patches.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: A patch got applied to v4l bypassing v4l lists

[Paul Mundt]

On Thu, Dec 18, 2008 at 05:22:22PM +0100, Guennadi Liakhovetski wrote:
> On Fri, 19 Dec 2008, Paul Mundt wrote:
> > If you wish to be CC-ed on all trivial patches relating to sh drivers in
> > the v4l space, that is certainly something we can watch out for in the
> > future, but I will still be applying the patches where it makes sense.
> 
> I certainly understand, that the patch in question didn't contain any 
> video specidic code, and you both are well able to justify its 
> correctness, I'm just saying that because of the way v4l patches are 
> handled it causes extra work, and not even knowing about such patches adds 
> the necessity to search for them first - ok, thanks to git-blame it wasn't 
> very difficult this time, but if the code had been removed, for instance, 
> it could have been a bit trickier. So, yes, please, at least cc the v4l 
> list on such patches.
> 
It should not cause extra work at all. The only time it may cause extra
work is if you are talking about splitting up the patch and pulling in
the v4l specific parts in to your v4l tree. My point is that this is
absolutely the wrong thing to do, since the changes are tied together for
a reason.

The last time you went down this splitting of the patch path you
completely broke bisection for us for an extended period of time, and
choosing policy over functionality is simply not something I will be part
of. If you want to split the patch up and merge parts in to your own
tree, that is perfectly fine, but it is both unnecessary, and I will
still be merging the change including its dependencies in one shot
without the split in my own tree so as to not break bi-section.

If v4l has a policy that anything modifying drivers/media in anyway
whatsoever needs to be split out and merged through the v4l tree, you
might consider rethinking your policy and reshaping it in to something
that actually makes sense. Breaking bisection is not acceptable, period.

Re: A patch got applied to v4l bypassing v4l lists

[Guennadi Liakhovetski]

On Fri, 19 Dec 2008, Paul Mundt wrote:

> It should not cause extra work at all. The only time it may cause extra
> work is if you are talking about splitting up the patch and pulling in
> the v4l specific parts in to your v4l tree. My point is that this is
> absolutely the wrong thing to do, since the changes are tied together for
> a reason.
> 
> The last time you went down this splitting of the patch path you
> completely broke bisection for us for an extended period of time, and
> choosing policy over functionality is simply not something I will be part
> of. If you want to split the patch up and merge parts in to your own
> tree, that is perfectly fine, but it is both unnecessary, and I will
> still be merging the change including its dependencies in one shot
> without the split in my own tree so as to not break bi-section.
> 
> If v4l has a policy that anything modifying drivers/media in anyway
> whatsoever needs to be split out and merged through the v4l tree, you
> might consider rethinking your policy and reshaping it in to something
> that actually makes sense. Breaking bisection is not acceptable, period.

Agree - breaking bisection is not something I'm looking into.

If you like, I can explain to you where this extra work comes from. That's 
my current understanding of the work flow on v4l, it might still be not 
quite right, so I'll be happy if anyone corrects me and tells me a better 
way to handle this.

v4l uses mercurial repositories as primary dveelopment trees. These 
repositories do not contain complete kernel trees, instead, they present a 
directory with some tools, where linux is a subdirectory of, and that's 
where a part of the kernel is reproduced.

That part includes of course drivers/media, include/media, some files 
under include/linux, and a couple more random files which has at some 
moment been integrated because they were relevant or because some patch 
touched simultaneously those files and v4l code.

These trees are used for out-of-tree driver development, besides, they are 
trying to make this development and testing possible with a few kernel 
versions back, which means they have to modify sources to include various

#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 27)
...
#else
...
#endif

blocks etc.

Now, it is implicitly assumed, that any development touching v4l code goes 
only in one direction - from these hg trees towards mainline. Any changes 
coming in the other direction involve extra work - they have to be 
back-ported to those hg-trees and specially marked to avoid scripts 
attempting to push them into git-trees again.

So, that's exactly what I had to do this time - find your patch, split off 
the v4l part, commit it marking "not for upstream".

So, now I'd really love to hear that I'm wrong and I oversee much easier 
ways to do this.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: A patch got applied to v4l bypassing v4l lists

[Mauro Carvalho Chehab]

Hi Paul,

> On Fri, 19 Dec 2008, Paul Mundt wrote:
> 
> > It should not cause extra work at all. The only time it may cause extra
> > work is if you are talking about splitting up the patch and pulling in
> > the v4l specific parts in to your v4l tree. My point is that this is
> > absolutely the wrong thing to do, since the changes are tied together for
> > a reason.

Not sure why, buy your replies didn't arrive at the ML. Anyway, from my side,
it is ok to tie the v4l and sh changes together and commit it via either sh or
v4l tree, provided that the patch won't break bisect.

Yet, it is nice if you can c/c us on such patches, in order to help to avoid
future merge conflicts (since, otherwise, development may be done using the
previous version). Otherwise, I'll backport the patch into the development tree
only after reaching Linus tree.

A side note: maybe the design of pxa_camera could be improved to avoid needing
to be touched as architecture changes. This is the only v4l driver that includes
asm/arch header files.

Cheers,
Mauro

Re: A patch got applied to v4l bypassing v4l lists

[Guennadi Liakhovetski]

On Thu, 18 Dec 2008, Mauro Carvalho Chehab wrote:

> Hi Paul,
> 
> > On Fri, 19 Dec 2008, Paul Mundt wrote:
> > 
> > > It should not cause extra work at all. The only time it may cause extra
> > > work is if you are talking about splitting up the patch and pulling in
> > > the v4l specific parts in to your v4l tree. My point is that this is
> > > absolutely the wrong thing to do, since the changes are tied together for
> > > a reason.
> 
> Not sure why, buy your replies didn't arrive at the ML. Anyway, from my side,
> it is ok to tie the v4l and sh changes together and commit it via either sh or
> v4l tree, provided that the patch won't break bisect.
> 
> Yet, it is nice if you can c/c us on such patches, in order to help to avoid
> future merge conflicts (since, otherwise, development may be done using the
> previous version). Otherwise, I'll backport the patch into the development tree
> only after reaching Linus tree.
> 
> A side note: maybe the design of pxa_camera could be improved to avoid needing
> to be touched as architecture changes. This is the only v4l driver that includes
> asm/arch header files.

The patch in question was for sh_mobile_ceu_camera.c - not for pxa, and 
even though that one doesn't include any asm headers, as you see, it is 
also tied pretty closely with respective platform code.

As for including asm headers in pxa_camera.c - it wouldn't be easy to get 
rid of them, one of the main obstacles is the use of the pxa-specific 
dma-channel handling API.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: A patch got applied to v4l bypassing v4l lists

[Mauro Carvalho Chehab]

On Fri, 19 Dec 2008 00:30:05 +0100 (CET)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

> > A side note: maybe the design of pxa_camera could be improved to avoid needing
> > to be touched as architecture changes. This is the only v4l driver that includes
> > asm/arch header files.
> 
> The patch in question was for sh_mobile_ceu_camera.c - not for pxa, and 
> even though that one doesn't include any asm headers, as you see, it is 
> also tied pretty closely with respective platform code.

> As for including asm headers in pxa_camera.c - it wouldn't be easy to get 
> rid of them, one of the main obstacles is the use of the pxa-specific 
> dma-channel handling API.

Ok. I dunno the specific details of the sh and pxa bindings, but it would be
better to have it more independent from architecture specific implementation
details.

Cheers,
Mauro

Re: A patch got applied to v4l bypassing v4l lists

[Guennadi Liakhovetski]

On Thu, 18 Dec 2008, Mauro Carvalho Chehab wrote:

> On Fri, 19 Dec 2008 00:30:05 +0100 (CET)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 
> > > A side note: maybe the design of pxa_camera could be improved to avoid needing
> > > to be touched as architecture changes. This is the only v4l driver that includes
> > > asm/arch header files.
> > 
> > The patch in question was for sh_mobile_ceu_camera.c - not for pxa, and 
> > even though that one doesn't include any asm headers, as you see, it is 
> > also tied pretty closely with respective platform code.
> 
> > As for including asm headers in pxa_camera.c - it wouldn't be easy to get 
> > rid of them, one of the main obstacles is the use of the pxa-specific 
> > dma-channel handling API.
> 
> Ok. I dunno the specific details of the sh and pxa bindings, but it would be
> better to have it more independent from architecture specific implementation
> details.

...that's what I'm saying - we have to work with the complete kernel tree. 
Working with a part of it is no fun. Just think about one thing - changing 
platform data struct layout. It will still compile as long as you preserve 
the name, because that's just a void * you get from platform, but you 
better don't try to run that... Or take this clock change now to the 
sh_mobile_ceu_camera.c. It will still compile with older kernels, but it 
won't run.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: A patch got applied to v4l bypassing v4l lists

[Jean-Christophe PLAGNIOL-VILLARD]

> Agree - breaking bisection is not something I'm looking into.
> 
> If you like, I can explain to you where this extra work comes from. That's 
> my current understanding of the work flow on v4l, it might still be not 
> quite right, so I'll be happy if anyone corrects me and tells me a better 
> way to handle this.
> 
> v4l uses mercurial repositories as primary dveelopment trees. These 
> repositories do not contain complete kernel trees, instead, they present a 
> directory with some tools, where linux is a subdirectory of, and that's 
> where a part of the kernel is reproduced.

so why do not you use git and git-modules it will save you time and will be
simplest to use and track the v4l tree

Best Regards,
J.

Re: A patch got applied to v4l bypassing v4l lists

[Guennadi Liakhovetski]

On Thu, 18 Dec 2008, Jean-Christophe PLAGNIOL-VILLARD wrote:

> > Agree - breaking bisection is not something I'm looking into.
> > 
> > If you like, I can explain to you where this extra work comes from. That's 
> > my current understanding of the work flow on v4l, it might still be not 
> > quite right, so I'll be happy if anyone corrects me and tells me a better 
> > way to handle this.
> > 
> > v4l uses mercurial repositories as primary dveelopment trees. These 
> > repositories do not contain complete kernel trees, instead, they present a 
> > directory with some tools, where linux is a subdirectory of, and that's 
> > where a part of the kernel is reproduced.
> 
> so why do not you use git and git-modules it will save you time and will be
> simplest to use and track the v4l tree

I do use git, that's what I'm developing with, and then I have to export 
patches from git, import them in mercurial to prepare a pull request into 
the central v4l-dvb hg repository.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: A patch got applied to v4l bypassing v4l lists

[Magnus Damm]

Hi Guennadi,

On Fri, Dec 19, 2008 at 12:23 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> just stumbled upon a patch
>
> sh: sh_mobile ceu clock framework support
>
> that has been pulled through linux-sh ML and the sh tree without even
> being cc-ed to the v4l list, which wasn't a very good idea IMHO. Now this
> patch has to be manually "back-ported" to v4l hg repos using the
> "kernel-sync:" tag and only in part, because arch/sh directory is not in
> hg at all. Can we please avoid this in the future?

That specific patch set improved clock framework support and changed
quite a few SuperH drivers. I should have CC-ed you. Sorry about that,
will do next time.

/ magnus

Re: A patch got applied to v4l bypassing v4l lists

[Paul Mundt]

On Fri, Dec 19, 2008 at 08:15:19AM +0100, Guennadi Liakhovetski wrote:
> On Thu, 18 Dec 2008, Mauro Carvalho Chehab wrote:
> 
> > On Fri, 19 Dec 2008 00:30:05 +0100 (CET)
> > Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > 
> > > > A side note: maybe the design of pxa_camera could be improved to avoid needing
> > > > to be touched as architecture changes. This is the only v4l driver that includes
> > > > asm/arch header files.
> > > 
> > > The patch in question was for sh_mobile_ceu_camera.c - not for pxa, and 
> > > even though that one doesn't include any asm headers, as you see, it is 
> > > also tied pretty closely with respective platform code.
> > 
> > > As for including asm headers in pxa_camera.c - it wouldn't be easy to get 
> > > rid of them, one of the main obstacles is the use of the pxa-specific 
> > > dma-channel handling API.
> > 
> > Ok. I dunno the specific details of the sh and pxa bindings, but it would be
> > better to have it more independent from architecture specific implementation
> > details.
> 
> ...that's what I'm saying - we have to work with the complete kernel tree. 
> Working with a part of it is no fun. Just think about one thing - changing 
> platform data struct layout. It will still compile as long as you preserve 
> the name, because that's just a void * you get from platform, but you 
> better don't try to run that... Or take this clock change now to the 
> sh_mobile_ceu_camera.c. It will still compile with older kernels, but it 
> won't run.
> 
Correct, this is one of the key issues with not breaking bisection. It is
not just a matter of whether something compiles or not, but whether we
can actually boot and test the result. Things that have a fixed
dependency on each other need to happen in the same change, and can not
be split. That means that either the v4l tree needs to track changes to
architecture files as well, or it will have to resync and pull down
changes made upstream periodically. The latter is how every other tree in
existence is run, largely because of these sorts of reasons. It seems
like most of the trouble comes from running a partial tree, but since the
days of the driver model, it is no longer possible to keep dependent
changes confined and isolated to that directory structure.