Re: [PATCH 0/2] Fix a few more warnings

Re: [PATCH 0/2] Fix a few more warnings

[Sylwester Nawrocki]

On 10/29/2012 11:21 AM, Mauro Carvalho Chehab wrote:
> Hans Verkuil yesterday's build still got two warnings at the
> generic drivers:
>         http://hverkuil.home.xs4all.nl/logs/Sunday.log
> 
> They didn't appear at i386 build probably because of some
> optimization done there.
> 
> Anyway, fixing them are trivial, so let's do it.
> 
> After applying those patches, the only drivers left producing
> warnings are the following platform drivers:
> 
> drivers/media/platform/davinci/dm355_ccdc.c
> drivers/media/platform/davinci/dm644x_ccdc.c
> drivers/media/platform/davinci/vpbe_osd.c
> drivers/media/platform/omap3isp/ispccdc.c
> drivers/media/platform/omap3isp/isph3a_aewb.c
> drivers/media/platform/omap3isp/isph3a_af.c
> drivers/media/platform/omap3isp/isphist.c
> drivers/media/platform/omap3isp/ispqueue.c
> drivers/media/platform/omap3isp/ispvideo.c
> drivers/media/platform/omap/omap_vout.c
> drivers/media/platform/s5p-fimc/fimc-capture.c
> drivers/media/platform/s5p-fimc/fimc-lite.c

For these two files I've sent already a pull request [1], which
includes a fixup patch
s5p-fimc: Don't ignore return value of vb2_queue_init()

BTW, shouldn't things like these be taken care when someone does
a change at the core code ? I'm not having issues in this case at all,
but if there is many people doing constantly changes at the core it
might imply for driver authors/maintainers wasting much of their time
for fixing issues resulting from constant changes at the base code.

Thanks,
Sylwester

> drivers/media/platform/sh_vou.c
> drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
> 
> Platform driver maintainers: please fix, as those warnings could be
> hiding real bugs. Also, removing all warnings is interesting,
> as they helps to detect when new possible bugs got introduced.
> 
> I think Hans also use "make W=1" option when doing his tests.
> 
> Mauro Carvalho Chehab (2):
>   [media] drxk_hard: fix the return code from an error handler
>   [media] xc4000: Fix a few warnings
> 
>  drivers/media/dvb-frontends/drxk_hard.c | 1 +
>  drivers/media/tuners/xc4000.c           | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)

[1] http://patchwork.linuxtv.org/patch/15195/

Re: [PATCH 0/2] Fix a few more warnings

[Ezequiel Garcia]

On Mon, Oct 29, 2012 at 8:19 AM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> On 10/29/2012 11:21 AM, Mauro Carvalho Chehab wrote:
>> Hans Verkuil yesterday's build still got two warnings at the
>> generic drivers:
>>         http://hverkuil.home.xs4all.nl/logs/Sunday.log
>>
>> They didn't appear at i386 build probably because of some
>> optimization done there.
>>
>> Anyway, fixing them are trivial, so let's do it.
>>
>> After applying those patches, the only drivers left producing
>> warnings are the following platform drivers:
>>
>> drivers/media/platform/davinci/dm355_ccdc.c
>> drivers/media/platform/davinci/dm644x_ccdc.c
>> drivers/media/platform/davinci/vpbe_osd.c
>> drivers/media/platform/omap3isp/ispccdc.c
>> drivers/media/platform/omap3isp/isph3a_aewb.c
>> drivers/media/platform/omap3isp/isph3a_af.c
>> drivers/media/platform/omap3isp/isphist.c
>> drivers/media/platform/omap3isp/ispqueue.c
>> drivers/media/platform/omap3isp/ispvideo.c
>> drivers/media/platform/omap/omap_vout.c
>> drivers/media/platform/s5p-fimc/fimc-capture.c
>> drivers/media/platform/s5p-fimc/fimc-lite.c
>
> For these two files I've sent already a pull request [1], which
> includes a fixup patch
> s5p-fimc: Don't ignore return value of vb2_queue_init()
>
> BTW, shouldn't things like these be taken care when someone does
> a change at the core code ? I'm not having issues in this case at all,
> but if there is many people doing constantly changes at the core it
> might imply for driver authors/maintainers wasting much of their time
> for fixing issues resulting from constant changes at the base code.
>

