Xilinx: hwicap driver comments

Jiri Slaby
2008-02-07T20:12:42Z

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)
- semaphores are deprecated
- 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 record\n");
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
- don't understand this:
memcpy(kbuf, drvdata->read_buffer, bytes_remaining);
drvdata->read_buffer_in_use = bytes_remaining;
free_page((unsigned long)kbuf);
- can this overlap (=>memmove)?
memcpy(drvdata->read_buffer + bytes_to_read,
drvdata->read_buffer, 4 - bytes_to_read);
- is platform probing function race-proof (like pci)?
- run sparse on it, you mix __user with non-__user at least