> > > +		buffer[4] = (this_part - 12 - 3) & 255;
quoted text
> > > +		buffer[5] = ((this_part - 12 - 3) >> 8) & 255;
> > > +		buffer[6] = ((this_part - 12 - 3) >> 16) & 255;
> > > +		buffer[7] = ((this_part - 12 - 3) >> 24) & 255;
> > 
> > We have excellent endianness conversion macros.
> 
> For splitting values up into the individual byte portions?  I think this
> is far more obvious as to exactly what is going on, don't you?

No. Kernel code does not exist to show how to do endianness conversion.
We have clearly labeled functions.

quoted text
> > > +		buffer[8] = data->TermCharEnabled * 2;
> > > +		/* Use term character? */
> > 
> > smp_rmb(); /* we must make sure we don't read a stale terminator */
> 
> I'm not going to worry about races here, that's not a real issue.

There is no such thing as an ignorable race. On second thought
you can take the mutex in the sysfs handlers. That will also do the job.


quoted text
> > This and usbtmc_read() need a test for disconnection. Open() and disconnect
> > are guarded in usbcore, read & write are not. By reference count you've made
> > sure you have a valid device descriptor, but the device may have been reprobed.
> 
> If so, then struct usb_device would be different, right?  Oh, I see,
> disconnect() using usbfs/sysfs.  Bah, is it really something that
> happens in the real world?  Oh well, I'll go fix this...

This is oopsable from user space.

quoted text
> > If you ignore an error return, be open about it.
> 
> I'm not?  Should I print an error and then just continue on?  Would that
> be sufficient?

Yes.
 
quoted text
> > > +static void usbtmc_disconnect(struct usb_interface *intf)
> > > +{
> > > +	struct usbtmc_device_data *data;
> > > +
> > > +	dev_dbg(&intf->dev, "usbtmc_disconnect called\n");
> > > +
> > 
> > You must set a flag for read, write and ioctl.
> 
> Will do.  Then I need to lock the flag with a mutex, right?

Yes. You can set the intf pointer to NULL. That's sort of idiomatic.
And you should NULL intfdata.


	Regards
		Oliver


--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/