Alan Stern <[EMAIL PROTECTED]> writes:
>
> Here are some answers to your question. I don't know if you will find
> them very helpful...
Yes, they are helpful, they help me to be more ruthless w.r.t. those
fringe cases :-)
> On Wed, 25 Feb 2004, Juergen Stuber wrote:
>>
>> I'm currently working to correct the locking problems in the
>> LEGO USB Tower driver. In the course of that I've read through
>> the current version of usb-skeleton.c in 2.6.3, and I have
>> a few questions and remarks:
>>
>> - Parallel read and write are impossible due to locking on dev->sem.
>> Is that ok?
>
> It's okay for usb-skeleton. Whether or not it's okay for the LEGO Tower
> driver is up to you.
I guess it is ok then.
>> - Read and write aren't interruptible and don't support nonblocking I/O.
>> How important is it to support nonblocking I/O?
>
> Importance is a matter for the user to decide, not the kernel developer.
Well, the only way to let the user decide is to implement it,
which for usb-skeleton isn't done, and for legousbtower would
complicate the code somewhat.
But I think I'll do it for legousbtower, might be useful some day.
>> - It would be nice to use a bigger buffer size than the packet size
>> as that is typical.
>
> Then go ahead and do so.
Ok.
>> - Shouldn't seeking be forbidden by adding an llseek method
>> that forbids seeking by returning -ESPIPE?
>
> It wouldn't hurt. It's not clear whether it would help.
>
>> - I suppose __FUNCTION__ should be replaced by __func__.
>
> I've never heard of __func__. If it means something different from
> __FUNCTION__ then no, it shouldn't be replaced. If it means the same
> thing, then it would be okay (but probably nonstandard) to do the
> replacement.
It's the same, and actually __func__ is standard as of C99,
__FUNCTION__ is an older gcc extension.
>> - The module use count is decremented after release, not before as
>> written in the comment at line 151.
>
> So submit a patch to fix the comment.
Ok, here is a patch for the last four points:
--- usb-skeleton.c.orig 2004-02-18 04:57:49.000000000 +0100
+++ usb-skeleton.c 2004-02-25 19:33:29.000000000 +0100
@@ -71,6 +71,10 @@
MODULE_PARM(debug, "i");
MODULE_PARM_DESC(debug, "Debug enabled or not");
+static size_t buffer_size = 1024;
+MODULE_PARM(buffer_size, "i");
+MODULE_PARM_DESC(buffer_size, "Maximal USB transfer size");
+
/* Define these values to match your devices */
#define USB_SKEL_VENDOR_ID 0xfff0
@@ -126,6 +130,7 @@
static int skel_ioctl (struct inode *inode, struct file *file, unsigned int
cmd, unsigned long arg);
static int skel_open (struct inode *inode, struct file *file);
static int skel_release (struct inode *inode, struct file *file);
+static loff_t skel_llseek (struct file *file, loff_t off, int whence);
static int skel_probe (struct usb_interface *interface, const struct
usb_device_id *id);
static void skel_disconnect (struct usb_interface *interface);
@@ -148,7 +153,7 @@
* which module to increment the use-counter of
* BEFORE it calls the device's open() function.
* This also means that the kernel can decrement
- * the use-counter again before calling release()
+ * the use-counter again after calling release()
* or should the open() function fail.
*/
.owner = THIS_MODULE,
@@ -158,6 +163,7 @@
.ioctl = skel_ioctl,
.open = skel_open,
.release = skel_release,
+ .llseek = skel_llseek,
};
/*
@@ -224,7 +230,7 @@
int subminor;
int retval = 0;
- dbg("%s", __FUNCTION__);
+ dbg("%s", __func__);
subminor = iminor(inode);
@@ -234,7 +240,7 @@
interface = usb_find_interface (&skel_driver, subminor);
if (!interface) {
err ("%s - error, can't find device for minor %d",
- __FUNCTION__, subminor);
+ __func__, subminor);
retval = -ENODEV;
goto exit_no_device;
}
@@ -273,17 +279,17 @@
dev = (struct usb_skel *)file->private_data;
if (dev == NULL) {
- dbg ("%s - object is NULL", __FUNCTION__);
+ dbg ("%s - object is NULL", __func__);
return -ENODEV;
}
- dbg("%s - minor %d", __FUNCTION__, dev->minor);
+ dbg("%s - minor %d", __func__, dev->minor);
/* lock our device */
down (&dev->sem);
if (dev->open <= 0) {
- dbg ("%s - device not opened", __FUNCTION__);
+ dbg ("%s - device not opened", __func__);
retval = -ENODEV;
goto exit_not_opened;
}
@@ -308,6 +314,15 @@
}
+/**
+ * skel_llseek
+ */
+static loff_t skel_llseek (struct file *file, loff_t off, int whence)
+{
+ return -ESPIPE; /* unseekable */
+}
+
+
/**
* skel_read
*/
@@ -318,7 +333,7 @@
dev = (struct usb_skel *)file->private_data;
- dbg("%s - minor %d, count = %d", __FUNCTION__, dev->minor, count);
+ dbg("%s - minor %d, count = %d", __func__, dev->minor, count);
/* lock this object */
down (&dev->sem);
@@ -374,7 +389,7 @@
dev = (struct usb_skel *)file->private_data;
- dbg("%s - minor %d, count = %d", __FUNCTION__, dev->minor, count);
+ dbg("%s - minor %d, count = %d", __func__, dev->minor, count);
/* lock this object */
down (&dev->sem);
@@ -387,7 +402,7 @@
/* verify that we actually have some data to write */
if (count == 0) {
- dbg("%s - write request of 0 bytes", __FUNCTION__);
+ dbg("%s - write request of 0 bytes", __func__);
goto exit;
}
@@ -409,7 +424,7 @@
goto exit;
}
- usb_skel_debug_data (__FUNCTION__, bytes_written,
+ usb_skel_debug_data (__func__, bytes_written,
dev->write_urb->transfer_buffer);
/* this urb was already set up, except for this write size */
@@ -424,7 +439,7 @@
if (retval) {
atomic_set (&dev->write_busy, 0);
err("%s - failed submitting write urb, error %d",
- __FUNCTION__, retval);
+ __func__, retval);
} else {
retval = bytes_written;
}
@@ -455,7 +470,7 @@
return -ENODEV;
}
- dbg("%s - minor %d, cmd 0x%.4x, arg %ld", __FUNCTION__,
+ dbg("%s - minor %d, cmd 0x%.4x, arg %ld", __func__,
dev->minor, cmd, arg);
/* fill in your device specific stuff here */
@@ -475,13 +490,13 @@
{
struct usb_skel *dev = (struct usb_skel *)urb->context;
- dbg("%s - minor %d", __FUNCTION__, dev->minor);
+ dbg("%s - minor %d", __func__, dev->minor);
/* sync/async unlink faults aren't errors */
if (urb->status && !(urb->status == -ENOENT ||
urb->status == -ECONNRESET)) {
dbg("%s - nonzero write bulk status received: %d",
- __FUNCTION__, urb->status);
+ __func__, urb->status);
}
/* notify anyone waiting that the write has finished */
@@ -502,7 +517,6 @@
struct usb_skel *dev = NULL;
struct usb_host_interface *iface_desc;
struct usb_endpoint_descriptor *endpoint;
- size_t buffer_size;
int i;
int retval = -ENOMEM;
@@ -536,7 +550,6 @@
((endpoint->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
== USB_ENDPOINT_XFER_BULK)) {
/* we found a bulk in endpoint */
- buffer_size = endpoint->wMaxPacketSize;
dev->bulk_in_size = buffer_size;
dev->bulk_in_endpointAddr = endpoint->bEndpointAddress;
dev->bulk_in_buffer = kmalloc (buffer_size, GFP_KERNEL);
@@ -566,7 +579,6 @@
* more than one packet, so that i/o delays between
* packets don't hurt throughput.
*/
- buffer_size = endpoint->wMaxPacketSize;
dev->bulk_out_size = buffer_size;
dev->write_urb->transfer_flags = (URB_NO_TRANSFER_DMA_MAP |
URB_ASYNC_UNLINK);
J�rgen
--
J�rgen Stuber <[EMAIL PROTECTED]>
http://www.loria.fr/~stuber/
-------------------------------------------------------
SF.Net is sponsored by: Speed Start Your Linux Apps Now.
Build and deploy apps & Web services for Linux with
a free DVD software kit from IBM. Click Now!
http://ads.osdn.com/?ad_id=1356&alloc_id=3438&op=click
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
