* Re: [PATCH 14/14] Input: atmel_mxt_ts - remove platform data support
From: Dmitry Torokhov @ 2018-03-16 0:06 UTC (permalink / raw)
To: Nick Dyer; +Cc: linux-input, Benson Leung, Olof Johansson, linux-kernel
In-Reply-To: <20180314205938.GD26353@lava.h.shmanahar.org>
On Wed, Mar 14, 2018 at 08:59:38PM +0000, Nick Dyer wrote:
> On Mon, Mar 12, 2018 at 12:09:07PM -0700, Dmitry Torokhov wrote:
> > Date: Mon, 12 Mar 2018 12:09:07 -0700
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > To: linux-input@vger.kernel.org, Benson Leung <bleung@chromium.org>
> > Cc: Nick Dyer <nick@shmanahar.org>, Olof Johansson <olof@lixom.net>,
> > linux-kernel@vger.kernel.org
> > Subject: [PATCH 14/14] Input: atmel_mxt_ts - remove platform data support
> >
> > Now that there are no users of custom Atmel platform data, and everyone
> > has switched to the generic device properties, we can remove support for
> > the platform data.
>
> Thanks, this is a nice tidy up.
>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > MAINTAINERS | 1 -
> > drivers/input/touchscreen/atmel_mxt_ts.c | 125 +++++++++------------
> > include/linux/platform_data/atmel_mxt_ts.h | 31 -----
> > 3 files changed, 50 insertions(+), 107 deletions(-)
> > delete mode 100644 include/linux/platform_data/atmel_mxt_ts.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 4623caf8d72d8..37b70874a4771 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2394,7 +2394,6 @@ T: git git://github.com/ndyer/linux.git
> > S: Maintained
> > F: Documentation/devicetree/bindings/input/atmel,maxtouch.txt
> > F: drivers/input/touchscreen/atmel_mxt_ts.c
> > -F: include/linux/platform_data/atmel_mxt_ts.h
> >
> > ATMEL SAMA5D2 ADC DRIVER
> > M: Ludovic Desroches <ludovic.desroches@microchip.com>
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index cf2aac4e79ae8..642211254acc4 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -23,10 +23,10 @@
> > #include <linux/delay.h>
> > #include <linux/firmware.h>
> > #include <linux/i2c.h>
> > -#include <linux/platform_data/atmel_mxt_ts.h>
> > #include <linux/input/mt.h>
> > #include <linux/interrupt.h>
> > #include <linux/of.h>
> > +#include <linux/property.h>
> > #include <linux/slab.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/property.h>
> > @@ -274,7 +274,6 @@ struct mxt_data {
> > struct i2c_client *client;
> > struct input_dev *input_dev;
> > char phys[64]; /* device physical location */
> > - const struct mxt_platform_data *pdata;
> > struct mxt_object *object_table;
> > struct mxt_info info;
> > unsigned int irq;
> > @@ -325,6 +324,9 @@ struct mxt_data {
> >
> > /* for config update handling */
> > struct completion crc_completion;
> > +
> > + u32 *t19_keymap;
> > + unsigned int t19_num_keys;
> > };
> >
> > struct mxt_vb2_buffer {
> > @@ -743,15 +745,14 @@ static int mxt_write_object(struct mxt_data *data,
> > static void mxt_input_button(struct mxt_data *data, u8 *message)
> > {
> > struct input_dev *input = data->input_dev;
> > - const struct mxt_platform_data *pdata = data->pdata;
> > int i;
> >
> > - for (i = 0; i < pdata->t19_num_keys; i++) {
> > - if (pdata->t19_keymap[i] == KEY_RESERVED)
> > + for (i = 0; i < data->t19_num_keys; i++) {
> > + if (data->t19_keymap[i] == KEY_RESERVED)
> > continue;
> >
> > /* Active-low switch */
> > - input_report_key(input, pdata->t19_keymap[i],
> > + input_report_key(input, data->t19_keymap[i],
> > !(message[1] & BIT(i)));
> > }
> > }
> > @@ -759,7 +760,7 @@ static void mxt_input_button(struct mxt_data *data, u8 *message)
> > static void mxt_input_sync(struct mxt_data *data)
> > {
> > input_mt_report_pointer_emulation(data->input_dev,
> > - data->pdata->t19_num_keys);
> > + data->t19_num_keys);
> > input_sync(data->input_dev);
> > }
> >
> > @@ -1859,7 +1860,6 @@ static void mxt_input_close(struct input_dev *dev);
> > static void mxt_set_up_as_touchpad(struct input_dev *input_dev,
> > struct mxt_data *data)
> > {
> > - const struct mxt_platform_data *pdata = data->pdata;
> > int i;
> >
> > input_dev->name = "Atmel maXTouch Touchpad";
> > @@ -1873,15 +1873,14 @@ static void mxt_set_up_as_touchpad(struct input_dev *input_dev,
> > input_abs_set_res(input_dev, ABS_MT_POSITION_Y,
> > MXT_PIXELS_PER_MM);
> >
> > - for (i = 0; i < pdata->t19_num_keys; i++)
> > - if (pdata->t19_keymap[i] != KEY_RESERVED)
> > + for (i = 0; i < data->t19_num_keys; i++)
> > + if (data->t19_keymap[i] != KEY_RESERVED)
> > input_set_capability(input_dev, EV_KEY,
> > - pdata->t19_keymap[i]);
> > + data->t19_keymap[i]);
> > }
> >
> > static int mxt_initialize_input_device(struct mxt_data *data)
> > {
> > - const struct mxt_platform_data *pdata = data->pdata;
> > struct device *dev = &data->client->dev;
> > struct input_dev *input_dev;
> > int error;
> > @@ -1947,7 +1946,7 @@ static int mxt_initialize_input_device(struct mxt_data *data)
> > }
> >
> > /* If device has buttons we assume it is a touchpad */
> > - if (pdata->t19_num_keys) {
> > + if (data->t19_num_keys) {
> > mxt_set_up_as_touchpad(input_dev, data);
> > mt_flags |= INPUT_MT_POINTER;
> > } else {
> > @@ -2921,51 +2920,42 @@ static void mxt_input_close(struct input_dev *dev)
> > mxt_stop(data);
> > }
> >
> > -static const struct mxt_platform_data *
> > -mxt_parse_device_properties(struct i2c_client *client)
> > +static int mxt_parse_device_properties(struct mxt_data *data)
> > {
> > static const char keymap_property[] = "linux,gpio-keymap";
> > - struct mxt_platform_data *pdata;
> > + struct device *dev = &data->client->dev;
> > u32 *keymap;
> > int n_keys;
> > int error;
> >
> > - pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> > - if (!pdata)
> > - return ERR_PTR(-ENOMEM);
> > -
> > - if (device_property_present(&client->dev, keymap_property)) {
> > - n_keys = device_property_read_u32_array(&client->dev,
> > - keymap_property,
> > + if (device_property_present(dev, keymap_property)) {
> > + n_keys = device_property_read_u32_array(dev, keymap_property,
> > NULL, 0);
> > if (n_keys <= 0) {
> > error = n_keys < 0 ? n_keys : -EINVAL;
> > - dev_err(&client->dev,
> > - "invalid/malformed '%s' property: %d\n",
> > + dev_err(dev, "invalid/malformed '%s' property: %d\n",
> > keymap_property, error);
> > - return ERR_PTR(error);
> > + return error;
> > }
> >
> > - keymap = devm_kmalloc_array(&client->dev, n_keys, sizeof(u32),
> > + keymap = devm_kmalloc_array(dev, n_keys, sizeof(*keymap),
> > GFP_KERNEL);
> > if (!keymap)
> > - return ERR_PTR(-ENOMEM);
> > + return -ENOMEM;
> >
> > - error = device_property_read_u32_array(&client->dev,
> > - keymap_property,
> > + error = device_property_read_u32_array(dev, keymap_property,
> > keymap, n_keys);
> > if (error) {
> > - dev_err(&client->dev,
> > - "failed to parse '%s' property: %d\n",
> > + dev_err(dev, "failed to parse '%s' property: %d\n",
> > keymap_property, error);
> > - return ERR_PTR(error);
> > + return error;
> > }
> >
> > - pdata->t19_keymap = keymap;
> > - pdata->t19_num_keys = n_keys;
> > + data->t19_keymap = keymap;
> > + data->t19_num_keys = n_keys;
> > }
> >
> > - return pdata;
> > + return 0;
> > }
> >
> > #ifdef CONFIG_ACPI
> > @@ -3048,25 +3038,12 @@ static const struct dmi_system_id mxt_dmi_table[] = {
> > { }
> > };
> >
> > -static int mxt_acpi_probe(struct i2c_client *client)
> > +static int mxt_prepare_acpi_properties(struct i2c_client *client)
> > {
> > struct acpi_device *adev;
> > const struct dmi_system_id *system_id;
> > const struct mxt_acpi_platform_data *acpi_pdata;
> >
> > - /*
> > - * Ignore ACPI devices representing bootloader mode.
> > - *
> > - * This is a bit of a hack: Google Chromebook BIOS creates ACPI
> > - * devices for both application and bootloader modes, but we are
> > - * interested in application mode only (if device is in bootloader
> > - * mode we'll end up switching into application anyway). So far
> > - * application mode addresses were all above 0x40, so we'll use it
> > - * as a threshold.
> > - */
> > - if (client->addr < 0x40)
> > - return -ENXIO;
>
> Could you use 0x4a, since that's the lowest application mode address,
> same in the other place.
OK, this is existing code though, so I'll make a separate patch.
>
> Can I ask if you've checked the behaviour if the device is probed while
> it's already in bootloader mode? Ideally you want to be able to recover
> from a bad firmware flash.
Yes, the device will come up at the bootloader address, but we will
ignore it here. The 2nd ACPI device with application mode address will
be probed, and we'll fail to read anything in mxt_initialize() and will
try to see if it responds at the bootloader address. It did work for me.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal
From: Miguel Ojeda @ 2018-03-16 3:05 UTC (permalink / raw)
To: Kees Cook
Cc: Linus Torvalds, Andrew Morton, Josh Poimboeuf, Rasmus Villemoes,
Randy Dunlap, Ingo Molnar, David Laight, Ian Abbott, linux-input,
linux-btrfs, Network Development, Linux Kernel Mailing List,
Kernel Hardening
In-Reply-To: <CAGXu5j+Dv_wQgHWgT=9eSoKX7AcqnumuRsNFUoZ765WWizKytw@mail.gmail.com>
On Fri, Mar 16, 2018 at 12:49 AM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Mar 15, 2018 at 4:46 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> What I'm *not* so much ok with is "const_max(5,sizeof(x))" erroring
>> out, or silently causing insane behavior due to hidden subtle type
>> casts..
>
> Yup! I like it as an explicit argument. Thanks!
>
What about something like this?
#define INTMAXT_MAX LLONG_MAX
typedef int64_t intmax_t;
#define const_max(x, y) \
__builtin_choose_expr( \
!__builtin_constant_p(x) || !__builtin_constant_p(y), \
__error_not_const_arg(), \
__builtin_choose_expr( \
(x) > INTMAXT_MAX || (y) > INTMAXT_MAX, \
__error_too_big(), \
__builtin_choose_expr( \
(intmax_t)(x) >= (intmax_t)(y), \
(x), \
(y) \
) \
) \
)
Works for different types, allows to mix negatives and positives and
returns the original type, e.g.:
const_max(-1, sizeof(char));
is of type 'long unsigned int', but:
const_max(2, sizeof(char));
is of type 'int'. While I am not a fan that the return type depends on
the arguments, it is useful if you are going to use the expression in
something that needs expects a precise (a printk() for instance?).
The check against the INTMAXT_MAX is there to avoid complexity (if we
do not handle those cases, it is safe to use intmax_t for the
comparison; otherwise you have to have another compile time branch for
the case positive-positive using uintmax_t) and also avoids odd
warnings for some cases above LLONG_MAX about comparisons with 0 for
unsigned expressions being always true. On the positive side, it
prevents using the macro for thing like "(size_t)-1".
Cheers,
Miguel
^ permalink raw reply
* [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Kees Cook @ 2018-03-16 4:25 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, Linus Torvalds, Josh Poimboeuf, Rasmus Villemoes,
Randy Dunlap, Miguel Ojeda, Ingo Molnar, David Laight, Ian Abbott,
linux-input, linux-btrfs, netdev, linux-kernel, kernel-hardening
Patch 1 adds const_max_t(), patch 2 uses it in all the places max()
was used for stack arrays. Commit log from patch 1:
---snip---
kernel.h: Introduce const_max_t() for VLA removal
In the effort to remove all VLAs from the kernel[1], it is desirable to
build with -Wvla. However, this warning is overly pessimistic, in that
it is only happy with stack array sizes that are declared as constant
expressions, and not constant values. One case of this is the evaluation
of the max() macro which, due to its construction, ends up converting
constant expression arguments into a constant value result. Attempts
to adjust the behavior of max() ran afoul of version-dependent compiler
behavior[2].
To work around this and still gain -Wvla coverage, this patch introduces
a new macro, const_max_t(), for use in these cases of stack array size
declaration, where the constant expressions are retained. Since this means
losing the double-evaluation protections of the max() macro, this macro is
designed to explicitly fail if used on non-constant arguments.
Older compilers will fail with the unhelpful message:
error: first argument to ‘__builtin_choose_expr’ not a constant
Newer compilers will fail with a hopefully more helpful message:
error: call to ‘__error_non_const_arg’ declared with attribute error: const_max_t() used with non-constant expression
To gain the ability to compare differing types, the desired type must
be explicitly declared, as with the existing max_t() macro. This is
needed when comparing different enum types and to allow things like:
int foo[const_max_t(size_t, 6, sizeof(something))];
[1] https://lkml.org/lkml/2018/3/7/621
[2] https://lkml.org/lkml/2018/3/10/170
---eol---
Hopefully this reads well as a summary from all the things that got tried.
I've tested this on allmodconfig builds with gcc 4.4.4 and 6.3.0, with and
without -Wvla.
-Kees
v5: explicit type argument
v4: forced size_t type
^ permalink raw reply
* [PATCH v5 1/2] kernel.h: Introduce const_max_t() for VLA removal
From: Kees Cook @ 2018-03-16 4:25 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, Linus Torvalds, Josh Poimboeuf, Rasmus Villemoes,
Randy Dunlap, Miguel Ojeda, Ingo Molnar, David Laight, Ian Abbott,
linux-input, linux-btrfs, netdev, linux-kernel, kernel-hardening
In-Reply-To: <1521174359-46392-1-git-send-email-keescook@chromium.org>
In the effort to remove all VLAs from the kernel[1], it is desirable to
build with -Wvla. However, this warning is overly pessimistic, in that
it is only happy with stack array sizes that are declared as constant
expressions, and not constant values. One case of this is the evaluation
of the max() macro which, due to its construction, ends up converting
constant expression arguments into a constant value result. Attempts
to adjust the behavior of max() ran afoul of version-dependent compiler
behavior[2].
To work around this and still gain -Wvla coverage, this patch introduces
a new macro, const_max_t(), for use in these cases of stack array size
declaration, where the constant expressions are retained. Since this means
losing the double-evaluation protections of the max() macro, this macro is
designed to explicitly fail if used on non-constant arguments.
Older compilers will fail with the unhelpful message:
error: first argument to ‘__builtin_choose_expr’ not a constant
Newer compilers will fail with a hopefully more helpful message:
error: call to ‘__error_non_const_arg’ declared with attribute error: const_max_t() used with non-constant expression
To gain the ability to compare differing types, the desired type must
be explicitly declared, as with the existing max_t() macro. This is
needed when comparing different enum types and to allow things like:
int foo[const_max_t(size_t, 6, sizeof(something))];
[1] https://lkml.org/lkml/2018/3/7/621
[2] https://lkml.org/lkml/2018/3/10/170
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/kernel.h | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3fd291503576..e14531781568 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -820,6 +820,25 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
x, y)
/**
+ * const_max_t - return maximum of two compile-time constant expressions
+ * @type: type used for evaluation
+ * @x: first compile-time constant expression
+ * @y: second compile-time constant expression
+ *
+ * This has no multi-evaluation defenses, and must only ever be used with
+ * compile-time constant expressions (for example when calculating a stack
+ * array size).
+ */
+size_t __error_non_const_arg(void) \
+__compiletime_error("const_max_t() used with non-constant expression");
+#define const_max_t(type, x, y) \
+ __builtin_choose_expr(__builtin_constant_p(x) && \
+ __builtin_constant_p(y), \
+ (type)(x) > (type)(y) ? \
+ (type)(x) : (type)(y), \
+ __error_non_const_arg())
+
+/**
* min3 - return minimum of three values
* @x: first value
* @y: second value
--
2.7.4
^ permalink raw reply related
* [PATCH v5 2/2] Remove false-positive VLAs when using max()
From: Kees Cook @ 2018-03-16 4:25 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, Linus Torvalds, Josh Poimboeuf, Rasmus Villemoes,
Randy Dunlap, Miguel Ojeda, Ingo Molnar, David Laight, Ian Abbott,
linux-input, linux-btrfs, netdev, linux-kernel, kernel-hardening
In-Reply-To: <1521174359-46392-1-git-send-email-keescook@chromium.org>
As part of removing VLAs from the kernel[1], we want to build with -Wvla,
but it is overly pessimistic and only accepts constant expressions for
stack array sizes, instead of also constant values. The max() macro
triggers the warning, so this refactors these uses of max() to use the
new const_max() instead.
[1] https://lkml.org/lkml/2018/3/7/621
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/input/touchscreen/cyttsp4_core.c | 2 +-
fs/btrfs/tree-checker.c | 3 ++-
lib/vsprintf.c | 5 +++--
net/ipv4/proc.c | 8 ++++----
net/ipv6/proc.c | 11 +++++------
5 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/input/touchscreen/cyttsp4_core.c b/drivers/input/touchscreen/cyttsp4_core.c
index 727c3232517c..7fb9bd48e41c 100644
--- a/drivers/input/touchscreen/cyttsp4_core.c
+++ b/drivers/input/touchscreen/cyttsp4_core.c
@@ -868,7 +868,7 @@ static void cyttsp4_get_mt_touches(struct cyttsp4_mt_data *md, int num_cur_tch)
struct cyttsp4_touch tch;
int sig;
int i, j, t = 0;
- int ids[max(CY_TMA1036_MAX_TCH, CY_TMA4XX_MAX_TCH)];
+ int ids[const_max_t(size_t, CY_TMA1036_MAX_TCH, CY_TMA4XX_MAX_TCH)];
memset(ids, 0, si->si_ofs.tch_abs[CY_TCH_T].max * sizeof(int));
for (i = 0; i < num_cur_tch; i++) {
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index c3c8d48f6618..d83244e3821f 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -341,7 +341,8 @@ static int check_dir_item(struct btrfs_root *root,
*/
if (key->type == BTRFS_DIR_ITEM_KEY ||
key->type == BTRFS_XATTR_ITEM_KEY) {
- char namebuf[max(BTRFS_NAME_LEN, XATTR_NAME_MAX)];
+ char namebuf[const_max_t(size_t, BTRFS_NAME_LEN,
+ XATTR_NAME_MAX)];
read_extent_buffer(leaf, namebuf,
(unsigned long)(di + 1), name_len);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7a708f82559..12ff57a36171 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -744,8 +744,9 @@ char *resource_string(char *buf, char *end, struct resource *res,
#define FLAG_BUF_SIZE (2 * sizeof(res->flags))
#define DECODED_BUF_SIZE sizeof("[mem - 64bit pref window disabled]")
#define RAW_BUF_SIZE sizeof("[mem - flags 0x]")
- char sym[max(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
- 2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];
+ char sym[const_max_t(size_t,
+ 2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
+ 2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];
char *p = sym, *pend = sym + sizeof(sym);
int decode = (fmt[0] == 'R') ? 1 : 0;
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index dc5edc8f7564..7f5c3b40dac9 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -46,7 +46,7 @@
#include <net/sock.h>
#include <net/raw.h>
-#define TCPUDP_MIB_MAX max_t(u32, UDP_MIB_MAX, TCP_MIB_MAX)
+#define TCPUDP_MIB_MAX const_max_t(size_t, UDP_MIB_MAX, TCP_MIB_MAX)
/*
* Report socket allocation statistics [mea@utu.fi]
@@ -404,7 +404,7 @@ static int snmp_seq_show_tcp_udp(struct seq_file *seq, void *v)
struct net *net = seq->private;
int i;
- memset(buff, 0, TCPUDP_MIB_MAX * sizeof(unsigned long));
+ memset(buff, 0, sizeof(buff));
seq_puts(seq, "\nTcp:");
for (i = 0; snmp4_tcp_list[i].name; i++)
@@ -421,7 +421,7 @@ static int snmp_seq_show_tcp_udp(struct seq_file *seq, void *v)
seq_printf(seq, " %lu", buff[i]);
}
- memset(buff, 0, TCPUDP_MIB_MAX * sizeof(unsigned long));
+ memset(buff, 0, sizeof(buff));
snmp_get_cpu_field_batch(buff, snmp4_udp_list,
net->mib.udp_statistics);
@@ -432,7 +432,7 @@ static int snmp_seq_show_tcp_udp(struct seq_file *seq, void *v)
for (i = 0; snmp4_udp_list[i].name; i++)
seq_printf(seq, " %lu", buff[i]);
- memset(buff, 0, TCPUDP_MIB_MAX * sizeof(unsigned long));
+ memset(buff, 0, sizeof(buff));
/* the UDP and UDP-Lite MIBs are the same */
seq_puts(seq, "\nUdpLite:");
diff --git a/net/ipv6/proc.c b/net/ipv6/proc.c
index b67814242f78..b68c233de296 100644
--- a/net/ipv6/proc.c
+++ b/net/ipv6/proc.c
@@ -30,10 +30,9 @@
#include <net/transp_v6.h>
#include <net/ipv6.h>
-#define MAX4(a, b, c, d) \
- max_t(u32, max_t(u32, a, b), max_t(u32, c, d))
-#define SNMP_MIB_MAX MAX4(UDP_MIB_MAX, TCP_MIB_MAX, \
- IPSTATS_MIB_MAX, ICMP_MIB_MAX)
+#define SNMP_MIB_MAX const_max_t(u32, \
+ const_max_t(u32, UDP_MIB_MAX, TCP_MIB_MAX), \
+ const_max_t(u32, IPSTATS_MIB_MAX, ICMP_MIB_MAX))
static int sockstat6_seq_show(struct seq_file *seq, void *v)
{
@@ -199,7 +198,7 @@ static void snmp6_seq_show_item(struct seq_file *seq, void __percpu *pcpumib,
int i;
if (pcpumib) {
- memset(buff, 0, sizeof(unsigned long) * SNMP_MIB_MAX);
+ memset(buff, 0, sizeof(buff));
snmp_get_cpu_field_batch(buff, itemlist, pcpumib);
for (i = 0; itemlist[i].name; i++)
@@ -218,7 +217,7 @@ static void snmp6_seq_show_item64(struct seq_file *seq, void __percpu *mib,
u64 buff64[SNMP_MIB_MAX];
int i;
- memset(buff64, 0, sizeof(u64) * SNMP_MIB_MAX);
+ memset(buff64, 0, sizeof(buff64));
snmp_get_cpu_field64_batch(buff64, itemlist, mib, syncpoff);
for (i = 0; itemlist[i].name; i++)
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 00/16] remove eight obsolete architectures
From: afzal mohammed @ 2018-03-16 4:50 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David Howells, linux-arch, Linux Kernel Mailing List,
open list:DOCUMENTATION, linux-block, IDE-ML,
open list:HID CORE LAYER, Networking, linux-wireless, linux-pwm,
linux-rtc, linux-spi, linux-usb, dri-devel, linux-fbdev,
linux-watchdog, Linux FS-devel Mailing List, Linux-MM
In-Reply-To: <CAK8P3a10hBz7QYk5v5MfhVMPOwFnWYTn95WZp1HtHrd7-GQpRg@mail.gmail.com>
Hi,
On Thu, Mar 15, 2018 at 10:56:48AM +0100, Arnd Bergmann wrote:
> On Thu, Mar 15, 2018 at 10:42 AM, David Howells <dhowells@redhat.com> wrote:
> > Do we have anything left that still implements NOMMU?
Please don't kill !MMU.
> Yes, plenty.
> I've made an overview of the remaining architectures for my own reference[1].
> The remaining NOMMU architectures are:
>
> - arch/arm has ARMv7-M (Cortex-M microcontroller), which is actually
> gaining traction
ARMv7-R as well, also seems ARM is coming up with more !MMU's - v8-M,
v8-R. In addition, though only of academic interest, ARM MMU capable
platform's can run !MMU Linux.
afzal
> - arch/sh has an open-source J2 core that was added not that long ago,
> it seems to
> be the only SH compatible core that anyone is working on.
> - arch/microblaze supports both MMU/NOMMU modes (most use an MMU)
> - arch/m68k supports several NOMMU targets, both the coldfire SoCs and the
> classic processors
> - c6x has no MMU
^ permalink raw reply
* Re: [PATCH v4 2/2] Remove false-positive VLAs when using max()
From: Nikolay Borisov @ 2018-03-16 7:52 UTC (permalink / raw)
To: Kees Cook, Andrew Morton
Cc: Linus Torvalds, Josh Poimboeuf, Rasmus Villemoes, Randy Dunlap,
Miguel Ojeda, Ingo Molnar, David Laight, Ian Abbott, linux-input,
linux-btrfs, netdev, linux-kernel, kernel-hardening
In-Reply-To: <1521143266-31350-3-git-send-email-keescook@chromium.org>
On 15.03.2018 21:47, Kees Cook wrote:
> As part of removing VLAs from the kernel[1], we want to build with -Wvla,
> but it is overly pessimistic and only accepts constant expressions for
> stack array sizes, instead of also constant values. The max() macro
> triggers the warning, so this refactors these uses of max() to use the
> new const_max() instead.
>
> [1] https://lkml.org/lkml/2018/3/7/621
For the btrfs portion :
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> drivers/input/touchscreen/cyttsp4_core.c | 2 +-
> fs/btrfs/tree-checker.c | 3 ++-
> lib/vsprintf.c | 4 ++--
> net/ipv4/proc.c | 8 ++++----
> net/ipv6/proc.c | 10 ++++------
> 5 files changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/input/touchscreen/cyttsp4_core.c b/drivers/input/touchscreen/cyttsp4_core.c
> index 727c3232517c..f89497940051 100644
> --- a/drivers/input/touchscreen/cyttsp4_core.c
> +++ b/drivers/input/touchscreen/cyttsp4_core.c
> @@ -868,7 +868,7 @@ static void cyttsp4_get_mt_touches(struct cyttsp4_mt_data *md, int num_cur_tch)
> struct cyttsp4_touch tch;
> int sig;
> int i, j, t = 0;
> - int ids[max(CY_TMA1036_MAX_TCH, CY_TMA4XX_MAX_TCH)];
> + int ids[const_max(CY_TMA1036_MAX_TCH, CY_TMA4XX_MAX_TCH)];
>
> memset(ids, 0, si->si_ofs.tch_abs[CY_TCH_T].max * sizeof(int));
> for (i = 0; i < num_cur_tch; i++) {
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index c3c8d48f6618..1ddd6cc3c4fc 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -341,7 +341,8 @@ static int check_dir_item(struct btrfs_root *root,
> */
> if (key->type == BTRFS_DIR_ITEM_KEY ||
> key->type == BTRFS_XATTR_ITEM_KEY) {
> - char namebuf[max(BTRFS_NAME_LEN, XATTR_NAME_MAX)];
> + char namebuf[const_max(BTRFS_NAME_LEN,
> + XATTR_NAME_MAX)];
>
> read_extent_buffer(leaf, namebuf,
> (unsigned long)(di + 1), name_len);
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index d7a708f82559..9d5610b643ce 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -744,8 +744,8 @@ char *resource_string(char *buf, char *end, struct resource *res,
> #define FLAG_BUF_SIZE (2 * sizeof(res->flags))
> #define DECODED_BUF_SIZE sizeof("[mem - 64bit pref window disabled]")
> #define RAW_BUF_SIZE sizeof("[mem - flags 0x]")
> - char sym[max(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
> - 2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];
> + char sym[const_max(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
> + 2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];
>
> char *p = sym, *pend = sym + sizeof(sym);
> int decode = (fmt[0] == 'R') ? 1 : 0;
> diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
> index dc5edc8f7564..fad6f989004e 100644
> --- a/net/ipv4/proc.c
> +++ b/net/ipv4/proc.c
> @@ -46,7 +46,7 @@
> #include <net/sock.h>
> #include <net/raw.h>
>
> -#define TCPUDP_MIB_MAX max_t(u32, UDP_MIB_MAX, TCP_MIB_MAX)
> +#define TCPUDP_MIB_MAX const_max(UDP_MIB_MAX, TCP_MIB_MAX)
>
> /*
> * Report socket allocation statistics [mea@utu.fi]
> @@ -404,7 +404,7 @@ static int snmp_seq_show_tcp_udp(struct seq_file *seq, void *v)
> struct net *net = seq->private;
> int i;
>
> - memset(buff, 0, TCPUDP_MIB_MAX * sizeof(unsigned long));
> + memset(buff, 0, sizeof(buff));
>
> seq_puts(seq, "\nTcp:");
> for (i = 0; snmp4_tcp_list[i].name; i++)
> @@ -421,7 +421,7 @@ static int snmp_seq_show_tcp_udp(struct seq_file *seq, void *v)
> seq_printf(seq, " %lu", buff[i]);
> }
>
> - memset(buff, 0, TCPUDP_MIB_MAX * sizeof(unsigned long));
> + memset(buff, 0, sizeof(buff));
>
> snmp_get_cpu_field_batch(buff, snmp4_udp_list,
> net->mib.udp_statistics);
> @@ -432,7 +432,7 @@ static int snmp_seq_show_tcp_udp(struct seq_file *seq, void *v)
> for (i = 0; snmp4_udp_list[i].name; i++)
> seq_printf(seq, " %lu", buff[i]);
>
> - memset(buff, 0, TCPUDP_MIB_MAX * sizeof(unsigned long));
> + memset(buff, 0, sizeof(buff));
>
> /* the UDP and UDP-Lite MIBs are the same */
> seq_puts(seq, "\nUdpLite:");
> diff --git a/net/ipv6/proc.c b/net/ipv6/proc.c
> index b67814242f78..58bbfc4fa7fa 100644
> --- a/net/ipv6/proc.c
> +++ b/net/ipv6/proc.c
> @@ -30,10 +30,8 @@
> #include <net/transp_v6.h>
> #include <net/ipv6.h>
>
> -#define MAX4(a, b, c, d) \
> - max_t(u32, max_t(u32, a, b), max_t(u32, c, d))
> -#define SNMP_MIB_MAX MAX4(UDP_MIB_MAX, TCP_MIB_MAX, \
> - IPSTATS_MIB_MAX, ICMP_MIB_MAX)
> +#define SNMP_MIB_MAX const_max(const_max(UDP_MIB_MAX, TCP_MIB_MAX), \
> + const_max(IPSTATS_MIB_MAX, ICMP_MIB_MAX))
>
> static int sockstat6_seq_show(struct seq_file *seq, void *v)
> {
> @@ -199,7 +197,7 @@ static void snmp6_seq_show_item(struct seq_file *seq, void __percpu *pcpumib,
> int i;
>
> if (pcpumib) {
> - memset(buff, 0, sizeof(unsigned long) * SNMP_MIB_MAX);
> + memset(buff, 0, sizeof(buff));
>
> snmp_get_cpu_field_batch(buff, itemlist, pcpumib);
> for (i = 0; itemlist[i].name; i++)
> @@ -218,7 +216,7 @@ static void snmp6_seq_show_item64(struct seq_file *seq, void __percpu *mib,
> u64 buff64[SNMP_MIB_MAX];
> int i;
>
> - memset(buff64, 0, sizeof(u64) * SNMP_MIB_MAX);
> + memset(buff64, 0, sizeof(buff64));
>
> snmp_get_cpu_field64_batch(buff64, itemlist, mib, syncpoff);
> for (i = 0; itemlist[i].name; i++)
>
^ permalink raw reply
* Re: [PATCH] Input: alps - Demystify trackstick initialization for v3 and v6 protocols
From: Pali Rohár @ 2018-03-16 10:58 UTC (permalink / raw)
To: Masaki Ota
Cc: Dmitry Torokhov, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <TYXPR01MB07191C487C66BA03E5D9AB3DC7D10@TYXPR01MB0719.jpnprd01.prod.outlook.com>
Great, thank you. Enabling that RAW/extended/SP4 mode is done by
sequence E6, E6, E6, F3, C8, F3, 14 (written in page 35) and it matches
what function alps_trackstick_enter_extended_mode_v3_v6() in my patch is
doing. So is correct.
On page 36 I see that there is described Method 1 for reporting stick
data which prevents cursor jumps. Seems that kernel uses Method 2.
Method 1 depends on some prioritization.
Do you have some information how to activate Method 1? Sometimes I
observe that problem with "cursor jumps" and from Method 1 could prevent
it. So I would like to try experimenting...
On Wednesday 14 March 2018 23:56:46 Masaki Ota wrote:
> Hi, Pali,
>
> I have added Appendix.
> According to this spec documents, SP raw mode is SP 4 byte mode.
> I think Extended mode meaning is almost all the same as Raw mode.
> The description of how to set is written in Page 35.
>
> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Thursday, March 15, 2018 7:58 AM
> To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Input: alps - Demystify trackstick initialization for v3 and v6 protocols
>
> Hi! Thank you for information.
>
> Your PS/2 Aux Port Control description seems to really matches. Just there is reverse order of bits. Bit 0 in description is highest, therefore matches BIT(7) macro.
>
> Bit 6 in description (BIT(1) in code) describes SP Extended Mode which alps.c enabled. And in that description is written:
>
> "If 1 SP is extended packet format (driver must set SP raw mode and GP absolute mode)."
>
> Do you have any idea what "SP raw mode" is? How to set it? For me it looks like it could be that extended mode of trackstick itself.
>
> "GP absolute mode" I guess is GlidePoint absolute mode, therefore enable
> 6 byte absolute mode for touchpad.
>
> And for Bit 7 (BIT(0)) is written:
>
> This bit is used with the PS/2 Aux port to use the Pass-Thru mode ( see appendix A ). Do you have some information about this appendix A?
>
> On Wednesday 14 March 2018 10:21:43 Masaki Ota wrote:
> > Hi, Pali,
> >
> > I just picked up the spec which relates with trackstic.
> >
> > Best Regards,
> > Masaki Ota
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Tuesday, March 13, 2018 8:14 AM
> > To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>; Dmitry Torokhov
> > <dmitry.torokhov@gmail.com>
> > Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] Input: alps - Demystify trackstick initialization
> > for v3 and v6 protocols
> >
> > Masaki, if you have access to the internal ALPS v3 / Rushmore
> > documentation, I would like to have a review of this patch or
> > confirmation of those information :-)
>
> --
> Pali Rohár
> pali.rohar@gmail.com
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Florian Weimer @ 2018-03-16 11:47 UTC (permalink / raw)
To: Kees Cook, Andrew Morton
Cc: Linus Torvalds, Josh Poimboeuf, Rasmus Villemoes, Randy Dunlap,
Miguel Ojeda, Ingo Molnar, David Laight, Ian Abbott, linux-input,
linux-btrfs, netdev, linux-kernel, kernel-hardening
In-Reply-To: <1521174359-46392-1-git-send-email-keescook@chromium.org>
On 03/16/2018 05:25 AM, Kees Cook wrote:
> In the effort to remove all VLAs from the kernel[1], it is desirable to
> build with -Wvla. However, this warning is overly pessimistic, in that
> it is only happy with stack array sizes that are declared as constant
> expressions, and not constant values. One case of this is the evaluation
> of the max() macro which, due to its construction, ends up converting
> constant expression arguments into a constant value result. Attempts
> to adjust the behavior of max() ran afoul of version-dependent compiler
> behavior[2].
I find this commit message confusing. VLAs have precisely defined
semantics which differ from other arrays, and these differences can be
observable (maybe not in the kernel, but certainly for userspace), so
the compiler has to treat a VLA as such even if the length is a constant
known at compile time. (The original intent of the warning probably was
a portability check anyway.)
If you want to catch stack frames which have unbounded size,
-Werror=stack-usage=1000 or -Werror=vla-larger-than=1000 (with the
constant adjusted as needed) might be the better approach.
Thanks,
Florian
^ permalink raw reply
* Re: [RESEND PATCH v5] input: pxrc: new driver for PhoenixRC Flight Controller Adapter
From: Marcus Folkesson @ 2018-03-16 13:39 UTC (permalink / raw)
To: Dmitry Torokhov, Jonathan Corbet; +Cc: linux-input, linux-doc, linux-kernel
In-Reply-To: <20180218161747.21110-1-marcus.folkesson@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 13068 bytes --]
ping.
I do not want to nag, but would someone please have a look at this?
Thanks,
Marcus Folkesson
On Sun, Feb 18, 2018 at 05:17:46PM +0100, Marcus Folkesson wrote:
> This driver let you plug in your RC controller to the adapter and
> use it as input device in various RC simulators.
>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
>
> v5:
> - Drop autosuspend support
> - Use pm_mutex instead of input_dev->mutex
> - Use pxrc->is_open instead of input_dev->users
> v4:
> - Add call to usb_mark_last_busy() in irq
> - Move code from pxrc_resume() to pxrc_reset_resume()
> v3:
> - Use RUDDER and MISC instead of TILT_X and TILT_Y
> - Drop kref and anchor
> - Rework URB handling
> - Add PM support
> v2:
> - Change module license to GPLv2 to match SPDX tag
>
>
> Documentation/input/devices/pxrc.rst | 57 +++++++
> drivers/input/joystick/Kconfig | 9 ++
> drivers/input/joystick/Makefile | 1 +
> drivers/input/joystick/pxrc.c | 303 +++++++++++++++++++++++++++++++++++
> 4 files changed, 370 insertions(+)
> create mode 100644 Documentation/input/devices/pxrc.rst
> create mode 100644 drivers/input/joystick/pxrc.c
>
> diff --git a/Documentation/input/devices/pxrc.rst b/Documentation/input/devices/pxrc.rst
> new file mode 100644
> index 000000000000..ca11f646bae8
> --- /dev/null
> +++ b/Documentation/input/devices/pxrc.rst
> @@ -0,0 +1,57 @@
> +=======================================================
> +pxrc - PhoenixRC Flight Controller Adapter
> +=======================================================
> +
> +:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> +
> +This driver let you use your own RC controller plugged into the
> +adapter that comes with PhoenixRC [1]_ or other compatible adapters.
> +
> +The adapter supports 7 analog channels and 1 digital input switch.
> +
> +Notes
> +=====
> +
> +Many RC controllers is able to configure which stick goes to which channel.
> +This is also configurable in most simulators, so a matching is not necessary.
> +
> +The driver is generating the following input event for analog channels:
> +
> ++---------+----------------+
> +| Channel | Event |
> ++=========+================+
> +| 1 | ABS_X |
> ++---------+----------------+
> +| 2 | ABS_Y |
> ++---------+----------------+
> +| 3 | ABS_RX |
> ++---------+----------------+
> +| 4 | ABS_RY |
> ++---------+----------------+
> +| 5 | ABS_RUDDER |
> ++---------+----------------+
> +| 6 | ABS_THROTTLE |
> ++---------+----------------+
> +| 7 | ABS_MISC |
> ++---------+----------------+
> +
> +The digital input switch is generated as an `BTN_A` event.
> +
> +Manual Testing
> +==============
> +
> +To test this driver's functionality you may use `input-event` which is part of
> +the `input layer utilities` suite [2]_.
> +
> +For example::
> +
> + > modprobe pxrc
> + > input-events <devnr>
> +
> +To print all input events from input `devnr`.
> +
> +References
> +==========
> +
> +.. [1] http://www.phoenix-sim.com/
> +.. [2] https://www.kraxel.org/cgit/input/
> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index f3c2f6ea8b44..332c0cc1b2ab 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -351,4 +351,13 @@ config JOYSTICK_PSXPAD_SPI_FF
>
> To drive rumble motor a dedicated power supply is required.
>
> +config JOYSTICK_PXRC
> + tristate "PhoenixRC Flight Controller Adapter"
> + depends on USB_ARCH_HAS_HCD
> + depends on USB
> + help
> + Say Y here if you want to use the PhoenixRC Flight Controller Adapter.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called pxrc.
> endif
> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> index 67651efda2e1..dd0492ebbed7 100644
> --- a/drivers/input/joystick/Makefile
> +++ b/drivers/input/joystick/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_JOYSTICK_JOYDUMP) += joydump.o
> obj-$(CONFIG_JOYSTICK_MAGELLAN) += magellan.o
> obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o
> obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
> +obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
> obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
> obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
> obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o
> diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
> new file mode 100644
> index 000000000000..07a0dbd3ced2
> --- /dev/null
> +++ b/drivers/input/joystick/pxrc.c
> @@ -0,0 +1,303 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for Phoenix RC Flight Controller Adapter
> + *
> + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/uaccess.h>
> +#include <linux/usb.h>
> +#include <linux/usb/input.h>
> +#include <linux/mutex.h>
> +#include <linux/input.h>
> +
> +#define PXRC_VENDOR_ID (0x1781)
> +#define PXRC_PRODUCT_ID (0x0898)
> +
> +static const struct usb_device_id pxrc_table[] = {
> + { USB_DEVICE(PXRC_VENDOR_ID, PXRC_PRODUCT_ID) },
> + { }
> +};
> +MODULE_DEVICE_TABLE(usb, pxrc_table);
> +
> +struct pxrc {
> + struct input_dev *input;
> + struct usb_device *udev;
> + struct usb_interface *intf;
> + struct urb *urb;
> + struct mutex pm_mutex;
> + bool is_open;
> + __u8 epaddr;
> + char phys[64];
> + unsigned char *data;
> + size_t bsize;
> +};
> +
> +static void pxrc_usb_irq(struct urb *urb)
> +{
> + struct pxrc *pxrc = urb->context;
> + int error;
> +
> + switch (urb->status) {
> + case 0:
> + /* success */
> + break;
> + case -ETIME:
> + /* this urb is timing out */
> + dev_dbg(&pxrc->intf->dev,
> + "%s - urb timed out - was the device unplugged?\n",
> + __func__);
> + return;
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + case -EPIPE:
> + /* this urb is terminated, clean up */
> + dev_dbg(&pxrc->intf->dev, "%s - urb shutting down with status: %d\n",
> + __func__, urb->status);
> + return;
> + default:
> + dev_dbg(&pxrc->intf->dev, "%s - nonzero urb status received: %d\n",
> + __func__, urb->status);
> + goto exit;
> + }
> +
> + if (urb->actual_length == 8) {
> + input_report_abs(pxrc->input, ABS_X, pxrc->data[0]);
> + input_report_abs(pxrc->input, ABS_Y, pxrc->data[2]);
> + input_report_abs(pxrc->input, ABS_RX, pxrc->data[3]);
> + input_report_abs(pxrc->input, ABS_RY, pxrc->data[4]);
> + input_report_abs(pxrc->input, ABS_RUDDER, pxrc->data[5]);
> + input_report_abs(pxrc->input, ABS_THROTTLE, pxrc->data[6]);
> + input_report_abs(pxrc->input, ABS_MISC, pxrc->data[7]);
> +
> + input_report_key(pxrc->input, BTN_A, pxrc->data[1]);
> + }
> +
> +exit:
> + /* Resubmit to fetch new fresh URBs */
> + error = usb_submit_urb(urb, GFP_ATOMIC);
> + if (error && error != -EPERM)
> + dev_err(&pxrc->intf->dev,
> + "%s - usb_submit_urb failed with result: %d",
> + __func__, error);
> +}
> +
> +static int pxrc_open(struct input_dev *input)
> +{
> + struct pxrc *pxrc = input_get_drvdata(input);
> + int retval;
> +
> + mutex_lock(&pxrc->pm_mutex);
> + retval = usb_submit_urb(pxrc->urb, GFP_KERNEL);
> + if (retval) {
> + dev_err(&pxrc->intf->dev,
> + "%s - usb_submit_urb failed, error: %d\n",
> + __func__, retval);
> + retval = -EIO;
> + goto out;
> + }
> +
> + pxrc->is_open = true;
> +
> +out:
> + mutex_unlock(&pxrc->pm_mutex);
> + return retval;
> +}
> +
> +static void pxrc_close(struct input_dev *input)
> +{
> + struct pxrc *pxrc = input_get_drvdata(input);
> +
> + mutex_lock(&pxrc->pm_mutex);
> + usb_kill_urb(pxrc->urb);
> + pxrc->is_open = false;
> + mutex_unlock(&pxrc->pm_mutex);
> +}
> +
> +static int pxrc_usb_init(struct pxrc *pxrc)
> +{
> + struct usb_endpoint_descriptor *epirq;
> + unsigned int pipe;
> + int retval;
> +
> + /* Set up the endpoint information */
> + /* This device only has an interrupt endpoint */
> + retval = usb_find_common_endpoints(pxrc->intf->cur_altsetting,
> + NULL, NULL, &epirq, NULL);
> + if (retval) {
> + dev_err(&pxrc->intf->dev,
> + "Could not find endpoint\n");
> + goto error;
> + }
> +
> + pxrc->bsize = usb_endpoint_maxp(epirq);
> + pxrc->epaddr = epirq->bEndpointAddress;
> + pxrc->data = devm_kmalloc(&pxrc->intf->dev, pxrc->bsize, GFP_KERNEL);
> + if (!pxrc->data) {
> + retval = -ENOMEM;
> + goto error;
> + }
> +
> + usb_set_intfdata(pxrc->intf, pxrc);
> + usb_make_path(pxrc->udev, pxrc->phys, sizeof(pxrc->phys));
> + strlcat(pxrc->phys, "/input0", sizeof(pxrc->phys));
> +
> + pxrc->urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!pxrc->urb) {
> + retval = -ENOMEM;
> + goto error;
> + }
> +
> + pipe = usb_rcvintpipe(pxrc->udev, pxrc->epaddr),
> + usb_fill_int_urb(pxrc->urb, pxrc->udev, pipe, pxrc->data, pxrc->bsize,
> + pxrc_usb_irq, pxrc, 1);
> +
> +error:
> + return retval;
> +
> +
> +}
> +
> +static int pxrc_input_init(struct pxrc *pxrc)
> +{
> + pxrc->input = devm_input_allocate_device(&pxrc->intf->dev);
> + if (pxrc->input == NULL) {
> + dev_err(&pxrc->intf->dev, "couldn't allocate input device\n");
> + return -ENOMEM;
> + }
> +
> + pxrc->input->name = "PXRC Flight Controller Adapter";
> + pxrc->input->phys = pxrc->phys;
> + usb_to_input_id(pxrc->udev, &pxrc->input->id);
> +
> + pxrc->input->open = pxrc_open;
> + pxrc->input->close = pxrc_close;
> +
> + input_set_capability(pxrc->input, EV_KEY, BTN_A);
> + input_set_abs_params(pxrc->input, ABS_X, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_Y, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_RX, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_RY, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_RUDDER, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_THROTTLE, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_MISC, 0, 255, 0, 0);
> +
> + input_set_drvdata(pxrc->input, pxrc);
> +
> + return input_register_device(pxrc->input);
> +}
> +
> +static int pxrc_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + struct pxrc *pxrc;
> + int retval;
> +
> + pxrc = devm_kzalloc(&intf->dev, sizeof(*pxrc), GFP_KERNEL);
> + if (!pxrc)
> + return -ENOMEM;
> +
> + mutex_init(&pxrc->pm_mutex);
> + pxrc->udev = usb_get_dev(interface_to_usbdev(intf));
> + pxrc->intf = intf;
> +
> + retval = pxrc_usb_init(pxrc);
> + if (retval)
> + goto error;
> +
> + retval = pxrc_input_init(pxrc);
> + if (retval)
> + goto err_free_urb;
> +
> + return 0;
> +
> +err_free_urb:
> + usb_free_urb(pxrc->urb);
> +
> +error:
> + return retval;
> +}
> +
> +static void pxrc_disconnect(struct usb_interface *intf)
> +{
> + struct pxrc *pxrc = usb_get_intfdata(intf);
> +
> + usb_free_urb(pxrc->urb);
> + usb_set_intfdata(intf, NULL);
> +}
> +
> +static int pxrc_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> + struct pxrc *pxrc = usb_get_intfdata(intf);
> +
> + mutex_lock(&pxrc->pm_mutex);
> + if (pxrc->is_open)
> + usb_kill_urb(pxrc->urb);
> + mutex_unlock(&pxrc->pm_mutex);
> +
> + return 0;
> +}
> +
> +static int pxrc_resume(struct usb_interface *intf)
> +{
> + struct pxrc *pxrc = usb_get_intfdata(intf);
> + int retval = 0;
> +
> + mutex_lock(&pxrc->pm_mutex);
> + if (pxrc->is_open && usb_submit_urb(pxrc->urb, GFP_KERNEL) < 0)
> + retval = -EIO;
> +
> + mutex_unlock(&pxrc->pm_mutex);
> + return retval;
> +}
> +
> +static int pxrc_pre_reset(struct usb_interface *intf)
> +{
> + struct pxrc *pxrc = usb_get_intfdata(intf);
> +
> + mutex_lock(&pxrc->pm_mutex);
> + usb_kill_urb(pxrc->urb);
> + return 0;
> +}
> +
> +static int pxrc_post_reset(struct usb_interface *intf)
> +{
> + struct pxrc *pxrc = usb_get_intfdata(intf);
> + int retval = 0;
> +
> + if (pxrc->is_open && usb_submit_urb(pxrc->urb, GFP_KERNEL) < 0)
> + retval = -EIO;
> +
> + mutex_unlock(&pxrc->pm_mutex);
> +
> + return retval;
> +}
> +
> +static int pxrc_reset_resume(struct usb_interface *intf)
> +{
> + return pxrc_resume(intf);
> +}
> +
> +static struct usb_driver pxrc_driver = {
> + .name = "pxrc",
> + .probe = pxrc_probe,
> + .disconnect = pxrc_disconnect,
> + .id_table = pxrc_table,
> + .suspend = pxrc_suspend,
> + .resume = pxrc_resume,
> + .pre_reset = pxrc_pre_reset,
> + .post_reset = pxrc_post_reset,
> + .reset_resume = pxrc_reset_resume,
> +};
> +
> +module_usb_driver(pxrc_driver);
> +
> +MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@gmail.com>");
> +MODULE_DESCRIPTION("PhoenixRC Flight Controller Adapter");
> +MODULE_LICENSE("GPL v2");
> --
> 2.15.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: Fix deadlocks in autosuspend
From: Marcus Folkesson @ 2018-03-16 13:41 UTC (permalink / raw)
To: Dmitry Torokhov, Arvind Yadav; +Cc: linux-input, linux-kernel
In-Reply-To: <20180228133803.30040-1-marcus.folkesson@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3659 bytes --]
Ping.
Would someone please have a look?
Thanks,
Marcus Folkesson
On Wed, Feb 28, 2018 at 02:37:57PM +0100, Marcus Folkesson wrote:
> Hello,
>
> I have not recieved any feedback on these so I resend them.
>
> I got this deadlock on my own driver (pxrc) when using the same
> construction.
>
> Please have a look
>
> Here is a clip from
> [PATCH v3] input: pxrc: new driver for PhoenixRC Flight Controller Adapter [1]
> that describes the problem.
>
> -------------------------------8<----------------------------------------------
> Also, I think we have a deadlock in the synaptics_usb driver.
>
> When the device is suspended and someone is open the device, the input
> subsystem will call input_open_device() which takes the
> input_dev->mutex and then call input_dev->open().
>
> synusb_open() has a call to usb_autopm_get_interface() which will
> result in a call to the registered resume-function if the device is
> suspended. (see Documentation/driver-api/usb/power-manaement.rst).
>
> In the case of snaptics_usb, it will take the input_dev->mutex in the
> resume function.
>
> I have no synaptic mouse, but tested to put the same code into my
> driver just to confirm, and got the following dump:
>
> [ 9215.626476] INFO: task input-events:8590 blocked for more than 120 seconds.
> [ 9215.626495] Not tainted 4.15.0-rc8-ARCH+ #6
> [ 9215.626500] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> [ 9215.626507] input-events D 0 8590 4394 0x00000004
> [ 9215.626520] Call Trace:
> [ 9215.626546] ? __schedule+0x236/0x850
> [ 9215.626559] schedule+0x2f/0x90
> [ 9215.626569] schedule_preempt_disabled+0x11/0x20
> [ 9215.626579] __mutex_lock.isra.0+0x1aa/0x520
> [ 9215.626609] ? usb_runtime_suspend+0x70/0x70 [usbcore]
> [ 9215.626622] ? pxrc_resume+0x37/0x70 [pxrc]
> [ 9215.626632] pxrc_resume+0x37/0x70 [pxrc]
> [ 9215.626655] usb_resume_interface.isra.2+0x39/0xe0 [usbcore]
> [ 9215.626676] usb_resume_both+0xd2/0x120 [usbcore]
> [ 9215.626688] __rpm_callback+0xb6/0x1f0
> [ 9215.626699] rpm_callback+0x1f/0x70
> [ 9215.626718] ? usb_runtime_suspend+0x70/0x70 [usbcore]
> [ 9215.626726] rpm_resume+0x4e2/0x7f0
> [ 9215.626737] rpm_resume+0x582/0x7f0
> [ 9215.626749] __pm_runtime_resume+0x3a/0x50
> [ 9215.626767] usb_autopm_get_interface+0x1d/0x50 [usbcore]
> [ 9215.626780] pxrc_open+0x17/0x8d [pxrc]
> [ 9215.626791] input_open_device+0x70/0xa0
> [ 9215.626804] evdev_open+0x183/0x1c0 [evdev]
> [ 9215.626819] chrdev_open+0xa0/0x1b0
> [ 9215.626830] ? cdev_put.part.1+0x20/0x20
> [ 9215.626840] do_dentry_open+0x1ad/0x2c0
> [ 9215.626855] path_openat+0x576/0x1300
> [ 9215.626868] ? alloc_set_pte+0x22c/0x520
> [ 9215.626883] ? filemap_map_pages+0x19b/0x340
> [ 9215.626893] do_filp_open+0x9b/0x110
> [ 9215.626908] ? __check_object_size+0x9d/0x190
> [ 9215.626920] ? __alloc_fd+0xaf/0x160
> [ 9215.626931] ? do_sys_open+0x1bd/0x250
> [ 9215.626942] do_sys_open+0x1bd/0x250
> [ 9215.626956] entry_SYSCALL_64_fastpath+0x20/0x83
> [ 9215.626967] RIP: 0033:0x7fbf6358f7ae
>
>
> tablet/pegasus_notetaker.c and touchscreen/usbtouchscreen.c has the same
> construction (taking input_dev->mutex in resume/suspend and call
> usb_autopm_get_interface() in open()).
>
> I will create a separate "pm_mutex" to use instead of input_dev->mutex
> to get rid of the lockups in those drivers
>
> -------------------------------8<----------------------------------------------
>
>
> [1] https://lkml.org/lkml/2018/1/20/191
>
> Thanks,
>
> Best regards
> Marcus Folkesson
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RESEND PATCH] Input: joystick/analog - Use get_cycles() on PPC
From: Michael Ellerman @ 2018-03-16 14:00 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, linuxppc-dev
In-Reply-To: <20180314163913.GC18065@dtor-ws>
Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> On Wed, Mar 14, 2018 at 10:17:52PM +1100, Michael Ellerman wrote:
>> The analog joystick driver spits a warning at us:
>>
>> drivers/input/joystick/analog.c:176:2: warning: #warning Precise timer
>> not defined for this architecture.
>>
>> PPC has get_cycles() so use that.
>>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>
> Applied, thank you.
Thanks.
cheers
^ permalink raw reply
* Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal
From: Rasmus Villemoes @ 2018-03-16 14:15 UTC (permalink / raw)
To: Linus Torvalds, Kees Cook
Cc: Andrew Morton, Josh Poimboeuf, Rasmus Villemoes, Randy Dunlap,
Miguel Ojeda, Ingo Molnar, David Laight, Ian Abbott, linux-input,
linux-btrfs, Network Development, Linux Kernel Mailing List,
Kernel Hardening
In-Reply-To: <CA+55aFyRV9KXzeQZpVYsZYVUJm-ASgu_4_1+8Y8-0KH-YT2M8Q@mail.gmail.com>
On 2018-03-16 00:46, Linus Torvalds wrote:
> On Thu, Mar 15, 2018 at 4:41 PM, Kees Cook <keescook@chromium.org> wrote:
>>
>> I much prefer explicit typing, but both you and Rasmus mentioned
>> wanting the int/sizeof_t mixing.
>
> Well, the explicit typing allows that mixing, in that you can just
> have "const_max_t(5,sizeof(x))"
>
> So I'm ok with that.
>
> What I'm *not* so much ok with is "const_max(5,sizeof(x))" erroring
> out, or silently causing insane behavior due to hidden subtle type
> casts..
I don't like const_max_t, at least not as the "primary" interface -
forcing the user to pass in a type, or equivalently passing in cast
expressions to a const_max(), can hide errors, e.g. if the -1 is really
SOME_MACRO or some complicated expression that is usually positive, but
that expression always gets cast to size_t because the user was forced to do
const_max_t(size_t, SOME_MACRO, sizeof(foo))
to make the code compile. Not to mention that it's both easier to read
and write if one could just do
const_max(SOME_MACRO, sizeof(foo))
Can we instead do one of the following:
(1) Effectively do the comparison in an infinitely wide signed integer,
i.e. implement
x < 0 && y >= 0 --> y
x >= 0 && y < 0 --> x
otherwise, if both have the same sign (but not necessarily the same
signedness of their types), the type promotions do not alter either's
value, so __builtin_choose_expr(x > y, x, y) will do the right thing
with the resulting thing having the same type as the chosen one of x and
y. [Or having type typeof(x+y), which would just be a cast in the
macro.] This would allow const_max(-1, sizeof(foo)) and give
sizeof(foo), but perhaps that's too magic.
(2) Allow mixed types, but ensure the build fails if one of the values
is not representable in typeof(x+y) (i.e., one value is negative but the
common type is unsigned). That allows the const_max(SOME_MACRO,
sizeof()), but prevents silent failure in case some weird combination of
CONFIG options make SOME_MACRO evaluate to something negative.
The user can always pass in (size_t)-1 explicitly if needed, or cast the
sizeof() to int if that's what makes sense, but that's a case-by-case
thing. I'd really like that the simple case
const_max(16, sizeof(foo))
Just Works. Then if a lot users turn up that do need some casting,
const_max_t can be implemented as a trivial const_max wrapper.
Rasmus
(1) something like __builtin_choose_expr((x >= 0 && y < 0) || \
(x >= 0 && y >= 0 && x > y) || \
(x < 0 && y < 0 && x > y), x, y)
(2) something like
// 1 or build error
#define __check_promotion(t, x) ( 1/(((t)(x) < 0) == ((x) < 0)) )
__builtin_choose_expr(__check_promotion(typeof((x)+(y)), x) && \
__check_promotion(typeof((x)+(y)), y) && \
(x) > (y), x, y)
Not sure how to get a more sensible error message, I'd like this to also
work outside functions.
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Linus Torvalds @ 2018-03-16 17:29 UTC (permalink / raw)
To: Florian Weimer
Cc: Kees Cook, Andrew Morton, Josh Poimboeuf, Rasmus Villemoes,
Randy Dunlap, Miguel Ojeda, Ingo Molnar, David Laight, Ian Abbott,
linux-input, linux-btrfs, Network Development,
Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <ab2933a2-ca7b-eb3b-744d-bc037b1be688@redhat.com>
On Fri, Mar 16, 2018 at 4:47 AM, Florian Weimer <fweimer@redhat.com> wrote:
>
> If you want to catch stack frames which have unbounded size,
> -Werror=stack-usage=1000 or -Werror=vla-larger-than=1000 (with the constant
> adjusted as needed) might be the better approach.
No, we want to catch *variable* stack sizes.
Does "-Werror=vla-larger-than=0" perhaps work for that? No, because
the stupid compiler says that is "meaningless".
And no, using "-Werror=vla-larger-than=1" doesn't work either, because
the moronic compiler continues to think that "vla" is about the
_type_, not the code:
t.c: In function ‘test’:
t.c:6:6: error: argument to variable-length array is too large
[-Werror=vla-larger-than=]
int array[(1,100)];
Gcc people are crazy.
Is there really no way to just say "shut up about the stupid _syntax_
issue that is entirely irrelevant, and give us the _code_ issue".
Linus
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Florian Weimer @ 2018-03-16 17:32 UTC (permalink / raw)
To: Linus Torvalds
Cc: Kees Cook, Andrew Morton, Josh Poimboeuf, Rasmus Villemoes,
Randy Dunlap, Miguel Ojeda, Ingo Molnar, David Laight, Ian Abbott,
linux-input, linux-btrfs, Network Development,
Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CA+55aFx92dXg-qnrjGS2Rsna6TE5HSPBgGpSA3cv4n_n3RqBzA@mail.gmail.com>
On 03/16/2018 06:29 PM, Linus Torvalds wrote:
> Gcc people are crazy.
End of discussion from me. This is not acceptable.
Florian
^ permalink raw reply
* RE: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: David Laight @ 2018-03-16 17:44 UTC (permalink / raw)
To: 'Linus Torvalds', Florian Weimer
Cc: Kees Cook, Andrew Morton, Josh Poimboeuf, Rasmus Villemoes,
Randy Dunlap, Miguel Ojeda, Ingo Molnar, Ian Abbott, linux-input,
linux-btrfs, Network Development, Linux Kernel Mailing List,
Kernel Hardening
In-Reply-To: <CA+55aFx92dXg-qnrjGS2Rsna6TE5HSPBgGpSA3cv4n_n3RqBzA@mail.gmail.com>
From: Linus Torvalds
> Sent: 16 March 2018 17:29
> On Fri, Mar 16, 2018 at 4:47 AM, Florian Weimer <fweimer@redhat.com> wrote:
> >
> > If you want to catch stack frames which have unbounded size,
> > -Werror=stack-usage=1000 or -Werror=vla-larger-than=1000 (with the constant
> > adjusted as needed) might be the better approach.
>
> No, we want to catch *variable* stack sizes.
>
> Does "-Werror=vla-larger-than=0" perhaps work for that? No, because
> the stupid compiler says that is "meaningless".
>
> And no, using "-Werror=vla-larger-than=1" doesn't work either, because
> the moronic compiler continues to think that "vla" is about the
> _type_, not the code:
>
> t.c: In function ‘test’:
> t.c:6:6: error: argument to variable-length array is too large
> [-Werror=vla-larger-than=]
> int array[(1,100)];
>
> Gcc people are crazy.
>
> Is there really no way to just say "shut up about the stupid _syntax_
> issue that is entirely irrelevant, and give us the _code_ issue".
I looked at the generated code for one of the constant sized VLA that
the compiler barfed at.
It seemed to subtract constants from %sp separately for the VLA.
So it looks like the compiler treats them as VLA even though it
knows the size.
That is probably missing optimisation.
David
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Al Viro @ 2018-03-16 17:55 UTC (permalink / raw)
To: Linus Torvalds
Cc: Florian Weimer, Kees Cook, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Miguel Ojeda, Ingo Molnar,
David Laight, Ian Abbott, linux-input, linux-btrfs,
Network Development, Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CA+55aFx92dXg-qnrjGS2Rsna6TE5HSPBgGpSA3cv4n_n3RqBzA@mail.gmail.com>
On Fri, Mar 16, 2018 at 10:29:16AM -0700, Linus Torvalds wrote:
> t.c: In function ‘test’:
> t.c:6:6: error: argument to variable-length array is too large
> [-Werror=vla-larger-than=]
> int array[(1,100)];
>
> Gcc people are crazy.
That's not them, that's C standard regarding ICE. 1,100 is *not* a
constant expression as far as the standard is concerned, and that
type is actually a VLA with the size that can be optimized into
a compiler-calculated value.
Would you argue that in
void foo(char c)
{
int a[(c<<1) + 10 - c + 2 - c];
a is not a VLA? Sure, compiler probably would be able to reduce
that expression to 12, but demanding that to be recognized means
that compiler must do a bunch of optimizations in the middle of
typechecking.
expr, constant_expression is not a constant_expression. And in
this particular case the standard is not insane - the only reason
for using that is typechecking and _that_ can be achieved without
violating 6.6p6:
sizeof(expr,0) * 0 + ICE
*is* an integer constant expression, and it gives you exact same
typechecking. So if somebody wants to play odd games, they can
do that just fine, without complicating the logics for compilers...
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Al Viro @ 2018-03-16 18:14 UTC (permalink / raw)
To: Linus Torvalds
Cc: Florian Weimer, Kees Cook, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Miguel Ojeda, Ingo Molnar,
David Laight, Ian Abbott, linux-input, linux-btrfs,
Network Development, Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <20180316175502.GE30522@ZenIV.linux.org.uk>
On Fri, Mar 16, 2018 at 05:55:02PM +0000, Al Viro wrote:
> On Fri, Mar 16, 2018 at 10:29:16AM -0700, Linus Torvalds wrote:
> > t.c: In function ‘test’:
> > t.c:6:6: error: argument to variable-length array is too large
> > [-Werror=vla-larger-than=]
> > int array[(1,100)];
> >
> > Gcc people are crazy.
>
> That's not them, that's C standard regarding ICE. 1,100 is *not* a
> constant expression as far as the standard is concerned, and that
> type is actually a VLA with the size that can be optimized into
> a compiler-calculated value.
>
> Would you argue that in
s/argue/agree/, sorry
> void foo(char c)
> {
> int a[(c<<1) + 10 - c + 2 - c];
>
> a is not a VLA?
FWIW, 6.6 starts with
constant-expression:
conditional-expression
for syntax, with 6.6p3 being "Constant expression shall not contain
assignment, increment, decrement, function call or comma operators,
except when they are contained in a subexpression that is not evaluated",
with "The operand of sizeof operator is usually not evaluated (6.5.3.4)"
as a footnote.
6.6p10 allows implementation to accept other forms of constant expressions,
but arguing that such-and-such construct surely must be recognized as one,
when there are perfectly portable ways to achieve the same...
Realistically, code like that can come only from macros, and one can wrap
the damn thing into 0 * sizeof(..., 0) + just fine there. Which will
satisfy the conditions for sizeof argument not being evaluated...
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Linus Torvalds @ 2018-03-16 19:27 UTC (permalink / raw)
To: Al Viro
Cc: Florian Weimer, Kees Cook, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Miguel Ojeda, Ingo Molnar,
David Laight, Ian Abbott, linux-input, linux-btrfs,
Network Development, Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <20180316175502.GE30522@ZenIV.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 3975 bytes --]
On Fri, Mar 16, 2018 at 10:55 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> That's not them, that's C standard regarding ICE.
Yes. The C standard talks about "integer constant expression". I know.
It's come up in this very thread before.
The C standard at no point talks about - or forbids - "variable length
arrays". That never comes up in the whole standard, I checked.
So we are right now hindered by a _syntactic_ check, without any way
to have a _semantic_ check.
That's my problem. The warnings are misleading and imply semantics.
And apparently there is no way to actually check semantics.
> 1,100 is *not* a constant expression as far as the standard is concerned,
I very much know.
But it sure isn't "variable" either as far as the standard is
concerned, because the standard doesn't even have that concept (it
uses "variable" for argument numbers and for variables).
So being pedantic doesn't really change anything.
> Would you argue that in
> void foo(char c)
> {
> int a[(c<<1) + 10 - c + 2 - c];
Yeah, I don't think that even counts as a constant value, even if it
can be optimized to one. I would not at all be unhppy to see
__builtin_constant_p() to return zero.
But that is very much different from the syntax issue.
So I would like to get some way to get both type-checking and constant
checking without the annoying syntax issue.
> expr, constant_expression is not a constant_expression. And in
> this particular case the standard is not insane - the only reason
> for using that is typechecking and _that_ can be achieved without
> violating 6.6p6:
> sizeof(expr,0) * 0 + ICE
> *is* an integer constant expression, and it gives you exact same
> typechecking. So if somebody wants to play odd games, they can
> do that just fine, without complicating the logics for compilers...
Now that actually looks like a good trick. Maybe we can use that
instead of the comma expression that causes problems.
And using sizeof() to make sure that __builtin_choose_expression()
really gets an integer constant expression and that there should be no
ambiguity looks good.
Hmm.
This works for me, and I'm being *very* careful (those casts to
pointer types are inside that sizeof, because it's not an integral
type, and non-integral casts are not valid in an ICE either) but
somebody needs to check gcc-4.4:
#define __typecheck(a,b) \
(!!(sizeof((typeof(a)*)1==(typeof(b)*)1)))
#define __no_side_effects(a,b) \
(__builtin_constant_p(a)&&__builtin_constant_p(b))
#define __safe_cmp(a,b) \
(__typecheck(a,b) && __no_side_effects(a,b))
#define __cmp(a,b,op) ((a)op(b)?(a):(b))
#define __cmp_once(a,b,op) ({ \
typeof(a) __a = (a); \
typeof(b) __b = (b); \
__cmp(__a,__b,op); })
#define __careful_cmp(a,b,op) \
__builtin_choose_expr(__safe_cmp(a,b), __cmp(a,b,op),
__cmp_once(a,b,op))
#define min(a,b) __careful_cmp(a,b,<)
#define max(a,b) __careful_cmp(a,b,>)
#define min_t(t,a,b) __careful_cmp((t)(a),(t)(b),<)
#define max_t(t,a,b) __careful_cmp((t)(a),(t)(b),>)
and yes, it does cause new warnings for that
comparison between ‘enum tis_defaults’ and ‘enum tpm2_const’
in drivers/char/tpm/tpm_tis_core.h due to
#define TIS_TIMEOUT_A_MAX max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_A)
but technically that warning is actually correct, I'm just confused
why gcc cares about the cast placement or something.
That warning is easy to fix by turning it into a "max_t(int, enum1,
enum2)' and that is technically the right thing to do, it's just not
warned about for some odd reason with the current code.
Kees - is there some online "gcc-4.4 checker" somewhere? This does
seem to work with my gcc. I actually tested some of those files you
pointed at now.
Linus
[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2965 bytes --]
include/linux/kernel.h | 77 +++++++++++++-------------------------------------
1 file changed, 20 insertions(+), 57 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3fd291503576..23c31bf1d7fb 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -787,37 +787,29 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
* strict type-checking.. See the
* "unnecessary" pointer comparison.
*/
-#define __min(t1, t2, min1, min2, x, y) ({ \
- t1 min1 = (x); \
- t2 min2 = (y); \
- (void) (&min1 == &min2); \
- min1 < min2 ? min1 : min2; })
+#define __typecheck(a,b) \
+ (!!(sizeof((typeof(a)*)1==(typeof(b)*)1)))
-/**
- * min - return minimum of two values of the same or compatible types
- * @x: first value
- * @y: second value
- */
-#define min(x, y) \
- __min(typeof(x), typeof(y), \
- __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \
- x, y)
+#define __no_side_effects(a,b) \
+ (__builtin_constant_p(a)&&__builtin_constant_p(b))
-#define __max(t1, t2, max1, max2, x, y) ({ \
- t1 max1 = (x); \
- t2 max2 = (y); \
- (void) (&max1 == &max2); \
- max1 > max2 ? max1 : max2; })
+#define __safe_cmp(a,b) \
+ (__typecheck(a,b) && __no_side_effects(a,b))
-/**
- * max - return maximum of two values of the same or compatible types
- * @x: first value
- * @y: second value
- */
-#define max(x, y) \
- __max(typeof(x), typeof(y), \
- __UNIQUE_ID(max1_), __UNIQUE_ID(max2_), \
- x, y)
+#define __cmp(a,b,op) ((a)op(b)?(a):(b))
+
+#define __cmp_once(a,b,op) ({ \
+ typeof(a) __a = (a); \
+ typeof(b) __b = (b); \
+ __cmp(__a,__b,op); })
+
+#define __careful_cmp(a,b,op) \
+ __builtin_choose_expr(__safe_cmp(a,b), __cmp(a,b,op), __cmp_once(a,b,op))
+
+#define min(a,b) __careful_cmp(a,b,<)
+#define max(a,b) __careful_cmp(a,b,>)
+#define min_t(t,a,b) __careful_cmp((t)(a),(t)(b),<)
+#define max_t(t,a,b) __careful_cmp((t)(a),(t)(b),>)
/**
* min3 - return minimum of three values
@@ -856,35 +848,6 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
*/
#define clamp(val, lo, hi) min((typeof(val))max(val, lo), hi)
-/*
- * ..and if you can't take the strict
- * types, you can specify one yourself.
- *
- * Or not use min/max/clamp at all, of course.
- */
-
-/**
- * min_t - return minimum of two values, using the specified type
- * @type: data type to use
- * @x: first value
- * @y: second value
- */
-#define min_t(type, x, y) \
- __min(type, type, \
- __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \
- x, y)
-
-/**
- * max_t - return maximum of two values, using the specified type
- * @type: data type to use
- * @x: first value
- * @y: second value
- */
-#define max_t(type, x, y) \
- __max(type, type, \
- __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \
- x, y)
-
/**
* clamp_t - return a value clamped to a given range using a given type
* @type: the type of variable to use
^ permalink raw reply related
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Miguel Ojeda @ 2018-03-16 20:03 UTC (permalink / raw)
To: Linus Torvalds
Cc: Al Viro, Florian Weimer, Kees Cook, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Ingo Molnar, David Laight,
Ian Abbott, linux-input, linux-btrfs, Network Development,
Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CA+55aFzsrVF7W+YQye=EeF4Wk3yDOTbb_vSiM8s1zkKbE4JV4Q@mail.gmail.com>
On Fri, Mar 16, 2018 at 8:27 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Mar 16, 2018 at 10:55 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>
>> That's not them, that's C standard regarding ICE.
>
> Yes. The C standard talks about "integer constant expression". I know.
> It's come up in this very thread before.
>
> The C standard at no point talks about - or forbids - "variable length
> arrays". That never comes up in the whole standard, I checked.
>
> So we are right now hindered by a _syntactic_ check, without any way
> to have a _semantic_ check.
>
> That's my problem. The warnings are misleading and imply semantics.
>
> And apparently there is no way to actually check semantics.
>
>> 1,100 is *not* a constant expression as far as the standard is concerned,
>
> I very much know.
>
> But it sure isn't "variable" either as far as the standard is
> concerned, because the standard doesn't even have that concept (it
> uses "variable" for argument numbers and for variables).
>
> So being pedantic doesn't really change anything.
>
>> Would you argue that in
>> void foo(char c)
>> {
>> int a[(c<<1) + 10 - c + 2 - c];
>
> Yeah, I don't think that even counts as a constant value, even if it
> can be optimized to one. I would not at all be unhppy to see
> __builtin_constant_p() to return zero.
>
> But that is very much different from the syntax issue.
>
> So I would like to get some way to get both type-checking and constant
> checking without the annoying syntax issue.
>
>> expr, constant_expression is not a constant_expression. And in
>> this particular case the standard is not insane - the only reason
>> for using that is typechecking and _that_ can be achieved without
>> violating 6.6p6:
>> sizeof(expr,0) * 0 + ICE
>> *is* an integer constant expression, and it gives you exact same
>> typechecking. So if somebody wants to play odd games, they can
>> do that just fine, without complicating the logics for compilers...
>
> Now that actually looks like a good trick. Maybe we can use that
> instead of the comma expression that causes problems.
>
> And using sizeof() to make sure that __builtin_choose_expression()
> really gets an integer constant expression and that there should be no
> ambiguity looks good.
>
> Hmm.
>
> This works for me, and I'm being *very* careful (those casts to
> pointer types are inside that sizeof, because it's not an integral
> type, and non-integral casts are not valid in an ICE either) but
> somebody needs to check gcc-4.4:
>
> #define __typecheck(a,b) \
> (!!(sizeof((typeof(a)*)1==(typeof(b)*)1)))
>
> #define __no_side_effects(a,b) \
> (__builtin_constant_p(a)&&__builtin_constant_p(b))
>
> #define __safe_cmp(a,b) \
> (__typecheck(a,b) && __no_side_effects(a,b))
>
> #define __cmp(a,b,op) ((a)op(b)?(a):(b))
>
> #define __cmp_once(a,b,op) ({ \
> typeof(a) __a = (a); \
> typeof(b) __b = (b); \
> __cmp(__a,__b,op); })
>
> #define __careful_cmp(a,b,op) \
> __builtin_choose_expr(__safe_cmp(a,b), __cmp(a,b,op),
> __cmp_once(a,b,op))
>
> #define min(a,b) __careful_cmp(a,b,<)
> #define max(a,b) __careful_cmp(a,b,>)
> #define min_t(t,a,b) __careful_cmp((t)(a),(t)(b),<)
> #define max_t(t,a,b) __careful_cmp((t)(a),(t)(b),>)
>
> and yes, it does cause new warnings for that
>
> comparison between ‘enum tis_defaults’ and ‘enum tpm2_const’
>
> in drivers/char/tpm/tpm_tis_core.h due to
>
> #define TIS_TIMEOUT_A_MAX max(TIS_SHORT_TIMEOUT, TPM2_TIMEOUT_A)
>
> but technically that warning is actually correct, I'm just confused
> why gcc cares about the cast placement or something.
>
> That warning is easy to fix by turning it into a "max_t(int, enum1,
> enum2)' and that is technically the right thing to do, it's just not
> warned about for some odd reason with the current code.
>
> Kees - is there some online "gcc-4.4 checker" somewhere? This does
> seem to work with my gcc. I actually tested some of those files you
> pointed at now.
I use this one:
https://godbolt.org/
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Al Viro @ 2018-03-16 20:12 UTC (permalink / raw)
To: Linus Torvalds
Cc: Florian Weimer, Kees Cook, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Miguel Ojeda, Ingo Molnar,
David Laight, Ian Abbott, linux-input, linux-btrfs,
Network Development, Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CA+55aFzsrVF7W+YQye=EeF4Wk3yDOTbb_vSiM8s1zkKbE4JV4Q@mail.gmail.com>
On Fri, Mar 16, 2018 at 12:27:23PM -0700, Linus Torvalds wrote:
> But it sure isn't "variable" either as far as the standard is
> concerned, because the standard doesn't even have that concept (it
> uses "variable" for argument numbers and for variables).
Huh? 6.7.5.2p4:
If the size is not present, the array type is an incomplete type.
If the size is * instead of being an expression, the array type is
a variable length array type of unspecified size, which can only be
used in declarations with function prototype scope [footnote]; such
arrays are nonetheless complete types. If the size is an integer
constant expression and the element type has a known constant size,
the array type is not a variable length array type; otherwise, the
array type is a variable length array type.
footnote: Thus, * can be used only in function declarations that are
not definitions (see 6.7.5.3).
That's C99, straight from N1256.pdf (C99-TC3)...
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Linus Torvalds @ 2018-03-16 20:14 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Al Viro, Florian Weimer, Kees Cook, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Ingo Molnar, David Laight,
Ian Abbott, linux-input, linux-btrfs, Network Development,
Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CANiq72mjDQWsPG60JqfuLSY-6SDzB4goP4n35XZxc8bfFx1NDg@mail.gmail.com>
On Fri, Mar 16, 2018 at 1:03 PM, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> Kees - is there some online "gcc-4.4 checker" somewhere? This does
>> seem to work with my gcc. I actually tested some of those files you
>> pointed at now.
>
> I use this one:
>
> https://godbolt.org/
Well, my *test* code works on that one and -Wvla -Werror.
It does not work with gcc-4.1.x, but works with gcc-4.4.x.
I can't seem to see the errors any way, I wonder if
__builtin_choose_expr() simply didn't exist back then.
Odd that you can't view warnings/errors with it.
But it's possible that it fails on more complex stuff in the kernel.
I've done a "allmodconfig" build with that patch, and the only issue
it found was that (real) type issue in tpm_tis_core.h.
Linus
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Linus Torvalds @ 2018-03-16 20:15 UTC (permalink / raw)
To: Al Viro
Cc: Florian Weimer, Kees Cook, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Miguel Ojeda, Ingo Molnar,
David Laight, Ian Abbott, linux-input, linux-btrfs,
Network Development, Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <20180316201215.GG30522@ZenIV.linux.org.uk>
On Fri, Mar 16, 2018 at 1:12 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> That's C99, straight from N1256.pdf (C99-TC3)...
I checked C90, since the error is
ISO C90 forbids variable length array
and I didn't see anything there.
Admittedly I only found a draft copy.
Linus
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Al Viro @ 2018-03-16 20:18 UTC (permalink / raw)
To: Linus Torvalds
Cc: Florian Weimer, Kees Cook, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Miguel Ojeda, Ingo Molnar,
David Laight, Ian Abbott, linux-input, linux-btrfs,
Network Development, Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CA+55aFwxWEWya44pVAxSLM8oH8NyrOVf8QfLowSnX4WUXvC0xg@mail.gmail.com>
On Fri, Mar 16, 2018 at 01:15:27PM -0700, Linus Torvalds wrote:
> On Fri, Mar 16, 2018 at 1:12 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > That's C99, straight from N1256.pdf (C99-TC3)...
>
> I checked C90, since the error is
>
> ISO C90 forbids variable length array
>
> and I didn't see anything there.
Well, yes - VLA are new in C99; C90 never had those...
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Linus Torvalds @ 2018-03-16 20:19 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Al Viro, Florian Weimer, Kees Cook, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Ingo Molnar, David Laight,
Ian Abbott, linux-input, linux-btrfs, Network Development,
Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CA+55aFxLpdfyfTqhkHQoedQuqTcLRw3bYOgyz1s0eRW6cBmFTA@mail.gmail.com>
On Fri, Mar 16, 2018 at 1:14 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> It does not work with gcc-4.1.x, but works with gcc-4.4.x.
>
> I can't seem to see the errors any way, I wonder if
> __builtin_choose_expr() simply didn't exist back then.
No, that goes further back.
It seems to be -Wvla itself that doesn't exist in 4.1, so the test
build failed simply because I used that flag ;)
Linus
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox