RE: Xilinx: hwicap driver comments

Stephen Neuendorffer
2008-02-08T17:20:41Z

Missed the 'send' button on this last night.

> -----Original Message-----
> From: Jiri Slaby [mailto:jirislaby@gmail.com]
> Sent: Thursday, February 07, 2008 12:09 PM
> To: Stephen Neuendorffer; Grant Likely
> Cc: Linux Kernel Mailing List; Andrew Morton
> Subject: Xilinx: hwicap driver comments
>
> Hi,
>
> first of all, I think that the driver should go through lkml before
upstream
> merge or at least be in -mm for a while (I think this used to be a
rule some
> time ago), correct me if I'm wrong, but none of it happened.
>
> Few comments I have:
> - release f_op retval is silently ignored, I guess you will get your
device into
> undefined state when the first function fails (esp. when you interrupt
the sem)

Fixed.

> - semaphores are deprecated
converted to mutexes.

> - class_device_create is deprecated
Fixed.

> - module_init/exit functions should be __init, not __devinit/exit (not
a bug,
> it's subset)
Fixed.

> - this piece:
> drvdata = kmalloc(sizeof(struct hwicap_drvdata), GFP_KERNEL);
> if (!drvdata) {
> dev_err(dev, "Couldn't allocate device private
record\n");
> return -ENOMEM;
> }
> memset((void *)drvdata, 0, sizeof(struct hwicap_drvdata));
>
> kmalloc + memset = kzalloc
Fixed.

> null probed_devices[id] on that fail path and on failed1 label
Fixed.

> - from/to (void *) casts are useless
> - io resources are at least ulong
Fixed to be resource_size_t.

> - don't understand this:
> memcpy(kbuf, drvdata->read_buffer, bytes_remaining);
> drvdata->read_buffer_in_use = bytes_remaining;
> free_page((unsigned long)kbuf);
Yeow. Fixed so the sense of the memcpy correct.

> - can this overlap (=>memmove)?
> memcpy(drvdata->read_buffer + bytes_to_read,
> drvdata->read_buffer, 4 -
bytes_to_read);
Yes, it can! fixed.

Given the length of time that these errors have been there, I'm really
wondering if the added complexity of this buffer is worth it.

> - is platform probing function race-proof (like pci)?
Added another mutex around access to probed_devices

> - run sparse on it, you mix __user with non-__user at least
Fixed. There is one warning (related to a function that is not called),
but I'd like to leave this in, as it should get bound to an ioctl, most
likely.

Thanks alot for the comments!

Steve