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