linux-kernel - Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZeY6Zvo5TZABhWvF@lizhi-Precision-Tower-5810>
Date: Mon, 4 Mar 2024 16:17:26 -0500
From: Frank Li <Frank.li@....com>
To: Niklas Cassel <Niklas.Cassel@....com>
Cc: Manivannan Sadhasivam <mani@...nel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	Jingoo Han <jingoohan1@...il.com>,
	Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
	Jon Mason <jdmason@...zu.us>,
	"open list:PCI DRIVER FOR SYNOPSYS DESIGNWARE" <linux-pci@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root
 complex reinit endpoint

On Mon, Mar 04, 2024 at 09:04:30PM +0000, Niklas Cassel wrote:
> On Mon, Mar 04, 2024 at 02:13:27PM -0500, Frank Li wrote:
> > > 
> > > Niklas's initial suggestion of keeping u8 for the array and 0 as the unallocated
> > > placeholder sounds good to me. Please use that instead.
> > > 
> > 
> > It is impossible to keep u8, because 255 + 1 will 0 for u8. Previously
> > Niklas's initial suggestion have not consider this condition. If u8 have to
> > change to u16 or s16 anyways, I prefer use -1 as free.
> 
> Well, to be fair, my suggestion was:
> "If we continue to use a u8, and offset the saved value by one,
> we will at least be able to support 255-1 == 254 iATUs."
> 
> But we have this define:
> drivers/pci/controller/dwc/pcie-designware.h:#define MAX_IATU_IN 256
> (Even if it isn't used anywhere.)
> 
> But as ridiculous as it may seem to support that many inbound ranges,
> that is the max number of windows supported by the hardware, so why
> not just let the driver support the max supported by the hardware?
> 
> 
> We are talking about:
> struct dw_pcie_ep {
> 	...
>         u8                      bar_to_atu[PCI_STD_NUM_BARS];
> 	...
> }
> 
> where PCI_STD_NUM_BARS == 6.
> 
> And where struct dw_pcie_ep is kzalloced for what I assume is all drivers.
> 
> So I'm actually for your idea of changing it to u16, or even unsigned int.
> 
> If the code is simplified if we use a u16 or unsigned int (because we don't
> need any weird if (val < MAX_IATU_IN - 1) check), then I'm all for it.
> 
> 
> What I personally don't like with your patch in $subject,
> was that you changed both dw_pcie_ep_clear_bar() to set the "clear value"
> to -1, but you also need a
> memset(ep->bar_to_atu, -1, sizeof(ep->bar_to_atu)); in dw_pcie_ep_init().
> 
> 
> I much prefer to have 0 as the default/unused value, because it will
> automatically get set when you do kzalloc().
> Seeing a memset -1 just looks a bit hackish to be, but I realize that
> it is personal preference.
> 
> 
> If it is super important to save 8 bytes from the heap, then I would
> even prefer changing the MAX_IATU_IN 256 to something smaller, like
> 127 or whatever, just as long as we don't need that extra memset -1 :)

Okay, Let me work on '0' version.

Frank

> 
> 
> Kind regards,
> Niklas