[PATCH] media: stkwebcam: move stk_camera_read_reg() scratch buffer to struct stk_camera

[PATCH] media: stkwebcam: move stk_camera_read_reg() scratch buffer to struct stk_camera

[trix]

From: Tom Rix <trix@redhat.com>

In stk_camera_read_reg() a single byte buffer is alloc-ed and
freed on every function call.  Since the size is known,
move the buffer to the struct stk_camera where it will be alloc-ed
and freed once.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/media/usb/stkwebcam/stk-webcam.c | 11 ++---------
 drivers/media/usb/stkwebcam/stk-webcam.h |  2 ++
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
index 5b822214ccc5c..787edb3d47c23 100644
--- a/drivers/media/usb/stkwebcam/stk-webcam.c
+++ b/drivers/media/usb/stkwebcam/stk-webcam.c
@@ -150,25 +150,18 @@ int stk_camera_write_reg(struct stk_camera *dev, u16 index, u8 value)
 int stk_camera_read_reg(struct stk_camera *dev, u16 index, u8 *value)
 {
 	struct usb_device *udev = dev->udev;
-	unsigned char *buf;
 	int ret;
 
-	buf = kmalloc(sizeof(u8), GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
 	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
 			0x00,
 			USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
 			0x00,
 			index,
-			buf,
+			&dev->read_reg_scratch,
 			sizeof(u8),
 			500);
 	if (ret >= 0)
-		*value = *buf;
-
-	kfree(buf);
+		*value = dev->read_reg_scratch;
 
 	if (ret < 0)
 		return ret;
diff --git a/drivers/media/usb/stkwebcam/stk-webcam.h b/drivers/media/usb/stkwebcam/stk-webcam.h
index 14519e5308b18..136decffe9ced 100644
--- a/drivers/media/usb/stkwebcam/stk-webcam.h
+++ b/drivers/media/usb/stkwebcam/stk-webcam.h
@@ -105,6 +105,8 @@ struct stk_camera {
 	struct list_head sio_avail;
 	struct list_head sio_full;
 	unsigned sequence;
+
+	u8 read_reg_scratch;
 };
 
 #define vdev_to_camera(d) container_of(d, struct stk_camera, vdev)
-- 
2.26.3

Re: [PATCH] media: stkwebcam: move stk_camera_read_reg() scratch buffer to struct stk_camera

[Pavel Skripkin]

Hi Trix,

On 3/12/22 20:30, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> In stk_camera_read_reg() a single byte buffer is alloc-ed and
> freed on every function call.  Since the size is known,
> move the buffer to the struct stk_camera where it will be alloc-ed
> and freed once.
> 
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>   drivers/media/usb/stkwebcam/stk-webcam.c | 11 ++---------
>   drivers/media/usb/stkwebcam/stk-webcam.h |  2 ++
>   2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
> index 5b822214ccc5c..787edb3d47c23 100644
> --- a/drivers/media/usb/stkwebcam/stk-webcam.c
> +++ b/drivers/media/usb/stkwebcam/stk-webcam.c
> @@ -150,25 +150,18 @@ int stk_camera_write_reg(struct stk_camera *dev, u16 index, u8 value)
>   int stk_camera_read_reg(struct stk_camera *dev, u16 index, u8 *value)
>   {
>   	struct usb_device *udev = dev->udev;
> -	unsigned char *buf;
>   	int ret;
>   
> -	buf = kmalloc(sizeof(u8), GFP_KERNEL);
> -	if (!buf)
> -		return -ENOMEM;
> -
>   	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>   			0x00,
>   			USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
>   			0x00,
>   			index,
> -			buf,
> +			&dev->read_reg_scratch,
>   			sizeof(u8),
>   			500);


Wouldn't it be better to move to modern USB API like usb_control_msg_recv()?

This API does not require buffer to be allocated via kmalloc(), so you 
will be able to use value directly.




With regards,
Pavel Skripkin

Re: [PATCH] media: stkwebcam: move stk_camera_read_reg() scratch buffer to struct stk_camera

[Pavel Skripkin]

On 3/12/22 20:30, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> In stk_camera_read_reg() a single byte buffer is alloc-ed and
> freed on every function call.  Since the size is known,
> move the buffer to the struct stk_camera where it will be alloc-ed
> and freed once.
> 
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>   drivers/media/usb/stkwebcam/stk-webcam.c | 11 ++---------
>   drivers/media/usb/stkwebcam/stk-webcam.h |  2 ++
>   2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
> index 5b822214ccc5c..787edb3d47c23 100644
> --- a/drivers/media/usb/stkwebcam/stk-webcam.c
> +++ b/drivers/media/usb/stkwebcam/stk-webcam.c
> @@ -150,25 +150,18 @@ int stk_camera_write_reg(struct stk_camera *dev, u16 index, u8 value)
>   int stk_camera_read_reg(struct stk_camera *dev, u16 index, u8 *value)

And just random note: there are 4 possible uninit value bugs.

stk_start_stream() calls stk_camera_read_reg 4 times, but ignores return 
values.

stk_camera_read_reg() should have __must_check annotation and return 
value should be checked on each call.


If you have time you can take care of it :) Or I will fix it one day...





With regards,
Pavel Skripkin

Re: [PATCH] media: stkwebcam: move stk_camera_read_reg() scratch buffer to struct stk_camera

[Tom Rix]

On 3/12/22 11:58 AM, Pavel Skripkin wrote:
> On 3/12/22 20:30, trix@redhat.com wrote:
>> From: Tom Rix <trix@redhat.com>
>>
>> In stk_camera_read_reg() a single byte buffer is alloc-ed and
>> freed on every function call.  Since the size is known,
>> move the buffer to the struct stk_camera where it will be alloc-ed
>> and freed once.
>>
>> Signed-off-by: Tom Rix <trix@redhat.com>
>> ---
>>   drivers/media/usb/stkwebcam/stk-webcam.c | 11 ++---------
>>   drivers/media/usb/stkwebcam/stk-webcam.h |  2 ++
>>   2 files changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c 
>> b/drivers/media/usb/stkwebcam/stk-webcam.c
>> index 5b822214ccc5c..787edb3d47c23 100644
>> --- a/drivers/media/usb/stkwebcam/stk-webcam.c
>> +++ b/drivers/media/usb/stkwebcam/stk-webcam.c
>> @@ -150,25 +150,18 @@ int stk_camera_write_reg(struct stk_camera 
>> *dev, u16 index, u8 value)
>>   int stk_camera_read_reg(struct stk_camera *dev, u16 index, u8 *value)
>
> And just random note: there are 4 possible uninit value bugs.
>
> stk_start_stream() calls stk_camera_read_reg 4 times, but ignores 
> return values.
>
> stk_camera_read_reg() should have __must_check annotation and return 
> value should be checked on each call.
>
>
> If you have time you can take care of it :) Or I will fix it one day...

These do show up in my usual static analysis and it why I was looking at 
this file.

And was sidetracked by the short malloc.

Unfortunately I looked and there are many other similar instances 
treewide ~100

These aren't caught in checkpatch, so working on that..

Tom

>
>
>
>
>
> With regards,
> Pavel Skripkin
>

Re: [PATCH] media: stkwebcam: move stk_camera_read_reg() scratch buffer to struct stk_camera

[Pavel Skripkin]

Hi Tom,

On 3/13/22 02:48, Tom Rix wrote:
> These do show up in my usual static analysis and it why I was looking at
> this file.
> 
> And was sidetracked by the short malloc.
> 
> Unfortunately I looked and there are many other similar instances
> treewide ~100
> 

Most of them are in very old drivers and I don't think they ever be 
fixed. I've looked into one bug reported by syzkaller and there was like 
30 calls w/o proper error handling in one driver. Redoing whole driver 
logic without access to hw seems dangerous :))


> These aren't caught in checkpatch, so working on that..
> 

I think, it's not checkpath responsibility. Maybe it worth adding such 
check to smatch. I tried to implement such checker, but never finished it :(



With regards,
Pavel Skripkin

Re: [PATCH] media: stkwebcam: move stk_camera_read_reg() scratch buffer to struct stk_camera

[Tom Rix]

On 3/13/22 8:11 AM, Pavel Skripkin wrote:
> Hi Tom,
>
> On 3/13/22 02:48, Tom Rix wrote:
>> These do show up in my usual static analysis and it why I was looking at
>> this file.
>>
>> And was sidetracked by the short malloc.
>>
>> Unfortunately I looked and there are many other similar instances
>> treewide ~100
>>
>
> Most of them are in very old drivers and I don't think they ever be 
> fixed. I've looked into one bug reported by syzkaller and there was 
> like 30 calls w/o proper error handling in one driver. Redoing whole 
> driver logic without access to hw seems dangerous :))

 From the checkpatch change below, there are about 150 dinky allocs treewide

Here is a refactoring

https://lore.kernel.org/lkml/20220313141008.1503638-1-trix@redhat.com/

