Linux virtualization list
 help / color / mirror / Atom feed
* RE: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
From: KY Srinivasan @ 2011-03-14 12:43 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
	virtualization@lists.osdl.org, Haiyang Zhang, Mike Sterling,
	Abhishek Kane (Mindtree Consulting PVT LTD), Hank Janssen
In-Reply-To: <20110314032437.GA18645@suse.de>



> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Sunday, March 13, 2011 11:25 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane
> (Mindtree Consulting PVT LTD); Hank Janssen
> Subject: Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
> 
> On Sat, Mar 12, 2011 at 11:23:05PM +0000, KY Srinivasan wrote:
> > Greg, I have redone this patch as well as [PATCH 12/21]. Do you want
> > me send you just these two patches or the entire series including
> > these two.
> 
> Just resend those two patches if that is easier.

Will do.
> 
> > Also, does this patch-set address all of architectural issues you had
> > noted earlier in the vmbus core. Please let us know what else needs to
> > be done to exit staging as far as the vmbus driver is concerned. I
> > want get a head start before the new week begins! Also, we have
> > patches ready for all DPRINT cleanup. Hank is holding them off until
> > we finish addressing the architectural issues first.
> 
> I do not know if this addresses everything, sorry, I have not had the
> time to review all of them yet.  Give me a few days at the least to go
> over them and apply them before I will be able to tell you this.

Thanks for taking the time to look at this.

> 
> Also note that there shouldn't be anything holding back the DPRINT
> stuff, why wait?  If they apply on top of yours that should be fine,
> right?

You are right. We will submit the DPRINT patches soon.

Regards,

K. Y 

^ permalink raw reply

* [PATCH] virtio: console: Enable call to hvc_remove() on console port remove
From: Amit Shah @ 2011-03-14 12:15 UTC (permalink / raw)
  To: Rusty Russell; +Cc: Amit Shah, Virtualization List

This call was disabled as hot-unplugging one virtconsole port led to
another virtconsole port freezing.

Upon testing it again, this now works, so enable it.

In addition, a bug was found in qemu wherein removing a port of one type
caused the guest output from another port to stop working.  I doubt it
was just this bug that caused it (since disabling the hvc_remove() call
did allow other ports to continue working), but since it's all solved
now, we're fine with hot-unplugging of virtconsole ports.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 84b164d..838568a 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1280,18 +1280,7 @@ static void unplug_port(struct port *port)
 		spin_lock_irq(&pdrvdata_lock);
 		list_del(&port->cons.list);
 		spin_unlock_irq(&pdrvdata_lock);
-#if 0
-		/*
-		 * hvc_remove() not called as removing one hvc port
-		 * results in other hvc ports getting frozen.
-		 *
-		 * Once this is resolved in hvc, this functionality
-		 * will be enabled.  Till that is done, the -EPIPE
-		 * return from get_chars() above will help
-		 * hvc_console.c to clean up on ports we remove here.
-		 */
 		hvc_remove(port->cons.hvc);
-#endif
 	}
 
 	/* Remove unused data this port might have received. */
-- 
1.7.4

^ permalink raw reply related

* [PATCH] virtio_pci: Prevent double-free of pci regions after device hot-unplug
From: Amit Shah @ 2011-03-14 12:15 UTC (permalink / raw)
  To: Rusty Russell
  Cc: Amit Shah, Mark McLoughlin, Virtualization List,
	Michael S. Tsirkin

In the case where a virtio-console port is in use (opened by a program)
and a virtio-console device is removed, the port is kept around but all
the virtio-related state is assumed to be gone.

When the port is finally released (close() called), we call
device_destroy() on the port's device.  This results in the parent
device's structures to be freed as well.  This includes the PCI regions
for the virtio-console PCI device.

Once this is done, however, virtio_pci_release_dev() kicks in, as the
last ref to the virtio device is now gone, and attempts to do

     pci_iounmap(pci_dev, vp_dev->ioaddr);
     pci_release_regions(pci_dev);
     pci_disable_device(pci_dev);

which results in a double-free warning.

Move the code that releases regions, etc., to the virtio_pci_remove()
function, and all that's now left in release_dev is the final freeing of
the vp_dev.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/virtio/virtio_pci.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 4fb5b2b..4bcc8b8 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -590,15 +590,10 @@ static struct virtio_config_ops virtio_pci_config_ops = {
 
 static void virtio_pci_release_dev(struct device *_d)
 {
-	struct virtio_device *dev = container_of(_d, struct virtio_device, dev);
+	struct virtio_device *dev = container_of(_d, struct virtio_device,
+						 dev);
 	struct virtio_pci_device *vp_dev = to_vp_device(dev);
-	struct pci_dev *pci_dev = vp_dev->pci_dev;
 
-	vp_del_vqs(dev);
-	pci_set_drvdata(pci_dev, NULL);
-	pci_iounmap(pci_dev, vp_dev->ioaddr);
-	pci_release_regions(pci_dev);
-	pci_disable_device(pci_dev);
 	kfree(vp_dev);
 }
 
@@ -681,6 +676,12 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
 	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
 
 	unregister_virtio_device(&vp_dev->vdev);
+
+	vp_del_vqs(&vp_dev->vdev);
+	pci_set_drvdata(pci_dev, NULL);
+	pci_iounmap(pci_dev, vp_dev->ioaddr);
+	pci_release_regions(pci_dev);
+	pci_disable_device(pci_dev);
 }
 
 #ifdef CONFIG_PM
-- 
1.7.4

^ permalink raw reply related

* Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
From: Greg KH @ 2011-03-14  3:24 UTC (permalink / raw)
  To: KY Srinivasan
  Cc: Abhishek Kane (Mindtree Consulting PVT LTD), Haiyang Zhang,
	linux-kernel@vger.kernel.org, virtualization@lists.osdl.org,
	Mike Sterling, devel@linuxdriverproject.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E048018F4D@TK5EX14MBXC128.redmond.corp.microsoft.com>

On Sat, Mar 12, 2011 at 11:23:05PM +0000, KY Srinivasan wrote:
> Greg, I have redone this patch as well as [PATCH 12/21]. Do you want
> me send you just these two patches or the entire series including
> these two.

Just resend those two patches if that is easier.

> Also, does this patch-set address all of architectural issues you had
> noted earlier in the vmbus core. Please let us know what else needs to
> be done to exit staging as far as the vmbus driver is concerned. I
> want get a head start before the new week begins! Also, we have
> patches ready for all DPRINT cleanup. Hank is holding them off until
> we finish addressing the architectural issues first.

I do not know if this addresses everything, sorry, I have not had the
time to review all of them yet.  Give me a few days at the least to go
over them and apply them before I will be able to tell you this.

Also note that there shouldn't be anything holding back the DPRINT
stuff, why wait?  If they apply on top of yours that should be fine,
right?

thanks,

greg k-h

^ permalink raw reply

* Re: [PATCH 3/3] vhost-net: use lock_sock_fast() in peek_head_len()
From: Michael S. Tsirkin @ 2011-03-13 21:11 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Jason Wang, virtualization, netdev, kvm, linux-kernel
In-Reply-To: <1300038092.2761.41.camel@edumazet-laptop>

On Sun, Mar 13, 2011 at 06:41:32PM +0100, Eric Dumazet wrote:
> Le dimanche 13 mars 2011 à 18:43 +0200, Michael S. Tsirkin a écrit :
> > On Sun, Mar 13, 2011 at 05:32:07PM +0100, Eric Dumazet wrote:
> > > Le dimanche 13 mars 2011 à 18:19 +0200, Michael S. Tsirkin a écrit :
> > > 
> > > > Other side is in drivers/net/tun.c and net/packet/af_packet.c
> > > > At least wrt tun it seems clear socket is not locked.
> > > 
> > > Yes (assuming you refer to tun_net_xmit())
> > > 
> > > > Besides queue, dequeue seems to be done without socket locked.
> > > > 
> > > 
> > > It seems this code (assuming you speak of drivers/vhost/net.c ?) has
> > > some races indeed.
> > > 
> > 
> > Hmm. Any more besides the one fixed here?
> > 
> 
> If writers and readers dont share a common lock, how can they reliably
> synchronize states ?

They are all supposed to use sk_receive_queue.lock I think.

> For example, the check at line 420 seems unsafe or useless.
> 
> skb_queue_empty(&sock->sk->sk_receive_queue)
> 

It's mostly useless: code that is called after this
 does skb_peek and checks the result under the spinlock.
This was supposed to be an optimization: quickly check
that queue is not empty before we bother disabling notifications
etc, but I dont' remember at this point whether it actually gives any gain.
Thanks for pointing this out, I'll take it out I think (below).
Note: there are two places of this call in upstream: handle_rx_bug and
handle_rx_mergeable, but they are merged into a single
handle_rx by a patch by Jason Wang.
The below patch is on top.
If you like to look at the latest code,
it's here master.kernel.org:/home/mst/pub/vhost.git
branch vhost-net-next has it all.

