Re: [PATCH v3.16 2/2] v4l: event: Add subscription to list before calling "add" operation

[Greg KH]

On Thu, Nov 08, 2018 at 02:03:50PM +0200, Sakari Ailus wrote:
> [ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
> 
> Patch ad608fbcf166 changed how events were subscribed to address an issue
> elsewhere. As a side effect of that change, the "add" callback was called
> before the event subscription was added to the list of subscribed events,
> causing the first event queued by the add callback (and possibly other
> events arriving soon afterwards) to be lost.
> 
> Fix this by adding the subscription to the list before calling the "add"
> callback, and clean up afterwards if that fails.
> 
> Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions 
> while accessed")

Now applied, thanks.

greg k-h

Re: [PATCH v3.16 1/2] v4l: event: Prevent freeing event subscriptions while accessed

[Greg KH]

On Thu, Nov 08, 2018 at 02:03:49PM +0200, Sakari Ailus wrote:
> [ upstream commit ad608fbcf166fec809e402d548761768f602702c ]

This is already in 3.18.124.

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH vicodec] media: pvrusb2: replace `printk` with `pr_*`

[Greg KH]

On Mon, Oct 08, 2018 at 03:29:03PM +0200, Hans Verkuil wrote:
> On 10/08/2018 03:07 PM, Greg KH wrote:
> > On Mon, Oct 08, 2018 at 03:06:47PM +0300, Dafna Hirschfeld wrote:
> >> Replace calls to `printk` with the appropriate `pr_*`
> >> macro.
> >>
> >> Signed-off-by: Dafna Hirschfeld 
> >> ---
> >>  drivers/media/usb/pvrusb2/pvrusb2-debug.h|  2 +-
> >>  drivers/media/usb/pvrusb2/pvrusb2-hdw.c  |  8 +++---
> >>  drivers/media/usb/pvrusb2/pvrusb2-i2c-core.c | 28 +---
> >>  drivers/media/usb/pvrusb2/pvrusb2-main.c |  4 +--
> >>  drivers/media/usb/pvrusb2/pvrusb2-v4l2.c |  4 +--
> >>  5 files changed, 22 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-debug.h 
> >> b/drivers/media/usb/pvrusb2/pvrusb2-debug.h
> >> index 5cd16292e2fa..1323f949f454 100644
> >> --- a/drivers/media/usb/pvrusb2/pvrusb2-debug.h
> >> +++ b/drivers/media/usb/pvrusb2/pvrusb2-debug.h
> >> @@ -17,7 +17,7 @@
> >>  
> >>  extern int pvrusb2_debug;
> >>  
> >> -#define pvr2_trace(msk, fmt, arg...) do {if(msk & pvrusb2_debug) 
> >> printk(KERN_INFO "pvrusb2: " fmt "\n", ##arg); } while (0)
> >> +#define pvr2_trace(msk, fmt, arg...) do {if (msk & pvrusb2_debug) 
> >> pr_info("pvrusb2: " fmt "\n", ##arg); } while (0)
> > 
> > You should not need prefixes for pr_info() calls.
> > 
> >>  
> >>  /* These are listed in *rough* order of decreasing usefulness and
> >> increasing noise level. */
> >> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c 
> >> b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> >> index a8519da0020b..7702285c1519 100644
> >> --- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> >> +++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> >> @@ -3293,12 +3293,12 @@ void pvr2_hdw_trigger_module_log(struct pvr2_hdw 
> >> *hdw)
> >>int nr = pvr2_hdw_get_unit_number(hdw);
> >>LOCK_TAKE(hdw->big_lock);
> >>do {
> >> -  printk(KERN_INFO "pvrusb2: =  START STATUS CARD 
> >> #%d  =\n", nr);
> >> +  pr_info("pvrusb2: =  START STATUS CARD #%d  
> >> =\n", nr);
> > 
> > A driver should be using dev_info(), not pr_*.
> 
> pvrusb2 is an exception due to historical reasons. I'd rather not switch
> over to dev_*.

Why should a historical reason be needed to fix up code to be correct?

Why not use the correct functions?  You will just constantly be
rejecting patches like this for the next 20+ years :(

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH vicodec] media: pvrusb2: replace `printk` with `pr_*`

[Greg KH]

On Mon, Oct 08, 2018 at 03:06:47PM +0300, Dafna Hirschfeld wrote:
> Replace calls to `printk` with the appropriate `pr_*`
> macro.
> 
> Signed-off-by: Dafna Hirschfeld 
> ---
>  drivers/media/usb/pvrusb2/pvrusb2-debug.h|  2 +-
>  drivers/media/usb/pvrusb2/pvrusb2-hdw.c  |  8 +++---
>  drivers/media/usb/pvrusb2/pvrusb2-i2c-core.c | 28 +---
>  drivers/media/usb/pvrusb2/pvrusb2-main.c |  4 +--
>  drivers/media/usb/pvrusb2/pvrusb2-v4l2.c |  4 +--
>  5 files changed, 22 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-debug.h 
> b/drivers/media/usb/pvrusb2/pvrusb2-debug.h
> index 5cd16292e2fa..1323f949f454 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-debug.h
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-debug.h
> @@ -17,7 +17,7 @@
>  
>  extern int pvrusb2_debug;
>  
> -#define pvr2_trace(msk, fmt, arg...) do {if(msk & pvrusb2_debug) 
> printk(KERN_INFO "pvrusb2: " fmt "\n", ##arg); } while (0)
> +#define pvr2_trace(msk, fmt, arg...) do {if (msk & pvrusb2_debug) 
> pr_info("pvrusb2: " fmt "\n", ##arg); } while (0)

You should not need prefixes for pr_info() calls.

>  
>  /* These are listed in *rough* order of decreasing usefulness and
> increasing noise level. */
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c 
> b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> index a8519da0020b..7702285c1519 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
> @@ -3293,12 +3293,12 @@ void pvr2_hdw_trigger_module_log(struct pvr2_hdw *hdw)
>   int nr = pvr2_hdw_get_unit_number(hdw);
>   LOCK_TAKE(hdw->big_lock);
>   do {
> - printk(KERN_INFO "pvrusb2: =  START STATUS CARD 
> #%d  =\n", nr);
> + pr_info("pvrusb2: =  START STATUS CARD #%d  
> =\n", nr);

A driver should be using dev_info(), not pr_*.

Also, for the outreachy application process, I can not accept patches
outside of drivers/staging/.

sorry,

greg k-h

Re: [PATCH] media: uvcvideo: Support UVC 1.5 video probe & commit controls

[Greg KH]

On Sun, Sep 30, 2018 at 01:38:16PM +0300, Laurent Pinchart wrote:
> From: ming_qian 
> 
> commit f620d1d7afc7db57ab59f35000752840c91f67e7 upstream.
> 
> The length of UVC 1.5 video control is 48, and it is 34 for UVC 1.1.
> Change it to 48 for UVC 1.5 device, and the UVC 1.5 device can be
> recognized.
> 
> More changes to the driver are needed for full UVC 1.5 compatibility.
> However, at least the UVC 1.5 Realtek RTS5847/RTS5852 cameras have been
> reported to work well.
> 
> [laurent.pinch...@ideasonboard.com: Factor out code to helper function, 
> update size checks]
> 
> Signed-off-by: ming_qian 
> Signed-off-by: Laurent Pinchart 
> Tested-by: Kai-Heng Feng 
> Tested-by: Ana Guerrero Lopez 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/media/usb/uvc/uvc_video.c | 24 ++--
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> Hello,
> 
> This patch was originally marked as a stable candidate, but a driver-wide
> switch from __u{8,16,32} to u{8,16,32} created conflicts that prevented
> backporting. This version fixes the conflicts and is otherwise not modified.
> 
> The decision to mark the patch as a stable candidate came after reports from
> distro users that their UVC 1.5 camera was otherwise unusable. A guide has
> even been published to tell Debian users how to patch their kernel to fix the
> problem. Including the fix in stable will make their life much easier.

Now queued up, thanks.

greg k-h

Re: [PATCH] media: cleanup fall-through comments

[Greg KH]

On Tue, Aug 07, 2018 at 09:33:03AM -0700, Nick Desaulniers wrote:
> On Tue, Aug 7, 2018 at 5:07 AM Mauro Carvalho Chehab
>  wrote:
> >
> > As Ian pointed out, adding a '-' to the fallthrough seems to meet
> > the regex requirements at level 3 of the warning, at least when
> > the comment fits into a single line.
> >
> > So, replace by a single line the comments that were broken into
> > multiple lines just to make gcc -Wimplicit-fallthrough=3 happy.
> >
> > Suggested-by: Ian Arkver 
> > Signed-off-by: Mauro Carvalho Chehab 
> > ---
> >  drivers/media/dvb-frontends/drx39xyj/drxj.c |  3 +--
> >  drivers/media/dvb-frontends/drxd_hard.c |  6 ++
> >  drivers/media/dvb-frontends/drxk_hard.c | 18 ++
> >  drivers/staging/media/imx/imx-media-csi.c   |  3 +--
> >  4 files changed, 10 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/media/dvb-frontends/drx39xyj/drxj.c 
> > b/drivers/media/dvb-frontends/drx39xyj/drxj.c
> > index 2ddb7d218ace..2948d12d7c14 100644
> > --- a/drivers/media/dvb-frontends/drx39xyj/drxj.c
> > +++ b/drivers/media/dvb-frontends/drx39xyj/drxj.c
> > @@ -2841,8 +2841,7 @@ ctrl_set_cfg_mpeg_output(struct drx_demod_instance 
> > *demod, struct drx_cfg_mpeg_o
> > /* coef = 188/204  */
> > max_bit_rate =
> > (ext_attr->curr_symbol_rate / 8) * nr_bits * 
> > 188;
> > -   /* pass through as b/c Annex A/c need following 
> > settings */
> > -   /* fall-through */
> > +   /* fall-through - as b/c Annex A/C need following 
> > settings */
> > case DRX_STANDARD_ITU_B:
> > rc = drxj_dap_write_reg16(dev_addr, 
> > FEC_OC_FCT_USAGE__A, FEC_OC_FCT_USAGE__PRE, 0);
> > if (rc != 0) {
> > diff --git a/drivers/media/dvb-frontends/drxd_hard.c 
> > b/drivers/media/dvb-frontends/drxd_hard.c
> > index 11fc259e4383..684d428efb0d 100644
> > --- a/drivers/media/dvb-frontends/drxd_hard.c
> > +++ b/drivers/media/dvb-frontends/drxd_hard.c
> > @@ -1970,8 +1970,7 @@ static int DRX_Start(struct drxd_state *state, s32 
> > off)
> > switch (p->transmission_mode) {
> > default:/* Not set, detect it automatically */
> > operationMode |= SC_RA_RAM_OP_AUTO_MODE__M;
> > -   /* try first guess DRX_FFTMODE_8K */
> > -   /* fall through */
> > +   /* fall through - try first guess DRX_FFTMODE_8K */
> > case TRANSMISSION_MODE_8K:
> > transmissionParams |= SC_RA_RAM_OP_PARAM_MODE_8K;
> > if (state->type_A) {
> > @@ -2144,8 +2143,7 @@ static int DRX_Start(struct drxd_state *state, s32 
> > off)
> > switch (p->modulation) {
> > default:
> > operationMode |= SC_RA_RAM_OP_AUTO_CONST__M;
> > -   /* try first guess DRX_CONSTELLATION_QAM64 */
> > -   /* fall through */
> > +   /* fall through - try first guess 
> > DRX_CONSTELLATION_QAM64 */
> > case QAM_64:
> > transmissionParams |= 
> > SC_RA_RAM_OP_PARAM_CONST_QAM64;
> > if (state->type_A) {
> > diff --git a/drivers/media/dvb-frontends/drxk_hard.c 
> > b/drivers/media/dvb-frontends/drxk_hard.c
> > index ac10781d3550..f1886945a7bc 100644
> > --- a/drivers/media/dvb-frontends/drxk_hard.c
> > +++ b/drivers/media/dvb-frontends/drxk_hard.c
> > @@ -3270,13 +3270,11 @@ static int dvbt_sc_command(struct drxk_state *state,
> > case OFDM_SC_RA_RAM_CMD_SET_PREF_PARAM:
> > case OFDM_SC_RA_RAM_CMD_PROGRAM_PARAM:
> > status |= write16(state, OFDM_SC_RA_RAM_PARAM1__A, param1);
> > -   /* All commands using 1 parameters */
> > -   /* fall through */
> > +   /* fall through - All commands using 1 parameters */
> > case OFDM_SC_RA_RAM_CMD_SET_ECHO_TIMING:
> > case OFDM_SC_RA_RAM_CMD_USER_IO:
> > status |= write16(state, OFDM_SC_RA_RAM_PARAM0__A, param0);
> > -   /* All commands using 0 parameters */
> > -   /* fall through */
> > +   /* fall through - All commands using 0 parameters */
> > case OFDM_SC_RA_RAM_CMD_GET_OP_PARAM:
> > case OFDM_SC_RA_RAM_CMD_NULL:
> > /* Write command */
> > @@ -3784,8 +3782,7 @@ static int set_dvbt(struct drxk_state *state, u16 
> > intermediate_freqk_hz,
> > case TRANSMISSION_MODE_AUTO:
> > default:
> > operation_mode |= OFDM_SC_RA_RAM_OP_AUTO_MODE__M;
> > -   /* try first guess DRX_FFTMODE_8K */
> > -   /* fall through */
> > +   /* fall through - try first guess DRX_FFTMODE_8K */
> > case TRANSMISSION_MODE_8K:
> > 

Re: [PATCH] usbtv: Fix refcounting mixup

[Greg KH]

On Tue, May 15, 2018 at 01:51:37PM +0200, Oliver Neukum wrote:
> The premature free in the error path is blocked by V4L
> refcounting, not USB refcounting. Thanks to
> Ben Hutchings for review.
> 
> Signed-off-by: Oliver Neukum 
> Fixes: 50e704453553 ("media: usbtv: prevent double free in error case")

Please add:
Cc: stable 

to this patch as well so I pick up the fix in the stable trees.

And a "Reported-by:" line would be nice as well to give credit to Ben :)

thanks,

greg k-h

Re: [PATCH stable v4.15 1/3] media: staging: lirc_zilog: broken reference counting

[Greg KH]

On Mon, Apr 16, 2018 at 10:15:28AM +0100, Sean Young wrote:
> On Mon, Apr 16, 2018 at 10:50:15AM +0200, Greg KH wrote:
> > On Mon, Apr 16, 2018 at 09:43:45AM +0100, Sean Young wrote:
> > > On Mon, Apr 16, 2018 at 09:52:28AM +0200, Greg KH wrote:
> > > > What is the git commit id of this patch, and the other patches in this
> > > > series and the 4.14 patch series that you sent out?
> > > 
> > > lirc_zilog was dropped in v4.16, so this can't be patched upstream.
> > 
> > Ah you are right, should we just ditch them here as well as they
> > obviously do not work? :)
> > 
> > > > Please read:
> > > > 
> > > > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > > > for how to do this in a way that I can pick them up.
> > > 
> > > These patches have been tested with different types of hardware. Is there
> > > anything else I can do to get these patches included?
> > 
> > When submitting patches to stable, you need to be explicit as to why
> > they are needed, and if they are not upstream, why not.
> > 
> > In this case, for obviously broken code that is not used anymore (as
> > it is gone in 4.16), why don't we just take the patch that removed the
> > driver to the stable trees as well?
> 
> Well in v4.16 the ir-kbd-i2c.c driver can do what the lirc_zilog does in
> v4.15 (and earlier), so it wasn't ditched as such. It's a case of replaced
> by mainline.
> 
> Since I was getting bug reports on it, there must be users of the lirc_zilog
> driver.
> 
> That being said, the old lirc_dev and lirc_zilog is pretty awful code.

Ok, I've queued these up for 4.14.y now.  4.15 is end-of-life, so I
can't apply these patches there, sorry.

greg k-h

Re: [PATCH stable v4.15 1/3] media: staging: lirc_zilog: broken reference counting

[Greg KH]

On Mon, Apr 16, 2018 at 09:43:45AM +0100, Sean Young wrote:
> On Mon, Apr 16, 2018 at 09:52:28AM +0200, Greg KH wrote:
> > On Sun, Apr 15, 2018 at 10:54:20AM +0100, Sean Young wrote:
> > > commit 615cd3fe6ccc ("[media] media: lirc_dev: make better use of
> > > file->private_data") removed the reference get from open, so on the first
> > > close the reference count hits zero and the lirc device is freed.
> > > 
> > > BUG: unable to handle kernel NULL pointer dereference at 0040
> > > IP: lirc_thread+0x94/0x520 [lirc_zilog]
> > > PGD 22d69c067 P4D 22d69c067 PUD 22d69d067 PMD 0
> > > Oops:  [#1] SMP NOPTI
> > > CPU: 2 PID: 701 Comm: zilog-rx-i2c-7 Tainted: P C OE
> > > 4.15.14-300.fc27.x86_64 #1
> > > Hardware name: Gigabyte Technology Co., Ltd. 
> > > GA-MA790FXT-UD5P/GA-MA790FXT-UD5P, BIOS F6 08/06/2009
> > > RIP: 0010:lirc_thread+0x94/0x520 [lirc_zilog]
> > > RSP: 0018:b482c131be98 EFLAGS: 00010246
> > > RAX:  RBX: 8fdabf056000 RCX: 
> > > RDX:  RSI: 0246 RDI: 0246
> > > RBP: 8fdab740af00 R08: 8fdacfd214a0 R09: 
> > > R10:  R11: 0040 R12: b482c10dba48
> > > R13: 8fdabea89e00 R14: 8fdab740af00 R15: c0b5e500
> > > FS:  () GS:8fdacfd0() 
> > > knlGS:
> > > CS:  0010 DS:  ES:  CR0: 80050033
> > > CR2: 0040 CR3: 0002124c CR4: 06e0
> > > Call Trace:
> > >  ? __schedule+0x247/0x880
> > >  ? get_ir_tx+0x40/0x40 [lirc_zilog]
> > >  kthread+0x113/0x130
> > >  ? kthread_create_worker_on_cpu+0x70/0x70
> > >  ? do_syscall_64+0x74/0x180
> > >  ? SyS_exit_group+0x10/0x10
> > >  ret_from_fork+0x22/0x40
> > > Code: 20 8b 85 80 00 00 00 85 c0 0f 84 a6 00 00 00 bf 04 01 00 00 e8 ee 
> > > 34 d4 d7 e8 69 88 56 d7 84 c0 75 69 48 8b 45 18 c6 44 24 37 00 <48> 8b 58 
> > > 40 4c 8d 6b 18 4c 89 ef e8 fc 4d d4 d7 4c 89 ef 48 89
> > > RIP: lirc_thread+0x94/0x520 [lirc_zilog] RSP: b482c131be98
> > > CR2: 0040
> > > This code has been replaced completely in kernel v4.16 by a new driver,
> > > see commit acaa34bf06e9 ("media: rc: implement zilog transmitter"), and
> > > commit f95367a7b758 ("media: staging: remove lirc_zilog driver").
> > > 
> > > Fixes: 615cd3fe6ccc ("[media] media: lirc_dev: make better use of 
> > > file->private_data")
> > > 
> > > Cc: sta...@vger.kernel.org # v4.15
> > > Reported-by: Warren Sturm <warren.st...@gmail.com>
> > > Tested-by: Warren Sturm <warren.st...@gmail.com>
> > > Signed-off-by: Sean Young <s...@mess.org>
> > > ---
> > >  drivers/staging/media/lirc/lirc_zilog.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
> > > b/drivers/staging/media/lirc/lirc_zilog.c
> > > index 6bd0717bf76e..bf6869e48a0f 100644
> > > --- a/drivers/staging/media/lirc/lirc_zilog.c
> > > +++ b/drivers/staging/media/lirc/lirc_zilog.c
> > > @@ -1291,6 +1291,7 @@ static int open(struct inode *node, struct file 
> > > *filep)
> > >  
> > >   lirc_init_pdata(node, filep);
> > >   ir = lirc_get_pdata(filep);
> > > + get_ir_device(ir, false);
> > >  
> > >   atomic_inc(>open_count);
> > >  
> > > -- 
> > > 2.14.3
> > 
> > What is the git commit id of this patch, and the other patches in this
> > series and the 4.14 patch series that you sent out?
> 
> lirc_zilog was dropped in v4.16, so this can't be patched upstream.

Ah you are right, should we just ditch them here as well as they
obviously do not work? :)

> > Please read:
> > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > for how to do this in a way that I can pick them up.
> 
> These patches have been tested with different types of hardware. Is there
> anything else I can do to get these patches included?

When submitting patches to stable, you need to be explicit as to why
they are needed, and if they are not upstream, why not.

In this case, for obviously broken code that is not used anymore (as
it is gone in 4.16), why don't we just take the patch that removed the
driver to the stable trees as well?

thanks,

greg k-h

Re: [PATCH stable v4.15 1/3] media: staging: lirc_zilog: broken reference counting

[Greg KH]

On Sun, Apr 15, 2018 at 10:54:20AM +0100, Sean Young wrote:
> commit 615cd3fe6ccc ("[media] media: lirc_dev: make better use of
> file->private_data") removed the reference get from open, so on the first
> close the reference count hits zero and the lirc device is freed.
> 
> BUG: unable to handle kernel NULL pointer dereference at 0040
> IP: lirc_thread+0x94/0x520 [lirc_zilog]
> PGD 22d69c067 P4D 22d69c067 PUD 22d69d067 PMD 0
> Oops:  [#1] SMP NOPTI
> CPU: 2 PID: 701 Comm: zilog-rx-i2c-7 Tainted: P C OE
> 4.15.14-300.fc27.x86_64 #1
> Hardware name: Gigabyte Technology Co., Ltd. 
> GA-MA790FXT-UD5P/GA-MA790FXT-UD5P, BIOS F6 08/06/2009
> RIP: 0010:lirc_thread+0x94/0x520 [lirc_zilog]
> RSP: 0018:b482c131be98 EFLAGS: 00010246
> RAX:  RBX: 8fdabf056000 RCX: 
> RDX:  RSI: 0246 RDI: 0246
> RBP: 8fdab740af00 R08: 8fdacfd214a0 R09: 
> R10:  R11: 0040 R12: b482c10dba48
> R13: 8fdabea89e00 R14: 8fdab740af00 R15: c0b5e500
> FS:  () GS:8fdacfd0() knlGS:
> CS:  0010 DS:  ES:  CR0: 80050033
> CR2: 0040 CR3: 0002124c CR4: 06e0
> Call Trace:
>  ? __schedule+0x247/0x880
>  ? get_ir_tx+0x40/0x40 [lirc_zilog]
>  kthread+0x113/0x130
>  ? kthread_create_worker_on_cpu+0x70/0x70
>  ? do_syscall_64+0x74/0x180
>  ? SyS_exit_group+0x10/0x10
>  ret_from_fork+0x22/0x40
> Code: 20 8b 85 80 00 00 00 85 c0 0f 84 a6 00 00 00 bf 04 01 00 00 e8 ee 34 d4 
> d7 e8 69 88 56 d7 84 c0 75 69 48 8b 45 18 c6 44 24 37 00 <48> 8b 58 40 4c 8d 
> 6b 18 4c 89 ef e8 fc 4d d4 d7 4c 89 ef 48 89
> RIP: lirc_thread+0x94/0x520 [lirc_zilog] RSP: b482c131be98
> CR2: 0040
> This code has been replaced completely in kernel v4.16 by a new driver,
> see commit acaa34bf06e9 ("media: rc: implement zilog transmitter"), and
> commit f95367a7b758 ("media: staging: remove lirc_zilog driver").
> 
> Fixes: 615cd3fe6ccc ("[media] media: lirc_dev: make better use of 
> file->private_data")
> 
> Cc: sta...@vger.kernel.org # v4.15
> Reported-by: Warren Sturm 
> Tested-by: Warren Sturm 
> Signed-off-by: Sean Young 
> ---
>  drivers/staging/media/lirc/lirc_zilog.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
> b/drivers/staging/media/lirc/lirc_zilog.c
> index 6bd0717bf76e..bf6869e48a0f 100644
> --- a/drivers/staging/media/lirc/lirc_zilog.c
> +++ b/drivers/staging/media/lirc/lirc_zilog.c
> @@ -1291,6 +1291,7 @@ static int open(struct inode *node, struct file *filep)
>  
>   lirc_init_pdata(node, filep);
>   ir = lirc_get_pdata(filep);
> + get_ir_device(ir, false);
>  
>   atomic_inc(>open_count);
>  
> -- 
> 2.14.3

What is the git commit id of this patch, and the other patches in this
series and the 4.14 patch series that you sent out?

Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this in a way that I can pick them up.

thanks,

greg k-h

Re: [PATCH for v3.18 00/18] Backport CVE-2017-13166 fixes to Kernel 3.18

[Greg KH]

On Wed, Mar 28, 2018 at 03:12:19PM -0300, Mauro Carvalho Chehab wrote:
> Hi Greg,
> 
> Those are the backports meant to solve CVE-2017-13166 on Kernel 3.18.
> 
> It contains two v4l2-ctrls fixes that are required to avoid crashes
> at the test application.
> 
> I wrote two patches myself for Kernel 3.18 in order to solve some
> issues specific for Kernel 3.18 with aren't needed upstream.
> one is actually a one-line change backport. The other one makes
> sure that both 32-bits and 64-bits version of some ioctl calls
> will return the same value for a reserved field.
> 
> I noticed an extra bug while testing it, but the bug also hits upstream,
> and should be backported all the way down all stable/LTS versions.
> So, I'll send it the usual way, after merging upsream.

I've queued these all up now, thanks.

greg k-h

Re: [PATCH for v3.18 00/18] Backport CVE-2017-13166 fixes to Kernel 3.18

[Greg KH]

On Thu, Mar 29, 2018 at 03:39:54PM +0900, Inki Dae wrote:
> 2018년 03월 29일 13:25에 Greg KH 이(가) 쓴 글:
> > On Thu, Mar 29, 2018 at 08:22:08AM +0900, Inki Dae wrote:
> >> Really thanks for doing this. :) There would be many users who use
> >> Linux-3.18 for their products yet.
> > 
> > For new products?  They really should not be.  The kernel is officially
> 
> Really no. Old products would still be using Linux-3.18 kernel without
> kernel upgrade. For new product, most of SoC vendors will use
> Linux-4.x including us.
> Actually, we are preparing for kernel upgrade for some devices even
> some old devices (to Linux-4.14-LTS) and almost done.

That is great to hear.

> > What is keeping you on 3.18.y and not allowing you to move to a newer
> > kernel version?
> 
> We also want to move to latest kernel version. However, there is a case that 
> we cannot upgrade the kernel.
> In case that SoC vendor never share firmwares and relevant data
> sheets, we cannot upgrade the kernel. However, we have to resolve the
> security issues for users of this device.

It sounds like you need to be getting those security updates from those
SoC vendors, as they are the ones you are paying for support for that
kernel version that they are forcing you to stay on.

good luck!

greg k-h

Re: [PATCH for v3.18 00/18] Backport CVE-2017-13166 fixes to Kernel 3.18

[Greg KH]

On Thu, Mar 29, 2018 at 08:22:08AM +0900, Inki Dae wrote:
> Really thanks for doing this. :) There would be many users who use
> Linux-3.18 for their products yet.

For new products?  They really should not be.  The kernel is officially
end-of-life, but I'm keeping it alive for a short while longer just
because too many people seem to still be using it.  However, they are
not actually updating the kernel in their devices, so I don't think I
will be doing many more new 3.18.y releases.

It's a problem when people ask for support, and then don't use the
releases given to them :(

What is keeping you on 3.18.y and not allowing you to move to a newer
kernel version?

thanks,

greg k-h

Re: [PATCH v3 1/2] staging: media: davinci_vpfe: add error handling on kmalloc failure

[Greg KH]

On Tue, Mar 27, 2018 at 08:20:59AM +0300, Dan Carpenter wrote:
> On Tue, Mar 27, 2018 at 02:00:45PM +0900, Ji-Hun Kim wrote:
> > 
> > Are there any opinions? I'd like to know how this patch is going.
> > 
> 
> 
> Looks good.  Thanks!
> 
> Greg just hasn't gotten to it yet.

Greg does not take drivers/staging/media/* patches :)

Re: [PATCH 0/5] SPDX license identifiers in all DD drivers

[Greg KH]

On Wed, Mar 21, 2018 at 06:29:48PM +0100, Daniel Scheller wrote:
> Hi Greg,
> 
> Am Wed, 21 Mar 2018 10:49:32 +0100
> schrieb Greg KH <gre...@linuxfoundation.org>:
> 
> > On Tue, Mar 20, 2018 at 10:01:27PM +0100, Daniel Scheller wrote:
> > > From: Daniel Scheller <d.schel...@gmx.net>
> > > 
> > > This series adds SPDX license identifiers to all source files which are
> > > copyright by either Digital Devices GmbH or Metzlerbros GbR, who are
> > > the original authors of the ddbridge, ngene, cxd2099, mxl5xx, stv0910
> > > and stv6111 bridge/demod/tuner drivers, with the mxl5xx driver being
> > > based on source code released by MaxLinear.
> > > [...]
> > > The original intention was to fully replace all the licensing headers
> > > with only the SPDX License Identifiers as it is done in a lot of other
> > > in-tree drivers nowadays. However, Digital Devices disagreed to do this
> > > and expressed major concerns regarding this, in that a machine readable
> > > license tag instead of a full license boilerplate won't hold up equally,
> > > so we agreed to keep the license boilerplate text as is right now.  
> > 
> > That's really odd, who at that company can I talk to about this?  Or
> > really, what lawyer at that company can I point my lawyer at to talk
> > about this, that's the only way this is going to get resolved.
> 
> I'm not entirely sure, but I guess for a first start it's best to
> contact Ralph (from Metzlerbros) and Manfred (from Digital
> Devices), being the authors and copyright owners of the DDDVB driver
> package where the drivers originate from and thus is the upstream for
> the mainlined copies of the mentioned drivers. Both are in the Cc list
> (rjkm and mvoelkel) of this thread.
> 
> > If it helps, _ALL_ of the major companies that are kernel developers are
> > onboard with the removal of the crazy boiler-plate text, so this tiny
> > holdout should be easy to resolve.
> > 
> > > Greg, I'm Cc'ing you on this due to the last paragraph, as AFAIK you're
> > > one of the initiators of the SPDX tagging initiative, and you even added
> > > tags to 10k+ files all over the tree :-) so we maybe can discuss this
> > > further, also with DD, in the hopes you're fine with this - sorry in
> > > advance if not.  
> > 
> > See my review of your first patch here, this needs to be done a lot
> > differently...
> 
> Check. Thanks for reviewing. The intent was to do a full cleanup of all
> licensing things in one go, per driver. Will do one patch for SPDX and
> eventual boilerplate cleanup for all drivers, one for MODULE_LICENSE
> and one for missing headers in the next iteration. Though I'd wait
> with that for now if you like to contact Ralph and Manfred, and do a v2
> based on the outcome.

You can always just do the "add a SPDX line" patches now, that touch
nothing else.  No one can get upset at that.

thanks,

greg k-h

Re: [PATCH 0/5] SPDX license identifiers in all DD drivers

[Greg KH]

On Tue, Mar 20, 2018 at 10:01:27PM +0100, Daniel Scheller wrote:
> From: Daniel Scheller 
> 
> This series adds SPDX license identifiers to all source files which are
> copyright by either Digital Devices GmbH or Metzlerbros GbR, who are
> the original authors of the ddbridge, ngene, cxd2099, mxl5xx, stv0910
> and stv6111 bridge/demod/tuner drivers, with the mxl5xx driver being
> based on source code released by MaxLinear.
> 
> All source code either carries the license text "redistribute and/or
> modify it under the terms of the GNU GPL version 2 only as published
> by the FSF", or simply "... GPL version 2" in the case of the mxl5xx
> driver, which all should equal to the SPDX License Identifier
> "GPL-2.0-only" as of SPDX License List Version 3.0 published on
> December the 28th, 2017, which is applied as license tag to all files
> of the mentioned drivers by this series.
> 
> During checking of those modules I noticed that the module info carries
> the "GPL" version tag, which, according to include/linux/module.h, equals
> to "GPL version 2 or later", which (I believe) in turn is a mismatch to
> what is written in the file header's license boilerplates. This series
> corrects this by setting all MODULE_LICENSE() descriptors to "GPL v2",
> which equals to the "GNU GPL version 2 only" phrase.
> 
> Besides that, this fixes some whitespace cosmetics in the headers, and
> removes the link to gnu.org (if existing), which points to the GPLv3
> license anyway.
> 
> The original intention was to fully replace all the licensing headers
> with only the SPDX License Identifiers as it is done in a lot of other
> in-tree drivers nowadays. However, Digital Devices disagreed to do this
> and expressed major concerns regarding this, in that a machine readable
> license tag instead of a full license boilerplate won't hold up equally,
> so we agreed to keep the license boilerplate text as is right now.

That's really odd, who at that company can I talk to about this?  Or
really, what lawyer at that company can I point my lawyer at to talk
about this, that's the only way this is going to get resolved.

If it helps, _ALL_ of the major companies that are kernel developers are
onboard with the removal of the crazy boiler-plate text, so this tiny
holdout should be easy to resolve.

> Greg, I'm Cc'ing you on this due to the last paragraph, as AFAIK you're
> one of the initiators of the SPDX tagging initiative, and you even added
> tags to 10k+ files all over the tree :-) so we maybe can discuss this
> further, also with DD, in the hopes you're fine with this - sorry in
> advance if not.

See my review of your first patch here, this needs to be done a lot
differently...

thanks,

greg k-h

Re: [PATCH 1/5] [media] stv0910/stv6111: add SPDX license headers

[Greg KH]

On Tue, Mar 20, 2018 at 10:01:28PM +0100, Daniel Scheller wrote:
> From: Daniel Scheller 
> 
> Add SPDX license headers to the stv0910 and stv6111 DVB frontend
> drivers. Both drivers are licensed as GPL-2.0-only, so fix this in the
> MODULE_LICENSE while at it. Also, the includes were lacking any license
> headers at all, so add them now.
> 
> Signed-off-by: Daniel Scheller 
> ---
>  drivers/media/dvb-frontends/stv0910.c | 5 +++--
>  drivers/media/dvb-frontends/stv0910.h | 9 +
>  drivers/media/dvb-frontends/stv6111.c | 6 +++---
>  drivers/media/dvb-frontends/stv6111.h | 7 +++
>  4 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/dvb-frontends/stv0910.c 
> b/drivers/media/dvb-frontends/stv0910.c
> index 52355c14fd64..ce82264e99ef 100644
> --- a/drivers/media/dvb-frontends/stv0910.c
> +++ b/drivers/media/dvb-frontends/stv0910.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0-only

Please only use the identifiers documented in
Documentation/process/license-rules.rst right now.  We wrote and got
that merged _before_ SPDX bumped the tags to 3.0 which added the (imo
crazy) -only variants.

So please stick with what is already in the kernel tree, if we do decide
to update to a newer version of SPDX, we will hit the tree all at once
with a script to give to Linus to run.


>  /*
>   * Driver for the ST STV0910 DVB-S/S2 demodulator.
>   *
> @@ -11,7 +12,7 @@
>   *
>   * This program is distributed in the hope that it will be useful,
>   * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

Why did you change this in this patch?

Please only do "one" thing per patch.



>   * GNU General Public License for more details.
>   */
>  
> @@ -1836,4 +1837,4 @@ EXPORT_SYMBOL_GPL(stv0910_attach);
>  
>  MODULE_DESCRIPTION("ST STV0910 multistandard frontend driver");
>  MODULE_AUTHOR("Ralph and Marcus Metzler, Manfred Voelkel");
> -MODULE_LICENSE("GPL");
> +MODULE_LICENSE("GPL v2");

Again, this should be a separate patch.


> diff --git a/drivers/media/dvb-frontends/stv0910.h 
> b/drivers/media/dvb-frontends/stv0910.h
> index fccd8d9b665f..93de08540ce4 100644
> --- a/drivers/media/dvb-frontends/stv0910.h
> +++ b/drivers/media/dvb-frontends/stv0910.h
> @@ -1,3 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Driver for the ST STV0910 DVB-S/S2 demodulator.
> + *
> + * Copyright (C) 2014-2015 Ralph Metzler 
> + * Marcus Metzler 
> + * developed for Digital Devices GmbH
> + */

Where did that copyright notice come from?

This patch is a total mix of different things, please do not do that at
all!

thanks,

greg k-h

Re: [PATCH v2] Add udmabuf misc device

[Greg KH]

On Fri, Mar 16, 2018 at 08:46:49AM +0100, Gerd Hoffmann wrote:
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/dma-buf/udmabuf.c
> @@ -0,0 +1,69 @@
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

No license text at all?  Come on, I already made one commit in the tree
that touched 11k files, I don't want to have to do it again :)

thanks,

greg k-h

Re: [PATCH v2] Add udmabuf misc device

[Greg KH]

On Fri, Mar 16, 2018 at 08:46:49AM +0100, Gerd Hoffmann wrote:
> --- /dev/null
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -0,0 +1,261 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */

No SPDX line or copyright here?  You put it in the .h file :(

thanks,

greg k-h

Re: [PATCH] [media] cpia2_usb: drop bogus interface-release call

[Greg KH]

On Wed, Mar 07, 2018 at 10:49:36AM +0100, Johan Hovold wrote:
> Drop bogus call to usb_driver_release_interface() from the disconnect()
> callback. As the interface is already being unbound at this point,
> usb_driver_release_interface() simply returns early.
> 
> Signed-off-by: Johan Hovold 

Reviewed-by: Greg Kroah-Hartman 

Re: [PATCH v2] Staging: bcm2048: Fix function argument alignment in radio-bcm2048.c.

[Greg KH]

On Tue, Feb 27, 2018 at 08:32:30AM +0100, Hans Verkuil wrote:
> On 02/27/2018 02:53 AM, Quytelda Kahja wrote:
> > Hans,
> > 
> > Thank you very much for your input on the patch; however this patch
> > has already been applied to the staging tree.  Additionally:
> 
> I have no record of this being applied through linux-media. Did someone
> else pick this up? Greg perhaps?

Did I pick this up?  Oops, sorry, didn't mean to, I'll go revert it now,
sorry...

greg k-h

Re: [PATCH v2 1/3] staging: xm2mvscale: Driver support for Xilinx M2M Video Scaler

[Greg KH]

On Wed, Feb 21, 2018 at 02:43:14PM -0800, Rohit Athavale wrote:
> This commit adds driver support for the pre-release Xilinx M2M Video
> Scaler IP. There are three parts to this driver :
> 
>  - The Hardware/IP layer that reads and writes register of the IP
>contained in the scaler_hw_xm2m.c
>  - The set of ioctls that applications would need to know contained
>in ioctl_xm2mvsc.h
>  - The char driver that consumes the IP layer in xm2m_vscale.c
> 
> Signed-off-by: Rohit Athavale 
> ---

I need an ack from the linux-media maintainers before I can consider
this for staging, as this really looks like an "odd" video driver...

thanks,

greg k-h

Re: [PATCH for v4.4 00/14] v4l2-compat-ioctl32.c: remove set_fs(KERNEL_DS)

[Greg KH]

On Wed, Feb 14, 2018 at 12:52:26PM +0100, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> This patch series fixes a number of bugs and culminates in the removal
> of the set_fs(KERNEL_DS) call in v4l2-compat-ioctl32.c.
> 
> This was tested with a VM running 4.4, the vivid driver (since that
> emulates almost all V4L2 ioctls that need to pass through 
> v4l2-compat-ioctl32.c)
> and a 32-bit v4l2-compliance utility since that exercises almost all ioctls
> as well. Combined this gives good test coverage.
> 
> Most of the v4l2-compat-ioctl32.c do cleanups and fix subtle issues that
> v4l2-compliance complained about. The purpose is to 1) make it easy to
> verify that the final patch didn't introduce errors by first eliminating
> errors caused by other known bugs, and 2) keep the final patch at least
> somewhat readable.

All now applied, many thanks for these.

greg k-h

Re: [PATCH for v4.9 00/13] v4l2-compat-ioctl32.c: remove set_fs(KERNEL_DS)

[Greg KH]

On Wed, Feb 14, 2018 at 12:48:17PM +0100, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> This patch series fixes a number of bugs and culminates in the removal
> of the set_fs(KERNEL_DS) call in v4l2-compat-ioctl32.c.
> 
> This was tested with a VM running 4.9, the vivid driver (since that
> emulates almost all V4L2 ioctls that need to pass through 
> v4l2-compat-ioctl32.c)
> and a 32-bit v4l2-compliance utility since that exercises almost all ioctls
> as well. Combined this gives good test coverage.
> 
> Most of the v4l2-compat-ioctl32.c do cleanups and fix subtle issues that
> v4l2-compliance complained about. The purpose is to 1) make it easy to
> verify that the final patch didn't introduce errors by first eliminating
> errors caused by other bugs, and 2) keep the final patch at least somewhat
> readable.

All now applied, thanks.

greg k-h

Re: [PATCH for v4.14 00/13] v4l2-compat-ioctl32.c: remove set_fs(KERNEL_DS)

[Greg KH]

On Wed, Feb 14, 2018 at 12:44:21PM +0100, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> This patch series fixes a number of bugs and culminates in the removal
> of the set_fs(KERNEL_DS) call in v4l2-compat-ioctl32.c.
> 
> This was tested with a VM running 4.14, the vivid driver (since that
> emulates almost all V4L2 ioctls that need to pass through 
> v4l2-compat-ioctl32.c)
> and a 32-bit v4l2-compliance utility since that exercises almost all ioctls
> as well. Combined this gives good test coverage.
> 
> Most of the v4l2-compat-ioctl32.c do cleanups and fix subtle issues that
> v4l2-compliance complained about. The purpose is to 1) make it easy to
> verify that the final patch didn't introduce errors by first eliminating
> errors caused by other bugs, and 2) keep the final patch at least somewhat
> readable.

All now queued up, thanks.

greg k-h

Re: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Greg KH]

On Tue, Jan 09, 2018 at 04:26:28PM +0200, Laurent Pinchart wrote:
> Hi Greg,
> 
> On Tuesday, 9 January 2018 12:04:10 EET Greg KH wrote:
> > On Tue, Jan 09, 2018 at 10:40:21AM +0200, Laurent Pinchart wrote:
> > > On Saturday, 6 January 2018 11:40:26 EET Greg KH wrote:
> > >> On Sat, Jan 06, 2018 at 10:09:07AM +0100, Greg KH wrote:
> > >> 
> > >> While I'm all for fixing this type of thing, I feel like we need to do
> > >> something "else" for this as playing whack-a-mole for this pattern is
> > >> going to be a never-ending battle for all drivers for forever.
> > > 
> > > That's my concern too, as even if we managed to find and fix all the
> > > occurrences of the problematic patterns (and we won't), new ones will keep
> > > being merged all the time.
> > 
> > And what about the millions of lines of out-of-tree drivers that we all
> > rely on every day in our devices?  What about the distro kernels that
> > add random new drivers?
> 
> Of course, even though the out-of-tree drivers probably come with lots of 
> security issues worse than this one.

Sure, but I have worked with some teams that have used coverity to find
and fix all of the reported bugs it founds.  So some companies are
trying to fix their problems here, let's not make it impossible for them :)

> > We need some sort of automated way to scan for this.
> 
> Is there any initiative to implement such a scan in an open-source tool ?

Sure, if you want to, but I have no such initiative...

> We also need to educate developers. An automatic scanner could help there, 
> but 
> in the end the information has to spread to all our brains. It won't be easy, 
> and is likely not fully feasible, but it's no different than how developers 
> have to be educated about race conditions and locking for instance. It's a 
> mind set.

Agreed.

> > Intel, any chance we can get your coverity rules?  Given that the date
> > of this original patchset was from last August, has anyone looked at
> > what is now in Linus's tree?  What about linux-next?  I just added 3
> > brand-new driver subsystems to the kernel tree there, how do we know
> > there isn't problems in them?
> > 
> > And what about all of the other ways user-data can be affected?  Again,
> > as Peter pointed out, USB devices.  I want some chance to be able to at
> > least audit the codebase we have to see if that path is an issue.
> > Without any hint of how to do this in an automated manner, we are all
> > in deep shit for forever.
> 
> Or at least until the hardware architecture evolves. Let's drop the x86 
> instruction set, expose the µops, and have gcc handle the scheduling. Sure, 
> it 
> will mean recompiling everything for every x86 CPU model out there, but we 
> have source-based distros to the rescue :-D

Then we are back at the itanium mess, where all of the hardware issues
were supposed be fixed by the compiler writers.  We all remember how
well that worked out...

> > >> Either we need some way to mark this data path to make it easy for tools
> > >> like sparse to flag easily, or we need to catch the issue in the driver
> > >> subsystems, which unfortunatly, would harm the drivers that don't have
> > >> this type of issue (like here.)
> > > 
> > > But how would you do so ?
> > 
> > I do not know, it all depends on the access pattern, right?
> 
> Any data coming from userspace could trigger such accesses. If we want 
> complete coverage the only way I can think of is starting from syscalls and 
> tainting data down the call stacks (__user could help to some extend), but 
> we'll likely be drowned in false positives. I don't see how we could mark 
> paths manually.

I agree, which is why I want to see how someone did this work
originally.  We have no idea as no one is telling us anything :(

How do we "know" that these are the only problem areas?  When was the
last scan run?  On what tree?  And so on...

thanks,

greg k-h

Re: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Greg KH]

On Tue, Jan 09, 2018 at 10:40:21AM +0200, Laurent Pinchart wrote:
> On Saturday, 6 January 2018 11:40:26 EET Greg KH wrote:
> > On Sat, Jan 06, 2018 at 10:09:07AM +0100, Greg KH wrote:
> > 
> > While I'm all for fixing this type of thing, I feel like we need to do
> > something "else" for this as playing whack-a-mole for this pattern is
> > going to be a never-ending battle for all drivers for forever.
> 
> That's my concern too, as even if we managed to find and fix all the 
> occurrences of the problematic patterns (and we won't), new ones will keep 
> being merged all the time.

And what about the millions of lines of out-of-tree drivers that we all
rely on every day in our devices?  What about the distro kernels that
add random new drivers?

We need some sort of automated way to scan for this.

Intel, any chance we can get your coverity rules?  Given that the date
of this original patchset was from last August, has anyone looked at
what is now in Linus's tree?  What about linux-next?  I just added 3
brand-new driver subsystems to the kernel tree there, how do we know
there isn't problems in them?

And what about all of the other ways user-data can be affected?  Again,
as Peter pointed out, USB devices.  I want some chance to be able to at
least audit the codebase we have to see if that path is an issue.
Without any hint of how to do this in an automated manner, we are all
in deep shit for forever.

> > Either we need some way to mark this data path to make it easy for tools
> > like sparse to flag easily, or we need to catch the issue in the driver
> > subsystems, which unfortunatly, would harm the drivers that don't have
> > this type of issue (like here.)
> 
> But how would you do so ?

I do not know, it all depends on the access pattern, right?

> > I'm guessing that other operating systems, which don't have the luxury
> > of auditing all of their drivers are going for the "big hammer in the
> > subsystem" type of fix, right?
> 
> Other operating systems that ship closed-source drivers authored by hardware 
> vendors and not reviewed by third parties will likely stay vulnerable 
> forever. 
> That's a small concern though as I expect those drivers to contain much large 
> security holes anyway.

Well yes, but odds are those operating systems are doing something to
mitigate this, right?  Slowing down all user/kernel data paths?
Targeted code analysis tools?  Something else?  I doubt they just don't
care at all about it.  At the least, I would think Coverity would be
trying to sell licenses for this :(

thanks,

greg k-h

Re: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Greg KH]

On Sat, Jan 06, 2018 at 09:41:17AM -0800, Dan Williams wrote:
> On Sat, Jan 6, 2018 at 1:40 AM, Greg KH <gre...@linuxfoundation.org> wrote:
> > On Sat, Jan 06, 2018 at 10:09:07AM +0100, Greg KH wrote:
> >> On Fri, Jan 05, 2018 at 05:10:32PM -0800, Dan Williams wrote:
> >> > Static analysis reports that 'index' may be a user controlled value that
> >> > is used as a data dependency to read 'pin' from the
> >> > 'selector->baSourceID' array. In order to avoid potential leaks of
> >> > kernel memory values, block speculative execution of the instruction
> >> > stream that could issue reads based on an invalid value of 'pin'.
> >> >
> >> > Based on an original patch by Elena Reshetova.
> >> >
> >> > Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> >> > Cc: Mauro Carvalho Chehab <mche...@kernel.org>
> >> > Cc: linux-media@vger.kernel.org
> >> > Signed-off-by: Elena Reshetova <elena.reshet...@intel.com>
> >> > Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
> >> > ---
> >> >  drivers/media/usb/uvc/uvc_v4l2.c |7 +--
> >> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c 
> >> > b/drivers/media/usb/uvc/uvc_v4l2.c
> >> > index 3e7e283a44a8..7442626dc20e 100644
> >> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> >> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >> > @@ -22,6 +22,7 @@
> >> >  #include 
> >> >  #include 
> >> >  #include 
> >> > +#include 
> >> >
> >> >  #include 
> >> >  #include 
> >> > @@ -810,6 +811,7 @@ static int uvc_ioctl_enum_input(struct file *file, 
> >> > void *fh,
> >> > struct uvc_entity *iterm = NULL;
> >> > u32 index = input->index;
> >> > int pin = 0;
> >> > +   __u8 *elem;
> >> >
> >> > if (selector == NULL ||
> >> > (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
> >> > @@ -820,8 +822,9 @@ static int uvc_ioctl_enum_input(struct file *file, 
> >> > void *fh,
> >> > break;
> >> > }
> >> > pin = iterm->id;
> >> > -   } else if (index < selector->bNrInPins) {
> >> > -   pin = selector->baSourceID[index];
> >> > +   } else if ((elem = nospec_array_ptr(selector->baSourceID, index,
> >> > +   selector->bNrInPins))) {
> >> > +   pin = *elem;
> >>
> >> I dug through this before, and I couldn't find where index came from
> >> userspace, I think seeing the coverity rule would be nice.
> >
> > Ok, I take it back, this looks correct.  Ugh, the v4l ioctl api is
> > crazy complex (rightfully so), it's amazing that coverity could navigate
> > that whole thing :)
> >
> > While I'm all for fixing this type of thing, I feel like we need to do
> > something "else" for this as playing whack-a-mole for this pattern is
> > going to be a never-ending battle for all drivers for forever.  Either
> > we need some way to mark this data path to make it easy for tools like
> > sparse to flag easily, or we need to catch the issue in the driver
> > subsystems, which unfortunatly, would harm the drivers that don't have
> > this type of issue (like here.)
> >
> > I'm guessing that other operating systems, which don't have the luxury
> > of auditing all of their drivers are going for the "big hammer in the
> > subsystem" type of fix, right?
> >
> > I don't have a good answer for this, but if there was some better way to
> > rewrite these types of patterns to just prevent the need for the
> > nospec_array_ptr() type thing, that might be the best overall for
> > everyone.  Much like ebpf did with their changes.  That way a simple
> > coccinelle rule would be able to catch the pattern and rewrite it.
> >
> > Or am I just dreaming?
> 
> At least on the coccinelle front you're dreaming. Julia already took a
> look and said:
> 
> "I don't think Coccinelle would be good for doing this (ie
> implementing taint analysis) because the dataflow is too complicated."

Sorry for the confusion, no, I don't mean the "taint tracking", I mean
the generic pattern of "speculative out of bounds access" that we are
fixing here.

Yes, a

Re: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Greg KH]

On Sat, Jan 06, 2018 at 10:09:07AM +0100, Greg KH wrote:
> On Fri, Jan 05, 2018 at 05:10:32PM -0800, Dan Williams wrote:
> > Static analysis reports that 'index' may be a user controlled value that
> > is used as a data dependency to read 'pin' from the
> > 'selector->baSourceID' array. In order to avoid potential leaks of
> > kernel memory values, block speculative execution of the instruction
> > stream that could issue reads based on an invalid value of 'pin'.
> > 
> > Based on an original patch by Elena Reshetova.
> > 
> > Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> > Cc: Mauro Carvalho Chehab <mche...@kernel.org>
> > Cc: linux-media@vger.kernel.org
> > Signed-off-by: Elena Reshetova <elena.reshet...@intel.com>
> > Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
> > ---
> >  drivers/media/usb/uvc/uvc_v4l2.c |7 +--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c 
> > b/drivers/media/usb/uvc/uvc_v4l2.c
> > index 3e7e283a44a8..7442626dc20e 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -22,6 +22,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include 
> >  #include 
> > @@ -810,6 +811,7 @@ static int uvc_ioctl_enum_input(struct file *file, void 
> > *fh,
> > struct uvc_entity *iterm = NULL;
> > u32 index = input->index;
> > int pin = 0;
> > +   __u8 *elem;
> >  
> > if (selector == NULL ||
> > (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
> > @@ -820,8 +822,9 @@ static int uvc_ioctl_enum_input(struct file *file, void 
> > *fh,
> > break;
> > }
> > pin = iterm->id;
> > -   } else if (index < selector->bNrInPins) {
> > -   pin = selector->baSourceID[index];
> > +   } else if ((elem = nospec_array_ptr(selector->baSourceID, index,
> > +   selector->bNrInPins))) {
> > +   pin = *elem;
> 
> I dug through this before, and I couldn't find where index came from
> userspace, I think seeing the coverity rule would be nice.

Ok, I take it back, this looks correct.  Ugh, the v4l ioctl api is
crazy complex (rightfully so), it's amazing that coverity could navigate
that whole thing :)

While I'm all for fixing this type of thing, I feel like we need to do
something "else" for this as playing whack-a-mole for this pattern is
going to be a never-ending battle for all drivers for forever.  Either
we need some way to mark this data path to make it easy for tools like
sparse to flag easily, or we need to catch the issue in the driver
subsystems, which unfortunatly, would harm the drivers that don't have
this type of issue (like here.)

I'm guessing that other operating systems, which don't have the luxury
of auditing all of their drivers are going for the "big hammer in the
subsystem" type of fix, right?

I don't have a good answer for this, but if there was some better way to
rewrite these types of patterns to just prevent the need for the
nospec_array_ptr() type thing, that might be the best overall for
everyone.  Much like ebpf did with their changes.  That way a simple
coccinelle rule would be able to catch the pattern and rewrite it.

Or am I just dreaming?

thanks,

greg k-h

Re: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution

[Greg KH]

On Fri, Jan 05, 2018 at 05:10:32PM -0800, Dan Williams wrote:
> Static analysis reports that 'index' may be a user controlled value that
> is used as a data dependency to read 'pin' from the
> 'selector->baSourceID' array. In order to avoid potential leaks of
> kernel memory values, block speculative execution of the instruction
> stream that could issue reads based on an invalid value of 'pin'.
> 
> Based on an original patch by Elena Reshetova.
> 
> Cc: Laurent Pinchart 
> Cc: Mauro Carvalho Chehab 
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Elena Reshetova 
> Signed-off-by: Dan Williams 
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c |7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c 
> b/drivers/media/usb/uvc/uvc_v4l2.c
> index 3e7e283a44a8..7442626dc20e 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -810,6 +811,7 @@ static int uvc_ioctl_enum_input(struct file *file, void 
> *fh,
>   struct uvc_entity *iterm = NULL;
>   u32 index = input->index;
>   int pin = 0;
> + __u8 *elem;
>  
>   if (selector == NULL ||
>   (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
> @@ -820,8 +822,9 @@ static int uvc_ioctl_enum_input(struct file *file, void 
> *fh,
>   break;
>   }
>   pin = iterm->id;
> - } else if (index < selector->bNrInPins) {
> - pin = selector->baSourceID[index];
> + } else if ((elem = nospec_array_ptr(selector->baSourceID, index,
> + selector->bNrInPins))) {
> + pin = *elem;

I dug through this before, and I couldn't find where index came from
userspace, I think seeing the coverity rule would be nice.

And if this value really is user controlled, then why is this the only
v4l driver that is affected?  This is a common callback.

thanks,

greg k-h

Re: [PATCH v3 00/27] kill devm_ioremap_nocache

[Greg KH]

On Sat, Dec 23, 2017 at 06:55:25PM +0800, Yisheng Xie wrote:
> Hi all,
> 
> When I tried to use devm_ioremap function and review related code, I found
> devm_ioremap and devm_ioremap_nocache is almost the same with each other,
> except one use ioremap while the other use ioremap_nocache.

For all arches?  Really?  Look at MIPS, and x86, they have different
functions.

> While ioremap's
> default function is ioremap_nocache, so devm_ioremap_nocache also have the
> same function with devm_ioremap, which can just be killed to reduce the size
> of devres.o(from 20304 bytes to 18992 bytes in my compile environment).
> 
> I have posted two versions, which use macro instead of function for
> devm_ioremap_nocache[1] or devm_ioremap[2]. And Greg suggest me to kill
> devm_ioremap_nocache for no need to keep a macro around for the duplicate
> thing. So here comes v3 and please help to review.

I don't think this can be done, what am I missing?  These functions are
not identical, sorry for missing that before.

thanks,

greg k-h

Re: [PATCH v3 27/27] devres: kill devm_ioremap_nocache

[Greg KH]

On Sat, Dec 23, 2017 at 07:02:59PM +0800, Yisheng Xie wrote:
> --- a/lib/devres.c
> +++ b/lib/devres.c
> @@ -44,35 +44,6 @@ void __iomem *devm_ioremap(struct device *dev, 
> resource_size_t offset,
>  EXPORT_SYMBOL(devm_ioremap);
>  
>  /**
> - * devm_ioremap_nocache - Managed ioremap_nocache()
> - * @dev: Generic device to remap IO address for
> - * @offset: Resource address to map
> - * @size: Size of map
> - *
> - * Managed ioremap_nocache().  Map is automatically unmapped on driver
> - * detach.
> - */
> -void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t 
> offset,
> -resource_size_t size)
> -{
> - void __iomem **ptr, *addr;
> -
> - ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
> - if (!ptr)
> - return NULL;
> -
> - addr = ioremap_nocache(offset, size);

Wait, devm_ioremap() calls ioremap(), not ioremap_nocache(), are you
_SURE_ that these are all identical?  For all arches?  If so, then
ioremap_nocache() can also be removed, right?

In my quick glance, I don't think you can do this series at all :(

greg k-h

Re: [PATCH] media: v4l: xilinx: Use SPDX-License-Identifier

[Greg KH]

On Thu, Dec 14, 2017 at 10:44:16PM +0200, Laurent Pinchart wrote:
> Hi Greg,
> 
> On Thursday, 14 December 2017 22:08:51 EET Greg KH wrote:
> > On Thu, Dec 14, 2017 at 09:05:27PM +0200, Laurent Pinchart wrote:
> > > On Thursday, 14 December 2017 20:54:39 EET Joe Perches wrote:
> > >> On Thu, 2017-12-14 at 20:37 +0200, Laurent Pinchart wrote:
> > >>> On Thursday, 14 December 2017 20:32:20 EET Joe Perches wrote:
> > >>>> On Thu, 2017-12-14 at 20:28 +0200, Laurent Pinchart wrote:
> > >>>>> On Thursday, 14 December 2017 19:05:27 EET Mauro Carvalho Chehab 
> wrote:
> > >>>>>> Em Fri,  8 Dec 2017 18:05:37 +0530 Dhaval Shah escreveu:
> > >>>>>>> SPDX-License-Identifier is used for the Xilinx Video IP and
> > >>>>>>> related drivers.
> > >>>>>>> 
> > >>>>>>> Signed-off-by: Dhaval Shah <dhaval23031...@gmail.com>
> > >>>>>> 
> > >>>>>> Hi Dhaval,
> > >>>>>> 
> > >>>>>> You're not listed as one of the Xilinx driver maintainers. I'm
> > >>>>>> afraid that, without their explicit acks, sent to the ML, I can't
> > >>>>>> accept a patch touching at the driver's license tags.
> > >>>>> 
> > >>>>> The patch doesn't change the license, I don't see why it would cause
> > >>>>> any issue. Greg isn't listed as the maintainer or copyright holder
> > >>>>> of any of the 10k+ files to which he added an SPDX license header in
> > >>>>> the last kernel release.
> > >>>> 
> > >>>> Adding a comment line that describes an implicit or
> > >>>> explicit license is different than removing the license
> > >>>> text itself.
> > >>> 
> > >>> The SPDX license header is meant to be equivalent to the license text.
> > >> 
> > >> I understand that.
> > >> At a minimum, removing BSD license text is undesirable
> > >> 
> > >> as that license states:
> > >>  ** Redistributions of source code must retain the above copyright
> > >>  *  notice, this list of conditions and the following disclaimer.
> > >> 
> > >> etc...
> > > 
> > > But this patch only removes the following text:
> > > 
> > > - * This program is free software; you can redistribute it and/or modify
> > > - * it under the terms of the GNU General Public License version 2 as
> > > - * published by the Free Software Foundation.
> > > 
> > > and replaces it by the corresponding SPDX header.
> > > 
> > >>> The only reason why the large SPDX patch didn't touch the whole kernel
> > >>> in one go was that it was easier to split in in multiple chunks.
> > >> 
> > >> Not really, it was scripted.
> > > 
> > > But still manually reviewed as far as I know.
> > > 
> > >>> This is no different than not including the full GPL license in every
> > >>> header file but only pointing to it through its name and reference, as
> > >>> every kernel source file does.
> > >> 
> > >> Not every kernel source file had a license text
> > >> or a reference to another license file.
> > > 
> > > Correct, but the files touched by this patch do.
> > > 
> > > This issue is in no way specific to linux-media and should be decided upon
> > > at the top level, not on a per-subsystem basis. Greg, could you comment
> > > on this ?
> > 
> > Comment on what exactly?  I don't understand the problem here, care to
> > summarize it?
> 
> In a nutshell (if I understand it correctly), Dhaval Shah submitted https://
> patchwork.kernel.org/patch/10102451/ which replaces
> 
> +// SPDX-License-Identifier: GPL-2.0
> [...]
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> 
> in all .c and .h files of the Xilinx V4L2 driver (drivers/media/platform/
> xilinx). I have reviewed the patch and acked it. Mauro then rejected it, 
> stating that he can't accept a change to license text without an explicit ack 
> from the official driver's maintainers. My position is that such a change 
> doesn't change the license and thus doesn't need to track al

Re: [PATCH] media: v4l: xilinx: Use SPDX-License-Identifier

[Greg KH]

On Thu, Dec 14, 2017 at 09:05:27PM +0200, Laurent Pinchart wrote:
> Hi Joe,
> 
> (CC'ing Greg and adding context for easier understanding)
> 
> On Thursday, 14 December 2017 20:54:39 EET Joe Perches wrote:
> > On Thu, 2017-12-14 at 20:37 +0200, Laurent Pinchart wrote:
> > > On Thursday, 14 December 2017 20:32:20 EET Joe Perches wrote:
> > >> On Thu, 2017-12-14 at 20:28 +0200, Laurent Pinchart wrote:
> > >>> On Thursday, 14 December 2017 19:05:27 EET Mauro Carvalho Chehab wrote:
> >  Em Fri,  8 Dec 2017 18:05:37 +0530 Dhaval Shah escreveu:
> > > SPDX-License-Identifier is used for the Xilinx Video IP and
> > > related drivers.
> > > 
> > > Signed-off-by: Dhaval Shah 
> >  
> >  Hi Dhaval,
> >  
> >  You're not listed as one of the Xilinx driver maintainers. I'm afraid
> >  that, without their explicit acks, sent to the ML, I can't accept a
> >  patch touching at the driver's license tags.
> > >>> 
> > >>> The patch doesn't change the license, I don't see why it would cause
> > >>> any issue. Greg isn't listed as the maintainer or copyright holder of
> > >>> any of the 10k+ files to which he added an SPDX license header in the
> > >>> last kernel release.
> > >> Adding a comment line that describes an implicit or
> > >> explicit license is different than removing the license
> > >> text itself.
> > > 
> > > The SPDX license header is meant to be equivalent to the license text.
> > 
> > I understand that.
> > At a minimum, removing BSD license text is undesirable
> > as that license states:
> > 
> >  ** Redistributions of source code must retain the above copyright
> >  *  notice, this list of conditions and the following disclaimer.
> > etc...
> 
> But this patch only removes the following text:
> 
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> 
> and replaces it by the corresponding SPDX header.
> 
> > > The only reason why the large SPDX patch didn't touch the whole kernel in
> > > one go was that it was easier to split in in multiple chunks.
> > 
> > Not really, it was scripted.
> 
> But still manually reviewed as far as I know.
> 
> > > This is no different than not including the full GPL license in every
> > > header file but only pointing to it through its name and reference, as
> > > every kernel source file does.
> > 
> > Not every kernel source file had a license text
> > or a reference to another license file.
> 
> Correct, but the files touched by this patch do.
> 
> This issue is in no way specific to linux-media and should be decided upon at 
> the top level, not on a per-subsystem basis. Greg, could you comment on this ?

Comment on what exactly?  I don't understand the problem here, care to
summarize it?

thanks,

greg k-h

Re: [PATCH 0/4] Backported amdgpu ttm deadlock fixes for 4.14

[Greg KH]

On Thu, Nov 30, 2017 at 07:23:02PM -0500, Lyude Paul wrote:
> I haven't gone to see where it started, but as of late a good number of
> pretty nasty deadlock issues have appeared with the kernel. Easy
> reproduction recipe on a laptop with i915/amdgpu prime with lockdep enabled:
> 
> DRI_PRIME=1 glxinfo
> 
> Additionally, some more race conditions exist that I've managed to
> trigger with piglit and lockdep enabled after applying these patches:
> 
> =
> WARNING: suspicious RCU usage
> 4.14.3Lyude-Test+ #2 Not tainted
> -
> ./include/linux/reservation.h:216 suspicious rcu_dereference_protected() 
> usage!
> 
> other info that might help us debug this:
> 
> rcu_scheduler_active = 2, debug_locks = 1
> 1 lock held by ext_image_dma_b/27451:
>  #0:  (reservation_ww_class_mutex){+.+.}, at: [] 
> ttm_bo_unref+0x9f/0x3c0 [ttm]
> 
> stack backtrace:
> CPU: 0 PID: 27451 Comm: ext_image_dma_b Not tainted 4.14.3Lyude-Test+ #2
> Hardware name: HP HP ZBook 15 G4/8275, BIOS P70 Ver. 01.02 06/09/2017
> Call Trace:
>  dump_stack+0x8e/0xce
>  lockdep_rcu_suspicious+0xc5/0x100
>  reservation_object_copy_fences+0x292/0x2b0
>  ? ttm_bo_unref+0x9f/0x3c0 [ttm]
>  ttm_bo_unref+0xbd/0x3c0 [ttm]
>  amdgpu_bo_unref+0x2a/0x50 [amdgpu]
>  amdgpu_gem_object_free+0x4b/0x50 [amdgpu]
>  drm_gem_object_free+0x1f/0x40 [drm]
>  drm_gem_object_put_unlocked+0x40/0xb0 [drm]
>  drm_gem_object_handle_put_unlocked+0x6c/0xb0 [drm]
>  drm_gem_object_release_handle+0x51/0x90 [drm]
>  drm_gem_handle_delete+0x5e/0x90 [drm]
>  ? drm_gem_handle_create+0x40/0x40 [drm]
>  drm_gem_close_ioctl+0x20/0x30 [drm]
>  drm_ioctl_kernel+0x5d/0xb0 [drm]
>  drm_ioctl+0x2f7/0x3b0 [drm]
>  ? drm_gem_handle_create+0x40/0x40 [drm]
>  ? trace_hardirqs_on_caller+0xf4/0x190
>  ? trace_hardirqs_on+0xd/0x10
>  amdgpu_drm_ioctl+0x4f/0x90 [amdgpu]
>  do_vfs_ioctl+0x93/0x670
>  ? __fget+0x108/0x1f0
>  SyS_ioctl+0x79/0x90
>  entry_SYSCALL_64_fastpath+0x23/0xc2
> 
> I've also added the relevant fixes for the issue mentioned above.
> 
> Christian König (3):
>   drm/ttm: fix ttm_bo_cleanup_refs_or_queue once more
>   dma-buf: make reservation_object_copy_fences rcu save
>   drm/amdgpu: reserve root PD while releasing it
> 
> Michel Dänzer (1):
>   drm/ttm: Always and only destroy bo->ttm_resv in ttm_bo_release_list

All now queued up, thanks.

greg k-h

Re: [PATCH 2/3] media: staging: atomisp: defined as static some const arrays which don't need external linkage.

[Greg KH]

On Wed, Nov 29, 2017 at 11:08:17AM +0200, Sakari Ailus wrote:
> Hi Greg,
> 
> On Mon, Nov 27, 2017 at 01:21:25PM +0100, Greg KH wrote:
> > On Mon, Nov 27, 2017 at 11:30:53AM +, Jeremy Sowden wrote:
> > > Signed-off-by: Jeremy Sowden <jer...@azazel.net>
> > > ---
> > >  .../isp/kernels/eed1_8/ia_css_eed1_8.host.c| 24 
> > > +++---
> > >  1 file changed, 12 insertions(+), 12 deletions(-)
> > 
> > I can never take patches without any changelog text, and so no one
> > should write them that way :)
> 
> I've been applying the atomisp patches to my tree for some time now,
> further on passing them to Mauro to be merged via the media tree. To avoid
> conflicts, I suggest to avoid merging them via the staging tree.

I'm not touching any of these patches, just commenting that patches
without changelogs should not be written :)

thanks,

greg k-h

Re: [PATCH] staging/media: lirc: style fix - replace hard-coded function names

[Greg KH]

On Sun, Nov 26, 2017 at 08:49:42PM +0100, Martin Homuth wrote:
> This patch fixes the remaining coding style warnings in the lirc module.
> 
> It fixes the following checkpatch.pl warning:
> 
> WARNING: Prefer using '"%s...", __func__' to using 'read', this
> function's name, in a string

> >From f11f24667ba6696cb71ac33a67fc0c7d3b4cd542 Mon Sep 17 00:00:00 2001
> From: Martin Homuth 
> Date: Sun, 26 Nov 2017 20:14:33 +0100
> Subject: [PATCH] lirc: style fix - replace hard-coded function names
> 
> Instead of hard coding the function name the __func__ variable
> should be used.
> 
> Signed-off-by: Martin Homuth 
> ---
>  drivers/staging/media/lirc/lirc_zilog.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
> b/drivers/staging/media/lirc/lirc_zilog.c
> index 6bd0717bf76e..be68ee652071 100644
> --- a/drivers/staging/media/lirc/lirc_zilog.c
> +++ b/drivers/staging/media/lirc/lirc_zilog.c
> @@ -888,9 +888,9 @@ static ssize_t read(struct file *filep, char __user 
> *outbuf, size_t n,
>   unsigned int m;
>   DECLARE_WAITQUEUE(wait, current);
>  
> - dev_dbg(ir->dev, "read called\n");
> + dev_dbg(ir->dev, "%s called\n", __func__);
>   if (n % rbuf->chunk_size) {
> - dev_dbg(ir->dev, "read result = -EINVAL\n");
> + dev_dbg(ir->dev, "%s result = -EINVAL\n", __func__);
>   return -EINVAL;
>   }
>  
> @@ -949,7 +949,7 @@ static ssize_t read(struct file *filep, char __user 
> *outbuf, size_t n,
>   retries++;
>   }
>   if (retries >= 5) {
> - dev_err(ir->dev, "Buffer read failed!\n");
> + dev_err(ir->dev, "%s failed!\n", __func__);
>   ret = -EIO;
>   }
>   }
> @@ -959,7 +959,7 @@ static ssize_t read(struct file *filep, char __user 
> *outbuf, size_t n,
>   put_ir_rx(rx, false);
>   set_current_state(TASK_RUNNING);
>  
> - dev_dbg(ir->dev, "read result = %d (%s)\n", ret,
> + dev_dbg(ir->dev, "%s result = %d (%s)\n", __func__, ret,
>   ret ? "Error" : "OK");
>  
>   return ret ? ret : written;
> -- 
> 2.13.6
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch was attached, please place it inline so that it can be
  applied directly from the email message itself.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH 2/3] media: staging: atomisp: defined as static some const arrays which don't need external linkage.

[Greg KH]

On Mon, Nov 27, 2017 at 11:30:53AM +, Jeremy Sowden wrote:
> Signed-off-by: Jeremy Sowden 
> ---
>  .../isp/kernels/eed1_8/ia_css_eed1_8.host.c| 24 
> +++---
>  1 file changed, 12 insertions(+), 12 deletions(-)

I can never take patches without any changelog text, and so no one
should write them that way :)

Try it again?

thanks,

greg k-h

Re: [PATCH 00/18] use ARRAY_SIZE macro

[Greg KH]

On Sun, Oct 01, 2017 at 08:52:20PM -0400, Jérémy Lefaure wrote:
> On Mon, 2 Oct 2017 09:01:31 +1100
> "Tobin C. Harding"  wrote:
> 
> > > In order to reduce the size of the To: and Cc: lines, each patch of the
> > > series is sent only to the maintainers and lists concerned by the patch.
> > > This cover letter is sent to every list concerned by this series.  
> > 
> > Why don't you just send individual patches for each subsystem? I'm not a 
> > maintainer but I don't see
> > how any one person is going to be able to apply this whole series, it is 
> > making it hard for
> > maintainers if they have to pick patches out from among the series (if 
> > indeed any will bother
> > doing that).
> Yeah, maybe it would have been better to send individual patches.
> 
> From my point of view it's a series because the patches are related (I
> did a git format-patch from my local branch). But for the maintainers
> point of view, they are individual patches.

And the maintainers view is what matters here, if you wish to get your
patches reviewed and accepted...

thanks,

greg k-h

Re: [PATCH] [media] staging/atomisp: fix minor coding style warnings

[Greg KH]

On Thu, Jul 13, 2017 at 08:06:21AM -0700, smklearn wrote:
> Below were the minor issues flagged by checkpatch.pl:
> - WARNING: Block comments use * on subsequent lines
> - ERROR: space prohibited after that open parenthesis '('

Don't do multiple things in the same patch please, this should be
multiple patches.

> Signed-off-by: Shy More 

This name doesn't match your "From:" name in the email :(

thanks,

greg k-h

Re: [PATCH] staging drivers fixed coding style error

[Greg KH]

On Thu, Jul 13, 2017 at 07:17:56AM -0700, smklearn wrote:
> Fixed coding style error flagged checkpatch.pl:
>   - ERROR: space prohibited after that open parenthesis '('
>   - WARNING: Block comments use * on subsequent lines
> 
> Signed-off-by: Shy More 
> 
> Output after fixing coding style issues:
> 
> $KERN/scripts/checkpatch.pl -f
>   ./media/atomisp/pci/atomisp2/css2400/runtime/isys/src/ibuf_ctrl_rmgr.c
> 
> total: 0 errors, 0 warnings, 141 lines checked

Please don't put anything below the Signed-off-by: line, you will note
that all other commits are written that way.

Also, your subject: needs a lot of work, again, look at other commits
for the driver you are modifying to get it right.

good luck!

greg k-h

Re: [PATCH] Staging: media: fix missing blank line coding style issue in atomisp_tpg.c

[Greg KH]

On Thu, May 18, 2017 at 05:31:20PM +0100, Alan Cox wrote:
> On Wed, 2017-05-17 at 21:48 -0400, Manny Vindiola wrote:
> > This is a patch to the atomisp_tpg.c file that fixes up a missing
> > blank line warning found by the checkpatch.pl tool
> > 
> > Signed-off-by: Manny Vindiola 
> > ---
> >  drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
> > b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
> > index 996d1bd..48b9604 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
> > @@ -56,6 +56,7 @@ static int tpg_set_fmt(struct v4l2_subdev *sd,
> >        struct v4l2_subdev_format *format)
> >  {
> >     struct v4l2_mbus_framefmt *fmt = >format;
> > +
> >     if (format->pad)
> >     return -EINVAL;
> >     /* only raw8 grbg is supported by TPG */
> 
> The TODO fille for this driver specifically says not to send formatting
> patches at this point.
> 
> There is no point making trivial spacing changes in code that needs
> lots of real work. It's like polishing your car when the doors have
> fallen off.

Unfortunatly, given that the code is in staging, that's not going to
happen, people are going to send cleanup patches, and that's ok.  They
should be easy to merge around, it's the price for being in the tree.

thanks,

greg k-h

Re: [PATCH] ATOMISP: Tidies up code warnings and errors in file

[Greg KH]

On Mon, May 08, 2017 at 11:25:55PM +0100, Mark Railton wrote:
> Cleared up some errors and warnings in
> drivers/staging/media/atomisp/i2c/ap1302.c
> 
> Signed-off-by: Mark Railton 
> ---
>  drivers/staging/media/atomisp/i2c/ap1302.c | 83 
> ++
>  1 file changed, 50 insertions(+), 33 deletions(-)

Always be specific as to what exactly you are doing, and don't do
multiple different things in a single patch like you did here (hint,
"all warnings/errors isn't one thing".

thanks,

greg k-h

Re: uvcvideo logging kernel warnings on device disconnect

[Greg KH]

On Sun, Apr 30, 2017 at 06:19:59PM +0200, Greg KH wrote:
> On Sun, Apr 16, 2017 at 02:11:31PM +0300, Laurent Pinchart wrote:
> > Hi Greg,
> > 
> > On Wednesday 21 Dec 2016 10:59:54 Greg KH wrote:
> > > On Tue, Dec 20, 2016 at 11:19:23AM +, Dave Stevenson wrote:
> > > > On 09/12/16 09:43, Greg KH wrote:
> > > >> On Fri, Dec 09, 2016 at 11:14:41AM +0200, Laurent Pinchart wrote:
> > > >>> On Friday 09 Dec 2016 10:11:13 Greg KH wrote:
> > > >>>> On Fri, Dec 09, 2016 at 10:59:24AM +0200, Laurent Pinchart wrote:
> > > >>>>> On Friday 09 Dec 2016 08:25:52 Greg KH wrote:
> > > >>>>>> On Fri, Dec 09, 2016 at 01:09:21AM +0200, Laurent Pinchart wrote:
> > > >>>>>>> On Thursday 08 Dec 2016 12:31:55 Dave Stevenson wrote:
> > > >>>>>>>> Hi All.
> > > >>>>>>>> 
> > > >>>>>>>> I'm working with a USB webcam which has been seen to
> > > >>>>>>>> spontaneously disconnect when in use. That's a separate
> > > >>>>>>>> issue, but when it does it throws a load of warnings into
> > > >>>>>>>> the kernel log if there is a file handle on the device open
> > > >>>>>>>> at the time, even if not streaming.
> > > >>>>>>>> 
> > > >>>>>>>> I've reproduced this with a generic Logitech C270 webcam on:
> > > >>>>>>>> - Ubuntu 16.04 (kernel 4.4.0-51) vanilla, and with the
> > > >>>>>>>> latest media tree from linuxtv.org
> > > >>>>>>>> - Ubuntu 14.04 (kernel 4.4.0-42) vanilla
> > > >>>>>>>> - an old 3.10.x tree on an embedded device.
> > > >>>>>>>> 
> > > >>>>>>>> To reproduce:
> > > >>>>>>>> - connect USB webcam.
> > > >>>>>>>> - run a simple app that opens /dev/videoX, sleeps for a
> > > >>>>>>>> while, and then closes the handle.
> > > >>>>>>>> - disconnect the webcam whilst the app is running.
> > > >>>>>>>> - read kernel logs - observe warnings. We get the disconnect
> > > >>>>>>>> logged as it occurs, but the warnings all occur when the
> > > >>>>>>>> file descriptor is closed. (A copy of the logs from my
> > > >>>>>>>> Ubuntu 14.04 machine are below).
> > > >>>>>>>> 
> > > >>>>>>>> I can fully appreciate that the open file descriptor is
> > > >>>>>>>> holding references to a now invalid device, but is there a
> > > >>>>>>>> way to avoid them? Or do we really not care and have to put
> > > >>>>>>>> up with the log noise when doing such silly things?
> > > >>>>>>> 
> > > >>>>>>> This is a known problem, caused by the driver core trying to
> > > >>>>>>> remove the same sysfs attributes group twice.
> > > >>>>>> 
> > > >>>>>> Ick, not good.
> > > >>>>>> 
> > > >>>>>>> The group is first removed when the USB device is
> > > >>>>>>> disconnected. The input device and media device created by the
> > > >>>>>>> uvcvideo driver are children of the USB interface device,
> > > >>>>>>> which is deleted from the system when the camera is unplugged.
> > > >>>>>>> Due to the parent-child relationship, all sysfs attribute
> > > >>>>>>> groups of the children are removed.
> > > >>>>>> 
> > > >>>>>> Wait, why is the USB device being removed from sysfs at this
> > > >>>>>> point, didn't the input and media subsystems grab a reference to
> > > >>>>>> it so that it does not disappear just yet?
> > > >>>>> 
> > > >>>>> References are taken in uvc_prove():
> > > >>>>> dev->udev = usb_get_dev(udev);
> > > >>>>> dev->intf = usb_get_intf(intf);
> > > >>>> 
> > > >>>

Re: uvcvideo logging kernel warnings on device disconnect

[Greg KH]

On Sun, Apr 16, 2017 at 02:11:31PM +0300, Laurent Pinchart wrote:
> Hi Greg,
> 
> On Wednesday 21 Dec 2016 10:59:54 Greg KH wrote:
> > On Tue, Dec 20, 2016 at 11:19:23AM +, Dave Stevenson wrote:
> > > On 09/12/16 09:43, Greg KH wrote:
> > >> On Fri, Dec 09, 2016 at 11:14:41AM +0200, Laurent Pinchart wrote:
> > >>> On Friday 09 Dec 2016 10:11:13 Greg KH wrote:
> > >>>> On Fri, Dec 09, 2016 at 10:59:24AM +0200, Laurent Pinchart wrote:
> > >>>>> On Friday 09 Dec 2016 08:25:52 Greg KH wrote:
> > >>>>>> On Fri, Dec 09, 2016 at 01:09:21AM +0200, Laurent Pinchart wrote:
> > >>>>>>> On Thursday 08 Dec 2016 12:31:55 Dave Stevenson wrote:
> > >>>>>>>> Hi All.
> > >>>>>>>> 
> > >>>>>>>> I'm working with a USB webcam which has been seen to
> > >>>>>>>> spontaneously disconnect when in use. That's a separate
> > >>>>>>>> issue, but when it does it throws a load of warnings into
> > >>>>>>>> the kernel log if there is a file handle on the device open
> > >>>>>>>> at the time, even if not streaming.
> > >>>>>>>> 
> > >>>>>>>> I've reproduced this with a generic Logitech C270 webcam on:
> > >>>>>>>> - Ubuntu 16.04 (kernel 4.4.0-51) vanilla, and with the
> > >>>>>>>> latest media tree from linuxtv.org
> > >>>>>>>> - Ubuntu 14.04 (kernel 4.4.0-42) vanilla
> > >>>>>>>> - an old 3.10.x tree on an embedded device.
> > >>>>>>>> 
> > >>>>>>>> To reproduce:
> > >>>>>>>> - connect USB webcam.
> > >>>>>>>> - run a simple app that opens /dev/videoX, sleeps for a
> > >>>>>>>> while, and then closes the handle.
> > >>>>>>>> - disconnect the webcam whilst the app is running.
> > >>>>>>>> - read kernel logs - observe warnings. We get the disconnect
> > >>>>>>>> logged as it occurs, but the warnings all occur when the
> > >>>>>>>> file descriptor is closed. (A copy of the logs from my
> > >>>>>>>> Ubuntu 14.04 machine are below).
> > >>>>>>>> 
> > >>>>>>>> I can fully appreciate that the open file descriptor is
> > >>>>>>>> holding references to a now invalid device, but is there a
> > >>>>>>>> way to avoid them? Or do we really not care and have to put
> > >>>>>>>> up with the log noise when doing such silly things?
> > >>>>>>> 
> > >>>>>>> This is a known problem, caused by the driver core trying to
> > >>>>>>> remove the same sysfs attributes group twice.
> > >>>>>> 
> > >>>>>> Ick, not good.
> > >>>>>> 
> > >>>>>>> The group is first removed when the USB device is
> > >>>>>>> disconnected. The input device and media device created by the
> > >>>>>>> uvcvideo driver are children of the USB interface device,
> > >>>>>>> which is deleted from the system when the camera is unplugged.
> > >>>>>>> Due to the parent-child relationship, all sysfs attribute
> > >>>>>>> groups of the children are removed.
> > >>>>>> 
> > >>>>>> Wait, why is the USB device being removed from sysfs at this
> > >>>>>> point, didn't the input and media subsystems grab a reference to
> > >>>>>> it so that it does not disappear just yet?
> > >>>>> 
> > >>>>> References are taken in uvc_prove():
> > >>>>> dev->udev = usb_get_dev(udev);
> > >>>>> dev->intf = usb_get_intf(intf);
> > >>>> 
> > >>>> s/uvc_prove/uvc_probe/ ?  :)
> > >>> 
> > >>> Oops :-)
> > >>> 
> > >>>>> and released in uvc_delete(), called when the last video device
> > >>>>> node is closed. This prevents the device from being released
> > >>>>> (freed), but device_del() is synchronous to device unplug as far
> > >>>>> as I understand.
> > >>>> 
> > >>>> Ok, good, that means the UVC driver is doing the right thing here.
> > >>>> 
> > >>>> But the sysfs files should only be attempted to be removed by the
> > >>>> driver core once, when the device is removed from sysfs, not twice,
> > >>>> which is really odd.
> > >>>> 
> > >>>> Is there a copy of the "simple app that grabs the device node"
> > >>>> anywhere so that I can test it out here with my USB camera device to
> > >>>> try to track down where the problem is?
> > >>> 
> > >>> Sure. The easiest way is to grab http://git.ideasonboard.org/yavta.git
> > >>> and run
> > >>> 
> > >>> yavta -c /dev/video0
> > >>> 
> > >>> (your mileage may vary if you have other video devices)
> > >> 
> > >> I'll point it at the correct device, /dev/video0 is built into this
> > >> laptop and can't be physically removed :)
> > >> 
> > >>> While the application is running, unplug the webcam, and then
> > >>> terminate the application with ctrl-C.
> > >> 
> > >> Ok, will try this out this afternoon and let you know how it goes.
> > > 
> > > I hate to pester, but wondered if you had found anything obvious.
> > > I really do appreciate you taking the time to look.
> > 
> > Sorry, I haven't had the chance and now will not be able to until
> > January
> 
> Did you mean January 2017 or 2018 ? :-)

Heh, sorry about this, I think David's patch should resolve this now.

thanks,

greg k-h

Re: [PATCH v2] [media] uvcvideo: Refactor teardown of uvc on USB disconnect

[Greg KH]

On Sun, Apr 23, 2017 at 02:53:49PM +1000, Daniel Axtens wrote:
> Currently, disconnecting a USB webcam while it is in use prints out a
> number of warnings, such as:
> 
> WARNING: CPU: 2 PID: 3118 at 
> /build/linux-ezBi1T/linux-4.8.0/fs/sysfs/group.c:237 
> sysfs_remove_group+0x8b/0x90
> sysfs group a7cd0780 not found for kobject 'event13'
> 
> This has been noticed before. [0]
> 
> This is because of the order in which things are torn down.
> 
> If there are no streams active during a USB disconnect:
> 
>  - uvc_disconnect() is invoked via device_del() through the bus
>notifier mechanism.
> 
>  - this calls uvc_unregister_video().
> 
>  - uvc_unregister_video() unregisters the video device for each
>stream,
> 
>  - because there are no streams open, it calls uvc_delete()
> 
>  - uvc_delete() calls uvc_status_cleanup(), which cleans up the status
>input device.
> 
>  - uvc_delete() calls media_device_unregister(), which cleans up the
>media device
> 
>  - uvc_delete(), uvc_unregister_video() and uvc_disconnect() all
>return, and we end up back in device_del().
> 
>  - device_del() then cleans up the sysfs folder for the camera with
>dpm_sysfs_remove(). Because uvc_status_cleanup() and
>media_device_unregister() have already been called, this all works
>nicely.
> 
> If, on the other hand, there *are* streams active during a USB disconnect:
> 
>  - uvc_disconnect() is invoked
> 
>  - this calls uvc_unregister_video()
> 
>  - uvc_unregister_video() unregisters the video device for each
>stream,
> 
>  - uvc_unregister_video() and uvc_disconnect() return, and we end up
>back in device_del().
> 
>  - device_del() then cleans up the sysfs folder for the camera with
>dpm_sysfs_remove(). Because the status input device and the media
>device are children of the USB device, this also deletes their
>sysfs folders.
> 
>  - Sometime later, the final stream is closed, invoking uvc_release().
> 
>  - uvc_release() calls uvc_delete()
> 
>  - uvc_delete() calls uvc_status_cleanup(), which cleans up the status
>input device. Because the sysfs directory has already been removed,
>this causes a WARNing.
> 
>  - uvc_delete() calls media_device_unregister(), which cleans up the
>media device. Because the sysfs directory has already been removed,
>this causes another WARNing.
> 
> To fix this, we need to make sure the devices are always unregistered
> before the end of uvc_disconnect(). To this, move the unregistration
> into the disconnect path:
> 
>  - split uvc_status_cleanup() into two parts, one on disconnect that
>unregisters and one on delete that frees.
> 
>  - move v4l2_device_unregister() and media_device_unregister() into
>the disconnect path.
> 
> [0]: https://lkml.org/lkml/2016/12/8/657
> 
> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Cc: Dave Stevenson <linux-me...@destevenson.freeserve.co.uk>
> Cc: Greg KH <g...@kroah.com>
> Signed-off-by: Daniel Axtens <d...@axtens.net>


Acked-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>

Very nice work, what a tangled web this is...

greg k-h

Re: [PATCH 1/2] [media] uvcvideo: Refactor teardown of uvc on USB disconnect

[Greg KH]

On Mon, Apr 17, 2017 at 03:23:55PM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch (and the investigation).
> 
> On Monday 17 Apr 2017 18:52:39 Daniel Axtens wrote:
> > Currently, disconnecting a USB webcam while it is in use prints out a
> > number of warnings, such as:
> > 
> > WARNING: CPU: 2 PID: 3118 at
> > /build/linux-ezBi1T/linux-4.8.0/fs/sysfs/group.c:237
> > sysfs_remove_group+0x8b/0x90 sysfs group a7cd0780 not found for
> > kobject 'event13'
> > 
> > This has been noticed before. [0]
> > 
> > This is because of the order in which things are torn down.
> > 
> > If there are no streams active during a USB disconnect:
> > 
> >  - uvc_disconnect() is invoked via device_del() through the bus
> >notifier mechanism.
> > 
> >  - this calls uvc_unregister_video().
> > 
> >  - uvc_unregister_video() unregisters the video device for each
> >stream,
> > 
> >  - because there are no streams open, it calls uvc_delete()
> > 
> >  - uvc_delete() calls uvc_status_cleanup(), which cleans up the status
> >input device.
> > 
> >  - uvc_delete() calls media_device_unregister(), which cleans up the
> >media device
> > 
> >  - uvc_delete(), uvc_unregister_video() and uvc_disconnect() all
> >return, and we end up back in device_del().
> > 
> >  - device_del() then cleans up the sysfs folder for the camera with
> >dpm_sysfs_remove(). Because uvc_status_cleanup() and
> >media_device_unregister() have already been called, this all works
> >nicely.
> > 
> > If, on the other hand, there *are* streams active during a USB disconnect:
> > 
> >  - uvc_disconnect() is invoked
> > 
> >  - this calls uvc_unregister_video()
> > 
> >  - uvc_unregister_video() unregisters the video device for each
> >stream,
> > 
> >  - uvc_unregister_video() and uvc_disconnect() return, and we end up
> >back in device_del().
> > 
> >  - device_del() then cleans up the sysfs folder for the camera with
> >dpm_sysfs_remove(). Because the status input device and the media
> >device are children of the USB device, this also deletes their
> >sysfs folders.
> > 
> >  - Sometime later, the final stream is closed, invoking uvc_release().
> > 
> >  - uvc_release() calls uvc_delete()
> > 
> >  - uvc_delete() calls uvc_status_cleanup(), which cleans up the status
> >input device. Because the sysfs directory has already been removed,
> >this causes a WARNing.
> > 
> >  - uvc_delete() calls media_device_unregister(), which cleans up the
> >media device. Because the sysfs directory has already been removed,
> >this causes another WARNing.
> > 
> > To fix this, we need to make sure the devices are always unregistered
> > before the end of uvc_disconnect(). To this, move the unregistration
> > into the disconnect path:
> >
> >  - split uvc_status_cleanup() into two parts, one on disconnect that
> >unregisters and one on delete that frees.
> > 
> >  - move media_device_unregister() into the disconnect path.
> 
> While the patch looks reasonable to me (with one comment below though), isn't 
> this an issue with the USB core, or possibly the device core ? It's a common 
> practice to create device nodes as children of physical devices. Does the 
> device core really require all device nodes to be unregistered synchronously 
> with physical device hot-unplug ? If so, shouldn't it warn somehow when a 
> device is deleted and still has children, instead of accepting that silently 
> and later complaining due to sysfs issues ?

When a physical device, or any other device, is removed, the children
should all also be removed.  You can't leave them around to be cleaned
up later, this has always been the case.

Yes, userspace can still hold references, but that should be fine as the
device will still be unregistered, just not freed yet, right?

> > [0]: https://lkml.org/lkml/2016/12/8/657
> > 
> > Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> > Cc: Dave Stevenson <linux-me...@destevenson.freeserve.co.uk>
> > Cc: Greg KH <g...@kroah.com>
> > Signed-off-by: Daniel Axtens <d...@axtens.net>
> > 
> > ---
> > 
> > Tested with cheese and yavta.
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 8 ++--
> >  drivers/media/usb/uvc/uvc_status.c | 8 ++--
> >  drivers/media/usb/uvc/uvcvideo.h   | 1 +
> >  3 files changed, 13 insertions(+), 4 deletions(-)
> > 
> &g

Re: [PATCH v3] [media] staging: css2400: fix checkpatch error

[Greg KH]

On Wed, Mar 29, 2017 at 10:50:08AM +0300, Haim Daniel wrote:
> isp_capture_defs.h: clean up ERROR: Macros with complex values should be 
> enclosed in parentheses
> 
> Signed-off-by: Haim Daniel 
> ---
>  .../pci/atomisp2/css2400/css_2401_csi2p_system/hrt/isp_capture_defs.h   | 2 
> +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git 
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/css_2401_csi2p_system/hrt/isp_capture_defs.h
>  
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/css_2401_csi2p_system/hrt/isp_capture_defs.h
> index aa413df..117c7a2 100644
> --- 
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/css_2401_csi2p_system/hrt/isp_capture_defs.h
> +++ 
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/css_2401_csi2p_system/hrt/isp_capture_defs.h
> @@ -19,7 +19,7 @@
>  #define _ISP_CAPTURE_BITS_PER_ELEM32  /* only for data, not 
> SOP */  
>  #define _ISP_CAPTURE_BYTES_PER_ELEM   
> (_ISP_CAPTURE_BITS_PER_ELEM/8  )  
>  #define _ISP_CAPTURE_BYTES_PER_WORD   32 /* 256/8 */ 
> -#define _ISP_CAPTURE_ELEM_PER_WORD
> _ISP_CAPTURE_BYTES_PER_WORD / _ISP_CAPTURE_BYTES_PER_ELEM 
> +#define _ISP_CAPTURE_ELEM_PER_WORD
> (_ISP_CAPTURE_BYTES_PER_WORD / _ISP_CAPTURE_BYTES_PER_ELEM)

Why only fix one of these instances?

And why is this define even needed?  No one touches it, right?

Also, please cc: Alan on patches to this driver when you resend.  You
are using scripts/get_maintainer.pl, right?

thanks,

greg k-h

Re: [PATCH 14/14] atomisp: remove UDS kernel code

[Greg KH]

On Fri, Apr 14, 2017 at 03:27:08AM +0800, kbuild test robot wrote:
> Hi Alan,
> 
> [auto build test ERROR on next-20170412]
> [cannot apply to linuxtv-media/master v4.9-rc8 v4.9-rc7 v4.9-rc6 v4.11-rc6]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Alan-Cox/staging-atomisp-use-local-variable-to-reduce-number-of-references/20170413-112312
> config: i386-allmodconfig (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
> >> make[7]: *** No rule to make target 
> >> 'drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/uds/uds_1.0/ia_css_uds.host.o',
> >>  needed by 'drivers/staging/media/atomisp/pci/atomisp2/atomisp.o'.
>make[7]: *** No rule to make target 
> 'drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/fixedbds/fixedbds_1.0/ia_css_fixedbds.host.o',
>  needed by 'drivers/staging/media/atomisp/pci/atomisp2/atomisp.o'.
>make[7]: Target '__build' not remade because of errors.

This too is odd, are you not rebuilding everything properly when a file
is removed?  This works for me here.

thanks,

greg k-h

Re: [PATCH 12/14] atomisp: remove fixedbds kernel code

[Greg KH]

On Thu, Apr 13, 2017 at 07:53:58PM +0800, kbuild test robot wrote:
> Hi Alan,
> 
> [auto build test ERROR on next-20170412]
> [cannot apply to linuxtv-media/master v4.9-rc8 v4.9-rc7 v4.9-rc6 v4.11-rc6]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Alan-Cox/staging-atomisp-use-local-variable-to-reduce-number-of-references/20170413-112312
> config: i386-allmodconfig (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
> >> make[7]: *** No rule to make target 
> >> 'drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/fixedbds/fixedbds_1.0/ia_css_fixedbds.host.o',
> >>  needed by 'drivers/staging/media/atomisp/pci/atomisp2/atomisp.o'.
>make[7]: Target '__build' not remade because of errors.

That's an odd error, this works fine for me here.

greg k-h

Re: [PATCH 28/38] Annotate hardware config module parameters in drivers/staging/media/

[Greg KH]

On Wed, Apr 05, 2017 at 06:01:01PM +0100, David Howells wrote:
> When the kernel is running in secure boot mode, we lock down the kernel to
> prevent userspace from modifying the running kernel image.  Whilst this
> includes prohibiting access to things like /dev/mem, it must also prevent
> access by means of configuring driver modules in such a way as to cause a
> device to access or modify the kernel image.
> 
> To this end, annotate module_param* statements that refer to hardware
> configuration and indicate for future reference what type of parameter they
> specify.  The parameter parser in the core sees this information and can
> skip such parameters with an error message if the kernel is locked down.
> The module initialisation then runs as normal, but just sees whatever the
> default values for those parameters is.
> 
> Note that we do still need to do the module initialisation because some
> drivers have viable defaults set in case parameters aren't specified and
> some drivers support automatic configuration (e.g. PNP or PCI) in addition
> to manually coded parameters.
> 
> This patch annotates drivers in drivers/staging/media/.
> 
> Suggested-by: Alan Cox 
> Signed-off-by: David Howells 
> cc: Mauro Carvalho Chehab 
> cc: Greg Kroah-Hartman 
> cc: linux-media@vger.kernel.org
> cc: de...@driverdev.osuosl.org

Acked-by: Greg Kroah-Hartman 

Re: [PATCH]: staging: media: css2400: fix checkpatch error

[Greg KH]

On Wed, Mar 29, 2017 at 08:36:27AM +0300, Haim Daniel wrote:

> >From 41d35b455f8eb139912909639e914469ef5e06fb Mon Sep 17 00:00:00 2001
> From: Haim Daniel 
> Date: Tue, 28 Mar 2017 19:27:57 +0300
> Subject: [PATCH] [media] staging: css2400: fix checkpatch error
> 
> isp_capture_defs.h:

What is this line for?

> 
> enclose macro with complex values in parentheses.
> 
> Signed-off-by: Haim Daniel 
> ---
>  .../pci/atomisp2/css2400/css_2401_csi2p_system/hrt/isp_capture_defs.h   | 2 
> +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Please don't attach your patch, use 'git send-email' to send it
properly.

thanks,

greg k-h

Re: [PATCH v2] [media] staging: css2400: fix checkpatch error

[Greg KH]

On Wed, Mar 29, 2017 at 10:12:28AM +0300, Haim Daniel wrote:
> isp_capture_defs.h:

What is this line for?

> fix checkpatch ERROR: 

Trailing whitespace?

> Macros with complex values should be enclosed in parentheses
> 
> Signed-off-by: Haim Daniel 
> ---
>  .../pci/atomisp2/css2400/css_2401_csi2p_system/hrt/isp_capture_defs.h   | 2 
> +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git 
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/css_2401_csi2p_system/hrt/isp_capture_defs.h
>  
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/css_2401_csi2p_system/hrt/isp_capture_defs.h
> index aa413df..78cbbf6 100644
> --- 
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/css_2401_csi2p_system/hrt/isp_capture_defs.h
> +++ 
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/css_2401_csi2p_system/hrt/isp_capture_defs.h
> @@ -19,7 +19,7 @@
>  #define _ISP_CAPTURE_BITS_PER_ELEM32  /* only for data, not 
> SOP */  
>  #define _ISP_CAPTURE_BYTES_PER_ELEM   
> (_ISP_CAPTURE_BITS_PER_ELEM/8  )  
>  #define _ISP_CAPTURE_BYTES_PER_WORD   32 /* 256/8 */ 
> -#define _ISP_CAPTURE_ELEM_PER_WORD
> _ISP_CAPTURE_BYTES_PER_WORD / _ISP_CAPTURE_BYTES_PER_ELEM 
> +#define _ISP_CAPTURE_ELEM_PER_WORD
> (_ISP_CAPTURE_BYTES_PER_WORD / _ISP_CAPTURE_BYTES_PER_ELEM) 

Does this change really make sense?  Why keep the trailing whitespace if
you touch the line?

thanks,

greg k-h

Re: [PATCH] Remove atomisp/i2c style errors.

[Greg KH]

On Tue, Mar 28, 2017 at 08:31:37PM -0700, Daniel Cashman wrote:
> From: Dan Cashman 

Please list what the issue you fixed in the subject line.

Also change the subject to match others for this driver, a 'git log'
will show you what to do there.

> 
> Remove two ' , ' issues and change spaces to tabs found by poking around in
> drivers/staging/. Warnings left untouched.
> 
> Test: Run checkpatch script in drivers/staging/media/atomisp/i2c before and
> after change.  Errors go from 3 to 0.

This isn't needed, and really, you didn't test the code, only a random
perl script :)

thanks,

greg k-h

Re: [PATCH] staging: media: atomisp: i2c: removed unnecessary white space before comma in memset()

[Greg KH]

On Tue, Mar 28, 2017 at 11:02:45AM +0530, vaibhavd...@gmail.com wrote:
> From: Vaibhav Kothari 
> 
> Removed extra space before comma in memset() as a part of
> checkpatch.pl fix-up.
> 
> Signed-off-by: Vaibhav Kothari 
> ---
>  drivers/staging/media/atomisp/i2c/gc2235.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

What changed from your prior emails with the same subject?  Always
version your patches, as SubmittingPatches describes how to do.

Please fix up and resend.

thanks,

greg k-h

Re: [PATCH] staging: media: atomisp: i2c: removed unnecessary white space before comma in memset()

[Greg KH]

On Tue, Mar 28, 2017 at 10:44:44AM +0530, vaibhavd...@gmail.com wrote:
> gc2235.c

Why is this file name here?

> 
>  Removed extra space before comma in memset() as a part of
>  checkpatch.pl fix-up.

Why the extra space at the beginning of the line?

> Signed-off-by: Vaibhav Kothari 

This doesn't match your "From:" line above :(

Please fix up.

thanks,

greg k-h

Re: [PATCH 06/24] atomisp: kill another define

[Greg KH]

On Mon, Mar 20, 2017 at 02:39:38PM +, Alan Cox wrote:
> We don't need an ifdef for the sake of 8-12 bytes. This undoes the ifdef 
> added by
> fde469701c7efabebf885e785edf367bfb1a8f3f. Instead turn it into a single const 
> string
> array at a fixed location thereby saving even more memory.
> 
> Signed-off-by: Alan Cox 
> ---
>  .../staging/media/atomisp/pci/atomisp2/hmm/hmm.c   |   23 
> +---
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 

This patch didn't apply to my tree, can you rebase it and resend?

thanks,

greg k-h

Re: [PATCH v1 1/7] staging: gc2235: Remove unnecessary typecast of c90 int constant

[Greg KH]

On Fri, Mar 10, 2017 at 04:20:23AM +0530, simran singhal wrote:
> This patch removes unnecessary typecast of c90 int constant.
> 
> WARNING: Unnecessary typecast of c90 int constant
> 
> Signed-off-by: simran singhal 
> ---
>  drivers/staging/media/atomisp/i2c/gc2235.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

This series doesn't apply to my tree at all, please rebase and resend.

thanks,

greg k-h

Re: [PATCH v1] staging: media: Remove unused function atomisp_set_stop_timeout()

[Greg KH]

On Fri, Mar 10, 2017 at 07:05:05PM +0530, simran singhal wrote:
> The function atomisp_set_stop_timeout on being called, simply returns
> back. The function hasn't been mentioned in the TODO and doesn't have
> FIXME code around. Hence, atomisp_set_stop_timeout and its calls have been
> removed.
> 
> This was done using Coccinelle.
> 
> @@
> identifier f;
> @@
> 
> void f(...) {
> 
> -return;
> 
> }
> 
> Signed-off-by: simran singhal 
> ---
>  v1:
>-Change Subject to include name of function
>-change commit message to include the coccinelle script

You should also cc: the developers doing all of the current work on this
driver, Alan Cox, to get their comment if this really is something that
can be removed or not.

thanks,

greg k-h

Re: [PATCH] staging: media: Remove parentheses from return arguments

[Greg KH]

On Fri, Mar 03, 2017 at 10:31:39PM +0530, simran singhal wrote:
> The sematic patch used for this is:
> @@
> identifier i;
> constant c;
> @@
> return
> - (
> \(i\|-i\|i(...)\|c\)
> - )
>   ;
> 
> Signed-off-by: simran singhal 
> Acked-by: Julia Lawall 
> ---
>  .../media/atomisp/pci/atomisp2/css2400/sh_css.c  | 20 
> ++--
>  .../atomisp/pci/atomisp2/css2400/sh_css_firmware.c   |  2 +-
>  2 files changed, 11 insertions(+), 11 deletions(-)

Again, one patch per driver.

Re: [PATCH 1/7] staging: media: Remove unnecessary typecast of c90 int constant

[Greg KH]

On Fri, Mar 03, 2017 at 01:21:56AM +0530, simran singhal wrote:
> This patch removes unnecessary typecast of c90 int constant.
> 
> WARNING: Unnecessary typecast of c90 int constant
> 
> Signed-off-by: simran singhal 
> ---
>  drivers/staging/media/atomisp/i2c/gc2235.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

The subject needs to say which driver is being touched here.  So this
would be:
 [PATCH 1/7] staging: gc2235: Remove unnecessary typecast of c90 int 
constant

Please fix up for all of these and resend.

thanks,

greg k-h

Re: [PATCH] atomisp2: unify some ifdef cases caused by format changes

[Greg KH]

On Wed, Mar 08, 2017 at 01:49:23PM +0100, Hans Verkuil wrote:
> OK, so I discovered that these patches are for a driver added to linux-next
> without it ever been cross-posted to linux-media.
> 
> To be polite, I think that's rather impolite.

They were, but got rejected due to the size :(

Mauro was cc:ed directly, he knew these were coming...

I can take care of the cleanup patches for now, you don't have to review
them if you don't want to.

thanks,

greg k-h

Re: [PATCH] atomisp2: unify some ifdef cases caused by format changes

[Greg KH]

On Wed, Mar 08, 2017 at 02:55:44PM +0100, Hans Verkuil wrote:
> On 08/03/17 14:45, Hans Verkuil wrote:
> > On 08/03/17 14:39, Greg KH wrote:
> > > On Wed, Mar 08, 2017 at 01:49:23PM +0100, Hans Verkuil wrote:
> > > > OK, so I discovered that these patches are for a driver added to 
> > > > linux-next
> > > > without it ever been cross-posted to linux-media.
> > > > 
> > > > To be polite, I think that's rather impolite.
> > > 
> > > They were, but got rejected due to the size :(
> > > 
> > > Mauro was cc:ed directly, he knew these were coming...
> > > 
> > > I can take care of the cleanup patches for now, you don't have to review
> > > them if you don't want to.
> > 
> > Please do.
> > 
> > For the next time if the patches are too large: at least post a message with
> > a link to a repo for people to look at. I would like to know what's going
> > on in staging/media, especially since I will do a lot of the reviewing (at
> > least if it is a V4L2 driver) when they want to move it out of staging.
> 
> Same issue BTW with the bcm2835 driver. That too landed in staging without
> ever being posted to the linux-media mailinglist. Size is no excuse for that
> driver since it isn't that large.
> 
> I'll handle cleanup patches for the bcm2835 driver since it is now in our 
> tree.

Nope, it got moved out as it didn't belong there yet :)

It's now in drivers/staging/vc04_services/ as all of the issues so far
aren't media ones, but vc04 api issues.  Once those get ironed out, then
the media people can have at it :)

thanks,

greg k-h

Re: [PATCH] Staging: media: platform: bcm2835 - Style fix

[Greg KH]

On Sun, Mar 05, 2017 at 10:43:19AM +1300, Derek Robson wrote:
> On Sat, Mar 04, 2017 at 02:57:22PM +0300, Dan Carpenter wrote:
> > Copy a patch prefix that everyone else has been using:
> > 
> > git log --oneline drivers/staging/media/platform/bcm2835/
> > 
> > The subject is too vague as well.
> 
> Is this what you are looking for?
> 
> [patch] Staging: bcm2835: fixed style of block comments

Yes.

> And should I just re-send as a V2 with new subject?

Yes.

thanks,

greg k-h

Re: [PATCH] staging/atomisp:fix build issue verus 4.11-rc1

[Greg KH]

On Mon, Mar 06, 2017 at 11:21:28AM +, Alan Cox wrote:
> From: xingzhen 
> 
> commit:2a1f062a4acf move sigpending method fatal_signal_pending from
>  into  cause the build issue,fix it.
> 
> Signed-off-by: xingzhen 
> Signed-off-by: Alan Cox 
> ---
>  .../media/atomisp/pci/atomisp2/hmm/hmm_bo.c|2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm_bo.c 
> b/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm_bo.c
> index 05ff912..e2aa949 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm_bo.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm_bo.c
> @@ -39,7 +39,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 

Ah, I already did this myself in my branch, thanks though!

greg k-h

Re: [PATCH] media: staging: bcm2048: use unsigned int instead of unsigned

[Greg KH]

On Mon, Feb 20, 2017 at 05:08:56PM -0700, David Cako wrote:
> Signed-off-by: David Cako 
> ---
>  drivers/staging/media/bcm2048/radio-bcm2048.c | 44 
> +--
>  1 file changed, 22 insertions(+), 22 deletions(-)

We can't take patches without any changelog text, sorry.  And always
test-build your patches, this is a known tricky one, you have to ignore
checkpatch, it is wrong.

sorry,

greg k-h

Re: [PATCH] siano: make it work again with CONFIG_VMAP_STACK

[Greg KH]

On Tue, Feb 14, 2017 at 05:32:11PM -0200, Mauro Carvalho Chehab wrote:
> Reported as a Kaffeine bug:
>   https://bugs.kde.org/show_bug.cgi?id=375811
> 
> The USB control messages require DMA to work. We cannot pass
> a stack-allocated buffer, as it is not warranted that the
> stack would be into a DMA enabled area.
> 
> On Kernel 4.9, the default is to not accept DMA on stack anymore.
> 
> Tested with USB ID 2040:5510: Hauppauge Windham
> 
> Cc: sta...@vger.kernel.org # For 4.9+

Unless there is some major reason, this should go into _all_ stable
releases, as the driver would be broken on them all for platforms that
can't handle USB data that is not DMA-able.  This has been a requirement
for USB drivers since the 2.2 days.

thanks,

greg k-h

Re: [PATCH] Staging: media: bcm2048: Fixed an error

[Greg KH]

On Sat, Feb 11, 2017 at 12:41:29AM +0200, Ran Algawi wrote:
> Fixed an error where the system was given a code in the form of decimal
> instead of octal.

It's not really an "error", right?  Please be more descriptive of
exactly what is going on here (hint, it's a coding style warning...)

thanks,

greg k-h

Re: [PATCH 1/2] Staging: media: bcm2048: fixed 20+ warings/errors

[Greg KH]

On Fri, Feb 10, 2017 at 11:37:04AM +0200, Ran Algawi wrote:
> Fixed a coding style issues, and two major erros about complex macros
> and an error where the driver used a decimal number insted of an octal
> number when using a warning.

Only do one thing-per-patch please.

thanks,

gre gk-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch] Staging: media: bcm2048: fixed errors and warnings

[Greg KH]

On Fri, Feb 10, 2017 at 11:41:41AM +0200, Ran Algawi wrote:
> 



Never attach patches, and always test-build them yourself to ensure they
do not break the build :(

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/2] staging: omap4iss: fix multiline comment style

[Greg KH]

On Tue, Feb 07, 2017 at 05:40:57PM +0200, Avraham Shukron wrote:
> Signed-off-by: Avraham Shukron 
> ---
>  drivers/staging/media/omap4iss/iss_video.c | 38 
> --
>  1 file changed, 25 insertions(+), 13 deletions(-)

Again, I don't take patches with no changelog text :(
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Staging: omap4iss: Fix coding style issues

[Greg KH]

On Mon, Feb 06, 2017 at 07:58:35PM +0200, Avraham Shukron wrote:
> Fixes line-over-80-characters issues as well as multiline comments style.

When you say things like "as well as", that's a hint that this needs to
be broken up into different patches.  Please do so here.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Staging: omap4iss: fix coding style issues. This is a patch that fixes checkpatch.pl issues in omap4iss/iss_video.c Specifically, it fixes "line over 80 characters" issues.

[Greg KH]

On Sat, Jan 28, 2017 at 05:59:43PM +0200, Avraham Shukron wrote:
> ---
>  drivers/staging/media/omap4iss/iss_video.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)

That's a crazy subject line :(

Please fix...
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] Staging: media: bcm2048: style fix - bare use of unsigned

[Greg KH]

On Mon, Jan 16, 2017 at 08:09:51PM +1300, Derek Robson wrote:
> Changed macro to not pass signedness and size as seprate fields.
> This is to improve code readablity.

Not really, it reads just fine as is.  In fact, it forces you to think
about the signed vs. unsigned of the variable and doesn't let you forget
it, which seems to be the intention of the code as-is.

So I would recommend just leaving it alone.

Remember, checkpatch is a hint, you always have to use your brain when
making kernel changes, and always test-build them :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Staging: media: bcm2048: style fix - bare use of unsigned

[Greg KH]

On Mon, Jan 16, 2017 at 06:06:25PM +1300, Derek Robson wrote:
> On Sun, Jan 15, 2017 at 10:40:02PM -0600, Scott Matheina wrote:
> > 
> > 
> > > On Jan 15, 2017, at 10:30 PM, Derek Robson  wrote:
> > > 
> > > Changed bare use of 'unsigned' to the prefered us of 'unsigned int'
> > > found using checkpatch
> > 
> > Just wondering if you compiled? This patch looks exactly like a patch I 
> > tried, but it didn't compile. 
> > 
> 
> It complied for me, I am on an X86 system.

I don't believe you, and kbuild backs that up  :)
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: uvcvideo logging kernel warnings on device disconnect

[Greg KH]

On Tue, Dec 20, 2016 at 11:19:23AM +, Dave Stevenson wrote:
> Hi Greg.
> 
> On 09/12/16 09:43, Greg KH wrote:
> > On Fri, Dec 09, 2016 at 11:14:41AM +0200, Laurent Pinchart wrote:
> > > Hi Greg,
> > > 
> > > On Friday 09 Dec 2016 10:11:13 Greg KH wrote:
> > > > On Fri, Dec 09, 2016 at 10:59:24AM +0200, Laurent Pinchart wrote:
> > > > > On Friday 09 Dec 2016 08:25:52 Greg KH wrote:
> > > > > > On Fri, Dec 09, 2016 at 01:09:21AM +0200, Laurent Pinchart wrote:
> > > > > > > On Thursday 08 Dec 2016 12:31:55 Dave Stevenson wrote:
> > > > > > > > Hi All.
> > > > > > > > 
> > > > > > > > I'm working with a USB webcam which has been seen to 
> > > > > > > > spontaneously
> > > > > > > > disconnect when in use. That's a separate issue, but when it 
> > > > > > > > does it
> > > > > > > > throws a load of warnings into the kernel log if there is a file
> > > > > > > > handle on the device open at the time, even if not streaming.
> > > > > > > > 
> > > > > > > > I've reproduced this with a generic Logitech C270 webcam on:
> > > > > > > > - Ubuntu 16.04 (kernel 4.4.0-51) vanilla, and with the latest 
> > > > > > > > media
> > > > > > > > tree from linuxtv.org
> > > > > > > > - Ubuntu 14.04 (kernel 4.4.0-42) vanilla
> > > > > > > > - an old 3.10.x tree on an embedded device.
> > > > > > > > 
> > > > > > > > To reproduce:
> > > > > > > > - connect USB webcam.
> > > > > > > > - run a simple app that opens /dev/videoX, sleeps for a while, 
> > > > > > > > and
> > > > > > > > then closes the handle.
> > > > > > > > - disconnect the webcam whilst the app is running.
> > > > > > > > - read kernel logs - observe warnings. We get the disconnect 
> > > > > > > > logged
> > > > > > > > as it occurs, but the warnings all occur when the file 
> > > > > > > > descriptor is
> > > > > > > > closed. (A copy of the logs from my Ubuntu 14.04 machine are 
> > > > > > > > below).
> > > > > > > > 
> > > > > > > > I can fully appreciate that the open file descriptor is holding
> > > > > > > > references to a now invalid device, but is there a way to avoid 
> > > > > > > > them?
> > > > > > > > Or do we really not care and have to put up with the log noise 
> > > > > > > > when
> > > > > > > > doing such silly things?
> > > > > > > 
> > > > > > > This is a known problem, caused by the driver core trying to 
> > > > > > > remove
> > > > > > > the same sysfs attributes group twice.
> > > > > > 
> > > > > > Ick, not good.
> > > > > > 
> > > > > > > The group is first removed when the USB device is disconnected. 
> > > > > > > The
> > > > > > > input device and media device created by the uvcvideo driver are
> > > > > > > children of the USB interface device, which is deleted from the 
> > > > > > > system
> > > > > > > when the camera is unplugged. Due to the parent-child 
> > > > > > > relationship,
> > > > > > > all sysfs attribute groups of the children are removed.
> > > > > > 
> > > > > > Wait, why is the USB device being removed from sysfs at this point,
> > > > > > didn't the input and media subsystems grab a reference to it so 
> > > > > > that it
> > > > > > does not disappear just yet?
> > > > > 
> > > > > References are taken in uvc_prove():
> > > > > dev->udev = usb_get_dev(udev);
> > > > > dev->intf = usb_get_intf(intf);
> > > > 
> > > > s/uvc_prove/uvc_probe/ ?  :)
> > > 
> > > Oops :-)
> > > 
> > > > > and released in uvc_delete(), called when the last video device node 
> > > > > is
> > > > > closed. This prevents the devi

Re: [PATCH RFC] omap3isp: prevent releasing MC too early

[Greg KH]

On Thu, Dec 15, 2016 at 10:57:16AM -0200, Mauro Carvalho Chehab wrote:
> Em Thu, 15 Dec 2016 09:42:35 -0300
> Javier Martinez Canillas  escreveu:
> 
> > Hello Mauro,
> > 
> > On 12/15/2016 09:37 AM, Mauro Carvalho Chehab wrote:
> > 
> > [snip]
> > 
> > > 
> > > What happens is that omap3isp driver calls media_device_unregister()
> > > too early. Right now, it is called at omap3isp_video_device_release(),
> > > with happens when a driver unbind is ordered by userspace, and not after
> > > the last usage of all /dev/video?? devices.
> > > 
> > > There are two possible fixes:
> > > 
> > > 1) at omap3isp_video_device_release(), streamoff all streams and mark
> > > that the media device will be gone.
> 
> I actually meant to say: isp_unregister_entities() here.
> 
> > > 
> > > 2) instead of using video_device_release_empty for the 
> > > video->video.release,
> > > create a omap3isp_video_device_release() that will call
> > > media_device_unregister() when destroying the last /dev/video?? devnode.
> > >  
> > 
> > There's also option (3), to have a proper refcounting to make sure that
> > the media device node is not freed until all references to it are gone.
> 
> Yes, that's another alternative.
> 
> > I understand that's what Sakari's RFC patches do. I'll try to make some
> > time tomorrow to test and review his patches.
> 
> The biggest problem with Sakari's patches is that it starts by 
> reverting 3 patches, and this will cause regressions on existing
> devices.
> 
> Development should be incremental.

How can reverting patches cause regressions?  If a patch that is applied
breaks something else, it needs to be reverted, end of story.  If that
patch happened to have fixed a different issue, that's fine, we are back
to the original issue, it's not a "regression" at all, the patch was
wrong in the first place.

So please, just revert them now.  That's the correct thing to do, as we
will be back to the previous release's behavior.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] omap3isp: prevent releasing MC too early

[Greg KH]

On Thu, Dec 15, 2016 at 02:13:42PM +0200, Laurent Pinchart wrote:
> Hi Mauro,
> 
> (CC'ing Greg)
> 
> On Wednesday 14 Dec 2016 13:14:06 Mauro Carvalho Chehab wrote:
> > Avoid calling streamoff without having the media structs allocated.
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> 
> The driver has a maintainer listed in MAINTAINERS, and you know that Sakari 
> is 
> also actively involved here. You could have CC'ed us.
> 
> > ---
> > 
> > Javier,
> > 
> > Could you please test this patch?
> > 
> > Thanks!
> > Mauro
> > 
> >  drivers/media/platform/omap3isp/ispvideo.c | 10 --
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> > b/drivers/media/platform/omap3isp/ispvideo.c index
> > 7354469670b7..f60995ed0a1f 100644
> > --- a/drivers/media/platform/omap3isp/ispvideo.c
> > +++ b/drivers/media/platform/omap3isp/ispvideo.c
> > @@ -1488,11 +1488,17 @@ int omap3isp_video_register(struct isp_video *video,
> > struct v4l2_device *vdev) "%s: could not register video device (%d)\n",
> > __func__, ret);
> > 
> > +   /* Prevent destroying MC before unregistering */
> > +   kobject_get(vdev->v4l2_dev->mdev->devnode->dev.parent);
> 
> This doesn't even compile. Please make sure to at least compile-test patches 
> you send for review, otherwise you end up wasting time for all reviewers and 
> testers. I assume you meant
> 
>   kobject_get(>mdev->devnode->dev.parent->kobj);
> 
> and similarly below.
> 
> That's a long list of pointer dereferences, going deep down the device core. 
> Greg, are drivers allowed to do this by the driver model ?

WTF?

Eeek, no no no no!

First off, no driver should EVER have to call a "raw" kobject call,
that's a huge sign that you are doing something really really wrong.

Secondly, you NEVER grab a reference to a structure like this, you use
the "correct" driver/bus api calls.

Thirdly, ugh, how to say this nicely...  The whole idea that something
like this could actually be a real solution to a problem is insane.

Look at what you are really doing here, trying to grab an extra
reference on something that in reality, should never go away anyhow.
Your parent structure should already always have the reference count
incremented and will not disappear underneath you at all.  And if this
isn't your "parent", you have no right at all to go grab random
references across the device tree for no reason other than you feel you
don't want it to go away.  If you don't want something to go away, you
properly get the reference (hint, you already should have if you have
this type of pointer chain to the object.)

If this type of solution is somehow the "correct" one, the v4l driver
model interaction is severely broken and needs to be fixed.

What bug is this that caused this type of hack to even be proposed?  Is
it a bug or a regression?  If a regression, can someone show me the
commit that would cause such a monstrosity to be proposed?

Laurent, sorry, I know I said I was going to debug a USB V4L reference
counting problem last week, I had to travel and haven't had the chance
to do so.  I'll try to get to it today.  Hopefully that issue has
nothing to do with this problem.  Because if so, ugh...

Mauro, again, please never propose something like this again.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: uvcvideo logging kernel warnings on device disconnect

[Greg KH]

On Fri, Dec 09, 2016 at 11:14:41AM +0200, Laurent Pinchart wrote:
> Hi Greg,
> 
> On Friday 09 Dec 2016 10:11:13 Greg KH wrote:
> > On Fri, Dec 09, 2016 at 10:59:24AM +0200, Laurent Pinchart wrote:
> > > On Friday 09 Dec 2016 08:25:52 Greg KH wrote:
> > >> On Fri, Dec 09, 2016 at 01:09:21AM +0200, Laurent Pinchart wrote:
> > >>> On Thursday 08 Dec 2016 12:31:55 Dave Stevenson wrote:
> > >>>> Hi All.
> > >>>> 
> > >>>> I'm working with a USB webcam which has been seen to spontaneously
> > >>>> disconnect when in use. That's a separate issue, but when it does it
> > >>>> throws a load of warnings into the kernel log if there is a file
> > >>>> handle on the device open at the time, even if not streaming.
> > >>>> 
> > >>>> I've reproduced this with a generic Logitech C270 webcam on:
> > >>>> - Ubuntu 16.04 (kernel 4.4.0-51) vanilla, and with the latest media
> > >>>> tree from linuxtv.org
> > >>>> - Ubuntu 14.04 (kernel 4.4.0-42) vanilla
> > >>>> - an old 3.10.x tree on an embedded device.
> > >>>> 
> > >>>> To reproduce:
> > >>>> - connect USB webcam.
> > >>>> - run a simple app that opens /dev/videoX, sleeps for a while, and
> > >>>> then closes the handle.
> > >>>> - disconnect the webcam whilst the app is running.
> > >>>> - read kernel logs - observe warnings. We get the disconnect logged
> > >>>> as it occurs, but the warnings all occur when the file descriptor is
> > >>>> closed. (A copy of the logs from my Ubuntu 14.04 machine are below).
> > >>>> 
> > >>>> I can fully appreciate that the open file descriptor is holding
> > >>>> references to a now invalid device, but is there a way to avoid them?
> > >>>> Or do we really not care and have to put up with the log noise when
> > >>>> doing such silly things?
> > >>> 
> > >>> This is a known problem, caused by the driver core trying to remove
> > >>> the same sysfs attributes group twice.
> > >> 
> > >> Ick, not good.
> > >> 
> > >>> The group is first removed when the USB device is disconnected. The
> > >>> input device and media device created by the uvcvideo driver are
> > >>> children of the USB interface device, which is deleted from the system
> > >>> when the camera is unplugged. Due to the parent-child relationship,
> > >>> all sysfs attribute groups of the children are removed.
> > >> 
> > >> Wait, why is the USB device being removed from sysfs at this point,
> > >> didn't the input and media subsystems grab a reference to it so that it
> > >> does not disappear just yet?
> > > 
> > > References are taken in uvc_prove():
> > > dev->udev = usb_get_dev(udev);
> > > dev->intf = usb_get_intf(intf);
> > 
> > s/uvc_prove/uvc_probe/ ?  :)
> 
> Oops :-)
> 
> > > and released in uvc_delete(), called when the last video device node is
> > > closed. This prevents the device from being released (freed), but
> > > device_del() is synchronous to device unplug as far as I understand.
> > 
> > Ok, good, that means the UVC driver is doing the right thing here.
> > 
> > But the sysfs files should only be attempted to be removed by the driver
> > core once, when the device is removed from sysfs, not twice, which is
> > really odd.
> > 
> > Is there a copy of the "simple app that grabs the device node" anywhere
> > so that I can test it out here with my USB camera device to try to track
> > down where the problem is?
> 
> Sure. The easiest way is to grab http://git.ideasonboard.org/yavta.git and run
> 
> yavta -c /dev/video0
> 
> (your mileage may vary if you have other video devices)

I'll point it at the correct device, /dev/video0 is built into this
laptop and can't be physically removed :)

> While the application is running, unplug the webcam, and then terminate the 
> application with ctrl-C.

Ok, will try this out this afternoon and let you know how it goes.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: uvcvideo logging kernel warnings on device disconnect

[Greg KH]

On Fri, Dec 09, 2016 at 10:59:24AM +0200, Laurent Pinchart wrote:
> Hi Greg,
> 
> On Friday 09 Dec 2016 08:25:52 Greg KH wrote:
> > On Fri, Dec 09, 2016 at 01:09:21AM +0200, Laurent Pinchart wrote:
> > > On Thursday 08 Dec 2016 12:31:55 Dave Stevenson wrote:
> > >> Hi All.
> > >> 
> > >> I'm working with a USB webcam which has been seen to spontaneously
> > >> disconnect when in use. That's a separate issue, but when it does it
> > >> throws a load of warnings into the kernel log if there is a file handle
> > >> on the device open at the time, even if not streaming.
> > >> 
> > >> I've reproduced this with a generic Logitech C270 webcam on:
> > >> - Ubuntu 16.04 (kernel 4.4.0-51) vanilla, and with the latest media tree
> > >> from linuxtv.org
> > >> - Ubuntu 14.04 (kernel 4.4.0-42) vanilla
> > >> - an old 3.10.x tree on an embedded device.
> > >> 
> > >> To reproduce:
> > >> - connect USB webcam.
> > >> - run a simple app that opens /dev/videoX, sleeps for a while, and then
> > >> closes the handle.
> > >> - disconnect the webcam whilst the app is running.
> > >> - read kernel logs - observe warnings. We get the disconnect logged as
> > >> it occurs, but the warnings all occur when the file descriptor is
> > >> closed. (A copy of the logs from my Ubuntu 14.04 machine are below).
> > >> 
> > >> I can fully appreciate that the open file descriptor is holding
> > >> references to a now invalid device, but is there a way to avoid them? Or
> > >> do we really not care and have to put up with the log noise when doing
> > >> such silly things?
> > > 
> > > This is a known problem, caused by the driver core trying to remove the
> > > same sysfs attributes group twice.
> > 
> > Ick, not good.
> > 
> > > The group is first removed when the USB device is disconnected. The input
> > > device and media device created by the uvcvideo driver are children of the
> > > USB interface device, which is deleted from the system when the camera is
> > > unplugged. Due to the parent-child relationship, all sysfs attribute
> > > groups of the children are removed.
> > 
> > Wait, why is the USB device being removed from sysfs at this point,
> > didn't the input and media subsystems grab a reference to it so that it
> > does not disappear just yet?
> 
> References are taken in uvc_prove():
> 
> dev->udev = usb_get_dev(udev);
> dev->intf = usb_get_intf(intf);

s/uvc_prove/uvc_probe/ ?  :)

> 
> and released in uvc_delete(), called when the last video device node is 
> closed. This prevents the device from being released (freed), but 
> device_del() 
> is synchronous to device unplug as far as I understand.

Ok, good, that means the UVC driver is doing the right thing here.

But the sysfs files should only be attempted to be removed by the driver
core once, when the device is removed from sysfs, not twice, which is
really odd.

Is there a copy of the "simple app that grabs the device node" anywhere
so that I can test it out here with my USB camera device to try to track
down where the problem is?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: uvcvideo logging kernel warnings on device disconnect

[Greg KH]

On Fri, Dec 09, 2016 at 01:09:21AM +0200, Laurent Pinchart wrote:
> Hi Dave,
> 
> (CC'ing LKML and Greg KH)
> 
> On Thursday 08 Dec 2016 12:31:55 Dave Stevenson wrote:
> > Hi All.
> > 
> > I'm working with a USB webcam which has been seen to spontaneously
> > disconnect when in use. That's a separate issue, but when it does it
> > throws a load of warnings into the kernel log if there is a file handle
> > on the device open at the time, even if not streaming.
> > 
> > I've reproduced this with a generic Logitech C270 webcam on:
> > - Ubuntu 16.04 (kernel 4.4.0-51) vanilla, and with the latest media tree
> > from linuxtv.org
> > - Ubuntu 14.04 (kernel 4.4.0-42) vanilla
> > - an old 3.10.x tree on an embedded device.
> > 
> > To reproduce:
> > - connect USB webcam.
> > - run a simple app that opens /dev/videoX, sleeps for a while, and then
> > closes the handle.
> > - disconnect the webcam whilst the app is running.
> > - read kernel logs - observe warnings. We get the disconnect logged as
> > it occurs, but the warnings all occur when the file descriptor is
> > closed. (A copy of the logs from my Ubuntu 14.04 machine are below).
> > 
> > I can fully appreciate that the open file descriptor is holding
> > references to a now invalid device, but is there a way to avoid them? Or
> > do we really not care and have to put up with the log noise when doing
> > such silly things?
> 
> This is a known problem, caused by the driver core trying to remove the same 
> sysfs attributes group twice.

Ick, not good.

> The group is first removed when the USB device is disconnected. The input 
> device and media device created by the uvcvideo driver are children of the 
> USB 
> interface device, which is deleted from the system when the camera is 
> unplugged. Due to the parent-child relationship, all sysfs attribute groups 
> of 
> the children are removed.

Wait, why is the USB device being removed from sysfs at this point,
didn't the input and media subsystems grab a reference to it so that it
does not disappear just yet?

> Then, when the device node is closed, the media device and input device are 
> unregistered, causing the corresponding devices to be deleted too. The driver 
> core tries to remove the sysfs attributes groups related to those devices, 
> and 
> issues a warning as they have been removed already.
> 
> I'm not sure how to fix that, any hint from LKML would be appreciated.

Properly grab a reference to the USB device?  :)

If that's already happening, please let me know and I'll see what needs
to be done, but I think that should solve the issue for you.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: core: urb make use of usb_endpoint_maxp_mult

[Greg KH]

On Sun, Nov 13, 2016 at 01:31:16PM +0300, Mike Krinkin wrote:
> Since usb_endpoint_maxp now returns only lower 11 bits mult
> calculation here isn't correct anymore and that breaks webcam
> for me. Patch make use of usb_endpoint_maxp_mult instead of
> direct calculation.
> 
> Fixes: abb621844f6a ("usb: ch9: make usb_endpoint_maxp() return
>only packet size")
> 
> Signed-off-by: Mike Krinkin 
> ---
>  drivers/usb/core/urb.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 0be49a1..d75cb8c 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -412,11 +412,8 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>   }
>  
>   /* "high bandwidth" mode, 1-3 packets/uframe? */
> - if (dev->speed == USB_SPEED_HIGH) {
> - int mult = 1 + ((max >> 11) & 0x03);
> - max &= 0x07ff;
> - max *= mult;
> - }
> + if (dev->speed == USB_SPEED_HIGH)
> + max *= usb_endpoint_maxp_mult(>desc);
>  
>   if (urb->number_of_packets <= 0)
>   return -EINVAL;

Felipe, this looks like it belongs in your tree...
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/11] getting back -Wmaybe-uninitialized

[Greg KH]

On Sun, Nov 13, 2016 at 09:47:41AM +0100, Greg KH wrote:
> On Sat, Nov 12, 2016 at 01:27:12PM +, Jonathan Cameron wrote:
> > On 11/11/16 19:49, Arnd Bergmann wrote:
> > > On Friday, November 11, 2016 9:13:00 AM CET Linus Torvalds wrote:
> > >> On Thu, Nov 10, 2016 at 8:44 AM, Arnd Bergmann <a...@arndb.de> wrote:
> > >>>
> > >>> Please merge these directly if you are happy with the result.
> > >>
> > >> I will take this.
> > > 
> > > Thanks a lot!
> > >  
> > >> I do see two warnings, but they both seem to be valid and recent,
> > >> though, so I have no issues with the spurious cases.
> > > 
> > > Ok, both of them should have my fixes coming your way already.
> > > 
> > >> Warning #1:
> > >>
> > >>   sound/soc/qcom/lpass-platform.c: In function 
> > >> ‘lpass_platform_pcmops_open’:
> > >>   sound/soc/qcom/lpass-platform.c:83:29: warning: ‘dma_ch’ may be used
> > >> uninitialized in this function [-Wmaybe-uninitialized]
> > >> drvdata->substream[dma_ch] = substream;
> > >> ~~~^~~
> > >>
> > >> and 'dma_ch' usage there really is crazy and wrong. Broken by
> > >> 022d00ee0b55 ("ASoC: lpass-platform: Fix broken pcm data usage")
> > > 
> > > Right, the patches crossed here, the bugfix patch that introduced
> > > this came into linux-next over the kernel summit, and the fix I
> > > sent on Tuesday made it into Mark Brown's tree on Wednesday but not
> > > before you pulled alsa tree. It should be fixed the next time you
> > > pull from the alsa tree, the commit is
> > > 
> > > 3b89e4b77ef9 ("ASoC: lpass-platform: initialize dma channel number")
> > >  
> > >> Warning #2 is not a real bug, but it's reasonable that gcc doesn't
> > >> know that storage_bytes (chip->read_size) has to be 2/4. Again,
> > >> introduced recently by commit 231147ee77f3 ("iio: maxim_thermocouple:
> > >> Align 16 bit big endian value of raw reads"), so you didn't see it.
> > > 
> > > This is the one I mentioned in the commit message as one that
> > > is fixed in linux-next and that should make it in soon.
> > > 
> > >>   drivers/iio/temperature/maxim_thermocouple.c: In function
> > >> ‘maxim_thermocouple_read_raw’:
> > >>   drivers/iio/temperature/maxim_thermocouple.c:141:5: warning: ‘ret’
> > >> may be used uninitialized in this function [-Wmaybe-uninitialized]
> > >> if (ret)
> > >>^
> > >>   drivers/iio/temperature/maxim_thermocouple.c:128:6: note: ‘ret’ was
> > >> declared here
> > >> int ret;
> > >> ^~~
> > >>
> > >> and I guess that code can just initialize 'ret' to '-EINVAL' or
> > >> something to just make the theoretical "somehow we had a wrong
> > >> chip->read_size" case error out cleanly.
> > > 
> > > Right, that was my conclusion too. I sent the bugfix on Oct 25
> > > for linux-next but it didn't make it in until this Monday, after
> > > you pulled the patch that introduced it on Oct 29.
> > > 
> > > The commit in staging-testing is
> > > 32cb7d27e65d ("iio: maxim_thermocouple: detect invalid storage size in 
> > > read()")
> > > 
> > > Greg and Jonathan, I see now that this is part of the 'iio-for-4.10b'
> > > branch, so I suspect you were not planning to send this before the
> > > merge window. Could you make sure this ends up in v4.9 so we get
> > > a clean build when -Wmaybe-uninitialized gets enabled again?
> > I'll queue this up and send a pull to Greg tomorrow.
> > 
> > Was highly doubtful that a false warning suppression (be it an
> > understandable one) was worth sending mid cycle, hence it was
> > taking the slow route.
> 
> I can just cherry-pick this, no need to send a separate pull request.

Now done and sent to Linus, so all should be good here.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/11] getting back -Wmaybe-uninitialized

[Greg KH]

On Sat, Nov 12, 2016 at 01:27:12PM +, Jonathan Cameron wrote:
> On 11/11/16 19:49, Arnd Bergmann wrote:
> > On Friday, November 11, 2016 9:13:00 AM CET Linus Torvalds wrote:
> >> On Thu, Nov 10, 2016 at 8:44 AM, Arnd Bergmann  wrote:
> >>>
> >>> Please merge these directly if you are happy with the result.
> >>
> >> I will take this.
> > 
> > Thanks a lot!
> >  
> >> I do see two warnings, but they both seem to be valid and recent,
> >> though, so I have no issues with the spurious cases.
> > 
> > Ok, both of them should have my fixes coming your way already.
> > 
> >> Warning #1:
> >>
> >>   sound/soc/qcom/lpass-platform.c: In function 
> >> ‘lpass_platform_pcmops_open’:
> >>   sound/soc/qcom/lpass-platform.c:83:29: warning: ‘dma_ch’ may be used
> >> uninitialized in this function [-Wmaybe-uninitialized]
> >> drvdata->substream[dma_ch] = substream;
> >> ~~~^~~
> >>
> >> and 'dma_ch' usage there really is crazy and wrong. Broken by
> >> 022d00ee0b55 ("ASoC: lpass-platform: Fix broken pcm data usage")
> > 
> > Right, the patches crossed here, the bugfix patch that introduced
> > this came into linux-next over the kernel summit, and the fix I
> > sent on Tuesday made it into Mark Brown's tree on Wednesday but not
> > before you pulled alsa tree. It should be fixed the next time you
> > pull from the alsa tree, the commit is
> > 
> > 3b89e4b77ef9 ("ASoC: lpass-platform: initialize dma channel number")
> >  
> >> Warning #2 is not a real bug, but it's reasonable that gcc doesn't
> >> know that storage_bytes (chip->read_size) has to be 2/4. Again,
> >> introduced recently by commit 231147ee77f3 ("iio: maxim_thermocouple:
> >> Align 16 bit big endian value of raw reads"), so you didn't see it.
> > 
> > This is the one I mentioned in the commit message as one that
> > is fixed in linux-next and that should make it in soon.
> > 
> >>   drivers/iio/temperature/maxim_thermocouple.c: In function
> >> ‘maxim_thermocouple_read_raw’:
> >>   drivers/iio/temperature/maxim_thermocouple.c:141:5: warning: ‘ret’
> >> may be used uninitialized in this function [-Wmaybe-uninitialized]
> >> if (ret)
> >>^
> >>   drivers/iio/temperature/maxim_thermocouple.c:128:6: note: ‘ret’ was
> >> declared here
> >> int ret;
> >> ^~~
> >>
> >> and I guess that code can just initialize 'ret' to '-EINVAL' or
> >> something to just make the theoretical "somehow we had a wrong
> >> chip->read_size" case error out cleanly.
> > 
> > Right, that was my conclusion too. I sent the bugfix on Oct 25
> > for linux-next but it didn't make it in until this Monday, after
> > you pulled the patch that introduced it on Oct 29.
> > 
> > The commit in staging-testing is
> > 32cb7d27e65d ("iio: maxim_thermocouple: detect invalid storage size in 
> > read()")
> > 
> > Greg and Jonathan, I see now that this is part of the 'iio-for-4.10b'
> > branch, so I suspect you were not planning to send this before the
> > merge window. Could you make sure this ends up in v4.9 so we get
> > a clean build when -Wmaybe-uninitialized gets enabled again?
> I'll queue this up and send a pull to Greg tomorrow.
> 
> Was highly doubtful that a false warning suppression (be it an
> understandable one) was worth sending mid cycle, hence it was
> taking the slow route.

I can just cherry-pick this, no need to send a separate pull request.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] v4l: rcar-fcp: Don't force users to check for disabled FCP support

[Greg KH]

On Tue, Oct 18, 2016 at 04:24:20PM +0300, Laurent Pinchart wrote:
> commit fd44aa9a254b18176ec3792a18e7de6977030ca8 upstream.
> 
> The rcar_fcp_enable() function immediately returns successfully when the
> FCP device pointer is NULL to avoid forcing the users to check the FCP
> device manually before every call. However, the stub version of the
> function used when the FCP driver is disabled returns -ENOSYS
> unconditionally, resulting in a different API contract for the two
> versions of the function.
> 
> As a user that requires FCP support will fail at probe time when calling
> rcar_fcp_get() if the FCP driver is disabled, the stub version of the
> rcar_fcp_enable() function will only be called with a NULL FCP device.
> We can thus return 0 unconditionally to align the behaviour with the
> normal version of the function.
> 
> Fixes: 94fcdf829793 ("[media] v4l: vsp1: Add FCP support")
> Reported-by: Sergei Shtylyov 
> Signed-off-by: Laurent Pinchart 
> Reviewed-by: Geert Uytterhoeven 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  include/media/rcar-fcp.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

What stable kernel(s) do you want this applied to?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Staging: media: omap4iss: fixed coding style issues

[Greg KH]

On Sun, Oct 16, 2016 at 05:18:56PM +0200, Hector Roussille wrote:
> Fixed coding style issues


You need to be a lot more specific please.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/28] media: don't print error when allocating urb fails

[Greg KH]

On Thu, Aug 11, 2016 at 11:03:36PM +0200, Wolfram Sang wrote:
> This per-subsystem series is part of a tree wide cleanup. usb_alloc_urb() uses
> kmalloc which already prints enough information on failure. So, let's simply
> remove those "allocation failed" messages from drivers like we did already for
> other -ENOMEM cases. gkh acked this approach when we talked about it at LCJ in
> Tokyo a few weeks ago.

I've taken all of these through the usb tree given the delay in response
from the media developers :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Backport a Security Fix for CVE-2015-7833 to v4.1

[Greg KH]

On Mon, Apr 18, 2016 at 06:01:19PM +0900, Yuki Machida wrote:
> Hi Greg and Sasha,
> 
> Please do not accept patch of 588afcc to stable tree,
> because above patch has some problem.
> It reported by Vladis and Hans.
> https://patchwork.linuxtv.org/patch/32798/
> https://www.spinics.net/lists/linux-media/msg96936.html
> http://article.gmane.org/gmane.linux.kernel.stable/174202/match=cve+2015+7833

Ok, now dropped from the 3.14-stable and 4.4-stable queues, thanks.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: USB xHCI regression after upgrading from kernel 3.19.0-51 to 4.2.0-34.

[Greg KH]

On Fri, Apr 08, 2016 at 10:51:26AM +0200, Hans de Goede wrote:
> Hi,
> 
> It is probably best to resend this mail to
> linux-usb 
> since this is more of a usb problem then a v4l2 problem,
> and all the usb experts are subscribed to that list.

Heh, I told him to send it to linux-media, sorry about that.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: On Lindent shortcomings and massive style fixing

[Greg KH]

On Mon, Dec 28, 2015 at 04:33:27PM +0200, Andrey Utkin wrote:
> After some iterations of checkpatch.pl, on a new developed driver
> (tw5864), now I have the following:
> 
>  $ grep 'WARNING\|ERROR' /src/checkpatch.tw5864 | sort | uniq -c
>  31 ERROR: do not use C99 // comments
> 147 WARNING: Block comments use a trailing */ on a separate line
> 144 WARNING: Block comments use * on subsequent lines
> 435 WARNING: line over 80 characters
> 
> At this point, Lindent was already used, and checkpatch.pl warnings
> introduced by Lindent itself were fixed. Usage of "indent
> --linux-style" (which behaves differently BTW) doesn't help anymore,
> too.
> 
> Could anybody please advise how to sort out these issues
> automatically, because they look like perfectly solvable in automated
> fashion. Of course manual work would result in more niceness, but I am
> not eager to go through hundreds of place of code just to fix "over 80
> characters" issues now.

Shouldn't take very long to do so, all of the above can be fixed in less
than a day's worth of work manually.  Or you can use indent to fix up
the line length issues, but watch out for the results, sometimes it's
better to refactor the code than to just blindly accept the output of
that tool.

good luck!

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] [media] Postpone the addition of MEDIA_IOC_G_TOPOLOGY

[Greg KH]

On Mon, Dec 28, 2015 at 08:37:19AM -0700, Shuah Khan wrote:
> On 12/28/2015 07:03 AM, Mauro Carvalho Chehab wrote:
> > There are a few discussions left with regards to this ioctl:
> > 
> > 1) the name of the new structs will contain _v2_ on it?
> > 2) what's the best alternative to avoid compat32 issues?
> > 
> > Due to that, let's postpone the addition of this new ioctl to
> > the next Kernel version, to give people more time to discuss it.
> 
> I thought we discussed this in our irc meeting and
> arrived at a good solution for compat32 issue
> 
> My recommendation is getting this ioctl into 4.5 with
> a warning that it could change. The reason for that is
> that this ioctl helps with testing the media controller
> v2 api. Without this API, we won't see much testing from
> userspace in 4.5

People will ignore the warning, that never works :(

sorry,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/15] dma: remove external references to dma_supported

[Greg KH]

On Sat, Oct 03, 2015 at 05:19:39PM +0200, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig 
> ---
>  Documentation/DMA-API.txt   | 13 -
>  drivers/usb/host/ehci-hcd.c |  2 +-
>  drivers/usb/host/fotg210-hcd.c  |  2 +-
>  drivers/usb/host/fusbh200-hcd.c |  2 +-
>  drivers/usb/host/oxu210hp-hcd.c |  2 +-
>  5 files changed, 4 insertions(+), 17 deletions(-)


Acked-by: Greg Kroah-Hartman 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] staging: media:lirc: Added a newline character after declaration

[Greg KH]

On Wed, Aug 12, 2015 at 08:41:42PM +0530, Aparna Karuthodi wrote:
 Added a newline character to remove a coding style warning detected
 by checkpatch.
 
 The warning is given below:
 drivers/staging/media/lirc/lirc_serial.c:1169: WARNING: quoted string split
 across lines
 
 Signed-off-by: Aparna Karuthodi kdasapa...@gmail.com
 ---
  drivers/staging/media/lirc/lirc_serial.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/staging/media/lirc/lirc_serial.c 
 b/drivers/staging/media/lirc/lirc_serial.c
 index 19628d0..628577f 100644
 --- a/drivers/staging/media/lirc/lirc_serial.c
 +++ b/drivers/staging/media/lirc/lirc_serial.c
 @@ -1165,7 +1165,7 @@ module_init(lirc_serial_init_module);
  module_exit(lirc_serial_exit_module);
  
  MODULE_DESCRIPTION(Infra-red receiver driver for serial ports.);
 -MODULE_AUTHOR(Ralph Metzler, Trent Piepho, Ben Pfaff, 
 +MODULE_AUTHOR(Ralph Metzler, Trent Piepho, Ben Pfaff,\n
 Christoph Bartelmus, Andrei Tanas);

No, you just changed the way this string looks, that's not ok at all.

This is fine the way it is, you can ignore it.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] Staging: media: davinci_vpfe: fix over 80 characters coding style issue.

[Greg KH]

On Sat, Aug 08, 2015 at 03:42:18PM +0530, Sudip Mukherjee wrote:
 On Sat, Aug 08, 2015 at 01:55:02AM -0500, Junsu Shin wrote:
  
  On 08/06/2015 11:45 PM, Sudip Mukherjee wrote:
   On Thu, Aug 06, 2015 at 09:55:54PM -0500, Junsu Shin wrote:
   
 snip
  
  Thanks for pointing it out.
  Again, this is a patch to the dm365_ipipe.c that fixes over 80 characters 
  warning detected by the script.
  This time I fixed up the indentation issue claimed in the previous one.
  Signed-off-by: Junsu Shin jjun...@gmail.com
  ---
 Greg will not accept patches like this way. please send it as v2.

Greg does not accept drivers/staging/media/* patches, but anyway, no one
will accept a patch in this format, as you say.

thanks,

greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Y2038] [PATCH] Staging: media: lirc: Replace timeval with ktime_t

[Greg KH]

On Mon, Jun 08, 2015 at 09:37:24PM +0200, Ksenija Stanojević wrote:
 Hi Greg,
 
 It's been over two weeks that I've sent this patch.  Have you missed it?

Not at all, please look at the output of
$ ./scripts/get_maintainer.pl --file 
drivers/staging/media/lirc/lirc_sir.c

To see why I ignored this.

greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] radio-bcm2048: Fix region selection

[Greg KH]

On Sat, May 16, 2015 at 01:22:27PM +0200, Jan Roemisch wrote:
 Oh sorry, the real name is Jan Roemisch.

Ok, thanks, can someone please fix up the patches and resend them?

greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] dma-buf: add ref counting for module as exporter

[Greg KH]

On Fri, May 08, 2015 at 07:12:43PM +0530, Sumit Semwal wrote:
 Add reference counting on a kernel module that exports dma-buf and
 implements its operations. This prevents the module from being unloaded
 while DMABUF file is in use.
 
 The original patch [1] was submitted by Tomasz Stanislawski, but this
 is a simpler way to do it.
 
 v3: call module_put() as late as possible, per gregkh's comment.
 v2: move owner to struct dma_buf, and use DEFINE_DMA_BUF_EXPORT_INFO
 macro to simplify the change.
 
 Signed-off-by: Sumit Semwal sumit.sem...@linaro.org
 
 [1]: https://lkml.org/lkml/2012/8/8/163
 ---
  drivers/dma-buf/dma-buf.c | 10 +-
  include/linux/dma-buf.h   | 10 --
  2 files changed, 17 insertions(+), 3 deletions(-)

Acked-by: Greg Kroah-Hartman gre...@linuxfoundation.org
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma-buf: add ref counting for module as exporter

[Greg KH]

On Thu, May 07, 2015 at 01:00:52PM +0530, Sumit Semwal wrote:
 Add reference counting on a kernel module that exports dma-buf and
 implements its operations. This prevents the module from being unloaded
 while DMABUF file is in use.
 
 The original patch [1] was submitted by Tomasz, but he's since shifted
 jobs and a ping didn't elicit any response.
 
   [tomasz: Original author]
 Signed-off-by: Tomasz Stanislawski t.stanisl...@samsung.com
 Acked-by: Daniel Vetter dan...@ffwll.ch
   [sumits: updated  rebased as per review comments]
 Signed-off-by: Sumit Semwal sumit.sem...@linaro.org
 
 [1]: https://lkml.org/lkml/2012/8/8/163
 ---
  drivers/dma-buf/dma-buf.c  | 9 -
  drivers/gpu/drm/armada/armada_gem.c| 1 +
  drivers/gpu/drm/drm_prime.c| 1 +
  drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 1 +
  drivers/gpu/drm/i915/i915_gem_dmabuf.c | 1 +
  drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c  | 1 +
  drivers/gpu/drm/tegra/gem.c| 1 +
  drivers/gpu/drm/udl/udl_dmabuf.c   | 1 +
  drivers/gpu/drm/vmwgfx/vmwgfx_prime.c  | 1 +
  drivers/media/v4l2-core/videobuf2-dma-contig.c | 1 +
  drivers/media/v4l2-core/videobuf2-dma-sg.c | 1 +
  drivers/media/v4l2-core/videobuf2-vmalloc.c| 1 +
  drivers/staging/android/ion/ion.c  | 1 +
  include/linux/dma-buf.h| 2 ++
  14 files changed, 22 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
 index c5a9138a6a8d..9583eac0238f 100644
 --- a/drivers/dma-buf/dma-buf.c
 +++ b/drivers/dma-buf/dma-buf.c
 @@ -29,6 +29,7 @@
  #include linux/anon_inodes.h
  #include linux/export.h
  #include linux/debugfs.h
 +#include linux/module.h
  #include linux/seq_file.h
  #include linux/poll.h
  #include linux/reservation.h
 @@ -64,6 +65,7 @@ static int dma_buf_release(struct inode *inode, struct file 
 *file)
   BUG_ON(dmabuf-cb_shared.active || dmabuf-cb_excl.active);
  
   dmabuf-ops-release(dmabuf);
 + module_put(dmabuf-ops-owner);
  
   mutex_lock(db_list.lock);
   list_del(dmabuf-list_node);
 @@ -302,9 +304,14 @@ struct dma_buf *dma_buf_export(const struct 
 dma_buf_export_info *exp_info)
   return ERR_PTR(-EINVAL);
   }
  
 + if (!try_module_get(exp_info-ops-owner))
 + return ERR_PTR(-ENOENT);
 +
   dmabuf = kzalloc(alloc_size, GFP_KERNEL);
 - if (dmabuf == NULL)
 + if (!dmabuf) {
 + module_put(exp_info-ops-owner);
   return ERR_PTR(-ENOMEM);
 + }
  
   dmabuf-priv = exp_info-priv;
   dmabuf-ops = exp_info-ops;
 diff --git a/drivers/gpu/drm/armada/armada_gem.c 
 b/drivers/gpu/drm/armada/armada_gem.c
 index 580e10acaa3a..d2c5fc1269b7 100644
 --- a/drivers/gpu/drm/armada/armada_gem.c
 +++ b/drivers/gpu/drm/armada/armada_gem.c
 @@ -524,6 +524,7 @@ armada_gem_dmabuf_mmap(struct dma_buf *buf, struct 
 vm_area_struct *vma)
  }
  
  static const struct dma_buf_ops armada_gem_prime_dmabuf_ops = {
 + .owner = THIS_MODULE,
   .map_dma_buf= armada_gem_prime_map_dma_buf,
   .unmap_dma_buf  = armada_gem_prime_unmap_dma_buf,
   .release= drm_gem_dmabuf_release,

The easier way to do this is change the registration function to add
the module owner automatically, which keeps you from having to modify
all of the individual drivers.  Look at how pci and usb do this for
their driver registration functions.  That should result in a much
smaller patch, that always works properly for everyone (there's no way
for driver to get it wrong.)

thanks,

greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] dma-buf: add ref counting for module as exporter

[Greg KH]

On Thu, May 07, 2015 at 06:58:44PM +0530, Sumit Semwal wrote:
 Add reference counting on a kernel module that exports dma-buf and
 implements its operations. This prevents the module from being unloaded
 while DMABUF file is in use.
 
 The original patch [1] was submitted by Tomasz Stanislawski, but this
 is a simpler way to do it.
 
 v2: move owner to struct dma_buf, and use DEFINE_DMA_BUF_EXPORT_INFO
 macro to simplify the change.
 
 Signed-off-by: Sumit Semwal sumit.sem...@linaro.org
 
 [1]: https://lkml.org/lkml/2012/8/8/163
 ---
  drivers/dma-buf/dma-buf.c | 10 +-
  include/linux/dma-buf.h   | 10 --
  2 files changed, 17 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
 index c5a9138a6a8d..0eff4bf56ef6 100644
 --- a/drivers/dma-buf/dma-buf.c
 +++ b/drivers/dma-buf/dma-buf.c
 @@ -29,6 +29,7 @@
  #include linux/anon_inodes.h
  #include linux/export.h
  #include linux/debugfs.h
 +#include linux/module.h
  #include linux/seq_file.h
  #include linux/poll.h
  #include linux/reservation.h
 @@ -64,6 +65,7 @@ static int dma_buf_release(struct inode *inode, struct file 
 *file)
   BUG_ON(dmabuf-cb_shared.active || dmabuf-cb_excl.active);
  
   dmabuf-ops-release(dmabuf);
 + module_put(dmabuf-owner);

The module can now go away.

  
   mutex_lock(db_list.lock);
   list_del(dmabuf-list_node);

But you reference it here :(

Please drop the reference at the last possible moment.

thanks,

greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 0.led_name 2.other.led.name in /sysfs Re: [PATCH/RFC v11 01/20] leds: flash: document sysfs interface

[Greg KH]

On Fri, Feb 20, 2015 at 05:15:10PM +0100, Jacek Anaszewski wrote:
 On 02/20/2015 04:36 PM, Greg KH wrote:
 On Fri, Feb 20, 2015 at 08:56:11AM +0100, Jacek Anaszewski wrote:
 On 02/19/2015 10:40 PM, Greg KH wrote:
 On Thu, Feb 19, 2015 at 11:02:04AM +0200, Sakari Ailus wrote:
 On Wed, Feb 18, 2015 at 11:47:47PM +0100, Pavel Machek wrote:
 
 On Wed 2015-02-18 17:20:22, Jacek Anaszewski wrote:
 Add a documentation of LED Flash class specific sysfs attributes.
 
 Signed-off-by: Jacek Anaszewski j.anaszew...@samsung.com
 Acked-by: Kyungmin Park kyungmin.p...@samsung.com
 Cc: Bryan Wu coolo...@gmail.com
 Cc: Richard Purdie rpur...@rpsys.net
 
 NAK-ed-by: Pavel Machek
 
 +What:  /sys/class/leds/led/available_sync_leds
 +Date:  February 2015
 +KernelVersion: 3.20
 +Contact:   Jacek Anaszewski j.anaszew...@samsung.com
 +Description:   read/write
 +   Space separated list of LEDs available for flash strobe
 +   synchronization, displayed in the format:
 +
 +   led1_id.led1_name led2_id.led2_name led3_id.led3_name 
 etc.
 
 Multiple values per file, with all the problems we had in /proc. I
 assume led_id is an integer? What prevents space or dot in led name?
 
 Very good point. How about using a newline instead? That'd be a little bit
 easier to parse, too.
 
 No, please make it one value per-file, which is what sysfs requires.
 
 The purpose of this attribute is only to provide an information about
 the range of valid identifiers that can be written to the
 flash_sync_strobe attribute. Wouldn't splitting this to many attributes
 be an unnecessary inflation of sysfs files?
 
 Ok a list of allowed values to write is acceptable, as long as it is not
 hard to parse and always is space separated.
 
 Is a new line character also acceptable as a delimiter?

No.

Again, sysfs files should not need to be parsed, they are
one-value-per-file for a good reason.

If you want to do something else, wonderful, but don't use sysfs.  It's
looking like this whole interface should not be using sysfs as it
doesn't fit there at all.

sorry,

greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html