Re: [PATCH v2] staging: dgap: fix kernel oops on port open
On 02/27/2014 04:52 PM, Dan Carpenter wrote:
This isn't a real patch, and it deliberately doesn't compile, but it's
sort of what the patch should look like.
The first thing to do is to get rid of the stupid DGAP_UNLOCK() macro.
Disabling IRQs more than once doesn't help anything and it doesn't make
sense to have lock_flags and lock_flags2. It should just be one
flags.
I guess the function name should have something to do with wake_up but
I just made up a dummy name.
regards,
dan carpenter
diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 7cb1ad597ced..41de09af27d1 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -6278,7 +6278,31 @@ static void dgap_parity_scan(struct channel_t *ch,
unsigned char *cbuf, unsigned
}
+static void futz_with_un_flags(struct board_t *bd, struct channel_t *ch,
+ struct un_t *un, unsigned long *irq_flags)
+{
+ if (!(un-un_flags UN_LOW))
+ return;
+
+ un-un_flags = ~UN_LOW;
+
+ if (!(un-un_flags UN_ISOPEN))
+ return;
+
+ if ((un-un_tty-flags (1 TTY_DO_WRITE_WAKEUP))
+ un-un_tty-ldisc-ops-write_wakeup) {
+ spin_unlock(ch-ch_lock);
+ spin_unlock_irqrestore(bd-bd_lock, *irq_flags);
+ (un-un_tty-ldisc-ops-write_wakeup)(un-un_tty);
+
+ spin_lock_irqsave(bd-bd_lock, *irq_flags);
+ spin_lock(ch-ch_lock);
+ }
+ wake_up_interruptible(un-un_tty-write_wait);
+ wake_up_interruptible(un-un_flags_wait);
+ DPR_EVENT((event: Got low event. jiffies: %lu\n, jiffies));
+}
/*===
*
@@ -6442,44 +6466,8 @@ static int dgap_event(struct board_t *bd)
DPR_EVENT((event: got low event\n));
- if (ch-ch_tun.un_flags UN_LOW) {
- ch-ch_tun.un_flags = ~UN_LOW;
-
- if (ch-ch_tun.un_flags UN_ISOPEN) {
- if ((ch-ch_tun.un_tty-flags
- (1 TTY_DO_WRITE_WAKEUP))
-
ch-ch_tun.un_tty-ldisc-ops-write_wakeup)
- {
- DGAP_UNLOCK(ch-ch_lock,
lock_flags2);
- DGAP_UNLOCK(bd-bd_lock,
lock_flags);
-
(ch-ch_tun.un_tty-ldisc-ops-write_wakeup)(ch-ch_tun.un_tty);
- DGAP_LOCK(bd-bd_lock,
lock_flags);
- DGAP_LOCK(ch-ch_lock,
lock_flags2);
- }
-
wake_up_interruptible(ch-ch_tun.un_tty-write_wait);
-
wake_up_interruptible(ch-ch_tun.un_flags_wait);
-
- DPR_EVENT((event: Got low event. jiffies:
%lu\n, jiffies));
- }
- }
-
- if (ch-ch_pun.un_flags UN_LOW) {
- ch-ch_pun.un_flags = ~UN_LOW;
- if (ch-ch_pun.un_flags UN_ISOPEN) {
- if ((ch-ch_pun.un_tty-flags
- (1 TTY_DO_WRITE_WAKEUP))
-
ch-ch_pun.un_tty-ldisc-ops-write_wakeup)
- {
- DGAP_UNLOCK(ch-ch_lock,
lock_flags2);
- DGAP_UNLOCK(bd-bd_lock,
lock_flags);
-
(ch-ch_pun.un_tty-ldisc-ops-write_wakeup)(ch-ch_pun.un_tty);
- DGAP_LOCK(bd-bd_lock,
lock_flags);
- DGAP_LOCK(ch-ch_lock,
lock_flags2);
- }
-
wake_up_interruptible(ch-ch_pun.un_tty-write_wait);
-
wake_up_interruptible(ch-ch_pun.un_flags_wait);
- }
- }
+ futz_with_un_flags(bd, ch, ch-ch_tun, flags);
+ futz_with_un_flags(bd, ch, ch-ch_pun, flags);
if (ch-ch_flags CH_WLOW) {
ch-ch_flags = ~CH_WLOW;
Thanks Dan. I see what should be done. I like and can work on this. But
is it OK to save all the 80 char problems until the end of this next
series or more likely a separate patch all together? Since I'm trying to
make individual patches that address specific checkpatch problems I am
running into a chicken and egg sort of thing. Some of the next series
will have
Re: [PATCH v2] staging: dgap: fix kernel oops on port open
On Fri, Feb 28, 2014 at 08:56:37AM -0500, Mark Hounschell wrote: Thanks Dan. I see what should be done. I like and can work on this. But is it OK to save all the 80 char problems until the end of this next series or more likely a separate patch all together? What ever you want to do is ok. :P Greg applies them on a first come first serve basis. In theory, you should sent bug fixes first. Then my hint would be to send the patches in the least controversial to most controversial. That way if you have to redo one it will be at the end and we can apply the earlier ones. Otherwise send them in whatever order you want. Since I'm trying to make individual patches that address specific checkpatch problems I am running into a chicken and egg sort of thing. Some of the next series will have checkpatch warnings/errors that are corrected in later patches. It's fine for your patch to have checkpatch warnings if they were there in the original code. It's also fine if you change indentation and it makes you go over the 80 character mark. Common sense applies. Will this be OK? I think the review process will be much easier this way? Fine fine. Just send us what you have so far. It's better to not get too big of pile of patches until you get a feel for which things we are going to complain about. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: dgap: fix kernel oops on port open
On 02/26/2014 10:30 AM, Dan Carpenter wrote:
On Wed, Feb 26, 2014 at 10:18:26AM -0500, Mark Hounschell wrote:
This patch addresses the follow error message followed
by a kernel oops:
dgap: driver does not set tty-port. This will crash the kernel later. Fix
the driver
It also renames the main function this patch addresses because
its name is misleading.
Thanks.
regards,
dan carpenter
I'm in the process of running dgap.c through checkpatch and creating another
patch set. Before I get too far into it I wanted to get clarification on a
couple of things.
1. Should I wait until I know the status of my last patch before I post new
ones?
2. There are MANY over 80 char lines warnings. I'm uncertain of the
acceptable way to fix some of them. Here is an example of one:
while (tail != head) {
if (reason IFTLW) {
if (ch-ch_tun.un_flags UN_LOW) {
ch-ch_tun.un_flags = ~UN_LOW;
// everything in this block is over 80 chars
if (ch-ch_tun.un_flags UN_ISOPEN) {
if ((ch-ch_tun.un_tty-flags
(1 TTY_DO_WRITE_WAKEUP))
ch-ch_tun.un_tty-ldisc-ops-write_wakeup) { //
DGAP_UNLOCK(ch-ch_lock,
lock_flags2);
DGAP_UNLOCK(bd-bd_lock,
lock_flags);
(ch-ch_tun.un_tty-ldisc-ops-write_wakeup)(ch-ch_tun.un_tty); //
DGAP_LOCK(bd-bd_lock,
lock_flags);
DGAP_LOCK(ch-ch_lock,
lock_flags2);
}
wake_up_interruptible(ch-ch_tun.un_tty-write_wait);
wake_up_interruptible(ch-ch_tun.un_flags_wait);
// end of nasty block
}
}
I figured I'd leave them for the last patch but there are a few that if I wait
will show up in
one or more of the patches preceding that last one. This one is actually one of
them. While
fixing up bracket errors with chechpatch -file, chechpatch doesn't like the
patch created.
Thanks
Mark
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: dgap: fix kernel oops on port open
On Thu, Feb 27, 2014 at 03:39:08PM -0500, Mark Hounschell wrote:
On 02/26/2014 10:30 AM, Dan Carpenter wrote:
On Wed, Feb 26, 2014 at 10:18:26AM -0500, Mark Hounschell wrote:
This patch addresses the follow error message followed
by a kernel oops:
dgap: driver does not set tty-port. This will crash the kernel later. Fix
the driver
It also renames the main function this patch addresses because
its name is misleading.
Thanks.
regards,
dan carpenter
I'm in the process of running dgap.c through checkpatch and creating another
patch set. Before I get too far into it I wanted to get clarification on a
couple of things.
Please line-wrap your emails :(
1. Should I wait until I know the status of my last patch before I post new
ones?
I just now applied it :)
2. There are MANY over 80 char lines warnings. I'm uncertain of the
acceptable way to fix some of them. Here is an example of one:
while (tail != head) {
if (reason IFTLW) {
if (ch-ch_tun.un_flags UN_LOW) {
ch-ch_tun.un_flags = ~UN_LOW;
// everything in this block is over 80 chars
if (ch-ch_tun.un_flags UN_ISOPEN) {
if ((ch-ch_tun.un_tty-flags
(1 TTY_DO_WRITE_WAKEUP))
ch-ch_tun.un_tty-ldisc-ops-write_wakeup) { //
DGAP_UNLOCK(ch-ch_lock,
lock_flags2);
DGAP_UNLOCK(bd-bd_lock,
lock_flags);
(ch-ch_tun.un_tty-ldisc-ops-write_wakeup)(ch-ch_tun.un_tty); //
DGAP_LOCK(bd-bd_lock,
lock_flags);
DGAP_LOCK(ch-ch_lock,
lock_flags2);
}
wake_up_interruptible(ch-ch_tun.un_tty-write_wait);
wake_up_interruptible(ch-ch_tun.un_flags_wait);
// end of nasty block
}
}
I figured I'd leave them for the last patch but there are a few that if I
wait will show up in
one or more of the patches preceding that last one. This one is actually one
of them. While
fixing up bracket errors with chechpatch -file, chechpatch doesn't like the
patch created.
For major logic chunks like this, I'd recommend just leaving them over
80 columns for now, until they can be refactored into something more
readable later. Don't try to just trail characters from the 70-80
character columns to make the tool happy, that's not a good idea.
Also, usually some of the if statement logic can be reversed to get the
code to be moved to the right easier, but maybe not.
good luck!
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: dgap: fix kernel oops on port open
On Thu, Feb 27, 2014 at 03:39:08PM -0500, Mark Hounschell wrote:
On 02/26/2014 10:30 AM, Dan Carpenter wrote:
On Wed, Feb 26, 2014 at 10:18:26AM -0500, Mark Hounschell wrote:
This patch addresses the follow error message followed
by a kernel oops:
dgap: driver does not set tty-port. This will crash the kernel later. Fix
the driver
It also renames the main function this patch addresses because
its name is misleading.
Thanks.
regards,
dan carpenter
I'm in the process of running dgap.c through checkpatch and creating
another patch set. Before I get too far into it I wanted to get
clarification on a couple of things.
1. Should I wait until I know the status of my last patch before I
post new ones?
Just assume they will be applied. If not you will have to redo.
2. There are MANY over 80 char lines warnings. I'm uncertain of the
acceptable way to fix some of them. Here is an example of one:
while (tail != head) {
if (reason IFTLW) {
if (ch-ch_tun.un_flags UN_LOW) {
ch-ch_tun.un_flags = ~UN_LOW;
// everything in this block is over 80 chars
if (ch-ch_tun.un_flags UN_ISOPEN) {
if ((ch-ch_tun.un_tty-flags
(1 TTY_DO_WRITE_WAKEUP))
ch-ch_tun.un_tty-ldisc-ops-write_wakeup) { //
DGAP_UNLOCK(ch-ch_lock,
lock_flags2);
DGAP_UNLOCK(bd-bd_lock,
lock_flags);
(ch-ch_tun.un_tty-ldisc-ops-write_wakeup)(ch-ch_tun.un_tty); //
DGAP_LOCK(bd-bd_lock,
lock_flags);
DGAP_LOCK(ch-ch_lock,
lock_flags2);
}
wake_up_interruptible(ch-ch_tun.un_tty-write_wait);
wake_up_interruptible(ch-ch_tun.un_flags_wait);
// end of nasty block
}
}
I figured I'd leave them for the last patch but there are a few that if I
wait will show up in
one or more of the patches preceding that last one. This one is actually one
of them. While
fixing up bracket errors with chechpatch -file, chechpatch doesn't like the
patch created.
Break it up into separate functions.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: dgap: fix kernel oops on port open
On Wed, Feb 26, 2014 at 10:18:26AM -0500, Mark Hounschell wrote: This patch addresses the follow error message followed by a kernel oops: dgap: driver does not set tty-port. This will crash the kernel later. Fix the driver It also renames the main function this patch addresses because its name is misleading. Thanks. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
