Re: [WARNING: A/V UNSCANNABLE][Merge tag 'media/v4.11-1' of git] ff58d005cd: BUG: unable to handle kernel NULL pointer dereference at 0000039c

[Ingo Molnar]

* Linus Torvalds  wrote:

> In other words: what will happen is that distros start getting bootup problem 
> reports six months or a year after we've done it, and *if* they figure out 
> it's 
> the irq enabling, they'll disable it, because they have no way to solve it 
> either.
> 
> And core developers will just maybe see the occasional "4.12 doesn't boot for 
> me" reports, but by then developers will ahve moved on to 4.16 or something.

Yeah, you are right, there's over 2,100 request_irq() calls in the kernel and 
perhaps only 1% of them gets tested on real hardware by the time a change gets 
upstream :-/

So in theory we could require all *new* drivers handle this properly, as new 
drivers tend to come through developers who can fix such bugs - which would at 
least guarantee that with time the problem would obsolete itself.

But I cannot see an easy non-intrusive way to do that - we'd have to rename all 
existing request_irq() calls:

 - We could rename request_irq() to request_irq_legacy() - which does not do 
the 
   tests.

 - The 'new' request_irq() function would do the tests unconditionally.

... and that's just too much churn - unless you think it's worth it, or if 
anyone 
can think of a better method to phase in the new behavior without affecting old 
users.

Another, less intrusive method would be to introduce a new request_irq_shared() 
API call, mark request_irq() obsolete (without putting warnings into the build 
though), and put a check into checkpatch that warns about request_irq() use.

The flip side would be that:

 - request_irq() is such a nice and well-known name to waste

 - plus request_irq_shared() is a misnomer, as this has nothing to do with 
sharing 
   IRQs, it's about getting IRQs in unexpected moments.

I'd rather do the renaming that is easy to automate and the pain is one time 
only.

Or revert the retrigger change and muddle through, although as per Thomas's 
observations spurious interrupts are very common.

Thanks,

Ingo

cron job: media_tree daily build: ERRORS

[Hans Verkuil]

This message is generated daily by a cron job that builds media_tree for
the kernels and architectures in the list below.

Results of the daily build of media_tree:

date:   Tue Feb 28 05:00:21 CET 2017
media-tree git hash:e6b377dbbb944d5e3ceef4e5d429fc5c841e3692
media_build git hash:   785cdf7f0798964681b33aad44fc2ff4d734733d
v4l-utils git hash: 1a5954c991a4ba5483bec6bdee708f25345de025
gcc version:i686-linux-gcc (GCC) 6.2.0
sparse version: v0.5.0-3553-g78b2ea6
smatch version: v0.5.0-3553-g78b2ea6
host hardware:  x86_64
host os:4.9.0-164

linux-git-arm-at91: OK
linux-git-arm-davinci: OK
linux-git-arm-multi: ERRORS
linux-git-arm-pxa: OK
linux-git-blackfin-bf561: OK
linux-git-i686: OK
linux-git-m32r: OK
linux-git-mips: OK
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: OK
linux-2.6.36.4-i686: WARNINGS
linux-2.6.37.6-i686: WARNINGS
linux-2.6.38.8-i686: WARNINGS
linux-2.6.39.4-i686: WARNINGS
linux-3.0.60-i686: WARNINGS
linux-3.1.10-i686: WARNINGS
linux-3.2.37-i686: WARNINGS
linux-3.3.8-i686: WARNINGS
linux-3.4.27-i686: WARNINGS
linux-3.5.7-i686: WARNINGS
linux-3.6.11-i686: WARNINGS
linux-3.7.4-i686: WARNINGS
linux-3.8-i686: WARNINGS
linux-3.9.2-i686: WARNINGS
linux-3.10.1-i686: WARNINGS
linux-3.11.1-i686: WARNINGS
linux-3.12.67-i686: WARNINGS
linux-3.13.11-i686: WARNINGS
linux-3.14.9-i686: WARNINGS
linux-3.15.2-i686: WARNINGS
linux-3.16.7-i686: WARNINGS
linux-3.17.8-i686: WARNINGS
linux-3.18.7-i686: WARNINGS
linux-3.19-i686: WARNINGS
linux-4.0.9-i686: WARNINGS
linux-4.1.33-i686: WARNINGS
linux-4.2.8-i686: WARNINGS
linux-4.3.6-i686: WARNINGS
linux-4.4.22-i686: WARNINGS
linux-4.5.7-i686: WARNINGS
linux-4.6.7-i686: WARNINGS
linux-4.7.5-i686: WARNINGS
linux-4.8-i686: OK
linux-4.9-i686: OK
linux-4.10-rc3-i686: OK
linux-2.6.36.4-x86_64: WARNINGS
linux-2.6.37.6-x86_64: WARNINGS
linux-2.6.38.8-x86_64: WARNINGS
linux-2.6.39.4-x86_64: WARNINGS
linux-3.0.60-x86_64: WARNINGS
linux-3.1.10-x86_64: WARNINGS
linux-3.2.37-x86_64: WARNINGS
linux-3.3.8-x86_64: WARNINGS
linux-3.4.27-x86_64: WARNINGS
linux-3.5.7-x86_64: WARNINGS
linux-3.6.11-x86_64: WARNINGS
linux-3.7.4-x86_64: WARNINGS
linux-3.8-x86_64: WARNINGS
linux-3.9.2-x86_64: WARNINGS
linux-3.10.1-x86_64: WARNINGS
linux-3.11.1-x86_64: WARNINGS
linux-3.12.67-x86_64: WARNINGS
linux-3.13.11-x86_64: WARNINGS
linux-3.14.9-x86_64: WARNINGS
linux-3.15.2-x86_64: WARNINGS
linux-3.16.7-x86_64: WARNINGS
linux-3.17.8-x86_64: WARNINGS
linux-3.18.7-x86_64: WARNINGS
linux-3.19-x86_64: WARNINGS
linux-4.0.9-x86_64: WARNINGS
linux-4.1.33-x86_64: WARNINGS
linux-4.2.8-x86_64: WARNINGS
linux-4.3.6-x86_64: WARNINGS
linux-4.4.22-x86_64: WARNINGS
linux-4.5.7-x86_64: WARNINGS
linux-4.6.7-x86_64: WARNINGS
linux-4.7.5-x86_64: WARNINGS
linux-4.8-x86_64: OK
linux-4.9-x86_64: OK
linux-4.10-rc3-x86_64: OK
apps: WARNINGS
spec-git: OK
sparse: WARNINGS

Detailed results are available here:

http://www.xs4all.nl/~hverkuil/logs/Tuesday.log

Full logs are available here:

http://www.xs4all.nl/~hverkuil/logs/Tuesday.tar.bz2

The Media Infrastructure API from this daily build is here:

http://www.xs4all.nl/~hverkuil/spec/index.html

Re: Kaffeine commit b510bff2 won't compile

[bill murphy]

Hi Mauro,

Thanks for looking in to it. All is well now.

On a sidenote, given 700 MHz is used for LTE, and not broadcasting

anymore, would you folks consider removing ch 52 thru 69

in the us-atsc-frequencies if I posted a simple patch to dtv-scan-tables?


Bill

On 02/27/2017 05:11 AM, Mauro Carvalho Chehab wrote:

Em Sun, 26 Feb 2017 20:57:20 -0500
bill murphy  escreveu:


Hi,
Can someone double check me on this?

It seems there might be a missing header,
in the src directory, preventing the last commit from
compiling. The commit prior compiles fine. So not that big a deal, just
letting folks know what I ran in to.

I don't see this file, 'log.h', anywhere in the src directory. Guessing
it wasn't 'added' for tracking?

git://anongit.kde.org/kaffeine

diff between master and previous commit...just a snippet, as other files
are including the same missing header.

diff --git a/src/dvb/dvbcam_linux.cpp b/src/dvb/dvbcam_linux.cpp
index ceb9dbd..5c9c575 100644
--- a/src/dvb/dvbcam_linux.cpp
+++ b/src/dvb/dvbcam_linux.cpp
@@ -18,11 +18,7 @@
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/

-#include 
-#include 
-#if QT_VERSION < 0x050500
-# define qInfo qDebug
-#endif
+#include "../log.h"

   #include 
   #include 

where compile complains of that missing header...

Scanning dependencies of target kaffeine
[ 20%] Building CXX object
src/CMakeFiles/kaffeine.dir/dvb/dvbcam_linux.cpp.o
/home/user/src2/kaffeine/src/dvb/dvbcam_linux.cpp:21:20: fatal error:
../log.h: No such file or directory
compilation terminated.

Thanks for complaining about it! I forgot to add src/log.h on the
commit.

You should be able to compile it now.

Thanks,
Mauro

Re: [PATCH] [media] tw5864: handle unknown video std gracefully

[Andrey Utkin]

Hi Arnd,

Thanks for sending this patch.

On Mon, Feb 27, 2017 at 09:32:34PM +0100, Arnd Bergmann wrote:
> tw5864_frameinterval_get() only initializes its output when it successfully
> identifies the video standard in tw5864_input. We get a warning here because
> gcc can't always track the state if initialized warnings across a WARN()
> macro, and thinks it might get used incorrectly in tw5864_s_parm:
> 
> media/pci/tw5864/tw5864-video.c: In function 'tw5864_s_parm':
> media/pci/tw5864/tw5864-video.c:816:38: error: 'time_base.numerator' may be 
> used uninitialized in this function [-Werror=maybe-uninitialized]
> media/pci/tw5864/tw5864-video.c:819:31: error: 'time_base.denominator' may be 
> used uninitialized in this function [-Werror=maybe-uninitialized]

I think behaviour of tw5864_frameinterval_get() is ok.
I don't see how WARN() could affect gcc state tracking. There's "return
-EINVAL" right after WARN() which lets caller handle the failure case
gracefully. Maybe I just don't see how confusing WARN() can be for gcc
in this situation, but it's not as confusing as BUG() would be, right?

I see the reason of that warning is 

 - time_base being not initialized in tw5864_s_parm()
 - gcc being too dumb to recognize that we have checked the retcode in
   tw5864_s_parm() and proceed only when we are sure we have correctly
   initialized time_base.

Is that you compiling with manually added -Werror=maybe-uninitialized or
is that default compilation flags? I don't remember encountering that
and I doubt a lot of kernel code compiles without warnings with such
flag.

Also, which GCC version are you using?

> This particular use happens to be ok, but we do copy the uninitialized
> output of tw5864_frameinterval_get() into other memory without checking
> the return code, interestingly without getting a warning here.

Retcode checking takes place everywhere, but currently it overwrites
supplied structs with potentially-uninitialized values. To make it
cleaner, it should be (e.g. tw5864_g_parm())

ret = tw5864_frameinterval_get(input, >timeperframe);
if (ret)
return ret;
cp->timeperframe.numerator *= input->frame_interval;
cp->capturemode = 0;
cp->readbuffers = 2;
return 0;

and not

ret = tw5864_frameinterval_get(input, >timeperframe);
cp->timeperframe.numerator *= input->frame_interval;
cp->capturemode = 0;
cp->readbuffers = 2;
return ret;

That would resolve your concerns of uninitialized values propagation
without writing bogus values 1/1 in case of failure. I think I'd
personally prefer a called function to leave my data structs intact when
it fails.

> 
> This initializes the output to 1/1s for the case, to make sure we do
> get an intialization that doesn't cause a division-by-zero exception
> in case we end up using this uninitialized data later.

Personally I won't object against such patch, but I find it a bit too
much "defensive" for kernel coding taste.

Making sure somebody who doesn't check return codes don't get a crash is
traditionally not considered a valid concern AFAIK.

Please let me know what you think about this.

Re: [PATCH] bcm2048: Fix checkpatch checks

[Man Choy]

On Mon, Feb 27, 2017 at 4:21 PM, Greg Kroah-Hartman
 wrote:
> On Sat, Feb 18, 2017 at 11:52:37AM +0800, Man Choy wrote:
>> Fix following checks:
>>
>> CHECK: Avoid crashing the kernel - try using WARN_ON & recovery code rather 
>> than BUG() or BUG_ON()
>> +   BUG_ON((index+2) >= BCM2048_MAX_RDS_RT);
>>
>> CHECK: spaces preferred around that '+' (ctx:VxV)
>> +   BUG_ON((index+2) >= BCM2048_MAX_RDS_RT);
>>  ^
>>
>> CHECK: Avoid crashing the kernel - try using WARN_ON & recovery code rather 
>> than BUG() or BUG_ON()
>> +   BUG_ON((index+4) >= BCM2048_MAX_RDS_RT);
>>
>> CHECK: spaces preferred around that '+' (ctx:VxV)
>> +   BUG_ON((index+4) >= BCM2048_MAX_RDS_RT);
>>  ^
>> ---
>>  drivers/staging/media/bcm2048/radio-bcm2048.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/bcm2048/radio-bcm2048.c 
>> b/drivers/staging/media/bcm2048/radio-bcm2048.c
>> index 37bd439..d5ee279 100644
>> --- a/drivers/staging/media/bcm2048/radio-bcm2048.c
>> +++ b/drivers/staging/media/bcm2048/radio-bcm2048.c
>> @@ -1534,7 +1534,7 @@ static int bcm2048_parse_rt_match_c(struct 
>> bcm2048_device *bdev, int i,
>>   if (crc == BCM2048_RDS_CRC_UNRECOVARABLE)
>>   return 0;
>>
>> - BUG_ON((index+2) >= BCM2048_MAX_RDS_RT);
>> + WARN_ON((index + 2) >= BCM2048_MAX_RDS_RT);
>
> Ick, no to all of these!  What happens if this is true, the code will
> crash, right?  You have to properly recover from this, don't just throw
> the message out to userspace and then keep on going.
>
> You can't just do a search/replace for this, otherwise it would have
> been done already :)
>
> thanks,
>
> greg k-h

Okay, noted. Thanks.

Re: [PATCH 14/15] media: s5p-mfc: Use preallocated block allocator always for MFC v6+

[Shuah Khan]

On 02/27/2017 05:50 AM, Marek Szyprowski wrote:
> Hi Shuah,
> 
> On 2017-02-24 15:23, Shuah Khan wrote:
>> On Thu, Feb 23, 2017 at 11:26 PM, Marek Szyprowski
>>  wrote:
>>> On 2017-02-23 22:43, Shuah Khan wrote:
 On Tue, Feb 14, 2017 at 12:52 AM, Marek Szyprowski
  wrote:
> It turned out that all versions of MFC v6+ hardware doesn't have a strict
> requirement for ALL buffers to be allocated on higher addresses than the
> firmware base like it was documented for MFC v5. This requirement is true
> only for the device and per-context buffers. All video data buffers can
> be
> allocated anywhere for all MFC v6+ versions. Basing on this fact, the
> special DMA configuration based on two reserved memory regions is not
> really needed for MFC v6+ devices, because the memory requirements for
> the
> firmware, device and per-context buffers can be fulfilled by the simple
> probe-time pre-allocated block allocator instroduced in previous patch.
>
> This patch enables support for such pre-allocated block based allocator
> always for MFC v6+ devices. Due to the limitations of the memory
> management
> subsystem the largest supported size of the pre-allocated buffer when no
> CMA (Contiguous Memory Allocator) is enabled is 4MiB.
>
> This patch also removes the requirement to provide two reserved memory
> regions for MFC v6+ devices in device tree. Now the driver is fully
> functional without them.
>
> Signed-off-by: Marek Szyprowski 
 Hi Marek,

 This patch breaks display manager. exynos_drm_gem_create() isn't happy.
 dmesg and console are flooded with

 odroid login: [  209.170566] [drm:exynos_drm_gem_create] *ERROR* failed to
 allo.
 [  212.173222] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  215.354790] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  218.736464] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  221.837128] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  226.284827] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  229.242498] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  232.063150] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  235.73] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  239.472061] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  242.567465] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  246.500541] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  249.996018] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  253.837272] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  257.048782] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  260.084819] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  263.448611] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  266.271074] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  269.011558] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  272.039066] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  275.404938] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  278.339033] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  281.274751] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  284.641202] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  287.461039] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  291.062011] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  294.746870] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.
 [  298.246570] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
 buffer.

 I don't think this is an acceptable behavior. It is a regression.
>>>
>>> This is a really poor bug report... Could you elaborate a bit how to
>>> reproduce
>>> this? Could you provide your kernel config and information about test
>>> environment?
>> Yeah. I should have give you more information. My bad.
>>
>>> I suspect that you use CMA without IOMMU and you have too small global CMA
>>> region.
>> Yes. I have CMA and using exynos_defconfig. Nothing fancy. I think
>> what's happening is s5p_mfc pre-allocates and there is nothing left
>> when disaply manager starts requestuing gem buffers. This failure
>> happens when systemd kicks off lightdm.
>>
>>> After this patch MFC driver uses global CMA region instead of the MFC's
>>> private
>>> ones, 

Re: [WARNING: A/V UNSCANNABLE][Merge tag 'media/v4.11-1' of git] ff58d005cd: BUG: unable to handle kernel NULL pointer dereference at 0000039c

[Linus Torvalds]

On Mon, Feb 27, 2017 at 7:41 AM, Ingo Molnar  wrote:
>
> BTW., instead of trying to avoid the scenario, wow about moving in the other
> direction: making CONFIG_DEBUG_SHIRQ=y unconditional property in the IRQ core 
> code
> starting from v4.12 or so

The problem is that it's generally almost undebuggable ahead of time
by developers, and most users won't be able to do good reports either,
because the symptom is geberally a boot-time crash, often with no
logs.

So this option is *not* good for actual users. It's been tried before.

It's a wonderful thing for developers to run with to make sure the
drivers they are working on are resilient to this problem, but we have
too many legacy drivers and lots of random users, and it's unrealistic
to expect them to handle it.

In other words: what will happen is that distros start getting bootup
problem reports six months or a year after we've done it, and *if*
they figure out it's the irq enabling, they'll disable it, because
they have no way to solve it either.

