Re: [ patch 4/7] drivers/serial/jsm: new serial device driver

[Russell King <rmk+lkml@arm.linux.org.uk>]

On Mon, Mar 07, 2005 at 10:44:25PM -0800, Greg KH wrote:
> On Mon, Mar 07, 2005 at 05:46:51PM -0500, Wen Xiong wrote:
> > +static ssize_t jsm_tty_baud_show(struct class_device *class_dev, char *buf)
> > +{
> > +	struct jsm_channel *ch;
> > +
> > +	if (class_dev) {
> > +		ch = class_get_devdata(class_dev);
> > +		if ( ch && (ch->ch_bd->state == BOARD_READY))
> > +		return snprintf(buf, PAGE_SIZE, "%d\n", ch->ch_old_baud);
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +static CLASS_DEVICE_ATTR(baud, S_IRUGO, jsm_tty_baud_show, NULL);
> 
> No, please delete these, and the other sysfs files that duplicate the
> same info that you can get by using the standard Linux termios calls.
> There is no need for them here.

Greg, there's several other points about why the above is Bad(tm).

"class_dev" will always be non-null.

Note that this (and similar code) is racy.  Consider what happens when
the class device is removed (and the class devdata is NULL'd or kfree'd)
while another process on another CPU reads from one of these sysfs files.
*BANG*.

Also, if a class device attribute method is going to access data outside
the same allocation which the class device is a part of, you *absolutely*
*must* have some form of locking.

Also note that the formatting of these snippets of code is abismal.
There is a missing tab which makes it very non-readable.

With all of the above comments, it should be something like:

+static ssize_t jsm_tty_baud_show(struct class_device *class_dev, char *buf)
+{
+	struct jsm_channel *ch;
+	int ret = -EINVAL;
+
+	down(&some_lock);
+	ch = class_get_devdata(class_dev);
+	if (ch && (ch->ch_bd->state == BOARD_READY))
+		ret = snprintf(buf, PAGE_SIZE, "%d\n", ch->ch_old_baud);
+	up(&some_lock);
+
+	return ret;
+}
+static CLASS_DEVICE_ATTR(baud, S_IRUGO, jsm_tty_baud_show, NULL);

where "some_lock" is also taken in the device unregistration path, at
the point where the class devdata is NULL'd out.  (which the driver is
also missing.)

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core