[PATCH v2 0/2] Ignore bad graph port nodes on Dell XPS 9315

[PATCH v2 0/2] Ignore bad graph port nodes on Dell XPS 9315

[Sakari Ailus]

Hi Rafael,

These two patches skip instantiating bad port nodes on Dell XPS 9315.

The ACPI firmware contains Linux specific camera declarations but they are
missing the IVSC which sits between the camera and the IPU. The IPU bridge
can be instead used to obtain a configuration that is usable in the
system.

I presume there are other models that have the same issue.

since v1:

- Use a better subject for the second patch.

- Don't explicitly initialise static variable (second patch).

- Improve the commit message of the second patch slightly.

Sakari Ailus (2):
  ACPI: utils: Make acpi_handle_path() not static
  ACPI: property: Ignore bad graph port nodes on Dell XPS 9315

 drivers/acpi/internal.h       |  1 +
 drivers/acpi/mipi-disco-img.c | 71 +++++++++++++++++++++++++++++++++++
 drivers/acpi/property.c       |  3 ++
 drivers/acpi/utils.c          |  2 +-
 include/linux/acpi.h          |  1 +
 5 files changed, 77 insertions(+), 1 deletion(-)

-- 
2.39.2

[PATCH v2 1/2] ACPI: utils: Make acpi_handle_path() not static

[Sakari Ailus]

acpi_handle_path() will soon be required for node name comparison
elsewhere in ACPI framework. Remove the static keyword and add the
prototype to include/linux/acpi.h.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/utils.c | 2 +-
 include/linux/acpi.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index abac5cc25477..202234ba54bd 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -559,7 +559,7 @@ EXPORT_SYMBOL(acpi_evaluate_ost);
  *
  * Caller must free the returned buffer
  */