Eric, thanks very much for pointing out these.
Is there anything else that you see in this driver?


Thanks!


    vhost-net: remove unlocked use of receive_queue
    
    Use of skb_queue_empty(&sock->sk->sk_receive_queue)
    without taking the sk_receive_queue.lock is unsafe
    or useless. Take it out.
    
    Reported-by:  Eric Dumazet <eric.dumazet@gmail.com>
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 5720301..2f7c76a 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -311,7 +311,7 @@ static void handle_rx(struct vhost_net *net)
 	/* TODO: check that we are running from vhost_worker? */
 	struct socket *sock = rcu_dereference_check(vq->private_data, 1);
 
-	if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
+	if (!sock)
 		return;
 
 	mutex_lock(&vq->mutex);

^ permalink raw reply related

* Re: [PATCH 3/3] vhost-net: use lock_sock_fast() in peek_head_len()
From: Eric Dumazet @ 2011-03-13 17:41 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Jason Wang, virtualization, netdev, kvm, linux-kernel
In-Reply-To: <20110313164359.GA32562@redhat.com>

Le dimanche 13 mars 2011 à 18:43 +0200, Michael S. Tsirkin a écrit :
> On Sun, Mar 13, 2011 at 05:32:07PM +0100, Eric Dumazet wrote:
> > Le dimanche 13 mars 2011 à 18:19 +0200, Michael S. Tsirkin a écrit :
> > 
> > > Other side is in drivers/net/tun.c and net/packet/af_packet.c
> > > At least wrt tun it seems clear socket is not locked.
> > 
> > Yes (assuming you refer to tun_net_xmit())
> > 
> > > Besides queue, dequeue seems to be done without socket locked.
> > > 
> > 
> > It seems this code (assuming you speak of drivers/vhost/net.c ?) has
> > some races indeed.
> > 
> 
> Hmm. Any more besides the one fixed here?
> 

If writers and readers dont share a common lock, how can they reliably
synchronize states ?

For example, the check at line 420 seems unsafe or useless.

skb_queue_empty(&sock->sk->sk_receive_queue)

^ permalink raw reply

* Re: [PATCH 3/3] vhost-net: use lock_sock_fast() in peek_head_len()
From: Michael S. Tsirkin @ 2011-03-13 16:43 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Jason Wang, virtualization, netdev, kvm, linux-kernel
In-Reply-To: <1300033927.2761.26.camel@edumazet-laptop>

On Sun, Mar 13, 2011 at 05:32:07PM +0100, Eric Dumazet wrote:
> Le dimanche 13 mars 2011 à 18:19 +0200, Michael S. Tsirkin a écrit :
> 
> > Other side is in drivers/net/tun.c and net/packet/af_packet.c
> > At least wrt tun it seems clear socket is not locked.
> 
> Yes (assuming you refer to tun_net_xmit())
> 
> > Besides queue, dequeue seems to be done without socket locked.
> > 
> 
> It seems this code (assuming you speak of drivers/vhost/net.c ?) has
> some races indeed.
> 

Hmm. Any more besides the one fixed here?

-- 
MST

^ permalink raw reply

* Re: [PATCH 3/3] vhost-net: use lock_sock_fast() in peek_head_len()
From: Eric Dumazet @ 2011-03-13 16:32 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Jason Wang, virtualization, netdev, kvm, linux-kernel
In-Reply-To: <20110313161915.GB30642@redhat.com>

Le dimanche 13 mars 2011 à 18:19 +0200, Michael S. Tsirkin a écrit :

> Other side is in drivers/net/tun.c and net/packet/af_packet.c
> At least wrt tun it seems clear socket is not locked.

Yes (assuming you refer to tun_net_xmit())

> Besides queue, dequeue seems to be done without socket locked.
> 

It seems this code (assuming you speak of drivers/vhost/net.c ?) has
some races indeed.

^ permalink raw reply

* Re: [PATCH 3/3] vhost-net: use lock_sock_fast() in peek_head_len()
From: Michael S. Tsirkin @ 2011-03-13 16:19 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Jason Wang, virtualization, netdev, kvm, linux-kernel
In-Reply-To: <1300031570.2761.22.camel@edumazet-laptop>

On Sun, Mar 13, 2011 at 04:52:50PM +0100, Eric Dumazet wrote:
> Le dimanche 13 mars 2011 à 17:06 +0200, Michael S. Tsirkin a écrit :
> > On Mon, Jan 17, 2011 at 04:11:17PM +0800, Jason Wang wrote:
> > > We can use lock_sock_fast() instead of lock_sock() in order to get
> > > speedup in peek_head_len().
> > > 
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  drivers/vhost/net.c |    4 ++--
> > >  1 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > index c32a2e4..50b622a 100644
> > > --- a/drivers/vhost/net.c
> > > +++ b/drivers/vhost/net.c
> > > @@ -211,12 +211,12 @@ static int peek_head_len(struct sock *sk)
> > >  {
> > >  	struct sk_buff *head;
> > >  	int len = 0;
> > > +	bool slow = lock_sock_fast(sk);
> > >  
> > > -	lock_sock(sk);
> > >  	head = skb_peek(&sk->sk_receive_queue);
> > >  	if (head)
> > >  		len = head->len;
> > > -	release_sock(sk);
> > > +	unlock_sock_fast(sk, slow);
> > >  	return len;
> > >  }
> > >  
> > 
> > Wanted to apply this, but looking at the code I think the lock_sock here
> > is wrong. What we really need is to handle the case where the skb is
> > pulled from the receive queue after skb_peek.  However this is not the
> > right lock to use for that, sk_receive_queue.lock is.
> > So I expect the following is the right way to handle this.
> > Comments?
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 0329c41..5720301 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -213,12 +213,13 @@ static int peek_head_len(struct sock *sk)
> >  {
> >  	struct sk_buff *head;
> >  	int len = 0;
> > +	unsigned long flags;
> >  
> > -	lock_sock(sk);
> > +	spin_lock_irqsave(&sk->sk_receive_queue.lock, flags);
> >  	head = skb_peek(&sk->sk_receive_queue);
> > -	if (head)
> > +	if (likely(head))
> >  		len = head->len;
> > -	release_sock(sk);
> > +	spin_unlock_irqrestore(&sk->sk_receive_queue.lock, flags);
> >  	return len;
> >  }
> >  
> 
> You may be right, only way to be sure is to check the other side.
> 
> If it uses skb_queue_tail(), then yes, your patch is fine.
> 
> If other side did not lock socket, then your patch is a bug fix.
> 
> 

Other side is in drivers/net/tun.c and net/packet/af_packet.c
At least wrt tun it seems clear socket is not locked.
Besides queue, dequeue seems to be done without socket locked.

-- 
MST

^ permalink raw reply

* Re: [PATCH 3/3] vhost-net: use lock_sock_fast() in peek_head_len()
From: Eric Dumazet @ 2011-03-13 15:52 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Jason Wang, virtualization, netdev, kvm, linux-kernel
In-Reply-To: <20110313150646.GA30494@redhat.com>

Le dimanche 13 mars 2011 à 17:06 +0200, Michael S. Tsirkin a écrit :
> On Mon, Jan 17, 2011 at 04:11:17PM +0800, Jason Wang wrote:
> > We can use lock_sock_fast() instead of lock_sock() in order to get
> > speedup in peek_head_len().
> > 
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  drivers/vhost/net.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index c32a2e4..50b622a 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -211,12 +211,12 @@ static int peek_head_len(struct sock *sk)
> >  {
> >  	struct sk_buff *head;
> >  	int len = 0;
> > +	bool slow = lock_sock_fast(sk);
> >  
> > -	lock_sock(sk);
> >  	head = skb_peek(&sk->sk_receive_queue);
> >  	if (head)
> >  		len = head->len;
> > -	release_sock(sk);
> > +	unlock_sock_fast(sk, slow);
> >  	return len;
> >  }
> >  
> 
> Wanted to apply this, but looking at the code I think the lock_sock here
> is wrong. What we really need is to handle the case where the skb is
> pulled from the receive queue after skb_peek.  However this is not the
> right lock to use for that, sk_receive_queue.lock is.
> So I expect the following is the right way to handle this.
> Comments?
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 0329c41..5720301 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -213,12 +213,13 @@ static int peek_head_len(struct sock *sk)
>  {
>  	struct sk_buff *head;
>  	int len = 0;
> +	unsigned long flags;
>  
> -	lock_sock(sk);
> +	spin_lock_irqsave(&sk->sk_receive_queue.lock, flags);
>  	head = skb_peek(&sk->sk_receive_queue);
> -	if (head)
> +	if (likely(head))
>  		len = head->len;
> -	release_sock(sk);
> +	spin_unlock_irqrestore(&sk->sk_receive_queue.lock, flags);
>  	return len;
>  }
>  

You may be right, only way to be sure is to check the other side.

If it uses skb_queue_tail(), then yes, your patch is fine.

