diff mbox series

[SRU,F,G,v2,1/1] s390/pci: Fix zpci_alloc_domain() over allocation

Message ID 20200505102209.881344-2-frank.heimes@canonical.com
State New
Headers show
Series [SRU,F,G,v2,1/1] s390/pci: Fix zpci_alloc_domain() over allocation | expand

Commit Message

Frank Heimes May 5, 2020, 10:22 a.m. UTC 
From: Niklas Schnelle <schnelle@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@linux.ibm.com>
OriginalAuthor: Niklas Schnelle <schnelle@linux.ibm.com>
(backported from commit 969ae01bab2fe938b4c8324836038b5ac1c78fac)
Signed-off-by: Frank Heimes <frank.heimes@canonical.com>
---
 arch/s390/include/asm/pci.h |  1 +
 arch/s390/pci/pci.c         | 34 ++++++++++++++++++++--------------
 2 files changed, 21 insertions(+), 14 deletions(-)

Comments

Kleber Sacilotto de Souza May 5, 2020, 10:34 a.m. UTC | #1 
On 05.05.20 12:22, frank.heimes@canonical.com wrote:
> From: Niklas Schnelle <schnelle@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@linux.ibm.com>
> OriginalAuthor: Niklas Schnelle <schnelle@linux.ibm.com>
> (backported from commit 969ae01bab2fe938b4c8324836038b5ac1c78fac)
> Signed-off-by: Frank Heimes <frank.heimes@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@linux.ibm.com>
    Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
    Signed-off-by: Vasily Gorbik <gor@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);
>  }
>  
>
Frank Heimes May 6, 2020, 6:32 a.m. UTC | #2 
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.)
- 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@linux.ibm.com>
     Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
     Signed-off-by: Vasily Gorbik <gor@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.
- 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@virtuousgeek.org>
     Tested-by: "Paulo J. S. Silva" <pjssilva@gmail.com>
     Signed-off-by: Eric Anholt <eric@anholt.net>
     *Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de <gregkh@suse.de>>*
  but the Wiki example just lists:
     *OriginalAuthor: Jesse Barnes <jbarnes@virtuousgeek.org
<jbarnes@virtuousgeek.org>>  *
     Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
     Tested-by: "Paulo J. S. Silva" <pjssilva@gmail.com>
     Signed-off-by: Eric Anholt <eric@anholt.net>
     (backported from commit c9fcc5d269949a0fbd46ffbea6cc83741e61c05f)
     [ bjf: context adjustments. ]
     Acked-by: Brad Figg <brad.figg@canonical.com>
     Acked-by: Steve Conklin <sconklin@canonical.com>
     Signed-off-by: Steve Conklin <sconklin@canonical.com>
  Hence even here the entire provenance is *not* fully listed, since
'Signed-off-by: Greg Kroah-Hartman <gregkh@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@virtuousgeek.org>

Maybe I misinterpreted the wiki or it became a bit outdated in between ...

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.

Bye, Frank

On Tue, May 5, 2020 at 12:34 PM Kleber Souza <kleber.souza@canonical.com>
wrote:

> On 05.05.20 12:22, frank.heimes@canonical.com wrote:
> > From: Niklas Schnelle <schnelle@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@linux.ibm.com>
> > OriginalAuthor: Niklas Schnelle <schnelle@linux.ibm.com>
> > (backported from commit 969ae01bab2fe938b4c8324836038b5ac1c78fac)
> > Signed-off-by: Frank Heimes <frank.heimes@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@linux.ibm.com>
>     Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
>     Signed-off-by: Vasily Gorbik <gor@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);
> >  }
> >
> >
>
>
Kleber Sacilotto de Souza May 14, 2020, 9:10 a.m. UTC | #3 
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@linux.ibm.com <mailto:schnelle@linux.ibm.com>>
>      Reviewed-by: Pierre Morel <pmorel@linux.ibm.com <mailto:pmorel@linux.ibm.com>>
>      Signed-off-by: Vasily Gorbik <gor@linux.ibm.com <mailto:gor@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.