And core developers will just maybe see the occasional "4.12 doesn't
boot for me" reports, but by then developers will ahve moved on to
4.16 or something.

So I don't disagree that in a perfect world all drivers should just
handle it. It's just that it's not realistic.

The fact that we have now *twice* gotten an oops report or a "this
machine doesn't boot" report etc within a week or so of merging the
problematic patch does *not* indicate that it's easy to fix or rare.

Quite the reverse.

It indicates that it's just rare enough that core developers don't see
it, but it's common enough to have triggered issues in random places.

And it will just get *much* worse when you then get the random
end-users that usually have older machines than the developers who
actually test daily development -git trees.

Then we'll just hit *other* random places, and without having testers
that are competent and willing or able to bisect or debug.

  Linus

em28xx: new board id [1d19:6901]

[Łukasz Strzeszkowski]

Hi,

I’ve found a new device which is not listed

model: LogiLink VG0011
vendor/product: [1d19:6901] Dexatek Technology Ltd.

mode: analog

I am unable to load a driver, because there is no such vendor in driver list.

dmesg output:
[ 1232.506295] usb 2-4: new high-speed USB device number 4 using xhci_hcd
[ 1232.637496] usb 2-4: New USB device found, idVendor=1d19, idProduct=6901
[ 1232.637500] usb 2-4: New USB device strings: Mfr=0, Product=1, SerialNumber=0
[ 1232.637502] usb 2-4: Product: USB 2861 Video
[ 1232.660061] usbcore: registered new interface driver snd-usb-audio


Regards
Łukasz Strzeszkowski

Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Sakari Ailus]

Hi Pavel,

Please find my comments below.

On Sat, Feb 25, 2017 at 11:12:55PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > On Mon 2017-02-20 15:56:36, Sakari Ailus wrote:
> > > > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> > > > > I've tested ACPI, will test DT soon...
> > > > 
> > > > DT case works, too (Nokia N9).
> > > 
> > > Hmm. Good to know. Now to figure out how to get N900 case to work...
> > > 
> > > AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes
> > > seem to be in, so I'll need to figure out which, and will still need
> > > omap3isp modifications...
> > 
> > Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices.
> > 
> > It's essentially the functionality in the four patches. The data-lane and
> > clock-name properties have been renamed as data-lanes and clock-lanes (i.e.
> > plural) to match the property documentation.
> 
> Yes, it seems to work.
> 
> Here's a patch. It has checkpatch issues, I can fix them.  More
> support is needed on the ispcsiphy.c side... Could you take (fixed)
> version of this to your fwnode branch?
> 
> Thanks,
>   Pavel
> 
> 
> 
> 
> ---
> 
> omap3isp: add support for CSI1 bus
> 
> Signed-off-by: Pavel Machek 
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c 
> b/drivers/media/platform/omap3isp/isp.c
> index 245225a..4b10cfe 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2032,6 +2034,7 @@ static int isp_fwnode_parse(struct device *dev, struct 
> fwnode_handle *fwn,
>   struct v4l2_fwnode_endpoint vfwn;
>   unsigned int i;
>   int ret;
> + int csi1 = 0;
>  
>   ret = v4l2_fwnode_endpoint_parse(fwn, );
>   if (ret)
> @@ -2059,38 +2062,82 @@ static int isp_fwnode_parse(struct device *dev, 
> struct fwnode_handle *fwn,
>  
>   case ISP_OF_PHY_CSIPHY1:
>   case ISP_OF_PHY_CSIPHY2:
> - /* FIXME: always assume CSI-2 for now. */
> + switch (vfwn.bus_type) {
> + case V4L2_MBUS_CSI2:
> + dev_dbg(dev, "csi2 configuration\n");
> + csi1 = 0;
> + break;
> + case V4L2_MBUS_CCP2:
> + case V4L2_MBUS_CSI1:
> + dev_dbg(dev, "csi1 configuration\n");
> + csi1 = 1;
> + break;
> + default:
> + dev_err(dev, "unkonwn bus type\n");
> + }
> +
>   switch (vfwn.base.port) {
>   case ISP_OF_PHY_CSIPHY1:
> - buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> + if (csi1)

You could compare vfwn.bus_type == V4L2_MBUS_CSI2 for this. But if you
choose the local variable, please make it bool instead.

> + buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
> + else
> + buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
>   break;
>   case ISP_OF_PHY_CSIPHY2:
> - buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> + if (csi1)
> + buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
> + else
> + buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
>   break;
> + default:
> + dev_err(dev, "bad port\n");
>   }
> - buscfg->bus.csi2.lanecfg.clk.pos = 
> vfwn.bus.mipi_csi2.clock_lane;
> - buscfg->bus.csi2.lanecfg.clk.pol =
> - vfwn.bus.mipi_csi2.lane_polarities[0];
> - dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> - buscfg->bus.csi2.lanecfg.clk.pol,
> - buscfg->bus.csi2.lanecfg.clk.pos);
> -
> - for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
> - buscfg->bus.csi2.lanecfg.data[i].pos =
> - vfwn.bus.mipi_csi2.data_lanes[i];
> - buscfg->bus.csi2.lanecfg.data[i].pol =
> - vfwn.bus.mipi_csi2.lane_polarities[i + 1];
> + if (csi1) {
> + buscfg->bus.ccp2.lanecfg.clk.pos = 
> vfwn.bus.mipi_csi1.clock_lane;

Wrap after "="?

> + buscfg->bus.ccp2.lanecfg.clk.pol =
> + vfwn.bus.mipi_csi1.lane_polarity[0];
> + dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> + buscfg->bus.ccp2.lanecfg.clk.pol,
> + buscfg->bus.ccp2.lanecfg.clk.pos);
> +
> + buscfg->bus.ccp2.lanecfg.data[0].pos = 1;

Shouldn't this be vfwn.bus.mipi_csi1.data_lane ?

> + buscfg->bus.ccp2.lanecfg.data[0].pol = 0;

And this one is vfwn.bus.mipi_csi1.lane_polarity[1] .

> +
>   

[PATCH] [media] tw5864: handle unknown video std gracefully

[Arnd Bergmann]

tw5864_frameinterval_get() only initializes its output when it successfully
identifies the video standard in tw5864_input. We get a warning here because
gcc can't always track the state if initialized warnings across a WARN()
macro, and thinks it might get used incorrectly in tw5864_s_parm:

media/pci/tw5864/tw5864-video.c: In function 'tw5864_s_parm':
media/pci/tw5864/tw5864-video.c:816:38: error: 'time_base.numerator' may be 
used uninitialized in this function [-Werror=maybe-uninitialized]
media/pci/tw5864/tw5864-video.c:819:31: error: 'time_base.denominator' may be 
used uninitialized in this function [-Werror=maybe-uninitialized]

This particular use happens to be ok, but we do copy the uninitialized
output of tw5864_frameinterval_get() into other memory without checking
the return code, interestingly without getting a warning here.

This initializes the output to 1/1s for the case, to make sure we do
get an intialization that doesn't cause a division-by-zero exception
in case we end up using this uninitialized data later.

This also avoids the warning.

Signed-off-by: Arnd Bergmann 
---
 drivers/media/pci/tw5864/tw5864-video.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/pci/tw5864/tw5864-video.c 
b/drivers/media/pci/tw5864/tw5864-video.c
index 9421216bb942..a451c2081fde 100644
--- a/drivers/media/pci/tw5864/tw5864-video.c
+++ b/drivers/media/pci/tw5864/tw5864-video.c
@@ -728,6 +728,8 @@ static int tw5864_frameinterval_get(struct tw5864_input 
*input,
frameinterval->denominator = 25;
break;
default:
+   frameinterval->numerator = 1;
+   frameinterval->denominator = 1;
WARN(1, "tw5864_frameinterval_get requested for unknown std 
%d\n",
 input->std);
return -EINVAL;
-- 
2.9.0

Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Pavel Machek]

Hi!

> > > On Mon 2017-02-20 15:56:36, Sakari Ailus wrote:
> > > > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> > > > > I've tested ACPI, will test DT soon...
> > > > 
> > > > DT case works, too (Nokia N9).
> > > 
> > > Hmm. Good to know. Now to figure out how to get N900 case to work...
> > > 
> > > AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes
> > > seem to be in, so I'll need to figure out which, and will still need
> > > omap3isp modifications...
> > 
> > Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices.
> > 
> > It's essentially the functionality in the four patches. The data-lane and
> > clock-name properties have been renamed as data-lanes and clock-lanes (i.e.
> > plural) to match the property documentation.
> 
> Yes, it seems to work.
> 
> Here's a patch. It has checkpatch issues, I can fix them.  More
> support is needed on the ispcsiphy.c side... Could you take (fixed)
> version of this to your fwnode branch?

Any feedback would be welcome :-)
Pavel


> omap3isp: add support for CSI1 bus
> 
> Signed-off-by: Pavel Machek 
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c 
> b/drivers/media/platform/omap3isp/isp.c
> index 245225a..4b10cfe 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2032,6 +2034,7 @@ static int isp_fwnode_parse(struct device *dev, struct 
> fwnode_handle *fwn,
>   struct v4l2_fwnode_endpoint vfwn;
>   unsigned int i;
>   int ret;
> + int csi1 = 0;
>  
>   ret = v4l2_fwnode_endpoint_parse(fwn, );
>   if (ret)
> @@ -2059,38 +2062,82 @@ static int isp_fwnode_parse(struct device *dev, 
> struct fwnode_handle *fwn,
>  
>   case ISP_OF_PHY_CSIPHY1:
>   case ISP_OF_PHY_CSIPHY2:
> - /* FIXME: always assume CSI-2 for now. */
> + switch (vfwn.bus_type) {
> + case V4L2_MBUS_CSI2:
> + dev_dbg(dev, "csi2 configuration\n");
> + csi1 = 0;
> + break;
> + case V4L2_MBUS_CCP2:
> + case V4L2_MBUS_CSI1:
> + dev_dbg(dev, "csi1 configuration\n");
> + csi1 = 1;
> + break;
> + default:
> + dev_err(dev, "unkonwn bus type\n");
> + }
> +
>   switch (vfwn.base.port) {
>   case ISP_OF_PHY_CSIPHY1:
> - buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> + if (csi1)
> + buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
> + else
> + buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
>   break;
>   case ISP_OF_PHY_CSIPHY2:
> - buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> + if (csi1)
> + buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
> + else
> + buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
>   break;
> + default:
> + dev_err(dev, "bad port\n");
>   }
> - buscfg->bus.csi2.lanecfg.clk.pos = 
> vfwn.bus.mipi_csi2.clock_lane;
> - buscfg->bus.csi2.lanecfg.clk.pol =
> - vfwn.bus.mipi_csi2.lane_polarities[0];
> - dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> - buscfg->bus.csi2.lanecfg.clk.pol,
> - buscfg->bus.csi2.lanecfg.clk.pos);
> -
> - for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
> - buscfg->bus.csi2.lanecfg.data[i].pos =
> - vfwn.bus.mipi_csi2.data_lanes[i];
> - buscfg->bus.csi2.lanecfg.data[i].pol =
> - vfwn.bus.mipi_csi2.lane_polarities[i + 1];
> + if (csi1) {
> + buscfg->bus.ccp2.lanecfg.clk.pos = 
> vfwn.bus.mipi_csi1.clock_lane;
> + buscfg->bus.ccp2.lanecfg.clk.pol =
> + vfwn.bus.mipi_csi1.lane_polarity[0];
> + dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> + buscfg->bus.ccp2.lanecfg.clk.pol,
> + buscfg->bus.ccp2.lanecfg.clk.pos);
> +
> + buscfg->bus.ccp2.lanecfg.data[0].pos = 1;
> + buscfg->bus.ccp2.lanecfg.data[0].pol = 0;
> +
>   dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> - buscfg->bus.csi2.lanecfg.data[i].pol,
> - buscfg->bus.csi2.lanecfg.data[i].pos);
> + buscfg->bus.ccp2.lanecfg.data[0].pol,
> + buscfg->bus.ccp2.lanecfg.data[0].pos);
> +
> + 

Re: [PATCHv4 1/9] video: add hotplug detect notifier support

[Russell King - ARM Linux]

On Mon, Feb 27, 2017 at 06:21:05PM +0100, Hans Verkuil wrote:
> On 02/27/2017 06:04 PM, Russell King - ARM Linux wrote:
> > I'm afraid that I walked away from this after it became clear that there
> > was little hope for any forward progress being made in a timely manner
> > for multiple reasons (mainly the core CEC code being out of mainline.)
> 
> In case you missed it: the core CEC code was moved out of staging and into
> mainline in 4.10.

I was aware (even though I've not been publishing anything, I do keep
dw-hdmi-cec and tda9950/tda998x up to date with every final kernel
release.)

> > If you can think of a better approach, then I'm sure there's lots of
> > people who'd be willing to do the coding for it... if not, I'm not
> > sure where we go from here (apart from keeping code in private/vendor
> > trees.)
> 
> For CEC there are just two things that it needs: the physical address
> (contained in the EDID) and it needs to be informed when the EDID disappears
> (typically due to a disconnect), in which case the physical address
> becomes invalid (f.f.f.f).

Yep.  CEC really only needs to know "have new phys address" and
"disconnect" provided that CEC drivers understand that they may receive
a new phys address with no intervening disconnect.  (Consider the case
where EDID changes, but the HDMI driver failed to spot the HPD signal
pulse - unfortunately, there's hardware out there where HPD is next to
useless.)

> Russell, do you have pending code that needs the ELD support in the
> notifier?  CEC doesn't need it, so from my perspective it can be
> dropped in the first version.

I was looking for that while writing the previous mail, and I think
it's time to drop it - I had thought dw-hdmi-*audio would use it, or
the ASoC people, but it's still got no users, so I think it's time
to drop it.

I have seen some patch sets go by which are making use of the notifier,
but I haven't paid close attention to how they're using it or what
they're using it for... as I sort of implied in my previous mail, I
had lost interest in mainline wrt CEC stuff due to the glacial rate
of progress.  (That's not a criticism.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCHv4 1/9] video: add hotplug detect notifier support

[Hans Verkuil]

On 02/27/2017 06:04 PM, Russell King - ARM Linux wrote:
> On Mon, Feb 27, 2017 at 05:08:41PM +0100, Daniel Vetter wrote:
>> On Mon, Feb 06, 2017 at 11:29:43AM +0100, Hans Verkuil wrote:
>>> From: Hans Verkuil 
>>>
>>> Add support for video hotplug detect and EDID/ELD notifiers, which is used
>>> to convey information from video drivers to their CEC and audio 
>>> counterparts.
>>>
>>> Based on an earlier version from Russell King:
>>>
>>> https://patchwork.kernel.org/patch/9277043/
>>>
>>> The hpd_notifier is a reference counted object containing the HPD/EDID/ELD 
>>> state
>>> of a video device.
>>>
>>> When a new notifier is registered the current state will be reported to
>>> that notifier at registration time.
>>>
>>> Signed-off-by: Hans Verkuil 
>>> Tested-by: Marek Szyprowski 
>>
>> So I'm super late to the party because I kinda ignored all things CEC thus
>> far. Ḯ'm not sure this is a great design, with two main concerns:
> 
> I'm afraid that I walked away from this after it became clear that there
> was little hope for any forward progress being made in a timely manner
> for multiple reasons (mainly the core CEC code being out of mainline.)

In case you missed it: the core CEC code was moved out of staging and into
mainline in 4.10.

> 
> The original notifier was created in August 2015, before there was any
> "hdmi codec" support or anything of the like.  At some point (I'm not
> sure when) Philipp gave his ack on it, and I definitely know it was
> subsequently posted for RFC in August 2016.  We're now 1.5 years after
> its creation, 7 months after it was definitely publically posted to
> dri-devel, and you've only just said that you don't like the approach...
> 
> Anyway, the hdmi-codec header you point at is only relevant when you
> have a driver using ASoC and you have the codec part tightly integrated
> with your HDMI interface.  That generally works fine there, because
> generally they are on the same device, and are very dependent (due to
> the need to know the HDMI bus clock.)
> 
> The same is not true of CEC though - for example, the TDA998x is
> actually two devices - the HDMI bridge, and an entirely separate
> TDA9950 CEC device.  They may be in the same package, but the TDA9950
> was available as an entirely separate device.  The reason that is the
> case is because they are entirely separate entities as far as
> functionality goes: nothing on the CEC communication side electrically
> depends on the HDMI bus itself.  The only common thing in common is
> the connector.
> 
> From the protocol point of view, CEC requires the "physical address"
> of a device, and that is part of the EDID information from the HDMI
> device - so CEC needs to have access to the EDID.  CEC also needs to
> know when if/when the EDID information is updated, or when connection/
> disconnection events occur so that it can re-negotiate its "logical
> address", and update for any physical address changes.
> 
> For example, if you have a CEC device connected to an AV receiver,
> which is in turn connected to a TV, and the TV is powered down but
> the AV receiver is powered up, then the AV receiver will give all
> devices connected to it a physical address to the best of its
> knowledge.  Turn the TV on, and the physical address will change
> (especially so if the AV receiver has been moved between different
> inputs on the TV.)
> 
> This all needs the HDMI driver to _notify_ the CEC part of these state
> changes - you can't get away from the need to _notify_ these events.
> 
> So, what we need is:
> 
> (a) some way for CEC to be _notified_ of all HPD change events
> (b) some way for CEC to query the EDID in a race free manner w.r.t. HPD
> 
> (a) pretty much involves some kind of notification system.  It doesn't
> matter whether it's a real notifier, or a struct of function pointers,
> the effect is going to be the same no matter what - the basic requirement
> is that we run some code in the CEC side when a HPD state change occurs.
> Given that, what you seem to be objecting to (wrt locking on this) is
> against the fundamental requirement that CEC needs to track the HPD
> state.
> 
> (b) can be done in other ways, but I'd suggest reversing the design (iow,
> having CEC explicitly query the HDMI part for the current EDID) is more
> racy than having the HDMI part notify CEC - you have the situation where
> CEC could be querying the EDID on one CPU while HDMI on another CPU is
> saying that the HPD changed state.
> 
> The query approach also carries with it a whole new set of locking issues,
> because we can get into this situation:
> 
>  HDMI  CEC
>   --- HPD insert --->
>   <--- EDID read 
> 
> The problem then is that if HDMI holds a lock while sending the HPD insert
> message, and it tries to take the same lock when supplying the EDID back
> to CEC, you have an immediate deadlock.
> 
> So, given that HDMI needs to notify CEC 

