> -----Original Message-----
quoted text
> 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
quoted text
> merge or at least be in -mm for a while (I think this used to be a
rule some
quoted text
> 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
quoted text
> undefined state when the first function fails (esp. when you interrupt
the sem)

Hmm..  hadn't realized that.  I'm open to suggestions on how to do this
better.  The real reason why the synchronization is there is to make
sure that only one client is using the device at a time, using the
is_open flag.

quoted text
> - semaphores are deprecated
> - class_device_create is deprecated

What is preferred?

quoted text
> - module_init/exit functions should be __init, not __devinit/exit (not
a bug,
quoted text
> 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");
quoted text
>                  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

easily fixed.

quoted text
> - don't understand this:
>                  memcpy(kbuf, drvdata->read_buffer, bytes_remaining);
>                  drvdata->read_buffer_in_use = bytes_remaining;
>                  free_page((unsigned long)kbuf);

The physical device only generates/accepts complete words, the intention
is to account for the possibility that a read does not read complete
words, and to fulfill the read whenever possible.
It's arguable that read() and write() should not accept or return
partial words, I suppose

quoted text
> - can this overlap (=>memmove)?
>                  memcpy(drvdata->read_buffer + bytes_to_read,
>                                  drvdata->read_buffer, 4 -
bytes_to_read);
quoted text
> - is platform probing function race-proof (like pci)?

no idea.  even if it is, it's unlikely that the of_driver probing is
protected by the same lock as the platform_driver probing.

quoted text
> - run sparse on it, you mix __user with non-__user at least

I thought I had been.... hmm... looks like a few other things to fix
there, too.

Steve


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