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