Re: [PATCHv4 1/9] video: add hotplug detect notifier support

[Russell King - ARM Linux]

On Mon, Feb 27, 2017 at 05:08:41PM +0100, Daniel Vetter wrote:
> On Mon, Feb 06, 2017 at 11:29:43AM +0100, Hans Verkuil wrote:
> > From: Hans Verkuil 
> > 
> > Add support for video hotplug detect and EDID/ELD notifiers, which is used
> > to convey information from video drivers to their CEC and audio 
> > counterparts.
> > 
> > Based on an earlier version from Russell King:
> > 
> > https://patchwork.kernel.org/patch/9277043/
> > 
> > The hpd_notifier is a reference counted object containing the HPD/EDID/ELD 
> > state
> > of a video device.
> > 
> > When a new notifier is registered the current state will be reported to
> > that notifier at registration time.
> > 
> > Signed-off-by: Hans Verkuil 
> > Tested-by: Marek Szyprowski 
> 
> So I'm super late to the party because I kinda ignored all things CEC thus
> far. Ḯ'm not sure this is a great design, with two main concerns:

I'm afraid that I walked away from this after it became clear that there
was little hope for any forward progress being made in a timely manner
for multiple reasons (mainly the core CEC code being out of mainline.)

The original notifier was created in August 2015, before there was any
"hdmi codec" support or anything of the like.  At some point (I'm not
sure when) Philipp gave his ack on it, and I definitely know it was
subsequently posted for RFC in August 2016.  We're now 1.5 years after
its creation, 7 months after it was definitely publically posted to
dri-devel, and you've only just said that you don't like the approach...

Anyway, the hdmi-codec header you point at is only relevant when you
have a driver using ASoC and you have the codec part tightly integrated
with your HDMI interface.  That generally works fine there, because
generally they are on the same device, and are very dependent (due to
the need to know the HDMI bus clock.)

The same is not true of CEC though - for example, the TDA998x is
actually two devices - the HDMI bridge, and an entirely separate
TDA9950 CEC device.  They may be in the same package, but the TDA9950
was available as an entirely separate device.  The reason that is the
case is because they are entirely separate entities as far as
functionality goes: nothing on the CEC communication side electrically
depends on the HDMI bus itself.  The only common thing in common is
the connector.

>From the protocol point of view, CEC requires the "physical address"
of a device, and that is part of the EDID information from the HDMI
device - so CEC needs to have access to the EDID.  CEC also needs to
know when if/when the EDID information is updated, or when connection/
disconnection events occur so that it can re-negotiate its "logical
address", and update for any physical address changes.

For example, if you have a CEC device connected to an AV receiver,
which is in turn connected to a TV, and the TV is powered down but
the AV receiver is powered up, then the AV receiver will give all
devices connected to it a physical address to the best of its
knowledge.  Turn the TV on, and the physical address will change
(especially so if the AV receiver has been moved between different
inputs on the TV.)

This all needs the HDMI driver to _notify_ the CEC part of these state
changes - you can't get away from the need to _notify_ these events.

So, what we need is:

(a) some way for CEC to be _notified_ of all HPD change events
(b) some way for CEC to query the EDID in a race free manner w.r.t. HPD

(a) pretty much involves some kind of notification system.  It doesn't
matter whether it's a real notifier, or a struct of function pointers,
the effect is going to be the same no matter what - the basic requirement
is that we run some code in the CEC side when a HPD state change occurs.
Given that, what you seem to be objecting to (wrt locking on this) is
against the fundamental requirement that CEC needs to track the HPD
state.

(b) can be done in other ways, but I'd suggest reversing the design (iow,
having CEC explicitly query the HDMI part for the current EDID) is more
racy than having the HDMI part notify CEC - you have the situation where
CEC could be querying the EDID on one CPU while HDMI on another CPU is
saying that the HPD changed state.

The query approach also carries with it a whole new set of locking issues,
because we can get into this situation:

 HDMI  CEC
  --- HPD insert --->
  <--- EDID read 

The problem then is that if HDMI holds a lock while sending the HPD insert
message, and it tries to take the same lock when supplying the EDID back
to CEC, you have an immediate deadlock.

So, given that HDMI needs to notify CEC about HPD changes, it also makes
sense to keep the overall flow of data the same for everything - avoid
back-queries, and have HDMI notify CEC of the new EDID.

