Re: [PATCH] fix qla2xxx regression on sparc64

Himanshu Madhani
2020-08-21T20:39:13Z

> On Aug 21, 2020, at 3:21 PM, René Rebe <rene@exactcode.de> wrote:
>
> Hey,
>
>> On 21. Aug 2020, at 20:00, Himanshu Madhani <himanshu.madhani@oracle.com> wrote:
>>
>>
>>
>>> On Aug 21, 2020, at 12:49 PM, René Rebe <rene@exactcode.de> wrote:
>>>
>>> Hi,
>>>
>>>> On 21. Aug 2020, at 17:17, Himanshu Madhani <himanshu.madhani@oracle.com> wrote:
>>>>
>>>> Hi Rene,
>>>>
>>>>> On Aug 21, 2020, at 9:47 AM, René Rebe <rene@exactcode.de> wrote:
>>>>>
>>>>> Hey,
>>>>>
>>>>>> On 21. Aug 2020, at 15:49, Himanshu Madhani <himanshu.madhani@oracle.com> wrote:
>>>>>>
>>>>>> Hi Rene,
>>>>>>
>>>>>>
>>>>>>> On Aug 21, 2020, at 7:28 AM, Rene Rebe <rene@exactcode.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>> Commit 9a50aaefc1b896e734bf7faf3d085f71a360ce97 in 2014 broke
>>>>>>> qla2xxx on sparc64, e.g. as in the Sun Blade 1000 / 2000.
>>>>>>> Unbreak by fixing endianess in nvram firmware defaults
>>>>>>> initialization.
>>>>>>>
>>>>>>
>>>>>> I could not find this commit 9a50aaefc1b896e734bf7faf3d085f71a360ce97 in Linus or SCSI tree.
>>>>>
>>>>> Did I copy this wrong?
>>>>> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/scsi/qla2xxx?id=9a50aaefc1b896e734bf7faf3d085f71a360ce97__;!!GqivPVa7Brio!L6-irbVsNXqZh_TSb_WyaBZ1S3Evo18GqWr8hX2Z-tNl3rZbWv0orEt7Igy_n_89mkIf$
>>>>>
>>>>
>>>> Yes. The actual commit that change those lines is following
>>>>
>>>> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=98aee70d19a7e3203649fa2078464e4f402a0ad8__;!!GqivPVa7Brio!Jsf7iuXhoEimJ56ynA6RrwNm3nkfk3ATaMfNVTUhnU9-NfIZT3eVBiXtsQHXXzu7NMDs$
>>>>
>>>> So your fixes tag should be
>>>>
>>>> Fixes: 98aee70d19a7e ("qla2xxx: Add endianizer to max_payload_size modifier.”)
>>>>
>>>> And looking at the changes, I see that the frame_payload_size was changed to __le16 so this should not cause regression in your env.
>>>>
>>>> Interestingly this patch had fixed the regression and was originally tested on SunBlade-1000
>>>>
>>>> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4e08df3f91837656c36712f559d5ce8d80852760__;!!GqivPVa7Brio!Jsf7iuXhoEimJ56ynA6RrwNm3nkfk3ATaMfNVTUhnU9-NfIZT3eVBiXtsQHXX5pZx4bu$
>>>>
>>>> Can you try reverting 98aee70d19a7e and retest to confirm that this patch indeed introduced regression for you.
>>>
>>> Well, linux kernels since 3.17 did not work on this adapter on sparc64:
>>>
>>> and I reverted the whole 9a50aaefc1b896e734bf7faf3d085f71a360ce97 and factored the changes out and tested what I sent with 5..8.1.
>>>
>>> This was also reported back in the say, but somehow never applied, ..?
>>>
>>> https://urldefense.com/v3/__https://lore.kernel.org/lkml/86A17A977688E04689F40F6B1EC20EC601940E06A9@AVMB1.qlogic.org/__;!!GqivPVa7Brio!Jsf7iuXhoEimJ56ynA6RrwNm3nkfk3ATaMfNVTUhnU9-NfIZT3eVBiXtsQHXX9w-_N_o$
>>>
>>
>> Wow. This should have been done the moment Author acknowledged the revert. I am stumped that we are still talking about this revert after 5+ yrs and its not done. Would you please send just plain revert request for commit 98aee70d19a7e rather than reverting whole merge window patch.
>
> Well, how many people care about vintage Sun sparc64 hardware, ..? ;-)
>
> IMHO it makes no sense to revert this in full today. It does not revert cleanly and all the other __le16 conversions did not cause regressions that I know of.
> So instead having one not __le16 member, I suggest the partial revert I posted initially. I can resend it with the updated Fixes: reference as requested
>

Okay. I am fine with the patch you send earlier with Fixes tag and also add Cc to Stable as well so that it get ported in stable release.
I would also like you to add link for the past discussion in the commit message so that we remember why this patch was added.