> - 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@virtuousgeek.org <mailto:jbarnes@virtuousgeek.org>>
>      Tested-by: "Paulo J. S. Silva" <pjssilva@gmail.com <mailto:pjssilva@gmail.com>>
>      Signed-off-by: Eric Anholt <eric@anholt.net <mailto:eric@anholt.net>>
>      *Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de <mailto:gregkh@suse.de>>*
>   but the Wiki example just lists:
>      *OriginalAuthor: Jesse Barnes <jbarnes@virtuousgeek.org <mailto:jbarnes@virtuousgeek.org>>  *
>      Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org <mailto:jbarnes@virtuousgeek.org>>  
>      Tested-by: "Paulo J. S. Silva" <pjssilva@gmail.com <mailto:pjssilva@gmail.com>>  
>      Signed-off-by: Eric Anholt <eric@anholt.net <mailto:eric@anholt.net>>
>      (backported from commit c9fcc5d269949a0fbd46ffbea6cc83741e61c05f)
>      [ bjf: context adjustments. ]
>      Acked-by: Brad Figg <brad.figg@canonical.com <mailto:brad.figg@canonical.com>>
>      Acked-by: Steve Conklin <sconklin@canonical.com <mailto:sconklin@canonical.com>>
>      Signed-off-by: Steve Conklin <sconklin@canonical.com <mailto:sconklin@canonical.com>>
>   Hence even here the entire provenance is *not* fully listed, since 'Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de <mailto:gregkh@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@virtuousgeek.org <mailto:jbarnes@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@canonical.com <mailto:kleber.souza@canonical.com>> wrote:
> 
>     On 05.05.20 12:22, frank.heimes@canonical.com <mailto:frank.heimes@canonical.com> wrote:
>     > From: Niklas Schnelle <schnelle@linux.ibm.com <mailto:schnelle@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@linux.ibm.com <mailto:schnelle@linux.ibm.com>>
>     > OriginalAuthor: Niklas Schnelle <schnelle@linux.ibm.com <mailto:schnelle@linux.ibm.com>>
>     > (backported from commit 969ae01bab2fe938b4c8324836038b5ac1c78fac)
>     > Signed-off-by: Frank Heimes <frank.heimes@canonical.com <mailto:frank.heimes@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@linux.ibm.com <mailto:schnelle@linux.ibm.com>>
>         Reviewed-by: Pierre Morel <pmorel@linux.ibm.com <mailto:pmorel@linux.ibm.com>>
>         Signed-off-by: Vasily Gorbik <gor@linux.ibm.com <mailto:gor@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);
>     >  }
>     > 
>     >
>
Stefan Bader May 14, 2020, 9:18 a.m. UTC | #4 
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@linux.ibm.com <mailto:schnelle@linux.ibm.com>>
>>      Reviewed-by: Pierre Morel <pmorel@linux.ibm.com <mailto:pmorel@linux.ibm.com>>
>>      Signed-off-by: Vasily Gorbik <gor@linux.ibm.com <mailto:gor@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@virtuousgeek.org <mailto:jbarnes@virtuousgeek.org>>
>>      Tested-by: "Paulo J. S. Silva" <pjssilva@gmail.com <mailto:pjssilva@gmail.com>>
>>      Signed-off-by: Eric Anholt <eric@anholt.net <mailto:eric@anholt.net>>
>>      *Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de <mailto:gregkh@suse.de>>*
>>   but the Wiki example just lists:
>>      *OriginalAuthor: Jesse Barnes <jbarnes@virtuousgeek.org <mailto:jbarnes@virtuousgeek.org>>  *
>>      Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org <mailto:jbarnes@virtuousgeek.org>>  
>>      Tested-by: "Paulo J. S. Silva" <pjssilva@gmail.com <mailto:pjssilva@gmail.com>>  
>>      Signed-off-by: Eric Anholt <eric@anholt.net <mailto:eric@anholt.net>>
>>      (backported from commit c9fcc5d269949a0fbd46ffbea6cc83741e61c05f)
>>      [ bjf: context adjustments. ]
>>      Acked-by: Brad Figg <brad.figg@canonical.com <mailto:brad.figg@canonical.com>>
>>      Acked-by: Steve Conklin <sconklin@canonical.com <mailto:sconklin@canonical.com>>
>>      Signed-off-by: Steve Conklin <sconklin@canonical.com <mailto:sconklin@canonical.com>>
>>   Hence even here the entire provenance is *not* fully listed, since 'Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de <mailto:gregkh@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@virtuousgeek.org <mailto:jbarnes@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@canonical.com <mailto:kleber.souza@canonical.com>> wrote:
>>
>>     On 05.05.20 12:22, frank.heimes@canonical.com <mailto:frank.heimes@canonical.com> wrote:
>>     > From: Niklas Schnelle <schnelle@linux.ibm.com <mailto:schnelle@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@linux.ibm.com <mailto:schnelle@linux.ibm.com>>
>>     > OriginalAuthor: Niklas Schnelle <schnelle@linux.ibm.com <mailto:schnelle@linux.ibm.com>>
>>     > (backported from commit 969ae01bab2fe938b4c8324836038b5ac1c78fac)
>>     > Signed-off-by: Frank Heimes <frank.heimes@canonical.com <mailto:frank.heimes@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@linux.ibm.com <mailto:schnelle@linux.ibm.com>>
>>         Reviewed-by: Pierre Morel <pmorel@linux.ibm.com <mailto:pmorel@linux.ibm.com>>
>>         Signed-off-by: Vasily Gorbik <gor@linux.ibm.com <mailto:gor@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);
>>     >  }
>>     > 
>>     >
>>
>
diff mbox series

Patch

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);
 }