Indeed. When I changed vb2_queue_init() to return something,
I went and fix every user. I don't know why I missed those.

My bad,

    Ezequiel

Re: [PATCH 0/2] Fix a few more warnings

[Mauro Carvalho Chehab]

Em Mon, 29 Oct 2012 12:19:32 +0100
Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:

> On 10/29/2012 11:21 AM, Mauro Carvalho Chehab wrote:
> > Hans Verkuil yesterday's build still got two warnings at the
> > generic drivers:
> >         http://hverkuil.home.xs4all.nl/logs/Sunday.log
> > 
> > They didn't appear at i386 build probably because of some
> > optimization done there.
> > 
> > Anyway, fixing them are trivial, so let's do it.
> > 
> > After applying those patches, the only drivers left producing
> > warnings are the following platform drivers:
> > 
> > drivers/media/platform/davinci/dm355_ccdc.c
> > drivers/media/platform/davinci/dm644x_ccdc.c
> > drivers/media/platform/davinci/vpbe_osd.c
> > drivers/media/platform/omap3isp/ispccdc.c
> > drivers/media/platform/omap3isp/isph3a_aewb.c
> > drivers/media/platform/omap3isp/isph3a_af.c
> > drivers/media/platform/omap3isp/isphist.c
> > drivers/media/platform/omap3isp/ispqueue.c
> > drivers/media/platform/omap3isp/ispvideo.c
> > drivers/media/platform/omap/omap_vout.c
> > drivers/media/platform/s5p-fimc/fimc-capture.c
> > drivers/media/platform/s5p-fimc/fimc-lite.c
> 
> For these two files I've sent already a pull request [1], which
> includes a fixup patch
> s5p-fimc: Don't ignore return value of vb2_queue_init()
> 
> BTW, shouldn't things like these be taken care when someone does
> a change at the core code ? 

Sure. I remember I saw one patch with s5p on that series[1].
Can't remember anymore if it were acked and merged directly, if
it was opted to send it via your tree (or maybe that patch was just
incomplete, and got unnoticed on that time).

[1] https://patchwork.kernel.org/patch/1372871/

It is not easy to enforce those kind of things for platform drivers,
as there's not yet a single .config file that could be used to test
all arm drivers. Hans automatic builds might be useful, if there weren't
any warns at the -git tree build at the tested archs, but there are
so many warnings that I think I never saw any such report saying that
there's no warning.

Btw, are there anyone really consistently using his reports to fix things?

> I'm not having issues in this case at all,
> but if there is many people doing constantly changes at the core it
> might imply for driver authors/maintainers wasting much of their time
> for fixing issues resulting from constant changes at the base code.

Regards,
Mauro

Re: [PATCH 0/2] Fix a few more warnings

[Mauro Carvalho Chehab]

Em Mon, 29 Oct 2012 11:38:39 +0100
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Monday 29 October 2012 08:21:56 Mauro Carvalho Chehab wrote:
> > Hans Verkuil yesterday's build still got two warnings at the
> > generic drivers:
> >         http://hverkuil.home.xs4all.nl/logs/Sunday.log
> > 
> > They didn't appear at i386 build probably because of some
> > optimization done there.
> > 
> > Anyway, fixing them are trivial, so let's do it.
> > 
> > After applying those patches, the only drivers left producing
> > warnings are the following platform drivers:
> > 
> > drivers/media/platform/davinci/dm355_ccdc.c
> > drivers/media/platform/davinci/dm644x_ccdc.c
> > drivers/media/platform/davinci/vpbe_osd.c
> > drivers/media/platform/omap3isp/ispccdc.c
> > drivers/media/platform/omap3isp/isph3a_aewb.c
> > drivers/media/platform/omap3isp/isph3a_af.c
> > drivers/media/platform/omap3isp/isphist.c
> > drivers/media/platform/omap3isp/ispqueue.c
> > drivers/media/platform/omap3isp/ispvideo.c
> > drivers/media/platform/omap/omap_vout.c
> > drivers/media/platform/s5p-fimc/fimc-capture.c
> > drivers/media/platform/s5p-fimc/fimc-lite.c
> > drivers/media/platform/sh_vou.c
> > drivers/media/platform/soc_camera/sh_mobile_ceu_camera.c
> > 
> > Platform driver maintainers: please fix, as those warnings could be
> > hiding real bugs.
> 
> I've sent you a pull request for v3.7 on Thu, 25 Oct 2012 with two patches 
> that fix most of the warnings:
> 
>       omap3isp: video: Fix warning caused by bad vidioc_s_crop prototype
>       omap3isp: Fix warning caused by bad subdev events operations prototypes

