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@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.
>
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
