RE: [PATCH 3/3] drivers: Add visorbus to the drivers/virt directory

2016-09-11 Thread Sell, Timothy C 
On Sun, 11 Sep 2016 02:17:10 -0700, Greg KH wrote:
> On Tue, Aug 30, 2016 at 04:29:07PM +, Sell, Timothy C wrote:
>> E.g., so even though no obvious error-recovery occurs above in-response to
>> kzalloc() failures, the fact that -CONTROLVM_RESP_ERROR_KMALLOC_FAILED is
>> provided to bus_epilog() is in-fact sufficient to report the error.
>> 
>> Is this making sense?

> Yes, it does a bit more, but, you should make this more explicit.

Agreed.  I believe your recommendation below is the right way to accomplish
this.

>> Can you suggest how we might modify our code to make this error-handling /
>> recovery strategy clearer?

> Have a real error be returned by the function, and then have the caller
> handle the error by propagating it back to the firmware through the
> message.  That might save you a lot of boiler-plate error handling logic
> as well, right?

> That will make it obvious that if an error happens, it is caught, and
> how it is handled correctly.

> Does that help?

Yes it does.  We'll work out a scheme where all of these functions in fact
return their status code to a common dispatch function, whose job it will
be to send this status code back to the firmware.  This might even let us
consolidate/eliminate a few of the other functions we have that deal with
various ways of formatting firmware responses, by encompassing their logic
into this common dispatch function.

That strategy should make it clearer and more explicit as to what is
going on here.

Thanks!

Tim Sell

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] drivers: Add visorbus to the drivers/virt directory

2016-09-11 Thread 'Greg KH' 
On Tue, Aug 30, 2016 at 04:29:07PM +, Sell, Timothy C wrote:
> E.g., so even though no obvious error-recovery occurs above in-response to
> kzalloc() failures, the fact that -CONTROLVM_RESP_ERROR_KMALLOC_FAILED is
> provided to bus_epilog() is in-fact sufficient to report the error.
> 
> Is this making sense?

Yes, it does a bit more, but, you should make this more explicit.

> Can you suggest how we might modify our code to make this error-handling /
> recovery strategy clearer?

Have a real error be returned by the function, and then have the caller
handle the error by propagating it back to the firmware through the
message.  That might save you a lot of boiler-plate error handling logic
as well, right?

That will make it obvious that if an error happens, it is caught, and
how it is handled correctly.

Does that help?

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 3/3] drivers: Add visorbus to the drivers/virt directory

2016-08-30 Thread Sell, Timothy C 
> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Sunday, August 21, 2016 2:05 PM
> To: Kershner, David A 
> 
> On Fri, Jun 10, 2016 at 11:23:56PM -0400, David Kershner wrote:
> > visorbus is currently located at drivers/staging/visorbus,
> > this patch moves it to drivers/virt.
> >
> > Signed-off-by: David Kershner 
> > Reviewed-by: Tim Sell 
> 
> I picked one random file here, this last one, and found a number of
> "odd" things in it.
> 
> So, given that I can't really comment on the patch itself, I'm going to
> include the file below, quote it, and then provide some comments, ok?

There's obviously a lot of work we have to do here (and that work is
underway), but I'd like to focus this email on explaining a class of comments
you made about our error-handling.

You have correctly pointed out some areas where we are deficient in our
error-handling, but I think there are others where our error-handling is
indeed sufficient, but this is perhaps NOT obvious because we are sending
error codes to our partitioning firmware environment (via a message-passing
interface across a shared-memory interface known as the 'controlvm channel')
instead of specifying them in function return values.  This partitioning
firmware environment is NOT Linux, and is external to the Linux OS environment
where the visorbus driver is running.

It helps to understand the overall flow of visorchipset.c, which basically
fills the role of the server responsible for handling requests initiated by
the partitioning firmware environment:

* controlvm_periodic_work() / handle_command() reads a request from the
  partitioning firmware environment, which will be something like "create a
  virtual bus" or "create a virtual device".

* A case statement in handle_command() calls one of various functions to handle
  the request, which sends the success/failure result code of the operation
  back to the partitioning firmware environment using one of the paths:
  * bus_epilog() / bus_responder() / controlvm_respond()
  * device_epilog() / device_responder() / controlvm_respond()
  This success/failure result code is indicated via one of those
  CONTROLVM_RESP_* error mnemonics (which you clearly indicated your hatred
  for, and which we will change).

* The partitioning firmware environment will react accordingly to the error
  code returned.  E.g., if the request was to create the virtual bus containing
  the boot device, and the error code was CONTROLVM_RESP_ERROR_KMALLOC_FAILED,
  the partitioning firmware environment would fail the boot of the Linux guest
  and inform the user that the Linux guest boot failed due to an out-of-memory
  condition.

(This also should clarify why we use awkward mnemonics like 
CONTROLVM_RESP_ERROR_KMALLOC_FAILED instead of -ENOMEM -- it's because the
error codes are not defined by a Linux environment, because it isn't a Linux
environment that is actually making the controlvm requests.  But that's another
issue.)

Let me use visorchipset.c bus_create() as an example:

static void
bus_create(struct controlvm_message *inmsg)
{
 struct controlvm_message_packet *cmd = >cmd;
 u32 bus_no = cmd->create_bus.bus_no;
 int rc = CONTROLVM_RESP_SUCCESS;
 struct visor_device *bus_info;
 struct visorchannel *visorchannel;

 bus_info = visorbus_get_device_by_id(bus_no, BUS_ROOT_DEVICE, NULL);
 if (bus_info && (bus_info->state.created == 1)) {
 POSTCODE_LINUX_3(BUS_CREATE_FAILURE_PC, bus_no,
  POSTCODE_SEVERITY_ERR);
 rc = -CONTROLVM_RESP_ERROR_ALREADY_DONE;
 goto out_bus_epilog;
 }
 bus_info = kzalloc(sizeof(*bus_info), GFP_KERNEL);
 if (!bus_info) {
 POSTCODE_LINUX_3(BUS_CREATE_FAILURE_PC, bus_no,
  POSTCODE_SEVERITY_ERR);
 rc = -CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
 goto out_bus_epilog;
 }

 INIT_LIST_HEAD(_info->list_all);
 bus_info->chipset_bus_no = bus_no;
 bus_info->chipset_dev_no = BUS_ROOT_DEVICE;

 POSTCODE_LINUX_3(BUS_CREATE_ENTRY_PC, bus_no, POSTCODE_SEVERITY_INFO);

 visorchannel = visorchannel_create(cmd->create_bus.channel_addr,
cmd->create_bus.channel_bytes,
GFP_KERNEL,
cmd->create_bus.bus_data_type_uuid);

 if (!visorchannel) {
 POSTCODE_LINUX_3(BUS_CREATE_FAILURE_PC, bus_no,
  POSTCODE_SEVERITY_ERR);
 rc = -CONTROLVM_RESP_ERROR_KMALLOC_FAILED;
 kfree(bus_info);
 bus_info = NULL;
 goto out_bus_epilog;
 }
 bus_info->visorchannel = visorchannel;
 if

Re: [PATCH 3/3] drivers: Add visorbus to the drivers/virt directory

2016-08-22 Thread 'Greg KH' 
On Mon, Aug 22, 2016 at 10:02:43AM -0400, 'Greg KH' wrote:
> On Mon, Aug 22, 2016 at 10:46:10AM +, Sell, Timothy C wrote:
> > > -Original Message-
> > > From: 'Greg KH' [mailto:gre...@linuxfoundation.org]
> > > Sent: Monday, August 22, 2016 4:16 AM
> > > To: Sell, Timothy C <timothy.s...@unisys.com>
> > > Cc: cor...@lwn.net; t...@linutronix.de; mi...@redhat.com;
> > > h...@zytor.com; Arfvidson, Erik <erik.arfvid...@unisys.com>;
> > > hof...@osadl.org; dzic...@redhat.com; jes.soren...@redhat.com; Curtin,
> > > Alexander Paul <alexander.cur...@unisys.com>;
> > > janani.rvchn...@gmail.com; sudipm.mukher...@gmail.com;
> > > pra...@redhat.com; Binder, David Anthony <david.bin...@unisys.com>;
> > > nhor...@redhat.com; dan.j.willi...@intel.com; linux-
> > > ker...@vger.kernel.org; linux-...@vger.kernel.org; driverdev-
> > > de...@linuxdriverproject.org; *S-Par-Maintainer
> > > <sparmaintai...@unisys.com>; Kershner, David A
> > > <david.kersh...@unisys.com>
> > > Subject: Re: [PATCH 3/3] drivers: Add visorbus to the drivers/virt 
> > > directory
> > > 
> > > oh that's funny, I have no idea what happened here, perhaps when I
> > > copied the file into the original email?  Strange things happen while on
> > > planes...
> > > 
> > > Anyway, ok, one comment addressed, great!  Now about the many many
> > > others?
> > 
> > This particular comment happened to affect the credibility of all the other
> > ones, so it needed to be addressed first!
> 
> Oh, so everything else was credible?  Glad to hear it :)

