| 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: <CAHQ1cqHd0KyfKi8My5WiRAZNBAurDjEn40rpmnqA5P=VOLo_jg@mail.gmail.com>
Date: Mon, 24 Jul 2017 12:53:00 -0700
From: Andrey Smirnov <andrew.smirnov@...il.com>
To: Andy Shevchenko <andy.shevchenko@...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 v3 1/2] platform: Add driver for RAVE Supervisory Processor
On Mon, Jul 24, 2017 at 10:25 AM, Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
> On Mon, Jul 24, 2017 at 6:09 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.
>
> Thanks for an update!
>
> My (minor) comments below (most of them are related to spelling issues).
>
>> +#define RAVE_SP_RX_BUFFER_SIZE (RAVE_SP_MAX_DATA_SIZE + \
>> + RAVE_SP_CHECKSUM_SIZE)
>
> Slightly better style is
>
> #define RAVE_SP_RX_BUFFER_SIZE \
> (RAVE_SP_MAX_DATA_SIZE + RAVE_SP_CHECKSUM_SIZE)
>
>> +#define RAVE_SP_TX_BUFFER_SIZE (RAVE_SP_STX_ETX_SIZE + \
>> + 2 * RAVE_SP_RX_BUFFER_SIZE)
>
> Ditto.
Sure, will fix in v4.
>
>> +/**
>> + * enum rave_sp_deframer_state - Possible state for de-framer
>> + *
>> + * @RAVE_SP_EXPECT_SOF: scanning data stream for start of frame
>> + * marker
>
> One line?
>
>> + * @RAVE_SP_EXPECT_DATA: start of frame marker detected, collecting
>> + * frame
>
> Ditto
>
>> + * @RAVE_SP_EXPECT_ESCAPED_DATA: escape character received, collecting
>> + * escaped byte
>
> Ditto.
Sure. Will do in v4.
>
>> + */
>
>> +/**
>> + * struct rave_sp_variant_cmds - Variant specific command vtable
>> + *
>> + * @translate: Subroutine to translate common command identifiers to
>> + * device specific variants. Different generations of the
>> + * ICD employed different numberical constants to
>
> Typo: numerical
>
>> + * represent same/similar commands and this function is
>> + * there to address that.
>> + *
>> + * @get_boot_source: Variant dependent implementaion of "get boot
>> + * source" operation
>
> Typo implementation
>
>> + * @set_boot_source: Variant dependent implementaion of "set boot
>> + * source" operation
>
> Ditto.
>
> Field descriptions are supposed to be _short_.
>
>> + */
>
>> +/**
>> + * struct rave_sp_variant - RAVE supervisory processor core variant
>> + *
>> + * @checksum: Variant specific checksum implementation
>> + * @cmd: Variant specific command vtable
>
> Decode vtable.
>
>> + * @init: Variant specific initialization sequence implementation
>> + * @group: Attrubute group for exposed sysfs entries
>
> Run spell checker over the file, please.
>
Hmm, could've sworn I had it as a my pre-commit hook, but it looks
like I didn't. Will do in v4.
>> + */
>
>> + * @part_number_firmware:
>> + * @part_number_bootloader:
>> + * @reset_reason:
>> + * @copper_rev_rmb:
>> + * @copper_rev_deb:
>> + * @silicon_devid:
>> + * @silicon_devrev:
>> + * @copper_mod_rmb:
>> + * @copper_mod_deb:
>
> ???
>
That's my interpretation of you advice to describe those fields in
detailed comment below.
>> + *
>> + * @variant: Device variant specific parameters and
>> + * functions
>> + * @event_notifier_list: Input event notification chain (used with
>> + * corresponding input MFD cell driver)
>
> Make them _short_.
OK, will do in v4.
>
>> + * @group: Attrubute group for exposed sysfs entries
>> + *
>> + *
>> + * part_number_*, reset_reason, copper_*, and silicon_* fields are all
>
>> + * strings retrived from the device during device probing and made
>> + * availible for later userspace consumption via sysfs
>
> Spell checker.
>
Ditto.
>> + *
>> + */
>
>> +int devm_rave_sp_register_event_notifier(struct device *dev,
>> + struct notifier_block *nb)
>> +{
>> + struct rave_sp *sp = dev_get_drvdata(dev->parent);
>> + struct notifier_block **rcnb;
>> + int ret;
>> +
>> + rcnb = devres_alloc(rave_sp_unregister_event_notifier,
>> + sizeof(*rcnb), GFP_KERNEL);
>> + if (!rcnb)
>> + return -ENOMEM;
>> +
>> + ret = blocking_notifier_chain_register(&sp->event_notifier_list, nb);
>> + if (!ret) {
>> + *rcnb = nb;
>> + devres_add(dev, rcnb);
>> + } else {
>> + devres_free(rcnb);
>> + }
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_rave_sp_register_event_notifier);
>
> Did I miss
>
> EXPORT_SYMBOL_GPL(devm_rave_sp_unregister_event_notifier);
>
> ?
No, you did not, as I mentioned in my reply for v2 to you, there's no
use-case for having that function, there's only one MFD-cell driver
that calls devm_rave_sp_register_event_notifier() and it does so as
the last step of its probe(), so there's not going to be any users of
devm_rave_sp_unregister_event_notifier().
>
>> +static const char *devm_rave_sp_version(struct device *dev, const char *buf)
>> +{
>> + /*
>> + * NOTE: Ther format string below uses %02d to display u16
>> + * intentionally for the sake of backwards compatibility with
>> + * legacy software.
>> + */
>> + return devm_kasprintf(dev, GFP_KERNEL, "%02d%02d%02d.%c%c\n",
>> + buf[0], le16_to_cpup((const __le16 *)&buf[1]),
>> + buf[3], buf[4], buf[5]);
>> +}
>
> One more question about le16_to_cpup() use. Is the variable in the
> buffer guaranteed to be always in little endian format?
> Okay, looks like it's protocol which is little endian. Ok.
>
> By the way, here it might be needed to call get_unaligned().
>
Sure, I'll add that just in case.
>> +static ssize_t
>> +i2c_device_status_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + ssize_t ret;
>> + struct rave_sp *sp = dev_get_drvdata(dev);
>> + u8 status[2];
>> + u8 cmd[] = {
>> + [0] = RAVE_SP_CMD_GET_I2C_DEVICE_STATUS,
>> + [1] = 0
>> + };
>> +
>> + ret = rave_sp_exec(sp, cmd, sizeof(cmd), &status, sizeof(status));
>> + if (ret < 0)
>> + return ret;
>> +
>> + return sprintf(buf, "%04x\n", le16_to_cpup((__le16 *)status));
>> +}
>
> Ditto.
>
Will add in v4.
>> +static int rave_sp_write(struct rave_sp *sp, const u8 *data, u8 data_size)
>> +{
>
>> + size_t length;
>
> Slightly better to put this line after *dest definition.
>
OK, coming in v4.
>> + const size_t checksum_length = sp->variant->checksum->length;
>> + unsigned char crc[checksum_length];
>> + unsigned char frame[RAVE_SP_TX_BUFFER_SIZE];
>> + unsigned char *dest = frame;
>
>> +static u8 rave_sp_reply_code(u8 command)
>> +{
>> + /*
>
>> + * There isn't a signle rule that describes command code ->
>
> Typo: single.
>
>> + * ACK code transformation, but, going through various
>> + * versions of ICDs, there appear to be three distinct groups
>> + * that can be described by simple transformation.
>> + */
>
>> + switch (command) {
>> + case 0xA0 ... 0xBE:
>> + /*
>> + * Commands implemented by firmware found in RDU1 and
>> + * older devices all seem to obey the following rule
>> + */
>> + return command + 0x20;
>> + case 0xE0 ... 0xEF:
>> + /*
>> + * Events emitted by all version of the firmare use
>> + * least signifiact bit to get an ACK code
>
> Typos.
>
Will fix in v4.
>> + */
>> + return command | 0x01;
>> + default:
>> + /*
>> + * Commands implemented by firmware found in RDU2 are
>> + * similar to "old" commands, but they use slightly
>> + * different offset
>> + */
>> + 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),
>> + };
>> +
>
>> + if (command < 0)
>> + return command;
>
>
> Shouldn't be like
>
> command = sp->variant->cmd.translate(data[0]);
> if (command < 0)
> return command;
>
> reply.code = rave_sp_reply_code((u8)command);
>
> ?
Shouldn't really make any difference, rave_sp_reply_code() will either
return -EINVAL or some ACK code but in either case it would not be
used due to early return on "command" being less that 0.
>
>> +
>> + mutex_lock(&sp->bus_lock);
>> +
>> + mutex_lock(&sp->reply_lock);
>> + sp->reply = &reply;
>> + mutex_unlock(&sp->reply_lock);
>> +
>
>> + data[0] = (u8)command;
>
> unsigned char implies u8.
Fair point. Will remove the typecast.
>
>> + data[1] = ackid;
>> +
>> + rave_sp_write(sp, data, data_size);
>> +
>> + if (!wait_for_completion_timeout(&reply.received, HZ)) {
>> + dev_err(&sp->serdev->dev, "Command timeout\n");
>> + ret = -ETIMEDOUT;
>> +
>> + mutex_lock(&sp->reply_lock);
>> + sp->reply = NULL;
>> + mutex_unlock(&sp->reply_lock);
>> + }
>> +
>> + mutex_unlock(&sp->bus_lock);
>> + return ret;
>> +}
>
>> +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;
>> +
>
>> + if (command == RAVE_SP_CMD_REQ_COPPER_REV)
>> + return 0x28;
>
> Comment for use magic values?
> Definition for it?
>
>> +
>> + return rave_sp_rdu1_cmd_translate(command);
>> +}
>
>> +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_BOOT_SOURCE:
>> + return 0x14;
>> + case RAVE_SP_CMD_SW_WDT:
>> + return 0x1C;
>> + case RAVE_SP_CMD_RESET:
>> + return 0x1E;
>> + case RAVE_SP_CMD_RESET_REASON:
>> + return 0x1F;
>
> Ditto.
I'll add comment explaining the origin of the magic values.
>
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>
>> +static const char *rave_sp_silicon_to_string(struct device *dev,
>> + const __le32 *version)
>> +{
>> + return devm_kasprintf(dev, GFP_KERNEL, "%08x\n", le32_to_cpup(version));
>
> I don't see why you can't use version value instead of pointer.
>
Sure, I can do that.
>> +}
>
>> +static const char *rave_sp_copper_to_string(struct device *dev, uint8_t version)
>> +{
>> + return devm_kasprintf(dev, GFP_KERNEL, "%02x\n", version);
>> +}
>> +
>> +
>
> Remove extra empty line.
>
>> + .driver = {
>> + .name = KBUILD_MODNAME,
>
> This is bad idea. It might be someone can use a platform driver (as
> part of MFD, for example) and someone else rename driver at some
> point.
> I understand that in practice it's quite unlikely, though better not
> to put potential issues in the first place.
>
Sure, I can convert it to a explicit string.
>> + .of_match_table = rave_sp_dt_ids,
>> + },
>
>> +static inline u8 rave_sp_action_unpack_event(unsigned long action)
>> +{
>> + return action & 0xff;
>> +}
>> +
>> +static inline u8 rave_sp_action_unpack_value(unsigned long action)
>> +{
>> + return (action >> 8) & 0xff;
>> +}
>
> You can drop & 0xff in both cases as it's implied by u8.
>
Will remove in v4.
Thanks,
Andrey Smirnov
Powered by blists - more mailing lists