linux-kernel - Re: [PATCH v8 3/6] PCI: endpoint: Add pci_epf_align

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z0WWjiamjkWfQXKk@ryzen>
Date: Tue, 26 Nov 2024 10:36:14 +0100
From: Niklas Cassel <cassel@...nel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: Frank Li <Frank.li@....com>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>, Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	imx@...ts.linux.dev, dlemoal@...nel.org, maz@...nel.org,
	tglx@...utronix.de, jdmason@...zu.us
Subject: Re: [PATCH v8 3/6] PCI: endpoint: Add pci_epf_align_addr() helper
 for address alignment

On Tue, Nov 26, 2024 at 09:49:03AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Nov 25, 2024 at 02:22:23PM -0500, Frank Li wrote:
> > On Sun, Nov 24, 2024 at 01:02:39PM +0530, Manivannan Sadhasivam wrote:
> > > On Sat, Nov 16, 2024 at 09:40:43AM -0500, Frank Li wrote:
> > > > Introduce the helper function pci_epf_align_addr() to adjust addresses
> > >
> > > pci_epf_align_inbound_addr()?
> > >
> > > > according to PCI BAR alignment requirements, converting addresses into base
> > > > and offset values.
> > > >
> > > > Signed-off-by: Frank Li <Frank.Li@....com>
> > > > ---
> > > > change from v7 to v8
> > > > - change name to pci_epf_align_inbound_addr()
> > > > - update comment said only need for memory, which not allocated by
> > > > pci_epf_alloc_space().
> > > >
> > > > change from v6 to v7
> > > > - new patch
> > > > ---
> > > >  drivers/pci/endpoint/pci-epf-core.c | 45 +++++++++++++++++++++++++++++++++++++
> > > >  include/linux/pci-epf.h             | 14 ++++++++++++
> > > >  2 files changed, 59 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > > > index 8fa2797d4169a..4dfc218ebe20b 100644
> > > > --- a/drivers/pci/endpoint/pci-epf-core.c
> > > > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > > > @@ -464,6 +464,51 @@ struct pci_epf *pci_epf_create(const char *name)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(pci_epf_create);
> > > >
> > > > +/**
> > > > + * pci_epf_align_inbound_addr() - Get base address and offset that match bar's
> > >
> > > BAR's
> > >
> > > > + *			  alignment requirement
> > > > + * @epf: the EPF device
> > > > + * @addr: the address of the memory
> > > > + * @bar: the BAR number corresponding to map addr
> > > > + * @base: return base address, which match BAR's alignment requirement, nothing
> > > > + *	  return if NULL
> > >
> > > Below, you are updating 'base' only if it is not NULL. Why would anyone call
> > > this API with 'base' and 'offset' set to NULL?
> > 
> > Some time, they may just want one of two.
> > 
> 
> What would be the purpose? I fail to see it.

Currently, the only user of this function is the call:
ret = pci_epf_align_inbound_addr_lo_hi(epf, bar, msg->address_lo, msg->address_hi,
				       &db_bar.phys_addr, &offset);

Which doesn't send in NULL as either 'base' or 'offset', so these NULL
checks do currently look meaningless to me. I suggest to just kill them.


Kind regards,
Niklas