Hello Andy, Linus, Arnd, Gregory, and others,
I am sending v5 of the series adding Turris Omnia MCU driver.
See the cover letters for v1, v2, v3 and v4:
https://patchwork.kernel.org/project/linux-soc/cover/[email protected]/
https://patchwork.kernel.org/project/linux-soc/cover/[email protected]/
https://patchwork.kernel.org/project/linux-soc/cover/[email protected]/
https://patchwork.kernel.org/project/linux-soc/cover/[email protected]/
Changes since v4:
- added new patches
06/11 devm-helpers: Add resource managed version of irq_create_mapping()
07/11 platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG
08/11 devm-helpers: Add resource managed version of debugfs directory create
function
09/11 platform: cznic: turris-omnia-mcu: Add support for digital message
signing via debugfs
- for changes specific to patches which were also sent in previous versions see
the notes in those patches
Marek Behún (11):
dt-bindings: arm: add cznic,turris-omnia-mcu binding
platform: cznic: Add preliminary support for Turris Omnia MCU
platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs
platform: cznic: turris-omnia-mcu: Add support for poweroff and wakeup
platform: cznic: turris-omnia-mcu: Add support for MCU watchdog
devm-helpers: Add resource managed version of irq_create_mapping()
platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG
devm-helpers: Add resource managed version of debugfs directory create
function
platform: cznic: turris-omnia-mcu: Add support for digital message
signing via debugfs
ARM: dts: turris-omnia: Add MCU system-controller node
ARM: dts: turris-omnia: Add GPIO key node for front button
.../ABI/testing/debugfs-turris-omnia-mcu | 13 +
.../sysfs-bus-i2c-devices-turris-omnia-mcu | 126 ++
.../bindings/arm/cznic,turris-omnia-mcu.yaml | 86 ++
MAINTAINERS | 5 +
.../dts/marvell/armada-385-turris-omnia.dts | 35 +-
drivers/crypto/caam/ctrl.c | 16 +-
drivers/crypto/caam/jr.c | 8 +-
drivers/gpio/gpio-mockup.c | 11 +-
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 13 +-
drivers/hwmon/hp-wmi-sensors.c | 15 +-
drivers/hwmon/mr75203.c | 15 +-
drivers/hwmon/pmbus/pmbus_core.c | 16 +-
drivers/platform/Kconfig | 2 +
drivers/platform/Makefile | 1 +
drivers/platform/cznic/Kconfig | 51 +
drivers/platform/cznic/Makefile | 10 +
.../platform/cznic/turris-omnia-mcu-base.c | 406 +++++++
.../platform/cznic/turris-omnia-mcu-debugfs.c | 216 ++++
.../platform/cznic/turris-omnia-mcu-gpio.c | 1056 +++++++++++++++++
.../cznic/turris-omnia-mcu-sys-off-wakeup.c | 258 ++++
.../platform/cznic/turris-omnia-mcu-trng.c | 89 ++
.../cznic/turris-omnia-mcu-watchdog.c | 122 ++
drivers/platform/cznic/turris-omnia-mcu.h | 214 ++++
include/linux/devm-helpers.h | 94 ++
include/linux/turris-omnia-mcu-interface.h | 238 ++++
25 files changed, 3044 insertions(+), 72 deletions(-)
create mode 100644 Documentation/ABI/testing/debugfs-turris-omnia-mcu
create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
create mode 100644 Documentation/devicetree/bindings/arm/cznic,turris-omnia-mcu.yaml
create mode 100644 drivers/platform/cznic/Kconfig
create mode 100644 drivers/platform/cznic/Makefile
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-base.c
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-debugfs.c
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-gpio.c
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.c
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-trng.c
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-watchdog.c
create mode 100644 drivers/platform/cznic/turris-omnia-mcu.h
create mode 100644 include/linux/turris-omnia-mcu-interface.h
--
2.43.2
Add resource managed version of irq_create_mapping(), to help drivers
automatically dispose a linux irq mapping when driver is detached.
The new function devm_irq_create_mapping() is not yet used, but the
action function can be used in the FSL CAAM driver.
Signed-off-by: Marek Behún <[email protected]>
---
drivers/crypto/caam/jr.c | 8 ++----
include/linux/devm-helpers.h | 54 ++++++++++++++++++++++++++++++++++++
2 files changed, 56 insertions(+), 6 deletions(-)
diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index 26eba7de3fb0..ad0295b055f8 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -7,6 +7,7 @@
* Copyright 2019, 2023 NXP
*/
+#include <linux/devm-helpers.h>
#include <linux/of_irq.h>
#include <linux/of_address.h>
#include <linux/platform_device.h>
@@ -576,11 +577,6 @@ static int caam_jr_init(struct device *dev)
return error;
}
-static void caam_jr_irq_dispose_mapping(void *data)
-{
- irq_dispose_mapping((unsigned long)data);
-}
-
/*
* Probe routine for each detected JobR subsystem.
*/
@@ -656,7 +652,7 @@ static int caam_jr_probe(struct platform_device *pdev)
return -EINVAL;
}
- error = devm_add_action_or_reset(jrdev, caam_jr_irq_dispose_mapping,
+ error = devm_add_action_or_reset(jrdev, devm_irq_mapping_drop,
(void *)(unsigned long)jrpriv->irq);
if (error)
return error;
diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
index 74891802200d..3805551fd433 100644
--- a/include/linux/devm-helpers.h
+++ b/include/linux/devm-helpers.h
@@ -24,6 +24,8 @@
*/
#include <linux/device.h>
+#include <linux/kconfig.h>
+#include <linux/irqdomain.h>
#include <linux/workqueue.h>
static inline void devm_delayed_work_drop(void *res)
@@ -76,4 +78,56 @@ static inline int devm_work_autocancel(struct device *dev,
return devm_add_action(dev, devm_work_drop, w);
}
+/**
+ * devm_irq_mapping_drop - devm action for disposing an irq mapping
+ * @res: linux irq number cast to the void * type
+ *
+ * devm_irq_mapping_drop() can be used as an action parameter for the
+ * devm_add_action_or_reset() function in order to automatically dispose
+ * a linux irq mapping when a device driver is detached.
+ */
+static inline void devm_irq_mapping_drop(void *res)
+{
+ irq_dispose_mapping((unsigned int)(unsigned long)res);
+}
+
+/**
+ * devm_irq_create_mapping - Resource managed version of irq_create_mapping()
+ * @dev: Device which lifetime the mapping is bound to
+ * @domain: domain owning this hardware interrupt or NULL for default domain
+ * @hwirq: hardware irq number in that domain space
+ *
+ * Create an irq mapping to linux irq space which is automatically disposed when
+ * the driver is detached.
+ * devm_irq_create_mapping() can be used to omit the explicit
+ * irq_dispose_mapping() call when driver is detached.
+ *
+ * Returns a linux irq number on success, 0 if mapping could not be created, or
+ * a negative error number if devm action could not be added.
+ */
+static inline int devm_irq_create_mapping(struct device *dev,
+ struct irq_domain *domain,
+ irq_hw_number_t hwirq)
+{
+ unsigned int virq = irq_create_mapping(domain, hwirq);
+
+ if (!virq)
+ return 0;
+
+ /*
+ * irq_dispose_mapping() is an empty function if CONFIG_IRQ_DOMAIN is
+ * disabled. No need to register an action in that case.
+ */
+ if (IS_ENABLED(CONFIG_IRQ_DOMAIN)) {
+ int err;
+
+ err = devm_add_action_or_reset(dev, devm_irq_mapping_drop,
+ (void *)(unsigned long)virq);
+ if (err)
+ return err;
+ }
+
+ return virq;
+}
+
#endif
--
2.43.2
Add support for true random number generator provided by the MCU.
New Omnia boards come without the Atmel SHA204-A chip. Instead the
crypto functionality is provided by new microcontroller, which has
a TRNG peripheral.
Signed-off-by: Marek Behún <[email protected]>
---
drivers/platform/cznic/Kconfig | 2 +
drivers/platform/cznic/Makefile | 1 +
.../platform/cznic/turris-omnia-mcu-base.c | 6 +-
.../platform/cznic/turris-omnia-mcu-gpio.c | 2 +-
.../platform/cznic/turris-omnia-mcu-trng.c | 89 +++++++++++++++++++
drivers/platform/cznic/turris-omnia-mcu.h | 8 ++
6 files changed, 106 insertions(+), 2 deletions(-)
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-trng.c
diff --git a/drivers/platform/cznic/Kconfig b/drivers/platform/cznic/Kconfig
index e2649cdecc38..750d5f47dba8 100644
--- a/drivers/platform/cznic/Kconfig
+++ b/drivers/platform/cznic/Kconfig
@@ -19,6 +19,7 @@ config TURRIS_OMNIA_MCU
depends on I2C
select GPIOLIB
select GPIOLIB_IRQCHIP
+ select HW_RANDOM
select RTC_CLASS
select WATCHDOG_CORE
help
@@ -28,6 +29,7 @@ config TURRIS_OMNIA_MCU
- board poweroff into true low power mode (with voltage regulators
disabled) and the ability to configure wake up from this mode (via
rtcwake)
+ - true random number generator (if available on the MCU)
- MCU watchdog
- GPIO pins
- to get front button press events (the front button can be
diff --git a/drivers/platform/cznic/Makefile b/drivers/platform/cznic/Makefile
index a43997a12d74..8fd4c6cbcb1b 100644
--- a/drivers/platform/cznic/Makefile
+++ b/drivers/platform/cznic/Makefile
@@ -4,4 +4,5 @@ obj-$(CONFIG_TURRIS_OMNIA_MCU) += turris-omnia-mcu.o
turris-omnia-mcu-objs := turris-omnia-mcu-base.o
turris-omnia-mcu-objs += turris-omnia-mcu-gpio.o
turris-omnia-mcu-objs += turris-omnia-mcu-sys-off-wakeup.o
+turris-omnia-mcu-objs += turris-omnia-mcu-trng.o
turris-omnia-mcu-objs += turris-omnia-mcu-watchdog.o
diff --git a/drivers/platform/cznic/turris-omnia-mcu-base.c b/drivers/platform/cznic/turris-omnia-mcu-base.c
index 5a45834003cd..30771004a627 100644
--- a/drivers/platform/cznic/turris-omnia-mcu-base.c
+++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
@@ -335,7 +335,11 @@ static int omnia_mcu_probe(struct i2c_client *client)
if (err)
return err;
- return omnia_mcu_register_gpiochip(mcu);
+ err = omnia_mcu_register_gpiochip(mcu);
+ if (err)
+ return err;
+
+ return omnia_mcu_register_trng(mcu);
}
static const struct of_device_id of_omnia_mcu_match[] = {
diff --git a/drivers/platform/cznic/turris-omnia-mcu-gpio.c b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
index 7f885be23a47..90b2caa679ea 100644
--- a/drivers/platform/cznic/turris-omnia-mcu-gpio.c
+++ b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
@@ -161,7 +161,7 @@ static const struct omnia_gpio {
};
/* mapping from interrupts to indexes of GPIOs in the omnia_gpios array */
-static const u8 omnia_int_to_gpio_idx[32] = {
+const u8 omnia_int_to_gpio_idx[32] = {
[__bf_shf(INT_CARD_DET)] = 4,
[__bf_shf(INT_MSATA_IND)] = 5,
[__bf_shf(INT_USB30_OVC)] = 6,
diff --git a/drivers/platform/cznic/turris-omnia-mcu-trng.c b/drivers/platform/cznic/turris-omnia-mcu-trng.c
new file mode 100644
index 000000000000..b08111b5c337
--- /dev/null
+++ b/drivers/platform/cznic/turris-omnia-mcu-trng.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CZ.NIC's Turris Omnia MCU TRNG driver
+ *
+ * 2024 by Marek Behún <[email protected]>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/completion.h>
+#include <linux/devm-helpers.h>
+#include <linux/irqdomain.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/turris-omnia-mcu-interface.h>
+#include <linux/types.h>
+
+#include "turris-omnia-mcu.h"
+
+#define CMD_TRNG_MAX_ENTROPY_LEN 64
+
+static irqreturn_t omnia_trng_irq_handler(int irq, void *dev_id)
+{
+ struct omnia_mcu *mcu = dev_id;
+
+ complete(&mcu->trng_completion);
+
+ return IRQ_HANDLED;
+}
+
+static int omnia_trng_read(struct hwrng *rng, void *data, size_t max, bool wait)
+{
+ struct omnia_mcu *mcu = (struct omnia_mcu *)rng->priv;
+ u8 reply[1 + CMD_TRNG_MAX_ENTROPY_LEN];
+ int err, bytes;
+
+ if (!wait && !completion_done(&mcu->trng_completion))
+ return 0;
+
+ do {
+ if (wait_for_completion_interruptible(&mcu->trng_completion))
+ return -EINTR;
+
+ err = omnia_cmd_read(mcu->client, CMD_TRNG_COLLECT_ENTROPY,
+ reply, sizeof(reply));
+ if (err)
+ return err;
+
+ bytes = min3(reply[0], max, CMD_TRNG_MAX_ENTROPY_LEN);
+ } while (wait && !bytes);
+
+ memcpy(data, &reply[1], bytes);
+
+ return bytes;
+}
+
+int omnia_mcu_register_trng(struct omnia_mcu *mcu)
+{
+ struct device *dev = &mcu->client->dev;
+ int irq, err;
+ u8 irq_idx;
+
+ if (!(mcu->features & FEAT_TRNG))
+ return 0;
+
+ irq_idx = omnia_int_to_gpio_idx[__bf_shf(INT_TRNG)];
+ irq = devm_irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
+ if (irq <= 0)
+ return dev_err_probe(dev, irq ?: -ENXIO,
+ "Cannot map TRNG IRQ\n");
+
+ init_completion(&mcu->trng_completion);
+
+ err = devm_request_threaded_irq(dev, irq, NULL, omnia_trng_irq_handler,
+ IRQF_ONESHOT, "turris-omnia-mcu-trng",
+ mcu);
+ if (err)
+ return dev_err_probe(dev, err, "Cannot request TRNG IRQ\n");
+
+ mcu->trng.name = "turris-omnia-mcu-trng";
+ mcu->trng.read = omnia_trng_read;
+ mcu->trng.priv = (unsigned long)mcu;
+
+ err = devm_hwrng_register(dev, &mcu->trng);
+ if (err)
+ return dev_err_probe(dev, err, "Cannot register TRNG\n");
+
+ return 0;
+}
diff --git a/drivers/platform/cznic/turris-omnia-mcu.h b/drivers/platform/cznic/turris-omnia-mcu.h
index 3e2c96079e64..f5b8f7ed3e6e 100644
--- a/drivers/platform/cznic/turris-omnia-mcu.h
+++ b/drivers/platform/cznic/turris-omnia-mcu.h
@@ -9,7 +9,9 @@
#define __TURRIS_OMNIA_MCU_H
#include <linux/bitops.h>
+#include <linux/completion.h>
#include <linux/gpio/driver.h>
+#include <linux/hw_random.h>
#include <linux/i2c.h>
#include <linux/if_ether.h>
#include <linux/mutex.h>
@@ -46,6 +48,10 @@ struct omnia_mcu {
/* MCU watchdog */
struct watchdog_device wdt;
+
+ /* true random number generator */
+ struct hwrng trng;
+ struct completion trng_completion;
};
static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,
@@ -166,11 +172,13 @@ static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
return err ?: reply;
}
+extern const u8 omnia_int_to_gpio_idx[32];
extern const struct attribute_group omnia_mcu_gpio_group;
extern const struct attribute_group omnia_mcu_poweroff_group;
int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu);
int omnia_mcu_register_sys_off_and_wakeup(struct omnia_mcu *mcu);
+int omnia_mcu_register_trng(struct omnia_mcu *mcu);
int omnia_mcu_register_watchdog(struct omnia_mcu *mcu);
#endif /* __TURRIS_OMNIA_MCU_H */
--
2.43.2
A few drivers register a devm action to remove a debugfs directory,
implementing a one-liner function that calls debufs_remove_recursive().
Help drivers avoid this repeated implementations by adding managed
version of debugfs directory create function.
Use the new function devm_debugfs_create_dir() in the following
drivers:
drivers/crypto/caam/ctrl.c
drivers/gpu/drm/bridge/ti-sn65dsi86.c
drivers/hwmon/hp-wmi-sensors.c
drivers/hwmon/mr75203.c
drivers/hwmon/pmbus/pmbus_core.c
Also use the action function devm_debugfs_dir_recursive_drop() in
driver
drivers/gpio/gpio-mockup.c
As per Dan Williams' request [1], do not touch the driver
drivers/cxl/mem.c
[1] https://lore.kernel.org/linux-gpio/65d7918b358a5_1ee3129432@dwillia2-mobl3.amr.corp.intel.com.notmuch/
Signed-off-by: Marek Behún <[email protected]>
---
drivers/crypto/caam/ctrl.c | 16 +++--------
drivers/gpio/gpio-mockup.c | 11 ++------
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 13 ++-------
drivers/hwmon/hp-wmi-sensors.c | 15 ++--------
drivers/hwmon/mr75203.c | 15 ++++------
drivers/hwmon/pmbus/pmbus_core.c | 16 ++++-------
include/linux/devm-helpers.h | 40 +++++++++++++++++++++++++++
7 files changed, 61 insertions(+), 65 deletions(-)
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index bdf367f3f679..ea3ed9a17f1a 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -7,6 +7,7 @@
*/
#include <linux/device.h>
+#include <linux/devm-helpers.h>
#include <linux/of_address.h>
#include <linux/of_irq.h>
#include <linux/platform_device.h>
@@ -604,11 +605,6 @@ static int init_clocks(struct device *dev, const struct caam_imx_data *data)
return devm_add_action_or_reset(dev, disable_clocks, ctrlpriv);
}
-static void caam_remove_debugfs(void *root)
-{
- debugfs_remove_recursive(root);
-}
-
#ifdef CONFIG_FSL_MC_BUS
static bool check_version(struct fsl_mc_version *mc_version, u32 major,
u32 minor, u32 revision)
@@ -1058,13 +1054,9 @@ static int caam_probe(struct platform_device *pdev)
ctrlpriv->era = caam_get_era(perfmon);
ctrlpriv->domain = iommu_get_domain_for_dev(dev);
- dfs_root = debugfs_create_dir(dev_name(dev), NULL);
- if (IS_ENABLED(CONFIG_DEBUG_FS)) {
- ret = devm_add_action_or_reset(dev, caam_remove_debugfs,
- dfs_root);
- if (ret)
- return ret;
- }
+ dfs_root = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
+ if (IS_ERR(dfs_root))
+ return PTR_ERR(dfs_root);
caam_debugfs_init(ctrlpriv, perfmon, dfs_root);
diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 455eecf6380e..adbe0fe09490 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -12,6 +12,7 @@
#include <linux/cleanup.h>
#include <linux/debugfs.h>
#include <linux/device.h>
+#include <linux/devm-helpers.h>
#include <linux/gpio/driver.h>
#include <linux/interrupt.h>
#include <linux/irq.h>
@@ -390,13 +391,6 @@ static void gpio_mockup_debugfs_setup(struct device *dev,
}
}
-static void gpio_mockup_debugfs_cleanup(void *data)
-{
- struct gpio_mockup_chip *chip = data;
-
- debugfs_remove_recursive(chip->dbg_dir);
-}
-
static void gpio_mockup_dispose_mappings(void *data)
{
struct gpio_mockup_chip *chip = data;
@@ -480,7 +474,8 @@ static int gpio_mockup_probe(struct platform_device *pdev)
gpio_mockup_debugfs_setup(dev, chip);
- return devm_add_action_or_reset(dev, gpio_mockup_debugfs_cleanup, chip);
+ return devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop,
+ chip->dbg_dir);
}
static const struct of_device_id gpio_mockup_of_match[] = {
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 84698a0b27a8..85987350f108 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -10,6 +10,7 @@
#include <linux/bits.h>
#include <linux/clk.h>
#include <linux/debugfs.h>
+#include <linux/devm-helpers.h>
#include <linux/gpio/consumer.h>
#include <linux/gpio/driver.h>
#include <linux/i2c.h>
@@ -427,18 +428,12 @@ static int status_show(struct seq_file *s, void *data)
DEFINE_SHOW_ATTRIBUTE(status);
-static void ti_sn65dsi86_debugfs_remove(void *data)
-{
- debugfs_remove_recursive(data);
-}
-
static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata)
{
struct device *dev = pdata->dev;
struct dentry *debugfs;
- int ret;
- debugfs = debugfs_create_dir(dev_name(dev), NULL);
+ debugfs = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
/*
* We might get an error back if debugfs wasn't enabled in the kernel
@@ -447,10 +442,6 @@ static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata)
if (IS_ERR_OR_NULL(debugfs))
return;
- ret = devm_add_action_or_reset(dev, ti_sn65dsi86_debugfs_remove, debugfs);
- if (ret)
- return;
-
debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
}
diff --git a/drivers/hwmon/hp-wmi-sensors.c b/drivers/hwmon/hp-wmi-sensors.c
index b5325d0e72b9..2a7c33763ce8 100644
--- a/drivers/hwmon/hp-wmi-sensors.c
+++ b/drivers/hwmon/hp-wmi-sensors.c
@@ -23,6 +23,7 @@
#include <linux/acpi.h>
#include <linux/debugfs.h>
+#include <linux/devm-helpers.h>
#include <linux/hwmon.h>
#include <linux/jiffies.h>
#include <linux/mutex.h>
@@ -1304,12 +1305,6 @@ static int current_reading_show(struct seq_file *seqf, void *ignored)
}
DEFINE_SHOW_ATTRIBUTE(current_reading);
-/* hp_wmi_devm_debugfs_remove - devm callback for debugfs cleanup */
-static void hp_wmi_devm_debugfs_remove(void *res)
-{
- debugfs_remove_recursive(res);
-}
-
/* hp_wmi_debugfs_init - create and populate debugfs directory tree */
static void hp_wmi_debugfs_init(struct device *dev, struct hp_wmi_info *info,
struct hp_wmi_platform_events *pevents,
@@ -1320,21 +1315,15 @@ static void hp_wmi_debugfs_init(struct device *dev, struct hp_wmi_info *info,
struct dentry *debugfs;
struct dentry *entries;
struct dentry *dir;
- int err;
u8 i;
/* dev_name() gives a not-very-friendly GUID for WMI devices. */
scnprintf(buf, sizeof(buf), "hp-wmi-sensors-%u", dev->id);
- debugfs = debugfs_create_dir(buf, NULL);
+ debugfs = devm_debugfs_create_dir(dev, buf, NULL);
if (IS_ERR(debugfs))
return;
- err = devm_add_action_or_reset(dev, hp_wmi_devm_debugfs_remove,
- debugfs);
- if (err)
- return;
-
entries = debugfs_create_dir("sensor", debugfs);
for (i = 0; i < icount; i++, info++) {
diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 50a8b9c3f94d..50f348fca108 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -10,6 +10,7 @@
#include <linux/bits.h>
#include <linux/clk.h>
#include <linux/debugfs.h>
+#include <linux/devm-helpers.h>
#include <linux/hwmon.h>
#include <linux/kstrtox.h>
#include <linux/module.h>
@@ -216,17 +217,11 @@ static const struct file_operations pvt_ts_coeff_j_fops = {
.llseek = default_llseek,
};
-static void devm_pvt_ts_dbgfs_remove(void *data)
-{
- struct pvt_device *pvt = (struct pvt_device *)data;
-
- debugfs_remove_recursive(pvt->dbgfs_dir);
- pvt->dbgfs_dir = NULL;
-}
-
static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
{
- pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
+ pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
+ if (IS_ERR(pvt->dbgfs_dir))
+ return PTR_ERR(pvt->dbgfs_dir);
debugfs_create_u32("ts_coeff_h", 0644, pvt->dbgfs_dir,
&pvt->ts_coeff.h);
@@ -237,7 +232,7 @@ static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
debugfs_create_file("ts_coeff_j", 0644, pvt->dbgfs_dir, pvt,
&pvt_ts_coeff_j_fops);
- return devm_add_action_or_reset(dev, devm_pvt_ts_dbgfs_remove, pvt);
+ return 0;
}
static umode_t pvt_is_visible(const void *data, enum hwmon_sensor_types type,
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index cb4c65a7f288..88d27bb3b69a 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -7,6 +7,7 @@
*/
#include <linux/debugfs.h>
+#include <linux/devm-helpers.h>
#include <linux/kernel.h>
#include <linux/math64.h>
#include <linux/module.h>
@@ -3336,13 +3337,6 @@ static const struct file_operations pmbus_debugfs_ops_mfr = {
.open = simple_open,
};
-static void pmbus_remove_debugfs(void *data)
-{
- struct dentry *entry = data;
-
- debugfs_remove_recursive(entry);
-}
-
static int pmbus_init_debugfs(struct i2c_client *client,
struct pmbus_data *data)
{
@@ -3357,8 +3351,9 @@ static int pmbus_init_debugfs(struct i2c_client *client,
* Create the debugfs directory for this device. Use the hwmon device
* name to avoid conflicts (hwmon numbers are globally unique).
*/
- data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
- pmbus_debugfs_dir);
+ data->debugfs = devm_debugfs_create_dir(data->dev,
+ dev_name(data->hwmon_dev),
+ pmbus_debugfs_dir);
if (IS_ERR_OR_NULL(data->debugfs)) {
data->debugfs = NULL;
return -ENODEV;
@@ -3542,8 +3537,7 @@ static int pmbus_init_debugfs(struct i2c_client *client,
}
}
- return devm_add_action_or_reset(data->dev,
- pmbus_remove_debugfs, data->debugfs);
+ return 0;
}
#else
static int pmbus_init_debugfs(struct i2c_client *client,
diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
index 3805551fd433..feefe152c752 100644
--- a/include/linux/devm-helpers.h
+++ b/include/linux/devm-helpers.h
@@ -23,6 +23,7 @@
* already ran.
*/
+#include <linux/debugfs.h>
#include <linux/device.h>
#include <linux/kconfig.h>
#include <linux/irqdomain.h>
@@ -130,4 +131,43 @@ static inline int devm_irq_create_mapping(struct device *dev,
return virq;
}
+static inline void devm_debugfs_dir_recursive_drop(void *res)
+{
+ debugfs_remove_recursive(res);
+}
+
+/**
+ * devm_debugfs_create_dir - Resource managed debugfs directory creation
+ * @dev: Device which lifetime the directory is bound to
+ * @name: a pointer to a string containing the name of the directory to
+ * create
+ * @parent: a pointer to the parent dentry for this file. This should be a
+ * directory dentry if set. If this parameter is NULL, then the
+ * directory will be created in the root of the debugfs filesystem.
+ *
+ * Create a debugfs directory which is automatically recursively removed when
+ * the driver is detached. A few drivers create debugfs directories which they
+ * want removed before driver is detached.
+ * devm_debugfs_create_dir() can be used to omit the explicit
+ * debugfs_remove_recursive() call when driver is detached.
+ */
+static inline struct dentry *
+devm_debugfs_create_dir(struct device *dev, const char *name,
+ struct dentry *parent)
+{
+ struct dentry *dentry;
+ int err;
+
+ dentry = debugfs_create_dir(name, parent);
+ if (IS_ERR(dentry))
+ return dentry;
+
+ err = devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop,
+ dentry);
+ if (err < 0)
+ return ERR_PTR(err);
+
+ return dentry;
+}
+
#endif
--
2.43.2
Add support for digital message signing with private key stored in the
MCU. Boards with MKL MCUs have a NIST256p ECDSA private key created
when manufactured. The private key is not readable from the MCU, but
MCU allows for signing messages with it and retrieving the public key.
As described in a similar commit 50524d787de3 ("firmware:
turris-mox-rwtm: support ECDSA signatures via debugfs"):
The optimal solution would be to register an akcipher provider via
kernel's crypto API, but crypto API does not yet support accessing
akcipher API from userspace (and probably won't for some time, see
https://www.spinics.net/lists/linux-crypto/msg38388.html).
Therefore we add support for accessing this signature generation
mechanism via debugfs for now, so that userspace can access it.
Signed-off-by: Marek Behún <[email protected]>
---
.../ABI/testing/debugfs-turris-omnia-mcu | 13 ++
.../sysfs-bus-i2c-devices-turris-omnia-mcu | 13 ++
MAINTAINERS | 1 +
drivers/platform/cznic/Kconfig | 2 +
drivers/platform/cznic/Makefile | 12 +-
.../platform/cznic/turris-omnia-mcu-base.c | 45 +++-
.../platform/cznic/turris-omnia-mcu-debugfs.c | 216 ++++++++++++++++++
drivers/platform/cznic/turris-omnia-mcu.h | 40 +++-
8 files changed, 331 insertions(+), 11 deletions(-)
create mode 100644 Documentation/ABI/testing/debugfs-turris-omnia-mcu
create mode 100644 drivers/platform/cznic/turris-omnia-mcu-debugfs.c
diff --git a/Documentation/ABI/testing/debugfs-turris-omnia-mcu b/Documentation/ABI/testing/debugfs-turris-omnia-mcu
new file mode 100644
index 000000000000..1665005c2dcd
--- /dev/null
+++ b/Documentation/ABI/testing/debugfs-turris-omnia-mcu
@@ -0,0 +1,13 @@
+What: /sys/kernel/debug/turris-omnia-mcu/do_sign
+Date: July 2024
+KernelVersion: 6.10
+Contact: Marek Behún <[email protected]>
+Description:
+
+ ======= ===========================================================
+ (Write) Message to sign with the ECDSA private key stored in MCU.
+ The message must be exactly 32 bytes long (since this is
+ intended to be a SHA-256 hash).
+ (Read) The resulting signature, 64 bytes. This contains the R and
+ S values of the ECDSA signature, both in big-endian format.
+ ======= ===========================================================
diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
index 564119849388..b239224cf9bc 100644
--- a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
+++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
@@ -90,6 +90,19 @@ Description: (RO) Contains the microcontroller type (STM32, GD32, MKL).
Format: %s.
+What: /sys/bus/i2c/devices/<mcu_device>/public_key
+Date: July 2024
+KernelVersion: 6.10
+Contact: Marek Behún <[email protected]>
+Description: (RO) Contains board ECDSA public key.
+
+ Only available if MCU supports signing messages with the ECDSA
+ algorithm. If so, the board has a private key stored in the MCU
+ that was generated during manufacture and cannot be retrieved
+ from the MCU.
+
+ Format: %s.
+
What: /sys/bus/i2c/devices/<mcu_device>/reset_selector
Date: July 2024
KernelVersion: 6.10
diff --git a/MAINTAINERS b/MAINTAINERS
index 529e16d386e8..3706a4ec818e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2139,6 +2139,7 @@ M: Marek Behún <[email protected]>
S: Maintained
W: https://www.turris.cz/
F: Documentation/ABI/testing/debugfs-moxtet
+F: Documentation/ABI/testing/debugfs-turris-omnia-mcu
F: Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
F: Documentation/ABI/testing/sysfs-bus-moxtet-devices
F: Documentation/ABI/testing/sysfs-firmware-turris-mox-rwtm
diff --git a/drivers/platform/cznic/Kconfig b/drivers/platform/cznic/Kconfig
index 750d5f47dba8..2d45495c0a2d 100644
--- a/drivers/platform/cznic/Kconfig
+++ b/drivers/platform/cznic/Kconfig
@@ -30,6 +30,8 @@ config TURRIS_OMNIA_MCU
disabled) and the ability to configure wake up from this mode (via
rtcwake)
- true random number generator (if available on the MCU)
+ - ECDSA message signing with board private key (if available on the
+ MCU)
- MCU watchdog
- GPIO pins
- to get front button press events (the front button can be
diff --git a/drivers/platform/cznic/Makefile b/drivers/platform/cznic/Makefile
index 8fd4c6cbcb1b..22ac5075b750 100644
--- a/drivers/platform/cznic/Makefile
+++ b/drivers/platform/cznic/Makefile
@@ -1,8 +1,10 @@
# SPDX-License-Identifier: GPL-2.0-only
obj-$(CONFIG_TURRIS_OMNIA_MCU) += turris-omnia-mcu.o
-turris-omnia-mcu-objs := turris-omnia-mcu-base.o
-turris-omnia-mcu-objs += turris-omnia-mcu-gpio.o
-turris-omnia-mcu-objs += turris-omnia-mcu-sys-off-wakeup.o
-turris-omnia-mcu-objs += turris-omnia-mcu-trng.o
-turris-omnia-mcu-objs += turris-omnia-mcu-watchdog.o
+turris-omnia-mcu-objs-y := turris-omnia-mcu-base.o
+turris-omnia-mcu-objs-y += turris-omnia-mcu-gpio.o
+turris-omnia-mcu-objs-y += turris-omnia-mcu-sys-off-wakeup.o
+turris-omnia-mcu-objs-y += turris-omnia-mcu-trng.o
+turris-omnia-mcu-objs-y += turris-omnia-mcu-watchdog.o
+turris-omnia-mcu-objs-$(CONFIG_DEBUG_FS) += turris-omnia-mcu-debugfs.o
+turris-omnia-mcu-objs := $(turris-omnia-mcu-objs-y)
diff --git a/drivers/platform/cznic/turris-omnia-mcu-base.c b/drivers/platform/cznic/turris-omnia-mcu-base.c
index 30771004a627..0113d6c876c6 100644
--- a/drivers/platform/cznic/turris-omnia-mcu-base.c
+++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
@@ -125,6 +125,16 @@ static ssize_t board_revision_show(struct device *dev,
}
static DEVICE_ATTR_RO(board_revision);
+static ssize_t public_key_show(struct device *dev, struct device_attribute *a,
+ char *buf)
+{
+ struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));
+
+ return sysfs_emit(buf, "%*phN\n", OMNIA_MCU_CRYPTO_PUBLIC_KEY_LEN,
+ mcu->board_public_key);
+}
+static DEVICE_ATTR_RO(public_key);
+
static struct attribute *omnia_mcu_base_attrs[] = {
&dev_attr_fw_version_hash_application.attr,
&dev_attr_fw_version_hash_bootloader.attr,
@@ -134,6 +144,7 @@ static struct attribute *omnia_mcu_base_attrs[] = {
&dev_attr_serial_number.attr,
&dev_attr_first_mac_address.attr,
&dev_attr_board_revision.attr,
+ &dev_attr_public_key.attr,
NULL
};
@@ -149,6 +160,9 @@ static umode_t omnia_mcu_base_attrs_visible(struct kobject *kobj,
a == &dev_attr_board_revision.attr) &&
!(mcu->features & FEAT_BOARD_INFO))
mode = 0;
+ else if (a == &dev_attr_public_key.attr &&
+ !(mcu->features & FEAT_CRYPTO))
+ mode = 0;
return mode;
}
@@ -299,6 +313,24 @@ static int omnia_mcu_read_board_info(struct omnia_mcu *mcu)
return 0;
}
+static int omnia_mcu_read_public_key(struct omnia_mcu *mcu)
+{
+ u8 reply[1 + OMNIA_MCU_CRYPTO_PUBLIC_KEY_LEN];
+ int err;
+
+ err = omnia_cmd_read(mcu->client, CMD_CRYPTO_GET_PUBLIC_KEY, reply,
+ sizeof(reply));
+ if (err)
+ return err;
+
+ if (reply[0] != OMNIA_MCU_CRYPTO_PUBLIC_KEY_LEN)
+ return -EIO;
+
+ memcpy(mcu->board_public_key, &reply[1], sizeof(mcu->board_public_key));
+
+ return 0;
+}
+
static int omnia_mcu_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
@@ -327,6 +359,13 @@ static int omnia_mcu_probe(struct i2c_client *client)
"Cannot read board info\n");
}
+ if (mcu->features & FEAT_CRYPTO) {
+ err = omnia_mcu_read_public_key(mcu);
+ if (err)
+ return dev_err_probe(dev, err,
+ "Cannot read board public key\n");
+ }
+
err = omnia_mcu_register_sys_off_and_wakeup(mcu);
if (err)
return err;
@@ -339,7 +378,11 @@ static int omnia_mcu_probe(struct i2c_client *client)
if (err)
return err;
- return omnia_mcu_register_trng(mcu);
+ err = omnia_mcu_register_trng(mcu);
+ if (err)
+ return err;
+
+ return omnia_mcu_register_debugfs(mcu);
}
static const struct of_device_id of_omnia_mcu_match[] = {
diff --git a/drivers/platform/cznic/turris-omnia-mcu-debugfs.c b/drivers/platform/cznic/turris-omnia-mcu-debugfs.c
new file mode 100644
index 000000000000..6c4baaf26b2c
--- /dev/null
+++ b/drivers/platform/cznic/turris-omnia-mcu-debugfs.c
@@ -0,0 +1,216 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CZ.NIC's Turris Omnia MCU ECDSA message signing via debugfs
+ *
+ * 2024 by Marek Behún <[email protected]>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/completion.h>
+#include <linux/debugfs.h>
+#include <linux/devm-helpers.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/string.h>
+#include <linux/turris-omnia-mcu-interface.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+
+#include "turris-omnia-mcu.h"
+
+#define CMD_CRYPTO_SIGN_MESSAGE_LEN 32
+
+enum {
+ SIGN_STATE_CLOSED = 0,
+ SIGN_STATE_OPEN = 1,
+ SIGN_STATE_REQUESTED = 2,
+ SIGN_STATE_COLLECTED = 3,
+};
+
+static irqreturn_t omnia_msg_signed_irq_handler(int irq, void *dev_id)
+{
+ u8 reply[1 + OMNIA_MCU_CRYPTO_SIGNATURE_LEN];
+ struct omnia_mcu *mcu = dev_id;
+ int err;
+
+ err = omnia_cmd_read(mcu->client, CMD_CRYPTO_COLLECT_SIGNATURE, reply,
+ sizeof(reply));
+ if (!err && reply[0] != OMNIA_MCU_CRYPTO_SIGNATURE_LEN)
+ err = -EIO;
+
+ guard(mutex)(&mcu->sign_lock);
+
+ if (mcu->sign_state == SIGN_STATE_REQUESTED) {
+ mcu->sign_err = err;
+ if (!err)
+ memcpy(mcu->signature, &reply[1],
+ OMNIA_MCU_CRYPTO_SIGNATURE_LEN);
+ mcu->sign_state = SIGN_STATE_COLLECTED;
+ complete(&mcu->msg_signed_completion);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int do_sign_open(struct inode *inode, struct file *file)
+{
+ struct omnia_mcu *mcu = inode->i_private;
+
+ guard(mutex)(&mcu->sign_lock);
+
+ /* do_sign is allowed to be opened only once */
+ if (mcu->sign_state != SIGN_STATE_CLOSED)
+ return -EBUSY;
+
+ mcu->sign_state = SIGN_STATE_OPEN;
+
+ file->private_data = mcu;
+
+ return nonseekable_open(inode, file);
+}
+
+static int do_sign_release(struct inode *inode, struct file *file)
+{
+ struct omnia_mcu *mcu = file->private_data;
+
+ guard(mutex)(&mcu->sign_lock);
+
+ mcu->sign_state = SIGN_STATE_CLOSED;
+
+ /* forget signature on release even if it was not read, for security */
+ memzero_explicit(mcu->signature, sizeof(mcu->signature));
+
+ return 0;
+}
+
+static ssize_t do_sign_read(struct file *file, char __user *buf, size_t len,
+ loff_t *ppos)
+{
+ struct omnia_mcu *mcu = file->private_data;
+
+ /* only allow read of one whole signature */
+ if (len != sizeof(mcu->signature))
+ return -EINVAL;
+
+ scoped_guard(mutex, &mcu->sign_lock)
+ if (mcu->sign_state != SIGN_STATE_REQUESTED &&
+ mcu->sign_state != SIGN_STATE_COLLECTED)
+ return -ENODATA;
+
+ if (wait_for_completion_interruptible(&mcu->msg_signed_completion))
+ return -EINTR;
+
+ guard(mutex)(&mcu->sign_lock);
+
+ mcu->sign_state = SIGN_STATE_OPEN;
+
+ if (mcu->sign_err)
+ return mcu->sign_err;
+
+ if (copy_to_user(buf, mcu->signature, len))
+ return -EFAULT;
+
+ /* on read forget the signature, for security */
+ memzero_explicit(mcu->signature, sizeof(mcu->signature));
+
+ return len;
+}
+
+static ssize_t do_sign_write(struct file *file, const char __user *buf,
+ size_t len, loff_t *ppos)
+{
+ u8 cmd[1 + CMD_CRYPTO_SIGN_MESSAGE_LEN], reply;
+ struct omnia_mcu *mcu = file->private_data;
+ int err;
+
+ /*
+ * the input is a SHA-256 hash of a message to sign, so exactly
+ * 32 bytes have to be read
+ */
+ if (len != CMD_CRYPTO_SIGN_MESSAGE_LEN)
+ return -EINVAL;
+
+ cmd[0] = CMD_CRYPTO_SIGN_MESSAGE;
+
+ if (copy_from_user(&cmd[1], buf, len))
+ return -EFAULT;
+
+ guard(mutex)(&mcu->sign_lock);
+
+ if (mcu->sign_state != SIGN_STATE_OPEN)
+ return -EBUSY;
+
+ err = omnia_cmd_write_read(mcu->client, cmd, sizeof(cmd), &reply, 1);
+ if (err)
+ return err;
+
+ if (reply)
+ mcu->sign_state = SIGN_STATE_REQUESTED;
+
+ return reply ? len : -EBUSY;
+}
+
+static const struct file_operations do_sign_fops = {
+ .owner = THIS_MODULE,
+ .open = do_sign_open,
+ .read = do_sign_read,
+ .write = do_sign_write,
+ .release = do_sign_release,
+ .llseek = no_llseek,
+};
+
+/* this will go away once we have devm_mutex_init() */
+static void omnia_mcu_mutex_destroy(void *lock)
+{
+ mutex_destroy(lock);
+}
+
+int omnia_mcu_register_debugfs(struct omnia_mcu *mcu)
+{
+ struct device *dev = &mcu->client->dev;
+ struct dentry *root, *entry;
+ int irq, err;
+ u8 irq_idx;
+
+ if (!(mcu->features & FEAT_CRYPTO))
+ return 0;
+
+ irq_idx = omnia_int_to_gpio_idx[__bf_shf(INT_MESSAGE_SIGNED)];
+ irq = devm_irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
+ if (irq <= 0)
+ return dev_err_probe(dev, irq ?: -ENXIO,
+ "Cannot map MESSAGE_SIGNED IRQ\n");
+
+ mutex_init(&mcu->sign_lock);
+ err = devm_add_action_or_reset(dev, omnia_mcu_mutex_destroy,
+ &mcu->sign_lock);
+ if (err)
+ return err;
+
+ mcu->sign_state = 0;
+
+ init_completion(&mcu->msg_signed_completion);
+
+ err = devm_request_threaded_irq(dev, irq, NULL,
+ omnia_msg_signed_irq_handler,
+ IRQF_ONESHOT,
+ "turris-omnia-mcu-debugfs", mcu);
+ if (err)
+ return dev_err_probe(dev, err,
+ "Cannot request MESSAGE_SIGNED IRQ\n");
+
+ root = devm_debugfs_create_dir(dev, "turris-omnia-mcu", NULL);
+ if (IS_ERR(root))
+ return dev_err_probe(dev, PTR_ERR(root),
+ "Cannot create debugfs dir\n");
+
+
+ entry = debugfs_create_file_unsafe("do_sign", 0600, root, mcu,
+ &do_sign_fops);
+ if (IS_ERR(entry))
+ return dev_err_probe(dev, PTR_ERR(entry),
+ "Cannot create debugfs entry\n");
+
+ return 0;
+}
diff --git a/drivers/platform/cznic/turris-omnia-mcu.h b/drivers/platform/cznic/turris-omnia-mcu.h
index f5b8f7ed3e6e..b4f1f2e8f936 100644
--- a/drivers/platform/cznic/turris-omnia-mcu.h
+++ b/drivers/platform/cznic/turris-omnia-mcu.h
@@ -22,6 +22,9 @@
#include <asm/byteorder.h>
#include <asm/unaligned.h>
+#define OMNIA_MCU_CRYPTO_PUBLIC_KEY_LEN 33
+#define OMNIA_MCU_CRYPTO_SIGNATURE_LEN 64
+
struct omnia_mcu {
struct i2c_client *client;
const char *type;
@@ -31,6 +34,7 @@ struct omnia_mcu {
u64 board_serial_number;
u8 board_first_mac[ETH_ALEN];
u8 board_revision;
+ u8 board_public_key[OMNIA_MCU_CRYPTO_PUBLIC_KEY_LEN];
/* GPIO chip */
struct gpio_chip gc;
@@ -52,6 +56,16 @@ struct omnia_mcu {
/* true random number generator */
struct hwrng trng;
struct completion trng_completion;
+
+#ifdef CONFIG_DEBUG_FS
+ /* MCU ECDSA message signing via debugfs */
+ struct dentry *debugfs_root;
+ struct completion msg_signed_completion;
+ struct mutex sign_lock;
+ unsigned int sign_state;
+ u8 signature[OMNIA_MCU_CRYPTO_SIGNATURE_LEN];
+ int sign_err;
+#endif
};
static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,
@@ -94,19 +108,20 @@ static inline int omnia_cmd_write_u32(const struct i2c_client *client, u8 cmd,
return omnia_cmd_write(client, buf, sizeof(buf));
}
-static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd,
- void *reply, unsigned int len)
+static inline int omnia_cmd_write_read(const struct i2c_client *client,
+ void *cmd, unsigned int cmd_len,
+ void *reply, unsigned int reply_len)
{
struct i2c_msg msgs[2];
int ret;
msgs[0].addr = client->addr;
msgs[0].flags = 0;
- msgs[0].len = 1;
- msgs[0].buf = &cmd;
+ msgs[0].len = cmd_len;
+ msgs[0].buf = cmd;
msgs[1].addr = client->addr;
msgs[1].flags = I2C_M_RD;
- msgs[1].len = len;
+ msgs[1].len = reply_len;
msgs[1].buf = reply;
ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
@@ -118,6 +133,12 @@ static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd,
return 0;
}
+static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd,
+ void *reply, unsigned int len)
+{
+ return omnia_cmd_write_read(client, &cmd, 1, reply, len);
+}
+
/* Returns 0 on success */
static inline int omnia_cmd_read_bits(const struct i2c_client *client, u8 cmd,
u32 bits, u32 *dst)
@@ -181,4 +202,13 @@ int omnia_mcu_register_sys_off_and_wakeup(struct omnia_mcu *mcu);
int omnia_mcu_register_trng(struct omnia_mcu *mcu);
int omnia_mcu_register_watchdog(struct omnia_mcu *mcu);
+#ifdef CONFIG_DEBUG_FS
+int omnia_mcu_register_debugfs(struct omnia_mcu *mcu);
+#else
+static inline int omnia_mcu_register_debugfs(struct omnia_mcu *mcu)
+{
+ return 0;
+}
+#endif
+
#endif /* __TURRIS_OMNIA_MCU_H */
--
2.43.2
On 3/23/24 09:43, Marek Behún wrote:
> A few drivers register a devm action to remove a debugfs directory,
> implementing a one-liner function that calls debufs_remove_recursive().
> Help drivers avoid this repeated implementations by adding managed
> version of debugfs directory create function.
>
> Use the new function devm_debugfs_create_dir() in the following
> drivers:
> drivers/crypto/caam/ctrl.c
> drivers/gpu/drm/bridge/ti-sn65dsi86.c
> drivers/hwmon/hp-wmi-sensors.c
> drivers/hwmon/mr75203.c
> drivers/hwmon/pmbus/pmbus_core.c
>
Please split this up into multiple patches, at least separating out
the hwmon patches.
Guenter
Le 23/03/2024 à 17:43, Marek Behún a écrit :
> A few drivers register a devm action to remove a debugfs directory,
> implementing a one-liner function that calls debufs_remove_recursive().
> Help drivers avoid this repeated implementations by adding managed
> version of debugfs directory create function.
>
> Use the new function devm_debugfs_create_dir() in the following
> drivers:
> drivers/crypto/caam/ctrl.c
> drivers/gpu/drm/bridge/ti-sn65dsi86.c
> drivers/hwmon/hp-wmi-sensors.c
> drivers/hwmon/mr75203.c
> drivers/hwmon/pmbus/pmbus_core.c
>
> Also use the action function devm_debugfs_dir_recursive_drop() in
> driver
> drivers/gpio/gpio-mockup.c
>
> As per Dan Williams' request [1], do not touch the driver
> drivers/cxl/mem.c
>
> [1] https://lore.kernel.org/linux-gpio/65d7918b358a5_1ee3129432@dwillia2-mobl3.amr.corp.intel.com.notmuch/
>
> Signed-off-by: Marek Behún <[email protected]>
> ---
> drivers/crypto/caam/ctrl.c | 16 +++--------
> drivers/gpio/gpio-mockup.c | 11 ++------
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 13 ++-------
> drivers/hwmon/hp-wmi-sensors.c | 15 ++--------
> drivers/hwmon/mr75203.c | 15 ++++------
> drivers/hwmon/pmbus/pmbus_core.c | 16 ++++-------
> include/linux/devm-helpers.h | 40 +++++++++++++++++++++++++++
> 7 files changed, 61 insertions(+), 65 deletions(-)
...
> diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
> index 455eecf6380e..adbe0fe09490 100644
> --- a/drivers/gpio/gpio-mockup.c
> +++ b/drivers/gpio/gpio-mockup.c
> @@ -12,6 +12,7 @@
> #include <linux/cleanup.h>
> #include <linux/debugfs.h>
> #include <linux/device.h>
> +#include <linux/devm-helpers.h>
> #include <linux/gpio/driver.h>
> #include <linux/interrupt.h>
> #include <linux/irq.h>
> @@ -390,13 +391,6 @@ static void gpio_mockup_debugfs_setup(struct device *dev,
> }
> }
>
> -static void gpio_mockup_debugfs_cleanup(void *data)
> -{
> - struct gpio_mockup_chip *chip = data;
> -
> - debugfs_remove_recursive(chip->dbg_dir);
> -}
> -
> static void gpio_mockup_dispose_mappings(void *data)
> {
> struct gpio_mockup_chip *chip = data;
> @@ -480,7 +474,8 @@ static int gpio_mockup_probe(struct platform_device *pdev)
>
> gpio_mockup_debugfs_setup(dev, chip);
>
> - return devm_add_action_or_reset(dev, gpio_mockup_debugfs_cleanup, chip);
> + return devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop,
> + chip->dbg_dir);
This look strange. Shouldn't the debugfs_create_dir() call in
gpio_mockup_debugfs_setup() be changed, instead?
(I've not look in the previous version if something was said about it,
so apologies if already discussed)
> }
>
> static const struct of_device_id gpio_mockup_of_match[] = {
...
> diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
> index 50a8b9c3f94d..50f348fca108 100644
> --- a/drivers/hwmon/mr75203.c
> +++ b/drivers/hwmon/mr75203.c
> @@ -10,6 +10,7 @@
> #include <linux/bits.h>
> #include <linux/clk.h>
> #include <linux/debugfs.h>
> +#include <linux/devm-helpers.h>
> #include <linux/hwmon.h>
> #include <linux/kstrtox.h>
> #include <linux/module.h>
> @@ -216,17 +217,11 @@ static const struct file_operations pvt_ts_coeff_j_fops = {
> .llseek = default_llseek,
> };
>
> -static void devm_pvt_ts_dbgfs_remove(void *data)
> -{
> - struct pvt_device *pvt = (struct pvt_device *)data;
> -
> - debugfs_remove_recursive(pvt->dbgfs_dir);
> - pvt->dbgfs_dir = NULL;
> -}
> -
> static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
> {
> - pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
> + pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
> + if (IS_ERR(pvt->dbgfs_dir))
> + return PTR_ERR(pvt->dbgfs_dir);
Not sure if the test and error handling should be added here.
*If I'm correct*, functions related to debugfs already handle this case
and just do nothing. And failure in debugfs related code is not
considered as something that need to be reported and abort a probe function.
Maybe the same other (already existing) tests in this patch should be
removed as well, in a separated patch.
just my 2c
CJ
>
> debugfs_create_u32("ts_coeff_h", 0644, pvt->dbgfs_dir,
> &pvt->ts_coeff.h);
> @@ -237,7 +232,7 @@ static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
> debugfs_create_file("ts_coeff_j", 0644, pvt->dbgfs_dir, pvt,
> &pvt_ts_coeff_j_fops);
>
> - return devm_add_action_or_reset(dev, devm_pvt_ts_dbgfs_remove, pvt);
> + return 0;
> }
...
On Sat, 23 Mar 2024 22:10:40 +0100
Christophe JAILLET <[email protected]> wrote:
> > - return devm_add_action_or_reset(dev, gpio_mockup_debugfs_cleanup, chip);
> > + return devm_add_action_or_reset(dev, devm_debugfs_dir_recursive_drop,
> > + chip->dbg_dir);
>
> This look strange. Shouldn't the debugfs_create_dir() call in
> gpio_mockup_debugfs_setup() be changed, instead?
>
> (I've not look in the previous version if something was said about it,
> so apologies if already discussed)
Yeah, this specific user needs a more complicated refactoring.
I was reluctant to do more complicated refactors in the patch that also
introduces these helpers.
But Guenter asked me to split the patch anyway...
> > static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
> > {
> > - pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
> > + pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
> > + if (IS_ERR(pvt->dbgfs_dir))
> > + return PTR_ERR(pvt->dbgfs_dir);
>
> Not sure if the test and error handling should be added here.
> *If I'm correct*, functions related to debugfs already handle this case
> and just do nothing. And failure in debugfs related code is not
> considered as something that need to be reported and abort a probe function.
>
> Maybe the same other (already existing) tests in this patch should be
> removed as well, in a separated patch.
Functions related to debugfs maybe do, but devm_ resource management
functions may fail to allocate release structure, and those errors need
to be handled, AFAIK.
Marek
Le 23/03/2024 à 22:25, Marek Behún a écrit :
> On Sat, 23 Mar 2024 22:10:40 +0100
> Christophe JAILLET <[email protected]> wrote:
>
...
>>> static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
>>> {
>>> - pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
>>> + pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
>>> + if (IS_ERR(pvt->dbgfs_dir))
>>> + return PTR_ERR(pvt->dbgfs_dir);
>>
>> Not sure if the test and error handling should be added here.
>> *If I'm correct*, functions related to debugfs already handle this case
>> and just do nothing. And failure in debugfs related code is not
>> considered as something that need to be reported and abort a probe function.
>>
>> Maybe the same other (already existing) tests in this patch should be
>> removed as well, in a separated patch.
>
> Functions related to debugfs maybe do, but devm_ resource management
> functions may fail to allocate release structure, and those errors need
> to be handled, AFAIK.
I would say no.
If this memory allocation fails, then debugfs_create_dir() will not be
called, but that's not a really big deal if the driver itself can still
run normally without it.
Up to you to leave it as-is or remove what I think is a useless error
handling.
At least, maybe it could be said in the commit log, so that maintainers
can comment on it, if they don't spot the error handling you introduce.
CJ
>
> Marek
>
On Sun, 24 Mar 2024 10:21:28 +0100
Christophe JAILLET <[email protected]> wrote:
> Le 23/03/2024 à 22:25, Marek Behún a écrit :
> > On Sat, 23 Mar 2024 22:10:40 +0100
> > Christophe JAILLET <[email protected]> wrote:
> >
>
> ...
>
> >>> static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
> >>> {
> >>> - pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
> >>> + pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
> >>> + if (IS_ERR(pvt->dbgfs_dir))
> >>> + return PTR_ERR(pvt->dbgfs_dir);
> >>
> >> Not sure if the test and error handling should be added here.
> >> *If I'm correct*, functions related to debugfs already handle this case
> >> and just do nothing. And failure in debugfs related code is not
> >> considered as something that need to be reported and abort a probe function.
> >>
> >> Maybe the same other (already existing) tests in this patch should be
> >> removed as well, in a separated patch.
> >
> > Functions related to debugfs maybe do, but devm_ resource management
> > functions may fail to allocate release structure, and those errors need
> > to be handled, AFAIK.
>
> I would say no.
> If this memory allocation fails, then debugfs_create_dir() will not be
> called, but that's not a really big deal if the driver itself can still
> run normally without it.
debugfs_create_dir() will always be called. Resource allocation is done
afterwards, and if it fails, then the created dir will be removed.
But now I don't know what to do, because yes, it seems that the debugfs
errors are being ignored at many places...
>
> Up to you to leave it as-is or remove what I think is a useless error
> handling.
> At least, maybe it could be said in the commit log, so that maintainers
> can comment on it, if they don't spot the error handling you introduce.
>
> CJ
>
> >
> > Marek
> >
>
On 3/23/24 18:43, Marek Behún wrote:
> Add resource managed version of irq_create_mapping(), to help drivers
> automatically dispose a linux irq mapping when driver is detached.
>
> The new function devm_irq_create_mapping() is not yet used, but the
> action function can be used in the FSL CAAM driver.
>
> Signed-off-by: Marek Behún <[email protected]>
> ---
> drivers/crypto/caam/jr.c | 8 ++----
> include/linux/devm-helpers.h | 54 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 56 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
> index 26eba7de3fb0..ad0295b055f8 100644
> --- a/drivers/crypto/caam/jr.c
> +++ b/drivers/crypto/caam/jr.c
> @@ -7,6 +7,7 @@
> * Copyright 2019, 2023 NXP
> */
>
> +#include <linux/devm-helpers.h>
> #include <linux/of_irq.h>
> #include <linux/of_address.h>
> #include <linux/platform_device.h>
> @@ -576,11 +577,6 @@ static int caam_jr_init(struct device *dev)
> return error;
> }
>
> -static void caam_jr_irq_dispose_mapping(void *data)
> -{
> - irq_dispose_mapping((unsigned long)data);
> -}
> -
> /*
> * Probe routine for each detected JobR subsystem.
> */
> @@ -656,7 +652,7 @@ static int caam_jr_probe(struct platform_device *pdev)
> return -EINVAL;
> }
>
> - error = devm_add_action_or_reset(jrdev, caam_jr_irq_dispose_mapping,
> + error = devm_add_action_or_reset(jrdev, devm_irq_mapping_drop,
> (void *)(unsigned long)jrpriv->irq);
> if (error)
> return error;
> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
> index 74891802200d..3805551fd433 100644
> --- a/include/linux/devm-helpers.h
> +++ b/include/linux/devm-helpers.h
> @@ -24,6 +24,8 @@
> */
>
> #include <linux/device.h>
> +#include <linux/kconfig.h>
> +#include <linux/irqdomain.h>
> #include <linux/workqueue.h>
My confidence level is not terribly high today, so I am likely to accept
just about any counter arguments :) But ... More I think of this whole
header, less convinced I am that this (the header) is a great idea. I
wonder who has authored a concept like this... :rolleyes:
Pulling punch of unrelated APIs (or, unrelated except the devm-usage) in
one header has potential to be including a lot of unneeded stuff to the
users. I am under impression this can be bad for example for the build
times.
I think that ideally the devm-APIs should live close to their non-devm
counterparts, and this header should be just used as a last resort, when
all the other options fail :) May I assume all other options have failed
for the IRQ stuff?
Well, I will leave the big picture to the bigger minds. When just
looking at the important things like the function names and coding style
- this change looks Ok to me ;)
> static inline void devm_delayed_work_drop(void *res)
> @@ -76,4 +78,56 @@ static inline int devm_work_autocancel(struct device *dev,
> return devm_add_action(dev, devm_work_drop, w);
> }
>
> +/**
> + * devm_irq_mapping_drop - devm action for disposing an irq mapping
> + * @res: linux irq number cast to the void * type
> + *
> + * devm_irq_mapping_drop() can be used as an action parameter for the
> + * devm_add_action_or_reset() function in order to automatically dispose
> + * a linux irq mapping when a device driver is detached.
> + */
> +static inline void devm_irq_mapping_drop(void *res)
> +{
> + irq_dispose_mapping((unsigned int)(unsigned long)res);
> +}
> +
> +/**
> + * devm_irq_create_mapping - Resource managed version of irq_create_mapping()
> + * @dev: Device which lifetime the mapping is bound to
> + * @domain: domain owning this hardware interrupt or NULL for default domain
> + * @hwirq: hardware irq number in that domain space
> + *
> + * Create an irq mapping to linux irq space which is automatically disposed when
> + * the driver is detached.
> + * devm_irq_create_mapping() can be used to omit the explicit
> + * irq_dispose_mapping() call when driver is detached.
> + *
> + * Returns a linux irq number on success, 0 if mapping could not be created, or
> + * a negative error number if devm action could not be added.
> + */
> +static inline int devm_irq_create_mapping(struct device *dev,
> + struct irq_domain *domain,
> + irq_hw_number_t hwirq)
> +{
> + unsigned int virq = irq_create_mapping(domain, hwirq);
> +
> + if (!virq)
> + return 0;
> +
> + /*
> + * irq_dispose_mapping() is an empty function if CONFIG_IRQ_DOMAIN is
> + * disabled. No need to register an action in that case.
> + */
> + if (IS_ENABLED(CONFIG_IRQ_DOMAIN)) {
> + int err;
> +
> + err = devm_add_action_or_reset(dev, devm_irq_mapping_drop,
> + (void *)(unsigned long)virq);
> + if (err)
> + return err;
> + }
> +
> + return virq;
> +}
> +
> #endif
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Mon, 25 Mar 2024 11:40:20 +0200
Matti Vaittinen <[email protected]> wrote:
> On 3/23/24 18:43, Marek Behún wrote:
> > Add resource managed version of irq_create_mapping(), to help drivers
> > automatically dispose a linux irq mapping when driver is detached.
> >
> > The new function devm_irq_create_mapping() is not yet used, but the
> > action function can be used in the FSL CAAM driver.
> >
> > Signed-off-by: Marek Behún <[email protected]>
> > ---
> > drivers/crypto/caam/jr.c | 8 ++----
> > include/linux/devm-helpers.h | 54 ++++++++++++++++++++++++++++++++++++
> > 2 files changed, 56 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
> > index 26eba7de3fb0..ad0295b055f8 100644
> > --- a/drivers/crypto/caam/jr.c
> > +++ b/drivers/crypto/caam/jr.c
> > @@ -7,6 +7,7 @@
> > * Copyright 2019, 2023 NXP
> > */
> >
> > +#include <linux/devm-helpers.h>
> > #include <linux/of_irq.h>
> > #include <linux/of_address.h>
> > #include <linux/platform_device.h>
> > @@ -576,11 +577,6 @@ static int caam_jr_init(struct device *dev)
> > return error;
> > }
> >
> > -static void caam_jr_irq_dispose_mapping(void *data)
> > -{
> > - irq_dispose_mapping((unsigned long)data);
> > -}
> > -
> > /*
> > * Probe routine for each detected JobR subsystem.
> > */
> > @@ -656,7 +652,7 @@ static int caam_jr_probe(struct platform_device *pdev)
> > return -EINVAL;
> > }
> >
> > - error = devm_add_action_or_reset(jrdev, caam_jr_irq_dispose_mapping,
> > + error = devm_add_action_or_reset(jrdev, devm_irq_mapping_drop,
> > (void *)(unsigned long)jrpriv->irq);
> > if (error)
> > return error;
> > diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
> > index 74891802200d..3805551fd433 100644
> > --- a/include/linux/devm-helpers.h
> > +++ b/include/linux/devm-helpers.h
> > @@ -24,6 +24,8 @@
> > */
> >
> > #include <linux/device.h>
> > +#include <linux/kconfig.h>
> > +#include <linux/irqdomain.h>
> > #include <linux/workqueue.h>
>
> My confidence level is not terribly high today, so I am likely to accept
> just about any counter arguments :) But ... More I think of this whole
> header, less convinced I am that this (the header) is a great idea. I
> wonder who has authored a concept like this... :rolleyes:
>
> Pulling punch of unrelated APIs (or, unrelated except the devm-usage) in
> one header has potential to be including a lot of unneeded stuff to the
> users. I am under impression this can be bad for example for the build
> times.
>
> I think that ideally the devm-APIs should live close to their non-devm
> counterparts, and this header should be just used as a last resort, when
> all the other options fail :) May I assume all other options have failed
> for the IRQ stuff?
>
> Well, I will leave the big picture to the bigger minds. When just
> looking at the important things like the function names and coding style
> - this change looks Ok to me ;)
If the authors of devm-helpers or someone else decide it should not
exist (due for example of long build times), I am OK with that.
But currently this seems to me to be the proper place to put this into.
Marek
On Sat, Mar 23, 2024 at 05:43:54PM +0100, Marek Beh?n wrote:
> +/**
> + * devm_irq_create_mapping - Resource managed version of irq_create_mapping()
> + * @dev: Device which lifetime the mapping is bound to
> + * @domain: domain owning this hardware interrupt or NULL for default domain
> + * @hwirq: hardware irq number in that domain space
> + *
> + * Create an irq mapping to linux irq space which is automatically disposed when
> + * the driver is detached.
> + * devm_irq_create_mapping() can be used to omit the explicit
> + * irq_dispose_mapping() call when driver is detached.
> + *
> + * Returns a linux irq number on success, 0 if mapping could not be created, or
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> + * a negative error number if devm action could not be added.
> + */
> +static inline int devm_irq_create_mapping(struct device *dev,
> + struct irq_domain *domain,
> + irq_hw_number_t hwirq)
> +{
> + unsigned int virq = irq_create_mapping(domain, hwirq);
> +
> + if (!virq)
> + return 0;
What is the point of returning zero instead of an error code? Neither
of the callers that are introduced later in the patchset use this.
I understand that it matches some of the other legacy irq function
behaviors, but I think we are trying to move away from that because it
just leads to bugs.
Since we don't need the zero now, let's wait until we have a user before
introducing this behavior. Then we can add a new function that returns
zero, but we'll still encourage people to use the standard error code
function where possible. And at the same time, when we do introduce the
zero is an error code, function you should contact
[email protected] so someone an write a static checker
rule to detect the bugs that result from it.
regards,
dan carpenter
> +
> + /*
> + * irq_dispose_mapping() is an empty function if CONFIG_IRQ_DOMAIN is
> + * disabled. No need to register an action in that case.
> + */
> + if (IS_ENABLED(CONFIG_IRQ_DOMAIN)) {
> + int err;
> +
> + err = devm_add_action_or_reset(dev, devm_irq_mapping_drop,
> + (void *)(unsigned long)virq);
> + if (err)
> + return err;
> + }
> +
> + return virq;
> +}
> +
> #endif
> --
> 2.43.2
>
On Tue, 26 Mar 2024 12:00:25 +0300
Dan Carpenter <[email protected]> wrote:
> On Sat, Mar 23, 2024 at 05:43:54PM +0100, Marek Behún wrote:
> > +/**
> > + * devm_irq_create_mapping - Resource managed version of irq_create_mapping()
> > + * @dev: Device which lifetime the mapping is bound to
> > + * @domain: domain owning this hardware interrupt or NULL for default domain
> > + * @hwirq: hardware irq number in that domain space
> > + *
> > + * Create an irq mapping to linux irq space which is automatically disposed when
> > + * the driver is detached.
> > + * devm_irq_create_mapping() can be used to omit the explicit
> > + * irq_dispose_mapping() call when driver is detached.
> > + *
> > + * Returns a linux irq number on success, 0 if mapping could not be created, or
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> > + * a negative error number if devm action could not be added.
> > + */
> > +static inline int devm_irq_create_mapping(struct device *dev,
> > + struct irq_domain *domain,
> > + irq_hw_number_t hwirq)
> > +{
> > + unsigned int virq = irq_create_mapping(domain, hwirq);
> > +
> > + if (!virq)
> > + return 0;
>
> What is the point of returning zero instead of an error code? Neither
> of the callers that are introduced later in the patchset use this.
>
> I understand that it matches some of the other legacy irq function
> behaviors, but I think we are trying to move away from that because it
> just leads to bugs.
>
> Since we don't need the zero now, let's wait until we have a user before
> introducing this behavior. Then we can add a new function that returns
> zero, but we'll still encourage people to use the standard error code
> function where possible. And at the same time, when we do introduce the
> zero is an error code, function you should contact
> [email protected] so someone an write a static checker
> rule to detect the bugs that result from it.
Hi Dan,
the first user of this function is the very next patch of this series,
and it does this:
+ irq = devm_irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
+ if (irq <= 0)
+ return dev_err_probe(dev, irq ?: -ENXIO,
+ "Cannot map MESSAGE_SIGNED IRQ\n");
So it handles !irq as -ENXIO.
I looked into several users who do
virq = irq_create_mapping()
and then reutrn errno if !virq:
git grep -A 3 'virq = irq_create_mapping'
Some return -ENOMEM, some -ENXIO, some -EINVAL.
What do you think?
Or should I send this driver without introducing this helper for now?
Marek
On Wed, Mar 27, 2024 at 10:34:19AM +0100, Marek Beh?n wrote:
> On Tue, 26 Mar 2024 12:00:25 +0300
> Dan Carpenter <[email protected]> wrote:
>
> > On Sat, Mar 23, 2024 at 05:43:54PM +0100, Marek Beh?n wrote:
> > > +/**
> > > + * devm_irq_create_mapping - Resource managed version of irq_create_mapping()
> > > + * @dev: Device which lifetime the mapping is bound to
> > > + * @domain: domain owning this hardware interrupt or NULL for default domain
> > > + * @hwirq: hardware irq number in that domain space
> > > + *
> > > + * Create an irq mapping to linux irq space which is automatically disposed when
> > > + * the driver is detached.
> > > + * devm_irq_create_mapping() can be used to omit the explicit
> > > + * irq_dispose_mapping() call when driver is detached.
> > > + *
> > > + * Returns a linux irq number on success, 0 if mapping could not be created, or
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > > + * a negative error number if devm action could not be added.
> > > + */
> > > +static inline int devm_irq_create_mapping(struct device *dev,
> > > + struct irq_domain *domain,
> > > + irq_hw_number_t hwirq)
> > > +{
> > > + unsigned int virq = irq_create_mapping(domain, hwirq);
> > > +
> > > + if (!virq)
> > > + return 0;
> >
> > What is the point of returning zero instead of an error code? Neither
> > of the callers that are introduced later in the patchset use this.
> >
> > I understand that it matches some of the other legacy irq function
> > behaviors, but I think we are trying to move away from that because it
> > just leads to bugs.
> >
> > Since we don't need the zero now, let's wait until we have a user before
> > introducing this behavior. Then we can add a new function that returns
> > zero, but we'll still encourage people to use the standard error code
> > function where possible. And at the same time, when we do introduce the
> > zero is an error code, function you should contact
> > [email protected] so someone an write a static checker
> > rule to detect the bugs that result from it.
>
> Hi Dan,
>
> the first user of this function is the very next patch of this series,
> and it does this:
>
> + irq = devm_irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
> + if (irq <= 0)
> + return dev_err_probe(dev, irq ?: -ENXIO,
> + "Cannot map MESSAGE_SIGNED IRQ\n");
>
> So it handles !irq as -ENXIO.
Yeah. But imagine how much easier it would be if devm_irq_create_mapping()
returned -ENXIO directly.
irq = devm_irq_create_mapping();
if (irq < 0)
return dev_err_probe(dev, irq,
"Cannot map MESSAGE_SIGNED IRQ\n");
>
> I looked into several users who do
> virq = irq_create_mapping()
> and then reutrn errno if !virq:
>
> git grep -A 3 'virq = irq_create_mapping'
>
> Some return -ENOMEM, some -ENXIO, some -EINVAL.
>
> What do you think?
-ENXIO is fine.
regards,
dan carpenter