linux-kernel - Re: [PATCH v2] software node: Implement device_get_match

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: <Zie3ebHOEpWHj1qV@smile.fi.intel.com>
Date: Tue, 23 Apr 2024 16:28:25 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Sui Jingfeng <sui.jingfeng@...ux.dev>
Cc: dri-devel@...ts.freedesktop.org, linux-acpi@...r.kernel.org,
	linux-kernel@...r.kernel.org, Daniel Scally <djrscally@...il.com>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH v2] software node: Implement device_get_match_data fwnode
 callback

On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote:
> Because the software node backend of the fwnode API framework lacks an
> implementation for the .device_get_match_data function callback. This
> makes it difficult to use(and/or test) a few drivers that originates

Missing space before opening parenthesis.

> from DT world on the non-DT platform.
> 
> Implement the .device_get_match_data fwnode callback, device drivers or
> platform setup codes are expected to provide a string property, named as
> "compatible", the value of this software node string property is used to
> match against the compatible entries in the of_device_id table.

Yep and again, how is this related? If you want to test a driver originating
from DT, you would probably want to have a DT (overlay) to be provided.

> This also helps to keep the three backends of the fwnode API aligned as
> much as possible, which is a fundamential step to make device driver
> OF-independent truely possible.
> 
> Fixes: ffb42e64561e ("drm/tiny/repaper: Make driver OF-independent")
> Fixes: 5703d6ae9573 ("drm/tiny/st7735r: Make driver OF-independent")

How is it a fix?

> Closes: https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/

Yes, and then Reported-by, which is missing here.

> Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> Cc: Daniel Scally <djrscally@...il.com>
> Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@...nel.org>

Please, move these after the cutter '---' line (note you may have that line in
your local repo).

..

> +static const void *
> +software_node_get_match_data(const struct fwnode_handle *fwnode,
> +			     const struct device *dev)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	const struct of_device_id *matches = dev->driver->of_match_table;
> +	const char *val = NULL;
> +	int ret;

> +	ret = property_entry_read_string_array(swnode->node->properties,
> +					       "compatible", &val, 1);

And if there are more than one compatible provided?

> +	if (ret < 0 || !val)
> +		return NULL;

> +	while (matches && matches->compatible[0]) {

First part of the conditional is invariant to the loop. Can be simply

	matches = dev->driver->of_match_table;
	if (!matches)
		return NULL;

	while (...)

> +		if (!strcmp(matches->compatible, val))
> +			return matches->data;
> +
> +		matches++;
> +	}
> +
> +	return NULL;
> +}

-- 
With Best Regards,
Andy Shevchenko