[PATCH] iscsi_ibft: search for broadcom specific ibft sign

[PATCH] iscsi_ibft: search for broadcom specific ibft sign

[vikas.chaudhary]

From: Vikas Chaudhary <vikas.chaudhary@qlogic.com>

Broadcom iscsi offload firmware uses a non standard ibft sign of "BIFT".
This patch modifies the ibft search code to search for "BIFT" along
with the other possible values.

Signed-off-by: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
Acked-by: Kevin Tran <ktran@broadcom.com>
Acked-by: Eddie Wai <eddie.wai@broadcom.com>
---
 drivers/firmware/iscsi_ibft.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/iscsi_ibft.c b/drivers/firmware/iscsi_ibft.c
index 3ee852c..071c2c9 100644
--- a/drivers/firmware/iscsi_ibft.c
+++ b/drivers/firmware/iscsi_ibft.c
@@ -756,6 +756,7 @@ static const struct {
 	 */
 	{ ACPI_SIG_IBFT },
 	{ "iBFT" },
+	{ "BIFT" },	/* Broadcom iSCSI Offload */
 };
 
 static void __init acpi_find_ibft_region(void)
-- 
1.8.2.GIT

Re: [PATCH] iscsi_ibft: search for broadcom specific ibft sign

[Konrad Rzeszutek Wilk]

On Wed, May 07, 2014 at 05:00:20AM -0400, vikas.chaudhary@qlogic.com wrote:
> From: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
> 
> Broadcom iscsi offload firmware uses a non standard ibft sign of "BIFT".

Why? If it uses the standard iBFT format why does it use
a non-standard signature?

> This patch modifies the ibft search code to search for "BIFT" along
> with the other possible values.
> 
> Signed-off-by: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
> Acked-by: Kevin Tran <ktran@broadcom.com>
> Acked-by: Eddie Wai <eddie.wai@broadcom.com>
> ---
>  drivers/firmware/iscsi_ibft.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/firmware/iscsi_ibft.c b/drivers/firmware/iscsi_ibft.c
> index 3ee852c..071c2c9 100644
> --- a/drivers/firmware/iscsi_ibft.c
> +++ b/drivers/firmware/iscsi_ibft.c
> @@ -756,6 +756,7 @@ static const struct {
>  	 */
>  	{ ACPI_SIG_IBFT },
>  	{ "iBFT" },
> +	{ "BIFT" },	/* Broadcom iSCSI Offload */
>  };
>  
>  static void __init acpi_find_ibft_region(void)
> -- 
> 1.8.2.GIT
> 

Re: [PATCH] iscsi_ibft: search for broadcom specific ibft sign

[James Bottomley]

On Wed, 2014-05-07 at 09:47 -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, May 07, 2014 at 05:00:20AM -0400, vikas.chaudhary@qlogic.com wrote:
> > From: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
> > 
> > Broadcom iscsi offload firmware uses a non standard ibft sign of "BIFT".
> 
> Why? If it uses the standard iBFT format why does it use
> a non-standard signature?

This is useful as an academic exercise (and perhaps even a reminder to
broadcom not to do it again) but I don't think we can make it a show
stopper.  The boards have shipped with the non-standard signature, so we
have to work with them.

James

Re: [PATCH] iscsi_ibft: search for broadcom specific ibft sign

[Konrad Rzeszutek Wilk]

On Wed, May 07, 2014 at 07:12:31PM +0000, James Bottomley wrote:
> On Wed, 2014-05-07 at 09:47 -0400, Konrad Rzeszutek Wilk wrote:
> > On Wed, May 07, 2014 at 05:00:20AM -0400, vikas.chaudhary@qlogic.com wrote:
> > > From: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
> > > 
> > > Broadcom iscsi offload firmware uses a non standard ibft sign of "BIFT".
> > 
> > Why? If it uses the standard iBFT format why does it use
> > a non-standard signature?
> 
> This is useful as an academic exercise (and perhaps even a reminder to
> broadcom not to do it again) but I don't think we can make it a show
> stopper.  The boards have shipped with the non-standard signature, so we
> have to work with them.