It also avoids the problem where we may see HPD assert, but it may take
some time for the EDID to become available from HDMI (eg, in the 

Re: [WARNING: A/V UNSCANNABLE][Merge tag 'media/v4.11-1' of git] ff58d005cd: BUG: unable to handle kernel NULL pointer dereference at 0000039c

[Thomas Gleixner]

On Mon, 27 Feb 2017, Tony Lindgren wrote:
> * Ingo Molnar  [170227 07:44]:
> > Because it's not the requirement that hurts primarily, but the resulting 
> > non-determinism and the sporadic crashes. Which can be solved by making the 
> > race 
> > deterministic via the debug facility.
> > 
> > If the IRQ handler crashed the moment it was first written by the driver 
> > author 
> > we'd never see these problems.
> 
> Just in case this is PM related.. Maybe the spurious interrupt is pending
> from earlier? This could be caused by glitches on the lines with runtime PM,
> or a pending interrupt during suspend/resume. In that case IRQ_DISABLE_UNLAZY
> might provide more clues if the problem goes away.

It's not PM related.  That's just silly hardware. At the moment when you
enable some magic bit in the control register, which is required to probe
the version, the fricking thing spits out a spurious interrupt despite the
interrupt enable bit in the same control register being still disabled. Of
course we cannot install an interrupt handler before having probed the
version and setup other stuff, except we add magic 'if (!initialized)'
crappola into the handler and lose the ability to install version dependent
handlers afterwards.

Wonderful crap that, isn't it?

Thanks,

tglx

Re: [WARNING: A/V UNSCANNABLE][Merge tag 'media/v4.11-1' of git] ff58d005cd: BUG: unable to handle kernel NULL pointer dereference at 0000039c

[Thomas Gleixner]

On Mon, 27 Feb 2017, Ingo Molnar wrote:
> * Thomas Gleixner  wrote:
> 
> > The pending interrupt issue happens, at least on my test boxen, mostly on
> > the 'legacy' interrupts (0 - 15). But even the IOAPIC interrupts >=16
> > happen occasionally.
> >
> > 
> >  - Spurious interrupts on IRQ7, which are triggered by IRQ 0 (PIT/HPET). On
> >one of the affected machines this stops when the interrupt system is
> >switched to interrupt remapping !?!?!?
> > 
> >  - Spurious interrupts on various interrupt lines, which are triggered by
> >IOAPIC interrupts >= IRQ16. That's a known issue on quite some chipsets
> >that the legacy PCI interrupt (which is used when IOAPIC is disabled) is
> >triggered when the IOAPIC >=16 interrupt fires.
> > 
> >  - Spurious interrupt caused by driver probing itself. I.e. the driver
> >probing code causes an interrupt issued from the device
> >inadvertently. That happens even on IRQ >= 16.
> > 
> >This problem might be handled by the device driver code itself, but
> >that's going to be ugly. See below.
> 
> That's pretty colorful behavior...
> 
> > We can try to sample more data from the machines of affected users, but I 
> > doubt 
> > that it will give us more information than confirming that we really have 
> > to 
> > deal with all that hardware wreckage out there in some way or the other.
> 
> BTW., instead of trying to avoid the scenario, wow about moving in the other 
> direction: making CONFIG_DEBUG_SHIRQ=y unconditional property in the IRQ core 
> code 
> starting from v4.12 or so, i.e. requiring device driver IRQ handlers to 
> handle the 
> invocation of IRQ handlers at pretty much any moment. (We could also extend 
> it a 
> bit, such as invoking IRQ handlers early after suspend/resume wakeup.)
> 
> Because it's not the requirement that hurts primarily, but the resulting 
> non-determinism and the sporadic crashes. Which can be solved by making the 
> race 
> deterministic via the debug facility.
> 
> If the IRQ handler crashed the moment it was first written by the driver 
> author 
> we'd never see these problems.

Yes, I'd love to do that. That's just a nightmare as well.

See commit 6d83f94db95cf, which added the _FIXME suffix to that code.

So recently I tried to invoke the primary handler, which causes another
issue:

  Some of the low level code (e.g. IOAPIC interrupt migration, but also
  some PPC irq chip machinery) depends on being called in hard interrupt
  context. They invoke get_irq_regs(), which obviously does not work from
  thread context.

So I removed that one from -next as well and postponed it another time. And
I should have known before I tried it that it does not work. Simply because
of that stuff x86 cannot use the software based resend mechanism.

Still trying to wrap my head around a proper solution for the problem. On
x86 we might just check whether we are really in hard irq context and
otherwise skip the part which depends on get_irq_regs(). That would be a
sane thing to do. Have not yet looked at the PPC side of affairs, whether
that's easy to solve as well. But even if it is, then there might be still
other magic code in some irq chip drivers which relies on things which are
only available/correct when actually invoked by a hardware interrupt.

Not only the hardware has colorful behaviour 

Thanks,

tglx

Re: [PATCH v3 2/3] stih-cec: add HPD notifier support

[Rob Herring]

On Fri, Feb 17, 2017 at 11:46:51AM +0100, Benjamin Gaignard wrote:
> By using the HPD notifier framework there is no longer any reason
> to manually set the physical address. This was the one blocking
> issue that prevented this driver from going out of staging, so do
> this move as well.
> 
> Update the bindings documentation the new hdmi phandle.

Should be a separate commit, but it's fine unless you do another spin.

> 
> Signed-off-by: Benjamin Gaignard 
> Signed-off-by: Hans Verkuil 
> CC: devicet...@vger.kernel.org
> 
> version 3:
> - change hdmi phandle from "st,hdmi-handle" to "hdmi-handle"
> ---
>  .../devicetree/bindings/media/stih-cec.txt |   2 +

Acked-by: Rob Herring 

>  drivers/media/platform/Kconfig |  10 +
>  drivers/media/platform/Makefile|   1 +
>  drivers/media/platform/sti/cec/Makefile|   1 +
>  drivers/media/platform/sti/cec/stih-cec.c  | 404 
> +
>  drivers/staging/media/Kconfig  |   2 -
>  drivers/staging/media/Makefile |   1 -
>  drivers/staging/media/st-cec/Kconfig   |   8 -
>  drivers/staging/media/st-cec/Makefile  |   1 -
>  drivers/staging/media/st-cec/TODO  |   7 -
>  drivers/staging/media/st-cec/stih-cec.c| 379 ---
>  11 files changed, 418 insertions(+), 398 deletions(-)
>  create mode 100644 drivers/media/platform/sti/cec/Makefile
>  create mode 100644 drivers/media/platform/sti/cec/stih-cec.c
>  delete mode 100644 drivers/staging/media/st-cec/Kconfig
>  delete mode 100644 drivers/staging/media/st-cec/Makefile
>  delete mode 100644 drivers/staging/media/st-cec/TODO
>  delete mode 100644 drivers/staging/media/st-cec/stih-cec.c

Re: [WARNING: A/V UNSCANNABLE][Merge tag 'media/v4.11-1' of git] ff58d005cd: BUG: unable to handle kernel NULL pointer dereference at 0000039c

[Tony Lindgren]

* Thomas Gleixner  [170227 08:20]:
> On Mon, 27 Feb 2017, Tony Lindgren wrote:
> > * Ingo Molnar  [170227 07:44]:
> > > Because it's not the requirement that hurts primarily, but the resulting 
> > > non-determinism and the sporadic crashes. Which can be solved by making 
> > > the race 
> > > deterministic via the debug facility.
> > > 
> > > If the IRQ handler crashed the moment it was first written by the driver 
> > > author 
> > > we'd never see these problems.
> > 
> > Just in case this is PM related.. Maybe the spurious interrupt is pending
> > from earlier? This could be caused by glitches on the lines with runtime PM,
> > or a pending interrupt during suspend/resume. In that case 
> > IRQ_DISABLE_UNLAZY
> > might provide more clues if the problem goes away.
> 
> It's not PM related.  That's just silly hardware. At the moment when you
> enable some magic bit in the control register, which is required to probe
> the version, the fricking thing spits out a spurious interrupt despite the
> interrupt enable bit in the same control register being still disabled. Of
> course we cannot install an interrupt handler before having probed the
> version and setup other stuff, except we add magic 'if (!initialized)'
> crappola into the handler and lose the ability to install version dependent
> handlers afterwards.

OK and presumably no -EPROBE_DEFER happening either.

> Wonderful crap that, isn't it?

Sounds broken..

Regards,

Tony

Re: [PATCHv4 1/9] video: add hotplug detect notifier support

[Daniel Vetter]

On Mon, Feb 06, 2017 at 11:29:43AM +0100, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> Add support for video hotplug detect and EDID/ELD notifiers, which is used
> to convey information from video drivers to their CEC and audio counterparts.
> 
> Based on an earlier version from Russell King:
> 
> https://patchwork.kernel.org/patch/9277043/
> 
> The hpd_notifier is a reference counted object containing the HPD/EDID/ELD 
> state
> of a video device.
> 
> When a new notifier is registered the current state will be reported to
> that notifier at registration time.
> 
> Signed-off-by: Hans Verkuil 
> Tested-by: Marek Szyprowski 

So I'm super late to the party because I kinda ignored all things CEC thus
far. Ḯ'm not sure this is a great design, with two main concerns:

- notifiers have a tendency to make locking utter painful. By design
  notifiers use their own lock to make sure any callbacks don't disappear
  unduly, but then on the other hand the locking order at
  register/unregister time is inverted. Or anything where you need to go
  the other way. For simple things it works, but my experience is that
  sooner or later things stop being simple, and then you're in complete
  pain. Viz fbdev notifier. I much more prefer if we have a direct
  interface, where we can define the lifetime rules and locking rules
  seperately, and if needed separately for each interface. Especially for
  something which is meant to connect different drivers from different
  subsystems so widely.
  
  You could object that this is the only interaction, and therefore there
  can't be loops, but we have dma-buf, reservation_obj and dma_fence
  sharing going on between lots of drivers already, and lots more will
  follow, so there's plenty of other cross-subsystem interactions going on
  to help complete the loop.

- The other concern I have is that we already have interfaces for ELD and
  hotplug notification. Atm their only restricted for use between gfx and
  snd, and e.g. i915 rolls 2 different kinds of its own, but there is a
  semi-standard one:

  include/sound/hdmi-codec.h

  That also deals with ELD and stuff, would be great if we don't force
  drivers to signal ELD changes in multiple different ways. Also, CEC
  should only exist with a HDMI output, so same thing.

- Afaiui we'll always should have a 1:1 mapping between drm HDMI connector
  and a given CEC pin. Notifiers are for n:m, how is this supposed to work
  if you have multiple HDMI ports around? I see that you look up by struct
  device, but it's a bit unclear what kind of device is supposed to be
  used as the key. If we extend the hdmi-codec stuff from sound and make
  it a notch more generic, then we'd already have the arbiter object to
  ties things together.

Just some thoughts on this. And again my apologies for being late with my
input.

Thanks, Daniel

> ---
>  drivers/video/Kconfig|   3 +
>  drivers/video/Makefile   |   1 +
>  drivers/video/hpd-notifier.c | 134 
> +++
>  include/linux/hpd-notifier.h | 109 +++
>  4 files changed, 247 insertions(+)
>  create mode 100644 drivers/video/hpd-notifier.c
>  create mode 100644 include/linux/hpd-notifier.h
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 3c20af999893..a3a58d8481e9 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -36,6 +36,9 @@ config VIDEOMODE_HELPERS
>  config HDMI
>   bool
>  
> +config HPD_NOTIFIER
> + bool
> +
>  if VT
>   source "drivers/video/console/Kconfig"
>  endif
> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> index 9ad3c17d6456..2697ae5c4166 100644
> --- a/drivers/video/Makefile
> +++ b/drivers/video/Makefile
> @@ -1,5 +1,6 @@
>  obj-$(CONFIG_VGASTATE)+= vgastate.o
>  obj-$(CONFIG_HDMI)+= hdmi.o
> +obj-$(CONFIG_HPD_NOTIFIER)+= hpd-notifier.o
>  
>  obj-$(CONFIG_VT)   += console/
>  obj-$(CONFIG_LOGO) += logo/
> diff --git a/drivers/video/hpd-notifier.c b/drivers/video/hpd-notifier.c
> new file mode 100644
> index ..971e823ead00
> --- /dev/null
> +++ b/drivers/video/hpd-notifier.c
> @@ -0,0 +1,134 @@
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static LIST_HEAD(hpd_notifiers);
> +static DEFINE_MUTEX(hpd_notifiers_lock);
> +
> +struct hpd_notifier *hpd_notifier_get(struct device *dev)
> +{
> + struct hpd_notifier *n;
> +
> + mutex_lock(_notifiers_lock);
> + list_for_each_entry(n, _notifiers, head) {
> + if (n->dev == dev) {
> + mutex_unlock(_notifiers_lock);
> + kref_get(>kref);
> + return n;
> + }
> + }
> + n = kzalloc(sizeof(*n), GFP_KERNEL);
> + if (!n)
> + goto unlock;
> + n->dev = dev;
> + mutex_init(>lock);
> + 

Re: [WARNING: A/V UNSCANNABLE][Merge tag 'media/v4.11-1' of git] ff58d005cd: BUG: unable to handle kernel NULL pointer dereference at 0000039c

[Tony Lindgren]

* Ingo Molnar  [170227 07:44]:
> 
> * Thomas Gleixner  wrote:
> 
> > The pending interrupt issue happens, at least on my test boxen, mostly on
> > the 'legacy' interrupts (0 - 15). But even the IOAPIC interrupts >=16
> > happen occasionally.
> >
> > 
> >  - Spurious interrupts on IRQ7, which are triggered by IRQ 0 (PIT/HPET). On
> >one of the affected machines this stops when the interrupt system is
> >switched to interrupt remapping !?!?!?
> > 
> >  - Spurious interrupts on various interrupt lines, which are triggered by
> >IOAPIC interrupts >= IRQ16. That's a known issue on quite some chipsets
> >that the legacy PCI interrupt (which is used when IOAPIC is disabled) is
> >triggered when the IOAPIC >=16 interrupt fires.
> > 
> >  - Spurious interrupt caused by driver probing itself. I.e. the driver
> >probing code causes an interrupt issued from the device
> >inadvertently. That happens even on IRQ >= 16.

This sounds a lot like what we saw few weeks ago with dwc3. See commit
12a7f17fac5b ("usb: dwc3: omap: fix race of pm runtime with irq handler in
probe"). It was caused by runtime PM and -EPROBE_DEFER, see the description
Grygorii wrote up in that commit.

> >This problem might be handled by the device driver code itself, but
> >that's going to be ugly. See below.
> 
> That's pretty colorful behavior...
> 
> > We can try to sample more data from the machines of affected users, but I 
> > doubt 
> > that it will give us more information than confirming that we really have 
> > to 
> > deal with all that hardware wreckage out there in some way or the other.
> 
> BTW., instead of trying to avoid the scenario, wow about moving in the other 
> direction: making CONFIG_DEBUG_SHIRQ=y unconditional property in the IRQ core 
> code 
> starting from v4.12 or so, i.e. requiring device driver IRQ handlers to 
> handle the 
> invocation of IRQ handlers at pretty much any moment. (We could also extend 
> it a 
> bit, such as invoking IRQ handlers early after suspend/resume wakeup.)
> 
> Because it's not the requirement that hurts primarily, but the resulting 
> non-determinism and the sporadic crashes. Which can be solved by making the 
> race 
> deterministic via the debug facility.
> 
> If the IRQ handler crashed the moment it was first written by the driver 
> author 
> we'd never see these problems.

Just in case this is PM related.. Maybe the spurious interrupt is pending
from earlier? This could be caused by glitches on the lines with runtime PM,
or a pending interrupt during suspend/resume. In that case IRQ_DISABLE_UNLAZY
might provide more clues if the problem goes away.

Regards,

Tony

Re: [WARNING: A/V UNSCANNABLE][Merge tag 'media/v4.11-1' of git] ff58d005cd: BUG: unable to handle kernel NULL pointer dereference at 0000039c

[Ingo Molnar]

* Thomas Gleixner  wrote:

> The pending interrupt issue happens, at least on my test boxen, mostly on
> the 'legacy' interrupts (0 - 15). But even the IOAPIC interrupts >=16
> happen occasionally.
>
> 
>  - Spurious interrupts on IRQ7, which are triggered by IRQ 0 (PIT/HPET). On
>one of the affected machines this stops when the interrupt system is
>switched to interrupt remapping !?!?!?
> 
>  - Spurious interrupts on various interrupt lines, which are triggered by
>IOAPIC interrupts >= IRQ16. That's a known issue on quite some chipsets
>that the legacy PCI interrupt (which is used when IOAPIC is disabled) is
>triggered when the IOAPIC >=16 interrupt fires.
> 
>  - Spurious interrupt caused by driver probing itself. I.e. the driver
>probing code causes an interrupt issued from the device
>inadvertently. That happens even on IRQ >= 16.
> 
>This problem might be handled by the device driver code itself, but
>that's going to be ugly. See below.

That's pretty colorful behavior...

> We can try to sample more data from the machines of affected users, but I 
> doubt 
> that it will give us more information than confirming that we really have to 
> deal with all that hardware wreckage out there in some way or the other.

BTW., instead of trying to avoid the scenario, wow about moving in the other 
direction: making CONFIG_DEBUG_SHIRQ=y unconditional property in the IRQ core 
code 
starting from v4.12 or so, i.e. requiring device driver IRQ handlers to handle 
the 
invocation of IRQ handlers at pretty much any moment. (We could also extend it 
a 
bit, such as invoking IRQ handlers early after suspend/resume wakeup.)

Because it's not the requirement that hurts primarily, but the resulting 
non-determinism and the sporadic crashes. Which can be solved by making the 
race 
deterministic via the debug facility.

If the IRQ handler crashed the moment it was first written by the driver author 
we'd never see these problems.

Thanks,

Ingo

Re: [PATCH v4 24/36] [media] add Omnivision OV5640 sensor driver

[Rob Herring]

On Wed, Feb 15, 2017 at 06:19:26PM -0800, Steve Longerbeam wrote:
> This driver is based on ov5640_mipi.c from Freescale imx_3.10.17_1.0.0_beta
> branch, modified heavily to bring forward to latest interfaces and code
> cleanup.
> 
> Signed-off-by: Steve Longerbeam 
> ---
>  .../devicetree/bindings/media/i2c/ov5640.txt   |   43 +

Please split to separate commit.

>  drivers/media/i2c/Kconfig  |7 +
>  drivers/media/i2c/Makefile |1 +
>  drivers/media/i2c/ov5640.c | 2109 
> 
>  4 files changed, 2160 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5640.txt
>  create mode 100644 drivers/media/i2c/ov5640.c
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt 
> b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
> new file mode 100644
> index 000..4607bbe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
> @@ -0,0 +1,43 @@
> +* Omnivision OV5640 MIPI CSI-2 sensor
> +
> +Required Properties:
> +- compatible: should be "ovti,ov5640"
> +- clocks: reference to the xclk input clock.
> +- clock-names: should be "xclk".
> +- DOVDD-supply: Digital I/O voltage supply, 1.8 volts
> +- AVDD-supply: Analog voltage supply, 2.8 volts
> +- DVDD-supply: Digital core voltage supply, 1.5 volts
> +
> +Optional Properties:
> +- reset-gpios: reference to the GPIO connected to the reset pin, if any.
> +- pwdn-gpios: reference to the GPIO connected to the pwdn pin, if any.

Use powerdown-gpios here as that is a somewhat standard name.

Both need to state what is the active state.

> +
> +The device node must contain one 'port' child node for its digital output
> +video port, in accordance with the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +
> + {
> + ov5640: camera@3c {
> + compatible = "ovti,ov5640";
> + pinctrl-names = "default";
> + pinctrl-0 = <_ov5640>;
> + reg = <0x3c>;
> + clocks = < IMX6QDL_CLK_CKO>;
> + clock-names = "xclk";
> + DOVDD-supply = <_reg>; /* 1.8v */
> + AVDD-supply = <_reg>;  /* 2.8v */
> + DVDD-supply = <_reg>;  /* 1.5v */
> + pwdn-gpios = < 19 GPIO_ACTIVE_HIGH>;
> + reset-gpios = < 20 GPIO_ACTIVE_LOW>;
> +
> + port {
> + ov5640_to_mipi_csi2: endpoint {
> + remote-endpoint = <_csi2_from_ov5640>;
> + clock-lanes = <0>;
> + data-lanes = <1 2>;
> + };
> + };
> + };
> +};

Re: [PATCH v4 15/36] platform: add video-multiplexer subdevice driver

[Rob Herring]

On Wed, Feb 15, 2017 at 06:19:17PM -0800, Steve Longerbeam wrote:
> From: Philipp Zabel 
> 
> This driver can handle SoC internal and external video bus multiplexers,
> controlled either by register bit fields or by a GPIO. The subdevice
> passes through frame interval and mbus configuration of the active input
> to the output side.
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Philipp Zabel 
> 
> --
> 
> - fixed a cut error in vidsw_remove(): v4l2_async_register_subdev()
>   should be unregister.
> 
> - added media_entity_cleanup() and v4l2_device_unregister_subdev()
>   to vidsw_remove().
> 
> - added missing MODULE_DEVICE_TABLE().
>   Suggested-by: Javier Martinez Canillas 
> 
> - there was a line left over from a previous iteration that negated
>   the new way of determining the pad count just before it which
>   has been removed (num_pads = of_get_child_count(np)).
> 
> - Philipp Zabel has developed a set of patches that allow adding
>   to the subdev async notifier waiting list using a chaining method
>   from the async registered callbacks (v4l2_of_subdev_registered()
>   and the prep patches for that). For now, I've removed the use of
>   v4l2_of_subdev_registered() for the vidmux driver's registered
>   callback. This doesn't affect the functionality of this driver,
>   but allows for it to be merged now, before adding the chaining
>   support.
> 
> Signed-off-by: Steve Longerbeam 
> ---
>  .../bindings/media/video-multiplexer.txt   |  59 +++

Please make this a separate commit.

>  drivers/media/platform/Kconfig |   8 +
>  drivers/media/platform/Makefile|   2 +
>  drivers/media/platform/video-multiplexer.c | 474 
> +
>  4 files changed, 543 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/media/video-multiplexer.txt
>  create mode 100644 drivers/media/platform/video-multiplexer.c
> 
> diff --git a/Documentation/devicetree/bindings/media/video-multiplexer.txt 
> b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> new file mode 100644
> index 000..9d133d9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> @@ -0,0 +1,59 @@
> +Video Multiplexer
> +=
> +
> +Video multiplexers allow to select between multiple input ports. Video 
> received
> +on the active input port is passed through to the output port. Muxes 
> described
> +by this binding may be controlled by a syscon register bitfield or by a GPIO.
> +
> +Required properties:
> +- compatible : should be "video-multiplexer"
> +- reg: should be register base of the register containing the control 
> bitfield
> +- bit-mask: bitmask of the control bitfield in the control register
> +- bit-shift: bit offset of the control bitfield in the control register
> +- gpios: alternatively to reg, bit-mask, and bit-shift, a single GPIO phandle
> +  may be given to switch between two inputs
> +- #address-cells: should be <1>
> +- #size-cells: should be <0>
> +- port@*: at least three port nodes containing endpoints connecting to the
> +  source and sink devices according to of_graph bindings. The last port is
> +  the output port, all others are inputs.
> +
> +Example:
> +
> +syscon {
> + compatible = "syscon", "simple-mfd";
> +
> + mux {
> + compatible = "video-multiplexer";
> + /* Single bit (1 << 19) in syscon register 0x04: */
> + reg = <0x04>;
> + bit-mask = <1>;
> + bit-shift = <19>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + mux_in0: endpoint {
> + remote-endpoint = <_source0_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> +
> + mux_in1: endpoint {
> + remote-endpoint = <_source1_out>;
> + };
> + };
> +
> + port@2 {
> + reg = <2>;
> +
> + mux_out: endpoint {
> + remote-endpoint = <_interface_in>;
> + };
> + };
> + };
> +};

Re: [PATCH v4 01/36] [media] dt-bindings: Add bindings for i.MX media driver

[Rob Herring]

On Wed, Feb 15, 2017 at 06:19:03PM -0800, Steve Longerbeam wrote:
> Add bindings documentation for the i.MX media driver.
> 
> Signed-off-by: Steve Longerbeam 
> ---
>  Documentation/devicetree/bindings/media/imx.txt | 66 
> +
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/imx.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/imx.txt 
> b/Documentation/devicetree/bindings/media/imx.txt
> new file mode 100644
> index 000..fd5af50
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/imx.txt
> @@ -0,0 +1,66 @@
> +Freescale i.MX Media Video Device
> +=
> +
> +Video Media Controller node
> +---
> +
> +This is the media controller node for video capture support. It is a
> +virtual device that lists the camera serial interface nodes that the
> +media device will control.
> +
> +Required properties:
> +- compatible : "fsl,imx-capture-subsystem";
> +- ports  : Should contain a list of phandles pointing to camera
> + sensor interface ports of IPU devices
> +
> +example:
> +
> +capture-subsystem {
> + compatible = "fsl,capture-subsystem";
> + ports = <_csi0>, <_csi1>;
> +};
> +
> +fim child node
> +--
> +
> +This is an optional child node of the ipu_csi port nodes. If present and
> +available, it enables the Frame Interval Monitor. Its properties can be
> +used to modify the method in which the FIM measures frame intervals.
> +Refer to Documentation/media/v4l-drivers/imx.rst for more info on the
> +Frame Interval Monitor.

This should have a compatible string.

> +
> +Optional properties:
> +- fsl,input-capture-channel: an input capture channel and channel flags,
> +  specified as . The channel number
> +  must be 0 or 1. The flags can be
> +  IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING, or
> +  IRQ_TYPE_EDGE_BOTH, and specify which input
> +  capture signal edge will trigger the input
> +  capture event. If an input capture channel is
> +  specified, the FIM will use this method to
> +  measure frame intervals instead of via the EOF
> +  interrupt. The input capture method is much
> +  preferred over EOF as it is not subject to
> +  interrupt latency errors. However it requires
> +  routing the VSYNC or FIELD output signals of
> +  the camera sensor to one of the i.MX input
> +  capture pads (SD1_DAT0, SD1_DAT1), which also
> +  gives up support for SD1.
> +
> +
> +mipi_csi2 node
> +--
> +
> +This is the device node for the MIPI CSI-2 Receiver, required for MIPI
> +CSI-2 sensors.
> +
> +Required properties:
> +- compatible : "fsl,imx6-mipi-csi2", "snps,dw-mipi-csi2";
> +- reg   : physical base address and length of the register set;
> +- clocks : the MIPI CSI-2 receiver requires three clocks: hsi_tx
> +  (the DPHY clock), video_27m, and eim_podf;
> +- clock-names: must contain "dphy", "cfg", "pix";

Don't you need ports to describe the sensor and IPU connections?

> +
> +Optional properties:
> +- interrupts : must contain two level-triggered interrupts,
> +  in order: 100 and 101;
> -- 
> 2.7.4
> 

[PATCH 4/9] cec: return -EPERM when no LAs are configured

[Hans Verkuil]

From: Hans Verkuil 

The CEC_TRANSMIT ioctl now returns -EPERM if an attempt is made to
transmit a message for an unconfigured adapter (i.e. userspace
never called CEC_ADAP_S_LOG_ADDRS).

This differentiates this case from when LAs are configured, but no
physical address is set. In that case -ENONET is returned.

Signed-off-by: Hans Verkuil 
---
 drivers/media/cec/cec-api.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/cec/cec-api.c b/drivers/media/cec/cec-api.c
index 627cdf7b12d1..cea350ea2a52 100644
--- a/drivers/media/cec/cec-api.c
+++ b/drivers/media/cec/cec-api.c
@@ -198,7 +198,9 @@ static long cec_transmit(struct cec_adapter *adap, struct 
cec_fh *fh,
return -EINVAL;
 
mutex_lock(>lock);
-   if (adap->is_configuring)
+   if (adap->log_addrs.num_log_addrs == 0)
+   err = -EPERM;
+   else if (adap->is_configuring)
err = -ENONET;
else if (!adap->is_configured && (msg.msg[0] != 0xf0 || msg.reply))
err = -ENONET;
-- 
2.11.0

[PATCH 3/9] cec: allow specific messages even when unconfigured

[Hans Verkuil]

From: Hans Verkuil 

The CEC specifications explicitly allows you to send poll messages and
Image/Text View On messages to a TV, even when unconfigured (i.e. there is
no hotplug signal detected). Some TVs will pull the HPD low when switching
to another input, or when going into standby, but CEC should still be
allowed to wake up such a display.

Add support for sending messages with initiator 0xf (Unregistered) and
destination 0 (TV) when no physical address is present.

This also required another change: the CEC adapter has to stay enabled as
long as 1) the CEC device is configured or 2) at least one filehandle is open
for the CEC device.

Signed-off-by: Hans Verkuil 
---
 drivers/media/cec/cec-adap.c | 27 ---
 drivers/media/cec/cec-api.c  | 19 +--
 2 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
index 421472b492ee..78a85c44d96e 100644
--- a/drivers/media/cec/cec-adap.c
+++ b/drivers/media/cec/cec-adap.c
@@ -367,7 +367,6 @@ int cec_thread_func(void *_adap)
 */
err = 
wait_event_interruptible_timeout(adap->kthread_waitq,
kthread_should_stop() ||
-   (!adap->is_configured && !adap->is_configuring) 
||
(!adap->transmitting &&
 !list_empty(>transmit_queue)),
msecs_to_jiffies(CEC_XFER_TIMEOUT_MS));
@@ -382,8 +381,7 @@ int cec_thread_func(void *_adap)
 
mutex_lock(>lock);
 
-   if ((!adap->is_configured && !adap->is_configuring) ||
-   kthread_should_stop()) {
+   if (kthread_should_stop()) {
cec_flush(adap);
goto unlock;
}
@@ -414,6 +412,7 @@ int cec_thread_func(void *_adap)
struct cec_data, list);
list_del_init(>list);
adap->transmit_queue_sz--;
+
/* Make this the current transmitting message */
adap->transmitting = data;
 
@@ -647,7 +646,8 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct 
cec_msg *msg,
cec_msg_initiator(msg));
return -EINVAL;
}
-   if (!adap->is_configured && !adap->is_configuring)
+   if (!adap->is_configured && !adap->is_configuring &&
+   (msg->msg[0] != 0xf0 || msg->reply))
return -ENONET;
 
if (adap->transmit_queue_sz >= CEC_MAX_MSG_TX_QUEUE_SZ)
@@ -696,6 +696,7 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct 
cec_msg *msg,
 
if (fh)
list_add_tail(>xfer_list, >xfer_list);
+
list_add_tail(>list, >transmit_queue);
adap->transmit_queue_sz++;
if (!adap->transmitting)
@@ -1121,6 +1122,7 @@ static void cec_adap_unconfigure(struct cec_adapter *adap)
adap->is_configuring = false;
adap->is_configured = false;
memset(adap->phys_addrs, 0xff, sizeof(adap->phys_addrs));
+   cec_flush(adap);
wake_up_interruptible(>kthread_waitq);
cec_post_state_event(adap);
 }
@@ -1352,19 +1354,30 @@ void __cec_s_phys_addr(struct cec_adapter *adap, u16 
phys_addr, bool block)
/* Disabling monitor all mode should always succeed */
if (adap->monitor_all_cnt)
WARN_ON(call_op(adap, adap_monitor_all_enable, false));
-   WARN_ON(adap->ops->adap_enable(adap, false));
+   mutex_lock(>devnode.lock);
+   if (list_empty(>devnode.fhs))
+   WARN_ON(adap->ops->adap_enable(adap, false));
+   mutex_unlock(>devnode.lock);
if (phys_addr == CEC_PHYS_ADDR_INVALID)
return;
}
 
-   if (adap->ops->adap_enable(adap, true))
+   mutex_lock(>devnode.lock);
+   if (list_empty(>devnode.fhs) &&
+   adap->ops->adap_enable(adap, true)) {
+   mutex_unlock(>devnode.lock);
return;
+   }
 
if (adap->monitor_all_cnt &&
call_op(adap, adap_monitor_all_enable, true)) {
-   WARN_ON(adap->ops->adap_enable(adap, false));
+   if (list_empty(>devnode.fhs))
+   WARN_ON(adap->ops->adap_enable(adap, false));
+   mutex_unlock(>devnode.lock);
return;
}
+   mutex_unlock(>devnode.lock);
+
adap->phys_addr = phys_addr;
cec_post_state_event(adap);
if (adap->log_addrs.num_log_addrs)
diff --git a/drivers/media/cec/cec-api.c b/drivers/media/cec/cec-api.c
index 8950b6c9d6a9..627cdf7b12d1 100644
--- a/drivers/media/cec/cec-api.c
+++ b/drivers/media/cec/cec-api.c
@@ -198,7 +198,9 @@ static long cec_transmit(struct cec_adapter *adap, struct 
cec_fh 

[PATCH 8/9] cec: improve cec_transmit_msg_fh logging

[Hans Verkuil]

From: Hans Verkuil 

Several error paths didn't log why an error was returned. Add this.

Also handle the corner case of "adapter is unconfigured AND the message
is from Unregistered to TV AND reply is non-zero" separately and return
EINVAL in that case, since it really is an invalid value and not an
unconfigured CEC device.

Signed-off-by: Hans Verkuil 
---
 drivers/media/cec/cec-adap.c | 17 +
 drivers/media/cec/cec-api.c  |  2 +-
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
index 4f1e571d10b7..9e25ba20f4d1 100644
--- a/drivers/media/cec/cec-adap.c
+++ b/drivers/media/cec/cec-adap.c
@@ -646,12 +646,21 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct 
cec_msg *msg,
__func__, cec_msg_initiator(msg));
return -EINVAL;
}
-   if (!adap->is_configured && !adap->is_configuring &&
-   (msg->msg[0] != 0xf0 || msg->reply))
-   return -ENONET;
+   if (!adap->is_configured && !adap->is_configuring) {
+   if (msg->msg[0] != 0xf0) {
+   dprintk(1, "%s: adapter is unconfigured\n", __func__);
+   return -ENONET;
+   }
+   if (msg->reply) {
+   dprintk(1, "%s: invalid msg->reply\n", __func__);
+   return -EINVAL;
+   }
+   }
 
-   if (adap->transmit_queue_sz >= CEC_MAX_MSG_TX_QUEUE_SZ)
+   if (adap->transmit_queue_sz >= CEC_MAX_MSG_TX_QUEUE_SZ) {
+   dprintk(1, "%s: transmit queue full\n", __func__);
return -EBUSY;
+   }
 
data = kzalloc(sizeof(*data), GFP_KERNEL);
if (!data)
diff --git a/drivers/media/cec/cec-api.c b/drivers/media/cec/cec-api.c
index cea350ea2a52..0860fb458757 100644
--- a/drivers/media/cec/cec-api.c
+++ b/drivers/media/cec/cec-api.c
@@ -202,7 +202,7 @@ static long cec_transmit(struct cec_adapter *adap, struct 
cec_fh *fh,
err = -EPERM;
else if (adap->is_configuring)
err = -ENONET;
-   else if (!adap->is_configured && (msg.msg[0] != 0xf0 || msg.reply))
+   else if (!adap->is_configured && msg.msg[0] != 0xf0)
err = -ENONET;
else if (cec_is_busy(adap, fh))
err = -EBUSY;
-- 
2.11.0

[PATCH 1/9] cec: documentation fixes

[Hans Verkuil]

From: Hans Verkuil 

Fixed a few spelling mistakes, but mostly incorrect rst syntax that caused wrong
references or font style.

No actual documentation changes, just fixes.

Signed-off-by: Hans Verkuil 
---
 Documentation/media/uapi/cec/cec-func-ioctl.rst  | 2 +-
 Documentation/media/uapi/cec/cec-func-open.rst   | 2 +-
 Documentation/media/uapi/cec/cec-func-poll.rst   | 4 ++--
 Documentation/media/uapi/cec/cec-ioc-dqevent.rst | 2 +-
 Documentation/media/uapi/cec/cec-ioc-receive.rst | 4 ++--
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/media/uapi/cec/cec-func-ioctl.rst 
b/Documentation/media/uapi/cec/cec-func-ioctl.rst
index 7dcfd178fb24..22fb6304a2df 100644
--- a/Documentation/media/uapi/cec/cec-func-ioctl.rst
+++ b/Documentation/media/uapi/cec/cec-func-ioctl.rst
@@ -30,7 +30,7 @@ Arguments
 
 ``request``
 CEC ioctl request code as defined in the cec.h header file, for
-example :c:func:`CEC_ADAP_G_CAPS`.
+example :ref:`CEC_ADAP_G_CAPS `.
 
 ``argp``
 Pointer to a request-specific structure.
diff --git a/Documentation/media/uapi/cec/cec-func-open.rst 
b/Documentation/media/uapi/cec/cec-func-open.rst
index 0304388cd159..18dfb62f2efe 100644
--- a/Documentation/media/uapi/cec/cec-func-open.rst
+++ b/Documentation/media/uapi/cec/cec-func-open.rst
@@ -33,7 +33,7 @@ Arguments
 Open flags. Access mode must be ``O_RDWR``.
 
 When the ``O_NONBLOCK`` flag is given, the
-:ref:`CEC_RECEIVE ` and :c:func:`CEC_DQEVENT` ioctls
+:ref:`CEC_RECEIVE ` and :ref:`CEC_DQEVENT ` 
ioctls
 will return the ``EAGAIN`` error code when no message or event is 
available, and
 ioctls :ref:`CEC_TRANSMIT `,
 :ref:`CEC_ADAP_S_PHYS_ADDR ` and
diff --git a/Documentation/media/uapi/cec/cec-func-poll.rst 
b/Documentation/media/uapi/cec/cec-func-poll.rst
index 6a863cfda6e0..fa0abd8fb160 100644
--- a/Documentation/media/uapi/cec/cec-func-poll.rst
+++ b/Documentation/media/uapi/cec/cec-func-poll.rst
@@ -30,7 +30,7 @@ Arguments
List of FD events to be watched
 
 ``nfds``
-   Number of FD efents at the \*ufds array
+   Number of FD events at the \*ufds array
 
 ``timeout``
Timeout to wait for events
@@ -49,7 +49,7 @@ is non-zero). CEC devices set the ``POLLIN`` and 
``POLLRDNORM`` flags in
 the ``revents`` field if there are messages in the receive queue. If the
 transmit queue has room for new messages, the ``POLLOUT`` and
 ``POLLWRNORM`` flags are set. If there are events in the event queue,
-then the ``POLLPRI`` flag is set. When the function timed out it returns
+then the ``POLLPRI`` flag is set. When the function times out it returns
 a value of zero, on failure it returns -1 and the ``errno`` variable is
 set appropriately.
 
diff --git a/Documentation/media/uapi/cec/cec-ioc-dqevent.rst 
b/Documentation/media/uapi/cec/cec-ioc-dqevent.rst
index 6e589a1fae17..89ef6c1a2e42 100644
--- a/Documentation/media/uapi/cec/cec-ioc-dqevent.rst
+++ b/Documentation/media/uapi/cec/cec-ioc-dqevent.rst
@@ -56,7 +56,7 @@ it is guaranteed that the state did change in between the two 
events.
 * - __u16
   - ``phys_addr``
   - The current physical address. This is ``CEC_PHYS_ADDR_INVALID`` if no
-  valid physical address is set.
+valid physical address is set.
 * - __u16
   - ``log_addr_mask``
   - The current set of claimed logical addresses. This is 0 if no logical
diff --git a/Documentation/media/uapi/cec/cec-ioc-receive.rst 
b/Documentation/media/uapi/cec/cec-ioc-receive.rst
index dc2adb391c0a..f8a28c303ade 100644
--- a/Documentation/media/uapi/cec/cec-ioc-receive.rst
+++ b/Documentation/media/uapi/cec/cec-ioc-receive.rst
@@ -51,13 +51,13 @@ A received message can be:
be non-zero).
 
 To send a CEC message the application has to fill in the struct
-:c:type:` cec_msg` and pass it to :ref:`ioctl CEC_TRANSMIT `.
+:c:type:`cec_msg` and pass it to :ref:`ioctl CEC_TRANSMIT `.
 The :ref:`ioctl CEC_TRANSMIT ` is only available if
 ``CEC_CAP_TRANSMIT`` is set. If there is no more room in the transmit
 queue, then it will return -1 and set errno to the ``EBUSY`` error code.
 The transmit queue has enough room for 18 messages (about 1 second worth
 of 2-byte messages). Note that the CEC kernel framework will also reply
-to core messages (see :ref:cec-core-processing), so it is not a good
+to core messages (see :ref:`cec-core-processing`), so it is not a good
 idea to fully fill up the transmit queue.
 
 If the file descriptor is in non-blocking mode then the transmit will
-- 
2.11.0

[PATCH 0/9] cec: code and doc fixes/improvements

[Hans Verkuil]

From: Hans Verkuil 

Besides various documentation and logging improvements, the main
addition to CEC is support for a special corner case:

When the physical address is invalid, it is still allowed by the CEC
specification to send messages from 0xf ('Unregistered') to 0 ('TV').

This is a workaround for devices that pull their HPD pin low when they
go into standby or switch to another input, even though CEC messages are
still working.

Regards,

Hans

Hans Verkuil (9):
  cec: documentation fixes
  cec: improve flushing queue
  cec: allow specific messages even when unconfigured
  cec: return -EPERM when no LAs are configured
  cec: document the error codes
  cec: document the special unconfigured case
  cec: use __func__ in log messages.
  cec: improve cec_transmit_msg_fh logging
  cec: log reason for returning -EINVAL

 Documentation/media/uapi/cec/cec-func-ioctl.rst|   2 +-
 Documentation/media/uapi/cec/cec-func-open.rst |   2 +-
 Documentation/media/uapi/cec/cec-func-poll.rst |   4 +-
 .../media/uapi/cec/cec-ioc-adap-g-log-addrs.rst|  13 ++
 .../media/uapi/cec/cec-ioc-adap-g-phys-addr.rst|  13 ++
 Documentation/media/uapi/cec/cec-ioc-dqevent.rst   |  13 +-
 Documentation/media/uapi/cec/cec-ioc-g-mode.rst|  12 ++
 Documentation/media/uapi/cec/cec-ioc-receive.rst   |  55 -
 drivers/media/cec/cec-adap.c   | 134 +
 drivers/media/cec/cec-api.c|  21 +++-
 10 files changed, 208 insertions(+), 61 deletions(-)

-- 
2.11.0

[PATCH 9/9] cec: log reason for returning -EINVAL

[Hans Verkuil]

When validating the struct cec_s_log_addrs input a debug message is printed
for all except two of the 'return -EINVAL' paths.

Also log the reason for the missing two paths.

Signed-off-by: Hans Verkuil 
---
 drivers/media/cec/cec-adap.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
index 9e25ba20f4d1..46b7da6df9b5 100644
--- a/drivers/media/cec/cec-adap.c
+++ b/drivers/media/cec/cec-adap.c
@@ -1461,12 +1461,16 @@ int __cec_s_log_addrs(struct cec_adapter *adap,
 * within the correct range.
 */
if (log_addrs->vendor_id != CEC_VENDOR_ID_NONE &&
-   (log_addrs->vendor_id & 0xff00) != 0)
+   (log_addrs->vendor_id & 0xff00) != 0) {
+   dprintk(1, "invalid vendor ID\n");
return -EINVAL;
+   }
 
if (log_addrs->cec_version != CEC_OP_CEC_VERSION_1_4 &&
-   log_addrs->cec_version != CEC_OP_CEC_VERSION_2_0)
+   log_addrs->cec_version != CEC_OP_CEC_VERSION_2_0) {
+   dprintk(1, "invalid CEC version\n");
return -EINVAL;
+   }
 
if (log_addrs->num_log_addrs > 1)
for (i = 0; i < log_addrs->num_log_addrs; i++)
-- 
2.11.0

[PATCH 5/9] cec: document the error codes

[Hans Verkuil]

Document all the various error codes returned by the CEC ioctls.

These were never documented, instead the documentation relied on a reference
to the generic error codes, but that's not sufficient.

Signed-off-by: Hans Verkuil 
---
 .../media/uapi/cec/cec-ioc-adap-g-log-addrs.rst| 13 
 .../media/uapi/cec/cec-ioc-adap-g-phys-addr.rst| 13 
 Documentation/media/uapi/cec/cec-ioc-dqevent.rst   | 11 +++
 Documentation/media/uapi/cec/cec-ioc-g-mode.rst| 12 +++
 Documentation/media/uapi/cec/cec-ioc-receive.rst   | 37 ++
 5 files changed, 86 insertions(+)

diff --git a/Documentation/media/uapi/cec/cec-ioc-adap-g-log-addrs.rst 
b/Documentation/media/uapi/cec/cec-ioc-adap-g-log-addrs.rst
index 09f09bbe28d4..fcf863ab6f43 100644
--- a/Documentation/media/uapi/cec/cec-ioc-adap-g-log-addrs.rst
+++ b/Documentation/media/uapi/cec/cec-ioc-adap-g-log-addrs.rst
@@ -351,3 +351,16 @@ On success 0 is returned, on error -1 and the ``errno`` 
variable is set
 appropriately. The generic error codes are described at the
 :ref:`Generic Error Codes ` chapter.
 
+The :ref:`ioctl CEC_ADAP_S_LOG_ADDRS ` can return the 
following
+error codes:
+
+ENOTTY
+The ``CEC_CAP_LOG_ADDRS`` capability wasn't set, so this ioctl is not 
supported.
+
+EBUSY
+The CEC adapter is currently configuring itself, or it is already 
configured and
+``num_log_addrs`` is non-zero, or another filehandle is in exclusive 
follower or
+initiator mode, or the filehandle is in mode ``CEC_MODE_NO_INITIATOR``.
+
+EINVAL
+The contents of struct :c:type:`cec_log_addrs` is invalid.
diff --git a/Documentation/media/uapi/cec/cec-ioc-adap-g-phys-addr.rst 
b/Documentation/media/uapi/cec/cec-ioc-adap-g-phys-addr.rst
index a3cdc75cec3e..9e49d4be35d5 100644
--- a/Documentation/media/uapi/cec/cec-ioc-adap-g-phys-addr.rst
+++ b/Documentation/media/uapi/cec/cec-ioc-adap-g-phys-addr.rst
@@ -78,3 +78,16 @@ Return Value
 On success 0 is returned, on error -1 and the ``errno`` variable is set
 appropriately. The generic error codes are described at the
 :ref:`Generic Error Codes ` chapter.
+
+The :ref:`ioctl CEC_ADAP_S_PHYS_ADDR ` can return the 
following
+error codes:
+
+ENOTTY
+The ``CEC_CAP_PHYS_ADDR`` capability wasn't set, so this ioctl is not 
supported.
+
+EBUSY
+Another filehandle is in exclusive follower or initiator mode, or the 
filehandle
+is in mode ``CEC_MODE_NO_INITIATOR``.
+
+EINVAL
+The physical address is malformed.
diff --git a/Documentation/media/uapi/cec/cec-ioc-dqevent.rst 
b/Documentation/media/uapi/cec/cec-ioc-dqevent.rst
index 89ef6c1a2e42..4d3570c2e0b3 100644
--- a/Documentation/media/uapi/cec/cec-ioc-dqevent.rst
+++ b/Documentation/media/uapi/cec/cec-ioc-dqevent.rst
@@ -174,3 +174,14 @@ Return Value
 On success 0 is returned, on error -1 and the ``errno`` variable is set
 appropriately. The generic error codes are described at the
 :ref:`Generic Error Codes ` chapter.
+
+The :ref:`ioctl CEC_DQEVENT ` can return the following
+error codes:
+
+EAGAIN
+This is returned when the filehandle is in non-blocking mode and there
+are no pending events.
+
+ERESTARTSYS
+An interrupt (e.g. Ctrl-C) arrived while in blocking mode waiting for
+events to arrive.
diff --git a/Documentation/media/uapi/cec/cec-ioc-g-mode.rst 
b/Documentation/media/uapi/cec/cec-ioc-g-mode.rst
index e4ded9df0a84..664f0d47bbcd 100644
--- a/Documentation/media/uapi/cec/cec-ioc-g-mode.rst
+++ b/Documentation/media/uapi/cec/cec-ioc-g-mode.rst
@@ -249,3 +249,15 @@ Return Value
 On success 0 is returned, on error -1 and the ``errno`` variable is set
 appropriately. The generic error codes are described at the
 :ref:`Generic Error Codes ` chapter.
