Re: [PATCH] [SCSI] pm8001: fix endian issue with code optimization.

[Mark Salyzyn]

Speaking for the Author in the name of expediency, although I have little knowledge of his actions and history ;-)

void * piomb is used to permit pointer math and abstraction from the multitude of response frames. Making it an __u32* would result in side-effects regarding the pointer math code-paths as well. To be pedantic, an union of all the response frame types would provide the abstraction and type checking to work, but the author obviously considered that burdensome, and I would agree.

The opc field is actually 12 bits in length. The 0xFFF respects that. To be pedantic u16 should have been utilized. However, this driver only uses OPCs from 0x001 to 0x029, so the author obviously decided to optimize the code by using an u8 for the moment (loop up OPC_OUB_* in pm8001_hwi.h for list of OPC field entries). One would hope if a higher-order opcode is utilized and or reported from the controller in the future, we would upgrade to an u16 for this ...

Sincerely -- Mark Salyzyn

On Mar 7, 2012, at 10:58 AM, walter harms wrote:

> 
> 
> Am 07.03.2012 16:11, schrieb Mark Salyzyn:
>> One more NAK:
>> 
>>> @@ -3497,7 +3499,7 @@ static int mpi_hw_event(struct pm8001_hba_info *pm8001_ha, void* piomb)
>>> static void process_one_iomb(struct pm8001_hba_info *pm8001_ha, void *piomb)
>>> {
>>> 	u32 pHeader = (u32)*(u32 *)piomb;
>>> -	u8 opc = (u8)((le32_to_cpu(pHeader)) & 0xFFF);
>>> +	u8 opc = (u8)(pHeader & 0xFFF);
>>> 
>>> 	PM8001_MSG_DBG(pm8001_ha, pm8001_printk("process_one_iomb:"));
>> 
>> The swap is necessary. Should be:
>> 
>> 	__le32 pHeader = (__le32)*(__le32 *)piomb;
>> 
>> instead ...
>> 
> 
> hi,
> 
> would it help to make piomb __le32 instead of void ?
> 
> Also i do not understand what want to gain from 0xFFF
> Doing (u8) is effectively doing & 0xFF.
> 
> re,
> wh
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/