NAK: [SRU][F][G][PATCH v2 1/1] s390/pci: Fix zpci_alloc_domain() over allocation
Stefan Bader
stefan.bader at canonical.com
Thu May 14 09:18:43 UTC 2020
On 14.05.20 11:10, Kleber Souza wrote:
> Hi Frank,
>
> Thank you for your reply. Some comments below.
>
> On 06.05.20 08:32, Frank Heimes wrote:
>> Hi Kleber,
>> well, I thought about that, and I can add of course the entire provenance block of the base commit to backport, but:
>>
>> - I strictly added the name(s) to the provevance block to this SRU, that are mentioned in the backport (actually git tooling does that automatically),
>> since I think that the people who signed the base commit only signed the base commit, and not the backport (they are probably even not aware of the backport).
>> (I always did it this way in the past.)
>
> It's a common practice to keep the whole provenance block with the S-O-B's and ACK's when doing a
> backport. The original author and people involved don't need to be aware of the changes that are
> made afterwards to the patches, but we need to give credit to the people involved on the original
> version of the code.
>
>> - The backport is coming from IBM, means I have to (kind of) modify the backport with add. provenance info - which didn't seem to be right to me.
>> - Hence I thought that adding the provenance block from the base commit (like you asked for):
>> Signed-off-by: Niklas Schnelle <schnelle at linux.ibm.com <mailto:schnelle at linux.ibm.com>>
>> Reviewed-by: Pierre Morel <pmorel at linux.ibm.com <mailto:pmorel at linux.ibm.com>>
>> Signed-off-by: Vasily Gorbik <gor at linux.ibm.com <mailto:gor at linux.ibm.com>>
>> to the SRU for the backport may lead to the assumption that these people also signed-off / reviewed the backport itself, which is obviously not the case.
>> Hence I (personally) think this would be missleading information.
>
> Maybe we need to agree on a formal procedure to get patches from the customers. IMO the person
> providing the backport needs to add their S-O-B as well keeping the original block.
>
> I'm adding Stefan on CC to see what he thinks about it.
All the SOBs and reviewed by would be above your backported from and thus there
is no confusion about it. You add your sob after the backport and we add things
below there. It is just a continuous record of history.
>
>> - And I want to strictly follow to the StablePatchFormat documentation: https://wiki.ubuntu.com/Kernel/Dev/StablePatchFormat
>> In the 'Complete Example" at the end a backport of (the real world) commit "c9fcc5d269949a0fbd46ffbea6cc83741e61c05f" is given.
>> If one looks-up that commit, the (upstream / base) provenance is listed like this:
>> Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org <mailto:jbarnes at virtuousgeek.org>>
>> Tested-by: "Paulo J. S. Silva" <pjssilva at gmail.com <mailto:pjssilva at gmail.com>>
>> Signed-off-by: Eric Anholt <eric at anholt.net <mailto:eric at anholt.net>>
>> *Signed-off-by: Greg Kroah-Hartman <gregkh at suse.de <mailto:gregkh at suse.de>>*
>> but the Wiki example just lists:
>> *OriginalAuthor: Jesse Barnes <jbarnes at virtuousgeek.org <mailto:jbarnes at virtuousgeek.org>> *
>> Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org <mailto:jbarnes at virtuousgeek.org>>
>> Tested-by: "Paulo J. S. Silva" <pjssilva at gmail.com <mailto:pjssilva at gmail.com>>
>> Signed-off-by: Eric Anholt <eric at anholt.net <mailto:eric at anholt.net>>
>> (backported from commit c9fcc5d269949a0fbd46ffbea6cc83741e61c05f)
>> [ bjf: context adjustments. ]
>> Acked-by: Brad Figg <brad.figg at canonical.com <mailto:brad.figg at canonical.com>>
>> Acked-by: Steve Conklin <sconklin at canonical.com <mailto:sconklin at canonical.com>>
>> Signed-off-by: Steve Conklin <sconklin at canonical.com <mailto:sconklin at canonical.com>>
>> Hence even here the entire provenance is *not* fully listed, since 'Signed-off-by: Greg Kroah-Hartman <gregkh at suse.de <mailto:gregkh at suse.de>>' is missing.
>> - For me the fact that the line "(backported from commit c9fcc5d269949a0fbd46ffbea6cc83741e61c05f)" is added,
>> allows everybody to look up the original commit and the provenance of that.
>> - And btw. this example is also the reason why I added a line about the OriginalAuthor.
>> OriginalAuthor: Jesse Barnes <jbarnes at virtuousgeek.org <mailto:jbarnes at virtuousgeek.org>>
>>
>> Maybe I misinterpreted the wiki or it became a bit outdated in between ...
>
> I see where you are coming from and sorry to make you re-work the patch.
>
> I believe the example on the wiki page is outdated. We have not been changing the "From:"
> header of the email and we keep the email of the original author, hence we don't use the
> "OriginalAuthor:" tag anymore.
>
> I'll talk to the team to have it updated.
>
>>
>> Anyway, I'm going to submit a v3 with the entire provenance, which means also manually copying over the provenance from the base commit to the backport, and will drop the OriginalAuthor line.
>
> Thank you!
>
> Kleber
>
>>
>> Bye, Frank
>>
>> On Tue, May 5, 2020 at 12:34 PM Kleber Souza <kleber.souza at canonical.com <mailto:kleber.souza at canonical.com>> wrote:
>>
>> On 05.05.20 12:22, frank.heimes at canonical.com <mailto:frank.heimes at canonical.com> wrote:
>> > From: Niklas Schnelle <schnelle at linux.ibm.com <mailto:schnelle at linux.ibm.com>>
>> >
>> > BugLink: https://bugs.launchpad.net/bugs/1874057
>> >
>> > Until now zpci_alloc_domain() only prevented more than
>> > CONFIG_PCI_NR_FUNCTIONS from being added when using automatic domain
>> > allocation. When explicit UIDs were defined UIDs above
>> > CONFIG_PCI_NR_FUNCTIONS were not counted at all.
>> > When more PCI functions are added this could lead to various errors
>> > including under sized IRQ vectors and similar issues.
>> >
>> > Fix this by explicitly tracking the number of allocated domains.
>> >
>> > Signed-off-by: Niklas Schnelle <schnelle at linux.ibm.com <mailto:schnelle at linux.ibm.com>>
>> > OriginalAuthor: Niklas Schnelle <schnelle at linux.ibm.com <mailto:schnelle at linux.ibm.com>>
>> > (backported from commit 969ae01bab2fe938b4c8324836038b5ac1c78fac)
>> > Signed-off-by: Frank Heimes <frank.heimes at canonical.com <mailto:frank.heimes at canonical.com>>
>>
>> Hi Frank,
>>
>> When we backport/cherry-pick patches from upstream or any other external
>> tree we need to keep the whole provenance block from the original commit
>> and add our tags below it. Looking at commit 969ae01bab2f, the original
>> provenance block is:
>>
>> Signed-off-by: Niklas Schnelle <schnelle at linux.ibm.com <mailto:schnelle at linux.ibm.com>>
>> Reviewed-by: Pierre Morel <pmorel at linux.ibm.com <mailto:pmorel at linux.ibm.com>>
>> Signed-off-by: Vasily Gorbik <gor at linux.ibm.com <mailto:gor at linux.ibm.com>>
>>
>> All of this needs to be kept, adding the '(backported from ...)' and
>> your s-o-b lines below them. We don't need to add the 'OriginalAuthor:'
>> tag as git will automatically create the commit with the author set
>> to the name/email from the "From:" tag added as first line of the
>> patch.
>>
>>
>> Thanks,
>> Kleber
>>
>>
>> > ---
>> > arch/s390/include/asm/pci.h | 1 +
>> > arch/s390/pci/pci.c | 34 ++++++++++++++++++++--------------
>> > 2 files changed, 21 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
>> > index 6087a4e9b2bf..7850e8c8c79a 100644
>> > --- a/arch/s390/include/asm/pci.h
>> > +++ b/arch/s390/include/asm/pci.h
>> > @@ -28,6 +28,7 @@ int pci_proc_domain(struct pci_bus *);
>> >
>> > #define ZPCI_NR_DMA_SPACES 1
>> > #define ZPCI_NR_DEVICES CONFIG_PCI_NR_FUNCTIONS
>> > +#define ZPCI_DOMAIN_BITMAP_SIZE (1 << 16)
>> >
>> > /* PCI Function Controls */
>> > #define ZPCI_FC_FN_ENABLED 0x80
>> > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
>> > index 6105b1b6e49b..0af46683dd66 100644
>> > --- a/arch/s390/pci/pci.c
>> > +++ b/arch/s390/pci/pci.c
>> > @@ -39,8 +39,9 @@
>> > static LIST_HEAD(zpci_list);
>> > static DEFINE_SPINLOCK(zpci_list_lock);
>> >
>> > -static DECLARE_BITMAP(zpci_domain, ZPCI_NR_DEVICES);
>> > +static DECLARE_BITMAP(zpci_domain, ZPCI_DOMAIN_BITMAP_SIZE);
>> > static DEFINE_SPINLOCK(zpci_domain_lock);
>> > +static unsigned int zpci_num_domains_allocated;
>> >
>> > #define ZPCI_IOMAP_ENTRIES \
>> > min(((unsigned long) ZPCI_NR_DEVICES * PCI_BAR_COUNT / 2), \
>> > @@ -651,39 +652,44 @@ struct dev_pm_ops pcibios_pm_ops = {
>> >
>> > static int zpci_alloc_domain(struct zpci_dev *zdev)
>> > {
>> > + spin_lock(&zpci_domain_lock);
>> > + if (zpci_num_domains_allocated > (ZPCI_NR_DEVICES - 1)) {
>> > + spin_unlock(&zpci_domain_lock);
>> > + pr_err("Adding PCI function %08x failed because the configured limit of %d is reached\n",
>> > + zdev->fid, ZPCI_NR_DEVICES);
>> > + return -ENOSPC;
>> > + }
>> > +
>> > if (zpci_unique_uid) {
>> > zdev->domain = (u16) zdev->uid;
>> > - if (zdev->domain >= ZPCI_NR_DEVICES)
>> > - return 0;
>> > -
>> > - spin_lock(&zpci_domain_lock);
>> > if (test_bit(zdev->domain, zpci_domain)) {
>> > spin_unlock(&zpci_domain_lock);
>> > + pr_err("Adding PCI function %08x failed because domain %04x is already assigned\n",
>> > + zdev->fid, zdev->domain);
>> > return -EEXIST;
>> > }
>> > set_bit(zdev->domain, zpci_domain);
>> > + zpci_num_domains_allocated++;
>> > spin_unlock(&zpci_domain_lock);
>> > return 0;
>> > }
>> > -
>> > - spin_lock(&zpci_domain_lock);
>> > + /*
>> > + * We can always auto allocate domains below ZPCI_NR_DEVICES.
>> > + * There is either a free domain or we have reached the maximum in
>> > + * which case we would have bailed earlier.
>> > + */
>> > zdev->domain = find_first_zero_bit(zpci_domain, ZPCI_NR_DEVICES);
>> > - if (zdev->domain == ZPCI_NR_DEVICES) {
>> > - spin_unlock(&zpci_domain_lock);
>> > - return -ENOSPC;
>> > - }
>> > set_bit(zdev->domain, zpci_domain);
>> > + zpci_num_domains_allocated++;
>> > spin_unlock(&zpci_domain_lock);
>> > return 0;
>> > }
>> >
>> > static void zpci_free_domain(struct zpci_dev *zdev)
>> > {
>> > - if (zdev->domain >= ZPCI_NR_DEVICES)
>> > - return;
>> > -
>> > spin_lock(&zpci_domain_lock);
>> > clear_bit(zdev->domain, zpci_domain);
>> > + zpci_num_domains_allocated--;
>> > spin_unlock(&zpci_domain_lock);
>> > }
>> >
>> >
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20200514/00b5854c/attachment.sig>
More information about the kernel-team
mailing list