linux-kernel - Re: [RFC PATCH 1/6] ACPI/IORT: Set PMCG device parent

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: <c7bfc6ac-71a1-50d2-e18f-4d352837b93b@huawei.com>
Date:   Tue, 15 Oct 2019 10:07:54 +0100
From:   John Garry <john.garry@...wei.com>
To:     Hanjun Guo <guohanjun@...wei.com>, <lorenzo.pieralisi@....com>,
        <sudeep.holla@....com>, <robin.murphy@....com>,
        <mark.rutland@....com>, <will@...nel.org>
CC:     <shameerali.kolothum.thodi@...wei.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <iommu@...ts.linux-foundation.org>,
        <rjw@...ysocki.net>, <lenb@...nel.org>, <nleeder@...eaurora.org>,
        <linuxarm@...wei.com>
Subject: Re: [RFC PATCH 1/6] ACPI/IORT: Set PMCG device parent

Hi Hanjun,

Thanks for checking this.

>>   */
>>  static int __init iort_add_platform_device(struct acpi_iort_node *node,
>> -					   const struct iort_dev_config *ops)
>> +					   const struct iort_dev_config *ops, struct device *parent)
>
> Since you added a input for this function, could you please update
> the comments of this function as well?

Right, that can be updated. Indeed, the current comment omit the @ops 
argument also.

>
>>  {
>>  	struct fwnode_handle *fwnode;
>>  	struct platform_device *pdev;
>> @@ -1466,6 +1466,8 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node,
>>  	if (!pdev)
>>  		return -ENOMEM;
>>
>> +	pdev->dev.parent = parent;
>> +
>>  	if (ops->dev_set_proximity) {
>>  		ret = ops->dev_set_proximity(&pdev->dev, node);
>>  		if (ret)
>> @@ -1573,6 +1575,11 @@ static void __init iort_enable_acs(struct acpi_iort_node *iort_node)
>>  static inline void iort_enable_acs(struct acpi_iort_node *iort_node) { }
>>  #endif
>>
>> +static int iort_fwnode_match(struct device *dev, const void *fwnode)
>> +{
>> +	return dev->fwnode == fwnode;
>> +}
>> +
>>  static void __init iort_init_platform_devices(void)
>>  {
>>  	struct acpi_iort_node *iort_node, *iort_end;
>> @@ -1594,11 +1601,34 @@ static void __init iort_init_platform_devices(void)
>>  				iort_table->length);
>>
>>  	for (i = 0; i < iort->node_count; i++) {
>> +		struct device *parent = NULL;
>> +
>>  		if (iort_node >= iort_end) {
>>  			pr_err("iort node pointer overflows, bad table\n");
>>  			return;
>>  		}
>>
>> +		/* Fixme: handle parent declared in IORT after PMCG */
>> +		if (iort_node->type == ACPI_IORT_NODE_PMCG) {
>> +			struct acpi_iort_node *iort_assoc_node;
>> +			struct acpi_iort_pmcg *pmcg;
>> +			u32 node_reference;
>> +
>> +			pmcg = (struct acpi_iort_pmcg *)iort_node->node_data;
>> +
>> +			node_reference = pmcg->node_reference;
>> +			iort_assoc_node = ACPI_ADD_PTR(struct acpi_iort_node, iort,
>> +				 node_reference);
>> +
>> +			if (iort_assoc_node->type == ACPI_IORT_NODE_SMMU_V3) {
>> +				struct fwnode_handle *assoc_fwnode;
>> +
>> +				assoc_fwnode = iort_get_fwnode(iort_assoc_node);
>> +
>> +				parent = bus_find_device(&platform_bus_type, NULL,
>> +				      assoc_fwnode, iort_fwnode_match);
>> +			}
>> +		}
>
> How about using a function to include those new added code to make this
> function (iort_init_platform_devices()) a bit cleaner?
>

Can do. But I still need to add code to deal with the scenario of the 
parent PMCG not being an SMMU, which is supported in the spec as I recall.

Note that I would not have FW to test that, so maybe can omit support 
for now as long as there's no regression.

>>  		iort_enable_acs(iort_node);
>>
>>  		ops = iort_get_dev_cfg(iort_node);
>> @@ -1609,7 +1639,7 @@ static void __init iort_init_platform_devices(void)
>>
>>  			iort_set_fwnode(iort_node, fwnode);
>>
>> -			ret = iort_add_platform_device(iort_node, ops);
>> +			ret = iort_add_platform_device(iort_node, ops, parent);
>
> This function is called if ops is valid, so retrieve the parent
> can be done before this function I think.

Ah, yes

Thanks,
John

>

> Thanks
> Hanjun
>
>
> .
>