Reviewing drivers/staging/unisys/visornic/visornic_main.c
212 static int
213 visor_copy_fragsinfo_from_skb(struct sk_buff *skb, unsigned int firstfraglen,
214 unsigned int frags_max,
215 struct phys_info frags[])
216 {
217 unsigned int count = 0, ii, size, offset = 0, numfrags;
^^
The second "i" character indicates that it is an integer. Which is ugly
and, of course, misleading.
296 static ssize_t enable_ints_write(struct file *file,
297 const char __user *buffer,
298 size_t count, loff_t *ppos)
299 {
300 /*
301 * Don't want to break ABI here by having a debugfs
302 * file that no longer exists or is writable, so
303 * lets just make this a vestigual function
304 */
305 return count;
306 }
We don't care about ABI so much as we do about applications or scripts.
This was added by Neil Horman because the original code was useless and
dangerous. It was the right patch for him, because he doesn't know
about the userspace so he can't say if anything will break. But
presumably you guys know. Does anything break if we remove this debugfs
file?
Also if so then shame shame shame. User space is not supposed to depend
on debugfs being writable. Just delete this...
1019 */
1020 static void
1021 visornic_set_multi(struct net_device *netdev)
1022 {
1023 struct uiscmdrsp *cmdrsp;
1024 struct visornic_devdata *devdata = netdev_priv(netdev);
1025
1026 /* any filtering changes */
1027 if (devdata->old_flags != netdev->flags) {
1028 if ((netdev->flags & IFF_PROMISC) !=
1029 (devdata->old_flags & IFF_PROMISC)) {
Reverse these if statements and pull the code in two indent levels.
1154 /**
1155 * visornic_rx - Handle receive packets coming back from IO Part
1156 * @cmdrsp: Receive packet returned from IO Part
1157 *
1158 * Got a receive packet back from the IO Part, handle it and send
1159 * it up the stack.
1160 * Returns void
Actually it returns either zero or one but in a convoluted way. Update
it to use literals and update the comment.
1361 /**
1362 * devdata_initialize - Initialize devdata structure
1363 * @devdata: visornic_devdata structure to initialize
1364 * #dev: visorbus_deviced it belongs to
1365 *
1366 * Setup initial values for the visornic based on channel and default
1367 * values.
1368 * Returns a pointer to the devdata if successful, else NULL
1369 */
1370 static struct visornic_devdata *
1371 devdata_initialize(struct visornic_devdata *devdata, struct visor_device *dev)
1372 {
1373 if (!devdata)
1374 return NULL;
This check doesn't make sense and the error handling is kind of odd.
Just remove it, remove the error handling and update the comment.
1817 err = visorbus_read_channel(dev, channel_offset,
1818 &devdata->num_rcv_bufs, 4);
1819 if (err) {
1820 dev_err(&dev->device,
1821 "%s failed to get #rcv bufs from chan (%d)\n",
1822 __func__, err);
1823 goto cleanup_netdev;
1824 }
1825
1826 devdata->rcvbuf = kcalloc(devdata->num_rcv_bufs,
1827 sizeof(struct sk_buff *), GFP_KERNEL);
1828 if (!devdata->rcvbuf) {
1829 err = -ENOMEM;
1830 goto cleanup_rcvbuf;
We never allocated rcvbuf. It should still be goto cleanup_netdev; The
other gotos after this in this function are slightly off as well. But
thanks for the nice label names because it makes reviewing this much
easier. :)
1831 }
In visornic_init():
2126 err = visorbus_register_visor_driver(&visornic_driver);
2127 if (!err)
2128 return 0;
Flip this around.
if (err)
goto cleanup_debugfs;
return 0;
These things should be reviewed by people on netdev and the scsi thing
should get a review by the scsi people.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
