RE: Xilinx: hwicap driver comments

[Stephen Neuendorffer]

Missed the 'send' button on this last night.

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

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


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