Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Fri, 3 Jun 2022 18:58:56 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Mohamed Khalfella <mkhalfella@purestorage.com>
Cc:     linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        msaggi@purestorage.com, ebadger@purestorage.com,
        stable@vger.kernel.org, bhelgaas@google.com, oohall@gmail.com,
        rajatja@google.com, linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH] PCI/AER: Iterate over error counters instead of error
Message-ID: <20220603235856.GA117911@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220603221247.5118-1-mkhalfella@purestorage.com>
Precedence: bulk

On Fri, Jun 03, 2022 at 10:12:47PM +0000, Mohamed Khalfella wrote:
> Is there any chance for this to land in 5.19?

Too late for v5.19, since the merge window will end in a couple days.
Remind me again if you don't see it in -next by v5.20-rc5 or so.

> On 5/10/22 14:17, Mohamed Khalfella wrote:
> > > Thanks for catching this; it definitely looks like a real issue!  I
> > > guess you're probably seeing junk in the sysfs files?
> > 
> > That is correct. The initial report was seeing junk when reading sysfs
> > files. As descibed, this is happening because we reading data past the
> > end of the stats counters array.
> > 
> > 
> > > I think maybe we should populate the currently NULL entries in the
> > > string[] arrays and simplify the code here, e.g.,
> > > 
> > > static const char *aer_correctable_error_string[] = {
> > >        "RxErr",                        /* Bit Position 0       */
> > >        "dev_cor_errs_bit[1]",
> > >	...
> > >
> > >  if (stats[i])
> > >    len += sysfs_emit_at(buf, len, "%s %llu\n", strings_array[i], stats[i]);
> > 
> > Doing it this way will change the output format. In this case we will show
> > stats only if their value is greater than zero. The current code shows all the
> > stats those have names (regardless of their value) plus those have non-zero
> > values.
> > 
> > >> @@ -1342,6 +1342,11 @@ static int aer_probe(struct pcie_device *dev)
> > >>  	struct device *device = &dev->device;
> > >>  	struct pci_dev *port = dev->port;
> > >>
> > >> +	BUILD_BUG_ON(ARRAY_SIZE(aer_correctable_error_string) <
> > >> +		     AER_MAX_TYPEOF_COR_ERRS);
> > >> +	BUILD_BUG_ON(ARRAY_SIZE(aer_uncorrectable_error_string) <
> > >> +		     AER_MAX_TYPEOF_UNCOR_ERRS);
> > >
> > > And make these check for "!=" instead of "<".
> 
> I am happy to remove these BUILD_BUG_ON() if you think it is a good
> idea to do so.

I think it's good to enforce correctness there somehow, so let's leave
them there unless somebody has a better idea.

> > This will require unnecessarily extending stats arrays to have 32 entries
> > in order to match names arrays. If you don't feel strogly about changing
> > "<" to "!=", I prefer to keep the code as it is.