linux-kernel - Re: [PATCH] hid Logitech G13 Driver 0.0.5

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:	Tue, 2 Mar 2010 16:46:45 -0700
From:	"Rick L. Vinyard, Jr." <rvinyard@...nmsu.edu>
To:	"Jaya Kumar" <jayakumar.lkml@...il.com>
Cc:	linux-kernel@...r.kernel.org, felipe.balbi@...ia.com, pavel@....cz,
	linux-usb@...r.kernel.org, oliver@...kum.org,
	linux-input@...r.kernel.org, dmitry.torokhov@...il.com,
	npavel@...ner.com, tomi.valkeinen@...ia.com, tony@...mide.com,
	florianschandinat@....de, krzysztof.h1@...pl,
	akpm@...ux-foundation.org, linux-fbdev@...r.kernel.org,
	jkosina@...e.cz, bonbons@...ux-vserver.org
Subject: Re: [PATCH] hid Logitech G13 Driver 0.0.5


Jaya Kumar wrote:
> On Wed, Mar 3, 2010 at 2:48 AM, Rick L. Vinyard Jr.
> <rvinyard@...nmsu.edu> wrote:
>> [PATCH] hid Logitech G13 Driver 0.0.5
>
> Hi Rick,
>
> Thanks for posting this. It has improved a lot.
>
>>
>> This driver relies on two patches:
>>  1. Bruno Prémont's [PATCH 2/3] hid: add suspend/resume hooks for hid
>> drivers
>>  2. [PATCH] Add sysfs support for fbdefio delay
>
> Please help me understand in what way does this patch have to have the
> global userspace exposure of defio delay in sysfs support. If that
> sysfs patch was not accepted, I don't see this patch affected
> negatively.
>

It's in the deferred I/O struct definition with members delay_min and
delay_max.

static struct fb_deferred_io g13_fb_defio = {
	.delay = HZ / G13FB_UPDATE_RATE_DEFAULT,
	.delay_min = HZ / 20,
	.delay_max = HZ * 3,
	.deferred_io = g13_fb_deferred_io,
};


>> However, the driver does use the HID feature reports extensively to
>> control the backlight, LEDs, et. al.
>
> Ok.
>
>> - Removed the update rate sysfs attribute as it is in the second patch
>> above.
>
> Why? There is no consensus that exposing defio delay for _all_ fb
> drivers is a good idea. I would rather you left that in here for now.
>

If the consensus is that defio delay will be exposed on a driver by driver
basis I can paste that back in here and modify the userspace g13 library
accordingly.

>> +/*
>> + * This is a default logo to be loaded upon driver initialization
>> + * replacing the default Logitech G13 image loaded on device
>> + * initialization. This is to provide the user a cue that the
>> + * Linux driver is loaded and ready.
>> + *
>> + * This logo contains the text G13 in the center with two penguins
>> + * on each side of the text. The penguins are a 33x40 rendition of
>> + * the default framebuffer 80x80 monochrome image scaled down and
>> + * cleaned up to retain something that looks a little better than
>> + * a simple scaling.
>> + *
>> + * This logo is a simple xbm image created in GIMP and exported.
>> + * To view the image copy the following two #defines plus the
>> + * g13_lcd_bits to an ASCII text file and save with extension
>> + * .xbm, then open with GIMP or any other graphical editor
>> + * such as eog that recognizes the .xbm format.
>> + */
> ...
>> +static unsigned char g13_lcd_bits[] = {
>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> 0x00,
>
> I think you'll agree above is not an elegant solution.
>

I could make it a config option.

>> +
>> +static DEVICE_ATTR(rgb, 0666, g13_rgb_show, g13_rgb_store);
>
> Reading above code, it looks like this new userspace sysfs attribute
> is for backlight control. Could that be better exposed using the
> existing backlight class?
>

I looked at the backlight class and it didn't seem to be a very good fit.

/* This structure defines all the properties of a backlight */
struct backlight_properties {
	/* Current User requested brightness (0 - max_brightness) */
	int brightness;
	/* Maximal value for brightness (read-only) */
	int max_brightness;
	/* Current FB Power mode (0: full on, 1..3: power saving
	   modes; 4: full off), see FB_BLANK_XXX */
	int power;
	/* FB Blanking active? (values as for power) */
	/* Due to be removed, please use (state & BL_CORE_FBBLANK) */
	int fb_blank;
	/* Flags used to signal drivers of state changes */
	/* Upper 4 bits are reserved for driver internal use */
	unsigned int state;
};

The g13's backlight doesn't support any of these properties. All you can
set is an rgb value on the g13; no power modes, no brightness, etc. I
could add a brightness attribute by scaling the rgb values, but I would
prefer to do that from userspace rather than the driver since it's not
supported in the device itself.

And, we'd still have to have the rgb attribute since the backlight class
doesn't have color.

-- 

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/