Re: [PATCH 09/69] staging: unisys: refactor create_bus()
devendra.aaru wrote: > You might consider not using camel case letters.. The purpose of patch 8 is to fix spacing errors across the entire file. The camelcase names are fixed in later patches. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 09/19] staging: unisys: visorchipset: Move MYDRVNAME to visorchipset.h
On 04/30/2015 10:36 AM, Greg KH wrote: Because patch 08 didn't apply, this one has fuzz problems, and the reset in the series doesn't apply. Can you refresh against my tree and resend the rest? Will do, thanks for taking these. :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 040/141] staging: unisys: visorchannel cleanup visorchannel_create_guts()
On 05/06/2015 08:15 AM, Dan Carpenter wrote: Also there were some unrelated changes. Benjamin, you should have complained about those when the patch was first sent. That's like the most obvious thing to check when you're reviewing patches. I'm sorry, I don't understand what part of this is unrelated? To me it looked like Prarit was refactoring the function and its subroutines, mostly the error handling but also cleanup. I've sent similar patches in the past that were refactoring patches, and the patch does what it says in the description, so I thought it was okay. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 000/141] staging: unisys: s-Par driver rebuild series
On 05/08/2015 04:52 AM, Dan Carpenter wrote: I'm finished going through these patches. Pretty decent over all. My only comment was that there were three? places where we introduced a bug and then fixed it in a later patch. I kind of wish the fix were folded into the original patch. I don't know how awkward that is. If it's too difficult then don't worry about it. It seems that Greg is merging most (if not all) of the patchset into staging-testing now, so it'd be more trouble to generate a v2 than it's worth, but we are going to make sure we take care of all of the flaws you've pointed out. :) I also missed all three of those bugs so my review obviously wasn't perfect. :P The feedback was very valuable to us! :) Thanks for taking the time to go over everything. I'm hoping that we'll have patches sent in by next week, for each of the mistakes of ours that you found. :) Thanks again! :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: unisys: use schedule_timeout_interruptible()
As the actually intended timeout is not documented and msecs_to_jiffies timeouts can be a factor 10 different from the current effective timeout this needs to be checked by someone who knows the details of this driver in any case it should be passed in a HZ independent manner. I need an ack from the maintainers before I can take this. thanks, greg k-h This looks okay to me and applies cleanly to the end of our v4 series, so could you please apply it after the end of the v4 patch series I just sent? :) Thanks!! -- Ben Signed-off-by: Benjamin Romer ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: unisys: visorbus: add static declarations
On 05/28/2015 10:04 PM, Drew Fustini wrote: Add static declarations to statisfy sparse warnings in: drivers/staging/unisys/visorbus/visorbus_main.c Hi, I'd really like to take this patch, but it doesn't apply at the end of the current set of patches I'm working with, and if I put it ahead of them it'll make them not apply. Would you mind if I sent a second version of this patch with it rebased against my last set of patches, so it will apply? Thanks for the help! -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: unisys: visorbus: add static declarations
On 06/01/2015 06:17 PM, Drew Fustini wrote: On Mon, Jun 01, 2015 at 02:34:16PM -0400, Ben Romer wrote: Would you mind if I sent a second version of this patch with it rebased against my last set of patches, so it will apply? Please go ahead. thanks, drew Sorry, never mind, it looks like Greg applied this before my series anyway. It's already in. :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/14] staging: unisys: add channel interrupt support
On 11/17/2015 05:18 PM, Greg KH wrote: On Tue, Nov 17, 2015 at 09:57:58AM -0500, Benjamin Romer wrote: This patch series adds a centralized infrastructure and device support for channel interrupts sent to s-Par virtual devices. With these changes, the visorhba device is ~80% faster than with only polling, and visornic receives a speedup of over 3500% (from ~9Mb/s to between 360Mb/s and 390Mb/s). Why are you adding new functions to the code before it is merged out of staging? Please don't do that, please get the code out of staging first. thanks, greg k-h Hi Greg, Interrupt support was originally part of the driver set when we submitted it, but we removed it when we renamed the drivers and restructured the set. The work was listed in the TODO so I thought it would be okay to patch it in staging, but if we should wait until the drivers are out, then we will wait. :) Do you think our drivers are clean enough to be moved out soon? None of the files generate checkpatch errors anymore (the one in visorhba_main was discussed before and decided to be okay). There are 7 files that generate warnings still, controlvmchannel.h being the biggest offender, with the rest all in single digits. I'll get those warnings cleaned up - are there other things you'd like us to fix? -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: unisys: use common return path
On 12/01/2015 03:00 AM, Dan Carpenter wrote:
Doing One Err style error handling is often a mistake but it's ok here.
Why is it okay here? I don't understand why this function would be any
different than the other places where the code used a goto.
If we *have* to change it I would prefer that we not add a goto and
instead add an additional boolean local variable to control serverdown
completion. That's less complex and makes the intent clear.
like this:
visornic_serverdown(struct visornic_devdata *devdata,
visorbus_state_complete_func complete_func)
{
unsigned long flags;
int retval = 0;
bool complete_serverdown = false;
spin_lock_irqsave(&devdata->priv_lock, flags);
if (!devdata->server_down && !devdata->server_change_state) {
if (devdata->going_away) {
dev_dbg(&devdata->dev->device,
"%s aborting because device removal pending\n",
__func__);
retval = -ENODEV;
} else {
devdata->server_change_state = true;
devdata->server_down_complete_func = complete_func;
complete_serverdown = true;
}
} else if (devdata->server_change_state) {
dev_dbg(&devdata->dev->device, "%s changing state\n",
__func__);
spin_unlock_irqrestore(&devdata->priv_lock, flags);
retval = -EINVAL;
}
spin_unlock_irqrestore(&devdata->priv_lock, flags);
if (complete_serverdown)
visornic_serverdown_complete(devdata);
return retval;
}
-- Ben
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: unisys: use common return path
On 12/01/2015 10:57 AM, Dan Carpenter wrote: What I meant was that I'm generally opposed to "common exit paths". Mixing all the exit paths together often makes the code more complicated and leads to errors. That makes sense from a common sense perspective that doing many things is more difficult than doing one thing? Anyway it's easy enough to verify empirically that this style is bug prone. On the other hand there are times where all exit paths need to unlock or to free a variable and in those cases using a common exit path makes sense. Just don't standardize on "Every function should only have a single return". That works for me. Mainly my issue with it is that I've spent a lot of time trying to eliminate "goto Away" code from the drivers, so I'd rather not put any back if possible. If we *have* to change it I don't think we have to change it at all. Using direct returns makes finding locking bugs easier for static checkers. That's true, and I think the code is fine as it is. spin_unlock_irqrestore(&devdata->priv_lock, flags); This is a bug. Indeed, but I'd rather not have any of these changes made anyway. This function isn't broken so it doesn't need to be fixed. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch] staging: unisys: remove some dead code
On 01/07/2016 04:34 AM, Dan Carpenter wrote: queue_delayed_work() returns bool, not negative error codes. It returns false if the work has already been queued or true otherwise. Since we don't care about that, we can just remove the test. Signed-off-by: Dan Carpenter Tested with s-Par, works fine (of course). :) Signed-off-by: Benjamin Romer ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: unisys: visornic: remove useless memset
On Mon, 2016-01-25 at 20:22 +, Hugo Camboulive wrote: > alloc_etherdev() calls alloc_netdev_mqs(), which > already uses kzalloc/vzalloc. > > This clears a sparse warning : > drivers/staging/unisys/visornic/visornic_main.c:1366:15: warning: > memset with byte count of 1460112 > > Signed-off-by: Hugo Camboulive Tested, works fine. :) Signed-off-by: Benjamin Romer ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/16] staging: unisys: cleanup series
On Sun, 2016-02-07 at 14:03 -0800, Greg KH wrote: > On Fri, Jan 29, 2016 at 10:21:26AM -0500, Benjamin Romer wrote: > > This patch series cleans up all the remaining issues reported by > > checkpatch.pl that can be fixed. The series was rebased against > > the current contents of staging-next. > > As this was a 'v2' series of patches, you should have said so, > describing what was changed from the original set, so that people > know > if you did or did not respond to the last set of review comments :( > > Please fix that up and resend this. > > thanks, > > greg k-h Sorry about that, I wasn't sure about what was going on with the series, so I just rebased it and re-sent it without any other changes. Some of the original set was partially accepted today, so I will fix the remaining patches that no longer applied, and resend those as v3. :) Thanks! :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 13/14] staging: unisys: fix else statement in visornic_main.c
On Mon, 2016-02-08 at 22:39 +0530, Sudip Mukherjee wrote: > maybe this is better where you have single exit point and so only one > spin_unlock_irqrestore(). We discussed this before. I don't want to put any of the goto messes back in because I don't think it shortens the code or makes it any simpler. [snip] > + > +exit_unlock: > + spin_unlock_irqrestore(&devdata->priv_lock, flags); > return 0; This should be returning ret. It's less likely that someone will accidentally overwrite or throw away the status later, if the code just returns directly at the point of error. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Time for a code audit?
Greg, Thank you very much for taking our patches. All of us here appreciate it a lot. :) It looks to me like our driver set is clean as far as checkpatch.pl is concerned - there's one error, which was said to be acceptable as it was, and a small number of checks that can't be made any prettier. I was hoping we could get started with the code audit and work on getting the drivers out of the staging tree. What's the best way to do that? Again, thanks for taking the time to put our cleanups in. :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Time for a code audit?
On Sat, 2016-02-13 at 01:01 +0300, Dan Carpenter wrote: > I looked at the Smatch warnings, plus some bonus stuff I'm still > working > on. > > drivers/staging/unisys/include/iochannel.h:592 add_physinfo_entries() > warn: inconsistent indenting > drivers/staging/unisys/include/iochannel.h:596 add_physinfo_entries() > warn: inconsistent indenting > drivers/staging/unisys/include/iochannel.h:600 add_physinfo_entries() > warn: XXX should 'inp_pfn + i' be a 64 bit type? > Hi Dan, Thank you for this list! We'll get started on fixing these issues immediately. I've run Smatch using the make options O=../builds/testbuild CC=gcc-4.9 CHECK="smatch -p=kernel" C=1 but when I use it this way, I don't get that line where it complains about the variable's size. What settings do you use? If I can generate a list of problems that way across the whole driver set, we can just resolve them all without having to bother you or Greg until we have patches for them. Thanks a ton for the review. :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/7] staging: unisys: cleanup goto in create_visor_device()
On Tue, 2016-02-23 at 19:16 +0300, Dan Carpenter wrote: > When you're reviewing, you only have to care about the most recent > allocation. After this point we need to call put_device(). After > this > point we need to call unregister. The "goto unregister" is pretty > obvious what it does so it means you don't have to scroll down. Then > when you read to the bottom, it's pretty obvious that the labels do > what the name implies. That makes sense. It seemed to me to be unnecessary complication to use a goto, since there was only the one place where unregister was needed. I'll put it back in and resend this one. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 3/7] staging: unisys: cleanup goto in create_visor_device()
disregard this patch, I sent the wrong file. v3 coming. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: FW: [PATCH] staging: unisys: Update MAINTAINERS file
On Mon, 2016-02-29 at 17:02 +, Kershner, David A wrote: > > On Mon, Feb 29, 2016 at 10:24:06AM -0500, David Kershner wrote: > > > Benjamin Romer is no longer a mainta.iner for the s-Par drivers. David > > > Kershenr will now be the maintainer > > > Signed-off-by: David Kershner > > > --- > > > drivers/staging/unisys/MAINTAINERS | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > Can I get a signed-off-by from Ben to validate this? > > Signed-off-by: Benjamin Romer Hopefully my personal address is okay, I've submitted patches with it in th e past. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: unisys: Add s-Par visorhba
Hi Greg, I was wondering if you'd had a chance to take a look at this patch, and if you had any additional comments? It should have all of your previous comments addressed. :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/2] staging: unisys: visornic: Convert to using napi
Please disregard this version, it assumes that the kthread_park() patch was present. I will send a v3. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 1/5] staging: unisys: remove redundant variable
On 03/26/2015 08:01 AM, Greg Kroah-Hartman wrote: I need an ack from Benjamin and/or David before I can take these, as they are the maintainers of the driver, and have the ability to test these patches. I'll just wait to apply them until that happens. Ben/David? I'll test them today. :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 1/5] staging: unisys: remove redundant variable
On 03/26/2015 09:47 AM, Ben Romer wrote: On 03/26/2015 08:01 AM, Greg Kroah-Hartman wrote: I need an ack from Benjamin and/or David before I can take these, as they are the maintainers of the driver, and have the ability to test these patches. I'll just wait to apply them until that happens. Ben/David? I'll test them today. :) -- Ben I've tested them and everything appears to function correctly, so please accept this patch series. Thanks! -- Ben Signed-off-by: Benjamin Romer ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 03/13] staging: unisys: visorhid: rename to visorinput
On 10/02/2015 05:40 AM, Greg KH wrote: Please generate patches with the -M flag, so that it is easy to see that you are moving files around, not just removing and adding them somewhere else. Can you fix this up and resend the series starting with this patch? Will do, do I need to v2 them? Or just resend from there? -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 9/9] staging: unisys: vmcallinterface.h: convert pragma to __packed
On 10/12/2015 11:51 PM, Greg KH wrote:
On Mon, Oct 12, 2015 at 03:19:47PM -0400, Benjamin Romer wrote:
From: David Kershner
Convert from pragma to __packed
Signed-off-by: David Kershner
Signed-off-by: Benjamin Romer
---
drivers/staging/unisys/visorbus/vmcallinterface.h | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/staging/unisys/visorbus/vmcallinterface.h
b/drivers/staging/unisys/visorbus/vmcallinterface.h
index 7a18aa8..276b784 100644
--- a/drivers/staging/unisys/visorbus/vmcallinterface.h
+++ b/drivers/staging/unisys/visorbus/vmcallinterface.h
@@ -84,7 +84,6 @@ enum vmcall_monitor_interface_method_tuple { /* VMCALL
identification tuples */
/* / BEGIN PRAGMA PACK PUSH 1 / */
/* / ONLY STRUCT TYPE SHOULD BE BELOW */
-#pragma pack(push, 1)
You didn't delete the comment lines here, why not?
Sorry, we'll fix this and send a v2. :(
-- 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
Sudip Mukherjee wrote: > fixed sparse warning : context imbalance in 'destroy_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 this patch and it looks good also. Thanks! :) -- Ben Tested-by: Benjamin Romer Acked-by: Benjamin Romer ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/8] staging: unisys: visornic/visorbus fixes
On 09/02/2015 08:40 PM, Greg KH wrote: These aren't all "fixes" so I can't just add them to the 4.3-final release, but some look like they should go there. So please split this into two different series and resend, one for 4.3-final and one for 4.4-rc1. Greg, We've been running and testing with all of these patches for a week or two now, and my team and I all feel that the entire set is safe to include in the new release. I'm not sure what your criteria are for inclusion in 4.3, so if you could let me know what those are, I'd be happy to split them up and send two sets, of course with the per-patch comments addressed. :) Thanks! -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/8] staging: unisys: visornic/visorbus fixes
On 09/04/2015 09:41 AM, Sudip Mukherjee wrote: Any new developments can only go in rc1 release. 4.3 merge window is already open so new developments cannot be added to 4.3-rc1. New codes has to go to 4.4-rc1. 4.3-rc2 - 4.3-rc7 (or 8) can only have fixes. I don't think any of the patches in this set specifically were adding new features, though I suppose the patches to remove list_all, devnum_pool, and devnum could be seen as not being *bug* fixes. It depends on if eliminating redundant code counts or not. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] staging: unisys: Add s-Par visorhba
On 09/23/2015 01:00 PM, Greg KH wrote: v4: * remove all BUG() and BUG_ON() - do not crash the kernel You just deleted them? No taking into account that you should just handle issues like this in a run-time-safe way instead? Removing them altogether was a better way to handle the problems overall. In both spots, the BUG_ON() was catching things on the client side of the driver that were actually a problem for the I/O service partition - bad values going in which could cause the service partition to crash. Obviously, crashing the guest was a bad solution, and it left an opening where someone could crash the service partition on purpose, so we needed to be able to handle things the right way. I apologize for the lack of information in the versioning. I will collect up all of the changes from each version, and send a v5 with comprehensive information. :) -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/8] staging: unisys: visorchipset proc fixes
Greg KH wrote: > What happened to the idea of only creating the sysfs files _if_ it is > needed? You are always creating these files, and then can return > -ENODEV if the device really isn't there, that's not what you should do > for a sysfs file. If the file is present, it should return data, not > return an error. If the device isn't there, just don't create the file. Greg, I submitted a set of patches before this set that does just that. I moved the controlvm channel function into visorchipset_main.c and removed the old files, and made it so that if the channel is not present the module wouldn't load. I also removed all the code that returns ENODEV, except for the module init function, where it gets returned if there's no controlvm channel present. I could change that to some other error, or let the module load and then not create files if the channel isn't present, if you'd prefer that? But if the module doesn't load, the files in sys don't get created, so I thought that would be a good solution. The commit numbers were 524b0b6 for the controlvm channel function, and 8a1182e for the extraneous checks and ENODEV errors being removed. -- Ben ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/6 v3] staging: unisys: remove partition information from proc
Greg KH wrote: > > Really > Obviously not, I'm sorry. :( ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
