[PATCH RFC only] staging: dgap: fix OOPS on open of port

[PATCH RFC only] staging: dgap: fix OOPS on open of port

[Mark Hounschell]

When opening a port the dgap driver OOPs with a message:

tty_init_dev: driver does not set tty->port... will crash the kernel... fix the driver... etc...

Then I have to reboot the box.

I think before too much more work is done on this driver (by me anyway), 
it should at least be in a usable state. There are a lot more changes to come 
and I would like to be able to "test" along the way.

I've looked at some of the other drivers as examples and come up with this patch that 
does in fact allow me to use the driver. I would like to submit it but am uncertain that this
is proper.   

Thanks for reviewing.
mark

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 7cb1ad5..d56b3b2 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_sysfs_create(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_sysfs_create(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].port);
 			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].port);
 			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,51 @@ 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_sysfs_create(struct board_t *brd)
 {
 	struct channel_t *ch;
-	int j = 0;
+	struct dgap_port *p;
+	int j;
+
+	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 (j = 0, p = brd->SerialPorts; j < brd->nasync; j++, p++)
+		tty_port_init(&p->port);
+
+	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 (j = 0, p = brd->PrinterPorts; j < brd->nasync; j++, p++)
+		tty_port_init(&p->port);
 
 	ch = brd->channels[0];
 	for (j = 0; j < brd->nasync; j++, ch = brd->channels[j]) {
 		struct device *classp;
-		classp = tty_register_device(brd->SerialDriver, j,
-						&(ch->ch_bd->pdev->dev));
+
+		classp = tty_port_register_device(&brd->SerialPorts[j].port,
+				brd->SerialDriver, brd->firstminor + j, 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[j].port,
+				brd->PrintDriver, brd->firstminor + j, 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..656d3ad 100644
--- a/drivers/staging/dgap/dgap.h
+++ b/drivers/staging/dgap/dgap.h
@@ -703,6 +703,9 @@ struct macounter
 #define	BD_FEP5PLUS	0x0001          /* Supports FEP5 Plus commands */
 #define BD_HAS_VPD	0x0002		/* Board has VPD info available */
 
+struct dgap_port {
+	struct tty_port port;
+};
 
 /*
  *	Per-board information
@@ -761,8 +764,10 @@ struct board_t
 	struct channel_t *channels[MAXPORTS]; /* array of pointers to our channels. */
 
 	struct tty_driver	*SerialDriver;
+	struct dgap_port *SerialPorts;
 	char		SerialName[200];
 	struct tty_driver	*PrintDriver;
+	struct dgap_port *PrinterPorts;
 	char		PrintName[200];
 
 	u32		dgap_Major_Serial_Registered;
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC only] staging: dgap: fix OOPS on open of port

[Greg KH]

On Tue, Feb 25, 2014 at 10:56:22AM -0500, Mark Hounschell wrote:
> When opening a port the dgap driver OOPs with a message:
> 
> tty_init_dev: driver does not set tty->port... will crash the kernel... fix the driver... etc...
> 
> Then I have to reboot the box.
> 
> I think before too much more work is done on this driver (by me anyway), 
> it should at least be in a usable state. There are a lot more changes to come 
> and I would like to be able to "test" along the way.
> 
> I've looked at some of the other drivers as examples and come up with this patch that 
> does in fact allow me to use the driver. I would like to submit it but am uncertain that this
> is proper.   
> 
> Thanks for reviewing.
> mark
> 
> diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
> index 7cb1ad5..d56b3b2 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_sysfs_create(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_sysfs_create(brd);
> +	if (ret)
> +		return ret;

Why are you putting the port initialization logic in the sysfs_create()
function?

Ideally we will get rid of that sysfs logic as a tty driver shouldn't be
creating special sysfs files for no real reason.

>  /*
>   * Create pr and tty device entries
>   */
> -static void dgap_sysfs_create(struct board_t *brd)
> +static int dgap_sysfs_create(struct board_t *brd)

I think the function name is misleading, this does the tty registration,
and you are right to do it here.  Just fix the name :)

>  {
>  	struct channel_t *ch;
> -	int j = 0;
> +	struct dgap_port *p;
> +	int j;
> +
> +	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 (j = 0, p = brd->SerialPorts; j < brd->nasync; j++, p++)
> +		tty_port_init(&p->port);
> +
> +	brd->PrinterPorts = kcalloc(brd->nasync, sizeof(*brd->PrinterPorts),
> +					GFP_KERNEL);

What are "printer ports"?  How are they different from "serial ports" on
this device?

> +struct dgap_port {
> +	struct tty_port port;
> +};

Do you really need a wrapping structure here?

thanks,

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

Re: [PATCH RFC only] staging: dgap: fix OOPS on open of port

[Mark Hounschell]

On 02/25/2014 11:48 AM, Greg KH wrote:
> On Tue, Feb 25, 2014 at 10:56:22AM -0500, Mark Hounschell wrote:
>> When opening a port the dgap driver OOPs with a message:
>>
>> tty_init_dev: driver does not set tty->port... will crash the kernel... fix the driver... etc...
>>
>> Then I have to reboot the box.
>>
>> I think before too much more work is done on this driver (by me anyway),
>> it should at least be in a usable state. There are a lot more changes to come
>> and I would like to be able to "test" along the way.
>>
>> I've looked at some of the other drivers as examples and come up with this patch that
>> does in fact allow me to use the driver. I would like to submit it but am uncertain that this
>> is proper.
>>
>> Thanks for reviewing.
>> mark
>>
>> diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
>> index 7cb1ad5..d56b3b2 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_sysfs_create(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_sysfs_create(brd);
>> +	if (ret)
>> +		return ret;
>
> Why are you putting the port initialization logic in the sysfs_create()
> function?
>
> Ideally we will get rid of that sysfs logic as a tty driver shouldn't be
> creating special sysfs files for no real reason.
>
>>   /*
>>    * Create pr and tty device entries
>>    */
>> -static void dgap_sysfs_create(struct board_t *brd)
>> +static int dgap_sysfs_create(struct board_t *brd)
>
> I think the function name is misleading, this does the tty registration,
> and you are right to do it here.  Just fix the name :)
>

That's easy enough. Will do in patch.

>>   {
>>   	struct channel_t *ch;
>> -	int j = 0;
>> +	struct dgap_port *p;
>> +	int j;
>> +
>> +	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 (j = 0, p = brd->SerialPorts; j < brd->nasync; j++, p++)
>> +		tty_port_init(&p->port);
>> +
>> +	brd->PrinterPorts = kcalloc(brd->nasync, sizeof(*brd->PrinterPorts),
>> +					GFP_KERNEL);
>
> What are "printer ports"?  How are they different from "serial ports" on
> this device?
>

As I'm not the original author of this driver, I'm not entirely certain 
but this driver creates these device entries per board. Given an 8 port 
board variant:

/dev/dgap_mgmt    special managment node  1 per driver instance

/dev/pr_dgap_0_0   port 0 used for printer
/dev/pr_dgap_0_1
/dev/pr_dgap_0_2
/dev/pr_dgap_0_3
/dev/pr_dgap_0_4
/dev/pr_dgap_0_5
/dev/pr_dgap_0_6
/dev/pr_dgap_0_7

/dev/tty_dgap_0_0  port 0 used anything other than a printer
/dev/tty_dgap_0_1
/dev/tty_dgap_0_2
/dev/tty_dgap_0_3
/dev/tty_dgap_0_4
/dev/tty_dgap_0_5
/dev/tty_dgap_0_6
/dev/tty_dgap_0_7

I personally have not used the printer device nodes. Nor have I looked 
into the driver deep enough to see why they are provided. I do remember 
seeing something about Transparent Print though. We have some old rs232 
terminals with an additional rs232 printer port output that can be 
configured in "Transparent Print" mode such that certain things go 
straight though to the printer while other things show on the terminal? 
I just don't know for sure though.

As far as the dgap_mgmt device, some code using it was already removed 
before or immediately after it got merged into staging. I think there 
were only 3 or 4 ioctls that used it. These ioctls were for things like 
setting "special baud rates", statistics gathering, monitoring, etc... 
IMHO, for user land compatibility I think that stuff should be added 
back in before this driver leaves staging. Another day though.

>> +struct dgap_port {
>> +	struct tty_port port;
>> +};
>
> Do you really need a wrapping structure here?
>

I may be incorrect, but I think so. I will investigate this further 
before I make a patch.


Thanks
mark

Re: [PATCH RFC only] staging: dgap: fix OOPS on open of port

[Mark Hounschell]

On 02/25/2014 01:40 PM, Mark Hounschell wrote:

>
>>> +struct dgap_port {
>>> +    struct tty_port port;
>>> +};
>>
>> Do you really need a wrapping structure here?
>>
>
> I may be incorrect, but I think so. I will investigate this further
> before I make a patch.
>

It looks to me like this is how it should be done. As future serial 
changes are done won't additional things be required to be added to 
dgap_port? port_ops? I certainly don't know what those future changes 
are but looking at other drivers (moxa.c) it looks so. What am I missing?

Thanks
mark

Re: [PATCH RFC only] staging: dgap: fix OOPS on open of port

[Dan Carpenter]

On Tue, Feb 25, 2014 at 02:35:39PM -0500, Mark Hounschell wrote:
> On 02/25/2014 01:40 PM, Mark Hounschell wrote:
> 
> >
> >>>+struct dgap_port {
> >>>+    struct tty_port port;
> >>>+};
> >>
> >>Do you really need a wrapping structure here?
> >>
> >
> >I may be incorrect, but I think so. I will investigate this further
> >before I make a patch.
> >
> 
> It looks to me like this is how it should be done. As future serial
> changes are done won't additional things be required to be added to
> dgap_port? port_ops? I certainly don't know what those future
> changes are but looking at other drivers (moxa.c) it looks so. What
> am I missing?

In the kernel we hate baggage like this.  Maybe it's a wrapper for
some historical reason, to support some other operating system or some
future reason; we hate it all.  Don't try to plan ahead, just write it
as it exists now because no one can predict the future.  Changing it
later is easier than everyone assumes.

regards,
dan carpenter