Ok. I'll review it today.

> The other two OMAP3 ISP warnings are false positive. They will go away when 
> we'll switch to videobuf2 (which is on my to-do list).

While you're not ready to submit the omap3 vb2 patches, would you mind fixing
them? Failing to do that will prevent us to improve the process of detecting
warnings at day zero. For example, I suspect that  Hans automatic compilation
tool will keep saying that WARNINGS are present at -git, while we don't fix
all those platform warnings.

Regards,
Mauro

Re: [PATCH 0/2] Fix a few more warnings

[Ezequiel Garcia]

On Mon, Oct 29, 2012 at 8:32 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em Mon, 29 Oct 2012 12:19:32 +0100
> Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:
>
>> On 10/29/2012 11:21 AM, Mauro Carvalho Chehab wrote:
>> > Hans Verkuil yesterday's build still got two warnings at the
>> > generic drivers:
>> >         http://hverkuil.home.xs4all.nl/logs/Sunday.log
>> >
>> > They didn't appear at i386 build probably because of some
>> > optimization done there.
>> >
>> > Anyway, fixing them are trivial, so let's do it.
>> >
>> > After applying those patches, the only drivers left producing
>> > warnings are the following platform drivers:
>> >
>> > drivers/media/platform/davinci/dm355_ccdc.c
>> > drivers/media/platform/davinci/dm644x_ccdc.c
>> > drivers/media/platform/davinci/vpbe_osd.c
>> > drivers/media/platform/omap3isp/ispccdc.c
>> > drivers/media/platform/omap3isp/isph3a_aewb.c
>> > drivers/media/platform/omap3isp/isph3a_af.c
>> > drivers/media/platform/omap3isp/isphist.c
>> > drivers/media/platform/omap3isp/ispqueue.c
>> > drivers/media/platform/omap3isp/ispvideo.c
>> > drivers/media/platform/omap/omap_vout.c
>> > drivers/media/platform/s5p-fimc/fimc-capture.c
>> > drivers/media/platform/s5p-fimc/fimc-lite.c
>>
>> For these two files I've sent already a pull request [1], which
>> includes a fixup patch
>> s5p-fimc: Don't ignore return value of vb2_queue_init()
>>
>> BTW, shouldn't things like these be taken care when someone does
>> a change at the core code ?
>
> Sure. I remember I saw one patch with s5p on that series[1].
> Can't remember anymore if it were acked and merged directly, if
> it was opted to send it via your tree (or maybe that patch was just
> incomplete, and got unnoticed on that time).
>
> [1] https://patchwork.kernel.org/patch/1372871/
>
> It is not easy to enforce those kind of things for platform drivers,
> as there's not yet a single .config file that could be used to test
> all arm drivers. Hans automatic builds might be useful, if there weren't
> any warns at the -git tree build at the tested archs, but there are
> so many warnings that I think I never saw any such report saying that
> there's no warning.
>
> Btw, are there anyone really consistently using his reports to fix things?
>

I do.

    Ezequiel

Re: [PATCH 0/2] Fix a few more warnings

[Sylwester Nawrocki]

