Re: [PATCH v2 2/4] staging: media: atomisp: inline macros for checking the bo/bodev pointer

[Andy Shevchenko]

On Thu, Jul 23, 2026 at 09:51:19PM +0300, Nikolay Kulikov wrote:
> These macros perform a pointer check. Replace it with direct conditional
> expressions to simplify the code.

...

> struct hmm_buffer_object *hmm_bo_alloc(struct hmm_bo_device *bdev,
>  				       unsigned int pgnr)
>  {
>  	struct hmm_buffer_object *bo, *new_bo;
> -	struct rb_root *root = &bdev->free_rbtree;
> +	struct rb_root *root;
> +
> +	if (!bdev) {
> +		dev_err(atomisp_dev, "NULL hmm_bo_device.\n");
> +		return NULL;
> +	}
>  
> -	check_bodev_null_return(bdev, NULL);
> +	root = &bdev->free_rbtree;
>  	var_equal_return(hmm_bo_device_inited(bdev), 0, NULL,
>  			 "hmm_bo_device not inited yet.\n");

While at it, also replace here var_equal_return() with appropriate C code.

...

> void hmm_bo_device_exit(struct hmm_bo_device *bdev)
>  
>  	dev_dbg(atomisp_dev, "%s: entering!\n", __func__);
>  
> -	check_bodev_null_return_void(bdev);
> +	if (!bdev) {
> +		dev_err(atomisp_dev, "NULL hmm_bo_device.\n");
> +		return;
> +	}

While it's in the original code, usually in kernel we consider releasing or
existing functions be NULL-aware. Not sure if we need an error message to
be printed here. But I leave it to Sakari to decide.


...

> int hmm_bo_alloc_pages(struct hmm_buffer_object *bo,
>  {
>  	int ret = -EINVAL;

Do wee need to keep the above assignment?

> -	check_bo_null_return(bo, -EINVAL);
> +	if (!bo) {
> +		dev_err(atomisp_dev, "NULL hmm buffer object.\n");
> +		return -EINVAL;

Depending on the above it might be

		return ret;

But in such a case the above assignment should be split

	int ret;

	ret = -EINVAL;
	if (!bo) {
		dev_err(atomisp_dev, "NULL hmm buffer object.\n");
		return ret;

Looking now at this, I think that your variant is better, but with it it's
better to also check how ret is being used and split assignment.

	int ret;

	if (!bo) {
		dev_err(atomisp_dev, "NULL hmm buffer object.\n");
		return -EINVAL;
	...
	ret = -EINVAL;

> +	}

...

> static void hmm_bo_vm_open(struct vm_area_struct *vma)
>  {
>  	struct hmm_buffer_object *bo = vma->vm_private_data;
>  
> -	check_bo_null_return_void(bo);
> +	if (!bo) {
> +		dev_err(atomisp_dev, "NULL hmm buffer object.\n");
> +		return;
> +	}

In this case it's better to also split an assignment.

	struct hmm_buffer_object *bo;

	bo = vma->vm_private_data;
	if (!bo) {
		dev_err(atomisp_dev, "NULL hmm buffer object.\n");
		return;
	}

...

> static void hmm_bo_vm_close(struct vm_area_struct *vma)
>  {
>  	struct hmm_buffer_object *bo = vma->vm_private_data;
>  
> -	check_bo_null_return_void(bo);
> +	if (!bo) {
> +		dev_err(atomisp_dev, "NULL hmm buffer object.\n");
> +		return;
> +	}

Ditto.

-- 
With Best Regards,
Andy Shevchenko