-static char *acpi_handle_path(acpi_handle handle)
+char *acpi_handle_path(acpi_handle handle)
 {
 	struct acpi_buffer buffer = {
 		.length = ACPI_ALLOCATE_BUFFER,
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index b7165e52b3c6..a170c389dd74 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1170,6 +1170,7 @@ static inline void acpi_ec_set_gpe_wake_mask(u8 action) {}
 #endif
 
 #ifdef CONFIG_ACPI
+char *acpi_handle_path(acpi_handle handle);
 __printf(3, 4)
 void acpi_handle_printk(const char *level, acpi_handle handle,
 			const char *fmt, ...);
-- 
2.39.2

[PATCH v2 2/2] ACPI: property: Ignore bad graph port nodes on Dell XPS 9315

[Sakari Ailus]

Some systems were shipped with both Windows and Linux camera descriptions.
In general, if Linux description exist, they will be used and Windows
description ignored.

In this case the Linux descriptions were buggy so use Windows definition
instead. This patch ignores the bad graph port nodes on Dell XPS 9315 and
there are likely other such systems, too. The corresponding information
has been added to ipu-bridge to cover the missing bits.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/internal.h       |  1 +
 drivers/acpi/mipi-disco-img.c | 71 +++++++++++++++++++++++++++++++++++
 drivers/acpi/property.c       |  3 ++
 3 files changed, 75 insertions(+)

diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 6588525c45ef..e0145df519bd 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -301,5 +301,6 @@ void acpi_mipi_check_crs_csi2(acpi_handle handle);
 void acpi_mipi_scan_crs_csi2(void);
 void acpi_mipi_init_crs_csi2_swnodes(void);
 void acpi_mipi_crs_csi2_cleanup(void);
+bool acpi_graph_ignore_port(acpi_handle handle);
 
 #endif /* _ACPI_INTERNAL_H_ */
diff --git a/drivers/acpi/mipi-disco-img.c b/drivers/acpi/mipi-disco-img.c
index 7286cf4579bc..da71eb4bf6a6 100644
--- a/drivers/acpi/mipi-disco-img.c
+++ b/drivers/acpi/mipi-disco-img.c
@@ -19,6 +19,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/dmi.h>
 #include <linux/limits.h>
 #include <linux/list.h>
 #include <linux/module.h>
@@ -723,3 +724,73 @@ void acpi_mipi_crs_csi2_cleanup(void)
 	list_for_each_entry_safe(csi2, csi2_tmp, &acpi_mipi_crs_csi2_list, entry)
 		acpi_mipi_del_crs_csi2(csi2);
 }
+
+static const struct dmi_system_id dmi_ignore_port_nodes[] = {
+	{
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "XPS 9315"),
+		},
+	},
+	{ 0 }
+};
+
+static const char *strnext(const char *s1, const char *s2)
+{
+	s1 = strstr(s1, s2);
+
+	if (!s1)
+		return NULL;
+
+	return s1 + strlen(s2);
+}
+
+/**
+ * acpi_graph_ignore_port - Tell whether a port node should be ignored
+ * @handle: The ACPI handle of the node (which may be a port node)
+ *
+ * Returns true if a port node should be ignored and the data to that should
+ * come from other sources instead (Windows ACPI definitions and
+ * ipu-bridge). This is currently used to ignore bad port nodes related to IPU6
+ * ("IPU?") and camera sensor devices ("LNK?") in certain Dell systems with
+ * Intel VSC.
+ */
+bool acpi_graph_ignore_port(acpi_handle handle)
+{
+	const char *path = NULL, *orig_path;
+	static bool dmi_tested, ignore_port;
+
+	if (!dmi_tested) {
+		ignore_port = dmi_first_match(dmi_ignore_port_nodes);
+		dmi_tested = true;
+	}
+
+	if (!ignore_port)
+		return false;
+
+	/* Check if the device is either "IPU" or "LNK" (sensor). */
+	orig_path = acpi_handle_path(handle);
+	if (!orig_path)
+		return false;
+	path = strnext(orig_path, "IPU");
+	if (!path)
+		path = strnext(orig_path, "LNK");
+	if (!path)
+		goto out_free;
+
+	if (!(path[0] >= '0' && path[0] <= '9' && path[1] == '.'))
+		goto out_free;
+
+	/* Check if the node has a "PRT" prefix. */
+	path = strnext(path, "PRT");
+	if (path && path[0] >= '0' && path[0] <= '9' && !path[1]) {
+		acpi_handle_debug(handle, "ignoring data node\n");
+
+		kfree(orig_path);
+		return true;
+	}
+
+out_free:
+	kfree(orig_path);
+	return false;
+}
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index a6ead5204046..2b73580c9f36 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -80,6 +80,9 @@ static bool acpi_nondev_subnode_extract(union acpi_object *desc,
 	struct acpi_data_node *dn;
 	bool result;
 
+	if (acpi_graph_ignore_port(handle))
+		return false;
+
 	dn = kzalloc(sizeof(*dn), GFP_KERNEL);
 	if (!dn)
 		return false;
-- 
2.39.2

Re: [PATCH v2 2/2] ACPI: property: Ignore bad graph port nodes on Dell XPS 9315

[Rafael J. Wysocki]

On Tue, Feb 13, 2024 at 2:46 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Some systems were shipped with both Windows and Linux camera descriptions.
> In general, if Linux description exist, they will be used and Windows
> description ignored.
>
> In this case the Linux descriptions were buggy so use Windows definition
> instead. This patch ignores the bad graph port nodes on Dell XPS 9315 and
> there are likely other such systems, too. The corresponding information
> has been added to ipu-bridge to cover the missing bits.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/acpi/internal.h       |  1 +
>  drivers/acpi/mipi-disco-img.c | 71 +++++++++++++++++++++++++++++++++++
>  drivers/acpi/property.c       |  3 ++
>  3 files changed, 75 insertions(+)
>
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 6588525c45ef..e0145df519bd 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -301,5 +301,6 @@ void acpi_mipi_check_crs_csi2(acpi_handle handle);
>  void acpi_mipi_scan_crs_csi2(void);
>  void acpi_mipi_init_crs_csi2_swnodes(void);
>  void acpi_mipi_crs_csi2_cleanup(void);
> +bool acpi_graph_ignore_port(acpi_handle handle);
>
>  #endif /* _ACPI_INTERNAL_H_ */
> diff --git a/drivers/acpi/mipi-disco-img.c b/drivers/acpi/mipi-disco-img.c
> index 7286cf4579bc..da71eb4bf6a6 100644
> --- a/drivers/acpi/mipi-disco-img.c
> +++ b/drivers/acpi/mipi-disco-img.c
> @@ -19,6 +19,7 @@
>   */
>
>  #include <linux/acpi.h>
> +#include <linux/dmi.h>
>  #include <linux/limits.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
> @@ -723,3 +724,73 @@ void acpi_mipi_crs_csi2_cleanup(void)
>         list_for_each_entry_safe(csi2, csi2_tmp, &acpi_mipi_crs_csi2_list, entry)
>                 acpi_mipi_del_crs_csi2(csi2);
>  }
> +
> +static const struct dmi_system_id dmi_ignore_port_nodes[] = {
> +       {
> +               .matches = {
> +                       DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +                       DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "XPS 9315"),
> +               },
> +       },
> +       { 0 }
> +};
> +
> +static const char *strnext(const char *s1, const char *s2)
> +{
> +       s1 = strstr(s1, s2);
> +
> +       if (!s1)
> +               return NULL;
> +
> +       return s1 + strlen(s2);
> +}
> +
> +/**
> + * acpi_graph_ignore_port - Tell whether a port node should be ignored
> + * @handle: The ACPI handle of the node (which may be a port node)
> + *
> + * Returns true if a port node should be ignored and the data to that should
> + * come from other sources instead (Windows ACPI definitions and
> + * ipu-bridge). This is currently used to ignore bad port nodes related to IPU6
> + * ("IPU?") and camera sensor devices ("LNK?") in certain Dell systems with
> + * Intel VSC.
> + */
> +bool acpi_graph_ignore_port(acpi_handle handle)
> +{
> +       const char *path = NULL, *orig_path;
> +       static bool dmi_tested, ignore_port;
> +
> +       if (!dmi_tested) {
> +               ignore_port = dmi_first_match(dmi_ignore_port_nodes);
> +               dmi_tested = true;
> +       }
> +
> +       if (!ignore_port)
> +               return false;
> +
> +       /* Check if the device is either "IPU" or "LNK" (sensor). */
> +       orig_path = acpi_handle_path(handle);
> +       if (!orig_path)
> +               return false;
> +       path = strnext(orig_path, "IPU");
> +       if (!path)
> +               path = strnext(orig_path, "LNK");
> +       if (!path)
> +               goto out_free;
> +
> +       if (!(path[0] >= '0' && path[0] <= '9' && path[1] == '.'))
> +               goto out_free;
> +
> +       /* Check if the node has a "PRT" prefix. */
> +       path = strnext(path, "PRT");
> +       if (path && path[0] >= '0' && path[0] <= '9' && !path[1]) {
> +               acpi_handle_debug(handle, "ignoring data node\n");
> +
> +               kfree(orig_path);
> +               return true;
> +       }
> +
> +out_free:
> +       kfree(orig_path);
> +       return false;
> +}
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index a6ead5204046..2b73580c9f36 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -80,6 +80,9 @@ static bool acpi_nondev_subnode_extract(union acpi_object *desc,
>         struct acpi_data_node *dn;
>         bool result;
>
> +       if (acpi_graph_ignore_port(handle))
> +               return false;
> +
>         dn = kzalloc(sizeof(*dn), GFP_KERNEL);
>         if (!dn)
>                 return false;
> --

Applied as 6.9 material along with the [1/2], thanks!

Re: [PATCH v2 2/2] ACPI: property: Ignore bad graph port nodes on Dell XPS 9315

[andy.shevchenko]

Tue, Feb 13, 2024 at 03:46:06PM +0200, Sakari Ailus kirjoitti:
> Some systems were shipped with both Windows and Linux camera descriptions.
> In general, if Linux description exist, they will be used and Windows
> description ignored.
> 
> In this case the Linux descriptions were buggy so use Windows definition
> instead. This patch ignores the bad graph port nodes on Dell XPS 9315 and
> there are likely other such systems, too. The corresponding information
> has been added to ipu-bridge to cover the missing bits.

...

>  void acpi_mipi_scan_crs_csi2(void);
>  void acpi_mipi_init_crs_csi2_swnodes(void);
>  void acpi_mipi_crs_csi2_cleanup(void);

+ blank line?

> +bool acpi_graph_ignore_port(acpi_handle handle);
>  
>  #endif /* _ACPI_INTERNAL_H_ */

...

> +static const struct dmi_system_id dmi_ignore_port_nodes[] = {
> +	{
> +		.matches = {
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "XPS 9315"),
> +		},
> +	},
> +	{ 0 }

0 is not needed. Moreover, it's a bit unusual.

> +};