+
+The :ref:`ioctl CEC_S_MODE ` can return the following
+error codes:
+
+EINVAL
+The requested mode is invalid.
+
+EPERM
+Monitor mode is requested without having root permissions
+
+EBUSY
+Someone else is already an exclusive follower or initiator.
diff --git a/Documentation/media/uapi/cec/cec-ioc-receive.rst 
b/Documentation/media/uapi/cec/cec-ioc-receive.rst
index f8a28c303ade..3ce7307f55fa 100644
--- a/Documentation/media/uapi/cec/cec-ioc-receive.rst
+++ b/Documentation/media/uapi/cec/cec-ioc-receive.rst
@@ -289,3 +289,40 @@ Return Value
 On success 0 is returned, on error -1 and the ``errno`` variable is set
 appropriately. The generic error codes are described at the
 :ref:`Generic Error Codes ` chapter.
+
+The :ref:`ioctl CEC_RECEIVE ` can return the following
+error codes:
+
+EAGAIN
+No messages are in the receive queue, and the filehandle is in 
non-blocking mode.
+
+ETIMEDOUT
+The ``timeout`` was reached while waiting for a message.
+
+ERESTARTSYS
+The wait for a message was interrupted (e.g. by Ctrl-C).
+
+The :ref:`ioctl CEC_TRANSMIT ` can return the following
+error codes:
+
+ENOTTY
+The ``CEC_CAP_TRANSMIT`` capability wasn't set, so this ioctl is not 
supported.
+
+EPERM
+The CEC 

[PATCH 2/9] cec: improve flushing queue

[Hans Verkuil]

From: Hans Verkuil 

When the adapter is unloaded or unconfigured, then all transmits and
pending waits should be flushed.

Move this code into its own function and improve the code that cancels
delayed work to avoid having to unlock adap->lock.

Signed-off-by: Hans Verkuil 
---
 drivers/media/cec/cec-adap.c | 66 +++-
 1 file changed, 35 insertions(+), 31 deletions(-)

diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
index ccda41c2c9e4..421472b492ee 100644
--- a/drivers/media/cec/cec-adap.c
+++ b/drivers/media/cec/cec-adap.c
@@ -300,6 +300,40 @@ static void cec_data_cancel(struct cec_data *data)
 }
 
 /*
+ * Flush all pending transmits and cancel any pending timeout work.
+ *
+ * This function is called with adap->lock held.
+ */
+static void cec_flush(struct cec_adapter *adap)
+{
+   struct cec_data *data, *n;
+
+   /*
+* If the adapter is disabled, or we're asked to stop,
+* then cancel any pending transmits.
+*/
+   while (!list_empty(>transmit_queue)) {
+   data = list_first_entry(>transmit_queue,
+   struct cec_data, list);
+   cec_data_cancel(data);
+   }
+   if (adap->transmitting)
+   cec_data_cancel(adap->transmitting);
+
+   /* Cancel the pending timeout work. */
+   list_for_each_entry_safe(data, n, >wait_queue, list) {
+   if (cancel_delayed_work(>work))
+   cec_data_cancel(data);
+   /*
+* If cancel_delayed_work returned false, then
+* the cec_wait_timeout function is running,
+* which will call cec_data_completed. So no
+* need to do anything special in that case.
+*/
+   }
+}
+
+/*
  * Main CEC state machine
  *
  * Wait until the thread should be stopped, or we are not transmitting and
@@ -350,37 +384,7 @@ int cec_thread_func(void *_adap)
 
if ((!adap->is_configured && !adap->is_configuring) ||
kthread_should_stop()) {
-   /*
-* If the adapter is disabled, or we're asked to stop,
-* then cancel any pending transmits.
-*/
-   while (!list_empty(>transmit_queue)) {
-   data = list_first_entry(>transmit_queue,
-   struct cec_data, list);
-   cec_data_cancel(data);
-   }
-   if (adap->transmitting)
-   cec_data_cancel(adap->transmitting);
-
-   /*
-* Cancel the pending timeout work. We have to unlock
-* the mutex when flushing the work since
-* cec_wait_timeout() will take it. This is OK since
-* no new entries can be added to wait_queue as long
-* as adap->transmitting is NULL, which it is due to
-* the cec_data_cancel() above.
-*/
-   while (!list_empty(>wait_queue)) {
-   data = list_first_entry(>wait_queue,
-   struct cec_data, list);
-
-   if (!cancel_delayed_work(>work)) {
-   mutex_unlock(>lock);
-   flush_scheduled_work();
-   mutex_lock(>lock);
-   }
-   cec_data_cancel(data);
-   }
+   cec_flush(adap);
goto unlock;
}
 
-- 
2.11.0

[PATCH 7/9] cec: use __func__ in log messages.

[Hans Verkuil]

From: Hans Verkuil 

The hardcoded function name is actually wrong. Use __func__ instead.

Signed-off-by: Hans Verkuil 
---
 drivers/media/cec/cec-adap.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
index 78a85c44d96e..4f1e571d10b7 100644
--- a/drivers/media/cec/cec-adap.c
+++ b/drivers/media/cec/cec-adap.c
@@ -606,17 +606,17 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct 
cec_msg *msg,
 
/* Sanity checks */
if (msg->len == 0 || msg->len > CEC_MAX_MSG_SIZE) {
-   dprintk(1, "cec_transmit_msg: invalid length %d\n", msg->len);
+   dprintk(1, "%s: invalid length %d\n", __func__, msg->len);
return -EINVAL;
}
if (msg->timeout && msg->len == 1) {
-   dprintk(1, "cec_transmit_msg: can't reply for poll msg\n");
+   dprintk(1, "%s: can't reply for poll msg\n", __func__);
return -EINVAL;
}
memset(msg->msg + msg->len, 0, sizeof(msg->msg) - msg->len);
if (msg->len == 1) {
if (cec_msg_destination(msg) == 0xf) {
-   dprintk(1, "cec_transmit_msg: invalid poll message\n");
+   dprintk(1, "%s: invalid poll message\n", __func__);
return -EINVAL;
}
if (cec_has_log_addr(adap, cec_msg_destination(msg))) {
@@ -637,13 +637,13 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct 
cec_msg *msg,
}
if (msg->len > 1 && !cec_msg_is_broadcast(msg) &&
cec_has_log_addr(adap, cec_msg_destination(msg))) {
-   dprintk(1, "cec_transmit_msg: destination is the adapter 
itself\n");
+   dprintk(1, "%s: destination is the adapter itself\n", __func__);
return -EINVAL;
}
if (msg->len > 1 && adap->is_configured &&
!cec_has_log_addr(adap, cec_msg_initiator(msg))) {
-   dprintk(1, "cec_transmit_msg: initiator has unknown logical 
address %d\n",
-   cec_msg_initiator(msg));
+   dprintk(1, "%s: initiator has unknown logical address %d\n",
+   __func__, cec_msg_initiator(msg));
return -EINVAL;
}
if (!adap->is_configured && !adap->is_configuring &&
@@ -663,11 +663,11 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct 
cec_msg *msg,
}
 
if (msg->timeout)
-   dprintk(2, "cec_transmit_msg: %*ph (wait for 0x%02x%s)\n",
-   msg->len, msg->msg, msg->reply, !block ? ", nb" : "");
+   dprintk(2, "%s: %*ph (wait for 0x%02x%s)\n",
+   __func__, msg->len, msg->msg, msg->reply, !block ? ", 
nb" : "");
else
-   dprintk(2, "cec_transmit_msg: %*ph%s\n",
-   msg->len, msg->msg, !block ? " (nb)" : "");
+   dprintk(2, "%s: %*ph%s\n",
+   __func__, msg->len, msg->msg, !block ? " (nb)" : "");
 
data->msg = *msg;
data->fh = fh;
-- 
2.11.0

[PATCH 6/9] cec: document the special unconfigured case

[Hans Verkuil]

From: Hans Verkuil 

Even when the CEC device is unconfigured due to an invalid physical
address it is still allowed to send a message from 0xf (Unregistered)
to 0 (TV). This is a corner case explicitly allowed by the CEC specification.

Document this corner case.

Signed-off-by: Hans Verkuil 
---
 Documentation/media/uapi/cec/cec-ioc-receive.rst | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/media/uapi/cec/cec-ioc-receive.rst 
b/Documentation/media/uapi/cec/cec-ioc-receive.rst
index 3ce7307f55fa..267044f7ac30 100644
--- a/Documentation/media/uapi/cec/cec-ioc-receive.rst
+++ b/Documentation/media/uapi/cec/cec-ioc-receive.rst
@@ -69,6 +69,18 @@ The ``sequence`` field is filled in for every transmit and 
this can be
 checked against the received messages to find the corresponding transmit
 result.
 
+Normally calling :ref:`ioctl CEC_TRANSMIT ` when the physical
+address is invalid (due to e.g. a disconnect) will return ``ENONET``.
+
+However, the CEC specification allows sending messages from 'Unregistered' to
+'TV' when the physical address is invalid since some TVs pull the hotplug 
detect
+pin of the HDMI connector low when they go into standby, or when switching to
+another input.
+
+When the hotplug detect pin goes low the EDID disappears, and thus the
+physical address, but the cable is still connected and CEC still works.
+In order to detect/wake up the device it is allowed to send poll and 
'Image/Text
+View On' messages from initiator 0xf ('Unregistered') to destination 0 ('TV').
 
 .. tabularcolumns:: |p{1.0cm}|p{3.5cm}|p{13.0cm}|
 
@@ -315,6 +327,8 @@ EPERM
 ENONET
 The CEC adapter is not configured, i.e. :ref:`ioctl CEC_ADAP_S_LOG_ADDRS 
`
 was called, but the physical address is invalid so no logical address was 
claimed.
+An exception is made in this case for transmits from initiator 0xf 
('Unregistered')
+to destination 0 ('TV'). In that case the transmit will proceed as usual.
 
 EBUSY
 Another filehandle is in exclusive follower or initiator mode, or the 
filehandle
-- 
2.11.0

Re: [WARNING: A/V UNSCANNABLE][Merge tag 'media/v4.11-1' of git] ff58d005cd: BUG: unable to handle kernel NULL pointer dereference at 0000039c

[Thomas Gleixner]

On Mon, 27 Feb 2017, Thomas Gleixner wrote:
> On Sat, 25 Feb 2017, Linus Torvalds wrote:
> > There are several things that set IRQS_PENDING, ranging from "try to
> > test mis-routed interrupts while irqd was working", to "prepare for
> > suspend losing the irq for us", to "irq auto-probing uses it on
> > unassigned probable irqs".
> > 
> > The *actual* reason to re-send, namely getting a nested irq that we
> > had to drop because we got a second one while still handling the first
> > (or because it was disabled), is just one case.
> > 
> > Personally, I'd suspect some left-over state from auto-probing earlier
> > in the boot, but I don't know. Could we fix that underlying issue?
> 
> I'm on it.

Adding a few trace points to the irq code gives me a pretty consistent
picture across a bunch of different machines.

The pending interrupt issue happens, at least on my test boxen, mostly on
the 'legacy' interrupts (0 - 15). But even the IOAPIC interrupts >=16
happen occasionally.

 - Spurious interrupts on IRQ7, which are triggered by IRQ 0 (PIT/HPET). On
   one of the affected machines this stops when the interrupt system is
   switched to interrupt remapping !?!?!?

 - Spurious interrupts on various interrupt lines, which are triggered by
   IOAPIC interrupts >= IRQ16. That's a known issue on quite some chipsets
   that the legacy PCI interrupt (which is used when IOAPIC is disabled) is
   triggered when the IOAPIC >=16 interrupt fires.

 - Spurious interrupt caused by driver probing itself. I.e. the driver
   probing code causes an interrupt issued from the device
   inadvertently. That happens even on IRQ >= 16.

   This problem might be handled by the device driver code itself, but
   that's going to be ugly. See below.

None of that was caused by misrouted irq or autoprobing. Autoprobing is not
used on any modern maschine at all and the misrouted mechanism cannot
set the pending flag on a interrupt which has no action installed.

Sure, we might not set the pending flag on spurious interrupts, when an
interrupt has no action assigned. We had it that way quite some years
ago. But that caused issues on a couple of (non x86) systems because the
peripheral which sent the spurious interrupt was waiting for
acknowledgement forever and therefor not sending new interrupts. There was
no way to handle that other than resending the interrupt after the action
was installed. The reason for this is the following race:

Device driver init()

  ack_bogus_pending_irq_in_device();
  < Device sends spurious interrupt
  request_irq();

We cannot call ack_bogus_pending_irq_in_device() after the handler has been
installed because it might clear a real interrupt before the handler is
invoked. In fact we can, but that requires pretty ugly code. See below.

Yes, it's broken hardware, but in that area there is more broken than sane
hardware.

The other point here is, that the "IOAPIC triggers spurious interrupt on
legacy lines" issue can actually cause the same problem as the immediate
resend/retrigger:

  request_irq();
 < Other device sends IOAPIC interrupt which
   triggers the legacy line.

If the handler is not prepared at that point, then it explodes as
well. Same for this odd IRQ0 -> IRQ7 trigger, which I can observe on two
machines.

The whole resend/retrigger business is only relevant for edge type
interrupts to deal with the following problem:

  disable_irq();
< Device sends interrupt
  enable_irq();

  ===> Previously sent interrupt has not been acked in the device so no
   further interrupts happen. We lost an edge and are toast.

We never do the resend/retrigger for level type interrupts, because they
are resent by hardware if the interrupt is still pending, which is the case
when you don't acknowlegde it at the device level.

So now you might argue that a driver could handle this by doing:

  disable_irq();
< Device sends interrupt
  enable_irq();

  lock_device();
   do_some_magic_checks_and_handle_pending_irq();
  unlock_device();

Requiring this would add tons of even more broken code to the device
drivers and I really doubt that we want to go there. Same for the
request_irq() case.

I rather prefer that drivers are able to deal with spurious interrupts even
on non shared interrupt lines and let the resend/retrigger mechanism expose
them early when they do not.

We can try to sample more data from the machines of affected users, but I
doubt that it will give us more information than confirming that we really
have to deal with all that hardware wreckage out there in some way or the
other.

Thanks,

tglx

Re: [PATCH 14/15] media: s5p-mfc: Use preallocated block allocator always for MFC v6+

[Marek Szyprowski]

Hi Shuah,

On 2017-02-24 15:23, Shuah Khan wrote:

On Thu, Feb 23, 2017 at 11:26 PM, Marek Szyprowski
 wrote:

On 2017-02-23 22:43, Shuah Khan wrote:

On Tue, Feb 14, 2017 at 12:52 AM, Marek Szyprowski
 wrote:

It turned out that all versions of MFC v6+ hardware doesn't have a strict
requirement for ALL buffers to be allocated on higher addresses than the
firmware base like it was documented for MFC v5. This requirement is true
only for the device and per-context buffers. All video data buffers can
be
allocated anywhere for all MFC v6+ versions. Basing on this fact, the
special DMA configuration based on two reserved memory regions is not
really needed for MFC v6+ devices, because the memory requirements for
the
firmware, device and per-context buffers can be fulfilled by the simple
probe-time pre-allocated block allocator instroduced in previous patch.

This patch enables support for such pre-allocated block based allocator
always for MFC v6+ devices. Due to the limitations of the memory
management
subsystem the largest supported size of the pre-allocated buffer when no
CMA (Contiguous Memory Allocator) is enabled is 4MiB.

This patch also removes the requirement to provide two reserved memory
regions for MFC v6+ devices in device tree. Now the driver is fully
functional without them.

Signed-off-by: Marek Szyprowski 

Hi Marek,

This patch breaks display manager. exynos_drm_gem_create() isn't happy.
dmesg and console are flooded with

odroid login: [  209.170566] [drm:exynos_drm_gem_create] *ERROR* failed to
allo.
[  212.173222] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  215.354790] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  218.736464] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  221.837128] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  226.284827] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  229.242498] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  232.063150] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  235.73] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  239.472061] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  242.567465] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  246.500541] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  249.996018] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  253.837272] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  257.048782] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  260.084819] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  263.448611] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  266.271074] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  269.011558] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  272.039066] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  275.404938] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  278.339033] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  281.274751] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  284.641202] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  287.461039] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  291.062011] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  294.746870] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.
[  298.246570] [drm:exynos_drm_gem_create] *ERROR* failed to allocate
buffer.

I don't think this is an acceptable behavior. It is a regression.


This is a really poor bug report... Could you elaborate a bit how to
reproduce
this? Could you provide your kernel config and information about test
environment?

Yeah. I should have give you more information. My bad.


I suspect that you use CMA without IOMMU and you have too small global CMA
region.

Yes. I have CMA and using exynos_defconfig. Nothing fancy. I think
what's happening is s5p_mfc pre-allocates and there is nothing left
when disaply manager starts requestuing gem buffers. This failure
happens when systemd kicks off lightdm.


After this patch MFC driver uses global CMA region instead of the MFC's
private
ones, so one has to ensure that the global region is large enough.

This is still a regression since it requires users to take some
action. I think we need some kind of checks to warn users there isn't
a large enough CMA region. This is the same config I have been using
forever and with this patch, it breaks.

Easy to reproduce on odroid-xu4 with HDMI display. You just have to
boot the system with exynos_defconfig. Display manager will fail when
it requests buffers.


That is still a bit strange. MFC pre-allocates 8MiB buffer. The default CMA
global region size is 

[PATCH v2] soc-camera: fix rectangle adjustment in cropping

[Guennadi Liakhovetski]

From: Koji Matsuoka 

update_subrect() adjusts the sub-rectangle to be inside a base area.
It checks width and height to not exceed those of the area, then it
checks the low border (left or top) to lie within the area, then the
high border (right or bottom) to lie there too. This latter check has
a bug, which is fixed by this patch.

