Uh oh!
There was an error while loading. Please reload this page.
virtio: provide a rust interface of virtio_driver in rust - #841
virtio: provide a rust interface of virtio_driver in rust#841Richardhongyu wants to merge 1 commit into
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Richardhongyu
commented
Jul 29, 2022
The new commit fixes the above errors in the comment. |
Uh oh!
There was an error while loading. Please reload this page.
f936248 to
1fe931dComparefbq
commented
Aug 2, 2022
This looks great, but one question: shouldn't we use a dedicated crate for virtio support? First VIRTIO is not always enabled, we at least need to put some "cfg gate"s somewhere, second the virtio support is not a basic part of core kernel, we should separate it for maintainship. |
wedsonaf
left a comment
There was a problem hiding this comment.
Other than the of ids, this looks good! Thanks for contributing.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| vdrv.remove = Some(Self::remove_callback); | ||
| if let Some(t) = T::ID_TABLE { | ||
| vdrv.id_table = t.as_ref(); | ||
| } |
There was a problem hiding this comment.
If you want of ids to work here, you must set vdrv.driver.of_match_table -- see platform.rs for an example.
There was a problem hiding this comment.
I found that in the virtio drivers of C sides, the common way to match the driver with the device is to use 'id_table' rather than of. I write a macro define_virtio_id_table to implement this. It's more like define_amba_id_table in the amba.rs.
| // duration of this call, so it is guaranteed to remain alive for the lifetime of | ||
| // `vdev`. | ||
| let mut dev = unsafe { Device::from_ptr(vdev) }; | ||
| let data = T::probe(&mut dev)?; |
There was a problem hiding this comment.
of identifiers are allowed to carry some data with them. If the match happened through one of them, ideally you'll pass this data to the probe function. See get_id_info in platform.rs for an example of this. (Perhaps we should move this function to the driver module so all drivers that support of can use it.)
There was a problem hiding this comment.
The virtio drivers use id_table to match the driver with the device, which does not contain any data field. The probe function in the virtio Driver is more like the probe function in the amba Driver.
It's reasonable to move get_id_info into the driver module. I can open a new PR about it.
wedsonaf
commented
Aug 2, 2022
I don't think we support putting it a different crate just yet, although @ojeda has been thinking about this. As for a cfg gate, I agree we should have it. We normally put it in |
Richardhongyu
commented
Aug 6, 2022
Thanks for reviewing! I use '#[cfg(CONFIG_VIRTIO)]' in the new commit. |
Richardhongyu
commented
Aug 6, 2022
Thanks for reviewing! It seems I confuse the |
Richardhongyu
commented
Aug 17, 2022
@wedsonaf Hi, could you help have a look at this whenever you have time? Thanks in advance! |
Wrap the struct virtio_driver in rust. Signed-off-by: Li Hongyu <lihongyu1999@bupt.edu.cn>
054867b to
0980c9fCompareRichardhongyu
commented
Aug 17, 2022
Now the code in the comments does not use |
Wrap the struct virtio_driver in rust.
Signed-off-by: Li Hongyu lihongyu1999@bupt.edu.cn