lkml.org 
[lkml]   [2005]   [Mar]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [ patch 4/7] drivers/serial/jsm: new serial device driver
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:11    [W:0.090 / U:0.292 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site