> -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > Sent: Monday, May 02, 2016 8:55 AM > To: Kershner, David A > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; > Sell, Timothy C; *S-Par-Maintainer; Binder, David Anthony > Subject: Re: Time for a code audit? > > 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. > Appreciate the feedback Dan; we'll get right on cleaning this stuff up. After we get these changes in, is there a specific process we need to follow in order to have visornic inspected by the netdev folks (assume netdev@xxxxxxxxxxxxxxx) and visorhba inspected by the scsi folks (assume linux-scsi@xxxxxxxxxxxxxxx)? I also assume we would need to do the same thing for visorinput (linux-input@xxxxxxxxxxxxxxx). Thanks, Tim Sell > regards, > dan carpenter _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