And please, fix your quoting, you quote all of the cc: lines, yet none
of the text?  Odd emailer...

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] drivers: Add visorbus to the drivers/virt directory

2016-08-22 Thread 'Greg KH' 
On Mon, Aug 22, 2016 at 10:46:10AM +, Sell, Timothy C wrote:
> > -Original Message-
> > From: 'Greg KH' [mailto:gre...@linuxfoundation.org]
> > Sent: Monday, August 22, 2016 4:16 AM
> > To: Sell, Timothy C <timothy.s...@unisys.com>
> > Cc: cor...@lwn.net; t...@linutronix.de; mi...@redhat.com;
> > h...@zytor.com; Arfvidson, Erik <erik.arfvid...@unisys.com>;
> > hof...@osadl.org; dzic...@redhat.com; jes.soren...@redhat.com; Curtin,
> > Alexander Paul <alexander.cur...@unisys.com>;
> > janani.rvchn...@gmail.com; sudipm.mukher...@gmail.com;
> > pra...@redhat.com; Binder, David Anthony <david.bin...@unisys.com>;
> > nhor...@redhat.com; dan.j.willi...@intel.com; linux-
> > ker...@vger.kernel.org; linux-...@vger.kernel.org; driverdev-
> > de...@linuxdriverproject.org; *S-Par-Maintainer
> > <sparmaintai...@unisys.com>; Kershner, David A
> > <david.kersh...@unisys.com>
> > Subject: Re: [PATCH 3/3] drivers: Add visorbus to the drivers/virt directory
> > 
> > oh that's funny, I have no idea what happened here, perhaps when I
> > copied the file into the original email?  Strange things happen while on
> > planes...
> > 
> > Anyway, ok, one comment addressed, great!  Now about the many many
> > others?
> 
> This particular comment happened to affect the credibility of all the other
> ones, so it needed to be addressed first!

Oh, so everything else was credible?  Glad to hear it :)

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 3/3] drivers: Add visorbus to the drivers/virt directory

2016-08-22 Thread Sell, Timothy C 
> -Original Message-
> From: 'Greg KH' [mailto:gre...@linuxfoundation.org]
> Sent: Monday, August 22, 2016 4:16 AM
> To: Sell, Timothy C <timothy.s...@unisys.com>
> Cc: cor...@lwn.net; t...@linutronix.de; mi...@redhat.com;
> h...@zytor.com; Arfvidson, Erik <erik.arfvid...@unisys.com>;
> hof...@osadl.org; dzic...@redhat.com; jes.soren...@redhat.com; Curtin,
> Alexander Paul <alexander.cur...@unisys.com>;
> janani.rvchn...@gmail.com; sudipm.mukher...@gmail.com;
> pra...@redhat.com; Binder, David Anthony <david.bin...@unisys.com>;
> nhor...@redhat.com; dan.j.willi...@intel.com; linux-
> ker...@vger.kernel.org; linux-...@vger.kernel.org; driverdev-
> de...@linuxdriverproject.org; *S-Par-Maintainer
> <sparmaintai...@unisys.com>; Kershner, David A
> <david.kersh...@unisys.com>
> Subject: Re: [PATCH 3/3] drivers: Add visorbus to the drivers/virt directory
> 
> oh that's funny, I have no idea what happened here, perhaps when I
> copied the file into the original email?  Strange things happen while on
> planes...
> 
> Anyway, ok, one comment addressed, great!  Now about the many many
> others?

This particular comment happened to affect the credibility of all the other
ones, so it needed to be addressed first!

Tim Sell

> 
> thanks,
> 
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] drivers: Add visorbus to the drivers/virt directory

