* Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X
From: Michael S. Tsirkin @ 2011-08-20 20:00 UTC (permalink / raw)
To: Sasha Levin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <1313771587.12243.16.camel@lappy>
On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote:
> On Fri, 2011-08-19 at 18:23 +0300, Michael S. Tsirkin wrote:
> > On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote:
> > > The MAC of a virtio-net device is located at the first field of the device
> > > specific header. This header is located at offset 20 if the device doesn't
> > > support MSI-X or offset 24 if it does.
> > >
> > > Current code in virtnet_probe() used to probe the MAC before checking for
> > > MSI-X, which means that the read was always made from offset 20 regardless
> > > of whether MSI-X in enabled or not.
> > >
> > > This patch moves the MAC probe to after the detection of whether MSI-X is
> > > enabled. This way the MAC will be read from offset 24 if the device indeed
> > > supports MSI-X.
> > >
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: virtualization@lists.linux-foundation.org
> > > Cc: netdev@vger.kernel.org
> > > Cc: kvm@vger.kernel.org
> > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> >
> > I am not sure I see a bug in virtio: the config pace layout simply
> > changes as msix is enabled and disabled (and if you look at the latest
> > draft, also on whether 64 bit features are enabled).
> > It doesn't depend on msix capability being present in device.
> >
> > The spec seems to be explicit enough:
> > If MSI-X is enabled for the device, two additional fields immediately
> > follow this header.
> >
> > So I'm guessing the bug is in kvm tools which assume
> > same layout for when msix is enabled and disabled.
> > qemu-kvm seems to do the right thing so the device
> > seems to get the correct mac.
>
> We assumed that PCI config space has a static layout like most other
> devices. Having a behavior of "First bit 20 does something, but after
> enabling MSI-X it does something completely different" sounds strange.
The layout is always virtio header followed by device specific header.
We started with a small header so when more data was added, we could not
extend the header unconditionally.
We can't change that behaviour for MSI-X now, guests and
hosts rely on it.
>
> I'm wondering why offsets of the config structure change during run time
> and are not statically defined when the device is started.
That's because of backwards compatibility with old guests.
When we know the guest is new, we expose new layout,
but old guests must see old layout.
> It's not like VIRTIO_F_FEATURES_HI can be disabled after it was enabled,
Yes it can, e.g. at guest reset. Generally features can be tweaked
any way guest likes until status is set to OK.
> or MSI-X can be simply disabled during run time.
Not sure what you mean by 'run time'. Guest can reset
or disable the device, change any parameters,
then re-enable.
> Maybe this is better solved by copying the way it was done in PCI itself
> with capability linked list?
>
> --
>
> Sasha.
There are any number of ways to lay out the structure. I went for what
seemed a simplest one. For MSI-X the train has left the station. We
can probably still tweak where the high 32 bit features
for 64 bit features are. No idea if it's worth it.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio-blk: Add stats VQ to collect information about devices
From: Anthony Liguori @ 2011-08-21 23:31 UTC (permalink / raw)
To: Sasha Levin
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization,
Pekka Enberg, Avi Kivity, Ingo Molnar
In-Reply-To: <1313684984.30799.20.camel@lappy>
On 08/18/2011 11:29 AM, Sasha Levin wrote:
> On Thu, 2011-08-18 at 08:10 -0700, Avi Kivity wrote:
>> On 08/17/2011 09:38 PM, Sasha Levin wrote:
>>> On Wed, 2011-08-17 at 16:00 -0700, Avi Kivity wrote:
>>>> On 08/16/2011 12:47 PM, Sasha Levin wrote:
>>>> > This patch adds support for an optional stats vq that works similary to the
>>>> > stats vq provided by virtio-balloon.
>>>> >
>>>> > The purpose of this change is to allow collection of statistics about working
>>>> > virtio-blk devices to easily analyze performance without having to tap into
>>>> > the guest.
>>>> >
>>>> >
>>>>
>>>> Why can't you get the same info from the host? i.e. read sectors?
>>>
>>> Some of the stats you can collect from the host, but some you can't.
>>>
>>> The ones you can't include all the timing statistics and the internal
>>> queue statistics (read/write merges).
>>
>> Surely you can time the actual amount of time the I/O takes? It doesn't
>> account for the virtio round-trip, but does it matter?
>>
>> Why is the merge count important for the host?
>>
>
> I assumed that the time the request spends in the virtio layer is
> (somewhat) significant, specially since that this is something that adds
> up over time.
>
> Merge count can be useful for several testing scenarios (I'll describe
> the reasoning behind this patch below).
>
>>>
>>> The idea behind providing all of the stats on the stats vq (which is
>>> basically what you see in '/dev/block/[device]/stats') is to give a
>>> consistent snapshot of the state of the device.
>>>
>>>
>>
>> What can you do with it?
>>
>
> I was actually planning on submitting another patch that would add
> something similar into virtio-net. My plan was to enable collecting
> statistics regarding memory, network and disk usage in a simple manner
> without accessing guests.
Why not just add an interface that lets you read files from a guest
either via a guest agent (like qemu-ga) or a purpose built PV device?
That would let you access the guest's full sysfs which seems to be quite
a lot more useful long term than adding a bunch of specific interfaces.
Regards,
Anthony Liguori
^ permalink raw reply
* Re: [PULL] lguest fix, virtio documentation
From: Rusty Russell @ 2011-08-22 0:12 UTC (permalink / raw)
To: Sasha Levin; +Cc: Linus Torvalds, lkml - Kernel Mailing List, virtualization
In-Reply-To: <1313419712.3004.1.camel@sasha>
On Mon, 15 Aug 2011 17:48:32 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> On Mon, 2011-08-15 at 10:25 +0930, Rusty Russell wrote:
> > [ This adds a text copy of the virtio draft spec to the kernel source,
> > which should make it easier for people to find and read. The master
> > is still the lyx document on my server, so I'll keep it in sync. ]
> >
> > The following changes since commit 91d85ea6786107aa2837bef3e957165ad7c8b823:
> >
> > Merge branch 'hwmon-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/staging (2011-08-13 18:37:28 -0700)
> >
> > are available in the git repository at:
> >
> > ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-for-linus.git master
> >
> > Rusty Russell (2):
> > virtio: Add text copy of spec to Documentation/virtual.
>
> The "Device Status" paragraph says FAILED is 8, while it should be 0x80.
They're 1-based bit numbers. But since everyone makes this mistake,
I've changed them *all* to values rather than bit numbers.
Thanks!
Rusty.
^ permalink raw reply
* Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X
From: Rusty Russell @ 2011-08-22 0:24 UTC (permalink / raw)
To: Michael S. Tsirkin, Sasha Levin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20110819152335.GA19489@redhat.com>
On Fri, 19 Aug 2011 18:23:35 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote:
> > The MAC of a virtio-net device is located at the first field of the device
> > specific header. This header is located at offset 20 if the device doesn't
> > support MSI-X or offset 24 if it does.
> >
> > Current code in virtnet_probe() used to probe the MAC before checking for
> > MSI-X, which means that the read was always made from offset 20 regardless
> > of whether MSI-X in enabled or not.
> >
> > This patch moves the MAC probe to after the detection of whether MSI-X is
> > enabled. This way the MAC will be read from offset 24 if the device indeed
> > supports MSI-X.
> >
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: virtualization@lists.linux-foundation.org
> > Cc: netdev@vger.kernel.org
> > Cc: kvm@vger.kernel.org
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
>
> I am not sure I see a bug in virtio: the config pace layout simply
> changes as msix is enabled and disabled (and if you look at the latest
> draft, also on whether 64 bit features are enabled).
> It doesn't depend on msix capability being present in device.
>
> The spec seems to be explicit enough:
> If MSI-X is enabled for the device, two additional fields immediately
> follow this header.
>
> So I'm guessing the bug is in kvm tools which assume
> same layout for when msix is enabled and disabled.
> qemu-kvm seems to do the right thing so the device
> seems to get the correct mac.
So, the config space moves once MSI-X is enabled? In which case, it
should say "ONCE MSI-X is enabled..."
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X
From: Michael S. Tsirkin @ 2011-08-22 8:36 UTC (permalink / raw)
To: Rusty Russell; +Cc: netdev, virtualization, Sasha Levin, kvm, linux-kernel
In-Reply-To: <87hb5a5m3x.fsf@rustcorp.com.au>
On Mon, Aug 22, 2011 at 09:54:50AM +0930, Rusty Russell wrote:
> On Fri, 19 Aug 2011 18:23:35 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote:
> > > The MAC of a virtio-net device is located at the first field of the device
> > > specific header. This header is located at offset 20 if the device doesn't
> > > support MSI-X or offset 24 if it does.
> > >
> > > Current code in virtnet_probe() used to probe the MAC before checking for
> > > MSI-X, which means that the read was always made from offset 20 regardless
> > > of whether MSI-X in enabled or not.
> > >
> > > This patch moves the MAC probe to after the detection of whether MSI-X is
> > > enabled. This way the MAC will be read from offset 24 if the device indeed
> > > supports MSI-X.
> > >
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: virtualization@lists.linux-foundation.org
> > > Cc: netdev@vger.kernel.org
> > > Cc: kvm@vger.kernel.org
> > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> >
> > I am not sure I see a bug in virtio: the config pace layout simply
> > changes as msix is enabled and disabled (and if you look at the latest
> > draft, also on whether 64 bit features are enabled).
> > It doesn't depend on msix capability being present in device.
> >
> > The spec seems to be explicit enough:
> > If MSI-X is enabled for the device, two additional fields immediately
> > follow this header.
> >
> > So I'm guessing the bug is in kvm tools which assume
> > same layout for when msix is enabled and disabled.
> > qemu-kvm seems to do the right thing so the device
> > seems to get the correct mac.
>
> So, the config space moves once MSI-X is enabled? In which case, it
> should say "ONCE MSI-X is enabled..."
>
> Thanks,
> Rusty.
Yes. Or maybe 'WHEN' - since if MSI-X is disabled again, it moves back.
Let's update the spec to make it clearer?
--
MST
^ permalink raw reply
* Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X
From: Rusty Russell @ 2011-08-23 3:49 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, virtualization, Sasha Levin, kvm, linux-kernel
In-Reply-To: <20110822083613.GA18556@redhat.com>
On Mon, 22 Aug 2011 11:36:13 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Yes. Or maybe 'WHEN' - since if MSI-X is disabled again, it moves back.
> Let's update the spec to make it clearer?
I left the language as is, but added a footnote at the end of the
sentence:
If MSI-X is enabled for the device, two additional fields immediately
follow this header:
[footnote: ie. once you enable MSI-X on the device, the other
fields move. If you turn it off again, they move back!]
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to mod_devicetable.h
From: Greg KH @ 2011-08-23 22:41 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, Haiyang Zhang
In-Reply-To: <1310752065-27895-3-git-send-email-kys@microsoft.com>
On Fri, Jul 15, 2011 at 10:45:51AM -0700, K. Y. Srinivasan wrote:
> In preparation for implementing vmbus aliases for auto-loading
> Hyper-V drivers, define vmbus specific device ID.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> include/linux/mod_devicetable.h | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index ae28e93..5a12377 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -405,6 +405,13 @@ struct virtio_device_id {
> };
> #define VIRTIO_DEV_ANY_ID 0xffffffff
>
> +/*
> + * For Hyper-V devices we use the device guid as the id.
> + */
> +struct hv_vmbus_device_id {
> + __u8 guid[16];
> +};
Why do you not need a driver_data pointer here? Are you sure you aren't
ever going to need it in the future? Hint, I think you will...
greg k-h
^ permalink raw reply
* Re: [PATCH 006/117] Staging: hv: blkvsc: Use the newly introduced vmbus ID in the blockvsc driver
From: Greg KH @ 2011-08-23 22:45 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: devel, Haiyang Zhang, gregkh, linux-kernel, virtualization
In-Reply-To: <1310752065-27895-6-git-send-email-kys@microsoft.com>
On Fri, Jul 15, 2011 at 10:45:54AM -0700, K. Y. Srinivasan wrote:
> Use the newly introduced vmbus ID in the blockvsc driver.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/staging/hv/blkvsc_drv.c | 16 ++++++++++++++++
> 1 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/staging/hv/blkvsc_drv.c b/drivers/staging/hv/blkvsc_drv.c
> index 018b293..38117d96 100644
> --- a/drivers/staging/hv/blkvsc_drv.c
> +++ b/drivers/staging/hv/blkvsc_drv.c
> @@ -802,10 +802,26 @@ static void blkvsc_request(struct request_queue *queue)
> }
> }
>
> +static const struct hv_vmbus_device_id id_table[] = {
> + {
> + /* IDE guid */
> + .guid = {
> + 0x32, 0x26, 0x41, 0x32, 0xcb, 0x86, 0xa2, 0x44,
> + 0x9b, 0x5c, 0x50, 0xd1, 0x41, 0x73, 0x54, 0xf5
> + }
> + },
> + {
> + .guid = {
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> + }
No, just zero terminate the list with a simple { } entry, not the whole
thing spelled for every individual byte.
Same goes for the other patches that added this id_table, please fix
those.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 012/117] Staging: hv: blkvsc: Add the MODULE_DEVICE_TABLE() line
From: Greg KH @ 2011-08-23 22:46 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, Haiyang Zhang
In-Reply-To: <1310752065-27895-12-git-send-email-kys@microsoft.com>
On Fri, Jul 15, 2011 at 10:46:00AM -0700, K. Y. Srinivasan wrote:
> Add the MODULE_DEVICE_TABLE() line in blkvsc_drv.c.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/staging/hv/blkvsc_drv.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/staging/hv/blkvsc_drv.c b/drivers/staging/hv/blkvsc_drv.c
> index 38117d96..8e4c206 100644
> --- a/drivers/staging/hv/blkvsc_drv.c
> +++ b/drivers/staging/hv/blkvsc_drv.c
> @@ -818,6 +818,7 @@ static const struct hv_vmbus_device_id id_table[] = {
> },
> };
>
> +MODULE_DEVICE_TABLE(vmbus, id_table);
Please merge this back into the patch that added the id_table to the
driver, having it separate like this is just ridiculous.
greg k-h
^ permalink raw reply
* Re: [PATCH 011/117] Staging: hv: util: Use the newly introduced vmbus ID in util driver
From: Greg KH @ 2011-08-23 22:46 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, Haiyang Zhang
In-Reply-To: <1310752065-27895-11-git-send-email-kys@microsoft.com>
On Fri, Jul 15, 2011 at 10:45:59AM -0700, K. Y. Srinivasan wrote:
> Use the newly introduced vmbus ID in util driver.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/staging/hv/hv_util.c | 38 ++++++++++++++++++++++++++++++++++++++
> 1 files changed, 38 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/staging/hv/hv_util.c b/drivers/staging/hv/hv_util.c
> index e72bfe0..35540f5 100644
> --- a/drivers/staging/hv/hv_util.c
> +++ b/drivers/staging/hv/hv_util.c
> @@ -226,9 +226,47 @@ static int util_remove(struct hv_device *dev)
> return 0;
> }
>
> +static const struct hv_vmbus_device_id id_table[] = {
> + {
> + /* Shutdown guid */
> + .guid = {
> + 0x31, 0x60, 0x0B, 0X0E, 0x13, 0x52, 0x34, 0x49,
> + 0x81, 0x8B, 0x38, 0XD9, 0x0C, 0xED, 0x39, 0xDB
> + }
> + },
> + {
> + /* Time synch guid */
> + .guid = {
> + 0x30, 0xe6, 0x27, 0x95, 0xae, 0xd0, 0x7b, 0x49,
> + 0xad, 0xce, 0xe8, 0x0a, 0xb0, 0x17, 0x5c, 0xaf
> + }
> + },
> + {
> + /* Heartbeat guid */
> + .guid = {
> + 0x39, 0x4f, 0x16, 0x57, 0x15, 0x91, 0x78, 0x4e,
> + 0xab, 0x55, 0x38, 0x2f, 0x3b, 0xd5, 0x42, 0x2d
> + }
> + },
> + {
> + /* KVP guid */
> + .guid = {
> + 0xe7, 0xf4, 0xa0, 0xa9, 0x45, 0x5a, 0x96, 0x4d,
> + 0xb8, 0x27, 0x8a, 0x84, 0x1e, 0x8c, 0x3, 0xe6
> + }
> + },
> + {
> + .guid = {
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> + }
> + },
> +};
> +
Merge this into the previous patch please.
greg k-h
^ permalink raw reply
* Re: [PATCH 017/117] Staging: hv: mouse: Do not auto-load the mouse driver
From: Greg KH @ 2011-08-23 22:47 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: devel, Haiyang Zhang, gregkh, linux-kernel, virtualization
In-Reply-To: <1310752065-27895-17-git-send-email-kys@microsoft.com>
On Fri, Jul 15, 2011 at 10:46:05AM -0700, K. Y. Srinivasan wrote:
> The mouse driver is not functional and so, prevent auto-loading
> of this driver. Earlier we added the MODULE_DEVICE_TABLE() line
> to verify that the appropriate module alias was generated.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/staging/hv/hv_mouse.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
> index d6441d6..95986fb 100644
> --- a/drivers/staging/hv/hv_mouse.c
> +++ b/drivers/staging/hv/hv_mouse.c
> @@ -931,7 +931,10 @@ static const struct hv_vmbus_device_id id_table[] = {
> },
> };
>
> -MODULE_DEVICE_TABLE(vmbus, id_table);
> +/*
> + * The mouse driver is not functional; do not auto-load it.
> + */
> +/* MODULE_DEVICE_TABLE(vmbus, id_table); */
Then you should not have caused it to be autoloaded on the previous
patch. Please merge this with the previous patch, which would be merged
with the previous patch to that, smushing them all into a smaller patch
series.
greg k-h
^ permalink raw reply
* Re: [PATCH 019/117] Staging: hv: vmbus: Cleanup vmbus_uevent() code
From: Greg KH @ 2011-08-23 22:49 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, Haiyang Zhang
In-Reply-To: <1310752065-27895-19-git-send-email-kys@microsoft.com>
On Fri, Jul 15, 2011 at 10:46:07AM -0700, K. Y. Srinivasan wrote:
> Now generate appropriate uevent based on the modalias string. As part of this,
> cleanup the existing uevent code.
Note, you just change the user api here, did you have tools that relied
on the old format? If so, they just broke :(
> + for (i = 0; i < (sizeof(struct hv_vmbus_device_id) * 2); i += 2)
> + sprintf(&alias_name[i], "%02x", dev->dev_type.b[i/2]);
Don't we have a type for printing out a uuid already?
And what's with the jumping by 2 yet dividing? What am I missing here?
greg k-h
^ permalink raw reply
* Re: [PATCH 020/117] Staging: hv: vmbus: Support the notion of id tables in vmbus_match()
From: Greg KH @ 2011-08-23 22:51 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: devel, Haiyang Zhang, gregkh, linux-kernel, virtualization
In-Reply-To: <1310752065-27895-20-git-send-email-kys@microsoft.com>
On Fri, Jul 15, 2011 at 10:46:08AM -0700, K. Y. Srinivasan wrote:
> Introduce code to handle driver specific id tables to the vmbus core
> (vmbus_match). This would allow us to handle more than one device type
> with a given driver.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/staging/hv/vmbus_drv.c | 21 ++++++++++++++++-----
> 1 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> index 7baba62..3a8eb36 100644
> --- a/drivers/staging/hv/vmbus_drv.c
> +++ b/drivers/staging/hv/vmbus_drv.c
> @@ -255,21 +255,32 @@ static int vmbus_uevent(struct device *device, struct kobj_uevent_env *env)
> return ret;
> }
>
> +static bool is_not_null_guid(const __u8 *guid)
Negative function names cause people's heads to get confused.
Please do "is_null_guid()" instead.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 027/117] Staging: hv: blkvsc: Get rid of the dev_type guid from blkvsc_drv.c
From: Greg KH @ 2011-08-23 22:52 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: devel, Haiyang Zhang, gregkh, linux-kernel, virtualization
In-Reply-To: <1310752065-27895-27-git-send-email-kys@microsoft.com>
On Fri, Jul 15, 2011 at 10:46:15AM -0700, K. Y. Srinivasan wrote:
> Get rid of the dev_type guid from blkvsc_drv.c as it is no longer used.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/staging/hv/blkvsc_drv.c | 9 ---------
> 1 files changed, 0 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/hv/blkvsc_drv.c b/drivers/staging/hv/blkvsc_drv.c
> index 8e4c206..cb61598 100644
> --- a/drivers/staging/hv/blkvsc_drv.c
> +++ b/drivers/staging/hv/blkvsc_drv.c
> @@ -111,14 +111,6 @@ struct block_device_context {
>
> static const char *drv_name = "blkvsc";
>
> -/* {32412632-86cb-44a2-9b5c-50d1417354f5} */
> -static const uuid_le dev_type = {
> - .b = {
> - 0x32, 0x26, 0x41, 0x32, 0xcb, 0x86, 0xa2, 0x44,
> - 0x9b, 0x5c, 0x50, 0xd1, 0x41, 0x73, 0x54, 0xf5
> - }
> -};
Could be merged with the previous patch that added the guid in the other
format, right?
greg k-h
^ permalink raw reply
* Re: [PATCH 031/117] Staging: hv: netvsc: Get rid of the driver name initialization in netvsc.c
From: Greg KH @ 2011-08-23 22:56 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: devel, Haiyang Zhang, gregkh, linux-kernel, virtualization
In-Reply-To: <1310752065-27895-31-git-send-email-kys@microsoft.com>
On Fri, Jul 15, 2011 at 10:46:19AM -0700, K. Y. Srinivasan wrote:
> Now, get rid of the driver name initialization in int netvsc_initialize().
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/staging/hv/netvsc.c | 5 -----
> 1 files changed, 0 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/hv/netvsc.c b/drivers/staging/hv/netvsc.c
> index 6f4541b..d085018 100644
> --- a/drivers/staging/hv/netvsc.c
> +++ b/drivers/staging/hv/netvsc.c
> @@ -32,9 +32,6 @@
> #include "hyperv_net.h"
>
>
> -/* Globals */
> -static const char *driver_name = "netvsc";
> -
> static struct netvsc_device *alloc_net_device(struct hv_device *device)
> {
> struct netvsc_device *net_device;
> @@ -999,7 +996,5 @@ cleanup:
> int netvsc_initialize(struct hv_driver *drv)
> {
>
> - drv->name = driver_name;
> -
Please merge this with the previous patch.
greg k-h
^ permalink raw reply
* Re: [PATCH 035/117] Staging: hv: vmbus: Introduce a function to map the dev_type guid to a name
From: Greg KH @ 2011-08-23 23:01 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: devel, Haiyang Zhang, gregkh, linux-kernel, virtualization
In-Reply-To: <1310752065-27895-35-git-send-email-kys@microsoft.com>
On Fri, Jul 15, 2011 at 10:46:23AM -0700, K. Y. Srinivasan wrote:
> To support "human readable" sysfs attribute, introduce a function to map the
> dev_type guid to a string.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/staging/hv/channel_mgmt.c | 36 ++++++++++++++++++++++++++++++++++++
> drivers/staging/hv/hyperv.h | 2 +-
> 2 files changed, 37 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
> index 11beb41..926bc87 100644
> --- a/drivers/staging/hv/channel_mgmt.c
> +++ b/drivers/staging/hv/channel_mgmt.c
> @@ -112,6 +112,42 @@ static const uuid_le
>
> };
>
> +static const char *blk_dev_type = "hv_block";
> +static const char *net_dev_type = "hv_net";
> +static const char *scsi_dev_type = "hv_scsi";
> +static const char *mouse_dev_type = "hv_mouse";
> +static const char *util_dev_type = "hv_util";
Ick, no. There should never be any type of "central" knowledge of the
different driver names or types like this. It should all be
self-contained within the individual drivers.
You will note that no other bus type has this type of thing, which
should have been a hint that you should not have done this...
greg k-h
^ permalink raw reply
* Re: [PATCH 036/117] Staging: hv: vmbus: Make class_id attribute a human readable string
From: Greg KH @ 2011-08-23 23:02 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: devel, Haiyang Zhang, gregkh, linux-kernel, virtualization
In-Reply-To: <1310752065-27895-36-git-send-email-kys@microsoft.com>
On Fri, Jul 15, 2011 at 10:46:24AM -0700, K. Y. Srinivasan wrote:
> Now that we have a human readable device_type, use that and get rid of the
> guid based device type.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/staging/hv/vmbus_drv.c | 20 ++------------------
> 1 files changed, 2 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> index 9a0df2c..67415ab 100644
> --- a/drivers/staging/hv/vmbus_drv.c
> +++ b/drivers/staging/hv/vmbus_drv.c
> @@ -113,24 +113,8 @@ static ssize_t vmbus_show_device_attr(struct device *dev,
> get_channel_info(hv_dev, &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",
You just changed what this sysfs file contains to have something totally
different than what it used to.
What userspace tools just broke?
If you change the data in the file, shouldn't the name of the file
change as well?
greg k-h
^ permalink raw reply
* Re: [PATCH 037/117] Staging: hv: vmbus: Get rid of the device_id attribute
From: Greg KH @ 2011-08-23 23:03 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: devel, Haiyang Zhang, gregkh, linux-kernel, virtualization
In-Reply-To: <1310752065-27895-37-git-send-email-kys@microsoft.com>
On Fri, Jul 15, 2011 at 10:46:25AM -0700, K. Y. Srinivasan wrote:
> The guid based instance data is not very useful. There is enough information
> to identify the device. Get rid of this attribute.
But userspace probably wants to see this at times, don't remove it,
someone is going to need it, especially as this is how the driver/device
is bound together.
why delete it?
greg k-h
^ permalink raw reply
* Re: [PATCH 040/117] Staging: hv: vmbus: Cleanup error handling in hv_init()
From: Greg KH @ 2011-08-23 23:04 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: devel, Haiyang Zhang, gregkh, linux-kernel, virtualization
In-Reply-To: <1310752065-27895-40-git-send-email-kys@microsoft.com>
On Fri, Jul 15, 2011 at 10:46:28AM -0700, K. Y. Srinivasan wrote:
> Use standard Linux error codes.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/staging/hv/hv.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/hv/hv.c b/drivers/staging/hv/hv.c
> index 2f94880..e733173 100644
> --- a/drivers/staging/hv/hv.c
> +++ b/drivers/staging/hv/hv.c
> @@ -225,8 +225,8 @@ cleanup:
>
> vfree(virtaddr);
> }
> - ret = -1;
> - return ret;
> +
> + return -ENOTSUPP;
The ret variable is no longer needed here, so just remove it entirely in
this patch.
greg k-h
^ permalink raw reply
* Re: [PATCH 081/117] Staging: hv: vmbus: Introduce a lock to protect the ext field in hv_device
From: Greg KH @ 2011-08-23 23:08 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: devel, Haiyang Zhang, gregkh, linux-kernel, virtualization
In-Reply-To: <1310752065-27895-81-git-send-email-kys@microsoft.com>
On Fri, Jul 15, 2011 at 10:47:09AM -0700, K. Y. Srinivasan wrote:
> The current mechanism for handling references in broken.
> Introduce a lock to protect the ext field in hv_device.
Why would that lock ever be needed? How can things change to this
pointer in different ways like you are thinking it could? Doesn't the
reference counting in the device itself handle this properly?
greg k-h
^ permalink raw reply
* Re: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to manage ref_cnt
From: Greg KH @ 2011-08-23 23:10 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: devel, Haiyang Zhang, gregkh, linux-kernel, virtualization
In-Reply-To: <1310752065-27895-86-git-send-email-kys@microsoft.com>
On Fri, Jul 15, 2011 at 10:47:14AM -0700, K. Y. Srinivasan wrote:
> Now that we have a spin lock protecting access to the stor device pointer,
> use it manage the reference count as well.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/staging/hv/hyperv_storage.h | 8 ++++----
> drivers/staging/hv/storvsc.c | 10 +++++-----
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/hv/hyperv_storage.h b/drivers/staging/hv/hyperv_storage.h
> index 53b65be..d946211 100644
> --- a/drivers/staging/hv/hyperv_storage.h
> +++ b/drivers/staging/hv/hyperv_storage.h
> @@ -265,7 +265,7 @@ struct storvsc_device {
> struct hv_device *device;
>
> /* 0 indicates the device is being destroyed */
> - atomic_t ref_count;
> + int ref_count;
Is this really needed? Can't you rely on the reference count of the
hv_device itself?
greg k-h
^ permalink raw reply
* Re: [PATCH 0000/0117] Staging: hv: Driver cleanup
From: Greg KH @ 2011-08-23 23:11 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: devel, gregkh, linux-kernel, virtualization
In-Reply-To: <1310752024-27854-1-git-send-email-kys@microsoft.com>
On Fri, Jul 15, 2011 at 10:47:04AM -0700, K. Y. Srinivasan wrote:
> Further cleanup of the hv drivers. Back in June I had sent two patch
> sets to address these issues. I have addressed the comments I got from
> the community on my earlier patches here:
>
> 1) Implement code for autoloading the vmbus drivers without using PCI or DMI
> signatures. I have implemented this based on Greg's feedback on my earlier
> implementation.
>
> 2) Cleanup error handling across the board and use standard Linux error codes.
>
> 3) General cleanup
>
> 4) Some bug fixes.
>
> 5) Cleanup the reference counting mess for both stor and net devices.
>
> 6) Handle all block devices using the storvsc driver. I have modified
> the implementation here based on Christoph's feedback on my earlier
> implementation.
>
> 7) Accomodate some host side scsi emulation bugs.
>
> 8) In case of scsi errors off-line the device.
Care to respin this patch series based on my comments?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 1/8] Staging: hv: vmbus: Fix a checkpatch warning in ring_buffer.c
From: Greg KH @ 2011-08-23 23:14 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: devel, Haiyang Zhang, gregkh, linux-kernel, virtualization
In-Reply-To: <1311101065-23417-1-git-send-email-kys@microsoft.com>
On Tue, Jul 19, 2011 at 11:44:18AM -0700, K. Y. Srinivasan wrote:
> Fix a checkpatch warning in ring_buffer.c (line over 80 characters).
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/staging/hv/ring_buffer.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/staging/hv/ring_buffer.c b/drivers/staging/hv/ring_buffer.c
> index 9212699..e41d206 100644
> --- a/drivers/staging/hv/ring_buffer.c
> +++ b/drivers/staging/hv/ring_buffer.c
> @@ -34,7 +34,8 @@
>
>
> /* Amount of space to write to */
> -#define BYTES_AVAIL_TO_WRITE(r, w, z) ((w) >= (r)) ? ((z) - ((w) - (r))) : ((r) - (w))
> +#define BYTES_AVAIL_TO_WRITE(r, w, z) \
> +(((w) >= (r)) ? ((z) - ((w) - (r))) : ((r) - (w)))
Please at least indent the second line a little bit, otherwise it's a
pain to read, right?
greg k-h
^ permalink raw reply
* Re: [PATCH 4/8] Staging: hv: vmbus: Fix checkpatch warnings
From: Greg KH @ 2011-08-23 23:16 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: devel, Haiyang Zhang, gregkh, linux-kernel, virtualization
In-Reply-To: <1311101065-23417-4-git-send-email-kys@microsoft.com>
On Tue, Jul 19, 2011 at 11:44:21AM -0700, K. Y. Srinivasan wrote:
> Fix checkpatch warnings in hv.c
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/staging/hv/hv.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/hv/hv.c b/drivers/staging/hv/hv.c
> index e733173..14e6315 100644
> --- a/drivers/staging/hv/hv.c
> +++ b/drivers/staging/hv/hv.c
> @@ -111,7 +111,7 @@ static u64 do_hypercall(u64 control, void *input, void *output)
> u64 hv_status = 0;
> u64 input_address = (input) ? virt_to_phys(input) : 0;
> u64 output_address = (output) ? virt_to_phys(output) : 0;
> - volatile void *hypercall_page = hv_context.hypercall_page;
> + void *hypercall_page = hv_context.hypercall_page;
Are you sure? This was just someone being foolish? No other reason
someone tried to use volatile here?
greg k-h
^ permalink raw reply
* Re: [PATCH 0/8] Staging: hv: vmbus: Driver cleanup
From: Greg KH @ 2011-08-23 23:18 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: devel, gregkh, linux-kernel, virtualization
In-Reply-To: <1313446210-10611-1-git-send-email-kys@microsoft.com>
On Mon, Aug 15, 2011 at 03:10:10PM -0700, K. Y. Srinivasan wrote:
> Further cleanup of the vmbus driver:
>
> 1) Cleanup the interrupt handler by inlining some code.
>
> 2) Ensure message handling is performed on the same CPU that
> takes the vmbus interrupt.
>
> 3) Check for events before messages (from the host).
>
> 4) Disable auto eoi for the vmbus interrupt since Linux will eoi the
> interrupt anyway.
>
> 5) Some general cleanup.
As I didn't apply your other changes, care to respin all of your
outstanding hv patches against the next linux-next release and resend
them? I tried picking some of the other patches, but they started not
applying easily so it would be easier for everyone to just resend them
after you fix them up.
thanks,
greg k-h
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox