linux-kernel - Re: [RFC V1 7/8] smi2021: Add smi2021

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]

Date:	Mon, 18 Mar 2013 10:31:14 +0100
From:	Bjørn Mork <bjorn@...k.no>
To:	Jon Arne Jørgensen <jonarne@...arne.no>
Cc:	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	hverkuil@...all.nl, elezegarcia@...il.com
Subject: Re: [RFC V1 7/8] smi2021: Add smi2021_bl.c

Jon Arne Jørgensen <jonarne@...arne.no> writes:

> This is the smi2021-bootloader module.
> This module will upload the firmware for the different somagic devices.

I really don't understand why you want to make that a separate module.
Building both the bootlader driver and the real driver into the same
module will make sure that either both or none are available, document
the relationship to the end user, and allow you to tell the user that
firmware files may be needed for the real driver by using the
MODULE_FIRMWARE macro (which you should do, IMHO).

> +static unsigned int firmware_version;
> +module_param(firmware_version, int, 0644);
> +MODULE_PARM_DESC(firmware_version,
> +			"Firmware version to be uploaded to device\n"
> +			"if there are more than one firmware present");
> +
> +struct usb_device_id smi2021_bootloader_id_table[] = {
> +	{ USB_DEVICE(0x1c88, 0x0007) },
> +	{ }
> +};
> +
> +struct smi2021_firmware {
> +	int		id;
> +	const char	*name;
> +	int		found;
> +};
> +
> +struct smi2021_firmware available_fw[] = {
> +	{
> +		.id = 0x3c,
> +		.name = "smi2021_3c.bin",
> +	},
> +	{
> +		.id = 0x3e,
> +		.name = "smi2021_3e.bin",
> +	},
> +	{
> +		.id = 0x3f,
> +		.name = "smi2021_3f.bin",
> +	}
> +};


How does the user know which firmware to select?  Will any of them work,
or are they device specific? Either way I believe you should publish all
three names using MODULE_FIRMWARE.


> +static int smi2021_load_firmware(struct usb_device *udev,
> +					const struct firmware *firmware)
> +{
> +	int i, size, rc = 0;
> +	u8 *chunk;
> +	u16 ack = 0x0000;
> +
> +	if (udev == NULL)
> +		goto end_out;

Is this possible? 



> +	size = FIRMWARE_CHUNK_SIZE + FIRMWARE_HEADER_SIZE;
> +	chunk = kzalloc(size, GFP_KERNEL);
> +	chunk[0] = 0x05;
> +	chunk[1] = 0xff;
> +
> +	if (chunk == NULL) {

This on the other hand, will happen.  But you have already oopsed when
you test it here...


Bjørn
--
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/