linux-kernel - Re: [PATCH v1 2/2] media: atomisp: Replace open-coded i2c_find_device_by

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c9741df0-6c12-4ff5-90b4-5a13f0bd9b51@redhat.com>
Date: Wed, 10 Apr 2024 12:35:29 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 linux-media@...r.kernel.org, linux-staging@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v1 2/2] media: atomisp: Replace open-coded
 i2c_find_device_by_fwnode()

Hi Andy,

Thank you for the patch.

On 3/26/24 9:27 PM, Andy Shevchenko wrote:
> gmin_i2c_dev_exists() is using open-coded variant of
> i2c_find_device_by_fwnode(). Replace it with a corresponding call.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
>  drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> index 80aa2211cdc3..b7c477280986 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> @@ -394,12 +394,10 @@ static struct i2c_client *gmin_i2c_dev_exists(struct device *dev, char *name,
>  	if (!adev)
>  		return NULL;
>  
> -	d = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);
> +	d = get_device(acpi_get_first_physical_node(adev));
>  	acpi_dev_put(adev);
> -	if (!d)
> -		return NULL;
>  
> -	*client = i2c_verify_client(d);
> +	*client = i2c_find_device_by_fwnode(dev_fwnode(d));
>  	put_device(d);
>  
>  	dev_dbg(dev, "found '%s' at address 0x%02x, adapter %d\n",

If we are ging to simplify this I think the change should be:

>From ea66d15b9a72fcb8baf22a2ff059f2b842a91b67 Mon Sep 17 00:00:00 2001
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Date: Wed, 10 Apr 2024 12:32:11 +0200
Subject: [PATCH] media: atomisp: Replace open-coded
 i2c_find_device_by_fwnode()

gmin_i2c_dev_exists() is using open-coded variant of
i2c_find_device_by_fwnode(). Replace it with a corresponding call.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@...hat.com>
---
Changes in v2:
- Directly use the ACPI fwnode through acpi_fwnode_handle(adev) instead
  of first calling acpi_get_first_physical_node(adev)
---
 drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 804ffff245f3..ed6214327ce5 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -394,14 +394,11 @@ static struct i2c_client *gmin_i2c_dev_exists(struct device *dev, char *name,
 	if (!adev)
 		return NULL;
 
-	d = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);
+	*client = i2c_find_device_by_fwnode(acpi_fwnode_handle(adev));
 	acpi_dev_put(adev);
-	if (!d)
+	if (!*client)
 		return NULL;
 
-	*client = i2c_verify_client(d);
-	put_device(d);
-
 	dev_dbg(dev, "found '%s' at address 0x%02x, adapter %d\n",
 		(*client)->name, (*client)->addr, (*client)->adapter->nr);
 	return *client;
-- 
2.44.0

Note I added a:

	if (!*client)
		return NULL;

To fix a bug in the original code where the dev_dbg() arguments
are dereferencing a potential NULL *client ptr.

I'm going to merge this variant into my media-atomisp branch
instead of the orignal.

Regards,

Hans