If other side did not lock socket, then your patch is a bug fix.




^ permalink raw reply

* Re: [PATCH 3/3] vhost-net: use lock_sock_fast() in peek_head_len()
From: Michael S. Tsirkin @ 2011-03-13 15:06 UTC (permalink / raw)
  To: Jason Wang; +Cc: virtualization, netdev, kvm, linux-kernel
In-Reply-To: <20110117081117.18900.48672.stgit@dhcp-91-7.nay.redhat.com.englab.nay.redhat.com>

On Mon, Jan 17, 2011 at 04:11:17PM +0800, Jason Wang wrote:
> We can use lock_sock_fast() instead of lock_sock() in order to get
> speedup in peek_head_len().
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/vhost/net.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index c32a2e4..50b622a 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -211,12 +211,12 @@ static int peek_head_len(struct sock *sk)
>  {
>  	struct sk_buff *head;
>  	int len = 0;
> +	bool slow = lock_sock_fast(sk);
>  
> -	lock_sock(sk);
>  	head = skb_peek(&sk->sk_receive_queue);
>  	if (head)
>  		len = head->len;
> -	release_sock(sk);
> +	unlock_sock_fast(sk, slow);
>  	return len;
>  }
>  

Wanted to apply this, but looking at the code I think the lock_sock here
is wrong. What we really need is to handle the case where the skb is
pulled from the receive queue after skb_peek.  However this is not the
right lock to use for that, sk_receive_queue.lock is.
So I expect the following is the right way to handle this.
Comments?

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 0329c41..5720301 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -213,12 +213,13 @@ static int peek_head_len(struct sock *sk)
 {
 	struct sk_buff *head;
 	int len = 0;
+	unsigned long flags;
 
-	lock_sock(sk);
+	spin_lock_irqsave(&sk->sk_receive_queue.lock, flags);
 	head = skb_peek(&sk->sk_receive_queue);
-	if (head)
+	if (likely(head))
 		len = head->len;
-	release_sock(sk);
+	spin_unlock_irqrestore(&sk->sk_receive_queue.lock, flags);
 	return len;
 }
 

^ permalink raw reply related

* RE: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
From: KY Srinivasan @ 2011-03-12 23:23 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
	virtualization@lists.osdl.org, Haiyang Zhang, Mike Sterling,
	Abhishek Kane (Mindtree Consulting PVT LTD), Hank Janssen
In-Reply-To: <20110310223256.GA8669@suse.de>



> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Thursday, March 10, 2011 5:33 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane
> (Mindtree Consulting PVT LTD); Hank Janssen
> Subject: Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
> 
> On Thu, Mar 10, 2011 at 10:28:27PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@suse.de]
> > > Sent: Thursday, March 10, 2011 5:21 PM
> > > To: KY Srinivasan
> > > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > > virtualization@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane
> > > (Mindtree Consulting PVT LTD); Hank Janssen
> > > Subject: Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci
> driver
> > >
> > > On Thu, Mar 10, 2011 at 02:08:32PM -0800, K. Y. Srinivasan wrote:
> > > > Make vmbus driver a platform pci driver. This is
> > > > in preparation to cleaning up irq allocation for this
> > > > driver.
> > >
> > > The idea is nice, but the nameing is a bit confusing.
> > >
> > > We have "platform drivers" which are much different from what you are
> > > doing here, you are just creating a "normal" pci driver.
> > >
> > > Very minor comments below.
> > >
> > > >
> > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > Signed-off-by: Mike Sterling <mike.sterling@microsoft.com>
> > > > Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
> > > > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> > > > ---
> > > >  drivers/staging/hv/vmbus_drv.c |   63 +++++++++++++++++++++++--------
> -----
> > > ----
> > > >  1 files changed, 36 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/hv/vmbus_drv.c
> b/drivers/staging/hv/vmbus_drv.c
> > > > index 8b9394a..e4855ac 100644
> > > > --- a/drivers/staging/hv/vmbus_drv.c
> > > > +++ b/drivers/staging/hv/vmbus_drv.c
> > > > @@ -43,6 +43,8 @@
> > > >
> > > >  static struct device *root_dev; /* Root device */
> > > >
> > > > +struct pci_dev *hv_pci_dev;
> > > > +
> > > >  /* Main vmbus driver data structure */
> > > >  struct vmbus_driver_context {
> > > >
> > > > @@ -887,36 +889,24 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
> > > >  	}
> > > >  }
> > > >
> > > > -static struct dmi_system_id __initdata microsoft_hv_dmi_table[] = {
> > > > -	{
> > > > -		.ident = "Hyper-V",
> > > > -		.matches = {
> > > > -			DMI_MATCH(DMI_SYS_VENDOR, "Microsoft
> > > Corporation"),
> > > > -			DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
> > > > -			DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
> > > > -		},
> > > > -	},
> > > > -	{ },
> > > > -};
> > > > -MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
> > >
> > > You're sure it's safe to delete this now and just rely on the PCI ids,
> > > right?  For some wierd reason I thought we needed both to catch all
> > > types of systems, but I can't remember why.
> > I have tested this; I don't think we need the dmi table.
> 
> Ok, if you are sure, that's fine with me.
> 
> > > How about "hv_bus" as a name, as that's what this really is.  It's a
> > > "bus adapter", like USB, Firewire, and all sorts of other bus
> > > controllers.
> >
> > Sure; I will make these changes. Would you mind if I submit these name
> changes as a separate patch.
> 
> How about just redo this patch?  I haven't reviewed the others yet, so
> you might want to wait a day to see if I don't like any of them either
> :)

Greg, I have redone this patch as well as [PATCH 12/21]. Do you want me send you just these two 
patches or the entire series including these two. Also, does this patch-set address all of architectural 
issues you had noted earlier in the vmbus core. Please let us know what else needs to be done to 
exit staging as far as the vmbus driver is concerned. I want get a head start before the new week
begins! Also, we have patches ready for all DPRINT cleanup. Hank is holding them off until
we finish addressing the architectural issues first.

Regards,

K. Y

^ permalink raw reply

* Re: [PATCH 0/2] Fix hot-unplug: device removal while port in use
From: Amit Shah @ 2011-03-11 11:16 UTC (permalink / raw)
  To: Virtualization List; +Cc: markmc, Michael S. Tsirkin
In-Reply-To: <cover.1299054025.git.amit.shah@redhat.com>

On (Wed) 02 Mar 2011 [13:53:06], Amit Shah wrote:
> The second patch fixes a warning where the pci region is already
> freed.  I'm not sure what or how the region gets freed, any clues
> there will be helpful.

OK, so in the case where a virtio-console port is in use (opened by a
program) and a virtio-console device is removed, the port is kept
around but all the virtio-related state is assumed to be gone.

When the port is finally released (close() called), we call
device_destroy() on the port's device.  This results in the parent
device's structures to be freed as well.  This includes the PCI
regions for the virtio-console PCI device.

Once this is done, however, virtio_pci_release_dev() kicks in, as the
last ref to the virtio device is now gone, and attempts to do

     pci_iounmap(pci_dev, vp_dev->ioaddr);
     pci_release_regions(pci_dev);
     pci_disable_device(pci_dev);

which results in the double-free warning.

Ideally virtio_pci_release_dev() shouldn't be needed at all; all that
work can be moved to virtio_pci_remove().  virtio_pci_release_dev()
was added in 29f9f12e to curb a warning:

    virtio: add PCI device release() function
    
    Add a release() function for virtio_pci devices so as to avoid:
    
      Device 'virtio0' does not have a release() function, it is
      broken and must be fixed
    

So we could have an empty release() function that does nothing, and
all of the current functionality be moved to virtio_pci_remove(), as
it was earlier.  This should keep everyone happy.

Is that fine?

		Amit

^ permalink raw reply

* RE: [PATCH 13/21] [PATCH 13/21] Staging: hv: Rename vmbus_driver_context structure
From: KY Srinivasan @ 2011-03-11  3:17 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
	devel@linuxdriverproject.org, virtualization@lists.osdl.org,
	Haiyang Zhang, Mike Sterling,
	Abhishek Kane (Mindtree Consulting PVT LTD), Hank Janssen
In-Reply-To: <alpine.LFD.2.00.1103102347450.2787@localhost6.localdomain6>



> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Thursday, March 10, 2011 5:50 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang; Mike
> Sterling; Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen
> Subject: Re: [PATCH 13/21] [PATCH 13/21] Staging: hv: Rename
> vmbus_driver_context structure
> 
> On Thu, 10 Mar 2011, K. Y. Srinivasan wrote:
> > Now that struct vmbus_driver_context is properly
> > cleaned up, rename this structure appropriately and
> > cleanup the code.
> 
> > @@ -877,10 +873,10 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
> >  	/* Schedules a dpc if necessary */
> >  	if (ret > 0) {
> >  		if (test_bit(0, (unsigned long *)&ret))
> > -			tasklet_schedule(&vmbus_drv.msg_dpc);
> > +			tasklet_schedule(&hv_bus.msg_dpc);
> >
> >  		if (test_bit(1, (unsigned long *)&ret))
> > -			tasklet_schedule(&vmbus_drv.event_dpc);
> > +			tasklet_schedule(&hv_bus.event_dpc);
> 
> What's the plan to convert that tasklet/work stuff to threaded irqs ?


This is on my plate; I was going to work on this after I addressed all other issues 
preventing the vmbus driver from exiting staging.

Regards,

K. Y 

^ permalink raw reply

* RE: [PATCH 12/21] Staging: hv: Cleanup irq management
From: KY Srinivasan @ 2011-03-11  2:09 UTC (permalink / raw)
  To: Hank Janssen, Thomas Gleixner
  Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
	devel@linuxdriverproject.org, virtualization@lists.osdl.org,
	Haiyang Zhang, Mike Sterling,
	Abhishek Kane (Mindtree Consulting PVT LTD)
In-Reply-To: <E2765C4D-2B98-4D06-8007-3416952F19FD@microsoft.com>



> -----Original Message-----
> From: Hank Janssen
> Sent: Thursday, March 10, 2011 5:54 PM
> To: Thomas Gleixner
> Cc: KY Srinivasan; gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang; Mike
> Sterling; Abhishek Kane (Mindtree Consulting PVT LTD)
> Subject: Re: [PATCH 12/21] Staging: hv: Cleanup irq management
> 
> 
> 
> 
> 
> 
> On Mar 10, 2011, at 14:46, "Thomas Gleixner" <tglx@linutronix.de> wrote:
> >>
> >
> >>    }
> >> -    vector = VMBUS_IRQ_VECTOR;
> >>
> >> -    DPRINT_INFO(VMBUS_DRV, "irq 0x%x vector 0x%x", vmbus_irq, vector);
> >> +    vector = IRQ0_VECTOR + pdev->irq;
> >> +    DPRINT_INFO(VMBUS_DRV, "irq 0x%x vector 0x%x", pdev->irq,
> >> +            IRQ0_VECTOR + pdev->irq);
> >
> > Why evaluating vector first and then not using it for that debug print
> > thingy?
Good point; I will fix this before Hank gets rid of the DPRINT_INFO altogether.

Regards,

K. Y

^ permalink raw reply

* Re: [PATCH 12/21] Staging: hv: Cleanup irq management
From: Hank Janssen @ 2011-03-10 22:54 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: KY Srinivasan, gregkh@suse.de, linux-kernel@vger.kernel.org,
	devel@linuxdriverproject.org, virtualization@lists.osdl.org,
	Haiyang Zhang, Mike Sterling,
	Abhishek Kane (Mindtree Consulting PVT LTD)
In-Reply-To: <alpine.LFD.2.00.1103102338230.2787@localhost6.localdomain6>






On Mar 10, 2011, at 14:46, "Thomas Gleixner" <tglx@linutronix.de> wrote:
>> 
> 
>>    }
>> -    vector = VMBUS_IRQ_VECTOR;
>> 
>> -    DPRINT_INFO(VMBUS_DRV, "irq 0x%x vector 0x%x", vmbus_irq, vector);
>> +    vector = IRQ0_VECTOR + pdev->irq;
>> +    DPRINT_INFO(VMBUS_DRV, "irq 0x%x vector 0x%x", pdev->irq,
>> +            IRQ0_VECTOR + pdev->irq);
> 
> Why evaluating vector first and then not using it for that debug print
> thingy?
> 
> Btw, are you going to replace that DPRINT_* stuff as well ?
> 
> Thanks,
> 
>     

Yes, that is in my next set of patches. 

Hank

^ permalink raw reply

* Re: [PATCH 13/21] [PATCH 13/21] Staging: hv: Rename vmbus_driver_context structure
From: Thomas Gleixner @ 2011-03-10 22:49 UTC (permalink / raw)
  To: K. Y. Srinivasan
  Cc: Abhishek Kane, Haiyang Zhang, gregkh, linux-kernel,
	virtualization, Mike Sterling, devel
In-Reply-To: <1299795131-1335-1-git-send-email-kys@microsoft.com>

On Thu, 10 Mar 2011, K. Y. Srinivasan wrote:
> Now that struct vmbus_driver_context is properly
> cleaned up, rename this structure appropriately and
> cleanup the code.

> @@ -877,10 +873,10 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
>  	/* Schedules a dpc if necessary */
>  	if (ret > 0) {
>  		if (test_bit(0, (unsigned long *)&ret))
> -			tasklet_schedule(&vmbus_drv.msg_dpc);
> +			tasklet_schedule(&hv_bus.msg_dpc);
>  
>  		if (test_bit(1, (unsigned long *)&ret))
> -			tasklet_schedule(&vmbus_drv.event_dpc);
> +			tasklet_schedule(&hv_bus.event_dpc);

What's the plan to convert that tasklet/work stuff to threaded irqs ?

Thanks,

	tglx

^ permalink raw reply

* Re: [PATCH 12/21] Staging: hv: Cleanup irq management
From: Thomas Gleixner @ 2011-03-10 22:46 UTC (permalink / raw)
  To: K. Y. Srinivasan
  Cc: gregkh, linux-kernel, devel, virtualization, Haiyang Zhang,
	Mike Sterling, Abhishek Kane, Hank Janssen
In-Reply-To: <1299794933-1259-1-git-send-email-kys@microsoft.com>

On Thu, 10 Mar 2011, K. Y. Srinivasan wrote:
> Now that vmbus_driver is a platform pci driver,
> cleanup the irq allocation mess by using the standard
> irq allocation mechanisms.
> 
> Note that this patch generates an error when the checkpatch
> script is run because of the IRQF_SAMPLE_RANDOM flag used in
> request_irq() function. This interrupt is the only
> external event this VM will get and consequently if this
> flag (IRQF_SAMPLE_RANDOM) is not  specified, experimentally
> we have shown that the entropy in the VM will very very low.

Fair enough. We need to come up with some way to work around
this though.
 
>  	}
> -	vector = VMBUS_IRQ_VECTOR;
>  
> -	DPRINT_INFO(VMBUS_DRV, "irq 0x%x vector 0x%x", vmbus_irq, vector);
> +	vector = IRQ0_VECTOR + pdev->irq;
> +	DPRINT_INFO(VMBUS_DRV, "irq 0x%x vector 0x%x", pdev->irq,
> +			IRQ0_VECTOR + pdev->irq);

Why evaluating vector first and then not using it for that debug print
thingy?

Btw, are you going to replace that DPRINT_* stuff as well ?

Thanks,

	tglx

^ permalink raw reply

* RE: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
From: KY Srinivasan @ 2011-03-10 22:36 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
	virtualization@lists.osdl.org, Haiyang Zhang, Mike Sterling,
	Abhishek Kane (Mindtree Consulting PVT LTD), Hank Janssen
In-Reply-To: <20110310223256.GA8669@suse.de>



> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Thursday, March 10, 2011 5:33 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane
> (Mindtree Consulting PVT LTD); Hank Janssen
> Subject: Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
> 
> On Thu, Mar 10, 2011 at 10:28:27PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@suse.de]
> > > Sent: Thursday, March 10, 2011 5:21 PM
> > > To: KY Srinivasan
> > > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > > virtualization@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane
> > > (Mindtree Consulting PVT LTD); Hank Janssen
> > > Subject: Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci
> driver
> > >
> > > On Thu, Mar 10, 2011 at 02:08:32PM -0800, K. Y. Srinivasan wrote:
> > > > Make vmbus driver a platform pci driver. This is
> > > > in preparation to cleaning up irq allocation for this
> > > > driver.
> > >
> > > The idea is nice, but the nameing is a bit confusing.
> > >
> > > We have "platform drivers" which are much different from what you are
> > > doing here, you are just creating a "normal" pci driver.
> > >
> > > Very minor comments below.
> > >
> > > >
> > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > Signed-off-by: Mike Sterling <mike.sterling@microsoft.com>
> > > > Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
> > > > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> > > > ---
> > > >  drivers/staging/hv/vmbus_drv.c |   63 +++++++++++++++++++++++--------
> -----
> > > ----
> > > >  1 files changed, 36 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/hv/vmbus_drv.c
> b/drivers/staging/hv/vmbus_drv.c
> > > > index 8b9394a..e4855ac 100644
> > > > --- a/drivers/staging/hv/vmbus_drv.c
> > > > +++ b/drivers/staging/hv/vmbus_drv.c
> > > > @@ -43,6 +43,8 @@
> > > >
> > > >  static struct device *root_dev; /* Root device */
> > > >
> > > > +struct pci_dev *hv_pci_dev;
> > > > +
> > > >  /* Main vmbus driver data structure */
> > > >  struct vmbus_driver_context {
> > > >
> > > > @@ -887,36 +889,24 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
> > > >  	}
> > > >  }
> > > >
> > > > -static struct dmi_system_id __initdata microsoft_hv_dmi_table[] = {
> > > > -	{
> > > > -		.ident = "Hyper-V",
> > > > -		.matches = {
> > > > -			DMI_MATCH(DMI_SYS_VENDOR, "Microsoft
> > > Corporation"),
> > > > -			DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
> > > > -			DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
> > > > -		},
> > > > -	},
> > > > -	{ },
> > > > -};
> > > > -MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
> > >
> > > You're sure it's safe to delete this now and just rely on the PCI ids,
> > > right?  For some wierd reason I thought we needed both to catch all
> > > types of systems, but I can't remember why.
> > I have tested this; I don't think we need the dmi table.
> 
> Ok, if you are sure, that's fine with me.
> 
> > > How about "hv_bus" as a name, as that's what this really is.  It's a
> > > "bus adapter", like USB, Firewire, and all sorts of other bus
> > > controllers.
> >
> > Sure; I will make these changes. Would you mind if I submit these name
> changes as a separate patch.
> 
> How about just redo this patch?  I haven't reviewed the others yet, so
> you might want to wait a day to see if I don't like any of them either
> :)

