Why does this need to be a kernel driver at all? #29
Comments
|
You can mess around with some nasty libusb scripts but you need to unbind hid-generic first then run some scripts then rebind hid-generic otherwise you wont be able to type things (at least that was the case on Ubuntu when I tried that). Yeah I tried to avoid kernel drivers believe me. I actually think the kernel driver is probably the cleanest way to do what were trying to achieve.
Feel free but don't make a massive pull requests with near every line changed 😉 as that's much more annoying to review. |
|
Ok, if you need to use hid-generic for the non-led stuff, it makes sense for it to be a kernel module. I would send a pull request that changes all lines, as your coding style is not the same as the kernel's coding style, and there's not much I am willing to do until that happens. You also need that changed in order for you to get the code merged upstream, which I am glad to help out with. So do you want to make those changes first? |
|
@terrycain @gregkh As already pointed out in another issue:
I worked on a complete cleanup for 3 weeks and I would love to merge our efforts. Edit: There was a lot of duplicate stuff in the old source. I cleaned it up, reviewed everything, added mutexes and updated the USB communication code. The old code did not wait for a Razer device response and didn't validate it for success. Furthermore all errors are handled now and log messages are send. The ABI was cleaned up and various small fixes.. I think, that this rewrite is a clean base to get things running 😃 The missing support for some of the current supported Razer devices isn't a big problem. It's only a matter of a few days to add them. I would love to work on that... Edit 2: Right now the source is based on Tim Theede's old razer_chroma_drivers project. But I would compare your (@terrycain) changes and merge them... |
|
@gregkh go for it @m4ng0squ4sh Whilst I'd be happy for your efforts to be to be reintegrated into the repo the issue I have with rewriting everything is that at its current state the driver has been tested thoroughly by all the owners of the devices it supports. By replacing everything it runs a high risk of breaking device support. Not that I think that would happen but if it does it will be a mission to diagnose. |
|
@terrycain The driver is a complete rewrite and I would not suggest to merge it:
I'll handle this and carefully add the missing support. It's really not a lot of work, because the current OpenRazer driver supports already most of the modes and it is enough to add some simple if-else statements for some specific devices. Finally we should open a list on Github and request all users to test their devices against the new driver. I have 3 Razer devices, you surely have also some and there are a lot of willing Github users who will support us verifying if everything works. That's not a problem at all. I spend a lot of work to simplify the code and fixed some nasty bugs: your current driver source was not tested with the Razer Blade Stealth, although you are listing it as fully supported. It was not possible to set custom keys colors... An overall device support verification of the driver is required anyway. Please compare: OpenRazer// Set the key colors for the complete keyboard. Takes in an array of RGB bytes.
int razer_set_key_colors(struct razer_device *razer_dev,
unsigned char *row_cols, size_t row_cols_len)
{
int i, retval;
int rows = razer_get_rows(razer_dev->usb_dev);
int columns = razer_get_columns(razer_dev->usb_dev);
size_t row_cols_required_len = columns * 3 * rows;
if (columns < 0 || rows < 0 || row_cols_required_len < 0) {
pr_warn("set_key_colors: unsupported device\n");
return -EINVAL;
}
// Validate the input.
if (row_cols_len != row_cols_required_len) {
pr_warn("set_key_colors: wrong amount of RGB data provided: "
"%lu of %lu\n", row_cols_len, row_cols_required_len);
return -EINVAL;
}
for (i = 0; i < rows; i++) {
retval =
razer_set_key_row(razer_dev, i,
(unsigned char *)&row_cols[i * columns * 3],
columns * 3);
if (retval != 0) {
pr_warn("set_key_colors: failed to set colors for row: "
"%d\n", i);
return retval;
}
}
return 0;
}razer-drivers// Writes the colour rows on the keyboard. Takes in all the colours for the keyboard
static ssize_t razer_attr_write_set_key_row(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
{
size_t offset = 0;
unsigned char row_index;
struct usb_interface *intf = to_usb_interface(dev->parent);
struct usb_device *usb_dev = interface_to_usbdev(intf);
size_t buf_size = RAZER_BLACKWIDOW_CHROMA_ROW_LEN * 3 + 1;
while(offset < count) {
if((count-offset) < buf_size) {
printk(KERN_ALERT "Wrong Amount of RGB data provided: %d of %d\n",(int)(count-offset), (int)buf_size);
return -EINVAL;
}
row_index = (unsigned char)buf[offset];
razer_set_key_row(usb_dev, row_index, (unsigned char*)&buf[offset + 1]);
offset += buf_size;
}
return count;
}Analysis1. Invalid keyboard row handling for all keyboards except BlackWidow Chromasize_t buf_size = RAZER_BLACKWIDOW_CHROMA_ROW_LEN * 3 + 1;A constant fixed length RAZER_BLACKWIDOW_CHROMA_ROW_LEN is used, but this method is used by more keyboards, than the BlackWidow Chroma, resulting in a malfunctioning mode. 2. While loop with a lot of unnecessary calculationCompare: while(offset < count) {
if((count-offset) < buf_size) {
/*....*/ (int)(count-offset), (int)buf_size);
return -EINVAL;
}
row_index = (unsigned char)buf[offset];
/*...*/ &buf[offset + 1]);
offset += buf_size;
}with a simple for loop: for (i = 0; i < rows; i++) {3. Upper bound is not checked!while(offset < count) {
if((count-offset) < buf_size) {
/*...*/The user is able to provide more data than required to the driver. The driver does not throw an error! 4. No error check! Not handling the return value!razer_set_key_row(usb_dev, row_index, (unsigned char*)&buf[offset + 1]);Return values are almost never checked in your source code, which results into an invalid success return, even if the request failed... ConclusionI invested a lot of time to restructure, rethink and rewrite everything to fix all those small problems. This was only one small function which was analysed. I do really think, that it is the best to continue working on the cleaned up source 😃 The hard work is already done and I'll manage the last transition. @terrycain you don't have to worry about the kernel driver. I'll take care about the last device support, the testing and the verification. You are welcome to join the OpenRazer organization, I'll grant you admin rights and you can start right away working on your daemon. |
|
Ok, you all work out what kernel driver I should be looking at for proper kernel inclusion, I'm not going to get involved in arguing about which is "better" as really, both of these examples need fixing :) |
|
@gregkh We'll get that sorted out first.. Could you also please leave short hints about what's wrong with the OpenRazer code? Would love to get some feedback. Thanks. |
|
Point me at the OpenRazer code and I'll be glad to review it, but for a quick glance above I see:
|
|
@gregkh Thanks! The module source code was checked with the linux/checkpatch.pl script and I fixed everything, except some minor warnings, which I though are better readable: Quote from Documentation/SubmittingPatches:
Summary of output of linux/checkpatch.pl: The full source code can be viewed here.
EDIT: the checkpatch.pl script suggested to use the pr_* calls. This way, it automatically adds the module name razer: in front of the log message, by adding this to the top of the file: #define pr_fmt(fmt) KBUILD_MODNAME ": " fmtBelow is a complete code quote with the missing parts from the previous quote. Would be glad, if you could mention mistakes, invalid code conventions,... // Returns the row count of the keyboard.
// On error a value smaller than 0 is returned.
int razer_get_rows(struct usb_device *usb_dev)
{
switch (usb_dev->descriptor.idProduct) {
case USB_DEVICE_ID_RAZER_BLADE_STEALTH_2016:
return RAZER_STEALTH_2016_ROWS;
case USB_DEVICE_ID_RAZER_BLADE_14_2016:
return RAZER_BLADE_14_2016_ROWS;
case USB_DEVICE_ID_RAZER_BLACKWIDOW_CHROMA:
return RAZER_BLACKWIDOW_CHROMA_ROWS;
default:
return -EINVAL;
}
}
// Returns the column count of the keyboard.
// On error a value smaller than 0 is returned.
int razer_get_columns(struct usb_device *usb_dev)
{
switch (usb_dev->descriptor.idProduct) {
case USB_DEVICE_ID_RAZER_BLADE_STEALTH_2016:
return RAZER_STEALTH_2016_COLUMNS;
case USB_DEVICE_ID_RAZER_BLADE_14_2016:
return RAZER_BLADE_14_2016_COLUMNS;
case USB_DEVICE_ID_RAZER_BLACKWIDOW_CHROMA:
return RAZER_BLACKWIDOW_CHROMA_COLUMNS;
default:
return -EINVAL;
}
}
// Set the key colors for a specific row. Takes in an array of RGB bytes.
int razer_set_key_row(struct razer_device *razer_dev, unsigned char row_index,
unsigned char *row_cols, size_t row_cols_len)
{
int retval;
int rows = razer_get_rows(razer_dev->usb_dev);
int columns = razer_get_columns(razer_dev->usb_dev);
size_t row_cols_required_len = columns * 3;
struct razer_report report = razer_new_report();
if (rows < 0 || columns < 0) {
pr_warn("set_key_row: unsupported device\n");
return -EINVAL;
}
// Validate the input.
if (row_index >= rows) {
pr_warn("set_key_row: invalid row index: %d\n", row_index);
return -EINVAL;
}
if (row_cols_len != row_cols_required_len) {
pr_warn("set_key_row: wrong amount of RGB data provided: "
"%lu of %lu\n", row_cols_len, row_cols_required_len);
return -EINVAL;
}
report.command_class = 0x03;
report.command_id = 0x0B;
report.data_size = row_cols_required_len + 4;
report.transaction_id = 0x80; // Set a custom transaction ID.
report.arguments[0] = 0xFF; // Frame ID
report.arguments[1] = row_index; // Row
report.arguments[2] = 0x00; // Start Index
report.arguments[3] = columns - 1; // End Index (Calc to end of row)
memcpy(&report.arguments[4], row_cols, row_cols_required_len);
report.crc = razer_calculate_crc(&report);
retval = razer_send_check_response(razer_dev, &report);
if (retval != 0) {
razer_print_err_report(&report, KBUILD_MODNAME,
"set_key_row: request failed");
return retval;
}
return retval;
}
// Set the key colors for the complete keyboard. Takes in an array of RGB bytes.
int razer_set_key_colors(struct razer_device *razer_dev,
unsigned char *row_cols, size_t row_cols_len)
{
int i, retval;
int rows = razer_get_rows(razer_dev->usb_dev);
int columns = razer_get_columns(razer_dev->usb_dev);
size_t row_cols_required_len = columns * 3 * rows;
if (columns < 0 || rows < 0 || row_cols_required_len < 0) {
pr_warn("set_key_colors: unsupported device\n");
return -EINVAL;
}
// Validate the input.
if (row_cols_len != row_cols_required_len) {
pr_warn("set_key_colors: wrong amount of RGB data provided: "
"%lu of %lu\n", row_cols_len, row_cols_required_len);
return -EINVAL;
}
for (i = 0; i < rows; i++) {
retval =
razer_set_key_row(razer_dev, i,
(unsigned char *)&row_cols[i * columns * 3],
columns * 3);
if (retval != 0) {
pr_warn("set_key_colors: failed to set colors for row: "
"%d\n", i);
return retval;
}
}
return 0;
}
/*
* Write device file "set_key_colors"
* Set the colors of all keys of the keyboard. 3 bytes per color.
*/
static ssize_t razer_attr_write_set_key_colors(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
struct razer_device *razer_dev = dev_get_drvdata(dev);
int retval;
retval = razer_set_key_colors(razer_dev,
(unsigned char *)&buf[0], count);
if (retval != 0)
return retval;
retval = razer_set_custom_mode(razer_dev);
if (retval != 0)
return retval;
return count;
} |
|
As you say, don't trust everything that checkpatch.pl tells you, sometimes it is wrong. You are a driver, always use dev__() instead of pr__() as that will give you the exact driver/device/instance that is causing the message to be created. Also, stop using // for comments :) I'll look at the full driver over at the other place now... |
|
😄 ok good to know. I'll change all the pr_() calls to dev_().
Haha, ok. I'll stick to the
Great! Please open an issue, if you found anything... |
|
As a reference, here are Greg's improvements. @terrycain any chance we can come to an arrangement? |
|
Yeah I know the return values arn't checked and yeah that set_led function was dodgy, remember I took the driver on from tim ;). Make a PR for the driver, have you changed how any of the driver files work as I don't want to change how the driver is interfaced with as that will definitely be too much hassle. Yeah how I interface with the driver be my only condition, oh and make sure you reference me and tim in the copyright so that the authors are accredited appropriately. |
|
And make a PR with support for all thats currently supported. No point in loosing device coverage |
In my previous comment I explained in detail why I am suggesting to move our efforts to a Github organization. Please explain, why that's a problem for you?
Yes, I changed it slightly, because once this is committed to the kernel, we can't change the ABI anymore. That's why we should think about the ABI design and plan it very careful.
Please answer my points from the previous comment. |
|
As for the user/kernel api, wait until I review it before you think anything is stable, I really don't think it's going to work as-is, but will wait for the other cleanups and changes to be done first before working on that... |
|
@gregkh Good point. Here is the current ABI Documentation. However I am still not happy with it. Planing to change some device attribute names. We should discuss this together... Right now ASCII and binary values are also mixed... Not sure how this is handled normally. |
|
The question I'd ask is why are you needing to unbind-rebind? Not arguing or challenging, but rather to get common ground with the project- it's likely that one of their mechanical keyboards is in my future. Right now, I don't need to do anything with my almost finished G15Interface project on GitHub that is a ground-up re-implementation of things for the G15 and eventually G19 series of Logitech devices. It's via libHID and, in theory, could even work as a driver lib for Windows and OSX- and works better for this functionality from the libusb version OR the kernel versions, validated so against an actual G15, G13, and G510s. One API. One Library. I would be providing it to the Gnome15 project if I could be certain of who's the actual maintainer when the prior project sites went bye-bye. It'd replace most of the drivers proper and be much more stable for G15 type devices. This isn't saying you guys should be able to do that, but I'm trying to relate why we would need this when the Steelseries stuff doesn't need a kernel driver, nor my stuff does. Is there anything that you can't get by hooking things via libhid for this stuff? |
|
@madscientist42 I thought about this yesterday after receiving an e-mail from the linux mailing list: Quote from Benjamin Tissoires:
I must admit, that he is absolutely right and that's the way to go. I'll work on a hidraw driver in userspace and stop the development of the kernel driver 😃 |
|
@m4ng0squ4sh we (as in all of us hacking on this sort of thing...) ought to look at if there's commonality of function amongst the keyboards sufficient to merit a similar thing to libratbag (Not precisely the same thing as merging with that project, mind... There may/may not be enough there for the keyboards and I don't want to just waltz in and take over...not the way to do things...) and come up with a similar abstraction. One of the things I had in mind when Gnome15 (and I'm still going to keep working with these keyboards, I just want a good mechanical, and I don't think Logitech's in my short-term future there...) all but disappeared was to come up with something not unlike libratbag, etc. for them. I just had a bunch of things tie me up over the last two years or it'd already happened. Gnome15's "nice" but there's too much reliance on Python for some of the core that needs re-work and I didn't have time to slog through a bunch of Python code when I wasn't getting paid for it. :D |
|
@madscientist42 I had a look at several projects using hidraw and I think that's the right way to go. A universal core library handling various devices would be great. It might even handle devices from different manufacturers... A clean base library written in C, C++ or Go would be awesome. If you're interested to join efforts, please feel free to contact me and we'll have a chat in detail about further steps... :) |
|
@gregkh I just picked up one of these keyboards if you're helping them out this is great news. I'd be glad to test out. I use Linus master tree for drm/amdgpu changes so adding this driver into my builds would be great. |
|
Was wondering where things are up to with this idea. I've just got myself a BlackWidow Chroma and I am having some fun with these drivers and Linux is great in that there are many versions of the same thing... Linux is rubbish when there are many versions of the same thing! ;-) It's both a blessing and a curse! I think the OpenRazer idea sounds great and it would be great if you could all get together to come up with something slick - instead of multiple things which don't quite work. I'm not a Linux kernel or desktop developer at all, but would like to see if I can help. (@m4ngosqu4sh, I'll email you separately). Thanks. I'll butt out of your thread now! :-D |
|
This can be closed. |
Am I missing something, but why does this need to be in the kernel at all?
Can't you do all of the setting of attributes and commands directly from userspace using libusb instead of having a kernel driver?
If not, please point out what I'm missing.
Also, if this really does need to be a kernel driver, I'll be glad to help out and clean things up and fix the obvious errors / race conditions that I see :)
The text was updated successfully, but these errors were encountered: