[PATCH] staging: dgap: fix kernel oops on port open

[PATCH] staging: dgap: fix kernel oops on port open

[Mark Hounschell]

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.

Signed-off-by: Mark Hounschell <markh@compro.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/staging/dgap/dgap.c | 47 ++++++++++++++++++++++++++++++++++++---------
 drivers/staging/dgap/dgap.h |  3 ++-
 2 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 7cb1ad5..192f63a 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -223,7 +223,7 @@ static void dgap_get_vpd(struct board_t *brd);
 static void dgap_do_reset_board(struct board_t *brd);
 static void dgap_do_wait_for_bios(struct board_t *brd);
 static void dgap_do_wait_for_fep(struct board_t *brd);
-static void dgap_sysfs_create(struct board_t *brd);
+static int dgap_tty_register_ports(struct board_t *brd);
 static int dgap_firmware_load(struct pci_dev *pdev, int card_type);
 
 /* Driver load/unload functions */
@@ -1098,7 +1098,9 @@ static int dgap_firmware_load(struct pci_dev *pdev, int card_type)
 		return ret;
 	}
 
-	dgap_sysfs_create(brd);
+	ret = dgap_tty_register_ports(brd);
+	if (ret)
+		return ret;
 
 	brd->state = BOARD_READY;
 	brd->dpastatus = BD_RUNNING;
@@ -1730,6 +1732,7 @@ static void dgap_tty_uninit(struct board_t *brd)
 		dgap_BoardsByMajor[brd->SerialDriver->major] = NULL;
 		brd->dgap_Serial_Major = 0;
 		for (i = 0; i < brd->nasync; i++) {
+			tty_port_destroy(&brd->SerialPorts[i]);
 			dgap_remove_tty_sysfs(brd->channels[i]->ch_tun.un_sysfs);
 			tty_unregister_device(brd->SerialDriver, i);
 		}
@@ -1737,6 +1740,7 @@ static void dgap_tty_uninit(struct board_t *brd)
 		kfree(brd->SerialDriver->ttys);
 		brd->SerialDriver->ttys = NULL;
 		put_tty_driver(brd->SerialDriver);
+		kfree(brd->SerialPorts);
 		brd->dgap_Major_Serial_Registered = FALSE;
 	}
 
@@ -1744,6 +1748,7 @@ static void dgap_tty_uninit(struct board_t *brd)
 		dgap_BoardsByMajor[brd->PrintDriver->major] = NULL;
 		brd->dgap_TransparentPrint_Major = 0;
 		for (i = 0; i < brd->nasync; i++) {
+			tty_port_destroy(&brd->PrinterPorts[i]);
 			dgap_remove_tty_sysfs(brd->channels[i]->ch_pun.un_sysfs);
 			tty_unregister_device(brd->PrintDriver, i);
 		}
@@ -1751,6 +1756,7 @@ static void dgap_tty_uninit(struct board_t *brd)
 		kfree(brd->PrintDriver->ttys);
 		brd->PrintDriver->ttys = NULL;
 		put_tty_driver(brd->PrintDriver);
+		kfree(brd->PrinterPorts);
 		brd->dgap_Major_TransparentPrint_Registered = FALSE;
 	}
 }
@@ -4813,25 +4819,48 @@ static int dgap_after_config_loaded(int board)
 /*
  * Create pr and tty device entries
  */