Ok; I will wait for the reviews.

Regards,

K. Y

^ permalink raw reply

* Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
From: Greg KH @ 2011-03-10 22:32 UTC (permalink / raw)
  To: KY Srinivasan
  Cc: linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
	virtualization@lists.osdl.org, Haiyang Zhang, Mike Sterling,
	Abhishek Kane (Mindtree Consulting PVT LTD), Hank Janssen
In-Reply-To: <6E21E5352C11B742B20C142EB499E048018AA3@TK5EX14MBXC128.redmond.corp.microsoft.com>

On Thu, Mar 10, 2011 at 10:28:27PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@suse.de]
> > Sent: Thursday, March 10, 2011 5:21 PM
> > To: KY Srinivasan
> > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> > virtualization@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane
> > (Mindtree Consulting PVT LTD); Hank Janssen
> > Subject: Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
> > 
> > On Thu, Mar 10, 2011 at 02:08:32PM -0800, K. Y. Srinivasan wrote:
> > > Make vmbus driver a platform pci driver. This is
> > > in preparation to cleaning up irq allocation for this
> > > driver.
> > 
> > The idea is nice, but the nameing is a bit confusing.
> > 
> > We have "platform drivers" which are much different from what you are
> > doing here, you are just creating a "normal" pci driver.
> > 
> > Very minor comments below.
> > 
> > >
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > Signed-off-by: Mike Sterling <mike.sterling@microsoft.com>
> > > Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
> > > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> > > ---
> > >  drivers/staging/hv/vmbus_drv.c |   63 +++++++++++++++++++++++-------------
> > ----
> > >  1 files changed, 36 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> > > index 8b9394a..e4855ac 100644
> > > --- a/drivers/staging/hv/vmbus_drv.c
> > > +++ b/drivers/staging/hv/vmbus_drv.c
> > > @@ -43,6 +43,8 @@
> > >
> > >  static struct device *root_dev; /* Root device */
> > >
> > > +struct pci_dev *hv_pci_dev;
> > > +
> > >  /* Main vmbus driver data structure */
> > >  struct vmbus_driver_context {
> > >
> > > @@ -887,36 +889,24 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
> > >  	}
> > >  }
> > >
> > > -static struct dmi_system_id __initdata microsoft_hv_dmi_table[] = {
> > > -	{
> > > -		.ident = "Hyper-V",
> > > -		.matches = {
> > > -			DMI_MATCH(DMI_SYS_VENDOR, "Microsoft
> > Corporation"),
> > > -			DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
> > > -			DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
> > > -		},
> > > -	},
> > > -	{ },
> > > -};
> > > -MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
> > 
> > You're sure it's safe to delete this now and just rely on the PCI ids,
> > right?  For some wierd reason I thought we needed both to catch all
> > types of systems, but I can't remember why.
> I have tested this; I don't think we need the dmi table.

Ok, if you are sure, that's fine with me.

> > How about "hv_bus" as a name, as that's what this really is.  It's a
> > "bus adapter", like USB, Firewire, and all sorts of other bus
> > controllers.
> 
> Sure; I will make these changes. Would you mind if I submit these name changes as a separate patch.

How about just redo this patch?  I haven't reviewed the others yet, so
you might want to wait a day to see if I don't like any of them either
:)

> 
> Regards,
> 
> K. Y

^ permalink raw reply

* RE: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
From: KY Srinivasan @ 2011-03-10 22:28 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
	virtualization@lists.osdl.org, Haiyang Zhang, Mike Sterling,
	Abhishek Kane (Mindtree Consulting PVT LTD), Hank Janssen
In-Reply-To: <20110310222034.GA19323@suse.de>



> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Thursday, March 10, 2011 5:21 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; Haiyang Zhang; Mike Sterling; Abhishek Kane
> (Mindtree Consulting PVT LTD); Hank Janssen
> Subject: Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
> 
> On Thu, Mar 10, 2011 at 02:08:32PM -0800, K. Y. Srinivasan wrote:
> > Make vmbus driver a platform pci driver. This is
> > in preparation to cleaning up irq allocation for this
> > driver.
> 
> The idea is nice, but the nameing is a bit confusing.
> 
> We have "platform drivers" which are much different from what you are
> doing here, you are just creating a "normal" pci driver.
> 
> Very minor comments below.
> 
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > Signed-off-by: Mike Sterling <mike.sterling@microsoft.com>
> > Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
> > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> > ---
> >  drivers/staging/hv/vmbus_drv.c |   63 +++++++++++++++++++++++-------------
> ----
> >  1 files changed, 36 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> > index 8b9394a..e4855ac 100644
> > --- a/drivers/staging/hv/vmbus_drv.c
> > +++ b/drivers/staging/hv/vmbus_drv.c
> > @@ -43,6 +43,8 @@
> >
> >  static struct device *root_dev; /* Root device */
> >
> > +struct pci_dev *hv_pci_dev;
> > +
> >  /* Main vmbus driver data structure */
> >  struct vmbus_driver_context {
> >
> > @@ -887,36 +889,24 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
> >  	}
> >  }
> >
> > -static struct dmi_system_id __initdata microsoft_hv_dmi_table[] = {
> > -	{
> > -		.ident = "Hyper-V",
> > -		.matches = {
> > -			DMI_MATCH(DMI_SYS_VENDOR, "Microsoft
> Corporation"),
> > -			DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
> > -			DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
> > -		},
> > -	},
> > -	{ },
> > -};
> > -MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
> 
> You're sure it's safe to delete this now and just rely on the PCI ids,
> right?  For some wierd reason I thought we needed both to catch all
> types of systems, but I can't remember why.
I have tested this; I don't think we need the dmi table.

> 
> >
> > -static int __init vmbus_init(void)
> > +
> > +static int __devinit hv_pci_probe(struct pci_dev *pdev,
> > +				const struct pci_device_id *ent)
> >  {
> > -	DPRINT_INFO(VMBUS_DRV,
> > -		"Vmbus initializing.... current log level 0x%x (%x,%x)",
> > -		vmbus_loglevel, HIWORD(vmbus_loglevel),
> LOWORD(vmbus_loglevel));
> > -	/* Todo: it is used for loglevel, to be ported to new kernel. */
> > +	int err;
> >
> > -	if (!dmi_check_system(microsoft_hv_dmi_table))
> > -		return -ENODEV;
> > +	hv_pci_dev = pdev;
> >
> > -	return vmbus_bus_init();
> > -}
> > +	err = pci_enable_device(pdev);
> > +	if (err)
> > +		return err;
> >
> > -static void __exit vmbus_exit(void)
> > -{
> > -	vmbus_bus_exit();
> > -	/* Todo: it is used for loglevel, to be ported to new kernel. */
> > +	err = vmbus_bus_init();
> > +	if (err)
> > +		pci_disable_device(pdev);
> > +
> > +	return err;
> >  }
> >
> >  /*
> > @@ -931,10 +921,29 @@ static const struct pci_device_id
> microsoft_hv_pci_table[] = {
> >  };
> >  MODULE_DEVICE_TABLE(pci, microsoft_hv_pci_table);
> >
> > +static struct pci_driver platform_driver = {
> 
> "hv_bus_driver"?
> 
> > +	.name =           "hv-platform-pci",
> 
> How about "hv_bus" as a name, as that's what this really is.  It's a
> "bus adapter", like USB, Firewire, and all sorts of other bus
> controllers.

Sure; I will make these changes. Would you mind if I submit these name changes as a separate patch.

Regards,

K. Y

^ permalink raw reply

* Re: [PATCH 11/21] Staging: hv: Make vmbus driver a platform pci driver
From: Greg KH @ 2011-03-10 22:20 UTC (permalink / raw)
  To: K. Y. Srinivasan
  Cc: linux-kernel, devel, virtualization, Haiyang Zhang, Mike Sterling,
	Abhishek Kane, Hank Janssen
In-Reply-To: <1299794912-1233-1-git-send-email-kys@microsoft.com>

On Thu, Mar 10, 2011 at 02:08:32PM -0800, K. Y. Srinivasan wrote:
> Make vmbus driver a platform pci driver. This is
> in preparation to cleaning up irq allocation for this
> driver.

The idea is nice, but the nameing is a bit confusing.

We have "platform drivers" which are much different from what you are
doing here, you are just creating a "normal" pci driver.

Very minor comments below.

> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: Mike Sterling <mike.sterling@microsoft.com>
> Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
> Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> ---
>  drivers/staging/hv/vmbus_drv.c |   63 +++++++++++++++++++++++-----------------
>  1 files changed, 36 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> index 8b9394a..e4855ac 100644
> --- a/drivers/staging/hv/vmbus_drv.c
> +++ b/drivers/staging/hv/vmbus_drv.c
> @@ -43,6 +43,8 @@
>  
>  static struct device *root_dev; /* Root device */
>  
> +struct pci_dev *hv_pci_dev;
> +
>  /* Main vmbus driver data structure */
>  struct vmbus_driver_context {
>  
> @@ -887,36 +889,24 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
>  	}
>  }
>  
> -static struct dmi_system_id __initdata microsoft_hv_dmi_table[] = {
> -	{
> -		.ident = "Hyper-V",
> -		.matches = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> -			DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
> -			DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
> -		},
> -	},
> -	{ },
> -};
> -MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);

