| 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
| ||
|
Date: Tue, 10 May 2022 14:59:57 +0000
From: <Kavyasree.Kotagiri@...rochip.com>
To: <peda@...ntia.se>
CC: <devicetree@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <lee.jones@...aro.org>,
<linux@...linux.org.uk>, <Manohar.Puri@...rochip.com>,
<UNGLinuxDriver@...rochip.com>,
<krzysztof.kozlowski+dt@...aro.org>, <Nicolas.Ferre@...rochip.com>,
<alexandre.belloni@...tlin.com>, <Claudiu.Beznea@...rochip.com>
Subject: RE: [PATCH v2 4/4] mux: lan966: Add support for flexcom mux
controller
> > LAN966 SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
> > For each chip select of each flexcom there is a configuration
> > register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
> > configuration register is 21 because there are 21 shared pins
> > on each of which the chip select can be mapped. Each bit of the
> > register represents a different FLEXCOM_SHARED pin.
> >
> > Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@...rochip.com>
> > ---
> > arch/arm/mach-at91/Kconfig | 2 +
> > drivers/mfd/atmel-flexcom.c | 55 +++++++++++++++-
> > drivers/mux/Kconfig | 12 ++++
> > drivers/mux/Makefile | 2 +
> > drivers/mux/lan966-flx.c | 121
> ++++++++++++++++++++++++++++++++++++
> > 5 files changed, 191 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/mux/lan966-flx.c
> >
> > diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
> > index 279810381256..26fb0f4e1b79 100644
> > --- a/arch/arm/mach-at91/Kconfig
> > +++ b/arch/arm/mach-at91/Kconfig
> > @@ -74,6 +74,8 @@ config SOC_LAN966
> > select DW_APB_TIMER_OF
> > select ARM_GIC
> > select MEMORY
> > + select MULTIPLEXER
> > + select MUX_LAN966
> > help
> > This enables support for ARMv7 based Microchip LAN966 SoC family.
> >
> > diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
> > index 559eb4d352b6..7cfd0fc3f4f0 100644
> > --- a/drivers/mfd/atmel-flexcom.c
> > +++ b/drivers/mfd/atmel-flexcom.c
> > @@ -17,6 +17,7 @@
> > #include <linux/io.h>
> > #include <linux/clk.h>
> > #include <dt-bindings/mfd/atmel-flexcom.h>
> > +#include <linux/mux/consumer.h>
> >
> > /* I/O register offsets */
> > #define FLEX_MR 0x0 /* Mode Register */
> > @@ -28,6 +29,10 @@
> > #define FLEX_MR_OPMODE(opmode) (((opmode) <<
> FLEX_MR_OPMODE_OFFSET) & \
> > FLEX_MR_OPMODE_MASK)
> >
> > +struct atmel_flex_caps {
> > + bool has_flx_mux;
> > +};
> > +
> > struct atmel_flexcom {
> > void __iomem *base;
> > u32 opmode;
> > @@ -37,6 +42,7 @@ struct atmel_flexcom {
> > static int atmel_flexcom_probe(struct platform_device *pdev)
> > {
> > struct device_node *np = pdev->dev.of_node;
> > + const struct atmel_flex_caps *caps;
> > struct resource *res;
> > struct atmel_flexcom *ddata;
> > int err;
> > @@ -76,13 +82,60 @@ static int atmel_flexcom_probe(struct
> platform_device *pdev)
> > */
> > writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base + FLEX_MR);
> >
> > + caps = of_device_get_match_data(&pdev->dev);
> > + if (!caps) {
> > + dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* Flexcom Mux */
> > + if (caps->has_flx_mux && of_property_read_bool(np, "mux-controls"))
> {
> > + struct mux_control *flx_mux;
> > + struct of_phandle_args args;
> > + int i, count;
> > +
> > + flx_mux = devm_mux_control_get(&pdev->dev, NULL);
> > + if (IS_ERR(flx_mux))
> > + return PTR_ERR(flx_mux);
> > +
> > + count = of_property_count_strings(np, "mux-control-names");
> > + for (i = 0; i < count; i++) {
> > + err = of_parse_phandle_with_fixed_args(np, "mux-controls",
> 1, i, &args);
> > + if (err)
> > + break;
> > +
> > + err = mux_control_select(flx_mux, args.args[0]);
> > + if (!err) {
> > + mux_control_deselect(flx_mux);
>
> This is suspect. When you deselect the mux you rely on the mux to be
> configured with "as-is" as the idle state. But you do not document that.
> This is also fragile in that you silently rely on noone else selecting
> the mux to some unwanted state behind your back (mux controls are not
> exclusive the way e.g. gpio pins or pwms are). The protocol is that
> others may get a ref to the mux control and select it as long as noone
> else has selected it.
>
> The only sane thing to do is to keep the mux selected for the whole
> duration when you rely on it to be in your desired state.
>
Yes, mux is kept selected until configuring register is done. Please see below log where
I added debug prints just for understanding:
# dmesg | grep KK
[ 0.779827] KK: Func: atmel_flexcom_probe ***** [START flx muxing] ********
[ 0.779875] KK: Func: atmel_flexcom_probe i = 0 args[0] = 0
[ 0.779890] KK: Func: mux_control_select [Entered]
[ 0.779902] KK: Func: mux_lan966x_set [Entered] state = 0
[ 0.779977] KK: Func: mux_lan966x_set [Read] = 0x1fffef <<<----- setting mux_lan966x[state].ss_pin "4" which is passed from dts
[ 0.779992] KK: Func: mux_lan966x_set [Exit]
[ 0.780002] KK: Func: mux_control_select [Exit]
[ 0.780011] KK: Func: mux_control_deselect [Entered]
[ 0.780021] KK: Func: mux_control_deselect [Exit]
> > + } else {
> > + dev_err(&pdev->dev, "Failed to select FLEXCOM mux\n");
> > + return err;
> > + }
> > + }
> > + }
> > +
> > clk_disable_unprepare(ddata->clk);
> >
> > return devm_of_platform_populate(&pdev->dev);
> > }
> >
> > +static const struct atmel_flex_caps atmel_flexcom_caps = {};
> > +
> > +static const struct atmel_flex_caps lan966x_flexcom_caps = {
> > + .has_flx_mux = true,
> > +};
> > +
> > static const struct of_device_id atmel_flexcom_of_match[] = {
> > - { .compatible = "atmel,sama5d2-flexcom" },
> > + {
> > + .compatible = "atmel,sama5d2-flexcom",
> > + .data = &atmel_flexcom_caps,
> > + },
> > +
> > + {
> > + .compatible = "microchip,lan966-flexcom",
> > + .data = &lan966x_flexcom_caps,
> > + },
> > +
> > { /* sentinel */ }
> > };
> > MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
> > diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> > index e5c571fd232c..ea09f474bc2f 100644
> > --- a/drivers/mux/Kconfig
> > +++ b/drivers/mux/Kconfig
> > @@ -45,6 +45,18 @@ config MUX_GPIO
> > To compile the driver as a module, choose M here: the module will
> > be called mux-gpio.
> >
> > +config MUX_LAN966
> > + tristate "LAN966 Flexcom multiplexer"
> > + depends on OF || COMPILE_TEST
> > + help
> > + Lan966 Flexcom Multiplexer controller.
> > +
> > + The driver supports mapping 2 chip-selects of each of the lan966
> > + flexcoms to 21 flexcom shared pins.
> > +
> > + To compile the driver as a module, choose M here: the module will
> > + be called mux-lan966.
> > +
> > config MUX_MMIO
> > tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
> > depends on OF || COMPILE_TEST
> > diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> > index 6e9fa47daf56..53a9840d96fa 100644
> > --- a/drivers/mux/Makefile
> > +++ b/drivers/mux/Makefile
> > @@ -7,10 +7,12 @@ mux-core-objs := core.o
> > mux-adg792a-objs := adg792a.o
> > mux-adgs1408-objs := adgs1408.o
> > mux-gpio-objs := gpio.o
> > +mux-lan966-objs := lan966-flx.o
> > mux-mmio-objs := mmio.o
> >
> > obj-$(CONFIG_MULTIPLEXER) += mux-core.o
> > obj-$(CONFIG_MUX_ADG792A) += mux-adg792a.o
> > obj-$(CONFIG_MUX_ADGS1408) += mux-adgs1408.o
> > obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
> > +obj-$(CONFIG_MUX_LAN966) += mux-lan966.o
> > obj-$(CONFIG_MUX_MMIO) += mux-mmio.o
> > diff --git a/drivers/mux/lan966-flx.c b/drivers/mux/lan966-flx.c
> > new file mode 100644
> > index 000000000000..2c7dab616a6a
> > --- /dev/null
> > +++ b/drivers/mux/lan966-flx.c
> > @@ -0,0 +1,121 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LAN966 Flexcom MUX driver
> > + *
> > + * Copyright (c) 2022 Microchip Inc.
> > + *
> > + * Author: Kavyasree Kotagiri <kavyasree.kotagiri@...rochip.com>
>
> Looks like it is based on mmio.c?
>
Yes, I took mmio.c driver as reference.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/property.h>
> > +#include <linux/mux/driver.h>
> > +#include <linux/io.h>
> > +
> > +#define FLEX_SHRD_MASK 0x1FFFFF
> > +#define LAN966_MAX_CS 21
> > +
> > +static void __iomem *flx_shared_base;
>
> I would much prefer to store the combined address (base+offset)
> in the mux private data instead of only storing the offset and
> then unnecessarily rely on some piece of global state (that
> will be clobbered by other instances).
>
Ok. I will try to see if this is relevant and change accordingly.
> > +struct mux_lan966x {
>
> Why is the file named lan966, but then everything inside lan966x?
>
> > + u32 offset;
> > + u32 ss_pin;
> > +};
> > +
> > +static int mux_lan966x_set(struct mux_control *mux, int state)
> > +{
> > + struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
> > + u32 val;
> > +
> > + val = ~(1 << mux_lan966x[state].ss_pin) & FLEX_SHRD_MASK;
> > + writel(val, flx_shared_base + mux_lan966x[state].offset);
>
> This reads memory you have not allocated, if you select a state
> other than zero.
>
Ok. I will return error condition in case of trying to access none existing entry.
> > +
> > + return 0;
> > +}
> > +
> > +static const struct mux_control_ops mux_lan966x_ops = {
> > + .set = mux_lan966x_set,
> > +};
> > +
> > +static const struct of_device_id mux_lan966x_dt_ids[] = {
> > + { .compatible = "microchip,lan966-flx-mux", },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, mux_lan966x_dt_ids);
> > +
> > +static int mux_lan966x_probe(struct platform_device *pdev)
> > +{
> > + struct device_node *np = pdev->dev.of_node;
> > + struct device *dev = &pdev->dev;
> > + struct mux_lan966x *mux_lan966x;
> > + struct mux_chip *mux_chip;
> > + int ret, num_fields, i;
> > +
> > + ret = of_property_count_u32_elems(np, "mux-offset-pin");
> > + if (ret == 0 || ret % 2)
> > + ret = -EINVAL;
> > + if (ret < 0)
> > + return dev_err_probe(dev, ret,
> > + "mux-offset-pin property missing or invalid");
> > + num_fields = ret / 2;
> > +
> > + mux_chip = devm_mux_chip_alloc(dev, num_fields,
> sizeof(*mux_lan966x));
>
> I might be thoroughly mistaken and confused by the code, but it seems
> very strange that a subsequenct select is not undoing what a previous
> select once did. Each state seems to write to its own register offset,
> and there is nothing that restores the first register offset with you
> switch states.
>
> Care to explain how muxing works and what you are expected to do to
> manipulate the mux? Is there some link to public documentation? I did
> a quick search for lan966 but came up with nothing relevant.
>
LAN966 has 5 flexcoms(which can be used as USART/SPI/I2C interface).
FLEXCOM has two chip-select I/O lines namely CS0 and CS1
in SPI mode, CTS and RTS in USART mode. These FLEXCOM pins are optional.
These chip-selects can be mapped to flexcom shared pin [0-21] which can be
done by configuring flexcom multiplexer register(FLEXCOM_SHARED[0-4]:SS_MASK[0-1])
Driver explanation:
"flx_shared_base" is used to get base address of Flexcom shared multiplexer
"mux-offset-pin" property is used to get cs0/cs1 offset and flexcom shared pin[0-21] of a flexcom.
state value passed from atmel-flexcom is used to configure respective
FLEXCOM_SHARED[0-4]:SS_MASK[0-1] register with offset and flexcom shared pin.
> > + if (IS_ERR(mux_chip))
> > + return dev_err_probe(dev, PTR_ERR(mux_chip),
> > + "failed to allocate mux_chips\n");
> > +
> > + mux_lan966x = mux_chip_priv(mux_chip);
> > +
> > + flx_shared_base = devm_platform_get_and_ioremap_resource(pdev,
> 0, NULL);
> > + if (IS_ERR(flx_shared_base))
> > + return dev_err_probe(dev, PTR_ERR(flx_shared_base),
> > + "failed to get flexcom shared base address\n");
> > +
> > + for (i = 0; i < num_fields; i++) {
> > + struct mux_control *mux = &mux_chip->mux[i];
> > + u32 offset, shared_pin;
> > +
> > + ret = of_property_read_u32_index(np, "mux-offset-pin",
> > + 2 * i, &offset);
> > + if (ret == 0)
> > + ret = of_property_read_u32_index(np, "mux-offset-pin",
> > + 2 * i + 1,
> > + &shared_pin);
> > + if (ret < 0)
> > + return dev_err_probe(dev, ret,
> > + "failed to read mux-offset-pin property: %d", i);
> > +
> > + if (shared_pin >= LAN966_MAX_CS)
> > + return -EINVAL;
> > +
> > + mux_lan966x[i].offset = offset;
> > + mux_lan966x[i].ss_pin = shared_pin;
>
> This clobbers memory you have not allocated, if num_fields >= 1.
>
> Cheers,
> Peter
>
> > +
> > + mux->states = LAN966_MAX_CS;
> > + }
> > +
> > + mux_chip->ops = &mux_lan966x_ops;
> > +
> > + ret = devm_mux_chip_register(dev, mux_chip);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver mux_lan966x_driver = {
> > + .driver = {
> > + .name = "lan966-mux",
> > + .of_match_table = of_match_ptr(mux_lan966x_dt_ids),
> > + },
> > + .probe = mux_lan966x_probe,
> > +};
> > +
> > +module_platform_driver(mux_lan966x_driver);
> > +
> > +MODULE_DESCRIPTION("LAN966 Flexcom multiplexer driver");
> > +MODULE_AUTHOR("Kavyasree Kotagiri
> <kavyasree.kotagiri@...rochip.com>");
> > +MODULE_LICENSE("GPL v2");
> > +
Powered by blists - more mailing lists