Signed-off-by: Koji Matsuoka 
Signed-off-by: Yoshihiro Kaneko 
[g.liakhovet...@gmx.de: dropped supposedly wrong hunks]
Signed-off-by: Guennadi Liakhovetski 
---

v2: fix a silly typo, reported by Laurent, thanks! Also renamed the 
function, according to his suggestion.

 drivers/media/platform/soc_camera/soc_scale_crop.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c 
b/drivers/media/platform/soc_camera/soc_scale_crop.c
index f77252d..0116097 100644
--- a/drivers/media/platform/soc_camera/soc_scale_crop.c
+++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
@@ -62,7 +62,8 @@ int soc_camera_client_g_rect(struct v4l2_subdev *sd, struct 
v4l2_rect *rect)
 EXPORT_SYMBOL(soc_camera_client_g_rect);
 
 /* Client crop has changed, update our sub-rectangle to remain within the area 
*/
-static void update_subrect(struct v4l2_rect *rect, struct v4l2_rect *subrect)
+static void move_and_crop_subrect(struct v4l2_rect *rect,
+ struct v4l2_rect *subrect)
 {
if (rect->width < subrect->width)
subrect->width = rect->width;
@@ -72,14 +73,14 @@ static void update_subrect(struct v4l2_rect *rect, struct 
v4l2_rect *subrect)
 
if (rect->left > subrect->left)
subrect->left = rect->left;
-   else if (rect->left + rect->width >
+   else if (rect->left + rect->width <
 subrect->left + subrect->width)
subrect->left = rect->left + rect->width -
subrect->width;
 
if (rect->top > subrect->top)
subrect->top = rect->top;
-   else if (rect->top + rect->height >
+   else if (rect->top + rect->height <
 subrect->top + subrect->height)
subrect->top = rect->top + rect->height -
subrect->height;
@@ -216,7 +217,7 @@ int soc_camera_client_s_selection(struct v4l2_subdev *sd,
 
if (!ret) {
*target_rect = *cam_rect;
-   update_subrect(target_rect, subrect);
+   move_and_crop_subrect(target_rect, subrect);
}
 
return ret;
@@ -299,7 +300,7 @@ static int client_set_fmt(struct soc_camera_device *icd,
if (host_1to1)
*subrect = *rect;
else
-   update_subrect(rect, subrect);
+   move_and_crop_subrect(rect, subrect);
 
return 0;
 }
-- 
1.9.3

Re: Kaffeine commit b510bff2 won't compile

[Mauro Carvalho Chehab]

Em Sun, 26 Feb 2017 20:57:20 -0500
bill murphy  escreveu:

> Hi,
> Can someone double check me on this?
> 
> It seems there might be a missing header,
> in the src directory, preventing the last commit from
> compiling. The commit prior compiles fine. So not that big a deal, just 
> letting folks know what I ran in to.
> 
> I don't see this file, 'log.h', anywhere in the src directory. Guessing 
> it wasn't 'added' for tracking?
> 
> git://anongit.kde.org/kaffeine
> 
> diff between master and previous commit...just a snippet, as other files 
> are including the same missing header.
> 
> diff --git a/src/dvb/dvbcam_linux.cpp b/src/dvb/dvbcam_linux.cpp
> index ceb9dbd..5c9c575 100644
> --- a/src/dvb/dvbcam_linux.cpp
> +++ b/src/dvb/dvbcam_linux.cpp
> @@ -18,11 +18,7 @@
>* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>*/
> 
> -#include 
> -#include 
> -#if QT_VERSION < 0x050500
> -# define qInfo qDebug
> -#endif
> +#include "../log.h"
> 
>   #include 
>   #include 
> 
> where compile complains of that missing header...
> 
> Scanning dependencies of target kaffeine
> [ 20%] Building CXX object 
> src/CMakeFiles/kaffeine.dir/dvb/dvbcam_linux.cpp.o
> /home/user/src2/kaffeine/src/dvb/dvbcam_linux.cpp:21:20: fatal error: 
> ../log.h: No such file or directory
> compilation terminated.

Thanks for complaining about it! I forgot to add src/log.h on the
commit.

You should be able to compile it now.

Thanks,
Mauro

Re: [PATCH] soc-camera: fix rectangle adjustment in cropping

[Guennadi Liakhovetski]

On Mon, 27 Feb 2017, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Monday 27 Feb 2017 09:54:19 Guennadi Liakhovetski wrote:
> > On Mon, 27 Feb 2017, Laurent Pinchart wrote:
> > > On Sunday 26 Feb 2017 21:58:16 Guennadi Liakhovetski wrote:
> > >> From: Koji Matsuoka 
> > >> 
> > >> update_subrect() adjusts the sub-rectangle to be inside a base area.
> > >> It checks width and height to not exceed those of the area, then it
> > >> checks the low border (left or top) to lie within the area, then the
> > >> high border (right or bottom) to lie there too. This latter check has
> > >> a bug, which is fixed by this patch.
> > >> 
> > >> Signed-off-by: Koji Matsuoka 
> > >> Signed-off-by: Yoshihiro Kaneko 
> > >> [g.liakhovet...@gmx.de: dropped supposedly wrong hunks]
> > >> Signed-off-by: Guennadi Liakhovetski 
> > >> ---
> > >> 
> > >> This is a part of the https://patchwork.linuxtv.org/patch/26441/
> > >> submitted almost 2.5 years ago. Back then I commented to the patch but
> > >> never got a reply or an update. I preserved original authorship and Sob
> > >> tags, although this version only uses a small portion of the original
> > >> patch. This version is of course completely untested, any testing (at
> > >> least regression) would be highly appreciated! This code is only used by
> > >> the SH CEU driver and only in cropping / zooming scenarios.
> > >> 
> > >>  drivers/media/platform/soc_camera/soc_scale_crop.c | 4 ++--
> > >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >> 
> > >> diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c
> > >> b/drivers/media/platform/soc_camera/soc_scale_crop.c index
> > >> f77252d..4bfc1bf
> > >> 100644
> > >> --- a/drivers/media/platform/soc_camera/soc_scale_crop.c
> > >> +++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
> > >> @@ -70,14 +70,14 @@ static void update_subrect(struct v4l2_rect *rect,
> > >> struct v4l2_rect *subrect)
> > >>  if (rect->height < subrect->height)
> > >>  subrect->height = rect->height;
> > >> 
> > >> -if (rect->left > subrect->left)
> > >> +if (rect->left < subrect->left)
> > > 
> > > This looks wrong to me. If the purpose of the function is indeed to adjust
> > > subrect to stay within rect, the condition doesn't need to be changed.
> > > 
> > >>  subrect->left = rect->left;
> > >>  else if (rect->left + rect->width >
> > >>   subrect->left + subrect->width)
> > > 
> > > This condition, however, is wrong.
> > 
> > Agh, of course, I meant to change this one! Thanks for catching.
> > 
> > >>  subrect->left = rect->left + rect->width -
> > >>  subrect->width;
> > > 
> > > More than that, adjusting the width first and then the left coordinate can
> > > result in an incorrect width.
> > 
> > The width is adjusted in the beginning only to stay within the area, you
> > cannot go beyond it anyway. So, that has to be done anyway. And then the
> > origin is adjusted.
> > 
> > > It looks to me like you should drop the width
> > > check at the beginning of this function, and turn the "else if" here into
> > > an "if" with the right condition. Or, even better in my opinion, use the
> > > min/max/clamp macros.
> > 
> > Well, that depends on what result we want to achieve, what parameter we
> > prioritise. This approach prioritises width and height, and then adjusts
> > edges to accommodate as much of them as possible. A different approach
> > would be to prioritise the origin (top and left) and adjust width and
> > height to stay within the area. Do we have a preference for this?
> 
> Don't you need both ? "Inside the area" is a pretty well-defined concept :-)
> 
>   subrect->left = max(subrect->left, rect->left);

I prefer to avoid assignments like "a = max(a, b)" to avoid a redundant 
"a = a" assignment, when a >= b :-)

>   subrect->top = max(subrect->top, rect->top);
>   subrect->width = min(subrect->left + subrect->width,
>rect->left + rect->width) - subrect->left;
>   subrect->height = min(subrect->top + subrect->height,
> rect->top + rect->height) - subrect->top;

But this is exactly what I meant, isn't it? Consider an area 100..1000 and 
a subrect 200..2000. Obviously, width is wrong. You have two 
possibilities to adjust it: (1) size-priority. You maximise the size 
(900) and then adjust the origin to accommodate it (100). (2) 
origin-priority: you keep origin (200) and maximise size, based on that 
(800). My approach does (1), yours does (2). I prefer (1). Your approach 
also would break if origin is way too large, e.g. 1100..2000, whereas mine 
would work unchanged.

Thanks
Guennadi

> (Completely untested)
> 
> > > Same comments for the vertical checks.
> > > 
> > >> -if (rect->top > subrect->top)
> > >> +if 

Re: [WARNING: A/V UNSCANNABLE][Merge tag 'media/v4.11-1' of git] ff58d005cd: BUG: unable to handle kernel NULL pointer dereference at 0000039c

[Thomas Gleixner]

On Sat, 25 Feb 2017, Linus Torvalds wrote:
> On Sat, Feb 25, 2017 at 1:07 AM, Ingo Molnar  wrote:
> >
> > So, should we revert the hw-retrigger change:
> >
> >   a9b4f08770b4 x86/ioapic: Restore IO-APIC irq_chip retrigger callback
> >
> > ... until we managed to fix CONFIG_DEBUG_SHIRQ=y? If you'd like to revert it
> > upstream straight away:
> >
> > Acked-by: Ingo Molnar 
> 
> So I'm in no huge hurry to revert that commit as long as we're still
> in the merge window or early -rc's.
> 
> From a debug standpoint, the spurious early interrupts are fine, and
> hopefully will help us find more broken drivers.
> 
> It's just that I'd like to revert it before the actual 4.11 release,
> unless we can find a better solution.
> 
> Because it really seems like the interrupt re-trigger is entirely
> bogus. It's not an _actual_ "re-trigger the interrupt that may have
> gotten lost", it's some code that ends up triggering it for no good
> reason.
> 
> So I'd actually hope that we could figure out why IRQS_PENDING got
> set, and perhaps fix the underlying cause?
>
> There are several things that set IRQS_PENDING, ranging from "try to
> test mis-routed interrupts while irqd was working", to "prepare for
> suspend losing the irq for us", to "irq auto-probing uses it on
> unassigned probable irqs".
> 
> The *actual* reason to re-send, namely getting a nested irq that we
> had to drop because we got a second one while still handling the first
> (or because it was disabled), is just one case.
> 
> Personally, I'd suspect some left-over state from auto-probing earlier
> in the boot, but I don't know. Could we fix that underlying issue?

I'm on it.

Thanks,

tglx

Re: [PATCH] soc-camera: fix rectangle adjustment in cropping

[Laurent Pinchart]

Hi Guennadi,

Thank you for the patch.

On Sunday 26 Feb 2017 21:58:16 Guennadi Liakhovetski wrote:
> From: Koji Matsuoka 
> 
> update_subrect() adjusts the sub-rectangle to be inside a base area.
> It checks width and height to not exceed those of the area, then it
> checks the low border (left or top) to lie within the area, then the
> high border (right or bottom) to lie there too. This latter check has
> a bug, which is fixed by this patch.
> 
> Signed-off-by: Koji Matsuoka 
> Signed-off-by: Yoshihiro Kaneko 
> [g.liakhovet...@gmx.de: dropped supposedly wrong hunks]
> Signed-off-by: Guennadi Liakhovetski 
> ---
> 
> This is a part of the https://patchwork.linuxtv.org/patch/26441/ submitted
> almost 2.5 years ago. Back then I commented to the patch but never got a
> reply or an update. I preserved original authorship and Sob tags, although
> this version only uses a small portion of the original patch. This version
> is of course completely untested, any testing (at least regression) would
> be highly appreciated! This code is only used by the SH CEU driver and
> only in cropping / zooming scenarios.
> 
>  drivers/media/platform/soc_camera/soc_scale_crop.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c
> b/drivers/media/platform/soc_camera/soc_scale_crop.c index f77252d..4bfc1bf
> 100644
> --- a/drivers/media/platform/soc_camera/soc_scale_crop.c
> +++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
> @@ -70,14 +70,14 @@ static void update_subrect(struct v4l2_rect *rect,
> struct v4l2_rect *subrect) if (rect->height < subrect->height)
>   subrect->height = rect->height;
> 
> - if (rect->left > subrect->left)
> + if (rect->left < subrect->left)

This looks wrong to me. If the purpose of the function is indeed to adjust 
subrect to stay within rect, the condition doesn't need to be changed.

>   subrect->left = rect->left;
>   else if (rect->left + rect->width >
>subrect->left + subrect->width)

This condition, however, is wrong.

>   subrect->left = rect->left + rect->width -
>   subrect->width;

More than that, adjusting the width first and then the left coordinate can 
result in an incorrect width. It looks to me like you should drop the width 
check at the beginning of this function, and turn the "else if" here into an 
"if" with the right condition. Or, even better in my opinion, use the 
min/max/clamp macros.

Same comments for the vertical checks.

> - if (rect->top > subrect->top)
> + if (rect->top < subrect->top)
>   subrect->top = rect->top;
>   else if (rect->top + rect->height >
>subrect->top + subrect->height)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] soc-camera: fix rectangle adjustment in cropping

[Guennadi Liakhovetski]

On Mon, 27 Feb 2017, Hans Verkuil wrote:

> On 02/27/2017 10:02 AM, Laurent Pinchart wrote:
> > Hi Guennadi,
> > 
> > On Monday 27 Feb 2017 09:54:19 Guennadi Liakhovetski wrote:
> >> On Mon, 27 Feb 2017, Laurent Pinchart wrote:
> >>> On Sunday 26 Feb 2017 21:58:16 Guennadi Liakhovetski wrote:
>  From: Koji Matsuoka 
> 
>  update_subrect() adjusts the sub-rectangle to be inside a base area.
>  It checks width and height to not exceed those of the area, then it
>  checks the low border (left or top) to lie within the area, then the
>  high border (right or bottom) to lie there too. This latter check has
>  a bug, which is fixed by this patch.
> 
>  Signed-off-by: Koji Matsuoka 
>  Signed-off-by: Yoshihiro Kaneko 
>  [g.liakhovet...@gmx.de: dropped supposedly wrong hunks]
>  Signed-off-by: Guennadi Liakhovetski 
>  ---
> 
>  This is a part of the https://patchwork.linuxtv.org/patch/26441/
>  submitted almost 2.5 years ago. Back then I commented to the patch but
>  never got a reply or an update. I preserved original authorship and Sob
>  tags, although this version only uses a small portion of the original
>  patch. This version is of course completely untested, any testing (at
>  least regression) would be highly appreciated! This code is only used by
>  the SH CEU driver and only in cropping / zooming scenarios.
> 
>   drivers/media/platform/soc_camera/soc_scale_crop.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
>  diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c
>  b/drivers/media/platform/soc_camera/soc_scale_crop.c index
>  f77252d..4bfc1bf
>  100644
>  --- a/drivers/media/platform/soc_camera/soc_scale_crop.c
>  +++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
>  @@ -70,14 +70,14 @@ static void update_subrect(struct v4l2_rect *rect,
>  struct v4l2_rect *subrect)
>   if (rect->height < subrect->height)
>   subrect->height = rect->height;
> 
>  -if (rect->left > subrect->left)
>  +if (rect->left < subrect->left)
> >>>
> >>> This looks wrong to me. If the purpose of the function is indeed to adjust
> >>> subrect to stay within rect, the condition doesn't need to be changed.
> >>>
>   subrect->left = rect->left;
>   else if (rect->left + rect->width >
>    subrect->left + subrect->width)
> >>>
> >>> This condition, however, is wrong.
> >>
> >> Agh, of course, I meant to change this one! Thanks for catching.
> >>
>   subrect->left = rect->left + rect->width -
>   subrect->width;
> >>>
> >>> More than that, adjusting the width first and then the left coordinate can
> >>> result in an incorrect width.
> >>
> >> The width is adjusted in the beginning only to stay within the area, you
> >> cannot go beyond it anyway. So, that has to be done anyway. And then the
> >> origin is adjusted.
> >>
> >>> It looks to me like you should drop the width
> >>> check at the beginning of this function, and turn the "else if" here into
> >>> an "if" with the right condition. Or, even better in my opinion, use the
> >>> min/max/clamp macros.
> >>
> >> Well, that depends on what result we want to achieve, what parameter we
> >> prioritise. This approach prioritises width and height, and then adjusts
> >> edges to accommodate as much of them as possible. A different approach
> >> would be to prioritise the origin (top and left) and adjust width and
> >> height to stay within the area. Do we have a preference for this?
> > 
> > Don't you need both ? "Inside the area" is a pretty well-defined concept :-)
> 
> Generally the top-left is adjusted first, and then the width/height if it 
> still
> can't be made to fit. I.e. the priority is to keep the width/height unchanged
> if possible.

Ok, sure, you can use either order, but if we prioritise width / height, 
then the only restriction for them is to be <= original width / height, 
right? So, you can always first do

if (subrect->width > rect->width)
subrect->width = rect->width;

right? That wqay you guarantee, that you can fit and that you keep as much 
of the requested subrect width, as you can. And then you can adjust left / 
top if still needed.

Thanks
Guennadi

> 
> Regards,
> 
>   Hans
> 
> > 
> > subrect->left = max(subrect->left, rect->left);
> > subrect->top = max(subrect->top, rect->top);
> > subrect->width = min(subrect->left + subrect->width,
> >  rect->left + rect->width) - subrect->left;
> > subrect->height = min(subrect->top + subrect->height,
> >   rect->top + rect->height) - subrect->top;
> > 
> > (Completely untested)
> > 
> >>> Same 

Re: [PATCH] soc-camera: fix rectangle adjustment in cropping

[Laurent Pinchart]

Hi Guennadi,

On Monday 27 Feb 2017 10:13:53 Guennadi Liakhovetski wrote:
> On Mon, 27 Feb 2017, Laurent Pinchart wrote:
> > On Monday 27 Feb 2017 09:54:19 Guennadi Liakhovetski wrote:
> >> On Mon, 27 Feb 2017, Laurent Pinchart wrote:
> >>> On Sunday 26 Feb 2017 21:58:16 Guennadi Liakhovetski wrote:
>  From: Koji Matsuoka 
>  
>  update_subrect() adjusts the sub-rectangle to be inside a base area.
>  It checks width and height to not exceed those of the area, then it
>  checks the low border (left or top) to lie within the area, then the
>  high border (right or bottom) to lie there too. This latter check has
>  a bug, which is fixed by this patch.
>  
>  Signed-off-by: Koji Matsuoka 
>  Signed-off-by: Yoshihiro Kaneko 
>  [g.liakhovet...@gmx.de: dropped supposedly wrong hunks]
>  Signed-off-by: Guennadi Liakhovetski 
>  ---
>  
>  This is a part of the https://patchwork.linuxtv.org/patch/26441/
>  submitted almost 2.5 years ago. Back then I commented to the patch
>  but never got a reply or an update. I preserved original authorship
>  and Sob tags, although this version only uses a small portion of the
>  original patch. This version is of course completely untested, any
>  testing (at least regression) would be highly appreciated! This code
>  is only used by the SH CEU driver and only in cropping / zooming
>  scenarios.
>  
>   drivers/media/platform/soc_camera/soc_scale_crop.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>  
>  diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c
>  b/drivers/media/platform/soc_camera/soc_scale_crop.c index
>  f77252d..4bfc1bf
>  100644
>  --- a/drivers/media/platform/soc_camera/soc_scale_crop.c
>  +++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
>  @@ -70,14 +70,14 @@ static void update_subrect(struct v4l2_rect
>  *rect, struct v4l2_rect *subrect)
>   if (rect->height < subrect->height)
>   subrect->height = rect->height;
>  
>  -if (rect->left > subrect->left)
>  +if (rect->left < subrect->left)
> >>> 
> >>> This looks wrong to me. If the purpose of the function is indeed to
> >>> adjust subrect to stay within rect, the condition doesn't need to be
> >>> changed.
> >>> 
>   subrect->left = rect->left;
>   else if (rect->left + rect->width >
>    subrect->left + subrect->width)
> >>> 
> >>> This condition, however, is wrong.
> >> 
> >> Agh, of course, I meant to change this one! Thanks for catching.
> >> 
>   subrect->left = rect->left + rect->width -
>   subrect->width;
> >>> 
> >>> More than that, adjusting the width first and then the left coordinate
> >>> can result in an incorrect width.
> >> 
> >> The width is adjusted in the beginning only to stay within the area, you
> >> cannot go beyond it anyway. So, that has to be done anyway. And then the
> >> origin is adjusted.
> >> 
> >>> It looks to me like you should drop the width
> >>> check at the beginning of this function, and turn the "else if" here
> >>> into an "if" with the right condition. Or, even better in my opinion,
> >>> use the min/max/clamp macros.
> >> 
> >> Well, that depends on what result we want to achieve, what parameter we
> >> prioritise. This approach prioritises width and height, and then adjusts
> >> edges to accommodate as much of them as possible. A different approach
> >> would be to prioritise the origin (top and left) and adjust width and
> >> height to stay within the area. Do we have a preference for this?
> > 
> > Don't you need both ? "Inside the area" is a pretty well-defined concept
> > :-)
> >
> > subrect->left = max(subrect->left, rect->left);
> 
> I prefer to avoid assignments like "a = max(a, b)" to avoid a redundant
> "a = a" assignment, when a >= b :-)