You're sure it's safe to delete this now and just rely on the PCI ids,
right?  For some wierd reason I thought we needed both to catch all
types of systems, but I can't remember why.

>  
> -static int __init vmbus_init(void)
> +
> +static int __devinit hv_pci_probe(struct pci_dev *pdev,
> +				const struct pci_device_id *ent)
>  {
> -	DPRINT_INFO(VMBUS_DRV,
> -		"Vmbus initializing.... current log level 0x%x (%x,%x)",
> -		vmbus_loglevel, HIWORD(vmbus_loglevel), LOWORD(vmbus_loglevel));
> -	/* Todo: it is used for loglevel, to be ported to new kernel. */
> +	int err;
>  
> -	if (!dmi_check_system(microsoft_hv_dmi_table))
> -		return -ENODEV;
> +	hv_pci_dev = pdev;
>  
> -	return vmbus_bus_init();
> -}
> +	err = pci_enable_device(pdev);
> +	if (err)
> +		return err;
>  
> -static void __exit vmbus_exit(void)
> -{
> -	vmbus_bus_exit();
> -	/* Todo: it is used for loglevel, to be ported to new kernel. */
> +	err = vmbus_bus_init();
> +	if (err)
> +		pci_disable_device(pdev);
> +
> +	return err;
>  }
>  
>  /*
> @@ -931,10 +921,29 @@ static const struct pci_device_id microsoft_hv_pci_table[] = {
>  };
>  MODULE_DEVICE_TABLE(pci, microsoft_hv_pci_table);
>  
> +static struct pci_driver platform_driver = {

"hv_bus_driver"?

> +	.name =           "hv-platform-pci",

How about "hv_bus" as a name, as that's what this really is.  It's a
"bus adapter", like USB, Firewire, and all sorts of other bus
controllers.

> +	.probe =          hv_pci_probe,
> +	.id_table =       microsoft_hv_pci_table,
> +};
> +
> +static int __init hv_pci_init(void)
> +{
> +	return pci_register_driver(&platform_driver);
> +}
> +
> +static void __exit hv_pci_exit(void)
> +{
> +	vmbus_bus_exit();
> +	pci_unregister_driver(&platform_driver);
> +}
> +
> +
> +
>  MODULE_LICENSE("GPL");
>  MODULE_VERSION(HV_DRV_VERSION);
>  module_param(vmbus_irq, int, S_IRUGO);
>  module_param(vmbus_loglevel, int, S_IRUGO);
>  
> -module_init(vmbus_init);
> -module_exit(vmbus_exit);
> +module_init(hv_pci_init);
> +module_exit(hv_pci_exit);
> -- 
> 1.5.5.6

^ permalink raw reply

* [PATCH 21/21] Staging: hv: Get rid of the forward declaration for vmbus_show_device_attr
From: K. Y. Srinivasan @ 2011-03-10 22:16 UTC (permalink / raw)
  To: kys, gregkh, linux-kernel, devel, virtualization
  Cc: Haiyang Zhang, Mike Sterling, Abhishek Kane, Hank Janssen
In-Reply-To: <[PATCH 00/21] Staging: hv: Cleanup vmbus driver>

Get rid of the forward declaration of vmbus_show_device_attr by moving
the code around.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Mike Sterling <mike.sterling@microsoft.com>
Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
 drivers/staging/hv/vmbus_drv.c |  302 +++++++++++++++++++--------------------
 1 files changed, 147 insertions(+), 155 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index dfa3c66..881c3c0 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -50,18 +50,160 @@ struct hv_bus {
 	struct tasklet_struct event_dpc;
 };
 
+unsigned int vmbus_loglevel = (ALL_MODULES << 16 | INFO_LVL);
+EXPORT_SYMBOL(vmbus_loglevel);
+	/* (ALL_MODULES << 16 | DEBUG_LVL_ENTEREXIT); */
+	/* (((VMBUS | VMBUS_DRV)<<16) | DEBUG_LVL_ENTEREXIT); */
+
+
+static void get_channel_info(struct hv_device *device,
+			     struct hv_device_info *info)
+{
+	struct vmbus_channel_debug_info debug_info;
+
+	if (!device->channel)
+		return;
+
+	vmbus_get_debug_info(device->channel, &debug_info);
+
+	info->chn_id = debug_info.relid;
+	info->chn_state = debug_info.state;
+	memcpy(&info->chn_type, &debug_info.interfacetype,
+	       sizeof(struct hv_guid));
+	memcpy(&info->chn_instance, &debug_info.interface_instance,
+	       sizeof(struct hv_guid));
+
+	info->monitor_id = debug_info.monitorid;
+
+	info->server_monitor_pending = debug_info.servermonitor_pending;
+	info->server_monitor_latency = debug_info.servermonitor_latency;
+	info->server_monitor_conn_id = debug_info.servermonitor_connectionid;
+
+	info->client_monitor_pending = debug_info.clientmonitor_pending;
+	info->client_monitor_latency = debug_info.clientmonitor_latency;
+	info->client_monitor_conn_id = debug_info.clientmonitor_connectionid;
+
+	info->inbound.int_mask = debug_info.inbound.current_interrupt_mask;
+	info->inbound.read_idx = debug_info.inbound.current_read_index;
+	info->inbound.write_idx = debug_info.inbound.current_write_index;
+	info->inbound.bytes_avail_toread =
+		debug_info.inbound.bytes_avail_toread;
+	info->inbound.bytes_avail_towrite =
+		debug_info.inbound.bytes_avail_towrite;
 
+	info->outbound.int_mask =
+		debug_info.outbound.current_interrupt_mask;
+	info->outbound.read_idx = debug_info.outbound.current_read_index;
+	info->outbound.write_idx = debug_info.outbound.current_write_index;
+	info->outbound.bytes_avail_toread =
+		debug_info.outbound.bytes_avail_toread;
+	info->outbound.bytes_avail_towrite =
+		debug_info.outbound.bytes_avail_towrite;
+}
 