...

> +static const char *strnext(const char *s1, const char *s2)
> +{
> +	s1 = strstr(s1, s2);
> +
> +	if (!s1)
> +		return NULL;
> +
> +	return s1 + strlen(s2);
> +}

NIH str_has_prefix() ?

...

> +/**
> + * acpi_graph_ignore_port - Tell whether a port node should be ignored
> + * @handle: The ACPI handle of the node (which may be a port node)
> + *
> + * Returns true if a port node should be ignored and the data to that should

Please, run kernel-doc validation and fix the warnings.

> + * come from other sources instead (Windows ACPI definitions and
> + * ipu-bridge). This is currently used to ignore bad port nodes related to IPU6
> + * ("IPU?") and camera sensor devices ("LNK?") in certain Dell systems with
> + * Intel VSC.
> + */
> +bool acpi_graph_ignore_port(acpi_handle handle)
> +{
> +	const char *path = NULL, *orig_path;
> +	static bool dmi_tested, ignore_port;
> +
> +	if (!dmi_tested) {
> +		ignore_port = dmi_first_match(dmi_ignore_port_nodes);
> +		dmi_tested = true;
> +	}
> +
> +	if (!ignore_port)
> +		return false;
> +
> +	/* Check if the device is either "IPU" or "LNK" (sensor). */
> +	orig_path = acpi_handle_path(handle);
> +	if (!orig_path)
> +		return false;
> +	path = strnext(orig_path, "IPU");

IIUC this can be rewritten as

	path += str_has_prefix();
	if (path == orig_path)
		...

> +	if (!path)
> +		path = strnext(orig_path, "LNK");
> +	if (!path)
> +		goto out_free;
> +
> +	if (!(path[0] >= '0' && path[0] <= '9' && path[1] == '.'))

isdigit() ?

> +		goto out_free;
> +
> +	/* Check if the node has a "PRT" prefix. */
> +	path = strnext(path, "PRT");
> +	if (path && path[0] >= '0' && path[0] <= '9' && !path[1]) {

Ditto.

> +		acpi_handle_debug(handle, "ignoring data node\n");
> +
> +		kfree(orig_path);
> +		return true;
> +	}
> +
> +out_free:
> +	kfree(orig_path);
> +	return false;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] ACPI: property: Ignore bad graph port nodes on Dell XPS 9315

