Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Wed, Apr 24, 2024 at 7:14 PM Ondřej Jirman wrote: > On Wed, Apr 24, 2024 at 06:20:41PM GMT, Andy Shevchenko wrote: > > On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman wrote: > > > On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote: > > > > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan > > > > wrote: ... > > > > > ret = stk3310_init(indio_dev); > > > > > if (ret < 0) > > > > > - return ret; > > > > > + goto err_vdd_disable; > > > > > > > > This is wrong. You will have the regulator being disabled _before_ > > > > IRQ. Note, that the original code likely has a bug which sets states > > > > before disabling IRQ and removing a handler. > > > > > > How so? stk3310_init is called before enabling the interrupt. > > > > Exactly, IRQ is registered with devm and hence the error path and > > remove stages will got it in a wrong order. > > Makes no sense. Huh?! > IRQ is not enabled here, yet. So in error path, the code will > just disable the regulator and devm will unref it later on. IRQ doesn't enter > the picture here at all in the error path. Error path _after_ IRQ handler has been _successfully_ installed. And complete ->remove() stage. > > > Original code has a bug that IRQ is enabled before registering the > > > IIO device, > > > > Indeed, but this is another bug. > > > > > so if IRQ is triggered before registration, iio_push_event > > > from IRQ handler may be called on a not yet registered IIO device. > > > > > > Never saw it happen, though. :) > > > > Because nobody cares enough to enable DEBUG_SHIRQ. > > Nice debug tool. I bet it makes quite a mess when enabled. :) FWIW, I have had it enabled for ages, but I have only a few devices, so I fixed a few cases in the past WRT shared IRQ issues. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman wrote: > On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote: > > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan > > wrote: ... > > > ret = stk3310_init(indio_dev); > > > if (ret < 0) > > > - return ret; > > > + goto err_vdd_disable; > > > > This is wrong. You will have the regulator being disabled _before_ > > IRQ. Note, that the original code likely has a bug which sets states > > before disabling IRQ and removing a handler. > > How so? stk3310_init is called before enabling the interrupt. Exactly, IRQ is registered with devm and hence the error path and remove stages will got it in a wrong order. > Original code has a bug that IRQ is enabled before registering the > IIO device, Indeed, but this is another bug. > so if IRQ is triggered before registration, iio_push_event > from IRQ handler may be called on a not yet registered IIO device. > > Never saw it happen, though. :) Because nobody cares enough to enable DEBUG_SHIRQ. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan wrote:
>
> From: Ondrej Jirman
>
> VDD power input can be used to completely power off the chip during
> system suspend. Do so if available.
...
> ret = stk3310_init(indio_dev);
> if (ret < 0)
> - return ret;
> + goto err_vdd_disable;
This is wrong. You will have the regulator being disabled _before_
IRQ. Note, that the original code likely has a bug which sets states
before disabling IRQ and removing a handler.
Side note, you may make the driver neater with help of
struct device *dev = >dev;
defined in this patch.
...
> static int stk3310_suspend(struct device *dev)
> {
> struct stk3310_data *data;
> data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
Side note: This may be updated (in a separate change) to use
dev_get_drvdata() directly.
Jonathan, do we have something like iio_priv_from_drvdata(struct
device *dev)? Seems many drivers may utilise it.
> }
...
> static int stk3310_resume(struct device *dev)
Ditto.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 3/6] iio: light: stk3310: Manage LED power supply
On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan wrote: > > The stk3310 and stk3310 chips have an input for power to the infrared > LED. Add support for managing it's state. its ... > if (IS_ERR(data->vdd_reg)) > return dev_err_probe(>dev, ret, "get regulator vdd > failed\n"); > > + data->led_reg = devm_regulator_get(>dev, "leda"); > + if (IS_ERR(data->led_reg)) > + return dev_err_probe(>dev, ret, "get regulator led > failed\n"); Can't you use a bulk regulator API instead? -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Thu, Apr 18, 2024 at 8:50 PM Aren wrote: > On Thu, Apr 18, 2024 at 06:56:09PM +0300, Andy Shevchenko wrote: > > On Thu, Apr 18, 2024 at 6:06 PM Aren wrote: > > > On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote: > > > > On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan > > > > wrote: ... > > > > I forgot to check the order of freeing resources, be sure you have no > > > > devm_*() releases happening before this call. > > > > > > If I understand what you're saying, this should be fine. The driver just > > > uses devm to clean up acquired resources after remove is called. Or am I > > > missing something and resources could be freed before calling > > > stk3310_remove? > > > > I'm not objecting to that. The point here is that the resources should > > be freed in the reversed order. devm-allocated resources are deferred > > to be freed after the explicit driver ->remove() callback. At the end > > it should not interleave with each other, i.o.w. it should be > > probe: devm followed by non-devm > > remove: non-devm only. > > I think what you're describing is already the case, with the exception > of parts of the probe function not changed in this patch mixing > acquiring resources through devm with configuring the device. Okay, then we are fine! > I hope I'm not being dense, thanks for the clarification -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Thu, Apr 18, 2024 at 6:06 PM Aren wrote: > On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote: > > On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan > > wrote: ... > > > stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY); > > > + if (data->vdd_reg) > > > + regulator_disable(data->vdd_reg); > > > > I forgot to check the order of freeing resources, be sure you have no > > devm_*() releases happening before this call. > > If I understand what you're saying, this should be fine. The driver just > uses devm to clean up acquired resources after remove is called. Or am I > missing something and resources could be freed before calling > stk3310_remove? I'm not objecting to that. The point here is that the resources should be freed in the reversed order. devm-allocated resources are deferred to be freed after the explicit driver ->remove() callback. At the end it should not interleave with each other, i.o.w. it should be probe: devm followed by non-devm remove: non-devm only. -- With Best Regards, Andy Shevchenko
Re: [PATCH 3/4] iio: light: stk3310: log error if reading the chip id fails
On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan wrote:
>
> If the chip isn't powered, this call is likely to return an error.
> Without a log here the driver will silently fail to probe. Common errors
> are ENXIO (when the chip isn't powered) and ETIMEDOUT (when the i2c bus
> isn't powered).
> ret = regmap_read(data->regmap, STK3310_REG_ID, );
> - if (ret < 0)
> + if (ret < 0) {
> + dev_err(>dev, "failed to read chip id: %d", ret);
> return ret;
> + }
Briefly looking at the code it seems that this one is strictly part of
the probe phase, which means we may use
return dev_err_probe(...);
pattern. Yet, you may add another patch to clean up all of them:
_probe(), _init(), _regmap_init() to use the same pattern everywhere.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan wrote:
>
> From: Ondrej Jirman
>
> VDD power input can be used to completely power off the chip during
> system suspend. Do so if available.
...
> #include
> #include
> #include
> +#include
Move it to be ordered and add a blank line to separate iio/*.h group.
...
> + data->vdd_reg = devm_regulator_get_optional(>dev, "vdd");
> + if (IS_ERR(data->vdd_reg)) {
> + ret = PTR_ERR(data->vdd_reg);
> + if (ret == -ENODEV)
> + data->vdd_reg = NULL;
> + else
Redundant 'else' when you follow the pattern "check for error condition first".
> + return dev_err_probe(>dev, ret,
> +"get regulator vdd failed\n");
> + }
...
> + if (data->vdd_reg) {
> + ret = regulator_enable(data->vdd_reg);
> + if (ret)
> + return dev_err_probe(>dev, ret,
> +"regulator vdd enable failed\n");
> +
> + usleep_range(1000, 2000);
fsleep()
> + }
...
> stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
> + if (data->vdd_reg)
> + regulator_disable(data->vdd_reg);
I forgot to check the order of freeing resources, be sure you have no
devm_*() releases happening before this call.
...
> + usleep_range(1000, 2000);
fsleep()
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] [v3] module: don't ignore sysfs_create_link() failures
On Tue, Mar 26, 2024 at 03:57:18PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> The sysfs_create_link() return code is marked as __must_check, but the
> module_add_driver() function tries hard to not care, by assigning the
> return code to a variable. When building with 'make W=1', gcc still
> warns because this variable is only assigned but not used:
>
> drivers/base/module.c: In function 'module_add_driver':
> drivers/base/module.c:36:6: warning: variable 'no_warn' set but not used
> [-Wunused-but-set-variable]
>
> Rework the code to properly unwind and return the error code to the
> caller. My reading of the original code was that it tries to
> not fail when the links already exist, so keep ignoring -EEXIST
> errors.
> Cc: Luis Chamberlain
> Cc: linux-modu...@vger.kernel.org
> Cc: Greg Kroah-Hartman
> Cc: "Rafael J. Wysocki"
Wondering if you can move these to be after --- to avoid polluting commit
message. This will have the same effect and be archived on lore. But on
pros side it will unload the commit message(s) from unneeded noise.
...
> + error = module_add_driver(drv->owner, drv);
> + if (error) {
> + printk(KERN_ERR "%s: failed to create module links for %s\n",
> + __func__, drv->name);
What's wrong with pr_err()? Even if it's not a style used, in a new pieces of
code this can be improved beforehand. So, we will reduce a technical debt, and
not adding to it.
> + goto out_detach;
> + }
...
> +int module_add_driver(struct module *mod, struct device_driver *drv)
> {
> char *driver_name;
> - int no_warn;
> + int ret;
I would move it...
> struct module_kobject *mk = NULL;
...to be here.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/1] ACPI: NFIT: Switch to use acpi_evaluate_dsm_typed()
On Mon, Nov 20, 2023 at 07:19:44PM +0200, Andy Shevchenko wrote: > On Mon, Nov 20, 2023 at 04:11:54PM +0100, Rafael J. Wysocki wrote: > > On Mon, Nov 20, 2023 at 4:03 PM Andy Shevchenko > > wrote: > > > On Thu, Oct 19, 2023 at 06:03:28PM -0700, Dan Williams wrote: > > > > Andy Shevchenko wrote: > > > > > The acpi_evaluate_dsm_typed() provides a way to check the type of the > > > > > object evaluated by _DSM call. Use it instead of open coded variant. > > > > > > > > Looks good to me. > > > > > > > > Reviewed-by: Dan Williams > > > > > > Thank you! > > > > > > Who is taking care of this? Rafael? > > > > I can apply it. > > Would be nice, thank you! Any news on this? -- With Best Regards, Andy Shevchenko
Re: [PATCH] tracing histograms: Simplify parse_actions() function
On Mon, Jan 8, 2024 at 3:31 AM Steven Rostedt wrote:
>
> From: "Steven Rostedt (Google)"
>
> The parse_actions() function uses 'len = str_has_prefix()' to test which
> action is in the string being parsed. But then it goes and repeats the
> logic for each different action. This logic can be simplified and
> duplicate code can be removed as 'len' contains the length of the found
> prefix which should be used for all actions.
> Link: https://lore.kernel.org/all/20240107112044.6702c...@gandalf.local.home/
>
> Signed-off-by: Steven Rostedt (Google)
If you want Link to be formally a tag, you should drop the following
blank line.
> + if ((len = str_has_prefix(str, "onmatch(")))
> + hid = HANDLER_ONMATCH;
> + else if ((len = str_has_prefix(str, "onmax(")))
> + hid = HANDLER_ONMAX;
> + else if ((len = str_has_prefix(str, "onchange(")))
> + hid = HANDLER_ONCHANGE;
The repeating check for ( might be moved out as well after this like
if (str[len] != '(') {
// not sure if you need data to be assigned here as well
ret = -EINVAL;
...
}
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 0/5] params: harden string ops and allocatio ops
On Fri, Dec 01, 2023 at 09:43:34AM -0800, Kees Cook wrote: > On Mon, 20 Nov 2023 17:11:41 +0200, Andy Shevchenko wrote: > > A couple of patches are for get the string ops, used in the module, > > slightly harden. On top a few cleanups. > > > > Since the main part is rather hardening, I think the Kees' tree is > > the best fit for the series. It also possible to route via Greg's > > sysfs (driver core?), but I'm open for another option(s). [...] > Applied to for-next/hardening, thanks! Awesome, thanks! -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 0/5] params: harden string ops and allocatio ops
On Mon, Nov 20, 2023 at 05:11:41PM +0200, Andy Shevchenko wrote: > A couple of patches are for get the string ops, used in the module, > slightly harden. On top a few cleanups. > > Since the main part is rather hardening, I think the Kees' tree is > the best fit for the series. It also possible to route via Greg's > sysfs (driver core?), but I'm open for another option(s). Kees, Greg, can you apply this series? Or should I do something about it? -- With Best Regards, Andy Shevchenko
[PATCH v3 2/5] params: Do not go over the limit when getting the string length
We can use strnlen() even on early stages and it prevents from
going over the string boundaries in case it's already too long.
Reviewed-by: Luis Chamberlain
Reviewed-by: Kees Cook
Signed-off-by: Andy Shevchenko
---
kernel/params.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/kernel/params.c b/kernel/params.c
index 626fa8265932..f8e3c4139854 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -260,7 +260,10 @@ EXPORT_SYMBOL_GPL(param_set_uint_minmax);
int param_set_charp(const char *val, const struct kernel_param *kp)
{
- if (strlen(val) > 1024) {
+ size_t len, maxlen = 1024;
+
+ len = strnlen(val, maxlen + 1);
+ if (len == maxlen + 1) {
pr_err("%s: string parameter too long\n", kp->name);
return -ENOSPC;
}
@@ -270,7 +273,7 @@ int param_set_charp(const char *val, const struct
kernel_param *kp)
/* This is a hack. We can't kmalloc in early boot, and we
* don't need to; this mangled commandline is preserved. */
if (slab_is_available()) {
- *(char **)kp->arg = kmalloc_parameter(strlen(val)+1);
+ *(char **)kp->arg = kmalloc_parameter(len + 1);
if (!*(char **)kp->arg)
return -ENOMEM;
strcpy(*(char **)kp->arg, val);
@@ -508,7 +511,7 @@ int param_set_copystring(const char *val, const struct
kernel_param *kp)
{
const struct kparam_string *kps = kp->str;
- if (strlen(val)+1 > kps->maxlen) {
+ if (strnlen(val, kps->maxlen) == kps->maxlen) {
pr_err("%s: string doesn't fit in %u chars.\n",
kp->name, kps->maxlen-1);
return -ENOSPC;
--
2.43.0.rc1.1.gbec44491f096
[PATCH v3 5/5] params: Fix multi-line comment style
The multi-line comment style in the file is rather arbitrary.
Make it follow the standard one.
Reviewed-by: Luis Chamberlain
Reviewed-by: Kees Cook
Signed-off-by: Andy Shevchenko
---
kernel/params.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/kernel/params.c b/kernel/params.c
index eb55b32399b4..2e447f8ae183 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -1,8 +1,8 @@
// SPDX-License-Identifier: GPL-2.0-or-later
-/* Helpers for initial module or kernel cmdline parsing
- Copyright (C) 2001 Rusty Russell.
-
-*/
+/*
+ * Helpers for initial module or kernel cmdline parsing
+ * Copyright (C) 2001 Rusty Russell.
+ */
#include
#include
#include
@@ -271,8 +271,10 @@ int param_set_charp(const char *val, const struct
kernel_param *kp)
maybe_kfree_parameter(*(char **)kp->arg);
- /* This is a hack. We can't kmalloc in early boot, and we
-* don't need to; this mangled commandline is preserved. */
+ /*
+* This is a hack. We can't kmalloc() in early boot, and we
+* don't need to; this mangled commandline is preserved.
+*/
if (slab_is_available()) {
*(char **)kp->arg = kmalloc_parameter(len + 1);
if (!*(char **)kp->arg)
@@ -743,8 +745,10 @@ void module_param_sysfs_remove(struct module *mod)
{
if (mod->mkobj.mp) {
sysfs_remove_group(>mkobj.kobj, >mkobj.mp->grp);
- /* We are positive that no one is using any param
-* attrs at this point. Deallocate immediately. */
+ /*
+* We are positive that no one is using any param
+* attrs at this point. Deallocate immediately.
+*/
free_module_param_attrs(>mkobj);
}
}
--
2.43.0.rc1.1.gbec44491f096
[PATCH v3 4/5] params: Sort headers
Sort the headers in alphabetic order in order to ease the maintenance for this part. Reviewed-by: Luis Chamberlain Reviewed-by: Kees Cook Signed-off-by: Andy Shevchenko --- kernel/params.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/kernel/params.c b/kernel/params.c index c3a029fe183d..eb55b32399b4 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -3,18 +3,18 @@ Copyright (C) 2001 Rusty Russell. */ -#include -#include -#include -#include -#include -#include +#include #include #include +#include +#include +#include +#include +#include #include -#include -#include #include +#include +#include #ifdef CONFIG_SYSFS /* Protects all built-in parameters, modules use their own param_lock */ -- 2.43.0.rc1.1.gbec44491f096
[PATCH v3 1/5] params: Introduce the param_unknown_fn type
Introduce a new type for the callback to parse an unknown argument.
This unifies function prototypes which takes that as a parameter.
Reviewed-by: Luis Chamberlain
Reviewed-by: Kees Cook
Signed-off-by: Andy Shevchenko
---
include/linux/moduleparam.h | 6 +++---
kernel/params.c | 8 ++--
2 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 4fa9726bc328..bfb85fd13e1f 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -385,6 +385,8 @@ extern bool parameq(const char *name1, const char *name2);
*/
extern bool parameqn(const char *name1, const char *name2, size_t n);
+typedef int (*parse_unknown_fn)(char *param, char *val, const char *doing,
void *arg);
+
/* Called on module insert or kernel boot */
extern char *parse_args(const char *name,
char *args,
@@ -392,9 +394,7 @@ extern char *parse_args(const char *name,
unsigned num,
s16 level_min,
s16 level_max,
- void *arg,
- int (*unknown)(char *param, char *val,
-const char *doing, void *arg));
+ void *arg, parse_unknown_fn unknown);
/* Called by module remove. */
#ifdef CONFIG_SYSFS
diff --git a/kernel/params.c b/kernel/params.c
index 2d4a0564697e..626fa8265932 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -120,9 +120,7 @@ static int parse_one(char *param,
unsigned num_params,
s16 min_level,
s16 max_level,
-void *arg,
-int (*handle_unknown)(char *param, char *val,
-const char *doing, void *arg))
+void *arg, parse_unknown_fn handle_unknown)
{
unsigned int i;
int err;
@@ -165,9 +163,7 @@ char *parse_args(const char *doing,
unsigned num,
s16 min_level,
s16 max_level,
-void *arg,
-int (*unknown)(char *param, char *val,
- const char *doing, void *arg))
+void *arg, parse_unknown_fn unknown)
{
char *param, *val, *err = NULL;
--
2.43.0.rc1.1.gbec44491f096
[PATCH v3 3/5] params: Use size_add() for kmalloc()
Prevent allocations from integer overflow by using size_add().
Reviewed-by: Luis Chamberlain
Reviewed-by: Kees Cook
Signed-off-by: Andy Shevchenko
---
kernel/params.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/params.c b/kernel/params.c
index f8e3c4139854..c3a029fe183d 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -48,7 +49,7 @@ static void *kmalloc_parameter(unsigned int size)
{
struct kmalloced_param *p;
- p = kmalloc(sizeof(*p) + size, GFP_KERNEL);
+ p = kmalloc(size_add(sizeof(*p), size), GFP_KERNEL);
if (!p)
return NULL;
--
2.43.0.rc1.1.gbec44491f096
[PATCH v3 0/5] params: harden string ops and allocatio ops
A couple of patches are for get the string ops, used in the module, slightly harden. On top a few cleanups. Since the main part is rather hardening, I think the Kees' tree is the best fit for the series. It also possible to route via Greg's sysfs (driver core?), but I'm open for another option(s). Changelog v3: - added tags (Kees, Luis) Changelog v2: - dropped the s*printf() --> sysfs_emit() conversion as it revealed an issue, i.e. reuse getters with non-page-aligned pointer, which would be addressed separately - added cover letter and clarified the possible route for the series (Luis) Andy Shevchenko (5): params: Introduce the param_unknown_fn type params: Do not go over the limit when getting the string length params: Use size_add() for kmalloc() params: Sort headers params: Fix multi-line comment style include/linux/moduleparam.h | 6 ++-- kernel/params.c | 56 - 2 files changed, 33 insertions(+), 29 deletions(-) -- 2.43.0.rc1.1.gbec44491f096
Re: [PATCH v1 1/1] ACPI: NFIT: Switch to use acpi_evaluate_dsm_typed()
On Thu, Oct 19, 2023 at 06:03:28PM -0700, Dan Williams wrote: > Andy Shevchenko wrote: > > The acpi_evaluate_dsm_typed() provides a way to check the type of the > > object evaluated by _DSM call. Use it instead of open coded variant. > > Looks good to me. > > Reviewed-by: Dan Williams Thank you! Who is taking care of this? Rafael? -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] ACPI: NFIT: Switch to use acpi_evaluate_dsm_typed()
On Mon, Oct 02, 2023 at 04:54:58PM +0300, Andy Shevchenko wrote: > The acpi_evaluate_dsm_typed() provides a way to check the type of the > object evaluated by _DSM call. Use it instead of open coded variant. Dan, do you have any comments? -- With Best Regards, Andy Shevchenko
Re: [PATCH v2] ACPI: NFIT: Fix local use of devm_*()
On Sat, Oct 14, 2023 at 12:20 AM Dan Williams wrote: > Wilczynski, Michal wrote: ... > "The new cleanup.h facilities that arrived in v6.5-rc1 can replace the > the usage of devm semantics in acpi_nfit_init_interleave_set(). That > routine appears to only be using devm to avoid goto statements. The new > __free() annotation at variable declaration time can achieve the same > effect more efficiently. > > There is no end user visible side effects of this patch, I was motivated > to send this cleanup to practice using the new helpers." The end-user side effect (educational and not run-time) is that: "One should really be careful about the scope of the devm_*() APIs and use of them just for the sake of the RAII replacement is not the best idea, while code is still working. Hence it gives a better example for whoever tries to use this code for educational purposes." -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 3/6] ACPI: AC: Replace acpi_driver with platform_driver
On Fri, Oct 06, 2023 at 09:47:57PM +0200, Rafael J. Wysocki wrote:
> On Fri, Oct 6, 2023 at 8:33 PM Michal Wilczynski
> wrote:
...
> > struct acpi_ac {
> > struct power_supply *charger;
> > struct power_supply_desc charger_desc;
> > - struct acpi_device *device;
> > + struct device *dev;
>
> I'm not convinced about this change.
>
> If I'm not mistaken, you only use the dev pointer above to get the
> ACPI_COMPANION() of it, but the latter is already found in _probe(),
> so it can be stored in struct acpi_ac for later use and then the dev
> pointer in there will not be necessary any more.
>
> That will save you a bunch of ACPI_HANDLE() evaluations and there's
> nothing wrong with using ac->device->handle. The patch will then
> become almost trivial AFAICS and if you really need to get from ac to
> the underlying platform device, a pointer to it can be added to struct
> acpi_ac without removing the ACPI device pointer from it.
The idea behind is to eliminate data duplication.
> > unsigned long long state;
> > struct notifier_block battery_nb;
> > };
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 3/6] ACPI: AC: Replace acpi_driver with platform_driver
On Fri, Oct 06, 2023 at 08:30:52PM +0300, Michal Wilczynski wrote:
> AC driver uses struct acpi_driver incorrectly to register itself. This
> is wrong as the instances of the ACPI devices are not meant to
> be literal devices, they're supposed to describe ACPI entry of a
> particular device.
>
> Use platform_driver instead of acpi_driver. In relevant places call
> platform devices instances pdev to make a distinction with ACPI
> devices instances.
>
> Drop unnecessary casts from acpi_bus_generate_netlink_event() and
> acpi_notifier_call_chain().
>
> Add a blank line to distinguish pdev API vs local ACPI notify function.
...
> struct acpi_ac {
> struct power_supply *charger;
> struct power_supply_desc charger_desc;
> - struct acpi_device *device;
> + struct device *dev;
> unsigned long long state;
> struct notifier_block battery_nb;
> };
When changing this, also makes sense just to check if the moving a member in
the data structure makes code shorter, but it's not a show stopper.
...
> - status = acpi_evaluate_integer(ac->device->handle, "_PSR", NULL,
> + status = acpi_evaluate_integer(ACPI_HANDLE(ac->dev), "_PSR", NULL,
> >state);
> if (ACPI_FAILURE(status)) {
> - acpi_handle_info(ac->device->handle,
> + acpi_handle_info(ACPI_HANDLE(ac->dev),
Can we call ACPI_HANDLE() only once and cache that in a local variable and use
in all places?
...
> - struct acpi_ac *ac = acpi_driver_data(device);
> + struct acpi_ac *ac = data;
> + struct acpi_device *device = ACPI_COMPANION(ac->dev);
>
> switch (event) {
> default:
> - acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
> + acpi_handle_debug(ACPI_HANDLE(ac->dev), "Unsupported event
> [0x%x]\n",
> event);
Does it makes any sense now? Basically it duplicates the ACPI_COMPANION() call
as Rafael pointed out in previous version discussion.
> fallthrough;
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/1] ACPI: NFIT: Switch to use acpi_evaluate_dsm_typed()
On Mon, Oct 02, 2023 at 10:27:02PM +0200, Wilczynski, Michal wrote:
> On 10/2/2023 3:54 PM, Andy Shevchenko wrote:
...
> > + out_obj = acpi_evaluate_dsm_typed(handle, guid, revid, func, _obj,
> > ACPI_TYPE_BUFFER);
>
> This line is 90 characters long, wouldn't it be better to split it ?
I dunno it's a problem, but if people insist, I can redo that.
...
> > + if (!out_obj || out_obj->buffer.length < sizeof(smart)) {
> > dev_dbg(dev->parent, "%s: failed to retrieve initial health\n",
> > dev_name(dev));
>
> While at it maybe fix alignment ? :-)
I don't think it's in scope of this change.
> > ACPI_FREE(out_obj);
>
> Just nitpicks, functionally code seems correct to me.
> Reviewed-by: Michal Wilczynski
Thank you!
--
With Best Regards,
Andy Shevchenko
[PATCH v1 1/1] ACPI: NFIT: Switch to use acpi_evaluate_dsm_typed()
The acpi_evaluate_dsm_typed() provides a way to check the type of the
object evaluated by _DSM call. Use it instead of open coded variant.
Signed-off-by: Andy Shevchenko
---
drivers/acpi/nfit/core.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index f96bf32cd368..280da408c02c 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1737,9 +1737,8 @@ __weak void nfit_intel_shutdown_status(struct nfit_mem
*nfit_mem)
if ((nfit_mem->dsm_mask & (1 << func)) == 0)
return;
- out_obj = acpi_evaluate_dsm(handle, guid, revid, func, _obj);
- if (!out_obj || out_obj->type != ACPI_TYPE_BUFFER
- || out_obj->buffer.length < sizeof(smart)) {
+ out_obj = acpi_evaluate_dsm_typed(handle, guid, revid, func, _obj,
ACPI_TYPE_BUFFER);
+ if (!out_obj || out_obj->buffer.length < sizeof(smart)) {
dev_dbg(dev->parent, "%s: failed to retrieve initial health\n",
dev_name(dev));
ACPI_FREE(out_obj);
--
2.40.0.1.gaa8946217a0b
[PATCH v2 4/5] params: Sort headers
Sort the headers in alphabetic order in order to ease the maintenance for this part. Signed-off-by: Andy Shevchenko --- kernel/params.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/kernel/params.c b/kernel/params.c index c3a029fe183d..eb55b32399b4 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -3,18 +3,18 @@ Copyright (C) 2001 Rusty Russell. */ +#include +#include +#include +#include #include #include -#include -#include #include #include -#include -#include #include -#include -#include #include +#include +#include #ifdef CONFIG_SYSFS /* Protects all built-in parameters, modules use their own param_lock */ -- 2.40.0.1.gaa8946217a0b
[PATCH v2 1/5] params: Introduce the param_unknown_fn type
Introduce a new type for the callback to parse an unknown argument.
This unifies function prototypes which takes that as a parameter.
Signed-off-by: Andy Shevchenko
---
include/linux/moduleparam.h | 6 +++---
kernel/params.c | 8 ++--
2 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 4fa9726bc328..bfb85fd13e1f 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -385,6 +385,8 @@ extern bool parameq(const char *name1, const char *name2);
*/
extern bool parameqn(const char *name1, const char *name2, size_t n);
+typedef int (*parse_unknown_fn)(char *param, char *val, const char *doing,
void *arg);
+
/* Called on module insert or kernel boot */
extern char *parse_args(const char *name,
char *args,
@@ -392,9 +394,7 @@ extern char *parse_args(const char *name,
unsigned num,
s16 level_min,
s16 level_max,
- void *arg,
- int (*unknown)(char *param, char *val,
-const char *doing, void *arg));
+ void *arg, parse_unknown_fn unknown);
/* Called by module remove. */
#ifdef CONFIG_SYSFS
diff --git a/kernel/params.c b/kernel/params.c
index 2d4a0564697e..626fa8265932 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -120,9 +120,7 @@ static int parse_one(char *param,
unsigned num_params,
s16 min_level,
s16 max_level,
-void *arg,
-int (*handle_unknown)(char *param, char *val,
-const char *doing, void *arg))
+void *arg, parse_unknown_fn handle_unknown)
{
unsigned int i;
int err;
@@ -165,9 +163,7 @@ char *parse_args(const char *doing,
unsigned num,
s16 min_level,
s16 max_level,
-void *arg,
-int (*unknown)(char *param, char *val,
- const char *doing, void *arg))
+void *arg, parse_unknown_fn unknown)
{
char *param, *val, *err = NULL;
--
2.40.0.1.gaa8946217a0b
[PATCH v2 3/5] params: Use size_add() for kmalloc()
Prevent allocations from integer overflow by using size_add().
Signed-off-by: Andy Shevchenko
---
kernel/params.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/params.c b/kernel/params.c
index f8e3c4139854..c3a029fe183d 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -48,7 +49,7 @@ static void *kmalloc_parameter(unsigned int size)
{
struct kmalloced_param *p;
- p = kmalloc(sizeof(*p) + size, GFP_KERNEL);
+ p = kmalloc(size_add(sizeof(*p), size), GFP_KERNEL);
if (!p)
return NULL;
--
2.40.0.1.gaa8946217a0b
[PATCH v2 0/5] params: harden string ops and allocatio ops
A couple of patches are for get the string ops, used in the module, slightly harden. On top a few cleanups. Since the main part is rather hardening, I think the Kees' tree is the best fit for the series, but I'm open for another option(s). Changelog v2: - dropped the s*printf() --> sysfs_emit() conversion as it revealed an issue, i.e. reuse getters with non-page-aligned pointer, which would be addressed separately - added cover letter and clarified the possible route for the series (Luis) Andy Shevchenko (5): params: Introduce the param_unknown_fn type params: Do not go over the limit when getting the string length params: Use size_add() for kmalloc() params: Sort headers params: Fix multi-line comment style include/linux/moduleparam.h | 6 ++--- kernel/params.c | 52 - 2 files changed, 31 insertions(+), 27 deletions(-) -- 2.40.0.1.gaa8946217a0b
[PATCH v2 2/5] params: Do not go over the limit when getting the string length
We can use strnlen() even on early stages and it prevents from
going over the string boundaries in case it's already too long.
Signed-off-by: Andy Shevchenko
---
kernel/params.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/kernel/params.c b/kernel/params.c
index 626fa8265932..f8e3c4139854 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -260,7 +260,10 @@ EXPORT_SYMBOL_GPL(param_set_uint_minmax);
int param_set_charp(const char *val, const struct kernel_param *kp)
{
- if (strlen(val) > 1024) {
+ size_t len, maxlen = 1024;
+
+ len = strnlen(val, maxlen + 1);
+ if (len == maxlen + 1) {
pr_err("%s: string parameter too long\n", kp->name);
return -ENOSPC;
}
@@ -270,7 +273,7 @@ int param_set_charp(const char *val, const struct
kernel_param *kp)
/* This is a hack. We can't kmalloc in early boot, and we
* don't need to; this mangled commandline is preserved. */
if (slab_is_available()) {
- *(char **)kp->arg = kmalloc_parameter(strlen(val)+1);
+ *(char **)kp->arg = kmalloc_parameter(len + 1);
if (!*(char **)kp->arg)
return -ENOMEM;
strcpy(*(char **)kp->arg, val);
@@ -508,7 +511,7 @@ int param_set_copystring(const char *val, const struct
kernel_param *kp)
{
const struct kparam_string *kps = kp->str;
- if (strlen(val)+1 > kps->maxlen) {
+ if (strnlen(val, kps->maxlen) == kps->maxlen) {
pr_err("%s: string doesn't fit in %u chars.\n",
kp->name, kps->maxlen-1);
return -ENOSPC;
--
2.40.0.1.gaa8946217a0b
[PATCH v2 5/5] params: Fix multi-line comment style
The multi-line comment style in the file is rather arbitrary.
Make it follow the standard one.
Signed-off-by: Andy Shevchenko
---
kernel/params.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/kernel/params.c b/kernel/params.c
index eb55b32399b4..2e447f8ae183 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -1,8 +1,8 @@
// SPDX-License-Identifier: GPL-2.0-or-later
-/* Helpers for initial module or kernel cmdline parsing
- Copyright (C) 2001 Rusty Russell.
-
-*/
+/*
+ * Helpers for initial module or kernel cmdline parsing
+ * Copyright (C) 2001 Rusty Russell.
+ */
#include
#include
#include
@@ -271,8 +271,10 @@ int param_set_charp(const char *val, const struct
kernel_param *kp)
maybe_kfree_parameter(*(char **)kp->arg);
- /* This is a hack. We can't kmalloc in early boot, and we
-* don't need to; this mangled commandline is preserved. */
+ /*
+* This is a hack. We can't kmalloc() in early boot, and we
+* don't need to; this mangled commandline is preserved.
+*/
if (slab_is_available()) {
*(char **)kp->arg = kmalloc_parameter(len + 1);
if (!*(char **)kp->arg)
@@ -743,8 +745,10 @@ void module_param_sysfs_remove(struct module *mod)
{
if (mod->mkobj.mp) {
sysfs_remove_group(>mkobj.kobj, >mkobj.mp->grp);
- /* We are positive that no one is using any param
-* attrs at this point. Deallocate immediately. */
+ /*
+* We are positive that no one is using any param
+* attrs at this point. Deallocate immediately.
+*/
free_module_param_attrs(>mkobj);
}
}
--
2.40.0.1.gaa8946217a0b
Re: [PATCH v1 2/2] ACPI: NFIT: Use modern scope based rollback
On Tue, Sep 26, 2023 at 09:45:20PM +0300, Michal Wilczynski wrote: > Change rollback in acpi_nfit_init_interleave_set() to use modern scope > based attribute __free(). This is similar to C++ RAII and is a preferred > way for handling local memory allocations. LGTM, Reviewed-by: Andy Shevchenko -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/2] ACPI: NFIT: Fix memory leak, and local use of devm_*()
On Tue, Sep 26, 2023 at 09:45:19PM +0300, Michal Wilczynski wrote: > devm_*() family of functions purpose is managing memory attached to a > device. So in general it should only be used for allocations that should > last for the whole lifecycle of the device. This is not the case for > acpi_nfit_init_interleave_set(). There are two allocations that are only > used locally in this function. What's more - if the function exits on > error path memory is never freed. It's still attached to dev and would > be freed on device detach, so this leak could be called a 'local leak'. > > Fix this by switching from devm_kcalloc() to kcalloc(), and adding > proper rollback. LGTM, Reviewed-by: Andy Shevchenko -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 9/9] ACPI: NFIT: Don't use KBUILD_MODNAME for driver name
On Mon, Sep 25, 2023 at 05:48:42PM +0300, Michal Wilczynski wrote: > Driver name is part of the ABI, so it should be hard-coded, as ABI > should be always kept backward compatible. Prevent ABI from changing > accidentally in case KBUILD_MODNAME change. This is up to maintainers, probably we won't have any users outside of existing model (instantiating via ACPI ID). All the above is "strictly speaking"... -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] lib/string_helpers: Don't copy a tail in kstrdup_and_replace() if 'new' is \0
On Wed, Sep 13, 2023 at 12:45:57PM +0300, Andy Shevchenko wrote: > The kstrdup_and_replace() takes two characters, old and new, to replace > former with latter after the copying of the original string. But in case > when new is a NUL, there is no point to copy the rest of the string, > the contract with the callers is that that the function returns a > NUL-terminated string and not a buffer of the size filled with a given > data. With this we can optimize the memory consumption by copying only > meaningful part of the original string and drop the rest. Thinking about this more, I self NAK this. If the caller knows the size of the original message it can be handy to make a copy and replace all occurrences of old by NUL. This will be an optimized implementation of strsep(str, "$OLD"). -- With Best Regards, Andy Shevchenko
[PATCH v1 1/1] lib/string_helpers: Don't copy a tail in kstrdup_and_replace() if 'new' is \0
The kstrdup_and_replace() takes two characters, old and new, to replace
former with latter after the copying of the original string. But in case
when new is a NUL, there is no point to copy the rest of the string,
the contract with the callers is that that the function returns a
NUL-terminated string and not a buffer of the size filled with a given
data. With this we can optimize the memory consumption by copying only
meaningful part of the original string and drop the rest.
Signed-off-by: Andy Shevchenko
---
The first user of this is pending:
https://lore.kernel.org/platform-driver-x86/20230913092701.440959-1-andriy.shevche...@linux.intel.com/
lib/string_helpers.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 7713f73e66b0..e385bf3cc2de 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -723,11 +723,17 @@ EXPORT_SYMBOL_GPL(kstrdup_quotable_file);
/*
* Returns duplicate string in which the @old characters are replaced by @new.
+ *
+ * If @new is NUL, copy the string up to the first occurrence of @old, which
+ * will be replaced by a NUL.
*/
char *kstrdup_and_replace(const char *src, char old, char new, gfp_t gfp)
{
char *dst;
+ if (new == '\0')
+ return kmemdup_nul(src, strchrnul(src, old) - src, gfp);
+
dst = kstrdup(src, gfp);
if (!dst)
return NULL;
--
2.40.0.1.gaa8946217a0b
Re: [PATCH] nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()
On Fri, Apr 14, 2023 at 12:12:37PM +0200, Markus Elfring wrote:
> Date: Fri, 14 Apr 2023 12:01:15 +0200
>
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “nd_pfn_validate”.
>
> Thus avoid the risk for undefined behaviour by replacing the usage of
> the local variable “parent_uuid” by a direct function call within
> a later condition check.
> This issue was detected by using the Coccinelle software.
>
> Fixes: d1c6e08e7503649e4a4f3f9e700e2c05300b6379 ("libnvdimm/labels: Add uuid
> helpers")
Same issues as per patch 1.
...
> - if (memcmp(pfn_sb->parent_uuid, parent_uuid, 16) != 0)
> + if (memcmp(pfn_sb->parent_uuid, nd_dev_to_uuid(>dev), 16) != 0)
If parent_uuid is of uuid_t type, you better to replace memcmp() with
uuid_equal().
> return -ENODEV;
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/1] nvdimm/namespace: return uuid_null only once in nd_dev_to_uuid()
On Thu, Jul 14, 2022 at 11:24:05AM -0700, Dan Williams wrote: > Andy Shevchenko wrote: > > Refactor nd_dev_to_uuid() in order to make code shorter and cleaner > > by joining conditions and hence returning uuid_null only once. > > Apologies for the delay, applied for v5.20. No problem and thanks! P.S. One patch out of three is a fix, would be nice to have it in v5.19 release. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] nvdimm/namespace: drop nested variable in create_namespace_pmem()
On Tue, Jun 07, 2022 at 07:49:37PM +0300, Andy Shevchenko wrote: > Kernel build bot reported: > > namespace_devs.c:1991:10: warning: Local variable 'uuid' shadows outer > variable [shadowVariable] > > Refactor create_namespace_pmem() by dropping a nested version of > the same variable. Any comments on this and other two patches? -- With Best Regards, Andy Shevchenko
[PATCH v1 1/1] nvdimm/namespace: drop nested variable in create_namespace_pmem()
Kernel build bot reported:
namespace_devs.c:1991:10: warning: Local variable 'uuid' shadows outer
variable [shadowVariable]
Refactor create_namespace_pmem() by dropping a nested version of
the same variable.
Fixes: d1c6e08e7503 ("libnvdimm/labels: Add uuid helpers")
Reported-by: kernel test robot
Signed-off-by: Andy Shevchenko
---
drivers/nvdimm/namespace_devs.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 0f863fda56e6..dfade66bab73 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -1704,8 +1704,6 @@ static struct device *create_namespace_pmem(struct
nd_region *nd_region,
res->flags = IORESOURCE_MEM;
for (i = 0; i < nd_region->ndr_mappings; i++) {
- uuid_t uuid;
-
nsl_get_uuid(ndd, nd_label, );
if (has_uuid_at_pos(nd_region, , cookie, i))
continue;
--
2.35.1
[PATCH v1 1/1] nvdimm/namespace: drop unneeded temporary variable in size_store()
Refactor size_store() in order to remove temporary variable on stack
by joining conditionals.
Signed-off-by: Andy Shevchenko
---
drivers/nvdimm/namespace_devs.c | 13 -
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 3dae17c90e8c..0f863fda56e6 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -836,7 +836,6 @@ static ssize_t size_store(struct device *dev,
{
struct nd_region *nd_region = to_nd_region(dev->parent);
unsigned long long val;
- uuid_t **uuid = NULL;
int rc;
rc = kstrtoull(buf, 0, );
@@ -850,16 +849,12 @@ static ssize_t size_store(struct device *dev,
if (rc >= 0)
rc = nd_namespace_label_update(nd_region, dev);
- if (is_namespace_pmem(dev)) {
+ /* setting size zero == 'delete namespace' */
+ if (rc == 0 && val == 0 && is_namespace_pmem(dev)) {
struct nd_namespace_pmem *nspm = to_nd_namespace_pmem(dev);
- uuid = >uuid;
- }
-
- if (rc == 0 && val == 0 && uuid) {
- /* setting size zero == 'delete namespace' */
- kfree(*uuid);
- *uuid = NULL;
+ kfree(nspm->uuid);
+ nspm->uuid = NULL;
}
dev_dbg(dev, "%llx %s (%d)\n", val, rc < 0 ? "fail" : "success", rc);
--
2.35.1
[PATCH v1 1/1] nvdimm/namespace: return uuid_null only once in nd_dev_to_uuid()
Refactor nd_dev_to_uuid() in order to make code shorter and cleaner
by joining conditions and hence returning uuid_null only once.
Signed-off-by: Andy Shevchenko
---
drivers/nvdimm/namespace_devs.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index bf4f5c09d9b1..3dae17c90e8c 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -170,15 +170,12 @@ EXPORT_SYMBOL(nvdimm_namespace_disk_name);
const uuid_t *nd_dev_to_uuid(struct device *dev)
{
- if (!dev)
- return _null;
-
- if (is_namespace_pmem(dev)) {
+ if (dev && is_namespace_pmem(dev)) {
struct nd_namespace_pmem *nspm = to_nd_namespace_pmem(dev);
return nspm->uuid;
- } else
- return _null;
+ }
+ return _null;
}
EXPORT_SYMBOL(nd_dev_to_uuid);
--
2.35.1
Re: [PATCH v1 1/1] ACPI: Switch to use list_entry_is_head() helper
On Wed, Mar 02, 2022 at 05:36:20PM +0100, Rafael J. Wysocki wrote: > On Wed, Mar 2, 2022 at 4:50 PM Andy Shevchenko > wrote: > > On Fri, Feb 11, 2022 at 01:04:23PM +0200, Andy Shevchenko wrote: > > > Since we got list_entry_is_head() helper in the generic header, > > > we may switch the ACPI modules to use it. This eliminates the > > > need in additional variable. In some cases it reduces critical > > > sections as well. > > > > Besides the work required in a couple of cases (LKP) there is an > > ongoing discussion about list loops (and this particular API). > > > > Rafael, what do you think is the best course of action here? > > I think the current approach is to do the opposite of what this patch > is attempting to do: avoid using the list iterator outside of the > loop. OK, let's drop this change. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] ACPI: Switch to use list_entry_is_head() helper
On Fri, Feb 11, 2022 at 01:04:23PM +0200, Andy Shevchenko wrote: > Since we got list_entry_is_head() helper in the generic header, > we may switch the ACPI modules to use it. This eliminates the > need in additional variable. In some cases it reduces critical > sections as well. Besides the work required in a couple of cases (LKP) there is an ongoing discussion about list loops (and this particular API). Rafael, what do you think is the best course of action here? -- With Best Regards, Andy Shevchenko
[PATCH v1 1/1] ACPI: Switch to use list_entry_is_head() helper
Since we got list_entry_is_head() helper in the generic header,
we may switch the ACPI modules to use it. This eliminates the
need in additional variable. In some cases it reduces critical
sections as well.
Signed-off-by: Andy Shevchenko
---
drivers/acpi/acpi_ipmi.c | 16 ++--
drivers/acpi/glue.c | 8 +++-
drivers/acpi/nfit/core.c | 12 +++-
drivers/acpi/nfit/mce.c | 4 +---
drivers/acpi/resource.c | 9 +++--
drivers/acpi/utils.c | 7 ++-
6 files changed, 18 insertions(+), 38 deletions(-)
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
index a5fe2926bf50..f9e56138f8d1 100644
--- a/drivers/acpi/acpi_ipmi.c
+++ b/drivers/acpi/acpi_ipmi.c
@@ -354,27 +354,26 @@ static void ipmi_cancel_tx_msg(struct acpi_ipmi_device
*ipmi,
struct acpi_ipmi_msg *msg)
{
struct acpi_ipmi_msg *tx_msg, *temp;
- bool msg_found = false;
unsigned long flags;
spin_lock_irqsave(>tx_msg_lock, flags);
list_for_each_entry_safe(tx_msg, temp, >tx_msg_list, head) {
if (msg == tx_msg) {
- msg_found = true;
list_del(_msg->head);
break;
}
}
spin_unlock_irqrestore(>tx_msg_lock, flags);
- if (msg_found)
- acpi_ipmi_msg_put(tx_msg);
+ if (list_entry_is_head(tx_msg, >tx_msg_list, head)
+ return;
+
+ acpi_ipmi_msg_put(tx_msg);
}
static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data)
{
struct acpi_ipmi_device *ipmi_device = user_msg_data;
- bool msg_found = false;
struct acpi_ipmi_msg *tx_msg, *temp;
struct device *dev = ipmi_device->dev;
unsigned long flags;
@@ -389,14 +388,13 @@ static void ipmi_msg_handler(struct ipmi_recv_msg *msg,
void *user_msg_data)
spin_lock_irqsave(_device->tx_msg_lock, flags);
list_for_each_entry_safe(tx_msg, temp, _device->tx_msg_list, head)
{
if (msg->msgid == tx_msg->tx_msgid) {
- msg_found = true;
list_del(_msg->head);
break;
}
}
spin_unlock_irqrestore(_device->tx_msg_lock, flags);
- if (!msg_found) {
+ if (list_entry_is_head(tx_msg, _device->tx_msg_list, head)) {
dev_warn(dev,
"Unexpected response (msg id %ld) is returned.\n",
msg->msgid);
@@ -483,13 +481,11 @@ static void ipmi_register_bmc(int iface, struct device
*dev)
static void ipmi_bmc_gone(int iface)
{
struct acpi_ipmi_device *ipmi_device, *temp;
- bool dev_found = false;
mutex_lock(_data.ipmi_lock);
list_for_each_entry_safe(ipmi_device, temp,
_data.ipmi_devices, head) {
if (ipmi_device->ipmi_ifnum != iface) {
- dev_found = true;
__ipmi_dev_kill(ipmi_device);
break;
}
@@ -500,7 +496,7 @@ static void ipmi_bmc_gone(int iface)
struct acpi_ipmi_device, head);
mutex_unlock(_data.ipmi_lock);
- if (dev_found) {
+ if (!list_entry_is_head(ipmi_device, _data.ipmi_devices, head)) {
ipmi_flush_tx_msg(ipmi_device);
acpi_ipmi_dev_put(ipmi_device);
}
diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index ef104809f27b..ffc0b3ee190b 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -61,17 +61,15 @@ EXPORT_SYMBOL_GPL(unregister_acpi_bus_type);
static struct acpi_bus_type *acpi_get_bus_type(struct device *dev)
{
- struct acpi_bus_type *tmp, *ret = NULL;
+ struct acpi_bus_type *tmp;
down_read(_type_sem);
list_for_each_entry(tmp, _type_list, list) {
- if (tmp->match(dev)) {
- ret = tmp;
+ if (tmp->match(dev))
break;
- }
}
up_read(_type_sem);
- return ret;
+ return list_entry_is_head(tmp, _type_list, list) ? NULL : tmp;
}
#define FIND_CHILD_MIN_SCORE 1
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index e5d7f2bda13f..b31c16e5e42c 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1076,8 +1076,8 @@ static void nfit_mem_init_bdw(struct acpi_nfit_desc
*acpi_desc,
static int __nfit_mem_init(struct acpi_nfit_desc *acpi_desc,
struct acpi_nfit_system_address *spa)
{
- struct nfit_mem *nfit_mem, *found;
struct nfit_memdev *nfit_memdev;
+ struct nfit_mem *nfit_mem;
int type = spa ? nfit_spa_type(spa) : 0;
switch (type) {
@@ -1106,19 +1106,13 @@ static int __nfit_mem_init(struct acpi_nfit_desc
*acpi_desc,
[PATCH v1 1/1] ACPI: NFIT: Import GUID before use
Strictly speaking the comparison between guid_t and raw buffer
is not correct. Import GUID to variable of guid_t type and then
compare.
Signed-off-by: Andy Shevchenko
---
drivers/acpi/nfit/core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 7dd80acf92c7..e5d7f2bda13f 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -678,10 +678,12 @@ static const char *spa_type_name(u16 type)
int nfit_spa_type(struct acpi_nfit_system_address *spa)
{
+ guid_t guid;
int i;
+ import_guid(, spa->range_guid);
for (i = 0; i < NFIT_UUID_MAX; i++)
- if (guid_equal(to_nfit_uuid(i), (guid_t *)>range_guid))
+ if (guid_equal(to_nfit_uuid(i), ))
return i;
return -1;
}
--
2.33.0
[PATCH v2 2/2] spi: Avoid undefined behaviour when counting unused native CSs
ffz(), that has been used to count unused native CSs,
might cause undefined behaviour when called against ~0U.
To fix that, open code it with ffs(~value) - 1.
Fixes: 7d93aecdb58d ("spi: Add generic support for unused native cs with
cs-gpios")
Signed-off-by: Andy Shevchenko
---
v2: decoded UB abbreviation (Mark)
drivers/spi/spi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9c3730a9f7d5..01f95bee2ac8 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2609,7 +2609,7 @@ static int spi_get_gpio_descs(struct spi_controller *ctlr)
native_cs_mask |= BIT(i);
}
- ctlr->unused_native_cs = ffz(native_cs_mask);
+ ctlr->unused_native_cs = ffs(~native_cs_mask) - 1;
if ((ctlr->flags & SPI_MASTER_GPIO_SS) && num_cs_gpios &&
ctlr->max_native_cs && ctlr->unused_native_cs >=
ctlr->max_native_cs) {
--
2.30.2
[PATCH v2 1/2] spi: Allow to have all native CSs in use along with GPIOs
The commit 7d93aecdb58d ("spi: Add generic support for unused native cs
with cs-gpios") excludes the valid case for the controllers that doesn't
need to switch native CS in order to perform the transfer, i.e. when
0 native
... ...
- 1 native
GPIO
+ 1 GPIO
... ...
where defines maximum of native CSs supported by the controller.
To allow this, bail out from spi_get_gpio_descs() conditionally for
the controllers which explicitly marked with SPI_MASTER_GPIO_SS.
Fixes: 7d93aecdb58d ("spi: Add generic support for unused native cs with
cs-gpios")
Signed-off-by: Andy Shevchenko
---
v2: no changes
drivers/spi/spi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 36c46feab6d4..9c3730a9f7d5 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2610,8 +2610,9 @@ static int spi_get_gpio_descs(struct spi_controller *ctlr)
}
ctlr->unused_native_cs = ffz(native_cs_mask);
- if (num_cs_gpios && ctlr->max_native_cs &&
- ctlr->unused_native_cs >= ctlr->max_native_cs) {
+
+ if ((ctlr->flags & SPI_MASTER_GPIO_SS) && num_cs_gpios &&
+ ctlr->max_native_cs && ctlr->unused_native_cs >=
ctlr->max_native_cs) {
dev_err(dev, "No unused native chip select available\n");
return -EINVAL;
}
--
2.30.2
[PATCH v2 1/1] spi: Make error handling of gpiod_count() call cleaner
Each time we call spi_get_gpio_descs() the num_chipselect is overwritten
either by new value or by the old one. This is an extra operation in case
gpiod_count() returns an error. Besides that it slashes the error handling
of gpiod_count().
Refactor the code to make error handling of gpiod_count() call cleaner.
Note, that gpiod_count() never returns 0, take this into account as well.
Signed-off-by: Andy Shevchenko
---
v2: reformulated commit message and dropped Fixes tag
drivers/spi/spi.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 74b2b1dd358b..36c46feab6d4 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2558,13 +2558,14 @@ static int spi_get_gpio_descs(struct spi_controller
*ctlr)
unsigned int num_cs_gpios = 0;
nb = gpiod_count(dev, "cs");
- ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect);
-
- /* No GPIOs at all is fine, else return the error */
- if (nb == 0 || nb == -ENOENT)
- return 0;
- else if (nb < 0)
+ if (nb < 0) {
+ /* No GPIOs at all is fine, else return the error */
+ if (nb == -ENOENT)
+ return 0;
return nb;
+ }
+
+ ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect);
cs = devm_kcalloc(dev, ctlr->num_chipselect, sizeof(*cs),
GFP_KERNEL);
--
2.30.2
Re: [PATCH v1 2/2] spi: Avoid potential UB when counting unused native CSs
On Tue, Apr 20, 2021 at 03:56:16PM +0100, Mark Brown wrote: > On Tue, Apr 20, 2021 at 05:10:04PM +0300, Andy Shevchenko wrote: > > ffz(), that has been used to count unused native CSs, might produce UB > > Bit of an IA there... UB -- undefined behaviour. I'll decode it. Should I decode CS as well? -- With Best Regards, Andy Shevchenko
Re: [RESEND v2] iommu/vt-d: Use passthrough mode for the Intel IPUs
On Tue, Apr 20, 2021 at 05:37:27PM +0300, Sakari Ailus wrote: > On Tue, Apr 20, 2021 at 02:55:33PM +0300, Andy Shevchenko wrote: > > On Tue, Apr 20, 2021 at 01:56:40PM +0300, Sakari Ailus wrote: > > > On Tue, Apr 20, 2021 at 06:34:26PM +0800, Bingbu Cao wrote: > > > > On 4/20/21 6:20 PM, Andy Shevchenko wrote: > > > > > On Tue, Apr 20, 2021 at 10:48:33AM +0800, Bingbu Cao wrote: > > > > ... > > > > > > > This misses the changelog from v1 followed by the explanation why > > > > > resent. > > > > > > > > > I noticed there was a typo in the recipient list: > > > > stable.vger.kernel.org -> sta...@vger.kernel.org > > > > > > > > no code change for resent. > > > > > > When you're submitting a patch and want it reach the stable kernels, > > > you'll > > > need to add a Cc tag: > > > > > > Cc: sta...@vger.kernel.org > > > > > > But not actually add the address to cc. I dropped stable@vger address from > > > distribution. > > > > Does it really matter? > > Usually aligning what you're doing with > Documentation/process/submitting-patches.rst is not a bad idea. True, my point is that technically both ways will give the same result, no? -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] spi: Don't overwrite num_chipselect with error code
On Tue, Apr 20, 2021 at 4:19 PM Andy Shevchenko
wrote:
>
> The code currently organized in a way that num_chipselect is overwritten
> each time we call spi_get_gpio_descs(). It might be potentially dangerous
> in case when the gpiod_count() returns an error code.
>
> Note, that gpiod_count() never returns 0, take this into account as well.
>
> Fixes: f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs")
It doesn't fix anything. I missed the max_t(int).
In any case it makes error handling cleaner, so I'll reformulate the
commit message in v2 and drop Fixes tag.
--
With Best Regards,
Andy Shevchenko
[PATCH v1 1/2] spi: Allow to have all native CSs in use along with GPIOs
The commit 7d93aecdb58d ("spi: Add generic support for unused native cs
with cs-gpios") excludes the valid case for the controllers that doesn't
need to switch native CS in order to perform the transfer, i.e. when
0 native
... ...
- 1 native
GPIO
+ 1 GPIO
... ...
where defines maximum of native CSs supported by the controller.
To allow this, bail out from spi_get_gpio_descs() conditionally for
the controllers which explicitly marked with SPI_MASTER_GPIO_SS.
Fixes: 7d93aecdb58d ("spi: Add generic support for unused native cs with
cs-gpios")
Signed-off-by: Andy Shevchenko
---
drivers/spi/spi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 36c46feab6d4..9c3730a9f7d5 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2610,8 +2610,9 @@ static int spi_get_gpio_descs(struct spi_controller *ctlr)
}
ctlr->unused_native_cs = ffz(native_cs_mask);
- if (num_cs_gpios && ctlr->max_native_cs &&
- ctlr->unused_native_cs >= ctlr->max_native_cs) {
+
+ if ((ctlr->flags & SPI_MASTER_GPIO_SS) && num_cs_gpios &&
+ ctlr->max_native_cs && ctlr->unused_native_cs >=
ctlr->max_native_cs) {
dev_err(dev, "No unused native chip select available\n");
return -EINVAL;
}
--
2.30.2
[PATCH v1 2/2] spi: Avoid potential UB when counting unused native CSs
ffz(), that has been used to count unused native CSs, might produce UB
when called against ~0U. To fix that, open code it with ffs(~value) - 1.
Fixes: 7d93aecdb58d ("spi: Add generic support for unused native cs with
cs-gpios")
Signed-off-by: Andy Shevchenko
---
drivers/spi/spi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9c3730a9f7d5..01f95bee2ac8 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2609,7 +2609,7 @@ static int spi_get_gpio_descs(struct spi_controller *ctlr)
native_cs_mask |= BIT(i);
}
- ctlr->unused_native_cs = ffz(native_cs_mask);
+ ctlr->unused_native_cs = ffs(~native_cs_mask) - 1;
if ((ctlr->flags & SPI_MASTER_GPIO_SS) && num_cs_gpios &&
ctlr->max_native_cs && ctlr->unused_native_cs >=
ctlr->max_native_cs) {
--
2.30.2
[PATCH v1 1/1] spi: Don't overwrite num_chipselect with error code
The code currently organized in a way that num_chipselect is overwritten
each time we call spi_get_gpio_descs(). It might be potentially dangerous
in case when the gpiod_count() returns an error code.
Note, that gpiod_count() never returns 0, take this into account as well.
Fixes: f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs")
Signed-off-by: Andy Shevchenko
---
drivers/spi/spi.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 74b2b1dd358b..36c46feab6d4 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2558,13 +2558,14 @@ static int spi_get_gpio_descs(struct spi_controller
*ctlr)
unsigned int num_cs_gpios = 0;
nb = gpiod_count(dev, "cs");
- ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect);
-
- /* No GPIOs at all is fine, else return the error */
- if (nb == 0 || nb == -ENOENT)
- return 0;
- else if (nb < 0)
+ if (nb < 0) {
+ /* No GPIOs at all is fine, else return the error */
+ if (nb == -ENOENT)
+ return 0;
return nb;
+ }
+
+ ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect);
cs = devm_kcalloc(dev, ctlr->num_chipselect, sizeof(*cs),
GFP_KERNEL);
--
2.30.2
[PATCH v1 1/1] spi: Rename enable1 to activate in spi_set_cs()
The enable1 is confusing name. Change it to clearly show what is
the intention behind it. No functional changes.
Fixes: 25093bdeb6bc ("spi: implement SW control for CS times")
Signed-off-by: Andy Shevchenko
---
drivers/spi/spi.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index b08efe88ccd6..74b2b1dd358b 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -797,7 +797,7 @@ int spi_register_board_info(struct spi_board_info const
*info, unsigned n)
static void spi_set_cs(struct spi_device *spi, bool enable)
{
- bool enable1 = enable;
+ bool activate = enable;
/*
* Avoid calling into the driver (or doing delays) if the chip select
@@ -812,7 +812,7 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio) ||
!spi->controller->set_cs_timing) {
- if (enable1)
+ if (activate)
spi_delay_exec(>controller->cs_setup, NULL);
else
spi_delay_exec(>controller->cs_hold, NULL);
@@ -825,8 +825,7 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
if (!(spi->mode & SPI_NO_CS)) {
if (spi->cs_gpiod)
/* polarity handled by gpiolib */
- gpiod_set_value_cansleep(spi->cs_gpiod,
-enable1);
+ gpiod_set_value_cansleep(spi->cs_gpiod,
activate);
else
/*
* invert the enable line, as active low is
@@ -844,7 +843,7 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio) ||
!spi->controller->set_cs_timing) {
- if (!enable1)
+ if (!activate)
spi_delay_exec(>controller->cs_inactive, NULL);
}
}
--
2.30.2
Re: linux-next: Tree for Apr 20
On Tue, Apr 20, 2021 at 03:19:39PM +0300, Andy Shevchenko wrote: > On Tue, Apr 20, 2021 at 03:02:51PM +0300, Andy Shevchenko wrote: > > On Tue, Apr 20, 2021 at 07:47:18PM +1000, Stephen Rothwell wrote: ... > > I have full of build warnings / errors in x86 and iommu Found the culprit -- it was uncleaned stuff from the other build in the source tree. So, it was only me who experienced that :-) -- With Best Regards, Andy Shevchenko
Re: linux-next: Tree for Apr 20
On Tue, Apr 20, 2021 at 03:02:51PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 20, 2021 at 07:47:18PM +1000, Stephen Rothwell wrote:
> > Hi all,
> >
> > Changes since 20210419:
> >
> > The powerpc tree lost its build failure.
> >
> > The ftrace tree gained a conflict against the bpf-next tree.
> >
> > Non-merge commits (relative to Linus' tree): 12917
> > 11294 files changed, 619161 insertions(+), 276245 deletions(-)
> >
> >
> >
> > I have created today's linux-next tree at
> > git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> > (patches at http://www.kernel.org/pub/linux/kernel/next/ ). If you
> > are tracking the linux-next tree using git, you should not use "git pull"
> > to do so as that will try to merge the new linux-next release with the
> > old one. You should use "git fetch" and checkout or reset to the new
> > master.
> >
> > You can see which trees have been included by looking in the Next/Trees
> > file in the source. There are also quilt-import.log and merge.log
> > files in the Next directory. Between each merge, the tree was built
> > with a ppc64_defconfig for powerpc, an allmodconfig for x86_64, a
> > multi_v7_defconfig for arm and a native build of tools/perf. After
> > the final fixups (if any), I do an x86_64 modules_install followed by
> > builds for x86_64 allnoconfig, powerpc allnoconfig (32 and 64 bit),
> > ppc44x_defconfig, allyesconfig and pseries_le_defconfig and i386, sparc
> > and sparc64 defconfig and htmldocs. And finally, a simple boot test
> > of the powerpc pseries_le_defconfig kernel in qemu (with and without
> > kvm enabled).
> >
> > Below is a summary of the state of the merge.
> >
> > I am currently merging 340 trees (counting Linus' and 89 trees of bug
> > fix patches pending for the current merge release).
> >
> > Stats about the size of the tree over time can be seen at
> > http://neuling.org/linux-next-size.html .
> >
> > Status of my local build tests will be at
> > http://kisskb.ellerman.id.au/linux-next . If maintainers want to give
> > advice about cross compilers/configs that work, we are always open to add
> > more builds.
> >
> > Thanks to Randy Dunlap for doing many randconfig builds. And to Paul
> > Gortmaker for triage and bug fixes.
>
> I have full of build warnings / errors in x86 and iommu
>
> X86:
>
> arch/x86/include/asm/string_64.h:14:14: warning: conflicting types for
> built-in function ‘memcpy’; expected ‘void *(void *, const void *, long
> unsigned int)’ [-Wbuiltin-declaration-mismatch]
>14 | extern void *memcpy(void *to, const void *from, size_t len);
> | ^~
> arch/x86/include/asm/string_64.h:7:1: note: ‘memcpy’ is declared in header
> ‘’
> 6 | #include
> +++ |+#include
>
> And so on for standard string function definitions.
>
> IOMMU:
>
> drivers/iommu/amd/io_pgtable.c: In function ‘v1_alloc_pgtable’:
> drivers/iommu/amd/io_pgtable.c:551:32: error: assignment to ‘size_t
> (*)(struct io_pgtable_ops *, long unsigned int, size_t, struct
> iommu_iotlb_gather *)’ {aka ‘unsigned int (*)(struct io_pgtable_ops *, long
> unsigned int, unsigned int, struct iommu_iotlb_gather *)’} from
> incompatible pointer type ‘long unsigned int (*)(struct io_pgtable_ops *,
> long unsigned int, size_t, struct iommu_iotlb_gather *)’ {aka ‘long
> unsigned int (*)(struct io_pgtable_ops *, long unsigned int, unsigned int,
> struct iommu_iotlb_gather *)’} [-Werror=incompatible-pointer-types]
> 551 | pgtable->iop.ops.unmap= iommu_v1_unmap_page;
> |^
> cc1: some warnings being treated as errors
>
> Is it only me?
Okay, there is another bug and it seems compiler related:
net/socket.c:2320:3: note: in expansion of macro ‘BUILD_BUG_ON’
2320 | BUILD_BUG_ON(sizeof(struct cmsghdr) !=
| ^~~~
% gcc --version
gcc (Debian 10.2.1-6) 10.2.1 20210110
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
On Tue, Apr 20, 2021 at 2:36 PM Tomas Melin wrote:
> On 4/20/21 1:47 PM, Andy Shevchenko wrote:
> > On Tue, Apr 20, 2021 at 11:50 AM Tomas Melin
> > wrote:
...
> >>>> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> >>>> +indio_dev->masklength) {
> >>>> + ret = sca3300_read_reg(data,
> >>>> sca3300_channels[bit].address,
> >>>> + );
> >>>> + if (ret) {
> >>>> + dev_err(>spi->dev,
> >>>> + "failed to read register, error: %d\n",
> >>>> ret);
> >>>> + goto out;
> >>> Does it mean interrupt is handled in this case?
> >>> Perhaps a comment why it's okay to consider so?
> >> IRQ_HANDLED seemed more correct than IRQ_NONE.
> > Why? Care to explain?
>
> Thinking that IRQ was for the device and it was indeed handled. There
> were errors when handling
>
> it, but it was handled as much as possible.
>
> >
> >> Or did You have some
> >> other option in mind?
> >>
> >> How about something like:
> >>
> >> /* handled with errors */
> > But what if this is the very first interrupt (bit in the loop) that
> > failed? What about the rest?
>
> Aah, right. Other option could be to simply continue loop and set 'val'
> to e.g. 0 for
>
> readings with errors. But perhaps it is after all better to bail out,
> and only for cases
>
> when _all_ data is reliable, it is pushed to buffers(?)
>
> Comes to mind that perhaps better to have error message in this irq
> handler as
>
> dev_err_ratelimited(), to avoid possible flooding.
>
>
> So to conclude, proposing:
>
> *change to dev_err_ratelimited()
>
> * comment goto:
>
> /* handled, but bailing out this round due to errors */
>
> Would this be OK?
Sounds like a plan!
> >> goto out;
> >>
> >>>> + }
> >>>> + data->scan.channels[i++] = val;
> >>>> + }
--
With Best Regards,
Andy Shevchenko
Re: linux-next: Tree for Apr 20
On Tue, Apr 20, 2021 at 07:47:18PM +1000, Stephen Rothwell wrote:
> Hi all,
>
> Changes since 20210419:
>
> The powerpc tree lost its build failure.
>
> The ftrace tree gained a conflict against the bpf-next tree.
>
> Non-merge commits (relative to Linus' tree): 12917
> 11294 files changed, 619161 insertions(+), 276245 deletions(-)
>
>
>
> I have created today's linux-next tree at
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> (patches at http://www.kernel.org/pub/linux/kernel/next/ ). If you
> are tracking the linux-next tree using git, you should not use "git pull"
> to do so as that will try to merge the new linux-next release with the
> old one. You should use "git fetch" and checkout or reset to the new
> master.
>
> You can see which trees have been included by looking in the Next/Trees
> file in the source. There are also quilt-import.log and merge.log
> files in the Next directory. Between each merge, the tree was built
> with a ppc64_defconfig for powerpc, an allmodconfig for x86_64, a
> multi_v7_defconfig for arm and a native build of tools/perf. After
> the final fixups (if any), I do an x86_64 modules_install followed by
> builds for x86_64 allnoconfig, powerpc allnoconfig (32 and 64 bit),
> ppc44x_defconfig, allyesconfig and pseries_le_defconfig and i386, sparc
> and sparc64 defconfig and htmldocs. And finally, a simple boot test
> of the powerpc pseries_le_defconfig kernel in qemu (with and without
> kvm enabled).
>
> Below is a summary of the state of the merge.
>
> I am currently merging 340 trees (counting Linus' and 89 trees of bug
> fix patches pending for the current merge release).
>
> Stats about the size of the tree over time can be seen at
> http://neuling.org/linux-next-size.html .
>
> Status of my local build tests will be at
> http://kisskb.ellerman.id.au/linux-next . If maintainers want to give
> advice about cross compilers/configs that work, we are always open to add
> more builds.
>
> Thanks to Randy Dunlap for doing many randconfig builds. And to Paul
> Gortmaker for triage and bug fixes.
I have full of build warnings / errors in x86 and iommu
X86:
arch/x86/include/asm/string_64.h:14:14: warning: conflicting types for built-in
function ‘memcpy’; expected ‘void *(void *, const void *, long unsigned int)’
[-Wbuiltin-declaration-mismatch]
14 | extern void *memcpy(void *to, const void *from, size_t len);
| ^~
arch/x86/include/asm/string_64.h:7:1: note: ‘memcpy’ is declared in header
‘’
6 | #include
+++ |+#include
And so on for standard string function definitions.
IOMMU:
drivers/iommu/amd/io_pgtable.c: In function ‘v1_alloc_pgtable’:
drivers/iommu/amd/io_pgtable.c:551:32: error: assignment to ‘size_t (*)(struct
io_pgtable_ops *, long unsigned int, size_t, struct iommu_iotlb_gather *)’
{aka ‘unsigned int (*)(struct io_pgtable_ops *, long unsigned int, unsigned
int, struct iommu_iotlb_gather *)’} from incompatible pointer type ‘long
unsigned int (*)(struct io_pgtable_ops *, long unsigned int, size_t, struct
iommu_iotlb_gather *)’ {aka ‘long unsigned int (*)(struct io_pgtable_ops *,
long unsigned int, unsigned int, struct iommu_iotlb_gather *)’}
[-Werror=incompatible-pointer-types]
551 | pgtable->iop.ops.unmap= iommu_v1_unmap_page;
|^
cc1: some warnings being treated as errors
Is it only me?
--
With Best Regards,
Andy Shevchenko
Re: [RESEND v2] iommu/vt-d: Use passthrough mode for the Intel IPUs
On Tue, Apr 20, 2021 at 01:56:40PM +0300, Sakari Ailus wrote: > On Tue, Apr 20, 2021 at 06:34:26PM +0800, Bingbu Cao wrote: > > On 4/20/21 6:20 PM, Andy Shevchenko wrote: > > > On Tue, Apr 20, 2021 at 10:48:33AM +0800, Bingbu Cao wrote: ... > > > This misses the changelog from v1 followed by the explanation why resent. > > > > > I noticed there was a typo in the recipient list: > > stable.vger.kernel.org -> sta...@vger.kernel.org > > > > no code change for resent. > > When you're submitting a patch and want it reach the stable kernels, you'll > need to add a Cc tag: > > Cc: sta...@vger.kernel.org > > But not actually add the address to cc. I dropped stable@vger address from > distribution. Does it really matter? -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
On Tue, Apr 20, 2021 at 11:50 AM Tomas Melin wrote:
> On 4/19/21 4:55 PM, Andy Shevchenko wrote:
> > On Mon, Apr 19, 2021 at 4:26 PM Tomas Melin wrote:
...
> >> +#define SCA3300_MASK_STATUSGENMASK(8, 0)
> >> +#define SCA3300_MASK_RS_STATUS GENMASK(1, 0)
> > This feels like an orphan. Shouldn't you move it closer to the group
> > of corresponding register / etc definition?
>
> Tried to group these in alphabetical order, but IIUC preference would be
> towards grouping
Yes, alphabetical is about header block, and definition should be
understandable and HW represented.
> according to how they are used? Would this be clearer and acceptable?
1) with some amendments, see below.
> 1)
>
> /* Device mode register */
> #define SCA3300_REG_MODE0xd
> #define SCA3300_VALUE_SW_RESET0x20
SCA3300_MODE_SW_RESET
> /* Last register in map */
> #define SCA3300_REG_SELBANK0x1f
>
> /* Device status and related mask */
> #define SCA3300_REG_STATUS0x6
> #define SCA3300_MASK_STATUSGENMASK(8, 0)
SCA3300_STATUS_MASK
and so on (I guess you got the pattern)
> /* Device ID */
> #define SCA3300_REG_WHOAMI0x10
> #define SCA3300_VALUE_DEVICE_ID0x51
>
> /* Device return status and mask */
> #define SCA3300_VALUE_RS_ERROR0x3
> #define SCA3300_MASK_RS_STATUSGENMASK(1, 0)
...
> >> + * @txbuf: Transmit buffer
> >> + * @rxbuf: Receive buffer
> > Are the buffers subject to DMA? Shouldn't they have the proper alignment?
> Good point, I will add alignment.
Move them to the end of the structure to save few bytes,
...
> >> + sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);
> > Seems you ignored my comment. What is this 0x0? What is the meaning of it?
> > Same for all the rest magic numbers in the code.
>
> Sorry, not ignored but will remove this redundant 0x0 for next round.
Maybe it's not redundant after all (I noticed other magic numbers in
the same position)? Please, comment your intention case-by-case.
...
> >> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> >> +indio_dev->masklength) {
> >> + ret = sca3300_read_reg(data, sca3300_channels[bit].address,
> >> + );
> >> + if (ret) {
> >> + dev_err(>spi->dev,
> >> + "failed to read register, error: %d\n",
> >> ret);
> >> + goto out;
> > Does it mean interrupt is handled in this case?
> > Perhaps a comment why it's okay to consider so?
>
> IRQ_HANDLED seemed more correct than IRQ_NONE.
Why? Care to explain?
> Or did You have some
> other option in mind?
>
> How about something like:
>
> /* handled with errors */
But what if this is the very first interrupt (bit in the loop) that
failed? What about the rest?
> goto out;
>
> >> + }
> >> + data->scan.channels[i++] = val;
> >> + }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] optee: use export_uuid() to copy client UUID
On Tue, Apr 20, 2021 at 10:25:47AM +0200, Jens Wiklander wrote:
> Prior to this patch optee_open_session() was making assumptions about
> the internal format of uuid_t by casting a memory location in a
> parameter struct to uuid_t *. Fix this using export_uuid() to get a well
> defined binary representation and also add an octets field in struct
> optee_msg_param in order to avoid casting.
Wonderful! Thanks for fixing this!
Reviewed-by: Andy Shevchenko
A bit of off-topic, have you know by any chance who may consider applying this
one?
https://lore.kernel.org/linux-mips/20210121183741.45333-1-andriy.shevche...@linux.intel.com/
> Fixes: c5b4312bea5d ("tee: optee: Add support for session login client UUID
> generation")
> Suggested-by: Andy Shevchenko
> Signed-off-by: Jens Wiklander
> ---
> drivers/tee/optee/call.c | 6 --
> drivers/tee/optee/optee_msg.h | 6 --
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> index 7a77e375b503..6b52f0c526ba 100644
> --- a/drivers/tee/optee/call.c
> +++ b/drivers/tee/optee/call.c
> @@ -216,6 +216,7 @@ int optee_open_session(struct tee_context *ctx,
> struct optee_msg_arg *msg_arg;
> phys_addr_t msg_parg;
> struct optee_session *sess = NULL;
> + uuid_t client_uuid;
>
> /* +2 for the meta parameters added below */
> shm = get_msg_arg(ctx, arg->num_params + 2, _arg, _parg);
> @@ -236,10 +237,11 @@ int optee_open_session(struct tee_context *ctx,
> memcpy(_arg->params[0].u.value, arg->uuid, sizeof(arg->uuid));
> msg_arg->params[1].u.value.c = arg->clnt_login;
>
> - rc = tee_session_calc_client_uuid((uuid_t *)_arg->params[1].u.value,
> - arg->clnt_login, arg->clnt_uuid);
> + rc = tee_session_calc_client_uuid(_uuid, arg->clnt_login,
> + arg->clnt_uuid);
> if (rc)
> goto out;
> + export_uuid(msg_arg->params[1].u.octets, _uuid);
>
> rc = optee_to_msg_param(msg_arg->params + 2, arg->num_params, param);
> if (rc)
> diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> index 81ff593ac4ec..e3d72d09c484 100644
> --- a/drivers/tee/optee/optee_msg.h
> +++ b/drivers/tee/optee/optee_msg.h
> @@ -9,7 +9,7 @@
> #include
>
> /*
> - * This file defines the OP-TEE message protocol used to communicate
> + * This file defines the OP-TEE message protocol (ABI) used to communicate
> * with an instance of OP-TEE running in secure world.
> *
> * This file is divided into two sections.
> @@ -144,9 +144,10 @@ struct optee_msg_param_value {
> * @tmem:parameter by temporary memory reference
> * @rmem:parameter by registered memory reference
> * @value: parameter by opaque value
> + * @octets: parameter by octet string
> *
> * @attr & OPTEE_MSG_ATTR_TYPE_MASK indicates if tmem, rmem or value is used
> in
> - * the union. OPTEE_MSG_ATTR_TYPE_VALUE_* indicates value,
> + * the union. OPTEE_MSG_ATTR_TYPE_VALUE_* indicates value or octets,
> * OPTEE_MSG_ATTR_TYPE_TMEM_* indicates @tmem and
> * OPTEE_MSG_ATTR_TYPE_RMEM_* indicates @rmem,
> * OPTEE_MSG_ATTR_TYPE_NONE indicates that none of the members are used.
> @@ -157,6 +158,7 @@ struct optee_msg_param {
> struct optee_msg_param_tmem tmem;
> struct optee_msg_param_rmem rmem;
> struct optee_msg_param_value value;
> + u8 octets[24];
> } u;
> };
>
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
Re: [RESEND v2] iommu/vt-d: Use passthrough mode for the Intel IPUs
On Tue, Apr 20, 2021 at 10:48:33AM +0800, Bingbu Cao wrote: > Intel IPU(Image Processing Unit) has its own (IO)MMU hardware, > The IPU driver allocates its own page table that is not mapped > via the DMA, and thus the Intel IOMMU driver blocks access giving > this error: > > DMAR: DRHD: handling fault status reg 3 > DMAR: [DMA Read] Request device [00:05.0] PASID > fault addr 76406000 [fault reason 06] PTE Read access is not set > > As IPU is not an external facing device which is not risky, so use > IOMMU passthrough mode for Intel IPUs. I'm wondering if IPU MMU should be described properly in the DMAR table. -- With Best Regards, Andy Shevchenko
Re: [RESEND v2] iommu/vt-d: Use passthrough mode for the Intel IPUs
On Tue, Apr 20, 2021 at 10:48:33AM +0800, Bingbu Cao wrote:
> Intel IPU(Image Processing Unit) has its own (IO)MMU hardware,
> The IPU driver allocates its own page table that is not mapped
> via the DMA, and thus the Intel IOMMU driver blocks access giving
> this error:
>
> DMAR: DRHD: handling fault status reg 3
> DMAR: [DMA Read] Request device [00:05.0] PASID
> fault addr 76406000 [fault reason 06] PTE Read access is not set
>
> As IPU is not an external facing device which is not risky, so use
> IOMMU passthrough mode for Intel IPUs.
>
> Fixes: 26f5689592e2 ("media: staging/intel-ipu3: mmu: Implement driver")
> Signed-off-by: Bingbu Cao
> ---
> drivers/iommu/intel/iommu.c | 29 +
This misses the changelog from v1 followed by the explanation why resent.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 0/2] bitmap_parselist: support 'all' semantics
On Mon, Apr 19, 2021 at 05:01:29PM -0700, Yury Norov wrote: > RCU code supports a special group 'all' which selects all bits in a bitmap. > We have recently added 'N' extension for bitmap parse, so that '0-N' would > have exactly the same meaning as 'all'. But because the 'all' is already > used by RCU, it would be reasonable to support it in core bitmap code as a > common and easy-readable alias for '0-N'. > > Moving the 'all' support to core bitmap code adds another level of > flexibility for system configuration by supporting patterns. For example, > every second bit in cpumask may be selected like this: > isolcpus=all:1/2 After addressing a couple of nit-picks, Reviewed-by: Andy Shevchenko > Yury Norov (2): > bitmap_parse: support 'all' semantics > rcu/tree_plugin: don't handle the case of 'all' CPU range > > Documentation/admin-guide/kernel-parameters.rst | 5 + > kernel/rcu/tree_plugin.h| 9 +++-- > lib/bitmap.c| 9 + > lib/test_bitmap.c | 8 +++- > 4 files changed, 24 insertions(+), 7 deletions(-) > > -- > 2.25.1 > -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/2] rcu/tree_plugin: don't handle the case of 'all' CPU range
On Mon, Apr 19, 2021 at 05:01:31PM -0700, Yury Norov wrote: > The 'all' semantics is now supported by the bitmap_parselist() so we can > drop supporting it as a special case in RCU code. This patch does not > add any functional changes for existing users. > - if (!strcasecmp(str, "all"))/* legacy: use "0-N" instead */ Perhaps move comment as well to new location. -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/2] bitmap_parse: support 'all' semantics
On Mon, Apr 19, 2021 at 05:01:30PM -0700, Yury Norov wrote:
> RCU code supports an 'all' group as a special case when parsing
> rcu_nocbs parameter. This patch moves the 'all' support to the core
> bitmap_parse code, so that all bitmap users can enjoy this extension.
>
> Moving 'all' parsing to a bitmap_parse level, also allows users to
> pass patterns together with 'all' in regular group:pattern format
...
> {0, "0-31:1/3,1-31:1/3,2-31:1/3", [8 * step], 32, 0},
> {0, "1-10:8/12,8-31:24/29,0-31:0/3",[9 * step], 32, 0},
>
> + {0, "all",[8 * step], 32, 0},
> + {0, "0, 1, all, ", [8 * step], 32, 0},
> + {0, "all:1/2",[4 * step], 32, 0},
> + {0, "ALL:1/2",[4 * step], 32, 0},
> + {-EINVAL, "al", NULL, 8, 0},
> + {-EINVAL, "alll", NULL, 8, 0},
> +
Looking at the below hunk it seems like the two above should be actually placed
there.
> {-EINVAL, "-1", NULL, 8, 0},
> {-EINVAL, "-0", NULL, 8, 0},
> {-EINVAL, "10-1", NULL, 8, 0},
> @@ -384,7 +391,6 @@ static const struct test_bitmap_parselist
> parselist_tests[] __initconst = {
> {-EINVAL, "a-31:10/1", NULL, 8, 0},
> {-EINVAL, "0-31:a/1", NULL, 8, 0},
> {-EINVAL, "0-\n", NULL, 8, 0},
> -
Otherwise this change doesn't belong to the series.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/1] pinctrl: core: Show pin numbers for the controllers with base = 0
On Tue, Apr 20, 2021 at 12:32:18AM -0700, Drew Fustini wrote:
> On Thu, Apr 15, 2021 at 04:03:56PM +0300, Andy Shevchenko wrote:
> > The commit f1b206cf7c57 ("pinctrl: core: print gpio in pins debugfs file")
> > enabled GPIO pin number and label in debugfs for pin controller. However,
> > it limited that feature to the chips where base is positive number. This,
> > in particular, excluded chips where base is 0 for the historical or backward
> > compatibility reasons. Refactor the code to include the latter as well.
...
> > - chip = gpio_to_chip(gpio_num);
> > - if (chip && chip->gpiodev && chip->gpiodev->base)
> > - seq_printf(s, "%u:%s ", gpio_num -
> > - chip->gpiodev->base, chip->label);
> > + if (gpio_num >= 0)
> > + chip = gpio_to_chip(gpio_num);
> > + else
> > + chip = NULL;
> > + if (chip)
> > + seq_printf(s, "%u:%s ", gpio_num - chip->gpiodev->base,
> > chip->label);
> > else
> > seq_puts(s, "0:? ");
> Thank you, this makes sense to me. I had failed to consider what would
> happen when chip->gpiodev->base == 0.
If gpiodev->base == 0 it can happen only when
1) either base is 0 by the driver request
2) or it's a GPIO device which fits the (last) free slot in the number space
It can't be negative at all. So, it means whatever value is there it is always
valid.
> I have tested on the BeagleBone
> (AM3358) and the output works as expected.
Cool!
> /sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single# more pins
> registered pins: 142
> pin 0 (PIN0) 0:gpio-0-31 44e10800 0027 pinctrl-single
> pin 1 (PIN1) 1:gpio-0-31 44e10804 0027 pinctrl-single
> pin 2 (PIN2) 2:gpio-0-31 44e10808 0027 pinctrl-single
> pin 3 (PIN3) 3:gpio-0-31 44e1080c 0027 pinctrl-single
> pin 4 (PIN4) 4:gpio-0-31 44e10810 0027 pinctrl-single
> pin 5 (PIN5) 5:gpio-0-31 44e10814 0027 pinctrl-single
> pin 6 (PIN6) 6:gpio-0-31 44e10818 0027 pinctrl-single
> pin 7 (PIN7) 7:gpio-0-31 44e1081c 0027 pinctrl-single
> pin 8 (PIN8) 22:gpio-96-127 44e10820 0027 pinctrl-single
> pin 9 (PIN9) 23:gpio-96-127 44e10824 0037 pinctrl-single
> pin 10 (PIN10) 26:gpio-96-127 44e10828 0037 pinctrl-single
> pin 11 (PIN11) 27:gpio-96-127 44e1082c 0037 pinctrl-single
> pin 12 (PIN12) 12:gpio-0-31 44e10830 0037 pinctrl-single
> pin 13 (PIN13) 13:gpio-0-31 44e10834 0037 pinctrl-single
> pin 14 (PIN14) 14:gpio-0-31 44e10838 0037 pinctrl-single
> pin 15 (PIN15) 15:gpio-0-31 44e1083c 0037 pinctrl-single
> pin 16 (PIN16) 16:gpio-0-31 44e10840 0027 pinctrl-single
>
>
> Tested-by: Drew Fustini
> Reviewed-by: Drew Fustini
Thank you!
Linus, can it be applied now?
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/1] tee: optee: Provide special parameter field for UUID values
On Mon, Apr 19, 2021 at 5:30 PM Jens Wiklander
wrote:
> On Mon, Apr 19, 2021 at 3:40 PM Andy Shevchenko
> wrote:
> > On Mon, Apr 19, 2021 at 4:30 PM Jens Wiklander
> > wrote:
> > > On Mon, Apr 19, 2021 at 2:01 PM Andy Shevchenko
> > > wrote:
> > > > On Mon, Apr 19, 2021 at 01:35:51PM +0200, Jens Wiklander wrote:
> > > > > On Thu, Apr 15, 2021 at 4:58 PM Andy Shevchenko
> > > > > wrote:
> > > >
> > > > Thanks for review, my answer below.
> > > >
> > > > > > struct optee_msg_param_tmem tmem;
> > > > > > struct optee_msg_param_rmem rmem;
> > > > > > struct optee_msg_param_value value;
> > > > > > + uuid_t uuid;
> > > > >
> > > > > It's nice to get rid of the cast above, but I'm not that keen on the
> > > > > change in this struct. This file defines the ABI towards Secure world
> > > > > and adding dependencies on external complex types is a larger problem
> > > > > than the cast above in my opinion.
> > > >
> > > > I understand.
> > > >
> > > > So, the cast is simply wrong there. Can you add a comment above that
> > > > cast to
> > > > explain that and make it is marked as FIXME? Because there is no
> > > > guarantee that
> > > > internal Linux types can be 1:1 mapped to the ABI of something.
> > >
> > > We might as well fix it directly instead. How about storing the
> > > intermediate result in a proper uuid_t and then export it as:
> > > export_uuid((u8 *)_arg->params[1].u.uuid, );
> >
> > Still a casting here.
> > With u64 members you have a (potential) endianness issue (consider
> > BE-32 platform). Also you never know that a b c translates properly to
> > byte array.
> >
> > I would rather see a custom function
> >
> > optee_import_uuid(param, uuid_t *uuid)
> > {
> > u8 uuid_raw[UUID_SIZE];
> >
> > put_unaligned_le64(_raw[0], param.a); // not sure about endianness
> > put_unaligned_le64(_raw[0], param.b); // ditto
>
> I believe it's a memcpy() we want then, since UUIDs are supposed to be
> transmitted using a big endian memory pattern.
> We should perhaps add
> u8 octets[24];
> to that union. Then should the result be well defined using export_uuid().
Right, if you do that, it would be wonderful!
> > import_uuid();
> > }
> >
> > > > What you need, perhaps, is a middle layer function that will copy u64
> > > > data
> > > > to uuid_t or so. Also, u64 is not an ABI type, why the respective __uXX
> > > > variants are not in use?
> > >
> > > Does it make any difference? The file isn't shared with user space and
> > > I need to sync the file manually anyway since OP-TEE doesn't have the
> > > same include files.
> >
> > Yes. It gives a hint that these are ABI (that's why I felt free to add
> > a member to the union. I have no idea that's an ABI). Optionally a
> > comment suggesting that.
>
> It does say that it defines a protocol at the beginning of the file, I
> can add ABI too if you think that helps.
I read the structure definition, perhaps some clarification on a data
type level would be nice.
Thanks!
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] i2c: I2C_HISI should depend on ARCH_HISI && ACPI
On Mon, Apr 19, 2021 at 5:15 PM Geert Uytterhoeven wrote: > On Mon, Apr 19, 2021 at 3:58 PM Andy Shevchenko > wrote: > Please tell me how this driver will be probed when CONFIG_ACPI > is disabled (it cannot, as nothing instantiates platform devices of the > right type, so there is no reason to bother the user with a question about > this driver when configuring his kernel). Go ahead with it in v2. I'll not block you. -- With Best Regards, Andy Shevchenko
Re: [PATCH] i2c: I2C_HISI should depend on ARCH_HISI && ACPI
On Mon, Apr 19, 2021 at 5:18 PM Geert Uytterhoeven wrote: > On Mon, Apr 19, 2021 at 4:14 PM Andy Shevchenko > wrote: > > On Mon, Apr 19, 2021 at 4:58 PM Andy Shevchenko > > wrote: > > > On Mon, Apr 19, 2021 at 4:54 PM Geert Uytterhoeven > > > wrote: > > > > > > > In any case it's not true. We have the platform drivers w/o legacy > > > > > users that are not dependent on OF. > > > > > > > > Example? ;-) > > > > > > i2c-owl.c > > > > In case you want more > > sound/sparc/amd7930.c > > SND_SUN_AMD7930 depends on SND_SPARC && SBUS > SND_SPARC depends on SPARC > SPARC selects OF > > Hence, SND_SUN_AMD7930 depends on OF. Exactly my point. Read back what I wrote. TL;DR: It's *fine* to have _indirect_ dependency like this. -- With Best Regards, Andy Shevchenko
Re: [PATCH] i2c: I2C_HISI should depend on ARCH_HISI && ACPI
On Mon, Apr 19, 2021 at 4:58 PM Andy Shevchenko wrote: > On Mon, Apr 19, 2021 at 4:54 PM Geert Uytterhoeven > wrote: > > > In any case it's not true. We have the platform drivers w/o legacy > > > users that are not dependent on OF. > > > > Example? ;-) > > i2c-owl.c In case you want more sound/sparc/amd7930.c And I believe I can find zillions of them. -- With Best Regards, Andy Shevchenko
Re: [PATCH] i2c: I2C_HISI should depend on ARCH_HISI && ACPI
On Mon, Apr 19, 2021 at 4:54 PM Geert Uytterhoeven wrote: > On Mon, Apr 19, 2021 at 3:35 PM Andy Shevchenko > wrote: > > On Mon, Apr 19, 2021 at 4:02 PM Geert Uytterhoeven > > wrote: > > > On Thu, Apr 15, 2021 at 10:50 AM Andy Shevchenko > > > wrote: > > > > On Thu, Apr 15, 2021 at 3:43 AM Geert Uytterhoeven > > > > wrote: > > > > > On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko > > > > > wrote: > > > > > > On Wed, Apr 14, 2021 at 08:55:21PM +0200, Geert Uytterhoeven wrote: > > > > > > > On Wed, Apr 14, 2021 at 8:18 PM Andy Shevchenko > > > > > > > wrote: > > > > > > > > On Wed, Apr 14, 2021 at 08:06:18PM +0200, Geert Uytterhoeven > > > > > > > > wrote: > > > > > > > > > On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang > > > > > > > > > wrote: > > > > ... > > > > > > > > > > > I guess it's still fine to add a dependency on ACPI? > > > > > > > > > > > > > > > > But why? > > > > > > > > > > > > > > Please tell me how/when the driver is used when CONFIG_ACPI=n. > > > > > > > > > > > > I'm not using it at all. Ask the author :-) > > > > > > > > > > > > But if we follow your logic, then we need to mark all the > > > > > > _platform_ drivers > > > > > > for x86 world as ACPI dependent? This sounds ugly. > > > > > > > > > > Do all other x86 platform drivers have (1) an .acpi_match_table[] and > > > > > (2) no other way of instantiating their devices? > > > > > The first driver from the top of my memory I looked at is rtc-cmos: > > > > > it has no .acpi_match_table[], and the rtc-cmos device is instantiated > > > > > from arch/x86/kernel/rtc.c. > > > > > > > > > > For drivers with only an .of_match_table(), and no legacy users > > > > > instantiating platform devices, we do have dependencies on OF. > > > > > > > > This is not true. Entire IIO subsystem is an example. > > > > > > Do you care to elaborate? > > > Three quarters of the IIO drivers are I2C and SPI drivers, and thus not > > > subject to the above. > > > > It seems I missed that you are talking about platform device drivers. > > OK. > > > In any case it's not true. We have the platform drivers w/o legacy > > users that are not dependent on OF. > > Example? ;-) i2c-owl.c > > They may _indirectly_ be dependent, but this is fine as I stated above > > when suggested to move ACPI dependency on ARCH_xxx level. > > As per the response from the driver maintainer > https://lore.kernel.org/linux-arm-kernel/bd8db435-24e1-5ab3-6b35-1d4d8a292...@hisilicon.com/, > there is no dependency on ARCH_HISI, so moving the ACPI dependency > up won't help. So, an ACPI dependency is simply not applicable here as it's a compile dependency as well, which is not a limitation for this driver. Again, talk to Masahiro how to handle this, but I don't see any good justification to have ACPI (compile time) dependency here. So, again NAK! -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
On Mon, Apr 19, 2021 at 4:26 PM Tomas Melin wrote:
Thanks for an update, it's getting better! My comments below.
> Add initial support for Murata SCA3300 3-axis industrial
> accelerometer with digital SPI interface. This device also
> provides a temperature measurement.
First of all, you forgot Cc reviewer(s).
> Datasheet: https://www.murata.com/en-global/products/sensor/accel/sca3300
>
No blank line in the tag block.
> Signed-off-by: Tomas Melin
...
> +/*
> + * Copyright (c) 2021 Vaisala Oyj. All rights reserved.
> + */
One line.
...
> +#define SCA3300_MASK_STATUSGENMASK(8, 0)
> +#define SCA3300_MASK_RS_STATUS GENMASK(1, 0)
This feels like an orphan. Shouldn't you move it closer to the group
of corresponding register / etc definition?
> +#define SCA3300_REG_MODE 0xd
> +#define SCA3300_REG_SELBANK0x1f
> +#define SCA3300_REG_STATUS 0x6
> +#define SCA3300_REG_WHOAMI 0x10
> +
> +#define SCA3300_VALUE_DEVICE_ID0x51
> +#define SCA3300_VALUE_RS_ERROR 0x3
> +#define SCA3300_VALUE_SW_RESET 0x20
As above it doesn't shed a light for the relationship between
registers and these fields (?). I.o.w the names w/o properly grouped
(and probably commented) are confusing.
...
> +/**
> + * struct sca3300_data - device data
> + * @spi: SPI device structure
> + * @lock: Data buffer lock
> + * @txbuf: Transmit buffer
> + * @rxbuf: Receive buffer
Are the buffers subject to DMA? Shouldn't they have the proper alignment?
> + * @scan: Triggered buffer. Four channel 16-bit data + 64-bit timestamp
> + */
> +struct sca3300_data {
> + struct spi_device *spi;
> + struct mutex lock;
> + u8 txbuf[4];
> + u8 rxbuf[4];
> + struct {
> + s16 channels[4];
> + s64 ts __aligned(sizeof(s64));
> + } scan;
> +};
...
> + struct spi_delay delay = {.value = 10, .unit = SPI_DELAY_UNIT_USECS};
Missed space.
...
> + sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);
Seems you ignored my comment. What is this 0x0? What is the meaning of it?
Same for all the rest magic numbers in the code.
> + /*
> +* return status error is cleared after reading status register once,
> +* expect EINVAL here
/*
* Fix the style of all your multi-line comments.
* You may follow this example.
*/
> +*/
> + if (ret != -EINVAL) {
> + dev_err(_data->spi->dev,
> + "error reading device status: %d\n", ret);
> + return ret;
> + }
> +
> + dev_err(_data->spi->dev, "device status: 0x%lx\n",
> + (val & SCA3300_MASK_STATUS));
Too many parentheses.
> + return 0;
> +}
...
> +static irqreturn_t sca3300_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct sca3300_data *data = iio_priv(indio_dev);
> + int bit, ret, val, i = 0;
> +
> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> +indio_dev->masklength) {
> + ret = sca3300_read_reg(data, sca3300_channels[bit].address,
> + );
> + if (ret) {
> + dev_err(>spi->dev,
> + "failed to read register, error: %d\n", ret);
> + goto out;
Does it mean interrupt is handled in this case?
Perhaps a comment why it's okay to consider so?
> + }
> + data->scan.channels[i++] = val;
> + }
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, >scan,
> + iio_get_time_ns(indio_dev));
> +out:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
...
> + /*
> +* wait 1ms after SW-reset command
> +* wait 15ms for settling of signal paths
> +*/
> + usleep_range(16e3, 50e3);
+ blank line
> + ret = sca3300_read_reg(sca_data, SCA3300_REG_WHOAMI, );
> + if (ret)
> + return ret;
> + ret = devm_iio_device_register(>dev, indio_dev);
> + if (ret) {
> + dev_err(>dev, "iio device register failed, error: %d\n",
> + ret);
> + return ret;
> + }
> +
> + return ret;
Deduplicate it.
Simply leave the latter one.
> +}
...
> +
No need for this blank line.
> + .probe = sca3300_probe,
> +};
> +
Ditto.
> +module_spi_driver(sca3300_driver);
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] i2c: I2C_HISI should depend on ARCH_HISI && ACPI
On Mon, Apr 19, 2021 at 4:02 PM Geert Uytterhoeven wrote: > On Thu, Apr 15, 2021 at 10:50 AM Andy Shevchenko > wrote: > > On Thu, Apr 15, 2021 at 3:43 AM Geert Uytterhoeven > > wrote: > > > On Wed, Apr 14, 2021 at 9:14 PM Andy Shevchenko > > > wrote: > > > > On Wed, Apr 14, 2021 at 08:55:21PM +0200, Geert Uytterhoeven wrote: > > > > > On Wed, Apr 14, 2021 at 8:18 PM Andy Shevchenko > > > > > wrote: > > > > > > On Wed, Apr 14, 2021 at 08:06:18PM +0200, Geert Uytterhoeven wrote: > > > > > > > On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang > > > > > > > wrote: ... > > > > > > > I guess it's still fine to add a dependency on ACPI? > > > > > > > > > > > > But why? > > > > > > > > > > Please tell me how/when the driver is used when CONFIG_ACPI=n. > > > > > > > > I'm not using it at all. Ask the author :-) > > > > > > > > But if we follow your logic, then we need to mark all the _platform_ > > > > drivers > > > > for x86 world as ACPI dependent? This sounds ugly. > > > > > > Do all other x86 platform drivers have (1) an .acpi_match_table[] and > > > (2) no other way of instantiating their devices? > > > The first driver from the top of my memory I looked at is rtc-cmos: > > > it has no .acpi_match_table[], and the rtc-cmos device is instantiated > > > from arch/x86/kernel/rtc.c. > > > > > > For drivers with only an .of_match_table(), and no legacy users > > > instantiating platform devices, we do have dependencies on OF. > > > > This is not true. Entire IIO subsystem is an example. > > Do you care to elaborate? > Three quarters of the IIO drivers are I2C and SPI drivers, and thus not > subject to the above. It seems I missed that you are talking about platform device drivers. In any case it's not true. We have the platform drivers w/o legacy users that are not dependent on OF. They may _indirectly_ be dependent, but this is fine as I stated above when suggested to move ACPI dependency on ARCH_xxx level. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] tee: optee: Provide special parameter field for UUID values
On Mon, Apr 19, 2021 at 4:30 PM Jens Wiklander
wrote:
> On Mon, Apr 19, 2021 at 2:01 PM Andy Shevchenko
> wrote:
> >
> > On Mon, Apr 19, 2021 at 01:35:51PM +0200, Jens Wiklander wrote:
> > > On Thu, Apr 15, 2021 at 4:58 PM Andy Shevchenko
> > > wrote:
> >
> > Thanks for review, my answer below.
> >
> > > > struct optee_msg_param_tmem tmem;
> > > > struct optee_msg_param_rmem rmem;
> > > > struct optee_msg_param_value value;
> > > > + uuid_t uuid;
> > >
> > > It's nice to get rid of the cast above, but I'm not that keen on the
> > > change in this struct. This file defines the ABI towards Secure world
> > > and adding dependencies on external complex types is a larger problem
> > > than the cast above in my opinion.
> >
> > I understand.
> >
> > So, the cast is simply wrong there. Can you add a comment above that cast to
> > explain that and make it is marked as FIXME? Because there is no guarantee
> > that
> > internal Linux types can be 1:1 mapped to the ABI of something.
>
> We might as well fix it directly instead. How about storing the
> intermediate result in a proper uuid_t and then export it as:
> export_uuid((u8 *)_arg->params[1].u.uuid, );
Still a casting here.
With u64 members you have a (potential) endianness issue (consider
BE-32 platform). Also you never know that a b c translates properly to
byte array.
I would rather see a custom function
optee_import_uuid(param, uuid_t *uuid)
{
u8 uuid_raw[UUID_SIZE];
put_unaligned_le64(_raw[0], param.a); // not sure about endianness
put_unaligned_le64(_raw[0], param.b); // ditto
import_uuid();
}
> > What you need, perhaps, is a middle layer function that will copy u64 data
> > to uuid_t or so. Also, u64 is not an ABI type, why the respective __uXX
> > variants are not in use?
>
> Does it make any difference? The file isn't shared with user space and
> I need to sync the file manually anyway since OP-TEE doesn't have the
> same include files.
Yes. It gives a hint that these are ABI (that's why I felt free to add
a member to the union. I have no idea that's an ABI). Optionally a
comment suggesting that.
Besides the above mentioned issues.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
On Mon, Apr 19, 2021 at 2:36 PM Tomas Melin wrote:
> On 4/19/21 2:14 PM, Andy Shevchenko wrote:
> > On Mon, Apr 19, 2021 at 1:29 PM Tomas Melin wrote:
> >> On 4/17/21 3:39 PM, Andy Shevchenko wrote:
> >>> On Fri, Apr 16, 2021 at 5:21 PM Tomas Melin
> >>> wrote:
> >>>> Add initial support for Murata SCA3300 3-axis industrial
> >>>> accelerometer with digital SPI interface. This device also
> >>>> provides a temperature measurement.
> > ...
> >
> >>>> + ret = spi_sync_transfer(sca_data->spi, xfers, ARRAY_SIZE(xfers));
> >>>> + if (ret < 0) {
> >>>> + dev_err(_data->spi->dev,
> >>>> + "transfer error, error: %d\n", ret);
> >>>> + return -EIO;
> >>> Why shadowing error code?
> >> Returning EIO here to have full control over the return value from this
> >> function. As return value of this is used for testing
> > Care to show what kind of testing requires this?
> > Also why can't it be refactored to accept all error codes?
>
> I was referring to this:
>
> +static int sca3300_read_reg(struct sca3300_data *sca_data, u8 reg, int *val)
> +{
> + int ret;
> +
> + mutex_lock(_data->lock);
> + sca_data->txbuf[0] = 0x0 | (reg << 2);
> + ret = sca3300_transfer(sca_data, val);
> + mutex_unlock(_data->lock);
> + if (ret == -EINVAL)
> + ret = sca3300_error_handler(sca_data);
> +
> + return ret;
> +}
> +
> +static int sca3300_write_reg(struct sca3300_data *sca_data, u8 reg, int val)
> +{
> + int reg_val = 0;
> + int ret;
> +
> + mutex_lock(_data->lock);
> + sca_data->txbuf[0] = BIT(7) | (reg << 2);
> + put_unaligned_be16(val, _data->txbuf[1]);
> + ret = sca3300_transfer(sca_data, _val);
> + mutex_unlock(_data->lock);
> + if (ret == -EINVAL)
> + ret = sca3300_error_handler(sca_data);
> +
> + return ret;
> +}
>
> So this goes into error handling only when transfer indicates EINVAL
> (which happens when
>
> transfer otherwise is good, but device return status has error flags set
> i message).
In such cases I would recommend introducing your own error space (with
positive numbers) or playing around with the number of transfers (but
this usually works only if you anticipate several of them in a row).
Something like
#define SCA3300_ERROR_FLAGS 1
...
if (ret > 0)
return error_handler(..., ret); // ret in case if you want to
convert the code to something in Linux error code space.
> >> for possible status error (EINVAL), feels more confident to have it like
> >> this to avoid any confusion. And atleast spi_sync_transfer() return value
> >>
> >> would be visible in error message.
> >>>> + }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/1] tee: optee: Provide special parameter field for UUID values
On Mon, Apr 19, 2021 at 01:35:51PM +0200, Jens Wiklander wrote: > On Thu, Apr 15, 2021 at 4:58 PM Andy Shevchenko > wrote: Thanks for review, my answer below. > > struct optee_msg_param_tmem tmem; > > struct optee_msg_param_rmem rmem; > > struct optee_msg_param_value value; > > + uuid_t uuid; > > It's nice to get rid of the cast above, but I'm not that keen on the > change in this struct. This file defines the ABI towards Secure world > and adding dependencies on external complex types is a larger problem > than the cast above in my opinion. I understand. So, the cast is simply wrong there. Can you add a comment above that cast to explain that and make it is marked as FIXME? Because there is no guarantee that internal Linux types can be 1:1 mapped to the ABI of something. What you need, perhaps, is a middle layer function that will copy u64 data to uuid_t or so. Also, u64 is not an ABI type, why the respective __uXX variants are not in use? -- With Best Regards, Andy Shevchenko
[PATCH v1 6/6] mmc: mmc_spi: Make of_mmc_spi.c resource provider agnostic
In order to use the same driver on non-OF platforms, make
of_mmc_spi.c resource provider agnostic.
Signed-off-by: Andy Shevchenko
---
drivers/mmc/host/Makefile | 2 --
drivers/mmc/host/of_mmc_spi.c | 6 ++
include/linux/spi/mmc_spi.h | 9 -
3 files changed, 2 insertions(+), 15 deletions(-)
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 6df5c4774260..14004cc09aaa 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -34,9 +34,7 @@ obj-$(CONFIG_MMC_TIFM_SD) += tifm_sd.o
obj-$(CONFIG_MMC_MVSDIO) += mvsdio.o
obj-$(CONFIG_MMC_DAVINCI) += davinci_mmc.o
obj-$(CONFIG_MMC_SPI) += mmc_spi.o
-ifeq ($(CONFIG_OF),y)
obj-$(CONFIG_MMC_SPI) += of_mmc_spi.o
-endif
obj-$(CONFIG_MMC_S3C) += s3cmci.o
obj-$(CONFIG_MMC_SDRICOH_CS) += sdricoh_cs.o
obj-$(CONFIG_MMC_TMIO) += tmio_mmc.o
diff --git a/drivers/mmc/host/of_mmc_spi.c b/drivers/mmc/host/of_mmc_spi.c
index 009c3885f6ba..9d480a05f655 100644
--- a/drivers/mmc/host/of_mmc_spi.c
+++ b/drivers/mmc/host/of_mmc_spi.c
@@ -51,10 +51,9 @@ struct mmc_spi_platform_data *mmc_spi_get_pdata(struct
spi_device *spi)
{
struct mmc_host *mmc = dev_get_drvdata(>dev);
struct device *dev = >dev;
- struct device_node *np = dev->of_node;
struct of_mmc_spi *oms;
- if (dev->platform_data || !np)
+ if (dev->platform_data || !dev_fwnode(dev))
return dev->platform_data;
oms = kzalloc(sizeof(*oms), GFP_KERNEL);
@@ -83,10 +82,9 @@ EXPORT_SYMBOL(mmc_spi_get_pdata);
void mmc_spi_put_pdata(struct spi_device *spi)
{
struct device *dev = >dev;
- struct device_node *np = dev->of_node;
struct of_mmc_spi *oms = to_of_mmc_spi(dev);
- if (!dev->platform_data || !np)
+ if (!dev->platform_data || !dev_fwnode(dev))
return;
kfree(oms);
diff --git a/include/linux/spi/mmc_spi.h b/include/linux/spi/mmc_spi.h
index 778ae8eb1f3e..9ad9a06e488d 100644
--- a/include/linux/spi/mmc_spi.h
+++ b/include/linux/spi/mmc_spi.h
@@ -35,16 +35,7 @@ struct mmc_spi_platform_data {
void (*setpower)(struct device *, unsigned int maskval);
};
-#ifdef CONFIG_OF
extern struct mmc_spi_platform_data *mmc_spi_get_pdata(struct spi_device *spi);
extern void mmc_spi_put_pdata(struct spi_device *spi);
-#else
-static inline struct mmc_spi_platform_data *
-mmc_spi_get_pdata(struct spi_device *spi)
-{
- return spi->dev.platform_data;
-}
-static inline void mmc_spi_put_pdata(struct spi_device *spi) {}
-#endif /* CONFIG_OF */
#endif /* __LINUX_SPI_MMC_SPI_H */
--
2.30.2
[PATCH v1 5/6] mmc: mmc_spi: Use already parsed IRQ
SPI core already parses and maps IRQ for us if provided.
Use it instead of double parsing in mmc_spi_get_pdata().
Due to above, change condition, since SPI core can hold
an error pointer as invalid IRQ.
Signed-off-by: Andy Shevchenko
---
drivers/mmc/host/of_mmc_spi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/of_mmc_spi.c b/drivers/mmc/host/of_mmc_spi.c
index 0b038f5c392a..009c3885f6ba 100644
--- a/drivers/mmc/host/of_mmc_spi.c
+++ b/drivers/mmc/host/of_mmc_spi.c
@@ -64,8 +64,8 @@ struct mmc_spi_platform_data *mmc_spi_get_pdata(struct
spi_device *spi)
if (mmc_of_parse_voltage(mmc, >pdata.ocr_mask) < 0)
goto err_ocr;
- oms->detect_irq = irq_of_parse_and_map(np, 0);
- if (oms->detect_irq != 0) {
+ oms->detect_irq = spi->irq;
+ if (oms->detect_irq > 0) {
oms->pdata.init = of_mmc_spi_init;
oms->pdata.exit = of_mmc_spi_exit;
} else {
--
2.30.2
[PATCH v1 3/6] mmc: mmc_spi: Set up polling even if voltage-ranges is not present
When voltage-ranges property is not present the driver assumes that
it is 3.3v (3.2v..3.4v). But at the same time it disallows polling.
Fix that by dropping the comparison to 0 when no property is provided.
While at it, mark voltage-ranges property optional as it was initially.
Fixes: 9c43df57910b ("mmc_spi: Add support for OpenFirmware bindings")
Signed-off-by: Andy Shevchenko
---
Documentation/devicetree/bindings/mmc/mmc-spi-slot.txt | 6 +++---
drivers/mmc/host/of_mmc_spi.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/mmc/mmc-spi-slot.txt
b/Documentation/devicetree/bindings/mmc/mmc-spi-slot.txt
index 75486cca8054..5e74db69f581 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-spi-slot.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc-spi-slot.txt
@@ -5,11 +5,11 @@ by mmc.txt and the properties used by the mmc_spi driver.
Required properties:
- spi-max-frequency : maximum frequency for this device (Hz).
-- voltage-ranges : two cells are required, first cell specifies minimum
- slot voltage (mV), second cell specifies maximum slot voltage (mV).
- Several ranges could be specified.
Optional properties:
+- voltage-ranges : two cells are required, first cell specifies minimum
+ slot voltage (mV), second cell specifies maximum slot voltage (mV).
+ Several ranges could be specified. If not provided, 3.2v..3.4v is assumed.
- gpios : may specify GPIOs in this order: Card-Detect GPIO,
Write-Protect GPIO. Note that this does not follow the
binding from mmc.txt, for historical reasons.
diff --git a/drivers/mmc/host/of_mmc_spi.c b/drivers/mmc/host/of_mmc_spi.c
index acd96ea399b8..843ec3db891b 100644
--- a/drivers/mmc/host/of_mmc_spi.c
+++ b/drivers/mmc/host/of_mmc_spi.c
@@ -66,7 +66,7 @@ struct mmc_spi_platform_data *mmc_spi_get_pdata(struct
spi_device *spi)
if (!oms)
return NULL;
- if (mmc_of_parse_voltage(mmc, >pdata.ocr_mask) <= 0)
+ if (mmc_of_parse_voltage(mmc, >pdata.ocr_mask) < 0)
goto err_ocr;
oms->detect_irq = irq_of_parse_and_map(np, 0);
--
2.30.2
[PATCH v1 4/6] mmc: mmc_spi: Drop unused NO_IRQ definition
After the commit 073350f7b562 ("mmc: mmc_spi: Fix return value evaluation of
irq_of_parse_and_map()") the NO_IRQ is not used anymore, drop it for good.
Fixes: 073350f7b562 ("mmc: mmc_spi: Fix return value evaluation of
irq_of_parse_and_map()")
Signed-off-by: Andy Shevchenko
---
drivers/mmc/host/of_mmc_spi.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/mmc/host/of_mmc_spi.c b/drivers/mmc/host/of_mmc_spi.c
index 843ec3db891b..0b038f5c392a 100644
--- a/drivers/mmc/host/of_mmc_spi.c
+++ b/drivers/mmc/host/of_mmc_spi.c
@@ -19,11 +19,6 @@
#include
#include
-/* For archs that don't support NO_IRQ (such as mips), provide a dummy value */
-#ifndef NO_IRQ
-#define NO_IRQ 0
-#endif
-
MODULE_LICENSE("GPL");
struct of_mmc_spi {
--
2.30.2
[PATCH v1 2/6] mmc: core: Convert mmc_of_parse_voltage() to use device property API
mmc_of_parse() for a few years has been using device property API.
Convert mmc_of_parse_voltage() as well.
At the same time switch users to new API.
Signed-off-by: Andy Shevchenko
---
drivers/mmc/core/host.c| 46 +-
drivers/mmc/host/mmc_spi.c | 8 +++---
drivers/mmc/host/of_mmc_spi.c | 3 +-
drivers/mmc/host/sdhci-esdhc-imx.c | 2 +-
drivers/mmc/host/sdhci-of-esdhc.c | 2 +-
include/linux/mmc/host.h | 2 +-
6 files changed, 41 insertions(+), 22 deletions(-)
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index ce030c5aa53c..793c7425d0dd 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -379,44 +379,62 @@ EXPORT_SYMBOL(mmc_of_parse);
/**
* mmc_of_parse_voltage - return mask of supported voltages
- * @np: The device node need to be parsed.
+ * @host: host whose properties should be parsed.
* @mask: mask of voltages available for MMC/SD/SDIO
*
- * Parse the "voltage-ranges" DT property, returning zero if it is not
+ * Parse the "voltage-ranges" property, returning zero if it is not
* found, negative errno if the voltage-range specification is invalid,
* or one if the voltage-range is specified and successfully parsed.
*/
-int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
+int mmc_of_parse_voltage(struct mmc_host *host, u32 *mask)
{
- const u32 *voltage_ranges;
+ const char *prop = "voltage-ranges";
+ struct device *dev = host->parent;
+ u32 *voltage_ranges;
int num_ranges, i;
+ int ret;
- voltage_ranges = of_get_property(np, "voltage-ranges", _ranges);
- if (!voltage_ranges) {
- pr_debug("%pOF: voltage-ranges unspecified\n", np);
+ if (!device_property_present(dev, prop)) {
+ dev_dbg(dev, "%s unspecified\n", prop);
return 0;
}
- num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
+
+ ret = device_property_count_u32(dev, prop);
+ if (ret < 0)
+ return ret;
+
+ num_ranges = ret / 2;
if (!num_ranges) {
- pr_err("%pOF: voltage-ranges empty\n", np);
+ dev_err(dev, "%s empty\n", prop);
return -EINVAL;
}
+ voltage_ranges = kcalloc(2 * num_ranges, sizeof(*voltage_ranges),
GFP_KERNEL);
+ if (!voltage_ranges)
+ return -ENOMEM;
+
+ ret = device_property_read_u32_array(dev, prop, voltage_ranges, 2 *
num_ranges);
+ if (ret) {
+ kfree(voltage_ranges);
+ return ret;
+ }
+
for (i = 0; i < num_ranges; i++) {
const int j = i * 2;
u32 ocr_mask;
- ocr_mask = mmc_vddrange_to_ocrmask(
- be32_to_cpu(voltage_ranges[j]),
- be32_to_cpu(voltage_ranges[j + 1]));
+ ocr_mask = mmc_vddrange_to_ocrmask(voltage_ranges[j + 0],
+ voltage_ranges[j + 1]);
if (!ocr_mask) {
- pr_err("%pOF: voltage-range #%d is invalid\n",
- np, i);
+ dev_err(dev, "range #%d in %s is invalid\n", i, prop);
+ kfree(voltage_ranges);
return -EINVAL;
}
*mask |= ocr_mask;
}
+ kfree(voltage_ranges);
+
return 1;
}
EXPORT_SYMBOL(mmc_of_parse_voltage);
diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 02f4fd26e76a..9776a03a10f5 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -1397,6 +1397,8 @@ static int mmc_spi_probe(struct spi_device *spi)
host->ones = ones;
+ dev_set_drvdata(>dev, mmc);
+
/* Platform data is used to hook up things like card sensing
* and power switching gpios.
*/
@@ -1413,8 +1415,6 @@ static int mmc_spi_probe(struct spi_device *spi)
host->powerup_msecs = 250;
}
- dev_set_drvdata(>dev, mmc);
-
/* preallocate dma buffers */
host->data = kmalloc(sizeof(*host->data), GFP_KERNEL);
if (!host->data)
@@ -1494,8 +1494,8 @@ static int mmc_spi_probe(struct spi_device *spi)
fail_dma:
kfree(host->data);
fail_nobuf1:
- mmc_free_host(mmc);
mmc_spi_put_pdata(spi);
+ mmc_free_host(mmc);
nomem:
kfree(ones);
return status;
@@ -1518,8 +1518,8 @@ static int mmc_spi_remove(struct spi_device *spi)
kfree(host->ones);
spi->max_speed_hz = mmc->f_max;
- mmc_free_host(mmc);
mmc_spi_put_pdata(spi);
+ mmc_free_host(mmc);
return 0;
}
diff --git a/drivers/mmc/host/of_mmc_spi.c b/drivers/mmc/host/of_mmc_spi.c
[PATCH v1 1/6] mmc: core: Correct descriptions in mmc_of_parse()
Since it has been converted to use device property API, the function
and field descriptions become outdated. Correct them.
Fixes: 73a47a9bb3e2 ("mmc: core: Use device_property_read instead of
of_property_read")
Signed-off-by: Andy Shevchenko
---
drivers/mmc/core/host.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 9b89a91b6b47..ce030c5aa53c 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -209,8 +209,8 @@ mmc_of_parse_clk_phase(struct mmc_host *host, struct
mmc_clk_phase_map *map)
EXPORT_SYMBOL(mmc_of_parse_clk_phase);
/**
- * mmc_of_parse() - parse host's device-tree node
- * @host: host whose node should be parsed.
+ * mmc_of_parse() - parse host's device properties
+ * @host: host whose properties should be parsed.
*
* To keep the rest of the MMC subsystem unaware of whether DT has been
* used to to instantiate and configure this host instance or not, we
--
2.30.2
Re: [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
On Mon, Apr 19, 2021 at 1:29 PM Tomas Melin wrote:
> On 4/17/21 3:39 PM, Andy Shevchenko wrote:
> > On Fri, Apr 16, 2021 at 5:21 PM Tomas Melin wrote:
> >> Add initial support for Murata SCA3300 3-axis industrial
> >> accelerometer with digital SPI interface. This device also
> >> provides a temperature measurement.
...
> >> + ret = spi_sync_transfer(sca_data->spi, xfers, ARRAY_SIZE(xfers));
> >> + if (ret < 0) {
> >> + dev_err(_data->spi->dev,
> >> + "transfer error, error: %d\n", ret);
> >> + return -EIO;
> > Why shadowing error code?
>
> Returning EIO here to have full control over the return value from this
> function. As return value of this is used for testing
Care to show what kind of testing requires this?
Also why can't it be refactored to accept all error codes?
> for possible status error (EINVAL), feels more confident to have it like
> this to avoid any confusion. And atleast spi_sync_transfer() return value
>
> would be visible in error message.
> >> + }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/1] pinctrl: core: Show pin numbers for the controllers with base = 0
On Thu, Apr 15, 2021 at 4:07 PM Andy Shevchenko
wrote:
>
> The commit f1b206cf7c57 ("pinctrl: core: print gpio in pins debugfs file")
> enabled GPIO pin number and label in debugfs for pin controller. However,
> it limited that feature to the chips where the base is a positive number.
> This,
> in particular, excluded chips where base is 0 for the historical or backward
> compatibility reasons. Refactor the code to include the latter as well.
Linus, since we got one more week, can you consider applying this one
and the other one against kernel doc for the final release?
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] MAINTAINERS: adjust to removing i2c designware platform data
On Mon, Apr 19, 2021 at 9:38 AM Lukas Bulwahn wrote:
>
> Commit 5a517b5bf687 ("i2c: designware: Get rid of legacy platform data")
> removes ./include/linux/platform_data/i2c-designware.h, but misses to
> adjust the SYNOPSYS DESIGNWARE I2C DRIVER section in MAINTAINERS.
>
> Hence, ./scripts/get_maintainer.pl --self-test=patterns complains:
>
> warning: no file matches F: include/linux/platform_data/i2c-designware.h
>
> Remove the file entry to this removed file as well.
Oops, I was under the impression I grepped all occurrences, but I have not.
Reviewed-by: Andy Shevchenko
Thanks for the catch!
> Signed-off-by: Lukas Bulwahn
> ---
> applies cleanly on next-20210419
>
> Andy, please ack.
> Lee, please pick this minor patch on your -next tree.
>
> MAINTAINERS | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bbe356508f29..6b903aad27f4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17667,7 +17667,6 @@ R: Mika Westerberg
>
> L: linux-...@vger.kernel.org
> S: Maintained
> F: drivers/i2c/busses/i2c-designware-*
> -F: include/linux/platform_data/i2c-designware.h
>
> SYNOPSYS DESIGNWARE MMC/SD/SDIO DRIVER
> M: Jaehoon Chung
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 4/7] iio: st_sensors: Call st_sensors_power_enable() from bus drivers
On Sun, Apr 18, 2021 at 1:54 PM Jonathan Cameron wrote: > > On Wed, 14 Apr 2021 22:54:51 +0300 > Andy Shevchenko wrote: > > > In case we would initialize two IIO devices from one physical device, > > we shouldn't have a clash on regulators. That's why move > > st_sensors_power_enable() call from core to bus drivers. > > Why is this a problem? The two instances would double up and both get + > enable + disable the regulators. However, that shouldn't matter as > they are reference counted anyway. > Perhaps an example? Even in patch 6 I can only see that it is wasteful > to do it twice, rather than wrong as such. Yep, now I also understand that I do it twice after this patch. But lemme check next week this all P.S. I have real hardware to test on. But my tests I guess are limited to iio_info or so. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 6/7] iio: st_sensors: Add lsm9ds0 IMU support
On Sun, Apr 18, 2021 at 4:49 PM Andy Shevchenko wrote: > > On Sun, Apr 18, 2021 at 2:07 PM Jonathan Cameron wrote: > > Thanks for review, my answers below. > > > On Wed, 14 Apr 2021 22:54:53 +0300 > > Andy Shevchenko wrote: > > > > > We can utilize separate drivers for accelerometer and magnetometer, > > > so here is the glue driver to enable LSM9DS0 IMU support. > > > > > > The idea was suggested by Crestez Dan Leonard in [1]. The proposed change > > > was sent as RFC due to race condition concerns, which are indeed possible. > > > > If you are going to mention races, good to give some flavour in here! > > I meant that the initial idea is racy due to different devices > communicating to the same i2c address. > So, any sequence of transfers are not serialized and you may end up with > > drv1 -> i2c > drv2 -> i2c > drv1 <- i2c # garbage > > > This driver makes me very nervous indeed. > > Why?! This one is race free as far as I can see. Or maybe I interpret > this wrongly and you are talking about initial RFC? > > > I haven't 'found' any places > > where the fact we'll write the same registers from each of the drivers > > causes problems (e.g. int pin setup etc) but perhaps I'm missing something. > > > > Shall we say that makes me rather keener to get eyes (and thought) on this > > patch than normal :) > > How should I amend the commit message to state: > 1. First idea (RFC by the link) *is* racy AFAIU > 2. This one *is not* racy. I re-read this and now understand better what you meant. So, it may be that the initial proposal may work without any amendment, but since I haven't investigated much, I should rather use the phrase "potentially racy". In my variant it's using one regmap for both drivers (not two), which makes the register state consistent. Am I wrong? Do we have some places where we may write to the same register concurrently? -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 6/7] iio: st_sensors: Add lsm9ds0 IMU support
On Sun, Apr 18, 2021 at 2:07 PM Jonathan Cameron wrote:
Thanks for review, my answers below.
> On Wed, 14 Apr 2021 22:54:53 +0300
> Andy Shevchenko wrote:
>
> > We can utilize separate drivers for accelerometer and magnetometer,
> > so here is the glue driver to enable LSM9DS0 IMU support.
> >
> > The idea was suggested by Crestez Dan Leonard in [1]. The proposed change
> > was sent as RFC due to race condition concerns, which are indeed possible.
>
> If you are going to mention races, good to give some flavour in here!
I meant that the initial idea is racy due to different devices
communicating to the same i2c address.
So, any sequence of transfers are not serialized and you may end up with
drv1 -> i2c
drv2 -> i2c
drv1 <- i2c # garbage
> This driver makes me very nervous indeed.
Why?! This one is race free as far as I can see. Or maybe I interpret
this wrongly and you are talking about initial RFC?
> I haven't 'found' any places
> where the fact we'll write the same registers from each of the drivers
> causes problems (e.g. int pin setup etc) but perhaps I'm missing something.
>
> Shall we say that makes me rather keener to get eyes (and thought) on this
> patch than normal :)
How should I amend the commit message to state:
1. First idea (RFC by the link) *is* racy AFAIU
2. This one *is not* racy.
> > In order to amend the initial change,
You see, *in order to amend*, so here is the *amended* version.
> I went further by providing a specific
> > multi-instantiate probe driver that reuses existing accelerometer and
> > magnetometer.
> >
> > [1]: https://lore.kernel.org/patchwork/patch/670353/
> >
> > Suggested-by: Crestez Dan Leonard
> > Cc: mr.laho...@laposte.net
> > Cc: Matija Podravec
> > Cc: Sergey Borishchenko
> > Signed-off-by: Andy Shevchenko
>
> A few comments in here, though mostly about stuff related to the origin code
> you are copying so perhaps not tidying them up is preferable because it would
> complicate comparison of the two cases.
...
> > + {
> > + .wai = 0x49,
> > + .wai_addr = ST_SENSORS_DEFAULT_WAI_ADDRESS,
> > + .sensors_supported = {
> > + [0] = LSM9DS0_IMU_DEV_NAME,
>
> What does the name attribute report for these?
>
> Previously we've had the _accel etc postfix to differentiate the devices. I
> don't
> suppose it matters to much though as easy enough to identify the accelerometer
> etc from what channels are present.
>
> Of course driver may get name from somewhere different anyway, I haven't
> checked,
> just noticed this was different and wondered what the affect might be.
Yes, it has a postfix, that's why I leave it like this.
...
> > +static int st_lsm9ds0_power_enable(struct device *dev, struct st_lsm9ds0
> > *lsm9ds0)
> > +{
> > + int ret;
> > +
> > + /* Regulators not mandatory, but if requested we should enable them.
> > */
>
> That's a bit of a missleading comment though cut and paste from the other
> driver
> code. Key is that they will be handled by stub regulators if we don't provide
> the which is not really what that comment implies.
I see. I will remove it.
> > + lsm9ds0->vdd = devm_regulator_get(dev, "vdd");
> > + if (IS_ERR(lsm9ds0->vdd)) {
> > + dev_err(dev, "unable to get Vdd supply\n");
> > + return PTR_ERR(lsm9ds0->vdd);
> > + }
> > + ret = regulator_enable(lsm9ds0->vdd);
> > + if (ret) {
> > + dev_warn(dev, "Failed to enable specified Vdd supply\n");
>
> Given we fail to probe if this is true, dev_warn seems a bit soft.
Right. I'll move to dev_err().
> > + return ret;
> > + }
> > +
> > + lsm9ds0->vdd_io = devm_regulator_get(dev, "vddio");
> > + if (IS_ERR(lsm9ds0->vdd_io)) {
> > + dev_err(dev, "unable to get Vdd_IO supply\n");
> > + regulator_disable(lsm9ds0->vdd);
> > + return PTR_ERR(lsm9ds0->vdd_io);
> > + }
> > + ret = regulator_enable(lsm9ds0->vdd_io);
> > + if (ret) {
> > + dev_warn(dev, "Failed to enable specified Vdd_IO supply\n");
Ditto.
> > + regulator_disable(lsm9ds0->vdd);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 4/7] iio: st_sensors: Call st_sensors_power_enable() from bus drivers
On Sun, Apr 18, 2021 at 1:54 PM Jonathan Cameron wrote: > On Wed, 14 Apr 2021 22:54:51 +0300 > Andy Shevchenko wrote: > > > In case we would initialize two IIO devices from one physical device, > > we shouldn't have a clash on regulators. That's why move > > st_sensors_power_enable() call from core to bus drivers. > > Why is this a problem? You can't have two regulators of the same name on the same device. IIRC the regulator framework produces a good splat for this. > The two instances would double up and both get + > enable + disable the regulators. However, that shouldn't matter as > they are reference counted anyway. > > Perhaps an example? Even in patch 6 I can only see that it is wasteful > to do it twice, rather than wrong as such. Believe me, I would like to avoid that, but it seems a limitation of the regulator framework. It simply produces a splat. I'll try to reproduce it again (because this series started like a couple of years ago, just eventually I found a time to clean up and submit) and will tell you how it is going. -- With Best Regards, Andy Shevchenko
Re: [PATCH v8 2/2] Added AMS tsl2591 device tree binding
On Fri, Apr 16, 2021 at 8:49 PM Joe Sandom wrote:
>
> Device tree binding for AMS/TAOS tsl2591 ambient light sensor.
>
> This driver supports configuration via device tree and sysfs.
> Supported channels for raw infrared light intensity,
> raw combined light intensity and illuminance in lux.
> The driver additionally supports iio events on lower and
> upper thresholds.
>
> This is a very-high sensitivity light-to-digital converter that
> transforms light intensity into a digital signal.
With subject line fixed (other comments up to you)
Reviewed-by: Andy Shevchenko
> Signed-off-by: Joe Sandom
> Reviewed-by: Rob Herring
> ---
> Changes in v8:
> - No changes
>
> Notes:
> - Re-submitted to align the version with part 1 of the patch series
>
> .../bindings/iio/light/amstaos,tsl2591.yaml | 50 +++
> 1 file changed, 50 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
> b/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
> new file mode 100644
> index ..596a3bc770f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/amstaos,tsl2591.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMS/TAOS TSL2591 Ambient Light Sensor (ALS)
> +
> +maintainers:
> + - Joe Sandom
> +
> +description: |
> + AMS/TAOS TSL2591 is a very-high sensitivity
> + light-to-digital converter that transforms light intensity into a digital
> + signal.
> +
> +properties:
> + compatible:
> +const: amstaos,tsl2591
> +
> + reg:
> +maxItems: 1
> +
> + interrupts:
> +maxItems: 1
> +description:
> + Interrupt (INT:Pin 2) Active low. Should be set to
> IRQ_TYPE_EDGE_FALLING.
> + interrupt is used to detect if the light intensity has fallen below
> + or reached above the configured threshold values.
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +#include
> +i2c {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +tsl2591@29 {
> +compatible = "amstaos,tsl2591";
> +reg = <0x29>;
> +interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
> + };
> +};
> +...
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v8 1/2] Added AMS tsl2591 driver implementation
On Fri, Apr 16, 2021 at 8:49 PM Joe Sandom wrote: > > Driver implementation for AMS/TAOS tsl2591 ambient light sensor. > > This driver supports configuration via device tree and sysfs. > Supported channels for raw infrared light intensity, > raw combined light intensity and illuminance in lux. > The driver additionally supports iio events on lower and > upper thresholds. > > This is a very-high sensitivity light-to-digital converter that > transforms light intensity into a digital signal. Hmm... It's v8 and the subject line is wrongly formatted. Please add the corresponding prefix "iio: light: ..." Otherwise it's in very good shape. ... > +/* TSL2591 enable register definitions */ > +#define TSL2591_PWR_ON 0x01 > +#define TSL2591_PWR_OFF 0x00 > +#define TSL2591_ENABLE_ALS 0x02 > +#define TSL2591_ENABLE_ALS_INT 0x10 > +#define TSL2591_ENABLE_SLEEP_INT0x40 > +#define TSL2591_ENABLE_NP_INT 0x80 Is it a bitfield? ... > + als_lower_l = als_lower_threshold; >> 0, but it's up to you. > + als_lower_h = als_lower_threshold >> 8; ... > + als_upper_l = als_upper_threshold; > + als_upper_h = als_upper_threshold >> 8; Ditto. -- With Best Regards, Andy Shevchenko
Re: [PATCH v8 2/2] Added AMS tsl2591 device tree binding
On Fri, Apr 16, 2021 at 8:49 PM Joe Sandom wrote: > > Device tree binding for AMS/TAOS tsl2591 ambient light sensor. > > This driver supports configuration via device tree and sysfs. > Supported channels for raw infrared light intensity, > raw combined light intensity and illuminance in lux. > The driver additionally supports iio events on lower and > upper thresholds. > > This is a very-high sensitivity light-to-digital converter that > transforms light intensity into a digital signal. Subject should be something like dt-bindings: iio: ... -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
; +}
If you switch to use regmap, the read part will be a bonus automatically.
...
> + indio_dev = devm_iio_device_alloc(>dev, sizeof(*sca_data));
> + if (!indio_dev) {
> + dev_err(>dev,
> + "failed to allocate memory for iio device\n");
Noice. You will get the same from user space.
> + return -ENOMEM;
> + }
...
> + indio_dev->dev.parent = >dev;
This is done by IIO core.
...
> + ret = devm_iio_device_register(>dev, indio_dev);
> + if (ret < 0) {
> + dev_err(>dev, "iio device register failed, error: %d\n",
> + ret);
> + return ret;
> + }
> +
> + return 0;
return ret;
> +}
...
> +static const struct of_device_id sca3300_dt_ids[] = {
> + { .compatible = "murata,sca3300"},
> + {},
Drop comma. No need for a terminator line.
> +};
...
> +static struct spi_driver sca3300_driver = {
> + .driver = {
> + .name = SCA3300_ALIAS,
> + .owner = THIS_MODULE,
Redundant.
> + .of_match_table = of_match_ptr(sca3300_dt_ids),
Drop of_match_ptr(). In 99% its use is wrong.
> + },
> +
> + .probe = sca3300_probe,
> +};
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 1/3] gpio: zynq: use module_platform_driver to simplify the code
On Fri, Apr 16, 2021 at 9:28 PM Bartosz Golaszewski wrote: > On Wed, Apr 14, 2021 at 4:45 PM Srinivas Neeli wrote: > > > From: Bartosz Golaszewski > > > Sent: Tuesday, April 13, 2021 4:14 PM > > > On Sat, Apr 10, 2021 at 12:08 AM Andy Shevchenko > > > wrote: ... > > > Yep, this has been like this since the initial introduction of this > > > driver. > > > Unfortunately there's no documented reason so unless we can test it, it > > > has > > > to stay this way. > > > > > I tested driver, functionality wise everything working fine. > > Based on below conversation, I moved driver to module driver. > > https://lore.kernel.org/patchwork/patch/818202/ > > > > Andy: How about we give it a try then? If anyone yells, we'll just revert it. Sounds like a plan! -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] libnvdimm: Don't use GUID APIs against raw buffer
On Fri, Apr 16, 2021 at 01:08:06PM -0700, Dan Williams wrote: > [ add Erik ] > > On Fri, Apr 16, 2021 at 10:36 AM Andy Shevchenko > wrote: > > > > On Thu, Apr 15, 2021 at 05:37:54PM +0300, Andy Shevchenko wrote: > > > Strictly speaking the comparison between guid_t and raw buffer > > > is not correct. Return to plain memcmp() since the data structures > > > haven't changed to use uuid_t / guid_t the current state of affairs > > > is inconsistent. Either it should be changed altogether or left > > > as is. > > > > Dan, please review this one as well. I think here you may agree with me. > > You know, this is all a problem because ACPICA is using a raw buffer. And this is fine. It might be any other representation as well. > Erik, would it be possible to use the guid_t type in ACPICA? That > would allow NFIT to drop some ugly casts. guid_t is internal kernel type. If we ever decide to deviate from the current representation it wouldn't be possible in case a 3rd party is using it 1:1 (via typedef or so). -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] libnvdimm: Don't use GUID APIs against raw buffer
On Thu, Apr 15, 2021 at 05:37:54PM +0300, Andy Shevchenko wrote: > Strictly speaking the comparison between guid_t and raw buffer > is not correct. Return to plain memcmp() since the data structures > haven't changed to use uuid_t / guid_t the current state of affairs > is inconsistent. Either it should be changed altogether or left > as is. Dan, please review this one as well. I think here you may agree with me. -- With Best Regards, Andy Shevchenko
