search

rust-vmm / vhost-device

'vhost-user' device backends workspace
Apache License 2.0
67 stars 46 forks source link

Vhost device scmi notification implement #710

Open junnan01-wu opened 4 weeks ago

junnan01-wu commented 4 weeks ago

Summary of the PR

This is Samsung Team. We want to contribute scmi notification implement. In virtio-spec, if feature VIRTIO_SCMI_F_P2A_CHANNELS was negotiated, device should implement SCMI notification.

In our embeded environment, we enable iio sensor with notification feature, and configure this feature.

junnan01-wu commented 3 weeks ago

Update file "coverage_config_x86_64.json" to pass vhost-device-ci

mz-pdm commented 3 weeks ago

Update file "coverage_config_x86_64.json" to pass vhost-device-ci

Oh, reducing coverage_score is not very good. New code should be covered by unit tests if reasonably possible.

junnan01-wu commented 3 weeks ago

It's actually not a good idea, I will add new unit tests for new code in next push for order not reducing coverage rate

junnan01-wu commented 3 weeks ago

In "force-pushed the vhost-device-scmi branch from fd4d28e to bd6f00e", I did following works

  1. Modify "SENSOR_READING_GET" flow in handle. After supporting notification, once frontend wants to read from sysfs "_XXXraw", it should call "_notify_statusset" to disable notify before "reading_get", otherwise, it will return "device busy".

  2. Add struct "ChanScanType" in struct "Axis" In notification flow, data from /dev/iio:deviceX is formatted according to the rule from "_scan_elements/XXXtype". Therefore, each axis should store the parsing result from it. And I add a "new" implmentment for Axis, if scan_type exists, it will parse it and store in self.

  3. As we communicated previously, I modified the return type of function "_readingget". According to SCMI spec, Sensor Reading Descriptor is (i32,i32,u32,u32). Correspondingly, unit test of "_readingget" also modified.

  4. Refactor function "_readaxis". Previously, function "_readaxis" has two mainly processes. The first is reading data from "_XXXraw" The second is reading offset and scale, then multiplying raw with them The second flow can be reused by "_readingupdate", so I sperate it to a new function "_deal_axis_rawdata", so that "_readingupdate" can call it.

  5. In "_readingupdate", initialize buffer size according to scan_type.

  6. I squash last two commits, they are both for notification flow.

  7. Add some unit tests.

mz-pdm commented 2 weeks ago

Thank you for the update, it looks nice at the first glance. I'll try to review the whole PR again in the coming days. And sorry for a delayed response, I was away.

junnan01-wu commented 1 week ago

Hello mz-pdm, please leave review comment here in order to discuss conveniently.

I do some modifications according to your review comment on my fork repo.

More action items need to be done in next push

  1. Change Senser:new() and change name to Option, related unit test need to be changed.
  2. Add unit test for order remove the modification for "coverage"
junnan01-wu commented 1 week ago

In push branch from f6de7cf to c80b59f

I change Sensor::name to Option. Related test function also changed.

junnan01-wu commented 1 week ago

In push branch from from c80b59f to 02f5204

Some units test for notification flow have added.

Then, coverity score will not decrease.

mz-pdm commented 1 week ago

Thank you for all your changes. I'll be unresponsive the next 1-2 weeks but don't worry, I won't forget about your PR. Anyway, a good progress has been made and I think it's time to involve other reviewers or the maintainers now.

junnan01-wu commented 1 week ago

Thanks for your kindly review. It really help us a lot.

junnan01-wu commented 4 days ago

In latest push, I separate some patches.

stefano-garzarella commented 4 days ago

okay, LGTM, but I'm not a scmi expert at all, so I'd like an approval from @mz-pdm. I see a conversation still open, so I'd like to wait it resolved before merging this.

junnan01-wu commented 3 days ago

okay, LGTM, but I'm not a scmi expert at all, so I'd like an approval from @mz-pdm. I see a conversation still open, so I'd like to wait it resolved before merging this.

Well, that's fine