linux-kernel - RE: [PATCH 4/5] staging:ti dspbridge: remove unnecessary volatile variables

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, 2 Aug 2010 14:46:03 -0500
From:	"Ramos Falcon, Ernesto" <ernesto@...com>
To:	"Menon, Nishanth" <nm@...com>
CC:	"gregkh@...e.de" <gregkh@...e.de>,
	"Ramirez Luna, Omar" <omar.ramirez@...com>,
	"ohad@...ery.com" <ohad@...ery.com>,
	"ameya.palande@...ia.com" <ameya.palande@...ia.com>,
	"felipe.contreras@...ia.com" <felipe.contreras@...ia.com>,
	"Guzman Lugo, Fernando" <fernando.lugo@...com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>
Subject: RE: [PATCH 4/5] staging:ti dspbridge: remove unnecessary volatile
 variables

Hi,

>-----Original Message-----
>From: Menon, Nishanth
>Sent: Wednesday, July 28, 2010 10:04 AM
>To: Ramos Falcon, Ernesto
>Cc: gregkh@...e.de; Ramirez Luna, Omar; ohad@...ery.com;
>ameya.palande@...ia.com; felipe.contreras@...ia.com; Guzman Lugo, Fernando;
>linux-kernel@...r.kernel.org; andy.shevchenko@...il.com; linux-
>omap@...r.kernel.org
>Subject: Re: [PATCH 4/5] staging:ti dspbridge: remove unnecessary volatile
>variables

<snip>

>> index 08a2f5f..ae1f394 100644
>> --- a/drivers/staging/tidspbridge/core/tiomap3430.c
>> +++ b/drivers/staging/tidspbridge/core/tiomap3430.c
>> @@ -404,7 +404,7 @@ static int bridge_brd_start(struct bridge_dev_context
>*dev_ctxt,
>>  		pr_err("%s: Illegal SM base\n", __func__);
>>  		status = -EPERM;
>>  	} else
>> -		*((volatile u32 *)dw_sync_addr) = 0xffffffff;
>> +		__raw_writel(0xffffffff, dw_sync_addr);
>curious question: any reason not to use writel instead of __raw_writel?
>
I was not aware of the existence of writel, but I think I should use writel instead.

>>
>>  	if (DSP_SUCCEEDED(status)) {
>>  		resources = dev_context->resources;
>> @@ -584,7 +584,7 @@ static int bridge_brd_start(struct bridge_dev_context
>*dev_ctxt,
>>  		dev_dbg(bridge, "Waiting for Sync @ 0x%x\n", dw_sync_addr);
>>  		dev_dbg(bridge, "DSP c_int00 Address =  0x%x\n", dsp_addr);
>>  		if (dsp_debug)
>> -			while (*((volatile u16 *)dw_sync_addr))
>> +			while (__raw_readw(dw_sync_addr))
>>  				;;
>
>may be for a later on patch -> we seem to have a potential infinite loop
>here..
>
This is done intentionally and it is used to debug the dsp.  The system will hang here, and then Lautherback or jtag can be connected to take control of the program execution from this point.

>> diff --git a/drivers/staging/tidspbridge/pmgr/cmm.c
>b/drivers/staging/tidspbridge/pmgr/cmm.c
>> index 874ed64..b7cba1b 100644
>> --- a/drivers/staging/tidspbridge/pmgr/cmm.c
>> +++ b/drivers/staging/tidspbridge/pmgr/cmm.c
>> @@ -1008,6 +1008,7 @@ void *cmm_xlator_alloc_buf(struct cmm_xlatorobject
>*xlator, void *va_buf,
>>  {
>>  	struct cmm_xlator *xlator_obj = (struct cmm_xlator *)xlator;
>>  	void *pbuf = NULL;
>> +	void *tmp_va_buff;
>>  	struct cmm_attrs attrs;
>>
>>  	DBC_REQUIRE(refs > 0);
>> @@ -1019,16 +1020,16 @@ void *cmm_xlator_alloc_buf(struct
>cmm_xlatorobject *xlator, void *va_buf,
>>
>>  	if (xlator_obj) {
>>  		attrs.ul_seg_id = xlator_obj->ul_seg_id;
>> -		*(volatile u32 *)va_buf = 0;
>> +		__raw_writel(0, va_buf);
>>  		/* Alloc SM */
>>  		pbuf =
>>  		    cmm_calloc_buf(xlator_obj->hcmm_mgr, pa_size, &attrs,
>NULL);
>>  		if (pbuf) {
>>  			/* convert to translator(node/strm) process Virtual
>>  			 * address */
>> -			*(volatile u32 **)va_buf =
>> -			    (u32 *) cmm_xlator_translate(xlator,
>> +			 tmp_va_buff = cmm_xlator_translate(xlator,
>>  							 pbuf, CMM_PA2VA);
>> +			__raw_writel((u32)tmp_va_buff, va_buf);
>
>a) is the u32 cast of tmp_va_buff really necessary?
>b) tmp_va_buff is used only in this branch, why not reduce the scope of
>the variable to just to this if branch

Agree!


--
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/