[Sakari Ailus]

Hi Andy,

Thanks for the review.

I think Rafael has already applied this but I can send a new patch...

On Wed, Feb 21, 2024 at 12:21:13AM +0200, andy.shevchenko@gmail.com wrote:
> Tue, Feb 13, 2024 at 03:46:06PM +0200, Sakari Ailus kirjoitti:
> > Some systems were shipped with both Windows and Linux camera descriptions.
> > In general, if Linux description exist, they will be used and Windows
> > description ignored.
> > 
> > In this case the Linux descriptions were buggy so use Windows definition
> > instead. This patch ignores the bad graph port nodes on Dell XPS 9315 and
> > there are likely other such systems, too. The corresponding information
> > has been added to ipu-bridge to cover the missing bits.
> 
> ...
> 
> >  void acpi_mipi_scan_crs_csi2(void);
> >  void acpi_mipi_init_crs_csi2_swnodes(void);
> >  void acpi_mipi_crs_csi2_cleanup(void);
> 
> + blank line?

The usa of blank lines elsewhere in the file is consistent with the above.
These are all related.

> 
> > +bool acpi_graph_ignore_port(acpi_handle handle);
> >  
> >  #endif /* _ACPI_INTERNAL_H_ */
> 
> ...
> 
> > +static const struct dmi_system_id dmi_ignore_port_nodes[] = {
> > +	{
> > +		.matches = {
> > +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "XPS 9315"),
> > +		},
> > +	},
> > +	{ 0 }
> 
> 0 is not needed. Moreover, it's a bit unusual.

