Hi Michael, thanks for reviewing:
On jue, ago 11 2022 at 09:32:05, "Michael S. Tsirkin" <mst@xxxxxxxxxx> wrote:
> which tree is this for?
>
> Applying: docs: driver-api: virtio: virtio on Linux
> error: sha1 information is lacking or useless (MAINTAINERS).
> error: could not build fake ancestor
> Patch failed at 0001 docs: driver-api: virtio: virtio on Linux
linux-next, as stated in the cover letter:
Tested on linux-next (next-20220802)
I just verified that the patch also applies to the vhost tree
(linux-next branch). Where did you test it?
>> + buf = virtqueue_get_buf(dev->vq, &len);
>> + /* spurious callback? */
>> + if (!buf)
>> + return;
>
> most drivers need to do this in a loop, this code is only valid if
> there's just 1 buf in flight - unusual.
That's a driver-specific consideration and this is supposed to be the
simplest possible driver skeleton, but ok.
>> + static int virtio_dummy_probe(struct virtio_device *vdev)
>> + {
>> + struct virtio_dummy_dev *dev = NULL;
>> +
>> + /* initialize device data */
>> + dev = kzalloc(sizeof(struct virtio_dummy_dev), GFP_KERNEL);
>
>
> I dislike how we set dev to NULL and immediately to a different value
> just below.
This is a matter of style, I think. There are plenty of examples of this
in the kernel code, but I don't mind changing it.
> what is missing here is registration with Linux core.
Isn't that supposed to be done by module_virtio_driver()?
> depending on device you might need a call to device_ready, too.
I already explained below that this is done by default after probe()
(see virtio_dev_probe()). virtio_device_ready() is supposed to be useful
only if you need to use the vqs in the probe function.
>> + /*
>> + * Disable vq interrupts: equivalent to
>> + * vdev->config->reset(vdev)
>> + */
>> + virtio_reset_device(vdev);
>> +
>
> you highly likely need to detach unused buffers from vqs here.
Ack.
> let's be a bit clearer here that they must be enabled before add_buf
> triggers.
Ok.
> maybe clarify that they can still trigger even if enabled.
> if you want to disable reliable you have to reset the device
> or the vq.
I'll check that out, thanks.
Cheers,
Ricardo
