Message-ID: <CAOD=uF67VkUnqYQQpgP=+0ndw-x+c1uWqN3-JUAVN24bgV2c=w@mail.gmail.com>
Date:	Wed, 7 Mar 2012 22:54:17 +0530
From:	santosh prasad nayak <santoshprasadnayak@...il.com>
To:	Mark Salyzyn <mark_salyzyn@...atex.com>
Cc:	wharms@....de, lindar_liu <lindar_liu@...sh.com>,
	James Bottomley <JBottomley@...allels.com>,
	linux-scsi <linux-scsi@...r.kernel.org>,
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
	Jack Wang <jack_wang@...sh.com>
Subject: Re: [PATCH] [SCSI] pm8001: fix endian issue with code optimization.

Following the are the changes as suggested by you. Please let me know
if anymore changes are required
so that I can send a formal patch for it.  The below patch is just for
review not a formal one.


diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
index 597593b..3976fab 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -3150,7 +3150,7 @@ mpi_fw_flash_update_resp(struct pm8001_hba_info
*pm8001_ha, void *piomb)
        struct fw_control_ex    fw_control_context;
        struct fw_flash_Update_resp *ppayload =
                (struct fw_flash_Update_resp *)(piomb + 4);
-       u32 tag = ppayload->tag;
+       u32 tag = le32_to_cpu(ppayload->tag);
        struct pm8001_ccb_info *ccb = &pm8001_ha->ccb_info[tag];
        status = le32_to_cpu(ppayload->status);
        memcpy(&fw_control_context,
@@ -3496,8 +3496,8 @@ 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)(pHeader & 0xFFF);
+       __le32 pHeader = (__le32)*(__le32 *)piomb;
+       u8 opc = (u8)((le32_to_cpu(pHeader)) & 0xFFF);

        PM8001_MSG_DBG(pm8001_ha, pm8001_printk("process_one_iomb:"));

diff --git a/drivers/scsi/pm8001/pm8001_hwi.h b/drivers/scsi/pm8001/pm8001_hwi.h
index 1a4611e..d437309 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.h
+++ b/drivers/scsi/pm8001/pm8001_hwi.h
@@ -599,7 +599,7 @@ struct fw_flash_Update_req {
  *
  */
 struct fw_flash_Update_resp {
-       dma_addr_t      tag;
+       __le32  tag;
        __le32  status;
        u32     reserved[13];
 } __attribute__((packed, aligned(4)));





Regards
Santosh


On Wed, Mar 7, 2012 at 9:40 PM, Mark Salyzyn <mark_salyzyn@...atex.com> wrote:
> 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
>>
>>
>>
>
> ______________________________________________________________________
> This email may contain privileged or confidential information, which should only be used for the purpose for which it was sent by Xyratex. No further rights or licenses are granted to use such information. If you are not the intended recipient of this message, please notify the sender by return and delete it. You may not use, copy, disclose or rely on the information contained in it.
>
> Internet email is susceptible to data corruption, interception and unauthorised amendment for which Xyratex does not accept liability. While we have taken reasonable precautions to ensure that this email is free of viruses, Xyratex does not accept liability for the presence of any computer viruses in this email, nor for any losses caused as a result of viruses.
>
> Xyratex Technology Limited (03134912), Registered in England & Wales, Registered Office, Langstone Road, Havant, Hampshire, PO9 1SA.
>
> The Xyratex group of companies also includes, Xyratex Ltd, registered in Bermuda, Xyratex International Inc, registered in California, Xyratex (Malaysia) Sdn Bhd registered in Malaysia, Xyratex Technology (Wuxi) Co Ltd registered in The People's Republic of China and Xyratex Japan Limited registered in Japan.
> ______________________________________________________________________
>
>
--
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/

Powered by blists - more mailing lists