| 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
| ||
|
Message-ID: <1391039165.2422.11.camel@joe-AO722>
Date: Wed, 29 Jan 2014 15:46:05 -0800
From: Joe Perches <joe@...ches.com>
To: David Barksdale <dbarksdale@...ogix.com>
Cc: David Herrmann <dh.herrmann@...il.com>,
Jiri Kosina <jkosina@...e.cz>,
Benjamin Tissoires <benjamin.tissoires@...hat.com>,
Oliver Neukum <oliver@...kum.org>,
Jakub Kákona <kjakub@...il.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] HID: New hid-cp2112 driver
On Wed, 2014-01-29 at 17:26 -0600, David Barksdale wrote:
> This patch adds support for the Silicon Labs CP2112
> "Single-Chip HID USB to SMBus Master Bridge."
Just some trivial notes:
> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
[]
> +struct __attribute__ ((__packed__)) cp2112_smbus_config_report {
Please use
struct name {
...
} __packed;
This allows a simpler grep pattern to find the
struct definition. There are many of these.
[]
> +static int cp2112_hid_get(struct hid_device *hdev, unsigned char report_number,
> + u8 *data, size_t count, unsigned char report_type)
> +{
> + u8 *buf;
> + int ret;
> +
> + buf = kmalloc(count, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = hdev->hid_get_raw_report(hdev, report_number, buf, count,
> + report_type);
> + memcpy(data, buf, count);
> + kfree(buf);
> + return ret;
if the data is going to be copied in data,
why not just use data in hid_get_raw_report
and avoid the malloc?
> +static int cp2112_hid_output(struct hid_device *hdev, u8 *data, size_t count,
> + unsigned char report_type)
> +{
> + u8 *buf;
> + int ret;
> +
> + buf = kmemdup(data, count, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = hdev->hid_output_raw_report(hdev, buf, count, report_type);
> + kfree(buf);
> + return ret;
same question.
> +static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
> + unsigned short flags, char read_write, u8 command,
> + int size, union i2c_smbus_data *data)
> +{
> + struct cp2112_device *dev = (struct cp2112_device *)adap->algo_data;
> + struct hid_device *hdev = dev->hdev;
> + u8 buf[64];
> + __be16 word;
> + size_t count;
> + size_t read_length = 0;
> + size_t timeout;
size_t is an odd type for a variable named timeout
[]
> + for (timeout = 0; timeout < XFER_TIMEOUT; ++timeout) {
> + ret = cp2112_xfer_status(dev);
> + if (-EBUSY == ret)
> + continue;
> + if (ret < 0)
> + goto power_normal;
> + break;
> + }
and it looks more like an maximum attempt count
than a timeout.
> +
> + if (XFER_TIMEOUT <= timeout) {
> + hid_warn(hdev, "Transfer timed out, cancelling.\n");
> + buf[0] = CP2112_CANCEL_TRANSFER;
> + buf[1] = 0x01;
[]
> +#define CP2112_CONFIG_ATTR(name, store, format, ...) \
> +static ssize_t name##_store(struct device *kdev, \
> + struct device_attribute *attr, const char *buf, \
> + size_t count) \
> +{ \
> + struct hid_device *hdev = container_of(kdev, struct hid_device, dev); \
> + struct cp2112_usb_config_report cfg; \
> + int ret = cp2112_get_usb_config(hdev, &cfg); \
> + if (ret) \
> + return ret; \
> + store; \
> + ret = cp2112_set_usb_config(hdev, &cfg); \
> + if (ret) \
> + return ret; \
> + chmod_sysfs_attrs(hdev); \
> + return count; \
> +} \
> +static ssize_t name##_show(struct device *kdev, \
> + struct device_attribute *attr, char *buf) \
> +{ \
> + struct hid_device *hdev = container_of(kdev, struct hid_device, dev); \
> + struct cp2112_usb_config_report cfg; \
> + int ret = cp2112_get_usb_config(hdev, &cfg); \
> + if (ret) \
> + return ret; \
> + return scnprintf(buf, PAGE_SIZE, format, __VA_ARGS__); \
##__VA_ARGS__ would probably be better
> +} \
> +DEVICE_ATTR_RW(name);
> +
> +CP2112_CONFIG_ATTR(vendor_id, ({
> + u16 vid;
> +
> + if (sscanf(buf, "%hi", &vid) != 1)
> + return -EINVAL;
> +
> + cfg.vid = cpu_to_le16(vid);
> + cfg.mask = 0x01;
> +}), "0x%04x\n", le16_to_cpu(cfg.vid));
> +
> +CP2112_CONFIG_ATTR(product_id, ({
> + u16 pid;
> +
> + if (sscanf(buf, "%hi", &pid) != 1)
> + return -EINVAL;
> +
> + cfg.pid = cpu_to_le16(pid);
> + cfg.mask = 0x02;
> +}), "0x%04x\n", le16_to_cpu(cfg.pid));
These uses of CP2112_CONFIG_ATTR are kind of ugly.
> +
> +CP2112_CONFIG_ATTR(max_power, ({
> + int mA;
> +
> + if (sscanf(buf, "%i", &mA) != 1)
> + return -EINVAL;
> +
> + cfg.max_power = (mA + 1) / 2;
> + cfg.mask = 0x04;
> +}), "%u mA\n", cfg.max_power * 2);
> +
> +CP2112_CONFIG_ATTR(power_mode, ({
> + if (sscanf(buf, "%hhi", &cfg.power_mode) != 1)
> + return -EINVAL;
> +
> + cfg.mask = 0x08;
> +}), "%u\n", cfg.power_mode);
> +
> +CP2112_CONFIG_ATTR(release_version, ({
> + if (sscanf(buf, "%hhi.%hhi", &cfg.release_major, &cfg.release_minor)
> + != 2)
> + return -EINVAL;
> +
> + cfg.mask = 0x10;
> +}), "%u.%u\n", cfg.release_major, cfg.release_minor);
[]
> +static int cp2112_raw_event(struct hid_device *hdev, struct hid_report *report,
> + u8 *data, int size)
> +{
[]
> + case STATUS0_ERROR:
> + switch (xfer->status1) {
> + case STATUS1_TIMEOUT_NACK:
> + case STATUS1_TIMEOUT_BUS:
> + dev->xfer_status = -ETIMEDOUT;
> + break;
> + default:
> + dev->xfer_status = -EIO;
nicer with a break
--
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/
Powered by blists - more mailing lists