On Wed, Mar 24, 2010 at 6:56 PM, Peter Hutterer <[email protected]> wrote: > On Wed, Mar 24, 2010 at 06:15:31PM -0700, Ping Cheng wrote: >> On Wed, Mar 24, 2010 at 5:15 PM, Peter Hutterer >> <[email protected]> wrote: >> > On Wed, Mar 24, 2010 at 03:13:10PM -0700, Ping Cheng wrote: >> >> We might not get all the valid types for ISDV4 devices in >> >> wcmPreInit(). Assuming users know what they are doing so we make their >> >> types valid. >> >> >> >> Signed-off-by: Ping Cheng <[email protected]> >> > >> >> From fcf8974b5f9ee3d515a12296467efc0a14b6ce82 Mon Sep 17 00:00:00 2001 >> >> From: Ping Cheng <[email protected]> >> >> Date: Wed, 24 Mar 2010 13:54:32 -0700 >> >> Subject: [PATCH] User defined ISDV4 device types in xorg.conf are always >> >> valid >> >> >> >> We might not get all the valid types for ISDV4 devices in >> >> wcmPreInit(). Assuming users know what they are doing so we >> >> make their types valid. >> >> >> >> Signed-off-by: Ping Cheng <[email protected]> >> >> --- >> >> src/wcmValidateDevice.c | 28 +++++++++++++++++++++++++++- >> >> 1 files changed, 27 insertions(+), 1 deletions(-) >> >> >> >> diff --git a/src/wcmValidateDevice.c b/src/wcmValidateDevice.c >> >> index f153cd2..f121fb1 100644 >> >> --- a/src/wcmValidateDevice.c >> >> +++ b/src/wcmValidateDevice.c >> >> @@ -25,6 +25,7 @@ >> >> #include <sys/stat.h> >> >> #include <fcntl.h> >> >> #include <unistd.h> >> >> +#include <linux/serial.h> >> >> >> >> #define ARRAY_SIZE(a) (sizeof(a)/sizeof((a)[0])) >> >> >> >> @@ -139,6 +140,10 @@ Bool wcmIsAValidType(LocalDevicePtr local, const >> >> char* type) >> >> int j, ret = FALSE; >> >> WacomDevicePtr priv = (WacomDevicePtr)local->private; >> >> WacomCommonPtr common = priv->common; >> >> + char* dsource = xf86CheckStrOption(local->options, "_source", ""); >> >> + char* device = xf86SetStrOption(local->options, "Device", NULL); >> >> + int fd = -1; >> >> + struct serial_struct tmp; >> >> >> >> if (!type) >> >> return FALSE; >> >> @@ -147,11 +152,32 @@ Bool wcmIsAValidType(LocalDevicePtr local, const >> >> char* type) >> >> for (j = 0; j < ARRAY_SIZE(wcmType); j++) >> >> { >> >> if (!strcmp(wcmType[j].type, type)) >> >> - if (ISBITSET (common->wcmKeys, wcmType[j].tool)) >> >> + { >> >> + SYSCALL(fd = open(device, O_RDONLY)); >> > >> > file descriptor is leaked. >> >> Nice catch! Will fix that. >> >> >> >> + if (fd < 0) >> >> { >> >> + xf86Msg(X_WARNING, "%s: failed to open %s >> >> in " >> >> + "wcmIsAValidType.\n", local->name, device); >> >> + return FALSE; >> >> + } >> >> + >> >> + /* tool defined in xorg.conf and is an ISDV4 >> >> device? */ >> >> + if (!strlen(dsource) && !ioctl(fd, TIOCGSERIAL, >> >> &tmp)) >> >> + { >> >> + /* We might not get all the valid types for >> >> + * ISDV4 devices at this stage. Assuming >> >> user >> >> + * is right, let's make the type valid */ >> >> + common->wcmKeys[LONG(wcmType[j].tool)] >> >> + |= BIT(wcmType[j].tool); >> >> ret = TRUE; >> >> break; >> >> } >> >> + else if (ISBITSET (common->wcmKeys, >> >> wcmType[j].tool)) >> >> + { >> >> + ret = TRUE; >> >> + break; >> >> + } >> >> + } >> >> } >> >> return ret; >> >> } >> >> -- >> >> 1.6.6.1 >> > >> > thanks for the patch. A few comments: >> > This approach seems expensive. >> >> Yes, it is an expensive situation. Otherwise, we would have to open >> the serial port and query the actual data at the first place. We >> could go with that path if you feel that is cheaper than this one (I >> have no personal preference :). >> >> > for USB devices we can already assume that >> > the keys are set correctly but with this patch we now open the FD on every >> > one of them, do some checks, etc. before checking the bits. >> > I think it'd be better to check the keys first and only if they are >> > invalid, >> > then check the actual file. >> >> Yeah, I see your point. That can be done. >> >> >> > Having said that, this code seems in the wrong place. >> > >> > wcmIsAValidType() is called from only two locations - wcmNeedAutoHotplug() >> > and directly from wcmPreInit(). Only in the latter case are the checks >> > above >> > sensible. Why not move the check into isdv4ProbeKeys(), since we do all the >> > opening of the device file, the check for serial, etc. there anyway. It >> > should be trivial to add the bit flags for the device if it comes from the >> > xorg.conf. >> >> We can not move the check into isdv4ProbeKeys since it would only be >> called once for the first tool on the port, which leaves all the other >> valid tools as invalid. This patch is mainly for those ISDV4 devices >> that we could not find their id's (such as Fujitsu ones) in >> isdv4ProbeKeys for some reason. So, each type would have to be added >> manually. > > I think we have two separate issues here.
You are right. There are two issues here. I should say that the fix was inspired by supporting the two Fujitsu requests since they both had touch device and xorg.conf. Our default doesn't support touch type. > the fujitsu ones still provide some ID of the device, don't they? > looking at the wacom code, it only does the device name check, hence a > device WACf09 is id 0x09. the same goes for fujitsu devices, we could > extract the device ID from there and do the same key assignment as for > wacoms. All that should be needed there is another sscanf and an extension > of the ID table (or some way to assign wacom ids to the fujitsu devices). > This is the first issue. I will make another patch to cover this issue (I've already had one for linuxwacom when I supported the Fujitsu cases). >> Another way to handle this special case might be to trust our users to >> the most and make all xorg.conf defined types valid. It is cheap. But >> I don't know if I can trust myself all the time :). > > The issue is to accept the type set in the xorg.conf, regardless of the key > bits. Assuming the user knows what they're doing when they're editing the > xorg.conf can be risky, so the question is - if we have the first fix in, do > we still need to solve the second issue? Yes, we still need to resolve the second issue since there are types that we can not validate before querying the device. > What benefit do we have to let a user specify the type "stylus" if the > device doesn't have the required bits set? No benefit if the device doesn't support the type. But not much harm either since the driver would still work. However, if we don't support an valid type, users would be confused. It hurts even if they don't complain. Plus, if we take backward compatibility into the discussion, covering xorg.conf as much as we can is a way to make our "loyal" users happy. > Shouldn't we work on having the device have the bits available as required? In theory, yes we should. In reality, it would be hard. We are talking about OEM support here. There are decisions that the OEM vendor can not make for its users. So, I'd choose the cheap and effective solution: let all xorg.conf defined types pass. Why cannot we trust our smart users one more time, especially they are Linux users :)? Ping ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev _______________________________________________ Linuxwacom-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
