linux-kernel - Re: [PATCH v5 1/2] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc

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]

Date:   Tue, 21 May 2019 09:58:38 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Liming Sun <lsun@...lanox.com>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Andy Shevchenko <andy@...radead.org>,
        Darren Hart <dvhart@...radead.org>,
        Vadim Pasternak <vadimp@...lanox.com>,
        David Woods <dwoods@...lanox.com>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 1/2] platform/mellanox: Add bootctl driver for Mellanox
 BlueField Soc

On Mon, May 20, 2019 at 10:44 PM Liming Sun <lsun@...lanox.com> wrote:
> > -----Original Message-----
> > From: Greg KH <gregkh@...uxfoundation.org>
> > Sent: Monday, May 20, 2019 3:12 PM
> > To: Liming Sun <lsun@...lanox.com>
> > Cc: Andy Shevchenko <andy@...radead.org>; Darren Hart <dvhart@...radead.org>; Vadim Pasternak <vadimp@...lanox.com>; David
> > Woods <dwoods@...lanox.com>; platform-driver-x86@...r.kernel.org; linux-kernel@...r.kernel.org
> > Subject: Re: [PATCH v5 1/2] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc
> >
> > On Mon, May 20, 2019 at 06:07:44PM +0000, Liming Sun wrote:
> > > > > +static struct platform_driver mlxbf_bootctl_driver = {
> > > > > +       .probe = mlxbf_bootctl_probe,
> > > > > +       .driver = {
> > > > > +               .name = "mlxbf-bootctl",
> > > > > +               .groups = mlxbf_bootctl_groups,
> > > > > +               .acpi_match_table = mlxbf_bootctl_acpi_ids,
> > > >
> > > > Why is an acpi driver a platform driver?  Isn't there a "real" acpi
> > > > driver interface you should be tieing into instead?
> > > >
> > > > Only use a platform driver as an absolute last resort.  I don't think
> > > > that is the case here.
> > >
> > > The driver is trying to configure boot-swapping and display secure state,
> > > and is defined/initiated in ACPI table in UEFI. It seems a little hard to
> > > categorize this driver to any existing subsystem. Any suggestion
> > > where it might be a better fit (like drivers/misc, drivers/firmware, etc)?
> > > Please correct me if I misunderstand the comments. Thanks!.
> >
> > The comment was asking why an acpi driver is a platform driver, but then
> > I went and looked now at a bunch of acpi drivers, and they all are
> > platform drivers :(
> >
> > Anyway, drivers/acpi/ seems like the best place for this file, right?
>
> My understanding is that the "drivers/acpi" is mainly for the acpi common code.
> The vendor or platform specific drivers are spread in other various directories,
> most of which are 'platform' drivers.

It depends on how closely you are following the acpi specification.
If you just implement access to a standard ACPI feature, or you have
added your interface to the ACPI specification, then the driver
should work on any system that supports this feature.

> For this driver, we didn't find better sub-component for it, thus put it under
> 'drivers/platform/mellanox' which is vendor specific driver by its name.

drivers/platform/mellanox/ would be a good place for drivers running on
a host platform with a bluefield accelerator card as an add-on, but as I
understand, this is a driver that actually just runs in Linux on the bluefield
itself, so it should go in a different place.

We use drivers/soc/ for things that are specific to one SoC, and that
are typically used by other drivers, but that don't have (and should not
have) a generic abstraction, which probably is not the case here either.

What we do have in drivers/power/reset is a couple of drivers that
set the "reboot reason", communicating that to the firmware for the
next boot, using the reboot_mode_register() interface. I don't
know too much about that interface, but maybe you can use that
instead of adding another sysfs interface?

If you have a complex firmware on the system that you can talk
to, there is also drivers/firmware/ as another option to put
abstractions into.

         Arnd