2016-08-22 Thread 'Greg KH' 
On Mon, Aug 22, 2016 at 02:48:18AM +, Sell, Timothy C wrote:
> > -Original Message-
> > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > Sent: Sunday, August 21, 2016 2:05 PM
> > To: Kershner, David A <david.kersh...@unisys.com>
> > Cc: cor...@lwn.net; t...@linutronix.de; mi...@redhat.com;
> > h...@zytor.com; Arfvidson, Erik <erik.arfvid...@unisys.com>; Sell, Timothy
> > C <timothy.s...@unisys.com>; hof...@osadl.org; dzic...@redhat.com;
> > jes.soren...@redhat.com; Curtin, Alexander Paul
> > <alexander.cur...@unisys.com>; janani.rvchn...@gmail.com;
> > sudipm.mukher...@gmail.com; pra...@redhat.com; Binder, David Anthony
> > <david.bin...@unisys.com>; nhor...@redhat.com;
> > dan.j.willi...@intel.com; linux-ker...@vger.kernel.org; linux-
> > d...@vger.kernel.org; driverdev-devel@linuxdriverproject.org; *S-Par-
> > Maintainer <sparmaintai...@unisys.com>
> > Subject: Re: [PATCH 3/3] drivers: Add visorbus to the drivers/virt directory
> > 
> > On Fri, Jun 10, 2016 at 11:23:56PM -0400, David Kershner wrote:
> > > visorbus is currently located at drivers/staging/visorbus,
> > > this patch moves it to drivers/virt.
> > >
> > > Signed-off-by: David Kershner <david.kersh...@unisys.com>
> > > Reviewed-by: Tim Sell <timothy.s...@unisys.com>
> > > ---
> > >  drivers/staging/unisys/Kconfig| 
> > > 3 +--
> > >  drivers/staging/unisys/Makefile   | 
> > > 1 -
> > >  drivers/virt/Kconfig  | 
> > > 2 ++
> > >  drivers/virt/Makefile | 
> > > 1 +
> > >  drivers/{staging/unisys => virt}/visorbus/Kconfig | 0
> > >  drivers/{staging/unisys => virt}/visorbus/Makefile| 0
> > >  drivers/{staging/unisys => virt}/visorbus/controlvmchannel.h  | 0
> > >  drivers/{staging/unisys => virt}/visorbus/controlvmcompletionstatus.h | 0
> > >  drivers/{staging/unisys => virt}/visorbus/iovmcall_gnuc.h | 0
> > >  drivers/{staging/unisys => virt}/visorbus/vbuschannel.h   | 0
> > >  drivers/{staging/unisys => virt}/visorbus/vbusdeviceinfo.h| 0
> > >  drivers/{staging/unisys => virt}/visorbus/vbushelper.h| 0
> > >  drivers/{staging/unisys => virt}/visorbus/visorbus_main.c | 0
> > >  drivers/{staging/unisys => virt}/visorbus/visorbus_private.h  | 0
> > >  drivers/{staging/unisys => virt}/visorbus/visorchannel.c  | 0
> > >  drivers/{staging/unisys => virt}/visorbus/visorchipset.c  | 0
> > 
> > I picked one random file here, this last one, and found a number of
> > "odd" things in it.
> > 
> > So, given that I can't really comment on the patch itself, I'm going to
> > include the file below, quote it, and then provide some comments, ok?
> > > /* visorchipset_main.c
> > > ...
> > > static void
> > > controlvm_init_response(struct controlvm_message *msg,
> > >   struct controlvm_message_header *msg_hdr, int response)
> > > {
> > >   memset(msg, 0, sizeof(struct controlvm_message));
> > >   memcpy(>hdr, msg_hdr, sizeof(struct controlvm_message_header));
> > >   msg->hdr.payload_bytes = 0;
> > >   msg->hdr.payload_vm_offset = 0;
> > >   msg->hdr.payload_max_bytes = 0;
> > >   if (response < 0) {
> > >   msg->hdr.flags.failed = 1;
> > >   msg->hdr.completion_status = (u32)(-response);
> > >   }
> > > }
> > > 
> > > static void
> > > Billy(struct controlvm_message_header *msg_hdr, int response)
> > 
> > Not John?  Bob?  Sally?  Alice?  Bernise?  Jean?  Molly?
> 
> Huh? What version of source code are you looking at??
> 
> The file being moved by this patch should be
> drivers/staging/unisys/visorbus/visorchipset.c in your staging-next branch  
> (https://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/unisys/visorbus/visorchipset.c?h=staging-next).
> If you look there, you will NOT find "Billy()", but instead 
> "controlvm_respond()", i.e.:
> 
> static void
> controlvm_init_response(struct controlvm_message *msg,
>   struct controlvm_message_header *msg_hdr, int response)


oh that's funny, I have no idea what happened here, perhaps when I
copied the file into the original email?  Strange things happen while on
planes...

Anyway, ok, one comment addressed, great!  Now about the many many
others?

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH 3/3] drivers: Add visorbus to the drivers/virt directory

2016-08-21 Thread Sell, Timothy C 
> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Sunday, August 21, 2016 2:05 PM
> To: Kershner, David A <david.kersh...@unisys.com>
> Cc: cor...@lwn.net; t...@linutronix.de; mi...@redhat.com;
> h...@zytor.com; Arfvidson, Erik <erik.arfvid...@unisys.com>; Sell, Timothy
> C <timothy.s...@unisys.com>; hof...@osadl.org; dzic...@redhat.com;
> jes.soren...@redhat.com; Curtin, Alexander Paul
> <alexander.cur...@unisys.com>; janani.rvchn...@gmail.com;
> sudipm.mukher...@gmail.com; pra...@redhat.com; Binder, David Anthony
> <david.bin...@unisys.com>; nhor...@redhat.com;
> dan.j.willi...@intel.com; linux-ker...@vger.kernel.org; linux-
> d...@vger.kernel.org; driverdev-devel@linuxdriverproject.org; *S-Par-
> Maintainer <sparmaintai...@unisys.com>
> Subject: Re: [PATCH 3/3] drivers: Add visorbus to the drivers/virt directory
> 
> On Fri, Jun 10, 2016 at 11:23:56PM -0400, David Kershner wrote:
> > visorbus is currently located at drivers/staging/visorbus,
> > this patch moves it to drivers/virt.
> >
> > Signed-off-by: David Kershner <david.kersh...@unisys.com>
> > Reviewed-by: Tim Sell <timothy.s...@unisys.com>
> > ---
> >  drivers/staging/unisys/Kconfig| 3 
> > +--
> >  drivers/staging/unisys/Makefile   | 1 -
> >  drivers/virt/Kconfig  | 2 
> > ++
> >  drivers/virt/Makefile | 1 +
> >  drivers/{staging/unisys => virt}/visorbus/Kconfig | 0
> >  drivers/{staging/unisys => virt}/visorbus/Makefile| 0
> >  drivers/{staging/unisys => virt}/visorbus/controlvmchannel.h  | 0
> >  drivers/{staging/unisys => virt}/visorbus/controlvmcompletionstatus.h | 0
> >  drivers/{staging/unisys => virt}/visorbus/iovmcall_gnuc.h | 0
> >  drivers/{staging/unisys => virt}/visorbus/vbuschannel.h   | 0
> >  drivers/{staging/unisys => virt}/visorbus/vbusdeviceinfo.h| 0
> >  drivers/{staging/unisys => virt}/visorbus/vbushelper.h| 0
> >  drivers/{staging/unisys => virt}/visorbus/visorbus_main.c | 0
> >  drivers/{staging/unisys => virt}/visorbus/visorbus_private.h  | 0
> >  drivers/{staging/unisys => virt}/visorbus/visorchannel.c  | 0
> >  drivers/{staging/unisys => virt}/visorbus/visorchipset.c  | 0
> 
> I picked one random file here, this last one, and found a number of
> "odd" things in it.
> 
> So, given that I can't really comment on the patch itself, I'm going to
> include the file below, quote it, and then provide some comments, ok?
> > /* visorchipset_main.c
> > ...
> > static void
> > controlvm_init_response(struct controlvm_message *msg,
> > struct controlvm_message_header *msg_hdr, int response)
> > {
> > memset(msg, 0, sizeof(struct controlvm_message));
> > memcpy(>hdr, msg_hdr, sizeof(struct controlvm_message_header));
> > msg->hdr.payload_bytes = 0;
> > msg->hdr.payload_vm_offset = 0;
> > msg->hdr.payload_max_bytes = 0;
> > if (response < 0) {
> > msg->hdr.flags.failed = 1;
> > msg->hdr.completion_status = (u32)(-response);
> > }
> > }
> > 
> > static void
> > Billy(struct controlvm_message_header *msg_hdr, int response)
> 
> Not John?  Bob?  Sally?  Alice?  Bernise?  Jean?  Molly?

Huh? What version of source code are you looking at??

The file being moved by this patch should be
drivers/staging/unisys/visorbus/visorchipset.c in your staging-next branch  
(https://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/unisys/visorbus/visorchipset.c?h=staging-next).
If you look there, you will NOT find "Billy()", but instead 
"controlvm_respond()", i.e.:

static void
controlvm_init_response(struct controlvm_message *msg,
struct controlvm_message_header *msg_hdr, int response)
{
memset(msg, 0, sizeof(struct controlvm_message));
memcpy(>hdr, msg_hdr, sizeof(struct controlvm_message_header));
msg->hdr.payload_bytes = 0;
msg->hdr.payload_vm_offset = 0;
msg->hdr.payload_max_bytes = 0;
if (response < 0) {
msg->hdr.flags.failed = 1;
msg->hdr.completion_status = (u32)(-response);
}
}

static void
controlvm_respond(struct controlvm_message_header *msg_hdr, int response)
{
struct controlvm_message outmsg;

Re: [PATCH 3/3] drivers: Add visorbus to the drivers/virt directory

2016-08-21 Thread Greg KH 
On Fri, Jun 10, 2016 at 11:23:56PM -0400, David Kershner wrote:
> visorbus is currently located at drivers/staging/visorbus,
> this patch moves it to drivers/virt.
> 
> Signed-off-by: David Kershner 
> Reviewed-by: Tim Sell 
> ---
>  drivers/staging/unisys/Kconfig| 3 +--
>  drivers/staging/unisys/Makefile   | 1 -
>  drivers/virt/Kconfig  | 2 ++
>  drivers/virt/Makefile | 1 +
>  drivers/{staging/unisys => virt}/visorbus/Kconfig | 0
>  drivers/{staging/unisys => virt}/visorbus/Makefile| 0
>  drivers/{staging/unisys => virt}/visorbus/controlvmchannel.h  | 0
>  drivers/{staging/unisys => virt}/visorbus/controlvmcompletionstatus.h | 0
>  drivers/{staging/unisys => virt}/visorbus/iovmcall_gnuc.h | 0
>  drivers/{staging/unisys => virt}/visorbus/vbuschannel.h   | 0
>  drivers/{staging/unisys => virt}/visorbus/vbusdeviceinfo.h| 0
>  drivers/{staging/unisys => virt}/visorbus/vbushelper.h| 0
>  drivers/{staging/unisys => virt}/visorbus/visorbus_main.c | 0
>  drivers/{staging/unisys => virt}/visorbus/visorbus_private.h  | 0
>  drivers/{staging/unisys => virt}/visorbus/visorchannel.c  | 0
>  drivers/{staging/unisys => virt}/visorbus/visorchipset.c  | 0

I picked one random file here, this last one, and found a number of
"odd" things in it.

So, given that I can't really comment on the patch itself, I'm going to
include the file below, quote it, and then provide some comments, ok?


> /* visorchipset_main.c
> >  *
>  * Copyright (C) 2010 - 2015 UNISYS CORPORATION
>  * All rights reserved.
>  *
>  * 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 that it will be useful, but
>  * WITHOUT ANY WARRANTY; without even the implied warranty of
>  * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
>  * NON INFRINGEMENT.  See the GNU General Public License for more
>  * details.
>  */
> 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> #include 
> 
> #include "channel_guid.h"
> #include "controlvmchannel.h"
> #include "controlvmcompletionstatus.h"
> #include "guestlinuxdebug.h"
> #include "version.h"

