Signed-off-by: Ricardo Ribalda <[email protected]>
---
Ricardo Ribalda (6):
media: usb: siano: Fix allocation of urbs
media: cxd2880: Replaze kmalloc with kzalloc
media: platform: sti: hva: clk_unprepare unconditionally
media: v4l2-ctrls-core.c: Do not use iterator outside loop
media: adv7180: Only request valids IRQs
media: touchscreen: sur40: convert le16 to cpu before use
drivers/input/touchscreen/sur40.c | 2 +-
drivers/media/i2c/adv7180.c | 2 +-
drivers/media/platform/st/sti/hva/hva-hw.c | 3 +--
drivers/media/spi/cxd2880-spi.c | 2 +-
drivers/media/usb/siano/smsusb.c | 28 +++++++++++++++++--------
drivers/media/v4l2-core/v4l2-ctrls-api.c | 33 +++++++++++++++++-------------
6 files changed, 43 insertions(+), 27 deletions(-)
---
base-commit: 34d7bf1c8e59f5fbf438ee32c96389ebe41ca2e8
change-id: 20240410-smatch-8f235d50753d
Best regards,
--
Ricardo Ribalda <[email protected]>
USB urbs must be allocated with usb_alloc_urb. Quoting the manual
Only use this function (usb_init_urb) if you _really_ understand what you
are doing.
Fix the following smatch error:
drivers/media/usb/siano/smsusb.c:53:38: warning: array of flexible structures
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/usb/siano/smsusb.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
index 723510520d092..d85308e0785db 100644
--- a/drivers/media/usb/siano/smsusb.c
+++ b/drivers/media/usb/siano/smsusb.c
@@ -40,7 +40,7 @@ struct smsusb_urb_t {
struct smscore_buffer_t *cb;
struct smsusb_device_t *dev;
- struct urb urb;
+ struct urb *urb;
/* For the bottom half */
struct work_struct wq;
@@ -160,7 +160,7 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev,
}
usb_fill_bulk_urb(
- &surb->urb,
+ surb->urb,
dev->udev,
usb_rcvbulkpipe(dev->udev, dev->in_ep),
surb->cb->p,
@@ -168,9 +168,9 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev,
smsusb_onresponse,
surb
);
- surb->urb.transfer_flags |= URB_FREE_BUFFER;
+ surb->urb->transfer_flags |= URB_FREE_BUFFER;
- return usb_submit_urb(&surb->urb, GFP_ATOMIC);
+ return usb_submit_urb(surb->urb, GFP_ATOMIC);
}
static void smsusb_stop_streaming(struct smsusb_device_t *dev)
@@ -178,7 +178,7 @@ static void smsusb_stop_streaming(struct smsusb_device_t *dev)
int i;
for (i = 0; i < MAX_URBS; i++) {
- usb_kill_urb(&dev->surbs[i].urb);
+ usb_kill_urb(dev->surbs[i].urb);
if (dev->surbs[i].wq.func)
cancel_work_sync(&dev->surbs[i].wq);
@@ -338,6 +338,8 @@ static void smsusb_term_device(struct usb_interface *intf)
struct smsusb_device_t *dev = usb_get_intfdata(intf);
if (dev) {
+ int i;
+
dev->state = SMSUSB_DISCONNECTED;
smsusb_stop_streaming(dev);
@@ -346,6 +348,9 @@ static void smsusb_term_device(struct usb_interface *intf)
if (dev->coredev)
smscore_unregister_device(dev->coredev);
+ for (i = 0; i < MAX_URBS; i++)
+ usb_free_urb(dev->surbs[i].urb);
+
pr_debug("device 0x%p destroyed\n", dev);
kfree(dev);
}
@@ -390,6 +395,7 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
void *mdev;
int i, rc;
int align = 0;
+ int n_urb = 0;
/* create device object */
dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL);
@@ -461,9 +467,11 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
dev->coredev->is_usb_device = true;
/* initialize urbs */
- for (i = 0; i < MAX_URBS; i++) {
- dev->surbs[i].dev = dev;
- usb_init_urb(&dev->surbs[i].urb);
+ for (n_urb = 0; n_urb < MAX_URBS; n_urb++) {
+ dev->surbs[n_urb].dev = dev;
+ dev->surbs[n_urb].urb = usb_alloc_urb(0, GFP_KERNEL);
+ if (!dev->surbs[n_urb].urb)
+ goto free_urbs;
}
pr_debug("smsusb_start_streaming(...).\n");
@@ -485,6 +493,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
return rc;
+free_urbs:
+ for (i = 0; i < n_urb; i++)
+ usb_free_urb(dev->surbs[n_urb].urb);
+
err_unregister_device:
smsusb_term_device(intf);
#ifdef CONFIG_MEDIA_CONTROLLER_DVB
--
2.44.0.478.gd926399ef9-goog
Fix smatch error:
drivers/media/spi/cxd2880-spi.c:391 cxd2880_start_feed() warn: Please consider using kzalloc instead of kmalloc
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/spi/cxd2880-spi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/spi/cxd2880-spi.c b/drivers/media/spi/cxd2880-spi.c
index 6be4e5528879f..65fa7f857fcaf 100644
--- a/drivers/media/spi/cxd2880-spi.c
+++ b/drivers/media/spi/cxd2880-spi.c
@@ -388,7 +388,7 @@ static int cxd2880_start_feed(struct dvb_demux_feed *feed)
if (dvb_spi->feed_count == 0) {
dvb_spi->ts_buf =
- kmalloc(MAX_TRANS_PKT * 188,
+ kzalloc(MAX_TRANS_PKT * 188,
GFP_KERNEL | GFP_DMA);
if (!dvb_spi->ts_buf) {
pr_err("ts buffer allocate failed\n");
--
2.44.0.478.gd926399ef9-goog
hva->clk cannot be NULL at this point. Simplify the code and make smatch
happy:
drivers/media/platform/st/sti/hva/hva-hw.c:412 hva_hw_probe() warn: 'hva->clk' from clk_prepare() not released on lines: 412
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/platform/st/sti/hva/hva-hw.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/media/platform/st/sti/hva/hva-hw.c b/drivers/media/platform/st/sti/hva/hva-hw.c
index fe4ea2e7f37e3..fcb18fb52fdd7 100644
--- a/drivers/media/platform/st/sti/hva/hva-hw.c
+++ b/drivers/media/platform/st/sti/hva/hva-hw.c
@@ -406,8 +406,7 @@ int hva_hw_probe(struct platform_device *pdev, struct hva_dev *hva)
err_disable:
pm_runtime_disable(dev);
err_clk:
- if (hva->clk)
- clk_unprepare(hva->clk);
+ clk_unprepare(hva->clk);
return ret;
}
--
2.44.0.478.gd926399ef9-goog
Simplify a bit the code introducing a new variable for iterating through
the control list.
It also makes smatch happy:
drivers/media/v4l2-core/v4l2-ctrls-api.c:1091 v4l2_query_ext_ctrl() warn: iterator used outside loop: 'ref'
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/v4l2-core/v4l2-ctrls-api.c | 33 ++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
index d9a422017bd9d..42b7a45bfa79c 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
@@ -1052,35 +1052,40 @@ int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctr
if (id >= node2id(hdl->ctrl_refs.prev)) {
ref = NULL; /* Yes, so there is no next control */
} else if (ref) {
+ struct v4l2_ctrl_ref *pos = ref;
+
/*
* We found a control with the given ID, so just get
* the next valid one in the list.
*/
- list_for_each_entry_continue(ref, &hdl->ctrl_refs, node) {
- is_compound = ref->ctrl->is_array ||
- ref->ctrl->type >= V4L2_CTRL_COMPOUND_TYPES;
- if (id < ref->ctrl->id &&
- (is_compound & mask) == match)
+ ref = NULL;
+ list_for_each_entry_continue(pos, &hdl->ctrl_refs, node) {
+ is_compound = pos->ctrl->is_array ||
+ pos->ctrl->type >= V4L2_CTRL_COMPOUND_TYPES;
+ if (id < pos->ctrl->id &&
+ (is_compound & mask) == match) {
+ ref = pos;
break;
+ }
}
- if (&ref->node == &hdl->ctrl_refs)
- ref = NULL;
} else {
+ struct v4l2_ctrl_ref *pos;
+
/*
* No control with the given ID exists, so start
* searching for the next largest ID. We know there
* is one, otherwise the first 'if' above would have
* been true.
*/
- list_for_each_entry(ref, &hdl->ctrl_refs, node) {
- is_compound = ref->ctrl->is_array ||
- ref->ctrl->type >= V4L2_CTRL_COMPOUND_TYPES;
- if (id < ref->ctrl->id &&
- (is_compound & mask) == match)
+ list_for_each_entry(pos, &hdl->ctrl_refs, node) {
+ is_compound = pos->ctrl->is_array ||
+ pos->ctrl->type >= V4L2_CTRL_COMPOUND_TYPES;
+ if (id < pos->ctrl->id &&
+ (is_compound & mask) == match) {
+ ref = pos;
break;
+ }
}
- if (&ref->node == &hdl->ctrl_refs)
- ref = NULL;
}
}
mutex_unlock(hdl->lock);
--
2.44.0.478.gd926399ef9-goog
i2c_device_probe(), seems to assume that irq = 0 means that there is no
irq to request.
The driver also believes that on the clean path. So lets be consistent
here.
Also make smatch happy.
Fix:
drivers/media/i2c/adv7180.c:1526 adv7180_probe() warn: 'client->irq' from request_threaded_irq() not released on lines: 1526
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/i2c/adv7180.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 4829cbe324198..819ff9f7c90fe 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -1486,7 +1486,7 @@ static int adv7180_probe(struct i2c_client *client)
if (ret)
goto err_media_entity_cleanup;
- if (state->irq) {
+ if (state->irq > 0) {
ret = request_threaded_irq(client->irq, NULL, adv7180_irq,
IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
KBUILD_MODNAME, state);
--
2.44.0.478.gd926399ef9-goog
Smatch found this issue:
drivers/input/touchscreen/sur40.c:424:55: warning: incorrect type in argument 2 (different base types)
drivers/input/touchscreen/sur40.c:424:55: expected int key
drivers/input/touchscreen/sur40.c:424:55: got restricted __le16 [usertype] blob_id
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/input/touchscreen/sur40.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index ae3aab4283370..5f2cf8881e724 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -421,7 +421,7 @@ static void sur40_report_blob(struct sur40_blob *blob, struct input_dev *input)
if (blob->type != SUR40_TOUCH)
return;
- slotnum = input_mt_get_slot_by_key(input, blob->blob_id);
+ slotnum = input_mt_get_slot_by_key(input, le16_to_cpu(blob->blob_id));
if (slotnum < 0 || slotnum >= MAX_CONTACTS)
return;
--
2.44.0.478.gd926399ef9-goog
On 10/04/2024 23:54, Ricardo Ribalda wrote:
> USB urbs must be allocated with usb_alloc_urb. Quoting the manual
>
> Only use this function (usb_init_urb) if you _really_ understand what you
> are doing.
>
> Fix the following smatch error:
>
> drivers/media/usb/siano/smsusb.c:53:38: warning: array of flexible structures
>
> Signed-off-by: Ricardo Ribalda <[email protected]>
> ---
> drivers/media/usb/siano/smsusb.c | 28 ++++++++++++++++++++--------
> 1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
> index 723510520d092..d85308e0785db 100644
> --- a/drivers/media/usb/siano/smsusb.c
> +++ b/drivers/media/usb/siano/smsusb.c
> @@ -40,7 +40,7 @@ struct smsusb_urb_t {
> struct smscore_buffer_t *cb;
> struct smsusb_device_t *dev;
>
> - struct urb urb;
> + struct urb *urb;
>
> /* For the bottom half */
> struct work_struct wq;
> @@ -160,7 +160,7 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev,
> }
>
> usb_fill_bulk_urb(
> - &surb->urb,
> + surb->urb,
> dev->udev,
> usb_rcvbulkpipe(dev->udev, dev->in_ep),
> surb->cb->p,
> @@ -168,9 +168,9 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev,
> smsusb_onresponse,
> surb
> );
> - surb->urb.transfer_flags |= URB_FREE_BUFFER;
> + surb->urb->transfer_flags |= URB_FREE_BUFFER;
>
> - return usb_submit_urb(&surb->urb, GFP_ATOMIC);
> + return usb_submit_urb(surb->urb, GFP_ATOMIC);
> }
>
> static void smsusb_stop_streaming(struct smsusb_device_t *dev)
> @@ -178,7 +178,7 @@ static void smsusb_stop_streaming(struct smsusb_device_t *dev)
> int i;
>
> for (i = 0; i < MAX_URBS; i++) {
> - usb_kill_urb(&dev->surbs[i].urb);
> + usb_kill_urb(dev->surbs[i].urb);
> if (dev->surbs[i].wq.func)
> cancel_work_sync(&dev->surbs[i].wq);
>
> @@ -338,6 +338,8 @@ static void smsusb_term_device(struct usb_interface *intf)
> struct smsusb_device_t *dev = usb_get_intfdata(intf);
>
> if (dev) {
> + int i;
> +
> dev->state = SMSUSB_DISCONNECTED;
>
> smsusb_stop_streaming(dev);
> @@ -346,6 +348,9 @@ static void smsusb_term_device(struct usb_interface *intf)
> if (dev->coredev)
> smscore_unregister_device(dev->coredev);
>
> + for (i = 0; i < MAX_URBS; i++)
> + usb_free_urb(dev->surbs[i].urb);
> +
> pr_debug("device 0x%p destroyed\n", dev);
> kfree(dev);
> }
> @@ -390,6 +395,7 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
> void *mdev;
> int i, rc;
> int align = 0;
> + int n_urb = 0;
>
> /* create device object */
> dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL);
> @@ -461,9 +467,11 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
> dev->coredev->is_usb_device = true;
>
> /* initialize urbs */
> - for (i = 0; i < MAX_URBS; i++) {
> - dev->surbs[i].dev = dev;
> - usb_init_urb(&dev->surbs[i].urb);
> + for (n_urb = 0; n_urb < MAX_URBS; n_urb++) {
> + dev->surbs[n_urb].dev = dev;
> + dev->surbs[n_urb].urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!dev->surbs[n_urb].urb)
> + goto free_urbs;
> }
After allocating the URBs there are a few more error paths that do
'goto err_unregister_device;' instead of 'goto free_urbs;'. From what
I can see, those need to go through 'free_urbs' as well.
>
> pr_debug("smsusb_start_streaming(...).\n");
> @@ -485,6 +493,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
>
> return rc;
>
> +free_urbs:
> + for (i = 0; i < n_urb; i++)
> + usb_free_urb(dev->surbs[n_urb].urb);
Would it be better to also assign NULL to dev->surbs[n_urb].urb?
That way there are no invalid pointers that can mess up things.
Regards,
Hans
> +
> err_unregister_device:
> smsusb_term_device(intf);
> #ifdef CONFIG_MEDIA_CONTROLLER_DVB
>
On 15/04/2024 12:08, Hans Verkuil wrote:
> On 10/04/2024 23:54, Ricardo Ribalda wrote:
>> USB urbs must be allocated with usb_alloc_urb. Quoting the manual
>>
>> Only use this function (usb_init_urb) if you _really_ understand what you
>> are doing.
>>
>> Fix the following smatch error:
>>
>> drivers/media/usb/siano/smsusb.c:53:38: warning: array of flexible structures
>>
>> Signed-off-by: Ricardo Ribalda <[email protected]>
>> ---
>> drivers/media/usb/siano/smsusb.c | 28 ++++++++++++++++++++--------
>> 1 file changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
>> index 723510520d092..d85308e0785db 100644
>> --- a/drivers/media/usb/siano/smsusb.c
>> +++ b/drivers/media/usb/siano/smsusb.c
>> @@ -40,7 +40,7 @@ struct smsusb_urb_t {
>> struct smscore_buffer_t *cb;
>> struct smsusb_device_t *dev;
>>
>> - struct urb urb;
>> + struct urb *urb;
>>
>> /* For the bottom half */
>> struct work_struct wq;
>> @@ -160,7 +160,7 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev,
>> }
>>
>> usb_fill_bulk_urb(
>> - &surb->urb,
>> + surb->urb,
>> dev->udev,
>> usb_rcvbulkpipe(dev->udev, dev->in_ep),
>> surb->cb->p,
>> @@ -168,9 +168,9 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev,
>> smsusb_onresponse,
>> surb
>> );
>> - surb->urb.transfer_flags |= URB_FREE_BUFFER;
>> + surb->urb->transfer_flags |= URB_FREE_BUFFER;
>>
>> - return usb_submit_urb(&surb->urb, GFP_ATOMIC);
>> + return usb_submit_urb(surb->urb, GFP_ATOMIC);
>> }
>>
>> static void smsusb_stop_streaming(struct smsusb_device_t *dev)
>> @@ -178,7 +178,7 @@ static void smsusb_stop_streaming(struct smsusb_device_t *dev)
>> int i;
>>
>> for (i = 0; i < MAX_URBS; i++) {
>> - usb_kill_urb(&dev->surbs[i].urb);
>> + usb_kill_urb(dev->surbs[i].urb);
>> if (dev->surbs[i].wq.func)
>> cancel_work_sync(&dev->surbs[i].wq);
>>
>> @@ -338,6 +338,8 @@ static void smsusb_term_device(struct usb_interface *intf)
>> struct smsusb_device_t *dev = usb_get_intfdata(intf);
>>
>> if (dev) {
>> + int i;
>> +
>> dev->state = SMSUSB_DISCONNECTED;
>>
>> smsusb_stop_streaming(dev);
>> @@ -346,6 +348,9 @@ static void smsusb_term_device(struct usb_interface *intf)
>> if (dev->coredev)
>> smscore_unregister_device(dev->coredev);
>>
>> + for (i = 0; i < MAX_URBS; i++)
>> + usb_free_urb(dev->surbs[i].urb);
>> +
>> pr_debug("device 0x%p destroyed\n", dev);
>> kfree(dev);
>> }
>> @@ -390,6 +395,7 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
>> void *mdev;
>> int i, rc;
>> int align = 0;
>> + int n_urb = 0;
>>
>> /* create device object */
>> dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL);
>> @@ -461,9 +467,11 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
>> dev->coredev->is_usb_device = true;
>>
>> /* initialize urbs */
>> - for (i = 0; i < MAX_URBS; i++) {
>> - dev->surbs[i].dev = dev;
>> - usb_init_urb(&dev->surbs[i].urb);
>> + for (n_urb = 0; n_urb < MAX_URBS; n_urb++) {
>> + dev->surbs[n_urb].dev = dev;
>> + dev->surbs[n_urb].urb = usb_alloc_urb(0, GFP_KERNEL);
>> + if (!dev->surbs[n_urb].urb)
>> + goto free_urbs;
>> }
>
> After allocating the URBs there are a few more error paths that do
> 'goto err_unregister_device;' instead of 'goto free_urbs;'. From what
> I can see, those need to go through 'free_urbs' as well.
>
>>
>> pr_debug("smsusb_start_streaming(...).\n");
>> @@ -485,6 +493,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
>>
>> return rc;
>>
>> +free_urbs:
>> + for (i = 0; i < n_urb; i++)
>> + usb_free_urb(dev->surbs[n_urb].urb);
>
> Would it be better to also assign NULL to dev->surbs[n_urb].urb?
> That way there are no invalid pointers that can mess up things.
Ricardo, just post a v2 of this patch. I posted a PR for the other
patches. Nice work, BTW!
Regards,
Hans
>
> Regards,
>
> Hans
>
>> +
>> err_unregister_device:
>> smsusb_term_device(intf);
>> #ifdef CONFIG_MEDIA_CONTROLLER_DVB
>>
>
>
On Wed, Apr 10, 2024 at 09:54:43PM +0000, Ricardo Ribalda wrote:
> Smatch found this issue:
> drivers/input/touchscreen/sur40.c:424:55: warning: incorrect type in argument 2 (different base types)
> drivers/input/touchscreen/sur40.c:424:55: expected int key
> drivers/input/touchscreen/sur40.c:424:55: got restricted __le16 [usertype] blob_id
>
> Signed-off-by: Ricardo Ribalda <[email protected]>
> ---
> drivers/input/touchscreen/sur40.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index ae3aab4283370..5f2cf8881e724 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
> @@ -421,7 +421,7 @@ static void sur40_report_blob(struct sur40_blob *blob, struct input_dev *input)
> if (blob->type != SUR40_TOUCH)
> return;
>
> - slotnum = input_mt_get_slot_by_key(input, blob->blob_id);
> + slotnum = input_mt_get_slot_by_key(input, le16_to_cpu(blob->blob_id));
We are not really using the real value of the ID for computations but only
as an opaque "key", so doing the conversion is not strictly necessary,
but it is cheap so we may as well do it.
> if (slotnum < 0 || slotnum >= MAX_CONTACTS)
> return;
Applied, thank you.
--
Dmitry