| 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
| ||
|
Message-ID: <95eaaf28-4472-dfd7-624f-73d58bfccaf@linux.intel.com>
Date: Wed, 2 Nov 2022 11:57:20 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: matthew.gerlach@...ux.intel.com
cc: Xu Yilun <yilun.xu@...el.com>, hao.wu@...el.com,
Russ Weight <russell.h.weight@...el.com>,
basheer.ahmed.muddebihal@...el.com, trix@...hat.com,
mdf@...nel.org, linux-fpga@...r.kernel.org,
linux-doc@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
tianfei.zhang@...el.com, corbet@....net,
linux-serial <linux-serial@...r.kernel.org>,
Jiri Slaby <jirislaby@...nel.org>, geert+renesas@...der.be,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
niklas.soderlund+renesas@...natech.se, macro@...am.me.uk,
johan@...nel.org, Lukas Wunner <lukas@...ner.de>,
marpagan@...hat.com
Subject: Re: [PATCH v4 4/4] tty: serial: 8250: add DFL bus driver for Altera
16550.
On Tue, 1 Nov 2022, matthew.gerlach@...ux.intel.com wrote:
>
>
> On Tue, 1 Nov 2022, Ilpo Järvinen wrote:
>
> > On Tue, 1 Nov 2022, matthew.gerlach@...ux.intel.com wrote:
> >
> > >
> > >
> > > On Tue, 1 Nov 2022, Xu Yilun wrote:
> > >
> > > > On 2022-10-31 at 17:34:39 -0700, matthew.gerlach@...ux.intel.com wrote:
> > > > >
> > > > >
> > > > > On Sat, 29 Oct 2022, Xu Yilun wrote:
> > > > >
> > > > > > On 2022-10-20 at 14:26:10 -0700, matthew.gerlach@...ux.intel.com
> > > > > > wrote:
> > > > > > > From: Matthew Gerlach <matthew.gerlach@...ux.intel.com>
> > > > > > >
> > > > > > > Add a Device Feature List (DFL) bus driver for the Altera
> > > > > > > 16550 implementation of UART.
> > > > > > >
> > > > > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@...ux.intel.com>
> > > > > > > ---
> > > > > > > v4: use dev_err_probe() everywhere that is appropriate
> > > > > > > clean up noise
> > > > > > > change error messages to use the word, unsupported
> > > > > > > tried again to sort Makefile and KConfig better
> > > > > > > reorder probe function for easier error handling
> > > > > > > use new dfh_find_param API
> > > > > > >
> > > > > > > v3: use passed in location of registers
> > > > > > > use cleaned up functions for parsing parameters
> > > > > > >
> > > > > > > v2: clean up error messages
> > > > > > > alphabetize header files
> > > > > > > fix 'missing prototype' error by making function static
> > > > > > > tried to sort Makefile and Kconfig better
> > > > > > > ---
> > > > > > > drivers/tty/serial/8250/8250_dfl.c | 149
> > > > > > > +++++++++++++++++++++++++++++
> > > > > > > drivers/tty/serial/8250/Kconfig | 12 +++
> > > > > > > drivers/tty/serial/8250/Makefile | 1 +
> > > > > > > 3 files changed, 162 insertions(+)
> > > > > > > create mode 100644 drivers/tty/serial/8250/8250_dfl.c
> > > > > > >
> > > > > > > diff --git a/drivers/tty/serial/8250/8250_dfl.c
> > > > > > > b/drivers/tty/serial/8250/8250_dfl.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..f02f0ba2a565
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/tty/serial/8250/8250_dfl.c
> > > > > > > @@ -0,0 +1,149 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > +/*
> > > > > > > + * Driver for FPGA UART
> > > > > > > + *
> > > > > > > + * Copyright (C) 2022 Intel Corporation, Inc.
> > > > > > > + *
> > > > > > > + * Authors:
> > > > > > > + * Ananda Ravuri <ananda.ravuri@...el.com>
> > > > > > > + * Matthew Gerlach <matthew.gerlach@...ux.intel.com>
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <linux/bitfield.h>
> > > > > > > +#include <linux/dfl.h>
> > > > > > > +#include <linux/io-64-nonatomic-lo-hi.h>
> > > > > > > +#include <linux/kernel.h>
> > > > > > > +#include <linux/module.h>
> > > > > > > +#include <linux/serial.h>
> > > > > > > +#include <linux/serial_8250.h>
> > > > > > > +
> > > > > > > +struct dfl_uart {
> > > > > > > + int line;
> > > > > > > +};
> > > > > > > +
> > > > > > > +static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct
> > > > > > > uart_8250_port *uart)
> > > > > > > +{
> > > > > > > + struct device *dev = &dfl_dev->dev;
> > > > > > > + u64 v, fifo_len, reg_width;
> > > > > > > + u64 *p;
> > > > > > > +
> > > > > > > + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
> > > > > > > + if (!p)
> > > > > > > + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ
> > > > > > > param\n");
> > > > > > > +
> > > > > > > + uart->port.uartclk = *p;
> > > > > > > + dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
> > > > > > > +
> > > > > > > + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
> > > > > > > + if (!p)
> > > > > > > + return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN
> > > > > > > param\n");
> > > > > > > +
> > > > > > > + fifo_len = *p;
> > > > > > > + dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
> > > > > > > +
> > > > > > > + switch (fifo_len) {
> > > > > > > + case 32:
> > > > > > > + uart->port.type = PORT_ALTR_16550_F32;
> > > > > > > + break;
> > > > > > > +
> > > > > > > + case 64:
> > > > > > > + uart->port.type = PORT_ALTR_16550_F64;
> > > > > > > + break;
> > > > > > > +
> > > > > > > + case 128:
> > > > > > > + uart->port.type = PORT_ALTR_16550_F128;
> > > > > > > + break;
> > > > > > > +
> > > > > > > + default:
> > > > > > > + return dev_err_probe(dev, -EINVAL, "unsupported
> > > > > > > fifo_len %llu\n", fifo_len);
> > > > > > > + }
> > > > > > > +
> > > > > > > + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
> > > > > > > + if (!p)
> > > > > > > + return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT
> > > > > > > param\n");
> > > > > > > +
> > > > > > > + v = *p;
> > > > > > > + uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> > > > > > > + reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
> > > > > >
> > > > > > I have concern that the raw layout inside the parameter block is
> > > > > > still exposed to drivers and need to be parsed by each driver.
> > > > >
> > > > > Raw parameter block will always have to be passed to the driver
> > > > > because HW
> > > > > specific properties can be defined that will need to be parsed by the
> > > > > specific driver.
> > > >
> > > > So there is a question about the scope of the definitions of these
> > > > parameter
> > > > blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT
> > > > seems specific to uart?
> > >
> > > There are definitely two classes of parameter blocks. One class is HW
> > > agnostic parameters where the parameters are relevant to many different
> > > kinds
> > > of HW components. MSI-X, and input clock-frequency are certainly HW
> > > agnostic,
> > > and it turns out that REG_LAYOUT is not specific to uart. You can see
> > > reg_bits and reg_stride in struct regmap_config. There are also device
> > > tree
> > > bindings for reg-shift and reg-io-width. The second class of parameters
> > > would
> > > be specific to HW component. In the case of this uart driver, all
> > > parameters
> > > would be considered HW agnostic parameters.
> > >
> > > >
> > > > If a parameter block is widely used in dfl drivers, duplicate the
> > > > parsing
> > > > from HW layout in each driver may not be a good idea. While for device
> > > > specific parameter block, it's OK.
> > >
> > > It sounds like we are in agreement.
> > >
> > > >
> > > > Another concern is the indexing of the parameter IDs. If some parameter
> > > > blocks should be device specific, then no need to have globally indexed
> > > > parameter IDs. Index them locally in device is OK. So put the
> > > > definitions
> > > > of ID values, HW layout and their parsing operation in each driver.
> > >
> > > It may be confusing for two drivers to use the same parameter id that have
> > > different meanings and data layout. Since all the parameters for this
> > > driver
> > > would be considered HW agnostic, we'd don't need to address this issue
> > > with
> > > this patchset.
> > >
> > > > > > How about we define HW agnostic IDs for parameter specific fields
> > > > > > like:
> > > > > >
> > > > > > PARAM_ID FIELD_ID
> > > > > > ================================
> > > > > > MSIX STARTV
> > > > > > NUMV
> > > > > > --------------------------------
> > > > > > CLK FREQ
> > > > > > --------------------------------
> > > > > > FIFO LEN
> > > > > > --------------------------------
> > > > > > REG_LAYOUT WIDTH
> > > > > > SHIFT
> > > > > >
> > > > > > And define like u64 dfl_find_param(struct dfl_device *, int
> > > > > > param_id,
> > > > > > int field_id)
> > > > >
> > > > > I don't think dfl_find_param as defined above adds much value.
> > > > >
> > > > > >
> > > > > > Think further, if we have to define HW agnostic property - value
> > > > > > pairs,
> > > > > > why don't we just use "Software nodes for the firmware node", see
> > > > > > drivers/base/swnode.c. I think this may be a better choice.
> > > > >
> > > > > I am looking into "Software nodes for the firmware node", and it can
> > > > > be
> > > > > used
> > > > > for HW agnostic properties. Each dfl driver will still have to make a
> > > > > function call to fetch each HW agnostice property value as well as a
> > > > > function call to find the HW specific parameters and then parse those
> > > > > parameters.
> >
> > Btw, another aspect this discussion has completely overlooked is the
> > presence of parameter version and how it impacts data layout. Is v1
> > always going be a subset of v2 or can a later version remove something
> > v1 had?
>
> In general it would be preferable for v1 to be a subset of v2. This allows
> for v1 SW to work on v2 HW.
In that case, shouldn't the minimum acceptable version be part of
dfh_find_param() parameters?
Currently there's no way for the caller to even look what version the
parameter is from dfh_find_param()'s return value (except with some
negative offset hack to access parameter header).
--
i.
Powered by blists - more mailing lists