linux-kernel - Re: [PATCH] UCC TDM driver for QE based MPC83xx platforms.

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: <20080124171920.49d805cb.sfr@canb.auug.org.au>
Date:	Thu, 24 Jan 2008 17:19:20 +1100
From:	Stephen Rothwell <sfr@...b.auug.org.au>
To:	Poonam_Aggrwal-b10812 <b10812@...escale.com>
Cc:	kumar.gala@...escale.com, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	rubini@...ion.unipv.it, linuxppc-dev@...abs.org,
	michael.barkowski@...escale.com, rich.cutler@...escale.com,
	timur@...escale.com, ashish.kalra@...escale.com
Subject: Re: [PATCH] UCC TDM driver for QE based MPC83xx platforms.

On Thu, 24 Jan 2008 10:16:42 +0530 (IST) Poonam_Aggrwal-b10812 <b10812@...escale.com> wrote:
>
> +static int ucc_tdm_probe(struct of_device *ofdev,
> +			 const struct of_device_id *match)
> +{
> +	struct device_node *np = ofdev->node;
> +	struct resource res;
> +	const unsigned int *prop;
> +	u32 ucc_num, device_num, err, ret = 0;
> +	struct device_node *np_tmp = NULL;

You don't need to initialise this.

> +	dma_addr_t physaddr;
> +	void *tdm_buff;
> +	struct ucc_tdm_info *ut_info;
> +
> +	prop = of_get_property(np, "device-id", NULL);

You should check for (prop == NULL).

> +	ucc_num = *prop - 1;
> +	if ((ucc_num < 0) || (ucc_num > 7))
> +		return -ENODEV;
> +
> +	ut_info = &utdm_info[ucc_num];
> +	if (ut_info == NULL) {

This cannot be NULL as you have just taken the address of an array
element.

> +	tdm_ctrl[device_num]->ut_info = ut_info;
> +
> +	tdm_ctrl[device_num]->ut_info->uf_info.ucc_num = ucc_num;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is the same as "ut_info".

> +	tdm_ctrl[device_num]->ut_info->uf_info.tdm_tx_clk =
> +			(char *) of_get_property(np, "fsl,tdm-tx-clk", NULL);
                                ^
We don't normall put spaces here.

> +	tdm_ctrl[device_num]->ut_info->uf_info.tdm_rx_clk =
> +			(char *) of_get_property(np, "fsl,tdm-rx-clk", NULL);
                                ^
Ditto. And later as well.

> +	tdm_ctrl[device_num]->ut_info->uf_info.irq =
> +					irq_of_parse_and_map(np, 0);
> +	err = of_address_to_resource(np, 0, &res);
> +	if (err) {
> +		ret = EINVAL;

This should be -EINVAL or err.

> +		goto get_property_error;

You need to do something about unmapping the irq in the error path.

> +	tdm_ctrl[device_num]->uf_regs = of_iomap(np, 0);
> +
> +	np_tmp = of_find_compatible_node(np_tmp, "slic", "legerity-slic");
> +	if (np_tmp != NULL)
> +		tdm_ctrl[device_num]->leg_slic = 1;
> +	else
> +		tdm_ctrl[device_num]->leg_slic = 0;

	of_node_ut(np_tmp);

> +	tdm_buff = dma_alloc_coherent(NULL, 2 * NR_BUFS * SAMPLE_DEPTH *
> +				tdm_ctrl[device_num]->cfg_ctrl.active_num_ts,
> +					&physaddr, GFP_KERNEL);
> +	if (!tdm_buff) {
> +		printk(KERN_ERR "ucc-tdm: could not allocate buffer"
> +					"descriptors\n");
> +		ret = -ENOMEM;
> +		goto get_property_error;

You need to unmap the uf_regs in the error path.

> +get_property_error:
> +	kfree(tdm_ctrl[device_num]);

Do you need to set "tdm_ctrl[device_num]" to NULL and decrement
num_tdm_devices?

> +	return ret;
> +}
> +
> +static int ucc_tdm_remove(struct of_device *ofdev)
> +{
> +	struct tdm_ctrl *tdm_c;
> +	struct ucc_tdm_info *ut_info;
> +	u32 ucc_num;
> +
> +	tdm_c = dev_get_drvdata(&(ofdev->dev));

	dev_set_drvdata(&of_dev->dev, NULL);

> +	ucc_num = tdm_c->ut_info->uf_info.ucc_num;
> +	ut_info = &utdm_info[ucc_num];
> +	tdm_stop(tdm_c);
> +	tdm_deinit(tdm_c);
> +
> +	ucc_fast_free(tdm_c->uf_private);
> +
> +	dma_free_coherent(NULL, 2 * NR_BUFS * SAMPLE_DEPTH *
> +				tdm_c->cfg_ctrl.active_num_ts,
> +				tdm_c->tdm_input_data,
> +				tdm_c->dma_input_addr);
> +

You need to unmap the uf_reg and the irq.

> +static struct of_device_id ucc_tdm_match[] = {

const, please.

> +	{
> +	 .type = "tdm",
> +	 .compatible = "fsl,ucc-tdm",
> +	 }, {},

We euld normall format this like:

	{ .type = "tdm", .compatible = "fsl,ucc-tdm", },
	{},

> +static struct of_platform_driver ucc_tdm_driver = {

	.driver = {

> +	.name = DRV_NAME,

	},

-- 
Cheers,
Stephen Rothwell                    sfr@...b.auug.org.au
http://www.canb.auug.org.au/~sfr/

Content of type "application/pgp-signature" skipped