> /* Firmware Initialization Control Block. */
> - uint16_t version;
> + __le16 version;
> uint16_t reserved_1;
> ++<<<<<<< HEAD
> + __le16 frame_payload_size;
> + __le16 execution_throttle;
> + __le16 exchange_count;
> + __le16 hard_address;
> ++=======
> + uint16_t frame_payload_size;
> + uint16_t execution_throttle;
> + uint16_t exchange_count;
> + uint16_t hard_address;
> ++>>>>>>> parent of 98aee70d19a7... qla2xxx: Add endianizer to max_payload_size modifier.
>
> …
>
> @@@ -8411,11 -5903,11 +8437,19 @@@ qla81xx_nvram_config(scsi_qla_host_t *v
> * Set default initialization control block.
> */
> memset(nv, 0, ha->nvram_size);
> ++<<<<<<< HEAD
> + nv->nvram_version = cpu_to_le16(ICB_VERSION);
> + nv->version = cpu_to_le16(ICB_VERSION);
> + nv->frame_payload_size = cpu_to_le16(2048);
> + nv->execution_throttle = cpu_to_le16(0xFFFF);
> + nv->exchange_count = cpu_to_le16(0);
> ++=======
> + nv->nvram_version = __constant_cpu_to_le16(ICB_VERSION);
> + nv->version = __constant_cpu_to_le16(ICB_VERSION);
> + nv->frame_payload_size = __constant_cpu_to_le16(2048);
> + nv->execution_throttle = __constant_cpu_to_le16(0xFFFF);
> + nv->exchange_count = __constant_cpu_to_le16(0);
> ++>>>>>>> parent of 98aee70d19a7... qla2xxx: Add endianizer to max_payload_size modifier.
>
> …
>
>>> Regards,
>>> René
>>>
>>>>>> Can you please provide details of the commit which introduced this regression.
>>>>>
>>>>> See above, there was plenty of more reworks and code shuffling, and the quoted commit is also a condensed changes of other /14 series and other scsi work, …
>>>>>
>>>>> Have a nice weekend,
>>>>> René
>>>>>
>>>>>> Also when you
>>>>>> resubmit this patch please use Fixes tag. See documentation here for the correct format.
>>>>>>
>>>>>> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst__;!!GqivPVa7Brio!L6-irbVsNXqZh_TSb_WyaBZ1S3Evo18GqWr8hX2Z-tNl3rZbWv0orEt7Igy_n620C3iF$
>>>>>>
>>>>>>> Signed-off-by: René Rebe <rene@exactcode.de>
>>>>>>>
>>>>>>> --- linux-5.8/drivers/scsi/qla2xxx/qla_init.c.vanilla 2020-08-21 09:55:18.600926737 +0200
>>>>>>> +++ linux-5.8/drivers/scsi/qla2xxx/qla_init.c 2020-08-21 09:57:35.992926885 +0200
>>>>>>> @@ -4603,18 +4603,18 @@
>>>>>>> nv->firmware_options[1] = BIT_7 | BIT_5;
>>>>>>> nv->add_firmware_options[0] = BIT_5;
>>>>>>> nv->add_firmware_options[1] = BIT_5 | BIT_4;
>>>>>>> - nv->frame_payload_size = 2048;
>>>>>>> + nv->frame_payload_size = cpu_to_le16(2048);
>>>>>>> nv->special_options[1] = BIT_7;
>>>>>>> } else if (IS_QLA2200(ha)) {
>>>>>>> nv->firmware_options[0] = BIT_2 | BIT_1;
>>>>>>> nv->firmware_options[1] = BIT_7 | BIT_5;
>>>>>>> nv->add_firmware_options[0] = BIT_5;
>>>>>>> nv->add_firmware_options[1] = BIT_5 | BIT_4;
>>>>>>> - nv->frame_payload_size = 1024;
>>>>>>> + nv->frame_payload_size = cpu_to_le16(1024);
>>>>>>> } else if (IS_QLA2100(ha)) {
>>>>>>> nv->firmware_options[0] = BIT_3 | BIT_1;
>>>>>>> nv->firmware_options[1] = BIT_5;
>>>>>>> - nv->frame_payload_size = 1024;
>>>>>>> + nv->frame_payload_size = cpu_to_le16(1024);
>>>>>>> }
>>>>>>>
>>>>>>> nv->max_iocb_allocation = cpu_to_le16(256);
>>>>>>>
>>>>>>> --
>>>>>>> René Rebe, ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin
>>>>>>> https://urldefense.com/v3/__https://exactcode.com__;!!GqivPVa7Brio!KU2euUgVJIRXqzU0RdhkxvMdgtotnSUfdk9KmK73n26lyGlTMhmSvyadcgXXxI-1EV4u$ | https://urldefense.com/v3/__https://t2sde.org__;!!GqivPVa7Brio!KU2euUgVJIRXqzU0RdhkxvMdgtotnSUfdk9KmK73n26lyGlTMhmSvyadcgXXxAAjd8CY$ | https://urldefense.com/v3/__https://rene.rebe.de__;!!GqivPVa7Brio!KU2euUgVJIRXqzU0RdhkxvMdgtotnSUfdk9KmK73n26lyGlTMhmSvyadcgXXxDUjMdad$
>>>>>>
>>>>>> --
>>>>>> Himanshu Madhani Oracle Linux Engineering
>>>>>>
>>>>>
>>>>> --
>>>>> ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin, https://urldefense.com/v3/__https://exactcode.com__;!!GqivPVa7Brio!L6-irbVsNXqZh_TSb_WyaBZ1S3Evo18GqWr8hX2Z-tNl3rZbWv0orEt7Igy_n-uIVWXr$
>>>>> https://urldefense.com/v3/__https://exactscan.com__;!!GqivPVa7Brio!L6-irbVsNXqZh_TSb_WyaBZ1S3Evo18GqWr8hX2Z-tNl3rZbWv0orEt7Igy_n2dniKkK$ | https://urldefense.com/v3/__https://ocrkit.com__;!!GqivPVa7Brio!L6-irbVsNXqZh_TSb_WyaBZ1S3Evo18GqWr8hX2Z-tNl3rZbWv0orEt7Igy_n-v2wVJD$ | https://urldefense.com/v3/__https://t2sde.org__;!!GqivPVa7Brio!L6-irbVsNXqZh_TSb_WyaBZ1S3Evo18GqWr8hX2Z-tNl3rZbWv0orEt7Igy_n9g--eFu$ | https://urldefense.com/v3/__https://rene.rebe.de__;!!GqivPVa7Brio!L6-irbVsNXqZh_TSb_WyaBZ1S3Evo18GqWr8hX2Z-tNl3rZbWv0orEt7Igy_nxllUxx0$
>>>>
>>>> --
>>>> Himanshu Madhani Oracle Linux Engineering
>>>>
>>>
>>> --
>>> ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin, https://urldefense.com/v3/__https://exactcode.com__;!!GqivPVa7Brio!Jsf7iuXhoEimJ56ynA6RrwNm3nkfk3ATaMfNVTUhnU9-NfIZT3eVBiXtsQHXX6haBk6D$
>>> https://urldefense.com/v3/__https://exactscan.com__;!!GqivPVa7Brio!Jsf7iuXhoEimJ56ynA6RrwNm3nkfk3ATaMfNVTUhnU9-NfIZT3eVBiXtsQHXX4W9vLaa$ | https://urldefense.com/v3/__https://ocrkit.com__;!!GqivPVa7Brio!Jsf7iuXhoEimJ56ynA6RrwNm3nkfk3ATaMfNVTUhnU9-NfIZT3eVBiXtsQHXX04ZQeL4$ | https://urldefense.com/v3/__https://t2sde.org__;!!GqivPVa7Brio!Jsf7iuXhoEimJ56ynA6RrwNm3nkfk3ATaMfNVTUhnU9-NfIZT3eVBiXtsQHXX6DRH0bV$ | https://urldefense.com/v3/__https://rene.rebe.de__;!!GqivPVa7Brio!Jsf7iuXhoEimJ56ynA6RrwNm3nkfk3ATaMfNVTUhnU9-NfIZT3eVBiXtsQHXXz0D-Wul$
>>
>> --
>> Himanshu Madhani Oracle Linux Engineering
>>
>
> --
> ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin, https://urldefense.com/v3/__https://exactcode.com__;!!GqivPVa7Brio!Pzi_Eeyo1QdtyP0r5gMlCBHiq2DPWzBnz2dTVcfduR8O8gSkBXIZ3RI4DP0Q74GDxqJE$
> https://urldefense.com/v3/__https://exactscan.com__;!!GqivPVa7Brio!Pzi_Eeyo1QdtyP0r5gMlCBHiq2DPWzBnz2dTVcfduR8O8gSkBXIZ3RI4DP0Q76FLfX-m$ | https://urldefense.com/v3/__https://ocrkit.com__;!!GqivPVa7Brio!Pzi_Eeyo1QdtyP0r5gMlCBHiq2DPWzBnz2dTVcfduR8O8gSkBXIZ3RI4DP0Q70Zxm72q$ | https://urldefense.com/v3/__https://t2sde.org__;!!GqivPVa7Brio!Pzi_Eeyo1QdtyP0r5gMlCBHiq2DPWzBnz2dTVcfduR8O8gSkBXIZ3RI4DP0Q79_X62BE$ | https://urldefense.com/v3/__https://rene.rebe.de__;!!GqivPVa7Brio!Pzi_Eeyo1QdtyP0r5gMlCBHiq2DPWzBnz2dTVcfduR8O8gSkBXIZ3RI4DP0Q71A3vvWa$
>

--
Himanshu Madhani Oracle Linux Engineering