+/*
+ * vmbus_show_device_attr - Show the device attribute in sysfs.
+ *
+ * This is invoked when user does a
+ * "cat /sys/bus/vmbus/devices/<busdevice>/<attr name>"
+ */
 static ssize_t vmbus_show_device_attr(struct device *dev,
 				      struct device_attribute *dev_attr,
-				      char *buf);
+				      char *buf)
+{
+	struct hv_device *device_ctx = device_to_hv_device(dev);
+	struct hv_device_info device_info;
 
+	memset(&device_info, 0, sizeof(struct hv_device_info));
 
-unsigned int vmbus_loglevel = (ALL_MODULES << 16 | INFO_LVL);
-EXPORT_SYMBOL(vmbus_loglevel);
-	/* (ALL_MODULES << 16 | DEBUG_LVL_ENTEREXIT); */
-	/* (((VMBUS | VMBUS_DRV)<<16) | DEBUG_LVL_ENTEREXIT); */
+	get_channel_info(device_ctx, &device_info);
 
+	if (!strcmp(dev_attr->attr.name, "class_id")) {
+		return sprintf(buf, "{%02x%02x%02x%02x-%02x%02x-%02x%02x-"
+			       "%02x%02x%02x%02x%02x%02x%02x%02x}\n",
+			       device_info.chn_type.data[3],
+			       device_info.chn_type.data[2],
+			       device_info.chn_type.data[1],
+			       device_info.chn_type.data[0],
+			       device_info.chn_type.data[5],
+			       device_info.chn_type.data[4],
+			       device_info.chn_type.data[7],
+			       device_info.chn_type.data[6],
+			       device_info.chn_type.data[8],
+			       device_info.chn_type.data[9],
+			       device_info.chn_type.data[10],
+			       device_info.chn_type.data[11],
+			       device_info.chn_type.data[12],
+			       device_info.chn_type.data[13],
+			       device_info.chn_type.data[14],
+			       device_info.chn_type.data[15]);
+	} else if (!strcmp(dev_attr->attr.name, "device_id")) {
+		return sprintf(buf, "{%02x%02x%02x%02x-%02x%02x-%02x%02x-"
+			       "%02x%02x%02x%02x%02x%02x%02x%02x}\n",
+			       device_info.chn_instance.data[3],
+			       device_info.chn_instance.data[2],
+			       device_info.chn_instance.data[1],
+			       device_info.chn_instance.data[0],
+			       device_info.chn_instance.data[5],
+			       device_info.chn_instance.data[4],
+			       device_info.chn_instance.data[7],
+			       device_info.chn_instance.data[6],
+			       device_info.chn_instance.data[8],
+			       device_info.chn_instance.data[9],
+			       device_info.chn_instance.data[10],
+			       device_info.chn_instance.data[11],
+			       device_info.chn_instance.data[12],
+			       device_info.chn_instance.data[13],
+			       device_info.chn_instance.data[14],
+			       device_info.chn_instance.data[15]);
+	} else if (!strcmp(dev_attr->attr.name, "state")) {
+		return sprintf(buf, "%d\n", device_info.chn_state);
+	} else if (!strcmp(dev_attr->attr.name, "id")) {
+		return sprintf(buf, "%d\n", device_info.chn_id);
+	} else if (!strcmp(dev_attr->attr.name, "out_intr_mask")) {
+		return sprintf(buf, "%d\n", device_info.outbound.int_mask);
+	} else if (!strcmp(dev_attr->attr.name, "out_read_index")) {
+		return sprintf(buf, "%d\n", device_info.outbound.read_idx);
+	} else if (!strcmp(dev_attr->attr.name, "out_write_index")) {
+		return sprintf(buf, "%d\n", device_info.outbound.write_idx);
+	} else if (!strcmp(dev_attr->attr.name, "out_read_bytes_avail")) {
+		return sprintf(buf, "%d\n",
+			       device_info.outbound.bytes_avail_toread);
+	} else if (!strcmp(dev_attr->attr.name, "out_write_bytes_avail")) {
+		return sprintf(buf, "%d\n",
+			       device_info.outbound.bytes_avail_towrite);
+	} else if (!strcmp(dev_attr->attr.name, "in_intr_mask")) {
+		return sprintf(buf, "%d\n", device_info.inbound.int_mask);
+	} else if (!strcmp(dev_attr->attr.name, "in_read_index")) {
+		return sprintf(buf, "%d\n", device_info.inbound.read_idx);
+	} else if (!strcmp(dev_attr->attr.name, "in_write_index")) {
+		return sprintf(buf, "%d\n", device_info.inbound.write_idx);
+	} else if (!strcmp(dev_attr->attr.name, "in_read_bytes_avail")) {
+		return sprintf(buf, "%d\n",
+			       device_info.inbound.bytes_avail_toread);
+	} else if (!strcmp(dev_attr->attr.name, "in_write_bytes_avail")) {
+		return sprintf(buf, "%d\n",
+			       device_info.inbound.bytes_avail_towrite);
+	} else if (!strcmp(dev_attr->attr.name, "monitor_id")) {
+		return sprintf(buf, "%d\n", device_info.monitor_id);
+	} else if (!strcmp(dev_attr->attr.name, "server_monitor_pending")) {
+		return sprintf(buf, "%d\n", device_info.server_monitor_pending);
+	} else if (!strcmp(dev_attr->attr.name, "server_monitor_latency")) {
+		return sprintf(buf, "%d\n", device_info.server_monitor_latency);
+	} else if (!strcmp(dev_attr->attr.name, "server_monitor_conn_id")) {
+		return sprintf(buf, "%d\n",
+			       device_info.server_monitor_conn_id);
+	} else if (!strcmp(dev_attr->attr.name, "client_monitor_pending")) {
+		return sprintf(buf, "%d\n", device_info.client_monitor_pending);
+	} else if (!strcmp(dev_attr->attr.name, "client_monitor_latency")) {
+		return sprintf(buf, "%d\n", device_info.client_monitor_latency);
+	} else if (!strcmp(dev_attr->attr.name, "client_monitor_conn_id")) {
+		return sprintf(buf, "%d\n",
+			       device_info.client_monitor_conn_id);
+	} else {
+		return 0;
+	}
+}
 
 /* Set up per device attributes in /sys/bus/vmbus/devices/<bus device> */
 static struct device_attribute vmbus_device_attrs[] = {
@@ -441,156 +583,6 @@ static irqreturn_t vmbus_isr(int irq, void *dev_id)
 	}
 }
 