I agree as the train has left, but this got me thinking about these
questions that I hope Qlogic folks could answer:

 - Mention what else is different - perhaps there are other entries that
   are a bit different? Or maybe the are some non-standard ones added on?

 - How has this been tested? As in had all the fields been tested (so CHAP
   on/off, extra ports, etc).

 - Do future hardware of these cards use the standard one? If so what are they?
   "Anything produced in 2012 and later.." ?
 - Is the subset of hardware that use the non-standard small enough? Would it
   be good to mention it in the commit.

Thanks!
> 
> James
> 

Re: [PATCH] iscsi_ibft: search for broadcom specific ibft sign

[Mike Christie]

On 05/07/2014 02:21 PM, Konrad Rzeszutek Wilk wrote:
> On Wed, May 07, 2014 at 07:12:31PM +0000, James Bottomley wrote:
>> On Wed, 2014-05-07 at 09:47 -0400, Konrad Rzeszutek Wilk wrote:
>>> On Wed, May 07, 2014 at 05:00:20AM -0400, vikas.chaudhary@qlogic.com wrote:
>>>> From: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
>>>>
>>>> Broadcom iscsi offload firmware uses a non standard ibft sign of "BIFT".
>>>
>>> Why? If it uses the standard iBFT format why does it use
>>> a non-standard signature?
>>
>> This is useful as an academic exercise (and perhaps even a reminder to
>> broadcom not to do it again) but I don't think we can make it a show
>> stopper.  The boards have shipped with the non-standard signature, so we
>> have to work with them.
> 
> I agree as the train has left, but this got me thinking about these
> questions that I hope Qlogic folks could answer:
> 
>  - Mention what else is different - perhaps there are other entries that
>    are a bit different? Or maybe the are some non-standard ones added on?
> 
>  - How has this been tested? As in had all the fields been tested (so CHAP
>    on/off, extra ports, etc).
> 

This supports the same stuff as was added in the original commit for
that string:

140363500ddadad0c09cb512cc0c96a4d3efa053

It just was not carried over in the acpi specific table in commit
935a9fee51c945b8942be2d7b4bae069167b4886.

Re: [PATCH] iscsi_ibft: search for broadcom specific ibft sign

[Peter Jones]

On Wed, May 07, 2014 at 02:49:59PM -0500, Mike Christie wrote:
> On 05/07/2014 02:21 PM, Konrad Rzeszutek Wilk wrote:
> > On Wed, May 07, 2014 at 07:12:31PM +0000, James Bottomley wrote:
> >> On Wed, 2014-05-07 at 09:47 -0400, Konrad Rzeszutek Wilk wrote:
> >>> On Wed, May 07, 2014 at 05:00:20AM -0400, vikas.chaudhary@qlogic.com wrote:
> >>>> From: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
> >>>>
> >>>> Broadcom iscsi offload firmware uses a non standard ibft sign of "BIFT".
> >>>
> >>> Why? If it uses the standard iBFT format why does it use
> >>> a non-standard signature?
> >>
> >> This is useful as an academic exercise (and perhaps even a reminder to
> >> broadcom not to do it again) but I don't think we can make it a show
> >> stopper.  The boards have shipped with the non-standard signature, so we
> >> have to work with them.
> > 
> > I agree as the train has left, but this got me thinking about these
> > questions that I hope Qlogic folks could answer:
> > 
> >  - Mention what else is different - perhaps there are other entries that
> >    are a bit different? Or maybe the are some non-standard ones added on?
> > 
> >  - How has this been tested? As in had all the fields been tested (so CHAP
> >    on/off, extra ports, etc).
> > 
> 
> This supports the same stuff as was added in the original commit for
> that string:
> 
> 140363500ddadad0c09cb512cc0c96a4d3efa053
> 
> It just was not carried over in the acpi specific table in commit
> 935a9fee51c945b8942be2d7b4bae069167b4886.

Okay, but that patch leaves the scanning for it pre-ACPI intact.  So
is what you're effectively saying is that they've changed to using ACPI
to identify it, as opposed to scanning through RAM, but kept the
nonstandard table name while doing that?  That is, "BIFT" is really
showing up as the ACPI table name?

