linux-kernel - Re: [PATCH] hid: Logitech G13 driver 0.0.3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 14 Jan 2010 11:24:17 -0700
From:	"Rick L. Vinyard, Jr." <rvinyard@...nmsu.edu>
To:	"Jiri Kosina" <jkosina@...e.cz>
Cc:	"Jaya Kumar" <jayakumar.lkml@...il.com>,
	linux-kernel@...r.kernel.org, felipe.balbi@...ia.com,
	"Pavel Machek" <pavel@....cz>, linux-fbdev@...r.kernel.org,
	krzysztof.h1@...pl, "Andrew Morton" <akpm@...ux-foundation.org>,
	linux-usb@...r.kernel.org, "Oliver Neukum" <oliver@...kum.org>,
	linux-input@...r.kernel.org,
	"Dmitry Torokhov" <dmitry.torokhov@...il.com>
Subject: Re: [PATCH] hid: Logitech G13 driver 0.0.3

Jiri Kosina wrote:
> On Thu, 7 Jan 2010, Rick L. Vinyard, Jr. wrote:
>
>> >> +static ssize_t g13_mled_store(struct device *dev,
>> >> +                             struct device_attribute *attr,
>> >> +        const char *buf, size_t count)
>> >> +{
>> >> +       struct hid_device *hdev;
>> >> +       int i;
>> >> +       unsigned m[4];
>> >> +       unsigned mled;
>> >> +       ssize_t set_result;
>> >> +
>> >> +       /* Get the hid associated with the device */
>> >> +       hdev = container_of(dev, struct hid_device, dev);
>> >> +
>> >> +       /* If we have an invalid pointer we'll return ENODATA */
>> >> +       if (hdev == NULL || &(hdev->dev) != dev)
>> >> +               return -ENODATA;
>> >> +
>> >> +       i = sscanf(buf, "%u %u %u %u", m, m+1, m+2, m+3);
>> >> +       if (!(i == 4 || i == 1)) {
>> >> +               printk(KERN_ERR "unrecognized input: %s", buf);
>> >> +               return -1;
>> >> +       }
>> >> +
>> >> +       if (i == 1)
>> >> +               mled = m[0];
>> >> +       else
>> >> +               mled = (m[0] ? 1 : 0) | (m[1] ? 2 : 0) |
>> >> +                               (m[2] ? 4 : 0) | (m[3] ? 8 : 0);
>> >> +
>> >> +       set_result = g13_set_mled(hdev, mled);
>> >> +
>> >> +       if (set_result < 0)
>> >> +               return set_result;
>> >> +
>> >> +       return count;
>> >> +}
>> >> +
>> >> +static DEVICE_ATTR(mled, 0666, g13_mled_show, g13_mled_store);
>> >
>> > Have you considered the use of the LED class driver as an alternative
>> > to introducing these sysfs led controls for the device?
>> >
>>
>> I did, but this seemed a simpler approach to let user space (such as a
>> daemon) manage the leds. In particular this could be used by a user
>> space
>> program to map the keys. The MR led could be used to indicate an active
>> record mode, etc.
>
> I finally had some time to go through the driver as well (thanks Dmitry
> for beating me with proper review).
>
> Could you be more specific about reasons why you are hesitating to use LED
> subsystem instead of the whole mled stuff in the driver?
>
> I don't see anything that couldn't be achieved using LED class.
>

It seems to overly complicate the driver, especially as these are LEDS
inside the buttons. I don't think they will leverage the bulk of the LED
structure such as triggers, brightness, et. al.

It also seems to complicate the userspace access. A single
sysfs_get_device_attr() is replaced with looking up a unique device
identifier, creating a unique led device path, an additional
sysfs_open_device_path() and then the sysfs_get_device_attr().

But, I've added the LED class support.

However, the question I have now is that each device as it's registered
will have to be assigned a unique identifier so that each led can be
uniquely named for each device, otherwise there will be conflicts in
/sys/class/leds/.

Since the led naming scheme is "device:color:function" I was thinking of
using the minor number of the hid device and name the leds
"g13_minor#:red:m1" et. al.

But, I'd really prefer to use something that userspace can access from
sysfs to build the led device path. That way, given a udev device and a
numeric led value you could do something like:

  struct sysfs_device* leddev;
  struct sysfs_attribute* bright_attr;
  char* ledsyspath;
  char ledvalstr[4] = {0};
  ledsyspath = malloc(strlen(udev_device_get_syspath(udev_device)+22);

  *** Here you need to get the minor hid # ***

  sprintf(ledsyspath, "%s/leds/g13_%d:red:m1",
          udev_device_get_syspath(udev_device), minor);
  leddev = sysfs_open_device_path(ledsyspath);
  bright_attr = sysfs_get_device_attr(leddev, "brightness");
  sprintf(ledvalstr,"%d",ledval);
  sysfs_write_attribute(sysfsattr, ledvalstr, 4);

Any suggestions other than the minor number?

Thanks,

---

Rick


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/