Re: [PATCH V7 v7] virtio: introduce SUSPEND bit in device status

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Sep 27, 2024 at 12:08:54PM +0800, Jason Wang wrote:
> On Wed, Sep 25, 2024 at 7:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Sep 25, 2024 at 11:47:49AM +0800, Jason Wang wrote:
> > > On Wed, Sep 25, 2024 at 7:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Tue, Sep 24, 2024 at 03:35:57PM +0800, Jason Wang wrote:
> > > > > >
> > > > > > > And I still don't see why
> > > > > > > introducing a single new bit in the status brings any new troubles
> > > > > > > compared with the existing reset and other state transitions.
> > > > > >
> > > > > > reset is not a state.
> > > > >
> > > > > Well, I'm not sure I get this. Each value read from status should
> > > > > represent a device state.
> > > >
> > > > What makes you say so?
> > > > status represents driver state, not device state.
> > > > In fact, driver writes there and does not double check with the
> > > > sole exception of reset.
> > >
> > >
> > > For example, spec said
> > >
> > > """
> > >
> > > 2.1 Device Status Field
> >
> > yes, this is confusingly written
> 
> Anything makes you think it is confusing?


It says Device Status but most of it reflects the driver probe progress.
Compare to pci RO status and RW control.

> >
> > > During device initialization by a driver, the driver follows the
> > > sequence of steps specified in 3.1.
> > >
> > > The device status field provides a simple low-level indication of the
> > > completed steps of this sequence.
> >
> > And note the sequence is driver sequence.
> 
> Right but it means the driver needs to know the status of the device
> in order to proceed.

again this is not device status at all.



> >
> > > It’s most useful to imagine it
> > > hooked up to traffic lights on the console indicating the status of
> > > each device. The following bits are defined (listed below in the order
> > > in which they would be typically set):
> > >
> > > """
> >
> >
> >
> >
> > > Driver knows its own state, so there's no need for the driver to read
> > > it from the device.
> >
> > That is why it never reads status.
> 
> Well, looking at the current Linux drivers there're plenty (in virtio
> core, there could be other in the transport)
> 
> drivers/virtio/virtio.c:        return sysfs_emit(buf, "0x%08x\n",
> dev->config->get_status(dev));
> drivers/virtio/virtio.c:        dev->config->set_status(dev,
> dev->config->get_status(dev) | status);
> drivers/virtio/virtio.c:        status = dev->config->get_status(dev);
> drivers/virtio/virtio.c:        if (!(dev->config->get_status(dev) &
> VIRTIO_CONFIG_S_DRIVER_OK))
> drivers/virtio/virtio.c:        WARN_ON_ONCE(dev->config->get_status(dev));
> drivers/virtio/virtio.c:        dev->failed =
> dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
> drivers/virtio/virtio.c:        if (!(dev->config->get_status(dev) &
> VIRTIO_CONFIG_S_DRIVER_OK))

It's done like this for robustness and simplicity.
But we could easily cache it in the driver on write.


> >
> >
> > > >
> > > > > And 0 is a valid status which means device
> > > > > is resetted:
> > > > >
> > > > > """
> > > > > The driver SHOULD consider a driver-initiated reset complete when it
> > > > > reads device status as 0.
> > > > > """
> > > >
> > > > It means all registers have their reset values,
> > >
> > > This is exactly the device status? Drivers need to know that there's a
> > > device state that is safe to start to program.
> >
> > yes but otherwise, all registers respond normally
> > device is assumed to be fully operational, and
> > it is assumed that no time at all is needed to
> > get from that to DRIVER.
> >
> >
> > > > but it is
> > > > not a special state in that device behaves normally in this
> > > > state, it does not need to be handles in any special way.
> > > >
> > > > Contrast with suspend where device does not respond and
> > > > must be first resumed.
> > >
> > > I think not. Driver can choose to reset the device when the device is suspended.
> > >
> > > Thanks
> >
> > Yes, reset should take device out of suspend.
> 
> Thanks
> 
> >
> > > >
> > >
> > >
> > > >
> > > > --
> > > > MST
> > > >
> >