| 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: <20131226225317.GD18562@core.coreip.homeip.net>
Date: Thu, 26 Dec 2013 14:53:17 -0800
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Andrey Smirnov <andrew.smirnov@...il.com>
Cc: linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ims-pcu: Add commands supported by the new version of
the FW
Hi Andrey,
On Sat, Dec 21, 2013 at 11:16:48AM -0800, Andrey Smirnov wrote:
> New version of the PCU firmware supports two new commands:
> - IMS_PCU_CMD_OFN_SET_CONFIG which allows to write data to the
> registers of one finger navigation(OFN) chip present on the device
> - IMS_PCU_CMD_OFN_GET_CONFIG which allows to read data form the
> registers of said chip.
>
> This commit adds two helper functions to use those commands and sysfs
> attributes to use them. It also exposes some OFN configuration
> parameters via sysfs.
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@...il.com>
> ---
> drivers/input/misc/ims-pcu.c | 274 +++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 263 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index e204f26..050c960 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -68,6 +68,9 @@ struct ims_pcu {
> char bl_version[IMS_PCU_BL_VERSION_LEN];
> char reset_reason[IMS_PCU_BL_RESET_REASON_LEN];
> int update_firmware_status;
> + u8 device_id;
> +
> + u8 ofn_reg_addr;
>
> struct usb_interface *ctrl_intf;
>
> @@ -371,6 +374,8 @@ static void ims_pcu_destroy_gamepad(struct ims_pcu *pcu)
> #define IMS_PCU_CMD_GET_DEVICE_ID 0xae
> #define IMS_PCU_CMD_SPECIAL_INFO 0xb0
> #define IMS_PCU_CMD_BOOTLOADER 0xb1 /* Pass data to bootloader */
> +#define IMS_PCU_CMD_OFN_SET_CONFIG 0xb3
> +#define IMS_PCU_CMD_OFN_GET_CONFIG 0xb4
>
> /* PCU responses */
> #define IMS_PCU_RSP_STATUS 0xc0
> @@ -389,6 +394,9 @@ static void ims_pcu_destroy_gamepad(struct ims_pcu *pcu)
> #define IMS_PCU_RSP_GET_DEVICE_ID 0xce
> #define IMS_PCU_RSP_SPECIAL_INFO 0xd0
> #define IMS_PCU_RSP_BOOTLOADER 0xd1 /* Bootloader response */
> +#define IMS_PCU_RSP_OFN_SET_CONFIG 0xd2
> +#define IMS_PCU_RSP_OFN_GET_CONFIG 0xd3
> +
>
> #define IMS_PCU_RSP_EVNT_BUTTONS 0xe0 /* Unsolicited, button state */
> #define IMS_PCU_GAMEPAD_MASK 0x0001ff80UL /* Bits 7 through 16 */
> @@ -1216,6 +1224,226 @@ ims_pcu_update_firmware_status_show(struct device *dev,
> static DEVICE_ATTR(update_firmware_status, S_IRUGO,
> ims_pcu_update_firmware_status_show, NULL);
>
> +enum pcu_ofn_offsets {
> + OFN_REG_RESULT_LSB_OFFSET = 2,
> + OFN_REG_RESULT_MSB_OFFSET = 3,
> +};
> +
> +static ssize_t ims_pcu_ofn_reg_data_show(struct device *dev,
> + struct device_attribute *dattr,
> + char *buf)
> +{
> + struct usb_interface *intf = to_usb_interface(dev);
> + struct ims_pcu *pcu = usb_get_intfdata(intf);
> + int error;
> +
> + mutex_lock(&pcu->cmd_mutex);
> +
> + error = ims_pcu_execute_command(pcu, OFN_GET_CONFIG,
> + &pcu->ofn_reg_addr,
> + sizeof(pcu->ofn_reg_addr));
> + if (error >= 0) {
> + const s16 result = pcu->cmd_buf[OFN_REG_RESULT_MSB_OFFSET] << 8 |
> + pcu->cmd_buf[OFN_REG_RESULT_LSB_OFFSET];
const here seems overkill.
> + if (result < 0)
> + error = result;
> + else
> + error = scnprintf(buf, PAGE_SIZE, "%x\n", (u8)result);
Why cast to u8?
> + }
> +
> + mutex_unlock(&pcu->cmd_mutex);
> +
> + return error;
> +}
> +
> +static ssize_t ims_pcu_ofn_reg_data_store(struct device *dev,
> + struct device_attribute *dattr,
> + const char *buf, size_t count)
> +{
> + struct usb_interface *intf = to_usb_interface(dev);
> + struct ims_pcu *pcu = usb_get_intfdata(intf);
> + int error;
> + int value;
> + u8 buffer[2];
> +
> + error = kstrtoint(buf, 0, &value);
> + if (error)
> + return error;
> +
> + buffer[0] = pcu->ofn_reg_addr;
> + buffer[1] = (u8) value;
If you want u8 we have kstrtou8().
> +
> + mutex_lock(&pcu->cmd_mutex);
> +
> + error = ims_pcu_execute_command(pcu, OFN_SET_CONFIG,
> + &buffer, sizeof(buffer));
> +
> + mutex_unlock(&pcu->cmd_mutex);
> +
> + if (!error) {
> + const s16 result = pcu->cmd_buf[OFN_REG_RESULT_MSB_OFFSET] << 8 |
> + pcu->cmd_buf[OFN_REG_RESULT_LSB_OFFSET];
You should not be checking contents of pcu->cmd_buf after releasing
mutex as someone else could be accessing the same sysfs attribute and
stomping on your data.
> + error = result;
Does firmware guarantee that it would return errors that make sense to
Linux?
> + }
> +
> + return error ?: count;
> +}
> +
> +static DEVICE_ATTR(ofn_reg_data, S_IRUGO | S_IWUSR,
> + ims_pcu_ofn_reg_data_show, ims_pcu_ofn_reg_data_store);
> +
> +static ssize_t ims_pcu_ofn_reg_addr_show(struct device *dev,
> + struct device_attribute *dattr,
> + char *buf)
> +{
> + struct usb_interface *intf = to_usb_interface(dev);
> + struct ims_pcu *pcu = usb_get_intfdata(intf);
> + int error;
> +
> + mutex_lock(&pcu->cmd_mutex);
> + error = scnprintf(buf, PAGE_SIZE, "%x\n", pcu->ofn_reg_addr);
> + mutex_unlock(&pcu->cmd_mutex);
> +
> + return error;
> +}
> +
> +static ssize_t ims_pcu_ofn_reg_addr_store(struct device *dev,
> + struct device_attribute *dattr,
> + const char *buf, size_t count)
> +{
> + struct usb_interface *intf = to_usb_interface(dev);
> + struct ims_pcu *pcu = usb_get_intfdata(intf);
> + int error;
> + int value;
> +
> + error = kstrtoint(buf, 0, &value);
> + if (error)
> + return error;
kstrtou8().
> +
> + mutex_lock(&pcu->cmd_mutex);
> + pcu->ofn_reg_addr = (u8) value;
> + mutex_unlock(&pcu->cmd_mutex);
> +
> + return error ?: count;
> +}
> +
> +static DEVICE_ATTR(ofn_reg_addr, S_IRUGO | S_IWUSR,
> + ims_pcu_ofn_reg_addr_show, ims_pcu_ofn_reg_addr_store);
> +
> +static ssize_t ims_pcu_ofn_bit_show(u8 addr, u8 nr,
> + struct device *dev,
> + struct device_attribute *dattr,
> + char *buf)
> +{
> + struct usb_interface *intf = to_usb_interface(dev);
> + struct ims_pcu *pcu = usb_get_intfdata(intf);
> + int error;
> +
> + mutex_lock(&pcu->cmd_mutex);
> +
> + error = ims_pcu_execute_command(pcu, OFN_GET_CONFIG,
> + &addr, sizeof(addr));
> + if (error >= 0) {
> + const s16 result = pcu->cmd_buf[OFN_REG_RESULT_MSB_OFFSET] << 8 |
> + pcu->cmd_buf[OFN_REG_RESULT_LSB_OFFSET];
> + if (result < 0)
> + error = result;
> + else
> + error = scnprintf(buf, PAGE_SIZE, "%d\n",
> + !!(result & (1 << nr)));
> + }
> +
> + mutex_unlock(&pcu->cmd_mutex);
> + return error;
> +}
> +
> +static ssize_t ims_pcu_ofn_bit_store(u8 addr, u8 nr,
> + struct device *dev,
> + struct device_attribute *dattr,
> + const char *buf, size_t count)
> +{
> + struct usb_interface *intf = to_usb_interface(dev);
> + struct ims_pcu *pcu = usb_get_intfdata(intf);
> + int error;
> + int value;
> + u8 contents;
> +
> +
> + error = kstrtoint(buf, 0, &value);
> + if (error)
> + return error;
> +
> + if (value > 1)
> + return -EINVAL;
> +
> + mutex_lock(&pcu->cmd_mutex);
> +
> + error = ims_pcu_execute_command(pcu, OFN_GET_CONFIG,
> + &addr, sizeof(addr));
> + if (error < 0)
> + goto exit;
> +
> + {
Generally dislike artificial code blocks. Please declare all needed
variables upfront.
> + const s16 result = pcu->cmd_buf[OFN_REG_RESULT_MSB_OFFSET] << 8 |
> + pcu->cmd_buf[OFN_REG_RESULT_LSB_OFFSET];
> + if (result < 0) {
> + error = result;
> + goto exit;
> + }
> + contents = (u8) result;
> + }
> +
> + if (value)
> + contents |= 1 << nr;
> + else
> + contents &= ~(1 << nr);
> +
> + {
> + const u8 buffer[] = { addr, contents };
> + error = ims_pcu_execute_command(pcu, OFN_SET_CONFIG,
> + &buffer, sizeof(buffer));
> + }
> +
> +exit:
> + mutex_unlock(&pcu->cmd_mutex);
> +
> + if (!error) {
> + const s16 result = pcu->cmd_buf[OFN_REG_RESULT_MSB_OFFSET] << 8 |
> + pcu->cmd_buf[OFN_REG_RESULT_LSB_OFFSET];
> + error = result;
> + }
> +
> + return error ?: count;
> +}
> +
> +
> +#define IMS_PCU_BIT_ATTR(name, addr, nr) \
> + static ssize_t ims_pcu_##name##_show(struct device *dev, \
> + struct device_attribute *dattr, \
> + char *buf) \
> + { \
> + return ims_pcu_ofn_bit_show(addr, nr, dev, dattr, buf); \
> + } \
> + \
> + static ssize_t ims_pcu_##name##_store(struct device *dev, \
> + struct device_attribute *dattr, \
> + const char *buf, size_t count) \
> + { \
> + return ims_pcu_ofn_bit_store(addr, nr, dev, dattr, buf, count); \
> + } \
> + \
> + static DEVICE_ATTR(name, S_IRUGO | S_IWUSR, \
> + ims_pcu_##name##_show, ims_pcu_##name##_store)
> +
> +IMS_PCU_BIT_ATTR(ofn_engine_enable, 0x60, 7);
> +IMS_PCU_BIT_ATTR(ofn_speed_enable, 0x60, 6);
> +IMS_PCU_BIT_ATTR(ofn_assert_enable, 0x60, 5);
> +IMS_PCU_BIT_ATTR(ofn_xyquant_enable, 0x60, 4);
> +IMS_PCU_BIT_ATTR(ofn_xyscale_enable, 0x60, 1);
> +
> +IMS_PCU_BIT_ATTR(ofn_scale_x2, 0x63, 6);
> +IMS_PCU_BIT_ATTR(ofn_scale_y2, 0x63, 7);
> +
> static struct attribute *ims_pcu_attrs[] = {
> &ims_pcu_attr_part_number.dattr.attr,
> &ims_pcu_attr_serial_number.dattr.attr,
> @@ -1226,6 +1454,18 @@ static struct attribute *ims_pcu_attrs[] = {
> &dev_attr_reset_device.attr,
> &dev_attr_update_firmware.attr,
> &dev_attr_update_firmware_status.attr,
> +
> +#define IMS_PCU_ATTRS_OFN_START_OFFSET (9)
> +
> + &dev_attr_ofn_reg_data.attr,
> + &dev_attr_ofn_reg_addr.attr,
> + &dev_attr_ofn_engine_enable.attr,
> + &dev_attr_ofn_speed_enable.attr,
> + &dev_attr_ofn_assert_enable.attr,
> + &dev_attr_ofn_xyquant_enable.attr,
> + &dev_attr_ofn_xyscale_enable.attr,
> + &dev_attr_ofn_scale_x2.attr,
> + &dev_attr_ofn_scale_y2.attr,
> NULL
> };
>
> @@ -1244,8 +1484,21 @@ static umode_t ims_pcu_is_attr_visible(struct kobject *kobj,
> mode = 0;
> }
> } else {
> - if (attr == &dev_attr_update_firmware_status.attr)
> + if (attr == &dev_attr_update_firmware_status.attr) {
> mode = 0;
> + } else if (pcu->device_id == 5) {
> + /*
> + PCU-B devices, both GEN_1 and GEN_2(device_id == 5),
> + have no OFN sensor so exposing those attributes
> + for them does not make any sense
> + */
> + int i;
> + for (i = IMS_PCU_ATTRS_OFN_START_OFFSET; ims_pcu_attrs[i]; i++)
> + if (attr == ims_pcu_attrs[i]) {
> + mode = 0;
> + break;
> + }
> + }
> }
>
> return mode;
> @@ -1624,7 +1877,6 @@ static int ims_pcu_init_application_mode(struct ims_pcu *pcu)
> static atomic_t device_no = ATOMIC_INIT(0);
>
> const struct ims_pcu_device_info *info;
> - u8 device_id;
> int error;
>
> error = ims_pcu_get_device_info(pcu);
> @@ -1633,7 +1885,7 @@ static int ims_pcu_init_application_mode(struct ims_pcu *pcu)
> return error;
> }
>
> - error = ims_pcu_identify_type(pcu, &device_id);
> + error = ims_pcu_identify_type(pcu, &pcu->device_id);
> if (error) {
> dev_err(pcu->dev,
> "Failed to identify device, error: %d\n", error);
> @@ -1645,9 +1897,9 @@ static int ims_pcu_init_application_mode(struct ims_pcu *pcu)
> return 0;
> }
>
> - if (device_id >= ARRAY_SIZE(ims_pcu_device_info) ||
> - !ims_pcu_device_info[device_id].keymap) {
> - dev_err(pcu->dev, "Device ID %d is not valid\n", device_id);
> + if (pcu->device_id >= ARRAY_SIZE(ims_pcu_device_info) ||
> + !ims_pcu_device_info[pcu->device_id].keymap) {
> + dev_err(pcu->dev, "Device ID %d is not valid\n", pcu->device_id);
> /* Same as above, punt to userspace */
> return 0;
> }
> @@ -1659,7 +1911,7 @@ static int ims_pcu_init_application_mode(struct ims_pcu *pcu)
> if (error)
> return error;
>
> - info = &ims_pcu_device_info[device_id];
> + info = &ims_pcu_device_info[pcu->device_id];
> error = ims_pcu_setup_buttons(pcu, info->keymap, info->keymap_len);
> if (error)
> goto err_destroy_backlight;
> @@ -1783,14 +2035,14 @@ static int ims_pcu_probe(struct usb_interface *intf,
> if (error)
> goto err_stop_io;
>
> - error = sysfs_create_group(&intf->dev.kobj, &ims_pcu_attr_group);
> - if (error)
> - goto err_stop_io;
> -
> error = pcu->bootloader_mode ?
> ims_pcu_init_bootloader_mode(pcu) :
> ims_pcu_init_application_mode(pcu);
> if (error)
> + goto err_stop_io;
> +
> + error = sysfs_create_group(&intf->dev.kobj, &ims_pcu_attr_group);
> + if (error)
> goto err_remove_sysfs;
Why did you move sysfs group creation? Now one can not observe progress
of firmware update...
Thanks.
--
Dmitry
--
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