| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <BANLkTi=XSevO72NqCYOYuketjE8XjeQU+A@mail.gmail.com>
Date: Wed, 11 May 2011 00:18:00 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: Joe Perches <joe@...ches.com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
Grant Likely <grant.likely@...retlab.ca>,
Lee Jones <lee.jones@...aro.org>,
Martin Persson <martin.persson@...ricsson.com>
Subject: Re: [PATCH 1/4] drivers: create a pinmux subsystem
2011/5/2 Joe Perches <joe@...ches.com>:
> On Mon, 2011-05-02 at 21:16 +0200, Linus Walleij wrote:
>> From: Linus Walleij <linus.walleij@...aro.org>
>> diff --git a/drivers/pinmux/core.c b/drivers/pinmux/core.c
>
> Trivial comments follow
>
> []
>> +static ssize_t pinmux_name_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct pinmux_dev *pmxdev = dev_get_drvdata(dev);
>> +
>> + return sprintf(buf, "%s\n", pmxdev_get_name(pmxdev));
>> +}
>
> Unsized buffer, maybe snprintf?
This is the idiomatic way of providing sysfs strings (compare e.g.
*_show() in drivers/regulator/core.c), the char *buf comes
from the sysfs core in struct device_attribute in <linus/device.h>
with this prototype:
ssize_t (*show)(struct device *dev, struct device_attribute *attr,
char *buf);
and I have no way of knowing how large that buffer is. Migrating all
of sysfs to provide the size of its buffers may help but do you really
mean I should do that as part of this patchset? It will require
refactoring the entire kernel :-(
>> +static int pin_request(int pin, const char *function, bool gpio)
>> +{
>> + struct pin_desc *desc;
>> + struct pinmux_dev *pmxdev;
>> + struct pinmux_ops *ops;
>> + int status = -EINVAL;
>> + unsigned long flags;
>> +
>> + pr_debug("pin_request: request pin %d for %s\n", pin, function);
>
> pr_debug("%s: request pin...", __func__?
>
>> + pr_err("pin_request: pin is invalid\n");
>
> same here, etc...
What I am referring to here is not the name of the C function being
executed but the function that this group of pins is performing,
so a different ontology altogether.
But the prefix is indeed the function name so I get what you mean,
fixing it!
>> + if (!pmxdev) {
>> + pr_warning("pin_warning: no pinmux device is handling %d!\n",
>
> You use both pr_warning and pr_warn. Please just use pr_warn.
Sure.
> Why use "pin_warning: "?
I was drunk.
> Maybe it'd be better to add
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> or
> #define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__
>
> if you really want __func__.
> I suggest that __func__ isn't useful.
Yep, I'll use the first one and replace all prefixes with pure meaningful
messages instead.
(Omitting such comments below - all fixed.)
>> +static int pinmux_devices_show(struct seq_file *s, void *what)
>> +{
>> + struct pinmux_dev *pmxdev;
>> +
>> + seq_printf(s, "Available pinmux settings per pinmux device:\n");
>> + list_for_each_entry(pmxdev, &pinmuxdev_list, node) {
>> + struct pinmux_ops *ops = pmxdev->desc->ops;
>
> const struct pinmux_ops?
Yepps, const:ed it everywhere!
>> + unsigned selector = 0;
>> +
>> + seq_printf(s, "\nDevice %s:\n", pmxdev->desc->name);
>
> I think the initial newline isn't necessary.
Nope. Leftover.
>> + while (ops->list_functions(pmxdev, selector) >= 0) {
>> + unsigned *pins;
>> + unsigned num_pins;
>> + const char *func = ops->get_function_name(pmxdev,
>> + selector);
>> + int ret;
>> + int i;
>> +
>> + ret = ops->get_function_pins(pmxdev, selector,
>> + &pins, &num_pins);
>> +
>> + if (ret)
>> + seq_printf(s, "%s [ERROR GETTING PINS]\n",
>> + func);
>> +
>> + else {
>> + seq_printf(s, "function: %s, pins = [ ", func);
>> + for (i = 0; i < num_pins; i++)
>> + seq_printf(s, "%d ", pins[i]);
>> + seq_printf(s, "]\n");
>
> seq_printf used without additional arguments could be seq_puts
Yep, fixed everywhere I sent in a non-argumented string.
>> + (void) debugfs_create_file("devices", S_IFREG | S_IRUGO,
>> + debugfs_root, NULL, &pinmux_devices_ops);
>> + (void) debugfs_create_file("maps", S_IFREG | S_IRUGO,
>> + debugfs_root, NULL, &pinmux_maps_ops);
>> + (void) debugfs_create_file("pins", S_IFREG | S_IRUGO,
>> + debugfs_root, NULL, &pinmux_pins_ops);
>
> Unnecessary casts to (void)?
Yep lost them.
>> +static int __init pinmux_init(void)
>> +{
>> + int ret;
>> +
>> + ret = class_register(&pinmux_class);
>> + pr_info("pinmux framwork: handle up to %d pins\n", MACH_NR_PINS);
>
> framework?
Should be subsystem. Fixed it.
>> diff --git a/include/linux/pinmux.h b/include/linux/pinmux.h
> []
>> +/*
>> + * Valid pin numbers are nonnegative and < MACH_NR_PINS. Invalid numbers can
>> + * be used to indicate no-such-pin.
>> + */
>> +static inline int pin_is_valid(int pin)
>> +{
>> + return ((unsigned)pin) < MACH_NR_PINS;
>> +}
>
> Couldn't pin just be declared unsigned or maybe u32?
No, because like in the GPIO subsystem you *may* want to send in invalid
pins, and those are identified by negative numbers.
Thanks a *lot* for your detailed review Joe, please supply your Reviewed-by:
on the next (v2) patch set if you think it looks alright.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists