RE: [PATCH v5 1/2] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc

[Liming Sun]

Thanks Arnd for the comments! Please also see my response below.

> -----Original Message-----
> From: Arnd Bergmann <arnd@xxxxxxxx>
> Sent: Tuesday, May 21, 2019 3:59 AM
> To: Liming Sun <lsun@xxxxxxxxxxxx>
> Cc: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>; Andy Shevchenko <andy@xxxxxxxxxxxxx>; Darren Hart <dvhart@xxxxxxxxxxxxx>; Vadim
> Pasternak <vadimp@xxxxxxxxxxxx>; David Woods <dwoods@xxxxxxxxxxxx>; platform-driver-x86@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxx> wrote:
> > > -----Original Message-----
> > > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > Sent: Monday, May 20, 2019 3:12 PM
> > > To: Liming Sun <lsun@xxxxxxxxxxxx>
> > > Cc: Andy Shevchenko <andy@xxxxxxxxxxxxx>; Darren Hart <dvhart@xxxxxxxxxxxxx>; Vadim Pasternak <vadimp@xxxxxxxxxxxx>;
> David
> > > Woods <dwoods@xxxxxxxxxxxx>; platform-driver-x86@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > > 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.

Yes, the driver is actually running on the bluefield itself.
So looks like we'll need to find another location for it.

> 
> 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.

I did a 'grep' for 'module_platform_driver' and 'DEVICE_ATTR' under
drivers/soc, and found quite some under drivers/soc. This 'bootctl' driver
is SoC specific and tries to control the low-level boot partition (not
Linux related). Please also see my response to Mark's comments about
the SoC boot flow. Probably the 'drivers/soc' would be better fit for
this driver? Please advise.

> 
> 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?

I checked the history of the 'reboot_mode'. Looks like it tries to
control how Linux boots, which is different than what this commit
does. This commit tries to control how ATF/UEFI boots, not how
Linux boots.

> 
> 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.

I feel like 'drivers/soc' migh be better fit. We could certainly try
drivers/firmware/ and abstractions if you think 'drivers/soc' is not 
a good option.

> 
>          Arnd