But also required by the C standard.
 
> > +};
> 
> ...
> 
> > +static const char *strnext(const char *s1, const char *s2)
> > +{
> > +	s1 = strstr(s1, s2);
> > +
> > +	if (!s1)
> > +		return NULL;
> > +
> > +	return s1 + strlen(s2);
> > +}
> 
> NIH str_has_prefix() ?

It doesn't do the same thing. Yes, it could be used, but the code looks
cleaner with this.

> 
> ...
> 
> > +/**
> > + * acpi_graph_ignore_port - Tell whether a port node should be ignored
> > + * @handle: The ACPI handle of the node (which may be a port node)
> > + *
> > + * Returns true if a port node should be ignored and the data to that should
> 
> Please, run kernel-doc validation and fix the warnings.

Running kernel-doc on the file doesn't produce any here.

> 
> > + * come from other sources instead (Windows ACPI definitions and
> > + * ipu-bridge). This is currently used to ignore bad port nodes related to IPU6
> > + * ("IPU?") and camera sensor devices ("LNK?") in certain Dell systems with
> > + * Intel VSC.
> > + */
> > +bool acpi_graph_ignore_port(acpi_handle handle)
> > +{
> > +	const char *path = NULL, *orig_path;
> > +	static bool dmi_tested, ignore_port;
> > +
> > +	if (!dmi_tested) {
> > +		ignore_port = dmi_first_match(dmi_ignore_port_nodes);
> > +		dmi_tested = true;
> > +	}
> > +
> > +	if (!ignore_port)
> > +		return false;
> > +
> > +	/* Check if the device is either "IPU" or "LNK" (sensor). */
> > +	orig_path = acpi_handle_path(handle);
> > +	if (!orig_path)
> > +		return false;
> > +	path = strnext(orig_path, "IPU");
> 
> IIUC this can be rewritten as
> 
> 	path += str_has_prefix();
> 	if (path == orig_path)

You could do that here, but now below, without introducing a new temporary
variable and shuffling that and "path". I prefer to keep it as-is.

> 		...
> 
> > +	if (!path)
> > +		path = strnext(orig_path, "LNK");
> > +	if (!path)
> > +		goto out_free;
> > +
> > +	if (!(path[0] >= '0' && path[0] <= '9' && path[1] == '.'))
> 
> isdigit() ?

Makes sense.

> 
> > +		goto out_free;
> > +
> > +	/* Check if the node has a "PRT" prefix. */
> > +	path = strnext(path, "PRT");
> > +	if (path && path[0] >= '0' && path[0] <= '9' && !path[1]) {
> 
> Ditto.

Yes.

> 
> > +		acpi_handle_debug(handle, "ignoring data node\n");
> > +
> > +		kfree(orig_path);
> > +		return true;
> > +	}
> > +
> > +out_free:
> > +	kfree(orig_path);
> > +	return false;
> > +}
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 2/2] ACPI: property: Ignore bad graph port nodes on Dell XPS 9315

