Re: [PATCH 01/10] alpha: use libata instead of the legacy ide driver
Måns Rullgård writes: > Christoph Hellwig writes: > >> On Thu, Mar 18, 2021 at 05:54:55AM +, Al Viro wrote: >>> On Thu, Mar 18, 2021 at 05:56:57AM +0100, Christoph Hellwig wrote: >>> > Switch the alpha defconfig from the legacy ide driver to libata. >>> >>> Umm... I don't have an IDE alpha box in a usable shape (fans on >>> CPU module shat themselves), and it would take a while to resurrect >>> it, but I remember the joy it used to cause in some versions. >>> >>> Do you have reports of libata variants of drivers actually tested on >>> those? >> >> No, I haven't. The whole point is that we're not going to keep 4 >> lines of code around despite notice for users that don't exist or >> care. If there is a regression we'll fix it, but we're not going to >> make life miserable just because we can. > > The pata_ali driver works fine on my UP1500 machine, unless something > broke recently. I'll build the latest kernel and report back. 5.11.7 seems fine too. -- Måns Rullgård
Re: [PATCH 01/10] alpha: use libata instead of the legacy ide driver
Christoph Hellwig writes: > On Thu, Mar 18, 2021 at 05:54:55AM +, Al Viro wrote: >> On Thu, Mar 18, 2021 at 05:56:57AM +0100, Christoph Hellwig wrote: >> > Switch the alpha defconfig from the legacy ide driver to libata. >> >> Umm... I don't have an IDE alpha box in a usable shape (fans on >> CPU module shat themselves), and it would take a while to resurrect >> it, but I remember the joy it used to cause in some versions. >> >> Do you have reports of libata variants of drivers actually tested on >> those? > > No, I haven't. The whole point is that we're not going to keep 4 > lines of code around despite notice for users that don't exist or > care. If there is a regression we'll fix it, but we're not going to > make life miserable just because we can. The pata_ali driver works fine on my UP1500 machine, unless something broke recently. I'll build the latest kernel and report back. -- Måns Rullgård
Re: [PATCH] serial: 8250: add option to disable registration of legacy ISA ports
Andy Shevchenko writes: > On Thursday, January 28, 2021, Mans Rullgard wrote: > >> On systems that do not have the traditional PC ISA serial ports, the >> 8250 driver still creates non-functional device nodes. This change >> makes only ports that actually exist (PCI, DT, ...) get device nodes. >> >> > > This is kinda ABI breakage. At least this will break x86 platforms with > HSUARTs (all modern ones) that are used in embedded systems. > > I think you would rather need an option to disable this and select it by > the platforms where it is known not to break anything. What exactly breaks? The new option is enabled by default, just like the one right beside it (SERIAL_8250_PNP), so nothing at all changes unless this is actively disabled. On a system that doesn't have those ports, any attempt to access the device nodes produces some kind of error. How is it breaking anything to not create device nodes for hardware that doesn't exist? -- Måns Rullgård
Re: [PATCH] serial: 8250: add option to disable registration of legacy ISA ports
Greg Kroah-Hartman writes: > On Sun, Jan 31, 2021 at 01:18:47PM +0000, Måns Rullgård wrote: >> Greg Kroah-Hartman writes: >> >> > On Thu, Jan 28, 2021 at 05:22:44PM +, Mans Rullgard wrote: >> >> On systems that do not have the traditional PC ISA serial ports, the >> >> 8250 driver still creates non-functional device nodes. This change >> >> makes only ports that actually exist (PCI, DT, ...) get device nodes. >> >> >> >> Signed-off-by: Mans Rullgard >> >> --- >> >> drivers/tty/serial/8250/8250_core.c | 26 -- >> >> drivers/tty/serial/8250/Kconfig | 5 + >> >> 2 files changed, 25 insertions(+), 6 deletions(-) >> >> >> >> diff --git a/drivers/tty/serial/8250/8250_core.c >> >> b/drivers/tty/serial/8250/8250_core.c >> >> index cae61d1ebec5..49695dd3677c 100644 >> >> --- a/drivers/tty/serial/8250/8250_core.c >> >> +++ b/drivers/tty/serial/8250/8250_core.c >> >> @@ -555,6 +555,7 @@ static void __init serial8250_isa_init_ports(void) >> >> } >> >> } >> >> >> >> +#ifdef CONFIG_SERIAL_8250_ISA >> > >> > This is just making a mess of the code. >> >> It was already a mess. > > True, but don't make it a worse one please. > >> >> > To do this right, pull the isa code out into a separate file and put >> > the #ifdef in a .h file, so we can properly maintain and support this >> > code over time. This change as-is is not going to make that any >> > easier :( >> >> I might put in that effort if there's a reasonable chance this change >> will be accepted. If it's going to be rejected regardless, I'd rather >> not waste my time. >> >> >> +config SERIAL_8250_ISA >> >> + bool "8250/16550 ISA device support" if EXPERT >> > >> > So, no one will set this? >> >> I followed the pattern of the existing SERIAL_8250_PNP option. Was that >> a mistake? How would you prefer it? > > I don't know, I'm just asking. > >> > What userspace visable change will be caused by this? >> >> There won't be /dev/ttyS devices for ports that don't exist. >> >> > Will ports get renumbered? >> >> Not if they had predictable numbers to begin with. > > So that would be "yes"? If so, obviously we couldn't take this, right? On a Beaglebone Black based system with some of the UARTs enabled, those keep their numbers such that e.g. ttyS0, ttyS1, and ttyS4 show up in /dev while ttyS2 and ttyS3 do not since they don't correspond to any (enabled) ports. If any of the very many variants of this driver do not assign fixed numbers, those would possibly be renumbered. Should that be the case, the numbering was never guaranteed to begin with, so I fail to see any problem. -- Måns Rullgård
Re: [PATCH] serial: 8250: add option to disable registration of legacy ISA ports
Greg Kroah-Hartman writes: > On Thu, Jan 28, 2021 at 05:22:44PM +, Mans Rullgard wrote: >> On systems that do not have the traditional PC ISA serial ports, the >> 8250 driver still creates non-functional device nodes. This change >> makes only ports that actually exist (PCI, DT, ...) get device nodes. >> >> Signed-off-by: Mans Rullgard >> --- >> drivers/tty/serial/8250/8250_core.c | 26 -- >> drivers/tty/serial/8250/Kconfig | 5 + >> 2 files changed, 25 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/tty/serial/8250/8250_core.c >> b/drivers/tty/serial/8250/8250_core.c >> index cae61d1ebec5..49695dd3677c 100644 >> --- a/drivers/tty/serial/8250/8250_core.c >> +++ b/drivers/tty/serial/8250/8250_core.c >> @@ -555,6 +555,7 @@ static void __init serial8250_isa_init_ports(void) >> } >> } >> >> +#ifdef CONFIG_SERIAL_8250_ISA > > This is just making a mess of the code. It was already a mess. > To do this right, pull the isa code out into a separate file and put > the #ifdef in a .h file, so we can properly maintain and support this > code over time. This change as-is is not going to make that any > easier :( I might put in that effort if there's a reasonable chance this change will be accepted. If it's going to be rejected regardless, I'd rather not waste my time. >> +config SERIAL_8250_ISA >> +bool "8250/16550 ISA device support" if EXPERT > > So, no one will set this? I followed the pattern of the existing SERIAL_8250_PNP option. Was that a mistake? How would you prefer it? > What userspace visable change will be caused by this? There won't be /dev/ttyS devices for ports that don't exist. > Will ports get renumbered? Not if they had predictable numbers to begin with. > What harm is this causing systems today without this change? It means I have to somehow maintain a separate list of which ports are real and which should be ignored, or else try to distinguish real errors from those caused by trying to access the bogus ports. -- Måns Rullgård
Re: [PATCH] pci: remove tango host controller driver
-static const struct pci_ecam_ops smp8759_ecam_ops = {
> - .pci_ops= {
> - .map_bus= pci_ecam_map_bus,
> - .read = smp8759_config_read,
> - .write = smp8759_config_write,
> - }
> -};
> -
> -static int tango_pcie_link_up(struct tango_pcie *pcie)
> -{
> - void __iomem *test_out = pcie->base + SMP8759_TEST_OUT;
> - int i;
> -
> - writel_relaxed(16, test_out);
> - for (i = 0; i < 10; ++i) {
> - u32 ltssm_state = readl_relaxed(test_out) >> 8;
> - if ((ltssm_state & 0x1f) == 0xf) /* L0 */
> - return 1;
> - usleep_range(3000, 4000);
> - }
> -
> - return 0;
> -}
> -
> -static int tango_pcie_probe(struct platform_device *pdev)
> -{
> - struct device *dev = >dev;
> - struct tango_pcie *pcie;
> - struct resource *res;
> - struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node);
> - struct irq_domain *msi_dom, *irq_dom;
> - struct of_pci_range_parser parser;
> - struct of_pci_range range;
> - int virq, offset;
> -
> - dev_warn(dev, "simultaneous PCI config and MMIO accesses may cause data
> corruption\n");
> - add_taint(TAINT_CRAP, LOCKDEP_STILL_OK);
> -
> - pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> - if (!pcie)
> - return -ENOMEM;
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> - pcie->base = devm_ioremap_resource(dev, res);
> - if (IS_ERR(pcie->base))
> - return PTR_ERR(pcie->base);
> -
> - platform_set_drvdata(pdev, pcie);
> -
> - if (!tango_pcie_link_up(pcie))
> - return -ENODEV;
> -
> - if (of_pci_dma_range_parser_init(, dev->of_node) < 0)
> - return -ENOENT;
> -
> - if (of_pci_range_parser_one(, ) == NULL)
> - return -ENOENT;
> -
> - range.pci_addr += range.size;
> - pcie->msi_doorbell = range.pci_addr + res->start + SMP8759_DOORBELL;
> -
> - for (offset = 0; offset < MSI_MAX / 8; offset += 4)
> - writel_relaxed(0, pcie->base + SMP8759_ENABLE + offset);
> -
> - virq = platform_get_irq(pdev, 1);
> - if (virq < 0)
> - return virq;
> -
> - irq_dom = irq_domain_create_linear(fwnode, MSI_MAX, _ops, pcie);
> - if (!irq_dom) {
> - dev_err(dev, "Failed to create IRQ domain\n");
> - return -ENOMEM;
> - }
> -
> - msi_dom = pci_msi_create_irq_domain(fwnode, _dom_info, irq_dom);
> - if (!msi_dom) {
> - dev_err(dev, "Failed to create MSI domain\n");
> - irq_domain_remove(irq_dom);
> - return -ENOMEM;
> - }
> -
> - pcie->dom = irq_dom;
> - spin_lock_init(>used_msi_lock);
> - irq_set_chained_handler_and_data(virq, tango_msi_isr, pcie);
> -
> - return pci_host_common_probe(pdev);
> -}
> -
> -static const struct of_device_id tango_pcie_ids[] = {
> - {
> - .compatible = "sigma,smp8759-pcie",
> - .data = _ecam_ops,
> - },
> - { },
> -};
> -
> -static struct platform_driver tango_pcie_driver = {
> - .probe = tango_pcie_probe,
> - .driver = {
> - .name = KBUILD_MODNAME,
> - .of_match_table = tango_pcie_ids,
> - .suppress_bind_attrs = true,
> - },
> -};
> -builtin_platform_driver(tango_pcie_driver);
> -
> -/*
> - * The root complex advertises the wrong device class.
> - * Header Type 1 is for PCI-to-PCI bridges.
> - */
> -static void tango_fixup_class(struct pci_dev *dev)
> -{
> - dev->class = PCI_CLASS_BRIDGE_PCI << 8;
> -}
> -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x0024, tango_fixup_class);
> -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x0028, tango_fixup_class);
> -
> -/*
> - * The root complex exposes a "fake" BAR, which is used to filter
> - * bus-to-system accesses. Only accesses within the range defined by this
> - * BAR are forwarded to the host, others are ignored.
> - *
> - * By default, the DMA framework expects an identity mapping, and DRAM0 is
> - * mapped at 0x8000.
> - */
> -static void tango_fixup_bar(struct pci_dev *dev)
> -{
> - dev->non_compliant_bars = true;
> - pci_write_config_dword(dev, PCI_BASE_ADDRESS_0, 0x8000);
> -}
> -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x0024, tango_fixup_bar);
> -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SIGMA, 0x0028, tango_fixup_bar);
> --
>
> 2.29.2
>
--
Måns Rullgård
Re: [PATCH 1/2] thermal: remove tango driver
; - if (temp_above_thresh(priv->base, idx)) {
> - /* Search upward by incrementing thresh_idx */
> - while (idx < IDX_MAX && temp_above_thresh(priv->base, ++idx))
> - cpu_relax();
> - idx = idx - 1; /* always return lower bound */
> - } else {
> - /* Search downward by decrementing thresh_idx */
> - while (idx > IDX_MIN && !temp_above_thresh(priv->base, --idx))
> - cpu_relax();
> - }
> -
> - *res = (idx * 9 / 2 - 38) * 1000; /* millidegrees Celsius */
> - priv->thresh_idx = idx;
> -
> - return 0;
> -}
> -
> -static const struct thermal_zone_of_device_ops ops = {
> - .get_temp = tango_get_temp,
> -};
> -
> -static void tango_thermal_init(struct tango_thermal_priv *priv)
> -{
> - writel(0, priv->base + TEMPSI_CFG);
> - writel(CMD_ON, priv->base + TEMPSI_CMD);
> -}
> -
> -static int tango_thermal_probe(struct platform_device *pdev)
> -{
> - struct resource *res;
> - struct tango_thermal_priv *priv;
> - struct thermal_zone_device *tzdev;
> -
> - priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
> - if (!priv)
> - return -ENOMEM;
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - priv->base = devm_ioremap_resource(>dev, res);
> - if (IS_ERR(priv->base))
> - return PTR_ERR(priv->base);
> -
> - platform_set_drvdata(pdev, priv);
> - priv->thresh_idx = IDX_MIN;
> - tango_thermal_init(priv);
> -
> - tzdev = devm_thermal_zone_of_sensor_register(>dev, 0, priv, );
> - return PTR_ERR_OR_ZERO(tzdev);
> -}
> -
> -static int __maybe_unused tango_thermal_resume(struct device *dev)
> -{
> - tango_thermal_init(dev_get_drvdata(dev));
> - return 0;
> -}
> -
> -static SIMPLE_DEV_PM_OPS(tango_thermal_pm, NULL, tango_thermal_resume);
> -
> -static const struct of_device_id tango_sensor_ids[] = {
> - {
> - .compatible = "sigma,smp8758-thermal",
> - },
> - { /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, tango_sensor_ids);
> -
> -static struct platform_driver tango_thermal_driver = {
> - .probe = tango_thermal_probe,
> - .driver = {
> - .name = "tango-thermal",
> - .of_match_table = tango_sensor_ids,
> - .pm = _thermal_pm,
> - },
> -};
> -
> -module_platform_driver(tango_thermal_driver);
> -
> -MODULE_LICENSE("GPL");
> -MODULE_AUTHOR("Sigma Designs");
> -MODULE_DESCRIPTION("Tango temperature sensor");
> --
>
> 2.29.2
>
--
Måns Rullgård
Re: [PATCH 4/5] watchdog: remove tango driver
tangox_wdt_restart(struct watchdog_device *wdt,
> - unsigned long action, void *data)
> -{
> - struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt);
> -
> - writel(1, dev->base + WD_COUNTER);
> -
> - return 0;
> -}
> -
> -static const struct watchdog_ops tangox_wdt_ops = {
> - .start = tangox_wdt_start,
> - .stop = tangox_wdt_stop,
> - .set_timeout= tangox_wdt_set_timeout,
> - .get_timeleft = tangox_wdt_get_timeleft,
> - .restart= tangox_wdt_restart,
> -};
> -
> -static void tangox_clk_disable_unprepare(void *data)
> -{
> - clk_disable_unprepare(data);
> -}
> -
> -static int tangox_wdt_probe(struct platform_device *pdev)
> -{
> - struct tangox_wdt_device *dev;
> - u32 config;
> - int err;
> -
> - dev = devm_kzalloc(>dev, sizeof(*dev), GFP_KERNEL);
> - if (!dev)
> - return -ENOMEM;
> -
> - dev->base = devm_platform_ioremap_resource(pdev, 0);
> - if (IS_ERR(dev->base))
> - return PTR_ERR(dev->base);
> -
> - dev->clk = devm_clk_get(>dev, NULL);
> - if (IS_ERR(dev->clk))
> - return PTR_ERR(dev->clk);
> -
> - err = clk_prepare_enable(dev->clk);
> - if (err)
> - return err;
> - err = devm_add_action_or_reset(>dev,
> -tangox_clk_disable_unprepare, dev->clk);
> - if (err)
> - return err;
> -
> - dev->clk_rate = clk_get_rate(dev->clk);
> - if (!dev->clk_rate)
> - return -EINVAL;
> -
> - dev->wdt.parent = >dev;
> - dev->wdt.info = _wdt_info;
> - dev->wdt.ops = _wdt_ops;
> - dev->wdt.timeout = DEFAULT_TIMEOUT;
> - dev->wdt.min_timeout = 1;
> - dev->wdt.max_hw_heartbeat_ms = (U32_MAX - 1) / dev->clk_rate;
> -
> - watchdog_init_timeout(>wdt, timeout, >dev);
> - watchdog_set_nowayout(>wdt, nowayout);
> - watchdog_set_drvdata(>wdt, dev);
> -
> - /*
> - * Deactivate counter if disable bit is set to avoid
> - * accidental reset.
> - */
> - config = readl(dev->base + WD_CONFIG);
> - if (config & WD_CONFIG_DISABLE)
> - writel(0, dev->base + WD_COUNTER);
> -
> - writel(WD_CONFIG_XTAL_IN, dev->base + WD_CONFIG);
> -
> - /*
> - * Mark as active and restart with configured timeout if
> - * already running.
> - */
> - if (readl(dev->base + WD_COUNTER)) {
> - set_bit(WDOG_HW_RUNNING, >wdt.status);
> - tangox_wdt_start(>wdt);
> - }
> -
> - watchdog_set_restart_priority(>wdt, 128);
> -
> - watchdog_stop_on_unregister(>wdt);
> - err = devm_watchdog_register_device(>dev, >wdt);
> - if (err)
> - return err;
> -
> - platform_set_drvdata(pdev, dev);
> -
> - dev_info(>dev, "SMP86xx/SMP87xx watchdog registered\n");
> -
> - return 0;
> -}
> -
> -static const struct of_device_id tangox_wdt_dt_ids[] = {
> - { .compatible = "sigma,smp8642-wdt" },
> - { .compatible = "sigma,smp8759-wdt" },
> - { }
> -};
> -MODULE_DEVICE_TABLE(of, tangox_wdt_dt_ids);
> -
> -static struct platform_driver tangox_wdt_driver = {
> - .probe = tangox_wdt_probe,
> - .driver = {
> - .name = "tangox-wdt",
> - .of_match_table = tangox_wdt_dt_ids,
> - },
> -};
> -
> -module_platform_driver(tangox_wdt_driver);
> -
> -MODULE_AUTHOR("Mans Rullgard ");
> -MODULE_DESCRIPTION("SMP86xx/SMP87xx Watchdog driver");
> -MODULE_LICENSE("GPL");
> --
>
> 2.29.2
>
--
Måns Rullgård
Re: [PATCH net-next] net: remove aurora nb8800 driver
Arnd Bergmann writes:
> From: Arnd Bergmann
>
> The tango4 platform is getting removed, and this driver has no
> other known users, so it can be removed.
>
> Cc: Marc Gonzalez
> Cc: Mans Rullgard
> Signed-off-by: Arnd Bergmann
Acked-by: Mans Rullgard
> ---
> drivers/net/ethernet/Kconfig |1 -
> drivers/net/ethernet/Makefile|1 -
> drivers/net/ethernet/aurora/Kconfig | 23 -
> drivers/net/ethernet/aurora/Makefile |2 -
> drivers/net/ethernet/aurora/nb8800.c | 1520 --
> drivers/net/ethernet/aurora/nb8800.h | 316 --
> 6 files changed, 1863 deletions(-)
> delete mode 100644 drivers/net/ethernet/aurora/Kconfig
> delete mode 100644 drivers/net/ethernet/aurora/Makefile
> delete mode 100644 drivers/net/ethernet/aurora/nb8800.c
> delete mode 100644 drivers/net/ethernet/aurora/nb8800.h
>
> diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
> index de50e8b9e656..ad04660b97b8 100644
> --- a/drivers/net/ethernet/Kconfig
> +++ b/drivers/net/ethernet/Kconfig
> @@ -33,7 +33,6 @@ source "drivers/net/ethernet/apple/Kconfig"
> source "drivers/net/ethernet/aquantia/Kconfig"
> source "drivers/net/ethernet/arc/Kconfig"
> source "drivers/net/ethernet/atheros/Kconfig"
> -source "drivers/net/ethernet/aurora/Kconfig"
> source "drivers/net/ethernet/broadcom/Kconfig"
> source "drivers/net/ethernet/brocade/Kconfig"
> source "drivers/net/ethernet/cadence/Kconfig"
> diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
> index f8f38dcb5f8a..1e7dc8a7762d 100644
> --- a/drivers/net/ethernet/Makefile
> +++ b/drivers/net/ethernet/Makefile
> @@ -19,7 +19,6 @@ obj-$(CONFIG_NET_VENDOR_APPLE) += apple/
> obj-$(CONFIG_NET_VENDOR_AQUANTIA) += aquantia/
> obj-$(CONFIG_NET_VENDOR_ARC) += arc/
> obj-$(CONFIG_NET_VENDOR_ATHEROS) += atheros/
> -obj-$(CONFIG_NET_VENDOR_AURORA) += aurora/
> obj-$(CONFIG_NET_VENDOR_CADENCE) += cadence/
> obj-$(CONFIG_NET_VENDOR_BROADCOM) += broadcom/
> obj-$(CONFIG_NET_VENDOR_BROCADE) += brocade/
> diff --git a/drivers/net/ethernet/aurora/Kconfig
> b/drivers/net/ethernet/aurora/Kconfig
> deleted file mode 100644
> index 9ee30ea90bfa..
> --- a/drivers/net/ethernet/aurora/Kconfig
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -# SPDX-License-Identifier: GPL-2.0-only
> -config NET_VENDOR_AURORA
> - bool "Aurora VLSI devices"
> - default y
> - help
> - If you have a network (Ethernet) device belonging to this class,
> - say Y.
> -
> - Note that the answer to this question doesn't directly affect the
> - kernel: saying N will just cause the configurator to skip all
> - questions about Aurora devices. If you say Y, you will be asked
> - for your specific device in the following questions.
> -
> -if NET_VENDOR_AURORA
> -
> -config AURORA_NB8800
> - tristate "Aurora AU-NB8800 support"
> - depends on HAS_DMA
> - select PHYLIB
> - help
> - Support for the AU-NB8800 gigabit Ethernet controller.
> -
> -endif
> diff --git a/drivers/net/ethernet/aurora/Makefile
> b/drivers/net/ethernet/aurora/Makefile
> deleted file mode 100644
> index f3d599867619..
> --- a/drivers/net/ethernet/aurora/Makefile
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -# SPDX-License-Identifier: GPL-2.0-only
> -obj-$(CONFIG_AURORA_NB8800) += nb8800.o
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> deleted file mode 100644
> index 5b20185cbd62..
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ /dev/null
> @@ -1,1520 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-or-later
> -/*
> - * Copyright (C) 2015 Mans Rullgard
> - *
> - * Mostly rewritten, based on driver from Sigma Designs. Original
> - * copyright notice below.
> - *
> - * Driver for tangox SMP864x/SMP865x/SMP867x/SMP868x builtin Ethernet Mac.
> - *
> - * Copyright (C) 2005 Maxime Bizon
> - */
> -
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -
> -#include "nb8800.h"
> -
> -static void nb8800_tx_done(struct net_device *dev);
> -static int nb8800_dma_stop(struct net_device *dev);
> -
> -static inline u8 nb8800_readb(struct nb8800_priv *priv, int reg)
> -{
> - return readb_relaxed(priv->base + reg);
> -}
> -
> -static inline u32 nb8800_readl(struct nb8800_priv *priv, int reg)
> -{
> - return readl_relaxed(priv->base + reg);
> -}
> -
> -static inline void nb8800_writeb(struct nb8800_priv *priv, int reg, u8 val)
> -{
> - writeb_relaxed(val, priv->base + reg);
> -}
> -
> -static inline void nb8800_writew(struct nb8800_priv *priv, int reg, u16 val)
> -{
> - writew_relaxed(val, priv->base + reg);
> -}
> -
> -static inline void nb8800_writel(struct nb8800_priv *priv, int reg, u32 val)
> -{
> - writel_relaxed(val, priv->base + reg);
> -}
> -
> -static inline void
Re: [PATCH 1/2] media: rc: remove tango ir driver
> -
> - mode = data0 >> 1 & 7;
> - if (mode != 0)
> - return;
> -
> - toggle = data0 & 1;
> - addr = data0 >> 16;
> - cmd = data1;
> -
> - code = RC_SCANCODE_RC6_0(addr, cmd);
> - rc_keydown(ir->rc, RC_PROTO_RC6_0, code, toggle);
> -}
> -
> -static irqreturn_t tango_ir_irq(int irq, void *dev_id)
> -{
> - struct tango_ir *ir = dev_id;
> - unsigned int rc5_stat;
> - unsigned int rc6_stat;
> -
> - rc5_stat = readl_relaxed(ir->rc5_base + IR_INT);
> - writel_relaxed(rc5_stat, ir->rc5_base + IR_INT);
> -
> - rc6_stat = readl_relaxed(ir->rc6_base + RC6_CTRL);
> - writel_relaxed(rc6_stat, ir->rc6_base + RC6_CTRL);
> -
> - if (!(rc5_stat & 3) && !(rc6_stat & BIT(31)))
> - return IRQ_NONE;
> -
> - if (rc5_stat & BIT(0))
> - tango_ir_handle_rc5(ir);
> -
> - if (rc5_stat & BIT(1))
> - tango_ir_handle_nec(ir);
> -
> - if (rc6_stat & BIT(31))
> - tango_ir_handle_rc6(ir);
> -
> - return IRQ_HANDLED;
> -}
> -
> -static int tango_change_protocol(struct rc_dev *dev, u64 *rc_type)
> -{
> - struct tango_ir *ir = dev->priv;
> - u32 rc5_ctrl = DISABLE_NEC;
> - u32 rc6_ctrl = 0;
> -
> - if (*rc_type & NEC_ANY)
> - rc5_ctrl = 0;
> -
> - if (*rc_type & RC_PROTO_BIT_RC5)
> - rc5_ctrl |= ENABLE_RC5;
> -
> - if (*rc_type & RC_PROTO_BIT_RC6_0)
> - rc6_ctrl = ENABLE_RC6;
> -
> - writel_relaxed(rc5_ctrl, ir->rc5_base + IR_CTRL);
> - writel_relaxed(rc6_ctrl, ir->rc6_base + RC6_CTRL);
> -
> - return 0;
> -}
> -
> -static int tango_ir_probe(struct platform_device *pdev)
> -{
> - const char *map_name = RC_MAP_TANGO;
> - struct device *dev = >dev;
> - struct rc_dev *rc;
> - struct tango_ir *ir;
> - u64 clkrate, clkdiv;
> - int irq, err;
> - u32 val;
> -
> - irq = platform_get_irq(pdev, 0);
> - if (irq <= 0)
> - return -EINVAL;
> -
> - ir = devm_kzalloc(dev, sizeof(*ir), GFP_KERNEL);
> - if (!ir)
> - return -ENOMEM;
> -
> - ir->rc5_base = devm_platform_ioremap_resource(pdev, 0);
> - if (IS_ERR(ir->rc5_base))
> - return PTR_ERR(ir->rc5_base);
> -
> - ir->rc6_base = devm_platform_ioremap_resource(pdev, 1);
> - if (IS_ERR(ir->rc6_base))
> - return PTR_ERR(ir->rc6_base);
> -
> - ir->clk = devm_clk_get(dev, NULL);
> - if (IS_ERR(ir->clk))
> - return PTR_ERR(ir->clk);
> -
> - rc = devm_rc_allocate_device(dev, RC_DRIVER_SCANCODE);
> - if (!rc)
> - return -ENOMEM;
> -
> - of_property_read_string(dev->of_node, "linux,rc-map-name", _name);
> -
> - rc->device_name = DRIVER_NAME;
> - rc->driver_name = DRIVER_NAME;
> - rc->input_phys = DRIVER_NAME "/input0";
> - rc->map_name = map_name;
> - rc->allowed_protocols = NEC_ANY | RC_PROTO_BIT_RC5 | RC_PROTO_BIT_RC6_0;
> - rc->change_protocol = tango_change_protocol;
> - rc->priv = ir;
> - ir->rc = rc;
> -
> - err = clk_prepare_enable(ir->clk);
> - if (err)
> - return err;
> -
> - clkrate = clk_get_rate(ir->clk);
> -
> - clkdiv = clkrate * NEC_TIME_BASE;
> - do_div(clkdiv, 100);
> -
> - val = NEC_CAP(31) | GPIO_SEL(12) | clkdiv;
> - writel_relaxed(val, ir->rc5_base + IR_NEC_CTRL);
> -
> - clkdiv = clkrate * RC5_TIME_BASE;
> - do_div(clkdiv, 100);
> -
> - writel_relaxed(DISABLE_NEC, ir->rc5_base + IR_CTRL);
> - writel_relaxed(clkdiv, ir->rc5_base + IR_RC5_CLK_DIV);
> - writel_relaxed(ACK_IR_INT, ir->rc5_base + IR_INT);
> -
> - clkdiv = clkrate * RC6_TIME_BASE;
> - do_div(clkdiv, RC6_CARRIER);
> -
> - writel_relaxed(ACK_RC6_INT, ir->rc6_base + RC6_CTRL);
> - writel_relaxed((clkdiv >> 2) << 18 | clkdiv, ir->rc6_base + RC6_CLKDIV);
> -
> - err = devm_request_irq(dev, irq, tango_ir_irq, IRQF_SHARED,
> - dev_name(dev), ir);
> - if (err)
> - goto err_clk;
> -
> - err = devm_rc_register_device(dev, rc);
> - if (err)
> - goto err_clk;
> -
> - platform_set_drvdata(pdev, ir);
> - return 0;
> -
> -err_clk:
> - clk_disable_unprepare(ir->clk);
> - return err;
> -}
> -
> -static int tango_ir_remove(struct platform_device *pdev)
> -{
> - struct tango_ir *ir = platform_get_drvdata(pdev);
> -
> - clk_disable_unprepare(ir->clk);
> - return 0;
> -}
> -
> -static const struct of_device_id tango_ir_dt_ids[] = {
> - { .compatible = "sigma,smp8642-ir" },
> - { }
> -};
> -MODULE_DEVICE_TABLE(of, tango_ir_dt_ids);
> -
> -static struct platform_driver tango_ir_driver = {
> - .probe = tango_ir_probe,
> - .remove = tango_ir_remove,
> - .driver = {
> - .name = DRIVER_NAME,
> - .of_match_table = tango_ir_dt_ids,
> - },
> -};
> -module_platform_driver(tango_ir_driver);
> -
> -MODULE_DESCRIPTION("SMP86xx IR decoder driver");
> -MODULE_AUTHOR("Mans Rullgard ");
> -MODULE_LICENSE("GPL");
> --
>
> 2.29.2
>
--
Måns Rullgård
Re: [PATCH 1/2] irqchip: remove sigma tango driver
ype = IRQ_TYPE_LEVEL_MASK;
> - ct[0].handler = handle_level_irq;
> -
> - ct[1].type = IRQ_TYPE_EDGE_BOTH;
> - ct[1].handler = handle_edge_irq;
> -
> - intc_writel(chip, ct->regs.disable, 0x);
> - intc_writel(chip, ct->regs.ack, 0x);
> -}
> -
> -static void __init tangox_irq_domain_init(struct irq_domain *dom)
> -{
> - struct irq_chip_generic *gc;
> - int i;
> -
> - for (i = 0; i < 2; i++) {
> - gc = irq_get_domain_generic_chip(dom, i * 32);
> - tangox_irq_init_chip(gc, i * IRQ_CTL_HI, i * EDGE_CTL_HI);
> - }
> -}
> -
> -static int __init tangox_irq_init(void __iomem *base, struct resource
> *baseres,
> - struct device_node *node)
> -{
> - struct tangox_irq_chip *chip;
> - struct irq_domain *dom;
> - struct resource res;
> - int irq;
> - int err;
> -
> - irq = irq_of_parse_and_map(node, 0);
> - if (!irq)
> - panic("%pOFn: failed to get IRQ", node);
> -
> - err = of_address_to_resource(node, 0, );
> - if (err)
> - panic("%pOFn: failed to get address", node);
> -
> - chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> - chip->ctl = res.start - baseres->start;
> - chip->base = base;
> -
> - dom = irq_domain_add_linear(node, 64, _generic_chip_ops, chip);
> - if (!dom)
> - panic("%pOFn: failed to create irqdomain", node);
> -
> - err = irq_alloc_domain_generic_chips(dom, 32, 2, node->name,
> - handle_level_irq, 0, 0, 0);
> - if (err)
> - panic("%pOFn: failed to allocate irqchip", node);
> -
> - tangox_irq_domain_init(dom);
> -
> - irq_set_chained_handler_and_data(irq, tangox_irq_handler, dom);
> -
> - return 0;
> -}
> -
> -static int __init tangox_of_irq_init(struct device_node *node,
> - struct device_node *parent)
> -{
> - struct device_node *c;
> - struct resource res;
> - void __iomem *base;
> -
> - base = of_iomap(node, 0);
> - if (!base)
> - panic("%pOFn: of_iomap failed", node);
> -
> - of_address_to_resource(node, 0, );
> -
> - for_each_child_of_node(node, c)
> - tangox_irq_init(base, , c);
> -
> - return 0;
> -}
> -IRQCHIP_DECLARE(tangox_intc, "sigma,smp8642-intc", tangox_of_irq_init);
> --
>
> 2.29.2
>
--
Måns Rullgård
Re: [PATCH 2/4] timer: remove tango driver
Arnd Bergmann writes:
> From: Arnd Bergmann
>
> The tango platform is getting removed, so the driver is no
> longer needed.
>
> Cc: Marc Gonzalez
> Cc: Mans Rullgard
> Signed-off-by: Arnd Bergmann
Acked-by: Mans Rullgard
> ---
> drivers/clocksource/Kconfig| 8
> drivers/clocksource/Makefile | 1 -
> drivers/clocksource/timer-tango-xtal.c | 57 --
> 3 files changed, 66 deletions(-)
> delete mode 100644 drivers/clocksource/timer-tango-xtal.c
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 824a0f6b77d4..1feadd067677 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -560,14 +560,6 @@ config CLKSRC_MIPS_GIC
> select CLOCKSOURCE_WATCHDOG
> select TIMER_OF
>
> -config CLKSRC_TANGO_XTAL
> - bool "Clocksource for Tango SoC" if COMPILE_TEST
> - depends on ARM
> - select TIMER_OF
> - select CLKSRC_MMIO
> - help
> - This enables the clocksource for Tango SoC.
> -
> config CLKSRC_PXA
> bool "Clocksource for PXA or SA-11x0 platform" if COMPILE_TEST
> depends on HAS_IOMEM
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 41c154478a1a..1089f2ed8560 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -72,7 +72,6 @@ obj-$(CONFIG_KEYSTONE_TIMER)+=
> timer-keystone.o
> obj-$(CONFIG_INTEGRATOR_AP_TIMER)+= timer-integrator-ap.o
> obj-$(CONFIG_CLKSRC_VERSATILE) += timer-versatile.o
> obj-$(CONFIG_CLKSRC_MIPS_GIC)+= mips-gic-timer.o
> -obj-$(CONFIG_CLKSRC_TANGO_XTAL) += timer-tango-xtal.o
> obj-$(CONFIG_CLKSRC_IMX_GPT) += timer-imx-gpt.o
> obj-$(CONFIG_CLKSRC_IMX_TPM) += timer-imx-tpm.o
> obj-$(CONFIG_TIMER_IMX_SYS_CTR) += timer-imx-sysctr.o
> diff --git a/drivers/clocksource/timer-tango-xtal.c
> b/drivers/clocksource/timer-tango-xtal.c
> deleted file mode 100644
> index 3f94e454ef99..
> --- a/drivers/clocksource/timer-tango-xtal.c
> +++ /dev/null
> @@ -1,57 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -
> -static void __iomem *xtal_in_cnt;
> -static struct delay_timer delay_timer;
> -
> -static unsigned long notrace read_xtal_counter(void)
> -{
> - return readl_relaxed(xtal_in_cnt);
> -}
> -
> -static u64 notrace read_sched_clock(void)
> -{
> - return read_xtal_counter();
> -}
> -
> -static int __init tango_clocksource_init(struct device_node *np)
> -{
> - struct clk *clk;
> - int xtal_freq, ret;
> -
> - xtal_in_cnt = of_iomap(np, 0);
> - if (xtal_in_cnt == NULL) {
> - pr_err("%pOF: invalid address\n", np);
> - return -ENXIO;
> - }
> -
> - clk = of_clk_get(np, 0);
> - if (IS_ERR(clk)) {
> - pr_err("%pOF: invalid clock\n", np);
> - return PTR_ERR(clk);
> - }
> -
> - xtal_freq = clk_get_rate(clk);
> - delay_timer.freq = xtal_freq;
> - delay_timer.read_current_timer = read_xtal_counter;
> -
> - ret = clocksource_mmio_init(xtal_in_cnt, "tango-xtal", xtal_freq, 350,
> - 32, clocksource_mmio_readl_up);
> - if (ret) {
> - pr_err("%pOF: registration failed\n", np);
> - return ret;
> - }
> -
> - sched_clock_register(read_sched_clock, 32, xtal_freq);
> - register_current_timer_delay(_timer);
> -
> - return 0;
> -}
> -
> -TIMER_OF_DECLARE(tango, "sigma,tick-counter", tango_clocksource_init);
> --
>
> 2.29.2
>
--
Måns Rullgård
Re: Old platforms: bring out your dead
Marc Gonzalez writes: > [ Dropping maintainers of other platforms ] > > On 08/01/2021 23:55, Arnd Bergmann wrote: > >> After v5.10 was officially declared an LTS kernel, I had a look around >> the Arm platforms that look like they have not seen any patches from >> their maintainers or users that are actually running the hardware for >> at least five years (2015 or earlier). I made some statistics and lists >> for my lwn.net article last year [1], so I'd thought I'd share a summary >> here for discussion about what we should remove. As I found three >> years ago when I removed several CPU architectures, it makes sense >> to do this in bulk, to simplify a scripted search for device drivers, header >> files and Kconfig options that become unused in the process. >> >> This is probably a mix of platforms that are completely unused and >> those that just work, but I have no good way of knowing which one >> it is. Without hearing back about these, I'd propose removing all of >> these: >> >> * asm9260 -- added in 2014, no notable changes after 2015 >> * axxia -- added in 2014, no notable changes after 2015 >> * bcm/kona -- added in 2013, no notable changes after 2014 >> * digicolor -- added in 2014, no notable changes after 2015 >> * dove -- added in 2009, obsoleted by mach-mvebu in 2015 >> * efm32 -- added in 2011, first Cortex-M, no notable changes after 2013 >> * nspire -- added in 2013, no notable changes after 2015 >> * picoxcell -- added in 2011, already queued for removal >> * prima2 -- added in 20111, no notable changes since 2015 >> * spear -- added in 2010, no notable changes since 2015 >> * tango -- added in 2015, sporadic changes until 2017, but abandoned >> * u300 -- added in 2009, no notable changes since 2013 >> * vt8500 -- added in 2010, no notable changes since 2014 >> * zx --added in 2015 for both 32, 2017 for 64 bit, no notable changes >> >> If any of the above are not dead yet[2], please let me know, >> and we'll keep them. >> > > I didn't see Mans in the CC list. Not sure he's seen this message. > > As far as tango is concerned, I didn't keep any boards. > > Mans might have some tango3 and tango4 boards. > > Waiting for his take on the matter. > > I can point out some device-specific drivers that would become > useless if tango support were dropped. I have tango3 and tango4 boards. Can't say I'm using them for anything, though. With the entire platform dead at the vendor level, removal seems like a reasonable choice. -- Måns Rullgård
Re: [RESEND PATCH 4/5] ARM: dts: tango: Align L2 cache-controller nodename with dtschema
Krzysztof Kozlowski writes:
> Fix dtschema validator warnings like:
> l2-cache-controller@2010: $nodename:0:
> 'l2-cache-controller@2010' does not match
> '^(cache-controller|cpu)(@[0-9a-f,]+)*$'
>
> Signed-off-by: Krzysztof Kozlowski
Acked-by: Mans Rullgard
> ---
> arch/arm/boot/dts/tango4-common.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/tango4-common.dtsi
> b/arch/arm/boot/dts/tango4-common.dtsi
> index 54fd522badfc..d584da314500 100644
> --- a/arch/arm/boot/dts/tango4-common.dtsi
> +++ b/arch/arm/boot/dts/tango4-common.dtsi
> @@ -51,7 +51,7 @@
> };
> };
>
> - l2cc: l2-cache-controller@2010 {
> + l2cc: cache-controller@2010 {
> compatible = "arm,pl310-cache";
> reg = <0x20100000 0x1000>;
> cache-level = <2>;
> --
> 2.17.1
>
--
Måns Rullgård
Re: [PATCH] drm: sun4i: hdmi: Fix inverted HPD result
Chen-Yu Tsai writes:
> From: Chen-Yu Tsai
>
> When the extra HPD polling in sun4i_hdmi was removed, the result of
> HPD was accidentally inverted.
>
> Fix this by inverting the check.
>
> Fixes: bda8eaa6dee7 ("drm: sun4i: hdmi: Remove extra HPD polling")
> Signed-off-by: Chen-Yu Tsai
Tested-by: Mans Rullgard
> ---
>
> Sorry for the screw-up.
>
> ---
> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> index 557cbe5ab35f..2f2c9f0a1071 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> @@ -260,7 +260,7 @@ sun4i_hdmi_connector_detect(struct drm_connector
> *connector, bool force)
> unsigned long reg;
>
> reg = readl(hdmi->base + SUN4I_HDMI_HPD_REG);
> - if (reg & SUN4I_HDMI_HPD_HIGH) {
> + if (!(reg & SUN4I_HDMI_HPD_HIGH)) {
> cec_phys_addr_invalidate(hdmi->cec_adap);
> return connector_status_disconnected;
> }
> --
> 2.27.0
>
--
Måns Rullgård
Re: [PATCH] i2c: core: check returned size of emulated smbus block read
Wolfram Sang writes:
> On Sat, Jun 13, 2020 at 11:41:09AM +0100, Mans Rullgard wrote:
>> If the i2c bus driver ignores the I2C_M_RECV_LEN flag (as some of
>> them do), it is possible for an I2C_SMBUS_BLOCK_DATA read issued
>
> Out of interest, which driver did you use?
I saw it with the mv64xxx (Allwinner) and omap (Beaglebone) drivers.
>From a quick look, it seems like quite a few others have the same
problem.
>> on some random device to return an arbitrary value in the first
>> byte (and nothing else). When this happens, i2c_smbus_xfer_emulated()
>> will happily write past the end of the supplied data buffer, thus
>> causing Bad Things to happen. To prevent this, check the size
>> before copying the data block and return an error if it is too large.
>
> Good catch, we were relying on the drivers too much here. I think the
> same fix is needed for the non-emulated case as well. Will have a look.
>
>> +if (msg[1].buf[0] > I2C_SMBUS_BLOCK_MAX) {
>> +dev_err(>dev,
>> +"Invalid block size returned: %d\n",
>> +msg[1].buf[0]);
>> +status = -EINVAL;
>
> I changed this to -EPROTO as described in
> Documentation/i2c/fault-codes.rst.
>
> Applied to for-current, thanks!
>
--
Måns Rullgård
Re: [PATCH] ARM: dts: tango: Align L2 cache-controller nodename with dtschema
Krzysztof Kozlowski writes:
> Fix dtschema validator warnings like:
> l2-cache-controller@2010: $nodename:0:
> 'l2-cache-controller@2010' does not match
> '^(cache-controller|cpu)(@[0-9a-f,]+)*$'
>
> Signed-off-by: Krzysztof Kozlowski
Acked-by: Mans Rullgard
> ---
> arch/arm/boot/dts/tango4-common.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/tango4-common.dtsi
> b/arch/arm/boot/dts/tango4-common.dtsi
> index 54fd522badfc..d584da314500 100644
> --- a/arch/arm/boot/dts/tango4-common.dtsi
> +++ b/arch/arm/boot/dts/tango4-common.dtsi
> @@ -51,7 +51,7 @@
> };
> };
>
> - l2cc: l2-cache-controller@2010 {
> + l2cc: cache-controller@2010 {
> compatible = "arm,pl310-cache";
> reg = <0x20100000 0x1000>;
> cache-level = <2>;
> --
> 2.17.1
>
--
Måns Rullgård
Re: [PATCH] media: rc: Use devm_platform_ioremap_resource() in tango_ir_probe()
Markus Elfring writes: > From: Markus Elfring > Date: Wed, 18 Sep 2019 12:30:18 +0200 > > Simplify this function implementation by using a known wrapper function. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring Acked-by: Mans Rullgard > --- > drivers/media/rc/tango-ir.c | 14 ++ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/rc/tango-ir.c b/drivers/media/rc/tango-ir.c > index 451ec4e9dcfa..b8eb5bc4d9be 100644 > --- a/drivers/media/rc/tango-ir.c > +++ b/drivers/media/rc/tango-ir.c > @@ -157,20 +157,10 @@ static int tango_ir_probe(struct platform_device *pdev) > struct device *dev = >dev; > struct rc_dev *rc; > struct tango_ir *ir; > - struct resource *rc5_res; > - struct resource *rc6_res; > u64 clkrate, clkdiv; > int irq, err; > u32 val; > > - rc5_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - if (!rc5_res) > - return -EINVAL; > - > - rc6_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > - if (!rc6_res) > - return -EINVAL; > - > irq = platform_get_irq(pdev, 0); > if (irq <= 0) > return -EINVAL; > @@ -179,11 +169,11 @@ static int tango_ir_probe(struct platform_device *pdev) > if (!ir) > return -ENOMEM; > > - ir->rc5_base = devm_ioremap_resource(dev, rc5_res); > + ir->rc5_base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(ir->rc5_base)) > return PTR_ERR(ir->rc5_base); > > - ir->rc6_base = devm_ioremap_resource(dev, rc6_res); > + ir->rc6_base = devm_platform_ioremap_resource(pdev, 1); > if (IS_ERR(ir->rc6_base)) > return PTR_ERR(ir->rc6_base); > > -- > 2.23.0 > -- Måns Rullgård
Re: [PATCH -next 12/15] thermal: tango: use devm_platform_ioremap_resource() to simplify code
YueHaibing writes:
> Use devm_platform_ioremap_resource() to simplify the code a bit.
> This is detected by coccinelle.
>
> Reported-by: Hulk Robot
> Signed-off-by: YueHaibing
Acked-by: Mans Rullgard
> ---
> drivers/thermal/tango_thermal.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/tango_thermal.c b/drivers/thermal/tango_thermal.c
> index 304b461..f1b 100644
> --- a/drivers/thermal/tango_thermal.c
> +++ b/drivers/thermal/tango_thermal.c
> @@ -73,7 +73,6 @@ static void tango_thermal_init(struct tango_thermal_priv
> *priv)
>
> static int tango_thermal_probe(struct platform_device *pdev)
> {
> - struct resource *res;
> struct tango_thermal_priv *priv;
> struct thermal_zone_device *tzdev;
>
> @@ -81,8 +80,7 @@ static int tango_thermal_probe(struct platform_device *pdev)
> if (!priv)
> return -ENOMEM;
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - priv->base = devm_ioremap_resource(>dev, res);
> + priv->base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(priv->base))
> return PTR_ERR(priv->base);
>
> --
> 2.7.4
>
--
Måns Rullgård
Re: [PATCH] irqchip/tango: Add NULL check after memory operation
Hariprasad Kelam writes:
> Add NULL check after kzalloc operation.
>
> Fix below issue reported by coccicheck
> ./drivers/irqchip/irq-tango.c:189:1-5: alloc with no test, possible
> model on line 193
>
> Signed-off-by: Hariprasad Kelam
> ---
> drivers/irqchip/irq-tango.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
> index 34290f0..761b9fa 100644
> --- a/drivers/irqchip/irq-tango.c
> +++ b/drivers/irqchip/irq-tango.c
> @@ -187,6 +187,8 @@ static int __init tangox_irq_init(void __iomem *base,
> struct resource *baseres,
> panic("%pOFn: failed to get address", node);
>
> chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> chip->ctl = res.start - baseres->start;
> chip->base = base;
>
> --
Nothing checks the return value of that function, so bad things will
still happen, only more confusing to debug. If you really want to
"fix" this, you should either:
- Simply panic() like the other error cases. If anything here fails,
the system will not work anyway.
- Replace the panic() calls with error returns and check the return
value in tangox_of_irq_init(). The system will still end up unusable.
--
Måns Rullgård
Re: [PATCH] usb: musb: sunxi: propagate devicetree node to glue pdev
Mans Rullgard writes:
> In order for devicetree nodes to be correctly associated with attached
> devices, the controller node needs to be propagated to the glue device.
>
> Signed-off-by: Mans Rullgard
> ---
> This depends on 2c1ea6abde88 ("platform: set of_node in
> platform_device_register_full()") which is currently winding its way
> through the staging trees.
That patch is in v5.1, so this one can go ahead now. Assuming there are
no objections, of course.
> ---
> drivers/usb/musb/sunxi.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/musb/sunxi.c b/drivers/usb/musb/sunxi.c
> index 832a41f9ee7d..a72665fbf111 100644
> --- a/drivers/usb/musb/sunxi.c
> +++ b/drivers/usb/musb/sunxi.c
> @@ -781,6 +781,8 @@ static int sunxi_musb_probe(struct platform_device *pdev)
> pinfo.name = "musb-hdrc";
> pinfo.id= PLATFORM_DEVID_AUTO;
> pinfo.parent= >dev;
> + pinfo.fwnode= of_fwnode_handle(pdev->dev.of_node);
> + pinfo.of_node_reused = true;
> pinfo.res = pdev->resource;
> pinfo.num_res = pdev->num_resources;
> pinfo.data =
> --
> 2.20.1
>
--
Måns Rullgård
Re: [PATCH 0/5] Exynos EHCI/OHCI: resolve conflict with the generic USB device bindings
Marek Szyprowski writes:
> Hi Måns
>
> On 2019-05-21 15:30, Måns Rullgård wrote:
>> Marek Szyprowski writes:
>>> Dear All,
>>>
>>> Commit 69bec7259853 ("USB: core: let USB device know device node") added
>>> support for attaching devicetree node for USB devices. Those nodes are
>>> children of their USB host controller. However Exynos EHCI and OHCI
>>> driver bindings already define child-nodes for each physical root hub
>>> port and assigns respective PHY controller and parameters to them. This
>>> leads to the conflict. A workaround for it has been merged as commit
>>> 01d4071486fe ("usb: exynos: add workaround for the USB device bindings
>>> conflict"), but it disabled support for USB device binding for Exynos
>>> EHCI/OHCI controllers.
>>>
>>> This patchset tries to resolve this binding conflict by changing Exynos
>>> EHCI/OHCI bindings: PHYs are moved from the sub-nodes to a standard array
>>> under the 'phys' property. Such solution has been suggested by Måns
>>> Rullgård in the following thread: https://lkml.org/lkml/2019/5/13/228
>>>
>>> To keep everything working during the transitional time, the changes has
>>> been split into 2 steps. First step (patches 1-3) need to be merged before
>>> the second one (patches 4-5). Patches from each step can be merged to
>>> respective trees without any dependencies - the only requirement is that
>>> second step has to be merged after merging all patches from the first one.
>>>
>>> This patchset has been tested on various Exynos4 boards with different
>>> USB host controller configurations (Odroids family: X2, U3, XU3).
>>>
>>> Best regards
>>> Marek Szyprowski
>>> Samsung R Institute Poland
>>>
>>> Marek Szyprowski (5):
>>>dt-bindings: switch Exynos EHCI/OHCI bindings to use array of generic
>>> PHYs
>>>ARM: dts: exynos: Add array of generic PHYs to EHCI/OHCI devices
>>>usb: exynos: add support for getting PHYs from the standard dt array
>>>ARM: dts: exynos: Remove obsolete port sub-nodes from EHCI/OHCI
>>> devices
>>>usb: exynos: Remove support for legacy PHY bindings
>> You could retain compatibility with old devicetrees (which may be
>> useful) by using the "phys" property if it exists and falling back
>> on the old method if it doesn't. Then you would get this sequence
>> of changes:
>>
>> 1. Update binding definition.
>> 2. Support new binding in driver, with fallback to old.
>> 3. Switch dts files to new binding.
>
> This is exactly what I did in this patchset. Until Patch #5 is applied,
> Exynos EHCI/OHCI drivers supports both ways of getting PHYs and is fully
> compatible with existing DTBs. This last patch should be applied at
> least one release later that the first 3 patches to keep everything
> working during the -rcX time.
I'm suggesting you keep the fallback in the driver. It does no harm,
and it's contained in one place.
On the dts side, you're adding the new phys property without removing
the old-style nodes at first. If you put the driver change first, the
dts could be switched to the new style in one patch without a confusing
hybrid ever existing.
> Compatibility with so called old DTBs is not so important, because there
> are no boards with Exynos4 and Exynos5 SoCs, which would not update DTB
> together with the kernel zImage. There have been already some
> significant compatibility breaks related to those SoCs during last years.
You can't possibly know what's out there. Besides, isn't the general
policy to not break compatibility without a very good reason?
--
Måns Rullgård
Re: [PATCH 0/5] Exynos EHCI/OHCI: resolve conflict with the generic USB device bindings
Marek Szyprowski writes:
> Dear All,
>
> Commit 69bec7259853 ("USB: core: let USB device know device node") added
> support for attaching devicetree node for USB devices. Those nodes are
> children of their USB host controller. However Exynos EHCI and OHCI
> driver bindings already define child-nodes for each physical root hub
> port and assigns respective PHY controller and parameters to them. This
> leads to the conflict. A workaround for it has been merged as commit
> 01d4071486fe ("usb: exynos: add workaround for the USB device bindings
> conflict"), but it disabled support for USB device binding for Exynos
> EHCI/OHCI controllers.
>
> This patchset tries to resolve this binding conflict by changing Exynos
> EHCI/OHCI bindings: PHYs are moved from the sub-nodes to a standard array
> under the 'phys' property. Such solution has been suggested by Måns
> Rullgård in the following thread: https://lkml.org/lkml/2019/5/13/228
>
> To keep everything working during the transitional time, the changes has
> been split into 2 steps. First step (patches 1-3) need to be merged before
> the second one (patches 4-5). Patches from each step can be merged to
> respective trees without any dependencies - the only requirement is that
> second step has to be merged after merging all patches from the first one.
>
> This patchset has been tested on various Exynos4 boards with different
> USB host controller configurations (Odroids family: X2, U3, XU3).
>
> Best regards
> Marek Szyprowski
> Samsung R Institute Poland
>
> Marek Szyprowski (5):
> dt-bindings: switch Exynos EHCI/OHCI bindings to use array of generic
> PHYs
> ARM: dts: exynos: Add array of generic PHYs to EHCI/OHCI devices
> usb: exynos: add support for getting PHYs from the standard dt array
> ARM: dts: exynos: Remove obsolete port sub-nodes from EHCI/OHCI
> devices
> usb: exynos: Remove support for legacy PHY bindings
You could retain compatibility with old devicetrees (which may be
useful) by using the "phys" property if it exists and falling back
on the old method if it doesn't. Then you would get this sequence
of changes:
1. Update binding definition.
2. Support new binding in driver, with fallback to old.
3. Switch dts files to new binding.
--
Måns Rullgård
Re: [PATCH v2] usb: core: verify devicetree nodes for disabled interfaces
Marek Szyprowski writes:
> Hi
>
> On 2019-05-08 13:46, Måns Rullgård wrote:
>> Marek Szyprowski writes:
>>> Commit 01fdf179f4b0 ("usb: core: skip interfaces disabled in devicetree")
>>> add support for disabling given USB device interface by adding nodes to
>>> the USB host controller device. The mentioned commit however identifies
>>> the given USB interface node only by the 'reg' property in the host
>>> controller children nodes and then checks for their the 'status'. The USB
>>> device interface nodes however also has to have a 'compatible' property as
>>> described in Documentation/devicetree/bindings/usb/usb-device.txt. This is
>>> important, because USB host controller might have child-nodes for other
>>> purposes. For example, Exynos EHCI and OHCI drivers already define
>>> child-nodes for each physical root hub port and assigns respective PHY
>>> controller and parameters for them. This conflicts with the proposed
>>> approach and verifying for the presence of the compatible property fixes
>>> this issue without changing the bindings and the way the PHY controllers
>>> are handled by Exynos EHCI/OHCI drivers.
>>>
>>> Reported-by: Markus Reichl
>>> Fixes: 01fdf179f4b0 ("usb: core: skip interfaces disabled in devicetree")
>>> Signed-off-by: Marek Szyprowski
>>> ---
>>> drivers/usb/core/message.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
>>> index e844bb7b5676..6f7d047392bd 100644
>>> --- a/drivers/usb/core/message.c
>>> +++ b/drivers/usb/core/message.c
>>> @@ -2009,6 +2009,7 @@ int usb_set_configuration(struct usb_device *dev, int
>>> configuration)
>>> struct usb_interface *intf = cp->interface[i];
>>>
>>> if (intf->dev.of_node &&
>>> + of_find_property(intf->dev.of_node, "compatible", NULL) &&
>>> !of_device_is_available(intf->dev.of_node)) {
>>> dev_info(>dev, "skipping disabled interface %d\n",
>>> intf->cur_altsetting->desc.bInterfaceNumber);
>>> --
>> I don't think this is the right approach. We don't want to be adding
>> such checks everywhere the of_node is used. A better way might be to
>> not set of_node at all in the absence of a proper "compatible" string.
>
> Right, this will be a better approach. I've just checked the code and a
> simple check for 'compatible' property presence can be easily added in
> drivers/usb/core/of.c in usb_of_get_device_node() and
> usb_of_get_interface_node() functions.
>
> The second check could be added in drivers/usb/core/hub.c in
> usb_new_device() - to ensure that the device's vid/pid matches of_node
> compatible string.
>
> Is this okay? Or just add a latter one?
I'm not sure where the best place to check is. Someone else will have
to weigh in on that.
>> Then there's the problem of how to resolve the incompatibility between
>> the generic USB and Exynos bindings. One possible fix could be to use
>> a child node of the controller node to represent the root hub. Since
>> the driver currently doesn't work at all if a devicetree has nodes for
>> USB devices, there should be no compatibility concerns.
>
> So far we don't have any use case for adding devicetree nodes for usb
> devices under Exynos EHCI/OHCI hcd, so this shouldn't be a problem for now.
None that you know of, that is. Regardless, the bindings are
inconsistent, and that needs to be fixed.
--
Måns Rullgård
Re: [PATCH 23/23] watchdog: tangox_wdt: Convert to use device managed functions and other improvements
Guenter Roeck writes:
> Use device managed functions to simplify error handling, reduce
> source code size, improve readability, and reduce the likelyhood of bugs.
> Other improvements as listed below.
>
> The conversion was done automatically with coccinelle using the
> following semantic patches. The semantic patches and the scripts
> used to generate this commit log are available at
> https://github.com/groeck/coccinelle-patches
>
> - Drop assignments to otherwise unused variables
> - Drop unnecessary braces around conditional return statements
> - Drop empty remove function
> - Use devm_add_action_or_reset() for calls to clk_disable_unprepare
> - Replace stop on remove with call to watchdog_stop_on_unregister()
> - Use devm_watchdog_register_driver() to register watchdog device
>
> Cc: Marc Gonzalez
> Cc: Mans Rullgard
> Signed-off-by: Guenter Roeck
> ---
> drivers/watchdog/tangox_wdt.c | 37 ++---
> 1 file changed, 14 insertions(+), 23 deletions(-)
Acked-by: Mans Rullgard
> diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
> index 16611fe0d9d1..1afb0e9d808c 100644
> --- a/drivers/watchdog/tangox_wdt.c
> +++ b/drivers/watchdog/tangox_wdt.c
> @@ -108,6 +108,11 @@ static const struct watchdog_ops tangox_wdt_ops = {
> .restart= tangox_wdt_restart,
> };
>
> +static void tangox_clk_disable_unprepare(void *data)
> +{
> + clk_disable_unprepare(data);
> +}
> +
> static int tangox_wdt_probe(struct platform_device *pdev)
> {
> struct tangox_wdt_device *dev;
> @@ -129,12 +134,14 @@ static int tangox_wdt_probe(struct platform_device
> *pdev)
> err = clk_prepare_enable(dev->clk);
> if (err)
> return err;
> + err = devm_add_action_or_reset(>dev,
> +tangox_clk_disable_unprepare, dev->clk);
> + if (err)
> + return err;
>
> dev->clk_rate = clk_get_rate(dev->clk);
> - if (!dev->clk_rate) {
> - err = -EINVAL;
> - goto err;
> - }
> + if (!dev->clk_rate)
> + return -EINVAL;
>
> dev->wdt.parent = >dev;
> dev->wdt.info = _wdt_info;
> @@ -168,31 +175,16 @@ static int tangox_wdt_probe(struct platform_device
> *pdev)
>
> watchdog_set_restart_priority(>wdt, 128);
>
> - err = watchdog_register_device(>wdt);
> + watchdog_stop_on_unregister(>wdt);
> + err = devm_watchdog_register_device(>dev, >wdt);
> if (err)
> - goto err;
> + return err;
>
> platform_set_drvdata(pdev, dev);
>
> dev_info(>dev, "SMP86xx/SMP87xx watchdog registered\n");
>
> return 0;
> -
> - err:
> - clk_disable_unprepare(dev->clk);
> - return err;
> -}
> -
> -static int tangox_wdt_remove(struct platform_device *pdev)
> -{
> - struct tangox_wdt_device *dev = platform_get_drvdata(pdev);
> -
> - tangox_wdt_stop(>wdt);
> - clk_disable_unprepare(dev->clk);
> -
> - watchdog_unregister_device(>wdt);
> -
> - return 0;
> }
>
> static const struct of_device_id tangox_wdt_dt_ids[] = {
> @@ -204,7 +196,6 @@ MODULE_DEVICE_TABLE(of, tangox_wdt_dt_ids);
>
> static struct platform_driver tangox_wdt_driver = {
> .probe = tangox_wdt_probe,
> - .remove = tangox_wdt_remove,
> .driver = {
> .name = "tangox-wdt",
> .of_match_table = tangox_wdt_dt_ids,
> --
> 2.7.4
>
--
Måns Rullgård
Re: [PATCH v2 1/3] ARM: use arch_extension directive instead of arch argument
Stefan Agner writes:
> The LLVM Target parser currently does not allow to specify the security
> extension as part of -march (see also LLVM Bug 40186 [0]). When trying
> to use Clang with LLVM's integrated assembler, this leads to build
> errors such as this:
> clang-8: error: the clang compiler does not support '-Wa,-march=armv7-a+sec'
>
> Use ".arch_extension sec" to enable the security extension in a more
> portable fasion. Also make sure to use ".arch armv7-a" in case a v6/v7
> multi-platform kernel is being built.
>
> Note that this is technically not exactly the same as the old code
> checked for availabilty of the security extension by calling as-instr.
> However, there are already other sites which use ".arch_extension sec"
> unconditionally, hence de-facto we need an assembler capable of
> ".arch_extension sec" already today (arch/arm/mm/proc-v7.S). The
> arch extension "sec" is available since binutils 2.21 according to
> its documentation [1].
>
> [0] https://bugs.llvm.org/show_bug.cgi?id=40186
> [1] https://sourceware.org/binutils/docs-2.21/as/ARM-Options.html
>
> Signed-off-by: Stefan Agner
> Acked-by: Mans Rullgard
> Acked-by: Arnd Bergmann
> Acked-by: Krzysztof Kozlowski
> ---
> Changes since v1:
> - Explicitly specify assembler architecture as armv7-a to avoid
> build issues when bulding v6/v7 multi arch kernel.
>
> arch/arm/mach-bcm/Makefile | 3 ---
> arch/arm/mach-bcm/bcm_kona_smc.c | 2 --
> arch/arm/mach-exynos/Makefile | 4
> arch/arm/mach-exynos/exynos-smc.S | 3 ++-
> arch/arm/mach-exynos/sleep.S | 3 ++-
> arch/arm/mach-highbank/Makefile| 3 ---
> arch/arm/mach-highbank/smc.S | 3 ++-
> arch/arm/mach-keystone/Makefile| 3 ---
> arch/arm/mach-keystone/smc.S | 1 +
> arch/arm/mach-omap2/Makefile | 8
> arch/arm/mach-omap2/omap-headsmp.S | 2 ++
> arch/arm/mach-omap2/omap-smc.S | 3 ++-
> arch/arm/mach-omap2/sleep33xx.S| 1 +
> arch/arm/mach-omap2/sleep34xx.S| 2 ++
> arch/arm/mach-omap2/sleep43xx.S| 2 ++
> arch/arm/mach-omap2/sleep44xx.S| 2 ++
> arch/arm/mach-tango/Makefile | 3 ---
> arch/arm/mach-tango/smc.S | 1 +
> 18 files changed, 19 insertions(+), 30 deletions(-)
[...]
> diff --git a/arch/arm/mach-bcm/bcm_kona_smc.c
> b/arch/arm/mach-bcm/bcm_kona_smc.c
> index a55a7ecf146a..541e850a736c 100644
> --- a/arch/arm/mach-bcm/bcm_kona_smc.c
> +++ b/arch/arm/mach-bcm/bcm_kona_smc.c
> @@ -125,9 +125,7 @@ static int bcm_kona_do_smc(u32 service_id, u32
> buffer_phys)
> __asmeq("%2", "r4")
> __asmeq("%3", "r5")
> __asmeq("%4", "r6")
> -#ifdef REQUIRES_SEC
> ".arch_extension sec\n"
> -#endif
> " smc#0\n"
> : "=r" (ip), "=r" (r0)
> : "r" (r4), "r" (r5), "r" (r6)
[...]
> diff --git a/arch/arm/mach-keystone/smc.S b/arch/arm/mach-keystone/smc.S
> index d15de8179fab..ec03dc499270 100644
> --- a/arch/arm/mach-keystone/smc.S
> +++ b/arch/arm/mach-keystone/smc.S
> @@ -21,6 +21,7 @@
> *
> * Return: Non zero value on failure
> */
> + .arch_extension sec
> ENTRY(keystone_cpu_smc)
> stmfd sp!, {r4-r11, lr}
> smc #0
[...]
> diff --git a/arch/arm/mach-tango/smc.S b/arch/arm/mach-tango/smc.S
> index 361a8dc89804..cf2d21e5226c 100644
> --- a/arch/arm/mach-tango/smc.S
> +++ b/arch/arm/mach-tango/smc.S
> @@ -1,6 +1,7 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> #include
>
> + .arch_extension sec
> ENTRY(tango_smc)
> push{lr}
> mov ip, r1
Is there some reason these three don't need the .arch directive?
--
Måns Rullgård
Re: [PATCH 1/3] ARM: use arch_extension directive instead of arch argument
Stefan Agner writes:
> The LLVM Target parser currently does not allow to specify the security
> extension as part of -march (see also LLVM Bug 40186 [0]). When trying
> to use Clang with LLVM's integrated assembler, this leads to a build
> errors such as this:
> clang-8: error: the clang compiler does not support '-Wa,-march=armv7-a+sec'
>
> Use ".arch_extension sec" to enable the security extension in a more
> portable fasion.
>
> Note that this is technically not exactly the same as the old code
> checked for availabilty of the security extension by calling as-instr.
> However, there are already other sites which use ".arch_extension sec"
> unconditionally, hence de-facto we need an assembler capable of
> ".arch_extension sec" already today (arch/arm/mm/proc-v7.S). The
> arch extension "sec" is available since binutils 2.21 according to
> its documentation [1].
>
> [0] https://bugs.llvm.org/show_bug.cgi?id=40186
> [1] https://sourceware.org/binutils/docs-2.21/as/ARM-Options.html
>
> Signed-off-by: Stefan Agner
> ---
> arch/arm/mach-bcm/Makefile | 3 ---
> arch/arm/mach-bcm/bcm_kona_smc.c | 2 --
> arch/arm/mach-exynos/Makefile | 4
> arch/arm/mach-exynos/exynos-smc.S | 2 +-
> arch/arm/mach-exynos/sleep.S | 2 +-
> arch/arm/mach-highbank/Makefile| 3 ---
> arch/arm/mach-highbank/smc.S | 2 +-
> arch/arm/mach-keystone/Makefile| 3 ---
> arch/arm/mach-keystone/smc.S | 1 +
> arch/arm/mach-omap2/Makefile | 8
> arch/arm/mach-omap2/omap-headsmp.S | 1 +
> arch/arm/mach-omap2/omap-smc.S | 2 +-
> arch/arm/mach-omap2/sleep34xx.S| 1 +
> arch/arm/mach-omap2/sleep43xx.S| 1 +
> arch/arm/mach-omap2/sleep44xx.S| 1 +
> arch/arm/mach-tango/Makefile | 3 ---
> arch/arm/mach-tango/smc.S | 1 +
> 17 files changed, 10 insertions(+), 30 deletions(-)
[...]
> diff --git a/arch/arm/mach-tango/Makefile b/arch/arm/mach-tango/Makefile
> index da6c633d3cc0..97cd04508fa1 100644
> --- a/arch/arm/mach-tango/Makefile
> +++ b/arch/arm/mach-tango/Makefile
> @@ -1,7 +1,4 @@
> # SPDX-License-Identifier: GPL-2.0
> -plus_sec := $(call as-instr,.arch_extension sec,+sec)
> -AFLAGS_smc.o := -Wa,-march=armv7-a$(plus_sec)
> -
> obj-y += setup.o smc.o
> obj-$(CONFIG_SMP) += platsmp.o
> obj-$(CONFIG_SUSPEND) += pm.o
> diff --git a/arch/arm/mach-tango/smc.S b/arch/arm/mach-tango/smc.S
> index 361a8dc89804..cf2d21e5226c 100644
> --- a/arch/arm/mach-tango/smc.S
> +++ b/arch/arm/mach-tango/smc.S
> @@ -1,6 +1,7 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> #include
>
> + .arch_extension sec
> ENTRY(tango_smc)
> push{lr}
> mov ip, r1
For Tango:
Acked-by: Mans Rullgard
--
Måns Rullgård
Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property
Maxime Ripard writes:
> On Thu, Mar 14, 2019 at 04:09:13PM +0000, Måns Rullgård wrote:
>> Maxime Ripard writes:
>>
>> > On Mon, Mar 11, 2019 at 04:11:06PM +, Måns Rullgård wrote:
>> >> Maxime Ripard writes:
>> >>
>> >> > Hi!
>> >> >
>> >> > On Mon, Mar 11, 2019 at 01:47:13PM +, Mans Rullgard wrote:
>> >> >> Sometimes it is desirabled to use a separate i2c controller for ddc
>> >> >> access. This adds support for the ddc-i2c-bus property of the
>> >> >> hdmi-connector node, using the specified controller if provided.
>> >> >>
>> >> >> Signed-off-by: Mans Rullgard
>> >> >> ---
>> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 +
>> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 37 +++---
>> >> >> 2 files changed, 35 insertions(+), 3 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >> >> b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >> >> index b685ee11623d..b08c4453d47c 100644
>> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> >> >> @@ -269,6 +269,7 @@ struct sun4i_hdmi {
>> >> >>struct clk *tmds_clk;
>> >> >>
>> >> >>struct i2c_adapter *i2c;
>> >> >> + struct i2c_adapter *ddc_i2c;
>> >> >>
>> >> >>/* Regmap fields for I2C adapter */
>> >> >>struct regmap_field *field_ddc_en;
>> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >> >> b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >> >> index 061d2e0d9011..5b2fac79f5d6 100644
>> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> >> >> @@ -212,7 +212,7 @@ static int sun4i_hdmi_get_modes(struct
>> >> >> drm_connector *connector)
>> >> >>struct edid *edid;
>> >> >>int ret;
>> >> >>
>> >> >> - edid = drm_get_edid(connector, hdmi->i2c);
>> >> >> + edid = drm_get_edid(connector, hdmi->ddc_i2c ?: hdmi->i2c);
>> >> >
>> >> > You can't test whether ddc_i2c is NULL or not...
>> >> >
>> >> >>if (!edid)
>> >> >>return 0;
>> >> >>
>> >> >> @@ -228,6 +228,28 @@ static int sun4i_hdmi_get_modes(struct
>> >> >> drm_connector *connector)
>> >> >>return ret;
>> >> >> }
>> >> >>
>> >> >> +static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev)
>> >> >> +{
>> >> >> + struct device_node *phandle, *remote;
>> >> >> + struct i2c_adapter *ddc;
>> >> >> +
>> >> >> + remote = of_graph_get_remote_node(dev->of_node, 1, -1);
>> >> >> + if (!remote)
>> >> >> + return ERR_PTR(-EINVAL);
>> >> >> +
>> >> >> + phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0);
>> >> >> + of_node_put(remote);
>> >> >> + if (!phandle)
>> >> >> + return NULL;
>> >> >> +
>> >> >> + ddc = of_get_i2c_adapter_by_node(phandle);
>> >> >> + of_node_put(phandle);
>> >> >> + if (!ddc)
>> >> >> + return ERR_PTR(-EPROBE_DEFER);
>> >> >> +
>> >> >> + return ddc;
>> >> >
>> >> > ... Since even in (most) error cases you're returning a !NULL pointer.
>> >> >
>> >> >> +}
>> >> >> +
>> >> >> static const struct drm_connector_helper_funcs
>> >> >> sun4i_hdmi_connector_helper_funcs = {
>> >> >>.get_modes = sun4i_hdmi_get_modes,
>> >> >> };
>> >> >> @@ -575,6 +597,12 @@ static int sun4i_hdmi_bind(struct device *dev,
>> >> >> struct device *master,
>> >> >>goto err_disable_mod_clk;
>> >> >>
Re: [PATCH 3/3] auxdisplay: charlcd: make backlight initial state configurable
Miguel Ojeda writes: > On Tue, Mar 12, 2019 at 4:48 PM Måns Rullgård wrote: >> >> The current code unconditionally flashes the light once. I though it >> best to keep that behaviour as default, even if it's not seen as ideal. > > Sent into -next. If no one else says anything after a few days, I will > send the series for -rc2. > > By the way, what about the other comment Andy mentioned? i.e. > "defined(_OFF)" (in case you missed to answer it). With a correct config, there should be no difference. If someone managed to create a config without any of the alternatives selected, a compiler error would result. I don't really have much of an opinion on which behaviour is preferable in that situation. -- Måns Rullgård
Re: [PATCH v2 0/4] Patches to allow consistent mmc / mmcblk numbering w/ device tree
Stefan Agner writes:
> On 16.03.2019 16:39, Russell King - ARM Linux admin wrote:
>> On Sat, Mar 16, 2019 at 01:33:58PM +0100, Marek Vasut wrote:
>>> If you have a FS or partition table there, it does.
>>> If you don't, I agree ... that's a problem.
>>
>> eMMC boot partitions are called mmcblkXbootY, and unless you have more
>> than one eMMC device on the system, they can be found either by looking
>> for /dev/mmcblk*boot* or by querying udev. The advantage of using udev
>> is you can discover the physical device behind it by looking at DEVPATH,
>> ID_PATH, etc, but you may not have that installed on an embedded device.
>>
>> However, as I say, just looking for /dev/mmcblk*boot* is sufficient to
>> find the eMMC boot partitions where there is just one eMMC device
>> present (which seems to be the standard setup.)
>>
>>> > I don't care the slightest what the numbering is, as long as it is
>>> > stable. On some hardware, with an unpatched kernel, the mmc device
>>> > numbering changes depending on whether or not an SD card is inserted on
>>> > boot. Getting rid of that behaviour is really all I want.
>>>
>>> Agreed, that would be an improvement.
>>
>> The mmc device numbering was tied to the mmc host numbering a while back
>> and the order that the hosts are probed should be completely independent
>> of whether a card is inserted or not:
>>
>> snprintf(md->disk->disk_name, sizeof(md->disk->disk_name),
>> "mmcblk%u%s", card->host->index, subname ? subname : "");
>>
>> snprintf(rpmb_name, sizeof(rpmb_name),
>> "mmcblk%u%s", card->host->index, subname ? subname : "");
>>
>> I suspect that Mans is quoting something from the dim and distant past
>> to confuse the issue - as shown above, it is now dependent on the host
>> numbering order not the order in which cards are inserted.
>
> Commit 9aaf3437aa72 ("mmc: block: Use the mmc host device index as the
> mmcblk device index") which came in with v4.6 enables constant mmc block
> device numbering. I can confirm that it works nicely, and it improved
> the situation a lot.
That's the answer I was looking for. I guess we can drop these patches
from our kernels then.
> That being said, we still use a patch downstream which allows
> renumbering using an alias. We deal with a bunch of different boards
> with different SoC's. I have a couple of SD cards with various rootfs
> and use internal eMMC boot quite often as well. Remembering which board
> uses which numbering is a pain. Maintaining a patch is just easier...
> Furthermore, U-Boot allows reordering and all boards I deal with use mmc
> 0 for the internal eMMC. The aliases allow consistency.
Since pretty much every other device type supports renumbering with DT
aliases, it would make sense to do this for mmc as well.
--
Måns Rullgård
Re: [PATCH 3/3] auxdisplay: charlcd: make backlight initial state configurable
Andy Shevchenko writes: > On Fri, Mar 1, 2019 at 9:14 PM Mans Rullgard wrote: >> >> The charlcd driver currently flashes the backlight once on init. >> This may not be desirable. Thus, add options for turning the >> backlight off or on as well. >> >> Signed-off-by: Mans Rullgard >> --- >> drivers/auxdisplay/Kconfig | 21 + >> drivers/auxdisplay/charlcd.c | 10 +- >> 2 files changed, 30 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig >> index 7d3fe27d6868..c52c738e554a 100644 >> --- a/drivers/auxdisplay/Kconfig >> +++ b/drivers/auxdisplay/Kconfig >> @@ -445,6 +445,27 @@ config PANEL_BOOT_MESSAGE >> An empty message will only clear the display at driver init time. >> Any other >> printf()-formatted message is valid with newline and escape codes. >> >> +choice >> + prompt "Backlight initial state" >> + default CHARLCD_BL_FLASH > > LGTM, but I don't agree on this default. I would prefer either on or > off, not flashing for sure. > Though it seems the case before the patch... The current code unconditionally flashes the light once. I though it best to keep that behaviour as default, even if it's not seen as ideal. -- Måns Rullgård
Re: [PATCH 1/3] auxdisplay: deconfuse configuration
Miguel Ojeda writes: > On Fri, Mar 1, 2019 at 7:48 PM Mans Rullgard wrote: >> >> The auxdisplay Kconfig is confusing. It creates two separate menus >> even though the settings are closely related. Moreover, the options >> for setting the boot message depend on CONFIG_PARPORT even though they >> are used by drivers that do not. >> >> Clear up the confustion by moving the "Parallel port LCD/Keypad" menu > > "confustion" -> "confusion" Darn, must have been confused while typing. >> under auxdisplay where it logically belongs. Change the boot message >> options to depend only on CONFIG_CHARLCD, making them accessible also >> when only the HD44780 is selected. >> >> Since the "Parallel port LCD/Keypad" driver now has a new dependency >> on CONFIG_AUXDISPLAY, rename its Kconfig symbol and keep the old one >> such that make oldconfig will not disable the driver. >> >> Signed-off-by: Mans Rullgard >> --- >> drivers/auxdisplay/Kconfig | 17 - >> drivers/auxdisplay/Makefile | 2 +- >> 2 files changed, 13 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig >> index 57410f9c5d44..7d3fe27d6868 100644 >> --- a/drivers/auxdisplay/Kconfig >> +++ b/drivers/auxdisplay/Kconfig >> @@ -164,9 +164,7 @@ config ARM_CHARLCD >> line and the Linux version on the second line, but that's >> still useful. >> >> -endif # AUXDISPLAY >> - >> -menuconfig PANEL >> +menuconfig PARPORT_PANEL > > Do we want the PARPORT_ prefix here but not on the suboptions? I was trying to bring some sanity to it without changing more than necessary. > Also, having PANEL_PARPORT and PARPORT_PANEL seems confusing... It is... >> tristate "Parallel port LCD/Keypad Panel support" >> depends on PARPORT >> select CHARLCD >> @@ -178,7 +176,7 @@ menuconfig PANEL >> compiled as a module, or linked into the kernel and started at >> boot. >> If you don't understand what all this is about, say N. >> >> -if PANEL >> +if PARPORT_PANEL >> >> config PANEL_PARPORT >> int "Default parallel port number (0=LPT1)" >> @@ -419,8 +417,11 @@ config PANEL_LCD_PIN_BL >> >> Default for the 'BL' pin in custom profile is '0' (uncontrolled). >> >> +endif # PARPORT_PANEL >> + >> config PANEL_CHANGE_MESSAGE >> bool "Change LCD initialization message ?" >> + depends on CHARLCD >> default "n" >> ---help--- >> This allows you to replace the boot message indicating the kernel >> version >> @@ -444,7 +445,13 @@ config PANEL_BOOT_MESSAGE >> An empty message will only clear the display at driver init time. >> Any other >> printf()-formatted message is valid with newline and escape codes. >> >> -endif # PANEL >> +endif # AUXDISPLAY >> + >> +config PANEL >> + tristate "Parallel port LCD/Keypad Panel support (OLD OPTION)" > > Hm... what do you mean by "OLD OPTION"? Should we keep it? (I don't > see any other place using this marking). The option is there so 'make oldconfig' on existing configurations doesn't silently drop that driver since it now depends on AUXDISPLAY. There have been similar cases when shuffling options. Maybe they used a different tag. We could of course let the three people using that driver deal with it themselves. >> + depends on PARPORT >> + select AUXDISPLAY >> + select PARPORT_PANEL > > I agree the menu was a bit convoluted and we didn't get to clean it. > > Since you are touching the panel.c options, CC'ing the maintainers > (please do run get_maintainer.pl in that case!) I always run get_maintainer.pl on patches. Sometimes it isn't clever enough to figure out all the people who might be interested. -- Måns Rullgård
Re: [PATCH 3/3] auxdisplay: charlcd: make backlight initial state configurable
Miguel Ojeda writes: > On Fri, Mar 1, 2019 at 7:48 PM Mans Rullgard wrote: >> >> +#ifdef CONFIG_CHARLCD_BL_ON >> +#define LCD_INIT_BL "\x1b[L+" >> +#elif defined (CONFIG_CHARLCD_BL_FLASH) > > Style nitpick: no space after "elif defined". Do you mind if I change > it before sending it to linux-next? Otherwise, looks fine to me. > Thanks! Of course I don't mind. -- Måns Rullgård
Re: [PATCH v2 0/4] Patches to allow consistent mmc / mmcblk numbering w/ device tree
Douglas Anderson writes: > This series picks patches from various different places to produce what > I consider the best solution to getting consistent mmc and mmcblk > ordering. > > Why consistent ordering and why not just use UUIDs? IMHO consistent > ordering solves a few different problems: > > 1. For poor, feeble-minded humans like me, have sane numbering for >devices helps a lot. When grepping through dmesg it's terribly handy >if a given SDMMC device has a consistent number. I know that I can >do "dmesg | grep mmc0" or "dmesg | grep mmcblk0" to find info about >the eMMC. I know that I can do "dmesg | grep mmc1" to find info >about the SD card slot. I don't want it to matter which one probed >first, I don't want it to matter if I'm working on a variant of the >hardware that has the SD card slot disabled, and I don't want to care >what my boot device was. Worrying about what device number I got >increases my cognitive load. > > 2. There are cases where it's not trivially easy during development to >use the UUID. Specifically I work a lot with coreboot / depthcharge >as a BIOS. When configured properly, that BIOS has a nice feature to >allow you to fetch the kernel and kernel command line from TFTP by >pressing Ctrl-N. In this particular case the BIOS doesn't actually >know which disk I'd like for my root filesystem, so it's not so easy >for it to put the right UUID into the command line. For this >purpose, knowing that "mmcblk0" will always refer to eMMC is handy. > > Changes in v2: > - Rebased atop mmc-next > - Stat dynamic allocation after fixed allocation; thanks Wolfram! > - rk3288 patch new for v2 > > Douglas Anderson (1): > ARM: dts: rockchip: Add mmc aliases for rk3288 platform > > Jaehoon Chung (1): > Documentation: mmc: Document mmc aliases > > Stefan Agner (2): > mmc: read mmc alias from device tree > mmc: use SD/MMC host ID for block device name ID > > Documentation/devicetree/bindings/mmc/mmc.txt | 11 +++ > arch/arm/boot/dts/rk3288.dtsi | 4 > drivers/mmc/card/block.c | 2 +- > drivers/mmc/core/host.c | 17 - > 4 files changed, 32 insertions(+), 2 deletions(-) Did anyone ever come up with an acceptable solution for this? After three years, I'm getting tired of rebasing these patches onto every new kernel. UUIDs or similar are NOT an option for multiple reasons: - We have two rootfs partitions for ping-pong updates, so simply referring to "the thing with ID foo" doesn't work. - Installing said updates needs direct access the device/partition, which may not even have a filesystem. - The u-boot environment is stored in an eMMC "boot" partition, and userspace needs to know where to find it. I'm sure I'm not the only one in a similar situation. Russel, feel free to shout abuse at me. I don't care, but it makes you look stupid. -- Måns Rullgård
Re: [PATCH] can: ti_hecc: fix close when napi poll is active
Jeroen Hofstee writes:
> When closing this CAN interface while napi poll is active, for example with:
> `ip link set can0 down` several interfaces freeze. This seemed to be caused
> by napi_disable called from ti_hecc_close expecting the scheduled probe to
> either return quota or call napi_complete. Since the poll functions has a
> check for netif_running it returns 0 and doesn't call napi_complete and hence
> violates the napi its expectation.
>
> So remove this check, so either napi_complete is called or quota is returned.
>
> Signed-off-by: Jeroen Hofstee
> ---
> drivers/net/can/ti_hecc.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
> index db6ea93..42813d3 100644
> --- a/drivers/net/can/ti_hecc.c
> +++ b/drivers/net/can/ti_hecc.c
> @@ -603,9 +603,6 @@ static int ti_hecc_rx_poll(struct napi_struct *napi, int
> quota)
> u32 mbx_mask;
> unsigned long pending_pkts, flags;
>
> - if (!netif_running(ndev))
> - return 0;
> -
> while ((pending_pkts = hecc_read(priv, HECC_CANRMP)) &&
> num_pkts < quota) {
> mbx_mask = BIT(priv->rx_next); /* next rx mailbox to process */
> --
> 2.7.4
This seems to have been lost or forgotten.
--
Måns Rullgård
Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517
Tony Lindgren writes:
> * Jeroen Hofstee [161028 11:19]:
>> Hello Tony,
>>
>> On 28-10-16 17:52, Tony Lindgren wrote:
>> > * Jeroen Hofstee [161028 08:33]:
>> > > Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
>> > > id to common file") did not only move the code for an am3517, it also
>> > > added the slave parameter, resulting in an invalid (all zero) mac address
>> > > being returned for an am3517, since it only has a single emac and the
>> > > slave
>> > > parameter is pointing to the second. So simply always read the first and
>> > > valid mac-address for a ti,am3517-emac.
>> > And others davinci_emac.c users can have more than one. So is the
>> > reason the slave parameter points to the second instance because
>> > of the location in the hardware?
>>
>> Sort of, the slave parameter gets determined by the fact if there is one
>> or two register range(s) associated with the davinci_emac. In davinci_emac.c
>>
>> res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> ...
>> rc = davinci_emac_try_get_mac(pdev, res_ctrl ? 0 : 1,
>> priv->mac_addr);
>>
>> So it there are two ranges, the slave param becomes 0. It there is only one,
>> it
>> will be 1. Since the am3517 only has a single regs entry it ends up with
>> slave 1,
>> while there is only a single davinci_emac.
>
> OK thanks for clarifying it:
>
> Acked-by: Tony Lindgren
What happened to this patch?
--
Måns Rullgård
Re: [RFC][PATCH] dt-bindings: usb: add non-removable device property
Greg Kroah-Hartman writes:
> On Thu, Feb 28, 2019 at 03:22:24PM +0000, Måns Rullgård wrote:
>> Greg Kroah-Hartman writes:
>>
>> > On Thu, Feb 28, 2019 at 02:33:44PM +, Mans Rullgard wrote:
>> >> Add a boolean property indicating that a device is hardwired to the
>> >> upstream port. Although hubs can provide this information, they are not
>> >> always configured correctly. An alternate means of indicating this for
>> >> built-in USB devices is thus useful.
>> >>
>> >> Signed-off-by: Mans Rullgard
>> >> ---
>> >> I have a situation where userspace would like to know which USB devices
>> >> are built-in, but the on-board hub doesn't have the right setting.
>> >> Also, the hub itself can't be indicated as fixed in any other way that
>> >> I'm aware of.
>> >
>> > Then that's a firmware bug, right? We have a way for firmware to export
>> > this to the USB core, why not use that? Your on-board hub should get a
>> > firmware update with this information, let's not try to create
>> > yet-another-way to define this type of information please.
>>
>> What firmware? This is not an ACPI system, obviously, so DT _is_ the
>> firmware.
>
> Firmware in your hub. There's a whole crazy software stack in that
> beast :)
The hub chip itself (SMSC/Microchip USB2512B in the case at hand) is
fine. The problem is that whoever designed the PCB didn't add the
pull-ups marking the ports non-removable. Besides, the hub can't
indicate that it itself is hardwired to the host port. That information
needs to be supplied elsewhere.
>> >> In a way, adding this property seems a bit silly since dt can only
>> >> sensibly be used for hardwired devices in the first place. Thus the
>> >> mere presence of a dt node could be taken to indicate the same thing.
>> >> On the other hand, it's conceivable that someone might dynamically
>> >> generate a devicetree based on what happens to be connected on boot or
>> >> something. For that reason, and explicit property seems safer.
>> >> ---
>> >> Documentation/devicetree/bindings/usb/usb-device.txt | 8
>> >> 1 file changed, 8 insertions(+)
>> >
>> > Can you show some code actually using this? Again, this should "just
>> > work" for USB today unless your platform is really broken (and if it is,
>> > go complain to the vendor...)
>>
>> You know full well that complaining to the vendor is rarely something
>> that works. Especially not when there are already thousands of the
>> devices in the field.
>
> Understood, but constantly working around broken hardware is annoying at
> times.
It's annoying, sure. It is also the reality, and we have to deal with it.
Ignoring such hardware won't make it go away.
>> This is how I meant to use it:
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 3adff4da2ee1..81ef3cb705b7 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -2392,6 +2392,14 @@ static void set_usb_port_removable(struct usb_device
>> *udev)
>> break;
>> }
>>
>> + /*
>> +* Otherwise, check whether DT indicates this device is
>> non-removable.
>> +*/
>> + if (of_property_read_bool(udev->dev.of_node, "non-removable")) {
>> + udev->removable = USB_DEVICE_FIXED;
>> + return;
>> + }
>
> Shouldn't this be an attribute of the USB hub's port, not the device
> itself? That's the way it works with ACPI, and odds are any description
> of USB devices in DT is also going to look much like how ACPI describes
> the devices, let's not try to diverge when it is not necessary.
Fine with me. That's why I asked.
How about a non-removable-ports property in the hub node listing the
hardwired ports?
--
Måns Rullgård
Re: [RFC][PATCH] dt-bindings: usb: add non-removable device property
Greg Kroah-Hartman writes:
> On Thu, Feb 28, 2019 at 02:33:44PM +, Mans Rullgard wrote:
>> Add a boolean property indicating that a device is hardwired to the
>> upstream port. Although hubs can provide this information, they are not
>> always configured correctly. An alternate means of indicating this for
>> built-in USB devices is thus useful.
>>
>> Signed-off-by: Mans Rullgard
>> ---
>> I have a situation where userspace would like to know which USB devices
>> are built-in, but the on-board hub doesn't have the right setting.
>> Also, the hub itself can't be indicated as fixed in any other way that
>> I'm aware of.
>
> Then that's a firmware bug, right? We have a way for firmware to export
> this to the USB core, why not use that? Your on-board hub should get a
> firmware update with this information, let's not try to create
> yet-another-way to define this type of information please.
What firmware? This is not an ACPI system, obviously, so DT _is_ the
firmware.
>> In a way, adding this property seems a bit silly since dt can only
>> sensibly be used for hardwired devices in the first place. Thus the
>> mere presence of a dt node could be taken to indicate the same thing.
>> On the other hand, it's conceivable that someone might dynamically
>> generate a devicetree based on what happens to be connected on boot or
>> something. For that reason, and explicit property seems safer.
>> ---
>> Documentation/devicetree/bindings/usb/usb-device.txt | 8
>> 1 file changed, 8 insertions(+)
>
> Can you show some code actually using this? Again, this should "just
> work" for USB today unless your platform is really broken (and if it is,
> go complain to the vendor...)
You know full well that complaining to the vendor is rarely something
that works. Especially not when there are already thousands of the
devices in the field.
This is how I meant to use it:
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 3adff4da2ee1..81ef3cb705b7 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2392,6 +2392,14 @@ static void set_usb_port_removable(struct usb_device
*udev)
break;
}
+ /*
+* Otherwise, check whether DT indicates this device is non-removable.
+*/
+ if (of_property_read_bool(udev->dev.of_node, "non-removable")) {
+ udev->removable = USB_DEVICE_FIXED;
+ return;
+ }
+
/*
* Otherwise, check whether the hub knows whether a port is removable
* or not
--
Måns Rullgård
Re: [PATCH] USB: serial: option: set driver_info for SIM5218 and compatibles
Johan Hovold writes:
> Adding Bjørn.
>
> On Wed, Feb 27, 2019 at 11:57:16AM +0000, Måns Rullgård wrote:
>> Johan Hovold writes:
>>
>> > On Tue, Feb 26, 2019 at 05:07:10PM +, Mans Rullgard wrote:
>> >> The SIMCom SIM5218 and compatible devices have 5 USB interfaces, only 4
>> >> of which are serial ports. The fifth is a network interface supported
>> >> by the qmi-wwan driver. Furthermore, the serial ports do not support
>> >> modem control signals. Add driver_info flags to reflect this.
>> >>
>> >> Signed-off-by: Mans Rullgard
>> >> ---
>> >> drivers/usb/serial/option.c | 3 ++-
>> >> 1 file changed, 2 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
>> >> index fb544340888b..af4cbfecc3ff 100644
>> >> --- a/drivers/usb/serial/option.c
>> >> +++ b/drivers/usb/serial/option.c
>> >> @@ -1066,7 +1066,8 @@ static const struct usb_device_id option_ids[] = {
>> >> .driver_info = RSVD(3) },
>> >> { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x6613)}, /* Onda H600/ZTE MF330 */
>> >> { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x0023)}, /* ONYX 3G device */
>> >> - { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x9000)}, /* SIMCom SIM5218 */
>> >> + { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x9000), /* SIMCom SIM5218 */
>> >> + .driver_info = NCTRL(0) | NCTRL(1) | NCTRL(2) | NCTRL(3) | RSVD(4) },
>> >> /* Quectel products using Qualcomm vendor ID */
>> >> { USB_DEVICE(QUALCOMM_VENDOR_ID, QUECTEL_PRODUCT_UC15)},
>> >> { USB_DEVICE(QUALCOMM_VENDOR_ID, QUECTEL_PRODUCT_UC20),
>> >
>> > Could you please provide the output of usb-devices (or lsusb -v) for
>> > this device?
>>
>> lsusb -v:
>> [...]
> So the patch looks fine to me. The fifth interface is QMI, but hasn't
> been available for use until now then, and this seems to have been the
> vendors idea from the start:
>
>
> http://www.microchip.ua/simcom/WCDMA/APPNOTES/SIMCom_3G_Linux_driver_Application%20Note_V1.00.pdf
That document predates the qmi-wwan driver in the kernel. Note that
this driver has an ID table entry for interface 4 of this device. Right
now, whichever driver is probed first claims that interface. I haven't
actually tried using the QMI interface, though.
> And you're seeing errors when opening ports 0-3 due to the DTR calls
> which I guess no one noticed or cared about before?
Right, some userspace tools complain about this.
> Before you sent me the lsusb I searched for it and came across the below
> thread where Bjørn's having a go at SIMCom. In it there's output from a
> second device using the same id but with entirely different descriptors.
>
>
> https://forum.openwrt.org/t/lte-wireless-module-support-by-openwrt-led-on-tplink/13586?page=3
>
> If this is a common theme with this vendor we may need to be extra
> careful when making changes.
Isn't this a common theme with most USB vendors, especially wireless things?
Regardless, setting the NCTRL flag should be harmless.
--
Måns Rullgård
Re: [PATCH] USB: serial: option: set driver_info for SIM5218 and compatibles
rfaceClass 255 Vendor Specific Class bInterfaceSubClass255 Vendor Specific Subclass bInterfaceProtocol255 Vendor Specific Protocol iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x83 EP 3 IN bmAttributes2 Transfer TypeBulk Synch Type None Usage Type Data wMaxPacketSize 0x0200 1x 512 bytes bInterval 32 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x03 EP 3 OUT bmAttributes2 Transfer TypeBulk Synch Type None Usage Type Data wMaxPacketSize 0x0200 1x 512 bytes bInterval 32 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber3 bAlternateSetting 0 bNumEndpoints 3 bInterfaceClass 255 Vendor Specific Class bInterfaceSubClass255 Vendor Specific Subclass bInterfaceProtocol255 Vendor Specific Protocol iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x84 EP 4 IN bmAttributes3 Transfer TypeInterrupt Synch Type None Usage Type Data wMaxPacketSize 0x0040 1x 64 bytes bInterval 5 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x85 EP 5 IN bmAttributes2 Transfer TypeBulk Synch Type None Usage Type Data wMaxPacketSize 0x0200 1x 512 bytes bInterval 32 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x04 EP 4 OUT bmAttributes2 Transfer TypeBulk Synch Type None Usage Type Data wMaxPacketSize 0x0200 1x 512 bytes bInterval 32 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber4 bAlternateSetting 0 bNumEndpoints 3 bInterfaceClass 255 Vendor Specific Class bInterfaceSubClass255 Vendor Specific Subclass bInterfaceProtocol255 Vendor Specific Protocol iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x86 EP 6 IN bmAttributes3 Transfer TypeInterrupt Synch Type None Usage Type Data wMaxPacketSize 0x0040 1x 64 bytes bInterval 5 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x87 EP 7 IN bmAttributes2 Transfer TypeBulk Synch Type None Usage Type Data wMaxPacketSize 0x0200 1x 512 bytes bInterval 32 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x05 EP 5 OUT bmAttributes2 Transfer TypeBulk Synch Type None Usage Type Data wMaxPacketSize 0x0200 1x 512 bytes bInterval 32 Device Qualifier (for other device speed): bLength10 bDescriptorType 6 bcdUSB 2.00 bDeviceClass0 bDeviceSubClass 0 bDeviceProtocol 0 bMaxPacketSize064 bNumConfigurations 1 can't get debug descriptor: Resource temporarily unavailable Device Status: 0x (Bus Powered) -- Måns Rullgård
Re: [PATCH v3] platform: set of_node in platform_device_register_full()
"Rafael J. Wysocki" writes:
> On Thu, Feb 21, 2019 at 12:29 PM Mans Rullgard wrote:
>>
>> If the provided fwnode is an OF node, set dev.of_node as well.
>>
>> Also add an of_node_reused flag to struct platform_device_info and copy
>> this to the new device. This is needed to avoid pinctrl settings being
>> requested twice. See 4e75e1d7dac9 ("driver core: add helper to reuse a
>> device-tree node") for a longer explanation.
>>
>> Some drivers are just shims that create extra "glue" devices with the
>> DT device as parent and have the real driver bind to these. In these
>> cases, the glue device needs to get a reference to the original DT node
>> in order for the main driver to access properties and child nodes.
>>
>> For example, the sunxi-musb driver creates such a glue device using
>> platform_device_register_full(). Consequently, devices attached to
>> this USB interface don't get associated with DT nodes, if present,
>> the way they do with EHCI.
>>
>> This change will allow sunxi-musb and similar drivers to easily
>> propagate the DT node to child devices as required.
>>
>> Signed-off-by: Mans Rullgard
>
> Generally
>
> Reviewed-by: Rafael J. Wysocki
>
> but I'm a bit concerned about the existing users of
> platform_device_register_full() who may pass a valid DT fwnode to it
> and not expect of_node to be set. Arguably, they should expect it to
> be set, but then there may be some fallout resulting from this change.
I did a quick search, and found three drivers using
platform_device_register_full() that pass an fwnode that isn't
explicitly an ACPI handle.
The Cadence MACB and pxa2xx-spi drivers both set fwnode in their PCI
glue. Near as I can tell, setting of_node as a result should be
harmless. If someone has a devicetree describing the PCI topology,
these drivers would start picking properties set there just as they do
when using a platform bus. One could argue that this is the correct
behaviour.
The third case is Qcom HIDMA management driver. It contains this code
fragment:
for_each_available_child_of_node(np, child) {
struct platform_device *new_pdev;
/* irrelevant code */
memset(, 0, sizeof(pdevinfo));
pdevinfo.fwnode = >fwnode;
pdevinfo.parent = pdev_parent ? _parent->dev : NULL;
pdevinfo.name = child->name;
pdevinfo.id = object_counter++;
pdevinfo.res = res;
pdevinfo.num_res = 3;
pdevinfo.data = NULL;
pdevinfo.size_data = 0;
pdevinfo.dma_mask = DMA_BIT_MASK(64);
new_pdev = platform_device_register_full();
if (IS_ERR(new_pdev)) {
ret = PTR_ERR(new_pdev);
goto out;
}
of_node_get(child);
new_pdev->dev.of_node = child;
of_dma_configure(_pdev->dev, child, true);
/*
* It is assumed that calling of_msi_configure is safe on
* platforms with or without MSI support.
*/
of_msi_configure(_pdev->dev, child);
of_node_put(child);
}
This already looks rather weird. It manually sets of_node of the new
device _after_ it has been added. The reason this works at all is that
the matching driver hasn't been registered yet. This, to me, is
slightly confusing, if not actually broken. Worse is that the of_node
is copied without an of_node_get() to match the of_node_put() in
platform_device_put(), should it ever be called. Moreover, these
devices are not removed if the module is unloaded or if there is an
error partway through the loop.
That said, it doesn't seem like setting of_node in
platform_device_register_full() should cause any (new) problems here.
--
Måns Rullgård
Re: [PATCH] platform: set of_node in platform_device_register_full()
"Rafael J. Wysocki" writes:
> On Wed, Feb 20, 2019 at 1:12 PM Måns Rullgård wrote:
>>
>> Johan Hovold writes:
>>
>> > On Wed, Feb 20, 2019 at 11:35:06AM +, Mans Rullgard wrote:
>> >> If the provided fwnode is an OF node, set dev.of_node as well.
>> >>
>> >> Some drivers are just shims that create extra "glue" devices with the
>> >> DT device as parent and have the real driver bind to these. In these
>> >> cases, the glue device needs to get a reference to the original DT node
>> >> in order for the main driver to access properties and child nodes.
>> >>
>> >> For example, the sunxi-musb driver creates such a glue device using
>> >> platform_device_register_full(). Consequently, devices attached to
>> >> this USB interface don't get associated with DT nodes, if present,
>> >> the way they do with EHCI.
>> >>
>> >> This change will allow sunxi-musb and similar driver to easily
>> >> propagate the DT node to child devices as required.
>> >
>> > Just a drive-by comment, didn't look to closely at this patch, but this
>> > all sounds familiar.
>> >
>> > Note that if both platform devices are bound to drivers you may end up
>> > with some resources like pinctrl which are handled automatically by
>> > driver core at probe time to be requested twice (and failing the second
>> > time).
>> >
>> > Take a look at 4e75e1d7dac9 ("driver core: add helper to reuse a
>> > device-tree node"), which provides a means to avoid this, and
>> > 49484abd93ab ("USB: musb: dsps: propagate device-tree node").
>>
>> Thanks, and ugh. So we should be setting the of_node_reused flag when
>> this is the case. It's easy for the musb-dsps driver since it doesn't
>> use platform_device_register_full() and can do this before the
>> device_add() call. How can we convey that this flag needs to be set?
>
> Through pdevinfo I guess?
Not without adding another field to it. The most direct is of course to
simply add an of_node_reused flag there too and copy it over. Would
that be OK, or is there a better way?
--
Måns Rullgård
Re: [PATCH] platform: set of_node in platform_device_register_full()
Johan Hovold writes:
> On Wed, Feb 20, 2019 at 11:35:06AM +, Mans Rullgard wrote:
>> If the provided fwnode is an OF node, set dev.of_node as well.
>>
>> Some drivers are just shims that create extra "glue" devices with the
>> DT device as parent and have the real driver bind to these. In these
>> cases, the glue device needs to get a reference to the original DT node
>> in order for the main driver to access properties and child nodes.
>>
>> For example, the sunxi-musb driver creates such a glue device using
>> platform_device_register_full(). Consequently, devices attached to
>> this USB interface don't get associated with DT nodes, if present,
>> the way they do with EHCI.
>>
>> This change will allow sunxi-musb and similar driver to easily
>> propagate the DT node to child devices as required.
>
> Just a drive-by comment, didn't look to closely at this patch, but this
> all sounds familiar.
>
> Note that if both platform devices are bound to drivers you may end up
> with some resources like pinctrl which are handled automatically by
> driver core at probe time to be requested twice (and failing the second
> time).
>
> Take a look at 4e75e1d7dac9 ("driver core: add helper to reuse a
> device-tree node"), which provides a means to avoid this, and
> 49484abd93ab ("USB: musb: dsps: propagate device-tree node").
Thanks, and ugh. So we should be setting the of_node_reused flag when
this is the case. It's easy for the musb-dsps driver since it doesn't
use platform_device_register_full() and can do this before the
device_add() call. How can we convey that this flag needs to be set?
>> Signed-off-by: Mans Rullgard
>> ---
>> drivers/base/platform.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> index dff82a3c2caa..853a1d0e5845 100644
>> --- a/drivers/base/platform.c
>> +++ b/drivers/base/platform.c
>> @@ -512,6 +512,7 @@ struct platform_device *platform_device_register_full(
>>
>> pdev->dev.parent = pdevinfo->parent;
>> pdev->dev.fwnode = pdevinfo->fwnode;
>> +pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
>>
>> if (pdevinfo->dma_mask) {
>> /*
>
> Johan
--
Måns Rullgård
Re: [PATCH] platform: set of_node in platform_device_register_full()
Mans Rullgard writes:
> If the provided fwnode is an OF node, set dev.of_node as well.
>
> Some drivers are just shims that create extra "glue" devices with the
> DT device as parent and have the real driver bind to these. In these
> cases, the glue device needs to get a reference to the original DT node
> in order for the main driver to access properties and child nodes.
>
> For example, the sunxi-musb driver creates such a glue device using
> platform_device_register_full(). Consequently, devices attached to
> this USB interface don't get associated with DT nodes, if present,
> the way they do with EHCI.
>
> This change will allow sunxi-musb and similar driver to easily
> propagate the DT node to child devices as required.
>
> Signed-off-by: Mans Rullgard
> ---
> drivers/base/platform.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index dff82a3c2caa..853a1d0e5845 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -512,6 +512,7 @@ struct platform_device *platform_device_register_full(
>
> pdev->dev.parent = pdevinfo->parent;
> pdev->dev.fwnode = pdevinfo->fwnode;
> + pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
>
> if (pdevinfo->dma_mask) {
> /*
> --
Sorry, I forgot to add a v2 to this. Anyway, the only change is the
commit message.
--
Måns Rullgård
Re: [PATCH] platform: set of_node in platform_device_register_full()
"Rafael J. Wysocki" writes: > On Wed, Feb 20, 2019 at 11:41 AM Måns Rullgård wrote: >> >> "Rafael J. Wysocki" writes: >> >> > On Mon, Feb 18, 2019 at 12:10 PM Måns Rullgård wrote: >> >> >> >> "Rafael J. Wysocki" writes: >> >> >> >> > On Sat, Feb 16, 2019 at 5:50 PM Mans Rullgard wrote: >> >> >> >> >> >> If the provided fwnode is an OF node, set dev.of_node as well. >> >> >> >> >> >> Signed-off-by: Mans Rullgard >> >> >> --- >> >> >> drivers/base/platform.c | 1 + >> >> >> 1 file changed, 1 insertion(+) >> >> >> >> >> >> diff --git a/drivers/base/platform.c b/drivers/base/platform.c >> >> >> index dff82a3c2caa..853a1d0e5845 100644 >> >> >> --- a/drivers/base/platform.c >> >> >> +++ b/drivers/base/platform.c >> >> >> @@ -512,6 +512,7 @@ struct platform_device >> >> >> *platform_device_register_full( >> >> >> >> >> >> pdev->dev.parent = pdevinfo->parent; >> >> >> pdev->dev.fwnode = pdevinfo->fwnode; >> >> >> + pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode)); >> >> > >> >> > of_node_get() generally does a kobject_get() on the node's kobject, so >> >> > when is that reference dropped? Or if it doesn't need to be dropped >> >> > at all, why is this the case? >> >> >> >> platform_device_release() calls of_device_node_put(). >> > >> > Yes, it does, but this is the reference that's already acquired for >> > devices added while parsing DT, isn't it? >> > >> > Your change adds an extra reference AFAICS. >> > >> > Also, why is this patch needed? >> >> Some drivers are just shims that create extra "glue" devices with the DT >> device as parent and have the real driver bind to these. In other >> cases, the same real driver matches the DT node directly. When a glue >> device is used, it needs to get a reference to the original DT node in >> order for the main driver to access properties and child nodes. >> >> Right now, my problem is that the suxi-musb driver creates such a glue >> device for the musb core driver to bind to without setting of_node. >> This means devices attached to this USB interface don't get associated >> with DT nodes, if present, the way they do with EHCI. > > You really should describe problems that you want to address in patch > changelogs. This helps a lot to understand the motivation for the > changes. Do you want me to send a new patch with the above explanation in the commit message? >> The sunxi-musb driver uses platform_device_register_full(), so this >> seemed like the easiest way to let it set of_node of the new device. >> Since this creates a second reference to the same node, of_node_get() >> is required. > > But what about devices that already have of_node set at this point? > > Maybe check if of_node is NULL before trying to set it? It's a brand new device allocated a few lines above. of_node has to be null here. -- Måns Rullgård
Re: [PATCH] ARM: dts: sun7i: add pinctrl for missing uart mux options
Maxime Ripard writes:
> On Wed, Feb 20, 2019 at 04:58:49PM +0800, Chen-Yu Tsai wrote:
>> On Sun, Feb 17, 2019 at 2:21 AM Mans Rullgard wrote:
>> >
>> > This adds pinctrl settings for various missing uart options.
>> >
>> > Signed-off-by: Mans Rullgard
>> > ---
>> > arch/arm/boot/dts/sun7i-a20.dtsi | 45
>> > 1 file changed, 45 insertions(+)
>> >
>> > diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi
>> > b/arch/arm/boot/dts/sun7i-a20.dtsi
>> > index af5b067a5f83..2295ff5adf48 100644
>> > --- a/arch/arm/boot/dts/sun7i-a20.dtsi
>> > +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
>> > @@ -944,6 +944,31 @@
>> > function = "uart0";
>> > };
>> >
>> > + uart0_pf_pins: uart0-pf-pins {
>> > + pins = "PF2", "PF4";
>> > + function = "uart0";
>> > + };
>>
>> We've had the policy of not adding pinctrl nodes that aren't used in-tree,
>> to avoid bloating up the blob size. However DTC 1.4.7 introduced the new
>> /omit-if-no-ref/ directive, which would make the compiler discard marked
>> nodes if they aren't referenced.
>>
>> So please add this to all the new nodes. It seems to work regardless whether
>> you add it before or after the label, though having it after the label seems
>> to make vim syntax highlighting happier.
Should we also add this to existing nodes?
BTW, I really do need all those uart pin options.
--
Måns Rullgård
Re: [PATCH] platform: set of_node in platform_device_register_full()
"Rafael J. Wysocki" writes: > On Mon, Feb 18, 2019 at 12:10 PM Måns Rullgård wrote: >> >> "Rafael J. Wysocki" writes: >> >> > On Sat, Feb 16, 2019 at 5:50 PM Mans Rullgard wrote: >> >> >> >> If the provided fwnode is an OF node, set dev.of_node as well. >> >> >> >> Signed-off-by: Mans Rullgard >> >> --- >> >> drivers/base/platform.c | 1 + >> >> 1 file changed, 1 insertion(+) >> >> >> >> diff --git a/drivers/base/platform.c b/drivers/base/platform.c >> >> index dff82a3c2caa..853a1d0e5845 100644 >> >> --- a/drivers/base/platform.c >> >> +++ b/drivers/base/platform.c >> >> @@ -512,6 +512,7 @@ struct platform_device *platform_device_register_full( >> >> >> >> pdev->dev.parent = pdevinfo->parent; >> >> pdev->dev.fwnode = pdevinfo->fwnode; >> >> + pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode)); >> > >> > of_node_get() generally does a kobject_get() on the node's kobject, so >> > when is that reference dropped? Or if it doesn't need to be dropped >> > at all, why is this the case? >> >> platform_device_release() calls of_device_node_put(). > > Yes, it does, but this is the reference that's already acquired for > devices added while parsing DT, isn't it? > > Your change adds an extra reference AFAICS. > > Also, why is this patch needed? Some drivers are just shims that create extra "glue" devices with the DT device as parent and have the real driver bind to these. In other cases, the same real driver matches the DT node directly. When a glue device is used, it needs to get a reference to the original DT node in order for the main driver to access properties and child nodes. Right now, my problem is that the suxi-musb driver creates such a glue device for the musb core driver to bind to without setting of_node. This means devices attached to this USB interface don't get associated with DT nodes, if present, the way they do with EHCI. The sunxi-musb driver uses platform_device_register_full(), so this seemed like the easiest way to let it set of_node of the new device. Since this creates a second reference to the same node, of_node_get() is required. If you'd prefer solving this in some other way, please tell me how. -- Måns Rullgård
Re: [PATCH] ARM: dts: sun7i: add pinctrl for missing uart mux options
Maxime Ripard writes:
> On Wed, Feb 20, 2019 at 04:58:49PM +0800, Chen-Yu Tsai wrote:
>> On Sun, Feb 17, 2019 at 2:21 AM Mans Rullgard wrote:
>> >
>> > This adds pinctrl settings for various missing uart options.
>> >
>> > Signed-off-by: Mans Rullgard
>> > ---
>> > arch/arm/boot/dts/sun7i-a20.dtsi | 45
>> > 1 file changed, 45 insertions(+)
>> >
>> > diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi
>> > b/arch/arm/boot/dts/sun7i-a20.dtsi
>> > index af5b067a5f83..2295ff5adf48 100644
>> > --- a/arch/arm/boot/dts/sun7i-a20.dtsi
>> > +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
>> > @@ -944,6 +944,31 @@
>> > function = "uart0";
>> > };
>> >
>> > + uart0_pf_pins: uart0-pf-pins {
>> > + pins = "PF2", "PF4";
>> > + function = "uart0";
>> > + };
>>
>> We've had the policy of not adding pinctrl nodes that aren't used in-tree,
>> to avoid bloating up the blob size. However DTC 1.4.7 introduced the new
>> /omit-if-no-ref/ directive, which would make the compiler discard marked
>> nodes if they aren't referenced.
>>
>> So please add this to all the new nodes. It seems to work regardless whether
>> you add it before or after the label, though having it after the label seems
>> to make vim syntax highlighting happier.
>
> It also works (both for dtc and vim) if we put it on the previous
> line, so something like:
>
> /omit-if-no-ref/
> uart0_pf_pins: uart0-pf-pins {
> };
>
> And it does have the advantage of keeping the same line width, which
> could get pretty long on some nodes.
Thanks, I'd missed that directive. It always seems needlessly annoying
to make everybody working on a new board to add those nodes separately.
--
Måns Rullgård
Re: [PATCH] usb: core: skip interfaces disabled in devicetree
Greg Kroah-Hartman writes:
> On Sat, Feb 16, 2019 at 05:04:52PM +, Mans Rullgard wrote:
>> If an interface has an associated devicetree node with status disabled,
>> do not register the device. This is useful for boards with a built-in
>> multifunction USB device where some functions are broken or otherwise
>> undesired.
>>
>> Signed-off-by: Mans Rullgard
>> ---
>> drivers/usb/core/message.c | 4
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
>> index bfa5eda0cc26..6b45d4835e41 100644
>> --- a/drivers/usb/core/message.c
>> +++ b/drivers/usb/core/message.c
>> @@ -2007,6 +2007,10 @@ int usb_set_configuration(struct usb_device *dev, int
>> configuration)
>> for (i = 0; i < nintf; ++i) {
>> struct usb_interface *intf = cp->interface[i];
>>
>> +if (intf->dev.of_node &&
>> +!of_device_is_available(intf->dev.of_node))
>> +continue;
>
> Shouldn't you at least print some message out saying you are skipping
> this? Odds are this is going to cause regressions in devices that were
> not expecting this, right? So pointing them at why their devices now no
> longer work would be good :)
They will only be skipped if there is a device tree node for the
interface _and_ it has and explicit status = "disabled" property.
The default is still to create devices for all interfaces.
That said, printing a message is probably a good idea anyway. Would
"info" level be appropriate for this?
--
Måns Rullgård
Re: [PATCH] platform: set of_node in platform_device_register_full()
"Rafael J. Wysocki" writes: > On Sat, Feb 16, 2019 at 5:50 PM Mans Rullgard wrote: >> >> If the provided fwnode is an OF node, set dev.of_node as well. >> >> Signed-off-by: Mans Rullgard >> --- >> drivers/base/platform.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/base/platform.c b/drivers/base/platform.c >> index dff82a3c2caa..853a1d0e5845 100644 >> --- a/drivers/base/platform.c >> +++ b/drivers/base/platform.c >> @@ -512,6 +512,7 @@ struct platform_device *platform_device_register_full( >> >> pdev->dev.parent = pdevinfo->parent; >> pdev->dev.fwnode = pdevinfo->fwnode; >> + pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode)); > > of_node_get() generally does a kobject_get() on the node's kobject, so > when is that reference dropped? Or if it doesn't need to be dropped > at all, why is this the case? platform_device_release() calls of_device_node_put(). -- Måns Rullgård
Re: [PATCH -next] irqchip/tango: Fix potential NULL pointer dereference
Marc Zyngier writes:
> On Tue, 29 Jan 2019 08:01:22 +,
> YueHaibing wrote:
>>
>> There is a potential NULL pointer dereference in case kzalloc()
>> fails and returns NULL.
>>
>> Fixes: 4bba66899ac6 ("irqchip/tango: Add support for Sigma Designs
>> SMP86xx/SMP87xx interrupt controller")
>> Signed-off-by: YueHaibing
>> ---
>> drivers/irqchip/irq-tango.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
>> index ae28d86..a63b828 100644
>> --- a/drivers/irqchip/irq-tango.c
>> +++ b/drivers/irqchip/irq-tango.c
>> @@ -191,6 +191,8 @@ static int __init tangox_irq_init(void __iomem *base,
>> struct resource *baseres,
>> panic("%pOFn: failed to get address", node);
>>
>> chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>> +if (!chip)
>> +return -ENOMEM;
>> chip->ctl = res.start - baseres->start;
>> chip->base = base;
>>
>
> This is a commendable effort, but given that the whole error handling
> of this driver is just to simply panic, I have the ugly feeling that
> this lack of check is more a feature than a bug... Not that I like it,
> but at least it is consistent.
That seemed to be the norm for irqchip drivers when I wrote this one,
and a fair number of them still panic on errors during init. There's
really not much else that can sanely be done since nothing will work
without irq handling.
As for the error return added by this patch, nothing checks it, so a
failure would merely result in the irqchip being silently skipped and
nothing working. Propagating the error back to of_irq_init() also has
no effect, not even a warning. Besides, kzalloc() is extremely unlikely
to fail at this stage, and if it does, you have much bigger problems.
--
Måns Rullgård
Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing
Miguel Ojeda writes:
> Hi Måns,
>
> On Thu, Dec 6, 2018 at 1:06 PM Måns Rullgård wrote:
>>
>> >> BTW, the parsing of this command has been broken since 3.2-rc1 due to
>> >> commit 129957069e6a.
>> >
>> > Thanks for checking! Are you able to test this?
>>
>> Yes, that's how I noticed it was broken.
>
> Fantastic, thank you.
>
> Can you please add the explanation about commit 129957069e6a
> ("staging: panel: Fixed checkpatch warning about simple_strtoul()") in
> the commit message so that we don't lose it? I will pick it into
> linux-next.
The bad code from that commit was already completely replaced with the
new parse_xy() function.
--
Måns Rullgård
Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing
Miguel Ojeda writes: > On Wed, Dec 5, 2018 at 6:58 PM Måns Rullgård wrote: >> >> Suppose the command "\e[Lx0y0;" is written to the device. The >> charlcd_write_char() function adds one character at a time to the escape >> sequence buffer. > > Ah, yes, that is much more clear. Indeed, parse_xy() expects the > entire command; the strchr() should still be there. > > By the way, if we are going to move to a direct check, I would also do > so in the generator command too if possible, to be consistent (in > another patch, possibly). I'd consider that separately. You're the maintainer, so it's up to you. >> BTW, the parsing of this command has been broken since 3.2-rc1 due to >> commit 129957069e6a. > > Thanks for checking! Are you able to test this? Yes, that's how I noticed it was broken. -- Måns Rullgård
Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing
Miguel Ojeda writes: > On Wed, Dec 5, 2018 at 6:58 PM Måns Rullgård wrote: >> >> Suppose the command "\e[Lx0y0;" is written to the device. The >> charlcd_write_char() function adds one character at a time to the escape >> sequence buffer. > > Ah, yes, that is much more clear. Indeed, parse_xy() expects the > entire command; the strchr() should still be there. > > By the way, if we are going to move to a direct check, I would also do > so in the generator command too if possible, to be consistent (in > another patch, possibly). I'd consider that separately. You're the maintainer, so it's up to you. >> BTW, the parsing of this command has been broken since 3.2-rc1 due to >> commit 129957069e6a. > > Thanks for checking! Are you able to test this? Yes, that's how I noticed it was broken. -- Måns Rullgård
Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing
Miguel Ojeda writes:
> Hi Mans,
>
> [CC'ing a few people involved previously on this]
>
> On Wed, Dec 5, 2018 at 2:53 PM Mans Rullgard wrote:
>>
>> Commit b34050fadb86 ("auxdisplay: charlcd: Fix and clean up handling of
>> x/y commands") fixed some problems by rewriting the parsing code,
>> but also broke things further by removing the check for a complete
>> command before attempting to parse it. As a result, parsing is
>> terminated at the first x or y character.
>
> Why is it necessary to check for ";" to be exactly at the end? Or, in
> other words, what requires it?
>
> If the syntax supported by parse_xy() is wrong for some reason, we
> should fix that (instead of adding a check before calling it).
Suppose the command "\e[Lx0y0;" is written to the device. The
charlcd_write_char() function adds one character at a time to the escape
sequence buffer. Once the 'L' has been seen, it calls
handle_lcd_special_code() after each additional character is added. On
encountering the 'x' this function will attempt to parse the command
using parse_xy(), which fails since it is incomplete. It is nonetheless
reported as processed, and the escape sequence is flushed. The
remainder of the command (i.e. "0y0;") is then displayed as text.
Since parse_xy() can never return success (true) unless there is a
semicolon, it is pointless to call it until we have seen one. With the
characters being added to the buffer one by one, it is enough to check
for semicolon at the end.
BTW, the parsing of this command has been broken since 3.2-rc1 due to
commit 129957069e6a.
--
Måns Rullgård
Re: [PATCH] auxdisplay: charlcd: fix x/y command parsing
Miguel Ojeda writes:
> Hi Mans,
>
> [CC'ing a few people involved previously on this]
>
> On Wed, Dec 5, 2018 at 2:53 PM Mans Rullgard wrote:
>>
>> Commit b34050fadb86 ("auxdisplay: charlcd: Fix and clean up handling of
>> x/y commands") fixed some problems by rewriting the parsing code,
>> but also broke things further by removing the check for a complete
>> command before attempting to parse it. As a result, parsing is
>> terminated at the first x or y character.
>
> Why is it necessary to check for ";" to be exactly at the end? Or, in
> other words, what requires it?
>
> If the syntax supported by parse_xy() is wrong for some reason, we
> should fix that (instead of adding a check before calling it).
Suppose the command "\e[Lx0y0;" is written to the device. The
charlcd_write_char() function adds one character at a time to the escape
sequence buffer. Once the 'L' has been seen, it calls
handle_lcd_special_code() after each additional character is added. On
encountering the 'x' this function will attempt to parse the command
using parse_xy(), which fails since it is incomplete. It is nonetheless
reported as processed, and the escape sequence is flushed. The
remainder of the command (i.e. "0y0;") is then displayed as text.
Since parse_xy() can never return success (true) unless there is a
semicolon, it is pointless to call it until we have seen one. With the
characters being added to the buffer one by one, it is enough to check
for semicolon at the end.
BTW, the parsing of this command has been broken since 3.2-rc1 due to
commit 129957069e6a.
--
Måns Rullgård
Re: [PATCH] mm: fix insert_pfn() return value
Matthew Wilcox writes:
> On Tue, Nov 27, 2018 at 02:43:51PM +, Mans Rullgard wrote:
>> Commit 9b5a8e00d479 ("mm: convert insert_pfn() to vm_fault_t") accidentally
>> made insert_pfn() always return an error. Fix this.
>
> Umm. VM_FAULT_NOPAGE is not an error. It's saying "I inserted the PFN,
> there's no struct page for the core VM to do anything with". Which is
> the correct response from a device driver which has called insert_pfn().
>
> Could you explain a bit more what led you to think there's a problem here?
Apparently some (not in mainline) driver code had been hastily converted
to the vm_fault_t codes, and that is where the error is. Sorry for the
noise. Please disregard this.
(The quickest way to get the correct answer is still to send a bad
patch.)
> Also, rather rude of you not to cc the patch author when you claim to
> be fixing a bug in their patch.
Sorry about that. Blame the get-maintainers script.
--
Måns Rullgård
Re: [PATCH] mm: fix insert_pfn() return value
Matthew Wilcox writes:
> On Tue, Nov 27, 2018 at 02:43:51PM +, Mans Rullgard wrote:
>> Commit 9b5a8e00d479 ("mm: convert insert_pfn() to vm_fault_t") accidentally
>> made insert_pfn() always return an error. Fix this.
>
> Umm. VM_FAULT_NOPAGE is not an error. It's saying "I inserted the PFN,
> there's no struct page for the core VM to do anything with". Which is
> the correct response from a device driver which has called insert_pfn().
>
> Could you explain a bit more what led you to think there's a problem here?
Apparently some (not in mainline) driver code had been hastily converted
to the vm_fault_t codes, and that is where the error is. Sorry for the
noise. Please disregard this.
(The quickest way to get the correct answer is still to send a bad
patch.)
> Also, rather rude of you not to cc the patch author when you claim to
> be fixing a bug in their patch.
Sorry about that. Blame the get-maintainers script.
--
Måns Rullgård
Re: [PATCH] dmaengine: enable mxs-dma for imx6sx
Vinod Koul <vinod.k...@intel.com> writes: > On Thu, Aug 17, 2017 at 11:41:43AM +0100, Mans Rullgard wrote: >> The mxs-dma unit is also used on i.MX6SX. Make it selectable. > > This doesnt apply for me, looking at below I do not think this was send on > any latest kernel.. Sorry, I missed a recent change that makes this unnecessary. >> Signed-off-by: Mans Rullgard <m...@mansr.com> >> --- >> drivers/dma/Kconfig | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig >> index 24e8597b2c3e..cb7ec780892d 100644 >> --- a/drivers/dma/Kconfig >> +++ b/drivers/dma/Kconfig >> @@ -354,7 +354,7 @@ config MV_XOR_V2 >> >> config MXS_DMA >> bool "MXS DMA support" >> -depends on SOC_IMX23 || SOC_IMX28 || SOC_IMX6Q || SOC_IMX6UL >> +depends on SOC_IMX23 || SOC_IMX28 || SOC_IMX6Q || SOC_IMX6SX || >> SOC_IMX6UL >> select STMP_DEVICE >> select DMA_ENGINE >> help >> -- >> 2.14.1 >> > > -- > ~Vinod -- Måns Rullgård
Re: [PATCH] dmaengine: enable mxs-dma for imx6sx
Vinod Koul writes: > On Thu, Aug 17, 2017 at 11:41:43AM +0100, Mans Rullgard wrote: >> The mxs-dma unit is also used on i.MX6SX. Make it selectable. > > This doesnt apply for me, looking at below I do not think this was send on > any latest kernel.. Sorry, I missed a recent change that makes this unnecessary. >> Signed-off-by: Mans Rullgard >> --- >> drivers/dma/Kconfig | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig >> index 24e8597b2c3e..cb7ec780892d 100644 >> --- a/drivers/dma/Kconfig >> +++ b/drivers/dma/Kconfig >> @@ -354,7 +354,7 @@ config MV_XOR_V2 >> >> config MXS_DMA >> bool "MXS DMA support" >> -depends on SOC_IMX23 || SOC_IMX28 || SOC_IMX6Q || SOC_IMX6UL >> +depends on SOC_IMX23 || SOC_IMX28 || SOC_IMX6Q || SOC_IMX6SX || >> SOC_IMX6UL >> select STMP_DEVICE >> select DMA_ENGINE >> help >> -- >> 2.14.1 >> > > -- > ~Vinod -- Måns Rullgård
Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback
Florian Fainelli <f.faine...@gmail.com> writes: > What do we do with this patch series to move forward? Can we get Doug's > changes queued up for 4.14? My opinion is that the correct combined function should be added and the tango driver updated to use it. Patches already exist, so what are we waiting for? -- Måns Rullgård
Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback
Florian Fainelli writes: > What do we do with this patch series to move forward? Can we get Doug's > changes queued up for 4.14? My opinion is that the correct combined function should be added and the tango driver updated to use it. Patches already exist, so what are we waiting for? -- Måns Rullgård
Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback
Florian Fainelli <f.faine...@gmail.com> writes: > On 07/25/2017 06:29 AM, Måns Rullgård wrote: >> Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes: >> >>> On 25/07/2017 15:16, Måns Rullgård wrote: >>> >>>> What happened to the patch adding the proper combined function? >>> >>> It appears you're not CCed on v2. >>> >>> https://patchwork.kernel.org/patch/9859799/ >>> >>> Doug wrote: >>>> Yes, you understand correctly. The irq_mask_ack method is entirely >>>> optional and I assume that is why this issue went undetected for so >>>> long; however, it is slightly more efficient to combine the functions >>>> (even if the ack is unnecessary) which is why I chose to do so for my >>>> changes to the irqchip-brcmstb-l2 driver where I first discovered this >>>> issue. How much value the improved efficiency has is certainly >>>> debatable, but interrupt handling is one area where people might care >>>> about such a small difference. As the irqchip-tango driver maintainer >>>> you are welcome to decide whether or not the irq_mask_ack method makes >>>> sense to you. >>> >>> My preference goes to leaving the irq_mask_ack callback undefined, >>> and let the irqchip framework use irq_mask and irq_ack instead. >> >> Why would you prefer the less efficient way? >> > > Same question here, that does not really make sense to me. > > The whole point of this patch series is to have a set of efficient and > bugfree (or nearly) helper functions that drivers can rely on, are you > saying that somehow using irq_mask_and_ack is exposing a bug in the > tango irqchip driver and using the separate functions does not expose > this bug? There is currently a bug in that the function used doesn't do what its name implies which can't be good. Using the separate mask and ack functions obviously works, but combining them saves a lock/unlock sequence. The correct combined function has already been written, so I see no reason not to use it. -- Måns Rullgård
Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback
Florian Fainelli writes: > On 07/25/2017 06:29 AM, Måns Rullgård wrote: >> Marc Gonzalez writes: >> >>> On 25/07/2017 15:16, Måns Rullgård wrote: >>> >>>> What happened to the patch adding the proper combined function? >>> >>> It appears you're not CCed on v2. >>> >>> https://patchwork.kernel.org/patch/9859799/ >>> >>> Doug wrote: >>>> Yes, you understand correctly. The irq_mask_ack method is entirely >>>> optional and I assume that is why this issue went undetected for so >>>> long; however, it is slightly more efficient to combine the functions >>>> (even if the ack is unnecessary) which is why I chose to do so for my >>>> changes to the irqchip-brcmstb-l2 driver where I first discovered this >>>> issue. How much value the improved efficiency has is certainly >>>> debatable, but interrupt handling is one area where people might care >>>> about such a small difference. As the irqchip-tango driver maintainer >>>> you are welcome to decide whether or not the irq_mask_ack method makes >>>> sense to you. >>> >>> My preference goes to leaving the irq_mask_ack callback undefined, >>> and let the irqchip framework use irq_mask and irq_ack instead. >> >> Why would you prefer the less efficient way? >> > > Same question here, that does not really make sense to me. > > The whole point of this patch series is to have a set of efficient and > bugfree (or nearly) helper functions that drivers can rely on, are you > saying that somehow using irq_mask_and_ack is exposing a bug in the > tango irqchip driver and using the separate functions does not expose > this bug? There is currently a bug in that the function used doesn't do what its name implies which can't be good. Using the separate mask and ack functions obviously works, but combining them saves a lock/unlock sequence. The correct combined function has already been written, so I see no reason not to use it. -- Måns Rullgård
Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes: > On 25/07/2017 15:16, Måns Rullgård wrote: > >> What happened to the patch adding the proper combined function? > > It appears you're not CCed on v2. > > https://patchwork.kernel.org/patch/9859799/ > > Doug wrote: >> Yes, you understand correctly. The irq_mask_ack method is entirely >> optional and I assume that is why this issue went undetected for so >> long; however, it is slightly more efficient to combine the functions >> (even if the ack is unnecessary) which is why I chose to do so for my >> changes to the irqchip-brcmstb-l2 driver where I first discovered this >> issue. How much value the improved efficiency has is certainly >> debatable, but interrupt handling is one area where people might care >> about such a small difference. As the irqchip-tango driver maintainer >> you are welcome to decide whether or not the irq_mask_ack method makes >> sense to you. > > My preference goes to leaving the irq_mask_ack callback undefined, > and let the irqchip framework use irq_mask and irq_ack instead. Why would you prefer the less efficient way? -- Måns Rullgård
Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback
Marc Gonzalez writes: > On 25/07/2017 15:16, Måns Rullgård wrote: > >> What happened to the patch adding the proper combined function? > > It appears you're not CCed on v2. > > https://patchwork.kernel.org/patch/9859799/ > > Doug wrote: >> Yes, you understand correctly. The irq_mask_ack method is entirely >> optional and I assume that is why this issue went undetected for so >> long; however, it is slightly more efficient to combine the functions >> (even if the ack is unnecessary) which is why I chose to do so for my >> changes to the irqchip-brcmstb-l2 driver where I first discovered this >> issue. How much value the improved efficiency has is certainly >> debatable, but interrupt handling is one area where people might care >> about such a small difference. As the irqchip-tango driver maintainer >> you are welcome to decide whether or not the irq_mask_ack method makes >> sense to you. > > My preference goes to leaving the irq_mask_ack callback undefined, > and let the irqchip framework use irq_mask and irq_ack instead. Why would you prefer the less efficient way? -- Måns Rullgård
Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:
> irq_gc_mask_disable_reg_and_ack() is not equivalent to
> irq_gc_mask_disable_reg() and irq_gc_ack_set_bit().
>
> Leave the irq_mask_ack callback undefined, and let the irqchip
> framework use irq_mask and irq_ack instead.
>
> Reported-by: Doug Berger <open...@gmail.com>
> Fixes: 4bba66899ac6 ("irqchip/tango: Add support for Sigma Designs
> SMP86xx/SMP87xx interrupt controller")
> Signed-off-by: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
> Cc: sta...@vger.kernel.org
> ---
> As discussed previously, it is acceptable for tango to rely
> on mask_ack_irq() doing the right thing(TM).
> ---
> drivers/irqchip/irq-tango.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
> index bdbb5c0ff7fe..825085cdab99 100644
> --- a/drivers/irqchip/irq-tango.c
> +++ b/drivers/irqchip/irq-tango.c
> @@ -141,7 +141,6 @@ static void __init tangox_irq_init_chip(struct
> irq_chip_generic *gc,
> for (i = 0; i < 2; i++) {
> ct[i].chip.irq_ack = irq_gc_ack_set_bit;
> ct[i].chip.irq_mask = irq_gc_mask_disable_reg;
> - ct[i].chip.irq_mask_ack = irq_gc_mask_disable_reg_and_ack;
> ct[i].chip.irq_unmask = irq_gc_unmask_enable_reg;
> ct[i].chip.irq_set_type = tangox_irq_set_type;
> ct[i].chip.name = gc->domain->name;
> --
What happened to the patch adding the proper combined function?
--
Måns Rullgård
Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback
Marc Gonzalez writes:
> irq_gc_mask_disable_reg_and_ack() is not equivalent to
> irq_gc_mask_disable_reg() and irq_gc_ack_set_bit().
>
> Leave the irq_mask_ack callback undefined, and let the irqchip
> framework use irq_mask and irq_ack instead.
>
> Reported-by: Doug Berger
> Fixes: 4bba66899ac6 ("irqchip/tango: Add support for Sigma Designs
> SMP86xx/SMP87xx interrupt controller")
> Signed-off-by: Marc Gonzalez
> Cc: sta...@vger.kernel.org
> ---
> As discussed previously, it is acceptable for tango to rely
> on mask_ack_irq() doing the right thing(TM).
> ---
> drivers/irqchip/irq-tango.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
> index bdbb5c0ff7fe..825085cdab99 100644
> --- a/drivers/irqchip/irq-tango.c
> +++ b/drivers/irqchip/irq-tango.c
> @@ -141,7 +141,6 @@ static void __init tangox_irq_init_chip(struct
> irq_chip_generic *gc,
> for (i = 0; i < 2; i++) {
> ct[i].chip.irq_ack = irq_gc_ack_set_bit;
> ct[i].chip.irq_mask = irq_gc_mask_disable_reg;
> - ct[i].chip.irq_mask_ack = irq_gc_mask_disable_reg_and_ack;
> ct[i].chip.irq_unmask = irq_gc_unmask_enable_reg;
> ct[i].chip.irq_set_type = tangox_irq_set_type;
> ct[i].chip.name = gc->domain->name;
> --
What happened to the patch adding the proper combined function?
--
Måns Rullgård
Re: [PATCH 2/6] irqchip/tango: Use irq_gc_mask_disable_and_ack_set
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:
> Hello,
>
> I've added Mans (the code's author) to the recipient list.
>
> Regards.
>
> On 07/07/2017 21:20, Doug Berger wrote:
>
>> From: Florian Fainelli <f.faine...@gmail.com>
>>
>> Any usage of the irq_gc_mask_disable_reg_and_ack() function should be
>> replaced with either the irq_gc_mask_disable_and_ack_set() or the
>> irq_gc_mask_set_and_ack_set() function depending on which corrects
>> the bugs in irq_gc_mask_disable_reg_and_ack() for a given usage.
>>
>> For the Tango irqchip driver, irq_gc_mask_disable_and_ack_set() seems to
>> be what is intended, so use it.
>>
>> Signed-off-by: Florian Fainelli <f.faine...@gmail.com>
>> Signed-off-by: Doug Berger <open...@gmail.com>
As I said elsewhere, this looks more correct although I haven't tested it.
Acked-by: Mans Rullgard <m...@mansr.com>
>> ---
>> drivers/irqchip/irq-tango.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
>> index bdbb5c0ff7fe..0c085303a583 100644
>> --- a/drivers/irqchip/irq-tango.c
>> +++ b/drivers/irqchip/irq-tango.c
>> @@ -141,7 +141,7 @@ static void __init tangox_irq_init_chip(struct
>> irq_chip_generic *gc,
>> for (i = 0; i < 2; i++) {
>> ct[i].chip.irq_ack = irq_gc_ack_set_bit;
>> ct[i].chip.irq_mask = irq_gc_mask_disable_reg;
>> -ct[i].chip.irq_mask_ack = irq_gc_mask_disable_reg_and_ack;
>> +ct[i].chip.irq_mask_ack = irq_gc_mask_disable_and_ack_set;
>> ct[i].chip.irq_unmask = irq_gc_unmask_enable_reg;
>> ct[i].chip.irq_set_type = tangox_irq_set_type;
>> ct[i].chip.name = gc->domain->name;
>>
>
--
Måns Rullgård
Re: [PATCH 2/6] irqchip/tango: Use irq_gc_mask_disable_and_ack_set
Marc Gonzalez writes:
> Hello,
>
> I've added Mans (the code's author) to the recipient list.
>
> Regards.
>
> On 07/07/2017 21:20, Doug Berger wrote:
>
>> From: Florian Fainelli
>>
>> Any usage of the irq_gc_mask_disable_reg_and_ack() function should be
>> replaced with either the irq_gc_mask_disable_and_ack_set() or the
>> irq_gc_mask_set_and_ack_set() function depending on which corrects
>> the bugs in irq_gc_mask_disable_reg_and_ack() for a given usage.
>>
>> For the Tango irqchip driver, irq_gc_mask_disable_and_ack_set() seems to
>> be what is intended, so use it.
>>
>> Signed-off-by: Florian Fainelli
>> Signed-off-by: Doug Berger
As I said elsewhere, this looks more correct although I haven't tested it.
Acked-by: Mans Rullgard
>> ---
>> drivers/irqchip/irq-tango.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
>> index bdbb5c0ff7fe..0c085303a583 100644
>> --- a/drivers/irqchip/irq-tango.c
>> +++ b/drivers/irqchip/irq-tango.c
>> @@ -141,7 +141,7 @@ static void __init tangox_irq_init_chip(struct
>> irq_chip_generic *gc,
>> for (i = 0; i < 2; i++) {
>> ct[i].chip.irq_ack = irq_gc_ack_set_bit;
>> ct[i].chip.irq_mask = irq_gc_mask_disable_reg;
>> -ct[i].chip.irq_mask_ack = irq_gc_mask_disable_reg_and_ack;
>> +ct[i].chip.irq_mask_ack = irq_gc_mask_disable_and_ack_set;
>> ct[i].chip.irq_unmask = irq_gc_unmask_enable_reg;
>> ct[i].chip.irq_set_type = tangox_irq_set_type;
>> ct[i].chip.name = gc->domain->name;
>>
>
--
Måns Rullgård
Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions
Doug Berger <open...@gmail.com> writes:
> Mans, as the author of the only existing upstream user of this code,
> should have received this as well.
>
> -Doug
>
> On 07/07/2017 12:20 PM, Doug Berger wrote:
>> The irq_gc_mask_disable_reg_and_ack() function name implies that it
>> provides the combined functions of irq_gc_mask_disable_reg() and
>> irq_gc_ack(). However, the implementation does not actually do
>> that since it writes the mask instead of the disable register. It
>> also does not maintain the mask cache which makes it inappropriate
>> to use with other masking functions.
>>
>> In addition, commit 659fb32d1b67 ("genirq: replace irq_gc_ack() with
>> {set,clr}_bit variants (fwd)") effectively renamed irq_gc_ack() to
>> irq_gc_set_bit() so this function probably should have also been
>> renamed at that time.
>>
>> Since this generic chip code provides three mask functions and two
>> ack functions, this commit provides generic implementations for all
>> six combinations of the mask and ack functions suitable for use
>> with the irq_mask_ack member of the struct irq_chip.
>>
>> The '_reg' and '_bit' portions of the base function names were left
>> out of the new combined function names in an attempt to keep the
>> function name lengths manageable with the 80 character source code
>> line length while still capturing the distinct aspects of each
>> combination of functions.
>>
>> Signed-off-by: Doug Berger <open...@gmail.com>
Hmm, something is wrong here. The irq_gc_mask_disable_reg_and_ack()
function writes to regs.mask, but the irq-tango driver doesn't set this
field (there is no corresponding hardware register). Either it is never
called, or the write ends up being harmless. I don't remember why I set
irq_mask_ack that way.
>> /**
>> + * irq_gc_mask_disable_and_ack_set - Mask and ack pending interrupt
>> + * @d: irq_data
>> + *
>> + * Chip has separate enable/disable registers instead of a single mask
>> + * register and pending interrupt is acknowledged by setting a bit.
>> + */
>> +void irq_gc_mask_disable_and_ack_set(struct irq_data *d)
>> +{
>> +struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> +struct irq_chip_type *ct = irq_data_get_chip_type(d);
>> +u32 mask = d->mask;
>> +
>> +irq_gc_lock(gc);
>> +irq_reg_writel(gc, mask, ct->regs.disable);
>> +*ct->mask_cache &= ~mask;
>> +irq_reg_writel(gc, mask, ct->regs.ack);
>> +irq_gc_unlock(gc);
>> +}
This function looks like it should probably be used instead. I'll try
to remember to test it when I have time to fire up that hardware.
--
Måns Rullgård
Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions
Doug Berger writes:
> Mans, as the author of the only existing upstream user of this code,
> should have received this as well.
>
> -Doug
>
> On 07/07/2017 12:20 PM, Doug Berger wrote:
>> The irq_gc_mask_disable_reg_and_ack() function name implies that it
>> provides the combined functions of irq_gc_mask_disable_reg() and
>> irq_gc_ack(). However, the implementation does not actually do
>> that since it writes the mask instead of the disable register. It
>> also does not maintain the mask cache which makes it inappropriate
>> to use with other masking functions.
>>
>> In addition, commit 659fb32d1b67 ("genirq: replace irq_gc_ack() with
>> {set,clr}_bit variants (fwd)") effectively renamed irq_gc_ack() to
>> irq_gc_set_bit() so this function probably should have also been
>> renamed at that time.
>>
>> Since this generic chip code provides three mask functions and two
>> ack functions, this commit provides generic implementations for all
>> six combinations of the mask and ack functions suitable for use
>> with the irq_mask_ack member of the struct irq_chip.
>>
>> The '_reg' and '_bit' portions of the base function names were left
>> out of the new combined function names in an attempt to keep the
>> function name lengths manageable with the 80 character source code
>> line length while still capturing the distinct aspects of each
>> combination of functions.
>>
>> Signed-off-by: Doug Berger
Hmm, something is wrong here. The irq_gc_mask_disable_reg_and_ack()
function writes to regs.mask, but the irq-tango driver doesn't set this
field (there is no corresponding hardware register). Either it is never
called, or the write ends up being harmless. I don't remember why I set
irq_mask_ack that way.
>> /**
>> + * irq_gc_mask_disable_and_ack_set - Mask and ack pending interrupt
>> + * @d: irq_data
>> + *
>> + * Chip has separate enable/disable registers instead of a single mask
>> + * register and pending interrupt is acknowledged by setting a bit.
>> + */
>> +void irq_gc_mask_disable_and_ack_set(struct irq_data *d)
>> +{
>> +struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> +struct irq_chip_type *ct = irq_data_get_chip_type(d);
>> +u32 mask = d->mask;
>> +
>> +irq_gc_lock(gc);
>> +irq_reg_writel(gc, mask, ct->regs.disable);
>> +*ct->mask_cache &= ~mask;
>> +irq_reg_writel(gc, mask, ct->regs.ack);
>> +irq_gc_unlock(gc);
>> +}
This function looks like it should probably be used instead. I'll try
to remember to test it when I have time to fire up that hardware.
--
Måns Rullgård
Re: Arrays of variable length
Tomas Winkler <tom...@gmail.com> writes: > On Thu, Mar 9, 2017 at 4:26 PM, Måns Rullgård <m...@mansr.com> wrote: >> Tomas Winkler <tom...@gmail.com> writes: >> >>> On Thu, Mar 9, 2017 at 4:16 PM, Måns Rullgård <m...@mansr.com> wrote: >>>> Tomas Winkler <tom...@gmail.com> writes: >>>> >>>>> On Thu, Mar 9, 2017 at 3:02 PM, Måns Rullgård <m...@mansr.com> wrote: >>>>>> Tomas Winkler <tom...@gmail.com> writes: >>>>>> >>>>>>> On Mon, Mar 6, 2017 at 2:31 AM, Måns Rullgård <m...@mansr.com> wrote: >>>>>>>> Henrique de Moraes Holschuh <h...@hmh.eng.br> writes: >>>>>>>> >>>>>>>>> On Sun, 05 Mar 2017, Måns Rullgård wrote: >>>>>>>>>> Tomas Winkler <tom...@gmail.com> writes: >>>>>>>>>> > Sparse complains for arrays declared with variable length >>>>>>>>>> > >>>>>>>>>> > 'warning: Variable length array is used' >>>>>>>>>> > >>>>>>>>>> > Prior to c99 this was not allowed but lgcc (c99) doesn't have >>>>>>>>>> > problem >>>>>>>>>> > with that https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html. >>>>>>>>>> > And also Linux kernel compilation with W=1 doesn't complain. >>>>>>>>>> > >>>>>>>>>> > Since sparse is used extensively would like to ask what is the >>>>>>>>>> > correct >>>>>>>>>> > usage of arrays of variable length >>>>>>>>>> > within Linux Kernel. >>>>>>>>>> >>>>>>>>>> Variable-length arrays are a very bad idea. Don't use them, ever. >>>>>>>>>> If the size has a sane upper bound, just use that value statically. >>>>>>>>>> Otherwise, you have a stack overflow waiting to happen and should be >>>>>>>>>> using some kind of dynamic allocation instead. >>>>>>>>>> >>>>>>>>>> Furthermore, use of VLAs generally results in less efficient code. >>>>>>>>>> For >>>>>>>>>> instance, it forces gcc to waste a register for the frame pointer, >>>>>>>>>> and >>>>>>>>>> it often prevents inlining. >>>>>>>>> >>>>>>>>> Well, if we're going to forbid VLAs in the kernel, IMHO the kernel >>>>>>>>> build >>>>>>>>> system should call gcc with -Werror=vla to get that point across >>>>>>>>> early, >>>>>>>>> and flush out any offenders. >>>>>>>> >>>>>>>> If it were up to me, that's exactly what I'd do. >>>>>>> >>>>>>>> >>>>>>> Some parts of the kernel depends on VLA such as ___ON_STACK macros in >>>>>>> include/crypto/hash.h >>>>>>> It's actually pretty neat implementation, maybe it's too harsh to >>>>>>> disable VLA completely. >>>>>> >>>>>> And what happens if the requested size is insane? >>>>> >>>>> One option is to add '-Wvla-larger-than=n' >>>> >>>> If you know the upper bound, why use VLAs in the first place? >>> >>> This is a water mark and not actual usage, but maybe I didn't >>> understand your comment. >> >> If there is an upper bound known at compile time, why not simply use >> that size statically? If there is no upper bound, well, then you have a >> problem. > > If the compiler can do the job, why not to use this flexibility ? Because, as I already said, there are security implications if the size is unbounded, and even with safely bounded size, using VLAs interferes with compiler optimisations. Ensuring VLAs are used safely is usually more work than simply avoiding them in the first place. -- Måns Rullgård
Re: Arrays of variable length
Tomas Winkler writes: > On Thu, Mar 9, 2017 at 4:26 PM, Måns Rullgård wrote: >> Tomas Winkler writes: >> >>> On Thu, Mar 9, 2017 at 4:16 PM, Måns Rullgård wrote: >>>> Tomas Winkler writes: >>>> >>>>> On Thu, Mar 9, 2017 at 3:02 PM, Måns Rullgård wrote: >>>>>> Tomas Winkler writes: >>>>>> >>>>>>> On Mon, Mar 6, 2017 at 2:31 AM, Måns Rullgård wrote: >>>>>>>> Henrique de Moraes Holschuh writes: >>>>>>>> >>>>>>>>> On Sun, 05 Mar 2017, Måns Rullgård wrote: >>>>>>>>>> Tomas Winkler writes: >>>>>>>>>> > Sparse complains for arrays declared with variable length >>>>>>>>>> > >>>>>>>>>> > 'warning: Variable length array is used' >>>>>>>>>> > >>>>>>>>>> > Prior to c99 this was not allowed but lgcc (c99) doesn't have >>>>>>>>>> > problem >>>>>>>>>> > with that https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html. >>>>>>>>>> > And also Linux kernel compilation with W=1 doesn't complain. >>>>>>>>>> > >>>>>>>>>> > Since sparse is used extensively would like to ask what is the >>>>>>>>>> > correct >>>>>>>>>> > usage of arrays of variable length >>>>>>>>>> > within Linux Kernel. >>>>>>>>>> >>>>>>>>>> Variable-length arrays are a very bad idea. Don't use them, ever. >>>>>>>>>> If the size has a sane upper bound, just use that value statically. >>>>>>>>>> Otherwise, you have a stack overflow waiting to happen and should be >>>>>>>>>> using some kind of dynamic allocation instead. >>>>>>>>>> >>>>>>>>>> Furthermore, use of VLAs generally results in less efficient code. >>>>>>>>>> For >>>>>>>>>> instance, it forces gcc to waste a register for the frame pointer, >>>>>>>>>> and >>>>>>>>>> it often prevents inlining. >>>>>>>>> >>>>>>>>> Well, if we're going to forbid VLAs in the kernel, IMHO the kernel >>>>>>>>> build >>>>>>>>> system should call gcc with -Werror=vla to get that point across >>>>>>>>> early, >>>>>>>>> and flush out any offenders. >>>>>>>> >>>>>>>> If it were up to me, that's exactly what I'd do. >>>>>>> >>>>>>>> >>>>>>> Some parts of the kernel depends on VLA such as ___ON_STACK macros in >>>>>>> include/crypto/hash.h >>>>>>> It's actually pretty neat implementation, maybe it's too harsh to >>>>>>> disable VLA completely. >>>>>> >>>>>> And what happens if the requested size is insane? >>>>> >>>>> One option is to add '-Wvla-larger-than=n' >>>> >>>> If you know the upper bound, why use VLAs in the first place? >>> >>> This is a water mark and not actual usage, but maybe I didn't >>> understand your comment. >> >> If there is an upper bound known at compile time, why not simply use >> that size statically? If there is no upper bound, well, then you have a >> problem. > > If the compiler can do the job, why not to use this flexibility ? Because, as I already said, there are security implications if the size is unbounded, and even with safely bounded size, using VLAs interferes with compiler optimisations. Ensuring VLAs are used safely is usually more work than simply avoiding them in the first place. -- Måns Rullgård
Re: Arrays of variable length
Tomas Winkler <tom...@gmail.com> writes: > On Thu, Mar 9, 2017 at 4:16 PM, Måns Rullgård <m...@mansr.com> wrote: >> Tomas Winkler <tom...@gmail.com> writes: >> >>> On Thu, Mar 9, 2017 at 3:02 PM, Måns Rullgård <m...@mansr.com> wrote: >>>> Tomas Winkler <tom...@gmail.com> writes: >>>> >>>>> On Mon, Mar 6, 2017 at 2:31 AM, Måns Rullgård <m...@mansr.com> wrote: >>>>>> Henrique de Moraes Holschuh <h...@hmh.eng.br> writes: >>>>>> >>>>>>> On Sun, 05 Mar 2017, Måns Rullgård wrote: >>>>>>>> Tomas Winkler <tom...@gmail.com> writes: >>>>>>>> > Sparse complains for arrays declared with variable length >>>>>>>> > >>>>>>>> > 'warning: Variable length array is used' >>>>>>>> > >>>>>>>> > Prior to c99 this was not allowed but lgcc (c99) doesn't have problem >>>>>>>> > with that https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html. >>>>>>>> > And also Linux kernel compilation with W=1 doesn't complain. >>>>>>>> > >>>>>>>> > Since sparse is used extensively would like to ask what is the >>>>>>>> > correct >>>>>>>> > usage of arrays of variable length >>>>>>>> > within Linux Kernel. >>>>>>>> >>>>>>>> Variable-length arrays are a very bad idea. Don't use them, ever. >>>>>>>> If the size has a sane upper bound, just use that value statically. >>>>>>>> Otherwise, you have a stack overflow waiting to happen and should be >>>>>>>> using some kind of dynamic allocation instead. >>>>>>>> >>>>>>>> Furthermore, use of VLAs generally results in less efficient code. For >>>>>>>> instance, it forces gcc to waste a register for the frame pointer, and >>>>>>>> it often prevents inlining. >>>>>>> >>>>>>> Well, if we're going to forbid VLAs in the kernel, IMHO the kernel build >>>>>>> system should call gcc with -Werror=vla to get that point across early, >>>>>>> and flush out any offenders. >>>>>> >>>>>> If it were up to me, that's exactly what I'd do. >>>>> >>>>>> >>>>> Some parts of the kernel depends on VLA such as ___ON_STACK macros in >>>>> include/crypto/hash.h >>>>> It's actually pretty neat implementation, maybe it's too harsh to >>>>> disable VLA completely. >>>> >>>> And what happens if the requested size is insane? >>> >>> One option is to add '-Wvla-larger-than=n' >> >> If you know the upper bound, why use VLAs in the first place? > > This is a water mark and not actual usage, but maybe I didn't > understand your comment. If there is an upper bound known at compile time, why not simply use that size statically? If there is no upper bound, well, then you have a problem. -- Måns Rullgård
Re: Arrays of variable length
Tomas Winkler writes: > On Thu, Mar 9, 2017 at 4:16 PM, Måns Rullgård wrote: >> Tomas Winkler writes: >> >>> On Thu, Mar 9, 2017 at 3:02 PM, Måns Rullgård wrote: >>>> Tomas Winkler writes: >>>> >>>>> On Mon, Mar 6, 2017 at 2:31 AM, Måns Rullgård wrote: >>>>>> Henrique de Moraes Holschuh writes: >>>>>> >>>>>>> On Sun, 05 Mar 2017, Måns Rullgård wrote: >>>>>>>> Tomas Winkler writes: >>>>>>>> > Sparse complains for arrays declared with variable length >>>>>>>> > >>>>>>>> > 'warning: Variable length array is used' >>>>>>>> > >>>>>>>> > Prior to c99 this was not allowed but lgcc (c99) doesn't have problem >>>>>>>> > with that https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html. >>>>>>>> > And also Linux kernel compilation with W=1 doesn't complain. >>>>>>>> > >>>>>>>> > Since sparse is used extensively would like to ask what is the >>>>>>>> > correct >>>>>>>> > usage of arrays of variable length >>>>>>>> > within Linux Kernel. >>>>>>>> >>>>>>>> Variable-length arrays are a very bad idea. Don't use them, ever. >>>>>>>> If the size has a sane upper bound, just use that value statically. >>>>>>>> Otherwise, you have a stack overflow waiting to happen and should be >>>>>>>> using some kind of dynamic allocation instead. >>>>>>>> >>>>>>>> Furthermore, use of VLAs generally results in less efficient code. For >>>>>>>> instance, it forces gcc to waste a register for the frame pointer, and >>>>>>>> it often prevents inlining. >>>>>>> >>>>>>> Well, if we're going to forbid VLAs in the kernel, IMHO the kernel build >>>>>>> system should call gcc with -Werror=vla to get that point across early, >>>>>>> and flush out any offenders. >>>>>> >>>>>> If it were up to me, that's exactly what I'd do. >>>>> >>>>>> >>>>> Some parts of the kernel depends on VLA such as ___ON_STACK macros in >>>>> include/crypto/hash.h >>>>> It's actually pretty neat implementation, maybe it's too harsh to >>>>> disable VLA completely. >>>> >>>> And what happens if the requested size is insane? >>> >>> One option is to add '-Wvla-larger-than=n' >> >> If you know the upper bound, why use VLAs in the first place? > > This is a water mark and not actual usage, but maybe I didn't > understand your comment. If there is an upper bound known at compile time, why not simply use that size statically? If there is no upper bound, well, then you have a problem. -- Måns Rullgård
Re: Arrays of variable length
Tomas Winkler <tom...@gmail.com> writes: > On Thu, Mar 9, 2017 at 3:02 PM, Måns Rullgård <m...@mansr.com> wrote: >> Tomas Winkler <tom...@gmail.com> writes: >> >>> On Mon, Mar 6, 2017 at 2:31 AM, Måns Rullgård <m...@mansr.com> wrote: >>>> Henrique de Moraes Holschuh <h...@hmh.eng.br> writes: >>>> >>>>> On Sun, 05 Mar 2017, Måns Rullgård wrote: >>>>>> Tomas Winkler <tom...@gmail.com> writes: >>>>>> > Sparse complains for arrays declared with variable length >>>>>> > >>>>>> > 'warning: Variable length array is used' >>>>>> > >>>>>> > Prior to c99 this was not allowed but lgcc (c99) doesn't have problem >>>>>> > with that https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html. >>>>>> > And also Linux kernel compilation with W=1 doesn't complain. >>>>>> > >>>>>> > Since sparse is used extensively would like to ask what is the correct >>>>>> > usage of arrays of variable length >>>>>> > within Linux Kernel. >>>>>> >>>>>> Variable-length arrays are a very bad idea. Don't use them, ever. >>>>>> If the size has a sane upper bound, just use that value statically. >>>>>> Otherwise, you have a stack overflow waiting to happen and should be >>>>>> using some kind of dynamic allocation instead. >>>>>> >>>>>> Furthermore, use of VLAs generally results in less efficient code. For >>>>>> instance, it forces gcc to waste a register for the frame pointer, and >>>>>> it often prevents inlining. >>>>> >>>>> Well, if we're going to forbid VLAs in the kernel, IMHO the kernel build >>>>> system should call gcc with -Werror=vla to get that point across early, >>>>> and flush out any offenders. >>>> >>>> If it were up to me, that's exactly what I'd do. >>> >>>> >>> Some parts of the kernel depends on VLA such as ___ON_STACK macros in >>> include/crypto/hash.h >>> It's actually pretty neat implementation, maybe it's too harsh to >>> disable VLA completely. >> >> And what happens if the requested size is insane? > > One option is to add '-Wvla-larger-than=n' If you know the upper bound, why use VLAs in the first place? -- Måns Rullgård
Re: Arrays of variable length
Tomas Winkler writes: > On Thu, Mar 9, 2017 at 3:02 PM, Måns Rullgård wrote: >> Tomas Winkler writes: >> >>> On Mon, Mar 6, 2017 at 2:31 AM, Måns Rullgård wrote: >>>> Henrique de Moraes Holschuh writes: >>>> >>>>> On Sun, 05 Mar 2017, Måns Rullgård wrote: >>>>>> Tomas Winkler writes: >>>>>> > Sparse complains for arrays declared with variable length >>>>>> > >>>>>> > 'warning: Variable length array is used' >>>>>> > >>>>>> > Prior to c99 this was not allowed but lgcc (c99) doesn't have problem >>>>>> > with that https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html. >>>>>> > And also Linux kernel compilation with W=1 doesn't complain. >>>>>> > >>>>>> > Since sparse is used extensively would like to ask what is the correct >>>>>> > usage of arrays of variable length >>>>>> > within Linux Kernel. >>>>>> >>>>>> Variable-length arrays are a very bad idea. Don't use them, ever. >>>>>> If the size has a sane upper bound, just use that value statically. >>>>>> Otherwise, you have a stack overflow waiting to happen and should be >>>>>> using some kind of dynamic allocation instead. >>>>>> >>>>>> Furthermore, use of VLAs generally results in less efficient code. For >>>>>> instance, it forces gcc to waste a register for the frame pointer, and >>>>>> it often prevents inlining. >>>>> >>>>> Well, if we're going to forbid VLAs in the kernel, IMHO the kernel build >>>>> system should call gcc with -Werror=vla to get that point across early, >>>>> and flush out any offenders. >>>> >>>> If it were up to me, that's exactly what I'd do. >>> >>>> >>> Some parts of the kernel depends on VLA such as ___ON_STACK macros in >>> include/crypto/hash.h >>> It's actually pretty neat implementation, maybe it's too harsh to >>> disable VLA completely. >> >> And what happens if the requested size is insane? > > One option is to add '-Wvla-larger-than=n' If you know the upper bound, why use VLAs in the first place? -- Måns Rullgård
Re: Arrays of variable length
Tomas Winkler <tom...@gmail.com> writes: > On Mon, Mar 6, 2017 at 2:31 AM, Måns Rullgård <m...@mansr.com> wrote: >> Henrique de Moraes Holschuh <h...@hmh.eng.br> writes: >> >>> On Sun, 05 Mar 2017, Måns Rullgård wrote: >>>> Tomas Winkler <tom...@gmail.com> writes: >>>> > Sparse complains for arrays declared with variable length >>>> > >>>> > 'warning: Variable length array is used' >>>> > >>>> > Prior to c99 this was not allowed but lgcc (c99) doesn't have problem >>>> > with that https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html. >>>> > And also Linux kernel compilation with W=1 doesn't complain. >>>> > >>>> > Since sparse is used extensively would like to ask what is the correct >>>> > usage of arrays of variable length >>>> > within Linux Kernel. >>>> >>>> Variable-length arrays are a very bad idea. Don't use them, ever. >>>> If the size has a sane upper bound, just use that value statically. >>>> Otherwise, you have a stack overflow waiting to happen and should be >>>> using some kind of dynamic allocation instead. >>>> >>>> Furthermore, use of VLAs generally results in less efficient code. For >>>> instance, it forces gcc to waste a register for the frame pointer, and >>>> it often prevents inlining. >>> >>> Well, if we're going to forbid VLAs in the kernel, IMHO the kernel build >>> system should call gcc with -Werror=vla to get that point across early, >>> and flush out any offenders. >> >> If it were up to me, that's exactly what I'd do. > >> > Some parts of the kernel depends on VLA such as ___ON_STACK macros in > include/crypto/hash.h > It's actually pretty neat implementation, maybe it's too harsh to > disable VLA completely. And what happens if the requested size is insane? -- Måns Rullgård
Re: Arrays of variable length
Tomas Winkler writes: > On Mon, Mar 6, 2017 at 2:31 AM, Måns Rullgård wrote: >> Henrique de Moraes Holschuh writes: >> >>> On Sun, 05 Mar 2017, Måns Rullgård wrote: >>>> Tomas Winkler writes: >>>> > Sparse complains for arrays declared with variable length >>>> > >>>> > 'warning: Variable length array is used' >>>> > >>>> > Prior to c99 this was not allowed but lgcc (c99) doesn't have problem >>>> > with that https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html. >>>> > And also Linux kernel compilation with W=1 doesn't complain. >>>> > >>>> > Since sparse is used extensively would like to ask what is the correct >>>> > usage of arrays of variable length >>>> > within Linux Kernel. >>>> >>>> Variable-length arrays are a very bad idea. Don't use them, ever. >>>> If the size has a sane upper bound, just use that value statically. >>>> Otherwise, you have a stack overflow waiting to happen and should be >>>> using some kind of dynamic allocation instead. >>>> >>>> Furthermore, use of VLAs generally results in less efficient code. For >>>> instance, it forces gcc to waste a register for the frame pointer, and >>>> it often prevents inlining. >>> >>> Well, if we're going to forbid VLAs in the kernel, IMHO the kernel build >>> system should call gcc with -Werror=vla to get that point across early, >>> and flush out any offenders. >> >> If it were up to me, that's exactly what I'd do. > >> > Some parts of the kernel depends on VLA such as ___ON_STACK macros in > include/crypto/hash.h > It's actually pretty neat implementation, maybe it's too harsh to > disable VLA completely. And what happens if the requested size is insane? -- Måns Rullgård
Re: Arrays of variable length
Henrique de Moraes Holschuh <h...@hmh.eng.br> writes: > On Sun, 05 Mar 2017, Måns Rullgård wrote: >> Tomas Winkler <tom...@gmail.com> writes: >> > Sparse complains for arrays declared with variable length >> > >> > 'warning: Variable length array is used' >> > >> > Prior to c99 this was not allowed but lgcc (c99) doesn't have problem >> > with that https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html. >> > And also Linux kernel compilation with W=1 doesn't complain. >> > >> > Since sparse is used extensively would like to ask what is the correct >> > usage of arrays of variable length >> > within Linux Kernel. >> >> Variable-length arrays are a very bad idea. Don't use them, ever. >> If the size has a sane upper bound, just use that value statically. >> Otherwise, you have a stack overflow waiting to happen and should be >> using some kind of dynamic allocation instead. >> >> Furthermore, use of VLAs generally results in less efficient code. For >> instance, it forces gcc to waste a register for the frame pointer, and >> it often prevents inlining. > > Well, if we're going to forbid VLAs in the kernel, IMHO the kernel build > system should call gcc with -Werror=vla to get that point across early, > and flush out any offenders. If it were up to me, that's exactly what I'd do. -- Måns Rullgård
Re: Arrays of variable length
Henrique de Moraes Holschuh writes: > On Sun, 05 Mar 2017, Måns Rullgård wrote: >> Tomas Winkler writes: >> > Sparse complains for arrays declared with variable length >> > >> > 'warning: Variable length array is used' >> > >> > Prior to c99 this was not allowed but lgcc (c99) doesn't have problem >> > with that https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html. >> > And also Linux kernel compilation with W=1 doesn't complain. >> > >> > Since sparse is used extensively would like to ask what is the correct >> > usage of arrays of variable length >> > within Linux Kernel. >> >> Variable-length arrays are a very bad idea. Don't use them, ever. >> If the size has a sane upper bound, just use that value statically. >> Otherwise, you have a stack overflow waiting to happen and should be >> using some kind of dynamic allocation instead. >> >> Furthermore, use of VLAs generally results in less efficient code. For >> instance, it forces gcc to waste a register for the frame pointer, and >> it often prevents inlining. > > Well, if we're going to forbid VLAs in the kernel, IMHO the kernel build > system should call gcc with -Werror=vla to get that point across early, > and flush out any offenders. If it were up to me, that's exactly what I'd do. -- Måns Rullgård
Re: Arrays of variable length
Tomas Winkler <tom...@gmail.com> writes: > Sparse complains for arrays declared with variable length > > 'warning: Variable length array is used' > > Prior to c99 this was not allowed but lgcc (c99) doesn't have problem > with that https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html. > And also Linux kernel compilation with W=1 doesn't complain. > > Since sparse is used extensively would like to ask what is the correct > usage of arrays of variable length > within Linux Kernel. Variable-length arrays are a very bad idea. Don't use them, ever. If the size has a sane upper bound, just use that value statically. Otherwise, you have a stack overflow waiting to happen and should be using some kind of dynamic allocation instead. Furthermore, use of VLAs generally results in less efficient code. For instance, it forces gcc to waste a register for the frame pointer, and it often prevents inlining. -- Måns Rullgård
Re: Arrays of variable length
Tomas Winkler writes: > Sparse complains for arrays declared with variable length > > 'warning: Variable length array is used' > > Prior to c99 this was not allowed but lgcc (c99) doesn't have problem > with that https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html. > And also Linux kernel compilation with W=1 doesn't complain. > > Since sparse is used extensively would like to ask what is the correct > usage of arrays of variable length > within Linux Kernel. Variable-length arrays are a very bad idea. Don't use them, ever. If the size has a sane upper bound, just use that value statically. Otherwise, you have a stack overflow waiting to happen and should be using some kind of dynamic allocation instead. Furthermore, use of VLAs generally results in less efficient code. For instance, it forces gcc to waste a register for the frame pointer, and it often prevents inlining. -- Måns Rullgård
Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:
>> So far we have a claim that a cast to a void * may somehow be different
>> to a cast to a different pointer, if used as function argument, and that
>> the behavior with such a cast may be undefined. In other words, you claim
>> that a function implemented as, say,
>>
>>void func(int *var) {}
>>
>> might result in undefined behavior if some header file declares it as
>>
>> void func(void *);
>>
>> and it is called as
>>
>> int var;
>>
>> func();
>>
>> That seems really far fetched to me.
>
> Thanks for giving me an opportunity to play the language lawyer :-)
>
> C99 6.3.2.3 sub-clause 8 states:
>
> "A pointer to a function of one type may be converted to a pointer to
> a function of another type and back again; the result shall compare
> equal to the original pointer. If a converted pointer is used to call
> a function whose type is not compatible with the pointed-to type, the
> behavior is undefined."
>
> So, the behavior is undefined, not when you cast clk_disable_unprepare,
> but when clk_disable_unprepare is later called through the devres->action
> function pointer.
Only if the function types are incompatible. C99 6.7.5.3 subclause 15:
For two function types to be compatible, both shall specify compatible
return types. Moreover, the parameter type lists, if both are
present, shall agree in the number of parameters and in use of the
ellipsis terminator; corresponding parameters shall have compatible
types.
The question then is whether pointer to void and pointer to struct clk
are compatible types. 6.7.5.1 subclause 2:
For two pointer types to be compatible, both shall be identically
qualified and both shall be pointers to compatible types.
6.2.5 subclause 27:
A pointer to void shall have the same representation and alignment
requirements as a pointer to a character type. 39)
39) The same representation and alignment requirements are meant to
imply interchangeability as arguments to functions, return values
from functions, and members of unions.
6.3.2.3 subclause 1:
A pointer to void may be converted to or from a pointer to any
incomplete or object type.
>From what I can tell, the standard stops just short of declaring pointer
to void compatible with other pointer types.
> However, I agree that it will work as expected on typical platforms
> (where all pointers are the same size, and the calling convention
> treats all pointers the same).
Yes, I don't see how it could possibly go wrong.
--
Måns Rullgård
Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions
Marc Gonzalez writes:
>> So far we have a claim that a cast to a void * may somehow be different
>> to a cast to a different pointer, if used as function argument, and that
>> the behavior with such a cast may be undefined. In other words, you claim
>> that a function implemented as, say,
>>
>>void func(int *var) {}
>>
>> might result in undefined behavior if some header file declares it as
>>
>> void func(void *);
>>
>> and it is called as
>>
>> int var;
>>
>> func();
>>
>> That seems really far fetched to me.
>
> Thanks for giving me an opportunity to play the language lawyer :-)
>
> C99 6.3.2.3 sub-clause 8 states:
>
> "A pointer to a function of one type may be converted to a pointer to
> a function of another type and back again; the result shall compare
> equal to the original pointer. If a converted pointer is used to call
> a function whose type is not compatible with the pointed-to type, the
> behavior is undefined."
>
> So, the behavior is undefined, not when you cast clk_disable_unprepare,
> but when clk_disable_unprepare is later called through the devres->action
> function pointer.
Only if the function types are incompatible. C99 6.7.5.3 subclause 15:
For two function types to be compatible, both shall specify compatible
return types. Moreover, the parameter type lists, if both are
present, shall agree in the number of parameters and in use of the
ellipsis terminator; corresponding parameters shall have compatible
types.
The question then is whether pointer to void and pointer to struct clk
are compatible types. 6.7.5.1 subclause 2:
For two pointer types to be compatible, both shall be identically
qualified and both shall be pointers to compatible types.
6.2.5 subclause 27:
A pointer to void shall have the same representation and alignment
requirements as a pointer to a character type. 39)
39) The same representation and alignment requirements are meant to
imply interchangeability as arguments to functions, return values
from functions, and members of unions.
6.3.2.3 subclause 1:
A pointer to void may be converted to or from a pointer to any
incomplete or object type.
>From what I can tell, the standard stops just short of declaring pointer
to void compatible with other pointer types.
> However, I agree that it will work as expected on typical platforms
> (where all pointers are the same size, and the calling convention
> treats all pointers the same).
Yes, I don't see how it could possibly go wrong.
--
Måns Rullgård
Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions
Guenter Roeck <li...@roeck-us.net> writes: > On 01/11/2017 04:31 AM, Marc Gonzalez wrote: >> On 11/01/2017 11:52, Guenter Roeck wrote: >> >>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote: >>> >>>>> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device >>>>> *pdev) >>>>> err = clk_prepare_enable(dev->clk); >>>>> if (err) >>>>> return err; >>>>> + err = devm_add_action_or_reset(>dev, >>>>> +(void(*)(void *))clk_disable_unprepare, >>>>> +dev->clk); >>>>> + if (err) >>>>> + return err; >>>> >>>> Hello Guenter, >>>> >>>> I would rather avoid the function pointer cast. >>>> How about defining an auxiliary function for the cleanup action? >>>> >>>> clk_disable_unprepare() is static inline, so gcc will have to >>>> define an auxiliary function either way. What do you think? >>> >>> Not really. It would just make it more complicated to replace the >>> call with devm_clk_prepare_enable(), should it ever find its way >>> into the light of day. >> >> More complicated, because the cleanup function will have to be deleted later? >> The compiler will warn if someone forgets to do that. >> >> In my opinion, it's not a good idea to rely on the fact that casting >> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected >> on most platforms. (It has undefined behavior, strictly speaking.) >> > I do hear that you object to this code. > > However, I must admit that you completely lost me here. It is a cast from > one function pointer to another, passed as argument to another function, > with a secondary cast of its argument from a typed pointer to a void pointer. > I don't think C permits for "undefined behavior, strictly speaking". > Besides, that same mechanism is already used elsewhere, which is how I > got the idea. Are you claiming that there are situations where it won't > work ? A pointer to void is interchangeable with any other pointer type. That doesn't necessarily imply that pointers to functions taking arguments of different pointer types (as we have here) are always compatible. I'd have to read the C standard carefully to see if there's any such promise, and I have other things to do right now. I am, however, not aware of any ABI (certainly none used by Linux) where it would pose a problem. -- Måns Rullgård
Re: [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions
Guenter Roeck writes: > On 01/11/2017 04:31 AM, Marc Gonzalez wrote: >> On 11/01/2017 11:52, Guenter Roeck wrote: >> >>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote: >>> >>>>> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device >>>>> *pdev) >>>>> err = clk_prepare_enable(dev->clk); >>>>> if (err) >>>>> return err; >>>>> + err = devm_add_action_or_reset(>dev, >>>>> +(void(*)(void *))clk_disable_unprepare, >>>>> +dev->clk); >>>>> + if (err) >>>>> + return err; >>>> >>>> Hello Guenter, >>>> >>>> I would rather avoid the function pointer cast. >>>> How about defining an auxiliary function for the cleanup action? >>>> >>>> clk_disable_unprepare() is static inline, so gcc will have to >>>> define an auxiliary function either way. What do you think? >>> >>> Not really. It would just make it more complicated to replace the >>> call with devm_clk_prepare_enable(), should it ever find its way >>> into the light of day. >> >> More complicated, because the cleanup function will have to be deleted later? >> The compiler will warn if someone forgets to do that. >> >> In my opinion, it's not a good idea to rely on the fact that casting >> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected >> on most platforms. (It has undefined behavior, strictly speaking.) >> > I do hear that you object to this code. > > However, I must admit that you completely lost me here. It is a cast from > one function pointer to another, passed as argument to another function, > with a secondary cast of its argument from a typed pointer to a void pointer. > I don't think C permits for "undefined behavior, strictly speaking". > Besides, that same mechanism is already used elsewhere, which is how I > got the idea. Are you claiming that there are situations where it won't > work ? A pointer to void is interchangeable with any other pointer type. That doesn't necessarily imply that pointers to functions taking arguments of different pointer types (as we have here) are always compatible. I'd have to read the C standard carefully to see if there's any such promise, and I have other things to do right now. I am, however, not aware of any ABI (certainly none used by Linux) where it would pose a problem. -- Måns Rullgård
Re: [PATCH 4/4] watchdog: tangox: Use watchdog core to install restart handler
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:
> [ Adding Mans ]
>
> Guenter, patch c7ef68c32265 states "Fixes: a3e376d26ace".
> Is that true? I mean, they seem quite orthogonal; then again I know
> nothing of this framework.
I don't see the relation of either of those to this patch.
> On 04/01/2017 22:28, Guenter Roeck wrote:
>> Use the infrastructure provided by the watchdog core to install
>> the restart handler.
>>
>> Signed-off-by: Guenter Roeck <li...@roeck-us.net>
>> ---
>> drivers/watchdog/tangox_wdt.c | 32 +++-
>> 1 file changed, 11 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
>> index 202c4b9cc921..49e6e805db7c 100644
>> --- a/drivers/watchdog/tangox_wdt.c
>> +++ b/drivers/watchdog/tangox_wdt.c
>> @@ -15,9 +15,7 @@
>> #include
>> #include
>> #include
>> -#include
>> #include
>> -#include
>> #include
>>
>> #define DEFAULT_TIMEOUT 30
>> @@ -47,7 +45,6 @@ struct tangox_wdt_device {
>> void __iomem *base;
>> unsigned long clk_rate;
>> struct clk *clk;
>> -struct notifier_block restart;
>> };
>>
>> static int tangox_wdt_set_timeout(struct watchdog_device *wdt,
>> @@ -96,24 +93,24 @@ static const struct watchdog_info tangox_wdt_info = {
>> .identity = "tangox watchdog",
>> };
>>
>> +static int tangox_wdt_restart(struct watchdog_device *wdt,
>> + unsigned long action, void *data)
>> +{
>> +struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt);
>> +
>> +writel(1, dev->base + WD_COUNTER);
>> +
>> +return 0;
>> +}
>> +
>> static const struct watchdog_ops tangox_wdt_ops = {
>> .start = tangox_wdt_start,
>> .stop = tangox_wdt_stop,
>> .set_timeout= tangox_wdt_set_timeout,
>> .get_timeleft = tangox_wdt_get_timeleft,
>> +.restart= tangox_wdt_restart,
>> };
>>
>> -static int tangox_wdt_restart(struct notifier_block *nb, unsigned long
>> action,
>> - void *data)
>> -{
>> -struct tangox_wdt_device *dev =
>> -container_of(nb, struct tangox_wdt_device, restart);
>> -
>> -writel(1, dev->base + WD_COUNTER);
>> -
>> -return NOTIFY_DONE;
>> -}
>> -
>> static int tangox_wdt_probe(struct platform_device *pdev)
>> {
>> struct tangox_wdt_device *dev;
>> @@ -180,12 +177,6 @@ static int tangox_wdt_probe(struct platform_device
>> *pdev)
>>
>> platform_set_drvdata(pdev, dev);
>>
>> -dev->restart.notifier_call = tangox_wdt_restart;
>> -dev->restart.priority = 128;
>> - err = register_restart_handler(>restart);
>> -if (err)
>> -dev_warn(>dev, "failed to register restart handler\n");
>> -
>> dev_info(>dev, "SMP86xx/SMP87xx watchdog registered\n");
>>
>> return 0;
>> @@ -202,7 +193,6 @@ static int tangox_wdt_remove(struct platform_device
>> *pdev)
>> tangox_wdt_stop(>wdt);
>> clk_disable_unprepare(dev->clk);
>>
>> -unregister_restart_handler(>restart);
>> watchdog_unregister_device(>wdt);
>>
>> return 0;
>>
--
Måns Rullgård
Re: [PATCH 4/4] watchdog: tangox: Use watchdog core to install restart handler
Marc Gonzalez writes:
> [ Adding Mans ]
>
> Guenter, patch c7ef68c32265 states "Fixes: a3e376d26ace".
> Is that true? I mean, they seem quite orthogonal; then again I know
> nothing of this framework.
I don't see the relation of either of those to this patch.
> On 04/01/2017 22:28, Guenter Roeck wrote:
>> Use the infrastructure provided by the watchdog core to install
>> the restart handler.
>>
>> Signed-off-by: Guenter Roeck
>> ---
>> drivers/watchdog/tangox_wdt.c | 32 +++-
>> 1 file changed, 11 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/watchdog/tangox_wdt.c b/drivers/watchdog/tangox_wdt.c
>> index 202c4b9cc921..49e6e805db7c 100644
>> --- a/drivers/watchdog/tangox_wdt.c
>> +++ b/drivers/watchdog/tangox_wdt.c
>> @@ -15,9 +15,7 @@
>> #include
>> #include
>> #include
>> -#include
>> #include
>> -#include
>> #include
>>
>> #define DEFAULT_TIMEOUT 30
>> @@ -47,7 +45,6 @@ struct tangox_wdt_device {
>> void __iomem *base;
>> unsigned long clk_rate;
>> struct clk *clk;
>> -struct notifier_block restart;
>> };
>>
>> static int tangox_wdt_set_timeout(struct watchdog_device *wdt,
>> @@ -96,24 +93,24 @@ static const struct watchdog_info tangox_wdt_info = {
>> .identity = "tangox watchdog",
>> };
>>
>> +static int tangox_wdt_restart(struct watchdog_device *wdt,
>> + unsigned long action, void *data)
>> +{
>> +struct tangox_wdt_device *dev = watchdog_get_drvdata(wdt);
>> +
>> +writel(1, dev->base + WD_COUNTER);
>> +
>> +return 0;
>> +}
>> +
>> static const struct watchdog_ops tangox_wdt_ops = {
>> .start = tangox_wdt_start,
>> .stop = tangox_wdt_stop,
>> .set_timeout= tangox_wdt_set_timeout,
>> .get_timeleft = tangox_wdt_get_timeleft,
>> +.restart= tangox_wdt_restart,
>> };
>>
>> -static int tangox_wdt_restart(struct notifier_block *nb, unsigned long
>> action,
>> - void *data)
>> -{
>> -struct tangox_wdt_device *dev =
>> -container_of(nb, struct tangox_wdt_device, restart);
>> -
>> -writel(1, dev->base + WD_COUNTER);
>> -
>> -return NOTIFY_DONE;
>> -}
>> -
>> static int tangox_wdt_probe(struct platform_device *pdev)
>> {
>> struct tangox_wdt_device *dev;
>> @@ -180,12 +177,6 @@ static int tangox_wdt_probe(struct platform_device
>> *pdev)
>>
>> platform_set_drvdata(pdev, dev);
>>
>> -dev->restart.notifier_call = tangox_wdt_restart;
>> -dev->restart.priority = 128;
>> -err = register_restart_handler(>restart);
>> -if (err)
>> -dev_warn(>dev, "failed to register restart handler\n");
>> -
>> dev_info(>dev, "SMP86xx/SMP87xx watchdog registered\n");
>>
>> return 0;
>> @@ -202,7 +193,6 @@ static int tangox_wdt_remove(struct platform_device
>> *pdev)
>> tangox_wdt_stop(>wdt);
>> clk_disable_unprepare(dev->clk);
>>
>> -unregister_restart_handler(>restart);
>> watchdog_unregister_device(>wdt);
>>
>> return 0;
>>
--
Måns Rullgård
Re: Moratorium on coding style patches (was Re: [PATCH] include/linux/kernel.h: fixed coding style issues)
Joe Perches <j...@perches.com> writes: > On Wed, 2016-12-14 at 18:13 +0000, Måns Rullgård wrote: >> Alexey Dobriyan <adobri...@gmail.com> writes: >> > I call for a tree wide moratorium on pure coding style changes. > [] >> I'd relax this slightly and say don't do style >> patches unless as a prelude to some real work. > > And exclude drivers/staging I wouldn't mind seeing staging deleted. What makes those drivers so special that they deserve to be included in the main tree without meeting even the most basic of standards normally required? -- Måns Rullgård
Re: Moratorium on coding style patches (was Re: [PATCH] include/linux/kernel.h: fixed coding style issues)
Joe Perches writes: > On Wed, 2016-12-14 at 18:13 +0000, Måns Rullgård wrote: >> Alexey Dobriyan writes: >> > I call for a tree wide moratorium on pure coding style changes. > [] >> I'd relax this slightly and say don't do style >> patches unless as a prelude to some real work. > > And exclude drivers/staging I wouldn't mind seeing staging deleted. What makes those drivers so special that they deserve to be included in the main tree without meeting even the most basic of standards normally required? -- Måns Rullgård
Re: Moratorium on coding style patches (was Re: [PATCH] include/linux/kernel.h: fixed coding style issues)
Alexey Dobriyan <adobri...@gmail.com> writes: > OK, someone needs to say it. > > These type of patches are advertised by some people as a good way > to enter Linux kernel development. You know, to learn how the process > works, how to setup email client pipeline, to get initial feedback. > And it is true. What those people aren't saying is that the above is > about ~0.01% of what is kernel or any other project development is about. > It is the easy part. > > But the patches also create problems for those who are already in. > The very immediate is that "git-blame" stops working. It simply points > to the irrelevant commit and developer is forced to either search > manually through the history or search the web for "git-blame" options. > (maybe there is such an option but that's not the point). > > And "git-blame" usually happens in important cases: when developer > searches for a possible bugfix or wondering who wrote that crap. > > Au contraire, coding style patch is something unimportant. > Whitespace here, whitespace there, who cares. On the grand scale, > coding style compliance is important but in my experience Linux > kernel CS compliance is top notch for the project of Linux's size. > > So the tradeoff is not in the patch favour and all you need to follow > coding style is basically "indent -kr -i8 -l80" for new code. > It is then becomes non problem because editors defaults are doing > the job. > > And they create rejects against other non-coding style patches, > again slowing down people who need to fix real problems. I agree with all of that, yet I still sometimes create pure style patches. This tends to happen when I can't look at the code I'm trying to debug without wanting to claw my eyes out. In such cases, an initial cleanup patch often makes the actual fix much easier to comprehend. There's not a lot such code in the kernel, thankfully, but it does exist. > Said that, I call for a tree wide moratorium on pure coding style changes. In light of the above, I'd relax this slightly and say don't do style patches unless as a prelude to some real work. -- Måns Rullgård
Re: Moratorium on coding style patches (was Re: [PATCH] include/linux/kernel.h: fixed coding style issues)
Alexey Dobriyan writes: > OK, someone needs to say it. > > These type of patches are advertised by some people as a good way > to enter Linux kernel development. You know, to learn how the process > works, how to setup email client pipeline, to get initial feedback. > And it is true. What those people aren't saying is that the above is > about ~0.01% of what is kernel or any other project development is about. > It is the easy part. > > But the patches also create problems for those who are already in. > The very immediate is that "git-blame" stops working. It simply points > to the irrelevant commit and developer is forced to either search > manually through the history or search the web for "git-blame" options. > (maybe there is such an option but that's not the point). > > And "git-blame" usually happens in important cases: when developer > searches for a possible bugfix or wondering who wrote that crap. > > Au contraire, coding style patch is something unimportant. > Whitespace here, whitespace there, who cares. On the grand scale, > coding style compliance is important but in my experience Linux > kernel CS compliance is top notch for the project of Linux's size. > > So the tradeoff is not in the patch favour and all you need to follow > coding style is basically "indent -kr -i8 -l80" for new code. > It is then becomes non problem because editors defaults are doing > the job. > > And they create rejects against other non-coding style patches, > again slowing down people who need to fix real problems. I agree with all of that, yet I still sometimes create pure style patches. This tends to happen when I can't look at the code I'm trying to debug without wanting to claw my eyes out. In such cases, an initial cleanup patch often makes the actual fix much easier to comprehend. There's not a lot such code in the kernel, thankfully, but it does exist. > Said that, I call for a tree wide moratorium on pure coding style changes. In light of the above, I'd relax this slightly and say don't do style patches unless as a prelude to some real work. -- Måns Rullgård
Re: Misalignment, MIPS, and ip_hdr(skb)->version
David Laight <david.lai...@aculab.com> writes: > From: Måns Rullgård >> Sent: 10 December 2016 13:25 > ... >> I solved this problem in an Ethernet driver by copying the initial part >> of the packet to an aligned skb and appending the remainder using >> skb_add_rx_frag(). The kernel network stack only cares about the >> headers, so the alignment of the packet payload doesn't matter. > > That rather depends on where the packet payload ends up. > It is likely that it will be copied to userspace (or maybe > into some aligned kernel buffer). > In which case you get an expensive misaligned copy. There's not much to be done about that. The only other option is to bypass DMA entirely, and that's sure to be even worse. > What do the hardware engineers think the ethernet interface will > be used for! Ticking boxes in marketing material. -- Måns Rullgård
Re: Misalignment, MIPS, and ip_hdr(skb)->version
David Laight writes: > From: Måns Rullgård >> Sent: 10 December 2016 13:25 > ... >> I solved this problem in an Ethernet driver by copying the initial part >> of the packet to an aligned skb and appending the remainder using >> skb_add_rx_frag(). The kernel network stack only cares about the >> headers, so the alignment of the packet payload doesn't matter. > > That rather depends on where the packet payload ends up. > It is likely that it will be copied to userspace (or maybe > into some aligned kernel buffer). > In which case you get an expensive misaligned copy. There's not much to be done about that. The only other option is to bypass DMA entirely, and that's sure to be even worse. > What do the hardware engineers think the ethernet interface will > be used for! Ticking boxes in marketing material. -- Måns Rullgård
Re: Misalignment, MIPS, and ip_hdr(skb)->version
Willy Tarreau <w...@1wt.eu> writes: > Hi Jason, > > On Thu, Dec 08, 2016 at 11:20:04PM +0100, Jason A. Donenfeld wrote: >> Hi David, >> >> On Thu, Dec 8, 2016 at 1:37 AM, David Miller <da...@davemloft.net> wrote: >> > You really have to land the IP header on a proper 4 byte boundary. >> > >> > I would suggest pushing 3 dummy garbage bytes of padding at the front >> > or the end of your header. >> >> Are you sure 3 bytes to get 4 byte alignment is really the best? > > It's always the best. However there's another option which should be > considered : maybe it's difficult but not impossible to move some bits > from the current protocol to remove one byte. That's not always easy, > and sometimes you cannot do it just for one bit. However after you run > through this exercise, if you notice there's really no way to shave > this extra byte, you'll realize there's no room left for future > extensions and you'll more easily accept to add 3 empty bytes for > this, typically protocol version, tags, qos or flagss that you'll be > happy to rely on for future versions of your protocol. Always include some way of extending the protocol in the future. A single bit is often enough. Require a value of zero initially, then if you ever want to change anything, setting it to one can indicate whatever you want, including a complete redesign of the header. Alternatively, a one-bit field can indicate the presence of an extended header yet to be defined. Then old software can still make sense of the basic header. -- Måns Rullgård
Re: Misalignment, MIPS, and ip_hdr(skb)->version
Willy Tarreau writes: > Hi Jason, > > On Thu, Dec 08, 2016 at 11:20:04PM +0100, Jason A. Donenfeld wrote: >> Hi David, >> >> On Thu, Dec 8, 2016 at 1:37 AM, David Miller wrote: >> > You really have to land the IP header on a proper 4 byte boundary. >> > >> > I would suggest pushing 3 dummy garbage bytes of padding at the front >> > or the end of your header. >> >> Are you sure 3 bytes to get 4 byte alignment is really the best? > > It's always the best. However there's another option which should be > considered : maybe it's difficult but not impossible to move some bits > from the current protocol to remove one byte. That's not always easy, > and sometimes you cannot do it just for one bit. However after you run > through this exercise, if you notice there's really no way to shave > this extra byte, you'll realize there's no room left for future > extensions and you'll more easily accept to add 3 empty bytes for > this, typically protocol version, tags, qos or flagss that you'll be > happy to rely on for future versions of your protocol. Always include some way of extending the protocol in the future. A single bit is often enough. Require a value of zero initially, then if you ever want to change anything, setting it to one can indicate whatever you want, including a complete redesign of the header. Alternatively, a one-bit field can indicate the presence of an extended header yet to be defined. Then old software can still make sense of the basic header. -- Måns Rullgård
Re: Misalignment, MIPS, and ip_hdr(skb)->version
Felix Fietkau <n...@nbd.name> writes: > On 2016-12-10 14:25, Måns Rullgård wrote: >> Felix Fietkau <n...@nbd.name> writes: >> >>> On 2016-12-07 19:54, Jason A. Donenfeld wrote: >>>> On Wed, Dec 7, 2016 at 7:51 PM, David Miller <da...@davemloft.net> wrote: >>>>> It's so much better to analyze properly where the misalignment comes from >>>>> and address it at the source, as we have for various cases that trip up >>>>> Sparc too. >>>> >>>> That's sort of my attitude too, hence starting this thread. Any >>>> pointers you have about this would be most welcome, so as not to >>>> perpetuate what already seems like an issue in other parts of the >>>> stack. >>> Hi Jason, >>> >>> I'm the author of that hackish LEDE/OpenWrt patch that works around the >>> misalignment issues. Here's some context regarding that patch: >>> >>> I intentionally put it in the target specific patches for only one of >>> our MIPS targets. There are a few ar71xx devices where the misalignment >>> cannot be fixed, because the Ethernet MAC has a 4-byte DMA alignment >>> requirement, and does not support inserting 2 bytes of padding to >>> correct the IP header misalignment. >>> >>> With these limitations the choice was between this ugly network stack >>> patch or inserting a very expensive memmove in the data path (which is >>> better than taking the mis-alignment traps, but still hurts routing >>> performance significantly). >> >> I solved this problem in an Ethernet driver by copying the initial part >> of the packet to an aligned skb and appending the remainder using >> skb_add_rx_frag(). The kernel network stack only cares about the >> headers, so the alignment of the packet payload doesn't matter. > > I considered that as well, but it's bad for routing performance if the > ethernet MAC does not support scatter/gather for xmit. > Unfortunately that limitation is quite common on embedded hardware. Yes, I can see that being an issue. However, if you're doing zero-copy routing, the header part of the original buffer should still be there, unused, so you could presumably copy the header of the outgoing packet there and then do dma as usual. Maybe there's something in the network stack that makes this impossible though. -- Måns Rullgård
Re: Misalignment, MIPS, and ip_hdr(skb)->version
Felix Fietkau writes: > On 2016-12-10 14:25, Måns Rullgård wrote: >> Felix Fietkau writes: >> >>> On 2016-12-07 19:54, Jason A. Donenfeld wrote: >>>> On Wed, Dec 7, 2016 at 7:51 PM, David Miller wrote: >>>>> It's so much better to analyze properly where the misalignment comes from >>>>> and address it at the source, as we have for various cases that trip up >>>>> Sparc too. >>>> >>>> That's sort of my attitude too, hence starting this thread. Any >>>> pointers you have about this would be most welcome, so as not to >>>> perpetuate what already seems like an issue in other parts of the >>>> stack. >>> Hi Jason, >>> >>> I'm the author of that hackish LEDE/OpenWrt patch that works around the >>> misalignment issues. Here's some context regarding that patch: >>> >>> I intentionally put it in the target specific patches for only one of >>> our MIPS targets. There are a few ar71xx devices where the misalignment >>> cannot be fixed, because the Ethernet MAC has a 4-byte DMA alignment >>> requirement, and does not support inserting 2 bytes of padding to >>> correct the IP header misalignment. >>> >>> With these limitations the choice was between this ugly network stack >>> patch or inserting a very expensive memmove in the data path (which is >>> better than taking the mis-alignment traps, but still hurts routing >>> performance significantly). >> >> I solved this problem in an Ethernet driver by copying the initial part >> of the packet to an aligned skb and appending the remainder using >> skb_add_rx_frag(). The kernel network stack only cares about the >> headers, so the alignment of the packet payload doesn't matter. > > I considered that as well, but it's bad for routing performance if the > ethernet MAC does not support scatter/gather for xmit. > Unfortunately that limitation is quite common on embedded hardware. Yes, I can see that being an issue. However, if you're doing zero-copy routing, the header part of the original buffer should still be there, unused, so you could presumably copy the header of the outgoing packet there and then do dma as usual. Maybe there's something in the network stack that makes this impossible though. -- Måns Rullgård