-- 
        Peter

Re: [PATCH] iscsi_ibft: search for broadcom specific ibft sign

[Mike Christie]

On 05/07/2014 03:15 PM, Peter Jones wrote:
> On Wed, May 07, 2014 at 02:49:59PM -0500, Mike Christie wrote:
>> On 05/07/2014 02:21 PM, Konrad Rzeszutek Wilk wrote:
>>> On Wed, May 07, 2014 at 07:12:31PM +0000, James Bottomley wrote:
>>>> On Wed, 2014-05-07 at 09:47 -0400, Konrad Rzeszutek Wilk wrote:
>>>>> On Wed, May 07, 2014 at 05:00:20AM -0400, vikas.chaudhary@qlogic.com wrote:
>>>>>> From: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
>>>>>>
>>>>>> Broadcom iscsi offload firmware uses a non standard ibft sign of "BIFT".
>>>>>
>>>>> Why? If it uses the standard iBFT format why does it use
>>>>> a non-standard signature?
>>>>
>>>> This is useful as an academic exercise (and perhaps even a reminder to
>>>> broadcom not to do it again) but I don't think we can make it a show
>>>> stopper.  The boards have shipped with the non-standard signature, so we
>>>> have to work with them.
>>>
>>> I agree as the train has left, but this got me thinking about these
>>> questions that I hope Qlogic folks could answer:
>>>
>>>  - Mention what else is different - perhaps there are other entries that
>>>    are a bit different? Or maybe the are some non-standard ones added on?
>>>
>>>  - How has this been tested? As in had all the fields been tested (so CHAP
>>>    on/off, extra ports, etc).
>>>
>>
>> This supports the same stuff as was added in the original commit for
>> that string:
>>
>> 140363500ddadad0c09cb512cc0c96a4d3efa053
>>
>> It just was not carried over in the acpi specific table in commit
>> 935a9fee51c945b8942be2d7b4bae069167b4886.
> 
> Okay, but that patch leaves the scanning for it pre-ACPI intact. 

Before 935a9fee51c945b8942be2d7b4bae069167b4886, didn't we check for
BIFT in the ACPI table case?

Before that patch, we used to do:
drivers/firmware/iscsi_ibft_find.c:find_ibft_region()

        for (i = 0; i < ARRAY_SIZE(ibft_signs) && !ibft_addr; i++)
                acpi_table_parse(ibft_signs[i].sign, acpi_find_ibft);

and BIFT was in that ibft_signs array.

I was just saying I thought since we added support for BIFT, we had been
checking for it in the ACPI case.

Re: [PATCH] iscsi_ibft: search for broadcom specific ibft sign

[Mike Christie]

