linux-kernel - Xilinx: hwicap driver comments

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <47AB6552.7040503@gmail.com>
Date:	Thu, 07 Feb 2008 21:08:50 +0100
From:	Jiri Slaby <jirislaby@...il.com>
To:	Stephen Neuendorffer <stephen.neuendorffer@...inx.com>,
	Grant Likely <grant.likely@...retlab.ca>
CC:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>
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)
- 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/