| 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: <CAHp75Vdtz+XpNodE_tuJT0=z8mmfo0m-PJiSCpxUi8boQrGswg@mail.gmail.com>
Date: Tue, 18 Jul 2017 21:48:34 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Andrey Smirnov <andrew.smirnov@...il.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Chris Healy <cphealy@...il.com>,
Lucas Stach <l.stach@...gutronix.de>,
Nikita Yushchenko <nikita.yoush@...entembedded.com>
Subject: Re: [PATCH v2 1/2] platform: Add driver for RAVE Supervisory Processor
On Tue, Jul 18, 2017 at 8:56 PM, Andrey Smirnov
<andrew.smirnov@...il.com> wrote:
> Add a driver for RAVE Supervisory Processor, an MCU implementing
> varoius bits of housekeeping functionality (watchdoging, backlight
> control, LED control, etc) on RAVE family of products by Zodiac
> Inflight Innovations.
>
> This driver implementes core MFD/serdev device as well as
> communication subroutines necessary for commanding the device.
> +/**
> + * struct rave_sp - RAVE supervisory processor core
> + *
> + * @serdev: Pointer to underlying serdev
> + * @deframer: Stored state of the protocol deframer
> + * @ackid: ACK ID used in last reply sent to the device
> + * @bus_lock: Lock to serialize access to the device
> + * @reply_lock: Lock protecting @reply
> + * @reply: Pointer to memory to store reply payload
> + *
Do you need it?
> + * @part_number_firmware: String containing firmware part number
> + * (retrived once during probing)
No need to repeat this over each parameter. Just move it to (long)
description part and tell that
"part_*, copper_*, and silicon_* parameters are retrived once during probing."
> +static void
> +devm_rave_sp_unregister_event_notifier(struct device *dev, void *res)
static devm_*?!
It looks let's say interesting... (yes, better to provide releasing
devres API as well)
> +static const char *devm_rave_sp_version(struct device *dev, const char *buf)
> +{
> + return devm_kasprintf(dev, GFP_KERNEL, "%02d%02d%02d.%c%c\n",
> + buf[0], le16_to_cpup((const __le16 *)&buf[1]),
To cpu_P_?!
__le16 and %02d?
Are you sure?
> + buf[3], buf[4], buf[5]);
> +}
> +static ssize_t
> +rave_sp_show_part_number(char *buf, const char *version, size_t version_length)
> +{
> + memcpy(buf, version, version_length + 1);
> + return version_length;
Looks suspicious. If it's string, use snprintf(), it it might have
garbage, use %pE.
> +}
> + return sprintf(buf, "%04x\n", le16_to_cpup((__le16 *)status));
cpu_P_?!
> +static int rave_sp_common_get_boot_source(struct rave_sp *sp)
> +{
> + u8 cmd[] = {
> + [0] = RAVE_SP_CMD_BOOT_SOURCE,
> + [1] = 0,
> + [2] = RAVE_SP_BOOT_SOURCE_SET,
> + [3] = 0,
> + };
> + u8 boot_source;
> + const int ret = rave_sp_exec(sp, cmd, sizeof(cmd),
> + &boot_source, sizeof(boot_source));
Why not
const int ret;
ret = rave_sp_exec(sp, cmd, sizeof(cmd), &boot_source, sizeof(boot_source));
> + return (ret < 0) ? ret : boot_source;
Ditto for every similar cases.
> +}
> +static void csum_8b2c(const u8 *buf, size_t size, u8 *crc)
> +{
> + *crc = *buf++;
> + size--;
> +
> + while (size--)
*crc = 0;
while (size--)
*crc += *buf++;
?
> + *crc += *buf++;
> +
> + *crc = 1 + ~(*crc);
> +}
I dunno if lib/crc8.c helps here, worth to check.
> +static void *stuff(unsigned char *dest, const unsigned char *src, size_t n)
> +{
> + size_t i;
> +
> + for (i = 0; i < n; i++) {
Useless i.
while (n--) {
...
}
> + const unsigned char byte = *src++;
> +
> + switch (byte) {
> + case RAVE_SP_STX:
> + case RAVE_SP_ETX:
> + case RAVE_SP_DLE:
> + *dest++ = RAVE_SP_DLE;
> + default:
> + *dest++ = byte;
> + }
> + }
> +
> + return dest;
> +}
> +static u8 rave_sp_reply_code(u8 command)
> +{
> + switch (command) {
> + case 0xA0 ... 0xBE:
> + return command + 0x20;
> + case 0xE0 ... 0xEF:
> + return command | 0x01;
These all look strange. Perhaps this function needs more comments.
> + default:
> + return command + 0x40;
> + }
> +}
> +int rave_sp_exec(struct rave_sp *sp,
> + void *__data, size_t data_size,
> + void *reply_data, size_t reply_data_size)
> +{
> + int ret = 0;
> + unsigned char *data = __data;
> + const u8 ackid = (u8)atomic_inc_return(&sp->ackid);
> + const int command = sp->variant->cmd.translate(data[0]);
> + struct rave_sp_reply reply = {
> + .code = rave_sp_reply_code((u8)command),
> + .ackid = ackid,
> + .data = reply_data,
> + .length = reply_data_size,
> + .received = COMPLETION_INITIALIZER_ONSTACK(reply.received),
> + };
> +
+ command = ...; here!
> + if (command < 0)
> + return command;
> +static void rave_sp_receive_reply(struct rave_sp *sp,
> + const unsigned char *data, size_t length)
> +{
> + struct device *dev = &sp->serdev->dev;
> + struct rave_sp_reply *reply;
> + const size_t payload_length = length - 2;
> +
> + mutex_lock(&sp->reply_lock);
> + reply = sp->reply;
> +
> + if (reply && reply->code == data[0] && reply->ackid == data[1] &&
> + payload_length >= reply->length) {
> + /*
> + * We are relying on memcpy(dst, src, 0) to be a no-op
> + * when handling commands that have a no-payload reply
> + */
> + memcpy(reply->data, &data[2], reply->length);
> + complete(&reply->received);
> + sp->reply = NULL;
> + } else {
> + dev_err(dev, "Ignoring incorrect reply\n");
> + dev_dbg(dev, "Code: expected = 0x%08x received = 0x%08x\n",
> + reply->code, data[0]);
NULL pointer dereference.
> + dev_dbg(dev, "ACK ID: expected = 0x%08x received = 0x%08x\n",
> + reply->ackid, data[1]);
> + dev_dbg(dev, "Length: expected = %zu received = %zu\n",
> + reply->length, payload_length);
... in all cases.
> + }
> +
> + mutex_unlock(&sp->reply_lock);
> +}
> +
> +static void rave_sp_receive_frame(struct rave_sp *sp,
> + const unsigned char *data,
> + size_t length)
> +{
> + if (rave_sp_id_is_event(data[0]))
> + rave_sp_receive_event(sp, data, length);
> + else
> + rave_sp_receive_reply(sp, data, length);
> +}
> +
> +static int rave_sp_rdu1_cmd_translate(enum rave_sp_command command)
> +{
> + if (command >= RAVE_SP_CMD_STATUS &&
> + command <= RAVE_SP_CMD_CONTROL_EVENTS)
> + return command;
> + else
Redundant.
> + return -EINVAL;
> +}
> +
> +static int rave_sp_rdu2_cmd_translate(enum rave_sp_command command)
> +{
> + if (command >= RAVE_SP_CMD_GET_FIRMWARE_VERSION &&
> + command <= RAVE_SP_CMD_GET_GPIO_STATE)
> + return command;
> + else if (command == RAVE_SP_CMD_REQ_COPPER_REV)
> + return 0x28;
> + else
> + return rave_sp_rdu1_cmd_translate(command);
Redundant else in both cases.
> +}
> +static int rave_sp_default_cmd_translate(enum rave_sp_command command)
> +{
> + switch (command) {
> + case RAVE_SP_CMD_GET_FIRMWARE_VERSION:
> + return 0x11;
> + case RAVE_SP_CMD_GET_BOOTLOADER_VERSION:
> + return 0x12;
> + case RAVE_SP_CMD_RESET_REASON:
> + return 0x1F;
> + case RAVE_SP_CMD_BOOT_SOURCE:
> + return 0x14;
> + case RAVE_SP_CMD_RESET:
> + return 0x1E;
> + case RAVE_SP_CMD_SW_WDT:
> + return 0x1C;
Can you set them in order of returned value?
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static void rave_sp_common_init(struct rave_sp *sp)
> +{
> + if (!ret)
Other way around, please.
> + sp->part_number_firmware =
> + devm_rave_sp_version(dev, version);
> + else
> + dev_warn(dev, "CMD_GET_FIRMWARE_VERSION failed %d\n", ret);
> +
> + cmd[0] = RAVE_SP_CMD_GET_BOOTLOADER_VERSION;
> + ret = rave_sp_exec(sp, cmd, sizeof(cmd), version, sizeof(version));
> + if (!ret)
Ditto.
Do this for every similar place.
> + sp->part_number_bootloader =
> + devm_rave_sp_version(dev, version);
> + else
> + dev_warn(dev, "CMD_GET_BOOTLOADER_VERSION failed %d\n", ret);
> +}
> +
> +static void rave_sp_rdu2_init(struct rave_sp *sp)
> +{
> + ret = rave_sp_exec(sp, cmd, sizeof(cmd),
> + &copper_rev, sizeof(copper_rev));
> + if (!ret) {
This is a bit unusual.
> + sp->copper_rev_rmb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
> + copper_rev & 0x1F);
%01x and & 0x1f ?!
> + sp->copper_mod_rmb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
> + copper_rev >> 5);
> + } else {
> + dev_warn(dev,
> + "RAVE_SP_CMD_REQ_COPPER_REV(RMB) failed %d\n", ret);
> + }
> +
> + cmd[2] = RAVE_SP_RDU2_BOARD_TYPE_DEB;
> +
> + ret = rave_sp_exec(sp, cmd, sizeof(cmd),
> + &copper_rev, sizeof(copper_rev));
> + if (!ret) {
> + sp->copper_rev_deb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
> + copper_rev & 0x1F);
> + sp->copper_mod_deb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
> + copper_rev >> 5);
Duplicate! Perhaps, helper function.
> + } else {
> + dev_warn(dev,
> + "RAVE_SP_CMD_REQ_COPPER_REV(DEB) failed %d\n", ret);
> + }
> +}
> +static int rave_sp_probe(struct serdev_device *serdev)
> +{
> + static const struct serdev_device_ops serdev_device_ops = {
> + .receive_buf = rave_sp_receive_buf,
> + .write_wakeup = serdev_device_write_wakeup,
> + };
Please, move outside.
> + struct rave_sp *sp;
> + struct device *dev = &serdev->dev;
Make it first line in this function.
> + u32 baud;
> + int ret;
> + mutex_init(&sp->bus_lock);
> + mutex_init(&sp->reply_lock);
> +static struct serdev_device_driver rave_sp_drv = {
> + .probe = rave_sp_probe,
> + .remove = rave_sp_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .owner = THIS_MODULE,
Redundant I suppose.
> + .of_match_table = rave_sp_dt_ids,
> + },
> +};
> +static inline unsigned long rave_sp_action(u8 event, u8 value)
_pack_action?
> +{
> + return ((unsigned long)value << 8) | event;
> +}
> +
> +static inline u8 rave_sp_action_get_event(unsigned long action)
_unpack_event?
> +{
> + return action & 0xff;
> +}
> +
> +static inline u8 rave_sp_action_get_value(unsigned long action)
_unpack_value?
> +{
> + return (action >> 8) & 0xff;
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists