[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