Re: [PATCH v2 1/2] platform: Add driver for RAVE Supervisory Processor

From: Andy Shevchenko
Date: Tue Jul 18 2017 - 14:48:41 EST

Next message: Shaohua Li: "[GIT PULL] MD update for 4.13-rc2"
Previous message: Jim Mattson: "Re: [PATCH] KVM: nVMX: Fix exception injection"
In reply to: Andrey Smirnov: "[PATCH v2 1/2] platform: Add driver for RAVE Supervisory Processor"
Next in thread: Andrey Smirnov: "Re: [PATCH v2 1/2] platform: Add driver for RAVE Supervisory Processor"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Jul 18, 2017 at 8:56 PM, Andrey Smirnov
<andrew.smirnov@xxxxxxxxx> 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

Next message: Shaohua Li: "[GIT PULL] MD update for 4.13-rc2"
Previous message: Jim Mattson: "Re: [PATCH] KVM: nVMX: Fix exception injection"
In reply to: Andrey Smirnov: "[PATCH v2 1/2] platform: Add driver for RAVE Supervisory Processor"
Next in thread: Andrey Smirnov: "Re: [PATCH v2 1/2] platform: Add driver for RAVE Supervisory Processor"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]