On Fri, Mar 20, 2015 at 11:28:47AM +0100, Gerd Hoffmann wrote:
> Hi,
>
> > > +static int virtinput_send_status(struct virtio_input *vi,
> > > + u16 type, u16 code, s32 value)
> > > +{
> > > + struct virtio_input_event *stsbuf;
> > > + struct scatterlist sg[1];
> > > +
> > > + stsbuf = kzalloc(sizeof(*stsbuf), GFP_ATOMIC);
> > > + if (!stsbuf)
> > > + return -ENOMEM;
> >
> > Does this return an error to userspace?
> > If so it's not a good idea I think, GFP_ATOMIC failures are
> > transient conditions and should not be reported
> > to userspace.
> > Can use GFP_KERNEL here?
>
> Waiting for an answer from the ioput guys here.
>
> > > +
> > > + stsbuf->type = cpu_to_le16(type);
> > > + stsbuf->code = cpu_to_le16(code);
> > > + stsbuf->value = cpu_to_le32(value);
> > > + sg_init_one(sg, stsbuf, sizeof(*stsbuf));
> > > + virtqueue_add_outbuf(vi->sts, sg, 1, stsbuf, GFP_ATOMIC);
> >
> > This can fail if queue is full. What prevents this from happening?
>
> Nothing. It's highly unlikely though given the throughput we have for
> input devices, not sure it is that useful to put too much effort into
> this. Except for freeing stsbuf in the error case.
>
> > > + virtqueue_kick(vi->sts);
> >
> > Also what prevents multiple virtinput_send_status calls
> > from racing with each other? Is there locking at a higher level?
>
> I think input_dev->event_lock does this.
>
> > > +static void virtinput_recv_status(struct virtqueue *vq)
> > > +{
> > > + struct virtio_input *vi = vq->vdev->priv;
> > > + struct virtio_input_event *stsbuf;
> > > + unsigned int len;
> > > +
> > > + while ((stsbuf = virtqueue_get_buf(vi->sts, &len)) != NULL)
> >
> > looks like this get_buf can race with add above.
>
> Yes, it can.
>
> > Need some locking.
>
> I'll add it.
>
> > Also pls avoid != NULL, removing it makes code less verbose.
> >
> > > + kfree(stsbuf);
> > > + virtqueue_kick(vq);
> >
> > Why are you kicking here?
>
> Hmm, it is pointless indeed.
>
> > > + if (select == VIRTIO_INPUT_CFG_EV_BITS)
> > > + set_bit(subsel, vi->idev->evbit);
> > > + for (bit = 0; bit < bitcount; bit++) {
> > > + if ((bit % 8) == 0)
> > > + virtio_cread(vi->vdev, struct virtio_input_config,
> > > + u.bitmap[bit/8], &cfg);
> >
> > coding style violations above. you need spaces around ops like / and *.
> > Please run checkpatch.pl
> >
> > > + if (cfg & (1 << (bit % 8)))
> > > + set_bit(bit, bits);
> >
> > what if not set? does something clear the mask?
>
> kzalloc?
So you are really just reading in array of bytes?
All this set bit trickery is just to convert things from LE?
> > > + }
> >
> > doesn't above just implement bitmap_copy or bitmap_or?
>
> Not fully sure how bitmaps are defined. virtio has a stream of bytes,
> first byte carries bits 0-7, second 8-15 etc. linux kernel bitmaps ops
> are operating on longs, and native byteorder longs would be something
> else ...
This still looks too complex.
At least, this needs a comment explaining what the function does,
and maybe wrap it in a helper like virtio_input_bitmap_copy or
virtio_bitmap_or.
> > > + size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ABS_INFO, abs);
> > > + virtio_cread(vi->vdev, struct virtio_input_config, u.abs.min, &mi);
> > > + virtio_cread(vi->vdev, struct virtio_input_config, u.abs.max, &ma);
> > > + virtio_cread(vi->vdev, struct virtio_input_config, u.abs.fuzz, &fu);
> > > + virtio_cread(vi->vdev, struct virtio_input_config, u.abs.flat, &fl);
> >
> > you read le field into u32 value.
> > Please run sparse on this code. you will get a ton
> > of warnings. Same error appears elsewhere.
>
> Indeed. IIRC that wasn't the case a while back. Guess those bitwise
> annotations have been added with the virtio 1.0 patches?
>
> In any case I'll fix it up.
I see you still didn't in v2?
> > > +static int virtinput_probe(struct virtio_device *vdev)
> > > +{
> > > + struct virtio_input *vi;
> > > + size_t size;
> > > + int abs, err;
> >
> > How about checking VERSION_1 and bailing out of not there?
>
> I don't think this is needed. There isn't a hard dependency on virtio
> 1.0. It's just that config space is relatively large and because of
> that I want it be 1.0 on the host (qemu) side to not allocate large
> portions of I/O address space for the legacy virtio pci bar.
You are doing leXXX everywhere, that's VERSION_1 dependency.
virtio_cread will do byteswaps differently without VERSION_1.
Just don't go there.
> > > + vi->idev->name = vi->name;
> > > + vi->idev->phys = vi->phys;
> > > + vi->idev->id.bustype = BUS_VIRTUAL;
> > > + vi->idev->id.vendor = 0x0001;
> > > + vi->idev->id.product = 0x0001;
> > > + vi->idev->id.version = 0x0100;
> >
> > Add comments explaining why these #s make sense?
>
> See other subthread, will be changed to be host-provided (like name).
>
> > > + err = input_register_device(vi->idev);
> >
> > Once you do this, virtinput_status can get called,
> > and that will kick, correct?
>
> Correct.
>
> > If so, you must call device_ready before this.
>
> Ok.
>
> > > + if (err)
> > > + goto out4;
> > > +
> > > + return 0;
> > > +
> > > +out4:
> > > + input_free_device(vi->idev);
> > > +out3:
> > > + vdev->config->del_vqs(vdev);
> > > +out2:
> > > + kfree(vi);
> > > +out1:
> > > + return err;
> >
> > free on error is out of order with initialization.
> > Might lead to leaks or other bugs.
> > Also - can you name labels something sensible pls?
> > out is usually for exiting on success too...
> > E.g. out4 -> err_register etc.
>
> Will fix.
>
> > > +static void virtinput_remove(struct virtio_device *vdev)
> > > +{
> > > + struct virtio_input *vi = vdev->priv;
> > > +
> > > + input_unregister_device(vi->idev);
> > > + vdev->config->reset(vdev);
> >
> > You don't really need a reset if you just to del_vqs.
> > People do this if they want to prevent interrupts
> > without deleting vqs.
>
> Ok.
>
> > > + vdev->config->del_vqs(vdev);
> > > + kfree(vi);
> >
> > free_device seems to be missing?
>
> Indeed, good catch.
>
> thanks,
> Gerd
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
