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.
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
Cc Tianshu and others.
Hi, Dan,
Thanks a lot for the code review.
Quoted 44 lines
-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: Friday, January 4, 2019 6:29 AM
To: Zhi, Yong <[email protected]>
Cc: [email protected]
Subject: [bug report] media: staging/intel-ipu3: Add imgu top level pci device driverHello 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.c472 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 */ ^^
Quoted 9 lines
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.
Quoted 10 lines
494 break; 495 dev_err(&imgu->pci_dev->dev, 496 "failed to dequeue buffers (%ld)\n", 497 PTR_ERR(b)); 498 break; 499 } 500regards,
dan carpenter