netdev - Re: [resend/standalone PATCH v4] Add auxiliary bus 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: <20210105001341.GL552508@nvidia.com>
Date:   Mon, 4 Jan 2021 20:13:41 -0400
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Mark Brown <broonie@...nel.org>
CC:     Greg KH <gregkh@...uxfoundation.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        <alsa-devel@...a-project.org>, Kiran Patil <kiran.patil@...el.com>,
        linux-rdma <linux-rdma@...r.kernel.org>,
        Shiraz Saleem <shiraz.saleem@...el.com>,
        Martin Habets <mhabets@...arflare.com>,
        "Liam Girdwood" <lgirdwood@...il.com>,
        Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
        Fred Oh <fred.oh@...ux.intel.com>,
        "Dave Ertman" <david.m.ertman@...el.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Netdev <netdev@...r.kernel.org>,
        Leon Romanovsky <leonro@...dia.com>,
        David Miller <davem@...emloft.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Parav Pandit <parav@...lanox.com>, <lee.jones@...aro.org>
Subject: Re: [resend/standalone PATCH v4] Add auxiliary bus support

On Mon, Jan 04, 2021 at 09:19:30PM +0000, Mark Brown wrote:

> > Regardless of the shortcut to make everything a struct
> > platform_device, I think it was a mistake to put OF devices on
> > platform_bus. Those should have remained on some of_bus even if they
> 
> Like I keep saying the same thing applies to all non-enumerable buses -
> exactly the same considerations exist for all the other buses like I2C
> (including the ACPI naming issue you mention below), and for that matter
> with enumerable buses which can have firmware info.

And most busses do already have their own bus type. ACPI, I2C, PCI,
etc. It is just a few that have been squished into platform, notably
OF.

> > are represented by struct platform_device and fiddling in the core
> > done to make that work OK.
> 
> What exactly is the fiddling in the core here, I'm a bit unclear?

I'm not sure, but I bet there is a small fall out to making bus_type
not 1:1 with the struct device type.. Would have to attempt it to see

> > This feels like a good conference topic someday..
> 
> We should have this discussion *before* we get too far along with trying
> to implement things, we should at least have some idea where we want to
> head there.

Well, auxillary bus is clearly following the original bus model
intention with a dedicated bus type with a controlled naming
scheme. The debate here seems to be "what about platform bus" and
"what to do with mfd"?

> Those APIs all take a struct device for lookup so it's the same call for
> looking things up regardless of the bus the device is on or what
> firmware the system is using - where there are firmware specific lookup
> functions they're generally historical and shouldn't be used for new
> code.  It's generally something in the form
> 
> 	api_type *api_get(struct device *dev, const char *name);

Well, that is a nice improvement since a few years back when I last
worked on this stuff.

But now it begs the question, why not push harder to make 'struct
device' the generic universal access point and add some resource_get()
API along these lines so even a platform_device * isn't needed?

Then the path seems much clearer, add a multi-bus-type device_driver
that has a probe(struct device *) and uses the 'universal api_get()'
style interface to find the generic 'resources'.

The actual bus types and bus structs can then be split properly
without the boilerplate that caused them all to be merged to platform,
even PCI could be substantially merged like this.

Bonus points to replace the open coded method disptach:

int gpiod_count(struct device *dev, const char *con_id)
{
	int count = -ENOENT;

	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
		count = of_gpio_get_count(dev, con_id);
	else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev))
		count = acpi_gpio_count(dev, con_id);

	if (count < 0)
		count = platform_gpio_count(dev, con_id);

With an actual bus specific virtual function:

    return dev->bus->gpio_count(dev);

> ...and then do the same thing for every other bus with firmware
> bindings.  If it's about the firmware interfaces it really isn't a
> platform bus specific thing.  It's not clear to me if that's what it is
> though or if this is just some tangent.

It should be split up based on the unique naming scheme and any bus
specific API elements - like raw access to ACPI or OF data or what
have you for other FW bus types.

Jason