[Andy Shevchenko]

On Wed, Feb 21, 2024 at 8:56 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> I think Rafael has already applied this but I can send a new patch...

Depends if it's final or can be dropped.
If the former is the case, follow ups, please.

> On Wed, Feb 21, 2024 at 12:21:13AM +0200, andy.shevchenko@gmail.com wrote:
> > Tue, Feb 13, 2024 at 03:46:06PM +0200, Sakari Ailus kirjoitti:

...

> > >  void acpi_mipi_scan_crs_csi2(void);
> > >  void acpi_mipi_init_crs_csi2_swnodes(void);
> > >  void acpi_mipi_crs_csi2_cleanup(void);
> >
> > + blank line?
>
> The usa of blank lines elsewhere in the file is consistent with the above.
> These are all related.

Naming does not suggest that. I see either naming or semantic tights
issues here. Hence the proposal to have a blank line.

> > > +bool acpi_graph_ignore_port(acpi_handle handle);

...

> > > +static const struct dmi_system_id dmi_ignore_port_nodes[] = {
> > > +   {
> > > +           .matches = {
> > > +                   DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > > +                   DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "XPS 9315"),
> > > +           },
> > > +   },
> > > +   { 0 }
> >
> > 0 is not needed. Moreover, it's a bit unusual.
>
> But also required by the C standard.

We have a lot of code that doesn't use that (yet this is valid C23,
and yes I know that we rely on C11).

> > > +};

...

> > > +static const char *strnext(const char *s1, const char *s2)
> > > +{
> > > +   s1 = strstr(s1, s2);
> > > +
> > > +   if (!s1)
> > > +           return NULL;
> > > +
> > > +   return s1 + strlen(s2);
> > > +}
> >
> > NIH str_has_prefix() ?
>
> It doesn't do the same thing. Yes, it could be used, but the code looks
> cleaner with this.

"Not Invented Here" was even at Nokia times, why do we repeat our mistakes?

...

> > > +/**
> > > + * acpi_graph_ignore_port - Tell whether a port node should be ignored
> > > + * @handle: The ACPI handle of the node (which may be a port node)
> > > + *
> > > + * Returns true if a port node should be ignored and the data to that should
> >
> > Please, run kernel-doc validation and fix the warnings.
>
> Running kernel-doc on the file doesn't produce any here.

You haven't run it correctly?

$ scripts/kernel-doc -v -none -Wall drivers/acpi/mipi-disco-img.c
drivers/acpi/mipi-disco-img.c:163: info: Scanning doc for function
acpi_mipi_check_crs_csi2
drivers/acpi/mipi-disco-img.c:384: info: Scanning doc for function
acpi_mipi_scan_crs_csi2
drivers/acpi/mipi-disco-img.c:703: info: Scanning doc for function
acpi_mipi_init_crs_csi2_swnodes
drivers/acpi/mipi-disco-img.c:718: info: Scanning doc for function
acpi_mipi_crs_csi2_cleanup
drivers/acpi/mipi-disco-img.c:749: info: Scanning doc for function
acpi_graph_ignore_port
drivers/acpi/mipi-disco-img.c:759: warning: No description found for
return value of 'acpi_graph_ignore_port'
1 warnings

> > > + * come from other sources instead (Windows ACPI definitions and
> > > + * ipu-bridge). This is currently used to ignore bad port nodes related to IPU6
> > > + * ("IPU?") and camera sensor devices ("LNK?") in certain Dell systems with
> > > + * Intel VSC.
> > > + */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] ACPI: property: Ignore bad graph port nodes on Dell XPS 9315

[Sakari Ailus]

