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.
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.
For example, many of the etherner drivers allow re-programming of the
EEPROM which contains MAC addresses and other critical configuration data
(via ethtool). These are "use at your own risk," but very valuable to
those of us who know what we're doing with those tools.
> (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.
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
--
Matthew Dharm Home: mdharm-usb@xxxxxxxxxxxxxxxxxx
Maintainer, Linux USB Mass Storage Driver
It's monday. It must be monday.
-- Greg
User Friendly, 5/4/1998
Attachment:
pgp0WEIq2oxRj.pgp
Description: PGP signature
