linux-kernel - Re: [Device-drivers-devel] [PATCH] sigma-firmware: loader for Analog Devices' SigmaStudio

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AANLkTinCqNTsy2qo4HHe8pc3=OZ1N3xtEwhXF_PyE3ux@mail.gmail.com>
Date:	Wed, 19 Jan 2011 21:36:51 -0500
From:	Mike Frysinger <vapier@...too.org>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	linux-kernel@...r.kernel.org,
	device-drivers-devel@...ckfin.uclinux.org
Subject: Re: [Device-drivers-devel] [PATCH] sigma-firmware: loader for Analog
 Devices' SigmaStudio

On Wed, Jan 19, 2011 at 19:37, Andrew Morton wrote:
> On Wed, 12 Jan 2011 00:41:31 -0500 Mike Frysinger wrote:
> The newly-added code has no callsites.  What are we to make of this?

the ASoC drivers have already been submitted and are going through the process

>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> index e8b6a13..8f4f7b2 100644
>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -134,4 +134,16 @@ config ISCSI_IBFT
>>         detect iSCSI boot parameters dynamically during system boot, say Y.
>>         Otherwise, say N.
>>
>> +config SIGMA
>> +     tristate "SigmaStudio firmware loader"
>> +     depends on I2C
>
> The dependence on just I2C looks odd.  What's the rationale here?

the code clearly calls i2c_master_send

>> +int process_sigma_actions(struct i2c_client *client, struct sigma_firmware *ssfw)
>
> Global scope appears to be unnecessary.

idea originally was to let people process things more directly, but
there hasnt been a need so far.  so localizing them until someone
needs them is probably fine.

>> +     pr_debug("%s: crc=%x\n", __func__, crc32(0, fw->data, fw->size));
>
> The code calculates and prints the crc but doesn't actually check it
> against anything.  What's up with that?

it's supposed to abort.  guess we forgot to.

>> +static inline u32 sigma_action_len(struct sigma_action *sa)
>> +{
>> +     return (sa->len_hi << 16) | sa->len;
>> +}
>> +
>> +static inline size_t sigma_action_size(struct sigma_action *sa, u32 payload_len)
>> +{
>> +     return sizeof(*sa) + payload_len + (payload_len % 2);
>> +}
>
> Did these two functions need to be in a header?  If they're only ever
> used in sigma.c then they should be located in that file.

part of the original idea of allowing the callers to process things
more directly if necessary.  should be harmless to leave it here.
-mike
--
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/