[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Feedback for CSPRD03
Greetings, I've finally managed to "convince" one of my colleagues to provide some feedback. Thanks, Brian! (I'm just acting as an IPR proxy here...) Here it goes: * 2.3.1 "nor reads from multiple fields" Should be "nor *are* reads...", possibly "from multiple adjacent fields"? * 2.3.4 "This effects the block capacity and network mac fields" Should be "This *affects* the block capacity and network MAC fields" * 2.4 "When the device has finished a buffer" Would this be clearer as "When the device has finished *using/accessing/processing/something* a buffer"? * 18.104.22.168.1 "The driver MUST NOT set the VRING_DESC_F_INDIRECT flag unless the VIRTIO_RING_F_INDIRECT_DESC feature was negotiated." What happens if we set the flag? Should we explicitly say the device ignores it? Is there any way of signalling an error? * 22.214.171.124.1 "The driver MUST NOT set the VIRTQ_DESC_F_INDIRECT flag within an indirect descriptor (ie. only one table per descriptor)." OK, good, but again what does the failure mode look like? Should the device ignore it? * 3.1.1 "If any of these steps go irrecoverably wrong, the driver SHOULD set the FAILED status bit to indicate that it has given up on the device (it can reset the device later to restart if desired). The driver MUST not continue initialization in that case." In terms of impl, does this mean if FAILED is set, the device must respond to nothing but reset? * 3.1.1 "The driver MUST not accept" Should be "The driver MUST NOT accept" * 3.1.2 "Legacy devices do not support the FEATURES_OK status bit, and thus did not have a graceful way for the device to indicate unsupported feature combinations. It also did not provide a clear mechanism to end feature negotiation, which meant that devices finalized features on first-use, and no features could be introduced which radically changed the initial operation of the device." This has mixed tenses ("do not"/"did not"/"which meant"). Probably should all be present tense, since all the statements are still true of legacy devices now as well as in the past. * 4.2 "Therefore most of operations like..." Should be "Therefore most operations including" * 4.2.1 "MMIO provides no generic device discovery" Should be "MMIO provides no generic device discovery mechanism" * 4.2.2 "MMIO virtio devices provides" Should be "MMIO virtio devices provide" or "An MMIO virtio device provides" * 4.2.2 DeviceFeatures, DriverFeatures: "First bit" Should probably be "the least signficiant bit". * 4.2.2 QueueNum "therefore size of the Descriptor Table and both Available and Used rings" This is a bit confusing and a bit ambiguous. I think it would be clearer as "i.e. the size of the descriptor table, the number of entries in the availabe ring, and the number of entries in the used ring." (otherwise it's not clear if you mean size(DT) == size(available)+size(used), or * 4.2.2 QueueReady: "Ready to be used" Slightly confusing here: could mean 'CPU has updated the available ring/DT', when it really means 'the DT/available ring have first been initialised to sensible values'. Might be better to copy the PCI description which says 'the device may execute requests from this virtual queue'. * 4.2.2 ConfigGeneration: I was a bit lost as to what counts as a configuration change. I think the ref to section 2.3 doesn't help. Sec 126.96.36.199 is a bit better: "The device MUST change ConfigGeneration if there is any risk of a device seeing an inconsistent configuration state, but it MAY only change the value after a configuration read operation.", but I think that the PCI description (and the note following) is a bit clearer: "The device MUST present a changed config_generation after the driver has read a device-specific configuration value which has changed since any part of the device-specific configuration was last read. (Note, counter can wrap so do this...) * 4.2.2 InterruptAck: "Writing to this register notifies the device that the interrupt has been handled" Does this mean if InterruptStatus=3 and I write 1, I'm acking the ring update but not the configuration change? What happens if I try to ack something that hasn't happened, eg write 3 when InterruptStatus==0? * 4.2.2 Along with saying the registers are LE, should we say that all register accesses must be aligned, 32-bit accesses, and anything else is undefined behaviour? * 188.8.131.52 "When QueueReady is not zero, the drive..." Period missing at end of sentence. * 184.108.40.206.7.2 "a offload" Should be "an offload"