Hi Dmitry,
On 7/21/19 7:48 PM, Dmitry Torokhov wrote:
> 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.
>
Very good point. Thanks.
>> +
>> + 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.
>
You are right. It should be removed.
>> + (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.
>
Thanks a lot for your help!
