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