Hi,> > 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. > > Can do that, sure.Well, the function where this is in already cares about the bitmap copy only. Can add a comment though.> > You are doing leXXX everywhere, that's VERSION_1 dependency. > > virtio_cread will do byteswaps differently without VERSION_1. > > Just don't go there. > > Changed that for v2, for the config space structs. They have normal u32 > in there now. virtio_cread() wants it this way.I liked the __le32 in the config space structs more though, so I've waded through the virtio_config.h header file. To me it looks like we need separate virtio_cread() versions for non-transitional drivers, which do __le32 -> u32 translation instead of __virtio32 -> u32 translation, so I can have __le32 types in the config space structs. Or I could use vdev->config->get() directly instead of virtio_cread, but I'll loose sparse checking that way. Hmm. Recommendations? Better ideas? cheers, Gerd
On Mon, Mar 23, 2015 at 02:44:52PM +0100, Gerd Hoffmann wrote:> Hi, > > > > 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. > > > > Can do that, sure. > > Well, the function where this is in already cares about the bitmap copy > only. Can add a comment though.OK, I think that will be enough for now.> > > You are doing leXXX everywhere, that's VERSION_1 dependency. > > > virtio_cread will do byteswaps differently without VERSION_1. > > > Just don't go there. > > > > Changed that for v2, for the config space structs. They have normal u32 > > in there now. virtio_cread() wants it this way. > > I liked the __le32 in the config space structs more though, so I've > waded through the virtio_config.h header file. > > To me it looks like we need separate virtio_cread() versions for > non-transitional drivers, which do __le32 -> u32 translation instead of > __virtio32 -> u32 translation, so I can have __le32 types in the config > space structs. > > Or I could use vdev->config->get() directly instead of virtio_cread, but > I'll loose sparse checking that way. > > Hmm. Recommendations? Better ideas? > > cheers, > GerdSo to clarify, you dislike using __virtio32 in virtio input header? -- MST
Hi,> > > > You are doing leXXX everywhere, that's VERSION_1 dependency. > > > > virtio_cread will do byteswaps differently without VERSION_1. > > > > Just don't go there.> So to clarify, you dislike using __virtio32 in virtio input header?Well, as I understand things __virtio32 implies byteorder depends on whenever we are using VERSION_1 or not. And non-transitional drivers should not need it as everything is by definition little endian. So, yes, your suggestion to just require VERSION_1 in the driver implies in my eyes that there should be no reason to use __virtio32 instead of __le32. Or do I miss something here? cheers, Gerd