| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2ef2a0c2-cb71-c6d0-1dbc-386c55019817@roeck-us.net>
Date: Mon, 19 Sep 2016 00:04:40 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Vadim Pasternak <vadimp@...lanox.com>,
"tglx@...utronix.de" <tglx@...utronix.de>
Cc: "mingo@...hat.com" <mingo@...hat.com>,
"hpa@...or.com" <hpa@...or.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"geert@...ux-m68k.org" <geert@...ux-m68k.org>,
"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"kvalo@...eaurora.org" <kvalo@...eaurora.org>,
"mchehab@...nel.org" <mchehab@...nel.org>,
"x86@...nel.org" <x86@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"platform-driver-x86@...r.kernel.org"
<platform-driver-x86@...r.kernel.org>,
"jiri@...nulli.us" <jiri@...nulli.us>
Subject: Re: [patch v2] x86/platform/mellanox: introduce support for Mellanox
systems platform
On 09/18/2016 11:12 PM, Vadim Pasternak wrote:
>
>
>> -----Original Message-----
>> From: Guenter Roeck [mailto:linux@...ck-us.net]
>> Sent: Monday, September 19, 2016 9:03 AM
>> To: Vadim Pasternak <vadimp@...lanox.com>; tglx@...utronix.de
>> Cc: mingo@...hat.com; hpa@...or.com; davem@...emloft.net; geert@...ux-
>> m68k.org; akpm@...ux-foundation.org; gregkh@...uxfoundation.org;
>> kvalo@...eaurora.org; mchehab@...nel.org; x86@...nel.org; linux-
>> kernel@...r.kernel.org; platform-driver-x86@...r.kernel.org; jiri@...nulli.us
>> Subject: Re: [patch v2] x86/platform/mellanox: introduce support for Mellanox
>> systems platform
>>
>> On 09/15/2016 02:55 AM, vadimp@...lanox.com wrote:
>>> From: Vadim Pasternak <vadimp@...lanox.com>
>>>
>>> Enable system support for the Mellanox Technologies platform, which
>>> provides support for the next Mellanox basic systems: "msx6710",
>>> "msx6720", "msb7700", "msn2700", "msx1410", "msn2410", "msb7800",
>>> "msn2740", "msn2100" and also various number of derivative systems
>>> from the above basic types.
>>>
>>> The Kconfig currently controlling compilation of this code is:
>>> arch/x86/platform:config MLX_PLATFORM
>>> arch/x86/platform: tristate "Mellanox Technologies platform support"
>>>
>>> Signed-off-by: Vadim Pasternak <vadimp@...lanox.com>
>>> ---
>>> v1->v2:
>>> Comments pointed out by Greg:
>>> - kick out all PCI related code;
>>> ---
>>> MAINTAINERS | 7 +
>>> arch/x86/Kconfig | 25 +++
>>> arch/x86/platform/Makefile | 1 +
>>> arch/x86/platform/mellanox/Makefile | 1 +
>>> arch/x86/platform/mellanox/mlx-platform.c | 275
>>> ++++++++++++++++++++++++++++++
>>> 5 files changed, 309 insertions(+)
>>> create mode 100644 arch/x86/platform/mellanox/Makefile
>>> create mode 100644 arch/x86/platform/mellanox/mlx-platform.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS index 4705c94..8a675de 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -7664,6 +7664,13 @@ W: http://www.mellanox.com
>>> Q: http://patchwork.ozlabs.org/project/netdev/list/
>>> F: drivers/net/ethernet/mellanox/mlxsw/
>>>
>>> +MELLANOX PLATFORM DRIVER
>>> +M: Vadim Pasternak <vadimp@...lanox.com>
>>> +L: platform-driver-x86@...r.kernel.org
>>> +S: Supported
>>> +W: http://www.mellanox.com
>>> +F: arch/x86/platform/mellanox/mlx-platform.c
>>> +
>>> SOFT-ROCE DRIVER (rxe)
>>> M: Moni Shoua <monis@...lanox.com>
>>> L: linux-rdma@...r.kernel.org
>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index
>>> c580d8c..1346441 100644
>>> --- a/arch/x86/Kconfig
>>> +++ b/arch/x86/Kconfig
>>> @@ -550,6 +550,31 @@ config X86_INTEL_QUARK
>>> Say Y here if you have a Quark based system such as the Arduino
>>> compatible Intel Galileo.
>>>
>>> +config MLX_PLATFORM
>>> + tristate "Mellanox Technologies platform support"
>>> + depends on X86_64
>>> + depends on X86_EXTENDED_PLATFORM
>>> + depends on PCI
>>
>> No longer needed.
>>
>>> + depends on DMI
>>> + depends on I2C_MLXCPLD
>>> + depends on I2C_MUX_REG
>>> + select HWMON
>>> + select PMBUS
>>> + select SENSORS_PMBUS
>>> + select LM75
>>> + select NEW_LEDS
>>> + select LEDS_CLASS
>>> + select LEDS_TRIGGERS
>>> + select LEDS_TRIGGER_TIMER
>>> + select LEDS_MLXCPLD
>>
>> Kind of unusual to select drivers not directly related to this one like this.
>
> These selection is required by all systems.
> Do you suggest to remove all that is not related directly?
>
Normally unrelated drivers would be selected by the configuration file.
Otherwise you could argue that you want to select _everything_ that is
normally in the configuration file here. IP, IPV6, networking, ...
>>
>>> + ---help---
>>> + This option enables system support for the Mellanox Technologies
>>> + platform.
>>> +
>>> + Say Y here if you are building a kernel for Mellanox system.
>>> +
>>> + Otherwise, say N.
>>> +
>>> config X86_INTEL_LPSS
>>> bool "Intel Low Power Subsystem Support"
>>> depends on X86 && ACPI
>>> diff --git a/arch/x86/platform/Makefile b/arch/x86/platform/Makefile
>>> index 184842e..3c3c19e 100644
>>> --- a/arch/x86/platform/Makefile
>>> +++ b/arch/x86/platform/Makefile
>>> @@ -8,6 +8,7 @@ obj-y += iris/
>>> obj-y += intel/
>>> obj-y += intel-mid/
>>> obj-y += intel-quark/
>>> +obj-y += mellanox/
>>> obj-y += olpc/
>>> obj-y += scx200/
>>> obj-y += sfi/
>>> diff --git a/arch/x86/platform/mellanox/Makefile
>>> b/arch/x86/platform/mellanox/Makefile
>>> new file mode 100644
>>> index 0000000..f43c931
>>> --- /dev/null
>>> +++ b/arch/x86/platform/mellanox/Makefile
>>> @@ -0,0 +1 @@
>>> +obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o
>>> diff --git a/arch/x86/platform/mellanox/mlx-platform.c
>>> b/arch/x86/platform/mellanox/mlx-platform.c
>>> new file mode 100644
>>> index 0000000..d29da68
>>> --- /dev/null
>>> +++ b/arch/x86/platform/mellanox/mlx-platform.c
>>> @@ -0,0 +1,275 @@
>>> +/*
>>> + * arch/x86/platform/mellanox/mlx-platform.c
>>> + * Copyright (c) 2016 Mellanox Technologies. All rights reserved.
>>> + * Copyright (c) 2016 Vadim Pasternak <vadimp@...lanox.com>
>>> + *
>>> + * Redistribution and use in source and binary forms, with or without
>>> + * modification, are permitted provided that the following conditions are
>> met:
>>> + *
>>> + * 1. Redistributions of source code must retain the above copyright
>>> + * notice, this list of conditions and the following disclaimer.
>>> + * 2. Redistributions in binary form must reproduce the above copyright
>>> + * notice, this list of conditions and the following disclaimer in the
>>> + * documentation and/or other materials provided with the distribution.
>>> + * 3. Neither the names of the copyright holders nor the names of its
>>> + * contributors may be used to endorse or promote products derived from
>>> + * this software without specific prior written permission.
>>> + *
>>> + * Alternatively, this software may be distributed under the terms of
>>> +the
>>> + * GNU General Public License ("GPL") version 2 as published by the
>>> +Free
>>> + * Software Foundation.
>>> + *
>>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
>> CONTRIBUTORS "AS IS"
>>> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> LIMITED
>>> +TO, THE
>>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
>> PARTICULAR
>>> +PURPOSE
>>> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
>>> +CONTRIBUTORS BE
>>> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY,
>>> +OR
>>> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
>> PROCUREMENT
>>> +OF
>>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
>>> +BUSINESS
>>> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
>>> +WHETHER IN
>>> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
>>> +OTHERWISE)
>>> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
>>> +ADVISED OF THE
>>> + * POSSIBILITY OF SUCH DAMAGE.
>>> + */
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/dmi.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/i2c-mux.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/platform_data/i2c-mux-reg.h>
>>> +
>>> +#define MLX_PLAT_DEVICE_NAME "mlxplat"
>>> +
>>> +/* LPC bus IO offsets */
>>> +#define MLXPLAT_CPLD_LPC_I2C_BASE_ADRR 0x2000
>>> +#define MLXPLAT_CPLD_LPC_REG_BASE_ADRR 0x2500
>>> +#define MLXPLAT_CPLD_LPC_IO_RANGE 0x100
>>> +#define MLXPLAT_CPLD_LPC_I2C_CH1_OFF 0xdb
>>> +#define MLXPLAT_CPLD_LPC_I2C_CH2_OFF 0xda
>>> +#define MLXPLAT_CPLD_LPC_PIO_OFFSET 0x10000UL
>>> +#define MLXPLAT_CPLD_LPC_REG1
>> ((MLXPLAT_CPLD_LPC_REG_BASE_ADRR + \
>>> + MLXPLAT_CPLD_LPC_I2C_CH1_OFF) | \
>>> + MLXPLAT_CPLD_LPC_PIO_OFFSET)
>>> +#define MLXPLAT_CPLD_LPC_REG2
>> ((MLXPLAT_CPLD_LPC_REG_BASE_ADRR + \
>>> + MLXPLAT_CPLD_LPC_I2C_CH2_OFF) | \
>>> + MLXPLAT_CPLD_LPC_PIO_OFFSET)
>>> +
>>> +/* Start channel numbers */
>>> +#define MLXPLAT_CPLD_CH1 2
>>> +#define MLXPLAT_CPLD_CH2 10
>>> +
>>> +/* Number of LPC attached MUX platform devices */
>>> +#define MLXPLAT_CPLD_LPC_MUX_DEVS 2
>>> +
>>> +/* mlxplat_priv - platform private data
>>> + * @pdev_i2c - i2c controller platform device
>>> + * @pdev_mux - array of mux platform devices */ struct mlxplat_priv
>>> +{
>>> + struct platform_device *pdev_i2c;
>>> + struct platform_device *pdev_mux[MLXPLAT_CPLD_LPC_MUX_DEVS];
>>> +};
>>> +
>>> +/* Regions for LPC I2C controller and LPC base register space */
>>> +static struct resource mlxplat_lpc_resources[] = {
>>> + [0] = DEFINE_RES_NAMED(MLXPLAT_CPLD_LPC_I2C_BASE_ADRR,
>>> + MLXPLAT_CPLD_LPC_IO_RANGE,
>>> + "mlxplat_cpld_lpc_i2c_ctrl", IORESOURCE_IO),
>>> + [1] = DEFINE_RES_NAMED(MLXPLAT_CPLD_LPC_REG_BASE_ADRR,
>>> + MLXPLAT_CPLD_LPC_IO_RANGE,
>>> + "mlxplat_cpld_lpc_regs",
>>> + IORESOURCE_IO),
>>> +};
>>> +
>>> +/* Platform default channels */
>>> +static int mlxplat_default_channels[][8] = {
>>> + {
>>> + MLXPLAT_CPLD_CH1, MLXPLAT_CPLD_CH1 + 1,
>> MLXPLAT_CPLD_CH1 + 2,
>>> + MLXPLAT_CPLD_CH1 + 3, MLXPLAT_CPLD_CH1 + 4,
>> MLXPLAT_CPLD_CH1 +
>>> + 5, MLXPLAT_CPLD_CH1 + 6, MLXPLAT_CPLD_CH1 + 7
>>> + },
>>> + {
>>> + MLXPLAT_CPLD_CH2, MLXPLAT_CPLD_CH2 + 1,
>> MLXPLAT_CPLD_CH2 + 2,
>>> + MLXPLAT_CPLD_CH2 + 3, MLXPLAT_CPLD_CH2 + 4,
>> MLXPLAT_CPLD_CH2 +
>>
>> Odd line split.
>>
>>> + + 5, MLXPLAT_CPLD_CH2 + 6, MLXPLAT_CPLD_CH2 + 7
>>> + },
>>> +};
>>> +
>>> +/* Platform channels for MSN21xx system family */ static int
>>> +mlxplat_msn21xx_channels[][8] = {
>>> + {
>>> + 1, 2, 3, 4, 5, 6, 7, 8
>>> + },
>>> + {
>>> + 1, 2, 3, 4, 5, 6, 7, 8
>>> + },
>>
>> Seems to be a waste. A single dimensional array should be sufficient.
>>
> OK
>
>>> +};
>>> +
>>> +/* Platform mux data */
>>> +struct i2c_mux_reg_platform_data mlxplat_mux_data[] = {
>>
>> Why global ?
>>
>>> + {
>>> + .parent = 1,
>>> + .base_nr = MLXPLAT_CPLD_CH1,
>>> + .write_only = 1,
>>> + .reg = (void __iomem *)MLXPLAT_CPLD_LPC_REG1,
>>> + .reg_size = 1,
>>> + .idle_in_use = 1,
>>> + },
>>> + {
>>> + .parent = 1,
>>> + .base_nr = MLXPLAT_CPLD_CH2,
>>> + .write_only = 1,
>>> + .reg = (void __iomem *)MLXPLAT_CPLD_LPC_REG2,
>>> + .reg_size = 1,
>>> + .idle_in_use = 1,
>>> + },
>>> +
>>> +};
>>> +
>>> +struct platform_device *mlxplat_dev;
>>> +
>>
>> Is this global on purpose ? If so, why ?
>
> No, it should be both static.
>
>>
>>> +static int __init mlxplat_dmi_default_matched(const struct
>>> +dmi_system_id *dmi) {
>>> + int i;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(mlxplat_mux_data); i++) {
>>> + mlxplat_mux_data[i].values = mlxplat_default_channels[i];
>>> + mlxplat_mux_data[i].n_values =
>>> + ARRAY_SIZE(mlxplat_default_channels[i]);
>>> + }
>>> +
>>> + return 1;
>>> +};
>>> +
>>> +static int __init mlxplat_dmi_msn21xx_matched(const struct
>>> +dmi_system_id *dmi) {
>>> + int i;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(mlxplat_mux_data); i++) {
>>> + mlxplat_mux_data[i].values = mlxplat_msn21xx_channels[i],
>>> + mlxplat_mux_data[i].n_values =
>>> + ARRAY_SIZE(mlxplat_msn21xx_channels[i]);
>>> + }
>>> +
>>> + return 1;
>>> +};
>>> +
>>> +static struct dmi_system_id mlxplat_dmi_table[] __initdata = {
>>> + {
>>> + .callback = mlxplat_dmi_default_matched,
>>> + .matches = {
>>> + DMI_MATCH(DMI_BOARD_VENDOR, "Mellanox
>> Technologies"),
>>> + DMI_MATCH(DMI_PRODUCT_NAME, "MSN24"),
>>> + },
>>> + },
>>> + {
>>> + .callback = mlxplat_dmi_default_matched,
>>> + .matches = {
>>> + DMI_MATCH(DMI_BOARD_VENDOR, "Mellanox
>> Technologies"),
>>> + DMI_MATCH(DMI_PRODUCT_NAME, "MSN27"),
>>> + },
>>> + },
>>> + {
>>> + .callback = mlxplat_dmi_default_matched,
>>> + .matches = {
>>> + DMI_MATCH(DMI_BOARD_VENDOR, "Mellanox
>> Technologies"),
>>> + DMI_MATCH(DMI_PRODUCT_NAME, "MSB"),
>>> + },
>>> + },
>>> + {
>>> + .callback = mlxplat_dmi_default_matched,
>>> + .matches = {
>>> + DMI_MATCH(DMI_BOARD_VENDOR, "Mellanox
>> Technologies"),
>>> + DMI_MATCH(DMI_PRODUCT_NAME, "MSX"),
>>> + },
>>> + },
>>> + {
>>> + .callback = mlxplat_dmi_msn21xx_matched,
>>> + .matches = {
>>> + DMI_MATCH(DMI_BOARD_VENDOR, "Mellanox
>> Technologies"),
>>> + DMI_MATCH(DMI_PRODUCT_NAME, "MSN21"),
>>> + },
>>> + },
>>> + { }
>>> +};
>>> +
>>> +static int __init mlxplat_init(void)
>>> +{
>>> + struct mlxplat_priv *priv;
>>> + int i;
>>> + int err;
>>> +
>>> + if (!dmi_check_system(mlxplat_dmi_table))
>>> + return -ENODEV;
>>> +
>>> + mlxplat_dev =
>> platform_device_register_simple(MLX_PLAT_DEVICE_NAME, -1,
>>> + mlxplat_lpc_resources,
>>> + ARRAY_SIZE(mlxplat_lpc_resources));
>>> +
>>> + if (!mlxplat_dev) {
>>> + pr_err("Alloc %s platform device failed\n",
>>> + MLX_PLAT_DEVICE_NAME);
>>
>> Error messages for memory allocations are unnecessary.
>>
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + priv = devm_kzalloc(&mlxplat_dev->dev, sizeof(struct mlxplat_priv),
>>> + GFP_KERNEL);
>>> + if (!priv) {
>>> + err = -ENOMEM;
>>> + dev_err(&mlxplat_dev->dev, "Failed to allocate
>> mlxplat_priv\n");
>>
>> Same here.
>>
>>> + goto fail_alloc;
>>> + }
>>> + platform_set_drvdata(mlxplat_dev, priv);
>>> +
>>> + priv->pdev_i2c = platform_device_register_simple("i2c_mlxcpld", -1,
>>> + NULL, 0);
>>> + if (IS_ERR(priv->pdev_i2c)) {
>>> + err = PTR_ERR(priv->pdev_i2c);
>>> + goto fail_alloc;
>>> + };
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(mlxplat_mux_data); i++) {
>>> + priv->pdev_mux[i] = platform_device_register_resndata(
>>> + &mlxplat_dev->dev,
>>> + "i2c-mux-reg", i, NULL,
>>> + 0, &mlxplat_mux_data[i],
>>> + sizeof(mlxplat_mux_data[i]));
>>> + if (IS_ERR(priv->pdev_mux[i])) {
>>> + err = PTR_ERR(priv->pdev_mux[i]);
>>> + goto fail_platform_mux_register;
>>> + }
>>> + }
>>> +
>>> + return err;
>>
>> return 0;
>>
>>> +
>>> +fail_platform_mux_register:
>>> + for (i--; i > 0 ; i--)
>>
>> >= 0 ?
>>
>>> + platform_device_unregister(priv->pdev_mux[i]);
>>> + platform_device_unregister(priv->pdev_i2c);
>>> +fail_alloc:
>>> + platform_device_unregister(mlxplat_dev);
>>> +
>>> + return err;
>>> +}
>>> +
>>> +static void __exit mlxplat_exit(void) {
>>> + struct mlxplat_priv *priv = platform_get_drvdata(mlxplat_dev);
>>> + int i;
>>> +
>>> + for (i = ARRAY_SIZE(mlxplat_mux_data) - 1; i >= 0 ; i--)
>>> + platform_device_unregister(priv->pdev_mux[i]);
>>> +
>>> + platform_device_unregister(priv->pdev_i2c);
>>> + platform_device_unregister(mlxplat_dev);
>>> +}
>>> +
>>> +module_init(mlxplat_init);
>>> +module_exit(mlxplat_exit);
>>> +
>>> +MODULE_AUTHOR("Vadim Pasternak (vadimp@...lanox.com)");
>>> +MODULE_DESCRIPTION("Mellanox platform driver");
>> MODULE_LICENSE("GPL
>>> +v2"); MODULE_ALIAS("platform:mlx-platform");
>>>
>> Should this possibly be a dmi module alias ?
>
> Do you mean just to change it to
> MODULE_ALIAS("dmi:mlx-platform"); ?
>
No. Please check how other drivers use it.
Guenter
Powered by blists - more mailing lists