This patch tries to implement the synchronize_cbs() for ccw. For the
vring_interrupt() that is called via virtio_airq_handler(), the
synchronization is simply done via the airq_info's lock. For the
vring_interrupt() that is called via virtio_ccw_int_handler(), a per
device spinlock for irq is introduced ans used in the synchronization
method.
Cc: Thomas Gleixner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Halil Pasic <[email protected]>
Cc: Cornelia Huck <[email protected]>
Signed-off-by: Jason Wang <[email protected]>
---
drivers/s390/virtio/virtio_ccw.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index d35e7a3f7067..001e1f0e6037 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -62,6 +62,7 @@ struct virtio_ccw_device {
unsigned int revision; /* Transport revision */
wait_queue_head_t wait_q;
spinlock_t lock;
+ rwlock_t irq_lock;
struct mutex io_lock; /* Serializes I/O requests */
struct list_head virtqueues;
bool is_thinint;
@@ -984,6 +985,27 @@ static const char *virtio_ccw_bus_name(struct virtio_device *vdev)
return dev_name(&vcdev->cdev->dev);
}
+static void virtio_ccw_synchronize_cbs(struct virtio_device *vdev)
+{
+ struct virtio_ccw_device *vcdev = to_vc_device(vdev);
+ struct airq_info *info = vcdev->airq_info;
+
+ if (info) {
+ /*
+ * Synchronize with the vring_interrupt() with airq indicator
+ */
+ write_lock(&info->lock);
+ write_unlock(&info->lock);
+ } else {
+ /*
+ * Synchronize with the vring_interrupt() called by
+ * virtio_ccw_int_handler().
+ */
+ write_lock(&vcdev->irq_lock);
+ write_unlock(&vcdev->irq_lock);
+ }
+}
+
static const struct virtio_config_ops virtio_ccw_config_ops = {
.get_features = virtio_ccw_get_features,
.finalize_features = virtio_ccw_finalize_features,
@@ -995,6 +1017,7 @@ static const struct virtio_config_ops virtio_ccw_config_ops = {
.find_vqs = virtio_ccw_find_vqs,
.del_vqs = virtio_ccw_del_vqs,
.bus_name = virtio_ccw_bus_name,
+ .synchronize_cbs = virtio_ccw_synchronize_cbs,
};
@@ -1079,6 +1102,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
{
__u32 activity = intparm & VIRTIO_CCW_INTPARM_MASK;
struct virtio_ccw_device *vcdev = dev_get_drvdata(&cdev->dev);
+ unsigned long flags;
int i;
struct virtqueue *vq;
@@ -1106,6 +1130,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
vcdev->err = -EIO;
}
virtio_ccw_check_activity(vcdev, activity);
+ read_lock_irqsave(&vcdev->irq_lock, flags);
for_each_set_bit(i, indicators(vcdev),
sizeof(*indicators(vcdev)) * BITS_PER_BYTE) {
/* The bit clear must happen before the vring kick. */
@@ -1114,6 +1139,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
vq = virtio_ccw_vq_by_ind(vcdev, i);
vring_interrupt(0, vq);
}
+ read_unlock_irqrestore(&vcdev->irq_lock, flags);
if (test_bit(0, indicators2(vcdev))) {
virtio_config_changed(&vcdev->vdev);
clear_bit(0, indicators2(vcdev));
@@ -1284,6 +1310,7 @@ static int virtio_ccw_online(struct ccw_device *cdev)
init_waitqueue_head(&vcdev->wait_q);
INIT_LIST_HEAD(&vcdev->virtqueues);
spin_lock_init(&vcdev->lock);
+ rwlock_init(&vcdev->irq_lock);
mutex_init(&vcdev->io_lock);
spin_lock_irqsave(get_ccwdev_lock(cdev), flags);
--
2.25.1
On Sat, May 07, 2022 at 03:19:51PM +0800, Jason Wang wrote:
> This patch tries to implement the synchronize_cbs() for ccw. For the
> vring_interrupt() that is called via virtio_airq_handler(), the
> synchronization is simply done via the airq_info's lock. For the
> vring_interrupt() that is called via virtio_ccw_int_handler(), a per
> device spinlock for irq is introduced ans used in the synchronization
> method.
>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Halil Pasic <[email protected]>
> Cc: Cornelia Huck <[email protected]>
> Signed-off-by: Jason Wang <[email protected]>
> ---
> drivers/s390/virtio/virtio_ccw.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index d35e7a3f7067..001e1f0e6037 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -62,6 +62,7 @@ struct virtio_ccw_device {
> unsigned int revision; /* Transport revision */
> wait_queue_head_t wait_q;
> spinlock_t lock;
> + rwlock_t irq_lock;
> struct mutex io_lock; /* Serializes I/O requests */
> struct list_head virtqueues;
> bool is_thinint;
> @@ -984,6 +985,27 @@ static const char *virtio_ccw_bus_name(struct virtio_device *vdev)
> return dev_name(&vcdev->cdev->dev);
> }
>
> +static void virtio_ccw_synchronize_cbs(struct virtio_device *vdev)
> +{
> + struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> + struct airq_info *info = vcdev->airq_info;
> +
> + if (info) {
> + /*
> + * Synchronize with the vring_interrupt() with airq indicator
> + */
> + write_lock(&info->lock);
> + write_unlock(&info->lock);
> + } else {
> + /*
> + * Synchronize with the vring_interrupt() called by
> + * virtio_ccw_int_handler().
> + */
> + write_lock(&vcdev->irq_lock);
> + write_unlock(&vcdev->irq_lock);
> + }
> +}
> +
> static const struct virtio_config_ops virtio_ccw_config_ops = {
> .get_features = virtio_ccw_get_features,
> .finalize_features = virtio_ccw_finalize_features,
> @@ -995,6 +1017,7 @@ static const struct virtio_config_ops virtio_ccw_config_ops = {
> .find_vqs = virtio_ccw_find_vqs,
> .del_vqs = virtio_ccw_del_vqs,
> .bus_name = virtio_ccw_bus_name,
> + .synchronize_cbs = virtio_ccw_synchronize_cbs,
> };
>
>
> @@ -1079,6 +1102,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
> {
> __u32 activity = intparm & VIRTIO_CCW_INTPARM_MASK;
> struct virtio_ccw_device *vcdev = dev_get_drvdata(&cdev->dev);
> + unsigned long flags;
> int i;
> struct virtqueue *vq;
>
> @@ -1106,6 +1130,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
> vcdev->err = -EIO;
> }
> virtio_ccw_check_activity(vcdev, activity);
> + read_lock_irqsave(&vcdev->irq_lock, flags);
> for_each_set_bit(i, indicators(vcdev),
> sizeof(*indicators(vcdev)) * BITS_PER_BYTE) {
> /* The bit clear must happen before the vring kick. */
Cornelia sent a lockdep trace on this.
Basically I think this gets the irqsave/restore logic wrong.
It attempts to disable irqs in the handler (which is an interrupt
anyway).
And it does not disable irqs in the synchronize_cbs.
As a result in interrupt might try to take a read lock while
.synchronize_cbs has the writer lock, resulting in a deadlock.
I think you want regular read_lock + write_lock_irq.
> @@ -1114,6 +1139,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
> vq = virtio_ccw_vq_by_ind(vcdev, i);
> vring_interrupt(0, vq);
> }
> + read_unlock_irqrestore(&vcdev->irq_lock, flags);
> if (test_bit(0, indicators2(vcdev))) {
> virtio_config_changed(&vcdev->vdev);
> clear_bit(0, indicators2(vcdev));
> @@ -1284,6 +1310,7 @@ static int virtio_ccw_online(struct ccw_device *cdev)
> init_waitqueue_head(&vcdev->wait_q);
> INIT_LIST_HEAD(&vcdev->virtqueues);
> spin_lock_init(&vcdev->lock);
> + rwlock_init(&vcdev->irq_lock);
> mutex_init(&vcdev->io_lock);
>
> spin_lock_irqsave(get_ccwdev_lock(cdev), flags);
> --
> 2.25.1
On Tue, May 10, 2022 at 7:28 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Sat, May 07, 2022 at 03:19:51PM +0800, Jason Wang wrote:
> > This patch tries to implement the synchronize_cbs() for ccw. For the
> > vring_interrupt() that is called via virtio_airq_handler(), the
> > synchronization is simply done via the airq_info's lock. For the
> > vring_interrupt() that is called via virtio_ccw_int_handler(), a per
> > device spinlock for irq is introduced ans used in the synchronization
> > method.
> >
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: "Paul E. McKenney" <[email protected]>
> > Cc: Marc Zyngier <[email protected]>
> > Cc: Halil Pasic <[email protected]>
> > Cc: Cornelia Huck <[email protected]>
> > Signed-off-by: Jason Wang <[email protected]>
> > ---
> > drivers/s390/virtio/virtio_ccw.c | 27 +++++++++++++++++++++++++++
> > 1 file changed, 27 insertions(+)
> >
> > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> > index d35e7a3f7067..001e1f0e6037 100644
> > --- a/drivers/s390/virtio/virtio_ccw.c
> > +++ b/drivers/s390/virtio/virtio_ccw.c
> > @@ -62,6 +62,7 @@ struct virtio_ccw_device {
> > unsigned int revision; /* Transport revision */
> > wait_queue_head_t wait_q;
> > spinlock_t lock;
> > + rwlock_t irq_lock;
> > struct mutex io_lock; /* Serializes I/O requests */
> > struct list_head virtqueues;
> > bool is_thinint;
> > @@ -984,6 +985,27 @@ static const char *virtio_ccw_bus_name(struct virtio_device *vdev)
> > return dev_name(&vcdev->cdev->dev);
> > }
> >
> > +static void virtio_ccw_synchronize_cbs(struct virtio_device *vdev)
> > +{
> > + struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> > + struct airq_info *info = vcdev->airq_info;
> > +
> > + if (info) {
> > + /*
> > + * Synchronize with the vring_interrupt() with airq indicator
> > + */
> > + write_lock(&info->lock);
> > + write_unlock(&info->lock);
> > + } else {
> > + /*
> > + * Synchronize with the vring_interrupt() called by
> > + * virtio_ccw_int_handler().
> > + */
> > + write_lock(&vcdev->irq_lock);
> > + write_unlock(&vcdev->irq_lock);
> > + }
> > +}
> > +
> > static const struct virtio_config_ops virtio_ccw_config_ops = {
> > .get_features = virtio_ccw_get_features,
> > .finalize_features = virtio_ccw_finalize_features,
> > @@ -995,6 +1017,7 @@ static const struct virtio_config_ops virtio_ccw_config_ops = {
> > .find_vqs = virtio_ccw_find_vqs,
> > .del_vqs = virtio_ccw_del_vqs,
> > .bus_name = virtio_ccw_bus_name,
> > + .synchronize_cbs = virtio_ccw_synchronize_cbs,
> > };
> >
> >
> > @@ -1079,6 +1102,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
> > {
> > __u32 activity = intparm & VIRTIO_CCW_INTPARM_MASK;
> > struct virtio_ccw_device *vcdev = dev_get_drvdata(&cdev->dev);
> > + unsigned long flags;
> > int i;
> > struct virtqueue *vq;
> >
> > @@ -1106,6 +1130,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
> > vcdev->err = -EIO;
> > }
> > virtio_ccw_check_activity(vcdev, activity);
> > + read_lock_irqsave(&vcdev->irq_lock, flags);
> > for_each_set_bit(i, indicators(vcdev),
> > sizeof(*indicators(vcdev)) * BITS_PER_BYTE) {
> > /* The bit clear must happen before the vring kick. */
>
> Cornelia sent a lockdep trace on this.
>
> Basically I think this gets the irqsave/restore logic wrong.
> It attempts to disable irqs in the handler (which is an interrupt
> anyway).
The reason I use irqsave/restore is that it can be called from process
context (if I was not wrong), e.g from io_subchannel_quiesce().
> And it does not disable irqs in the synchronize_cbs.
>
> As a result in interrupt might try to take a read lock while
> .synchronize_cbs has the writer lock, resulting in a deadlock.
>
> I think you want regular read_lock + write_lock_irq.
Yes.
Thanks
>
>
> > @@ -1114,6 +1139,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
> > vq = virtio_ccw_vq_by_ind(vcdev, i);
> > vring_interrupt(0, vq);
> > }
> > + read_unlock_irqrestore(&vcdev->irq_lock, flags);
> > if (test_bit(0, indicators2(vcdev))) {
> > virtio_config_changed(&vcdev->vdev);
> > clear_bit(0, indicators2(vcdev));
> > @@ -1284,6 +1310,7 @@ static int virtio_ccw_online(struct ccw_device *cdev)
> > init_waitqueue_head(&vcdev->wait_q);
> > INIT_LIST_HEAD(&vcdev->virtqueues);
> > spin_lock_init(&vcdev->lock);
> > + rwlock_init(&vcdev->irq_lock);
> > mutex_init(&vcdev->io_lock);
> >
> > spin_lock_irqsave(get_ccwdev_lock(cdev), flags);
> > --
> > 2.25.1
>
On Wed, May 11 2022, Jason Wang <[email protected]> wrote:
> On Tue, May 10, 2022 at 7:28 PM Michael S. Tsirkin <[email protected]> wrote:
>>
>> On Sat, May 07, 2022 at 03:19:51PM +0800, Jason Wang wrote:
>> > @@ -1106,6 +1130,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
>> > vcdev->err = -EIO;
>> > }
>> > virtio_ccw_check_activity(vcdev, activity);
>> > + read_lock_irqsave(&vcdev->irq_lock, flags);
>> > for_each_set_bit(i, indicators(vcdev),
>> > sizeof(*indicators(vcdev)) * BITS_PER_BYTE) {
>> > /* The bit clear must happen before the vring kick. */
>>
>> Cornelia sent a lockdep trace on this.
>>
>> Basically I think this gets the irqsave/restore logic wrong.
>> It attempts to disable irqs in the handler (which is an interrupt
>> anyway).
>
> The reason I use irqsave/restore is that it can be called from process
> context (if I was not wrong), e.g from io_subchannel_quiesce().
io_subchannel_quiesce() should disable interrupts, though? Otherwise, it
would be a bug.
On Wed, May 11, 2022 at 4:17 PM Cornelia Huck <[email protected]> wrote:
>
> On Wed, May 11 2022, Jason Wang <[email protected]> wrote:
>
> > On Tue, May 10, 2022 at 7:28 PM Michael S. Tsirkin <[email protected]> wrote:
> >>
> >> On Sat, May 07, 2022 at 03:19:51PM +0800, Jason Wang wrote:
> >> > @@ -1106,6 +1130,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
> >> > vcdev->err = -EIO;
> >> > }
> >> > virtio_ccw_check_activity(vcdev, activity);
> >> > + read_lock_irqsave(&vcdev->irq_lock, flags);
> >> > for_each_set_bit(i, indicators(vcdev),
> >> > sizeof(*indicators(vcdev)) * BITS_PER_BYTE) {
> >> > /* The bit clear must happen before the vring kick. */
> >>
> >> Cornelia sent a lockdep trace on this.
> >>
> >> Basically I think this gets the irqsave/restore logic wrong.
> >> It attempts to disable irqs in the handler (which is an interrupt
> >> anyway).
> >
> > The reason I use irqsave/restore is that it can be called from process
> > context (if I was not wrong), e.g from io_subchannel_quiesce().
>
> io_subchannel_quiesce() should disable interrupts, though? Otherwise, it
> would be a bug.
Right, it was protected by a spin_lock_irq(), but I can see other
cdev->handler() in e.g device_fsm.c, the irq status is not obvious, do
they have the same assumption which IRQ is disabled?
Thanks
>
On Wed, May 11 2022, Jason Wang <[email protected]> wrote:
> On Wed, May 11, 2022 at 4:17 PM Cornelia Huck <[email protected]> wrote:
>>
>> On Wed, May 11 2022, Jason Wang <[email protected]> wrote:
>>
>> > On Tue, May 10, 2022 at 7:28 PM Michael S. Tsirkin <[email protected]> wrote:
>> >>
>> >> On Sat, May 07, 2022 at 03:19:51PM +0800, Jason Wang wrote:
>> >> > @@ -1106,6 +1130,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
>> >> > vcdev->err = -EIO;
>> >> > }
>> >> > virtio_ccw_check_activity(vcdev, activity);
>> >> > + read_lock_irqsave(&vcdev->irq_lock, flags);
>> >> > for_each_set_bit(i, indicators(vcdev),
>> >> > sizeof(*indicators(vcdev)) * BITS_PER_BYTE) {
>> >> > /* The bit clear must happen before the vring kick. */
>> >>
>> >> Cornelia sent a lockdep trace on this.
>> >>
>> >> Basically I think this gets the irqsave/restore logic wrong.
>> >> It attempts to disable irqs in the handler (which is an interrupt
>> >> anyway).
>> >
>> > The reason I use irqsave/restore is that it can be called from process
>> > context (if I was not wrong), e.g from io_subchannel_quiesce().
>>
>> io_subchannel_quiesce() should disable interrupts, though? Otherwise, it
>> would be a bug.
>
> Right, it was protected by a spin_lock_irq(), but I can see other
> cdev->handler() in e.g device_fsm.c, the irq status is not obvious, do
> they have the same assumption which IRQ is disabled?
Yes, that should be the case for any invocations via the fsm as well.
It's been some time since I've worked on that part of the code, though,
so let's cc: the s390 cio maintainers so that they can speak up if I'm
wrong.
On Wed, May 11, 2022 at 5:13 PM Cornelia Huck <[email protected]> wrote:
>
> On Wed, May 11 2022, Jason Wang <[email protected]> wrote:
>
> > On Wed, May 11, 2022 at 4:17 PM Cornelia Huck <[email protected]> wrote:
> >>
> >> On Wed, May 11 2022, Jason Wang <[email protected]> wrote:
> >>
> >> > On Tue, May 10, 2022 at 7:28 PM Michael S. Tsirkin <[email protected]> wrote:
> >> >>
> >> >> On Sat, May 07, 2022 at 03:19:51PM +0800, Jason Wang wrote:
> >> >> > @@ -1106,6 +1130,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
> >> >> > vcdev->err = -EIO;
> >> >> > }
> >> >> > virtio_ccw_check_activity(vcdev, activity);
> >> >> > + read_lock_irqsave(&vcdev->irq_lock, flags);
> >> >> > for_each_set_bit(i, indicators(vcdev),
> >> >> > sizeof(*indicators(vcdev)) * BITS_PER_BYTE) {
> >> >> > /* The bit clear must happen before the vring kick. */
> >> >>
> >> >> Cornelia sent a lockdep trace on this.
> >> >>
> >> >> Basically I think this gets the irqsave/restore logic wrong.
> >> >> It attempts to disable irqs in the handler (which is an interrupt
> >> >> anyway).
> >> >
> >> > The reason I use irqsave/restore is that it can be called from process
> >> > context (if I was not wrong), e.g from io_subchannel_quiesce().
> >>
> >> io_subchannel_quiesce() should disable interrupts, though? Otherwise, it
> >> would be a bug.
> >
> > Right, it was protected by a spin_lock_irq(), but I can see other
> > cdev->handler() in e.g device_fsm.c, the irq status is not obvious, do
> > they have the same assumption which IRQ is disabled?
>
> Yes, that should be the case for any invocations via the fsm as well.
>
Ok.
> It's been some time since I've worked on that part of the code, though,
> so let's cc: the s390 cio maintainers so that they can speak up if I'm
> wrong.
Ok, I will do that.
Thanks
>
On Wed, May 11, 2022 at 05:28:11PM +0800, Jason Wang wrote:
> On Wed, May 11, 2022 at 5:13 PM Cornelia Huck <[email protected]> wrote:
> >
> > On Wed, May 11 2022, Jason Wang <[email protected]> wrote:
> >
> > > On Wed, May 11, 2022 at 4:17 PM Cornelia Huck <[email protected]> wrote:
> > >>
> > >> On Wed, May 11 2022, Jason Wang <[email protected]> wrote:
> > >>
> > >> > On Tue, May 10, 2022 at 7:28 PM Michael S. Tsirkin <[email protected]> wrote:
> > >> >>
> > >> >> On Sat, May 07, 2022 at 03:19:51PM +0800, Jason Wang wrote:
> > >> >> > @@ -1106,6 +1130,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
> > >> >> > vcdev->err = -EIO;
> > >> >> > }
> > >> >> > virtio_ccw_check_activity(vcdev, activity);
> > >> >> > + read_lock_irqsave(&vcdev->irq_lock, flags);
> > >> >> > for_each_set_bit(i, indicators(vcdev),
> > >> >> > sizeof(*indicators(vcdev)) * BITS_PER_BYTE) {
> > >> >> > /* The bit clear must happen before the vring kick. */
> > >> >>
> > >> >> Cornelia sent a lockdep trace on this.
> > >> >>
> > >> >> Basically I think this gets the irqsave/restore logic wrong.
> > >> >> It attempts to disable irqs in the handler (which is an interrupt
> > >> >> anyway).
> > >> >
> > >> > The reason I use irqsave/restore is that it can be called from process
> > >> > context (if I was not wrong), e.g from io_subchannel_quiesce().
> > >>
> > >> io_subchannel_quiesce() should disable interrupts, though? Otherwise, it
> > >> would be a bug.
> > >
> > > Right, it was protected by a spin_lock_irq(), but I can see other
> > > cdev->handler() in e.g device_fsm.c, the irq status is not obvious, do
> > > they have the same assumption which IRQ is disabled?
> >
> > Yes, that should be the case for any invocations via the fsm as well.
> >
>
> Ok.
>
> > It's been some time since I've worked on that part of the code, though,
> > so let's cc: the s390 cio maintainers so that they can speak up if I'm
> > wrong.
>
> Ok, I will do that.
>
> Thanks
>
> >
Thank you Corny to looking in to this. I agree, the cdev->handler is
called with lock held. And as you mentioned, in the fsm these handler
invocations are done with IRQ disabled, which will otherwise end up in a
deadlock.
thanks.
On Wed, May 11, 2022 at 10:52 PM Vineeth Vijayan <[email protected]> wrote:
>
> On Wed, May 11, 2022 at 05:28:11PM +0800, Jason Wang wrote:
> > On Wed, May 11, 2022 at 5:13 PM Cornelia Huck <[email protected]> wrote:
> > >
> > > On Wed, May 11 2022, Jason Wang <[email protected]> wrote:
> > >
> > > > On Wed, May 11, 2022 at 4:17 PM Cornelia Huck <[email protected]> wrote:
> > > >>
> > > >> On Wed, May 11 2022, Jason Wang <[email protected]> wrote:
> > > >>
> > > >> > On Tue, May 10, 2022 at 7:28 PM Michael S. Tsirkin <[email protected]> wrote:
> > > >> >>
> > > >> >> On Sat, May 07, 2022 at 03:19:51PM +0800, Jason Wang wrote:
> > > >> >> > @@ -1106,6 +1130,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
> > > >> >> > vcdev->err = -EIO;
> > > >> >> > }
> > > >> >> > virtio_ccw_check_activity(vcdev, activity);
> > > >> >> > + read_lock_irqsave(&vcdev->irq_lock, flags);
> > > >> >> > for_each_set_bit(i, indicators(vcdev),
> > > >> >> > sizeof(*indicators(vcdev)) * BITS_PER_BYTE) {
> > > >> >> > /* The bit clear must happen before the vring kick. */
> > > >> >>
> > > >> >> Cornelia sent a lockdep trace on this.
> > > >> >>
> > > >> >> Basically I think this gets the irqsave/restore logic wrong.
> > > >> >> It attempts to disable irqs in the handler (which is an interrupt
> > > >> >> anyway).
> > > >> >
> > > >> > The reason I use irqsave/restore is that it can be called from process
> > > >> > context (if I was not wrong), e.g from io_subchannel_quiesce().
> > > >>
> > > >> io_subchannel_quiesce() should disable interrupts, though? Otherwise, it
> > > >> would be a bug.
> > > >
> > > > Right, it was protected by a spin_lock_irq(), but I can see other
> > > > cdev->handler() in e.g device_fsm.c, the irq status is not obvious, do
> > > > they have the same assumption which IRQ is disabled?
> > >
> > > Yes, that should be the case for any invocations via the fsm as well.
> > >
> >
> > Ok.
> >
> > > It's been some time since I've worked on that part of the code, though,
> > > so let's cc: the s390 cio maintainers so that they can speak up if I'm
> > > wrong.
> >
> > Ok, I will do that.
> >
> > Thanks
> >
> > >
> Thank you Corny to looking in to this. I agree, the cdev->handler is
> called with lock held. And as you mentioned, in the fsm these handler
> invocations are done with IRQ disabled, which will otherwise end up in a
> deadlock.
> thanks.
>
Thanks a lot for the confirmation, I will use
spin_lock()/spin_unlock() in the next version.