On 05/07/2014 03:30 PM, Mike Christie wrote:
> On 05/07/2014 03:15 PM, Peter Jones wrote:
>> On Wed, May 07, 2014 at 02:49:59PM -0500, Mike Christie wrote:
>>> On 05/07/2014 02:21 PM, Konrad Rzeszutek Wilk wrote:
>>>> On Wed, May 07, 2014 at 07:12:31PM +0000, James Bottomley wrote:
>>>>> On Wed, 2014-05-07 at 09:47 -0400, Konrad Rzeszutek Wilk wrote:
>>>>>> On Wed, May 07, 2014 at 05:00:20AM -0400, vikas.chaudhary@qlogic.com wrote:
>>>>>>> From: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
>>>>>>>
>>>>>>> Broadcom iscsi offload firmware uses a non standard ibft sign of "BIFT".
>>>>>>
>>>>>> Why? If it uses the standard iBFT format why does it use
>>>>>> a non-standard signature?
>>>>>
>>>>> This is useful as an academic exercise (and perhaps even a reminder to
>>>>> broadcom not to do it again) but I don't think we can make it a show
>>>>> stopper.  The boards have shipped with the non-standard signature, so we
>>>>> have to work with them.
>>>>
>>>> I agree as the train has left, but this got me thinking about these
>>>> questions that I hope Qlogic folks could answer:
>>>>
>>>>  - Mention what else is different - perhaps there are other entries that
>>>>    are a bit different? Or maybe the are some non-standard ones added on?
>>>>
>>>>  - How has this been tested? As in had all the fields been tested (so CHAP
>>>>    on/off, extra ports, etc).
>>>>
>>>
>>> This supports the same stuff as was added in the original commit for
>>> that string:
>>>
>>> 140363500ddadad0c09cb512cc0c96a4d3efa053
>>>
>>> It just was not carried over in the acpi specific table in commit
>>> 935a9fee51c945b8942be2d7b4bae069167b4886.
>>
>> Okay, but that patch leaves the scanning for it pre-ACPI intact. 
> 
> Before 935a9fee51c945b8942be2d7b4bae069167b4886, didn't we check for
> BIFT in the ACPI table case?
> 
> Before that patch, we used to do:
> drivers/firmware/iscsi_ibft_find.c:find_ibft_region()
> 
>         for (i = 0; i < ARRAY_SIZE(ibft_signs) && !ibft_addr; i++)
>                 acpi_table_parse(ibft_signs[i].sign, acpi_find_ibft);
> 
> and BIFT was in that ibft_signs array.
> 
> I was just saying I thought since we added support for BIFT, we had been
> checking for it in the ACPI case.


I think I am in the wrong. When I added that support I thought BIFT was
supposed to be for both the ACPI and the RAM case, so I had coded it
like above. I am not seeing that in the old mails though, so you might
be right and they just are now adding support for ACPI. Will just wait
for qlogic/broadcom.

Re: [PATCH] iscsi_ibft: search for broadcom specific ibft sign

[Vikas Chaudhary]

[-- Attachment #1: Type: text/plain, Size: 3233 bytes --]



On 08/05/14 2:27 am, "Mike Christie" <michaelc@cs.wisc.edu> wrote:

>On 05/07/2014 03:30 PM, Mike Christie wrote:
>> On 05/07/2014 03:15 PM, Peter Jones wrote:
>>> On Wed, May 07, 2014 at 02:49:59PM -0500, Mike Christie wrote:
>>>> On 05/07/2014 02:21 PM, Konrad Rzeszutek Wilk wrote:
>>>>> On Wed, May 07, 2014 at 07:12:31PM +0000, James Bottomley wrote:
>>>>>> On Wed, 2014-05-07 at 09:47 -0400, Konrad Rzeszutek Wilk wrote:
>>>>>>> On Wed, May 07, 2014 at 05:00:20AM -0400,
>>>>>>>vikas.chaudhary@qlogic.com wrote:
>>>>>>>> From: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
>>>>>>>>
>>>>>>>> Broadcom iscsi offload firmware uses a non standard ibft sign of
>>>>>>>>"BIFT".
>>>>>>>
>>>>>>> Why? If it uses the standard iBFT format why does it use
>>>>>>> a non-standard signature?
>>>>>>
>>>>>> This is useful as an academic exercise (and perhaps even a reminder
>>>>>>to
>>>>>> broadcom not to do it again) but I don't think we can make it a show
>>>>>> stopper.  The boards have shipped with the non-standard signature,
>>>>>>so we
>>>>>> have to work with them.
>>>>>
>>>>> I agree as the train has left, but this got me thinking about these
>>>>> questions that I hope Qlogic folks could answer:
>>>>>
>>>>>  - Mention what else is different - perhaps there are other entries
>>>>>that
>>>>>    are a bit different? Or maybe the are some non-standard ones
>>>>>added on?
>>>>>
>>>>>  - How has this been tested? As in had all the fields been tested
>>>>>(so CHAP
>>>>>    on/off, extra ports, etc).
>>>>>
>>>>
>>>> This supports the same stuff as was added in the original commit for
>>>> that string:
>>>>
>>>> 140363500ddadad0c09cb512cc0c96a4d3efa053
>>>>
>>>> It just was not carried over in the acpi specific table in commit
>>>> 935a9fee51c945b8942be2d7b4bae069167b4886.
>>>
>>> Okay, but that patch leaves the scanning for it pre-ACPI intact.
>> 
>> Before 935a9fee51c945b8942be2d7b4bae069167b4886, didn't we check for
>> BIFT in the ACPI table case?
>> 
>> Before that patch, we used to do:
>> drivers/firmware/iscsi_ibft_find.c:find_ibft_region()
>> 
>>         for (i = 0; i < ARRAY_SIZE(ibft_signs) && !ibft_addr; i++)
>>                 acpi_table_parse(ibft_signs[i].sign, acpi_find_ibft);
>> 
>> and BIFT was in that ibft_signs array.
>> 
>> I was just saying I thought since we added support for BIFT, we had been
>> checking for it in the ACPI case.
>
>
>I think I am in the wrong. When I added that support I thought BIFT was
>supposed to be for both the ACPI and the RAM case, so I had coded it
>like above. I am not seeing that in the old mails though, so you might
>be right and they just are now adding support for ACPI. Will just wait
>for qlogic/broadcom.

Mike, In your original patch 140363500ddadad0c09cb512cc0c96a4d3efa053 we
are checking for BIFT, and BIFT was in ibft_signs[] array which is defined
in iscsi_ibft_find.c.
Latter when patch 935a9fee51c945b8942be2d7b4bae069167b4886 get added, this
patch defined new array of ibft_signs[] in iscsi_ibft.c which does not
have BIFT signature.
Patch 935a9fee51c945b8942be2d7b4bae069167b4886 added to fix finding IBFT
ACPI table on UEFI. We are just enhancing this patch.


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 6091 bytes --]