>
>
>> These aren't caught in checkpatch, so working on that..
>>
>
> I think, it's not checkpath responsibility. Maybe it worth adding such 
> check to smatch. I tried to implement such checker, but never finished 
> it :(
>
>
Poking new development to not do dinky allocs I think is worth it, here 
is my checkpatch patch

https://lore.kernel.org/lkml/20220313140827.1503359-1-trix@redhat.com/

steal the regex for smatch.

Tom

>
> With regards,
> Pavel Skripkin
>

Re: [PATCH] media: stkwebcam: move stk_camera_read_reg() scratch buffer to struct stk_camera

[Hans Verkuil]

Hi Tom,

On 12/03/2022 18:30, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> In stk_camera_read_reg() a single byte buffer is alloc-ed and
> freed on every function call.  Since the size is known,
> move the buffer to the struct stk_camera where it will be alloc-ed
> and freed once.

I read the replies to this patch, but I am not certain if you still want
this patch to be merged, or will make a v2. I have no problem applying this
patch as-is, but I just want to have confirmation that there won't be a v2.

Regards,

	Hans

> 
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  drivers/media/usb/stkwebcam/stk-webcam.c | 11 ++---------
>  drivers/media/usb/stkwebcam/stk-webcam.h |  2 ++
>  2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
> index 5b822214ccc5c..787edb3d47c23 100644
> --- a/drivers/media/usb/stkwebcam/stk-webcam.c
> +++ b/drivers/media/usb/stkwebcam/stk-webcam.c
> @@ -150,25 +150,18 @@ int stk_camera_write_reg(struct stk_camera *dev, u16 index, u8 value)
>  int stk_camera_read_reg(struct stk_camera *dev, u16 index, u8 *value)
>  {
>  	struct usb_device *udev = dev->udev;
> -	unsigned char *buf;
>  	int ret;
>  
> -	buf = kmalloc(sizeof(u8), GFP_KERNEL);
> -	if (!buf)
> -		return -ENOMEM;
> -
>  	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>  			0x00,
>  			USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
>  			0x00,
>  			index,
> -			buf,
> +			&dev->read_reg_scratch,
>  			sizeof(u8),
>  			500);
>  	if (ret >= 0)
> -		*value = *buf;
> -
> -	kfree(buf);
> +		*value = dev->read_reg_scratch;
>  
>  	if (ret < 0)
>  		return ret;
> diff --git a/drivers/media/usb/stkwebcam/stk-webcam.h b/drivers/media/usb/stkwebcam/stk-webcam.h
> index 14519e5308b18..136decffe9ced 100644
> --- a/drivers/media/usb/stkwebcam/stk-webcam.h
> +++ b/drivers/media/usb/stkwebcam/stk-webcam.h
> @@ -105,6 +105,8 @@ struct stk_camera {
>  	struct list_head sio_avail;
>  	struct list_head sio_full;
>  	unsigned sequence;
> +
> +	u8 read_reg_scratch;
>  };
>  
>  #define vdev_to_camera(d) container_of(d, struct stk_camera, vdev)

Re: [PATCH] media: stkwebcam: move stk_camera_read_reg() scratch buffer to struct stk_camera

[Tom Rix]

On 4/26/22 10:38 PM, Hans Verkuil wrote:
> Hi Tom,
>
> On 12/03/2022 18:30, trix@redhat.com wrote:
>> From: Tom Rix <trix@redhat.com>
>>
>> In stk_camera_read_reg() a single byte buffer is alloc-ed and
>> freed on every function call.  Since the size is known,
>> move the buffer to the struct stk_camera where it will be alloc-ed
>> and freed once.
> I read the replies to this patch, but I am not certain if you still want
> this patch to be merged, or will make a v2. I have no problem applying this
> patch as-is, but I just want to have confirmation that there won't be a v2.

Merged as-is is fine.

Thanks

Tom


>
> Regards,
>
> 	Hans
>
>> Signed-off-by: Tom Rix <trix@redhat.com>
>> ---
>>   drivers/media/usb/stkwebcam/stk-webcam.c | 11 ++---------
>>   drivers/media/usb/stkwebcam/stk-webcam.h |  2 ++
>>   2 files changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
>> index 5b822214ccc5c..787edb3d47c23 100644
>> --- a/drivers/media/usb/stkwebcam/stk-webcam.c
>> +++ b/drivers/media/usb/stkwebcam/stk-webcam.c
>> @@ -150,25 +150,18 @@ int stk_camera_write_reg(struct stk_camera *dev, u16 index, u8 value)
>>   int stk_camera_read_reg(struct stk_camera *dev, u16 index, u8 *value)
>>   {
>>   	struct usb_device *udev = dev->udev;
>> -	unsigned char *buf;
>>   	int ret;
>>   
>> -	buf = kmalloc(sizeof(u8), GFP_KERNEL);
>> -	if (!buf)
>> -		return -ENOMEM;
>> -
>>   	ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>>   			0x00,
>>   			USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
>>   			0x00,
>>   			index,
>> -			buf,
>> +			&dev->read_reg_scratch,
>>   			sizeof(u8),
>>   			500);
>>   	if (ret >= 0)
>> -		*value = *buf;
>> -
>> -	kfree(buf);
>> +		*value = dev->read_reg_scratch;
>>   
>>   	if (ret < 0)
>>   		return ret;
>> diff --git a/drivers/media/usb/stkwebcam/stk-webcam.h b/drivers/media/usb/stkwebcam/stk-webcam.h
>> index 14519e5308b18..136decffe9ced 100644
>> --- a/drivers/media/usb/stkwebcam/stk-webcam.h
>> +++ b/drivers/media/usb/stkwebcam/stk-webcam.h
>> @@ -105,6 +105,8 @@ struct stk_camera {
>>   	struct list_head sio_avail;
>>   	struct list_head sio_full;
>>   	unsigned sequence;
>> +
>> +	u8 read_reg_scratch;
>>   };
>>   
>>   #define vdev_to_camera(d) container_of(d, struct stk_camera, vdev)