-
-static void get_channel_info(struct hv_device *device,
-			     struct hv_device_info *info)
-{
-	struct vmbus_channel_debug_info debug_info;
-
-	if (!device->channel)
-		return;
-
-	vmbus_get_debug_info(device->channel, &debug_info);
-
-	info->chn_id = debug_info.relid;
-	info->chn_state = debug_info.state;
-	memcpy(&info->chn_type, &debug_info.interfacetype,
-	       sizeof(struct hv_guid));
-	memcpy(&info->chn_instance, &debug_info.interface_instance,
-	       sizeof(struct hv_guid));
-
-	info->monitor_id = debug_info.monitorid;
-
-	info->server_monitor_pending = debug_info.servermonitor_pending;
-	info->server_monitor_latency = debug_info.servermonitor_latency;
-	info->server_monitor_conn_id = debug_info.servermonitor_connectionid;
-
-	info->client_monitor_pending = debug_info.clientmonitor_pending;
-	info->client_monitor_latency = debug_info.clientmonitor_latency;
-	info->client_monitor_conn_id = debug_info.clientmonitor_connectionid;
-
-	info->inbound.int_mask = debug_info.inbound.current_interrupt_mask;
-	info->inbound.read_idx = debug_info.inbound.current_read_index;
-	info->inbound.write_idx = debug_info.inbound.current_write_index;
-	info->inbound.bytes_avail_toread =
-		debug_info.inbound.bytes_avail_toread;
-	info->inbound.bytes_avail_towrite =
-		debug_info.inbound.bytes_avail_towrite;
-
-	info->outbound.int_mask =
-		debug_info.outbound.current_interrupt_mask;
-	info->outbound.read_idx = debug_info.outbound.current_read_index;
-	info->outbound.write_idx = debug_info.outbound.current_write_index;
-	info->outbound.bytes_avail_toread =
-		debug_info.outbound.bytes_avail_toread;
-	info->outbound.bytes_avail_towrite =
-		debug_info.outbound.bytes_avail_towrite;
-}
-
-/*
- * vmbus_show_device_attr - Show the device attribute in sysfs.
- *
- * This is invoked when user does a
- * "cat /sys/bus/vmbus/devices/<busdevice>/<attr name>"
- */
-static ssize_t vmbus_show_device_attr(struct device *dev,
-				      struct device_attribute *dev_attr,
-				      char *buf)
-{
-	struct hv_device *device_ctx = device_to_hv_device(dev);
-	struct hv_device_info device_info;
-
-	memset(&device_info, 0, sizeof(struct hv_device_info));
-
-	get_channel_info(device_ctx, &device_info);
-
-	if (!strcmp(dev_attr->attr.name, "class_id")) {
-		return sprintf(buf, "{%02x%02x%02x%02x-%02x%02x-%02x%02x-"
-			       "%02x%02x%02x%02x%02x%02x%02x%02x}\n",
-			       device_info.chn_type.data[3],
-			       device_info.chn_type.data[2],
-			       device_info.chn_type.data[1],
-			       device_info.chn_type.data[0],
-			       device_info.chn_type.data[5],
-			       device_info.chn_type.data[4],
-			       device_info.chn_type.data[7],
-			       device_info.chn_type.data[6],
-			       device_info.chn_type.data[8],
-			       device_info.chn_type.data[9],
-			       device_info.chn_type.data[10],
-			       device_info.chn_type.data[11],
-			       device_info.chn_type.data[12],
-			       device_info.chn_type.data[13],
-			       device_info.chn_type.data[14],
-			       device_info.chn_type.data[15]);
-	} else if (!strcmp(dev_attr->attr.name, "device_id")) {
-		return sprintf(buf, "{%02x%02x%02x%02x-%02x%02x-%02x%02x-"
-			       "%02x%02x%02x%02x%02x%02x%02x%02x}\n",
-			       device_info.chn_instance.data[3],
-			       device_info.chn_instance.data[2],
-			       device_info.chn_instance.data[1],
-			       device_info.chn_instance.data[0],
-			       device_info.chn_instance.data[5],
-			       device_info.chn_instance.data[4],
-			       device_info.chn_instance.data[7],
-			       device_info.chn_instance.data[6],
-			       device_info.chn_instance.data[8],
-			       device_info.chn_instance.data[9],
-			       device_info.chn_instance.data[10],
-			       device_info.chn_instance.data[11],
-			       device_info.chn_instance.data[12],
-			       device_info.chn_instance.data[13],
-			       device_info.chn_instance.data[14],
-			       device_info.chn_instance.data[15]);
-	} else if (!strcmp(dev_attr->attr.name, "state")) {
-		return sprintf(buf, "%d\n", device_info.chn_state);
-	} else if (!strcmp(dev_attr->attr.name, "id")) {
-		return sprintf(buf, "%d\n", device_info.chn_id);
-	} else if (!strcmp(dev_attr->attr.name, "out_intr_mask")) {
-		return sprintf(buf, "%d\n", device_info.outbound.int_mask);
-	} else if (!strcmp(dev_attr->attr.name, "out_read_index")) {
-		return sprintf(buf, "%d\n", device_info.outbound.read_idx);
-	} else if (!strcmp(dev_attr->attr.name, "out_write_index")) {
-		return sprintf(buf, "%d\n", device_info.outbound.write_idx);
-	} else if (!strcmp(dev_attr->attr.name, "out_read_bytes_avail")) {
-		return sprintf(buf, "%d\n",
-			       device_info.outbound.bytes_avail_toread);
-	} else if (!strcmp(dev_attr->attr.name, "out_write_bytes_avail")) {
-		return sprintf(buf, "%d\n",
-			       device_info.outbound.bytes_avail_towrite);
-	} else if (!strcmp(dev_attr->attr.name, "in_intr_mask")) {
-		return sprintf(buf, "%d\n", device_info.inbound.int_mask);
-	} else if (!strcmp(dev_attr->attr.name, "in_read_index")) {
-		return sprintf(buf, "%d\n", device_info.inbound.read_idx);
-	} else if (!strcmp(dev_attr->attr.name, "in_write_index")) {
-		return sprintf(buf, "%d\n", device_info.inbound.write_idx);
-	} else if (!strcmp(dev_attr->attr.name, "in_read_bytes_avail")) {
-		return sprintf(buf, "%d\n",
-			       device_info.inbound.bytes_avail_toread);
-	} else if (!strcmp(dev_attr->attr.name, "in_write_bytes_avail")) {
-		return sprintf(buf, "%d\n",
-			       device_info.inbound.bytes_avail_towrite);
-	} else if (!strcmp(dev_attr->attr.name, "monitor_id")) {
-		return sprintf(buf, "%d\n", device_info.monitor_id);
-	} else if (!strcmp(dev_attr->attr.name, "server_monitor_pending")) {
-		return sprintf(buf, "%d\n", device_info.server_monitor_pending);
-	} else if (!strcmp(dev_attr->attr.name, "server_monitor_latency")) {
-		return sprintf(buf, "%d\n", device_info.server_monitor_latency);
-	} else if (!strcmp(dev_attr->attr.name, "server_monitor_conn_id")) {
-		return sprintf(buf, "%d\n",
-			       device_info.server_monitor_conn_id);
-	} else if (!strcmp(dev_attr->attr.name, "client_monitor_pending")) {
-		return sprintf(buf, "%d\n", device_info.client_monitor_pending);
-	} else if (!strcmp(dev_attr->attr.name, "client_monitor_latency")) {
-		return sprintf(buf, "%d\n", device_info.client_monitor_latency);
-	} else if (!strcmp(dev_attr->attr.name, "client_monitor_conn_id")) {
-		return sprintf(buf, "%d\n",
-			       device_info.client_monitor_conn_id);
-	} else {
-		return 0;
-	}
-}
-
 /*
  * vmbus_bus_init -Main vmbus driver initialization routine.
  *
-- 
1.5.5.6

^ permalink raw reply related

* [PATCH 20/21] Staging: hv: Get rid of the forward declaration for vmbus_isr
From: K. Y. Srinivasan @ 2011-03-10 22:15 UTC (permalink / raw)
  To: kys, gregkh, linux-kernel, devel, virtualization
  Cc: Haiyang Zhang, Mike Sterling, Abhishek Kane, Hank Janssen
In-Reply-To: <[PATCH 00/21] Staging: hv: Cleanup vmbus driver>

Get rid of the forward declaration of vmbus_isr by moving
the code around.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Mike Sterling <mike.sterling@microsoft.com>
Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
 drivers/staging/hv/vmbus_drv.c |   47 +++++++++++++++++++--------------------
 1 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 7c16acd..dfa3c66 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -51,7 +51,6 @@ struct hv_bus {
 };
 
 
-static irqreturn_t vmbus_isr(int irq, void *dev_id);
 
 static ssize_t vmbus_show_device_attr(struct device *dev,
 				      struct device_attribute *dev_attr,
@@ -94,6 +93,7 @@ static struct device_attribute vmbus_device_attrs[] = {
 	__ATTR_NULL
 };
 
+
 /*
  * vmbus_uevent - add uevent for our device
  *
@@ -420,6 +420,28 @@ static int vmbus_on_isr(void)
 	return ret;
 }
 
+
+static irqreturn_t vmbus_isr(int irq, void *dev_id)
+{
+	int ret;
+
+	ret = vmbus_on_isr();
+
+	/* Schedules a dpc if necessary */
+	if (ret > 0) {
+		if (test_bit(0, (unsigned long *)&ret))
+			tasklet_schedule(&hv_bus.msg_dpc);
+
+		if (test_bit(1, (unsigned long *)&ret))
+			tasklet_schedule(&hv_bus.event_dpc);
+
+		return IRQ_HANDLED;
+	} else {
+		return IRQ_NONE;
+	}
+}
+
+
 static void get_channel_info(struct hv_device *device,
 			     struct hv_device_info *info)
 {
@@ -858,29 +880,6 @@ void vmbus_child_device_unregister(struct hv_device *device_obj)
 }
 
 
-
-static irqreturn_t vmbus_isr(int irq, void *dev_id)
-{
-	int ret;
-
-	ret = vmbus_on_isr();
-
-	/* Schedules a dpc if necessary */
-	if (ret > 0) {
-		if (test_bit(0, (unsigned long *)&ret))
-			tasklet_schedule(&hv_bus.msg_dpc);
-
-		if (test_bit(1, (unsigned long *)&ret))
-			tasklet_schedule(&hv_bus.event_dpc);
-
-		return IRQ_HANDLED;
-	} else {
-		return IRQ_NONE;
-	}
-}
-
-
-
 static int __devinit hv_pci_probe(struct pci_dev *pdev,
 				const struct pci_device_id *ent)
 {
-- 
1.5.5.6

^ permalink raw reply related

* [PATCH 19/21] Staging: hv: Get rid of the forward declaration for vmbus_device_release
From: K. Y. Srinivasan @ 2011-03-10 22:15 UTC (permalink / raw)
  To: kys, gregkh, linux-kernel, devel, virtualization
  Cc: Haiyang Zhang, Mike Sterling, Abhishek Kane, Hank Janssen
In-Reply-To: <[PATCH 00/21] Staging: hv: Cleanup vmbus driver>

Get rid of the forward declaration of vmbus_device_release by moving
the code around.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Mike Sterling <mike.sterling@microsoft.com>
Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
 drivers/staging/hv/vmbus_drv.c |   27 ++++++++++++---------------
 1 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 6f4094b..7c16acd 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -53,8 +53,6 @@ struct hv_bus {
 
 static irqreturn_t vmbus_isr(int irq, void *dev_id);
 
-static void vmbus_device_release(struct device *device);
-
 static ssize_t vmbus_show_device_attr(struct device *dev,
 				      struct device_attribute *dev_attr,
 				      char *buf);
@@ -300,6 +298,18 @@ static void vmbus_shutdown(struct device *child_device)
 	return;
 }
 
+
+/*
+ * vmbus_device_release - Final callback release of the vmbus child device
+ */
+static void vmbus_device_release(struct device *device)
+{
+	struct hv_device *device_ctx = device_to_hv_device(device);
+
+	kfree(device_ctx);
+
+}
+
 /* The one and only one */
 static struct hv_bus  hv_bus = {
 	.bus.name =		"vmbus",
@@ -848,19 +858,6 @@ void vmbus_child_device_unregister(struct hv_device *device_obj)
 }
 
 
-/*
- * vmbus_device_release - Final callback release of the vmbus child device
- */
-static void vmbus_device_release(struct device *device)
-{
-	struct hv_device *device_ctx = device_to_hv_device(device);
-
-	kfree(device_ctx);
-
-	/* !!DO NOT REFERENCE device_ctx anymore at this point!! */
-}
-
-
 
 static irqreturn_t vmbus_isr(int irq, void *dev_id)
 {
-- 
1.5.5.6

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox