[lkml]   [2008]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRE: Xilinx: hwicap driver comments
Missed the 'send' button on this last night.

> -----Original Message-----
> From: Jiri Slaby []
> 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
> 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)


> - semaphores are deprecated
converted to mutexes.

> - class_device_create is deprecated

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

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

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

> - 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 -
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

Thanks alot for the comments!


 \ /
  Last update: 2008-02-08 18:25    [W:0.110 / U:11.528 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site