Why do you have this "version.h" file anyway?  It should be deleted, as
parts of it are obviously wrong :(

> #include "visorbus.h"
> #include "visorbus_private.h"
> #include "vmcallinterface.h"

That's loads of "private" .h files, are they all really needed?

> 
> #define CURRENT_FILE_PC VISOR_CHIPSET_PC_visorchipset_main_c

???

> 
> #define MAX_NAME_SIZE 128

name of what?  I don't think you even use this in the file!

> #define MAX_IP_SIZE   50

Huh?  You don't use this either?

> #define MAXOUTSTANDINGCHANNELCOMMAND 256

Here, have a '_', they are free.

But again, I don't see this being used.

> #define POLLJIFFIES_CONTROLVMCHANNEL_FAST   1
> #define POLLJIFFIES_CONTROLVMCHANNEL_SLOW 100

Right-hand alignment?  That's a glutton for punishment.

> 
> #define MAX_CONTROLVM_PAYLOAD_BYTES (1024 * 128)
> 
> #define VISORCHIPSET_MMAP_CONTROLCHANOFFSET   0x
> 
> #define UNISYS_SPAR_LEAF_ID 0x4000
> 
> /* The s-Par leaf ID returns "UnisysSpar64" encoded across ebx, ecx, edx */
> #define UNISYS_SPAR_ID_EBX 0x73696e55
> #define UNISYS_SPAR_ID_ECX 0x70537379
> #define UNISYS_SPAR_ID_EDX 0x34367261
> 
> /*
>  * Module parameters
>  */
> static int visorchipset_major;

major number should not be a module option, just grab a dynamic one and
run with it please.

> static int visorchipset_visorbusregwait = 1;  /* default is on */

Why even have this option?  Who is going to ever change it?  Why would
they?

> static unsigned long controlvm_payload_bytes_buffered;

Not a module option :(

> static u32 dump_vhba_bus;

Not an option, only ever set, never tested :(

> 
> static int
> visorchipset_open(struct inode *inode, struct file *file)
> {
>   unsigned int minor_number = iminor(inode);
> 
>   if (minor_number)
>   return -ENODEV;

What is this check for?  You only ever want one minor number?  If so,
why do you want a whole major number?

>   file->private_data = NULL;

Isn't this already true?

>   return 0;
> }
> 
> static int
> visorchipset_release(struct inode *inode, struct file *file)
> {
>   return 0;
> }

If you do nothing in a release function, then don't provide it at all.

> 
> /*
>  * When the controlvm channel is idle for at least MIN_IDLE_SECONDS,
>  * we switch to slow polling mode. As soon as we get a controlvm
>  * message, we switch back

[PATCH 3/3] drivers: Add visorbus to the drivers/virt directory

2016-06-10 Thread David Kershner 
visorbus is currently located at drivers/staging/visorbus,
this patch moves it to drivers/virt.

Signed-off-by: David Kershner 
Reviewed-by: Tim Sell 
---
 drivers/staging/unisys/Kconfig| 3 +--
 drivers/staging/unisys/Makefile   | 1 -
 drivers/virt/Kconfig  | 2 ++
 drivers/virt/Makefile | 1 +
 drivers/{staging/unisys => virt}/visorbus/Kconfig | 0
 drivers/{staging/unisys => virt}/visorbus/Makefile| 0
 drivers/{staging/unisys => virt}/visorbus/controlvmchannel.h  | 0
 drivers/{staging/unisys => virt}/visorbus/controlvmcompletionstatus.h | 0
 drivers/{staging/unisys => virt}/visorbus/iovmcall_gnuc.h | 0
 drivers/{staging/unisys => virt}/visorbus/vbuschannel.h   | 0
 drivers/{staging/unisys => virt}/visorbus/vbusdeviceinfo.h| 0
 drivers/{staging/unisys => virt}/visorbus/vbushelper.h| 0
 drivers/{staging/unisys => virt}/visorbus/visorbus_main.c | 0
 drivers/{staging/unisys => virt}/visorbus/visorbus_private.h  | 0
 drivers/{staging/unisys => virt}/visorbus/visorchannel.c  | 0
 drivers/{staging/unisys => virt}/visorbus/visorchipset.c  | 0
 drivers/{staging/unisys => virt}/visorbus/vmcallinterface.h   | 0
 17 files changed, 4 insertions(+), 3 deletions(-)
 rename drivers/{staging/unisys => virt}/visorbus/Kconfig (100%)
 rename drivers/{staging/unisys => virt}/visorbus/Makefile (100%)
 rename drivers/{staging/unisys => virt}/visorbus/controlvmchannel.h (100%)
 rename drivers/{staging/unisys => virt}/visorbus/controlvmcompletionstatus.h 
(100%)
 rename drivers/{staging/unisys => virt}/visorbus/iovmcall_gnuc.h (100%)
 rename drivers/{staging/unisys => virt}/visorbus/vbuschannel.h (100%)
 rename drivers/{staging/unisys => virt}/visorbus/vbusdeviceinfo.h (100%)
 rename drivers/{staging/unisys => virt}/visorbus/vbushelper.h (100%)
 rename drivers/{staging/unisys => virt}/visorbus/visorbus_main.c (100%)
 rename drivers/{staging/unisys => virt}/visorbus/visorbus_private.h (100%)
 rename drivers/{staging/unisys => virt}/visorbus/visorchannel.c (100%)
 rename drivers/{staging/unisys => virt}/visorbus/visorchipset.c (100%)
 rename drivers/{staging/unisys => virt}/visorbus/vmcallinterface.h (100%)

diff --git a/drivers/staging/unisys/Kconfig b/drivers/staging/unisys/Kconfig
index 4f1f5e6..dab09a9 100644
--- a/drivers/staging/unisys/Kconfig
+++ b/drivers/staging/unisys/Kconfig
@@ -3,7 +3,7 @@
 #
 menuconfig UNISYSSPAR
bool "Unisys SPAR driver support"
-   depends on X86_64 && !UML
+   depends on X86_64 && !UML && VIRT_DRIVERS
select PCI
select ACPI
---help---
@@ -11,7 +11,6 @@ menuconfig UNISYSSPAR
 
 if UNISYSSPAR
 
-source "drivers/staging/unisys/visorbus/Kconfig"
 source "drivers/staging/unisys/visornic/Kconfig"
 source "drivers/staging/unisys/visorinput/Kconfig"
 source "drivers/staging/unisys/visorhba/Kconfig"
diff --git a/drivers/staging/unisys/Makefile b/drivers/staging/unisys/Makefile
index 20eb098..e45f44b 100644
--- a/drivers/staging/unisys/Makefile
+++ b/drivers/staging/unisys/Makefile
@@ -1,7 +1,6 @@
 #
 # Makefile for Unisys SPAR drivers
 #
-obj-$(CONFIG_UNISYS_VISORBUS)  += visorbus/
 obj-$(CONFIG_UNISYS_VISORNIC)  += visornic/
 obj-$(CONFIG_UNISYS_VISORINPUT)+= visorinput/
 obj-$(CONFIG_UNISYS_VISORHBA)  += visorhba/
diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
index 99ebdde..0c60896 100644
--- a/drivers/virt/Kconfig
+++ b/drivers/virt/Kconfig
@@ -30,4 +30,6 @@ config FSL_HV_MANAGER
   4) A kernel interface for receiving callbacks when a managed
 partition shuts down.
 
+source "drivers/virt/visorbus/Kconfig"
 endif
+
diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
index c47f04d..44aebd2 100644
--- a/drivers/virt/Makefile
+++ b/drivers/virt/Makefile
@@ -3,3 +3,4 @@
 #
 
 obj-$(CONFIG_FSL_HV_MANAGER)   += fsl_hypervisor.o
+obj-$(CONFIG_UNISYS_VISORBUS)  += visorbus/
diff --git a/drivers/staging/unisys/visorbus/Kconfig 
b/drivers/virt/visorbus/Kconfig
similarity index 100%
rename from drivers/staging/unisys/visorbus/Kconfig
rename to drivers/virt/visorbus/Kconfig
diff --git a/drivers/staging/unisys/visorbus/Makefile 
b/drivers/virt/visorbus/Makefile
similarity index 100%
rename from drivers/staging/unisys/visorbus/Makefile
rename to drivers/virt/visorbus/Makefile
diff --git a/drivers/staging/unisys/visorbus/controlvmchannel.h 
b/drivers/virt/visorbus/controlvmchannel.h
similarity index 100%
rename from drivers/staging/unisys/visorbus/controlvmchannel.h
rename to drivers/virt/visorbus/controlvmchannel.h
diff --git a/drivers/staging/unisys/visorbus/controlvmcompletionstatus.h