The compiler should hopefully optimize that for you.

> > subrect->top = max(subrect->top, rect->top);
> > subrect->width = min(subrect->left + subrect->width,
> >  rect->left + rect->width) - subrect->left;
> > subrect->height = min(subrect->top + subrect->height,
> >   rect->top + rect->height) - subrect->top;
> 
> But this is exactly what I meant, isn't it? Consider an area 100..1000 and
> a subrect 200..2000. Obviously, width is wrong. You have two
> possibilities to adjust it: (1) size-priority. You maximise the size
> (900) and then adjust the origin to accommodate it (100). (2)
> origin-priority: you keep origin (200) and maximise size, based on that
> (800). My approach does (1), yours does (2). I prefer (1). Your approach
> also would break if origin is way too large, e.g. 1100..2000, whereas mine
> would work 

Re: [PATCH] soc-camera: fix rectangle adjustment in cropping

[Hans Verkuil]

On 02/27/2017 10:02 AM, Laurent Pinchart wrote:
> Hi Guennadi,
> 
> On Monday 27 Feb 2017 09:54:19 Guennadi Liakhovetski wrote:
>> On Mon, 27 Feb 2017, Laurent Pinchart wrote:
>>> On Sunday 26 Feb 2017 21:58:16 Guennadi Liakhovetski wrote:
 From: Koji Matsuoka 

 update_subrect() adjusts the sub-rectangle to be inside a base area.
 It checks width and height to not exceed those of the area, then it
 checks the low border (left or top) to lie within the area, then the
 high border (right or bottom) to lie there too. This latter check has
 a bug, which is fixed by this patch.

 Signed-off-by: Koji Matsuoka 
 Signed-off-by: Yoshihiro Kaneko 
 [g.liakhovet...@gmx.de: dropped supposedly wrong hunks]
 Signed-off-by: Guennadi Liakhovetski 
 ---

 This is a part of the https://patchwork.linuxtv.org/patch/26441/
 submitted almost 2.5 years ago. Back then I commented to the patch but
 never got a reply or an update. I preserved original authorship and Sob
 tags, although this version only uses a small portion of the original
 patch. This version is of course completely untested, any testing (at
 least regression) would be highly appreciated! This code is only used by
 the SH CEU driver and only in cropping / zooming scenarios.

  drivers/media/platform/soc_camera/soc_scale_crop.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c
 b/drivers/media/platform/soc_camera/soc_scale_crop.c index
 f77252d..4bfc1bf
 100644
 --- a/drivers/media/platform/soc_camera/soc_scale_crop.c
 +++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
 @@ -70,14 +70,14 @@ static void update_subrect(struct v4l2_rect *rect,
 struct v4l2_rect *subrect)
if (rect->height < subrect->height)
subrect->height = rect->height;

 -  if (rect->left > subrect->left)
 +  if (rect->left < subrect->left)
>>>
>>> This looks wrong to me. If the purpose of the function is indeed to adjust
>>> subrect to stay within rect, the condition doesn't need to be changed.
>>>
subrect->left = rect->left;
else if (rect->left + rect->width >
 subrect->left + subrect->width)
>>>
>>> This condition, however, is wrong.
>>
>> Agh, of course, I meant to change this one! Thanks for catching.
>>
subrect->left = rect->left + rect->width -
subrect->width;
>>>
>>> More than that, adjusting the width first and then the left coordinate can
>>> result in an incorrect width.
>>
>> The width is adjusted in the beginning only to stay within the area, you
>> cannot go beyond it anyway. So, that has to be done anyway. And then the
>> origin is adjusted.
>>
>>> It looks to me like you should drop the width
>>> check at the beginning of this function, and turn the "else if" here into
>>> an "if" with the right condition. Or, even better in my opinion, use the
>>> min/max/clamp macros.
>>
>> Well, that depends on what result we want to achieve, what parameter we
>> prioritise. This approach prioritises width and height, and then adjusts
>> edges to accommodate as much of them as possible. A different approach
>> would be to prioritise the origin (top and left) and adjust width and
>> height to stay within the area. Do we have a preference for this?
> 
> Don't you need both ? "Inside the area" is a pretty well-defined concept :-)

Generally the top-left is adjusted first, and then the width/height if it still
can't be made to fit. I.e. the priority is to keep the width/height unchanged
if possible.

Regards,

Hans

> 
>   subrect->left = max(subrect->left, rect->left);
>   subrect->top = max(subrect->top, rect->top);
>   subrect->width = min(subrect->left + subrect->width,
>rect->left + rect->width) - subrect->left;
>   subrect->height = min(subrect->top + subrect->height,
> rect->top + rect->height) - subrect->top;
> 
> (Completely untested)
> 
>>> Same comments for the vertical checks.
>>>
 -  if (rect->top > subrect->top)
 +  if (rect->top < subrect->top)
subrect->top = rect->top;
else if (rect->top + rect->height >
 subrect->top + subrect->height)
> 

Re: [PATCH] soc-camera: fix rectangle adjustment in cropping

[Laurent Pinchart]

Hi Guennadi,

On Monday 27 Feb 2017 09:54:19 Guennadi Liakhovetski wrote:
> On Mon, 27 Feb 2017, Laurent Pinchart wrote:
> > On Sunday 26 Feb 2017 21:58:16 Guennadi Liakhovetski wrote:
> >> From: Koji Matsuoka 
> >> 
> >> update_subrect() adjusts the sub-rectangle to be inside a base area.
> >> It checks width and height to not exceed those of the area, then it
> >> checks the low border (left or top) to lie within the area, then the
> >> high border (right or bottom) to lie there too. This latter check has
> >> a bug, which is fixed by this patch.
> >> 
> >> Signed-off-by: Koji Matsuoka 
> >> Signed-off-by: Yoshihiro Kaneko 
> >> [g.liakhovet...@gmx.de: dropped supposedly wrong hunks]
> >> Signed-off-by: Guennadi Liakhovetski 
> >> ---
> >> 
> >> This is a part of the https://patchwork.linuxtv.org/patch/26441/
> >> submitted almost 2.5 years ago. Back then I commented to the patch but
> >> never got a reply or an update. I preserved original authorship and Sob
> >> tags, although this version only uses a small portion of the original
> >> patch. This version is of course completely untested, any testing (at
> >> least regression) would be highly appreciated! This code is only used by
> >> the SH CEU driver and only in cropping / zooming scenarios.
> >> 
> >>  drivers/media/platform/soc_camera/soc_scale_crop.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c
> >> b/drivers/media/platform/soc_camera/soc_scale_crop.c index
> >> f77252d..4bfc1bf
> >> 100644
> >> --- a/drivers/media/platform/soc_camera/soc_scale_crop.c
> >> +++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
> >> @@ -70,14 +70,14 @@ static void update_subrect(struct v4l2_rect *rect,
> >> struct v4l2_rect *subrect)
> >>if (rect->height < subrect->height)
> >>subrect->height = rect->height;
> >> 
> >> -  if (rect->left > subrect->left)
> >> +  if (rect->left < subrect->left)
> > 
> > This looks wrong to me. If the purpose of the function is indeed to adjust
> > subrect to stay within rect, the condition doesn't need to be changed.
> > 
> >>subrect->left = rect->left;
> >>else if (rect->left + rect->width >
> >> subrect->left + subrect->width)
> > 
> > This condition, however, is wrong.
> 
> Agh, of course, I meant to change this one! Thanks for catching.
> 
> >>subrect->left = rect->left + rect->width -
> >>subrect->width;
> > 
> > More than that, adjusting the width first and then the left coordinate can
> > result in an incorrect width.
> 
> The width is adjusted in the beginning only to stay within the area, you
> cannot go beyond it anyway. So, that has to be done anyway. And then the
> origin is adjusted.
> 
> > It looks to me like you should drop the width
> > check at the beginning of this function, and turn the "else if" here into
> > an "if" with the right condition. Or, even better in my opinion, use the
> > min/max/clamp macros.
> 
> Well, that depends on what result we want to achieve, what parameter we
> prioritise. This approach prioritises width and height, and then adjusts
> edges to accommodate as much of them as possible. A different approach
> would be to prioritise the origin (top and left) and adjust width and
> height to stay within the area. Do we have a preference for this?

Don't you need both ? "Inside the area" is a pretty well-defined concept :-)

subrect->left = max(subrect->left, rect->left);
subrect->top = max(subrect->top, rect->top);
subrect->width = min(subrect->left + subrect->width,
 rect->left + rect->width) - subrect->left;
subrect->height = min(subrect->top + subrect->height,
  rect->top + rect->height) - subrect->top;

(Completely untested)

> > Same comments for the vertical checks.
> > 
> >> -  if (rect->top > subrect->top)
> >> +  if (rect->top < subrect->top)
> >>subrect->top = rect->top;
> >>else if (rect->top + rect->height >
> >> subrect->top + subrect->height)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] soc-camera: fix rectangle adjustment in cropping

[Guennadi Liakhovetski]

Hi Laurent,

On Mon, 27 Feb 2017, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thank you for the patch.
> 
> On Sunday 26 Feb 2017 21:58:16 Guennadi Liakhovetski wrote:
> > From: Koji Matsuoka 
> > 
> > update_subrect() adjusts the sub-rectangle to be inside a base area.
> > It checks width and height to not exceed those of the area, then it
> > checks the low border (left or top) to lie within the area, then the
> > high border (right or bottom) to lie there too. This latter check has
> > a bug, which is fixed by this patch.
> > 
> > Signed-off-by: Koji Matsuoka 
> > Signed-off-by: Yoshihiro Kaneko 
> > [g.liakhovet...@gmx.de: dropped supposedly wrong hunks]
> > Signed-off-by: Guennadi Liakhovetski 
> > ---
> > 
> > This is a part of the https://patchwork.linuxtv.org/patch/26441/ submitted
> > almost 2.5 years ago. Back then I commented to the patch but never got a
> > reply or an update. I preserved original authorship and Sob tags, although
> > this version only uses a small portion of the original patch. This version
> > is of course completely untested, any testing (at least regression) would
> > be highly appreciated! This code is only used by the SH CEU driver and
> > only in cropping / zooming scenarios.
> > 
> >  drivers/media/platform/soc_camera/soc_scale_crop.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c
> > b/drivers/media/platform/soc_camera/soc_scale_crop.c index f77252d..4bfc1bf
> > 100644
> > --- a/drivers/media/platform/soc_camera/soc_scale_crop.c
> > +++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
> > @@ -70,14 +70,14 @@ static void update_subrect(struct v4l2_rect *rect,
> > struct v4l2_rect *subrect) if (rect->height < subrect->height)
> > subrect->height = rect->height;
> > 
> > -   if (rect->left > subrect->left)
> > +   if (rect->left < subrect->left)
> 
> This looks wrong to me. If the purpose of the function is indeed to adjust 
> subrect to stay within rect, the condition doesn't need to be changed.
> 
> > subrect->left = rect->left;
> > else if (rect->left + rect->width >
> >  subrect->left + subrect->width)
> 
> This condition, however, is wrong.

Agh, of course, I meant to change this one! Thanks for catching.

> 
> > subrect->left = rect->left + rect->width -
> > subrect->width;
> 
> More than that, adjusting the width first and then the left coordinate can 
> result in an incorrect width.

The width is adjusted in the beginning only to stay within the area, you 
cannot go beyond it anyway. So, that has to be done anyway. And then the 
origin is adjusted.

> It looks to me like you should drop the width 
> check at the beginning of this function, and turn the "else if" here into an 
> "if" with the right condition. Or, even better in my opinion, use the 
> min/max/clamp macros.

Well, that depends on what result we want to achieve, what parameter we 
prioritise. This approach prioritises width and height, and then adjusts 
edges to accommodate as much of them as possible. A different approach 
would be to prioritise the origin (top and left) and adjust width and 
height to stay within the area. Do we have a preference for this?

Thanks
Guennadi

> 
> Same comments for the vertical checks.
> 
> > -   if (rect->top > subrect->top)
> > +   if (rect->top < subrect->top)
> > subrect->top = rect->top;
> > else if (rect->top + rect->height >
> >  subrect->top + subrect->height)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCH] bcm2048: Fix checkpatch checks

[Greg Kroah-Hartman]

On Sat, Feb 18, 2017 at 11:52:37AM +0800, Man Choy wrote:
> Fix following checks:
> 
> CHECK: Avoid crashing the kernel - try using WARN_ON & recovery code rather 
> than BUG() or BUG_ON()
> +   BUG_ON((index+2) >= BCM2048_MAX_RDS_RT);
> 
> CHECK: spaces preferred around that '+' (ctx:VxV)
> +   BUG_ON((index+2) >= BCM2048_MAX_RDS_RT);
>  ^
> 
> CHECK: Avoid crashing the kernel - try using WARN_ON & recovery code rather 
> than BUG() or BUG_ON()
> +   BUG_ON((index+4) >= BCM2048_MAX_RDS_RT);
> 
> CHECK: spaces preferred around that '+' (ctx:VxV)
> +   BUG_ON((index+4) >= BCM2048_MAX_RDS_RT);
>  ^
> ---
>  drivers/staging/media/bcm2048/radio-bcm2048.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/bcm2048/radio-bcm2048.c 
> b/drivers/staging/media/bcm2048/radio-bcm2048.c
> index 37bd439..d5ee279 100644
> --- a/drivers/staging/media/bcm2048/radio-bcm2048.c
> +++ b/drivers/staging/media/bcm2048/radio-bcm2048.c
> @@ -1534,7 +1534,7 @@ static int bcm2048_parse_rt_match_c(struct 
> bcm2048_device *bdev, int i,
>   if (crc == BCM2048_RDS_CRC_UNRECOVARABLE)
>   return 0;
>  
> - BUG_ON((index+2) >= BCM2048_MAX_RDS_RT);
> + WARN_ON((index + 2) >= BCM2048_MAX_RDS_RT);

Ick, no to all of these!  What happens if this is true, the code will
crash, right?  You have to properly recover from this, don't just throw
the message out to userspace and then keep on going.

You can't just do a search/replace for this, otherwise it would have
been done already :)

thanks,

greg k-h