Hi Markus,
On Sun, Jul 21, 2019 at 02:37:59PM +0200, Markus Koch wrote:
> +static int fsia6b_serio_connect(struct serio *serio, struct serio_driver *drv)
> +{
> + struct fsia6b *fsia6b;
> + struct input_dev *input_dev;
> + int err;
> + int i, j;
> + int sw_id = 0;
> +
> + fsia6b = kzalloc(sizeof(*fsia6b), GFP_KERNEL);
> + if (!fsia6b)
> + return -ENOMEM;
> +
> + fsia6b->packet.ibuf = 0;
> + fsia6b->packet.offset = 0;
> + fsia6b->packet.state = SYNC;
> +
> + serio_set_drvdata(serio, fsia6b);
> +
> + err = serio_open(serio, drv);
> + if (err)
> + goto fail1;
> +
I just noticed this: we should allocate the input device before opening
the serio port, otherwise interrupt may come in earlier and you will end
up with a null pointer dereference.
No need to resubmit, I can adjust on my side.
> +
> + input_dev = input_allocate_device();
> + if (!input_dev) {
> + err = -ENOMEM;
> + goto fail2;
> + }
> + fsia6b->dev = input_dev;
> +
> + snprintf(fsia6b->phys, sizeof(fsia6b->phys), "%s/input0", serio->phys);
> +
> + input_dev->name = DRIVER_DESC;
> + input_dev->phys = fsia6b->phys;
> + input_dev->id.bustype = BUS_RS232;
> + input_dev->id.vendor = SERIO_FSIA6B;
> + input_dev->id.product = serio->id.id;
> + input_dev->id.version = 0x0100;
> + input_dev->dev.parent = &serio->dev;
> +
> + for (i = 0; i < IBUS_SERVO_COUNT; ++i) {
> + input_set_abs_params(input_dev, fsia6b_axes[i],
> + 1000, 2000, 2, 2);
> + }
> +
> + // Register switch configuration
> + for (i = 0; i < IBUS_SERVO_COUNT; ++i) {
> + if (((switch_config[i] == '\0') && (i != IBUS_SERVO_COUNT)) ||
I believe this condition is not needed, as you will never get here with
i >= IBUS_SERVO_COUNT, and premature end of string will trip the
check below as well.
I will remove it.
> + (switch_config[i] < '0') ||
> + (switch_config[i] > '3')) {
> + dev_err(&fsia6b->dev->dev,
> + "Invalid switch configuration supplied for fsia6b.\n");
> + err = -EINVAL;
> + goto fail3;
> + }
> +
Thanks.
--
Dmitry