On 10/29/2012 12:32 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 29 Oct 2012 12:19:32 +0100
> Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:
> 
>> On 10/29/2012 11:21 AM, Mauro Carvalho Chehab wrote:
>>> Hans Verkuil yesterday's build still got two warnings at the
>>> generic drivers:
>>>         http://hverkuil.home.xs4all.nl/logs/Sunday.log
>>>
>>> They didn't appear at i386 build probably because of some
>>> optimization done there.
>>>
>>> Anyway, fixing them are trivial, so let's do it.
>>>
>>> After applying those patches, the only drivers left producing
>>> warnings are the following platform drivers:
>>>
>>> drivers/media/platform/davinci/dm355_ccdc.c
>>> drivers/media/platform/davinci/dm644x_ccdc.c
>>> drivers/media/platform/davinci/vpbe_osd.c
>>> drivers/media/platform/omap3isp/ispccdc.c
>>> drivers/media/platform/omap3isp/isph3a_aewb.c
>>> drivers/media/platform/omap3isp/isph3a_af.c
>>> drivers/media/platform/omap3isp/isphist.c
>>> drivers/media/platform/omap3isp/ispqueue.c
>>> drivers/media/platform/omap3isp/ispvideo.c
>>> drivers/media/platform/omap/omap_vout.c
>>> drivers/media/platform/s5p-fimc/fimc-capture.c
>>> drivers/media/platform/s5p-fimc/fimc-lite.c
>>
>> For these two files I've sent already a pull request [1], which
>> includes a fixup patch
>> s5p-fimc: Don't ignore return value of vb2_queue_init()
>>
>> BTW, shouldn't things like these be taken care when someone does
>> a change at the core code ? 
> 
> Sure. I remember I saw one patch with s5p on that series[1].
> Can't remember anymore if it were acked and merged directly, if
> it was opted to send it via your tree (or maybe that patch was just
> incomplete, and got unnoticed on that time).

I think this was one of the first patches from Ezequiel, when he wanted
to change the vb2_queue_init() function signature so it returns void (as
there were only BUG_ON()s used inside it). But what we need now at drivers
is the opposite, i.e. to keep checking the return value and to add where
such checks are missing. Thus patch [1] is not applicable, since BUG_ONs
were replaced with WARN_ON and __must_check annotation was added to the
vb2_queue_init() function declaration.

> [1] https://patchwork.kernel.org/patch/1372871/
> 
> It is not easy to enforce those kind of things for platform drivers,
> as there's not yet a single .config file that could be used to test
> all arm drivers. Hans automatic builds might be useful, if there weren't

The ARM arch consolidation efforts are ongoing, for 1.5 year now IIRC
and there are good results. Still it looks like there is one year or so 
needed to be able to build one single image usable on all ARM sub-archs.
I think the progress is good and it all looks very promising, perhaps 
mostly thanks to the Linaro initiative.

> any warns at the -git tree build at the tested archs, but there are
> so many warnings that I think I never saw any such report saying that
> there's no warning.
> 
> Btw, are there anyone really consistently using his reports to fix things?

Yes, I'm often looking at those logs. I find them useful, especially that 
it nearly doesn't happen I build some drivers on architectures other than 
ARM. So it's good to have those build logs.

Some projects, e.g. [2], use build/test systems that allow to track status
after each commit. Not sure if something like this is feasible for whole
media subsystem.

[2] https://chromium-build.appspot.com/p/chromium/console

--
Regards,
Sylwester

Re: [PATCH 0/2] Fix a few more warnings

[Mauro Carvalho Chehab]

Em Mon, 29 Oct 2012 15:12:03 +0100
Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:

> On 10/29/2012 12:32 PM, Mauro Carvalho Chehab wrote:
> > Em Mon, 29 Oct 2012 12:19:32 +0100
> > Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu:
> > 
> >> On 10/29/2012 11:21 AM, Mauro Carvalho Chehab wrote:
> >>> Hans Verkuil yesterday's build still got two warnings at the
> >>> generic drivers:
> >>>         http://hverkuil.home.xs4all.nl/logs/Sunday.log
> >>>
> >>> They didn't appear at i386 build probably because of some
> >>> optimization done there.
> >>>
> >>> Anyway, fixing them are trivial, so let's do it.
> >>>
> >>> After applying those patches, the only drivers left producing
> >>> warnings are the following platform drivers:
> >>>
> >>> drivers/media/platform/davinci/dm355_ccdc.c
> >>> drivers/media/platform/davinci/dm644x_ccdc.c
> >>> drivers/media/platform/davinci/vpbe_osd.c
> >>> drivers/media/platform/omap3isp/ispccdc.c
> >>> drivers/media/platform/omap3isp/isph3a_aewb.c
> >>> drivers/media/platform/omap3isp/isph3a_af.c
> >>> drivers/media/platform/omap3isp/isphist.c
> >>> drivers/media/platform/omap3isp/ispqueue.c
> >>> drivers/media/platform/omap3isp/ispvideo.c
> >>> drivers/media/platform/omap/omap_vout.c
> >>> drivers/media/platform/s5p-fimc/fimc-capture.c
> >>> drivers/media/platform/s5p-fimc/fimc-lite.c
> >>
> >> For these two files I've sent already a pull request [1], which
> >> includes a fixup patch
> >> s5p-fimc: Don't ignore return value of vb2_queue_init()
> >>
> >> BTW, shouldn't things like these be taken care when someone does
> >> a change at the core code ? 
> > 
> > Sure. I remember I saw one patch with s5p on that series[1].
> > Can't remember anymore if it were acked and merged directly, if
> > it was opted to send it via your tree (or maybe that patch was just
> > incomplete, and got unnoticed on that time).
> 
> I think this was one of the first patches from Ezequiel, when he wanted
> to change the vb2_queue_init() function signature so it returns void (as
> there were only BUG_ON()s used inside it). But what we need now at drivers
> is the opposite, i.e. to keep checking the return value and to add where
> such checks are missing. Thus patch [1] is not applicable, since BUG_ONs
> were replaced with WARN_ON and __must_check annotation was added to the
> vb2_queue_init() function declaration.

Ah, ok.

> > [1] https://patchwork.kernel.org/patch/1372871/
> > 
> > It is not easy to enforce those kind of things for platform drivers,
> > as there's not yet a single .config file that could be used to test
> > all arm drivers. Hans automatic builds might be useful, if there weren't
> 
> The ARM arch consolidation efforts are ongoing, for 1.5 year now IIRC
> and there are good results. Still it looks like there is one year or so 
> needed to be able to build one single image usable on all ARM sub-archs.
> I think the progress is good and it all looks very promising, perhaps 
> mostly thanks to the Linaro initiative.

Yeah, when all platform drivers we have can be compiled here, I'll eventually
add it on my test logic. The thing is that changing from one arch to the other
will require doing a make clean, with can be a little painful.
> 
> > any warns at the -git tree build at the tested archs, but there are
> > so many warnings that I think I never saw any such report saying that
> > there's no warning.
> > 
> > Btw, are there anyone really consistently using his reports to fix things?
> 
> Yes, I'm often looking at those logs. I find them useful, especially that 
> it nearly doesn't happen I build some drivers on architectures other than 
> ARM. So it's good to have those build logs.
> 
> Some projects, e.g. [2], use build/test systems that allow to track status
> after each commit. Not sure if something like this is feasible for whole
> media subsystem.
> 
> [2] https://chromium-build.appspot.com/p/chromium/console

The idea is good. The evil is on details. For example, I prefer to not mix
any build setup like that with the main linuxtv site, due to machine's
reliability.

Even running it locally would also likely require two machines, as the multi-arch
compilation will take some time, and several GB of diskspace, as each arch will
need a local working copy of the git tree.

Asynchronous compilation of the kernel, while patches are being added has
some issues: if the build fails, patches need to be reverted, as we don't
want to break git bisect. That would mean that we would need a temporary
"untested" tree, and some logic there that will cherry-pick patches to the
"tested" one when compilation succeeds, or stop and warn maintainer if a
patch breaks. The maintainer will then need to rebase the "untested" tree
which can, in tune, cause troubles at the testing daemon.

Anyway, implementing it would require some time and resources 
that I don't currently have. If anyone could do it, that could be
a nice project.

Regards,
Mauro