Re: [RFC PATCH 3/3] drm/amdgpu: add the OMNI13B1 (OV13B10) ISP sensor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/amdgpu: missing gpiod_remove_lookup_table() in isp_v4_1_1_hw_init()/fini()
--

commit a5133602b5141a1308e409981d3e875c2a078846
Author: Seifelden Abdelmonem <saif.elden.hatim@gmail.com>

drm/amdgpu: add the OMNI13B1 (OV13B10) ISP sensor

This patch adds the OMNI13B1 ACPI ID to isp_sensor_ids so amdgpu detects
the OmniVision OV13B10 camera sensor on ISP 4.1.1 platforms. It also
defines and registers the GPIO lookup tables required by the ISP capture
and ov13b10 sensor drivers during ISP 4.1.1 hardware initialization.

> diff --git a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c b/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
> index 363266f3e0a3e..7d6072e279468 100644
> --- a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
> @@ -65,6 +65,14 @@ static struct gpiod_lookup_table isp_sensor_gpio_table = {
>  	},
>  };
>  
> +static struct gpiod_lookup_table isp_ov13b10_gpio_table = {
> +	.dev_id = "i2c-ov13b10",
> +	.table = {
> +		GPIO_LOOKUP("amdisp-pinctrl", 0, "reset", GPIO_ACTIVE_LOW),
> +		{ }
> +	},
> +};
> +
>  static int isp_poweroff(struct generic_pm_domain *genpd)
>  {
>  	struct amdgpu_isp *isp = container_of(genpd, struct amdgpu_isp, ispgpd);
> @@ -240,10 +248,13 @@ static int isp_v4_1_1_hw_init(struct amdgpu_isp *isp)
>  		return 0;
>  	}
>  
> -	/* add GPIO resources required for OMNI5C10 sensor */
> +	/* add GPIO resources required for the sensor */
>  	if (!strcmp("OMNI5C10", acpi_device_hid(acpi_dev))) {
>  		gpiod_add_lookup_table(&isp_gpio_table);
>  		gpiod_add_lookup_table(&isp_sensor_gpio_table);
> +	} else if (!strcmp("OMNI13B1", acpi_device_hid(acpi_dev))) {
> +		gpiod_add_lookup_table(&isp_gpio_table);
> +		gpiod_add_lookup_table(&isp_ov13b10_gpio_table);
>  	}

[Severity: High]
When isp_v4_1_1_hw_init() matches OMNI13B1 (or OMNI5C10), it links the static
isp_gpio_table and isp_ov13b10_gpio_table structures into the global
gpio_lookup_list via gpiod_add_lookup_table().

However, neither the error paths in isp_v4_1_1_hw_init() (if pm_genpd_init(),
memory allocations, or mfd_add_hotplug_devices() fail) nor
isp_v4_1_1_hw_fini() removes these tables:

drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c:isp_v4_1_1_hw_fini() {
	device_for_each_child(isp->parent, NULL,
			      isp_genpd_remove_device);

	mfd_remove_devices(isp->parent);

	kfree(isp->isp_res);
	kfree(isp->isp_cell);
	kfree(isp->isp_pdata);
	kfree(isp->isp_i2c_res);
	kfree(isp->isp_gpio_res);

	return 0;
}

If the PCI device is unbound and rebound, or if isp_v4_1_1_hw_init() is
retried after a failure, won't calling gpiod_add_lookup_table() again on the
already-linked static list_head nodes corrupt gpio_lookup_list? Furthermore,
if amdgpu is built as a module and unloaded, won't leaving isp_gpio_table and
isp_ov13b10_gpio_table linked in gpio_lookup_list cause a use-after-free in
gpiolib when subsequent GPIO lookups traverse the freed module memory?

Should isp_v4_1_1_hw_init() on failure and isp_v4_1_1_hw_fini() on teardown
call gpiod_remove_lookup_table() to unregister the lookup tables?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103155313.114347-1-saif.elden.hatim@gmail.com?part=3