Re: [PATCH v5 00/15] Add Broadcom VK driver

[Olof Johansson]

Hi,

On Thu, Oct 1, 2020 at 4:38 AM Florian Fainelli <f.fainelli@xxxxxxxxx> wrote:
>
> On 9/30/2020 6:27 PM, Scott Branden wrote:
> > This patch series drops previous patches in [1]
> > that were incorporated by Kees Cook into patch series
> > "Introduce partial kernel_read_file() support" [2].
> >
> > Remaining patches are contained in this series to add Broadcom VK driver.
> > (which depends on request_firmware_into_buf API addition in
> > other patch series [2] being applied first).
> >
> > Please note this patch series will not compile without [2].
> >
> > [1] https://lore.kernel.org/lkml/20200706232309.12010-1-scott.branden@xxxxxxxxxxxx/
> > [2] https://lore.kernel.org/lkml/20200729175845.1745471-1-keescook@xxxxxxxxxxxx/
>
> Disclaimer: I am well aware that it is the complete wild west right now
> as far as accelerators go and that every vendor (that I happen to work
> for, just not in the same group as Scott) is just seeking to get their
> drivers included upstream and hopefully for good reasons.

I'm not sure there are ever bad reasons to get code upstreamed?

I've mentioned this before, but until we see the code and
implementations we can guess where the likely commonality is, but
we'll always miss some of it and overdesign other parts of it. The
path of allowing some entropy to later be refactored and made common
is something we've done pretty much everywhere, and it's an approach
that's been proven to work. While those who have already taken that
journey think it's "just a matter of jumping to the end state and just
do things the way they did in the end" is shortsighted: They're not at
a static endpoint and futureproof solution themselves. Some of these
things just need to play out naturally over time.

Luckily, most kernel-facing interfaces are fairly simple and sit as
isolated drivers today, so it doesn't add maintainer burden across the
rest of the kernel so the cost of letting some of this code in isn't
huge. For a few things such as the hwmon and tty aspects it makes
sense to integrate better just as you suggested, but the "subdrivers"
for that are fairly simple and don't add a lot of dependencies on
intricate or complex subsystem features.

Some devices, such as Habana's latest one, is looking at more complex
integrations with other subsystems (RDMA/networking), and there's
plenty of discussion going on there.

>  From a cursory look however, it sounds like there could be a little
> better re-utilization of standards, standard framework and interfaces:

I think most of these points are valid, but also not dealbreakers.
I'll add my opinions on a couple of them below.

> - about 2/3 of your sysfs attributes should incline you to implement a
> HWMON device, not a complicated one, but anything that indicates
> current, power, temperature, alerts etc. should be considered

This likely makes sense, and hopefully isn't too much work to move.
Just like with TTY below, if it needs more time it might make sense to
take it out of this patchset and follow up with it, since it's useful
but not strictly required functionality for the rest of the
driver/device.

> - cannot the firmware loading be supported over remoteproc somehow?

remoteproc is really useful for devices living in a shared SoC,
needing memory carveout, and run control of the coprocessor. In this
case the programming model is different, there's a normal/regular PCIe
host/device relationship and the process of loading firmware onto a
device is something we do in lots of drivers without using remoteproc.
I don't think we need to bring that framework into these kind of
drivers, unless there are tighter coupling between the host/device to
consider for some reason.

> - could not the TTY interface be using virtio or an existing UART?

hvc is super convenient to integrate with since all you need is a
getchar and putchar implementation, but the others would work as well.
I don't have a strong opinion on path forward on this, and if TTY ends
up holding up the rest of the driver it might make sense to leave that
piece out and merge the rest. I'm not sure we're at that junction
quite yet though?

> - what is the format of the message over BAR2 that you expose in patch 13?
>
> Is there a reference user-space implementation that you can link to this
> patch submission in case people are curious?


-Olof