On Wednesday 26 May 2010 11:07:15 am Matthew Dharm wrote:
> On Tue, May 25, 2010 at 06:51:48PM -0600, Steve McKown wrote:
> > On Friday 21 May 2010 11:22:19 am Matthew Dharm wrote:
> > > On Fri, May 21, 2010 at 08:25:19AM -0600, Steve McKown wrote:
> > > > Nice! I think the data written to /sys/blah/blah/configuration
> > > > should have a magic string at the beginning and a CRC field at the
> > > > end, so the driver can validate the input. This would prevent
> > > > accidental writing of bad data to the cp210x, and perhaps mitigate
> > > > the need for the lock/unlock files.
> > >
> > > I disagree here. Adding the requirement for a header w/ magic string
> > > and CRC means that, practically, you need a userspace library to use
> > > this interface.
> > >
> > > Maybe you need a "magic string" to unlock (instead of just "echo 1",
> > > you could use "echo UnLockMe".
> > >
> > > But the individual value fields should be
> > > easily readable/writeable without hoops to jump through.
> >
> > Since the USB code already provides a mechanism to read the descriptor
> > fields, we only need to provide the ability to write those descriptors
> > that are writable, and both read and write the port configuration.
> >
> > By 'easily readable/writable', do you mean relative to a human
> > manipulating the data? The port configuration structure as sent to the
> > cp210x is shown below. Bit positions in the __u16 fields correspond to
> > different communicaitons pins on the internal port, like GPIO_0, RxD,
> > SUSPEND, etc.
>
> Generally, yes, I mean "relative to a human".
>
> > /* Port config definitions */
> > struct cp210x_port_state {
> > __u16 mode; /* Push-pull = 1, Open-drain = 0 */
> > __u16 lowPower;
> > __u16 latch; /* Logic high = 1, Logic low = 0 */
> > } __attribute__((packed));
> >
> > struct cp210x_port_config {
> > struct cp210x_port_state reset;
> > struct cp210x_port_state suspend;
> > __u8 enhancedFxn;
> > } __attribute__((packed));
> >
> > The driver could of course accept an ascii representation of this data,
> > then parse it internally to create the necessary binary structure to push
> > to the device. Does this logic best reside in the driver? One
> > alternative is a userspace utility that a creates the necessary binary
> > data as an output stream that could be written via sysfs.
>
> If you are going to have a userspace utility, then why do this in the
> driver at all? You might as well use libusb to commicate directly with the
> device. Making the user use a utility *and* then feed that to the driver
> seems silly.
Good point. Since programming the device configuration is a quite infrequent
behavior, perhaps there is less justification for keeping its implementation
in the driver proper...
>
> One could easily imagine the above example implemented as two sysfs files:
> gpio_reset_state and gpio_suspend_state. Each file would readback and
> accept for writes a single line containing 3 16-bit hex numbers
> (representing mode, lowPower, and latch).
>
> > > As for preventing "accidental writing of bad data" -- there are two
> > > parts to this: "accidental writing" and "bad data". "Accidental
> > > writing" is easily solved with lock/unlock.
> > > "Bad Data" can't be prevented, unless you
> > > can validate the data somehow (which, in the case of VID/PID or
> > > strings, you really can't do).
> >
> > I really think the configuration data should be (1) somewhat hard to
> > change, and (2) relatively easy to verify as valid at time of write to a
> > cp210x.
> >
> > (1) Why somewhat hard to change? The port configuration is intimately
> > tied to the hardware design. Change it and break the device.
> > Applications interacting with the device likely rely on the integrity of
> > the descriptor fields. All configuration data, except serial number, are
> > constant across all devices of the same model/version/revision. I view
> > this kind of configuration data as part of the device firmware, loaded
> > once at production time. Changing it after the fact is largely
> > unnecessary and only a source for problems for users and support people
> > alike.
>
> I disagree about this point. The data should be hard to change
> *accidentally*. If I want to change it, I bear the responsibility of
> knowing what I'm doing and not borking the device. But, it is not the
> responsibility of the driver to protect the user from their own stupidity.
You're right. I'm trying to filter out inappropriate changes through
management of the workflows in which they are made. I know that some
progress can be made using this approach because companies do it all the
time. Ironically, I've argued against this behavior in other situations.
Instead, the alternative is to accept that these parameters can be changed and
accommodate this fact in how the device is created, used and supported. I
can walk through each of my points of concern and find alternative solutions
that follow this model.
Thanks, Matt, this has been a very useful discussion. Given all this, if
configuration is best implemented in the driver, your lock concept seems the
the best option expressed thus far.
> [snip]
> > (2) Why verify at time of write to device? Better to catch data
> > corruption as soon as possible. Consider a production environment where
> > 1,000 devices are programmed a week. Failing to catch a corrupted
> > configuration for even a day means non-trivial unit rework and increased
> > production costs. Because the configuration may be unchanged for several
> > years (at least for us small shops!), the possibility of its corruption
> > from various causes human and other is that much greater, as it sits
> > there on the production server.
>
> How will you "verify" the data? What criteria could you possibly use? I
> may have very legitimate reasons for using a custom VID/PID, or any
> arbitrary GPIO configuration.
>
> Or do you mean integrity verification via the CRC? If that's what you
> mean, then that really falls into the first category of accidental
> mis-programming.
>
> Really, I don't put much weight in your example of silent corruption in a
> medium-scale production facility. That's a matter of implementing proper
> change control (for human errors) and writing your programming/test
> procedures properly (for causes "other") to catch this before it gets
> applied to 1k units; do an immediate readback-verify after programming, for
> example.
I gave a bad example, not comparing two equivalent solutions. CRC is a nice
end-to-end (developer-to-device) verification of the config data, but I was
viewing it as a "freebie". It doesn't make sense to me either, if the CRC
didn't have a higher purpose in the design.
>
> If this is what you mean, then I see why you want the separate userspace
> utility. If the utility is not on the production system, then there is no
> way for the datafile to be corrupted with something that would pass the CRC
> test. But, again, I think you're over-complicating the system to defend
> against something that shouldn't happen if you have proper processes in
> place.
>
> Matt
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
