linux-kernel - Re: [PATCH v4 5/9] drivers: base: cacheinfo: arm64: Add support for ACPI based firmware tables

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: <c4f8dd3a-75ee-242e-e499-d85710e91b78@arm.com>
Date:   Mon, 20 Nov 2017 18:14:15 +0000
From:   Sudeep Holla <sudeep.holla@....com>
To:     Jeremy Linton <jeremy.linton@....com>
Cc:     Sudeep Holla <sudeep.holla@....com>, linux-acpi@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, hanjun.guo@...aro.org,
        lorenzo.pieralisi@....com, rjw@...ysocki.net, will.deacon@....com,
        catalin.marinas@....com, gregkh@...uxfoundation.org,
        viresh.kumar@...aro.org, mark.rutland@....com,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        jhugo@...eaurora.org, wangxiongfeng2@...wei.com,
        Jonathan.Zhang@...ium.com, ahs3@...hat.com,
        Jayachandran.Nair@...ium.com, austinwc@...eaurora.org,
        lenb@...nel.org, robert.moore@...el.com, lv.zheng@...el.com,
        devel@...ica.org
Subject: Re: [PATCH v4 5/9] drivers: base: cacheinfo: arm64: Add support for
 ACPI based firmware tables



On 20/11/17 18:02, Jeremy Linton wrote:
> On 11/20/2017 10:56 AM, Sudeep Holla wrote:
> 
> (trimming)
> 
>>> *    case there's no explicit cache node or the cache node
>>> itself in the *    device tree + * @firmware_node: Shared with
>>> of_node. When not using DT, this may contain + *    pointers to
>>> other firmware based values. Particularly ACPI/PPTT + *    unique
>>> values. * @disable_sysfs: indicates whether this node is visible
>>> to the user via *    sysfs or not * @priv: pointer to any private
>>> data structure specific to particular @@ -64,8 +67,10 @@ struct
>>> cacheinfo { #define CACHE_ALLOCATE_POLICY_MASK    \ 
>>> (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE) #define CACHE_ID
>>> BIT(4) - -    struct device_node *of_node; +    union { +
>>> struct device_node *of_node; +        void *firmware_node; +
>>> };
>> 
>> I would prefer struct device_node *of_node; changed to struct
>> fwnode_handle *fwnode;
>> 
>> You can then have struct pptt_fwnode { <.....> /*below fwnode
>> allocated using acpi_alloc_fwnode_static */ struct fwnode_handle
>> *fwnode; };
>> 
>> This gives a good starting point to abstract DT and ACPI.
>> 
>> If not now, we can later implement fwnode.ops=pptt_cache_ops and
>> then use get property for both DT and ACPI.
> 
> 
> I'm obviously confused why this keeps coming up. On the surface it 
> sounds like a good idea. But then, given that I've actually
> implemented a portion of it, what becomes clear is that the PPTT
> isn't a good match.

Fair enough.

> Converting the OF routines to use the fwnode is fairly
> straightforward, but that doesn't help the ACPI situation other than
> to create a lot of misleading code (and the possibility of creating
> nonstandard DSDT entries). The fact that this hasn't been done for
> other tables MADT/SLIT/SRAT/etc makes me wonder why we should do it
> for the PPTT?
> 

IRQ/IORT does use it. If we don't want to use it fine. But the union
doesn't make sense and breaks the flow many other subsystems follow.
Hence I raised. Sorry, I hadn't followed the last revision/discussion on
this, my bad. But I had this thought since the beginning, hence I
brought this up.

> Particularly, when one considers fwnode is more a DSDT<->DT
> abstraction and thus has a lot of API surface that simply doesn't
> make any sense given the PPTT binary tree structure. Given that most
> of the fwnode routines are translating string properties (for
> example fwnode_property_read_string()) it might be possible to build
> a translator of some form which takes DT style properties and
> attempts to map them to the ACPI PPTT tree. What this adds I can't
> fathom, beyond the fact that suddenly the fwnode interface is a
> partial/brittle implementation where a large subset of the
> fwnode_operations will tend to be degenerate cases. The result likely
> will be a poorly implemented translator which breaks or is
> meaningless over a large part of the fwnode API surface.

Sure, I just mentioned ops thing, but that's optional. I just didn't
like the union which has of_node and void ptr instead of fwhandle. I am
fine if many agree that it's bad idea to use fwhandle here.

-- 
Regards,
Sudeep

-- 
Regards,
Sudeep