[RFC v4 00/21] Make use of kref in media device, grab references as needed

[RFC v4 00/21] Make use of kref in media device, grab references as needed

[Sakari Ailus]

Hi folks,

This is the third version of the RFC set to fix referencing in media
devices.

The lifetime of the media device (and media devnode) is now bound to that
of struct device embedded in it and its memory is only released once the
last reference is gone: unregistering is simply unregistering, it no
longer should release memory which could be further accessed.
                                                                                
A video node or a sub-device node also gets a reference to the media
device, i.e. the release function of the video device node will release
its reference to the media device. The same goes for file handles to the
media device.
                                                                                
As a side effect of this is that the media device, it is allocate together
with the media devnode. The driver may also rely its own resources to the
media device. Alternatively there's also a priv field to hold drivers
private pointer (for container_of() is an option in this case). We could
drop one of these options but currently both are possible.
                                                                                
I've tested this by manually unbinding the omap3isp platform device while
streaming. Driver changes are required for this to work; by not using
dynamic allocation (i.e. media_device_alloc()) the old behaviour is still
supported. This is still unlikely to be a grave problem as there are not
that many device drivers that support physically removable devices. We've
had this problem for other devices for many years without paying much
notice --- that doesn't mean I don't think at least drivers for removable
devices shouldn't be changed as part of the set later on, I'd just like to
get review comments on the approach first.
                                                                                
The three patches that originally partially resolved some of these issues
are reverted in the beginning of the set. I'm still posting this as an RFC
mainly since the testing is somewhat limited so far.

A sample of what happens if streaming is stopped while the device is being
removed during streaming without this set:

-----------------8<----------------------
[  288.911560] omap3isp 480bc000.isp: media_gobj_destroy id 98: interface link id 97 ==> id 8
[  288.920349] omap3isp 480bc000.isp: media_gobj_destroy id 52: data link id 10 ==> id 12
[  288.928741] omap3isp 480bc000.isp: media_gobj_destroy id 51: data link id 10 ==> id 12
[  288.937103] omap3isp 480bc000.isp: media_gobj_destroy id 66: data link id 10 ==> id 16
[  288.945465] omap3isp 480bc000.isp: media_gobj_destroy id 65: data link id 10 ==> id 16
[  288.953796] omap3isp 480bc000.isp: media_gobj_destroy id 9: sink pad 'OMAP3 ISP CSI2a':0
[  288.962341] omap3isp 480bc000.isp: media_gobj_destroy id 10: source pad 'OMAP3 ISP CSI2a':1
[  288.971160] omap3isp 480bc000.isp: media_gobj_destroy id 8: entity 'OMAP3 ISP CSI2a'
[  289.117187] omap3isp 480bc000.isp: media_gobj_destroy id 97: intf_devnode v4l-subdev - major: 81, minor: 8
[  289.159820] omap3isp 480bc000.isp: media_gobj_destroy id 96: interface link id 95 ==> id 1
[  289.168640] omap3isp 480bc000.isp: media_gobj_destroy id 53: data link id 5 ==> id 2
[  289.176849] omap3isp 480bc000.isp: media_gobj_destroy id 54: data link id 5 ==> id 2
[  289.185058] omap3isp 480bc000.isp: media_gobj_destroy id 68: data link id 3 ==> id 16
[  289.193298] omap3isp 480bc000.isp: media_gobj_destroy id 67: data link id 3 ==> id 16
[  289.201568] omap3isp 480bc000.isp: media_gobj_destroy id 2: sink pad 'OMAP3 ISP CCP2':0
[  289.210021] omap3isp 480bc000.isp: media_gobj_destroy id 3: source pad 'OMAP3 ISP CCP2':1
[  289.218658] omap3isp 480bc000.isp: media_gobj_destroy id 1: entity 'OMAP3 ISP CCP2'
[  289.402587] omap3isp 480bc000.isp: media_gobj_destroy id 95: intf_devnode v4l-subdev - major: 81, minor: 7
[  289.448974] omap3isp 480bc000.isp: media_gobj_destroy id 7: interface link id 6 ==> id 4
[  289.457641] omap3isp 480bc000.isp: media_gobj_destroy id 6: intf_devnode v4l-video - major: 81, minor: 0
[  289.467773] omap3isp 480bc000.isp: media_gobj_destroy id 5: source pad 'OMAP3 ISP CCP2 input':0
[  289.476959] omap3isp 480bc000.isp: media_gobj_destroy id 4: entity 'OMAP3 ISP CCP2 input'
[  289.485595] omap3isp 480bc000.isp: media_gobj_destroy id 100: interface link id 99 ==> id 15
[  289.494537] omap3isp 480bc000.isp: media_gobj_destroy id 56: data link id 17 ==> id 20
[  289.502868] omap3isp 480bc000.isp: media_gobj_destroy id 55: data link id 17 ==> id 20
[  289.511230] omap3isp 480bc000.isp: media_gobj_destroy id 70: data link id 18 ==> id 24
[  289.519592] omap3isp 480bc000.isp: media_gobj_destroy id 69: data link id 18 ==> id 24
[  289.527954] omap3isp 480bc000.isp: media_gobj_destroy id 72: data link id 17 ==> id 35
[  289.536315] omap3isp 480bc000.isp: media_gobj_destroy id 71: data link id 17 ==> id 35
[  289.544738] omap3isp 480bc000.isp: media_gobj_destroy id 76: data link id 18 ==> id 46
[  289.553070] omap3isp 480bc000.isp: media_gobj_destroy id 75: data link id 18 ==> id 46
[  289.561462] omap3isp 480bc000.isp: media_gobj_destroy id 78: data link id 18 ==> id 48
[  289.569824] omap3isp 480bc000.isp: media_gobj_destroy id 77: data link id 18 ==> id 48
[  289.578186] omap3isp 480bc000.isp: media_gobj_destroy id 80: data link id 18 ==> id 50
[  289.586578] omap3isp 480bc000.isp: media_gobj_destroy id 79: data link id 18 ==> id 50
[  289.594940] omap3isp 480bc000.isp: media_gobj_destroy id 16: sink pad 'OMAP3 ISP CCDC':0
[  289.603454] omap3isp 480bc000.isp: media_gobj_destroy id 17: source pad 'OMAP3 ISP CCDC':1
[  289.612182] omap3isp 480bc000.isp: media_gobj_destroy id 18: source pad 'OMAP3 ISP CCDC':2
[  289.620910] omap3isp 480bc000.isp: media_gobj_destroy id 15: entity 'OMAP3 ISP CCDC'
[  289.979309] omap3isp 480bc000.isp: media_gobj_destroy id 99: intf_devnode v4l-subdev - major: 81, minor: 9
[  290.023925] omap3isp 480bc000.isp: media_gobj_destroy id 22: interface link id 21 ==> id 19
[  290.032867] omap3isp 480bc000.isp: media_gobj_destroy id 21: intf_devnode v4l-video - major: 81, minor: 2
[  290.042968] omap3isp 480bc000.isp: media_gobj_destroy id 20: sink pad 'OMAP3 ISP CCDC output':0
[  290.052154] omap3isp 480bc000.isp: media_gobj_destroy id 19: entity 'OMAP3 ISP CCDC output'
[  290.061004] omap3isp 480bc000.isp: media_gobj_destroy id 102: interface link id 101 ==> id 23
[  290.070007] omap3isp 480bc000.isp: media_gobj_destroy id 57: data link id 27 ==> id 24
[  290.078399] omap3isp 480bc000.isp: media_gobj_destroy id 58: data link id 27 ==> id 24
[  290.086761] omap3isp 480bc000.isp: media_gobj_destroy id 60: data link id 25 ==> id 31
[  290.095123] omap3isp 480bc000.isp: media_gobj_destroy id 59: data link id 25 ==> id 31
[  290.103454] omap3isp 480bc000.isp: media_gobj_destroy id 74: data link id 25 ==> id 35
[  290.111816] omap3isp 480bc000.isp: media_gobj_destroy id 73: data link id 25 ==> id 35
[  290.120208] omap3isp 480bc000.isp: media_gobj_destroy id 24: sink pad 'OMAP3 ISP preview':0
[  290.128997] omap3isp 480bc000.isp: media_gobj_destroy id 25: source pad 'OMAP3 ISP preview':1
[  290.138000] omap3isp 480bc000.isp: media_gobj_destroy id 23: entity 'OMAP3 ISP preview'
[  290.394989] omap3isp 480bc000.isp: media_gobj_destroy id 101: intf_devnode v4l-subdev - major: 81, minor: 10
[  290.440246] omap3isp 480bc000.isp: media_gobj_destroy id 29: interface link id 28 ==> id 26
[  290.449157] omap3isp 480bc000.isp: media_gobj_destroy id 28: intf_devnode v4l-video - major: 81, minor: 3
[  290.459320] omap3isp 480bc000.isp: media_gobj_destroy id 27: source pad 'OMAP3 ISP preview input':0
[  290.468841] omap3isp 480bc000.isp: media_gobj_destroy id 26: entity 'OMAP3 ISP preview input'
[  290.571075] omap3isp 480bc000.isp: media_gobj_destroy id 33: interface link id 32 ==> id 30
[  290.580017] omap3isp 480bc000.isp: media_gobj_destroy id 32: intf_devnode v4l-video - major: 81, minor: 4
[  290.590148] omap3isp 480bc000.isp: media_gobj_destroy id 31: sink pad 'OMAP3 ISP preview output':0
[  290.599578] omap3isp 480bc000.isp: media_gobj_destroy id 30: entity 'OMAP3 ISP preview output'
[  290.608703] omap3isp 480bc000.isp: media_gobj_destroy id 104: interface link id 103 ==> id 34
[  290.617706] omap3isp 480bc000.isp: media_gobj_destroy id 61: data link id 38 ==> id 35
[  290.626068] omap3isp 480bc000.isp: media_gobj_destroy id 62: data link id 38 ==> id 35
[  290.634460] omap3isp 480bc000.isp: media_gobj_destroy id 64: data link id 36 ==> id 42
[  290.642791] omap3isp 480bc000.isp: media_gobj_destroy id 63: data link id 36 ==> id 42
[  290.651153] omap3isp 480bc000.isp: media_gobj_destroy id 35: sink pad 'OMAP3 ISP resizer':0
[  290.659973] omap3isp 480bc000.isp: media_gobj_destroy id 36: source pad 'OMAP3 ISP resizer':1
[  290.668945] omap3isp 480bc000.isp: media_gobj_destroy id 34: entity 'OMAP3 ISP resizer'
[  290.907714] omap3isp 480bc000.isp: media_gobj_destroy id 103: intf_devnode v4l-subdev - major: 81, minor: 11
[  290.952911] omap3isp 480bc000.isp: media_gobj_destroy id 40: interface link id 39 ==> id 37
[  290.961883] omap3isp 480bc000.isp: media_gobj_destroy id 39: intf_devnode v4l-video - major: 81, minor: 5
[  290.972015] omap3isp 480bc000.isp: media_gobj_destroy id 38: source pad 'OMAP3 ISP resizer input':0
[  290.981567] omap3isp 480bc000.isp: media_gobj_destroy id 37: entity 'OMAP3 ISP resizer input'
[  291.085205] omap3isp 480bc000.isp: media_gobj_destroy id 44: interface link id 43 ==> id 41
[  291.094024] omap3isp 480bc000.isp: media_gobj_destroy id 43: intf_devnode v4l-video - major: 81, minor: 6
[  291.104187] omap3isp 480bc000.isp: media_gobj_destroy id 42: sink pad 'OMAP3 ISP resizer output':0
[  291.113616] omap3isp 480bc000.isp: media_gobj_destroy id 41: entity 'OMAP3 ISP resizer output'
[  291.122711] omap3isp 480bc000.isp: media_gobj_destroy id 106: interface link id 105 ==> id 45
[  291.131805] omap3isp 480bc000.isp: media_gobj_destroy id 46: sink pad 'OMAP3 ISP AEWB':0
[  291.140350] omap3isp 480bc000.isp: media_gobj_destroy id 45: entity 'OMAP3 ISP AEWB'
[  291.311187] omap3isp 480bc000.isp: media_gobj_destroy id 105: intf_devnode v4l-subdev - major: 81, minor: 12
[  291.321655] omap3isp 480bc000.isp: media_gobj_destroy id 108: interface link id 107 ==> id 47
[  291.330688] omap3isp 480bc000.isp: media_gobj_destroy id 48: sink pad 'OMAP3 ISP AF':0
[  291.339050] omap3isp 480bc000.isp: media_gobj_destroy id 47: entity 'OMAP3 ISP AF'
[  291.423278] omap3isp 480bc000.isp: media_gobj_destroy id 107: intf_devnode v4l-subdev - major: 81, minor: 13
[  291.433746] omap3isp 480bc000.isp: media_gobj_destroy id 110: interface link id 109 ==> id 49
[  291.442779] omap3isp 480bc000.isp: media_gobj_destroy id 50: sink pad 'OMAP3 ISP histogram':0
[  291.451782] omap3isp 480bc000.isp: media_gobj_destroy id 49: entity 'OMAP3 ISP histogram'
[  291.560607] omap3isp 480bc000.isp: media_gobj_destroy id 109: intf_devnode v4l-subdev - major: 81, minor: 14
[  291.571197] omap3isp 480bc000.isp: media_gobj_destroy id 14: interface link id 13 ==> id 11
[  291.580047] omap3isp 480bc000.isp: media_gobj_destroy id 12: sink pad 'OMAP3 ISP CSI2a output':0
[  291.589324] omap3isp 480bc000.isp: media_gobj_destroy id 11: entity 'OMAP3 ISP CSI2a output'
[  291.598236] omap3isp 480bc000.isp: media_gobj_destroy id 13: intf_devnode v4l-video - major: 81, minor: 1
[  291.608306] omap3isp 480bc000.isp: Media device unregistered
[  291.742919] omap3isp 480bc000.isp: Media device released
[  291.748687] media: media_devnode_release: Media Devnode Deallocated
[  291.755432] omap3isp 480bc000.isp: OMAP3 ISP AEWB: all buffers were freed.
[  291.762756] omap3isp 480bc000.isp: OMAP3 ISP AF: all buffers were freed.
[  291.769897] omap3isp 480bc000.isp: OMAP3 ISP histogram: all buffers were freed.
[  291.863433] clk_unregister: unregistering prepared clock: cam_xclka
[  291.880340] iommu: Removing device 480bc000.isp from group 0
[  294.156921] Unable to handle kernel paging request at virtual address 6b6b6b7b
[  294.164703] pgd = ed9f8000
[  294.167572] [6b6b6b7b] *pgd=00000000
[  294.171386] Internal error: Oops: 5 [#1] ARM
[  294.175933] Modules linked in: smiapp smiapp_pll omap3_isp videobuf2_dma_contig videobuf2_memops videobuf2_v4l2 videobuf2_core v4l2_common videodev media
[  294.190582] CPU: 0 PID: 2264 Comm: yavta Not tainted 4.9.0-rc1-00219-gbd676c0-dirty #734
[  294.199157] Hardware name: Generic OMAP36xx (Flattened Device Tree)
[  294.205810] task: ed820d40 task.stack: ed8bc000
[  294.210845] PC is at v4l2_ioctl+0x2c/0xac [videodev]
[  294.216125] LR is at vfs_ioctl+0x18/0x30
[  294.220306] pc : [<bf00f508>]    lr : [<c01e2684>]    psr: a0000013
               sp : ed8bdef0  ip : 0000545e  fp : be980d94
[  294.232452] r10: 00000000  r9 : ed8bc000  r8 : c044560f
[  294.237976] r7 : be980d94  r6 : ed97a8a8  r5 : ed90d040  r4 : be980d94
[  294.244873] r3 : 6b6b6b6b  r2 : bf027d00  r1 : c044560f  r0 : ed90d040
[  294.251800] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[  294.259338] Control: 10c5387d  Table: ad9f8019  DAC: 00000051
[  294.265441] Process yavta (pid: 2264, stack limit = 0xed8bc208)
[  294.271697] Stack: (0xed8bdef0 to 0xed8be000)
[  294.276336] dee0:                                     be980d94 ed90d040 00000003 c044560f
[  294.285003] df00: 00000003 c01e2684 be980d94 c01e31dc 00000000 00000000 c01f3484 ee367418
[  294.293670] df20: 00000008 ffffffff ffffffff ed88c580 ee369ae0 ee367418 00000008 c01d30f4
[  294.302368] df40: 00000000 00000000 00000000 ed88c678 ee858750 ed821150 ed820d40 00000000
[  294.311035] df60: ed8bdf7c ed8bdf84 ed90d040 ed90d040 be980d94 c044560f 00000003 ed8bc000
[  294.319732] df80: 00000000 c01e3294 00000001 be981368 be98136c 00000036 c0107bc4 ed8bc000
[  294.328399] dfa0: 00000000 c0107a20 00000001 be981368 00000003 c044560f be980d94 00000001
[  294.337066] dfc0: 00000001 be981368 be98136c 00000036 00000000 00000000 00000000 be980d94
[  294.345733] dfe0: 0001716c be980bac 00009a70 b6eaaa5c 60000010 00000003 00000000 00000000
[  294.354553] [<bf00f508>] (v4l2_ioctl [videodev]) from [<c01e2684>] (vfs_ioctl+0x18/0x30)
[  294.363159] [<c01e2684>] (vfs_ioctl) from [<c01e31dc>] (do_vfs_ioctl+0x8c0/0x928)
[  294.371093] [<c01e31dc>] (do_vfs_ioctl) from [<c01e3294>] (SyS_ioctl+0x50/0x6c)
[  294.378845] [<c01e3294>] (SyS_ioctl) from [<c0107a20>] (ret_fast_syscall+0x0/0x1c)
[  294.386901] Code: e3c334ff e3c3360f e7926103 e59630e4 (e5933010) 
[  294.393432] ---[ end trace c1adb47143f9f62b ]---
[  294.499237] Unable to handle kernel paging request at virtual address 6b6b6b8b
[  294.507324] pgd = c0004000
[  294.510223] [6b6b6b8b] *pgd=00000000
[  294.514038] Internal error: Oops: 5 [#2] ARM
[  294.518554] Modules linked in: smiapp smiapp_pll omap3_isp videobuf2_dma_contig videobuf2_memops videobuf2_v4l2 videobuf2_core v4l2_common videodev media
[  294.533203] CPU: 0 PID: 2264 Comm: yavta Tainted: G      D         4.9.0-rc1-00219-gbd676c0-dirty #734
[  294.543060] Hardware name: Generic OMAP36xx (Flattened Device Tree)
[  294.549713] task: ed820d40 task.stack: ed8bc000
[  294.554748] PC is at v4l2_release+0x20/0x70 [videodev]
[  294.560211] LR is at __fput+0xe0/0x1b8
[  294.564178] pc : [<bf00f3b4>]    lr : [<c01d302c>]    psr: a0000013
               sp : ed8bdc50  ip : ed8bc000  fp : ee84f110
[  294.576324] r10: ed85f130  r9 : 00000000  r8 : ed90d048
[  294.581878] r7 : 00000008  r6 : ee342c78  r5 : ed85f130  r4 : ed97a8a8
[  294.588775] r3 : 6b6b6b6b  r2 : bf027d00  r1 : ed90d040  r0 : ed85f130
[  294.595703] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[  294.603240] Control: 10c5387d  Table: ad984019  DAC: 00000051
[  294.609344] Process yavta (pid: 2264, stack limit = 0xed8bc208)
[  294.615600] Stack: (0xed8bdc50 to 0xed8be000)
[  294.620239] dc40:                                     ed90d040 ed85f130 ee342c78 c01d302c
[  294.628906] dc60: 00000000 00000000 00000001 ed90d138 00000000 ed821150 ed820d40 ed90d2c0
[  294.637573] dc80: ed8bdc9c ed8bdca4 00000000 c061262c 00000008 c0145a60 ed821160 00000001
[  294.646240] dca0: 00000001 ed90d040 ed820d40 ed821160 ed8a0480 00000001 ed8a04d4 c012d41c
[  294.654937] dcc0: 00000008 60000113 00000000 00000002 c082f0e4 c0ff2b90 0000230e 00000015
[  294.663604] dce0: 00000000 00000000 00000000 00000001 00000001 00000000 c0165274 ed8bdd1c
[  294.672302] dd00: 43f9f62b c082f0e4 c061d27a 0000000b c0806b78 00000001 00000000 bf00f50c
[  294.680969] dd20: 00000000 c061262c 00000008 c010b7a0 ed8bc208 0000000b bf000000 60000113
[  294.689636] dd40: 00000000 00000000 66663433 63336520 30363333 37652066 31363239 65203330
[  294.698303] dd60: 33363935 20346530 39356528 31303333 00202930 c016711c c061355a ed8bdd9c
[  294.707000] dd80: ed8bdd8c 6b6b6b7b ed8bdea0 00000000 00000005 ed8a0480 ed8a04d4 ed8a0480
[  294.715667] dda0: 00010000 c0111580 ed8bdea0 6b6b6b7b ed820d40 00000014 00000005 c01117f8
[  294.724334] ddc0: 00000000 c085e8dc c0834a78 00000000 00000007 00000000 eeb7f9c0 00000005
[  294.733001] dde0: 6b6b6b7b c0806e1c ed8bdea0 c044560f ed8bc000 00000000 be980d94 c010136c
[  294.741668] de00: 0000007c 00000000 00000001 c015db90 ed820d40 ed821218 00000001 00000000
[  294.750366] de20: 0000021b 00000000 00000001 c015db90 0000007c 00000000 bfc3755a 748d35fb
[  294.759033] de40: c0c5b8ac 00000001 582cb09f 89c973bb ed820d40 ed8211f8 00000000 00000000
[  294.767700] de60: 00000050 00000000 00000000 c015db90 ee36748c 00000001 1db89764 17cc08f3
[  294.776397] de80: ed821218 eeb1c390 ed820d60 bf00f508 a0000013 ffffffff ed8bded4 c010bcbc
[  294.785064] dea0: ed90d040 c044560f bf027d00 6b6b6b6b be980d94 ed90d040 ed97a8a8 be980d94
[  294.793731] dec0: c044560f ed8bc000 00000000 be980d94 0000545e ed8bdef0 c01e2684 bf00f508
[  294.802398] dee0: a0000013 ffffffff 00000051 bf000000 be980d94 ed90d040 00000003 c044560f
[  294.811096] df00: 00000003 c01e2684 be980d94 c01e31dc 00000000 00000000 c01f3484 ee367418
[  294.819763] df20: 00000008 ffffffff ffffffff ed88c580 ee369ae0 ee367418 00000008 c01d30f4
[  294.828430] df40: 00000000 00000000 00000000 ed88c678 ee858750 ed821150 ed820d40 00000000
[  294.837127] df60: ed8bdf7c ed8bdf84 ed90d040 ed90d040 be980d94 c044560f 00000003 ed8bc000
[  294.845794] df80: 00000000 c01e3294 00000001 be981368 be98136c 00000036 c0107bc4 ed8bc000
[  294.854461] dfa0: 00000000 c0107a20 00000001 be981368 00000003 c044560f be980d94 00000001
[  294.863159] dfc0: 00000001 be981368 be98136c 00000036 00000000 00000000 00000000 be980d94
[  294.871826] dfe0: 0001716c be980bac 00009a70 b6eaaa5c 60000010 00000003 00000000 00000000
[  294.880645] [<bf00f3b4>] (v4l2_release [videodev]) from [<c01d302c>] (__fput+0xe0/0x1b8)
[  294.889251] [<c01d302c>] (__fput) from [<c0145a60>] (task_work_run+0xa4/0xbc)
[  294.896789] [<c0145a60>] (task_work_run) from [<c012d41c>] (do_exit+0x468/0x558)
[  294.904663] [<c012d41c>] (do_exit) from [<c010b7a0>] (die+0x38c/0x3e0)
[  294.911590] [<c010b7a0>] (die) from [<c0111580>] (__do_kernel_fault+0x64/0x84)
[  294.919281] [<c0111580>] (__do_kernel_fault) from [<c01117f8>] (do_page_fault+0x258/0x270)
[  294.928039] [<c01117f8>] (do_page_fault) from [<c010136c>] (do_DataAbort+0x34/0xb0)
[  294.936157] [<c010136c>] (do_DataAbort) from [<c010bcbc>] (__dabt_svc+0x5c/0xa0)
[  294.944000] Exception stack(0xed8bdea0 to 0xed8bdee8)
[  294.949371] dea0: ed90d040 c044560f bf027d00 6b6b6b6b be980d94 ed90d040 ed97a8a8 be980d94
[  294.958038] dec0: c044560f ed8bc000 00000000 be980d94 0000545e ed8bdef0 c01e2684 bf00f508
[  294.966735] dee0: a0000013 ffffffff
[  294.970550] [<c010bcbc>] (__dabt_svc) from [<bf00f508>] (v4l2_ioctl+0x2c/0xac [videodev])
[  294.979339] [<bf00f508>] (v4l2_ioctl [videodev]) from [<c01e2684>] (vfs_ioctl+0x18/0x30)
[  294.987915] [<c01e2684>] (vfs_ioctl) from [<c01e31dc>] (do_vfs_ioctl+0x8c0/0x928)
[  294.995880] [<c01e31dc>] (do_vfs_ioctl) from [<c01e3294>] (SyS_ioctl+0x50/0x6c)
[  295.003631] [<c01e3294>] (SyS_ioctl) from [<c0107a20>] (ret_fast_syscall+0x0/0x1c)
[  295.011657] Code: e3c334ff e3c3360f e7924103 e59430e4 (e5935020) 
[  295.018402] ---[ end trace c1adb47143f9f62c ]---
[  295.023284] Fixing recursive fault but reboot is needed!
-----------------8<----------------------

changes since v3:

- Rebased on top of current media-tree.git master.

changes since v2:

- Rework the set in order to make the changes more consistent, easier to
  understand and better ordered.

- Properly change referencing media_dev->dev (patch "media device: Get the
  media device driver's device" added).

- Only set the release() callback to media device if the new
  media_device_alloc() API is used. (The callback just printed a debug
  message before this series.)

- Call cdev_del() before removing the device (patch 7).

- Document media_device_init() and media_device_cleanup() as deprecated.

- Spelling fixes.

The to-do list includes changes to drivers that can be physically removed.
Drivers not using the new API can mostly ignore these changes, albeit
media_device_init() now grabs a reference to struct device of the media
device which must be released.

-- 
Kind regards,
Sakari

[RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race"

[Sakari Ailus]

This reverts commit 6f0dd24a084a ("[media] media: fix media devnode
ioctl/syscall and unregister race"). The commit was part of an original
patchset to avoid crashes when an unregistering device is in use.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c  | 15 +++++++--------
 drivers/media/media-devnode.c |  8 +-------
 include/media/media-devnode.h | 16 ++--------------
 3 files changed, 10 insertions(+), 29 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 2783531..f2525eb 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -730,7 +730,6 @@ int __must_check __media_device_register(struct media_device *mdev,
 	if (ret < 0) {
 		/* devnode free is handled in media_devnode_*() */
 		mdev->devnode = NULL;
-		media_devnode_unregister_prepare(devnode);
 		media_devnode_unregister(devnode);
 		return ret;
 	}
@@ -787,9 +786,6 @@ void media_device_unregister(struct media_device *mdev)
 		return;
 	}
 
-	/* Clear the devnode register bit to avoid races with media dev open */
-	media_devnode_unregister_prepare(mdev->devnode);
-
 	/* Remove all entities from the media device */
 	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
 		__media_device_unregister_entity(entity);
@@ -810,10 +806,13 @@ void media_device_unregister(struct media_device *mdev)
 
 	dev_dbg(mdev->dev, "Media device unregistered\n");
 
-	device_remove_file(&mdev->devnode->dev, &dev_attr_model);
-	media_devnode_unregister(mdev->devnode);
-	/* devnode free is handled in media_devnode_*() */
-	mdev->devnode = NULL;
+	/* Check if mdev devnode was registered */
+	if (media_devnode_is_registered(mdev->devnode)) {
+		device_remove_file(&mdev->devnode->dev, &dev_attr_model);
+		media_devnode_unregister(mdev->devnode);
+		/* devnode free is handled in media_devnode_*() */
+		mdev->devnode = NULL;
+	}
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index f2772ba..5b605ff 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -287,7 +287,7 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	return ret;
 }
 
-void media_devnode_unregister_prepare(struct media_devnode *devnode)
+void media_devnode_unregister(struct media_devnode *devnode)
 {
 	/* Check if devnode was ever registered at all */
 	if (!media_devnode_is_registered(devnode))
@@ -295,12 +295,6 @@ void media_devnode_unregister_prepare(struct media_devnode *devnode)
 
 	mutex_lock(&media_devnode_lock);
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
-	mutex_unlock(&media_devnode_lock);
-}
-
-void media_devnode_unregister(struct media_devnode *devnode)
-{
-	mutex_lock(&media_devnode_lock);
 	/* Delete the cdev on this minor as well */
 	cdev_del(&devnode->cdev);
 	mutex_unlock(&media_devnode_lock);
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index cd23e91..d55ec2b 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -128,26 +128,14 @@ int __must_check media_devnode_register(struct media_device *mdev,
 					struct module *owner);
 
 /**
- * media_devnode_unregister_prepare - clear the media device node register bit
- * @devnode: the device node to prepare for unregister
- *
- * This clears the passed device register bit. Future open calls will be met
- * with errors. Should be called before media_devnode_unregister() to avoid
- * races with unregister and device file open calls.
- *
- * This function can safely be called if the device node has never been
- * registered or has already been unregistered.
- */
-void media_devnode_unregister_prepare(struct media_devnode *devnode);
-
-/**
  * media_devnode_unregister - unregister a media device node
  * @devnode: the device node to unregister
  *
  * This unregisters the passed device. Future open calls will be met with
  * errors.
  *
- * Should be called after media_devnode_unregister_prepare()
+ * This function can safely be called if the device node has never been
+ * registered or has already been unregistered.
  */
 void media_devnode_unregister(struct media_devnode *devnode);
 
-- 
2.1.4

[RFC v4 02/21] Revert "[media] media: fix use-after-free in cdev_put() when app exits after driver unbind"

[Sakari Ailus]

This reverts commit 5b28dde51d0c ("[media] media: fix use-after-free in
cdev_put() when app exits after driver unbind"). The commit was part of an
original patchset to avoid crashes when an unregistering device is in use.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c  |  6 ++----
 drivers/media/media-devnode.c | 48 +++++++++++++++++--------------------------
 2 files changed, 21 insertions(+), 33 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index f2525eb..6f5ed09 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -721,16 +721,16 @@ int __must_check __media_device_register(struct media_device *mdev,
 
 	ret = media_devnode_register(mdev, devnode, owner);
 	if (ret < 0) {
-		/* devnode free is handled in media_devnode_*() */
 		mdev->devnode = NULL;
+		kfree(devnode);
 		return ret;
 	}
 
 	ret = device_create_file(&devnode->dev, &dev_attr_model);
 	if (ret < 0) {
-		/* devnode free is handled in media_devnode_*() */
 		mdev->devnode = NULL;
 		media_devnode_unregister(devnode);
+		kfree(devnode);
 		return ret;
 	}
 
@@ -810,8 +810,6 @@ void media_device_unregister(struct media_device *mdev)
 	if (media_devnode_is_registered(mdev->devnode)) {
 		device_remove_file(&mdev->devnode->dev, &dev_attr_model);
 		media_devnode_unregister(mdev->devnode);
-		/* devnode free is handled in media_devnode_*() */
-		mdev->devnode = NULL;
 	}
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index 5b605ff..ecdc02d 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -63,8 +63,13 @@ static void media_devnode_release(struct device *cd)
 	struct media_devnode *devnode = to_media_devnode(cd);
 
 	mutex_lock(&media_devnode_lock);
+
+	/* Delete the cdev on this minor as well */
+	cdev_del(&devnode->cdev);
+
 	/* Mark device node number as free */
 	clear_bit(devnode->minor, media_devnode_nums);
+
 	mutex_unlock(&media_devnode_lock);
 
 	/* Release media_devnode and perform other cleanups as needed. */
@@ -72,7 +77,6 @@ static void media_devnode_release(struct device *cd)
 		devnode->release(devnode);
 
 	kfree(devnode);
-	pr_debug("%s: Media Devnode Deallocated\n", __func__);
 }
 
 static struct bus_type media_bus_type = {
@@ -201,8 +205,6 @@ static int media_release(struct inode *inode, struct file *filp)
 	/* decrease the refcount unconditionally since the release()
 	   return value is ignored. */
 	put_device(&devnode->dev);
-
-	pr_debug("%s: Media Release\n", __func__);
 	return 0;
 }
 
@@ -233,7 +235,6 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	if (minor == MEDIA_NUM_DEVICES) {
 		mutex_unlock(&media_devnode_lock);
 		pr_err("could not get a free minor\n");
-		kfree(devnode);
 		return -ENFILE;
 	}
 
@@ -243,31 +244,27 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	devnode->minor = minor;
 	devnode->media_dev = mdev;
 
-	/* Part 1: Initialize dev now to use dev.kobj for cdev.kobj.parent */
-	devnode->dev.bus = &media_bus_type;
-	devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
-	devnode->dev.release = media_devnode_release;
-	if (devnode->parent)
-		devnode->dev.parent = devnode->parent;
-	dev_set_name(&devnode->dev, "media%d", devnode->minor);
-	device_initialize(&devnode->dev);
-
 	/* Part 2: Initialize and register the character device */
 	cdev_init(&devnode->cdev, &media_devnode_fops);
 	devnode->cdev.owner = owner;
-	devnode->cdev.kobj.parent = &devnode->dev.kobj;
 
 	ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t), devnode->minor), 1);
 	if (ret < 0) {
 		pr_err("%s: cdev_add failed\n", __func__);
-		goto cdev_add_error;
+		goto error;
 	}
 
-	/* Part 3: Add the media device */
-	ret = device_add(&devnode->dev);
+	/* Part 3: Register the media device */
+	devnode->dev.bus = &media_bus_type;
+	devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
+	devnode->dev.release = media_devnode_release;
+	if (devnode->parent)
+		devnode->dev.parent = devnode->parent;
+	dev_set_name(&devnode->dev, "media%d", devnode->minor);
+	ret = device_register(&devnode->dev);
 	if (ret < 0) {
-		pr_err("%s: device_add failed\n", __func__);
-		goto device_add_error;
+		pr_err("%s: device_register failed\n", __func__);
+		goto error;
 	}
 
 	/* Part 4: Activate this minor. The char device can now be used. */
@@ -275,15 +272,12 @@ int __must_check media_devnode_register(struct media_device *mdev,
 
 	return 0;
 
-device_add_error:
-	cdev_del(&devnode->cdev);
-cdev_add_error:
+error:
 	mutex_lock(&media_devnode_lock);
+	cdev_del(&devnode->cdev);
 	clear_bit(devnode->minor, media_devnode_nums);
-	devnode->media_dev = NULL;
 	mutex_unlock(&media_devnode_lock);
 
-	put_device(&devnode->dev);
 	return ret;
 }
 
@@ -295,12 +289,8 @@ void media_devnode_unregister(struct media_devnode *devnode)
 
 	mutex_lock(&media_devnode_lock);
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
-	/* Delete the cdev on this minor as well */
-	cdev_del(&devnode->cdev);
 	mutex_unlock(&media_devnode_lock);
-	device_del(&devnode->dev);
-	devnode->media_dev = NULL;
-	put_device(&devnode->dev);
+	device_unregister(&devnode->dev);
 }
 
 /*
-- 
2.1.4

[RFC v4 03/21] Revert "[media] media-device: dynamically allocate struct media_devnode"

[Sakari Ailus]

This reverts commit a087ce704b80 ("[media] media-device: dynamically
allocate struct media_devnode"). The commit was part of an original
patchset to avoid crashes when an unregistering device is in use.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c           | 44 +++++++++++-----------------------
 drivers/media/media-devnode.c          |  7 +-----
 drivers/media/usb/au0828/au0828-core.c |  4 ++--
 drivers/media/usb/uvc/uvc_driver.c     |  2 +-
 include/media/media-device.h           |  5 +++-
 include/media/media-devnode.h          | 15 ++----------
 6 files changed, 24 insertions(+), 53 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 6f5ed09..0e07300 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -417,7 +417,7 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
 			       unsigned long __arg)
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
-	struct media_device *dev = devnode->media_dev;
+	struct media_device *dev = to_media_device(devnode);
 	const struct media_ioctl_info *info;
 	void __user *arg = (void __user *)__arg;
 	char __karg[256], *karg = __karg;
@@ -493,7 +493,7 @@ static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
 				      unsigned long arg)
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
-	struct media_device *dev = devnode->media_dev;
+	struct media_device *dev = to_media_device(devnode);
 	long ret;
 
 	switch (cmd) {
@@ -529,8 +529,7 @@ static const struct media_file_operations media_device_fops = {
 static ssize_t show_model(struct device *cd,
 			  struct device_attribute *attr, char *buf)
 {
-	struct media_devnode *devnode = to_media_devnode(cd);
-	struct media_device *mdev = devnode->media_dev;
+	struct media_device *mdev = to_media_device(to_media_devnode(cd));
 
 	return sprintf(buf, "%.*s\n", (int)sizeof(mdev->model), mdev->model);
 }
@@ -703,34 +702,23 @@ EXPORT_SYMBOL_GPL(media_device_cleanup);
 int __must_check __media_device_register(struct media_device *mdev,
 					 struct module *owner)
 {
-	struct media_devnode *devnode;
 	int ret;
 
-	devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
-	if (!devnode)
-		return -ENOMEM;
-
 	/* Register the device node. */
-	mdev->devnode = devnode;
-	devnode->fops = &media_device_fops;
-	devnode->parent = mdev->dev;
-	devnode->release = media_device_release;
+	mdev->devnode.fops = &media_device_fops;
+	mdev->devnode.parent = mdev->dev;
+	mdev->devnode.release = media_device_release;
 
 	/* Set version 0 to indicate user-space that the graph is static */
 	mdev->topology_version = 0;
 
-	ret = media_devnode_register(mdev, devnode, owner);
-	if (ret < 0) {
-		mdev->devnode = NULL;
-		kfree(devnode);
+	ret = media_devnode_register(&mdev->devnode, owner);
+	if (ret < 0)
 		return ret;
-	}
 
-	ret = device_create_file(&devnode->dev, &dev_attr_model);
+	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
 	if (ret < 0) {
-		mdev->devnode = NULL;
-		media_devnode_unregister(devnode);
-		kfree(devnode);
+		media_devnode_unregister(&mdev->devnode);
 		return ret;
 	}
 
@@ -781,7 +769,7 @@ void media_device_unregister(struct media_device *mdev)
 	mutex_lock(&mdev->graph_mutex);
 
 	/* Check if mdev was ever registered at all */
-	if (!media_devnode_is_registered(mdev->devnode)) {
+	if (!media_devnode_is_registered(&mdev->devnode)) {
 		mutex_unlock(&mdev->graph_mutex);
 		return;
 	}
@@ -804,13 +792,9 @@ void media_device_unregister(struct media_device *mdev)
 
 	mutex_unlock(&mdev->graph_mutex);
 
-	dev_dbg(mdev->dev, "Media device unregistered\n");
-
-	/* Check if mdev devnode was registered */
-	if (media_devnode_is_registered(mdev->devnode)) {
-		device_remove_file(&mdev->devnode->dev, &dev_attr_model);
-		media_devnode_unregister(mdev->devnode);
-	}
+	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
+	dev_dbg(mdev->dev, "Media device unregistering\n");
+	media_devnode_unregister(&mdev->devnode);
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index ecdc02d..7481c96 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -44,7 +44,6 @@
 #include <linux/uaccess.h>
 
 #include <media/media-devnode.h>
-#include <media/media-device.h>
 
 #define MEDIA_NUM_DEVICES	256
 #define MEDIA_NAME		"media"
@@ -75,8 +74,6 @@ static void media_devnode_release(struct device *cd)
 	/* Release media_devnode and perform other cleanups as needed. */
 	if (devnode->release)
 		devnode->release(devnode);
-
-	kfree(devnode);
 }
 
 static struct bus_type media_bus_type = {
@@ -222,8 +219,7 @@ static const struct file_operations media_devnode_fops = {
 	.llseek = no_llseek,
 };
 
-int __must_check media_devnode_register(struct media_device *mdev,
-					struct media_devnode *devnode,
+int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner)
 {
 	int minor;
@@ -242,7 +238,6 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	mutex_unlock(&media_devnode_lock);
 
 	devnode->minor = minor;
-	devnode->media_dev = mdev;
 
 	/* Part 2: Initialize and register the character device */
 	cdev_init(&devnode->cdev, &media_devnode_fops);
diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index bf53553..321ea5c 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -142,7 +142,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
 	struct media_device *mdev = dev->media_dev;
 	struct media_entity_notify *notify, *nextp;
 
-	if (!mdev || !media_devnode_is_registered(mdev->devnode))
+	if (!mdev || !media_devnode_is_registered(&mdev->devnode))
 		return;
 
 	/* Remove au0828 entity_notify callbacks */
@@ -482,7 +482,7 @@ static int au0828_media_device_register(struct au0828_dev *dev,
 	if (!dev->media_dev)
 		return 0;
 
-	if (!media_devnode_is_registered(dev->media_dev->devnode)) {
+	if (!media_devnode_is_registered(&dev->media_dev->devnode)) {
 
 		/* register media device */
 		ret = media_device_register(dev->media_dev);
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 87b2fc3b..52cdef6 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1780,7 +1780,7 @@ static void uvc_delete(struct uvc_device *dev)
 	if (dev->vdev.dev)
 		v4l2_device_unregister(&dev->vdev);
 #ifdef CONFIG_MEDIA_CONTROLLER
-	if (media_devnode_is_registered(dev->mdev.devnode))
+	if (media_devnode_is_registered(&dev->mdev.devnode))
 		media_device_unregister(&dev->mdev);
 	media_device_cleanup(&dev->mdev);
 #endif
diff --git a/include/media/media-device.h b/include/media/media-device.h
index ef93e21..96de915 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -127,7 +127,7 @@ struct media_device_ops {
 struct media_device {
 	/* dev->driver_data points to this struct. */
 	struct device *dev;
-	struct media_devnode *devnode;
+	struct media_devnode devnode;
 
 	char model[32];
 	char driver_name[32];
@@ -172,6 +172,9 @@ struct usb_device;
 #define MEDIA_DEV_NOTIFY_PRE_LINK_CH	0
 #define MEDIA_DEV_NOTIFY_POST_LINK_CH	1
 
+/* media_devnode to media_device */
+#define to_media_device(node) container_of(node, struct media_device, devnode)
+
 /**
  * media_entity_enum_init - Initialise an entity enumeration
  *
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index d55ec2b..a68b4b6 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -33,8 +33,6 @@
 #include <linux/device.h>
 #include <linux/cdev.h>
 
-struct media_device;
-
 /*
  * Flag to mark the media_devnode struct as registered. Drivers must not touch
  * this flag directly, it will be set and cleared by media_devnode_register and
@@ -85,8 +83,6 @@ struct media_file_operations {
  * before registering the node.
  */
 struct media_devnode {
-	struct media_device *media_dev;
-
 	/* device ops */
 	const struct media_file_operations *fops;
 
@@ -109,8 +105,7 @@ struct media_devnode {
 /**
  * media_devnode_register - register a media device node
  *
- * @mdev: struct media_device we want to register a device node
- * @devnode: media device node structure we want to register
+ * @devnode: struct media_devnode we want to register a device node
  * @owner: should be filled with %THIS_MODULE
  *
  * The registration code assigns minor numbers and registers the new device node
@@ -123,8 +118,7 @@ struct media_devnode {
  * the media_devnode structure is *not* called, so the caller is responsible for
  * freeing any data.
  */
-int __must_check media_devnode_register(struct media_device *mdev,
-					struct media_devnode *devnode,
+int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner);
 
 /**
@@ -154,14 +148,9 @@ static inline struct media_devnode *media_devnode_data(struct file *filp)
  *	false otherwise.
  *
  * @devnode: pointer to struct &media_devnode.
- *
- * Note: If mdev is NULL, it also returns false.
  */
 static inline int media_devnode_is_registered(struct media_devnode *devnode)
 {
-	if (!devnode)
-		return false;
-
 	return test_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 }
 
-- 
2.1.4

[RFC v4 04/21] media: Remove useless curly braces and parentheses

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-device.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 0e07300..bb19c04 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -594,9 +594,8 @@ int __must_check media_device_register_entity(struct media_device *mdev,
 			       &entity->pads[i].graph_obj);
 
 	/* invoke entity_notify callbacks */
-	list_for_each_entry_safe(notify, next, &mdev->entity_notify, list) {
-		(notify)->notify(entity, notify->notify_data);
-	}
+	list_for_each_entry_safe(notify, next, &mdev->entity_notify, list)
+		notify->notify(entity, notify->notify_data);
 
 	if (mdev->entity_internal_idx_max
 	    >= mdev->pm_count_walk.ent_enum.idx_max) {
-- 
2.1.4

[RFC v4 05/21] media: devnode: Rename mdev argument as devnode

[Sakari Ailus]

Historically, mdev argument name was being used on both struct
media_device and struct media_devnode. Recently most occurrences of mdev
referring to struct media_devnode were replaced by devnode, which makes
more sense. Fix the last remaining occurrence.

Fixes: 163f1e93e9950 ("[media] media-devnode: fix namespace mess")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index bb19c04..a9d543f 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -540,9 +540,9 @@ static DEVICE_ATTR(model, S_IRUGO, show_model, NULL);
  * Registration/unregistration
  */
 
-static void media_device_release(struct media_devnode *mdev)
+static void media_device_release(struct media_devnode *devnode)
 {
-	dev_dbg(mdev->parent, "Media device released\n");
+	dev_dbg(devnode->parent, "Media device released\n");
 }
 
 /**
-- 
2.1.4

[RFC v4 06/21] media device: Drop nop release callback

[Sakari Ailus]

The release callback is only used to print a debug message. Drop it. (It
will be re-introduced later in a different form.)

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index a9d543f..a31329d 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -540,11 +540,6 @@ static DEVICE_ATTR(model, S_IRUGO, show_model, NULL);
  * Registration/unregistration
  */
 
-static void media_device_release(struct media_devnode *devnode)
-{
-	dev_dbg(devnode->parent, "Media device released\n");
-}
-
 /**
  * media_device_register_entity - Register an entity with a media device
  * @mdev:	The media device
@@ -706,7 +701,6 @@ int __must_check __media_device_register(struct media_device *mdev,
 	/* Register the device node. */
 	mdev->devnode.fops = &media_device_fops;
 	mdev->devnode.parent = mdev->dev;
-	mdev->devnode.release = media_device_release;
 
 	/* Set version 0 to indicate user-space that the graph is static */
 	mdev->topology_version = 0;
-- 
2.1.4

[RFC v4 07/21] media-device: Make devnode.dev->kobj parent of devnode.cdev

[Sakari Ailus]

The struct cdev embedded in struct media_devnode contains its own kobj.
Instead of trying to manage its lifetime separately from struct
media_devnode, make the cdev kobj a parent of the struct media_device.dev
kobj.

The cdev will thus be released during unregistering the media_devnode, not
in media_devnode.dev kobj's release callback.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-devnode.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index 7481c96..a8302fc 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -63,9 +63,6 @@ static void media_devnode_release(struct device *cd)
 
 	mutex_lock(&media_devnode_lock);
 
-	/* Delete the cdev on this minor as well */
-	cdev_del(&devnode->cdev);
-
 	/* Mark device node number as free */
 	clear_bit(devnode->minor, media_devnode_nums);
 
@@ -241,6 +238,7 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
 
 	/* Part 2: Initialize and register the character device */
 	cdev_init(&devnode->cdev, &media_devnode_fops);
+	devnode->cdev.kobj.parent = &devnode->dev.kobj;
 	devnode->cdev.owner = owner;
 
 	ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t), devnode->minor), 1);
@@ -285,6 +283,7 @@ void media_devnode_unregister(struct media_devnode *devnode)
 	mutex_lock(&media_devnode_lock);
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 	mutex_unlock(&media_devnode_lock);
+	cdev_del(&devnode->cdev);
 	device_unregister(&devnode->dev);
 }
 
-- 
2.1.4

[RFC v4 08/21] media: Enable allocating the media device dynamically

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@iki.fi>

Allow allocating the media device dynamically. As the struct media_device
embeds struct media_devnode, the lifetime of that object is that same than
that of the media_device.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 15 +++++++++++++++
 include/media/media-device.h | 13 +++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index a31329d..496195e 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
 }
 EXPORT_SYMBOL_GPL(media_device_init);
 
+struct media_device *media_device_alloc(struct device *dev)
+{
+	struct media_device *mdev;
+
+	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
+	if (!mdev)
+		return NULL;
+
+	mdev->dev = dev;
+	media_device_init(mdev);
+
+	return mdev;
+}
+EXPORT_SYMBOL_GPL(media_device_alloc);
+
 void media_device_cleanup(struct media_device *mdev)
 {
 	ida_destroy(&mdev->entity_internal_idx);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 96de915..c9b5798 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -207,6 +207,15 @@ static inline __must_check int media_entity_enum_init(
 void media_device_init(struct media_device *mdev);
 
 /**
+ * media_device_alloc() - Allocate and initialise a media device
+ *
+ * @dev:	The associated struct device pointer
+ *
+ * Allocate and initialise a media device. Returns a media device.
+ */
+struct media_device *media_device_alloc(struct device *dev);
+
+/**
  * media_device_cleanup() - Cleanups a media device element
  *
  * @mdev:	pointer to struct &media_device
@@ -451,6 +460,10 @@ void __media_device_usb_init(struct media_device *mdev,
 			     const char *driver_name);
 
 #else
+static inline struct media_device *media_device_alloc(struct device *dev)
+{
+	return NULL;
+}
 static inline int media_device_register(struct media_device *mdev)
 {
 	return 0;
-- 
2.1.4

[RFC v4 09/21] media: Split initialising and adding media devnode

[Sakari Ailus]

As registering a device node of an entity belonging to a media device
will require a reference to the struct device. Taking that reference is
only possible once the device has been initialised, which took place only
when it was registered. Split this in two, and initialise the device when
the media device is allocated.

Don't distribute the effects of these changes yet. Add media_device_get()
and media_device_put() first.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c  | 18 +++++++++++++-----
 drivers/media/media-devnode.c | 17 +++++++++++------
 include/media/media-devnode.h | 19 ++++++++++++++-----
 3 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 496195e..c79a9f5 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -720,19 +720,26 @@ int __must_check __media_device_register(struct media_device *mdev,
 	/* Set version 0 to indicate user-space that the graph is static */
 	mdev->topology_version = 0;
 
+	media_devnode_init(&mdev->devnode);
+
 	ret = media_devnode_register(&mdev->devnode, owner);
 	if (ret < 0)
-		return ret;
+		goto out_put;
 
 	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
-	if (ret < 0) {
-		media_devnode_unregister(&mdev->devnode);
-		return ret;
-	}
+	if (ret < 0)
+		goto out_unregister;
 
 	dev_dbg(mdev->dev, "Media device registered\n");
 
 	return 0;
+
+out_unregister:
+	media_devnode_unregister(&mdev->devnode);
+out_put:
+	put_device(&mdev->devnode.dev);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(__media_device_register);
 
@@ -803,6 +810,7 @@ void media_device_unregister(struct media_device *mdev)
 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
 	dev_dbg(mdev->dev, "Media device unregistering\n");
 	media_devnode_unregister(&mdev->devnode);
+	put_device(&mdev->devnode.dev);
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index a8302fc..178d692 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -216,6 +216,11 @@ static const struct file_operations media_devnode_fops = {
 	.llseek = no_llseek,
 };
 
+void media_devnode_init(struct media_devnode *devnode)
+{
+	device_initialize(&devnode->dev);
+}
+
 int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner)
 {
@@ -254,7 +259,7 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
 	if (devnode->parent)
 		devnode->dev.parent = devnode->parent;
 	dev_set_name(&devnode->dev, "media%d", devnode->minor);
-	ret = device_register(&devnode->dev);
+	ret = device_add(&devnode->dev);
 	if (ret < 0) {
 		pr_err("%s: device_register failed\n", __func__);
 		goto error;
@@ -284,13 +289,13 @@ void media_devnode_unregister(struct media_devnode *devnode)
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 	mutex_unlock(&media_devnode_lock);
 	cdev_del(&devnode->cdev);
-	device_unregister(&devnode->dev);
+	device_del(&devnode->dev);
 }
 
 /*
  *	Initialise media for linux
  */
-static int __init media_devnode_init(void)
+static int __init media_devnode_module_init(void)
 {
 	int ret;
 
@@ -312,14 +317,14 @@ static int __init media_devnode_init(void)
 	return 0;
 }
 
-static void __exit media_devnode_exit(void)
+static void __exit media_devnode_module_exit(void)
 {
 	bus_unregister(&media_bus_type);
 	unregister_chrdev_region(media_dev_t, MEDIA_NUM_DEVICES);
 }
 
-subsys_initcall(media_devnode_init);
-module_exit(media_devnode_exit)
+subsys_initcall(media_devnode_module_init);
+module_exit(media_devnode_module_exit)
 
 MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
 MODULE_DESCRIPTION("Device node registration for media drivers");
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index a68b4b6..ffa2d18 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -103,6 +103,17 @@ struct media_devnode {
 #define to_media_devnode(cd) container_of(cd, struct media_devnode, dev)
 
 /**
+ * media_devnode_init - initialise a media devnode
+ *
+ * @devnode: struct media_devnode we want to initialise
+ *
+ * Initialise a media devnode. Note that after initialising the media
+ * devnode is refcounted. Releasing references to it may be done using
+ * put_device().
+ */
+void media_devnode_init(struct media_devnode *devnode);
+
+/**
  * media_devnode_register - register a media device node
  *
  * @devnode: struct media_devnode we want to register a device node
@@ -112,11 +123,9 @@ struct media_devnode {
  * with the kernel. An error is returned if no free minor number can be found,
  * or if the registration of the device node fails.
  *
- * Zero is returned on success.
- *
- * Note that if the media_devnode_register call fails, the release() callback of
- * the media_devnode structure is *not* called, so the caller is responsible for
- * freeing any data.
+ * Zero is returned on success. Note that in case
+ * media_devnode_register() fails, the caller is responsible for
+ * releasing the reference to the device using put_device().
  */
 int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner);
-- 
2.1.4

[RFC v4 10/21] media: Shuffle functions around

[Sakari Ailus]

As the call paths of the functions in question will change, move them
around in anticipation of that. No other changes.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-device.c | 56 ++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index c79a9f5..0920c02 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -660,6 +660,34 @@ void media_device_unregister_entity(struct media_entity *entity)
 }
 EXPORT_SYMBOL_GPL(media_device_unregister_entity);
 
+int __must_check media_device_register_entity_notify(struct media_device *mdev,
+					struct media_entity_notify *nptr)
+{
+	mutex_lock(&mdev->graph_mutex);
+	list_add_tail(&nptr->list, &mdev->entity_notify);
+	mutex_unlock(&mdev->graph_mutex);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(media_device_register_entity_notify);
+
+/*
+ * Note: Should be called with mdev->lock held.
+ */
+static void __media_device_unregister_entity_notify(struct media_device *mdev,
+					struct media_entity_notify *nptr)
+{
+	list_del(&nptr->list);
+}
+
+void media_device_unregister_entity_notify(struct media_device *mdev,
+					struct media_entity_notify *nptr)
+{
+	mutex_lock(&mdev->graph_mutex);
+	__media_device_unregister_entity_notify(mdev, nptr);
+	mutex_unlock(&mdev->graph_mutex);
+}
+EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
+
 /**
  * media_device_init() - initialize a media device
  * @mdev:	The media device
@@ -743,34 +771,6 @@ int __must_check __media_device_register(struct media_device *mdev,
 }
 EXPORT_SYMBOL_GPL(__media_device_register);
 
-int __must_check media_device_register_entity_notify(struct media_device *mdev,
-					struct media_entity_notify *nptr)
-{
-	mutex_lock(&mdev->graph_mutex);
-	list_add_tail(&nptr->list, &mdev->entity_notify);
-	mutex_unlock(&mdev->graph_mutex);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(media_device_register_entity_notify);
-
-/*
- * Note: Should be called with mdev->lock held.
- */
-static void __media_device_unregister_entity_notify(struct media_device *mdev,
-					struct media_entity_notify *nptr)
-{
-	list_del(&nptr->list);
-}
-
-void media_device_unregister_entity_notify(struct media_device *mdev,
-					struct media_entity_notify *nptr)
-{
-	mutex_lock(&mdev->graph_mutex);
-	__media_device_unregister_entity_notify(mdev, nptr);
-	mutex_unlock(&mdev->graph_mutex);
-}
-EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
-
 void media_device_unregister(struct media_device *mdev)
 {
 	struct media_entity *entity;
-- 
2.1.4

[RFC v4 11/21] media device: Refcount the media device

[Sakari Ailus]

As the struct media_device embeds struct media_devnode, the lifetime of
that object must be that same than that of the media_device.

References are obtained by media_entity_get() and released by
media_entity_put(). In case a driver uses media_device_alloc() to allocate
its media device, it must release the media device by calling
media_device_put() rather than media_device_cleanup().

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 13 +++++++++++++
 include/media/media-device.h | 31 +++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 0920c02..e9f6e76 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -712,6 +712,17 @@ void media_device_init(struct media_device *mdev)
 }
 EXPORT_SYMBOL_GPL(media_device_init);
 
+static void media_device_release(struct media_devnode *devnode)
+{
+	struct media_device *mdev = to_media_device(devnode);
+
+	dev_dbg(devnode->parent, "Media device released\n");
+
+	media_device_cleanup(mdev);
+
+	kfree(mdev);
+}
+
 struct media_device *media_device_alloc(struct device *dev)
 {
 	struct media_device *mdev;
@@ -723,6 +734,8 @@ struct media_device *media_device_alloc(struct device *dev)
 	mdev->dev = dev;
 	media_device_init(mdev);
 
+	mdev->devnode.release = media_device_release;
+
 	return mdev;
 }
 EXPORT_SYMBOL_GPL(media_device_alloc);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index c9b5798..fc0d82a 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -212,10 +212,39 @@ void media_device_init(struct media_device *mdev);
  * @dev:	The associated struct device pointer
  *
  * Allocate and initialise a media device. Returns a media device.
+ * The media device is refcounted, and this function returns a media
+ * device the refcount of which is one (1).
+ *
+ * References are taken and given using media_device_get() and
+ * media_device_put().
  */
 struct media_device *media_device_alloc(struct device *dev);
 
 /**
+ * media_device_get() - Get a reference to a media device
+ *
+ * mdev: media device
+ */
+#define media_device_get(mdev)						\
+	do {								\
+		dev_dbg((mdev)->dev, "%s: get media device %s\n",	\
+			__func__, (mdev)->bus_info);			\
+		get_device(&(mdev)->devnode.dev);			\
+	} while (0)
+
+/**
+ * media_device_put() - Put a reference to a media device
+ *
+ * mdev: media device
+ */
+#define media_device_put(mdev)						\
+	do {								\
+		dev_dbg((mdev)->dev, "%s: put media device %s\n",	\
+			__func__, (mdev)->bus_info);			\
+		put_device(&(mdev)->devnode.dev);			\
+	} while (0)
+
+/**
  * media_device_cleanup() - Cleanups a media device element
  *
  * @mdev:	pointer to struct &media_device
@@ -464,6 +493,8 @@ static inline struct media_device *media_device_alloc(struct device *dev)
 {
 	return NULL;
 }
+#define media_device_get(mdev) do { } while (0)
+#define media_device_put(mdev) do { } while (0)
 static inline int media_device_register(struct media_device *mdev)
 {
 	return 0;
-- 
2.1.4

[RFC v4 12/21] media device: Initialise media devnode in media_device_init()

[Sakari Ailus]

Call media_devnode_init() from media_device_init(). This has the effect of
creating a struct device for the media_devnode before it is registered,
making it possible to obtain a reference to it for e.g. video devices.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index e9f6e76..2e52e44 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -708,6 +708,8 @@ void media_device_init(struct media_device *mdev)
 	mutex_init(&mdev->graph_mutex);
 	ida_init(&mdev->entity_internal_idx);
 
+	media_devnode_init(&mdev->devnode);
+
 	dev_dbg(mdev->dev, "Media device initialized\n");
 }
 EXPORT_SYMBOL_GPL(media_device_init);
@@ -718,7 +720,10 @@ static void media_device_release(struct media_devnode *devnode)
 
 	dev_dbg(devnode->parent, "Media device released\n");
 
-	media_device_cleanup(mdev);
+	ida_destroy(&mdev->entity_internal_idx);
+	mdev->entity_internal_idx_max = 0;
+	media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
+	mutex_destroy(&mdev->graph_mutex);
 
 	kfree(mdev);
 }
@@ -746,6 +751,7 @@ void media_device_cleanup(struct media_device *mdev)
 	mdev->entity_internal_idx_max = 0;
 	media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
 	mutex_destroy(&mdev->graph_mutex);
+	media_device_put(mdev);
 }
 EXPORT_SYMBOL_GPL(media_device_cleanup);
 
@@ -761,26 +767,19 @@ int __must_check __media_device_register(struct media_device *mdev,
 	/* Set version 0 to indicate user-space that the graph is static */
 	mdev->topology_version = 0;
 
-	media_devnode_init(&mdev->devnode);
-
 	ret = media_devnode_register(&mdev->devnode, owner);
 	if (ret < 0)
-		goto out_put;
+		return ret;
 
 	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
-	if (ret < 0)
-		goto out_unregister;
+	if (ret < 0) {
+		media_devnode_unregister(&mdev->devnode);
+		return ret;
+	}
 
 	dev_dbg(mdev->dev, "Media device registered\n");
 
 	return 0;
-
-out_unregister:
-	media_devnode_unregister(&mdev->devnode);
-out_put:
-	put_device(&mdev->devnode.dev);
-
-	return ret;
 }
 EXPORT_SYMBOL_GPL(__media_device_register);
 
@@ -823,7 +822,6 @@ void media_device_unregister(struct media_device *mdev)
 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
 	dev_dbg(mdev->dev, "Media device unregistering\n");
 	media_devnode_unregister(&mdev->devnode);
-	put_device(&mdev->devnode.dev);
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
-- 
2.1.4

[RFC v4 13/21] media device: Deprecate media_device_{init,cleanup}() for drivers

[Sakari Ailus]

Drivers should no longer directly allocate media_device but rely on
media_device_alloc(), media_device_get() and media_device_put() instead.
Deprecate media_device_init() and media_device_cleanup().

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/media/media-device.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/media/media-device.h b/include/media/media-device.h
index fc0d82a..ae2bc08 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -203,6 +203,10 @@ static inline __must_check int media_entity_enum_init(
  * So drivers need to first initialize the media device, register any entity
  * within the media device, create pad to pad links and then finally register
  * the media device by calling media_device_register() as a final step.
+ *
+ * Note that using this function in drivers is DEPRECATED. New drivers
+ * must use media_device_alloc() and manage references using
+ * media_device_get() and media_device_put() instead.
  */
 void media_device_init(struct media_device *mdev);
 
@@ -251,6 +255,10 @@ struct media_device *media_device_alloc(struct device *dev);
  *
  * This function that will destroy the graph_mutex that is
  * initialized in media_device_init().
+ *
+ * Note that using this function in drivers is DEPRECATED. New drivers
+ * must use media_device_alloc() and manage references using
+ * media_device_get() and media_device_put() instead.
  */
 void media_device_cleanup(struct media_device *mdev);
 
-- 
2.1.4

[RFC v4 14/21] media device: Get the media device driver's device

[Sakari Ailus]

The struct device of the media device driver (i.e. not that of the media
devnode) is pointed to by the media device. The struct device pointer is
mostly used for debug prints.

Ensure it will stay around as long as the media device does.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 2e52e44..648c64c 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -724,6 +724,7 @@ static void media_device_release(struct media_devnode *devnode)
 	mdev->entity_internal_idx_max = 0;
 	media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
 	mutex_destroy(&mdev->graph_mutex);
+	put_device(mdev->dev);
 
 	kfree(mdev);
 }
@@ -732,9 +733,15 @@ struct media_device *media_device_alloc(struct device *dev)
 {
 	struct media_device *mdev;
 
+	dev = get_device(dev);
+	if (!dev)
+		return NULL;
+
 	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
-	if (!mdev)
+	if (!mdev) {
+		put_device(dev);
 		return NULL;
+	}
 
 	mdev->dev = dev;
 	media_device_init(mdev);
-- 
2.1.4

[RFC v4 15/21] media: Provide a way to the driver to set a private pointer

[Sakari Ailus]

Now that the media device can be allocated dynamically, drivers have no
longer a way to conveniently obtain the driver private data structure.
Provide one again in the form of a private pointer passed to the
media_device_alloc() function.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-device.c |  3 ++-
 include/media/media-device.h | 15 ++++++++++++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 648c64c..7d9f76d 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -729,7 +729,7 @@ static void media_device_release(struct media_devnode *devnode)
 	kfree(mdev);
 }
 
-struct media_device *media_device_alloc(struct device *dev)
+struct media_device *media_device_alloc(struct device *dev, void *priv)
 {
 	struct media_device *mdev;
 
@@ -745,6 +745,7 @@ struct media_device *media_device_alloc(struct device *dev)
 
 	mdev->dev = dev;
 	media_device_init(mdev);
+	mdev->priv = priv;
 
 	mdev->devnode.release = media_device_release;
 
diff --git a/include/media/media-device.h b/include/media/media-device.h
index ae2bc08..94e96ef 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -62,6 +62,7 @@ struct media_device_ops {
  * struct media_device - Media device
  * @dev:	Parent device
  * @devnode:	Media device node
+ * @priv:	A pointer to driver private data
  * @driver_name: Optional device driver name. If not set, calls to
  *		%MEDIA_IOC_DEVICE_INFO will return ``dev->driver->name``.
  *		This is needed for USB drivers for example, as otherwise
@@ -128,6 +129,7 @@ struct media_device {
 	/* dev->driver_data points to this struct. */
 	struct device *dev;
 	struct media_devnode devnode;
+	void *priv;
 
 	char model[32];
 	char driver_name[32];
@@ -214,6 +216,7 @@ void media_device_init(struct media_device *mdev);
  * media_device_alloc() - Allocate and initialise a media device
  *
  * @dev:	The associated struct device pointer
+ * @priv:	pointer to a driver private data structure
  *
  * Allocate and initialise a media device. Returns a media device.
  * The media device is refcounted, and this function returns a media
@@ -222,7 +225,7 @@ void media_device_init(struct media_device *mdev);
  * References are taken and given using media_device_get() and
  * media_device_put().
  */
-struct media_device *media_device_alloc(struct device *dev);
+struct media_device *media_device_alloc(struct device *dev, void *priv);
 
 /**
  * media_device_get() - Get a reference to a media device
@@ -249,6 +252,16 @@ struct media_device *media_device_alloc(struct device *dev);
 	} while (0)
 
 /**
+ * media_device_priv() - Obtain the driver private pointer
+ *
+ * Returns a pointer passed to the media_device_alloc() function.
+ */
+static inline void *media_device_priv(struct media_device *mdev)
+{
+	return mdev->priv;
+}
+
+/**
  * media_device_cleanup() - Cleanups a media device element
  *
  * @mdev:	pointer to struct &media_device
-- 
2.1.4

[RFC v4 16/21] media: Add release callback for media device

[Sakari Ailus]

The release callback may be used by the driver to signal the release of
the media device. This way the lifetime of the driver's own memory
allocations may be made dependent on that of the media device.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 4 ++++
 include/media/media-device.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 7d9f76d..e9dfc87 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -724,6 +724,10 @@ static void media_device_release(struct media_devnode *devnode)
 	mdev->entity_internal_idx_max = 0;
 	media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
 	mutex_destroy(&mdev->graph_mutex);
+
+	if (mdev->ops && mdev->ops->release)
+		mdev->ops->release(mdev);
+
 	put_device(mdev->dev);
 
 	kfree(mdev);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 94e96ef..81f09ed 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -56,6 +56,7 @@ struct media_entity_notify {
 struct media_device_ops {
 	int (*link_notify)(struct media_link *link, u32 flags,
 			   unsigned int notification);
+	void (*release)(struct media_device *mdev);
 };
 
 /**
-- 
2.1.4

[RFC v4 17/21] v4l: Acquire a reference to the media device for every video device

[Sakari Ailus]

The video device depends on the existence of its media device --- if there
is one. Acquire a reference to it.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-dev.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 8be561a..530d53e 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -171,6 +171,9 @@ static void v4l2_device_release(struct device *cd)
 {
 	struct video_device *vdev = to_video_device(cd);
 	struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
+#ifdef CONFIG_MEDIA_CONTROLLER
+	struct media_device *mdev = v4l2_dev->mdev;
+#endif
 
 	mutex_lock(&videodev_lock);
 	if (WARN_ON(video_device[vdev->minor] != vdev)) {
@@ -193,8 +196,8 @@ static void v4l2_device_release(struct device *cd)
 
 	mutex_unlock(&videodev_lock);
 
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	if (v4l2_dev->mdev) {
+#ifdef CONFIG_MEDIA_CONTROLLER
+	if (mdev) {
 		/* Remove interfaces and interface links */
 		media_devnode_remove(vdev->intf_devnode);
 		if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
@@ -220,6 +223,11 @@ static void v4l2_device_release(struct device *cd)
 	/* Decrease v4l2_device refcount */
 	if (v4l2_dev)
 		v4l2_device_put(v4l2_dev);
+
+#ifdef CONFIG_MEDIA_CONTROLLER
+	if (mdev)
+		media_device_put(mdev);
+#endif
 }
 
 static struct class video_class = {
@@ -813,6 +821,7 @@ static int video_register_media_controller(struct video_device *vdev, int type)
 
 	/* FIXME: how to create the other interface links? */
 
+	media_device_get(vdev->v4l2_dev->mdev);
 #endif
 	return 0;
 }
-- 
2.1.4

[RFC v4 18/21] media-device: Postpone graph object removal until free

[Sakari Ailus]

The media device itself will be unregistered based on it being unbound and
driver's remove callback being called. The graph objects themselves may
still be in use; rely on the media device release callback to release
them.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-device.c | 44 ++++++++++++++++++++------------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index e9dfc87..7a5884e 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -717,6 +717,26 @@ EXPORT_SYMBOL_GPL(media_device_init);
 static void media_device_release(struct media_devnode *devnode)
 {
 	struct media_device *mdev = to_media_device(devnode);
+	struct media_entity *entity;
+	struct media_entity *next;
+	struct media_interface *intf, *tmp_intf;
+	struct media_entity_notify *notify, *nextp;
+
+	/* Remove all entities from the media device */
+	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
+		__media_device_unregister_entity(entity);
+
+	/* Remove all entity_notify callbacks from the media device */
+	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list)
+		__media_device_unregister_entity_notify(mdev, notify);
+
+	/* Remove all interfaces from the media device */
+	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
+				 graph_obj.list) {
+		__media_remove_intf_links(intf);
+		media_gobj_destroy(&intf->graph_obj);
+		kfree(intf);
+	}
 
 	dev_dbg(devnode->parent, "Media device released\n");
 
@@ -797,38 +817,14 @@ EXPORT_SYMBOL_GPL(__media_device_register);
 
 void media_device_unregister(struct media_device *mdev)
 {
-	struct media_entity *entity;
-	struct media_entity *next;
-	struct media_interface *intf, *tmp_intf;
-	struct media_entity_notify *notify, *nextp;
-
 	if (mdev == NULL)
 		return;
 
 	mutex_lock(&mdev->graph_mutex);
-
-	/* Check if mdev was ever registered at all */
 	if (!media_devnode_is_registered(&mdev->devnode)) {
 		mutex_unlock(&mdev->graph_mutex);
 		return;
 	}
-
-	/* Remove all entities from the media device */
-	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
-		__media_device_unregister_entity(entity);
-
-	/* Remove all entity_notify callbacks from the media device */
-	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list)
-		__media_device_unregister_entity_notify(mdev, notify);
-
-	/* Remove all interfaces from the media device */
-	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
-				 graph_obj.list) {
-		__media_remove_intf_links(intf);
-		media_gobj_destroy(&intf->graph_obj);
-		kfree(intf);
-	}
-
 	mutex_unlock(&mdev->graph_mutex);
 
 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
-- 
2.1.4

[RFC v4 19/21] omap3isp: Allocate the media device dynamically

[Sakari Ailus]

Use the new media_device_alloc() API to allocate and release the media
device.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c      | 24 +++++++++++++-----------
 drivers/media/platform/omap3isp/isp.h      |  2 +-
 drivers/media/platform/omap3isp/ispvideo.c |  2 +-
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 2e1b17e..8bc7a7c 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1601,8 +1601,8 @@ static void isp_unregister_entities(struct isp_device *isp)
 	omap3isp_stat_unregister_entities(&isp->isp_hist);
 
 	v4l2_device_unregister(&isp->v4l2_dev);
-	media_device_unregister(&isp->media_dev);
-	media_device_cleanup(&isp->media_dev);
+	media_device_unregister(isp->media_dev);
+	media_device_put(isp->media_dev);
 }
 
 static int isp_link_entity(
@@ -1680,14 +1680,16 @@ static int isp_register_entities(struct isp_device *isp)
 {
 	int ret;
 
-	isp->media_dev.dev = isp->dev;
-	strlcpy(isp->media_dev.model, "TI OMAP3 ISP",
-		sizeof(isp->media_dev.model));
-	isp->media_dev.hw_revision = isp->revision;
-	isp->media_dev.ops = &isp_media_ops;
-	media_device_init(&isp->media_dev);
+	isp->media_dev = media_device_alloc(isp->dev, isp);
+	if (!isp->media_dev)
+		return -ENOMEM;
+
+	strlcpy(isp->media_dev->model, "TI OMAP3 ISP",
+		sizeof(isp->media_dev->model));
+	isp->media_dev->hw_revision = isp->revision;
+	isp->media_dev->ops = &isp_media_ops;
 
-	isp->v4l2_dev.mdev = &isp->media_dev;
+	isp->v4l2_dev.mdev = isp->media_dev;
 	ret = v4l2_device_register(isp->dev, &isp->v4l2_dev);
 	if (ret < 0) {
 		dev_err(isp->dev, "%s: V4L2 device registration failed (%d)\n",
@@ -2165,7 +2167,7 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 	struct isp_bus_cfg *bus;
 	int ret;
 
-	ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
+	ret = media_entity_enum_init(&isp->crashed, isp->media_dev);
 	if (ret)
 		return ret;
 
@@ -2183,7 +2185,7 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 	if (ret < 0)
 		return ret;
 
-	return media_device_register(&isp->media_dev);
+	return media_device_register(isp->media_dev);
 }
 
 /*
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index 7e6f663..7378279 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -176,7 +176,7 @@ struct isp_xclk {
 struct isp_device {
 	struct v4l2_device v4l2_dev;
 	struct v4l2_async_notifier notifier;
-	struct media_device media_dev;
+	struct media_device *media_dev;
 	struct device *dev;
 	u32 revision;
 
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 7354469..33e74b9 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1104,7 +1104,7 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	pipe = video->video.entity.pipe
 	     ? to_isp_pipeline(&video->video.entity) : &video->pipe;
 
-	ret = media_entity_enum_init(&pipe->ent_enum, &video->isp->media_dev);
+	ret = media_entity_enum_init(&pipe->ent_enum, video->isp->media_dev);
 	if (ret)
 		goto err_enum_init;
 
-- 
2.1.4

[RFC v4 20/21] omap3isp: Release the isp device struct by media device callback

[Sakari Ailus]

Use the media device release callback to release the isp device's data
structure. This approach has the benefit of not releasing memory which may
still be accessed through open file handles whilst the isp driver is being
unbound.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 8bc7a7c..54b84a0 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -657,8 +657,11 @@ static irqreturn_t isp_isr(int irq, void *_isp)
 	return IRQ_HANDLED;
 }
 
+static void isp_release(struct media_device *mdev);
+
 static const struct media_device_ops isp_media_ops = {
 	.link_notify = v4l2_pipeline_link_notify,
+	.release = isp_release,
 };
 
 /* -----------------------------------------------------------------------------
@@ -1948,6 +1951,20 @@ static void isp_detach_iommu(struct isp_device *isp)
 	iommu_group_remove_device(isp->dev);
 }
 
+static void isp_release(struct media_device *mdev)
+{
+	struct isp_device *isp = media_device_priv(mdev);
+
+	isp_cleanup_modules(isp);
+	isp_xclk_cleanup(isp);
+
+	__omap3isp_get(isp, false);
+	isp_detach_iommu(isp);
+	__omap3isp_put(isp, false);
+
+	media_entity_enum_cleanup(&isp->crashed);
+}
+
 static int isp_attach_iommu(struct isp_device *isp)
 {
 	struct dma_iommu_mapping *mapping;
@@ -2008,14 +2025,6 @@ static int isp_remove(struct platform_device *pdev)
 
 	v4l2_async_notifier_unregister(&isp->notifier);
 	isp_unregister_entities(isp);
-	isp_cleanup_modules(isp);
-	isp_xclk_cleanup(isp);
-
-	__omap3isp_get(isp, false);
-	isp_detach_iommu(isp);
-	__omap3isp_put(isp, false);
-
-	media_entity_enum_cleanup(&isp->crashed);
 
 	return 0;
 }
-- 
2.1.4

[RFC v4 21/21] omap3isp: Don't rely on devm for memory resource management

[Sakari Ailus]

devm functions are fine for managing resources that are directly related
to the device at hand and that have no other dependencies. However, a
process holding a file handle to a device created by a driver for a device
may result in the file handle left behind after the device is long gone.
This will result in accessing released (and potentially reallocated)
memory.

Instead, rely on the media device which will stick around until all users
are gone.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c         | 38 ++++++++++++++++++++-------
 drivers/media/platform/omap3isp/ispccp2.c     |  3 ++-
 drivers/media/platform/omap3isp/isph3a_aewb.c | 20 +++++++++-----
 drivers/media/platform/omap3isp/isph3a_af.c   | 19 ++++++++++----
 drivers/media/platform/omap3isp/isphist.c     |  5 ++--
 drivers/media/platform/omap3isp/ispstat.c     |  2 ++
 6 files changed, 63 insertions(+), 24 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 54b84a0..5777c55 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1377,7 +1377,7 @@ static int isp_get_clocks(struct isp_device *isp)
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(isp_clocks); ++i) {
-		clk = devm_clk_get(isp->dev, isp_clocks[i]);
+		clk = clk_get(isp->dev, isp_clocks[i]);
 		if (IS_ERR(clk)) {
 			dev_err(isp->dev, "clk_get %s failed\n", isp_clocks[i]);
 			return PTR_ERR(clk);
@@ -1389,6 +1389,14 @@ static int isp_get_clocks(struct isp_device *isp)
 	return 0;
 }
 
+static void isp_put_clocks(struct isp_device *isp)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(isp_clocks); ++i)
+		clk_put(isp->clock[i]);
+}
+
 /*
  * omap3isp_get - Acquire the ISP resource.
  *
@@ -1603,7 +1611,6 @@ static void isp_unregister_entities(struct isp_device *isp)
 	omap3isp_stat_unregister_entities(&isp->isp_af);
 	omap3isp_stat_unregister_entities(&isp->isp_hist);
 
-	v4l2_device_unregister(&isp->v4l2_dev);
 	media_device_unregister(isp->media_dev);
 	media_device_put(isp->media_dev);
 }
@@ -1955,6 +1962,8 @@ static void isp_release(struct media_device *mdev)
 {
 	struct isp_device *isp = media_device_priv(mdev);
 
+	v4l2_device_unregister(&isp->v4l2_dev);
+
 	isp_cleanup_modules(isp);
 	isp_xclk_cleanup(isp);
 
@@ -1963,6 +1972,10 @@ static void isp_release(struct media_device *mdev)
 	__omap3isp_put(isp, false);
 
 	media_entity_enum_cleanup(&isp->crashed);
+
+	isp_put_clocks(isp);
+
+	kfree(isp);
 }
 
 static int isp_attach_iommu(struct isp_device *isp)
@@ -2215,7 +2228,7 @@ static int isp_probe(struct platform_device *pdev)
 	int ret;
 	int i, m;
 
-	isp = devm_kzalloc(&pdev->dev, sizeof(*isp), GFP_KERNEL);
+	isp = kzalloc(sizeof(*isp), GFP_KERNEL);
 	if (!isp) {
 		dev_err(&pdev->dev, "could not allocate memory\n");
 		return -ENOMEM;
@@ -2224,21 +2237,23 @@ static int isp_probe(struct platform_device *pdev)
 	ret = of_property_read_u32(pdev->dev.of_node, "ti,phy-type",
 				   &isp->phy_type);
 	if (ret)
-		return ret;
+		goto error_release_isp;
 
 	isp->syscon = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
 						      "syscon");
-	if (IS_ERR(isp->syscon))
-		return PTR_ERR(isp->syscon);
+	if (IS_ERR(isp->syscon)) {
+		ret = PTR_ERR(isp->syscon);
+		goto error_release_isp;
+	}
 
 	ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1,
 					 &isp->syscon_offset);
 	if (ret)
-		return ret;
+		goto error_release_isp;
 
 	ret = isp_of_parse_nodes(&pdev->dev, &isp->notifier);
 	if (ret < 0)
-		return ret;
+		goto error_release_isp;
 
 	isp->autoidle = autoidle;
 
@@ -2255,8 +2270,8 @@ static int isp_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, isp);
 
 	/* Regulators */
-	isp->isp_csiphy1.vdd = devm_regulator_get(&pdev->dev, "vdd-csiphy1");
-	isp->isp_csiphy2.vdd = devm_regulator_get(&pdev->dev, "vdd-csiphy2");
+	isp->isp_csiphy1.vdd = regulator_get(&pdev->dev, "vdd-csiphy1");
+	isp->isp_csiphy2.vdd = regulator_get(&pdev->dev, "vdd-csiphy2");
 
 	/* Clocks
 	 *
@@ -2388,6 +2403,9 @@ static int isp_probe(struct platform_device *pdev)
 	__omap3isp_put(isp, false);
 error:
 	mutex_destroy(&isp->isp_mutex);
+	isp_put_clocks(isp);
+error_release_isp:
+	kfree(isp);
 
 	return ret;
 }
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index ca09523..d49ce8a 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -1135,7 +1135,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 	 * TODO: Don't hardcode the usage of PHY1 (shared with CSI2c).
 	 */
 	if (isp->revision == ISP_REVISION_2_0) {
-		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
+		ccp2->vdds_csib = regulator_get(isp->dev, "vdds_csib");
 		if (IS_ERR(ccp2->vdds_csib)) {
 			dev_dbg(isp->dev,
 				"Could not get regulator vdds_csib\n");
@@ -1163,4 +1163,5 @@ void omap3isp_ccp2_cleanup(struct isp_device *isp)
 
 	omap3isp_video_cleanup(&ccp2->video_in);
 	media_entity_cleanup(&ccp2->subdev.entity);
+	regulator_put(ccp2->vdds_csib);
 }
diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c b/drivers/media/platform/omap3isp/isph3a_aewb.c
index d44626f2..38eaa92 100644
--- a/drivers/media/platform/omap3isp/isph3a_aewb.c
+++ b/drivers/media/platform/omap3isp/isph3a_aewb.c
@@ -289,9 +289,10 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
 {
 	struct ispstat *aewb = &isp->isp_aewb;
 	struct omap3isp_h3a_aewb_config *aewb_cfg;
-	struct omap3isp_h3a_aewb_config *aewb_recover_cfg;
+	struct omap3isp_h3a_aewb_config *aewb_recover_cfg = NULL;
+	int ret;
 
-	aewb_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_cfg), GFP_KERNEL);
+	aewb_cfg = kzalloc(sizeof(*aewb_cfg), GFP_KERNEL);
 	if (!aewb_cfg)
 		return -ENOMEM;
 
@@ -301,12 +302,12 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
 	aewb->isp = isp;
 
 	/* Set recover state configuration */
-	aewb_recover_cfg = devm_kzalloc(isp->dev, sizeof(*aewb_recover_cfg),
-					GFP_KERNEL);
+	aewb_recover_cfg = kzalloc(sizeof(*aewb_recover_cfg), GFP_KERNEL);
 	if (!aewb_recover_cfg) {
 		dev_err(aewb->isp->dev,
 			"AEWB: cannot allocate memory for recover configuration.\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err;
 	}
 
 	aewb_recover_cfg->saturation_limit = OMAP3ISP_AEWB_MAX_SATURATION_LIM;
@@ -323,13 +324,20 @@ int omap3isp_h3a_aewb_init(struct isp_device *isp)
 	if (h3a_aewb_validate_params(aewb, aewb_recover_cfg)) {
 		dev_err(aewb->isp->dev,
 			"AEWB: recover configuration is invalid.\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	}
 
 	aewb_recover_cfg->buf_size = h3a_aewb_get_buf_size(aewb_recover_cfg);
 	aewb->recover_priv = aewb_recover_cfg;
 
 	return omap3isp_stat_init(aewb, "AEWB", &h3a_aewb_subdev_ops);
+
+err:
+	kfree(aewb_cfg);
+	kfree(aewb_recover_cfg);
+
+	return ret;
 }
 
 /*
diff --git a/drivers/media/platform/omap3isp/isph3a_af.c b/drivers/media/platform/omap3isp/isph3a_af.c
index 99bd6cc..39e1a0c 100644
--- a/drivers/media/platform/omap3isp/isph3a_af.c
+++ b/drivers/media/platform/omap3isp/isph3a_af.c
@@ -352,9 +352,10 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
 {
 	struct ispstat *af = &isp->isp_af;
 	struct omap3isp_h3a_af_config *af_cfg;
-	struct omap3isp_h3a_af_config *af_recover_cfg;
+	struct omap3isp_h3a_af_config *af_recover_cfg = NULL;
+	int ret;
 
-	af_cfg = devm_kzalloc(isp->dev, sizeof(*af_cfg), GFP_KERNEL);
+	af_cfg = kzalloc(sizeof(*af_cfg), GFP_KERNEL);
 	if (af_cfg == NULL)
 		return -ENOMEM;
 
@@ -364,12 +365,13 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
 	af->isp = isp;
 
 	/* Set recover state configuration */
-	af_recover_cfg = devm_kzalloc(isp->dev, sizeof(*af_recover_cfg),
-				      GFP_KERNEL);
+	af_recover_cfg = kzalloc(sizeof(*af_recover_cfg), GFP_KERNEL);
 	if (!af_recover_cfg) {
 		dev_err(af->isp->dev,
 			"AF: cannot allocate memory for recover configuration.\n");
 		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err;
 	}
 
 	af_recover_cfg->paxel.h_start = OMAP3ISP_AF_PAXEL_HZSTART_MIN;
@@ -381,13 +383,20 @@ int omap3isp_h3a_af_init(struct isp_device *isp)
 	if (h3a_af_validate_params(af, af_recover_cfg)) {
 		dev_err(af->isp->dev,
 			"AF: recover configuration is invalid.\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	}
 
 	af_recover_cfg->buf_size = h3a_af_get_buf_size(af_recover_cfg);
 	af->recover_priv = af_recover_cfg;
 
 	return omap3isp_stat_init(af, "AF", &h3a_af_subdev_ops);
+
+err:
+	kfree(af_cfg);
+	kfree(af_recover_cfg);
+
+	return ret;
 }
 
 void omap3isp_h3a_af_cleanup(struct isp_device *isp)
diff --git a/drivers/media/platform/omap3isp/isphist.c b/drivers/media/platform/omap3isp/isphist.c
index 7138b04..976cab0 100644
--- a/drivers/media/platform/omap3isp/isphist.c
+++ b/drivers/media/platform/omap3isp/isphist.c
@@ -477,9 +477,9 @@ int omap3isp_hist_init(struct isp_device *isp)
 {
 	struct ispstat *hist = &isp->isp_hist;
 	struct omap3isp_hist_config *hist_cfg;
-	int ret = -1;
+	int ret;
 
-	hist_cfg = devm_kzalloc(isp->dev, sizeof(*hist_cfg), GFP_KERNEL);
+	hist_cfg = kzalloc(sizeof(*hist_cfg), GFP_KERNEL);
 	if (hist_cfg == NULL)
 		return -ENOMEM;
 
@@ -517,6 +517,7 @@ int omap3isp_hist_init(struct isp_device *isp)
 	if (ret) {
 		if (hist->dma_ch)
 			dma_release_channel(hist->dma_ch);
+		kfree(hist_cfg);
 	}
 
 	return ret;
diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c
index 47cbc7e..aad0bc3 100644
--- a/drivers/media/platform/omap3isp/ispstat.c
+++ b/drivers/media/platform/omap3isp/ispstat.c
@@ -1067,4 +1067,6 @@ void omap3isp_stat_cleanup(struct ispstat *stat)
 	mutex_destroy(&stat->ioctl_lock);
 	isp_stat_bufs_free(stat);
 	kfree(stat->buf);
+	kfree(stat->priv);
+	kfree(stat->recover_priv);
 }
-- 
2.1.4

Re: [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race"

[Mauro Carvalho Chehab]

Em Tue,  8 Nov 2016 15:55:10 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> This reverts commit 6f0dd24a084a ("[media] media: fix media devnode
> ioctl/syscall and unregister race"). The commit was part of an original
> patchset to avoid crashes when an unregistering device is in use.

As I said before: I will only look on such patch series if you don't
randomly revert patches that aren't broken.

Also, if this series require changes on drivers, it is up to you
to do such changes in a way that won't break patch bisectability,
so, each change should be self-contained and touch all drivers that
are affected by the kAPI change.

Thanks,
Mauro

Re: [RFC v4 08/21] media: Enable allocating the media device dynamically

[Shuah Khan]

On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> From: Sakari Ailus <sakari.ailus@iki.fi>
>
> Allow allocating the media device dynamically. As the struct media_device
> embeds struct media_devnode, the lifetime of that object is that same than
> that of the media_device.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/media-device.c | 15 +++++++++++++++
>  include/media/media-device.h | 13 +++++++++++++
>  2 files changed, 28 insertions(+)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index a31329d..496195e 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
>  }
>  EXPORT_SYMBOL_GPL(media_device_init);
>
> +struct media_device *media_device_alloc(struct device *dev)
> +{
> +       struct media_device *mdev;
> +
> +       mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> +       if (!mdev)
> +               return NULL;
> +
> +       mdev->dev = dev;
> +       media_device_init(mdev);
> +
> +       return mdev;
> +}
> +EXPORT_SYMBOL_GPL(media_device_alloc);
> +

One problem with this allocation is, this media device can't be shared across
drivers. For au0828 and snd-usb-audio should be able to share the
media_device. That is what the Media Allocator API patch series does.
This a quick review and I will review the patch series and get back to
you.

thanks,
-- Shuah

>  void media_device_cleanup(struct media_device *mdev)
>  {
>         ida_destroy(&mdev->entity_internal_idx);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index 96de915..c9b5798 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -207,6 +207,15 @@ static inline __must_check int media_entity_enum_init(
>  void media_device_init(struct media_device *mdev);
>
>  /**
> + * media_device_alloc() - Allocate and initialise a media device
> + *
> + * @dev:       The associated struct device pointer
> + *
> + * Allocate and initialise a media device. Returns a media device.
> + */
> +struct media_device *media_device_alloc(struct device *dev);
> +
> +/**
>   * media_device_cleanup() - Cleanups a media device element
>   *
>   * @mdev:      pointer to struct &media_device
> @@ -451,6 +460,10 @@ void __media_device_usb_init(struct media_device *mdev,
>                              const char *driver_name);
>
>  #else
> +static inline struct media_device *media_device_alloc(struct device *dev)
> +{
> +       return NULL;
> +}
>  static inline int media_device_register(struct media_device *mdev)
>  {
>         return 0;
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race"

[Laurent Pinchart]

Hi Mauro,

On Tuesday 08 Nov 2016 15:00:27 Mauro Carvalho Chehab wrote:
> Em Tue,  8 Nov 2016 15:55:10 +0200 Sakari Ailus escreveu:
> > This reverts commit 6f0dd24a084a ("[media] media: fix media devnode
> > ioctl/syscall and unregister race"). The commit was part of an original
> > patchset to avoid crashes when an unregistering device is in use.
> 
> As I said before: I will only look on such patch series if you don't
> randomly revert patches that aren't broken.

That's not always an option. What you're essentially saying is that if taking 
a step forward improves the situation, we can only improve it more by taking 
more steps forward. Sometimes the step forward leads to a dead end, making a U 
turn unavoidable to avoid running into a wall :-)

Sure, we could squash the reverts as part of a large patch to would be too big 
to review, but I hardly see that as an improvement.

This being said, I haven't checked whether it would be possible to shuffle 
patches around and revert the first three on top of the rest of the series, 
but at first glance it seems very difficult. I don't think we should use the 
fact that the three patches were merged despite known issues a strong argument 
to prevent them from being reverted. Fixes are being reverted all the time in 
the kernel when they cause more harm than good, there's nothing special here.

> Also, if this series require changes on drivers, it is up to you
> to do such changes in a way that won't break patch bisectability,
> so, each change should be self-contained and touch all drivers that
> are affected by the kAPI change.

-- 
Regards,

Laurent Pinchart

Re: [RFC v4 08/21] media: Enable allocating the media device dynamically

[Laurent Pinchart]

Hi Shuah,

On Tuesday 08 Nov 2016 12:20:29 Shuah Khan wrote:
> On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus wrote:
> > From: Sakari Ailus <sakari.ailus@iki.fi>
> > 
> > Allow allocating the media device dynamically. As the struct media_device
> > embeds struct media_devnode, the lifetime of that object is that same than
> > that of the media_device.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > 
> >  drivers/media/media-device.c | 15 +++++++++++++++
> >  include/media/media-device.h | 13 +++++++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index a31329d..496195e 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
> >  }
> >  EXPORT_SYMBOL_GPL(media_device_init);
> > 
> > +struct media_device *media_device_alloc(struct device *dev)
> > +{
> > +       struct media_device *mdev;
> > +
> > +       mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> > +       if (!mdev)
> > +               return NULL;
> > +
> > +       mdev->dev = dev;
> > +       media_device_init(mdev);
> > +
> > +       return mdev;
> > +}
> > +EXPORT_SYMBOL_GPL(media_device_alloc);
> > +
> 
> One problem with this allocation is, this media device can't be shared
> across drivers. For au0828 and snd-usb-audio should be able to share the
> media_device. That is what the Media Allocator API patch series does.

No disagreement here, Sakari's patches don't address the issues that the media 
allocator API fixes. The media allocator API, when ready, should replace (or 
at least complement, if we decide to keep a simpler API for drivers that don't 
need to share a media device, but I have no opinion on this at this time) this 
allocation function.

> This a quick review and I will review the patch series and get back to
> you.
>
> >  void media_device_cleanup(struct media_device *mdev)
> >  {
> >         ida_destroy(&mdev->entity_internal_idx);
> > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > index 96de915..c9b5798 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -207,6 +207,15 @@ static inline __must_check int
> > media_entity_enum_init(
> >  void media_device_init(struct media_device *mdev);
> >  
> >  /**
> > + * media_device_alloc() - Allocate and initialise a media device
> > + *
> > + * @dev:       The associated struct device pointer
> > + *
> > + * Allocate and initialise a media device. Returns a media device.
> > + */
> > +struct media_device *media_device_alloc(struct device *dev);
> > +
> > +/**
> >   * media_device_cleanup() - Cleanups a media device element
> >   *
> >   * @mdev:      pointer to struct &media_device
> > @@ -451,6 +460,10 @@ void __media_device_usb_init(struct media_device
> > *mdev,
> >                              const char *driver_name);
> >  #else
> > +static inline struct media_device *media_device_alloc(struct device *dev)
> > +{
> > +       return NULL;
> > +}
> >  static inline int media_device_register(struct media_device *mdev)
> >  {
> >         return 0;

-- 
Regards,

Laurent Pinchart

Re: [RFC v4 08/21] media: Enable allocating the media device dynamically

[Shuah Khan]

On 11/10/2016 04:53 PM, Laurent Pinchart wrote:
> Hi Shuah,
> 
> On Tuesday 08 Nov 2016 12:20:29 Shuah Khan wrote:
>> On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus wrote:
>>> From: Sakari Ailus <sakari.ailus@iki.fi>
>>>
>>> Allow allocating the media device dynamically. As the struct media_device
>>> embeds struct media_devnode, the lifetime of that object is that same than
>>> that of the media_device.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> ---
>>>
>>>  drivers/media/media-device.c | 15 +++++++++++++++
>>>  include/media/media-device.h | 13 +++++++++++++
>>>  2 files changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>> index a31329d..496195e 100644
>>> --- a/drivers/media/media-device.c
>>> +++ b/drivers/media/media-device.c
>>> @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
>>>  }
>>>  EXPORT_SYMBOL_GPL(media_device_init);
>>>
>>> +struct media_device *media_device_alloc(struct device *dev)
>>> +{
>>> +       struct media_device *mdev;
>>> +
>>> +       mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
>>> +       if (!mdev)
>>> +               return NULL;
>>> +
>>> +       mdev->dev = dev;
>>> +       media_device_init(mdev);
>>> +
>>> +       return mdev;
>>> +}
>>> +EXPORT_SYMBOL_GPL(media_device_alloc);
>>> +
>>
>> One problem with this allocation is, this media device can't be shared
>> across drivers. For au0828 and snd-usb-audio should be able to share the
>> media_device. That is what the Media Allocator API patch series does.
> 
> No disagreement here, Sakari's patches don't address the issues that the media 
> allocator API fixes. The media allocator API, when ready, should replace (or 
> at least complement, if we decide to keep a simpler API for drivers that don't 
> need to share a media device, but I have no opinion on this at this time) this 
> allocation function.

Media Device Allocator API is ready and reviewed. au0828 uses it as the first
driver using it. I will be sending out snd-usb-audio patch soon that makes use
of the shared media device.

thanks,
-- Shuah

> 
>> This a quick review and I will review the patch series and get back to
>> you.
>>
>>>  void media_device_cleanup(struct media_device *mdev)
>>>  {
>>>         ida_destroy(&mdev->entity_internal_idx);
>>> diff --git a/include/media/media-device.h b/include/media/media-device.h
>>> index 96de915..c9b5798 100644
>>> --- a/include/media/media-device.h
>>> +++ b/include/media/media-device.h
>>> @@ -207,6 +207,15 @@ static inline __must_check int
>>> media_entity_enum_init(
>>>  void media_device_init(struct media_device *mdev);
>>>  
>>>  /**
>>> + * media_device_alloc() - Allocate and initialise a media device
>>> + *
>>> + * @dev:       The associated struct device pointer
>>> + *
>>> + * Allocate and initialise a media device. Returns a media device.
>>> + */
>>> +struct media_device *media_device_alloc(struct device *dev);
>>> +
>>> +/**
>>>   * media_device_cleanup() - Cleanups a media device element
>>>   *
>>>   * @mdev:      pointer to struct &media_device
>>> @@ -451,6 +460,10 @@ void __media_device_usb_init(struct media_device
>>> *mdev,
>>>                              const char *driver_name);
>>>  #else
>>> +static inline struct media_device *media_device_alloc(struct device *dev)
>>> +{
>>> +       return NULL;
>>> +}
>>>  static inline int media_device_register(struct media_device *mdev)
>>>  {
>>>         return 0;
> 

Re: [RFC v4 08/21] media: Enable allocating the media device dynamically

[Laurent Pinchart]

Hi Shuah,

On Thursday 10 Nov 2016 17:00:16 Shuah Khan wrote:
> On 11/10/2016 04:53 PM, Laurent Pinchart wrote:
> > On Tuesday 08 Nov 2016 12:20:29 Shuah Khan wrote:
> >> On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus wrote:
> >>> From: Sakari Ailus <sakari.ailus@iki.fi>
> >>> 
> >>> Allow allocating the media device dynamically. As the struct
> >>> media_device embeds struct media_devnode, the lifetime of that object is
> >>> that same than that of the media_device.
> >>> 
> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>> ---
> >>> 
> >>>  drivers/media/media-device.c | 15 +++++++++++++++
> >>>  include/media/media-device.h | 13 +++++++++++++
> >>>  2 files changed, 28 insertions(+)
> >>> 
> >>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >>> index a31329d..496195e 100644
> >>> --- a/drivers/media/media-device.c
> >>> +++ b/drivers/media/media-device.c
> >>> @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
> >>>  }
> >>>  EXPORT_SYMBOL_GPL(media_device_init);
> >>> 
> >>> +struct media_device *media_device_alloc(struct device *dev)
> >>> +{
> >>> +       struct media_device *mdev;
> >>> +
> >>> +       mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> >>> +       if (!mdev)
> >>> +               return NULL;
> >>> +
> >>> +       mdev->dev = dev;
> >>> +       media_device_init(mdev);
> >>> +
> >>> +       return mdev;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(media_device_alloc);
> >>> +
> >> 
> >> One problem with this allocation is, this media device can't be shared
> >> across drivers. For au0828 and snd-usb-audio should be able to share the
> >> media_device. That is what the Media Allocator API patch series does.
> > 
> > No disagreement here, Sakari's patches don't address the issues that the
> > media allocator API fixes. The media allocator API, when ready, should
> > replace (or at least complement, if we decide to keep a simpler API for
> > drivers that don't need to share a media device, but I have no opinion on
> > this at this time) this allocation function.
> 
> Media Device Allocator API is ready and reviewed. au0828 uses it as the
> first driver using it. I will be sending out snd-usb-audio patch soon that
> makes use of the shared media device.

I don't think it would be too difficult to rebase this series on top of the 
media allocator API, as all that is needed here is a way to dynamically 
allocate the media device in a clean fashion. I don't think Sakari's patches 
depend on a specific implementation of media_device_alloc(). Sakari, please 
let me know if I got this wrong.

> >> This a quick review and I will review the patch series and get back to
> >> you.

-- 
Regards,

Laurent Pinchart

Re: [RFC v4 08/21] media: Enable allocating the media device dynamically

[Shuah Khan]

On 11/10/2016 05:11 PM, Laurent Pinchart wrote:
> Hi Shuah,
> 
> On Thursday 10 Nov 2016 17:00:16 Shuah Khan wrote:
>> On 11/10/2016 04:53 PM, Laurent Pinchart wrote:
>>> On Tuesday 08 Nov 2016 12:20:29 Shuah Khan wrote:
>>>> On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus wrote:
>>>>> From: Sakari Ailus <sakari.ailus@iki.fi>
>>>>>
>>>>> Allow allocating the media device dynamically. As the struct
>>>>> media_device embeds struct media_devnode, the lifetime of that object is
>>>>> that same than that of the media_device.
>>>>>
>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>> ---
>>>>>
>>>>>  drivers/media/media-device.c | 15 +++++++++++++++
>>>>>  include/media/media-device.h | 13 +++++++++++++
>>>>>  2 files changed, 28 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>>>> index a31329d..496195e 100644
>>>>> --- a/drivers/media/media-device.c
>>>>> +++ b/drivers/media/media-device.c
>>>>> @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
>>>>>  }
>>>>>  EXPORT_SYMBOL_GPL(media_device_init);
>>>>>
>>>>> +struct media_device *media_device_alloc(struct device *dev)
>>>>> +{
>>>>> +       struct media_device *mdev;
>>>>> +
>>>>> +       mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
>>>>> +       if (!mdev)
>>>>> +               return NULL;
>>>>> +
>>>>> +       mdev->dev = dev;
>>>>> +       media_device_init(mdev);
>>>>> +
>>>>> +       return mdev;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(media_device_alloc);
>>>>> +
>>>>
>>>> One problem with this allocation is, this media device can't be shared
>>>> across drivers. For au0828 and snd-usb-audio should be able to share the
>>>> media_device. That is what the Media Allocator API patch series does.
>>>
>>> No disagreement here, Sakari's patches don't address the issues that the
>>> media allocator API fixes. The media allocator API, when ready, should
>>> replace (or at least complement, if we decide to keep a simpler API for
>>> drivers that don't need to share a media device, but I have no opinion on
>>> this at this time) this allocation function.
>>
>> Media Device Allocator API is ready and reviewed. au0828 uses it as the
>> first driver using it. I will be sending out snd-usb-audio patch soon that
>> makes use of the shared media device.
> 
> I don't think it would be too difficult to rebase this series on top of the 
> media allocator API, as all that is needed here is a way to dynamically 
> allocate the media device in a clean fashion. I don't think Sakari's patches 
> depend on a specific implementation of media_device_alloc(). Sakari, please 
> let me know if I got this wrong.

Media Device Allocator API is independent as well. It doesn't make any
assumptions on media_device register and doesn't care really whether it
is shared or not. So I think Sakari's work can use the Media Device Allocator
API instead of the allocator routine it is adding.

thanks,
-- Shuah
> 
>>>> This a quick review and I will review the patch series and get back to
>>>> you.
> 

Re: [RFC v4 08/21] media: Enable allocating the media device dynamically

[Laurent Pinchart]

Hi Shuah,

On Thursday 10 Nov 2016 17:16:31 Shuah Khan wrote:
> On 11/10/2016 05:11 PM, Laurent Pinchart wrote:
> > On Thursday 10 Nov 2016 17:00:16 Shuah Khan wrote:
> >> On 11/10/2016 04:53 PM, Laurent Pinchart wrote:
> >>> On Tuesday 08 Nov 2016 12:20:29 Shuah Khan wrote:
> >>>> On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus wrote:
> >>>>> From: Sakari Ailus <sakari.ailus@iki.fi>
> >>>>> 
> >>>>> Allow allocating the media device dynamically. As the struct
> >>>>> media_device embeds struct media_devnode, the lifetime of that object
> >>>>> is that same than that of the media_device.
> >>>>> 
> >>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>>> ---
> >>>>> 
> >>>>>  drivers/media/media-device.c | 15 +++++++++++++++
> >>>>>  include/media/media-device.h | 13 +++++++++++++
> >>>>>  2 files changed, 28 insertions(+)
> >>>>> 
> >>>>> diff --git a/drivers/media/media-device.c
> >>>>> b/drivers/media/media-device.c
> >>>>> index a31329d..496195e 100644
> >>>>> --- a/drivers/media/media-device.c
> >>>>> +++ b/drivers/media/media-device.c
> >>>>> @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
> >>>>>  }
> >>>>>  EXPORT_SYMBOL_GPL(media_device_init);
> >>>>> 
> >>>>> +struct media_device *media_device_alloc(struct device *dev)
> >>>>> +{
> >>>>> +       struct media_device *mdev;
> >>>>> +
> >>>>> +       mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> >>>>> +       if (!mdev)
> >>>>> +               return NULL;
> >>>>> +
> >>>>> +       mdev->dev = dev;
> >>>>> +       media_device_init(mdev);
> >>>>> +
> >>>>> +       return mdev;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(media_device_alloc);
> >>>>> +
> >>>> 
> >>>> One problem with this allocation is, this media device can't be shared
> >>>> across drivers. For au0828 and snd-usb-audio should be able to share
> >>>> the media_device. That is what the Media Allocator API patch series
> >>>> does.
> >>> 
> >>> No disagreement here, Sakari's patches don't address the issues that the
> >>> media allocator API fixes. The media allocator API, when ready, should
> >>> replace (or at least complement, if we decide to keep a simpler API for
> >>> drivers that don't need to share a media device, but I have no opinion
> >>> on this at this time) this allocation function.
> >> 
> >> Media Device Allocator API is ready and reviewed. au0828 uses it as the
> >> first driver using it. I will be sending out snd-usb-audio patch soon
> >> that makes use of the shared media device.
> > 
> > I don't think it would be too difficult to rebase this series on top of
> > the media allocator API, as all that is needed here is a way to
> > dynamically allocate the media device in a clean fashion. I don't think
> > Sakari's patches depend on a specific implementation of
> > media_device_alloc(). Sakari, please let me know if I got this wrong.
> 
> Media Device Allocator API is independent as well. It doesn't make any
> assumptions on media_device register and doesn't care really whether it
> is shared or not. So I think Sakari's work can use the Media Device
> Allocator API instead of the allocator routine it is adding.

I think you're right, and the goal is certainly to merge both. The question is 
just which series should go in first. I have no strong opinion on that, 
whichever series is ready first can probably get merged first. I'd just like 
to get a confirmation from Sakari that both orders would be equally fine.

This being said, I'd of course like to test the combination of both series to 
make sure neither introduce an issue for the other one.

> >>>> This a quick review and I will review the patch series and get back to
> >>>> you.

-- 
Regards,

Laurent Pinchart

Re: [RFC v4 08/21] media: Enable allocating the media device dynamically

[Shuah Khan]

On 11/10/2016 05:19 PM, Laurent Pinchart wrote:
> Hi Shuah,
> 
> On Thursday 10 Nov 2016 17:16:31 Shuah Khan wrote:
>> On 11/10/2016 05:11 PM, Laurent Pinchart wrote:
>>> On Thursday 10 Nov 2016 17:00:16 Shuah Khan wrote:
>>>> On 11/10/2016 04:53 PM, Laurent Pinchart wrote:
>>>>> On Tuesday 08 Nov 2016 12:20:29 Shuah Khan wrote:
>>>>>> On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus wrote:
>>>>>>> From: Sakari Ailus <sakari.ailus@iki.fi>
>>>>>>>
>>>>>>> Allow allocating the media device dynamically. As the struct
>>>>>>> media_device embeds struct media_devnode, the lifetime of that object
>>>>>>> is that same than that of the media_device.
>>>>>>>
>>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>>>> ---
>>>>>>>
>>>>>>>  drivers/media/media-device.c | 15 +++++++++++++++
>>>>>>>  include/media/media-device.h | 13 +++++++++++++
>>>>>>>  2 files changed, 28 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/media/media-device.c
>>>>>>> b/drivers/media/media-device.c
>>>>>>> index a31329d..496195e 100644
>>>>>>> --- a/drivers/media/media-device.c
>>>>>>> +++ b/drivers/media/media-device.c
>>>>>>> @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
>>>>>>>  }
>>>>>>>  EXPORT_SYMBOL_GPL(media_device_init);
>>>>>>>
>>>>>>> +struct media_device *media_device_alloc(struct device *dev)
>>>>>>> +{
>>>>>>> +       struct media_device *mdev;
>>>>>>> +
>>>>>>> +       mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
>>>>>>> +       if (!mdev)
>>>>>>> +               return NULL;
>>>>>>> +
>>>>>>> +       mdev->dev = dev;
>>>>>>> +       media_device_init(mdev);
>>>>>>> +
>>>>>>> +       return mdev;
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL_GPL(media_device_alloc);
>>>>>>> +
>>>>>>
>>>>>> One problem with this allocation is, this media device can't be shared
>>>>>> across drivers. For au0828 and snd-usb-audio should be able to share
>>>>>> the media_device. That is what the Media Allocator API patch series
>>>>>> does.
>>>>>
>>>>> No disagreement here, Sakari's patches don't address the issues that the
>>>>> media allocator API fixes. The media allocator API, when ready, should
>>>>> replace (or at least complement, if we decide to keep a simpler API for
>>>>> drivers that don't need to share a media device, but I have no opinion
>>>>> on this at this time) this allocation function.
>>>>
>>>> Media Device Allocator API is ready and reviewed. au0828 uses it as the
>>>> first driver using it. I will be sending out snd-usb-audio patch soon
>>>> that makes use of the shared media device.
>>>
>>> I don't think it would be too difficult to rebase this series on top of
>>> the media allocator API, as all that is needed here is a way to
>>> dynamically allocate the media device in a clean fashion. I don't think
>>> Sakari's patches depend on a specific implementation of
>>> media_device_alloc(). Sakari, please let me know if I got this wrong.
>>
>> Media Device Allocator API is independent as well. It doesn't make any
>> assumptions on media_device register and doesn't care really whether it
>> is shared or not. So I think Sakari's work can use the Media Device
>> Allocator API instead of the allocator routine it is adding.
> 
> I think you're right, and the goal is certainly to merge both. The question is 
> just which series should go in first. I have no strong opinion on that, 
> whichever series is ready first can probably get merged first. I'd just like 
> to get a confirmation from Sakari that both orders would be equally fine.
> 
> This being said, I'd of course like to test the combination of both series to 
> make sure neither introduce an issue for the other one.

Audio maintainer has been waiting for the snd-usb-audio media controller api
work to use it as a reference for changing other drivers to use media controller
api.

I have an urgency here to get the snd-usb-audio. So I am going to work
towards getting Media Device Allocator API, au0828, and snd-usb-audio patches
that uses Media Device Allocator API into 4.10

Sakari's work could take longer as it requires driver changes. So I am going
to assume, it will happen after the work I am doing.

thanks,
-- Shuah

Re: [RFC v4 08/21] media: Enable allocating the media device dynamically

[Sakari Ailus]

Hi Shuah,

On Tue, Nov 08, 2016 at 12:20:29PM -0700, Shuah Khan wrote:
> On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> > From: Sakari Ailus <sakari.ailus@iki.fi>
> >
> > Allow allocating the media device dynamically. As the struct media_device
> > embeds struct media_devnode, the lifetime of that object is that same than
> > that of the media_device.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/media-device.c | 15 +++++++++++++++
> >  include/media/media-device.h | 13 +++++++++++++
> >  2 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index a31329d..496195e 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
> >  }
> >  EXPORT_SYMBOL_GPL(media_device_init);
> >
> > +struct media_device *media_device_alloc(struct device *dev)
> > +{
> > +       struct media_device *mdev;
> > +
> > +       mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> > +       if (!mdev)
> > +               return NULL;
> > +
> > +       mdev->dev = dev;
> > +       media_device_init(mdev);
> > +
> > +       return mdev;
> > +}
> > +EXPORT_SYMBOL_GPL(media_device_alloc);
> > +
> 
> One problem with this allocation is, this media device can't be shared across
> drivers. For au0828 and snd-usb-audio should be able to share the
> media_device. That is what the Media Allocator API patch series does.
> This a quick review and I will review the patch series and get back to
> you.

The assumption has always been there that a media device has a single struct
device related to it. It hasn't been visible in the function call API
though, just in the data structures.

I have to admit I may have forgotten something that was discussed back then,
but do you need to share the same media device over multiple devices in the
system? I don't see that at least in the allocator patch itself. It's
"[PATCH v3] media: Media Device Allocator API", isn't it?

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC v4 08/21] media: Enable allocating the media device dynamically

[Shuah Khan]

On 11/14/2016 06:40 AM, Sakari Ailus wrote:
> Hi Shuah,
> 
> On Tue, Nov 08, 2016 at 12:20:29PM -0700, Shuah Khan wrote:
>> On Tue, Nov 8, 2016 at 6:55 AM, Sakari Ailus
>> <sakari.ailus@linux.intel.com> wrote:
>>> From: Sakari Ailus <sakari.ailus@iki.fi>
>>>
>>> Allow allocating the media device dynamically. As the struct media_device
>>> embeds struct media_devnode, the lifetime of that object is that same than
>>> that of the media_device.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> ---
>>>  drivers/media/media-device.c | 15 +++++++++++++++
>>>  include/media/media-device.h | 13 +++++++++++++
>>>  2 files changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>> index a31329d..496195e 100644
>>> --- a/drivers/media/media-device.c
>>> +++ b/drivers/media/media-device.c
>>> @@ -684,6 +684,21 @@ void media_device_init(struct media_device *mdev)
>>>  }
>>>  EXPORT_SYMBOL_GPL(media_device_init);
>>>
>>> +struct media_device *media_device_alloc(struct device *dev)
>>> +{
>>> +       struct media_device *mdev;
>>> +
>>> +       mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
>>> +       if (!mdev)
>>> +               return NULL;
>>> +
>>> +       mdev->dev = dev;
>>> +       media_device_init(mdev);
>>> +
>>> +       return mdev;
>>> +}
>>> +EXPORT_SYMBOL_GPL(media_device_alloc);
>>> +
>>
>> One problem with this allocation is, this media device can't be shared across
>> drivers. For au0828 and snd-usb-audio should be able to share the
>> media_device. That is what the Media Allocator API patch series does.
>> This a quick review and I will review the patch series and get back to
>> you.
> 
> The assumption has always been there that a media device has a single struct
> device related to it. It hasn't been visible in the function call API
> though, just in the data structures.
> 
> I have to admit I may have forgotten something that was discussed back then,
> but do you need to share the same media device over multiple devices in the
> system? I don't see that at least in the allocator patch itself. It's
> "[PATCH v3] media: Media Device Allocator API", isn't it?
> 


Hi Sakari,

Remember the work I am doing that adds Media Controller API to snd-usb-audio
and au0828 so they can share the media resources. That is where we need the
media device sharable. Please see the following: this patch series includes
the API and au0828 change to use it. I tested it with snd-usb-audio change,
didn't include it at that time.

https://www.mail-archive.com/linux-media@vger.kernel.org/msg98793.html
https://www.mail-archive.com/linux-media@vger.kernel.org/msg97779.html
https://www.mail-archive.com/linux-media@vger.kernel.org/msg97704.html

I am going to be sending the rebased Media Device Allocator patches with both
au0828 and snd-usb-audio using it in a couple of days. No code changes, just
rebased to Linux 4.9-rc4

thanks,
-- Shuah

Re: [RFC v4 14/21] media device: Get the media device driver's device

[Hans Verkuil]

On 08/11/16 14:55, Sakari Ailus wrote:
> The struct device of the media device driver (i.e. not that of the media
> devnode) is pointed to by the media device. The struct device pointer is
> mostly used for debug prints.
>
> Ensure it will stay around as long as the media device does.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/media-device.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 2e52e44..648c64c 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -724,6 +724,7 @@ static void media_device_release(struct media_devnode *devnode)
>  	mdev->entity_internal_idx_max = 0;
>  	media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
>  	mutex_destroy(&mdev->graph_mutex);
> +	put_device(mdev->dev);
>
>  	kfree(mdev);
>  }
> @@ -732,9 +733,15 @@ struct media_device *media_device_alloc(struct device *dev)
>  {
>  	struct media_device *mdev;
>
> +	dev = get_device(dev);
> +	if (!dev)
> +		return NULL;

I don't think this is right. When you allocate the media_device struct 
it should
just be initialized, but not have any side effects until it is actually 
registered.

When the device is registered the device_add call will increase the parent's
refcount as it should, thus ensuring it stays around for as long as is 
needed.

Regards,

	Hans

> +
>  	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> -	if (!mdev)
> +	if (!mdev) {
> +		put_device(dev);
>  		return NULL;
> +	}
>
>  	mdev->dev = dev;
>  	media_device_init(mdev);
>

Re: [RFC v4 14/21] media device: Get the media device driver's device

[Laurent Pinchart]

Hi Hans,

On Tuesday 22 Nov 2016 10:46:31 Hans Verkuil wrote:
> On 08/11/16 14:55, Sakari Ailus wrote:
> > The struct device of the media device driver (i.e. not that of the media
> > devnode) is pointed to by the media device. The struct device pointer is
> > mostly used for debug prints.
> > 
> > Ensure it will stay around as long as the media device does.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > 
> >  drivers/media/media-device.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 2e52e44..648c64c 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -724,6 +724,7 @@ static void media_device_release(struct media_devnode
> > *devnode)
> >  	mdev->entity_internal_idx_max = 0;
> >  	media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
> >  	mutex_destroy(&mdev->graph_mutex);
> > +	put_device(mdev->dev);
> >  	kfree(mdev);
> >  }
> > 
> > @@ -732,9 +733,15 @@ struct media_device *media_device_alloc(struct device
> > *dev)
> >  {
> >  	struct media_device *mdev;
> > 
> > +	dev = get_device(dev);
> > +	if (!dev)
> > +		return NULL;
> 
> I don't think this is right. When you allocate the media_device struct
> it should just be initialized, but not have any side effects until it is
> actually registered.
> 
> When the device is registered the device_add call will increase the parent's
> refcount as it should, thus ensuring it stays around for as long as is
> needed.

We're storing a pointer to dev in mdev a few lines below. As dev is 
refcounted, we need to ensure that we take a reference appropriately. We can 
either borrow a reference taken elsewhere or take our own reference.

Borrowing a reference is only valid if we know it will exist for at least as 
long as we need to borrow it. That might be the case when creating the media 
device as the driver performing the operation should hold a reference to the 
struct device instance (especially given that allocation and registration are 
usually - but not always - performed at probe time for that driver), but it's 
harder to guarantee at unregistration time, especially when userspace can keep 
device nodes open across unregistration. This patch ensures that the pointer 
always remains valid until we stop needing it.

> > +
> > 
> >  	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> > -	if (!mdev)
> > +	if (!mdev) {
> > +		put_device(dev);
> >  		return NULL;
> > +	}
> > 
> >  	mdev->dev = dev;

I would move the get_device() call here:

	mdev->dev = get_device(dev);
	if (!mdev->dev) {
		kfree(mdev);
		return NULL;
	}

I believe it makes the code more readable.

In theory we could even remove the error check, as we have a guarantee that 
the caller gives us a valid struct device reference, but I don't mind keeping 
it.

> >  	media_device_init(mdev);

-- 
Regards,

Laurent Pinchart

Re: [RFC v4 04/21] media: Remove useless curly braces and parentheses

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tuesday 08 Nov 2016 15:55:13 Sakari Ailus wrote:
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/media-device.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 0e07300..bb19c04 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -594,9 +594,8 @@ int __must_check media_device_register_entity(struct
> media_device *mdev, &entity->pads[i].graph_obj);
> 
>  	/* invoke entity_notify callbacks */
> -	list_for_each_entry_safe(notify, next, &mdev->entity_notify, list) {
> -		(notify)->notify(entity, notify->notify_data);
> -	}
> +	list_for_each_entry_safe(notify, next, &mdev->entity_notify, list)
> +		notify->notify(entity, notify->notify_data);
> 
>  	if (mdev->entity_internal_idx_max
> 
>  	    >= mdev->pm_count_walk.ent_enum.idx_max) {

-- 
Regards,

Laurent Pinchart

Re: [RFC v4 05/21] media: devnode: Rename mdev argument as devnode

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tuesday 08 Nov 2016 15:55:14 Sakari Ailus wrote:
> Historically, mdev argument name was being used on both struct
> media_device and struct media_devnode. Recently most occurrences of mdev
> referring to struct media_devnode were replaced by devnode, which makes
> more sense. Fix the last remaining occurrence.
> 
> Fixes: 163f1e93e9950 ("[media] media-devnode: fix namespace mess")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/media-device.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index bb19c04..a9d543f 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -540,9 +540,9 @@ static DEVICE_ATTR(model, S_IRUGO, show_model, NULL);
>   * Registration/unregistration
>   */
> 
> -static void media_device_release(struct media_devnode *mdev)
> +static void media_device_release(struct media_devnode *devnode)
>  {
> -	dev_dbg(mdev->parent, "Media device released\n");
> +	dev_dbg(devnode->parent, "Media device released\n");
>  }
> 
>  /**

-- 
Regards,

Laurent Pinchart

Re: [RFC v4 06/21] media device: Drop nop release callback

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tuesday 08 Nov 2016 15:55:15 Sakari Ailus wrote:
> The release callback is only used to print a debug message. Drop it. (It
> will be re-introduced later in a different form.)
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/media-device.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index a9d543f..a31329d 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -540,11 +540,6 @@ static DEVICE_ATTR(model, S_IRUGO, show_model, NULL);
>   * Registration/unregistration
>   */
> 
> -static void media_device_release(struct media_devnode *devnode)
> -{
> -	dev_dbg(devnode->parent, "Media device released\n");
> -}
> -
>  /**
>   * media_device_register_entity - Register an entity with a media device
>   * @mdev:	The media device
> @@ -706,7 +701,6 @@ int __must_check __media_device_register(struct
> media_device *mdev, /* Register the device node. */
>  	mdev->devnode.fops = &media_device_fops;
>  	mdev->devnode.parent = mdev->dev;
> -	mdev->devnode.release = media_device_release;
> 
>  	/* Set version 0 to indicate user-space that the graph is static */
>  	mdev->topology_version = 0;

-- 
Regards,

Laurent Pinchart

Re: [RFC v4 01/21] Revert "[media] media: fix media devnode ioctl/syscall and unregister race"

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tuesday 08 Nov 2016 15:55:10 Sakari Ailus wrote:
> This reverts commit 6f0dd24a084a ("[media] media: fix media devnode
> ioctl/syscall and unregister race"). The commit was part of an original
> patchset to avoid crashes when an unregistering device is in use.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

For 01/21 to 03/21,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/media-device.c  | 15 +++++++--------
>  drivers/media/media-devnode.c |  8 +-------
>  include/media/media-devnode.h | 16 ++--------------
>  3 files changed, 10 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 2783531..f2525eb 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -730,7 +730,6 @@ int __must_check __media_device_register(struct
> media_device *mdev, if (ret < 0) {
>  		/* devnode free is handled in media_devnode_*() */
>  		mdev->devnode = NULL;
> -		media_devnode_unregister_prepare(devnode);
>  		media_devnode_unregister(devnode);
>  		return ret;
>  	}
> @@ -787,9 +786,6 @@ void media_device_unregister(struct media_device *mdev)
>  		return;
>  	}
> 
> -	/* Clear the devnode register bit to avoid races with media dev open 
*/
> -	media_devnode_unregister_prepare(mdev->devnode);
> -
>  	/* Remove all entities from the media device */
>  	list_for_each_entry_safe(entity, next, &mdev->entities, 
graph_obj.list)
>  		__media_device_unregister_entity(entity);
> @@ -810,10 +806,13 @@ void media_device_unregister(struct media_device
> *mdev)
> 
>  	dev_dbg(mdev->dev, "Media device unregistered\n");
> 
> -	device_remove_file(&mdev->devnode->dev, &dev_attr_model);
> -	media_devnode_unregister(mdev->devnode);
> -	/* devnode free is handled in media_devnode_*() */
> -	mdev->devnode = NULL;
> +	/* Check if mdev devnode was registered */
> +	if (media_devnode_is_registered(mdev->devnode)) {
> +		device_remove_file(&mdev->devnode->dev, &dev_attr_model);
> +		media_devnode_unregister(mdev->devnode);
> +		/* devnode free is handled in media_devnode_*() */
> +		mdev->devnode = NULL;
> +	}
>  }
>  EXPORT_SYMBOL_GPL(media_device_unregister);
> 
> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
> index f2772ba..5b605ff 100644
> --- a/drivers/media/media-devnode.c
> +++ b/drivers/media/media-devnode.c
> @@ -287,7 +287,7 @@ int __must_check media_devnode_register(struct
> media_device *mdev, return ret;
>  }
> 
> -void media_devnode_unregister_prepare(struct media_devnode *devnode)
> +void media_devnode_unregister(struct media_devnode *devnode)
>  {
>  	/* Check if devnode was ever registered at all */
>  	if (!media_devnode_is_registered(devnode))
> @@ -295,12 +295,6 @@ void media_devnode_unregister_prepare(struct
> media_devnode *devnode)
> 
>  	mutex_lock(&media_devnode_lock);
>  	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
> -	mutex_unlock(&media_devnode_lock);
> -}
> -
> -void media_devnode_unregister(struct media_devnode *devnode)
> -{
> -	mutex_lock(&media_devnode_lock);
>  	/* Delete the cdev on this minor as well */
>  	cdev_del(&devnode->cdev);
>  	mutex_unlock(&media_devnode_lock);
> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> index cd23e91..d55ec2b 100644
> --- a/include/media/media-devnode.h
> +++ b/include/media/media-devnode.h
> @@ -128,26 +128,14 @@ int __must_check media_devnode_register(struct
> media_device *mdev, struct module *owner);
> 
>  /**
> - * media_devnode_unregister_prepare - clear the media device node register
> bit - * @devnode: the device node to prepare for unregister
> - *
> - * This clears the passed device register bit. Future open calls will be
> met - * with errors. Should be called before media_devnode_unregister() to
> avoid - * races with unregister and device file open calls.
> - *
> - * This function can safely be called if the device node has never been
> - * registered or has already been unregistered.
> - */
> -void media_devnode_unregister_prepare(struct media_devnode *devnode);
> -
> -/**
>   * media_devnode_unregister - unregister a media device node
>   * @devnode: the device node to unregister
>   *
>   * This unregisters the passed device. Future open calls will be met with
>   * errors.
>   *
> - * Should be called after media_devnode_unregister_prepare()
> + * This function can safely be called if the device node has never been
> + * registered or has already been unregistered.
>   */
>  void media_devnode_unregister(struct media_devnode *devnode);

-- 
Regards,

Laurent Pinchart

Re: [RFC v4 19/21] omap3isp: Allocate the media device dynamically

[Hans Verkuil]

On 08/11/16 14:55, Sakari Ailus wrote:
> Use the new media_device_alloc() API to allocate and release the media
> device.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/platform/omap3isp/isp.c      | 24 +++++++++++++-----------
>  drivers/media/platform/omap3isp/isp.h      |  2 +-
>  drivers/media/platform/omap3isp/ispvideo.c |  2 +-
>  3 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 2e1b17e..8bc7a7c 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1601,8 +1601,8 @@ static void isp_unregister_entities(struct isp_device *isp)
>  	omap3isp_stat_unregister_entities(&isp->isp_hist);
>
>  	v4l2_device_unregister(&isp->v4l2_dev);
> -	media_device_unregister(&isp->media_dev);
> -	media_device_cleanup(&isp->media_dev);
> +	media_device_unregister(isp->media_dev);
> +	media_device_put(isp->media_dev);
>  }
>
>  static int isp_link_entity(
> @@ -1680,14 +1680,16 @@ static int isp_register_entities(struct isp_device *isp)
>  {
>  	int ret;
>
> -	isp->media_dev.dev = isp->dev;
> -	strlcpy(isp->media_dev.model, "TI OMAP3 ISP",
> -		sizeof(isp->media_dev.model));
> -	isp->media_dev.hw_revision = isp->revision;
> -	isp->media_dev.ops = &isp_media_ops;
> -	media_device_init(&isp->media_dev);
> +	isp->media_dev = media_device_alloc(isp->dev, isp);
> +	if (!isp->media_dev)
> +		return -ENOMEM;
> +
> +	strlcpy(isp->media_dev->model, "TI OMAP3 ISP",
> +		sizeof(isp->media_dev->model));
> +	isp->media_dev->hw_revision = isp->revision;
> +	isp->media_dev->ops = &isp_media_ops;
>
> -	isp->v4l2_dev.mdev = &isp->media_dev;
> +	isp->v4l2_dev.mdev = isp->media_dev;
>  	ret = v4l2_device_register(isp->dev, &isp->v4l2_dev);
>  	if (ret < 0) {
>  		dev_err(isp->dev, "%s: V4L2 device registration failed (%d)\n",
> @@ -2165,7 +2167,7 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
>  	struct isp_bus_cfg *bus;
>  	int ret;
>
> -	ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
> +	ret = media_entity_enum_init(&isp->crashed, isp->media_dev);
>  	if (ret)
>  		return ret;
>
> @@ -2183,7 +2185,7 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
>  	if (ret < 0)
>  		return ret;
>
> -	return media_device_register(&isp->media_dev);
> +	return media_device_register(isp->media_dev);

I wonder if this is correct. Usually if the register fails, then the 
release/delete function
has to be called explicitly. That doesn't happen here.

E.g. from adv7604.c:

static int adv76xx_registered(struct v4l2_subdev *sd)
{
         struct adv76xx_state *state = to_state(sd);
         int err;

         err = cec_register_adapter(state->cec_adap);
         if (err)
                 cec_delete_adapter(state->cec_adap);
         return err;
}

>  }
>
>  /*
> diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
> index 7e6f663..7378279 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -176,7 +176,7 @@ struct isp_xclk {
>  struct isp_device {
>  	struct v4l2_device v4l2_dev;
>  	struct v4l2_async_notifier notifier;
> -	struct media_device media_dev;
> +	struct media_device *media_dev;
>  	struct device *dev;
>  	u32 revision;
>
> diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
> index 7354469..33e74b9 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -1104,7 +1104,7 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
>  	pipe = video->video.entity.pipe
>  	     ? to_isp_pipeline(&video->video.entity) : &video->pipe;
>
> -	ret = media_entity_enum_init(&pipe->ent_enum, &video->isp->media_dev);
> +	ret = media_entity_enum_init(&pipe->ent_enum, video->isp->media_dev);
>  	if (ret)
>  		goto err_enum_init;
>
>

Regards,

	Hans

Re: [RFC v4 14/21] media device: Get the media device driver's device

[Hans Verkuil]

On 22/11/16 10:58, Laurent Pinchart wrote:
> Hi Hans,
>
> On Tuesday 22 Nov 2016 10:46:31 Hans Verkuil wrote:
>> On 08/11/16 14:55, Sakari Ailus wrote:
>>> The struct device of the media device driver (i.e. not that of the media
>>> devnode) is pointed to by the media device. The struct device pointer is
>>> mostly used for debug prints.
>>>
>>> Ensure it will stay around as long as the media device does.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> ---
>>>
>>>  drivers/media/media-device.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>> index 2e52e44..648c64c 100644
>>> --- a/drivers/media/media-device.c
>>> +++ b/drivers/media/media-device.c
>>> @@ -724,6 +724,7 @@ static void media_device_release(struct media_devnode
>>> *devnode)
>>>  	mdev->entity_internal_idx_max = 0;
>>>  	media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
>>>  	mutex_destroy(&mdev->graph_mutex);
>>> +	put_device(mdev->dev);
>>>  	kfree(mdev);
>>>  }
>>>
>>> @@ -732,9 +733,15 @@ struct media_device *media_device_alloc(struct device
>>> *dev)
>>>  {
>>>  	struct media_device *mdev;
>>>
>>> +	dev = get_device(dev);
>>> +	if (!dev)
>>> +		return NULL;
>>
>> I don't think this is right. When you allocate the media_device struct
>> it should just be initialized, but not have any side effects until it is
>> actually registered.
>>
>> When the device is registered the device_add call will increase the parent's
>> refcount as it should, thus ensuring it stays around for as long as is
>> needed.
>
> We're storing a pointer to dev in mdev a few lines below. As dev is
> refcounted, we need to ensure that we take a reference appropriately. We can
> either borrow a reference taken elsewhere or take our own reference.
>
> Borrowing a reference is only valid if we know it will exist for at least as
> long as we need to borrow it. That might be the case when creating the media
> device as the driver performing the operation should hold a reference to the
> struct device instance (especially given that allocation and registration are
> usually - but not always - performed at probe time for that driver), but it's
> harder to guarantee at unregistration time, especially when userspace can keep
> device nodes open across unregistration. This patch ensures that the pointer
> always remains valid until we stop needing it.

I disagree. There is no reason to keep the parent device in memory once the
media devnode is unregistered.

It seems to be pretty much only used for some debugging. I suspect that in
almost all cases the debugging happens when the devnode is registered, and
not when it is unregistered. But in that case you can also use &devnode.dev
as the device pointer for dev_dbg, or use pr_debug.

Looking at what the CEC framework does I see that I pass a device pointer
to the allocate function, but I really don't need to do that. It is not
used anywhere until the register function, so the parent device pointer
should be passed as an argument to the register function, not to the
allocate function.

BTW, I would very much prefer it if mdev->dev is renamed to mdev->parent.
Or better yet, just dropped completely since it is also available as
mdev->devnode.parent. And even devnode.parent can be dropped and just
use mdev->devnode.dev.parent.

I plan on posting such a patch for the cec framework as well, since
it avoids having duplicates of the same device parent pointer in the
data structures.

Regards,

	Hans

Re: [RFC v4 14/21] media device: Get the media device driver's device

[Laurent Pinchart]

On Tuesday 22 Nov 2016 11:58:43 Hans Verkuil wrote:
> On 22/11/16 10:58, Laurent Pinchart wrote:
> > Hi Hans,
> > 
> > On Tuesday 22 Nov 2016 10:46:31 Hans Verkuil wrote:
> >> On 08/11/16 14:55, Sakari Ailus wrote:
> >>> The struct device of the media device driver (i.e. not that of the media
> >>> devnode) is pointed to by the media device. The struct device pointer is
> >>> mostly used for debug prints.
> >>> 
> >>> Ensure it will stay around as long as the media device does.
> >>> 
> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>> ---
> >>> 
> >>>  drivers/media/media-device.c | 9 ++++++++-
> >>>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >>> index 2e52e44..648c64c 100644
> >>> --- a/drivers/media/media-device.c
> >>> +++ b/drivers/media/media-device.c
> >>> @@ -724,6 +724,7 @@ static void media_device_release(struct
> >>> media_devnode
> >>> *devnode)
> >>> 
> >>>  	mdev->entity_internal_idx_max = 0;
> >>>  	media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
> >>>  	mutex_destroy(&mdev->graph_mutex);
> >>> 
> >>> +	put_device(mdev->dev);
> >>> 
> >>>  	kfree(mdev);
> >>>  
> >>>  }
> >>> 
> >>> @@ -732,9 +733,15 @@ struct media_device *media_device_alloc(struct
> >>> device
> >>> *dev)
> >>> 
> >>>  {
> >>>  
> >>>  	struct media_device *mdev;
> >>> 
> >>> +	dev = get_device(dev);
> >>> +	if (!dev)
> >>> +		return NULL;
> >> 
> >> I don't think this is right. When you allocate the media_device struct
> >> it should just be initialized, but not have any side effects until it is
> >> actually registered.
> >> 
> >> When the device is registered the device_add call will increase the
> >> parent's refcount as it should, thus ensuring it stays around for as
> >> long as is needed.
> > 
> > We're storing a pointer to dev in mdev a few lines below. As dev is
> > refcounted, we need to ensure that we take a reference appropriately. We
> > can either borrow a reference taken elsewhere or take our own reference.
> > 
> > Borrowing a reference is only valid if we know it will exist for at least
> > as long as we need to borrow it. That might be the case when creating the
> > media device as the driver performing the operation should hold a
> > reference to the struct device instance (especially given that allocation
> > and registration are usually - but not always - performed at probe time
> > for that driver), but it's harder to guarantee at unregistration time,
> > especially when userspace can keep device nodes open across
> > unregistration. This patch ensures that the pointer always remains valid
> > until we stop needing it.
> 
> I disagree. There is no reason to keep the parent device in memory once the
> media devnode is unregistered.

There's a very big one: the media device is accessed through a large number of 
APIs, not only through its own devnode. It can for instance be accessed 
through V4L2 devnodes, and thus has to live as long as *anything* can access 
it.

struct media_devnode was a very very bad idea. The original goal was to share 
the implementation with the V4L2 devnodes, but when that got rejected I really 
should have merged struct media_device and struct media_devnode into a single 
structure. We can keep media_devnode separate if that is believed to improve 
readability of the code, but there is absolutely no reason for allocating 
media_devnode separately from media_device.

> It seems to be pretty much only used for some debugging. I suspect that in
> almost all cases the debugging happens when the devnode is registered, and
> not when it is unregistered. But in that case you can also use &devnode.dev
> as the device pointer for dev_dbg, or use pr_debug.
> 
> Looking at what the CEC framework does I see that I pass a device pointer
> to the allocate function, but I really don't need to do that. It is not
> used anywhere until the register function, so the parent device pointer
> should be passed as an argument to the register function, not to the
> allocate function.
> 
> BTW, I would very much prefer it if mdev->dev is renamed to mdev->parent.
> Or better yet, just dropped completely since it is also available as
> mdev->devnode.parent. And even devnode.parent can be dropped and just
> use mdev->devnode.dev.parent.
> 
> I plan on posting such a patch for the cec framework as well, since
> it avoids having duplicates of the same device parent pointer in the
> data structures.

-- 
Regards,

Laurent Pinchart

Re: [RFC v4 19/21] omap3isp: Allocate the media device dynamically

[Sakari Ailus]

Hi Hans,

On Tue, Nov 22, 2016 at 11:05:49AM +0100, Hans Verkuil wrote:
...
> >@@ -2183,7 +2185,7 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
> > 	if (ret < 0)
> > 		return ret;
> >
> >-	return media_device_register(&isp->media_dev);
> >+	return media_device_register(isp->media_dev);
> 
> I wonder if this is correct. Usually if the register fails, then the
> release/delete function
> has to be called explicitly. That doesn't happen here.

This patch is really about making the media_dev a pointer in struct
omap3isp_device. Currently the cleanup takes place when the device is
unbound. That's perhaps not ideal but on the other hand optimising error
handling is often just not worth it.

Improvements could be done how the async framework handles errors but that
shouldn't be in the scope of this patchset.

> 
> E.g. from adv7604.c:
> 
> static int adv76xx_registered(struct v4l2_subdev *sd)
> {
>         struct adv76xx_state *state = to_state(sd);
>         int err;
> 
>         err = cec_register_adapter(state->cec_adap);
>         if (err)
>                 cec_delete_adapter(state->cec_adap);
>         return err;
> }

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk