[bug report] media: staging/intel-ipu3: Add imgu top level pci device driver

[bug report] media: staging/intel-ipu3: Add imgu top level pci device driver

[Dan Carpenter]

Hello Yong Zhi,

The patch 7fc7af649ca7: "media: staging/intel-ipu3: Add imgu top
level pci device driver" from Dec 6, 2018, leads to the following
static checker warning:

	drivers/staging/media/ipu3/ipu3.c:493 imgu_isr_threaded()
	warn: 'b' is an error pointer or valid

drivers/staging/media/ipu3/ipu3.c
    472 static irqreturn_t imgu_isr_threaded(int irq, void *imgu_ptr)
    473 {
    474 	struct imgu_device *imgu = imgu_ptr;
    475 	struct imgu_media_pipe *imgu_pipe;
    476 	int p;
    477 
    478 	/* Dequeue / queue buffers */
    479 	do {
    480 		u64 ns = ktime_get_ns();
    481 		struct ipu3_css_buffer *b;
    482 		struct imgu_buffer *buf = NULL;
    483 		unsigned int node, pipe;
    484 		bool dummy;
    485 
    486 		do {
    487 			mutex_lock(&imgu->lock);
    488 			b = ipu3_css_buf_dequeue(&imgu->css);
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ipu3_css_buf_dequeue() doesn't return NULL.

    489 			mutex_unlock(&imgu->lock);
    490 		} while (PTR_ERR(b) == -EAGAIN);
    491
    492 		if (IS_ERR_OR_NULL(b)) {
                            ^^^^^^^^^^^^^^^^^
--> 493 			if (!b || PTR_ERR(b) == -EBUSY)	/* All done */
                                    ^^
When a function returns both NULL and error pointers, then NULL is
considered a special case of success.  Like perhaps you request a
feature, but that feature isn't enabled in the config.  It's fine,
because the user *chose* to turn off the feature, so it's not an error
but we also don't have a valid pointer we can use.

It looks like you were probably trying to do something like that but
you missed part of the commit?  Otherwise we should delete the dead
code.

    494 				break;
    495 			dev_err(&imgu->pci_dev->dev,
    496 				"failed to dequeue buffers (%ld)\n",
    497 				PTR_ERR(b));
    498 			break;
    499 		}
    500 

regards,
dan carpenter

RE: [bug report] media: staging/intel-ipu3: Add imgu top level pci device driver

[Zhi, Yong]

Cc Tianshu and others.

Hi, Dan,

Thanks a lot for the code review.

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, January 4, 2019 6:29 AM
> To: Zhi, Yong <yong.zhi@intel.com>
> Cc: linux-media@vger.kernel.org
> Subject: [bug report] media: staging/intel-ipu3: Add imgu top level pci device
> driver
> 
> Hello Yong Zhi,
> 
> The patch 7fc7af649ca7: "media: staging/intel-ipu3: Add imgu top level pci
> device driver" from Dec 6, 2018, leads to the following static checker warning:
> 
> 	drivers/staging/media/ipu3/ipu3.c:493 imgu_isr_threaded()
> 	warn: 'b' is an error pointer or valid
> 
> drivers/staging/media/ipu3/ipu3.c
>     472 static irqreturn_t imgu_isr_threaded(int irq, void *imgu_ptr)
>     473 {
>     474 	struct imgu_device *imgu = imgu_ptr;
>     475 	struct imgu_media_pipe *imgu_pipe;
>     476 	int p;
>     477
>     478 	/* Dequeue / queue buffers */
>     479 	do {
>     480 		u64 ns = ktime_get_ns();
>     481 		struct ipu3_css_buffer *b;
>     482 		struct imgu_buffer *buf = NULL;
>     483 		unsigned int node, pipe;
>     484 		bool dummy;
>     485
>     486 		do {
>     487 			mutex_lock(&imgu->lock);
>     488 			b = ipu3_css_buf_dequeue(&imgu->css);
>                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ipu3_css_buf_dequeue() doesn't return NULL.
> 
>     489 			mutex_unlock(&imgu->lock);
>     490 		} while (PTR_ERR(b) == -EAGAIN);
>     491
>     492 		if (IS_ERR_OR_NULL(b)) {
>                             ^^^^^^^^^^^^^^^^^
> --> 493 			if (!b || PTR_ERR(b) == -EBUSY)	/* All done */
>                                     ^^
> When a function returns both NULL and error pointers, then NULL is
> considered a special case of success.  Like perhaps you request a feature, but
> that feature isn't enabled in the config.  It's fine, because the user *chose* to
> turn off the feature, so it's not an error but we also don't have a valid pointer
> we can use.
> 
> It looks like you were probably trying to do something like that but you
> missed part of the commit?  Otherwise we should delete the dead code.
> 

Ack, with recent code changes the NULL check becomes useless, thanks for catching this.

>     494 				break;
>     495 			dev_err(&imgu->pci_dev->dev,
>     496 				"failed to dequeue buffers (%ld)\n",
>     497 				PTR_ERR(b));
>     498 			break;
>     499 		}
>     500
> 
> regards,
> dan carpenter