Re: [PATCH v3 06/26] s390/pci: Use PCI_STD_NUM_BARS

[Bjorn Helgaas]

On Wed, Sep 18, 2019 at 09:58:06AM +0100, Andrew Murray wrote:
> On Mon, Sep 16, 2019 at 11:41:38PM +0300, Denis Efremov wrote:
> > Remove local definition PCI_BAR_COUNT for the number of PCI BARs and use
> > global one PCI_STD_NUM_BARS instead.
> > 
> > Acked-by: Sebastian Ott <sebott@xxxxxxxxxxxxx>
> > Cc: Gerald Schaefer <gerald.schaefer@xxxxxxxxxx>
> > Signed-off-by: Denis Efremov <efremov@xxxxxxxxx>
> > ---
> >  arch/s390/include/asm/pci.h     |  5 +----
> >  arch/s390/include/asm/pci_clp.h |  6 +++---
> >  arch/s390/pci/pci.c             | 16 ++++++++--------
> >  arch/s390/pci/pci_clp.c         |  6 +++---
> >  4 files changed, 15 insertions(+), 18 deletions(-)
> > 
> > diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> > index a2399eff84ca..3a06c264ea53 100644
> > --- a/arch/s390/include/asm/pci.h
> > +++ b/arch/s390/include/asm/pci.h
> > @@ -2,9 +2,6 @@
> >  #ifndef __ASM_S390_PCI_H
> >  #define __ASM_S390_PCI_H
> >  
> > -/* must be set before including pci_clp.h */
> > -#define PCI_BAR_COUNT	6
> > -
> >  #include <linux/pci.h>
> >  #include <linux/mutex.h>
> >  #include <linux/iommu.h>
> > @@ -138,7 +135,7 @@ struct zpci_dev {
> >  
> >  	char res_name[16];
> >  	bool mio_capable;
> > -	struct zpci_bar_struct bars[PCI_BAR_COUNT];
> > +	struct zpci_bar_struct bars[PCI_STD_NUM_BARS];
> >  
> >  	u64		start_dma;	/* Start of available DMA addresses */
> >  	u64		end_dma;	/* End of available DMA addresses */
> > diff --git a/arch/s390/include/asm/pci_clp.h b/arch/s390/include/asm/pci_clp.h
> > index 50359172cc48..bd2cb4ea7d93 100644
> > --- a/arch/s390/include/asm/pci_clp.h
> > +++ b/arch/s390/include/asm/pci_clp.h
> > @@ -77,7 +77,7 @@ struct mio_info {
> >  	struct {
> >  		u64 wb;
> >  		u64 wt;
> > -	} addr[PCI_BAR_COUNT];
> > +	} addr[PCI_STD_NUM_BARS];
> >  	u32 reserved[6];
> >  } __packed;
> >  
> > @@ -98,9 +98,9 @@ struct clp_rsp_query_pci {
> >  	u16 util_str_avail	:  1;	/* utility string available? */
> >  	u16 pfgid		:  8;	/* pci function group id */
> >  	u32 fid;			/* pci function id */
> > -	u8 bar_size[PCI_BAR_COUNT];
> > +	u8 bar_size[PCI_STD_NUM_BARS];
> >  	u16 pchid;
> > -	__le32 bar[PCI_BAR_COUNT];
> > +	__le32 bar[PCI_STD_NUM_BARS];
> >  	u8 pfip[CLP_PFIP_NR_SEGMENTS];	/* pci function internal path */
> >  	u32			: 16;
> >  	u8 fmb_len;
> > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> > index b0e3b9a0e488..aca372c8e34f 100644
> > --- a/arch/s390/pci/pci.c
> > +++ b/arch/s390/pci/pci.c
> > @@ -43,7 +43,7 @@ static DECLARE_BITMAP(zpci_domain, ZPCI_NR_DEVICES);
> >  static DEFINE_SPINLOCK(zpci_domain_lock);
> >  
> >  #define ZPCI_IOMAP_ENTRIES						\
> > -	min(((unsigned long) ZPCI_NR_DEVICES * PCI_BAR_COUNT / 2),	\
> > +	min(((unsigned long) ZPCI_NR_DEVICES * PCI_STD_NUM_BARS / 2),	\
> >  	    ZPCI_IOMAP_MAX_ENTRIES)
> >  
> >  static DEFINE_SPINLOCK(zpci_iomap_lock);
> > @@ -294,7 +294,7 @@ static void __iomem *pci_iomap_range_mio(struct pci_dev *pdev, int bar,
> >  void __iomem *pci_iomap_range(struct pci_dev *pdev, int bar,
> >  			      unsigned long offset, unsigned long max)
> >  {
> > -	if (!pci_resource_len(pdev, bar) || bar >= PCI_BAR_COUNT)
> > +	if (bar >= PCI_STD_NUM_BARS || !pci_resource_len(pdev, bar))
> >  		return NULL;
> >  
> >  	if (static_branch_likely(&have_mio))
> > @@ -324,7 +324,7 @@ static void __iomem *pci_iomap_wc_range_mio(struct pci_dev *pdev, int bar,
> >  void __iomem *pci_iomap_wc_range(struct pci_dev *pdev, int bar,
> >  				 unsigned long offset, unsigned long max)
> >  {
> > -	if (!pci_resource_len(pdev, bar) || bar >= PCI_BAR_COUNT)
> > +	if (bar >= PCI_STD_NUM_BARS || !pci_resource_len(pdev, bar))
> >  		return NULL;
> 
> This looks like a latent bug fix here. If 'bar' is out of range we return
> NULL instead accessing an invalid item of an array. Should this not be
> a separate patch and tagged as stable?

Sharp eyes!  I didn't think of this as accessing an invalid item, but
indeed it does (if 'bar' is out of range).  But I doubt it's worth the
hassle of a separate patch, since we return failure anyway.