Re: [PATCH] iscsi_ibft: search for broadcom specific ibft sign

[Konrad Rzeszutek Wilk]

On Fri, May 09, 2014 at 11:50:20AM +0000, Vikas Chaudhary wrote:
> 
> 
> On 08/05/14 2:27 am, "Mike Christie" <michaelc@cs.wisc.edu> wrote:
> 
> >On 05/07/2014 03:30 PM, Mike Christie wrote:
> >> On 05/07/2014 03:15 PM, Peter Jones wrote:
> >>> On Wed, May 07, 2014 at 02:49:59PM -0500, Mike Christie wrote:
> >>>> On 05/07/2014 02:21 PM, Konrad Rzeszutek Wilk wrote:
> >>>>> On Wed, May 07, 2014 at 07:12:31PM +0000, James Bottomley wrote:
> >>>>>> On Wed, 2014-05-07 at 09:47 -0400, Konrad Rzeszutek Wilk wrote:
> >>>>>>> On Wed, May 07, 2014 at 05:00:20AM -0400,
> >>>>>>>vikas.chaudhary@qlogic.com wrote:
> >>>>>>>> From: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
> >>>>>>>>
> >>>>>>>> Broadcom iscsi offload firmware uses a non standard ibft sign of
> >>>>>>>>"BIFT".
> >>>>>>>
> >>>>>>> Why? If it uses the standard iBFT format why does it use
> >>>>>>> a non-standard signature?
> >>>>>>
> >>>>>> This is useful as an academic exercise (and perhaps even a reminder
> >>>>>>to
> >>>>>> broadcom not to do it again) but I don't think we can make it a show
> >>>>>> stopper.  The boards have shipped with the non-standard signature,
> >>>>>>so we
> >>>>>> have to work with them.
> >>>>>
> >>>>> I agree as the train has left, but this got me thinking about these
> >>>>> questions that I hope Qlogic folks could answer:
> >>>>>
> >>>>>  - Mention what else is different - perhaps there are other entries
> >>>>>that
> >>>>>    are a bit different? Or maybe the are some non-standard ones
> >>>>>added on?
> >>>>>
> >>>>>  - How has this been tested? As in had all the fields been tested
> >>>>>(so CHAP
> >>>>>    on/off, extra ports, etc).
> >>>>>
> >>>>
> >>>> This supports the same stuff as was added in the original commit for
> >>>> that string:
> >>>>
> >>>> 140363500ddadad0c09cb512cc0c96a4d3efa053
> >>>>
> >>>> It just was not carried over in the acpi specific table in commit
> >>>> 935a9fee51c945b8942be2d7b4bae069167b4886.
> >>>
> >>> Okay, but that patch leaves the scanning for it pre-ACPI intact.
> >> 
> >> Before 935a9fee51c945b8942be2d7b4bae069167b4886, didn't we check for
> >> BIFT in the ACPI table case?
> >> 
> >> Before that patch, we used to do:
> >> drivers/firmware/iscsi_ibft_find.c:find_ibft_region()
> >> 
> >>         for (i = 0; i < ARRAY_SIZE(ibft_signs) && !ibft_addr; i++)
> >>                 acpi_table_parse(ibft_signs[i].sign, acpi_find_ibft);
> >> 
> >> and BIFT was in that ibft_signs array.
> >> 
> >> I was just saying I thought since we added support for BIFT, we had been
> >> checking for it in the ACPI case.
> >
> >
> >I think I am in the wrong. When I added that support I thought BIFT was
> >supposed to be for both the ACPI and the RAM case, so I had coded it
> >like above. I am not seeing that in the old mails though, so you might
> >be right and they just are now adding support for ACPI. Will just wait
> >for qlogic/broadcom.
> 
> Mike, In your original patch 140363500ddadad0c09cb512cc0c96a4d3efa053 we
> are checking for BIFT, and BIFT was in ibft_signs[] array which is defined
> in iscsi_ibft_find.c.
> Latter when patch 935a9fee51c945b8942be2d7b4bae069167b4886 get added, this
> patch defined new array of ibft_signs[] in iscsi_ibft.c which does not
> have BIFT signature.
> Patch 935a9fee51c945b8942be2d7b4bae069167b4886 added to fix finding IBFT
> ACPI table on UEFI. We are just enhancing this patch.

In a nutsheel this is a fix for a regression that has been there since 3.2
and introduced by 935a9fee51c945b8942be2d7b4bae069167b4886 ("ibft: Fix finding
IBFT ACPI table on UEFI").

Vikas,
Could you resend the patch and include these details in the commit messages:
That this is a fix for said regression and what cards it impacts (or firmwares).

Thank you.

Since this is a regression I can send the patch to Linus right away - but
I really would like to have that information in the git commit message
so that Linus doesn't look funny at me.

Re: [PATCH] iscsi_ibft: search for broadcom specific ibft sign

[Vikas Chaudhary]

[-- Attachment #1: Type: text/plain, Size: 4343 bytes --]



On 09/05/14 6:50 pm, "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>
wrote:

>On Fri, May 09, 2014 at 11:50:20AM +0000, Vikas Chaudhary wrote:
>> 
>> 
>> On 08/05/14 2:27 am, "Mike Christie" <michaelc@cs.wisc.edu> wrote:
>> 
>> >On 05/07/2014 03:30 PM, Mike Christie wrote:
>> >> On 05/07/2014 03:15 PM, Peter Jones wrote:
>> >>> On Wed, May 07, 2014 at 02:49:59PM -0500, Mike Christie wrote:
>> >>>> On 05/07/2014 02:21 PM, Konrad Rzeszutek Wilk wrote:
>> >>>>> On Wed, May 07, 2014 at 07:12:31PM +0000, James Bottomley wrote:
>> >>>>>> On Wed, 2014-05-07 at 09:47 -0400, Konrad Rzeszutek Wilk wrote:
>> >>>>>>> On Wed, May 07, 2014 at 05:00:20AM -0400,
>> >>>>>>>vikas.chaudhary@qlogic.com wrote:
>> >>>>>>>> From: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
>> >>>>>>>>
>> >>>>>>>> Broadcom iscsi offload firmware uses a non standard ibft sign
>>of
>> >>>>>>>>"BIFT".
>> >>>>>>>
>> >>>>>>> Why? If it uses the standard iBFT format why does it use
>> >>>>>>> a non-standard signature?
>> >>>>>>
>> >>>>>> This is useful as an academic exercise (and perhaps even a
>>reminder
>> >>>>>>to
>> >>>>>> broadcom not to do it again) but I don't think we can make it a
>>show
>> >>>>>> stopper.  The boards have shipped with the non-standard
>>signature,
>> >>>>>>so we
>> >>>>>> have to work with them.
>> >>>>>
>> >>>>> I agree as the train has left, but this got me thinking about
>>these
>> >>>>> questions that I hope Qlogic folks could answer:
>> >>>>>
>> >>>>>  - Mention what else is different - perhaps there are other
>>entries
>> >>>>>that
>> >>>>>    are a bit different? Or maybe the are some non-standard ones
>> >>>>>added on?
>> >>>>>
>> >>>>>  - How has this been tested? As in had all the fields been tested
>> >>>>>(so CHAP
>> >>>>>    on/off, extra ports, etc).
>> >>>>>
>> >>>>
>> >>>> This supports the same stuff as was added in the original commit
>>for
>> >>>> that string:
>> >>>>
>> >>>> 140363500ddadad0c09cb512cc0c96a4d3efa053
>> >>>>
>> >>>> It just was not carried over in the acpi specific table in commit
>> >>>> 935a9fee51c945b8942be2d7b4bae069167b4886.
>> >>>
>> >>> Okay, but that patch leaves the scanning for it pre-ACPI intact.
>> >> 
>> >> Before 935a9fee51c945b8942be2d7b4bae069167b4886, didn't we check for
>> >> BIFT in the ACPI table case?
>> >> 
>> >> Before that patch, we used to do:
>> >> drivers/firmware/iscsi_ibft_find.c:find_ibft_region()
>> >> 
>> >>         for (i = 0; i < ARRAY_SIZE(ibft_signs) && !ibft_addr; i++)
>> >>                 acpi_table_parse(ibft_signs[i].sign, acpi_find_ibft);
>> >> 
>> >> and BIFT was in that ibft_signs array.
>> >> 
>> >> I was just saying I thought since we added support for BIFT, we had
>>been
>> >> checking for it in the ACPI case.
>> >
>> >
>> >I think I am in the wrong. When I added that support I thought BIFT was
>> >supposed to be for both the ACPI and the RAM case, so I had coded it
>> >like above. I am not seeing that in the old mails though, so you might
>> >be right and they just are now adding support for ACPI. Will just wait
>> >for qlogic/broadcom.
>> 
>> Mike, In your original patch 140363500ddadad0c09cb512cc0c96a4d3efa053 we
>> are checking for BIFT, and BIFT was in ibft_signs[] array which is
>>defined
>> in iscsi_ibft_find.c.
>> Latter when patch 935a9fee51c945b8942be2d7b4bae069167b4886 get added,
>>this
>> patch defined new array of ibft_signs[] in iscsi_ibft.c which does not
>> have BIFT signature.
>> Patch 935a9fee51c945b8942be2d7b4bae069167b4886 added to fix finding IBFT
>> ACPI table on UEFI. We are just enhancing this patch.
>
>In a nutsheel this is a fix for a regression that has been there since 3.2
>and introduced by 935a9fee51c945b8942be2d7b4bae069167b4886 ("ibft: Fix
>finding
>IBFT ACPI table on UEFI").
>
>Vikas,
>Could you resend the patch and include these details in the commit
>messages:
>That this is a fix for said regression and what cards it impacts (or
>firmwares).
>
>Thank you.
>
>Since this is a regression I can send the patch to Linus right away - but
>I really would like to have that information in the git commit message
>so that Linus doesn't look funny at me.

I have posted updated patch here:-
http://marc.info/?l=linux-scsi&m=139998320611652&w=2

Thanks,
Vikas.


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 6867 bytes --]

Re: [PATCH] iscsi_ibft: search for broadcom specific ibft sign

[Giridhar Malavali]

[-- Attachment #1: Type: text/plain, Size: 1620 bytes --]



On 5/7/14 12:21 PM, "Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com> wrote:

>On Wed, May 07, 2014 at 07:12:31PM +0000, James Bottomley wrote:
>> On Wed, 2014-05-07 at 09:47 -0400, Konrad Rzeszutek Wilk wrote:
>> > On Wed, May 07, 2014 at 05:00:20AM -0400, vikas.chaudhary@qlogic.com
>>wrote:
>> > > From: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
>> > > 
>> > > Broadcom iscsi offload firmware uses a non standard ibft sign of
>>"BIFT".
>> > 
>> > Why? If it uses the standard iBFT format why does it use
>> > a non-standard signature?
>> 
>> This is useful as an academic exercise (and perhaps even a reminder to
>> broadcom not to do it again) but I don't think we can make it a show
>> stopper.  The boards have shipped with the non-standard signature, so we
>> have to work with them.
>
>I agree as the train has left, but this got me thinking about these
>questions that I hope Qlogic folks could answer:
>
> - Mention what else is different - perhaps there are other entries that
>   are a bit different? Or maybe the are some non-standard ones added on?

We are looking into this and will update.

>
> - How has this been tested? As in had all the fields been tested (so CHAP
>   on/off, extra ports, etc).
>
> - Do future hardware of these cards use the standard one? If so what are
>they?
>   "Anything produced in 2012 and later.." ?
> - Is the subset of hardware that use the non-standard small enough?
>Would it
>   be good to mention it in the commit.

Sure, we will provide the required details in commit information.



>
>Thanks!
>> 
>> James
>> 


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4719 bytes --]

Re: [PATCH] iscsi_ibft: search for broadcom specific ibft sign

[Mike Christie]

On 05/07/2014 04:00 AM, vikas.chaudhary@qlogic.com wrote:
> From: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
> 
> Broadcom iscsi offload firmware uses a non standard ibft sign of "BIFT".
> This patch modifies the ibft search code to search for "BIFT" along
> with the other possible values.
> 
> Signed-off-by: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
> Acked-by: Kevin Tran <ktran@broadcom.com>
> Acked-by: Eddie Wai <eddie.wai@broadcom.com>
> ---
>  drivers/firmware/iscsi_ibft.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/firmware/iscsi_ibft.c b/drivers/firmware/iscsi_ibft.c
> index 3ee852c..071c2c9 100644
> --- a/drivers/firmware/iscsi_ibft.c
> +++ b/drivers/firmware/iscsi_ibft.c
> @@ -756,6 +756,7 @@ static const struct {
>  	 */
>  	{ ACPI_SIG_IBFT },
>  	{ "iBFT" },
> +	{ "BIFT" },	/* Broadcom iSCSI Offload */
>  };
>  
>  static void __init acpi_find_ibft_region(void)
> 

Patch looks ok to me.

Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>

Re: [PATCH] iscsi_ibft: search for broadcom specific ibft sign

[Konrad Rzeszutek Wilk]

On Wed, May 07, 2014 at 02:01:48PM -0500, Mike Christie wrote:
> On 05/07/2014 04:00 AM, vikas.chaudhary@qlogic.com wrote:
> > From: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
> > 
> > Broadcom iscsi offload firmware uses a non standard ibft sign of "BIFT".
> > This patch modifies the ibft search code to search for "BIFT" along
> > with the other possible values.
> > 
> > Signed-off-by: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
> > Acked-by: Kevin Tran <ktran@broadcom.com>
> > Acked-by: Eddie Wai <eddie.wai@broadcom.com>
> > ---
> >  drivers/firmware/iscsi_ibft.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/firmware/iscsi_ibft.c b/drivers/firmware/iscsi_ibft.c
> > index 3ee852c..071c2c9 100644
> > --- a/drivers/firmware/iscsi_ibft.c
> > +++ b/drivers/firmware/iscsi_ibft.c
> > @@ -756,6 +756,7 @@ static const struct {
> >  	 */
> >  	{ ACPI_SIG_IBFT },
> >  	{ "iBFT" },
> > +	{ "BIFT" },	/* Broadcom iSCSI Offload */
> >  };
> >  
> >  static void __init acpi_find_ibft_region(void)
> > 
> 
> Patch looks ok to me.
> 
> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>

I've converted your SoB in an Ack-by as this patch is not going
through your tree but mine.