[Fwd: [PATCH]: Fix silly output for virtio devices in /proc/interrupts]

[Fwd: [PATCH]: Fix silly output for virtio devices in /proc/interrupts]

[Chris Lalancette]

[-- Attachment #1: Type: text/plain, Size: 35 bytes --]

(sent to the kvm list erroneously)

[-- Attachment #2: [PATCH]: Fix silly output for virtio devices in /proc/interrupts.eml --]
[-- Type: message/rfc822, Size: 4544 bytes --]

[-- Attachment #2.1.1: Type: text/plain, Size: 289 bytes --]

register_virtio_device() is doing something silly by overwriting what the caller
put into .bus_id.  This causes the interrupt line for all virtio devices to show
up as "0", "1", etc. in /proc/interrupts.  The attached patch fixes it.

Signed-off-by: Chris Lalancette <clalance@redhat.com>

[-- Attachment #2.1.2: virtio-blk-fix-proc-interrupts.patch --]
[-- Type: text/x-patch, Size: 1060 bytes --]

commit cb97605728fc1d7a15ddc6ab689e8bcd23871133
Author: Chris Lalancette <clalance@redhat.com>
Date:   Thu May 15 09:04:55 2008 -0400

    register_virtio_device was doing something silly, in that it was overwriting
    what the calling driver stuck into .bus_id" for the name.  This caused
    problems in the output of /proc/interrupts, since when you request_irq(),
    it doesn't actually copy the devname you pass in but just stores a pointer
    to the data.  The fix is to just not have register_virtio_device do anything
    with the bus_id, and assume the higher level driver set it up properly.

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 138a7f0..1556ac2 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -128,7 +128,6 @@ int register_virtio_device(struct virtio_device *dev)
 	int err;
 
 	dev->dev.bus = &virtio_bus;
-	sprintf(dev->dev.bus_id, "%u", dev->index);
 
 	/* We always start by resetting the device, in case a previous
 	 * driver messed it up.  This also tests that code path a little. */

[-- Attachment #3: Type: text/plain, Size: 184 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [Fwd: [PATCH]: Fix silly output for virtio devices in /proc/interrupts]

[Rusty Russell]

On Wednesday 21 May 2008 23:13:05 Chris Lalancette wrote:
> Author: Chris Lalancette <clalance@redhat.com>
> Date:   Thu May 15 09:04:55 2008 -0400
>
>     register_virtio_device was doing something silly, in that it was
> overwriting what the calling driver stuck into .bus_id" for the name.  This
> caused problems in the output of /proc/interrupts, since when you
> request_irq(), it doesn't actually copy the devname you pass in but just
> stores a pointer to the data.  The fix is to just not have
> register_virtio_device do anything with the bus_id, and assume the higher
> level driver set it up properly.

OK, but only one higher-level driver will set it up properly: kvm.  Neither
lguest nor s/390 do this, and as a result, they fail to register *any*
devices.

The following patch should fix it for s/390 (it's identical to the lguest
patch), but would prefer testing (S/390-ers cc'd).

===
virtio: S/390 set name of virtio devices directly.

Chris has a patch 'Fix silly output for virtio devices in /proc/interrupts'
which requires callers to the virtio driver infrastructure to set the bus_ids
themselves.  This does that for s/390.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Carsten Otte <cotte@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Chris Lalancette <clalance@redhat.com>
---
 drivers/s390/kvm/kvm_virtio.c |    2 ++
 1 file changed, 2 insertions(+)

diff -r c903ef6b391f drivers/s390/kvm/kvm_virtio.c
--- a/drivers/s390/kvm/kvm_virtio.c	Thu May 22 22:31:31 2008 +1000
+++ b/drivers/s390/kvm/kvm_virtio.c	Thu May 22 22:32:55 2008 +1000
@@ -265,6 +265,8 @@ static void add_kvm_device(struct kvm_de
 
 	kdev->vdev.dev.parent = &kvm_root;
 	kdev->vdev.index = dev_index++;
+	snprintf(kdev->vdev.dev.bus_id, BUS_ID_SIZE, "virtio%d",
+		 kdev->vdev.index);
 	kdev->vdev.id.device = d->type;
 	kdev->vdev.config = &kvm_vq_configspace_ops;
 	kdev->desc = d;

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [Fwd: [PATCH]: Fix silly output for virtio devices in /proc/interrupts]

[Chris Lalancette]

Rusty Russell wrote:
> On Wednesday 21 May 2008 23:13:05 Chris Lalancette wrote:
>> Author: Chris Lalancette <clalance@redhat.com>
>> Date:   Thu May 15 09:04:55 2008 -0400
>>
>>     register_virtio_device was doing something silly, in that it was
>> overwriting what the calling driver stuck into .bus_id" for the name.  This
>> caused problems in the output of /proc/interrupts, since when you
>> request_irq(), it doesn't actually copy the devname you pass in but just
>> stores a pointer to the data.  The fix is to just not have
>> register_virtio_device do anything with the bus_id, and assume the higher
>> level driver set it up properly.
> 
> OK, but only one higher-level driver will set it up properly: kvm.  Neither
> lguest nor s/390 do this, and as a result, they fail to register *any*
> devices.

Ah, OK.  Alternatively, we could do:

snprintf(bus_id, BUS_ID_SIZE, "virtio%d", index)

in register_virtio_device(), and just fix the one user who does it themselves
(kvm) to not duplicate the work.

Either way is fine with me.

Chris Lalancette

Re: [Fwd: [PATCH]: Fix silly output for virtio devices in /proc/interrupts]

[Christian Borntraeger]

Am Donnerstag, 22. Mai 2008 schrieb Rusty Russell:

> virtio: S/390 set name of virtio devices directly.
> 
> Chris has a patch 'Fix silly output for virtio devices in /proc/interrupts'
> which requires callers to the virtio driver infrastructure to set the 
bus_ids
> themselves.  This does that for s/390.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Cc: Carsten Otte <cotte@de.ibm.com>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Chris Lalancette <clalance@redhat.com>
> ---
>  drivers/s390/kvm/kvm_virtio.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff -r c903ef6b391f drivers/s390/kvm/kvm_virtio.c
> --- a/drivers/s390/kvm/kvm_virtio.c	Thu May 22 22:31:31 2008 +1000
> +++ b/drivers/s390/kvm/kvm_virtio.c	Thu May 22 22:32:55 2008 +1000
> @@ -265,6 +265,8 @@ static void add_kvm_device(struct kvm_de
>  
>  	kdev->vdev.dev.parent = &kvm_root;
>  	kdev->vdev.index = dev_index++;
> +	snprintf(kdev->vdev.dev.bus_id, BUS_ID_SIZE, "virtio%d",
> +		 kdev->vdev.index);
>  	kdev->vdev.id.device = d->type;
>  	kdev->vdev.config = &kvm_vq_configspace_ops;
>  	kdev->desc = d;
> 
> 

Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

This will probably create a fuzzy hunk with another patch from Cornelia Huck 
(CCed) which will be send to Gregs tree soon. This patch converts kvm_root to 
dynamic allocation, but the fuzz will be easy to fix.

Christian

Re: [Fwd: [PATCH]: Fix silly output for virtio devices in /proc/interrupts]

[Anthony Liguori]

Rusty Russell wrote:
> On Wednesday 21 May 2008 23:13:05 Chris Lalancette wrote:
>   
>> Author: Chris Lalancette <clalance@redhat.com>
>> Date:   Thu May 15 09:04:55 2008 -0400
>>
>>     register_virtio_device was doing something silly, in that it was
>> overwriting what the calling driver stuck into .bus_id" for the name.  This
>> caused problems in the output of /proc/interrupts, since when you
>> request_irq(), it doesn't actually copy the devname you pass in but just
>> stores a pointer to the data.  The fix is to just not have
>> register_virtio_device do anything with the bus_id, and assume the higher
>> level driver set it up properly.
>>     
>
> OK, but only one higher-level driver will set it up properly: kvm.  Neither
> lguest nor s/390 do this, and as a result, they fail to register *any*
> devices.
>
> The following patch should fix it for s/390 (it's identical to the lguest
> patch), but would prefer testing (S/390-ers cc'd).
>   

It may actually be better for virtio to set this up.  The problem is 
that if you have multiple transports that are registering virtio 
devices, it's impossible at the transport level to guarantee uniqueness 
while still using the "virtio%d" naming.  Except the current scheme is 
no good, we'd have to push the dev_index into virtio too.

Regards,

Anthony Liguori

> ===
> virtio: S/390 set name of virtio devices directly.
>
> Chris has a patch 'Fix silly output for virtio devices in /proc/interrupts'
> which requires callers to the virtio driver infrastructure to set the bus_ids
> themselves.  This does that for s/390.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Cc: Carsten Otte <cotte@de.ibm.com>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Chris Lalancette <clalance@redhat.com>
> ---
>  drivers/s390/kvm/kvm_virtio.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff -r c903ef6b391f drivers/s390/kvm/kvm_virtio.c
> --- a/drivers/s390/kvm/kvm_virtio.c	Thu May 22 22:31:31 2008 +1000
> +++ b/drivers/s390/kvm/kvm_virtio.c	Thu May 22 22:32:55 2008 +1000
> @@ -265,6 +265,8 @@ static void add_kvm_device(struct kvm_de
>  
>  	kdev->vdev.dev.parent = &kvm_root;
>  	kdev->vdev.index = dev_index++;
> +	snprintf(kdev->vdev.dev.bus_id, BUS_ID_SIZE, "virtio%d",
> +		 kdev->vdev.index);
>  	kdev->vdev.id.device = d->type;
>  	kdev->vdev.config = &kvm_vq_configspace_ops;
>  	kdev->desc = d;
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: [Fwd: [PATCH]: Fix silly output for virtio devices in /proc/interrupts]

[Rusty Russell]

On Friday 23 May 2008 07:58:44 Anthony Liguori wrote:
> Rusty Russell wrote:
> > On Wednesday 21 May 2008 23:13:05 Chris Lalancette wrote:
> >> Author: Chris Lalancette <clalance@redhat.com>
> >> Date:   Thu May 15 09:04:55 2008 -0400
> >>
> >>     register_virtio_device was doing something silly, in that it was
> >> overwriting what the calling driver stuck into .bus_id" for the name. 
> >> This caused problems in the output of /proc/interrupts, since when you
> >> request_irq(), it doesn't actually copy the devname you pass in but just
> >> stores a pointer to the data.  The fix is to just not have
> >> register_virtio_device do anything with the bus_id, and assume the
> >> higher level driver set it up properly.
> >
> > OK, but only one higher-level driver will set it up properly: kvm. 
> > Neither lguest nor s/390 do this, and as a result, they fail to register
> > *any* devices.
> >
> > The following patch should fix it for s/390 (it's identical to the lguest
> > patch), but would prefer testing (S/390-ers cc'd).
>
> It may actually be better for virtio to set this up.  The problem is
> that if you have multiple transports that are registering virtio
> devices, it's impossible at the transport level to guarantee uniqueness
> while still using the "virtio%d" naming.  Except the current scheme is
> no good, we'd have to push the dev_index into virtio too.

That's true.  OK, let's hoist the index counter into common code instead.  The 
alternative is to use different busid namings for each transport, and that 
seems like too much confusion: sysfs will tell you where it comes from if you 
really need to know.

Patch series coming.

Thanks,
Rusty.