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@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxx>
> >>>>>>>> Tested-by: Timothy Pearson<tpearson@xxxxxxxxxxxxxxxxxxxxxxxx>
> >>>>>>>> ---
> >>>>>>>> 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@xxxxxxxxxxxxx)
> >>>>>>>> + * Copyright (C) 2015 Raptor Engineering
> >>>>>>>> + * (mailto: support@xxxxxxxxxxxxxxxxxxxxxxxxx)
> >>>>>>>> *
> >>>>>>>> * 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@xxxxxxxxxxx>
> >>>
> >>> -- 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
