| Message ID | 149365469232.12922.13451178429094271759.stgit@zeus.hardeman.nu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 01, 2017 at 06:04:52PM +0200, David Härdeman wrote: > The name is only used for a few debug messages and the name of the parent > device as well as the name of the lirc device (e.g. "lirc0") are sufficient > anyway. > > Signed-off-by: David Härdeman <david@hardeman.nu> > --- > drivers/media/rc/ir-lirc-codec.c | 2 -- > drivers/media/rc/lirc_dev.c | 25 ++++++++----------------- > drivers/staging/media/lirc/lirc_zilog.c | 1 - > include/media/lirc_dev.h | 3 --- > 4 files changed, 8 insertions(+), 23 deletions(-) > > diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c > index 2c1221a61ea1..74ce27f92901 100644 > --- a/drivers/media/rc/ir-lirc-codec.c > +++ b/drivers/media/rc/ir-lirc-codec.c > @@ -380,8 +380,6 @@ static int ir_lirc_register(struct rc_dev *dev) > if (dev->max_timeout) > features |= LIRC_CAN_SET_REC_TIMEOUT; > > - snprintf(drv->name, sizeof(drv->name), "ir-lirc-codec (%s)", > - dev->driver_name); > drv->features = features; > drv->data = &dev->raw->lirc; > drv->rbuf = NULL; > diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c > index 4ba6c7e2d41b..10783ef75a25 100644 > --- a/drivers/media/rc/lirc_dev.c > +++ b/drivers/media/rc/lirc_dev.c > @@ -28,8 +28,6 @@ > #include <media/lirc.h> > #include <media/lirc_dev.h> > > -#define LOGHEAD "lirc_dev (%s[%d]): " > - > static dev_t lirc_base_dev; > > /** > @@ -160,7 +158,6 @@ lirc_register_driver(struct lirc_driver *d) > return -EBADRQC; > } > > - d->name[sizeof(d->name)-1] = '\0'; > if (d->features == 0) > d->features = LIRC_CAN_REC_LIRCCODE; > > @@ -207,8 +204,7 @@ lirc_register_driver(struct lirc_driver *d) > if (err) > goto out_cdev; > > - dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n", > - ir->d.name, ir->d.minor); > + dev_info(ir->d.dev, "lirc device registered as lirc%d\n", minor); I'm not so sure this is a good idea. First of all, the documentation says you can use "dmesg |grep lirc_dev" to find your lirc devices and you've just replaced lirc_dev with lirc. https://linuxtv.org/downloads/v4l-dvb-apis/uapi/rc/lirc-dev-intro.html It's useful having the driver name in the message. For example, I have two rc devices connected usually: [sean@bigcore ~]$ dmesg | grep lirc_dev [ 5.938284] lirc_dev: IR Remote Control driver registered, major 239 [ 5.945324] rc rc0: lirc_dev: driver ir-lirc-codec (winbond-cir) registered at minor = 0 [ 5111.830118] rc rc1: lirc_dev: driver ir-lirc-codec (mceusb) registered at minor = 1 With the driver name I know which one is which. Maybe lirc_driver.name should be a "const char*" so no strcpy is needed (the ir-lirc-codec does not seem necessary). Thanks Sean > > d->lirc_internal = ir; > return 0; > @@ -242,13 +238,11 @@ lirc_unregister_driver(struct lirc_driver *d) > > mutex_lock(&ir->mutex); > > - dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n", > - ir->d.name, ir->d.minor); > + dev_dbg(&ir->dev, "unregistered\n"); > > ir->dead = true; > if (ir->users) { > - dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n", > - ir->d.name, ir->d.minor); > + dev_dbg(&ir->dev, "releasing opened driver\n"); > wake_up_interruptible(&ir->buf->wait_poll); > } > > @@ -278,7 +272,7 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file) > > mutex_unlock(&ir->mutex); > > - dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor); > + dev_dbg(&ir->dev, "open called\n"); > > if (ir->d.rdev) { > retval = rc_open(ir->d.rdev); > @@ -332,8 +326,7 @@ unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait) > else > ret = POLLIN | POLLRDNORM; > > - dev_dbg(ir->d.dev, LOGHEAD "poll result = %d\n", > - ir->d.name, ir->d.minor, ret); > + dev_dbg(&ir->dev, "poll result = %d\n", ret); > > return ret; > } > @@ -345,12 +338,10 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > __u32 mode; > int result = 0; > > - dev_dbg(ir->d.dev, LOGHEAD "ioctl called (0x%x)\n", > - ir->d.name, ir->d.minor, cmd); > + dev_dbg(&ir->dev, "ioctl called (0x%x)\n", cmd); > > if (ir->dead) { > - dev_err(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n", > - ir->d.name, ir->d.minor); > + dev_err(&ir->dev, "ioctl result = -ENODEV\n"); > return -ENODEV; > } > > @@ -428,7 +419,7 @@ ssize_t lirc_dev_fop_read(struct file *file, > if (!LIRC_CAN_REC(ir->d.features)) > return -EINVAL; > > - dev_dbg(ir->d.dev, LOGHEAD "read called\n", ir->d.name, ir->d.minor); > + dev_dbg(&ir->dev, "read called\n"); > > buf = kzalloc(ir->chunk_size, GFP_KERNEL); > if (!buf) > diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c > index ffb70dee4547..131d87a04aab 100644 > --- a/drivers/staging/media/lirc/lirc_zilog.c > +++ b/drivers/staging/media/lirc/lirc_zilog.c > @@ -1377,7 +1377,6 @@ static const struct file_operations lirc_fops = { > }; > > static struct lirc_driver lirc_template = { > - .name = "lirc_zilog", > .code_length = 13, > .buffer_size = BUFLEN / 2, > .chunk_size = 2, > diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h > index f7629ff116a9..11f455a34090 100644 > --- a/include/media/lirc_dev.h > +++ b/include/media/lirc_dev.h > @@ -120,8 +120,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf, > /** > * struct lirc_driver - Defines the parameters on a LIRC driver > * > - * @name: this string will be used for logs > - * > * @minor: indicates minor device (/dev/lirc) number for > * registered driver if caller fills it with negative > * value, then the first free minor number will be used > @@ -167,7 +165,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf, > * @lirc_internal: lirc_dev bookkeeping data, don't touch. > */ > struct lirc_driver { > - char name[40]; > int minor; > __u32 code_length; > unsigned int buffer_size; /* in chunks holding one code each */
On Tue, May 02, 2017 at 06:04:18PM +0100, Sean Young wrote: >On Mon, May 01, 2017 at 06:04:52PM +0200, David Härdeman wrote: >> The name is only used for a few debug messages and the name of the parent >> device as well as the name of the lirc device (e.g. "lirc0") are sufficient >> anyway. >>... >> @@ -207,8 +204,7 @@ lirc_register_driver(struct lirc_driver *d) >> if (err) >> goto out_cdev; >> >> - dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n", >> - ir->d.name, ir->d.minor); >> + dev_info(ir->d.dev, "lirc device registered as lirc%d\n", minor); > >I'm not so sure this is a good idea. First of all, the documentation says >you can use "dmesg |grep lirc_dev" to find your lirc devices and you've >just replaced lirc_dev with lirc. Sure, no strong preferences here, you could change the line to say "lirc_dev device...", or drop the patch. >https://linuxtv.org/downloads/v4l-dvb-apis/uapi/rc/lirc-dev-intro.html > >It's useful having the driver name in the message. For example, I have >two rc devices connected usually: > >[sean@bigcore ~]$ dmesg | grep lirc_dev >[ 5.938284] lirc_dev: IR Remote Control driver registered, major 239 >[ 5.945324] rc rc0: lirc_dev: driver ir-lirc-codec (winbond-cir) registered at minor = 0 >[ 5111.830118] rc rc1: lirc_dev: driver ir-lirc-codec (mceusb) registered at minor = 1 winbond-cir....good man :) How about "dmesg | grep lirc -A2 -B2"? I don't think the situation is that different from how you'd know which input dev is allocated to any given rc_dev? With this patch applied the relevant output will be: [ 0.393494] rc rc0: rc-core loopback device as /devices/virtual/rc/rc0 [ 0.394458] input: rc-core loopback device as /devices/virtual/rc/rc0/input2 [ 0.395717] rc rc0: lirc device registered as lirc0 [ 12.612313] rc rc1: mceusb device as /devices/virtual/rc/rc1 [ 12.612768] input: mceusb device as /devices/virtual/rc/rc1/input4 [ 12.613112] rc rc1: lirc device registered as lirc1 (and we might want to change the lirc line to include the sysfs path?) But realistically, how much dmesg grepping are we expecting normal end-users to be doing? Anyway, as I said, this patch isn't crucial, and we can revisit printk's later (I'm looking at the ioctl locking right now and I think an ir-lirc-codec and lirc_dev merger might be a good idea once the fate of lirc_zilog has been decided). >With the driver name I know which one is which. > >Maybe lirc_driver.name should be a "const char*" so no strcpy is needed >(the ir-lirc-codec does not seem necessary). Not that it really pertains to whether d->name should be kept or not, but I think that lirc_dev shouldn't copy the lirc_driver struct into an irctl struct internal copy at all, but just keep a normal pointer. I haven't gotten around to vetting the (ab)use of the lirc_driver struct yet though. Regards, David
diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c index 2c1221a61ea1..74ce27f92901 100644 --- a/drivers/media/rc/ir-lirc-codec.c +++ b/drivers/media/rc/ir-lirc-codec.c @@ -380,8 +380,6 @@ static int ir_lirc_register(struct rc_dev *dev) if (dev->max_timeout) features |= LIRC_CAN_SET_REC_TIMEOUT; - snprintf(drv->name, sizeof(drv->name), "ir-lirc-codec (%s)", - dev->driver_name); drv->features = features; drv->data = &dev->raw->lirc; drv->rbuf = NULL; diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c index 4ba6c7e2d41b..10783ef75a25 100644 --- a/drivers/media/rc/lirc_dev.c +++ b/drivers/media/rc/lirc_dev.c @@ -28,8 +28,6 @@ #include <media/lirc.h> #include <media/lirc_dev.h> -#define LOGHEAD "lirc_dev (%s[%d]): " - static dev_t lirc_base_dev; /** @@ -160,7 +158,6 @@ lirc_register_driver(struct lirc_driver *d) return -EBADRQC; } - d->name[sizeof(d->name)-1] = '\0'; if (d->features == 0) d->features = LIRC_CAN_REC_LIRCCODE; @@ -207,8 +204,7 @@ lirc_register_driver(struct lirc_driver *d) if (err) goto out_cdev; - dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n", - ir->d.name, ir->d.minor); + dev_info(ir->d.dev, "lirc device registered as lirc%d\n", minor); d->lirc_internal = ir; return 0; @@ -242,13 +238,11 @@ lirc_unregister_driver(struct lirc_driver *d) mutex_lock(&ir->mutex); - dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n", - ir->d.name, ir->d.minor); + dev_dbg(&ir->dev, "unregistered\n"); ir->dead = true; if (ir->users) { - dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n", - ir->d.name, ir->d.minor); + dev_dbg(&ir->dev, "releasing opened driver\n"); wake_up_interruptible(&ir->buf->wait_poll); } @@ -278,7 +272,7 @@ int lirc_dev_fop_open(struct inode *inode, struct file *file) mutex_unlock(&ir->mutex); - dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor); + dev_dbg(&ir->dev, "open called\n"); if (ir->d.rdev) { retval = rc_open(ir->d.rdev); @@ -332,8 +326,7 @@ unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait) else ret = POLLIN | POLLRDNORM; - dev_dbg(ir->d.dev, LOGHEAD "poll result = %d\n", - ir->d.name, ir->d.minor, ret); + dev_dbg(&ir->dev, "poll result = %d\n", ret); return ret; } @@ -345,12 +338,10 @@ long lirc_dev_fop_ioctl(struct file *file, unsigned int cmd, unsigned long arg) __u32 mode; int result = 0; - dev_dbg(ir->d.dev, LOGHEAD "ioctl called (0x%x)\n", - ir->d.name, ir->d.minor, cmd); + dev_dbg(&ir->dev, "ioctl called (0x%x)\n", cmd); if (ir->dead) { - dev_err(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n", - ir->d.name, ir->d.minor); + dev_err(&ir->dev, "ioctl result = -ENODEV\n"); return -ENODEV; } @@ -428,7 +419,7 @@ ssize_t lirc_dev_fop_read(struct file *file, if (!LIRC_CAN_REC(ir->d.features)) return -EINVAL; - dev_dbg(ir->d.dev, LOGHEAD "read called\n", ir->d.name, ir->d.minor); + dev_dbg(&ir->dev, "read called\n"); buf = kzalloc(ir->chunk_size, GFP_KERNEL); if (!buf) diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c index ffb70dee4547..131d87a04aab 100644 --- a/drivers/staging/media/lirc/lirc_zilog.c +++ b/drivers/staging/media/lirc/lirc_zilog.c @@ -1377,7 +1377,6 @@ static const struct file_operations lirc_fops = { }; static struct lirc_driver lirc_template = { - .name = "lirc_zilog", .code_length = 13, .buffer_size = BUFLEN / 2, .chunk_size = 2, diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h index f7629ff116a9..11f455a34090 100644 --- a/include/media/lirc_dev.h +++ b/include/media/lirc_dev.h @@ -120,8 +120,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf, /** * struct lirc_driver - Defines the parameters on a LIRC driver * - * @name: this string will be used for logs - * * @minor: indicates minor device (/dev/lirc) number for * registered driver if caller fills it with negative * value, then the first free minor number will be used @@ -167,7 +165,6 @@ static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf, * @lirc_internal: lirc_dev bookkeeping data, don't touch. */ struct lirc_driver { - char name[40]; int minor; __u32 code_length; unsigned int buffer_size; /* in chunks holding one code each */
The name is only used for a few debug messages and the name of the parent device as well as the name of the lirc device (e.g. "lirc0") are sufficient anyway. Signed-off-by: David Härdeman <david@hardeman.nu> --- drivers/media/rc/ir-lirc-codec.c | 2 -- drivers/media/rc/lirc_dev.c | 25 ++++++++----------------- drivers/staging/media/lirc/lirc_zilog.c | 1 - include/media/lirc_dev.h | 3 --- 4 files changed, 8 insertions(+), 23 deletions(-)