linux-kernel - Re: [PATCH v3 3/3] platform/surface: Add OF support

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: <d08d41ad-edcb-48ad-a848-53edc45ab8eb@gmail.com>
Date: Wed, 28 Aug 2024 11:10:42 +0200
From: Maximilian Luz <luzmaximilian@...il.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>,
 Konrad Dybcio <konradybcio@...nel.org>
Cc: Rob Herring <robh@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Jiri Slaby <jirislaby@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
 Len Brown <lenb@...nel.org>, Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Marijn Suijten <marijn.suijten@...ainline.org>,
 linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, linux-acpi@...r.kernel.org,
 platform-driver-x86@...r.kernel.org, Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <quic_kdybcio@...cinc.com>
Subject: Re: [PATCH v3 3/3] platform/surface: Add OF support

Hi,

I thought I should provide some context:

Am 26/08/2024 um 22:54 schrieb Andy Shevchenko:
> Wed, Aug 14, 2024 at 12:27:27PM +0200, Konrad Dybcio kirjoitti:
>> From: Konrad Dybcio <quic_kdybcio@...cinc.com>

[...]

>> +	/*
>> +	 * When using DT, we have to register the platform hub driver manually,
>> +	 * as it can't be matched based on top-level board compatible (like it
>> +	 * does the ACPI case).
>> +	 */
>> +	if (!ssh) {
>> +		struct platform_device *ph_pdev =
>> +			platform_device_register_simple("surface_aggregator_platform_hub",
>> +							0, NULL, 0);
>> +		if (IS_ERR(ph_pdev))
>> +			return dev_err_probe(dev, PTR_ERR(ph_pdev),
>> +					     "Failed to register the platform hub driver\n"); 
>> +	}

[...]

>>   static int ssam_platform_hub_probe(struct platform_device *pdev)
>>   {
>>   	const struct software_node **nodes;
>> +	const struct of_device_id *match;
>> +	struct device_node *fdt_root;
>>   	struct ssam_controller *ctrl;
>>   	struct fwnode_handle *root;
>>   	int status;
>>   
>>   	nodes = (const struct software_node **)acpi_device_get_match_data(&pdev->dev);
> 
> Hmm... Why this doesn't use simple device_get_match_data()?
> 
>> -	if (!nodes)
>> -		return -ENODEV;
>> +	if (!nodes) {
>> +		fdt_root = of_find_node_by_path("/");
>> +		if (!fdt_root)
>> +			return -ENODEV;
>> +
>> +		match = of_match_node(ssam_platform_hub_of_match, fdt_root);
>> +		of_node_put(fdt_root);
>> +		if (!match)
>> +			return -ENODEV;
>> +
>> +		nodes = (const struct software_node **)match->data;
> 
> This is quite strange! Where are they being defined?

Essentially, this whole module is a giant workaround because there
doesn't seem to be a way to auto-discover which functions or subdevices
the EC actually supports. So this module builds a registry of software
nodes and matches against a Surface-model-specific ACPI ID (in ACPI
mode). Based on that ID, we retrieve the tree of software nodes that
define the EC subdevices and register them using a (virtual) platform
hub device.

The snippet way above registers the platform hub device for DT,
because there we don't have an equivalent ACPI device that we can
use. The code here retrieves the respective nodes.

>> +		if (!nodes)
>> +			return -ENODEV;
>> +	}
> 
> ...
> 
>> +MODULE_ALIAS("platform:surface_aggregator_platform_hub");
> 
> Can it be platfrom device ID table instead? But do you really need it?
> 

I think the explanation above already kind of answers this, but the
module is named differently than the driver (so that they reflect the
specific nature of each, registry vs hub device). And the platform hub
device added in the snippet I left above is named after the driver. So
for the registry module to load when the platform hub driver is
requested, it is needed.

Regards,
Max