linux-kernel - Re: [RFC PATCH 06/10] spi: add support for ACPI reconfigure notifications

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: <CAE1zot+wvU_he-6cFQi1np9X1=FcXWoF-mbxYDRYUkyOT+7nyQ@mail.gmail.com>
Date:	Tue, 5 Apr 2016 14:49:13 +0300
From:	Octavian Purdila <octavian.purdila@...el.com>
To:	"Rafael J. Wysocki" <rafael@...nel.org>
Cc:	Mark Brown <broonie@...nel.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Len Brown <lenb@...nel.org>,
	Matt Fleming <matt@...eblueprint.co.uk>,
	Wolfram Sang <wsa@...-dreams.de>,
	Joel Becker <jlbec@...lplan.org>,
	Christoph Hellwig <hch@....de>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	linux-efi@...r.kernel.org, linux-i2c <linux-i2c@...r.kernel.org>,
	linux-spi@...r.kernel.org, lkml <linux-kernel@...r.kernel.org>,
	Irina Tirdea <irina.tirdea@...el.com>
Subject: Re: [RFC PATCH 06/10] spi: add support for ACPI reconfigure notifications

On Tue, Apr 5, 2016 at 12:18 AM, Rafael J. Wysocki <rafael@...nel.org> wrote:
> On Mon, Apr 4, 2016 at 9:34 PM, Octavian Purdila
> <octavian.purdila@...el.com> wrote:
>> On Mon, Apr 4, 2016 at 7:03 PM, Mark Brown <broonie@...nel.org> wrote:
>>> On Mon, Apr 04, 2016 at 01:25:56PM +0300, Octavian Purdila wrote:
>>>> On Sat, Apr 2, 2016 at 7:24 PM, Mark Brown <broonie@...nel.org> wrote:
>>>
>>>> > What I don't understand is why the flow on inital probe isn't simply to
>>>> > register the controller which then triggers the walk of the children.
>>>> > That way any bus that supports initial probe also supports hotplug
>>>> > without needing to go and manually add a second code path.
>>>
>>>> Do you mean register the notifier per controller instead of per
>>>> subsystem? Either way we need changes at the subsystem level and I
>>>> choose to follow the device tree implementation for consistency.
>>>
>>> No!  I mean use the exact same callback you've got now for everything.
>>>
>>>> The other reason is that (pending other ACPICA changes) we can add
>>>> other notification events in the future such as node added or removed
>>>> (just like device tree), and in that case the probe and hotplug
>>>> handling would be different (and a bit more efficient).
>>>
>>> Why is probe different to hotplug?  We don't need to do that in the
>>> normal driver model.
>>
>> There might be some confusion with the term, I am referring to slave
>> hotplug, not controller hotplug.
>>
>> The way I see it, there are two logical operations: probe of a
>> controller and the associated enumeration of the SPI slaves for that
>> bus and "hotplug" of new SPI slaves and the enumeration of those
>> particular slaves.
>>
>> When we probe the controller we search DT/ACPI and enumerate all the
>> slaves for *that* controller.
>>
>> When a slave hotplug happens for device tree we get a device node
>> notification and we can instantiate the SPI slave based on that info.
>> In case of ACPI, (at this point) we get a global callback and in that
>> callback we need to iterate through *all* controllers.
>
> Is that really necessary?
>
> The namespace rescan could notify the parent of a new node in
> acpi_default_enumeration(), couldn't it?

We could do that, with some complexity, but I am not sure if it is
worth it - see below. And in either case I still don't see how we can
avoid two enumeration paths: one from the probe of the controller ->
spi_register_master() -> acpi_register_spi_devices()  and one from the
notification handler -> acpi_register_spi_devices(). Both will end up
are calling the same enumeration function but there are still two
paths.

Even right now, in the case of dynamic DT, we have the two enumeration paths.

If we really want to have a single path for ACPI enumeration we could
do that by using an ACPI SPI bridge driver or scan handlers after
extending the matching mechanisms. But we would still need to modify
the SPI subsystem and I don't think its worth it just to save a call
to acpi_register_spi_devices() from spi_register_master().

Now for parent notification complexity: in the case of SPI we can
easily to this because current ACPI SPI enumeration supports only
direct children as slaves. However, on I2C we can have an unrelated
node as a slave - that is why the I2C scanning searches the whole
namespaces for references to a particular i2c_adapter. So, we would
need to retrieve the parent node from the namespace node information
which means that we will do SPI specific stuff in ACPI generic code. I
don't think it is a big issue, because we already treat SPI / I2C
special, right?

Once we have the parent handle we would still have to iterate through
all controllers to get the spi_master matching the ACPI handle. It is
a bit more efficient then global notification, but probably won't
matter in practice.