-static void dgap_sysfs_create(struct board_t *brd)
+static int dgap_tty_register_ports(struct board_t *brd)
 {
 	struct channel_t *ch;
-	int j = 0;
+	int i;
+
+	brd->SerialPorts = kcalloc(brd->nasync, sizeof(*brd->SerialPorts),
+					GFP_KERNEL);
+	if (brd->SerialPorts == NULL) {
+		pr_err("dgap: Cannot allocate serial port memory\n");
+		return -ENOMEM;
+	}
+	for (i = 0; i < brd->nasync; i++)
+		tty_port_init(&brd->SerialPorts[i]);
+
+	brd->PrinterPorts = kcalloc(brd->nasync, sizeof(*brd->PrinterPorts),
+					GFP_KERNEL);
+	if (brd->PrinterPorts == NULL) {
+		pr_err("dgap: Cannot allocate printer port memory\n");
+		return -ENOMEM;
+	}
+	for (i = 0; i < brd->nasync; i++)
+		tty_port_init(&brd->PrinterPorts[i]);
 
 	ch = brd->channels[0];
-	for (j = 0; j < brd->nasync; j++, ch = brd->channels[j]) {
+	for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
 		struct device *classp;
-		classp = tty_register_device(brd->SerialDriver, j,
-						&(ch->ch_bd->pdev->dev));
+
+		classp = tty_port_register_device(&brd->SerialPorts[i],
+				brd->SerialDriver, brd->firstminor + i, NULL);
+
 		ch->ch_tun.un_sysfs = classp;
 		dgap_create_tty_sysfs(&ch->ch_tun, classp);
 
-		classp = tty_register_device(brd->PrintDriver, j,
-						&(ch->ch_bd->pdev->dev));
+		classp = tty_port_register_device(&brd->PrinterPorts[i],
+				brd->PrintDriver, brd->firstminor + i, NULL);
+
 		ch->ch_pun.un_sysfs = classp;
 		dgap_create_tty_sysfs(&ch->ch_pun, classp);
 	}
 	dgap_create_ports_sysfiles(brd);
+
+	return 0;
 }
 
 
diff --git a/drivers/staging/dgap/dgap.h b/drivers/staging/dgap/dgap.h
index 573aa18..24db468 100644
--- a/drivers/staging/dgap/dgap.h
+++ b/drivers/staging/dgap/dgap.h
@@ -703,7 +703,6 @@ struct macounter
 #define	BD_FEP5PLUS	0x0001          /* Supports FEP5 Plus commands */
 #define BD_HAS_VPD	0x0002		/* Board has VPD info available */
 
-
 /*
  *	Per-board information
  */
@@ -761,8 +760,10 @@ struct board_t
 	struct channel_t *channels[MAXPORTS]; /* array of pointers to our channels. */
 
 	struct tty_driver	*SerialDriver;
+	struct tty_port *SerialPorts;
 	char		SerialName[200];
 	struct tty_driver	*PrintDriver;
+	struct tty_port *PrinterPorts;
 	char		PrintName[200];
 
 	u32		dgap_Major_Serial_Registered;
-- 
1.8.1.4

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

Re: [PATCH] staging: dgap: fix kernel oops on port open

[Dan Carpenter]

On Wed, Feb 26, 2014 at 08:41:38AM -0500, Mark Hounschell wrote:
> -static void dgap_sysfs_create(struct board_t *brd)
> +static int dgap_tty_register_ports(struct board_t *brd)
>  {
>  	struct channel_t *ch;
> -	int j = 0;
> +	int i;
> +
> +	brd->SerialPorts = kcalloc(brd->nasync, sizeof(*brd->SerialPorts),
> +					GFP_KERNEL);
> +	if (brd->SerialPorts == NULL) {
> +		pr_err("dgap: Cannot allocate serial port memory\n");

Don't add these error messages here.  kcalloc() already prints a much
better error message.

> +		return -ENOMEM;
> +	}
> +	for (i = 0; i < brd->nasync; i++)
> +		tty_port_init(&brd->SerialPorts[i]);
> +
> +	brd->PrinterPorts = kcalloc(brd->nasync, sizeof(*brd->PrinterPorts),
> +					GFP_KERNEL);
> +	if (brd->PrinterPorts == NULL) {
> +		pr_err("dgap: Cannot allocate printer port memory\n");

kfree(brd->SerialPorts);

> +		return -ENOMEM;
> +	}
> +	for (i = 0; i < brd->nasync; i++)
> +		tty_port_init(&brd->PrinterPorts[i]);
>  
>  	ch = brd->channels[0];
> -	for (j = 0; j < brd->nasync; j++, ch = brd->channels[j]) {
> +	for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
>  		struct device *classp;

Put a blank line after the declaration block.

> -		classp = tty_register_device(brd->SerialDriver, j,
> -						&(ch->ch_bd->pdev->dev));
> +
> +		classp = tty_port_register_device(&brd->SerialPorts[i],
> +				brd->SerialDriver, brd->firstminor + i, NULL);


The more traditional way to break up the long lines is:

		classp = tty_port_register_device(&brd->SerialPorts[i],
						  brd->SerialDriver,
						  brd->firstminor + i, NULL);
		dgap_create_tty_sysfs(&ch->ch_tun, classp);
		ch->ch_tun.un_sysfs = classp;

		classp = tty_port_register_device(&brd->PrinterPorts[i],
						  brd->PrintDriver,
						  brd->firstminor + i,
						  NULL);
		dgap_create_tty_sysfs(&ch->ch_pun, classp);
		ch->ch_pun.un_sysfs = classp;

(also i re-ordered the last two lines).

(also I wouldn't have sent this email at all if there hadn't been that
missing kfree()).

regards,
dan carpenter

> +
>  		ch->ch_tun.un_sysfs = classp;
>  		dgap_create_tty_sysfs(&ch->ch_tun, classp);
>  
> -		classp = tty_register_device(brd->PrintDriver, j,
> -						&(ch->ch_bd->pdev->dev));
> +		classp = tty_port_register_device(&brd->PrinterPorts[i],
> +				brd->PrintDriver, brd->firstminor + i, NULL);
> +
>  		ch->ch_pun.un_sysfs = classp;
>  		dgap_create_tty_sysfs(&ch->ch_pun, classp);
>  	}
>  	dgap_create_ports_sysfiles(brd);
> +
> +	return 0;

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

Re: [PATCH] staging: dgap: fix kernel oops on port open

[Mark Hounschell]

On 02/26/2014 09:01 AM, Dan Carpenter wrote:
> On Wed, Feb 26, 2014 at 08:41:38AM -0500, Mark Hounschell wrote:
>> -static void dgap_sysfs_create(struct board_t *brd)
>> +static int dgap_tty_register_ports(struct board_t *brd)
>>   {
>>   	struct channel_t *ch;
>> -	int j = 0;
>> +	int i;
>> +
>> +	brd->SerialPorts = kcalloc(brd->nasync, sizeof(*brd->SerialPorts),
>> +					GFP_KERNEL);
>> +	if (brd->SerialPorts == NULL) {
>> +		pr_err("dgap: Cannot allocate serial port memory\n");
>
> Don't add these error messages here.  kcalloc() already prints a much
> better error message.
>

OK

>> +		return -ENOMEM;
>> +	}
>> +	for (i = 0; i < brd->nasync; i++)
>> +		tty_port_init(&brd->SerialPorts[i]);
>> +
>> +	brd->PrinterPorts = kcalloc(brd->nasync, sizeof(*brd->PrinterPorts),
>> +					GFP_KERNEL);
>> +	if (brd->PrinterPorts == NULL) {
>> +		pr_err("dgap: Cannot allocate printer port memory\n");
>
> kfree(brd->SerialPorts);
>
>> +		return -ENOMEM;
>> +	}
>> +	for (i = 0; i < brd->nasync; i++)
>> +		tty_port_init(&brd->PrinterPorts[i]);
>>
>>   	ch = brd->channels[0];
>> -	for (j = 0; j < brd->nasync; j++, ch = brd->channels[j]) {
>> +	for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
>>   		struct device *classp;
>
> Put a blank line after the declaration block.
>

OK

>> -		classp = tty_register_device(brd->SerialDriver, j,
>> -						&(ch->ch_bd->pdev->dev));
>> +
>> +		classp = tty_port_register_device(&brd->SerialPorts[i],
>> +				brd->SerialDriver, brd->firstminor + i, NULL);
>
>
> The more traditional way to break up the long lines is:
>
> 		classp = tty_port_register_device(&brd->SerialPorts[i],
> 						  brd->SerialDriver,
> 						  brd->firstminor + i, NULL);
> 		dgap_create_tty_sysfs(&ch->ch_tun, classp);
> 		ch->ch_tun.un_sysfs = classp;
>
> 		classp = tty_port_register_device(&brd->PrinterPorts[i],
> 						  brd->PrintDriver,
> 						  brd->firstminor + i,
> 						  NULL);
> 		dgap_create_tty_sysfs(&ch->ch_pun, classp);
> 		ch->ch_pun.un_sysfs = classp;
>
> (also i re-ordered the last two lines).
>

OK again.

> (also I wouldn't have sent this email at all if there hadn't been that
> missing kfree()).
>
> regards,
> dan carpenter

Thanks. I'll fix and resend

Mark

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

[PATCH v2] staging: dgap: fix kernel oops on port open

[Mark Hounschell]

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.

Signed-off-by: Mark Hounschell <markh@compro.net>
---
 drivers/staging/dgap/dgap.c | 52 +++++++++++++++++++++++++++++++++++----------
 drivers/staging/dgap/dgap.h |  3 ++-
 2 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 7cb1ad5..f9adf8a 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -223,7 +223,7 @@ static void dgap_get_vpd(struct board_t *brd);
 static void dgap_do_reset_board(struct board_t *brd);
 static void dgap_do_wait_for_bios(struct board_t *brd);
 static void dgap_do_wait_for_fep(struct board_t *brd);
-static void dgap_sysfs_create(struct board_t *brd);
+static int dgap_tty_register_ports(struct board_t *brd);
 static int dgap_firmware_load(struct pci_dev *pdev, int card_type);
 
 /* Driver load/unload functions */
@@ -1098,7 +1098,9 @@ static int dgap_firmware_load(struct pci_dev *pdev, int card_type)
 		return ret;
 	}
 
-	dgap_sysfs_create(brd);
+	ret = dgap_tty_register_ports(brd);
+	if (ret)
+		return ret;
 
 	brd->state = BOARD_READY;
 	brd->dpastatus = BD_RUNNING;
@@ -1730,6 +1732,7 @@ static void dgap_tty_uninit(struct board_t *brd)
 		dgap_BoardsByMajor[brd->SerialDriver->major] = NULL;
 		brd->dgap_Serial_Major = 0;
 		for (i = 0; i < brd->nasync; i++) {
+			tty_port_destroy(&brd->SerialPorts[i]);
 			dgap_remove_tty_sysfs(brd->channels[i]->ch_tun.un_sysfs);
 			tty_unregister_device(brd->SerialDriver, i);
 		}
@@ -1737,6 +1740,7 @@ static void dgap_tty_uninit(struct board_t *brd)
 		kfree(brd->SerialDriver->ttys);
 		brd->SerialDriver->ttys = NULL;
 		put_tty_driver(brd->SerialDriver);
+		kfree(brd->SerialPorts);
 		brd->dgap_Major_Serial_Registered = FALSE;
 	}
 
@@ -1744,6 +1748,7 @@ static void dgap_tty_uninit(struct board_t *brd)
 		dgap_BoardsByMajor[brd->PrintDriver->major] = NULL;
 		brd->dgap_TransparentPrint_Major = 0;
 		for (i = 0; i < brd->nasync; i++) {
+			tty_port_destroy(&brd->PrinterPorts[i]);
 			dgap_remove_tty_sysfs(brd->channels[i]->ch_pun.un_sysfs);
 			tty_unregister_device(brd->PrintDriver, i);
 		}
@@ -1751,6 +1756,7 @@ static void dgap_tty_uninit(struct board_t *brd)
 		kfree(brd->PrintDriver->ttys);
 		brd->PrintDriver->ttys = NULL;
 		put_tty_driver(brd->PrintDriver);
+		kfree(brd->PrinterPorts);
 		brd->dgap_Major_TransparentPrint_Registered = FALSE;
 	}
 }
@@ -4813,25 +4819,49 @@ static int dgap_after_config_loaded(int board)
 /*
  * Create pr and tty device entries
  */
-static void dgap_sysfs_create(struct board_t *brd)
+static int dgap_tty_register_ports(struct board_t *brd)
 {
 	struct channel_t *ch;
-	int j = 0;
+	int i;
+
+	brd->SerialPorts = kcalloc(brd->nasync, sizeof(*brd->SerialPorts),
+					GFP_KERNEL);
+	if (brd->SerialPorts == NULL)
+		return -ENOMEM;
+	for (i = 0; i < brd->nasync; i++)
+		tty_port_init(&brd->SerialPorts[i]);
+
+	brd->PrinterPorts = kcalloc(brd->nasync, sizeof(*brd->PrinterPorts),
+					GFP_KERNEL);
+	if (brd->PrinterPorts == NULL) {
+		kfree(brd->SerialPorts);
+		return -ENOMEM;
+	}
+	for (i = 0; i < brd->nasync; i++)
+		tty_port_init(&brd->PrinterPorts[i]);
 
 	ch = brd->channels[0];
-	for (j = 0; j < brd->nasync; j++, ch = brd->channels[j]) {
+	for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
+
 		struct device *classp;
-		classp = tty_register_device(brd->SerialDriver, j,
-						&(ch->ch_bd->pdev->dev));
-		ch->ch_tun.un_sysfs = classp;
+
+		classp = tty_port_register_device(&brd->SerialPorts[i],
+					brd->SerialDriver,
+					brd->firstminor + i, NULL);
+
 		dgap_create_tty_sysfs(&ch->ch_tun, classp);
+		ch->ch_tun.un_sysfs = classp;
+
+		classp = tty_port_register_device(&brd->PrinterPorts[i],
+					brd->PrintDriver,
+					brd->firstminor + i, NULL);
 
-		classp = tty_register_device(brd->PrintDriver, j,
-						&(ch->ch_bd->pdev->dev));
-		ch->ch_pun.un_sysfs = classp;
 		dgap_create_tty_sysfs(&ch->ch_pun, classp);
+		ch->ch_pun.un_sysfs = classp;
 	}
 	dgap_create_ports_sysfiles(brd);
+
+	return 0;
 }
 
 
diff --git a/drivers/staging/dgap/dgap.h b/drivers/staging/dgap/dgap.h
index 573aa18..24db468 100644
--- a/drivers/staging/dgap/dgap.h
+++ b/drivers/staging/dgap/dgap.h
@@ -703,7 +703,6 @@ struct macounter
 #define	BD_FEP5PLUS	0x0001          /* Supports FEP5 Plus commands */
 #define BD_HAS_VPD	0x0002		/* Board has VPD info available */
 
-
 /*
  *	Per-board information
  */
@@ -761,8 +760,10 @@ struct board_t
 	struct channel_t *channels[MAXPORTS]; /* array of pointers to our channels. */
 
 	struct tty_driver	*SerialDriver;
+	struct tty_port *SerialPorts;
 	char		SerialName[200];
 	struct tty_driver	*PrintDriver;
+	struct tty_port *PrinterPorts;
 	char		PrintName[200];
 
 	u32		dgap_Major_Serial_Registered;
-- 
1.8.1.4

Re: [PATCH v2] staging: dgap: fix kernel oops on port open

[Dan Carpenter]

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
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: dgap: fix kernel oops on port open

[Mark Hounschell]

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
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: dgap: fix kernel oops on port open

[Greg Kroah-Hartman]

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
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: dgap: fix kernel oops on port open

[Dan Carpenter]

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

Re: [PATCH v2] staging: dgap: fix kernel oops on port open

[Dan Carpenter]

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;

Re: [PATCH v2] staging: dgap: fix kernel oops on port open

[Mark Hounschell]

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 checkpatch warnings/errors that are corrected in later patches.
Will this be OK? I think the review process will be much easier this way?

Thanks
Mark

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

Re: [PATCH v2] staging: dgap: fix kernel oops on port open

[Dan Carpenter]

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
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel