Re: [PATCH 00/10] parser header and c file patches
On Mon, 2014-12-01 at 07:26 -0500, Jeffrey Brown wrote: > This series of patches made corrections to the camel casing and typedef > problems in parser header and parser.c. There is a camel case in parser header > where visorchipset_main.c was affected as well. Smaller problems like space > after casts, braces, and logical coninuations in parser.c were solved as well. > > Jeffrey Brown (10): > staging: unisys: parser_init camel cases > staging: unisys: parser_init_byteStream camel case > staging: unisys: simpleString_get & byteStream_get camel case > staging: unisys: PARSER_CONTEXT typedef > staging: unisys: PARSER_WHICH_STRING typedef > staging: unisys: Controlvm_Payload_Bytes_Buffered camel case > staging: unisys: parser_init_guts and parser_param_start camel case > staging: unisys: parser.c space after casts > staging: unisys: parser.c braces > staging: unisys: parser.c logical continuation > > drivers/staging/unisys/visorchipset/parser.c | 117 > +++-- > drivers/staging/unisys/visorchipset/parser.h | 29 ++--- > .../unisys/visorchipset/visorchipset_main.c| 12 ++- > 3 files changed, 81 insertions(+), 77 deletions(-) > Greg, Please disregard this patchset - Jeffrey was not aware of your previous email and sent this before I was able to talk to anyone. Sorry for the trouble. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: unisys: remove duplicate header
On Tue, 2014-12-02 at 16:20 +0530, Sudip Mukherjee wrote: > these header files were included multiple times > > Signed-off-by: Sudip Mukherjee Looks good to me, thanks for the help. :) Signed-off-by: Benjamin Romer ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch 1/8] staging: visorutil driver to provide common functionality to other s-Par drivers
Hi Greg,
The copyright text is old boilerplate that we'd been carrying. We'll
change it so that it's correctly GPL 2 only, and we'll fix all of the
EXPORT_SYMBOL()s also.
The majority of our proc entries are switches for debugging or purely
informational, so these could be moved to /sys with little trouble. The
only user space program that would care about them is our guest
installation software, and we at Unisys can easily update that when the
API changes.
-- Ben
-Original Message-
From: Greg KH
To: Ken Cox
Subject: Re: [patch 1/8] staging: visorutil driver to provide common
functionality to other s-Par drivers
Date: Mon, 3 Mar 2014 11:15:13 -0800
On Mon, Mar 03, 2014 at 11:30:19AM -0600, Ken Cox wrote:
> +/* periodic_work.h
> + *
> + * Copyright © 2010 - 2013 UNISYS CORPORATION
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or (at
> + * your option) any later version.
I have to ask, do you really mean "any later version" here?
> +CHARQUEUE *charqueue_create(ulong nslots)
> +{
> +int alloc_size = sizeof(CHARQUEUE) + nslots + 1;
> +CHARQUEUE *cq = kmalloc(alloc_size, GFP_KERNEL|__GFP_NORETRY);
> +if (cq == NULL) {
> +ERRDRV("charqueue_create allocation failed (alloc_size=%d)",
> + alloc_size);
> +return NULL;
> +}
> +cq->alloc_size = alloc_size;
> +cq->nslots = nslots;
> +cq->head = cq->tail = 0;
> +spin_lock_init(&cq->lock);
> +return cq;
> +}
> +EXPORT_SYMBOL(charqueue_create);
For staging drivers, I really want to see EXPORT_SYMBOL_GPL() for the
exports for a variety of reasons dealing with historical issues...
Can you change all of these please?
And how "wed" are you to the proc interface, that is going to change,
right? What userspace tools rely on it and who is going to be
responsible for changing them as the user/kernel api changes with this
code?
thanks,
g
greg k-h
--
Ben Romer | Software Engineer |
Virtual Systems Development
Unisys Corporation | 2476
Swedesford Rd | Malvern, PA 19355
| 610-648-7140
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/1] staging: unisys: Fix MAINTAINERS
This patch updates the MAINTAINERS file to add the maintainer for the Unisys s-Par driver set. Signed-off-by: Ken Cox signed-off-by: Ben Romer diff --git a/MAINTAINERS b/MAINTAINERS index c3ff623..06dc169 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9031,6 +9031,11 @@ F: include/linux/cdrom.h F: include/uapi/linux/cdrom.h +UNISYS S-PAR DRIVERS +M: Ben Romer +S: Maintained +F: drivers/staging/unisys/ + UNIVERSAL FLASH STORAGE HOST CONTROLLER DRIVER M: Vinayak Holikatti M: Santosh Y ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] staging: unisys: Fix MAINTAINERS and TODO
On Thu, 2014-03-06 at 08:47 -0600, Ken Cox wrote: > On 03/05/2014 10:40 PM, Greg KH wrote: > > On Wed, Mar 05, 2014 at 02:52:26PM -0600, Ken Cox wrote: > >> Add the Unisys s-Par maintainer entry to the MAINTAINERS file. > >> Add Greg Kroah-Hartman to the list of patch recipients in the TODO file > >> > >> Signed-off-by: Ken Cox > >> --- > >> MAINTAINERS| 5 + > >> drivers/staging/unisys/MAINTAINERS | 6 -- > >> drivers/staging/unisys/TODO| 1 + > >> 3 files changed, 6 insertions(+), 6 deletions(-) > >> delete mode 100644 drivers/staging/unisys/MAINTAINERS > >> > >> diff --git a/MAINTAINERS b/MAINTAINERS > >> index c3ff623..06dc169 100644 > >> --- a/MAINTAINERS > >> +++ b/MAINTAINERS > >> @@ -9031,6 +9031,11 @@ F: drivers/cdrom/cdrom.c > >> F: include/linux/cdrom.h > >> F: include/uapi/linux/cdrom.h > >> > >> +UNISYS S-PAR DRIVERS > >> +M:Ben Romer > >> +S:Maintained > >> +F:drivers/staging/unisys/ > > Why can't Ben submit this? I'd prefer that there is at least some proof > > that he can submit valid patches that can be accepted :) > I'll talk to Ben and get him to submit. Will do. :) ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] staging: unisys: Fix MAINTAINERS
On Thu, 2014-03-06 at 12:01 -0800, gre...@linuxfoundation.org wrote: > On Thu, Mar 06, 2014 at 10:01:04AM -0600, Romer, Benjamin M wrote: > > This patch updates the MAINTAINERS file to add the maintainer for the > > Unisys s-Par driver set. > > > > Signed-off-by: Ken Cox > > signed-off-by: Ben Romer > > Please capitalize things properly. > > And your From: address does not match your "Signed-off-by:" line. Why > are you trying to use an "alias" as a maintainer or a sign-off? We want > a "real" address / name please, especially as it's obvious you have one :) > > thanks, > > greg k-h The email address I listed is a group address - I'll change the contents of the file so they more accurately describe how we've set things up. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] staging: unisys: Fix MAINTAINERS
On Fri, 2014-03-07 at 17:41 +0300, Dan Carpenter wrote: > On Fri, Mar 07, 2014 at 08:18:44AM -0600, Romer, Benjamin M wrote: > > > > On Thu, 2014-03-06 at 12:01 -0800, gre...@linuxfoundation.org wrote: > > > On Thu, Mar 06, 2014 at 10:01:04AM -0600, Romer, Benjamin M wrote: > > > > This patch updates the MAINTAINERS file to add the maintainer for the > > > > Unisys s-Par driver set. > > > > > > > > Signed-off-by: Ken Cox > > > > signed-off-by: Ben Romer > > > > > > Please capitalize things properly. > > > > > > And your From: address does not match your "Signed-off-by:" line. Why > > > are you trying to use an "alias" as a maintainer or a sign-off? We want > > > a "real" address / name please, especially as it's obvious you have one :) > > > > > > thanks, > > > > > > greg k-h > > > > The email address I listed is a group address - I'll change the contents of > > the file > > so they more accurately describe how we've set things up. > > The file is fine (I think). > > Signed-off-by is like signing a legal document. You should sign on your > own behalf and not for a group of people. And if the Signed-off-by > doesn't match your From then how do we know it's really you? > > regards, > dan carpenter > That's what I mean though, the file isn't quite accurate. sparmaintai...@unisys.com is a group email address for which I'm the primary contact, but there are other developers and managers with access to it. I'll list myself and one other person by email address for handling patches, and put the group address as the mailing list entry. Also, the file should say "Supported" instead of "Maintained", so that wasn't quite right either. :) ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/1] staging: unisys: update MAINTAINERS and TODO
This patch adds the Unisys s-Par driver maintainers to the MAINTAINERS file, changes the state to "Supported", modifies TODO to address patches to the Unisys mailing list, and adds Greg Kroah-Hartman to the patch recipients list. Signed-off-by: Benjamin Romer --- diff --git a/MAINTAINERS b/MAINTAINERS index c3ff623..405c4c9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9031,6 +9031,13 @@ F: drivers/cdrom/cdrom.c F: include/linux/cdrom.h F: include/uapi/linux/cdrom.h +UNISYS S-PAR DRIVERS +M: Benjamin Romer +M: David Kershner +L: sparmaintai...@unisys.com (Unisys internal) +S: Supported +F: drivers/staging/unisys/ + UNIVERSAL FLASH STORAGE HOST CONTROLLER DRIVER M: Vinayak Holikatti M: Santosh Y diff --git a/drivers/staging/unisys/TODO b/drivers/staging/unisys/TODO index c4265a2..034ac61 100644 --- a/drivers/staging/unisys/TODO +++ b/drivers/staging/unisys/TODO @@ -16,5 +16,6 @@ TODO: Patches to: + Greg Kroah-Hartman Ken Cox - Ben Romer + Unisys s-Par maintainer mailing list ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [visorchipset] invalid opcode: 0000 [#1] PREEMPT SMP
On Tue, 2014-04-08 at 10:53 +0800, Fengguang Wu wrote: > Hi Benjamin, > > > Fengguang, > > > > I ran your script against freshly-checked-out source from staging-next, and > > was not able to reproduce the error with it. My boot log is attached. I > > noticed that your log did not have "Hypervisor detected: KVM" in the trace. > > The KVM options in your script also differ substantially from the ones > > shown at the end of your trace... > > > When I reran your script with the "-cpu Haswell,+smep,+smap" option I was > > able to get the same result as you. IMHO KVM should not be setting this bit > > if it's emulating bare metal. > > Sorry.. We tried to provide a simplified reproduce script and in your > case, it has a significant mismatch with the real KVM options. We'll > fix it, thanks for pointing it out! > > Thanks, > Fengguang That will be helpful, and as I mentioned, I can reproduce your results, but I'm still not sure why a virtualized processor is giving an invalid opcode fault on a vmcall. The Intel documentation is pretty specific about this - IF not in VMX operation THEN #UD; ELSIF in VMX non-root operation THEN VM exit. Either KVM should be saying "I'm a real processor and not a virtual CPU, really!" - in which case, the hypervisor bit should be off and vmcalls should cause an invalid opcode fault, or, KVM should be saying "I'm a vritualized processor!" and setting the hypervisor bit, and doing a vmexit on vmcall instead. This seems like a KVM bug to me. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] unisys: staging: Check for s-Par firmware before initializing s-Par modules
This patch adds a function, is_spar_system(), to check that s-Par
firmware is present, and then uses this function at the beginning of
each module to verify that the modules are being run on an s-Par system
before beginning initialization. If the firmware is not detected, the
module will return a failure code.
Checking for s-Par firmware uses the cpuid instruction to verify that
the processor is running with virtualization turned on, and then uses a
second cpuid instruction to check that the hypervisor ID is
"UnisysSpar64".
Additionally, some minor clean-up was done on copyright tags and
unnecessary messages were removed from the visorchipset_main() function.
This patch was tested with KVM to ensure that the modules do not load
when s-Par firmware is not present, and tested with s-Par 4.0.12 to
verify that the modules load correctly when the firmware is present.
Signed-off-by: Benjamin Romer
---
diff --git a/drivers/staging/unisys/include/timskmodutils.h
b/drivers/staging/unisys/include/timskmodutils.h
index 2d81d46..cc439d3 100644
--- a/drivers/staging/unisys/include/timskmodutils.h
+++ b/drivers/staging/unisys/include/timskmodutils.h
@@ -1,6 +1,6 @@
/* timskmodutils.h
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -71,5 +71,6 @@
u64 somethings, char *buf, size_t bufsize);
struct seq_file *visor_seq_file_new_buffer(void *buf, size_t buf_size);
void visor_seq_file_done_buffer(struct seq_file *m);
+int is_spar_system( void );
#endif
diff --git a/drivers/staging/unisys/uislib/uislib.c
b/drivers/staging/unisys/uislib/uislib.c
index 8ea9c46..aa60ccb 100644
--- a/drivers/staging/unisys/uislib/uislib.c
+++ b/drivers/staging/unisys/uislib/uislib.c
@@ -1,6 +1,6 @@
/* uislib.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -2276,6 +2276,11 @@
static int __init
uislib_mod_init(void)
{
+ /* check for s-Par support */
+ if( !is_spar_system() ) {
+ printk( "s-Par not detected.\n" );
+ return -EPERM;
+ }
LOGINF("MONITORAPIS");
diff --git a/drivers/staging/unisys/virthba/virthba.c
b/drivers/staging/unisys/virthba/virthba.c
index 817b11d..862509d 100644
--- a/drivers/staging/unisys/virthba/virthba.c
+++ b/drivers/staging/unisys/virthba/virthba.c
@@ -1,6 +1,6 @@
/* virthba.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -1691,6 +1691,12 @@
int error;
int i;
+ /* check for s-Par support */
+ if( !is_spar_system() ) {
+ printk( "s-Par not detected.\n" );
+ return -EPERM;
+ }
+
LOGINF("Entering virthba_mod_init...\n");
POSTCODE_LINUX_2(VHBA_CREATE_ENTRY_PC, POSTCODE_SEVERITY_INFO);
diff --git a/drivers/staging/unisys/virtpci/virtpci.c
b/drivers/staging/unisys/virtpci/virtpci.c
index 8e34650..0d06f7e 100644
--- a/drivers/staging/unisys/virtpci/virtpci.c
+++ b/drivers/staging/unisys/virtpci/virtpci.c
@@ -1,6 +1,6 @@
/* virtpci.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -21,6 +21,7 @@
#ifdef CONFIG_MODVERSIONS
#include
#endif
+#include "timskmodutils.h"
#include "uniklog.h"
#include "diagnostics/appos_subsystems.h"
#include "uisutils.h"
@@ -1686,6 +1687,11 @@
{
int ret;
+ /* check for s-Par support */
+ if( !is_spar_system() ) {
+ printk( "s-Par not detected.\n" );
+ return -EPERM;
+ }
LOGINF("Module build: Date:%s Time:%s...\n", __DATE__, __TIME__);
diff --git a/drivers/staging/unisys/visorchipset/visorchipset_main.c
b/drivers/staging/unisys/visorchipset/visorchipset_main.c
index 8252ca1..aa35aa2 100644
--- a/drivers/staging/unisys/visorchipset/visorchipset_main.c
+++ b/drivers/staging/unisys/visorchipset/visorchipset_main.c
@@ -1,6 +1,6 @@
/* visorchipset_main.c
*
- * Copyright � 2010 - 2013 UNISYS CORPORATION
+ * Copyright © 2010 - 2013 UNISYS CORPORATION
* All rights reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -2681,18 +2681,13 @@
struct proc_dir_entry *toolaction_file;
struct proc_dir_entry *bootToTool_file;
- LOGINF("chipset driver version %s loaded", VERSION);
- /* process module options */
- POSTCODE_LINUX_2(DRIVER_ENTRY_PC, POSTCODE_SEVERITY_INFO);
+ /* check for s-Par support */
+ if( !is_spar_system() ) {
+ printk( "s-Par
Re: [PATCH] unisys: staging: Check for s-Par firmware before initializing s-Par modules
On Wed, 2014-04-09 at 12:25 -0700, Greg Kroah-Hartman wrote: > Patches need to do only one thing, so can you please split this up in to > multiple patches, each one only doing one thing. Sorry about that! I'll send another patch that fixes all of the copyright statements at once then. :) > You forgot to add a "Reported-by:" line here, and possibly a > "Tested-by:" if someone tested it and reported that it solved the > problem. Proper attribution is very important. Will do! Thanks for all the feedback. I'll rework my patch. :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: unisys: fix copyright notices
This patch replaces the inconsistent copyright symbols in each source file with (C). Signed-off-by: Benjamin Romer --- drivers/staging/unisys/channels/channel.c | 2 +- drivers/staging/unisys/channels/chanstub.c | 2 +- drivers/staging/unisys/channels/chanstub.h | 2 +- drivers/staging/unisys/common-spar/include/channels/channel.h | 2 +- drivers/staging/unisys/common-spar/include/channels/channel_guid.h | 2 +- drivers/staging/unisys/common-spar/include/channels/controlframework.h | 2 +- drivers/staging/unisys/common-spar/include/channels/controlvmchannel.h | 2 +- drivers/staging/unisys/common-spar/include/channels/diagchannel.h | 2 +- drivers/staging/unisys/common-spar/include/channels/iochannel.h | 2 +- drivers/staging/unisys/common-spar/include/channels/vbuschannel.h | 2 +- drivers/staging/unisys/common-spar/include/controlvmcompletionstatus.h | 2 +- .../staging/unisys/common-spar/include/diagnostics/appos_subsystems.h | 2 +- drivers/staging/unisys/common-spar/include/iovmcall_gnuc.h | 2 +- drivers/staging/unisys/common-spar/include/vbusdeviceinfo.h | 2 +- drivers/staging/unisys/common-spar/include/version.h| 2 +- drivers/staging/unisys/common-spar/include/vmcallinterface.h| 2 +- drivers/staging/unisys/include/commontypes.h| 2 +- drivers/staging/unisys/include/guestlinuxdebug.h| 2 +- drivers/staging/unisys/include/guidutils.h | 2 +- drivers/staging/unisys/include/periodic_work.h | 2 +- drivers/staging/unisys/include/procobjecttree.h | 2 +- drivers/staging/unisys/include/sparstop.h | 2 +- drivers/staging/unisys/include/timskmod.h | 2 +- drivers/staging/unisys/include/timskmodutils.h | 2 +- drivers/staging/unisys/include/uisqueue.h | 2 +- drivers/staging/unisys/include/uisthread.h | 2 +- drivers/staging/unisys/include/uisutils.h | 2 +- drivers/staging/unisys/include/uniklog.h| 2 +- drivers/staging/unisys/uislib/uislib.c | 2 +- drivers/staging/unisys/uislib/uisqueue.c| 2 +- drivers/staging/unisys/uislib/uisthread.c | 2 +- drivers/staging/unisys/uislib/uisutils.c| 2 +- drivers/staging/unisys/virthba/virthba.c| 2 +- drivers/staging/unisys/virthba/virthba.h| 2 +- drivers/staging/unisys/virtpci/virtpci.c| 2 +- drivers/staging/unisys/virtpci/virtpci.h| 2 +- drivers/staging/unisys/visorchannel/globals.h | 2 +- drivers/staging/unisys/visorchannel/visorchannel.h | 2 +- drivers/staging/unisys/visorchannel/visorchannel_funcs.c| 2 +- drivers/staging/unisys/visorchannel/visorchannel_main.c | 2 +- drivers/staging/unisys/visorchipset/controlvm.h | 2 +- drivers/staging/unisys/visorchipset/controlvm_direct.c | 2 +- drivers/staging/unisys/visorchipset/file.c | 2 +- drivers/staging/unisys/visorchipset/file.h | 2 +- drivers/staging/unisys/visorchipset/globals.h | 2 +- drivers/staging/unisys/visorchipset/parser.c| 2 +- drivers/staging/unisys/visorchipset/parser.h| 2 +- drivers/staging/unisys/visorchipset/testing.h | 2 +- drivers/staging/unisys/visorchipset/visorchipset.h | 2 +- drivers/staging/unisys/visorchipset/visorchipset_main.c | 2 +- drivers/staging/unisys/visorchipset/visorchipset_umode.h| 2 +- drivers/staging/unisys/visorutil/charqueue.c| 2 +- drivers/staging/unisys/visorutil/charqueue.h| 2 +- drivers/staging/unisys/visorutil/easyproc.c | 2 +- drivers/staging/unisys/visorutil/easyproc.h | 2 +- drivers/staging/unisys/visorutil/memregion.h| 2 +- drivers/staging/unisys/visorutil/memregion_direct.c | 2 +- drivers/staging/unisys/visorutil/periodic_work.c| 2 +- drivers/staging/unisys/visorutil/procobjecttree.c | 2 +- drivers/staging/unisys/visorutil/visorkmodutils.c | 2 +- 60 files changed, 60 insertions(+), 60 deletions(-) diff --git a/drivers/sta
Re: [PATCH 55/69] staging: unisys: get rid of LOGWRN() macro and uisklog.h
On Fri, 2015-01-09 at 17:40 -0800, Greg KH wrote: > On Fri, Dec 05, 2014 at 05:09:30PM -0500, Benjamin Romer wrote: > > Get rid of LOGWRN() and all related macros, and call pr_warn() directly > > instead. > > Side note, are you setting pr_fmt() properly so that everything is > "unified" with these messages? No driver subsystem should ever use a > pr_* call unless it is at startup / shutdown where there is no hardware > involved. I think that's not the case here so use the correct dev_* > versions instead. > > thanks, > > greg k-h Greg, You're absolutely right that almost all of the messages being output in our code aren't useful from a user's perspective, so what I'd like to do is to just remove all of the logging macros in one set of patches, and then re-introduce a small number of properly-coded error messages in appropriate spots in a later set of patches. Would that be okay? If so, is it alright to take out one set of macros per patch, or is that too much at once? Thanks! -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 0/5] unisys: kthread cleanup
On Wed, 2015-01-21 at 04:03 -0500, Devendra Naga wrote: > The 1st patch (replace kthread_create..) replace the > kthread_create and wake_up_process with a call to > kthread_run. > > The 2nd patch changes the library API uisthread_start > and uisthread_stop to use the kthread API. > > The 3rd patch and 4th patch checks on kthread_should_stop > instead of checking should_stop variable. > > The 5th patch removes the variables should_stop and > KILL as they are no longer required. > > > All patches applies on next-20150120 cleanly. All the patches are > compile tested on X86_64 allmodconfig. > Hi, I've built and tested your set of patches on an s-Par guest, and with the changes I get a kernel oops during boot, but I haven't determined the cause yet. I'll let you know what I find. :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 55/69] staging: unisys: get rid of LOGWRN() macro and uisklog.h
On Thu, 2015-01-22 at 11:19 +0800, Greg KH wrote: > On Wed, Jan 21, 2015 at 06:53:31PM +0300, Dan Carpenter wrote: > > Generally "delete code" patches are easy to review. But sometimes you > > have to change formatting and remove variables and curly braces. > > > > $ grep LOG drivers/staging/unisys/ -R | wc -l > > 415 > > > > There isn't a firm rule on way a patch is just too big and annoying to > > review. Probably break it up into: > > > > [patch 1/x] delete LOGINF() > > [patch 2/x] delete LOGERR() > > ... > > Yes, this would be best. > > Always remember, what would you like to have to review if you were the > receiver of patches? > > thanks, > > greg k-h Thank you both for the feedback. :) I'll do one patch per macro. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: unisys: rework signal remove/insert to avoid sparse lock warnings
On Sun, 2015-01-25 at 19:48 +0800, Greg KH wrote: > On Sat, Jan 17, 2015 at 10:39:53PM +1100, Zachary Warren wrote: > > Avoids the following warnings from sparse: > > visorchannel_funcs.c:457:9: warning: > > context imbalance in 'visorchannel_signalremove' - different lock contexts > > for basic block > > visorchannel_funcs.c:512:9: warning: > > context imbalance in 'visorchannel_signalinsert' - different lock contexts > > for basic > Benjamin, I need an ack from you before I can take this. > > thanks, > > greg k-h This one looks good, thanks! Sorry about the late reply. --Ben Acked-by: Benjamin Romer ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 0/5] unisys: kthread cleanup
On Mon, 2015-01-26 at 00:39 -0500, devendra.aaru wrote: > Hello, > > Sorry for the late reply. > > Would reverting last two patches help ? > > Thanks, > Devendra Devendra, I am really sorry for how long this is taking. I don't think that there's anything wrong with your patches, but rather that something else that was changed recently is causing it not to boot on s-Par. Taking the last two out didn't make any difference so I don't think these are causing the problem. If you don't mind, I'd like to take these patches and hang on to them until I find and fix the problem and then I will resubmit them with my sign-off. Is that okay? :) -- Ben Romer | Software Engineer | Virtual Systems Development Unisys Corporation | 2476 Swedesford Rd | Malvern, PA 19355 | 610-648-7140 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/30] staging: unisys: serverdown variable change bool to int virthba
On Wed, 2015-02-11 at 11:36 +0300, Dan Carpenter wrote: > On Tue, Feb 10, 2015 at 12:58:35PM -0500, Benjamin Romer wrote: > > From: Erik Arfvidson > > > > This patch changes serverdown variable to int instead of bool > > > > Why? It looks like bool is more appropriate? Hi Dan, We had received some comments on our code that said that our BOOL typedef wasn't acceptable, and that we really ought to be returning 0 for success and error values in failure cases. By switching these to int we're taking a first step towards that. -- Ben Romer | Software Engineer | Virtual Systems Development Unisys Corporation | 2476 Swedesford Rd | Malvern, PA 19355 | 610-648-7140 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: unisys: Rework Kconfig dependencies
On Sat, 2015-02-21 at 13:16 +0100, Jean Delvare wrote: > Signed-off-by: Jean Delvare > Cc: Benjamin Romer > Cc: David Kershner > Cc: Greg Kroah-Hartman > --- > This is an humble proposal if you like it. I don't use the driver > myself, I don't even know what it is for ;-) > I like this idea a lot. Thanks!! :) -- Ben Signed-off-by: Benjamin Romer ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/5] staging: unisys: remove DBGINF, DBGVER, DEBUGDEV, and DEBUGDRV macros
On Thu, 2015-02-26 at 18:07 +0300, Dan Carpenter wrote: > Gar... Actually everything is double tabbed. What's the deal with > that? There are other examples as well. > > regards, > dan carpenter > I'll fix these and resubmit the patch set. -- Ben Romer | Software Engineer | Virtual Systems Development Unisys Corporation | 2476 Swedesford Rd | Malvern, PA 19355 | 610-648-7140 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/5] staging: unisys: remove LOGINF macros
On Thu, 2015-02-26 at 20:19 +0300, Dan Carpenter wrote: > > + wake_up_process(thrinfo->task); > Please send bug fixes separately. > > regards, > dan carpenter > Actually, I think this is a merge error on my part, I'll send a v3 of just this specific patch if that's alright? -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/5] staging: unisys: remove ERRDEV macros
On Thu, 2015-02-26 at 20:37 +0300, Dan Carpenter wrote: > There is only one place which calls this ASSERT() macro. It should > probably be changed to: > > WARN_ON(!pw->is_scheduled); I can do that in a subsequent patch, but my intent was to remove the logging macros, not to fix the functions using them. If I have to fix the functions first before removing the macros it'd be a better use of time to just remove them as part of refactoring each function, and not try to do an across the board patch like this. > Double tab. Yes, I missed a couple - I will fix these... > Is this really necessary? I don't know for sure since I'm not the original author of this code, all I was doing was removing the logging macros. I can try to find the person who wrote it and see if the function can be simplified any, but again, if each function I touch needs to be fixed before I can remove the logging, there's no point trying to remove the logging macros across the board. > Gotta run... Enough reviewing for tonight. Thanks, I appreciate the help. :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/6] staging: unisys: remove DBGINF, DBGVER, DEBUGDEV, and DEBUGDRV macros
On Wed, 2015-03-04 at 18:59 +0300, Dan Carpenter wrote: > I don't think you meant to delete this line. No, I didn't, thanks for the catch. I'll fix both of these problems and resubmit. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 5/6] staging: unisys: remove ERRDEV macros
On Wed, 2015-03-04 at 19:34 +0300, Dan Carpenter wrote: > Btw, I'm these patches are quite large. You're going to have to redo > them to fix the three behavior changes. I'm hoping I can just diff the > two patchsets instead of reviewing everything again so please redo them > within the next couple days so that we don't merge other changes in the > mean time. I would like minimal unrelated changes between this patchset > and the next version. > > regards, > dan carpenter I intend to have these changes made and resubmitted today, and there should not be any changes other than the ones you've mentioned. Thanks for the eagle eye on these, they are large and I really appreciate you spending your time helping me with them. :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 0/6] staging: unisys: remove logging macros from all drivers
On Wed, 2015-03-04 at 22:11 +0300, Dan Carpenter wrote: > Looks good to me. Thanks! > > regards, > dan carpenter > Thank you for all the help!!! :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/7] staging: unisys: move uislib/platform proc entry to debugfs
On Sun, 2014-05-18 at 09:49 -0700, Greg KH wrote: > There's no need to keep this dentry around, you can just remove all the > debugfs files in your directory recursively when you exit. That will > save you code and logic overall. Ah, thanks! I wasn't aware that I could do that. That's an easy fix. :) > I'll leave this for now, you can clean that up in a future patch. Thanks! I'll do that. :) > Also, why are these entries moving to debugfs at all? Why are they > needed? Who will use them? Are tools relying on them to be there? The tuning entries are sometimes used to help adjust the behavior of our IO service partitions for better performance. I'm not sure about the platform entry, but I'll find out if anyone still uses it. If not, I'll send another patch to remove it entirely. > thanks, > > greg k-h -- Ben Romer | Software Engineer | Virtual Systems Development Unisys Corporation | 2476 Swedesford Rd | Malvern, PA 19355 | 610-648-7140 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/7] staging: unisys: move uislib/platform proc entry to debugfs
On Tue, 2014-05-20 at 10:09 +0900, Greg KH wrote: > On Mon, May 19, 2014 at 10:57:08PM +0300, Dan Carpenter wrote: > > On Mon, May 19, 2014 at 09:42:22AM -0500, Romer, Benjamin M wrote: > > > On Sun, 2014-05-18 at 09:49 -0700, Greg KH wrote: > > > > Also, why are these entries moving to debugfs at all? Why are they > > > > needed? Who will use them? Are tools relying on them to be there? > > > > > > The tuning entries are sometimes used to help adjust the behavior of our > > > IO service partitions for better performance. > > > > That sounds like it really belongs in sysfs instead of debugfs. > > Exactly. debugfs files are for "debugging". Consider them files that > your driver can work properly if no one ever touches them. > That is what I was trying to say - these are used when someone is changing the behavior of IO service partitions, not the guest partition where the driver is running. No typical user will need or want to change these settings - only someone working on the IO *server side* performance will need access to them. All of our drivers communicate with another partition to perform IO on shared devices. By turning off features, or changing the rate of messaging in the channels, it makes it easier for someone working on the IO service partition to tune the performance there. The guest itself isn't being tuned. > "tuning" files imply something that has to be touched by users. > Ideally, you would never need such a thing as no one wants to have to > write things to files to make the kernel work better. That is indeed what we want - a user should not need to touch these settings. Someone manipulating these particular settings would be tweaking performance on the server end, like I was trying to say. I believe that most of our proc entries are really debug-time tweaks of that sort, and not something a typical user would ever want to touch. > But if you really > need it, they should be sysfs files, with the needed documentation. > > thanks, > > greg k-h Sorry about the confusion! -- Ben Romer | Software Engineer | Virtual Systems Development Unisys Corporation | 2476 Swedesford Rd | Malvern, PA 19355 | 610-648-7140 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Fri, 2014-09-05 at 14:52 +0530, Sudip Mukherjee wrote: > fixed sparse warning : context imbalance in 'pause_device' > unexpected unlock > this patch will generate warning from checkpatch for > lines over 80 character , but since those are user-visible strings > so it was not modified. > > Signed-off-by: Sudip Mukherjee > --- > > hi , can you please review the patch and see if the approach is correct. > The functiion is still doing the same what it was doing , only the logic > is changed. if the approach is ok, then i can send a patch to fix the > other two similar warning in the file. Hi Sudip, I was able to successfully build and test your patch. The changes look good to me too, so I think we should take this patch. :) Thanks! -- Ben Signed-off-by: Benjamin Romer Tested-by: Benjamin Romer ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] unisys: Fix sparse error - accessing __iomem directly
On Mon, 2014-09-08 at 17:04 +0300, Dan Carpenter wrote: > On Mon, Sep 08, 2014 at 01:44:49PM +0100, Luke Hart wrote: > > Copy the channel type into a temporary buffer so that code will work for > > architectures that don't support MMIO. This now works in same way as other > > tests in same function. > > > > Re-sent since I initially didn't copy de...@driverdev.osuosl.org and > > sparmaintai...@unisys.com doesn't seem to be a mailing list (despite what > > get_maintainer.pl indicates). > > The MAINTAINERS entry is: > L: sparmaintai...@unisys.com (Unisys internal) > > I believe that it actually is a private list as described. We discussed > the sparmaintainer email address in the past. Yes, this is an internal address that goes to several people inside of Unisys. > Otherwise the change seems sensible to me but I'm not terribly familiar > with the code so I'll defer to Ben on this. It looks good to me as well, and I can test it on x86_64, however the s-Par firmware only runs on Intel 64-bit systems so there's no way to test it on another architecture. A successful compilation is all I can give you there. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Mon, 2014-09-08 at 21:57 +0530, Sudip Mukherjee wrote: > Hi Ben, > thanks. the same file is having two more similar warnings. if you want i can > resend a patch fixing all the three warnings , or i can send two separate > patches. > I personally will prefer two separate patches , as that will be > easier for you to test and review. > > thanks > sudip That would be perfect, thanks. :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Mon, 2014-09-08 at 22:08 +0530, Sudip Mukherjee wrote: > Hi Ben, > sorry to disturb you again. i got confused , which one is perfect one > combined patch or > separate patches? > > thanks > sudip Two patches, as you preferred. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Mon, 2014-09-08 at 10:06 -0700, Greg Kroah-Hartman wrote: > Traditionally, you would respond with a: > Acked-by: Developer Name > so I can add it to the patch. > > Care to do that here? > > thanks, > > greg k-h Of course. :) Acked-by: Benjamin Romer Should I do this instead of Tested-by and Signed-off-by, in the future? -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Mon, 2014-09-08 at 10:24 -0700, Greg Kroah-Hartman wrote: > If you test it, do a tested-by. > > If you sign off on it (i.e. it flows through you to me), then a > signed-off is correct. > > Hope this helps, > > greg k-h It does. :) We should add the Tested-by then, too. Acked-by: Benjamin Romer Tested-by: Benjamin Romer -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: unisys: uislib: uislib.c: sparse warning of context imbalance
On Tue, 2014-09-09 at 16:11 +0530, Sudip Mukherjee wrote: > fixed sparse warning : context imbalance in 'resume_device' > unexpected unlock > this patch will generate warning from checkpatch for > lines over 80 character , but since those are user-visible strings > so it was not modified. > > Signed-off-by: Sudip Mukherjee I tested your changes, and this patch works fine. Thanks! -- Ben Acked-by: Benjamin Romer Tested-by: Benjamin Romer ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Staging: unisys: base drivers complete
> Have you actually ran the checkpatch.pl tool on this code? You still
> have a lot of cleanup to do (hint, typedefs for drivers are not
> allowed...)
Yes, I've been using checkpatch.pl a lot, though admittedly I did not
know about --strict. I'll start addressing the check issues as well as
the warnings and errors.
About the "WARNING: do not add new typedefs" messages that are
generated, I have a question about what typedefs are permitted. It's
easy enough to replace "typedef enum {...} x;" with "enum x {...}", but
there are a lot of typedef struct declarations throughout the s-Par
driver code used to give clearer names to internally-used structures. I
don't know of any kernel-defined types being renamed anymore, and when I
did a grep of the driver tree to see what other drivers were doing in
this area, I found lots of drivers with "typedef struct" in them.
Are we restricted from doing *any* typedefs at all? If not, could you
give me a good guideline to follow?
> And what bout the TODO file? The first 3 items are not completed. Why
> even have a TODO file if you aren't going to look at it? :(
I'd like to expand it to include as much as we can, so when people ask
me "when will our drivers be done?" or "what can I work on?" I have
something they can read. :) It also helps us fix up the code we haven't
submitted yet, and if we can get it into shape before you have to see
it, that saves you a lot of time checking it as well.
I've definitely underestimated the work to do on cleanup because we
weren't aware of --strict, but I thought we had gotten rid of all of the
proc entries and moved anything important to sysfs/debugfs. The major
numbers issue was next.
-- Ben
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/9] staging: unisys: checkpatch strict cleanup in include
On Thu, 2014-09-25 at 00:02 +0200, Greg KH wrote: > On Wed, Sep 24, 2014 at 07:12:03PM +0300, Dan Carpenter wrote: > > Gar. I hate to make you redo a lot of work but these need to be broken > > up into "one thing per patch". That rules is kind of vague so we > > normally tell people to fix one type of checkpatch.pl warning at a time > > as a simple way of explaining it. > > > > But definitely "fixing every checkpatch.pl warning in a file" is not > > one thing unless there are like 2 lines of change only. > > I agree, please fix this up into "one logical change per patch". > > thanks, > > greg k-h That's no problem, the same amount of work needs to get done regardless of how it's split up. :) When you say type of warning, do you mean individual warnings, or can I group them into sets, i.e. fixing spacing and doubled blank lines together as a single whitespace formatting patch? When fixing the camel-case warnings do I need to do a separate patch for each function or variable or struct, or can I group all of the camel-case fixes for a single file together? -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/9] staging: unisys: checkpatch strict cleanup in include
On Thu, 2014-09-25 at 16:30 +0300, Dan Carpenter wrote: > Probably that's fine, but normally we suggest: > > patch 1: clean up long lines > patch 2: delete extra blank lines > patch 3: comments > > or whatever... > OK, thanks. I suppose if I'm not sure, I'll make individual patches and I can just squash them together if someone complains that they're too small. > What really helps me is if you list the exact variables that were > changed in the changelog like this: > > Cleanup CamelCase names: > FooBar => foo_bar > OneTwo => one_two > > I use a script to sed these renames out so I can just cut and paste the > renames and verify that nothing else changed. I'll do that! Thanks for the help! :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/9] staging: unisys: clean up periodic_work.c and periodic_work.h
On Wed, 2014-09-24 at 19:34 +0300, Dan Carpenter wrote: > On Wed, Sep 24, 2014 at 11:56:19AM -0400, Benjamin Romer wrote: > > +struct periodic_work * > > + visor_periodic_work_create(ulong jiffy_interval, > > + struct workqueue_struct *workqueue, > > + void (*workfunc)(void *), > > + void *workfuncarg, > > + const char *devnam); > > No. This isn't the right way to do it. The way the lines were broken > up originally was fine. It's ok to pull the parameter declarations back > to make it under the 80 character limit. Sorry, it was kind of an act of desperation to try and pass the strict check for parenthesis alignment. I originally wanted to do it like this: struct periodic_work *visor_periodic_work_create(ulong jiffy_interval, struct workqueue_struct *workqueue, void (*workfunc)(void*), void *workfuncarg, const char *devnam); But that generates the same parenthesis check message with --strict turned on. Trying to align everything with the parenthesis was very ugly, so I tried to save space by splitting the line at the return type. So should I just ignore the parenthesis warning for this one function? I'm kind of confused about this particular check to be honest. In Documentation/CodingStyle it says to never use spaces for indentation, but there's no way to pass this check without using spaces, or getting lucky and having things line up exactly on a tab. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 04/67] staging: unisys: remove stray blank line in timskmod.h
On Thu, 2014-10-02 at 09:41 -0700, Greg KH wrote:
> > -
> > /* @} */
>
> What is that odd comment?
I think it's part of a doxygen block that starts above the #defines:
/**
* @addtogroup driverlogging
* @{
*/
To make sure, I'll ask the original author when I get the chance.
-- Ben
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] staging: unisys: added virtpci info entry
On Thu, 2014-07-10 at 07:51 -0700, Greg KH wrote: > On Thu, Jul 10, 2014 at 10:34:14AM -0400, Erik Arfvidson wrote: > > This patch adds the virtpci debugfs directory and the info entry > > inside of it. > > > > Signed-off-by: Erik Arfvidson > > Signed-off-by: Benjamin Romer > > 2/2? Where is patch 1/2? > > Please resend this correctly, and properly version your patches (this is > what, version 10 or something?). > > Your subsystem maintainer has a short-term memory of a gnat, you need to > be explicit otherwise I'll ignore it... > > greg k-h I mistakenly clicked reply instead of reply all the first time. :( Anyhow, I'm really sorry Greg, this patch was the second of a two part set and you'd already taken the first one, so I thought we should only resend the corrected patch and I told Erik to do that, so it's my fault. Should we send you this as a [1/1 version 4] or resend the entire set despite one patch already being in? -- Ben Romer | Software Engineer | Virtual Systems Development Unisys Corporation | 2476 Swedesford Rd | Malvern, PA 19355 | 610-648-7140 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/10] staging: unisys: add toolaction to sysfs
On Tue, 2014-07-15 at 21:50 -0700, Greg KH wrote: > On Tue, Jul 15, 2014 at 01:30:42PM -0400, Benjamin Romer wrote: > All sysfs files need a Documentation/ABI/ entry. As this isn't in the > "real" part of the kernel yet, just create the entries in the unisys/ > subdir and then when it moves out, we can move them to the "right" place > in the tree. > > Can you redo this patch with that entry? > Of course. I'll add an ABI-documentation file to our subdir with all of the documentation as the first patch in my resubmission. > How can this not be true? I tried to follow the code here, but it's > horrid, why is this assigned in a work queue and not when the module > starts up? I'm not sure of the history or original purpose for the delayed initialization, and it also looks like there are two different ways defined in separate files for the channel to be assigned an address, so what's there now is definitely overcomplicated. In addition to the initialization fix, I'll move the channel address function into this file and remove the other files to simplify things further. > > + U8 toolAction; > > + > > + visorchannel_read(ControlVm_channel, > > + offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL, > > + ToolAction), &toolAction, sizeof(U8)); > > + return scnprintf(buf, PAGE_SIZE, "%u\n", toolAction); > > + } else > > + return -ENODEV; > > +} > > Why would this sysfs file be created if there is not a device? That's > not how sysfs files work, it should only be present if the file has a > value, and if not, it shouldn't be there. I'll clean this up so that it works that way, and remove the extraneous checks for the channel address. > Same goes for your other sysfs files in this patchset, you should never > be return -ENODEV, you just shouldn't have created the file in the first > place. That makes your kernel code simpler, and hopefully, your > userspace code easier too. It will. :) I'll do a cleanup patch set first and then redo the proc changeover set after that. > thanks, > > greg k-h -- Ben Romer | Software Engineer | Virtual Systems Development Unisys Corporation | 2476 Swedesford Rd | Malvern, PA 19355 | 610-648-7140 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/8] staging: unisys: add toolaction to sysfs
On Sat, 2014-07-19 at 09:36 -0700, Greg KH wrote:
> > /* /sys/devices/platform/visorchipset */
> > static struct platform_device Visorchipset_platform_device = {
> > .name = "visorchipset",
> > .id = -1,
> > + .dev.groups = visorchipset_dev_groups,
>
> Only create this device when ControlVm_channel is present in the system,
> that should take out your check for it in the show/store function.
>
> Same goes for the rest of these patches.
>
> thanks,
>
> greg k-h
I'm not sure I understand. Do you mean that we should only create sysfs
devices dynamically, and not use a static here?
I assume that any other sysfs devices in our driver set created this way
would have the same issue?
--
Ben Romer | Software Engineer |
Virtual Systems Development
Unisys Corporation | 2476
Swedesford Rd | Malvern, PA 19355
| 610-648-7140
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 8/8 v2] staging: unisys: ABI documentation for new sysfs entries
On Tue, 2014-07-22 at 16:14 -0700, Greg KH wrote: > What is the format of the chipsetready file? It looks like you want to > write 2 values to it? Please describe it better, for all of these. > I think the intent was to pass in a tuple from a script started by a udev event, as an indication of completion. I'm getting in touch with the original author of that function to see how we can fix this to do things correctly in sysfs. I'll send another set of patches once I know what I need to change in user space to match whatever we end up deciding on doing in the kernel. > thanks, > > greg k-h -- Ben Romer | Software Engineer | Virtual Systems Development Unisys Corporation | 2476 Swedesford Rd | Malvern, PA 19355 | 610-648-7140 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Subject: [PATCH 6/6 v3] staging: unisys: ABI documentation for new sysfs entries
Please disregard this specific email - "Subject: [PATCH 6/6 v3] staging: unisys: ABI documentation for new sysfs entries" - the subject was corrected and this individual patch was resent. Sorry for any confusion. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/6 v3] staging: unisys: move installer to sysfs and split fields
On Fri, 2014-07-25 at 14:05 +0300, Dan Carpenter wrote: > visorchannel_write() returns either 0 or -EFAULT so this condition is > never true. This bug occurs in several places. Thank you for catching my mistake. I'll fix this and change visorchannel_write() to return -EIO as you suggested. > Look, Ma, no indenting! Also -EIO is wrong-ish. visorchannel_write() > should probably return -EIO instead of -EFAULT. Do it like this: Will do. I'll also check for additional places where we can simplify the code like that. Thank you for the help!! :) -- Ben Romer | Software Engineer | Virtual Systems Development Unisys Corporation | 2476 Swedesford Rd | Malvern, PA 19355 | 610-648-7140 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5] staging: unisys: move parahotplug to sysfs
On Tue, 2014-07-29 at 12:13 +0300, Dan Carpenter wrote: > This is the same bug I mentioned ealier in a different patch. Yes, I know that - my intent was to get this patch in to move the entry, then fix the error in the function, like how the other sysfs functions got fixed. The code in this patches' sysfs functions is a cut and paste of what was in parahotplug_proc_write() with small modifications so that it will build. I'll split this into a patch that deletes the proc entry, and a separate patch that adds the sysfs entries with corrected code, then. Hopefully I won't get any complaints about that being too many logical changes. -- Ben Romer | Software Engineer | Virtual Systems Development Unisys Corporation | 2476 Swedesford Rd | Malvern, PA 19355 | 610-648-7140 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5] staging: unisys: move parahotplug to sysfs
On Tue, 2014-07-29 at 16:58 +0300, Dan Carpenter wrote: > > This is broken code which clearly hasn't been tested. Wat??? Oh, I think I see what you mean. When I said tested, I meant that the entries appear in sysfs instead of proc, not that they pass data anywhere or work with s-Par correctly. I just tested that my patch does what it said it does, which is moving the entries. That particular line I believe generated an error in checkpatch.pl when I removed the second field and split the single entry into two. It says that the kstrto*() functions are preferred for single parameters over sscanf(). > I don't understand how that is ok? It's not, which is why they needed to be fixed too. Anyway, I split the patch into two separate patches, one to remove the proc code, and another to add completely new sysfs functions with corrected code from the start. -- Ben Romer | Software Engineer | Virtual Systems Development Unisys Corporation | 2476 Swedesford Rd | Malvern, PA 19355 | 610-648-7140 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 11/14] staging: unisys: remove do{} while(0) in macros in channel.h
On Wed, 2014-08-06 at 11:18 +0300, Dan Carpenter wrote: > On Tue, Aug 05, 2014 at 02:57:55PM -0400, Benjamin Romer wrote: > > The CHANNEL_*_MISMATCH error message macros should not be inside of do > > blocks. > > > > Why not? We do that so they can be called like a function. These seem > to not be called at all. Just delete them (in a later patch). I ran checkpatch.pl against the channel.h file and it complains about the do blocks in the macros there. It also complains about do blocks in iochannel.h. It's only a warning though. I'm all in favor of deleting them - they get used in ULTRA_check_channel_client() in the same file, and only there. I'll just remove the macros and put the equivalent code in where they were used. -- Ben Romer | Software Engineer | Virtual Systems Development Unisys Corporation | 2476 Swedesford Rd | Malvern, PA 19355 | 610-648-7140 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
