* Re: [PATCH 1/1] Staging: hv: mousevsc: Move the mouse driver out of staging
From: Joe Perches @ 2011-10-15 6:36 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, dmitry.torokhov,
linux-input, Haiyang Zhang
In-Reply-To: <1318658907-16698-1-git-send-email-kys@microsoft.com>
On Fri, 2011-10-14 at 23:08 -0700, K. Y. Srinivasan wrote:
> In preparation for moving the mouse driver out of staging, seek community
> review of the mouse driver code.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Just some stylistic things, none of which should
prevent any code movement.
> +++ b/drivers/hid/hv_mouse.c
[]
> +struct mousevsc_dev {
> + struct hv_device *device;
> + unsigned char init_complete;
bool?
[]
> +static void mousevsc_on_channel_callback(void *context)
> +{
> + const int packetSize = 0x100;
> + int ret = 0;
> + struct hv_device *device = (struct hv_device *)context;
> +
> + u32 bytes_recvd;
> + u64 req_id;
> + unsigned char packet[0x100];
[packetSize];
> + struct vmpacket_descriptor *desc;
> + unsigned char *buffer = packet;
> + int bufferlen = packetSize;
> +
> +
trivial extra blank line
> + do {
> + ret = vmbus_recvpacket_raw(device->channel, buffer,
> + bufferlen, &bytes_recvd, &req_id);
> +
> + if (ret == 0) {
> + if (bytes_recvd > 0) {
> + desc = (struct vmpacket_descriptor *)buffer;
> +
> + switch (desc->type) {
> + case VM_PKT_COMP:
> + break;
> +
> + case VM_PKT_DATA_INBAND:
> + mousevsc_on_receive(
> + device, desc);
> + break;
> +
> + default:
> + pr_err("unhandled packet type %d, tid %llx len %d\n",
> + desc->type,
> + req_id,
> + bytes_recvd);
> + break;
> + }
> +
> + /* reset */
> + if (bufferlen > packetSize) {
> + kfree(buffer);
> +
> + buffer = packet;
> + bufferlen = packetSize;
> + }
> + } else {
> + if (bufferlen > packetSize) {
> + kfree(buffer);
> +
> + buffer = packet;
> + bufferlen = packetSize;
> + }
> + break;
> + }
> + } else if (ret == -ENOBUFS) {
> + /* Handle large packet */
> + bufferlen = bytes_recvd;
> + buffer = kzalloc(bytes_recvd, GFP_ATOMIC);
> +
> + if (buffer == NULL) {
> + buffer = packet;
> + bufferlen = packetSize;
> + break;
> + }
> + }
> + } while (1);
This do {} while (1); seems like it could be simpler,
less indented and less confusing if it used continues
or gotos like below (if I wrote it correctly...)
loop:
ret = vmbus_bus_recvpacket_raw(device->channel, buffer,
bufferlen, &bytes_recvd, &req_id);
switch (ret) {
case -ENOBUFS:
/* Handle large packet */
bufferlen = bytes_recvd;
buffer = kzalloc(bytes_recvd, GFP_ATOMIC);
/*
Why kzalloc and not kmalloc?
The stack variable packet is not memset to 0,
why should buffer be zeroed?
*/
if (!buffer)
return;
goto loop;
case 0:
if (bytes_recvd <= 0)
goto loop;
desc = (struct vmpacket_descriptor *)buffer;
switch (desc->type) {
case VM_PKT_COMP:
break;
case VM_PKT_DATA_INBAND:
mousevsc_on_receive(device, desc);
break;
default:
pr_err("unhandled packet type %d, tid %llx len %d\n",
desc->type, req_id, bytes_recvd);
break;
}
/* reset to original buffers */
if (bufferlen > packetSize) {
kfree(buffer);
buffer = packet;
bufferlen = packetSize;
}
}
goto loop;
}
> +
> +static int mousevsc_connect_to_vsp(struct hv_device *device)
> +{
[]
> + if (!response->response.approved) {
> + pr_err("synthhid protocol request failed (version %d)",
> + SYNTHHID_INPUT_VERSION);
Missing \n at end of format string
[]
> +static int mousevsc_on_device_add(struct hv_device *device)
> +{
[]
> + ret = vmbus_open(device->channel,
> + INPUTVSC_SEND_RING_BUFFER_SIZE,
> + INPUTVSC_RECV_RING_BUFFER_SIZE,
> + NULL,
> + 0,
> + mousevsc_on_channel_callback,
> + device
> + );
Nicer if aligned to open paren I think.
ret = vmbus_open(device->channel,
INPUTVSC_SEND_RING_BUFFER_SIZE,
INPUTVSC_RECV_RING_BUFFER_SIZE,
NULL, 0, mousevsc_on_channel_callback,
device);
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: mousevsc: Move the mouse driver out of staging
From: KY Srinivasan @ 2011-10-15 13:24 UTC (permalink / raw)
To: Joe Perches
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
dmitry.torokhov@gmail.com, linux-input@vger.kernel.org,
Haiyang Zhang
In-Reply-To: <1318660596.6035.27.camel@Joe-Laptop>
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Saturday, October 15, 2011 2:37 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org;
> dmitry.torokhov@gmail.com; linux-input@vger.kernel.org; Haiyang Zhang
> Subject: Re: [PATCH 1/1] Staging: hv: mousevsc: Move the mouse driver out of
> staging
>
> On Fri, 2011-10-14 at 23:08 -0700, K. Y. Srinivasan wrote:
> > In preparation for moving the mouse driver out of staging, seek community
> > review of the mouse driver code.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
>
> Just some stylistic things, none of which should
> prevent any code movement.
Thanks Joe. I will fix the issues you have noted either before or after
moving the code out of staging based on other review comments.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 1/1] Staging: hv: storvsc: Move the storage driver out of staging
From: James Bottomley @ 2011-10-15 21:27 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, ohering, linux-scsi,
hch, Haiyang Zhang
In-Reply-To: <1318660132-21529-1-git-send-email-kys@microsoft.com>
On Fri, 2011-10-14 at 23:28 -0700, K. Y. Srinivasan wrote:
> In preparation for moving the storage driver out of staging, seek community
> review of the storage driver code.
That's not exactly a very descriptive commit message for me to put into
SCSI. It doesn't have to be huge, just something like "driver to enable
hyperv windows guest on Linux" or something.
> drivers/scsi/Kconfig | 7 +
> drivers/scsi/Makefile | 3 +
> drivers/scsi/storvsc_drv.c | 1480 ++++++++++++++++++++++++++++++++++++++
> drivers/staging/hv/Kconfig | 6 -
> drivers/staging/hv/Makefile | 2 -
> drivers/staging/hv/storvsc_drv.c | 1480 --------------------------------------
> 6 files changed, 1490 insertions(+), 1488 deletions(-)
What tree is this against? The hv/storvsc_drv.c in upstream only has
jejb@dabdike> wc -l drivers/staging/hv/storvsc_drv.c
792 drivers/staging/hv/storvsc_drv.c
i.e. whatever you're sending is double the length (and obviously I have
trouble applying the patch.
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -0,0 +1,1480 @@
So not a detailed review, but a couple of structural observations:
The whole driver is a writeout deadlock trap since I assume you intend
for it to be used as root/swap of a linux guest? The writeout deadlock
occurs when we can't make forward progress on a direct writeout I/O
because one of the GFP_ATOMIC memory allocations in your driver fails.
The only way to fix this is to back the allocations with mempools (one
per actual device) so you can guarantee that whatever memory pressure
the system is under, one command can always make the round trip to
storage and back.
You use the locked version of queuecommand, but it looks like you don't
need the host lock in the submit path, so why not just use the unlocked
version?
The bounce buffer handling for discontiguous sg lists is a complete
nightmare. And can't you just fix the hypervisor instead of pulling
this level of nastiness in the driver, I mean really, the last piece of
real SCSI hardware that failed this badly was produced in the dark ages.
Assuming the answer is "no", I really think you need to set the minimum
block size to 4k, which will completely avoid the situation.
Minor, but use accessors like shost_priv()
The SET WINDOW command is an obsolete scanner command ... the comment
about smart issuing it looks completely bogus, but even if there's
something issuing scanner commands and your hypervisor crashes on them,
you need to reject it with ILLEGAL_REQUEST not DID_ERROR.
All the business around max segment size and supplying your own merge
biovec function looks a bit bogus, don't you just want to disable
clustering? This looks like another knock on from the weird hypervisor
sg list handling, where you apparently need one sg entry per page. This
was originally a bug in the circa 1990 era NCR 53c800 host controllers
we then had, so that's actually why the clustering parameter exists ...
it's sort of nostalgic to see you resurrecting such an ancient bug.
James
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: storvsc: Move the storage driver out of staging
From: KY Srinivasan @ 2011-10-16 3:01 UTC (permalink / raw)
To: James Bottomley
Cc: linux-scsi@vger.kernel.org, Haiyang Zhang, gregkh@suse.de,
ohering@suse.com, linux-kernel@vger.kernel.org, hch@infradead.org,
virtualization@lists.osdl.org, devel@linuxdriverproject.org
In-Reply-To: <1318714047.7271.49.camel@dabdike.int.hansenpartnership.com>
> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Saturday, October 15, 2011 5:27 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> linux-scsi@vger.kernel.org; hch@infradead.org; Haiyang Zhang
> Subject: Re: [PATCH 1/1] Staging: hv: storvsc: Move the storage driver out of
> staging
>
> On Fri, 2011-10-14 at 23:28 -0700, K. Y. Srinivasan wrote:
> > In preparation for moving the storage driver out of staging, seek community
> > review of the storage driver code.
>
> That's not exactly a very descriptive commit message for me to put into
> SCSI. It doesn't have to be huge, just something like "driver to enable
> hyperv windows guest on Linux" or something.
Sorry about the commit message. I will have a more descriptive message in the next
submission.
>
>
> > drivers/scsi/Kconfig | 7 +
> > drivers/scsi/Makefile | 3 +
> > drivers/scsi/storvsc_drv.c | 1480
> ++++++++++++++++++++++++++++++++++++++
> > drivers/staging/hv/Kconfig | 6 -
> > drivers/staging/hv/Makefile | 2 -
> > drivers/staging/hv/storvsc_drv.c | 1480 --------------------------------------
> > 6 files changed, 1490 insertions(+), 1488 deletions(-)
>
> What tree is this against? The hv/storvsc_drv.c in upstream only has
>
> jejb@dabdike> wc -l drivers/staging/hv/storvsc_drv.c
> 792 drivers/staging/hv/storvsc_drv.c
>
> i.e. whatever you're sending is double the length (and obviously I have
> trouble applying the patch.
This patch moves the file from drivers/staging/hv/ directory to the
drivers/scsi directory; hence double the length. I saw an email from Greg earlier where
he recommended that the patch we post should not delete/modify the files in the hv
directory. When I submit this next, I will take that approach.
>
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -0,0 +1,1480 @@
>
> So not a detailed review, but a couple of structural observations:
>
> The whole driver is a writeout deadlock trap since I assume you intend
> for it to be used as root/swap of a linux guest? The writeout deadlock
> occurs when we can't make forward progress on a direct writeout I/O
> because one of the GFP_ATOMIC memory allocations in your driver fails.
> The only way to fix this is to back the allocations with mempools (one
> per actual device) so you can guarantee that whatever memory pressure
> the system is under, one command can always make the round trip to
> storage and back.
I will implement a mempool based allocation scheme as you have suggested.
>
> You use the locked version of queuecommand, but it looks like you don't
> need the host lock in the submit path, so why not just use the unlocked
> version?
Will do.
>
> The bounce buffer handling for discontiguous sg lists is a complete
> nightmare. And can't you just fix the hypervisor instead of pulling
> this level of nastiness in the driver, I mean really, the last piece of
> real SCSI hardware that failed this badly was produced in the dark ages.
The restrictions (as they are) are really from the windows host side and for what it is
worth, I have never seen this code triggered at least for I/O initiated by the file system.
> Assuming the answer is "no", I really think you need to set the minimum
> block size to 4k, which will completely avoid the situation.
I would love to get rid of the bounce buffer handling code by ensuring that the
upper level code would never present scatter gather lists that would trigger bounce buffer
handling code. How would I accomplish that.
>
> Minor, but use accessors like shost_priv()
Will do.
>
> The SET WINDOW command is an obsolete scanner command ... the comment
> about smart issuing it looks completely bogus, but even if there's
> something issuing scanner commands and your hypervisor crashes on them,
> you need to reject it with ILLEGAL_REQUEST not DID_ERROR.
Will do.
>
> All the business around max segment size and supplying your own merge
> biovec function looks a bit bogus, don't you just want to disable
> clustering? This looks like another knock on from the weird hypervisor
> sg list handling, where you apparently need one sg entry per page. This
> was originally a bug in the circa 1990 era NCR 53c800 host controllers
> we then had, so that's actually why the clustering parameter exists ...
> it's sort of nostalgic to see you resurrecting such an ancient bug.
I will try to clean this up. Would you want all the cleanup you have suggested here
done before the code can be moved out of staging or could you take the code and I could
give you the patches to address the issues listed by you. Once again, thanks for taking the time
to look at this code.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 1/1] Staging: hv: mousevsc: Move the mouse driver out of staging
From: Dan Carpenter @ 2011-10-16 22:28 UTC (permalink / raw)
To: Joe Perches
Cc: gregkh, Haiyang Zhang, dmitry.torokhov, linux-kernel,
virtualization, linux-input, devel
In-Reply-To: <1318660596.6035.27.camel@Joe-Laptop>
On Fri, Oct 14, 2011 at 11:36:36PM -0700, Joe Perches wrote:
> This do {} while (1); seems like it could be simpler,
> less indented and less confusing if it used continues
> or gotos like below (if I wrote it correctly...)
>
> loop:
> ret = vmbus_bus_recvpacket_raw(device->channel, buffer,
> bufferlen, &bytes_recvd, &req_id);
> switch (ret) {
> case -ENOBUFS:
> /* Handle large packet */
> bufferlen = bytes_recvd;
> buffer = kzalloc(bytes_recvd, GFP_ATOMIC);
> /*
> Why kzalloc and not kmalloc?
> The stack variable packet is not memset to 0,
> why should buffer be zeroed?
> */
> if (!buffer)
> return;
> goto loop;
> case 0:
> if (bytes_recvd <= 0)
> goto loop;
In the original we called break here (which is equivelent to a
return). Btw setting a stack variable and then returning immediately
like the original code did is pointless.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH 1/1] Staging: hv: mousevsc: Move the mouse driver out of staging
From: Joe Perches @ 2011-10-16 23:03 UTC (permalink / raw)
To: Dan Carpenter
Cc: K. Y. Srinivasan, dmitry.torokhov, Haiyang Zhang, gregkh,
linux-kernel, virtualization, linux-input, devel
In-Reply-To: <20111016222847.GM18470@longonot.mountain>
On Mon, 2011-10-17 at 01:28 +0300, Dan Carpenter wrote:
> On Fri, Oct 14, 2011 at 11:36:36PM -0700, Joe Perches wrote:
[]
> > case 0:
> > if (bytes_recvd <= 0)
> > goto loop;
> In the original we called break here (which is equivelent to a
> return). Btw setting a stack variable and then returning immediately
> like the original code did is pointless.
OK, return it is.
Perhaps this is better. (two of the breaks could be goto loop)
I did add a couple of checks to make sure kmalloc'd
memory was always freed.
Another possible simplification is to always use kmalloc
instead of using stack and/or kmalloc.
static void mousevsc_on_channel_callback(void *context)
{
static const int packetSize = 0x100;
unsigned char packet[packetSize];
int ret;
struct hv_device *device = (struct hv_device *)context;
u32 bytes_recvd;
u64 req_id;
struct vmpacket_descriptor *desc;
unsigned char *buffer = packet;
int bufferlen = packetSize;
bool malloced = false;
loop:
ret = vmbus_bus_recvpacket_raw(device->channel, buffer, bufferlen,
&bytes_recvd, &req_id);
switch (ret) {
case -ENOBUFS: /* Handle large packet */
if (malloced) /* Maybe repeated -ENOBUFS ? */
kfree(buffer);
bufferlen = bytes_recvd;
buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
if (!buffer)
return;
malloced = true;
break;
case 0:
if (bytes_recvd <= 0) {
if (malloced)
kfree(buffer);
return;
}
desc = (struct vmpacket_descriptor *)buffer;
switch (desc->type) {
case VM_PKT_DATA_INBAND:
mousevsc_on_receive(device, desc);
break;
default:
pr_err("unhandled packet type %d, tid %llx len %d\n",
desc->type, req_id, bytes_recvd);
break;
}
/* reset to original buffers */
if (malloced) {
kfree(buffer);
malloced = false;
buffer = packet;
bufferlen = packetSize;
}
break;
}
goto loop;
}
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: storvsc: Move the storage driver out of staging
From: James Bottomley @ 2011-10-17 13:59 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, linux-scsi@vger.kernel.org, hch@infradead.org,
Haiyang Zhang
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481AA0FED0@TK5EX14MBXC128.redmond.corp.microsoft.com>
On Sun, 2011-10-16 at 03:01 +0000, KY Srinivasan wrote:
>
> > -----Original Message-----
> > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> > Sent: Saturday, October 15, 2011 5:27 PM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> > linux-scsi@vger.kernel.org; hch@infradead.org; Haiyang Zhang
> > Subject: Re: [PATCH 1/1] Staging: hv: storvsc: Move the storage driver out of
> > staging
> >
> > On Fri, 2011-10-14 at 23:28 -0700, K. Y. Srinivasan wrote:
> > > In preparation for moving the storage driver out of staging, seek community
> > > review of the storage driver code.
> >
> > That's not exactly a very descriptive commit message for me to put into
> > SCSI. It doesn't have to be huge, just something like "driver to enable
> > hyperv windows guest on Linux" or something.
>
> Sorry about the commit message. I will have a more descriptive message in the next
> submission.
>
> >
> >
> > > drivers/scsi/Kconfig | 7 +
> > > drivers/scsi/Makefile | 3 +
> > > drivers/scsi/storvsc_drv.c | 1480
> > ++++++++++++++++++++++++++++++++++++++
> > > drivers/staging/hv/Kconfig | 6 -
> > > drivers/staging/hv/Makefile | 2 -
> > > drivers/staging/hv/storvsc_drv.c | 1480 --------------------------------------
> > > 6 files changed, 1490 insertions(+), 1488 deletions(-)
> >
> > What tree is this against? The hv/storvsc_drv.c in upstream only has
> >
> > jejb@dabdike> wc -l drivers/staging/hv/storvsc_drv.c
> > 792 drivers/staging/hv/storvsc_drv.c
> >
> > i.e. whatever you're sending is double the length (and obviously I have
> > trouble applying the patch.
>
> This patch moves the file from drivers/staging/hv/ directory to the
> drivers/scsi directory; hence double the length.
No, that's not it. Look again: the storvsc_drv.c in staging is removing
1480 lines, but in git head, this file is only 792 lines long ... is
there an alternative tree with the rest in?
The point I'm making is that the staging file you're modifying isn't the
one I see in Linus' git head, so which git tree is it in (I assume it's
somewhere waiting for the merge window)?
[...]
> > The bounce buffer handling for discontiguous sg lists is a complete
> > nightmare. And can't you just fix the hypervisor instead of pulling
> > this level of nastiness in the driver, I mean really, the last piece of
> > real SCSI hardware that failed this badly was produced in the dark ages.
>
> The restrictions (as they are) are really from the windows host side
> and for what it is
> worth, I have never seen this code triggered at least for I/O
> initiated by the file system.
>
>
> > Assuming the answer is "no", I really think you need to set the minimum
> > block size to 4k, which will completely avoid the situation.
>
> I would love to get rid of the bounce buffer handling code by ensuring
> that the
> upper level code would never present scatter gather lists that would
> trigger bounce buffer
> handling code. How would I accomplish that.
OK, so as I read the code, what it can't handle is fragments except at
the ends. As long as everything always transfers in multiples of 4k,
the problem will never occur. This means that all devices you present
need to tell the OS they have 4k sectors. I *think* you can do this by
setting blk_limits_io_min() in the driver ... but you'll have to test
this. The minimum sector size gets set also in sd.c when we probe the
disk. I think that won't override blk_limits_io_min(), but please test
(we don't have any SCSI drivers which have ever used this setting).
The way to test, is to read back the /sys/block/<dev>/queue/ settings
when presenting a 512 byte sector disk. (And the way to activate your
bounce code would be to create a filesystem with a < PAGE_SIZE block
size).
James
^ permalink raw reply
* Re: [PATCH 1/1] Staging: hv: storvsc: Move the storage driver out of staging
From: Julian Andres Klode @ 2011-10-17 14:10 UTC (permalink / raw)
To: James Bottomley
Cc: KY Srinivasan, linux-scsi@vger.kernel.org, Haiyang Zhang,
gregkh@suse.de, ohering@suse.com, linux-kernel@vger.kernel.org,
hch@infradead.org, virtualization@lists.osdl.org,
devel@linuxdriverproject.org
In-Reply-To: <1318859976.4794.8.camel@dabdike.int.hansenpartnership.com>
On Mon, Oct 17, 2011 at 08:59:36AM -0500, James Bottomley wrote:
> On Sun, 2011-10-16 at 03:01 +0000, KY Srinivasan wrote:
> >
> > > -----Original Message-----
> > > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> > > Sent: Saturday, October 15, 2011 5:27 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> > > linux-scsi@vger.kernel.org; hch@infradead.org; Haiyang Zhang
> > > Subject: Re: [PATCH 1/1] Staging: hv: storvsc: Move the storage driver out of
> > > staging
> > >
> > > On Fri, 2011-10-14 at 23:28 -0700, K. Y. Srinivasan wrote:
> > > > In preparation for moving the storage driver out of staging, seek community
> > > > review of the storage driver code.
> > >
> > > That's not exactly a very descriptive commit message for me to put into
> > > SCSI. It doesn't have to be huge, just something like "driver to enable
> > > hyperv windows guest on Linux" or something.
> >
> > Sorry about the commit message. I will have a more descriptive message in the next
> > submission.
> >
> > >
> > >
> > > > drivers/scsi/Kconfig | 7 +
> > > > drivers/scsi/Makefile | 3 +
> > > > drivers/scsi/storvsc_drv.c | 1480
> > > ++++++++++++++++++++++++++++++++++++++
> > > > drivers/staging/hv/Kconfig | 6 -
> > > > drivers/staging/hv/Makefile | 2 -
> > > > drivers/staging/hv/storvsc_drv.c | 1480 --------------------------------------
> > > > 6 files changed, 1490 insertions(+), 1488 deletions(-)
> > >
> > > What tree is this against? The hv/storvsc_drv.c in upstream only has
> > >
> > > jejb@dabdike> wc -l drivers/staging/hv/storvsc_drv.c
> > > 792 drivers/staging/hv/storvsc_drv.c
> > >
> > > i.e. whatever you're sending is double the length (and obviously I have
> > > trouble applying the patch.
> >
> > This patch moves the file from drivers/staging/hv/ directory to the
> > drivers/scsi directory; hence double the length.
>
> No, that's not it. Look again: the storvsc_drv.c in staging is removing
> 1480 lines, but in git head, this file is only 792 lines long ... is
> there an alternative tree with the rest in?
>
> The point I'm making is that the staging file you're modifying isn't the
> one I see in Linus' git head, so which git tree is it in (I assume it's
> somewhere waiting for the merge window)?
I guess it's the staging tree you're looking for.
--
Julian Andres Klode - Debian Developer, Ubuntu Member
See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: storvsc: Move the storage driver out of staging
From: KY Srinivasan @ 2011-10-17 15:15 UTC (permalink / raw)
To: James Bottomley
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, linux-scsi@vger.kernel.org, hch@infradead.org,
Haiyang Zhang
In-Reply-To: <1318859976.4794.8.camel@dabdike.int.hansenpartnership.com>
> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Monday, October 17, 2011 10:00 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> linux-scsi@vger.kernel.org; hch@infradead.org; Haiyang Zhang
> Subject: RE: [PATCH 1/1] Staging: hv: storvsc: Move the storage driver out of
> staging
>
> On Sun, 2011-10-16 at 03:01 +0000, KY Srinivasan wrote:
> >
> > > -----Original Message-----
> > > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> > > Sent: Saturday, October 15, 2011 5:27 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org;
> ohering@suse.com;
> > > linux-scsi@vger.kernel.org; hch@infradead.org; Haiyang Zhang
> > > Subject: Re: [PATCH 1/1] Staging: hv: storvsc: Move the storage driver out of
> > > staging
> > >
> > > On Fri, 2011-10-14 at 23:28 -0700, K. Y. Srinivasan wrote:
> > > > In preparation for moving the storage driver out of staging, seek community
> > > > review of the storage driver code.
> > >
> > > That's not exactly a very descriptive commit message for me to put into
> > > SCSI. It doesn't have to be huge, just something like "driver to enable
> > > hyperv windows guest on Linux" or something.
> >
> > Sorry about the commit message. I will have a more descriptive message in the
> next
> > submission.
> >
> > >
> > >
> > > > drivers/scsi/Kconfig | 7 +
> > > > drivers/scsi/Makefile | 3 +
> > > > drivers/scsi/storvsc_drv.c | 1480
> > > ++++++++++++++++++++++++++++++++++++++
> > > > drivers/staging/hv/Kconfig | 6 -
> > > > drivers/staging/hv/Makefile | 2 -
> > > > drivers/staging/hv/storvsc_drv.c | 1480 --------------------------------------
> > > > 6 files changed, 1490 insertions(+), 1488 deletions(-)
> > >
> > > What tree is this against? The hv/storvsc_drv.c in upstream only has
> > >
> > > jejb@dabdike> wc -l drivers/staging/hv/storvsc_drv.c
> > > 792 drivers/staging/hv/storvsc_drv.c
> > >
> > > i.e. whatever you're sending is double the length (and obviously I have
> > > trouble applying the patch.
> >
> > This patch moves the file from drivers/staging/hv/ directory to the
> > drivers/scsi directory; hence double the length.
>
> No, that's not it. Look again: the storvsc_drv.c in staging is removing
> 1480 lines, but in git head, this file is only 792 lines long ... is
> there an alternative tree with the rest in?
This patch is against Greg's staging tree where recently I merged all of the storage
drivers into a single driver. This change may not have percolated up yet - Greg has
checked this into his staging tree though.
As I deal with the review comments now, do you want me to get these changes
applied first to Greg's tree before you would consider moving this driver to drivers/scsi
directory, or could you move the driver as it is in Greg's tree under staging and I could give
you patches against the moved driver.
>
> The point I'm making is that the staging file you're modifying isn't the
> one I see in Linus' git head, so which git tree is it in (I assume it's
> somewhere waiting for the merge window)?
>
> [...]
> > > The bounce buffer handling for discontiguous sg lists is a complete
> > > nightmare. And can't you just fix the hypervisor instead of pulling
> > > this level of nastiness in the driver, I mean really, the last piece of
> > > real SCSI hardware that failed this badly was produced in the dark ages.
> >
> > The restrictions (as they are) are really from the windows host side
> > and for what it is
> > worth, I have never seen this code triggered at least for I/O
> > initiated by the file system.
> >
> >
> > > Assuming the answer is "no", I really think you need to set the minimum
> > > block size to 4k, which will completely avoid the situation.
> >
> > I would love to get rid of the bounce buffer handling code by ensuring
> > that the
> > upper level code would never present scatter gather lists that would
> > trigger bounce buffer
> > handling code. How would I accomplish that.
>
> OK, so as I read the code, what it can't handle is fragments except at
> the ends. As long as everything always transfers in multiples of 4k,
> the problem will never occur. This means that all devices you present
> need to tell the OS they have 4k sectors. I *think* you can do this by
> setting blk_limits_io_min() in the driver ... but you'll have to test
> this. The minimum sector size gets set also in sd.c when we probe the
> disk. I think that won't override blk_limits_io_min(), but please test
> (we don't have any SCSI drivers which have ever used this setting).
>
> The way to test, is to read back the /sys/block/<dev>/queue/ settings
> when presenting a 512 byte sector disk. (And the way to activate your
> bounce code would be to create a filesystem with a < PAGE_SIZE block
> size).
Given that the bounce buffer handling code would typically
never be triggered, I not sure how useful it will be to try to get rid of the bounce buffer
code using a feature that is not well tested. In any event, I will try your suggestion though.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: storvsc: Move the storage driver out of staging
From: James Bottomley @ 2011-10-17 15:26 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, linux-scsi@vger.kernel.org, hch@infradead.org,
Haiyang Zhang
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481AA15174@TK5EX14MBXC124.redmond.corp.microsoft.com>
On Mon, 2011-10-17 at 15:15 +0000, KY Srinivasan wrote:
> This patch is against Greg's staging tree where recently I merged all
> of the storage
> drivers into a single driver. This change may not have percolated up
> yet - Greg has
> checked this into his staging tree though.
OK, that explains the difference.
> As I deal with the review comments now, do you want me to get these
> changes
> applied first to Greg's tree before you would consider moving this
> driver to drivers/scsi
> directory, or could you move the driver as it is in Greg's tree under
> staging and I could give
> you patches against the moved driver.
Either way is fine by me. The best route for this is to get the staging
update into Linus head, then work on the actual SCSI problems. I'd like
other SCSI people besides me to review it.
> > OK, so as I read the code, what it can't handle is fragments except at
> > the ends. As long as everything always transfers in multiples of 4k,
> > the problem will never occur. This means that all devices you present
> > need to tell the OS they have 4k sectors. I *think* you can do this by
> > setting blk_limits_io_min() in the driver ... but you'll have to test
> > this. The minimum sector size gets set also in sd.c when we probe the
> > disk. I think that won't override blk_limits_io_min(), but please test
> > (we don't have any SCSI drivers which have ever used this setting).
> >
> > The way to test, is to read back the /sys/block/<dev>/queue/ settings
> > when presenting a 512 byte sector disk. (And the way to activate your
> > bounce code would be to create a filesystem with a < PAGE_SIZE block
> > size).
>
> Given that the bounce buffer handling code would typically
> never be triggered, I not sure how useful it will be to try to get rid
> of the bounce buffer
> code using a feature that is not well tested.
As opposed to bounce buffer code which hasn't been well tested either if
it can't be triggered.
> In any event, I will try your suggestion though.
Just to be clear: Setting the minimum I/O size is completely well
tested: it's how we handle 4k sector drives. What hasn't been tested is
the ability of the *driver* to enforce the minimum using
blk_limits_io_min(). As long at that doesn't get overridden by sd when
it sees a 512b drive, the rest of the block paths (those which actually
enforce the minimum) have all been well tested.
James
^ permalink raw reply
* Re: [PATCH 1/1] Staging: hv: storvsc: Move the storage driver out of staging
From: Greg KH @ 2011-10-17 15:54 UTC (permalink / raw)
To: James Bottomley
Cc: KY Srinivasan, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, linux-scsi@vger.kernel.org, hch@infradead.org,
Haiyang Zhang
In-Reply-To: <1318865202.4794.21.camel@dabdike.int.hansenpartnership.com>
On Mon, Oct 17, 2011 at 10:26:42AM -0500, James Bottomley wrote:
> On Mon, 2011-10-17 at 15:15 +0000, KY Srinivasan wrote:
> > This patch is against Greg's staging tree where recently I merged all
> > of the storage
> > drivers into a single driver. This change may not have percolated up
> > yet - Greg has
> > checked this into his staging tree though.
>
> OK, that explains the difference.
>
> > As I deal with the review comments now, do you want me to get these
> > changes
> > applied first to Greg's tree before you would consider moving this
> > driver to drivers/scsi
> > directory, or could you move the driver as it is in Greg's tree under
> > staging and I could give
> > you patches against the moved driver.
>
> Either way is fine by me. The best route for this is to get the staging
> update into Linus head, then work on the actual SCSI problems. I'd like
> other SCSI people besides me to review it.
That will happen for the 3.2-rc1 release, so that might be the better
time to review this.
greg k-h
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: storvsc: Move the storage driver out of staging
From: KY Srinivasan @ 2011-10-17 23:00 UTC (permalink / raw)
To: James Bottomley
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, linux-scsi@vger.kernel.org, hch@infradead.org,
Haiyang Zhang
In-Reply-To: <1318865202.4794.21.camel@dabdike.int.hansenpartnership.com>
> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Monday, October 17, 2011 11:27 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> linux-scsi@vger.kernel.org; hch@infradead.org; Haiyang Zhang
> Subject: RE: [PATCH 1/1] Staging: hv: storvsc: Move the storage driver out of
> staging
>
> On Mon, 2011-10-17 at 15:15 +0000, KY Srinivasan wrote:
> > This patch is against Greg's staging tree where recently I merged all
> > of the storage
> > drivers into a single driver. This change may not have percolated up
> > yet - Greg has
> > checked this into his staging tree though.
>
> OK, that explains the difference.
>
> > As I deal with the review comments now, do you want me to get these
> > changes
> > applied first to Greg's tree before you would consider moving this
> > driver to drivers/scsi
> > directory, or could you move the driver as it is in Greg's tree under
> > staging and I could give
> > you patches against the moved driver.
>
> Either way is fine by me. The best route for this is to get the staging
> update into Linus head, then work on the actual SCSI problems. I'd like
> other SCSI people besides me to review it.
>
> > > OK, so as I read the code, what it can't handle is fragments except at
> > > the ends. As long as everything always transfers in multiples of 4k,
> > > the problem will never occur. This means that all devices you present
> > > need to tell the OS they have 4k sectors. I *think* you can do this by
> > > setting blk_limits_io_min() in the driver ... but you'll have to test
> > > this. The minimum sector size gets set also in sd.c when we probe the
> > > disk. I think that won't override blk_limits_io_min(), but please test
> > > (we don't have any SCSI drivers which have ever used this setting).
> > >
> > > The way to test, is to read back the /sys/block/<dev>/queue/ settings
> > > when presenting a 512 byte sector disk. (And the way to activate your
> > > bounce code would be to create a filesystem with a < PAGE_SIZE block
> > > size).
> >
> > Given that the bounce buffer handling code would typically
> > never be triggered, I not sure how useful it will be to try to get rid
> > of the bounce buffer
> > code using a feature that is not well tested.
>
> As opposed to bounce buffer code which hasn't been well tested either if
> it can't be triggered.
>
> > In any event, I will try your suggestion though.
>
> Just to be clear: Setting the minimum I/O size is completely well
> tested: it's how we handle 4k sector drives. What hasn't been tested is
> the ability of the *driver* to enforce the minimum using
> blk_limits_io_min(). As long at that doesn't get overridden by sd when
> it sees a 512b drive, the rest of the block paths (those which actually
> enforce the minimum) have all been well tested.
I will work on addressing the issues you have raised and any other comments I might
get from the community. I will submit these patches to Greg and I will copy you and other
scsi maintainers as well. Once Greg's staging tree is synced with Linus' tree, then I will submit
the code for further review in preparation for moving the code out of staging.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 1/1] Staging: hv: mousevsc: Move the mouse driver out of staging
From: Dmitry Torokhov @ 2011-10-23 7:24 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, linux-input,
Haiyang Zhang, Jiri Kosina
In-Reply-To: <1318658907-16698-1-git-send-email-kys@microsoft.com>
Hi K. Y.,
On Fri, Oct 14, 2011 at 11:08:27PM -0700, K. Y. Srinivasan wrote:
> In preparation for moving the mouse driver out of staging, seek community
> review of the mouse driver code.
>
Because it is a HID driver it should go through Jiri Kosina's tree
(CCed). Still, a few comments below.
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/hid/Kconfig | 6 +
> drivers/hid/Makefile | 1 +
> drivers/hid/hv_mouse.c | 599 +++++++++++++++++++++++++++++++++++++++++
> drivers/staging/hv/Kconfig | 6 -
> drivers/staging/hv/Makefile | 1 -
> drivers/staging/hv/hv_mouse.c | 599 -----------------------------------------
> 6 files changed, 606 insertions(+), 606 deletions(-)
> create mode 100644 drivers/hid/hv_mouse.c
> delete mode 100644 drivers/staging/hv/hv_mouse.c
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 1130a89..f8b98b8 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -613,6 +613,12 @@ config HID_ZYDACRON
> ---help---
> Support for Zydacron remote control.
>
> +config HYPERV_MOUSE
> + tristate "Microsoft Hyper-V mouse driver"
> + depends on HYPERV
> + help
> + Select this option to enable the Hyper-V mouse driver.
> +
> endmenu
>
> endif # HID_SUPPORT
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 0a0a38e..436ac2e 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_HID_ZYDACRON) += hid-zydacron.o
> obj-$(CONFIG_HID_WACOM) += hid-wacom.o
> obj-$(CONFIG_HID_WALTOP) += hid-waltop.o
> obj-$(CONFIG_HID_WIIMOTE) += hid-wiimote.o
> +obj-$(CONFIG_HYPERV_MOUSE) += hv_mouse.o
>
> obj-$(CONFIG_USB_HID) += usbhid/
> obj-$(CONFIG_USB_MOUSE) += usbhid/
> diff --git a/drivers/hid/hv_mouse.c b/drivers/hid/hv_mouse.c
> new file mode 100644
> index 0000000..ccd39c7
> --- /dev/null
> +++ b/drivers/hid/hv_mouse.c
> @@ -0,0 +1,599 @@
> +/*
> + * Copyright (c) 2009, Citrix Systems, Inc.
> + * Copyright (c) 2010, Microsoft Corporation.
> + * Copyright (c) 2011, Novell Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + */
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/workqueue.h>
Is this really needed?
> +#include <linux/sched.h>
You probably want completion.h instead.
> +#include <linux/wait.h>
> +#include <linux/input.h>
> +#include <linux/hid.h>
> +#include <linux/hiddev.h>
> +#include <linux/hyperv.h>
> +
> +
> +struct hv_input_dev_info {
> + unsigned int size;
> + unsigned short vendor;
> + unsigned short product;
> + unsigned short version;
> + unsigned short reserved[11];
> +};
> +
> +/* The maximum size of a synthetic input message. */
> +#define SYNTHHID_MAX_INPUT_REPORT_SIZE 16
> +
> +/*
> + * Current version
> + *
> + * History:
> + * Beta, RC < 2008/1/22 1,0
> + * RC > 2008/1/22 2,0
> + */
> +#define SYNTHHID_INPUT_VERSION_MAJOR 2
> +#define SYNTHHID_INPUT_VERSION_MINOR 0
> +#define SYNTHHID_INPUT_VERSION (SYNTHHID_INPUT_VERSION_MINOR | \
> + (SYNTHHID_INPUT_VERSION_MAJOR << 16))
> +
> +
> +#pragma pack(push, 1)
> +/*
> + * Message types in the synthetic input protocol
> + */
> +enum synthhid_msg_type {
> + SYNTH_HID_PROTOCOL_REQUEST,
> + SYNTH_HID_PROTOCOL_RESPONSE,
> + SYNTH_HID_INITIAL_DEVICE_INFO,
> + SYNTH_HID_INITIAL_DEVICE_INFO_ACK,
> + SYNTH_HID_INPUT_REPORT,
> + SYNTH_HID_MAX
> +};
> +
> +/*
> + * Basic message structures.
> + */
> +struct synthhid_msg_hdr {
> + enum synthhid_msg_type type;
> + u32 size;
> +};
> +
> +struct synthhid_msg {
> + struct synthhid_msg_hdr header;
> + char data[1]; /* Enclosed message */
> +};
> +
> +union synthhid_version {
> + struct {
> + u16 minor_version;
> + u16 major_version;
> + };
> + u32 version;
> +};
> +
> +/*
> + * Protocol messages
> + */
> +struct synthhid_protocol_request {
> + struct synthhid_msg_hdr header;
> + union synthhid_version version_requested;
> +};
> +
> +struct synthhid_protocol_response {
> + struct synthhid_msg_hdr header;
> + union synthhid_version version_requested;
> + unsigned char approved;
> +};
> +
> +struct synthhid_device_info {
> + struct synthhid_msg_hdr header;
> + struct hv_input_dev_info hid_dev_info;
> + struct hid_descriptor hid_descriptor;
> +};
> +
> +struct synthhid_device_info_ack {
> + struct synthhid_msg_hdr header;
> + unsigned char reserved;
> +};
> +
> +struct synthhid_input_report {
> + struct synthhid_msg_hdr header;
> + char buffer[1];
> +};
> +
> +#pragma pack(pop)
> +
> +#define INPUTVSC_SEND_RING_BUFFER_SIZE (10*PAGE_SIZE)
> +#define INPUTVSC_RECV_RING_BUFFER_SIZE (10*PAGE_SIZE)
> +
> +#define NBITS(x) (((x)/BITS_PER_LONG)+1)
> +
> +enum pipe_prot_msg_type {
> + PIPE_MESSAGE_INVALID,
> + PIPE_MESSAGE_DATA,
> + PIPE_MESSAGE_MAXIMUM
> +};
> +
> +
> +struct pipe_prt_msg {
> + enum pipe_prot_msg_type type;
> + u32 size;
> + char data[1];
> +};
> +
> +struct mousevsc_prt_msg {
> + enum pipe_prot_msg_type type;
> + u32 size;
> + union {
> + struct synthhid_protocol_request request;
> + struct synthhid_protocol_response response;
> + struct synthhid_device_info_ack ack;
> + };
> +};
> +
> +/*
> + * Represents an mousevsc device
> + */
> +struct mousevsc_dev {
> + struct hv_device *device;
> + unsigned char init_complete;
> + struct mousevsc_prt_msg protocol_req;
> + struct mousevsc_prt_msg protocol_resp;
> + /* Synchronize the request/response if needed */
> + struct completion wait_event;
> + int dev_info_status;
> +
> + struct hid_descriptor *hid_desc;
> + unsigned char *report_desc;
> + u32 report_desc_size;
> + struct hv_input_dev_info hid_dev_info;
> + int connected;
bool?
> + struct hid_device *hid_device;
> +};
> +
> +
> +static struct mousevsc_dev *alloc_input_device(struct hv_device *device)
> +{
> + struct mousevsc_dev *input_dev;
> +
> + input_dev = kzalloc(sizeof(struct mousevsc_dev), GFP_KERNEL);
> +
This blank line is extra.
> + if (!input_dev)
> + return NULL;
> +
> + input_dev->device = device;
> + hv_set_drvdata(device, input_dev);
> + init_completion(&input_dev->wait_event);
> +
> + return input_dev;
> +}
> +
> +static void free_input_device(struct mousevsc_dev *device)
> +{
> + kfree(device->hid_desc);
> + kfree(device->report_desc);
> + hv_set_drvdata(device->device, NULL);
> + kfree(device);
> +}
> +
> +
> +static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device,
> + struct synthhid_device_info *device_info)
> +{
> + int ret = 0;
> + struct hid_descriptor *desc;
> + struct mousevsc_prt_msg ack;
> +
> + /* Assume success for now */
> + input_device->dev_info_status = 0;
> +
> + memcpy(&input_device->hid_dev_info, &device_info->hid_dev_info,
> + sizeof(struct hv_input_dev_info));
> +
> + desc = &device_info->hid_descriptor;
> + WARN_ON(desc->bLength == 0);
> +
> + input_device->hid_desc = kzalloc(desc->bLength, GFP_ATOMIC);
> +
> + if (!input_device->hid_desc)
> + goto cleanup;
> +
> + memcpy(input_device->hid_desc, desc, desc->bLength);
> +
> + input_device->report_desc_size = desc->desc[0].wDescriptorLength;
> + if (input_device->report_desc_size == 0)
> + goto cleanup;
> + input_device->report_desc = kzalloc(input_device->report_desc_size,
> + GFP_ATOMIC);
> +
> + if (!input_device->report_desc)
> + goto cleanup;
> +
> + memcpy(input_device->report_desc,
> + ((unsigned char *)desc) + desc->bLength,
> + desc->desc[0].wDescriptorLength);
> +
> + /* Send the ack */
> + memset(&ack, 0, sizeof(struct mousevsc_prt_msg));
> +
> + ack.type = PIPE_MESSAGE_DATA;
> + ack.size = sizeof(struct synthhid_device_info_ack);
> +
> + ack.ack.header.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK;
> + ack.ack.header.size = 1;
> + ack.ack.reserved = 0;
That looks like a constant structure...
> +
> + ret = vmbus_sendpacket(input_device->device->channel,
> + &ack,
> + sizeof(struct pipe_prt_msg) - sizeof(unsigned char) +
> + sizeof(struct synthhid_device_info_ack),
> + (unsigned long)&ack,
> + VM_PKT_DATA_INBAND,
> + VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> + if (ret != 0)
> + goto cleanup;
> +
> + complete(&input_device->wait_event);
> +
> + return;
> +
> +cleanup:
> + kfree(input_device->hid_desc);
> + input_device->hid_desc = NULL;
> +
> + kfree(input_device->report_desc);
> + input_device->report_desc = NULL;
> +
> + input_device->dev_info_status = -1;
> + complete(&input_device->wait_event);
> +}
> +
> +static void mousevsc_on_receive(struct hv_device *device,
> + struct vmpacket_descriptor *packet)
> +{
> + struct pipe_prt_msg *pipe_msg;
> + struct synthhid_msg *hid_msg;
> + struct mousevsc_dev *input_dev = hv_get_drvdata(device);
> + struct synthhid_input_report *input_report;
> +
> + pipe_msg = (struct pipe_prt_msg *)((unsigned long)packet +
> + (packet->offset8 << 3));
> +
> + if (pipe_msg->type != PIPE_MESSAGE_DATA)
> + return;
> +
> + hid_msg = (struct synthhid_msg *)&pipe_msg->data[0];
> +
> + switch (hid_msg->header.type) {
> + case SYNTH_HID_PROTOCOL_RESPONSE:
> + memcpy(&input_dev->protocol_resp, pipe_msg,
> + pipe_msg->size + sizeof(struct pipe_prt_msg) -
> + sizeof(unsigned char));
> + complete(&input_dev->wait_event);
> + break;
> +
> + case SYNTH_HID_INITIAL_DEVICE_INFO:
> + WARN_ON(pipe_msg->size < sizeof(struct hv_input_dev_info));
> +
> + /*
> + * Parse out the device info into device attr,
> + * hid desc and report desc
> + */
> + mousevsc_on_receive_device_info(input_dev,
> + (struct synthhid_device_info *)&pipe_msg->data[0]);
> + break;
> + case SYNTH_HID_INPUT_REPORT:
> + input_report =
> + (struct synthhid_input_report *)&pipe_msg->data[0];
> + if (!input_dev->init_complete)
> + break;
> + hid_input_report(input_dev->hid_device,
> + HID_INPUT_REPORT, input_report->buffer,
> + input_report->header.size, 1);
> + break;
> + default:
> + pr_err("unsupported hid msg type - type %d len %d",
> + hid_msg->header.type, hid_msg->header.size);
> + break;
> + }
> +
> +}
> +
> +static void mousevsc_on_channel_callback(void *context)
> +{
> + const int packetSize = 0x100;
> + int ret = 0;
> + struct hv_device *device = (struct hv_device *)context;
No need to cast.
> +
> + u32 bytes_recvd;
> + u64 req_id;
> + unsigned char packet[0x100];
Hmm, 100 bytes on stack... and are we in interrupt context by any
chance?
> + struct vmpacket_descriptor *desc;
> + unsigned char *buffer = packet;
> + int bufferlen = packetSize;
> +
> +
> + do {
> + ret = vmbus_recvpacket_raw(device->channel, buffer,
> + bufferlen, &bytes_recvd, &req_id);
> +
> + if (ret == 0) {
> + if (bytes_recvd > 0) {
> + desc = (struct vmpacket_descriptor *)buffer;
> +
> + switch (desc->type) {
> + case VM_PKT_COMP:
> + break;
> +
> + case VM_PKT_DATA_INBAND:
> + mousevsc_on_receive(
> + device, desc);
> + break;
> +
> + default:
> + pr_err("unhandled packet type %d, tid %llx len %d\n",
> + desc->type,
> + req_id,
> + bytes_recvd);
> + break;
> + }
> +
> + /* reset */
> + if (bufferlen > packetSize) {
> + kfree(buffer);
> +
> + buffer = packet;
> + bufferlen = packetSize;
> + }
> + } else {
> + if (bufferlen > packetSize) {
> + kfree(buffer);
> +
> + buffer = packet;
> + bufferlen = packetSize;
> + }
> + break;
> + }
> + } else if (ret == -ENOBUFS) {
> + /* Handle large packet */
> + bufferlen = bytes_recvd;
> + buffer = kzalloc(bytes_recvd, GFP_ATOMIC);
> +
What happens if we receive even larger amount of data after allocating
the buffer?
> + if (buffer == NULL) {
> + buffer = packet;
> + bufferlen = packetSize;
> + break;
> + }
> + }
> + } while (1);
So we can be looping indefinitely here as long as other side keeps
sending data?
> +
> + return;
No naked returns please.
> +}
> +
> +static int mousevsc_connect_to_vsp(struct hv_device *device)
> +{
> + int ret = 0;
> + int t;
> + struct mousevsc_dev *input_dev = hv_get_drvdata(device);
> + struct mousevsc_prt_msg *request;
> + struct mousevsc_prt_msg *response;
> +
> +
> + request = &input_dev->protocol_req;
> +
> + memset(request, 0, sizeof(struct mousevsc_prt_msg));
> +
> + request->type = PIPE_MESSAGE_DATA;
> + request->size = sizeof(struct synthhid_protocol_request);
> +
> + request->request.header.type = SYNTH_HID_PROTOCOL_REQUEST;
> + request->request.header.size = sizeof(unsigned int);
> + request->request.version_requested.version = SYNTHHID_INPUT_VERSION;
> +
> +
> + ret = vmbus_sendpacket(device->channel, request,
> + sizeof(struct pipe_prt_msg) -
> + sizeof(unsigned char) +
> + sizeof(struct synthhid_protocol_request),
> + (unsigned long)request,
> + VM_PKT_DATA_INBAND,
> + VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> + if (ret != 0)
> + goto cleanup;
> +
> + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> + if (t == 0) {
> + ret = -ETIMEDOUT;
> + goto cleanup;
> + }
> +
> + response = &input_dev->protocol_resp;
> +
> + if (!response->response.approved) {
> + pr_err("synthhid protocol request failed (version %d)",
> + SYNTHHID_INPUT_VERSION);
> + ret = -ENODEV;
> + goto cleanup;
> + }
> +
> + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
We just completed the wait for this completion, why are we waiting on
the same completion again?
> + if (t == 0) {
> + ret = -ETIMEDOUT;
> + goto cleanup;
> + }
> +
> + /*
> + * We should have gotten the device attr, hid desc and report
> + * desc at this point
> + */
> + if (input_dev->dev_info_status)
> + ret = -ENOMEM;
-ENOMEM seems wrong.
> +
> +cleanup:
> +
> + return ret;
> +}
> +
> +static int mousevsc_hid_open(struct hid_device *hid)
> +{
> + return 0;
> +}
> +
> +static void mousevsc_hid_close(struct hid_device *hid)
> +{
> +}
> +
> +static struct hid_ll_driver mousevsc_ll_driver = {
> + .open = mousevsc_hid_open,
> + .close = mousevsc_hid_close,
> +};
> +
> +static struct hid_driver mousevsc_hid_driver;
> +
> +static void reportdesc_callback(struct hv_device *dev, void *packet, u32 len)
> +{
This is called from mousevsc_on_device_add() which is part of device
probe. Why it is split into separate function with bizzare arguments is
beyond me.
> + struct hid_device *hid_dev;
> + struct mousevsc_dev *input_device = hv_get_drvdata(dev);
> +
> + hid_dev = hid_allocate_device();
> + if (IS_ERR(hid_dev))
> + return;
This is not very helpful.
> +
> + hid_dev->ll_driver = &mousevsc_ll_driver;
> + hid_dev->driver = &mousevsc_hid_driver;
> +
> + if (hid_parse_report(hid_dev, packet, len))
> + return;
Neither is this.
> +
> + hid_dev->bus = BUS_VIRTUAL;
> + hid_dev->vendor = input_device->hid_dev_info.vendor;
> + hid_dev->product = input_device->hid_dev_info.product;
> + hid_dev->version = input_device->hid_dev_info.version;
> +
> + sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
> +
> + if (!hidinput_connect(hid_dev, 0)) {
> + hid_dev->claimed |= HID_CLAIMED_INPUT;
Why do you force hidinput claim? Let the normal rules do it.
> +
> + input_device->connected = 1;
input_device->connected = true;
> +
> + }
> +
> + input_device->hid_device = hid_dev;
This assignment is probably too late.
> +}
> +
> +static int mousevsc_on_device_add(struct hv_device *device)
The only caller of this is mousevsc_probe() so why do you have it>
> +{
> + int ret = 0;
> + struct mousevsc_dev *input_dev;
> +
> + input_dev = alloc_input_device(device);
> +
> + if (!input_dev)
> + return -ENOMEM;
> +
> + input_dev->init_complete = false;
> +
> + ret = vmbus_open(device->channel,
> + INPUTVSC_SEND_RING_BUFFER_SIZE,
> + INPUTVSC_RECV_RING_BUFFER_SIZE,
> + NULL,
> + 0,
> + mousevsc_on_channel_callback,
> + device
> + );
> +
> + if (ret != 0) {
> + free_input_device(input_dev);
> + return ret;
> + }
> +
> +
> + ret = mousevsc_connect_to_vsp(device);
> +
> + if (ret != 0) {
> + vmbus_close(device->channel);
> + free_input_device(input_dev);
> + return ret;
> + }
> +
> +
> + /* workaround SA-167 */
> + if (input_dev->report_desc[14] == 0x25)
> + input_dev->report_desc[14] = 0x29;
> +
> + reportdesc_callback(device, input_dev->report_desc,
> + input_dev->report_desc_size);
> +
> + input_dev->init_complete = true;
> +
> + return ret;
> +}
> +
> +static int mousevsc_probe(struct hv_device *dev,
> + const struct hv_vmbus_device_id *dev_id)
> +{
> +
> + return mousevsc_on_device_add(dev);
> +
> +}
> +
> +static int mousevsc_remove(struct hv_device *dev)
> +{
> + struct mousevsc_dev *input_dev = hv_get_drvdata(dev);
> +
> + vmbus_close(dev->channel);
> +
> + if (input_dev->connected) {
> + hidinput_disconnect(input_dev->hid_device);
> + input_dev->connected = 0;
> + hid_destroy_device(input_dev->hid_device);
hid_destroy_device() should disconnect registered handlers for you; you
do not need to do that manually.
> + }
> +
> + free_input_device(input_dev);
Calling it input device is terribly confusing to me. I'd also freed it
inline (and used gotos to unwind errors in probe()).
> +
> + return 0;
> +}
> +
> +static const struct hv_vmbus_device_id id_table[] = {
> + /* Mouse guid */
> + { VMBUS_DEVICE(0x9E, 0xB6, 0xA8, 0xCF, 0x4A, 0x5B, 0xc0, 0x4c,
> + 0xB9, 0x8B, 0x8B, 0xA1, 0xA1, 0xF3, 0xF9, 0x5A) },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(vmbus, id_table);
> +
> +static struct hv_driver mousevsc_drv = {
> + .name = "mousevsc",
> + .id_table = id_table,
> + .probe = mousevsc_probe,
> + .remove = mousevsc_remove,
> +};
> +
> +static int __init mousevsc_init(void)
> +{
> + return vmbus_driver_register(&mousevsc_drv);
> +}
> +
> +static void __exit mousevsc_exit(void)
> +{
> + vmbus_driver_unregister(&mousevsc_drv);
> +}
> +
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(HV_DRV_VERSION);
> +module_init(mousevsc_init);
> +module_exit(mousevsc_exit);
Thanks.
--
Dmitry
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: mousevsc: Move the mouse driver out of staging
From: KY Srinivasan @ 2011-10-23 14:33 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jiri Kosina, Haiyang Zhang, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org,
linux-input@vger.kernel.org, devel@linuxdriverproject.org
In-Reply-To: <20111023072427.GA13268@core.coreip.homeip.net>
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Sunday, October 23, 2011 3:24 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; linux-
> input@vger.kernel.org; Haiyang Zhang; Jiri Kosina
> Subject: Re: [PATCH 1/1] Staging: hv: mousevsc: Move the mouse driver out of
> staging
>
> Hi K. Y.,
>
> On Fri, Oct 14, 2011 at 11:08:27PM -0700, K. Y. Srinivasan wrote:
> > In preparation for moving the mouse driver out of staging, seek community
> > review of the mouse driver code.
> >
>
> Because it is a HID driver it should go through Jiri Kosina's tree
> (CCed). Still, a few comments below.
Thanks Dmitry. I will address the comments you have given me, and will
work with Jiri to get this driver out of staging.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: mousevsc: Move the mouse driver out of staging
From: KY Srinivasan @ 2011-10-23 15:45 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
linux-input@vger.kernel.org, Haiyang Zhang, Jiri Kosina
In-Reply-To: <20111023072427.GA13268@core.coreip.homeip.net>
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Sunday, October 23, 2011 3:24 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; linux-
> input@vger.kernel.org; Haiyang Zhang; Jiri Kosina
> Subject: Re: [PATCH 1/1] Staging: hv: mousevsc: Move the mouse driver out of
> staging
>
> Hi K. Y.,
>
> On Fri, Oct 14, 2011 at 11:08:27PM -0700, K. Y. Srinivasan wrote:
> > In preparation for moving the mouse driver out of staging, seek community
> > review of the mouse driver code.
> >
>
> Because it is a HID driver it should go through Jiri Kosina's tree
> (CCed). Still, a few comments below.
>
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> > drivers/hid/Kconfig | 6 +
> > drivers/hid/Makefile | 1 +
> > drivers/hid/hv_mouse.c | 599
> +++++++++++++++++++++++++++++++++++++++++
> > drivers/staging/hv/Kconfig | 6 -
> > drivers/staging/hv/Makefile | 1 -
> > drivers/staging/hv/hv_mouse.c | 599 -----------------------------------------
> > 6 files changed, 606 insertions(+), 606 deletions(-)
> > create mode 100644 drivers/hid/hv_mouse.c
> > delete mode 100644 drivers/staging/hv/hv_mouse.c
> >
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 1130a89..f8b98b8 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -613,6 +613,12 @@ config HID_ZYDACRON
> > ---help---
> > Support for Zydacron remote control.
> >
> > +config HYPERV_MOUSE
> > + tristate "Microsoft Hyper-V mouse driver"
> > + depends on HYPERV
> > + help
> > + Select this option to enable the Hyper-V mouse driver.
> > +
> > endmenu
> >
> > endif # HID_SUPPORT
> > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > index 0a0a38e..436ac2e 100644
> > --- a/drivers/hid/Makefile
> > +++ b/drivers/hid/Makefile
> > @@ -76,6 +76,7 @@ obj-$(CONFIG_HID_ZYDACRON) += hid-zydacron.o
> > obj-$(CONFIG_HID_WACOM) += hid-wacom.o
> > obj-$(CONFIG_HID_WALTOP) += hid-waltop.o
> > obj-$(CONFIG_HID_WIIMOTE) += hid-wiimote.o
> > +obj-$(CONFIG_HYPERV_MOUSE) += hv_mouse.o
> >
> > obj-$(CONFIG_USB_HID) += usbhid/
> > obj-$(CONFIG_USB_MOUSE) += usbhid/
> > diff --git a/drivers/hid/hv_mouse.c b/drivers/hid/hv_mouse.c
> > new file mode 100644
> > index 0000000..ccd39c7
> > --- /dev/null
> > +++ b/drivers/hid/hv_mouse.c
> > @@ -0,0 +1,599 @@
> > +/*
> > + * Copyright (c) 2009, Citrix Systems, Inc.
> > + * Copyright (c) 2010, Microsoft Corporation.
> > + * Copyright (c) 2011, Novell Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> or
> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> for
> > + * more details.
> > + */
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/workqueue.h>
>
> Is this really needed?
>
> > +#include <linux/sched.h>
>
> You probably want completion.h instead.
I will fix this.
>
> > +#include <linux/wait.h>
> > +#include <linux/input.h>
> > +#include <linux/hid.h>
> > +#include <linux/hiddev.h>
> > +#include <linux/hyperv.h>
> > +
> > +
> > +struct hv_input_dev_info {
> > + unsigned int size;
> > + unsigned short vendor;
> > + unsigned short product;
> > + unsigned short version;
> > + unsigned short reserved[11];
> > +};
> > +
> > +/* The maximum size of a synthetic input message. */
> > +#define SYNTHHID_MAX_INPUT_REPORT_SIZE 16
> > +
> > +/*
> > + * Current version
> > + *
> > + * History:
> > + * Beta, RC < 2008/1/22 1,0
> > + * RC > 2008/1/22 2,0
> > + */
> > +#define SYNTHHID_INPUT_VERSION_MAJOR 2
> > +#define SYNTHHID_INPUT_VERSION_MINOR 0
> > +#define SYNTHHID_INPUT_VERSION
> (SYNTHHID_INPUT_VERSION_MINOR | \
> > + (SYNTHHID_INPUT_VERSION_MAJOR <<
> 16))
> > +
> > +
> > +#pragma pack(push, 1)
> > +/*
> > + * Message types in the synthetic input protocol
> > + */
> > +enum synthhid_msg_type {
> > + SYNTH_HID_PROTOCOL_REQUEST,
> > + SYNTH_HID_PROTOCOL_RESPONSE,
> > + SYNTH_HID_INITIAL_DEVICE_INFO,
> > + SYNTH_HID_INITIAL_DEVICE_INFO_ACK,
> > + SYNTH_HID_INPUT_REPORT,
> > + SYNTH_HID_MAX
> > +};
> > +
> > +/*
> > + * Basic message structures.
> > + */
> > +struct synthhid_msg_hdr {
> > + enum synthhid_msg_type type;
> > + u32 size;
> > +};
> > +
> > +struct synthhid_msg {
> > + struct synthhid_msg_hdr header;
> > + char data[1]; /* Enclosed message */
> > +};
> > +
> > +union synthhid_version {
> > + struct {
> > + u16 minor_version;
> > + u16 major_version;
> > + };
> > + u32 version;
> > +};
> > +
> > +/*
> > + * Protocol messages
> > + */
> > +struct synthhid_protocol_request {
> > + struct synthhid_msg_hdr header;
> > + union synthhid_version version_requested;
> > +};
> > +
> > +struct synthhid_protocol_response {
> > + struct synthhid_msg_hdr header;
> > + union synthhid_version version_requested;
> > + unsigned char approved;
> > +};
> > +
> > +struct synthhid_device_info {
> > + struct synthhid_msg_hdr header;
> > + struct hv_input_dev_info hid_dev_info;
> > + struct hid_descriptor hid_descriptor;
> > +};
> > +
> > +struct synthhid_device_info_ack {
> > + struct synthhid_msg_hdr header;
> > + unsigned char reserved;
> > +};
> > +
> > +struct synthhid_input_report {
> > + struct synthhid_msg_hdr header;
> > + char buffer[1];
> > +};
> > +
> > +#pragma pack(pop)
> > +
> > +#define INPUTVSC_SEND_RING_BUFFER_SIZE (10*PAGE_SIZE)
> > +#define INPUTVSC_RECV_RING_BUFFER_SIZE (10*PAGE_SIZE)
> > +
> > +#define NBITS(x) (((x)/BITS_PER_LONG)+1)
> > +
> > +enum pipe_prot_msg_type {
> > + PIPE_MESSAGE_INVALID,
> > + PIPE_MESSAGE_DATA,
> > + PIPE_MESSAGE_MAXIMUM
> > +};
> > +
> > +
> > +struct pipe_prt_msg {
> > + enum pipe_prot_msg_type type;
> > + u32 size;
> > + char data[1];
> > +};
> > +
> > +struct mousevsc_prt_msg {
> > + enum pipe_prot_msg_type type;
> > + u32 size;
> > + union {
> > + struct synthhid_protocol_request request;
> > + struct synthhid_protocol_response response;
> > + struct synthhid_device_info_ack ack;
> > + };
> > +};
> > +
> > +/*
> > + * Represents an mousevsc device
> > + */
> > +struct mousevsc_dev {
> > + struct hv_device *device;
> > + unsigned char init_complete;
> > + struct mousevsc_prt_msg protocol_req;
> > + struct mousevsc_prt_msg protocol_resp;
> > + /* Synchronize the request/response if needed */
> > + struct completion wait_event;
> > + int dev_info_status;
> > +
> > + struct hid_descriptor *hid_desc;
> > + unsigned char *report_desc;
> > + u32 report_desc_size;
> > + struct hv_input_dev_info hid_dev_info;
> > + int connected;
>
> bool?
>
> > + struct hid_device *hid_device;
> > +};
> > +
> > +
> > +static struct mousevsc_dev *alloc_input_device(struct hv_device *device)
> > +{
> > + struct mousevsc_dev *input_dev;
> > +
> > + input_dev = kzalloc(sizeof(struct mousevsc_dev), GFP_KERNEL);
> > +
>
> This blank line is extra.
>
> > + if (!input_dev)
> > + return NULL;
> > +
> > + input_dev->device = device;
> > + hv_set_drvdata(device, input_dev);
> > + init_completion(&input_dev->wait_event);
> > +
> > + return input_dev;
> > +}
> > +
> > +static void free_input_device(struct mousevsc_dev *device)
> > +{
> > + kfree(device->hid_desc);
> > + kfree(device->report_desc);
> > + hv_set_drvdata(device->device, NULL);
> > + kfree(device);
> > +}
> > +
> > +
> > +static void mousevsc_on_receive_device_info(struct mousevsc_dev
> *input_device,
> > + struct synthhid_device_info *device_info)
> > +{
> > + int ret = 0;
> > + struct hid_descriptor *desc;
> > + struct mousevsc_prt_msg ack;
> > +
> > + /* Assume success for now */
> > + input_device->dev_info_status = 0;
> > +
> > + memcpy(&input_device->hid_dev_info, &device_info->hid_dev_info,
> > + sizeof(struct hv_input_dev_info));
> > +
> > + desc = &device_info->hid_descriptor;
> > + WARN_ON(desc->bLength == 0);
> > +
> > + input_device->hid_desc = kzalloc(desc->bLength, GFP_ATOMIC);
> > +
> > + if (!input_device->hid_desc)
> > + goto cleanup;
> > +
> > + memcpy(input_device->hid_desc, desc, desc->bLength);
> > +
> > + input_device->report_desc_size = desc->desc[0].wDescriptorLength;
> > + if (input_device->report_desc_size == 0)
> > + goto cleanup;
> > + input_device->report_desc = kzalloc(input_device->report_desc_size,
> > + GFP_ATOMIC);
> > +
> > + if (!input_device->report_desc)
> > + goto cleanup;
> > +
> > + memcpy(input_device->report_desc,
> > + ((unsigned char *)desc) + desc->bLength,
> > + desc->desc[0].wDescriptorLength);
> > +
> > + /* Send the ack */
> > + memset(&ack, 0, sizeof(struct mousevsc_prt_msg));
> > +
> > + ack.type = PIPE_MESSAGE_DATA;
> > + ack.size = sizeof(struct synthhid_device_info_ack);
> > +
> > + ack.ack.header.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK;
> > + ack.ack.header.size = 1;
> > + ack.ack.reserved = 0;
>
> That looks like a constant structure...
Will clean this up.
>
> > +
> > + ret = vmbus_sendpacket(input_device->device->channel,
> > + &ack,
> > + sizeof(struct pipe_prt_msg) - sizeof(unsigned char) +
> > + sizeof(struct synthhid_device_info_ack),
> > + (unsigned long)&ack,
> > + VM_PKT_DATA_INBAND,
> > +
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > + if (ret != 0)
> > + goto cleanup;
> > +
> > + complete(&input_device->wait_event);
> > +
> > + return;
> > +
> > +cleanup:
> > + kfree(input_device->hid_desc);
> > + input_device->hid_desc = NULL;
> > +
> > + kfree(input_device->report_desc);
> > + input_device->report_desc = NULL;
> > +
> > + input_device->dev_info_status = -1;
> > + complete(&input_device->wait_event);
> > +}
> > +
> > +static void mousevsc_on_receive(struct hv_device *device,
> > + struct vmpacket_descriptor *packet)
> > +{
> > + struct pipe_prt_msg *pipe_msg;
> > + struct synthhid_msg *hid_msg;
> > + struct mousevsc_dev *input_dev = hv_get_drvdata(device);
> > + struct synthhid_input_report *input_report;
> > +
> > + pipe_msg = (struct pipe_prt_msg *)((unsigned long)packet +
> > + (packet->offset8 << 3));
> > +
> > + if (pipe_msg->type != PIPE_MESSAGE_DATA)
> > + return;
> > +
> > + hid_msg = (struct synthhid_msg *)&pipe_msg->data[0];
> > +
> > + switch (hid_msg->header.type) {
> > + case SYNTH_HID_PROTOCOL_RESPONSE:
> > + memcpy(&input_dev->protocol_resp, pipe_msg,
> > + pipe_msg->size + sizeof(struct pipe_prt_msg) -
> > + sizeof(unsigned char));
> > + complete(&input_dev->wait_event);
> > + break;
> > +
> > + case SYNTH_HID_INITIAL_DEVICE_INFO:
> > + WARN_ON(pipe_msg->size < sizeof(struct hv_input_dev_info));
> > +
> > + /*
> > + * Parse out the device info into device attr,
> > + * hid desc and report desc
> > + */
> > + mousevsc_on_receive_device_info(input_dev,
> > + (struct synthhid_device_info *)&pipe_msg->data[0]);
> > + break;
> > + case SYNTH_HID_INPUT_REPORT:
> > + input_report =
> > + (struct synthhid_input_report *)&pipe_msg->data[0];
> > + if (!input_dev->init_complete)
> > + break;
> > + hid_input_report(input_dev->hid_device,
> > + HID_INPUT_REPORT, input_report->buffer,
> > + input_report->header.size, 1);
> > + break;
> > + default:
> > + pr_err("unsupported hid msg type - type %d len %d",
> > + hid_msg->header.type, hid_msg->header.size);
> > + break;
> > + }
> > +
> > +}
> > +
> > +static void mousevsc_on_channel_callback(void *context)
> > +{
> > + const int packetSize = 0x100;
> > + int ret = 0;
> > + struct hv_device *device = (struct hv_device *)context;
>
> No need to cast.
>
> > +
> > + u32 bytes_recvd;
> > + u64 req_id;
> > + unsigned char packet[0x100];
>
> Hmm, 100 bytes on stack... and are we in interrupt context by any
> chance?
>
> > + struct vmpacket_descriptor *desc;
> > + unsigned char *buffer = packet;
> > + int bufferlen = packetSize;
> > +
> > +
> > + do {
> > + ret = vmbus_recvpacket_raw(device->channel, buffer,
> > + bufferlen, &bytes_recvd, &req_id);
> > +
> > + if (ret == 0) {
> > + if (bytes_recvd > 0) {
> > + desc = (struct vmpacket_descriptor *)buffer;
> > +
> > + switch (desc->type) {
> > + case VM_PKT_COMP:
> > + break;
> > +
> > + case VM_PKT_DATA_INBAND:
> > + mousevsc_on_receive(
> > + device, desc);
> > + break;
> > +
> > + default:
> > + pr_err("unhandled packet type %d, tid
> %llx len %d\n",
> > + desc->type,
> > + req_id,
> > + bytes_recvd);
> > + break;
> > + }
> > +
> > + /* reset */
> > + if (bufferlen > packetSize) {
> > + kfree(buffer);
> > +
> > + buffer = packet;
> > + bufferlen = packetSize;
> > + }
> > + } else {
> > + if (bufferlen > packetSize) {
> > + kfree(buffer);
> > +
> > + buffer = packet;
> > + bufferlen = packetSize;
> > + }
> > + break;
> > + }
> > + } else if (ret == -ENOBUFS) {
> > + /* Handle large packet */
> > + bufferlen = bytes_recvd;
> > + buffer = kzalloc(bytes_recvd, GFP_ATOMIC);
> > +
>
> What happens if we receive even larger amount of data after allocating
> the buffer?
I will cleanup this function.
>
> > + if (buffer == NULL) {
> > + buffer = packet;
> > + bufferlen = packetSize;
> > + break;
> > + }
> > + }
> > + } while (1);
>
> So we can be looping indefinitely here as long as other side keeps
> sending data?
The current protocol requires that the consumer handle all available data on the
channel before exiting the handler; this is used to optimize signaling on the ring
buffer between the producer and the consumer.
>
> > +
> > + return;
>
> No naked returns please.
>
> > +}
> > +
> > +static int mousevsc_connect_to_vsp(struct hv_device *device)
> > +{
> > + int ret = 0;
> > + int t;
> > + struct mousevsc_dev *input_dev = hv_get_drvdata(device);
> > + struct mousevsc_prt_msg *request;
> > + struct mousevsc_prt_msg *response;
> > +
> > +
> > + request = &input_dev->protocol_req;
> > +
> > + memset(request, 0, sizeof(struct mousevsc_prt_msg));
> > +
> > + request->type = PIPE_MESSAGE_DATA;
> > + request->size = sizeof(struct synthhid_protocol_request);
> > +
> > + request->request.header.type = SYNTH_HID_PROTOCOL_REQUEST;
> > + request->request.header.size = sizeof(unsigned int);
> > + request->request.version_requested.version =
> SYNTHHID_INPUT_VERSION;
> > +
> > +
> > + ret = vmbus_sendpacket(device->channel, request,
> > + sizeof(struct pipe_prt_msg) -
> > + sizeof(unsigned char) +
> > + sizeof(struct synthhid_protocol_request),
> > + (unsigned long)request,
> > + VM_PKT_DATA_INBAND,
> > +
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > + if (ret != 0)
> > + goto cleanup;
> > +
> > + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> > + if (t == 0) {
> > + ret = -ETIMEDOUT;
> > + goto cleanup;
> > + }
> > +
> > + response = &input_dev->protocol_resp;
> > +
> > + if (!response->response.approved) {
> > + pr_err("synthhid protocol request failed (version %d)",
> > + SYNTHHID_INPUT_VERSION);
> > + ret = -ENODEV;
> > + goto cleanup;
> > + }
> > +
> > + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
>
> We just completed the wait for this completion, why are we waiting on
> the same completion again?
In response to our initial query, we expect the host to respond back with two
distinct pieces of information; we wait for both these responses.
>
> > + if (t == 0) {
> > + ret = -ETIMEDOUT;
> > + goto cleanup;
> > + }
> > +
> > + /*
> > + * We should have gotten the device attr, hid desc and report
> > + * desc at this point
> > + */
> > + if (input_dev->dev_info_status)
> > + ret = -ENOMEM;
>
> -ENOMEM seems wrong.
>
There are many failures here and not being able to allocate memory is the
primary one; and so I chose to capture that.
> > +
> > +cleanup:
> > +
> > + return ret;
> > +}
> > +
> > +static int mousevsc_hid_open(struct hid_device *hid)
> > +{
> > + return 0;
> > +}
> > +
> > +static void mousevsc_hid_close(struct hid_device *hid)
> > +{
> > +}
> > +
> > +static struct hid_ll_driver mousevsc_ll_driver = {
> > + .open = mousevsc_hid_open,
> > + .close = mousevsc_hid_close,
> > +};
> > +
> > +static struct hid_driver mousevsc_hid_driver;
> > +
> > +static void reportdesc_callback(struct hv_device *dev, void *packet, u32 len)
> > +{
>
> This is called from mousevsc_on_device_add() which is part of device
> probe. Why it is split into separate function with bizzare arguments is
> beyond me.
I will clean this up.
>
> > + struct hid_device *hid_dev;
> > + struct mousevsc_dev *input_device = hv_get_drvdata(dev);
> > +
> > + hid_dev = hid_allocate_device();
> > + if (IS_ERR(hid_dev))
> > + return;
>
> This is not very helpful.
I will clean this up.
>
> > +
> > + hid_dev->ll_driver = &mousevsc_ll_driver;
> > + hid_dev->driver = &mousevsc_hid_driver;
> > +
> > + if (hid_parse_report(hid_dev, packet, len))
> > + return;
>
> Neither is this.
>
> > +
> > + hid_dev->bus = BUS_VIRTUAL;
> > + hid_dev->vendor = input_device->hid_dev_info.vendor;
> > + hid_dev->product = input_device->hid_dev_info.product;
> > + hid_dev->version = input_device->hid_dev_info.version;
> > +
> > + sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
> > +
> > + if (!hidinput_connect(hid_dev, 0)) {
> > + hid_dev->claimed |= HID_CLAIMED_INPUT;
>
> Why do you force hidinput claim? Let the normal rules do it.
>
> > +
> > + input_device->connected = 1;
>
> input_device->connected = true;
>
> > +
> > + }
> > +
> > + input_device->hid_device = hid_dev;
>
> This assignment is probably too late.
I will address this.
>
> > +}
> > +
> > +static int mousevsc_on_device_add(struct hv_device *device)
>
> The only caller of this is mousevsc_probe() so why do you have it
I will address this.
>
> > +{
> > + int ret = 0;
> > + struct mousevsc_dev *input_dev;
> > +
> > + input_dev = alloc_input_device(device);
> > +
> > + if (!input_dev)
> > + return -ENOMEM;
> > +
> > + input_dev->init_complete = false;
> > +
> > + ret = vmbus_open(device->channel,
> > + INPUTVSC_SEND_RING_BUFFER_SIZE,
> > + INPUTVSC_RECV_RING_BUFFER_SIZE,
> > + NULL,
> > + 0,
> > + mousevsc_on_channel_callback,
> > + device
> > + );
> > +
> > + if (ret != 0) {
> > + free_input_device(input_dev);
> > + return ret;
> > + }
> > +
> > +
> > + ret = mousevsc_connect_to_vsp(device);
> > +
> > + if (ret != 0) {
> > + vmbus_close(device->channel);
> > + free_input_device(input_dev);
> > + return ret;
> > + }
> > +
> > +
> > + /* workaround SA-167 */
> > + if (input_dev->report_desc[14] == 0x25)
> > + input_dev->report_desc[14] = 0x29;
> > +
> > + reportdesc_callback(device, input_dev->report_desc,
> > + input_dev->report_desc_size);
> > +
> > + input_dev->init_complete = true;
> > +
> > + return ret;
> > +}
> > +
> > +static int mousevsc_probe(struct hv_device *dev,
> > + const struct hv_vmbus_device_id *dev_id)
> > +{
> > +
> > + return mousevsc_on_device_add(dev);
> > +
> > +}
> > +
> > +static int mousevsc_remove(struct hv_device *dev)
> > +{
> > + struct mousevsc_dev *input_dev = hv_get_drvdata(dev);
> > +
> > + vmbus_close(dev->channel);
> > +
> > + if (input_dev->connected) {
> > + hidinput_disconnect(input_dev->hid_device);
> > + input_dev->connected = 0;
> > + hid_destroy_device(input_dev->hid_device);
>
> hid_destroy_device() should disconnect registered handlers for you; you
> do not need to do that manually.
>
> > + }
> > +
> > + free_input_device(input_dev);
>
> Calling it input device is terribly confusing to me. I'd also freed it
> inline (and used gotos to unwind errors in probe()).
>
I will clean this up.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH RFC V2 4/5] kvm guest : Added configuration support to enable debug information for KVM Guests
From: Sasha Levin @ 2011-10-24 10:01 UTC (permalink / raw)
To: Raghavendra K T
Cc: Peter Zijlstra, Virtualization, H. Peter Anvin,
Jeremy Fitzhardinge, Dave Jiang, KVM, x86, Ingo Molnar,
Avi Kivity, Rik van Riel, Stefano Stabellini, Srivatsa Vaddagiri,
Xen, Sedat Dilek, Thomas Gleixner, Yinghai Lu,
Konrad Rzeszutek Wilk, Greg Kroah-Hartman, LKML, Suzuki Poulose
In-Reply-To: <20111023190700.16364.7548.sendpatchset@oc5400248562.ibm.com>
On Mon, 2011-10-24 at 00:37 +0530, Raghavendra K T wrote:
> Added configuration support to enable debug information
> for KVM Guests in debugfs
>
> Signed-off-by: Srivatsa Vaddagiri <vatsa@linux.vnet.ibm.com>
> Signed-off-by: Suzuki Poulose <suzuki@in.ibm.com>
> Signed-off-by: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
> ---
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 1f03f82..ed34269 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -562,6 +562,15 @@ config KVM_GUEST
> This option enables various optimizations for running under the KVM
> hypervisor.
>
> +config KVM_DEBUG_FS
> + bool "Enable debug information for KVM Guests in debugfs"
> + depends on KVM_GUEST
Shouldn't it depend on DEBUG_FS as well?
> + default n
> + ---help---
> + This option enables collection of various statistics for KVM guest.
> + Statistics are displayed in debugfs filesystem. Enabling this option
> + may incur significant overhead.
> +
> source "arch/x86/lguest/Kconfig"
>
> config PARAVIRT
--
Sasha.
^ permalink raw reply
* Re: [PATCH RFC V2 3/5] kvm hypervisor : Add two hypercalls to support pv-ticketlock
From: Sasha Levin @ 2011-10-24 10:01 UTC (permalink / raw)
To: Raghavendra K T
Cc: KVM, Peter Zijlstra, Virtualization, H. Peter Anvin,
Jeremy Fitzhardinge, Dave Jiang, x86, Ingo Molnar, Avi Kivity,
Rik van Riel, Stefano Stabellini, Srivatsa Vaddagiri, Xen,
Sedat Dilek, Thomas Gleixner, Yinghai Lu, Konrad Rzeszutek Wilk,
Greg Kroah-Hartman, LKML, Suzuki Poulose
In-Reply-To: <20111023190558.16364.2136.sendpatchset@oc5400248562.ibm.com>
On Mon, 2011-10-24 at 00:35 +0530, Raghavendra K T wrote:
> Add two hypercalls to KVM hypervisor to support pv-ticketlocks.
>
> KVM_HC_WAIT_FOR_KICK blocks the calling vcpu until another vcpu kicks it or it
> is woken up because of an event like interrupt.
>
> KVM_HC_KICK_CPU allows the calling vcpu to kick another vcpu.
>
> The presence of these hypercalls is indicated to guest via
> KVM_FEATURE_WAIT_FOR_KICK/KVM_CAP_WAIT_FOR_KICK.
>
> Qemu needs a corresponding patch to pass up the presence of this feature to
> guest via cpuid. Patch to qemu will be sent separately.
>
> There is no Xen/KVM hypercall interface to await kick from.
>
> Signed-off-by: Srivatsa Vaddagiri <vatsa@linux.vnet.ibm.com>
> Signed-off-by: Suzuki Poulose <suzuki@in.ibm.com>
> Signed-off-by: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
> ---
> diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
> index 734c376..2874c19 100644
> --- a/arch/x86/include/asm/kvm_para.h
> +++ b/arch/x86/include/asm/kvm_para.h
> @@ -16,12 +16,14 @@
> #define KVM_FEATURE_CLOCKSOURCE 0
> #define KVM_FEATURE_NOP_IO_DELAY 1
> #define KVM_FEATURE_MMU_OP 2
> +
> /* This indicates that the new set of kvmclock msrs
> * are available. The use of 0x11 and 0x12 is deprecated
> */
> #define KVM_FEATURE_CLOCKSOURCE2 3
> #define KVM_FEATURE_ASYNC_PF 4
> #define KVM_FEATURE_STEAL_TIME 5
> +#define KVM_FEATURE_WAIT_FOR_KICK 6
>
> /* The last 8 bits are used to indicate how to interpret the flags field
> * in pvclock structure. If no bits are set, all flags are ignored.
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 84a28ea..b43fd18 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2077,6 +2077,7 @@ int kvm_dev_ioctl_check_extension(long ext)
> case KVM_CAP_XSAVE:
> case KVM_CAP_ASYNC_PF:
> case KVM_CAP_GET_TSC_KHZ:
> + case KVM_CAP_WAIT_FOR_KICK:
> r = 1;
> break;
> case KVM_CAP_COALESCED_MMIO:
> @@ -2548,7 +2549,8 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> (1 << KVM_FEATURE_NOP_IO_DELAY) |
> (1 << KVM_FEATURE_CLOCKSOURCE2) |
> (1 << KVM_FEATURE_ASYNC_PF) |
> - (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
> + (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT) |
> + (1 << KVM_FEATURE_WAIT_FOR_KICK);
>
> if (sched_info_on())
> entry->eax |= (1 << KVM_FEATURE_STEAL_TIME);
> @@ -5231,6 +5233,61 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
> return 1;
> }
>
> +/*
> + * kvm_pv_wait_for_kick_op : Block until kicked by either a KVM_HC_KICK_CPU
> + * hypercall or a event like interrupt.
> + *
> + * @vcpu : vcpu which is blocking.
> + */
> +static void kvm_pv_wait_for_kick_op(struct kvm_vcpu *vcpu)
> +{
> + DEFINE_WAIT(wait);
> +
> + /*
> + * Blocking on vcpu->wq allows us to wake up sooner if required to
> + * service pending events (like interrupts).
> + *
> + * Also set state to TASK_INTERRUPTIBLE before checking vcpu->kicked to
> + * avoid racing with kvm_pv_kick_cpu_op().
> + */
> + prepare_to_wait(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
> +
> + /*
> + * Somebody has already tried kicking us. Acknowledge that
> + * and terminate the wait.
> + */
> + if (vcpu->kicked) {
> + vcpu->kicked = 0;
> + goto end_wait;
> + }
> +
> + /* Let's wait for either KVM_HC_KICK_CPU or someother event
> + * to wake us up.
> + */
> +
> + srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
> + schedule();
> + vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
> +
> +end_wait:
> + finish_wait(&vcpu->wq, &wait);
> +}
> +
> +/*
> + * kvm_pv_kick_cpu_op: Kick a vcpu.
> + *
> + * @cpu - vcpu to be kicked.
> + */
> +static void kvm_pv_kick_cpu_op(struct kvm *kvm, int cpu)
> +{
> + struct kvm_vcpu *vcpu = kvm_get_vcpu(kvm, cpu);
> +
> + if (vcpu) {
> + vcpu->kicked = 1;
I'm not sure about it, but maybe we want a memory barrier over here?
> + wake_up_interruptible(&vcpu->wq);
> + }
> +}
--
Sasha.
^ permalink raw reply
* Re: [PATCH v3] virtio: Add platform bus driver for memory mapped virtio device
From: Pawel Moll @ 2011-10-24 13:06 UTC (permalink / raw)
To: Rusty Russell
Cc: Anthony Liguori, Michael S.Tsirkin, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <87wrbv15lp.fsf@rustcorp.com.au>
On Mon, 2011-10-24 at 03:33 +0100, Rusty Russell wrote:
> No, that's it I think. Please send a diff for the documentation, since
> I'm updating the LyX master and I've already applied your previous
> version.
Here it goes (below). Also do you think you would be able to merge the
driver (corresponding v4 patch follows) in the 3.2 merge window that
seems to have just opened? ;-)
Cheers!
Pawel
--- virtio-mmio.orig 2011-10-24 11:17:08.263907000 +0100
+++ virtio-mmio.tex 2011-10-24 13:58:29.752757000 +0100
@@ -59,9 +59,18 @@
\item 0x050 | W | QueueNotify \\
Queue notifier.\\
Writing a queue index to this register notifies the Host that there are new buffers to process in the queue.
-\item 0x060 | W | InterruptACK \\
+\item 0x060 | R | InterruptStatus \\
+Interrupt status. \\
+Reading from this register returns a bit mask of interrupts asserted by the device. An interrupt is asserted if the corresponding bit is set, ie. equals one (1). \\
+\begin{itemize}
+\item Bit 0 | Used Ring update \\
+This interrupt is asserted when the Host has updated the Used Ring in at least one of the active virtual queues.
+\item Bit 1 | Configuration change \\
+This interrupt is asserted when configuration of the device has changed.
+\end{itemize}
+\item 0x064 | W | InterruptACK \\
Interrupt acknowledge. \\
-Writing to this register notifies the Host that the Guest finished receiving used buffers from the device and therefore serviced an asserted interrupt. Values written to this register are currently not used, but for future extensions it must be set to one (0x1).
+Writing to this register notifies the Host that the Guest finished handling interrupts. Every bit of the value clears corresponding bit of the InterruptStatus register. \\
\item 0x070 | RW | Status \\
Device status. \\
Reading from this register returns the current device status flags. \\
@@ -100,8 +109,7 @@
The memory mapped virtio device behaves in the same way as described in p. 2.4 ``Device Operation'', with the following exceptions:
\begin{enumerate}
\item The device is notified about new buffers available in a queue by writing the queue index to register QueueNum instead of the virtio header in PCI I/O space (p. 2.4.1.4 ``Notifying The Device'').
-\item As the memory mapped virtio device is using single, dedicated interrupt signal, its handling is much simpler than in the PCI (MSI-X) case (p. 2.4.2 ``Receiving Used Buffer From The Device''). Therefore all the Guest interrupt handler should do after receiving used buffers is acknowledging the interrupt by writing a value to the InterruptACK register. Currently this value does not carry any meaning, but for future extensions it must be set to one (0x1).
-\item The dynamic configuration changes, as described in p. 2.4.3 ``Dealing With Configuration Changes'' are not permitted.
+\item The memory mapped virtio device is using single, dedicated interrupt signal. After receiving an interrupt, the driver must read the InterruptStatus register to check what caused the interrupt (see the register description). After the interrupt is handled, the driver must acknowledge it by writing a bit mask corresponding to the serviced interrupt to the InterruptACK register.
\end{enumerate}
\end{document}
^ permalink raw reply
* [PATCH v4] virtio: Add platform bus driver for memory mapped virtio device
From: Pawel Moll @ 2011-10-24 13:07 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, virtualization
Cc: Anthony Liguori, Pawel Moll, Michael S.Tsirkin
In-Reply-To: <1319461560.3668.16.camel@hornet.cambridge.arm.com>
This patch, based on virtio PCI driver, adds support for memory
mapped (platform) virtio device. This should allow environments
like qemu to use virtio-based block & network devices even on
platforms without PCI support.
One can define and register a platform device which resources
will describe memory mapped control registers and "mailbox"
interrupt. Such device can be also instantiated using the Device
Tree node with compatible property equal "virtio,mmio".
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Anthony Liguori <aliguori@us.ibm.com>
Cc: Michael S.Tsirkin <mst@redhat.com>
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
Changes since v3:
* Dynamic config changes are handled now
* Interrupt acknowledge register moved to 0x064
* Added interrupt status register at 0x060
* Added interrupt flags (VIRTIO_MMIO_INT_VRING & VIRTIO_MMIO_INT_CONFIG)
Changes since v2:
* Fixed bug a bug in vm_find_vqs() error handling code (interrupt was
freed twice)
Changes since v1:
* Added new QueueNumMax register at 0x034, shifting QueueNum,
QueueAlign and QueuePFN by 4 bytes
* Queue page allocation strategy changed
Documentation/devicetree/bindings/virtio/mmio.txt | 17 +
drivers/virtio/Kconfig | 11 +
drivers/virtio/Makefile | 1 +
drivers/virtio/virtio_mmio.c | 479 +++++++++++++++++++++
include/linux/virtio_mmio.h | 111 +++++
5 files changed, 619 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/virtio/mmio.txt
create mode 100644 drivers/virtio/virtio_mmio.c
create mode 100644 include/linux/virtio_mmio.h
diff --git a/Documentation/devicetree/bindings/virtio/mmio.txt b/Documentation/devicetree/bindings/virtio/mmio.txt
new file mode 100644
index 0000000..5069c1b
--- /dev/null
+++ b/Documentation/devicetree/bindings/virtio/mmio.txt
@@ -0,0 +1,17 @@
+* virtio memory mapped device
+
+See http://ozlabs.org/~rusty/virtio-spec/ for more details.
+
+Required properties:
+
+- compatible: "virtio,mmio" compatibility string
+- reg: control registers base address and size including configuration space
+- interrupts: interrupt generated by the device
+
+Example:
+
+ virtio_block@3000 {
+ compatible = "virtio,mmio";
+ reg = <0x3000 0x100>;
+ interrupts = <41>;
+ }
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 57e493b..816ed08 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -35,4 +35,15 @@ config VIRTIO_BALLOON
If unsure, say M.
+ config VIRTIO_MMIO
+ tristate "Platform bus driver for memory mapped virtio devices (EXPERIMENTAL)"
+ depends on EXPERIMENTAL
+ select VIRTIO
+ select VIRTIO_RING
+ ---help---
+ This drivers provides support for memory mapped virtio
+ platform device driver.
+
+ If unsure, say N.
+
endmenu
diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index 6738c44..5a4c63c 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -1,4 +1,5 @@
obj-$(CONFIG_VIRTIO) += virtio.o
obj-$(CONFIG_VIRTIO_RING) += virtio_ring.o
+obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
new file mode 100644
index 0000000..acc5e43
--- /dev/null
+++ b/drivers/virtio/virtio_mmio.c
@@ -0,0 +1,479 @@
+/*
+ * Virtio memory mapped device driver
+ *
+ * Copyright 2011, ARM Ltd.
+ *
+ * This module allows virtio devices to be used over a virtual, memory mapped
+ * platform device.
+ *
+ * Registers layout (all 32-bit wide):
+ *
+ * offset d. name description
+ * ------ -- ---------------- -----------------
+ *
+ * 0x000 R MagicValue Magic value "virt"
+ * 0x004 R Version Device version (current max. 1)
+ * 0x008 R DeviceID Virtio device ID
+ * 0x00c R VendorID Virtio vendor ID
+ *
+ * 0x010 R HostFeatures Features supported by the host
+ * 0x014 W HostFeaturesSel Set of host features to access via HostFeatures
+ *
+ * 0x020 W GuestFeatures Features activated by the guest
+ * 0x024 W GuestFeaturesSel Set of activated features to set via GuestFeatures
+ * 0x028 W GuestPageSize Size of guest's memory page in bytes
+ *
+ * 0x030 W QueueSel Queue selector
+ * 0x034 R QueueNumMax Maximum size of the currently selected queue
+ * 0x038 W QueueNum Queue size for the currently selected queue
+ * 0x03c W QueueAlign Used Ring alignment for the current queue
+ * 0x040 RW QueuePFN PFN for the currently selected queue
+ *
+ * 0x050 W QueueNotify Queue notifier
+ * 0x060 R InterruptStatus Interrupt status register
+ * 0x060 W InterruptACK Interrupt acknowledge register
+ * 0x070 RW Status Device status register
+ *
+ * 0x100+ RW Device-specific configuration space
+ *
+ * Based on Virtio PCI driver by Anthony Liguori, copyright IBM Corp. 2007
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include <linux/highmem.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/virtio.h>
+#include <linux/virtio_config.h>
+#include <linux/virtio_mmio.h>
+#include <linux/virtio_ring.h>
+
+
+
+/* The alignment to use between consumer and producer parts of vring.
+ * Currently hardcoded to the page size. */
+#define VIRTIO_MMIO_VRING_ALIGN PAGE_SIZE
+
+
+
+#define to_virtio_mmio_device(_plat_dev) \
+ container_of(_plat_dev, struct virtio_mmio_device, vdev)
+
+struct virtio_mmio_device {
+ struct virtio_device vdev;
+ struct platform_device *pdev;
+
+ void __iomem *base;
+ unsigned long version;
+
+ /* a list of queues so we can dispatch IRQs */
+ spinlock_t lock;
+ struct list_head virtqueues;
+};
+
+struct virtio_mmio_vq_info {
+ /* the actual virtqueue */
+ struct virtqueue *vq;
+
+ /* the number of entries in the queue */
+ unsigned int num;
+
+ /* the index of the queue */
+ int queue_index;
+
+ /* the virtual address of the ring queue */
+ void *queue;
+
+ /* the list node for the virtqueues list */
+ struct list_head node;
+};
+
+
+
+/* Configuration interface */
+
+static u32 vm_get_features(struct virtio_device *vdev)
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+
+ /* TODO: Features > 32 bits */
+ writel(0, vm_dev->base + VIRTIO_MMIO_HOST_FEATURES_SEL);
+
+ return readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES);
+}
+
+static void vm_finalize_features(struct virtio_device *vdev)
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+ int i;
+
+ /* Give virtio_ring a chance to accept features. */
+ vring_transport_features(vdev);
+
+ for (i = 0; i < ARRAY_SIZE(vdev->features); i++) {
+ writel(i, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SET);
+ writel(vdev->features[i],
+ vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
+ }
+}
+
+static void vm_get(struct virtio_device *vdev, unsigned offset,
+ void *buf, unsigned len)
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+ u8 *ptr = buf;
+ int i;
+
+ for (i = 0; i < len; i++)
+ ptr[i] = readb(vm_dev->base + VIRTIO_MMIO_CONFIG + offset + i);
+}
+
+static void vm_set(struct virtio_device *vdev, unsigned offset,
+ const void *buf, unsigned len)
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+ const u8 *ptr = buf;
+ int i;
+
+ for (i = 0; i < len; i++)
+ writeb(ptr[i], vm_dev->base + VIRTIO_MMIO_CONFIG + offset + i);
+}
+
+static u8 vm_get_status(struct virtio_device *vdev)
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+
+ return readl(vm_dev->base + VIRTIO_MMIO_STATUS) & 0xff;
+}
+
+static void vm_set_status(struct virtio_device *vdev, u8 status)
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+
+ /* We should never be setting status to 0. */
+ BUG_ON(status == 0);
+
+ writel(status, vm_dev->base + VIRTIO_MMIO_STATUS);
+}
+
+static void vm_reset(struct virtio_device *vdev)
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+
+ /* 0 status means a reset. */
+ writel(0, vm_dev->base + VIRTIO_MMIO_STATUS);
+}
+
+
+
+/* Transport interface */
+
+/* the notify function used when creating a virt queue */
+static void vm_notify(struct virtqueue *vq)
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
+ struct virtio_mmio_vq_info *info = vq->priv;
+
+ /* We write the queue's selector into the notification register to
+ * signal the other end */
+ writel(info->queue_index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
+}
+
+/* Notify all virtqueues on an interrupt. */
+static irqreturn_t vm_interrupt(int irq, void *opaque)
+{
+ struct virtio_mmio_device *vm_dev = opaque;
+ struct virtio_mmio_vq_info *info;
+ struct virtio_driver *vdrv = container_of(vm_dev->vdev.dev.driver,
+ struct virtio_driver, driver);
+ unsigned long status;
+ unsigned long flags;
+ irqreturn_t ret = IRQ_NONE;
+
+ /* Read and acknowledge interrupts */
+ status = readl(vm_dev->base + VIRTIO_MMIO_INTERRUPT_STATUS);
+ writel(status, vm_dev->base + VIRTIO_MMIO_INTERRUPT_ACK);
+
+ if (unlikely(status & VIRTIO_MMIO_INT_CONFIG)
+ && vdrv && vdrv->config_changed) {
+ vdrv->config_changed(&vm_dev->vdev);
+ ret = IRQ_HANDLED;
+ }
+
+ if (likely(status & VIRTIO_MMIO_INT_VRING)) {
+ spin_lock_irqsave(&vm_dev->lock, flags);
+ list_for_each_entry(info, &vm_dev->virtqueues, node)
+ ret |= vring_interrupt(irq, info->vq);
+ spin_unlock_irqrestore(&vm_dev->lock, flags);
+ }
+
+ return ret;
+}
+
+
+
+static void vm_del_vq(struct virtqueue *vq)
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
+ struct virtio_mmio_vq_info *info = vq->priv;
+ unsigned long flags, size;
+
+ spin_lock_irqsave(&vm_dev->lock, flags);
+ list_del(&info->node);
+ spin_unlock_irqrestore(&vm_dev->lock, flags);
+
+ vring_del_virtqueue(vq);
+
+ /* Select and deactivate the queue */
+ writel(info->queue_index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
+ writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
+
+ size = PAGE_ALIGN(vring_size(info->num, VIRTIO_MMIO_VRING_ALIGN));
+ free_pages_exact(info->queue, size);
+ kfree(info);
+}
+
+static void vm_del_vqs(struct virtio_device *vdev)
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+ struct virtqueue *vq, *n;
+
+ list_for_each_entry_safe(vq, n, &vdev->vqs, list)
+ vm_del_vq(vq);
+
+ free_irq(platform_get_irq(vm_dev->pdev, 0), vm_dev);
+}
+
+
+
+static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
+ void (*callback)(struct virtqueue *vq),
+ const char *name)
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+ struct virtio_mmio_vq_info *info;
+ struct virtqueue *vq;
+ unsigned long flags, size;
+ int err;
+
+ /* Select the queue we're interested in */
+ writel(index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
+
+ /* Queue shouldn't already be set up. */
+ if (readl(vm_dev->base + VIRTIO_MMIO_QUEUE_PFN)) {
+ err = -ENOENT;
+ goto error_available;
+ }
+
+ /* Allocate and fill out our active queue description */
+ info = kmalloc(sizeof(*info), GFP_KERNEL);
+ if (!info) {
+ err = -ENOMEM;
+ goto error_kmalloc;
+ }
+ info->queue_index = index;
+
+ /* Allocate pages for the queue - start with a queue as big as
+ * possible (limited by maximum size allowed by device), drop down
+ * to a minimal size, just big enough to fit descriptor table
+ * and two rings (which makes it "alignment_size * 2")
+ */
+ info->num = readl(vm_dev->base + VIRTIO_MMIO_QUEUE_NUM_MAX);
+ while (1) {
+ size = PAGE_ALIGN(vring_size(info->num,
+ VIRTIO_MMIO_VRING_ALIGN));
+ /* Already smallest possible allocation? */
+ if (size <= VIRTIO_MMIO_VRING_ALIGN * 2) {
+ err = -ENOMEM;
+ goto error_alloc_pages;
+ }
+
+ info->queue = alloc_pages_exact(size, GFP_KERNEL | __GFP_ZERO);
+ if (info->queue)
+ break;
+
+ info->num /= 2;
+ }
+
+ /* Activate the queue */
+ writel(info->num, vm_dev->base + VIRTIO_MMIO_QUEUE_NUM);
+ writel(VIRTIO_MMIO_VRING_ALIGN,
+ vm_dev->base + VIRTIO_MMIO_QUEUE_ALIGN);
+ writel(virt_to_phys(info->queue) >> PAGE_SHIFT,
+ vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
+
+ /* Create the vring */
+ vq = vring_new_virtqueue(info->num, VIRTIO_MMIO_VRING_ALIGN,
+ vdev, info->queue, vm_notify, callback, name);
+ if (!vq) {
+ err = -ENOMEM;
+ goto error_new_virtqueue;
+ }
+
+ vq->priv = info;
+ info->vq = vq;
+
+ spin_lock_irqsave(&vm_dev->lock, flags);
+ list_add(&info->node, &vm_dev->virtqueues);
+ spin_unlock_irqrestore(&vm_dev->lock, flags);
+
+ return vq;
+
+error_new_virtqueue:
+ writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
+ free_pages_exact(info->queue, size);
+error_alloc_pages:
+ kfree(info);
+error_kmalloc:
+error_available:
+ return ERR_PTR(err);
+}
+
+static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
+ struct virtqueue *vqs[],
+ vq_callback_t *callbacks[],
+ const char *names[])
+{
+ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+ unsigned int irq = platform_get_irq(vm_dev->pdev, 0);
+ int i, err;
+
+ err = request_irq(irq, vm_interrupt, IRQF_SHARED,
+ dev_name(&vdev->dev), vm_dev);
+ if (err)
+ return err;
+
+ for (i = 0; i < nvqs; ++i) {
+ vqs[i] = vm_setup_vq(vdev, i, callbacks[i], names[i]);
+ if (IS_ERR(vqs[i])) {
+ vm_del_vqs(vdev);
+ return PTR_ERR(vqs[i]);
+ }
+ }
+
+ return 0;
+}
+
+
+
+static struct virtio_config_ops virtio_mmio_config_ops = {
+ .get = vm_get,
+ .set = vm_set,
+ .get_status = vm_get_status,
+ .set_status = vm_set_status,
+ .reset = vm_reset,
+ .find_vqs = vm_find_vqs,
+ .del_vqs = vm_del_vqs,
+ .get_features = vm_get_features,
+ .finalize_features = vm_finalize_features,
+};
+
+
+
+/* Platform device */
+
+static int __devinit virtio_mmio_probe(struct platform_device *pdev)
+{
+ struct virtio_mmio_device *vm_dev;
+ struct resource *mem;
+ unsigned long magic;
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!mem)
+ return -EINVAL;
+
+ if (!devm_request_mem_region(&pdev->dev, mem->start,
+ resource_size(mem), pdev->name))
+ return -EBUSY;
+
+ vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL);
+ if (!vm_dev)
+ return -ENOMEM;
+
+ vm_dev->vdev.dev.parent = &pdev->dev;
+ vm_dev->vdev.config = &virtio_mmio_config_ops;
+ vm_dev->pdev = pdev;
+ INIT_LIST_HEAD(&vm_dev->virtqueues);
+ spin_lock_init(&vm_dev->lock);
+
+ vm_dev->base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
+ if (vm_dev->base == NULL)
+ return -EFAULT;
+
+ /* Check magic value */
+ magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
+ if (memcmp(&magic, "virt", 4) != 0) {
+ dev_warn(&pdev->dev, "Wrong magic value 0x%08lx!\n", magic);
+ return -ENODEV;
+ }
+
+ /* Check device version */
+ vm_dev->version = readl(vm_dev->base + VIRTIO_MMIO_VERSION);
+ if (vm_dev->version != 1) {
+ dev_err(&pdev->dev, "Version %ld not supported!\n",
+ vm_dev->version);
+ return -ENXIO;
+ }
+
+ vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
+ vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
+
+ writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
+
+ platform_set_drvdata(pdev, vm_dev);
+
+ return register_virtio_device(&vm_dev->vdev);
+}
+
+static int __devexit virtio_mmio_remove(struct platform_device *pdev)
+{
+ struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
+
+ unregister_virtio_device(&vm_dev->vdev);
+
+ return 0;
+}
+
+
+
+/* Platform driver */
+
+static struct of_device_id virtio_mmio_match[] = {
+ { .compatible = "virtio,mmio", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, virtio_mmio_match);
+
+static struct platform_driver virtio_mmio_driver = {
+ .probe = virtio_mmio_probe,
+ .remove = __devexit_p(virtio_mmio_remove),
+ .driver = {
+ .name = "virtio-mmio",
+ .owner = THIS_MODULE,
+ .of_match_table = virtio_mmio_match,
+ },
+};
+
+static int __init virtio_mmio_init(void)
+{
+ return platform_driver_register(&virtio_mmio_driver);
+}
+
+static void __exit virtio_mmio_exit(void)
+{
+ platform_driver_unregister(&virtio_mmio_driver);
+}
+
+module_init(virtio_mmio_init);
+module_exit(virtio_mmio_exit);
+
+MODULE_AUTHOR("Pawel Moll <pawel.moll@arm.com>");
+MODULE_DESCRIPTION("Platform bus driver for memory mapped virtio devices");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/virtio_mmio.h b/include/linux/virtio_mmio.h
new file mode 100644
index 0000000..27c7ede
--- /dev/null
+++ b/include/linux/virtio_mmio.h
@@ -0,0 +1,111 @@
+/*
+ * Virtio platform device driver
+ *
+ * Copyright 2011, ARM Ltd.
+ *
+ * Based on Virtio PCI driver by Anthony Liguori, copyright IBM Corp. 2007
+ *
+ * This header is BSD licensed so anyone can use the definitions to implement
+ * compatible drivers/servers.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of IBM nor the names of its contributors
+ * may be used to endorse or promote products derived from this software
+ * without specific prior written permission.
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL IBM OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#ifndef _LINUX_VIRTIO_MMIO_H
+#define _LINUX_VIRTIO_MMIO_H
+
+/*
+ * Control registers
+ */
+
+/* Magic value ("virt" string) - Read Only */
+#define VIRTIO_MMIO_MAGIC_VALUE 0x000
+
+/* Virtio device version - Read Only */
+#define VIRTIO_MMIO_VERSION 0x004
+
+/* Virtio device ID - Read Only */
+#define VIRTIO_MMIO_DEVICE_ID 0x008
+
+/* Virtio vendor ID - Read Only */
+#define VIRTIO_MMIO_VENDOR_ID 0x00c
+
+/* Bitmask of the features supported by the host
+ * (32 bits per set) - Read Only */
+#define VIRTIO_MMIO_HOST_FEATURES 0x010
+
+/* Host features set selector - Write Only */
+#define VIRTIO_MMIO_HOST_FEATURES_SEL 0x014
+
+/* Bitmask of features activated by the guest
+ * (32 bits per set) - Write Only */
+#define VIRTIO_MMIO_GUEST_FEATURES 0x020
+
+/* Activated features set selector - Write Only */
+#define VIRTIO_MMIO_GUEST_FEATURES_SET 0x024
+
+/* Guest's memory page size in bytes - Write Only */
+#define VIRTIO_MMIO_GUEST_PAGE_SIZE 0x028
+
+/* Queue selector - Write Only */
+#define VIRTIO_MMIO_QUEUE_SEL 0x030
+
+/* Maximum size of the currently selected queue - Read Only */
+#define VIRTIO_MMIO_QUEUE_NUM_MAX 0x034
+
+/* Queue size for the currently selected queue - Write Only */
+#define VIRTIO_MMIO_QUEUE_NUM 0x038
+
+/* Used Ring alignment for the currently selected queue - Write Only */
+#define VIRTIO_MMIO_QUEUE_ALIGN 0x03c
+
+/* Guest's PFN for the currently selected queue - Read Write */
+#define VIRTIO_MMIO_QUEUE_PFN 0x040
+
+/* Queue notifier - Write Only */
+#define VIRTIO_MMIO_QUEUE_NOTIFY 0x050
+
+/* Interrupt status - Read Only */
+#define VIRTIO_MMIO_INTERRUPT_STATUS 0x060
+
+/* Interrupt acknowledge - Write Only */
+#define VIRTIO_MMIO_INTERRUPT_ACK 0x064
+
+/* Device status register - Read Write */
+#define VIRTIO_MMIO_STATUS 0x070
+
+/* The config space is defined by each driver as
+ * the per-driver configuration space - Read Write */
+#define VIRTIO_MMIO_CONFIG 0x100
+
+
+
+/*
+ * Interrupt flags (re: interrupt status & acknowledge registers)
+ */
+
+#define VIRTIO_MMIO_INT_VRING (1 << 0)
+#define VIRTIO_MMIO_INT_CONFIG (1 << 1)
+
+#endif
--
1.6.3.3
^ permalink raw reply related
* [PATCH 1/1] Staging: hv: vmbus: Support building the vmbus driver as part of the kernel
From: K. Y. Srinivasan @ 2011-10-24 18:28 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering
Cc: K. Y. Srinivasan, Haiyang Zhang
Modify the way we initialize the vmbus driver so that all the hyper-v drivers
can be linked with the kernel and still ensure that the vmbus driver
is fully initialized before the drivers that depend upon the vmbus
driver attempt to initialize.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/hv/vmbus_drv.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 0c048dd..7d3bccb3 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -783,4 +783,4 @@ cleanup:
MODULE_LICENSE("GPL");
MODULE_VERSION(HV_DRV_VERSION);
-module_init(hv_acpi_init);
+subsys_initcall(hv_acpi_init);
--
1.7.4.1
^ permalink raw reply related
* [PATCH 1/1] Staging: hv: vmbus: Support building the vmbus driver as part of the kernel
From: K. Y. Srinivasan @ 2011-10-24 20:31 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering
Cc: K. Y. Srinivasan, Haiyang Zhang
Modify the way we initialize the vmbus driver so that all the hyper-v drivers
can be linked with the kernel and still ensure that the vmbus driver
is fully initialized before the drivers that depend upon the vmbus
driver attempt to initialize.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/hv/vmbus_drv.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 0c048dd..7d3bccb3 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -783,4 +783,4 @@ cleanup:
MODULE_LICENSE("GPL");
MODULE_VERSION(HV_DRV_VERSION);
-module_init(hv_acpi_init);
+subsys_initcall(hv_acpi_init);
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH 1/1] Staging: hv: vmbus: Support building the vmbus driver as part of the kernel
From: Greg KH @ 2011-10-25 2:27 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, ohering,
Haiyang Zhang
In-Reply-To: <1319488265-26936-1-git-send-email-kys@microsoft.com>
On Mon, Oct 24, 2011 at 01:31:05PM -0700, K. Y. Srinivasan wrote:
> Modify the way we initialize the vmbus driver so that all the hyper-v drivers
> can be linked with the kernel and still ensure that the vmbus driver
> is fully initialized before the drivers that depend upon the vmbus
> driver attempt to initialize.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Any reason you sent this out twice? Was there a difference between the
two versions that I missed?
greg k-h
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: vmbus: Support building the vmbus driver as part of the kernel
From: KY Srinivasan @ 2011-10-25 2:43 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, Haiyang Zhang
In-Reply-To: <20111025022717.GA32457@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Monday, October 24, 2011 10:27 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> Haiyang Zhang
> Subject: Re: [PATCH 1/1] Staging: hv: vmbus: Support building the vmbus driver
> as part of the kernel
>
> On Mon, Oct 24, 2011 at 01:31:05PM -0700, K. Y. Srinivasan wrote:
> > Modify the way we initialize the vmbus driver so that all the hyper-v drivers
> > can be linked with the kernel and still ensure that the vmbus driver
> > is fully initialized before the drivers that depend upon the vmbus
> > driver attempt to initialize.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
>
> Any reason you sent this out twice? Was there a difference between the
> two versions that I missed?
No difference. It is the mail server acting up. After I sent the mail initially,
it did not go out for several hours. I sent it again and after several hours, both were
sent out. Sorry for the confusion; you can ignore one of them.
Regards,
K. Y
>
> greg k-h
^ permalink raw reply
* Re: [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean
From: Joe Perches @ 2011-10-26 0:03 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, Haiyang Zhang
In-Reply-To: <1319588392-9982-1-git-send-email-kys@microsoft.com>
On Tue, 2011-10-25 at 17:19 -0700, K. Y. Srinivasan wrote:
> Make some state that is boolean in nature, a boolean variable.
[]
> diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
[]
@@ -148,7 +148,7 @@ struct mousevsc_prt_msg {
> */
> struct mousevsc_dev {
> struct hv_device *device;
> - unsigned char init_complete;
> + bool init_complete;
> struct mousevsc_prt_msg protocol_req;
> struct mousevsc_prt_msg protocol_resp;
> /* Synchronize the request/response if needed */
> @@ -159,7 +159,7 @@ struct mousevsc_dev {
> unsigned char *report_desc;
> u32 report_desc_size;
> struct hv_input_dev_info hid_dev_info;
> - int connected;
> + bool connected;
> struct hid_device *hid_device;
> };
One of the bools should be moved to be adjacent to the other.
^ permalink raw reply
* [PATCH 0/6] Staging: hv: mousevsc: cleanup the mouse driver
From: K. Y. Srinivasan @ 2011-10-26 0:18 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, joe
Cc: K. Y. Srinivasan
As part of the effort to move the mouse driver out of staging, I had posted
the code for review by the community. This patch-set addresses the comments I
received. I would like to thank Dmitry and Joe for their comments and suggestions.
Regards,
K. Y
^ 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