On Wed, Feb 21, 2024 at 05:33:36PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 21, 2024 at 8:56 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> > I think Rafael has already applied this but I can send a new patch...
> 
> Depends if it's final or can be dropped.
> If the former is the case, follow ups, please.
> 
> > On Wed, Feb 21, 2024 at 12:21:13AM +0200, andy.shevchenko@gmail.com wrote:
> > > Tue, Feb 13, 2024 at 03:46:06PM +0200, Sakari Ailus kirjoitti:
> 
> ...
> 
> > > >  void acpi_mipi_scan_crs_csi2(void);
> > > >  void acpi_mipi_init_crs_csi2_swnodes(void);
> > > >  void acpi_mipi_crs_csi2_cleanup(void);
> > >
> > > + blank line?
> >
> > The usa of blank lines elsewhere in the file is consistent with the above.
> > These are all related.
> 
> Naming does not suggest that. I see either naming or semantic tights
> issues here. Hence the proposal to have a blank line.
> 
> > > > +bool acpi_graph_ignore_port(acpi_handle handle);
> 
> ...
> 
> > > > +static const struct dmi_system_id dmi_ignore_port_nodes[] = {
> > > > +   {
> > > > +           .matches = {
> > > > +                   DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > > > +                   DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "XPS 9315"),
> > > > +           },
> > > > +   },
> > > > +   { 0 }
> > >
> > > 0 is not needed. Moreover, it's a bit unusual.
> >
> > But also required by the C standard.
> 
> We have a lot of code that doesn't use that (yet this is valid C23,
> and yes I know that we rely on C11).

The current compilers support it and I guess we'll switch to C23 at some
point. I can drop the 0.

> 
> > > > +};
> 
> ...
> 
> > > > +static const char *strnext(const char *s1, const char *s2)
> > > > +{
> > > > +   s1 = strstr(s1, s2);
> > > > +
> > > > +   if (!s1)
> > > > +           return NULL;
> > > > +
> > > > +   return s1 + strlen(s2);
> > > > +}
> > >
> > > NIH str_has_prefix() ?
> >
> > It doesn't do the same thing. Yes, it could be used, but the code looks
> > cleaner with this.
> 
> "Not Invented Here" was even at Nokia times, why do we repeat our mistakes?

As I noted previously, while str_has_prefix() could be used, it would make
the code in the caller harder to read. Changes may be needed to that code
later on as this isn't the only system with the issue so readability does
count.

> 
> ...
> 
> > > > +/**
> > > > + * acpi_graph_ignore_port - Tell whether a port node should be ignored
> > > > + * @handle: The ACPI handle of the node (which may be a port node)
> > > > + *
> > > > + * Returns true if a port node should be ignored and the data to that should
> > >
> > > Please, run kernel-doc validation and fix the warnings.
> >
> > Running kernel-doc on the file doesn't produce any here.
> 
> You haven't run it correctly?
> 
> $ scripts/kernel-doc -v -none -Wall drivers/acpi/mipi-disco-img.c
> drivers/acpi/mipi-disco-img.c:163: info: Scanning doc for function
> acpi_mipi_check_crs_csi2
> drivers/acpi/mipi-disco-img.c:384: info: Scanning doc for function
> acpi_mipi_scan_crs_csi2
> drivers/acpi/mipi-disco-img.c:703: info: Scanning doc for function
> acpi_mipi_init_crs_csi2_swnodes
> drivers/acpi/mipi-disco-img.c:718: info: Scanning doc for function
> acpi_mipi_crs_csi2_cleanup
> drivers/acpi/mipi-disco-img.c:749: info: Scanning doc for function
> acpi_graph_ignore_port
> drivers/acpi/mipi-disco-img.c:759: warning: No description found for
> return value of 'acpi_graph_ignore_port'
> 1 warnings

Ah, I guess it was -Wall option that was required to produce the warning.
I'll address it.

> 
> > > > + * come from other sources instead (Windows ACPI definitions and
> > > > + * ipu-bridge). This is currently used to ignore bad port nodes related to IPU6
> > > > + * ("IPU?") and camera sensor devices ("LNK?") in certain Dell systems with
> > > > + * Intel VSC.
> > > > + */
> 

-- 
Regards,

Sakari Ailus