| Message ID | 557B578D.50706@raptorengineeringinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/12/2015 05:05 PM, Timothy Pearson wrote: > The mpt2sas driver crashes if the BIOS does not set up at least one > memory I/O resource. This failure can happen if the device is too > slow to respond during POST and is missed by the BIOS, but Linux > then detects the device later in the boot process. > > This patch aborts initialization and prints a warning if no memory I/O > resources are found. > > Signed-off-by: Timothy Pearson <tpearson@raptorengineeringinc.com> > Tested-by: Timothy Pearson <tpearson@raptorengineeringinc.com> > --- > drivers/scsi/mpt2sas/mpt2sas_base.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c > b/drivers/scsi/mpt2sas/mpt2sas_base.c > index 11248de..15c9504 100644 > --- a/drivers/scsi/mpt2sas/mpt2sas_base.c > +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c > @@ -6,6 +6,8 @@ > * Copyright (C) 2007-2014 LSI Corporation > * Copyright (C) 20013-2014 Avago Technologies > * (mailto: MPT-FusionLinux.pdl@avagotech.com) > + * Copyright (C) 2015 Raptor Engineering > + * (mailto: support@araptorengineeringinc.com) > * > * This program is free software; you can redistribute it and/or > * modify it under the terms of the GNU General Public License > @@ -1582,6 +1584,13 @@ mpt2sas_base_map_resources(struct MPT2SAS_ADAPTER > *ioc) > } > } > > + if (ioc->chip == NULL) { > + printk(MPT2SAS_ERR_FMT "unable to map " > + "adapter memory (resource not found)!\n", ioc->name); > + r = -EINVAL; > + goto out_fail; > + } > + > _base_mask_interrupts(ioc); > > r = _base_get_ioc_facts(ioc, CAN_SLEEP); Just following up on this patch as I have not yet received any response. Thanks!
On 06/16/2015 12:28 PM, Timothy Pearson wrote: > On 06/12/2015 05:05 PM, Timothy Pearson wrote: >> The mpt2sas driver crashes if the BIOS does not set up at least one >> memory I/O resource. This failure can happen if the device is too >> slow to respond during POST and is missed by the BIOS, but Linux >> then detects the device later in the boot process. >> >> This patch aborts initialization and prints a warning if no memory I/O >> resources are found. >> >> Signed-off-by: Timothy Pearson <tpearson@raptorengineeringinc.com> >> Tested-by: Timothy Pearson <tpearson@raptorengineeringinc.com> >> --- >> drivers/scsi/mpt2sas/mpt2sas_base.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c >> b/drivers/scsi/mpt2sas/mpt2sas_base.c >> index 11248de..15c9504 100644 >> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c >> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c >> @@ -6,6 +6,8 @@ >> * Copyright (C) 2007-2014 LSI Corporation >> * Copyright (C) 20013-2014 Avago Technologies >> * (mailto: MPT-FusionLinux.pdl@avagotech.com) >> + * Copyright (C) 2015 Raptor Engineering >> + * (mailto: support@araptorengineeringinc.com) >> * >> * This program is free software; you can redistribute it and/or >> * modify it under the terms of the GNU General Public License >> @@ -1582,6 +1584,13 @@ mpt2sas_base_map_resources(struct MPT2SAS_ADAPTER >> *ioc) >> } >> } >> >> + if (ioc->chip == NULL) { >> + printk(MPT2SAS_ERR_FMT "unable to map " >> + "adapter memory (resource not found)!\n", ioc->name); >> + r = -EINVAL; >> + goto out_fail; >> + } >> + >> _base_mask_interrupts(ioc); >> >> r = _base_get_ioc_facts(ioc, CAN_SLEEP); > > Just following up on this patch as I have not yet received any response. > > Thanks! Hi Tim -- just curious, why was the similar check on ioc->chip just a few lines above the one added by the patch insufficient? That loop block sets memap_sz when it finds an IORESOURCE_MEM so that it only sets ioc->chip once. I wonder if the fix might be simpler if the existing ioc->chip check relocated entirely to where you put it (maybe also pulling the entire error text onto one line for easier grepping). Regards, -- Joe -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/16/2015 12:42 PM, Joe Lawrence wrote: > On 06/16/2015 12:28 PM, Timothy Pearson wrote: >> On 06/12/2015 05:05 PM, Timothy Pearson wrote: >>> The mpt2sas driver crashes if the BIOS does not set up at least one >>> memory I/O resource. This failure can happen if the device is too >>> slow to respond during POST and is missed by the BIOS, but Linux >>> then detects the device later in the boot process. >>> >>> This patch aborts initialization and prints a warning if no memory I/O >>> resources are found. >>> >>> Signed-off-by: Timothy Pearson<tpearson@raptorengineeringinc.com> >>> Tested-by: Timothy Pearson<tpearson@raptorengineeringinc.com> >>> --- >>> drivers/scsi/mpt2sas/mpt2sas_base.c | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c >>> b/drivers/scsi/mpt2sas/mpt2sas_base.c >>> index 11248de..15c9504 100644 >>> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c >>> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c >>> @@ -6,6 +6,8 @@ >>> * Copyright (C) 2007-2014 LSI Corporation >>> * Copyright (C) 20013-2014 Avago Technologies >>> * (mailto: MPT-FusionLinux.pdl@avagotech.com) >>> + * Copyright (C) 2015 Raptor Engineering >>> + * (mailto: support@araptorengineeringinc.com) >>> * >>> * This program is free software; you can redistribute it and/or >>> * modify it under the terms of the GNU General Public License >>> @@ -1582,6 +1584,13 @@ mpt2sas_base_map_resources(struct MPT2SAS_ADAPTER >>> *ioc) >>> } >>> } >>> >>> + if (ioc->chip == NULL) { >>> + printk(MPT2SAS_ERR_FMT "unable to map " >>> + "adapter memory (resource not found)!\n", ioc->name); >>> + r = -EINVAL; >>> + goto out_fail; >>> + } >>> + >>> _base_mask_interrupts(ioc); >>> >>> r = _base_get_ioc_facts(ioc, CAN_SLEEP); >> >> Just following up on this patch as I have not yet received any response. >> >> Thanks! > > Hi Tim -- just curious, why was the similar check on ioc->chip just a > few lines above the one added by the patch insufficient? > > That loop block sets memap_sz when it finds an IORESOURCE_MEM so that it > only sets ioc->chip once. I wonder if the fix might be simpler if the > existing ioc->chip check relocated entirely to where you put it (maybe > also pulling the entire error text onto one line for easier grepping). > > Regards, > > -- Joe If there are no IORESOURCE_MEM resources allocated by the BIOS (i.e. if the BIOS does not run resource allocation on the mpt2sas device) then the check you are referring to is not executed, and the driver attempts to perform operations on a null ioc->chip pointer. I can relocate the check if desired.
On 06/16/2015 01:49 PM, Timothy Pearson wrote: > On 06/16/2015 12:42 PM, Joe Lawrence wrote: >> On 06/16/2015 12:28 PM, Timothy Pearson wrote: >>> On 06/12/2015 05:05 PM, Timothy Pearson wrote: >>>> The mpt2sas driver crashes if the BIOS does not set up at least one >>>> memory I/O resource. This failure can happen if the device is too >>>> slow to respond during POST and is missed by the BIOS, but Linux >>>> then detects the device later in the boot process. >>>> >>>> This patch aborts initialization and prints a warning if no memory I/O >>>> resources are found. >>>> >>>> Signed-off-by: Timothy Pearson<tpearson@raptorengineeringinc.com> >>>> Tested-by: Timothy Pearson<tpearson@raptorengineeringinc.com> >>>> --- >>>> drivers/scsi/mpt2sas/mpt2sas_base.c | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c >>>> b/drivers/scsi/mpt2sas/mpt2sas_base.c >>>> index 11248de..15c9504 100644 >>>> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c >>>> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c >>>> @@ -6,6 +6,8 @@ >>>> * Copyright (C) 2007-2014 LSI Corporation >>>> * Copyright (C) 20013-2014 Avago Technologies >>>> * (mailto: MPT-FusionLinux.pdl@avagotech.com) >>>> + * Copyright (C) 2015 Raptor Engineering >>>> + * (mailto: support@araptorengineeringinc.com) >>>> * >>>> * This program is free software; you can redistribute it and/or >>>> * modify it under the terms of the GNU General Public License >>>> @@ -1582,6 +1584,13 @@ mpt2sas_base_map_resources(struct >>>> MPT2SAS_ADAPTER >>>> *ioc) >>>> } >>>> } >>>> >>>> + if (ioc->chip == NULL) { >>>> + printk(MPT2SAS_ERR_FMT "unable to map " >>>> + "adapter memory (resource not found)!\n", ioc->name); >>>> + r = -EINVAL; >>>> + goto out_fail; >>>> + } >>>> + >>>> _base_mask_interrupts(ioc); >>>> >>>> r = _base_get_ioc_facts(ioc, CAN_SLEEP); >>> >>> Just following up on this patch as I have not yet received any response. >>> >>> Thanks! >> >> Hi Tim -- just curious, why was the similar check on ioc->chip just a >> few lines above the one added by the patch insufficient? >> >> That loop block sets memap_sz when it finds an IORESOURCE_MEM so that it >> only sets ioc->chip once. I wonder if the fix might be simpler if the >> existing ioc->chip check relocated entirely to where you put it (maybe >> also pulling the entire error text onto one line for easier grepping). >> >> Regards, >> >> -- Joe > > If there are no IORESOURCE_MEM resources allocated by the BIOS (i.e. if > the BIOS does not run resource allocation on the mpt2sas device) then > the check you are referring to is not executed, and the driver attempts > to perform operations on a null ioc->chip pointer. > > I can relocate the check if desired. > On looking more closely at the code I think having the two separate checks is useful for debugging. The first error message is triggered if the resource exists but Linux cannot map it for some reason, and the second error message is triggered if the resource does not exist at all (buggy BIOS).
On 06/21/2015 02:46 PM, Timothy Pearson wrote: > On 06/16/2015 01:49 PM, Timothy Pearson wrote: >> On 06/16/2015 12:42 PM, Joe Lawrence wrote: >>> On 06/16/2015 12:28 PM, Timothy Pearson wrote: >>>> On 06/12/2015 05:05 PM, Timothy Pearson wrote: >>>>> The mpt2sas driver crashes if the BIOS does not set up at least one >>>>> memory I/O resource. This failure can happen if the device is too >>>>> slow to respond during POST and is missed by the BIOS, but Linux >>>>> then detects the device later in the boot process. >>>>> >>>>> This patch aborts initialization and prints a warning if no memory I/O >>>>> resources are found. >>>>> >>>>> Signed-off-by: Timothy Pearson<tpearson@raptorengineeringinc.com> >>>>> Tested-by: Timothy Pearson<tpearson@raptorengineeringinc.com> >>>>> --- >>>>> drivers/scsi/mpt2sas/mpt2sas_base.c | 9 +++++++++ >>>>> 1 file changed, 9 insertions(+) >>>>> >>>>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>> b/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>> index 11248de..15c9504 100644 >>>>> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>> @@ -6,6 +6,8 @@ >>>>> * Copyright (C) 2007-2014 LSI Corporation >>>>> * Copyright (C) 20013-2014 Avago Technologies >>>>> * (mailto: MPT-FusionLinux.pdl@avagotech.com) >>>>> + * Copyright (C) 2015 Raptor Engineering >>>>> + * (mailto: support@araptorengineeringinc.com) >>>>> * >>>>> * This program is free software; you can redistribute it and/or >>>>> * modify it under the terms of the GNU General Public License >>>>> @@ -1582,6 +1584,13 @@ mpt2sas_base_map_resources(struct >>>>> MPT2SAS_ADAPTER >>>>> *ioc) >>>>> } >>>>> } >>>>> >>>>> + if (ioc->chip == NULL) { >>>>> + printk(MPT2SAS_ERR_FMT "unable to map " >>>>> + "adapter memory (resource not found)!\n", ioc->name); >>>>> + r = -EINVAL; >>>>> + goto out_fail; >>>>> + } >>>>> + >>>>> _base_mask_interrupts(ioc); >>>>> >>>>> r = _base_get_ioc_facts(ioc, CAN_SLEEP); >>>> >>>> Just following up on this patch as I have not yet received any >>>> response. >>>> >>>> Thanks! >>> >>> Hi Tim -- just curious, why was the similar check on ioc->chip just a >>> few lines above the one added by the patch insufficient? >>> >>> That loop block sets memap_sz when it finds an IORESOURCE_MEM so that it >>> only sets ioc->chip once. I wonder if the fix might be simpler if the >>> existing ioc->chip check relocated entirely to where you put it (maybe >>> also pulling the entire error text onto one line for easier grepping). >>> >>> Regards, >>> >>> -- Joe >> >> If there are no IORESOURCE_MEM resources allocated by the BIOS (i.e. if >> the BIOS does not run resource allocation on the mpt2sas device) then >> the check you are referring to is not executed, and the driver attempts >> to perform operations on a null ioc->chip pointer. >> >> I can relocate the check if desired. >> > > On looking more closely at the code I think having the two separate > checks is useful for debugging. The first error message is triggered if > the resource exists but Linux cannot map it for some reason, and the > second error message is triggered if the resource does not exist at all > (buggy BIOS). Fair enough. The two error messages are unique, so grepping will lead to the correct check. Only other nits would be the addition of a copyright (up to Avago I guess) and an mpt3sas version (these drivers are 99% the same code). Acked-by: Joe Lawrence <joe.lawrence@stratus.com> -- Joe -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Upstream contributor should not add copyright to this driver code. Regards, Sreekanth On Tue, Jun 23, 2015 at 9:24 AM, Joe Lawrence <joe.lawrence@stratus.com> wrote: > > > On 06/21/2015 02:46 PM, Timothy Pearson wrote: >> On 06/16/2015 01:49 PM, Timothy Pearson wrote: >>> On 06/16/2015 12:42 PM, Joe Lawrence wrote: >>>> On 06/16/2015 12:28 PM, Timothy Pearson wrote: >>>>> On 06/12/2015 05:05 PM, Timothy Pearson wrote: >>>>>> The mpt2sas driver crashes if the BIOS does not set up at least one >>>>>> memory I/O resource. This failure can happen if the device is too >>>>>> slow to respond during POST and is missed by the BIOS, but Linux >>>>>> then detects the device later in the boot process. >>>>>> >>>>>> This patch aborts initialization and prints a warning if no memory I/O >>>>>> resources are found. >>>>>> >>>>>> Signed-off-by: Timothy Pearson<tpearson@raptorengineeringinc.com> >>>>>> Tested-by: Timothy Pearson<tpearson@raptorengineeringinc.com> >>>>>> --- >>>>>> drivers/scsi/mpt2sas/mpt2sas_base.c | 9 +++++++++ >>>>>> 1 file changed, 9 insertions(+) >>>>>> >>>>>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>>> b/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>>> index 11248de..15c9504 100644 >>>>>> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>>> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>>> @@ -6,6 +6,8 @@ >>>>>> * Copyright (C) 2007-2014 LSI Corporation >>>>>> * Copyright (C) 20013-2014 Avago Technologies >>>>>> * (mailto: MPT-FusionLinux.pdl@avagotech.com) >>>>>> + * Copyright (C) 2015 Raptor Engineering >>>>>> + * (mailto: support@araptorengineeringinc.com) >>>>>> * >>>>>> * This program is free software; you can redistribute it and/or >>>>>> * modify it under the terms of the GNU General Public License >>>>>> @@ -1582,6 +1584,13 @@ mpt2sas_base_map_resources(struct >>>>>> MPT2SAS_ADAPTER >>>>>> *ioc) >>>>>> } >>>>>> } >>>>>> >>>>>> + if (ioc->chip == NULL) { >>>>>> + printk(MPT2SAS_ERR_FMT "unable to map " >>>>>> + "adapter memory (resource not found)!\n", ioc->name); >>>>>> + r = -EINVAL; >>>>>> + goto out_fail; >>>>>> + } >>>>>> + >>>>>> _base_mask_interrupts(ioc); >>>>>> >>>>>> r = _base_get_ioc_facts(ioc, CAN_SLEEP); >>>>> >>>>> Just following up on this patch as I have not yet received any >>>>> response. >>>>> >>>>> Thanks! >>>> >>>> Hi Tim -- just curious, why was the similar check on ioc->chip just a >>>> few lines above the one added by the patch insufficient? >>>> >>>> That loop block sets memap_sz when it finds an IORESOURCE_MEM so that it >>>> only sets ioc->chip once. I wonder if the fix might be simpler if the >>>> existing ioc->chip check relocated entirely to where you put it (maybe >>>> also pulling the entire error text onto one line for easier grepping). >>>> >>>> Regards, >>>> >>>> -- Joe >>> >>> If there are no IORESOURCE_MEM resources allocated by the BIOS (i.e. if >>> the BIOS does not run resource allocation on the mpt2sas device) then >>> the check you are referring to is not executed, and the driver attempts >>> to perform operations on a null ioc->chip pointer. >>> >>> I can relocate the check if desired. >>> >> >> On looking more closely at the code I think having the two separate >> checks is useful for debugging. The first error message is triggered if >> the resource exists but Linux cannot map it for some reason, and the >> second error message is triggered if the resource does not exist at all >> (buggy BIOS). > > Fair enough. The two error messages are unique, so grepping will lead > to the correct check. > > Only other nits would be the addition of a copyright (up to Avago I > guess) and an mpt3sas version (these drivers are 99% the same code). > > Acked-by: Joe Lawrence <joe.lawrence@stratus.com> > > -- Joe
On Tue, 2015-06-23 at 18:06 +0530, Sreekanth Reddy wrote: > Hi, > > Upstream contributor should not add copyright to this driver code. I can't agree with that as a general rule: it depends on the significance of the contribution. The somewhat ill defined standard for this is the contribution must be "an original work of authorship". I can agree that a trivial bug fix like this is not an original work of authorship, so in this case adding copyright isn't appropriate. James > Regards, > Sreekanth > > On Tue, Jun 23, 2015 at 9:24 AM, Joe Lawrence <joe.lawrence@stratus.com> wrote: > > > > > > On 06/21/2015 02:46 PM, Timothy Pearson wrote: > >> On 06/16/2015 01:49 PM, Timothy Pearson wrote: > >>> On 06/16/2015 12:42 PM, Joe Lawrence wrote: > >>>> On 06/16/2015 12:28 PM, Timothy Pearson wrote: > >>>>> On 06/12/2015 05:05 PM, Timothy Pearson wrote: > >>>>>> The mpt2sas driver crashes if the BIOS does not set up at least one > >>>>>> memory I/O resource. This failure can happen if the device is too > >>>>>> slow to respond during POST and is missed by the BIOS, but Linux > >>>>>> then detects the device later in the boot process. > >>>>>> > >>>>>> This patch aborts initialization and prints a warning if no memory I/O > >>>>>> resources are found. > >>>>>> > >>>>>> Signed-off-by: Timothy Pearson<tpearson@raptorengineeringinc.com> > >>>>>> Tested-by: Timothy Pearson<tpearson@raptorengineeringinc.com> > >>>>>> --- > >>>>>> drivers/scsi/mpt2sas/mpt2sas_base.c | 9 +++++++++ > >>>>>> 1 file changed, 9 insertions(+) > >>>>>> > >>>>>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c > >>>>>> b/drivers/scsi/mpt2sas/mpt2sas_base.c > >>>>>> index 11248de..15c9504 100644 > >>>>>> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c > >>>>>> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c > >>>>>> @@ -6,6 +6,8 @@ > >>>>>> * Copyright (C) 2007-2014 LSI Corporation > >>>>>> * Copyright (C) 20013-2014 Avago Technologies > >>>>>> * (mailto: MPT-FusionLinux.pdl@avagotech.com) > >>>>>> + * Copyright (C) 2015 Raptor Engineering > >>>>>> + * (mailto: support@araptorengineeringinc.com) > >>>>>> * > >>>>>> * This program is free software; you can redistribute it and/or > >>>>>> * modify it under the terms of the GNU General Public License > >>>>>> @@ -1582,6 +1584,13 @@ mpt2sas_base_map_resources(struct > >>>>>> MPT2SAS_ADAPTER > >>>>>> *ioc) > >>>>>> } > >>>>>> } > >>>>>> > >>>>>> + if (ioc->chip == NULL) { > >>>>>> + printk(MPT2SAS_ERR_FMT "unable to map " > >>>>>> + "adapter memory (resource not found)!\n", ioc->name); > >>>>>> + r = -EINVAL; > >>>>>> + goto out_fail; > >>>>>> + } > >>>>>> + > >>>>>> _base_mask_interrupts(ioc); > >>>>>> > >>>>>> r = _base_get_ioc_facts(ioc, CAN_SLEEP); > >>>>> > >>>>> Just following up on this patch as I have not yet received any > >>>>> response. > >>>>> > >>>>> Thanks! > >>>> > >>>> Hi Tim -- just curious, why was the similar check on ioc->chip just a > >>>> few lines above the one added by the patch insufficient? > >>>> > >>>> That loop block sets memap_sz when it finds an IORESOURCE_MEM so that it > >>>> only sets ioc->chip once. I wonder if the fix might be simpler if the > >>>> existing ioc->chip check relocated entirely to where you put it (maybe > >>>> also pulling the entire error text onto one line for easier grepping). > >>>> > >>>> Regards, > >>>> > >>>> -- Joe > >>> > >>> If there are no IORESOURCE_MEM resources allocated by the BIOS (i.e. if > >>> the BIOS does not run resource allocation on the mpt2sas device) then > >>> the check you are referring to is not executed, and the driver attempts > >>> to perform operations on a null ioc->chip pointer. > >>> > >>> I can relocate the check if desired. > >>> > >> > >> On looking more closely at the code I think having the two separate > >> checks is useful for debugging. The first error message is triggered if > >> the resource exists but Linux cannot map it for some reason, and the > >> second error message is triggered if the resource does not exist at all > >> (buggy BIOS). > > > > Fair enough. The two error messages are unique, so grepping will lead > > to the correct check. > > > > Only other nits would be the addition of a copyright (up to Avago I > > guess) and an mpt3sas version (these drivers are 99% the same code). > > > > Acked-by: Joe Lawrence <joe.lawrence@stratus.com> > > > > -- Joe > > > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 23, 2015 at 7:05 PM, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > On Tue, 2015-06-23 at 18:06 +0530, Sreekanth Reddy wrote: >> Hi, >> >> Upstream contributor should not add copyright to this driver code. > > I can't agree with that as a general rule: it depends on the > significance of the contribution. The somewhat ill defined standard for > this is the contribution must be "an original work of authorship". Thanks James. I am not aware of this and this is a learning point for me. As most of the time Avago is the major contributor for this driver so Initial I thought like that. Thanks, Sreekanth > > I can agree that a trivial bug fix like this is not an original work of > authorship, so in this case adding copyright isn't appropriate. > > James > >> Regards, >> Sreekanth >> >> On Tue, Jun 23, 2015 at 9:24 AM, Joe Lawrence <joe.lawrence@stratus.com> wrote: >> > >> > >> > On 06/21/2015 02:46 PM, Timothy Pearson wrote: >> >> On 06/16/2015 01:49 PM, Timothy Pearson wrote: >> >>> On 06/16/2015 12:42 PM, Joe Lawrence wrote: >> >>>> On 06/16/2015 12:28 PM, Timothy Pearson wrote: >> >>>>> On 06/12/2015 05:05 PM, Timothy Pearson wrote: >> >>>>>> The mpt2sas driver crashes if the BIOS does not set up at least one >> >>>>>> memory I/O resource. This failure can happen if the device is too >> >>>>>> slow to respond during POST and is missed by the BIOS, but Linux >> >>>>>> then detects the device later in the boot process. >> >>>>>> >> >>>>>> This patch aborts initialization and prints a warning if no memory I/O >> >>>>>> resources are found. >> >>>>>> >> >>>>>> Signed-off-by: Timothy Pearson<tpearson@raptorengineeringinc.com> >> >>>>>> Tested-by: Timothy Pearson<tpearson@raptorengineeringinc.com> >> >>>>>> --- >> >>>>>> drivers/scsi/mpt2sas/mpt2sas_base.c | 9 +++++++++ >> >>>>>> 1 file changed, 9 insertions(+) >> >>>>>> >> >>>>>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c >> >>>>>> b/drivers/scsi/mpt2sas/mpt2sas_base.c >> >>>>>> index 11248de..15c9504 100644 >> >>>>>> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c >> >>>>>> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c >> >>>>>> @@ -6,6 +6,8 @@ >> >>>>>> * Copyright (C) 2007-2014 LSI Corporation >> >>>>>> * Copyright (C) 20013-2014 Avago Technologies >> >>>>>> * (mailto: MPT-FusionLinux.pdl@avagotech.com) >> >>>>>> + * Copyright (C) 2015 Raptor Engineering >> >>>>>> + * (mailto: support@araptorengineeringinc.com) >> >>>>>> * >> >>>>>> * This program is free software; you can redistribute it and/or >> >>>>>> * modify it under the terms of the GNU General Public License >> >>>>>> @@ -1582,6 +1584,13 @@ mpt2sas_base_map_resources(struct >> >>>>>> MPT2SAS_ADAPTER >> >>>>>> *ioc) >> >>>>>> } >> >>>>>> } >> >>>>>> >> >>>>>> + if (ioc->chip == NULL) { >> >>>>>> + printk(MPT2SAS_ERR_FMT "unable to map " >> >>>>>> + "adapter memory (resource not found)!\n", ioc->name); >> >>>>>> + r = -EINVAL; >> >>>>>> + goto out_fail; >> >>>>>> + } >> >>>>>> + >> >>>>>> _base_mask_interrupts(ioc); >> >>>>>> >> >>>>>> r = _base_get_ioc_facts(ioc, CAN_SLEEP); >> >>>>> >> >>>>> Just following up on this patch as I have not yet received any >> >>>>> response. >> >>>>> >> >>>>> Thanks! >> >>>> >> >>>> Hi Tim -- just curious, why was the similar check on ioc->chip just a >> >>>> few lines above the one added by the patch insufficient? >> >>>> >> >>>> That loop block sets memap_sz when it finds an IORESOURCE_MEM so that it >> >>>> only sets ioc->chip once. I wonder if the fix might be simpler if the >> >>>> existing ioc->chip check relocated entirely to where you put it (maybe >> >>>> also pulling the entire error text onto one line for easier grepping). >> >>>> >> >>>> Regards, >> >>>> >> >>>> -- Joe >> >>> >> >>> If there are no IORESOURCE_MEM resources allocated by the BIOS (i.e. if >> >>> the BIOS does not run resource allocation on the mpt2sas device) then >> >>> the check you are referring to is not executed, and the driver attempts >> >>> to perform operations on a null ioc->chip pointer. >> >>> >> >>> I can relocate the check if desired. >> >>> >> >> >> >> On looking more closely at the code I think having the two separate >> >> checks is useful for debugging. The first error message is triggered if >> >> the resource exists but Linux cannot map it for some reason, and the >> >> second error message is triggered if the resource does not exist at all >> >> (buggy BIOS). >> > >> > Fair enough. The two error messages are unique, so grepping will lead >> > to the correct check. >> > >> > Only other nits would be the addition of a copyright (up to Avago I >> > guess) and an mpt3sas version (these drivers are 99% the same code). >> > >> > Acked-by: Joe Lawrence <joe.lawrence@stratus.com> >> > >> > -- Joe >> >> >> > > >
On 06/23/2015 08:35 AM, James Bottomley wrote: > On Tue, 2015-06-23 at 18:06 +0530, Sreekanth Reddy wrote: >> Hi, >> >> Upstream contributor should not add copyright to this driver code. > > I can't agree with that as a general rule: it depends on the > significance of the contribution. The somewhat ill defined standard for > this is the contribution must be "an original work of authorship". > > I can agree that a trivial bug fix like this is not an original work of > authorship, so in this case adding copyright isn't appropriate. > > James > >> Regards, >> Sreekanth >> >> On Tue, Jun 23, 2015 at 9:24 AM, Joe Lawrence<joe.lawrence@stratus.com> wrote: >>> >>> >>> On 06/21/2015 02:46 PM, Timothy Pearson wrote: >>>> On 06/16/2015 01:49 PM, Timothy Pearson wrote: >>>>> On 06/16/2015 12:42 PM, Joe Lawrence wrote: >>>>>> On 06/16/2015 12:28 PM, Timothy Pearson wrote: >>>>>>> On 06/12/2015 05:05 PM, Timothy Pearson wrote: >>>>>>>> The mpt2sas driver crashes if the BIOS does not set up at least one >>>>>>>> memory I/O resource. This failure can happen if the device is too >>>>>>>> slow to respond during POST and is missed by the BIOS, but Linux >>>>>>>> then detects the device later in the boot process. >>>>>>>> >>>>>>>> This patch aborts initialization and prints a warning if no memory I/O >>>>>>>> resources are found. >>>>>>>> >>>>>>>> Signed-off-by: Timothy Pearson<tpearson@raptorengineeringinc.com> >>>>>>>> Tested-by: Timothy Pearson<tpearson@raptorengineeringinc.com> >>>>>>>> --- >>>>>>>> drivers/scsi/mpt2sas/mpt2sas_base.c | 9 +++++++++ >>>>>>>> 1 file changed, 9 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>>>>> b/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>>>>> index 11248de..15c9504 100644 >>>>>>>> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>>>>> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>>>>> @@ -6,6 +6,8 @@ >>>>>>>> * Copyright (C) 2007-2014 LSI Corporation >>>>>>>> * Copyright (C) 20013-2014 Avago Technologies >>>>>>>> * (mailto: MPT-FusionLinux.pdl@avagotech.com) >>>>>>>> + * Copyright (C) 2015 Raptor Engineering >>>>>>>> + * (mailto: support@araptorengineeringinc.com) >>>>>>>> * >>>>>>>> * This program is free software; you can redistribute it and/or >>>>>>>> * modify it under the terms of the GNU General Public License >>>>>>>> @@ -1582,6 +1584,13 @@ mpt2sas_base_map_resources(struct >>>>>>>> MPT2SAS_ADAPTER >>>>>>>> *ioc) >>>>>>>> } >>>>>>>> } >>>>>>>> >>>>>>>> + if (ioc->chip == NULL) { >>>>>>>> + printk(MPT2SAS_ERR_FMT "unable to map " >>>>>>>> + "adapter memory (resource not found)!\n", ioc->name); >>>>>>>> + r = -EINVAL; >>>>>>>> + goto out_fail; >>>>>>>> + } >>>>>>>> + >>>>>>>> _base_mask_interrupts(ioc); >>>>>>>> >>>>>>>> r = _base_get_ioc_facts(ioc, CAN_SLEEP); >>>>>>> >>>>>>> Just following up on this patch as I have not yet received any >>>>>>> response. >>>>>>> >>>>>>> Thanks! >>>>>> >>>>>> Hi Tim -- just curious, why was the similar check on ioc->chip just a >>>>>> few lines above the one added by the patch insufficient? >>>>>> >>>>>> That loop block sets memap_sz when it finds an IORESOURCE_MEM so that it >>>>>> only sets ioc->chip once. I wonder if the fix might be simpler if the >>>>>> existing ioc->chip check relocated entirely to where you put it (maybe >>>>>> also pulling the entire error text onto one line for easier grepping). >>>>>> >>>>>> Regards, >>>>>> >>>>>> -- Joe >>>>> >>>>> If there are no IORESOURCE_MEM resources allocated by the BIOS (i.e. if >>>>> the BIOS does not run resource allocation on the mpt2sas device) then >>>>> the check you are referring to is not executed, and the driver attempts >>>>> to perform operations on a null ioc->chip pointer. >>>>> >>>>> I can relocate the check if desired. >>>>> >>>> >>>> On looking more closely at the code I think having the two separate >>>> checks is useful for debugging. The first error message is triggered if >>>> the resource exists but Linux cannot map it for some reason, and the >>>> second error message is triggered if the resource does not exist at all >>>> (buggy BIOS). >>> >>> Fair enough. The two error messages are unique, so grepping will lead >>> to the correct check. >>> >>> Only other nits would be the addition of a copyright (up to Avago I >>> guess) and an mpt3sas version (these drivers are 99% the same code). >>> >>> Acked-by: Joe Lawrence<joe.lawrence@stratus.com> >>> >>> -- Joe >> >> >> > Thank you for clarifying the guidelines on copyright lines. I wasn't sure if this was significant enough of a contribution to justify the line or not. Do I need to re-send or can I just state here that the patch can be merged without the copyright line? I don't have any mpt3sas hardware to test with; should I just send in a patch anyway without a Tested-by line? Thanks!
On Tue, 2015-06-23 at 12:33 -0500, Timothy Pearson wrote: > On 06/23/2015 08:35 AM, James Bottomley wrote: > > On Tue, 2015-06-23 at 18:06 +0530, Sreekanth Reddy wrote: > >> Hi, > >> > >> Upstream contributor should not add copyright to this driver code. > > > > I can't agree with that as a general rule: it depends on the > > significance of the contribution. The somewhat ill defined standard for > > this is the contribution must be "an original work of authorship". > > > > I can agree that a trivial bug fix like this is not an original work of > > authorship, so in this case adding copyright isn't appropriate. > > > > James > > > >> Regards, > >> Sreekanth > >> > >> On Tue, Jun 23, 2015 at 9:24 AM, Joe Lawrence<joe.lawrence@stratus.com> wrote: > >>> > >>> > >>> On 06/21/2015 02:46 PM, Timothy Pearson wrote: > >>>> On 06/16/2015 01:49 PM, Timothy Pearson wrote: > >>>>> On 06/16/2015 12:42 PM, Joe Lawrence wrote: > >>>>>> On 06/16/2015 12:28 PM, Timothy Pearson wrote: > >>>>>>> On 06/12/2015 05:05 PM, Timothy Pearson wrote: > >>>>>>>> The mpt2sas driver crashes if the BIOS does not set up at least one > >>>>>>>> memory I/O resource. This failure can happen if the device is too > >>>>>>>> slow to respond during POST and is missed by the BIOS, but Linux > >>>>>>>> then detects the device later in the boot process. > >>>>>>>> > >>>>>>>> This patch aborts initialization and prints a warning if no memory I/O > >>>>>>>> resources are found. > >>>>>>>> > >>>>>>>> Signed-off-by: Timothy Pearson<tpearson@raptorengineeringinc.com> > >>>>>>>> Tested-by: Timothy Pearson<tpearson@raptorengineeringinc.com> > >>>>>>>> --- > >>>>>>>> drivers/scsi/mpt2sas/mpt2sas_base.c | 9 +++++++++ > >>>>>>>> 1 file changed, 9 insertions(+) > >>>>>>>> > >>>>>>>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c > >>>>>>>> b/drivers/scsi/mpt2sas/mpt2sas_base.c > >>>>>>>> index 11248de..15c9504 100644 > >>>>>>>> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c > >>>>>>>> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c > >>>>>>>> @@ -6,6 +6,8 @@ > >>>>>>>> * Copyright (C) 2007-2014 LSI Corporation > >>>>>>>> * Copyright (C) 20013-2014 Avago Technologies > >>>>>>>> * (mailto: MPT-FusionLinux.pdl@avagotech.com) > >>>>>>>> + * Copyright (C) 2015 Raptor Engineering > >>>>>>>> + * (mailto: support@araptorengineeringinc.com) > >>>>>>>> * > >>>>>>>> * This program is free software; you can redistribute it and/or > >>>>>>>> * modify it under the terms of the GNU General Public License > >>>>>>>> @@ -1582,6 +1584,13 @@ mpt2sas_base_map_resources(struct > >>>>>>>> MPT2SAS_ADAPTER > >>>>>>>> *ioc) > >>>>>>>> } > >>>>>>>> } > >>>>>>>> > >>>>>>>> + if (ioc->chip == NULL) { > >>>>>>>> + printk(MPT2SAS_ERR_FMT "unable to map " > >>>>>>>> + "adapter memory (resource not found)!\n", ioc->name); > >>>>>>>> + r = -EINVAL; > >>>>>>>> + goto out_fail; > >>>>>>>> + } > >>>>>>>> + > >>>>>>>> _base_mask_interrupts(ioc); > >>>>>>>> > >>>>>>>> r = _base_get_ioc_facts(ioc, CAN_SLEEP); > >>>>>>> > >>>>>>> Just following up on this patch as I have not yet received any > >>>>>>> response. > >>>>>>> > >>>>>>> Thanks! > >>>>>> > >>>>>> Hi Tim -- just curious, why was the similar check on ioc->chip just a > >>>>>> few lines above the one added by the patch insufficient? > >>>>>> > >>>>>> That loop block sets memap_sz when it finds an IORESOURCE_MEM so that it > >>>>>> only sets ioc->chip once. I wonder if the fix might be simpler if the > >>>>>> existing ioc->chip check relocated entirely to where you put it (maybe > >>>>>> also pulling the entire error text onto one line for easier grepping). > >>>>>> > >>>>>> Regards, > >>>>>> > >>>>>> -- Joe > >>>>> > >>>>> If there are no IORESOURCE_MEM resources allocated by the BIOS (i.e. if > >>>>> the BIOS does not run resource allocation on the mpt2sas device) then > >>>>> the check you are referring to is not executed, and the driver attempts > >>>>> to perform operations on a null ioc->chip pointer. > >>>>> > >>>>> I can relocate the check if desired. > >>>>> > >>>> > >>>> On looking more closely at the code I think having the two separate > >>>> checks is useful for debugging. The first error message is triggered if > >>>> the resource exists but Linux cannot map it for some reason, and the > >>>> second error message is triggered if the resource does not exist at all > >>>> (buggy BIOS). > >>> > >>> Fair enough. The two error messages are unique, so grepping will lead > >>> to the correct check. > >>> > >>> Only other nits would be the addition of a copyright (up to Avago I > >>> guess) and an mpt3sas version (these drivers are 99% the same code). > >>> > >>> Acked-by: Joe Lawrence<joe.lawrence@stratus.com> > >>> > >>> -- Joe > >> > >> > >> > > > > Thank you for clarifying the guidelines on copyright lines. I wasn't > sure if this was significant enough of a contribution to justify the > line or not. > > Do I need to re-send or can I just state here that the patch can be > merged without the copyright line? It's easier if you send a clean copy rather than risk me botching something in the hand edit. > I don't have any mpt3sas hardware to test with; should I just send in a > patch anyway without a Tested-by line? Well, if you haven't actually tested it, adding a tested-by line wouldn't be correct. James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/23/2015 12:45 PM, James Bottomley wrote: > On Tue, 2015-06-23 at 12:33 -0500, Timothy Pearson wrote: >> On 06/23/2015 08:35 AM, James Bottomley wrote: >>> On Tue, 2015-06-23 at 18:06 +0530, Sreekanth Reddy wrote: >>>> Hi, >>>> >>>> Upstream contributor should not add copyright to this driver code. >>> >>> I can't agree with that as a general rule: it depends on the >>> significance of the contribution. The somewhat ill defined standard for >>> this is the contribution must be "an original work of authorship". >>> >>> I can agree that a trivial bug fix like this is not an original work of >>> authorship, so in this case adding copyright isn't appropriate. >>> >>> James >>> >>>> Regards, >>>> Sreekanth >>>> >>>> On Tue, Jun 23, 2015 at 9:24 AM, Joe Lawrence<joe.lawrence@stratus.com> wrote: >>>>> >>>>> >>>>> On 06/21/2015 02:46 PM, Timothy Pearson wrote: >>>>>> On 06/16/2015 01:49 PM, Timothy Pearson wrote: >>>>>>> On 06/16/2015 12:42 PM, Joe Lawrence wrote: >>>>>>>> On 06/16/2015 12:28 PM, Timothy Pearson wrote: >>>>>>>>> On 06/12/2015 05:05 PM, Timothy Pearson wrote: >>>>>>>>>> The mpt2sas driver crashes if the BIOS does not set up at least one >>>>>>>>>> memory I/O resource. This failure can happen if the device is too >>>>>>>>>> slow to respond during POST and is missed by the BIOS, but Linux >>>>>>>>>> then detects the device later in the boot process. >>>>>>>>>> >>>>>>>>>> This patch aborts initialization and prints a warning if no memory I/O >>>>>>>>>> resources are found. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Timothy Pearson<tpearson@raptorengineeringinc.com> >>>>>>>>>> Tested-by: Timothy Pearson<tpearson@raptorengineeringinc.com> >>>>>>>>>> --- >>>>>>>>>> drivers/scsi/mpt2sas/mpt2sas_base.c | 9 +++++++++ >>>>>>>>>> 1 file changed, 9 insertions(+) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>>>>>>> b/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>>>>>>> index 11248de..15c9504 100644 >>>>>>>>>> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>>>>>>> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>>>>>>> @@ -6,6 +6,8 @@ >>>>>>>>>> * Copyright (C) 2007-2014 LSI Corporation >>>>>>>>>> * Copyright (C) 20013-2014 Avago Technologies >>>>>>>>>> * (mailto: MPT-FusionLinux.pdl@avagotech.com) >>>>>>>>>> + * Copyright (C) 2015 Raptor Engineering >>>>>>>>>> + * (mailto: support@araptorengineeringinc.com) >>>>>>>>>> * >>>>>>>>>> * This program is free software; you can redistribute it and/or >>>>>>>>>> * modify it under the terms of the GNU General Public License >>>>>>>>>> @@ -1582,6 +1584,13 @@ mpt2sas_base_map_resources(struct >>>>>>>>>> MPT2SAS_ADAPTER >>>>>>>>>> *ioc) >>>>>>>>>> } >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> + if (ioc->chip == NULL) { >>>>>>>>>> + printk(MPT2SAS_ERR_FMT "unable to map " >>>>>>>>>> + "adapter memory (resource not found)!\n", ioc->name); >>>>>>>>>> + r = -EINVAL; >>>>>>>>>> + goto out_fail; >>>>>>>>>> + } >>>>>>>>>> + >>>>>>>>>> _base_mask_interrupts(ioc); >>>>>>>>>> >>>>>>>>>> r = _base_get_ioc_facts(ioc, CAN_SLEEP); >>>>>>>>> >>>>>>>>> Just following up on this patch as I have not yet received any >>>>>>>>> response. >>>>>>>>> >>>>>>>>> Thanks! >>>>>>>> >>>>>>>> Hi Tim -- just curious, why was the similar check on ioc->chip just a >>>>>>>> few lines above the one added by the patch insufficient? >>>>>>>> >>>>>>>> That loop block sets memap_sz when it finds an IORESOURCE_MEM so that it >>>>>>>> only sets ioc->chip once. I wonder if the fix might be simpler if the >>>>>>>> existing ioc->chip check relocated entirely to where you put it (maybe >>>>>>>> also pulling the entire error text onto one line for easier grepping). >>>>>>>> >>>>>>>> Regards, >>>>>>>> >>>>>>>> -- Joe >>>>>>> >>>>>>> If there are no IORESOURCE_MEM resources allocated by the BIOS (i.e. if >>>>>>> the BIOS does not run resource allocation on the mpt2sas device) then >>>>>>> the check you are referring to is not executed, and the driver attempts >>>>>>> to perform operations on a null ioc->chip pointer. >>>>>>> >>>>>>> I can relocate the check if desired. >>>>>>> >>>>>> >>>>>> On looking more closely at the code I think having the two separate >>>>>> checks is useful for debugging. The first error message is triggered if >>>>>> the resource exists but Linux cannot map it for some reason, and the >>>>>> second error message is triggered if the resource does not exist at all >>>>>> (buggy BIOS). >>>>> >>>>> Fair enough. The two error messages are unique, so grepping will lead >>>>> to the correct check. >>>>> >>>>> Only other nits would be the addition of a copyright (up to Avago I >>>>> guess) and an mpt3sas version (these drivers are 99% the same code). >>>>> >>>>> Acked-by: Joe Lawrence<joe.lawrence@stratus.com> >>>>> >>>>> -- Joe >>>> >>>> >>>> >>> >> >> Thank you for clarifying the guidelines on copyright lines. I wasn't >> sure if this was significant enough of a contribution to justify the >> line or not. >> >> Do I need to re-send or can I just state here that the patch can be >> merged without the copyright line? > > It's easier if you send a clean copy rather than risk me botching > something in the hand edit. OK, will do. >> I don't have any mpt3sas hardware to test with; should I just send in a >> patch anyway without a Tested-by line? > > Well, if you haven't actually tested it, adding a tested-by line > wouldn't be correct. Of course not. My question was, should I send in a patch anyway (with appropriate commit message fields) even though it has not been tested? Thanks!
On Tue, 2015-06-23 at 12:47 -0500, Timothy Pearson wrote: > On 06/23/2015 12:45 PM, James Bottomley wrote: > > On Tue, 2015-06-23 at 12:33 -0500, Timothy Pearson wrote: > >> On 06/23/2015 08:35 AM, James Bottomley wrote: > >>> On Tue, 2015-06-23 at 18:06 +0530, Sreekanth Reddy wrote: > >>>> Hi, > >>>> > >>>> Upstream contributor should not add copyright to this driver code. > >>> > >>> I can't agree with that as a general rule: it depends on the > >>> significance of the contribution. The somewhat ill defined standard for > >>> this is the contribution must be "an original work of authorship". > >>> > >>> I can agree that a trivial bug fix like this is not an original work of > >>> authorship, so in this case adding copyright isn't appropriate. > >>> > >>> James > >>> > >>>> Regards, > >>>> Sreekanth > >>>> > >>>> On Tue, Jun 23, 2015 at 9:24 AM, Joe Lawrence<joe.lawrence@stratus.com> wrote: > >>>>> > >>>>> > >>>>> On 06/21/2015 02:46 PM, Timothy Pearson wrote: > >>>>>> On 06/16/2015 01:49 PM, Timothy Pearson wrote: > >>>>>>> On 06/16/2015 12:42 PM, Joe Lawrence wrote: > >>>>>>>> On 06/16/2015 12:28 PM, Timothy Pearson wrote: > >>>>>>>>> On 06/12/2015 05:05 PM, Timothy Pearson wrote: > >>>>>>>>>> The mpt2sas driver crashes if the BIOS does not set up at least one > >>>>>>>>>> memory I/O resource. This failure can happen if the device is too > >>>>>>>>>> slow to respond during POST and is missed by the BIOS, but Linux > >>>>>>>>>> then detects the device later in the boot process. > >>>>>>>>>> > >>>>>>>>>> This patch aborts initialization and prints a warning if no memory I/O > >>>>>>>>>> resources are found. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Timothy Pearson<tpearson@raptorengineeringinc.com> > >>>>>>>>>> Tested-by: Timothy Pearson<tpearson@raptorengineeringinc.com> > >>>>>>>>>> --- > >>>>>>>>>> drivers/scsi/mpt2sas/mpt2sas_base.c | 9 +++++++++ > >>>>>>>>>> 1 file changed, 9 insertions(+) > >>>>>>>>>> > >>>>>>>>>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c > >>>>>>>>>> b/drivers/scsi/mpt2sas/mpt2sas_base.c > >>>>>>>>>> index 11248de..15c9504 100644 > >>>>>>>>>> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c > >>>>>>>>>> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c > >>>>>>>>>> @@ -6,6 +6,8 @@ > >>>>>>>>>> * Copyright (C) 2007-2014 LSI Corporation > >>>>>>>>>> * Copyright (C) 20013-2014 Avago Technologies > >>>>>>>>>> * (mailto: MPT-FusionLinux.pdl@avagotech.com) > >>>>>>>>>> + * Copyright (C) 2015 Raptor Engineering > >>>>>>>>>> + * (mailto: support@araptorengineeringinc.com) > >>>>>>>>>> * > >>>>>>>>>> * This program is free software; you can redistribute it and/or > >>>>>>>>>> * modify it under the terms of the GNU General Public License > >>>>>>>>>> @@ -1582,6 +1584,13 @@ mpt2sas_base_map_resources(struct > >>>>>>>>>> MPT2SAS_ADAPTER > >>>>>>>>>> *ioc) > >>>>>>>>>> } > >>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>>> + if (ioc->chip == NULL) { > >>>>>>>>>> + printk(MPT2SAS_ERR_FMT "unable to map " > >>>>>>>>>> + "adapter memory (resource not found)!\n", ioc->name); > >>>>>>>>>> + r = -EINVAL; > >>>>>>>>>> + goto out_fail; > >>>>>>>>>> + } > >>>>>>>>>> + > >>>>>>>>>> _base_mask_interrupts(ioc); > >>>>>>>>>> > >>>>>>>>>> r = _base_get_ioc_facts(ioc, CAN_SLEEP); > >>>>>>>>> > >>>>>>>>> Just following up on this patch as I have not yet received any > >>>>>>>>> response. > >>>>>>>>> > >>>>>>>>> Thanks! > >>>>>>>> > >>>>>>>> Hi Tim -- just curious, why was the similar check on ioc->chip just a > >>>>>>>> few lines above the one added by the patch insufficient? > >>>>>>>> > >>>>>>>> That loop block sets memap_sz when it finds an IORESOURCE_MEM so that it > >>>>>>>> only sets ioc->chip once. I wonder if the fix might be simpler if the > >>>>>>>> existing ioc->chip check relocated entirely to where you put it (maybe > >>>>>>>> also pulling the entire error text onto one line for easier grepping). > >>>>>>>> > >>>>>>>> Regards, > >>>>>>>> > >>>>>>>> -- Joe > >>>>>>> > >>>>>>> If there are no IORESOURCE_MEM resources allocated by the BIOS (i.e. if > >>>>>>> the BIOS does not run resource allocation on the mpt2sas device) then > >>>>>>> the check you are referring to is not executed, and the driver attempts > >>>>>>> to perform operations on a null ioc->chip pointer. > >>>>>>> > >>>>>>> I can relocate the check if desired. > >>>>>>> > >>>>>> > >>>>>> On looking more closely at the code I think having the two separate > >>>>>> checks is useful for debugging. The first error message is triggered if > >>>>>> the resource exists but Linux cannot map it for some reason, and the > >>>>>> second error message is triggered if the resource does not exist at all > >>>>>> (buggy BIOS). > >>>>> > >>>>> Fair enough. The two error messages are unique, so grepping will lead > >>>>> to the correct check. > >>>>> > >>>>> Only other nits would be the addition of a copyright (up to Avago I > >>>>> guess) and an mpt3sas version (these drivers are 99% the same code). > >>>>> > >>>>> Acked-by: Joe Lawrence<joe.lawrence@stratus.com> > >>>>> > >>>>> -- Joe > >>>> > >>>> > >>>> > >>> > >> > >> Thank you for clarifying the guidelines on copyright lines. I wasn't > >> sure if this was significant enough of a contribution to justify the > >> line or not. > >> > >> Do I need to re-send or can I just state here that the patch can be > >> merged without the copyright line? > > > > It's easier if you send a clean copy rather than risk me botching > > something in the hand edit. > > OK, will do. > > >> I don't have any mpt3sas hardware to test with; should I just send in a > >> patch anyway without a Tested-by line? > > > > Well, if you haven't actually tested it, adding a tested-by line > > wouldn't be correct. > > Of course not. My question was, should I send in a patch anyway (with > appropriate commit message fields) even though it has not been tested? You mean do you need to retest for a non-material change to a patch still to have a tested-by on it? Ideally, yes, but as long as there are no actual code changes, I don't think it's required. James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/23/2015 12:56 PM, James Bottomley wrote: > On Tue, 2015-06-23 at 12:47 -0500, Timothy Pearson wrote: >> On 06/23/2015 12:45 PM, James Bottomley wrote: >>> On Tue, 2015-06-23 at 12:33 -0500, Timothy Pearson wrote: >>>> On 06/23/2015 08:35 AM, James Bottomley wrote: >>>>> On Tue, 2015-06-23 at 18:06 +0530, Sreekanth Reddy wrote: >>>>>> Hi, >>>>>> >>>>>> Upstream contributor should not add copyright to this driver code. >>>>> >>>>> I can't agree with that as a general rule: it depends on the >>>>> significance of the contribution. The somewhat ill defined standard for >>>>> this is the contribution must be "an original work of authorship". >>>>> >>>>> I can agree that a trivial bug fix like this is not an original work of >>>>> authorship, so in this case adding copyright isn't appropriate. >>>>> >>>>> James >>>>> >>>>>> Regards, >>>>>> Sreekanth >>>>>> >>>>>> On Tue, Jun 23, 2015 at 9:24 AM, Joe Lawrence<joe.lawrence@stratus.com> wrote: >>>>>>> >>>>>>> >>>>>>> On 06/21/2015 02:46 PM, Timothy Pearson wrote: >>>>>>>> On 06/16/2015 01:49 PM, Timothy Pearson wrote: >>>>>>>>> On 06/16/2015 12:42 PM, Joe Lawrence wrote: >>>>>>>>>> On 06/16/2015 12:28 PM, Timothy Pearson wrote: >>>>>>>>>>> On 06/12/2015 05:05 PM, Timothy Pearson wrote: >>>>>>>>>>>> The mpt2sas driver crashes if the BIOS does not set up at least one >>>>>>>>>>>> memory I/O resource. This failure can happen if the device is too >>>>>>>>>>>> slow to respond during POST and is missed by the BIOS, but Linux >>>>>>>>>>>> then detects the device later in the boot process. >>>>>>>>>>>> >>>>>>>>>>>> This patch aborts initialization and prints a warning if no memory I/O >>>>>>>>>>>> resources are found. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Timothy Pearson<tpearson@raptorengineeringinc.com> >>>>>>>>>>>> Tested-by: Timothy Pearson<tpearson@raptorengineeringinc.com> >>>>>>>>>>>> --- >>>>>>>>>>>> drivers/scsi/mpt2sas/mpt2sas_base.c | 9 +++++++++ >>>>>>>>>>>> 1 file changed, 9 insertions(+) >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>>>>>>>>> b/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>>>>>>>>> index 11248de..15c9504 100644 >>>>>>>>>>>> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>>>>>>>>> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c >>>>>>>>>>>> @@ -6,6 +6,8 @@ >>>>>>>>>>>> * Copyright (C) 2007-2014 LSI Corporation >>>>>>>>>>>> * Copyright (C) 20013-2014 Avago Technologies >>>>>>>>>>>> * (mailto: MPT-FusionLinux.pdl@avagotech.com) >>>>>>>>>>>> + * Copyright (C) 2015 Raptor Engineering >>>>>>>>>>>> + * (mailto: support@araptorengineeringinc.com) >>>>>>>>>>>> * >>>>>>>>>>>> * This program is free software; you can redistribute it and/or >>>>>>>>>>>> * modify it under the terms of the GNU General Public License >>>>>>>>>>>> @@ -1582,6 +1584,13 @@ mpt2sas_base_map_resources(struct >>>>>>>>>>>> MPT2SAS_ADAPTER >>>>>>>>>>>> *ioc) >>>>>>>>>>>> } >>>>>>>>>>>> } >>>>>>>>>>>> >>>>>>>>>>>> + if (ioc->chip == NULL) { >>>>>>>>>>>> + printk(MPT2SAS_ERR_FMT "unable to map " >>>>>>>>>>>> + "adapter memory (resource not found)!\n", ioc->name); >>>>>>>>>>>> + r = -EINVAL; >>>>>>>>>>>> + goto out_fail; >>>>>>>>>>>> + } >>>>>>>>>>>> + >>>>>>>>>>>> _base_mask_interrupts(ioc); >>>>>>>>>>>> >>>>>>>>>>>> r = _base_get_ioc_facts(ioc, CAN_SLEEP); >>>>>>>>>>> >>>>>>>>>>> Just following up on this patch as I have not yet received any >>>>>>>>>>> response. >>>>>>>>>>> >>>>>>>>>>> Thanks! >>>>>>>>>> >>>>>>>>>> Hi Tim -- just curious, why was the similar check on ioc->chip just a >>>>>>>>>> few lines above the one added by the patch insufficient? >>>>>>>>>> >>>>>>>>>> That loop block sets memap_sz when it finds an IORESOURCE_MEM so that it >>>>>>>>>> only sets ioc->chip once. I wonder if the fix might be simpler if the >>>>>>>>>> existing ioc->chip check relocated entirely to where you put it (maybe >>>>>>>>>> also pulling the entire error text onto one line for easier grepping). >>>>>>>>>> >>>>>>>>>> Regards, >>>>>>>>>> >>>>>>>>>> -- Joe >>>>>>>>> >>>>>>>>> If there are no IORESOURCE_MEM resources allocated by the BIOS (i.e. if >>>>>>>>> the BIOS does not run resource allocation on the mpt2sas device) then >>>>>>>>> the check you are referring to is not executed, and the driver attempts >>>>>>>>> to perform operations on a null ioc->chip pointer. >>>>>>>>> >>>>>>>>> I can relocate the check if desired. >>>>>>>>> >>>>>>>> >>>>>>>> On looking more closely at the code I think having the two separate >>>>>>>> checks is useful for debugging. The first error message is triggered if >>>>>>>> the resource exists but Linux cannot map it for some reason, and the >>>>>>>> second error message is triggered if the resource does not exist at all >>>>>>>> (buggy BIOS). >>>>>>> >>>>>>> Fair enough. The two error messages are unique, so grepping will lead >>>>>>> to the correct check. >>>>>>> >>>>>>> Only other nits would be the addition of a copyright (up to Avago I >>>>>>> guess) and an mpt3sas version (these drivers are 99% the same code). >>>>>>> >>>>>>> Acked-by: Joe Lawrence<joe.lawrence@stratus.com> >>>>>>> >>>>>>> -- Joe >>>>>> >>>>>> >>>>>> >>>>> >>>> >>>> Thank you for clarifying the guidelines on copyright lines. I wasn't >>>> sure if this was significant enough of a contribution to justify the >>>> line or not. >>>> >>>> Do I need to re-send or can I just state here that the patch can be >>>> merged without the copyright line? >>> >>> It's easier if you send a clean copy rather than risk me botching >>> something in the hand edit. >> >> OK, will do. >> >>>> I don't have any mpt3sas hardware to test with; should I just send in a >>>> patch anyway without a Tested-by line? >>> >>> Well, if you haven't actually tested it, adding a tested-by line >>> wouldn't be correct. >> >> Of course not. My question was, should I send in a patch anyway (with >> appropriate commit message fields) even though it has not been tested? > > You mean do you need to retest for a non-material change to a patch > still to have a tested-by on it? Ideally, yes, but as long as there are > no actual code changes, I don't think it's required. > > James I guess I'm being somewhat confusing today, my apologies for that. I understand there is no real need to re-test when removing the copyright line for the mpt2sas driver patch. My question is related to your earlier comment about the mpt3sas driver, which apparently uses mostly the same codebase as the mpt2sas driver. Specifically, when referring to the mpt3sas driver (which I cannot directly test), should I send in an untested patch for the mpt3sas driver, which would be based on the tested / working patch for the mpt2sas driver? Thanks!
On Tue, 2015-06-23 at 12:59 -0500, Timothy Pearson wrote: > On 06/23/2015 12:56 PM, James Bottomley wrote: > > On Tue, 2015-06-23 at 12:47 -0500, Timothy Pearson wrote: > >> On 06/23/2015 12:45 PM, James Bottomley wrote: > >>> On Tue, 2015-06-23 at 12:33 -0500, Timothy Pearson wrote: > >>>> On 06/23/2015 08:35 AM, James Bottomley wrote: > >>>>> On Tue, 2015-06-23 at 18:06 +0530, Sreekanth Reddy wrote: > >>>>>> Hi, > >>>>>> > >>>>>> Upstream contributor should not add copyright to this driver code. > >>>>> > >>>>> I can't agree with that as a general rule: it depends on the > >>>>> significance of the contribution. The somewhat ill defined standard for > >>>>> this is the contribution must be "an original work of authorship". > >>>>> > >>>>> I can agree that a trivial bug fix like this is not an original work of > >>>>> authorship, so in this case adding copyright isn't appropriate. > >>>>> > >>>>> James > >>>>> > >>>>>> Regards, > >>>>>> Sreekanth > >>>>>> > >>>>>> On Tue, Jun 23, 2015 at 9:24 AM, Joe Lawrence<joe.lawrence@stratus.com> wrote: > >>>>>>> > >>>>>>> > >>>>>>> On 06/21/2015 02:46 PM, Timothy Pearson wrote: > >>>>>>>> On 06/16/2015 01:49 PM, Timothy Pearson wrote: > >>>>>>>>> On 06/16/2015 12:42 PM, Joe Lawrence wrote: > >>>>>>>>>> On 06/16/2015 12:28 PM, Timothy Pearson wrote: > >>>>>>>>>>> On 06/12/2015 05:05 PM, Timothy Pearson wrote: > >>>>>>>>>>>> The mpt2sas driver crashes if the BIOS does not set up at least one > >>>>>>>>>>>> memory I/O resource. This failure can happen if the device is too > >>>>>>>>>>>> slow to respond during POST and is missed by the BIOS, but Linux > >>>>>>>>>>>> then detects the device later in the boot process. > >>>>>>>>>>>> > >>>>>>>>>>>> This patch aborts initialization and prints a warning if no memory I/O > >>>>>>>>>>>> resources are found. > >>>>>>>>>>>> > >>>>>>>>>>>> Signed-off-by: Timothy Pearson<tpearson@raptorengineeringinc.com> > >>>>>>>>>>>> Tested-by: Timothy Pearson<tpearson@raptorengineeringinc.com> > >>>>>>>>>>>> --- > >>>>>>>>>>>> drivers/scsi/mpt2sas/mpt2sas_base.c | 9 +++++++++ > >>>>>>>>>>>> 1 file changed, 9 insertions(+) > >>>>>>>>>>>> > >>>>>>>>>>>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c > >>>>>>>>>>>> b/drivers/scsi/mpt2sas/mpt2sas_base.c > >>>>>>>>>>>> index 11248de..15c9504 100644 > >>>>>>>>>>>> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c > >>>>>>>>>>>> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c > >>>>>>>>>>>> @@ -6,6 +6,8 @@ > >>>>>>>>>>>> * Copyright (C) 2007-2014 LSI Corporation > >>>>>>>>>>>> * Copyright (C) 20013-2014 Avago Technologies > >>>>>>>>>>>> * (mailto: MPT-FusionLinux.pdl@avagotech.com) > >>>>>>>>>>>> + * Copyright (C) 2015 Raptor Engineering > >>>>>>>>>>>> + * (mailto: support@araptorengineeringinc.com) > >>>>>>>>>>>> * > >>>>>>>>>>>> * This program is free software; you can redistribute it and/or > >>>>>>>>>>>> * modify it under the terms of the GNU General Public License > >>>>>>>>>>>> @@ -1582,6 +1584,13 @@ mpt2sas_base_map_resources(struct > >>>>>>>>>>>> MPT2SAS_ADAPTER > >>>>>>>>>>>> *ioc) > >>>>>>>>>>>> } > >>>>>>>>>>>> } > >>>>>>>>>>>> > >>>>>>>>>>>> + if (ioc->chip == NULL) { > >>>>>>>>>>>> + printk(MPT2SAS_ERR_FMT "unable to map " > >>>>>>>>>>>> + "adapter memory (resource not found)!\n", ioc->name); > >>>>>>>>>>>> + r = -EINVAL; > >>>>>>>>>>>> + goto out_fail; > >>>>>>>>>>>> + } > >>>>>>>>>>>> + > >>>>>>>>>>>> _base_mask_interrupts(ioc); > >>>>>>>>>>>> > >>>>>>>>>>>> r = _base_get_ioc_facts(ioc, CAN_SLEEP); > >>>>>>>>>>> > >>>>>>>>>>> Just following up on this patch as I have not yet received any > >>>>>>>>>>> response. > >>>>>>>>>>> > >>>>>>>>>>> Thanks! > >>>>>>>>>> > >>>>>>>>>> Hi Tim -- just curious, why was the similar check on ioc->chip just a > >>>>>>>>>> few lines above the one added by the patch insufficient? > >>>>>>>>>> > >>>>>>>>>> That loop block sets memap_sz when it finds an IORESOURCE_MEM so that it > >>>>>>>>>> only sets ioc->chip once. I wonder if the fix might be simpler if the > >>>>>>>>>> existing ioc->chip check relocated entirely to where you put it (maybe > >>>>>>>>>> also pulling the entire error text onto one line for easier grepping). > >>>>>>>>>> > >>>>>>>>>> Regards, > >>>>>>>>>> > >>>>>>>>>> -- Joe > >>>>>>>>> > >>>>>>>>> If there are no IORESOURCE_MEM resources allocated by the BIOS (i.e. if > >>>>>>>>> the BIOS does not run resource allocation on the mpt2sas device) then > >>>>>>>>> the check you are referring to is not executed, and the driver attempts > >>>>>>>>> to perform operations on a null ioc->chip pointer. > >>>>>>>>> > >>>>>>>>> I can relocate the check if desired. > >>>>>>>>> > >>>>>>>> > >>>>>>>> On looking more closely at the code I think having the two separate > >>>>>>>> checks is useful for debugging. The first error message is triggered if > >>>>>>>> the resource exists but Linux cannot map it for some reason, and the > >>>>>>>> second error message is triggered if the resource does not exist at all > >>>>>>>> (buggy BIOS). > >>>>>>> > >>>>>>> Fair enough. The two error messages are unique, so grepping will lead > >>>>>>> to the correct check. > >>>>>>> > >>>>>>> Only other nits would be the addition of a copyright (up to Avago I > >>>>>>> guess) and an mpt3sas version (these drivers are 99% the same code). > >>>>>>> > >>>>>>> Acked-by: Joe Lawrence<joe.lawrence@stratus.com> > >>>>>>> > >>>>>>> -- Joe > >>>>>> > >>>>>> > >>>>>> > >>>>> > >>>> > >>>> Thank you for clarifying the guidelines on copyright lines. I wasn't > >>>> sure if this was significant enough of a contribution to justify the > >>>> line or not. > >>>> > >>>> Do I need to re-send or can I just state here that the patch can be > >>>> merged without the copyright line? > >>> > >>> It's easier if you send a clean copy rather than risk me botching > >>> something in the hand edit. > >> > >> OK, will do. > >> > >>>> I don't have any mpt3sas hardware to test with; should I just send in a > >>>> patch anyway without a Tested-by line? > >>> > >>> Well, if you haven't actually tested it, adding a tested-by line > >>> wouldn't be correct. > >> > >> Of course not. My question was, should I send in a patch anyway (with > >> appropriate commit message fields) even though it has not been tested? > > > > You mean do you need to retest for a non-material change to a patch > > still to have a tested-by on it? Ideally, yes, but as long as there are > > no actual code changes, I don't think it's required. > > > > James > > I guess I'm being somewhat confusing today, my apologies for that. I > understand there is no real need to re-test when removing the copyright > line for the mpt2sas driver patch. > > My question is related to your earlier comment about the mpt3sas driver, > which apparently uses mostly the same codebase as the mpt2sas driver. > Specifically, when referring to the mpt3sas driver (which I cannot > directly test), should I send in an untested patch for the mpt3sas > driver, which would be based on the tested / working patch for the > mpt2sas driver? You can send untested patches, yes. The maintainer will review them and decide on inclusion. James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c index 11248de..15c9504 100644 --- a/drivers/scsi/mpt2sas/mpt2sas_base.c +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c @@ -6,6 +6,8 @@ * Copyright (C) 2007-2014 LSI Corporation * Copyright (C) 20013-2014 Avago Technologies * (mailto: MPT-FusionLinux.pdl@avagotech.com) + * Copyright (C) 2015 Raptor Engineering + * (mailto: support@araptorengineeringinc.com) * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -1582,6 +1584,13 @@ mpt2sas_base_map_resources(struct MPT2SAS_ADAPTER *ioc) } } + if (ioc->chip == NULL) { + printk(MPT2SAS_ERR_FMT "unable to map " + "adapter memory (resource not found)!\n", ioc->name); + r = -EINVAL; + goto out_fail; + } + _base_mask_interrupts(ioc);