On Tue, Mar 03, 2015 at 01:19:52PM +0100, Noralf Trønnes wrote:
> >>+static ssize_t lcdreg_debugfs_read_wr(struct file *file,
> >>+ const char __user *user_buf,
> >>+ size_t count, loff_t *ppos)
> >>+{
> >>+ struct lcdreg *reg = file->private_data;
> >>+ struct lcdreg_transfer tr = {
> >>+ .index = (reg->quirks & LCDREG_INDEX0_ON_READ) ? 0 : 1,
> >>+ .width = reg->debugfs_read_width,
> >>+ .count = 1,
> >>+ };
> >>+ u32 txbuf[1];
> >>+ char *buf = NULL;
> >>+ int ret;
> >>+
> >>+ ret = lcdreg_userbuf_to_u32(user_buf, count, txbuf, ARRAY_SIZE(txbuf));
> >>+ if (ret != 1)
> >>+ return -EINVAL;
> >>+
> >>+ tr.buf = kmalloc(lcdreg_bytes_per_word(tr.width), GFP_KERNEL);
> >>+ if (!tr.buf)
> >>+ return -ENOMEM;
> >>+
> >>+ add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> >>+
> >>+ lcdreg_lock(reg);
> >>+ ret = lcdreg_read(reg, txbuf[0], &tr);
> >>+ lcdreg_unlock(reg);
> >>+ if (ret)
> >>+ goto error_out;
> >>+
> >>+ if (!reg->debugfs_read_result) {
> >>+ reg->debugfs_read_result = kmalloc(16, GFP_KERNEL);
> >>+ if (!reg->debugfs_read_result) {
> >>+ ret = -ENOMEM;
> >>+ goto error_out;
> >>+ }
> >>+ }
> >Allocating here is strange. We only free ->debugfs_read_result on
> >error so it's sort of buggy as well. Also is it racy if we have
> >multiple readers and writers at once?
> I spent some time on figuring out how to do this.
> To read a register, first write the register number to the 'read' file.
> Then get the result by reading the 'read' file.
> So I have to keep the result somewhere, but you're right this leaks
> memory. Maybe I can use devm_kmalloc here.
> And yes this is racy. I'm not sure if this is a problem.
> This is to be used during development or debugging, so
> the user should have this under control.
>
> But I'm open to other ways of doing this. This is the best I could
> come up with after several iterations on this code.
> My other solutions where worse :-)
We can't do it probe()? We could add another flag to say if we had
written to it. I haven't looked at this closely so whatever you decide
is fine.
> >>+#if defined(CONFIG_DYNAMIC_DEBUG)
> >>+
> >>+#define lcdreg_dbg_transfer_buf(transfer) \
> >>+do { \
> >>+ int groupsize = lcdreg_bytes_per_word(transfer->width); \
> >>+ size_t len = min_t(size_t, 32, transfer->count * groupsize); \
> >>+ \
> >>+ print_hex_dump_debug(" buf=", DUMP_PREFIX_NONE, 32, \
> >>+ groupsize, transfer->buf, len, false); \
> >>+} while (0)
> >>+
> >>+#elif defined(DEBUG)
> >>+
> >>+#define lcdreg_dbg_transfer_buf(transfer) \
> >>+do { \
> >>+ int groupsize = lcdreg_bytes_per_word(transfer->width); \
> >>+ size_t len = min_t(size_t, 32, transfer->count * groupsize); \
> >>+ \
> >>+ print_hex_dump(KERN_DEBUG, " buf=", DUMP_PREFIX_NONE, 32, \
> >>+ groupsize, transfer->buf, len, false); \
> >>+} while (0)
> >I don't understand this. Why can't we use print_hex_dump_debug() for
> >both? In other words:
> >
> >#if defined(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
> >...
> >#else
> The reason I do this is to be able to switch on/off debugging
> quickly when developing.
> I have dynamic debug enabled, but it's more cumbersome to use, so I
> define DEBUG
> when I need this output.
> print_hex_dump_debug() doesn't look at DEBUG, it only cares about
> DYNAMIC_DEBUG.
> But, it's not suited for production code, so I guess I have to use a
> script or something
> to easily switch dynamic debug on/off instead.
>
Fine fine. Whatever you decide is ok.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
