RE: Xilinx: hwicap driver comments

[Stephen Neuendorffer]

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

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.

> - semaphores are deprecated
> - class_device_create is deprecated

What is preferred?

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

easily fixed.

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

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

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.

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/